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

PDF file preview #6941

Open
wants to merge 2 commits into
base: v5/develop
Choose a base branch
from
Open

Conversation

distantnative
Copy link
Member

@distantnative distantnative commented Jan 23, 2025

Description

I think this could be a neat addition for the core. With fallback in case inline PDF viewing isn't supported.

Or if not core, we could release it as small plugin.

Changelog

Feature

  • PDF file preview

Ready?

  • In-code documentation (wherever needed)
  • Unit tests for fixed bug/feature
  • Tests and CI checks all pass

For review team

  • Add lab and/or sandbox examples (wherever helpful)
  • Add changes & docs to release notes draft in Notion

@distantnative distantnative self-assigned this Jan 23, 2025
@distantnative distantnative added this to the 5.0.0-beta.3 milestone Jan 23, 2025
@distantnative distantnative added the needs: discussion 🗣 Requires further discussion to proceed label Jan 23, 2025
@bastianallgeier
Copy link
Member

This is great. The fallback is super elegant. I would only be worried if loading PDFs could lead to any new JS security issues. I've read somewhere that PDFs have optional JS support, which is pretty crazy. But I'm not aware if this could be exploited or not. Maybe @lukasbestle has some additional thoughts?

@distantnative
Copy link
Member Author

mdn has a section on security concerns with frames, but uses object as example for PDFs without any further security mentions: https://developer.mozilla.org/en-US/docs/Learn_web_development/Core/Structuring_content/General_embedding_technologies#the_embed_and_object_elements

@distantnative distantnative marked this pull request as ready for review January 24, 2025 20:42
@distantnative distantnative removed the needs: discussion 🗣 Requires further discussion to proceed label Jan 24, 2025
@lukasbestle
Copy link
Member

lukasbestle commented Jan 24, 2025

PDFs do have JS support, but I would assume that when embedding a PDF in <object>, browsers sandbox the embed similar to when using the <img> element with an SVG. So the JS code won't have access to the running Panel instance. However I don't have experience with the <object> element in a security context.

@lukasbestle
Copy link
Member

Actually not a very good comparison as SVGs in <img> just don't execute JS at all, but what I wanted to say is that embeds with <object> should not be able to interact with the page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants