Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(183)

Issue 901903003: CSP: Adding the 'upgrade-insecure-requests' directive. (Closed)

Created:
5 years, 7 months ago by Mike West
Modified:
5 years, 7 months ago
CC:
blink-reviews, blink-reviews-dom_chromium.org, dglazkov+blink, eae+blinkwatch, gavinp+loader_chromium.org, Nate Chapin, mkwst+watchlist-csp_chromium.org, rwlbuis, sof, tyoshino+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

CSP: Adding the 'upgrade-insecure-requests' directive. This is an initial implementation of the upgrade mechanism specified in https://w3c.github.io/webappsec/specs/upgrade/. We don't have layout tests, as the upgrade intentionally doesn't touch the port, and we use excitingly interesting ports like 8080 and 8443, which mean that the resources won't load even after upgrade. Test coverage is provided by unit tests which verify that CSP sets the InsecureContentPolicy is correctly set for a document based on a given policy, and that RequestFetcher and DOMWebSocket use that policy information to upgrade URLs. The new directive is behind the "experimental csp features" flag, and is nowhere near shipping. Intent to Implement: https://groups.google.com/a/chromium.org/d/msg/blink-dev/rjeFL53OV4I/_NvMh0_qsWEJ BUG=455674 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189646

Patch Set 1 #

Total comments: 12

Patch Set 2 : Feedback. #

Total comments: 3

Patch Set 3 : Rearranging. #

Total comments: 5

Patch Set 4 : unittest #

Total comments: 5

Patch Set 5 : WebSockets + Tests. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+339 lines, -15 lines) Patch
M Source/core/core.gypi View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/dom/Document.cpp View 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/dom/DocumentInit.h View 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/dom/DocumentInit.cpp View 1 chunk +6 lines, -0 lines 0 comments Download
M Source/core/dom/SecurityContext.h View 1 3 chunks +11 lines, -0 lines 0 comments Download
M Source/core/dom/SecurityContext.cpp View 1 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/fetch/ResourceFetcher.h View 1 2 3 2 chunks +3 lines, -0 lines 0 comments Download
M Source/core/fetch/ResourceFetcher.cpp View 1 2 3 3 chunks +17 lines, -0 lines 0 comments Download
M Source/core/fetch/ResourceFetcherTest.cpp View 1 2 3 4 1 chunk +104 lines, -14 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 4 chunks +18 lines, -0 lines 0 comments Download
M Source/core/frame/csp/ContentSecurityPolicy.h View 1 2 3 4 4 chunks +8 lines, -0 lines 0 comments Download
M Source/core/frame/csp/ContentSecurityPolicy.cpp View 1 2 3 4 5 chunks +14 lines, -1 line 0 comments Download
A Source/core/frame/csp/ContentSecurityPolicyTest.cpp View 1 2 3 4 1 chunk +81 lines, -0 lines 0 comments Download
M Source/core/loader/FrameLoader.h View 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/loader/FrameLoader.cpp View 1 1 chunk +14 lines, -0 lines 0 comments Download
M Source/modules/websockets/DOMWebSocket.cpp View 1 2 3 4 2 chunks +8 lines, -0 lines 0 comments Download
M Source/modules/websockets/DOMWebSocketTest.cpp View 1 2 3 4 2 chunks +46 lines, -0 lines 0 comments Download

Messages

Total messages: 26 (4 generated)
Mike West
Jochen, Yoav, WDYT? https://codereview.chromium.org/901903003/diff/1/Source/core/fetch/ResourceFetcher.cpp File Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/901903003/diff/1/Source/core/fetch/ResourceFetcher.cpp#newcode725 Source/core/fetch/ResourceFetcher.cpp:725: url.setPort(8443); :( I think we need ...
5 years, 7 months ago (2015-02-05 12:35:01 UTC) #2
Yoav Weiss
All in all the code looks good, but I'm slightly afraid of being too "trigger-happy" ...
5 years, 7 months ago (2015-02-05 13:25:36 UTC) #3
Mike West
https://codereview.chromium.org/901903003/diff/1/Source/core/dom/SecurityContext.h File Source/core/dom/SecurityContext.h (right): https://codereview.chromium.org/901903003/diff/1/Source/core/dom/SecurityContext.h#newcode45 Source/core/dom/SecurityContext.h:45: InsecureContentIgnore = 0, On 2015/02/05 at 13:25:36, Yoav Weiss ...
5 years, 7 months ago (2015-02-05 13:39:34 UTC) #4
Yoav Weiss
https://codereview.chromium.org/901903003/diff/1/Source/core/dom/SecurityContext.h File Source/core/dom/SecurityContext.h (right): https://codereview.chromium.org/901903003/diff/1/Source/core/dom/SecurityContext.h#newcode45 Source/core/dom/SecurityContext.h:45: InsecureContentIgnore = 0, On 2015/02/05 13:39:34, Mike West wrote: ...
5 years, 7 months ago (2015-02-05 13:49:13 UTC) #5
Yoav Weiss
LGTM https://codereview.chromium.org/901903003/diff/20001/Source/core/dom/SecurityContext.h File Source/core/dom/SecurityContext.h (right): https://codereview.chromium.org/901903003/diff/20001/Source/core/dom/SecurityContext.h#newcode45 Source/core/dom/SecurityContext.h:45: InsecureContentDoNotUpgrade = 0, Even better :) https://codereview.chromium.org/901903003/diff/20001/Source/core/fetch/ResourceFetcher.cpp File ...
5 years, 7 months ago (2015-02-05 13:55:29 UTC) #6
Yoav Weiss
The comment in https://w3c.github.io/webappsec/specs/upgrade/#upgrade-request doesn't refer to ports 8000 and 8080. Is the move from ...
5 years, 7 months ago (2015-02-05 14:01:31 UTC) #7
Mike West
On 2015/02/05 at 13:55:29, yoav wrote: > LGTM > > https://codereview.chromium.org/901903003/diff/20001/Source/core/dom/SecurityContext.h > File Source/core/dom/SecurityContext.h (right): ...
5 years, 7 months ago (2015-02-05 14:01:42 UTC) #8
Mike West
https://codereview.chromium.org/901903003/diff/20001/Source/core/fetch/ResourceFetcher.cpp File Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/901903003/diff/20001/Source/core/fetch/ResourceFetcher.cpp#newcode724 Source/core/fetch/ResourceFetcher.cpp:724: if (url.port() == 8000 || url.port() == 8080) And, ...
5 years, 7 months ago (2015-02-05 14:02:52 UTC) #9
Mike West
Also also, you should tell me to send an intent to implement to blink-dev. :)
5 years, 7 months ago (2015-02-05 14:07:41 UTC) #10
Yoav Weiss
On 2015/02/05 14:07:41, Mike West wrote: > Also also, you should tell me to send ...
5 years, 7 months ago (2015-02-05 14:09:30 UTC) #11
Ryan Sleevi
This isn't hidden behind a features flag? https://codereview.chromium.org/901903003/diff/40001/Source/core/fetch/ResourceFetcher.cpp File Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/901903003/diff/40001/Source/core/fetch/ResourceFetcher.cpp#newcode907 Source/core/fetch/ResourceFetcher.cpp:907: if (m_document->insecureContentPolicy() ...
5 years, 7 months ago (2015-02-05 20:03:04 UTC) #14
Mike West
On 2015/02/05 at 20:03:04, rsleevi wrote: > This isn't hidden behind a features flag? It ...
5 years, 7 months ago (2015-02-05 20:19:08 UTC) #15
Mike West
Yoav, WDYT about the unittests here?
5 years, 7 months ago (2015-02-06 09:24:54 UTC) #16
Yoav Weiss
These layout tests look great, but I don't think they cover everything. Specifically, they do ...
5 years, 7 months ago (2015-02-06 09:58:36 UTC) #17
Mike West
Take another look? :) I added WebSocket support to make Ryan happy.
5 years, 7 months ago (2015-02-06 11:06:57 UTC) #18
Yoav Weiss
On 2015/02/06 11:06:57, Mike West wrote: > Take another look? :) I added WebSocket support ...
5 years, 7 months ago (2015-02-06 11:35:10 UTC) #19
Mike West
I'll land it to make sure something is in next week's Canary to play around ...
5 years, 7 months ago (2015-02-06 14:16:20 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/901903003/80001
5 years, 7 months ago (2015-02-06 14:16:34 UTC) #22
commit-bot: I haz the power
Committed patchset #5 (id:80001) as https://src.chromium.org/viewvc/blink?view=rev&revision=189646
5 years, 7 months ago (2015-02-06 14:19:39 UTC) #23
Yoav Weiss
On 2015/02/06 14:16:20, Mike West wrote: > I'll land it to make sure something is ...
5 years, 7 months ago (2015-02-06 14:19:50 UTC) #24
Ryan Sleevi
The websockets bits LGTM too, as it all does. Assuming it's flag protected :)
5 years, 7 months ago (2015-02-07 00:50:10 UTC) #25
Mike West
5 years, 7 months ago (2015-02-07 04:46:00 UTC) #26
Message was sent while issue was closed.
On 2015/02/07 at 00:50:10, rsleevi wrote:
> The websockets bits LGTM too, as it all does. Assuming it's flag protected :)

It's behind a flag, and the
`RuntimeEnabledFeatures::setExperimentalContentSecurityPolicyFeaturesEnabled(true);`
bits in `ContentSecurityPolicyTest.cpp` verify that behavior.

Powered by Google App Engine
This is Rietveld 408576698