Address Greptile secrets review

Co-Authored-By: Paperclip <noreply@paperclip.ing>

packages/db/src/migrations/meta/_journal.json

@@ -579,7 +579,7 @@
     {
       "idx": 82,
       "version": "7",
-      "when": 1777239150329,
+      "when": 1778067785041,
       "tag": "0082_dry_vision",
       "breakpoints": true
     },

server/src/__tests__/aws-secrets-manager-provider.test.ts

@@ -215,6 +215,7 @@ describe("awsSecretsManagerProvider", () => {
     expect(headers.authorization).toContain("/us-east-1/secretsmanager/aws4_request");
     expect(headers.authorization).toContain("SignedHeaders=");
     expect(headers.authorization).toContain("Signature=");
+    expect(init?.signal).toBeInstanceOf(AbortSignal);
   });
 
   it("creates new AWS secret versions against a namespace-valid existing secret reference", async () => {

server/src/__tests__/secrets-service.test.ts

@@ -265,6 +265,47 @@ describeEmbeddedPostgres("secretService", () => {
     expect(rows.find((row) => row.id === second.id)?.isDefault).toBe(true);
   });
 
+  it("does not set a disabled provider vault as default", async () => {
+    const companyId = await seedCompany();
+    const svc = secretService(db);
+    const vault = await svc.createProviderConfig(companyId, {
+      provider: "local_encrypted",
+      displayName: "Local disabled",
+      config: {},
+    });
+
+    await svc.disableProviderConfig(vault.id);
+    await expect(svc.setDefaultProviderConfig(vault.id)).rejects.toThrow(
+      /ready or warning/i,
+    );
+  });
+
+  it("hides soft-deleted secrets and allows name/key reuse", async () => {
+    const companyId = await seedCompany();
+    const svc = secretService(db);
+    const secretName = `reusable-${randomUUID()}`;
+    const secret = await svc.create(companyId, {
+      name: secretName,
+      key: "reusable-key",
+      provider: "local_encrypted",
+      value: "first-value",
+    });
+
+    await svc.update(secret.id, { status: "deleted" });
+    const listed = await svc.list(companyId);
+    const recreated = await svc.create(companyId, {
+      name: secretName,
+      key: "reusable-key",
+      provider: "local_encrypted",
+      value: "second-value",
+    });
+
+    expect(listed.map((row) => row.id)).not.toContain(secret.id);
+    expect(recreated.id).not.toBe(secret.id);
+    expect(recreated.name).toBe(secretName);
+    expect(recreated.key).toBe("reusable-key");
+  });
+
   it("rejects provider vaults from another company when creating a secret", async () => {
     const companyA = await seedCompany("A");
     const companyB = await seedCompany("B");
@@ -855,6 +896,63 @@ describeEmbeddedPostgres("secretService", () => {
     expect(persisted).toBeNull();
   });
 
+  it("removes DB rows even when the attached provider vault is disabled", async () => {
+    const companyId = await seedCompany();
+    const svc = secretService(db);
+    const vault = await svc.createProviderConfig(companyId, {
+      provider: "aws_secrets_manager",
+      displayName: "AWS disabled later",
+      config: {
+        region: "us-east-1",
+        namespace: "prod",
+      },
+    });
+    const externalRef =
+      "arn:aws:secretsmanager:us-east-1:123456789012:secret:paperclip/prod/company-1/openai-api-key";
+    const secret = await db
+      .insert(companySecrets)
+      .values({
+        companyId,
+        key: "openai-api-key",
+        name: "OpenAI API Key",
+        provider: "aws_secrets_manager",
+        providerConfigId: vault.id,
+        managedMode: "paperclip_managed",
+        externalRef,
+        latestVersion: 1,
+        status: "active",
+      })
+      .returning()
+      .then((rows) => rows[0]);
+
+    await db.insert(companySecretVersions).values({
+      secretId: secret.id,
+      version: 1,
+      material: {
+        scheme: "aws_secrets_manager_v1",
+        secretId: externalRef,
+        versionId: "aws-version-1",
+        source: "managed",
+      },
+      valueSha256: "value-sha-1",
+      fingerprintSha256: "fingerprint-sha-1",
+      providerVersionRef: "aws-version-1",
+      status: "current",
+    });
+    await svc.disableProviderConfig(vault.id);
+    const deleteSpy = vi.spyOn(awsSecretsManagerProvider, "deleteOrArchive").mockResolvedValue();
+
+    await expect(svc.remove(secret.id)).resolves.toMatchObject({ id: secret.id });
+    const persisted = await db
+      .select()
+      .from(companySecrets)
+      .where(eq(companySecrets.id, secret.id))
+      .then((rows) => rows[0] ?? null);
+
+    expect(deleteSpy).not.toHaveBeenCalled();
+    expect(persisted).toBeNull();
+  });
+
   it("refuses to resolve secrets once they are disabled or archived", async () => {
     const companyId = await seedCompany();
     const svc = secretService(db);

server/src/secrets/aws-secrets-manager-provider.ts

@@ -20,6 +20,7 @@ const DEFAULT_PREFIX = "paperclip";
 const DEFAULT_OWNER_TAG = "paperclip";
 const DEFAULT_VERSION_STAGE = "AWSCURRENT";
 const DEFAULT_DELETE_RECOVERY_WINDOW_DAYS = 30;
+const AWS_SECRETS_MANAGER_REQUEST_TIMEOUT_MS = 30_000;
 const AWS_RUNTIME_CREDENTIAL_WARNING =
   "AWS bootstrap credentials must be available to the Paperclip server runtime through the AWS SDK default credential provider chain: IAM role/workload identity, AWS_PROFILE/SSO/shared credentials, web identity, container/instance metadata, or short-lived shell credentials.";
 const AWS_CREDENTIAL_CUSTODY_WARNING =
@@ -685,6 +686,7 @@ class AwsSecretsManagerJsonGateway implements AwsSecretsManagerGateway {
       method: "POST",
       headers,
       body,
+      signal: AbortSignal.timeout(AWS_SECRETS_MANAGER_REQUEST_TIMEOUT_MS),
     });
     const text = await response.text();
     const parsed = text ? (JSON.parse(text) as Record<string, unknown>) : {};

server/src/services/secrets.ts

@@ -1,4 +1,4 @@
-import { and, desc, eq, inArray, sql } from "drizzle-orm";
+import { and, desc, eq, inArray, ne, notInArray, sql } from "drizzle-orm";
 import type { Db } from "@paperclipai/db";
 import {
   agents,
@@ -237,7 +237,11 @@ export function secretService(db: Db) {
     return db
       .select()
       .from(companySecrets)
-      .where(and(eq(companySecrets.companyId, companyId), eq(companySecrets.name, name)))
+      .where(and(
+        eq(companySecrets.companyId, companyId),
+        eq(companySecrets.name, name),
+        ne(companySecrets.status, "deleted"),
+      ))
       .then((rows) => rows[0] ?? null);
   }
 
@@ -990,19 +994,33 @@ export function secretService(db: Db) {
         throw unprocessable("Only ready or warning provider vaults can be default");
       }
       return db.transaction(async (tx) => {
+        const current = await tx
+          .select()
+          .from(companySecretProviderConfigs)
+          .where(eq(companySecretProviderConfigs.id, id))
+          .then((rows) => rows[0] ?? null);
+        if (!current) return null;
+        if (current.status === "coming_soon" || current.status === "disabled") {
+          throw unprocessable("Only ready or warning provider vaults can be default");
+        }
         await tx
           .update(companySecretProviderConfigs)
           .set({ isDefault: false, updatedAt: new Date() })
           .where(and(
-            eq(companySecretProviderConfigs.companyId, existing.companyId),
-            eq(companySecretProviderConfigs.provider, existing.provider),
+            eq(companySecretProviderConfigs.companyId, current.companyId),
+            eq(companySecretProviderConfigs.provider, current.provider),
           ));
-        return tx
+        const updated = await tx
           .update(companySecretProviderConfigs)
           .set({ isDefault: true, updatedAt: new Date() })
-          .where(eq(companySecretProviderConfigs.id, id))
+          .where(and(
+            eq(companySecretProviderConfigs.id, id),
+            notInArray(companySecretProviderConfigs.status, ["coming_soon", "disabled"]),
+          ))
           .returning()
           .then((rows) => rows[0] ?? null);
+        if (!updated) throw unprocessable("Only ready or warning provider vaults can be default");
+        return updated;
       });
     },
 
@@ -1043,7 +1061,7 @@ export function secretService(db: Db) {
         db
           .select()
           .from(companySecrets)
-          .where(eq(companySecrets.companyId, companyId))
+          .where(and(eq(companySecrets.companyId, companyId), ne(companySecrets.status, "deleted")))
           .orderBy(desc(companySecrets.createdAt)),
         db
           .select({
@@ -1418,7 +1436,11 @@ export function secretService(db: Db) {
       const duplicateKey = await db
         .select()
         .from(companySecrets)
-        .where(and(eq(companySecrets.companyId, companyId), eq(companySecrets.key, key)))
+        .where(and(
+          eq(companySecrets.companyId, companyId),
+          eq(companySecrets.key, key),
+          ne(companySecrets.status, "deleted"),
+        ))
         .then((rows) => rows[0] ?? null);
       if (duplicateKey) throw conflict(`Secret key already exists: ${key}`);
 
@@ -1619,7 +1641,11 @@ export function secretService(db: Db) {
         const duplicateKey = await db
           .select()
           .from(companySecrets)
-          .where(and(eq(companySecrets.companyId, secret.companyId), eq(companySecrets.key, nextKey)))
+          .where(and(
+            eq(companySecrets.companyId, secret.companyId),
+            eq(companySecrets.key, nextKey),
+            ne(companySecrets.status, "deleted"),
+          ))
           .then((rows) => rows[0] ?? null);
         if (duplicateKey && duplicateKey.id !== secret.id) {
           throw conflict(`Secret key already exists: ${nextKey}`);
@@ -1658,8 +1684,8 @@ export function secretService(db: Db) {
       return db
         .update(companySecrets)
         .set({
-          key: nextKey,
-          name: patch.name ?? secret.name,
+          key: deleting ? `${secret.key}__deleted__${secret.id}` : nextKey,
+          name: deleting ? `${secret.name}__deleted__${secret.id}` : patch.name ?? secret.name,
           status: patch.status ?? secret.status,
           providerConfigId:
             patch.providerConfigId === undefined ? secret.providerConfigId : patch.providerConfigId,
@@ -1830,23 +1856,35 @@ export function secretService(db: Db) {
       const versionRow = await getSecretVersion(secret.id, secret.latestVersion);
       const providerId = secret.provider as SecretProvider;
       const provider = getSecretProvider(providerId);
-      const providerConfig = await getSelectableRuntimeProviderConfig({
-        companyId: secret.companyId,
-        provider: providerId,
-        providerConfigId: secret.providerConfigId,
-      });
-      await provider.deleteOrArchive({
-        material: versionRow?.material as Record<string, unknown> | undefined,
-        externalRef: secret.externalRef,
-        providerConfig,
-        context: {
-          companyId: secret.companyId,
-          secretKey: secret.key,
-          secretName: secret.name,
-          version: secret.latestVersion,
-        },
-        mode: "delete",
-      });
+      await db
+        .update(companySecrets)
+        .set({
+          status: "deleted",
+          deletedAt: secret.deletedAt ?? new Date(),
+          updatedAt: new Date(),
+        })
+        .where(eq(companySecrets.id, secretId));
+      const providerConfig = secret.providerConfigId
+        ? await getProviderConfigById(secret.providerConfigId)
+        : null;
+      const providerRuntimeConfig =
+        providerConfig && providerConfig.status !== "disabled" && providerConfig.status !== "coming_soon"
+          ? toProviderVaultRuntimeConfig(providerConfig)
+          : null;
+      if (!secret.providerConfigId || providerRuntimeConfig) {
+        await provider.deleteOrArchive({
+          material: versionRow?.material as Record<string, unknown> | undefined,
+          externalRef: secret.externalRef,
+          providerConfig: providerRuntimeConfig,
+          context: {
+            companyId: secret.companyId,
+            secretKey: secret.key,
+            secretName: secret.name,
+            version: secret.latestVersion,
+          },
+          mode: "delete",
+        });
+      }
       await db.delete(companySecrets).where(eq(companySecrets.id, secretId));
       return secret;
     },