Loosen up heuristics for detecting account creation forms.

chrome/renderer/autofill/password_generation_manager.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/renderer/autofill/password_generation_manager.h"
 
 #include "base/logging.h"
 #include "chrome/common/autofill_messages.h"
 #include "chrome/common/net/gaia/gaia_urls.h"
 #include "chrome/common/password_generation_util.h"
@@ skipping 15 matching lines @@
 
 // Returns true if we think that this form is for account creation. |passwords|
 // is filled with the password field(s) in the form.
 bool GetAccountCreationPasswordFields(
     const WebKit::WebFormElement& form,
     std::vector<WebKit::WebInputElement>* passwords) {
   // Grab all of the passwords for the form.
   WebKit::WebVector<WebKit::WebFormControlElement> control_elements;
   form.getFormControlElements(control_elements);
 
+  int input_elements = 0;
   for (size_t i = 0; i < control_elements.size(); i++) {
     WebKit::WebInputElement* input_element =
         toWebInputElement(&control_elements[i]);
     // Only pay attention to visible password fields.
     if (input_element &&
-        input_element->isPasswordField() &&
+        input_element->isTextField() &&
         input_element->hasNonEmptyBoundingBox()) {
-      passwords->push_back(*input_element);
+      input_elements++;
+      if (input_element->isPasswordField())
+        passwords->push_back(*input_element);
     }
   }
 
-  // For now, just assume that if there are two password fields in the
-  // form that this is meant for account creation.
-  // TODO(gcasto): Determine better heauristics for this.
-  if (passwords->size() == 2)
+  // This may be too lenient, but we assume that any form with at least three
+  // input elements where at least one of them is a password is an account
+  // creation form.

[Ilya Sherman, 2012/08/18 00:44:40]

This is definitely too lenient.  How bad are false positives?

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

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.

+  if (!passwords->empty() && input_elements >= 3) {
+    // We trim |passwords| because occasionally there are forms where the
+    // security question answers are put in password fields and we don't want
+    // to fill those.
+    if (passwords->size() > 2) {
+      passwords->resize(2);
+    }

[Ilya Sherman, 2012/08/18 00:44:40]

nit: No need for curly braces for a one-line if-stmt

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

Done.

     return true;
+  }
 
   return false;
 }
 
 } // namespace
 
 PasswordGenerationManager::PasswordGenerationManager(
     content::RenderView* render_view)
     : content::RenderViewObserver(render_view),
       render_view_(render_view),
@@ skipping 23 matching lines @@
   // them.
   if (!enabled_)
     return;
 
   if (!ShouldAnalyzeDocument(frame->document()))
     return;
 
   WebKit::WebVector<WebKit::WebFormElement> forms;
   frame->document().forms(forms);
   for (size_t i = 0; i < forms.size(); ++i) {
-    // Ignore forms with autocomplete turned off for now. We may remove this in
-    // the future, as we only want to avoid creating passwords if the signin
-    // form has autocomplete turned off.
-    if (forms[i].isNull() || !forms[i].autoComplete())

[Ilya Sherman, 2012/08/18 00:44:40]

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.

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

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
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.

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

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 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.

+    if (forms[i].isNull())
       continue;
 
     // If we can't get a valid PasswordForm, we skip this form because the
     // the password won't get saved even if we generate it.
     scoped_ptr<webkit::forms::PasswordForm> password_form(
         webkit::forms::PasswordFormDomManager::CreatePasswordForm(forms[i]));
     if (!password_form.get()) {
-      DVLOG(2) << "Invalid action on form";
+      DVLOG(2) << "Skipping form as it would not be saved";
       continue;
     }
 
     // Do not generate password for GAIA since it is used to retrieve the
     // generated paswords.
     GURL realm(password_form->signon_realm);
     if (realm == GURL(GaiaUrls::GetInstance()->gaia_login_form_realm()))
       continue;
 
     std::vector<WebKit::WebInputElement> passwords;
@@ skipping 41 matching lines @@
 WebKit::WebCString PasswordGenerationManager::imageNameForDisabledState() {
   return imageNameForNormalState();
 }
 
 WebKit::WebCString PasswordGenerationManager::imageNameForReadOnlyState() {
   return imageNameForNormalState();
 }
 
 void PasswordGenerationManager::handleClick(WebKit::WebInputElement& element) {
   gfx::Rect rect(element.decorationElementFor(this).boundsInViewportSpace());
-  webkit::forms::PasswordForm* password_form(
+  scoped_ptr<webkit::forms::PasswordForm> password_form(
       webkit::forms::PasswordFormDomManager::CreatePasswordForm(
           element.form()));
-  if (password_form) {
+  if (password_form.get()) {
     Send(new AutofillHostMsg_ShowPasswordGenerationPopup(routing_id(),
                                                          rect,
                                                          element.maxLength(),
                                                          *password_form));
     password_generation::LogPasswordGenerationEvent(
         password_generation::BUBBLE_SHOWN);
+  } else {
+    // We should not have shown the icon in this case.
+    NOTREACHED();

[Ilya Sherman, 2012/08/18 00:44:40]

Write this as DCHECK(password_form.get()) and skip the if-stmt?

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

Done.

   }
 }
 
 void PasswordGenerationManager::willDetach(
     const WebKit::WebInputElement& element) {
   // No implementation
 }
 
 bool PasswordGenerationManager::OnMessageReceived(const IPC::Message& message) {
   bool handled = true;
@@ skipping 47 matching lines @@
       passwords_[0].decorationElementFor(this).setAttribute("style",
                                                             "display:block");
       password_generation::LogPasswordGenerationEvent(
           password_generation::ICON_SHOWN);
       return;
     }
   }
 }
 
 }  // namespace autofill