mirror of
https://github.com/bitwarden/browser
synced 2025-12-16 08:13:42 +00:00
[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 <jfink@bitwarden.com>
This commit is contained in:
@@ -118,7 +118,17 @@ export class CollectionAdminView extends CollectionView {
|
|||||||
orgKey: OrgKey,
|
orgKey: OrgKey,
|
||||||
): Promise<CollectionAdminView> {
|
): Promise<CollectionAdminView> {
|
||||||
const view = new CollectionAdminView({ ...collection });
|
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.assigned = collection.assigned;
|
||||||
view.readOnly = collection.readOnly;
|
view.readOnly = collection.readOnly;
|
||||||
view.hidePasswords = collection.hidePasswords;
|
view.hidePasswords = collection.hidePasswords;
|
||||||
@@ -144,9 +154,22 @@ export class CollectionAdminView extends CollectionView {
|
|||||||
encryptService: EncryptService,
|
encryptService: EncryptService,
|
||||||
orgKey: OrgKey,
|
orgKey: OrgKey,
|
||||||
): Promise<CollectionAdminView> {
|
): Promise<CollectionAdminView> {
|
||||||
|
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({
|
const collectionAdminView = new CollectionAdminView({
|
||||||
id: collection.id,
|
id: collection.id,
|
||||||
name: await encryptService.decryptString(new EncString(collection.name), orgKey),
|
name: collectionName,
|
||||||
organizationId: collection.organizationId,
|
organizationId: collection.organizationId,
|
||||||
});
|
});
|
||||||
|
|
||||||
|
|||||||
@@ -144,7 +144,15 @@ export class CollectionView implements View, ITreeNodeObject {
|
|||||||
): Promise<CollectionView> {
|
): Promise<CollectionView> {
|
||||||
const view = new CollectionView({ ...collection });
|
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.externalId = collection.externalId;
|
||||||
view.type = collection.type;
|
view.type = collection.type;
|
||||||
view.assigned = collection.assigned;
|
view.assigned = collection.assigned;
|
||||||
|
|||||||
@@ -314,12 +314,18 @@ export class TokenService implements TokenServiceAbstraction {
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
const decryptedAccessToken = await this.encryptService.decryptString(
|
try {
|
||||||
encryptedAccessToken,
|
const decryptedAccessToken = await this.encryptService.decryptString(
|
||||||
accessTokenKey,
|
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;
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
|
|||||||
@@ -28,6 +28,9 @@ export abstract class EncryptService {
|
|||||||
|
|
||||||
/**
|
/**
|
||||||
* Decrypts an EncString to a string
|
* 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 encString - The EncString containing the encrypted string.
|
||||||
* @param key - The key to decrypt the value with
|
* @param key - The key to decrypt the value with
|
||||||
* @returns The decrypted string
|
* @returns The decrypted string
|
||||||
@@ -36,10 +39,12 @@ export abstract class EncryptService {
|
|||||||
abstract decryptString(encString: EncString, key: SymmetricCryptoKey): Promise<string>;
|
abstract decryptString(encString: EncString, key: SymmetricCryptoKey): Promise<string>;
|
||||||
/**
|
/**
|
||||||
* Decrypts an EncString to a Uint8Array
|
* 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 encString - The EncString containing the encrypted bytes.
|
||||||
* @param key - The key to decrypt the value with
|
* @param key - The key to decrypt the value with
|
||||||
* @returns The decrypted bytes as a Uint8Array
|
* @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
|
* @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<Uint8Array>;
|
abstract decryptBytes(encString: EncString, key: SymmetricCryptoKey): Promise<Uint8Array>;
|
||||||
|
|||||||
@@ -180,9 +180,13 @@ export class EncString {
|
|||||||
|
|
||||||
const encryptService = Utils.getContainerService().getEncryptService();
|
const encryptService = Utils.getContainerService().getEncryptService();
|
||||||
this.decryptedValue = await encryptService.decryptString(this, key);
|
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) {
|
} 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;
|
this.decryptedValue = DECRYPT_ERROR;
|
||||||
}
|
}
|
||||||
return this.decryptedValue;
|
return this.decryptedValue;
|
||||||
|
|||||||
@@ -50,7 +50,14 @@ export class Folder extends Domain {
|
|||||||
const folderView = new FolderView();
|
const folderView = new FolderView();
|
||||||
folderView.id = this.id;
|
folderView.id = this.id;
|
||||||
folderView.revisionDate = this.revisionDate;
|
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;
|
return folderView;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -827,10 +827,14 @@ export class DefaultKeyService implements KeyServiceAbstraction {
|
|||||||
}
|
}
|
||||||
|
|
||||||
return this.stateProvider.getUser(userId, USER_ENCRYPTED_PRIVATE_KEY).state$.pipe(
|
return this.stateProvider.getUser(userId, USER_ENCRYPTED_PRIVATE_KEY).state$.pipe(
|
||||||
switchMap(
|
switchMap(async (encryptedPrivateKey) => {
|
||||||
async (encryptedPrivateKey) =>
|
try {
|
||||||
await this.decryptPrivateKey(encryptedPrivateKey, userKey),
|
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
|
// Combine outerscope info with user private key
|
||||||
map((userPrivateKey) => ({
|
map((userPrivateKey) => ({
|
||||||
userKey,
|
userKey,
|
||||||
|
|||||||
Reference in New Issue
Block a user