From 53ee0837ec134cbbb447e1dbd3c4aceed9652538 Mon Sep 17 00:00:00 2001 From: Nicky Leach Date: Fri, 26 Jun 2026 00:06:40 +0000 Subject: [PATCH] Harden dynamic secret command tests Restrict dynamic-command dry-runs to persisted secret bindings and run generators with a sanitized process environment. Co-Authored-By: Paperclip --- packages/shared/src/validators/secret.test.ts | 19 ++- packages/shared/src/validators/secret.ts | 11 +- server/src/__tests__/secrets-routes.test.ts | 54 +++++++ server/src/__tests__/secrets-service.test.ts | 145 +++++++++++++++++- server/src/routes/secrets.ts | 14 +- server/src/services/secrets.ts | 66 ++++++-- ui/src/api/secrets.ts | 5 +- ui/src/pages/Secrets.tsx | 79 +--------- 8 files changed, 282 insertions(+), 111 deletions(-) diff --git a/packages/shared/src/validators/secret.test.ts b/packages/shared/src/validators/secret.test.ts index b38c4a33ddc..af09088e323 100644 --- a/packages/shared/src/validators/secret.test.ts +++ b/packages/shared/src/validators/secret.test.ts @@ -30,15 +30,24 @@ describe("secret validators", () => { expect(parsed.staticArgv).toBeUndefined(); }); - it("validates a dynamic-command test payload and defaults argv to []", () => { + it("validates a dynamic-command test payload for a saved secret binding", () => { const parsed = testDynamicSecretCommandSchema.parse({ - command: "/usr/local/bin/mint-github-token", + secretId: "11111111-1111-1111-1111-111111111111", + bindingId: "22222222-2222-2222-2222-222222222222", + }); + expect(parsed).toEqual({ + secretId: "11111111-1111-1111-1111-111111111111", + bindingId: "22222222-2222-2222-2222-222222222222", }); - expect(parsed.staticArgv).toEqual([]); }); - it("rejects an empty command on the dynamic-command test payload", () => { - expect(() => testDynamicSecretCommandSchema.parse({ command: " " })).toThrow(); + it("rejects arbitrary command text on the dynamic-command test payload", () => { + expect(() => + testDynamicSecretCommandSchema.parse({ + command: "/usr/local/bin/mint-github-token", + staticArgv: ["--installation", "123"], + }), + ).toThrow(); }); it("allows dynamic command secrets with host-command generator config", () => { diff --git a/packages/shared/src/validators/secret.ts b/packages/shared/src/validators/secret.ts index b02e6849839..23a7dec80f2 100644 --- a/packages/shared/src/validators/secret.ts +++ b/packages/shared/src/validators/secret.ts @@ -39,13 +39,12 @@ export const dynamicSecretCommandSchema = z.object({ export const staticArgvSchema = z.array(z.string().max(4096)).max(64).default([]); -// Operator dry-run of a dynamic (host-command) generator before saving a -// secret or binding. The resolved value is never returned to the caller — -// only a pass/fail signal plus non-sensitive metadata. +// Operator dry-run of a saved dynamic (host-command) generator. The route +// never accepts arbitrary command text; it reuses persisted secret + binding +// configuration and returns only pass/fail metadata. export const testDynamicSecretCommandSchema = z.object({ - command: z.string().trim().min(1).max(2048), - ttlSeconds: z.number().int().min(1).max(86_400).optional(), - staticArgv: staticArgvSchema, + secretId: z.string().uuid(), + bindingId: z.string().uuid(), }).strict(); export type TestDynamicSecretCommand = z.infer; diff --git a/server/src/__tests__/secrets-routes.test.ts b/server/src/__tests__/secrets-routes.test.ts index b97b4b17b58..6524428f6ab 100644 --- a/server/src/__tests__/secrets-routes.test.ts +++ b/server/src/__tests__/secrets-routes.test.ts @@ -23,11 +23,16 @@ const mockSecretService = vi.hoisted(() => ({ remove: vi.fn(), previewRemoteImport: vi.fn(), importRemoteSecrets: vi.fn(), + testDynamicCommand: vi.fn(), })); const mockLogActivity = vi.hoisted(() => vi.fn()); +const mockInstanceSettingsService = vi.hoisted(() => ({ + getExperimental: vi.fn(), +})); vi.mock("../services/index.js", () => ({ secretService: () => mockSecretService, + instanceSettingsService: () => mockInstanceSettingsService, logActivity: mockLogActivity, })); @@ -54,6 +59,8 @@ describe("secret routes", () => { for (const mock of Object.values(mockSecretService)) { mock.mockReset(); } + mockInstanceSettingsService.getExperimental.mockReset(); + mockInstanceSettingsService.getExperimental.mockResolvedValue({ enableDynamicSecrets: true }); mockLogActivity.mockReset(); }); @@ -95,6 +102,53 @@ describe("secret routes", () => { expect(mockSecretService.create).not.toHaveBeenCalled(); }); + it("rejects arbitrary dynamic command tests before calling the service", async () => { + const res = await request(createApp()) + .post("/api/companies/company-1/secrets/dynamic-command/test") + .send({ + command: process.execPath, + staticArgv: ["./mint-token.js"], + }); + + expect(res.status).toBe(400); + expect(mockInstanceSettingsService.getExperimental).not.toHaveBeenCalled(); + expect(mockSecretService.testDynamicCommand).not.toHaveBeenCalled(); + }); + + it("dry-runs only a saved dynamic command secret binding", async () => { + mockSecretService.testDynamicCommand.mockResolvedValue({ + ok: true, + posture: "soft", + bytes: 24, + }); + + const res = await request(createApp()) + .post("/api/companies/company-1/secrets/dynamic-command/test") + .send({ + secretId: "11111111-1111-1111-1111-111111111111", + bindingId: "22222222-2222-2222-2222-222222222222", + }); + + expect(res.status).toBe(200); + expect(res.body).toEqual({ ok: true, posture: "soft", bytes: 24 }); + expect(mockSecretService.testDynamicCommand).toHaveBeenCalledWith({ + companyId: "company-1", + secretId: "11111111-1111-1111-1111-111111111111", + bindingId: "22222222-2222-2222-2222-222222222222", + }); + expect(mockLogActivity).toHaveBeenCalledWith(expect.anything(), expect.objectContaining({ + action: "secret.dynamic_command.tested", + entityType: "secret", + entityId: "11111111-1111-1111-1111-111111111111", + details: { + ok: true, + bindingId: "22222222-2222-2222-2222-222222222222", + errorCode: null, + }, + })); + expect(JSON.stringify(res.body)).not.toContain("minted"); + }); + it("rejects provider vault routes for non-board actors", async () => { const res = await request(createApp({ type: "agent", diff --git a/server/src/__tests__/secrets-service.test.ts b/server/src/__tests__/secrets-service.test.ts index 3f8268a1715..dee91142f41 100644 --- a/server/src/__tests__/secrets-service.test.ts +++ b/server/src/__tests__/secrets-service.test.ts @@ -308,6 +308,63 @@ console.log("minted-" + count + ":" + process.argv.slice(2).join("|")); expect(activityJson).not.toContain(scriptPath); }); + it("runs dynamic command generators with a sanitized environment", async () => { + const previousEnv = { + DATABASE_URL: process.env.DATABASE_URL, + OPENAI_API_KEY: process.env.OPENAI_API_KEY, + PAPERCLIP_API_KEY: process.env.PAPERCLIP_API_KEY, + PAPERCLIP_TEST_TOKEN: process.env.PAPERCLIP_TEST_TOKEN, + }; + process.env.DATABASE_URL = "postgres://paperclip:secret@example.test/paperclip"; + process.env.OPENAI_API_KEY = "sk-should-not-leak"; + process.env.PAPERCLIP_API_KEY = "paperclip-api-key"; + process.env.PAPERCLIP_TEST_TOKEN = "token-should-not-leak"; + try { + const companyId = await seedCompany(); + await enableDynamicSecrets(); + const scriptPath = writeGeneratorScript("dynamic-env", ` +const sensitive = ["DATABASE_URL", "OPENAI_API_KEY", "PAPERCLIP_API_KEY", "PAPERCLIP_TEST_TOKEN"]; +const visible = sensitive.filter((key) => process.env[key]).join(","); +console.log(JSON.stringify({ visible, path: typeof process.env.PATH === "string" && process.env.PATH.length > 0 })); +`); + const svc = secretService(db); + const secret = await svc.create(companyId, { + name: `dynamic-env-${randomUUID()}`, + provider: "host_command", + managedMode: "dynamic_command", + dynamicCommand: { + provider: "host-command", + command: process.execPath, + ttlSeconds: 60, + }, + }); + await svc.createBinding({ + companyId, + secretId: secret.id, + targetType: "agent", + targetId: "agent-env", + configPath: "env.GITHUB_TOKEN", + staticArgv: [scriptPath], + }); + + const resolved = await svc.resolveAdapterConfigForRuntime( + companyId, + { env: { GITHUB_TOKEN: { type: "secret_ref", secretId: secret.id } } }, + { consumerType: "agent", consumerId: "agent-env" }, + ); + + expect(JSON.parse(resolved.config.env.GITHUB_TOKEN)).toEqual({ visible: "", path: true }); + } finally { + for (const [key, value] of Object.entries(previousEnv)) { + if (value === undefined) { + delete process.env[key]; + } else { + process.env[key] = value; + } + } + } + }); + it("fails closed at runtime when dynamic secrets are disabled after creation", async () => { const companyId = await seedCompany(); await enableDynamicSecrets(); @@ -415,7 +472,7 @@ console.log("should-not-run"); expect(activities).toHaveLength(0); }); - it("dry-runs a dynamic generator without returning the value", async () => { + it("dry-runs a saved dynamic generator binding without returning the value", async () => { const companyId = await seedCompany(); await enableDynamicSecrets(); const svc = secretService(db); @@ -424,11 +481,26 @@ console.log("should-not-run"); "console.log('minted:' + process.argv.slice(2).join('|'));", ); - const ok = await svc.testDynamicCommand({ + const secret = await svc.create(companyId, { + name: `dynamic-test-${randomUUID()}`, + provider: "host_command", + managedMode: "dynamic_command", + dynamicCommand: { + provider: "host-command", + command: process.execPath, + ttlSeconds: 60, + }, + }); + const binding = await svc.createBinding({ companyId, - command: process.execPath, + secretId: secret.id, + targetType: "agent", + targetId: "agent-test", + configPath: "env.GITHUB_TOKEN", staticArgv: [scriptPath, "--installation", "999"], }); + + const ok = await svc.testDynamicCommand({ companyId, secretId: secret.id, bindingId: binding.id }); expect(ok.ok).toBe(true); expect(ok.posture).toBe("soft"); expect(ok.bytes).toBe("minted:--installation|999".length); @@ -438,10 +510,28 @@ console.log("should-not-run"); "dynamic-test-fail", "process.stderr.write('boom'); process.exit(2);", ); + const failingSecret = await svc.create(companyId, { + name: `dynamic-test-fail-${randomUUID()}`, + provider: "host_command", + managedMode: "dynamic_command", + dynamicCommand: { + provider: "host-command", + command: process.execPath, + ttlSeconds: 60, + }, + }); + const failingBinding = await svc.createBinding({ + companyId, + secretId: failingSecret.id, + targetType: "agent", + targetId: "agent-test-fail", + configPath: "env.GITHUB_TOKEN", + staticArgv: [failing], + }); const failure = await svc.testDynamicCommand({ companyId, - command: process.execPath, - staticArgv: [failing], + secretId: failingSecret.id, + bindingId: failingBinding.id, }); expect(failure.ok).toBe(false); expect(failure.posture).toBe("soft"); @@ -449,6 +539,51 @@ console.log("should-not-run"); expect(failure.bytes).toBeUndefined(); }); + it("rejects dynamic command dry-runs for bindings outside the secret company", async () => { + const companyA = await seedCompany("A"); + const companyB = await seedCompany("B"); + await enableDynamicSecrets(); + const svc = secretService(db); + const scriptPath = writeGeneratorScript("dynamic-test-foreign", "console.log('minted');"); + const secretA = await svc.create(companyA, { + name: `dynamic-company-a-${randomUUID()}`, + provider: "host_command", + managedMode: "dynamic_command", + dynamicCommand: { + provider: "host-command", + command: process.execPath, + ttlSeconds: 60, + }, + }); + const secretB = await svc.create(companyB, { + name: `dynamic-company-b-${randomUUID()}`, + provider: "host_command", + managedMode: "dynamic_command", + dynamicCommand: { + provider: "host-command", + command: process.execPath, + ttlSeconds: 60, + }, + }); + const foreignBinding = await svc.createBinding({ + companyId: companyB, + secretId: secretB.id, + targetType: "agent", + targetId: "agent-foreign", + configPath: "env.GITHUB_TOKEN", + staticArgv: [scriptPath], + }); + + const result = await svc.testDynamicCommand({ + companyId: companyA, + secretId: secretA.id, + bindingId: foreignBinding.id, + }); + + expect(result.ok).toBe(false); + expect(result.errorCode).toBe("binding_missing"); + }); + it("threads operator static argv from env bindings into binding rows", async () => { const companyId = await seedCompany(); await enableDynamicSecrets(); diff --git a/server/src/routes/secrets.ts b/server/src/routes/secrets.ts index d0434af10c7..42f6a8a09f6 100644 --- a/server/src/routes/secrets.ts +++ b/server/src/routes/secrets.ts @@ -307,9 +307,9 @@ export function secretRoutes(db: Db) { res.status(201).json(created); }); - // Operator-only dry-run of a dynamic (host-command) generator before saving. - // Gated behind the experimental flag and the board scope. Returns only a - // pass/fail signal and non-sensitive metadata — never the generated value. + // Operator-only dry-run of a saved dynamic (host-command) generator. This + // endpoint deliberately does not accept arbitrary command text; the service + // resolves persisted secret + binding config after company ownership checks. router.post( "/companies/:companyId/secrets/dynamic-command/test", validate(testDynamicSecretCommandSchema), @@ -323,8 +323,8 @@ export function secretRoutes(db: Db) { } const result = await svc.testDynamicCommand({ companyId, - command: req.body.command, - staticArgv: req.body.staticArgv, + secretId: req.body.secretId, + bindingId: req.body.bindingId, }); await logActivity(db, { companyId, @@ -332,8 +332,8 @@ export function secretRoutes(db: Db) { actorId: req.actor.userId ?? "board", action: "secret.dynamic_command.tested", entityType: "secret", - entityId: "dynamic-command-test", - details: { ok: result.ok, errorCode: result.errorCode ?? null }, + entityId: req.body.secretId, + details: { ok: result.ok, bindingId: req.body.bindingId, errorCode: result.errorCode ?? null }, }); res.json(result); }, diff --git a/server/src/services/secrets.ts b/server/src/services/secrets.ts index 6c095601080..94ff61f5b8e 100644 --- a/server/src/services/secrets.ts +++ b/server/src/services/secrets.ts @@ -76,6 +76,15 @@ const EMPTY_STATIC_ARGV: string[] = []; const DEFAULT_DYNAMIC_SECRET_COMMAND_TIMEOUT_MS = 30_000; const REDACTED_DYNAMIC_SECRET_FIELD = "***REDACTED***"; const DYNAMIC_SECRET_CONFIG_VERSION = 1; +const DYNAMIC_SECRET_COMMAND_ENV_ALLOWLIST = new Set([ + "PATH", + "HOME", + "TMPDIR", + "TEMP", + "TMP", + "SystemRoot", + "ComSpec", +]); const dynamicSecretCache = new Map(); type DbTransaction = Parameters[0]>[0]; type SecretBindingDb = Pick; @@ -394,6 +403,15 @@ function dynamicSecretCommandTimeoutMs() { : DEFAULT_DYNAMIC_SECRET_COMMAND_TIMEOUT_MS; } +function buildDynamicSecretCommandEnv(source: NodeJS.ProcessEnv = process.env): NodeJS.ProcessEnv { + const env: NodeJS.ProcessEnv = {}; + for (const key of DYNAMIC_SECRET_COMMAND_ENV_ALLOWLIST) { + const value = source[key]; + if (typeof value === "string") env[key] = value; + } + return env; +} + async function runDynamicSecretCommand(input: { command: string; staticArgv: string[]; @@ -407,7 +425,7 @@ async function runDynamicSecretCommand(input: { shell: false, windowsHide: true, stdio: ["ignore", "pipe", "pipe"], - env: process.env, + env: buildDynamicSecretCommandEnv(), }); const timeout = setTimeout(() => { if (settled) return; @@ -608,6 +626,14 @@ export function secretService(db: Db) { .then((rows) => rows[0] ?? null); } + async function getBindingById(bindingId: string) { + return db + .select() + .from(companySecretBindings) + .where(eq(companySecretBindings.id, bindingId)) + .then((rows) => rows[0] ?? null); + } + async function assertBindingContext( companyId: string, secretId: string, @@ -2619,16 +2645,14 @@ export function secretService(db: Db) { .then((rows) => rows[0] ?? null); }, - // Operator dry-run of a dynamic (host-command) generator. Runs the command - // under Paperclip's privileged control exactly as injection would, but - // never returns the resolved value — only a pass/fail signal plus - // non-sensitive metadata (byte length, posture). Posture is "soft" because - // an unbound generator is not yet attached to any hard-isolation - // environment; the runtime resolver labels the bound posture per Stage 5. + // Operator dry-run of a saved dynamic (host-command) generator. Runs the + // persisted command and persisted binding argv under Paperclip's privileged + // control exactly as injection would, but never accepts arbitrary command + // text and never returns the resolved value. testDynamicCommand: async (input: { companyId: string; - command: string; - staticArgv?: string[]; + secretId: string; + bindingId: string; }): Promise<{ ok: boolean; posture: "hard" | "soft"; @@ -2638,9 +2662,29 @@ export function secretService(db: Db) { }> => { try { await assertDynamicSecretsEnabled(); + const secret = await assertSecretInCompany(input.companyId, input.secretId); + if (secret.managedMode !== "dynamic_command" || secret.provider !== "host_command") { + throw unprocessable("Secret is not a dynamic command secret", { + code: "dynamic_secret_command_failed", + }); + } + const binding = await getBindingById(input.bindingId); + if (!binding) throw notFound("Secret binding not found"); + if (binding.companyId !== input.companyId || binding.secretId !== secret.id) { + throw unprocessable("Secret binding must belong to same company and secret", { + code: "binding_missing", + }); + } + const dynamicCommand = await resolveDynamicCommandConfig({ + id: secret.id, + companyId: secret.companyId, + key: secret.key, + latestVersion: secret.latestVersion, + }); + const staticArgv = await resolveBindingStaticArgv(binding); const value = await runDynamicSecretCommand({ - command: input.command, - staticArgv: normalizeStaticArgv(input.staticArgv ?? EMPTY_STATIC_ARGV), + command: dynamicCommand.command, + staticArgv, }); return { ok: true, posture: "soft", bytes: Buffer.byteLength(value) }; } catch (err) { diff --git a/ui/src/api/secrets.ts b/ui/src/api/secrets.ts index 9109d6ab546..8ced1499c5a 100644 --- a/ui/src/api/secrets.ts +++ b/ui/src/api/secrets.ts @@ -36,9 +36,8 @@ export interface CreateSecretInput { } export interface TestDynamicCommandInput { - command: string; - ttlSeconds?: number; - staticArgv?: string[]; + secretId: string; + bindingId: string; } export interface TestDynamicCommandResult { diff --git a/ui/src/pages/Secrets.tsx b/ui/src/pages/Secrets.tsx index 4e2f051744e..3185b8380a7 100644 --- a/ui/src/pages/Secrets.tsx +++ b/ui/src/pages/Secrets.tsx @@ -21,7 +21,6 @@ import { ShieldAlert, Clock, TerminalSquare, - Play, Star, Trash2, X, @@ -50,7 +49,6 @@ import { type CreateSecretInput, type CreateSecretProviderConfigInput, type SecretProviderHealthResponse, - type TestDynamicCommandResult, type UpdateSecretProviderConfigInput, } from "../api/secrets"; import { instanceSettingsApi } from "../api/instanceSettings"; @@ -496,7 +494,6 @@ export function Secrets() { ttlSeconds: String(DEFAULT_DYNAMIC_TTL_SECONDS), }); const [createError, setCreateError] = useState(null); - const [testResult, setTestResult] = useState(null); const [rotateOpen, setRotateOpen] = useState(false); const [rotateValue, setRotateValue] = useState(""); const [rotateExternalRef, setRotateExternalRef] = useState(""); @@ -705,7 +702,6 @@ export function Secrets() { ttlSeconds: String(DEFAULT_DYNAMIC_TTL_SECONDS), }); setCreateError(null); - setTestResult(null); setSelectedSecretId(created.id); invalidateAll([created.id]); }, @@ -714,24 +710,6 @@ export function Secrets() { }, }); - const testDynamicMutation = useMutation({ - mutationFn: () => - secretsApi.testDynamicCommand(selectedCompanyId!, { - command: createForm.command.trim(), - ttlSeconds: dynamicTtlValid ? dynamicTtlSeconds : undefined, - }), - onSuccess: (result) => { - setTestResult(result); - }, - onError: (error) => { - setTestResult({ - ok: false, - posture: "soft", - message: error instanceof ApiError ? error.message : (error as Error).message, - }); - }, - }); - const rotateMutation = useMutation({ mutationFn: () => { if (!selectedSecret) throw new Error("Select a secret first"); @@ -1334,10 +1312,6 @@ export function Secrets() { open={createOpen} onOpenChange={(open) => { setCreateOpen(open); - if (!open) { - setTestResult(null); - testDynamicMutation.reset(); - } }} > @@ -1353,7 +1327,6 @@ export function Secrets() { value={createMode} onValueChange={(value) => { setCreateMode(value as CreateMode); - setTestResult(null); }} > { const command = event.target.value; setCreateForm((current) => ({ ...current, command })); - setTestResult(null); }} placeholder="/usr/local/bin/mint-github-token" className="pl-7 font-mono text-xs" @@ -1574,52 +1546,11 @@ export function Secrets() {

) : null} - -
-
- Test generator before saving - -
- {testResult ? ( - testResult.ok ? ( -

- - Generator produced a fresh value - {typeof testResult.bytes === "number" ? ` (${testResult.bytes} bytes)` : ""}. - The value is not shown. -

- ) : ( -

- - - Generator failed{testResult.errorCode ? ` (${testResult.errorCode})` : ""}.{" "} - {testResult.message ?? "Check the command and try again."} - -

- ) - ) : ( -

- Runs the command once under Paperclip's control to confirm it returns a value. - Nothing is saved. -

- )} -
+ +

+ Saved generators are tested from an existing secret binding so Paperclip can reuse + the persisted command and operator-fixed arguments. +