[PM-13115] Allow users to disable extension content script injections by domain #11826

jprusik · 2024-11-01T19:31:13Z

🎟️ Tracking

📔 Objective

Create a concept of disabled domains for the Bitwarden extension; unlike the excluded domains in the notification settings, a page belonging to the disabled domain list would prevent the Bitwarden extension from interacting with the page at all.

Notes

This feature is gated by the block-browser-injections-by-domain feature flag
While this work is functionally complete, there are follow up UX/design tasks (see tracked tasks in the ticket) that must be completed before this is released (via feature-flag)
This would not replace or otherwise impact the “excluded domains” concept for notifications (at least not within the scope of this PR; see comment)
Changes made to the blocked domains will not be reflected until the next content script injection to the page (typically a page navigation or refresh)

Screen capture

Kapture.2024-12-16.at.11.09.36.mp4

Kapture.2024-12-16.at.11.28.39.mp4

🦮 Reviewer guidelines

👍 (:+1:) or similar for great changes
📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
❓ (:question:) for questions
🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
🎨 (:art:) for suggestions / improvements
❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
⛏ (:pick:) for minor or nitpick changes

github-actions · 2024-11-11T22:18:16Z

Checkmarx One – Scan Summary & Details – b588e267-fc91-496b-b64c-714c24601841

No New Or Fixed Issues Found

jprusik · 2024-12-16T14:59:02Z

apps/browser/src/autofill/popup/settings/autofill.component.ts

@@ -67,7 +66,6 @@ import { PopupPageComponent } from "../../../platform/popup/layout/popup-page.co
    JslibModule,
    LinkModule,
    PopOutComponent,
-    PopupFooterComponent,


PopupFooterComponent was not in use anywhere here or in the template

jprusik · 2024-12-16T15:07:27Z

apps/browser/src/autofill/popup/settings/blocked-domains.component.ts

This file is largely a copy of apps/browser/src/autofill/popup/settings/excluded-domains.component.ts

We have an open bug for the existing UI here PM-13808 with the input styling. Do you think we can get that fixed so we don't duplicate the UI into this new component?

jprusik · 2024-12-16T15:11:12Z

...popup/components/vault-v2/autofill-vault-list-items/autofill-vault-list-items.component.html

@@ -3,6 +3,7 @@
  [ciphers]="ciphers"
  [title]="'autofillSuggestions' | i18n"
  [showRefresh]="showRefresh"
+  [sectionIndicators]="sectionIndicators"


This concept allows for the conditional addition of icon indicators alongside the section title (in this case, an informational icon indicating the view contains ciphers for a domain that has been added to the user's block list).

jprusik · 2024-12-16T15:25:23Z

apps/browser/src/vault/popup/components/vault/vault-v2.component.html

+  <bit-banner
+    *ngIf="vaultState !== VaultStateEnum.Empty && showScriptInjectionIsBlockedBanner"
+    id="domain-script-injection-blocked-banner"
+    bannerType="info"
+    (onClose)="handleScriptInjectionIsBlockedBannerDismiss()"
+  >
+    {{ "autofillBlockedNotice" | i18n }}
+  </bit-banner>


Follow up work will ultimately leverage the pop up page component slot added in #12279

jprusik · 2024-12-16T15:49:09Z

apps/browser/src/vault/popup/components/vault/vault-v2.component.ts

+        this.domainSettingsService
+          .setBlockedInteractionsUris({
+            ...blockedURIs,
+            [this.autofillTabHostname]: { bannerIsDismissed: true },


using the existing NeverDomains data shape give us tons of flexibility in the future if we implement more granular injection-blocking options.

As it is, we could merge the neverDomains state concept (expressed in the interface as "Excluded Domains" and currently used to allow users to block the notification bar on specified domains) into blockedInteractionsUris concept with a state migration.

For example (not a well-considered data shape, just for illustrative purposes):

{ "banking-portal.real-tld": { "blockedInjections": null, // all injections blocked "dismissedMessages": ['autofillBlockedBanner'] }, "duckduckgo.com": { "blockedInjections": ["notifications", "inline-menu"], "dismissedMessages": [] } }

For example (not a well-considered data shape, just for illustrative purposes):

💭 You might want to update this example to account for our "no-null" guidance. If you want to keep the example short, I'd just swap out the null for true or "all" (no list).

A more comprehensive example that would be easier to port to Rust (as an enum) uses a discriminated union.

type ScriptType = "notifications" | "inline-menu"; type Injectable = { type: "allow-all" } | { type: "deny-list", injections: ScriptType[] } | { type: "deny-all" };

jprusik · 2024-12-16T16:00:19Z

apps/browser/src/platform/services/browser-script-injector.service.ts

+    if (!injectionAllowedInTab) {
+      throw new Error("This URI of this tab is on the blocked domains list.");
+    }


This is purposeful blocking, no? Do we want a visible error like this?

… content scripts

…s to autofill settings

… is on the blocked domains list

…n header

…d it

codecov · 2024-12-19T21:16:18Z

Codecov Report

Attention: Patch coverage is 14.37908% with 131 lines in your changes missing coverage. Please review.

Project coverage is 33.54%. Comparing base (997d40f) to head (ab2f085).
Report is 2 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...tofill/popup/settings/blocked-domains.component.ts	0.00%	73 Missing ⚠️
...vault/popup/components/vault/vault-v2.component.ts	0.00%	28 Missing ⚠️
...-container/vault-list-items-container.component.ts	0.00%	7 Missing ⚠️
...ofill/popup/settings/excluded-domains.component.ts	0.00%	6 Missing ⚠️
...n/src/autofill/services/domain-settings.service.ts	53.84%	5 Missing and 1 partial ⚠️
...-list-items/autofill-vault-list-items.component.ts	0.00%	3 Missing ⚠️
...c/autofill/popup/settings/autofill-v1.component.ts	0.00%	2 Missing ⚠️
.../src/autofill/popup/settings/autofill.component.ts	0.00%	2 Missing ⚠️
...atform/services/browser-script-injector.service.ts	83.33%	0 Missing and 2 partials ⚠️
apps/browser/src/background/main.background.ts	0.00%	1 Missing ⚠️
... and 1 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #11826      +/-   ##
==========================================
- Coverage   33.56%   33.54%   -0.03%     
==========================================
  Files        2926     2927       +1     
  Lines       91527    91658     +131     
  Branches    17395    17422      +27     
==========================================
+ Hits        30721    30744      +23     
- Misses      58392    58497     +105     
- Partials     2414     2417       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

audreyality · 2024-12-20T16:02:52Z

apps/browser/src/tools/background/fileless-importer.background.spec.ts

@@ -93,6 +102,7 @@ describe("FilelessImporterBackground ", () => {
    });

    it("posts a message to the port indicating that the fileless import feature is disabled if the user's auth status is not unlocked", async () => {
+      (firstValueFrom as jest.Mock).mockResolvedValue(false);


♻️ firstValueFrom may be used in many places in the code. This line makes every call return false, which is almost certainly incorrect behavior. The observable passed to firstValueFrom should be mocked instead.

🤔 If firstValueFrom is being called from within an autofill API, consider implementing a fake for the API or some other means to simulate the awaited observable. If it's being called from tools code, then I'll follow up with the tools team to fix the test.

audreyality · 2024-12-20T16:09:40Z

libs/common/src/autofill/services/domain-settings.service.ts

+  blockedInteractionsUris$: Observable<NeverDomains>;
+  setBlockedInteractionsUris: (newValue: NeverDomains) => Promise<void>;


⛏️ Since these members are exported from this module and overridden in numerous tests, their behavior should be documented.

audreyality · 2024-12-20T16:21:00Z

libs/common/src/autofill/services/domain-settings.service.ts

+    this.blockedInteractionsUris$ = combineLatest([
+      this.blockedInteractionsUrisState.state$,
+      this.configService?.getFeatureFlag$(FeatureFlag.BlockBrowserInjectionsByDomain),
+    ]).pipe(
+      map(([blockedUris, blockBrowserInjectionsByDomainEnabled]) => {
+        if (!blockBrowserInjectionsByDomainEnabled) {
+          return null;
+        }
+
+        return blockedUris ?? null;
+      }),
+    );


🎨 This can be simplified if configService becomes required.

this.configService.getFeatureFlag$(FeatureFlag.BlockBrowserInjectionsByDomain).pipe( switchMap((enabled) => enabled ? this.blockedInteractionsUrisState.state$ : of([])), map(blocked => blocked ?? []) );

The main benefit of this is that it won't emit false updates from the state service when the feature is disabled.

One thing to note, however, is that combineLatest only completes when both of its input streams complete. Since blockedInteractionsUrisState.state$ never completes, neither will blockedInteractionsUris$.

switchMap, on the other hand, completes once its source and argument streams complete. There's a subtle difference between the constructions because of([]) completes after emitting its last argument. If getFeatureFlag$ completes after emitting false then the whole observable will complete.

Ultimately, which construction works best for you depends on how you consume this.blockedInteractionsUris$. If you want it to update as soon as the flag becomes available, then combineLatest (with logic to filter false updates) is preferable. Your notes indicate that you need to navigate in order for it to pick up the new settings, however, which leads me to believe the switchMap approach more closely models your expectations.

📝 I also eliminated null in my example code per ADR-14.

coltonhurst

Two questions just to start!

coltonhurst · 2024-12-20T17:13:39Z

apps/browser/src/_locales/en/messages.json

+    "message": "Autofill and other related features will not be offered for these websites. You must refresh the page for changes to take effect."
+  },
+  "autofillBlockedNotice": {
+    "message": "Autofill is blocked for this website. Review or change this in settings."


Do we have any way to deep link to the relevant settings page in the notice?

coltonhurst · 2024-12-20T18:29:24Z

apps/browser/src/platform/services/browser-script-injector.service.ts

+    if (!injectionAllowedInTab) {
+      throw new Error("This URI of this tab is on the blocked domains list.");
+    }


This is purposeful blocking, no? Do we want a visible error like this?

jprusik self-assigned this Nov 1, 2024

jprusik force-pushed the pm-13115 branch 2 times, most recently from 77d3472 to e8ddd67 Compare November 1, 2024 19:53

jprusik force-pushed the pm-13115 branch from e8ddd67 to 5b208b3 Compare November 11, 2024 22:07

jprusik force-pushed the pm-13115 branch from c03f070 to f96b5f1 Compare November 15, 2024 22:56

jprusik force-pushed the pm-13115 branch 3 times, most recently from 308c302 to 2695a47 Compare December 16, 2024 14:51

jprusik commented Dec 16, 2024

View reviewed changes

jprusik force-pushed the pm-13115 branch 6 times, most recently from fe32841 to 1dd180e Compare December 17, 2024 16:25

jprusik added 9 commits December 19, 2024 15:48

add disabledInteractionsUris state to the domain settings service

0fe1376

add routes and ui for user disabledInteractionsUris state management

b37ba48

use disabled URIs service state as a preemptive conditon to injecting…

92a9cdc

… content scripts

move disabled domains navigation button from account security setting…

ef8530d

…s to autofill settings

update disabled domain terminology to blocked domain terminology

dc5048f

update copy

017aaae

handle blocked domains initializing with null value

16bcb32

add dismissable banner to the vault view when the active autofill tab…

941c6ff

… is on the blocked domains list

add autofill blocked domain indicators to autofill suggestions sectio…

453c745

…n header

jprusik added 3 commits December 19, 2024 15:48

add BlockBrowserInjectionsByDomain feature flag and put feature behin…

63e8faa

…d it

update router config to new style

1fe4232

update tests and cleanup

ab2f085

jprusik force-pushed the pm-13115 branch from 1dd180e to ab2f085 Compare December 19, 2024 20:54

jprusik changed the title ~~[PM-13115] Allow users to disable the Bitwarden extension by domain~~ [PM-13115] Allow users to disable extension content script injections by domain Dec 19, 2024

jprusik marked this pull request as ready for review December 19, 2024 21:05

jprusik requested review from a team as code owners December 19, 2024 21:05

jprusik requested review from addisonbeck, shane-melton, danielleflinn and wsalmon-bw December 19, 2024 21:05

audreyality self-requested a review December 20, 2024 15:46

audreyality reviewed Dec 20, 2024

View reviewed changes

coltonhurst reviewed Dec 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PM-13115] Allow users to disable extension content script injections by domain #11826

[PM-13115] Allow users to disable extension content script injections by domain #11826

jprusik commented Nov 1, 2024 •

edited

Loading

github-actions bot commented Nov 11, 2024 •

edited

Loading

jprusik Dec 16, 2024 •

edited

Loading

jprusik Dec 16, 2024

danielleflinn Dec 19, 2024

jprusik Dec 16, 2024

jprusik Dec 16, 2024

jprusik Dec 16, 2024

audreyality Dec 20, 2024

jprusik Dec 16, 2024

coltonhurst Dec 20, 2024

codecov bot commented Dec 19, 2024 •

edited

Loading

audreyality Dec 20, 2024

audreyality Dec 20, 2024

audreyality Dec 20, 2024 •

edited

Loading

coltonhurst left a comment

coltonhurst Dec 20, 2024

coltonhurst Dec 20, 2024

		blockedInteractionsUris$: Observable<NeverDomains>;
		setBlockedInteractionsUris: (newValue: NeverDomains) => Promise<void>;

[PM-13115] Allow users to disable extension content script injections by domain #11826

Are you sure you want to change the base?

[PM-13115] Allow users to disable extension content script injections by domain #11826

Conversation

jprusik commented Nov 1, 2024 • edited Loading

🎟️ Tracking

📔 Objective

Notes

Screen capture

🦮 Reviewer guidelines

github-actions bot commented Nov 11, 2024 • edited Loading

No New Or Fixed Issues Found

jprusik Dec 16, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 19, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

audreyality Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

coltonhurst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jprusik commented Nov 1, 2024 •

edited

Loading

github-actions bot commented Nov 11, 2024 •

edited

Loading

jprusik Dec 16, 2024 •

edited

Loading

codecov bot commented Dec 19, 2024 •

edited

Loading

audreyality Dec 20, 2024 •

edited

Loading