From ad7148eb622375e276f128cf857141e43ea93dc4 Mon Sep 17 00:00:00 2001 From: Puneet Date: Fri, 17 Apr 2026 23:02:05 +0530 Subject: [PATCH 1/2] fix(secret-v2-bridge): handle personal overrides on move (#4627) --- .../secret-v2-bridge/secret-v2-bridge-dal.ts | 43 ++- .../secret-v2-bridge-move-overrides.test.ts | 274 ++++++++++++++++++ .../secret-v2-bridge-service.ts | 11 + 3 files changed, 327 insertions(+), 1 deletion(-) create mode 100644 backend/src/services/secret-v2-bridge/secret-v2-bridge-move-overrides.test.ts diff --git a/backend/src/services/secret-v2-bridge/secret-v2-bridge-dal.ts b/backend/src/services/secret-v2-bridge/secret-v2-bridge-dal.ts index c01fc9178c0..ebf4a55b99d 100644 --- a/backend/src/services/secret-v2-bridge/secret-v2-bridge-dal.ts +++ b/backend/src/services/secret-v2-bridge/secret-v2-bridge-dal.ts @@ -1174,6 +1174,45 @@ export const secretV2BridgeDALFactory = ({ db, keyStore }: TSecretV2DalArg) => { } }; + const movePersonalOverrides = async ( + sourceFolderId: string, + destinationFolderId: string, + secretKeys: string[], + tx?: Knex + ) => { + if (secretKeys.length === 0) return []; + try { + const movedSecrets = await (tx || db)(TableName.SecretV2) + .where({ + folderId: sourceFolderId, + type: SecretType.Personal + }) + .whereIn("key", secretKeys) + .update({ folderId: destinationFolderId }) + .returning("*"); + return movedSecrets; + } catch (error) { + throw new DatabaseError({ error, name: "move personal overrides" }); + } + }; + + const deletePersonalOverridesByKeys = async (folderId: string, secretKeys: string[], tx?: Knex) => { + if (secretKeys.length === 0) return []; + try { + const deletedSecrets = await (tx || db)(TableName.SecretV2) + .where({ + folderId, + type: SecretType.Personal + }) + .whereIn("key", secretKeys) + .delete() + .returning("*"); + return deletedSecrets; + } catch (error) { + throw new DatabaseError({ error, name: "delete personal overrides by keys" }); + } + }; + return { ...secretOrm, update, @@ -1199,6 +1238,8 @@ export const secretV2BridgeDALFactory = ({ db, keyStore }: TSecretV2DalArg) => { findSecretsWithReminderRecipientsOld, findReferencedSecretReferencesBySecretKey, updateSecretReferenceSecretKey, - updateSecretReferenceEnvAndPath + updateSecretReferenceEnvAndPath, + movePersonalOverrides, + deletePersonalOverridesByKeys }; }; diff --git a/backend/src/services/secret-v2-bridge/secret-v2-bridge-move-overrides.test.ts b/backend/src/services/secret-v2-bridge/secret-v2-bridge-move-overrides.test.ts new file mode 100644 index 00000000000..5d910012109 --- /dev/null +++ b/backend/src/services/secret-v2-bridge/secret-v2-bridge-move-overrides.test.ts @@ -0,0 +1,274 @@ +/* eslint-disable @typescript-eslint/no-explicit-any */ +/* eslint-disable @typescript-eslint/no-unsafe-assignment */ +/* eslint-disable @typescript-eslint/no-unsafe-member-access */ +/* eslint-disable @typescript-eslint/no-unsafe-argument */ + +import { SecretType, TableName } from "@app/db/schemas"; + +import { secretV2BridgeDALFactory } from "./secret-v2-bridge-dal"; + +/** + * Tests for personal override DAL helpers used by moveSecrets + * to keep personal overrides aligned with shared secrets. + * + * These tests use a mock DB layer to verify the query logic. + */ + +// Helper to create a mock knex/db chain +const createMockDb = () => { + const chain: Record> = {}; + + chain.where = vi.fn().mockReturnValue(chain); + chain.whereIn = vi.fn().mockReturnValue(chain); + chain.update = vi.fn().mockReturnValue(chain); + chain.delete = vi.fn().mockReturnValue(chain); + chain.returning = vi.fn().mockResolvedValue([]); + + const db = vi.fn().mockReturnValue(chain); + // replicaNode returns the same callable + (db as any).replicaNode = vi.fn().mockReturnValue(db); + // ref / raw helpers used by other DAL methods + (db as any).ref = vi.fn().mockReturnValue({ withSchema: vi.fn().mockReturnValue({ as: vi.fn() }) }); + (db as any).raw = vi.fn(); + + return { db, chain }; +}; + +describe("movePersonalOverrides", () => { + test("should call db with correct filter for personal secrets by key and folderId", async () => { + const { db, chain } = createMockDb(); + + const movedOverrides = [ + { id: "override-1", key: "API_URL", type: SecretType.Personal, folderId: "dest-folder", userId: "user-1" }, + { id: "override-2", key: "API_URL", type: SecretType.Personal, folderId: "dest-folder", userId: "user-2" } + ]; + chain.returning.mockResolvedValue(movedOverrides); + + // Create a minimal DAL with just the method we need + const mockKeyStore = { + pgIncrementBy: vi.fn(), + deleteItem: vi.fn() + } as any; + + const dal = secretV2BridgeDALFactory({ db: db as any, keyStore: mockKeyStore }); + + const result = await dal.movePersonalOverrides("source-folder", "dest-folder", ["API_URL"]); + + // Verify correct table was targeted + expect(db).toHaveBeenCalledWith(TableName.SecretV2); + + // Verify where clause filters by source folderId and Personal type + expect(chain.where).toHaveBeenCalledWith({ + folderId: "source-folder", + type: SecretType.Personal + }); + + // Verify whereIn filters by the secret keys + expect(chain.whereIn).toHaveBeenCalledWith("key", ["API_URL"]); + + // Verify update sets correct destination folderId + expect(chain.update).toHaveBeenCalledWith({ folderId: "dest-folder" }); + + // Verify returning all columns + expect(chain.returning).toHaveBeenCalledWith("*"); + + // Verify result + expect(result).toEqual(movedOverrides); + expect(result).toHaveLength(2); + }); + + test("should return empty array and skip DB call when secretKeys is empty", async () => { + const { db } = createMockDb(); + const mockKeyStore = { + pgIncrementBy: vi.fn(), + deleteItem: vi.fn() + } as any; + + const dal = secretV2BridgeDALFactory({ db: db as any, keyStore: mockKeyStore }); + + const result = await dal.movePersonalOverrides("source-folder", "dest-folder", []); + + // Should not call db at all + expect(db).not.toHaveBeenCalledWith(TableName.SecretV2); + expect(result).toEqual([]); + }); + + test("should handle multiple secret keys in bulk move", async () => { + const { db, chain } = createMockDb(); + + const movedOverrides = [ + { id: "override-1", key: "API_URL", type: SecretType.Personal, folderId: "dest-folder", userId: "user-1" }, + { id: "override-2", key: "DB_HOST", type: SecretType.Personal, folderId: "dest-folder", userId: "user-1" }, + { id: "override-3", key: "API_URL", type: SecretType.Personal, folderId: "dest-folder", userId: "user-2" } + ]; + chain.returning.mockResolvedValue(movedOverrides); + + const mockKeyStore = { + pgIncrementBy: vi.fn(), + deleteItem: vi.fn() + } as any; + + const dal = secretV2BridgeDALFactory({ db: db as any, keyStore: mockKeyStore }); + + const result = await dal.movePersonalOverrides("source-folder", "dest-folder", ["API_URL", "DB_HOST"]); + + // Verify whereIn with multiple keys + expect(chain.whereIn).toHaveBeenCalledWith("key", ["API_URL", "DB_HOST"]); + expect(result).toHaveLength(3); + }); + + test("should use provided transaction when given", async () => { + const { chain } = createMockDb(); + + // Create a separate tx mock + const txChain: Record> = {}; + txChain.where = vi.fn().mockReturnValue(txChain); + txChain.whereIn = vi.fn().mockReturnValue(txChain); + txChain.update = vi.fn().mockReturnValue(txChain); + txChain.returning = vi.fn().mockResolvedValue([]); + + const tx = vi.fn().mockReturnValue(txChain); + + // Create db that should NOT be called + const db = vi.fn().mockReturnValue(chain); + (db as any).replicaNode = vi.fn().mockReturnValue(db); + (db as any).ref = vi.fn().mockReturnValue({ withSchema: vi.fn().mockReturnValue({ as: vi.fn() }) }); + (db as any).raw = vi.fn(); + + const mockKeyStore = { + pgIncrementBy: vi.fn(), + deleteItem: vi.fn() + } as any; + + const dal = secretV2BridgeDALFactory({ db: db as any, keyStore: mockKeyStore }); + + await dal.movePersonalOverrides("source-folder", "dest-folder", ["API_URL"], tx as any); + + // tx should be used instead of db + expect(tx).toHaveBeenCalledWith(TableName.SecretV2); + // db should NOT have been called for this query + expect(db).not.toHaveBeenCalledWith(TableName.SecretV2); + }); + + test("should return empty array when no personal overrides exist for given keys", async () => { + const { db, chain } = createMockDb(); + chain.returning.mockResolvedValue([]); + + const mockKeyStore = { + pgIncrementBy: vi.fn(), + deleteItem: vi.fn() + } as any; + + const dal = secretV2BridgeDALFactory({ db: db as any, keyStore: mockKeyStore }); + + const result = await dal.movePersonalOverrides("source-folder", "dest-folder", ["NON_EXISTENT_KEY"]); + + expect(result).toEqual([]); + }); +}); + +describe("deletePersonalOverridesByKeys", () => { + test("should delete personal overrides in source folder by keys", async () => { + const { db, chain } = createMockDb(); + + const deletedOverrides = [ + { id: "override-1", key: "API_URL", type: SecretType.Personal, folderId: "source-folder" } + ]; + chain.returning.mockResolvedValue(deletedOverrides); + + const mockKeyStore = { + pgIncrementBy: vi.fn(), + deleteItem: vi.fn() + } as any; + + const dal = secretV2BridgeDALFactory({ db: db as any, keyStore: mockKeyStore }); + + const result = await dal.deletePersonalOverridesByKeys("source-folder", ["API_URL"]); + + expect(db).toHaveBeenCalledWith(TableName.SecretV2); + expect(chain.where).toHaveBeenCalledWith({ + folderId: "source-folder", + type: SecretType.Personal + }); + expect(chain.whereIn).toHaveBeenCalledWith("key", ["API_URL"]); + expect(chain.delete).toHaveBeenCalled(); + expect(chain.returning).toHaveBeenCalledWith("*"); + expect(result).toEqual(deletedOverrides); + }); + + test("should return empty array and skip DB call when secretKeys is empty", async () => { + const { db } = createMockDb(); + + const mockKeyStore = { + pgIncrementBy: vi.fn(), + deleteItem: vi.fn() + } as any; + + const dal = secretV2BridgeDALFactory({ db: db as any, keyStore: mockKeyStore }); + + const result = await dal.deletePersonalOverridesByKeys("source-folder", []); + + expect(db).not.toHaveBeenCalledWith(TableName.SecretV2); + expect(result).toEqual([]); + }); +}); + +describe("moveSecrets personal override handling - integration logic", () => { + /** + * These tests verify the logical flow of what moveSecrets should do + * with personal overrides in different scenarios. + */ + + test("scenario: direct move (no approval policies) should move overrides", () => { + // This test documents the expected behavior: + // When isDestinationUpdated = true AND isSourceUpdated = true + // (no approval policies on either side), personal overrides should be moved. + + const isDestinationUpdated = true; + const isSourceUpdated = true; + + // In the fixed code, movePersonalOverrides is called when: + // - Source has no approval policy (direct delete path) + // - AND destination was directly updated + const shouldMoveOverrides = isDestinationUpdated && isSourceUpdated; + expect(shouldMoveOverrides).toBe(true); + }); + + test("scenario: destination has approval policy should delete source overrides", () => { + // When destination has approval policy, isDestinationUpdated = false. + // Source shared secrets can still be deleted directly, so source overrides + // must be deleted to prevent orphaned personal records. + + const isDestinationUpdated = false; // approval policy at destination + const isSourceUpdated = true; + + const shouldMoveOverrides = isDestinationUpdated; + const shouldDeleteOverrides = !isDestinationUpdated && isSourceUpdated; + + expect(shouldMoveOverrides).toBe(false); + expect(shouldDeleteOverrides).toBe(true); + }); + + test("scenario: source has approval policy should NOT move overrides", () => { + // When source has approval policy, the code enters the approval branch + // and never reaches the direct-delete + move-overrides code. + + const isSourceUpdated = false; // approval policy at source + + // The movePersonalOverrides call is inside the `else` branch + // of the source approval policy check, so it's never reached. + const sourceHasApprovalPolicy = !isSourceUpdated; + expect(sourceHasApprovalPolicy).toBe(true); + // In this case, no overrides are moved (correct behavior: + // shared secret still exists at source pending approval) + }); + + test("scenario: both have approval policies should NOT move overrides", () => { + const isDestinationUpdated = false; // approval policy at destination + const isSourceUpdated = false; // approval policy at source + + // Neither direct path is taken, no overrides moved + const shouldMoveOverrides = isDestinationUpdated && isSourceUpdated; + expect(shouldMoveOverrides).toBe(false); + }); +}); diff --git a/backend/src/services/secret-v2-bridge/secret-v2-bridge-service.ts b/backend/src/services/secret-v2-bridge/secret-v2-bridge-service.ts index 4df76875940..53c4ca0a265 100644 --- a/backend/src/services/secret-v2-bridge/secret-v2-bridge-service.ts +++ b/backend/src/services/secret-v2-bridge/secret-v2-bridge-service.ts @@ -3255,6 +3255,17 @@ export const secretV2BridgeServiceFactory = ({ tx ); + const movedSecretKeys = decryptedSourceSecrets.map((s) => s.key); + + // Keep personal overrides aligned with shared-secret move semantics. + // If destination updated directly, move overrides with the shared secrets. + // If destination is approval-gated, remove source overrides to avoid orphaned records. + if (isDestinationUpdated) { + await secretDAL.movePersonalOverrides(sourceFolder.id, destinationFolder.id, movedSecretKeys, tx); + } else { + await secretDAL.deletePersonalOverridesByKeys(sourceFolder.id, movedSecretKeys, tx); + } + isSourceUpdated = true; } From 48fb9083b679ee075d12b03f2916f1627bc6cae0 Mon Sep 17 00:00:00 2001 From: Puneet Date: Fri, 17 Apr 2026 23:17:48 +0530 Subject: [PATCH 2/2] fix(secret-v2-bridge): resolve destination conflicts when moving personal overrides When moving source personal overrides to destination, delete any existing destination overrides for the same keys first. This prevents creating duplicate personal override rows and maintains the one-override-per-user/key invariant used by secret resolution logic. --- .../secret-v2-bridge/secret-v2-bridge-dal.ts | 12 ++++++ .../secret-v2-bridge-move-overrides.test.ts | 40 +++++++++++++++++++ 2 files changed, 52 insertions(+) diff --git a/backend/src/services/secret-v2-bridge/secret-v2-bridge-dal.ts b/backend/src/services/secret-v2-bridge/secret-v2-bridge-dal.ts index ebf4a55b99d..07f178f5698 100644 --- a/backend/src/services/secret-v2-bridge/secret-v2-bridge-dal.ts +++ b/backend/src/services/secret-v2-bridge/secret-v2-bridge-dal.ts @@ -1182,6 +1182,18 @@ export const secretV2BridgeDALFactory = ({ db, keyStore }: TSecretV2DalArg) => { ) => { if (secretKeys.length === 0) return []; try { + // Delete destination overrides for the same keys to prevent duplicates. + // This ensures source overrides replace any pre-existing destination overrides + // and maintains the one-override-per-user/key assumption. + await (tx || db)(TableName.SecretV2) + .where({ + folderId: destinationFolderId, + type: SecretType.Personal + }) + .whereIn("key", secretKeys) + .delete(); + + // Move source overrides to destination folder. const movedSecrets = await (tx || db)(TableName.SecretV2) .where({ folderId: sourceFolderId, diff --git a/backend/src/services/secret-v2-bridge/secret-v2-bridge-move-overrides.test.ts b/backend/src/services/secret-v2-bridge/secret-v2-bridge-move-overrides.test.ts index 5d910012109..8ea7f046862 100644 --- a/backend/src/services/secret-v2-bridge/secret-v2-bridge-move-overrides.test.ts +++ b/backend/src/services/secret-v2-bridge/secret-v2-bridge-move-overrides.test.ts @@ -125,6 +125,7 @@ describe("movePersonalOverrides", () => { txChain.where = vi.fn().mockReturnValue(txChain); txChain.whereIn = vi.fn().mockReturnValue(txChain); txChain.update = vi.fn().mockReturnValue(txChain); + txChain.delete = vi.fn().mockReturnValue(txChain); txChain.returning = vi.fn().mockResolvedValue([]); const tx = vi.fn().mockReturnValue(txChain); @@ -150,6 +151,45 @@ describe("movePersonalOverrides", () => { expect(db).not.toHaveBeenCalledWith(TableName.SecretV2); }); + test("should delete conflicting destination overrides before moving source overrides", async () => { + const { db, chain } = createMockDb(); + + const movedOverrides = [ + { id: "source-override-1", key: "API_URL", type: SecretType.Personal, folderId: "dest-folder", userId: "user-1" } + ]; + chain.returning.mockResolvedValue(movedOverrides); + chain.delete.mockReturnValue(chain); + + const mockKeyStore = { + pgIncrementBy: vi.fn(), + deleteItem: vi.fn() + } as any; + + const dal = secretV2BridgeDALFactory({ db: db as any, keyStore: mockKeyStore }); + + const result = await dal.movePersonalOverrides("source-folder", "dest-folder", ["API_URL"]); + + // Verify delete was called on destination first + const { calls } = (chain.where as any).mock; + // First call: delete destination overrides + expect(calls[0][0]).toEqual({ + folderId: "dest-folder", + type: SecretType.Personal + }); + // Second call: move source overrides + expect(calls[1][0]).toEqual({ + folderId: "source-folder", + type: SecretType.Personal + }); + + // Verify delete was called + expect(chain.delete).toHaveBeenCalled(); + // Verify update was called + expect(chain.update).toHaveBeenCalledWith({ folderId: "dest-folder" }); + // Verify result is the moved overrides + expect(result).toEqual(movedOverrides); + }); + test("should return empty array when no personal overrides exist for given keys", async () => { const { db, chain } = createMockDb(); chain.returning.mockResolvedValue([]);