From 917dad6d8ea94cfb8b629b9a6db9d92cf859a797 Mon Sep 17 00:00:00 2001 From: Deepankar Bajpeyi Date: Thu, 8 Aug 2024 18:32:25 +0900 Subject: [PATCH] [Matching] Update newPassword form regex (#628) This PR removes `reset` from `newPassword` regexes, as that keyword likely exists in login forms e.g https://camelcamelcamel.com/login Currently (on camelcamelcamel), the password input misses on https://github.com/duckduckgo/duckduckgo-autofill/blob/5c8abec8b6e8a7676b81db8839072454ea48a14c/src/Form/matching.js#L426 check since the regex only matches "current password" or "password current". Following that it immediately matches in https://github.com/duckduckgo/duckduckgo-autofill/blob/5c8abec8b6e8a7676b81db8839072454ea48a14c/src/Form/matching.js#L432 for `newPassword`. This works, as there's a `labelText` for the password input that has the word "reset" in it. I think this is a valid use case, for password reset label to be linked with a password input. We shouldn't consider `reset` to be a token related to new passwords only. --- dist/autofill-debug.js | 2 +- dist/autofill.js | 2 +- integration-test/helpers/mocks.js | 3 +- integration-test/helpers/pages/genericPage.js | 16 +++-- integration-test/helpers/pages/loginPage.js | 4 +- .../tests/login-form.macos.spec.js | 65 +++++++++---------- .../__generated__/compiled-matching-config.js | 2 +- .../matching-config/matching-config-source.js | 2 +- src/Form/matching.test.js | 6 ++ .../Resources/assets/autofill-debug.js | 2 +- swift-package/Resources/assets/autofill.js | 2 +- test-forms/camelcamelcamel_login.html | 17 +++++ test-forms/index.json | 3 +- 13 files changed, 76 insertions(+), 50 deletions(-) create mode 100644 test-forms/camelcamelcamel_login.html diff --git a/dist/autofill-debug.js b/dist/autofill-debug.js index 8baddfebd..c7025f0fe 100644 --- a/dist/autofill-debug.js +++ b/dist/autofill-debug.js @@ -12503,7 +12503,7 @@ const matchingConfiguration = exports.matchingConfiguration = { forceUnknown: /captcha|mfa|2fa|two factor|otp|pin/iu }, newPassword: { - match: /new|re.?(enter|type)|repeat|update|reset\b/iu + match: /new|re.?(enter|type)|repeat|update\b/iu }, currentPassword: { match: /current|old|previous|expired|existing/iu diff --git a/dist/autofill.js b/dist/autofill.js index 97c5ed0b6..0f2651de8 100644 --- a/dist/autofill.js +++ b/dist/autofill.js @@ -8337,7 +8337,7 @@ const matchingConfiguration = exports.matchingConfiguration = { forceUnknown: /captcha|mfa|2fa|two factor|otp|pin/iu }, newPassword: { - match: /new|re.?(enter|type)|repeat|update|reset\b/iu + match: /new|re.?(enter|type)|repeat|update\b/iu }, currentPassword: { match: /current|old|previous|expired|existing/iu diff --git a/integration-test/helpers/mocks.js b/integration-test/helpers/mocks.js index 9c97697d1..067e48cd6 100644 --- a/integration-test/helpers/mocks.js +++ b/integration-test/helpers/mocks.js @@ -25,7 +25,8 @@ export const constants = { 'loginMultistep': `${privacyTestPagesPrefix}/autoprompt/3-multistep-form.html`, 'shadowDom': `${privacyTestPagesPrefix}/shadow-dom.html`, 'selectInput': `${localPagesPrefix}/select-input.html`, - 'shadowInputsLogin': `${localPagesPrefix}/shadow-inputs-login.html` + 'shadowInputsLogin': `${localPagesPrefix}/shadow-inputs-login.html`, + 'loginWithPasswordReset': `${localPagesPrefix}/login-with-password-reset.html` }, forms: { 'www_ulisboa_pt_login.html': `${testFormsPrefix}/www_ulisboa_pt_login.html` diff --git a/integration-test/helpers/pages/genericPage.js b/integration-test/helpers/pages/genericPage.js index 73fe34857..39623bcd7 100644 --- a/integration-test/helpers/pages/genericPage.js +++ b/integration-test/helpers/pages/genericPage.js @@ -15,7 +15,7 @@ export function genericPage (page) { await expect(field).toHaveAttribute('data-ddg-inputtype') const passwordStyle = await page.locator(selector).getAttribute('style') - expect(passwordStyle).toContain(constants.iconMatchers.keyFill) + await expect(passwordStyle).toContain(constants.iconMatchers.keyFill) } async passwordFieldShowsGenKey (selector = '#password') { @@ -24,19 +24,27 @@ export function genericPage (page) { await expect(field).toHaveAttribute('data-ddg-inputtype') const passwordStyle = await page.locator(selector).getAttribute('style') - expect(passwordStyle).toContain(constants.iconMatchers.keyGen) + await expect(passwordStyle).toContain(constants.iconMatchers.keyGen) } async passwordHasNoIcon (selector = '#password') { const passwordStyle = await page.locator(selector).getAttribute('style') - expect(passwordStyle || '').not.toContain('data:image/svg+xml;base64,') + await expect(passwordStyle || '').not.toContain('data:image/svg+xml;base64,') } - async clickThePasswordField (selector = '#password') { + async locateAndClick (selector) { const input = page.locator(selector) await input.click({force: true}) } + async clickThePasswordField (selector = '#password') { + await this.locateAndClick(selector) + } + + async clickTheUsernameField (selector = '#username') { + await this.locateAndClick(selector) + } + async selectGeneratedPassword (selector = '#password') { const input = page.locator(selector) await input.click({force: true}) diff --git a/integration-test/helpers/pages/loginPage.js b/integration-test/helpers/pages/loginPage.js index f39e0454e..626721881 100644 --- a/integration-test/helpers/pages/loginPage.js +++ b/integration-test/helpers/pages/loginPage.js @@ -121,8 +121,8 @@ export function loginPage (page, opts = {}) { * @param {string} username * @return {Promise} */ - async assertUsernameFilled (username) { - const emailField = page.locator('#email') + async assertUsernameFilled (username, locator = '#email') { + const emailField = page.locator(locator) await expect(emailField).toHaveValue(username) } diff --git a/integration-test/tests/login-form.macos.spec.js b/integration-test/tests/login-form.macos.spec.js index 06055fc95..62ad0582b 100644 --- a/integration-test/tests/login-form.macos.spec.js +++ b/integration-test/tests/login-form.macos.spec.js @@ -32,6 +32,22 @@ async function mocks (page) { return {personalAddress, password} } +/** + * @param {import("@playwright/test").Page} page + * @param {{ + * overlay?: boolean, + * clickLabel?: boolean, + * pageType?: keyof typeof constants.pages, + * availableInputTypes?: import('./login-form.android.spec.js').AvailableInputTypes + * }} [opts] + */ +async function loadAutofillWithReplacements (page, opts) { + await createAutofillScript() + .replaceAll(macosContentScopeReplacements(opts)) + .platform('macos') + .applyTo(page) +} + /** * @param {import("@playwright/test").Page} page * @param {{overlay?: boolean, clickLabel?: boolean, pageType?: keyof typeof constants.pages}} [opts] @@ -45,10 +61,7 @@ async function testLoginPage (page, opts = {}) { const {personalAddress, password} = await mocks(page) // Load the autofill.js script with replacements - await createAutofillScript() - .replaceAll(macosContentScopeReplacements({overlay})) - .platform('macos') - .applyTo(page) + await loadAutofillWithReplacements(page, {overlay}) const login = loginPage(page, {overlay, clickLabel}) await login.navigate(pageType) @@ -67,10 +80,7 @@ async function createLoginFormInModalPage (page) { await mocks(page) // Pretend we're running in a top-frame scenario - await createAutofillScript() - .replaceAll(macosContentScopeReplacements()) - .platform('macos') - .applyTo(page) + await loadAutofillWithReplacements(page) const login = loginPage(page) await login.navigate('loginWithFormInModal') @@ -86,10 +96,8 @@ test.describe('Auto-fill a login form on macOS', () => { test('clicking on username should fill the username and password field', async ({page}) => { await forwardConsoleMessages(page) await mocks(page) - await createAutofillScript() - .replaceAll(macosContentScopeReplacements({})) - .platform('macos') - .applyTo(page) + await loadAutofillWithReplacements(page, {}) + const login = shadowInputsLoginPage(page) await login.navigate() await login.clickTheUsernameField(personalAddress) @@ -118,10 +126,7 @@ test.describe('Auto-fill a login form on macOS', () => { await mocks(page) // Load the autofill.js script with replacements - await createAutofillScript() - .replaceAll(macosContentScopeReplacements({overlay: true})) - .platform('macos') - .applyTo(page) + await loadAutofillWithReplacements(page, {overlay: true}) const login = loginPage(page, {overlay: true}) @@ -139,10 +144,7 @@ test.describe('Auto-fill a login form on macOS', () => { await mocks(page) // Load the autofill.js script with replacements - await createAutofillScript() - .replaceAll(macosContentScopeReplacements({overlay: true})) - .platform('macos') - .applyTo(page) + await loadAutofillWithReplacements(page, {overlay: true}) const login = loginPage(page, {overlay: true}) @@ -257,10 +259,7 @@ test.describe('Auto-fill a login form on macOS', () => { await forwardConsoleMessages(page) const {personalAddress, password} = await mocks(page) - await createAutofillScript() - .replaceAll(macosContentScopeReplacements()) - .platform('macos') - .applyTo(page) + await loadAutofillWithReplacements(page) const login = loginPage(page) await login.navigate('loginWithText') @@ -292,14 +291,11 @@ test.describe('Auto-fill a login form on macOS', () => { await createWebkitMocks() .applyTo(page) - await createAutofillScript() - .replaceAll(macosContentScopeReplacements({ - availableInputTypes: { - credentials: {username: false, password: false} - } - })) - .platform('macos') - .applyTo(page) + await loadAutofillWithReplacements(page, { + availableInputTypes: { + credentials: {username: false, password: false} + } + }) const login = loginPage(page) await login.navigate() @@ -317,10 +313,7 @@ test.describe('Auto-fill a login form on macOS', () => { .withPrivateEmail('random123@duck.com') .applyTo(page) - await createAutofillScript() - .replaceAll(macosContentScopeReplacements()) - .platform('macos') - .applyTo(page) + await loadAutofillWithReplacements(page) const login = loginPage(page) await login.navigate() diff --git a/src/Form/matching-config/__generated__/compiled-matching-config.js b/src/Form/matching-config/__generated__/compiled-matching-config.js index 3b099a26a..40c90cad7 100644 --- a/src/Form/matching-config/__generated__/compiled-matching-config.js +++ b/src/Form/matching-config/__generated__/compiled-matching-config.js @@ -249,7 +249,7 @@ const matchingConfiguration = { skip: /email|one-time|error|hint|^username$/iu, forceUnknown: /captcha|mfa|2fa|two factor|otp|pin/iu }, - newPassword: { match: /new|re.?(enter|type)|repeat|update|reset\b/iu }, + newPassword: { match: /new|re.?(enter|type)|repeat|update\b/iu }, currentPassword: { match: /current|old|previous|expired|existing/iu }, username: { match: /(user|account|online.?id|membership.?id|log(i|o)n|net)((.)?(name|i.?d.?|log(i|o)n).?)?(.?((or|\/).+|\*|:)( required)?)?$|(nome|id|login).?utente|(nome|id) (dell.)?account|codice (cliente|uten)|nutzername|anmeldename|gebruikersnaam|nom d.utilisateur|identifiant|pseudo|usuari|cuenta|identificador|apodo|\bdni\b|\bnie\b| del? documento|documento de identidad|användarnamn|kontonamn|användar-id/iu, diff --git a/src/Form/matching-config/matching-config-source.js b/src/Form/matching-config/matching-config-source.js index 459d2b40e..5e0b3531f 100644 --- a/src/Form/matching-config/matching-config-source.js +++ b/src/Form/matching-config/matching-config-source.js @@ -268,7 +268,7 @@ const matchingConfiguration = { forceUnknown: 'captcha|mfa|2fa|two factor|otp|pin' }, newPassword: { - match: 'new|re.?(enter|type)|repeat|update|reset\\b' + match: 'new|re.?(enter|type)|repeat|update\\b' }, currentPassword: { match: 'current|old|previous|expired|existing' diff --git a/src/Form/matching.test.js b/src/Form/matching.test.js index c6096f234..85896d139 100644 --- a/src/Form/matching.test.js +++ b/src/Form/matching.test.js @@ -172,6 +172,12 @@ describe('matching', () => { html: ``, subtype: 'credentials.username', opts: {isHybrid: true, hasCredentials: true, supportsIdentitiesAutofill: true} + }, + { + // a login form's password input with 'reset' in the label should be a current subtype + html: ``, + subtype: 'credentials.password.current', + opts: {isLogin: true, hasCredentials: true, supportsIdentitiesAutofill: true} } ])(`$html should be '$subtype'`, (args) => { const { html, subtype, opts } = args diff --git a/swift-package/Resources/assets/autofill-debug.js b/swift-package/Resources/assets/autofill-debug.js index 8baddfebd..c7025f0fe 100644 --- a/swift-package/Resources/assets/autofill-debug.js +++ b/swift-package/Resources/assets/autofill-debug.js @@ -12503,7 +12503,7 @@ const matchingConfiguration = exports.matchingConfiguration = { forceUnknown: /captcha|mfa|2fa|two factor|otp|pin/iu }, newPassword: { - match: /new|re.?(enter|type)|repeat|update|reset\b/iu + match: /new|re.?(enter|type)|repeat|update\b/iu }, currentPassword: { match: /current|old|previous|expired|existing/iu diff --git a/swift-package/Resources/assets/autofill.js b/swift-package/Resources/assets/autofill.js index 97c5ed0b6..0f2651de8 100644 --- a/swift-package/Resources/assets/autofill.js +++ b/swift-package/Resources/assets/autofill.js @@ -8337,7 +8337,7 @@ const matchingConfiguration = exports.matchingConfiguration = { forceUnknown: /captcha|mfa|2fa|two factor|otp|pin/iu }, newPassword: { - match: /new|re.?(enter|type)|repeat|update|reset\b/iu + match: /new|re.?(enter|type)|repeat|update\b/iu }, currentPassword: { match: /current|old|previous|expired|existing/iu diff --git a/test-forms/camelcamelcamel_login.html b/test-forms/camelcamelcamel_login.html new file mode 100644 index 000000000..ab2a794d6 --- /dev/null +++ b/test-forms/camelcamelcamel_login.html @@ -0,0 +1,17 @@ + + + + Replicating camelcamelcamel.com's login page, which has a password reset message in a label, linked to the password field + + +
+ + + + + + + +
+ + diff --git a/test-forms/index.json b/test-forms/index.json index b169cb52f..e9230084b 100644 --- a/test-forms/index.json +++ b/test-forms/index.json @@ -543,5 +543,6 @@ { "html": "myecp_login.html", "expectedFailures": ["username"] }, { "html": "fullcolor_login.html" }, { "html": "malta-taxes-payment-form.html", "expectedFailures": ["expirationMonth", "expirationYear", "cardSecurityCode"] }, - { "html": "single-select-form.html" } + { "html": "single-select-form.html" }, + { "html": "camelcamelcamel_login.html" } ]