[PM-1397] Display a warning when a user attempts to auto-fill an iframe #4994

eliykat · 2023-03-14T05:00:49Z

Type of change

- [ ] Bug fix
- [ ] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

Warn the user before autofilling into an untrusted iframe.

Code changes

autofillService - decide whether an iframe is "trusted" and include this result in the fillScript. FillScripts targeting an untrusted iframe can be discarded (used for autofill on page load) or used but this will be picked up by autofill.js.
autofill.js - display an alert to the user before filling an untrusted iframe
settingsService - add a method to get equivalent domains for a URL
autofillPageDetails - this had a tabUrl property which was incorrectly set by autofill.js (it would always be the url of the page, which in an iframe just gives you the src of the iframe rather than the outer tab). This made terminology confusing when we want to distinguish the url of the tab and the url of an iframe. It appeared to be unused so I deleted it.

Everything else is updating types/interfaces/parameters, adding tests, and adding documentation comments where it was helpful.

Screenshots

Before you submit

Please add unit tests where it makes sense to do so (encouraged but not required)
If this change requires a documentation update - notify the documentation team
If this change has particular deployment requirements - notify the DevOps team
Ensure that all UI additions follow WCAG AA requirements

Co-authored-by: Todd Martin <106564991+trmartin4@users.noreply.github.com>

Try/catch does not work consistently around the call to confirm() This reverts commit 3046419.

MGibson1 · 2023-03-14T13:18:39Z

libs/common/src/abstractions/state.service.ts

@@ -245,7 +245,7 @@ export abstract class StateService<T extends Account = Account> {
  setEntityType: (value: string, options?: StorageOptions) => Promise<void>;
  getEnvironmentUrls: (options?: StorageOptions) => Promise<EnvironmentUrls>;
  setEnvironmentUrls: (value: EnvironmentUrls, options?: StorageOptions) => Promise<void>;
-  getEquivalentDomains: (options?: StorageOptions) => Promise<any>;
+  getEquivalentDomains: (options?: StorageOptions) => Promise<string[][]>;


I add a bit of typing each time I use it 😇

it's a little treat to myself

MGibson1 · 2023-03-14T13:24:45Z

apps/browser/src/autofill/services/autofill.service.ts

+        this.logService.info("Auto-fill on page load was blocked due to an untrusted iframe.");
+        return;


I had thought we were going to display a popup to the user. This would fail silently to most people.

This is only hit for autofill on page load case, which I agree that we should fail on.

MGibson1 · 2023-03-14T13:28:44Z

apps/browser/src/autofill/services/autofill.service.ts

+        options.allowUntrustedIframe != undefined &&
+        !options.allowUntrustedIframe


This untrusted iframe stuff seems very weird if there's no user override. It feels like we're excluding command autofill from some legitimate safety measures

apps/browser/src/autofill/services/autofill.service.ts

Co-authored-by: Todd Martin <106564991+trmartin4@users.noreply.github.com>

MGibson1 · 2023-03-14T19:14:10Z

apps/browser/src/autofill/services/autofill.service.ts

      this.logService.debug("iframe at " + pageUrl + " trusted because it matches a saved URI");
      return false;
    }

    return true;
  }

+  // TODO should this be put in a common place (Utils maybe?) to be used both here and by CipherService?


Do you really need to recreate the domain matching rules to get access to them here? :(

They're currently buried in a callback in cipherService.getAllDecryptedForUrl, so they're not currently accessible. My thought was to extract it to its own public method for reuse, if there are no changes to the code itself.

But this is being treated as a hotfix so I understand the reluctance to refactor unrelated existing methods.

EDIT: and I think that is correct here - I'm OK with the copy + paste personally, provided there's a follow up ticket.

I did not see another way, besides spending a lot more time than is needed for a hotfix kind of item.

MGibson1 · 2023-03-14T19:20:16Z

apps/browser/src/autofill/content/autofill.js

+            var acceptedIframeWarning = confirm("Are you sure you want to auto-fill your credentials on " +
+              window.location.hostname + "? Choose OK to auto-fill, or Cancel to stop.");


It's hard to trace context here, does this display the iframe source's hostname?

Is this display once per iframe in the page?

It's definitely the iframe's source, but I haven't been able to test multiple untrusted iframes, yet. I think it'll call once per untrusted iframe since doAutofill is called once per content script (per iframe message response)

I agree that:

it'll call once per untrusted iframe since doAutofill is called once per content script (per iframe message response)

Although I haven't tested it.

But, it'll only call once per untrusted iframe that we're going to autofill. If an iframe doesn't have a login form to be filled, no fillScript is sent back to the content script for those pageDetails.

So it is possible that you get multiple alerts, but only if you have multiple untrusted iframes with login forms in them. Which I don't think is very common, and if it does happen, the risk is still there to warn the user about. The UX could be improved, but I think that's OK to be out of scope here given that it should be an edge case. (although obviously I am making some assumptions here)

This will display once per iframe, but if there are multiple it will cancel autofill for all but only show the last one.

apps/browser/src/autofill/content/autofill.js

apps/browser/src/autofill/services/autofill.service.ts

eliykat · 2023-03-14T21:37:41Z

apps/browser/src/autofill/services/autofill.service.ts

+  }
+
+  // TODO should this be put in a common place (Utils maybe?) to be used both here and by CipherService?
+  private uriMatches(uri: LoginUriView, url: string): boolean {


This is just a lift & shift from CipherService?

Relevant discussion: #4994 (comment)

apps/browser/src/autofill/services/autofill.service.ts

…fill-on-a-site-with-an-iframe' of https://github.com/bitwarden/clients into PM-1397-display-a-warning-when-a-user-attempts-to-auto-fill-on-a-site-with-an-iframe

differsthecat · 2023-03-15T00:36:10Z

apps/browser/src/autofill/services/autofill.service.ts

    fillScript.untrustedIframe =
-      pageDetails.url !== options.tabUrl && // If page URL matches tab URL, we're not in an iframe
-      !this.iframeUrlMatches(pageDetails.url, options.cipher, options.defaultUriMatch);
+      inIframe && !this.iframeUrlMatches(pageDetails.url, options.cipher, options.defaultUriMatch);


I like this inFrame check out of the madness of the other method 💯

differsthecat

Looks good, thank you @eliykat !

…me (#4994) * add settingsService.getEquivalentDomains * check that an iframe URL matches cipher.login.uris before autofilling * disable autofill on page load if it doesn't match * show a warning to the user on regular autofill if it doesn't match --------- Co-authored-by: Todd Martin <106564991+trmartin4@users.noreply.github.com> Co-authored-by: Robyn MacCallum <robyntmaccallum@gmail.com> (cherry picked from commit 0d85bdc)

eliykat and others added 22 commits March 13, 2023 14:51

WIP put basic plumbing in

3d9ee5a

Add alert

609dcf4

Co-authored-by: Todd Martin <106564991+trmartin4@users.noreply.github.com>

Add documentation and start to fill out logic

472a774

clarify confusing names

8c80815

Create method for getting equivalent domains

83c86ee

Finish first pass of untrusted iframe logic

417aa56

Make untrustedIframe interface more readable

bdc850a

Disable autofill on page load for untrusted iframes

c1da67f

Add logging

5471a63

Tidy up

e96bd88

Update warning text

56229c1

Add try catch around autofill confirmation

3046419

Revert "Add try catch around autofill confirmation"

d1d093a

Try/catch does not work consistently around the call to confirm() This reverts commit 3046419.

Add comment explaining sandbox behaviour

d9ba0a2

Stub out tests

5de3a9a

Build out unit tests

eddf752

Simplify tests, make method public

ea0ad6a

Match domains not hostnames

18a6748

simplify tests

84667b2

Remove unnecessary setup

9b06a46

Remove unused and incorrect pageDetails.tabUrl property

3c01417

Don't repeat method calls

4cc9a06

eliykat requested a review from differsthecat March 14, 2023 05:00

eliykat requested a review from a team as a code owner March 14, 2023 05:00

github-actions bot added the needs-qa Marks a PR as requiring QA approval label Mar 14, 2023

MGibson1 reviewed Mar 14, 2023

View reviewed changes

differsthecat added 2 commits March 14, 2023 14:20

Use the uri's match detection srategy to find a match

a0c95fc

Updated untrustediframe tests

c8ab900

trmartin4 reviewed Mar 14, 2023

View reviewed changes

apps/browser/src/autofill/services/autofill.service.ts Outdated Show resolved Hide resolved

Update apps/browser/src/autofill/services/autofill.service.ts

fdd3920

Co-authored-by: Todd Martin <106564991+trmartin4@users.noreply.github.com>

MGibson1 reviewed Mar 14, 2023

View reviewed changes

eliykat commented Mar 14, 2023

View reviewed changes

differsthecat and others added 5 commits March 14, 2023 19:17

simplify urlMatched

9a8f9ed

Only use match detection, fix match detection

8581094

Replace comment with variable

852d57e

Merge branch 'PM-1397-display-a-warning-when-a-user-attempts-to-auto-…

5d0988f

…fill-on-a-site-with-an-iframe' of https://github.com/bitwarden/clients into PM-1397-display-a-warning-when-a-user-attempts-to-auto-fill-on-a-site-with-an-iframe

Concat all possible equivalent domains

f1eb68e

differsthecat reviewed Mar 15, 2023

View reviewed changes

eliykat and others added 2 commits March 15, 2023 10:37

Remove disabled tests

c71976f

Update warning text

b4341d7

differsthecat approved these changes Mar 15, 2023

View reviewed changes

eliykat merged commit 0d85bdc into master Mar 15, 2023

eliykat deleted the PM-1397-display-a-warning-when-a-user-attempts-to-auto-fill-on-a-site-with-an-iframe branch March 15, 2023 01:19

trmartin4 mentioned this pull request Apr 2, 2023

[PM-1685] Remove ShadowRoot changes to autofill that decreased performance #5147

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PM-1397] Display a warning when a user attempts to auto-fill an iframe #4994

[PM-1397] Display a warning when a user attempts to auto-fill an iframe #4994

eliykat commented Mar 14, 2023 •

edited

Loading

MGibson1 Mar 14, 2023

eliykat Mar 14, 2023

eliykat Mar 14, 2023

MGibson1 Mar 14, 2023

differsthecat Mar 14, 2023

MGibson1 Mar 14, 2023

MGibson1 Mar 14, 2023

eliykat Mar 14, 2023 •

edited

Loading

eliykat Mar 14, 2023 •

edited

Loading

differsthecat Mar 14, 2023

MGibson1 Mar 14, 2023

MGibson1 Mar 14, 2023 •

edited

Loading

eliykat Mar 14, 2023 •

edited

Loading

differsthecat Mar 14, 2023

eliykat Mar 14, 2023

differsthecat Mar 14, 2023

differsthecat Mar 14, 2023

differsthecat Mar 15, 2023

differsthecat left a comment

		this.logService.info("Auto-fill on page load was blocked due to an untrusted iframe.");
		return;

		options.allowUntrustedIframe != undefined &&
		!options.allowUntrustedIframe

		var acceptedIframeWarning = confirm("Are you sure you want to auto-fill your credentials on " +
		window.location.hostname + "? Choose OK to auto-fill, or Cancel to stop.");

[PM-1397] Display a warning when a user attempts to auto-fill an iframe #4994

[PM-1397] Display a warning when a user attempts to auto-fill an iframe #4994

Conversation

eliykat commented Mar 14, 2023 • edited Loading

Type of change

Objective

Code changes

Screenshots

Before you submit

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliykat Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

eliykat Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MGibson1 Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

eliykat Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

differsthecat left a comment

Choose a reason for hiding this comment

eliykat commented Mar 14, 2023 •

edited

Loading

eliykat Mar 14, 2023 •

edited

Loading

eliykat Mar 14, 2023 •

edited

Loading

MGibson1 Mar 14, 2023 •

edited

Loading

eliykat Mar 14, 2023 •

edited

Loading