From bf38f2dfeec7ba51de9d85eb0ce8e32198eb3926 Mon Sep 17 00:00:00 2001 From: Cesar Gonzalez Date: Thu, 3 Oct 2024 16:53:45 -0500 Subject: [PATCH] [PM-12775] Password autofill should not occur within 2FA fields (#11303) --- .../autofill/content/autofill-init.spec.ts | 2 +- .../src/autofill/content/autofill-init.ts | 2 +- .../autofill/services/autofill-constants.ts | 12 ++++--- .../services/autofill.service.spec.ts | 20 ++++------- .../src/autofill/services/autofill.service.ts | 20 +++++++++-- ...inline-menu-field-qualification.service.ts | 33 ++++++++++++++----- 6 files changed, 57 insertions(+), 32 deletions(-) diff --git a/apps/browser/src/autofill/content/autofill-init.spec.ts b/apps/browser/src/autofill/content/autofill-init.spec.ts index ebfbda75b5..b98d297d13 100644 --- a/apps/browser/src/autofill/content/autofill-init.spec.ts +++ b/apps/browser/src/autofill/content/autofill-init.spec.ts @@ -74,7 +74,7 @@ describe("AutofillInit", () => { Object.defineProperty(document, "readyState", { value: "complete", writable: true }); autofillInit.init(); - jest.advanceTimersByTime(250); + jest.advanceTimersByTime(750); expect(sendExtensionMessageSpy).toHaveBeenCalledWith("bgCollectPageDetails", { sender: "autofillInit", diff --git a/apps/browser/src/autofill/content/autofill-init.ts b/apps/browser/src/autofill/content/autofill-init.ts index e901000dbb..6c34508cb0 100644 --- a/apps/browser/src/autofill/content/autofill-init.ts +++ b/apps/browser/src/autofill/content/autofill-init.ts @@ -78,7 +78,7 @@ class AutofillInit implements AutofillInitInterface { this.clearCollectPageDetailsOnLoadTimeout(); this.collectPageDetailsOnLoadTimeout = setTimeout( () => this.sendExtensionMessage("bgCollectPageDetails", { sender: "autofillInit" }), - 250, + 750, ); }; diff --git a/apps/browser/src/autofill/services/autofill-constants.ts b/apps/browser/src/autofill/services/autofill-constants.ts index c379daaf2d..656387f00b 100644 --- a/apps/browser/src/autofill/services/autofill-constants.ts +++ b/apps/browser/src/autofill/services/autofill-constants.ts @@ -34,28 +34,30 @@ export class AutoFillConstants { "totpcode", "2facode", "approvals_code", - "code", "mfacode", - "otc", "otc-code", - "otp", + "onetimecode", "otp-code", "otpcode", - "pin", + "onetimepassword", "security_code", "twofactor", "twofa", "twofactorcode", "verificationCode", + "verification code", ]; + static readonly AmbiguousTotpFieldNames: string[] = ["code", "pin", "otc", "otp"]; + static readonly SearchFieldNames: string[] = ["search", "query", "find", "go"]; static readonly FieldIgnoreList: string[] = ["captcha", "findanything", "forgot"]; static readonly PasswordFieldExcludeList: string[] = [ + "hint", ...AutoFillConstants.FieldIgnoreList, - "onetimepassword", + ...AutoFillConstants.TotpFieldNames, ]; static readonly ExcludedAutofillLoginTypes: string[] = [ diff --git a/apps/browser/src/autofill/services/autofill.service.spec.ts b/apps/browser/src/autofill/services/autofill.service.spec.ts index 455c171e59..7bd08caaf3 100644 --- a/apps/browser/src/autofill/services/autofill.service.spec.ts +++ b/apps/browser/src/autofill/services/autofill.service.spec.ts @@ -2260,29 +2260,23 @@ describe("AutofillService", () => { options, ); - expect(AutofillService.fieldIsFuzzyMatch).toHaveBeenCalledTimes(4); - expect(AutofillService.fieldIsFuzzyMatch).toHaveBeenNthCalledWith( - 1, + expect(AutofillService.fieldIsFuzzyMatch).toHaveBeenCalledWith( usernameField, AutoFillConstants.UsernameFieldNames, ); - expect(AutofillService.fieldIsFuzzyMatch).toHaveBeenNthCalledWith( - 2, + expect(AutofillService.fieldIsFuzzyMatch).toHaveBeenCalledWith( emailField, AutoFillConstants.UsernameFieldNames, ); - expect(AutofillService.fieldIsFuzzyMatch).toHaveBeenNthCalledWith( - 3, + expect(AutofillService.fieldIsFuzzyMatch).toHaveBeenCalledWith( telephoneField, AutoFillConstants.UsernameFieldNames, ); - expect(AutofillService.fieldIsFuzzyMatch).toHaveBeenNthCalledWith( - 4, + expect(AutofillService.fieldIsFuzzyMatch).toHaveBeenCalledWith( totpField, AutoFillConstants.UsernameFieldNames, ); - expect(AutofillService.fieldIsFuzzyMatch).not.toHaveBeenNthCalledWith( - 5, + expect(AutofillService.fieldIsFuzzyMatch).not.toHaveBeenCalledWith( nonViewableField, AutoFillConstants.UsernameFieldNames, ); @@ -2328,6 +2322,7 @@ describe("AutofillService", () => { it("will not attempt to fuzzy match a totp field if totp autofill is not allowed", async () => { options.allowTotpAutofill = false; + jest.spyOn(autofillService as any, "findMatchingFieldIndex"); await autofillService["generateLoginFillScript"]( fillScript, @@ -2336,7 +2331,7 @@ describe("AutofillService", () => { options, ); - expect(AutofillService.fieldIsFuzzyMatch).not.toHaveBeenCalledWith( + expect(autofillService["findMatchingFieldIndex"]).not.toHaveBeenCalledWith( expect.anything(), AutoFillConstants.TotpFieldNames, ); @@ -2386,7 +2381,6 @@ describe("AutofillService", () => { false, false, ); - expect(AutofillService.fieldIsFuzzyMatch).not.toHaveBeenCalled(); expect(AutofillService.fillByOpid).toHaveBeenCalledTimes(2); expect(AutofillService.fillByOpid).toHaveBeenNthCalledWith( 1, diff --git a/apps/browser/src/autofill/services/autofill.service.ts b/apps/browser/src/autofill/services/autofill.service.ts index de79fd974e..ea68b80e84 100644 --- a/apps/browser/src/autofill/services/autofill.service.ts +++ b/apps/browser/src/autofill/services/autofill.service.ts @@ -887,7 +887,10 @@ export default class AutofillService implements AutofillServiceInterface { options.allowTotpAutofill && f.viewable && (f.type === "text" || f.type === "number") && - (AutofillService.fieldIsFuzzyMatch(f, AutoFillConstants.TotpFieldNames) || + (AutofillService.fieldIsFuzzyMatch(f, [ + ...AutoFillConstants.TotpFieldNames, + ...AutoFillConstants.AmbiguousTotpFieldNames, + ]) || f.autoCompleteType === "one-time-code") ) { totps.push(f); @@ -2558,6 +2561,11 @@ export default class AutofillService implements AutofillServiceInterface { return; } + // We want to avoid treating TOTP fields as password fields + if (AutofillService.fieldIsFuzzyMatch(f, AutoFillConstants.TotpFieldNames)) { + return; + } + const isLikePassword = () => { if (f.type !== "text") { return false; @@ -2670,12 +2678,18 @@ export default class AutofillService implements AutofillServiceInterface { (withoutForm || f.form === passwordField.form) && (canBeHidden || f.viewable) && (f.type === "text" || f.type === "number") && - AutofillService.fieldIsFuzzyMatch(f, AutoFillConstants.TotpFieldNames) + AutofillService.fieldIsFuzzyMatch(f, [ + ...AutoFillConstants.TotpFieldNames, + ...AutoFillConstants.AmbiguousTotpFieldNames, + ]) ) { totpField = f; if ( - this.findMatchingFieldIndex(f, AutoFillConstants.TotpFieldNames) > -1 || + this.findMatchingFieldIndex(f, [ + ...AutoFillConstants.TotpFieldNames, + ...AutoFillConstants.AmbiguousTotpFieldNames, + ]) > -1 || f.autoCompleteType === "one-time-code" ) { // We found an exact match. No need to keep looking. diff --git a/apps/browser/src/autofill/services/inline-menu-field-qualification.service.ts b/apps/browser/src/autofill/services/inline-menu-field-qualification.service.ts index 0b04b83ce4..9a98f65f7a 100644 --- a/apps/browser/src/autofill/services/inline-menu-field-qualification.service.ts +++ b/apps/browser/src/autofill/services/inline-menu-field-qualification.service.ts @@ -30,7 +30,6 @@ export class InlineMenuFieldQualificationService this.webAuthnAutocompleteValue, ]); private fieldIgnoreListString = AutoFillConstants.FieldIgnoreList.join(","); - private passwordFieldExcludeListString = AutoFillConstants.PasswordFieldExcludeList.join(","); private currentPasswordAutocompleteValue = "current-password"; private newPasswordAutoCompleteValue = "new-password"; private autofillFieldKeywordsMap: AutofillKeywordsMap = new WeakMap(); @@ -927,7 +926,7 @@ export class InlineMenuFieldQualificationService return false; } - return !(this.passwordFieldExcludeListString.indexOf(cleanedValue) > -1); + return !AutoFillConstants.PasswordFieldExcludeList.some((i) => cleanedValue.indexOf(i) > -1); } /** @@ -1094,13 +1093,29 @@ export class InlineMenuFieldQualificationService ]; const keywordsSet = new Set(); for (let i = 0; i < keywords.length; i++) { - if (typeof keywords[i] === "string") { - keywords[i] - .toLowerCase() - .replace(/-/g, "") - .replace(/[^a-zA-Z0-9]+/g, "|") - .split("|") - .forEach((keyword) => keywordsSet.add(keyword)); + if (keywords[i] && typeof keywords[i] === "string") { + let keywordEl = keywords[i].toLowerCase(); + keywordsSet.add(keywordEl); + + // Remove hyphens from all potential keywords, we want to treat these as a single word. + keywordEl = keywordEl.replace(/-/g, ""); + + // Split the keyword by non-alphanumeric characters to get the keywords without treating a space as a separator. + keywordEl.split(/[^\p{L}\d]+/gu).forEach((keyword) => { + if (keyword) { + keywordsSet.add(keyword); + } + }); + + // Collapse all spaces and split by non-alphanumeric characters to get the keywords + keywordEl + .replace(/\s/g, "") + .split(/[^\p{L}\d]+/gu) + .forEach((keyword) => { + if (keyword) { + keywordsSet.add(keyword); + } + }); } }