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

CSP Request Header and CORS preflight fetch. #52

Closed
horo-t opened this Issue May 15, 2015 · 14 comments

Comments

6 participants
@horo-t

horo-t commented May 15, 2015

According to the CSP spec
https://w3c.github.io/webappsec/specs/content-security-policy/#csp-request-header

If the user agent monitors or enforces a policy that contains a directive that contains a source list, then the user agent MUST set a CSP Request Header when requesting cross-origin resources, as described in §3.4 The CSP HTTP Request Header.

But "CSP" is not a simple header
https://fetch.spec.whatwg.org/#simple-header

A simple header is a header whose name is either one of Accept, Accept-Language, and Content-Language, or whose name is Content-Type and value, once parsed, has a MIME type (ignoring parameters) that is one of application/x-www-form-urlencoded, multipart/form-data, and text/plain.

So when the user agent requests a cross-origin resource which CSP is set, it must send a CORS preflight fetch.

This means when we use CSP, we can't use CDN which doesn't support CORS preflight.

Is this my understanding correct?

@mikewest

This comment has been minimized.

Show comment
Hide comment
@mikewest

mikewest May 15, 2015

Member

That certainly seems terrible, and isn't what I intended with that addition. @annevk, wdyt?

Member

mikewest commented May 15, 2015

That certainly seems terrible, and isn't what I intended with that addition. @annevk, wdyt?

dstockwell pushed a commit to dstockwell/blink that referenced this issue May 15, 2015

Stop sending the 'CSP' header.
The 'CSP' header is causing CORS preflights when requesting cross-origin
resources, which is going to break a certain number of CDN-hosted
resources on sites that are using CSP. That's no good at all.

Dropping the header for the moment while we work out a reasonable
solution. Ideally, we'd just be reverting the whole implementation
(https://codereview.chromium.org/1009583003/), but since we're almost
certainly going to need to merge this back, this patch leaves most of
the machinery in place, but just neuters the "shouldSendCSPHeader"
check.

See whatwg/fetch#52 for discussion.

BUG=452819

Review URL: https://codereview.chromium.org/1142623002

git-svn-id: svn://svn.chromium.org/blink/trunk@195418 bbb929c8-8fbe-4397-9dbb-9b2b20218538
@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk May 18, 2015

Member

I see three problems. 1. It's not clear when this header is supposed to be set and therefore it's not clear whether e.g. service workers are supposed to be able to observe it. 2. This header can be set by fetch() and XMLHttpRequest because it is not prefixed with Sec- so you need to handle duplicates somehow when you set it. 3. It's not clear whether including this header cross-origin is a security risk or not and therefore whether we should preflight it. So far we have answered yes to this question.

Member

annevk commented May 18, 2015

I see three problems. 1. It's not clear when this header is supposed to be set and therefore it's not clear whether e.g. service workers are supposed to be able to observe it. 2. This header can be set by fetch() and XMLHttpRequest because it is not prefixed with Sec- so you need to handle duplicates somehow when you set it. 3. It's not clear whether including this header cross-origin is a security risk or not and therefore whether we should preflight it. So far we have answered yes to this question.

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Jul 16, 2015

Member

Ping!

Member

annevk commented Jul 16, 2015

Ping!

@annevk annevk added the needsinfo label Jul 17, 2015

@mikewest

This comment has been minimized.

Show comment
Hide comment
@mikewest

mikewest Jul 27, 2015

Member

The goal is to inform a page that any redirect they perform is potentially observable (in a binary "Happened" or "Didn't happen." sense). We plugged the whole which made the endpoint completely observable, so I've been less interested in figuring out this header, as it's less valuable than it used to be.

That said:

  1. It should reflect the state of the thing that's making the request, and therefore the thing that can observe redirects. That is, if a Document sends a request, and it passes through a service worker, the header should be present. If a SW intercepts a request from that Document, and then sends it's own request in order to fulfill it, then the header would be present iff the service worker had an active policy.
  2. That seems fine. The value of the header is immaterial; it's 1 at the moment, but could just as easily be 1,1,1,1,1,1 or lkjahfklj;dvsnf. That might change in the future if we actually use the header for anything interesting, but for now, presence is enough.
  3. I doubt it is, but, I also didn't think that an HTTPS header would be dangerous, so, what do I know?
Member

mikewest commented Jul 27, 2015

The goal is to inform a page that any redirect they perform is potentially observable (in a binary "Happened" or "Didn't happen." sense). We plugged the whole which made the endpoint completely observable, so I've been less interested in figuring out this header, as it's less valuable than it used to be.

That said:

  1. It should reflect the state of the thing that's making the request, and therefore the thing that can observe redirects. That is, if a Document sends a request, and it passes through a service worker, the header should be present. If a SW intercepts a request from that Document, and then sends it's own request in order to fulfill it, then the header would be present iff the service worker had an active policy.
  2. That seems fine. The value of the header is immaterial; it's 1 at the moment, but could just as easily be 1,1,1,1,1,1 or lkjahfklj;dvsnf. That might change in the future if we actually use the header for anything interesting, but for now, presence is enough.
  3. I doubt it is, but, I also didn't think that an HTTPS header would be dangerous, so, what do I know?
@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Jul 27, 2015

Member

If we don't think this is needed anymore, happy to not add it. Will leave it open so we can reconsider when CSP is rewritten.

Member

annevk commented Jul 27, 2015

If we don't think this is needed anymore, happy to not add it. Will leave it open so we can reconsider when CSP is rewritten.

dstockwell pushed a commit to dstockwell/chromium that referenced this issue Sep 23, 2015

Stop sending the 'CSP' header.
The 'CSP' header is causing CORS preflights when requesting cross-origin
resources, which is going to break a certain number of CDN-hosted
resources on sites that are using CSP. That's no good at all.

Dropping the header for the moment while we work out a reasonable
solution. Ideally, we'd just be reverting the whole implementation
(https://codereview.chromium.org/1009583003/), but since we're almost
certainly going to need to merge this back, this patch leaves most of
the machinery in place, but just neuters the "shouldSendCSPHeader"
check.

See whatwg/fetch#52 for discussion.

BUG=452819

Review URL: https://codereview.chromium.org/1142623002

git-svn-id: svn://svn.chromium.org/blink/trunk@195418 bbb929c8-8fbe-4397-9dbb-9b2b20218538
@tyoshino

This comment has been minimized.

Show comment
Hide comment
@tyoshino

tyoshino Feb 18, 2016

Member

There's a similar known issue regarding the EventSource (https://html.spec.whatwg.org/multipage/comms.html#last-event-id). It sends out Last-Event-ID. It's currently treated as a non-simple header and Chromium's ServiceWorker would issue a preflight for it (cc/ @horo-t).

In a different context, we're about to exempt the Save-Data header (http://httpwg.org/http-extensions/client-hints.html#the-save-data-hint, https://code.google.com/p/chromium/issues/detail?id=584889). It's currently experimental and specific to Chromium but planned to be standardized.

There could be more stuffs like this coming that are intended to be observable/alterable by a ServiceWorker.

For the Last-Event-ID, I think we can just add it to the simple header list and leave it open to XHR/Fetch (i.e. leave it not forbidden).

Member

tyoshino commented Feb 18, 2016

There's a similar known issue regarding the EventSource (https://html.spec.whatwg.org/multipage/comms.html#last-event-id). It sends out Last-Event-ID. It's currently treated as a non-simple header and Chromium's ServiceWorker would issue a preflight for it (cc/ @horo-t).

In a different context, we're about to exempt the Save-Data header (http://httpwg.org/http-extensions/client-hints.html#the-save-data-hint, https://code.google.com/p/chromium/issues/detail?id=584889). It's currently experimental and specific to Chromium but planned to be standardized.

There could be more stuffs like this coming that are intended to be observable/alterable by a ServiceWorker.

For the Last-Event-ID, I think we can just add it to the simple header list and leave it open to XHR/Fetch (i.e. leave it not forbidden).

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Feb 18, 2016

Member

@tyoshino in part it really depends on when the header is set. Is it set at the "feature level", the "fetch level", the "service worker level", or the "just prior to network level".

The theoretical security problem with adding headers to the simple header list is that they can then be set to any value, thereby potentially compromising the security of a server which would not be affected otherwise (since it assumes that for logged in users the value will always be correct). I would love to figure out what the best solution for that is. @igrigorik wants to know it too I think. And so does @jakearchibald.

Member

annevk commented Feb 18, 2016

@tyoshino in part it really depends on when the header is set. Is it set at the "feature level", the "fetch level", the "service worker level", or the "just prior to network level".

The theoretical security problem with adding headers to the simple header list is that they can then be set to any value, thereby potentially compromising the security of a server which would not be affected otherwise (since it assumes that for logged in users the value will always be correct). I would love to figure out what the best solution for that is. @igrigorik wants to know it too I think. And so does @jakearchibald.

@tyoshino

This comment has been minimized.

Show comment
Hide comment
@tyoshino

tyoshino Feb 18, 2016

Member

Yeah. Regarding Last-Event-ID, I'm considering that it's in "feature level". Me too love to figure out the right way.

My initial impression for Save-Data was that it should live in "just prior to network level", but seems Ben Greenstein (Data reduction proxy developer) and @igrigorik's idea is that it should be observable by SW. Thanks for looping him in.

Member

tyoshino commented Feb 18, 2016

Yeah. Regarding Last-Event-ID, I'm considering that it's in "feature level". Me too love to figure out the right way.

My initial impression for Save-Data was that it should live in "just prior to network level", but seems Ben Greenstein (Data reduction proxy developer) and @igrigorik's idea is that it should be observable by SW. Thanks for looping him in.

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Feb 18, 2016

Member

Agreed about Last-Event-ID being feature level and that does indeed argue for it being a "simple header", but that also creates the problem I illustrated. Up until now servers could assume it contains an ID they controlled, and the moment it becomes a "simple header" it can be any value an attacker wants.

What does Chrome's network security team think about that? I'm also not sure what Mozilla thinks on this matter, FWIW. It's always a little muddy on the edges of the same-origin policy.

Member

annevk commented Feb 18, 2016

Agreed about Last-Event-ID being feature level and that does indeed argue for it being a "simple header", but that also creates the problem I illustrated. Up until now servers could assume it contains an ID they controlled, and the moment it becomes a "simple header" it can be any value an attacker wants.

What does Chrome's network security team think about that? I'm also not sure what Mozilla thinks on this matter, FWIW. It's always a little muddy on the edges of the same-origin policy.

@igrigorik

This comment has been minimized.

Show comment
Hide comment
@igrigorik

igrigorik Feb 19, 2016

Member

Related Client-Hint threads:

FWIW, being able to observe CH headers within SW is an important use case. Similarly, requests initiated by fetch() and within SW should advertise appropriate UA-set CH headers.

Does it make sense to allow UA-set headers to be treated as "simple headers", and trigger a preflight if set or modified by the developer?

Member

igrigorik commented Feb 19, 2016

Related Client-Hint threads:

FWIW, being able to observe CH headers within SW is an important use case. Similarly, requests initiated by fetch() and within SW should advertise appropriate UA-set CH headers.

Does it make sense to allow UA-set headers to be treated as "simple headers", and trigger a preflight if set or modified by the developer?

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Feb 19, 2016

Member

That would make headers more complex than they currently are, or require a side table I suppose. (Alternatively, you could go with validation and allow both developers and user agents the same kind of syntax.) None of these is very attractive, but perhaps that is the best option?

Member

annevk commented Feb 19, 2016

That would make headers more complex than they currently are, or require a side table I suppose. (Alternatively, you could go with validation and allow both developers and user agents the same kind of syntax.) None of these is very attractive, but perhaps that is the best option?

@igrigorik

This comment has been minimized.

Show comment
Hide comment
@igrigorik

igrigorik Feb 20, 2016

Member

Hmm, well.. The CH spec does specify the BNF for each header. It doesn't seem like a far stretch to make UA require a validation step for the provided values?

Member

igrigorik commented Feb 20, 2016

Hmm, well.. The CH spec does specify the BNF for each header. It doesn't seem like a far stretch to make UA require a validation step for the provided values?

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Feb 20, 2016

Member

We could I suppose, if user agents implement it... So it'd be something like:

'A simple header is ... or a header whose name is "CH header 1", "CH header 2", or ... and whose value, once parsed, is not failure.'

Member

annevk commented Feb 20, 2016

We could I suppose, if user agents implement it... So it'd be something like:

'A simple header is ... or a header whose name is "CH header 1", "CH header 2", or ... and whose value, once parsed, is not failure.'

igrigorik added a commit to igrigorik/fetch that referenced this issue Mar 22, 2016

treat Last-Event-ID and Client-Hints headers as simple headers
- Last-Event-ID is defined in the HTML spec as a utf-8 string, hence we
  don't place any restrictions on its value
- Each Client Hints header has a BNF grammar that should be validated by
  the user agent; the header is defined as simple if the name of the
  header matches one of CH header names, and its value conforms to the
  defined grammar

Closes:
- whatwg#52
- httpwg/http-extensions#141

igrigorik added a commit to igrigorik/fetch that referenced this issue Apr 5, 2016

integrate Client Hints with Fetch
- DPR, Save-Data, Viewport-Width are sent on navigation requests
- Subresource requests are subject to the set client hints policy
- Client Hints headers are treated as simple headers: each Client Hints
  heaer has a BNF grammar that should be validated bythe user agent.

Closes:
- whatwg#52
- httpwg/http-extensions#141

igrigorik added a commit to igrigorik/fetch that referenced this issue Apr 5, 2016

integrate Client Hints with Fetch
- DPR, Save-Data, Viewport-Width are sent on navigation requests
- Subresource requests are subject to the set client hints policy
- Client Hints headers are treated as simple headers: each Client Hints
  heaer has a BNF grammar that should be validated bythe user agent.

Closes:
- whatwg#52
- httpwg/http-extensions#141

igrigorik added a commit to igrigorik/fetch that referenced this issue Apr 8, 2016

integrate Client Hints with Fetch
- DPR, Save-Data, Viewport-Width are sent on navigation requests
- Subresource requests are subject to the set client hints policy
- Client Hints headers are treated as simple headers: each Client Hints
  heaer has a BNF grammar that should be validated bythe user agent.

Closes:
- whatwg#52
- httpwg/http-extensions#141

@annevk annevk closed this in 404dc3a Apr 10, 2016

@wanderview

This comment has been minimized.

Show comment
Hide comment
@wanderview

wanderview Apr 10, 2016

Member

I'm not sure what plans we have, if any, for Client-Hints, but I filed this gecko bug:

https://bugzilla.mozilla.org/show_bug.cgi?id=1263446

Member

wanderview commented Apr 10, 2016

I'm not sure what plans we have, if any, for Client-Hints, but I filed this gecko bug:

https://bugzilla.mozilla.org/show_bug.cgi?id=1263446

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment