added the allowcertificateerror method to the headless browser client so that it can handle certifi…

added the allowcertificateerror method to the headless browser client so that it can handle certificate errors in headless mode, instead of just stalling

BUG=702107
Review-Url: https://codereview.chromium.org/2853763004
Cr-Commit-Position: refs/heads/master@{#470209}
Committed: https://chromium.googlesource.com/chromium/src/+/fd9f2947de07f272f69edea9da56309d5790701d

[Oleg Sushkov, 2017-05-02 01:02:13]

sushkov@chromium.org changed reviewers:
+ alexclarke@chromium.org

[Oleg Sushkov, 2017-05-02 01:02:14]

Trivially handle certificate errors in headless browser client.

[alex clarke (OOO till 29th), 2017-05-02 10:07:03]

Some commands where added recently to handle certificate errors over DevTools
protocol: https://codereview.chromium.org/2639203003/

Do you think this is this still useful in light of that?  If so I'd suggest
adding this new default behaviour behind a commandline flag.  Also please write
a test if possible.

[Oleg Sushkov, 2017-05-03 04:36:39]

On 2017/05/02 at 10:07:03, alexclarke wrote:
> Some commands where added recently to handle certificate errors over DevTools
protocol: https://codereview.chromium.org/2639203003/
> 
> Do you think this is this still useful in light of that?  If so I'd suggest
adding this new default behaviour behind a commandline flag.  Also please write
a test if possible.

I've added a test.

This change does not conflict with Iris's change but is more complementary. Her
change allows dev-tools to specify whether to allows or deny an SSL error,
instead of a UI popup as would happen in normal Chrome. However, there are other
SSL errors that can happen that dont cause a UI popup in normal chrome. In this
example (https://www.lemonde.fr) the error is that the website is making http
requests (image fetches) even though it is an https url, this being insecure.
The behavour of chrome in this case is simply to ignore this error and log it
(instead of a user-actionable popup). Previously Headless would simply stall
since it never called the target callback. Now I'm just adding the code for
headless in the case of these errors to ignore them. Iris's devtools override
would still work here, but is not a requirement.

[Sami, 2017-05-03 13:27:52]

On 2017/05/03 04:36:39, Oleg Sushkov wrote:
> This change does not conflict with Iris's change but is more complementary.
Her
> change allows dev-tools to specify whether to allows or deny an SSL error,
> instead of a UI popup as would happen in normal Chrome. However, there are
other
> SSL errors that can happen that dont cause a UI popup in normal chrome. In
this
> example (https://www.lemonde.fr) the error is that the website is making http
> requests (image fetches) even though it is an https url, this being insecure.
> The behavour of chrome in this case is simply to ignore this error and log it
> (instead of a user-actionable popup). Previously Headless would simply stall
> since it never called the target callback. Now I'm just adding the code for
> headless in the case of these errors to ignore them. Iris's devtools override
> would still work here, but is not a requirement.

Thanks for the explanation. Looks like even Content Shell fails to load
lemonde.fr for the same reason. Does Iris's DevTools override let us handle
these types of resource fetch failures too? If so, I think we should be strict
here and always fail the request. Otherwise we are silently compromising content
integrity. *Maybe* we could add a command line flag that changes the default,
but I'd prefer to stick with DevTools if possible.

[Oleg Sushkov, 2017-05-04 00:20:07]

On 2017/05/03 at 13:27:52, skyostil wrote:
> On 2017/05/03 04:36:39, Oleg Sushkov wrote:
> > This change does not conflict with Iris's change but is more complementary.
Her
> > change allows dev-tools to specify whether to allows or deny an SSL error,
> > instead of a UI popup as would happen in normal Chrome. However, there are
other
> > SSL errors that can happen that dont cause a UI popup in normal chrome. In
this
> > example (https://www.lemonde.fr) the error is that the website is making
http
> > requests (image fetches) even though it is an https url, this being
insecure.
> > The behavour of chrome in this case is simply to ignore this error and log
it
> > (instead of a user-actionable popup). Previously Headless would simply stall
> > since it never called the target callback. Now I'm just adding the code for
> > headless in the case of these errors to ignore them. Iris's devtools
override
> > would still work here, but is not a requirement.
> 
> Thanks for the explanation. Looks like even Content Shell fails to load
lemonde.fr for the same reason. Does Iris's DevTools override let us handle
these types of resource fetch failures too? If so, I think we should be strict
here and always fail the request. Otherwise we are silently compromising content
integrity. *Maybe* we could add a command line flag that changes the default,
but I'd prefer to stick with DevTools if possible.

I'm ok with that, it won't quite match the behaviour of Chrome for some pages
(which seems to simply ignore some SSL errors), but denying the error is better
than the current behaviour which simply stalls.

PTAL, I've changed the behaviour.

[Oleg Sushkov, 2017-05-08 00:42:11]

On 2017/05/04 at 00:20:07, Oleg Sushkov wrote:
> On 2017/05/03 at 13:27:52, skyostil wrote:
> > On 2017/05/03 04:36:39, Oleg Sushkov wrote:
> > > This change does not conflict with Iris's change but is more
complementary. Her
> > > change allows dev-tools to specify whether to allows or deny an SSL error,
> > > instead of a UI popup as would happen in normal Chrome. However, there are
other
> > > SSL errors that can happen that dont cause a UI popup in normal chrome. In
this
> > > example (https://www.lemonde.fr) the error is that the website is making
http
> > > requests (image fetches) even though it is an https url, this being
insecure.
> > > The behavour of chrome in this case is simply to ignore this error and log
it
> > > (instead of a user-actionable popup). Previously Headless would simply
stall
> > > since it never called the target callback. Now I'm just adding the code
for
> > > headless in the case of these errors to ignore them. Iris's devtools
override
> > > would still work here, but is not a requirement.
> > 
> > Thanks for the explanation. Looks like even Content Shell fails to load
lemonde.fr for the same reason. Does Iris's DevTools override let us handle
these types of resource fetch failures too? If so, I think we should be strict
here and always fail the request. Otherwise we are silently compromising content
integrity. *Maybe* we could add a command line flag that changes the default,
but I'd prefer to stick with DevTools if possible.
> 
> I'm ok with that, it won't quite match the behaviour of Chrome for some pages
(which seems to simply ignore some SSL errors), but denying the error is better
than the current behaviour which simply stalls.
> 
> PTAL, I've changed the behaviour.

*ping*

[Sami, 2017-05-08 18:12:54]

skyostil@chromium.org changed reviewers:
+ skyostil@chromium.org

[Sami, 2017-05-08 18:12:55]

lgtm, thanks for the test!

[alex clarke (OOO till 29th), 2017-05-08 18:48:06]

lgtm

[Oleg Sushkov, 2017-05-09 03:31:10]

The CQ bit was checked by sushkov@chromium.org

[commit-bot: I haz the power, 2017-05-09 03:32:42]

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-05-09 04:34:39]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494300670949870,
"parent_rev": "6a8c3317e754288840610dd169c3a93d4d34aeb1", "commit_rev":
"fd9f2947de07f272f69edea9da56309d5790701d"}

[commit-bot: I haz the power, 2017-05-09 04:34:46]

Description was changed from

==========
added the allowcertificateerror method to the headless browser client so that it
can handle certificate errors in headless mode, instead of just stalling

BUG=702107
==========

to

==========
added the allowcertificateerror method to the headless browser client so that it
can handle certificate errors in headless mode, instead of just stalling

BUG=702107

Review-Url: https://codereview.chromium.org/2853763004
Cr-Commit-Position: refs/heads/master@{#470209}
Committed:
https://chromium.googlesource.com/chromium/src/+/fd9f2947de07f272f69edea9da56...
==========

[commit-bot: I haz the power, 2017-05-09 04:34:48]

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