fix(server): stop requiring PAPERCLIP_DECISION_SIGNING_SECRET at startup (#10594)
## 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` at9c1f8e7887. 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` at9c1f8e7887. **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 `<instance>/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 <noreply@paperclip.ing>
This commit is contained in:
Dotta
GitHub
parent
b163c6c473
commit
92c3d9f0d9
|
|
@ -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 () => {
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -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<StartedServer> {
|
|||
// 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) {
|
||||
|
|
|
|||
|
|
@ -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) {
|
||||
|
|
|
|||
Loading…
Reference in New Issue