w3c / webappsec-referrer-policy

WebAppSec Referrer Policy
https://w3c.github.io/webappsec-referrer-policy/
Other
26 stars 27 forks source link

Relax ABNF for invalid policy tokens #102

Closed estark37 closed 7 years ago

estark37 commented 7 years ago

@annevk pointed out in https://github.com/w3c/web-platform-tests/pull/5054#issuecomment-284664280 that when policy-token is defined as a collection of literals corresponding to the valid referrer policies, the browser should technically completely disregard a header of the form Referrer-Policy: origin, blah. Instead, we want the browser to ignore blah as an unknown policy value and fall back to origin.

So this PR relaxes the ABNF of the Referrer-Policy header to parse invalid policy values.

estark37 commented 7 years ago

@annevk does this address the issue you raised in https://github.com/w3c/web-platform-tests/pull/5054#issuecomment-284664280?

I'm not sure if any changes are needed to https://w3c.github.io/webappsec-referrer-policy/#parse-referrer-policy-from-header. I think it works as is, by throwing out any token which is not a referrer policy?

annevk commented 7 years ago

So this works, but this also means that a value such as origin no-referrer results in failure since you don't allow spaces. That seems fine by the way, just pointing it out since it will affect the outcome of one of the tests.

The parsing steps still look good indeed.

@mikewest might also want to look at this since I suspect CSP might suffer from similar issues, but not sure.

annevk commented 7 years ago

Oh, for the term ABNF you might want to reference https://fetch.spec.whatwg.org/#abnf or say something equivalent.

mikewest commented 7 years ago

I'm pretty sure the structure of directives in CSP is loose enough to formally allow most everything: https://w3c.github.io/webappsec-csp/#framework-directives. The ABNF for parsing source lists probably needs an "everything else" bucket, but I'm not sure it's a good idea to drop the list of valid keywords as this patch does. I find it pretty useful to be able to link to things like https://w3c.github.io/webappsec-csp/#grammardef-unsafe-inline (if only because it helps me catch spelling errors).

annevk commented 7 years ago

An alternative that @estark37 might be willing to consider is leaving the explicit keywords in and defining something like an "extension-token" production that is ALPHA / "-". HTTP uses that at times I believe.

mikewest commented 7 years ago

Right, that's what I was clumsily suggesting with an "everything else" bucket. Something like:

"Referrer-Policy:" 1#(policy-token / extension-token)

policy-token    = "no-referrer" / "no-referrer-when-downgrade" / "strict-origin" / "strict-origin-when-cross-origin" / "same-origin" / "origin" / "origin-when-cross-origin" / "unsafe-url"
extension-token = ALPHA / "-"
estark37 commented 7 years ago

Ohh ok, perfect. Went back to the explicit keywords with an extension-token bucket. Thanks!

estark37 commented 7 years ago

@mikewest does the latest iteration lgty?

mikewest commented 7 years ago

Yes! LGTM. Sorry, I thought you'd landed this already. :)