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

"origin's origin policy" seems like a bad spec primitive #73

Closed
domenic opened this issue Feb 12, 2020 · 7 comments · Fixed by #80
Closed

"origin's origin policy" seems like a bad spec primitive #73

domenic opened this issue Feb 12, 2020 · 7 comments · Fixed by #80
Assignees

Comments

@domenic
Copy link
Collaborator

domenic commented Feb 12, 2020

The current spec says in https://wicg.github.io/origin-policy/#origin-origin-policy

We define the origin policy for a given origin to be the result of retrieving the cached origin policy for that origin.

and then the way that anyone (e.g., the CSP or FP spec) is supposed to use origin policy, is by consulting that definition. E.g. in https://wicg.github.io/origin-policy/#monkeypatch-fp it is consulted by

The create a feature policy for a browsing context algorithm is modified by modifying the returned feature policy's declared policy to be a clone of origin’s origin policy's feature policy.

Now, this particular example works, because it makes a copy, at a point in the request lifecycle which reasonably makes sense. But the general idea of there being an ambient "origin's origin policy" which anyone could grab, synchronously, from specs, is not a good one.

For example, consider if we tried to define #48 using this. It'd be something like

The originPolicyIds getter returns a frozen array created from the current settings object's origin's origin policy's IDs

This would (kind of, modulo frozen-array weirdness) imply that, if the cache entry for /.well-known/origin-policy expires, we'd start returning a different value. Whereas what we really want is to snapshot the IDs at some startup time (Window creation time, I think?) and return the same ones forevermore.

In summary, it's possible to use "origin's origin policy" correctly, but it's way too easy to use it incorrectly.

Potential mitigations:

  • Remove the "origin's origin policy" alias and instead require folks to use "retrieving the cached origin policy", to at least be more explicit.
  • Have some specific time in spec-land at which we retrieve the cached origin policy, and store it on the global (or document, for Window?), and then have other parts of the spec consult that.

Probably the second is more aligned with implementation architecture.

Relatedly, it's not obvious from the spec right now that cached origin policies apply even when no origin-policy header is present. Consulting them is hidden inside the CSP/FP monkeypatches. That would be more obvious if there were such an explicit copying point.

@annevk
Copy link

annevk commented Feb 13, 2020

I suspect you need both mitigations as sometimes the policy is relevant before you have constructed a global to consult (e.g., navigation, cross-origin subresource).

Also, given how this will use the "cache key" (or whatever we'll end up calling it) using the origin alone is wrong as it suggests a much wider scope than it has.

@domenic
Copy link
Collaborator Author

domenic commented Feb 18, 2020

Also, given how this will use the "cache key" (or whatever we'll end up calling it) using the origin alone is wrong as it suggests a much wider scope than it has.

Hmm yeah, since we are keying that off of fetch clients, then all of the current null-client fetches are probably not correct... that's not good. I'll open a new fetch issue to discuss...

Edit: In the process of writing up a new fetch issue I realized the solution is actually relatively simple. I just need to pass the request's client from Fetch and re-use that when fetching origin policies. I will open a new issue to track fixing that so that this one can stay focused on the "origin's origin policy" problem.

@domenic
Copy link
Collaborator Author

domenic commented Feb 20, 2020

Sanity check please: when creating the initial about:blank document, it does have an origin, and we should apply any feature policies/CSPs/etc. for that origin to the about:blank document. I.e., we need to apply origin policies both at browsing context time and at navigation. Right?

/cc @mikewest @clelland

@annevk
Copy link

annevk commented Feb 21, 2020

Yeah, you'll also need to consider blob: URLs, data: URLs, javascript: URLs, etc. that also inherit. See some of the tests I recently landed for COOP/COEP (beware, some of them still use templates). And maybe also some of the open issues. In particular for blob: URLs there's some agreement on a better design in that the policy would be copied at createObjectURL() time which would be observable if it were to change between that and fetching time.

@domenic
Copy link
Collaborator Author

domenic commented Feb 21, 2020

I think I can mostly piggyback on the existing feature policy/CSP/etc. infrastructure. It was just a question as to which parts I do so. E.g. FP hooks in to both "create a new browsing context" and "initialize a new Document object". I have a patch locally that does so for FP-via-OP as well.

CSP is a bit more complicated because: (1) it seems to ignore "create a new browsing context", but I think I convinced myself that was on purpose; (2) it also checks a response against itsself, which we probably want to get in on for CSP-via-OP, and thus we need to hook in earlier in the pipeline.

The cleanest solution for CSP (which also matches our current implementation) might involve something very early in the pipeline that modifies the response to have both header-CSP and OP-CSP, so that they are treated equally. We'll see.

domenic added a commit that referenced this issue Feb 21, 2020
Previously, the Fetch integration would "update" the origin's origin policy, i.e. make sure the version in the cache was updated. Then the various integration points (so far, CSP and FP) would grab the origin policy from the cache. This architecture is fragile (see discussion in #73) and does not match how implementations would reasonably work.

This new version stores the origin policy on the response, and then uses that when constructing CSP and FP from the response.

Fixes #73.
@domenic domenic self-assigned this Feb 21, 2020
domenic added a commit that referenced this issue Feb 25, 2020
Previously, the Fetch integration would "update" the origin's origin policy, i.e. make sure the version in the cache was updated. Then the various integration points (so far, CSP and FP) would grab the origin policy from the cache. This architecture is fragile (see discussion in #73) and does not match how implementations would reasonably work.

This new version stores the origin policy on the response, and then uses that when constructing CSP and FP from the response.

Fixes #73.
@clelland
Copy link

A bit late here, but yes, for feature policy at least, we always apply the parent's policy (modulo any container policy) to an about:blank child, as well as data:, blob:, srcdoc etc. URLs. Document policy should do the same.

Feature policy hooks in to two places in HTML, since the container policy is defined on the browsing context: once when the browsing context is created, and there is no response yet, and then again when the document is initialized, and we have the response headers. CSP (without CSPEE, at least) may not need the first one if it is strictly scoped to single documents.

@domenic
Copy link
Collaborator Author

domenic commented Feb 25, 2020

Thanks @clelland! I think the current spec (after merging #80) manages to get most of the right FP behavior "for free" by hooking in around the same place HTTP headers do, when responses are being processed. See https://wicg.github.io/origin-policy/#monkeypatch-fp, and please holler if anything looks wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants