feat: delete entries from user storage if un-decryptable

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

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<void> {
+  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.

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