Skip to content

Commit 1038780

Browse files
jcfsKandev Agentclaude
authored
fix(auth): scope in-session agent MCP calls to the task owner (#1937)
* fix(auth): scope in-session agent MCP calls to the task owner 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> * fix(auth): never leave in-session MCP dispatch unscoped 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> --------- Co-authored-by: Kandev Agent <agent@kandev.dev> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 919a39e commit 1038780

13 files changed

Lines changed: 1157 additions & 7 deletions

File tree

apps/backend/AGENTS.md

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -168,7 +168,8 @@ and can leak ACP subprocesses.
168168
**Remote SSH executor platforms:** Treat supported remote OS/arch values as an end-to-end contract. Platform probe/normalization, lifecycle support checks, agentctl helper resolution, platform default shell, SSH readiness endpoints, frontend response types, and tests must stay aligned. Preserve raw unsupported platform details in user-facing errors, but use normalized values for supported-platform matching. Keep shell defaults platform-aware: Darwin defaults to `zsh`, Linux defaults to `bash`, unless an explicit shell is saved.
169169

170170
**Opt-in authentication & per-user scoping** (`internal/auth/`): auth is OFF by default; the global middleware (`auth/httpmw`, installed after CORS in `backendapp.buildHTTPServer`) then injects a synthetic admin identity for the pre-auth single user, so behavior is unchanged. Enablement is the `features.auth` runtime flag (`KANDEV_FEATURES_AUTH`, Settings > System > Feature Toggles) — the auth service derives its mode from `cfg.Features.Auth` (`disabled` / `setup` = flag-on-no-admin / `enabled` = flag-on-admin-exists); there is no separate `auth.mode` setting. When enabled, requests authenticate via a `kandev_session` cookie (opaque token, SHA-256 at rest, DB-backed) or a `kandev_pat_*` bearer token. Scoping rules:
171-
- **Identity travels in the request context** (`authn.IdentityFromContext`). No identity = internal caller (pollers, event bus, MCP stream) = unscoped. Synthetic identity = auth disabled = unscoped.
171+
- **Identity travels in the request context** (`authn.IdentityFromContext`). No identity = internal caller (pollers, event bus, office schedulers) = unscoped. Synthetic identity = auth disabled = unscoped.
172+
- **In-session agent MCP is scoped to the task owner.** The MCP tools an agent gets inside its own session are relayed over the agent's WebSocket stream, which carries no credential of its own. `internal/mcp/scope` resolves the stream's task → workspace → owner and attaches that user's real identity before dispatch (`lifecycle.Manager.SetMCPIdentityScoper`), so the same `authorize*` checks apply as for the PAT-authenticated `/mcp` endpoint. The owning task comes from the `AgentExecution`, never from the agent-supplied payload — do not "improve" this by reading `session_id`/`task_id` out of the request. Tool handlers stay identity-agnostic. Under enforced auth every dispatch is scoped to *somebody*: a resolvable active owner gets their identity; an unowned workspace gets a sentinel user ID that reaches unowned rows only; anything unresolvable (missing task/workspace row, or an owner whose account was deleted or disabled) is **denied**. Never return an identity-free context from this path — the task service reads that as an internal caller and grants everything.
172173
- **Workspaces are per-user** (`workspaces.owner_id`); the task service filters/denies at the service layer with `*NotFound` sentinels (no existence leak). Unowned rows (`owner_id=''`) stay visible until the setup wizard claims them for the admin.
173174
- **New user-facing service entry points must apply scoping** — call the `authorize*` helpers in `task/service/service_access.go` (or the same pattern) when adding routes that read or mutate workspace-scoped data.
174175
- **WS**: clients carry their identity; dispatched actions and subscriptions are scoped; workspace-carrying events route via `Hub.BroadcastToWorkspace`. A new `hub.Broadcast` (global) call site needs a `//ws:global` justification comment.

apps/backend/internal/agent/runtime/lifecycle/manager.go

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -349,6 +349,22 @@ func (m *Manager) SetMCPHandler(handler agentctl.MCPHandler) {
349349
m.streamManager.mcpHandler = handler
350350
}
351351

352+
// SetMCPIdentityScoper installs the per-user scoping hook for in-session MCP
353+
// tool calls.
354+
//
355+
// Unlike the external /mcp endpoint — where the agent presents a personal
356+
// access token and the auth middleware resolves the identity — MCP requests
357+
// relayed over an agent's own stream carry no credential. Without this hook
358+
// they reach the task service with no identity, which that service reads as an
359+
// internal caller and serves unscoped, so an agent supplying another user's
360+
// task_id or workflow_id would be given their data.
361+
//
362+
// Set once during startup wiring, before agents start making MCP calls. Leave
363+
// unset to keep dispatch unscoped (single-user instances).
364+
func (m *Manager) SetMCPIdentityScoper(scoper MCPIdentityScoper) {
365+
m.streamManager.mcpIdentityScoper = scoper
366+
}
367+
352368
// SetSessionAccessChecker installs the per-user session visibility check used
353369
// by GetOrEnsureExecution and EnsurePassthroughExecution. The checker must
354370
// return nil for contexts without a request identity (internal callers). Set
Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
package lifecycle
2+
3+
import (
4+
"context"
5+
6+
"go.uber.org/zap"
7+
8+
agentctl "github.com/kandev/kandev/internal/agent/runtime/agentctl"
9+
"github.com/kandev/kandev/internal/common/logger"
10+
ws "github.com/kandev/kandev/pkg/websocket"
11+
)
12+
13+
// MCPIdentityScoper attaches the identity of the user who owns taskID to ctx,
14+
// so the in-session MCP tools an agent gets automatically are authorized as
15+
// that user instead of running unscoped. Returning an error denies the
16+
// dispatch — see internal/mcp/scope for the production implementation and why
17+
// it fails closed rather than falling back to full access.
18+
type MCPIdentityScoper func(ctx context.Context, taskID string) (context.Context, error)
19+
20+
// taskScopedMCPHandler scopes every MCP request on one agent stream to the
21+
// owner of that stream's task.
22+
//
23+
// The task ID comes from the AgentExecution this stream belongs to, never from
24+
// the request payload: an agent controls its own payloads, so honoring a
25+
// payload session_id would let it name another user's session and inherit
26+
// their identity — turning the scoping fix into a privilege escalation.
27+
type taskScopedMCPHandler struct {
28+
inner agentctl.MCPHandler
29+
scope MCPIdentityScoper
30+
taskID string
31+
logger *logger.Logger
32+
}
33+
34+
func (h *taskScopedMCPHandler) Dispatch(ctx context.Context, msg *ws.Message) (*ws.Message, error) {
35+
scoped, err := h.scope(ctx, h.taskID)
36+
if err != nil {
37+
h.logger.Error("denying in-session MCP request: cannot resolve the task owner",
38+
zap.String("task_id", h.taskID),
39+
zap.String("action", msg.Action),
40+
zap.Error(err))
41+
return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeInternalError,
42+
"failed to resolve the session owner", nil)
43+
}
44+
return h.inner.Dispatch(scoped, msg)
45+
}
46+
47+
// mcpHandlerFor returns the MCP handler for one execution's stream, scoped to
48+
// the task's owning user when per-user scoping has been wired. Without a
49+
// scoper (tests, or the runtime tier used standalone) the handler is passed
50+
// through unchanged.
51+
func (sm *StreamManager) mcpHandlerFor(execution *AgentExecution) agentctl.MCPHandler {
52+
if sm.mcpHandler == nil || sm.mcpIdentityScoper == nil || execution.TaskID == "" {
53+
return sm.mcpHandler
54+
}
55+
return &taskScopedMCPHandler{
56+
inner: sm.mcpHandler,
57+
scope: sm.mcpIdentityScoper,
58+
taskID: execution.TaskID,
59+
logger: sm.logger,
60+
}
61+
}
Lines changed: 162 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,162 @@
1+
package lifecycle
2+
3+
import (
4+
"context"
5+
"encoding/json"
6+
"errors"
7+
"testing"
8+
9+
"github.com/kandev/kandev/internal/auth/authn"
10+
"github.com/kandev/kandev/internal/common/logger"
11+
ws "github.com/kandev/kandev/pkg/websocket"
12+
)
13+
14+
// recordingMCPHandler captures the context its dispatch ran under so tests can
15+
// assert which identity (if any) reached the tool handlers.
16+
type recordingMCPHandler struct {
17+
gotCtx context.Context
18+
calls int
19+
}
20+
21+
func (h *recordingMCPHandler) Dispatch(ctx context.Context, msg *ws.Message) (*ws.Message, error) {
22+
h.gotCtx = ctx
23+
h.calls++
24+
return ws.NewResponse(msg.ID, msg.Action, map[string]interface{}{"ok": true})
25+
}
26+
27+
func newMCPStreamManager(t *testing.T, inner *recordingMCPHandler, scoper MCPIdentityScoper) *StreamManager {
28+
t.Helper()
29+
log, err := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "json"})
30+
if err != nil {
31+
t.Fatalf("logger: %v", err)
32+
}
33+
sm := NewStreamManager(log, StreamCallbacks{}, inner, nil)
34+
sm.mcpIdentityScoper = scoper
35+
return sm
36+
}
37+
38+
func mcpRequest(t *testing.T, payload map[string]interface{}) *ws.Message {
39+
t.Helper()
40+
data, err := json.Marshal(payload)
41+
if err != nil {
42+
t.Fatalf("marshal payload: %v", err)
43+
}
44+
return &ws.Message{ID: "req-1", Type: ws.MessageTypeRequest, Action: "mcp.list_tasks", Payload: data}
45+
}
46+
47+
// TestMCPHandlerForScopesToExecutionTask is the core wiring assertion: the
48+
// identity handed to the tool handlers comes from the execution that owns the
49+
// stream.
50+
func TestMCPHandlerForScopesToExecutionTask(t *testing.T) {
51+
inner := &recordingMCPHandler{}
52+
var scopedTaskIDs []string
53+
sm := newMCPStreamManager(t, inner, func(ctx context.Context, taskID string) (context.Context, error) {
54+
scopedTaskIDs = append(scopedTaskIDs, taskID)
55+
return authn.WithIdentity(ctx, authn.Identity{UserID: "owner-of-" + taskID, Role: authn.RoleMember}), nil
56+
})
57+
58+
handler := sm.mcpHandlerFor(&AgentExecution{ID: "exec-1", TaskID: "task-a"})
59+
resp, err := handler.Dispatch(context.Background(), mcpRequest(t, map[string]interface{}{}))
60+
if err != nil {
61+
t.Fatalf("Dispatch: %v", err)
62+
}
63+
if resp.Type != ws.MessageTypeResponse {
64+
t.Fatalf("response type = %q, want response", resp.Type)
65+
}
66+
67+
if len(scopedTaskIDs) != 1 || scopedTaskIDs[0] != "task-a" {
68+
t.Fatalf("scoped task IDs = %v, want [task-a]", scopedTaskIDs)
69+
}
70+
identity, ok := authn.IdentityFromContext(inner.gotCtx)
71+
if !ok {
72+
t.Fatal("tool handlers received no identity")
73+
}
74+
if identity.UserID != "owner-of-task-a" {
75+
t.Errorf("UserID = %q, want owner-of-task-a", identity.UserID)
76+
}
77+
}
78+
79+
// TestMCPHandlerForIgnoresPayloadSessionID is the privilege-escalation pin. The
80+
// payload is agent-controlled, so scoping must key off the execution's task
81+
// even when the request names a different session or task.
82+
func TestMCPHandlerForIgnoresPayloadSessionID(t *testing.T) {
83+
inner := &recordingMCPHandler{}
84+
var scopedTaskIDs []string
85+
sm := newMCPStreamManager(t, inner, func(ctx context.Context, taskID string) (context.Context, error) {
86+
scopedTaskIDs = append(scopedTaskIDs, taskID)
87+
return ctx, nil
88+
})
89+
90+
handler := sm.mcpHandlerFor(&AgentExecution{ID: "exec-1", TaskID: "task-a"})
91+
_, err := handler.Dispatch(context.Background(), mcpRequest(t, map[string]interface{}{
92+
"session_id": "session-of-victim",
93+
"task_id": "task-victim",
94+
}))
95+
if err != nil {
96+
t.Fatalf("Dispatch: %v", err)
97+
}
98+
99+
if len(scopedTaskIDs) != 1 || scopedTaskIDs[0] != "task-a" {
100+
t.Errorf("scoped task IDs = %v, want [task-a] — payload IDs must not steer scoping", scopedTaskIDs)
101+
}
102+
}
103+
104+
// TestMCPHandlerForDeniesWhenScopingFails pins fail-closed behavior: an
105+
// unresolvable owner must not fall through to the unscoped handlers.
106+
func TestMCPHandlerForDeniesWhenScopingFails(t *testing.T) {
107+
inner := &recordingMCPHandler{}
108+
sm := newMCPStreamManager(t, inner, func(context.Context, string) (context.Context, error) {
109+
return nil, errors.New("db unavailable")
110+
})
111+
112+
handler := sm.mcpHandlerFor(&AgentExecution{ID: "exec-1", TaskID: "task-a"})
113+
resp, err := handler.Dispatch(context.Background(), mcpRequest(t, map[string]interface{}{}))
114+
if err != nil {
115+
t.Fatalf("Dispatch: %v", err)
116+
}
117+
118+
if resp.Type != ws.MessageTypeError {
119+
t.Errorf("response type = %q, want error", resp.Type)
120+
}
121+
if inner.calls != 0 {
122+
t.Errorf("inner handler ran %d times, want 0 — the request must be denied", inner.calls)
123+
}
124+
}
125+
126+
// TestMCPHandlerForPassesThroughWithoutScoper keeps single-user instances and
127+
// isolated tests on the original unwrapped handler.
128+
func TestMCPHandlerForPassesThroughWithoutScoper(t *testing.T) {
129+
inner := &recordingMCPHandler{}
130+
sm := newMCPStreamManager(t, inner, nil)
131+
132+
if got := sm.mcpHandlerFor(&AgentExecution{ID: "exec-1", TaskID: "task-a"}); got != inner {
133+
t.Errorf("handler = %T, want the unwrapped inner handler", got)
134+
}
135+
}
136+
137+
// TestMCPHandlerForPassesThroughWithoutTaskID covers executions with no task
138+
// (there is no owner to resolve, so wrapping would deny every call).
139+
func TestMCPHandlerForPassesThroughWithoutTaskID(t *testing.T) {
140+
inner := &recordingMCPHandler{}
141+
sm := newMCPStreamManager(t, inner, func(ctx context.Context, _ string) (context.Context, error) {
142+
return ctx, nil
143+
})
144+
145+
if got := sm.mcpHandlerFor(&AgentExecution{ID: "exec-1"}); got != inner {
146+
t.Errorf("handler = %T, want the unwrapped inner handler", got)
147+
}
148+
}
149+
150+
func TestSetMCPIdentityScoperReachesStreamManager(t *testing.T) {
151+
log, err := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "json"})
152+
if err != nil {
153+
t.Fatalf("logger: %v", err)
154+
}
155+
m := &Manager{streamManager: NewStreamManager(log, StreamCallbacks{}, &recordingMCPHandler{}, nil)}
156+
157+
m.SetMCPIdentityScoper(func(ctx context.Context, _ string) (context.Context, error) { return ctx, nil })
158+
159+
if m.streamManager.mcpIdentityScoper == nil {
160+
t.Error("SetMCPIdentityScoper did not reach the stream manager")
161+
}
162+
}

apps/backend/internal/agent/runtime/lifecycle/streams.go

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,10 @@ type StreamManager struct {
3131
logger *logger.Logger
3232
callbacks StreamCallbacks
3333
mcpHandler agentctl.MCPHandler
34+
// mcpIdentityScoper scopes in-session MCP dispatches to the owner of the
35+
// stream's task. Nil leaves dispatch unscoped (single-user instances and
36+
// isolated tests); set via Manager.SetMCPIdentityScoper.
37+
mcpIdentityScoper MCPIdentityScoper
3438
// stopCh is the Manager-owned shutdown signal. The retry/backoff and
3539
// connected `<-ws.Done() / <-stop>` select read from it so they drain on
3640
// Manager.Stop. May be nil when isolated tests don't care about external
@@ -282,7 +286,7 @@ func (sm *StreamManager) connectUpdatesStream(execution *AgentExecution, ready c
282286
if sm.callbacks.OnAgentEvent != nil {
283287
sm.callbacks.OnAgentEvent(execution, event)
284288
}
285-
}, sm.mcpHandler, func(disconnectErr error) {
289+
}, sm.mcpHandlerFor(execution), func(disconnectErr error) {
286290
// WebSocket dropped — signal promptDoneCh so SendPrompt doesn't hang forever.
287291
// Only signal on unexpected errors (not normal close).
288292
if disconnectErr != nil {
@@ -324,7 +328,7 @@ func (sm *StreamManager) connectUpdatesStream(execution *AgentExecution, ready c
324328
// closing this stream is expected, not an error.
325329
func (sm *StreamManager) connectMCPStream(execution *AgentExecution) {
326330
ctx := sm.streamContext(execution)
327-
err := execution.agentctl.StreamUpdates(ctx, func(agentctl.AgentEvent) {}, sm.mcpHandler, func(disconnectErr error) {
331+
err := execution.agentctl.StreamUpdates(ctx, func(agentctl.AgentEvent) {}, sm.mcpHandlerFor(execution), func(disconnectErr error) {
328332
if disconnectErr != nil {
329333
sm.logger.Debug("passthrough MCP stream disconnected",
330334
zap.String("execution_id", execution.ID),

apps/backend/internal/auth/service_credentials.go

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,28 @@ func (s *Service) ResolveBearer(ctx context.Context, token string) (authn.Identi
121121
return authn.Identity{UserID: user.ID, Role: roleOf(user), TokenID: record.ID}, true
122122
}
123123

124+
// IdentityForUser resolves a stored user ID to the identity that user carries
125+
// on an authenticated request, without needing one of their credentials.
126+
//
127+
// It exists for callers that already know *whose* work they are performing but
128+
// have no request to authenticate: in-session agent MCP calls arrive over the
129+
// agent's own WebSocket stream, so the owning user is derived from the stream's
130+
// task rather than from a cookie or PAT (see internal/mcp/scope). The returned
131+
// identity is real — never synthetic — so per-user scoping applies to it.
132+
//
133+
// Returns false while authentication is disabled (callers keep the pre-auth
134+
// unscoped behavior) and when the account is missing or no longer active.
135+
func (s *Service) IdentityForUser(ctx context.Context, userID string) (authn.Identity, bool) {
136+
if s == nil || userID == "" || s.Mode() == ModeDisabled {
137+
return authn.Identity{}, false
138+
}
139+
user, err := s.activeUser(ctx, userID)
140+
if err != nil {
141+
return authn.Identity{}, false
142+
}
143+
return authn.Identity{UserID: user.ID, Role: roleOf(user)}, true
144+
}
145+
124146
// ListSessions returns the user's sessions for the account page.
125147
func (s *Service) ListSessions(ctx context.Context, userID string) ([]*store.Session, error) {
126148
return s.store.ListSessionsByUser(ctx, userID)

0 commit comments

Comments
 (0)