Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weโ€™ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PM-12548] Fido2 scripts should not load when user is logged out #11444

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -1484,9 +1484,7 @@ export class OverlayBackground implements OverlayBackgroundInterface {
}

/**
* Gets the user's authentication status from the auth service. If the user's authentication
* status has changed, the inline menu button's authentication status will be updated
* and the inline menu list's ciphers will be updated.
* Gets the user's authentication status from the auth service.
*/
private async getAuthStatus() {
return await firstValueFrom(this.authService.activeAccountStatus$);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ type Fido2BackgroundExtensionMessageHandlers = {

interface Fido2Background {
init(): void;
injectFido2ContentScriptsInAllTabs(): Promise<void>;
}

export {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import { mock, MockProxy } from "jest-mock-extended";
import { BehaviorSubject } from "rxjs";

import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service";
import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status";
import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service";
import { Fido2ActiveRequestManager } from "@bitwarden/common/platform/abstractions/fido2/fido2-active-request-manager.abstraction";
import {
Expand Down Expand Up @@ -59,6 +61,8 @@ describe("Fido2Background", () => {
let scriptInjectorServiceMock!: MockProxy<BrowserScriptInjectorService>;
let configServiceMock!: MockProxy<ConfigService>;
let enablePasskeysMock$!: BehaviorSubject<boolean>;
let activeAccountStatusMock$: BehaviorSubject<AuthenticationStatus>;
let authServiceMock!: MockProxy<AuthService>;
let fido2Background!: Fido2Background;

beforeEach(() => {
Expand All @@ -81,13 +85,17 @@ describe("Fido2Background", () => {
vaultSettingsService.enablePasskeys$ = enablePasskeysMock$;
fido2ActiveRequestManager = mock<Fido2ActiveRequestManager>();
fido2ClientService.isFido2FeatureEnabled.mockResolvedValue(true);
activeAccountStatusMock$ = new BehaviorSubject(AuthenticationStatus.Unlocked);
authServiceMock = mock<AuthService>();
authServiceMock.activeAccountStatus$ = activeAccountStatusMock$;
fido2Background = new Fido2Background(
logService,
fido2ActiveRequestManager,
fido2ClientService,
vaultSettingsService,
scriptInjectorServiceMock,
configServiceMock,
authServiceMock,
);
fido2Background["abortManager"] = abortManagerMock;
abortManagerMock.runWithAbortController.mockImplementation((_requestId, runner) =>
Expand All @@ -101,62 +109,39 @@ describe("Fido2Background", () => {
jest.clearAllMocks();
});

describe("injectFido2ContentScriptsInAllTabs", () => {
it("does not inject any FIDO2 content scripts when no tabs have a secure url protocol", async () => {
const insecureTab = mock<chrome.tabs.Tab>({ id: 789, url: "http://example.com" });
tabsQuerySpy.mockResolvedValueOnce([insecureTab]);
describe("handleAuthStatusUpdate", () => {
let updateContentScriptRegistrationSpy: jest.SpyInstance;

await fido2Background.injectFido2ContentScriptsInAllTabs();

expect(scriptInjectorServiceMock.inject).not.toHaveBeenCalled();
beforeEach(() => {
updateContentScriptRegistrationSpy = jest
.spyOn(fido2Background as any, "updateContentScriptRegistration")
.mockImplementation();
});

it("only injects the FIDO2 content script into tabs that contain a secure url protocol", async () => {
const secondTabMock = mock<chrome.tabs.Tab>({ id: 456, url: "https://example.com" });
const insecureTab = mock<chrome.tabs.Tab>({ id: 789, url: "http://example.com" });
const noUrlTab = mock<chrome.tabs.Tab>({ id: 101, url: undefined });
tabsQuerySpy.mockResolvedValueOnce([tabMock, secondTabMock, insecureTab, noUrlTab]);
it("skips triggering the passkeys settings update if the user is logged out", async () => {
activeAccountStatusMock$.next(AuthenticationStatus.LoggedOut);

await fido2Background.injectFido2ContentScriptsInAllTabs();
fido2Background.init();
await flushPromises();

expect(scriptInjectorServiceMock.inject).toHaveBeenCalledWith({
tabId: tabMock.id,
injectDetails: contentScriptDetails,
});
expect(scriptInjectorServiceMock.inject).toHaveBeenCalledWith({
tabId: secondTabMock.id,
injectDetails: contentScriptDetails,
});
expect(scriptInjectorServiceMock.inject).not.toHaveBeenCalledWith({
tabId: insecureTab.id,
injectDetails: contentScriptDetails,
});
expect(scriptInjectorServiceMock.inject).not.toHaveBeenCalledWith({
tabId: noUrlTab.id,
injectDetails: contentScriptDetails,
});
expect(updateContentScriptRegistrationSpy).not.toHaveBeenCalled();
});

it("injects the `page-script.js` content script into the provided tab", async () => {
tabsQuerySpy.mockResolvedValueOnce([tabMock]);
it("triggers the passkeys setting update if the user is logged in", async () => {
activeAccountStatusMock$.next(AuthenticationStatus.Unlocked);

await fido2Background.injectFido2ContentScriptsInAllTabs();
fido2Background.init();
await flushPromises();

expect(scriptInjectorServiceMock.inject).toHaveBeenCalledWith({
tabId: tabMock.id,
injectDetails: sharedScriptInjectionDetails,
mv2Details: { file: Fido2ContentScript.PageScriptAppend },
mv3Details: { file: Fido2ContentScript.PageScript, world: "MAIN" },
});
expect(updateContentScriptRegistrationSpy).toHaveBeenCalled();
});
});

describe("handleEnablePasskeysUpdate", () => {
let portMock!: MockProxy<chrome.runtime.Port>;

beforeEach(() => {
jest.spyOn(fido2Background as any, "handleAuthStatusUpdate").mockImplementation();
fido2Background.init();
jest.spyOn(BrowserApi, "registerContentScriptsMv2");
jest.spyOn(BrowserApi, "registerContentScriptsMv3");
Expand All @@ -168,6 +153,15 @@ describe("Fido2Background", () => {
tabsQuerySpy.mockResolvedValue([tabMock]);
});

it("skips handling the passkey update if the user is logged out", async () => {
activeAccountStatusMock$.next(AuthenticationStatus.LoggedOut);

enablePasskeysMock$.next(true);

expect(portMock.disconnect).not.toHaveBeenCalled();
expect(scriptInjectorServiceMock.inject).not.toHaveBeenCalled();
});

it("does not destroy and re-inject the content scripts when triggering `handleEnablePasskeysUpdate` with an undefined currentEnablePasskeysSetting property", async () => {
await flushPromises();

Expand Down Expand Up @@ -421,6 +415,7 @@ describe("Fido2Background", () => {
let portMock!: MockProxy<chrome.runtime.Port>;

beforeEach(() => {
jest.spyOn(fido2Background as any, "handleAuthStatusUpdate").mockImplementation();
fido2Background.init();
portMock = createPortSpyMock(Fido2PortName.InjectedScript);
triggerRuntimeOnConnectEvent(portMock);
Expand Down
43 changes: 39 additions & 4 deletions apps/browser/src/autofill/fido2/background/fido2.background.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import { firstValueFrom, startWith } from "rxjs";
import { firstValueFrom, startWith, Subscription } from "rxjs";
import { pairwise } from "rxjs/operators";

import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service";
import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status";
import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum";
import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service";
import { Fido2ActiveRequestManager } from "@bitwarden/common/platform/abstractions/fido2/fido2-active-request-manager.abstraction";
Expand Down Expand Up @@ -29,6 +31,7 @@ import {
} from "./abstractions/fido2.background";

export class Fido2Background implements Fido2BackgroundInterface {
private currentAuthStatus$: Subscription;
private abortManager = new AbortManager();
private fido2ContentScriptPortsSet = new Set<chrome.runtime.Port>();
private registeredContentScripts: browser.contentScripts.RegisteredContentScript;
Expand All @@ -55,6 +58,7 @@ export class Fido2Background implements Fido2BackgroundInterface {
private vaultSettingsService: VaultSettingsService,
private scriptInjectorService: ScriptInjectorService,
private configService: ConfigService,
private authService: AuthService,
) {}

/**
Expand All @@ -68,12 +72,32 @@ export class Fido2Background implements Fido2BackgroundInterface {
this.vaultSettingsService.enablePasskeys$
.pipe(startWith(undefined), pairwise())
.subscribe(([previous, current]) => this.handleEnablePasskeysUpdate(previous, current));
this.currentAuthStatus$ = this.authService.activeAccountStatus$
.pipe(startWith(undefined), pairwise())
.subscribe(([_previous, current]) => this.handleAuthStatusUpdate(current));
}

/**
* Handles initializing the FIDO2 content scripts based on the current
* authentication status. We only want to inject the FIDO2 content scripts
* if the user is logged in.
*
* @param authStatus - The current authentication status.
*/
private async handleAuthStatusUpdate(authStatus: AuthenticationStatus) {
if (authStatus === AuthenticationStatus.LoggedOut) {
return;
}

const enablePasskeys = await this.isPasskeySettingEnabled();
await this.handleEnablePasskeysUpdate(enablePasskeys, enablePasskeys);
this.currentAuthStatus$.unsubscribe();
}

/**
* Injects the FIDO2 content and page script into all existing browser tabs.
*/
async injectFido2ContentScriptsInAllTabs() {
private async injectFido2ContentScriptsInAllTabs() {
const tabs = await BrowserApi.tabsQuery({});

for (let index = 0; index < tabs.length; index++) {
Expand All @@ -85,6 +109,13 @@ export class Fido2Background implements Fido2BackgroundInterface {
}
}

/**
* Gets the user's authentication status from the auth service.
*/
private async getAuthStatus() {
return await firstValueFrom(this.authService.activeAccountStatus$);
}

/**
* Handles reacting to the enablePasskeys setting being updated. If the setting
* is enabled, the FIDO2 content scripts are injected into all tabs. If the setting
Expand All @@ -98,13 +129,17 @@ export class Fido2Background implements Fido2BackgroundInterface {
previousEnablePasskeysSetting: boolean,
enablePasskeys: boolean,
) {
this.fido2ActiveRequestManager.removeAllActiveRequests();
await this.updateContentScriptRegistration();
if ((await this.getAuthStatus()) === AuthenticationStatus.LoggedOut) {
return;
}

if (previousEnablePasskeysSetting === undefined) {
return;
}

this.fido2ActiveRequestManager.removeAllActiveRequests();
await this.updateContentScriptRegistration();

this.destroyLoadedFido2ContentScripts();
if (enablePasskeys) {
void this.injectFido2ContentScriptsInAllTabs();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

const script = globalContext.document.createElement("script");
script.src = chrome.runtime.getURL("content/fido2-page-script.js");
script.async = false;

const scriptInsertionPoint =
globalContext.document.head || globalContext.document.documentElement;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

const script = globalContext.document.createElement("script");
script.src = chrome.runtime.getURL("content/fido2-page-script.js");
script.async = false;

Check warning on line 12 in apps/browser/src/autofill/fido2/content/fido2-page-script-delay-append.mv2.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/autofill/fido2/content/fido2-page-script-delay-append.mv2.ts#L12

Added line #L12 was not covered by tests

// We are ensuring that the script injection is delayed in the event that we are loading
// within an iframe element. This prevents an issue with web mail clients that load content
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,12 @@
import { Messenger } from "./messaging/messenger";

(function (globalContext) {
if (globalContext.document.currentScript) {
globalContext.document.currentScript.parentNode.removeChild(

Check warning on line 8 in apps/browser/src/autofill/fido2/content/fido2-page-script.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/autofill/fido2/content/fido2-page-script.ts#L8

Added line #L8 was not covered by tests
globalContext.document.currentScript,
);
}

const shouldExecuteContentScript =
globalContext.document.contentType === "text/html" &&
(globalContext.document.location.protocol === "https:" ||
Expand Down
2 changes: 1 addition & 1 deletion apps/browser/src/background/main.background.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1104,6 +1104,7 @@ export default class MainBackground {
this.vaultSettingsService,
this.scriptInjectorService,
this.configService,
this.authService,
);

const lockService = new DefaultLockService(this.accountService, this.vaultTimeoutService);
Expand All @@ -1119,7 +1120,6 @@ export default class MainBackground {
this.messagingService,
this.logService,
this.configService,
this.fido2Background,
messageListener,
this.accountService,
lockService,
Expand Down
3 changes: 0 additions & 3 deletions apps/browser/src/background/runtime.background.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import {
openTwoFactorAuthPopout,
} from "../auth/popup/utils/auth-popout-window";
import { LockedVaultPendingNotificationsData } from "../autofill/background/abstractions/notification.background";
import { Fido2Background } from "../autofill/fido2/background/abstractions/fido2.background";
import { AutofillService } from "../autofill/services/abstractions/autofill.service";
import { BrowserApi } from "../platform/browser/browser-api";
import { BrowserEnvironmentService } from "../platform/services/browser-environment.service";
Expand All @@ -46,7 +45,6 @@ export default class RuntimeBackground {
private messagingService: MessagingService,
private logService: LogService,
private configService: ConfigService,
private fido2Background: Fido2Background,
private messageListener: MessageListener,
private accountService: AccountService,
private readonly lockService: LockService,
Expand Down Expand Up @@ -365,7 +363,6 @@ export default class RuntimeBackground {

private async checkOnInstalled() {
setTimeout(async () => {
void this.fido2Background.injectFido2ContentScriptsInAllTabs();
void this.autofillService.loadAutofillScriptsOnInstall();

if (this.onInstalledReason != null) {
Expand Down
Loading