fix: PR feedbacks & improvements

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,14 +113,15 @@ 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);
       },
     );
 
     const result = await actCallGetUserStorage();
 
     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);
 

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);
 

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);

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);
+    });
+  });
 });

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 {

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<string>();
-  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);
-}