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

Use the Window's associated Document for allow-modals sandbox checks #1473

Merged
merged 1 commit into from
Jul 13, 2016

Conversation

domenic
Copy link
Member

@domenic domenic commented Jun 29, 2016

Use the Window's associated Document for allow-modals sandbox checks

This fixes #1206. As noted there, the previous indirections were wacky
and unnecessary.

This matches the behavior as implemented in Chrome and Firefox Nightly
(which are the only browsers which implement sandboxing of modals), as
shown by https://allow-modals-check-yucxowumar.now.sh/entry.html.

This also helps the efforts in #1430.

@bzbarsky to review since he raised #1206 initially. /cc @mikewest.

@bzbarsky
Copy link
Contributor

I won't have time to really look into this until I get back from vacation in a week, but have you checked what non-Chrome browsers do here?

@domenic
Copy link
Member Author

domenic commented Jun 29, 2016

Ah, I kind of assumed they didn't implement based on your comments in #1430. Let me expand the test case to see if that's actually true. I will update the OP if nothing contradicts the earlier results, or post a new comment if we end up with contradictory results.

@domenic domenic force-pushed the allow-modals-unwacky branch 3 times, most recently from b9f5663 to 47b9152 Compare June 29, 2016 21:47
This fixes #1206. As noted there, the previous indirections were wacky
and unnecessary.

This matches the behavior as implemented in Chrome and Firefox Nightly
(which are the only browsers which implement sandboxing of modals), as
shown by https://allow-modals-check-yucxowumar.now.sh/entry.html.

This also helps the efforts in #1430.
@bzbarsky
Copy link
Contributor

bzbarsky commented Jul 7, 2016

This matches the behavior as implemented in Chrome and Firefox Nightly

I am 100% sure it does not match the behavior in Firefox nightly. Given that, I have low confidence that it matches Chrome either, because your testcase did not detect the difference between what you think Firefox nightly does and what Firefox nightly actually does....

What Firefox nightly implements pending this spec being sorted out is that we use the sandbox flags of the active document of the browsing context. Yes, this is dumb. I wasn't going to put in the effort to do something more complicated (in our codebase, it so happens that the simplest place to gate this was on the browsing context, not the window) until we'd decided on an actual spec and had some commitments from others to implementing it.

So given all that, what is the actual Chrome behavior here? Is it in fact to use the Window's most recent document, or something else? Have you written a testcase to tell apart "Window's most recent document" and "browsing context's active document"?

@domenic
Copy link
Member Author

domenic commented Jul 7, 2016

Thanks for reviewing. Clearly I have not written a test case distinguishing that, and overstated my case. I wrote a test case distinguishing between the entry, incumbent, current, and relevant window's most recent documents, but did not add more exotic variations like the 4 other variations possible by replacing "window's most recent document" with "window's browsing context's active document". I can do so later, if you think it's possibly a better semantic.

@bzbarsky
Copy link
Contributor

bzbarsky commented Jul 7, 2016

I think it's a daft semantic. I just want to know exactly what Chrome's behavior actually is and is planned to be, because what I don't want is for the spec to say X, Firefox to ship X, Chrome to ship Y and then end up with the spec changing to Y later like tends to happen.

@domenic
Copy link
Member Author

domenic commented Jul 7, 2016

By code inspection it looks like Chrome does implement the daft semantic: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp?q=allow-modals&sq=package:chromium&dr=C&l=789

@mikewest, thoughts on updating that and its friends from frame()->document() to just document()?

@domenic
Copy link
Member Author

domenic commented Jul 13, 2016

@bzbarsky, can you help clarify when it is actually possible for window.alert() and friends to be called when the Window's associated Document is not equal to the Window's browsing context's active document? The only case I could think of is when you are on a navigated-away-from document, but I wasn't able to stretch my imagination enough to figure out how to perform an alert on such a document, given that the WindowProxy will change which Window it points to. (That is, I can't figure out how to cause this difference to appear for window.* APIs; document.* APIs I could do.)

When I talked about navigated away from documents with @mikewest, he said "Can we just not trigger modal dialogs in that state at all?" which seems also pretty reasonable (albeit a somewhat separate conversation).

@bzbarsky
Copy link
Contributor

Consider this:

function f() {
  alert('hey');
}

and now someone grabs a reference to that function, navigates away from the document/window the function came from, then calls the function.

There is no obvious windowproxy involved here afaict, but the actual behavior here (that is, how the actual Window is procured by the function) is rather underspecified at the moment. In Gecko, it's procured by seeing that this is undefined and setting it to the current global of the alert function, which is a Window, not a WindowProxy.

For what it's worth, if you try my testcase in Gecko, you will get a security exception. For certain methods on window (and alert is one of them) we will just throw if the this is not the current Window. In general, this is for methods that involve the browsing context in some way.

@domenic
Copy link
Member Author

domenic commented Jul 13, 2016

Ah, right, the good old bare global reference trick -_-. Thanks.

For what it's worth, if you try my testcase in Gecko, you will get a security exception. For certain methods on window (and alert is one of them) we will just throw if the this is not the current Window. In general, this is for methods that involve the browsing context in some way.

I've put the test case up at https://allow-modals-navigation-wbeliirtbv.now.sh. Both Firefox and Edge disallow this, and both Chrome and Safari TP allow it. However, it sounds like @mikewest might be interested in disallowing it in Chrome too, in which case we could consider a spec change to add a check that the document of the window on which alert is being invoked is active. (Fully active?)

What would you advise on the best route here? I see as possibilities:

  • Spec window's browsing context's active document, as Chrome and Firefox implement.
  • Spec window's most recent document, which is apparently indistinguishable from what Firefox implements due to the added security check.
  • Block on this until we can further consider adding an active document check to all the modal dialog methods.

@bzbarsky
Copy link
Contributor

I would be quite happy to spec it as window's most recent document and work on adding that active document check in a separate issue.

@domenic
Copy link
Member Author

domenic commented Jul 13, 2016

OK, great! I filed #1548 as the spin-off issue. With that in place, the commit message on this pull request needs updating, but I think the contents are good. With the following updated commit message, do the contents LGTY?

Use the Window's associated Document for allow-modals sandbox checks

This fixes #1206. As noted there, the previous indirections were wacky
and unnecessary.

Chrome and Firefox nightly implement a slightly different behavior,
choosing the Window's browsing context's active Document, instead of the
Window's associated Document. This can be different in edge cases as
discussed in https://github.com/whatwg/html/pull/1473#issuecomment-232441391.
However, in Firefox they are not distinguishable, due to another
security check that prevents these edge cases from being triggered.
#1548 proposes to add a security check like Firefox's to the spec, which
would obviate the difference in all cases. In the meantime, the choice
of associated Document is more straightforward.

@bzbarsky
Copy link
Contributor

Yep, that sounds good. Thanks!

@domenic domenic merged commit 29ebd5b into master Jul 13, 2016
@domenic domenic deleted the allow-modals-unwacky branch July 13, 2016 21:37
alice pushed a commit to alice/html that referenced this pull request Jan 8, 2019
This fixes whatwg#1206. As noted there, the previous indirections were wacky
and unnecessary.

Chrome and Firefox nightly implement a slightly different behavior,
choosing the Window's browsing context's active Document, instead of the
Window's associated Document. This can be different in edge cases as
discussed in whatwg#1473 (comment).
However, in Firefox they are not distinguishable, due to another
security check that prevents these edge cases from being triggered.
whatwg#1548 proposes to add a security check like Firefox's to the spec, which
would obviate the difference in all cases. In the meantime, the choice
of associated Document is more straightforward.
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.

allow-modals usage of incumbent globals is wacky
3 participants