diff --git a/server/src/__tests__/secrets-service.test.ts b/server/src/__tests__/secrets-service.test.ts index fcdb0311e9c..01f13041650 100644 --- a/server/src/__tests__/secrets-service.test.ts +++ b/server/src/__tests__/secrets-service.test.ts @@ -724,6 +724,64 @@ describeEmbeddedPostgres("secretService", () => { })); }); + it("keeps a local cleanup handle when create rollback cleanup fails", async () => { + const companyId = await seedCompany(); + const svc = secretService(db); + const awsVault = await svc.createProviderConfig(companyId, { + provider: "aws_secrets_manager", + displayName: "AWS production", + config: { region: "us-east-1", namespace: "prod-use1" }, + }); + const prepared = { + material: { + scheme: "aws_secrets_manager_v1", + secretId: + "arn:aws:secretsmanager:us-east-1:123456789012:secret:paperclip/prod-use1/company/create-cleanup-handle", + versionId: "aws-version-1", + source: "managed", + }, + valueSha256: "value-sha-1", + fingerprintSha256: "fingerprint-sha-1", + externalRef: + "arn:aws:secretsmanager:us-east-1:123456789012:secret:paperclip/prod-use1/company/create-cleanup-handle", + providerVersionRef: "aws-version-1", + }; + vi.spyOn(awsSecretsManagerProvider, "createSecret").mockResolvedValue(prepared); + vi.spyOn(awsSecretsManagerProvider, "deleteOrArchive").mockRejectedValue( + new Error("cleanup failed"), + ); + vi.spyOn(db, "transaction").mockRejectedValueOnce(new Error("db activate failed")); + + await expect( + svc.create(companyId, { + name: "Create Cleanup Handle", + key: "create-cleanup-handle", + provider: "aws_secrets_manager", + providerConfigId: awsVault.id, + value: "runtime-secret", + }), + ).rejects.toThrow("db activate failed"); + + const persisted = await svc.getByName(companyId, "Create Cleanup Handle"); + expect(persisted).toMatchObject({ + key: "create-cleanup-handle", + status: "archived", + externalRef: prepared.externalRef, + latestVersion: 1, + }); + + const version = await db + .select() + .from(companySecretVersions) + .where(eq(companySecretVersions.secretId, persisted!.id)) + .then((rows) => rows[0] ?? null); + expect(version).toMatchObject({ + version: 1, + status: "disabled", + material: prepared.material, + }); + }); + it("archives managed provider versions when rotate persistence fails", async () => { const companyId = await seedCompany(); const svc = secretService(db); @@ -789,6 +847,76 @@ describeEmbeddedPostgres("secretService", () => { })); }); + it("keeps a disabled version cleanup handle when rotate rollback cleanup fails", async () => { + const companyId = await seedCompany(); + const svc = secretService(db); + const awsVault = await svc.createProviderConfig(companyId, { + provider: "aws_secrets_manager", + displayName: "AWS production", + config: { region: "us-east-1", namespace: "prod-use1" }, + }); + vi.spyOn(awsSecretsManagerProvider, "createSecret").mockResolvedValue({ + material: { + scheme: "aws_secrets_manager_v1", + secretId: + "arn:aws:secretsmanager:us-east-1:123456789012:secret:paperclip/prod-use1/company/rotate-cleanup-handle", + versionId: "aws-version-1", + source: "managed", + }, + valueSha256: "value-sha-1", + fingerprintSha256: "fingerprint-sha-1", + externalRef: + "arn:aws:secretsmanager:us-east-1:123456789012:secret:paperclip/prod-use1/company/rotate-cleanup-handle", + providerVersionRef: "aws-version-1", + }); + const secret = await svc.create(companyId, { + name: "Rotate Cleanup Handle", + key: "rotate-cleanup-handle", + provider: "aws_secrets_manager", + providerConfigId: awsVault.id, + value: "runtime-secret", + }); + const prepared = { + material: { + scheme: "aws_secrets_manager_v1", + secretId: + "arn:aws:secretsmanager:us-east-1:123456789012:secret:paperclip/prod-use1/company/rotate-cleanup-handle", + versionId: "aws-version-2", + source: "managed", + }, + valueSha256: "value-sha-2", + fingerprintSha256: "fingerprint-sha-2", + externalRef: + "arn:aws:secretsmanager:us-east-1:123456789012:secret:paperclip/prod-use1/company/rotate-cleanup-handle", + providerVersionRef: "aws-version-2", + }; + vi.spyOn(awsSecretsManagerProvider, "createVersion").mockResolvedValue(prepared); + vi.spyOn(awsSecretsManagerProvider, "deleteOrArchive").mockRejectedValue( + new Error("cleanup failed"), + ); + vi.spyOn(db, "transaction").mockRejectedValueOnce(new Error("db rotate failed")); + + await expect(svc.rotate(secret.id, { value: "rotated-runtime-secret" })).rejects.toThrow( + "db rotate failed", + ); + + const persisted = await svc.getById(secret.id); + expect(persisted?.latestVersion).toBe(1); + + const versions = await db + .select() + .from(companySecretVersions) + .where(eq(companySecretVersions.secretId, secret.id)); + expect(versions).toEqual(expect.arrayContaining([ + expect.objectContaining({ version: 1, status: "current" }), + expect.objectContaining({ + version: 2, + status: "disabled", + material: prepared.material, + }), + ])); + }); + it("rejects generic provider vault reassignment for managed secrets", async () => { const companyId = await seedCompany(); const svc = secretService(db); diff --git a/server/src/services/secrets.ts b/server/src/services/secrets.ts index 8bbfe892af6..335f998005a 100644 --- a/server/src/services/secrets.ts +++ b/server/src/services/secrets.ts @@ -139,7 +139,7 @@ async function cleanupPreparedProviderWrite(input: { context: SecretProviderWriteContext; mode: "archive" | "delete"; operation: string; -}) { +}): Promise { try { await input.provider.deleteOrArchive({ material: input.prepared.material, @@ -148,6 +148,7 @@ async function cleanupPreparedProviderWrite(input: { context: input.context, mode: input.mode, }); + return true; } catch (cleanupError) { logger.warn( { @@ -159,6 +160,7 @@ async function cleanupPreparedProviderWrite(input: { }, "remote secret provider cleanup failed after db write failure", ); + return false; } } @@ -1502,61 +1504,115 @@ export function secretService(db: Db) { secretName: input.name, version: 1, }; - const prepared = - managedMode === "external_reference" - ? await provider.linkExternalSecret({ - externalRef: input.externalRef ?? "", - providerVersionRef: input.providerVersionRef ?? null, - providerConfig, - context: providerWriteContext, - }) - : await provider.createSecret({ - value: input.value ?? "", - externalRef: null, - providerConfig, - context: providerWriteContext, - }); + const reservedSecret = await db + .insert(companySecrets) + .values({ + companyId, + key, + name: input.name, + provider: input.provider, + providerConfigId: input.providerConfigId ?? null, + status: "archived", + managedMode, + externalRef: null, + providerMetadata: input.providerMetadata ?? null, + latestVersion: 0, + description: input.description ?? null, + createdByAgentId: actor?.agentId ?? null, + createdByUserId: actor?.userId ?? null, + }) + .returning() + .then((rows) => rows[0]); + + let prepared: PreparedSecretVersion; + try { + prepared = + managedMode === "external_reference" + ? await provider.linkExternalSecret({ + externalRef: input.externalRef ?? "", + providerVersionRef: input.providerVersionRef ?? null, + providerConfig, + context: providerWriteContext, + }) + : await provider.createSecret({ + value: input.value ?? "", + externalRef: null, + providerConfig, + context: providerWriteContext, + }); + } catch (error) { + await db.delete(companySecrets).where(eq(companySecrets.id, reservedSecret.id)).catch(() => undefined); + throw error; + } + + try { + await db + .update(companySecrets) + .set({ + externalRef: prepared.externalRef, + latestVersion: 1, + updatedAt: new Date(), + }) + .where(eq(companySecrets.id, reservedSecret.id)); + await db.insert(companySecretVersions).values({ + secretId: reservedSecret.id, + version: 1, + material: prepared.material, + valueSha256: prepared.valueSha256, + fingerprintSha256: prepared.fingerprintSha256 ?? prepared.valueSha256, + providerVersionRef: prepared.providerVersionRef ?? null, + status: "disabled", + createdByAgentId: actor?.agentId ?? null, + createdByUserId: actor?.userId ?? null, + }); + } catch (error) { + if (managedMode === "paperclip_managed") { + const cleaned = await cleanupPreparedProviderWrite({ + provider, + prepared, + providerConfig, + context: providerWriteContext, + mode: "delete", + operation: "create.prepare_rollback", + }); + if (cleaned) { + await db.delete(companySecrets).where(eq(companySecrets.id, reservedSecret.id)).catch(() => undefined); + } + } else { + await db.delete(companySecrets).where(eq(companySecrets.id, reservedSecret.id)).catch(() => undefined); + } + throw error; + } try { return await db.transaction(async (tx) => { + await tx + .update(companySecretVersions) + .set({ status: "current" }) + .where(and( + eq(companySecretVersions.secretId, reservedSecret.id), + eq(companySecretVersions.version, 1), + )); + const secret = await tx - .insert(companySecrets) - .values({ - companyId, - key, - name: input.name, - provider: input.provider, - providerConfigId: input.providerConfigId ?? null, + .update(companySecrets) + .set({ status: "active", - managedMode, externalRef: prepared.externalRef, - providerMetadata: input.providerMetadata ?? null, latestVersion: 1, - description: input.description ?? null, lastRotatedAt: new Date(), - createdByAgentId: actor?.agentId ?? null, - createdByUserId: actor?.userId ?? null, + updatedAt: new Date(), }) + .where(eq(companySecrets.id, reservedSecret.id)) .returning() .then((rows) => rows[0]); - await tx.insert(companySecretVersions).values({ - secretId: secret.id, - version: 1, - material: prepared.material, - valueSha256: prepared.valueSha256, - fingerprintSha256: prepared.fingerprintSha256 ?? prepared.valueSha256, - providerVersionRef: prepared.providerVersionRef ?? null, - status: "current", - createdByAgentId: actor?.agentId ?? null, - createdByUserId: actor?.userId ?? null, - }); - + if (!secret) throw notFound("Secret not found"); return secret; }); } catch (error) { if (managedMode === "paperclip_managed") { - await cleanupPreparedProviderWrite({ + const cleaned = await cleanupPreparedProviderWrite({ provider, prepared, providerConfig, @@ -1564,6 +1620,11 @@ export function secretService(db: Db) { mode: "delete", operation: "create.rollback", }); + if (cleaned) { + await db.delete(companySecrets).where(eq(companySecrets.id, reservedSecret.id)).catch(() => undefined); + } + } else { + await db.delete(companySecrets).where(eq(companySecrets.id, reservedSecret.id)).catch(() => undefined); } throw error; } @@ -1622,23 +1683,48 @@ export function secretService(db: Db) { context: providerWriteContext, }); + try { + await db.insert(companySecretVersions).values({ + secretId: secret.id, + version: nextVersion, + material: prepared.material, + valueSha256: prepared.valueSha256, + fingerprintSha256: prepared.fingerprintSha256 ?? prepared.valueSha256, + providerVersionRef: prepared.providerVersionRef ?? null, + status: "disabled", + createdByAgentId: actor?.agentId ?? null, + createdByUserId: actor?.userId ?? null, + }); + } catch (error) { + if (secret.managedMode !== "external_reference") { + await cleanupPreparedProviderWrite({ + provider, + prepared, + providerConfig, + context: providerWriteContext, + mode: "archive", + operation: "rotate.prepare_rollback", + }); + } + throw error; + } + try { return await db.transaction(async (tx) => { await tx .update(companySecretVersions) .set({ status: "previous" }) - .where(eq(companySecretVersions.secretId, secret.id)); - await tx.insert(companySecretVersions).values({ - secretId: secret.id, - version: nextVersion, - material: prepared.material, - valueSha256: prepared.valueSha256, - fingerprintSha256: prepared.fingerprintSha256 ?? prepared.valueSha256, - providerVersionRef: prepared.providerVersionRef ?? null, - status: "current", - createdByAgentId: actor?.agentId ?? null, - createdByUserId: actor?.userId ?? null, - }); + .where(and( + eq(companySecretVersions.secretId, secret.id), + ne(companySecretVersions.version, nextVersion), + )); + await tx + .update(companySecretVersions) + .set({ status: "current" }) + .where(and( + eq(companySecretVersions.secretId, secret.id), + eq(companySecretVersions.version, nextVersion), + )); const updated = await tx .update(companySecrets) @@ -1658,7 +1744,7 @@ export function secretService(db: Db) { }); } catch (error) { if (secret.managedMode !== "external_reference") { - await cleanupPreparedProviderWrite({ + const cleaned = await cleanupPreparedProviderWrite({ provider, prepared, providerConfig, @@ -1666,6 +1752,15 @@ export function secretService(db: Db) { mode: "archive", operation: "rotate.rollback", }); + if (cleaned) { + await db + .delete(companySecretVersions) + .where(and( + eq(companySecretVersions.secretId, secret.id), + eq(companySecretVersions.version, nextVersion), + )) + .catch(() => undefined); + } } throw error; }