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

SRI: Respond to mnot comments #217

Closed
devd opened this issue Mar 18, 2015 · 17 comments
Closed

SRI: Respond to mnot comments #217

devd opened this issue Mar 18, 2015 · 17 comments
Labels
Milestone

Comments

@devd
Copy link
Contributor

devd commented Mar 18, 2015

see https://readable-email.org/list/public-webappsec/topic/sri-updates-to-the-spec-and-outstanding-issues#content-3

Opening this as a tracker task just to make sure it doesn't fall through the cracks.

@devd devd added this to the SRI-v1-LC milestone Mar 18, 2015
@mikewest mikewest added the SRI label Mar 18, 2015
@mozfreddyb
Copy link
Contributor

Re-reading to notice that this is some hefty feedback.
I'll try to break this down, but there are some items which I don't fully understand yet. Rigorous feedback from the other editors is highly appreciated here.

option-value-char  = "!" / "#" / "$" / "%" / "&" / "'" / "*"
                                / "+" / "-" / "." / "^" / "_" / "`" / "|" / "~" 
                                / DIGIT / ALPHA
                                ; any VCHAR, except delimiters
  • 3.8. CSP directive: What if integrity-policy block occurs in CSP report-only header? Maybe let CSP header decide if report/block and just use integrityas a CSP directive → remove for v1, file issue for vnext
  • 4. Proxies has clients add CC: no-transform in hopes that proxies won't modify the response body, but that's not how it works: http://httpwg.github.io/specs/rfc7230.html#message.transformations (no-transform only applies to the message it occurs within). (SRI: no need to add "Cache-Control: no-transform" #403)
  • Integrity data will be easier to add in some server-side scenarios if it's possible to convey it in HTTP headers on the response, rather than in the HTML (or CSS, or JS). For example
HTTP/1.1 200 OK
Content-Type: text/html
Link: <https://widgets.example.com/foowidget>; rel="stylesheet"; type="text/css"; integrity="..."

Also, CCing @mnot

@mozfreddyb
Copy link
Contributor

Another nit:

  • parse metadata (3.3.3) does not talk about options yet.

@devd
Copy link
Contributor Author

devd commented Mar 23, 2015

I think for the more complex issues, it is fine to create a new issue for discussion.

@fmarier
Copy link
Member

fmarier commented Mar 24, 2015

Regarding 3.8, are we going to drop the CSP directive from V1? It's not implemented in Firefox (or Chrome as far as I know).

mozfreddyb added a commit that referenced this issue Mar 24, 2015
@devd
Copy link
Contributor Author

devd commented Mar 24, 2015

Yeah we are
On Mar 24, 2015 1:58 AM, "Francois Marier" notifications@github.com
wrote:

Regarding 3.8, are we going to drop the CSP directive from V1? It's not
implemented in Firefox (or Chrome as far as I know).


Reply to this email directly or view it on GitHub
#217 (comment).

@joelweinberger
Copy link
Contributor

I just uploaded a pull request to address @mnot's suggestion to expand the option-value-chars. Note, however, that this pull request also adds "/" as a character since we hope to support MIME type's in the future, and those distinctly use "/". Let me know if you think I'm missing something by leaving "/" in.

@fmarier fmarier changed the title Respond to mnot comments SRI: Respond to mnot comments Apr 25, 2015
@devd
Copy link
Contributor Author

devd commented Jun 15, 2015

I resolved two more issues here because we are only relying on CORS now. Also, I don't think we are going to investigate the "support integrity metadata through headers" in v1.

@mnot I couldn't find examples of mistaken use of resource or capital case must/should/ etc. Do you have notes on the cases where you thought the spec was wrong?

I also don't understand your point about proxies/no-transform. The link seems to match what the spec wants to achieve: tell the proxy not to transform the body of the response? I am sure I am missing something but would be great if you can explain a bit more.

@mnot
Copy link
Member

mnot commented Jun 15, 2015

WRT two issues resolved due to CORS - which ones?

Re: must/may/should - I'll take another look and either point them out or make a pull.

About proxies and no-transform - the problem is that a request cannot use no-transform to indicate that the response body should be left along; for better or worse, the semantics are restricted to the message that they occur within.

@devd
Copy link
Contributor Author

devd commented Jun 15, 2015

I marked the following two as resolved:

3.2.2 ambiguity between response and request headers
3.2.2. http authentication and integrity?

We have switched the spec to be completely CORS based and removed any magic about making a decision on whether something is eligible for integrity based on headers or auth or anything like that.

Suggestions on what we should say about proxies/no-transform then? Also, for my own curiosity, what does "message they occur within" mean?

@fmarier
Copy link
Member

fmarier commented Jun 15, 2015

About proxies and no-transform - the problem is that a request cannot use no-transform to indicate that the response body should be left along; for better or worse, the semantics are restricted to the message that they occur within.

So with what we've got right now, we're instructing proxies not to modify the request body?

@mnot
Copy link
Member

mnot commented Jun 16, 2015

@fmarier - exactly so.

@devd
Copy link
Contributor Author

devd commented Jun 16, 2015

Aah OK. So we should just kill that note

Sent from my mobile device. Please forgive the brevity and tpyos.
On Jun 15, 2015 6:12 PM, "Mark Nottingham" notifications@github.com wrote:

@fmarier https://github.com/fmarier - exactly so.


Reply to this email directly or view it on GitHub
#217 (comment).

fmarier pushed a commit to fmarier/webappsec that referenced this issue Jun 16, 2015
This directive only applies to the request it's in. It doesn't
apply to the response, which was the whole point of us adding that
header.

w3c#217 (comment)
@annevk
Copy link
Member

annevk commented Jun 16, 2015

Great, that makes Fetch integration a lot easier too.

@devd
Copy link
Contributor Author

devd commented Jun 26, 2015

hey @mnot, friendly ping; this is the last issue for SRIv1 still open on our issue tracker.

mikewest pushed a commit to mikewest/webappsec that referenced this issue Jun 29, 2015
@mnot
Copy link
Member

mnot commented Jun 29, 2015

The RFC2119-happyness is just an editorial comment, and shouldn't block anything; re-skimming the spec, it doesn't seem as bad now.

WRT "resource" - most instances in the doc (there are currently 60) should be changed to "response." @annevk, thoughts?

@annevk
Copy link
Member

annevk commented Jun 29, 2015

Seems reasonable.

fmarier pushed a commit to fmarier/webappsec that referenced this issue Jul 3, 2015
This directive only applies to the request it's in. It doesn't
apply to the response, which was the whole point of us adding that
header.

w3c#217 (comment)
@devd
Copy link
Contributor Author

devd commented Jul 6, 2015

closing this out because of 7debd29 and bea9a06

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

7 participants