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

Issue 19940002: [HTML Import] Respect Content Security Policy Model (Closed)

Created:
7 years, 5 months ago by Hajime Morrita
Modified:
7 years, 5 months ago
CC:
blink-reviews, eae+blinkwatch, dglazkov+blink, adamk+blink_chromium.org, Nate Chapin, mkwst+watchlist_chromium.org, gavinp+loader_chromium.org
Visibility:
Public.

Description

[HTML Import] Respect Content Security Policy Model See https://www.w3.org/Bugs/Public/show_bug.cgi?id=22752 for the expected behavior. This change teaches DocumentInit about Imports: - The security context of the imported document should be master's context (frame). - Each imported document should have its own CSP. It is enforced by the HTTP header which it is served with, for example. This change also teaches CachedResourceLoader about Imports. That is, imports should be loaded as if it is a script in terms of CSP. This change doesn't cover scripting on imported documents since it is yet to be implemented. We should test CSP cases when we implemented the scripting. BUG=240592 TEST=http/tests/htmlimports/csp-*.html R=abarth,dglazkov Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=154924

Patch Set 1 #

Patch Set 2 : Fix mac build failure. #

Total comments: 5

Patch Set 3 : Updated ToT, detailed the rationale for the ASSERT(). #

Patch Set 4 : Added ASSERTs #

Patch Set 5 : Fix Mac build #

Total comments: 4

Patch Set 6 : Switched to Per-import CSP model #

Total comments: 2

Patch Set 7 : Fixed a test, landing. #

Patch Set 8 : Landing. #

Patch Set 9 : Fix Mac build failure #

Unified diffs Side-by-side diffs Delta from patch set Stats (+304 lines, -51 lines) Patch
A LayoutTests/http/tests/htmlimports/csp-block-import.html View 1 chunk +19 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/htmlimports/csp-block-import-expected.txt View 1 chunk +4 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/htmlimports/csp-block-import-non-self.html View 1 chunk +25 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/htmlimports/csp-block-import-non-self-expected.txt View 1 chunk +5 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/htmlimports/csp-in-imports.html View 1 2 3 4 5 1 chunk +25 lines, -0 lines 0 comments Download
A + LayoutTests/http/tests/htmlimports/csp-in-imports-expected.txt View 1 2 3 4 5 0 chunks +-1 lines, --1 lines 0 comments Download
A LayoutTests/http/tests/htmlimports/csp-not-block-import-in-import.html View 1 2 3 4 5 6 1 chunk +19 lines, -0 lines 0 comments Download
A + LayoutTests/http/tests/htmlimports/csp-not-block-import-in-import-expected.txt View 1 2 3 4 5 0 chunks +-1 lines, --1 lines 0 comments Download
A LayoutTests/http/tests/htmlimports/resources/csp-blocking.cgi View 1 2 3 4 5 1 chunk +9 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/htmlimports/resources/having-csp-directive.html View 1 2 3 4 5 1 chunk +8 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/htmlimports/resources/importing-cors.html View 1 chunk +7 lines, -0 lines 0 comments Download
M Source/core/dom/Document.h View 1 2 3 4 5 2 chunks +2 lines, -1 line 0 comments Download
M Source/core/dom/Document.cpp View 1 2 3 4 5 6 3 chunks +8 lines, -6 lines 0 comments Download
M Source/core/dom/DocumentInit.h View 1 2 3 4 5 6 2 chunks +3 lines, -0 lines 0 comments Download
M Source/core/dom/DocumentInit.cpp View 1 2 3 1 chunk +17 lines, -2 lines 0 comments Download
M Source/core/html/HTMLImport.h View 1 2 3 4 5 6 2 chunks +4 lines, -0 lines 0 comments Download
M Source/core/html/HTMLImport.cpp View 1 chunk +10 lines, -0 lines 0 comments Download
M Source/core/html/HTMLImportsController.h View 1 2 3 4 5 6 4 chunks +7 lines, -2 lines 0 comments Download
M Source/core/html/HTMLImportsController.cpp View 1 2 3 4 5 6 7 8 10 chunks +40 lines, -16 lines 0 comments Download
M Source/core/loader/FrameLoader.cpp View 1 2 3 4 5 2 chunks +1 line, -17 lines 0 comments Download
M Source/core/loader/cache/CachedResource.h View 1 2 3 4 5 1 chunk +2 lines, -1 line 0 comments Download
M Source/core/loader/cache/ResourceFetcher.h View 1 2 3 4 5 2 chunks +2 lines, -0 lines 0 comments Download
M Source/core/loader/cache/ResourceFetcher.cpp View 1 2 3 4 5 6 15 chunks +42 lines, -7 lines 0 comments Download
M Source/core/page/ContentSecurityPolicy.h View 1 2 3 4 5 3 chunks +20 lines, -0 lines 0 comments Download
M Source/core/page/ContentSecurityPolicy.cpp View 1 2 3 4 5 3 chunks +27 lines, -1 line 0 comments Download

Messages

Total messages: 26 (0 generated)
Hajime Morrita
Hi Adam, could you take a look? This is a preparation for https://codereview.chromium.org/19762002/ which makes ...
7 years, 5 months ago (2013-07-22 11:22:51 UTC) #1
Mike West
A few drive-by comments: https://codereview.chromium.org/19940002/diff/4001/LayoutTests/http/tests/htmlimports/csp-block-import-in-import.html File LayoutTests/http/tests/htmlimports/csp-block-import-in-import.html (right): https://codereview.chromium.org/19940002/diff/4001/LayoutTests/http/tests/htmlimports/csp-block-import-in-import.html#newcode12 LayoutTests/http/tests/htmlimports/csp-block-import-in-import.html:12: if (window.target.import.querySelector('#cors').import == null) Forgive ...
7 years, 5 months ago (2013-07-22 14:39:42 UTC) #2
Hajime Morrita
Thanks for the view, Mike! On 2013/07/22 14:39:42, Mike West wrote: > A few drive-by ...
7 years, 5 months ago (2013-07-23 01:27:11 UTC) #3
Mike West
On 2013/07/23 01:27:11, morrita1 wrote: > https://codereview.chromium.org/19940002/diff/4001/Source/core/dom/DocumentInit.cpp#newcode57 > > Source/core/dom/DocumentInit.cpp:57: return > > frameForSecurityContext()->loader()->effectiveSandboxFlags(); > ...
7 years, 5 months ago (2013-07-23 05:07:45 UTC) #4
Hajime Morrita
On 2013/07/23 05:07:45, Mike West wrote: > On 2013/07/23 01:27:11, morrita1 wrote: > > > ...
7 years, 5 months ago (2013-07-23 07:02:10 UTC) #5
Hajime Morrita
How should I move this forward?
7 years, 5 months ago (2013-07-24 01:37:36 UTC) #6
dglazkov
On 2013/07/24 01:37:36, morrita1 wrote: > How should I move this forward? send a cupcake ...
7 years, 5 months ago (2013-07-24 02:53:23 UTC) #7
Hajime Morrita
On 2013/07/24 02:53:23, Dimitri Glazkov wrote: > On 2013/07/24 01:37:36, morrita1 wrote: > > How ...
7 years, 5 months ago (2013-07-24 02:55:54 UTC) #8
Hajime Morrita
Jokes aside, Adam, could you take a look when you have time? As you know, ...
7 years, 5 months ago (2013-07-24 06:00:42 UTC) #9
Mike West
Code change LGTM. For clarity, I'd appreciate you taking another pass at the tests to ...
7 years, 5 months ago (2013-07-24 09:54:39 UTC) #10
Mike West
Hrm. These should have come out with the last message. https://codereview.chromium.org/19940002/diff/20001/LayoutTests/http/tests/htmlimports/csp-ignored-in-import-expected.txt File LayoutTests/http/tests/htmlimports/csp-ignored-in-import-expected.txt (right): https://codereview.chromium.org/19940002/diff/20001/LayoutTests/http/tests/htmlimports/csp-ignored-in-import-expected.txt#newcode2 ...
7 years, 5 months ago (2013-07-24 09:55:21 UTC) #11
abarth-chromium
On 2013/07/24 06:00:42, morrita1 wrote: > Jokes aside, Adam, could you take a look when ...
7 years, 5 months ago (2013-07-24 18:29:55 UTC) #12
abarth-chromium
https://codereview.chromium.org/19940002/diff/20001/Source/core/dom/Document.cpp File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/19940002/diff/20001/Source/core/dom/Document.cpp#newcode4215 Source/core/dom/Document.cpp:4215: // This can occur via document.implementation.createDocument(). Does this occur ...
7 years, 5 months ago (2013-07-24 18:39:19 UTC) #13
abarth-chromium
I don't think there's any reason to combine the policies from the master document and ...
7 years, 5 months ago (2013-07-24 18:41:29 UTC) #14
Hajime Morrita
Adam, Mike, thanks for the review and sharing your thoughts! I find that I was ...
7 years, 5 months ago (2013-07-25 00:30:46 UTC) #15
Hajime Morrita
> > CSP already has such a concept: > https://dvcs.w3.org/hg/content-security-policy/raw-file/tip/csp-specification.dev.html#enforcing-multiple-policies. This is great to know! ...
7 years, 5 months ago (2013-07-25 00:42:06 UTC) #16
abarth-chromium
On 2013/07/25 00:30:46, morrita1 wrote: > I find that I was slightly confused between several ...
7 years, 5 months ago (2013-07-25 01:08:45 UTC) #17
Hajime Morrita
> > Sounds good. Okay, I updated the patch which does per-import CSP enforcement. Could ...
7 years, 5 months ago (2013-07-25 04:15:59 UTC) #18
abarth-chromium
LGTM https://codereview.chromium.org/19940002/diff/37001/Source/core/dom/DocumentInit.h File Source/core/dom/DocumentInit.h (right): https://codereview.chromium.org/19940002/diff/37001/Source/core/dom/DocumentInit.h#newcode63 Source/core/dom/DocumentInit.h:63: Looks like you've got an extra blank line ...
7 years, 5 months ago (2013-07-25 04:53:09 UTC) #19
Hajime Morrita
Adam, thanks for the instant review! On 2013/07/25 04:53:09, abarth wrote: > LGTM > > ...
7 years, 5 months ago (2013-07-25 05:48:33 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/morrita@chromium.org/19940002/44001
7 years, 5 months ago (2013-07-25 07:47:41 UTC) #21
commit-bot: I haz the power
Sorry for I got bad news for ya. Compile failed with a clobber build on ...
7 years, 5 months ago (2013-07-25 08:07:21 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/morrita@chromium.org/19940002/57001
7 years, 5 months ago (2013-07-25 08:19:31 UTC) #23
commit-bot: I haz the power
Failed to trigger a try job on mac_layout_rel HTTP Error 400: Bad Request
7 years, 5 months ago (2013-07-25 08:37:51 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/morrita@chromium.org/19940002/55002
7 years, 5 months ago (2013-07-25 08:38:03 UTC) #25
commit-bot: I haz the power
7 years, 5 months ago (2013-07-25 18:42:16 UTC) #26
Message was sent while issue was closed.
Change committed as 154924

Powered by Google App Engine
This is Rietveld 408576698