Disable Name Mismatch redirection for non-overridable SSL errors

Disable name mismatch redirection for non-overridable SSL errors

BUG=507454
This CL ensures that we do not do name mismatch redirection for HSTS enabled sites. Also, we make sure that redirection is done only when Common Name Invalid is the only error present. This automatically ensures that SSL pinning failures are not handled by name mismatch redirection.

Committed: https://crrev.com/3999e3789ae5924180a25ba5002e8eb0c8d9bff2
Cr-Commit-Position: refs/heads/master@{#344715}

[Bhanu Dev, 2015-08-20 20:29:23]

bhanudev@google.com changed reviewers:
+ meacer@chromium.org

[Bhanu Dev, 2015-08-20 20:31:54]

HSTS is checked as a part of deciding Overridable and Non overridable errors.
So, do WWW Mismatch handling only for overridable errors. SSL pinning failure is
a separate cert error, since we do this if Common Name Invalid error  is the
only error present, it is already taken care of.

[meacer, 2015-08-20 20:51:34]

On 2015/08/20 20:31:54, Bhanu Dev wrote:
> HSTS is checked as a part of deciding Overridable and Non overridable errors.
> So, do WWW Mismatch handling only for overridable errors. SSL pinning failure
is
> a separate cert error, since we do this if Common Name Invalid error  is the
> only error present, it is already taken care of.

^ You should put this summary in the description :)

[meacer, 2015-08-20 20:51:39]

https://codereview.chromium.org/1294673005/diff/1/chrome/browser/ssl/ssl_erro...
File chrome/browser/ssl/ssl_error_handler.cc (right):

https://codereview.chromium.org/1294673005/diff/1/chrome/browser/ssl/ssl_erro...
chrome/browser/ssl/ssl_error_handler.cc:159:
SSLBlockingPage::IsOverridable(options_mask_, profile_) &&
Can you add a unit test for this?

[Bhanu Dev, 2015-08-20 22:45:06]

https://codereview.chromium.org/1294673005/diff/1/chrome/browser/ssl/ssl_erro...
File chrome/browser/ssl/ssl_error_handler.cc (right):

https://codereview.chromium.org/1294673005/diff/1/chrome/browser/ssl/ssl_erro...
chrome/browser/ssl/ssl_error_handler.cc:159:
SSLBlockingPage::IsOverridable(options_mask_, profile_) &&
On 2015/08/20 20:51:39, Mustafa Emre Acer wrote:
> Can you add a unit test for this?

Done.

[meacer, 2015-08-20 23:15:22]

https://codereview.chromium.org/1294673005/diff/20001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_error_handler.h (right):

https://codereview.chromium.org/1294673005/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_handler.h:107: virtual bool IsOverridable();
nit: Rename this to IsErrorOverridable()

https://codereview.chromium.org/1294673005/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_handler.h:107: virtual bool IsOverridable();
const method

https://codereview.chromium.org/1294673005/diff/20001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_error_handler_unittest.cc (right):

https://codereview.chromium.org/1294673005/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_handler_unittest.cc:104: void SetOverridable(bool
is_overridable) { is_overridable_ = is_overridable; }
lower case for simple setters (set_overridable_error). Actually, name this
set_non_overridable_error() since is_overridable param is never used as |true|.

https://codereview.chromium.org/1294673005/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_handler_unittest.cc:113: is_overridable_ = true;
This method should only clear the counters. Each test case should create a new
error_handler() so you shouldn't need to set |is_overridable_| to true here.

https://codereview.chromium.org/1294673005/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_handler_unittest.cc:144: bool is_overridable_;
nit: Rename to is_overridable_error_

https://codereview.chromium.org/1294673005/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_handler_unittest.cc:278:
TEST_F(SSLErrorHandlerTest, ShouldNotCheckSuggestedUrlOnNonOverridableError) {
nit: Change the test name to ShouldNotHandleNameMismatchOnNonOverridableError.

[Bhanu Dev, 2015-08-21 00:52:16]

https://codereview.chromium.org/1294673005/diff/20001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_error_handler.h (right):

https://codereview.chromium.org/1294673005/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_handler.h:107: virtual bool IsOverridable();
On 2015/08/20 23:15:22, Mustafa Emre Acer wrote:
> nit: Rename this to IsErrorOverridable()

Done.

https://codereview.chromium.org/1294673005/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_handler.h:107: virtual bool IsOverridable();
On 2015/08/20 23:15:22, Mustafa Emre Acer wrote:
> const method

Done.

https://codereview.chromium.org/1294673005/diff/20001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_error_handler_unittest.cc (right):

https://codereview.chromium.org/1294673005/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_handler_unittest.cc:104: void SetOverridable(bool
is_overridable) { is_overridable_ = is_overridable; }
On 2015/08/20 23:15:22, Mustafa Emre Acer wrote:
> lower case for simple setters (set_overridable_error). Actually, name this
> set_non_overridable_error() since is_overridable param is never used as
|true|.

Done.

https://codereview.chromium.org/1294673005/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_handler_unittest.cc:113: is_overridable_ = true;
On 2015/08/20 23:15:22, Mustafa Emre Acer wrote:
> This method should only clear the counters. Each test case should create a new
> error_handler() so you shouldn't need to set |is_overridable_| to true here.

Done.

https://codereview.chromium.org/1294673005/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_handler_unittest.cc:144: bool is_overridable_;
On 2015/08/20 23:15:22, Mustafa Emre Acer wrote:
> nit: Rename to is_overridable_error_

Done.

https://codereview.chromium.org/1294673005/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_handler_unittest.cc:278:
TEST_F(SSLErrorHandlerTest, ShouldNotCheckSuggestedUrlOnNonOverridableError) {
On 2015/08/20 23:15:22, Mustafa Emre Acer wrote:
> nit: Change the test name to ShouldNotHandleNameMismatchOnNonOverridableError.

Done.

[meacer, 2015-08-21 01:00:10]

LGTM, please add a better CL description for the first line. (e.g. "Disable name
mismatch redirection for non-overridable SSL errors")

https://codereview.chromium.org/1294673005/diff/40001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_error_handler_unittest.cc (right):

https://codereview.chromium.org/1294673005/diff/40001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_handler_unittest.cc:279:
error_handler()->set_non_overridable_error();
nit: Move this to the first line, so that it's a bit clearer what the test is
doing.

[Bhanu Dev, 2015-08-21 01:11:26]

https://codereview.chromium.org/1294673005/diff/40001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_error_handler_unittest.cc (right):

https://codereview.chromium.org/1294673005/diff/40001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_error_handler_unittest.cc:279:
error_handler()->set_non_overridable_error();
On 2015/08/21 01:00:10, Mustafa Emre Acer wrote:
> nit: Move this to the first line, so that it's a bit clearer what the test is
> doing.

Done.

[Bhanu Dev, 2015-08-21 02:00:41]

The CQ bit was checked by bhanudev@google.com

[Bhanu Dev, 2015-08-21 02:00:41]

The patchset sent to the CQ was uploaded after l-g-t-m from meacer@chromium.org
Link to the patchset: https://codereview.chromium.org/1294673005/#ps60001
(title: "Comments, Rebase")

[commit-bot: I haz the power, 2015-08-21 02:01:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1294673005/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1294673005/60001

[commit-bot: I haz the power, 2015-08-21 03:36:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-21 03:36:33]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Bhanu Dev, 2015-08-21 04:16:35]

The CQ bit was checked by bhanudev@google.com

[commit-bot: I haz the power, 2015-08-21 04:17:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1294673005/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1294673005/60001

[Bhanu Dev, 2015-08-21 05:20:32]

The CQ bit was unchecked by bhanudev@google.com

[Bhanu Dev, 2015-08-21 05:38:03]

The CQ bit was checked by bhanudev@google.com

[Bhanu Dev, 2015-08-21 05:38:03]

The patchset sent to the CQ was uploaded after l-g-t-m from meacer@chromium.org
Link to the patchset: https://codereview.chromium.org/1294673005/#ps80001
(title: "Rebase Again")

[commit-bot: I haz the power, 2015-08-21 05:38:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1294673005/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1294673005/80001

[commit-bot: I haz the power, 2015-08-21 07:23:20]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-08-21 07:23:54]

Patchset 5 (id:??) landed as
https://crrev.com/3999e3789ae5924180a25ba5002e8eb0c8d9bff2
Cr-Commit-Position: refs/heads/master@{#344715}