mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-06 10:48:12 +02:00
fix(server): reject invalid agent credentials instead of downgrading to the local user actor (#11589)
<!-- Write all pull request text in Simplified Technical English (ASD-STE100): short sentences, one instruction per sentence, simple approved vocabulary, and the active voice. --> ## 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: <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 <noreply@paperclip.ing> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
6 files changed
+170
-14
No files matched your search
@@ -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) {
|
||||
|
||||
@@ -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,
|
||||
};
|
||||
},
|
||||
});
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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 = [
|
||||
{
|
||||
|
||||
Reference in new issue
Block a user