Issue 294343004: New SSL interstitial (behind Finch flag)

Issue 294343004: New SSL interstitial (behind Finch flag) (Closed)

Created:
6 years, 7 months ago by felt

Modified:
6 years, 6 months ago

Reviewers:
Bernhard Bauer, palmer, edwardjung, Lei Zhang

CC:
chromium-reviews, arv+watch_chromium.org, edwardjung

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

This adds the new SSL interstitial (text & layout) behind a Finch flag. Once complete, this layout will have common parts factored out so that it can be shared across other similar interstitials. (E.g., malware.) But that day is not today. For now, this is behind a Finch flag with the old interstitial running as default. The two will trade places when this version (v2) is launched. BUG=331453 TBR=thestig@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274440

Patch Set 1 #

Patch Set 2 : Working version 1.0 #

Patch Set 3 : Style fix #

Total comments: 16

Patch Set 4 : html/js fixes #

Total comments: 1

Patch Set 5 : Followed edward's suggestions #

Total comments: 6

Patch Set 6 : Hopefully fixed git, made small changes #

Patch Set 7 : Missing copyright #

Patch Set 8 : Removed spurious OVERRIDE #

Created: 6 years, 6 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+199 lines, -4 lines)			Patch
M	chrome/app/generated_resources.grd	View	1 2 3 4 5	1 chunk	+7 lines, -4 lines	0 comments	Download
M	chrome/browser/browser_resources.grd	View	1 2 3 4 5	1 chunk	+1 line, -0 lines	0 comments	Download
A +	chrome/browser/resources/ssl/overridable_v2.css	View	1 2 3 4 5 6	1 chunk	+75 lines, -0 lines	0 comments	Download
A +	chrome/browser/resources/ssl/overridable_v2.html	View	1 2 3 4 5	1 chunk	+31 lines, -0 lines	0 comments	Download
A	chrome/browser/resources/ssl/overridable_v2.js	View	1 2 3 4 5	1 chunk	+31 lines, -0 lines	0 comments	Download
M	chrome/browser/ssl/ssl_blocking_page.h	View	1 2 3 4 5 6 7	1 chunk	+3 lines, -0 lines	0 comments	Download
M	chrome/browser/ssl/ssl_blocking_page.cc	View	1 2 3 4 5	3 chunks	+51 lines, -0 lines	0 comments	Download

Messages

Total messages: 24 (0 generated)

Expand Messages | Collapse Messages

Bernhard Bauer

https://codereview.chromium.org/294343004/diff/40001/chrome/browser/resources/ssl/overridable_v2.html File chrome/browser/resources/ssl/overridable_v2.html (right): https://codereview.chromium.org/294343004/diff/40001/chrome/browser/resources/ssl/overridable_v2.html#newcode2 chrome/browser/resources/ssl/overridable_v2.html:2: <html class="google" lang="en" i18n-values="dir:textDirection"> We always set lang="en"? https://codereview.chromium.org/294343004/diff/40001/chrome/browser/resources/ssl/overridable_v2.html#newcode6 ...

6 years, 7 months ago (2014-05-26 10:13:20 UTC) #2

felt

6 years, 7 months ago (2014-05-27 12:25:05 UTC) #3

Bernhard Bauer

LGTM, with a suggestion below. https://codereview.chromium.org/294343004/diff/40001/chrome/browser/resources/ssl/overridable_v2.html File chrome/browser/resources/ssl/overridable_v2.html (right): https://codereview.chromium.org/294343004/diff/40001/chrome/browser/resources/ssl/overridable_v2.html#newcode22 chrome/browser/resources/ssl/overridable_v2.html:22: <a href="javascript:void(0)" id="details-button" On ...

6 years, 7 months ago (2014-05-27 14:13:33 UTC) #4

felt

https://codereview.chromium.org/294343004/diff/40001/chrome/browser/resources/ssl/overridable_v2.html File chrome/browser/resources/ssl/overridable_v2.html (right): https://codereview.chromium.org/294343004/diff/40001/chrome/browser/resources/ssl/overridable_v2.html#newcode22 chrome/browser/resources/ssl/overridable_v2.html:22: <a href="javascript:void(0)" id="details-button" On 2014/05/27 14:13:34, Bernhard Bauer wrote: ...

6 years, 6 months ago (2014-05-28 12:56:28 UTC) #5

https://codereview.chromium.org/294343004/diff/40001/chrome/browser/resources...
File chrome/browser/resources/ssl/overridable_v2.html (right):

https://codereview.chromium.org/294343004/diff/40001/chrome/browser/resources...
chrome/browser/resources/ssl/overridable_v2.html:22: <a
href="javascript:void(0)" id="details-button"
On 2014/05/27 14:13:34, Bernhard Bauer wrote:
> On 2014/05/27 12:25:06, felt wrote:
> > On 2014/05/26 10:13:20, Bernhard Bauer wrote:
> > > Just set the target to "#"? It's shorter, and you'd be able to set a CSP
> that
> > > disallows inline script.
> > 
> > Done. How do I set a CSP for a Chrome page? Is there a way to set headers or
> is
> > there a special CSP command?
> 
> Hm... The prefered way would be via HTTP headers, but if this is an
interstitial
> page, IIRC it will be directly injected into its WebContents. I think you can
> use meta tags though:
>
https://w3c.github.io/webappsec/specs/content-security-policy/#delivery-html-...

I didn't know about the meta tags, cool. However, I've discovered two problems:

1. Applying a CSP that blocks inline scripts breaks the page. It looks like the
templating system is adding inline scripts. Is it worth trying to figure out
what is going on with the templating? There are no user- or website-supplied
strings in the interstitial.

2. Doing href="#" causes a navigation that isn't permitted for interstitials. I
originally assumed it was some other bug in how the c++ was handling CMD_MORE,
but now I realize it's the "#" navigation. So this needs to be
javascript:void(0) or something else that won't cause a navigation but still
makes the link act like a link.

https://codereview.chromium.org/294343004/diff/40001/chrome/browser/resources...
File chrome/browser/resources/ssl/overridable_v2.js (right):

https://codereview.chromium.org/294343004/diff/40001/chrome/browser/resources...
chrome/browser/resources/ssl/overridable_v2.js:8: var clickEventType =
('ontouchstart' in window ? 'touchend' : 'click');
On 2014/05/27 12:25:06, felt wrote:
> On 2014/05/26 10:13:20, Bernhard Bauer wrote:
> > Does this mean that we *require* a touch if the device supports touches?
> > Couldn't we simply listen for both?
> 
> Oh, right. I guess on devices like a Pixel there are both. Done.

It turns out I just need the 'click', touchscreen devices will send both touch
and click. So I've removed the 'touch' as it is redundant.

Bernhard Bauer

https://codereview.chromium.org/294343004/diff/40001/chrome/browser/resources/ssl/overridable_v2.html File chrome/browser/resources/ssl/overridable_v2.html (right): https://codereview.chromium.org/294343004/diff/40001/chrome/browser/resources/ssl/overridable_v2.html#newcode22 chrome/browser/resources/ssl/overridable_v2.html:22: <a href="javascript:void(0)" id="details-button" On 2014/05/28 12:56:29, felt wrote: > ...

6 years, 6 months ago (2014-05-28 13:17:47 UTC) #6

https://codereview.chromium.org/294343004/diff/40001/chrome/browser/resources...
File chrome/browser/resources/ssl/overridable_v2.html (right):

https://codereview.chromium.org/294343004/diff/40001/chrome/browser/resources...
chrome/browser/resources/ssl/overridable_v2.html:22: <a
href="javascript:void(0)" id="details-button"
On 2014/05/28 12:56:29, felt wrote:
> On 2014/05/27 14:13:34, Bernhard Bauer wrote:
> > On 2014/05/27 12:25:06, felt wrote:
> > > On 2014/05/26 10:13:20, Bernhard Bauer wrote:
> > > > Just set the target to "#"? It's shorter, and you'd be able to set a CSP
> > that
> > > > disallows inline script.
> > > 
> > > Done. How do I set a CSP for a Chrome page? Is there a way to set headers
or
> > is
> > > there a special CSP command?
> > 
> > Hm... The prefered way would be via HTTP headers, but if this is an
> interstitial
> > page, IIRC it will be directly injected into its WebContents. I think you
can
> > use meta tags though:
> >
>
https://w3c.github.io/webappsec/specs/content-security-policy/#delivery-html-...
> 
> I didn't know about the meta tags, cool. However, I've discovered two
problems:
> 
> 1. Applying a CSP that blocks inline scripts breaks the page. It looks like
the
> templating system is adding inline scripts. Is it worth trying to figure out
> what is going on with the templating? There are no user- or website-supplied
> strings in the interstitial.

Hm, I don't know any reason why the templating system should add inline scripts.
If this is a general problem with i18n_template.js, it might be worth fixing, as
that would allow other WebUI pages to apply CSP too.

One thing I noticed is that you're using v1 of i18n_template.js. There is a
newer version, which you'll get if you have an object of class
webui::UseVersion2 in scope (don't ask me why). Maybe that will help already?

> 2. Doing href="#" causes a navigation that isn't permitted for interstitials.
I
> originally assumed it was some other bug in how the c++ was handling CMD_MORE,
> but now I realize it's the "#" navigation. So this needs to be
> javascript:void(0) or something else that won't cause a navigation but still
> makes the link act like a link.

Oh, ok. Hm, to get rid of the inline script, maybe you could keep href="#" but
set an event listener that will cancel the navigation?

https://codereview.chromium.org/294343004/diff/40001/chrome/browser/resources...
File chrome/browser/resources/ssl/overridable_v2.js (right):

https://codereview.chromium.org/294343004/diff/40001/chrome/browser/resources...
chrome/browser/resources/ssl/overridable_v2.js:8: var clickEventType =
('ontouchstart' in window ? 'touchend' : 'click');
On 2014/05/28 12:56:29, felt wrote:
> On 2014/05/27 12:25:06, felt wrote:
> > On 2014/05/26 10:13:20, Bernhard Bauer wrote:
> > > Does this mean that we *require* a touch if the device supports touches?
> > > Couldn't we simply listen for both?
> > 
> > Oh, right. I guess on devices like a Pixel there are both. Done.
> 
> It turns out I just need the 'click', touchscreen devices will send both touch
> and click. So I've removed the 'touch' as it is redundant.

OK, cool.

https://codereview.chromium.org/294343004/diff/80001/chrome/browser/resources...
File chrome/browser/resources/ssl/overridable_v2.html (right):

https://codereview.chromium.org/294343004/diff/80001/chrome/browser/resources...
chrome/browser/resources/ssl/overridable_v2.html:13: <body
il18n-values=".style.fontFamily:fontFamily;.style.fontSize:fontsize">
i18n-values.

edwardjung

I had a couple of comments on the CSS. https://codereview.chromium.org/294343004/diff/80001/chrome/browser/resources/ssl/overridable_v2.css File chrome/browser/resources/ssl/overridable_v2.css (right): https://codereview.chromium.org/294343004/diff/80001/chrome/browser/resources/ssl/overridable_v2.css#newcode14 chrome/browser/resources/ssl/overridable_v2.css:14: ...

6 years, 6 months ago (2014-05-28 13:41:27 UTC) #7

felt

https://codereview.chromium.org/294343004/diff/40001/chrome/browser/resources/ssl/overridable_v2.html File chrome/browser/resources/ssl/overridable_v2.html (right): https://codereview.chromium.org/294343004/diff/40001/chrome/browser/resources/ssl/overridable_v2.html#newcode22 chrome/browser/resources/ssl/overridable_v2.html:22: <a href="javascript:void(0)" id="details-button" On 2014/05/28 13:17:47, Bernhard Bauer wrote: ...

6 years, 6 months ago (2014-06-02 17:58:46 UTC) #8

https://codereview.chromium.org/294343004/diff/40001/chrome/browser/resources...
File chrome/browser/resources/ssl/overridable_v2.html (right):

https://codereview.chromium.org/294343004/diff/40001/chrome/browser/resources...
chrome/browser/resources/ssl/overridable_v2.html:22: <a
href="javascript:void(0)" id="details-button"
On 2014/05/28 13:17:47, Bernhard Bauer wrote:
> On 2014/05/28 12:56:29, felt wrote:
> > On 2014/05/27 14:13:34, Bernhard Bauer wrote:
> > > On 2014/05/27 12:25:06, felt wrote:
> > > > On 2014/05/26 10:13:20, Bernhard Bauer wrote:
> > > > > Just set the target to "#"? It's shorter, and you'd be able to set a
CSP
> > > that
> > > > > disallows inline script.
> > > > 
> > > > Done. How do I set a CSP for a Chrome page? Is there a way to set
headers
> or
> > > is
> > > > there a special CSP command?
> > > 
> > > Hm... The prefered way would be via HTTP headers, but if this is an
> > interstitial
> > > page, IIRC it will be directly injected into its WebContents. I think you
> can
> > > use meta tags though:
> > >
> >
>
https://w3c.github.io/webappsec/specs/content-security-policy/#delivery-html-...
> > 
> > I didn't know about the meta tags, cool. However, I've discovered two
> problems:
> > 
> > 1. Applying a CSP that blocks inline scripts breaks the page. It looks like
> the
> > templating system is adding inline scripts. Is it worth trying to figure out
> > what is going on with the templating? There are no user- or website-supplied
> > strings in the interstitial.
> 
> Hm, I don't know any reason why the templating system should add inline
scripts.
> If this is a general problem with i18n_template.js, it might be worth fixing,
as
> that would allow other WebUI pages to apply CSP too.
> 
> One thing I noticed is that you're using v1 of i18n_template.js. There is a
> newer version, which you'll get if you have an object of class
> webui::UseVersion2 in scope (don't ask me why). Maybe that will help already?
> 
> > 2. Doing href="#" causes a navigation that isn't permitted for
interstitials.
> I
> > originally assumed it was some other bug in how the c++ was handling
CMD_MORE,
> > but now I realize it's the "#" navigation. So this needs to be
> > javascript:void(0) or something else that won't cause a navigation but still
> > makes the link act like a link.
> 
> Oh, ok. Hm, to get rid of the inline script, maybe you could keep href="#" but
> set an event listener that will cancel the navigation?

I've updated this to use the v2 and removed the inline script in the <a> tag,
but I keep getting CSP errors saying there is an inline script. I assume this
must be something that's being done dynamically by one of the scripts I've
included. I will investigate further, but I'd like to land this without the CSP.
It's not particularly urgent here given that there is no untrusted content on
the page.

https://codereview.chromium.org/294343004/diff/80001/chrome/browser/resources...
File chrome/browser/resources/ssl/overridable_v2.css (right):

https://codereview.chromium.org/294343004/diff/80001/chrome/browser/resources...
chrome/browser/resources/ssl/overridable_v2.css:14: border-box: box-sizing;
On 2014/05/28 13:41:27, edwardjung wrote:
> I know this is my fault, should be the other way round.
> 
> box-sizing: border-box;
> 

Done.

https://codereview.chromium.org/294343004/diff/80001/chrome/browser/resources...
chrome/browser/resources/ssl/overridable_v2.css:28: font-size: 22pt;
On 2014/05/28 13:41:27, edwardjung wrote:
> For accessibility I would use relative font sizes (em's or %). If the user has
> the default font size set to something else like 'larger' the font-sizes would
> scale up accordingly, but not if using absolute sizes.
> 
> Same for the line-height too defined here and the body.

Switched to using em everywhere.

https://codereview.chromium.org/294343004/diff/80001/chrome/browser/resources...
File chrome/browser/resources/ssl/overridable_v2.html (right):

https://codereview.chromium.org/294343004/diff/80001/chrome/browser/resources...
chrome/browser/resources/ssl/overridable_v2.html:13: <body
il18n-values=".style.fontFamily:fontFamily;.style.fontSize:fontsize">
On 2014/05/28 13:17:48, Bernhard Bauer wrote:
> i18n-values.

No wonder! I thought perhaps the default font on Linux was just really ugly. ;)

felt

Something exploded in my git checkout, uploading failed. Hold on

6 years, 6 months ago (2014-06-02 18:06:37 UTC) #10

felt

On 2014/06/02 18:06:37, felt wrote: > Something exploded in my git checkout, uploading failed. Hold ...

6 years, 6 months ago (2014-06-02 18:46:28 UTC) #11