From 5042227a8bc59676a42ea135219b728a38ebf6c7 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Tue, 26 Nov 2024 13:21:43 +0100 Subject: [PATCH 01/11] feat: re-encrypt batchGet results when they have different salts --- .../__fixtures__/mockResponses.ts | 19 +++---- .../controllers/user-storage/services.test.ts | 50 +++++++++++++++++++ .../src/controllers/user-storage/services.ts | 16 ++++++ .../controllers/user-storage/utils.test.ts | 44 +++++++++++++++- .../src/controllers/user-storage/utils.ts | 26 ++++++++++ .../src/shared/encryption/encryption.test.ts | 12 +++++ .../src/shared/encryption/encryption.ts | 26 ++++++++++ .../src/shared/encryption/utils.ts | 12 +++++ 8 files changed, 195 insertions(+), 10 deletions(-) diff --git a/packages/profile-sync-controller/src/controllers/user-storage/__fixtures__/mockResponses.ts b/packages/profile-sync-controller/src/controllers/user-storage/__fixtures__/mockResponses.ts index 218eb9a997..fc81ff44b3 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/__fixtures__/mockResponses.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/__fixtures__/mockResponses.ts @@ -58,15 +58,16 @@ export async function createMockGetStorageResponse( export async function createMockAllFeatureEntriesResponse( dataArr: string[] = [MOCK_STORAGE_DATA], ): Promise { - return await Promise.all( - dataArr.map(async function (d) { - const encryptedData = await MOCK_ENCRYPTED_STORAGE_DATA(d); - return { - HashedKey: 'HASHED_KEY', - Data: encryptedData, - }; - }), - ); + const decryptedData = []; + + for (const data of dataArr) { + decryptedData.push({ + HashedKey: 'HASHED_KEY', + Data: await MOCK_ENCRYPTED_STORAGE_DATA(data), + }); + } + + return decryptedData; } /** diff --git a/packages/profile-sync-controller/src/controllers/user-storage/services.test.ts b/packages/profile-sync-controller/src/controllers/user-storage/services.test.ts index 7fda37ed15..2adb3f8a6f 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/services.test.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/services.test.ts @@ -24,6 +24,7 @@ import { deleteUserStorageAllFeatureEntries, deleteUserStorage, } from './services'; +import { getIfEntriesHaveDifferentSalts } from './utils'; describe('user-storage/services.ts - getUserStorage() tests', () => { const actCallGetUserStorage = async () => { @@ -103,6 +104,55 @@ describe('user-storage/services.ts - getUserStorageAllFeatureEntries() tests', ( expect(result).toStrictEqual([MOCK_STORAGE_DATA]); }); + it('re-encrypts data if entries were encrypted with different salts, and saves it back to user storage', async () => { + // This corresponds to [['entry1', 'data1'], ['entry2', 'data2']] + // Each entry has been encrypted with a different salt + const mockResponse = [ + { + HashedKey: 'entry1', + Data: '{"v":"1","t":"scrypt","d":"HIu+WgFBCtKo6rEGy0R8h8t/JgXhzC2a3AF6epahGY2h6GibXDKxSBf6ppxM099Gmg==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}', + }, + { + HashedKey: 'entry2', + Data: '{"v":"1","t":"scrypt","d":"3ioo9bxhjDjTmJWIGQMnOlnfa4ysuUNeLYTTmJ+qrq7gwI6hURH3ooUcBldJkHtvuQ==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}', + }, + ]; + + const mockGetUserStorageAllFeatureEntries = + await mockEndpointGetUserStorageAllFeatureEntries( + USER_STORAGE_FEATURE_NAMES.notifications, + { + status: 200, + body: JSON.stringify(mockResponse), + }, + ); + + const mockBatchUpsertUserStorage = mockEndpointBatchUpsertUserStorage( + USER_STORAGE_FEATURE_NAMES.notifications, + undefined, + async (_uri, requestBody) => { + if (typeof requestBody === 'string') { + return; + } + + const doEntriesHaveDifferentSalts = getIfEntriesHaveDifferentSalts( + Object.entries(requestBody.data).map((entry) => ({ + HashedKey: entry[0], + Data: entry[1] as string, + })), + ); + + expect(doEntriesHaveDifferentSalts).toBe(false); + }, + ); + + const result = await actCallGetUserStorageAllFeatureEntries(); + + mockGetUserStorageAllFeatureEntries.done(); + mockBatchUpsertUserStorage.done(); + expect(result).toStrictEqual(['data1', 'data2']); + }); + it('returns null if endpoint does not have entry', async () => { const mockGetUserStorage = await mockEndpointGetUserStorageAllFeatureEntries( diff --git a/packages/profile-sync-controller/src/controllers/user-storage/services.ts b/packages/profile-sync-controller/src/controllers/user-storage/services.ts index 6680d8ca73..f8b2c36b72 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/services.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/services.ts @@ -8,6 +8,7 @@ import type { } from '../../shared/storage-schema'; import { createEntryPath } from '../../shared/storage-schema'; import type { NativeScrypt } from '../../shared/types/encryption'; +import { getIfEntriesHaveDifferentSalts } from './utils'; const ENV_URLS = getEnvUrls(Env.PRD); @@ -135,6 +136,11 @@ export async function getUserStorageAllFeatureEntries( return null; } + // Before decrypting, check if entries have different salts + // If they do, we need to re-encrypt all entries with the same salt + const doEntriesHaveDifferentSalts = + userStorage.length > 1 && getIfEntriesHaveDifferentSalts(userStorage); + const decryptedData: string[] = []; for (const entry of userStorage) { @@ -154,6 +160,16 @@ export async function getUserStorageAllFeatureEntries( } } + if (doEntriesHaveDifferentSalts) { + // If we have different salts, we need to re-encrypt all entries with the same salt + // This is done so we minimize the number of key computations when subsequently decrypting the entries + const hashedKeys = userStorage.map((e) => e.HashedKey); + const newEntries = decryptedData.map( + (d, idx) => [hashedKeys[idx], d] as [string, string], + ); + await batchUpsertUserStorage(newEntries, opts); + } + return decryptedData; } catch (e) { log.error('Failed to get user storage', e); diff --git a/packages/profile-sync-controller/src/controllers/user-storage/utils.test.ts b/packages/profile-sync-controller/src/controllers/user-storage/utils.test.ts index cecdf729f2..a214fffba3 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/utils.test.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/utils.test.ts @@ -1,4 +1,8 @@ -import { setDifference, setIntersection } from './utils'; +import { + getIfEntriesHaveDifferentSalts, + setDifference, + setIntersection, +} from './utils'; describe('utils - setDifference()', () => { it('should return the difference between 2 sets', () => { @@ -27,3 +31,41 @@ describe('utils - setIntersection()', () => { expect(inBothSetsWithParamsReversed).toStrictEqual([3]); }); }); + +describe('utils - getIfEntriesHaveDifferentSalts()', () => { + it('should return true if entries have different salts', () => { + const entries = [ + { + HashedKey: + '997050281e559a2bb40d1c2e73d9f0887cbea1b81ff9dd7815917949e37f4f2f', + Data: '{"v":"1","t":"scrypt","d":"1yC/ZXarV57HbqEZ46nH0JWgXfPl86nTHD7kai2g5gm290FM9tw5QjOaAAwIuQESEE8TIM/J9pIj7nmlGi+BZrevTtK3DXWXwnUQsCP7amKd5Q4gs3EEQgXpA0W+WJUgyElj869rwIv/C6tl5E2pK4j/0EAjMSIm1TGoj9FPohyRgZsOIt8VhZfb7w0GODsjPwPIkN6zazvJ3gAFYFPh7yRtebFs86z3fzqCWZ9zakdCHntchC2oZiaApXR9yzaPlGgnPg==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}', + }, + { + HashedKey: + 'e53d8feb65b4cf0c339e57bee2a81b155e056622f9192c54b707f928c8a42a7a', + Data: '{"v":"1","t":"scrypt","d":"x7QqsdqsdEtUo7q/jG+UNkD/HOxQARGGRXsGPrLsDlkwDfgfoYlPI0To/M3pJRBlKD0RLEFIPHtHBEA5bv/2izB21VljvhMnhHfo0KgQ+e8Uq1t7grwa+r+ge3qbPNY+w78Xt8GtC+Hkrw5fORKvCn+xjzaCHYV6RxKYbp1TpyCJq7hDrr1XiyL8kqbpE0hAHALrrQOoV9/WXJi9pC5J118kquXx8CNA1P5wO/BXKp1AbryGR6kVW3lsp1sy3lYE/TApa5lTj+","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}', + }, + ]; + + const result = getIfEntriesHaveDifferentSalts(entries); + expect(result).toBe(true); + }); + + it('should return false if entries have the same salts', () => { + const entries = [ + { + HashedKey: + '997050281e559a2bb40d1c2e73d9f0887cbea1b81ff9dd7815917949e37f4f2f', + Data: '{"v":"1","t":"scrypt","d":"+nhJkMMjQljyyyytsnhO4dIzFL/hGR4Y6hb2qUGrPb/hjxHVJUk1jcJAyHP9eUzgZQ==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}', + }, + { + HashedKey: + 'e53d8feb65b4cf0c339e57bee2a81b155e056622f9192c54b707f928c8a42a7a', + Data: '{"v":"1","t":"scrypt","d":"+nhJkMMjQljyyyytsnhO4XYxpF0N3IXuhCpPM9dAyw5pO2gcqcXNucJs60rBtgKttA==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}', + }, + ]; + + const result = getIfEntriesHaveDifferentSalts(entries); + expect(result).toBe(false); + }); +}); diff --git a/packages/profile-sync-controller/src/controllers/user-storage/utils.ts b/packages/profile-sync-controller/src/controllers/user-storage/utils.ts index e57e317b63..4f071ba297 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/utils.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/utils.ts @@ -1,3 +1,7 @@ +import encryption from '../../shared/encryption'; +import { areAllUInt8ArraysUnique } from '../../shared/encryption/utils'; +import type { GetUserStorageAllFeatureEntriesResponse } from './services'; + /** * Returns the difference between 2 sets. * NOTE - this is temporary until we can support Set().difference method @@ -26,3 +30,25 @@ export function setIntersection( a.forEach((e) => b.has(e) && intersection.add(e)); return intersection; } + +/** + * Returns a boolean indicating if the entries have different salts. + * + * @param entries - User Storage Entries + * @returns A boolean indicating if the entries have different salts. + */ +export function getIfEntriesHaveDifferentSalts( + entries: GetUserStorageAllFeatureEntriesResponse, +): boolean { + const salts = entries + .map((e) => { + try { + return encryption.getSalt(e.Data); + } catch { + return undefined; + } + }) + .filter((s): s is Uint8Array => s !== undefined); + + return areAllUInt8ArraysUnique(salts); +} diff --git a/packages/profile-sync-controller/src/shared/encryption/encryption.test.ts b/packages/profile-sync-controller/src/shared/encryption/encryption.test.ts index 0b63381685..ccba08ecfd 100644 --- a/packages/profile-sync-controller/src/shared/encryption/encryption.test.ts +++ b/packages/profile-sync-controller/src/shared/encryption/encryption.test.ts @@ -37,4 +37,16 @@ describe('encryption tests', () => { const hash2 = createSHA256Hash(DATA); expect(hash1).toBe(hash2); }); + + it('should be able to get the salt from an encrypted string', async () => { + const encryptedData = `{"v":"1","t":"scrypt","d":"d9k8wRtOOq97OyNqRNnTCa3ct7+z9nRjV75Am+ND9yMoV/bMcnrzZqO2EhjL3viJyA==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}`; + const saltUsedToPreviouslyEncryptData = new Uint8Array([ + 119, 217, 60, 193, 27, 78, 58, 175, 123, 59, 35, 106, 68, 217, 211, 9, + ]); + + const salt = encryption.getSalt(encryptedData); + expect(salt).toBeInstanceOf(Uint8Array); + expect(salt).toHaveLength(16); + expect(salt).toStrictEqual(saltUsedToPreviouslyEncryptData); + }); }); diff --git a/packages/profile-sync-controller/src/shared/encryption/encryption.ts b/packages/profile-sync-controller/src/shared/encryption/encryption.ts index 969f613751..87a5db1cac 100644 --- a/packages/profile-sync-controller/src/shared/encryption/encryption.ts +++ b/packages/profile-sync-controller/src/shared/encryption/encryption.ts @@ -171,6 +171,32 @@ class EncryptorDecryptor { return bytesToUtf8(this.#decrypt(ciphertextAndNonce, key)); } + getSalt(encryptedDataStr: string) { + try { + const encryptedData: EncryptedPayload = JSON.parse(encryptedDataStr); + if (encryptedData.v === '1') { + if (encryptedData.t === 'scrypt') { + const { d: base64CiphertextAndNonceAndSalt, saltLen } = encryptedData; + + // Decode the base64. + const ciphertextAndNonceAndSalt = base64ToByteArray( + base64CiphertextAndNonceAndSalt, + ); + + // Create buffers of salt and ciphertextAndNonce. + const salt = ciphertextAndNonceAndSalt.slice(0, saltLen); + return salt; + } + } + throw new Error( + `Unsupported encrypted data payload - ${encryptedDataStr}`, + ); + } catch (e) { + const errorMessage = e instanceof Error ? e.message : JSON.stringify(e); + throw new Error(`Unable to decrypt string - ${errorMessage}`); + } + } + #encrypt(plaintext: Uint8Array, key: Uint8Array): Uint8Array { const nonce = randomBytes(ALGORITHM_NONCE_SIZE); diff --git a/packages/profile-sync-controller/src/shared/encryption/utils.ts b/packages/profile-sync-controller/src/shared/encryption/utils.ts index c9caa55fe8..ea323e3075 100644 --- a/packages/profile-sync-controller/src/shared/encryption/utils.ts +++ b/packages/profile-sync-controller/src/shared/encryption/utils.ts @@ -14,3 +14,15 @@ export const bytesToUtf8 = (byteArray: Uint8Array) => { export const stringToByteArray = (str: string) => { return new TextEncoder().encode(str); }; + +export const areAllUInt8ArraysUnique = (arrays: Uint8Array[]) => { + const seen = new Set(); + for (const arr of arrays) { + const str = arr.toString(); + if (seen.has(str)) { + return false; + } + seen.add(str); + } + return true; +}; From 72a99e30258a157ddde04ff61dc17552d1938af2 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Fri, 29 Nov 2024 15:09:46 +0100 Subject: [PATCH 02/11] add empty salt logic, keep backwards compatibility, add re-encryption to the sdk --- .../controllers/user-storage/services.test.ts | 68 ++++++++++-- .../src/controllers/user-storage/services.ts | 37 ++++--- .../controllers/user-storage/utils.test.ts | 44 +------- .../src/controllers/user-storage/utils.ts | 26 ----- .../src/sdk/__fixtures__/mock-userstorage.ts | 14 +-- .../src/sdk/user-storage.test.ts | 102 +++++++++++++++++- .../src/sdk/user-storage.ts | 67 ++++++++---- .../src/shared/encryption/cache.ts | 18 ++-- .../src/shared/encryption/encryption.test.ts | 25 +++++ .../src/shared/encryption/encryption.ts | 5 +- .../src/shared/encryption/utils.test.ts | 33 ++++++ .../src/shared/encryption/utils.ts | 22 ++++ 12 files changed, 327 insertions(+), 134 deletions(-) create mode 100644 packages/profile-sync-controller/src/shared/encryption/utils.test.ts diff --git a/packages/profile-sync-controller/src/controllers/user-storage/services.test.ts b/packages/profile-sync-controller/src/controllers/user-storage/services.test.ts index 2adb3f8a6f..ff41a46139 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/services.test.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/services.test.ts @@ -1,6 +1,8 @@ import encryption, { createSHA256Hash } from '../../shared/encryption'; +import { getIfEntriesHaveDifferentSalts } from '../../shared/encryption/utils'; import type { UserStorageFeatureKeys } from '../../shared/storage-schema'; import { USER_STORAGE_FEATURE_NAMES } from '../../shared/storage-schema'; +import { createMockGetStorageResponse } from './__fixtures__'; import { mockEndpointGetUserStorage, mockEndpointUpsertUserStorage, @@ -24,7 +26,6 @@ import { deleteUserStorageAllFeatureEntries, deleteUserStorage, } from './services'; -import { getIfEntriesHaveDifferentSalts } from './utils'; describe('user-storage/services.ts - getUserStorage() tests', () => { const actCallGetUserStorage = async () => { @@ -82,6 +83,44 @@ describe('user-storage/services.ts - getUserStorage() tests', () => { mockGetUserStorage.done(); expect(result).toBeNull(); }); + + it('re-encrypts data if received entry was encrypted with a non-empty salt, and saves it back to user storage', async () => { + // This corresponds to 'data1' + // Encrypted with a non-empty salt + const mockResponse = { + HashedKey: 'entry1', + Data: '{"v":"1","t":"scrypt","d":"HIu+WgFBCtKo6rEGy0R8h8t/JgXhzC2a3AF6epahGY2h6GibXDKxSBf6ppxM099Gmg==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}', + }; + + const mockGetUserStorage = await mockEndpointGetUserStorage( + `${USER_STORAGE_FEATURE_NAMES.notifications}.notification_settings`, + { + status: 200, + body: JSON.stringify(mockResponse), + }, + ); + + const mockUpsertUserStorage = mockEndpointUpsertUserStorage( + `${USER_STORAGE_FEATURE_NAMES.notifications}.notification_settings`, + undefined, + async (requestBody) => { + if (typeof requestBody === 'string') { + return; + } + + const isNewSaltEmpty = + encryption.getSalt(requestBody.data).length === 0; + + expect(isNewSaltEmpty).toBe(true); + }, + ); + + const result = await actCallGetUserStorage(); + + mockGetUserStorage.done(); + mockUpsertUserStorage.done(); + expect(result).toBe('data1'); + }); }); describe('user-storage/services.ts - getUserStorageAllFeatureEntries() tests', () => { @@ -104,9 +143,10 @@ describe('user-storage/services.ts - getUserStorageAllFeatureEntries() tests', ( expect(result).toStrictEqual([MOCK_STORAGE_DATA]); }); - it('re-encrypts data if entries were encrypted with different salts, and saves it back to user storage', async () => { - // This corresponds to [['entry1', 'data1'], ['entry2', 'data2']] - // Each entry has been encrypted with a different salt + it('re-encrypts data if received entries were encrypted with non-empty salts, and saves it back to user storage', async () => { + // This corresponds to [['entry1', 'data1'], ['entry2', 'data2'], ['HASHED_KEY', '{ "hello": "world" }']] + // Each entry has been encrypted with a non-empty salt, except for the last entry + // The last entry is used to test if the function can handle entries with either empty or non-empty salts const mockResponse = [ { HashedKey: 'entry1', @@ -116,6 +156,7 @@ describe('user-storage/services.ts - getUserStorageAllFeatureEntries() tests', ( HashedKey: 'entry2', Data: '{"v":"1","t":"scrypt","d":"3ioo9bxhjDjTmJWIGQMnOlnfa4ysuUNeLYTTmJ+qrq7gwI6hURH3ooUcBldJkHtvuQ==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}', }, + await createMockGetStorageResponse('data3'), ]; const mockGetUserStorageAllFeatureEntries = @@ -136,13 +177,22 @@ describe('user-storage/services.ts - getUserStorageAllFeatureEntries() tests', ( } const doEntriesHaveDifferentSalts = getIfEntriesHaveDifferentSalts( - Object.entries(requestBody.data).map((entry) => ({ - HashedKey: entry[0], - Data: entry[1] as string, - })), + Object.entries(requestBody.data).map((entry) => entry[1] as string), ); expect(doEntriesHaveDifferentSalts).toBe(false); + + const doEntriesHaveEmptySalts = Object.entries(requestBody.data).every( + ([_entryKey, entryValue]) => + encryption.getSalt(entryValue as string).length === 0, + ); + + expect(doEntriesHaveEmptySalts).toBe(true); + + const wereOnlyNonEmptySaltEntriesUploaded = + Object.entries(requestBody.data).length === 2; + + expect(wereOnlyNonEmptySaltEntriesUploaded).toBe(true); }, ); @@ -150,7 +200,7 @@ describe('user-storage/services.ts - getUserStorageAllFeatureEntries() tests', ( mockGetUserStorageAllFeatureEntries.done(); mockBatchUpsertUserStorage.done(); - expect(result).toStrictEqual(['data1', 'data2']); + expect(result).toStrictEqual(['data1', 'data2', MOCK_STORAGE_DATA]); }); it('returns null if endpoint does not have entry', async () => { diff --git a/packages/profile-sync-controller/src/controllers/user-storage/services.ts b/packages/profile-sync-controller/src/controllers/user-storage/services.ts index f8b2c36b72..4fb979db04 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/services.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/services.ts @@ -8,7 +8,6 @@ import type { } from '../../shared/storage-schema'; import { createEntryPath } from '../../shared/storage-schema'; import type { NativeScrypt } from '../../shared/types/encryption'; -import { getIfEntriesHaveDifferentSalts } from './utils'; const ENV_URLS = getEnvUrls(Env.PRD); @@ -93,6 +92,12 @@ export async function getUserStorage( nativeScryptCrypto, ); + // Re-encrypt the entry if the salt is non-empty + const salt = encryption.getSalt(encryptedData); + if (salt.length) { + await upsertUserStorage(decryptedData, opts); + } + return decryptedData; } catch (e) { log.error('Failed to get user storage', e); @@ -136,12 +141,8 @@ export async function getUserStorageAllFeatureEntries( return null; } - // Before decrypting, check if entries have different salts - // If they do, we need to re-encrypt all entries with the same salt - const doEntriesHaveDifferentSalts = - userStorage.length > 1 && getIfEntriesHaveDifferentSalts(userStorage); - const decryptedData: string[] = []; + const reEncryptedEntries: [string, string][] = []; for (const entry of userStorage) { if (!entry.Data) { @@ -155,19 +156,27 @@ export async function getUserStorageAllFeatureEntries( nativeScryptCrypto, ); decryptedData.push(data); + + // Re-encrypt the entry if the salt is non-empty + const salt = encryption.getSalt(entry.Data); + if (salt.length) { + reEncryptedEntries.push([ + entry.HashedKey, + await encryption.encryptString( + data, + opts.storageKey, + nativeScryptCrypto, + ), + ]); + } } catch { // do nothing } } - if (doEntriesHaveDifferentSalts) { - // If we have different salts, we need to re-encrypt all entries with the same salt - // This is done so we minimize the number of key computations when subsequently decrypting the entries - const hashedKeys = userStorage.map((e) => e.HashedKey); - const newEntries = decryptedData.map( - (d, idx) => [hashedKeys[idx], d] as [string, string], - ); - await batchUpsertUserStorage(newEntries, opts); + // Re-upload the re-encrypted entries + if (reEncryptedEntries.length) { + await batchUpsertUserStorage(reEncryptedEntries, opts); } return decryptedData; diff --git a/packages/profile-sync-controller/src/controllers/user-storage/utils.test.ts b/packages/profile-sync-controller/src/controllers/user-storage/utils.test.ts index a214fffba3..cecdf729f2 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/utils.test.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/utils.test.ts @@ -1,8 +1,4 @@ -import { - getIfEntriesHaveDifferentSalts, - setDifference, - setIntersection, -} from './utils'; +import { setDifference, setIntersection } from './utils'; describe('utils - setDifference()', () => { it('should return the difference between 2 sets', () => { @@ -31,41 +27,3 @@ describe('utils - setIntersection()', () => { expect(inBothSetsWithParamsReversed).toStrictEqual([3]); }); }); - -describe('utils - getIfEntriesHaveDifferentSalts()', () => { - it('should return true if entries have different salts', () => { - const entries = [ - { - HashedKey: - '997050281e559a2bb40d1c2e73d9f0887cbea1b81ff9dd7815917949e37f4f2f', - Data: '{"v":"1","t":"scrypt","d":"1yC/ZXarV57HbqEZ46nH0JWgXfPl86nTHD7kai2g5gm290FM9tw5QjOaAAwIuQESEE8TIM/J9pIj7nmlGi+BZrevTtK3DXWXwnUQsCP7amKd5Q4gs3EEQgXpA0W+WJUgyElj869rwIv/C6tl5E2pK4j/0EAjMSIm1TGoj9FPohyRgZsOIt8VhZfb7w0GODsjPwPIkN6zazvJ3gAFYFPh7yRtebFs86z3fzqCWZ9zakdCHntchC2oZiaApXR9yzaPlGgnPg==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}', - }, - { - HashedKey: - 'e53d8feb65b4cf0c339e57bee2a81b155e056622f9192c54b707f928c8a42a7a', - Data: '{"v":"1","t":"scrypt","d":"x7QqsdqsdEtUo7q/jG+UNkD/HOxQARGGRXsGPrLsDlkwDfgfoYlPI0To/M3pJRBlKD0RLEFIPHtHBEA5bv/2izB21VljvhMnhHfo0KgQ+e8Uq1t7grwa+r+ge3qbPNY+w78Xt8GtC+Hkrw5fORKvCn+xjzaCHYV6RxKYbp1TpyCJq7hDrr1XiyL8kqbpE0hAHALrrQOoV9/WXJi9pC5J118kquXx8CNA1P5wO/BXKp1AbryGR6kVW3lsp1sy3lYE/TApa5lTj+","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}', - }, - ]; - - const result = getIfEntriesHaveDifferentSalts(entries); - expect(result).toBe(true); - }); - - it('should return false if entries have the same salts', () => { - const entries = [ - { - HashedKey: - '997050281e559a2bb40d1c2e73d9f0887cbea1b81ff9dd7815917949e37f4f2f', - Data: '{"v":"1","t":"scrypt","d":"+nhJkMMjQljyyyytsnhO4dIzFL/hGR4Y6hb2qUGrPb/hjxHVJUk1jcJAyHP9eUzgZQ==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}', - }, - { - HashedKey: - 'e53d8feb65b4cf0c339e57bee2a81b155e056622f9192c54b707f928c8a42a7a', - Data: '{"v":"1","t":"scrypt","d":"+nhJkMMjQljyyyytsnhO4XYxpF0N3IXuhCpPM9dAyw5pO2gcqcXNucJs60rBtgKttA==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}', - }, - ]; - - const result = getIfEntriesHaveDifferentSalts(entries); - expect(result).toBe(false); - }); -}); diff --git a/packages/profile-sync-controller/src/controllers/user-storage/utils.ts b/packages/profile-sync-controller/src/controllers/user-storage/utils.ts index 4f071ba297..e57e317b63 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/utils.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/utils.ts @@ -1,7 +1,3 @@ -import encryption from '../../shared/encryption'; -import { areAllUInt8ArraysUnique } from '../../shared/encryption/utils'; -import type { GetUserStorageAllFeatureEntriesResponse } from './services'; - /** * Returns the difference between 2 sets. * NOTE - this is temporary until we can support Set().difference method @@ -30,25 +26,3 @@ export function setIntersection( a.forEach((e) => b.has(e) && intersection.add(e)); return intersection; } - -/** - * Returns a boolean indicating if the entries have different salts. - * - * @param entries - User Storage Entries - * @returns A boolean indicating if the entries have different salts. - */ -export function getIfEntriesHaveDifferentSalts( - entries: GetUserStorageAllFeatureEntriesResponse, -): boolean { - const salts = entries - .map((e) => { - try { - return encryption.getSalt(e.Data); - } catch { - return undefined; - } - }) - .filter((s): s is Uint8Array => s !== undefined); - - return areAllUInt8ArraysUnique(salts); -} diff --git a/packages/profile-sync-controller/src/sdk/__fixtures__/mock-userstorage.ts b/packages/profile-sync-controller/src/sdk/__fixtures__/mock-userstorage.ts index 6b37dc41d0..4b7a8fbe44 100644 --- a/packages/profile-sync-controller/src/sdk/__fixtures__/mock-userstorage.ts +++ b/packages/profile-sync-controller/src/sdk/__fixtures__/mock-userstorage.ts @@ -1,6 +1,6 @@ import nock from 'nock'; -import encryption from '../../shared/encryption'; +import encryption, { createSHA256Hash } from '../../shared/encryption'; import { Env } from '../../shared/env'; import { USER_STORAGE_FEATURE_NAMES } from '../../shared/storage-schema'; import { STORAGE_URL } from '../user-storage'; @@ -20,19 +20,19 @@ const MOCK_STORAGE_URL_ALL_FEATURE_ENTRIES = STORAGE_URL( USER_STORAGE_FEATURE_NAMES.notifications, ); -export const MOCK_STORAGE_KEY = 'MOCK_STORAGE_KEY'; +export const MOCK_STORAGE_KEY = createSHA256Hash('mockStorageKey'); // TODO: Either fix this lint violation or explain why it's necessary to ignore. // eslint-disable-next-line @typescript-eslint/naming-convention -export const MOCK_NOTIFICATIONS_DATA = { is_compact: false }; -export const MOCK_NOTIFICATIONS_DATA_ENCRYPTED = async () => +export const MOCK_NOTIFICATIONS_DATA = '{ is_compact: false }'; +export const MOCK_NOTIFICATIONS_DATA_ENCRYPTED = async (data?: string) => await encryption.encryptString( - JSON.stringify(MOCK_NOTIFICATIONS_DATA), + data ?? MOCK_NOTIFICATIONS_DATA, MOCK_STORAGE_KEY, ); -export const MOCK_STORAGE_RESPONSE = async () => ({ +export const MOCK_STORAGE_RESPONSE = async (data?: string) => ({ HashedKey: '8485d2c14c333ebca415140a276adaf546619b0efc204586b73a5d400a18a5e2', - Data: await MOCK_NOTIFICATIONS_DATA_ENCRYPTED(), + Data: await MOCK_NOTIFICATIONS_DATA_ENCRYPTED(data), }); export const handleMockUserStorageGet = async (mockReply?: MockReply) => { diff --git a/packages/profile-sync-controller/src/sdk/user-storage.test.ts b/packages/profile-sync-controller/src/sdk/user-storage.test.ts index 7782d4c239..921425c700 100644 --- a/packages/profile-sync-controller/src/sdk/user-storage.test.ts +++ b/packages/profile-sync-controller/src/sdk/user-storage.test.ts @@ -1,4 +1,5 @@ import encryption, { createSHA256Hash } from '../shared/encryption'; +import { getIfEntriesHaveDifferentSalts } from '../shared/encryption/utils'; import { Env } from '../shared/env'; import type { UserStorageFeatureKeys } from '../shared/storage-schema'; import { USER_STORAGE_FEATURE_NAMES } from '../shared/storage-schema'; @@ -12,6 +13,7 @@ import { handleMockUserStorageDeleteAllFeatureEntries, handleMockUserStorageDelete, handleMockUserStorageBatchDelete, + MOCK_STORAGE_RESPONSE, } from './__fixtures__/mock-userstorage'; import { arrangeAuth, typedMockFn } from './__fixtures__/test-utils'; import type { IBaseAuth } from './authentication-jwt-bearer/types'; @@ -40,7 +42,7 @@ describe('User Storage', () => { const mockGet = await handleMockUserStorageGet(); // Test Set - const data = JSON.stringify(MOCK_NOTIFICATIONS_DATA); + const data = MOCK_NOTIFICATIONS_DATA; await userStorage.setItem( `${USER_STORAGE_FEATURE_NAMES.notifications}.notification_settings`, data, @@ -71,7 +73,7 @@ describe('User Storage', () => { const mockGet = await handleMockUserStorageGet(); // Test Set - const data = JSON.stringify(MOCK_NOTIFICATIONS_DATA); + const data = MOCK_NOTIFICATIONS_DATA; await userStorage.setItem( `${USER_STORAGE_FEATURE_NAMES.notifications}.notification_settings`, data, @@ -87,13 +89,49 @@ describe('User Storage', () => { expect(response).toBe(data); }); + it('re-encrypts data if received entry was encrypted with a non-empty salt, and saves it back to user storage', async () => { + const { auth } = arrangeAuth('SRP', MOCK_SRP); + const { userStorage } = arrangeUserStorage(auth); + + // This corresponds to 'data1' + // Encrypted with a non-empty salt + const mockResponse = { + HashedKey: 'entry1', + Data: '{"v":"1","t":"scrypt","d":"HIu+WgFBCtKo6rEGy0R8h8t/JgXhzC2a3AF6epahGY2h6GibXDKxSBf6ppxM099Gmg==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}', + }; + + const mockGet = await handleMockUserStorageGet({ + status: 200, + body: JSON.stringify(mockResponse), + }); + const mockPut = handleMockUserStoragePut( + undefined, + async (_, requestBody) => { + if (typeof requestBody === 'string') { + return; + } + + const isNewSaltEmpty = + encryption.getSalt(requestBody.data).length === 0; + + expect(isNewSaltEmpty).toBe(true); + }, + ); + + await userStorage.getItem( + `${USER_STORAGE_FEATURE_NAMES.notifications}.notification_settings`, + ); + expect(mockGet.isDone()).toBe(true); + expect(mockPut.isDone()).toBe(true); + }); + it('gets all feature entries', async () => { const { auth } = arrangeAuth('SRP', MOCK_SRP); const { userStorage } = arrangeUserStorage(auth); const mockGetAll = await handleMockUserStorageGetAllFeatureEntries(); - const data = JSON.stringify(MOCK_NOTIFICATIONS_DATA); + const data = MOCK_NOTIFICATIONS_DATA; const responseAllFeatureEntries = await userStorage.getAllFeatureItems( USER_STORAGE_FEATURE_NAMES.notifications, ); @@ -101,6 +139,64 @@ describe('User Storage', () => { expect(responseAllFeatureEntries).toStrictEqual([data]); }); + it('re-encrypts data if received entries were encrypted with non-empty salts, and saves it back to user storage', async () => { + // This corresponds to [['entry1', 'data1'], ['entry2', 'data2'], ['HASHED_KEY', '{ "hello": "world" }']] + // Each entry has been encrypted with a non-empty salt, except for the last entry + // The last entry is used to test if the function can handle entries with either empty or non-empty salts + const mockResponse = [ + { + HashedKey: 'entry1', + Data: '{"v":"1","t":"scrypt","d":"HIu+WgFBCtKo6rEGy0R8h8t/JgXhzC2a3AF6epahGY2h6GibXDKxSBf6ppxM099Gmg==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}', + }, + { + HashedKey: 'entry2', + Data: '{"v":"1","t":"scrypt","d":"3ioo9bxhjDjTmJWIGQMnOlnfa4ysuUNeLYTTmJ+qrq7gwI6hURH3ooUcBldJkHtvuQ==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}', + }, + await MOCK_STORAGE_RESPONSE('data3'), + ]; + + const { auth } = arrangeAuth('SRP', MOCK_SRP); + const { userStorage } = arrangeUserStorage(auth); + + const mockGetAll = await handleMockUserStorageGetAllFeatureEntries({ + status: 200, + body: mockResponse, + }); + + const mockPut = handleMockUserStoragePut( + undefined, + async (_, requestBody) => { + if (typeof requestBody === 'string') { + return; + } + + const doEntriesHaveDifferentSalts = getIfEntriesHaveDifferentSalts( + Object.entries(requestBody.data).map((entry) => entry[1] as string), + ); + + expect(doEntriesHaveDifferentSalts).toBe(false); + + const doEntriesHaveEmptySalts = Object.entries(requestBody.data).every( + ([_entryKey, entryValue]) => + encryption.getSalt(entryValue as string).length === 0, + ); + + expect(doEntriesHaveEmptySalts).toBe(true); + + const wereOnlyNonEmptySaltEntriesUploaded = + Object.entries(requestBody.data).length === 2; + + expect(wereOnlyNonEmptySaltEntriesUploaded).toBe(true); + }, + ); + + await userStorage.getAllFeatureItems( + USER_STORAGE_FEATURE_NAMES.notifications, + ); + expect(mockGetAll.isDone()).toBe(true); + expect(mockPut.isDone()).toBe(true); + }); + it('batch set items', async () => { const dataToStore: [ UserStorageFeatureKeys, diff --git a/packages/profile-sync-controller/src/sdk/user-storage.ts b/packages/profile-sync-controller/src/sdk/user-storage.ts index 9376e90641..6deb4aa326 100644 --- a/packages/profile-sync-controller/src/sdk/user-storage.ts +++ b/packages/profile-sync-controller/src/sdk/user-storage.ts @@ -28,7 +28,7 @@ export type UserStorageOptions = { storage?: StorageOptions; }; -type GetUserStorageAllFeatureEntriesResponse = { +export type GetUserStorageAllFeatureEntriesResponse = { // eslint-disable-next-line @typescript-eslint/naming-convention HashedKey: string; // eslint-disable-next-line @typescript-eslint/naming-convention @@ -87,9 +87,9 @@ export class UserStorage { return this.#deleteUserStorageAllFeatureEntries(path); } - async batchDeleteItems( - path: FeatureName, - values: UserStorageFeatureKeys[], + async batchDeleteItems( + path: UserStoragePathWithFeatureOnly, + values: string[], ) { return this.#batchDeleteUserStorage(path, values); } @@ -149,9 +149,9 @@ export class UserStorage { } } - async #batchUpsertUserStorage( - path: FeatureName, - data: [UserStorageFeatureKeys, string][], + async #batchUpsertUserStorage( + path: UserStoragePathWithFeatureOnly, + data: [string, string][], ): Promise { try { if (!data.length) { @@ -231,7 +231,18 @@ export class UserStorage { } const { Data: encryptedData } = await response.json(); - return encryption.decryptString(encryptedData, storageKey); + const decryptedData = await encryption.decryptString( + encryptedData, + storageKey, + ); + + // Re-encrypt the entry if the salt is non-empty + const salt = encryption.getSalt(encryptedData); + if (salt.length) { + await this.#upsertUserStorage(path, decryptedData); + } + + return decryptedData; } catch (e) { if (e instanceof NotFoundError) { throw e; @@ -281,17 +292,37 @@ export class UserStorage { return null; } - const decryptedData = userStorage.flatMap((entry) => { + const decryptedData: string[] = []; + const reEncryptedEntries: [string, string][] = []; + + for (const entry of userStorage) { if (!entry.Data) { - return []; + continue; } - return encryption.decryptString(entry.Data, storageKey); - }); + try { + const data = await encryption.decryptString(entry.Data, storageKey); + decryptedData.push(data); + + // Re-encrypt the entry if the salt is non-empty + const salt = encryption.getSalt(entry.Data); + if (salt.length) { + reEncryptedEntries.push([ + entry.HashedKey, + await encryption.encryptString(data, storageKey), + ]); + } + } catch { + // do nothing + } + } - return (await Promise.allSettled(decryptedData)) - .map((d) => (d.status === 'fulfilled' ? d.value : undefined)) - .filter((d): d is string => d !== undefined); + // Re-upload the re-encrypted entries + if (reEncryptedEntries.length) { + await this.#batchUpsertUserStorage(path, reEncryptedEntries); + } + + return decryptedData; } catch (e) { if (e instanceof NotFoundError) { throw e; @@ -393,9 +424,9 @@ export class UserStorage { } } - async #batchDeleteUserStorage( - path: FeatureName, - data: UserStorageFeatureKeys[], + async #batchDeleteUserStorage( + path: UserStoragePathWithFeatureOnly, + data: string[], ): Promise { try { if (!data.length) { diff --git a/packages/profile-sync-controller/src/shared/encryption/cache.ts b/packages/profile-sync-controller/src/shared/encryption/cache.ts index 3b14925b13..d7e5d16f0e 100644 --- a/packages/profile-sync-controller/src/shared/encryption/cache.ts +++ b/packages/profile-sync-controller/src/shared/encryption/cache.ts @@ -1,4 +1,4 @@ -import { base64ToByteArray, byteArrayToBase64 } from './utils'; +import { byteArrayToBase64 } from './utils'; type CachedEntry = { salt: Uint8Array; @@ -56,22 +56,16 @@ export function getAnyCachedKey( hashedPassword: string, ): CachedEntry | undefined { const cache = getPasswordCache(hashedPassword); + const cachedKey = cache.get(''); - // Takes 1 item from an Iterator via Map.entries() - const cachedEntry: [string, Uint8Array] | undefined = cache - .entries() - .next().value; - - if (!cachedEntry) { + if (!cachedKey) { return undefined; } - const base64Salt = cachedEntry[0]; - const bytesSalt = base64ToByteArray(base64Salt); return { - salt: bytesSalt, - base64Salt, - key: cachedEntry[1], + salt: new Uint8Array(), + base64Salt: '', + key: cachedKey, }; } diff --git a/packages/profile-sync-controller/src/shared/encryption/encryption.test.ts b/packages/profile-sync-controller/src/shared/encryption/encryption.test.ts index ccba08ecfd..ad66168954 100644 --- a/packages/profile-sync-controller/src/shared/encryption/encryption.test.ts +++ b/packages/profile-sync-controller/src/shared/encryption/encryption.test.ts @@ -2,9 +2,17 @@ import encryption, { createSHA256Hash } from './encryption'; describe('encryption tests', () => { const PASSWORD = '123'; + const PASSWORD2 = '1234'; const DATA1 = 'Hello World'; const DATA2 = JSON.stringify({ foo: 'bar' }); + const ACCOUNTS_PASSWORD = + '0d55d30da233959674d14076737198c05ae3fb8631a17e20d3c28c60dddd82f7'; + const ACCOUNTS_ENCRYPTED_DATA_WITH_SALT = + '{"v":"1","t":"scrypt","d":"1yC/ZXarV57HbqEZ46nH0JWgXfPl86nTHD7kai2g5gm290FM9tw5QjOaAAwIuQESEE8TIM/J9pIj7nmlGi+BZrevTtK3DXWXwnUQsCP7amKd5Q4gs3EEQgXpA0W+WJUgyElj869rwIv/C6tl5E2pK4j/0EAjMSIm1TGoj9FPohyRgZsOIt8VhZfb7w0GODsjPwPIkN6zazvJ3gAFYFPh7yRtebFs86z3fzqCWZ9zakdCHntchC2oZiaApXR9yzaPlGgnPg==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}'; + const ACCOUNTS_DECRYPTED_DATA = + '{"v":"1","a":"0xd2a4afe5c2ff0a16bf81f77ba4201a8107aa874b","i":"c590ab50-add6-4de4-8af7-9b696b5e9c6a","n":"My Second Synced Account","nlu":1729234343749}'; + it('should encrypt and decrypt data', async () => { const actEncryptDecrypt = async (data: string) => { const encryptedString = await encryption.encryptString(data, PASSWORD); @@ -49,4 +57,21 @@ describe('encryption tests', () => { expect(salt).toHaveLength(16); expect(salt).toStrictEqual(saltUsedToPreviouslyEncryptData); }); + + it('should be able to decrypt an entry that has no salt', async () => { + const encryptedData = await encryption.encryptString(DATA1, PASSWORD); + const decryptedData = await encryption.decryptString( + encryptedData, + PASSWORD, + ); + expect(decryptedData).toBe(DATA1); + }); + + it('should be able to decrypt an entry that has a salt', async () => { + const decryptedData = await encryption.decryptString( + ACCOUNTS_ENCRYPTED_DATA_WITH_SALT, + ACCOUNTS_PASSWORD, + ); + expect(decryptedData).toBe(ACCOUNTS_DECRYPTED_DATA); + }); }); diff --git a/packages/profile-sync-controller/src/shared/encryption/encryption.ts b/packages/profile-sync-controller/src/shared/encryption/encryption.ts index 87a5db1cac..b0a83fcfa6 100644 --- a/packages/profile-sync-controller/src/shared/encryption/encryption.ts +++ b/packages/profile-sync-controller/src/shared/encryption/encryption.ts @@ -42,7 +42,7 @@ const ALGORITHM_KEY_SIZE = 16; // 16 bytes // Scrypt settings // see: https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html#scrypt -const SCRYPT_SALT_SIZE = 16; // 16 bytes +const SCRYPT_SALT_SIZE = 0; // We don't use a salt const SCRYPT_N = 2 ** 17; // CPU/memory cost parameter (must be a power of 2, > 1) // eslint-disable-next-line @typescript-eslint/naming-convention const SCRYPT_r = 8; // Block size parameter @@ -236,7 +236,7 @@ class EncryptorDecryptor { }; } - const newSalt = salt ?? randomBytes(SCRYPT_SALT_SIZE); + const newSalt = salt ?? new Uint8Array(SCRYPT_SALT_SIZE); let newKey: Uint8Array; @@ -257,6 +257,7 @@ class EncryptorDecryptor { dkLen: o.dkLen, }); } + setCachedKey(hashedPassword, newSalt, newKey); return { diff --git a/packages/profile-sync-controller/src/shared/encryption/utils.test.ts b/packages/profile-sync-controller/src/shared/encryption/utils.test.ts new file mode 100644 index 0000000000..d9374e3c53 --- /dev/null +++ b/packages/profile-sync-controller/src/shared/encryption/utils.test.ts @@ -0,0 +1,33 @@ +import { getIfEntriesHaveDifferentSalts } from './utils'; + +describe('utils - getIfEntriesHaveDifferentSalts()', () => { + it('should return true if entries have different salts', () => { + const entries = [ + '{"v":"1","t":"scrypt","d":"1yC/ZXarV57HbqEZ46nH0JWgXfPl86nTHD7kai2g5gm290FM9tw5QjOaAAwIuQESEE8TIM/J9pIj7nmlGi+BZrevTtK3DXWXwnUQsCP7amKd5Q4gs3EEQgXpA0W+WJUgyElj869rwIv/C6tl5E2pK4j/0EAjMSIm1TGoj9FPohyRgZsOIt8VhZfb7w0GODsjPwPIkN6zazvJ3gAFYFPh7yRtebFs86z3fzqCWZ9zakdCHntchC2oZiaApXR9yzaPlGgnPg==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}', + '{"v":"1","t":"scrypt","d":"x7QqsdqsdEtUo7q/jG+UNkD/HOxQARGGRXsGPrLsDlkwDfgfoYlPI0To/M3pJRBlKD0RLEFIPHtHBEA5bv/2izB21VljvhMnhHfo0KgQ+e8Uq1t7grwa+r+ge3qbPNY+w78Xt8GtC+Hkrw5fORKvCn+xjzaCHYV6RxKYbp1TpyCJq7hDrr1XiyL8kqbpE0hAHALrrQOoV9/WXJi9pC5J118kquXx8CNA1P5wO/BXKp1AbryGR6kVW3lsp1sy3lYE/TApa5lTj+","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}', + ]; + + const result = getIfEntriesHaveDifferentSalts(entries); + expect(result).toBe(true); + }); + + it('should return false if entries have the same salts', () => { + const entries = [ + '{"v":"1","t":"scrypt","d":"+nhJkMMjQljyyyytsnhO4dIzFL/hGR4Y6hb2qUGrPb/hjxHVJUk1jcJAyHP9eUzgZQ==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}', + '{"v":"1","t":"scrypt","d":"+nhJkMMjQljyyyytsnhO4XYxpF0N3IXuhCpPM9dAyw5pO2gcqcXNucJs60rBtgKttA==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}', + ]; + + const result = getIfEntriesHaveDifferentSalts(entries); + expect(result).toBe(false); + }); + + it('should return false if entries do not have salts', () => { + const entries = [ + '{"v":"1","t":"scrypt","d":"CgHcOM6xCaaNFnPCr0etqyxCq4xoJNQ9gfP9+GRn94hGtKurbOuXzyDoHJgzaJxDKd1zQHJhDwLjnH6oCZvC8XKvZZ6RcrN9BicZHpzpojon+HwpcPHceM/pvoMabYfiXqbokYHXZymGTxE5X+TjFo+HB7/Y6xOCU1usz47bru9vfyZrdQ66qGlMO2MUFx00cnh8xHOksDNC","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":0}', + '{"v":"1","t":"scrypt","d":"OCrYnCFkt7a33cjaAL65D/WypM+oVxIiGVwMk+mjijcpnG4r3vzPl6OzFpx2LNKHj6YN59wcLje3QK2hISU0R8iXyZubdkeAiY89SsI7owLda96ysF+q6PuyxnWfNfWe+5a1+4O8BVkR8p/9PYimwTN0QGhX2lkfLt5r0aYgsLnWld/5k9G7cB4yqoduIopzpojS5ZGI8PFW","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":0}', + ]; + + const result = getIfEntriesHaveDifferentSalts(entries); + expect(result).toBe(false); + }); +}); diff --git a/packages/profile-sync-controller/src/shared/encryption/utils.ts b/packages/profile-sync-controller/src/shared/encryption/utils.ts index ea323e3075..7e21665244 100644 --- a/packages/profile-sync-controller/src/shared/encryption/utils.ts +++ b/packages/profile-sync-controller/src/shared/encryption/utils.ts @@ -1,3 +1,5 @@ +import encryption from './encryption'; + export const byteArrayToBase64 = (byteArray: Uint8Array) => { return Buffer.from(byteArray).toString('base64'); }; @@ -26,3 +28,23 @@ export const areAllUInt8ArraysUnique = (arrays: Uint8Array[]) => { } return true; }; + +/** + * Returns a boolean indicating if the entries have different salts. + * + * @param entries - User Storage Entries + * @returns A boolean indicating if the entries have different salts. + */ +export function getIfEntriesHaveDifferentSalts(entries: string[]): boolean { + const salts = entries + .map((e) => { + try { + return encryption.getSalt(e); + } catch { + return undefined; + } + }) + .filter((s): s is Uint8Array => s !== undefined); + + return areAllUInt8ArraysUnique(salts); +} From ac4dbdaf783a0d1920e6da09aec6112ba850f579 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Fri, 29 Nov 2024 15:14:22 +0100 Subject: [PATCH 03/11] fix: remove unused import --- .../src/shared/encryption/encryption.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/profile-sync-controller/src/shared/encryption/encryption.test.ts b/packages/profile-sync-controller/src/shared/encryption/encryption.test.ts index ad66168954..aa63566ad5 100644 --- a/packages/profile-sync-controller/src/shared/encryption/encryption.test.ts +++ b/packages/profile-sync-controller/src/shared/encryption/encryption.test.ts @@ -2,7 +2,6 @@ import encryption, { createSHA256Hash } from './encryption'; describe('encryption tests', () => { const PASSWORD = '123'; - const PASSWORD2 = '1234'; const DATA1 = 'Hello World'; const DATA2 = JSON.stringify({ foo: 'bar' }); From 2c3923bf158041cc89705e33157334c80c4869e7 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Fri, 29 Nov 2024 15:52:40 +0100 Subject: [PATCH 04/11] fix: remove unused param --- .../src/controllers/user-storage/services.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/profile-sync-controller/src/controllers/user-storage/services.test.ts b/packages/profile-sync-controller/src/controllers/user-storage/services.test.ts index ebb41e17a7..1e46723cda 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/services.test.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/services.test.ts @@ -156,7 +156,7 @@ describe('user-storage/services.ts - getUserStorageAllFeatureEntries() tests', ( HashedKey: 'entry2', Data: '{"v":"1","t":"scrypt","d":"3ioo9bxhjDjTmJWIGQMnOlnfa4ysuUNeLYTTmJ+qrq7gwI6hURH3ooUcBldJkHtvuQ==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}', }, - await createMockGetStorageResponse('data3'), + await createMockGetStorageResponse(), ]; const mockGetUserStorageAllFeatureEntries = From 97c860b5840579d2df14885c084a9a2afefbef61 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Fri, 29 Nov 2024 16:40:04 +0100 Subject: [PATCH 05/11] fix: new method in order to batch upsert already hashed and encrypted entries --- .../controllers/user-storage/services.test.ts | 74 +++++++++++++++++++ .../src/controllers/user-storage/services.ts | 42 ++++++++++- .../src/sdk/user-storage.ts | 46 +++++++++++- 3 files changed, 159 insertions(+), 3 deletions(-) diff --git a/packages/profile-sync-controller/src/controllers/user-storage/services.test.ts b/packages/profile-sync-controller/src/controllers/user-storage/services.test.ts index 1e46723cda..ea090ebe90 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/services.test.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/services.test.ts @@ -25,6 +25,7 @@ import { upsertUserStorage, deleteUserStorageAllFeatureEntries, deleteUserStorage, + batchUpsertUserStorageWithAlreadyHashedAndEncryptedEntries, } from './services'; describe('user-storage/services.ts - getUserStorage() tests', () => { @@ -375,6 +376,79 @@ describe('user-storage/services.ts - batchUpsertUserStorage() tests', () => { }); }); +describe('user-storage/services.ts - batchUpsertUserStorageWithAlreadyHashedAndEncryptedEntries() tests', () => { + let dataToStore: [string, string][]; + const getDataToStore = async (): Promise<[string, string][]> => + (dataToStore ??= [ + [ + createSHA256Hash(`0x123${MOCK_STORAGE_KEY}`), + await encryption.encryptString(MOCK_STORAGE_DATA, MOCK_STORAGE_KEY), + ], + [ + createSHA256Hash(`0x456${MOCK_STORAGE_KEY}`), + await encryption.encryptString(MOCK_STORAGE_DATA, MOCK_STORAGE_KEY), + ], + ]); + + const actCallBatchUpsertUserStorage = async () => { + return await batchUpsertUserStorageWithAlreadyHashedAndEncryptedEntries( + await getDataToStore(), + { + bearerToken: 'MOCK_BEARER_TOKEN', + path: USER_STORAGE_FEATURE_NAMES.accounts, + storageKey: MOCK_STORAGE_KEY, + }, + ); + }; + + it('invokes upsert endpoint with no errors', async () => { + const mockUpsertUserStorage = mockEndpointBatchUpsertUserStorage( + USER_STORAGE_FEATURE_NAMES.accounts, + undefined, + async (_uri, requestBody) => { + if (typeof requestBody === 'string') { + return; + } + + const expectedBody = Object.fromEntries(await getDataToStore()); + + expect(requestBody.data).toStrictEqual(expectedBody); + }, + ); + + await actCallBatchUpsertUserStorage(); + + expect(mockUpsertUserStorage.isDone()).toBe(true); + }); + + it('throws error if unable to upsert user storage', async () => { + const mockUpsertUserStorage = mockEndpointBatchUpsertUserStorage( + USER_STORAGE_FEATURE_NAMES.accounts, + { + status: 500, + }, + ); + + await expect(actCallBatchUpsertUserStorage()).rejects.toThrow( + expect.any(Error), + ); + mockUpsertUserStorage.done(); + }); + + it('does nothing if empty data is provided', async () => { + const mockUpsertUserStorage = + mockEndpointBatchUpsertUserStorage('accounts_v2'); + + await batchUpsertUserStorage([], { + bearerToken: 'MOCK_BEARER_TOKEN', + path: 'accounts_v2', + storageKey: MOCK_STORAGE_KEY, + }); + + expect(mockUpsertUserStorage.isDone()).toBe(false); + }); +}); + describe('user-storage/services.ts - deleteUserStorage() tests', () => { const actCallDeleteUserStorage = async () => { return await deleteUserStorage({ diff --git a/packages/profile-sync-controller/src/controllers/user-storage/services.ts b/packages/profile-sync-controller/src/controllers/user-storage/services.ts index cc4c995bde..3251ebf713 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/services.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/services.ts @@ -93,7 +93,7 @@ export async function getUserStorage( nativeScryptCrypto, ); - // Re-encrypt the entry if the salt is non-empty + // Re-encrypt and re-upload the entry if the salt is non-empty const salt = encryption.getSalt(encryptedData); if (salt.length) { await upsertUserStorage(decryptedData, opts); @@ -178,7 +178,10 @@ export async function getUserStorageAllFeatureEntries( // Re-upload the re-encrypted entries if (reEncryptedEntries.length) { - await batchUpsertUserStorage(reEncryptedEntries, opts); + await batchUpsertUserStorageWithAlreadyHashedAndEncryptedEntries( + reEncryptedEntries, + opts, + ); } return decryptedData; @@ -266,6 +269,41 @@ export async function batchUpsertUserStorage( } } +/** + * User Storage Service - Set multiple storage entries for one specific feature. + * You cannot use this method to set multiple features at once. + * + * @param encryptedData - data to store, in the form of an array of [hashedKey, encryptedData] pairs + * @param opts - storage options + */ +export async function batchUpsertUserStorageWithAlreadyHashedAndEncryptedEntries( + encryptedData: [string, string][], + opts: UserStorageBatchUpsertOptions, +): Promise { + if (!encryptedData.length) { + return; + } + + const { bearerToken, path } = opts; + + const url = new URL(`${USER_STORAGE_ENDPOINT}/${path}`); + + const formattedData = Object.fromEntries(encryptedData); + + const res = await fetch(url.toString(), { + method: 'PUT', + headers: { + 'Content-Type': 'application/json', + Authorization: `Bearer ${bearerToken}`, + }, + body: JSON.stringify({ data: formattedData }), + }); + + if (!res.ok) { + throw new Error('user-storage - unable to batch upsert data'); + } +} + /** * User Storage Service - Delete Storage Entry. * diff --git a/packages/profile-sync-controller/src/sdk/user-storage.ts b/packages/profile-sync-controller/src/sdk/user-storage.ts index 6deb4aa326..85e6221e26 100644 --- a/packages/profile-sync-controller/src/sdk/user-storage.ts +++ b/packages/profile-sync-controller/src/sdk/user-storage.ts @@ -200,6 +200,47 @@ export class UserStorage { } } + async #batchUpsertUserStorageWithAlreadyHashedAndEncryptedEntries( + path: UserStoragePathWithFeatureOnly, + encryptedData: [string, string][], + ): Promise { + try { + if (!encryptedData.length) { + return; + } + + const headers = await this.#getAuthorizationHeader(); + + const url = new URL(STORAGE_URL(this.env, path)); + + const response = await fetch(url.toString(), { + method: 'PUT', + headers: { + 'Content-Type': 'application/json', + ...headers, + }, + body: JSON.stringify({ data: Object.fromEntries(encryptedData) }), + }); + + if (!response.ok) { + const responseBody: ErrorMessage = await response.json().catch(() => ({ + message: 'unknown', + error: 'unknown', + })); + throw new Error( + `HTTP error message: ${responseBody.message}, error: ${responseBody.error}`, + ); + } + } catch (e) { + /* istanbul ignore next */ + const errorMessage = + e instanceof Error ? e.message : JSON.stringify(e ?? ''); + throw new UserStorageError( + `failed to batch upsert user storage for path '${path}'. ${errorMessage}`, + ); + } + } + async #getUserStorage( path: UserStoragePathWithFeatureAndKey, ): Promise { @@ -319,7 +360,10 @@ export class UserStorage { // Re-upload the re-encrypted entries if (reEncryptedEntries.length) { - await this.#batchUpsertUserStorage(path, reEncryptedEntries); + await this.#batchUpsertUserStorageWithAlreadyHashedAndEncryptedEntries( + path, + reEncryptedEntries, + ); } return decryptedData; From e6004fd59c088a1cc5ee2429b4ffc8bcddd734b9 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Fri, 29 Nov 2024 17:14:32 +0100 Subject: [PATCH 06/11] fix: PR feedbacks & improvements --- .../controllers/user-storage/services.test.ts | 18 +++++---- .../src/sdk/user-storage.test.ts | 8 ++-- .../src/shared/encryption/cache.ts | 7 ++-- .../src/shared/encryption/encryption.test.ts | 39 +++++++++++++++++++ .../src/shared/encryption/encryption.ts | 25 ++++++++++-- .../src/shared/encryption/utils.test.ts | 33 ---------------- .../src/shared/encryption/utils.ts | 34 ---------------- 7 files changed, 79 insertions(+), 85 deletions(-) delete mode 100644 packages/profile-sync-controller/src/shared/encryption/utils.test.ts diff --git a/packages/profile-sync-controller/src/controllers/user-storage/services.test.ts b/packages/profile-sync-controller/src/controllers/user-storage/services.test.ts index ea090ebe90..da661f7e72 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/services.test.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/services.test.ts @@ -1,5 +1,4 @@ import encryption, { createSHA256Hash } from '../../shared/encryption'; -import { getIfEntriesHaveDifferentSalts } from '../../shared/encryption/utils'; import type { UserStorageFeatureKeys } from '../../shared/storage-schema'; import { USER_STORAGE_FEATURE_NAMES } from '../../shared/storage-schema'; import { createMockGetStorageResponse } from './__fixtures__'; @@ -86,12 +85,13 @@ describe('user-storage/services.ts - getUserStorage() tests', () => { }); it('re-encrypts data if received entry was encrypted with a non-empty salt, and saves it back to user storage', async () => { - // This corresponds to 'data1' - // Encrypted with a non-empty salt - const mockResponse = { + const DECRYPED_DATA = 'data1'; + const INITIAL_ENCRYPTED_DATA = { HashedKey: 'entry1', Data: '{"v":"1","t":"scrypt","d":"HIu+WgFBCtKo6rEGy0R8h8t/JgXhzC2a3AF6epahGY2h6GibXDKxSBf6ppxM099Gmg==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}', }; + // Encrypted with a non-empty salt + const mockResponse = INITIAL_ENCRYPTED_DATA; const mockGetUserStorage = await mockEndpointGetUserStorage( `${USER_STORAGE_FEATURE_NAMES.notifications}.notification_settings`, @@ -113,6 +113,7 @@ describe('user-storage/services.ts - getUserStorage() tests', () => { encryption.getSalt(requestBody.data).length === 0; expect(isNewSaltEmpty).toBe(true); + expect(JSON.parse(requestBody.data).saltLen).toBe(0); }, ); @@ -120,7 +121,7 @@ describe('user-storage/services.ts - getUserStorage() tests', () => { mockGetUserStorage.done(); mockUpsertUserStorage.done(); - expect(result).toBe('data1'); + expect(result).toBe(DECRYPED_DATA); }); }); @@ -177,9 +178,10 @@ describe('user-storage/services.ts - getUserStorageAllFeatureEntries() tests', ( return; } - const doEntriesHaveDifferentSalts = getIfEntriesHaveDifferentSalts( - Object.entries(requestBody.data).map((entry) => entry[1] as string), - ); + const doEntriesHaveDifferentSalts = + encryption.getIfEntriesHaveDifferentSalts( + Object.entries(requestBody.data).map((entry) => entry[1] as string), + ); expect(doEntriesHaveDifferentSalts).toBe(false); diff --git a/packages/profile-sync-controller/src/sdk/user-storage.test.ts b/packages/profile-sync-controller/src/sdk/user-storage.test.ts index 921425c700..39e6dc7dec 100644 --- a/packages/profile-sync-controller/src/sdk/user-storage.test.ts +++ b/packages/profile-sync-controller/src/sdk/user-storage.test.ts @@ -1,5 +1,4 @@ import encryption, { createSHA256Hash } from '../shared/encryption'; -import { getIfEntriesHaveDifferentSalts } from '../shared/encryption/utils'; import { Env } from '../shared/env'; import type { UserStorageFeatureKeys } from '../shared/storage-schema'; import { USER_STORAGE_FEATURE_NAMES } from '../shared/storage-schema'; @@ -170,9 +169,10 @@ describe('User Storage', () => { return; } - const doEntriesHaveDifferentSalts = getIfEntriesHaveDifferentSalts( - Object.entries(requestBody.data).map((entry) => entry[1] as string), - ); + const doEntriesHaveDifferentSalts = + encryption.getIfEntriesHaveDifferentSalts( + Object.entries(requestBody.data).map((entry) => entry[1] as string), + ); expect(doEntriesHaveDifferentSalts).toBe(false); diff --git a/packages/profile-sync-controller/src/shared/encryption/cache.ts b/packages/profile-sync-controller/src/shared/encryption/cache.ts index d7e5d16f0e..d77b8e22c4 100644 --- a/packages/profile-sync-controller/src/shared/encryption/cache.ts +++ b/packages/profile-sync-controller/src/shared/encryption/cache.ts @@ -47,12 +47,13 @@ export function getCachedKeyBySalt( } /** - * Gets any cached key for a given hashed password + * Gets the cached key that was generated without a salt, if it exists. + * This is unique per hashed password. * * @param hashedPassword - hashed password for cache lookup - * @returns any (the first) cached key + * @returns the cached key */ -export function getAnyCachedKey( +export function getCachedKeyGeneratedWithoutSalt( hashedPassword: string, ): CachedEntry | undefined { const cache = getPasswordCache(hashedPassword); diff --git a/packages/profile-sync-controller/src/shared/encryption/encryption.test.ts b/packages/profile-sync-controller/src/shared/encryption/encryption.test.ts index aa63566ad5..434706b58d 100644 --- a/packages/profile-sync-controller/src/shared/encryption/encryption.test.ts +++ b/packages/profile-sync-controller/src/shared/encryption/encryption.test.ts @@ -57,6 +57,13 @@ describe('encryption tests', () => { expect(salt).toStrictEqual(saltUsedToPreviouslyEncryptData); }); + it('should throw an error when trying to get the salt from an unsupported encrypted string', () => { + const encryptedData = `{"v":"1","t":"unsupported","d":"d9k8wRtOOq97OyNqRNnTCa3ct7+z9nRjV75Am+ND9yMoV/bMcnrzZqO2EhjL3viJyA==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}`; + expect(() => encryption.getSalt(encryptedData)).toThrow( + `Unsupported encrypted data payload - ${encryptedData}`, + ); + }); + it('should be able to decrypt an entry that has no salt', async () => { const encryptedData = await encryption.encryptString(DATA1, PASSWORD); const decryptedData = await encryption.decryptString( @@ -73,4 +80,36 @@ describe('encryption tests', () => { ); expect(decryptedData).toBe(ACCOUNTS_DECRYPTED_DATA); }); + + describe('getIfEntriesHaveDifferentSalts()', () => { + it('should return true if entries have different salts', () => { + const entries = [ + '{"v":"1","t":"scrypt","d":"1yC/ZXarV57HbqEZ46nH0JWgXfPl86nTHD7kai2g5gm290FM9tw5QjOaAAwIuQESEE8TIM/J9pIj7nmlGi+BZrevTtK3DXWXwnUQsCP7amKd5Q4gs3EEQgXpA0W+WJUgyElj869rwIv/C6tl5E2pK4j/0EAjMSIm1TGoj9FPohyRgZsOIt8VhZfb7w0GODsjPwPIkN6zazvJ3gAFYFPh7yRtebFs86z3fzqCWZ9zakdCHntchC2oZiaApXR9yzaPlGgnPg==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}', + '{"v":"1","t":"scrypt","d":"x7QqsdqsdEtUo7q/jG+UNkD/HOxQARGGRXsGPrLsDlkwDfgfoYlPI0To/M3pJRBlKD0RLEFIPHtHBEA5bv/2izB21VljvhMnhHfo0KgQ+e8Uq1t7grwa+r+ge3qbPNY+w78Xt8GtC+Hkrw5fORKvCn+xjzaCHYV6RxKYbp1TpyCJq7hDrr1XiyL8kqbpE0hAHALrrQOoV9/WXJi9pC5J118kquXx8CNA1P5wO/BXKp1AbryGR6kVW3lsp1sy3lYE/TApa5lTj+","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}', + ]; + + const result = encryption.getIfEntriesHaveDifferentSalts(entries); + expect(result).toBe(true); + }); + + it('should return false if entries have the same salts', () => { + const entries = [ + '{"v":"1","t":"scrypt","d":"+nhJkMMjQljyyyytsnhO4dIzFL/hGR4Y6hb2qUGrPb/hjxHVJUk1jcJAyHP9eUzgZQ==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}', + '{"v":"1","t":"scrypt","d":"+nhJkMMjQljyyyytsnhO4XYxpF0N3IXuhCpPM9dAyw5pO2gcqcXNucJs60rBtgKttA==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}', + ]; + + const result = encryption.getIfEntriesHaveDifferentSalts(entries); + expect(result).toBe(false); + }); + + it('should return false if entries do not have salts', () => { + const entries = [ + '{"v":"1","t":"scrypt","d":"CgHcOM6xCaaNFnPCr0etqyxCq4xoJNQ9gfP9+GRn94hGtKurbOuXzyDoHJgzaJxDKd1zQHJhDwLjnH6oCZvC8XKvZZ6RcrN9BicZHpzpojon+HwpcPHceM/pvoMabYfiXqbokYHXZymGTxE5X+TjFo+HB7/Y6xOCU1usz47bru9vfyZrdQ66qGlMO2MUFx00cnh8xHOksDNC","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":0}', + '{"v":"1","t":"scrypt","d":"OCrYnCFkt7a33cjaAL65D/WypM+oVxIiGVwMk+mjijcpnG4r3vzPl6OzFpx2LNKHj6YN59wcLje3QK2hISU0R8iXyZubdkeAiY89SsI7owLda96ysF+q6PuyxnWfNfWe+5a1+4O8BVkR8p/9PYimwTN0QGhX2lkfLt5r0aYgsLnWld/5k9G7cB4yqoduIopzpojS5ZGI8PFW","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":0}', + ]; + + const result = encryption.getIfEntriesHaveDifferentSalts(entries); + expect(result).toBe(false); + }); + }); }); diff --git a/packages/profile-sync-controller/src/shared/encryption/encryption.ts b/packages/profile-sync-controller/src/shared/encryption/encryption.ts index b0a83fcfa6..984741722b 100644 --- a/packages/profile-sync-controller/src/shared/encryption/encryption.ts +++ b/packages/profile-sync-controller/src/shared/encryption/encryption.ts @@ -5,7 +5,11 @@ import { sha256 } from '@noble/hashes/sha256'; import { utf8ToBytes, concatBytes, bytesToHex } from '@noble/hashes/utils'; import type { NativeScrypt } from '../types/encryption'; -import { getAnyCachedKey, getCachedKeyBySalt, setCachedKey } from './cache'; +import { + getCachedKeyBySalt, + getCachedKeyGeneratedWithoutSalt, + setCachedKey, +} from './cache'; import { base64ToByteArray, byteArrayToBase64, @@ -193,10 +197,25 @@ class EncryptorDecryptor { ); } catch (e) { const errorMessage = e instanceof Error ? e.message : JSON.stringify(e); - throw new Error(`Unable to decrypt string - ${errorMessage}`); + throw new Error(`Unable to get salt - ${errorMessage}`); } } + getIfEntriesHaveDifferentSalts(entries: string[]): boolean { + const salts = entries + .map((e) => { + try { + return this.getSalt(e); + } catch { + return undefined; + } + }) + .filter((s): s is Uint8Array => s !== undefined); + + const strSet = new Set(salts.map((arr) => arr.toString())); + return strSet.size === salts.length; + } + #encrypt(plaintext: Uint8Array, key: Uint8Array): Uint8Array { const nonce = randomBytes(ALGORITHM_NONCE_SIZE); @@ -227,7 +246,7 @@ class EncryptorDecryptor { const hashedPassword = createSHA256Hash(password); const cachedKey = salt ? getCachedKeyBySalt(hashedPassword, salt) - : getAnyCachedKey(hashedPassword); + : getCachedKeyGeneratedWithoutSalt(hashedPassword); if (cachedKey) { return { diff --git a/packages/profile-sync-controller/src/shared/encryption/utils.test.ts b/packages/profile-sync-controller/src/shared/encryption/utils.test.ts deleted file mode 100644 index d9374e3c53..0000000000 --- a/packages/profile-sync-controller/src/shared/encryption/utils.test.ts +++ /dev/null @@ -1,33 +0,0 @@ -import { getIfEntriesHaveDifferentSalts } from './utils'; - -describe('utils - getIfEntriesHaveDifferentSalts()', () => { - it('should return true if entries have different salts', () => { - const entries = [ - '{"v":"1","t":"scrypt","d":"1yC/ZXarV57HbqEZ46nH0JWgXfPl86nTHD7kai2g5gm290FM9tw5QjOaAAwIuQESEE8TIM/J9pIj7nmlGi+BZrevTtK3DXWXwnUQsCP7amKd5Q4gs3EEQgXpA0W+WJUgyElj869rwIv/C6tl5E2pK4j/0EAjMSIm1TGoj9FPohyRgZsOIt8VhZfb7w0GODsjPwPIkN6zazvJ3gAFYFPh7yRtebFs86z3fzqCWZ9zakdCHntchC2oZiaApXR9yzaPlGgnPg==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}', - '{"v":"1","t":"scrypt","d":"x7QqsdqsdEtUo7q/jG+UNkD/HOxQARGGRXsGPrLsDlkwDfgfoYlPI0To/M3pJRBlKD0RLEFIPHtHBEA5bv/2izB21VljvhMnhHfo0KgQ+e8Uq1t7grwa+r+ge3qbPNY+w78Xt8GtC+Hkrw5fORKvCn+xjzaCHYV6RxKYbp1TpyCJq7hDrr1XiyL8kqbpE0hAHALrrQOoV9/WXJi9pC5J118kquXx8CNA1P5wO/BXKp1AbryGR6kVW3lsp1sy3lYE/TApa5lTj+","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}', - ]; - - const result = getIfEntriesHaveDifferentSalts(entries); - expect(result).toBe(true); - }); - - it('should return false if entries have the same salts', () => { - const entries = [ - '{"v":"1","t":"scrypt","d":"+nhJkMMjQljyyyytsnhO4dIzFL/hGR4Y6hb2qUGrPb/hjxHVJUk1jcJAyHP9eUzgZQ==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}', - '{"v":"1","t":"scrypt","d":"+nhJkMMjQljyyyytsnhO4XYxpF0N3IXuhCpPM9dAyw5pO2gcqcXNucJs60rBtgKttA==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}', - ]; - - const result = getIfEntriesHaveDifferentSalts(entries); - expect(result).toBe(false); - }); - - it('should return false if entries do not have salts', () => { - const entries = [ - '{"v":"1","t":"scrypt","d":"CgHcOM6xCaaNFnPCr0etqyxCq4xoJNQ9gfP9+GRn94hGtKurbOuXzyDoHJgzaJxDKd1zQHJhDwLjnH6oCZvC8XKvZZ6RcrN9BicZHpzpojon+HwpcPHceM/pvoMabYfiXqbokYHXZymGTxE5X+TjFo+HB7/Y6xOCU1usz47bru9vfyZrdQ66qGlMO2MUFx00cnh8xHOksDNC","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":0}', - '{"v":"1","t":"scrypt","d":"OCrYnCFkt7a33cjaAL65D/WypM+oVxIiGVwMk+mjijcpnG4r3vzPl6OzFpx2LNKHj6YN59wcLje3QK2hISU0R8iXyZubdkeAiY89SsI7owLda96ysF+q6PuyxnWfNfWe+5a1+4O8BVkR8p/9PYimwTN0QGhX2lkfLt5r0aYgsLnWld/5k9G7cB4yqoduIopzpojS5ZGI8PFW","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":0}', - ]; - - const result = getIfEntriesHaveDifferentSalts(entries); - expect(result).toBe(false); - }); -}); diff --git a/packages/profile-sync-controller/src/shared/encryption/utils.ts b/packages/profile-sync-controller/src/shared/encryption/utils.ts index 7e21665244..c9caa55fe8 100644 --- a/packages/profile-sync-controller/src/shared/encryption/utils.ts +++ b/packages/profile-sync-controller/src/shared/encryption/utils.ts @@ -1,5 +1,3 @@ -import encryption from './encryption'; - export const byteArrayToBase64 = (byteArray: Uint8Array) => { return Buffer.from(byteArray).toString('base64'); }; @@ -16,35 +14,3 @@ export const bytesToUtf8 = (byteArray: Uint8Array) => { export const stringToByteArray = (str: string) => { return new TextEncoder().encode(str); }; - -export const areAllUInt8ArraysUnique = (arrays: Uint8Array[]) => { - const seen = new Set(); - for (const arr of arrays) { - const str = arr.toString(); - if (seen.has(str)) { - return false; - } - seen.add(str); - } - return true; -}; - -/** - * Returns a boolean indicating if the entries have different salts. - * - * @param entries - User Storage Entries - * @returns A boolean indicating if the entries have different salts. - */ -export function getIfEntriesHaveDifferentSalts(entries: string[]): boolean { - const salts = entries - .map((e) => { - try { - return encryption.getSalt(e); - } catch { - return undefined; - } - }) - .filter((s): s is Uint8Array => s !== undefined); - - return areAllUInt8ArraysUnique(salts); -} From e67adad3433d61d169d2eee49c2965552145841f Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Fri, 29 Nov 2024 18:09:30 +0100 Subject: [PATCH 07/11] fix: use a shared salt instead of an empty one --- .../controllers/user-storage/services.test.ts | 26 +++++++++-------- .../src/controllers/user-storage/services.ts | 9 +++--- .../src/sdk/user-storage.test.ts | 25 +++++++++-------- .../src/sdk/user-storage.ts | 7 +++-- .../src/shared/encryption/cache.ts | 10 ++++--- .../src/shared/encryption/constants.ts | 14 ++++++++++ .../src/shared/encryption/encryption.ts | 28 ++++++++----------- 7 files changed, 69 insertions(+), 50 deletions(-) create mode 100644 packages/profile-sync-controller/src/shared/encryption/constants.ts diff --git a/packages/profile-sync-controller/src/controllers/user-storage/services.test.ts b/packages/profile-sync-controller/src/controllers/user-storage/services.test.ts index da661f7e72..ec2b698b0c 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/services.test.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/services.test.ts @@ -1,4 +1,5 @@ import encryption, { createSHA256Hash } from '../../shared/encryption'; +import { SHARED_SALT } from '../../shared/encryption/constants'; import type { UserStorageFeatureKeys } from '../../shared/storage-schema'; import { USER_STORAGE_FEATURE_NAMES } from '../../shared/storage-schema'; import { createMockGetStorageResponse } from './__fixtures__'; @@ -84,13 +85,13 @@ describe('user-storage/services.ts - getUserStorage() tests', () => { expect(result).toBeNull(); }); - it('re-encrypts data if received entry was encrypted with a non-empty salt, and saves it back to user storage', async () => { + it('re-encrypts data if received entry was encrypted with a random salt, and saves it back to user storage', async () => { const DECRYPED_DATA = 'data1'; const INITIAL_ENCRYPTED_DATA = { HashedKey: 'entry1', Data: '{"v":"1","t":"scrypt","d":"HIu+WgFBCtKo6rEGy0R8h8t/JgXhzC2a3AF6epahGY2h6GibXDKxSBf6ppxM099Gmg==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}', }; - // Encrypted with a non-empty salt + // Encrypted with a random salt const mockResponse = INITIAL_ENCRYPTED_DATA; const mockGetUserStorage = await mockEndpointGetUserStorage( @@ -109,11 +110,11 @@ describe('user-storage/services.ts - getUserStorage() tests', () => { return; } - const isNewSaltEmpty = - encryption.getSalt(requestBody.data).length === 0; + const isEncryptedUsingSharedSalt = + encryption.getSalt(requestBody.data).toString() === + SHARED_SALT.toString(); - expect(isNewSaltEmpty).toBe(true); - expect(JSON.parse(requestBody.data).saltLen).toBe(0); + expect(isEncryptedUsingSharedSalt).toBe(true); }, ); @@ -145,10 +146,10 @@ describe('user-storage/services.ts - getUserStorageAllFeatureEntries() tests', ( expect(result).toStrictEqual([MOCK_STORAGE_DATA]); }); - it('re-encrypts data if received entries were encrypted with non-empty salts, and saves it back to user storage', async () => { + it('re-encrypts data if received entries were encrypted with random salts, and saves it back to user storage', async () => { // This corresponds to [['entry1', 'data1'], ['entry2', 'data2'], ['HASHED_KEY', '{ "hello": "world" }']] - // Each entry has been encrypted with a non-empty salt, except for the last entry - // The last entry is used to test if the function can handle entries with either empty or non-empty salts + // Each entry has been encrypted with a random salt, except for the last entry + // The last entry is used to test if the function can handle entries with both random salts and the shared salt const mockResponse = [ { HashedKey: 'entry1', @@ -185,12 +186,13 @@ describe('user-storage/services.ts - getUserStorageAllFeatureEntries() tests', ( expect(doEntriesHaveDifferentSalts).toBe(false); - const doEntriesHaveEmptySalts = Object.entries(requestBody.data).every( + const doEntriesUseSharedSalt = Object.entries(requestBody.data).every( ([_entryKey, entryValue]) => - encryption.getSalt(entryValue as string).length === 0, + encryption.getSalt(entryValue as string).toString() === + SHARED_SALT.toString(), ); - expect(doEntriesHaveEmptySalts).toBe(true); + expect(doEntriesUseSharedSalt).toBe(true); const wereOnlyNonEmptySaltEntriesUploaded = Object.entries(requestBody.data).length === 2; diff --git a/packages/profile-sync-controller/src/controllers/user-storage/services.ts b/packages/profile-sync-controller/src/controllers/user-storage/services.ts index 3251ebf713..04032396c2 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/services.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/services.ts @@ -1,6 +1,7 @@ import log from 'loglevel'; import encryption, { createSHA256Hash } from '../../shared/encryption'; +import { SHARED_SALT } from '../../shared/encryption/constants'; import { Env, getEnvUrls } from '../../shared/env'; import type { UserStoragePathWithFeatureAndKey, @@ -93,9 +94,9 @@ export async function getUserStorage( nativeScryptCrypto, ); - // Re-encrypt and re-upload the entry if the salt is non-empty + // Re-encrypt and re-upload the entry if the salt is random const salt = encryption.getSalt(encryptedData); - if (salt.length) { + if (salt.toString() !== SHARED_SALT.toString()) { await upsertUserStorage(decryptedData, opts); } @@ -159,9 +160,9 @@ export async function getUserStorageAllFeatureEntries( ); decryptedData.push(data); - // Re-encrypt the entry if the salt is non-empty + // Re-encrypt the entry if the salt is different from the shared one const salt = encryption.getSalt(entry.Data); - if (salt.length) { + if (salt.toString() !== SHARED_SALT.toString()) { reEncryptedEntries.push([ entry.HashedKey, await encryption.encryptString( diff --git a/packages/profile-sync-controller/src/sdk/user-storage.test.ts b/packages/profile-sync-controller/src/sdk/user-storage.test.ts index 39e6dc7dec..fb2c7dbb35 100644 --- a/packages/profile-sync-controller/src/sdk/user-storage.test.ts +++ b/packages/profile-sync-controller/src/sdk/user-storage.test.ts @@ -1,4 +1,5 @@ import encryption, { createSHA256Hash } from '../shared/encryption'; +import { SHARED_SALT } from '../shared/encryption/constants'; import { Env } from '../shared/env'; import type { UserStorageFeatureKeys } from '../shared/storage-schema'; import { USER_STORAGE_FEATURE_NAMES } from '../shared/storage-schema'; @@ -88,12 +89,12 @@ describe('User Storage', () => { expect(response).toBe(data); }); - it('re-encrypts data if received entry was encrypted with a non-empty salt, and saves it back to user storage', async () => { + it('re-encrypts data if received entry was encrypted with a random salt, and saves it back to user storage', async () => { const { auth } = arrangeAuth('SRP', MOCK_SRP); const { userStorage } = arrangeUserStorage(auth); // This corresponds to 'data1' - // Encrypted with a non-empty salt + // Encrypted with a random salt const mockResponse = { HashedKey: 'entry1', Data: '{"v":"1","t":"scrypt","d":"HIu+WgFBCtKo6rEGy0R8h8t/JgXhzC2a3AF6epahGY2h6GibXDKxSBf6ppxM099Gmg==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}', @@ -110,10 +111,11 @@ describe('User Storage', () => { return; } - const isNewSaltEmpty = - encryption.getSalt(requestBody.data).length === 0; + const isEncryptedUsingSharedSalt = + encryption.getSalt(requestBody.data).toString() === + SHARED_SALT.toString(); - expect(isNewSaltEmpty).toBe(true); + expect(isEncryptedUsingSharedSalt).toBe(true); }, ); @@ -138,10 +140,10 @@ describe('User Storage', () => { expect(responseAllFeatureEntries).toStrictEqual([data]); }); - it('re-encrypts data if received entries were encrypted with non-empty salts, and saves it back to user storage', async () => { + it('re-encrypts data if received entries were encrypted with random salts, and saves it back to user storage', async () => { // This corresponds to [['entry1', 'data1'], ['entry2', 'data2'], ['HASHED_KEY', '{ "hello": "world" }']] - // Each entry has been encrypted with a non-empty salt, except for the last entry - // The last entry is used to test if the function can handle entries with either empty or non-empty salts + // Each entry has been encrypted with a random salt, except for the last entry + // The last entry is used to test if the function can handle payloads that contain both random salts and the shared salt const mockResponse = [ { HashedKey: 'entry1', @@ -176,12 +178,13 @@ describe('User Storage', () => { expect(doEntriesHaveDifferentSalts).toBe(false); - const doEntriesHaveEmptySalts = Object.entries(requestBody.data).every( + const doEntriesUseSharedSalt = Object.entries(requestBody.data).every( ([_entryKey, entryValue]) => - encryption.getSalt(entryValue as string).length === 0, + encryption.getSalt(entryValue as string).toString() === + SHARED_SALT.toString(), ); - expect(doEntriesHaveEmptySalts).toBe(true); + expect(doEntriesUseSharedSalt).toBe(true); const wereOnlyNonEmptySaltEntriesUploaded = Object.entries(requestBody.data).length === 2; diff --git a/packages/profile-sync-controller/src/sdk/user-storage.ts b/packages/profile-sync-controller/src/sdk/user-storage.ts index 85e6221e26..4ef8b49b5a 100644 --- a/packages/profile-sync-controller/src/sdk/user-storage.ts +++ b/packages/profile-sync-controller/src/sdk/user-storage.ts @@ -1,4 +1,5 @@ import encryption, { createSHA256Hash } from '../shared/encryption'; +import { SHARED_SALT } from '../shared/encryption/constants'; import type { Env } from '../shared/env'; import { getEnvUrls } from '../shared/env'; import type { @@ -277,7 +278,7 @@ export class UserStorage { storageKey, ); - // Re-encrypt the entry if the salt is non-empty + // Re-encrypt the entry if it was encrypted with a random salt const salt = encryption.getSalt(encryptedData); if (salt.length) { await this.#upsertUserStorage(path, decryptedData); @@ -345,9 +346,9 @@ export class UserStorage { const data = await encryption.decryptString(entry.Data, storageKey); decryptedData.push(data); - // Re-encrypt the entry if the salt is non-empty + // Re-encrypt the entry was encrypted with a random salt const salt = encryption.getSalt(entry.Data); - if (salt.length) { + if (salt.toString() !== SHARED_SALT.toString()) { reEncryptedEntries.push([ entry.HashedKey, await encryption.encryptString(data, storageKey), diff --git a/packages/profile-sync-controller/src/shared/encryption/cache.ts b/packages/profile-sync-controller/src/shared/encryption/cache.ts index d77b8e22c4..7552ef60d1 100644 --- a/packages/profile-sync-controller/src/shared/encryption/cache.ts +++ b/packages/profile-sync-controller/src/shared/encryption/cache.ts @@ -1,3 +1,4 @@ +import { SHARED_SALT } from './constants'; import { byteArrayToBase64 } from './utils'; type CachedEntry = { @@ -53,19 +54,20 @@ export function getCachedKeyBySalt( * @param hashedPassword - hashed password for cache lookup * @returns the cached key */ -export function getCachedKeyGeneratedWithoutSalt( +export function getCachedKeyGeneratedWithSharedSalt( hashedPassword: string, ): CachedEntry | undefined { const cache = getPasswordCache(hashedPassword); - const cachedKey = cache.get(''); + const base64Salt = byteArrayToBase64(SHARED_SALT); + const cachedKey = cache.get(base64Salt); if (!cachedKey) { return undefined; } return { - salt: new Uint8Array(), - base64Salt: '', + salt: SHARED_SALT, + base64Salt, key: cachedKey, }; } diff --git a/packages/profile-sync-controller/src/shared/encryption/constants.ts b/packages/profile-sync-controller/src/shared/encryption/constants.ts new file mode 100644 index 0000000000..ccb9588210 --- /dev/null +++ b/packages/profile-sync-controller/src/shared/encryption/constants.ts @@ -0,0 +1,14 @@ +// Nonce/Key Sizes +export const ALGORITHM_NONCE_SIZE = 12; // 12 bytes +export const ALGORITHM_KEY_SIZE = 16; // 16 bytes + +// Scrypt settings +// see: https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html#scrypt +export const SCRYPT_SALT_SIZE = 16; // 16 bytes +export const SCRYPT_N = 2 ** 17; // CPU/memory cost parameter (must be a power of 2, > 1) +// eslint-disable-next-line @typescript-eslint/naming-convention +export const SCRYPT_r = 8; // Block size parameter +// eslint-disable-next-line @typescript-eslint/naming-convention +export const SCRYPT_p = 1; // Parallelization parameter + +export const SHARED_SALT = new Uint8Array([...Array(SCRYPT_SALT_SIZE).keys()]); diff --git a/packages/profile-sync-controller/src/shared/encryption/encryption.ts b/packages/profile-sync-controller/src/shared/encryption/encryption.ts index 984741722b..7b36a15e64 100644 --- a/packages/profile-sync-controller/src/shared/encryption/encryption.ts +++ b/packages/profile-sync-controller/src/shared/encryption/encryption.ts @@ -7,9 +7,18 @@ import { utf8ToBytes, concatBytes, bytesToHex } from '@noble/hashes/utils'; import type { NativeScrypt } from '../types/encryption'; import { getCachedKeyBySalt, - getCachedKeyGeneratedWithoutSalt, + getCachedKeyGeneratedWithSharedSalt, setCachedKey, } from './cache'; +import { + ALGORITHM_KEY_SIZE, + ALGORITHM_NONCE_SIZE, + SCRYPT_N, + SCRYPT_p, + SCRYPT_r, + SCRYPT_SALT_SIZE, + SHARED_SALT, +} from './constants'; import { base64ToByteArray, byteArrayToBase64, @@ -40,19 +49,6 @@ export type EncryptedPayload = { saltLen: number; }; -// Nonce/Key Sizes -const ALGORITHM_NONCE_SIZE = 12; // 12 bytes -const ALGORITHM_KEY_SIZE = 16; // 16 bytes - -// Scrypt settings -// see: https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html#scrypt -const SCRYPT_SALT_SIZE = 0; // We don't use a salt -const SCRYPT_N = 2 ** 17; // CPU/memory cost parameter (must be a power of 2, > 1) -// eslint-disable-next-line @typescript-eslint/naming-convention -const SCRYPT_r = 8; // Block size parameter -// eslint-disable-next-line @typescript-eslint/naming-convention -const SCRYPT_p = 1; // Parallelization parameter - class EncryptorDecryptor { async encryptString( plaintext: string, @@ -246,7 +242,7 @@ class EncryptorDecryptor { const hashedPassword = createSHA256Hash(password); const cachedKey = salt ? getCachedKeyBySalt(hashedPassword, salt) - : getCachedKeyGeneratedWithoutSalt(hashedPassword); + : getCachedKeyGeneratedWithSharedSalt(hashedPassword); if (cachedKey) { return { @@ -255,7 +251,7 @@ class EncryptorDecryptor { }; } - const newSalt = salt ?? new Uint8Array(SCRYPT_SALT_SIZE); + const newSalt = salt ?? SHARED_SALT; let newKey: Uint8Array; From 9f5ef506a04a0526fd84a0c58d2959994b2ddd4f Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Mon, 2 Dec 2024 14:19:51 +0100 Subject: [PATCH 08/11] un-1337 the UInt8Array construction --- .../src/shared/encryption/constants.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/profile-sync-controller/src/shared/encryption/constants.ts b/packages/profile-sync-controller/src/shared/encryption/constants.ts index ccb9588210..b39d1aa8a9 100644 --- a/packages/profile-sync-controller/src/shared/encryption/constants.ts +++ b/packages/profile-sync-controller/src/shared/encryption/constants.ts @@ -11,4 +11,6 @@ export const SCRYPT_r = 8; // Block size parameter // eslint-disable-next-line @typescript-eslint/naming-convention export const SCRYPT_p = 1; // Parallelization parameter -export const SHARED_SALT = new Uint8Array([...Array(SCRYPT_SALT_SIZE).keys()]); +export const SHARED_SALT = new Uint8Array([ + 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, +]); From 7d3a5ee2198781104bbc8c550dd50d511cebdfb9 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Mon, 2 Dec 2024 15:27:09 +0100 Subject: [PATCH 09/11] fix: salt comparison condition --- packages/profile-sync-controller/src/sdk/user-storage.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/profile-sync-controller/src/sdk/user-storage.ts b/packages/profile-sync-controller/src/sdk/user-storage.ts index 4ef8b49b5a..ad2c52e39f 100644 --- a/packages/profile-sync-controller/src/sdk/user-storage.ts +++ b/packages/profile-sync-controller/src/sdk/user-storage.ts @@ -280,7 +280,7 @@ export class UserStorage { // Re-encrypt the entry if it was encrypted with a random salt const salt = encryption.getSalt(encryptedData); - if (salt.length) { + if (salt.toString() !== SHARED_SALT.toString()) { await this.#upsertUserStorage(path, decryptedData); } From 32621e11fabbb1b69be1e59501cc96e444a7177d Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Tue, 3 Dec 2024 12:40:21 +0100 Subject: [PATCH 10/11] feat: delete entries from user storage if un-decryptable --- .../controllers/user-storage/services.test.ts | 148 +++++++++++++++++- .../src/controllers/user-storage/services.ts | 77 +++++++-- .../src/sdk/__fixtures__/mock-userstorage.ts | 10 +- .../src/sdk/user-storage.test.ts | 72 +++++++++ .../src/sdk/user-storage.ts | 88 +++++++++-- 5 files changed, 357 insertions(+), 38 deletions(-) diff --git a/packages/profile-sync-controller/src/controllers/user-storage/services.test.ts b/packages/profile-sync-controller/src/controllers/user-storage/services.test.ts index ec2b698b0c..cb21b1aaf1 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/services.test.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/services.test.ts @@ -26,6 +26,7 @@ import { deleteUserStorageAllFeatureEntries, deleteUserStorage, batchUpsertUserStorageWithAlreadyHashedAndEncryptedEntries, + batchDeleteUserStorageWithAlreadyHashedEntries, } from './services'; describe('user-storage/services.ts - getUserStorage() tests', () => { @@ -124,6 +125,30 @@ describe('user-storage/services.ts - getUserStorage() tests', () => { mockUpsertUserStorage.done(); expect(result).toBe(DECRYPED_DATA); }); + + it('deletes entry if unable to decrypt data', async () => { + const badResponseData: GetUserStorageResponse = { + HashedKey: 'MOCK_HASH', + Data: 'Bad Encrypted Data', + }; + const mockGetUserStorage = await mockEndpointGetUserStorage( + `${USER_STORAGE_FEATURE_NAMES.notifications}.notification_settings`, + { + status: 200, + body: badResponseData, + }, + ); + + const mockDeleteUserStorage = mockEndpointDeleteUserStorage( + `${USER_STORAGE_FEATURE_NAMES.notifications}.notification_settings`, + ); + + const result = await actCallGetUserStorage(); + + mockGetUserStorage.done(); + expect(mockDeleteUserStorage.isDone()).toBe(true); + expect(result).toBeNull(); + }); }); describe('user-storage/services.ts - getUserStorageAllFeatureEntries() tests', () => { @@ -208,6 +233,61 @@ describe('user-storage/services.ts - getUserStorageAllFeatureEntries() tests', ( expect(result).toStrictEqual(['data1', 'data2', MOCK_STORAGE_DATA]); }); + it('deletes entries if unable to decrypt data', async () => { + // This corresponds to [['entry1', 'data1'], ['entry2', 'data2'], ['HASHED_KEY', 'Bad Encrypted Data']] + // Each entry has been encrypted with a random salt, except for the last entry + // The last entry is used to test if the function can handle entries with both random salts and the shared salt, mixed with bad data + const mockResponse = [ + { + HashedKey: 'entry1', + Data: '{"v":"1","t":"scrypt","d":"HIu+WgFBCtKo6rEGy0R8h8t/JgXhzC2a3AF6epahGY2h6GibXDKxSBf6ppxM099Gmg==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}', + }, + { + HashedKey: 'entry2', + Data: '{"v":"1","t":"scrypt","d":"3ioo9bxhjDjTmJWIGQMnOlnfa4ysuUNeLYTTmJ+qrq7gwI6hURH3ooUcBldJkHtvuQ==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}', + }, + { + HashedKey: 'HASHED_KEY', + Data: 'Bad Encrypted Data', + }, + ]; + + const mockGetUserStorageAllFeatureEntries = + await mockEndpointGetUserStorageAllFeatureEntries( + USER_STORAGE_FEATURE_NAMES.notifications, + { + status: 200, + body: JSON.stringify(mockResponse), + }, + ); + + const mockBatchUpsertUserStorage = mockEndpointBatchUpsertUserStorage( + USER_STORAGE_FEATURE_NAMES.notifications, + ); + + const mockBatchDeleteUserStorage = mockEndpointBatchDeleteUserStorage( + USER_STORAGE_FEATURE_NAMES.notifications, + undefined, + async (_uri, requestBody) => { + console.log('requestBody', requestBody); + if (typeof requestBody === 'string') { + return; + } + + const expectedBody = ['HASHED_KEY']; + + expect(requestBody.batch_delete).toStrictEqual(expectedBody); + }, + ); + + const result = await actCallGetUserStorageAllFeatureEntries(); + + mockGetUserStorageAllFeatureEntries.done(); + expect(mockBatchUpsertUserStorage.isDone()).toBe(true); + expect(mockBatchDeleteUserStorage.isDone()).toBe(true); + expect(result).toStrictEqual(['data1', 'data2']); + }); + it('returns null if endpoint does not have entry', async () => { const mockGetUserStorage = await mockEndpointGetUserStorageAllFeatureEntries( @@ -624,12 +704,74 @@ describe('user-storage/services.ts - batchDeleteUserStorage() tests', () => { }); it('does nothing if empty data is provided', async () => { - const mockDeleteUserStorage = - mockEndpointBatchDeleteUserStorage('accounts_v2'); + const mockDeleteUserStorage = mockEndpointBatchDeleteUserStorage( + USER_STORAGE_FEATURE_NAMES.accounts, + ); await batchDeleteUserStorage([], { bearerToken: 'MOCK_BEARER_TOKEN', - path: 'accounts_v2', + path: USER_STORAGE_FEATURE_NAMES.accounts, + storageKey: MOCK_STORAGE_KEY, + }); + + expect(mockDeleteUserStorage.isDone()).toBe(false); + }); +}); + +describe('user-storage/services.ts - batchDeleteUserStorageWithAlreadyHashedEntries() tests', () => { + const keysToDelete = [ + createSHA256Hash(`0x123${MOCK_STORAGE_KEY}`), + createSHA256Hash(`0x456${MOCK_STORAGE_KEY}`), + ]; + + const actCallBatchDeleteUserStorage = async () => { + return await batchDeleteUserStorageWithAlreadyHashedEntries(keysToDelete, { + bearerToken: 'MOCK_BEARER_TOKEN', + path: USER_STORAGE_FEATURE_NAMES.accounts, + storageKey: MOCK_STORAGE_KEY, + }); + }; + + it('invokes upsert endpoint with no errors', async () => { + const mockDeleteUserStorage = mockEndpointBatchDeleteUserStorage( + USER_STORAGE_FEATURE_NAMES.accounts, + undefined, + async (_uri, requestBody) => { + if (typeof requestBody === 'string') { + return; + } + + expect(requestBody.batch_delete).toStrictEqual(keysToDelete); + }, + ); + + await actCallBatchDeleteUserStorage(); + + expect(mockDeleteUserStorage.isDone()).toBe(true); + }); + + it('throws error if unable to upsert user storage', async () => { + const mockDeleteUserStorage = mockEndpointBatchDeleteUserStorage( + USER_STORAGE_FEATURE_NAMES.accounts, + { + status: 500, + }, + ); + + await expect(actCallBatchDeleteUserStorage()).rejects.toThrow( + expect.any(Error), + ); + mockDeleteUserStorage.done(); + }); + + it('does nothing if empty data is provided', async () => { + const mockDeleteUserStorage = mockEndpointBatchDeleteUserStorage( + USER_STORAGE_FEATURE_NAMES.accounts, + ); + + await batchDeleteUserStorage([], { + bearerToken: 'MOCK_BEARER_TOKEN', + path: USER_STORAGE_FEATURE_NAMES.accounts, storageKey: MOCK_STORAGE_KEY, }); diff --git a/packages/profile-sync-controller/src/controllers/user-storage/services.ts b/packages/profile-sync-controller/src/controllers/user-storage/services.ts index 04032396c2..41890d4e8a 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/services.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/services.ts @@ -88,19 +88,26 @@ export async function getUserStorage( return null; } - const decryptedData = await encryption.decryptString( - encryptedData, - opts.storageKey, - nativeScryptCrypto, - ); - - // Re-encrypt and re-upload the entry if the salt is random - const salt = encryption.getSalt(encryptedData); - if (salt.toString() !== SHARED_SALT.toString()) { - await upsertUserStorage(decryptedData, opts); - } + try { + const decryptedData = await encryption.decryptString( + encryptedData, + opts.storageKey, + nativeScryptCrypto, + ); - return decryptedData; + // Re-encrypt and re-upload the entry if the salt is random + const salt = encryption.getSalt(encryptedData); + if (salt.toString() !== SHARED_SALT.toString()) { + await upsertUserStorage(decryptedData, opts); + } + + return decryptedData; + } catch { + // If the data cannot be decrypted, delete it from user storage + await deleteUserStorage(opts); + + return null; + } } catch (e) { log.error('Failed to get user storage', e); return null; @@ -145,6 +152,7 @@ export async function getUserStorageAllFeatureEntries( const decryptedData: string[] = []; const reEncryptedEntries: [string, string][] = []; + const entriesToDelete: string[] = []; for (const entry of userStorage) { /* istanbul ignore if - unreachable if statement, but kept as edge case */ @@ -173,7 +181,8 @@ export async function getUserStorageAllFeatureEntries( ]); } } catch { - // do nothing + // If the data cannot be decrypted, delete it from user storage + entriesToDelete.push(entry.HashedKey); } } @@ -185,6 +194,14 @@ export async function getUserStorageAllFeatureEntries( ); } + // Delete the entries that cannot be decrypted + if (entriesToDelete.length) { + await batchDeleteUserStorageWithAlreadyHashedEntries( + entriesToDelete, + opts, + ); + } + return decryptedData; } catch (e) { log.error('Failed to get user storage', e); @@ -334,6 +351,40 @@ export async function deleteUserStorage( } } +/** + * User Storage Service - Delete multiple storage entries for one specific feature. + * You cannot use this method to delete multiple features at once. + * + * @param encryptedData - data to delete, in the form of an array hashedKey[] + * @param opts - storage options + */ +export async function batchDeleteUserStorageWithAlreadyHashedEntries( + encryptedData: string[], + opts: UserStorageBatchUpsertOptions, +): Promise { + if (!encryptedData.length) { + return; + } + + const { bearerToken, path } = opts; + + const url = new URL(`${USER_STORAGE_ENDPOINT}/${path}`); + + const res = await fetch(url.toString(), { + method: 'PUT', + headers: { + 'Content-Type': 'application/json', + Authorization: `Bearer ${bearerToken}`, + }, + // eslint-disable-next-line @typescript-eslint/naming-convention + body: JSON.stringify({ batch_delete: encryptedData }), + }); + + if (!res.ok) { + throw new Error('user-storage - unable to batch delete data'); + } +} + /** * User Storage Service - Delete multiple storage entries for one specific feature. * You cannot use this method to delete multiple features at once. diff --git a/packages/profile-sync-controller/src/sdk/__fixtures__/mock-userstorage.ts b/packages/profile-sync-controller/src/sdk/__fixtures__/mock-userstorage.ts index 4b7a8fbe44..243d868076 100644 --- a/packages/profile-sync-controller/src/sdk/__fixtures__/mock-userstorage.ts +++ b/packages/profile-sync-controller/src/sdk/__fixtures__/mock-userstorage.ts @@ -41,7 +41,6 @@ export const handleMockUserStorageGet = async (mockReply?: MockReply) => { body: await MOCK_STORAGE_RESPONSE(), }; const mockEndpoint = nock(MOCK_STORAGE_URL) - .persist() .get(/.*/u) .reply(reply.status, reply.body); @@ -56,7 +55,6 @@ export const handleMockUserStorageGetAllFeatureEntries = async ( body: [await MOCK_STORAGE_RESPONSE()], }; const mockEndpoint = nock(MOCK_STORAGE_URL_ALL_FEATURE_ENTRIES) - .persist() .get('') .reply(reply.status, reply.body); @@ -69,7 +67,6 @@ export const handleMockUserStoragePut = ( ) => { const reply = mockReply ?? { status: 204 }; const mockEndpoint = nock(MOCK_STORAGE_URL) - .persist() .put(/.*/u) .reply(reply.status, async (uri, requestBody) => { return await callback?.(uri, requestBody); @@ -84,7 +81,6 @@ export const handleMockUserStorageBatchDelete = ( ) => { const reply = mockReply ?? { status: 204 }; const mockEndpoint = nock(MOCK_STORAGE_URL) - .persist() .put(/.*/u) .reply(reply.status, async (uri, requestBody) => { return await callback?.(uri, requestBody); @@ -95,10 +91,7 @@ export const handleMockUserStorageBatchDelete = ( export const handleMockUserStorageDelete = async (mockReply?: MockReply) => { const reply = mockReply ?? { status: 204 }; - const mockEndpoint = nock(MOCK_STORAGE_URL) - .persist() - .delete(/.*/u) - .reply(reply.status); + const mockEndpoint = nock(MOCK_STORAGE_URL).delete(/.*/u).reply(reply.status); return mockEndpoint; }; @@ -108,7 +101,6 @@ export const handleMockUserStorageDeleteAllFeatureEntries = async ( ) => { const reply = mockReply ?? { status: 204 }; const mockEndpoint = nock(MOCK_STORAGE_URL_ALL_FEATURE_ENTRIES) - .persist() .delete('') .reply(reply.status); diff --git a/packages/profile-sync-controller/src/sdk/user-storage.test.ts b/packages/profile-sync-controller/src/sdk/user-storage.test.ts index fb2c7dbb35..51c3f41f4e 100644 --- a/packages/profile-sync-controller/src/sdk/user-storage.test.ts +++ b/packages/profile-sync-controller/src/sdk/user-storage.test.ts @@ -126,6 +126,28 @@ describe('User Storage', () => { expect(mockPut.isDone()).toBe(true); }); + it('deletes entry if unable to decrypt data', async () => { + const { auth } = arrangeAuth('SRP', MOCK_SRP); + const { userStorage } = arrangeUserStorage(auth); + + const mockGet = await handleMockUserStorageGet({ + status: 200, + body: JSON.stringify({ + HashedKey: 'entry1', + Data: 'invalid-encrypted-data', + }), + }); + + const mockDelete = await handleMockUserStorageDelete(); + + const result = await userStorage.getItem( + `${USER_STORAGE_FEATURE_NAMES.notifications}.notification_settings`, + ); + expect(mockGet.isDone()).toBe(true); + expect(mockDelete.isDone()).toBe(true); + expect(result).toBeNull(); + }); + it('gets all feature entries', async () => { const { auth } = arrangeAuth('SRP', MOCK_SRP); const { userStorage } = arrangeUserStorage(auth); @@ -200,6 +222,56 @@ describe('User Storage', () => { expect(mockPut.isDone()).toBe(true); }); + it('deletes entries if unable to decrypt data', async () => { + // This corresponds to [['entry1', 'data1'], ['entry2', 'invalid-encrypted-data'], ['HASHED_KEY', 'data3']] + // Each entry has been encrypted with a random salt, except for the last entry + // The last entry is used to test if the function can handle entries with both random salts and the shared salt, mixed with invalid data + + const mockResponse = [ + { + HashedKey: 'entry1', + Data: '{"v":"1","t":"scrypt","d":"HIu+WgFBCtKo6rEGy0R8h8t/JgXhzC2a3AF6epahGY2h6GibXDKxSBf6ppxM099Gmg==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}', + }, + { + HashedKey: 'entry2', + Data: 'invalid-encrypted-data', + }, + await MOCK_STORAGE_RESPONSE('data3'), + ]; + + const { auth } = arrangeAuth('SRP', MOCK_SRP); + const { userStorage } = arrangeUserStorage(auth); + + const mockGetAll = await handleMockUserStorageGetAllFeatureEntries({ + status: 200, + body: mockResponse, + }); + + const mockPut = handleMockUserStoragePut(); + + const mockDelete = handleMockUserStorageBatchDelete( + undefined, + async (_, requestBody) => { + if (typeof requestBody === 'string') { + return; + } + + const expectedBody = ['entry2']; + + expect(requestBody.batch_delete).toStrictEqual(expectedBody); + }, + ); + + const result = await userStorage.getAllFeatureItems( + USER_STORAGE_FEATURE_NAMES.notifications, + ); + + expect(mockGetAll.isDone()).toBe(true); + expect(mockPut.isDone()).toBe(true); + expect(mockDelete.isDone()).toBe(true); + expect(result).toStrictEqual(['data1', 'data3']); + }); + it('batch set items', async () => { const dataToStore: [ UserStorageFeatureKeys, diff --git a/packages/profile-sync-controller/src/sdk/user-storage.ts b/packages/profile-sync-controller/src/sdk/user-storage.ts index ad2c52e39f..f70d19b3ad 100644 --- a/packages/profile-sync-controller/src/sdk/user-storage.ts +++ b/packages/profile-sync-controller/src/sdk/user-storage.ts @@ -68,7 +68,9 @@ export class UserStorage { await this.#batchUpsertUserStorage(path, values); } - async getItem(path: UserStoragePathWithFeatureAndKey): Promise { + async getItem( + path: UserStoragePathWithFeatureAndKey, + ): Promise { return this.#getUserStorage(path); } @@ -244,7 +246,7 @@ export class UserStorage { async #getUserStorage( path: UserStoragePathWithFeatureAndKey, - ): Promise { + ): Promise { try { const headers = await this.#getAuthorizationHeader(); const storageKey = await this.getStorageKey(); @@ -273,18 +275,26 @@ export class UserStorage { } const { Data: encryptedData } = await response.json(); - const decryptedData = await encryption.decryptString( - encryptedData, - storageKey, - ); - // Re-encrypt the entry if it was encrypted with a random salt - const salt = encryption.getSalt(encryptedData); - if (salt.toString() !== SHARED_SALT.toString()) { - await this.#upsertUserStorage(path, decryptedData); - } + try { + const decryptedData = await encryption.decryptString( + encryptedData, + storageKey, + ); - return decryptedData; + // Re-encrypt the entry if it was encrypted with a random salt + const salt = encryption.getSalt(encryptedData); + if (salt.toString() !== SHARED_SALT.toString()) { + await this.#upsertUserStorage(path, decryptedData); + } + + return decryptedData; + } catch { + // If the data cannot be decrypted, delete it from user storage + await this.#deleteUserStorage(path); + + return null; + } } catch (e) { if (e instanceof NotFoundError) { throw e; @@ -336,6 +346,7 @@ export class UserStorage { const decryptedData: string[] = []; const reEncryptedEntries: [string, string][] = []; + const entriesToDelete: string[] = []; for (const entry of userStorage) { if (!entry.Data) { @@ -355,7 +366,8 @@ export class UserStorage { ]); } } catch { - // do nothing + // If the data cannot be decrypted, delete it from user storage + entriesToDelete.push(entry.HashedKey); } } @@ -367,6 +379,14 @@ export class UserStorage { ); } + // Delete entries that could not be decrypted + if (entriesToDelete.length) { + await this.#batchDeleteUserStorageWithAlreadyHashedEntries( + path, + entriesToDelete, + ); + } + return decryptedData; } catch (e) { if (e instanceof NotFoundError) { @@ -516,6 +536,48 @@ export class UserStorage { } } + async #batchDeleteUserStorageWithAlreadyHashedEntries( + path: UserStoragePathWithFeatureOnly, + encryptedData: string[], + ): Promise { + try { + if (!encryptedData.length) { + return; + } + + const headers = await this.#getAuthorizationHeader(); + + const url = new URL(STORAGE_URL(this.env, path)); + + const response = await fetch(url.toString(), { + method: 'PUT', + headers: { + 'Content-Type': 'application/json', + ...headers, + }, + // eslint-disable-next-line @typescript-eslint/naming-convention + body: JSON.stringify({ batch_delete: encryptedData }), + }); + + if (!response.ok) { + const responseBody: ErrorMessage = await response.json().catch(() => ({ + message: 'unknown', + error: 'unknown', + })); + throw new Error( + `HTTP error message: ${responseBody.message}, error: ${responseBody.error}`, + ); + } + } catch (e) { + /* istanbul ignore next */ + const errorMessage = + e instanceof Error ? e.message : JSON.stringify(e ?? ''); + throw new UserStorageError( + `failed to batch delete user storage for path '${path}'. ${errorMessage}`, + ); + } + } + #createEntryKey(key: string, storageKey: string): string { const hashedKey = createSHA256Hash(key + storageKey); return hashedKey; From 035dc217eeed1f300cc3ce05cbf4fbe684e2526f Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Tue, 3 Dec 2024 14:54:47 +0100 Subject: [PATCH 11/11] Revert "feat: delete entries from user storage if un-decryptable" This reverts commit 32621e11fabbb1b69be1e59501cc96e444a7177d. --- .../controllers/user-storage/services.test.ts | 148 +----------------- .../src/controllers/user-storage/services.ts | 77 ++------- .../src/sdk/__fixtures__/mock-userstorage.ts | 10 +- .../src/sdk/user-storage.test.ts | 72 --------- .../src/sdk/user-storage.ts | 88 ++--------- 5 files changed, 38 insertions(+), 357 deletions(-) diff --git a/packages/profile-sync-controller/src/controllers/user-storage/services.test.ts b/packages/profile-sync-controller/src/controllers/user-storage/services.test.ts index cb21b1aaf1..ec2b698b0c 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/services.test.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/services.test.ts @@ -26,7 +26,6 @@ import { deleteUserStorageAllFeatureEntries, deleteUserStorage, batchUpsertUserStorageWithAlreadyHashedAndEncryptedEntries, - batchDeleteUserStorageWithAlreadyHashedEntries, } from './services'; describe('user-storage/services.ts - getUserStorage() tests', () => { @@ -125,30 +124,6 @@ describe('user-storage/services.ts - getUserStorage() tests', () => { mockUpsertUserStorage.done(); expect(result).toBe(DECRYPED_DATA); }); - - it('deletes entry if unable to decrypt data', async () => { - const badResponseData: GetUserStorageResponse = { - HashedKey: 'MOCK_HASH', - Data: 'Bad Encrypted Data', - }; - const mockGetUserStorage = await mockEndpointGetUserStorage( - `${USER_STORAGE_FEATURE_NAMES.notifications}.notification_settings`, - { - status: 200, - body: badResponseData, - }, - ); - - const mockDeleteUserStorage = mockEndpointDeleteUserStorage( - `${USER_STORAGE_FEATURE_NAMES.notifications}.notification_settings`, - ); - - const result = await actCallGetUserStorage(); - - mockGetUserStorage.done(); - expect(mockDeleteUserStorage.isDone()).toBe(true); - expect(result).toBeNull(); - }); }); describe('user-storage/services.ts - getUserStorageAllFeatureEntries() tests', () => { @@ -233,61 +208,6 @@ describe('user-storage/services.ts - getUserStorageAllFeatureEntries() tests', ( expect(result).toStrictEqual(['data1', 'data2', MOCK_STORAGE_DATA]); }); - it('deletes entries if unable to decrypt data', async () => { - // This corresponds to [['entry1', 'data1'], ['entry2', 'data2'], ['HASHED_KEY', 'Bad Encrypted Data']] - // Each entry has been encrypted with a random salt, except for the last entry - // The last entry is used to test if the function can handle entries with both random salts and the shared salt, mixed with bad data - const mockResponse = [ - { - HashedKey: 'entry1', - Data: '{"v":"1","t":"scrypt","d":"HIu+WgFBCtKo6rEGy0R8h8t/JgXhzC2a3AF6epahGY2h6GibXDKxSBf6ppxM099Gmg==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}', - }, - { - HashedKey: 'entry2', - Data: '{"v":"1","t":"scrypt","d":"3ioo9bxhjDjTmJWIGQMnOlnfa4ysuUNeLYTTmJ+qrq7gwI6hURH3ooUcBldJkHtvuQ==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}', - }, - { - HashedKey: 'HASHED_KEY', - Data: 'Bad Encrypted Data', - }, - ]; - - const mockGetUserStorageAllFeatureEntries = - await mockEndpointGetUserStorageAllFeatureEntries( - USER_STORAGE_FEATURE_NAMES.notifications, - { - status: 200, - body: JSON.stringify(mockResponse), - }, - ); - - const mockBatchUpsertUserStorage = mockEndpointBatchUpsertUserStorage( - USER_STORAGE_FEATURE_NAMES.notifications, - ); - - const mockBatchDeleteUserStorage = mockEndpointBatchDeleteUserStorage( - USER_STORAGE_FEATURE_NAMES.notifications, - undefined, - async (_uri, requestBody) => { - console.log('requestBody', requestBody); - if (typeof requestBody === 'string') { - return; - } - - const expectedBody = ['HASHED_KEY']; - - expect(requestBody.batch_delete).toStrictEqual(expectedBody); - }, - ); - - const result = await actCallGetUserStorageAllFeatureEntries(); - - mockGetUserStorageAllFeatureEntries.done(); - expect(mockBatchUpsertUserStorage.isDone()).toBe(true); - expect(mockBatchDeleteUserStorage.isDone()).toBe(true); - expect(result).toStrictEqual(['data1', 'data2']); - }); - it('returns null if endpoint does not have entry', async () => { const mockGetUserStorage = await mockEndpointGetUserStorageAllFeatureEntries( @@ -704,74 +624,12 @@ describe('user-storage/services.ts - batchDeleteUserStorage() tests', () => { }); it('does nothing if empty data is provided', async () => { - const mockDeleteUserStorage = mockEndpointBatchDeleteUserStorage( - USER_STORAGE_FEATURE_NAMES.accounts, - ); - - await batchDeleteUserStorage([], { - bearerToken: 'MOCK_BEARER_TOKEN', - path: USER_STORAGE_FEATURE_NAMES.accounts, - storageKey: MOCK_STORAGE_KEY, - }); - - expect(mockDeleteUserStorage.isDone()).toBe(false); - }); -}); - -describe('user-storage/services.ts - batchDeleteUserStorageWithAlreadyHashedEntries() tests', () => { - const keysToDelete = [ - createSHA256Hash(`0x123${MOCK_STORAGE_KEY}`), - createSHA256Hash(`0x456${MOCK_STORAGE_KEY}`), - ]; - - const actCallBatchDeleteUserStorage = async () => { - return await batchDeleteUserStorageWithAlreadyHashedEntries(keysToDelete, { - bearerToken: 'MOCK_BEARER_TOKEN', - path: USER_STORAGE_FEATURE_NAMES.accounts, - storageKey: MOCK_STORAGE_KEY, - }); - }; - - it('invokes upsert endpoint with no errors', async () => { - const mockDeleteUserStorage = mockEndpointBatchDeleteUserStorage( - USER_STORAGE_FEATURE_NAMES.accounts, - undefined, - async (_uri, requestBody) => { - if (typeof requestBody === 'string') { - return; - } - - expect(requestBody.batch_delete).toStrictEqual(keysToDelete); - }, - ); - - await actCallBatchDeleteUserStorage(); - - expect(mockDeleteUserStorage.isDone()).toBe(true); - }); - - it('throws error if unable to upsert user storage', async () => { - const mockDeleteUserStorage = mockEndpointBatchDeleteUserStorage( - USER_STORAGE_FEATURE_NAMES.accounts, - { - status: 500, - }, - ); - - await expect(actCallBatchDeleteUserStorage()).rejects.toThrow( - expect.any(Error), - ); - mockDeleteUserStorage.done(); - }); - - it('does nothing if empty data is provided', async () => { - const mockDeleteUserStorage = mockEndpointBatchDeleteUserStorage( - USER_STORAGE_FEATURE_NAMES.accounts, - ); + const mockDeleteUserStorage = + mockEndpointBatchDeleteUserStorage('accounts_v2'); await batchDeleteUserStorage([], { bearerToken: 'MOCK_BEARER_TOKEN', - path: USER_STORAGE_FEATURE_NAMES.accounts, + path: 'accounts_v2', storageKey: MOCK_STORAGE_KEY, }); diff --git a/packages/profile-sync-controller/src/controllers/user-storage/services.ts b/packages/profile-sync-controller/src/controllers/user-storage/services.ts index 41890d4e8a..04032396c2 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/services.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/services.ts @@ -88,26 +88,19 @@ export async function getUserStorage( return null; } - try { - const decryptedData = await encryption.decryptString( - encryptedData, - opts.storageKey, - nativeScryptCrypto, - ); - - // Re-encrypt and re-upload the entry if the salt is random - const salt = encryption.getSalt(encryptedData); - if (salt.toString() !== SHARED_SALT.toString()) { - await upsertUserStorage(decryptedData, opts); - } - - return decryptedData; - } catch { - // If the data cannot be decrypted, delete it from user storage - await deleteUserStorage(opts); - - return null; + const decryptedData = await encryption.decryptString( + encryptedData, + opts.storageKey, + nativeScryptCrypto, + ); + + // Re-encrypt and re-upload the entry if the salt is random + const salt = encryption.getSalt(encryptedData); + if (salt.toString() !== SHARED_SALT.toString()) { + await upsertUserStorage(decryptedData, opts); } + + return decryptedData; } catch (e) { log.error('Failed to get user storage', e); return null; @@ -152,7 +145,6 @@ export async function getUserStorageAllFeatureEntries( const decryptedData: string[] = []; const reEncryptedEntries: [string, string][] = []; - const entriesToDelete: string[] = []; for (const entry of userStorage) { /* istanbul ignore if - unreachable if statement, but kept as edge case */ @@ -181,8 +173,7 @@ export async function getUserStorageAllFeatureEntries( ]); } } catch { - // If the data cannot be decrypted, delete it from user storage - entriesToDelete.push(entry.HashedKey); + // do nothing } } @@ -194,14 +185,6 @@ export async function getUserStorageAllFeatureEntries( ); } - // Delete the entries that cannot be decrypted - if (entriesToDelete.length) { - await batchDeleteUserStorageWithAlreadyHashedEntries( - entriesToDelete, - opts, - ); - } - return decryptedData; } catch (e) { log.error('Failed to get user storage', e); @@ -351,40 +334,6 @@ export async function deleteUserStorage( } } -/** - * User Storage Service - Delete multiple storage entries for one specific feature. - * You cannot use this method to delete multiple features at once. - * - * @param encryptedData - data to delete, in the form of an array hashedKey[] - * @param opts - storage options - */ -export async function batchDeleteUserStorageWithAlreadyHashedEntries( - encryptedData: string[], - opts: UserStorageBatchUpsertOptions, -): Promise { - if (!encryptedData.length) { - return; - } - - const { bearerToken, path } = opts; - - const url = new URL(`${USER_STORAGE_ENDPOINT}/${path}`); - - const res = await fetch(url.toString(), { - method: 'PUT', - headers: { - 'Content-Type': 'application/json', - Authorization: `Bearer ${bearerToken}`, - }, - // eslint-disable-next-line @typescript-eslint/naming-convention - body: JSON.stringify({ batch_delete: encryptedData }), - }); - - if (!res.ok) { - throw new Error('user-storage - unable to batch delete data'); - } -} - /** * User Storage Service - Delete multiple storage entries for one specific feature. * You cannot use this method to delete multiple features at once. diff --git a/packages/profile-sync-controller/src/sdk/__fixtures__/mock-userstorage.ts b/packages/profile-sync-controller/src/sdk/__fixtures__/mock-userstorage.ts index 243d868076..4b7a8fbe44 100644 --- a/packages/profile-sync-controller/src/sdk/__fixtures__/mock-userstorage.ts +++ b/packages/profile-sync-controller/src/sdk/__fixtures__/mock-userstorage.ts @@ -41,6 +41,7 @@ export const handleMockUserStorageGet = async (mockReply?: MockReply) => { body: await MOCK_STORAGE_RESPONSE(), }; const mockEndpoint = nock(MOCK_STORAGE_URL) + .persist() .get(/.*/u) .reply(reply.status, reply.body); @@ -55,6 +56,7 @@ export const handleMockUserStorageGetAllFeatureEntries = async ( body: [await MOCK_STORAGE_RESPONSE()], }; const mockEndpoint = nock(MOCK_STORAGE_URL_ALL_FEATURE_ENTRIES) + .persist() .get('') .reply(reply.status, reply.body); @@ -67,6 +69,7 @@ export const handleMockUserStoragePut = ( ) => { const reply = mockReply ?? { status: 204 }; const mockEndpoint = nock(MOCK_STORAGE_URL) + .persist() .put(/.*/u) .reply(reply.status, async (uri, requestBody) => { return await callback?.(uri, requestBody); @@ -81,6 +84,7 @@ export const handleMockUserStorageBatchDelete = ( ) => { const reply = mockReply ?? { status: 204 }; const mockEndpoint = nock(MOCK_STORAGE_URL) + .persist() .put(/.*/u) .reply(reply.status, async (uri, requestBody) => { return await callback?.(uri, requestBody); @@ -91,7 +95,10 @@ export const handleMockUserStorageBatchDelete = ( export const handleMockUserStorageDelete = async (mockReply?: MockReply) => { const reply = mockReply ?? { status: 204 }; - const mockEndpoint = nock(MOCK_STORAGE_URL).delete(/.*/u).reply(reply.status); + const mockEndpoint = nock(MOCK_STORAGE_URL) + .persist() + .delete(/.*/u) + .reply(reply.status); return mockEndpoint; }; @@ -101,6 +108,7 @@ export const handleMockUserStorageDeleteAllFeatureEntries = async ( ) => { const reply = mockReply ?? { status: 204 }; const mockEndpoint = nock(MOCK_STORAGE_URL_ALL_FEATURE_ENTRIES) + .persist() .delete('') .reply(reply.status); diff --git a/packages/profile-sync-controller/src/sdk/user-storage.test.ts b/packages/profile-sync-controller/src/sdk/user-storage.test.ts index 51c3f41f4e..fb2c7dbb35 100644 --- a/packages/profile-sync-controller/src/sdk/user-storage.test.ts +++ b/packages/profile-sync-controller/src/sdk/user-storage.test.ts @@ -126,28 +126,6 @@ describe('User Storage', () => { expect(mockPut.isDone()).toBe(true); }); - it('deletes entry if unable to decrypt data', async () => { - const { auth } = arrangeAuth('SRP', MOCK_SRP); - const { userStorage } = arrangeUserStorage(auth); - - const mockGet = await handleMockUserStorageGet({ - status: 200, - body: JSON.stringify({ - HashedKey: 'entry1', - Data: 'invalid-encrypted-data', - }), - }); - - const mockDelete = await handleMockUserStorageDelete(); - - const result = await userStorage.getItem( - `${USER_STORAGE_FEATURE_NAMES.notifications}.notification_settings`, - ); - expect(mockGet.isDone()).toBe(true); - expect(mockDelete.isDone()).toBe(true); - expect(result).toBeNull(); - }); - it('gets all feature entries', async () => { const { auth } = arrangeAuth('SRP', MOCK_SRP); const { userStorage } = arrangeUserStorage(auth); @@ -222,56 +200,6 @@ describe('User Storage', () => { expect(mockPut.isDone()).toBe(true); }); - it('deletes entries if unable to decrypt data', async () => { - // This corresponds to [['entry1', 'data1'], ['entry2', 'invalid-encrypted-data'], ['HASHED_KEY', 'data3']] - // Each entry has been encrypted with a random salt, except for the last entry - // The last entry is used to test if the function can handle entries with both random salts and the shared salt, mixed with invalid data - - const mockResponse = [ - { - HashedKey: 'entry1', - Data: '{"v":"1","t":"scrypt","d":"HIu+WgFBCtKo6rEGy0R8h8t/JgXhzC2a3AF6epahGY2h6GibXDKxSBf6ppxM099Gmg==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}', - }, - { - HashedKey: 'entry2', - Data: 'invalid-encrypted-data', - }, - await MOCK_STORAGE_RESPONSE('data3'), - ]; - - const { auth } = arrangeAuth('SRP', MOCK_SRP); - const { userStorage } = arrangeUserStorage(auth); - - const mockGetAll = await handleMockUserStorageGetAllFeatureEntries({ - status: 200, - body: mockResponse, - }); - - const mockPut = handleMockUserStoragePut(); - - const mockDelete = handleMockUserStorageBatchDelete( - undefined, - async (_, requestBody) => { - if (typeof requestBody === 'string') { - return; - } - - const expectedBody = ['entry2']; - - expect(requestBody.batch_delete).toStrictEqual(expectedBody); - }, - ); - - const result = await userStorage.getAllFeatureItems( - USER_STORAGE_FEATURE_NAMES.notifications, - ); - - expect(mockGetAll.isDone()).toBe(true); - expect(mockPut.isDone()).toBe(true); - expect(mockDelete.isDone()).toBe(true); - expect(result).toStrictEqual(['data1', 'data3']); - }); - it('batch set items', async () => { const dataToStore: [ UserStorageFeatureKeys, diff --git a/packages/profile-sync-controller/src/sdk/user-storage.ts b/packages/profile-sync-controller/src/sdk/user-storage.ts index f70d19b3ad..ad2c52e39f 100644 --- a/packages/profile-sync-controller/src/sdk/user-storage.ts +++ b/packages/profile-sync-controller/src/sdk/user-storage.ts @@ -68,9 +68,7 @@ export class UserStorage { await this.#batchUpsertUserStorage(path, values); } - async getItem( - path: UserStoragePathWithFeatureAndKey, - ): Promise { + async getItem(path: UserStoragePathWithFeatureAndKey): Promise { return this.#getUserStorage(path); } @@ -246,7 +244,7 @@ export class UserStorage { async #getUserStorage( path: UserStoragePathWithFeatureAndKey, - ): Promise { + ): Promise { try { const headers = await this.#getAuthorizationHeader(); const storageKey = await this.getStorageKey(); @@ -275,26 +273,18 @@ export class UserStorage { } const { Data: encryptedData } = await response.json(); + const decryptedData = await encryption.decryptString( + encryptedData, + storageKey, + ); - try { - const decryptedData = await encryption.decryptString( - encryptedData, - storageKey, - ); - - // Re-encrypt the entry if it was encrypted with a random salt - const salt = encryption.getSalt(encryptedData); - if (salt.toString() !== SHARED_SALT.toString()) { - await this.#upsertUserStorage(path, decryptedData); - } - - return decryptedData; - } catch { - // If the data cannot be decrypted, delete it from user storage - await this.#deleteUserStorage(path); - - return null; + // Re-encrypt the entry if it was encrypted with a random salt + const salt = encryption.getSalt(encryptedData); + if (salt.toString() !== SHARED_SALT.toString()) { + await this.#upsertUserStorage(path, decryptedData); } + + return decryptedData; } catch (e) { if (e instanceof NotFoundError) { throw e; @@ -346,7 +336,6 @@ export class UserStorage { const decryptedData: string[] = []; const reEncryptedEntries: [string, string][] = []; - const entriesToDelete: string[] = []; for (const entry of userStorage) { if (!entry.Data) { @@ -366,8 +355,7 @@ export class UserStorage { ]); } } catch { - // If the data cannot be decrypted, delete it from user storage - entriesToDelete.push(entry.HashedKey); + // do nothing } } @@ -379,14 +367,6 @@ export class UserStorage { ); } - // Delete entries that could not be decrypted - if (entriesToDelete.length) { - await this.#batchDeleteUserStorageWithAlreadyHashedEntries( - path, - entriesToDelete, - ); - } - return decryptedData; } catch (e) { if (e instanceof NotFoundError) { @@ -536,48 +516,6 @@ export class UserStorage { } } - async #batchDeleteUserStorageWithAlreadyHashedEntries( - path: UserStoragePathWithFeatureOnly, - encryptedData: string[], - ): Promise { - try { - if (!encryptedData.length) { - return; - } - - const headers = await this.#getAuthorizationHeader(); - - const url = new URL(STORAGE_URL(this.env, path)); - - const response = await fetch(url.toString(), { - method: 'PUT', - headers: { - 'Content-Type': 'application/json', - ...headers, - }, - // eslint-disable-next-line @typescript-eslint/naming-convention - body: JSON.stringify({ batch_delete: encryptedData }), - }); - - if (!response.ok) { - const responseBody: ErrorMessage = await response.json().catch(() => ({ - message: 'unknown', - error: 'unknown', - })); - throw new Error( - `HTTP error message: ${responseBody.message}, error: ${responseBody.error}`, - ); - } - } catch (e) { - /* istanbul ignore next */ - const errorMessage = - e instanceof Error ? e.message : JSON.stringify(e ?? ''); - throw new UserStorageError( - `failed to batch delete user storage for path '${path}'. ${errorMessage}`, - ); - } - } - #createEntryKey(key: string, storageKey: string): string { const hashedKey = createSHA256Hash(key + storageKey); return hashedKey;