[PM-4127] Bugfix - Check original target tab URL before executing def…

…erred action due to reprompt (#6434) * remove solve for pm-3613 (will readdress in pm-4014) * check original target tab URL before executing deferred action due to reprompt * only check if target tab host+path changed during reprompt
bitwarden · Oct 13, 2023 · ee2f2e1 · ee2f2e1
1 parent 95d4406
commit ee2f2e1
Show file tree

Hide file tree

Showing 4 changed files with 12 additions and 52 deletions.
diff --git a/apps/browser/src/autofill/content/autofill.js b/apps/browser/src/autofill/content/autofill.js
@@ -993,11 +993,6 @@
       function fillTheElement(el, op) {
           var shouldCheck;
           if (el && null !== op && void 0 !== op && !(el.disabled || el.a || el.readOnly)) {
-              const tabURLChanged = !fillScript.savedUrls?.some(url => url.startsWith(window.location.origin))
-              // Check to make sure the page location didn't change
-              if (tabURLChanged) {
-                return;
-              }
               switch (markTheFilling && el.form && !el.form.opfilled && (el.form.opfilled = true),
               el.type ? el.type.toLowerCase() : null) {
                   case 'checkbox':

diff --git a/apps/browser/src/autofill/services/insert-autofill-content.service.spec.ts b/apps/browser/src/autofill/services/insert-autofill-content.service.spec.ts
@@ -108,7 +108,6 @@ describe("InsertAutofillContentService", () => {
       jest.spyOn(insertAutofillContentService as any, "fillingWithinSandboxedIframe");
       jest.spyOn(insertAutofillContentService as any, "userCancelledInsecureUrlAutofill");
       jest.spyOn(insertAutofillContentService as any, "userCancelledUntrustedIframeAutofill");
-      jest.spyOn(insertAutofillContentService as any, "tabURLChanged");
       jest.spyOn(insertAutofillContentService as any, "runFillScriptAction");
 
       insertAutofillContentService.fillForm(fillScript);
@@ -120,7 +119,6 @@ describe("InsertAutofillContentService", () => {
       expect(
         insertAutofillContentService["userCancelledUntrustedIframeAutofill"]
       ).not.toHaveBeenCalled();
-      expect(insertAutofillContentService["tabURLChanged"]).not.toHaveBeenCalled();
       expect(insertAutofillContentService["runFillScriptAction"]).not.toHaveBeenCalled();
     });
 
@@ -130,7 +128,6 @@ describe("InsertAutofillContentService", () => {
         .mockReturnValue(true);
       jest.spyOn(insertAutofillContentService as any, "userCancelledInsecureUrlAutofill");
       jest.spyOn(insertAutofillContentService as any, "userCancelledUntrustedIframeAutofill");
-      jest.spyOn(insertAutofillContentService as any, "tabURLChanged");
       jest.spyOn(insertAutofillContentService as any, "runFillScriptAction");
 
       insertAutofillContentService.fillForm(fillScript);
@@ -142,7 +139,6 @@ describe("InsertAutofillContentService", () => {
       expect(
         insertAutofillContentService["userCancelledUntrustedIframeAutofill"]
       ).not.toHaveBeenCalled();
-      expect(insertAutofillContentService["tabURLChanged"]).not.toHaveBeenCalled();
       expect(insertAutofillContentService["runFillScriptAction"]).not.toHaveBeenCalled();
     });
 
@@ -154,7 +150,6 @@ describe("InsertAutofillContentService", () => {
         .spyOn(insertAutofillContentService as any, "userCancelledInsecureUrlAutofill")
         .mockReturnValue(true);
       jest.spyOn(insertAutofillContentService as any, "userCancelledUntrustedIframeAutofill");
-      jest.spyOn(insertAutofillContentService as any, "tabURLChanged");
       jest.spyOn(insertAutofillContentService as any, "runFillScriptAction");
 
       insertAutofillContentService.fillForm(fillScript);
@@ -164,7 +159,6 @@ describe("InsertAutofillContentService", () => {
       expect(
         insertAutofillContentService["userCancelledUntrustedIframeAutofill"]
       ).not.toHaveBeenCalled();
-      expect(insertAutofillContentService["tabURLChanged"]).not.toHaveBeenCalled();
       expect(insertAutofillContentService["runFillScriptAction"]).not.toHaveBeenCalled();
     });
 
@@ -178,7 +172,6 @@ describe("InsertAutofillContentService", () => {
       jest
         .spyOn(insertAutofillContentService as any, "userCancelledUntrustedIframeAutofill")
         .mockReturnValue(true);
-      jest.spyOn(insertAutofillContentService as any, "tabURLChanged").mockReturnValue(false);
       jest.spyOn(insertAutofillContentService as any, "runFillScriptAction");
 
       insertAutofillContentService.fillForm(fillScript);
@@ -188,31 +181,6 @@ describe("InsertAutofillContentService", () => {
       expect(
         insertAutofillContentService["userCancelledUntrustedIframeAutofill"]
       ).toHaveBeenCalled();
-      expect(insertAutofillContentService["tabURLChanged"]).not.toHaveBeenCalled();
-      expect(insertAutofillContentService["runFillScriptAction"]).not.toHaveBeenCalled();
-    });
-
-    it("returns early if the page location origin does not match against any of the cipher saved URLs", () => {
-      jest
-        .spyOn(insertAutofillContentService as any, "fillingWithinSandboxedIframe")
-        .mockReturnValue(false);
-      jest
-        .spyOn(insertAutofillContentService as any, "userCancelledInsecureUrlAutofill")
-        .mockReturnValue(false);
-      jest
-        .spyOn(insertAutofillContentService as any, "userCancelledUntrustedIframeAutofill")
-        .mockReturnValue(false);
-      jest.spyOn(insertAutofillContentService as any, "tabURLChanged").mockReturnValue(true);
-      jest.spyOn(insertAutofillContentService as any, "runFillScriptAction");
-
-      insertAutofillContentService.fillForm(fillScript);
-
-      expect(insertAutofillContentService["fillingWithinSandboxedIframe"]).toHaveBeenCalled();
-      expect(insertAutofillContentService["userCancelledInsecureUrlAutofill"]).toHaveBeenCalled();
-      expect(
-        insertAutofillContentService["userCancelledUntrustedIframeAutofill"]
-      ).toHaveBeenCalled();
-      expect(insertAutofillContentService["tabURLChanged"]).toHaveBeenCalled();
       expect(insertAutofillContentService["runFillScriptAction"]).not.toHaveBeenCalled();
     });
 
@@ -226,7 +194,6 @@ describe("InsertAutofillContentService", () => {
       jest
         .spyOn(insertAutofillContentService as any, "userCancelledUntrustedIframeAutofill")
         .mockReturnValue(false);
-      jest.spyOn(insertAutofillContentService as any, "tabURLChanged").mockReturnValue(false);
       jest.spyOn(insertAutofillContentService as any, "runFillScriptAction");
 
       insertAutofillContentService.fillForm(fillScript);
@@ -236,7 +203,6 @@ describe("InsertAutofillContentService", () => {
       expect(
         insertAutofillContentService["userCancelledUntrustedIframeAutofill"]
       ).toHaveBeenCalled();
-      expect(insertAutofillContentService["tabURLChanged"]).toHaveBeenCalled();
       expect(insertAutofillContentService["runFillScriptAction"]).toHaveBeenCalledTimes(3);
       expect(insertAutofillContentService["runFillScriptAction"]).toHaveBeenNthCalledWith(
         1,

diff --git a/apps/browser/src/autofill/services/insert-autofill-content.service.ts b/apps/browser/src/autofill/services/insert-autofill-content.service.ts
@@ -38,25 +38,14 @@ class InsertAutofillContentService implements InsertAutofillContentServiceInterf
       !fillScript.script?.length ||
       this.fillingWithinSandboxedIframe() ||
       this.userCancelledInsecureUrlAutofill(fillScript.savedUrls) ||
-      this.userCancelledUntrustedIframeAutofill(fillScript) ||
-      this.tabURLChanged(fillScript.savedUrls)
+      this.userCancelledUntrustedIframeAutofill(fillScript)
     ) {
       return;
     }
 
     fillScript.script.forEach(this.runFillScriptAction);
   }
 
-  /**
-   * Determines if the page URL no longer matches one of the cipher's savedURL domains
-   * @param {string[] | null} savedUrls
-   * @returns {boolean}
-   * @private
-   */
-  private tabURLChanged(savedUrls?: AutofillScript["savedUrls"]): boolean {
-    return savedUrls && !savedUrls.some((url) => url.startsWith(window.location.origin));
-  }
-
   /**
    * Identifies if the execution of this script is happening
    * within a sandboxed iframe.

diff --git a/apps/browser/src/vault/popup/components/vault/view.component.ts b/apps/browser/src/vault/popup/components/vault/view.component.ts
@@ -331,11 +331,21 @@ export class ViewComponent extends BaseViewComponent {
   }
 
   private async doAutofill() {
+    const originalTabURL = this.tab.url?.length && new URL(this.tab.url);
+
     if (!(await this.promptPassword())) {
       return false;
     }
 
-    if (this.pageDetails == null || this.pageDetails.length === 0) {
+    const currentTabURL = this.tab.url?.length && new URL(this.tab.url);
+
+    const originalTabHostPath =
+      originalTabURL && `${originalTabURL.origin}${originalTabURL.pathname}`;
+    const currentTabHostPath = currentTabURL && `${currentTabURL.origin}${currentTabURL.pathname}`;
+
+    const tabUrlChanged = originalTabHostPath !== currentTabHostPath;
+
+    if (this.pageDetails == null || this.pageDetails.length === 0 || tabUrlChanged) {
       this.platformUtilsService.showToast("error", null, this.i18nService.t("autofillError"));
       return false;
     }