Added is_insecure_upgrade field to structures. [chromium/src : master]
Carlos IL (Gerrit)
Carlos IL would like Mike West to review this change.
Added is_insecure_upgrade field to structures.
Added flag to structures that will need it as part of
go/upgrade-insecure-redirects
Bug: 615885
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: Ia17955e9fb2388cbb514433ccf140f5749d5b89e
---
M content/common/navigation_params.h
M net/url_request/url_request.h
M services/network/public/cpp/network_ipc_param_traits.h
M services/network/public/cpp/resource_request.h
M third_party/blink/public/platform/web_url_request.h
M third_party/blink/renderer/platform/exported/web_url_request.cc
M third_party/blink/renderer/platform/loader/fetch/resource_request.cc
M third_party/blink/renderer/platform/loader/fetch/resource_request.h
8 files changed, 45 insertions(+), 1 deletion(-)
To view, visit change 1067846. To unsubscribe, or for help writing mail filters, visit settings.
Carlos IL (Gerrit)
mkwst: These are the changes to add the field we'll use for go/upgrade-insecure-redirects, can you PTAL? I'll send this to other owners once it LGTY. Thanks!
Patch set 3:Commit-Queue +1
Mike West (Gerrit)
LGTM % naming nits. Thanks!
Patch set 3:Code-Review +1
4 comments:
File content/common/navigation_params.h:
Patch Set #3, Line 318:
is the requestNit: Remove?
Patch Set #3, Line 318:
(and redirects of it)Nit: "(including redirects)"?
Nit: I'd point go "Upgrade-Insecure-Requests" instead of "CSP".
Patch Set #3, Line 320:
is_insecure_upgradeHow about "upgrade_if_insecure", here and elsewhere?
To view, visit change 1067846. To unsubscribe, or for help writing mail filters, visit settings.
Carlos IL (Gerrit)
Thanks for the review! Flag names have been changed.
4 comments:
File content/common/navigation_params.h:
Patch Set #3, Line 318:
(including redNit: Remove?
Done, thanks for the catch, this was a copy/paste error.
Patch Set #3, Line 318:
rects) should beNit: "(including redirects)"?
Done
Nit: I'd point go "Upgrade-Insecure-Requests" instead of "CSP".
Done
Patch Set #3, Line 320:
upgrade_if_insecure
How about "upgrade_if_insecure", here and elsewhere?
Done
To view, visit change 1067846. To unsubscribe, or for help writing mail filters, visit settings.
Carlos IL (Gerrit)
clamy: Can you review the changes to //content/common/navigation_params.h? Thanks!
mmenke: Can you review the changes under //net and //services/network? Thanks!
Patch set 4:Commit-Queue +1
Matt Menke (Gerrit)
I don't think we should be hooking stuff up that doesn't actually work, as that makes code impossible to test, and if something is never finally hooked up, leads others to make incorrect assumptions about the code (See, for instance, URLRequest::GetFullRequestHeaders, which doesn't work for QUIC or H2, but has a bunch of consumers).
Carlos IL (Gerrit)
Patch Set 4:
I don't think we should be hooking stuff up that doesn't actually work, as that makes code impossible to test, and if something is never finally hooked up, leads others to make incorrect assumptions about the code (See, for instance, URLRequest::GetFullRequestHeaders, which doesn't work for QUIC or H2, but has a bunch of consumers).
I separated this into 4 CLs to make review easier. All 4 are in the relation change and ready at this point (% updating the downstream ones for the naming changes here, and re-enabling tests on the last one). Would you prefer if I merge them into one? It might end up being a large-ish CL if I did that though, alternatively I can keep them separate but not land them until I get all LGTMs and then land them all together.
Matt Menke (Gerrit)
Patch Set 4:
Patch Set 4:
I don't think we should be hooking stuff up that doesn't actually work, as that makes code impossible to test, and if something is never finally hooked up, leads others to make incorrect assumptions about the code (See, for instance, URLRequest::GetFullRequestHeaders, which doesn't work for QUIC or H2, but has a bunch of consumers).
I separated this into 4 CLs to make review easier. All 4 are in the relation change and ready at this point (% updating the downstream ones for the naming changes here, and re-enabling tests on the last one). Would you prefer if I merge them into one? It might end up being a large-ish CL if I did that though, alternatively I can keep them separate but not land them until I get all LGTMs and then land them all together.
I'm fine with them merged, or re-ordered so they could each test what they add.
Matt Menke (Gerrit)
Patch Set 4:
Patch Set 4:
Patch Set 4:
I don't think we should be hooking stuff up that doesn't actually work, as that makes code impossible to test, and if something is never finally hooked up, leads others to make incorrect assumptions about the code (See, for instance, URLRequest::GetFullRequestHeaders, which doesn't work for QUIC or H2, but has a bunch of consumers).
I separated this into 4 CLs to make review easier. All 4 are in the relation change and ready at this point (% updating the downstream ones for the naming changes here, and re-enabling tests on the last one). Would you prefer if I merge them into one? It might end up being a large-ish CL if I did that though, alternatively I can keep them separate but not land them until I get all LGTMs and then land them all together.
I'm fine with them merged, or re-ordered so they could each test what they add.
(i.e., the one that adds this bit to URLRequest should also have a test to make sure the bit is respected)
Carlos IL (Gerrit)
Carlos IL uploaded patch set #6 to this change.
upgrade-insecure-requests is now obeyed for non-navigation redirects.
This cl implements go/upgrade-insecure-redirects, a flag is set for
resource requests that should be upgraded to HTTPS, the flag is then
passed to //net and //net performs the upgrade if at any point the
request is redirected to HTTP.
Bug: 615885
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: Ia17955e9fb2388cbb514433ccf140f5749d5b89e
---
M content/browser/frame_host/navigation_request.cc
M content/browser/loader/navigation_url_loader_impl.cc
M content/browser/loader/resource_dispatcher_host_impl.cc
M content/common/navigation_params.h
M content/renderer/loader/web_url_loader_impl.cc
M net/url_request/url_request.h
M net/url_request/url_request_job.cc
M services/network/public/cpp/network_ipc_param_traits.h
M services/network/public/cpp/resource_request.h
M services/network/url_loader.cc
M third_party/WebKit/LayoutTests/TestExpectations
D third_party/WebKit/LayoutTests/external/wpt/upgrade-insecure-requests/image-redirect-upgrade.https-expected.txt
M third_party/blink/public/platform/web_url_request.h
M third_party/blink/renderer/core/loader/frame_loader.cc
M third_party/blink/renderer/platform/exported/web_url_request.cc
M third_party/blink/renderer/platform/loader/fetch/resource_request.cc
M third_party/blink/renderer/platform/loader/fetch/resource_request.h
17 files changed, 73 insertions(+), 55 deletions(-)
To view, visit change 1067846. To unsubscribe, or for help writing mail filters, visit settings.
Carlos IL (Gerrit)
Squashed this with downstream CL's as per mmenke's comment.
mmenke and clamy: Now that this is merged with the implementation code, this needs unit tests, please hold off on reviewing. I'll ping the CL again once it's ready for review. Thanks!
mkwst: Since this now includes the whole implementation, I'll also ping you for a re-review once the tests are ready. Thanks!
Mike West (Gerrit)
Patch Set 6:
Squashed this with downstream CL's as per mmenke's comment.
mmenke and clamy: Now that this is merged with the implementation code, this needs unit tests, please hold off on reviewing. I'll ping the CL again once it's ready for review. Thanks!
mkwst: Since this now includes the whole implementation, I'll also ping you for a re-review once the tests are ready. Thanks!
Thanks! Dropping the LGTM for the time being, excited to review once you've got everything in place. :)
Patch set 6:-Code-Review
Matt Menke (Gerrit)
Patch Set 6: -Code-Review
Patch Set 6:
Squashed this with downstream CL's as per mmenke's comment.
mmenke and clamy: Now that this is merged with the implementation code, this needs unit tests, please hold off on reviewing. I'll ping the CL again once it's ready for review. Thanks!
mkwst: Since this now includes the whole implementation, I'll also ping you for a re-review once the tests are ready. Thanks!
Thanks! Dropping the LGTM for the time being, excited to review once you've got everything in place. :)
Just to make it easier for myself to keep track of my pending reviews, I'm going to remove myself from this CL. Please add me back when it's read for review (And thanks for merging the CLs! - sorry to be a stickler here)
Carlos IL (Gerrit)
Carlos IL uploaded patch set #8 to this change.
upgrade-insecure-requests is now obeyed for non-navigation redirects.
This cl implements go/upgrade-insecure-redirects, a flag is set for
resource requests that should be upgraded to HTTPS, the flag is then
passed to //net and //net performs the upgrade if at any point the
request is redirected to HTTP.
This only covers enforcement of the CSP, it does not cover reporting,
which will be done in a separate CL.
Bug: 615885
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: Ia17955e9fb2388cbb514433ccf140f5749d5b89e
---
M content/browser/frame_host/navigation_request.cc
M content/browser/loader/navigation_url_loader_impl.cc
M content/browser/loader/navigation_url_loader_impl_unittest.cc
M content/browser/loader/resource_dispatcher_host_impl.cc
M content/common/navigation_params.h
M content/renderer/loader/web_url_loader_impl.cc
A content/test/data/redirect301-to-http
A content/test/data/redirect301-to-http.mock-http-headers
M net/BUILD.gn
M net/url_request/url_request.cc
M net/url_request/url_request.h
M net/url_request/url_request_job.cc
M net/url_request/url_request_test_util.cc
M net/url_request/url_request_test_util.h
M net/url_request/url_request_unittest.cc
M services/network/public/cpp/network_ipc_param_traits.h
M services/network/public/cpp/resource_request.h
M services/network/url_loader.cc
M third_party/WebKit/LayoutTests/TestExpectations
D third_party/WebKit/LayoutTests/external/wpt/upgrade-insecure-requests/image-redirect-upgrade.https-expected.txt
M third_party/blink/public/platform/web_url_request.h
M third_party/blink/renderer/core/loader/frame_loader.cc
M third_party/blink/renderer/platform/exported/web_url_request.cc
M third_party/blink/renderer/platform/loader/fetch/resource_request.cc
M third_party/blink/renderer/platform/loader/fetch/resource_request.h
M third_party/blink/renderer/platform/loader/fetch/resource_request_test.cc
26 files changed, 186 insertions(+), 57 deletions(-)
To view, visit change 1067846. To unsubscribe, or for help writing mail filters, visit settings.
Carlos IL (Gerrit)
Carlos IL removed Camille Lamy from this change.
To view, visit change 1067846. To unsubscribe, or for help writing mail filters, visit settings.
Carlos IL (Gerrit)
Carlos IL removed Mike West from this change.
To view, visit change 1067846. To unsubscribe, or for help writing mail filters, visit settings.
Carlos IL (Gerrit)
I added unit tests now, so the CL is ready.
mmenke: Can you review the changes under //net and //services/network? Thanks!
clamy: Can you review the changes under //content? Thanks!
mkwst: Can you (re-)review the changes under //third_party/blink and //third_party/WebKit? Thanks!
Patch set 8:Commit-Queue +1
Carlos IL (Gerrit)
I added unit tests now, so the CL is ready.
mmenke: Can you review the changes under //net and //services/network? Thanks!
clamy: Can you review the changes under //content? Thanks!
mkwst: Can you (re-)review the changes under //third_party/blink and //third_party/WebKit? Thanks!
Matt Menke (Gerrit)
7 comments:
File net/url_request/url_request_job.cc:
Patch Set #9, Line 394:
IsRedirectResponseMove this logic into IsRedirectResponse? (The two overloads of that method aren't relevant here, and are going away anyways)
Patch Set #9, Line 395:
upgrade_if_insecureI assume things break if we redirect to HTTP, and then re-redirect to HTTPs? Just a bit concerned about giving extensions the redirect header that we're not actually respecting.
What about WebSockets? Do we even support redirects with websockets?
Patch Set #9, Line 396:
schemenew_location.SchemeIs("http").
I don't believe this is needed - if we're using the default port with HTTP, we'll switch to use the default port with HTTPS.
File net/url_request/url_request_unittest.cc:
We should have a test that uses the default port, which means using URLrequestFilter. Using a url_request_mock_data_job.h with mock a mock header files (To make it a redirect) is probably the simplest way to do it. It looks like net/url_request/report_sender_unittest.cc is the only test to use that class in net/, though there are browser test examples as well.
Patch Set #9, Line 12664:
TestURLRequestContextWithProxyWe shouldn't be relying on a proxy to send us redirects (Why are we getting redirects from it, anyways? Not seeing how they're hooked up)
To view, visit change 1067846. To unsubscribe, or for help writing mail filters, visit settings.
Carlos IL (Gerrit)
Thanks for the review! Replies inline
Patch set 12:Commit-Queue +1
7 comments:
File net/url_request/url_request_job.cc:
Patch Set #9, Line 394:
est_->response_infMove this logic into IsRedirectResponse? (The two overloads of that method aren't relevant here, an […]
Done
Patch Set #9, Line 395:
&request_->responseI assume things break if we redirect to HTTP, and then re-redirect to HTTPs? Just a bit concerned a […]
I'm not sure I fully understand the comment, but the way things work for redirects is
If https://A redirects to http://B which redirects to https://C, the intermediate redirect to http://B will be upgraded to https://B, if https://B is not reachable, then that would break that redirect (which is what the spec says we should do). I'm not sure if I addressed your concern though, I'm also not sure how this would change for the header passed to extensions, wouldn't they get the one with the new_location?
new_location.SchemeIs("http").
Done
What about WebSockets? Do we even support redirects with websockets?
I'm not sure where the ws to wss upgrade happens for non-redirects (i.e. the code I pointed to in the other comment does only seem to handle http to https, and I couldn't find a similar spot for wss), so this CL only covers http to https. I pinged mkwst in the design doc about ws upgrades, once I figure out where that upgrade happens, I'll implement it for redirects on a separate CL.
I don't believe this is needed - if we're using the default port with HTTP, we'll switch to use the […]
The reason why this is added is for urls that are explicitely set to port 80 (i.e. without it, http://example.com would be upgraded to https://example.com, and the default port correctly upgraded to 443, but http://example.com:80 would be upgraded to https://example.com:80, which is not what the spec says should happen).
Here's the analogous code for non-redirect upgrades: https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/loader/frame_loader.cc?rcl=bf822f7d4dc10cf2fc9cf3e7d1e6dfbc53a560c1&l=1810
File net/url_request/url_request_unittest.cc:
We should have a test that uses the default port, which means using URLrequestFilter. […]
I did this with a URLRequestMockHTTPJob (which is also using the default port), but to make it a redirect I used a network delegate with a redirect url set since that seems more consistent with other redirect tests in this file, and the test is a bit simpler. LMK what you think.
Patch Set #9, Line 12664:
network_delegate.set_redirect_uWe shouldn't be relying on a proxy to send us redirects (Why are we getting redirects from it, anywa […]
Thanks for catching this, this was a copy/paste leftover from NetworkDelegateRedirectRequest, changed it to a regular TestURLRequestContext now (what causes the redirect is the network_delegate, so the proxy is not needed).
To view, visit change 1067846. To unsubscribe, or for help writing mail filters, visit settings.
Matt Menke (Gerrit)
4 comments:
File net/url_request/url_request_job.cc:
Patch Set #9, Line 395:
&request_->responseI'm not sure I fully understand the comment, but the way things work for redirects is […]
Extensions will see the response headers here...Which indicate we're being redirect to http://B (They may also see the URL we're actually being redirected to, not sure, but they definitely see the headers we're not actually obeying). One way to way to fix that would be to, in that case, have https://A redirect to http://B, and then have that redirect to https://B (Using a bogus URLRequestRedirectJob), which redirects to https://C. i.e., instead of obeying the setting at redirect time, obey it when there's an http request. That would also mean request http://A with the header set would automatically redirect to https://A.
Not sure if that has any problematic side effects.
I'm not sure where the ws to wss upgrade happens for non-redirects (i.e. […]
SGTM.
File net/url_request/url_request_job.cc:
Patch Set #13, Line 204:
replacements.SetPortStr("443");Per earlier comment, this isn't needed. GURL eats explicitly specified default ports. If you think I'm mistaken, please write a test that fails if this line isn't present (And make sure it fails without this line).
File net/url_request/url_request_unittest.cc:
Patch Set #13, Line 12709:
simpleOh, we're forcing a redirect via the delegate. These don't even need to be a real GURLs. Can get rid of URLRequestMockHTTPJob entirely, and just use:
const GURL kOriginalUrl("https://original/");
const GURL kRedirectUrl("http://redirect/foo");Similarly, in the above two tests, the test server is completely irrelevant, since we never even hit it. Should do the same to them as well.
That makes this test identical to the above ones, so can then just make this the one with the non-default port, by explicitly setting it in the mock URL.
To view, visit change 1067846. To unsubscribe, or for help writing mail filters, visit settings.
Carlos IL (Gerrit)
4 comments:
File net/url_request/url_request_job.cc:
Patch Set #9, Line 395:
ersComplete();Extensions will see the response headers here... […]
That's similar to what Mike suggested (for the spec) in https://github.com/whatwg/fetch/issues/324, although that was for doing all upgrades that way, not just redirects. Happy to implement this that way if that sounds better for extensions, (plus, it will also make reporting easier since WillRedirectRequest will be called in throttles), I think the only side effect would be this counting towards the 20 redirect limit.
The reason why this is added is for urls that are explicitely set to port 80 (i.e. […]
Done (see other reply)
File net/url_request/url_request_job.cc:
Per earlier comment, this isn't needed. GURL eats explicitly specified default ports. […]
Yeah, you're right, the 80 gets normalized away before this even runs. Removed it, and added a test for a URL with an explicit port 80.
File net/url_request/url_request_unittest.cc:
Oh, we're forcing a redirect via the delegate. These don't even need to be a real GURLs. […]
Done
To view, visit change 1067846. To unsubscribe, or for help writing mail filters, visit settings.
Matt Menke (Gerrit)
This approach LGTM. If you want to change approaches, I'll want to re-review, of course.
Patch set 14:Code-Review +1
2 comments:
Patch Set #9, Line 395:
ersComplete();
That's similar to what Mike suggested (for the spec) in https://github. […]
So I think they may be two other potential side effects:
- If we're clearing referrers on HTTPS->HTTP transitions (or on cross-origin requests, even if the domain is the same) the referrer will be cleared. Whether this is the right behavior or not depends on the spec.
- The renderer sees the HTTP url. If there's logic there blocking HTTP requests, it would block the request at the intermediary URL.
I'm fine with either approach, just thought I'd raise the issue.
File net/url_request/url_request_unittest.cc:
Patch Set #14, Line 12662:
original_urlnit: In net, at least, we generally use kContNamingScheme with this pattern (x8)
To view, visit change 1067846. To unsubscribe, or for help writing mail filters, visit settings.
Carlos IL (Gerrit)
Patch Set 14: Code-Review+1
(2 comments)
This approach LGTM. If you want to change approaches, I'll want to re-review, of course.
Thanks for the review! Since most of the plumbing would be needed for the other approach anyways, I'll keep the current approach for now, and if we do decide to migrate to handling this as a redirect, I'll send that change for review as a separate CL at that point.
2 comments:
Patch Set #9, Line 395:
ersComplete();
So I think they may be two other potential side effects: […]
That's right, I think 1) was one of the concerns for Mike's proposal, and 2) might bite us for some cases where this gets blocked before it has the change to be upgraded. I'll keep the current approach for now, and if we end up deciding to switch to handling as redirects, I'll send a separate CL for that. Thanks!
File net/url_request/url_request_unittest.cc:
Patch Set #14, Line 12662:
kOriginalUrl
nit: In net, at least, we generally use kContNamingScheme with this pattern (x8)
Done
To view, visit change 1067846. To unsubscribe, or for help writing mail filters, visit settings.
Matt Menke (Gerrit)
Oh, and my LGTM covers src/net and services/network. Though I'm a content/browser/loader owner, I'm not familiar with the navigation code there, and did not review it - Camille, however, is an expert there, so I'll defer to her.
Carlos IL (Gerrit)
Friendly ping to clamy, and friendly preloaded ping to mkwst for when they're back. :)
Mike West (Gerrit)
Patch Set 16:
friendly preloaded ping to mkwst for when they're back. :)
//third_party/blink LGTM. Pretty straightforward changes on that side of things. Thanks!
Patch set 16:Code-Review +1
2 comments:
File third_party/WebKit/LayoutTests/TestExpectations:
Patch Set #16, Line 4812:
crbug.com/615885 external/wpt/content-security-policy/securitypolicyviolation/img-src-redirect-upgrade-reporting.https.html [ Timeout ]Ah. I see. Nevermind my comment above. :)
Yay!
I would have expected this to fix https://wpt.fyi/results/upgrade-insecure-requests/iframe-redirect-upgrade.https.html as well, though. Does it not?
To view, visit change 1067846. To unsubscribe, or for help writing mail filters, visit settings.
Camille Lamy (Gerrit)
Thanks! It's a bit unclear to me why you're touching the navigation code if this CL is only implementing the flag for non-navigation redirect. I think it may be better to split the navigation changes out of the CL.
1 comment:
File content/browser/loader/navigation_url_loader_impl_unittest.cc:
Patch Set #16, Line 377:
true /* expect_request_fail */));Why do we fail in this test?
To view, visit change 1067846. To unsubscribe, or for help writing mail filters, visit settings.
Carlos IL (Gerrit)
Patch Set 16:
(1 comment)
Thanks! It's a bit unclear to me why you're touching the navigation code if this CL is only implementing the flag for non-navigation redirect. I think it may be better to split the navigation changes out of the CL.
I should have been more clear, we are not (in this CL) handling navigation upgrades that happen after page load, but we are handling iframe upgrades. That being said, moving the iframe upgrades to their own CL SGTM, I'll send it to you for review once this one lands. Thanks!
3 comments:
File content/browser/loader/navigation_url_loader_impl_unittest.cc:
Patch Set #16, Line 377:
// Redirected request should also have modified headers.
Why do we fail in this test?
Since there is no server listening on https, the request will fail, but for the purposes of this test, we only need to validate that the request URL scheme was upgraded to https.
File third_party/WebKit/LayoutTests/TestExpectations:
Patch Set #16, Line 4812:
# Sheriff 2018-05-30Ah. I see. Nevermind my comment above. […]
Since the iframe changes are now separate (as per clamy@'s request), I readded the failure exception, but will remove it on the CL with the iframe changes (which I'll send for review once this one lands), I'll cc you there. Thanks!
Yay! […]
Ack (replied on the other comment).
To view, visit change 1067846. To unsubscribe, or for help writing mail filters, visit settings.
Carlos IL (Gerrit)
Carlos IL removed Camille Lamy from this change.
To view, visit change 1067846. To unsubscribe, or for help writing mail filters, visit settings.
Carlos IL (Gerrit)
Patch set 19:Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Fixed merge error" https://chromium-review.googlesource.com/c/1067846/19
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/1067846/19
Bot data: {"action": "start", "triggered_at": "2018-06-04T17:50:38.0Z", "cq_cfg_revision": "40cc775e09b6eaaae7048743640f1ae986b3edb7", "revision": "71e0273957955579bab54966b7726fee52f359c6"}
Carlos IL (Gerrit)
Carlos IL removed a vote from this change.
To view, visit change 1067846. To unsubscribe, or for help writing mail filters, visit settings.
Carlos IL (Gerrit)
Patch set 20:Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Moved test expectation to prevent further conflicts" https://chromium-review.googlesource.com/c/1067846/20
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/1067846/20
Bot data: {"action": "start", "triggered_at": "2018-06-04T18:18:27.0Z", "cq_cfg_revision": "40cc775e09b6eaaae7048743640f1ae986b3edb7", "revision": "0aa88e8ca762ccff9285fdad685ec155969d4177"}
Carlos IL (Gerrit)
Carlos IL removed a vote from this change.
To view, visit change 1067846. To unsubscribe, or for help writing mail filters, visit settings.
Carlos IL (Gerrit)
Patch set 21:Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Readded test expectations" https://chromium-review.googlesource.com/c/1067846/21
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/1067846/21
Bot data: {"action": "start", "triggered_at": "2018-06-04T19:51:18.0Z", "cq_cfg_revision": "40cc775e09b6eaaae7048743640f1ae986b3edb7", "revision": "70986e7520743ed5f2bbe2919e50f74b7b07ff3e"}
Carlos IL (Gerrit)
Carlos IL removed a vote from this change.
To view, visit change 1067846. To unsubscribe, or for help writing mail filters, visit settings.
Carlos IL (Gerrit)
Patch set 22:Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Removed change to TestExpectations" https://chromium-review.googlesource.com/c/1067846/22
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/1067846/22
Bot data: {"action": "start", "triggered_at": "2018-06-04T20:09:07.0Z", "cq_cfg_revision": "40cc775e09b6eaaae7048743640f1ae986b3edb7", "revision": "cdd5c61ca637da16dfb28c1ae009d8569156bc9c"}
Commit Bot (Gerrit)
Commit Bot merged this change.
upgrade-insecure-requests is now obeyed for non-navigation redirects.
This cl implements go/upgrade-insecure-redirects, a flag is set for
resource requests that should be upgraded to HTTPS, the flag is then
passed to //net and //net performs the upgrade if at any point the
request is redirected to HTTP.
This only covers enforcement of the CSP, it does not cover reporting,
which will be done in a separate CL.
Bug: 615885
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: Ia17955e9fb2388cbb514433ccf140f5749d5b89e
Reviewed-on: https://chromium-review.googlesource.com/1067846
Commit-Queue: Carlos IL <carl...@chromium.org>
Reviewed-by: Mike West <mk...@chromium.org>
Reviewed-by: Matt Menke <mme...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#564243}
---
M net/url_request/url_request.cc
M net/url_request/url_request.h
M net/url_request/url_request_job.cc
M net/url_request/url_request_test_util.cc
M net/url_request/url_request_test_util.h
M net/url_request/url_request_unittest.cc
M services/network/public/cpp/network_ipc_param_traits.h
M services/network/public/cpp/resource_request.h
M services/network/url_loader.cc
M third_party/blink/public/platform/web_url_request.h
M third_party/blink/renderer/core/loader/frame_loader.cc
M third_party/blink/renderer/platform/exported/web_url_request.cc
M third_party/blink/renderer/platform/loader/fetch/resource_request.cc
M third_party/blink/renderer/platform/loader/fetch/resource_request.h
M third_party/blink/renderer/platform/loader/fetch/resource_request_test.cc
15 files changed, 178 insertions(+), 3 deletions(-)
Camille Lamy (Gerrit)
Patch Set 19:
(3 comments)
Patch Set 16:
(1 comment)
Thanks! It's a bit unclear to me why you're touching the navigation code if this CL is only implementing the flag for non-navigation redirect. I think it may be better to split the navigation changes out of the CL.
I should have been more clear, we are not (in this CL) handling navigation upgrades that happen after page load, but we are handling iframe upgrades. That being said, moving the iframe upgrades to their own CL SGTM, I'll send it to you for review once this one lands. Thanks!
I see. From the navigation point-of-view, loading an iframe is a navigation. So IIUC, you don't support main-frame navigation upgrades.