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

Add policy container #6504

Merged
merged 9 commits into from
May 3, 2021
Merged

Add policy container #6504

merged 9 commits into from
May 3, 2021

Conversation

antosart
Copy link
Member

@antosart antosart commented Mar 17, 2021

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:

  • This only includes CSPs.
  • For blob URLs, the policy container of the blob URL entry environment is cloned when retrieving the URL. This approach avoids the need of storing a copy of the policy container in the blob URL entry and avoids changes to the FileAPI spec.
  • This also attaches a policy container to workers and worklets. I figured it is easier to do this than not, since in this way we always have a policy container for an environment settings object, regardless of whether it's a Window or a WorkerGlobalScope.

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 )

@antosart
Copy link
Member Author

@ParisMeuleman ParisMeuleman self-assigned this Mar 17, 2021
Copy link
Member

@mikewest mikewest left a 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 Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
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>
Copy link
Member

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...

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member

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.)

Copy link
Member Author

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).

Copy link
Member

@annevk annevk Mar 19, 2021

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.)

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member

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.

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@mikewest mikewest added the topic: policy container The policy container proposal label Mar 18, 2021
Copy link
Member Author

@antosart antosart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mikewest!

source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@antosart antosart force-pushed the add-policy-container branch 3 times, most recently from fe843f2 to ac636a9 Compare March 25, 2021 07:36
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really promising! I hope you can work with @domenic and @mikewest to land this initial part of policy container as I'll be off for two weeks, but if not I'll be happy to help get this landed then.

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
Copy link
Member

@domfarolino domfarolino left a 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?

source Outdated Show resolved Hide resolved
@antosart
Copy link
Member Author

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

@domfarolino
Copy link
Member

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.

@antosart antosart force-pushed the add-policy-container branch 2 times, most recently from 6b78100 to 3ad254f Compare April 15, 2021 12:21
Copy link
Member

@annevk annevk left a 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?

source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
@antosart
Copy link
Member Author

antosart commented Apr 19, 2021

I created implementation bugs and updated the checklist.

Copy link
Member

@annevk annevk left a 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. 😊

source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
Copy link
Member

@mikewest mikewest left a 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!

source Outdated Show resolved Hide resolved
Copy link
Member

@annevk annevk left a 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?

Copy link
Member

@domenic domenic left a 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>
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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.

source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
@annevk
Copy link
Member

annevk commented Apr 27, 2021

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

@antosart
Copy link
Member Author

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)

@annevk
Copy link
Member

annevk commented Apr 27, 2021

@domfarolino requested to do a final review pass on IRC so I'll wait for that before landing.

@antosart
Copy link
Member Author

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).

Copy link
Member

@domfarolino domfarolino left a 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

@annevk
Copy link
Member

annevk commented May 3, 2021

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

@antosart
Copy link
Member Author

antosart commented May 3, 2021

@annevk sound good!

@annevk annevk merged commit 30244b9 into whatwg:main May 3, 2021
antosart added a commit to w3c/webappsec-csp that referenced this pull request May 5, 2021
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.
ryandel8834 added a commit to ryandel8834/WebAppSec-CSP that referenced this pull request Aug 13, 2022
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.
@whatwg whatwg deleted a comment from armandRobled Nov 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: policy container The policy container proposal
Development

Successfully merging this pull request may close these issues.

6 participants