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

Ensure PDF browsing contexts have no DOM #6947

Closed
wants to merge 1 commit into from

Conversation

domenic
Copy link
Member

@domenic domenic commented Aug 9, 2021

Previously, the spec allowed either the "page load processing model for content that uses plugins" or the "page load processing model for inline content that doesn't have a DOM" to be used for PDFs. The observable difference was that plugin documents could be same-origin to their containing page, allowing the containing page to observe the <embed> element etc.

Since Firefox manages to get away with the "no DOM" path, and that path is simpler, we make it the only allowed path.

This also fixes a few bugs in the no-DOM processing model:

  • If it was reached for the PDF case, there was no check to disallow viewing the PDF in a sandboxed browsing context.
  • It was passing a null request to the Document creation algorithm, which was not allowed.
  • It would lose the history handling behavior, so it would not properly do replace navigations if requested.
  • It would lose the navigation id, so it would not properly signal the results to WebDriver BiDi.
  • It was missing a few other navigation params fields.

(See WHATWG Working Mode: Changes for more details.)


/browsing-the-web.html ( diff )
/index.html ( diff )
/origin.html ( diff )

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tackling this. I'm definitely supportive of not exposing these documents. I suspect it needs to be rebased due to removal of "secured" plugins.

source Show resolved Hide resolved
@annevk annevk added the agenda+ To be discussed at a triage meeting label Aug 30, 2021
Previously, the spec allowed either the "page load processing model for content that uses plugins" or the "page load processing model for inline content that doesn't have a DOM" to be used for PDFs. The observable difference was that plugin documents could be same-origin to their containing page, allowing the containing page to observe the <embed> element etc.

Since Firefox manages to get away with the "no DOM" path, and that path is simpler, we make it the only allowed path.

This also fixes a few bugs in the no-DOM processing model:

* If it was reached for the PDF case, there was no check to disallow viewing the PDF in a sandboxed browsing context.
* It was passing a null request to the Document creation algorithm, which was not allowed.
* It would lose the history handling behavior, so it would not properly do replace navigations if requested.
* It would lose the navigation id, so it would not properly signal the results to WebDriver BiDi.
* It was missing a few other navigation params fields.
@domenic domenic force-pushed the no-visible-embed-for-plugins branch from c743dac to b23a5c9 Compare August 30, 2021 18:52
@past past removed the agenda+ To be discussed at a triage meeting label Sep 3, 2021
@mfreed7
Copy link
Contributor

mfreed7 commented Oct 7, 2021

Sorry for the delay reviewing this. I'm generally supportive of the idea - I think it makes sense to remove the <embed> usage entirely and just have PDFs be inline content (no DOM). I have not had a chance to look into the Blink implementation to see how much of a change that'd be. I do suspect, as you mentioned, that this change should be fairly web compatible, though there might be some corner cases where people are using the knowledge that they can see into the PDF document and do things. But if they did that, I would think they should know they're doing things that might break in the future, and clearly it already wouldn't work on e.g. Firefox.

I've filed this Chromium bug to kick off the implementation question.

@mfreed7
Copy link
Contributor

mfreed7 commented Oct 11, 2021

Quick clarification here. Implementation-wise, if we were to just make <iframe src=myfile.pdf> always cross-origin to the embedding page, that would be new-spec compliant, correct? That appears to be how Mozilla works today.

@domenic
Copy link
Member Author

domenic commented Oct 11, 2021

Yep, that's exactly right.

@mfreed7
Copy link
Contributor

mfreed7 commented Oct 12, 2021

Yep, that's exactly right.

Thanks!

domenic added a commit to web-platform-tests/wpt that referenced this pull request Oct 11, 2022
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Oct 21, 2022
…sible, a=testonly

Automatic update from web-platform-tests
Test that plugin documents are not accessible

Follows whatwg/html#6947.

--

wpt-commits: 3f920a2c943783e2513957fe55cfde412ea603fc
wpt-pr: 29957
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Oct 26, 2022
…sible, a=testonly

Automatic update from web-platform-tests
Test that plugin documents are not accessible

Follows whatwg/html#6947.

--

wpt-commits: 3f920a2c943783e2513957fe55cfde412ea603fc
wpt-pr: 29957
@domenic domenic closed this in 0a97a81 Oct 31, 2022
@annevk annevk deleted the no-visible-embed-for-plugins branch October 31, 2022 06:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants