Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1223233002:
Common Name Mismatch Handler For WWW Subdomain Mismatch case (Closed)
DescriptionCommon Name Mismatch Handler For WWW Subdomain Mismatch case
When there is a WWW sub domain mismatch between
the hostname entered in the URL and the dns name present in the
certificate, we ping the www mismatched domain. If a valid response
code is received, we redirect the user to the domain with valid certificate and provide a console message for developers.
BUG=507454
Committed: https://crrev.com/2051ce7a01a4026ec1611c940a3d83a55a6e19ef
Cr-Commit-Position: refs/heads/master@{#344052}
Patch Set 1 : Added new class CommonNameMismatchHandler #Patch Set 2 : Added CommonNameMismatch check in SSLErrorHandler #Patch Set 3 : Changing function signature in unnittest file #
Total comments: 58
Patch Set 4 : Handling redirection, Resolving Comments #Patch Set 5 : Unittests #
Total comments: 49
Patch Set 6 : Added Navigation, Resolved Comments #Patch Set 7 : Resolving Comments #
Total comments: 10
Patch Set 8 : Browsertests #
Total comments: 3
Patch Set 9 : Minor Changes: Removing test code #
Total comments: 90
Patch Set 10 : Resolved comments, added UMA #
Total comments: 18
Patch Set 11 : resolved comments: documentation, style changes #
Total comments: 6
Patch Set 12 : mmenke's comments #
Total comments: 1
Patch Set 13 : Rebasing #Patch Set 14 : Browsertests using MockCertVerifier #
Total comments: 19
Patch Set 15 : Removed interstitial code, Implemented Redirection #Patch Set 16 : Revert changes in security_interstitial_page.h #Patch Set 17 : Fixing trybot failures #
Total comments: 2
Patch Set 18 : Documentation Changes #
Total comments: 56
Patch Set 19 : Resolving comments: Fixing some bugs, documentation #Patch Set 20 : Fixing Unittests #
Total comments: 53
Patch Set 21 : Resolving Comments #Patch Set 22 : Rebasing #
Total comments: 4
Patch Set 23 : Resolving comments #Messages
Total messages: 62 (9 generated)
bhanudev@google.com changed reviewers: + meacer@chromium.org, palmer@chromium.org
On 2015/07/09 03:38:00, bhanudev wrote: > mailto:bhanudev@google.com changed reviewers: > + mailto:meacer@chromium.org, mailto:palmer@chromium.org Hi Mustafa and Chris, This cl has www mismatch handler implementation. Tests have to be added. Can you review the code when you are free. Thanks. - Bhanu
Looking good. Now the hard part: tests :) https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/comm... File chrome/browser/ssl/common_name_mismatch_handler.cc (right): https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.cc:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. nit: New style uses "Copyright 2015" instead of "Copyright (c) 2015". https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.cc:78: return; no need for |return| https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.cc:89: &www_mismatch_host_name)) { Early exit from here: if (!...) return false; https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/comm... File chrome/browser/ssl/common_name_mismatch_handler.h (right): https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.h:30: // cert. is valid . nit: cert -> certificate. Remove space before the last period. https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.h:40: GURL new_url; You need to include gurl.h instead of fwd declare, because of this line. https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.h:49: // Triggers an check to validate suggested url. After completion, runs the an check -> a check https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.h:55: void CancelUrlCheck(); I don't think you'll need this, so I suggest removing it. https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page.h (right): https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.h:71: const GURL& suggested_url); As we discussed offline, let's separate UI bits into another CL and just collect metrics in this CL. https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.h:136: const GURL& suggested_url_; Keep a copy instead of a reference. https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:340: result = result || true; result = result || true means result = true :) https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:346: result = result || true; Same here. https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.h (right): https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:79: // www.example.com ~ example.com -> true I understand this isn't your comment, but can you mention what the first and the second domains refer to? https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:156: return 1; |return true| instead of 1. Actually, just remove this function and enable the check by default. It'll only be enabled on canary and dev for a few days until you land the finch CL. https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:165: DCHECK(!dns_names.empty()); I feel that dns_names can actually be empty, so not sure if this check is valid. https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:290: ShowSSLInterstitial(results.new_url); For this CL, let's just record the result in a histogram and call ShowSSLInterstitial without a suggested URL. You can then add the UI logic once the design is finalized. That means you don't have to keep around a suggested_url for this CL (if the tests turn out to be difficult to write because of this, let me know and we can reconsider) This is going to slightly affect how many captive portal interstitials we display since this CL is giving priority to common name mismatches, but I expect the difference to be minimal so it's fine. https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.h (right): https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:65: // This method gets the result of whether the suggested url is valid. Displays nit: Drop "This method " https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:111: const int common_name_handler_delay_in_seconds = 3; You'll need a non-const value for this so that you can change it in tests. I think you should just reuse the existing methods and variables for the captive portal check: (GetInterstitialDisplayDelay and SetInterstitialDelayTypeForTest) https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:82: void ShowSSLInterstitial(GURL& suggested_url) override { const
https://codereview.chromium.org/1223233002/diff/40001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1223233002/diff/40001/chrome/app/generated_re... chrome/app/generated_resources.grd:10066: + Attackers might be trying to steal your information from <ph name="BEGIN_BOLD"><strong></ph><ph name="REQUEST_DOMAIN">$1<ex>example.com</ex></ph><ph name="END_BOLD"></strong></ph> (for example, passwords, messages, or credit cards). <ph name="NEWLINE"><br></ph><ph name="NEWLINE"><br></ph>However, this server provides a valid certificate for <ph name="BEGIN_BOLD"><strong></ph><ph name="SUGGESTED_DOMAIN">$2<ex>www.example.com</ex></ph><ph name="END_BOLD"></strong></ph>. If you like to connect to <ph name="BEGIN_BOLD"><strong></ph><ph name="SUGGESTED_DOMAIN">$2<ex>www.example.com</ex></ph><ph name="END_BOLD"></strong></ph>, please click here <ph name="BEGIN_LINK"><a href="$3" ></ph><ph name="SUGGESTED_DOMAIN">$2<ex>www.example.com</ex></ph><ph name="END_LINK"></a></ph> Change "If you like to..." to "If you would like to...". I'm not sure about the first paragraph (" Attackers might be trying to steal your information..."). Maybe leave it off, and lead with "The server presented a certificate valid for www.example.com, but not example.com. Would you like to try www.example.com?" https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/comm... File chrome/browser/ssl/common_name_mismatch_handler.cc (right): https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.cc:76: if (url_fetcher->GetResponseCode() == 200) { Eventually, we will want to accept response codes other than 200, right? How about: static const int kGoodResponseCodes[] = { 200, 301, 302 }; static const int* kGoodResonseCodesEnd = kGoodResponseCodes + arraysize(kGoodResponseCodes); auto found = std::find(kGoodResponseCodes, kGoodResonseCodesEnd, url_fetcher->GetResponseCode()); results->result = found == kGoodResonseCodesEnd ? RESULT_SUGGESTED_URL_INVALID : RESULT_SUGGESTED_URL_VALID; Also, should we update results->new_url if RESULT_SUGGESTED_URL_INVALID? https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.cc:90: // Replaces the hostname in the request url with new host name. This comment is superfluous. https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/comm... File chrome/browser/ssl/common_name_mismatch_handler.h (right): https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.h:21: // This class contains methods to get a suggested url when there "url" -> "URL" through this file. "there is a ssl common name invalid error" -> "when certification validation fails due to |ERR_CERT_COMMON_NAME_INVALID|" Also, don't leave out articles: "to check validity" -> "to check the validity" https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.h:27: // Possible results of the validity of suggested url This comment is (IMO) superfluous. https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.h:57: // This method returns a bool indicating whether the error can be handled. Be more concise: "Returns true if the error can be handled." Use |...| to delineate C++ identifiers: "Interacts with |SslErrorClassification| to find the cause of the certificate validation error. If the error can be handled, writes a suggestion for a new URL to try into |suggested_url|." https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.h:60: static bool GetSuggestedUrl(const GURL request_url, Pass by reference to avoid an unnecessary copy: const GURL& request_url. https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.h:62: GURL& suggested_url); In Chromium style, output parameters ("out-params") are pointers: GURL* suggested_url. https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.h:69: // suggested URL, and fills a Results struct based on its result. Use |...| to delineate C++ identifiers. https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.h:70: void GetSuggestedUrlCheckResult(const net::URLFetcher* url_fetcher, Maybe const net::URLFetcher& instead of * ? https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.h:76: // URL request context. This comment is superfluous. https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.h:79: // Contains the callback method for urlfetch. This comment is superfluous.
https://codereview.chromium.org/1223233002/diff/40001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1223233002/diff/40001/chrome/app/generated_re... chrome/app/generated_resources.grd:10066: + Attackers might be trying to steal your information from <ph name="BEGIN_BOLD"><strong></ph><ph name="REQUEST_DOMAIN">$1<ex>example.com</ex></ph><ph name="END_BOLD"></strong></ph> (for example, passwords, messages, or credit cards). <ph name="NEWLINE"><br></ph><ph name="NEWLINE"><br></ph>However, this server provides a valid certificate for <ph name="BEGIN_BOLD"><strong></ph><ph name="SUGGESTED_DOMAIN">$2<ex>www.example.com</ex></ph><ph name="END_BOLD"></strong></ph>. If you like to connect to <ph name="BEGIN_BOLD"><strong></ph><ph name="SUGGESTED_DOMAIN">$2<ex>www.example.com</ex></ph><ph name="END_BOLD"></strong></ph>, please click here <ph name="BEGIN_LINK"><a href="$3" ></ph><ph name="SUGGESTED_DOMAIN">$2<ex>www.example.com</ex></ph><ph name="END_LINK"></a></ph> On 2015/07/09 19:13:01, palmer wrote: > Change "If you like to..." to "If you would like to...". > > I'm not sure about the first paragraph (" Attackers might be trying to steal > your information..."). Maybe leave it off, and lead with "The server presented a > certificate valid for http://www.example.com, but not http://example.com. Would you like to > try http://www.example.com?%22 Catherine is working on the strings for this. I will update this as soon as I get the new strings. https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/comm... File chrome/browser/ssl/common_name_mismatch_handler.cc (right): https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.cc:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. On 2015/07/09 17:58:55, Mustafa Emre Acer wrote: > nit: New style uses "Copyright 2015" instead of "Copyright (c) 2015". Done. https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.cc:76: if (url_fetcher->GetResponseCode() == 200) { On 2015/07/09 19:13:01, palmer wrote: > Eventually, we will want to accept response codes other than 200, right? How > about: > > static const int kGoodResponseCodes[] = { 200, 301, 302 }; > static const int* kGoodResonseCodesEnd = kGoodResponseCodes + > arraysize(kGoodResponseCodes); > > auto found = std::find(kGoodResponseCodes, kGoodResonseCodesEnd, > url_fetcher->GetResponseCode()); > results->result = found == kGoodResonseCodesEnd ? RESULT_SUGGESTED_URL_INVALID : > RESULT_SUGGESTED_URL_VALID; > > Also, should we update results->new_url if RESULT_SUGGESTED_URL_INVALID? Thanks for the code. |url_fetcher| is automatically following the redirects. It is not returning a 301 or 302 response code even when there is a redirect. So, I think we can just checking for response code 200. I tested it with https://google.com and it returned a 200 response code with the value of |url_fetcher->GetURL()| being https://www.google.com I will add a check to make sure it doesn't redirect to the current page. https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.cc:78: return; On 2015/07/09 17:58:55, Mustafa Emre Acer wrote: > no need for |return| Done. https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.cc:89: &www_mismatch_host_name)) { On 2015/07/09 17:58:55, Mustafa Emre Acer wrote: > Early exit from here: > > if (!...) > return false; Done. https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.cc:90: // Replaces the hostname in the request url with new host name. On 2015/07/09 19:13:01, palmer wrote: > This comment is superfluous. Done. https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/comm... File chrome/browser/ssl/common_name_mismatch_handler.h (right): https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.h:21: // This class contains methods to get a suggested url when there On 2015/07/09 19:13:01, palmer wrote: > "url" -> "URL" through this file. > > "there is a ssl common name invalid error" -> "when certification validation > fails due to |ERR_CERT_COMMON_NAME_INVALID|" > > Also, don't leave out articles: "to check validity" -> "to check the validity" Done. https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.h:27: // Possible results of the validity of suggested url On 2015/07/09 19:13:01, palmer wrote: > This comment is (IMO) superfluous. Done. https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.h:30: // cert. is valid . On 2015/07/09 17:58:55, Mustafa Emre Acer wrote: > nit: cert -> certificate. Remove space before the last period. Done. https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.h:40: GURL new_url; On 2015/07/09 17:58:55, Mustafa Emre Acer wrote: > You need to include gurl.h instead of fwd declare, because of this line. Done. https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.h:49: // Triggers an check to validate suggested url. After completion, runs the On 2015/07/09 17:58:55, Mustafa Emre Acer wrote: > an check -> a check Done. https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.h:55: void CancelUrlCheck(); On 2015/07/09 17:58:55, Mustafa Emre Acer wrote: > I don't think you'll need this, so I suggest removing it. Done. https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.h:57: // This method returns a bool indicating whether the error can be handled. On 2015/07/09 19:13:01, palmer wrote: > Be more concise: "Returns true if the error can be handled." > > Use |...| to delineate C++ identifiers: "Interacts with |SslErrorClassification| > to find the cause of the certificate validation error. If the error can be > handled, writes a suggestion for a new URL to try into |suggested_url|." Done. https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.h:60: static bool GetSuggestedUrl(const GURL request_url, On 2015/07/09 19:13:01, palmer wrote: > Pass by reference to avoid an unnecessary copy: const GURL& request_url. Done. https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.h:62: GURL& suggested_url); On 2015/07/09 19:13:01, palmer wrote: > In Chromium style, output parameters ("out-params") are pointers: GURL* > suggested_url. Done. https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.h:69: // suggested URL, and fills a Results struct based on its result. On 2015/07/09 19:13:01, palmer wrote: > Use |...| to delineate C++ identifiers. Done. https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.h:76: // URL request context. On 2015/07/09 19:13:01, palmer wrote: > This comment is superfluous. Done. https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.h:79: // Contains the callback method for urlfetch. On 2015/07/09 19:13:01, palmer wrote: > This comment is superfluous. Done. https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_blocking_page.h (right): https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.h:71: const GURL& suggested_url); On 2015/07/09 17:58:55, Mustafa Emre Acer wrote: > As we discussed offline, let's separate UI bits into another CL and just collect > metrics in this CL. Acknowledged. https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_blocking_page.h:136: const GURL& suggested_url_; On 2015/07/09 17:58:55, Mustafa Emre Acer wrote: > Keep a copy instead of a reference. Done. https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:340: result = result || true; On 2015/07/09 17:58:56, Mustafa Emre Acer wrote: > result = result || true means result = true :) Done. https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.cc:346: result = result || true; On 2015/07/09 17:58:56, Mustafa Emre Acer wrote: > Same here. Done. https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_classification.h (right): https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_classification.h:79: // www.example.com ~ example.com -> true On 2015/07/09 17:58:56, Mustafa Emre Acer wrote: > I understand this isn't your comment, but can you mention what the first and the > second domains refer to? Done. https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:156: return 1; On 2015/07/09 17:58:56, Mustafa Emre Acer wrote: > |return true| instead of 1. Actually, just remove this function and enable the > check by default. It'll only be enabled on canary and dev for a few days until > you land the finch CL. Done. https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:165: DCHECK(!dns_names.empty()); On 2015/07/09 17:58:56, Mustafa Emre Acer wrote: > I feel that dns_names can actually be empty, so not sure if this check is valid. |dns_names| contains the DNS names in SAN field or common name in the subject field. Can there be a certificate with no common name (CN)? https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:290: ShowSSLInterstitial(results.new_url); On 2015/07/09 17:58:56, Mustafa Emre Acer wrote: > For this CL, let's just record the result in a histogram and call > ShowSSLInterstitial without a suggested URL. You can then add the UI logic once > the design is finalized. That means you don't have to keep around a > suggested_url for this CL (if the tests turn out to be difficult to write > because of this, let me know and we can reconsider) > > This is going to slightly affect how many captive portal interstitials we > display since this CL is giving priority to common name mismatches, but I expect > the difference to be minimal so it's fine. Acknowledged. https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.h (right): https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:65: // This method gets the result of whether the suggested url is valid. Displays On 2015/07/09 17:58:56, Mustafa Emre Acer wrote: > nit: Drop "This method " Done. https://codereview.chromium.org/1223233002/diff/40001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.h:111: const int common_name_handler_delay_in_seconds = 3; On 2015/07/09 17:58:56, Mustafa Emre Acer wrote: > You'll need a non-const value for this so that you can change it in tests. I > think you should just reuse the existing methods and variables for the captive > portal check: (GetInterstitialDisplayDelay and SetInterstitialDelayTypeForTest) Done.
Added Unit Tests. Please let me know if these cover all the cases. Thanks.
https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/comm... File chrome/browser/ssl/common_name_mismatch_handler.cc (right): https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.cc:75: Remove extra line https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.cc:92: replacements.SetHostStr(www_mismatch_host_name); You might want to document that you are pinging the full URL with path and query params, and not just the hostname. https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/comm... File chrome/browser/ssl/common_name_mismatch_handler.h (right): https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.h:31: RESULT_SUGGESTED_URL_INVALID nit: rename to AVAILABLE/NOT_AVAILABLE instead of VALID/INVALID You need to change the comments and test names as well. https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.h:38: GURL new_url; suggested_url? https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.h:56: static bool GetSuggestedUrl(const GURL& request_url, I'd just return a GURL() here, with empty GURL meaning no suggested URL. https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:168: g_timer_started_callback->Run(web_contents_); nit: Add a comment here explanining why we don't need to do captive portal check if there is a common name mismatch. https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:63: const GURL new_url) { const GURL& https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:86: void SetSuggestedUrl() { suggested_url_exists_ = true; } I suggest changing this to SetSuggestedUrlExists(bool). See my second comment in ShouldNotCheckSuggestedUrlIfGetSuggestedUrlIsFalse. https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:97: int suggested_url_checked() const { return suggested_url_checked_; } bool return type https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:99: int common_name_mismatch_interstitial_shown() const { bool return type https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:135: bool suggested_url_checked_; nit: Put these in opposite order here in other places, so that it's a bit clearer which one is supposed to be true first: bool suggested_url_checked_; bool suggested_url_exists_; https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:153: reinterpret_cast<const char*>(google_der), sizeof(google_der)); Do you need an actual cert here? If not, you can create an empty one: ssl_info_.cert = new net::X509Certificate( "bad.host", "CA", base::Time::Max(), base::Time::Max()); https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:242: ShouldNotCheckSuggestedUrlIfGetSuggestedUrlIsFalse) { nit: name this ShouldNotCheckSuggestedUrlIfNoSuggestedUrl? https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:244: error_handler()->StartHandlingError(); Looks like you are using |SetSuggestedURL| to enable suggested url. How about adding a bool param to |SetSuggestedURL| and set it to false here so that it's clearer for the reader what to expect? https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:250: EXPECT_TRUE(error_handler()->ssl_interstitial_shown()); EXPECT_FALSE(error_handler()->IsTimerRunning()); before this. https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:254: ShouldNotCheckForCaptivePortalIfSuggestedUrlExists) { nit: This name uses CheckFor while the previous one uses Check. Choose one and be consistent. https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:264: EXPECT_TRUE(error_handler()->ssl_interstitial_shown()); EXPECT_FALSE(error_handler()->IsTimerRunning()); before this line https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:309: // Fake a Valid Suggested URL Check result. nit: don't capitalize "Valid Suggested URL Check" https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:313: GURL("https://random.example.com")); indent 4 more spaces https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:313: GURL("https://random.example.com")); Why is this URL different than the return value of GetSuggestedUrl? (www.example.com) https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:333: GURL()); indent 4 more spaces
On 2015/07/15 20:11:47, Mustafa Emre Acer wrote: > https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/comm... > File chrome/browser/ssl/common_name_mismatch_handler.cc (right): > > https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/comm... > chrome/browser/ssl/common_name_mismatch_handler.cc:75: > Remove extra line > > https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/comm... > chrome/browser/ssl/common_name_mismatch_handler.cc:92: > replacements.SetHostStr(www_mismatch_host_name); > You might want to document that you are pinging the full URL with path and query > params, and not just the hostname. > > https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/comm... > File chrome/browser/ssl/common_name_mismatch_handler.h (right): > > https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/comm... > chrome/browser/ssl/common_name_mismatch_handler.h:31: > RESULT_SUGGESTED_URL_INVALID > nit: rename to AVAILABLE/NOT_AVAILABLE instead of VALID/INVALID > > You need to change the comments and test names as well. > > https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/comm... > chrome/browser/ssl/common_name_mismatch_handler.h:38: GURL new_url; > suggested_url? > > https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/comm... > chrome/browser/ssl/common_name_mismatch_handler.h:56: static bool > GetSuggestedUrl(const GURL& request_url, > I'd just return a GURL() here, with empty GURL meaning no suggested URL. > > https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... > File chrome/browser/ssl/ssl_error_handler.cc (right): > > https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_handler.cc:168: > g_timer_started_callback->Run(web_contents_); > nit: Add a comment here explanining why we don't need to do captive portal check > if there is a common name mismatch. > > https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... > File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): > > https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_handler_unittest.cc:63: const GURL new_url) { > const GURL& > > https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_handler_unittest.cc:86: void SetSuggestedUrl() { > suggested_url_exists_ = true; } > I suggest changing this to SetSuggestedUrlExists(bool). See my second comment in > ShouldNotCheckSuggestedUrlIfGetSuggestedUrlIsFalse. > > https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_handler_unittest.cc:97: int suggested_url_checked() > const { return suggested_url_checked_; } > bool return type > > https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_handler_unittest.cc:99: int > common_name_mismatch_interstitial_shown() const { > bool return type > > https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_handler_unittest.cc:135: bool > suggested_url_checked_; > nit: Put these in opposite order here in other places, so that it's a bit > clearer which one is supposed to be true first: > > bool suggested_url_checked_; > bool suggested_url_exists_; > > https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_handler_unittest.cc:153: reinterpret_cast<const > char*>(google_der), sizeof(google_der)); > Do you need an actual cert here? If not, you can create an empty one: > > ssl_info_.cert = new net::X509Certificate( > "bad.host", "CA", base::Time::Max(), base::Time::Max()); > > https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_handler_unittest.cc:242: > ShouldNotCheckSuggestedUrlIfGetSuggestedUrlIsFalse) { > nit: name this ShouldNotCheckSuggestedUrlIfNoSuggestedUrl? > > https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_handler_unittest.cc:244: > error_handler()->StartHandlingError(); > Looks like you are using |SetSuggestedURL| to enable suggested url. How about > adding a bool param to |SetSuggestedURL| and set it to false here so that it's > clearer for the reader what to expect? > > https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_handler_unittest.cc:250: > EXPECT_TRUE(error_handler()->ssl_interstitial_shown()); > EXPECT_FALSE(error_handler()->IsTimerRunning()); before this. > > https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_handler_unittest.cc:254: > ShouldNotCheckForCaptivePortalIfSuggestedUrlExists) { > nit: This name uses CheckFor while the previous one uses Check. Choose one and > be consistent. > > https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_handler_unittest.cc:264: > EXPECT_TRUE(error_handler()->ssl_interstitial_shown()); > EXPECT_FALSE(error_handler()->IsTimerRunning()); before this line > > https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_handler_unittest.cc:309: // Fake a Valid Suggested > URL Check result. > nit: don't capitalize "Valid Suggested URL Check" > > https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_handler_unittest.cc:313: > GURL("https://random.example.com")); > indent 4 more spaces > > https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_handler_unittest.cc:313: > GURL("https://random.example.com")); > Why is this URL different than the return value of GetSuggestedUrl? > (http://www.example.com) > > https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... > chrome/browser/ssl/ssl_error_handler_unittest.cc:333: GURL()); > indent 4 more spaces Unit tests look good. I added a couple of other comments (mostly nits).
https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/comm... File chrome/browser/ssl/common_name_mismatch_handler.h (right): https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.h:50: const CheckUrlCallback& CheckUrlcallback); Nit: Name the argument |checkUrlCallback| (note capitalization) or, more simply, |callback|. https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.h:52: // Returns true if the error can be handled. Interacts with Nit: Don't use the passive voice here ("...be handled"), because it lets you leave out the actor (i.e. who is doing the handling). Say who does the handling, and/or use a verb more specific than "handle". E.g. "Returns true if any other DNS names in the certificate's subject alternative names list closely match the original request hostname and if those other names are available and serving valid certificates." https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.h:56: static bool GetSuggestedUrl(const GURL& request_url, > I'd just return a GURL() here, with empty GURL meaning no suggested URL. I agree.
https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/comm... File chrome/browser/ssl/common_name_mismatch_handler.cc (right): https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.cc:75: On 2015/07/15 20:11:46, Mustafa Emre Acer wrote: > Remove extra line Done. https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.cc:92: replacements.SetHostStr(www_mismatch_host_name); On 2015/07/15 20:11:45, Mustafa Emre Acer wrote: > You might want to document that you are pinging the full URL with path and query > params, and not just the hostname. Done. https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/comm... File chrome/browser/ssl/common_name_mismatch_handler.h (right): https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.h:31: RESULT_SUGGESTED_URL_INVALID On 2015/07/15 20:11:46, Mustafa Emre Acer wrote: > nit: rename to AVAILABLE/NOT_AVAILABLE instead of VALID/INVALID > > You need to change the comments and test names as well. Using AVAILABLE/NOT_AVAILABLE is a good idea, I am also thinking there would be more confusion between |suggested_url_exists| (to say that a suggested url exists and can be checked if its valid) and |suggested_url_available| than with |suggested_url_valid|. https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.h:38: GURL new_url; On 2015/07/15 20:11:46, Mustafa Emre Acer wrote: > suggested_url? If there is a redirect, |new_url| can be different from that of |suggested_url|. So, used a different name here. https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.h:50: const CheckUrlCallback& CheckUrlcallback); On 2015/07/16 22:55:32, palmer wrote: > Nit: Name the argument |checkUrlCallback| (note capitalization) or, more simply, > |callback|. Done. https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.h:52: // Returns true if the error can be handled. Interacts with On 2015/07/16 22:55:32, palmer wrote: > Nit: Don't use the passive voice here ("...be handled"), because it lets you > leave out the actor (i.e. who is doing the handling). Say who does the handling, > and/or use a verb more specific than "handle". > > E.g. "Returns true if any other DNS names in the certificate's subject > alternative names list closely match the original request hostname and if those > other names are available and serving valid certificates." Done. I just now realized that I use a lot of passive voice in documentation. Thanks. I will take care of it, from now on. https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.h:56: static bool GetSuggestedUrl(const GURL& request_url, On 2015/07/15 20:11:46, Mustafa Emre Acer wrote: > I'd just return a GURL() here, with empty GURL meaning no suggested URL. I thought that, returning bool makes it easy to check if suggested_url exists. Like using |if(GetSuggestedUrl(...))| would be easier in this case. Please let me know if you think the other case would be better. Thanks. https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:168: g_timer_started_callback->Run(web_contents_); On 2015/07/15 20:11:46, Mustafa Emre Acer wrote: > nit: Add a comment here explanining why we don't need to do captive portal check > if there is a common name mismatch. Done. https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:63: const GURL new_url) { On 2015/07/15 20:11:46, Mustafa Emre Acer wrote: > const GURL& Done. https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:86: void SetSuggestedUrl() { suggested_url_exists_ = true; } On 2015/07/15 20:11:47, Mustafa Emre Acer wrote: > I suggest changing this to SetSuggestedUrlExists(bool). See my second comment in > ShouldNotCheckSuggestedUrlIfGetSuggestedUrlIsFalse. Done. https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:97: int suggested_url_checked() const { return suggested_url_checked_; } On 2015/07/15 20:11:46, Mustafa Emre Acer wrote: > bool return type Done. https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:99: int common_name_mismatch_interstitial_shown() const { On 2015/07/15 20:11:46, Mustafa Emre Acer wrote: > bool return type Done. https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:135: bool suggested_url_checked_; On 2015/07/15 20:11:46, Mustafa Emre Acer wrote: > nit: Put these in opposite order here in other places, so that it's a bit > clearer which one is supposed to be true first: > > bool suggested_url_checked_; > bool suggested_url_exists_; If the suggested URL exists, we check for the validity of it. so, the order is correct here. https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:153: reinterpret_cast<const char*>(google_der), sizeof(google_der)); On 2015/07/15 20:11:46, Mustafa Emre Acer wrote: > Do you need an actual cert here? If not, you can create an empty one: > > ssl_info_.cert = new net::X509Certificate( > "bad.host", "CA", base::Time::Max(), base::Time::Max()); |SSLErrorHandler| calls method like |GetDNSNames| on the |X509Certificate| cert. class. These methods need an actual certificate and are giving a Segmentation Fault when I use an empty one. https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:242: ShouldNotCheckSuggestedUrlIfGetSuggestedUrlIsFalse) { On 2015/07/15 20:11:46, Mustafa Emre Acer wrote: > nit: name this ShouldNotCheckSuggestedUrlIfNoSuggestedUrl? Done. https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:244: error_handler()->StartHandlingError(); On 2015/07/15 20:11:46, Mustafa Emre Acer wrote: > Looks like you are using |SetSuggestedURL| to enable suggested url. How about > adding a bool param to |SetSuggestedURL| and set it to false here so that it's > clearer for the reader what to expect? It is good to add a bool param to |SetSuggestedURL|. Done. https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:250: EXPECT_TRUE(error_handler()->ssl_interstitial_shown()); On 2015/07/15 20:11:46, Mustafa Emre Acer wrote: > EXPECT_FALSE(error_handler()->IsTimerRunning()); before this. Done. https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:254: ShouldNotCheckForCaptivePortalIfSuggestedUrlExists) { On 2015/07/15 20:11:46, Mustafa Emre Acer wrote: > nit: This name uses CheckFor while the previous one uses Check. Choose one and > be consistent. Done. https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:264: EXPECT_TRUE(error_handler()->ssl_interstitial_shown()); On 2015/07/15 20:11:47, Mustafa Emre Acer wrote: > EXPECT_FALSE(error_handler()->IsTimerRunning()); before this line Done. https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:309: // Fake a Valid Suggested URL Check result. On 2015/07/15 20:11:46, Mustafa Emre Acer wrote: > nit: don't capitalize "Valid Suggested URL Check" Done. https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:313: GURL("https://random.example.com")); On 2015/07/15 20:11:46, Mustafa Emre Acer wrote: > Why is this URL different than the return value of GetSuggestedUrl? > (http://www.example.com) This URL is the new landing page and this can be different from the return value of |GetSuggestedUrl|, if there is a redirect. So, I changed it. I added a comment, let me know if you think its good to have the same URL here. https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:313: GURL("https://random.example.com")); On 2015/07/15 20:11:46, Mustafa Emre Acer wrote: > indent 4 more spaces Done. https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:313: GURL("https://random.example.com")); On 2015/07/15 20:11:46, Mustafa Emre Acer wrote: > indent 4 more spaces I tried to indent it, but |git cl format| is reverting the change. Here, |CommonNameMismatchHandler::SuggestedUrlCheckResult::RESULT_SUGGESTED_URL_VALID| is a single parameter and |GURL("https://random.example.com")| is the other, so its making them both aligned. https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:333: GURL()); On 2015/07/15 20:11:47, Mustafa Emre Acer wrote: > indent 4 more spaces Done.
https://codereview.chromium.org/1223233002/diff/120001/chrome/browser/ssl/com... File chrome/browser/ssl/common_name_mismatch_handler.cc (right): https://codereview.chromium.org/1223233002/diff/120001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.cc:71: const GURL landing_url = url_fetcher->GetURL(); This could be a reference, to avoid an unnecessary copy. https://codereview.chromium.org/1223233002/diff/120001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/1223233002/diff/120001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_blocking_page.cc:388: // If suggested url is not empty, display a modified message Nit: URL https://codereview.chromium.org/1223233002/diff/120001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_blocking_page.h (right): https://codereview.chromium.org/1223233002/diff/120001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_blocking_page.h:136: // Guide the user to this url, when there is a common name mismatch error Nit: URL https://codereview.chromium.org/1223233002/diff/120001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/1223233002/diff/120001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_classification.cc:355: std::string host_name = request_url_.host(); Use a const std::string& here, for const-correctness and to avoid a copy. https://codereview.chromium.org/1223233002/diff/120001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler.h (right): https://codereview.chromium.org/1223233002/diff/120001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.h:65: // Gets the result of whether the suggested url is valid. Displays Nit: URL, SSL. In general, make sure all your acronyms/initialisms are capitalized in comments (even though we don't do it in identifiers).
Wrote browser tests. Added code to check for www mismatch if |CERT_COMMON_NAME_INVALID| is the only error found. https://codereview.chromium.org/1223233002/diff/120001/chrome/browser/ssl/com... File chrome/browser/ssl/common_name_mismatch_handler.cc (right): https://codereview.chromium.org/1223233002/diff/120001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.cc:71: const GURL landing_url = url_fetcher->GetURL(); On 2015/07/17 00:15:59, palmer wrote: > This could be a reference, to avoid an unnecessary copy. Done. https://codereview.chromium.org/1223233002/diff/120001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/1223233002/diff/120001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_blocking_page.cc:388: // If suggested url is not empty, display a modified message On 2015/07/17 00:16:00, palmer wrote: > Nit: URL Done. https://codereview.chromium.org/1223233002/diff/120001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_blocking_page.h (right): https://codereview.chromium.org/1223233002/diff/120001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_blocking_page.h:136: // Guide the user to this url, when there is a common name mismatch error On 2015/07/17 00:16:00, palmer wrote: > Nit: URL Done. https://codereview.chromium.org/1223233002/diff/120001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/1223233002/diff/120001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_classification.cc:355: std::string host_name = request_url_.host(); On 2015/07/17 00:16:00, palmer wrote: > Use a const std::string& here, for const-correctness and to avoid a copy. Done. https://codereview.chromium.org/1223233002/diff/120001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler.h (right): https://codereview.chromium.org/1223233002/diff/120001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.h:65: // Gets the result of whether the suggested url is valid. Displays On 2015/07/17 00:16:00, palmer wrote: > Nit: URL, SSL. In general, make sure all your acronyms/initialisms are > capitalized in comments (even though we don't do it in identifiers). Done. https://codereview.chromium.org/1223233002/diff/140001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/1223233002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_classification.cc:273: return 1; I wrote this while I was testing. Will remove it in the next patch.
https://codereview.chromium.org/1223233002/diff/140001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/1223233002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_classification.cc:273: return 1; On 2015/07/23 20:11:06, Bhanu Dev wrote: > I wrote this while I was testing. Will remove it in the next patch. Can you remove it, do a |git cl format|, reupload the patch and do a |git cl try|? (just git cl try if you already formatted it)
https://codereview.chromium.org/1223233002/diff/140001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/1223233002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_classification.cc:273: return 1; On 2015/07/23 20:12:55, Mustafa Emre Acer wrote: > On 2015/07/23 20:11:06, Bhanu Dev wrote: > > I wrote this while I was testing. Will remove it in the next patch. > > Can you remove it, do a |git cl format|, reupload the patch and do a |git cl > try|? (just git cl try if you already formatted it) Done.
LGTM with nits. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/com... File chrome/browser/ssl/common_name_mismatch_handler.cc (right): https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.cc:40: // The first 0 means this can use a TestURLFetcherFactory in unit tests. Nit: It *might* be more idiomatic to leave this comment out, and instead say: url_fetcher_ = net::URLFetcher::Create(0 /* testing ID */, url, net::URLFetcher::HEAD, this); https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler.cc (left): https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.cc:51: #if defined(ENABLE_CAPTIVE_PORTAL_DETECTION) Did you intend to remove this? https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.cc:53: // - If a "captive portal detected" or "suggested url valid" result Nit: URL https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler.h (right): https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.h:114: // Timer duration for common name mismatch handler url check. Nit: URL https://codereview.chromium.org/1223233002/diff/160001/net/test/spawned_test_... File net/test/spawned_test_server/base_test_server.h (right): https://codereview.chromium.org/1223233002/diff/160001/net/test/spawned_test_... net/test/spawned_test_server/base_test_server.h:70: // 127.0.0.1 as its host name. Do you mean "...127.0.0.1 as its IP address"? The hostname is surely "example.com"? https://codereview.chromium.org/1223233002/diff/160001/net/test/spawned_test_... net/test/spawned_test_server/base_test_server.h:74: // has 127.0.0.1 as its host name. As above.
Looking good. I have some comments here and there, but I'm particularly curious why you are using a different timeout here than captive portals. If it's 3 sec for common name mismatch, we can/should make the other one 3 sec as well. https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/comm... File chrome/browser/ssl/common_name_mismatch_handler.h (right): https://codereview.chromium.org/1223233002/diff/80001/chrome/browser/ssl/comm... chrome/browser/ssl/common_name_mismatch_handler.h:56: static bool GetSuggestedUrl(const GURL& request_url, On 2015/07/16 23:38:06, Bhanu Dev wrote: > On 2015/07/15 20:11:46, Mustafa Emre Acer wrote: > > I'd just return a GURL() here, with empty GURL meaning no suggested URL. > > I thought that, returning bool makes it easy to check if suggested_url exists. > Like using |if(GetSuggestedUrl(...))| would be easier in this case. > > Please let me know if you think the other case would be better. Thanks. I think it's one less dimension you'll need to worry about when you just return a GURL. I always get confused thinking about possible combinations (4 in your case vs 2 when returning GURL). https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/security_warnings/interstitial_v2.js (right): https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/resourc... chrome/browser/resources/security_warnings/interstitial_v2.js:29: var CMD_NAVIGATE_SUGGESTED_URL = 12; nit: CMD_OPEN_SUGGESTED_URL instead, to be consistent with the previous enum values https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/resourc... chrome/browser/resources/security_warnings/interstitial_v2.js:94: var commonNameMismatchInterstitial = loadTimeData.getBoolean( Could also check for |ssl| here: var commonNameMismatchInterstitial = ssl && loadTimeData.getBoolean(...) https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/resourc... chrome/browser/resources/security_warnings/interstitial_v2.js:95: 'common_name_mismatch_interstitial'); nit: common_name_mismatch instead of common_name_mismatch_interstitial. (bad_clock doesn't have it) https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/resourc... chrome/browser/resources/security_warnings/interstitial_v2.js:186: } Move this block above |if (captivePortal)| since it's SSL related. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/resourc... chrome/browser/resources/security_warnings/interstitial_v2.js:188: Remove extra line https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/com... File chrome/browser/ssl/common_name_mismatch_handler.cc (right): https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.cc:24: CommonNameMismatchHandler::TestingState Comment with "// static" in the previous line https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.cc:69: // URL, and returns a CaptivePortalService::Result based on its result. The comment is referring to captive portals :) https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.cc:85: landing_url.host() != request_url_.host()) { I suppose we are OK with redirects to other origins here? E.g. www.example.com pings example.com which redirects to foo.com. The user will end up on foo.com. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.cc:97: std::string www_mismatch_host_name; nit: www_mismatch_hostname? https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.cc:102: // The full URL should be pinged, not just the new host name. So, get the host name -> hostname here and other places https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/com... File chrome/browser/ssl/common_name_mismatch_handler.h (right): https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.h:10: #include "base/compiler_specific.h" Why do you need this? https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.h:30: // Suggested URL is invalid nit: Period at the end of comment https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.h:30: // Suggested URL is invalid I still think these should be named AVAILABLE/NOT_AVAILABLE, but I'll leave it up to you. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.h:49: explicit CommonNameMismatchHandler( No need for explicit with a constructor with more than one argument. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.h:66: static TestingState testing_state_; Make this private and add a getter/setter. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.h:80: bool CheckingSuggestedUrl() const; Move this above request_url. Keep methods and members grouped together. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_blocking_page.cc:388: load_time_data->SetBoolean("common_name_mismatch_interstitial", false); nit: Drop _interstitial https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_blocking_page.cc:664: load_params.transition_type = ui::PAGE_TRANSITION_TYPED; TYPED is used for when the user explicitly types the URL which is not the case when they click the link. I think you need PAGE_TRANSITION_LINK instead. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_blocking_page.h (right): https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_blocking_page.h:110: void NavigateToSuggestedURL() const; nit: OpenSuggestedURL? Also, maybe add a comment. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2256: Remove empty line. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2266: // link displayed. the id of the link element displayed https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2274: IN_PROC_BROWSER_TEST_F(SSLUITest, CheckWWWSubdomainMismatch) { The naming of this test could be a bit more descriptive. How about something like |ShouldShowWWWSubdomainMismatchInterstitial| https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2281: GURL https_server_url = Maybe comment that the path does not matter https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2295: EXPECT_TRUE( Might want to check the interstitial type here: EXPECT_EQ(SSLBlockingPage::kTypeForTest, contents->GetInterstitialPage()->GetDelegate()->GetTypeForTesting()); or something like that (here and elsewhere) https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2303: contents->GetInterstitialPage(), "suggest-link")); These three asserts are repeated a couple times. You can put them into a helper method. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2344: CommonNameMismatchInterstitialNavigateToSuggestedURL) { Does this need to be a separate test? Can you not continue with clicking in the previous test? https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2440: // - |CommonNameMismatchHandler| does not give a callback as its set into the its -> it's https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2472: nit: Remove blank line https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2510: Remove blank line https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2546: Remove blank line https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2551: // |observer| waits for one navigation to finish. comment not necessary https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.cc:43: SSL_ERROR_HANDLER_EVENT_COUNT I think you should add an UMA event for the new interstitial as well (well two, overridable + non-overridable) https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.cc:168: // similarity check. This is a strong assertion, isn't it? GoogleWifi can serve a valid wifi.google.com cert for google.com (ignoring it's pinned and all that). Maybe reword it and say "...most captive portals don't serve..." https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.cc:180: Profile::FromBrowserContext(web_contents_->GetBrowserContext()); This is called 4 times in this class. Perhaps you could make it a member variable (or just a helper method)? https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.cc:202: bool SSLErrorHandler::GetSuggestedUrl(const GURL& request_url, |request_url| parameter isn't used. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.cc:218: CommonNameMismatchHandler::TestingState::NOT_TESTING; This looks very odd. Why do you need to change the testing state in the middle of the suggested url check? https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler.h (right): https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.h:100: GURL* suggested_url); const method https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.h:101: virtual void CheckSuggestedUrl(const GURL& suggested_url); const method? https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.h:115: const int common_name_handler_delay_in_seconds = 3; Why is this different than the delay for captive portal? https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler_unittest.cc:86: void SetSuggestedUrl(bool suggested_url_exists) { SetSuggestedUrl -> SetSuggestedUrlExists
https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/security_warnings/interstitial_v2.js (right): https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/resourc... chrome/browser/resources/security_warnings/interstitial_v2.js:29: var CMD_NAVIGATE_SUGGESTED_URL = 12; On 2015/07/28 01:18:05, Mustafa Emre Acer wrote: > nit: CMD_OPEN_SUGGESTED_URL instead, to be consistent with the previous enum > values Done. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/resourc... chrome/browser/resources/security_warnings/interstitial_v2.js:94: var commonNameMismatchInterstitial = loadTimeData.getBoolean( On 2015/07/28 01:18:05, Mustafa Emre Acer wrote: > Could also check for |ssl| here: > > var commonNameMismatchInterstitial = ssl && loadTimeData.getBoolean(...) Done. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/resourc... chrome/browser/resources/security_warnings/interstitial_v2.js:95: 'common_name_mismatch_interstitial'); On 2015/07/28 01:18:05, Mustafa Emre Acer wrote: > nit: common_name_mismatch instead of common_name_mismatch_interstitial. > (bad_clock doesn't have it) Done. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/resourc... chrome/browser/resources/security_warnings/interstitial_v2.js:186: } On 2015/07/28 01:18:05, Mustafa Emre Acer wrote: > Move this block above |if (captivePortal)| since it's SSL related. Done. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/resourc... chrome/browser/resources/security_warnings/interstitial_v2.js:188: On 2015/07/28 01:18:04, Mustafa Emre Acer wrote: > Remove extra line Done. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/com... File chrome/browser/ssl/common_name_mismatch_handler.cc (right): https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.cc:24: CommonNameMismatchHandler::TestingState On 2015/07/28 01:18:05, Mustafa Emre Acer wrote: > Comment with "// static" in the previous line Done. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.cc:40: // The first 0 means this can use a TestURLFetcherFactory in unit tests. On 2015/07/27 23:56:18, palmer wrote: > Nit: It *might* be more idiomatic to leave this comment out, and instead say: > > url_fetcher_ = net::URLFetcher::Create(0 /* testing ID */, url, > net::URLFetcher::HEAD, this); Done. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.cc:69: // URL, and returns a CaptivePortalService::Result based on its result. On 2015/07/28 01:18:05, Mustafa Emre Acer wrote: > The comment is referring to captive portals :) Copied it from captive portal code :P . Changing the comment. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.cc:85: landing_url.host() != request_url_.host()) { On 2015/07/28 01:18:05, Mustafa Emre Acer wrote: > I suppose we are OK with redirects to other origins here? E.g. http://www.example.com > pings http://example.com which redirects to http://foo.com. The user will end up on http://foo.com. I think it is OK. But, I do not know how the user is conveyed of this, as the info bar does not display where he is redirected to. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.cc:97: std::string www_mismatch_host_name; On 2015/07/28 01:18:05, Mustafa Emre Acer wrote: > nit: www_mismatch_hostname? Done. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.cc:102: // The full URL should be pinged, not just the new host name. So, get the On 2015/07/28 01:18:05, Mustafa Emre Acer wrote: > host name -> hostname here and other places Done. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/com... File chrome/browser/ssl/common_name_mismatch_handler.h (right): https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.h:10: #include "base/compiler_specific.h" On 2015/07/28 01:18:06, Mustafa Emre Acer wrote: > Why do you need this? Done. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.h:30: // Suggested URL is invalid On 2015/07/28 01:18:05, Mustafa Emre Acer wrote: > I still think these should be named AVAILABLE/NOT_AVAILABLE, but I'll leave it > up to you. Done. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.h:30: // Suggested URL is invalid On 2015/07/28 01:18:05, Mustafa Emre Acer wrote: > nit: Period at the end of comment Done. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.h:49: explicit CommonNameMismatchHandler( On 2015/07/28 01:18:06, Mustafa Emre Acer wrote: > No need for explicit with a constructor with more than one argument. Done. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.h:66: static TestingState testing_state_; On 2015/07/28 01:18:06, Mustafa Emre Acer wrote: > Make this private and add a getter/setter. Done. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.h:80: bool CheckingSuggestedUrl() const; On 2015/07/28 01:18:06, Mustafa Emre Acer wrote: > Move this above request_url. Keep methods and members grouped together. Done. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_blocking_page.cc:388: load_time_data->SetBoolean("common_name_mismatch_interstitial", false); On 2015/07/28 01:18:06, Mustafa Emre Acer wrote: > nit: Drop _interstitial Done. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_blocking_page.cc:664: load_params.transition_type = ui::PAGE_TRANSITION_TYPED; I too think "PAGE_TRANSITION_LINK" is more appropriate. I used this because captive portal used this at https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/cap... and its documented that TYPED is also used for other explicit navigation actions, at https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/page_trans... Changed this to LINK, as the user clicked on the link. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_blocking_page.h (right): https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_blocking_page.h:110: void NavigateToSuggestedURL() const; On 2015/07/28 01:18:06, Mustafa Emre Acer wrote: > nit: OpenSuggestedURL? Also, maybe add a comment. Done. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2256: On 2015/07/28 01:18:06, Mustafa Emre Acer wrote: > Remove empty line. Done. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2266: // link displayed. On 2015/07/28 01:18:06, Mustafa Emre Acer wrote: > the id of the link element displayed Done. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2274: IN_PROC_BROWSER_TEST_F(SSLUITest, CheckWWWSubdomainMismatch) { On 2015/07/28 01:18:06, Mustafa Emre Acer wrote: > The naming of this test could be a bit more descriptive. How about something > like |ShouldShowWWWSubdomainMismatchInterstitial| That sounds better. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2281: GURL https_server_url = On 2015/07/28 01:18:06, Mustafa Emre Acer wrote: > Maybe comment that the path does not matter Done. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2295: EXPECT_TRUE( On 2015/07/28 01:18:06, Mustafa Emre Acer wrote: > Might want to check the interstitial type here: > > EXPECT_EQ(SSLBlockingPage::kTypeForTest, > contents->GetInterstitialPage()->GetDelegate()->GetTypeForTesting()); > > or something like that (here and elsewhere) Done. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2344: CommonNameMismatchInterstitialNavigateToSuggestedURL) { On 2015/07/28 01:18:06, Mustafa Emre Acer wrote: > Does this need to be a separate test? Can you not continue with clicking in the > previous test? Yes, that would be better. I am removing this test. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2440: // - |CommonNameMismatchHandler| does not give a callback as its set into the On 2015/07/28 01:18:06, Mustafa Emre Acer wrote: > its -> it's Done. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2472: On 2015/07/28 01:18:06, Mustafa Emre Acer wrote: > nit: Remove blank line Done. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2510: On 2015/07/28 01:18:07, Mustafa Emre Acer wrote: > Remove blank line Done. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2546: On 2015/07/28 01:18:06, Mustafa Emre Acer wrote: > Remove blank line Done. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2551: // |observer| waits for one navigation to finish. On 2015/07/28 01:18:06, Mustafa Emre Acer wrote: > comment not necessary Done. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.cc:43: SSL_ERROR_HANDLER_EVENT_COUNT On 2015/07/28 01:18:07, Mustafa Emre Acer wrote: > I think you should add an UMA event for the new interstitial as well (well two, > overridable + non-overridable) Done. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.cc:53: // - If a "captive portal detected" or "suggested url valid" result On 2015/07/27 23:56:18, palmer wrote: > Nit: URL Done. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.cc:168: // similarity check. On 2015/07/28 01:18:07, Mustafa Emre Acer wrote: > This is a strong assertion, isn't it? GoogleWifi can serve a valid > http://wifi.google.com cert for http://google.com (ignoring it's pinned and all that). Maybe > reword it and say "...most captive portals don't serve..." Yes, your'e right. I've changed it. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.cc:202: bool SSLErrorHandler::GetSuggestedUrl(const GURL& request_url, On 2015/07/28 01:18:07, Mustafa Emre Acer wrote: > |request_url| parameter isn't used. Done. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.cc:218: CommonNameMismatchHandler::TestingState::NOT_TESTING; On 2015/07/28 01:18:07, Mustafa Emre Acer wrote: > This looks very odd. Why do you need to change the testing state in the middle > of the suggested url check? I was trying to make sure that the |testing_state| is set to |NOT_TESTING|, and not changed by some tests (Tests are independent from chrome, but I just added this.) Removed it now.. Is an assertion needed here instead? https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler.h (right): https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.h:100: GURL* suggested_url); On 2015/07/28 01:18:07, Mustafa Emre Acer wrote: > const method Done. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.h:101: virtual void CheckSuggestedUrl(const GURL& suggested_url); On 2015/07/28 01:18:07, Mustafa Emre Acer wrote: > const method? This method cannot be made const as this changes the value of |common_name_mismatch_handler_| https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.h:114: // Timer duration for common name mismatch handler url check. On 2015/07/27 23:56:18, palmer wrote: > Nit: URL Done. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.h:115: const int common_name_handler_delay_in_seconds = 3; On 2015/07/28 01:18:07, Mustafa Emre Acer wrote: > Why is this different than the delay for captive portal? Done. https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler_unittest.cc:86: void SetSuggestedUrl(bool suggested_url_exists) { On 2015/07/28 01:18:07, Mustafa Emre Acer wrote: > SetSuggestedUrl -> SetSuggestedUrlExists Done. https://codereview.chromium.org/1223233002/diff/160001/net/test/spawned_test_... File net/test/spawned_test_server/base_test_server.h (right): https://codereview.chromium.org/1223233002/diff/160001/net/test/spawned_test_... net/test/spawned_test_server/base_test_server.h:70: // 127.0.0.1 as its host name. On 2015/07/27 23:56:18, palmer wrote: > Do you mean "...127.0.0.1 as its IP address"? The hostname is surely > "example.com"? The server's actual hostname is "127.0.0.1". A |MockHostResolver| is used to map example.com to 127.0.0.1 I am removing that part of the comment, as 127.0.0.1 is the default ip address used. https://codereview.chromium.org/1223233002/diff/160001/net/test/spawned_test_... net/test/spawned_test_server/base_test_server.h:74: // has 127.0.0.1 as its host name. On 2015/07/27 23:56:18, palmer wrote: > As above. Done.
Lgtm modulo comments https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_blocking_page.cc (right): https://codereview.chromium.org/1223233002/diff/160001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_blocking_page.cc:664: load_params.transition_type = ui::PAGE_TRANSITION_TYPED; On 2015/07/30 02:39:10, Bhanu Dev wrote: > I too think "PAGE_TRANSITION_LINK" is more appropriate. I used this because > captive portal used this at > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/cap... > and its documented that TYPED is also used for other explicit navigation > actions, at > https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/page_trans... > > Changed this to LINK, as the user clicked on the link. I see, maybe captive portal uses TYPED because it's opening a new tab. https://codereview.chromium.org/1223233002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/security_warnings/interstitial_v2.js (right): https://codereview.chromium.org/1223233002/diff/180001/chrome/browser/resourc... chrome/browser/resources/security_warnings/interstitial_v2.js:157: // Send a command if user clicks the suggested URL. Do you need this comment? https://codereview.chromium.org/1223233002/diff/180001/chrome/browser/ssl/com... File chrome/browser/ssl/common_name_mismatch_handler.cc (right): https://codereview.chromium.org/1223233002/diff/180001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.cc:24: // Static. lower case, no period at the end: // static https://codereview.chromium.org/1223233002/diff/180001/chrome/browser/ssl/com... File chrome/browser/ssl/common_name_mismatch_handler.h (right): https://codereview.chromium.org/1223233002/diff/180001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.h:85: static TestingState testing_state_; Move this above request_url since it's static https://codereview.chromium.org/1223233002/diff/180001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/1223233002/diff/180001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2274: // and make sure the page navigates to a non SSL page. "non SSL page" isn't correct, suggested url is an SSL page. https://codereview.chromium.org/1223233002/diff/180001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2313: const char kClickConnectButtonJS[] = ConnectButton -> OpenSuggestedButton? https://codereview.chromium.org/1223233002/diff/180001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2315: Remove blank line. Just a single blank line before is sufficient to group code. https://codereview.chromium.org/1223233002/diff/180001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2321: Remove blank line https://codereview.chromium.org/1223233002/diff/180001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler.h (right): https://codereview.chromium.org/1223233002/diff/180001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.h:120: Remove blank line. https://codereview.chromium.org/1223233002/diff/180001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.h:121: Profile* profile_; Can this be const? (either const Profile* or Profile* const, or both)
https://codereview.chromium.org/1223233002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/security_warnings/interstitial_v2.js (right): https://codereview.chromium.org/1223233002/diff/180001/chrome/browser/resourc... chrome/browser/resources/security_warnings/interstitial_v2.js:157: // Send a command if user clicks the suggested URL. On 2015/07/30 19:40:21, Mustafa Emre Acer wrote: > Do you need this comment? Done. https://codereview.chromium.org/1223233002/diff/180001/chrome/browser/ssl/com... File chrome/browser/ssl/common_name_mismatch_handler.cc (right): https://codereview.chromium.org/1223233002/diff/180001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.cc:24: // Static. On 2015/07/30 19:40:21, Mustafa Emre Acer wrote: > lower case, no period at the end: > > // static Done. https://codereview.chromium.org/1223233002/diff/180001/chrome/browser/ssl/com... File chrome/browser/ssl/common_name_mismatch_handler.h (right): https://codereview.chromium.org/1223233002/diff/180001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.h:85: static TestingState testing_state_; On 2015/07/30 19:40:21, Mustafa Emre Acer wrote: > Move this above request_url since it's static Done. https://codereview.chromium.org/1223233002/diff/180001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/1223233002/diff/180001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2274: // and make sure the page navigates to a non SSL page. On 2015/07/30 19:40:21, Mustafa Emre Acer wrote: > "non SSL page" isn't correct, suggested url is an SSL page. Done. https://codereview.chromium.org/1223233002/diff/180001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2313: const char kClickConnectButtonJS[] = On 2015/07/30 19:40:21, Mustafa Emre Acer wrote: > ConnectButton -> OpenSuggestedButton? Done. https://codereview.chromium.org/1223233002/diff/180001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2315: On 2015/07/30 19:40:21, Mustafa Emre Acer wrote: > Remove blank line. Just a single blank line before is sufficient to group code. Done. https://codereview.chromium.org/1223233002/diff/180001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2321: On 2015/07/30 19:40:21, Mustafa Emre Acer wrote: > Remove blank line Done. https://codereview.chromium.org/1223233002/diff/180001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler.h (right): https://codereview.chromium.org/1223233002/diff/180001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.h:120: On 2015/07/30 19:40:22, Mustafa Emre Acer wrote: > Remove blank line. Done. https://codereview.chromium.org/1223233002/diff/180001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.h:121: Profile* profile_; On 2015/07/30 19:40:21, Mustafa Emre Acer wrote: > Can this be const? (either const Profile* or Profile* const, or both) it can be Profile* const profile_
Lgtm
Lgtm
Can you also expand the CL description and describe what it does?
Yes, I'm currently doing that :) Thanks. On Thu, Jul 30, 2015 at 5:14 PM, <meacer@chromium.org> wrote: > Can you also expand the CL description and describe what it does? > > https://codereview.chromium.org/1223233002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
bhanudev@google.com changed reviewers: + jhawkins@chromium.org, mmenke@chromium.org
@mmemke, @jhawkins, For owners review. PTAL. Thanks.
On 2015/07/31 01:13:15, Bhanu Dev wrote: > @mmemke, @jhawkins, > > For owners review. PTAL. Thanks. Please be clear about who owns what - jhawkins, in particular, owns a ton of code, and it looks like you've had an owner of browser/ssl already review this.
On 2015/07/31 02:24:33, mmenke wrote: > On 2015/07/31 01:13:15, Bhanu Dev wrote: > > @mmemke, @jhawkins, > > > > For owners review. PTAL. Thanks. > > Please be clear about who owns what - jhawkins, in particular, owns a ton of > code, and it looks like you've had an owner of browser/ssl already review this. Which owner should review what, rather.
On 2015/07/31 02:24:48, mmenke wrote: > On 2015/07/31 02:24:33, mmenke wrote: > > On 2015/07/31 01:13:15, Bhanu Dev wrote: > > > @mmemke, @jhawkins, > > > > > > For owners review. PTAL. Thanks. > > > > Please be clear about who owns what - jhawkins, in particular, owns a ton of > > code, and it looks like you've had an owner of browser/ssl already review > this. > > Which owner should review what, rather. Apologies, I wasn't clear. @mmenke, I would like you to review chrome/browser/captive_portal/captive_portal_browsertest.cc and code in net/, net/data/ssl/certificates/example_cert.pem net/data/ssl/certificates/www_example_cert.pem net/data/ssl/scripts/ee.cnf net/data/ssl/scripts/generate-test-certs.sh net/test/spawned_test_server/base_test_server.h net/test/spawned_test_server/base_test_server.cc I used a real certificate in captive portal browser test, as using an empty test certificate crashes chrome when |GetDNSNames| method is called. @jhawkins, I would like you to review chrome/browser/resources/security_warnings/interstitial_v2.js chrome/browser/ui/webui/interstitials/interstitial_ui.cc chrome/chrome_browser.gypi
This should be reviewed by a net/ person with some knowledge of the SSL code. Palmer no doubt knows what he's doing, but given that we have net owners who actually know something about SSL, one of them really should be the one reviewing this - davidben, rsleevi, or anyone else with a clue about SSL. https://codereview.chromium.org/1223233002/diff/200001/chrome/browser/captive... File chrome/browser/captive_portal/captive_portal_browsertest.cc (right): https://codereview.chromium.org/1223233002/diff/200001/chrome/browser/captive... chrome/browser/captive_portal/captive_portal_browsertest.cc:349: info.cert = net::X509Certificate::CreateFromBytes( While you're here, mind adding whatever header X509Certificate comes from? https://codereview.chromium.org/1223233002/diff/200001/chrome/browser/captive... chrome/browser/captive_portal/captive_portal_browsertest.cc:350: reinterpret_cast<const char*>(google_der), sizeof(google_der)); google_der is a global constant? Its naming violates the google style guide. HAte to block this on a refactor, but this seems pretty bad to me. https://codereview.chromium.org/1223233002/diff/200001/net/test/spawned_test_... File net/test/spawned_test_server/base_test_server.cc (right): https://codereview.chromium.org/1223233002/diff/200001/net/test/spawned_test_... net/test/spawned_test_server/base_test_server.cc:152: return base::FilePath(FILE_PATH_LITERAL("example_cert.pem")); This file should include file_path.h
bhanudev@google.com changed reviewers: + davidben@chromium.org
@davidben, For owners review. I would like you to review the files in net/, net/data/ssl/certificates/example_cert.pem net/data/ssl/certificates/www_example_cert.pem net/data/ssl/scripts/ee.cnf net/data/ssl/scripts/generate-test-certs.sh net/test/spawned_test_server/base_test_server.h net/test/spawned_test_server/base_test_server.cc Thanks. https://codereview.chromium.org/1223233002/diff/200001/chrome/browser/captive... File chrome/browser/captive_portal/captive_portal_browsertest.cc (right): https://codereview.chromium.org/1223233002/diff/200001/chrome/browser/captive... chrome/browser/captive_portal/captive_portal_browsertest.cc:349: info.cert = net::X509Certificate::CreateFromBytes( On 2015/07/31 15:16:49, mmenke wrote: > While you're here, mind adding whatever header X509Certificate comes from? Done. https://codereview.chromium.org/1223233002/diff/200001/chrome/browser/captive... chrome/browser/captive_portal/captive_portal_browsertest.cc:350: reinterpret_cast<const char*>(google_der), sizeof(google_der)); On 2015/07/31 15:16:49, mmenke wrote: > google_der is a global constant? Its naming violates the google style guide. > HAte to block this on a refactor, but this seems pretty bad to me. I have added a bug for this at crbug.com/515969. But, just saw that they're not global constants.(rsleevi's comment in bug) Thanks. https://codereview.chromium.org/1223233002/diff/200001/net/test/spawned_test_... File net/test/spawned_test_server/base_test_server.cc (right): https://codereview.chromium.org/1223233002/diff/200001/net/test/spawned_test_... net/test/spawned_test_server/base_test_server.cc:152: return base::FilePath(FILE_PATH_LITERAL("example_cert.pem")); On 2015/07/31 15:16:49, mmenke wrote: > This file should include file_path.h Done.
davidben@chromium.org changed reviewers: + rsleevi@chromium.org
Sorry we //net folks keep playing hot potato on this. :-) Bouncing to rsleevi as he's likely to have more opinions than me about our test cert collection.
On 2015/07/31 18:24:12, David Benjamin wrote: > Sorry we //net folks keep playing hot potato on this. :-) Bouncing to rsleevi as > he's likely to have more opinions than me about our test cert collection. Thanks. In this case, I think there is no other way to test the code other than generating a new test certificate.(Please correct me if I'm wrong) I cannot use existing certificates for localhost because, this WWW Mismatch handling is done only when the domain is a known TLD. Not using a real certificate and having something like "mock_cert_verifier()->set_default_result(net::OK);" is always good. I will surely change the code once its implemented.
On 2015/07/31 18:35:06, Bhanu Dev wrote: > On 2015/07/31 18:24:12, David Benjamin wrote: > > Sorry we //net folks keep playing hot potato on this. :-) Bouncing to rsleevi > as > > he's likely to have more opinions than me about our test cert collection. > > Thanks. > > In this case, I think there is no other way to test the code other than > generating a new test certificate.(Please correct me if I'm wrong) > I cannot use existing certificates for localhost because, this WWW Mismatch > handling is done only when the domain is a > known TLD. > > Not using a real certificate and having something like > > "mock_cert_verifier()->set_default_result(net::OK);" > > is always good. I will surely change the code once its implemented. @rsleevi, Can you please take a look at the code in net/. Thanks.
mmenke@chromium.org changed reviewers: - mmenke@chromium.org
mmenke@chromium.org changed reviewers: + mmenke@chromium.org
On 2015/08/03 18:30:24, Bhanu Dev wrote: > @rsleevi, Can you please take a look at the code in net/. I will be focusing on Emily's CL first. I really don't want to see us land more test server test certificates, and solving that will allow us to avoid the code churn / added overhead of reviewing cleanups, since we're close to finishing it.
Browsertests using MockCertVerifier. Removed test certificates generated, related code in net/.
I've taken a look at this, with a focus on testing. On a high-level, I really don't think we should just kick off such URL requests, from the point of view of bandwidth, of privacy, and of power usage. That is, I think Chrome already does too much 'magic under the hood', and we're seeing that the magic is costing us - and, for users in emerging markets, has real, tangible, monetary costs. From a design perspective, I'd love if you and -enamel could iterate and evaluate whether or not the background request is strictly necessary, or whether it still represents a user improvement to simply offer the Suggest URL w/o any background requests. I don't know if I'm stubborn enough to block this CL, but I think it's something that the costs/chance for harm of the background request exceed the value. https://codereview.chromium.org/1223233002/diff/220001/net/test/spawned_test_... File net/test/spawned_test_server/base_test_server.cc (left): https://codereview.chromium.org/1223233002/diff/220001/net/test/spawned_test_... net/test/spawned_test_server/base_test_server.cc:44: Unnecessary change? https://codereview.chromium.org/1223233002/diff/260001/chrome/browser/captive... File chrome/browser/captive_portal/captive_portal_browsertest.cc (right): https://codereview.chromium.org/1223233002/diff/260001/chrome/browser/captive... chrome/browser/captive_portal/captive_portal_browsertest.cc:351: reinterpret_cast<const char*>(google_der), sizeof(google_der)); I would encourage you to use ImportCertFromFile using any of those certs, since like I expressed on https://code.google.com/p/chromium/issues/detail?id=515969 , we can/should clean these constants up. https://codereview.chromium.org/1223233002/diff/260001/chrome/browser/ssl/com... File chrome/browser/ssl/common_name_mismatch_handler.h (right): https://codereview.chromium.org/1223233002/diff/260001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.h:22: class CommonNameMismatchHandler : public net::URLFetcherDelegate, From a design perspective, this seems really wrong to trigger such requests. We're already looking for ways at properly accounting for how Chrome eats up user data. I didn't realize the design rested on kicking off background requests. Is there any design suitable for your goal that doesn't involve doing this? [I'm ignoring the fact that URLFetcher will fail to properly handle client auth, proxy auth, HTTP auth, and a variety of ways in which it doesn't align with an actual navigation request, other than mentioning it here] https://codereview.chromium.org/1223233002/diff/260001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/1223233002/diff/260001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2279: reinterpret_cast<const char*>(google_der), sizeof(google_der)); Same comments; like I said on email, use ImportCertFromFile here; that's the preferred pattern :) https://codereview.chromium.org/1223233002/diff/260001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2305: IN_PROC_BROWSER_TEST_F(CertVerifierBrowserTest, From a testing design, normally the fixture naming matches what's being tested. I totally understand that your fixture either needs to be a CertVerifierBrowserTest, but you have two options to better express this for test naming 1) Using inheritance - i.e. class MyTestName : public CertVerifierBrowserTest 2) Using aliasing (typedefs or using) - e.g. using MyTestName = CertVerifierBrowserTest Both of these ensure that the stringified test name is meaningful, even if the actual implementation is a CertVerifierBrowserTest https://codereview.chromium.org/1223233002/diff/260001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2314: host_resolver()->AddRule("test.example.com", "127.0.0.1"); BUG: You shouldn't assume that SpawnedTestServer is 127.0.0.1 You should be using AddRule() to add the test server's hostname - either using AddRule(..., https_server_example_domain_.GetURL("").host()), or, simpler, AddRule(..., https_server_example_domain_.host_port_pair().host()) using host_port_pair() should properly handle any IPv6 support https://codereview.chromium.org/1223233002/diff/260001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2322: net::GetTestCertsDirectory(), "quic_test.example.com.crt"); Haha, I should have been clearer in my email. Out of *all* of the test certs you could have picked, you arguably picked the least-best one. Sorry! :P They're the least-best because they're kept in sync with Google3 and it gets a whole messy thing to update. I would pick any of the test certificates that have a checked in generator script in net/data/ssl/scripts (you can see what certificates this is via net/data/ssl/certificates/README ) https://codereview.chromium.org/1223233002/diff/260001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2357: ->GetTypeForTesting()); did git cl format do this? Weirdest formatting I've seen yet from it. https://codereview.chromium.org/1223233002/diff/260001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_classification.h (right): https://codereview.chromium.org/1223233002/diff/260001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_classification.h:84: static bool GetWWWSubDomainMatch(const std::string& host_name, This is more of a nit, but it seems like this would be better clumped with the remaining static functions on line 52. From an API contract level, it makes me somewhat sad to see all of these as static public, since it feels like it's revealing a lot of the internal details just for unit tests. Makes me wonder whether it'd be better with just friending the test fixture in the private section. You shouldn't have to do that in this CL, though, because we should update the other public static methods simultaneously. https://codereview.chromium.org/1223233002/diff/260001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_classification.h:99: // Returns true if GetWWWSubDomainMatch finds a www mismatch. Why have this function at all then? It seems like it can just be replaced/inlined, isn't needed as a separate function.
On 2015/08/07 00:14:11, Ryan Sleevi wrote: > I've taken a look at this, with a focus on testing. > > On a high-level, I really don't think we should just kick off such URL requests, > from the point of view of bandwidth, of privacy, and of power usage. That is, I > think Chrome already does too much 'magic under the hood', and we're seeing that > the magic is costing us - and, for users in emerging markets, has real, > tangible, monetary costs. > > From a design perspective, I'd love if you and -enamel could iterate and > evaluate whether or not the background request is strictly necessary, or whether > it still represents a user improvement to simply offer the Suggest URL w/o any > background requests. I don't know if I'm stubborn enough to block this CL, but I > think it's something that the costs/chance for harm of the background request > exceed the value. > > https://codereview.chromium.org/1223233002/diff/220001/net/test/spawned_test_... > File net/test/spawned_test_server/base_test_server.cc (left): > > https://codereview.chromium.org/1223233002/diff/220001/net/test/spawned_test_... > net/test/spawned_test_server/base_test_server.cc:44: > Unnecessary change? > > https://codereview.chromium.org/1223233002/diff/260001/chrome/browser/captive... > File chrome/browser/captive_portal/captive_portal_browsertest.cc (right): > > https://codereview.chromium.org/1223233002/diff/260001/chrome/browser/captive... > chrome/browser/captive_portal/captive_portal_browsertest.cc:351: > reinterpret_cast<const char*>(google_der), sizeof(google_der)); > I would encourage you to use ImportCertFromFile using any of those certs, since > like I expressed on https://code.google.com/p/chromium/issues/detail?id=515969 , > we can/should clean these constants up. > > https://codereview.chromium.org/1223233002/diff/260001/chrome/browser/ssl/com... > File chrome/browser/ssl/common_name_mismatch_handler.h (right): > > https://codereview.chromium.org/1223233002/diff/260001/chrome/browser/ssl/com... > chrome/browser/ssl/common_name_mismatch_handler.h:22: class > CommonNameMismatchHandler : public net::URLFetcherDelegate, > From a design perspective, this seems really wrong to trigger such requests. > > We're already looking for ways at properly accounting for how Chrome eats up > user data. I didn't realize the design rested on kicking off background > requests. Is there any design suitable for your goal that doesn't involve doing > this? > > [I'm ignoring the fact that URLFetcher will fail to properly handle client auth, > proxy auth, HTTP auth, and a variety of ways in which it doesn't align with an > actual navigation request, other than mentioning it here] > > https://codereview.chromium.org/1223233002/diff/260001/chrome/browser/ssl/ssl... > File chrome/browser/ssl/ssl_browser_tests.cc (right): > > https://codereview.chromium.org/1223233002/diff/260001/chrome/browser/ssl/ssl... > chrome/browser/ssl/ssl_browser_tests.cc:2279: reinterpret_cast<const > char*>(google_der), sizeof(google_der)); > Same comments; like I said on email, use ImportCertFromFile here; that's the > preferred pattern :) > > https://codereview.chromium.org/1223233002/diff/260001/chrome/browser/ssl/ssl... > chrome/browser/ssl/ssl_browser_tests.cc:2305: > IN_PROC_BROWSER_TEST_F(CertVerifierBrowserTest, > From a testing design, normally the fixture naming matches what's being tested. > > I totally understand that your fixture either needs to be a > CertVerifierBrowserTest, but you have two options to better express this for > test naming > > 1) Using inheritance - i.e. class MyTestName : public CertVerifierBrowserTest > 2) Using aliasing (typedefs or using) - e.g. using MyTestName = > CertVerifierBrowserTest > > Both of these ensure that the stringified test name is meaningful, even if the > actual implementation is a CertVerifierBrowserTest > > https://codereview.chromium.org/1223233002/diff/260001/chrome/browser/ssl/ssl... > chrome/browser/ssl/ssl_browser_tests.cc:2314: > host_resolver()->AddRule("test.example.com", "127.0.0.1"); > BUG: You shouldn't assume that SpawnedTestServer is 127.0.0.1 > > You should be using AddRule() to add the test server's hostname - either using > AddRule(..., https_server_example_domain_.GetURL("").host()), or, simpler, > AddRule(..., https_server_example_domain_.host_port_pair().host()) > > using host_port_pair() should properly handle any IPv6 support > > https://codereview.chromium.org/1223233002/diff/260001/chrome/browser/ssl/ssl... > chrome/browser/ssl/ssl_browser_tests.cc:2322: net::GetTestCertsDirectory(), > "quic_test.example.com.crt"); > Haha, I should have been clearer in my email. Out of *all* of the test certs you > could have picked, you arguably picked the least-best one. Sorry! :P > > They're the least-best because they're kept in sync with Google3 and it gets a > whole messy thing to update. I would pick any of the test certificates that have > a checked in generator script in net/data/ssl/scripts (you can see what > certificates this is via net/data/ssl/certificates/README ) > > https://codereview.chromium.org/1223233002/diff/260001/chrome/browser/ssl/ssl... > chrome/browser/ssl/ssl_browser_tests.cc:2357: ->GetTypeForTesting()); > did git cl format do this? Weirdest formatting I've seen yet from it. > > https://codereview.chromium.org/1223233002/diff/260001/chrome/browser/ssl/ssl... > File chrome/browser/ssl/ssl_error_classification.h (right): > > https://codereview.chromium.org/1223233002/diff/260001/chrome/browser/ssl/ssl... > chrome/browser/ssl/ssl_error_classification.h:84: static bool > GetWWWSubDomainMatch(const std::string& host_name, > This is more of a nit, but it seems like this would be better clumped with the > remaining static functions on line 52. > > From an API contract level, it makes me somewhat sad to see all of these as > static public, since it feels like it's revealing a lot of the internal details > just for unit tests. Makes me wonder whether it'd be better with just friending > the test fixture in the private section. You shouldn't have to do that in this > CL, though, because we should update the other public static methods > simultaneously. > > https://codereview.chromium.org/1223233002/diff/260001/chrome/browser/ssl/ssl... > chrome/browser/ssl/ssl_error_classification.h:99: // Returns true if > GetWWWSubDomainMatch finds a www mismatch. > Why have this function at all then? It seems like it can just be > replaced/inlined, isn't needed as a separate function. Thank you for the review. Yes, having extra URL checks might not be good. This CL does not add any extra background URL checks. When there is a www mismatch, we do not do the captive portal check. So, in these cases, instead of captive portal check, there is a check for some other URL. Also, this is a HEAD request, so this might be a little less resource intensive that if it was a GET request. Thanks.
The UI design has been changed, so we would be directly redirecting to the WWW mismatched domain, without any interstitial or infobar. This is the patch which implements this, unittests and browser tests are changed accordingly. So, the previous changes for interstitial, JS link handler, tests, ssl blocking page are all reverted. https://codereview.chromium.org/1223233002/diff/260001/chrome/browser/captive... File chrome/browser/captive_portal/captive_portal_browsertest.cc (right): https://codereview.chromium.org/1223233002/diff/260001/chrome/browser/captive... chrome/browser/captive_portal/captive_portal_browsertest.cc:351: reinterpret_cast<const char*>(google_der), sizeof(google_der)); On 2015/08/07 00:14:10, Ryan Sleevi wrote: > I would encourage you to use ImportCertFromFile using any of those certs, since > like I expressed on https://code.google.com/p/chromium/issues/detail?id=515969 , > we can/should clean these constants up. Done. https://codereview.chromium.org/1223233002/diff/260001/chrome/browser/ssl/com... File chrome/browser/ssl/common_name_mismatch_handler.h (right): https://codereview.chromium.org/1223233002/diff/260001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.h:22: class CommonNameMismatchHandler : public net::URLFetcherDelegate, On 2015/08/07 00:14:11, Ryan Sleevi wrote: > From a design perspective, this seems really wrong to trigger such requests. > > We're already looking for ways at properly accounting for how Chrome eats up > user data. I didn't realize the design rested on kicking off background > requests. Is there any design suitable for your goal that doesn't involve doing > this? > > [I'm ignoring the fact that URLFetcher will fail to properly handle client auth, > proxy auth, HTTP auth, and a variety of ways in which it doesn't align with an > actual navigation request, other than mentioning it here] Acknowledged. https://codereview.chromium.org/1223233002/diff/260001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/1223233002/diff/260001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2279: reinterpret_cast<const char*>(google_der), sizeof(google_der)); On 2015/08/07 00:14:11, Ryan Sleevi wrote: > Same comments; like I said on email, use ImportCertFromFile here; that's the > preferred pattern :) Done. https://codereview.chromium.org/1223233002/diff/260001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2305: IN_PROC_BROWSER_TEST_F(CertVerifierBrowserTest, On 2015/08/07 00:14:11, Ryan Sleevi wrote: > From a testing design, normally the fixture naming matches what's being tested. > > I totally understand that your fixture either needs to be a > CertVerifierBrowserTest, but you have two options to better express this for > test naming > > 1) Using inheritance - i.e. class MyTestName : public CertVerifierBrowserTest > 2) Using aliasing (typedefs or using) - e.g. using MyTestName = > CertVerifierBrowserTest > > Both of these ensure that the stringified test name is meaningful, even if the > actual implementation is a CertVerifierBrowserTest Done. Thank you for letting me know about this. https://codereview.chromium.org/1223233002/diff/260001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2314: host_resolver()->AddRule("test.example.com", "127.0.0.1"); On 2015/08/07 00:14:11, Ryan Sleevi wrote: > BUG: You shouldn't assume that SpawnedTestServer is 127.0.0.1 > > You should be using AddRule() to add the test server's hostname - either using > AddRule(..., https_server_example_domain_.GetURL("").host()), or, simpler, > AddRule(..., https_server_example_domain_.host_port_pair().host()) > > using host_port_pair() should properly handle any IPv6 support Done. I was using 127.0.0.1 as I verified that GetHostName() returns 127.0.0.1 for CERT_OK. Anyway, I agree with you that it is good to get the hostname each time. https://codereview.chromium.org/1223233002/diff/260001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2322: net::GetTestCertsDirectory(), "quic_test.example.com.crt"); On 2015/08/07 00:14:11, Ryan Sleevi wrote: > Haha, I should have been clearer in my email. Out of *all* of the test certs you > could have picked, you arguably picked the least-best one. Sorry! :P > > They're the least-best because they're kept in sync with Google3 and it gets a > whole messy thing to update. I would pick any of the test certificates that have > a checked in generator script in net/data/ssl/scripts (you can see what > certificates this is via net/data/ssl/certificates/README ) Haha. Ok, I went through most of the certs and *picked* this because this has the most suitable dns name(test.example.com) which is a top level domain and not 127.0.0.1 or localhost as used by most certs. I changed this and used spdy_pooling.pem now. https://codereview.chromium.org/1223233002/diff/260001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2357: ->GetTypeForTesting()); On 2015/08/07 00:14:11, Ryan Sleevi wrote: > did git cl format do this? Weirdest formatting I've seen yet from it. Yes. Anyway, the browser tests are changed now, so I don't need this anymore. https://codereview.chromium.org/1223233002/diff/260001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_classification.h (right): https://codereview.chromium.org/1223233002/diff/260001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_classification.h:84: static bool GetWWWSubDomainMatch(const std::string& host_name, On 2015/08/07 00:14:11, Ryan Sleevi wrote: > This is more of a nit, but it seems like this would be better clumped with the > remaining static functions on line 52. > > From an API contract level, it makes me somewhat sad to see all of these as > static public, since it feels like it's revealing a lot of the internal details > just for unit tests. Makes me wonder whether it'd be better with just friending > the test fixture in the private section. You shouldn't have to do that in this > CL, though, because we should update the other public static methods > simultaneously. Done. https://codereview.chromium.org/1223233002/diff/260001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_classification.h:99: // Returns true if GetWWWSubDomainMatch finds a www mismatch. On 2015/08/07 00:14:11, Ryan Sleevi wrote: > Why have this function at all then? It seems like it can just be > replaced/inlined, isn't needed as a separate function. This method gets the hostname,dns names and calls the static method GetWWWSubDomainMatch. I thought that it might not be good to have it inline with definition in header file, tests also use this.
https://codereview.chromium.org/1223233002/diff/260001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_classification.h (right): https://codereview.chromium.org/1223233002/diff/260001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_classification.h:99: // Returns true if GetWWWSubDomainMatch finds a www mismatch. On 2015/08/07 22:28:48, Bhanu Dev wrote: > This method gets the hostname,dns names and calls the static method > GetWWWSubDomainMatch. I thought that it might not be good to have it inline with > definition in header file, tests also use this. I didn't mean inlined in the .h file, I meant just delete this function entirely and inline what this function does in the one place it's called. That is, now that you have the static function, it's somewhat questionable whether or not there's a benefit from having a small function like this that calls said static function.
On 2015/08/07 22:31:11, Ryan Sleevi wrote: > https://codereview.chromium.org/1223233002/diff/260001/chrome/browser/ssl/ssl... > File chrome/browser/ssl/ssl_error_classification.h (right): > > https://codereview.chromium.org/1223233002/diff/260001/chrome/browser/ssl/ssl... > chrome/browser/ssl/ssl_error_classification.h:99: // Returns true if > GetWWWSubDomainMatch finds a www mismatch. > On 2015/08/07 22:28:48, Bhanu Dev wrote: > > This method gets the hostname,dns names and calls the static method > > GetWWWSubDomainMatch. I thought that it might not be good to have it inline > with > > definition in header file, tests also use this. > > I didn't mean inlined in the .h file, I meant just delete this function entirely > and inline what this function does in the one place it's called. That is, now > that you have the static function, it's somewhat questionable whether or not > there's a benefit from having a small function like this that calls said static > function. Actually, this gets called 5/6 times in SSLErrorClassification tests. I can add this code everywhere, but I thought that would not be good either.
@rsleevi, I have added the code to handle thread restrictions. Thanks a lot for helping me about that. I want to commit the CL, please let me know if some more changes are required. Thanks. @meacer, Can you please review the code. Thanks.
Still LGTM. Please wait for an l-g-t-m from rsleevi@ before submitting. https://codereview.chromium.org/1223233002/diff/320001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/1223233002/diff/320001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2298: net::ImportCertFromFile(net::GetTestCertsDirectory(), "spdy_pooling.pem"); Can you add a comment here saying the pem file doesn't matter?
https://codereview.chromium.org/1223233002/diff/320001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/1223233002/diff/320001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2298: net::ImportCertFromFile(net::GetTestCertsDirectory(), "spdy_pooling.pem"); On 2015/08/12 22:32:56, Mustafa Emre Acer wrote: > Can you add a comment here saying the pem file doesn't matter? Done. The file path matters here, the file path does not matter for the previous cert (at line 2294). Fixed the documentation. Thanks.
rsleevi@, Can you please let me know if the code is good enough to commit. mmenke@, Can you please take a look at the chrome/browser/captive_portal/captive_portal_browsertest.cc file, I used ImportCertFromFile method as Ryan suggested.
On 2015/08/13 20:03:17, Bhanu Dev wrote: > rsleevi@, Can you please let me know if the code is good enough to commit. > > mmenke@, Can you please take a look at the > chrome/browser/captive_portal/captive_portal_browsertest.cc file, I used > ImportCertFromFile method as Ryan suggested. captive_portal/ LGTM.
Chatted with felt@ a bit. I guess I'm still reserved about background requests, but not enough to block the CL by any means. I think I excised all my draft comments related to that, but if I left any talking about interstitial links or whinging about background requests, ignore that. My remaining comments mostly focused on documentation and style. I did highlight some edge cases with DANGER that could potentially lead to security bugs (in the future), so it's worth considering. If the comments are addressed, I would say this LGTM, although I won't be OOO and won't be around to do another pass. Hopefully someone with a critical eye (Matt, David) could take another pass if you felt uncomfortable with the suggestions or wanted another set of eyes to make sure you understood the underlying concerns. https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/com... File chrome/browser/ssl/common_name_mismatch_handler.cc (right): https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.cc:47: net::LOAD_BYPASS_CACHE | net::LOAD_DO_NOT_SAVE_COOKIES | The LOAD_BYPASS_CACHE is subtle and surprising - document why. [My own presumption is that you want to make sure that you don't get a 200-series success code, since there's more than 200 to indicate a successful load, and you also want to make sure the page will actually load if you directed the user there, even though the latter part seems somewhat weird because the actual load would be from the cache] https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.cc:64: callback.Run(results); GetSuggestedUrlCheckResult(...) url_fetcher_.reset(); base::ResetAndReturn(&check_url_callback_).Run(...); https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.cc:81: // Make sure the |landing_url| is a valid https page. s/HTTPS/ https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.cc:81: // Make sure the |landing_url| is a valid https page. Comment-wise, this seems to hide what 'valid' means - here, you seemingly are wanting to check if it loads and doesn't result in a redirect loop to the current page? https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.cc:91: bool CommonNameMismatchHandler::GetSuggestedUrl( The order of definitions should match the order of declarations - this should be before OnURLFetchComplete https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.cc:100: } else { Don't use else after return - https://www.chromium.org/developers/coding-style#Code_Formatting https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.cc:112: return url_fetcher_.get() != NULL; nullptr is recommended for new code ( https://chromium-cpp.appspot.com/ ), however, an even better strategy is to rely on the implicit bool coercion, which allows the same pattern across multiple containers return url_fetcher_; (scoped_ptr/scoped_refptr<> will convert to bool through conversion helpers, and raw pointers are always convertable) https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/com... File chrome/browser/ssl/common_name_mismatch_handler.h (right): https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.h:14: #include "net/url_request/url_fetcher.h" Can forward declare https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.h:21: // perform a network request to check the validity of new URL. As a class comment, this feels worded wrong. Perhaps the following: // This class handles errors due to common name mismatches (|ERR_CERT_COMMON_NAME_INVALID|) // and helps remediate them by suggesting alternative URLs that may be what // the user intended to load. https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.h:44: }; The use of a struct helper for this seems unnecessary; why not just return the two arguments? https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.h:49: const GURL request_url, const GURL& https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.h:54: // |callback|. This comment could use beefing up. If I'm only reading this header file, and trying to understand what the class does, what is a check to validate a URL? What might I reasonably expect? At the same time, you want to be careful to avoid documenting exactly what will happen (that is, your header comment shouldn't be a prose form of the code, since code will change but prose lives on), but attempt to explain the narrative of what are the preconditions/postconditions and what might happen / how might this be used https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.h:60: // to try into |suggested_url|. comment-wise, "writes a suggestion" feels weird in the broader context. // Determines if, for |request_url| serving a certificate that is valid for // the domain names |dns_names|, there is a name that the certificate is // valid for that closely matches the original name in |request_url|. If // so, returns true, and sets |*suggested_url| to a URL that is unlikely // to cause an ERR_CERT_COMMON_NAME_INVALID error. https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.h:66: static void set_state_for_testing(TestingState testing_state) { Static methods are generally a strong anti-pattern, especially for tests (as implicitly it makes test non-hermetic unless the state is reset) Why can't this be exposed as an instance method with an instance member? https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.h:69: static TestingState get_state_for_testing() { return testing_state_; } Why is it needed to access the testing state? We rarely (never?) return this. https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.h:78: Results* results) const; This doesn't seem like it needs to be a private method - that is, it could be fully localized to the .cc in an unnamed namespace. https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.h:81: bool CheckingSuggestedUrl() const; s/CheckingSuggestedUrl/IsCheckingSuggestedUrl/ https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.h:91: scoped_ptr<net::URLFetcher> url_fetcher_; There seems to be a lot of unnecessary vertical whitespace here. You can delete the line breaks on 84, 86, 88, and 90. https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2390: class SSLInterstitialTimerObserver { ODR VIOLATION DANGER: Putting this helper in the main scope is extremely dangerous and may cause ODR violations (I've found and fixed multiple bugs related to this in other tests in the past). Strongly suggest you move this helper up to the unnamed namespace. https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2404: scoped_refptr<content::MessageLoopRunner> message_loop_runner_; Wow, I'm surprised jam@ hasn't nuked this. base::RunLoop is the preferred hotness for this; anything you see that says "MessageLoop" is highly suspect and should be avoided as deprecated, and there should be alternatives that exist. https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_classification.cc:334: if (dns_names[i].empty() || dns_names[i].find('\0') != std::string::npos DANGER: There is _zero_ guarantee that |dns_names| are well-formed/canonical. That is, a cert could totally have the name "www....monkeypants!!@#example.com", which is in no way valid. This is independent of the CL, but reflects the danger in just blindly loading it. https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_classification.cc:340: base::ASCIIToUTF16(host_name)) { Hrm, this is problematic from the original code - converting to String16 only to ignore the result is just wasting bytes. I'll try to resolve this as part of https://crbug.com/488531 https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.cc:157: if (extra_cert_errors) { This is not correct. CertStatus contains errors _and_ informational status bits. Our preferred idiom is to look at both IsCertStatusError and IsCertStatusMinorError to make a decision. https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.cc:217: base::Unretained(this))); This seems incredibly dangerous - base::Unretained() is almost universally a red flag that signals "carefully review the threading contracts", and I don't see code that guarantees this is safe. https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler.h (right): https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.h:14: #include "chrome/browser/profiles/profile.h" Can forward declare https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.h:69: const CommonNameMismatchHandler::Results& results); Yet another reason not to use the struct for results and just pass it as two parameters - you could totally get away w/o including the CommonNameMismatchHandler header https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler_unittest.cc:250: base::MessageLoop::current()->RunUntilIdle(); Hrm, I missed that existing code calls this, but base::RunLoop should be preferred over calling base::MessageLoop::current() https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler_unittest.cc:287: EXPECT_FALSE(error_handler()->IsTimerRunning()); Why have this call? This is guaranteed by the testing framework, is it not? Every test will fully create a new SSLErrorHandler (and thus a new timer), and you should not have any URLs loaded.
felt@chromium.org changed reviewers: + felt@chromium.org
@rsleevi, Thanks a lot for your review. I think I learnt about some possible security bugs while addressing them. @davidben, can you please take a second pass and see if the comments are resolved well. Thanks. https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/com... File chrome/browser/ssl/common_name_mismatch_handler.cc (right): https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.cc:47: net::LOAD_BYPASS_CACHE | net::LOAD_DO_NOT_SAVE_COOKIES | On 2015/08/14 00:40:16, Ryan Sleevi (out until 8-24) wrote: > The LOAD_BYPASS_CACHE is subtle and surprising - document why. > > [My own presumption is that you want to make sure that you don't get a > 200-series success code, since there's more than 200 to indicate a successful > load, and you also want to make sure the page will actually load if you directed > the user there, even though the latter part seems somewhat weird because the > actual load would be from the cache] I used this because I like wanted to completely make sure the page loads. But, you are right as the load would be from cache only. Removing this. Thanks. https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.cc:64: callback.Run(results); On 2015/08/14 00:40:16, Ryan Sleevi (out until 8-24) wrote: > GetSuggestedUrlCheckResult(...) > url_fetcher_.reset(); > base::ResetAndReturn(&check_url_callback_).Run(...); Done. https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.cc:81: // Make sure the |landing_url| is a valid https page. On 2015/08/14 00:40:16, Ryan Sleevi (out until 8-24) wrote: > s/HTTPS/ Done. https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.cc:81: // Make sure the |landing_url| is a valid https page. On 2015/08/14 00:40:16, Ryan Sleevi (out until 8-24) wrote: > Comment-wise, this seems to hide what 'valid' means - here, you seemingly are > wanting to check if it loads and doesn't result in a redirect loop to the > current page? Done. I was meaning "valid" to be, without any error and having a valid certificate. Modified it. https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.cc:91: bool CommonNameMismatchHandler::GetSuggestedUrl( On 2015/08/14 00:40:16, Ryan Sleevi (out until 8-24) wrote: > The order of definitions should match the order of declarations - this should be > before OnURLFetchComplete Done. https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.cc:100: } else { On 2015/08/14 00:40:16, Ryan Sleevi (out until 8-24) wrote: > Don't use else after return - > https://www.chromium.org/developers/coding-style#Code_Formatting Done. Thanks. https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.cc:112: return url_fetcher_.get() != NULL; On 2015/08/14 00:40:16, Ryan Sleevi (out until 8-24) wrote: > nullptr is recommended for new code ( https://chromium-cpp.appspot.com/ ), > however, an even better strategy is to rely on the implicit bool coercion, which > allows the same pattern across multiple containers > > return url_fetcher_; > > (scoped_ptr/scoped_refptr<> will convert to bool through conversion helpers, and > raw pointers are always convertable) Done. https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/com... File chrome/browser/ssl/common_name_mismatch_handler.h (right): https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.h:14: #include "net/url_request/url_fetcher.h" On 2015/08/14 00:40:16, Ryan Sleevi (out until 8-24) wrote: > Can forward declare Done. https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.h:21: // perform a network request to check the validity of new URL. On 2015/08/14 00:40:16, Ryan Sleevi (out until 8-24) wrote: > As a class comment, this feels worded wrong. Perhaps the following: > > // This class handles errors due to common name mismatches > (|ERR_CERT_COMMON_NAME_INVALID|) > // and helps remediate them by suggesting alternative URLs that may be what > // the user intended to load. Done. https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.h:44: }; On 2015/08/14 00:40:16, Ryan Sleevi (out until 8-24) wrote: > The use of a struct helper for this seems unnecessary; why not just return the > two arguments? Done. https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.h:49: const GURL request_url, On 2015/08/14 00:40:16, Ryan Sleevi (out until 8-24) wrote: > const GURL& Done. https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.h:54: // |callback|. On 2015/08/14 00:40:16, Ryan Sleevi (out until 8-24) wrote: > This comment could use beefing up. > > If I'm only reading this header file, and trying to understand what the class > does, what is a check to validate a URL? What might I reasonably expect? > > At the same time, you want to be careful to avoid documenting exactly what will > happen (that is, your header comment shouldn't be a prose form of the code, > since code will change but prose lives on), but attempt to explain the narrative > of what are the preconditions/postconditions and what might happen / how might > this be used Done. https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.h:60: // to try into |suggested_url|. On 2015/08/14 00:40:17, Ryan Sleevi (out until 8-24) wrote: > comment-wise, "writes a suggestion" feels weird in the broader context. > > // Determines if, for |request_url| serving a certificate that is valid for > // the domain names |dns_names|, there is a name that the certificate is > // valid for that closely matches the original name in |request_url|. If > // so, returns true, and sets |*suggested_url| to a URL that is unlikely > // to cause an ERR_CERT_COMMON_NAME_INVALID error. Done. Thanks. https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.h:66: static void set_state_for_testing(TestingState testing_state) { On 2015/08/14 00:40:16, Ryan Sleevi (out until 8-24) wrote: > Static methods are generally a strong anti-pattern, especially for tests (as > implicitly it makes test non-hermetic unless the state is reset) > > Why can't this be exposed as an instance method with an instance member? I was considering that this is good enough as long as we are setting and resetting the states correctly. Also, was trying to follow existing peices of code like https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/cap... https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.h:69: static TestingState get_state_for_testing() { return testing_state_; } On 2015/08/14 00:40:16, Ryan Sleevi (out until 8-24) wrote: > Why is it needed to access the testing state? We rarely (never?) return this. Done. https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.h:78: Results* results) const; On 2015/08/14 00:40:16, Ryan Sleevi (out until 8-24) wrote: > This doesn't seem like it needs to be a private method - that is, it could be > fully localized to the .cc in an unnamed namespace. Done. https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.h:81: bool CheckingSuggestedUrl() const; On 2015/08/14 00:40:16, Ryan Sleevi (out until 8-24) wrote: > s/CheckingSuggestedUrl/IsCheckingSuggestedUrl/ Done. https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.h:91: scoped_ptr<net::URLFetcher> url_fetcher_; On 2015/08/14 00:40:16, Ryan Sleevi (out until 8-24) wrote: > There seems to be a lot of unnecessary vertical whitespace here. You can delete > the line breaks on 84, 86, 88, and 90. Done. https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2390: class SSLInterstitialTimerObserver { On 2015/08/14 00:40:17, Ryan Sleevi (out until 8-24) wrote: > ODR VIOLATION DANGER: Putting this helper in the main scope is extremely > dangerous and may cause ODR violations (I've found and fixed multiple bugs > related to this in other tests in the past). Strongly suggest you move this > helper up to the unnamed namespace. Done. https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2404: scoped_refptr<content::MessageLoopRunner> message_loop_runner_; On 2015/08/14 00:40:17, Ryan Sleevi (out until 8-24) wrote: > Wow, I'm surprised jam@ hasn't nuked this. > > base::RunLoop is the preferred hotness for this; anything you see that says > "MessageLoop" is highly suspect and should be avoided as deprecated, and there > should be alternatives that exist. Done. https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_classification.cc:334: if (dns_names[i].empty() || dns_names[i].find('\0') != std::string::npos On 2015/08/14 00:40:17, Ryan Sleevi (out until 8-24) wrote: > DANGER: There is _zero_ guarantee that |dns_names| are well-formed/canonical. > That is, a cert could totally have the name mailto:"www....monkeypants!!@#example.com", > which is in no way valid. > > This is independent of the CL, but reflects the danger in just blindly loading > it. Acknowledged. I think we are not executing the dns name or so, we just compare it with hostname. https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_classification.cc:340: base::ASCIIToUTF16(host_name)) { On 2015/08/14 00:40:17, Ryan Sleevi (out until 8-24) wrote: > Hrm, this is problematic from the original code - converting to String16 only to > ignore the result is just wasting bytes. > > I'll try to resolve this as part of https://crbug.com/488531 Done. https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.cc:157: if (extra_cert_errors) { On 2015/08/14 00:40:17, Ryan Sleevi (out until 8-24) wrote: > This is not correct. CertStatus contains errors _and_ informational status bits. > > Our preferred idiom is to look at both IsCertStatusError and > IsCertStatusMinorError to make a decision. Done. Thanks for pointing this out. This might have resulted in false negatives. https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.cc:217: base::Unretained(this))); On 2015/08/14 00:40:17, Ryan Sleevi (out until 8-24) wrote: > This seems incredibly dangerous - base::Unretained() is almost universally a red > flag that signals "carefully review the threading contracts", and I don't see > code that guarantees this is safe. Done. Added a method |Cancel()| in CommonNameMismatchHandler and it is called before the SSLErrorHandler gets deleted. https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler.h (right): https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.h:14: #include "chrome/browser/profiles/profile.h" On 2015/08/14 00:40:17, Ryan Sleevi (out until 8-24) wrote: > Can forward declare Done. https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.h:69: const CommonNameMismatchHandler::Results& results); On 2015/08/14 00:40:17, Ryan Sleevi (out until 8-24) wrote: > Yet another reason not to use the struct for results and just pass it as two > parameters - you could totally get away w/o including the > CommonNameMismatchHandler header Done. https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler_unittest.cc:250: base::MessageLoop::current()->RunUntilIdle(); On 2015/08/14 00:40:17, Ryan Sleevi (out until 8-24) wrote: > Hrm, I missed that existing code calls this, but base::RunLoop should be > preferred over calling base::MessageLoop::current() Done. https://codereview.chromium.org/1223233002/diff/340001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler_unittest.cc:287: EXPECT_FALSE(error_handler()->IsTimerRunning()); On 2015/08/14 00:40:17, Ryan Sleevi (out until 8-24) wrote: > Why have this call? This is guaranteed by the testing framework, is it not? > Every test will fully create a new SSLErrorHandler (and thus a new timer), and > you should not have any URLs loaded. Done. Yes, timer would not be running when SSLErrorHandler gets created. It starts only after the method StartHandlingError() is invoked. I just tried to make sure this and also since previous tests were doing the same.
https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/com... File chrome/browser/ssl/common_name_mismatch_handler.cc (right): https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.cc:41: net::URLFetcher::HEAD, this); If you're always passing zero, you can use the ID-less net::URLFetcher::Create(url, net::URLFetcher::HEAD, this); https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.cc:89: const GURL& landing_url = url_fetcher_.get()->GetURL(); url_fetcher_->GetOriginalURL(), url_fetcher_->GetURL() https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.cc:99: base::ResetAndReturn(&check_url_callback_).Run(result, suggested_url); Using suggested_url here is a UAF since you called url_fetcher_.reset(), you saved it as a const reference, and URLFetcher itself returns a const reference. It should be: GURL suggested_url = url_fetcher->GetOriginalURL(); https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.cc:103: return url_fetcher_; You should run a try job to check, but I think MSVC will get unhappy at this and want something like: return !!url_fetcher_; https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/com... File chrome/browser/ssl/common_name_mismatch_handler.h (right): https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.h:65: static void set_state_for_testing(TestingState testing_state) { Since this makes you skip the callback, it might be better to do a set_hang_requests_for_testing(bool) or something? I can't come up with a terribly clear name. Certainly the comment should mention that the callback won't get called since that's what happens from the consumer's perspective. (Why not URLFetcherFactory?) https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:230: } Nit: Any reason not to define these inline with the class definition? Saves you a little bit of repetition. https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:242: if (web_contents_ == web_contents && message_loop_runner_.get()) message_loop_runner_.get() is redundant, isn't it? https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2315: typedef CertVerifierBrowserTest CommonNameMismatchBrowserTest; Nit: C++11-style is preferred now: using CommonNameMismatchBrowserTest = CertVerifierBrowserTest; https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2335: // The pem file does not matter. Nit: pem -> PEM (Although, with the comment below, perhaps you don't need the comment at all.) https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2336: scoped_refptr<net::X509Certificate> cert1 = Nit: cert1 -> cert? I don't see a cert2. https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2337: net::ImportCertFromFile(net::GetTestCertsDirectory(), "ok_cert.pem"); This can now just be https_server_example_domain_.GetCertificate(). https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2355: // Request to "www.test.example.com" should not result in any error. www.test.example.com -> mail.example.com? https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2359: // The path does not matter. Nit: I'd maybe write: // Use a complex URL to ensure the path, etc., are preserved. The path itself does not matter. (Add a URL #fragment while you're at it? Those are kinda weird and might be worth testing.) https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2398: net::ImportCertFromFile(net::GetTestCertsDirectory(), "ok_cert.pem"); Ditto about GetCertificate and cert1 -> cert https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2455: net::ImportCertFromFile(net::GetTestCertsDirectory(), "ok_cert.pem"); Ditto about GetCertificate and cert1 -> cert https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2487: interstitial_timer_observer.WaitForTimerStarted(); Since you're mucking about with the timer, this probably should, to ensure we don't flake: SSLErrorHandler::SetInterstitialDelayTypeForTest(SSLErrorHandler::LONG); https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2522: net::ImportCertFromFile(net::GetTestCertsDirectory(), "ok_cert.pem"); Ditto about GetCertificate and cert1 -> cert https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2554: interstitial_timer_observer.WaitForTimerStarted(); Ditto about timer https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2587: net::ImportCertFromFile(net::GetTestCertsDirectory(), "ok_cert.pem"); Ditto about GetCertificate and cert1 -> cert https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_classification.cc:336: || !(IsHostNameKnownTLD(dns_names[i]))) { Existing code, but I believe we put the || at the end of the line, so it'd be: if (dns_names[i].empty() || dns_names[i].find('\0') != std::string::npos || dns_names[i].length() == host_name.length() || !IsHostNameKnownTLD(dns_names[i])) { continue; } https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_classification.cc:341: www_match_host_name->assign(dns_names[i].data(), dns_names[i].size()); This can just be: *www_match_host_name = dns_names[i]; https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_classification.cc:347: www_match_host_name->assign(dns_names[i].data(), dns_names[i].size()); Ditto. https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.cc:152: ssl_info_.cert_status ^ net::CERT_STATUS_COMMON_NAME_INVALID; What if CERT_STATUS_COMMON_NAME_INVALID isn't in cert_status? Then you're adding it to extra_cert_errors. If that's intentional, that probably needs a comment to explain what's going on, or better some more straight-forward logic. :-) https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.cc:223: load_params.transition_type = ui::PAGE_TRANSITION_TYPED; This seems off. The URL wasn't typed into the omnibox or anything, so it shouldn't appear in the history as such. You're just automatically redirecting there, right? It seems things default to LINK. https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler_unittest.cc (left): https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler_unittest.cc:20: #include "testing/gtest/include/gtest/gtest.h" This include should still be there due to IWYU. https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler_unittest.cc:244: EXPECT_TRUE(error_handler()->IsTimerRunning()); To confirm, the timer is running due to the captive portal check? Maybe worth a comment perhaps. (And perhaps a EXPECT_TRUE(...captive_portal_checked()) so we'll find out when the comment because false.)
https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/com... File chrome/browser/ssl/common_name_mismatch_handler.cc (right): https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.cc:103: return url_fetcher_; On 2015/08/17 18:59:18, David Benjamin wrote: > You should run a try job to check, but I think MSVC will get unhappy at this and > want something like: > return !!url_fetcher_; You've raised this before David ;) No, pointer->bool is well defined, as are scoped_ptr<> and scoped_refptr<> The only conversion is int/BOOL to bool. Everything else is fine :)
https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/com... File chrome/browser/ssl/common_name_mismatch_handler.cc (right): https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.cc:41: net::URLFetcher::HEAD, this); On 2015/08/17 18:59:18, David Benjamin wrote: > If you're always passing zero, you can use the ID-less > net::URLFetcher::Create(url, net::URLFetcher::HEAD, this); Done. https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.cc:89: const GURL& landing_url = url_fetcher_.get()->GetURL(); On 2015/08/17 18:59:18, David Benjamin wrote: > url_fetcher_->GetOriginalURL(), url_fetcher_->GetURL() Done. https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.cc:99: base::ResetAndReturn(&check_url_callback_).Run(result, suggested_url); On 2015/08/17 18:59:18, David Benjamin wrote: > Using suggested_url here is a UAF since you called url_fetcher_.reset(), you > saved it as a const reference, and URLFetcher itself returns a const reference. > It should be: > > GURL suggested_url = url_fetcher->GetOriginalURL(); Done. https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.cc:103: return url_fetcher_; On 2015/08/17 18:59:18, David Benjamin wrote: > You should run a try job to check, but I think MSVC will get unhappy at this and > want something like: > return !!url_fetcher_; Done. https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/com... File chrome/browser/ssl/common_name_mismatch_handler.h (right): https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.h:65: static void set_state_for_testing(TestingState testing_state) { On 2015/08/17 18:59:18, David Benjamin wrote: > Since this makes you skip the callback, it might be better to do a > set_hang_requests_for_testing(bool) or something? I can't come up with a > terribly clear name. Certainly the comment should mention that the callback > won't get called since that's what happens from the consumer's perspective. > > (Why not URLFetcherFactory?) Updated the comment. Similar method names and signatures are used in case of captive portal, I thought that it might be good to keep them similar to keep consistency. Also, I think that having an enum and setting its value might be better than using just a boolean variable. Thanks. https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:230: } On 2015/08/17 18:59:19, David Benjamin wrote: > Nit: Any reason not to define these inline with the class definition? Saves you > a little bit of repetition. Done. https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:242: if (web_contents_ == web_contents && message_loop_runner_.get()) On 2015/08/17 18:59:18, David Benjamin wrote: > message_loop_runner_.get() is redundant, isn't it? Done. https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2315: typedef CertVerifierBrowserTest CommonNameMismatchBrowserTest; On 2015/08/17 18:59:19, David Benjamin wrote: > Nit: C++11-style is preferred now: > > using CommonNameMismatchBrowserTest = CertVerifierBrowserTest; Done. https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2335: // The pem file does not matter. On 2015/08/17 18:59:18, David Benjamin wrote: > Nit: pem -> PEM > (Although, with the comment below, perhaps you don't need the comment at all.) Done. https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2336: scoped_refptr<net::X509Certificate> cert1 = On 2015/08/17 18:59:19, David Benjamin wrote: > Nit: cert1 -> cert? I don't see a cert2. Done. Initial implementation used to have cert2. Updated. Thanks. https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2337: net::ImportCertFromFile(net::GetTestCertsDirectory(), "ok_cert.pem"); On 2015/08/17 18:59:18, David Benjamin wrote: > This can now just be https_server_example_domain_.GetCertificate(). Done. https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2355: // Request to "www.test.example.com" should not result in any error. On 2015/08/17 18:59:19, David Benjamin wrote: > http://www.test.example.com -> mail.example.com? Done. https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2359: // The path does not matter. On 2015/08/17 18:59:18, David Benjamin wrote: > Nit: I'd maybe write: > > // Use a complex URL to ensure the path, etc., are preserved. The path itself > does not matter. > > (Add a URL #fragment while you're at it? Those are kinda weird and might be > worth testing.) Done. https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2398: net::ImportCertFromFile(net::GetTestCertsDirectory(), "ok_cert.pem"); On 2015/08/17 18:59:19, David Benjamin wrote: > Ditto about GetCertificate and cert1 -> cert Done. https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2455: net::ImportCertFromFile(net::GetTestCertsDirectory(), "ok_cert.pem"); On 2015/08/17 18:59:18, David Benjamin wrote: > Ditto about GetCertificate and cert1 -> cert Done. https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2487: interstitial_timer_observer.WaitForTimerStarted(); On 2015/08/17 18:59:19, David Benjamin wrote: > Since you're mucking about with the timer, this probably should, to ensure we > don't flake: > > SSLErrorHandler::SetInterstitialDelayTypeForTest(SSLErrorHandler::LONG); Done. https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2522: net::ImportCertFromFile(net::GetTestCertsDirectory(), "ok_cert.pem"); On 2015/08/17 18:59:19, David Benjamin wrote: > Ditto about GetCertificate and cert1 -> cert Done. https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2554: interstitial_timer_observer.WaitForTimerStarted(); On 2015/08/17 18:59:19, David Benjamin wrote: > Ditto about timer Done. https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2587: net::ImportCertFromFile(net::GetTestCertsDirectory(), "ok_cert.pem"); On 2015/08/17 18:59:19, David Benjamin wrote: > Ditto about GetCertificate and cert1 -> cert Done. https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_classification.cc:336: || !(IsHostNameKnownTLD(dns_names[i]))) { On 2015/08/17 18:59:19, David Benjamin wrote: > Existing code, but I believe we put the || at the end of the line, so it'd be: > > if (dns_names[i].empty() || > dns_names[i].find('\0') != std::string::npos || > dns_names[i].length() == host_name.length() || > !IsHostNameKnownTLD(dns_names[i])) { > continue; > } Done. https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_classification.cc:341: www_match_host_name->assign(dns_names[i].data(), dns_names[i].size()); On 2015/08/17 18:59:19, David Benjamin wrote: > This can just be: > *www_match_host_name = dns_names[i]; Done. https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_classification.cc:347: www_match_host_name->assign(dns_names[i].data(), dns_names[i].size()); On 2015/08/17 18:59:19, David Benjamin wrote: > Ditto. Done. https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.cc:152: ssl_info_.cert_status ^ net::CERT_STATUS_COMMON_NAME_INVALID; On 2015/08/17 18:59:19, David Benjamin wrote: > What if CERT_STATUS_COMMON_NAME_INVALID isn't in cert_status? Then you're adding > it to extra_cert_errors. If that's intentional, that probably needs a comment to > explain what's going on, or better some more straight-forward logic. :-) The cert_status must have CERT_STATUS_COMMON_NAME_INVALID and no other errors. The current code takes care of that, I added another condition in the above if loop to make it more clear and make an early exit if there is no CERT_STATUS_COMMON_NAME_INVALID error. https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.cc:223: load_params.transition_type = ui::PAGE_TRANSITION_TYPED; Used this since, https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/page_trans... says that TYPED is also used for other explicit navigation actions. I saw that similar method is used for captive portal at https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/cap... Thanks https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler_unittest.cc (left): https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler_unittest.cc:20: #include "testing/gtest/include/gtest/gtest.h" On 2015/08/17 18:59:19, David Benjamin wrote: > This include should still be there due to IWYU. Done. https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): https://codereview.chromium.org/1223233002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler_unittest.cc:244: EXPECT_TRUE(error_handler()->IsTimerRunning()); On 2015/08/17 18:59:19, David Benjamin wrote: > To confirm, the timer is running due to the captive portal check? Maybe worth a > comment perhaps. (And perhaps a EXPECT_TRUE(...captive_portal_checked()) so > we'll find out when the comment because false.) Done.
lgtm https://codereview.chromium.org/1223233002/diff/420001/chrome/browser/ssl/com... File chrome/browser/ssl/common_name_mismatch_handler.cc (right): https://codereview.chromium.org/1223233002/diff/420001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.cc:86: // |suggested_url| and |landing_url| can be different in case of a redirect. Nit: Given this looks like a typo, probably worth explicitly saying "Save a copy of |suggested_url| so it can be used after |url_fetcher_| is destroyed." https://codereview.chromium.org/1223233002/diff/420001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/1223233002/diff/420001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_classification.cc:337: !(IsHostNameKnownTLD(dns_names[i]))) { Nit: Unnecessary parens around IsHostNameKnownTLD.
https://codereview.chromium.org/1223233002/diff/420001/chrome/browser/ssl/com... File chrome/browser/ssl/common_name_mismatch_handler.cc (right): https://codereview.chromium.org/1223233002/diff/420001/chrome/browser/ssl/com... chrome/browser/ssl/common_name_mismatch_handler.cc:86: // |suggested_url| and |landing_url| can be different in case of a redirect. On 2015/08/18 20:19:05, David Benjamin (slow) wrote: > Nit: Given this looks like a typo, probably worth explicitly saying "Save a copy > of |suggested_url| so it can be used after |url_fetcher_| is destroyed." Done. https://codereview.chromium.org/1223233002/diff/420001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_classification.cc (right): https://codereview.chromium.org/1223233002/diff/420001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_classification.cc:337: !(IsHostNameKnownTLD(dns_names[i]))) { On 2015/08/18 20:19:05, David Benjamin (slow) wrote: > Nit: Unnecessary parens around IsHostNameKnownTLD. Done.
The CQ bit was checked by bhanudev@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org, meacer@chromium.org, rsleevi@chromium.org, mmenke@chromium.org, davidben@chromium.org Link to the patchset: https://codereview.chromium.org/1223233002/#ps440001 (title: "Resolving comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1223233002/440001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1223233002/440001
Message was sent while issue was closed.
Committed patchset #23 (id:440001)
Message was sent while issue was closed.
Patchset 23 (id:??) landed as https://crrev.com/2051ce7a01a4026ec1611c940a3d83a55a6e19ef Cr-Commit-Position: refs/heads/master@{#344052} |
