-
Notifications
You must be signed in to change notification settings - Fork 21
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
No JSON in header value #17
Conversation
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.
Some quick thoughts, thanks for putting this together!
index.src.html
Outdated
; See Section 2 of [[HTTP-JFV]], and Section 2 of [[RFC7159]] | ||
Clear-Site-Data = "Clear-Site-Data" ":" <a>clear-site-data-value</a> | ||
<dfn>clear-site-data-value</dfn> = OWS 1#(<a>type</a> OWS) | ||
<dfn>type</dfn> = atom |
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'd suggest something like:
Clear-Site-Data = 1#( <a>data-type-value</a> / <a>extension-type-value</a> )
<dfn>data-type-value</dfn> = "<dfn>cache</dfn>" / "<dfn>cookies</dfn>" / "<dfn>storage</dfn>" / "<dfn>executionContext</dfn>"
<dfn>extension-type-value</dfn> = 1*( <a>ALPHA</a> / "-" )
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.
Updated, but with some changes:
-> From what I have read in other specs so far, the definition of a header always includes the key and separator (i.e. "Clear-Site-Data" ":"), not just the value
-> That's why I have the token "clear-site-data-value", which stands for the header value only
-> The "data-type" and "extension-type" tokens do not need the suffix "-value" (they represent the datatypes, not the header value as a whole, so the name is expressive enough)
-> I still think we need to specify OWS; I don't think spaces around tokens should be a syntax error
-> You suggested that alpha is wrapped in anchors; do you want it to link to RFC5234?
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.
-> From what I have read in other specs so far, the definition of a header always includes the key and separator (i.e. "Clear-Site-Data" ":"), not just the value
I'm following along with things like https://tools.ietf.org/html/rfc7231#section-3.1.2.2 here. I'm sure I've done it differently elsewhere, but this is what I think IETF folks have solidified upon for header ABNF.
-> The "data-type" and "extension-type" tokens do not need the suffix "-value" (they represent the datatypes, not the header value as a whole, so the name is expressive enough)
Sure.
-> I still think we need to specify OWS; I don't think spaces around tokens should be a syntax error
This is part of the #
rule in https://tools.ietf.org/html/rfc7230#section-7 which we should be linking to. @annevk suggested elsewhere that we link to https://fetch.spec.whatwg.org/#abnf to make that clear.
-> You suggested that alpha is wrapped in anchors; do you want it to link to RFC5234?
Yes, please. Just copy/paste from https://github.com/w3c/webappsec-csp/blob/master/index.src.html#L101. :)
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.
Ah, alright. I must have read something older then. Updated.
I'm now linking to 7230 for #rule
, since it's more detailed (e.g. contains the discussion about OWS). That is helpful at least for a reader like myself.
index.src.html
Outdated
MUST be an array, and that array MUST contain only strings; any other types | ||
will result in a parse error. | ||
Each atom in the array represents a data type that the user agent MUST | ||
clear, and will be parsed as defined in [[#parsing]]. |
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'd replace these two sentences with something like [[#fetch-integration]] and [[#parsing]] describe how the
Clear-Site-Data header is processed.
index.src.html
Outdated
The following are the initial set of known types which may be specified in | ||
the member's array value. Future versions of this document may define | ||
The following are the initial set of known types which may be specified as | ||
the array's elements. Future versions of this document may define | ||
additional types, and user agents MUST ignore unknown types when parsing the | ||
header: |
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.
Maybe something like The <a grammar>data-type-value</a> grammar defines an initial set of known data types which can be cleared using this API. Future versions ...
, and then following it up with the list.
index.src.html
Outdated
|
||
5. Return |types|. | ||
5. Return |recognizedTypes|. |
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'd write this in terms of Fetch. That is:
1. Let |data-types| be an empty list.
2. Let |header| be the result of [=extracting header list values=] given `Clear-Site-Data`
and |response|'s [=response/header list=].
3. If |header| is `null`, return |data-types|.
4. For each |type| in |header|:
1. If |type| matches the <a grammar>data-type-value</a> grammar, and |type| is not
[=list/contained=] in |data-types|, then [=list/append=] |type| to |data-types|.
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 should account for failure too (e.g., x x
would be a value that leads to failure). Might also be clearer to call the return variable values rather than header.
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.
@annevk: The suggestion accounts for failure by adding |type|
to |data-types|
only if it matches the list of known keywords in data-type-value
. Do you think we should do something more than that?
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.
Then you wouldn't have to account for null either. But more importantly, you can't for each failure.
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 think we do have to account for null
, because we can't for...each
over null
. But reading Fetch again, I see your point: step 2 and 4.2 of https://fetch.spec.whatwg.org/#extract-header-list-values can return failure. I guess we'd want to handle that in step 3 as well. Thanks!
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.
Done. Agree with returning failure in level 3, this is what Chromium's implementation actually does (we print an error to the console, we don't just ignore it).
Clarifying question: Is it OK to omit the old step #1 which checked if the header exists? Because if we run the current version of the algorithm on any random response, it will return failure. My interpretation of that would be that the browser outputs a console error every time it doesn't see a Clear-Site-Data
header. Or is it assumed from the context that the algorithm is only executed for responses that do have the header?
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.
Done. Agree with returning failure in level 3, this is what Chromium's implementation actually does (we print an error to the console, we don't just ignore it).
I'd actually suggest changing this to something like "If |header| is `null` or failure, return an empty list.
", and adding a note that user agents should inform developers of failure to parse the header. If you "return failure", then you need to handle "failure" in https://w3c.github.io/webappsec-clear-site-data/#clear-response instead. I think it makes more sense to handle it here by returning an empty list, which means the clearing operation will just do the right thing (nothing).
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.
Clarifying question: Is it OK to omit the old step #1 which checked if the header exists?
Step 1 of https://fetch.spec.whatwg.org/#extract-header-list-values does this check, and returns null, in which case we return an empty list.
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.
Makes sense. Thanks for the explanation! Done.
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.
Left some small comments, thanks!
LGTM. I'll rebuild the file for you in a subsequent patch. |
Hi @mikewest !
Please have a look! This simplifies the header syntax from a JSON to a list of strings. One thing I wasn't sure how to do was how to refer to the parsing algorithm. The way I understand it, the ABNF definition itself already represents a parsing algorithm, so I just added s there and linked them below.