Add pinned cert interstitial to chrome://interstitials

Add pinned cert interstitial to chrome://interstitials

BUG=642935
TEST=Spin up an instance of Chrome and navigate to chrome://interstitials. Click on "Pinned certificate error." A full page interstitial should appear. Check to make sure the header reads "Privacy error" and the error code is "NET::ERR_SSL_PINNED_KEY_NOT_IN_CERT_CHAIN." See also interstitial_ui_browsertest.cc.

Review-Url: https://codereview.chromium.org/2957953002
Cr-Commit-Position: refs/heads/master@{#482810}
Committed: https://chromium.googlesource.com/chromium/src/+/6022f5947cba1db0d2f7f236d7de307955adf12a

[sperigo, 2017-06-26 23:42:15]

sperigo@chromium.org changed reviewers:
+ lgarron@chromium.org, meacer@chromium.org

[sperigo, 2017-06-26 23:43:54]

I added a link to the chrome://interstitials page and rendered the pinned
certificate interstitial conditionally based on the example the Safe Browsing
team used in the same file. Please let me know if you were imagining something
different!

[lgarron, 2017-06-27 00:56:00]

Looks nice and clean.

Could you also do the following?
- Add a screenshot on the bug.
- Make the test instructions self-sufficient, especially for people not familiar
with Chrome's code. In this case, it's probably enough to tell testers to visit
chrome://interstitials and check for the error code
NET::ERR_SSL_PINNED_KEY_NOT_IN_CERT_CHAIN and one or two specific strings to
make sure the right interstitial is showing.

https://codereview.chromium.org/2957953002/diff/1/chrome/browser/ui/webui/int...
File chrome/browser/ui/webui/interstitials/interstitial_ui.cc (right):

https://codereview.chromium.org/2957953002/diff/1/chrome/browser/ui/webui/int...
chrome/browser/ui/webui/interstitials/interstitial_ui.cc:158: if (type_param ==
"pinned_cert") {
"pinned_cert" is still a bit general. How about "pinning_failure" or
"hpkp_failure"?

[sperigo, 2017-06-27 17:35:12]

Description was changed from

==========
Add pinned cert interstitial to chrome://interstitials

BUG=642935

TEST=Spin up an instance of Chrome and navigate to chrome://interstitials. Click
on "Pinned certificate error" and observe behavior. See also
interstitial_ui_browsertest.cc.
==========

to

==========
Add pinned cert interstitial to chrome://interstitials

BUG=642935

TEST=Spin up an instance of Chrome and navigate to chrome://interstitials. Click
on "Pinned certificate error." A full page interstitial should appear. Check to
make sure the header reads "Privacy error" and the error code is
"NET::ERR_SSL_PINNED_KEY_NOT_IN_CERT_CHAIN." See also
interstitial_ui_browsertest.cc.
==========

[sperigo, 2017-06-27 18:57:28]

Thank you! Updated all three of these things.

On 2017/06/27 00:56:00, lgarron wrote:
> Looks nice and clean.
> 
> Could you also do the following?
> - Add a screenshot on the bug.
> - Make the test instructions self-sufficient, especially for people not
familiar
> with Chrome's code. In this case, it's probably enough to tell testers to
visit
> chrome://interstitials and check for the error code
> NET::ERR_SSL_PINNED_KEY_NOT_IN_CERT_CHAIN and one or two specific strings to
> make sure the right interstitial is showing.
> 
>
https://codereview.chromium.org/2957953002/diff/1/chrome/browser/ui/webui/int...
> File chrome/browser/ui/webui/interstitials/interstitial_ui.cc (right):
> 
>
https://codereview.chromium.org/2957953002/diff/1/chrome/browser/ui/webui/int...
> chrome/browser/ui/webui/interstitials/interstitial_ui.cc:158: if (type_param
==
> "pinned_cert") {
> "pinned_cert" is still a bit general. How about "pinning_failure" or
> "hpkp_failure"?

[lgarron, 2017-06-27 20:41:45]

LGTM (mod typo)

https://codereview.chromium.org/2957953002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/interstitials/interstitial_ui.cc (right):

https://codereview.chromium.org/2957953002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/interstitials/interstitial_ui.cc:158: if (type_param ==
"pinned_cert") {
This should match `hpkp_failure` below, right?

[meacer, 2017-06-27 21:10:17]

Lgtm

https://codereview.chromium.org/2957953002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/interstitials/interstitial_ui_browsertest.cc
(right):

https://codereview.chromium.org/2957953002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/interstitials/interstitial_ui_browsertest.cc:26:
browser()->tab_strip_model()->GetActiveWebContents()->GetTitle());
nit: Not necessary for this CL, but we might also consider checking the text
inside the interstitial since most interstitials have the same "Privacy error"
title. There is a method called IsInterstitialDisplayingText() that we can use
for this.

https://codereview.chromium.org/2957953002/diff/20001/components/security_int...
File
components/security_interstitials/core/browser/resources/list_of_interstitials.html
(right):

https://codereview.chromium.org/2957953002/diff/20001/components/security_int...
components/security_interstitials/core/browser/resources/list_of_interstitials.html:44:
<a href="ssl?overridable=0&strict_enforcement=0&type=hpkp_failure">Pinned
certificate error</a>
nit: Do we actually need overridable=0 and strict_enforcement=0 parameters?
Shouldn't type=hpkp_failure already imply them? (well, it implies
strict_enforcement=1, but still)

[sperigo, 2017-06-27 21:31:27]

https://codereview.chromium.org/2957953002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/interstitials/interstitial_ui.cc (right):

https://codereview.chromium.org/2957953002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/interstitials/interstitial_ui.cc:158: if (type_param ==
"pinned_cert") {
On 2017/06/27 20:41:44, lgarron wrote:
> This should match `hpkp_failure` below, right?

Oops! Definitely.

https://codereview.chromium.org/2957953002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/interstitials/interstitial_ui_browsertest.cc
(right):

https://codereview.chromium.org/2957953002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/interstitials/interstitial_ui_browsertest.cc:26:
browser()->tab_strip_model()->GetActiveWebContents()->GetTitle());
On 2017/06/27 21:10:17, meacer wrote:
> nit: Not necessary for this CL, but we might also consider checking the text
> inside the interstitial since most interstitials have the same "Privacy error"
> title. There is a method called IsInterstitialDisplayingText() that we can use
> for this.

Good idea. I noticed these tests were pretty bare. Should I file that as a bug
for future reference?

https://codereview.chromium.org/2957953002/diff/20001/components/security_int...
File
components/security_interstitials/core/browser/resources/list_of_interstitials.html
(right):

https://codereview.chromium.org/2957953002/diff/20001/components/security_int...
components/security_interstitials/core/browser/resources/list_of_interstitials.html:44:
<a href="ssl?overridable=0&strict_enforcement=0&type=hpkp_failure">Pinned
certificate error</a>
On 2017/06/27 21:10:17, meacer wrote:
> nit: Do we actually need overridable=0 and strict_enforcement=0 parameters?
> Shouldn't type=hpkp_failure already imply them? (well, it implies
> strict_enforcement=1, but still)

Doesn't look like it! I removed them.

[meacer, 2017-06-27 21:32:51]

Thanks!

https://codereview.chromium.org/2957953002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/interstitials/interstitial_ui_browsertest.cc
(right):

https://codereview.chromium.org/2957953002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/interstitials/interstitial_ui_browsertest.cc:26:
browser()->tab_strip_model()->GetActiveWebContents()->GetTitle());
On 2017/06/27 21:31:27, sperigo wrote:
> On 2017/06/27 21:10:17, meacer wrote:
> > nit: Not necessary for this CL, but we might also consider checking the text
> > inside the interstitial since most interstitials have the same "Privacy
error"
> > title. There is a method called IsInterstitialDisplayingText() that we can
use
> > for this.
> 
> Good idea. I noticed these tests were pretty bare. Should I file that as a bug
> for future reference?

Sure, feel free to file as a new bug.

[sperigo, 2017-06-27 22:57:59]

The CQ bit was checked by sperigo@chromium.org

[sperigo, 2017-06-27 22:58:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
lgarron@chromium.org, meacer@chromium.org
Link to the patchset: https://codereview.chromium.org/2957953002/#ps40001
(title: "Address CL comments")

[commit-bot: I haz the power, 2017-06-27 22:58:12]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-28 00:15:51]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1498604279904200,
"parent_rev": "5c4dca1c498431022f19bb2f32a6ea73c0cb3b05", "commit_rev":
"6022f5947cba1db0d2f7f236d7de307955adf12a"}

[commit-bot: I haz the power, 2017-06-28 00:16:05]

Description was changed from

==========
Add pinned cert interstitial to chrome://interstitials

BUG=642935

TEST=Spin up an instance of Chrome and navigate to chrome://interstitials. Click
on "Pinned certificate error." A full page interstitial should appear. Check to
make sure the header reads "Privacy error" and the error code is
"NET::ERR_SSL_PINNED_KEY_NOT_IN_CERT_CHAIN." See also
interstitial_ui_browsertest.cc.
==========

to

==========
Add pinned cert interstitial to chrome://interstitials

BUG=642935

TEST=Spin up an instance of Chrome and navigate to chrome://interstitials. Click
on "Pinned certificate error." A full page interstitial should appear. Check to
make sure the header reads "Privacy error" and the error code is
"NET::ERR_SSL_PINNED_KEY_NOT_IN_CERT_CHAIN." See also
interstitial_ui_browsertest.cc.

Review-Url: https://codereview.chromium.org/2957953002
Cr-Commit-Position: refs/heads/master@{#482810}
Committed:
https://chromium.googlesource.com/chromium/src/+/6022f5947cba1db0d2f7f236d7de...
==========

[commit-bot: I haz the power, 2017-06-28 00:16:06]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/6022f5947cba1db0d2f7f236d7de...