Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2258483002:
X-Chrome-Connected is stripped when it should not be in headers. (Closed)
|
Created:
4 years, 4 months ago by Ramin Halavati Modified:
4 years, 3 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionX-Chrome-Connected is stripped when it should not be in headers.
AppendMirrorRequestHeaderHelper added the X-Chrome-Connected header when it was required, but did not remove it when it was not required and already exited from previous redirection. The funcion is now renamed to FixMirrorRequestHeaderHelper and removes already existing header if it is not required.
BUG=588492
Committed: https://crrev.com/74362e9c627d1fabf537b351ffe5118874f30480
Cr-Commit-Position: refs/heads/master@{#417231}
Patch Set 1 #
Total comments: 17
Patch Set 2 : Addressed comments #Patch Set 3 : Removed extra headers #Patch Set 4 : X-Chrome-Connected header is not removed if not originated from Google. #
Total comments: 28
Patch Set 5 : Addressed comments #Patch Set 6 : Minor correction #
Total comments: 21
Patch Set 7 : Comments Addressed #
Total comments: 4
Patch Set 8 : Addressed comments #
Total comments: 10
Patch Set 9 : Addressed comments #Messages
Total messages: 47 (12 generated)
The CQ bit was checked by rhalavati@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...
Description was changed from ========== X-Chrome-Connected is stripped when it should not be in headers. AppendMirrorRequestHeaderHelper added the X-Chrome-Connected header when it was required, but did not remove it when it was not required and already exited from previous redirection. The funcion is now renamed to FixMirrorRequestHeaderHelper and removes already existing header if it is not required. BUG=588492 ========== to ========== X-Chrome-Connected is stripped when it should not be in headers. AppendMirrorRequestHeaderHelper added the X-Chrome-Connected header when it was required, but did not remove it when it was not required and already exited from previous redirection. The funcion is now renamed to FixMirrorRequestHeaderHelper and removes already existing header if it is not required. BUG=588492 ==========
rhalavati@chromium.org changed reviewers: + rogerta@chromium.org, sky@chromium.org
Hi, Roger, could you please review the following files: chrome\browser\signin\chrome_signin_helper.cc chrome\browser\signin\chrome_signin_helper.h components\signin\core\browser\signin_header_helper.cc components\signin\core\browser\signin_header_helper.h Sky@, could you please review all the files. Bests, Ramin
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Change looks good. Did you try the following scenarios: 1/ first time signin to chrome 2/ reauth in chrome 3/ the above two steps with a saml account
Hi Roger, I am not sure if I understood your suggestion correctly. I did the following test: 1. Signed in into chrome and went to a few google sites, and the header was there. 2. From inside google sites, chose URLs to outside google and the header was not there. 3. Signed into another Google account in browser (different from the one the I logged into Chrome) and still the header was existing for google sites and not for the others. Please elaborate on your suggestions if I did not do what you meant. Bests, Ramin On Wed, Aug 17, 2016 at 7:16 PM, <rogerta@chromium.org> wrote: > Change looks good. Did you try the following scenarios: > > 1/ first time signin to chrome > 2/ reauth in chrome > 3/ the above two steps with a saml account > > > https://codereview.chromium.org/2258483002/ > Ramin Halavati Software Engineer rhalavati@google.com +49 89 839300892 Google Germany GmbH Erika-Mann-Straße 33 80636 München Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Diese E-Mail ist vertraulich. Falls sie diese fälschlicherweise erhalten haben sollten, leiten Sie diese bitte nicht an jemand anderes weiter, löschen Sie alle Kopien und Anhänge davon und lassen Sie mich bitte wissen, dass die E-Mail an die falsche Person gesendet wurde. This e-mail is confidential. If you received this communication by mistake, please don't forward it to anyone else, please erase all copies and attachments, and please let me know that it has gone to the wrong person. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm Your tests are good Ramin. I was thinking of chrome signin, but this happens post sign in. Thanks.
sky@chromium.org changed reviewers: + eroman@chromium.org
I have to admit I'm not terribly familiar with URLRequestJob. +eroman . I'm specifically wondering about the best way to mock URLRequestJob. See changes to chrome_resource_dispatcher_host_delegate_browsertest.cc . https://codereview.chromium.org/2258483002/diff/1/chrome/browser/renderer_hos... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc (right): https://codereview.chromium.org/2258483002/diff/1/chrome/browser/renderer_hos... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:273: class MirrorMockURLRequestJob : public net::URLRequestJob { Can you use some of the test support classes in net for mocking rather than rolling your own?
eroman@chromium.org changed reviewers: + mmenke@chromium.org
https://codereview.chromium.org/2258483002/diff/1/chrome/browser/renderer_hos... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2258483002/diff/1/chrome/browser/renderer_hos... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:803: // In the Mirror world, Chrome should append a X-Chrome-Connected header to What is the Mirror world? Is this a public project name? https://codereview.chromium.org/2258483002/diff/1/chrome/browser/signin/chrom... File chrome/browser/signin/chrome_signin_helper.cc (right): https://codereview.chromium.org/2258483002/diff/1/chrome/browser/signin/chrom... chrome/browser/signin/chrome_signin_helper.cc:121: bool FixMirrorRequestHeaderHelper(net::URLRequest* request, style -- fix indentation https://codereview.chromium.org/2258483002/diff/1/chrome/browser/signin/chrom... chrome/browser/signin/chrome_signin_helper.cc:157: request->RemoveRequestHeaderByName(signin::kChromeConnectedHeader); This doesn't seem right. This code is called for all requests flowing through resource dispatcher host. Which means that any other consumer that tries to set their own header named X-Chrome-Connected will have it stripped at this layer. At best that is confusing, at worst that is a bug which prevents using this header name on non-google domains. https://codereview.chromium.org/2258483002/diff/1/chrome/browser/signin/chrom... File chrome/browser/signin/chrome_signin_helper.h (right): https://codereview.chromium.org/2258483002/diff/1/chrome/browser/signin/chrom... chrome/browser/signin/chrome_signin_helper.h:30: const GURL& redirect_url, indentation https://codereview.chromium.org/2258483002/diff/1/components/signin/core/brow... File components/signin/core/browser/signin_header_helper.cc (right): https://codereview.chromium.org/2258483002/diff/1/components/signin/core/brow... components/signin/core/browser/signin_header_helper.cc:111: bool is_google_url = is_enable_account_consistency && Separate issue: This doesn't seem right.... Isn't a consequence of this logic that any http:// google property will get the X-Chrome-Connected header set on it when "account consistency is enabled" ? If X-Chrome-Connected contains PII then it should not be set for non cryptographic schems.
https://codereview.chromium.org/2258483002/diff/1/chrome/browser/signin/chrom... File chrome/browser/signin/chrome_signin_helper.cc (right): https://codereview.chromium.org/2258483002/diff/1/chrome/browser/signin/chrom... chrome/browser/signin/chrome_signin_helper.cc:154: io_data->GetCookieSettings(), profile_mode_mask)) AppendMirrorRequestHeaderIfPossible seems problematic. It doesn't look like it checks for HTTPS, and relies on HSTS being enabled for Google domains...Which hopefully is true, but this code should not rely on it. That also means it exposes this information to HTTP-only extensions, even if all google domains are HSTS, which seems weird. A third party ServiceWorker can also serve requests to http://www.google.com with responses from http://www.evil.com... I believe we won't expose headers for the request in that case, either to the ServiceWorker or over the network but it still makes me very nervous.
https://codereview.chromium.org/2258483002/diff/1/chrome/browser/signin/chrom... File chrome/browser/signin/chrome_signin_helper.cc (right): https://codereview.chromium.org/2258483002/diff/1/chrome/browser/signin/chrom... chrome/browser/signin/chrome_signin_helper.cc:154: io_data->GetCookieSettings(), profile_mode_mask)) On 2016/08/23 20:34:16, mmenke wrote: > AppendMirrorRequestHeaderIfPossible seems problematic. It doesn't look like it > checks for HTTPS, and relies on HSTS being enabled for Google domains...Which > hopefully is true, but this code should not rely on it. That also means it > exposes this information to HTTP-only extensions, even if all google domains are > HSTS, which seems weird. > > A third party ServiceWorker can also serve requests to http://www.google.com > with responses from http://www.evil.com... I believe we won't expose headers > for the request in that case, either to the ServiceWorker or over the network > but it still makes me very nervous. And this also assumes all Google.* TLDs are owned by Google, which also seems like a problem. If someone registers google.mil, or just hijacks request to it, for instance, it looks like they could get this information.
Comments are addressed, please review again. https://codereview.chromium.org/2258483002/diff/1/chrome/browser/renderer_hos... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2258483002/diff/1/chrome/browser/renderer_hos... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:803: // In the Mirror world, Chrome should append a X-Chrome-Connected header to On 2016/08/23 20:02:36, eroman wrote: > What is the Mirror world? Is this a public project name? Added a comment. https://codereview.chromium.org/2258483002/diff/1/chrome/browser/renderer_hos... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc (right): https://codereview.chromium.org/2258483002/diff/1/chrome/browser/renderer_hos... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:273: class MirrorMockURLRequestJob : public net::URLRequestJob { On 2016/08/18 19:29:50, sky wrote: > Can you use some of the test support classes in net for mocking rather than > rolling your own? Done. https://codereview.chromium.org/2258483002/diff/1/chrome/browser/signin/chrom... File chrome/browser/signin/chrome_signin_helper.cc (right): https://codereview.chromium.org/2258483002/diff/1/chrome/browser/signin/chrom... chrome/browser/signin/chrome_signin_helper.cc:121: bool FixMirrorRequestHeaderHelper(net::URLRequest* request, On 2016/08/23 20:02:36, eroman wrote: > style -- fix indentation Done. https://codereview.chromium.org/2258483002/diff/1/chrome/browser/signin/chrom... chrome/browser/signin/chrome_signin_helper.cc:154: io_data->GetCookieSettings(), profile_mode_mask)) On 2016/08/23 20:34:16, mmenke wrote: > AppendMirrorRequestHeaderIfPossible seems problematic. It doesn't look like it > checks for HTTPS, and relies on HSTS being enabled for Google domains...Which > hopefully is true, but this code should not rely on it. That also means it > exposes this information to HTTP-only extensions, even if all google domains are > HSTS, which seems weird. > > A third party ServiceWorker can also serve requests to http://www.google.com > with responses from http://www.evil.com... I believe we won't expose headers > for the request in that case, either to the ServiceWorker or over the network > but it still makes me very nervous. I have added a test for content::IsOriginSecure and don't generate a header if the origin is not secure. https://codereview.chromium.org/2258483002/diff/1/chrome/browser/signin/chrom... chrome/browser/signin/chrome_signin_helper.cc:154: io_data->GetCookieSettings(), profile_mode_mask)) On 2016/08/23 20:44:52, mmenke wrote: > On 2016/08/23 20:34:16, mmenke wrote: > > AppendMirrorRequestHeaderIfPossible seems problematic. It doesn't look like > it > > checks for HTTPS, and relies on HSTS being enabled for Google domains...Which > > hopefully is true, but this code should not rely on it. That also means it > > exposes this information to HTTP-only extensions, even if all google domains > are > > HSTS, which seems weird. > > > > A third party ServiceWorker can also serve requests to http://www.google.com > > with responses from http://www.evil.com... I believe we won't expose headers > > for the request in that case, either to the ServiceWorker or over the network > > but it still makes me very nervous. > > And this also assumes all Google.* TLDs are owned by Google, which also seems > like a problem. If someone registers http://google.mil, or just hijacks request to it, > for instance, it looks like they could get this information. I think this is a bigger issue of google_util::IsGoogleDomainUrl. I think that multiple services depend on this function working properly. I think we should consider this out of scope for this CL. How would you suggest to fix this? https://codereview.chromium.org/2258483002/diff/1/chrome/browser/signin/chrom... chrome/browser/signin/chrome_signin_helper.cc:157: request->RemoveRequestHeaderByName(signin::kChromeConnectedHeader); On 2016/08/23 20:02:36, eroman wrote: > This doesn't seem right. > > This code is called for all requests flowing through resource dispatcher host. > Which means that any other consumer that tries to set their own header named > X-Chrome-Connected will have it stripped at this layer. At best that is > confusing, at worst that is a bug which prevents using this header name on > non-google domains. It's updated so that it is removed only when it is redirecting from a domain which is eligible for having this header, to one that is not eligible. https://codereview.chromium.org/2258483002/diff/1/chrome/browser/signin/chrom... File chrome/browser/signin/chrome_signin_helper.h (right): https://codereview.chromium.org/2258483002/diff/1/chrome/browser/signin/chrom... chrome/browser/signin/chrome_signin_helper.h:30: const GURL& redirect_url, On 2016/08/23 20:02:36, eroman wrote: > indentation Done. https://codereview.chromium.org/2258483002/diff/1/components/signin/core/brow... File components/signin/core/browser/signin_header_helper.cc (right): https://codereview.chromium.org/2258483002/diff/1/components/signin/core/brow... components/signin/core/browser/signin_header_helper.cc:111: bool is_google_url = is_enable_account_consistency && On 2016/08/23 20:02:36, eroman wrote: > Separate issue: This doesn't seem right.... Isn't a consequence of this logic > that any http:// google property will get the X-Chrome-Connected header set on > it when "account consistency is enabled" ? > If X-Chrome-Connected contains PII then it should not be set for non > cryptographic schems. Done.
https://codereview.chromium.org/2258483002/diff/1/chrome/browser/signin/chrom... File chrome/browser/signin/chrome_signin_helper.cc (right): https://codereview.chromium.org/2258483002/diff/1/chrome/browser/signin/chrom... chrome/browser/signin/chrome_signin_helper.cc:154: io_data->GetCookieSettings(), profile_mode_mask)) On 2016/08/26 17:04:31, RaminH wrote: > On 2016/08/23 20:44:52, mmenke wrote: > > On 2016/08/23 20:34:16, mmenke wrote: > > > AppendMirrorRequestHeaderIfPossible seems problematic. It doesn't look like > > it > > > checks for HTTPS, and relies on HSTS being enabled for Google > domains...Which > > > hopefully is true, but this code should not rely on it. That also means it > > > exposes this information to HTTP-only extensions, even if all google domains > > are > > > HSTS, which seems weird. > > > > > > A third party ServiceWorker can also serve requests to http://www.google.com > > > with responses from http://www.evil.com... I believe we won't expose > headers > > > for the request in that case, either to the ServiceWorker or over the > network > > > but it still makes me very nervous. > > > > And this also assumes all Google.* TLDs are owned by Google, which also seems > > like a problem. If someone registers http://google.mil, or just hijacks > request to it, > > for instance, it looks like they could get this information. > > I think this is a bigger issue of google_util::IsGoogleDomainUrl. I think that > multiple services depend on this function working properly. I think we should > consider this out of scope for this CL. How would you suggest to fix this? My feeling is that we should not rely on it for any security or privacy related checks. I don't know what project mirror is, but I don't think we should just be arbitrarily adding privacy-relevant headers to requests based on requested URLs. It seems like a problematic design to me. I do agree that this seems like a more general problem with IsGoogleDomainUrl itself, even with an HTTPS check added to it.
On 2016/08/26 17:42:21, mmenke (busy) wrote: > https://codereview.chromium.org/2258483002/diff/1/chrome/browser/signin/chrom... > File chrome/browser/signin/chrome_signin_helper.cc (right): > > https://codereview.chromium.org/2258483002/diff/1/chrome/browser/signin/chrom... > chrome/browser/signin/chrome_signin_helper.cc:154: io_data->GetCookieSettings(), > profile_mode_mask)) > On 2016/08/26 17:04:31, RaminH wrote: > > On 2016/08/23 20:44:52, mmenke wrote: > > > On 2016/08/23 20:34:16, mmenke wrote: > > > > AppendMirrorRequestHeaderIfPossible seems problematic. It doesn't look > like > > > it > > > > checks for HTTPS, and relies on HSTS being enabled for Google > > domains...Which > > > > hopefully is true, but this code should not rely on it. That also means > it > > > > exposes this information to HTTP-only extensions, even if all google > domains > > > are > > > > HSTS, which seems weird. > > > > > > > > A third party ServiceWorker can also serve requests to > http://www.google.com > > > > with responses from http://www.evil.com... I believe we won't expose > > headers > > > > for the request in that case, either to the ServiceWorker or over the > > network > > > > but it still makes me very nervous. > > > > > > And this also assumes all Google.* TLDs are owned by Google, which also > seems > > > like a problem. If someone registers http://google.mil, or just hijacks > > request to it, > > > for instance, it looks like they could get this information. > > > > I think this is a bigger issue of google_util::IsGoogleDomainUrl. I think that > > multiple services depend on this function working properly. I think we should > > consider this out of scope for this CL. How would you suggest to fix this? > > My feeling is that we should not rely on it for any security or privacy related > checks. I don't know what project mirror is, but I don't think we should just > be arbitrarily adding privacy-relevant headers to requests based on requested > URLs. It seems like a problematic design to me. > > I do agree that this seems like a more general problem with IsGoogleDomainUrl > itself, even with an HTTPS check added to it. Just consider: There's a .sucks TLD. A site with gripes about Google would be a legitimate use of that domain, and could even get a fully valid cert... So this just seems a very bad idea.
On 2016/08/26 20:14:28, mmenke (busy) wrote: > On 2016/08/26 17:42:21, mmenke (busy) wrote: > > > https://codereview.chromium.org/2258483002/diff/1/chrome/browser/signin/chrom... > > File chrome/browser/signin/chrome_signin_helper.cc (right): > > > > > https://codereview.chromium.org/2258483002/diff/1/chrome/browser/signin/chrom... > > chrome/browser/signin/chrome_signin_helper.cc:154: > io_data->GetCookieSettings(), > > profile_mode_mask)) > > On 2016/08/26 17:04:31, RaminH wrote: > > > On 2016/08/23 20:44:52, mmenke wrote: > > > > On 2016/08/23 20:34:16, mmenke wrote: > > > > > AppendMirrorRequestHeaderIfPossible seems problematic. It doesn't look > > like > > > > it > > > > > checks for HTTPS, and relies on HSTS being enabled for Google > > > domains...Which > > > > > hopefully is true, but this code should not rely on it. That also means > > it > > > > > exposes this information to HTTP-only extensions, even if all google > > domains > > > > are > > > > > HSTS, which seems weird. > > > > > > > > > > A third party ServiceWorker can also serve requests to > > http://www.google.com > > > > > with responses from http://www.evil.com... I believe we won't expose > > > headers > > > > > for the request in that case, either to the ServiceWorker or over the > > > network > > > > > but it still makes me very nervous. > > > > > > > > And this also assumes all Google.* TLDs are owned by Google, which also > > seems > > > > like a problem. If someone registers http://google.mil, or just hijacks > > > request to it, > > > > for instance, it looks like they could get this information. > > > > > > I think this is a bigger issue of google_util::IsGoogleDomainUrl. I think > that > > > multiple services depend on this function working properly. I think we > should > > > consider this out of scope for this CL. How would you suggest to fix this? > > > > My feeling is that we should not rely on it for any security or privacy > related > > checks. I don't know what project mirror is, but I don't think we should just > > be arbitrarily adding privacy-relevant headers to requests based on requested > > URLs. It seems like a problematic design to me. > > > > I do agree that this seems like a more general problem with IsGoogleDomainUrl > > itself, even with an HTTPS check added to it. > > Just consider: There's a .sucks TLD. A site with gripes about Google would be > a legitimate use of that domain, and could even get a fully valid cert... So > this just seems a very bad idea. Anyhow, the only two valid fixes for this seem to be a whitelist, or removing the feature (The current implementation of it, at least). I'd prefer the latter approach, but the former also works, though I'd say longer term, we should look at other options for implementing whatever this feature is.
On 2016/08/26 21:27:53, mmenke wrote: > On 2016/08/26 20:14:28, mmenke (busy) wrote: > > On 2016/08/26 17:42:21, mmenke (busy) wrote: > > > > > > https://codereview.chromium.org/2258483002/diff/1/chrome/browser/signin/chrom... > > > File chrome/browser/signin/chrome_signin_helper.cc (right): > > > > > > > > > https://codereview.chromium.org/2258483002/diff/1/chrome/browser/signin/chrom... > > > chrome/browser/signin/chrome_signin_helper.cc:154: > > io_data->GetCookieSettings(), > > > profile_mode_mask)) > > > On 2016/08/26 17:04:31, RaminH wrote: > > > > On 2016/08/23 20:44:52, mmenke wrote: > > > > > On 2016/08/23 20:34:16, mmenke wrote: > > > > > > AppendMirrorRequestHeaderIfPossible seems problematic. It doesn't > look > > > like > > > > > it > > > > > > checks for HTTPS, and relies on HSTS being enabled for Google > > > > domains...Which > > > > > > hopefully is true, but this code should not rely on it. That also > means > > > it > > > > > > exposes this information to HTTP-only extensions, even if all google > > > domains > > > > > are > > > > > > HSTS, which seems weird. > > > > > > > > > > > > A third party ServiceWorker can also serve requests to > > > http://www.google.com > > > > > > with responses from http://www.evil.com... I believe we won't expose > > > > headers > > > > > > for the request in that case, either to the ServiceWorker or over the > > > > network > > > > > > but it still makes me very nervous. > > > > > > > > > > And this also assumes all Google.* TLDs are owned by Google, which also > > > seems > > > > > like a problem. If someone registers http://google.mil, or just hijacks > > > > request to it, > > > > > for instance, it looks like they could get this information. > > > > > > > > I think this is a bigger issue of google_util::IsGoogleDomainUrl. I think > > that > > > > multiple services depend on this function working properly. I think we > > should > > > > consider this out of scope for this CL. How would you suggest to fix this? > > > > > > My feeling is that we should not rely on it for any security or privacy > > related > > > checks. I don't know what project mirror is, but I don't think we should > just > > > be arbitrarily adding privacy-relevant headers to requests based on > requested > > > URLs. It seems like a problematic design to me. > > > > > > I do agree that this seems like a more general problem with > IsGoogleDomainUrl > > > itself, even with an HTTPS check added to it. > > > > Just consider: There's a .sucks TLD. A site with gripes about Google would > be > > a legitimate use of that domain, and could even get a fully valid cert... So > > this just seems a very bad idea. > > Anyhow, the only two valid fixes for this seem to be a whitelist, or removing > the feature (The current implementation of it, at least). I'd prefer the latter > approach, but the former also works, though I'd say longer term, we should look > at other options for implementing whatever this feature is. So you suggest that we block the CL because, if anything, the CL restricts the domains that get the headers?
On 2016/08/29 07:15:37, Ramin Halavati wrote: > On 2016/08/26 21:27:53, mmenke wrote: > > On 2016/08/26 20:14:28, mmenke (busy) wrote: > > > On 2016/08/26 17:42:21, mmenke (busy) wrote: > > > > > > > > > > https://codereview.chromium.org/2258483002/diff/1/chrome/browser/signin/chrom... > > > > File chrome/browser/signin/chrome_signin_helper.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2258483002/diff/1/chrome/browser/signin/chrom... > > > > chrome/browser/signin/chrome_signin_helper.cc:154: > > > io_data->GetCookieSettings(), > > > > profile_mode_mask)) > > > > On 2016/08/26 17:04:31, RaminH wrote: > > > > > On 2016/08/23 20:44:52, mmenke wrote: > > > > > > On 2016/08/23 20:34:16, mmenke wrote: > > > > > > > AppendMirrorRequestHeaderIfPossible seems problematic. It doesn't > > look > > > > like > > > > > > it > > > > > > > checks for HTTPS, and relies on HSTS being enabled for Google > > > > > domains...Which > > > > > > > hopefully is true, but this code should not rely on it. That also > > means > > > > it > > > > > > > exposes this information to HTTP-only extensions, even if all google > > > > domains > > > > > > are > > > > > > > HSTS, which seems weird. > > > > > > > > > > > > > > A third party ServiceWorker can also serve requests to > > > > http://www.google.com > > > > > > > with responses from http://www.evil.com... I believe we won't > expose > > > > > headers > > > > > > > for the request in that case, either to the ServiceWorker or over > the > > > > > network > > > > > > > but it still makes me very nervous. > > > > > > > > > > > > And this also assumes all Google.* TLDs are owned by Google, which > also > > > > seems > > > > > > like a problem. If someone registers http://google.mil, or just > hijacks > > > > > request to it, > > > > > > for instance, it looks like they could get this information. > > > > > > > > > > I think this is a bigger issue of google_util::IsGoogleDomainUrl. I > think > > > that > > > > > multiple services depend on this function working properly. I think we > > > should > > > > > consider this out of scope for this CL. How would you suggest to fix > this? > > > > > > > > My feeling is that we should not rely on it for any security or privacy > > > related > > > > checks. I don't know what project mirror is, but I don't think we should > > just > > > > be arbitrarily adding privacy-relevant headers to requests based on > > requested > > > > URLs. It seems like a problematic design to me. > > > > > > > > I do agree that this seems like a more general problem with > > IsGoogleDomainUrl > > > > itself, even with an HTTPS check added to it. > > > > > > Just consider: There's a .sucks TLD. A site with gripes about Google would > > be > > > a legitimate use of that domain, and could even get a fully valid cert... > So > > > this just seems a very bad idea. > > > > Anyhow, the only two valid fixes for this seem to be a whitelist, or removing > > the feature (The current implementation of it, at least). I'd prefer the > latter > > approach, but the former also works, though I'd say longer term, we should > look > > at other options for implementing whatever this feature is. > > So you suggest that we block the CL because, if anything, the CL restricts the > domains that get the headers? I don't understand the comment. I'm saying this CL doesn't actually fix the issue.
On 2016/08/29 09:55:34, mmenke wrote: > On 2016/08/29 07:15:37, Ramin Halavati wrote: > > On 2016/08/26 21:27:53, mmenke wrote: > > > On 2016/08/26 20:14:28, mmenke (busy) wrote: > > > > On 2016/08/26 17:42:21, mmenke (busy) wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2258483002/diff/1/chrome/browser/signin/chrom... > > > > > File chrome/browser/signin/chrome_signin_helper.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2258483002/diff/1/chrome/browser/signin/chrom... > > > > > chrome/browser/signin/chrome_signin_helper.cc:154: > > > > io_data->GetCookieSettings(), > > > > > profile_mode_mask)) > > > > > On 2016/08/26 17:04:31, RaminH wrote: > > > > > > On 2016/08/23 20:44:52, mmenke wrote: > > > > > > > On 2016/08/23 20:34:16, mmenke wrote: > > > > > > > > AppendMirrorRequestHeaderIfPossible seems problematic. It doesn't > > > look > > > > > like > > > > > > > it > > > > > > > > checks for HTTPS, and relies on HSTS being enabled for Google > > > > > > domains...Which > > > > > > > > hopefully is true, but this code should not rely on it. That also > > > means > > > > > it > > > > > > > > exposes this information to HTTP-only extensions, even if all > google > > > > > domains > > > > > > > are > > > > > > > > HSTS, which seems weird. > > > > > > > > > > > > > > > > A third party ServiceWorker can also serve requests to > > > > > http://www.google.com > > > > > > > > with responses from http://www.evil.com... I believe we won't > > expose > > > > > > headers > > > > > > > > for the request in that case, either to the ServiceWorker or over > > the > > > > > > network > > > > > > > > but it still makes me very nervous. > > > > > > > > > > > > > > And this also assumes all Google.* TLDs are owned by Google, which > > also > > > > > seems > > > > > > > like a problem. If someone registers http://google.mil, or just > > hijacks > > > > > > request to it, > > > > > > > for instance, it looks like they could get this information. > > > > > > > > > > > > I think this is a bigger issue of google_util::IsGoogleDomainUrl. I > > think > > > > that > > > > > > multiple services depend on this function working properly. I think we > > > > should > > > > > > consider this out of scope for this CL. How would you suggest to fix > > this? > > > > > > > > > > My feeling is that we should not rely on it for any security or privacy > > > > related > > > > > checks. I don't know what project mirror is, but I don't think we > should > > > just > > > > > be arbitrarily adding privacy-relevant headers to requests based on > > > requested > > > > > URLs. It seems like a problematic design to me. > > > > > > > > > > I do agree that this seems like a more general problem with > > > IsGoogleDomainUrl > > > > > itself, even with an HTTPS check added to it. > > > > > > > > Just consider: There's a .sucks TLD. A site with gripes about Google > would > > > be > > > > a legitimate use of that domain, and could even get a fully valid cert... > > So > > > > this just seems a very bad idea. > > > > > > Anyhow, the only two valid fixes for this seem to be a whitelist, or > removing > > > the feature (The current implementation of it, at least). I'd prefer the > > latter > > > approach, but the former also works, though I'd say longer term, we should > > look > > > at other options for implementing whatever this feature is. > > > > So you suggest that we block the CL because, if anything, the CL restricts the > > domains that get the headers? > > I don't understand the comment. I'm saying this CL doesn't actually fix the > issue. I would argue that we have two problems 1) Chrome does not drop the mirror header after a redirect to a non-google domain. 2) Chrome's method to determine google vs. non-google domains is bad. Shouldn't we consider these different issues and fix them in separate CLs? This CL addresses only 1), the Mirror specific part, but not 2). [Actually, the CL also removes the mirror header from unencrypted HTTP traffic]
On 2016/08/29 11:02:08, battre (OOO until Aug 22) wrote: > On 2016/08/29 09:55:34, mmenke wrote: > > On 2016/08/29 07:15:37, Ramin Halavati wrote: > > > On 2016/08/26 21:27:53, mmenke wrote: > > > > On 2016/08/26 20:14:28, mmenke (busy) wrote: > > > > > On 2016/08/26 17:42:21, mmenke (busy) wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2258483002/diff/1/chrome/browser/signin/chrom... > > > > > > File chrome/browser/signin/chrome_signin_helper.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2258483002/diff/1/chrome/browser/signin/chrom... > > > > > > chrome/browser/signin/chrome_signin_helper.cc:154: > > > > > io_data->GetCookieSettings(), > > > > > > profile_mode_mask)) > > > > > > On 2016/08/26 17:04:31, RaminH wrote: > > > > > > > On 2016/08/23 20:44:52, mmenke wrote: > > > > > > > > On 2016/08/23 20:34:16, mmenke wrote: > > > > > > > > > AppendMirrorRequestHeaderIfPossible seems problematic. It > doesn't > > > > look > > > > > > like > > > > > > > > it > > > > > > > > > checks for HTTPS, and relies on HSTS being enabled for Google > > > > > > > domains...Which > > > > > > > > > hopefully is true, but this code should not rely on it. That > also > > > > means > > > > > > it > > > > > > > > > exposes this information to HTTP-only extensions, even if all > > google > > > > > > domains > > > > > > > > are > > > > > > > > > HSTS, which seems weird. > > > > > > > > > > > > > > > > > > A third party ServiceWorker can also serve requests to > > > > > > http://www.google.com > > > > > > > > > with responses from http://www.evil.com... I believe we won't > > > expose > > > > > > > headers > > > > > > > > > for the request in that case, either to the ServiceWorker or > over > > > the > > > > > > > network > > > > > > > > > but it still makes me very nervous. > > > > > > > > > > > > > > > > And this also assumes all Google.* TLDs are owned by Google, which > > > also > > > > > > seems > > > > > > > > like a problem. If someone registers http://google.mil, or just > > > hijacks > > > > > > > request to it, > > > > > > > > for instance, it looks like they could get this information. > > > > > > > > > > > > > > I think this is a bigger issue of google_util::IsGoogleDomainUrl. I > > > think > > > > > that > > > > > > > multiple services depend on this function working properly. I think > we > > > > > should > > > > > > > consider this out of scope for this CL. How would you suggest to fix > > > this? > > > > > > > > > > > > My feeling is that we should not rely on it for any security or > privacy > > > > > related > > > > > > checks. I don't know what project mirror is, but I don't think we > > should > > > > just > > > > > > be arbitrarily adding privacy-relevant headers to requests based on > > > > requested > > > > > > URLs. It seems like a problematic design to me. > > > > > > > > > > > > I do agree that this seems like a more general problem with > > > > IsGoogleDomainUrl > > > > > > itself, even with an HTTPS check added to it. > > > > > > > > > > Just consider: There's a .sucks TLD. A site with gripes about Google > > would > > > > be > > > > > a legitimate use of that domain, and could even get a fully valid > cert... > > > So > > > > > this just seems a very bad idea. > > > > > > > > Anyhow, the only two valid fixes for this seem to be a whitelist, or > > removing > > > > the feature (The current implementation of it, at least). I'd prefer the > > > latter > > > > approach, but the former also works, though I'd say longer term, we should > > > look > > > > at other options for implementing whatever this feature is. > > > > > > So you suggest that we block the CL because, if anything, the CL restricts > the > > > domains that get the headers? > > > > I don't understand the comment. I'm saying this CL doesn't actually fix the > > issue. > > I would argue that we have two problems > 1) Chrome does not drop the mirror header after a redirect to a non-google > domain. > 2) Chrome's method to determine google vs. non-google domains is bad. > > Shouldn't we consider these different issues and fix them in separate CLs? This > CL addresses only 1), the Mirror specific part, but not 2). [Actually, the CL > also removes the mirror header from unencrypted HTTP traffic] I think we have a third bug: 3) ServiceWorker: https://www.evil.com can make a request a subresource from https://www.google.com. This class then attaches the header. Then https://www.evil.com's ServiceWorker redirects the request to https://www.evil.com/. I said I didn't think it was an issue before, but I'm less sure now - in that case, does either the ServiceWorker or evil.com see the header? Normally, the evil.com page would have been responsible for setting the headers itself (Or through a script that runs in its context), so maybe it does.
On 2016/08/29 11:12:56, mmenke wrote: > On 2016/08/29 11:02:08, battre (OOO until Aug 22) wrote: > > On 2016/08/29 09:55:34, mmenke wrote: > > > On 2016/08/29 07:15:37, Ramin Halavati wrote: > > > > On 2016/08/26 21:27:53, mmenke wrote: > > > > > On 2016/08/26 20:14:28, mmenke (busy) wrote: > > > > > > On 2016/08/26 17:42:21, mmenke (busy) wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2258483002/diff/1/chrome/browser/signin/chrom... > > > > > > > File chrome/browser/signin/chrome_signin_helper.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2258483002/diff/1/chrome/browser/signin/chrom... > > > > > > > chrome/browser/signin/chrome_signin_helper.cc:154: > > > > > > io_data->GetCookieSettings(), > > > > > > > profile_mode_mask)) > > > > > > > On 2016/08/26 17:04:31, RaminH wrote: > > > > > > > > On 2016/08/23 20:44:52, mmenke wrote: > > > > > > > > > On 2016/08/23 20:34:16, mmenke wrote: > > > > > > > > > > AppendMirrorRequestHeaderIfPossible seems problematic. It > > doesn't > > > > > look > > > > > > > like > > > > > > > > > it > > > > > > > > > > checks for HTTPS, and relies on HSTS being enabled for Google > > > > > > > > domains...Which > > > > > > > > > > hopefully is true, but this code should not rely on it. That > > also > > > > > means > > > > > > > it > > > > > > > > > > exposes this information to HTTP-only extensions, even if all > > > google > > > > > > > domains > > > > > > > > > are > > > > > > > > > > HSTS, which seems weird. > > > > > > > > > > > > > > > > > > > > A third party ServiceWorker can also serve requests to > > > > > > > http://www.google.com > > > > > > > > > > with responses from http://www.evil.com... I believe we won't > > > > expose > > > > > > > > headers > > > > > > > > > > for the request in that case, either to the ServiceWorker or > > over > > > > the > > > > > > > > network > > > > > > > > > > but it still makes me very nervous. > > > > > > > > > > > > > > > > > > And this also assumes all Google.* TLDs are owned by Google, > which > > > > also > > > > > > > seems > > > > > > > > > like a problem. If someone registers http://google.mil, or just > > > > hijacks > > > > > > > > request to it, > > > > > > > > > for instance, it looks like they could get this information. > > > > > > > > > > > > > > > > I think this is a bigger issue of google_util::IsGoogleDomainUrl. > I > > > > think > > > > > > that > > > > > > > > multiple services depend on this function working properly. I > think > > we > > > > > > should > > > > > > > > consider this out of scope for this CL. How would you suggest to > fix > > > > this? > > > > > > > > > > > > > > My feeling is that we should not rely on it for any security or > > privacy > > > > > > related > > > > > > > checks. I don't know what project mirror is, but I don't think we > > > should > > > > > just > > > > > > > be arbitrarily adding privacy-relevant headers to requests based on > > > > > requested > > > > > > > URLs. It seems like a problematic design to me. > > > > > > > > > > > > > > I do agree that this seems like a more general problem with > > > > > IsGoogleDomainUrl > > > > > > > itself, even with an HTTPS check added to it. > > > > > > > > > > > > Just consider: There's a .sucks TLD. A site with gripes about Google > > > would > > > > > be > > > > > > a legitimate use of that domain, and could even get a fully valid > > cert... > > > > So > > > > > > this just seems a very bad idea. > > > > > > > > > > Anyhow, the only two valid fixes for this seem to be a whitelist, or > > > removing > > > > > the feature (The current implementation of it, at least). I'd prefer > the > > > > latter > > > > > approach, but the former also works, though I'd say longer term, we > should > > > > look > > > > > at other options for implementing whatever this feature is. > > > > > > > > So you suggest that we block the CL because, if anything, the CL restricts > > the > > > > domains that get the headers? > > > > > > I don't understand the comment. I'm saying this CL doesn't actually fix the > > > issue. > > > > I would argue that we have two problems > > 1) Chrome does not drop the mirror header after a redirect to a non-google > > domain. > > 2) Chrome's method to determine google vs. non-google domains is bad. > > > > Shouldn't we consider these different issues and fix them in separate CLs? > This > > CL addresses only 1), the Mirror specific part, but not 2). [Actually, the CL > > also removes the mirror header from unencrypted HTTP traffic] > > I think we have a third bug: > > 3) ServiceWorker: https://www.evil.com can make a request a subresource from > https://www.google.com. This class then attaches the header. Then > https://www.evil.com ServiceWorker redirects the request to > https://www.evil.com/. I said I didn't think it was an issue before, but I'm > less sure now - in that case, does either the ServiceWorker or http://evil.com see the > header? Normally, the http://evil.com page would have been responsible for setting the > headers itself (Or through a script that runs in its context), so maybe it does. As for different issues, I'd rather we either 1) have a concrete plan to fix all this, or 2) disable the feature entirely until it gets a full security audit.
Talked to battre, who convinced me someone cares enough to see that this issue is fixed. I'll take another look at this later today, and hopefully sign off on it then.
On 2016/08/29 11:14:36, mmenke wrote: > On 2016/08/29 11:12:56, mmenke wrote: > > On 2016/08/29 11:02:08, battre (OOO until Aug 22) wrote: > > > On 2016/08/29 09:55:34, mmenke wrote: > > > > On 2016/08/29 07:15:37, Ramin Halavati wrote: > > > > > On 2016/08/26 21:27:53, mmenke wrote: > > > > > > On 2016/08/26 20:14:28, mmenke (busy) wrote: > > > > > > > On 2016/08/26 17:42:21, mmenke (busy) wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2258483002/diff/1/chrome/browser/signin/chrom... > > > > > > > > File chrome/browser/signin/chrome_signin_helper.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2258483002/diff/1/chrome/browser/signin/chrom... > > > > > > > > chrome/browser/signin/chrome_signin_helper.cc:154: > > > > > > > io_data->GetCookieSettings(), > > > > > > > > profile_mode_mask)) > > > > > > > > On 2016/08/26 17:04:31, RaminH wrote: > > > > > > > > > On 2016/08/23 20:44:52, mmenke wrote: > > > > > > > > > > On 2016/08/23 20:34:16, mmenke wrote: > > > > > > > > > > > AppendMirrorRequestHeaderIfPossible seems problematic. It > > > doesn't > > > > > > look > > > > > > > > like > > > > > > > > > > it > > > > > > > > > > > checks for HTTPS, and relies on HSTS being enabled for > Google > > > > > > > > > domains...Which > > > > > > > > > > > hopefully is true, but this code should not rely on it. > That > > > also > > > > > > means > > > > > > > > it > > > > > > > > > > > exposes this information to HTTP-only extensions, even if > all > > > > google > > > > > > > > domains > > > > > > > > > > are > > > > > > > > > > > HSTS, which seems weird. > > > > > > > > > > > > > > > > > > > > > > A third party ServiceWorker can also serve requests to > > > > > > > > http://www.google.com > > > > > > > > > > > with responses from http://www.evil.com... I believe we > won't > > > > > expose > > > > > > > > > headers > > > > > > > > > > > for the request in that case, either to the ServiceWorker or > > > over > > > > > the > > > > > > > > > network > > > > > > > > > > > but it still makes me very nervous. > > > > > > > > > > > > > > > > > > > > And this also assumes all Google.* TLDs are owned by Google, > > which > > > > > also > > > > > > > > seems > > > > > > > > > > like a problem. If someone registers http://google.mil, or > just > > > > > hijacks > > > > > > > > > request to it, > > > > > > > > > > for instance, it looks like they could get this information. > > > > > > > > > > > > > > > > > > I think this is a bigger issue of > google_util::IsGoogleDomainUrl. > > I > > > > > think > > > > > > > that > > > > > > > > > multiple services depend on this function working properly. I > > think > > > we > > > > > > > should > > > > > > > > > consider this out of scope for this CL. How would you suggest to > > fix > > > > > this? > > > > > > > > > > > > > > > > My feeling is that we should not rely on it for any security or > > > privacy > > > > > > > related > > > > > > > > checks. I don't know what project mirror is, but I don't think we > > > > should > > > > > > just > > > > > > > > be arbitrarily adding privacy-relevant headers to requests based > on > > > > > > requested > > > > > > > > URLs. It seems like a problematic design to me. > > > > > > > > > > > > > > > > I do agree that this seems like a more general problem with > > > > > > IsGoogleDomainUrl > > > > > > > > itself, even with an HTTPS check added to it. > > > > > > > > > > > > > > Just consider: There's a .sucks TLD. A site with gripes about > Google > > > > would > > > > > > be > > > > > > > a legitimate use of that domain, and could even get a fully valid > > > cert... > > > > > So > > > > > > > this just seems a very bad idea. > > > > > > > > > > > > Anyhow, the only two valid fixes for this seem to be a whitelist, or > > > > removing > > > > > > the feature (The current implementation of it, at least). I'd prefer > > the > > > > > latter > > > > > > approach, but the former also works, though I'd say longer term, we > > should > > > > > look > > > > > > at other options for implementing whatever this feature is. > > > > > > > > > > So you suggest that we block the CL because, if anything, the CL > restricts > > > the > > > > > domains that get the headers? > > > > > > > > I don't understand the comment. I'm saying this CL doesn't actually fix > the > > > > issue. > > > > > > I would argue that we have two problems > > > 1) Chrome does not drop the mirror header after a redirect to a non-google > > > domain. > > > 2) Chrome's method to determine google vs. non-google domains is bad. > > > > > > Shouldn't we consider these different issues and fix them in separate CLs? > > This > > > CL addresses only 1), the Mirror specific part, but not 2). [Actually, the > CL > > > also removes the mirror header from unencrypted HTTP traffic] > > > > I think we have a third bug: > > > > 3) ServiceWorker: https://www.evil.com can make a request a subresource from > > https://www.google.com. This class then attaches the header. Then > > https://www.evil.com ServiceWorker redirects the request to > > https://www.evil.com/. I said I didn't think it was an issue before, but I'm > > less sure now - in that case, does either the ServiceWorker or http://evil.com > see the > > header? Normally, the http://evil.com page would have been responsible for > setting the > > headers itself (Or through a script that runs in its context), so maybe it > does. > > As for different issues, I'd rather we either 1) have a concrete plan to fix all > this, or 2) disable the feature entirely until it gets a full security audit. I have chatted with Matt and we agreed that - I will reach out to the ServiceWorkers forks to verify whether 3) is a problem, - reach out to Eli to understand why we need the header in the first place, and - I will generally try to keep the discussion about IsGoogleDomainUrl alive (even though I am not the most knowledgeable person to come up with the solution).
https://codereview.chromium.org/2258483002/diff/60001/chrome/browser/renderer... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc (right): https://codereview.chromium.org/2258483002/diff/60001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:307: }; Do the google checks care about port? It doesn't look like they do. If not, then rather than create a URLRequestJob subclass for this, you can just use embedded_test_server()->GetURL(echoheader?<header>), and check the contents of the page using javascript. std::string command ="domAutomationController.send(document.body.innerText);"; std::string body; EXPECT_TRUE(content::ExecuteScriptAndExtractString( browser->tab_strip_model()->GetActiveWebContents(), command, &body)); return body; Makes for a lot less boilerplate. The embedded_test_server can also handle redirects (Which this file already uses, higher up). You made need to add rules to host_resolver() to map www.google.com and www.redirect.com to 127.0.0.1. https://codereview.chromium.org/2258483002/diff/60001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:386: "User Data", false); Why are we appending this? Shouldn't the RDH do that on its own? https://codereview.chromium.org/2258483002/diff/60001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:429: urls_with_header.end()); This seems like it would be much cleaner as single loop that navigates, waits to hear back from other thread, checks the result and then continues. https://codereview.chromium.org/2258483002/diff/60001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:461: } I'm unable to figure out how we visit these URLs. It doesn't look to me like the one URL we visit redirects to the other two. https://codereview.chromium.org/2258483002/diff/60001/chrome/browser/signin/c... File chrome/browser/signin/chrome_signin_helper.cc (right): https://codereview.chromium.org/2258483002/diff/60001/chrome/browser/signin/c... chrome/browser/signin/chrome_signin_helper.cc:150: Think everything below this point may belong in a single method in signin_header_helper.h - seems like the code that handles AppendMirrorRequestHeaderIfPossible belongs in the same file that handles removing the header on redirect as well. https://codereview.chromium.org/2258483002/diff/60001/chrome/browser/signin/c... chrome/browser/signin/chrome_signin_helper.cc:152: // redirecting to another site and x-chrome-header exists, and the redirected x-chrome-header -> x-chrome-connected header (Or x-chrome header, I suppose) https://codereview.chromium.org/2258483002/diff/60001/chrome/browser/signin/c... chrome/browser/signin/chrome_signin_helper.cc:153: // site is not illigible and current site was illigible, remove it. illigible -> eligible (x2) https://codereview.chromium.org/2258483002/diff/60001/chrome/browser/signin/c... chrome/browser/signin/chrome_signin_helper.cc:158: return true; The return value isn't used, and it isn't clear what it means. Let's just make this method void. https://codereview.chromium.org/2258483002/diff/60001/chrome/browser/signin/c... chrome/browser/signin/chrome_signin_helper.cc:159: } else { Since you have the early return, this else isn't needed. https://codereview.chromium.org/2258483002/diff/60001/chrome/browser/signin/c... chrome/browser/signin/chrome_signin_helper.cc:162: signin::kChromeConnectedHeader) && // x-chrome-header exists Rather than inline comments, suggest writing out description in full sentences, just before the if. i.e. "If the request is being redirected from Google URL to a non-Google URL, remove the x-chrome-connected header, if present." https://codereview.chromium.org/2258483002/diff/60001/chrome/browser/signin/c... chrome/browser/signin/chrome_signin_helper.cc:165: request->RemoveRequestHeaderByName(signin::kChromeConnectedHeader); Use braces when an if condition takes multiple lines. https://codereview.chromium.org/2258483002/diff/60001/components/signin/core/... File components/signin/core/browser/signin_header_helper.cc (right): https://codereview.chromium.org/2258483002/diff/60001/components/signin/core/... components/signin/core/browser/signin_header_helper.cc:236: return url.SchemeIsCryptographic(); Suggest putting this first. Think that makes this check least susceptible to regression. https://codereview.chromium.org/2258483002/diff/60001/components/signin/core/... components/signin/core/browser/signin_header_helper.cc:238: } // namespace signin nit: Blank line before end of namespace.
https://codereview.chromium.org/2258483002/diff/60001/chrome/browser/renderer... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc (right): https://codereview.chromium.org/2258483002/diff/60001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:307: }; On 2016/08/30 19:12:26, mmenke wrote: > Do the google checks care about port? It doesn't look like they do. If not, > then rather than create a URLRequestJob subclass for this, you can just use > embedded_test_server()->GetURL(echoheader?<header>), and check the contents of > the page using javascript. > > std::string command ="domAutomationController.send(document.body.innerText);"; > std::string body; > EXPECT_TRUE(content::ExecuteScriptAndExtractString( > browser->tab_strip_model()->GetActiveWebContents(), command, &body)); > return body; > > Makes for a lot less boilerplate. > > The embedded_test_server can also handle redirects (Which this file already > uses, higher up). > > You made need to add rules to host_resolver() to map http://www.google.com and > http://www.redirect.com to 127.0.0.1. Actually, probably best to ignore this suggestion - we may want to restrict by port in the future, even if we aren't now, and this current arrangement lets us do that without modifying the tests.
Just as an FYI, I'm thinking that I'll move chrome_resource_dispatcher_host_delegate.cc into chrome/browser/net, so hopefully future changes there will get in front of a network person, who would be more likely to have caught this than a top-level chrome owner. I am going to wait until after this fix lands, though, to make merging this CL easier.
All comments addressed, please check again. https://codereview.chromium.org/2258483002/diff/60001/chrome/browser/renderer... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc (right): https://codereview.chromium.org/2258483002/diff/60001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:307: }; On 2016/08/30 19:23:39, mmenke wrote: > On 2016/08/30 19:12:26, mmenke wrote: > > Do the google checks care about port? It doesn't look like they do. If not, > > then rather than create a URLRequestJob subclass for this, you can just use > > embedded_test_server()->GetURL(echoheader?<header>), and check the contents of > > the page using javascript. > > > > std::string command > ="domAutomationController.send(document.body.innerText);"; > > std::string body; > > EXPECT_TRUE(content::ExecuteScriptAndExtractString( > > browser->tab_strip_model()->GetActiveWebContents(), command, &body)); > > return body; > > > > Makes for a lot less boilerplate. > > > > The embedded_test_server can also handle redirects (Which this file already > > uses, higher up). > > > > You made need to add rules to host_resolver() to map http://www.google.com and > > http://www.redirect.com to 127.0.0.1. > > Actually, probably best to ignore this suggestion - we may want to restrict by > port in the future, even if we aren't now, and this current arrangement lets us > do that without modifying the tests. Acknowledged. https://codereview.chromium.org/2258483002/diff/60001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:307: }; On 2016/08/30 19:12:26, mmenke wrote: > Do the google checks care about port? It doesn't look like they do. If not, > then rather than create a URLRequestJob subclass for this, you can just use > embedded_test_server()->GetURL(echoheader?<header>), and check the contents of > the page using javascript. > > std::string command ="domAutomationController.send(document.body.innerText);"; > std::string body; > EXPECT_TRUE(content::ExecuteScriptAndExtractString( > browser->tab_strip_model()->GetActiveWebContents(), command, &body)); > return body; > > Makes for a lot less boilerplate. > > The embedded_test_server can also handle redirects (Which this file already > uses, higher up). > > You made need to add rules to host_resolver() to map http://www.google.com and > http://www.redirect.com to 127.0.0.1. Acknowledged. https://codereview.chromium.org/2258483002/diff/60001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:386: "User Data", false); On 2016/08/30 19:12:26, mmenke wrote: > Why are we appending this? Shouldn't the RDH do that on its own? We added a new test in the last CL in which a website not owned by Google my add this header, and then redirects to another, and we check that the header should not be removed in this case. This part adds the header for this website. https://codereview.chromium.org/2258483002/diff/60001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:429: urls_with_header.end()); On 2016/08/30 19:12:26, mmenke wrote: > This seems like it would be much cleaner as single loop that navigates, waits to > hear back from other thread, checks the result and then continues. Done. https://codereview.chromium.org/2258483002/diff/60001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:461: } On 2016/08/30 19:12:26, mmenke wrote: > I'm unable to figure out how we visit these URLs. It doesn't look to me like > the one URL we visit redirects to the other two. We have 3 starting URLs here and each one redirects to another one: 1) http://www.google.com --> http://www.redirected.com 2) https://www.google.com --> https://www.redirected.com 3) http:/www.header_adder.com --> http://www.redirected_from_header_adder.com In the first case, we just check that the header is not added to any of them. In the second case the original site should have the header, but it should not be sent to the redirected site. In the third case, header is added to the original site and we check that the redirected site also receives that. https://codereview.chromium.org/2258483002/diff/60001/chrome/browser/signin/c... File chrome/browser/signin/chrome_signin_helper.cc (right): https://codereview.chromium.org/2258483002/diff/60001/chrome/browser/signin/c... chrome/browser/signin/chrome_signin_helper.cc:150: On 2016/08/30 19:12:26, mmenke wrote: > Think everything below this point may belong in a single method in > signin_header_helper.h - seems like the code that handles > AppendMirrorRequestHeaderIfPossible belongs in the same file that handles > removing the header on redirect as well. Done. The removed part is now moved to signin_header_helper.cc and the function AppendMirrorRequestHeaderIfPossible is renamed to AppendOrRemoveMirrorRequestHeaderIfPossible. Do you suggest that this one would also become void? https://codereview.chromium.org/2258483002/diff/60001/chrome/browser/signin/c... chrome/browser/signin/chrome_signin_helper.cc:152: // redirecting to another site and x-chrome-header exists, and the redirected On 2016/08/30 19:12:26, mmenke wrote: > x-chrome-header -> x-chrome-connected header (Or x-chrome header, I suppose) Done. https://codereview.chromium.org/2258483002/diff/60001/chrome/browser/signin/c... chrome/browser/signin/chrome_signin_helper.cc:153: // site is not illigible and current site was illigible, remove it. On 2016/08/30 19:12:27, mmenke wrote: > illigible -> eligible (x2) Done. https://codereview.chromium.org/2258483002/diff/60001/chrome/browser/signin/c... chrome/browser/signin/chrome_signin_helper.cc:158: return true; On 2016/08/30 19:12:27, mmenke wrote: > The return value isn't used, and it isn't clear what it means. Let's just make > this method void. Done. https://codereview.chromium.org/2258483002/diff/60001/chrome/browser/signin/c... chrome/browser/signin/chrome_signin_helper.cc:159: } else { On 2016/08/30 19:12:27, mmenke wrote: > Since you have the early return, this else isn't needed. Done. https://codereview.chromium.org/2258483002/diff/60001/chrome/browser/signin/c... chrome/browser/signin/chrome_signin_helper.cc:162: signin::kChromeConnectedHeader) && // x-chrome-header exists On 2016/08/30 19:12:27, mmenke wrote: > Rather than inline comments, suggest writing out description in full sentences, > just before the if. > > i.e. "If the request is being redirected from Google URL to a non-Google URL, > remove the x-chrome-connected header, if present." Done. https://codereview.chromium.org/2258483002/diff/60001/chrome/browser/signin/c... chrome/browser/signin/chrome_signin_helper.cc:165: request->RemoveRequestHeaderByName(signin::kChromeConnectedHeader); On 2016/08/30 19:12:27, mmenke wrote: > Use braces when an if condition takes multiple lines. Done. https://codereview.chromium.org/2258483002/diff/60001/components/signin/core/... File components/signin/core/browser/signin_header_helper.cc (right): https://codereview.chromium.org/2258483002/diff/60001/components/signin/core/... components/signin/core/browser/signin_header_helper.cc:236: return url.SchemeIsCryptographic(); On 2016/08/30 19:12:27, mmenke wrote: > Suggest putting this first. Think that makes this check least susceptible to > regression. Done. https://codereview.chromium.org/2258483002/diff/60001/components/signin/core/... components/signin/core/browser/signin_header_helper.cc:238: } // namespace signin On 2016/08/30 19:12:27, mmenke wrote: > nit: Blank line before end of namespace. Done.
Ok, I'm following the tests now, look good. https://codereview.chromium.org/2258483002/diff/100001/chrome/browser/rendere... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc (right): https://codereview.chromium.org/2258483002/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:359: void ReportRequestHeaders(std::map<std::string, std::string>* request_headers, Should document this. https://codereview.chromium.org/2258483002/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:359: void ReportRequestHeaders(std::map<std::string, std::string>* request_headers, include map https://codereview.chromium.org/2258483002/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:361: const std::string& headers) { DCHECK_CURRENTLY_ON(BrowserThread::UI); (Or whatever the syntax is) https://codereview.chromium.org/2258483002/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:361: const std::string& headers) { EXPECT_TRUE(request_header.find(url) == std::map::npos);? https://codereview.chromium.org/2258483002/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:369: class HeaderTestDispatcherHostDelegate Should document this. https://codereview.chromium.org/2258483002/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:392: DISALLOW_COPY_AND_ASSIGN(HeaderTestDispatcherHostDelegate); nit: Blank line before DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2258483002/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:421: base::Bind(&ReportRequestHeaders, &request_headers); Move this stuff into the loop, so it's cleared between loop iterations? https://codereview.chromium.org/2258483002/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:428: GURL urls[2]; Think you should name these two URLs - you have to double-up the checking code at the end, but think it's worth the clarity. something like "original_url" and "redirected_to_url" https://codereview.chromium.org/2258483002/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:455: dispatcher_host_delegate.get()); Here and below, should set this on the IO thread (Below should also wait for it to be set on the IO thread, before continuing) https://codereview.chromium.org/2258483002/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:488: content::ResourceDispatcherHost::Get()->SetDelegate(nullptr); We should restore the original RDHDelegate instead.
All comments are addressed, please check again. https://codereview.chromium.org/2258483002/diff/100001/chrome/browser/rendere... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc (right): https://codereview.chromium.org/2258483002/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:359: void ReportRequestHeaders(std::map<std::string, std::string>* request_headers, On 2016/09/01 18:31:48, mmenke wrote: > include map Done. https://codereview.chromium.org/2258483002/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:359: void ReportRequestHeaders(std::map<std::string, std::string>* request_headers, On 2016/09/01 18:31:48, mmenke wrote: > include map Done. https://codereview.chromium.org/2258483002/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:361: const std::string& headers) { On 2016/09/01 18:31:48, mmenke wrote: > DCHECK_CURRENTLY_ON(BrowserThread::UI); (Or whatever the syntax is) Done. https://codereview.chromium.org/2258483002/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:361: const std::string& headers) { On 2016/09/01 18:31:48, mmenke wrote: > EXPECT_TRUE(request_header.find(url) == std::map::npos);? Done. https://codereview.chromium.org/2258483002/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:369: class HeaderTestDispatcherHostDelegate On 2016/09/01 18:31:48, mmenke wrote: > Should document this. Done. https://codereview.chromium.org/2258483002/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:392: DISALLOW_COPY_AND_ASSIGN(HeaderTestDispatcherHostDelegate); On 2016/09/01 18:31:48, mmenke wrote: > nit: Blank line before DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2258483002/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:421: base::Bind(&ReportRequestHeaders, &request_headers); On 2016/09/01 18:31:48, mmenke wrote: > Move this stuff into the loop, so it's cleared between loop iterations? Done. https://codereview.chromium.org/2258483002/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:428: GURL urls[2]; On 2016/09/01 18:31:48, mmenke wrote: > Think you should name these two URLs - you have to double-up the checking code > at the end, but think it's worth the clarity. > > something like "original_url" and "redirected_to_url" Done. https://codereview.chromium.org/2258483002/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:455: dispatcher_host_delegate.get()); On 2016/09/01 18:31:48, mmenke wrote: > Here and below, should set this on the IO thread (Below should also wait for it > to be set on the IO thread, before continuing) Done. https://codereview.chromium.org/2258483002/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:488: content::ResourceDispatcherHost::Get()->SetDelegate(nullptr); On 2016/09/01 18:31:48, mmenke wrote: > We should restore the original RDHDelegate instead. I couldn't find anyway in public interface to get it, and in all other usages it is set to nullptr. Can you suggest how it should be done?
LGTM https://codereview.chromium.org/2258483002/diff/100001/chrome/browser/rendere... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc (right): https://codereview.chromium.org/2258483002/diff/100001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:488: content::ResourceDispatcherHost::Get()->SetDelegate(nullptr); On 2016/09/02 13:41:43, Ramin Halavati wrote: > On 2016/09/01 18:31:48, mmenke wrote: > > We should restore the original RDHDelegate instead. > > I couldn't find anyway in public interface to get it, and in all other usages it > is set to nullptr. Can you suggest how it should be done? You're right. Looks like the RDH mostly allows a nullptr delegate, so let's just keep setting it to nullptr. https://codereview.chromium.org/2258483002/diff/120001/chrome/browser/rendere... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc (right): https://codereview.chromium.org/2258483002/diff/120001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:471: // Setup mockup interceptors. nit: Set up (setup is a noun, not a verb) https://codereview.chromium.org/2258483002/diff/120001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:495: // If delegate is changed, remove it. Think it's worth noting that the correctness of this line depends on the BrowserThread::PostTaskAndReply and below (i.e., otherwise, the RDH will have an invalid delegate pointer when there may be stray requests hanging, like the request for the favicon resource, which could cause a crash) Or maybe better, just make this a PostTaskAndReply, so it works even if the next one is removed for some reason.
All comments addressed. https://codereview.chromium.org/2258483002/diff/120001/chrome/browser/rendere... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc (right): https://codereview.chromium.org/2258483002/diff/120001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:471: // Setup mockup interceptors. On 2016/09/02 16:29:43, mmenke wrote: > nit: Set up (setup is a noun, not a verb) Done. https://codereview.chromium.org/2258483002/diff/120001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:495: // If delegate is changed, remove it. On 2016/09/02 16:29:43, mmenke wrote: > Think it's worth noting that the correctness of this line depends on the > BrowserThread::PostTaskAndReply and below (i.e., otherwise, the RDH will have an > invalid delegate pointer when there may be stray requests hanging, like the > request for the favicon resource, which could cause a crash) > > Or maybe better, just make this a PostTaskAndReply, so it works even if the next > one is removed for some reason. Done.
https://codereview.chromium.org/2258483002/diff/140001/chrome/browser/rendere... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc (right): https://codereview.chromium.org/2258483002/diff/140001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:371: void EmptyClosure() {} Use DoNothing (in bind_helpers). https://codereview.chromium.org/2258483002/diff/140001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:421: base::test::ScopedCommandLine command_line; Do you really need this? Each browser_process runs in its own process, so I suspect you don't need this. https://codereview.chromium.org/2258483002/diff/140001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:465: new HeaderTestDispatcherHostDelegate(test_case.original_url)); MakeUnique is (apparently) the preferred way to new objects that are assigned to unique_ptrs. https://codereview.chromium.org/2258483002/diff/140001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:468: base::Bind(&SetDelegateOnIO, dispatcher_host_delegate.get())); Do you need to block at all to ensure this is install before continuing?
Comments are addressed, please check the following file again: chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc https://codereview.chromium.org/2258483002/diff/140001/chrome/browser/rendere... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc (right): https://codereview.chromium.org/2258483002/diff/140001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:371: void EmptyClosure() {} On 2016/09/06 18:29:04, sky wrote: > Use DoNothing (in bind_helpers). Done. https://codereview.chromium.org/2258483002/diff/140001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:421: base::test::ScopedCommandLine command_line; On 2016/09/06 18:29:04, sky wrote: > Do you really need this? Each browser_process runs in its own process, so I > suspect you don't need this. As other tests also use this fixture, I thought this is the cleanest approach. Just changed it to CommandLine::ForCurrentProcess() to be only for this process. https://codereview.chromium.org/2258483002/diff/140001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:465: new HeaderTestDispatcherHostDelegate(test_case.original_url)); On 2016/09/06 18:29:04, sky wrote: > MakeUnique is (apparently) the preferred way to new objects that are assigned to > unique_ptrs. Done. https://codereview.chromium.org/2258483002/diff/140001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:468: base::Bind(&SetDelegateOnIO, dispatcher_host_delegate.get())); On 2016/09/06 18:29:04, sky wrote: > Do you need to block at all to ensure this is install before continuing? I am not sure why you think we are blocking. I think we are not blocking. Any request to the RDH using the delegate should only happen on the IO thread and should be queued after the execution of this assignment. Therefore, I think that this should be fine.
SLGTM https://codereview.chromium.org/2258483002/diff/140001/chrome/browser/rendere... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc (right): https://codereview.chromium.org/2258483002/diff/140001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:468: base::Bind(&SetDelegateOnIO, dispatcher_host_delegate.get())); On 2016/09/07 07:51:21, Ramin Halavati wrote: > On 2016/09/06 18:29:04, sky wrote: > > Do you need to block at all to ensure this is install before continuing? > > I am not sure why you think we are blocking. > > I think we are not blocking. Any request to the RDH using the delegate should > only happen on the IO thread and should be queued after the execution of this > assignment. Therefore, I think that this should be fine. Sorry if I wasn't clear. My question is, do you need to block execution of the test until this function completes? Based on your answer, it sounds like no. Thanks.
[sky]: Doesn't look like rietveld likes your "SLGTM" https://codereview.chromium.org/2258483002/diff/140001/chrome/browser/rendere... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc (right): https://codereview.chromium.org/2258483002/diff/140001/chrome/browser/rendere... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate_browsertest.cc:468: base::Bind(&SetDelegateOnIO, dispatcher_host_delegate.get())); On 2016/09/07 15:16:46, sky wrote: > On 2016/09/07 07:51:21, Ramin Halavati wrote: > > On 2016/09/06 18:29:04, sky wrote: > > > Do you need to block at all to ensure this is install before continuing? > > > > I am not sure why you think we are blocking. > > > > I think we are not blocking. Any request to the RDH using the delegate should > > only happen on the IO thread and should be queued after the execution of this > > assignment. Therefore, I think that this should be fine. > > Sorry if I wasn't clear. My question is, do you need to block execution of the > test until this function completes? Based on your answer, it sounds like no. > Thanks. In the case where we're removing the delegate, we do (For reasons I mentioned earlier). In the case we're setting it, I think we're fine not waiting.
LGTM
The CQ bit was checked by rhalavati@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rogerta@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2258483002/#ps160001 (title: "Addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== X-Chrome-Connected is stripped when it should not be in headers. AppendMirrorRequestHeaderHelper added the X-Chrome-Connected header when it was required, but did not remove it when it was not required and already exited from previous redirection. The funcion is now renamed to FixMirrorRequestHeaderHelper and removes already existing header if it is not required. BUG=588492 ========== to ========== X-Chrome-Connected is stripped when it should not be in headers. AppendMirrorRequestHeaderHelper added the X-Chrome-Connected header when it was required, but did not remove it when it was not required and already exited from previous redirection. The funcion is now renamed to FixMirrorRequestHeaderHelper and removes already existing header if it is not required. BUG=588492 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== X-Chrome-Connected is stripped when it should not be in headers. AppendMirrorRequestHeaderHelper added the X-Chrome-Connected header when it was required, but did not remove it when it was not required and already exited from previous redirection. The funcion is now renamed to FixMirrorRequestHeaderHelper and removes already existing header if it is not required. BUG=588492 ========== to ========== X-Chrome-Connected is stripped when it should not be in headers. AppendMirrorRequestHeaderHelper added the X-Chrome-Connected header when it was required, but did not remove it when it was not required and already exited from previous redirection. The funcion is now renamed to FixMirrorRequestHeaderHelper and removes already existing header if it is not required. BUG=588492 Committed: https://crrev.com/74362e9c627d1fabf537b351ffe5118874f30480 Cr-Commit-Position: refs/heads/master@{#417231} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/74362e9c627d1fabf537b351ffe5118874f30480 Cr-Commit-Position: refs/heads/master@{#417231} |
