From 7985487d5b60495fc5e4bd7270394c824aa2a50b Mon Sep 17 00:00:00 2001 From: Bernd Schoolmann Date: Tue, 9 Sep 2025 23:19:00 +0900 Subject: [PATCH] [PM-25458] Add error handling stubs & logging for critical decrypt paths (#16284) * Add error handling stubs for critical decrypt paths * Fix collection name decrypt * Update docs * address feedback --------- Co-authored-by: Jake Fink --- .../models/collection-admin.view.ts | 27 +++++++++++++++++-- .../collections/models/collection.view.ts | 10 ++++++- .../common/src/auth/services/token.service.ts | 16 +++++++---- .../crypto/abstractions/encrypt.service.ts | 7 ++++- .../crypto/models/enc-string.ts | 8 ++++-- libs/common/src/vault/models/domain/folder.ts | 9 ++++++- libs/key-management/src/key.service.ts | 12 ++++++--- 7 files changed, 73 insertions(+), 16 deletions(-) diff --git a/libs/admin-console/src/common/collections/models/collection-admin.view.ts b/libs/admin-console/src/common/collections/models/collection-admin.view.ts index fcd2be17b8d..d5effaad3aa 100644 --- a/libs/admin-console/src/common/collections/models/collection-admin.view.ts +++ b/libs/admin-console/src/common/collections/models/collection-admin.view.ts @@ -118,7 +118,17 @@ export class CollectionAdminView extends CollectionView { orgKey: OrgKey, ): Promise { const view = new CollectionAdminView({ ...collection }); - view.name = await encryptService.decryptString(new EncString(view.name), orgKey); + try { + view.name = await encryptService.decryptString(new EncString(view.name), orgKey); + } catch (e) { + // Note: This should be replaced by the owning team with appropriate, domain-specific behavior. + // eslint-disable-next-line no-console + console.error( + "[CollectionAdminView/fromCollectionAccessDetails] Error decrypting collection name", + e, + ); + throw e; + } view.assigned = collection.assigned; view.readOnly = collection.readOnly; view.hidePasswords = collection.hidePasswords; @@ -144,9 +154,22 @@ export class CollectionAdminView extends CollectionView { encryptService: EncryptService, orgKey: OrgKey, ): Promise { + let collectionName: string; + try { + collectionName = await encryptService.decryptString(new EncString(collection.name), orgKey); + } catch (e) { + // Note: This should be updated by the owning team with appropriate, domain specific behavior + // eslint-disable-next-line no-console + console.error( + "[CollectionAdminView/fromCollectionResponse] Failed to decrypt the collection name", + e, + ); + throw e; + } + const collectionAdminView = new CollectionAdminView({ id: collection.id, - name: await encryptService.decryptString(new EncString(collection.name), orgKey), + name: collectionName, organizationId: collection.organizationId, }); diff --git a/libs/admin-console/src/common/collections/models/collection.view.ts b/libs/admin-console/src/common/collections/models/collection.view.ts index d3e15806c86..2991e8bb171 100644 --- a/libs/admin-console/src/common/collections/models/collection.view.ts +++ b/libs/admin-console/src/common/collections/models/collection.view.ts @@ -144,7 +144,15 @@ export class CollectionView implements View, ITreeNodeObject { ): Promise { const view = new CollectionView({ ...collection }); - view.name = await encryptService.decryptString(new EncString(collection.name), orgKey); + try { + view.name = await encryptService.decryptString(new EncString(collection.name), orgKey); + } catch (e) { + // Note: This should be replaced by the owning team with appropriate, domain-specific behavior. + // eslint-disable-next-line no-console + console.error("[CollectionView] Error decrypting collection name", e); + throw e; + } + view.externalId = collection.externalId; view.type = collection.type; view.assigned = collection.assigned; diff --git a/libs/common/src/auth/services/token.service.ts b/libs/common/src/auth/services/token.service.ts index 0721927bd13..c02bc85f124 100644 --- a/libs/common/src/auth/services/token.service.ts +++ b/libs/common/src/auth/services/token.service.ts @@ -314,12 +314,18 @@ export class TokenService implements TokenServiceAbstraction { ); } - const decryptedAccessToken = await this.encryptService.decryptString( - encryptedAccessToken, - accessTokenKey, - ); + try { + const decryptedAccessToken = await this.encryptService.decryptString( + encryptedAccessToken, + accessTokenKey, + ); + return decryptedAccessToken; + } catch (e) { + // Note: This should be replaced by the owning team with appropriate, domain-specific behavior. - return decryptedAccessToken; + this.logService.error("[TokenService] Error decrypting access token", e); + throw e; + } } /** diff --git a/libs/common/src/key-management/crypto/abstractions/encrypt.service.ts b/libs/common/src/key-management/crypto/abstractions/encrypt.service.ts index ed43e25bd0a..87af3852116 100644 --- a/libs/common/src/key-management/crypto/abstractions/encrypt.service.ts +++ b/libs/common/src/key-management/crypto/abstractions/encrypt.service.ts @@ -28,6 +28,9 @@ export abstract class EncryptService { /** * Decrypts an EncString to a string + * @throws IMPORTANT: This throws if decryption fails. If decryption failures are expected to happen, + * the callsite should log where the failure occurred, and handle it by domain specifc logic (e.g. show a UI error). + * * @param encString - The EncString containing the encrypted string. * @param key - The key to decrypt the value with * @returns The decrypted string @@ -36,10 +39,12 @@ export abstract class EncryptService { abstract decryptString(encString: EncString, key: SymmetricCryptoKey): Promise; /** * Decrypts an EncString to a Uint8Array + * @throws IMPORTANT: This throws if decryption fails. If decryption failures are expected to happen, + * the callsite should log where the failure occurred, and handle it by domain specifc logic (e.g. show a UI error). + * * @param encString - The EncString containing the encrypted bytes. * @param key - The key to decrypt the value with * @returns The decrypted bytes as a Uint8Array - * @throws Error if decryption fails * @deprecated Bytes are not the right abstraction to encrypt in. Use e.g. key wrapping or file encryption instead */ abstract decryptBytes(encString: EncString, key: SymmetricCryptoKey): Promise; diff --git a/libs/common/src/key-management/crypto/models/enc-string.ts b/libs/common/src/key-management/crypto/models/enc-string.ts index f39741f9faf..a737771f799 100644 --- a/libs/common/src/key-management/crypto/models/enc-string.ts +++ b/libs/common/src/key-management/crypto/models/enc-string.ts @@ -180,9 +180,13 @@ export class EncString { const encryptService = Utils.getContainerService().getEncryptService(); this.decryptedValue = await encryptService.decryptString(this, key); - // FIXME: Remove when updating file. Eslint update - // eslint-disable-next-line @typescript-eslint/no-unused-vars } catch (e) { + // eslint-disable-next-line no-console + console.error( + "[EncString Generic Decrypt] failed to decrypt encstring. Context: " + + (context ?? "No context"), + e, + ); this.decryptedValue = DECRYPT_ERROR; } return this.decryptedValue; diff --git a/libs/common/src/vault/models/domain/folder.ts b/libs/common/src/vault/models/domain/folder.ts index 01f5b9599bd..50c67eee01f 100644 --- a/libs/common/src/vault/models/domain/folder.ts +++ b/libs/common/src/vault/models/domain/folder.ts @@ -50,7 +50,14 @@ export class Folder extends Domain { const folderView = new FolderView(); folderView.id = this.id; folderView.revisionDate = this.revisionDate; - folderView.name = await encryptService.decryptString(this.name, key); + try { + folderView.name = await encryptService.decryptString(this.name, key); + } catch (e) { + // Note: This should be replaced by the owning team with appropriate, domain-specific behavior. + // eslint-disable-next-line no-console + console.error("[Folder] Error decrypting folder", e); + throw e; + } return folderView; } diff --git a/libs/key-management/src/key.service.ts b/libs/key-management/src/key.service.ts index ab41e662799..cdf7d594f28 100644 --- a/libs/key-management/src/key.service.ts +++ b/libs/key-management/src/key.service.ts @@ -827,10 +827,14 @@ export class DefaultKeyService implements KeyServiceAbstraction { } return this.stateProvider.getUser(userId, USER_ENCRYPTED_PRIVATE_KEY).state$.pipe( - switchMap( - async (encryptedPrivateKey) => - await this.decryptPrivateKey(encryptedPrivateKey, userKey), - ), + switchMap(async (encryptedPrivateKey) => { + try { + return await this.decryptPrivateKey(encryptedPrivateKey, userKey); + } catch (e) { + this.logService.error("Failed to decrypt private key for user ", userId, e); + throw e; + } + }), // Combine outerscope info with user private key map((userPrivateKey) => ({ userKey,