Add (some) password detection for HTTP-bad

Add (some) password detection for HTTP-bad

This CL adds WebContents methods for setting the new SSLStatus flags to
record a password or credit card field on an HTTP page. It also
(roughly) integrates with the ContentPasswordManagerDriver code to call
the new WebContents methods. The integration as written at present will
only set the SSLStatus flags whenever a password field is parsed;
eventually we will refine that to set the flags whenever a password field
is visible on the page (https://codereview.chromium.org/2378503002/).

BUG=647560
Committed: https://crrev.com/703f1b43a576ef25b5e267beaaba5f6ef70df08f
Cr-Commit-Position: refs/heads/master@{#424251}

[estark, 2016-09-21 23:15:39]

estark@chromium.org changed reviewers:
+ felt@chromium.org, jam@chromium.org

[estark, 2016-09-21 23:15:39]

felt, jam: I wanted to get some preliminary feedback on this unfinished CL to
hook up the password/credit card SSLStatus flags. (In particular, I haven't
written any tests yet.)

I feel sad that these methods have to be public WebContents methods so that the
password manager code can call them. Our handling of password/cc fields
parallels our handling of passive mixed content (which I like), but in the case
of mixed content, it's driven by IPCs so the methods can be on WebContentsImpl
instead of WebContents (which I don't like).

I think there has to be some method in the content public API for the password
manager to call to get these flags set, but it doesn't necessarily have to be on
WebContents -- it could just as well be on NavigationController or
NavigationEntry. Any thoughts?

Thanks.

[jam, 2016-09-22 03:46:36]

lgtm

I think WebContents is a fine place to put them, since the goal is to have
boolean members on WC holding this state.

[felt, 2016-09-22 16:28:10]

I defer to jam on this; I don't have a good intuition about the zen of layering
and public WC methods.

[estark, 2016-09-22 21:29:53]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-22 21:30:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[estark, 2016-09-22 21:30:27]

Description was changed from

==========
[DO NOT LAND] Rough draft of password detection for HTTP-bad

This CL adds WebContents methods for setting the new SSLStatus flags to
record a password or credit card field on an HTTP page. It also
(roughly) integrates with the ContentPasswordManagerDriver code to call
the new WebContents methods. (The integration as written at present will
only set the SSLStatus flags when a password field is visible on page
load.)

BUG=647560
==========

to

==========
Add (some) password detection for HTTP-bad

This CL adds WebContents methods for setting the new SSLStatus flags to
record a password or credit card field on an HTTP page. It also
(roughly) integrates with the ContentPasswordManagerDriver code to call
the new WebContents methods. (The integration as written at present will
only set the SSLStatus flags when a password field is visible on page
load.)

BUG=647560
==========

[estark, 2016-09-22 21:35:47]

estark@chromium.org changed reviewers:
+ engedy@chromium.org

[estark, 2016-09-22 21:35:47]

Ok, thanks both. I added browser tests to make this a real CL.

I'm also adding engedy@ as a password_manager OWNER, in hopes he can answer a
couple questions for me. engedy: this is the start of showing a warning when
password fields are displayed on HTTP pages.
(https://docs.google.com/document/d/1xno6g6OnA7strcyzE-o_drevW8L0Mb6ZBEkjsiwa6...)
Am I understanding correctly that
ContentPasswordManagerDriver::PasswordFormsRendered() is called with the
*visible* password forms on page load, and that PasswordFormsParsed() is called
any time a dynamically added form appears, whether visible or invisible? Does
ContentPasswordManagerDriver get notified in any way when a previously invisible
password form becomes visible? Thanks!

[commit-bot: I haz the power, 2016-09-22 21:54:02]

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

[commit-bot: I haz the power, 2016-09-22 21:54:03]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[estark, 2016-09-22 22:00:30]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-22 22:00:59]

Dry run: 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, 2016-09-22 23:42:45]

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

[commit-bot: I haz the power, 2016-09-22 23:42:46]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[estark, 2016-09-22 23:49:02]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-22 23:49:40]

Dry run: 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, 2016-09-23 01:25:34]

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

[commit-bot: I haz the power, 2016-09-23 01:25:35]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[estark, 2016-09-23 01:34:16]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-23 01:34:35]

Dry run: 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, 2016-09-23 02:41:20]

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

[commit-bot: I haz the power, 2016-09-23 02:41:21]

Dry run: This issue passed the CQ dry run.

[engedy, 2016-09-23 13:45:35]

> Am I understanding correctly that
> ContentPasswordManagerDriver::PasswordFormsRendered() is called with the
> *visible* password forms on page load, and that PasswordFormsParsed() is
called
> any time a dynamically added form appears, whether visible or invisible? Does
> ContentPasswordManagerDriver get notified in any way when a previously
invisible
> password form becomes visible? Thanks!

You can think of it this way:

For the purposes of filling, we do not rely on PasswordFormsRendered at all.
Regardless of whether a form is visible or not, as soon as we discover the form,
PasswordFormsParsed is invoked, and credentials will be filled, if any. Forms
defined in the HTML markup are discovered when we scan the entire DOM once on
DOMContentLoaded event, dynamic forms are discovered as they are added.

PasswordFormsRendered is needed for only one thing: to verify, after a form
submission, that the submission was successful. The idea here is the following.
Should the submission fail, we expect that by the time the subsequent page load
finishes (window.load event) a form similar to the one just submitted will
re-appear on the page (with error details, to given a second chance at logging
in, etc). To this end, we scan the entire page for *visible* forms once on the
window.load event.

So the TL;DR is that filling happens regardless of whether a form is visible or
not, and the browser is not notified when a previously hidden form becomes
visible. It should be, however, possible, to set up some state on the form so
that something happens on the renderer side when it becomes visible.

[estark, 2016-09-24 06:12:23]

On 2016/09/23 13:45:35, engedy wrote:
> > Am I understanding correctly that
> > ContentPasswordManagerDriver::PasswordFormsRendered() is called with the
> > *visible* password forms on page load, and that PasswordFormsParsed() is
> called
> > any time a dynamically added form appears, whether visible or invisible?
Does
> > ContentPasswordManagerDriver get notified in any way when a previously
> invisible
> > password form becomes visible? Thanks!
> 
> You can think of it this way:
> 
> For the purposes of filling, we do not rely on PasswordFormsRendered at all.
> Regardless of whether a form is visible or not, as soon as we discover the
form,
> PasswordFormsParsed is invoked, and credentials will be filled, if any. Forms
> defined in the HTML markup are discovered when we scan the entire DOM once on
> DOMContentLoaded event, dynamic forms are discovered as they are added.
> 
> PasswordFormsRendered is needed for only one thing: to verify, after a form
> submission, that the submission was successful. The idea here is the
following.
> Should the submission fail, we expect that by the time the subsequent page
load
> finishes (window.load event) a form similar to the one just submitted will
> re-appear on the page (with error details, to given a second chance at logging
> in, etc). To this end, we scan the entire page for *visible* forms once on the
> window.load event.
> 
> So the TL;DR is that filling happens regardless of whether a form is visible
or
> not, and the browser is not notified when a previously hidden form becomes
> visible. It should be, however, possible, to set up some state on the form so
> that something happens on the renderer side when it becomes visible.

Great, thanks so much for the explanation. I'm going to leave this CL here for a
bit while I work on another CL to notify the browser when a password field's
visibility changes, then I'll come back to hook this one up to that one.

[estark, 2016-10-10 19:32:48]

Description was changed from

==========
Add (some) password detection for HTTP-bad

This CL adds WebContents methods for setting the new SSLStatus flags to
record a password or credit card field on an HTTP page. It also
(roughly) integrates with the ContentPasswordManagerDriver code to call
the new WebContents methods. (The integration as written at present will
only set the SSLStatus flags when a password field is visible on page
load.)

BUG=647560
==========

to

==========
Add (some) password detection for HTTP-bad

This CL adds WebContents methods for setting the new SSLStatus flags to
record a password or credit card field on an HTTP page. It also
(roughly) integrates with the ContentPasswordManagerDriver code to call
the new WebContents methods. The integration as written at present will
only set the SSLStatus flags whenever a password field is parsed;
eventually we will refine that to set the flags whenever a password field
is visible on the page (https://codereview.chromium.org/2378503002/).

BUG=647560
==========

[estark, 2016-10-10 20:11:19]

Update: I was waiting for https://codereview.chromium.org/2378503002/ to land
before landing this CL, because that would allow us to trigger the HTTP-bad
omnibox when a password field becomes visible on the page. However,
https://codereview.chromium.org/2378503002/ is blocked on some
IntersectionObserver bugs, so we decided that I should go ahead and land this
one, triggering HTTP-bad whenever a password field is parsed. This is more
aggressive than the behavior we intend to eventually ship, but it'll allow us to
start testing the UI.

[estark, 2016-10-10 20:13:22]

The CQ bit was checked by estark@chromium.org

[estark, 2016-10-10 20:13:22]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2362523003/#ps120001
(title: "trigger the downgrade from OnPasswordFormsParsed")

[commit-bot: I haz the power, 2016-10-10 20:13:49]

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, 2016-10-10 21:37:36]

Description was changed from

==========
Add (some) password detection for HTTP-bad

This CL adds WebContents methods for setting the new SSLStatus flags to
record a password or credit card field on an HTTP page. It also
(roughly) integrates with the ContentPasswordManagerDriver code to call
the new WebContents methods. The integration as written at present will
only set the SSLStatus flags whenever a password field is parsed;
eventually we will refine that to set the flags whenever a password field
is visible on the page (https://codereview.chromium.org/2378503002/).

BUG=647560
==========

to

==========
Add (some) password detection for HTTP-bad

This CL adds WebContents methods for setting the new SSLStatus flags to
record a password or credit card field on an HTTP page. It also
(roughly) integrates with the ContentPasswordManagerDriver code to call
the new WebContents methods. The integration as written at present will
only set the SSLStatus flags whenever a password field is parsed;
eventually we will refine that to set the flags whenever a password field
is visible on the page (https://codereview.chromium.org/2378503002/).

BUG=647560
==========

[commit-bot: I haz the power, 2016-10-10 21:37:37]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-10-10 21:39:12]

Description was changed from

==========
Add (some) password detection for HTTP-bad

This CL adds WebContents methods for setting the new SSLStatus flags to
record a password or credit card field on an HTTP page. It also
(roughly) integrates with the ContentPasswordManagerDriver code to call
the new WebContents methods. The integration as written at present will
only set the SSLStatus flags whenever a password field is parsed;
eventually we will refine that to set the flags whenever a password field
is visible on the page (https://codereview.chromium.org/2378503002/).

BUG=647560
==========

to

==========
Add (some) password detection for HTTP-bad

This CL adds WebContents methods for setting the new SSLStatus flags to
record a password or credit card field on an HTTP page. It also
(roughly) integrates with the ContentPasswordManagerDriver code to call
the new WebContents methods. The integration as written at present will
only set the SSLStatus flags whenever a password field is parsed;
eventually we will refine that to set the flags whenever a password field
is visible on the page (https://codereview.chromium.org/2378503002/).

BUG=647560

Committed: https://crrev.com/703f1b43a576ef25b5e267beaaba5f6ef70df08f
Cr-Commit-Position: refs/heads/master@{#424251}
==========

[commit-bot: I haz the power, 2016-10-10 21:39:14]

Patchset 7 (id:??) landed as
https://crrev.com/703f1b43a576ef25b5e267beaaba5f6ef70df08f
Cr-Commit-Position: refs/heads/master@{#424251}

[engedy, 2016-10-11 08:38:33]

Sounds good, thanks for the heads-up!