Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(13)

Issue 2690333006: Captive portal certificate list should be checked when name mismatch is the only error (Closed)

Created:
3 years, 10 months ago by meacer
Modified:
3 years, 9 months ago
Reviewers:
estark
CC:
chromium-reviews
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Captive portal certificate list should be checked when name mismatch is the only error When there are errors in addition to a name-mismatch error, the captive portal certificate list feature should not display a captive portal error. This was supposed to be the case with the current implementation but it's buggy. The current check is done by checking cert_error in SSLErrorHandler which is not sufficient, because a cert_error of CERT_ERROR_COMMON_NAME_INVALID doesn't necessarily mean that it's the only error, it means that net::MapCertStatusToNetError determined CERT_ERROR_COMMON_NAME_INVALID to be the most important error. There are also existing test cases that were trying to test this behavior, but they were using authority-invalid errors to do so. This CL adds an extra test to check for name-mismatch and weak key errors. BUG=691163 Review-Url: https://codereview.chromium.org/2690333006 Cr-Commit-Position: refs/heads/master@{#454177} Committed: https://chromium.googlesource.com/chromium/src/+/4f8acbd89df62b20df694810ec309c1bf1e0d6c9

Patch Set 1 #

Total comments: 7

Patch Set 2 : estark comments #

Total comments: 10

Patch Set 3 : estark comments and rebase #

Total comments: 6

Patch Set 4 : estark comments #

Patch Set 5 : Fix build #

Patch Set 6 : Fix Android tests #

Unified diffs Side-by-side diffs Delta from patch set Stats (+251 lines, -120 lines) Patch
M chrome/browser/ssl/ssl_browser_tests.cc View 1 2 3 4 chunks +55 lines, -35 lines 0 comments Download
M chrome/browser/ssl/ssl_error_handler.cc View 1 2 3 2 chunks +14 lines, -6 lines 0 comments Download
M chrome/browser/ssl/ssl_error_handler_unittest.cc View 1 2 3 4 5 7 chunks +182 lines, -79 lines 0 comments Download

Messages

Total messages: 30 (13 generated)
meacer
estark: PTAL? Thanks.
3 years, 10 months ago (2017-02-14 23:20:24 UTC) #3
estark
https://codereview.chromium.org/2690333006/diff/1/chrome/browser/ssl/ssl_browser_tests.cc File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2690333006/diff/1/chrome/browser/ssl/ssl_browser_tests.cc#newcode4303 chrome/browser/ssl/ssl_browser_tests.cc:4303: net::CERT_STATUS_COMMON_NAME_INVALID | net::CERT_STATUS_WEAK_KEY, nit: it might be good to ...
3 years, 10 months ago (2017-02-15 00:31:41 UTC) #4
meacer
https://codereview.chromium.org/2690333006/diff/1/chrome/browser/ssl/ssl_browser_tests.cc File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2690333006/diff/1/chrome/browser/ssl/ssl_browser_tests.cc#newcode4303 chrome/browser/ssl/ssl_browser_tests.cc:4303: net::CERT_STATUS_COMMON_NAME_INVALID | net::CERT_STATUS_WEAK_KEY, On 2017/02/15 00:31:41, estark wrote: > ...
3 years, 10 months ago (2017-02-15 00:37:29 UTC) #5
estark
On 2017/02/15 00:37:29, Mustafa Emre Acer wrote: > https://codereview.chromium.org/2690333006/diff/1/chrome/browser/ssl/ssl_browser_tests.cc > File chrome/browser/ssl/ssl_browser_tests.cc (right): > > ...
3 years, 10 months ago (2017-02-16 07:01:33 UTC) #7
meacer
On 2017/02/16 07:01:33, estark (slow thru Feb 22) wrote: > On 2017/02/15 00:37:29, Mustafa Emre ...
3 years, 10 months ago (2017-02-23 23:27:51 UTC) #8
meacer
(and the comments) https://codereview.chromium.org/2690333006/diff/1/chrome/browser/ssl/ssl_browser_tests.cc File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2690333006/diff/1/chrome/browser/ssl/ssl_browser_tests.cc#newcode4303 chrome/browser/ssl/ssl_browser_tests.cc:4303: net::CERT_STATUS_COMMON_NAME_INVALID | net::CERT_STATUS_WEAK_KEY, Done. https://codereview.chromium.org/2690333006/diff/1/chrome/browser/ssl/ssl_error_handler_unittest.cc File ...
3 years, 10 months ago (2017-02-23 23:28:20 UTC) #9
estark
lgtm, thanks for removing the test getter! https://codereview.chromium.org/2690333006/diff/20001/chrome/browser/ssl/ssl_browser_tests.cc File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2690333006/diff/20001/chrome/browser/ssl/ssl_browser_tests.cc#newcode4304 chrome/browser/ssl/ssl_browser_tests.cc:4304: EXPECT_EQ(net::ERR_CERT_COMMON_NAME_INVALID, nit: ...
3 years, 10 months ago (2017-02-25 01:33:18 UTC) #10
meacer
https://codereview.chromium.org/2690333006/diff/20001/chrome/browser/ssl/ssl_browser_tests.cc File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2690333006/diff/20001/chrome/browser/ssl/ssl_browser_tests.cc#newcode4304 chrome/browser/ssl/ssl_browser_tests.cc:4304: EXPECT_EQ(net::ERR_CERT_COMMON_NAME_INVALID, On 2017/02/25 01:33:17, estark wrote: > nit: maybe ...
3 years, 9 months ago (2017-02-27 23:50:58 UTC) #11
meacer
estark: I splitted some tests so you might want to take another quick look.
3 years, 9 months ago (2017-02-28 21:04:34 UTC) #12
estark
lgtm https://codereview.chromium.org/2690333006/diff/40001/chrome/browser/ssl/ssl_browser_tests.cc File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2690333006/diff/40001/chrome/browser/ssl/ssl_browser_tests.cc#newcode4364 chrome/browser/ssl/ssl_browser_tests.cc:4364: // Sanity check that COMMON_NAME_INVALID is seen as ...
3 years, 9 months ago (2017-02-28 21:12:06 UTC) #13
meacer
Thanks! https://codereview.chromium.org/2690333006/diff/40001/chrome/browser/ssl/ssl_browser_tests.cc File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2690333006/diff/40001/chrome/browser/ssl/ssl_browser_tests.cc#newcode4364 chrome/browser/ssl/ssl_browser_tests.cc:4364: // Sanity check that COMMON_NAME_INVALID is seen as ...
3 years, 9 months ago (2017-02-28 23:57:01 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2690333006/60001
3 years, 9 months ago (2017-02-28 23:57:33 UTC) #17
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_compile_dbg_ng/builds/358095)
3 years, 9 months ago (2017-03-01 00:30:46 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2690333006/80001
3 years, 9 months ago (2017-03-01 02:01:07 UTC) #22
commit-bot: I haz the power
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_swarming_rel/builds/128390)
3 years, 9 months ago (2017-03-01 03:27:13 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2690333006/100001
3 years, 9 months ago (2017-03-02 02:29:22 UTC) #27
commit-bot: I haz the power
3 years, 9 months ago (2017-03-02 04:25:21 UTC) #30
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/4f8acbd89df62b20df694810ec30...

Powered by Google App Engine
This is Rietveld 408576698