From 531d81feacb5306300387558824ee4ce11e98b89 Mon Sep 17 00:00:00 2001 From: sense Date: Mon, 20 Mar 2017 03:12:07 -0700 Subject: [PATCH] Add password form search using blink::WebNode reference comparison. After an XHR is completed, PasswordAutofillAgent tries to check if the form is still visible (i.e. if the user has successfuly logged in or still sees the form) by searching the form with the same action URL. This works until the form has action URL. If it doesn't, the search is performed using field matching. This will fail if the form is modified by the website right before the XHR has completed. This CL adds support for the form search using blink::WebFormElement and blink::WebInputElement (for unowned forms) reference comparison. BUG=700862 R=dvadym@chromium.org Review-Url: https://codereview.chromium.org/2746033004 Cr-Commit-Position: refs/heads/master@{#458032} --- .../password_autofill_agent_browsertest.cc | 59 ++++++++++++++ components/autofill/content/renderer/BUILD.gn | 2 + .../content/renderer/form_autofill_util.cc | 6 ++ .../content/renderer/form_autofill_util.h | 8 +- .../renderer/password_autofill_agent.cc | 76 ++++++++++--------- .../renderer/password_autofill_agent.h | 17 +++-- .../provisionally_saved_password_form.cc | 42 ++++++++++ .../provisionally_saved_password_form.h | 58 ++++++++++++++ 8 files changed, 222 insertions(+), 46 deletions(-) create mode 100644 components/autofill/content/renderer/provisionally_saved_password_form.cc create mode 100644 components/autofill/content/renderer/provisionally_saved_password_form.h diff --git a/chrome/renderer/autofill/password_autofill_agent_browsertest.cc b/chrome/renderer/autofill/password_autofill_agent_browsertest.cc index b598bccc4fcb37..fbdc312d3bfbb4 100644 --- a/chrome/renderer/autofill/password_autofill_agent_browsertest.cc +++ b/chrome/renderer/autofill/password_autofill_agent_browsertest.cc @@ -2175,6 +2175,65 @@ TEST_F(PasswordAutofillAgentTest, ASSERT_FALSE(fake_driver_.called_password_form_submitted()); } +// Tests that no save promt is shown when an unowned form is changed and AJAX +// completed but the form is still visible. +TEST_F(PasswordAutofillAgentTest, + NoForm_NoPromptForAJAXSubmitWithoutNavigationAndNewElementAppeared) { + const char kNoFormHTMLWithHiddenField[] = + "" + "" + ""; + LoadHTML(kNoFormHTMLWithHiddenField); + + UpdateUsernameAndPasswordElements(); + WebElement captcha_element = + GetMainFrame()->document().getElementById(WebString::fromUTF8("captcha")); + ASSERT_FALSE(captcha_element.isNull()); + + SimulateUsernameChange("Bob"); + SimulatePasswordChange("mypassword"); + + // Simulate captcha element show up right before AJAX completed. + captcha_element.setAttribute("style", "display:inline;"); + password_autofill_agent_->AJAXSucceeded(); + + base::RunLoop().RunUntilIdle(); + EXPECT_FALSE(fake_driver_.called_inpage_navigation()); + EXPECT_FALSE(fake_driver_.called_password_form_submitted()); +} + +// Tests that no save promt is shown when a form with empty action URL is +// changed and AJAX completed but the form is still visible. +TEST_F(PasswordAutofillAgentTest, + NoAction_NoPromptForAJAXSubmitWithoutNavigationAndNewElementAppeared) { + // Form without an action URL. + const char kHTMLWithHiddenField[] = + "
" + " " + " " + " " + " " + "
"; + // Set the valid URL so the form action URL can be generated properly. + LoadHTMLWithUrlOverride(kHTMLWithHiddenField, "https://www.example.com"); + + UpdateUsernameAndPasswordElements(); + WebElement captcha_element = + GetMainFrame()->document().getElementById(WebString::fromUTF8("captcha")); + ASSERT_FALSE(captcha_element.isNull()); + + SimulateUsernameChange("Bob"); + SimulatePasswordChange("mypassword"); + + // Simulate captcha element show up right before AJAX completed. + captcha_element.setAttribute("style", "display:inline;"); + password_autofill_agent_->AJAXSucceeded(); + + base::RunLoop().RunUntilIdle(); + EXPECT_FALSE(fake_driver_.called_inpage_navigation()); + EXPECT_FALSE(fake_driver_.called_password_form_submitted()); +} + // Tests that credential suggestions are autofilled on a password (and change // password) forms having either ambiguous or empty name. TEST_F(PasswordAutofillAgentTest, diff --git a/components/autofill/content/renderer/BUILD.gn b/components/autofill/content/renderer/BUILD.gn index 2ddd7715dc2f6d..ce916440990590 100644 --- a/components/autofill/content/renderer/BUILD.gn +++ b/components/autofill/content/renderer/BUILD.gn @@ -21,6 +21,8 @@ static_library("renderer") { "password_form_conversion_utils.h", "password_generation_agent.cc", "password_generation_agent.h", + "provisionally_saved_password_form.cc", + "provisionally_saved_password_form.h", "renderer_save_password_progress_logger.cc", "renderer_save_password_progress_logger.h", ] diff --git a/components/autofill/content/renderer/form_autofill_util.cc b/components/autofill/content/renderer/form_autofill_util.cc index 9677f8c184a7bf..51c745fdb5253c 100644 --- a/components/autofill/content/renderer/form_autofill_util.cc +++ b/components/autofill/content/renderer/form_autofill_util.cc @@ -1203,6 +1203,7 @@ bool ExtractFormData(const WebFormElement& form_element, FormData* data) { } bool IsFormVisible(blink::WebFrame* frame, + const blink::WebFormElement& form_element, const GURL& canonical_action, const GURL& canonical_origin, const FormData& form_data) { @@ -1226,6 +1227,11 @@ bool IsFormVisible(blink::WebFrame* frame, if (!AreFormContentsVisible(form)) continue; + // Try to match the WebFormElement reference first. + if (!form_element.isNull() && form == form_element) { + return true; // Form still exists. + } + GURL iter_canonical_action = GetCanonicalActionForForm(form); bool form_action_is_empty = iter_canonical_action.is_empty() || iter_canonical_action == frame_origin; diff --git a/components/autofill/content/renderer/form_autofill_util.h b/components/autofill/content/renderer/form_autofill_util.h index ccf29157086447..2a5ef139c9a572 100644 --- a/components/autofill/content/renderer/form_autofill_util.h +++ b/components/autofill/content/renderer/form_autofill_util.h @@ -83,14 +83,16 @@ GURL StripAuthAndParams(const GURL& gurl); // successful. bool ExtractFormData(const blink::WebFormElement& form_element, FormData* data); -// Helper function to check if there exist any form on |frame| where its action +// Helper function to check if there exist any visible form on |frame| which +// equals |form_element|. If |form_element| is null, checks if forms action // equals |action|. Returns true if so. For forms with empty or unspecified // actions, all form data are used for comparison. Form data comparison is -// disabled on Mac and Android because the update prompt isn't implemented. -// It may cause many false password updates. +// disabled on Mac and Android because the update prompt isn't implemented. It +// may cause many false password updates. // TODO(kolos) Turn on all data comparing when the update prompt will be // implemented on Mac and Android. bool IsFormVisible(blink::WebFrame* frame, + const blink::WebFormElement& form_element, const GURL& action, const GURL& origin, const FormData& form_data); diff --git a/components/autofill/content/renderer/password_autofill_agent.cc b/components/autofill/content/renderer/password_autofill_agent.cc index abac9479d637fa..d48b9898bc398b 100644 --- a/components/autofill/content/renderer/password_autofill_agent.cc +++ b/components/autofill/content/renderer/password_autofill_agent.cc @@ -124,10 +124,14 @@ base::string16 FieldName(const FormFieldData& field, } bool IsUnownedPasswordFormVisible(blink::WebFrame* frame, + const blink::WebInputElement& input_element, const GURL& action, const GURL& origin, const FormData& form_data, const FormsPredictionsMap& form_predictions) { + if (!input_element.isNull() && form_util::IsWebNodeVisible(input_element)) + return true; + std::unique_ptr unowned_password_form( CreatePasswordFormFromUnownedInputElements(*frame, nullptr, &form_predictions)); @@ -710,7 +714,8 @@ void PasswordAutofillAgent::UpdateStateForTextChange( password_form = CreatePasswordFormFromWebForm( element.form(), &field_value_and_properties_map_, &form_predictions_); } - ProvisionallySavePassword(std::move(password_form), RESTRICTION_NONE); + ProvisionallySavePassword(std::move(password_form), element.form(), element, + RESTRICTION_NONE); if (element.isPasswordField()) { PasswordToLoginMap::iterator iter = password_to_username_.find(element); @@ -992,24 +997,26 @@ void PasswordAutofillAgent::AJAXSucceeded() { } void PasswordAutofillAgent::OnSamePageNavigationCompleted() { - if (!ProvisionallySavedPasswordIsValid()) + if (!provisionally_saved_form_.IsPasswordValid()) return; // Prompt to save only if the form is now gone, either invisible or // removed from the DOM. blink::WebFrame* frame = render_frame()->GetWebFrame(); - if (form_util::IsFormVisible(frame, provisionally_saved_form_->action, - provisionally_saved_form_->origin, - provisionally_saved_form_->form_data) || - IsUnownedPasswordFormVisible(frame, provisionally_saved_form_->action, - provisionally_saved_form_->origin, - provisionally_saved_form_->form_data, - form_predictions_)) { + const auto& password_form = provisionally_saved_form_.password_form(); + if (form_util::IsFormVisible(frame, provisionally_saved_form_.form_element(), + password_form.action, password_form.origin, + password_form.form_data) || + (provisionally_saved_form_.form_element().isNull() && + IsUnownedPasswordFormVisible( + frame, provisionally_saved_form_.input_element(), + password_form.action, password_form.origin, password_form.form_data, + form_predictions_))) { return; } - GetPasswordManagerDriver()->InPageNavigation(*provisionally_saved_form_); - provisionally_saved_form_.reset(); + GetPasswordManagerDriver()->InPageNavigation(password_form); + provisionally_saved_form_.Reset(); } void PasswordAutofillAgent::FirstUserGestureObserved() { @@ -1158,8 +1165,9 @@ void PasswordAutofillAgent::FrameDetached() { // into a password form, try to save the data. See https://crbug.com/450806 // for examples of sites that perform login using this technique. if (render_frame()->GetWebFrame()->parent() && - ProvisionallySavedPasswordIsValid()) { - GetPasswordManagerDriver()->InPageNavigation(*provisionally_saved_form_); + provisionally_saved_form_.IsPasswordValid()) { + GetPasswordManagerDriver()->InPageNavigation( + provisionally_saved_form_.password_form()); } FrameClosing(); } @@ -1181,7 +1189,8 @@ void PasswordAutofillAgent::WillSendSubmitEvent( // already have been updated in TextDidChangeInTextField. std::unique_ptr password_form = CreatePasswordFormFromWebForm( form, &field_value_and_properties_map_, &form_predictions_); - ProvisionallySavePassword(std::move(password_form), + ProvisionallySavePassword(std::move(password_form), form, + blink::WebInputElement(), RESTRICTION_NON_EMPTY_PASSWORD); } @@ -1207,16 +1216,15 @@ void PasswordAutofillAgent::WillSubmitForm(const blink::WebFormElement& form) { logger->LogPasswordForm(Logger::STRING_CREATED_PASSWORD_FORM, *submitted_form); } - if (provisionally_saved_form_ && - submitted_form->action == provisionally_saved_form_->action) { + if (provisionally_saved_form_.IsSet() && + submitted_form->action == + provisionally_saved_form_.password_form().action) { if (logger) logger->LogMessage(Logger::STRING_SUBMITTED_PASSWORD_REPLACED); - submitted_form->password_value = - provisionally_saved_form_->password_value; - submitted_form->new_password_value = - provisionally_saved_form_->new_password_value; - submitted_form->username_value = - provisionally_saved_form_->username_value; + const auto& saved_form = provisionally_saved_form_.password_form(); + submitted_form->password_value = saved_form.password_value; + submitted_form->new_password_value = saved_form.new_password_value; + submitted_form->username_value = saved_form.username_value; } // Some observers depend on sending this information now instead of when @@ -1224,7 +1232,7 @@ void PasswordAutofillAgent::WillSubmitForm(const blink::WebFormElement& form) { // RenderView to be instantiated (such as redirects to the WebStore) // we will never get to finish the load. GetPasswordManagerDriver()->PasswordFormSubmitted(*submitted_form); - provisionally_saved_form_.reset(); + provisionally_saved_form_.Reset(); } else if (logger) { logger->LogMessage(Logger::STRING_FORM_IS_NOT_PASSWORD); } @@ -1263,15 +1271,15 @@ void PasswordAutofillAgent::DidStartProvisionalLoad( ui::PageTransitionIsNewNavigation(type) && !blink::WebUserGestureIndicator::isProcessingUserGesture()) { // If onsubmit has been called, try and save that form. - if (provisionally_saved_form_) { + if (provisionally_saved_form_.IsSet()) { if (logger) { logger->LogPasswordForm( Logger::STRING_PROVISIONALLY_SAVED_FORM_FOR_FRAME, - *provisionally_saved_form_); + provisionally_saved_form_.password_form()); } GetPasswordManagerDriver()->PasswordFormSubmitted( - *provisionally_saved_form_); - provisionally_saved_form_.reset(); + provisionally_saved_form_.password_form()); + provisionally_saved_form_.Reset(); } else { std::vector> possible_submitted_forms; // Loop through the forms on the page looking for one that has been @@ -1552,7 +1560,7 @@ void PasswordAutofillAgent::FrameClosing() { password_to_username_.erase(iter.second.password_field); } web_input_to_password_info_.clear(); - provisionally_saved_form_.reset(); + provisionally_saved_form_.Reset(); field_value_and_properties_map_.clear(); } @@ -1573,20 +1581,16 @@ void PasswordAutofillAgent::ClearPreview( void PasswordAutofillAgent::ProvisionallySavePassword( std::unique_ptr password_form, + const blink::WebFormElement& form, + const blink::WebInputElement& input, ProvisionallySaveRestriction restriction) { if (!password_form || (restriction == RESTRICTION_NON_EMPTY_PASSWORD && password_form->password_value.empty() && password_form->new_password_value.empty())) { return; } - provisionally_saved_form_ = std::move(password_form); -} - -bool PasswordAutofillAgent::ProvisionallySavedPasswordIsValid() { - return provisionally_saved_form_ && - !provisionally_saved_form_->username_value.empty() && - !(provisionally_saved_form_->password_value.empty() && - provisionally_saved_form_->new_password_value.empty()); + DCHECK(password_form && (!form.isNull() || !input.isNull())); + provisionally_saved_form_.Set(std::move(password_form), form, input); } const mojom::AutofillDriverPtr& PasswordAutofillAgent::GetAutofillDriver() { diff --git a/components/autofill/content/renderer/password_autofill_agent.h b/components/autofill/content/renderer/password_autofill_agent.h index 12b7d03c639ea2..20f9c6aad5668f 100644 --- a/components/autofill/content/renderer/password_autofill_agent.h +++ b/components/autofill/content/renderer/password_autofill_agent.h @@ -14,6 +14,7 @@ #include "components/autofill/content/common/autofill_driver.mojom.h" #include "components/autofill/content/renderer/autofill_agent.h" #include "components/autofill/content/renderer/password_form_conversion_utils.h" +#include "components/autofill/content/renderer/provisionally_saved_password_form.h" #include "components/autofill/core/common/form_data_predictions.h" #include "components/autofill/core/common/password_form.h" #include "components/autofill/core/common/password_form_field_prediction_map.h" @@ -239,15 +240,17 @@ class PasswordAutofillAgent : public content::RenderFrameObserver, void ClearPreview(blink::WebInputElement* username, blink::WebInputElement* password); - // Saves |password_form| in |provisionally_saved_form_|, as long as it - // satisfies |restriction|. + // Saves |password_form|, |form| and |input| in |provisionally_saved_form_|, + // as long as it satisfies |restriction|. |form| and |input| are the elements + // user has just been interacting with before the form save. |form| or |input| + // can be null but not both at the same time. For example: if the form is + // unowned, |form| will be null; if the user has submitted the form, |input| + // will be null. void ProvisionallySavePassword(std::unique_ptr password_form, + const blink::WebFormElement& form, + const blink::WebInputElement& input, ProvisionallySaveRestriction restriction); - // Returns true if |provisionally_saved_form_| has enough information that - // it is likely filled out. - bool ProvisionallySavedPasswordIsValid(); - // Helper function called when in-page navigation completed void OnSamePageNavigationCompleted(); @@ -260,7 +263,7 @@ class PasswordAutofillAgent : public content::RenderFrameObserver, // Set if the user might be submitting a password form on the current page, // but the submit may still fail (i.e. doesn't pass JavaScript validation). - std::unique_ptr provisionally_saved_form_; + ProvisionallySavedPasswordForm provisionally_saved_form_; // Map WebFormControlElement to the pair of: // 1) The most recent text that user typed or PasswordManager autofilled in diff --git a/components/autofill/content/renderer/provisionally_saved_password_form.cc b/components/autofill/content/renderer/provisionally_saved_password_form.cc new file mode 100644 index 00000000000000..a9af1ed44bb237 --- /dev/null +++ b/components/autofill/content/renderer/provisionally_saved_password_form.cc @@ -0,0 +1,42 @@ +// Copyright 2017 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 "components/autofill/content/renderer/provisionally_saved_password_form.h" + +#include + +#include "components/autofill/core/common/password_form.h" + +namespace autofill { + +ProvisionallySavedPasswordForm::ProvisionallySavedPasswordForm() = default; + +ProvisionallySavedPasswordForm::~ProvisionallySavedPasswordForm() = default; + +void ProvisionallySavedPasswordForm::Set( + std::unique_ptr password_form, + const blink::WebFormElement& form_element, + const blink::WebInputElement& input_element) { + password_form_ = std::move(password_form); + form_element_ = form_element; + input_element_ = input_element; +} + +void ProvisionallySavedPasswordForm::Reset() { + password_form_.reset(); + form_element_.reset(); + input_element_.reset(); +} + +bool ProvisionallySavedPasswordForm::IsSet() const { + return static_cast(password_form_); +} + +bool ProvisionallySavedPasswordForm::IsPasswordValid() const { + return IsSet() && !password_form_->username_value.empty() && + !(password_form_->password_value.empty() && + password_form_->new_password_value.empty()); +} + +} // namespace autofill diff --git a/components/autofill/content/renderer/provisionally_saved_password_form.h b/components/autofill/content/renderer/provisionally_saved_password_form.h new file mode 100644 index 00000000000000..318eb189aedac9 --- /dev/null +++ b/components/autofill/content/renderer/provisionally_saved_password_form.h @@ -0,0 +1,58 @@ +// Copyright 2017 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. + +#ifndef COMPONENTS_AUTOFILL_CONTENT_RENDERER_PROVISIONALLY_SAVED_PASSWORD_FORM_H_ +#define COMPONENTS_AUTOFILL_CONTENT_RENDERER_PROVISIONALLY_SAVED_PASSWORD_FORM_H_ + +#include + +#include "base/macros.h" +#include "third_party/WebKit/public/web/WebInputElement.h" + +namespace autofill { + +struct PasswordForm; + +// Represents a possibly submitted password form. +class ProvisionallySavedPasswordForm { + public: + ProvisionallySavedPasswordForm(); + ~ProvisionallySavedPasswordForm(); + + // Sets the PasswordForm and web elements that were used in the PasswordForm + // update. + void Set(std::unique_ptr password_form, + const blink::WebFormElement& form_element, + const blink::WebInputElement& input_element); + void Reset(); + + // Returns true if the instance has |password_form_| set, but the actual + // password data may be invalid (e.g. empty username or password). + bool IsSet() const; + // Returns true if |password_form_| has enough information that it is likely + // filled out. + bool IsPasswordValid() const; + + const PasswordForm& password_form() const { + DCHECK(IsSet()); + return *password_form_; + } + const blink::WebFormElement& form_element() const { return form_element_; } + const blink::WebInputElement& input_element() const { return input_element_; } + + private: + std::unique_ptr password_form_; + // Last used WebFormElement for the PasswordForm submission. Can be null if + // the form is unowned. + blink::WebFormElement form_element_; + // Last used WebInputElement which led to the PasswordForm update. Can be null + // if the user has performed a form submission (via a button, for example). + blink::WebInputElement input_element_; + + DISALLOW_COPY_AND_ASSIGN(ProvisionallySavedPasswordForm); +}; + +} // namespace autofill + +#endif // COMPONENTS_AUTOFILL_CONTENT_RENDERER_PROVISIONALLY_SAVED_PASSWORD_FORM_H_