Skip to content

Commit

Permalink
[Matching] Update newPassword form regex (#628)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
dbajpeyi committed Aug 8, 2024
1 parent ff7f13d commit 917dad6
Show file tree
Hide file tree
Showing 13 changed files with 76 additions and 50 deletions.
2 changes: 1 addition & 1 deletion dist/autofill-debug.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion dist/autofill.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion integration-test/helpers/mocks.js
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand Down
16 changes: 12 additions & 4 deletions integration-test/helpers/pages/genericPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -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') {
Expand All @@ -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})
Expand Down
4 changes: 2 additions & 2 deletions integration-test/helpers/pages/loginPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -121,8 +121,8 @@ export function loginPage (page, opts = {}) {
* @param {string} username
* @return {Promise<void>}
*/
async assertUsernameFilled (username) {
const emailField = page.locator('#email')
async assertUsernameFilled (username, locator = '#email') {
const emailField = page.locator(locator)
await expect(emailField).toHaveValue(username)
}

Expand Down
65 changes: 29 additions & 36 deletions integration-test/tests/login-form.macos.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand All @@ -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)
Expand All @@ -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')
Expand All @@ -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)
Expand Down Expand Up @@ -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})

Expand All @@ -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})

Expand Down Expand Up @@ -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')
Expand Down Expand Up @@ -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()
Expand All @@ -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()
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion src/Form/matching-config/matching-config-source.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
6 changes: 6 additions & 0 deletions src/Form/matching.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,12 @@ describe('matching', () => {
html: `<input type="email" autocomplete="email" />`,
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: `<label for="password">Password (Forgot your password? Reset it)</label><input id="password" name></input>`,
subtype: 'credentials.password.current',
opts: {isLogin: true, hasCredentials: true, supportsIdentitiesAutofill: true}
}
])(`$html should be '$subtype'`, (args) => {
const { html, subtype, opts } = args
Expand Down
2 changes: 1 addition & 1 deletion swift-package/Resources/assets/autofill-debug.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion swift-package/Resources/assets/autofill.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

17 changes: 17 additions & 0 deletions test-forms/camelcamelcamel_login.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
<!DOCTYPE html>
<html>
<head>
<title>Replicating camelcamelcamel.com's login page, which has a password reset message in a label, linked to the password field</title>
</head>
<body>
<form method="post" action="/sessions/create" target="_parent" id="loginform">
<input type="hidden" name="return_to" value="">
<input type="hidden" name="remember_me" value="1">
<label class="first" for="login">Username or Email</label>
<input type="text" name="login" id="login" value="" tabindex="1" data-manual-scoring="username">
<label for="password">Password (Forgot your password? <a href="/forgot_password" tabindex="3">Reset it.</a>)</label>
<input type="password" name="password" id="password" tabindex="2" data-manual-scoring="password.current">
<input class="button large expanded" type="submit" value="Log in" data-manual-submit>
</form>
</body>
</html>
3 changes: 2 additions & 1 deletion test-forms/index.json
Original file line number Diff line number Diff line change
Expand Up @@ -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" }
]

0 comments on commit 917dad6

Please sign in to comment.