From cdb26d6eca6a33defe215a531fffa2b370a2f3b8 Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Mon, 26 Sep 2022 13:33:40 +1000 Subject: [PATCH 01/19] Initial POC --- .../spec/misc/encString.decorator.spec.ts | 18 ++++++++++ .../spec/services/encrypt.service.spec.ts | 34 +++++++++++++++++++ .../abstractions/abstractEncrypt.service.ts | 2 ++ libs/common/src/interfaces/IDecryptable.ts | 3 ++ libs/common/src/misc/decryptable.decorator.ts | 16 +++++++++ libs/common/src/misc/encString.decorator.ts | 16 +++++++++ libs/common/src/models/domain/login.ts | 29 +++++++++++++--- libs/common/src/services/encrypt.service.ts | 23 +++++++++++++ 8 files changed, 136 insertions(+), 5 deletions(-) create mode 100644 libs/common/spec/misc/encString.decorator.spec.ts create mode 100644 libs/common/src/interfaces/IDecryptable.ts create mode 100644 libs/common/src/misc/decryptable.decorator.ts create mode 100644 libs/common/src/misc/encString.decorator.ts diff --git a/libs/common/spec/misc/encString.decorator.spec.ts b/libs/common/spec/misc/encString.decorator.spec.ts new file mode 100644 index 000000000000..4bd74109c8b2 --- /dev/null +++ b/libs/common/spec/misc/encString.decorator.spec.ts @@ -0,0 +1,18 @@ +import { encString, getEncStringList } from "@bitwarden/common/misc/encString.decorator"; + +class TestClass { + @encString + encryptedString: string; + @encString + anotherEncryptedString: string; + + someOtherProperty: Date; +} + +describe("encString decorator", () => { + it("adds property name to list", () => { + const testClass = new TestClass(); + const result = getEncStringList(testClass); + expect(result).toEqual(["encryptedString", "anotherEncryptedString"]); + }); +}); diff --git a/libs/common/spec/services/encrypt.service.spec.ts b/libs/common/spec/services/encrypt.service.spec.ts index 9567d8fbb248..c37b1201d771 100644 --- a/libs/common/spec/services/encrypt.service.spec.ts +++ b/libs/common/spec/services/encrypt.service.spec.ts @@ -5,7 +5,9 @@ import { LogService } from "@bitwarden/common/abstractions/log.service"; import { EncryptionType } from "@bitwarden/common/enums/encryptionType"; import { EncArrayBuffer } from "@bitwarden/common/models/domain/encArrayBuffer"; import { EncString } from "@bitwarden/common/models/domain/encString"; +import { Login } from "@bitwarden/common/models/domain/login"; import { SymmetricCryptoKey } from "@bitwarden/common/models/domain/symmetricCryptoKey"; +import { LoginView } from "@bitwarden/common/models/view/loginView"; import { EncryptService } from "@bitwarden/common/services/encrypt.service"; import { makeStaticByteArray } from "../utils"; @@ -185,4 +187,36 @@ describe("EncryptService", () => { expect(actual).toEqual(key); }); }); + + describe("decryptItem", () => { + it("proof of concept test", async () => { + const date = new Date(); + + const login = new Login(); + login.uris = null; + login.username = new EncString("3.myUsername_Encrypted"); + login.password = new EncString("3.myPassword_Encrypted"); + login.totp = new EncString("3.myTotp_Encrypted"); + login.passwordRevisionDate = date; + login.autofillOnPageLoad = true; + + jest + .spyOn(encryptService, "decryptToUtf8") + .mockImplementation((encString, key) => + Promise.resolve(encString.data.replace("_Encrypted", "")) + ); + + const result = await encryptService.decryptItem(login, mock()); + + expect(result).toEqual({ + uris: null, + username: "myUsername", + password: "myPassword", + totp: "myTotp", + passwordRevisionDate: date, + autofillOnPageLoad: true, + }); + expect(result).toBeInstanceOf(LoginView); + }); + }); }); diff --git a/libs/common/src/abstractions/abstractEncrypt.service.ts b/libs/common/src/abstractions/abstractEncrypt.service.ts index c683f22d33d6..6c832591249b 100644 --- a/libs/common/src/abstractions/abstractEncrypt.service.ts +++ b/libs/common/src/abstractions/abstractEncrypt.service.ts @@ -1,6 +1,7 @@ import { EncString } from "@bitwarden/common/models/domain/encString"; import { SymmetricCryptoKey } from "@bitwarden/common/models/domain/symmetricCryptoKey"; +import { IDecryptable } from "../interfaces/IDecryptable"; import { IEncrypted } from "../interfaces/IEncrypted"; import { EncArrayBuffer } from "../models/domain/encArrayBuffer"; @@ -13,4 +14,5 @@ export abstract class AbstractEncryptService { abstract decryptToUtf8: (encString: EncString, key: SymmetricCryptoKey) => Promise; abstract decryptToBytes: (encThing: IEncrypted, key: SymmetricCryptoKey) => Promise; abstract resolveLegacyKey: (key: SymmetricCryptoKey, encThing: IEncrypted) => SymmetricCryptoKey; + abstract decryptItem: (item: IDecryptable, key: SymmetricCryptoKey) => Promise; } diff --git a/libs/common/src/interfaces/IDecryptable.ts b/libs/common/src/interfaces/IDecryptable.ts new file mode 100644 index 000000000000..456c7b266392 --- /dev/null +++ b/libs/common/src/interfaces/IDecryptable.ts @@ -0,0 +1,3 @@ +export interface IDecryptable { + toView: () => T; +} diff --git a/libs/common/src/misc/decryptable.decorator.ts b/libs/common/src/misc/decryptable.decorator.ts new file mode 100644 index 000000000000..d3d1b407327c --- /dev/null +++ b/libs/common/src/misc/decryptable.decorator.ts @@ -0,0 +1,16 @@ +import "reflect-metadata"; + +const metadataKey = Symbol("decryptableList"); + +export function decryptable(prototype: any, propertyKey: string) { + const encStringList: string[] = Reflect.getMetadata(metadataKey, prototype); + if (encStringList == null) { + Reflect.defineMetadata(metadataKey, [propertyKey], prototype); + } else { + encStringList.push(propertyKey); + } +} + +export function getDecryptableList(target: any) { + return Reflect.getMetadata(metadataKey, target); +} diff --git a/libs/common/src/misc/encString.decorator.ts b/libs/common/src/misc/encString.decorator.ts new file mode 100644 index 000000000000..25e506ed0aba --- /dev/null +++ b/libs/common/src/misc/encString.decorator.ts @@ -0,0 +1,16 @@ +import "reflect-metadata"; + +const metadataKey = Symbol("encStringList"); + +export function encString(prototype: any, propertyKey: string) { + const encStringList: string[] = Reflect.getMetadata(metadataKey, prototype); + if (encStringList == null) { + Reflect.defineMetadata(metadataKey, [propertyKey], prototype); + } else { + encStringList.push(propertyKey); + } +} + +export function getEncStringList(target: any): string[] { + return Reflect.getMetadata(metadataKey, target); +} diff --git a/libs/common/src/models/domain/login.ts b/libs/common/src/models/domain/login.ts index 76ba402000f8..54d2cf860ef7 100644 --- a/libs/common/src/models/domain/login.ts +++ b/libs/common/src/models/domain/login.ts @@ -1,3 +1,7 @@ +import { IDecryptable } from "@bitwarden/common/interfaces/IDecryptable"; +import { decryptable } from "@bitwarden/common/misc/decryptable.decorator"; +import { encString } from "@bitwarden/common/misc/encString.decorator"; + import { LoginData } from "../data/loginData"; import { LoginView } from "../view/loginView"; @@ -6,12 +10,12 @@ import { EncString } from "./encString"; import { LoginUri } from "./loginUri"; import { SymmetricCryptoKey } from "./symmetricCryptoKey"; -export class Login extends Domain { - uris: LoginUri[]; - username: EncString; - password: EncString; +export class Login extends Domain implements IDecryptable { + @decryptable uris: LoginUri[]; + @encString username: EncString; + @encString password: EncString; passwordRevisionDate?: Date; - totp: EncString; + @encString totp: EncString; autofillOnPageLoad: boolean; constructor(obj?: LoginData) { @@ -85,4 +89,19 @@ export class Login extends Domain { return l; } + + toView() { + const view = new LoginView(); + + // Unencrypted properties + view.autofillOnPageLoad = this.autofillOnPageLoad; + view.passwordRevisionDate = this.passwordRevisionDate; + + // Encrypted properties + view.username = this.username.decryptedValue; + view.password = this.password.decryptedValue; + view.totp = this.totp.decryptedValue; + + return view; + } } diff --git a/libs/common/src/services/encrypt.service.ts b/libs/common/src/services/encrypt.service.ts index fef33a3f9c3e..1e035e8f46f2 100644 --- a/libs/common/src/services/encrypt.service.ts +++ b/libs/common/src/services/encrypt.service.ts @@ -7,7 +7,9 @@ import { SymmetricCryptoKey } from "@bitwarden/common/models/domain/symmetricCry import { AbstractEncryptService } from "../abstractions/abstractEncrypt.service"; import { EncryptionType } from "../enums/encryptionType"; +import { IDecryptable } from "../interfaces/IDecryptable"; import { IEncrypted } from "../interfaces/IEncrypted"; +import { getEncStringList } from "../misc/encString.decorator"; import { EncArrayBuffer } from "../models/domain/encArrayBuffer"; export class EncryptService implements AbstractEncryptService { @@ -165,6 +167,27 @@ export class EncryptService implements AbstractEncryptService { return obj; } + async decryptItem(item: IDecryptable, key: SymmetricCryptoKey) { + const encStringProps = getEncStringList(item); + + const promises: Promise[] = []; + + // TODO: check that each is actually an encstring? + // TODO: general error handling if any problem decrypting? (Error: could not decrypt text) + // relies on each encString cacheing its result - is that itself a good pattern that we want to extend here? + encStringProps.forEach((prop) => + promises.push( + this.decryptToUtf8((item as any)[prop], key).then( + (decryptedValue) => ((item as any)[prop].decryptedValue = decryptedValue) + ) + ) + ); + + await Promise.all(promises); + + return item.toView(); + } + private logMacFailed(msg: string) { if (this.logMacFailures) { this.logService.error(msg); From cfe233991936052b19fb3a2df37a0e82b4972288 Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Mon, 26 Sep 2022 14:04:17 +1000 Subject: [PATCH 02/19] handle errors --- .../spec/services/encrypt.service.spec.ts | 32 ++++++++++++++++++- libs/common/src/services/encrypt.service.ts | 11 ++++--- 2 files changed, 38 insertions(+), 5 deletions(-) diff --git a/libs/common/spec/services/encrypt.service.spec.ts b/libs/common/spec/services/encrypt.service.spec.ts index c37b1201d771..b28852e803e8 100644 --- a/libs/common/spec/services/encrypt.service.spec.ts +++ b/libs/common/spec/services/encrypt.service.spec.ts @@ -189,9 +189,12 @@ describe("EncryptService", () => { }); describe("decryptItem", () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + it("proof of concept test", async () => { const date = new Date(); - const login = new Login(); login.uris = null; login.username = new EncString("3.myUsername_Encrypted"); @@ -218,5 +221,32 @@ describe("EncryptService", () => { }); expect(result).toBeInstanceOf(LoginView); }); + + it("handles decryption errors", async () => { + const date = new Date(); + const login = new Login(); + login.uris = null; + login.username = new EncString("3.myUsername_Encrypted"); + login.password = new EncString("3.myPassword_Encrypted"); + login.totp = new EncString("3.myTotp_Encrypted"); + login.passwordRevisionDate = date; + login.autofillOnPageLoad = true; + + jest.spyOn(encryptService, "decryptToUtf8").mockRejectedValue("some decryption error"); + + const result = await encryptService.decryptItem(login, mock()); + + const decryptionError = "[error: cannot decrypt]"; + + expect(result).toEqual({ + uris: null, + username: decryptionError, + password: decryptionError, + totp: decryptionError, + passwordRevisionDate: date, + autofillOnPageLoad: true, + }); + expect(result).toBeInstanceOf(LoginView); + }); }); }); diff --git a/libs/common/src/services/encrypt.service.ts b/libs/common/src/services/encrypt.service.ts index 1e035e8f46f2..e175224f7e1b 100644 --- a/libs/common/src/services/encrypt.service.ts +++ b/libs/common/src/services/encrypt.service.ts @@ -170,16 +170,19 @@ export class EncryptService implements AbstractEncryptService { async decryptItem(item: IDecryptable, key: SymmetricCryptoKey) { const encStringProps = getEncStringList(item); - const promises: Promise[] = []; + const promises: Promise[] = []; // TODO: check that each is actually an encstring? // TODO: general error handling if any problem decrypting? (Error: could not decrypt text) // relies on each encString cacheing its result - is that itself a good pattern that we want to extend here? encStringProps.forEach((prop) => promises.push( - this.decryptToUtf8((item as any)[prop], key).then( - (decryptedValue) => ((item as any)[prop].decryptedValue = decryptedValue) - ) + this.decryptToUtf8((item as any)[prop], key) + .then((decryptedValue) => ((item as any)[prop].decryptedValue = decryptedValue)) + .catch((e) => { + this.logService.error(e); + (item as any)[prop].decryptedValue = "[error: cannot decrypt]"; + }) ) ); From 6013538842e3a0c07d7fbd5a047ceba4fd2393c2 Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Mon, 26 Sep 2022 14:07:42 +1000 Subject: [PATCH 03/19] remove old comments --- libs/common/src/services/encrypt.service.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/libs/common/src/services/encrypt.service.ts b/libs/common/src/services/encrypt.service.ts index e175224f7e1b..ba67716d37f5 100644 --- a/libs/common/src/services/encrypt.service.ts +++ b/libs/common/src/services/encrypt.service.ts @@ -172,8 +172,6 @@ export class EncryptService implements AbstractEncryptService { const promises: Promise[] = []; - // TODO: check that each is actually an encstring? - // TODO: general error handling if any problem decrypting? (Error: could not decrypt text) // relies on each encString cacheing its result - is that itself a good pattern that we want to extend here? encStringProps.forEach((prop) => promises.push( From 0adf42c8ac6acc30e6420bb2cafe777d6c70064f Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Mon, 26 Sep 2022 14:37:48 +1000 Subject: [PATCH 04/19] nested IDecryptable test, broken --- apps/web/src/app/app.component.ts | 3 +++ libs/common/src/misc/decryptable.decorator.ts | 10 ++++---- libs/common/src/misc/encString.decorator.ts | 2 -- libs/common/src/models/domain/loginUri.ts | 13 +++++++++- libs/common/src/services/encrypt.service.ts | 19 ++++++++++++--- libs/common/test.setup.ts | 2 ++ package-lock.json | 24 +++++++++++++++---- package.json | 1 + 8 files changed, 58 insertions(+), 16 deletions(-) diff --git a/apps/web/src/app/app.component.ts b/apps/web/src/app/app.component.ts index b1700a855130..b2b2e5768d42 100644 --- a/apps/web/src/app/app.component.ts +++ b/apps/web/src/app/app.component.ts @@ -37,6 +37,9 @@ import { SendOptionsPolicy } from "./organizations/policies/send-options.compone import { SingleOrgPolicy } from "./organizations/policies/single-org.component"; import { TwoFactorAuthenticationPolicy } from "./organizations/policies/two-factor-authentication.component"; +// This must only be imported once, it's available in the global scope +import "reflect-metadata"; + const BroadcasterSubscriptionId = "AppComponent"; const IdleTimeout = 60000 * 10; // 10 minutes diff --git a/libs/common/src/misc/decryptable.decorator.ts b/libs/common/src/misc/decryptable.decorator.ts index d3d1b407327c..2476bb49cf1c 100644 --- a/libs/common/src/misc/decryptable.decorator.ts +++ b/libs/common/src/misc/decryptable.decorator.ts @@ -1,16 +1,14 @@ -import "reflect-metadata"; - const metadataKey = Symbol("decryptableList"); export function decryptable(prototype: any, propertyKey: string) { - const encStringList: string[] = Reflect.getMetadata(metadataKey, prototype); - if (encStringList == null) { + const decryptableList: string[] = Reflect.getMetadata(metadataKey, prototype); + if (decryptableList == null) { Reflect.defineMetadata(metadataKey, [propertyKey], prototype); } else { - encStringList.push(propertyKey); + decryptableList.push(propertyKey); } } -export function getDecryptableList(target: any) { +export function getDecryptableList(target: any): string[] { return Reflect.getMetadata(metadataKey, target); } diff --git a/libs/common/src/misc/encString.decorator.ts b/libs/common/src/misc/encString.decorator.ts index 25e506ed0aba..9cfcf432c574 100644 --- a/libs/common/src/misc/encString.decorator.ts +++ b/libs/common/src/misc/encString.decorator.ts @@ -1,5 +1,3 @@ -import "reflect-metadata"; - const metadataKey = Symbol("encStringList"); export function encString(prototype: any, propertyKey: string) { diff --git a/libs/common/src/models/domain/loginUri.ts b/libs/common/src/models/domain/loginUri.ts index 9bd78c655f47..33d576cb0c1c 100644 --- a/libs/common/src/models/domain/loginUri.ts +++ b/libs/common/src/models/domain/loginUri.ts @@ -1,3 +1,6 @@ +import { IDecryptable } from "@bitwarden/common/interfaces/IDecryptable"; +import { encString } from "@bitwarden/common/misc/encString.decorator"; + import { UriMatchType } from "../../enums/uriMatchType"; import { LoginUriData } from "../data/loginUriData"; import { LoginUriView } from "../view/loginUriView"; @@ -6,7 +9,8 @@ import Domain from "./domainBase"; import { EncString } from "./encString"; import { SymmetricCryptoKey } from "./symmetricCryptoKey"; -export class LoginUri extends Domain { +export class LoginUri extends Domain implements IDecryptable { + @encString uri: EncString; match: UriMatchType; @@ -51,4 +55,11 @@ export class LoginUri extends Domain { ); return u; } + + toView() { + const view = new LoginUriView(); + view.uri = this.uri.decryptedValue; + view.match = this.match; + return view; + } } diff --git a/libs/common/src/services/encrypt.service.ts b/libs/common/src/services/encrypt.service.ts index ba67716d37f5..4cf6844581d4 100644 --- a/libs/common/src/services/encrypt.service.ts +++ b/libs/common/src/services/encrypt.service.ts @@ -9,6 +9,7 @@ import { AbstractEncryptService } from "../abstractions/abstractEncrypt.service" import { EncryptionType } from "../enums/encryptionType"; import { IDecryptable } from "../interfaces/IDecryptable"; import { IEncrypted } from "../interfaces/IEncrypted"; +import { getDecryptableList } from "../misc/decryptable.decorator"; import { getEncStringList } from "../misc/encString.decorator"; import { EncArrayBuffer } from "../models/domain/encArrayBuffer"; @@ -168,12 +169,12 @@ export class EncryptService implements AbstractEncryptService { } async decryptItem(item: IDecryptable, key: SymmetricCryptoKey) { - const encStringProps = getEncStringList(item); - const promises: Promise[] = []; + // Decrypt all encStrings of object // relies on each encString cacheing its result - is that itself a good pattern that we want to extend here? - encStringProps.forEach((prop) => + const encStringProps = getEncStringList(item); + encStringProps?.forEach((prop) => promises.push( this.decryptToUtf8((item as any)[prop], key) .then((decryptedValue) => ((item as any)[prop].decryptedValue = decryptedValue)) @@ -184,6 +185,18 @@ export class EncryptService implements AbstractEncryptService { ) ); + // Decrypt all nested IDecryptables (recursive call) + const decryptableProps = getDecryptableList(item); + decryptableProps?.forEach((prop) => { + const propValue = (item as any)[prop]; + + if (propValue instanceof Array) { + propValue.forEach((subItem) => promises.push(this.decryptItem(subItem, key))); + } else { + promises.push(this.decryptItem(propValue, key)); + } + }); + await Promise.all(promises); return item.toView(); diff --git a/libs/common/test.setup.ts b/libs/common/test.setup.ts index 17254ea34c6c..7e339583b96a 100644 --- a/libs/common/test.setup.ts +++ b/libs/common/test.setup.ts @@ -2,6 +2,8 @@ import { webcrypto } from "crypto"; import { toEqualBuffer } from "./spec/matchers/toEqualBuffer"; +import "reflect-metadata"; + Object.defineProperty(window, "crypto", { value: webcrypto, }); diff --git a/package-lock.json b/package-lock.json index a0f5a037f804..88e734c187ed 100644 --- a/package-lock.json +++ b/package-lock.json @@ -27,6 +27,7 @@ "@koa/router": "^10.1.1", "@microsoft/signalr": "^6.0.7", "@microsoft/signalr-protocol-msgpack": "^6.0.7", + "@types/reflect-metadata": "^0.1.0", "big-integer": "^1.6.51", "bootstrap": "4.6.0", "braintree-web-drop-in": "^1.33.1", @@ -13087,6 +13088,15 @@ "@types/react": "*" } }, + "node_modules/@types/reflect-metadata": { + "version": "0.1.0", + "resolved": "https://registry.npmjs.org/@types/reflect-metadata/-/reflect-metadata-0.1.0.tgz", + "integrity": "sha512-bXltFLY3qhzCnVYP5iUpeSICagQ8rc9K2liS+8M0lBcz54BHs3O6W5UvqespVSuebo1BXLi+/y9ioELAW9SC2A==", + "deprecated": "This is a stub types definition for reflect-metadata (https://github.com/rbuckton/ReflectDecorators). reflect-metadata provides its own type definitions, so you don't need @types/reflect-metadata installed!", + "dependencies": { + "reflect-metadata": "*" + } + }, "node_modules/@types/responselike": { "version": "1.0.0", "resolved": "https://registry.npmjs.org/@types/responselike/-/responselike-1.0.0.tgz", @@ -36240,8 +36250,7 @@ "node_modules/reflect-metadata": { "version": "0.1.13", "resolved": "https://registry.npmjs.org/reflect-metadata/-/reflect-metadata-0.1.13.tgz", - "integrity": "sha512-Ts1Y/anZELhSsjMcU605fU9RE4Oi3p5ORujwbIKXfWa+0Zxs510Qrmrce5/Jowq3cHSZSJqBjypxmHarc+vEWg==", - "dev": true + "integrity": "sha512-Ts1Y/anZELhSsjMcU605fU9RE4Oi3p5ORujwbIKXfWa+0Zxs510Qrmrce5/Jowq3cHSZSJqBjypxmHarc+vEWg==" }, "node_modules/refractor": { "version": "3.6.0", @@ -52941,6 +52950,14 @@ "@types/react": "*" } }, + "@types/reflect-metadata": { + "version": "0.1.0", + "resolved": "https://registry.npmjs.org/@types/reflect-metadata/-/reflect-metadata-0.1.0.tgz", + "integrity": "sha512-bXltFLY3qhzCnVYP5iUpeSICagQ8rc9K2liS+8M0lBcz54BHs3O6W5UvqespVSuebo1BXLi+/y9ioELAW9SC2A==", + "requires": { + "reflect-metadata": "*" + } + }, "@types/responselike": { "version": "1.0.0", "resolved": "https://registry.npmjs.org/@types/responselike/-/responselike-1.0.0.tgz", @@ -70802,8 +70819,7 @@ "reflect-metadata": { "version": "0.1.13", "resolved": "https://registry.npmjs.org/reflect-metadata/-/reflect-metadata-0.1.13.tgz", - "integrity": "sha512-Ts1Y/anZELhSsjMcU605fU9RE4Oi3p5ORujwbIKXfWa+0Zxs510Qrmrce5/Jowq3cHSZSJqBjypxmHarc+vEWg==", - "dev": true + "integrity": "sha512-Ts1Y/anZELhSsjMcU605fU9RE4Oi3p5ORujwbIKXfWa+0Zxs510Qrmrce5/Jowq3cHSZSJqBjypxmHarc+vEWg==" }, "refractor": { "version": "3.6.0", diff --git a/package.json b/package.json index 0260152ce47c..6f085979aee3 100644 --- a/package.json +++ b/package.json @@ -152,6 +152,7 @@ "@koa/router": "^10.1.1", "@microsoft/signalr": "^6.0.7", "@microsoft/signalr-protocol-msgpack": "^6.0.7", + "@types/reflect-metadata": "^0.1.0", "big-integer": "^1.6.51", "bootstrap": "4.6.0", "braintree-web-drop-in": "^1.33.1", From ea818a165ab31984f3f62d7b0d1de94142846374 Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Mon, 26 Sep 2022 16:21:00 +1000 Subject: [PATCH 05/19] Fix tests and refactor decorator functionms --- apps/web/src/app/app.component.ts | 3 -- ....spec.ts => decryptable.decorator.spec.ts} | 2 +- libs/common/src/misc/decryptable.decorator.ts | 34 +++++++++++++------ libs/common/src/misc/encString.decorator.ts | 14 -------- libs/common/src/models/domain/login.ts | 3 +- libs/common/src/models/domain/loginUri.ts | 2 +- libs/common/src/services/encrypt.service.ts | 10 ++++-- libs/common/test.setup.ts | 2 -- 8 files changed, 34 insertions(+), 36 deletions(-) rename libs/common/spec/misc/{encString.decorator.spec.ts => decryptable.decorator.spec.ts} (94%) delete mode 100644 libs/common/src/misc/encString.decorator.ts diff --git a/apps/web/src/app/app.component.ts b/apps/web/src/app/app.component.ts index b2b2e5768d42..b1700a855130 100644 --- a/apps/web/src/app/app.component.ts +++ b/apps/web/src/app/app.component.ts @@ -37,9 +37,6 @@ import { SendOptionsPolicy } from "./organizations/policies/send-options.compone import { SingleOrgPolicy } from "./organizations/policies/single-org.component"; import { TwoFactorAuthenticationPolicy } from "./organizations/policies/two-factor-authentication.component"; -// This must only be imported once, it's available in the global scope -import "reflect-metadata"; - const BroadcasterSubscriptionId = "AppComponent"; const IdleTimeout = 60000 * 10; // 10 minutes diff --git a/libs/common/spec/misc/encString.decorator.spec.ts b/libs/common/spec/misc/decryptable.decorator.spec.ts similarity index 94% rename from libs/common/spec/misc/encString.decorator.spec.ts rename to libs/common/spec/misc/decryptable.decorator.spec.ts index 4bd74109c8b2..4545f7fc83ef 100644 --- a/libs/common/spec/misc/encString.decorator.spec.ts +++ b/libs/common/spec/misc/decryptable.decorator.spec.ts @@ -1,4 +1,4 @@ -import { encString, getEncStringList } from "@bitwarden/common/misc/encString.decorator"; +import { encString, getEncStringList } from "@bitwarden/common/misc/decryptable.decorator"; class TestClass { @encString diff --git a/libs/common/src/misc/decryptable.decorator.ts b/libs/common/src/misc/decryptable.decorator.ts index 2476bb49cf1c..554c1eb87ef7 100644 --- a/libs/common/src/misc/decryptable.decorator.ts +++ b/libs/common/src/misc/decryptable.decorator.ts @@ -1,14 +1,26 @@ -const metadataKey = Symbol("decryptableList"); - -export function decryptable(prototype: any, propertyKey: string) { - const decryptableList: string[] = Reflect.getMetadata(metadataKey, prototype); - if (decryptableList == null) { - Reflect.defineMetadata(metadataKey, [propertyKey], prototype); - } else { - decryptableList.push(propertyKey); - } +import "reflect-metadata"; // TODO: this can only be imported in one place, so shouldn't be here + +function listDecoratorFactory(metadataKey: symbol) { + return function (prototype: any, propertyKey: string) { + const encStringList: string[] = Reflect.getMetadata(metadataKey, prototype); + if (encStringList == null) { + Reflect.defineMetadata(metadataKey, [propertyKey], prototype); + } else { + encStringList.push(propertyKey); + } + }; } -export function getDecryptableList(target: any): string[] { - return Reflect.getMetadata(metadataKey, target); +function getListFactory(metadataKey: symbol) { + return function (target: any): string[] { + return Reflect.getMetadata(metadataKey, target); + }; } + +const encStringListKey = Symbol("encStringList"); +export const encString = listDecoratorFactory(encStringListKey); +export const getEncStringList = getListFactory(encStringListKey); + +const decryptableListKey = Symbol("encStringList"); +export const decryptable = listDecoratorFactory(decryptableListKey); +export const getDecryptableList = getListFactory(decryptableListKey); diff --git a/libs/common/src/misc/encString.decorator.ts b/libs/common/src/misc/encString.decorator.ts deleted file mode 100644 index 9cfcf432c574..000000000000 --- a/libs/common/src/misc/encString.decorator.ts +++ /dev/null @@ -1,14 +0,0 @@ -const metadataKey = Symbol("encStringList"); - -export function encString(prototype: any, propertyKey: string) { - const encStringList: string[] = Reflect.getMetadata(metadataKey, prototype); - if (encStringList == null) { - Reflect.defineMetadata(metadataKey, [propertyKey], prototype); - } else { - encStringList.push(propertyKey); - } -} - -export function getEncStringList(target: any): string[] { - return Reflect.getMetadata(metadataKey, target); -} diff --git a/libs/common/src/models/domain/login.ts b/libs/common/src/models/domain/login.ts index 54d2cf860ef7..b5e2caa1053c 100644 --- a/libs/common/src/models/domain/login.ts +++ b/libs/common/src/models/domain/login.ts @@ -1,6 +1,5 @@ import { IDecryptable } from "@bitwarden/common/interfaces/IDecryptable"; -import { decryptable } from "@bitwarden/common/misc/decryptable.decorator"; -import { encString } from "@bitwarden/common/misc/encString.decorator"; +import { encString, decryptable } from "@bitwarden/common/misc/decryptable.decorator"; import { LoginData } from "../data/loginData"; import { LoginView } from "../view/loginView"; diff --git a/libs/common/src/models/domain/loginUri.ts b/libs/common/src/models/domain/loginUri.ts index 33d576cb0c1c..110afc2f4565 100644 --- a/libs/common/src/models/domain/loginUri.ts +++ b/libs/common/src/models/domain/loginUri.ts @@ -1,5 +1,5 @@ import { IDecryptable } from "@bitwarden/common/interfaces/IDecryptable"; -import { encString } from "@bitwarden/common/misc/encString.decorator"; +import { encString } from "@bitwarden/common/misc/decryptable.decorator"; import { UriMatchType } from "../../enums/uriMatchType"; import { LoginUriData } from "../data/loginUriData"; diff --git a/libs/common/src/services/encrypt.service.ts b/libs/common/src/services/encrypt.service.ts index 4cf6844581d4..dd7ce3344810 100644 --- a/libs/common/src/services/encrypt.service.ts +++ b/libs/common/src/services/encrypt.service.ts @@ -9,8 +9,7 @@ import { AbstractEncryptService } from "../abstractions/abstractEncrypt.service" import { EncryptionType } from "../enums/encryptionType"; import { IDecryptable } from "../interfaces/IDecryptable"; import { IEncrypted } from "../interfaces/IEncrypted"; -import { getDecryptableList } from "../misc/decryptable.decorator"; -import { getEncStringList } from "../misc/encString.decorator"; +import { getEncStringList, getDecryptableList } from "../misc/decryptable.decorator"; import { EncArrayBuffer } from "../models/domain/encArrayBuffer"; export class EncryptService implements AbstractEncryptService { @@ -169,6 +168,10 @@ export class EncryptService implements AbstractEncryptService { } async decryptItem(item: IDecryptable, key: SymmetricCryptoKey) { + if (item == null) { + throw new Error("Cannot decrypt a null item"); + } + const promises: Promise[] = []; // Decrypt all encStrings of object @@ -189,6 +192,9 @@ export class EncryptService implements AbstractEncryptService { const decryptableProps = getDecryptableList(item); decryptableProps?.forEach((prop) => { const propValue = (item as any)[prop]; + if (propValue == null) { + return; + } if (propValue instanceof Array) { propValue.forEach((subItem) => promises.push(this.decryptItem(subItem, key))); diff --git a/libs/common/test.setup.ts b/libs/common/test.setup.ts index 7e339583b96a..17254ea34c6c 100644 --- a/libs/common/test.setup.ts +++ b/libs/common/test.setup.ts @@ -2,8 +2,6 @@ import { webcrypto } from "crypto"; import { toEqualBuffer } from "./spec/matchers/toEqualBuffer"; -import "reflect-metadata"; - Object.defineProperty(window, "crypto", { value: webcrypto, }); From b7e3d86c94658e07b9fdde625108e9fcb2b56496 Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Mon, 26 Sep 2022 16:40:51 +1000 Subject: [PATCH 06/19] Working nested decryptables --- .../spec/services/encrypt.service.spec.ts | 46 ++++++++++++++++--- libs/common/src/models/domain/login.ts | 7 +-- libs/common/src/services/encrypt.service.ts | 16 +++++-- 3 files changed, 55 insertions(+), 14 deletions(-) diff --git a/libs/common/spec/services/encrypt.service.spec.ts b/libs/common/spec/services/encrypt.service.spec.ts index b28852e803e8..76f5cf2f2598 100644 --- a/libs/common/spec/services/encrypt.service.spec.ts +++ b/libs/common/spec/services/encrypt.service.spec.ts @@ -11,6 +11,9 @@ import { LoginView } from "@bitwarden/common/models/view/loginView"; import { EncryptService } from "@bitwarden/common/services/encrypt.service"; import { makeStaticByteArray } from "../utils"; +import { LoginUri } from "@bitwarden/common/models/domain/loginUri"; +import { LoginUriView } from "@bitwarden/common/models/view/loginUriView"; +import { UriMatchType } from "@bitwarden/common/enums/uriMatchType"; describe("EncryptService", () => { const cryptoFunctionService = mock(); @@ -193,7 +196,7 @@ describe("EncryptService", () => { jest.clearAllMocks(); }); - it("proof of concept test", async () => { + it("decrypts encStrings on target object", async () => { const date = new Date(); const login = new Login(); login.uris = null; @@ -211,8 +214,7 @@ describe("EncryptService", () => { const result = await encryptService.decryptItem(login, mock()); - expect(result).toEqual({ - uris: null, + expect(result).toMatchObject({ username: "myUsername", password: "myPassword", totp: "myTotp", @@ -222,7 +224,7 @@ describe("EncryptService", () => { expect(result).toBeInstanceOf(LoginView); }); - it("handles decryption errors", async () => { + it("handles decryption errors in encStrings", async () => { const date = new Date(); const login = new Login(); login.uris = null; @@ -238,8 +240,7 @@ describe("EncryptService", () => { const decryptionError = "[error: cannot decrypt]"; - expect(result).toEqual({ - uris: null, + expect(result).toMatchObject({ username: decryptionError, password: decryptionError, totp: decryptionError, @@ -248,5 +249,38 @@ describe("EncryptService", () => { }); expect(result).toBeInstanceOf(LoginView); }); + + it("decrypts nested IDecryptables", async () => { + const uri1 = new LoginUri(); + uri1.uri = new EncString("3.someUri_Encrypted"); + uri1.match = UriMatchType.Domain; + + const uri2 = new LoginUri(); + uri2.uri = new EncString("3.anotherUri_Encrypted"); + uri2.match = UriMatchType.Host; + + const login = new Login(); + login.uris = [uri1, uri2]; + + jest + .spyOn(encryptService, "decryptToUtf8") + .mockImplementation((encString, key) => + Promise.resolve(encString.data.replace("_Encrypted", "")) + ); + + const result = await encryptService.decryptItem(login, mock()); + + expect(result.uris[0]).toMatchObject({ + uri: "someUri", + match: UriMatchType.Domain, + }); + expect(result.uris[0]).toBeInstanceOf(LoginUriView); + + expect(result.uris[1]).toMatchObject({ + uri: "anotherUri", + match: UriMatchType.Host, + }); + expect(result.uris[1]).toBeInstanceOf(LoginUriView); + }); }); }); diff --git a/libs/common/src/models/domain/login.ts b/libs/common/src/models/domain/login.ts index b5e2caa1053c..1642d2317a97 100644 --- a/libs/common/src/models/domain/login.ts +++ b/libs/common/src/models/domain/login.ts @@ -97,9 +97,10 @@ export class Login extends Domain implements IDecryptable { view.passwordRevisionDate = this.passwordRevisionDate; // Encrypted properties - view.username = this.username.decryptedValue; - view.password = this.password.decryptedValue; - view.totp = this.totp.decryptedValue; + view.username = this.username?.decryptedValue; + view.password = this.password?.decryptedValue; + view.totp = this.totp?.decryptedValue; + view.uris = this.uris?.map((uri) => uri.toView()); return view; } diff --git a/libs/common/src/services/encrypt.service.ts b/libs/common/src/services/encrypt.service.ts index dd7ce3344810..2cdcaf42166a 100644 --- a/libs/common/src/services/encrypt.service.ts +++ b/libs/common/src/services/encrypt.service.ts @@ -175,21 +175,27 @@ export class EncryptService implements AbstractEncryptService { const promises: Promise[] = []; // Decrypt all encStrings of object - // relies on each encString cacheing its result - is that itself a good pattern that we want to extend here? + // relies on each encString cacheing its result - is that a good pattern that we want to extend here? const encStringProps = getEncStringList(item); - encStringProps?.forEach((prop) => + encStringProps?.forEach((prop) => { + const propValue = (item as any)[prop]; + if (propValue == null) { + return; + } + promises.push( - this.decryptToUtf8((item as any)[prop], key) + this.decryptToUtf8(propValue, key) .then((decryptedValue) => ((item as any)[prop].decryptedValue = decryptedValue)) .catch((e) => { this.logService.error(e); (item as any)[prop].decryptedValue = "[error: cannot decrypt]"; }) - ) - ); + ); + }); // Decrypt all nested IDecryptables (recursive call) const decryptableProps = getDecryptableList(item); + decryptableProps?.forEach((prop) => { const propValue = (item as any)[prop]; if (propValue == null) { From 4ac5152453ea3032bc368d744b8d2d4689a490fa Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Mon, 26 Sep 2022 16:46:33 +1000 Subject: [PATCH 07/19] Add comments --- libs/common/src/misc/decryptable.decorator.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/libs/common/src/misc/decryptable.decorator.ts b/libs/common/src/misc/decryptable.decorator.ts index 554c1eb87ef7..4ef09202b14c 100644 --- a/libs/common/src/misc/decryptable.decorator.ts +++ b/libs/common/src/misc/decryptable.decorator.ts @@ -1,5 +1,8 @@ import "reflect-metadata"; // TODO: this can only be imported in one place, so shouldn't be here +/** + * Returns a decorator which will add the decorated property name to an array, which is stored under the metadataKey + */ function listDecoratorFactory(metadataKey: symbol) { return function (prototype: any, propertyKey: string) { const encStringList: string[] = Reflect.getMetadata(metadataKey, prototype); @@ -11,6 +14,9 @@ function listDecoratorFactory(metadataKey: symbol) { }; } +/** + * Returns a function which returns an array of property names stored with the list decorator under the metadataKey + */ function getListFactory(metadataKey: symbol) { return function (target: any): string[] { return Reflect.getMetadata(metadataKey, target); From dbd744b2b2714f2e5a527f04b44279a08bfc52e7 Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Mon, 3 Oct 2022 10:56:20 +1000 Subject: [PATCH 08/19] Combine decorators --- ...or.spec.ts => encrypted.decorator.spec.ts} | 8 ++--- libs/common/src/misc/decryptable.decorator.ts | 32 ----------------- libs/common/src/misc/encrypted.decorator.ts | 16 +++++++++ libs/common/src/models/domain/login.ts | 10 +++--- libs/common/src/models/domain/loginUri.ts | 4 +-- libs/common/src/services/encrypt.service.ts | 36 ++++++++----------- 6 files changed, 42 insertions(+), 64 deletions(-) rename libs/common/spec/misc/{decryptable.decorator.spec.ts => encrypted.decorator.spec.ts} (64%) delete mode 100644 libs/common/src/misc/decryptable.decorator.ts create mode 100644 libs/common/src/misc/encrypted.decorator.ts diff --git a/libs/common/spec/misc/decryptable.decorator.spec.ts b/libs/common/spec/misc/encrypted.decorator.spec.ts similarity index 64% rename from libs/common/spec/misc/decryptable.decorator.spec.ts rename to libs/common/spec/misc/encrypted.decorator.spec.ts index 4545f7fc83ef..1837a81958e7 100644 --- a/libs/common/spec/misc/decryptable.decorator.spec.ts +++ b/libs/common/spec/misc/encrypted.decorator.spec.ts @@ -1,9 +1,9 @@ -import { encString, getEncStringList } from "@bitwarden/common/misc/decryptable.decorator"; +import { encrypted, getEncryptedProperties } from "@bitwarden/common/misc/encrypted.decorator"; class TestClass { - @encString + @encrypted encryptedString: string; - @encString + @encrypted anotherEncryptedString: string; someOtherProperty: Date; @@ -12,7 +12,7 @@ class TestClass { describe("encString decorator", () => { it("adds property name to list", () => { const testClass = new TestClass(); - const result = getEncStringList(testClass); + const result = getEncryptedProperties(testClass); expect(result).toEqual(["encryptedString", "anotherEncryptedString"]); }); }); diff --git a/libs/common/src/misc/decryptable.decorator.ts b/libs/common/src/misc/decryptable.decorator.ts deleted file mode 100644 index 4ef09202b14c..000000000000 --- a/libs/common/src/misc/decryptable.decorator.ts +++ /dev/null @@ -1,32 +0,0 @@ -import "reflect-metadata"; // TODO: this can only be imported in one place, so shouldn't be here - -/** - * Returns a decorator which will add the decorated property name to an array, which is stored under the metadataKey - */ -function listDecoratorFactory(metadataKey: symbol) { - return function (prototype: any, propertyKey: string) { - const encStringList: string[] = Reflect.getMetadata(metadataKey, prototype); - if (encStringList == null) { - Reflect.defineMetadata(metadataKey, [propertyKey], prototype); - } else { - encStringList.push(propertyKey); - } - }; -} - -/** - * Returns a function which returns an array of property names stored with the list decorator under the metadataKey - */ -function getListFactory(metadataKey: symbol) { - return function (target: any): string[] { - return Reflect.getMetadata(metadataKey, target); - }; -} - -const encStringListKey = Symbol("encStringList"); -export const encString = listDecoratorFactory(encStringListKey); -export const getEncStringList = getListFactory(encStringListKey); - -const decryptableListKey = Symbol("encStringList"); -export const decryptable = listDecoratorFactory(decryptableListKey); -export const getDecryptableList = getListFactory(decryptableListKey); diff --git a/libs/common/src/misc/encrypted.decorator.ts b/libs/common/src/misc/encrypted.decorator.ts new file mode 100644 index 000000000000..8d9fcc36a42d --- /dev/null +++ b/libs/common/src/misc/encrypted.decorator.ts @@ -0,0 +1,16 @@ +import "reflect-metadata"; // TODO: this can only be imported in one place, so shouldn't be here + +const metadataKey = Symbol("encryptedPropertiesKey"); + +export function encrypted(prototype: any, propertyKey: string) { + const encStringList: string[] = Reflect.getMetadata(metadataKey, prototype); + if (encStringList == null) { + Reflect.defineMetadata(metadataKey, [propertyKey], prototype); + } else { + encStringList.push(propertyKey); + } +} + +export function getEncryptedProperties(target: any): string[] { + return Reflect.getMetadata(metadataKey, target); +} diff --git a/libs/common/src/models/domain/login.ts b/libs/common/src/models/domain/login.ts index 1642d2317a97..866c46464d0c 100644 --- a/libs/common/src/models/domain/login.ts +++ b/libs/common/src/models/domain/login.ts @@ -1,5 +1,5 @@ import { IDecryptable } from "@bitwarden/common/interfaces/IDecryptable"; -import { encString, decryptable } from "@bitwarden/common/misc/decryptable.decorator"; +import { encrypted } from "@bitwarden/common/misc/encrypted.decorator"; import { LoginData } from "../data/loginData"; import { LoginView } from "../view/loginView"; @@ -10,11 +10,11 @@ import { LoginUri } from "./loginUri"; import { SymmetricCryptoKey } from "./symmetricCryptoKey"; export class Login extends Domain implements IDecryptable { - @decryptable uris: LoginUri[]; - @encString username: EncString; - @encString password: EncString; + @encrypted uris: LoginUri[]; + @encrypted username: EncString; + @encrypted password: EncString; passwordRevisionDate?: Date; - @encString totp: EncString; + @encrypted totp: EncString; autofillOnPageLoad: boolean; constructor(obj?: LoginData) { diff --git a/libs/common/src/models/domain/loginUri.ts b/libs/common/src/models/domain/loginUri.ts index 110afc2f4565..75b3defe8c5d 100644 --- a/libs/common/src/models/domain/loginUri.ts +++ b/libs/common/src/models/domain/loginUri.ts @@ -1,5 +1,5 @@ import { IDecryptable } from "@bitwarden/common/interfaces/IDecryptable"; -import { encString } from "@bitwarden/common/misc/decryptable.decorator"; +import { encrypted } from "@bitwarden/common/misc/encrypted.decorator"; import { UriMatchType } from "../../enums/uriMatchType"; import { LoginUriData } from "../data/loginUriData"; @@ -10,7 +10,7 @@ import { EncString } from "./encString"; import { SymmetricCryptoKey } from "./symmetricCryptoKey"; export class LoginUri extends Domain implements IDecryptable { - @encString + @encrypted uri: EncString; match: UriMatchType; diff --git a/libs/common/src/services/encrypt.service.ts b/libs/common/src/services/encrypt.service.ts index 2cdcaf42166a..69efb719d7ef 100644 --- a/libs/common/src/services/encrypt.service.ts +++ b/libs/common/src/services/encrypt.service.ts @@ -9,7 +9,7 @@ import { AbstractEncryptService } from "../abstractions/abstractEncrypt.service" import { EncryptionType } from "../enums/encryptionType"; import { IDecryptable } from "../interfaces/IDecryptable"; import { IEncrypted } from "../interfaces/IEncrypted"; -import { getEncStringList, getDecryptableList } from "../misc/decryptable.decorator"; +import { getEncryptedProperties } from "../misc/encrypted.decorator"; import { EncArrayBuffer } from "../models/domain/encArrayBuffer"; export class EncryptService implements AbstractEncryptService { @@ -174,34 +174,28 @@ export class EncryptService implements AbstractEncryptService { const promises: Promise[] = []; - // Decrypt all encStrings of object - // relies on each encString cacheing its result - is that a good pattern that we want to extend here? - const encStringProps = getEncStringList(item); - encStringProps?.forEach((prop) => { + // TODO: do not store decrypted value in domain object - copy to intermediate temp object and then pass to view + const encryptedProperties = getEncryptedProperties(item); + encryptedProperties?.forEach((prop) => { const propValue = (item as any)[prop]; if (propValue == null) { return; } - promises.push( - this.decryptToUtf8(propValue, key) - .then((decryptedValue) => ((item as any)[prop].decryptedValue = decryptedValue)) - .catch((e) => { - this.logService.error(e); - (item as any)[prop].decryptedValue = "[error: cannot decrypt]"; - }) - ); - }); - - // Decrypt all nested IDecryptables (recursive call) - const decryptableProps = getDecryptableList(item); - - decryptableProps?.forEach((prop) => { - const propValue = (item as any)[prop]; - if (propValue == null) { + if (propValue instanceof EncString) { + // decrypt the EncString + promises.push( + this.decryptToUtf8(propValue, key) + .then((decryptedValue) => ((item as any)[prop].decryptedValue = decryptedValue)) + .catch((e) => { + this.logService.error(e); + (item as any)[prop].decryptedValue = "[error: cannot decrypt]"; + }) + ); return; } + // else it's a nested object with EncStrings, recursive call if (propValue instanceof Array) { propValue.forEach((subItem) => promises.push(this.decryptItem(subItem, key))); } else { From 165267f1c3120beb55f01d1b8e1be26b5d15014c Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Mon, 3 Oct 2022 13:47:50 +1000 Subject: [PATCH 09/19] Do not store decrypted value in encString --- .../spec/misc/encrypted.decorator.spec.ts | 2 +- .../spec/services/encrypt.service.spec.ts | 140 ++++++++---------- libs/common/spec/services/encryptedObject.ts | 51 +++++++ libs/common/src/interfaces/IDecryptable.ts | 2 +- libs/common/src/models/domain/encString.ts | 6 +- libs/common/src/models/domain/login.ts | 7 +- libs/common/src/models/domain/loginUri.ts | 6 +- libs/common/src/services/encrypt.service.ts | 26 +++- 8 files changed, 148 insertions(+), 92 deletions(-) create mode 100644 libs/common/spec/services/encryptedObject.ts diff --git a/libs/common/spec/misc/encrypted.decorator.spec.ts b/libs/common/spec/misc/encrypted.decorator.spec.ts index 1837a81958e7..b7c877c6c52b 100644 --- a/libs/common/spec/misc/encrypted.decorator.spec.ts +++ b/libs/common/spec/misc/encrypted.decorator.spec.ts @@ -9,7 +9,7 @@ class TestClass { someOtherProperty: Date; } -describe("encString decorator", () => { +describe("encrypted decorator", () => { it("adds property name to list", () => { const testClass = new TestClass(); const result = getEncryptedProperties(testClass); diff --git a/libs/common/spec/services/encrypt.service.spec.ts b/libs/common/spec/services/encrypt.service.spec.ts index 76f5cf2f2598..c9833e9cd0fc 100644 --- a/libs/common/spec/services/encrypt.service.spec.ts +++ b/libs/common/spec/services/encrypt.service.spec.ts @@ -3,17 +3,18 @@ import { mockReset, mock } from "jest-mock-extended"; import { CryptoFunctionService } from "@bitwarden/common/abstractions/cryptoFunction.service"; import { LogService } from "@bitwarden/common/abstractions/log.service"; import { EncryptionType } from "@bitwarden/common/enums/encryptionType"; +import { UriMatchType } from "@bitwarden/common/enums/uriMatchType"; import { EncArrayBuffer } from "@bitwarden/common/models/domain/encArrayBuffer"; import { EncString } from "@bitwarden/common/models/domain/encString"; import { Login } from "@bitwarden/common/models/domain/login"; +import { LoginUri } from "@bitwarden/common/models/domain/loginUri"; import { SymmetricCryptoKey } from "@bitwarden/common/models/domain/symmetricCryptoKey"; -import { LoginView } from "@bitwarden/common/models/view/loginView"; +import { LoginUriView } from "@bitwarden/common/models/view/loginUriView"; import { EncryptService } from "@bitwarden/common/services/encrypt.service"; import { makeStaticByteArray } from "../utils"; -import { LoginUri } from "@bitwarden/common/models/domain/loginUri"; -import { LoginUriView } from "@bitwarden/common/models/view/loginUriView"; -import { UriMatchType } from "@bitwarden/common/enums/uriMatchType"; + +import { SimpleEncryptedObject, SimpleEncryptedObjectView } from "./encryptedObject"; describe("EncryptService", () => { const cryptoFunctionService = mock(); @@ -196,91 +197,78 @@ describe("EncryptService", () => { jest.clearAllMocks(); }); - it("decrypts encStrings on target object", async () => { - const date = new Date(); - const login = new Login(); - login.uris = null; - login.username = new EncString("3.myUsername_Encrypted"); - login.password = new EncString("3.myPassword_Encrypted"); - login.totp = new EncString("3.myTotp_Encrypted"); - login.passwordRevisionDate = date; - login.autofillOnPageLoad = true; - - jest - .spyOn(encryptService, "decryptToUtf8") - .mockImplementation((encString, key) => - Promise.resolve(encString.data.replace("_Encrypted", "")) - ); + describe("given an object with encStrings", () => { + beforeEach(() => { + jest + .spyOn(encryptService, "decryptToUtf8") + .mockImplementation((encString, key) => + Promise.resolve(encString.data.replace("_Encrypted", "")) + ); + }); - const result = await encryptService.decryptItem(login, mock()); + it("decrypts encStrings", async () => { + const target = new SimpleEncryptedObject(); - expect(result).toMatchObject({ - username: "myUsername", - password: "myPassword", - totp: "myTotp", - passwordRevisionDate: date, - autofillOnPageLoad: true, + const result = await encryptService.decryptItem(target, mock()); + + expect(result).toMatchObject({ + foo: "foo", + bar: "bar", + plainValue: 9000, + }); + expect(result).toBeInstanceOf(SimpleEncryptedObjectView); }); - expect(result).toBeInstanceOf(LoginView); }); - it("handles decryption errors in encStrings", async () => { - const date = new Date(); - const login = new Login(); - login.uris = null; - login.username = new EncString("3.myUsername_Encrypted"); - login.password = new EncString("3.myPassword_Encrypted"); - login.totp = new EncString("3.myTotp_Encrypted"); - login.passwordRevisionDate = date; - login.autofillOnPageLoad = true; + describe("given an object with nested IDecryptables", () => { + it("decrypts an array of Decryptables", async () => { + const uri1 = new LoginUri(); + uri1.uri = new EncString("3.someUri_Encrypted"); + uri1.match = UriMatchType.Domain; + + const uri2 = new LoginUri(); + uri2.uri = new EncString("3.anotherUri_Encrypted"); + uri2.match = UriMatchType.Host; + + const login = new Login(); + login.uris = [uri1, uri2]; + + jest + .spyOn(encryptService, "decryptToUtf8") + .mockImplementation((encString, key) => + Promise.resolve(encString.data.replace("_Encrypted", "")) + ); + + const result = await encryptService.decryptItem(login, mock()); + + expect(result.uris[0]).toMatchObject({ + uri: "someUri", + match: UriMatchType.Domain, + }); + expect(result.uris[0]).toBeInstanceOf(LoginUriView); + + expect(result.uris[1]).toMatchObject({ + uri: "anotherUri", + match: UriMatchType.Host, + }); + expect(result.uris[1]).toBeInstanceOf(LoginUriView); + }); + }); + it("handles decryption errors", async () => { + const target = new SimpleEncryptedObject(); jest.spyOn(encryptService, "decryptToUtf8").mockRejectedValue("some decryption error"); - const result = await encryptService.decryptItem(login, mock()); + const result = await encryptService.decryptItem(target, mock()); const decryptionError = "[error: cannot decrypt]"; expect(result).toMatchObject({ - username: decryptionError, - password: decryptionError, - totp: decryptionError, - passwordRevisionDate: date, - autofillOnPageLoad: true, - }); - expect(result).toBeInstanceOf(LoginView); - }); - - it("decrypts nested IDecryptables", async () => { - const uri1 = new LoginUri(); - uri1.uri = new EncString("3.someUri_Encrypted"); - uri1.match = UriMatchType.Domain; - - const uri2 = new LoginUri(); - uri2.uri = new EncString("3.anotherUri_Encrypted"); - uri2.match = UriMatchType.Host; - - const login = new Login(); - login.uris = [uri1, uri2]; - - jest - .spyOn(encryptService, "decryptToUtf8") - .mockImplementation((encString, key) => - Promise.resolve(encString.data.replace("_Encrypted", "")) - ); - - const result = await encryptService.decryptItem(login, mock()); - - expect(result.uris[0]).toMatchObject({ - uri: "someUri", - match: UriMatchType.Domain, - }); - expect(result.uris[0]).toBeInstanceOf(LoginUriView); - - expect(result.uris[1]).toMatchObject({ - uri: "anotherUri", - match: UriMatchType.Host, + foo: decryptionError, + bar: decryptionError, + plainValue: 9000, }); - expect(result.uris[1]).toBeInstanceOf(LoginUriView); + expect(result).toBeInstanceOf(SimpleEncryptedObjectView); }); }); }); diff --git a/libs/common/spec/services/encryptedObject.ts b/libs/common/spec/services/encryptedObject.ts new file mode 100644 index 000000000000..596474b5d0c6 --- /dev/null +++ b/libs/common/spec/services/encryptedObject.ts @@ -0,0 +1,51 @@ +import { mock } from "jest-mock-extended"; + +import { IDecryptable } from "../../src/interfaces/IDecryptable"; +import { encrypted } from "../../src/misc/encrypted.decorator"; +import { EncString } from "../../src/models/domain/encString"; + +export class SimpleEncryptedObjectView { + foo: string; + bar: string; + plainValue: number; +} + +export class SimpleEncryptedObject implements IDecryptable { + @encrypted foo = new EncString("3.foo_Encrypted"); + @encrypted bar = new EncString("3.bar_Encrypted"); + plainValue = 9000; + + toView(decryptedProperties: any) { + return Object.assign( + new SimpleEncryptedObjectView(), + { + // Manually copy over unencrypted values + plainValue: this.plainValue, + }, + decryptedProperties // Assign everything else from the decryptedProperties object + ); + } +} + +export class SimpleEncryptedObjectArray implements IDecryptable { + @encrypted baz: EncString[] = [ + new EncString("3.ArrayItem1_Encrypted", "3.ArrayItem2_Encrypted", "3.ArrayItem3_Encrypted"), + ]; + + toView(decryptedProperties: any) { + return Object.assign(new SimpleEncryptedObjectArrayView(), decryptedProperties); + } +} + +export class SimpleEncryptedObjectArrayView extends SimpleEncryptedObjectView { + baz: string[]; +} + +export class NestedEncryptedObject implements IDecryptable { + @encrypted username = mock(); + @encrypted password = mock(); + + toView(decryptedProperties: any) { + return; + } +} diff --git a/libs/common/src/interfaces/IDecryptable.ts b/libs/common/src/interfaces/IDecryptable.ts index 456c7b266392..6a45a710fa98 100644 --- a/libs/common/src/interfaces/IDecryptable.ts +++ b/libs/common/src/interfaces/IDecryptable.ts @@ -1,3 +1,3 @@ export interface IDecryptable { - toView: () => T; + toView: (decryptedProperties: any) => T; } diff --git a/libs/common/src/models/domain/encString.ts b/libs/common/src/models/domain/encString.ts index 01376c90936d..c2efafa363e7 100644 --- a/libs/common/src/models/domain/encString.ts +++ b/libs/common/src/models/domain/encString.ts @@ -10,11 +10,15 @@ import { SymmetricCryptoKey } from "./symmetricCryptoKey"; export class EncString implements IEncrypted { encryptedString?: string; encryptionType?: EncryptionType; - decryptedValue?: string; data?: string; iv?: string; mac?: string; + /** + * @deprecated: EncStrings no longer store their decrypted values as this risks leaking data + */ + private decryptedValue?: string; + constructor( encryptedStringOrType: string | EncryptionType, data?: string, diff --git a/libs/common/src/models/domain/login.ts b/libs/common/src/models/domain/login.ts index 866c46464d0c..1f4854f3d336 100644 --- a/libs/common/src/models/domain/login.ts +++ b/libs/common/src/models/domain/login.ts @@ -89,7 +89,7 @@ export class Login extends Domain implements IDecryptable { return l; } - toView() { + toView(decryptedProperties: any) { const view = new LoginView(); // Unencrypted properties @@ -97,10 +97,7 @@ export class Login extends Domain implements IDecryptable { view.passwordRevisionDate = this.passwordRevisionDate; // Encrypted properties - view.username = this.username?.decryptedValue; - view.password = this.password?.decryptedValue; - view.totp = this.totp?.decryptedValue; - view.uris = this.uris?.map((uri) => uri.toView()); + Object.assign(view, decryptedProperties); return view; } diff --git a/libs/common/src/models/domain/loginUri.ts b/libs/common/src/models/domain/loginUri.ts index 75b3defe8c5d..bf45e347751d 100644 --- a/libs/common/src/models/domain/loginUri.ts +++ b/libs/common/src/models/domain/loginUri.ts @@ -56,10 +56,12 @@ export class LoginUri extends Domain implements IDecryptable { return u; } - toView() { + toView(decryptedProperties: any) { const view = new LoginUriView(); - view.uri = this.uri.decryptedValue; view.match = this.match; + + Object.assign(view, decryptedProperties); + return view; } } diff --git a/libs/common/src/services/encrypt.service.ts b/libs/common/src/services/encrypt.service.ts index 69efb719d7ef..a62177795a44 100644 --- a/libs/common/src/services/encrypt.service.ts +++ b/libs/common/src/services/encrypt.service.ts @@ -11,6 +11,7 @@ import { IDecryptable } from "../interfaces/IDecryptable"; import { IEncrypted } from "../interfaces/IEncrypted"; import { getEncryptedProperties } from "../misc/encrypted.decorator"; import { EncArrayBuffer } from "../models/domain/encArrayBuffer"; +import { View } from "../models/view/view"; export class EncryptService implements AbstractEncryptService { constructor( @@ -173,8 +174,8 @@ export class EncryptService implements AbstractEncryptService { } const promises: Promise[] = []; + const decryptedValues: Record | Array> = {}; - // TODO: do not store decrypted value in domain object - copy to intermediate temp object and then pass to view const encryptedProperties = getEncryptedProperties(item); encryptedProperties?.forEach((prop) => { const propValue = (item as any)[prop]; @@ -186,26 +187,39 @@ export class EncryptService implements AbstractEncryptService { // decrypt the EncString promises.push( this.decryptToUtf8(propValue, key) - .then((decryptedValue) => ((item as any)[prop].decryptedValue = decryptedValue)) + .then((decryptedValue) => (decryptedValues[prop] = decryptedValue)) .catch((e) => { this.logService.error(e); - (item as any)[prop].decryptedValue = "[error: cannot decrypt]"; + decryptedValues[prop] = "[error: cannot decrypt]"; }) ); return; } // else it's a nested object with EncStrings, recursive call + // TODO: what about an array of encstrings? if (propValue instanceof Array) { - propValue.forEach((subItem) => promises.push(this.decryptItem(subItem, key))); + decryptedValues[prop] = []; + + propValue.forEach((subItem) => + promises.push( + this.decryptItem(subItem, key).then((decryptedValue) => + (decryptedValues[prop] as Array).push(decryptedValue) + ) + ) + ); } else { - promises.push(this.decryptItem(propValue, key)); + promises.push( + this.decryptItem(propValue, key).then( + (decryptedValue) => (decryptedValues[prop] = decryptedValue) + ) + ); } }); await Promise.all(promises); - return item.toView(); + return item.toView(decryptedValues); } private logMacFailed(msg: string) { From b609826d5b306efa75b7db54a8d5c329b4d87e7e Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Mon, 3 Oct 2022 15:00:15 +1000 Subject: [PATCH 10/19] Rearrange tests --- libs/common/spec/services/encrypt.service.spec.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/libs/common/spec/services/encrypt.service.spec.ts b/libs/common/spec/services/encrypt.service.spec.ts index c9833e9cd0fc..036156d42d99 100644 --- a/libs/common/spec/services/encrypt.service.spec.ts +++ b/libs/common/spec/services/encrypt.service.spec.ts @@ -197,7 +197,7 @@ describe("EncryptService", () => { jest.clearAllMocks(); }); - describe("given an object with encStrings", () => { + describe("decrypts", () => { beforeEach(() => { jest .spyOn(encryptService, "decryptToUtf8") @@ -206,7 +206,7 @@ describe("EncryptService", () => { ); }); - it("decrypts encStrings", async () => { + it("encStrings", async () => { const target = new SimpleEncryptedObject(); const result = await encryptService.decryptItem(target, mock()); @@ -218,10 +218,10 @@ describe("EncryptService", () => { }); expect(result).toBeInstanceOf(SimpleEncryptedObjectView); }); - }); - describe("given an object with nested IDecryptables", () => { - it("decrypts an array of Decryptables", async () => { + it.todo("nested IDecryptables"); + + it("an array of nested IDecryptables", async () => { const uri1 = new LoginUri(); uri1.uri = new EncString("3.someUri_Encrypted"); uri1.match = UriMatchType.Domain; From 51ce66ac1c79d25776813599ecd8506081b6a03f Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Thu, 6 Oct 2022 09:19:45 +1000 Subject: [PATCH 11/19] Rewrite test encrypted objects --- .../spec/services/encrypt.service.spec.ts | 12 ++-- libs/common/spec/services/encryptedObject.ts | 63 ++++++++++++------- 2 files changed, 48 insertions(+), 27 deletions(-) diff --git a/libs/common/spec/services/encrypt.service.spec.ts b/libs/common/spec/services/encrypt.service.spec.ts index 036156d42d99..35a3dd56c0bc 100644 --- a/libs/common/spec/services/encrypt.service.spec.ts +++ b/libs/common/spec/services/encrypt.service.spec.ts @@ -212,9 +212,9 @@ describe("EncryptService", () => { const result = await encryptService.decryptItem(target, mock()); expect(result).toMatchObject({ - foo: "foo", - bar: "bar", - plainValue: 9000, + username: "myUsername", + password: "myPassword", + accessCount: 9000, }); expect(result).toBeInstanceOf(SimpleEncryptedObjectView); }); @@ -264,9 +264,9 @@ describe("EncryptService", () => { const decryptionError = "[error: cannot decrypt]"; expect(result).toMatchObject({ - foo: decryptionError, - bar: decryptionError, - plainValue: 9000, + username: decryptionError, + password: decryptionError, + accessCount: 9000, }); expect(result).toBeInstanceOf(SimpleEncryptedObjectView); }); diff --git a/libs/common/spec/services/encryptedObject.ts b/libs/common/spec/services/encryptedObject.ts index 596474b5d0c6..b46593fa4120 100644 --- a/libs/common/spec/services/encryptedObject.ts +++ b/libs/common/spec/services/encryptedObject.ts @@ -1,51 +1,72 @@ -import { mock } from "jest-mock-extended"; - import { IDecryptable } from "../../src/interfaces/IDecryptable"; import { encrypted } from "../../src/misc/encrypted.decorator"; import { EncString } from "../../src/models/domain/encString"; export class SimpleEncryptedObjectView { - foo: string; - bar: string; - plainValue: number; + username: string; + password: string; + accessCount: number; } +/** + * An object with EncStrings and non-encrypted fields. + */ export class SimpleEncryptedObject implements IDecryptable { - @encrypted foo = new EncString("3.foo_Encrypted"); - @encrypted bar = new EncString("3.bar_Encrypted"); - plainValue = 9000; + @encrypted username = new EncString("3.myUsername_Encrypted"); + @encrypted password = new EncString("3.myPassword_Encrypted"); + accessCount = 9000; toView(decryptedProperties: any) { return Object.assign( new SimpleEncryptedObjectView(), { // Manually copy over unencrypted values - plainValue: this.plainValue, + accessCount: this.accessCount, }, decryptedProperties // Assign everything else from the decryptedProperties object ); } } -export class SimpleEncryptedObjectArray implements IDecryptable { - @encrypted baz: EncString[] = [ - new EncString("3.ArrayItem1_Encrypted", "3.ArrayItem2_Encrypted", "3.ArrayItem3_Encrypted"), - ]; +export class NestedEncryptedObjectView {} + +/** + * An object with nested encrypted objects (i.e. objects containing EncStrings) + */ +export class NestedEncryptedObject implements IDecryptable { + @encrypted nestedLogin1 = new SimpleEncryptedObject(); + @encrypted nestedLogin2 = new SimpleEncryptedObject(); + collectionId = "myCollectionId"; toView(decryptedProperties: any) { - return Object.assign(new SimpleEncryptedObjectArrayView(), decryptedProperties); + return Object.assign( + new NestedArrayEncryptedObjectView(), + { + // Manually copy over unencrypted values + collectionId: this.collectionId, + }, + decryptedProperties // Assign everything else from the decryptedProperties object + ); } } -export class SimpleEncryptedObjectArrayView extends SimpleEncryptedObjectView { - baz: string[]; -} +export class NestedArrayEncryptedObjectView {} -export class NestedEncryptedObject implements IDecryptable { - @encrypted username = mock(); - @encrypted password = mock(); +/** + * An object with nested encrypted objects (i.e. objects containing EncStrings) in an array + */ +export class NestedArrayEncryptedObject implements IDecryptable { + @encrypted logins = [new SimpleEncryptedObject(), new SimpleEncryptedObject()]; + collectionId = "myCollectionId"; toView(decryptedProperties: any) { - return; + return Object.assign( + new NestedArrayEncryptedObjectView(), + { + // Manually copy over unencrypted values + collectionId: this.collectionId, + }, + decryptedProperties // Assign everything else from the decryptedProperties object + ); } } From 433094afd6a994bde21156542c1b36131a448c63 Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Thu, 6 Oct 2022 09:24:35 +1000 Subject: [PATCH 12/19] Test for nested IDecryptables --- .../spec/services/encrypt.service.spec.ts | 34 +++++++++++++++---- 1 file changed, 27 insertions(+), 7 deletions(-) diff --git a/libs/common/spec/services/encrypt.service.spec.ts b/libs/common/spec/services/encrypt.service.spec.ts index 35a3dd56c0bc..ac1bf0f57581 100644 --- a/libs/common/spec/services/encrypt.service.spec.ts +++ b/libs/common/spec/services/encrypt.service.spec.ts @@ -14,7 +14,12 @@ import { EncryptService } from "@bitwarden/common/services/encrypt.service"; import { makeStaticByteArray } from "../utils"; -import { SimpleEncryptedObject, SimpleEncryptedObjectView } from "./encryptedObject"; +import { + NestedArrayEncryptedObjectView, + NestedEncryptedObject, + SimpleEncryptedObject, + SimpleEncryptedObjectView, +} from "./encryptedObject"; describe("EncryptService", () => { const cryptoFunctionService = mock(); @@ -198,6 +203,12 @@ describe("EncryptService", () => { }); describe("decrypts", () => { + const simpleObjectDecrypted = { + username: "myUsername", + password: "myPassword", + accessCount: 9000, + }; + beforeEach(() => { jest .spyOn(encryptService, "decryptToUtf8") @@ -211,15 +222,24 @@ describe("EncryptService", () => { const result = await encryptService.decryptItem(target, mock()); - expect(result).toMatchObject({ - username: "myUsername", - password: "myPassword", - accessCount: 9000, - }); + expect(result).toMatchObject(simpleObjectDecrypted); expect(result).toBeInstanceOf(SimpleEncryptedObjectView); }); - it.todo("nested IDecryptables"); + it("nested IDecryptables", async () => { + const target = new NestedEncryptedObject(); + + const result = await encryptService.decryptItem(target, mock()); + + expect(result).toMatchObject({ + nestedLogin1: simpleObjectDecrypted, + nestedLogin2: simpleObjectDecrypted, + collectionId: "myCollectionId", + }); + expect(result).toBeInstanceOf(NestedArrayEncryptedObjectView); + expect(result.nestedLogin1).toBeInstanceOf(SimpleEncryptedObjectView); + expect(result.nestedLogin2).toBeInstanceOf(SimpleEncryptedObjectView); + }); it("an array of nested IDecryptables", async () => { const uri1 = new LoginUri(); From c1ae51ac2d19dbe38b8508154cac48e29a90ecb3 Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Thu, 6 Oct 2022 09:30:57 +1000 Subject: [PATCH 13/19] Rewrite tests using test classes --- .../spec/services/encrypt.service.spec.ts | 42 +++++-------------- libs/common/spec/services/encryptedObject.ts | 2 +- 2 files changed, 12 insertions(+), 32 deletions(-) diff --git a/libs/common/spec/services/encrypt.service.spec.ts b/libs/common/spec/services/encrypt.service.spec.ts index ac1bf0f57581..6be351a89489 100644 --- a/libs/common/spec/services/encrypt.service.spec.ts +++ b/libs/common/spec/services/encrypt.service.spec.ts @@ -3,20 +3,18 @@ import { mockReset, mock } from "jest-mock-extended"; import { CryptoFunctionService } from "@bitwarden/common/abstractions/cryptoFunction.service"; import { LogService } from "@bitwarden/common/abstractions/log.service"; import { EncryptionType } from "@bitwarden/common/enums/encryptionType"; -import { UriMatchType } from "@bitwarden/common/enums/uriMatchType"; import { EncArrayBuffer } from "@bitwarden/common/models/domain/encArrayBuffer"; import { EncString } from "@bitwarden/common/models/domain/encString"; -import { Login } from "@bitwarden/common/models/domain/login"; -import { LoginUri } from "@bitwarden/common/models/domain/loginUri"; import { SymmetricCryptoKey } from "@bitwarden/common/models/domain/symmetricCryptoKey"; -import { LoginUriView } from "@bitwarden/common/models/view/loginUriView"; import { EncryptService } from "@bitwarden/common/services/encrypt.service"; import { makeStaticByteArray } from "../utils"; import { + NestedArrayEncryptedObject, NestedArrayEncryptedObjectView, NestedEncryptedObject, + NestedEncryptedObjectView, SimpleEncryptedObject, SimpleEncryptedObjectView, } from "./encryptedObject"; @@ -236,42 +234,24 @@ describe("EncryptService", () => { nestedLogin2: simpleObjectDecrypted, collectionId: "myCollectionId", }); - expect(result).toBeInstanceOf(NestedArrayEncryptedObjectView); + expect(result).toBeInstanceOf(NestedEncryptedObjectView); expect(result.nestedLogin1).toBeInstanceOf(SimpleEncryptedObjectView); expect(result.nestedLogin2).toBeInstanceOf(SimpleEncryptedObjectView); }); it("an array of nested IDecryptables", async () => { - const uri1 = new LoginUri(); - uri1.uri = new EncString("3.someUri_Encrypted"); - uri1.match = UriMatchType.Domain; - - const uri2 = new LoginUri(); - uri2.uri = new EncString("3.anotherUri_Encrypted"); - uri2.match = UriMatchType.Host; - - const login = new Login(); - login.uris = [uri1, uri2]; - - jest - .spyOn(encryptService, "decryptToUtf8") - .mockImplementation((encString, key) => - Promise.resolve(encString.data.replace("_Encrypted", "")) - ); + const target = new NestedArrayEncryptedObject(); - const result = await encryptService.decryptItem(login, mock()); + const result = await encryptService.decryptItem(target, mock()); - expect(result.uris[0]).toMatchObject({ - uri: "someUri", - match: UriMatchType.Domain, + expect(result).toMatchObject({ + logins: [simpleObjectDecrypted, simpleObjectDecrypted], + collectionId: "myCollectionId", }); - expect(result.uris[0]).toBeInstanceOf(LoginUriView); - expect(result.uris[1]).toMatchObject({ - uri: "anotherUri", - match: UriMatchType.Host, - }); - expect(result.uris[1]).toBeInstanceOf(LoginUriView); + expect(result).toBeInstanceOf(NestedArrayEncryptedObjectView); + expect(result.logins[0]).toBeInstanceOf(SimpleEncryptedObjectView); + expect(result.logins[1]).toBeInstanceOf(SimpleEncryptedObjectView); }); }); diff --git a/libs/common/spec/services/encryptedObject.ts b/libs/common/spec/services/encryptedObject.ts index b46593fa4120..8b2f4f25e00e 100644 --- a/libs/common/spec/services/encryptedObject.ts +++ b/libs/common/spec/services/encryptedObject.ts @@ -40,7 +40,7 @@ export class NestedEncryptedObject implements IDecryptable Date: Thu, 6 Oct 2022 09:36:55 +1000 Subject: [PATCH 14/19] Use id to disambiguate test objects --- .../spec/services/encrypt.service.spec.ts | 43 +++++++++++++------ libs/common/spec/services/encryptedObject.ts | 17 ++++---- 2 files changed, 39 insertions(+), 21 deletions(-) diff --git a/libs/common/spec/services/encrypt.service.spec.ts b/libs/common/spec/services/encrypt.service.spec.ts index 6be351a89489..7a957750ba13 100644 --- a/libs/common/spec/services/encrypt.service.spec.ts +++ b/libs/common/spec/services/encrypt.service.spec.ts @@ -201,12 +201,6 @@ describe("EncryptService", () => { }); describe("decrypts", () => { - const simpleObjectDecrypted = { - username: "myUsername", - password: "myPassword", - accessCount: 9000, - }; - beforeEach(() => { jest .spyOn(encryptService, "decryptToUtf8") @@ -216,11 +210,15 @@ describe("EncryptService", () => { }); it("encStrings", async () => { - const target = new SimpleEncryptedObject(); + const target = new SimpleEncryptedObject(1); const result = await encryptService.decryptItem(target, mock()); - expect(result).toMatchObject(simpleObjectDecrypted); + expect(result).toMatchObject({ + id: 1, + username: "myUsername1", + password: "myPassword1", + }); expect(result).toBeInstanceOf(SimpleEncryptedObjectView); }); @@ -230,8 +228,16 @@ describe("EncryptService", () => { const result = await encryptService.decryptItem(target, mock()); expect(result).toMatchObject({ - nestedLogin1: simpleObjectDecrypted, - nestedLogin2: simpleObjectDecrypted, + nestedLogin1: { + id: 1, + username: "myUsername1", + password: "myPassword1", + }, + nestedLogin2: { + id: 2, + username: "myUsername2", + password: "myPassword2", + }, collectionId: "myCollectionId", }); expect(result).toBeInstanceOf(NestedEncryptedObjectView); @@ -245,7 +251,18 @@ describe("EncryptService", () => { const result = await encryptService.decryptItem(target, mock()); expect(result).toMatchObject({ - logins: [simpleObjectDecrypted, simpleObjectDecrypted], + logins: [ + { + id: 1, + username: "myUsername1", + password: "myPassword1", + }, + { + id: 2, + username: "myUsername2", + password: "myPassword2", + }, + ], collectionId: "myCollectionId", }); @@ -256,7 +273,7 @@ describe("EncryptService", () => { }); it("handles decryption errors", async () => { - const target = new SimpleEncryptedObject(); + const target = new SimpleEncryptedObject(1); jest.spyOn(encryptService, "decryptToUtf8").mockRejectedValue("some decryption error"); const result = await encryptService.decryptItem(target, mock()); @@ -264,9 +281,9 @@ describe("EncryptService", () => { const decryptionError = "[error: cannot decrypt]"; expect(result).toMatchObject({ + id: 1, username: decryptionError, password: decryptionError, - accessCount: 9000, }); expect(result).toBeInstanceOf(SimpleEncryptedObjectView); }); diff --git a/libs/common/spec/services/encryptedObject.ts b/libs/common/spec/services/encryptedObject.ts index 8b2f4f25e00e..cb4dbfe98790 100644 --- a/libs/common/spec/services/encryptedObject.ts +++ b/libs/common/spec/services/encryptedObject.ts @@ -3,25 +3,26 @@ import { encrypted } from "../../src/misc/encrypted.decorator"; import { EncString } from "../../src/models/domain/encString"; export class SimpleEncryptedObjectView { + id: number; username: string; password: string; - accessCount: number; } /** * An object with EncStrings and non-encrypted fields. */ export class SimpleEncryptedObject implements IDecryptable { - @encrypted username = new EncString("3.myUsername_Encrypted"); - @encrypted password = new EncString("3.myPassword_Encrypted"); - accessCount = 9000; + @encrypted username = new EncString("3.myUsername" + this.id + "_Encrypted"); + @encrypted password = new EncString("3.myPassword" + this.id + "_Encrypted"); + + constructor(public id: number) {} toView(decryptedProperties: any) { return Object.assign( new SimpleEncryptedObjectView(), { // Manually copy over unencrypted values - accessCount: this.accessCount, + id: this.id, }, decryptedProperties // Assign everything else from the decryptedProperties object ); @@ -34,8 +35,8 @@ export class NestedEncryptedObjectView {} * An object with nested encrypted objects (i.e. objects containing EncStrings) */ export class NestedEncryptedObject implements IDecryptable { - @encrypted nestedLogin1 = new SimpleEncryptedObject(); - @encrypted nestedLogin2 = new SimpleEncryptedObject(); + @encrypted nestedLogin1 = new SimpleEncryptedObject(1); + @encrypted nestedLogin2 = new SimpleEncryptedObject(2); collectionId = "myCollectionId"; toView(decryptedProperties: any) { @@ -56,7 +57,7 @@ export class NestedArrayEncryptedObjectView {} * An object with nested encrypted objects (i.e. objects containing EncStrings) in an array */ export class NestedArrayEncryptedObject implements IDecryptable { - @encrypted logins = [new SimpleEncryptedObject(), new SimpleEncryptedObject()]; + @encrypted logins = [new SimpleEncryptedObject(1), new SimpleEncryptedObject(2)]; collectionId = "myCollectionId"; toView(decryptedProperties: any) { From 17126063824f542b71cd549bce53a83ac253f7e4 Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Thu, 6 Oct 2022 09:42:10 +1000 Subject: [PATCH 15/19] Minor typing and comment changes --- libs/common/src/services/encrypt.service.ts | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/libs/common/src/services/encrypt.service.ts b/libs/common/src/services/encrypt.service.ts index a62177795a44..4304933b93e1 100644 --- a/libs/common/src/services/encrypt.service.ts +++ b/libs/common/src/services/encrypt.service.ts @@ -168,13 +168,13 @@ export class EncryptService implements AbstractEncryptService { return obj; } - async decryptItem(item: IDecryptable, key: SymmetricCryptoKey) { + async decryptItem(item: IDecryptable, key: SymmetricCryptoKey): Promise { if (item == null) { throw new Error("Cannot decrypt a null item"); } const promises: Promise[] = []; - const decryptedValues: Record | Array> = {}; + const decryptedValues: Record> = {}; const encryptedProperties = getEncryptedProperties(item); encryptedProperties?.forEach((prop) => { @@ -183,8 +183,8 @@ export class EncryptService implements AbstractEncryptService { return; } + // Case 1: the property is an EncString if (propValue instanceof EncString) { - // decrypt the EncString promises.push( this.decryptToUtf8(propValue, key) .then((decryptedValue) => (decryptedValues[prop] = decryptedValue)) @@ -196,8 +196,8 @@ export class EncryptService implements AbstractEncryptService { return; } - // else it's a nested object with EncStrings, recursive call - // TODO: what about an array of encstrings? + // Case 2: the property is an array of nested decryptables + // NB: arrays of EncStrings are not supported at this time (no current use cases) if (propValue instanceof Array) { decryptedValues[prop] = []; @@ -209,6 +209,7 @@ export class EncryptService implements AbstractEncryptService { ) ); } else { + // Case 3: the property is a single nested decryptable promises.push( this.decryptItem(propValue, key).then( (decryptedValue) => (decryptedValues[prop] = decryptedValue) From 1698dec7bf104534941750e34e6714142f3e81f9 Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Tue, 11 Oct 2022 09:34:21 +1000 Subject: [PATCH 16/19] Revert view model changes --- libs/common/src/models/domain/login.ts | 25 +++++------------------ libs/common/src/models/domain/loginUri.ts | 14 +------------ 2 files changed, 6 insertions(+), 33 deletions(-) diff --git a/libs/common/src/models/domain/login.ts b/libs/common/src/models/domain/login.ts index 413a2d235687..19b99e956af9 100644 --- a/libs/common/src/models/domain/login.ts +++ b/libs/common/src/models/domain/login.ts @@ -1,7 +1,5 @@ import { Jsonify } from "type-fest"; -import { IDecryptable } from "../../interfaces/IDecryptable"; -import { encrypted } from "../../misc/encrypted.decorator"; import { LoginData } from "../data/loginData"; import { LoginView } from "../view/loginView"; @@ -10,12 +8,12 @@ import { EncString } from "./encString"; import { LoginUri } from "./loginUri"; import { SymmetricCryptoKey } from "./symmetricCryptoKey"; -export class Login extends Domain implements IDecryptable { - @encrypted uris: LoginUri[]; - @encrypted username: EncString; - @encrypted password: EncString; +export class Login extends Domain { + uris: LoginUri[]; + username: EncString; + password: EncString; passwordRevisionDate?: Date; - @encrypted totp: EncString; + totp: EncString; autofillOnPageLoad: boolean; constructor(obj?: LoginData) { @@ -90,19 +88,6 @@ export class Login extends Domain implements IDecryptable { return l; } - toView(decryptedProperties: any) { - const view = new LoginView(); - - // Unencrypted properties - view.autofillOnPageLoad = this.autofillOnPageLoad; - view.passwordRevisionDate = this.passwordRevisionDate; - - // Encrypted properties - Object.assign(view, decryptedProperties); - - return view; - } - static fromJSON(obj: Partial>): Login { if (obj == null) { return null; diff --git a/libs/common/src/models/domain/loginUri.ts b/libs/common/src/models/domain/loginUri.ts index 528e17489cf5..419268cd514f 100644 --- a/libs/common/src/models/domain/loginUri.ts +++ b/libs/common/src/models/domain/loginUri.ts @@ -1,8 +1,6 @@ import { Jsonify } from "type-fest"; import { UriMatchType } from "../../enums/uriMatchType"; -import { IDecryptable } from "../../interfaces/IDecryptable"; -import { encrypted } from "../../misc/encrypted.decorator"; import { LoginUriData } from "../data/loginUriData"; import { LoginUriView } from "../view/loginUriView"; @@ -10,8 +8,7 @@ import Domain from "./domainBase"; import { EncString } from "./encString"; import { SymmetricCryptoKey } from "./symmetricCryptoKey"; -export class LoginUri extends Domain implements IDecryptable { - @encrypted +export class LoginUri extends Domain { uri: EncString; match: UriMatchType; @@ -57,15 +54,6 @@ export class LoginUri extends Domain implements IDecryptable { return u; } - toView(decryptedProperties: any) { - const view = new LoginUriView(); - view.match = this.match; - - Object.assign(view, decryptedProperties); - - return view; - } - static fromJSON(obj: Jsonify): LoginUri { if (obj == null) { return null; From d79c8b67011d165c1a5b960370e047364faf2058 Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Tue, 11 Oct 2022 09:54:31 +1000 Subject: [PATCH 17/19] Rename Decryptable interface --- libs/common/spec/services/encryptedObject.ts | 8 ++++---- libs/common/src/abstractions/abstractEncrypt.service.ts | 4 ++-- .../{IDecryptable.ts => decryptable.interface.ts} | 2 +- libs/common/src/services/encrypt.service.ts | 4 ++-- 4 files changed, 9 insertions(+), 9 deletions(-) rename libs/common/src/interfaces/{IDecryptable.ts => decryptable.interface.ts} (56%) diff --git a/libs/common/spec/services/encryptedObject.ts b/libs/common/spec/services/encryptedObject.ts index cb4dbfe98790..551699763fd7 100644 --- a/libs/common/spec/services/encryptedObject.ts +++ b/libs/common/spec/services/encryptedObject.ts @@ -1,4 +1,4 @@ -import { IDecryptable } from "../../src/interfaces/IDecryptable"; +import { Decryptable } from "../../src/interfaces/decryptable.interface"; import { encrypted } from "../../src/misc/encrypted.decorator"; import { EncString } from "../../src/models/domain/encString"; @@ -11,7 +11,7 @@ export class SimpleEncryptedObjectView { /** * An object with EncStrings and non-encrypted fields. */ -export class SimpleEncryptedObject implements IDecryptable { +export class SimpleEncryptedObject implements Decryptable { @encrypted username = new EncString("3.myUsername" + this.id + "_Encrypted"); @encrypted password = new EncString("3.myPassword" + this.id + "_Encrypted"); @@ -34,7 +34,7 @@ export class NestedEncryptedObjectView {} /** * An object with nested encrypted objects (i.e. objects containing EncStrings) */ -export class NestedEncryptedObject implements IDecryptable { +export class NestedEncryptedObject implements Decryptable { @encrypted nestedLogin1 = new SimpleEncryptedObject(1); @encrypted nestedLogin2 = new SimpleEncryptedObject(2); collectionId = "myCollectionId"; @@ -56,7 +56,7 @@ export class NestedArrayEncryptedObjectView {} /** * An object with nested encrypted objects (i.e. objects containing EncStrings) in an array */ -export class NestedArrayEncryptedObject implements IDecryptable { +export class NestedArrayEncryptedObject implements Decryptable { @encrypted logins = [new SimpleEncryptedObject(1), new SimpleEncryptedObject(2)]; collectionId = "myCollectionId"; diff --git a/libs/common/src/abstractions/abstractEncrypt.service.ts b/libs/common/src/abstractions/abstractEncrypt.service.ts index f34980568785..da5706a109f0 100644 --- a/libs/common/src/abstractions/abstractEncrypt.service.ts +++ b/libs/common/src/abstractions/abstractEncrypt.service.ts @@ -1,5 +1,5 @@ -import { IDecryptable } from "../interfaces/IDecryptable"; import { IEncrypted } from "../interfaces/IEncrypted"; +import { Decryptable } from "../interfaces/decryptable.interface"; import { EncArrayBuffer } from "../models/domain/encArrayBuffer"; import { EncString } from "../models/domain/encString"; import { SymmetricCryptoKey } from "../models/domain/symmetricCryptoKey"; @@ -13,5 +13,5 @@ export abstract class AbstractEncryptService { abstract decryptToUtf8: (encString: EncString, key: SymmetricCryptoKey) => Promise; abstract decryptToBytes: (encThing: IEncrypted, key: SymmetricCryptoKey) => Promise; abstract resolveLegacyKey: (key: SymmetricCryptoKey, encThing: IEncrypted) => SymmetricCryptoKey; - abstract decryptItem: (item: IDecryptable, key: SymmetricCryptoKey) => Promise; + abstract decryptItem: (item: Decryptable, key: SymmetricCryptoKey) => Promise; } diff --git a/libs/common/src/interfaces/IDecryptable.ts b/libs/common/src/interfaces/decryptable.interface.ts similarity index 56% rename from libs/common/src/interfaces/IDecryptable.ts rename to libs/common/src/interfaces/decryptable.interface.ts index 6a45a710fa98..7123e4ececab 100644 --- a/libs/common/src/interfaces/IDecryptable.ts +++ b/libs/common/src/interfaces/decryptable.interface.ts @@ -1,3 +1,3 @@ -export interface IDecryptable { +export interface Decryptable { toView: (decryptedProperties: any) => T; } diff --git a/libs/common/src/services/encrypt.service.ts b/libs/common/src/services/encrypt.service.ts index 5e2eddff8fcd..afb35bf050ad 100644 --- a/libs/common/src/services/encrypt.service.ts +++ b/libs/common/src/services/encrypt.service.ts @@ -2,8 +2,8 @@ import { AbstractEncryptService } from "../abstractions/abstractEncrypt.service" import { CryptoFunctionService } from "../abstractions/cryptoFunction.service"; import { LogService } from "../abstractions/log.service"; import { EncryptionType } from "../enums/encryptionType"; -import { IDecryptable } from "../interfaces/IDecryptable"; import { IEncrypted } from "../interfaces/IEncrypted"; +import { Decryptable } from "../interfaces/decryptable.interface"; import { getEncryptedProperties } from "../misc/encrypted.decorator"; import { Utils } from "../misc/utils"; import { EncArrayBuffer } from "../models/domain/encArrayBuffer"; @@ -167,7 +167,7 @@ export class EncryptService implements AbstractEncryptService { return obj; } - async decryptItem(item: IDecryptable, key: SymmetricCryptoKey): Promise { + async decryptItem(item: Decryptable, key: SymmetricCryptoKey): Promise { if (item == null) { throw new Error("Cannot decrypt a null item"); } From d01adc82a938226fac83531c84b62810898e3d1f Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Tue, 11 Oct 2022 09:56:03 +1000 Subject: [PATCH 18/19] Rename encrypted-objects --- libs/common/spec/services/encrypt.service.spec.ts | 2 +- .../spec/services/{encryptedObject.ts => encrypted-objects.ts} | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename libs/common/spec/services/{encryptedObject.ts => encrypted-objects.ts} (100%) diff --git a/libs/common/spec/services/encrypt.service.spec.ts b/libs/common/spec/services/encrypt.service.spec.ts index 7a957750ba13..144d7ca8fc20 100644 --- a/libs/common/spec/services/encrypt.service.spec.ts +++ b/libs/common/spec/services/encrypt.service.spec.ts @@ -17,7 +17,7 @@ import { NestedEncryptedObjectView, SimpleEncryptedObject, SimpleEncryptedObjectView, -} from "./encryptedObject"; +} from "./encrypted-objects"; describe("EncryptService", () => { const cryptoFunctionService = mock(); diff --git a/libs/common/spec/services/encryptedObject.ts b/libs/common/spec/services/encrypted-objects.ts similarity index 100% rename from libs/common/spec/services/encryptedObject.ts rename to libs/common/spec/services/encrypted-objects.ts From aebd065e1e0ba7f385ad309e6b9ec7b0b14b4976 Mon Sep 17 00:00:00 2001 From: Thomas Rittson Date: Tue, 11 Oct 2022 10:41:07 +1000 Subject: [PATCH 19/19] Update comment --- libs/common/src/models/domain/encString.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/common/src/models/domain/encString.ts b/libs/common/src/models/domain/encString.ts index 8bee993a67a5..20cfd59c9ecc 100644 --- a/libs/common/src/models/domain/encString.ts +++ b/libs/common/src/models/domain/encString.ts @@ -14,7 +14,7 @@ export class EncString implements IEncrypted { mac?: string; /** - * @deprecated: EncStrings no longer store their decrypted values as this risks leaking data + * @deprecated: EncStrings no longer store their decrypted values */ private decryptedValue?: string;