Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
714813003:
Referrer Policy: Add new policies to URLRequest. (Closed)
|
Created:
6 years, 1 month ago by Mike West Modified:
6 years, 1 month ago CC:
cbentzel+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, jam, mcasas+watch_chromium.org, posciak+watch_chromium.org, wjia+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionReferrer Policy: Add new policies to URLRequest.
This patch introduces two new referrer policy at the network level in order
to support stripping detail from the referrer header when a redirect
transitions across origins. These policies support both the "reduce referer
granularity" command-line flag, as well as the yet-to-be-implemented
"OriginWhenCrossOrigin" referrer policy specified at [1].
[1]: http://www.w3.org/TR/referrer-policy/#referrer-policy-state-origin-when-cross-origin
BUG=431711
Committed: https://crrev.com/0c5eab87cc0777e33407d2980622fa1cec4e3dfe
Cr-Commit-Position: refs/heads/master@{#305211}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Tests. #
Total comments: 6
Patch Set 3 : Feedback. #
Total comments: 1
Patch Set 4 : Helper. #
Total comments: 25
Patch Set 5 : Rework. #Patch Set 6 : I hate compilers. #
Total comments: 18
Patch Set 7 : Feedback #Patch Set 8 : Android. #Patch Set 9 : Tests. #
Messages
Total messages: 38 (7 generated)
mkwst@chromium.org changed reviewers: + jochen@chromium.org
Does this approach make sense? It's not clear to me how much the network stack should be responsible for; we could probably also move this logic out to ChromeNetworkDelegate::onBeforeRedirect, for instance (though we'd have to modify the interfaces to allow exporting a RedirectInfo object, and make it nonconst, and etc.) Regardless, this is more or less exactly what we'll need in order to support the 'OriginWhenCrossOrigin' policy, so we'll probably want something here whether or not we keep this flag around. If you think it's sane, I'll work on some tests for the net stack, and poke at a net/OWNER.
mkwst@chromium.org changed reviewers: + sleevi@google.com
Adding sleevi@ as a friendly net/ OWNER, as there's no way I'm going to have time to write a test today. Ryan, is this sane? If so, I'll write a test or three.
mkwst@chromium.org changed reviewers: + mmenke@chromium.org, rsleevi@chromium.org - sleevi@google.com
Adding the right Sleevi (which is what I assume the "r" in "rsleevi" stands for), and mmenke@, as Ryan is off sipping mai tais on a beach.
plz also update URLRequest::StartJob to verify that the referrer with the given policy matches the request.
and can we have tests plz? c/b/referrer_policy_browsertest.cc
What are the plans for actually using this? The bug doesn't make it clear. If it involves adding some new UI, have you talked to the UI guys? Also, is this only for redirects (i.e. not planning on clearing the referrer for cross-site links, just cross-site redirects) or is the other case being handled elsewhere?
+1 to tests. Should have net unit tests for this (I defer to jochen on browser tests. https://codereview.chromium.org/714813003/diff/1/content/browser/loader/resou... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/714813003/diff/1/content/browser/loader/resou... content/browser/loader/resource_dispatcher_host_impl.cc:256: CLEAR_REFERRER_ON_TRANSITION_FROM_SECURE_TO_INSECURE; Suggest just writing this out as an if statement. It uses about as many lines, and is more readable. https://codereview.chromium.org/714813003/diff/1/net/url_request/url_request_... File net/url_request/url_request_job.cc (right): https://codereview.chromium.org/714813003/diff/1/net/url_request/url_request_... net/url_request/url_request_job.cc:851: case URLRequest::CLEAR_REFERRER_ON_TRANSITION_FROM_SECURE_TO_INSECURE: optional: Suggest using braces on all blocks, for consistency. https://codereview.chromium.org/714813003/diff/1/net/url_request/url_request_... net/url_request/url_request_job.cc:854: redirect_info.new_referrer.clear(); nit: Use brace when conditional of an if takes up more than one line. https://codereview.chromium.org/714813003/diff/1/net/url_request/url_request_... net/url_request/url_request_job.cc:858: GURL original_referrer(request_->referrer()); No need for a copy here - can just use const GURL&. https://codereview.chromium.org/714813003/diff/1/net/url_request/url_request_... net/url_request/url_request_job.cc:871: break; fix indent
On 2014/11/12 15:03:29, mmenke wrote: > What are the plans for actually using this? The bug doesn't make it clear. If > it involves adding some new UI, have you talked to the UI guys? The "origin-when-cross-origin" policy will require something like this https://w3c.github.io/webappsec/specs/referrer-policy/#referrer-policy-state-.... I guess I can actually just implement that while I'm doing this, now that I think about it. :) > Also, is this only for redirects (i.e. not planning on clearing the referrer for > cross-site links, just cross-site redirects) or is the other case being handled > elsewhere? This bit is handled in Blink already; the network stack only needs to know about the policy for those redirections it doesn't push all the way back down the stack for policy checks.
On 2014/11/12 15:11:13, mmenke wrote: > +1 to tests. Should have net unit tests for this (I defer to jochen on browser > tests. Adding browser tests is somewhat straightforward, but there are unfortunately no existing unit tests in net for this functionality. It's not entirely clear to me how I'd check the behavior, really. mmenke@, can you point me at any tests that look at the referer bit of a URLRequest at all? And if they happen to do so after a redirect, even better! Thanks!
On 2014/11/13 18:24:26, Mike West wrote: > On 2014/11/12 15:11:13, mmenke wrote: > > +1 to tests. Should have net unit tests for this (I defer to jochen on > browser > > tests. > > Adding browser tests is somewhat straightforward, but there are unfortunately no > existing unit tests in net for this functionality. > > It's not entirely clear to me how I'd check the behavior, really. mmenke@, can > you point me at any tests that look at the referer bit of a URLRequest at all? > And if they happen to do so after a redirect, even better! > > Thanks! You're right - we don't seem to have any net/ tests for this logic. We do have some referer tests in url_request_unittests - see URLRequestTestHTTP.EmptyReferrerAfterValidReferrer for how to check the referer sent to the server. Since you only want to check the referer after redirects, you'll need to use redirects, too. To make the test server send a redirect, use the following path with the test_server, where dest_url is a GURL (The echoheader URL used in the test I mention above): "client-redirect?" + net::EscapeQueryParamValue(dest_url, false). Feel free to ask me more questions about how to set this up.
there's URLRequestTest.InvalidReferrerTest and URLRequestTestHTTP.HTTPSToHTTPRedirectNoRefererTest
Oops, I missed that one! That should be server-redirect, not client-redirect (That, the spelling of "referrer" in the test, and the lack of the use of the echoheader URL are why I missed that test) On Thu, Nov 13, 2014 at 1:40 PM, <jochen@chromium.org> wrote: > there's URLRequestTest.InvalidReferrerTest and > URLRequestTestHTTP.HTTPSToHTTPRedirectNoRefererTest > > https://codereview.chromium.org/714813003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Looks like Matt already pointed you to the tests. Just a chime-in on behaviour change. https://codereview.chromium.org/714813003/diff/1/net/url_request/url_request_... File net/url_request/url_request_job.cc (right): https://codereview.chromium.org/714813003/diff/1/net/url_request/url_request_... net/url_request/url_request_job.cc:855: break; BUG?: In the old code, this was unified as a single check. If it failed, it would fall through to line 856 - that is redirect_info.new_referrer = request_->referrer(); This new code no longer does that.
https://codereview.chromium.org/714813003/diff/1/net/url_request/url_request_... File net/url_request/url_request_job.cc (right): https://codereview.chromium.org/714813003/diff/1/net/url_request/url_request_... net/url_request/url_request_job.cc:855: break; On 2014/11/13 20:00:33, Ryan Sleevi (OOO until 11-18) wrote: > BUG?: In the old code, this was unified as a single check. If it failed, it > would fall through to line 856 - that is > > redirect_info.new_referrer = request_->referrer(); > > This new code no longer does that. The fact that no test caught that is very concerning. https://codereview.chromium.org/714813003/diff/1/net/url_request/url_request_... net/url_request/url_request_job.cc:855: break; On 2014/11/13 20:00:33, Ryan Sleevi (OOO until 11-18) wrote: > BUG?: In the old code, this was unified as a single check. If it failed, it > would fall through to line 856 - that is > > redirect_info.new_referrer = request_->referrer(); > > This new code no longer does that. The fact that no test caught that is rather concerning.
Mind taking another look? Now there are tests! https://codereview.chromium.org/714813003/diff/20001/content/browser/loader/r... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/714813003/diff/20001/content/browser/loader/r... content/browser/loader/resource_dispatcher_host_impl.cc:258: case blink::WebReferrerPolicyNoReferrerWhenDowngrade: Bah. TODO: I need `default:` here so I can add OriginWhenCrossOrigin as a referrer policy on the Blink side. https://codereview.chromium.org/714813003/diff/20001/net/url_request/url_requ... File net/url_request/url_request.h (right): https://codereview.chromium.org/714813003/diff/20001/net/url_request/url_requ... net/url_request/url_request.h:107: // rather than a full URL). Bah. TODO2: Add a description of ORIGIN_ONLY_ON_TRANSISITION_CROSS_ORIGIN. https://codereview.chromium.org/714813003/diff/20001/net/url_request/url_requ... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/714813003/diff/20001/net/url_request/url_requ... net/url_request/url_request_unittest.cc:2864: https_test_server.GetURL(std::string()).spec()); This bit is fairly hideous. Since I don't know the HTTP or HTTPS server's URL until it's started, I need to somehow explain to the method that it should use one or the other as the base, and then do an inline replacement. Better ideas welcome.
https://codereview.chromium.org/714813003/diff/20001/net/url_request/url_requ... File net/url_request/url_request.cc (right): https://codereview.chromium.org/714813003/diff/20001/net/url_request/url_requ... net/url_request/url_request.cc:659: REDUCE_REFERRER_GRANULARITY_ON_TRANSITION_CROSS_ORIGIN) && you could also check that for the "REDUCE_.*" case the cross origin bit is handled correctly. It could always happen that somebody creates an URL request where the initial referrer is already not matching the policy.
https://codereview.chromium.org/714813003/diff/20001/net/url_request/url_requ... File net/url_request/url_request.cc (right): https://codereview.chromium.org/714813003/diff/20001/net/url_request/url_requ... net/url_request/url_request.cc:659: REDUCE_REFERRER_GRANULARITY_ON_TRANSITION_CROSS_ORIGIN) && On 2014/11/19 10:03:48, jochen (slow) wrote: > you could also check that for the "REDUCE_.*" case the cross origin bit is > handled correctly. It could always happen that somebody creates an URL request > where the initial referrer is already not matching the policy. This got a bit complex. Still make sense? https://codereview.chromium.org/714813003/diff/20001/net/url_request/url_requ... File net/url_request/url_request.h (right): https://codereview.chromium.org/714813003/diff/20001/net/url_request/url_requ... net/url_request/url_request.h:107: // rather than a full URL). On 2014/11/19 09:57:37, Mike West wrote: > Bah. TODO2: Add a description of ORIGIN_ONLY_ON_TRANSISITION_CROSS_ORIGIN. Oh, I did this above. Nevermind, me!
https://codereview.chromium.org/714813003/diff/40001/net/url_request/url_requ... File net/url_request/url_request.cc (right): https://codereview.chromium.org/714813003/diff/40001/net/url_request/url_requ... net/url_request/url_request.cc:656: GURL referrer(referrer_); what about moving this to a helper function?
I haven't reviewed the browser tests (Which seem to be failing on the ng bots) https://codereview.chromium.org/714813003/diff/60001/content/browser/loader/r... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/714813003/diff/60001/content/browser/loader/r... content/browser/loader/resource_dispatcher_host_impl.cc:247: case blink::WebReferrerPolicyOrigin: Unrelated to this CL, but do we do validation somewhere that the referrer we get from webkit is correct, in the latter two cases? https://codereview.chromium.org/714813003/diff/60001/content/browser/loader/r... content/browser/loader/resource_dispatcher_host_impl.cc:259: default: default: not matching WebReferrerPolicyDefault: seems weird. https://codereview.chromium.org/714813003/diff/60001/net/url_request/url_requ... File net/url_request/url_request.cc (right): https://codereview.chromium.org/714813003/diff/60001/net/url_request/url_requ... net/url_request/url_request.cc:519: bool URLRequest::IsReferrerInvalid() const { Suggest moving this out of URLRequest and into an anonymous namespace, making it take 3 arguments. (referrer, url, and the policy), to make it clearer what it's doing, and avoid adding more stuff to an 850 line header file. https://codereview.chromium.org/714813003/diff/60001/net/url_request/url_requ... net/url_request/url_request.cc:519: bool URLRequest::IsReferrerInvalid() const { The new code here is not currently being exercised in tests. https://codereview.chromium.org/714813003/diff/60001/net/url_request/url_requ... net/url_request/url_request.cc:519: bool URLRequest::IsReferrerInvalid() const { As-is, this method is very hard to read. My suggestion in URLRequestJob would work around that, and result in more robust code. I've also included some suggestions here, but think the other approach is much cleaner. https://codereview.chromium.org/714813003/diff/60001/net/url_request/url_requ... net/url_request/url_request.cc:525: REDUCE_REFERRER_GRANULARITY_ON_TRANSITION_CROSS_ORIGIN; For security reasons, I suggest a whitelist rather than a blacklist, in case a new scheme is added and someone forgets to add something here (For example, if only the Job logic is update). Or alternatively, a switch statement with 4 cases - then adding a new option without updating the list would result in a warning that would lead to a compile failure. https://codereview.chromium.org/714813003/diff/60001/net/url_request/url_requ... net/url_request/url_request.cc:525: REDUCE_REFERRER_GRANULARITY_ON_TRANSITION_CROSS_ORIGIN; Rather than store a bool, why not just return false early. https://codereview.chromium.org/714813003/diff/60001/net/url_request/url_requ... net/url_request/url_request.cc:527: referrer.SchemeIsSecure() && !url().SchemeIsSecure(); Once you have the above code doing an early return, you can get rid of this bool, too, and just return true instantly if it's true. https://codereview.chromium.org/714813003/diff/60001/net/url_request/url_requ... net/url_request/url_request.cc:529: bool cross_origin_referrer = referrer.GetOrigin() != url().GetOrigin(); Suggest just inlining these bools in the if statement - which, with the above suggestions should be simpler. https://codereview.chromium.org/714813003/diff/60001/net/url_request/url_requ... File net/url_request/url_request.h (right): https://codereview.chromium.org/714813003/diff/60001/net/url_request/url_requ... net/url_request/url_request.h:107: // rather than a full URL). I don't think your last paragraph makes it sufficiently clear that it's applied to HTTP->HTTP and HTTP->HTTPS redirects, in addition to HTTPS->HTTPS ones. https://codereview.chromium.org/714813003/diff/60001/net/url_request/url_requ... File net/url_request/url_request_job.cc (right): https://codereview.chromium.org/714813003/diff/60001/net/url_request/url_requ... net/url_request/url_request_job.cc:899: break; Suggestion: This is basically a subset of the logic in URLRequestJob::ComputeRedirectInfo, just in another form. I suggest we make a new method with the logic here, and have them both share it it. GURL ComputeReferrerForRedirect(...); IsReferrerInvalid() then becomes: if (referrer_ != ComputeReferrerForRedirect(...)) And this code becomes: redirect_info_new_referrer = ComputeReferrerForRedirect(...). Then there's no need to carefully keep the logic in both methods identical, we just get it for free. I'm not quite sure where to put the new function - http_util would make sense, except that at the "http/" level, except the polices are declared in url_request/. Maybe just a static method of job? https://codereview.chromium.org/714813003/diff/60001/net/url_request/url_requ... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/714813003/diff/60001/net/url_request/url_requ... net/url_request/url_request_unittest.cc:2840: ASSERT_TRUE(test_server_.Start()); All the server starting and stopping, particularly in the Android case, where they're run on the host machine, makes me nervous. We've had timeout issues started the test server in browser tests before, and 8 or 12 started servers seems like a lot. Maybe divide up test by source/dest server, rather than by policy? One would test the cross site HTTPS->HTTPS transition for all policies, etc. https://codereview.chromium.org/714813003/diff/60001/net/url_request/url_requ... net/url_request/url_request_unittest.cc:3969: TEST_F(URLRequestTestHTTP, ReferrerPolicyClearRefererOnTransition) { It's unexciting, but for completeness, should test NEVER_CLEAR_REFERRER, too. https://codereview.chromium.org/714813003/diff/60001/net/url_request/url_requ... net/url_request/url_request_unittest.cc:4019: TEST_F(URLRequestTestHTTP, ReferrerPolicyReduceReferrerGranularity) { We actually have 6 cases for all these tests, not 4: 4 transition types: HTTP->HTTP, HTTP->HTTPS, HTTPS->HTTP, HTTPS->HTTPS. And then we have cross origin and same origin (We don't have same origin variants of the HTTPS->HTTP and HTTP->HTTPS transitions, of course) https://codereview.chromium.org/714813003/diff/60001/net/url_request/url_requ... net/url_request/url_request_unittest.cc:4045: "[HTTPS_ORIGIN]path/to/file.html", "[HTTPS_ORIGIN]path/to/file.html"); Shouldn't we test HTTPS cross origin, too, where we should clear the referrer? https://codereview.chromium.org/714813003/diff/60001/net/url_request/url_requ... net/url_request/url_request_unittest.cc:4050: "[HTTPS_ORIGIN]path/to/file.html", "[HTTPS_ORIGIN]"); I thought we should clear the referrer in this case?
Would you mind taking another look, Matt? I think this pass is a significant improvement thanks to your feedback. https://codereview.chromium.org/714813003/diff/60001/content/browser/loader/r... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/714813003/diff/60001/content/browser/loader/r... content/browser/loader/resource_dispatcher_host_impl.cc:247: case blink::WebReferrerPolicyOrigin: On 2014/11/19 16:29:03, mmenke wrote: > Unrelated to this CL, but do we do validation somewhere that the referrer we get > from webkit is correct, in the latter two cases? I don't believe we do. Jochen would know. https://codereview.chromium.org/714813003/diff/60001/content/browser/loader/r... content/browser/loader/resource_dispatcher_host_impl.cc:259: default: On 2014/11/19 16:29:03, mmenke wrote: > default: not matching WebReferrerPolicyDefault: seems weird. Fair point. https://codereview.chromium.org/714813003/diff/60001/net/url_request/url_requ... File net/url_request/url_request.cc (right): https://codereview.chromium.org/714813003/diff/60001/net/url_request/url_requ... net/url_request/url_request.cc:519: bool URLRequest::IsReferrerInvalid() const { On 2014/11/19 16:29:03, mmenke wrote: > The new code here is not currently being exercised in tests. Killed all of this in the new patchset. Thanks! https://codereview.chromium.org/714813003/diff/60001/net/url_request/url_requ... File net/url_request/url_request.h (right): https://codereview.chromium.org/714813003/diff/60001/net/url_request/url_requ... net/url_request/url_request.h:107: // rather than a full URL). On 2014/11/19 16:29:03, mmenke wrote: > I don't think your last paragraph makes it sufficiently clear that it's applied > to HTTP->HTTP and HTTP->HTTPS redirects, in addition to HTTPS->HTTPS ones. Attempted to clarify. WDYT? https://codereview.chromium.org/714813003/diff/60001/net/url_request/url_requ... File net/url_request/url_request_job.cc (right): https://codereview.chromium.org/714813003/diff/60001/net/url_request/url_requ... net/url_request/url_request_job.cc:899: break; On 2014/11/19 16:29:03, mmenke wrote: > I'm not quite sure where to put the new function - http_util would make sense, > except that at the "http/" level, except the polices are declared in > url_request/. Maybe just a static method of job? That makes a ton of sense. I've done it as a static method of URLRequestJob, but I agree that that's probably not the best place for it. If you come up with other suggestions, I'm happy to move it around. https://codereview.chromium.org/714813003/diff/60001/net/url_request/url_requ... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/714813003/diff/60001/net/url_request/url_requ... net/url_request/url_request_unittest.cc:2840: ASSERT_TRUE(test_server_.Start()); On 2014/11/19 16:29:03, mmenke wrote: > Maybe divide up test by source/dest server, rather than by policy? One would > test the cross site HTTPS->HTTPS transition for all policies, etc. That seems reasonable. It still requires us to spin up 10 servers for the 6 distinct possibilities. I'm not sure whether reducing that is a good idea (as, at the limit, it would mean dumping all the referrer policy tests into one test, spinning up the four server types (http A, http B, https A, and https B), and running all the tests). If you'd like me to go that route, I can, but I won't like it. :( https://codereview.chromium.org/714813003/diff/60001/net/url_request/url_requ... net/url_request/url_request_unittest.cc:4045: "[HTTPS_ORIGIN]path/to/file.html", "[HTTPS_ORIGIN]path/to/file.html"); On 2014/11/19 16:29:03, mmenke wrote: > Shouldn't we test HTTPS cross origin, too, where we should clear the referrer? Added cross-origin HTTPS tests. https://codereview.chromium.org/714813003/diff/60001/net/url_request/url_requ... net/url_request/url_request_unittest.cc:4050: "[HTTPS_ORIGIN]path/to/file.html", "[HTTPS_ORIGIN]"); On 2014/11/19 16:29:04, mmenke wrote: > I thought we should clear the referrer in this case? No, we clear it for "REDUCED_WHATEVER_...", but "ORIGIN_ONLY" always sends the origin. The use-case is a service like Facebook, which wishes to have full internal referrer information for debugging and etc, and wants (even insecure) external folks to know that their traffic is totally coming from Facebook, but doesn't want to leak URL details cross-origin.
chrome/ and content/ lgtm
This looks really good, just some minor suggestions. https://codereview.chromium.org/714813003/diff/60001/net/url_request/url_requ... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/714813003/diff/60001/net/url_request/url_requ... net/url_request/url_request_unittest.cc:2840: ASSERT_TRUE(test_server_.Start()); On 2014/11/20 10:45:30, Mike West wrote: > On 2014/11/19 16:29:03, mmenke wrote: > > Maybe divide up test by source/dest server, rather than by policy? One would > > test the cross site HTTPS->HTTPS transition for all policies, etc. > > That seems reasonable. It still requires us to spin up 10 servers for the 6 > distinct possibilities. I'm not sure whether reducing that is a good idea (as, > at the limit, it would mean dumping all the referrer policy tests into one test, > spinning up the four server types (http A, http B, https A, and https B), and > running all the tests). > > If you'd like me to go that route, I can, but I won't like it. :( Starting two servers per test is much less likely to run into server startup performance issues than 8 per test. Also didn't like relying on servers being restartable. https://codereview.chromium.org/714813003/diff/100001/net/url_request/url_req... File net/url_request/url_request.cc (right): https://codereview.chromium.org/714813003/diff/100001/net/url_request/url_req... net/url_request/url_request.cc:654: URLRequestJob::ComputeReferrerForRedirect(*this, url())) { optional nit: To make it clearer the second line isn't ||ed or &&ed with the first, suggest indenting it 4 more spaces. https://codereview.chromium.org/714813003/diff/100001/net/url_request/url_req... File net/url_request/url_request_job.cc (right): https://codereview.chromium.org/714813003/diff/100001/net/url_request/url_req... net/url_request/url_request_job.cc:831: // Static. nit: Think "// static" is more common (No period / not capitalized) https://codereview.chromium.org/714813003/diff/100001/net/url_request/url_req... net/url_request/url_request_job.cc:834: const GURL& redirect_destination) { Definition order should match declaration order. https://codereview.chromium.org/714813003/diff/100001/net/url_request/url_req... net/url_request/url_request_job.cc:852: return original_referrer.GetOrigin(); optional: Think using braces in else/ifs is a little more common in net/ https://codereview.chromium.org/714813003/diff/100001/net/url_request/url_req... net/url_request/url_request_job.cc:900: // Alter the referrer if we're redirecting out of https or cross-origin. nit: --we're https://codereview.chromium.org/714813003/diff/100001/net/url_request/url_req... File net/url_request/url_request_job.h (right): https://codereview.chromium.org/714813003/diff/100001/net/url_request/url_req... net/url_request/url_request_job.h:232: static GURL ComputeReferrerForRedirect(const URLRequest& request, Suggest making this take the policy and the original URL instead - think it's a little cleaner than taking something with as much random stuff as the request. https://codereview.chromium.org/714813003/diff/100001/net/url_request/url_req... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/714813003/diff/100001/net/url_request/url_req... net/url_request/url_request_unittest.cc:6740: : origin_server_->GetURL(std::string()); Maybe use GetURL("/echoheader?Referrer"), and then check that d->data_received() is also equal to expected.spec(), in addition to checking req->referrer()? https://codereview.chromium.org/714813003/diff/100001/net/url_request/url_req... net/url_request/url_request_unittest.cc:6754: EXPECT_EQ(destination_url, req->url()); Maybe add "EXPECT_EQ(200, req->response_headers()->response_code());", just out of paranoia? https://codereview.chromium.org/714813003/diff/100001/net/url_request/url_req... net/url_request/url_request_unittest.cc:6759: protected: Style guide requires all class variables be private, so just makes these private and add accessors.
Thanks for the feedback. I'll clean these up in the morning. Given the timezone difference, would you be comfortable stamping the CL now? Happy to go another round if you'd prefer.
On 2014/11/20 16:32:55, Mike West wrote: > Thanks for the feedback. I'll clean these up in the morning. Given the timezone > difference, would you be comfortable stamping the CL now? Happy to go another > round if you'd prefer. Sure, LGTM! I had assumed you were in MTV, so I could give quick double-check to your changes right after you uploaded them, but I'm fine signing off now.
The CQ bit was checked by mkwst@chromium.org
Thanks Matt! -mike https://codereview.chromium.org/714813003/diff/100001/net/url_request/url_req... File net/url_request/url_request.cc (right): https://codereview.chromium.org/714813003/diff/100001/net/url_request/url_req... net/url_request/url_request.cc:654: URLRequestJob::ComputeReferrerForRedirect(*this, url())) { On 2014/11/20 16:23:45, mmenke wrote: > optional nit: To make it clearer the second line isn't ||ed or &&ed with the > first, suggest indenting it 4 more spaces. Done. https://codereview.chromium.org/714813003/diff/100001/net/url_request/url_req... File net/url_request/url_request_job.cc (right): https://codereview.chromium.org/714813003/diff/100001/net/url_request/url_req... net/url_request/url_request_job.cc:831: // Static. On 2014/11/20 16:23:45, mmenke wrote: > nit: Think "// static" is more common (No period / not capitalized) Done. https://codereview.chromium.org/714813003/diff/100001/net/url_request/url_req... net/url_request/url_request_job.cc:834: const GURL& redirect_destination) { On 2014/11/20 16:23:45, mmenke wrote: > Definition order should match declaration order. Done. https://codereview.chromium.org/714813003/diff/100001/net/url_request/url_req... net/url_request/url_request_job.cc:852: return original_referrer.GetOrigin(); On 2014/11/20 16:23:45, mmenke wrote: > optional: Think using braces in else/ifs is a little more common in net/ Done. https://codereview.chromium.org/714813003/diff/100001/net/url_request/url_req... net/url_request/url_request_job.cc:900: // Alter the referrer if we're redirecting out of https or cross-origin. On 2014/11/20 16:23:45, mmenke wrote: > nit: --we're Done. https://codereview.chromium.org/714813003/diff/100001/net/url_request/url_req... File net/url_request/url_request_job.h (right): https://codereview.chromium.org/714813003/diff/100001/net/url_request/url_req... net/url_request/url_request_job.h:232: static GURL ComputeReferrerForRedirect(const URLRequest& request, On 2014/11/20 16:23:45, mmenke wrote: > Suggest making this take the policy and the original URL instead - think it's a > little cleaner than taking something with as much random stuff as the request. Done. https://codereview.chromium.org/714813003/diff/100001/net/url_request/url_req... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/714813003/diff/100001/net/url_request/url_req... net/url_request/url_request_unittest.cc:6740: : origin_server_->GetURL(std::string()); On 2014/11/20 16:23:45, mmenke wrote: > Maybe use GetURL("/echoheader?Referrer"), and then check that > d->data_received() is also equal to expected.spec(), in addition to checking > req->referrer()? Good idea! Done. https://codereview.chromium.org/714813003/diff/100001/net/url_request/url_req... net/url_request/url_request_unittest.cc:6754: EXPECT_EQ(destination_url, req->url()); On 2014/11/20 16:23:45, mmenke wrote: > Maybe add "EXPECT_EQ(200, req->response_headers()->response_code());", just out > of paranoia? The paranoia was justified! HTTPS SpawnedTestServer doesn't respond on anything other than 127.0.0.1. :( Revamped tests to avoid `localhost`. https://codereview.chromium.org/714813003/diff/100001/net/url_request/url_req... net/url_request/url_request_unittest.cc:6759: protected: On 2014/11/20 16:23:45, mmenke wrote: > Style guide requires all class variables be private, so just makes these private > and add accessors. Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/714813003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
The CQ bit was checked by mkwst@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/714813003/120001
The CQ bit was checked by mkwst@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/714813003/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/0c5eab87cc0777e33407d2980622fa1cec4e3dfe Cr-Commit-Position: refs/heads/master@{#305211} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
