feat: revamp user storage encryption process (#4981)

## Explanation

This PR adds new steps when both `UserStorageController` and SDK
performs:
- `getUserStorageAllFeatureEntries`
- `getUserStorage`

This steps looks for fetched entries' salts and, if random salts are
found, re-encrypts these entries with a constant salt and uploads them
back to user storage.

This PR also removes the salt randomness when generating the keys, by
adding a new shared salt.

This is done to prevent performance issues when decrypting multiple
entries that have different salts in applications using
`UserStorageController` / SDK.

## References

## Changelog

### `@metamask/profile-sync-controller`

- **CHANGED**: Stop using a random salt when generating scrypt keys and
use a shared one
- **ADDED**: Re-encrypt data fetched by
`getUserStorageAllFeatureEntries` and `getUserStorage` with the shared
salt if fetched entries were encrypted with random salts

## Checklist

- [x] I've updated the test suite for new or updated code as appropriate
- [x] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [x] I've highlighted breaking changes using the "BREAKING" category
above as appropriate
- [x] I've prepared draft pull requests for clients and consumer
packages to resolve any breaking changes

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<GetUserStorageAllFeatureEntriesResponse> {
-  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;
 }
 
 /**

packages/profile-sync-controller/src/controllers/user-storage/services.test.ts

@@ -1,6 +1,8 @@
 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__';
 import {
   mockEndpointGetUserStorage,
   mockEndpointUpsertUserStorage,
@@ -23,6 +25,7 @@ import {
   upsertUserStorage,
   deleteUserStorageAllFeatureEntries,
   deleteUserStorage,
+  batchUpsertUserStorageWithAlreadyHashedAndEncryptedEntries,
 } from './services';
 
 describe('user-storage/services.ts - getUserStorage() tests', () => {
@@ -81,6 +84,46 @@ describe('user-storage/services.ts - getUserStorage() tests', () => {
     mockGetUserStorage.done();
     expect(result).toBeNull();
   });
+
+  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 random salt
+    const mockResponse = INITIAL_ENCRYPTED_DATA;
+
+    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 isEncryptedUsingSharedSalt =
+          encryption.getSalt(requestBody.data).toString() ===
+          SHARED_SALT.toString();
+
+        expect(isEncryptedUsingSharedSalt).toBe(true);
+      },
+    );
+
+    const result = await actCallGetUserStorage();
+
+    mockGetUserStorage.done();
+    mockUpsertUserStorage.done();
+    expect(result).toBe(DECRYPED_DATA);
+  });
 });
 
 describe('user-storage/services.ts - getUserStorageAllFeatureEntries() tests', () => {
@@ -103,6 +146,68 @@ describe('user-storage/services.ts - getUserStorageAllFeatureEntries() tests', (
     expect(result).toStrictEqual([MOCK_STORAGE_DATA]);
   });
 
+  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 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',
+        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 createMockGetStorageResponse(),
+    ];
+
+    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 =
+          encryption.getIfEntriesHaveDifferentSalts(
+            Object.entries(requestBody.data).map((entry) => entry[1] as string),
+          );
+
+        expect(doEntriesHaveDifferentSalts).toBe(false);
+
+        const doEntriesUseSharedSalt = Object.entries(requestBody.data).every(
+          ([_entryKey, entryValue]) =>
+            encryption.getSalt(entryValue as string).toString() ===
+            SHARED_SALT.toString(),
+        );
+
+        expect(doEntriesUseSharedSalt).toBe(true);
+
+        const wereOnlyNonEmptySaltEntriesUploaded =
+          Object.entries(requestBody.data).length === 2;
+
+        expect(wereOnlyNonEmptySaltEntriesUploaded).toBe(true);
+      },
+    );
+
+    const result = await actCallGetUserStorageAllFeatureEntries();
+
+    mockGetUserStorageAllFeatureEntries.done();
+    mockBatchUpsertUserStorage.done();
+    expect(result).toStrictEqual(['data1', 'data2', MOCK_STORAGE_DATA]);
+  });
+
   it('returns null if endpoint does not have entry', async () => {
     const mockGetUserStorage =
       await mockEndpointGetUserStorageAllFeatureEntries(
@@ -275,6 +380,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({

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,6 +94,12 @@ export async function getUserStorage(
       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);
@@ -137,6 +144,7 @@ export async function getUserStorageAllFeatureEntries(
     }
 
     const decryptedData: string[] = [];
+    const reEncryptedEntries: [string, string][] = [];
 
     for (const entry of userStorage) {
       /* istanbul ignore if - unreachable if statement, but kept as edge case */
@@ -151,11 +159,32 @@ export async function getUserStorageAllFeatureEntries(
           nativeScryptCrypto,
         );
         decryptedData.push(data);
+
+        // Re-encrypt the entry if the salt is different from the shared one
+        const salt = encryption.getSalt(entry.Data);
+        if (salt.toString() !== SHARED_SALT.toString()) {
+          reEncryptedEntries.push([
+            entry.HashedKey,
+            await encryption.encryptString(
+              data,
+              opts.storageKey,
+              nativeScryptCrypto,
+            ),
+          ]);
+        }
       } catch {
         // do nothing
       }
     }
 
+    // Re-upload the re-encrypted entries
+    if (reEncryptedEntries.length) {
+      await batchUpsertUserStorageWithAlreadyHashedAndEncryptedEntries(
+        reEncryptedEntries,
+        opts,
+      );
+    }
+
     return decryptedData;
   } catch (e) {
     log.error('Failed to get user storage', e);
@@ -241,6 +270,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<void> {
+  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.
  *

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) => {