Fix: strip client-supplied X-Agent-ID/X-Task-ID to close agent impersonation#4313
Open
CatCatUncle wants to merge 2 commits into
Open
Fix: strip client-supplied X-Agent-ID/X-Task-ID to close agent impersonation#4313CatCatUncle wants to merge 2 commits into
CatCatUncle wants to merge 2 commits into
Conversation
…onation
A workspace member authenticating with a normal JWT/PAT could assume an
agent's identity by attaching X-Agent-ID + X-Task-ID headers. resolveActor's
legacy fallback only checked that the task belonged to the agent, but BOTH
ids are returned by member-readable endpoints (GET /api/issues/{id}/task-runs,
GET /api/agents/{id}/tasks). A member could read a real (agent_id, task_id)
pair, replay it as headers, satisfy task.AgentID == agentID, and be resolved
as actorType="agent" — authoring comments/issues as that agent and bypassing
the private-agent gate. Requiring "both headers present" did not help because
the task id is just as observable as the agent id.
Fix: Auth and DaemonAuth middlewares now strip any client-supplied
X-Agent-ID / X-Task-ID on entry, alongside the existing X-Actor-Source strip.
The only path that sets them is the server-controlled mat_ task-token branch,
so agent identity now flows exclusively from the unforgeable task token
(MUL-2600). resolveActor's fallback is kept (now unreachable from the network)
so in-process callers and handler unit tests can still assert agent-actor
behaviour by setting the headers directly; its comment is corrected to note it
is not a security boundary.
Tests: TestAuth_StripsForgedAgentIdentityHeaders and the DaemonAuth mirror
assert the forged headers never reach the handler.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@unclecatai is attempting to deploy a commit to the IndexLabs Team on Vercel. A member of the Team first needs to authorize it. |
…headers The comment-trigger integration tests posed as an agent by setting X-Agent-ID / X-Task-ID on a member-authenticated request. That is the exact impersonation path this PR closes, so once the Auth middleware strips those headers the actor resolves as a member and the agent-thread trigger assertions fail (TestCommentTriggerOnComment, TestCommentTriggerAtAllSuppression). Fix the helper to authenticate the way production agents do: mint a real mat_ task token bound to (agent, task, workspace, user) and present it as the bearer. The middleware re-stamps the agent/task headers from the token row, so the tests exercise genuine agent auth instead of the forgeable fallback. No header is set by the client anymore. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem (security)
A workspace member authenticating with a normal JWT/PAT can assume any agent's identity within their workspace, with no extra privileges.
resolveActor(server/internal/handler/handler.go) has a fallback: if the request carriesX-Agent-ID+X-Task-IDandtask.AgentID == agentID, the caller is resolved asactorType="agent". The only "proof" required is knowledge of a valid(agent_id, task_id)pair — but both ids are returned by member-readable endpoints:GET /api/issues/{id}/task-runs(ListTasksByIssue)GET /api/agents/{id}/tasks(ListAgentTasks)Both return
AgentTaskResponse{ id, agent_id, ... }.Exploit
(agent_id, task_id)pair.X-Agent-ID/X-Task-IDheaders on your own authenticated request.task.AgentID == agentIDpasses trivially (it's a real pair).actorType="agent"— you can author comments/issues as that agent, and bypass the private-agent gate (agent-to-agent flows trustactorType=="agent").The existing code comment claims requiring both headers "closes the impersonation path" — it doesn't, because the task id is just as observable as the agent id.
Fix
AuthandDaemonAuthmiddlewares now strip any client-suppliedX-Agent-ID/X-Task-IDon entry, right alongside the existingX-Actor-Sourcestrip. The only code that sets these headers is the server-controlledmat_task-token branch (which re-stamps them from the token row after stripping). So agent identity now flows exclusively from the unforgeable task token (MUL-2600).resolveActor's fallback is intentionally kept (it is now unreachable from the network, since every route that reaches it goes through one of the two stripping middlewares) so in-process callers and handler unit tests can still assert agent-actor behaviour by setting the headers directly. Its doc comment is corrected to state it is not a security boundary — the middleware strip is.Changed files
server/internal/middleware/auth.go— stripX-Agent-ID/X-Task-IDserver/internal/middleware/daemon_auth.go— same, for uniformity/defense-in-depthserver/internal/handler/handler.go— correctedresolveActordoc comment (behaviour unchanged)Tests
TestAuth_StripsForgedAgentIdentityHeaders— a valid member JWT + forgedX-Agent-ID/X-Task-IDreaches the handler with both headers cleared.TestDaemonAuth_StripsForgedAgentIdentityHeaders— same invariant on the daemon path (Redis-gated, mirrors the existingStripsClientSuppliedActorSourcetest).go build ./...,go vet ./internal/middleware/ ./internal/handler/, andgo test ./internal/middleware/ -run 'TestAuth|TestDaemonAuth'all pass; existing handler tests that set these headers directly (squad/subscriber/agent_env) are unaffected sinceresolveActor's fallback is preserved.🤖 Generated with Claude Code