[PM-12775] Password autofill should not occur within 2FA fields (#11303)

bitwarden · Oct 3, 2024 · bf38f2d · bf38f2d
1 parent f2d3311
commit bf38f2d
Show file tree

Hide file tree

Showing 6 changed files with 57 additions and 32 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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<string>();
       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);
+              }
+            });
         }
       }