voila-dashboards / voila

Voilà turns Jupyter notebooks into standalone web applications
https://voila.readthedocs.io
Other
5.31k stars 497 forks source link

Fix Voila IFrame renderer #1469

Closed trungleduc closed 1 month ago

trungleduc commented 1 month ago

References

Closes #1397

voila-pdf

Code changes

User-facing changes

Backwards-incompatible changes

github-actions[bot] commented 1 month ago

Binder :point_left: Launch a Binder on branch trungleduc/voila/fix-iframe

trungleduc commented 1 month ago

Failed OSX tests are not releated

trungleduc commented 1 month ago

cc @jgunstone

jgunstone commented 1 month ago

Hi @trungleduc - thanks for the mention - tried to test with the Binder link but is doesn't seem to be working... error.txt

trungleduc commented 1 month ago

Hi @trungleduc - thanks for the mention - tried to test with the Binder link but is doesn't seem to be working... error.txt

the binder link does not work anymore, we should update if in another PR

martinRenou commented 1 month ago

It allows disabling the sandbox option of the Voila iframe, it helps Chrome to render blocked contents like the pdf files.

What would be the impact of disabling the sandboxing by default?

trungleduc commented 1 month ago

It allows disabling the sandbox option of the Voila iframe, it helps Chrome to render blocked contents like the pdf files.

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.

jgunstone commented 1 month ago

is the "disable IFrame sandbox" a config variable? the app users won't know what that means so we'll need to be able to pre-confgiure it to ensure the pdf viewing just works from a user POV

trungleduc commented 1 month ago

is the "disable IFrame sandbox" a config variable? the app users won't know what that means so we'll need to be able to pre-confgiure it to ensure the pdf viewing just works from a user POV

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).

jgunstone commented 1 month ago

hmmm ok... is there any other way to preview a pdf? being able to preview a pdf from within an app using Chrome doesn't feel that niche...

trungleduc commented 1 month ago

hmmm ok... is there any other way to preview a pdf? being able to preview a pdf from within an app using Chrome doesn't feel that niche...

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

martinRenou commented 1 month ago

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?

jgunstone commented 1 month ago

hmmm ok... is there any other way to preview a pdf? being able to preview a pdf from within an app using Chrome doesn't feel that niche...

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 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...

trungleduc commented 1 month ago

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.

jgunstone commented 1 month ago

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...

martinRenou commented 1 month ago

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 this.content.node.querySelector('iframe').removeAttribute('sandbox') during the iframe widget creation.

What do you think @trungleduc? Could you make that change?

trungleduc commented 1 month ago

We're not sandboxing the iframe containing the PDF in the case of the Notebook

Yes, JupyterLab does, and it's not about sandboxing the pdf iframe, but the voila preview iframe which causes the issue

martinRenou commented 1 month ago

Yes, JupyterLab does

No it does not :)

Screenshot from 2024-05-22 14-35-34

but the voila preview iframe which causes the issue

Yeah I understand, that's why I think we should remove the sandbox on the voila-preview iframe.

trungleduc commented 1 month ago

Cool, I will do it

jgunstone commented 1 month ago

thanks @trungleduc