From 551d2c2441064a4fb5168b3375fa0ed5459db37c Mon Sep 17 00:00:00 2001 From: Andreas Coroiu Date: Tue, 2 Jan 2024 19:42:16 +0100 Subject: [PATCH] [PM-5302] Refactor Passkey feature enable/disable logic (#7242) * feat: add missing tests for `isFido2FeatureEnabled` * feat: add user logged in check * chore: rewrite with cartesian product * chore: remove test The test was more complex than the actual function, removing. * feat: add domain exclusion * feat: add origin equal vault case * chore: clean up the old code from `content-secript` * feat: return early to avoid making api calls * fix: prettier linting * fix: incorrect logic inversion --------- Co-authored-by: bnagawiecki <107435978+bnagawiecki@users.noreply.github.com> Co-authored-by: SmithThe4th --- .../src/background/runtime.background.ts | 2 +- .../src/vault/fido2/content/content-script.ts | 57 ++-------------- .../fido2/fido2-client.service.abstraction.ts | 4 +- .../fido2/fido2-client.service.spec.ts | 25 ++++++- .../services/fido2/fido2-client.service.ts | 67 +++++++++---------- 5 files changed, 66 insertions(+), 89 deletions(-) diff --git a/apps/browser/src/background/runtime.background.ts b/apps/browser/src/background/runtime.background.ts index 25ada1356be..f5e0c989e0e 100644 --- a/apps/browser/src/background/runtime.background.ts +++ b/apps/browser/src/background/runtime.background.ts @@ -264,7 +264,7 @@ export default class RuntimeBackground { this.abortManager.abort(msg.abortedRequestId); break; case "checkFido2FeatureEnabled": - return await this.main.fido2ClientService.isFido2FeatureEnabled(); + return await this.main.fido2ClientService.isFido2FeatureEnabled(msg.hostname, msg.origin); case "fido2RegisterCredentialRequest": return await this.abortManager.runWithAbortController( msg.requestId, diff --git a/apps/browser/src/vault/fido2/content/content-script.ts b/apps/browser/src/vault/fido2/content/content-script.ts index 54d18f2c8f4..3d64b2cfb64 100644 --- a/apps/browser/src/vault/fido2/content/content-script.ts +++ b/apps/browser/src/vault/fido2/content/content-script.ts @@ -9,46 +9,16 @@ import { Messenger } from "./messaging/messenger"; function isFido2FeatureEnabled(): Promise { return new Promise((resolve) => { chrome.runtime.sendMessage( - { command: "checkFido2FeatureEnabled" }, + { + command: "checkFido2FeatureEnabled", + hostname: window.location.hostname, + origin: window.location.origin, + }, (response: { result?: boolean }) => resolve(response.result), ); }); } -async function getFromLocalStorage(keys: string | string[]): Promise> { - return new Promise((resolve) => { - chrome.storage.local.get(keys, (storage: Record) => resolve(storage)); - }); -} - -async function getActiveUserSettings() { - // TODO: This is code copied from `notification-bar.tsx`. We should refactor this into a shared function. - // Look up the active user id from storage - const activeUserIdKey = "activeUserId"; - let activeUserId: string; - - const activeUserStorageValue = await getFromLocalStorage(activeUserIdKey); - if (activeUserStorageValue[activeUserIdKey]) { - activeUserId = activeUserStorageValue[activeUserIdKey]; - } - - const settingsStorage = await getFromLocalStorage(activeUserId); - - // Look up the user's settings from storage - return settingsStorage?.[activeUserId]?.settings; -} - -async function isDomainExcluded(activeUserSettings: Record) { - const excludedDomains = activeUserSettings?.neverDomains; - return excludedDomains && window.location.hostname in excludedDomains; -} - -async function hasActiveUser() { - const activeUserIdKey = "activeUserId"; - const activeUserStorageValue = await getFromLocalStorage(activeUserIdKey); - return activeUserStorageValue[activeUserIdKey] !== undefined; -} - function isSameOriginWithAncestors() { try { return window.self === window.top; @@ -56,11 +26,6 @@ function isSameOriginWithAncestors() { return false; } } - -async function isLocationBitwardenVault(activeUserSettings: Record) { - return window.location.origin === activeUserSettings.serverConfig.environment.vault; -} - const messenger = Messenger.forDOMCommunication(window); function injectPageScript() { @@ -156,17 +121,7 @@ function initializeFido2ContentScript() { } async function run() { - if (!(await hasActiveUser())) { - return; - } - - const activeUserSettings = await getActiveUserSettings(); - if ( - activeUserSettings == null || - !(await isFido2FeatureEnabled()) || - (await isDomainExcluded(activeUserSettings)) || - (await isLocationBitwardenVault(activeUserSettings)) - ) { + if (!(await isFido2FeatureEnabled())) { return; } diff --git a/libs/common/src/vault/abstractions/fido2/fido2-client.service.abstraction.ts b/libs/common/src/vault/abstractions/fido2/fido2-client.service.abstraction.ts index b806a470c4e..c459c60d07c 100644 --- a/libs/common/src/vault/abstractions/fido2/fido2-client.service.abstraction.ts +++ b/libs/common/src/vault/abstractions/fido2/fido2-client.service.abstraction.ts @@ -14,6 +14,8 @@ export type UserVerification = "discouraged" | "preferred" | "required"; * and for returning the results of the latter operations to the Web Authentication API's callers. */ export abstract class Fido2ClientService { + isFido2FeatureEnabled: (hostname: string, origin: string) => Promise; + /** * Allows WebAuthn Relying Party scripts to request the creation of a new public key credential source. * For more information please see: https://www.w3.org/TR/webauthn-3/#sctn-createCredential @@ -42,8 +44,6 @@ export abstract class Fido2ClientService { tab: chrome.tabs.Tab, abortController?: AbortController, ) => Promise; - - isFido2FeatureEnabled: () => Promise; } /** diff --git a/libs/common/src/vault/services/fido2/fido2-client.service.spec.ts b/libs/common/src/vault/services/fido2/fido2-client.service.spec.ts index 48b566820c6..1aab00752e7 100644 --- a/libs/common/src/vault/services/fido2/fido2-client.service.spec.ts +++ b/libs/common/src/vault/services/fido2/fido2-client.service.spec.ts @@ -1,4 +1,5 @@ import { mock, MockProxy } from "jest-mock-extended"; +import { of } from "rxjs"; import { AuthService } from "../../../auth/abstractions/auth.service"; import { AuthenticationStatus } from "../../../auth/enums/authentication-status"; @@ -23,6 +24,8 @@ import { Fido2Utils } from "./fido2-utils"; import { guidToRawFormat } from "./guid-utils"; const RpId = "bitwarden.com"; +const Origin = "https://bitwarden.com"; +const VaultUrl = "https://vault.bitwarden.com"; describe("FidoAuthenticatorService", () => { let authenticator!: MockProxy; @@ -40,7 +43,9 @@ describe("FidoAuthenticatorService", () => { client = new Fido2ClientService(authenticator, configService, authService, stateService); configService.getFeatureFlag.mockResolvedValue(true); + configService.serverConfig$ = of({ environment: { vault: VaultUrl } } as any); stateService.getEnablePasskeys.mockResolvedValue(true); + authService.getAuthStatus.mockResolvedValue(AuthenticationStatus.Unlocked); tab = { id: 123, windowId: 456 } as chrome.tabs.Tab; }); @@ -249,6 +254,15 @@ describe("FidoAuthenticatorService", () => { const rejects = expect(result).rejects; await rejects.toThrow(FallbackRequestedError); }); + + it("should throw FallbackRequestedError if origin equals the bitwarden vault", async () => { + const params = createParams({ origin: VaultUrl }); + + const result = async () => await client.createCredential(params, tab); + + const rejects = expect(result).rejects; + await rejects.toThrow(FallbackRequestedError); + }); }); function createParams(params: Partial = {}): CreateCredentialParams { @@ -420,6 +434,15 @@ describe("FidoAuthenticatorService", () => { const rejects = expect(result).rejects; await rejects.toThrow(FallbackRequestedError); }); + + it("should throw FallbackRequestedError if origin equals the bitwarden vault", async () => { + const params = createParams({ origin: VaultUrl }); + + const result = async () => await client.assertCredential(params, tab); + + const rejects = expect(result).rejects; + await rejects.toThrow(FallbackRequestedError); + }); }); describe("assert non-discoverable credential", () => { @@ -485,7 +508,7 @@ describe("FidoAuthenticatorService", () => { return { allowedCredentialIds: params.allowedCredentialIds ?? [], challenge: params.challenge ?? Fido2Utils.bufferToString(randomBytes(16)), - origin: params.origin ?? "https://bitwarden.com", + origin: params.origin ?? Origin, rpId: params.rpId ?? RpId, timeout: params.timeout, userVerification: params.userVerification, diff --git a/libs/common/src/vault/services/fido2/fido2-client.service.ts b/libs/common/src/vault/services/fido2/fido2-client.service.ts index 4f446b112dd..283a82ec594 100644 --- a/libs/common/src/vault/services/fido2/fido2-client.service.ts +++ b/libs/common/src/vault/services/fido2/fido2-client.service.ts @@ -1,3 +1,4 @@ +import { firstValueFrom } from "rxjs"; import { parse } from "tldts"; import { AuthService } from "../../../auth/abstractions/auth.service"; @@ -45,12 +46,31 @@ export class Fido2ClientService implements Fido2ClientServiceAbstraction { private logService?: LogService, ) {} - async isFido2FeatureEnabled(): Promise { + async isFido2FeatureEnabled(hostname: string, origin: string): Promise { + const userEnabledPasskeys = await this.stateService.getEnablePasskeys(); + const isUserLoggedIn = + (await this.authService.getAuthStatus()) !== AuthenticationStatus.LoggedOut; + + const neverDomains = await this.stateService.getNeverDomains(); + const isExcludedDomain = neverDomains != null && hostname in neverDomains; + + const serverConfig = await firstValueFrom(this.configService.serverConfig$); + const isOriginEqualBitwardenVault = origin === serverConfig.environment?.vault; + + if ( + !userEnabledPasskeys || + !isUserLoggedIn || + isExcludedDomain || + isOriginEqualBitwardenVault + ) { + return false; + } + const featureFlagEnabled = await this.configService.getFeatureFlag( FeatureFlag.Fido2VaultCredentials, ); - const userEnabledPasskeys = await this.stateService.getEnablePasskeys(); - return featureFlagEnabled && userEnabledPasskeys; + + return featureFlagEnabled; } async createCredential( @@ -58,20 +78,17 @@ export class Fido2ClientService implements Fido2ClientServiceAbstraction { tab: chrome.tabs.Tab, abortController = new AbortController(), ): Promise { - const enableFido2VaultCredentials = await this.isFido2FeatureEnabled(); + const parsedOrigin = parse(params.origin, { allowPrivateDomains: true }); + const enableFido2VaultCredentials = await this.isFido2FeatureEnabled( + parsedOrigin.hostname, + params.origin, + ); if (!enableFido2VaultCredentials) { this.logService?.warning(`[Fido2Client] Fido2VaultCredential is not enabled`); throw new FallbackRequestedError(); } - const authStatus = await this.authService.getAuthStatus(); - - if (authStatus === AuthenticationStatus.LoggedOut) { - this.logService?.warning(`[Fido2Client] Fido2VaultCredential is not enabled`); - throw new FallbackRequestedError(); - } - if (!params.sameOriginWithAncestors) { this.logService?.warning( `[Fido2Client] Invalid 'sameOriginWithAncestors' value: ${params.sameOriginWithAncestors}`, @@ -87,15 +104,7 @@ export class Fido2ClientService implements Fido2ClientServiceAbstraction { throw new TypeError("Invalid 'user.id' length"); } - const parsedOrigin = parse(params.origin, { allowPrivateDomains: true }); params.rp.id = params.rp.id ?? parsedOrigin.hostname; - - const neverDomains = await this.stateService.getNeverDomains(); - if (neverDomains != null && parsedOrigin.hostname in neverDomains) { - this.logService?.warning(`[Fido2Client] Excluded domain`); - throw new FallbackRequestedError(); - } - if (parsedOrigin.hostname == undefined || !params.origin.startsWith("https://")) { this.logService?.warning(`[Fido2Client] Invalid https origin: ${params.origin}`); throw new DOMException("'origin' is not a valid https origin", "SecurityError"); @@ -210,29 +219,19 @@ export class Fido2ClientService implements Fido2ClientServiceAbstraction { tab: chrome.tabs.Tab, abortController = new AbortController(), ): Promise { - const enableFido2VaultCredentials = await this.isFido2FeatureEnabled(); + const parsedOrigin = parse(params.origin, { allowPrivateDomains: true }); + const enableFido2VaultCredentials = await this.isFido2FeatureEnabled( + parsedOrigin.hostname, + params.origin, + ); if (!enableFido2VaultCredentials) { this.logService?.warning(`[Fido2Client] Fido2VaultCredential is not enabled`); throw new FallbackRequestedError(); } - const authStatus = await this.authService.getAuthStatus(); - - if (authStatus === AuthenticationStatus.LoggedOut) { - this.logService?.warning(`[Fido2Client] Fido2VaultCredential is not enabled`); - throw new FallbackRequestedError(); - } - - const parsedOrigin = parse(params.origin, { allowPrivateDomains: true }); params.rpId = params.rpId ?? parsedOrigin.hostname; - const neverDomains = await this.stateService.getNeverDomains(); - if (neverDomains != null && parsedOrigin.hostname in neverDomains) { - this.logService?.warning(`[Fido2Client] Excluded domain`); - throw new FallbackRequestedError(); - } - if (parsedOrigin.hostname == undefined || !params.origin.startsWith("https://")) { this.logService?.warning(`[Fido2Client] Invalid https origin: ${params.origin}`); throw new DOMException("'origin' is not a valid https origin", "SecurityError");