Loosen up heuristics for detecting account creation forms.

Loosen up heuristics for detecting account creation forms.

BUG=120779
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=156482

[Ilya Sherman, 2012-08-18 00:44:39]

http://codereview.chromium.org/10837324/diff/1/chrome/renderer/autofill/passw...
File chrome/renderer/autofill/password_generation_manager.cc (left):

http://codereview.chromium.org/10837324/diff/1/chrome/renderer/autofill/passw...
chrome/renderer/autofill/password_generation_manager.cc:98: if
(forms[i].isNull() || !forms[i].autoComplete())
autocomplete="off" actually means "don't remember the value entered into this
field", not "don't fill this field".  I don't think it's appropriate for us to
ignore it.  Besides the question of standards compatibility, ignoring this
attribute leaves authors no way to prevent password generation from triggering
and then failing to work correctly with the sign-in form.

http://codereview.chromium.org/10837324/diff/1/chrome/renderer/autofill/passw...
File chrome/renderer/autofill/password_generation_manager.cc (right):

http://codereview.chromium.org/10837324/diff/1/chrome/renderer/autofill/passw...
chrome/renderer/autofill/password_generation_manager.cc:52: // creation form.
This is definitely too lenient.  How bad are false positives?

http://codereview.chromium.org/10837324/diff/1/chrome/renderer/autofill/passw...
chrome/renderer/autofill/password_generation_manager.cc:59: }
nit: No need for curly braces for a one-line if-stmt

http://codereview.chromium.org/10837324/diff/1/chrome/renderer/autofill/passw...
chrome/renderer/autofill/password_generation_manager.cc:187: NOTREACHED();
Write this as DCHECK(password_form.get()) and skip the if-stmt?

[Garrett Casto, 2012-08-18 01:23:27]

http://codereview.chromium.org/10837324/diff/1/chrome/renderer/autofill/passw...
File chrome/renderer/autofill/password_generation_manager.cc (left):

http://codereview.chromium.org/10837324/diff/1/chrome/renderer/autofill/passw...
chrome/renderer/autofill/password_generation_manager.cc:98: if
(forms[i].isNull() || !forms[i].autoComplete())
On 2012/08/18 00:44:40, Ilya Sherman wrote:
> autocomplete="off" actually means "don't remember the value entered into this
> field", not "don't fill this field". 

If this is the case, do you know why so many account creation forms set this?
About 80% of the forms that I've seen with this set allow autocomplete on the
login page, so I assumed that they really didn't want the form filled (which I
have seen happen in other cases) and didn't care about saving.

 I don't think it's appropriate for us to
> ignore it.  Besides the question of standards compatibility, ignoring this
> attribute leaves authors no way to prevent password generation from triggering
> and then failing to work correctly with the sign-in form.

So I can see your point about not having a way of disabling this feature if you
know that you're site interacts badly with it for whatever reason. However I
think that we are overloading autocomplete=off to mean multiple things and if
there is some way to disable this feature it should be separate (different
attribute perhaps?).

For some background here, I've known that this has been a problem for a while
and have had conversations with a few different groups of people about this
(Brian and Glen, dpranke, other security folks). And the consensus has been that
it should be okay ignore autocomplete=off for this feature, even in the
autofilling case (as is done by third-party password managers). Now admittedly I
think that we are a biased sample because everyone I've talked to thinks that
autocomplete=off is a bad feature for security, but I haven't found anyone yet
that argues the other side.

http://codereview.chromium.org/10837324/diff/1/chrome/renderer/autofill/passw...
File chrome/renderer/autofill/password_generation_manager.cc (right):

http://codereview.chromium.org/10837324/diff/1/chrome/renderer/autofill/passw...
chrome/renderer/autofill/password_generation_manager.cc:52: // creation form.
On 2012/08/18 00:44:40, Ilya Sherman wrote:
> This is definitely too lenient.  How bad are false positives?

Pretty bad, in the sense that it's going to be very confusing to the user. I
wasn't intending on launching this as is, but the current heuristic is clearly
way to restrictive (we miss about 30% of account creation pages because of
this). We are planning on running some automated analysis to help determine what
the false positive rate of this will be, but also just having people browse with
this turned up will help us figure out where this fails.

http://codereview.chromium.org/10837324/diff/1/chrome/renderer/autofill/passw...
chrome/renderer/autofill/password_generation_manager.cc:59: }
On 2012/08/18 00:44:40, Ilya Sherman wrote:
> nit: No need for curly braces for a one-line if-stmt

Done.

http://codereview.chromium.org/10837324/diff/1/chrome/renderer/autofill/passw...
chrome/renderer/autofill/password_generation_manager.cc:187: NOTREACHED();
On 2012/08/18 00:44:40, Ilya Sherman wrote:
> Write this as DCHECK(password_form.get()) and skip the if-stmt?

Done.

[Ilya Sherman, 2012-08-18 01:31:56]

http://codereview.chromium.org/10837324/diff/1/chrome/renderer/autofill/passw...
File chrome/renderer/autofill/password_generation_manager.cc (left):

http://codereview.chromium.org/10837324/diff/1/chrome/renderer/autofill/passw...
chrome/renderer/autofill/password_generation_manager.cc:98: if
(forms[i].isNull() || !forms[i].autoComplete())
On 2012/08/18 01:23:27, Garrett Casto wrote:
> On 2012/08/18 00:44:40, Ilya Sherman wrote:
> > autocomplete="off" actually means "don't remember the value entered into
this
> > field", not "don't fill this field". 
> 
> If this is the case, do you know why so many account creation forms set this?
> About 80% of the forms that I've seen with this set allow autocomplete on the
> login page, so I assumed that they really didn't want the form filled (which I
> have seen happen in other cases) and didn't care about saving.

I suspect that you're correct that many account creation forms set this
attribute to prevent filling, rather than saving.  However, some of them might
set it for saving reasons, and that is the only case that the spec explicitly
supports.

>  I don't think it's appropriate for us to
> > ignore it.  Besides the question of standards compatibility, ignoring this
> > attribute leaves authors no way to prevent password generation from
triggering
> > and then failing to work correctly with the sign-in form.
> 
> So I can see your point about not having a way of disabling this feature if
you
> know that you're site interacts badly with it for whatever reason. However I
> think that we are overloading autocomplete=off to mean multiple things and if
> there is some way to disable this feature it should be separate (different
> attribute perhaps?).
> 
> For some background here, I've known that this has been a problem for a while
> and have had conversations with a few different groups of people about this
> (Brian and Glen, dpranke, other security folks). And the consensus has been
that
> it should be okay ignore autocomplete=off for this feature, even in the
> autofilling case (as is done by third-party password managers). Now admittedly
I
> think that we are a biased sample because everyone I've talked to thinks that
> autocomplete=off is a bad feature for security, but I haven't found anyone yet
> that argues the other side.

I agree that autocomplete="off" is generally broken, widely abused and
over-used.  However, I think we should be careful to (1) make sure there's an
escape hatch for websites who really want to opt-out of password generation,
e.g. due to bugs that would leave users stranded; and (2) work with the broader
community to revise the standard if needed.  Unilaterally ignoring established
expectations strikes me as a bad path forward.  I'm willing to be overruled by
team consensus on this, but FWIW I think it would be better to engage fully with
the standards bodies.  I would at least get Ian "Hixie" Hickson's thoughts, as
he knows a lot of the history and practical considerations that would be worth
keeping in mind.

[Garrett Casto, 2012-08-22 21:16:28]

On 2012/08/18 01:31:56, Ilya Sherman wrote:
>
http://codereview.chromium.org/10837324/diff/1/chrome/renderer/autofill/passw...
> File chrome/renderer/autofill/password_generation_manager.cc (left):
> 
>
http://codereview.chromium.org/10837324/diff/1/chrome/renderer/autofill/passw...
> chrome/renderer/autofill/password_generation_manager.cc:98: if
> (forms[i].isNull() || !forms[i].autoComplete())
> On 2012/08/18 01:23:27, Garrett Casto wrote:
> > On 2012/08/18 00:44:40, Ilya Sherman wrote:
> > > autocomplete="off" actually means "don't remember the value entered into
> this
> > > field", not "don't fill this field". 
> > 
> > If this is the case, do you know why so many account creation forms set
this?
> > About 80% of the forms that I've seen with this set allow autocomplete on
the
> > login page, so I assumed that they really didn't want the form filled (which
I
> > have seen happen in other cases) and didn't care about saving.
> 
> I suspect that you're correct that many account creation forms set this
> attribute to prevent filling, rather than saving.  However, some of them might
> set it for saving reasons, and that is the only case that the spec explicitly
> supports.
> 
> >  I don't think it's appropriate for us to
> > > ignore it.  Besides the question of standards compatibility, ignoring this
> > > attribute leaves authors no way to prevent password generation from
> triggering
> > > and then failing to work correctly with the sign-in form.
> > 
> > So I can see your point about not having a way of disabling this feature if
> you
> > know that you're site interacts badly with it for whatever reason. However I
> > think that we are overloading autocomplete=off to mean multiple things and
if
> > there is some way to disable this feature it should be separate (different
> > attribute perhaps?).
> > 
> > For some background here, I've known that this has been a problem for a
while
> > and have had conversations with a few different groups of people about this
> > (Brian and Glen, dpranke, other security folks). And the consensus has been
> that
> > it should be okay ignore autocomplete=off for this feature, even in the
> > autofilling case (as is done by third-party password managers). Now
admittedly
> I
> > think that we are a biased sample because everyone I've talked to thinks
that
> > autocomplete=off is a bad feature for security, but I haven't found anyone
yet
> > that argues the other side.
> 
> I agree that autocomplete="off" is generally broken, widely abused and
> over-used.  However, I think we should be careful to (1) make sure there's an
> escape hatch for websites who really want to opt-out of password generation,
> e.g. due to bugs that would leave users stranded; and (2) work with the
broader
> community to revise the standard if needed.  Unilaterally ignoring established
> expectations strikes me as a bad path forward.  I'm willing to be overruled by
> team consensus on this, but FWIW I think it would be better to engage fully
with
> the standards bodies.  I would at least get Ian "Hixie" Hickson's thoughts, as
> he knows a lot of the history and practical considerations that would be worth
> keeping in mind.



So I think that we are about on the same page, but I haven't done a great job of
explaining what I'm aiming for with this change.

The success of this feature is going to be based on being ubiquitous and easy to
use. Autocomplete=off is currently causing serious issues with the first
requirement and some minor issues with the second (imagine a case where the
account creation form doesn't have autocomplete=off set, but the login form
does). So the idea for now is to ignore autocomplete=off and see how useful of a
feature we can make without it. Having some standard for this seems perfectly
reasonable, it's just that the current one doesn't work very well. Talking with
Hixie is a good idea, I'll see what his thoughts are on this. There are some
other identity related tags that we we're looking to standardize anyway
(https://wiki.mozilla.org/Identity-inputs).

[Ilya Sherman, 2012-08-22 22:47:41]

> So I think that we are about on the same page, but I haven't done a great job
of
> explaining what I'm aiming for with this change.
> 
> The success of this feature is going to be based on being ubiquitous and easy
to
> use. Autocomplete=off is currently causing serious issues with the first
> requirement and some minor issues with the second (imagine a case where the
> account creation form doesn't have autocomplete=off set, but the login form
> does). So the idea for now is to ignore autocomplete=off and see how useful of
a
> feature we can make without it. Having some standard for this seems perfectly
> reasonable, it's just that the current one doesn't work very well. Talking
with
> Hixie is a good idea, I'll see what his thoughts are on this. There are some
> other identity related tags that we we're looking to standardize anyway
> (https://wiki.mozilla.org/Identity-inputs).

I think you should hold off on changing the behavior w.r.t. autocomplete="off"
until after discussing this further.  That doesn't need to block the rest of
this CL, but I think it's inappropriate to simply disregard the existing
standard without buy-in from the broader web developer community.

Just one reason why unilaterally ignoring autocomplete="off" might be a bad
idea:  Under some configurations for Linux, we store saved passwords in a
plain-text database.  Some of the websites that specify autocomplete="off" do so
precisely because they don't want passwords to be *stored* in an insecure
format, where they could be (relatively) easily accessed by evil-doers.

For reasons like this, I think there are lots more conversations that need to
happen before we just start ignoring autocomplete="off", despite the obvious
user benefits that would come from ignoring it in most cases.

[zysxqn, 2012-09-06 18:12:43]

On 2012/08/22 22:47:41, Ilya Sherman wrote:
> > So I think that we are about on the same page, but I haven't done a great
job
> of
> > explaining what I'm aiming for with this change.
> > 
> > The success of this feature is going to be based on being ubiquitous and
easy
> to
> > use. Autocomplete=off is currently causing serious issues with the first
> > requirement and some minor issues with the second (imagine a case where the
> > account creation form doesn't have autocomplete=off set, but the login form
> > does). So the idea for now is to ignore autocomplete=off and see how useful
of
> a
> > feature we can make without it. Having some standard for this seems
perfectly
> > reasonable, it's just that the current one doesn't work very well. Talking
> with
> > Hixie is a good idea, I'll see what his thoughts are on this. There are some
> > other identity related tags that we we're looking to standardize anyway
> > (https://wiki.mozilla.org/Identity-inputs).
> 
> I think you should hold off on changing the behavior w.r.t. autocomplete="off"
> until after discussing this further.  That doesn't need to block the rest of
> this CL, but I think it's inappropriate to simply disregard the existing
> standard without buy-in from the broader web developer community.
> 
> Just one reason why unilaterally ignoring autocomplete="off" might be a bad
> idea:  Under some configurations for Linux, we store saved passwords in a
> plain-text database.  Some of the websites that specify autocomplete="off" do
so
> precisely because they don't want passwords to be *stored* in an insecure
> format, where they could be (relatively) easily accessed by evil-doers.
> 
> For reasons like this, I think there are lots more conversations that need to
> happen before we just start ignoring autocomplete="off", despite the obvious
> user benefits that would come from ignoring it in most cases.

Ilya, I understand that whether to skip checking autocomplete has some debate
but we still want to loose the heuristic and try out and see its false positives
soon. Since Garrett is on leave I will patch this CL and try to submit for him.
I will not try to change the current behavior of how we check autocomplete and
just change the heuristic.

[Garrett Casto, 2012-09-10 20:28:24]

On 2012/08/22 22:47:41, Ilya Sherman wrote:
> > So I think that we are about on the same page, but I haven't done a great
job
> of
> > explaining what I'm aiming for with this change.
> > 
> > The success of this feature is going to be based on being ubiquitous and
easy
> to
> > use. Autocomplete=off is currently causing serious issues with the first
> > requirement and some minor issues with the second (imagine a case where the
> > account creation form doesn't have autocomplete=off set, but the login form
> > does). So the idea for now is to ignore autocomplete=off and see how useful
of
> a
> > feature we can make without it. Having some standard for this seems
perfectly
> > reasonable, it's just that the current one doesn't work very well. Talking
> with
> > Hixie is a good idea, I'll see what his thoughts are on this. There are some
> > other identity related tags that we we're looking to standardize anyway
> > (https://wiki.mozilla.org/Identity-inputs).
> 

Sorry for the delay on this. As Yue mentioned I was on paternity leave for a few
weeks.

> I think you should hold off on changing the behavior w.r.t. autocomplete="off"
> until after discussing this further.  That doesn't need to block the rest of
> this CL, but I think it's inappropriate to simply disregard the existing
> standard without buy-in from the broader web developer community.
> 
> Just one reason why unilaterally ignoring autocomplete="off" might be a bad
> idea:  Under some configurations for Linux, we store saved passwords in a
> plain-text database.  Some of the websites that specify autocomplete="off" do
so
> precisely because they don't want passwords to be *stored* in an insecure
> format, where they could be (relatively) easily accessed by evil-doers.
> 

Hui pointed this out to me a little while ago. This seems like it's a bug with
our implementation more than anything else, though I do wonder how much it
matters in practice. That is, if there is malware on your machine I don't think
that it matters which way the data is stored. Regardless, I see your point.

> For reasons like this, I think there are lots more conversations that need to
> happen before we just start ignoring autocomplete="off", despite the obvious
> user benefits that would come from ignoring it in most cases.

I'm removing the autocomplete change for now. I do want to make clear that I
wasn't trying to launch like this without further discussion, I just wanted to
try out the feature without this constraint and see how much of a difference it
made.

[Ilya Sherman, 2012-09-10 21:23:49]

LGTM, thanks

On 2012/09/10 20:28:24, Garrett Casto wrote:
> On 2012/08/22 22:47:41, Ilya Sherman wrote:
> > > So I think that we are about on the same page, but I haven't done a great
> job
> > of
> > > explaining what I'm aiming for with this change.
> > > 
> > > The success of this feature is going to be based on being ubiquitous and
> easy
> > to
> > > use. Autocomplete=off is currently causing serious issues with the first
> > > requirement and some minor issues with the second (imagine a case where
the
> > > account creation form doesn't have autocomplete=off set, but the login
form
> > > does). So the idea for now is to ignore autocomplete=off and see how
useful
> of
> > a
> > > feature we can make without it. Having some standard for this seems
> perfectly
> > > reasonable, it's just that the current one doesn't work very well. Talking
> > with
> > > Hixie is a good idea, I'll see what his thoughts are on this. There are
some
> > > other identity related tags that we we're looking to standardize anyway
> > > (https://wiki.mozilla.org/Identity-inputs).
> > 
> 
> Sorry for the delay on this. As Yue mentioned I was on paternity leave for a
few
> weeks.

No worries (and congratulations!).

> > I think you should hold off on changing the behavior w.r.t.
autocomplete="off"
> > until after discussing this further.  That doesn't need to block the rest of
> > this CL, but I think it's inappropriate to simply disregard the existing
> > standard without buy-in from the broader web developer community.
> > 
> > Just one reason why unilaterally ignoring autocomplete="off" might be a bad
> > idea:  Under some configurations for Linux, we store saved passwords in a
> > plain-text database.  Some of the websites that specify autocomplete="off"
do
> so
> > precisely because they don't want passwords to be *stored* in an insecure
> > format, where they could be (relatively) easily accessed by evil-doers.
> > 
> 
> Hui pointed this out to me a little while ago. This seems like it's a bug with
> our implementation more than anything else, though I do wonder how much it
> matters in practice. That is, if there is malware on your machine I don't
think
> that it matters which way the data is stored. Regardless, I see your point.
> 
> > For reasons like this, I think there are lots more conversations that need
to
> > happen before we just start ignoring autocomplete="off", despite the obvious
> > user benefits that would come from ignoring it in most cases.
> 
> I'm removing the autocomplete change for now. I do want to make clear that I
> wasn't trying to launch like this without further discussion, I just wanted to
> try out the feature without this constraint and see how much of a difference
it
> made.

My worry is that once the code lands, it'll be easy to forget about it and leave
it in, even past when the feature is behind a flag.  For that reason, I'd prefer
to either start the broader discussion first, or to add the code in a way that
makes it extra hard to accidentally launch without vetting with further
discussion and analysis.

http://codereview.chromium.org/10837324/diff/10001/chrome/renderer/autofill/p...
File chrome/renderer/autofill/password_generation_manager.cc (right):

http://codereview.chromium.org/10837324/diff/10001/chrome/renderer/autofill/p...
chrome/renderer/autofill/password_generation_manager.cc:36: int input_elements =
0;
nit: size_t

http://codereview.chromium.org/10837324/diff/10001/chrome/renderer/autofill/p...
chrome/renderer/autofill/password_generation_manager.cc:36: int input_elements =
0;
Optional nit: Perhaps |num_input_elements| or |input_element_count| or something
like that, to indicate that this is a count rather than a collection of
elements?

http://codereview.chromium.org/10837324/diff/10001/chrome/renderer/autofill/p...
chrome/renderer/autofill/password_generation_manager.cc:181: // We should not
have shown the icon in this case.
nit: What case does "in this case" refer to?  Could you reword this comment to
be a little bit more explicit?

[Garrett Casto, 2012-09-10 21:44:23]

http://codereview.chromium.org/10837324/diff/10001/chrome/renderer/autofill/p...
File chrome/renderer/autofill/password_generation_manager.cc (right):

http://codereview.chromium.org/10837324/diff/10001/chrome/renderer/autofill/p...
chrome/renderer/autofill/password_generation_manager.cc:36: int input_elements =
0;
On 2012/09/10 21:23:49, Ilya Sherman wrote:
> Optional nit: Perhaps |num_input_elements| or |input_element_count| or
something
> like that, to indicate that this is a count rather than a collection of
> elements?

Done.

http://codereview.chromium.org/10837324/diff/10001/chrome/renderer/autofill/p...
chrome/renderer/autofill/password_generation_manager.cc:36: int input_elements =
0;
On 2012/09/10 21:23:49, Ilya Sherman wrote:
> nit: size_t

Done.

http://codereview.chromium.org/10837324/diff/10001/chrome/renderer/autofill/p...
chrome/renderer/autofill/password_generation_manager.cc:181: // We should not
have shown the icon in this case.
On 2012/09/10 21:23:49, Ilya Sherman wrote:
> nit: What case does "in this case" refer to?  Could you reword this comment to
> be a little bit more explicit?

Done.

[commit-bot: I haz the power, 2012-09-13 00:44:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gcasto@chromium.org/10837324/14001

[commit-bot: I haz the power, 2012-09-13 03:00:21]

Change committed as 156482