|
|
Created:
4 years, 6 months ago by Mike West Modified:
3 years, 6 months ago CC:
arthursonzogni, blink-reviews, blink-reviews-api_chromium.org, cbentzel+watch_chromium.org, chromium-reviews, clamy, creis+watch_chromium.org, darin-cc_chromium.org, dglazkov+blink, estark, gavinp+loader_chromium.org, jam, Nate Chapin, loading-reviews_chromium.org, nasko+codewatch_chromium.org, Randy Smith (Not in Mondays), tyoshino+watch_chromium.org, wjmaclean, Yoav Weiss Base URL:
https://chromium.googlesource.com/chromium/src.git@replicate Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWIP: Move 'Upgrade-Insecure-Requests' to the browser process.
This is more or less a sanity check to see what mmenke@ thinks of this
approach. :) It needs more tests in //content, and some general cleanup
of the ductwork between Blink and //net before actual review.
BUG=615885
Patch Set 1 #
Total comments: 6
Patch Set 2 : Folding into URLRequestHTTPJob. #Patch Set 3 : Rebase. #Patch Set 4 : Punt on blink-side checks. #Patch Set 5 : Not sure I like this. #
Total comments: 2
Patch Set 6 : Just redirects. #
Total comments: 2
Patch Set 7 : DCHECK. #
Total comments: 15
Patch Set 8 : Rebase. :( #Messages
Total messages: 105 (57 generated)
Description was changed from ========== WIP: Move 'Upgrade-Insecure-Requests' to the browser process. This is more or less a sanity check to see what mmenke@ thinks of this approach. :) It needs more tests in //content, and some general cleanup of the ductwork between Blink and //net before actual review. BUG=615885 ========== to ========== WIP: Move 'Upgrade-Insecure-Requests' to the browser process. This is more or less a sanity check to see what mmenke@ thinks of this approach. :) It needs more tests in //content, and some general cleanup of the ductwork between Blink and //net before actual review. BUG=615885 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
mkwst@chromium.org changed reviewers: + mmenke@chromium.org
mmenke@: Is this anything like what you had in mind? I've put together a tiny 'URLRequestInterceptor' to handle redirect creation, but I'm not sure how much it buys us over just changing 'URLRequest::GetHSTSRedirect' directly, since it still requires request-specific information to live on 'URLRequest' itself so that the interceptor can do its intercepting. It's entirely possible that I've missed some simpler way to get this data from A to B, but the context objects available to the interceptor seem fairly generic, which makes them a bad place to stick this kind of data, but it feels pretty overcomplicated... clamy@: In particular, it seems like I should be able to skip piping data through WebURLRequest/ResourceRequest entirely, since we have information about the frame tree's upgrade state in the browser process already. Is there any existing way to get to the requesting frame tree node from ResourceDispatcherHostImpl? estark@: FYI.
https://codereview.chromium.org/2053693002/diff/1/content/browser/loader/reso... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2053693002/diff/1/content/browser/loader/reso... content/browser/loader/resource_dispatcher_host_impl.cc:1476: net::URLRequest::UPGRADE_ALL_INSECURE_REQUESTS); Does this have to be a member of the URLRequest?. It seems like there are at least two other options here: 1) Make it a member of the lovely struct ResourceRequestInfoImpl (Which can be grabbed from a URLRequest). 2) URLRequest implements SupportsUserData, so could set it that way. We could add it to URLRequest::GetHSTSRedirect, as you suggest. My concerns there are the scope of URLRequest's API (Which is constantly growing, and eventually may grow to encompass the entire Chrome code base), in addition to whether or not it belongs there. On the other hand, set up here is more than a little obscure, since we have to go through StoragePartition, which isn't great. I think either of those options are better than the other browser-based options (Modifying RedirectInfo, for the API/correctness reasons already mentioning, or do what extensions do - which is much along the same lines as modifying RedirectInfo, I believe - though they do it from the NetworkDelegate interface instead).
Let me think about this a bit - unfortunately, I haven't had the time to do so (Not too busy this week, but been sick since the end of last week).
On 2016/06/09 18:59:04, mmenke wrote: > Let me think about this a bit - unfortunately, I haven't had the time to do so > (Not too busy this week, but been sick since the end of last week). Also, I think we should have browser tests for this case, both in the case of SeviceWorker and in the general case.
On 2016/06/09 at 18:59:04, mmenke wrote: > Let me think about this a bit - unfortunately, I haven't had the time to do so (Not too busy this week, but been sick since the end of last week). Thanks for taking a look! I'm holding off on writing too many more tests until we've decided what we want the implementation to look like. I'm not a huge rush here, so please take care of any high priority things you're behind on, but it would be nice to get this into M53.
https://codereview.chromium.org/2053693002/diff/1/content/browser/loader/reso... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2053693002/diff/1/content/browser/loader/reso... content/browser/loader/resource_dispatcher_host_impl.cc:1476: net::URLRequest::UPGRADE_ALL_INSECURE_REQUESTS); On 2016/06/09 at 18:56:55, mmenke wrote: > 1) Make it a member of the lovely struct ResourceRequestInfoImpl (Which can be grabbed from a URLRequest). This seems like the best approach if you want to keep it out of URLRequest. > 2) URLRequest implements SupportsUserData, so could set it that way. If we have to resort to SupportsUserData, I feel like we're just admitting that we'd rather be lazy than design a reasonable solution. :) I mean, I _am_ lazy, but I'd like not to admit it so publicly! > We could add it to URLRequest::GetHSTSRedirect, as you suggest. My concerns there are the scope of URLRequest's API (Which is constantly growing, and eventually may grow to encompass the entire Chrome code base), in addition to whether or not it belongs there. I think there's a case to be made for stuffing it into URLRequest, however, as it's really triggering the same behavior as `URLRequest::GetHSTSRedirect`. It would be nice to merge those together, and pulling HSTS out of URLRequest seems like the wrong way to do it. I agree that it's increasing the scope of URLRequest, but since it's not actually adding a new feature, just giving the caller another way of triggering existing behavior, it seems like a pretty minor expansion. > On the other hand, set up here is more than a little obscure, since we have to go through StoragePartition, which isn't great. Right. If you hadn't explicitly suggested looking at `URLRequestInterceptor`, I don't think I'd have found this mechanism. That's not fatal, of course, but it is something to consider. > I think either of those options are better than the other browser-based options (Modifying RedirectInfo, for the API/correctness reasons already mentioning, or do what extensions do - which is much along the same lines as modifying RedirectInfo, I believe - though they do it from the NetworkDelegate interface instead). I defer to you on this; you have a much better picture of what you'd like the API to be than I do.
On 2016/06/10 09:55:09, Mike West wrote: > On 2016/06/09 at 18:59:04, mmenke wrote: > > Let me think about this a bit - unfortunately, I haven't had the time to do so > (Not too busy this week, but been sick since the end of last week). > > Thanks for taking a look! I'm holding off on writing too many more tests until > we've decided what we want the implementation to look like. I'm not a huge rush > here, so please take care of any high priority things you're behind on, but it > would be nice to get this into M53. I don't think that browser tests (or layout tests) actually depend on the implementation. That's one thing that's great about them - they don't make assumptions about how something is implemented, just that it is. Anyhow, fine with you waiting until we reach a decision here (Hope we can do that later today), but just thought I'd mention it.
On 2016/06/10 15:14:50, mmenke wrote: > On 2016/06/10 09:55:09, Mike West wrote: > > On 2016/06/09 at 18:59:04, mmenke wrote: > > > Let me think about this a bit - unfortunately, I haven't had the time to do > so > > (Not too busy this week, but been sick since the end of last week). > > > > Thanks for taking a look! I'm holding off on writing too many more tests until > > we've decided what we want the implementation to look like. I'm not a huge > rush > > here, so please take care of any high priority things you're behind on, but it > > would be nice to get this into M53. > > I don't think that browser tests (or layout tests) actually depend on the > implementation. That's one thing that's great about them - they don't make > assumptions about how something is implemented, just that it is. Anyhow, fine > with you waiting until we reach a decision here (Hope we can do that later > today), but just thought I'd mention it. What about referrers? Referrer policies of same-origin or origin-when-cross-origin (Or just clearning the referrer on secure to insecure) would be applied, if this is treated like HSTS is. Also, what sort of review has the spec gone through? Admit that I'm a bit skeptical of the "Updating websites is hard, so every browser has to support this feature for eternity" argument.
On 2016/06/10 16:33:06, mmenke wrote: > On 2016/06/10 15:14:50, mmenke wrote: > > On 2016/06/10 09:55:09, Mike West wrote: > > > On 2016/06/09 at 18:59:04, mmenke wrote: > > > > Let me think about this a bit - unfortunately, I haven't had the time to > do > > so > > > (Not too busy this week, but been sick since the end of last week). > > > > > > Thanks for taking a look! I'm holding off on writing too many more tests > until > > > we've decided what we want the implementation to look like. I'm not a huge > > rush > > > here, so please take care of any high priority things you're behind on, but > it > > > would be nice to get this into M53. > > > > I don't think that browser tests (or layout tests) actually depend on the > > implementation. That's one thing that's great about them - they don't make > > assumptions about how something is implemented, just that it is. Anyhow, fine > > with you waiting until we reach a decision here (Hope we can do that later > > today), but just thought I'd mention it. > > What about referrers? Referrer policies of same-origin or > origin-when-cross-origin (Or just clearning the referrer on secure to insecure) > would be applied, if this is treated like HSTS is. > > Also, what sort of review has the spec gone through? Admit that I'm a bit > skeptical of the "Updating websites is hard, so every browser has to support > this feature for eternity" argument. Hrm....Can't Service Worker do this, actually?
On 2016/06/10 16:44:41, mmenke wrote: > On 2016/06/10 16:33:06, mmenke wrote: > > On 2016/06/10 15:14:50, mmenke wrote: > > > On 2016/06/10 09:55:09, Mike West wrote: > > > > On 2016/06/09 at 18:59:04, mmenke wrote: > > > > > Let me think about this a bit - unfortunately, I haven't had the time to > > do > > > so > > > > (Not too busy this week, but been sick since the end of last week). > > > > > > > > Thanks for taking a look! I'm holding off on writing too many more tests > > until > > > > we've decided what we want the implementation to look like. I'm not a huge > > > rush > > > > here, so please take care of any high priority things you're behind on, > but > > it > > > > would be nice to get this into M53. > > > > > > I don't think that browser tests (or layout tests) actually depend on the > > > implementation. That's one thing that's great about them - they don't make > > > assumptions about how something is implemented, just that it is. Anyhow, > fine > > > with you waiting until we reach a decision here (Hope we can do that later > > > today), but just thought I'd mention it. > > > > What about referrers? Referrer policies of same-origin or > > origin-when-cross-origin (Or just clearning the referrer on secure to > insecure) > > would be applied, if this is treated like HSTS is. > > > > Also, what sort of review has the spec gone through? Admit that I'm a bit > > skeptical of the "Updating websites is hard, so every browser has to support > > this feature for eternity" argument. > > Hrm....Can't Service Worker do this, actually? Ahh, guess it can't, for cross-site requests, and maybe not for uploads, actually.
On 2016/06/10 at 16:33:06, mmenke wrote: > On 2016/06/10 15:14:50, mmenke wrote: > > On 2016/06/10 09:55:09, Mike West wrote: > > > On 2016/06/09 at 18:59:04, mmenke wrote: > > > > Let me think about this a bit - unfortunately, I haven't had the time to do > > so > > > (Not too busy this week, but been sick since the end of last week). > > > > > > Thanks for taking a look! I'm holding off on writing too many more tests until > > > we've decided what we want the implementation to look like. I'm not a huge > > rush > > > here, so please take care of any high priority things you're behind on, but it > > > would be nice to get this into M53. > > > > I don't think that browser tests (or layout tests) actually depend on the > > implementation. That's one thing that's great about them - they don't make > > assumptions about how something is implemented, just that it is. Anyhow, fine > > with you waiting until we reach a decision here (Hope we can do that later > > today), but just thought I'd mention it. You're totally right, but I'm lazy. I'd like to know that you're not going to tell me to do everything in Blink, because I'd write layout tests instead, which are 8 million times simpler. :) > > What about referrers? Referrer policies of same-origin or origin-when-cross-origin (Or just clearning the referrer on secure to insecure) would be applied, if this is treated like HSTS is. As specced, the rewrite happens before the referrer is set, so we'd want to apply the referrer policy for the rewritten URL. I think you're right that this implementation doesn't do that (nor for HSTS). I'll have to think about whether we can reasonably move either of those around; there's a bit of a conflict with CSP, which needs to apply post-upgrade. Part of the difficulty here is that Fetch is simply written differently than our implementation. Rather than redirecting (which would entail another pass through the Main Fetch algorithm), Fetch rewrites the URL. I wonder what Firefox does here? It might be worth aligning the spec with our behavior if we can reasonably make that work. (Based on a quick test, they're at least not showing a redirect in devtools... I'll ask about their internal implementation.) > Also, what sort of review has the spec gone through? Admit that I'm a bit skeptical of the "Updating websites is hard, so every browser has to support this feature for eternity" argument. It's a candidate rec at W3C, we shipped it ~3/4 of a year ago ago, Firefox shipped it shortly thereafter, and WebKit has most of an implementation done in ToT. Nothing is set in stone, but developers are certainly finding it pretty useful. Wired pointed to it as "the most important browser feature for easing the transition from HTTP to HTTPS" in https://www.wired.com/2016/05/wired-first-big-https-rollout-snag/. I understand the reticence to implement something like this, but I think the value outweighs the challenge. *shrug* But I would, because I wrote it an I'm biased. :)
On 2016/06/10 17:35:22, Mike West wrote: > On 2016/06/10 at 16:33:06, mmenke wrote: > > On 2016/06/10 15:14:50, mmenke wrote: > > > On 2016/06/10 09:55:09, Mike West wrote: > > > > On 2016/06/09 at 18:59:04, mmenke wrote: > > > > > Let me think about this a bit - unfortunately, I haven't had the time to > do > > > so > > > > (Not too busy this week, but been sick since the end of last week). > > > > > > > > Thanks for taking a look! I'm holding off on writing too many more tests > until > > > > we've decided what we want the implementation to look like. I'm not a huge > > > rush > > > > here, so please take care of any high priority things you're behind on, > but it > > > > would be nice to get this into M53. > > > > > > I don't think that browser tests (or layout tests) actually depend on the > > > implementation. That's one thing that's great about them - they don't make > > > assumptions about how something is implemented, just that it is. Anyhow, > fine > > > with you waiting until we reach a decision here (Hope we can do that later > > > today), but just thought I'd mention it. > > You're totally right, but I'm lazy. I'd like to know that you're not going to > tell me to do everything in Blink, because I'd write layout tests instead, which > are 8 million times simpler. :) > > > > > What about referrers? Referrer policies of same-origin or > origin-when-cross-origin (Or just clearning the referrer on secure to insecure) > would be applied, if this is treated like HSTS is. > > As specced, the rewrite happens before the referrer is set, so we'd want to > apply the referrer policy for the rewritten URL. I think you're right that this > implementation doesn't do that (nor for HSTS). I'll have to think about whether > we can reasonably move either of those around; there's a bit of a conflict with > CSP, which needs to apply post-upgrade. I actually think with HSTS, we *should* be treating it as a redirect. If one host is relying on clearing redirects from secure to non-secure, and then issues a cross-site request (Or navigation) to an HTTP URL, which happens to have an HSTS entry, I don't think we should be sending the referrer. This case is different, though, as it's the main site (The one with the policy) telling us to stick with HTTPS.... Of course, there's still the question of what to do if it requests URLs from third party sites, which then redirect to other HTTPS URLS. The whole thing just seems rife for breaking assumptions around referrer policies, no matter what we do. > Part of the difficulty here is that Fetch is simply written differently than our > implementation. Rather than redirecting (which would entail another pass through > the Main Fetch algorithm), Fetch rewrites the URL. I wonder what Firefox does > here? It might be worth aligning the spec with our behavior if we can reasonably > make that work. (Based on a quick test, they're at least not showing a redirect > in devtools... I'll ask about their internal implementation.) > > > Also, what sort of review has the spec gone through? Admit that I'm a bit > skeptical of the "Updating websites is hard, so every browser has to support > this feature for eternity" argument. > > It's a candidate rec at W3C, we shipped it ~3/4 of a year ago ago, Firefox > shipped it shortly thereafter, and WebKit has most of an implementation done in > ToT. > > Nothing is set in stone, but developers are certainly finding it pretty useful. > Wired pointed to it as "the most important browser feature for easing the > transition from HTTP to HTTPS" in > https://www.wired.com/2016/05/wired-first-big-https-rollout-snag/. I understand > the reticence to implement something like this, but I think the value outweighs > the challenge. *shrug* But I would, because I wrote it an I'm biased. :) If the answer were "we're a pioneer here, and no one else has currently any plans to implement it", I'd push back, but as-is, I'll defer to the wisdom of the masses.
On 2016/06/10 18:18:21, mmenke wrote: > On 2016/06/10 17:35:22, Mike West wrote: > > On 2016/06/10 at 16:33:06, mmenke wrote: > > > On 2016/06/10 15:14:50, mmenke wrote: > > > > On 2016/06/10 09:55:09, Mike West wrote: > > > > > On 2016/06/09 at 18:59:04, mmenke wrote: > > > > > > Let me think about this a bit - unfortunately, I haven't had the time > to > > do > > > > so > > > > > (Not too busy this week, but been sick since the end of last week). > > > > > > > > > > Thanks for taking a look! I'm holding off on writing too many more tests > > until > > > > > we've decided what we want the implementation to look like. I'm not a > huge > > > > rush > > > > > here, so please take care of any high priority things you're behind on, > > but it > > > > > would be nice to get this into M53. > > > > > > > > I don't think that browser tests (or layout tests) actually depend on the > > > > implementation. That's one thing that's great about them - they don't > make > > > > assumptions about how something is implemented, just that it is. Anyhow, > > fine > > > > with you waiting until we reach a decision here (Hope we can do that later > > > > today), but just thought I'd mention it. > > > > You're totally right, but I'm lazy. I'd like to know that you're not going to > > tell me to do everything in Blink, because I'd write layout tests instead, > which > > are 8 million times simpler. :) Having had to debug layout tests, I'd completely disagree. Hundreds of Javascript files in different directories calling each other, without any clear links between them, having to set up a webserver to try and debug things.... Give me a self-contained BrowserTest any day of the week over a layout test. :)
On 2016/06/10 18:21:14, mmenke wrote: > On 2016/06/10 18:18:21, mmenke wrote: > > On 2016/06/10 17:35:22, Mike West wrote: > > > On 2016/06/10 at 16:33:06, mmenke wrote: > > > > On 2016/06/10 15:14:50, mmenke wrote: > > > > > On 2016/06/10 09:55:09, Mike West wrote: > > > > > > On 2016/06/09 at 18:59:04, mmenke wrote: > > > > > > > Let me think about this a bit - unfortunately, I haven't had the > time > > to > > > do > > > > > so > > > > > > (Not too busy this week, but been sick since the end of last week). > > > > > > > > > > > > Thanks for taking a look! I'm holding off on writing too many more > tests > > > until > > > > > > we've decided what we want the implementation to look like. I'm not a > > huge > > > > > rush > > > > > > here, so please take care of any high priority things you're behind > on, > > > but it > > > > > > would be nice to get this into M53. > > > > > > > > > > I don't think that browser tests (or layout tests) actually depend on > the > > > > > implementation. That's one thing that's great about them - they don't > > make > > > > > assumptions about how something is implemented, just that it is. > Anyhow, > > > fine > > > > > with you waiting until we reach a decision here (Hope we can do that > later > > > > > today), but just thought I'd mention it. > > > > > > You're totally right, but I'm lazy. I'd like to know that you're not going > to > > > tell me to do everything in Blink, because I'd write layout tests instead, > > which > > > are 8 million times simpler. :) > > Having had to debug layout tests, I'd completely disagree. Hundreds of > Javascript files in different directories calling each other, without any clear > links between them, having to set up a webserver to try and debug things.... > Give me a self-contained BrowserTest any day of the week over a layout test. :) Also, layout tests seem to follow blink style, which seems to include a belief that adding support for comments to programming languages was a bad idea.
Sorry, forgot about this one. In my queue for tomorrow, after I do my simpler reviews.
Not a huge fan of the idea, but yea, I think the way to go is to put the logic in URLRequest. https://codereview.chromium.org/2053693002/diff/1/content/browser/loader/reso... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2053693002/diff/1/content/browser/loader/reso... content/browser/loader/resource_dispatcher_host_impl.cc:1486: if (request_data.resource_type == RESOURCE_TYPE_MAIN_FRAME) { BUG: Need to duplicate main frame request logic from BeginNavigationRequest here. PlzNavigate goes through the other path, but other main frame requests still currently go through here, I believe. https://codereview.chromium.org/2053693002/diff/1/content/browser/loader/reso... content/browser/loader/resource_dispatcher_host_impl.cc:2230: load_flags |= net::LOAD_MAIN_FRAME; Looks like we can we go through this for subframes, too? I'm not up on all the navigation logic, but if so, need to handle the subframe navigation case. https://codereview.chromium.org/2053693002/diff/1/content/browser/loader/reso... content/browser/loader/resource_dispatcher_host_impl.cc:2256: } I'm not following how the "UPGRADE_SAME_HOST_INSECURE_REQUESTS" logic. There's mention of an "upgrade insecure navigations set", but I'm not following a word of the description for how the "insecure navigations set" is created, and have no idea how all that maps to "UPGRADE_SAME_HOST_INSECURE_REQUESTS". https://codereview.chromium.org/2053693002/diff/1/content/browser/net/insecur... File content/browser/net/insecure_request_interceptor.cc (right): https://codereview.chromium.org/2053693002/diff/1/content/browser/net/insecur... content/browser/net/insecure_request_interceptor.cc:61: if (scheme_is_http || url.SchemeIs(url::kWsScheme)) { I'm not all that familiar with websocket requests, but I don't believe they go through the RDH, though they do go through the interceptors (I think?), so they won't have UPGRADE_SAME_HOST_INSECURE_REQUESTS set. In summary...Need tests.
On 2016/06/14 18:49:31, mmenke wrote: > Not a huge fan of the idea, but yea, I think the way to go is to put the logic > in URLRequest. And by "URLRequest", I of course mean have URLRequest store the setting, and put the check in URLRequestHttpJob::Factory. :)
On 2016/06/14 at 18:50:27, mmenke wrote: > On 2016/06/14 18:49:31, mmenke wrote: > Sorry, forgot about this one. In my queue for tomorrow, after I do my simpler reviews. No worries, and thank you for the comments! I'm booked up through next week, so I'll just drop this out of your queue until I have a chance to rework it. > > Not a huge fan of the idea, but yea, I think the way to go is to put the logic > > in URLRequest. > > And by "URLRequest", I of course mean have URLRequest store the setting, and put the check in URLRequestHttpJob::Factory. :) How would you feel about renaming `GetHSTSRedirect` to something like `GetSecureRedirect` and doing the check as part of that method?
mkwst@chromium.org changed reviewers: - mmenke@chromium.org
On 2016/06/16 11:59:53, Mike West (OOO - BlinkOn) wrote: > On 2016/06/14 at 18:50:27, mmenke wrote: > > On 2016/06/14 18:49:31, mmenke wrote: > > Sorry, forgot about this one. In my queue for tomorrow, after I do my simpler > reviews. > > No worries, and thank you for the comments! I'm booked up through next week, so > I'll just drop this out of your queue until I have a chance to rework it. > > > > Not a huge fan of the idea, but yea, I think the way to go is to put the > logic > > > in URLRequest. > > > > And by "URLRequest", I of course mean have URLRequest store the setting, and > put the check in URLRequestHttpJob::Factory. :) > > How would you feel about renaming `GetHSTSRedirect` to something like > `GetSecureRedirect` and doing the check as part of that method? My opinion is actually that GetHSTSRedirect does not belong it URLRequest - it's an HTTP feature, not a general URLRequest thing, and doesn't need anything from the URLRequest other than the URL. I'd actually rather see that moved to URLRequestHttpJob, and upgrade insecure requests logic put in there, too, just to keep everything together.
The CQ bit was checked by mkwst@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Your CL was about to rely on recently removed CQ feature(s): * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without "master." prefix is no longer supported: tryserver.chromium.linux For more details, see http://crbug.com/617627.
mkwst@chromium.org changed reviewers: + elawrence@chromium.org, estark@chromium.org, mmenke@chromium.org
Hello, friendly mmenke@. :) I'm resurrecting this patch. Skimming through the comments, you suggested that you didn't like the interceptor variant, so you might be wondering why I went back to it after the patchset in June that moved everything to URLRequestHTTPJob. An excellent question! In short, the timing is bad. 'Upgrade-Insecure-Requests' needs to happen before mixed content checks. 'Strict-Transport-Security' on the other hand, needs to happen _after_ mixed content checks. Once we implement https://wicg.github.io/hsts-priming/, we might be able to change that, but for the moment, we either need to modify a redirected request inside Blink (which I think you veto'd), or modify the request before we hit Blink. An interceptor has that timing, whereas the internal redirect mechanism doesn't. If you're still not a fan of this implementation, it's probably worth chatting in a slightly higher-bandwidth setting to work something out. :) WDYT? elawrence@, estark@: FYI. Feedback is ever so welcome from y'all as well. (Also, I have no idea how to make CSP reporting work for redirected resources after this patch... maybe we need to send a message down to the renderer regardless... Ugh.) :)
arthursonzogni@chromium.org changed reviewers: + arthursonzogni@chromium.org
+CC myself (arthursonzogni) I write almost the same code 1 month ago without knowing you have already written it 5-6 month ago :) https://codereview.chromium.org/2445993006/ I was blocked because I could not use the 'upgrade insecure navigations set' inside the interceptor (inside blink) and I was not sure to fully understand the specification at that time. I have one comment about your patch: Instead of using the 'upgrade insecure navigations set', it looks like you used UPGRADE_SAME_HOST_INSECURE_REQUESTS and the request's initiator. I understand that it covers 99% of the use cases, which is a good thing because redirection and upgrade insecure requests doesn't work currently. But some edge cases are still not covered like this one: --- Two iframes: A containing B. B asks A to navigate, but the request is redirected to A's origin. Then the request must be upgraded, but it will not be upgraded. The 'upgrade insecure navigations set' of B is {A,B}, but the check is made against B only. --- Am I right?
On 2016/12/06 14:57:34, Mike West (slow) wrote: > Hello, friendly mmenke@. :) Friendly? I think you have the wrong mmenke. :) So I'm going to say no to any implementation where URLRequest holds the flag, but content/ implements the behavior - that approach doesn't make any sense to me. Either URLRequest should explicitly have the field, and net/ should handle it, or ResourceRequestInfo should hold the data, and content should implement it. (Or could make use of URLRequest subclassing SupportsUserData, but given that this is partially implemented in loader/, that doesn't seem a better approach than the ResourceRequestInfo one, though ResourceRequestInfo is kinda huge). > I'm resurrecting this patch. Skimming through the comments, you suggested that > you didn't like the interceptor variant, so you might be wondering why I went > back to it after the patchset in June that moved everything to > URLRequestHTTPJob. > > An excellent question! > > In short, the timing is bad. 'Upgrade-Insecure-Requests' needs to happen before > mixed content checks. 'Strict-Transport-Security' on the other hand, needs to > happen _after_ mixed content checks. Once we implement > https://wicg.github.io/hsts-priming/, we might be able to change that, but for > the moment, we either need to modify a redirected request inside Blink (which I > think you veto'd), or modify the request before we hit Blink. An interceptor has > that timing, whereas the internal redirect mechanism doesn't. I don't understand why this implementation is before the secure context checks, and implementing this in net/ is not. > If you're still not a fan of this implementation, it's probably worth chatting > in a slightly higher-bandwidth setting to work something out. :) > > WDYT? > > elawrence@, estark@: FYI. Feedback is ever so welcome from y'all as well. (Also, > I have no idea how to make CSP reporting work for redirected resources after > this patch... maybe we need to send a message down to the renderer regardless... > Ugh.) :)
On 2016/12/06 at 16:03:28, mmenke wrote: > On 2016/12/06 14:57:34, Mike West (slow) wrote: > > Hello, friendly mmenke@. :) > > Friendly? I think you have the wrong mmenke. :) > > So I'm going to say no to any implementation where URLRequest holds the flag, but content/ implements the behavior - that approach doesn't make any sense to me. > > Either URLRequest should explicitly have the field, and net/ should handle it, or ResourceRequestInfo should hold the data, and content should implement it. (Or could make use of URLRequest subclassing SupportsUserData, but given that this is partially implemented in loader/, that doesn't seem a better approach than the ResourceRequestInfo one, though ResourceRequestInfo is kinda huge). Makes sense. I'll look at `ResourceRequestInfo` tomorrow. > > In short, the timing is bad. 'Upgrade-Insecure-Requests' needs to happen before > > mixed content checks. 'Strict-Transport-Security' on the other hand, needs to > > happen _after_ mixed content checks. Once we implement > > https://wicg.github.io/hsts-priming/, we might be able to change that, but for > > the moment, we either need to modify a redirected request inside Blink (which I > > think you veto'd), or modify the request before we hit Blink. An interceptor has > > that timing, whereas the internal redirect mechanism doesn't. > > I don't understand why this implementation is before the secure context checks, and implementing this in net/ is not. Because the interceptor triggers before we send the "We just totally redirected." message to ResourceLoader->Blink, whereas HSTS triggers after the various delegates have had a crack at cancelling the request (the former is triggered in `URLRequest::NotifyReceivedRedirect`: https://cs.chromium.org/chromium/src/net/url_request/url_request.cc?rcl=14810..., the latter from `URLRequestHttpJob::Factory` after all the notifications in https://cs.chromium.org/chromium/src/net/url_request/url_request_http_job.cc?...). We could certainly implement the interceptor in //net. It's not clear to me where we'd hook it in (the only non-test Interceptors I see are outside of //net), but I'm not dogmatic about putting the behavior in //content. It felt like a platform thing and not a network thing, but I'm happy to implement whatever you'd like. :)
On 2016/12/06 16:31:15, Mike West (slow) wrote: > On 2016/12/06 at 16:03:28, mmenke wrote: > > On 2016/12/06 14:57:34, Mike West (slow) wrote: > > > Hello, friendly mmenke@. :) > > > > Friendly? I think you have the wrong mmenke. :) > > > > So I'm going to say no to any implementation where URLRequest holds the flag, > but content/ implements the behavior - that approach doesn't make any sense to > me. > > > > Either URLRequest should explicitly have the field, and net/ should handle it, > or ResourceRequestInfo should hold the data, and content should implement it. > (Or could make use of URLRequest subclassing SupportsUserData, but given that > this is partially implemented in loader/, that doesn't seem a better approach > than the ResourceRequestInfo one, though ResourceRequestInfo is kinda huge). > > Makes sense. I'll look at `ResourceRequestInfo` tomorrow. > > > > In short, the timing is bad. 'Upgrade-Insecure-Requests' needs to happen > before > > > mixed content checks. 'Strict-Transport-Security' on the other hand, needs > to > > > happen _after_ mixed content checks. Once we implement > > > https://wicg.github.io/hsts-priming/, we might be able to change that, but > for > > > the moment, we either need to modify a redirected request inside Blink > (which I > > > think you veto'd), or modify the request before we hit Blink. An interceptor > has > > > that timing, whereas the internal redirect mechanism doesn't. > > > > I don't understand why this implementation is before the secure context > checks, and implementing this in net/ is not. > > Because the interceptor triggers before we send the "We just totally > redirected." message to ResourceLoader->Blink, whereas HSTS triggers after the > various delegates have had a crack at cancelling the request (the former is > triggered in `URLRequest::NotifyReceivedRedirect`: > https://cs.chromium.org/chromium/src/net/url_request/url_request.cc?rcl=14810..., > the latter from `URLRequestHttpJob::Factory` after all the notifications in > https://cs.chromium.org/chromium/src/net/url_request/url_request_http_job.cc?...). HSTS triggers in URLRequestHttpJob::Factory, which is where we end up when no interceptor catches the request earlier. So they hook at the exact same point. > We could certainly implement the interceptor in //net. It's not clear to me > where we'd hook it in (the only non-test Interceptors I see are outside of > //net), but I'm not dogmatic about putting the behavior in //content. It felt > like a platform thing and not a network thing, but I'm happy to implement > whatever you'd like. :)
On 2016/12/06 16:39:38, mmenke wrote: > On 2016/12/06 16:31:15, Mike West (slow) wrote: > > On 2016/12/06 at 16:03:28, mmenke wrote: > > > On 2016/12/06 14:57:34, Mike West (slow) wrote: > > > > Hello, friendly mmenke@. :) > > > > > > Friendly? I think you have the wrong mmenke. :) > > > > > > So I'm going to say no to any implementation where URLRequest holds the > flag, > > but content/ implements the behavior - that approach doesn't make any sense to > > me. > > > > > > Either URLRequest should explicitly have the field, and net/ should handle > it, > > or ResourceRequestInfo should hold the data, and content should implement it. > > (Or could make use of URLRequest subclassing SupportsUserData, but given that > > this is partially implemented in loader/, that doesn't seem a better approach > > than the ResourceRequestInfo one, though ResourceRequestInfo is kinda huge). > > > > Makes sense. I'll look at `ResourceRequestInfo` tomorrow. > > > > > > In short, the timing is bad. 'Upgrade-Insecure-Requests' needs to happen > > before > > > > mixed content checks. 'Strict-Transport-Security' on the other hand, needs > > to > > > > happen _after_ mixed content checks. Once we implement > > > > https://wicg.github.io/hsts-priming/, we might be able to change that, but > > for > > > > the moment, we either need to modify a redirected request inside Blink > > (which I > > > > think you veto'd), or modify the request before we hit Blink. An > interceptor > > has > > > > that timing, whereas the internal redirect mechanism doesn't. > > > > > > I don't understand why this implementation is before the secure context > > checks, and implementing this in net/ is not. > > > > Because the interceptor triggers before we send the "We just totally > > redirected." message to ResourceLoader->Blink, whereas HSTS triggers after the > > various delegates have had a crack at cancelling the request (the former is > > triggered in `URLRequest::NotifyReceivedRedirect`: > > > https://cs.chromium.org/chromium/src/net/url_request/url_request.cc?rcl=14810..., > > the latter from `URLRequestHttpJob::Factory` after all the notifications in > > > https://cs.chromium.org/chromium/src/net/url_request/url_request_http_job.cc?...). > > HSTS triggers in URLRequestHttpJob::Factory, which is where we end up when no > interceptor catches the request earlier. So they hook at the exact same point. (The exact same point, modulo other interceptors - like AppCache and ServiceWorker, which I assume should be getting first crach here, since neither goes over the network)
On 2016/12/06 at 15:55:39, arthursonzogni wrote: > +CC myself (arthursonzogni) > > I write almost the same code 1 month ago without knowing you have already written it 5-6 month ago :) > https://codereview.chromium.org/2445993006/ Wow. Yeah, it's very similar! Great minds... ;) > I was blocked because I could not use the 'upgrade insecure navigations set' inside the interceptor (inside blink) and I was not sure to fully understand the specification at that time. > > I have one comment about your patch: > Instead of using the 'upgrade insecure navigations set', it looks like you used UPGRADE_SAME_HOST_INSECURE_REQUESTS and the request's initiator. I understand that it covers 99% of the use cases, which is a good thing because redirection and upgrade insecure requests doesn't work currently. But some edge cases are still not covered like this one: > --- > Two iframes: A containing B. > B asks A to navigate, but the request is redirected to A's origin. > Then the request must be upgraded, but it will not be upgraded. > The 'upgrade insecure navigations set' of B is {A,B}, but the check is made against B only. > --- > Am I right? You're right. This patch takes care of a huge chunk of what we're missing, but basically punts on top-level navigation. Our implementation makes doing the thing the spec says we should do very hard, and doing most of what the spec says isn't nearly as hard. So, let's do that and worry about the gap once we have sane redirect behavior to begin with. Good > Perfect, etc. :)
On 2016/12/06 at 16:39:38, mmenke wrote: > On 2016/12/06 16:31:15, Mike West (slow) wrote: > > On 2016/12/06 at 16:03:28, mmenke wrote: > > > On 2016/12/06 14:57:34, Mike West (slow) wrote: > > > > Hello, friendly mmenke@. :) > > > > > > Friendly? I think you have the wrong mmenke. :) > > > > > > So I'm going to say no to any implementation where URLRequest holds the flag, > > but content/ implements the behavior - that approach doesn't make any sense to > > me. > > > > > > Either URLRequest should explicitly have the field, and net/ should handle it, > > or ResourceRequestInfo should hold the data, and content should implement it. > > (Or could make use of URLRequest subclassing SupportsUserData, but given that > > this is partially implemented in loader/, that doesn't seem a better approach > > than the ResourceRequestInfo one, though ResourceRequestInfo is kinda huge). > > > > Makes sense. I'll look at `ResourceRequestInfo` tomorrow. > > > > > > In short, the timing is bad. 'Upgrade-Insecure-Requests' needs to happen > > before > > > > mixed content checks. 'Strict-Transport-Security' on the other hand, needs > > to > > > > happen _after_ mixed content checks. Once we implement > > > > https://wicg.github.io/hsts-priming/, we might be able to change that, but > > for > > > > the moment, we either need to modify a redirected request inside Blink > > (which I > > > > think you veto'd), or modify the request before we hit Blink. An interceptor > > has > > > > that timing, whereas the internal redirect mechanism doesn't. > > > > > > I don't understand why this implementation is before the secure context > > checks, and implementing this in net/ is not. > > > > Because the interceptor triggers before we send the "We just totally > > redirected." message to ResourceLoader->Blink, whereas HSTS triggers after the > > various delegates have had a crack at cancelling the request (the former is > > triggered in `URLRequest::NotifyReceivedRedirect`: > > https://cs.chromium.org/chromium/src/net/url_request/url_request.cc?rcl=14810..., > > the latter from `URLRequestHttpJob::Factory` after all the notifications in > > https://cs.chromium.org/chromium/src/net/url_request/url_request_http_job.cc?...). > > HSTS triggers in URLRequestHttpJob::Factory, which is where we end up when no interceptor catches the request earlier. So they hook at the exact same point. In `URLRequestJob::NotifyHeadersComplete`, we determine that we got a redirect response from the server, and call `URLRequest::NotifyReceivedRedirect`, which kicks off the interceptor flow. If you look at `URLRequest::NotifyReceivedRedirect`, you'll see that if an interceptor intercepts the redirect and returns a URLRequestJob, we call `RestartWithJob` on that request, and do not call `delegate_->OnReceivedRedirect()`. That means that we never tell the ResourceLoader that we received a redirect. Which means that we never tell Blink. Which means Blink doesn't tell us not to follow the redirect. So, the flow for a request is somewhat convoluted from Blink's position: 1. Blink sees `<script src="http://whatever.com/">` 2. Blink says "Oh, I should totally upgrade that." and rewrites the URL to HTTPS. 3. The HTTPS URL it just rewrote passes the mixed content check. 4. Blink asks the browser to go out and grab the totally secure data. 5. The network stack grabs the data, and it turns out to be a 302 to `http://not-secure.com/`. 6. The network stack process the redirect, and tells Blink (via the delegate) that it's about to redirect to `http://not-secure.com`. 7. Blink cancels the redirect, as it fails a mixed content check. 8. Sadness. This patch changes things to replace step 6, 7, and 8 with: 6. //content intercepts the redirect response, and injects an internal redirect to `https://not-secure.com/`. 7. The network stack processes that redirect, and tells Blink (via the delegate) that it's about to redirect to `https://not-secure.com/` 8. Blink allows the redirect, as it passes a mixed content check. 9. Party. It would make me happier if Blink could _rewrite_ the redirect in something like `blink::ResourceFetcher::willFollowRedirect` (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/fetch/Res...), because then various things would be simpler. I guess we could even fake Blink out by pretending to rewrite the redirect for the purposes of the `canRequest()` call, but it would be strange for Blink to do one set of checks on a response, and the network stack to update it's understanding of what's going on at some later point. That seems risky, so I didn't pursue it.
On 2016/12/06 17:01:10, Mike West (slow) wrote: > On 2016/12/06 at 16:39:38, mmenke wrote: > > On 2016/12/06 16:31:15, Mike West (slow) wrote: > > > On 2016/12/06 at 16:03:28, mmenke wrote: > > > > On 2016/12/06 14:57:34, Mike West (slow) wrote: > > > > > Hello, friendly mmenke@. :) > > > > > > > > Friendly? I think you have the wrong mmenke. :) > > > > > > > > So I'm going to say no to any implementation where URLRequest holds the > flag, > > > but content/ implements the behavior - that approach doesn't make any sense > to > > > me. > > > > > > > > Either URLRequest should explicitly have the field, and net/ should handle > it, > > > or ResourceRequestInfo should hold the data, and content should implement > it. > > > (Or could make use of URLRequest subclassing SupportsUserData, but given > that > > > this is partially implemented in loader/, that doesn't seem a better > approach > > > than the ResourceRequestInfo one, though ResourceRequestInfo is kinda huge). > > > > > > Makes sense. I'll look at `ResourceRequestInfo` tomorrow. > > > > > > > > In short, the timing is bad. 'Upgrade-Insecure-Requests' needs to happen > > > before > > > > > mixed content checks. 'Strict-Transport-Security' on the other hand, > needs > > > to > > > > > happen _after_ mixed content checks. Once we implement > > > > > https://wicg.github.io/hsts-priming/, we might be able to change that, > but > > > for > > > > > the moment, we either need to modify a redirected request inside Blink > > > (which I > > > > > think you veto'd), or modify the request before we hit Blink. An > interceptor > > > has > > > > > that timing, whereas the internal redirect mechanism doesn't. > > > > > > > > I don't understand why this implementation is before the secure context > > > checks, and implementing this in net/ is not. > > > > > > Because the interceptor triggers before we send the "We just totally > > > redirected." message to ResourceLoader->Blink, whereas HSTS triggers after > the > > > various delegates have had a crack at cancelling the request (the former is > > > triggered in `URLRequest::NotifyReceivedRedirect`: > > > > https://cs.chromium.org/chromium/src/net/url_request/url_request.cc?rcl=14810..., > > > the latter from `URLRequestHttpJob::Factory` after all the notifications in > > > > https://cs.chromium.org/chromium/src/net/url_request/url_request_http_job.cc?...). > > > > HSTS triggers in URLRequestHttpJob::Factory, which is where we end up when no > interceptor catches the request earlier. So they hook at the exact same point. > > In `URLRequestJob::NotifyHeadersComplete`, we determine that we got a redirect > response from the server, and call `URLRequest::NotifyReceivedRedirect`, which > kicks off the interceptor flow. If you look at > `URLRequest::NotifyReceivedRedirect`, you'll see that if an interceptor > intercepts the redirect and returns a URLRequestJob, we call `RestartWithJob` on > that request, and do not call `delegate_->OnReceivedRedirect()`. > > That means that we never tell the ResourceLoader that we received a redirect. > Which means that we never tell Blink. Which means Blink doesn't tell us not to > follow the redirect. > > So, the flow for a request is somewhat convoluted from Blink's position: > > 1. Blink sees `<script src="http://whatever.com/">` > 2. Blink says "Oh, I should totally upgrade that." and rewrites the URL to > HTTPS. > 3. The HTTPS URL it just rewrote passes the mixed content check. > 4. Blink asks the browser to go out and grab the totally secure data. > 5. The network stack grabs the data, and it turns out to be a 302 to > `http://not-secure.com/`. > 6. The network stack process the redirect, and tells Blink (via the delegate) > that it's about to redirect to `http://not-secure.com`. > 7. Blink cancels the redirect, as it fails a mixed content check. > 8. Sadness. > > This patch changes things to replace step 6, 7, and 8 with: > > 6. //content intercepts the redirect response, and injects an internal redirect > to `https://not-secure.com/`. > 7. The network stack processes that redirect, and tells Blink (via the delegate) > that it's about to redirect to `https://not-secure.com/` > 8. Blink allows the redirect, as it passes a mixed content check. > 9. Party. > > It would make me happier if Blink could _rewrite_ the redirect in something like > `blink::ResourceFetcher::willFollowRedirect` > (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/fetch/Res...), > because then various things would be simpler. > > I guess we could even fake Blink out by pretending to rewrite the redirect for > the purposes of the `canRequest()` call, but it would be strange for Blink to do > one set of checks on a response, and the network stack to update it's > understanding of what's going on at some later point. That seems risky, so I > didn't pursue it. Ok, so this is all to avoid the redirect notification.... This seems a bit weird, as we're not notifying anything of redirected redirects, and we're lying about extensions to where we're redirected to, etc. The fact that we're not reporting anything about this magic redirect seems problematic to me. The fact that we issue a request for a URL that's different from the URLRequest's URL seems like a bug.
On 2016/12/06 at 17:07:47, mmenke wrote: > Ok, so this is all to avoid the redirect notification. Only insofar as the redirect notification is tied to cancellation behavior and security checks. I'm happy to notify Blink about the redirect if we can also give Blink the ability to poke at the redirect in ways that aren't limited to cancellation. There's a comment in ResourceLoader to that effect (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/fetch/Res...), though I'm not sure it reflects the //net team's opinions. >This seems a bit weird, as we're not notifying anything of redirected redirects, and we're lying about extensions to where we're redirected to, etc. The fact that we're not reporting anything about this magic redirect seems problematic to me. This is status-quo behavior for interceptors. I guess I assumed it was the behavior y'all wanted? Perhaps we could teach Blink to respond to a redirect with a redirect if you'd be happier with that? > The fact that we issue a request for a URL that's different from the URLRequest's URL seems like a bug. This is all pretty complicated. I guess I'm willing to do a little bit of work to make it more sane, but I'm not sure I have bandwidth to restructure things deeply. Perhaps it would be worth talking a little bit about what you'd like to see happen here?
On 2016/12/06 17:39:10, Mike West (slow) wrote: > On 2016/12/06 at 17:07:47, mmenke wrote: > > Ok, so this is all to avoid the redirect notification. > > Only insofar as the redirect notification is tied to cancellation behavior and > security checks. > > I'm happy to notify Blink about the redirect if we can also give Blink the > ability to poke at the redirect in ways that aren't limited to cancellation. > There's a comment in ResourceLoader to that effect > (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/fetch/Res...), > though I'm not sure it reflects the //net team's opinions. > > >This seems a bit weird, as we're not notifying anything of redirected > redirects, and we're lying about extensions to where we're redirected to, etc. > The fact that we're not reporting anything about this magic redirect seems > problematic to me. > > This is status-quo behavior for interceptors. I guess I assumed it was the > behavior y'all wanted? Perhaps we could teach Blink to respond to a redirect > with a redirect if you'd be happier with that? This is behavior that was added for appcache, which is going away. It's also used by DRP, but just to retry the original URL (Which is something that we can special case). This is actually a call I'd really rather not depend on, going forward. > > The fact that we issue a request for a URL that's different from the > URLRequest's URL seems like a bug. > > This is all pretty complicated. I guess I'm willing to do a little bit of work > to make it more sane, but I'm not sure I have bandwidth to restructure things > deeply. Perhaps it would be worth talking a little bit about what you'd like to > see happen here? We tell everything we're following one redirect, and then tell everything that we're following another redirect, which seems problematic... It's not as bad as I was initially thinking, since we just return a redirect, but I want to see if we can come up with another approach.
On 2016/12/06 at 17:46:31, mmenke wrote: > This is behavior that was added for appcache, which is going away. It's also used by DRP, but just to retry the original URL (Which is something that we can special case). This is actually a call I'd really rather not depend on, going forward. Ok. So, assuming that interceptors didn't exist, I can think of a few options: 1. We lump UIR in with HSTS (as per patchset #2 above). In order to make the feature work correctly, we'll need to poke at Blink to ensure that security checks that would pass after UIR also pass before UIR. That's doable, but it's a bit ugly. 2. We allow delegate to respond to a redirect notification with a redirect. We do something like this for extensions' WebRequest API (though I haven't looked at where that hooks in... it might be an interceptor? :) ), and it doesn't seem completely insane to me. Note that whatever we decide here, we'll eventually want to do the same thing for HSTS, once we resolve the determinism problem discussed in https://wicg.github.io/hsts-priming/. > We tell everything we're following one redirect, and then tell everything that we're following another redirect, which seems problematic... It's not as bad as I was initially thinking, since we just return a redirect, but I want to see if we can come up with another approach. *shrug* To me, this doesn't seem like a problem. The browser is making a decision a priori about the way it's going to handle a specific request. We have all the information we need at this point in order to make the decision, so why go through all the additional layers? Also, as a thing to note: Fetch defines both UIR and HSTS are specified as poking at the request's URL at request time, and not as redirects (https://fetch.spec.whatwg.org/#main-fetch). This doesn't mean that we have to do it that way (and I'm pretty sure Firefox also implements HSTS as a redirect internally), but it's the way folks are thinking about the features.
On 2016/12/06 at 18:04:02, Mike West (slow) wrote: > On 2016/12/06 at 17:46:31, mmenke wrote: > > This is behavior that was added for appcache, which is going away. It's also used by DRP, but just to retry the original URL (Which is something that we can special case). This is actually a call I'd really rather not depend on, going forward. > > Ok. So, assuming that interceptors didn't exist, I can think of a few options: > > 1. We lump UIR in with HSTS (as per patchset #2 above). In order to make the feature work correctly, we'll need to poke at Blink to ensure that security checks that would pass after UIR also pass before UIR. That's doable, but it's a bit ugly. > > 2. We allow delegate to respond to a redirect notification with a redirect. We do something like this for extensions' WebRequest API (though I haven't looked at where that hooks in... it might be an interceptor? :) ), and it doesn't seem completely insane to me. > > Note that whatever we decide here, we'll eventually want to do the same thing for HSTS, once we resolve the determinism problem discussed in https://wicg.github.io/hsts-priming/. > > > We tell everything we're following one redirect, and then tell everything that we're following another redirect, which seems problematic... It's not as bad as I was initially thinking, since we just return a redirect, but I want to see if we can come up with another approach. > > *shrug* To me, this doesn't seem like a problem. The browser is making a decision a priori about the way it's going to handle a specific request. We have all the information we need at this point in order to make the decision, so why go through all the additional layers? > > Also, as a thing to note: Fetch defines both UIR and HSTS are specified as poking at the request's URL at request time, and not as redirects (https://fetch.spec.whatwg.org/#main-fetch). This doesn't mean that we have to do it that way (and I'm pretty sure Firefox also implements HSTS as a redirect internally), but it's the way folks are thinking about the features. mmenke@: Ping! I'd like some feedback from you about how you'd like this to be structured. I'm not terribly excited about any of the options. :)
On 2016/12/08 05:42:19, Mike West (slow) wrote: > On 2016/12/06 at 18:04:02, Mike West (slow) wrote: > > On 2016/12/06 at 17:46:31, mmenke wrote: > > > This is behavior that was added for appcache, which is going away. It's > also used by DRP, but just to retry the original URL (Which is something that we > can special case). This is actually a call I'd really rather not depend on, > going forward. > > > > Ok. So, assuming that interceptors didn't exist, I can think of a few options: > > > > 1. We lump UIR in with HSTS (as per patchset #2 above). In order to make the > feature work correctly, we'll need to poke at Blink to ensure that security > checks that would pass after UIR also pass before UIR. That's doable, but it's a > bit ugly. > > > > 2. We allow delegate to respond to a redirect notification with a redirect. We > do something like this for extensions' WebRequest API (though I haven't looked > at where that hooks in... it might be an interceptor? :) ), and it doesn't seem > completely insane to me. > > > > Note that whatever we decide here, we'll eventually want to do the same thing > for HSTS, once we resolve the determinism problem discussed in > https://wicg.github.io/hsts-priming/. > > > > > We tell everything we're following one redirect, and then tell everything > that we're following another redirect, which seems problematic... It's not as > bad as I was initially thinking, since we just return a redirect, but I want to > see if we can come up with another approach. > > > > *shrug* To me, this doesn't seem like a problem. The browser is making a > decision a priori about the way it's going to handle a specific request. We have > all the information we need at this point in order to make the decision, so why > go through all the additional layers? Layers like to do weird things. Extensions do weird stuff. This basically forces an unexpected pattern of events on all 24 gazzilion ResourceHandlers and ResourceThrottles parasitically dangling off of ResourceLoader, as well as the ResourceDispatcherHostDelegate. Do they all handle this unexpected flow correctly? I have no clue, so it's best to at least try and give them a consistent API. It's not blink I'm concerned about, but everything else. > > Also, as a thing to note: Fetch defines both UIR and HSTS are specified as > poking at the request's URL at request time, and not as redirects > (https://fetch.spec.whatwg.org/#main-fetch). This doesn't mean that we have to > do it that way (and I'm pretty sure Firefox also implements HSTS as a redirect > internally), but it's the way folks are thinking about the features. > > mmenke@: Ping! I'd like some feedback from you about how you'd like this to be > structured. I'm not terribly excited about any of the options. :) Sorry, planned to get back to this, but didn't realize this was fully in my camp. So if blink is telling the browser process that it should automatically "upgrade" non-HTTP requests to HTTPS, should blink still be cancelling non-HTTPS redirects itself? Seems like it's usually the renderer we don't trust, not the browser.
On 2016/12/08 at 17:45:35, mmenke wrote: > Layers like to do weird things. Extensions do weird stuff. This basically forces an unexpected pattern of events on all 24 gazzilion ResourceHandlers and ResourceThrottles parasitically dangling off of ResourceLoader, as well as the ResourceDispatcherHostDelegate. Do they all handle this unexpected flow correctly? I have no clue, so it's best to at least try and give them a consistent API. Given timing of the existing interceptors, it seems like all the layers must already deal with an immediate redirect in a reasonable way. But I understand why you don't want to lock us into that mechanism, so I'm happy to work with you to come up with a better solution. > > > Also, as a thing to note: Fetch defines both UIR and HSTS are specified as > > poking at the request's URL at request time, and not as redirects > > (https://fetch.spec.whatwg.org/#main-fetch). This doesn't mean that we have to > > do it that way (and I'm pretty sure Firefox also implements HSTS as a redirect > > internally), but it's the way folks are thinking about the features. > > > > mmenke@: Ping! I'd like some feedback from you about how you'd like this to be > > structured. I'm not terribly excited about any of the options. :) > > Sorry, planned to get back to this, but didn't realize this was fully in my camp. I appreciate the time you take to review things thoroughly, and this isn't a super-high priority. I'd like to get it fixed, but it's not something I want you to stop doing actually important work to walk me through. :) > So if blink is telling the browser process that it should automatically "upgrade" non-HTTP requests to HTTPS, should blink still be cancelling non-HTTPS redirects itself? Seems like it's usually the renderer we don't trust, not the browser. Hrm. If we're going to explicitly delegate the authority to Blink's embedder, then I'd prefer not to reenter Blink at all before the internal redirect. Ok, what do you think about changing `content::WebURLLoaderImpl::Context::OnReceivedRedirect` (or something even higher up... `content::ResourceDispatcher::OnReceivedRedirect`?) to follow the redirect if the target is insecure and the policy is set to "upgrade"?
On 2016/12/08 20:56:42, Mike West (slow) wrote: > On 2016/12/08 at 17:45:35, mmenke wrote: > > Layers like to do weird things. Extensions do weird stuff. This basically > forces an unexpected pattern of events on all 24 gazzilion ResourceHandlers and > ResourceThrottles parasitically dangling off of ResourceLoader, as well as the > ResourceDispatcherHostDelegate. Do they all handle this unexpected flow > correctly? I have no clue, so it's best to at least try and give them a > consistent API. > > Given timing of the existing interceptors, it seems like all the layers must > already deal with an immediate redirect in a reasonable way. But I understand > why you don't want to lock us into that mechanism, so I'm happy to work with you > to come up with a better solution. > > > > > Also, as a thing to note: Fetch defines both UIR and HSTS are specified as > > > poking at the request's URL at request time, and not as redirects > > > (https://fetch.spec.whatwg.org/#main-fetch). This doesn't mean that we have > to > > > do it that way (and I'm pretty sure Firefox also implements HSTS as a > redirect > > > internally), but it's the way folks are thinking about the features. > > > > > > mmenke@: Ping! I'd like some feedback from you about how you'd like this to > be > > > structured. I'm not terribly excited about any of the options. :) > > > > Sorry, planned to get back to this, but didn't realize this was fully in my > camp. > > I appreciate the time you take to review things thoroughly, and this isn't a > super-high priority. I'd like to get it fixed, but it's not something I want you > to stop doing actually important work to walk me through. :) > > > So if blink is telling the browser process that it should automatically > "upgrade" non-HTTP requests to HTTPS, should blink still be cancelling non-HTTPS > redirects itself? Seems like it's usually the renderer we don't trust, not the > browser. > > Hrm. If we're going to explicitly delegate the authority to Blink's embedder, > then I'd prefer not to reenter Blink at all before the internal redirect. Ok, > what do you think about changing > `content::WebURLLoaderImpl::Context::OnReceivedRedirect` (or something even > higher up... `content::ResourceDispatcher::OnReceivedRedirect`?) to follow the > redirect if the target is insecure and the policy is set to "upgrade"? I have a few concerns about that, all in areas I know nothing about: * What devtools/har files will show (vs what they should show). * What plugins see (Should they see they HTTP URL if they requested it). * What redirect chains we expose to any other web-visible APIs. I think we should have an understanding of all of these, whatever approach we go with. One other thing that's different in the two approaches: Referrers. If we skip the HTTP URL at the URLRequest layer, we clear the referrer (Unless a referrer policy is explicitly set), as we follow an HTTPS to HTTP redirect. Does the fetch spec make clear whether or not we should be doing that?
The CQ bit was checked by mkwst@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Your CL was about to rely on recently removed CQ feature(s): * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without "master." prefix is no longer supported: tryserver.chromium.linux For more details, see http://crbug.com/617627.
The latest patchset moves the upgrade back to URLHttpRequestJob, lumping UIR and HSTS together. As we'll discuss in a moment, that doesn't entirely match the spec. On 2016/12/08 at 21:15:22, mmenke wrote: > I have a few concerns about that, all in areas I know nothing about: > > * What devtools/har files will show (vs what they should show). In the current patchset, they'll show the same thing for UIR-upgraded redirects that they show for HSTS: a 307 internal redirect. This is a little strange, as they won't show the same for requests that don't go through the network stack (because Blink internally rewrites the URL before handing it to the network stack). Both HSTS and URI are specified in terms of rewriting the URL, not redirecting it. That said, I'm fairly certain that Firefox implements HSTS as an internal redirect, but it doesn't expose that to the inspector. > * What plugins see (Should they see they HTTP URL if they requested it). Again, as specified, no, they shouldn't. As implemented in the latest patchset, yes, they would. > * What redirect chains we expose to any other web-visible APIs. We don't expose redirect chains to web-visible APIs, do we? It seems dangerous to do so. > I think we should have an understanding of all of these, whatever approach we go with. I agree. I'd also say that just because the spec says rewrite doesn't mean we should do it. However: > One other thing that's different in the two approaches: Referrers. If we skip the HTTP URL at the URLRequest layer, we clear the referrer (Unless a referrer policy is explicitly set), as we follow an HTTPS to HTTP redirect. Does the fetch spec make clear whether or not we should be doing that? This is the kind of thing that rewriting as opposed to redirecting is supposed to handle. Referrer policy is one set of challenges, CORS is another. It's a bit strange to have different behavior after a redirect than before it. As an alternative to the current patch, how would you feel about being even more intrusive into the network stack's innards, and modifying something like `URLRequestJob::IsRedirectResponse` to rewrite the redirect target after reading it from the response? That's more in line with what the spec suggests that we do, and with the underlying intent which is to let developers seamlessly migrate their pages to HTTPS without risking weird breakage or degrading the page's UI?
The CQ bit was checked by mkwst@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Your CL was about to rely on recently removed CQ feature(s): * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without "master." prefix is no longer supported: tryserver.chromium.linux For more details, see http://crbug.com/617627.
On 2016/12/09 15:12:22, Mike West (slow) wrote: > The latest patchset moves the upgrade back to URLHttpRequestJob, lumping UIR and > HSTS together. As we'll discuss in a moment, that doesn't entirely match the > spec. > > On 2016/12/08 at 21:15:22, mmenke wrote: > > I have a few concerns about that, all in areas I know nothing about: > > > > * What devtools/har files will show (vs what they should show). > > In the current patchset, they'll show the same thing for UIR-upgraded redirects > that they show for HSTS: a 307 internal redirect. This is a little strange, as > they won't show the same for requests that don't go through the network stack > (because Blink internally rewrites the URL before handing it to the network > stack). > > Both HSTS and URI are specified in terms of rewriting the URL, not redirecting > it. That said, I'm fairly certain that Firefox implements HSTS as an internal > redirect, but it doesn't expose that to the inspector. > > > * What plugins see (Should they see they HTTP URL if they requested it). > > Again, as specified, no, they shouldn't. As implemented in the latest patchset, > yes, they would. > > > * What redirect chains we expose to any other web-visible APIs. > > We don't expose redirect chains to web-visible APIs, do we? It seems dangerous > to do so. > > > I think we should have an understanding of all of these, whatever approach we > go with. > > I agree. I'd also say that just because the spec says rewrite doesn't mean we > should do it. However: > > > One other thing that's different in the two approaches: Referrers. If we > skip the HTTP URL at the URLRequest layer, we clear the referrer (Unless a > referrer policy is explicitly set), as we follow an HTTPS to HTTP redirect. > Does the fetch spec make clear whether or not we should be doing that? > > This is the kind of thing that rewriting as opposed to redirecting is supposed > to handle. Referrer policy is one set of challenges, CORS is another. It's a bit > strange to have different behavior after a redirect than before it. > > As an alternative to the current patch, how would you feel about being even more > intrusive into the network stack's innards, and modifying something like > `URLRequestJob::IsRedirectResponse` to rewrite the redirect target after reading > it from the response? That's more in line with what the spec suggests that we > do, and with the underlying intent which is to let developers seamlessly migrate > their pages to HTTPS without risking weird breakage or degrading the page's UI? This approach seems reasonable to me.
On 2016/12/09 15:20:19, mmenke wrote: > On 2016/12/09 15:12:22, Mike West (slow) wrote: > > The latest patchset moves the upgrade back to URLHttpRequestJob, lumping UIR > and > > HSTS together. As we'll discuss in a moment, that doesn't entirely match the > > spec. > > > > On 2016/12/08 at 21:15:22, mmenke wrote: > > > I have a few concerns about that, all in areas I know nothing about: > > > > > > * What devtools/har files will show (vs what they should show). > > > > In the current patchset, they'll show the same thing for UIR-upgraded > redirects > > that they show for HSTS: a 307 internal redirect. This is a little strange, as > > they won't show the same for requests that don't go through the network stack > > (because Blink internally rewrites the URL before handing it to the network > > stack). > > > > Both HSTS and URI are specified in terms of rewriting the URL, not redirecting > > it. That said, I'm fairly certain that Firefox implements HSTS as an internal > > redirect, but it doesn't expose that to the inspector. > > > > > * What plugins see (Should they see they HTTP URL if they requested it). > > > > Again, as specified, no, they shouldn't. As implemented in the latest > patchset, > > yes, they would. > > > > > * What redirect chains we expose to any other web-visible APIs. > > > > We don't expose redirect chains to web-visible APIs, do we? It seems dangerous > > to do so. > > > > > I think we should have an understanding of all of these, whatever approach > we > > go with. > > > > I agree. I'd also say that just because the spec says rewrite doesn't mean we > > should do it. However: > > > > > One other thing that's different in the two approaches: Referrers. If we > > skip the HTTP URL at the URLRequest layer, we clear the referrer (Unless a > > referrer policy is explicitly set), as we follow an HTTPS to HTTP redirect. > > Does the fetch spec make clear whether or not we should be doing that? > > > > This is the kind of thing that rewriting as opposed to redirecting is supposed > > to handle. Referrer policy is one set of challenges, CORS is another. It's a > bit > > strange to have different behavior after a redirect than before it. > > > > As an alternative to the current patch, how would you feel about being even > more > > intrusive into the network stack's innards, and modifying something like > > `URLRequestJob::IsRedirectResponse` to rewrite the redirect target after > reading > > it from the response? That's more in line with what the spec suggests that we > > do, and with the underlying intent which is to let developers seamlessly > migrate > > their pages to HTTPS without risking weird breakage or degrading the page's > UI? > > This approach seems reasonable to me. We should also add something to NetLog when it happens. I assume we don't want to re-write the location header.
The CQ bit was checked by mkwst@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Your CL was about to rely on recently removed CQ feature(s): * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without "master." prefix is no longer supported: tryserver.chromium.linux For more details, see http://crbug.com/617627.
The CQ bit was checked by mkwst@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Your CL was about to rely on recently removed CQ feature(s): * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without "master." prefix is no longer supported: tryserver.chromium.linux For more details, see http://crbug.com/617627.
I took a stab at rewriting things in URLRequestHttpJob instead of in Blink. I'm not sure I like the result. :/ WDYT, mmenke@?
Hrm...yea, I think that the RewriteURL is pretty ugly. I prefer it to the Interceptor approach, but wish we could make this part of the request a bit more natural. Also worth noting that things hooking off the (content) ResourceLoader stack that look at the initial request URL, and do stuff based on it before the request starts won't catch these requests (Like setting headers). https://codereview.chromium.org/2053693002/diff/80001/net/url_request/url_req... File net/url_request/url_request.h (right): https://codereview.chromium.org/2053693002/diff/80001/net/url_request/url_req... net/url_request/url_request.h:687: void RewriteURL(const GURL& url, const std::string& reason); This should be protected (URLRequestJob is a friend, and if we let random consumers do weird tortuous stuff to URLs, they will) https://codereview.chromium.org/2053693002/diff/80001/net/url_request/url_req... File net/url_request/url_request_http_job.cc (right): https://codereview.chromium.org/2053693002/diff/80001/net/url_request/url_req... net/url_request/url_request_http_job.cc:246: MaybeRewriteRequestURL(request); The redirect stuff all looks pretty reasonable to me. This, though, does seem really ugly. Also, something occurred to me: With this approach, Blink would be hiding HTTPS referrers, since it doesn't know the HTTP request will actually go over HTTPS, right?
On 2016/12/13 at 19:00:24, mmenke wrote: > Hrm...yea, I think that the RewriteURL is pretty ugly. I prefer it to the Interceptor approach, but wish we could make this part of the request a bit more natural. > > Also worth noting that things hooking off the (content) ResourceLoader stack that look at the initial request URL, and do stuff based on it before the request starts won't catch these requests (Like setting headers). See below: this is an argument in favor of doing the upgrade in Blink and here. > https://codereview.chromium.org/2053693002/diff/80001/net/url_request/url_req... > net/url_request/url_request.h:687: void RewriteURL(const GURL& url, const std::string& reason); > This should be protected (URLRequestJob is a friend, and if we let random consumers do weird tortuous stuff to URLs, they will) `URLRequestHttpJob` isn't a friend. I guess we could route the rewrite through `URLRequestJob`, though. > https://codereview.chromium.org/2053693002/diff/80001/net/url_request/url_req... > net/url_request/url_request_http_job.cc:246: MaybeRewriteRequestURL(request); > The redirect stuff all looks pretty reasonable to me. This, though, does seem really ugly. Yeah. :/ > Also, something occurred to me: With this approach, Blink would be hiding HTTPS referrers, since it doesn't know the HTTP request will actually go over HTTPS, right? Yeah. An alternative would be to split the upgrade across Blink and //net. That is, Blink would continue to upgrade the request before sending it up to the network stack, and we'd tag it with the relevant policy to ensure that redirects were properly upgraded. We could add a DCHECK to `URLRequest::set_insecure_request_policy()` to ensure that any request with an upgrade policy has an already-upgraded URL, and drop the rewrite from `URLRequestHttpJob::Factory()`. I'm not sure that ends up being any better than patchset 4, but I can slap it together today to see what you think.
The CQ bit was checked by mkwst@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Your CL was about to rely on recently removed CQ feature(s): * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without "master." prefix is no longer supported: tryserver.chromium.linux For more details, see http://crbug.com/617627.
WDYT of the latest patchset, mmenke@?
From a net/ standpoint, this seems reasonable to me. This seems pretty similar to what we do with referrer policy. I think this is pretty reasonable, and would be happy to sign off on it. It would be nice if we could put all the logic in one place, but I'm not sure there's a goo way of doing that. Should I do a full review? https://codereview.chromium.org/2053693002/diff/100001/net/url_request/url_re... File net/url_request/url_request.cc (right): https://codereview.chromium.org/2053693002/diff/100001/net/url_request/url_re... net/url_request/url_request.cc:477: (!initiator() || The checks seems overly complicated... If !initiator(), we allow HTTP requests, even if the policy is UPGRADE_ALL_INSECURE_REQUESTS? Should this actually be: insecure_request_policy == UPGRADE_SAME_HOST_INSECURE_REQUESTS && (!initiator() || initiator()->host() != url().host()) or maybe insecure_request_policy == UPGRADE_SAME_HOST_INSECURE_REQUESTS && (!initiator() && initiator()->host() != url().host()) (i.e., don't allow UPGRADE_SAME_HOST_INSECURE_REQUESTS without an initiator, as it makes no sense)
The CQ bit was checked by mkwst@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Your CL was about to rely on recently removed CQ feature(s): * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without "master." prefix is no longer supported: tryserver.chromium.linux For more details, see http://crbug.com/617627.
mkwst@chromium.org changed reviewers: + jochen@chromium.org
On 2016/12/14 at 19:41:05, mmenke wrote: > From a net/ standpoint, this seems reasonable to me. This seems pretty similar to what we do with referrer policy. I think this is pretty reasonable, and would be happy to sign off on it. It would be nice if we could put all the logic in one place, but I'm not sure there's a goo way of doing that. Should I do a full review? If you're happy with the general shape, then yes, I'd appreciate you running through it in more detail and stamping it if you're happy. estark@, elawrence@: Would one of you mind taking a closer look as well? jochen@: I'd appreciate it if you could stamp the //content bits once mmenke@ is happy with //net. https://codereview.chromium.org/2053693002/diff/100001/net/url_request/url_re... File net/url_request/url_request.cc (right): https://codereview.chromium.org/2053693002/diff/100001/net/url_request/url_re... net/url_request/url_request.cc:477: (!initiator() || On 2016/12/14 at 19:41:05, mmenke (Out Dec 17 to Jan 2) wrote: > The checks seems overly complicated... If !initiator(), we allow HTTP requests, even if the policy is UPGRADE_ALL_INSECURE_REQUESTS? > > Should this actually be: > > insecure_request_policy == UPGRADE_SAME_HOST_INSECURE_REQUESTS && (!initiator() || initiator()->host() != url().host()) > > or maybe > > insecure_request_policy == UPGRADE_SAME_HOST_INSECURE_REQUESTS && (!initiator() && initiator()->host() != url().host()) (i.e., don't allow UPGRADE_SAME_HOST_INSECURE_REQUESTS without an initiator, as it makes no sense) Yes. This was me trying to be clever about doing cheap checks first, and screwing it up. :)
The CQ bit was checked by mkwst@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Your CL was about to rely on recently removed CQ feature(s): * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without "master." prefix is no longer supported: tryserver.chromium.linux For more details, see http://crbug.com/617627.
content/ lgtm
Bunch of mostly minor comments https://codereview.chromium.org/2053693002/diff/120001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2053693002/diff/120001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1450: // If the initiating context wishes to upgrade insecure requests, we nit: Avoid "we" in comments. Often unclear what it means. "We" the browser, "We" the class, "We" the chrome developers. May be especially confusing for non-native speakers (x2). https://codereview.chromium.org/2053693002/diff/120001/net/log/net_log_event_... File net/log/net_log_event_type_list.h (right): https://codereview.chromium.org/2053693002/diff/120001/net/log/net_log_event_... net/log/net_log_event_type_list.h:849: // "reason": <Reason for the rewrite, as a string>, Should we also log the new URL? https://codereview.chromium.org/2053693002/diff/120001/net/url_request/url_re... File net/url_request/url_request.cc (right): https://codereview.chromium.org/2053693002/diff/120001/net/url_request/url_re... net/url_request/url_request.cc:476: url().SchemeIsCryptographic() || We allow insecure_request_policy == UPGRADE_SAME_HOST_INSECURE_REQUESTS without an initiator for cryptographic schemes? Think this DCHECK needs a comment saying just what we're trying to do with it. https://codereview.chromium.org/2053693002/diff/120001/net/url_request/url_re... File net/url_request/url_request.h (right): https://codereview.chromium.org/2053693002/diff/120001/net/url_request/url_re... net/url_request/url_request.h:338: InsecureRequestPolicy insecure_request_policy); If we see a redirect response with Content-Security-Policy: upgrade-insecure-requests, should we change the policy? I guess not? https://codereview.chromium.org/2053693002/diff/120001/net/url_request/url_re... File net/url_request/url_request_http_job.cc (right): https://codereview.chromium.org/2053693002/diff/120001/net/url_request/url_re... net/url_request/url_request_http_job.cc:1147: request_->initiator()->host() != location.host())) { ->host() => ->host_piece()? No need to create new strings here. https://codereview.chromium.org/2053693002/diff/120001/net/url_request/url_re... net/url_request/url_request_http_job.cc:1147: request_->initiator()->host() != location.host())) { I think this would be clearer with the unusual case first. Checking for one of a set of cases in generally clearer than checking that we're not in one of a set of cases. https://codereview.chromium.org/2053693002/diff/120001/net/url_request/url_re... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/2053693002/diff/120001/net/url_request/url_re... net/url_request/url_request_unittest.cc:8317: class URLRequestTestInsecureRequestPolicy : public URLRequestTest { Given that we only have one test, do we really need a new test fixture? https://codereview.chromium.org/2053693002/diff/120001/net/url_request/url_re... net/url_request/url_request_unittest.cc:8323: context_.Init(); We can just use default_context_, no? It uses a MockCachingHostResolver and a TestNetLog, which do what you need already, and you don't need the TestNetworkDelegate. https://codereview.chromium.org/2053693002/diff/120001/net/url_request/url_re... net/url_request/url_request_unittest.cc:8333: EXPECT_TRUE(https_server_->Start()); I don't think we even need the second server? https://codereview.chromium.org/2053693002/diff/120001/net/url_request/url_re... net/url_request/url_request_unittest.cc:8386: const GURL kHttpOrigin1 = http_server_->GetURL("origin1.test", "/"); Call Origin1 TargetOrigin? It's important for the tests that this is always the origin of the target. https://codereview.chromium.org/2053693002/diff/120001/net/url_request/url_re... net/url_request/url_request_unittest.cc:8387: const GURL kHttpOrigin2 = http_server_->GetURL("origin2.test", "/"); Do we really need to get these URLs from the server? it'll just give us SSL errors, which seems like a weird end for the test. Maybe enter full addresses, and call set_cancel_in_received_redirect(true) on the URLRequest? OR just set_quit_on_redirect(true). https://codereview.chromium.org/2053693002/diff/120001/net/url_request/url_re... net/url_request/url_request_unittest.cc:8441: URLRequest::DO_NOT_UPGRADE_INSECURE_REQUESTS, GURL::EmptyGURL()}, Run with a nullptr initiator? https://codereview.chromium.org/2053693002/diff/120001/net/url_request/url_re... net/url_request/url_request_unittest.cc:8451: URLRequest::UPGRADE_ALL_INSECURE_REQUESTS, kHttpsOrigin1}, Run with a nullptr initiator? https://codereview.chromium.org/2053693002/diff/120001/net/url_request/url_re... net/url_request/url_request_unittest.cc:8460: URLRequest::UPGRADE_SAME_HOST_INSECURE_REQUESTS, kHttpsOrigin1}, This is per-host and not per-origin?
https://codereview.chromium.org/2053693002/diff/120001/net/url_request/url_re... File net/url_request/url_request_http_job.cc (right): https://codereview.chromium.org/2053693002/diff/120001/net/url_request/url_re... net/url_request/url_request_http_job.cc:1130: RedirectInfo URLRequestHttpJob::ComputeRedirectInfo(const GURL& location, Why is this logic in URLRequestHttpJob instead of URLRequestJob with the referrer policy stuff?
The CQ bit was checked by mkwst@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Your CL was about to rely on recently removed CQ feature(s): * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without "master." prefix is no longer supported: tryserver.chromium.linux For more details, see http://crbug.com/617627.
The CQ bit was checked by mkwst@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Your CL was about to rely on recently removed CQ feature(s): * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without "master." prefix is no longer supported: tryserver.chromium.linux For more details, see http://crbug.com/617627.
The CQ bit was checked by mkwst@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Your CL was about to rely on recently removed CQ feature(s): * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without "master." prefix is no longer supported: tryserver.chromium.linux For more details, see http://crbug.com/617627.
Description was changed from ========== WIP: Move 'Upgrade-Insecure-Requests' to the browser process. This is more or less a sanity check to see what mmenke@ thinks of this approach. :) It needs more tests in //content, and some general cleanup of the ductwork between Blink and //net before actual review. BUG=615885 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== WIP: Move 'Upgrade-Insecure-Requests' to the browser process. This is more or less a sanity check to see what mmenke@ thinks of this approach. :) It needs more tests in //content, and some general cleanup of the ductwork between Blink and //net before actual review. BUG=615885 ==========
The CQ bit was checked by mkwst@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Removing myself from this CL. Feel free to add me back if you start working on it again, I have like 20 dead CLs on my pending review list. :(
Description was changed from ========== WIP: Move 'Upgrade-Insecure-Requests' to the browser process. This is more or less a sanity check to see what mmenke@ thinks of this approach. :) It needs more tests in //content, and some general cleanup of the ductwork between Blink and //net before actual review. BUG=615885 ========== to ========== WIP: Move 'Upgrade-Insecure-Requests' to the browser process. This is more or less a sanity check to see what mmenke@ thinks of this approach. :) It needs more tests in //content, and some general cleanup of the ductwork between Blink and //net before actual review. BUG=615885 ==========
mmenke@chromium.org changed reviewers: - mmenke@chromium.org |