Move origin into policy container. (#8302)

[wanderview]

• At least two implementers are interested (and none opposed):
  • …
  • …
• Tests are written and can be reviewed and commented upon at:
  • …
• Implementation bugs are filed:
  • Chromium: …
  • Gecko: …
  • WebKit: …
  • Deno (only for timers, structured clone, base64 utils, channel messaging, module resolution, web workers, and web storage): …
  • Node.js (only for timers, structured clone, base64 utils, channel messaging, and module resolution): …
• MDN issue is filed: …

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

---

/browsers.html ( diff )
/browsing-the-web.html ( diff )
/document-lifecycle.html ( diff )
/document-sequences.html ( diff )
/dom.html ( diff )
/nav-history-apis.html ( diff )
/webappapis.html ( diff )
/workers.html ( diff )
/worklets.html ( diff )

[wanderview]

@annevk can you please take a look at this? It attempts to move origin to the policy container. It removes origin from environment settings object and document. (I have not make a DOM spec PR yet.)

[domfarolino]

Out of curiosity what is the motivation for this change? Is there some background here?

[wanderview]

Out of curiosity what is the motivation for this change? Is there some background here?

See #8302. Basically it was decided this was a prerequisite for spec'ing storage partitioning.

[wanderview]

@annevk Gentle review ping. I'm busing looking at some other things at the moment, but I'd like to avoid having this sit too long to avoid bit rot. Thanks.

[annevk]

Some thoughts:

• This doesn't introduce the authority struct we discussed. It probably should to avoid having to change everything again in a follow-up PR? Or did you have another idea for that?
• To keep the number of changes small and make review easier, we keep document's origin as a concept, but redefine it as returning document's policy container's authority's origin. Eventually we'll have to do replacements, but I suspect we rather want to have "same authority" (or some such) operations that take a document/settings object on one side (or both sides).
• Depending on how many references there are to settings object's origin we do the same there, for now.

[wanderview]

This doesn't introduce the authority struct we discussed. It probably should to avoid having to change everything again in a follow-up PR? Or did you have another idea for that?

You mean create an authority struct with a single item in it for now?

I can do that. My thought was that I could do that in a separate PR to try to keep them smaller and also that it would be weird to have a struct with only one value right now.

[annevk]

That's also fine I suppose. Especially if we keep document's origin and settings object's origin working for now as there should be a lot fewer changes in general then.

[wanderview]

To keep the number of changes small and make review easier, we keep document's origin as a concept, but redefine it as returning document's policy container's authority's origin.

@annevk just to clarify, this means I should remove all changed reference document-concept-origin here and create a new PR against DOM spec to reference the unlanded origin policy container origin?

I feel like there is going to be some state where the specs are inconsistent until we can land both.

[wanderview]

Work-in-progress push so I can use github diff viewing tools.

[wanderview]

I need to write the associated DOM spec pull request now.

[wanderview]

Ok, I have tried creating the corresponding DOM pull request over in whatwg/dom#1142.

@annevk can you please take another look?

[annevk]

the value

[annevk]

Could you address the CI issues? It would be nice to be able to view the rendered HTML.

[annevk]

Looking at the rendered HTML I'm noticing a couple of oddities. I haven't been able to go through all the diffs, but I figured I'd comment here what I found thus far.

[annevk]

Looking at this in context this is weird. This really ought to be part of "determine navigation params policy container".

[wanderview]

I moved setting the origin in the "determine navigation params policy container", but left computing the origin in the caller because its also used in other places by the caller.

Also, I took a bit of a guess that the origin should be unchanged for a history entry policy container.

Let me know what you think.

[domenic]

I spent about 1.5 hours going down this rabbit hole.

I think it's definitely possible to make this easier to read. Now that effectively "determine navigation params policy container" + "determine the origin" are doing one big combined thing, it's pretty bad. They have the combined inputs responseURL, sandboxFlags, sourceOrigin, containerOrigin, historyPolicyContainer, initiatorPolicyContainer, parentPolicyContainer, responsePolicyContainer. Maybe sourceOrigin / containerOrigin /initiatorPolicyContainer / parentPolicyContainer have some redundancy; maybe not.

However, any work here ends up being pretty orthogonal to Ben's PR. I wrote it all out and it consists of a few steps:

1. Split "determine navigation params policy container" into three algorithms with three inputs, one for each of its call sites. These are much easier to read because, e.g., the srcdoc version is very short, and then there's no srcdoc stuff in the fetching version.

2. Try to consolidate "determine the origin" and "determine navigation params policy container" as much as possible. Some of this involves ensuring sourceOrigin /containerOrigin / initiatorPolicyContainer / parentPolicyContainer line up; I think they mostly do. I'm unsure yet whether the end state here has them as separate algorithms, or one combined algorithm, or whether we ensure that "determine the origin" is always called by the now-split-up "determine navigation params policy container" instead of having the call sites call both of them in sequence. TBD.

3. Potentially simplify the inputs to, or split up, "determine the origin" itself.

4. Potentially consolidate "create a policy container from a fetch response" into the above stuff, since after (1) it should be pretty closely tied with the first variant of "determine navigation params policy container".

I'm going to file a new issue with this. At the start of my journey I thought this should block the refactoring here, but after spending 1.5 hours on this I've convinced myself it's very separable work, it's not obvious how to complete all the steps, and each step needs close review.

[annevk]

This is an existing problem of sorts with this algorithm, but _ initiatorOrigin_ is coming out of nowhere. Perhaps introduced by "the rewrite" @domenic?

It's not clear to me that keeping the policy container but changing the origin is sound.

[domenic]

This is an existing problem of sorts with this algorithm, but _ initiatorOrigin_ is coming out of nowhere. Perhaps introduced by "the rewrite" @domenic?

Oops, yes, will fix. It should be newDocumentOrigin. #8711 to fix this.

It's not clear to me that keeping the policy container but changing the origin is sound.

So, the intent of policy container seems to be to keep together things that are inherited together. I'm not sure if origin is the same as other policy container members in this way. I.e., I'm not sure that whenever you inherit CSP/COEP/referrer policy, you should also inherit origin. @antosart as one of the original policy container people may have thoughts?

Note that https://github.com/antosart/policy-container-explained#the-security-policies never envisioned putting origin in the policy container.

That said, if it works everywhere else and this javascript: URL case is the only suspicious one, then we should be fine for the reasons explained in my next section.

---

Regarding javascript: URLs in particular, note that initiatorOrigin / newDocumentOrigin will always be same origin-domain with policyContainer's origin, because of the check in https://html.spec.whatwg.org/#navigate-to-a-javascript:-url step 3. So I don't think there's a huge concern here, if the issue is just around same-origin vs. same origin-domain. Maybe browsers don't even implement the spec here in various ways, e.g., maybe they do a same-origin check in that guard step, or maybe they don't overwrite the new document's origin in the cases where they differ. More testing would be ideal.

In any case, I strongly believe we shouldn't block the larger work here on javascript: URL edge cases around same-origin vs. same-origin domain. So if this is the only place where putting origin into policy container is problematic and requires a clone-but-patch-one-member pattern, then let's just slap an XXX box here, file an issue to write some tests and determine what's the most elegant behavior we can all converge on, and move on.

[antosart]

So, the intent of policy container seems to be to keep together things that are inherited together. I'm not sure if origin is the same as other policy container members in this way. I.e., I'm not sure that whenever you inherit CSP/COEP/referrer policy, you should also inherit origin. @antosart as one of the original policy container people may have thoughts?

Note that https://github.com/antosart/policy-container-explained#the-security-policies never envisioned putting origin in the policy container.

FWIW, we did not write it down, but there was the hope that at some point also the origin would end up in the policy container. I am very happy to see that this is finally getting done!

For the rest, I agree with @domenic. This particular point should matter only if the origin's domain was changed. I am not sure how policies like COOP would (and/or should) behave in such case. I don't know all the background here, but this seems an edge case to me (which we might hope to get rid of at some point?) - so I would not block on it.

[annevk]

@antosart your review of this PR would be much appreciated by the way.

[wanderview]

I've rebased this on top of Domenic's change. Please let me know if there is consensus on doing more here.

[wanderview]

Note, I'll be traveling next week and will have limited time to work on this. I should be able to respond to things tomorrow, however. Just FYI in terms of keeping momentum on this.

[annevk]

I think this would really benefit from @antosart and @domenic's review as it touches history, navigation, and the policy container.

[antosart]

I already started having a look but I got blocked because of #8725. Anyway, I'll move forward with a review tomorrow.

[domenic]

Can we assert these are the same instead of overwriting?

[domenic]

I spent about 1.5 hours going down this rabbit hole.

I think it's definitely possible to make this easier to read. Now that effectively "determine navigation params policy container" + "determine the origin" are doing one big combined thing, it's pretty bad. They have the combined inputs responseURL, sandboxFlags, sourceOrigin, containerOrigin, historyPolicyContainer, initiatorPolicyContainer, parentPolicyContainer, responsePolicyContainer. Maybe sourceOrigin / containerOrigin /initiatorPolicyContainer / parentPolicyContainer have some redundancy; maybe not.

However, any work here ends up being pretty orthogonal to Ben's PR. I wrote it all out and it consists of a few steps:

1. Split "determine navigation params policy container" into three algorithms with three inputs, one for each of its call sites. These are much easier to read because, e.g., the srcdoc version is very short, and then there's no srcdoc stuff in the fetching version.

2. Try to consolidate "determine the origin" and "determine navigation params policy container" as much as possible. Some of this involves ensuring sourceOrigin /containerOrigin / initiatorPolicyContainer / parentPolicyContainer line up; I think they mostly do. I'm unsure yet whether the end state here has them as separate algorithms, or one combined algorithm, or whether we ensure that "determine the origin" is always called by the now-split-up "determine navigation params policy container" instead of having the call sites call both of them in sequence. TBD.

3. Potentially simplify the inputs to, or split up, "determine the origin" itself.

4. Potentially consolidate "create a policy container from a fetch response" into the above stuff, since after (1) it should be pretty closely tied with the first variant of "determine navigation params policy container".

I'm going to file a new issue with this. At the start of my journey I thought this should block the refactoring here, but after spending 1.5 hours on this I've convinced myself it's very separable work, it's not obvious how to complete all the steps, and each step needs close review.

[domenic]

Let's add <span class="XXX">Does the origin actually change?</span>

[domenic]

This seems incorrect; the previous step should take the responseOrigin argument.

[domenic]

This shouldn't be part of the <dl>. Instead we should define it as a standalone <dfn> similar to "responsible event loop" below.

[domenic]

Suggested change

	<li><p><span>Assert</span> that <var>settings object</var>'s <span
	<li><p><span>Assert</span>: <var>settings object</var>'s <span

[domenic]

Is there a way to fold this stuff into "initialize a worker global scope's policy container"?

[antosart]

+1. Analogously as my comment above for properly initializing a document's policy container's origin in the policy containers section, I think it would be great to encapsulate also this logic within the policy container inheritance. At the end of the day, that's exactly the purpose of the policy container: encapsulating this inheritance logic!

[antosart]

Is this necessary? Can't we assert this instead?

[antosart]

I have the impression we don't actually need to pass the origin, and it would make more sense not to do so. The point of this algorithm is to encapsulate the inheritance of policies for local schemes. I think the origin is inherited in exactly the same way (if not, we should understand why and carve out exceptions). Could we omit passing this origin here?

Ideally we would move the algorithm for determining the origin here in the policy containers section, and the origin would follow the same logic as the other policies: the inheritance (i.e. determining the origin based on whether the scheme is local) should be implemented here in "determining navigation params policy container", while responsePolicyContainer's origin should be populated in "creating a policy container from a fetch response", as you are already doing.

[antosart]

+1. Analogously as my comment above for properly initializing a document's policy container's origin in the policy containers section, I think it would be great to encapsulate also this logic within the policy container inheritance. At the end of the day, that's exactly the purpose of the policy container: encapsulating this inheritance logic!