Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(646)

Issue 1009583003: Add CSP header for resources with an active policy (Closed)

Created:
5 years, 9 months ago by estark
Modified:
5 years, 9 months ago
CC:
blink-reviews, gavinp+loader_chromium.org, Nate Chapin, mkwst+watchlist-csp_chromium.org, tyoshino+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Add CSP header for resources with an active policy This change sends a `CSP: active` header as specified in https://w3c.github.io/webappsec/specs/content-security-policy/#csp-request-header. The header is sent on resource requests which the policy would effect (e.g. not sent on images if there is no img-src or default-src in the policy). Also fixed misspelling in the name of |addClientHintsIfNecessary|. BUG=452819 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192293

Patch Set 1 #

Patch Set 2 : test tweaks #

Total comments: 8

Patch Set 3 : rebase #

Patch Set 4 : move addCSPHeader to FetchContext, and push decision into CSPDirectiveList #

Patch Set 5 : send CSP header for frames, workers, manifests as well #

Patch Set 6 : consider plugin-types for sending CSP header on plugin requests #

Total comments: 7

Patch Set 7 : add return statement to avoid compiler error #

Unified diffs Side-by-side diffs Delta from patch set Stats (+108 lines, -0 lines) Patch
A LayoutTests/http/tests/security/contentSecurityPolicy/csp-header-is-sent.html View 1 1 chunk +13 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/security/contentSecurityPolicy/resources/csp-header-is-sent.js View 1 chunk +17 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/security/contentSecurityPolicy/resources/test-csp-header.pl View 1 1 chunk +13 lines, -0 lines 0 comments Download
M Source/core/fetch/FetchContext.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/fetch/FetchContext.cpp View 1 2 3 1 chunk +4 lines, -0 lines 0 comments Download
M Source/core/fetch/ResourceFetcher.cpp View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/frame/csp/CSPDirectiveList.h View 1 2 3 4 2 chunks +3 lines, -0 lines 0 comments Download
M Source/core/frame/csp/CSPDirectiveList.cpp View 1 2 3 4 5 6 1 chunk +33 lines, -0 lines 0 comments Download
M Source/core/frame/csp/ContentSecurityPolicy.h View 1 2 3 4 2 chunks +3 lines, -0 lines 0 comments Download
M Source/core/frame/csp/ContentSecurityPolicy.cpp View 1 2 3 4 1 chunk +9 lines, -0 lines 0 comments Download
M Source/core/loader/FrameFetchContext.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/loader/FrameFetchContext.cpp View 1 2 3 4 1 chunk +10 lines, -0 lines 0 comments Download

Messages

Total messages: 20 (5 generated)
estark
Mike, Jochen, could you please take a look? Thanks.
5 years, 9 months ago (2015-03-13 19:23:36 UTC) #2
estark
On 2015/03/13 19:23:36, emily wrote: > Mike, Jochen, could you please take a look? Thanks. ...
5 years, 9 months ago (2015-03-13 19:24:59 UTC) #3
jochen (gone - plz use gerrit)
I wonder whether we should really always send the header, and not just if the ...
5 years, 9 months ago (2015-03-16 10:00:46 UTC) #4
Mike West
On 2015/03/16 at 10:00:46, jochen wrote: > I wonder whether we should really always send ...
5 years, 9 months ago (2015-03-16 10:38:53 UTC) #5
Mike West
(and "reply" doesn't send comments. :( ) https://codereview.chromium.org/1009583003/diff/20001/Source/core/fetch/ResourceFetcher.cpp File Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/1009583003/diff/20001/Source/core/fetch/ResourceFetcher.cpp#newcode876 Source/core/fetch/ResourceFetcher.cpp:876: void ResourceFetcher::addCSPHeaderIfNecessary(Resource::Type ...
5 years, 9 months ago (2015-03-16 10:39:11 UTC) #6
estark
Thanks Mike and Jochen. https://codereview.chromium.org/1009583003/diff/20001/Source/core/fetch/ResourceFetcher.cpp File Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/1009583003/diff/20001/Source/core/fetch/ResourceFetcher.cpp#newcode876 Source/core/fetch/ResourceFetcher.cpp:876: void ResourceFetcher::addCSPHeaderIfNecessary(Resource::Type type, FetchRequest& fetchRequest) ...
5 years, 9 months ago (2015-03-17 18:27:34 UTC) #7
Mike West
LGTM % test, thanks! (Sorry for the delayed review; Jochen had to poke me about ...
5 years, 9 months ago (2015-03-20 14:53:54 UTC) #8
estark
On 2015/03/20 14:53:54, Mike West wrote: > LGTM % test, thanks! (Sorry for the delayed ...
5 years, 9 months ago (2015-03-20 20:27:35 UTC) #9
estark
https://codereview.chromium.org/1009583003/diff/100001/LayoutTests/http/tests/security/contentSecurityPolicy/csp-header-is-sent.html File LayoutTests/http/tests/security/contentSecurityPolicy/csp-header-is-sent.html (right): https://codereview.chromium.org/1009583003/diff/100001/LayoutTests/http/tests/security/contentSecurityPolicy/csp-header-is-sent.html#newcode7 LayoutTests/http/tests/security/contentSecurityPolicy/csp-header-is-sent.html:7: <!-- This sets 'script_loaded' to true if the CSP ...
5 years, 9 months ago (2015-03-20 20:27:53 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1009583003/100001
5 years, 9 months ago (2015-03-20 20:28:43 UTC) #12
commit-bot: I haz the power
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/android_chromium_gn_compile_rel/builds/30247)
5 years, 9 months ago (2015-03-20 20:47:56 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1009583003/120001
5 years, 9 months ago (2015-03-20 20:53:29 UTC) #17
commit-bot: I haz the power
Committed patchset #7 (id:120001) as https://src.chromium.org/viewvc/blink?view=rev&revision=192293
5 years, 9 months ago (2015-03-20 22:51:29 UTC) #18
Mike West
https://codereview.chromium.org/1009583003/diff/100001/LayoutTests/http/tests/security/contentSecurityPolicy/csp-header-is-sent.html File LayoutTests/http/tests/security/contentSecurityPolicy/csp-header-is-sent.html (right): https://codereview.chromium.org/1009583003/diff/100001/LayoutTests/http/tests/security/contentSecurityPolicy/csp-header-is-sent.html#newcode7 LayoutTests/http/tests/security/contentSecurityPolicy/csp-header-is-sent.html:7: <!-- This sets 'script_loaded' to true if the CSP ...
5 years, 9 months ago (2015-03-21 11:33:39 UTC) #19
Mike West
5 years, 7 months ago (2015-05-15 09:10:20 UTC) #20
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/1136843011/ by mkwst@chromium.org.

The reason for reverting is: This header is not a "simple" header, and is
causing issues with CORS preflights. Reverting until we can work out a better
way to do it..

Powered by Google App Engine
This is Rietveld 408576698