fix(auth): scope in-session agent MCP calls to the task owner#1937
Merged
Conversation
📝 WalkthroughWalkthroughIn-session MCP streams now resolve identity from the execution task’s workspace owner, attach that identity before dispatch, and fail closed when resolution fails. Authentication services, lifecycle wiring, backend registration, authorization tests, and guidance are updated accordingly. ChangesMCP identity scoping
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AgentExecution
participant StreamManager
participant MCPResolver
participant MCPHandler
AgentExecution->>StreamManager: Open MCP stream with TaskID
StreamManager->>MCPResolver: Resolve task owner identity
MCPResolver-->>StreamManager: Return scoped context or error
StreamManager->>MCPHandler: Dispatch request with scoped context
MCPHandler-->>StreamManager: Return MCP response or authorization error
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The MCP tools an agent gets automatically inside its own session are relayed over the agent's WebSocket stream, which carries no credential. The backend dispatched them on the raw stream context, so the task service saw no identity — its signal for an internal caller (event bus, pollers, office schedulers) — and served the request unscoped. An agent that supplied another user's task_id or workflow_id was given their data, violating the rule that knowing an ID must not grant access. Resolve the owning user from the stream's own AgentExecution (task -> workspace -> owner) and attach that real stored identity before dispatch, so the existing authorize* checks apply exactly as they do on the PAT-authenticated /mcp endpoint. The owning task never comes from the agent-supplied payload: honoring a payload session_id would let an agent name another user's session and inherit their identity. Tool handlers are unchanged. Auth disabled stays unscoped, unclaimed pre-auth workspaces stay visible to everyone, and a lookup failure denies the dispatch rather than falling back to full access. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f6d48dcb05
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
jcfs
force-pushed
the
feature/scope-in-session-age-14g
branch
from
July 24, 2026 23:16
f6d48dc to
c6c4a95
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/backend/internal/mcp/scope/scope.go`:
- Around line 95-103: The ownership-resolution flow in the relevant scope
resolver must fail closed when the task or workspace lookup returns nil without
an error: return an ownership-resolution error instead of the original context.
Preserve the no-op behavior only for an existing task with no workspace and an
existing workspace with no owner, and add tests covering missing task and
missing workspace rows.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4bafe1d8-a2de-493a-bc16-8ce1a7c234b7
📒 Files selected for processing (13)
apps/backend/AGENTS.mdapps/backend/internal/agent/runtime/lifecycle/manager.goapps/backend/internal/agent/runtime/lifecycle/mcp_identity.goapps/backend/internal/agent/runtime/lifecycle/mcp_identity_test.goapps/backend/internal/agent/runtime/lifecycle/streams.goapps/backend/internal/auth/service_credentials.goapps/backend/internal/auth/service_identity_test.goapps/backend/internal/backendapp/helpers.goapps/backend/internal/mcp/handlers/mcp_identity_scope_test.goapps/backend/internal/mcp/scope/scope.goapps/backend/internal/mcp/scope/scope_test.goapps/backend/internal/task/service/service_access.godocs/decisions/2026-07-24-opt-in-authentication.md
Review follow-up. Three paths still returned an identity-free context under enforced auth, and "no identity" is exactly what the task service reads as an internal caller and serves unscoped: - Unowned workspace. CreateWorkspace only stamps an owner for scoped callers, so internal callers keep producing owner_id='' rows after setup. Those streams got full cross-user access. Scope them to a sentinel user ID no account can hold: the service's "empty owner_id or my own ID" rule then limits them to the unowned rows that are public by the compatibility contract, and nothing else. - Task with no workspace. Same treatment, same reasoning. - Missing task or workspace row. A lookup that reports not-found as (nil, nil) fell through to unscoped; treat it as a resolution failure. Also deny, rather than fabricate a member identity, when the owner's account is gone or disabled. Disabling a user revokes their sessions and PATs, so a still-running agent session was the one remaining way into their workspace. Co-Authored-By: Claude Opus 5 (1M context) <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
1 participant
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.
Follow-up to the #1930 review. #1930 has since merged, so this is rebased onto and targets
main.The gap
There are two ways an agent reaches kandev's MCP, and under per-user auth they behaved differently.
External
/mcpendpoint — already correct, untouched. The agent authenticates with a PAT; the global auth middleware mirrors identity ontoc.Request.Context(),gin.WrapHhands the request to mcp-go, and its transports derive the tool-handler context fromr.Context(). Identity reaches the task service, soauthorize*returns NotFound for another user's task.In-session injected MCP — the leak. The tools an agent gets automatically inside its own session are served by agentctl and relayed back to the backend over the agent's WS stream. The backend dispatched them at
agent.godispatchMCPRequest→mcpHandler.Dispatch(ctx, &msg)using the raw stream context, which carries no credential. Enforcement is gated on ctx identity (callerScopeinservice_access.go), and "no identity" is the service's signal for an internal caller (event bus, pollers, office schedulers) → unscoped, full access.So an in-session agent that supplied another user's
task_id/workflow_idwas served that data — violating the explicit "knowing another user's ID must not grant access" requirement.This was reproduced first, as a genuine red test. Against the pre-fix dispatch,
TestInSessionMCPListTasksDeniesForeignWorkflowandTestInSessionMCPConversationDeniesForeignTaskboth failed withexpected: "error", actual: "response"— user A's agent was served both user B's task list and user B's conversation message content.The fix
New
internal/mcp/scoperesolves the stream's task → workspace → owner and attaches that user's real stored identity to the dispatch context, so the existingauthorize*checks apply exactly as on the PAT path. Wired at the lifecycle/gateway seam (Manager.SetMCPIdentityScoper, applied inStreamManager.mcpHandlerFor), not inside the agentctl client package.Tool handlers are unchanged.
Design points worth reviewing:
AgentExecution, never from the request payload. The payload'ssession_id/task_idare agent-controlled — trusting them would let an agent name another user's session and inherit their identity, turning this fix into a privilege escalation. Pinned byTestMCPHandlerForIgnoresPayloadSessionIDandTestInSessionMCPIgnoresPayloadSessionForScoping.auth.Service.IdentityForUserresolves the owner's stored account (member/admin role, active-status checked). Synthetic reads as unscoped, so it would silently reinstate the bug.Scopereturns an error and the dispatch is denied — returning an unscoped context would grant every user's data. If the owner's account is missing/disabled, it still scopes to the owner ID but drops to the least-privileged role rather than falling back to unscoped./mcppath and any other credentialed caller.owner_id) stay visible to everyone, matching the task service's own rule; internal non-user callers (pollers, event bus) never pass through the scoper, so they stay unscoped as intended.Both agent-stream dispatch sites are covered — the ACP updates stream and the passthrough MCP stream.
Tests
internal/mcp/handlers/mcp_identity_scope_test.go— integration over the real handler→service path with two users on in-memory sqlite: A-cannot-read-B (list_tasks+get_task_conversation), A-can-still-read-own, payload-spoofing denied, auth-disabled stays unscoped.internal/mcp/scope/scope_test.go— resolver units: real owner identity, auth-disabled no-op, unowned/no-workspace no-ops, existing identity preserved, fail-closed on both lookup errors, least-privilege fallback.internal/agent/runtime/lifecycle/mcp_identity_test.go— wrapper scopes to the execution's task, ignores payload IDs, denies on scoping failure, passes through unwrapped without a scoper or task ID.internal/auth/service_identity_test.go—IdentityForUser: real non-synthetic identity, correct role, false when auth disabled / user unknown / user disabled.Verification
golangci-lint run ./... --new-from-rev=919a39e49(main, post-feat(auth): opt-in authentication & multi-user segregation #1930-merge) → 0 issuesgo vet ./...→ cleango testoninternal/mcp/... internal/auth/... internal/agent/runtime/... internal/backendapp/ internal/orchestrator/ internal/gateway/...→ all passgo test -raceon./internal/agent/runtime/lifecycle/...→ pass, no goleak/race regressionsAll of the above were re-run after the rebase onto
main. The rebase replayed only this branch's own commit (git rebase --onto origin/main, since #1930 was squash-merged) and applied with zero conflicts.internal/task/servicehas 8 failing tests (TestCreateDirectoryRejectsInvalidOrExistingChild, 7 ×TestServiceInitializeLocalRepository*). These are pre-existing and unrelated — environment/filesystem related (all"parent directory cannot be accessed"), confirmed failing identically at the base commit in a separate worktree both before and after the rebase, and this commit has a zero-line diff in that package's test files.Docs updated where they stated the old (leaky) contract:
apps/backend/AGENTS.md, the auth ADR, and theservice_access.goheader comment all previously listed "MCP stream" among the unscoped internal callers.🤖 Generated with Claude Code