-
Notifications
You must be signed in to change notification settings - Fork 155
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
Header should be sufficient in some cases to delegate features (esp. Client Hints) #408
Comments
@annevk FYI, since this affects the header processing which has been a recent subject of some discussion. |
This allows the header alone to enable delegation of a feature to specific origins, if not otherwise blocked or affected by the container policy. The order of precedence becomes: 1. Explicitly blocked by header: Disabled 2. Explicitly blocked by allow attribute: Disabled 3. Explicitly allowed by allow attribute: Enabled 4. Explicitly allowed by header: Enabled 5a. (Default behaviour when default allowlist is '*'): Enabled 5b. (Default behaviour when default allowlist is 'self'): Enabled if same- origin; Disabled if cross-origin. Fixes: #408
I don't think Mozilla has taken a position on Client Hints and sync-xhr doesn't fit as a Permissions Policy as it's not disabled by default in cross-origin contexts. I thought we discussed before how that would be a better fit for Document Policy? So I'm not really in favor of changing this in this way. |
@annevk, I'm curious about where the objection stems from -- is is the algorithm itself, or the usage of Permissions Policy for CH in the first place? For the algorithm -- the original This essentially just makes the inheritance algorithm use these priorities:
Agreed that sync-xhr fits better in Document Policy, it'll just take some time to get there. I'd be happy to rip it out of Permissions Policy, but it's not the only case of a default '*' allowlist. Client hints are a really interesting delegation case, where the attribute isn't as useful as the header for expressing intent, and it would be a shame to have to introduce a different delegation mechanism for it. |
The objection is to making changes to Permissions Policy for the purposes of a feature that is pretty far from standardized. |
The counter to that is that we are making changes anyway, from the behaviour in I believe that this change is more web-compatible than the change from #378, while keeping the same security guarantees that were requested in #357. There are uses in the wild of the The delta between this proposal and #378 is just step 4 from above. It's still much easier to explain than the current FP algorithms, and means that Is feature enabled doesn't have to work differently than Define an inherited policy, which it currently does. |
I'm not sure how this relates to web compatibility as this is a new header with new syntax. And client hints only has a single implementation too. |
It's two-fold, I guess:
|
Specifically RE Client Hints, the feature indeed has a single implementation at the moment, but it's unclear to me why that means that we need to make sure that infrastructure features like Permission Policy explicitly don't support its use-cases (or any future policy we'd like to apply to resource requests), especially when that comes with no drawbacks. |
But we never had agreement on the use cases with the initial version. That covered all kinds of scenarios there was no agreement on. |
So, I still like this change -- it maintains more backwards compatibility with the shipped Feature-Policy header, it makes the Client Hints case more ergonomic, and I like the symmetry of the conditions: Header Deny > Attribute Deny > Attribute Allow > Header Allow That said, that's not really compelling enough to make the change, and there is definitely real value in being able to state that the I'm going to close this; it may get re-visited if other compelling use cases come up, but for now, we can explore how best to make features work within the existing algorithms. |
I think that the changes made in #378 may have gone a bit further than necessary. #357, which was the motivation for the change, asked that blocking policies set in the header not be overridable with the allow attribute.
#378 does this, by making policy inheritance essentially the logical AND of parent-header and iframe attribute policy, and necessarily switches the header defaults for every feature to "*". (That was necessary so that developers would not be required to set the header in order to enable delegation at all, but could use just the attribute in the common case.) This also ends up making the allow attribute required in every case to delegate any permission.
Unfortunately, this model breaks some deployed use cases, where the header could previously have been used to explicitly grant permission to specific origins, and isn't very ergonomic for the Client-hint delegation case, where not only would every hint need to be specified on every frame, but we lose the ability to tell from the header which hints should be sent with subresource requests.
@yoavweiss suggested in email a small tweak which would reenable the use case of "enable delegation for specific sites with the header" and would allow Client Hints to be delegated with the header only. It involves treating an explicit header declaration, where a feature is mentioned in the header, with an associated allowlist, differently from the 'default' behaviour when the feature is not mentioned at all in a header.
I want to propose a change which would allow this, as well as the existing use cases, while still being web-compatible:
We can do this with a simple change to the Define an inherited policy for feature in container at origin algorithm, inserting this step between what is currently steps 5 and 6:
I have tested a Chromium CL with this, which passes all existing Feature-Policy tests, except for those which expose the new "Should be irrevocably blocked by the header" behaviour, but that is the behaviour that we want in any case, so those test expectations should be updated to match.
I'll work up a PR to add this, as well.
Opinions welcomed :)
The text was updated successfully, but these errors were encountered: