fix(auth): forbid board-key fallback on run-scoped agent requests#9733
fix(auth): forbid board-key fallback on run-scoped agent requests#9733mvanhorn wants to merge 2 commits into
Conversation
A request carrying the run-scoped header x-paperclip-run-id could authenticate as a board actor: actorMiddleware resolved a board API key (source: board_key) before trying agent credentials, so a run-scoped request presenting a board token was attributed board authority instead of being rejected. This let agent execution silently escalate into board authority. Skip the board API key branch when x-paperclip-run-id is present, so a run-scoped request can only authenticate with agent credentials (agent API key or local agent JWT). Non-run-scoped board-key behavior is unchanged. Adds regression tests covering board-token rejection on a run-scoped request, unchanged non-run-scoped board resolution, and valid agent-JWT resolution with the header set.
|
✅ All checks passing — ready for Greptile review and maintainer approval. — commitperclip |
Greptile SummaryThis PR fixes a privilege-boundary bug in
Confidence Score: 5/5Safe to merge — the change is a one-line guard that only affects requests presenting both a board API key and The fix is surgical: a single No files require special attention — both changed files are straightforward and well-tested. Important Files Changed
Reviews (2): Last reviewed commit: "chore: re-run PR quality gates" | Re-trigger Greptile |
| req.actor = { | ||
| type: "board", | ||
| userId: boardKey.userId, | ||
| userName: access.user?.name ?? null, | ||
| userEmail: access.user?.email ?? null, | ||
| companyIds: access.companyIds, | ||
| memberships: access.memberships, | ||
| isInstanceAdmin: access.isInstanceAdmin, | ||
| keyId: boardKey.id, | ||
| runId: runIdHeader || undefined, | ||
| source: "board_key", | ||
| }; |
There was a problem hiding this comment.
Inside the
if (!runIdHeader) block, runIdHeader is guaranteed to be falsy, so runIdHeader || undefined always evaluates to undefined. The field is effectively dead code here — it can be removed or replaced with a literal undefined.
| req.actor = { | |
| type: "board", | |
| userId: boardKey.userId, | |
| userName: access.user?.name ?? null, | |
| userEmail: access.user?.email ?? null, | |
| companyIds: access.companyIds, | |
| memberships: access.memberships, | |
| isInstanceAdmin: access.isInstanceAdmin, | |
| keyId: boardKey.id, | |
| runId: runIdHeader || undefined, | |
| source: "board_key", | |
| }; | |
| req.actor = { | |
| type: "board", | |
| userId: boardKey.userId, | |
| userName: access.user?.name ?? null, | |
| userEmail: access.user?.email ?? null, | |
| companyIds: access.companyIds, | |
| memberships: access.memberships, | |
| isInstanceAdmin: access.isInstanceAdmin, | |
| keyId: boardKey.id, | |
| source: "board_key", | |
| }; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: server/src/middleware/auth.ts
Line: 235-246
Comment:
Inside the `if (!runIdHeader)` block, `runIdHeader` is guaranteed to be falsy, so `runIdHeader || undefined` always evaluates to `undefined`. The field is effectively dead code here — it can be removed or replaced with a literal `undefined`.
```suggestion
req.actor = {
type: "board",
userId: boardKey.userId,
userName: access.user?.name ?? null,
userEmail: access.user?.email ?? null,
companyIds: access.companyIds,
memberships: access.memberships,
isInstanceAdmin: access.isInstanceAdmin,
keyId: boardKey.id,
source: "board_key",
};
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Trigger commitperclip re-review after adding the dedup-search affirmation and public issue/PR links to the PR description.
Thinking Path
Linked Issues or Issue Description
Fixes: #8020
Related / superseded PR: #8016 — a prior attempt on this issue that diverged into large, unrelated repository churn and was closed. This PR supersedes it by shipping only the focused auth-boundary change plus its regression tests, with no unrelated changes. A search of the open and closed PR list found no other PRs touching this area.
What Changed
server/src/middleware/auth.ts: guarded the board API key branch (theboardAuth.findBoardApiKeyByTokenlookup that resolves atype: "board"/source: "board_key"actor) behindif (!runIdHeader). Whenx-paperclip-run-idis present the branch is skipped and control falls through to the existing agent API key lookup and local agent JWT path; a run-scoped request presenting only a board token resolves no actor and is rejected. No other logic was reordered.server/src/__tests__/agent-auth-middleware.test.ts: added regression tests for the run-scoped bearer path — a board token withX-Paperclip-Run-Idset does not resolve a board actor, a board token without the header still resolves the board actor (source: "board_key") as before, and a valid agent JWT with the header resolves an agent actor and never a board one.Verification
Run from the repo root:
The core regression test (
does not resolve a board key as a board actor for a run-scoped request) fails onmasterand passes with this change.Risks
x-paperclip-run-idis now rejected instead of being attributed board authority — which is the intended security fix.Model Used
AI was used for assistance.
Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template#NNN/github.com/paperclipai/paperclipURLs)docs/...,fix/...) and contains no internal Paperclip ticket id or instance-derived details