From 313d6ca11536da73eb60ab27f2f10b5732dea2d8 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Thu, 3 Sep 2026 14:12:17 -0500 Subject: [PATCH 1/5] fix(runner): materialize pinned OpenCode binary (#12782) ## Thinking Path > - Paperclip manages AI agents and their provider runtimes. > - Paid runner validation installs target dependencies with lifecycle scripts disabled. > - OpenCode leaves a sentinel executable until its package lifecycle script runs. > - Running arbitrary lifecycle code would weaken the paid-secret boundary. > - This pull request materializes one exact pinned binary before secrets are exposed. > - The benefit is working OpenCode validation without trusting dependency install scripts. ## Linked Issues or Issue Description **What happened?** Every local OpenCode paid cell stopped before provider startup because `pnpm install --ignore-scripts` correctly retained `opencode-ai/bin/opencode.exe` as a sentinel. **Expected behavior** The trusted workflow must make the exact lockfile-pinned OpenCode executable available without running package lifecycle scripts. **Steps to reproduce** Run a local legacy or native OpenCode paid cell from the trusted workflow after the target dependency install. The provider health check reports that the OpenCode postinstall script was not run. **Paperclip version or commit** Default branch commit `865b4854fb44d3689f1c0ff17e3e715d52aaea73`. ## What Changed - Materialize only `opencode-linux-x64-baseline@1.18.17` into the matching `opencode-ai@1.18.17` package. - Verify package identity, version, regular-file type, SHA-256 equality, executable permissions, and runtime `--version`. - Invoke the helper for local OpenCode and breadth cells and for remote provider-pack assembly. - Retain `pnpm install --ignore-scripts`. - Add helper and trusted-workflow security regressions. ## Verification - Helper syntax checks passed. - Helper unit tests passed: 2/2. - Workflow-security tests passed: 5/5. - Prettier, actionlint, and diff whitespace checks passed. ## Risks Risk is low and contained to paid runner setup. The helper supports only Linux x64, fails closed on package or version drift, and runs before provider credentials enter the job. ## Model Used OpenAI GPT-5 Codex with repository tools and code execution. ## Checklist - [x] I have included a thinking path that traces from project context to this change. - [x] I have specified the model used. - [x] I have checked ROADMAP.md and confirmed this does not duplicate planned core work. - [x] I have searched GitHub for duplicate or related PRs and found none. - [x] I have described the issue in this PR with the bug template labels. - [x] I have not referenced internal or instance-local issues. - [x] My branch name describes the change. - [x] Focused local tests pass. - [x] I added tests for the change. - [x] I updated the runner E2E security documentation. - [x] I documented the risks above. --- .github/workflows/runner-full-stack-e2e.yml | 8 ++ packages/paperclip-runner/package.json | 2 +- .../scripts/materialize-opencode-binary.mjs | 109 ++++++++++++++++++ .../materialize-opencode-binary.test.mjs | 107 +++++++++++++++++ tests/runner-e2e/SECURITY.md | 5 +- tests/runner-e2e/workflow-security.test.ts | 14 +++ 6 files changed, 242 insertions(+), 3 deletions(-) create mode 100644 packages/paperclip-runner/scripts/materialize-opencode-binary.mjs create mode 100644 packages/paperclip-runner/scripts/materialize-opencode-binary.test.mjs diff --git a/.github/workflows/runner-full-stack-e2e.yml b/.github/workflows/runner-full-stack-e2e.yml index 31719b0d3f..0568f01781 100644 --- a/.github/workflows/runner-full-stack-e2e.yml +++ b/.github/workflows/runner-full-stack-e2e.yml @@ -619,6 +619,10 @@ jobs: - if: needs.catalog.outputs.needs_remote_provider_pack == 'true' run: pnpm install --frozen-lockfile --ignore-scripts + - name: Materialize verified pinned OpenCode executable + if: needs.catalog.outputs.needs_remote_provider_pack == 'true' + run: node packages/paperclip-runner/scripts/materialize-opencode-binary.mjs + - name: Download immutable shared campaign outputs if: needs.catalog.outputs.needs_remote_provider_pack == 'true' uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8 @@ -762,6 +766,10 @@ jobs: # the protected environment during setup. - run: pnpm install --frozen-lockfile --ignore-scripts + - name: Materialize verified pinned OpenCode executable + if: matrix.environmentId == 'local' && (matrix.profileId == 'legacy-opencode' || matrix.profileId == 'runner-opencode' || matrix.suiteId == 'openrouter-model-breadth') + run: node packages/paperclip-runner/scripts/materialize-opencode-binary.mjs + - name: Download immutable campaign outputs uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8 with: diff --git a/packages/paperclip-runner/package.json b/packages/paperclip-runner/package.json index 6f05a2cb52..dcff595e2f 100644 --- a/packages/paperclip-runner/package.json +++ b/packages/paperclip-runner/package.json @@ -73,7 +73,7 @@ "typecheck:rust": "cargo fmt --manifest-path runner/Cargo.toml --all -- --check && cargo check --manifest-path runner/Cargo.toml --locked --workspace", "typecheck:browser": "tsc -p tsconfig.browser.json --noEmit", "test": "pnpm run test:typescript && pnpm run test:rust", - "test:typescript": "pnpm run ensure:eval-build-deps && pnpm run build:rust && node --test test/protocol-contract.test.mjs test/acpx-sidecar-contract.test.mjs test/acpx-codex-package-contract.test.mjs scripts/aws-agentcore-provisioning.test.mjs && vitest run", + "test:typescript": "pnpm run ensure:eval-build-deps && pnpm run build:rust && node --test test/protocol-contract.test.mjs test/acpx-sidecar-contract.test.mjs test/acpx-codex-package-contract.test.mjs scripts/aws-agentcore-provisioning.test.mjs scripts/materialize-opencode-binary.test.mjs && vitest run", "test:rust": "cargo test --release --manifest-path runner/Cargo.toml --locked --workspace", "test:codex": "cargo test --manifest-path runner/Cargo.toml --locked -p paperclip-runner-core --test codex_provider", "test:durable": "cargo test --manifest-path runner/Cargo.toml --locked -p paperclip-runner-core durable::", diff --git a/packages/paperclip-runner/scripts/materialize-opencode-binary.mjs b/packages/paperclip-runner/scripts/materialize-opencode-binary.mjs new file mode 100644 index 0000000000..37cb04a85a --- /dev/null +++ b/packages/paperclip-runner/scripts/materialize-opencode-binary.mjs @@ -0,0 +1,109 @@ +import { createHash } from "node:crypto"; +import { spawnSync } from "node:child_process"; +import { + chmodSync, + copyFileSync, + existsSync, + linkSync, + lstatSync, + readFileSync, + realpathSync, + unlinkSync, +} from "node:fs"; +import { dirname, join, resolve } from "node:path"; +import { pathToFileURL } from "node:url"; + +const OPENCODE_VERSION = "1.18.17"; +const BASELINE_PACKAGE = "opencode-linux-x64-baseline"; + +function readPackage(path) { + return JSON.parse(readFileSync(path, "utf8")); +} + +function sha256(path) { + return createHash("sha256").update(readFileSync(path)).digest("hex"); +} + +function assertPackage(packageRoot, expectedName) { + const packageJson = readPackage(join(packageRoot, "package.json")); + if ( + packageJson.name !== expectedName || + packageJson.version !== OPENCODE_VERSION + ) { + throw new Error( + `Expected ${expectedName}@${OPENCODE_VERSION}, received ${String(packageJson.name)}@${String(packageJson.version)}`, + ); + } +} + +export function materializePinnedOpenCodeBinary(options = {}) { + const platform = options.platform ?? process.platform; + const architecture = options.architecture ?? process.arch; + if (platform !== "linux" || architecture !== "x64") { + throw new Error( + `Pinned OpenCode materialization requires linux/x64, received ${platform}/${architecture}`, + ); + } + + const packageRoot = realpathSync( + options.packageRoot ?? + resolve(import.meta.dirname, "../node_modules/opencode-ai"), + ); + const dependencyRoot = dirname(packageRoot); + const baselineRoot = realpathSync(join(dependencyRoot, BASELINE_PACKAGE)); + assertPackage(packageRoot, "opencode-ai"); + assertPackage(baselineRoot, BASELINE_PACKAGE); + + const source = join(baselineRoot, "bin", "opencode"); + const target = join(packageRoot, "bin", "opencode.exe"); + if (!lstatSync(source).isFile()) { + throw new Error("Pinned OpenCode source executable is not a regular file"); + } + if (existsSync(target)) { + if (!lstatSync(target).isFile()) { + throw new Error("OpenCode target executable is not a regular file"); + } + unlinkSync(target); + } + try { + linkSync(source, target); + } catch (error) { + const code = error?.code; + if (!new Set(["EACCES", "EMLINK", "EPERM", "EXDEV"]).has(code)) { + throw error; + } + copyFileSync(source, target); + } + chmodSync(target, 0o755); + + const sourceDigest = sha256(source); + const targetDigest = sha256(target); + if (sourceDigest !== targetDigest) { + throw new Error("Materialized OpenCode executable digest mismatch"); + } + const targetStat = lstatSync(target); + const mode = targetStat.mode & 0o777; + if (!targetStat.isFile() || (mode & 0o111) === 0 || mode & 0o022) { + throw new Error("Materialized OpenCode executable has unsafe permissions"); + } + + const version = spawnSync(target, ["--version"], { + encoding: "utf8", + timeout: 30_000, + windowsHide: true, + }); + if (version.status !== 0 || version.stdout.trim() !== OPENCODE_VERSION) { + throw new Error( + `Materialized OpenCode executable did not report ${OPENCODE_VERSION}`, + ); + } + return { sourceDigest, target, version: OPENCODE_VERSION }; +} + +const invokedPath = process.argv[1] + ? pathToFileURL(realpathSync(process.argv[1])).href + : null; +if (invokedPath === import.meta.url) { + const result = materializePinnedOpenCodeBinary(); + process.stdout.write(`${JSON.stringify(result)}\n`); +} diff --git a/packages/paperclip-runner/scripts/materialize-opencode-binary.test.mjs b/packages/paperclip-runner/scripts/materialize-opencode-binary.test.mjs new file mode 100644 index 0000000000..b6f7bbfe7d --- /dev/null +++ b/packages/paperclip-runner/scripts/materialize-opencode-binary.test.mjs @@ -0,0 +1,107 @@ +import assert from "node:assert/strict"; +import { + chmod, + mkdir, + mkdtemp, + rm, + symlink, + writeFile, +} from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, test } from "node:test"; +import { materializePinnedOpenCodeBinary } from "./materialize-opencode-binary.mjs"; + +const temporaryDirectories = []; +afterEach(async () => { + await Promise.all( + temporaryDirectories + .splice(0) + .map((directory) => rm(directory, { recursive: true, force: true })), + ); +}); + +async function fixture(options = {}) { + const root = await mkdtemp(join(tmpdir(), "paperclip-opencode-binary-")); + temporaryDirectories.push(root); + const packageRoot = join(root, "opencode-ai"); + const baselineRoot = join(root, "opencode-linux-x64-baseline"); + await Promise.all([ + mkdir(join(packageRoot, "bin"), { recursive: true }), + mkdir(join(baselineRoot, "bin"), { recursive: true }), + ]); + await Promise.all([ + writeFile( + join(packageRoot, "package.json"), + JSON.stringify({ + name: "opencode-ai", + version: options.packageVersion ?? "1.18.17", + }), + ), + writeFile( + join(baselineRoot, "package.json"), + JSON.stringify({ + name: "opencode-linux-x64-baseline", + version: options.baselineVersion ?? "1.18.17", + }), + ), + writeFile(join(packageRoot, "bin", "opencode.exe"), "sentinel\n"), + ]); + const source = join(baselineRoot, "bin", "opencode"); + if (options.symlinkSource) { + const realSource = join(root, "real-opencode"); + await writeFile(realSource, "#!/bin/sh\necho 1.18.17\n"); + await chmod(realSource, 0o755); + await symlink(realSource, source); + } else { + await writeFile(source, "#!/bin/sh\necho 1.18.17\n"); + await chmod(source, 0o755); + } + return packageRoot; +} + +test("materializes the pinned baseline executable with a verified version", async () => { + const packageRoot = await fixture(); + const result = materializePinnedOpenCodeBinary({ + packageRoot, + platform: "linux", + architecture: "x64", + }); + assert.equal(result.version, "1.18.17"); + assert.match(result.sourceDigest, /^[0-9a-f]{64}$/); +}); + +test("refuses version, file-type, and platform drift", async () => { + const wrongVersion = await fixture({ baselineVersion: "1.18.18" }); + assert.throws( + () => + materializePinnedOpenCodeBinary({ + packageRoot: wrongVersion, + platform: "linux", + architecture: "x64", + }), + /Expected opencode-linux-x64-baseline@1\.18\.17/, + ); + + const symlinkSource = await fixture({ symlinkSource: true }); + assert.throws( + () => + materializePinnedOpenCodeBinary({ + packageRoot: symlinkSource, + platform: "linux", + architecture: "x64", + }), + /source executable is not a regular file/, + ); + + const unsupported = await fixture(); + assert.throws( + () => + materializePinnedOpenCodeBinary({ + packageRoot: unsupported, + platform: "darwin", + architecture: "arm64", + }), + /requires linux\/x64/, + ); +}); diff --git a/tests/runner-e2e/SECURITY.md b/tests/runner-e2e/SECURITY.md index cba5da13d6..1b812b225d 100644 --- a/tests/runner-e2e/SECURITY.md +++ b/tests/runner-e2e/SECURITY.md @@ -31,8 +31,9 @@ must never run repository lifecycle scripts. The shared-build and provider-pack jobs also receive no provider credentials and disable dependency lifecycle scripts; they package outputs with SHA-256 sidecars that consumers verify before extraction. The paid test job installs with lifecycle scripts disabled, -and provider secrets are scoped only to its final test step rather than -dependency setup. Report sanitization and AWS +and materializes the exact pinned OpenCode executable from its lockfile-verified +optional package without invoking package lifecycle code. Provider secrets are +scoped only to the final test step rather than dependency setup. Report sanitization and AWS history publication explicitly use the trusted workflow commit and do not consume the target lockfile. Never run the workflow definition from the target branch. diff --git a/tests/runner-e2e/workflow-security.test.ts b/tests/runner-e2e/workflow-security.test.ts index 1393260125..9f6c0039d0 100644 --- a/tests/runner-e2e/workflow-security.test.ts +++ b/tests/runner-e2e/workflow-security.test.ts @@ -306,6 +306,9 @@ describe("public repository paid workflow security", () => { expect(buildJob).toContain( "node packages/paperclip-runner/scripts/build-provider-pack.mjs", ); + expect(buildJob).toContain( + "node packages/paperclip-runner/scripts/materialize-opencode-binary.mjs", + ); expect(buildJob).toContain("runner-e2e-build-bundle.tar.gz.sha256"); expect(buildJob).toContain("runner-e2e-provider-pack.tar.gz.sha256"); expect(buildJob).toContain( @@ -339,6 +342,17 @@ describe("public repository paid workflow security", () => { ); expect(testJob).toContain(".payload.runnerSourceRevision == $revision"); expect(workflow).toContain("Qualify local provider Node interpreter"); + expect(testJob).toContain( + "Materialize verified pinned OpenCode executable", + ); + expect(testJob).toContain( + "matrix.profileId == 'legacy-opencode' || matrix.profileId == 'runner-opencode' || matrix.suiteId == 'openrouter-model-breadth'", + ); + expect(testJob).toContain( + "node packages/paperclip-runner/scripts/materialize-opencode-binary.mjs", + ); + expect(testJob).not.toContain("postinstall.mjs"); + expect(testJob).not.toContain("pnpm rebuild"); expect(testJob).not.toContain("build:typescript"); expect(testJob).not.toContain("build:runner-binaries"); expect(testJob).not.toContain("build-provider-pack.mjs"); From 31a63638ac22b263d692a9a87b67b8f09c289797 Mon Sep 17 00:00:00 2001 From: Paperclip DevOps Engineer Date: Thu, 3 Sep 2026 12:13:26 -0700 Subject: [PATCH 2/5] fix(agents): redact plaintext env values in agent read and mutation responses (#9860) 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 > - Agents are configured through `adapterConfig`, whose `env` block holds the credentials an agent needs to talk to its provider (API keys, tokens, and similar) > - Those bindings come in several shapes: a legacy bare string, `{ type: "plain", value }`, and the indirection forms `{ type: "secret_ref" }` / `{ type: "user_secret_ref" }` > - Every endpoint that serializes an agent returned `adapterConfig` as stored, so every `plain` binding was returned verbatim in the API response > - That means any caller able to read an agent — including the agent itself via `GET /api/agents/me` — received live credentials in plaintext, and those values then propagate into client state, logs, and network traces > - The exposure spans three response families that share no common serializer: the single-agent detail reads, the company agent-list read, and the create/update/lifecycle routes that echo the stored row straight back > - This pull request routes all three through one presenter that redacts `plain` env bindings, so the leak is closed server-side and cannot be bypassed by the caller > - The benefit is that agent credentials stop appearing in API responses while `secret_ref` indirection continues to work unchanged ## Linked Issues or Issue Description No public upstream issue exists for this, so the underlying bug is described inline below following `.github/ISSUE_TEMPLATE/bug_report.yml`. **What happened?** Every endpoint that serializes an agent returned the full plaintext value of each `adapterConfig.env` entry whose `type` was `"plain"` (and each legacy bare-string binding). Any actor authorized to read an agent received that agent's live credentials in the response body. Three distinct response families were affected: - **Single-agent reads** — `GET /api/agents/{id}` and `GET /api/agents/me`, via `buildAgentDetail`. - **Company agent list** — `GET /api/companies/{companyId}/agents`, which serializes rows directly and therefore does not inherit any fix applied to `buildAgentDetail`. Callers that pass the configuration-read check received unredacted rows for every agent in the company in a single request, making this the broadest of the three. - **Mutation responses** — agent create, `PATCH /api/agents/{id}`, and the `pause` / `resume` / `clear-error` / `approve` / `terminate` routes, each of which echoes the stored row back to the caller. **Expected behavior** Read endpoints should never emit stored plaintext credentials. `plain` bindings should be replaced with a redaction sentinel before serialization, while `secret_ref` and `user_secret_ref` bindings — which contain no secret material — pass through untouched. **Steps to reproduce** 1. Configure an agent with an `adapterConfig.env` entry such as `{ "OPENAI_API_KEY": { "type": "plain", "value": "sk-example" } }`. 2. Call `GET /api/agents/{id}` (or authenticate as that agent and call `GET /api/agents/me`). 3. Observe `sk-example` returned verbatim in the response body. 4. Call `GET /api/companies/{companyId}/agents` as a configuration-reading caller and observe `sk-example` returned verbatim for that agent alongside every other agent's credentials. 5. Call `PATCH /api/agents/{id}` with any unrelated field (for example `{ "title": "Renamed" }`) and observe `sk-example` returned verbatim in the mutation response. **Paperclip version or commit** Reproduced on `master` at `f12bb27b`. **Deployment mode** Self-hosted / local development server. ## What Changed - `server/src/redaction.ts`: adds `redactAgentAdapterConfig`, which rewrites every bare-string or `{ type: "plain", value }` env binding to `{ type: "plain", value: "***REDACTED***" }` and passes `secret_ref` / `user_secret_ref` bindings through unchanged. Reuses the existing `REDACTED_EVENT_VALUE` and `isSecretRefBinding` / `isUserSecretRefBinding` / `isPlainBinding` helpers — no new dependencies. - `server/src/redaction.ts`: `env` is destructured out and sanitized only by `redactAgentEnvBinding`, while the remaining adapter keys go through `redactEventPayload`. Previously the already-redacted `env` was passed back through `sanitizeRecord`, so each binding was processed twice — safe only because the sentinel is a fixed point of that second pass. The two paths are now disjoint, making the invariant structural rather than coincidental. - `server/src/routes/agents.ts`: `buildAgentDetail` applies `redactAgentAdapterConfig` before serialization, so `GET /api/agents/{id}` and `GET /api/agents/me` both redact at the response layer. Restricted views inherit the same protection. - `server/src/routes/agents.ts`: adds `redactAgentRowForResponse`, the single presenter for every response that emits a raw agent row, and applies it to the company agent-list route and to the create / update / pause / resume / clear-error / approve / terminate routes. It composes with `redactForRestrictedAgentView` rather than replacing it: that helper is an authorization filter (blank the whole config for low-trust actors), this one is secret hygiene (mask values for every actor scope), and the two invariants stay independent. `buildAgentDetail` now delegates to the same presenter instead of inlining the call. - `server/src/routes/agents.ts`: adds `restoreRedactedAgentEnv` on the PATCH path so a client that round-trips a redacted detail response back through `PATCH /api/agents/{id}` does not zero out stored values — redacted-sentinel entries matching an existing key are restored from storage. ## Verification - `pnpm --filter @paperclipai/server exec tsc --noEmit` — clean. - `pnpm --filter @paperclipai/server exec vitest run agent-permissions-routes.test.ts` — 57 tests pass. - Adjacent suites (`redaction`, `agent-adapter-validation-routes`, `agent-cross-tenant-authz-routes`, `agents-pending-approval-config`, `agents-service-secret-bindings`, `built-in-agent-routes`, `plugin-managed-agents`, `agent-skills-routes`) — 8 files, 72 tests pass, no regressions. - Both new route tests were confirmed to **fail** with the route changes reverted and pass with them applied, so they genuinely pin the behaviour rather than passing incidentally. Tests added: - `server/src/__tests__/redaction.test.ts`: covers legacy-string, `{ type: "plain" }`, `secret_ref`, and `user_secret_ref` bindings, asserting the plaintext value never appears in the serialized result; plus coverage that non-env adapter keys are still sanitized, that env binding shapes survive intact, and that configs with no `env` block are handled. - `server/src/__tests__/agent-permissions-routes.test.ts`: `GET /api/agents/{id}` asserts redaction rather than plaintext passthrough; new `GET /api/agents/me` redaction test across the same binding shapes; new test asserting the `PATCH` round-trip preserves stored values; new test asserting the board `GET /api/companies/{companyId}/agents` response redacts every binding shape; new test asserting a mutation response redacts rather than echoing the stored plaintext. No real secret values appear in any test, fixture, or commit message. ## Risks - **Behavioral change for API consumers.** Any client that read a plaintext credential out of an agent detail, agent-list, or mutation response will now receive `***REDACTED***`. This is the intended security fix, but it is a breaking change for such consumers, which must move to `secret_ref` indirection. - **Mutation responses are redacted too.** Callers that previously relied on a create or update response to echo back the credential they had just written must now read it from their own request. This is consistent with the `restoreRedactedAgentEnv` round-trip path, which already assumes the client holds a redacted copy. - **Round-trip data loss, mitigated.** A client that GETs an agent and PATCHes the object straight back would otherwise persist the sentinel over the real value. `restoreRedactedAgentEnv` restores redacted entries from storage; the round-trip is covered by a regression test. A PATCH that *intentionally* sets a value literally equal to the sentinel is not distinguishable and would be treated as "unchanged" — an acceptable trade-off given the sentinel is not a plausible credential. - **No migration.** Stored data is untouched; redaction happens purely at serialization time, so the change is fully reversible by revert. - **Overlap with existing PRs** — see the duplicate-search note below. Maintainers may prefer to consolidate rather than merge this in isolation. ## Model Used Claude Opus 4.8 (`claude-opus-4-8`), extended thinking enabled, with tool use and local test execution. ## Duplicate search Searching open and closed PRs for prior art surfaced several overlapping efforts against the same defect. Linking them for maintainer triage — I am not claiming this PR supersedes them, and consolidation may well be preferable: - #9823 — `fix(security): redact adapterConfig secrets on all agent read endpoints` (closest overlap) - #8779 — `fix(server): redact agent config secrets in read and mutation responses` - #8330 — `fix(server): redact adapterConfig.env for cross-actor agent reads` - #4856 — `fix(server): redact adapter env secrets in agent API responses` - #4763 — `fix(server): redact adapter_config secrets in agent detail responses` - #1839 — `fix: redact secret env vars from agent API responses` - #4967 — `fix(routines): redact adapterConfig.env in GET /api/routines/{id}` (same class, routines surface) ## 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 searched the GitHub PR list (open and closed) for similar or duplicate 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) - [ ] My branch name describes the change and contains no internal Paperclip ticket id — **not met**: the branch and title carry an internal ticket id. Renaming the branch would invalidate this PR; happy to reopen from a clean branch if maintainers prefer. - [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 (no user-facing docs affected) - [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 — the one P2 (env entries processed twice) is addressed above - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip Co-authored-by: Matthew Glover <5413384+glovario@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 Co-authored-by: Andrew Aymeloglu --- .../agent-permissions-routes.test.ts | 310 +++++++++++++++++- server/src/__tests__/redaction.test.ts | 70 ++++ server/src/redaction.ts | 29 ++ server/src/routes/agents.ts | 77 ++++- 4 files changed, 468 insertions(+), 18 deletions(-) diff --git a/server/src/__tests__/agent-permissions-routes.test.ts b/server/src/__tests__/agent-permissions-routes.test.ts index 4eadf5318a..cc6fe817e4 100644 --- a/server/src/__tests__/agent-permissions-routes.test.ts +++ b/server/src/__tests__/agent-permissions-routes.test.ts @@ -43,6 +43,7 @@ const baseAgent = { const mockAgentService = vi.hoisted(() => ({ getById: vi.fn(), getConfigRevision: vi.fn(), + listConfigRevisions: vi.fn(), list: vi.fn(), create: vi.fn(), activatePendingApproval: vi.fn(), @@ -291,6 +292,7 @@ describe.sequential("agent permission routes", () => { vi.resetAllMocks(); mockAgentService.getById.mockReset(); mockAgentService.getConfigRevision.mockReset(); + mockAgentService.listConfigRevisions.mockReset(); mockAgentService.list.mockReset(); mockAgentService.create.mockReset(); mockAgentService.activatePendingApproval.mockReset(); @@ -337,6 +339,7 @@ describe.sequential("agent permission routes", () => { mockGetTelemetryClient.mockReturnValue({ track: vi.fn() }); mockAgentService.getById.mockResolvedValue(baseAgent); mockAgentService.getConfigRevision.mockResolvedValue(null); + mockAgentService.listConfigRevisions.mockResolvedValue([]); mockAgentService.list.mockResolvedValue([baseAgent]); mockAgentService.getChainOfCommand.mockResolvedValue([]); mockAgentService.resolveByReference.mockResolvedValue({ ambiguous: false, agent: baseAgent }); @@ -422,7 +425,8 @@ describe.sequential("agent permission routes", () => { expect(res.body.runtimeConfig).toEqual({}); }, 20_000); - it("keeps board agent detail unredacted for low-trust agents", async () => { + it("redacts env values in board agent detail responses", async () => { + const plaintextValue = "plain-value-must-not-leak"; mockAgentService.getById.mockResolvedValue({ ...baseAgent, permissions: { @@ -431,7 +435,15 @@ describe.sequential("agent permission routes", () => { }, adapterConfig: { command: "pnpm agent:run", - env: { PAPERCLIP_API_KEY: "secret-test-key" }, + env: { + LEGACY_VALUE: plaintextValue, + PLAIN_VALUE: { type: "plain", value: plaintextValue }, + SECRET_REFERENCE: { + type: "secret_ref", + secretId: "33333333-3333-4333-8333-333333333333", + version: "latest", + }, + }, }, runtimeConfig: { heartbeat: { enabled: false }, @@ -451,14 +463,190 @@ describe.sequential("agent permission routes", () => { expect(res.status).toBe(200); expect(res.body.adapterConfig).toMatchObject({ command: "pnpm agent:run", - env: { PAPERCLIP_API_KEY: "secret-test-key" }, + env: { + LEGACY_VALUE: { type: "plain", value: "***REDACTED***" }, + PLAIN_VALUE: { type: "plain", value: "***REDACTED***" }, + SECRET_REFERENCE: { + type: "secret_ref", + secretId: "33333333-3333-4333-8333-333333333333", + version: "latest", + }, + }, }); + expect(JSON.stringify(res.body)).not.toContain(plaintextValue); expect(res.body.runtimeConfig).toMatchObject({ heartbeat: { enabled: false }, }); expect(res.body.permissions).toMatchObject({ trustPreset: LOW_TRUST_REVIEW_PRESET }); }, 20_000); + // TEC-7032 reported the leak against the company agent-list endpoint, which + // serialises rows directly instead of going through buildAgentDetail. + it("redacts env values in board GET /api/companies/:companyId/agents responses", async () => { + const plaintextValue = "listed-value-must-not-leak"; + mockAgentService.list.mockResolvedValue([ + { + ...baseAgent, + adapterConfig: { + command: "pnpm agent:run", + env: { + LEGACY_VALUE: plaintextValue, + PLAIN_VALUE: { type: "plain", value: plaintextValue }, + SECRET_REFERENCE: { + type: "secret_ref", + secretId: "55555555-5555-4555-8555-555555555555", + version: "latest", + }, + }, + }, + }, + ]); + + const app = await createApp({ + type: "board", + userId: "board-user", + source: "local_implicit", + isInstanceAdmin: true, + companyIds: [companyId], + }); + + const res = await requestApp(app, (baseUrl) => + request(baseUrl).get(`/api/companies/${companyId}/agents`), + ); + + expect(res.status).toBe(200); + expect(res.body).toHaveLength(1); + expect(res.body[0].adapterConfig).toMatchObject({ + command: "pnpm agent:run", + env: { + LEGACY_VALUE: { type: "plain", value: "***REDACTED***" }, + PLAIN_VALUE: { type: "plain", value: "***REDACTED***" }, + SECRET_REFERENCE: { + type: "secret_ref", + secretId: "55555555-5555-4555-8555-555555555555", + version: "latest", + }, + }, + }); + expect(JSON.stringify(res.body)).not.toContain(plaintextValue); + }, 20_000); + + // Mutation routes echo the stored row back, so they leak the same values the + // GET paths redact. + it("redacts env values in agent mutation responses", async () => { + const plaintextValue = "mutation-value-must-not-leak"; + const storedAgent = { + ...baseAgent, + adapterConfig: { + env: { PLAIN_VALUE: { type: "plain", value: plaintextValue } }, + }, + }; + mockAgentService.getById.mockResolvedValue(storedAgent); + mockAgentService.update.mockResolvedValue(storedAgent); + + const app = await createApp({ + type: "board", + userId: "board-user", + source: "local_implicit", + isInstanceAdmin: true, + companyIds: [companyId], + }); + + const res = await requestApp(app, (baseUrl) => + request(baseUrl).patch(`/api/agents/${agentId}`).send({ title: "Renamed" }), + ); + + expect(res.status).toBe(200); + expect(res.body.adapterConfig.env).toEqual({ + PLAIN_VALUE: { type: "plain", value: "***REDACTED***" }, + }); + expect(JSON.stringify(res.body)).not.toContain(plaintextValue); + }, 20_000); + + it("redacts env values in GET /api/agents/me responses", async () => { + const plaintextValue = "self-value-must-not-leak"; + mockAgentService.getById.mockResolvedValue({ + ...baseAgent, + adapterConfig: { + env: { + EXISTING_VALUE: plaintextValue, + NEW_VALUE: { type: "plain", value: plaintextValue }, + SECRET_REFERENCE: { + type: "secret_ref", + secretId: "44444444-4444-4444-8444-444444444444", + version: 2, + }, + }, + }, + }); + + const app = await createApp({ + type: "agent", + agentId, + companyId, + runId: null, + source: "agent_key", + }); + + const res = await requestApp(app, (baseUrl) => request(baseUrl).get("/api/agents/me")); + + expect(res.status).toBe(200); + expect(res.body.adapterConfig.env).toEqual({ + EXISTING_VALUE: { type: "plain", value: "***REDACTED***" }, + NEW_VALUE: { type: "plain", value: "***REDACTED***" }, + SECRET_REFERENCE: { + type: "secret_ref", + secretId: "44444444-4444-4444-8444-444444444444", + version: 2, + }, + }); + expect(JSON.stringify(res.body)).not.toContain(plaintextValue); + }, 20_000); + + it("preserves stored env values when a redacted detail response is submitted unchanged", async () => { + const plaintextValue = "stored-value-must-be-preserved"; + mockAgentService.getById.mockResolvedValue({ + ...baseAgent, + adapterConfig: { + env: { + EXISTING_VALUE: { type: "plain", value: plaintextValue }, + }, + }, + }); + mockAgentService.update.mockResolvedValue(baseAgent); + + const app = await createApp({ + type: "board", + userId: "board-user", + source: "local_implicit", + isInstanceAdmin: true, + companyIds: [companyId], + }); + + const redactedResponse = await requestApp(app, (baseUrl) => + request(baseUrl).get(`/api/agents/${agentId}`), + ); + expect(redactedResponse.status).toBe(200); + const redactedEnv = redactedResponse.body.adapterConfig.env as Record; + expect(redactedEnv.EXISTING_VALUE).toEqual({ type: "plain", value: "***REDACTED***" }); + + const patchRes = await requestApp(app, (baseUrl) => + request(baseUrl).patch(`/api/agents/${agentId}`).send({ + title: "Renamed while redacted env round-trips", + adapterConfig: redactedResponse.body.adapterConfig, + }), + ); + + expect(patchRes.status).toBe(200); + const updateCallArgs = mockAgentService.update.mock.calls[0]?.[1] as + | { adapterConfig?: Record } + | undefined; + expect(updateCallArgs?.adapterConfig?.env).toEqual({ + EXISTING_VALUE: { type: "plain", value: plaintextValue }, + }); + expect(JSON.stringify(updateCallArgs?.adapterConfig ?? {})).not.toContain("***REDACTED***"); + }, 20_000); + it("redacts company agent list for authenticated company members without agent admin permission", async () => { mockAccessService.canUser.mockResolvedValue(false); mockAccessService.decide.mockImplementation(async (input: { action?: string }) => ({ @@ -628,6 +816,43 @@ describe.sequential("agent permission routes", () => { expect(mockAgentService.rollbackConfigRevision).not.toHaveBeenCalled(); }); + it("redacts plaintext env values in configuration rollback responses", async () => { + const revisionId = "33333333-3333-4333-8333-333333333333"; + const plaintextValue = "rollback-value-must-not-leak"; + mockAgentService.getConfigRevision.mockResolvedValue({ + id: revisionId, + afterConfig: { + adapterType: "process", + adapterConfig: {}, + runtimeConfig: {}, + }, + }); + mockAgentService.rollbackConfigRevision.mockResolvedValue({ + ...baseAgent, + adapterConfig: { + env: { GENERIC_NAME: { type: "plain", value: plaintextValue } }, + }, + }); + + const app = await createApp({ + type: "board", + userId: "instance-admin-user", + source: "session", + isInstanceAdmin: true, + companyIds: [companyId], + }); + + const res = await requestApp(app, (baseUrl) => + request(baseUrl).post(`/api/agents/${agentId}/config-revisions/${revisionId}/rollback`), + ); + + expect(res.status).toBe(200); + expect(res.body.adapterConfig.env).toEqual({ + GENERIC_NAME: { type: "plain", value: "***REDACTED***" }, + }); + expect(JSON.stringify(res.body)).not.toContain(plaintextValue); + }); + it("blocks api key creation for authenticated company members without agent admin permission", async () => { mockAccessService.canUser.mockResolvedValue(false); @@ -1649,6 +1874,13 @@ describe.sequential("agent permission routes", () => { // the read-only permission loosening introduced by this PR. mockAccessService.canUser.mockResolvedValue(false); mockAccessService.hasPermission.mockResolvedValue(false); + const plaintextValue = "configuration-value-must-not-leak"; + mockAgentService.getById.mockResolvedValue({ + ...baseAgent, + adapterConfig: { + env: { GENERIC_NAME: { type: "plain", value: plaintextValue } }, + }, + }); const app = await createApp({ type: "board", @@ -1661,6 +1893,78 @@ describe.sequential("agent permission routes", () => { const res = await request(app).get(`/api/agents/${agentId}/configuration`); expect(res.status).toBe(200); + expect(res.body.adapterConfig.env).toEqual({ + GENERIC_NAME: { type: "plain", value: "***REDACTED***" }, + }); + expect(JSON.stringify(res.body)).not.toContain(plaintextValue); + }); + + it("redacts plaintext env values in company configuration-list responses", async () => { + const plaintextValue = "configuration-list-value-must-not-leak"; + mockAgentService.list.mockResolvedValue([ + { + ...baseAgent, + adapterConfig: { + env: { GENERIC_NAME: { type: "plain", value: plaintextValue } }, + }, + }, + ]); + + const app = await createApp({ + type: "board", + userId: "board-user", + source: "session", + isInstanceAdmin: false, + companyIds: [companyId], + }); + + const res = await request(app).get(`/api/companies/${companyId}/agent-configurations`); + + expect(res.status).toBe(200); + expect(res.body[0].adapterConfig.env).toEqual({ + GENERIC_NAME: { type: "plain", value: "***REDACTED***" }, + }); + expect(JSON.stringify(res.body)).not.toContain(plaintextValue); + }); + + it("redacts plaintext env values in configuration revisions", async () => { + const plaintextValue = "revision-value-must-not-leak"; + mockAgentService.listConfigRevisions.mockResolvedValue([ + { + id: "33333333-3333-4333-8333-333333333333", + beforeConfig: { + adapterConfig: { + env: { GENERIC_NAME: { type: "plain", value: plaintextValue } }, + }, + }, + afterConfig: { + adapterConfig: { + env: { GENERIC_NAME: plaintextValue }, + }, + }, + }, + ]); + + const app = await createApp({ + type: "board", + userId: "board-user", + source: "session", + isInstanceAdmin: false, + companyIds: [companyId], + }); + + const res = await request(app).get(`/api/agents/${agentId}/config-revisions`); + + expect(res.status).toBe(200); + expect(res.body[0].beforeConfig.adapterConfig.env.GENERIC_NAME).toEqual({ + type: "plain", + value: "***REDACTED***", + }); + expect(res.body[0].afterConfig.adapterConfig.env.GENERIC_NAME).toEqual({ + type: "plain", + value: "***REDACTED***", + }); + expect(JSON.stringify(res.body)).not.toContain(plaintextValue); }); it("denies an agent actor without configure or suggest grants when reading peer config", async () => { diff --git a/server/src/__tests__/redaction.test.ts b/server/src/__tests__/redaction.test.ts index 2de97caf96..b42699be7c 100644 --- a/server/src/__tests__/redaction.test.ts +++ b/server/src/__tests__/redaction.test.ts @@ -3,6 +3,7 @@ import { describe, expect, it } from "vitest"; import { PRP_V1_EVENT_TYPES, REDACTED_EVENT_VALUE, + redactAgentAdapterConfig, redactEventPayload, redactSensitiveText, sanitizeRecord, @@ -626,4 +627,73 @@ second-line\" status=401`, expect(result?.args).toEqual(["--api-key", "not-a-command-secret"]); expect(result?.argv).toEqual(["--api-key", REDACTED_EVENT_VALUE]); }); + + it("redacts every plaintext agent env binding while preserving secret references", () => { + const plaintextValue = "adapter-env-value-must-not-leak"; + + const result = redactAgentAdapterConfig({ + command: "pnpm agent:run", + env: { + EXISTING_VALUE: plaintextValue, + NEW_VALUE: { type: "plain", value: plaintextValue }, + SECRET_REFERENCE: { + type: "secret_ref", + secretId: "55555555-5555-4555-8555-555555555555", + version: "latest", + }, + USER_SECRET_REFERENCE: { + type: "user_secret_ref", + key: "GITHUB_TOKEN", + }, + }, + }); + + expect(result).toEqual({ + command: "pnpm agent:run", + env: { + EXISTING_VALUE: { type: "plain", value: REDACTED_EVENT_VALUE }, + NEW_VALUE: { type: "plain", value: REDACTED_EVENT_VALUE }, + SECRET_REFERENCE: { + type: "secret_ref", + secretId: "55555555-5555-4555-8555-555555555555", + version: "latest", + }, + USER_SECRET_REFERENCE: { + type: "user_secret_ref", + key: "GITHUB_TOKEN", + }, + }, + }); + expect(JSON.stringify(result)).not.toContain(plaintextValue); + }); + + it("redacts non-env adapter keys while leaving env binding shapes intact", () => { + const result = redactAgentAdapterConfig({ + command: "pnpm agent:run", + apiKey: "adapter-level-secret", + env: { + API_KEY: "env-level-secret", + AUTH_TOKEN: { type: "plain", value: "another-env-secret" }, + }, + }); + + // Non-env keys still go through the shared payload sanitizer. + expect(result.apiKey).toBe(REDACTED_EVENT_VALUE); + expect(result.command).toBe("pnpm agent:run"); + + // Env bindings keep their binding shape rather than collapsing to a bare + // sentinel string, which is what a second sanitizer pass would produce for + // these sensitive-looking key names. + expect(result.env).toEqual({ + API_KEY: { type: "plain", value: REDACTED_EVENT_VALUE }, + AUTH_TOKEN: { type: "plain", value: REDACTED_EVENT_VALUE }, + }); + }); + + it("redacts adapter configs that have no env block", () => { + expect(redactAgentAdapterConfig({ command: "pnpm agent:run", apiKey: "secret" })).toEqual({ + command: "pnpm agent:run", + apiKey: REDACTED_EVENT_VALUE, + }); + }); }); diff --git a/server/src/redaction.ts b/server/src/redaction.ts index fe381887ea..e9a29c5531 100644 --- a/server/src/redaction.ts +++ b/server/src/redaction.ts @@ -941,6 +941,35 @@ export function redactEventPayload( return sanitized; } +function redactAgentEnvBinding(value: unknown): unknown { + if (isSecretRefBinding(value) || isUserSecretRefBinding(value)) { + return sanitizeValue(value); + } + if (typeof value === "string" || isPlainBinding(value)) { + return { type: "plain", value: REDACTED_EVENT_VALUE }; + } + if (value === null || value === undefined) return value; + return REDACTED_EVENT_VALUE; +} + +export function redactAgentAdapterConfig( + adapterConfig: Record, +): Record { + if (!isPlainObject(adapterConfig)) return adapterConfig; + if (!isPlainObject(adapterConfig.env)) return redactEventPayload(adapterConfig) ?? {}; + + // Redact `env` here and sanitize the remaining keys separately, so bindings + // are never processed twice. `redactAgentEnvBinding` is authoritative for + // `env`; keeping it out of `sanitizeRecord` means a future change there + // cannot alter entries this function has already redacted. + const { env, ...rest } = adapterConfig; + const redactedEnv = Object.fromEntries( + Object.entries(env).map(([key, value]) => [key, redactAgentEnvBinding(value)]), + ); + + return { ...(redactEventPayload(rest) ?? {}), env: redactedEnv }; +} + export function redactSensitiveText(input: string): string { if (!maybeContainsSecretText(input)) return input; return redactCommandText( diff --git a/server/src/routes/agents.ts b/server/src/routes/agents.ts index d13477d0a9..6e3873ddc7 100644 --- a/server/src/routes/agents.ts +++ b/server/src/routes/agents.ts @@ -105,7 +105,11 @@ import { refreshAdapterModels, requireServerAdapter, } from "../adapters/index.js"; -import { redactEventPayload } from "../redaction.js"; +import { + REDACTED_EVENT_VALUE, + redactAgentAdapterConfig, + redactEventPayload, +} from "../redaction.js"; import { redactCurrentUserValue } from "../log-redaction.js"; import { HarnessRuntimeRequestResolutionError, @@ -1483,8 +1487,12 @@ export function agentRoutes( buildAgentAccessState(agent), ]); + const baseAgent = redactAgentRowForResponse( + options?.restricted ? redactForRestrictedAgentView(agent) : agent, + ); + return { - ...(options?.restricted ? redactForRestrictedAgentView(agent) : agent), + ...baseAgent, chainOfCommand, access: accessState, }; @@ -2773,6 +2781,21 @@ export function agentRoutes( }; } + // Single presenter for every response that emits a raw agent row. Restricted + // views blank the config wholesale for authorization reasons; this runs for + // config-reading (board) callers too, so plaintext `adapterConfig.env` values + // never leave the API regardless of actor scope. + function redactAgentRowForResponse( + agent: T, + ): T { + if (!agent || typeof agent !== "object") return agent; + if (!agent.adapterConfig || typeof agent.adapterConfig !== "object") return agent; + return { + ...agent, + adapterConfig: redactAgentAdapterConfig(agent.adapterConfig as Record), + }; + } + function redactAgentConfiguration(agent: Awaited>) { if (!agent) return null; return { @@ -2784,19 +2807,41 @@ export function agentRoutes( status: agent.status, reportsTo: agent.reportsTo, adapterType: agent.adapterType, - adapterConfig: redactEventPayload(agent.adapterConfig), + adapterConfig: redactAgentAdapterConfig(agent.adapterConfig), runtimeConfig: redactEventPayload(agent.runtimeConfig), permissions: agent.permissions, updatedAt: agent.updatedAt, }; } + function restoreRedactedAgentEnv( + requestedConfig: Record, + existingConfig: Record, + ): Record { + const requestedEnv = asRecord(requestedConfig.env); + const existingEnv = asRecord(existingConfig.env); + if (!requestedEnv || !existingEnv) return requestedConfig; + + const restoredEnv = { ...requestedEnv }; + for (const [key, value] of Object.entries(requestedEnv)) { + const binding = asRecord(value); + if ( + binding?.type === "plain" + && binding.value === REDACTED_EVENT_VALUE + && Object.prototype.hasOwnProperty.call(existingEnv, key) + ) { + restoredEnv[key] = existingEnv[key]; + } + } + return { ...requestedConfig, env: restoredEnv }; + } + function redactRevisionSnapshot(snapshot: unknown): Record { if (!snapshot || typeof snapshot !== "object" || Array.isArray(snapshot)) return {}; const record = snapshot as Record; return { ...record, - adapterConfig: redactEventPayload( + adapterConfig: redactAgentAdapterConfig( typeof record.adapterConfig === "object" && record.adapterConfig !== null ? (record.adapterConfig as Record) : {}, @@ -3466,7 +3511,7 @@ export function agentRoutes( const result = await filterAgentsForActor(req, await svc.list(companyId)); const canReadConfigs = await actorCanReadConfigurationsForCompany(req, companyId); if (canReadConfigs) { - res.json(result); + res.json(result.map((agent) => redactAgentRowForResponse(agent))); return; } res.json(result.map((agent) => redactForRestrictedAgentView(agent))); @@ -3806,7 +3851,7 @@ export function agentRoutes( details: { revisionId }, }); - res.json(updated); + res.json(redactAgentRowForResponse(updated)); }); router.get("/agents/:id/runtime-state", async (req, res) => { @@ -4210,7 +4255,7 @@ export function agentRoutes( ); } - res.status(201).json(agent); + res.status(201).json(redactAgentRowForResponse(agent)); }); router.patch("/agents/:id/permissions", validate(updateAgentPermissionsSchema), async (req, res) => { @@ -4575,9 +4620,11 @@ export function agentRoutes( ) { await assertCanManageInstructionsPath(req, existing); } - let rawEffectiveAdapterConfig = requestedAdapterConfig ?? existingAdapterConfig; + let rawEffectiveAdapterConfig = requestedAdapterConfig + ? restoreRedactedAgentEnv(requestedAdapterConfig, existingAdapterConfig) + : existingAdapterConfig; if (requestedAdapterConfig && !changingAdapterType && !replaceAdapterConfig) { - rawEffectiveAdapterConfig = { ...existingAdapterConfig, ...requestedAdapterConfig }; + rawEffectiveAdapterConfig = { ...existingAdapterConfig, ...rawEffectiveAdapterConfig }; } if (changingAdapterType) { // Preserve adapter-agnostic keys (env, cwd, etc.) from the existing config @@ -4688,7 +4735,7 @@ export function agentRoutes( details: summarizeAgentUpdateDetails(patchData), }); - res.json(agent); + res.json(redactAgentRowForResponse(agent)); }); router.post("/agents/:id/pause", async (req, res) => { @@ -4714,7 +4761,7 @@ export function agentRoutes( entityId: agent.id, }); - res.json(agent); + res.json(redactAgentRowForResponse(agent)); }); router.post("/agents/:id/resume", async (req, res) => { @@ -4749,7 +4796,7 @@ export function agentRoutes( entityId: agent.id, }); - res.json(agent); + res.json(redactAgentRowForResponse(agent)); }); router.post("/agents/:id/clear-error", async (req, res) => { @@ -4781,7 +4828,7 @@ export function agentRoutes( entityId: agent.id, }); - res.json(agent); + res.json(redactAgentRowForResponse(agent)); }); router.post("/agents/:id/approve", async (req, res) => { @@ -4835,7 +4882,7 @@ export function agentRoutes( details: { source: "agent_detail", approvalId: openApproval?.id ?? null }, }); - res.json(agent); + res.json(redactAgentRowForResponse(agent)); }); router.post("/agents/:id/terminate", async (req, res) => { @@ -4905,7 +4952,7 @@ export function agentRoutes( }, }); - res.json(agent); + res.json(redactAgentRowForResponse(agent)); }); router.delete("/agents/:id", async (req, res) => { From 9dd6526b47f3ca3a228e5eb7378de055e1622d8a Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Thu, 3 Sep 2026 14:15:32 -0500 Subject: [PATCH 3/5] fix(security): harden privileged server boundaries (#12776) 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 controls secrets, host files, outbound requests, and workspace commands > - A red-team review found cases where restricted callers could cross these trust boundaries > - These cases could expose credentials or let untrusted input reach privileged resources > - This pull request applies least-privilege checks at each affected server boundary > - The benefit is safer agent execution without changing the private-instance bootstrap contract ## Linked Issues or Issue Description **What happened?** Several server paths used authorization, redaction, or content-delivery rules that were too broad. Restricted agent keys could obtain company-level operational data. Some adapter and instruction paths could reach server-owned network or file resources without the required owner approval. **Expected behavior** Paperclip must redact credential values, enforce restricted-key scopes, guard outbound network access, prevent same-origin script execution, and reserve host-level file and command controls for authorized operators. **Steps to reproduce** 1. Configure an authenticated development instance at the parent commit. 2. Exercise the affected APIs with a restricted agent key or a non-instance-admin company user. 3. Observe that the parent commit returns privileged data or accepts a privileged operation. 4. Repeat on this branch and observe a redacted response, a safe download, or an HTTP 403 response. **Paperclip version or commit** The findings reproduce from commit `39898ab22` and are fixed by this pull request. **Deployment mode** Authenticated self-hosted server and local development modes. **Installation method** Built from source with pnpm. ## What Changed - Redact generic secret `value` and `token` fields recursively in structured logs. - Classify exact and separator-suffixed `KEY` environment names as secrets in company exports. - Limit restricted self-identity responses and protect company run, log, and secret catalog APIs. - Route HTTP adapter requests through DNS-pinned SSRF protection with exact private-origin allowlisting. - Download HTML, SVG, and other script-capable assets with `nosniff` and a sandbox CSP. - Require instance-admin access for external instruction roots and exports that read them. - Block agent-authenticated host command persistence across supported workspace runtime shapes. - Apply the central runtime-management decision before workspace command controls. - Keep the documented first-user instance-admin claim contract unchanged. - Add regression tests and server-owner configuration documentation. ## Verification - `pnpm -r typecheck` passes. - The Node 24 remediation suite passes with 365 tests. It skips 25 environment-gated tests. - `pnpm build` passes under Node 24. - `git diff --check` passes. - The full local runner reaches known macOS-only general-server harness failures before the serialized route lane. The Linux PR matrix is the authoritative full-suite gate. ## Risks - Restricted agent keys now receive HTTP 403 responses from company-wide run, log, and secret catalog endpoints. - Script-capable assets now download instead of rendering inline. - External instruction roots now require instance-admin access. - Private HTTP adapter endpoints now require an exact origin in `PAPERCLIP_HTTP_ADAPTER_PRIVATE_ENDPOINT_ALLOWLIST`. - Public HTTP adapter endpoints remain enabled. Redirects and metadata or link-local targets remain blocked. - No database migration is required. > 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 OpenAI Codex, GPT-5. The exact serving snapshot and context-window size are not exposed. The model used tool-enabled reasoning, repository access, code execution, and test execution. ## 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 --- .env.example | 4 + doc/DEPLOYMENT-MODES.md | 6 + doc/DEVELOPING.md | 18 ++ doc/SPEC-implementation.md | 5 + .../agent-instructions-routes.test.ts | 203 +++++++++++++++++- .../__tests__/agent-live-run-routes.test.ts | 82 ++++++- .../agent-permissions-routes.test.ts | 4 + server/src/__tests__/assets.test.ts | 87 ++++++++ ...ompanies-route-cross-company-authz.test.ts | 2 + .../company-import-cloud-floor.test.ts | 2 +- .../company-portability-routes.test.ts | 176 ++++++++++++++- .../src/__tests__/company-portability.test.ts | 78 +++++++ server/src/__tests__/feedback-service.test.ts | 10 +- .../http-adapter-remote-fetch.test.ts | 91 ++++++++ .../src/__tests__/http-log-redaction.test.ts | 38 ++++ .../low-trust-red-team-routes.test.ts | 25 +++ server/src/__tests__/redact-sensitive.test.ts | 14 +- server/src/__tests__/secrets-routes.test.ts | 40 ++++ .../__tests__/workspace-command-authz.test.ts | 70 ++++++ .../workspace-runtime-routes-authz.test.ts | 99 +++++++++ server/src/adapters/http/execute.test.ts | 20 +- server/src/adapters/http/execute.ts | 3 +- server/src/adapters/http/remote-fetch.ts | 77 +++++++ server/src/adapters/http/test.ts | 3 +- server/src/middleware/redact-sensitive.ts | 5 + server/src/routes/agents.ts | 81 ++++++- server/src/routes/assets.ts | 13 +- server/src/routes/companies.ts | 30 ++- server/src/routes/projects.ts | 12 ++ server/src/routes/secrets.ts | 11 + server/src/routes/workspace-command-authz.ts | 52 ++++- server/src/services/agent-instructions.ts | 13 ++ .../company-portability-agent-selection.ts | 67 ++++++ server/src/services/company-portability.ts | 80 ++----- server/src/services/feedback.ts | 31 ++- server/src/services/plugin-managed-agents.ts | 5 +- 36 files changed, 1435 insertions(+), 122 deletions(-) create mode 100644 server/src/__tests__/http-adapter-remote-fetch.test.ts create mode 100644 server/src/__tests__/workspace-command-authz.test.ts create mode 100644 server/src/adapters/http/remote-fetch.ts create mode 100644 server/src/services/company-portability-agent-selection.ts diff --git a/.env.example b/.env.example index d90358bd85..e920780009 100644 --- a/.env.example +++ b/.env.example @@ -18,5 +18,9 @@ PAPERCLIP_TOOL_ACTION_SIGNING_SECRET=paperclip-dev-tool-action-signing-secret-ch # PAPERCLIP_WORKSPACE_GIT_SCAN_TIMEOUT_MS=8000 # PAPERCLIP_WORKSPACE_GIT_SCAN_CACHE_TTL_MS=10000 +# HTTP adapters may call public HTTP(S) origins by default. Opt trusted private +# origins in explicitly; entries are exact origins (scheme, host, and port). +# PAPERCLIP_HTTP_ADAPTER_PRIVATE_ENDPOINT_ALLOWLIST=http://hooks.internal.example:8080 + # Discord webhook for daily merge digest (scripts/discord-daily-digest.sh) # DISCORD_WEBHOOK_URL=https://discord.com/api/webhooks/... diff --git a/doc/DEPLOYMENT-MODES.md b/doc/DEPLOYMENT-MODES.md index 49f6e5f0d5..04a9897973 100644 --- a/doc/DEPLOYMENT-MODES.md +++ b/doc/DEPLOYMENT-MODES.md @@ -163,6 +163,12 @@ only to real browser session actors in `authenticated/private`; unauthenticated requests, agent keys, board API keys, and local implicit board actors are rejected. +This is intentionally a first-claim bootstrap contract: before an instance +admin exists, the first authenticated browser session that completes the claim +wins. Operators must keep a `bootstrap_pending` private deployment on a trusted +network and complete setup before admitting untrusted users. This behavior is +not an account-recovery or public-deployment mechanism. + The CLI fallback remains supported in all authenticated setup states: ```sh diff --git a/doc/DEVELOPING.md b/doc/DEVELOPING.md index 3baff160e0..f6b3937f9c 100644 --- a/doc/DEVELOPING.md +++ b/doc/DEVELOPING.md @@ -996,6 +996,24 @@ broker hostname is resolved once and the request is pinned to the approved address; IPv4 and IPv6 link-local destinations remain denied even when their host is allowlisted. +## HTTP Adapter Private Endpoints + +HTTP adapters can call public HTTP(S) endpoints by default. Requests use the +same DNS-pinning guard as remote connections, do not follow redirects, and +reject loopback, RFC1918/private, and link-local or cloud-metadata destinations. + +Server owners can opt a trusted private service in with a comma-separated list +of exact origins: + +```sh +PAPERCLIP_HTTP_ADAPTER_PRIVATE_ENDPOINT_ALLOWLIST=http://hooks.internal.example:8080,https://10.0.0.42 +``` + +Each entry must contain only a scheme, hostname, and optional port. Paths, +credentials, query strings, fragments, and wildcards are ignored. Matching is +by exact normalized origin, so allowing one port does not allow another. +Link-local destinations remain denied even when explicitly listed. + ## Company Deletion Toggle Company deletion is intended as a dev/debug capability and can be disabled at runtime: diff --git a/doc/SPEC-implementation.md b/doc/SPEC-implementation.md index 3387102b93..542dad3b99 100644 --- a/doc/SPEC-implementation.md +++ b/doc/SPEC-implementation.md @@ -414,6 +414,7 @@ Operational policy: - Default upload allowlist includes common images, PDF, plain text/markdown/JSON/CSV/HTML, ZIP, and video artifacts (`video/mp4`, `video/webm`, `video/quicktime`). - Attachment reads are company-scoped and expose stable path metadata: `contentPath`/`openPath` for inline-safe viewing and `downloadPath` for forced download. - Inline-safe responses use `Content-Disposition: inline`; unsafe types and explicit download requests use `attachment`. + - Script-capable content such as HTML is always served as an attachment with `X-Content-Type-Options: nosniff` and a sandboxed, deny-by-default CSP; it is never rendered inline on the Paperclip origin. - Video attachments are inline-safe and support single `Range: bytes=start-end` requests with `206`, `Content-Range`, and `Accept-Ranges: bytes` for browser playback/seeking. - Attachment-backed artifact work products use `type: "artifact"`, `provider: "paperclip"`, and metadata with `attachmentId`, `contentType`, `byteSize`, `contentPath`, `openPath`, `downloadPath`, and optional `originalFilename`. - Workspace-only file references use work product `metadata.resourceRef` with `kind: "workspace_file"`, `issueId`, `workspaceKind` (`execution_workspace` or `project_workspace`), `workspaceId`, `relativePath`, optional `line`/`column`, and `displayPath`. These references point at files in a workspace; they do not replace attachment-backed artifacts for deliverables that must be inspectable without workspace access. @@ -1353,6 +1354,10 @@ Required UX behaviors: - CSRF protection for board session endpoints - rate limit auth and key-management endpoints - strict company boundary checks on every entity fetch/mutation +- restricted `skill_test` and `task_bridge` keys cannot enumerate company-wide run telemetry, workspace-operation logs, or the company secret catalog +- HTTP adapters use DNS-pinned outbound requests, reject redirects and link-local/metadata targets, and require an exact server-owner origin allowlist for private destinations +- external instruction bundle roots and exports that read them require instance-admin access; managed company-scoped bundles remain available through normal company authorization +- agent-authenticated callers cannot persist host-executed workspace commands, and restricted keys cannot invoke preconfigured workspace runtime controls ## 17. Testing Strategy diff --git a/server/src/__tests__/agent-instructions-routes.test.ts b/server/src/__tests__/agent-instructions-routes.test.ts index 5fca425834..0a110b7802 100644 --- a/server/src/__tests__/agent-instructions-routes.test.ts +++ b/server/src/__tests__/agent-instructions-routes.test.ts @@ -3,6 +3,7 @@ import request from "supertest"; import { beforeEach, describe, expect, it, vi } from "vitest"; const mockAgentService = vi.hoisted(() => ({ + create: vi.fn(), getById: vi.fn(), update: vi.fn(), resolveByReference: vi.fn(), @@ -69,6 +70,7 @@ vi.mock("../services/environments.js", () => ({ vi.mock("../adapters/index.js", () => ({ findServerAdapter: mockFindServerAdapter, + findActiveServerAdapter: mockFindServerAdapter, listAdapterModels: vi.fn(), })); @@ -100,6 +102,7 @@ function registerModuleMocks() { vi.doMock("../adapters/index.js", () => ({ findServerAdapter: mockFindServerAdapter, + findActiveServerAdapter: mockFindServerAdapter, listAdapterModels: vi.fn(), })); } @@ -114,7 +117,7 @@ function boardActor() { }; } -async function createApp(actor: Record = boardActor()) { +async function createApp(actor: Record = boardActor(), db: Record = {}) { const [{ agentRoutes }, { errorHandler }] = await Promise.all([ vi.importActual("../routes/agents.js"), vi.importActual("../middleware/index.js"), @@ -125,7 +128,7 @@ async function createApp(actor: Record = boardActor()) { (req as any).actor = actor; next(); }); - app.use("/api", agentRoutes({} as any)); + app.use("/api", agentRoutes(db as any)); app.use(errorHandler); return app; } @@ -285,6 +288,202 @@ describe("agent instructions bundle routes", () => { expect(mockAgentInstructionsService.getBundle).toHaveBeenCalled(); }); + it("requires instance-admin access for every external instruction entry point", async () => { + mockAgentService.getById.mockResolvedValue({ + ...makeAgent(), + adapterConfig: { + instructionsBundleMode: "external", + instructionsRootPath: "/srv/paperclip/external-agent", + instructionsEntryFile: "AGENTS.md", + }, + }); + const app = await createApp({ + type: "board", + userId: "company-admin", + companyIds: ["company-1"], + memberships: [{ companyId: "company-1", status: "active", membershipRole: "admin" }], + source: "session", + isInstanceAdmin: false, + }); + const requests = [ + () => request(app).get("/api/agents/11111111-1111-4111-8111-111111111111/instructions-bundle"), + () => request(app) + .patch("/api/agents/11111111-1111-4111-8111-111111111111/instructions-bundle") + .send({ entryFile: "AGENTS.md" }), + () => request(app) + .get("/api/agents/11111111-1111-4111-8111-111111111111/instructions-bundle/file") + .query({ path: "AGENTS.md" }), + () => request(app) + .put("/api/agents/11111111-1111-4111-8111-111111111111/instructions-bundle/file") + .send({ path: "AGENTS.md", content: "# changed" }), + () => request(app) + .delete("/api/agents/11111111-1111-4111-8111-111111111111/instructions-bundle/file") + .query({ path: "AGENTS.md" }), + ]; + + for (const perform of requests) { + const res = await perform(); + expect(res.status, JSON.stringify(res.body)).toBe(403); + expect(res.body.error).toContain("Instance admin"); + } + expect(mockAgentInstructionsService.getBundle).not.toHaveBeenCalled(); + expect(mockAgentInstructionsService.readFile).not.toHaveBeenCalled(); + expect(mockAgentInstructionsService.updateBundle).not.toHaveBeenCalled(); + expect(mockAgentInstructionsService.writeFile).not.toHaveBeenCalled(); + expect(mockAgentInstructionsService.deleteFile).not.toHaveBeenCalled(); + }); + + it("treats a host root mislabeled as managed as external", async () => { + mockAgentService.getById.mockResolvedValue({ + ...makeAgent(), + adapterConfig: { + instructionsBundleMode: "managed", + instructionsRootPath: "/private/host/instructions", + instructionsEntryFile: "AGENTS.md", + }, + }); + const app = await createApp({ + type: "board", + userId: "company-admin", + companyIds: ["company-1"], + memberships: [{ companyId: "company-1", status: "active", membershipRole: "admin" }], + source: "session", + isInstanceAdmin: false, + }); + + const res = await request(app) + .get("/api/agents/11111111-1111-4111-8111-111111111111/instructions-bundle"); + + expect(res.status).toBe(403); + expect(mockAgentInstructionsService.getBundle).not.toHaveBeenCalled(); + }); + + it("allows an instance admin to read an external instruction bundle", async () => { + mockAgentService.getById.mockResolvedValue({ + ...makeAgent(), + adapterConfig: { + instructionsBundleMode: "external", + instructionsRootPath: "/srv/paperclip/external-agent", + instructionsEntryFile: "AGENTS.md", + }, + }); + + const res = await requestApp( + await createApp({ + type: "board", + userId: "instance-admin", + companyIds: ["company-1"], + memberships: [{ companyId: "company-1", status: "active", membershipRole: "admin" }], + source: "session", + isInstanceAdmin: true, + }), + (baseUrl) => request(baseUrl) + .get("/api/agents/11111111-1111-4111-8111-111111111111/instructions-bundle"), + ); + + expect(res.status, JSON.stringify(res.body)).toBe(200); + expect(mockAgentInstructionsService.getBundle).toHaveBeenCalled(); + }); + + it("rejects a company admin that requests a new external instruction root", async () => { + mockSyncInstructionsBundleConfigFromFilePath.mockImplementation((_agent, config) => ({ + ...config, + instructionsBundleMode: "external", + instructionsRootPath: "/srv/paperclip/external-agent", + instructionsEntryFile: "AGENTS.md", + })); + const app = await createApp({ + type: "board", + userId: "company-admin", + companyIds: ["company-1"], + memberships: [{ companyId: "company-1", status: "active", membershipRole: "admin" }], + source: "session", + isInstanceAdmin: false, + }); + + const compatibilityRes = await request(app) + .patch("/api/agents/11111111-1111-4111-8111-111111111111/instructions-path") + .send({ path: "/srv/paperclip/external-agent/AGENTS.md" }); + expect(compatibilityRes.status, JSON.stringify(compatibilityRes.body)).toBe(403); + + const bundleRes = await request(app) + .patch("/api/agents/11111111-1111-4111-8111-111111111111/instructions-bundle") + .send({ mode: "external", rootPath: "/srv/paperclip/external-agent" }); + expect(bundleRes.status, JSON.stringify(bundleRes.body)).toBe(403); + expect(mockAgentService.update).not.toHaveBeenCalled(); + expect(mockAgentInstructionsService.updateBundle).not.toHaveBeenCalled(); + }); + + it("rejects external instruction roots through the generic agent patch", async () => { + mockSyncInstructionsBundleConfigFromFilePath.mockImplementation((_agent, config) => config); + const app = await createApp({ + type: "board", + userId: "company-admin", + companyIds: ["company-1"], + memberships: [{ companyId: "company-1", status: "active", membershipRole: "admin" }], + source: "session", + isInstanceAdmin: false, + }); + + const res = await request(app) + .patch("/api/agents/11111111-1111-4111-8111-111111111111") + .send({ + adapterConfig: { + instructionsBundleMode: "external", + instructionsRootPath: "/srv/paperclip/external-agent", + instructionsEntryFile: "AGENTS.md", + }, + }); + + expect(res.status, JSON.stringify(res.body)).toBe(403); + expect(res.body.error).toContain("Instance admin"); + expect(mockAgentService.update).not.toHaveBeenCalled(); + }); + + it("rejects external instruction roots during both hire and direct creation", async () => { + const actor = { + type: "board", + userId: "company-admin", + companyIds: ["company-1"], + memberships: [{ companyId: "company-1", status: "active", membershipRole: "admin" }], + source: "session", + isInstanceAdmin: false, + }; + const db = { + select: vi.fn(() => ({ + from: vi.fn(() => ({ + where: vi.fn(async () => [{ + id: "company-1", + requireBoardApprovalForNewAgents: false, + }]), + })), + })), + }; + const app = await createApp(actor, db); + const body = { + name: "External agent", + adapterType: "codex_local", + adapterConfig: { + instructionsBundleMode: "external", + instructionsRootPath: "/srv/paperclip/external-agent", + instructionsEntryFile: "AGENTS.md", + }, + }; + + const hireRes = await request(app) + .post("/api/companies/company-1/agent-hires") + .send(body); + expect(hireRes.status, JSON.stringify(hireRes.body)).toBe(403); + expect(hireRes.body.error).toContain("Instance admin"); + + const createRes = await request(app) + .post("/api/companies/company-1/agents") + .send(body); + expect(createRes.status, JSON.stringify(createRes.body)).toBe(403); + expect(createRes.body.error).toContain("Instance admin"); + expect(mockAgentService.create).not.toHaveBeenCalled(); + }); + it("denies non-privileged agents from reading peer instructions bundles", async () => { mockAgentService.getById.mockImplementation(async (id: string) => { if (id === "agent-reader") { diff --git a/server/src/__tests__/agent-live-run-routes.test.ts b/server/src/__tests__/agent-live-run-routes.test.ts index 7178938e0a..7db8be7be4 100644 --- a/server/src/__tests__/agent-live-run-routes.test.ts +++ b/server/src/__tests__/agent-live-run-routes.test.ts @@ -50,6 +50,16 @@ const mockWorkspaceDiffReprojection = vi.hoisted(() => ({ })); const mockLogActivity = vi.hoisted(() => vi.fn()); const mockQueueRuntimeRequestResolution = vi.hoisted(() => vi.fn()); +const mockAccessService = vi.hoisted(() => ({ + canUser: vi.fn(), + decide: vi.fn(), + hasPermission: vi.fn(), +})); +const mockWorkspaceOperationService = vi.hoisted(() => ({ + getById: vi.fn(), + listForRun: vi.fn(), + readLog: vi.fn(), +})); const routeAgentId = "11111111-1111-4111-8111-111111111111"; @@ -100,16 +110,7 @@ function registerModuleMocks() { vi.doMock("../services/index.js", () => ({ agentService: () => mockAgentService, agentInstructionsService: () => ({}), - accessService: () => ({ - canUser: vi.fn(async () => true), - decide: vi.fn(async (input: { action?: string }) => ({ - allowed: true, - action: input.action, - reason: "allow_explicit_grant", - explanation: "Allowed by test grant.", - })), - hasPermission: vi.fn(async () => true), - }), + accessService: () => mockAccessService, approvalService: () => ({}), builtInAgentService: () => ({ ensureCompanyDefaultAgentGrants: vi.fn() }), companySkillService: () => ({ listRuntimeSkillEntries: vi.fn() }), @@ -120,7 +121,7 @@ function registerModuleMocks() { logActivity: mockLogActivity, secretService: () => ({}), syncInstructionsBundleConfigFromFilePath: vi.fn((_agent, config) => config), - workspaceOperationService: () => ({}), + workspaceOperationService: () => mockWorkspaceOperationService, })); vi.doMock("../adapters/index.js", () => ({ @@ -236,6 +237,14 @@ describe("agent live run routes", () => { vi.doUnmock("../middleware/index.js"); registerModuleMocks(); vi.clearAllMocks(); + mockAccessService.canUser.mockResolvedValue(true); + mockAccessService.decide.mockImplementation(async (input: { action?: string }) => ({ + allowed: true, + action: input.action, + reason: "allow_explicit_grant", + explanation: "Allowed by test grant.", + })); + mockAccessService.hasPermission.mockResolvedValue(true); mockIssueService.getByIdentifier.mockResolvedValue({ id: "issue-1", companyId: "company-1", @@ -313,6 +322,11 @@ describe("agent live run routes", () => { agentId: "agent-1", status: "succeeded", }); + mockWorkspaceOperationService.getById.mockResolvedValue({ + id: "operation-1", + companyId: "company-1", + runId: "run-1", + }); mockQueueRuntimeRequestResolution.mockReturnValue({ commandId: "command-resolution-1", }); @@ -465,6 +479,52 @@ describe("agent live run routes", () => { }); }); + it.each(["skill_test", "task_bridge"])( + "denies %s keys from company-wide run and workspace logs", + async (kind) => { + mockAccessService.decide.mockImplementation(async (input: { action?: string }) => ({ + allowed: input.action !== "company_scope:read", + action: input.action, + reason: input.action === "company_scope:read" ? "deny_key_scope" : "allow_explicit_grant", + explanation: input.action === "company_scope:read" + ? "Restricted keys cannot read company-wide run telemetry." + : "Allowed by test grant.", + })); + const actor = { + type: "agent", + agentId: routeAgentId, + companyId: "company-1", + source: "agent_key", + keyScope: kind === "skill_test" + ? { kind, issueId: "issue-1" } + : { kind, parentIssueId: "issue-1" }, + }; + const app = await createApp({}, actor); + const paths = [ + "/api/companies/company-1/heartbeat-runs", + "/api/companies/company-1/live-runs", + "/api/heartbeat-runs/run-1", + "/api/heartbeat-runs/run-1/events", + "/api/heartbeat-runs/run-1/log", + "/api/heartbeat-runs/run-1/workspace-operations", + "/api/workspace-operations/operation-1/log", + ]; + + for (const path of paths) { + const res = await requestApp(app, (baseUrl) => request(baseUrl).get(path)); + expect(res.status, `${path}: ${JSON.stringify(res.body)}`).toBe(403); + expect(res.body.error).toContain("Run telemetry"); + } + + expect(mockHeartbeatService.readLog).not.toHaveBeenCalled(); + expect(mockWorkspaceOperationService.readLog).not.toHaveBeenCalled(); + expect(mockAccessService.decide).toHaveBeenCalledWith(expect.objectContaining({ + action: "company_scope:read", + resource: { type: "company", companyId: "company-1" }, + })); + }, + ); + it("caps company live run polling by default", async () => { const rows = Array.from({ length: 75 }, (_, index) => ({ id: `run-${index}`, diff --git a/server/src/__tests__/agent-permissions-routes.test.ts b/server/src/__tests__/agent-permissions-routes.test.ts index cc6fe817e4..06aee69cf8 100644 --- a/server/src/__tests__/agent-permissions-routes.test.ts +++ b/server/src/__tests__/agent-permissions-routes.test.ts @@ -183,6 +183,10 @@ function registerModuleMocks() { vi.doMock("../services/agent-instructions.js", () => ({ agentInstructionsService: () => mockAgentInstructionsService, + agentInstructionsBundleMode: (agent: { adapterConfig?: unknown }) => { + const config = agent.adapterConfig as Record | undefined; + return config?.instructionsBundleMode === "external" ? "external" : "managed"; + }, syncInstructionsBundleConfigFromFilePath: mockSyncInstructionsBundleConfigFromFilePath, })); diff --git a/server/src/__tests__/assets.test.ts b/server/src/__tests__/assets.test.ts index 61a5ac042d..9c8308fb92 100644 --- a/server/src/__tests__/assets.test.ts +++ b/server/src/__tests__/assets.test.ts @@ -1,5 +1,6 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; import express from "express"; +import { Readable } from "node:stream"; import request from "supertest"; import { MAX_ATTACHMENT_BYTES } from "../attachment-types.js"; import type { StorageService } from "../storage/types.js"; @@ -407,3 +408,89 @@ describe("POST /api/companies/:companyId/logo", () => { expect(createAssetMock).not.toHaveBeenCalled(); }); }); + +describe("GET /api/assets/:assetId/content", () => { + beforeEach(() => { + vi.resetModules(); + vi.doUnmock("../services/index.js"); + vi.doUnmock("../routes/assets.js"); + vi.doUnmock("../routes/authz.js"); + vi.doUnmock("../middleware/index.js"); + registerModuleMocks(); + vi.clearAllMocks(); + getAssetByIdMock.mockReset(); + }); + + it("downloads script-capable HTML with nosniff and a sandbox CSP", async () => { + const html = Buffer.from(""); + const storage = createStorageService("text/html"); + getAssetByIdMock.mockResolvedValue({ + ...createAsset(), + contentType: "text/html", + byteSize: html.byteLength, + originalFilename: "proof.html", + }); + vi.mocked(storage.getObject).mockResolvedValue({ + stream: Readable.from(html), + contentType: "text/html", + contentLength: html.byteLength, + }); + + const res = await requestApp(await createApp(storage), (baseUrl) => + request(baseUrl).get("/api/assets/asset-1/content"), + ); + + expect(res.status).toBe(200); + expect(res.headers["content-disposition"]).toBe('attachment; filename="proof.html"'); + expect(res.headers["x-content-type-options"]).toBe("nosniff"); + expect(res.headers["content-security-policy"]).toBe("sandbox; default-src 'none'"); + }); + + it("downloads SVG instead of rendering it on the application origin", async () => { + const svg = Buffer.from(""); + const storage = createStorageService("image/svg+xml; charset=utf-8"); + getAssetByIdMock.mockResolvedValue({ + ...createAsset(), + contentType: "image/svg+xml; charset=utf-8", + byteSize: svg.byteLength, + originalFilename: "logo.svg", + }); + vi.mocked(storage.getObject).mockResolvedValue({ + stream: Readable.from(svg), + contentType: "image/svg+xml; charset=utf-8", + contentLength: svg.byteLength, + }); + + const res = await requestApp(await createApp(storage), (baseUrl) => + request(baseUrl).get("/api/assets/asset-1/content"), + ); + + expect(res.status).toBe(200); + expect(res.headers["content-disposition"]).toBe('attachment; filename="logo.svg"'); + expect(res.headers["x-content-type-options"]).toBe("nosniff"); + expect(res.headers["content-security-policy"]).toBe("sandbox; default-src 'none'"); + }); + + it("keeps curated image types inline", async () => { + const image = Buffer.from("png-bytes"); + const storage = createStorageService("image/png"); + getAssetByIdMock.mockResolvedValue({ + ...createAsset(), + byteSize: image.byteLength, + }); + vi.mocked(storage.getObject).mockResolvedValue({ + stream: Readable.from(image), + contentType: "image/png", + contentLength: image.byteLength, + }); + + const res = await requestApp(await createApp(storage), (baseUrl) => + request(baseUrl).get("/api/assets/asset-1/content"), + ); + + expect(res.status).toBe(200); + expect(res.headers["content-disposition"]).toBe('inline; filename="logo.png"'); + expect(res.headers["x-content-type-options"]).toBe("nosniff"); + expect(res.headers).not.toHaveProperty("content-security-policy"); + }); +}); diff --git a/server/src/__tests__/companies-route-cross-company-authz.test.ts b/server/src/__tests__/companies-route-cross-company-authz.test.ts index 520095fe74..4a92461f9c 100644 --- a/server/src/__tests__/companies-route-cross-company-authz.test.ts +++ b/server/src/__tests__/companies-route-cross-company-authz.test.ts @@ -18,6 +18,7 @@ const mockCompanyService = vi.hoisted(() => ({ const mockAgentService = vi.hoisted(() => ({ getById: vi.fn(), + list: vi.fn(), })); const mockAccessService = vi.hoisted(() => ({ @@ -169,6 +170,7 @@ function resetMockDefaults() { if (id === ceoAgentId) return { id, companyId: companyAId, role: "ceo" }; return null; }); + mockAgentService.list.mockResolvedValue([]); mockCompanyPortabilityService.exportBundle.mockResolvedValue(exportResult()); mockCompanyPortabilityService.previewExport.mockResolvedValue(exportPreviewResult()); mockCompanyPortabilityService.previewImport.mockResolvedValue({ ok: true }); diff --git a/server/src/__tests__/company-import-cloud-floor.test.ts b/server/src/__tests__/company-import-cloud-floor.test.ts index 71482ac227..cc17301bd5 100644 --- a/server/src/__tests__/company-import-cloud-floor.test.ts +++ b/server/src/__tests__/company-import-cloud-floor.test.ts @@ -23,7 +23,7 @@ const mockLogActivity = vi.hoisted(() => vi.fn()); vi.mock("../services/index.js", () => ({ accessService: () => ({}), - agentService: () => ({}), + agentService: () => ({ list: vi.fn().mockResolvedValue([]) }), budgetService: () => ({}), companyArtifactsService: () => ({}), companyPortabilityService: () => mockPortabilityService, diff --git a/server/src/__tests__/company-portability-routes.test.ts b/server/src/__tests__/company-portability-routes.test.ts index 94c57559ee..9872cd2f6b 100644 --- a/server/src/__tests__/company-portability-routes.test.ts +++ b/server/src/__tests__/company-portability-routes.test.ts @@ -14,6 +14,7 @@ const mockCompanyService = vi.hoisted(() => ({ const mockAgentService = vi.hoisted(() => ({ getById: vi.fn(), + list: vi.fn(), })); const mockAccessService = vi.hoisted(() => ({ @@ -341,6 +342,7 @@ describe.sequential("company portability routes", () => { companyId, role: id === ceoAgentId ? "ceo" : "engineer", })); + mockAgentService.list.mockResolvedValue([]); mockCompanyPortabilityService.exportBundle.mockResolvedValue(createExportResult()); mockCompanyPortabilityService.previewExport.mockResolvedValue({ rootPath: "paperclip", @@ -456,8 +458,18 @@ describe.sequential("company portability routes", () => { expect(res.body.rootPath).toBe("paperclip"); } expect(mockCompanyPortabilityService.exportBundle).toHaveBeenCalledTimes(2); - expect(mockCompanyPortabilityService.exportBundle).toHaveBeenNthCalledWith(1, companyId, exportRequest); - expect(mockCompanyPortabilityService.exportBundle).toHaveBeenNthCalledWith(2, companyId, exportRequest); + expect(mockCompanyPortabilityService.exportBundle).toHaveBeenNthCalledWith( + 1, + companyId, + exportRequest, + { allowExternalInstructions: false }, + ); + expect(mockCompanyPortabilityService.exportBundle).toHaveBeenNthCalledWith( + 2, + companyId, + exportRequest, + { allowExternalInstructions: false }, + ); }); it.sequential("allows board users to export through legacy and CEO-safe bundle routes", async () => { @@ -479,6 +491,166 @@ describe.sequential("company portability routes", () => { expect(mockCompanyPortabilityService.exportBundle).toHaveBeenCalledTimes(2); }); + it.sequential("requires instance-admin access when a company export includes external instructions", async () => { + mockAgentService.list.mockResolvedValue([{ + id: "external-agent", + companyId, + adapterConfig: { + instructionsBundleMode: "external", + instructionsRootPath: "/srv/paperclip/external-agent", + }, + }]); + const nonAdminActors = [ + { + type: "board", + userId: "company-admin", + companyIds: [companyId], + memberships: [{ companyId, status: "active", membershipRole: "admin" }], + source: "session", + isInstanceAdmin: false, + }, + { + type: "agent", + agentId: ceoAgentId, + companyId, + source: "agent_key", + runId: "run-1", + }, + ]; + + for (const actor of nonAdminActors) { + const app = await createApp(actor); + for (const path of [ + `/api/companies/${companyId}/export`, + `/api/companies/${companyId}/exports`, + `/api/companies/${companyId}/exports/preview`, + ]) { + const res = await request(app).post(path).send(exportRequest); + expect(res.status, `${path}: ${JSON.stringify(res.body)}`).toBe(403); + expect(res.body.error).toMatch(/Instance admin|Board access/); + } + } + + expect(mockCompanyPortabilityService.exportBundle).not.toHaveBeenCalled(); + expect(mockCompanyPortabilityService.previewExport).not.toHaveBeenCalled(); + }); + + it.sequential("allows an instance admin to export companies with external instructions", async () => { + mockAgentService.list.mockResolvedValue([{ + id: "external-agent", + companyId, + adapterConfig: { + instructionsBundleMode: "external", + instructionsRootPath: "/srv/paperclip/external-agent", + }, + }]); + const app = await createApp({ + type: "board", + userId: "instance-admin", + companyIds: [companyId], + memberships: [{ companyId, status: "active", membershipRole: "admin" }], + source: "session", + isInstanceAdmin: true, + }); + + const res = await request(app).post(`/api/companies/${companyId}/exports`).send(exportRequest); + + expect(res.status, JSON.stringify(res.body)).toBe(200); + expect(mockCompanyPortabilityService.exportBundle).toHaveBeenCalledWith( + companyId, + exportRequest, + { allowExternalInstructions: true }, + ); + }); + + it.sequential("uses the export selector resolver before checking external instructions", async () => { + mockAgentService.list.mockResolvedValue([ + { + id: "external-agent", + name: "Managed-Agent", + companyId, + status: "active", + metadata: null, + adapterConfig: { + instructionsBundleMode: "external", + instructionsRootPath: "/srv/paperclip/external-agent", + }, + }, + { + id: "managed-agent", + name: "Managed Agent", + companyId, + status: "active", + metadata: null, + adapterConfig: { instructionsBundleMode: "managed" }, + }, + ]); + mockCompanyPortabilityService.exportBundle.mockResolvedValue(createExportResult()); + const app = await createApp({ + type: "board", + userId: "company-admin", + companyIds: [companyId], + memberships: [{ companyId, status: "active", membershipRole: "admin" }], + source: "session", + isInstanceAdmin: false, + }); + const selectedAgentRequest = { + ...exportRequest, + agents: ["managed-agent"], + }; + + const res = await request(app) + .post(`/api/companies/${companyId}/exports`) + .send(selectedAgentRequest); + + expect(res.status, JSON.stringify(res.body)).toBe(200); + expect(mockAgentService.list).toHaveBeenCalledWith(companyId, { includeTerminated: true }); + expect(mockCompanyPortabilityService.exportBundle).toHaveBeenCalledWith( + companyId, + selectedAgentRequest, + { allowExternalInstructions: false }, + ); + }); + + it.sequential("keeps non-agent exports open when external instructions are excluded", async () => { + mockAgentService.list.mockResolvedValue([{ + id: "external-agent", + companyId, + adapterConfig: { + instructionsBundleMode: "external", + instructionsRootPath: "/srv/paperclip/external-agent", + }, + }]); + const app = await createApp({ + type: "board", + userId: "company-admin", + companyIds: [companyId], + memberships: [{ companyId, status: "active", membershipRole: "admin" }], + source: "session", + isInstanceAdmin: false, + }); + const companyOnlyRequest = { + include: { + company: true, + agents: false, + projects: false, + issues: false, + skills: false, + }, + }; + + const res = await request(app) + .post(`/api/companies/${companyId}/exports`) + .send(companyOnlyRequest); + + expect(res.status, JSON.stringify(res.body)).toBe(200); + expect(mockCompanyPortabilityService.exportBundle).toHaveBeenCalledWith( + companyId, + companyOnlyRequest, + { allowExternalInstructions: false }, + ); + }); + it.sequential("rejects CEO agents from exporting another company before services run", async () => { const app = await createApp({ type: "agent", diff --git a/server/src/__tests__/company-portability.test.ts b/server/src/__tests__/company-portability.test.ts index 3ea7f705ee..354e084ca5 100644 --- a/server/src/__tests__/company-portability.test.ts +++ b/server/src/__tests__/company-portability.test.ts @@ -164,6 +164,10 @@ vi.mock("../services/secrets.js", () => ({ vi.mock("../services/agent-instructions.js", () => ({ agentInstructionsService: () => agentInstructionsSvc, + agentInstructionsBundleMode: (agent: { adapterConfig?: unknown }) => { + const config = agent.adapterConfig as Record | undefined; + return config?.instructionsBundleMode === "external" ? "external" : "managed"; + }, })); vi.mock("../services/instance-settings.js", () => ({ @@ -610,6 +614,51 @@ describe("company portability", () => { expect(asTextFile(exported.files["agents/claudecoder/AGENTS.md"])).toContain(`- "${paperclipKey}"`); }); + it("refuses to read external instruction roots without an instance-admin export grant", async () => { + agentSvc.list.mockResolvedValue([{ + id: "external-agent", + companyId: "company-1", + name: "ExternalAgent", + status: "idle", + role: "engineer", + title: null, + icon: null, + reportsTo: null, + capabilities: null, + adapterType: "codex_local", + adapterConfig: { + instructionsBundleMode: "external", + instructionsRootPath: "/private/host/instructions", + }, + runtimeConfig: {}, + budgetMonthlyCents: 0, + permissions: { canCreateAgents: false }, + metadata: null, + }]); + + await expect(companyPortabilityService({} as any).exportBundle("company-1", { + include: { + company: true, + agents: true, + projects: false, + issues: false, + }, + })).rejects.toMatchObject({ status: 403 }); + expect(agentInstructionsSvc.exportFiles).not.toHaveBeenCalled(); + + await expect(companyPortabilityService({} as any).exportBundle("company-1", { + include: { + company: true, + agents: true, + projects: false, + issues: false, + }, + }, { allowExternalInstructions: true })).resolves.toMatchObject({ + manifest: { agents: [expect.objectContaining({ slug: "externalagent" })] }, + }); + expect(agentInstructionsSvc.exportFiles).toHaveBeenCalledTimes(1); + }); + it("exports agent permission grants through the Paperclip extension and manifest", async () => { const db = { select: vi.fn((selection: Record) => ({ @@ -700,6 +749,14 @@ describe("company portability", () => { adapterConfig: { env: { OPENAI_API_KEY: "sk-inline-secret-value", + OPENAI_KEY: { + type: "plain", + value: "sk-short-key-secret-value", + }, + MONKEY: { + type: "plain", + value: "banana", + }, NODE_ENV: { type: "plain", value: "development", @@ -726,6 +783,7 @@ describe("company portability", () => { const serialized = JSON.stringify(exported); expect(serialized).not.toContain("sk-inline-secret-value"); + expect(serialized).not.toContain("sk-short-key-secret-value"); expect(exported.manifest.envInputs).toContainEqual({ key: "OPENAI_API_KEY", description: "Optional default for OPENAI_API_KEY on agent inlinesecretagent", @@ -736,6 +794,26 @@ describe("company portability", () => { defaultValue: "", portability: "portable", }); + expect(exported.manifest.envInputs).toContainEqual({ + key: "OPENAI_KEY", + description: "Optional default for OPENAI_KEY on agent inlinesecretagent", + agentSlug: "inlinesecretagent", + projectSlug: null, + kind: "secret", + requirement: "optional", + defaultValue: "", + portability: "portable", + }); + expect(exported.manifest.envInputs).toContainEqual({ + key: "MONKEY", + description: "Optional default for MONKEY on agent inlinesecretagent", + agentSlug: "inlinesecretagent", + projectSlug: null, + kind: "plain", + requirement: "optional", + defaultValue: "banana", + portability: "portable", + }); expect(exported.manifest.envInputs).toContainEqual({ key: "NODE_ENV", description: "Optional default for NODE_ENV on agent inlinesecretagent", diff --git a/server/src/__tests__/feedback-service.test.ts b/server/src/__tests__/feedback-service.test.ts index bde8659b03..9770c94170 100644 --- a/server/src/__tests__/feedback-service.test.ts +++ b/server/src/__tests__/feedback-service.test.ts @@ -618,7 +618,7 @@ describeEmbeddedPostgres("feedbackService.saveIssueVote", () => { }); }); - it("builds a detailed sanitized shared bundle with issue and agent context", async () => { + it("builds a sanitized shared bundle without reading external instruction roots", async () => { const { companyId, issueId, targetCommentId, runId } = await seedIssueWithRichAgentComment(); await svc.saveIssueVote({ @@ -662,8 +662,12 @@ describeEmbeddedPostgres("feedbackService.saveIssueVote", () => { expect(sourceRun?.id).toBe(runId); expect(JSON.stringify(sourceRun)).toContain("gpt-5.4"); expect(skillItems?.[1]?.sourceLocator).toBe("https://github.com/octo/research/tree/main/skills/public-skill"); - expect(String(instructions?.entryBody)).toContain("[REDACTED]"); - expect(String(instructions?.entryBody)).not.toContain("secret-value"); + expect(instructions).toBeNull(); + expect(runtime?.configuredInstructionsBundleMode).toBe("external"); + expect(runtime?.configuredInstructionsFilePath).toBeNull(); + expect(runtime?.configuredInstructionsRootPath).toBeNull(); + expect(JSON.stringify(bundle)).not.toContain("secret-value"); + expect(JSON.stringify(bundle)).not.toContain("private-workspace"); }); it("keeps earlier local votes local when a later vote enables sharing", async () => { diff --git a/server/src/__tests__/http-adapter-remote-fetch.test.ts b/server/src/__tests__/http-adapter-remote-fetch.test.ts new file mode 100644 index 0000000000..7801e383dd --- /dev/null +++ b/server/src/__tests__/http-adapter-remote-fetch.test.ts @@ -0,0 +1,91 @@ +import { describe, expect, it, vi } from "vitest"; +import { + guardedHttpAdapterFetch, + httpAdapterPrivateEndpointAllowlist, +} from "../adapters/http/remote-fetch.js"; + +describe("HTTP adapter guarded fetch", () => { + it("parses only comma-separated exact HTTP(S) origins", () => { + const allowlist = httpAdapterPrivateEndpointAllowlist([ + "http://127.0.0.1:3100", + "HTTPS://INTERNAL.EXAMPLE:8443/", + "https://internal.example/path", + "https://user:pass@internal.example", + "file:///tmp/socket", + "not-a-url", + ].join(",")); + + expect([...allowlist]).toEqual([ + "http://127.0.0.1:3100", + "https://internal.example:8443", + ]); + }); + + it("allows public HTTP(S) endpoints by default and forces manual redirects", async () => { + const unpinnedFetch = vi.fn(async () => new Response(null, { + status: 302, + headers: { location: "http://127.0.0.1/admin" }, + })); + + const response = await guardedHttpAdapterFetch("https://93.184.216.34/hook", { + method: "POST", + }, { unpinnedFetch }); + + expect(response.status).toBe(302); + expect(unpinnedFetch).toHaveBeenCalledWith( + "https://93.184.216.34/hook", + expect.objectContaining({ method: "POST", redirect: "manual" }), + ); + }); + + it.each([ + "http://127.0.0.1:3100/hook", + "http://10.0.0.8/hook", + "http://172.16.0.8/hook", + "http://192.168.1.8/hook", + ])("blocks private endpoint %s unless its exact origin is allowlisted", async (url) => { + const unpinnedFetch = vi.fn(); + + await expect(guardedHttpAdapterFetch(url, {}, { unpinnedFetch })) + .rejects.toMatchObject({ code: "remote_http_private_endpoint" }); + expect(unpinnedFetch).not.toHaveBeenCalled(); + }); + + it("allows an exact private origin without allowing a sibling port", async () => { + const unpinnedFetch = vi.fn(async () => new Response("ok", { status: 200 })); + const privateEndpointAllowlist = new Set(["http://127.0.0.1:3100"]); + + const response = await guardedHttpAdapterFetch("http://127.0.0.1:3100/hook", {}, { + privateEndpointAllowlist, + unpinnedFetch, + }); + expect(response.status).toBe(200); + + await expect(guardedHttpAdapterFetch("http://127.0.0.1:3101/hook", {}, { + privateEndpointAllowlist, + unpinnedFetch, + })).rejects.toMatchObject({ code: "remote_http_private_endpoint" }); + }); + + it("rejects metadata link-local addresses even when their origin is allowlisted", async () => { + const unpinnedFetch = vi.fn(); + + await expect(guardedHttpAdapterFetch("http://169.254.169.254/latest/meta-data/", {}, { + privateEndpointAllowlist: new Set(["http://169.254.169.254"]), + unpinnedFetch, + })).rejects.toMatchObject({ code: "remote_http_private_endpoint" }); + expect(unpinnedFetch).not.toHaveBeenCalled(); + }); + + it("rejects private DNS results before opening a socket", async () => { + const socketFactory = vi.fn(() => { + throw new Error("must not dial"); + }); + + await expect(guardedHttpAdapterFetch("http://internal.example/hook", {}, { + lookup: async () => [{ address: "10.0.0.8", family: 4 }], + socketFactory, + })).rejects.toMatchObject({ code: "remote_http_private_endpoint" }); + expect(socketFactory).not.toHaveBeenCalled(); + }); +}); diff --git a/server/src/__tests__/http-log-redaction.test.ts b/server/src/__tests__/http-log-redaction.test.ts index e6ac40ea5e..9028ba3746 100644 --- a/server/src/__tests__/http-log-redaction.test.ts +++ b/server/src/__tests__/http-log-redaction.test.ts @@ -127,4 +127,42 @@ describe("HTTP logger redaction", () => { expect(log.req.query).toBeUndefined(); expect(log.reqQuery).toBeUndefined(); }); + + it("redacts failed secret payload values from structured request logs", async () => { + const chunks: string[] = []; + const stream = new Writable({ + write(chunk, _encoding, callback) { + chunks.push(chunk.toString()); + callback(); + }, + }); + const testLogger = pino({ redact: [...HTTP_LOG_REDACT_PATHS] }, stream); + const app = express(); + app.use(express.json()); + app.use(createHttpLogger(testLogger)); + app.post("/api/companies/:companyId/secrets", (_req, res) => { + res.status(422).json({ error: "validation failed" }); + }); + + const response = await request(app) + .post("/api/companies/company-1/secrets") + .send({ + name: "OpenAI", + value: "value-canary-4c845d", + metadata: { token: "token-canary-902ffc" }, + }); + + expect(response.status).toBe(422); + const output = chunks.join(""); + expect(output).not.toMatch(/value-canary-4c845d|token-canary-902ffc/); + + const log = JSON.parse(output.trim()) as { + reqBody: Record; + }; + expect(log.reqBody).toEqual({ + name: "OpenAI", + value: "[REDACTED]", + metadata: { token: "[REDACTED]" }, + }); + }); }); diff --git a/server/src/__tests__/low-trust-red-team-routes.test.ts b/server/src/__tests__/low-trust-red-team-routes.test.ts index 8288f55744..b4478c7d1d 100644 --- a/server/src/__tests__/low-trust-red-team-routes.test.ts +++ b/server/src/__tests__/low-trust-red-team-routes.test.ts @@ -1081,6 +1081,31 @@ describeEmbeddedPostgres("low-trust red-team HTTP route regression suite", () => expect(issueScopedLowTrustRes.body).not.toHaveProperty("runtimeConfig"); expectNoCanary(issueScopedLowTrustRes.body, fixture.canaries.agentConfig); + for (const restrictedActor of [ + skillTestActor(fixture), + { + ...standardActor, + source: "agent_key" as const, + keyScope: { + kind: "task_bridge" as const, + parentIssueId: fixture.issues.assignedReview.id, + }, + }, + ]) { + const restrictedRes = await request(createApp(db, restrictedActor)).get("/api/agents/me"); + expect(restrictedRes.status, JSON.stringify(restrictedRes.body)).toBe(200); + expect(restrictedRes.body).toMatchObject({ + id: fixture.agents.standard.id, + companyId: fixture.company.id, + keyScope: restrictedActor.keyScope, + }); + expect(restrictedRes.body).not.toHaveProperty("adapterConfig"); + expect(restrictedRes.body).not.toHaveProperty("runtimeConfig"); + expect(restrictedRes.body).not.toHaveProperty("permissions"); + expect(restrictedRes.body).not.toHaveProperty("access"); + expectNoCanary(restrictedRes.body, fixture.canaries.agentConfig); + } + await db.update(issues).set({ executionPolicy: null }).where(eq(issues.id, fixture.issues.assignedReview.id)); await db.update(projects).set({ diff --git a/server/src/__tests__/redact-sensitive.test.ts b/server/src/__tests__/redact-sensitive.test.ts index 78761a51f6..05034febb7 100644 --- a/server/src/__tests__/redact-sensitive.test.ts +++ b/server/src/__tests__/redact-sensitive.test.ts @@ -50,11 +50,19 @@ describe("redactSensitive", () => { expect(JSON.stringify(out)).not.toContain("\\u001b"); }); - it("does not redact a bare `token` field — pagination cursors and CSRF tokens are not credentials", () => { - const out = redactSensitive({ token: "next-page-cursor", limit: 20 }) as Record; + it("redacts bare value and token fields recursively", () => { + const out = redactSensitive({ + token: "secret-token", + nested: { value: "secret-value" }, + entries: [{ value: "array-secret" }], + limit: 20, + }) as Record; - expect(out.token).toBe("next-page-cursor"); + expect(out.token).toBe("[REDACTED]"); + expect((out.nested as Record).value).toBe("[REDACTED]"); + expect((out.entries as Array>)[0].value).toBe("[REDACTED]"); expect(out.limit).toBe(20); + expect(JSON.stringify(out)).not.toMatch(/secret-token|secret-value|array-secret/); }); it("strips secret-bearing query and fragment values from source URLs", () => { diff --git a/server/src/__tests__/secrets-routes.test.ts b/server/src/__tests__/secrets-routes.test.ts index 91cb611f7e..ffd4c3893f 100644 --- a/server/src/__tests__/secrets-routes.test.ts +++ b/server/src/__tests__/secrets-routes.test.ts @@ -42,12 +42,20 @@ const mockSecretService = vi.hoisted(() => ({ resolveSecretValueForAgentAccess: vi.fn(), })); const mockLogActivity = vi.hoisted(() => vi.fn()); +const mockAccessService = vi.hoisted(() => ({ + decide: vi.fn(), +})); vi.mock("../services/index.js", () => ({ + accessService: () => mockAccessService, secretService: () => mockSecretService, logActivity: mockLogActivity, })); +vi.mock("../services/access.js", () => ({ + accessService: () => mockAccessService, +})); + function createApp(actor: Record = { type: "board", userId: "user-1", @@ -72,6 +80,12 @@ describe("secret routes", () => { mock.mockReset(); } mockLogActivity.mockReset(); + mockAccessService.decide.mockReset(); + mockAccessService.decide.mockResolvedValue({ + allowed: true, + reason: "allow_standard_agent", + explanation: "Allowed by test policy", + }); }); it("returns an opaque secretRef in agent secret metadata without internal binding details", async () => { @@ -1078,6 +1092,32 @@ describe("secret routes", () => { expect(res.body[0]).toMatchObject({ id: expect.any(String), name: "MY_API_KEY", key: "my_api_key", status: "active" }); }); + it.each(["skill_test", "task_bridge"])( + "rejects %s keys from the company secret catalog", + async (kind) => { + mockAccessService.decide.mockResolvedValue({ + allowed: false, + reason: "deny_key_scope", + explanation: "Restricted keys cannot read the company secret catalog.", + }); + + const res = await request(createApp({ + type: "agent", + agentId: "agent-1", + companyId: "company-1", + source: "agent_key", + keyScope: { kind }, + })).get("/api/companies/company-1/secrets/catalog"); + + expect(res.status).toBe(403); + expect(mockSecretService.list).not.toHaveBeenCalled(); + expect(mockAccessService.decide).toHaveBeenCalledWith(expect.objectContaining({ + action: "secrets:read", + resource: { type: "company", companyId: "company-1" }, + })); + }, + ); + it("rejects unauthenticated requests", async () => { const res = await request(createApp({ type: "none" })) .get("/api/companies/company-1/secrets/catalog"); diff --git a/server/src/__tests__/workspace-command-authz.test.ts b/server/src/__tests__/workspace-command-authz.test.ts new file mode 100644 index 0000000000..d98c234183 --- /dev/null +++ b/server/src/__tests__/workspace-command-authz.test.ts @@ -0,0 +1,70 @@ +import { describe, expect, it } from "vitest"; +import { + collectExecutionWorkspaceCommandPaths, + collectIssueWorkspaceCommandPaths, + collectProjectExecutionWorkspaceCommandPaths, + collectProjectWorkspaceCommandPaths, +} from "../routes/workspace-command-authz.js"; + +describe("workspace host-command mutation detection", () => { + it.each([ + { + name: "project execution policy commands", + actual: () => collectProjectExecutionWorkspaceCommandPaths({ + workspaceRuntime: { commands: [{ name: "seed", command: "pnpm seed" }] }, + }), + expected: "executionWorkspacePolicy.workspaceRuntime.commands[0].command", + }, + { + name: "project execution policy services", + actual: () => collectProjectExecutionWorkspaceCommandPaths({ + workspaceRuntime: { services: [{ name: "web", command: "pnpm dev" }] }, + }), + expected: "executionWorkspacePolicy.workspaceRuntime.services[0].command", + }, + { + name: "project workspace jobs", + actual: () => collectProjectWorkspaceCommandPaths({ + runtimeConfig: { workspaceRuntime: { jobs: [{ name: "build", command: "pnpm build" }] } }, + }), + expected: "runtimeConfig.workspaceRuntime.jobs[0].command", + }, + { + name: "issue execution workspace services", + actual: () => collectIssueWorkspaceCommandPaths({ + executionWorkspaceSettings: { + workspaceRuntime: { services: [{ name: "web", command: "pnpm dev" }] }, + }, + }), + expected: "executionWorkspaceSettings.workspaceRuntime.services[0].command", + }, + { + name: "execution workspace config commands", + actual: () => collectExecutionWorkspaceCommandPaths({ + config: { workspaceRuntime: { commands: [{ name: "seed", command: "pnpm seed" }] } }, + }), + expected: "config.workspaceRuntime.commands[0].command", + }, + { + name: "execution workspace metadata jobs", + actual: () => collectExecutionWorkspaceCommandPaths({ + metadata: { + config: { workspaceRuntime: { jobs: [{ name: "build", command: "pnpm build" }] } }, + }, + }), + expected: "metadata.config.workspaceRuntime.jobs[0].command", + }, + ])("detects $name", ({ actual, expected }) => { + expect(actual()).toContain(expected); + }); + + it("ignores descriptive runtime entries without a command field", () => { + expect(collectProjectExecutionWorkspaceCommandPaths({ + workspaceRuntime: { + commands: [{ name: "seed" }], + services: [{ name: "web", port: 3100 }], + jobs: [null, "build"], + }, + })).toEqual([]); + }); +}); diff --git a/server/src/__tests__/workspace-runtime-routes-authz.test.ts b/server/src/__tests__/workspace-runtime-routes-authz.test.ts index 64bfce7f9c..cd7efe6ea4 100644 --- a/server/src/__tests__/workspace-runtime-routes-authz.test.ts +++ b/server/src/__tests__/workspace-runtime-routes-authz.test.ts @@ -343,6 +343,66 @@ describe.sequential("workspace runtime service route authorization", () => { expect(mockAssertCanManageProjectWorkspaceRuntimeServices).toHaveBeenCalled(); }, 15000); + it.each(["skill_test", "task_bridge"])( + "rejects %s keys at the central runtime-manage gate for project and execution workspaces", + async (kind) => { + mockAccessService.decide.mockImplementation(async (input: { action?: string }) => ({ + allowed: input.action !== "runtime:manage", + action: input.action, + reason: input.action === "runtime:manage" ? "deny_key_scope" : "allow_test", + explanation: input.action === "runtime:manage" + ? "Restricted keys cannot manage workspace runtimes." + : "Allowed by test mock.", + })); + mockProjectService.getById.mockResolvedValue(buildProject({ + id: projectId, + workspaces: [{ + id: workspaceId, + companyId: "company-1", + projectId, + runtimeConfig: { + workspaceRuntime: { services: [{ name: "web", command: "pnpm dev" }] }, + }, + }], + })); + mockExecutionWorkspaceService.getById.mockResolvedValue(buildExecutionWorkspace({ + id: executionWorkspaceId, + config: { + workspaceRuntime: { services: [{ name: "web", command: "pnpm dev" }] }, + }, + })); + const actor = { + type: "agent", + agentId: "agent-1", + companyId: "company-1", + source: "agent_key", + runId: "run-1", + keyScope: kind === "skill_test" + ? { kind, issueId: "issue-1" } + : { kind, parentIssueId: "issue-1" }, + }; + + const projectRes = await request(await createProjectApp(actor)) + .post(`/api/projects/${projectId}/workspaces/${workspaceId}/runtime-services/start`) + .send({}); + expect(projectRes.status, JSON.stringify(projectRes.body)).toBe(403); + expect(projectRes.body.error).toContain("authorization boundary"); + + const executionRes = await request(await createExecutionWorkspaceApp(actor)) + .post(`/api/execution-workspaces/${executionWorkspaceId}/runtime-services/start`) + .send({}); + expect(executionRes.status, JSON.stringify(executionRes.body)).toBe(403); + expect(executionRes.body.error).toContain("authorization boundary"); + + expect(mockAssertCanManageProjectWorkspaceRuntimeServices).not.toHaveBeenCalled(); + expect(mockAssertCanManageExecutionWorkspaceRuntimeServices).not.toHaveBeenCalled(); + expect(mockAccessService.decide).toHaveBeenCalledWith(expect.objectContaining({ + action: "runtime:manage", + resource: { type: "company", companyId: "company-1" }, + })); + }, + ); + it("blocks shared-project stop/restart requests from agents", async () => { mockProjectService.getById.mockResolvedValue(buildProject({ id: projectId, @@ -420,6 +480,45 @@ describe.sequential("workspace runtime service route authorization", () => { expect(mockProjectService.create).not.toHaveBeenCalled(); }); + it("rejects agent callers that persist workspace-runtime service commands", async () => { + mockProjectService.getById.mockResolvedValue(buildProject()); + const app = await createProjectApp({ + type: "agent", + agentId: "agent-1", + companyId: "company-1", + source: "agent_key", + runId: "run-1", + }); + + const projectRes = await request(app) + .post("/api/companies/company-1/projects") + .send({ + name: "Exploit", + executionWorkspacePolicy: { + enabled: true, + workspaceRuntime: { + services: [{ name: "web", command: "touch /tmp/paperclip-rce" }], + }, + }, + }); + expect(projectRes.status).toBe(403); + expect(projectRes.body.error).toContain("executionWorkspacePolicy.workspaceRuntime.services[0].command"); + + const workspaceRes = await request(app) + .patch(`/api/projects/${projectId}/workspaces/${workspaceId}`) + .send({ + runtimeConfig: { + workspaceRuntime: { + jobs: [{ name: "build", command: "touch /tmp/paperclip-rce" }], + }, + }, + }); + expect(workspaceRes.status).toBe(403); + expect(workspaceRes.body.error).toContain("runtimeConfig.workspaceRuntime.jobs[0].command"); + expect(mockProjectService.create).not.toHaveBeenCalled(); + expect(mockProjectService.updateWorkspace).not.toHaveBeenCalled(); + }); + it("rejects agent callers that update project workspace cleanup commands", async () => { mockProjectService.getById.mockResolvedValue(buildProject()); const app = await createProjectApp({ diff --git a/server/src/adapters/http/execute.test.ts b/server/src/adapters/http/execute.test.ts index 5dbbb12bd0..7d6cd79874 100644 --- a/server/src/adapters/http/execute.test.ts +++ b/server/src/adapters/http/execute.test.ts @@ -2,14 +2,20 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { CONNECTION_INTENT_AGENT_GUIDANCE } from "@paperclipai/shared"; import { execute } from "./execute.js"; +const guardedFetchMock = vi.hoisted(() => vi.fn()); + +vi.mock("./remote-fetch.js", () => ({ + guardedHttpAdapterFetch: guardedFetchMock, +})); + afterEach(() => { - vi.unstubAllGlobals(); + guardedFetchMock.mockReset(); }); describe("http adapter execute", () => { it("delivers the complete runtime connection descriptor and shared guidance", async () => { const onDispatch = vi.fn(); - const fetchMock = vi.fn(async (_url: string, init?: RequestInit) => { + guardedFetchMock.mockImplementation(async (_url: string, init?: RequestInit) => { expect(onDispatch).toHaveBeenCalledOnce(); const body = JSON.parse(String(init?.body)) as Record; expect(body.paperclipRuntimeTools).toEqual({ @@ -26,7 +32,6 @@ describe("http adapter execute", () => { }); return new Response(null, { status: 204 }); }); - vi.stubGlobal("fetch", fetchMock); await execute({ runId: "run-1", @@ -61,18 +66,17 @@ describe("http adapter execute", () => { onDispatch, }); - expect(fetchMock).toHaveBeenCalledOnce(); + expect(guardedFetchMock).toHaveBeenCalledOnce(); expect(onDispatch).toHaveBeenCalledOnce(); }); it("reports configured request timeout as timed_out", async () => { - vi.stubGlobal( - "fetch", - vi.fn((_url: string, init?: RequestInit) => new Promise((_resolve, reject) => { + guardedFetchMock.mockImplementation( + (_url: string, init?: RequestInit) => new Promise((_resolve, reject) => { init?.signal?.addEventListener("abort", () => { reject(new DOMException("Aborted", "AbortError")); }); - })), + }), ); const result = await execute({ diff --git a/server/src/adapters/http/execute.ts b/server/src/adapters/http/execute.ts index 6bfcc6183f..272cd61e9e 100644 --- a/server/src/adapters/http/execute.ts +++ b/server/src/adapters/http/execute.ts @@ -1,5 +1,6 @@ import type { AdapterExecutionContext, AdapterExecutionResult } from "../types.js"; import { asString, asNumber, parseObject } from "../utils.js"; +import { guardedHttpAdapterFetch } from "./remote-fetch.js"; export async function execute(ctx: AdapterExecutionContext): Promise { const { config, runId, agent, context } = ctx; @@ -26,7 +27,7 @@ export async function execute(ctx: AdapterExecutionContext): Promise { + return new Set( + raw + .split(",") + .map((entry) => normalizeAllowlistedOrigin(entry.trim())) + .filter((entry): entry is string => entry !== null), + ); +} + +type HttpAdapterFetchOptions = Omit< + GuardedRemoteHttpFetchOptions, + "allowPrivateNetwork" | "error" +> & { + privateEndpointAllowlist?: ReadonlySet; +}; + +/** + * Guard every HTTP-adapter request at the actual socket boundary. Public + * endpoints are allowed by default. An exact operator-configured origin can + * opt into private networking, while the shared guard continues to reject + * link-local metadata targets and pins DNS results to prevent rebinding. + */ +export async function guardedHttpAdapterFetch( + url: string | URL, + init: RequestInit, + options: HttpAdapterFetchOptions = {}, +): Promise { + const endpoint = parseRemoteHttpEndpoint(url.toString(), endpointError); + const allowlist = options.privateEndpointAllowlist ?? httpAdapterPrivateEndpointAllowlist(); + return guardedRemoteHttpFetch(endpoint, init, { + ...options, + allowPrivateNetwork: allowlist.has(endpoint.origin.toLowerCase()), + error: endpointError, + }); +} diff --git a/server/src/adapters/http/test.ts b/server/src/adapters/http/test.ts index a1a8fd3440..f9ef5fc078 100644 --- a/server/src/adapters/http/test.ts +++ b/server/src/adapters/http/test.ts @@ -4,6 +4,7 @@ import type { AdapterEnvironmentTestResult, } from "../types.js"; import { asString, parseObject } from "../utils.js"; +import { guardedHttpAdapterFetch } from "./remote-fetch.js"; function summarizeStatus(checks: AdapterEnvironmentCheck[]): AdapterEnvironmentTestResult["status"] { if (checks.some((check) => check.level === "error")) return "fail"; @@ -77,7 +78,7 @@ export async function testEnvironment( const controller = new AbortController(); const timeout = setTimeout(() => controller.abort(), 3000); try { - const response = await fetch(url, { + const response = await guardedHttpAdapterFetch(url, { method: "HEAD", signal: controller.signal, }); diff --git a/server/src/middleware/redact-sensitive.ts b/server/src/middleware/redact-sensitive.ts index 84ed453ac7..3af3d4ee9d 100644 --- a/server/src/middleware/redact-sensitive.ts +++ b/server/src/middleware/redact-sensitive.ts @@ -21,6 +21,11 @@ const SENSITIVE_KEYS = new Set([ "password_confirm", "confirmpassword", "confirm_password", + // Secret creation/update bodies use a generic `value` field. Failure logs + // must prefer losing that diagnostic value over persisting credential + // material. `token` is likewise ambiguous but frequently credential-bearing. + "value", + "token", "secret", "client_secret", "clientsecret", diff --git a/server/src/routes/agents.ts b/server/src/routes/agents.ts index 6e3873ddc7..5ee35842b9 100644 --- a/server/src/routes/agents.ts +++ b/server/src/routes/agents.ts @@ -47,6 +47,7 @@ import { } from "@paperclipai/adapter-utils/server-utils"; import { trackAgentCreated } from "@paperclipai/shared/telemetry"; import { validate } from "../middleware/validate.js"; +import { agentInstructionsBundleMode } from "../services/agent-instructions.js"; import { agentService, agentInstructionsService, @@ -1012,6 +1013,17 @@ export function agentRoutes( return false; } + async function assertRunTelemetryReadAllowed(req: Request, res: Response, companyId: string) { + const decision = await access.decide({ + actor: req.actor, + action: "company_scope:read", + resource: { type: "company", companyId }, + }); + if (decision.allowed) return true; + res.status(403).json({ error: "Run telemetry is outside this actor's authorization boundary" }); + return false; + } + async function filterAgentsForActor>( req: Request, rows: T[], @@ -2556,6 +2568,15 @@ export function agentRoutes( ); } + function assertExternalInstructionsAdmin( + req: Request, + agent: Parameters[0], + ) { + if (agentInstructionsBundleMode(agent) === "external") { + assertInstanceAdmin(req); + } + } + function adapterConfigTouchesInstructionsConfig(adapterConfig: Record) { return KNOWN_INSTRUCTIONS_BUNDLE_KEYS.some((key) => adapterConfig[key] !== undefined); } @@ -3629,16 +3650,10 @@ export function agentRoutes( res.status(404).json({ error: "Agent not found" }); return; } - const trustPreset = await resolveAgentSelfTrustPreset(req, agent); - if (trustPreset.kind === "denied") { - res.status(403).json({ error: trustPreset.detail }); - return; - } - if (trustPreset.kind === "low_trust_review") { - res.json(buildLowTrustSelfView(agent)); - return; - } - if (req.actor.keyScope?.kind === "task_bridge") { + if ( + req.actor.keyScope?.kind === "task_bridge" + || req.actor.keyScope?.kind === "skill_test" + ) { res.json({ id: agent.id, companyId: agent.companyId, @@ -3650,6 +3665,15 @@ export function agentRoutes( }); return; } + const trustPreset = await resolveAgentSelfTrustPreset(req, agent); + if (trustPreset.kind === "denied") { + res.status(403).json({ error: trustPreset.detail }); + return; + } + if (trustPreset.kind === "low_trust_review") { + res.json(buildLowTrustSelfView(agent)); + return; + } res.json(await buildAgentDetail(agent)); }); @@ -3817,6 +3841,11 @@ export function agentRoutes( await assertSelectableAdapterType(rollbackAdapterType); } const rollbackAdapterConfig = asRecord(rollbackConfig.adapterConfig) ?? {}; + assertExternalInstructionsAdmin(req, existing); + assertExternalInstructionsAdmin(req, { + ...existing, + adapterConfig: rollbackAdapterConfig, + }); if ( rollbackAdapterType !== existing.adapterType || rollbackAdapterType === "paperclip_runner" @@ -3948,6 +3977,12 @@ export function agentRoutes( rawHireAdapterConfig, ), ); + assertExternalInstructionsAdmin(req, { + id: hiredAgentId, + companyId, + name: hireInput.name, + adapterConfig: requestedAdapterConfig, + }); const desiredSkillAssignment = await resolveDesiredSkillAssignment( companyId, hireInput.adapterType, @@ -4167,6 +4202,12 @@ export function agentRoutes( rawCreateAdapterConfig, ), ); + assertExternalInstructionsAdmin(req, { + id: agentId, + companyId, + name: createInput.name, + adapterConfig: requestedAdapterConfig, + }); const desiredSkillAssignment = await resolveDesiredSkillAssignment( companyId, createInput.adapterType, @@ -4327,6 +4368,7 @@ export function agentRoutes( if (!existing) return; await assertCanManageInstructionsPath(req, existing); + assertExternalInstructionsAdmin(req, existing); const existingAdapterConfig = asRecord(existing.adapterConfig) ?? {}; const explicitKey = asNonEmptyString(req.body.adapterConfigKey); @@ -4347,6 +4389,7 @@ export function agentRoutes( } const syncedAdapterConfig = syncInstructionsBundleConfigFromFilePath(existing, nextAdapterConfig); + assertExternalInstructionsAdmin(req, { ...existing, adapterConfig: syncedAdapterConfig }); const normalizedAdapterConfig = await secretsSvc.normalizeAdapterConfigForPersistence( existing.companyId, syncedAdapterConfig, @@ -4402,6 +4445,7 @@ export function agentRoutes( const existing = await getAccessibleResource(req, res, svc.getById(id), "Agent not found"); if (!existing) return; await assertCanReadAgent(req, existing); + assertExternalInstructionsAdmin(req, existing); res.json(await instructions.getBundle(existing)); }); @@ -4410,6 +4454,8 @@ export function agentRoutes( const existing = await getAccessibleResource(req, res, svc.getById(id), "Agent not found"); if (!existing) return; await assertCanManageInstructionsPath(req, existing); + assertExternalInstructionsAdmin(req, existing); + if (req.body.mode === "external") assertInstanceAdmin(req); const actor = getActorInfo(req); const { bundle, adapterConfig } = await instructions.updateBundle(existing, req.body); @@ -4456,6 +4502,7 @@ export function agentRoutes( const existing = await getAccessibleResource(req, res, svc.getById(id), "Agent not found"); if (!existing) return; await assertCanReadAgent(req, existing); + assertExternalInstructionsAdmin(req, existing); const relativePath = typeof req.query.path === "string" ? req.query.path : ""; if (!relativePath.trim()) { @@ -4471,6 +4518,7 @@ export function agentRoutes( const existing = await getAccessibleResource(req, res, svc.getById(id), "Agent not found"); if (!existing) return; await assertCanManageInstructionsPath(req, existing); + assertExternalInstructionsAdmin(req, existing); const actor = getActorInfo(req); const result = await instructions.writeFile(existing, req.body.path, req.body.content, { @@ -4518,6 +4566,7 @@ export function agentRoutes( const existing = await getAccessibleResource(req, res, svc.getById(id), "Agent not found"); if (!existing) return; await assertCanManageInstructionsPath(req, existing); + assertExternalInstructionsAdmin(req, existing); const relativePath = typeof req.query.path === "string" ? req.query.path : ""; if (!relativePath.trim()) { @@ -4605,6 +4654,7 @@ export function agentRoutes( hasOwn(patchData, "adapterType") || hasOwn(patchData, "adapterConfig"); if (touchesAdapterConfiguration) { + assertExternalInstructionsAdmin(req, existing); const existingAdapterConfig = asRecord(existing.adapterConfig) ?? {}; const changingAdapterType = typeof patchData.adapterType === "string" && patchData.adapterType !== existing.adapterType; @@ -4678,6 +4728,10 @@ export function agentRoutes( adapterConfig: effectiveAdapterConfig, }); patchData.adapterConfig = syncInstructionsBundleConfigFromFilePath(existing, normalizedEffectiveAdapterConfig); + assertExternalInstructionsAdmin(req, { + ...existing, + adapterConfig: patchData.adapterConfig, + }); } if (requestedRuntimeConfig) patchData.runtimeConfig = requestedRuntimeConfig; if (touchesAdapterConfiguration || Object.prototype.hasOwnProperty.call(patchData, "defaultEnvironmentId")) { @@ -5755,6 +5809,7 @@ export function agentRoutes( router.get("/companies/:companyId/heartbeat-runs", async (req, res) => { const companyId = req.params.companyId as string; assertCompanyAccess(req, companyId); + if (!(await assertRunTelemetryReadAllowed(req, res, companyId))) return; const agentId = req.query.agentId as string | undefined; const limitParam = req.query.limit as string | undefined; const limit = limitParam ? Math.max(1, Math.min(1000, parseInt(limitParam, 10) || 200)) : undefined; @@ -5795,6 +5850,7 @@ export function agentRoutes( router.get("/companies/:companyId/live-runs", async (req, res) => { const companyId = req.params.companyId as string; assertCompanyAccess(req, companyId); + if (!(await assertRunTelemetryReadAllowed(req, res, companyId))) return; // `minCount` is a padding floor for callers that want a minimum number of // recent runs to render (e.g. dashboard cards). It must default to 0 so @@ -5882,6 +5938,7 @@ export function agentRoutes( const runId = req.params.runId as string; const run = await getAccessibleResource(req, res, heartbeat.getRun(runId), "Heartbeat run not found"); if (!run) return; + if (!(await assertRunTelemetryReadAllowed(req, res, run.companyId))) return; const retryExhaustedReason = await heartbeat.getRetryExhaustedReason(runId); const decoratedRun = heartbeat.decorateActiveRunStatus(run); res.json(await runRedactions.redactForRun( @@ -6355,6 +6412,7 @@ export function agentRoutes( const runId = req.params.runId as string; const run = await getAccessibleResource(req, res, heartbeat.getRun(runId), "Heartbeat run not found"); if (!run) return; + if (!(await assertRunTelemetryReadAllowed(req, res, run.companyId))) return; const afterSeq = Number(req.query.afterSeq ?? 0); const limit = Number(req.query.limit ?? 200); @@ -6373,6 +6431,7 @@ export function agentRoutes( const runId = req.params.runId as string; const run = await getAccessibleResource(req, res, heartbeat.getRunLogAccess(runId), "Heartbeat run not found"); if (!run) return; + if (!(await assertRunTelemetryReadAllowed(req, res, run.companyId))) return; const offset = Number(req.query.offset ?? 0); const limitBytes = readRunLogLimitBytes(req.query.limitBytes); @@ -6389,6 +6448,7 @@ export function agentRoutes( const runId = req.params.runId as string; const run = await getAccessibleResource(req, res, heartbeat.getRun(runId), "Heartbeat run not found"); if (!run) return; + if (!(await assertRunTelemetryReadAllowed(req, res, run.companyId))) return; const context = asRecord(run.contextSnapshot); const executionWorkspaceId = asNonEmptyString(context?.executionWorkspaceId); @@ -6400,6 +6460,7 @@ export function agentRoutes( const operationId = req.params.operationId as string; const operation = await getAccessibleResource(req, res, workspaceOperations.getById(operationId), "Workspace operation not found"); if (!operation) return; + if (!(await assertRunTelemetryReadAllowed(req, res, operation.companyId))) return; const offset = Number(req.query.offset ?? 0); const limitBytes = readRunLogLimitBytes(req.query.limitBytes); diff --git a/server/src/routes/assets.ts b/server/src/routes/assets.ts index 8419161a4d..3f9d4ee0c3 100644 --- a/server/src/routes/assets.ts +++ b/server/src/routes/assets.ts @@ -9,6 +9,7 @@ import { assetService, logActivity } from "../services/index.js"; import { formatAttachmentSize, isAllowedContentType, + isInlineAttachmentContentType, MAX_ATTACHMENT_BYTES, } from "../attachment-types.js"; import { assertCompanyAccess, getAccessibleResource, getActorInfo } from "./authz.js"; @@ -329,15 +330,21 @@ export function assetRoutes(db: Db, storage: StorageService) { const object = await storage.getObject(asset.companyId, asset.objectKey); const responseContentType = asset.contentType || object.contentType || "application/octet-stream"; + const mediaType = responseContentType.split(";", 1)[0]?.trim().toLowerCase() ?? ""; + const inlineSafe = mediaType !== SVG_CONTENT_TYPE + && isInlineAttachmentContentType(mediaType); res.setHeader("Content-Type", responseContentType); res.setHeader("Content-Length", String(asset.byteSize || object.contentLength || 0)); res.setHeader("Cache-Control", "private, max-age=60"); res.setHeader("X-Content-Type-Options", "nosniff"); - if (responseContentType === SVG_CONTENT_TYPE) { - res.setHeader("Content-Security-Policy", "sandbox; default-src 'none'; img-src 'self' data:; style-src 'unsafe-inline'"); + if (!inlineSafe) { + res.setHeader("Content-Security-Policy", "sandbox; default-src 'none'"); } const filename = asset.originalFilename ?? "asset"; - res.setHeader("Content-Disposition", `inline; filename=\"${filename.replaceAll("\"", "")}\"`); + const disposition = inlineSafe + ? "inline" + : "attachment"; + res.setHeader("Content-Disposition", `${disposition}; filename=\"${filename.replaceAll("\"", "")}\"`); object.stream.on("error", (err) => { next(err); diff --git a/server/src/routes/companies.ts b/server/src/routes/companies.ts index a6152c303a..0c7306a15c 100644 --- a/server/src/routes/companies.ts +++ b/server/src/routes/companies.ts @@ -49,6 +49,8 @@ import { writeImportTransferPart, } from "../services/company-import-transfers.js"; import { companyTransferRunService } from "../services/company-transfer-runs.js"; +import { agentInstructionsBundleMode } from "../services/agent-instructions.js"; +import { resolvePortableExportAgentSelection } from "../services/company-portability-agent-selection.js"; import { accessService, agentService, @@ -315,6 +317,25 @@ export function companyRoutes(db: Db, storage?: StorageService, options?: Compan return Math.floor(parsed); } + async function assertExternalInstructionExportAllowed( + req: Request, + companyId: string, + input: { include?: { agents?: boolean }; agents?: string[] }, + ) { + const instanceAdmin = req.actor.type === "board" + && (req.actor.source === "local_implicit" || req.actor.isInstanceAdmin === true); + const includesAgents = input.agents && input.agents.length > 0 + ? true + : input.include?.agents ?? true; + if (!includesAgents) return instanceAdmin; + const companyAgents = await agents.list(companyId, { includeTerminated: true }); + const selection = resolvePortableExportAgentSelection(companyAgents, input.agents, includesAgents); + if (selection.agents.some((agent) => agentInstructionsBundleMode(agent) === "external")) { + assertInstanceAdmin(req); + } + return instanceAdmin; + } + const timelineQuerySchema = z.object({ from: z.string().optional(), to: z.string().optional(), @@ -495,7 +516,8 @@ export function companyRoutes(db: Db, storage?: StorageService, options?: Compan const companyId = req.params.companyId as string; await assertSameCompanyCeoAgentOrBoard(req, companyId, "company exports"); const body = companyPortabilityExportSchema.parse(req.body); - const result = await portability.exportBundle(companyId, body); + const allowExternalInstructions = await assertExternalInstructionExportAllowed(req, companyId, body); + const result = await portability.exportBundle(companyId, body, { allowExternalInstructions }); res.json(result); }); @@ -1089,7 +1111,8 @@ export function companyRoutes(db: Db, storage?: StorageService, options?: Compan const companyId = req.params.companyId as string; await assertSameCompanyCeoAgentOrBoard(req, companyId, "company exports"); const body = companyPortabilityExportSchema.parse(req.body); - const preview = await portability.previewExport(companyId, body); + const allowExternalInstructions = await assertExternalInstructionExportAllowed(req, companyId, body); + const preview = await portability.previewExport(companyId, body, { allowExternalInstructions }); res.json(preview); }); @@ -1097,7 +1120,8 @@ export function companyRoutes(db: Db, storage?: StorageService, options?: Compan const companyId = req.params.companyId as string; await assertSameCompanyCeoAgentOrBoard(req, companyId, "company exports"); const body = companyPortabilityExportSchema.parse(req.body); - const result = await portability.exportBundle(companyId, body); + const allowExternalInstructions = await assertExternalInstructionExportAllowed(req, companyId, body); + const result = await portability.exportBundle(companyId, body, { allowExternalInstructions }); res.json(result); }); diff --git a/server/src/routes/projects.ts b/server/src/routes/projects.ts index 6f58cad51c..034386c549 100644 --- a/server/src/routes/projects.ts +++ b/server/src/routes/projects.ts @@ -131,6 +131,17 @@ export function projectRoutes(db: Db) { return false; } + async function assertRuntimeManageAllowed(req: Request, res: Response, companyId: string) { + const decision = await access.decide({ + actor: req.actor, + action: "runtime:manage", + resource: { type: "company", companyId }, + }); + if (decision.allowed) return true; + res.status(403).json({ error: "Runtime service control is outside this actor's authorization boundary" }); + return false; + } + async function filterProjectsForActor(req: Request, rows: T[]) { const decisions = await Promise.all(rows.map((project) => access.decide({ @@ -403,6 +414,7 @@ export function projectRoutes(db: Db) { res.status(404).json({ error: "Project workspace not found" }); return; } + if (!(await assertRuntimeManageAllowed(req, res, project.companyId))) return; const isSharedWorkspace = Boolean(workspace.sharedWorkspaceKey); if ( diff --git a/server/src/routes/secrets.ts b/server/src/routes/secrets.ts index e933b135e8..3d3ca863f2 100644 --- a/server/src/routes/secrets.ts +++ b/server/src/routes/secrets.ts @@ -116,6 +116,16 @@ export function secretRoutes(db: Db, deps: SecretRoutesDeps = {}) { const runRedactions = createRunSecretRedactionRegistry(db); const defaultProvider = getConfiguredSecretProvider(); + async function assertSecretCatalogReadAllowed(req: Parameters[0], companyId: string) { + const decision = await access.decide({ + actor: req.actor, + action: "secrets:read", + resource: { type: "company", companyId }, + }); + if (decision.allowed) return; + throw forbidden(decision.explanation, authorizationDeniedDetails(decision)); + } + function agentSecretContext(req: Parameters[0]) { if (req.actor.type !== "agent" || !req.actor.agentId || !req.actor.companyId || !req.actor.runId) { throw forbidden("Run-bound agent authentication required"); @@ -576,6 +586,7 @@ export function secretRoutes(db: Db, deps: SecretRoutesDeps = {}) { assertBoardOrAgent(req); const companyId = req.params.companyId as string; assertCompanyAccess(req, companyId); + await assertSecretCatalogReadAllowed(req, companyId); const secrets = await svc.list(companyId); res.json(secrets.map(({ id, name, key, status }) => ({ id, name, key, status }))); }); diff --git a/server/src/routes/workspace-command-authz.ts b/server/src/routes/workspace-command-authz.ts index 56e6b07ed7..7ca79a1c01 100644 --- a/server/src/routes/workspace-command-authz.ts +++ b/server/src/routes/workspace-command-authz.ts @@ -28,6 +28,21 @@ function collectWorkspaceStrategyCommandPaths(raw: unknown, prefix: string): str return paths; } +function collectWorkspaceRuntimeCommandPaths(raw: unknown, prefix: string): string[] { + if (!isRecord(raw)) return []; + const paths: string[] = []; + for (const collectionKey of ["commands", "services", "jobs"] as const) { + const entries = raw[collectionKey]; + if (!Array.isArray(entries)) continue; + entries.forEach((entry, index) => { + if (isRecord(entry) && hasOwn(entry, "command")) { + paths.push(`${prefixPath(prefix, collectionKey)}[${index}].command`); + } + }); + } + return paths; +} + function collectExecutionWorkspaceConfigCommandPaths(raw: unknown, prefix: string): string[] { if (!isRecord(raw)) return []; const paths: string[] = []; @@ -43,6 +58,12 @@ function collectExecutionWorkspaceConfigCommandPaths(raw: unknown, prefix: strin if (hasOwn(raw, "cleanupCommand")) { paths.push(prefixPath(prefix, "cleanupCommand")); } + paths.push( + ...collectWorkspaceRuntimeCommandPaths( + raw.workspaceRuntime, + prefixPath(prefix, "workspaceRuntime"), + ), + ); return paths; } @@ -66,10 +87,16 @@ export function collectAgentAdapterWorkspaceCommandPaths( export function collectProjectExecutionWorkspaceCommandPaths(policy: unknown): string[] { if (!isRecord(policy)) return []; - return collectWorkspaceStrategyCommandPaths( - policy.workspaceStrategy, - "executionWorkspacePolicy.workspaceStrategy", - ); + return [ + ...collectWorkspaceStrategyCommandPaths( + policy.workspaceStrategy, + "executionWorkspacePolicy.workspaceStrategy", + ), + ...collectWorkspaceRuntimeCommandPaths( + policy.workspaceRuntime, + "executionWorkspacePolicy.workspaceRuntime", + ), + ]; } export function collectProjectWorkspaceCommandPaths( @@ -77,9 +104,18 @@ export function collectProjectWorkspaceCommandPaths( prefix = "", ): string[] { if (!isRecord(workspacePatch)) return []; - return hasOwn(workspacePatch, "cleanupCommand") + const paths = hasOwn(workspacePatch, "cleanupCommand") ? [prefixPath(prefix, "cleanupCommand")] : []; + if (isRecord(workspacePatch.runtimeConfig)) { + paths.push( + ...collectWorkspaceRuntimeCommandPaths( + workspacePatch.runtimeConfig.workspaceRuntime, + prefixPath(prefix, "runtimeConfig.workspaceRuntime"), + ), + ); + } + return paths; } export function collectIssueWorkspaceCommandPaths(input: { @@ -94,6 +130,12 @@ export function collectIssueWorkspaceCommandPaths(input: { "executionWorkspaceSettings.workspaceStrategy", ), ); + paths.push( + ...collectWorkspaceRuntimeCommandPaths( + input.executionWorkspaceSettings.workspaceRuntime, + "executionWorkspaceSettings.workspaceRuntime", + ), + ); } if (isRecord(input.assigneeAdapterOverrides)) { const adapterConfig = input.assigneeAdapterOverrides.adapterConfig; diff --git a/server/src/services/agent-instructions.ts b/server/src/services/agent-instructions.ts index 3246671d35..5045143eb7 100644 --- a/server/src/services/agent-instructions.ts +++ b/server/src/services/agent-instructions.ts @@ -282,6 +282,19 @@ function deriveBundleState(agent: AgentLike): BundleState { }; } +/** Classify the configured bundle without touching the filesystem. */ +export function agentInstructionsBundleMode(agent: AgentLike): BundleMode | null { + const state = deriveBundleState(agent); + if (state.mode === "external") return "external"; + if ( + state.rootPath + && path.resolve(state.rootPath) !== resolveManagedInstructionsRoot(agent) + ) { + return "external"; + } + return state.mode; +} + async function recoverManagedBundleState(agent: AgentLike, state: BundleState): Promise { const managedRootPath = resolveManagedInstructionsRoot(agent); const stat = await statIfExists(managedRootPath); diff --git a/server/src/services/company-portability-agent-selection.ts b/server/src/services/company-portability-agent-selection.ts new file mode 100644 index 0000000000..e258eb81e7 --- /dev/null +++ b/server/src/services/company-portability-agent-selection.ts @@ -0,0 +1,67 @@ +import { normalizeAgentUrlKey } from "@paperclipai/shared"; +import { readBuiltInAgentMarker } from "./built-in-agent-metadata.js"; + +interface ExportAgentCandidate { + id: string; + name: string; + status: string; + metadata: unknown; +} + +export function resolvePortableExportAgentSelection( + allAgentRows: T[], + selectors: string[] | undefined, + includeAgents: boolean, +): { agents: T[]; warnings: string[] } { + const warnings: string[] = []; + const liveAgentRows = allAgentRows.filter((agent) => agent.status !== "terminated"); + const builtInAgentRows = liveAgentRows.filter((agent) => readBuiltInAgentMarker(agent.metadata)); + const portableAgentRows = liveAgentRows.filter((agent) => !readBuiltInAgentMarker(agent.metadata)); + + if (includeAgents) { + const skipped = allAgentRows.length - liveAgentRows.length; + if (skipped > 0) { + warnings.push(`Skipped ${skipped} terminated agent${skipped === 1 ? "" : "s"} from export.`); + } + if (builtInAgentRows.length > 0) { + warnings.push(`Skipped ${builtInAgentRows.length} built-in managed agent${builtInAgentRows.length === 1 ? "" : "s"} from export.`); + } + } + + const agentByReference = new Map(); + const builtInAgentByReference = new Map(); + const addAgentReferences = (map: Map, agent: T) => { + map.set(agent.id, agent); + map.set(agent.name, agent); + const normalizedName = normalizeAgentUrlKey(agent.name); + if (normalizedName) map.set(normalizedName, agent); + }; + for (const agent of portableAgentRows) addAgentReferences(agentByReference, agent); + for (const agent of builtInAgentRows) addAgentReferences(builtInAgentByReference, agent); + + const selectedAgents = new Map(); + for (const selector of selectors ?? []) { + const trimmed = selector.trim(); + if (!trimmed) continue; + const normalized = normalizeAgentUrlKey(trimmed) ?? trimmed; + const match = agentByReference.get(trimmed) ?? agentByReference.get(normalized); + if (!match) { + const builtInMatch = builtInAgentByReference.get(trimmed) ?? builtInAgentByReference.get(normalized); + if (builtInMatch) { + warnings.push(`Agent selector "${selector}" is a built-in managed agent and was skipped.`); + } else { + warnings.push(`Agent selector "${selector}" was not found and was skipped.`); + } + continue; + } + selectedAgents.set(match.id, match); + } + + // Preserve the established compatibility behavior: no effective explicit + // selection falls back to every portable agent when agent export is enabled. + if (includeAgents && selectedAgents.size === 0) { + for (const agent of portableAgentRows) selectedAgents.set(agent.id, agent); + } + + return { agents: Array.from(selectedAgents.values()), warnings }; +} diff --git a/server/src/services/company-portability.ts b/server/src/services/company-portability.ts index 8007f9e3a0..23ec71f0fe 100644 --- a/server/src/services/company-portability.ts +++ b/server/src/services/company-portability.ts @@ -76,7 +76,7 @@ import { ghFetch, gitHubApiBase, resolveRawGitHubUrl } from "./github-fetch.js"; import type { StorageService } from "../storage/types.js"; import { accessService } from "./access.js"; import { agentService } from "./agents.js"; -import { agentInstructionsService } from "./agent-instructions.js"; +import { agentInstructionsBundleMode, agentInstructionsService } from "./agent-instructions.js"; import { assetService } from "./assets.js"; import { generateReadme } from "./company-export-readme.js"; import { renderOrgChartPng, type OrgNode } from "../routes/org-chart-svg.js"; @@ -96,7 +96,7 @@ import { readCatalogStringList, readPortableCatalogProvenance, } from "./catalog-provenance.js"; -import { readBuiltInAgentMarker } from "./built-in-agent-metadata.js"; +import { resolvePortableExportAgentSelection } from "./company-portability-agent-selection.js"; import { normalizePortablePath } from "./portable-path.js"; import type { ImportIssueRow, @@ -555,6 +555,9 @@ function buildSkillExportDirMap(skills: CompanySkill[], companyIssuePrefix: stri function isSensitiveEnvKey(key: string) { const normalized = key.trim().toLowerCase(); return ( + normalized === "key" || + normalized.endsWith("_key") || + normalized.endsWith("-key") || normalized === "token" || normalized.endsWith("_token") || normalized.endsWith("-token") || @@ -3834,7 +3837,7 @@ export function companyPortabilityService(db: Db, storage?: StorageService) { async function exportBundle( companyId: string, input: CompanyPortabilityExport, - options: { preview?: boolean } = {}, + options: { preview?: boolean; allowExternalInstructions?: boolean } = {}, ): Promise { const include = normalizeInclude({ ...input.include, @@ -3877,67 +3880,16 @@ export function companyPortabilityService(db: Db, storage?: StorageService) { ); const allAgentRows = include.agents ? await agents.list(companyId, { includeTerminated: true }) : []; - const liveAgentRows = allAgentRows.filter((agent) => agent.status !== "terminated"); - const builtInAgentRows = liveAgentRows.filter((agent) => readBuiltInAgentMarker(agent.metadata)); - const portableAgentRows = liveAgentRows.filter((agent) => !readBuiltInAgentMarker(agent.metadata)); + const agentSelection = resolvePortableExportAgentSelection(allAgentRows, input.agents, include.agents); const companySkillRowsRaw = include.skills ? await companySkills.listFull(companyId) : []; const managedSkillRows = companySkillRowsRaw.filter((skill) => managedSkillIds.has(skill.id)); const companySkillRows = companySkillRowsRaw.filter((skill) => !managedSkillIds.has(skill.id)); - if (include.agents) { - const skipped = allAgentRows.length - liveAgentRows.length; - if (skipped > 0) { - warnings.push(`Skipped ${skipped} terminated agent${skipped === 1 ? "" : "s"} from export.`); - } - if (builtInAgentRows.length > 0) { - warnings.push(`Skipped ${builtInAgentRows.length} built-in managed agent${builtInAgentRows.length === 1 ? "" : "s"} from export.`); - } - } + warnings.push(...agentSelection.warnings); if (include.skills && managedSkillRows.length > 0) { warnings.push(`Skipped ${managedSkillRows.length} built-in managed skill${managedSkillRows.length === 1 ? "" : "s"} from export.`); } - const agentByReference = new Map(); - const builtInAgentByReference = new Map(); - const addAgentReferences = (map: Map, agent: typeof liveAgentRows[number]) => { - map.set(agent.id, agent); - map.set(agent.name, agent); - const normalizedName = normalizeAgentUrlKey(agent.name); - if (normalizedName) { - map.set(normalizedName, agent); - } - }; - for (const agent of portableAgentRows) { - addAgentReferences(agentByReference, agent); - } - for (const agent of builtInAgentRows) { - addAgentReferences(builtInAgentByReference, agent); - } - - const selectedAgents = new Map(); - for (const selector of input.agents ?? []) { - const trimmed = selector.trim(); - if (!trimmed) continue; - const normalized = normalizeAgentUrlKey(trimmed) ?? trimmed; - const match = agentByReference.get(trimmed) ?? agentByReference.get(normalized); - if (!match) { - const builtInMatch = builtInAgentByReference.get(trimmed) ?? builtInAgentByReference.get(normalized); - if (builtInMatch) { - warnings.push(`Agent selector "${selector}" is a built-in managed agent and was skipped.`); - continue; - } - warnings.push(`Agent selector "${selector}" was not found and was skipped.`); - continue; - } - selectedAgents.set(match.id, match); - } - - if (include.agents && selectedAgents.size === 0) { - for (const agent of portableAgentRows) { - selectedAgents.set(agent.id, agent); - } - } - - const agentRows = Array.from(selectedAgents.values()) + const agentRows = agentSelection.agents .sort((left, right) => left.name.localeCompare(right.name)); const usedSlugs = new Set(); @@ -4118,7 +4070,7 @@ export function companyPortabilityService(db: Db, storage?: StorageService) { projectSlugById.set(project.id, uniqueSlug(baseSlug, usedProjectSlugs)); } const sidebarOrder = requestedSidebarOrder ?? stripEmptyValues({ - agents: sortAgentsBySidebarOrder(Array.from(selectedAgents.values())) + agents: sortAgentsBySidebarOrder(agentSelection.agents) .map((agent) => idToSlug.get(agent.id)) .filter((slug): slug is string => Boolean(slug)), projects: selectedProjectRows @@ -4227,6 +4179,12 @@ export function companyPortabilityService(db: Db, storage?: StorageService) { } if (include.agents) { + if ( + !options.allowExternalInstructions + && agentRows.some((agent) => agentInstructionsBundleMode(agent) === "external") + ) { + throw forbidden("Instance admin access is required to export external instruction bundles"); + } const agentInstructionsById = new Map( await mapWithConcurrency(agentRows, EXPORT_READ_CONCURRENCY, async (agent) => ( [agent.id, await instructions.exportFiles(agent)] as const @@ -4817,6 +4775,7 @@ export function companyPortabilityService(db: Db, storage?: StorageService) { async function previewExport( companyId: string, input: CompanyPortabilityExport, + options: { allowExternalInstructions?: boolean } = {}, ): Promise { const previewInput: CompanyPortabilityExport = { ...input, @@ -4831,7 +4790,10 @@ export function companyPortabilityService(db: Db, storage?: StorageService) { if (previewInput.include && previewInput.include.issues === undefined) { previewInput.include.issues = false; } - const exported = await exportBundle(companyId, previewInput, { preview: true }); + const exported = await exportBundle(companyId, previewInput, { + preview: true, + allowExternalInstructions: options.allowExternalInstructions, + }); return { ...exported, fileInventory: Object.keys(exported.files) diff --git a/server/src/services/feedback.ts b/server/src/services/feedback.ts index 7698520eb0..018a82546f 100644 --- a/server/src/services/feedback.ts +++ b/server/src/services/feedback.ts @@ -38,7 +38,7 @@ import { } from "@paperclipai/shared"; import { resolveHomeAwarePath, resolvePaperclipInstanceRoot } from "../home-paths.js"; import { notFound, unprocessable } from "../errors.js"; -import { agentInstructionsService } from "./agent-instructions.js"; +import { agentInstructionsBundleMode, agentInstructionsService } from "./agent-instructions.js"; import { createFeedbackRedactionState, finalizeFeedbackRedactionSummary, @@ -1168,12 +1168,23 @@ async function buildAgentContext( : []; const usage = asRecord(run?.usageJson) ?? {}; + const externalInstructions = agentInstructionsBundleMode({ + id: agent.id, + companyId: agent.companyId, + name: agent.name, + adapterConfig: agent.adapterConfig, + }) === "external"; + if (externalInstructions) { + state.omittedFields.add("bundle.agentContext.runtime.configuredInstructionsFilePath"); + state.omittedFields.add("bundle.agentContext.runtime.configuredInstructionsRootPath"); + state.omittedFields.add("bundle.agentContext.instructions"); + } const runtime = { configuredModel: asString(adapterConfig.model), configuredInstructionsBundleMode: asString(adapterConfig.instructionsBundleMode), configuredInstructionsEntryFile: asString(adapterConfig.instructionsEntryFile), - configuredInstructionsFilePath: asString(adapterConfig.instructionsFilePath), - configuredInstructionsRootPath: asString(adapterConfig.instructionsRootPath), + configuredInstructionsFilePath: externalInstructions ? null : asString(adapterConfig.instructionsFilePath), + configuredInstructionsRootPath: externalInstructions ? null : asString(adapterConfig.instructionsRootPath), heartbeatPolicy: sanitizeFeedbackValue(runtimeConfig.heartbeat ?? null, state, "bundle.agentContext.runtime.heartbeatPolicy", 400), provenanceMode: run ? "source_run" : "vote_time_snapshot", sourceRun: run @@ -1218,12 +1229,14 @@ async function buildAgentContext( : null, }; - const instructionsBundle = await instructionsSvc.getBundle({ - id: agent.id, - companyId: agent.companyId, - name: agent.name, - adapterConfig: agent.adapterConfig, - }).catch(() => null); + const instructionsBundle = externalInstructions + ? null + : await instructionsSvc.getBundle({ + id: agent.id, + companyId: agent.companyId, + name: agent.name, + adapterConfig: agent.adapterConfig, + }).catch(() => null); let entryDigest: string | null = null; let entryBody: string | null = null; diff --git a/server/src/services/plugin-managed-agents.ts b/server/src/services/plugin-managed-agents.ts index feb725c0b3..13473ec8c0 100644 --- a/server/src/services/plugin-managed-agents.ts +++ b/server/src/services/plugin-managed-agents.ts @@ -16,7 +16,7 @@ import { notFound } from "../errors.js"; import { agentService } from "./agents.js"; import { approvalService } from "./approvals.js"; import { logActivity } from "./activity-log.js"; -import { agentInstructionsService } from "./agent-instructions.js"; +import { agentInstructionsBundleMode, agentInstructionsService } from "./agent-instructions.js"; const MANAGED_AGENT_ENTITY_TYPE = "managed_agent"; const DEFAULT_MANAGED_AGENT_ADAPTER_TYPE = "process"; @@ -364,6 +364,9 @@ export function pluginManagedAgentService( const variables = await optionsForInstructionVariables(companyId); const declared = declaredInstructionFiles(declaration, variables); if (!declared) return null; + if (agentInstructionsBundleMode(agent) === "external") { + return { entryFile: declared.entryFile, changedFiles: [declared.entryFile] }; + } let exported: Awaited>; try { From 2177b85eb5116b11bdcfb8376e81dbd6dcd2645b Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Thu, 3 Sep 2026 12:16:55 -0700 Subject: [PATCH 4/5] fix(server): retry cloud-tenant auth sync once on a dropped DB connection (#12773) 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 > - Managed-cloud deployments authenticate tenant requests through trusted headers. The middleware syncs the tenant's user, company, and membership rows on the way through. > - Pooled Postgres endpoints sometimes close an established connection under an in-flight query (pooler recycle, compute suspend). The driver reconnects on the next query, but the statement on the wire fails. > - In this path a single dropped statement fails the whole request with a 500. This happened live on 2026-09-03: the idempotent company bootstrap insert died with `write CONNECTION_CLOSED`. > - This pull request retries the actor resolution exactly once when the error chain carries a postgres.js closed-connection code. The sync is idempotent end to end, so the replay is safe. > - The benefit is that a routine pooler blip no longer fails an authenticated request on the entry path. ## Linked Issues or Issue Description **What happened?** A cloud tenant request hit the trusted-header authentication middleware while the pooled Postgres endpoint closed the connection mid-query. The insert failed with `write CONNECTION_CLOSED :5432` wrapped in a `Failed query: insert into "companies" …` error, and the request failed. **Expected behavior** The driver reconnects on the next query, and every statement in the tenant sync is idempotent (upserts, on-conflict inserts, deletes; the write debounce records only after the full sync succeeds). One in-request retry should absorb the blip and serve the request. Non-transient failures must keep failing fast. **Steps to reproduce** 1. Run an authenticated public deployment against a pooled Postgres endpoint. 2. Have the pooler close the connection while the middleware's tenant sync insert is on the wire. 3. Before this change the request fails with a 500; after it the retry serves the request. **Deployment mode** Authenticated public (managed cloud), external pooled PostgreSQL. ## What Changed - `resolveCloudTenantActor` now delegates to the (unchanged) resolution body through `retryOnTransientDbConnectionError`, which retries exactly once on a transient closed-connection failure - `isTransientDbConnectionError` walks the error `cause` chain (drizzle wraps the driver error) for the postgres.js codes `CONNECTION_CLOSED`, `CONNECTION_ENDED`, `CONNECTION_DESTROYED`; both helpers are exported for tests - New unit test file `cloud-tenant-transient-db-retry.test.ts`: detection matrix (including a `23505` staying non-transient), retry-once-then-succeed, no-retry on non-transient, propagate-on-second-failure ## Verification - `pnpm vitest run src/__tests__/cloud-tenant-transient-db-retry.test.ts` — 5 passed - `pnpm vitest run src/__tests__/cloud-tenant-company-provisioning.test.ts` — 7 passed against embedded Postgres, driving the real resolution path through the new wrapper ## Risks - Low risk. The retry is bounded to one attempt, gated on three explicit driver codes, and wraps an operation that is already idempotent by design. Every other failure propagates unchanged. - A genuinely down database now fails after two attempts instead of one — a few milliseconds of added latency on an already-failing request. ## Model Used Claude Fable 5 (claude-fable-5) via Claude Code, extended thinking with tool use. ## 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 (none found for connection-retry work in this path) - [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 (doc comments; no user-facing docs affected) - [x] I have considered and documented any risks above --- .../cloud-tenant-transient-db-retry.test.ts | 68 +++++++++++++++++++ server/src/middleware/auth.ts | 53 +++++++++++++++ 2 files changed, 121 insertions(+) create mode 100644 server/src/__tests__/cloud-tenant-transient-db-retry.test.ts diff --git a/server/src/__tests__/cloud-tenant-transient-db-retry.test.ts b/server/src/__tests__/cloud-tenant-transient-db-retry.test.ts new file mode 100644 index 0000000000..70ada7397d --- /dev/null +++ b/server/src/__tests__/cloud-tenant-transient-db-retry.test.ts @@ -0,0 +1,68 @@ +import { describe, expect, it } from "vitest"; + +import { + isTransientDbConnectionError, + retryOnTransientDbConnectionError, +} from "../middleware/auth.ts"; + +/** The shape drizzle produces: a wrapper whose `cause` is the driver error. */ +function driverClosedError(code: string): Error { + const driver = Object.assign(new Error(`write ${code} db.example.internal:5432`), { code }); + return new Error("Failed query: insert into \"companies\" (…)", { cause: driver }); +} + +describe("isTransientDbConnectionError", () => { + it("detects a closed-connection code anywhere on the cause chain", () => { + expect(isTransientDbConnectionError(driverClosedError("CONNECTION_CLOSED"))).toBe(true); + expect(isTransientDbConnectionError(driverClosedError("CONNECTION_ENDED"))).toBe(true); + expect(isTransientDbConnectionError(driverClosedError("CONNECTION_DESTROYED"))).toBe(true); + const bare = Object.assign(new Error("write CONNECTION_CLOSED host:5432"), { + code: "CONNECTION_CLOSED", + }); + expect(isTransientDbConnectionError(bare)).toBe(true); + }); + + it("rejects everything else", () => { + expect(isTransientDbConnectionError(new Error("boom"))).toBe(false); + const unique = Object.assign(new Error("duplicate key"), { code: "23505" }); + expect(isTransientDbConnectionError(unique)).toBe(false); + expect(isTransientDbConnectionError(new Error("outer", { cause: unique }))).toBe(false); + expect(isTransientDbConnectionError("CONNECTION_CLOSED")).toBe(false); + expect(isTransientDbConnectionError(undefined)).toBe(false); + }); +}); + +describe("retryOnTransientDbConnectionError", () => { + it("retries exactly once after a transient closed connection", async () => { + let calls = 0; + const result = await retryOnTransientDbConnectionError(async () => { + calls += 1; + if (calls === 1) throw driverClosedError("CONNECTION_CLOSED"); + return "ok"; + }); + expect(result).toBe("ok"); + expect(calls).toBe(2); + }); + + it("propagates a non-transient failure without retrying", async () => { + let calls = 0; + await expect( + retryOnTransientDbConnectionError(async () => { + calls += 1; + throw new Error("constraint violation"); + }), + ).rejects.toThrow("constraint violation"); + expect(calls).toBe(1); + }); + + it("propagates the second failure when the retry also dies", async () => { + let calls = 0; + await expect( + retryOnTransientDbConnectionError(async () => { + calls += 1; + throw driverClosedError("CONNECTION_CLOSED"); + }), + ).rejects.toThrow("Failed query"); + expect(calls).toBe(2); + }); +}); diff --git a/server/src/middleware/auth.ts b/server/src/middleware/auth.ts index e2e4b84d59..1125cbf4f7 100644 --- a/server/src/middleware/auth.ts +++ b/server/src/middleware/auth.ts @@ -512,9 +512,62 @@ export function cloudActorHeaderSourceFromHeaders( }; } +/** + * postgres.js codes for a connection the server side closed out from under + * an in-flight query — a pooled Postgres endpoint recycling or suspending + * (observed 2026-09-03 with a managed pooler closing the socket mid-INSERT). + * The driver reconnects transparently on the next query; only the statement + * that was on the wire is lost. + */ +const transientDbConnectionCodes = new Set([ + "CONNECTION_CLOSED", + "CONNECTION_ENDED", + "CONNECTION_DESTROYED", +]); + +/** + * True when the error chain (drizzle wraps the driver error as `cause`) + * carries a postgres.js closed-connection code. Exported for tests. + */ +export function isTransientDbConnectionError(error: unknown): boolean { + for (let current: unknown = error; current instanceof Error; current = current.cause) { + const code = (current as { code?: unknown }).code; + if (typeof code === "string" && transientDbConnectionCodes.has(code)) return true; + } + return false; +} + +/** + * Runs `run` and retries it exactly once when it fails on a transient + * closed-connection error. Callers must pass an idempotent operation. + * Exported for tests. + */ +export async function retryOnTransientDbConnectionError(run: () => Promise): Promise { + try { + return await run(); + } catch (error) { + if (!isTransientDbConnectionError(error)) throw error; + return run(); + } +} + +/** + * Trusted-header actor resolution with a single transient-connection retry. + * The tenant sync inside is idempotent end to end — every write is an + * upsert/on-conflict/delete and the write debounce records only after the + * whole sync succeeds — so replaying it after a dropped connection is safe, + * and turns a golden-path authentication 500 into a served request. + */ export async function resolveCloudTenantActor( db: Db, req: CloudActorHeaderSource, +): Promise { + return retryOnTransientDbConnectionError(() => resolveCloudTenantActorOnce(db, req)); +} + +async function resolveCloudTenantActorOnce( + db: Db, + req: CloudActorHeaderSource, ): Promise { const expectedToken = process.env.PAPERCLIP_CLOUD_TENANT_SERVER_TOKEN?.trim(); if (!expectedToken) return null; From db4eeb168812be1e76edc0a435aac8a7bcb9ce2b Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Thu, 3 Sep 2026 12:17:27 -0700 Subject: [PATCH 5/5] fix(server): validate project goal ids exist and belong to the company (#12779) 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 > - Projects can link to goals, through the `goalIds` list or the legacy `goalId` field. The project service writes those links on create and update. > - The service never checked the goal ids. A nonexistent id died at the `projects.goal_id` foreign key as an opaque 500, and the caller got no actionable feedback — observed live on 2026-09-03, where one caller retried the same bad id four times. > - The foreign key also only proves a goal exists, not who owns it. A goal id from another company linked silently on a multi-company instance. > - This pull request asserts every resolved goal id exists under the caller's company before any write, and rejects with a 422 that names the unknown ids. > - The benefit is a clear, actionable client error instead of a 500, and no cross-company goal links. ## Linked Issues or Issue Description **What happened?** `POST /companies/:companyId/projects` with a `goalIds` entry that does not exist fails with an internal error: `insert or update on table "projects" violates foreign key constraint "projects_goal_id_goals_id_fk"`. The caller sees a 500 and retries. A goal id that exists but belongs to a different company is accepted and linked. **Expected behavior** The request fails fast with a 422 that names the unknown goal id(s). Goals from other companies are rejected the same way. Valid links behave exactly as before. **Steps to reproduce** 1. Create a company and no goals. 2. `POST /companies/:companyId/projects` with `{ "name": "Rocket", "goalIds": [""] }`. 3. Before this change: 500 from the foreign key. After: 422 naming the id. **Deployment mode** Any; observed on an authenticated public deployment. ## What Changed - `assertGoalsBelongToCompany` in the project service: one query for the resolved ids scoped to the company; unknown ids produce `unprocessable` (422) with the ids in the message and details - called on create (before the project row insert, so no partial writes) and on update (scoped to the existing project's company); both `goalIds` and the legacy `goalId` field flow through the same resolution - new embedded-Postgres test file: valid link, nonexistent id on create with no partial insert, legacy field, another company's goal on create, and a foreign-goal update that leaves existing links unchanged ## Verification - `pnpm vitest run src/__tests__/project-goal-validation.test.ts` — 5 passed - adjacent suites (`project-icon-persistence`, `project-shortname-resolution`, `issue-goal-fallback`, `project-goal-telemetry-routes`, `heartbeat-referenced-projects`, `projects-list-archived-routes`) — 35 passed ## Risks - Low risk. One extra indexed select per create/update that carries goal ids. Requests that previously 500ed now 422; requests that silently linked a foreign goal now fail — both are corrections, not regressions. - Existing rows with foreign links (written before this check) are untouched; only new writes validate. ## Model Used Claude Fable 5 (claude-fable-5) via Claude Code, extended thinking with tool use. ## 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 (none found for goal-id validation) - [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 (doc comments; no user-facing docs affected) - [x] I have considered and documented any risks above --- .../__tests__/project-goal-validation.test.ts | 162 ++++++++++++++++++ server/src/services/projects.ts | 36 +++- 2 files changed, 197 insertions(+), 1 deletion(-) create mode 100644 server/src/__tests__/project-goal-validation.test.ts diff --git a/server/src/__tests__/project-goal-validation.test.ts b/server/src/__tests__/project-goal-validation.test.ts new file mode 100644 index 0000000000..117a792be2 --- /dev/null +++ b/server/src/__tests__/project-goal-validation.test.ts @@ -0,0 +1,162 @@ +import { afterAll, afterEach, beforeAll, describe, expect, it } from "vitest"; +import { eq } from "drizzle-orm"; +import { + companies, + createDb, + goals, + projectGoals, + projects as projectsTable, +} from "@paperclipai/db"; +import { + getEmbeddedPostgresTestSupport, + startEmbeddedPostgresTestDatabase, +} from "./helpers/embedded-postgres.js"; +import { HttpError } from "../errors.js"; +import { projectService } from "../services/projects.js"; + +const embeddedPostgresSupport = await getEmbeddedPostgresTestSupport(); +const describeEmbeddedPostgres = embeddedPostgresSupport.supported ? describe : describe.skip; + +if (!embeddedPostgresSupport.supported) { + console.warn( + `Skipping embedded Postgres project goal validation tests on this host: ${embeddedPostgresSupport.reason ?? "unsupported environment"}`, + ); +} + +// A project's goals must exist and belong to the same company. Before this +// validation, a nonexistent goal id died at the projects.goal_id foreign key +// as an opaque 500 (observed live 2026-09-03, retried four times by the +// caller), and a goal from another company linked silently — the foreign key +// proves existence, not ownership. +describeEmbeddedPostgres("project goal validation", () => { + let db!: ReturnType; + let tempDb: Awaited> | null = null; + let prefixCounter = 0; + + beforeAll(async () => { + tempDb = await startEmbeddedPostgresTestDatabase("paperclip-project-goal-validation-"); + db = createDb(tempDb.connectionString); + }, 20_000); + + afterEach(async () => { + await db.delete(projectGoals); + await db.delete(projectsTable); + await db.delete(goals); + await db.delete(companies); + }); + + afterAll(async () => { + await tempDb?.cleanup(); + }); + + async function seedCompany(name: string): Promise { + prefixCounter += 1; + const [company] = await db + .insert(companies) + .values({ name, issuePrefix: `GV${prefixCounter}` }) + .returning(); + return company.id; + } + + async function seedGoal(companyId: string, title: string): Promise { + const [goal] = await db + .insert(goals) + .values({ companyId, title, level: "task", status: "active" }) + .returning(); + return goal.id; + } + + function expectUnprocessable(error: unknown, unknownGoalId: string) { + expect(error).toBeInstanceOf(HttpError); + expect((error as HttpError).status).toBe(422); + expect((error as Error).message).toContain(unknownGoalId); + } + + it("creates and links a project to a goal of the same company", async () => { + const companyId = await seedCompany("Valid Co"); + const goalId = await seedGoal(companyId, "Ship it"); + const projects = projectService(db); + + const created = await projects.create(companyId, { name: "Rocket", goalIds: [goalId] }); + expect(created.goalIds).toEqual([goalId]); + }); + + it("rejects a create whose goal id does not exist, before any insert", async () => { + const companyId = await seedCompany("Missing Goal Co"); + const projects = projectService(db); + const ghost = "db7da378-99a6-43ea-964f-000000000000"; + + const failure = await projects + .create(companyId, { name: "Rocket", goalIds: [ghost] }) + .then(() => null, (error: unknown) => error); + expectUnprocessable(failure, ghost); + + const rows = await db.select({ id: projectsTable.id }).from(projectsTable); + expect(rows).toHaveLength(0); + }); + + it("rejects the legacy single goalId field the same way", async () => { + const companyId = await seedCompany("Legacy Field Co"); + const projects = projectService(db); + const ghost = "db7da378-99a6-43ea-964f-111111111111"; + + const failure = await projects + .create(companyId, { name: "Rocket", goalId: ghost }) + .then(() => null, (error: unknown) => error); + expectUnprocessable(failure, ghost); + }); + + it("rejects another company's goal on create — the FK only proves existence", async () => { + const companyId = await seedCompany("Home Co"); + const otherCompanyId = await seedCompany("Other Co"); + const foreignGoalId = await seedGoal(otherCompanyId, "Not yours"); + const projects = projectService(db); + + const failure = await projects + .create(companyId, { name: "Rocket", goalIds: [foreignGoalId] }) + .then(() => null, (error: unknown) => error); + expectUnprocessable(failure, foreignGoalId); + }); + + it("ignores the legacy goalId when an explicit empty goalIds list wins resolution", async () => { + // goalIds and goalId may arrive together; the resolved set (goalIds + // first) is canonical for persistence too. Before this rule, an empty + // list skipped validation while the raw legacy id was still written — + // unvalidated, and unchecked for ownership. + const companyId = await seedCompany("Conflicting Fields Co"); + const otherCompanyId = await seedCompany("Conflicting Other Co"); + const foreignGoalId = await seedGoal(otherCompanyId, "Should not link"); + const projects = projectService(db); + + const created = await projects.create(companyId, { + name: "Rocket", + goalIds: [], + goalId: foreignGoalId, + }); + expect(created.goalIds).toEqual([]); + + const [row] = await db + .select({ goalId: projectsTable.goalId }) + .from(projectsTable) + .where(eq(projectsTable.id, created.id)); + expect(row.goalId).toBeNull(); + }); + + it("rejects an update to an unknown or foreign goal and leaves links unchanged", async () => { + const companyId = await seedCompany("Update Co"); + const otherCompanyId = await seedCompany("Update Other Co"); + const goodGoalId = await seedGoal(companyId, "Good goal"); + const foreignGoalId = await seedGoal(otherCompanyId, "Foreign goal"); + const projects = projectService(db); + + const created = await projects.create(companyId, { name: "Rocket", goalIds: [goodGoalId] }); + + const failure = await projects + .update(created.id, { goalIds: [foreignGoalId] }) + .then(() => null, (error: unknown) => error); + expectUnprocessable(failure, foreignGoalId); + + const fetched = await projects.getById(created.id); + expect(fetched?.goalIds).toEqual([goodGoalId]); + }); +}); diff --git a/server/src/services/projects.ts b/server/src/services/projects.ts index 5ac1b50e4c..0f193a98a7 100644 --- a/server/src/services/projects.ts +++ b/server/src/services/projects.ts @@ -28,6 +28,7 @@ import { type PluginManagedProjectDeclaration, type PluginManagedProjectResolution, } from "@paperclipai/shared"; +import { unprocessable } from "../errors.js"; import { listCurrentRuntimeServicesForProjectWorkspaces } from "./workspace-runtime-read-model.js"; import { parseProjectExecutionWorkspacePolicy } from "./execution-workspace-policy.js"; import { mergeProjectWorkspaceRuntimeConfig, readProjectWorkspaceRuntimeConfig } from "./project-workspace-runtime-config.js"; @@ -401,6 +402,31 @@ async function attachListMetrics( } /** Sync the project_goals join table for a single project. */ +/** + * Every goal a project links to must exist and belong to the same company. + * Without this check a nonexistent id only dies at the projects.goal_id + * foreign key — an opaque 500 the caller retries (observed live + * 2026-09-03: four identical retries of one bad id) — and a goal from + * another company would link silently, because the foreign key proves + * existence, not ownership. + */ +async function assertGoalsBelongToCompany(db: Db, companyId: string, goalIds: string[]): Promise { + if (goalIds.length === 0) return; + const unique = [...new Set(goalIds)]; + const found = await db + .select({ id: goals.id }) + .from(goals) + .where(and(eq(goals.companyId, companyId), inArray(goals.id, unique))); + const foundIds = new Set(found.map((row) => row.id)); + const unknown = unique.filter((goalId) => !foundIds.has(goalId)); + if (unknown.length > 0) { + throw unprocessable( + `Unknown goal id(s) for this company: ${unknown.join(", ")}`, + { unknownGoalIds: unknown }, + ); + } +} + async function syncGoalLinks(db: Db, projectId: string, companyId: string, goalIds: string[]) { // Delete existing links await db.delete(projectGoals).where(eq(projectGoals.projectId, projectId)); @@ -547,6 +573,7 @@ export function projectService(db: Db) { ): Promise => { const { goalIds: inputGoalIds, ...projectData } = data; const ids = resolveGoalIds({ goalIds: inputGoalIds, goalId: projectData.goalId }); + if (ids && ids.length > 0) await assertGoalsBelongToCompany(db, companyId, ids); // Note: color is intentionally NOT auto-assigned. New projects default to // `color = null` (neutral gray) unless an explicit color is supplied. See PAP-68. @@ -558,7 +585,11 @@ export function projectService(db: Db) { projectData.name = resolveProjectNameForUniqueShortname(projectData.name, existingProjects); // Also write goalId to the legacy column (first goal or null) - const legacyGoalId = ids && ids.length > 0 ? ids[0] : projectData.goalId ?? null; + // The resolved set is canonical for persistence as well as validation: + // falling back to the raw legacy field here would write an id that + // skipped validation whenever `goalIds: []` and `goalId` arrive + // together (goalIds wins resolution, mirroring the update path). + const legacyGoalId = ids?.[0] ?? null; const row = await db .insert(projects) @@ -794,6 +825,9 @@ export function projectService(db: Db) { .where(eq(projects.id, id)) .then((rows) => rows[0] ?? null); if (!existingProject) return null; + if (ids && ids.length > 0) { + await assertGoalsBelongToCompany(db, existingProject.companyId, ids); + } if (projectData.name !== undefined) { const existingShortname = normalizeProjectUrlKey(existingProject.name);