Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1017583002:
Set Origin header to "null" for cross origin redirects. (Closed)
DescriptionSet Origin header to "null" for cross origin redirects.
In order to avoid invaliding CSRF protections on the server, it is
necessary to set the Origin header to "null" on cross origin redirect.
This avoids problems where a malicious server redirects a POST request
to a sender to cause a confused deputy problem where the server believes
the request originated with the Origin header origin.
This adds a check to url_request.cc where if the redirect is cross
origin and an Origin header is present, this sets the Origin header to
"null." If there is not Origin header, it leaves it as-is.
R=rsleevi@chromium.org
BUG=465517
Committed: https://crrev.com/5fe460ffa7a39f010134cf7c5ca5311f274103e3
Cr-Commit-Position: refs/heads/master@{#322685}
Patch Set 1 #Patch Set 2 : Added Origin header logic to web_url_loader_impl.cc #Patch Set 3 : Rebase on ToT #
Total comments: 10
Patch Set 4 : Addressed nits #
Total comments: 15
Patch Set 5 : Rebase on ToT #Patch Set 6 : Address more of David's comments #
Total comments: 6
Patch Set 7 : Address nits from David #Messages
Total messages: 26 (4 generated)
jww@chromium.org changed reviewers: + rsleevi@chromium.org
Ryan, can you take a look at this for me? Thanks!
I won't be able to review this for a few days. That said, I'm probably the best reviewer for it. Normal layering question: Does this belong in Blink (which has historically been the arbiter of Origin decisions)?
rsleevi@chromium.org changed reviewers: + davidben@chromium.org
Adding David because he's thought about the layering In particular, should this be instead dispatched through the URLRequest::Delegate OnReceivedRedirect, which surfaces through the content ResourceDispatcher to blink? And perhaps defer the redirect to alter things? I'm not sure, but //net, we've so far tried to keep out logic related to Origin, CORS, etc, and left those for the blink Resource Loader.
On 2015/03/17 01:29:26, Ryan Sleevi wrote: > Adding David because he's thought about the layering > > In particular, should this be instead dispatched through the > URLRequest::Delegate OnReceivedRedirect, which surfaces through the content > ResourceDispatcher to blink? And perhaps defer the redirect to alter things? > > I'm not sure, but //net, we've so far tried to keep out logic related to Origin, > CORS, etc, and left those for the blink Resource Loader. Agreed that it's a bit unclear, but the reason I went with this approach was that StripPostSpecificHeaders() already exists in url_request.cc and does related logic. To me, this seems like a logical tack onto that. https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur... already
CC: mmenke (Could I have access to the bug?) Hrm. This is kind of awkward because the redirect deltas get applied in two different places. Both Blink and the browser process need a copy of the post-redirect request. So, if nothing else, we need this logic in Blink as well. That the POST header stripping isn't mirrored in web_url_loader_impl.cc is probably a bug. (This is relevant, e.g., for reloads.) https://code.google.com/p/chromium/codesearch#chromium/src/content/child/npap... https://code.google.com/p/chromium/codesearch#chromium/src/content/child/web_... The other constraint is that if Blink is on the path for processing redirects for navigations, it will cause problems for PlzNavigate. So putting it deep in Blink and letting FollowRedirect change the request would mean we'd have to have a duplicate copy of that logic for navigations in the PlzNavigate end (which isn't necessarily wrong but kind of awkward). I was hoping net::RedirectInfo could express the delta of the redirect (before we also had duplicate copies of ComputeMethodForRedirect which got really badly out of sync), but it is somewhat screwy for higher-level layers to make modifications on redirects now. I'm okay with putting it in //net for now. We do already handle the Origin header when redirecting from POST. But it'd need companion logic in web_url_loader_impl.cc. Perhaps ResourceMsg_ReceivedRedirect should just staple the entire request headers and be done with it... If we want it out of //net, //content/browser's ResourceLoader is probably the best bet? It's already web-platform-specific, and it'd avoid the PlzNavigate issue. Then there's the question of how //net consumers modify a redirect. The old first-party cookie logic was external and just called set_first_party_cookie_url() or whatnot on redirect. I wasn't a huge fan of that because the redirect hadn't been applied yet, so querying the URLRequest would give you half the old request and half the new (which is why the pending leg's changes are held in a net::RedirectInfo now). Maybe OnReceivedRedirect should let you modify the net::RedirectInfo (which then also contains an HttpRequestHeaders), either directly or via some methods on net::URLRequest.
On 2015/03/17 16:41:45, David Benjamin wrote: > CC: mmenke > > (Could I have access to the bug?) > > Hrm. This is kind of awkward because the redirect deltas get applied in two > different places. Both Blink and the browser process need a copy of the > post-redirect request. So, if nothing else, we need this logic in Blink as well. > That the POST header stripping isn't mirrored in web_url_loader_impl.cc is > probably a bug. (This is relevant, e.g., for reloads.) > > https://code.google.com/p/chromium/codesearch#chromium/src/content/child/npap... > https://code.google.com/p/chromium/codesearch#chromium/src/content/child/web_... > > The other constraint is that if Blink is on the path for processing redirects > for navigations, it will cause problems for PlzNavigate. So putting it deep in > Blink and letting FollowRedirect change the request would mean we'd have to have > a duplicate copy of that logic for navigations in the PlzNavigate end (which > isn't necessarily wrong but kind of awkward). > > I was hoping net::RedirectInfo could express the delta of the redirect (before > we also had duplicate copies of ComputeMethodForRedirect which got really badly > out of sync), but it is somewhat screwy for higher-level layers to make > modifications on redirects now. > > I'm okay with putting it in //net for now. We do already handle the Origin > header when redirecting from POST. But it'd need companion logic in > web_url_loader_impl.cc. Perhaps ResourceMsg_ReceivedRedirect should just staple > the entire request headers and be done with it... > > If we want it out of //net, //content/browser's ResourceLoader is probably the > best bet? It's already web-platform-specific, and it'd avoid the PlzNavigate > issue. Then there's the question of how //net consumers modify a redirect. The > old first-party cookie logic was external and just called > set_first_party_cookie_url() or whatnot on redirect. I wasn't a huge fan of that > because the redirect hadn't been applied yet, so querying the URLRequest would > give you half the old request and half the new (which is why the pending leg's > changes are held in a net::RedirectInfo now). Maybe OnReceivedRedirect should > let you modify the net::RedirectInfo (which then also contains an > HttpRequestHeaders), either directly or via some methods on net::URLRequest. Whoops, sorry for not adding you to the bug earlier, David. I've also added mmenke@. I'll take a look at web_url_loader_impl.cc and ResourceMsg_ReceivedRedirect and try to figure out the best way to add the logic there, too.
David, I added the header logic to web_url_loader_impl.cc as you suggested. Let me know how that looks to you.
https://codereview.chromium.org/1017583002/diff/40001/net/url_request/url_req... File net/url_request/url_request.cc (right): https://codereview.chromium.org/1017583002/diff/40001/net/url_request/url_req... net/url_request/url_request.cc:949: // https://tools.ietf.org/id/draft-abarth-origin-03.html#rfc.section.5). This Cite the RFC :P http://tools.ietf.org/html/rfc6454 https://codereview.chromium.org/1017583002/diff/40001/net/url_request/url_req... net/url_request/url_request.cc:951: // protections. If the header was not set to "null", a POST request from This doesn't align with how http://tools.ietf.org/html/rfc6454#section-7.3 describes it So why null rather than treating it as an origin list? That is, the RFC doesn't seem to support your explanation on 959-961, or at least, not why you chose one over the other. https://codereview.chromium.org/1017583002/diff/40001/net/url_request/url_req... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/1017583002/diff/40001/net/url_request/url_req... net/url_request/url_request_unittest.cc:2863: // Copy pasta? https://codereview.chromium.org/1017583002/diff/40001/net/url_request/url_req... net/url_request/url_request_unittest.cc:2882: base::RunLoop().Run(); slight readability pedantry: newlines between 2881 and 2883 (eg either side of the run loop)? and 2884 (the expect eq)
https://codereview.chromium.org/1017583002/diff/40001/net/url_request/url_req... File net/url_request/url_request.cc (right): https://codereview.chromium.org/1017583002/diff/40001/net/url_request/url_req... net/url_request/url_request.cc:949: // https://tools.ietf.org/id/draft-abarth-origin-03.html#rfc.section.5). This On 2015/03/19 03:46:34, Ryan Sleevi wrote: > Cite the RFC :P > > http://tools.ietf.org/html/rfc6454 Good catch. See my comment below. https://codereview.chromium.org/1017583002/diff/40001/net/url_request/url_req... net/url_request/url_request.cc:951: // protections. If the header was not set to "null", a POST request from On 2015/03/19 03:46:34, Ryan Sleevi wrote: > This doesn't align with how http://tools.ietf.org/html/rfc6454#section-7.3 > describes it > > So why null rather than treating it as an origin list? That is, the RFC doesn't > seem to support your explanation on 959-961, or at least, not why you chose one > over the other. Well, unfortunately, my description was based on the Origin Header draft, as you pointed out above :-( I still think this is "the right thing to do" in that it matches the behavior of Firefox and IE, and I think you could make an argument about the ambiguity of "privacy sensitive contexts" applying to this case. What's your preference/thought here? I'm updating the comment to describe this rationale, but I'm unsure of the "right" answer here. https://codereview.chromium.org/1017583002/diff/40001/net/url_request/url_req... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/1017583002/diff/40001/net/url_request/url_req... net/url_request/url_request_unittest.cc:2863: // On 2015/03/19 03:46:35, Ryan Sleevi wrote: > Copy pasta? Done. https://codereview.chromium.org/1017583002/diff/40001/net/url_request/url_req... net/url_request/url_request_unittest.cc:2882: base::RunLoop().Run(); On 2015/03/19 03:46:35, Ryan Sleevi wrote: > slight readability pedantry: newlines between 2881 and 2883 (eg either side of > the run loop)? and 2884 (the expect eq) Done.
Sorry about the delay. Oh what a rabbithole you've uncovered. :-/ https://codereview.chromium.org/1017583002/diff/40001/net/url_request/url_req... File net/url_request/url_request.cc (right): https://codereview.chromium.org/1017583002/diff/40001/net/url_request/url_req... net/url_request/url_request.cc:951: // protections. If the header was not set to "null", a POST request from On 2015/03/19 17:53:09, jww wrote: > On 2015/03/19 03:46:34, Ryan Sleevi wrote: > > This doesn't align with how http://tools.ietf.org/html/rfc6454#section-7.3 > > describes it > > > > So why null rather than treating it as an origin list? That is, the RFC > doesn't > > seem to support your explanation on 959-961, or at least, not why you chose > one > > over the other. > > Well, unfortunately, my description was based on the Origin Header draft, as you > pointed out above :-( I still think this is "the right thing to do" in that it > matches the behavior of Firefox and IE, and I think you could make an argument > about the ambiguity of "privacy sensitive contexts" applying to this case. > > What's your preference/thought here? I'm updating the comment to describe this > rationale, but I'm unsure of the "right" answer here. I believe null is correct. Looks like we ended up with more WHATWG/IETF/reality divergence. https://lists.w3.org/Archives/Public/www-archive/2012Jun/0001.html https://fetch.spec.whatwg.org/#origin-header Looks like the redirect behavior now comes from: https://fetch.spec.whatwg.org/#http-fetch (step 4, case 301-308, step 10.1) Expanding call sites further, it's... https://fetch.spec.whatwg.org/#http-network-or-cache-fetch (step 3, cites 'serialized') https://html.spec.whatwg.org/multipage/browsers.html#ascii-serialisation-of-a... (step 1) Or http://www.w3.org/TR/cors/#redirect-steps The former claims to supplant the latter, but exactly which bucket in the W3C/WHATWG mess this falls under, I have no clue. https://codereview.chromium.org/1017583002/diff/60001/content/child/web_url_l... File content/child/web_url_loader_impl.cc (right): https://codereview.chromium.org/1017583002/diff/60001/content/child/web_url_l... content/child/web_url_loader_impl.cc:573: request_.httpHeaderField(origin_header)); I think this does the opposite of what the comment says. The renderer doesn't really find out about any changes that happen during redirects. That's what I meant by we end up unfortunately duplicating a lot of stuff, and why there's a net::RedirectInfo now. This *is* where that all gets applied. In fact, this is kind of odd... I wonder if we've been failing to copy over all the extra headers (including Origin) from request leg to request leg (from Blink's perspective) all this time... To make matters worse, having URLRequest send a copy of the new extra request headers back down to renderer doesn't quite work either because if code in between staples more headers, Blink should be blind to them. And I don't even want to think about what happens if ServiceWorker mucks about with this. I *think* the thing to do for now is to remove this code (sorry!) and only put logic in URLRequest. We have a giant hairball to untangle around redirects and process boundaries and CORS... Blink apparently has its own version of this logic here. Which... actually if I'm reading it right, that does something completely different! https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Except I rather suspect this doesn't do anything. Certainly this never made its way to the network stack because Blink has no influence on redirects. I also don't think the Origin header got copied over anyway because this code never copies over random headers... ...which means that probably doing a form submission which then does a method-preserving redirect elsewhere and then pressing refresh causes all kinds of problems. Until all these navigation refactors go through and then they might not! (I suspect long term, we might want to move CORS out of the renderer process? If we can get rid of all the places where the renderer cares about the post-redirect request headers, that would resolve a lot of this.) https://codereview.chromium.org/1017583002/diff/60001/net/url_request/url_req... File net/url_request/url_request.cc (right): https://codereview.chromium.org/1017583002/diff/60001/net/url_request/url_req... net/url_request/url_request.cc:958: // here. This should cite CORS or Fetch (see my comment on previous patch set). https://codereview.chromium.org/1017583002/diff/60001/net/url_request/url_req... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/1017583002/diff/60001/net/url_request/url_req... net/url_request/url_request_unittest.cc:2868: redirect_url, DEFAULT_PRIORITY, &d, NULL)); Note: you'll need to remove that trailing NULL on a rebase. https://codereview.chromium.org/1017583002/diff/60001/net/url_request/url_req... net/url_request/url_request_unittest.cc:2881: // the request headers are in order and pass the checks below. It's sort of confusing to have these tests use FTP as the cross-origin target; FTP doesn't really have headers to begin with. https://codereview.chromium.org/1017583002/diff/60001/net/url_request/url_req... net/url_request/url_request_unittest.cc:6276: // The following tests check that we handle mutating the request method for Nit: "request method" -> "request", since you've made them do more. https://codereview.chromium.org/1017583002/diff/60001/net/url_request/url_req... net/url_request/url_request_unittest.cc:6278: // See http://crbug.com/56373 and http://crbug.com/102130. Nit: I guess cite your bug too? https://codereview.chromium.org/1017583002/diff/60001/net/url_request/url_req... net/url_request/url_request_unittest.cc:6292: HTTPRedirectOriginHeaderTest(url, "POST", "GET", std::string()); Aside: does the spec say to drop the Origin header going from POST to GET? This behavior was there before though, so we can leave it alone for now, but I suspect it's wrong.
On 2015/03/24 23:47:39, David Benjamin wrote: > I believe null is correct. Looks like we ended up with more WHATWG/IETF/reality > divergence. > > https://lists.w3.org/Archives/Public/www-archive/2012Jun/0001.html > https://fetch.spec.whatwg.org/#origin-header > > Looks like the redirect behavior now comes from: > > https://fetch.spec.whatwg.org/#http-fetch (step 4, case 301-308, step 10.1) > Expanding call sites further, it's... > https://fetch.spec.whatwg.org/#http-network-or-cache-fetch (step 3, cites > 'serialized') > https://html.spec.whatwg.org/multipage/browsers.html#ascii-serialisation-of-a... > (step 1) > > Or > > http://www.w3.org/TR/cors/#redirect-steps > > The former claims to supplant the latter, but exactly which bucket in the > W3C/WHATWG mess this falls under, I have no clue. Thanks for actually digging in on this David! This is what I was looking for! WHATWG > W3C > IETF, with the caveat that Joel's now on the hook to follow spec updates for changing this behaviour, since WHATWG == LIVING. ;)
On 2015/03/24 23:52:56, Ryan Sleevi wrote: > Thanks for actually digging in on this David! This is what I was looking for! > > WHATWG > W3C > IETF, with the caveat that Joel's now on the hook to follow spec > updates for changing this behaviour, since WHATWG == LIVING. ;) Oh, and I should add that because it's part of Fetch/CORS, it really belongs to be 'properly' done in //content. If //net doesn't have a good API for that, we should figure out how to get it up, but as part of the "Web Platform" side of things, it belongs there moreso than the HTTP layer. Which is weird to say, I know.
On 2015/03/24 23:55:12, Ryan Sleevi wrote: > On 2015/03/24 23:52:56, Ryan Sleevi wrote: > > Thanks for actually digging in on this David! This is what I was looking for! > > > > WHATWG > W3C > IETF, with the caveat that Joel's now on the hook to follow > spec > > updates for changing this behaviour, since WHATWG == LIVING. ;) > > Oh, and I should add that because it's part of Fetch/CORS, it really belongs to > be 'properly' done in //content. > > If //net doesn't have a good API for that, we should figure out how to get it > up, but as part of the "Web Platform" side of things, it belongs there moreso > than the HTTP layer. Which is weird to say, I know. Agreed, though see my comment on web_url_loader_impl.cc. Our redirect processing has pretty serious problems as it is right now. :-) Between //net and //content/browser/loader, certainly we need to expose some API on URLRequest to change the redirected request. I'm not very thrilled about having that API just be "call set functions on URLRequest before following the redirect" since that'll clash with URLRequest's own processing. Though maybe it's okay initially? We probably should move the Origin header stripping on POST -> GET redirects from //net too because that's also Web Platform, not HTTP. (Or remove it? Does Firefox do this? I don't see it in the spec.) Whereas Content-Type and Content-Length being there makes sense since the HTTP layer knows what a request body is and that GETs don't have them. If it's to live in //content, probably the best place for it is ResourceLoader. Otherwise you have to deal with there being multiple possible leaf ResourceHandlers. But there's still the URLRequest API. Between //content and Blink, we have... problems. It may be best to leave that alone until we can smash apart the boundaries between //content's resource stack and Blink's and tidy up that mess. Especially since apparently Blink never carries the Origin header along at all (???) and no one's noticed.
First of all, my apologies, but I accidentally addressed David's comments simultaneously with a rebase. I, of course, normally try to avoid doing this, and I just messed up :-/ Sorry! Secondly, I agree that there's a rats nest here, and it certainly seems like there are some layering violation to clean up. However, I feel pretty strongly that all of that should be a new bug and a new CL. I am definitely not prepared to address all of those issues right now, and I do feel we need to fix this bug (and I don't believe we're -adding- any layering violations; it all appears to be already existing legacy issues). How do you guys feel about this? https://codereview.chromium.org/1017583002/diff/40001/net/url_request/url_req... File net/url_request/url_request.cc (right): https://codereview.chromium.org/1017583002/diff/40001/net/url_request/url_req... net/url_request/url_request.cc:951: // protections. If the header was not set to "null", a POST request from On 2015/03/24 23:47:38, David Benjamin wrote: > On 2015/03/19 17:53:09, jww wrote: > > On 2015/03/19 03:46:34, Ryan Sleevi wrote: > > > This doesn't align with how http://tools.ietf.org/html/rfc6454#section-7.3 > > > describes it > > > > > > So why null rather than treating it as an origin list? That is, the RFC > > doesn't > > > seem to support your explanation on 959-961, or at least, not why you chose > > one > > > over the other. > > > > Well, unfortunately, my description was based on the Origin Header draft, as > you > > pointed out above :-( I still think this is "the right thing to do" in that it > > matches the behavior of Firefox and IE, and I think you could make an argument > > about the ambiguity of "privacy sensitive contexts" applying to this case. > > > > What's your preference/thought here? I'm updating the comment to describe this > > rationale, but I'm unsure of the "right" answer here. > > I believe null is correct. Looks like we ended up with more WHATWG/IETF/reality > divergence. > > https://lists.w3.org/Archives/Public/www-archive/2012Jun/0001.html > https://fetch.spec.whatwg.org/#origin-header > > Looks like the redirect behavior now comes from: > > https://fetch.spec.whatwg.org/#http-fetch (step 4, case 301-308, step 10.1) > Expanding call sites further, it's... > https://fetch.spec.whatwg.org/#http-network-or-cache-fetch (step 3, cites > 'serialized') > https://html.spec.whatwg.org/multipage/browsers.html#ascii-serialisation-of-a... > (step 1) > > Or > > http://www.w3.org/TR/cors/#redirect-steps > > The former claims to supplant the latter, but exactly which bucket in the > W3C/WHATWG mess this falls under, I have no clue. Great finds! I guess I wasn't aware of Ryan's total ordering of WHATWG > W3C > IETF, but that's good to know :-) I'll rewrite this comment to reflect and reference the Fetch spec. https://codereview.chromium.org/1017583002/diff/60001/content/child/web_url_l... File content/child/web_url_loader_impl.cc (right): https://codereview.chromium.org/1017583002/diff/60001/content/child/web_url_l... content/child/web_url_loader_impl.cc:573: request_.httpHeaderField(origin_header)); On 2015/03/24 23:47:38, David Benjamin wrote: > I think this does the opposite of what the comment says. The renderer doesn't > really find out about any changes that happen during redirects. That's what I > meant by we end up unfortunately duplicating a lot of stuff, and why there's a > net::RedirectInfo now. This *is* where that all gets applied. > > In fact, this is kind of odd... I wonder if we've been failing to copy over all > the extra headers (including Origin) from request leg to request leg (from > Blink's perspective) all this time... > > To make matters worse, having URLRequest send a copy of the new extra request > headers back down to renderer doesn't quite work either because if code in > between staples more headers, Blink should be blind to them. And I don't even > want to think about what happens if ServiceWorker mucks about with this. > > I *think* the thing to do for now is to remove this code (sorry!) and only put > logic in URLRequest. We have a giant hairball to untangle around redirects and > process boundaries and CORS... Blink apparently has its own version of this > logic here. Which... actually if I'm reading it right, that does something > completely different! > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > Except I rather suspect this doesn't do anything. Certainly this never made its > way to the network stack because Blink has no influence on redirects. I also > don't think the Origin header got copied over anyway because this code never > copies over random headers... > > ...which means that probably doing a form submission which then does a > method-preserving redirect elsewhere and then pressing refresh causes all kinds > of problems. Until all these navigation refactors go through and then they might > not! (I suspect long term, we might want to move CORS out of the renderer > process? If we can get rid of all the places where the renderer cares about the > post-redirect request headers, that would resolve a lot of this.) Hm, okay, I'm happy to remove it. All of the tests that I wrote still pass, so that does make me think it's unnecessary :-) https://codereview.chromium.org/1017583002/diff/60001/net/url_request/url_req... File net/url_request/url_request.cc (right): https://codereview.chromium.org/1017583002/diff/60001/net/url_request/url_req... net/url_request/url_request.cc:958: // here. On 2015/03/24 23:47:38, David Benjamin wrote: > This should cite CORS or Fetch (see my comment on previous patch set). See my response to your other comment (tl;dr "Done") https://codereview.chromium.org/1017583002/diff/60001/net/url_request/url_req... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/1017583002/diff/60001/net/url_request/url_req... net/url_request/url_request_unittest.cc:2868: redirect_url, DEFAULT_PRIORITY, &d, NULL)); On 2015/03/24 23:47:38, David Benjamin wrote: > Note: you'll need to remove that trailing NULL on a rebase. Will do. https://codereview.chromium.org/1017583002/diff/60001/net/url_request/url_req... net/url_request/url_request_unittest.cc:2881: // the request headers are in order and pass the checks below. On 2015/03/24 23:47:38, David Benjamin wrote: > It's sort of confusing to have these tests use FTP as the cross-origin target; > FTP doesn't really have headers to begin with. Agreed. I've rejiggered all of these new tests to be HTTPS (and since test_server_ is HTTP, this has the same cross-origin effect). https://codereview.chromium.org/1017583002/diff/60001/net/url_request/url_req... net/url_request/url_request_unittest.cc:6276: // The following tests check that we handle mutating the request method for On 2015/03/24 23:47:38, David Benjamin wrote: > Nit: "request method" -> "request", since you've made them do more. Done. https://codereview.chromium.org/1017583002/diff/60001/net/url_request/url_req... net/url_request/url_request_unittest.cc:6278: // See http://crbug.com/56373 and http://crbug.com/102130. On 2015/03/24 23:47:38, David Benjamin wrote: > Nit: I guess cite your bug too? Done. https://codereview.chromium.org/1017583002/diff/60001/net/url_request/url_req... net/url_request/url_request_unittest.cc:6292: HTTPRedirectOriginHeaderTest(url, "POST", "GET", std::string()); On 2015/03/24 23:47:38, David Benjamin wrote: > Aside: does the spec say to drop the Origin header going from POST to GET? This > behavior was there before though, so we can leave it alone for now, but I > suspect it's wrong. No, RFC 6454 does not say to do so, nor can I find it in any of the other specs. We definitely were doing it already, though, and I'm also not sure what other user agents do. Might be worth circling back with Brad Hill (who filed the initial bug) to see if he knows.
On 2015/03/27 22:16:15, jww wrote: > First of all, my apologies, but I accidentally addressed David's comments > simultaneously with a rebase. I, of course, normally try to avoid doing this, > and I just messed up :-/ Sorry! > > Secondly, I agree that there's a rats nest here, and it certainly seems like > there are some layering violation to clean up. However, I feel pretty strongly > that all of that should be a new bug and a new CL. I am definitely not prepared > to address all of those issues right now, and I do feel we need to fix this bug > (and I don't believe we're -adding- any layering violations; it all appears to > be already existing legacy issues). How do you guys feel about this? This CL arguably does make the layering violations more significant than before. Before it was just the Origin header stripping thing which seems like it might not be necessary to begin with? Still, I think I agree it doesn't make sense to block fixing this bug on untangling this rat's nest. There isn't much better of a place to stuff this into right now. And even if we put it in //content/browser/loader, it's still far from the rest of the CORS code in Blink. I've filed https://crbug.com/471397 to track fixing this mess (which can go back into the bucket of things that will maybe happen eventually. Also added it to mmenke's document, so maybe we'll get somewhere. :-/) So... lgtm with comments, but sadface at redirects being ridiculous in general. :-( https://codereview.chromium.org/1017583002/diff/60001/content/child/web_url_l... File content/child/web_url_loader_impl.cc (right): https://codereview.chromium.org/1017583002/diff/60001/content/child/web_url_l... content/child/web_url_loader_impl.cc:573: request_.httpHeaderField(origin_header)); On 2015/03/27 22:16:15, jww wrote: > On 2015/03/24 23:47:38, David Benjamin wrote: > > I think this does the opposite of what the comment says. The renderer doesn't > > really find out about any changes that happen during redirects. That's what I > > meant by we end up unfortunately duplicating a lot of stuff, and why there's a > > net::RedirectInfo now. This *is* where that all gets applied. > > > > In fact, this is kind of odd... I wonder if we've been failing to copy over > all > > the extra headers (including Origin) from request leg to request leg (from > > Blink's perspective) all this time... > > > > To make matters worse, having URLRequest send a copy of the new extra request > > headers back down to renderer doesn't quite work either because if code in > > between staples more headers, Blink should be blind to them. And I don't even > > want to think about what happens if ServiceWorker mucks about with this. > > > > I *think* the thing to do for now is to remove this code (sorry!) and only put > > logic in URLRequest. We have a giant hairball to untangle around redirects and > > process boundaries and CORS... Blink apparently has its own version of this > > logic here. Which... actually if I'm reading it right, that does something > > completely different! > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > Except I rather suspect this doesn't do anything. Certainly this never made > its > > way to the network stack because Blink has no influence on redirects. I also > > don't think the Origin header got copied over anyway because this code never > > copies over random headers... > > > > ...which means that probably doing a form submission which then does a > > method-preserving redirect elsewhere and then pressing refresh causes all > kinds > > of problems. Until all these navigation refactors go through and then they > might > > not! (I suspect long term, we might want to move CORS out of the renderer > > process? If we can get rid of all the places where the renderer cares about > the > > post-redirect request headers, that would resolve a lot of this.) > > Hm, okay, I'm happy to remove it. All of the tests that I wrote still pass, so > that does make me think it's unnecessary :-) Well, the cases where Blink's version of the request headers are important are pretty rare. I'm sure we have zero tests for those. I know of two (reloads and document.referrer), but I'm sure there's more. :-) https://codereview.chromium.org/1017583002/diff/100001/content/child/web_url_... File content/child/web_url_loader_impl.cc (right): https://codereview.chromium.org/1017583002/diff/100001/content/child/web_url_... content/child/web_url_loader_impl.cc:43: #include "third_party/WebKit/public/platform/WebString.h" Nit: I think these can go away now. https://codereview.chromium.org/1017583002/diff/100001/net/url_request/url_re... File net/url_request/url_request.cc (right): https://codereview.chromium.org/1017583002/diff/100001/net/url_request/url_re... net/url_request/url_request.cc:954: // set the Origin header to an "opaque identifier," in this case "null." This Nit: 'Origin header' -> 'request's origin' 'in this case "null."' -> 'which serializes to "null."'. (I also feel like 'which serializes to "null".' is somewhat less weird since we're quoting an identifier, but I realize English disagrees with me in general here...) https://codereview.chromium.org/1017583002/diff/100001/net/url_request/url_re... net/url_request/url_request.cc:962: url::Origin().string()); Could you add a TODO to this block and the Origin header bit up in StripPostSpecificHeaders referencing https://crbug.com/471397?
On 2015/03/27 at 22:46:52, davidben wrote: > On 2015/03/27 22:16:15, jww wrote: > > First of all, my apologies, but I accidentally addressed David's comments > > simultaneously with a rebase. I, of course, normally try to avoid doing this, > > and I just messed up :-/ Sorry! > > > > Secondly, I agree that there's a rats nest here, and it certainly seems like > > there are some layering violation to clean up. However, I feel pretty strongly > > that all of that should be a new bug and a new CL. I am definitely not prepared > > to address all of those issues right now, and I do feel we need to fix this bug > > (and I don't believe we're -adding- any layering violations; it all appears to > > be already existing legacy issues). How do you guys feel about this? > > This CL arguably does make the layering violations more significant than before. Before it was just the Origin header stripping thing which seems like it might not be necessary to begin with? > > Still, I think I agree it doesn't make sense to block fixing this bug on untangling this rat's nest. There isn't much better of a place to stuff this into right now. And even if we put it in //content/browser/loader, it's still far from the rest of the CORS code in Blink. I've filed https://crbug.com/471397 to track fixing this mess (which can go back into the bucket of things that will maybe happen eventually. Also added it to mmenke's document, so maybe we'll get somewhere. :-/) Fair enough, and thanks for the very helpful review! > > > So... lgtm with comments, but sadface at redirects being ridiculous in general. :-( > > https://codereview.chromium.org/1017583002/diff/60001/content/child/web_url_l... > File content/child/web_url_loader_impl.cc (right): > > https://codereview.chromium.org/1017583002/diff/60001/content/child/web_url_l... > content/child/web_url_loader_impl.cc:573: request_.httpHeaderField(origin_header)); > On 2015/03/27 22:16:15, jww wrote: > > On 2015/03/24 23:47:38, David Benjamin wrote: > > > I think this does the opposite of what the comment says. The renderer doesn't > > > really find out about any changes that happen during redirects. That's what I > > > meant by we end up unfortunately duplicating a lot of stuff, and why there's a > > > net::RedirectInfo now. This *is* where that all gets applied. > > > > > > In fact, this is kind of odd... I wonder if we've been failing to copy over > > all > > > the extra headers (including Origin) from request leg to request leg (from > > > Blink's perspective) all this time... > > > > > > To make matters worse, having URLRequest send a copy of the new extra request > > > headers back down to renderer doesn't quite work either because if code in > > > between staples more headers, Blink should be blind to them. And I don't even > > > want to think about what happens if ServiceWorker mucks about with this. > > > > > > I *think* the thing to do for now is to remove this code (sorry!) and only put > > > logic in URLRequest. We have a giant hairball to untangle around redirects and > > > process boundaries and CORS... Blink apparently has its own version of this > > > logic here. Which... actually if I'm reading it right, that does something > > > completely different! > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > > > Except I rather suspect this doesn't do anything. Certainly this never made > > its > > > way to the network stack because Blink has no influence on redirects. I also > > > don't think the Origin header got copied over anyway because this code never > > > copies over random headers... > > > > > > ...which means that probably doing a form submission which then does a > > > method-preserving redirect elsewhere and then pressing refresh causes all > > kinds > > > of problems. Until all these navigation refactors go through and then they > > might > > > not! (I suspect long term, we might want to move CORS out of the renderer > > > process? If we can get rid of all the places where the renderer cares about > > the > > > post-redirect request headers, that would resolve a lot of this.) > > > > Hm, okay, I'm happy to remove it. All of the tests that I wrote still pass, so > > that does make me think it's unnecessary :-) > > Well, the cases where Blink's version of the request headers are important are pretty rare. I'm sure we have zero tests for those. I know of two (reloads and document.referrer), but I'm sure there's more. :-) > > https://codereview.chromium.org/1017583002/diff/100001/content/child/web_url_... > File content/child/web_url_loader_impl.cc (right): > > https://codereview.chromium.org/1017583002/diff/100001/content/child/web_url_... > content/child/web_url_loader_impl.cc:43: #include "third_party/WebKit/public/platform/WebString.h" > Nit: I think these can go away now. > > https://codereview.chromium.org/1017583002/diff/100001/net/url_request/url_re... > File net/url_request/url_request.cc (right): > > https://codereview.chromium.org/1017583002/diff/100001/net/url_request/url_re... > net/url_request/url_request.cc:954: // set the Origin header to an "opaque identifier," in this case "null." This > Nit: 'Origin header' -> 'request's origin' > 'in this case "null."' -> 'which serializes to "null."'. > > (I also feel like 'which serializes to "null".' is somewhat less weird since we're quoting an identifier, but I realize English disagrees with me in general here...) > > https://codereview.chromium.org/1017583002/diff/100001/net/url_request/url_re... > net/url_request/url_request.cc:962: url::Origin().string()); > Could you add a TODO to this block and the Origin header bit up in StripPostSpecificHeaders referencing https://crbug.com/471397?
The CQ bit was checked by jww@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from davidben@chromium.org Link to the patchset: https://codereview.chromium.org/1017583002/#ps120001 (title: "Address nits from David")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1017583002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/5fe460ffa7a39f010134cf7c5ca5311f274103e3 Cr-Commit-Position: refs/heads/master@{#322685}
Message was sent while issue was closed.
Note that this broke http/tests/security/contentSecurityPolicy/connect-src-beacon-redirect-to-blocked.html . I'm going to attempt to mark that test as needing a rebaseline.
Message was sent while issue was closed.
On 2015/03/28 at 02:47:48, pkasting wrote: > Note that this broke http/tests/security/contentSecurityPolicy/connect-src-beacon-redirect-to-blocked.html . > > I'm going to attempt to mark that test as needing a rebaseline. Thanks, Peter!
Message was sent while issue was closed.
Apparently I forgot to send out my responses to David's comments :-/ For posterity's sake, here they are. https://codereview.chromium.org/1017583002/diff/100001/content/child/web_url_... File content/child/web_url_loader_impl.cc (right): https://codereview.chromium.org/1017583002/diff/100001/content/child/web_url_... content/child/web_url_loader_impl.cc:43: #include "third_party/WebKit/public/platform/WebString.h" On 2015/03/27 at 22:46:51, David Benjamin wrote: > Nit: I think these can go away now. Indeed! Thanks! https://codereview.chromium.org/1017583002/diff/100001/net/url_request/url_re... File net/url_request/url_request.cc (right): https://codereview.chromium.org/1017583002/diff/100001/net/url_request/url_re... net/url_request/url_request.cc:954: // set the Origin header to an "opaque identifier," in this case "null." This On 2015/03/27 at 22:46:51, David Benjamin wrote: > Nit: 'Origin header' -> 'request's origin' > 'in this case "null."' -> 'which serializes to "null."'. > > (I also feel like 'which serializes to "null".' is somewhat less weird since we're quoting an identifier, but I realize English disagrees with me in general here...) Done. https://codereview.chromium.org/1017583002/diff/100001/net/url_request/url_re... net/url_request/url_request.cc:962: url::Origin().string()); On 2015/03/27 at 22:46:52, David Benjamin wrote: > Could you add a TODO to this block and the Origin header bit up in StripPostSpecificHeaders referencing https://crbug.com/471397? Done. |
