From 122c3c780999b63c76515c35c4dad17666d145d0 Mon Sep 17 00:00:00 2001 From: Bernd Schoolmann Date: Fri, 25 Oct 2024 15:22:30 +0200 Subject: [PATCH 1/4] Add context to logs for decryption failures (#11684) * Add logging to decryption routines * Fix case of uknown encryption type * Add decryption context to log where failures occur * Update log message * Fix linting * Add more context logs * Add more fine grained logging * Update log message * Fix tests --- ...ocal-backed-session-storage.service.spec.ts | 16 ++++++++++++---- .../local-backed-session-storage.service.ts | 6 +++++- apps/cli/src/commands/get.command.ts | 1 + .../platform/abstractions/encrypt.service.ts | 6 +++++- .../platform/models/domain/enc-string.spec.ts | 16 ++++++++++++---- .../src/platform/models/domain/enc-string.ts | 15 +++++++++++++-- .../encrypt.service.implementation.ts | 18 +++++++++++++----- 7 files changed, 61 insertions(+), 17 deletions(-) diff --git a/apps/browser/src/platform/services/local-backed-session-storage.service.spec.ts b/apps/browser/src/platform/services/local-backed-session-storage.service.spec.ts index 8d43c8f2fe8..949ecebde8a 100644 --- a/apps/browser/src/platform/services/local-backed-session-storage.service.spec.ts +++ b/apps/browser/src/platform/services/local-backed-session-storage.service.spec.ts @@ -48,8 +48,12 @@ describe("LocalBackedSessionStorage", () => { localStorage.internalStore["session_test"] = encrypted.encryptedString; encryptService.decryptToUtf8.mockResolvedValue(JSON.stringify("decrypted")); const result = await sut.get("test"); - expect(encryptService.decryptToUtf8).toHaveBeenCalledWith(encrypted, sessionKey); - expect(result).toEqual("decrypted"); + expect(encryptService.decryptToUtf8).toHaveBeenCalledWith( + encrypted, + sessionKey, + "browser-session-key", + ), + expect(result).toEqual("decrypted"); }); it("caches the decrypted value when one is stored in local storage", async () => { @@ -65,8 +69,12 @@ describe("LocalBackedSessionStorage", () => { localStorage.internalStore["session_test"] = encrypted.encryptedString; encryptService.decryptToUtf8.mockResolvedValue(JSON.stringify("decrypted")); const result = await sut.get("test"); - expect(encryptService.decryptToUtf8).toHaveBeenCalledWith(encrypted, sessionKey); - expect(result).toEqual("decrypted"); + expect(encryptService.decryptToUtf8).toHaveBeenCalledWith( + encrypted, + sessionKey, + "browser-session-key", + ), + expect(result).toEqual("decrypted"); }); it("caches the decrypted value when one is stored in local storage", async () => { diff --git a/apps/browser/src/platform/services/local-backed-session-storage.service.ts b/apps/browser/src/platform/services/local-backed-session-storage.service.ts index 28abb78b19c..e4256d9c0cf 100644 --- a/apps/browser/src/platform/services/local-backed-session-storage.service.ts +++ b/apps/browser/src/platform/services/local-backed-session-storage.service.ts @@ -117,7 +117,11 @@ export class LocalBackedSessionStorageService return null; } - const valueJson = await this.encryptService.decryptToUtf8(new EncString(local), encKey); + const valueJson = await this.encryptService.decryptToUtf8( + new EncString(local), + encKey, + "browser-session-key", + ); if (valueJson == null) { // error with decryption, value is lost, delete state and start over await this.localStorage.remove(this.sessionStorageKey(key)); diff --git a/apps/cli/src/commands/get.command.ts b/apps/cli/src/commands/get.command.ts index 0bf1487b2e2..fc014534e03 100644 --- a/apps/cli/src/commands/get.command.ts +++ b/apps/cli/src/commands/get.command.ts @@ -455,6 +455,7 @@ export class GetCommand extends DownloadCommand { decCollection.name = await this.encryptService.decryptToUtf8( new EncString(response.name), orgKey, + `orgkey-${options.organizationId}`, ); const groups = response.groups == null diff --git a/libs/common/src/platform/abstractions/encrypt.service.ts b/libs/common/src/platform/abstractions/encrypt.service.ts index c70042e4186..5b28b98803b 100644 --- a/libs/common/src/platform/abstractions/encrypt.service.ts +++ b/libs/common/src/platform/abstractions/encrypt.service.ts @@ -8,7 +8,11 @@ import { SymmetricCryptoKey } from "../models/domain/symmetric-crypto-key"; export abstract class EncryptService { abstract encrypt(plainValue: string | Uint8Array, key: SymmetricCryptoKey): Promise; abstract encryptToBytes(plainValue: Uint8Array, key: SymmetricCryptoKey): Promise; - abstract decryptToUtf8(encString: EncString, key: SymmetricCryptoKey): Promise; + abstract decryptToUtf8( + encString: EncString, + key: SymmetricCryptoKey, + decryptContext?: string, + ): Promise; abstract decryptToBytes(encThing: Encrypted, key: SymmetricCryptoKey): Promise; abstract rsaEncrypt(data: Uint8Array, publicKey: Uint8Array): Promise; abstract rsaDecrypt(data: EncString, privateKey: Uint8Array): Promise; diff --git a/libs/common/src/platform/models/domain/enc-string.spec.ts b/libs/common/src/platform/models/domain/enc-string.spec.ts index 462a977ff8c..85108a9609b 100644 --- a/libs/common/src/platform/models/domain/enc-string.spec.ts +++ b/libs/common/src/platform/models/domain/enc-string.spec.ts @@ -149,7 +149,7 @@ describe("EncString", () => { const key = new SymmetricCryptoKey(makeStaticByteArray(32)); await encString.decryptWithKey(key, encryptService); - expect(encryptService.decryptToUtf8).toHaveBeenCalledWith(encString, key); + expect(encryptService.decryptToUtf8).toHaveBeenCalledWith(encString, key, "domain-withkey"); }); it("fails to decrypt when key is null", async () => { @@ -351,7 +351,7 @@ describe("EncString", () => { await encString.decrypt(null, key); expect(keyService.getUserKeyWithLegacySupport).not.toHaveBeenCalled(); - expect(encryptService.decryptToUtf8).toHaveBeenCalledWith(encString, key); + expect(encryptService.decryptToUtf8).toHaveBeenCalledWith(encString, key, "provided-key"); }); it("gets an organization key if required", async () => { @@ -362,7 +362,11 @@ describe("EncString", () => { await encString.decrypt("orgId", null); expect(keyService.getOrgKey).toHaveBeenCalledWith("orgId"); - expect(encryptService.decryptToUtf8).toHaveBeenCalledWith(encString, orgKey); + expect(encryptService.decryptToUtf8).toHaveBeenCalledWith( + encString, + orgKey, + "domain-orgkey-orgId", + ); }); it("gets the user's decryption key if required", async () => { @@ -373,7 +377,11 @@ describe("EncString", () => { await encString.decrypt(null, null); expect(keyService.getUserKeyWithLegacySupport).toHaveBeenCalledWith(); - expect(encryptService.decryptToUtf8).toHaveBeenCalledWith(encString, userKey); + expect(encryptService.decryptToUtf8).toHaveBeenCalledWith( + encString, + userKey, + "domain-withlegacysupport-masterkey", + ); }); }); diff --git a/libs/common/src/platform/models/domain/enc-string.ts b/libs/common/src/platform/models/domain/enc-string.ts index 40f36306bf1..6f01f46439c 100644 --- a/libs/common/src/platform/models/domain/enc-string.ts +++ b/libs/common/src/platform/models/domain/enc-string.ts @@ -159,16 +159,27 @@ export class EncString implements Encrypted { return this.decryptedValue; } + let keyContext = "provided-key"; try { if (key == null) { key = await this.getKeyForDecryption(orgId); + keyContext = orgId == null ? `domain-orgkey-${orgId}` : "domain-userkey|masterkey"; + if (orgId != null) { + keyContext = `domain-orgkey-${orgId}`; + } else { + const cryptoService = Utils.getContainerService().getKeyService(); + keyContext = + (await cryptoService.getUserKey()) == null + ? "domain-withlegacysupport-masterkey" + : "domain-withlegacysupport-userkey"; + } } if (key == null) { throw new Error("No key to decrypt EncString with orgId " + orgId); } const encryptService = Utils.getContainerService().getEncryptService(); - this.decryptedValue = await encryptService.decryptToUtf8(this, key); + this.decryptedValue = await encryptService.decryptToUtf8(this, key, keyContext); } catch (e) { this.decryptedValue = DECRYPT_ERROR; } @@ -181,7 +192,7 @@ export class EncString implements Encrypted { throw new Error("No key to decrypt EncString"); } - this.decryptedValue = await encryptService.decryptToUtf8(this, key); + this.decryptedValue = await encryptService.decryptToUtf8(this, key, "domain-withkey"); } catch (e) { this.decryptedValue = DECRYPT_ERROR; } diff --git a/libs/common/src/platform/services/cryptography/encrypt.service.implementation.ts b/libs/common/src/platform/services/cryptography/encrypt.service.implementation.ts index 8c42c724b24..137d67ca0f0 100644 --- a/libs/common/src/platform/services/cryptography/encrypt.service.implementation.ts +++ b/libs/common/src/platform/services/cryptography/encrypt.service.implementation.ts @@ -63,7 +63,11 @@ export class EncryptServiceImplementation implements EncryptService { return new EncArrayBuffer(encBytes); } - async decryptToUtf8(encString: EncString, key: SymmetricCryptoKey): Promise { + async decryptToUtf8( + encString: EncString, + key: SymmetricCryptoKey, + decryptContext: string = "no context", + ): Promise { if (key == null) { throw new Error("No key provided for decryption."); } @@ -75,8 +79,9 @@ export class EncryptServiceImplementation implements EncryptService { this.logService.error( "[Encrypt service] Key has mac key but payload is missing mac bytes. Key type " + encryptionTypeName(key.encType) + - " Payload type " + + "Payload type " + encryptionTypeName(encString.encryptionType), + "Decrypt context: " + decryptContext, ); return null; } @@ -85,8 +90,9 @@ export class EncryptServiceImplementation implements EncryptService { this.logService.error( "[Encrypt service] Key encryption type does not match payload encryption type. Key type " + encryptionTypeName(key.encType) + - " Payload type " + + "Payload type " + encryptionTypeName(encString.encryptionType), + "Decrypt context: " + decryptContext, ); return null; } @@ -108,8 +114,10 @@ export class EncryptServiceImplementation implements EncryptService { this.logMacFailed( "[Encrypt service] MAC comparison failed. Key or payload has changed. Key type " + encryptionTypeName(key.encType) + - " Payload type " + - encryptionTypeName(encString.encryptionType), + "Payload type " + + encryptionTypeName(encString.encryptionType) + + " Decrypt context: " + + decryptContext, ); return null; } From 3b82a8241696e0d24fd3349385219436dc8478cb Mon Sep 17 00:00:00 2001 From: Daniel James Smith <2670567+djsmith85@users.noreply.github.com> Date: Fri, 25 Oct 2024 17:04:39 +0200 Subject: [PATCH 2/4] [PM-13658] Update copy of password history to generator history (#11710) * Update copy from password history to generator history * Update copy on empty state --------- Co-authored-by: Daniel James Smith --- apps/browser/src/_locales/en/messages.json | 11 +++++++---- .../credential-generator-history.component.html | 2 +- .../generator/credential-generator.component.html | 2 +- .../src/empty-credential-history.component.html | 4 ++-- 4 files changed, 11 insertions(+), 8 deletions(-) diff --git a/apps/browser/src/_locales/en/messages.json b/apps/browser/src/_locales/en/messages.json index b8c263652ed..0c6dfe9e546 100644 --- a/apps/browser/src/_locales/en/messages.json +++ b/apps/browser/src/_locales/en/messages.json @@ -1794,6 +1794,9 @@ "passwordHistory": { "message": "Password history" }, + "generatorHistory": { + "message": "Generator history" + }, "back": { "message": "Back" }, @@ -1910,11 +1913,11 @@ "clearHistory": { "message": "Clear history" }, - "noPasswordsToShow": { - "message": "No passwords to show" + "nothingToShow": { + "message": "Nothing to show" }, - "noRecentlyGeneratedPassword": { - "message": "You haven't generated a password recently" + "nothingGeneratedRecently": { + "message": "You haven't generated anything recently" }, "remove": { "message": "Remove" diff --git a/apps/browser/src/tools/popup/generator/credential-generator-history.component.html b/apps/browser/src/tools/popup/generator/credential-generator-history.component.html index 48f0479d8d6..06b503534b5 100644 --- a/apps/browser/src/tools/popup/generator/credential-generator-history.component.html +++ b/apps/browser/src/tools/popup/generator/credential-generator-history.component.html @@ -1,5 +1,5 @@ - + diff --git a/apps/browser/src/tools/popup/generator/credential-generator.component.html b/apps/browser/src/tools/popup/generator/credential-generator.component.html index fd278592e69..aed02b46178 100644 --- a/apps/browser/src/tools/popup/generator/credential-generator.component.html +++ b/apps/browser/src/tools/popup/generator/credential-generator.component.html @@ -8,7 +8,7 @@ - {{ "passwordHistory" | i18n }} + {{ "generatorHistory" | i18n }} diff --git a/libs/tools/generator/components/src/empty-credential-history.component.html b/libs/tools/generator/components/src/empty-credential-history.component.html index ee28dc58958..f7f35fe9362 100644 --- a/libs/tools/generator/components/src/empty-credential-history.component.html +++ b/libs/tools/generator/components/src/empty-credential-history.component.html @@ -1,7 +1,7 @@
-

{{ "noPasswordsToShow" | i18n }}

-
{{ "noRecentlyGeneratedPassword" | i18n }}
+

{{ "nothingToShow" | i18n }}

+
{{ "nothingGeneratedRecently" | i18n }}
From fa537638bfe7b9ece1a29522401d90ccc7afc89c Mon Sep 17 00:00:00 2001 From: Jared Snider <116684653+JaredSnider-Bitwarden@users.noreply.github.com> Date: Fri, 25 Oct 2024 11:29:48 -0400 Subject: [PATCH 3/4] Auth/PM-13945 - Extension - Fix TDE User with MP can't navigate to Lock Screen (#11700) * PM-13945 - Extension Refresh redirects should persist query params as we use query params to execute guard logic (e.g., lockGuard). The loss of the from: login-initiated query param prevented navigation to the lock screen. * PM-13945 - Test updated --- .../unauth-ui-refresh-redirect.spec.ts | 28 +++++++++++++------ .../functions/unauth-ui-refresh-redirect.ts | 7 ++++- .../src/utils/extension-refresh-redirect.ts | 8 +++++- 3 files changed, 32 insertions(+), 11 deletions(-) diff --git a/libs/angular/src/auth/functions/unauth-ui-refresh-redirect.spec.ts b/libs/angular/src/auth/functions/unauth-ui-refresh-redirect.spec.ts index 5fc904a5b93..6a19f1ace7b 100644 --- a/libs/angular/src/auth/functions/unauth-ui-refresh-redirect.spec.ts +++ b/libs/angular/src/auth/functions/unauth-ui-refresh-redirect.spec.ts @@ -1,5 +1,5 @@ import { TestBed } from "@angular/core/testing"; -import { Router, UrlTree } from "@angular/router"; +import { Navigation, Router, UrlTree } from "@angular/router"; import { mock, MockProxy } from "jest-mock-extended"; import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum"; @@ -37,19 +37,29 @@ describe("unauthUiRefreshRedirect", () => { expect(router.parseUrl).not.toHaveBeenCalled(); }); - it("returns UrlTree when UnauthenticatedExtensionUIRefresh flag is enabled", async () => { - const mockUrlTree = mock(); + it("returns UrlTree when UnauthenticatedExtensionUIRefresh flag is enabled and preserves query params", async () => { configService.getFeatureFlag.mockResolvedValue(true); - router.parseUrl.mockReturnValue(mockUrlTree); - const result = await TestBed.runInInjectionContext(() => - unauthUiRefreshRedirect("/redirect")(), - ); + const queryParams = { test: "test" }; + + const navigation: Navigation = { + extras: { + queryParams: queryParams, + }, + id: 0, + initialUrl: new UrlTree(), + extractedUrl: new UrlTree(), + trigger: "imperative", + previousNavigation: undefined, + }; + + router.getCurrentNavigation.mockReturnValue(navigation); + + await TestBed.runInInjectionContext(() => unauthUiRefreshRedirect("/redirect")()); - expect(result).toBe(mockUrlTree); expect(configService.getFeatureFlag).toHaveBeenCalledWith( FeatureFlag.UnauthenticatedExtensionUIRefresh, ); - expect(router.parseUrl).toHaveBeenCalledWith("/redirect"); + expect(router.createUrlTree).toHaveBeenCalledWith(["/redirect"], { queryParams }); }); }); diff --git a/libs/angular/src/auth/functions/unauth-ui-refresh-redirect.ts b/libs/angular/src/auth/functions/unauth-ui-refresh-redirect.ts index 22ed23273b0..a54bad11479 100644 --- a/libs/angular/src/auth/functions/unauth-ui-refresh-redirect.ts +++ b/libs/angular/src/auth/functions/unauth-ui-refresh-redirect.ts @@ -16,7 +16,12 @@ export function unauthUiRefreshRedirect(redirectUrl: string): () => Promise Promise { const configService = inject(ConfigService); const router = inject(Router); + const shouldRedirect = await configService.getFeatureFlag(FeatureFlag.ExtensionRefresh); if (shouldRedirect) { - return router.parseUrl(redirectUrl); + const currentNavigation = router.getCurrentNavigation(); + const queryParams = currentNavigation?.extras?.queryParams || {}; + + // Preserve query params when redirecting as it is likely that the refreshed component + // will be consuming the same query params. + return router.createUrlTree([redirectUrl], { queryParams }); } else { return true; } From ab98bef28f413ac2488452dd35033537d586ba67 Mon Sep 17 00:00:00 2001 From: Daniel James Smith <2670567+djsmith85@users.noreply.github.com> Date: Fri, 25 Oct 2024 17:44:12 +0200 Subject: [PATCH 4/4] Add warning dialog when clearing generator history (#11711) Co-authored-by: Daniel James Smith --- apps/browser/src/_locales/en/messages.json | 6 ++++++ .../credential-generator-history.component.ts | 15 +++++++++++++-- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/apps/browser/src/_locales/en/messages.json b/apps/browser/src/_locales/en/messages.json index 0c6dfe9e546..8e52b1ba006 100644 --- a/apps/browser/src/_locales/en/messages.json +++ b/apps/browser/src/_locales/en/messages.json @@ -1797,6 +1797,12 @@ "generatorHistory": { "message": "Generator history" }, + "clearGeneratorHistoryTitle": { + "message": "Clear generator history" + }, + "cleargGeneratorHistoryDescription": { + "message": "If you continue, all entries will be permanently deleted from generator's history. Are you sure you want to continue?" + }, "back": { "message": "Back" }, diff --git a/apps/browser/src/tools/popup/generator/credential-generator-history.component.ts b/apps/browser/src/tools/popup/generator/credential-generator-history.component.ts index 0de71cf5b98..dd65bb96878 100644 --- a/apps/browser/src/tools/popup/generator/credential-generator-history.component.ts +++ b/apps/browser/src/tools/popup/generator/credential-generator-history.component.ts @@ -6,7 +6,7 @@ import { BehaviorSubject, distinctUntilChanged, firstValueFrom, map, switchMap } import { JslibModule } from "@bitwarden/angular/jslib.module"; import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { UserId } from "@bitwarden/common/types/guid"; -import { ButtonModule, ContainerComponent } from "@bitwarden/components"; +import { ButtonModule, ContainerComponent, DialogService } from "@bitwarden/components"; import { CredentialGeneratorHistoryComponent as CredentialGeneratorHistoryToolsComponent, EmptyCredentialHistoryComponent, @@ -42,6 +42,7 @@ export class CredentialGeneratorHistoryComponent { constructor( private accountService: AccountService, private history: GeneratorHistoryService, + private dialogService: DialogService, ) { this.accountService.activeAccount$ .pipe( @@ -61,6 +62,16 @@ export class CredentialGeneratorHistoryComponent { } clear = async () => { - await this.history.clear(await firstValueFrom(this.userId$)); + const confirmed = await this.dialogService.openSimpleDialog({ + title: { key: "clearGeneratorHistoryTitle" }, + content: { key: "cleargGeneratorHistoryDescription" }, + type: "warning", + acceptButtonText: { key: "clearHistory" }, + cancelButtonText: { key: "cancel" }, + }); + + if (confirmed) { + await this.history.clear(await firstValueFrom(this.userId$)); + } }; }