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

Specify browser behavior for CSP headers on 304 (not modified) responses #161

Closed
lweichselbaum opened this issue Dec 7, 2016 · 10 comments
Closed
Milestone

Comments

@lweichselbaum
Copy link

It seems that browser behave differently in case a CSP header is set on a 304 response.
E.g.:

  • Chrome uses the cached CSP header instead of the CSP header provided in the 304 response.
  • Firefox uses the CSP header provided in the 304 response instead of the cached CSP header.

In case of nonce based CSPs (which should change for every single response), Chrome's behavior doesn't break a site, if a CSP header with a new nonce is set on a 304.
While the preferred solution is probably to not set a CSP at all on 304s, it is likely that in practice not reusing the cached header will lead to (unexpected) breakages in a subset of browsers.

I think it would make sense to be consistent across browsers in handling CSP headers on 304s.

@annevk
Copy link
Member

annevk commented Dec 7, 2016

We should probably be clearer on this in Fetch somehow. I believe that what Firefox does is what's correct per HTTP and what browsers should align on. Copying @mnot and @reschke just in case.

@mikewest
Copy link
Member

mikewest commented Dec 8, 2016

I'm pretty sure @mnot has told me about this being wrong in Chrome (and probably Blink- && WebKit-based browsers in general), but I don't recall the details of the conversation. I'm also not sure it's a Fetch-level question but a HTTP-level question.

@annevk
Copy link
Member

annevk commented Dec 8, 2016

https://fetch.spec.whatwg.org/#concept-http-network-or-cache-fetch per step 21.3 Chrome is indeed wrong.

@mnot
Copy link
Member

mnot commented Feb 6, 2017

The headers in a 304 update a stored response. See:
http://httpwg.org/specs/rfc7234.html#freshening.responses

See also discussion in web-platform-tests/wpt#1400

@mikewest mikewest added this to the CSP3 CR milestone May 10, 2017
@andypaicu
Copy link
Collaborator

Reviving this issue, from what I gather from the links the correct behavior here is to use the new CSP header if it is set (Firefox behavior).

With regards to this breaking sites, I expect that the risk is not that large considering those sites would have been broken in Firefox anyway so this would only affect sites that are designed to only work in Chrome (or maybe in webkit/blink browsers?) that use CSP and specifically nonces and use 304 pages where they explicitly set CSP headers. Also the fix for these sites should not be particularly difficult as all it involves is making sure not to send a CSP header on 304 pages.

There is the linked closed and censored issue that seems to hint at some sort of security issue around CSP and 304 responses, anyone have any clue what that is about?

@annevk
Copy link
Member

annevk commented Oct 9, 2018

From the commit message it looks like a misunderstanding to me.

I suspect this also isn't tested yet.

@andypaicu
Copy link
Collaborator

So as far as I can tell the CSP spec does not need updating, this is all fetch/HTTP level.

Perhaps a note about this behavior is warranted to ensure everything is clear.

And adding WPT tests of course.

@annevk
Copy link
Member

annevk commented Oct 9, 2018

Yeah, agreed. I don't think a note is needed as this affects everything header-based. But tests do seem good.

@mnot
Copy link
Member

mnot commented Oct 12, 2018

Ah. See:
https://bugs.chromium.org/p/chromium/issues/detail?id=174301

I've been talking to @mikewest and crew about this in the background, need to write some tests to make them comfortable about changing it, I think.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 17, 2018
Also updated kNonUpdatedHeaders with more headers from the
nsHttpResponseHead file

Spec: https://fetch.spec.whatwg.org/#concept-http-network-or-cache-fetch
Spec issue: w3c/webappsec-csp#161

Bug: 174301
Change-Id: I8aab863b1f2733d051609e121539ad6acad36c6b
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 17, 2018
…ore on 304

Also updated kNonUpdatedHeaders with more headers from the
nsHttpResponseHead file

Spec: https://fetch.spec.whatwg.org/#concept-http-network-or-cache-fetch
Spec issue: w3c/webappsec-csp#161

Bug: 174301
Change-Id: I8aab863b1f2733d051609e121539ad6acad36c6b
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 23, 2018
…ore on 304

Also updated kNonUpdatedHeaders with more headers from the
nsHttpResponseHead file

Spec: https://fetch.spec.whatwg.org/#concept-http-network-or-cache-fetch
Spec issue: w3c/webappsec-csp#161

While the spec does not give any list of content headers that should be ignored
on a 304 request, some of them are directly dependent on the resource body and
as such should not be updated (for example `content-length` cannot be different
since the content remains identical).

The exact list of ignored headers is identical to the one that firefox uses.

Bug: 174301
Change-Id: I8aab863b1f2733d051609e121539ad6acad36c6b
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 23, 2018
…ore on 304

Also updated kNonUpdatedHeaders with more headers from the
nsHttpResponseHead file

Spec: https://fetch.spec.whatwg.org/#concept-http-network-or-cache-fetch
Spec issue: w3c/webappsec-csp#161

While the spec does not give any list of content headers that should be ignored
on a 304 request, some of them are directly dependent on the resource body and
as such should not be updated (for example `content-length` cannot be different
since the content remains identical).

The exact list of ignored headers is identical to the one that firefox uses.

Bug: 174301
Change-Id: I8aab863b1f2733d051609e121539ad6acad36c6b
aarongable pushed a commit to chromium/chromium that referenced this issue Oct 23, 2018
…ore on 304

Also updated kNonUpdatedHeaders with more headers from the
nsHttpResponseHead file

Spec: https://fetch.spec.whatwg.org/#concept-http-network-or-cache-fetch
Spec issue: w3c/webappsec-csp#161

While the spec does not give any list of content headers that should be ignored
on a 304 request, some of them are directly dependent on the resource body and
as such should not be updated (for example `content-length` cannot be different
since the content remains identical).

The exact list of ignored headers is identical to the one that firefox uses.

Bug: 174301
Change-Id: I8aab863b1f2733d051609e121539ad6acad36c6b
Reviewed-on: https://chromium-review.googlesource.com/c/1286427
Commit-Queue: Andy Paicu <andypaicu@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602001}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 23, 2018
…ore on 304

Also updated kNonUpdatedHeaders with more headers from the
nsHttpResponseHead file

Spec: https://fetch.spec.whatwg.org/#concept-http-network-or-cache-fetch
Spec issue: w3c/webappsec-csp#161

While the spec does not give any list of content headers that should be ignored
on a 304 request, some of them are directly dependent on the resource body and
as such should not be updated (for example `content-length` cannot be different
since the content remains identical).

The exact list of ignored headers is identical to the one that firefox uses.

Bug: 174301
Change-Id: I8aab863b1f2733d051609e121539ad6acad36c6b
Reviewed-on: https://chromium-review.googlesource.com/c/1286427
Commit-Queue: Andy Paicu <andypaicu@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602001}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 24, 2018
…ore on 304

Also updated kNonUpdatedHeaders with more headers from the
nsHttpResponseHead file

Spec: https://fetch.spec.whatwg.org/#concept-http-network-or-cache-fetch
Spec issue: w3c/webappsec-csp#161

While the spec does not give any list of content headers that should be ignored
on a 304 request, some of them are directly dependent on the resource body and
as such should not be updated (for example `content-length` cannot be different
since the content remains identical).

The exact list of ignored headers is identical to the one that firefox uses.

Bug: 174301
Change-Id: I8aab863b1f2733d051609e121539ad6acad36c6b
Reviewed-on: https://chromium-review.googlesource.com/c/1286427
Commit-Queue: Andy Paicu <andypaicu@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602001}
@andypaicu
Copy link
Collaborator

I've added tests (also fixed Chrome but that's probably orthogonal to this).

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Nov 10, 2018
…*" prefix from headers to ignore on 304, a=testonly

Automatic update from web-platform-testsAdded 304 CSP test and removed "content-*" prefix from headers to ignore on 304

Also updated kNonUpdatedHeaders with more headers from the
nsHttpResponseHead file

Spec: https://fetch.spec.whatwg.org/#concept-http-network-or-cache-fetch
Spec issue: w3c/webappsec-csp#161

While the spec does not give any list of content headers that should be ignored
on a 304 request, some of them are directly dependent on the resource body and
as such should not be updated (for example `content-length` cannot be different
since the content remains identical).

The exact list of ignored headers is identical to the one that firefox uses.

Bug: 174301
Change-Id: I8aab863b1f2733d051609e121539ad6acad36c6b
Reviewed-on: https://chromium-review.googlesource.com/c/1286427
Commit-Queue: Andy Paicu <andypaicu@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602001}

--

wpt-commits: 1b09004d92653856ca896d0f267d42bfda788434
wpt-pr: 13579
jyc pushed a commit to jyc/gecko that referenced this issue Nov 11, 2018
…*" prefix from headers to ignore on 304, a=testonly

Automatic update from web-platform-testsAdded 304 CSP test and removed "content-*" prefix from headers to ignore on 304

Also updated kNonUpdatedHeaders with more headers from the
nsHttpResponseHead file

Spec: https://fetch.spec.whatwg.org/#concept-http-network-or-cache-fetch
Spec issue: w3c/webappsec-csp#161

While the spec does not give any list of content headers that should be ignored
on a 304 request, some of them are directly dependent on the resource body and
as such should not be updated (for example `content-length` cannot be different
since the content remains identical).

The exact list of ignored headers is identical to the one that firefox uses.

Bug: 174301
Change-Id: I8aab863b1f2733d051609e121539ad6acad36c6b
Reviewed-on: https://chromium-review.googlesource.com/c/1286427
Commit-Queue: Andy Paicu <andypaicu@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602001}

--

wpt-commits: 1b09004d92653856ca896d0f267d42bfda788434
wpt-pr: 13579
oliverguenther added a commit to opf/openproject that referenced this issue Mar 7, 2019
Not all headers treat 304 responses equally, some do replay the CSP
headers from the original 200 GET, but we cannot rely on that.

If the header is not replayed, the frontend-cached nonces will not be
valid resulting in a CSP violation.

```
References:
w3c/webappsec-csp#161
https://bugs.chromium.org/p/chromium/issues/detail?id=174301
```

https://community.openproject.com/wp/29673
oliverguenther added a commit to opf/openproject that referenced this issue Mar 7, 2019
Not all headers treat 304 responses equally, some do replay the CSP
headers from the original 200 GET, but we cannot rely on that.

If the header is not replayed, the frontend-cached nonces will not be
valid resulting in a CSP violation.

```
References:
w3c/webappsec-csp#161
https://bugs.chromium.org/p/chromium/issues/detail?id=174301
```

https://community.openproject.com/wp/29673
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 3, 2019
…*" prefix from headers to ignore on 304, a=testonly

Automatic update from web-platform-testsAdded 304 CSP test and removed "content-*" prefix from headers to ignore on 304

Also updated kNonUpdatedHeaders with more headers from the
nsHttpResponseHead file

Spec: https://fetch.spec.whatwg.org/#concept-http-network-or-cache-fetch
Spec issue: w3c/webappsec-csp#161

While the spec does not give any list of content headers that should be ignored
on a 304 request, some of them are directly dependent on the resource body and
as such should not be updated (for example `content-length` cannot be different
since the content remains identical).

The exact list of ignored headers is identical to the one that firefox uses.

Bug: 174301
Change-Id: I8aab863b1f2733d051609e121539ad6acad36c6b
Reviewed-on: https://chromium-review.googlesource.com/c/1286427
Commit-Queue: Andy Paicu <andypaicuchromium.org>
Reviewed-by: Mike West <mkwstchromium.org>
Reviewed-by: Matt Menke <mmenkechromium.org>
Cr-Commit-Position: refs/heads/master{#602001}

--

wpt-commits: 1b09004d92653856ca896d0f267d42bfda788434
wpt-pr: 13579

UltraBlame original commit: c94869a31c0e382016b7c0c41c931560ebcfd9e6
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 3, 2019
…*" prefix from headers to ignore on 304, a=testonly

Automatic update from web-platform-testsAdded 304 CSP test and removed "content-*" prefix from headers to ignore on 304

Also updated kNonUpdatedHeaders with more headers from the
nsHttpResponseHead file

Spec: https://fetch.spec.whatwg.org/#concept-http-network-or-cache-fetch
Spec issue: w3c/webappsec-csp#161

While the spec does not give any list of content headers that should be ignored
on a 304 request, some of them are directly dependent on the resource body and
as such should not be updated (for example `content-length` cannot be different
since the content remains identical).

The exact list of ignored headers is identical to the one that firefox uses.

Bug: 174301
Change-Id: I8aab863b1f2733d051609e121539ad6acad36c6b
Reviewed-on: https://chromium-review.googlesource.com/c/1286427
Commit-Queue: Andy Paicu <andypaicuchromium.org>
Reviewed-by: Mike West <mkwstchromium.org>
Reviewed-by: Matt Menke <mmenkechromium.org>
Cr-Commit-Position: refs/heads/master{#602001}

--

wpt-commits: 1b09004d92653856ca896d0f267d42bfda788434
wpt-pr: 13579

UltraBlame original commit: c94869a31c0e382016b7c0c41c931560ebcfd9e6
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 3, 2019
…*" prefix from headers to ignore on 304, a=testonly

Automatic update from web-platform-testsAdded 304 CSP test and removed "content-*" prefix from headers to ignore on 304

Also updated kNonUpdatedHeaders with more headers from the
nsHttpResponseHead file

Spec: https://fetch.spec.whatwg.org/#concept-http-network-or-cache-fetch
Spec issue: w3c/webappsec-csp#161

While the spec does not give any list of content headers that should be ignored
on a 304 request, some of them are directly dependent on the resource body and
as such should not be updated (for example `content-length` cannot be different
since the content remains identical).

The exact list of ignored headers is identical to the one that firefox uses.

Bug: 174301
Change-Id: I8aab863b1f2733d051609e121539ad6acad36c6b
Reviewed-on: https://chromium-review.googlesource.com/c/1286427
Commit-Queue: Andy Paicu <andypaicuchromium.org>
Reviewed-by: Mike West <mkwstchromium.org>
Reviewed-by: Matt Menke <mmenkechromium.org>
Cr-Commit-Position: refs/heads/master{#602001}

--

wpt-commits: 1b09004d92653856ca896d0f267d42bfda788434
wpt-pr: 13579

UltraBlame original commit: c94869a31c0e382016b7c0c41c931560ebcfd9e6
aomarks added a commit to lit/lit.dev that referenced this issue Oct 1, 2021
- Handle 304 cases. The reason we were getting a lot of CSP errors since #532 was that we weren't accounting for 304 Not Modified responses. In those cases, no `Content-Type` header is set, because the browser will already know it from the most recent 200 response. Our middleware incorrectly assumed it could check `type` after awaiting the downstream Koa static middleware. We now instead set the policy based on the path of the request (anything ending in `/`), since that will work for both 200 and 304 responses.

   There's an interesting question about whether we *should* set a CSP at all for a 304 response. There's some discussion about that [here](w3c/webappsec-csp#161) and [here](https://bugs.chromium.org/p/chromium/issues/detail?id=174301). Chrome *does* update the CSP if it's included in a 304 response, otherwise it ignores it in favor of the CSP from the last 200 response (same with all headers, except for the ones enumerated [here](https://chromium.googlesource.com/chromium/src/net/+/refs/heads/main/http/http_response_headers.cc#81)).

    The advantage of including it seems to be that if we update the CSP, but the content (and hence ETag) of a file has *not* changed, then the browser would otherwise continue using the stale CSP. OTOH, if you are using a nonce based script allowlist (we use hashes instead currently), then this wouldn't really work unless you did something clever to make the ETag aware of some CSP version.

- Fix policy for Google Analytics. We covered the origin needed for the initial script, but then *that* script goes on to require a connection to a different origin.

- Hook up a new Google Analytics test ID that I just created, so that we will catch CSP reports related to Google Analytics during dev and in PR builds going forward.

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

No branches or pull requests

5 participants