-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Add policy container #6504
Add policy container #6504
Conversation
f9bb5b5
to
7180e1a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @antosart, thanks for putting this together! I could only do a quick first pass this morning, so here are some initial comments. I'm excited about getting this worked out!
source
Outdated
</ol> | ||
|
||
<p>To <dfn data-x="creating a policy container for a fetch response">create a policy container for | ||
a fetch response</dfn> from a given <span data-x="concept-response">response</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this should actually happen in Fetch, creating a new property on the response
that could be used for policy decisions (see https://fetch.spec.whatwg.org/#concept-response-csp-list, for example). That would nicely mirror the way we're slowly shifting parsing for these policy concepts out of Blink and into Chromium's network service, and explain how we're able to do certain checks at certain times.
Adding a policy container to the outgoing request could also simplify the explanation of fetch directive checks, and remove some indirection through browsing contexts that may or may not actually be alive at the time the check is performed...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had the same thought. I decided to avoid that for now because it needed a synced change to fetch, too. But if there is agreement that that is a better way, I would be happy to do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll defer to @annevk, but it seems like a good change to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For requests: I think taking a snapshot of the policy container is needed as the current setup has significant issues, e.g., whatwg/fetch#832. I do think that ideally we avoid requiring all callers to do so themselves. As long as they invoke fetch on the main thread and have a non-null request client, fetch can make that snapshot from the request's client.
For responses: I guess this is needed with policies that affect subsequent requests, such as Referrer Policy. I think for CSP we originally did it because we wanted to impact cookies, but that never came to pass. Or are there other reasons? In principle I'm supportive of this, but it might be tricky to find the right time that works for all policies. And I'm not sure all policies will need to be parsed for each redirect response and such. (E.g., consider step 16 of https://fetch.spec.whatwg.org/#concept-http-redirect-fetch.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For requests, I would consider doing that as a follow-up (if I understand correctly, that involves not only navigations, but all subresource fetches).
For responses, I am trying to understand the best approach, and I would like some more feedback. There are a few points to consider I believe:
- We don't want to move the whole inheritance logic into Fetch. Hence Fetch would construct a response policy container only for http/https, but not for local schemes. On the one side, this would match the fact that CSP 'frame-ancestors' must be checked only for network responses, for example. On the other side, this seems a bit unclean.
- Strictly speaking, a "policy container" for a response does not totally make sense to me. It would end up containing just the output of parsing headers (while in my opinion a document/global object's policies is somehow a bigger concept, which could contain policies which are not delivered through headers).
- Fetch would construct a response policy container for all responses (not just navigations). Do we want that? I have the impression that the separation between navigation fetches vs subresource fetches is owned by the html spec, and we don't want to pollute Fetch with that (but I might be wrong here).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requests: yes, all of them need access to static policies from "in parallel".
Responses: I think your model is correct. There are some policies, such as Referrer Policy, that we care about for all responses and there was some talk in the past about a similar thing with CSP, but by-and-large they are for responses that result in documents/workers and so are probably best created there.
I think we might even want to remove response's CSP list as it should be a side-effect free operation from obtaining a CSP list from response's header list. The main benefit to storing the result on the response would be that it's only parsed once, but that seems more like an implementation-decision then needed for interoperability or clarity. (I'm flexible here though as I mentioned above.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I had a look at fetch again and given your reply I think for now it makes more sense to me to leave this as it is, unless anyone has strong opinions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requests: I read Anne as asking for us to clone the document's policy container into the outgoing request, and that makes sense to me. I don't have a strong opinion as to whether you should do it in this PR or in a followup that happens after a change to Fetch; from a consistency perspective, it makes sense to make this a three-sided patch that introduces the Policy Container
concept into HTML, then introduces it as an attribute on Fetch's Request
concept, then adjusts HTML to use it. If Anne would prefer two patches that we land at the same time, that's fine by me as well.
Responses: I thought we applied referrer policy to redirects, in which case we need to handle parsing them in Fetch and attach them to the response somehow? If we don't do that, then I think I agree that we could shift the response-time checks out of Fetch and into HTML. It seems like we'd still want to deal with parsing policies into a container as a Fetch-level concept, since that more or less matches how browsers are handling it, and means that HTML can simply work with the response's container rather than parsing itself at every subresource entry point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a strong preference on how we go about landing the changes, though we should probably keep a checklist somewhere as this will be fairly involved overall.
Responses: for Referrer Policy indeed we call out to Referrer Policy for redirect responses, but it's to set request's referrer policy (as it impacts subsequent fetches, but doesn't impact the final policy if this happened to be fetches for a navigation resulting in a document). So it's somewhat special I'd say.
7180e1a
to
a08dc79
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mikewest!
fe843f2
to
ac636a9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a quick look, it's looking good!
What is the plan for policy containers on iframe srcdoc Documents once we expand to include referrer policy? Worker and Worklet environment settings objects have a copy of their inherited referrer policies, but Window environment settings objects dynamically resolve their referrer policy by crawling the chain of ancestor documents looking for the first non-iframe-srcdoc Document, and use its referrer policy.
This PR introduces a policy container to all environment settings objects, so if we want to maintain today's referrer policy inheritance when we expand the policy container to include referrer policy, I assume the policy container's future referrer policy member will have to be some getter that performs this dynamic ancestor look-up? Or do we plan on changing the current behavior and just copying its parent's referrer policy?
ac636a9
to
24e4b4d
Compare
@domfarolino regarding referrer policy for srcdoc iframes, I would be in favour of not treating it in a special way. I like to keep a consistent behaviour where policies are inherited at document creation time, but then each document has its separate own policies. Or are there any strong arguments for making the policies of a srcdoc iframe a "reference" to the parent's policies? Also, notice that the "reference" behaviour makes implementing srcdoc iframes out-of-process more problematic (that's something that no vendor implements at the moment AFAIK but I guess it is something we might want to have - for example for sandboxed srcdoc iframes). I understand that is not a problem of the html spec, but we might want to keep that in mind nevertheless. |
Yeah I am also in favor of not having the policy container dynamically reference a parent's referrer policy for srcdoc iframes. The only objection would be that it would change current web behavior, which certainly may be acceptable here. I just wanted to bring it to light. |
6b78100
to
3ad254f
Compare
3ad254f
to
6ca0962
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took another look and found some new nits, nothing substantive. Starting to look close to ready to me, but not sure if @domenic, @domfarolino, and @mikewest have more thoughts.
I suppose tests might not be applicable here as this is technically a refactor? Implementation bugs might still be good though to give implementations a heads up about this improved infrastructure and likely dependencies on it in the future (e.g., blob URL store).
Could resolve the outstanding PR conversations you consider resolved?
I created implementation bugs and updated the checklist. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Down to a couple of nits. 😊
45e192e
to
1ae9261
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've taken another pass through this, as well as the concrete changes to CSP in w3c/webappsec-csp#482. Both LGTM, thank you!
7b3d4f8
to
969628d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also happy with this, but would be good to resolve the note question I suppose.
@domenic want to do a final editorial review?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really solid work. I'm happy navigation params is paying off for us in allowing this sort of thing to thread through more easily. I'm very impressed with how well this has turned out and excited about moving more things into policy container in the future.
The only substantive comment I have is about which initiator notion you are using. It'd be great if you could make a principled decision backed up by tests there since it's so much of a mess currently and I'd like to not add to it.
Additionally, you should prepare a companion PR for service workers to give them a policy container on their environment settings object. (We used to have to do worklets as well, but they thankfully live in HTML now.)
Otherwise, all my comments are editorial. I'm OOO next week so don't block on me.
data-x="concept-csp-list">CSP list</span>. It is initially empty.</p></li> | ||
</ul> | ||
|
||
<p class="note">Each item has to define a default value for creating a new policy container.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really understand this note. Is this kind of a note to self about adding future items to the container? If so maybe make it a HTML comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the confusion and I turned the note into an HTML comment.
However, my question was (and I am not sure of this): for a given struct in the spec, can I always just create a new instance of it? What do the items default to? The important thing for the policy container is that "create a new policy container" should be a well-defined operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can always create a new instance. Items default to their stated defaults. If items don't state defaults you need to state their values during creation. Everything else would be a bug. So yeah, this seems good as a comment in the source for future maintainers.
ad9df75
to
8feded9
Compare
@antosart want to create the service workers PR and then we land this? Or would you prefer landing this now? Can you write a commit message in a new comment perhaps? |
I created the PR for the service workers: w3c/ServiceWorker#1588. (AFAIU service workers can only be created for network schemes so there is not much to do there) |
@domfarolino requested to do a final review pass on IRC so I'll wait for that before landing. |
Sounds good. Tentative commit message: This change adds the concept of the policy container to the html specification. The policy container serves as collection of policies to be applied to a document or global object, and its purpose is to simplify how policies are initialized and inherited. The policy container is attached to a Document, to a WorkerGlobalScope and to a WorkletGlobalScope. Its policies are populated by parsing headers and/or meta tags. A policy container can be cloned, hence supporting inheritance of policies. As a first step, with this change the policy container only contains the CSP list. Note: This is not meant to be a behavioural change, but rather a refactoring of the specification. Small behavioural changes introduced by this change (for example storing and reloading policies from history) address what are usually considered to be bugs in the specification/implementation (which often turn out to be security vulnerabilities). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took another quick pass and it LGTM! Thanks
@antosart this commit message okay? Add policy container This change adds the concept of a policy container. See also https://github.com/antosart/policy-container-explained. A policy container serves as collection of policies to be applied to a document, WorkerGlobalScope, or WorkletGlobalScope. Its purpose is to simplify how policies are initialized and inherited. Policies are populated by parsing headers and/or meta tags. A policy container can be cloned, hence supporting inheritance of policies. Initially a policy container only contains a CSP list. Note: This is not meant to be a behavioural change, but rather a refactoring. Small behavioural changes introduced by this change (for example storing and reloading policies from history) address what are usually considered to be bugs in the standard/implementation (which often turn out to be security vulnerabilities). CSP PR: w3c/webappsec-csp#482. Service Worker PR: w3c/ServiceWorker#1588. Helps with #4926. |
@annevk sound good! |
This change adapts the CSP spec to the concept of the policy container, which was just introduced in html (whatwg/html#6504). The main changes are: - the CSP list is now part of the policy container of a Document/WorkerGlobalScope/WorkletGlobalScope, and not directly owned by it anymore, - inheritance of CSP for local scheme documents is now taken care by the html spec as part of the policy container inheritance.
This change adapts the CSP spec to the concept of the policy container, which was just introduced in html (whatwg/html#6504). The main changes are: - the CSP list is now part of the policy container of a Document/WorkerGlobalScope/WorkletGlobalScope, and not directly owned by it anymore, - inheritance of CSP for local scheme documents is now taken care by the html spec as part of the policy container inheritance.
This change adds the concept of the policy container to the html spec following the explainer [1]. The policy container serves as collection of policies to be applied to a document or global object, and its purpose is to simplify how policies are initialized and inherited.
As a first step, with this change the policy container only contains the CSP list.
This is not meant to be a behavioural change, but rather a refactoring of the specification. I believe small behavioural changes introduced by this change (for example storing and reloading policies from history) address what are usually considered to be bugs in the specification/implementation (which often are security vulnerabilities).
Compared to what is outlined in [1], there are at least a few differences in the current change:
Alongside with this, I am opening a companion PR (w3c/webappsec-csp#482) on the CSP specification.
[1] https://github.com/antosart/policy-container-explained
As written above, this is meant to be a refactoring, so I believe the checklist applies only partially. Anyway, checklist:
/browsers.html ( diff )
/browsing-the-web.html ( diff )
/dom.html ( diff )
/history.html ( diff )
/iframe-embed-object.html ( diff )
/index.html ( diff )
/infrastructure.html ( diff )
/origin.html ( diff )
/urls-and-fetching.html ( diff )
/webappapis.html ( diff )
/window-object.html ( diff )
/workers.html ( diff )
/worklets.html ( diff )