Skip to content

GitLab

T
tangled
Commit 3c355fbe authored by Vasilii Sukhanov's avatar Vasilii Sukhanov Committed by Commit Bot
Browse files
Options

Prevent manual password generation for the case when it's broken. 

It doesn't work correctly for the password fields without name and id. In this
case Chrome should not allow the manual password generation to start. Automatic
password generation should not trigger on the problematic pages anyway.

Bug: 908385
Change-Id: Ic6611e01f99ad9e4052a4198ba5f9f13217d6dd6
Reviewed-on: https://chromium-review.googlesource.com/c/1352317
Commit-Queue: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: default avatarVadym Doroshenko <dvadym@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611723}
parent a1185fcb
Hide whitespace changes
Inline Side-by-side
Showing with 52 additions and 17 deletions
chrome/renderer/autofill/password_generation_agent_browsertest.cc
View file @ 3c355fbe
...@@ -193,7 +193,7 @@ class PasswordGenerationAgentTestForHtmlAnnotation ...@@ -193,7 +193,7 @@ class PasswordGenerationAgentTestForHtmlAnnotation
DISALLOW_COPY_AND_ASSIGN(PasswordGenerationAgentTestForHtmlAnnotation); DISALLOW_COPY_AND_ASSIGN(PasswordGenerationAgentTestForHtmlAnnotation);
}; };
const char kSigninFormHTML[] = constexpr char kSigninFormHTML[] =
"<FORM name = 'blah' action = 'http://www.random.com/'> " "<FORM name = 'blah' action = 'http://www.random.com/'> "
" <INPUT type = 'text' id = 'username'/> " " <INPUT type = 'text' id = 'username'/> "
" <INPUT type = 'password' id = 'password'/> " " <INPUT type = 'password' id = 'password'/> "
...@@ -201,7 +201,7 @@ const char kSigninFormHTML[] = ...@@ -201,7 +201,7 @@ const char kSigninFormHTML[] =
" <INPUT type = 'submit' value = 'LOGIN' />" " <INPUT type = 'submit' value = 'LOGIN' />"
"</FORM>"; "</FORM>";
const char kAccountCreationFormHTML[] = constexpr char kAccountCreationFormHTML[] =
"<FORM id = 'blah' action = 'http://www.random.com/pa/th?q=1&p=3#first'> " "<FORM id = 'blah' action = 'http://www.random.com/pa/th?q=1&p=3#first'> "
" <INPUT type = 'text' id = 'username'/> " " <INPUT type = 'text' id = 'username'/> "
" <INPUT type = 'password' id = 'first_password' size = 5/>" " <INPUT type = 'password' id = 'first_password' size = 5/>"
...@@ -211,7 +211,7 @@ const char kAccountCreationFormHTML[] = ...@@ -211,7 +211,7 @@ const char kAccountCreationFormHTML[] =
" <INPUT type = 'submit' value = 'LOGIN' />" " <INPUT type = 'submit' value = 'LOGIN' />"
"</FORM>"; "</FORM>";
const char kAccountCreationNoForm[] = constexpr char kAccountCreationNoForm[] =
"<INPUT type = 'text' id = 'username'/> " "<INPUT type = 'text' id = 'username'/> "
"<INPUT type = 'password' id = 'first_password' size = 5/>" "<INPUT type = 'password' id = 'first_password' size = 5/>"
"<INPUT type = 'password' id = 'second_password' size = 5/> " "<INPUT type = 'password' id = 'second_password' size = 5/> "
...@@ -219,7 +219,17 @@ const char kAccountCreationNoForm[] = ...@@ -219,7 +219,17 @@ const char kAccountCreationNoForm[] =
"<INPUT type = 'button' id = 'dummy'/> " "<INPUT type = 'button' id = 'dummy'/> "
"<INPUT type = 'submit' value = 'LOGIN' />"; "<INPUT type = 'submit' value = 'LOGIN' />";
const char kDisabledElementAccountCreationFormHTML[] = constexpr char kAccountCreationNoIds[] =
"<FORM action = 'http://www.random.com/pa/th?q=1&p=3#first'> "
" <INPUT type = 'text'/> "
" <INPUT type = 'password' class='first_password'/>"
" <INPUT type = 'password' class='second_password'/> "
" <INPUT type = 'text'/> "
" <INPUT type = 'button' id = 'dummy'/> "
" <INPUT type = 'submit' value = 'LOGIN'/>"
"</FORM>";
constexpr char kDisabledElementAccountCreationFormHTML[] =
"<FORM name = 'blah' action = 'http://www.random.com/'> " "<FORM name = 'blah' action = 'http://www.random.com/'> "
" <INPUT type = 'text' id = 'username'/> " " <INPUT type = 'text' id = 'username'/> "
" <INPUT type = 'password' id = 'first_password' " " <INPUT type = 'password' id = 'first_password' "
...@@ -231,7 +241,7 @@ const char kDisabledElementAccountCreationFormHTML[] = ...@@ -231,7 +241,7 @@ const char kDisabledElementAccountCreationFormHTML[] =
" <INPUT type = 'submit' value = 'LOGIN' />" " <INPUT type = 'submit' value = 'LOGIN' />"
"</FORM>"; "</FORM>";
const char kHiddenPasswordAccountCreationFormHTML[] = constexpr char kHiddenPasswordAccountCreationFormHTML[] =
"<FORM name = 'blah' action = 'http://www.random.com/'> " "<FORM name = 'blah' action = 'http://www.random.com/'> "
" <INPUT type = 'text' id = 'username'/> " " <INPUT type = 'text' id = 'username'/> "
" <INPUT type = 'password' id = 'first_password'/> " " <INPUT type = 'password' id = 'first_password'/> "
...@@ -240,7 +250,7 @@ const char kHiddenPasswordAccountCreationFormHTML[] = ...@@ -240,7 +250,7 @@ const char kHiddenPasswordAccountCreationFormHTML[] =
" <INPUT type = 'submit' value = 'LOGIN' />" " <INPUT type = 'submit' value = 'LOGIN' />"
"</FORM>"; "</FORM>";
const char kInvalidActionAccountCreationFormHTML[] = constexpr char kInvalidActionAccountCreationFormHTML[] =
"<FORM name = 'blah' action = 'invalid'> " "<FORM name = 'blah' action = 'invalid'> "
" <INPUT type = 'text' id = 'username'/> " " <INPUT type = 'text' id = 'username'/> "
" <INPUT type = 'password' id = 'first_password'/> " " <INPUT type = 'password' id = 'first_password'/> "
...@@ -249,7 +259,7 @@ const char kInvalidActionAccountCreationFormHTML[] = ...@@ -249,7 +259,7 @@ const char kInvalidActionAccountCreationFormHTML[] =
" <INPUT type = 'submit' value = 'LOGIN' />" " <INPUT type = 'submit' value = 'LOGIN' />"
"</FORM>"; "</FORM>";
const char kMultipleAccountCreationFormHTML[] = constexpr char kMultipleAccountCreationFormHTML[] =
"<FORM name = 'login' action = 'http://www.random.com/'> " "<FORM name = 'login' action = 'http://www.random.com/'> "
" <INPUT type = 'text' id = 'random'/> " " <INPUT type = 'text' id = 'random'/> "
" <INPUT type = 'text' id = 'username'/> " " <INPUT type = 'text' id = 'username'/> "
...@@ -266,7 +276,7 @@ const char kMultipleAccountCreationFormHTML[] = ...@@ -266,7 +276,7 @@ const char kMultipleAccountCreationFormHTML[] =
" <INPUT type = 'submit' value = 'LOGIN' />" " <INPUT type = 'submit' value = 'LOGIN' />"
"</FORM>"; "</FORM>";
const char kBothAutocompleteAttributesFormHTML[] = constexpr char kBothAutocompleteAttributesFormHTML[] =
"<FORM name = 'blah' action = 'http://www.random.com/'> " "<FORM name = 'blah' action = 'http://www.random.com/'> "
" <INPUT type = 'text' autocomplete='username' id = 'username'/> " " <INPUT type = 'text' autocomplete='username' id = 'username'/> "
" <INPUT type = 'password' id = 'first_password' " " <INPUT type = 'password' id = 'first_password' "
...@@ -276,7 +286,7 @@ const char kBothAutocompleteAttributesFormHTML[] = ...@@ -276,7 +286,7 @@ const char kBothAutocompleteAttributesFormHTML[] =
" <INPUT type = 'submit' value = 'LOGIN' />" " <INPUT type = 'submit' value = 'LOGIN' />"
"</FORM>"; "</FORM>";
const char kUsernameAutocompleteAttributeFormHTML[] = constexpr char kUsernameAutocompleteAttributeFormHTML[] =
"<FORM name = 'blah' action = 'http://www.random.com/'> " "<FORM name = 'blah' action = 'http://www.random.com/'> "
" <INPUT type = 'text' autocomplete='username' id = 'username'/> " " <INPUT type = 'text' autocomplete='username' id = 'username'/> "
" <INPUT type = 'password' id = 'first_password' size = 5/>" " <INPUT type = 'password' id = 'first_password' size = 5/>"
...@@ -285,7 +295,7 @@ const char kUsernameAutocompleteAttributeFormHTML[] = ...@@ -285,7 +295,7 @@ const char kUsernameAutocompleteAttributeFormHTML[] =
" <INPUT type = 'submit' value = 'LOGIN' />" " <INPUT type = 'submit' value = 'LOGIN' />"
"</FORM>"; "</FORM>";
const char kNewPasswordAutocompleteAttributeFormHTML[] = constexpr char kNewPasswordAutocompleteAttributeFormHTML[] =
"<FORM name = 'blah' action = 'http://www.random.com/'> " "<FORM name = 'blah' action = 'http://www.random.com/'> "
" <INPUT type = 'text' id = 'username'/> " " <INPUT type = 'text' id = 'username'/> "
" <INPUT type = 'password' id = 'first_password' " " <INPUT type = 'password' id = 'first_password' "
...@@ -295,7 +305,7 @@ const char kNewPasswordAutocompleteAttributeFormHTML[] = ...@@ -295,7 +305,7 @@ const char kNewPasswordAutocompleteAttributeFormHTML[] =
" <INPUT type = 'submit' value = 'LOGIN' />" " <INPUT type = 'submit' value = 'LOGIN' />"
"</FORM>"; "</FORM>";
const char kCurrentAndNewPasswordAutocompleteAttributeFormHTML[] = constexpr char kCurrentAndNewPasswordAutocompleteAttributeFormHTML[] =
"<FORM name = 'blah' action = 'http://www.random.com/'> " "<FORM name = 'blah' action = 'http://www.random.com/'> "
" <INPUT type = 'password' id = 'old_password' " " <INPUT type = 'password' id = 'old_password' "
" autocomplete='current-password'/>" " autocomplete='current-password'/>"
...@@ -307,7 +317,7 @@ const char kCurrentAndNewPasswordAutocompleteAttributeFormHTML[] = ...@@ -307,7 +317,7 @@ const char kCurrentAndNewPasswordAutocompleteAttributeFormHTML[] =
" <INPUT type = 'submit' value = 'LOGIN' />" " <INPUT type = 'submit' value = 'LOGIN' />"
"</FORM>"; "</FORM>";
const char kPasswordChangeFormHTML[] = constexpr char kPasswordChangeFormHTML[] =
"<FORM name = 'ChangeWithUsernameForm' action = 'http://www.bidule.com'> " "<FORM name = 'ChangeWithUsernameForm' action = 'http://www.bidule.com'> "
" <INPUT type = 'text' id = 'username'/> " " <INPUT type = 'text' id = 'username'/> "
" <INPUT type = 'password' id = 'password'/> " " <INPUT type = 'password' id = 'password'/> "
...@@ -317,7 +327,7 @@ const char kPasswordChangeFormHTML[] = ...@@ -317,7 +327,7 @@ const char kPasswordChangeFormHTML[] =
" <INPUT type = 'submit' value = 'Login'/> " " <INPUT type = 'submit' value = 'Login'/> "
"</FORM>"; "</FORM>";
const char kPasswordFormAndSpanHTML[] = constexpr char kPasswordFormAndSpanHTML[] =
"<FORM name = 'blah' action = 'http://www.random.com/pa/th?q=1&p=3#first'>" "<FORM name = 'blah' action = 'http://www.random.com/pa/th?q=1&p=3#first'>"
" <INPUT type = 'text' id = 'username'/> " " <INPUT type = 'text' id = 'username'/> "
" <INPUT type = 'password' id = 'password'/> " " <INPUT type = 'password' id = 'password'/> "
...@@ -861,6 +871,16 @@ TEST_F(PasswordGenerationAgentTest, ManualGenerationDoesntSuppressAutomatic) { ...@@ -861,6 +871,16 @@ TEST_F(PasswordGenerationAgentTest, ManualGenerationDoesntSuppressAutomatic) {
ExpectAutomaticGenerationAvailable("first_password", true); ExpectAutomaticGenerationAvailable("first_password", true);
} }
TEST_F(PasswordGenerationAgentTest, ManualGenerationNoIds) {
LoadHTMLWithUserGesture(kAccountCreationNoIds);
ExecuteJavaScriptForTests(
"document.getElementsByClassName('first_password')[0].focus();");
password_generation_->UserTriggeredGeneratePassword();
// TODO(crbug/866444): generation doesn't work properly on the password field
// without name and id. Temporarily it's disabled.
EXPECT_FALSE(GetCalledShowManualPasswordGenerationPopup());
}
TEST_F(PasswordGenerationAgentTest, PresavingGeneratedPassword) { TEST_F(PasswordGenerationAgentTest, PresavingGeneratedPassword) {
const struct { const struct {
const char* form; const char* form;
......
components/autofill/content/renderer/password_generation_agent.cc
View file @ 3c355fbe
...@@ -440,8 +440,10 @@ void PasswordGenerationAgent::GeneratedPasswordAccepted( ...@@ -440,8 +440,10 @@ void PasswordGenerationAgent::GeneratedPasswordAccepted(
render_frame()->GetRenderView()->GetWebView()->AdvanceFocus(false); render_frame()->GetRenderView()->GetWebView()->AdvanceFocus(false);
} }
std::unique_ptr<PasswordForm> presaved_form(CreatePasswordFormToPresave()); std::unique_ptr<PasswordForm> presaved_form(CreatePasswordFormToPresave());
if (presaved_form) if (presaved_form) {
DCHECK_NE(base::string16(), presaved_form->password_value);
GetPasswordManagerClient()->PresaveGeneratedPassword(*presaved_form); GetPasswordManagerClient()->PresaveGeneratedPassword(*presaved_form);
}
// Call UpdateStateForTextChange after the corresponding PasswordFormManager // Call UpdateStateForTextChange after the corresponding PasswordFormManager
// is notified that the password was generated. // is notified that the password was generated.
...@@ -612,6 +614,11 @@ bool PasswordGenerationAgent::SetUpUserTriggeredGeneration() { ...@@ -612,6 +614,11 @@ bool PasswordGenerationAgent::SetUpUserTriggeredGeneration() {
last_focused_password_element_.NameForAutofill().Utf16(), last_focused_password_element_.NameForAutofill().Utf16(),
last_focused_password_element_.FormControlTypeForAutofill() last_focused_password_element_.FormControlTypeForAutofill()
.Utf8()))); .Utf8())));
// TODO(crbug/866444): remove this once it's impossible that currently focused
// password field isn't found by FindPasswordElementsForGeneration.
if (!std::count(password_elements.begin(), password_elements.end(),
last_focused_password_element_))
return false;
current_generation_item_.reset(new GenerationItemInfo( current_generation_item_.reset(new GenerationItemInfo(
last_focused_password_element_, std::move(*password_form), last_focused_password_element_, std::move(*password_form),
std::move(password_elements))); std::move(password_elements)));
......
components/password_manager/core/browser/form_saver_impl.cc
View file @ 3c355fbe
...@@ -76,6 +76,7 @@ void FormSaverImpl::Update( ...@@ -76,6 +76,7 @@ void FormSaverImpl::Update(
} }
void FormSaverImpl::PresaveGeneratedPassword(const PasswordForm& generated) { void FormSaverImpl::PresaveGeneratedPassword(const PasswordForm& generated) {
DCHECK_NE(base::string16(), generated.password_value);
auto form = std::make_unique<PasswordForm>(generated); auto form = std::make_unique<PasswordForm>(generated);
SanitizeFormData(&form->form_data); SanitizeFormData(&form->form_data);
if (presaved_) if (presaved_)
......
components/password_manager/core/browser/password_manager_unittest.cc
View file @ 3c355fbe
...@@ -461,6 +461,7 @@ TEST_F(PasswordManagerTest, GeneratedPasswordFormSubmitEmptyStore) { ...@@ -461,6 +461,7 @@ TEST_F(PasswordManagerTest, GeneratedPasswordFormSubmitEmptyStore) {
EXPECT_CALL(client_, IsSavingAndFillingEnabledForCurrentPage()) EXPECT_CALL(client_, IsSavingAndFillingEnabledForCurrentPage())
.WillRepeatedly(Return(true)); .WillRepeatedly(Return(true));
EXPECT_CALL(*store_, AddLogin(_)); EXPECT_CALL(*store_, AddLogin(_));
form.password_value = form.new_password_value;
manager()->OnPresaveGeneratedPassword(&driver_, form); manager()->OnPresaveGeneratedPassword(&driver_, form);
OnPasswordFormSubmitted(form); OnPasswordFormSubmitted(form);
...@@ -1715,6 +1716,7 @@ TEST_F(PasswordManagerTest, PasswordGeneration_FailedSubmission) { ...@@ -1715,6 +1716,7 @@ TEST_F(PasswordManagerTest, PasswordGeneration_FailedSubmission) {
EXPECT_CALL(client_, IsSavingAndFillingEnabledForCurrentPage()) EXPECT_CALL(client_, IsSavingAndFillingEnabledForCurrentPage())
.WillRepeatedly(Return(true)); .WillRepeatedly(Return(true));
EXPECT_CALL(*store_, AddLogin(_)); EXPECT_CALL(*store_, AddLogin(_));
form.password_value = form.new_password_value;
manager()->OnPresaveGeneratedPassword(&driver_, form); manager()->OnPresaveGeneratedPassword(&driver_, form);
// Do not save generated password when the password form reappears. // Do not save generated password when the password form reappears.
...@@ -1743,11 +1745,12 @@ TEST_F(PasswordManagerTest, PasswordGenerationPasswordEdited_FailedSubmission) { ...@@ -1743,11 +1745,12 @@ TEST_F(PasswordManagerTest, PasswordGenerationPasswordEdited_FailedSubmission) {
EXPECT_CALL(client_, IsSavingAndFillingEnabledForCurrentPage()) EXPECT_CALL(client_, IsSavingAndFillingEnabledForCurrentPage())
.WillRepeatedly(Return(true)); .WillRepeatedly(Return(true));
EXPECT_CALL(*store_, AddLogin(_)); EXPECT_CALL(*store_, AddLogin(_));
form.password_value = form.new_password_value;
manager()->OnPresaveGeneratedPassword(&driver_, form); manager()->OnPresaveGeneratedPassword(&driver_, form);
// Simulate user editing and submitting a different password. Verify that // Simulate user editing and submitting a different password. Verify that
// the edited password is the one that is saved. // the edited password is the one that is saved.
form.new_password_value = ASCIIToUTF16("different_password"); form.password_value = ASCIIToUTF16("different_password");
OnPasswordFormSubmitted(form); OnPasswordFormSubmitted(form);
// Do not save generated password when the password form reappears. // Do not save generated password when the password form reappears.
...@@ -1777,10 +1780,11 @@ TEST_F(PasswordManagerTest, ...@@ -1777,10 +1780,11 @@ TEST_F(PasswordManagerTest,
EXPECT_CALL(client_, IsSavingAndFillingEnabledForCurrentPage()) EXPECT_CALL(client_, IsSavingAndFillingEnabledForCurrentPage())
.WillRepeatedly(Return(true)); .WillRepeatedly(Return(true));
EXPECT_CALL(*store_, AddLogin(_)); EXPECT_CALL(*store_, AddLogin(_));
form.password_value = form.new_password_value;
manager()->OnPresaveGeneratedPassword(&driver_, form); manager()->OnPresaveGeneratedPassword(&driver_, form);
// Simulate user removing generated password and adding a new one. // Simulate user removing generated password and adding a new one.
form.new_password_value = ASCIIToUTF16("different_password"); form.password_value = ASCIIToUTF16("different_password");
EXPECT_CALL(*store_, RemoveLogin(_)); EXPECT_CALL(*store_, RemoveLogin(_));
manager()->OnPasswordNoLongerGenerated(&driver_, form); manager()->OnPasswordNoLongerGenerated(&driver_, form);
...@@ -1811,10 +1815,11 @@ TEST_F(PasswordManagerTest, ...@@ -1811,10 +1815,11 @@ TEST_F(PasswordManagerTest,
EXPECT_CALL(client_, IsSavingAndFillingEnabledForCurrentPage()) EXPECT_CALL(client_, IsSavingAndFillingEnabledForCurrentPage())
.WillRepeatedly(Return(true)); .WillRepeatedly(Return(true));
EXPECT_CALL(*store_, AddLogin(_)); EXPECT_CALL(*store_, AddLogin(_));
form.password_value = form.new_password_value;
manager()->OnPresaveGeneratedPassword(&driver_, form); manager()->OnPresaveGeneratedPassword(&driver_, form);
// Simulate user removing generated password and adding a new one. // Simulate user removing generated password and adding a new one.
form.new_password_value = ASCIIToUTF16("different_password"); form.password_value = ASCIIToUTF16("different_password");
EXPECT_CALL(*store_, RemoveLogin(_)); EXPECT_CALL(*store_, RemoveLogin(_));
manager()->OnPasswordNoLongerGenerated(&driver_, form); manager()->OnPasswordNoLongerGenerated(&driver_, form);
...@@ -1844,6 +1849,7 @@ TEST_F(PasswordManagerTest, PasswordGenerationUsernameChanged) { ...@@ -1844,6 +1849,7 @@ TEST_F(PasswordManagerTest, PasswordGenerationUsernameChanged) {
EXPECT_CALL(client_, IsSavingAndFillingEnabledForCurrentPage()) EXPECT_CALL(client_, IsSavingAndFillingEnabledForCurrentPage())
.WillRepeatedly(Return(true)); .WillRepeatedly(Return(true));
EXPECT_CALL(*store_, AddLogin(_)); EXPECT_CALL(*store_, AddLogin(_));
form.password_value = form.new_password_value;
manager()->OnPresaveGeneratedPassword(&driver_, form); manager()->OnPresaveGeneratedPassword(&driver_, form);
// Simulate user changing the password and username, without ever completely // Simulate user changing the password and username, without ever completely
...@@ -1962,6 +1968,7 @@ TEST_F(PasswordManagerTest, PasswordGenerationPresavePasswordAndLogin) { ...@@ -1962,6 +1968,7 @@ TEST_F(PasswordManagerTest, PasswordGenerationPresavePasswordAndLogin) {
manager()->OnPasswordFormsRendered(&driver_, observed, true); manager()->OnPasswordFormsRendered(&driver_, observed, true);
// The user accepts generated password and makes successful login. // The user accepts generated password and makes successful login.
form.password_value = form.new_password_value;
PasswordForm presaved_form(form); PasswordForm presaved_form(form);
if (found_matched_logins_in_store) if (found_matched_logins_in_store)
presaved_form.username_value.clear(); presaved_form.username_value.clear();
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment