Skip to content
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

Merged
merged 6 commits into from
May 23, 2024
Merged

Conversation

trungleduc
Copy link
Member

@trungleduc trungleduc commented May 14, 2024

References

Closes #1397

voila-pdf

Code changes

User-facing changes

  • The preview extension now supports reading configuration files (voila.json, voila.py) like the voila standalone app.
  • A new checkbox is added to the extension toolbar. It allows disabling the sandbox option of the Voila iframe, it helps Chrome to render blocked contents like the pdf files.

Backwards-incompatible changes

Copy link
Contributor

Binder 👈 Launch a Binder on branch trungleduc/voila/fix-iframe

@trungleduc trungleduc added the bug Something isn't working label May 14, 2024
@trungleduc trungleduc marked this pull request as ready for review May 14, 2024 16:58
@trungleduc
Copy link
Member Author

Failed OSX tests are not releated

@trungleduc
Copy link
Member Author

cc @jgunstone

@jgunstone
Copy link

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

@trungleduc
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

trungleduc commented May 15, 2024

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
Copy link

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
Copy link
Member Author

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
Copy link

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
Copy link
Member Author

trungleduc commented May 15, 2024

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
Copy link
Member

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
Copy link

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
Copy link
Member Author

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
Copy link

jgunstone commented May 15, 2024

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
Copy link
Member

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
Copy link
Member Author

trungleduc commented May 22, 2024

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
Copy link
Member

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
Copy link
Member Author

Cool, I will do it

Copy link
Member

@martinRenou martinRenou left a 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

@martinRenou martinRenou merged commit 77d1b23 into voila-dashboards:main May 23, 2024
23 of 25 checks passed
@jgunstone
Copy link

thanks @trungleduc

@trungleduc trungleduc deleted the fix-iframe branch May 27, 2024 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

403 Forbidden - PDF IFrame Issue
3 participants