-
Notifications
You must be signed in to change notification settings - Fork 505
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Voila IFrame renderer #1469
Conversation
Failed OSX tests are not releated |
cc @jgunstone |
Hi @trungleduc - thanks for the mention - |
the binder link does not work anymore, we should update if in another PR |
What would be the impact of disabling the sandboxing by default? |
The only situation in which users need to disable the sandboxing is having a nested iframe of rich content in Chrome. It works well in other browsers without disabling the sandbox option. So I don't think disabling the sandbox by default is worth it. |
is the "disable IFrame sandbox" a config variable? |
not yet, it's possible but will need much more work for a very niche case (using iframe to open a pdf file in the preview extension while using Chrome). |
hmmm ok... is there any other way to preview a pdf? |
Yeah, but in the case of the voila preview extension, it's already an Iframe showing the Voila app. Then voila tries to display a nested IFrame containing the pdf file. The standalone Voila app works well with this use case |
If removing the sandboxing is one click away from the user and that is fine security-wise, I'd vote for disabling it by default. If there is a security issue we should not allow disabling it whatsoever. But I feel like having an halfway-secure solution where the user can disable sandboxing is not really a good design? |
if the extension preview requires a user click but the standard voila app will now work fine then that satisfies my use cases. The most important thing that a standard voila app is able to preview pdfs. r.e. the voila preview - I take @martinRenou 's point about halfway-secure feeling a little ropey... |
the standalone Voila app works well with the IFrame Pdf. Only the preview extension is having the issue in Chrome. I'm more than happy to remove the sandboxing hack. |
I don't have a strong preference r.e. sandboxing... if it is removed might be worth adding a note to say that IFrame displaying pdf's doesn't work in the voila-preview as that would be an easy rabit hole to fall down for an unsuspecting user... |
We're not sandboxing the iframe containing the PDF in the case of the Notebook, so let's not sandbox the iframe in the case of the voila-preview. We can run What do you think @trungleduc? Could you make that change? |
Yes, JupyterLab does, and it's not about sandboxing the pdf iframe, but the voila preview iframe which causes the issue |
Cool, I will do it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks a lot @trungleduc
thanks @trungleduc |
References
Closes #1397
Code changes
User-facing changes
Backwards-incompatible changes