From 1c366a9059b7ecb7ba1d8f70902704c766776d8c Mon Sep 17 00:00:00 2001 From: scotttong Date: Mon, 17 Aug 2026 21:36:11 -0700 Subject: [PATCH] fix(server): reject invalid agent credentials instead of downgrading to the local user actor (#11589) 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 authenticates each agent request in `actorMiddleware` before it attributes chat comments > - When an agent bearer token failed verification, the middleware called `next()` with no error and the request continued without an agent actor > - The request then fell back to the local user actor, so the server stored agent replies as user comments > - The task chat UI renders user comments in blue bubbles, so agent messages appeared as blue user bubbles > - This pull request rejects invalid agent credentials with 401 instead of a silent downgrade > - The benefit is that agent messages keep agent attribution, and broken credentials fail loudly with a clear retry message ## Linked Issues or Issue Description **What happened?** A user cancelled an onboarding question card. The agent posted a follow-up reply. The reply appeared in a blue bubble, which the UI reserves for human messages. The agent run held an expired local agent JWT. The auth middleware could not verify the token, called `next()` without an actor, and the request fell back to the local user identity. The server stored the agent comment as a user comment. **Expected behavior** Agent messages always render as agent bubbles. A request with invalid agent credentials must fail with 401 so the adapter can refresh credentials and retry. It must not post content under a human identity. **Steps to reproduce** 1. Start a local Paperclip instance. 2. Give an agent run an expired or malformed agent JWT. 3. Let the agent post an issue comment through the API bridge. 4. Before this change: the comment is stored with the local user identity and renders as a blue bubble. After this change: the request fails with 401 and a message that tells the caller to obtain fresh credentials. ## What Changed - `server/src/middleware/auth.ts`: a bearer token that fails verification now produces a 401 `unauthorized` error instead of a silent fall-through to the anonymous/local-user actor. - The 401 message states the cause: expired token, unverifiable token, empty bearer token, missing agent record, agent record in another company, terminated agent, or agent pending approval. - The API-key path now also rejects an agent record whose company does not match the key. - `packages/adapter-utils/src/execution-target.ts`: the bridge proxy now writes a `comment id: ` marker to the run log for each posted issue comment, so misattributed comments can be traced to a run. - `ui/src/components/task-chat/task-chat-adapter.test.ts`: a regression test asserts that a recovered `local-board` comment with a derived agent author renders as an agent bubble, not a user bubble. - `server/src/__tests__/agent-auth-middleware.test.ts` and `packages/adapter-utils/src/execution-target-sandbox.test.ts`: new tests cover each rejection path and the log marker. ## Verification - Run `pnpm vitest run src/__tests__/agent-auth-middleware.test.ts` in `server/` — 14 tests pass. - Run `pnpm vitest run execution-target-sandbox` at the repo root — 44 tests pass. - Run `pnpm vitest run src/components/task-chat/task-chat-adapter.test.ts` in `ui/` — 4 tests pass. - Manual check: post an issue comment with an expired agent JWT; the API returns 401 with a retry message and no comment is stored. ## Risks - Behavioral shift: requests that previously continued as anonymous or local-user actors after a failed agent-token verification now receive 401. Any caller that relied on the silent downgrade must refresh its credentials. This is the intended fix, and the adapters already handle 401 with a credential refresh. - No schema or migration changes. Low risk otherwise. > 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 - Claude (Anthropic), model ID `claude-fable-5`, via Claude Code with extended thinking and tool use (agent harness with shell, file, and git tools). ## 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 - [ ] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [ ] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip Co-authored-by: Claude Fable 5 --- .../src/execution-target-sandbox.test.ts | 8 ++ .../adapter-utils/src/execution-target.ts | 17 +++- .../__tests__/agent-auth-middleware.test.ts | 93 ++++++++++++++++++- server/src/middleware/auth.ts | 47 ++++++++-- tests/e2e/playwright.config.ts | 4 + .../task-chat/task-chat-adapter.test.ts | 15 +++ 6 files changed, 170 insertions(+), 14 deletions(-) diff --git a/packages/adapter-utils/src/execution-target-sandbox.test.ts b/packages/adapter-utils/src/execution-target-sandbox.test.ts index 002e418d288..3e41253f530 100644 --- a/packages/adapter-utils/src/execution-target-sandbox.test.ts +++ b/packages/adapter-utils/src/execution-target-sandbox.test.ts @@ -15,6 +15,7 @@ import { ensureAdapterExecutionTargetCommandResolvable, formatAdapterExecutionTimeoutErrorMessage, formatAdapterExecutionTimeoutStartLogLine, + postedIssueCommentLogMarker, resolveAdapterExecutionTargetTimeout, resolveAdapterExecutionTargetTimeoutSec, runAdapterExecutionTargetProcess, @@ -75,6 +76,13 @@ function createRecordingTraceContext(): { describe("sandbox adapter execution targets", () => { const cleanupDirs: string[] = []; + it("records successful issue comment ids for attribution recovery", () => { + expect(postedIssueCommentLogMarker("POST", "/api/issues/issue-1/comments", 201, '{"id":"comment-1"}')) + .toBe("comment id: comment-1\n"); + expect(postedIssueCommentLogMarker("POST", "/api/issues/issue-1/comments", 401, '{"id":"comment-1"}')) + .toBeNull(); + }); + afterEach(async () => { vi.unstubAllEnvs(); while (cleanupDirs.length > 0) { diff --git a/packages/adapter-utils/src/execution-target.ts b/packages/adapter-utils/src/execution-target.ts index ce575787baf..dcac81f41b2 100644 --- a/packages/adapter-utils/src/execution-target.ts +++ b/packages/adapter-utils/src/execution-target.ts @@ -56,6 +56,18 @@ import type { LocalProcessSandboxOptions } from "./local-process-sandbox.js"; export type { RuntimeProgressSink } from "./runtime-progress.js"; +export function postedIssueCommentLogMarker(method: string, requestPath: string, status: number, body: string) { + if (method !== "POST" || !/^\/api\/issues\/[^/]+\/comments$/.test(requestPath) || status < 200 || status >= 300) { + return null; + } + try { + const parsed = JSON.parse(body) as { id?: unknown }; + return typeof parsed.id === "string" && parsed.id.length > 0 ? `comment id: ${parsed.id}\n` : null; + } catch { + return null; + } +} + export type AdapterWorkspaceRealizationMode = "copy" | "in_place"; export interface AdapterWorkspacePathAlias { @@ -2303,10 +2315,13 @@ export async function startAdapterExecutionTargetPaperclipBridge(input: { `[paperclip] Bridge proxy response ${response.status} for ${method} ${request.path}${request.query ? `?${request.query}` : ""}\n`, ); } + const responseBody = await readBridgeForwardResponseBody(response, maxBodyBytes); + const commentMarker = postedIssueCommentLogMarker(method, request.path, response.status, responseBody); + if (commentMarker) await onLog("stdout", commentMarker); return { status: response.status, headers: buildBridgeResponseHeaders(response), - body: await readBridgeForwardResponseBody(response, maxBodyBytes), + body: responseBody, }; }, }); diff --git a/server/src/__tests__/agent-auth-middleware.test.ts b/server/src/__tests__/agent-auth-middleware.test.ts index ff27e685b77..231ad00c9a5 100644 --- a/server/src/__tests__/agent-auth-middleware.test.ts +++ b/server/src/__tests__/agent-auth-middleware.test.ts @@ -90,12 +90,12 @@ function createDbState(input: { return { db, activity }; } -function createApp(db: any) { +function createApp(db: any, deploymentMode: "authenticated" | "local_trusted" = "authenticated") { const app = express(); app.use(express.json()); app.use( actorMiddleware(db, { - deploymentMode: "authenticated", + deploymentMode, resolveSession: async () => null, }), ); @@ -124,6 +124,7 @@ function craftAgentJwtWithoutResponsibleClaim(input: { companyId: string; adapterType: string; runId: string; + expiresInSeconds?: number; }) { const now = Math.floor(Date.now() / 1000); const header = { alg: "HS256", typ: "JWT" }; @@ -133,7 +134,7 @@ function craftAgentJwtWithoutResponsibleClaim(input: { adapter_type: input.adapterType, run_id: input.runId, iat: now, - exp: now + 3600, + exp: now + (input.expiresInSeconds ?? 3600), iss: "paperclip", aud: "paperclip-api", }; @@ -171,6 +172,92 @@ describe("agent auth middleware", () => { else process.env.PAPERCLIP_INSTANCE_ID = originalInstanceId; }); + it("keeps header-less local requests as the implicit board actor with their run id", async () => { + const runId = randomUUID(); + const { db } = createDbState({ agent: { id: randomUUID(), companyId: randomUUID() } }); + + const res = await request(createApp(db, "local_trusted")) + .get("/actor") + .set("X-Paperclip-Run-Id", runId); + + expect(res.status).toBe(200); + expect(res.body).toMatchObject({ type: "board", userId: "local-board", runId }); + }); + + it.each([ + ["empty bearer token", "Bearer ", "Empty bearer token"], + ["unverified token", "Bearer not-a-token", "Agent token did not verify"], + ])("rejects %s instead of retaining the implicit local-board actor", async (_label, authorization, error) => { + const { db } = createDbState({ agent: { id: randomUUID(), companyId: randomUUID() } }); + let commentWrites = 0; + const app = createApp(db, "local_trusted"); + app.post("/comments", (_req, res) => { + commentWrites += 1; + res.status(201).json({ ok: true }); + }); + + const res = await request(app).post("/comments").set("Authorization", authorization).send({ body: "reply" }); + + expect(res.status).toBe(401); + expect(res.body.error).toContain(error); + expect(commentWrites).toBe(0); + }); + + it.each([ + ["terminated", "Agent is terminated"], + ["pending_approval", "Agent is pending approval"], + ])("rejects a %s agent JWT instead of retaining local-board", async (status, error) => { + const companyId = randomUUID(); + const agentId = randomUUID(); + const runId = randomUUID(); + const { db } = createDbState({ agent: { id: agentId, companyId, status } }); + const token = createLocalAgentJwt(agentId, companyId, "codex_local", runId, "user-1"); + + const res = await request(createApp(db, "local_trusted")) + .get("/actor") + .set("Authorization", `Bearer ${token}`); + + expect(res.status).toBe(401); + expect(res.body.error).toContain(error); + }); + + it("rejects an agent JWT when the agent record belongs to another company", async () => { + const companyId = randomUUID(); + const agentId = randomUUID(); + const runId = randomUUID(); + const { db } = createDbState({ agent: { id: agentId, companyId: randomUUID() } }); + const token = createLocalAgentJwt(agentId, companyId, "codex_local", runId, "user-1"); + + const res = await request(createApp(db, "local_trusted")) + .get("/actor") + .set("Authorization", `Bearer ${token}`); + + expect(res.status).toBe(401); + expect(res.body.error).toContain("missing or belongs to another company"); + }); + + it("reports an expired agent JWT specifically", async () => { + const companyId = randomUUID(); + const agentId = randomUUID(); + const runId = randomUUID(); + const { db } = createDbState({ agent: { id: agentId, companyId } }); + const token = craftAgentJwtWithoutResponsibleClaim({ + secret: process.env.PAPERCLIP_AGENT_JWT_SECRET!, + agentId, + companyId, + adapterType: "codex_local", + runId, + expiresInSeconds: -1, + }); + + const res = await request(createApp(db, "local_trusted")) + .get("/actor") + .set("Authorization", `Bearer ${token}`); + + expect(res.status).toBe(401); + expect(res.body.error).toContain("Expired agent token"); + }); + it("uses the signed responsible_user_id claim and keeps the signed run id authoritative", async () => { const companyId = randomUUID(); const agentId = randomUUID(); diff --git a/server/src/middleware/auth.ts b/server/src/middleware/auth.ts index 01df6fadf50..c073f14406f 100644 --- a/server/src/middleware/auth.ts +++ b/server/src/middleware/auth.ts @@ -46,7 +46,7 @@ function pruneCloudTenantWriteDebounce( } import { instanceSettingsService } from "../services/instance-settings.js"; import { ensureHumanRoleDefaultGrants } from "../services/principal-access-compatibility.js"; -import { forbidden, unprocessable } from "../errors.js"; +import { forbidden, unauthorized, unprocessable } from "../errors.js"; export { isCloudManagedInstance } from "../services/cloud-instance.js"; @@ -58,6 +58,20 @@ function normalizeOptionalString(value: string | null | undefined) { return value?.trim() || null; } +function invalidAgentTokenMessage(token: string) { + try { + const payload = JSON.parse(Buffer.from(token.split(".")[1] ?? "", "base64url").toString("utf8")) as { + exp?: unknown; + }; + if (typeof payload.exp === "number" && payload.exp <= Math.floor(Date.now() / 1000)) { + return "Expired agent token; obtain fresh credentials and retry"; + } + } catch { + // Malformed and incorrectly signed tokens share the generic failure below. + } + return "Agent token did not verify; obtain fresh credentials and retry"; +} + async function resolveLegacyRunResponsibleUserId( db: Db, input: { companyId: string; agentId: string; runId: string }, @@ -207,7 +221,8 @@ export function actorMiddleware(db: Db, opts: ActorMiddlewareOptions): RequestHa const runIdHeader = req.header("x-paperclip-run-id"); const authHeader = req.header("authorization"); - if (!authHeader?.toLowerCase().startsWith("bearer ")) { + const hasBearerCredentials = /^bearer(?:\s|$)/i.test(authHeader ?? ""); + if (!hasBearerCredentials) { if (opts.deploymentMode === "authenticated" && opts.resolveSession) { const cloudTenantActor = await resolveCloudTenantActor(db, req); if (cloudTenantActor) { @@ -259,9 +274,9 @@ export function actorMiddleware(db: Db, opts: ActorMiddlewareOptions): RequestHa return; } - const token = authHeader.slice("bearer ".length).trim(); + const token = authHeader!.slice("bearer".length).trim(); if (!token) { - next(); + next(unauthorized("Empty bearer token; provide valid agent credentials and retry")); return; } @@ -297,7 +312,7 @@ export function actorMiddleware(db: Db, opts: ActorMiddlewareOptions): RequestHa if (!key) { const claims = verifyLocalAgentJwt(token); if (!claims) { - next(); + next(unauthorized(invalidAgentTokenMessage(token))); return; } @@ -308,12 +323,16 @@ export function actorMiddleware(db: Db, opts: ActorMiddlewareOptions): RequestHa .then((rows) => rows[0] ?? null); if (!agentRecord || agentRecord.companyId !== claims.company_id) { - next(); + next(unauthorized("Agent record is missing or belongs to another company; obtain fresh credentials and retry")); return; } - if (agentRecord.status === "terminated" || agentRecord.status === "pending_approval") { - next(); + if (agentRecord.status === "terminated") { + next(unauthorized("Agent is terminated and cannot authenticate")); + return; + } + if (agentRecord.status === "pending_approval") { + next(unauthorized("Agent is pending approval and cannot authenticate")); return; } @@ -375,8 +394,16 @@ export function actorMiddleware(db: Db, opts: ActorMiddlewareOptions): RequestHa .where(eq(agents.id, key.agentId)) .then((rows) => rows[0] ?? null); - if (!agentRecord || agentRecord.status === "terminated" || agentRecord.status === "pending_approval") { - next(); + if (!agentRecord || agentRecord.companyId !== key.companyId) { + next(unauthorized("Agent record is missing or belongs to another company; obtain fresh credentials and retry")); + return; + } + if (agentRecord.status === "terminated") { + next(unauthorized("Agent is terminated and cannot authenticate")); + return; + } + if (agentRecord.status === "pending_approval") { + next(unauthorized("Agent is pending approval and cannot authenticate")); return; } diff --git a/tests/e2e/playwright.config.ts b/tests/e2e/playwright.config.ts index e5d6c2e7119..8fc3beae1fa 100644 --- a/tests/e2e/playwright.config.ts +++ b/tests/e2e/playwright.config.ts @@ -19,6 +19,10 @@ const PLAYWRIGHT_CHANNEL = process.env.PAPERCLIP_PLAYWRIGHT_CHANNEL; process.env.PAPERCLIP_HOME = PAPERCLIP_HOME; process.env.PAPERCLIP_CONFIG = PAPERCLIP_CONFIG; +// Specs that mint agent JWTs in-process (via createLocalAgentJwt) must derive +// the same per-instance signing key as the webServer, or verification fails +// with a 401 instead of authenticating as the agent. +process.env.PAPERCLIP_INSTANCE_ID = PAPERCLIP_INSTANCE_ID; process.env.PAPERCLIP_AGENT_JWT_SECRET = PAPERCLIP_AGENT_JWT_SECRET; process.env.PAPERCLIP_DECISION_SIGNING_SECRET = PAPERCLIP_DECISION_SIGNING_SECRET; process.env.PAPERCLIP_TOOL_ACTION_SIGNING_SECRET = PAPERCLIP_TOOL_ACTION_SIGNING_SECRET; diff --git a/ui/src/components/task-chat/task-chat-adapter.test.ts b/ui/src/components/task-chat/task-chat-adapter.test.ts index 27e0ad3b536..3596177cc1a 100644 --- a/ui/src/components/task-chat/task-chat-adapter.test.ts +++ b/ui/src/components/task-chat/task-chat-adapter.test.ts @@ -3,6 +3,21 @@ import type { IssueChatComment } from "@/lib/issue-chat-messages"; import { commentsToTaskChatItems } from "./task-chat-adapter"; describe("commentsToTaskChatItems", () => { + it("classifies a recovered local-board comment as an agent bubble", () => { + const items = commentsToTaskChatItems([{ + id: "c-recovered", + body: "Recovered agent reply.", + authorType: "user", + authorUserId: "local-board", + authorAgentId: null, + derivedAuthorAgentId: "agent-1", + createdAt: "2026-08-07T09:00:00.000Z", + } as unknown as IssueChatComment]); + + expect(items).toHaveLength(1); + expect(items[0]).toMatchObject({ kind: "message", author: "agent" }); + }); + it("never tags posted comments interstitial — the run's final reply keeps its bubble", () => { const comments = [ {