From 92c3d9f0d9fb11a9d06ca43fc09e8e25b9e64b08 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Fri, 31 Jul 2026 22:48:32 -0700 Subject: [PATCH] fix(server): stop requiring PAPERCLIP_DECISION_SIGNING_SECRET at startup (#10594) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The server signs decision specifications with an HMAC > - PR #10010 made `PAPERCLIP_DECISION_SIGNING_SECRET` a hard startup requirement > - Existing installs do not have this new environment variable > - Those installs now stop during startup > - This pull request uses a secure persisted instance key when the override is absent > - The benefit is that existing installs start without new configuration and decision signing remains fail-closed ## Linked Issues or Issue Description **What happened?** After #10010, `startServer()` throws when `PAPERCLIP_DECISION_SIGNING_SECRET` is unset or shorter than 32 characters. Existing installs without the new environment variable stop at startup. **Expected behavior** The server starts without manual configuration. A new optional feature must not add a required environment variable for existing installs. **Steps to reproduce** 1. Check out `master` at 9c1f8e7887. 2. Unset `PAPERCLIP_DECISION_SIGNING_SECRET`. 3. Start the server. 4. Observe that startup stops with a missing-secret error. **Paperclip version or commit** `master` at 9c1f8e7887. **Deployment mode** All deployment modes are affected when the environment variable is absent. ## What Changed - Treat `PAPERCLIP_DECISION_SIGNING_SECRET` as an optional override. - Generate a random per-instance key at `/secrets/decision-signing.key` when the override is absent. - Publish a complete first-time key with an atomic no-overwrite link so concurrent server starts use one key. - Repair permissive modes on process-owned secrets directories and regular key files, reject planted symlinks or foreign-owned paths, and fail startup if `0700`/`0600` cannot be enforced. - Keep an explicitly configured secret shorter than 32 characters as a startup error. - Add startup, permission, planted-symlink, fail-closed verification, and generated-key round-trip tests. ## Verification - `pnpm --filter @paperclipai/server exec vitest run src/__tests__/decisions-service.test.ts src/__tests__/server-startup-feedback-export.test.ts` — 45 tests passed. - `pnpm --filter @paperclipai/server exec tsc --noEmit` — passed. - Eight simultaneous resolver processes returned the same persisted key. The secrets directory/key modes were `0700`/`0600`. - `git diff --check` — passed. ## Risks - Existing configured secrets remain unchanged. - Removing a configured secret after a proposal makes the prior signature fail verification. Restoring the secret restores verification. - A restored secrets directory or key with unsafe permissions now fails startup when the server cannot repair it to `0700`/`0600`; symlinks and paths owned by another local user are rejected rather than trusted. - The generated key uses an atomic hard link in the instance secrets directory. An unsupported file system fails startup instead of replacing an existing key. - Existing installs that failed at startup did not sign decisions with a missing key. > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See `CONTRIBUTING.md`. ## Model Used - Anthropic Claude Fable 5, model ID `claude-fable-5`, produced the initial implementation with extended reasoning and tool use. - OpenAI Codex, model ID `gpt-5`, addressed review findings and prepared the PR with reasoning, repository editing, code execution, and GitHub tooling. The runtime did not expose the context-window size. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge Co-authored-by: Paperclip --- .../src/__tests__/decisions-service.test.ts | 36 ++++- .../server-startup-feedback-export.test.ts | 90 ++++++++++- server/src/index.ts | 4 +- server/src/services/decision-signing.ts | 147 +++++++++++++++++- 4 files changed, 263 insertions(+), 14 deletions(-) diff --git a/server/src/__tests__/decisions-service.test.ts b/server/src/__tests__/decisions-service.test.ts index 7c0a546dbd..febd4223ab 100644 --- a/server/src/__tests__/decisions-service.test.ts +++ b/server/src/__tests__/decisions-service.test.ts @@ -1,4 +1,7 @@ import { randomUUID } from "node:crypto"; +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import path from "node:path"; import { eq } from "drizzle-orm"; import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; import { @@ -279,13 +282,38 @@ describePg("decisionService", () => { expect(result.executions[0]?.result).toMatchObject({ originReason: "deny_missing_membership" }); }); - it("refuses execution when the decision signing secret is unavailable", async () => { + it("fails closed when the configured signing secret is removed after proposal", async () => { const created = await createCommentDecision(); + const originalHome = process.env.PAPERCLIP_HOME; + const tempHome = mkdtempSync(path.join(tmpdir(), "paperclip-decision-rotate-")); + process.env.PAPERCLIP_HOME = tempHome; delete process.env.PAPERCLIP_DECISION_SIGNING_SECRET; process.env.PAPERCLIP_AGENT_JWT_SECRET = "agent-jwt-secret-must-not-sign-decisions"; - await expect(service().decide({ id: created.id, optionId: "yes", decidedByUserId, userActor: boardActor() })) - .rejects.toThrow("PAPERCLIP_DECISION_SIGNING_SECRET is required"); - expect(await db.select().from(issueComments).where(eq(issueComments.issueId, targetIssueId))).toHaveLength(0); + try { + await expect(service().decide({ id: created.id, optionId: "yes", decidedByUserId, userActor: boardActor() })) + .rejects.toThrow("Decision signature verification failed"); + expect(await db.select().from(issueComments).where(eq(issueComments.issueId, targetIssueId))).toHaveLength(0); + } finally { + if (originalHome === undefined) delete process.env.PAPERCLIP_HOME; + else process.env.PAPERCLIP_HOME = originalHome; + rmSync(tempHome, { recursive: true, force: true }); + } + }); + + it("signs and verifies with an auto-generated key when no secret is configured", async () => { + const originalHome = process.env.PAPERCLIP_HOME; + const tempHome = mkdtempSync(path.join(tmpdir(), "paperclip-decision-generated-")); + process.env.PAPERCLIP_HOME = tempHome; + delete process.env.PAPERCLIP_DECISION_SIGNING_SECRET; + try { + const created = await createCommentDecision(); + const result = await service().decide({ id: created.id, optionId: "yes", decidedByUserId, userActor: boardActor() }); + expect(result.executionStatus).toBe("succeeded"); + } finally { + if (originalHome === undefined) delete process.env.PAPERCLIP_HOME; + else process.env.PAPERCLIP_HOME = originalHome; + rmSync(tempHome, { recursive: true, force: true }); + } }); it("records a failed effect and continues independent later effects", async () => { diff --git a/server/src/__tests__/server-startup-feedback-export.test.ts b/server/src/__tests__/server-startup-feedback-export.test.ts index 76a1ed981c..1abbefe45d 100644 --- a/server/src/__tests__/server-startup-feedback-export.test.ts +++ b/server/src/__tests__/server-startup-feedback-export.test.ts @@ -1,4 +1,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { chmodSync, mkdirSync, mkdtempSync, readFileSync, rmSync, statSync, symlinkSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import path from "node:path"; const ORIGINAL_PAPERCLIP_API_URL = process.env.PAPERCLIP_API_URL; const ORIGINAL_PAPERCLIP_RUNTIME_API_URL = process.env.PAPERCLIP_RUNTIME_API_URL; @@ -298,9 +301,92 @@ describe("startServer feedback export wiring", () => { process.env.BETTER_AUTH_SECRET = "test-secret"; }); - it("refuses startup when the decision signing secret is unavailable", async () => { + it("starts without PAPERCLIP_DECISION_SIGNING_SECRET by generating a persisted key", async () => { + const originalHome = process.env.PAPERCLIP_HOME; + const originalInstanceId = process.env.PAPERCLIP_INSTANCE_ID; + const tempHome = mkdtempSync(path.join(tmpdir(), "paperclip-decision-key-")); + process.env.PAPERCLIP_HOME = tempHome; + process.env.PAPERCLIP_INSTANCE_ID = "default"; delete process.env.PAPERCLIP_DECISION_SIGNING_SECRET; - await expect(startServer()).rejects.toThrow("PAPERCLIP_DECISION_SIGNING_SECRET is required"); + try { + const started = await startServer(); + expect(started.server).toBe(fakeServer); + const keyPath = path.join(tempHome, "instances", "default", "secrets", "decision-signing.key"); + expect(readFileSync(keyPath, "utf8").trim().length).toBeGreaterThanOrEqual(32); + if (process.platform !== "win32") { + expect(statSync(path.dirname(keyPath)).mode & 0o777).toBe(0o700); + expect(statSync(keyPath).mode & 0o777).toBe(0o600); + } + } finally { + if (originalHome === undefined) delete process.env.PAPERCLIP_HOME; + else process.env.PAPERCLIP_HOME = originalHome; + if (originalInstanceId === undefined) delete process.env.PAPERCLIP_INSTANCE_ID; + else process.env.PAPERCLIP_INSTANCE_ID = originalInstanceId; + rmSync(tempHome, { recursive: true, force: true }); + } + }); + + it("repairs permissive permissions on an existing generated decision signing key", async () => { + const originalHome = process.env.PAPERCLIP_HOME; + const originalInstanceId = process.env.PAPERCLIP_INSTANCE_ID; + const tempHome = mkdtempSync(path.join(tmpdir(), "paperclip-decision-key-mode-")); + const keyPath = path.join(tempHome, "instances", "default", "secrets", "decision-signing.key"); + const existingKey = Buffer.alloc(32, 7).toString("base64"); + mkdirSync(path.dirname(keyPath), { recursive: true, mode: 0o777 }); + chmodSync(path.dirname(keyPath), 0o777); + writeFileSync(keyPath, existingKey, { encoding: "utf8", mode: 0o644 }); + chmodSync(keyPath, 0o644); + process.env.PAPERCLIP_HOME = tempHome; + process.env.PAPERCLIP_INSTANCE_ID = "default"; + delete process.env.PAPERCLIP_DECISION_SIGNING_SECRET; + try { + const started = await startServer(); + expect(started.server).toBe(fakeServer); + expect(readFileSync(keyPath, "utf8")).toBe(existingKey); + if (process.platform !== "win32") { + expect(statSync(path.dirname(keyPath)).mode & 0o777).toBe(0o700); + expect(statSync(keyPath).mode & 0o777).toBe(0o600); + } + } finally { + if (originalHome === undefined) delete process.env.PAPERCLIP_HOME; + else process.env.PAPERCLIP_HOME = originalHome; + if (originalInstanceId === undefined) delete process.env.PAPERCLIP_INSTANCE_ID; + else process.env.PAPERCLIP_INSTANCE_ID = originalInstanceId; + rmSync(tempHome, { recursive: true, force: true }); + } + }); + + it("refuses a symlink planted as the generated decision signing key", async () => { + if (process.platform === "win32") return; + + const originalHome = process.env.PAPERCLIP_HOME; + const originalInstanceId = process.env.PAPERCLIP_INSTANCE_ID; + const tempHome = mkdtempSync(path.join(tmpdir(), "paperclip-decision-key-symlink-")); + const keyPath = path.join(tempHome, "instances", "default", "secrets", "decision-signing.key"); + const plantedTarget = path.join(tempHome, "planted.key"); + const plantedKey = Buffer.alloc(32, 9).toString("base64"); + mkdirSync(path.dirname(keyPath), { recursive: true, mode: 0o777 }); + chmodSync(path.dirname(keyPath), 0o777); + writeFileSync(plantedTarget, plantedKey, { encoding: "utf8", mode: 0o600 }); + symlinkSync(plantedTarget, keyPath); + process.env.PAPERCLIP_HOME = tempHome; + process.env.PAPERCLIP_INSTANCE_ID = "default"; + delete process.env.PAPERCLIP_DECISION_SIGNING_SECRET; + try { + await expect(startServer()).rejects.toThrow("must be a regular file"); + expect(readFileSync(plantedTarget, "utf8")).toBe(plantedKey); + } finally { + if (originalHome === undefined) delete process.env.PAPERCLIP_HOME; + else process.env.PAPERCLIP_HOME = originalHome; + if (originalInstanceId === undefined) delete process.env.PAPERCLIP_INSTANCE_ID; + else process.env.PAPERCLIP_INSTANCE_ID = originalInstanceId; + rmSync(tempHome, { recursive: true, force: true }); + } + }); + + it("refuses startup when an explicit decision signing secret is too short", async () => { + process.env.PAPERCLIP_DECISION_SIGNING_SECRET = "too-short"; + await expect(startServer()).rejects.toThrow("PAPERCLIP_DECISION_SIGNING_SECRET must be at least 32 characters"); expect(loadConfigMock).not.toHaveBeenCalled(); }); diff --git a/server/src/index.ts b/server/src/index.ts index 734525a2f6..9ecc6a850b 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -72,7 +72,7 @@ import { getBoardClaimWarningUrl, initializeBoardClaimChallenge } from "./board- import { maybePersistWorktreeRuntimePorts } from "./worktree-config.js"; import { initTelemetry, getTelemetryClient } from "./telemetry.js"; import { conflict } from "./errors.js"; -import { validateDecisionSigningSecret } from "./services/decision-signing.js"; +import { ensureDecisionSigningSecret } from "./services/decision-signing.js"; import { createDecisionWakeOriginAgent } from "./services/decision-wakeup.js"; import { coordinateHeartbeatSchedulerShutdown } from "./shutdown.js"; import { systemdNotify } from "./services/systemd-notify.js"; @@ -123,7 +123,7 @@ export async function startServer(): Promise { // Tracing must be active (or have failed and logged) before the first DB // connection or the HTTP server exists — see instrumentation.ts. await instrumentationReady; - validateDecisionSigningSecret(); + ensureDecisionSigningSecret(); let config = loadConfig(); initTelemetry({ enabled: config.telemetryEnabled }); if (process.env.PAPERCLIP_SECRETS_PROVIDER === undefined) { diff --git a/server/src/services/decision-signing.ts b/server/src/services/decision-signing.ts index f2c38d4b18..365e20ac7b 100644 --- a/server/src/services/decision-signing.ts +++ b/server/src/services/decision-signing.ts @@ -1,11 +1,146 @@ -import { createHmac, timingSafeEqual } from "node:crypto"; +import { createHmac, randomBytes, timingSafeEqual } from "node:crypto"; +import { chmodSync, linkSync, lstatSync, mkdirSync, readFileSync, type Stats, unlinkSync, writeFileSync } from "node:fs"; +import path from "node:path"; +import { resolveDefaultSecretsKeyFilePath } from "../home-paths.js"; const VERSION = "decision-spec-v1"; +const MIN_SECRET_LENGTH = 32; -export function validateDecisionSigningSecret() { - const value = process.env.PAPERCLIP_DECISION_SIGNING_SECRET?.trim(); - if (!value || value.length < 32) throw new Error("PAPERCLIP_DECISION_SIGNING_SECRET is required and must be at least 32 characters"); - return value; +function resolveGeneratedSecretFilePath() { + return path.join(path.dirname(resolveDefaultSecretsKeyFilePath()), "decision-signing.key"); +} + +function assertOwnedByCurrentUser(stats: Stats, description: string) { + if (process.platform === "win32") return; + + const currentUserId = process.getuid?.(); + if (currentUserId !== undefined && stats.uid !== currentUserId) { + throw new Error(`${description} must be owned by the Paperclip process user`); + } +} + +function enforceKeyFilePermissions(keyPath: string) { + let stats = lstatSync(keyPath); + if (!stats.isFile()) { + throw new Error(`Decision signing key at ${keyPath} must be a regular file`); + } + assertOwnedByCurrentUser(stats, `Decision signing key at ${keyPath}`); + if (process.platform === "win32") return; + + const mode = stats.mode & 0o777; + if ((mode & 0o077) !== 0) { + chmodSync(keyPath, 0o600); + stats = lstatSync(keyPath); + if (!stats.isFile()) { + throw new Error(`Decision signing key at ${keyPath} must be a regular file`); + } + assertOwnedByCurrentUser(stats, `Decision signing key at ${keyPath}`); + if ((stats.mode & 0o077) !== 0) { + throw new Error(`Decision signing key at ${keyPath} must have permissions 0600`); + } + } +} + +function enforceSecretsDirectoryPermissions(directoryPath: string) { + let stats = lstatSync(directoryPath); + if (!stats.isDirectory()) { + throw new Error(`Decision signing secrets directory at ${directoryPath} must be a directory`); + } + assertOwnedByCurrentUser(stats, `Decision signing secrets directory at ${directoryPath}`); + if (process.platform === "win32") return; + + const mode = stats.mode & 0o777; + if ((mode & 0o077) !== 0) { + chmodSync(directoryPath, 0o700); + stats = lstatSync(directoryPath); + if (!stats.isDirectory()) { + throw new Error(`Decision signing secrets directory at ${directoryPath} must be a directory`); + } + assertOwnedByCurrentUser(stats, `Decision signing secrets directory at ${directoryPath}`); + if ((stats.mode & 0o077) !== 0) { + throw new Error(`Decision signing secrets directory at ${directoryPath} must have permissions 0700`); + } + } +} + +function readGeneratedSecret(keyPath: string): string { + enforceKeyFilePermissions(keyPath); + const existing = readFileSync(keyPath, "utf8").trim(); + if (existing.length < MIN_SECRET_LENGTH) { + throw new Error( + `Invalid decision signing key at ${keyPath} (must be at least ${MIN_SECRET_LENGTH} characters); remove the file to regenerate it or set PAPERCLIP_DECISION_SIGNING_SECRET`, + ); + } + return existing; +} + +function isAlreadyExists(error: unknown) { + return (error as NodeJS.ErrnoException).code === "EEXIST"; +} + +function isNotFound(error: unknown) { + return (error as NodeJS.ErrnoException).code === "ENOENT"; +} + +function loadOrCreateGeneratedSecret(): string { + const keyPath = resolveGeneratedSecretFilePath(); + const secretsDirectoryPath = path.dirname(keyPath); + try { + enforceSecretsDirectoryPermissions(secretsDirectoryPath); + return readGeneratedSecret(keyPath); + } catch (error) { + if (!isNotFound(error)) throw error; + } + + mkdirSync(secretsDirectoryPath, { recursive: true, mode: 0o700 }); + enforceSecretsDirectoryPermissions(secretsDirectoryPath); + const generated = randomBytes(32).toString("base64"); + const temporaryPath = `${keyPath}.${process.pid}.${randomBytes(8).toString("hex")}.tmp`; + + try { + writeFileSync(temporaryPath, generated, { encoding: "utf8", mode: 0o600, flag: "wx" }); + enforceKeyFilePermissions(temporaryPath); + + try { + // Publish only a complete key. A hard link is atomic and never replaces + // a key another server process created first. + linkSync(temporaryPath, keyPath); + enforceKeyFilePermissions(keyPath); + return generated; + } catch (error) { + if (!isAlreadyExists(error)) throw error; + return readGeneratedSecret(keyPath); + } + } finally { + try { + unlinkSync(temporaryPath); + } catch (error) { + if (!isNotFound(error)) throw error; + } + } +} + +export function resolveDecisionSigningSecret(): string { + const fromEnv = process.env.PAPERCLIP_DECISION_SIGNING_SECRET?.trim(); + if (fromEnv) { + if (fromEnv.length < MIN_SECRET_LENGTH) { + throw new Error( + `PAPERCLIP_DECISION_SIGNING_SECRET must be at least ${MIN_SECRET_LENGTH} characters when set (unset it to use an auto-generated key)`, + ); + } + return fromEnv; + } + return loadOrCreateGeneratedSecret(); +} + +/** + * Startup guard: resolves the signing secret once so an invalid explicit + * PAPERCLIP_DECISION_SIGNING_SECRET fails fast and the generated key file is + * materialized before the first decision write. A missing env var is not an + * error — the secret is auto-generated and persisted per instance. + */ +export function ensureDecisionSigningSecret() { + resolveDecisionSigningSecret(); } function canonical(value: unknown): string { @@ -18,7 +153,7 @@ function canonical(value: unknown): string { } export function signDecisionSpec(value: unknown) { - return `${VERSION}.${createHmac("sha256", validateDecisionSigningSecret()).update(`${VERSION}:${canonical(value)}`).digest("hex")}`; + return `${VERSION}.${createHmac("sha256", resolveDecisionSigningSecret()).update(`${VERSION}:${canonical(value)}`).digest("hex")}`; } export function verifyDecisionSpec(value: unknown, signature: string) {