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

[domenic]

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]

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]

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.

[bzbarsky]

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]

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]

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]

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]

@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]

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]

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]

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]

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]

Yep, that sounds good. Thanks!