I'll take a look tomorrow.

Dropping the view restriction, as we're failing closed here by blocking the resource. This is certainly a bug, but not one with severe security implications. It's just annoying and incorrect. :/

Adding some labels so that this bug doesn't show up in the triage queue.

This turns out to be difficult to address: we're doing redirects up in the network stack, and ignoring blink's attempt to change the redirect destination. We're really going to have to move 'upgrade-insecure-requests' up to the browser in order to address it, which is a bit surprising, and more work that I expected.

Ah well.

Re #4: alternatively, we could give Blink a way to modify redirect details before the redirect is followed. (It's on my mind because that would come in handy for the Referrer-Policy header.) AFAICT we already defer following the redirect until Blink has had a chance to say whether it should be cancelled or not, so it doesn't immediately seem crazy to me that Blink should be able to modify the redirect before it's followed.

estark@: Hrm. I hadn't actually considered that... it might be a lot simpler.

mmenke@: How would you feel about making `URLRequest::Delegate::OnReceivedRedirect`'s |redirect_info| parameter (and all the bits and pieces piped down to Blink) non-const?

It would be a bit more complicated than passing it non-const, since it crosses the IPC boundary and is an async IPC, etc. I think there would have to be some IPC that Blink can send to modify the redirect, similar to how (I think) Blink cancels a redirect.

Well. Yes. That's the "bits and pieces". :)

Oh, sorry, I apparently only read every 7th word or so of comment 6.

Is there any reason these should look different to everything from HSTS redirects?  Currently, HSTS redirects are distinct redirects, so for hsts, everything (Extensions, SafeBrowsing, etc) would see 3 redirects:

request for http://domain.com/asset.png -> redirected to + request for https://domain.com/asset.png -> redirected to + request for http://domain.com/redirect.png -> redirected to + request for https://domain.com/redirect.png

IF you modified RedirectInfo instead, a great number of objects would see:

request for http://domain.com/asset.png -> redirected to + request for https://domain.com/asset.png -> redirected to http://domain.com/redirect.png -> request for https://domain.com/redirect.png

(Note the missing request and redirect for the last two).  I'd rather not add yet another redirect path that may or may not have side effects to random things watching requests, if we can avoid it.

On a side note, I think this case is somewhat bonkers, and am rather surprised the spec allows it.

That is, I'm assuming the spec allows it, and it's not just a FireFox quirk.

This also makes my head hurt in the ServiceWorker case...I don't suppose we could just re-issue the request, rather than trying to add redirect magic?

Ok. Doing things in line with HSTS is more or less what I was planning on aiming for.

> That is, I'm assuming the spec allows it, and it's not just a FireFox quirk.

The spec does indeed specify this behavior. See step 4 of https://fetch.spec.whatwg.org/#main-fetch, which is recursively called for redirects (HSTS is step 9, incidentally).

What's bonkers about it? :)

> This also makes my head hurt in the ServiceWorker case

As you note in comment 10, this is more or less the same as HSTS. Why would it make your head hurt?

> I don't suppose we could just re-issue the request, rather than trying to add redirect magic?

Re-issue the request from where? The network stack? Or Blink? What would this look like?

What's bonkers about it is that this is nominally an "http request" that has magic redirect behavior based on the context of the page that issued it (i.e., this isn't HTTP, it's a magic HTML thing that tells us to inject magic, context-specific, non-HTTP rules into the HTTP network stack).  Admittedly, ServiceWorker, "privacy mode", and the like have similar weirdness to varying degrees.  Also, we're being redirected to HTTP from an HTTPS page, from a server that really wants us to issue an HTTPS request which seems like a broken server.  Should we clear the referrer on this pseudo-redirect or not?

HSTS is basically an HTTP thing.  The network stack sees the request, it redirects it, regardless of context.  No problem, no mess.  In the Service Worker case, the URLRequest tells the Service Worker of the the redirect, it tells the wrapped URLRequest of the redirect (Does it even currently do this - the Service Worker has power over redirects, itself, doesn't it?), it tells the renderer process of the redirect.  It then decides it wants to go somewhere else, so it tells the wrapper URLRequest to go somewhere else.  It tells the ServiceWorker to go somewhere else.  It tells the real URLRequest to go somewhere else.  If we don't specifically hook up that case, I'm betting it will be broken, even if we fix the main case.

Issue a new request from blink, so it looks like a new network request.  It would look completely different from a redirect, but then, meta redirects look completely different, too.

Thanks! I think we might have different opinions about the role of //content in asking //net to do work. Certainly it's the case that our implementation doesn't at all match what https://fetch.spec.whatwg.org/ spells out, which might lead to this divergence. :/

> What's bonkers about it is that this is nominally an "http request" that has magic redirect behavior

Why is rewriting the redirect more magical than cancelling it? The latter seems like little more than a special case of the former.

> based on the context of the page that issued it

You call out "privacy mode", etc. but I think there are a few other pieces of URLRequest that are even more similar to what's proposed here: first-party cookie URL and its associated policy, referrers and their associated policy, SameSite cookie behaviors, etc. Really, anything we pass up via `content::RequestInfo` has an impact in one way or another on the request. Is all of that bonkers? If yes, what should we do about it? :)

> HSTS is basically an HTTP thing. The network stack sees the request, it redirects it, regardless of context.

"regardless of context" is a strange statement here, as URLRequest hops over to TransportSecurityState for some external data regarding the request. I'd suggest that things like referrer policy and cookie policy are just as contextual, just as bound to the HTTP layer, and fairly cleanly represented on URLRequest in a way that doesn't introduce fundamental new complexity.

I have ~half of a CL put together that teaches URLRequest about an insecure request policy, similar to these other properties. I'll send it over in a bit; it sounds like you'll love it. ;)

> Issue a new request from blink, so it looks like a new network request. It would look completely different from a redirect, but then, meta redirects look completely different, too.

Even if this were trivial to do, it doesn't seem like the right way to represent what's going on. The page has made an assertion that it doesn't want to load mixed content, and that an upgrade strategy is preferable to outright failure. Triggering a new request after a redirect seems like the wrong way of representing that upgrade, especially in light of the way we treat HSTS.

Now, with our current infrastructure it wouldn't be trivial to do, as Blink's loader doesn't have a good way of swapping out one request for another, or binding a new request to an element that issued a blocked request. I need to build something to support preflight requests for things like images and scripts, but even that simpler version of this problem is going to be a ton of work.

Maybe there's a place in //content where this sort of swap would be simpler? I don't really have the impression that moving things to ResourceDispatcher(Host?) or similar would make much difference... Do you?

>> What's bonkers about it is that this is nominally an "http request" that has magic redirect behavior

> Why is rewriting the redirect more magical than cancelling it? The latter seems like little more than a special case of the former.

Cancelling a request is a fairly basic behavior, and while HTTP spec may say nothing about it (Disclaimer:  I haven't verified that), HTTP2 and QUIC both allow cancelling requests, so the ability to be cancelled is effectively an HTTP behavior that it makes sense to expose to HTTP consumers, which are then free to do so, or not, for their own nefarious ends.

> "regardless of context" is a strange statement here, as URLRequest hops over to TransportSecurityState for some external data regarding the request. I'd suggest that things like referrer policy and cookie policy are just as contextual, just as bound to the HTTP layer, and fairly cleanly represented on URLRequest in a way that doesn't introduce fundamental new complexity.

We don't get any additional information from the renderer about the URLRequest, nor do we depend on weird metadata based on the context the page was loaded in there, as far as I know - we rely on data we previously received as a set of HTTP headers, which is applied to all HTTP requests impartially.

And yes, all the bolted on cookie policy and referrer policy magic doesn't really seem to me like the most natural fit in net/, but then, it doesn't seem like a great fit anywhere.  And on the plus side, it's simple enough that it can be fit wholly within net/.  If this desire to redirect requests came from anywhere except the renderer, I'd say the way to do it would be to implement a URLRequestJobIntercepter, and have it explicitly create URLRequestRedirectJobs for requests it wanted to redirect - that's the API the network stack supports that allows external things to redirect requests.

I put up a PoC at https://codereview.chromium.org/2053693002 for us to discuss, which moves most of UIR up to the browser process. It interacts with CSP incorrectly, so it's not landable as-is, but it's at least something concrete to chat about. :)

Not a low severity vulnerability, just a good to have security feature, removing tags.

Changing bug type since this isn't a security risk to users. (Moving it out of the security queue.)

 Issue 659684  has been merged into this issue.

 Issue 768529  has been merged into this issue.

 Issue 803734  has been merged into this issue.

 Issue 816899  has been merged into this issue.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4636db5e54c5dda5436cb6310c129a85093351b9

commit 4636db5e54c5dda5436cb6310c129a85093351b9
Author: Carlos IL <carlosil@chromium.org>
Date: Tue May 22 15:22:48 2018

Moved URL-change logic out of ShouldModifyRequestUrlForCsp

Separated ShouldModifyRequestUrlForCsp into
ShouldModifyRequestUrlForCsp and ModifyRequestUrlForCsp, this is
required for go/upgrade-insecure-redirects since we will need to set a
flag for requests that need upgrades (even if they are not upgraded due
to already being HTTPS).

Bug:  615885 
Change-Id: I06e6463f8b1a1a87d995f60f4692c09f4c4f88d9
Reviewed-on: https://chromium-review.googlesource.com/1067787
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Commit-Queue: Carlos IL <carlosil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#560595}
[modify] https://crrev.com/4636db5e54c5dda5436cb6310c129a85093351b9/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/4636db5e54c5dda5436cb6310c129a85093351b9/content/common/content_security_policy/csp_context.cc
[modify] https://crrev.com/4636db5e54c5dda5436cb6310c129a85093351b9/content/common/content_security_policy/csp_context.h
[modify] https://crrev.com/4636db5e54c5dda5436cb6310c129a85093351b9/content/common/content_security_policy/csp_context_unittest.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/aef65d67763ffd4aa4e8497585f335ad0a6c934e

commit aef65d67763ffd4aa4e8497585f335ad0a6c934e
Author: Carlos IL <carlosil@chromium.org>
Date: Mon Jun 04 21:24:13 2018

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 <carlosil@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#564243}
[modify] https://crrev.com/aef65d67763ffd4aa4e8497585f335ad0a6c934e/net/url_request/url_request.cc
[modify] https://crrev.com/aef65d67763ffd4aa4e8497585f335ad0a6c934e/net/url_request/url_request.h
[modify] https://crrev.com/aef65d67763ffd4aa4e8497585f335ad0a6c934e/net/url_request/url_request_job.cc
[modify] https://crrev.com/aef65d67763ffd4aa4e8497585f335ad0a6c934e/net/url_request/url_request_test_util.cc
[modify] https://crrev.com/aef65d67763ffd4aa4e8497585f335ad0a6c934e/net/url_request/url_request_test_util.h
[modify] https://crrev.com/aef65d67763ffd4aa4e8497585f335ad0a6c934e/net/url_request/url_request_unittest.cc
[modify] https://crrev.com/aef65d67763ffd4aa4e8497585f335ad0a6c934e/services/network/public/cpp/network_ipc_param_traits.h
[modify] https://crrev.com/aef65d67763ffd4aa4e8497585f335ad0a6c934e/services/network/public/cpp/resource_request.h
[modify] https://crrev.com/aef65d67763ffd4aa4e8497585f335ad0a6c934e/services/network/url_loader.cc
[modify] https://crrev.com/aef65d67763ffd4aa4e8497585f335ad0a6c934e/third_party/blink/public/platform/web_url_request.h
[modify] https://crrev.com/aef65d67763ffd4aa4e8497585f335ad0a6c934e/third_party/blink/renderer/core/loader/frame_loader.cc
[modify] https://crrev.com/aef65d67763ffd4aa4e8497585f335ad0a6c934e/third_party/blink/renderer/platform/exported/web_url_request.cc
[modify] https://crrev.com/aef65d67763ffd4aa4e8497585f335ad0a6c934e/third_party/blink/renderer/platform/loader/fetch/resource_request.cc
[modify] https://crrev.com/aef65d67763ffd4aa4e8497585f335ad0a6c934e/third_party/blink/renderer/platform/loader/fetch/resource_request.h
[modify] https://crrev.com/aef65d67763ffd4aa4e8497585f335ad0a6c934e/third_party/blink/renderer/platform/loader/fetch/resource_request_test.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a54c59a223c39cdcc6fc10c224f8f29b85b63959

commit a54c59a223c39cdcc6fc10c224f8f29b85b63959
Author: Carlos IL <carlosil@chromium.org>
Date: Mon Jun 11 19:43:03 2018

upgrade-insecure-requests is now obeyed for iframe upgrades with redirects.

This cl implements go/upgrade-insecure-redirects for iframe upgrades.

Bug:  615885 
Change-Id: I8501ef09740c18293580658df3084bf01e25d8e6
Reviewed-on: https://chromium-review.googlesource.com/1086005
Commit-Queue: Carlos IL <carlosil@chromium.org>
Reviewed-by: Camille Lamy <clamy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566109}
[modify] https://crrev.com/a54c59a223c39cdcc6fc10c224f8f29b85b63959/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/a54c59a223c39cdcc6fc10c224f8f29b85b63959/content/browser/frame_host/navigation_request.h
[modify] https://crrev.com/a54c59a223c39cdcc6fc10c224f8f29b85b63959/content/browser/frame_host/navigation_request_info.cc
[modify] https://crrev.com/a54c59a223c39cdcc6fc10c224f8f29b85b63959/content/browser/frame_host/navigation_request_info.h
[modify] https://crrev.com/a54c59a223c39cdcc6fc10c224f8f29b85b63959/content/browser/loader/navigation_url_loader_impl.cc
[modify] https://crrev.com/a54c59a223c39cdcc6fc10c224f8f29b85b63959/content/browser/loader/navigation_url_loader_impl_unittest.cc
[modify] https://crrev.com/a54c59a223c39cdcc6fc10c224f8f29b85b63959/content/browser/loader/navigation_url_loader_unittest.cc
[modify] https://crrev.com/a54c59a223c39cdcc6fc10c224f8f29b85b63959/content/browser/loader/resource_dispatcher_host_impl.cc
[modify] https://crrev.com/a54c59a223c39cdcc6fc10c224f8f29b85b63959/content/browser/loader/resource_dispatcher_host_unittest.cc
[add] https://crrev.com/a54c59a223c39cdcc6fc10c224f8f29b85b63959/content/test/data/redirect301-to-http
[add] https://crrev.com/a54c59a223c39cdcc6fc10c224f8f29b85b63959/content/test/data/redirect301-to-http.mock-http-headers
[modify] https://crrev.com/a54c59a223c39cdcc6fc10c224f8f29b85b63959/third_party/WebKit/LayoutTests/TestExpectations

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0d67fc7cdf40f27421f59f4c61c289c8b201b0d3

commit 0d67fc7cdf40f27421f59f4c61c289c8b201b0d3
Author: Carlos IL <carlosil@chromium.org>
Date: Fri Jun 22 17:18:44 2018

Fixed reporting for iframe ugprade-insecure-requests upgrades

This enables reporting for subframe navigations that involve a redirect
and are upgraded due to CSP policy.

Bug:  615885 
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: Ibcd0160ba0825d5d5d2f2f2a58ecfe9fb51c4688
Reviewed-on: https://chromium-review.googlesource.com/1096360
Reviewed-by: Naoki Fukino <fukino@chromium.org>
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Camille Lamy <clamy@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Reviewed-by: Luke Halliwell <halliwell@chromium.org>
Reviewed-by: Ben Wells <benwells@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Commit-Queue: Carlos IL <carlosil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569675}
[modify] https://crrev.com/0d67fc7cdf40f27421f59f4c61c289c8b201b0d3/chrome/browser/chromeos/fileapi/external_file_url_request_job.cc
[modify] https://crrev.com/0d67fc7cdf40f27421f59f4c61c289c8b201b0d3/chrome/browser/chromeos/fileapi/external_file_url_request_job.h
[modify] https://crrev.com/0d67fc7cdf40f27421f59f4c61c289c8b201b0d3/chromecast/browser/extension_request_protocol_handler.cc
[modify] https://crrev.com/0d67fc7cdf40f27421f59f4c61c289c8b201b0d3/content/browser/android/url_request_content_job.cc
[modify] https://crrev.com/0d67fc7cdf40f27421f59f4c61c289c8b201b0d3/content/browser/android/url_request_content_job.h
[modify] https://crrev.com/0d67fc7cdf40f27421f59f4c61c289c8b201b0d3/content/browser/devtools/devtools_url_loader_interceptor.cc
[modify] https://crrev.com/0d67fc7cdf40f27421f59f4c61c289c8b201b0d3/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/0d67fc7cdf40f27421f59f4c61c289c8b201b0d3/content/browser/frame_host/navigation_request.h
[modify] https://crrev.com/0d67fc7cdf40f27421f59f4c61c289c8b201b0d3/content/browser/loader/navigation_url_loader_impl_unittest.cc
[modify] https://crrev.com/0d67fc7cdf40f27421f59f4c61c289c8b201b0d3/content/common/service_worker/service_worker_loader_helpers.cc
[modify] https://crrev.com/0d67fc7cdf40f27421f59f4c61c289c8b201b0d3/extensions/browser/extension_protocols.cc
[modify] https://crrev.com/0d67fc7cdf40f27421f59f4c61c289c8b201b0d3/net/test/url_request/url_request_mock_http_job.cc
[modify] https://crrev.com/0d67fc7cdf40f27421f59f4c61c289c8b201b0d3/net/test/url_request/url_request_mock_http_job.h
[modify] https://crrev.com/0d67fc7cdf40f27421f59f4c61c289c8b201b0d3/net/url_request/redirect_info.cc
[modify] https://crrev.com/0d67fc7cdf40f27421f59f4c61c289c8b201b0d3/net/url_request/redirect_info.h
[modify] https://crrev.com/0d67fc7cdf40f27421f59f4c61c289c8b201b0d3/net/url_request/redirect_info_unittest.cc
[modify] https://crrev.com/0d67fc7cdf40f27421f59f4c61c289c8b201b0d3/net/url_request/url_request_file_job.cc
[modify] https://crrev.com/0d67fc7cdf40f27421f59f4c61c289c8b201b0d3/net/url_request/url_request_file_job.h
[modify] https://crrev.com/0d67fc7cdf40f27421f59f4c61c289c8b201b0d3/net/url_request/url_request_job.cc
[modify] https://crrev.com/0d67fc7cdf40f27421f59f4c61c289c8b201b0d3/net/url_request/url_request_job.h
[modify] https://crrev.com/0d67fc7cdf40f27421f59f4c61c289c8b201b0d3/net/url_request/url_request_test_job.cc
[modify] https://crrev.com/0d67fc7cdf40f27421f59f4c61c289c8b201b0d3/net/url_request/url_request_test_job.h
[modify] https://crrev.com/0d67fc7cdf40f27421f59f4c61c289c8b201b0d3/net/url_request/url_request_unittest.cc
[modify] https://crrev.com/0d67fc7cdf40f27421f59f4c61c289c8b201b0d3/services/network/public/cpp/net_ipc_param_traits.h
[modify] https://crrev.com/0d67fc7cdf40f27421f59f4c61c289c8b201b0d3/storage/browser/blob/view_blob_internals_job.cc
[modify] https://crrev.com/0d67fc7cdf40f27421f59f4c61c289c8b201b0d3/storage/browser/blob/view_blob_internals_job.h
[modify] https://crrev.com/0d67fc7cdf40f27421f59f4c61c289c8b201b0d3/storage/browser/fileapi/file_system_url_request_job.cc
[modify] https://crrev.com/0d67fc7cdf40f27421f59f4c61c289c8b201b0d3/storage/browser/fileapi/file_system_url_request_job.h