Add cross-origin opener policy #5334
Conversation
There was a problem hiding this comment.
I think the structure here generally makes sense. I've added some comments (mostly nits), but I think the algorithms generally match what I expected this to look like. I think it'd be helpful for @annevk to hop in to verify that the structure matches his expectations as well. If that's the case, we can work through the rest of the nits and indentation and what should link to where and etc. :)
Thanks!
There was a problem hiding this comment.
Thanks, @camillelamy! I skimmed through the rewrite, and though I have nits and details, I'm still generally happy. I'd like @annevk to sign off on the general structure (or propose a different way of tackling the problem!) before going through them, just so I don't spend your time on something you'll need to rework more completely later. :)
There was a problem hiding this comment.
Thanks for taking this on @camillelamy. I need to have another look at the generated HTML as that usually helps with blind spots, but it appears to be in pretty good shape. Left a bunch of inline comments and am curious to hear your thoughts on them.
There was a problem hiding this comment.
Regarding the timing between browsing context switch and document creation, my understanding is that document creation happens on step 6 of https://html.spec.whatwg.org/multipage/browsing-the-web.html#initialise-the-document-object. The spec PR adds the browsing context switch as step 4 of this algorithm. So for me we do the browsing context switch before we create the new document for the navigation.
There was a problem hiding this comment.
Ah yeah, I think you're right. Okay, I think this is starting to look pretty good. It needs some more formatting changes (I can help with those) and I wonder if we should add some examples and a bit more introductory text here and there.
It would also be great if @domenic or someone that's not me could have another detailed look to grow the set of people familiar with this and also to ensure I'm not overlooking things as I wrote the Markdown draft of this.
camillelamy
force-pushed
the
issue-4580
branch
2 times, most recently
from
c0c121f
to
6b15e55
Compare
|
I pushed a commit that changed the cross-origin opener policy value explanations and also did some editorial changes. Things that remain:
I suspect I need at least one more pass after that and @domenic should maybe review as well, but things are looking pretty good to me. (Feel free to make changes as subsequent commits by the way. We'll squash it all together when everyone is satisfied.) |
|
Thanks for your review Anne! I have pushed a new version that:
Thanks! |
There was a problem hiding this comment.
Alright, I have a couple more nits, but at this point I'd really like for @domenic and @mikewest to do a detailed review as I'm afraid I'm missing things due to having written the original text.
changes the incumbent variable names - to make sure I understand correctly, in the navigation algorithm incumbentNavigationOrigin is the current origin and activeDocumentNavigationOrigin is the origin we are navigating to?
I had another look at this and I don't think that's correct. incumbentNavigationOrigin seems to be the origin of the incumbent settings object (one of the parties responsible for making the navigation happen) and activeDocumentNavigationOrigin seems to be the origin of the document currently in the browsing context that is being navigated.
|
@annevk and @domenic : the latest version should now plumb the variables correctly in the various processing model algorithm. However, for calls to process a navigation response that happens in the navigate algorithm I am not quite sure what to do with some of the variables (in the special cases like JavaScript). |
Contrary to the current explanation, when an opener specifies a COOP of `same-origin-allow-popups` and the opened resource specifies `unsafe-none`, then the two documents *will* share the same browsing context group. This is supported by the explanation of `same-origin-allow-popups` which precedes this section: > A top-level document with `same-origin-allow-popups` retains references to > any of its popups which either don't set COOP or which opt out of isolation > by setting a COOP of `unsafe-none`. It is also supported by the proposed specification text [1] which reads: > To check if a response requires a browsing context group switch , > given a browsing context browsingContext , an origin responseOrigin > and a cross-origin opener policy responseCOOP , run the followign > steps: > > [...] > > 6. If all of the following are true: > > - isInitialAboutBlank > - activeDocumentCOOP is " same-origin-allow-popups ". > - responseCOOP is " unsafe-none ". > > then return false. Update the explanation to only include the relevant condition (that is: a COOP of `same-origin`). [1] whatwg/html#5334
Contrary to the current explanation, when an opener specifies a COOP of `same-origin-allow-popups` and the opened resource specifies `unsafe-none`, then the two documents *will* share the same browsing context group. This is supported by the explanation of `same-origin-allow-popups` which precedes this section: > A top-level document with `same-origin-allow-popups` retains references to > any of its popups which either don't set COOP or which opt out of isolation > by setting a COOP of `unsafe-none`. It is also supported by the proposed specification text [1] which reads: > To check if a response requires a browsing context group switch , > given a browsing context browsingContext , an origin responseOrigin > and a cross-origin opener policy responseCOOP , run the followign > steps: > > [...] > > 6. If all of the following are true: > > - isInitialAboutBlank > - activeDocumentCOOP is " same-origin-allow-popups ". > - responseCOOP is " unsafe-none ". > > then return false. Update the explanation to only include the relevant condition (that is: a COOP of `same-origin`). [1] whatwg/html#5334
|
I think this is wrong for navigating to a response (initial fetching of For |
|
For the response case, do you think we should do something like: |
|
I think only a nested document can be generated by navigating to a response and if that's true COOP doesn't really apply there, right? |
|
I have updated the PR for the navigation to a response case. Now we assert that we are not in a top-level browsing context and pass an unsafe-none COOP. |
|
Looking at various algorithms such as "process a navigate fetch" and its callers, I think that #4664 and #5098 ended up not updating various callers for these algorithms and sometimes even not update the signatures for these algorithms. And untangling this is tricky as it's not always clear what to pass. I guess I'll try to fix what I can and insert some Edit: this might be smaller than I thought, though it's hard to say for sure. |
| <li> | ||
| <p><span data-x="a browsing context is discarded">Discard</span> <var>browsingContext</var>.</p> | ||
|
|
||
| <p class="note">This does not close <var>browsingContext</var>'s <span data-x="tlbc |
There was a problem hiding this comment.
Is the concept of "closing" a BCG well-defined? If so, could we link to it? If not, maybe there's a better way of phrasing this?
There was a problem hiding this comment.
No it's not defined. I have rephrased the comment, hopefully it's better.
|
|
||
| <p class="XXX">The impact of swapping browsing context groups following a navigation is not | ||
| defined. It is currently under discussion in <a | ||
| href="https://github.com/whatwg/html/issues/5350">issue #5350</a>.</p> |
There was a problem hiding this comment.
Maybe "not fully defined"? After all, there are a lot of effects already defined, simply because the spec defines how different BCGs can't reach each other.
| <li> | ||
| <p>If <var>sandboxFlags</var> is not empty and <var>responseCOOP</var> is not "<code | ||
| data-x="coop-unsafe-none">unsafe-none</code>", then set <var>response</var> to an | ||
| appropriate <span>network error</span> and return.</p> |
There was a problem hiding this comment.
This should not return from the entire algorithm, right? It should just break from the loop. Which makes step 8 below unnecessary? I will push a fix, but please confirm.
There was a problem hiding this comment.
No it should return from the algorithm. At this point, we have found that we cannot load the page and will display an error page instead. There is no need to do a BCG switch for this error page, hence the early return.
There was a problem hiding this comment.
Displaying an error page requires continuing the algorithm; that's done in step 1 of "process a navigate response".
There was a problem hiding this comment.
You are correct. Sorry I got mixed up with the previous version where this was in a separate algorithm.
|
|
||
| <li> | ||
| <p>If <var>sandboxFlags</var> is not empty and <var>responseCOOP</var> is not "<code | ||
| data-x="coop-unsafe-none">unsafe-none</code>", then set <var>response</var> to an |
There was a problem hiding this comment.
Your IDE is introducing tabs, it seems; I'll fix.
There was a problem hiding this comment.
I am not seeing the tabs in the latest version of the file, so maybe you fixed this already?
Fixes whatwg#3740. Closes whatwg#4580. Need to check again if it closes them: * whatwg#4921 * whatwg#5168 * whatwg#5172 * whatwg#5198 (probably not?) Co-authored-by: Anne van Kesteren <[email protected]>
| <li> | ||
| <p><span data-x="a browsing context is discarded">Discard</span> <var>browsingContext</var>.</p> | ||
|
|
||
| <p class="note">This does not close <var>browsingContext</var>'s <span data-x="tlbc |
There was a problem hiding this comment.
No it's not defined. I have rephrased the comment, hopefully it's better.
|
|
||
| <p class="XXX">The impact of swapping browsing context groups following a navigation is not | ||
| defined. It is currently under discussion in <a | ||
| href="https://github.com/whatwg/html/issues/5350">issue #5350</a>.</p> |
| <li> | ||
| <p>If <var>sandboxFlags</var> is not empty and <var>responseCOOP</var> is not "<code | ||
| data-x="coop-unsafe-none">unsafe-none</code>", then set <var>response</var> to an | ||
| appropriate <span>network error</span> and return.</p> |
There was a problem hiding this comment.
You are correct. Sorry I got mixed up with the previous version where this was in a separate algorithm.
|
|
||
| <li> | ||
| <p>If <var>sandboxFlags</var> is not empty and <var>responseCOOP</var> is not "<code | ||
| data-x="coop-unsafe-none">unsafe-none</code>", then set <var>response</var> to an |
There was a problem hiding this comment.
I am not seeing the tabs in the latest version of the file, so maybe you fixed this already?
|
I noticed that (I'm pretty sure I left this comment earlier, but it seems to have gotten lost.) The other thing I realized thanks to Domenic's secure context work is that Chrome probably wants to also check the HTTPS state on the response and not support this feature when it's "deprecated" as that's technically not a secure context. I would be okay with landing this and having follow-up issues though as it's rather tricky to keep reviewing this over a lot of rebasing. Especially now that PR Preview is broken. |
|
Yay! I merged the result in c9fddd7. From what I can tell we closed #3740, #4580, #4921, and #5172. #5168 might be closed, but I'm not sure; certainly we didn't do what #5168 (comment) suggests. #5198 is probably not closed. Please check this list carefully, reopening or closing as necessary. I opened #5669 and #5670 for the followups @annevk mentions in his previous comment. I am tackling #5669. |
| @@ -81727,10 +82030,17 @@ interface <dfn>Location</dfn> { // but see also <a href="#the-location-interface | |||
| a <code>javascript:</code> URL request</span> given <var>resource</var>, the <span>source | |||
| browsing context</span>, and <var>browsingContext</var>.</p></li> | |||
|
|
|||
| <li><p>Let <var>finalSandboxFlags</var> be the union of <var>browsingContext</var>'s | |||
| <span>sandboxing flag set</span> and <var>response</var>'s <span>forced sandboxing flag | |||
| set</span>.</p></li> | |||
There was a problem hiding this comment.
How can response's forced sandboxing flag set be controlled in this case? I first thought it was via CSP directive, but after further research, that no longer seems possible (the header list contains only Content-Type and Referrer-Policy, and the sandbox directive is ignored if specified within a <meta> element).
There was a problem hiding this comment.
I would not expect what the specification states around javascript URLs to be particularly reliable. There are still many unresolved issues there, many of them reported against this repository.
There was a problem hiding this comment.
My interest is in writing tests, so I'm just as curious about what was intended by this change as what it literally means today. If the spec mechanics are incomplete, "tentative" tests would help communicate the intention and (via wpt.fyi) document current implementation status.
There was a problem hiding this comment.
I think the intent is that whatever browsing context is navigated to a javascript URL, its current active document (i.e., at the start of the navigation) doesn't have most of its policy state change. I think this particular step is probably a no-op.
There was a problem hiding this comment.
Thanks, Anne! Although I was initially focused on "response's forced sandboxing flag set", that's only part of the change. The new spec text also introduces a reference to "browsingContext's sandboxing flag set," which doesn't appear to be well-defined. I've opened gh-5682 to learn if this is an oversight or just a misreading on my part.
There was a problem hiding this comment.
I have some issues with this part in Chrome.
Writing the content of the string returned from the javascript: URL evaluation into the document is considered by chrome not to be a cross-document navigation. This looks closer to executing a document.write(..)
Chrome can't implement this correctly, if this isn't considered a navigation. Many inheritance mechanism that applies during a navigation (including sandbox) are missing.
In the recent past:
- I proposed to stop mutating the sandbox flags when doing document.write() in Chrome and converge with Firefox:
https://bugs.chromium.org/p/chromium/issues/detail?id=1186311&q=reporter%3Ame&can=2 - I also patched Chrome to stop mutating sandbox when executing javascript: URL:
https://chromium-review.googlesource.com/c/chromium/src/+/2577211
At this point, I believed we were converging with Firefox, since Firefox was passing the new WPT
I believe Firefox behavior doesn't depends on the sandbox of the initiator of document.write/javascript, but still apply the new sandbox_flags of the iframe to the document, resulting in a sandbox_flags mutation.
For instance:
let empty_iframe = document.createElement("iframe");
document.body.appendChild(empty_iframe);
// The initial empty document committed.
empty_iframe.sandbox = "allow-xxx"
// The sandbox flags of the frame have changed, they will be applied during the next navigation.
empty_iframe.src = "javascript:\"Hello world\"";
// - Chrome consider the previous line not to be a navigation.
// As a result the sandbox flags of empty document haven't changed.
// - Firefox apply the sandbox_flags of the <iframe> into the empty document.There was a problem hiding this comment.
@ArthurSonzogni can you please file that as a new issue so it doesn't get lost? We have a number of issues around javascript: URLs and perhaps now with appHistory and such it's a good time to figure out how to address them.
This commit adds the notion of cross-origin opener policy (COOP). COOP allows websites to restrict which origins they share their browsing context group with. annevk wrote a first draft of the behavior of COOP here: https://gist.github.com/annevk/6f2dd8c79c77123f39797f6bdac43f3e. This commit takes that draft and merges it into the spec.
Closes: #4580
/browsers.html ( diff )
/browsing-the-web.html ( diff )
/dom.html ( diff )
/history.html ( diff )
/iana.html ( diff )
/index.html ( diff )
/infrastructure.html ( diff )
/offline.html ( diff )
/origin.html ( diff )
/references.html ( diff )
/webappapis.html ( diff )