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

commas and semicolons both allowed, with different rules for overriding #148

Closed
dbaron opened this issue Mar 13, 2018 · 11 comments
Closed
Labels

Comments

@dbaron
Copy link
Member

dbaron commented Mar 13, 2018

If I execute (in my head, so error-prone!) the algorithm for 8.2. Parse header from value and origin, it splits the value on commas, and for each later piece, merges it in by adding only the features not already present. Thus earlier ones override later ones.

But the thing it does with the values is call 8.3. Parse policy directive from value and origin, which splits the value on semicolons, and replaces earlier entries with later ones.

I think, together, this means that in:

Feature-Policy: vibrate https://a.example.com/; vibrate https://b.example.com/, vibrate https://c.example.com/

the resulting policy is vibrate https://b.example.com/.

This seems pretty weird to me. Is it intended? If it is, perhaps there should be some prose explaining why?

@dbaron dbaron changed the title commas an semicolons both allowed, with different rules for overriding commas and semicolons both allowed, with different rules for overriding Mar 13, 2018
@annevk
Copy link
Member

annevk commented Mar 14, 2018

(This should also be a shared code path with CSP.)

@clelland
Copy link
Collaborator

It is weird, and may be entirely backwards to CSP :(

CSP treats multiple headers, or multiple comma-separated components (indistinguishable in HTTP at the browser's level) as separate policies, overlaid on top of each other. But then, within a policy, later directives with identical names as earlier ones are ignored.

At the very least, we should probably reverse the logic in 8.3. Parse policy directive from value and origin to keep the earlier features, and discard later ones with a notification, as in 2.1.1. Parse a serialized CSP as disposition

@briansmith
Copy link

As I mentioned in #314, HTTP header parsing rules mean that the following mean that:

Feature-Policy: x
Feature-Policy: y

means the same thing as:

Feature-Policy: x, y

This is why CSP uses semicolons to separate directives within a single policy.

I agree with @annevk that, in the absence of a strong motivation otherwise, it should work like CSP.

@clelland
Copy link
Collaborator

@briansmith, by 'work like CSP', do you mean that we intersect the policies if multiple headers (or multiple comma-separated declarations) are found?

I think that would just involve changing Merge directive with declared policy to do something more complex when an allowlist for a given feature has already been declared.

I'd like to keep the property that:

Feature-Policy: feature1 allowlist1
Feature-Policy: feature2 allowlist2

is the same as

Feature-Policy: feature1 allowlist1; feature2 allowlist2

but if there are better ways to handle

Feature-Policy: feature1 allowlist1
Feature-Policy: feature1 allowlist2

then we should explore those.

@briansmith
Copy link

@briansmith, by 'work like CSP', do you mean that we intersect the policies if multiple headers (or multiple comma-separated declarations) are found?

Yes.

I'm not saying it has to be done that way. What I'm saying is that it should be done that way in the absence of a strong motivation for doing things differently. These kinds of things are quite confusing to users, and having similar-but-different ways of combining features for similar-but-different features is confusing.

A lot of people don't consider the possibility that a header field could be duplicated, and a lot of people don't understand that web servers may combine a duplicated header field.

I'd like to keep the property that:

Why?

@annevk
Copy link
Member

annevk commented May 17, 2019

I'm strongly opposed to that property, FWIW. Header values need to be combined first, then parsed. And combining is with a comma.

@annevk
Copy link
Member

annevk commented May 17, 2019

Changing this shouldn't be that problematic either, right? Since presumably we want to use a different name from feature policy now it's scoped to permissions?

@clelland
Copy link
Collaborator

@briansmith:

Why?

@annevk:

I'm strongly opposed to that property, FWIW.

My reasoning is that many features aren't related to each other at all, so it might be totally reasonable for one part of a serving system to emit a header that controlled some features, and another part to emit another header for other features. I think it would be counterintuitive, for instance, if the intersection of

feature1 allowlist1

and

feature2 allowlist2

to be, say, an empty policy. (Or equivalently, to end up being a default policy for both features).

@clelland
Copy link
Collaborator

Unless I misread your objection, @annevk -- I was suggesting that concatenating two unrelated headers:

Feature-Policy: feature1 allowlist1, feature2 allowlist2

should result in the same policy as the single declaration:

Feature-Policy: feature1 allowlist1; feature2 allowlist2

assuming that we accept both syntaxes are valid.

I wasn't trying to suggest that we override HTTP semantics and concatenate with semicolon delimiters, but I re-read my initial suggestion and could see that interpretation.

@annevk
Copy link
Member

annevk commented May 17, 2019

I see, the main thing I care about with respect to headers is that we combine all of them (per HTTP rules), then parse. [Sounds like that is preserved.]

The secondary thing I care about with respect to this particular header is whether it should have a name more fitting to its scope, but that might be out-of-scope (harharhar) for this issue.

@clelland
Copy link
Collaborator

Closing this; the header has been replaced with the structured Permissions-Policy header, in which commas are the only legal separator, and there are rules defined by SH for combining multiple headers and resolving duplicates.

In the allow attribute, the comma should not be accepted, only semicolons to separate features. (Only a single directive is allowed).

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

No branches or pull requests

5 participants