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-16984] Improve type safety in decryption #12885

Open
wants to merge 4 commits into
base: km/improve-logging
Choose a base branch
from
Open
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 @@ -39,11 +39,10 @@ export class Collection extends Domain {
}

decrypt(orgKey: OrgKey): Promise<CollectionView> {
return this.decryptObj(
return this.decryptObj<Collection, CollectionView>(
this,
new CollectionView(this),
{
name: null,
},
["name"],
this.organizationId,
orgKey,
);
Expand Down
57 changes: 30 additions & 27 deletions libs/common/src/platform/models/domain/domain-base.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
// FIXME: Update this file to be type safe and remove this and next line
// @ts-strict-ignore
import { ConditionalExcept, ConditionalKeys, Constructor } from "type-fest";

import { View } from "../../../models/view/view";
Expand All @@ -15,6 +13,19 @@ export type DecryptedObject<
TDecryptedKeys extends EncStringKeys<TEncryptedObject>,
> = Record<TDecryptedKeys, string> & Omit<TEncryptedObject, TDecryptedKeys>;

// extracts shared keys from the domain and view types
type EncryptableKeys<D extends Domain, V extends View> = (keyof D &
ConditionalKeys<D, EncString | null>) &
(keyof V & ConditionalKeys<V, string | null>);

type DomainEncryptableKeys<D extends Domain> = {
[key in ConditionalKeys<D, EncString | null>]: EncString | null;
};

type ViewEncryptableKeys<V extends View> = {
[key in ConditionalKeys<V, string | null>]: string | null;
};

// https://contributing.bitwarden.com/architecture/clients/data-model#domain
export default class Domain {
protected buildDomainModel<D extends Domain>(
Expand All @@ -37,6 +48,7 @@ export default class Domain {
}
}
}

protected buildDataModel<D extends Domain>(
domain: D,
dataObj: any,
Expand All @@ -58,31 +70,24 @@ export default class Domain {
}
}

protected async decryptObj<T extends View>(
viewModel: T,
map: any,
orgId: string,
key: SymmetricCryptoKey = null,
protected async decryptObj<D extends Domain, V extends View>(
domain: DomainEncryptableKeys<D>,
viewModel: ViewEncryptableKeys<V>,
props: EncryptableKeys<D, V>[],
orgId: string | null,
key: SymmetricCryptoKey | null = null,
objectContext: string = "No Domain Context",
): Promise<T> {
const self: any = this;

for (const prop in map) {
// eslint-disable-next-line
if (!map.hasOwnProperty(prop)) {
continue;
}

const mapProp = map[prop] || prop;
if (self[mapProp]) {
(viewModel as any)[prop] = await self[mapProp].decrypt(
): Promise<V> {
for (const prop of props) {
viewModel[prop] =
(await domain[prop]?.decrypt(
orgId,
key,
`Property: ${prop}; ObjectContext: ${objectContext}`,
);
}
`Property: ${prop as string}; ObjectContext: ${objectContext}`,
)) ?? null;
}
return viewModel;

return viewModel as V;
}

/**
Expand Down Expand Up @@ -111,7 +116,7 @@ export default class Domain {
const decryptedObjects = [];

for (const prop of encryptedProperties) {
const value = (this as any)[prop] as EncString;
const value = this[prop] as EncString;
const decrypted = await this.decryptProperty(
prop,
value,
Expand All @@ -138,11 +143,9 @@ export default class Domain {
encryptService: EncryptService,
decryptTrace: string,
) {
let decrypted: string = null;
let decrypted: string | null = null;
if (value) {
decrypted = await value.decryptWithKey(key, encryptService, decryptTrace);
} else {
decrypted = null;
}
return {
[propertyKey]: decrypted,
Expand Down
6 changes: 5 additions & 1 deletion libs/common/src/platform/models/domain/enc-string.ts
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,11 @@ export class EncString implements Encrypted {
return EXPECTED_NUM_PARTS_BY_ENCRYPTION_TYPE[encType] === encPieces.length;
}

async decrypt(orgId: string, key: SymmetricCryptoKey = null, context?: string): Promise<string> {
async decrypt(
orgId: string | null,
key: SymmetricCryptoKey | null = null,
context?: string,
): Promise<string> {
if (this.decryptedValue != null) {
return this.decryptedValue;
}
Expand Down
9 changes: 1 addition & 8 deletions libs/common/src/tools/send/models/domain/send-access.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,14 +54,7 @@ export class SendAccess extends Domain {
async decrypt(key: SymmetricCryptoKey): Promise<SendAccessView> {
const model = new SendAccessView(this);

await this.decryptObj(
model,
{
name: null,
},
null,
key,
);
await this.decryptObj<SendAccess, SendAccessView>(this, model, ["name"], null, key);

switch (this.type) {
case SendType.File:
Expand Down
8 changes: 3 additions & 5 deletions libs/common/src/tools/send/models/domain/send-file.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,15 +34,13 @@ export class SendFile extends Domain {
}

async decrypt(key: SymmetricCryptoKey): Promise<SendFileView> {
const view = await this.decryptObj(
return await this.decryptObj<SendFile, SendFileView>(
this,
new SendFileView(this),
{
fileName: null,
},
["fileName"],
null,
key,
);
return view;
}

static fromJSON(obj: Jsonify<SendFile>) {
Expand Down
7 changes: 3 additions & 4 deletions libs/common/src/tools/send/models/domain/send-text.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,10 @@ export class SendText extends Domain {
}

decrypt(key: SymmetricCryptoKey): Promise<SendTextView> {
return this.decryptObj(
return this.decryptObj<SendText, SendTextView>(
this,
new SendTextView(this),
{
text: null,
},
["text"],
null,
key,
);
Expand Down
10 changes: 1 addition & 9 deletions libs/common/src/tools/send/models/domain/send.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,15 +87,7 @@ export class Send extends Domain {
// TODO: error?
}

await this.decryptObj(
model,
{
name: null,
notes: null,
},
null,
model.cryptoKey,
);
await this.decryptObj<Send, SendView>(this, model, ["name", "notes"], null, model.cryptoKey);

switch (this.type) {
case SendType.File:
Expand Down
7 changes: 3 additions & 4 deletions libs/common/src/vault/models/domain/attachment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,10 @@ export class Attachment extends Domain {
context = "No Cipher Context",
encKey?: SymmetricCryptoKey,
): Promise<AttachmentView> {
const view = await this.decryptObj(
const view = await this.decryptObj<Attachment, AttachmentView>(
this,
new AttachmentView(this),
{
fileName: null,
},
["fileName"],
orgId,
encKey,
"DomainType: Attachment; " + context,
Expand Down
12 changes: 3 additions & 9 deletions libs/common/src/vault/models/domain/card.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,16 +42,10 @@ export class Card extends Domain {
context = "No Cipher Context",
encKey?: SymmetricCryptoKey,
): Promise<CardView> {
return this.decryptObj(
return this.decryptObj<Card, CardView>(
this,
new CardView(),
{
cardholderName: null,
brand: null,
number: null,
expMonth: null,
expYear: null,
code: null,
},
["cardholderName", "brand", "number", "expMonth", "expYear", "code"],
orgId,
encKey,
"DomainType: Card; " + context,
Expand Down
8 changes: 3 additions & 5 deletions libs/common/src/vault/models/domain/cipher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -154,12 +154,10 @@ export class Cipher extends Domain implements Decryptable<CipherView> {
bypassValidation = false;
}

await this.decryptObj(
await this.decryptObj<Cipher, CipherView>(
this,
model,
{
name: null,
notes: null,
},
["name", "notes"],
this.organizationId,
encKey,
);
Expand Down
47 changes: 22 additions & 25 deletions libs/common/src/vault/models/domain/fido2-credential.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,41 +52,38 @@ export class Fido2Credential extends Domain {
}

async decrypt(orgId: string, encKey?: SymmetricCryptoKey): Promise<Fido2CredentialView> {
const view = await this.decryptObj(
const view = await this.decryptObj<Fido2Credential, Fido2CredentialView>(
this,
new Fido2CredentialView(),
{
credentialId: null,
keyType: null,
keyAlgorithm: null,
keyCurve: null,
keyValue: null,
rpId: null,
userHandle: null,
userName: null,
rpName: null,
userDisplayName: null,
discoverable: null,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removal of discoverable was deliberate, since it's boolean type in Fido2CredentialView and can't be decrypted from EncString directly. This did not error, since types do not exist on runtime and it's get's overriden as boolean few lines below.

},
[
"credentialId",
"keyType",
"keyAlgorithm",
"keyCurve",
"keyValue",
"rpId",
"userHandle",
"userName",
"rpName",
"userDisplayName",
],
orgId,
encKey,
);

const { counter } = await this.decryptObj(
{ counter: "" },
const { counter } = await this.decryptObj<
Fido2Credential,
{
counter: null,
},
orgId,
encKey,
);
counter: string;
}
>(this, { counter: "" }, ["counter"], orgId, encKey);
// Counter will end up as NaN if this fails
view.counter = parseInt(counter);

const { discoverable } = await this.decryptObj(
const { discoverable } = await this.decryptObj<Fido2Credential, { discoverable: string }>(
this,
{ discoverable: "" },
{
discoverable: null,
},
["discoverable"],
orgId,
encKey,
);
Expand Down
8 changes: 3 additions & 5 deletions libs/common/src/vault/models/domain/field.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,10 @@ export class Field extends Domain {
}

decrypt(orgId: string, encKey?: SymmetricCryptoKey): Promise<FieldView> {
return this.decryptObj(
return this.decryptObj<Field, FieldView>(
this,
new FieldView(this),
{
name: null,
value: null,
},
["name", "value"],
orgId,
encKey,
);
Expand Down
8 changes: 1 addition & 7 deletions libs/common/src/vault/models/domain/folder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,7 @@ export class Folder extends Domain {
}

decrypt(): Promise<FolderView> {
return this.decryptObj(
new FolderView(this),
{
name: null,
},
null,
);
return this.decryptObj<Folder, FolderView>(this, new FolderView(this), ["name"], null);
}

async decryptWithKey(
Expand Down
43 changes: 22 additions & 21 deletions libs/common/src/vault/models/domain/identity.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,28 +66,29 @@ export class Identity extends Domain {
context: string = "No Cipher Context",
encKey?: SymmetricCryptoKey,
): Promise<IdentityView> {
return this.decryptObj(
return this.decryptObj<Identity, IdentityView>(
this,
new IdentityView(),
{
title: null,
firstName: null,
middleName: null,
lastName: null,
address1: null,
address2: null,
address3: null,
city: null,
state: null,
postalCode: null,
country: null,
company: null,
email: null,
phone: null,
ssn: null,
username: null,
passportNumber: null,
licenseNumber: null,
},
[
"title",
"firstName",
"middleName",
"lastName",
"address1",
"address2",
"address3",
"city",
"state",
"postalCode",
"country",
"company",
"email",
"phone",
"ssn",
"username",
"passportNumber",
"licenseNumber",
],
orgId,
encKey,
"DomainType: Identity; " + context,
Expand Down
Loading
Loading