diff --git a/apps/backend/AGENTS.md b/apps/backend/AGENTS.md index 5300415592..4d9fbd5fb1 100644 --- a/apps/backend/AGENTS.md +++ b/apps/backend/AGENTS.md @@ -182,7 +182,9 @@ and can leak ACP subprocesses. - **Identity travels in the request context** (`authn.IdentityFromContext`). No identity = internal caller (pollers, event bus, office schedulers) = unscoped. Synthetic identity = auth disabled = unscoped. - **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. - **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. -- **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. +- **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. This includes **session-keyed** entry points: any service method taking a caller-supplied `sessionID` (or a `taskID` used to reach sessions) authorizes via `authorizeTaskID(session.TaskID)`. Authorize off the row you already read rather than calling `AuthorizeSessionAccess`, which re-reads the session. Batch helpers that take ID *lists* (`BatchGetSessionsForTasks`, `GetPrimarySessionInfoForTasks`, …) are exempt by convention: their callers derive the IDs from an already-authorized task list, so per-ID checks would add N queries to list views for no gain — keep it that way, and don't hand them caller-supplied IDs. +- **Two session paths bypass the task service and must guard themselves.** (1) Handlers that resolve an execution by a bare in-memory lookup (`GetExecutionBySessionID`, `*BySessionID`) skip the `GetOrEnsure*` chokepoint where the lifecycle check runs — call `service.AuthorizeSessionAccess` (see `ProcessHandlers.denySessionAccess`) or `lifecycleMgr.CheckSessionAccess` first. (2) The orchestrator reads sessions through its own repo handle, so it inherits nothing from the task service; session-keyed entry points there call `authorizeSession` / `authorizeTask`, wired by `SetSessionAccessChecker` / `SetTaskAccessChecker`. An entry point that accepts **both** a task and a session ID must use `authorizeTaskSessionPair`, never `authorizeSession` alone: the caller can satisfy a session-only check with one of their own sessions while pointing `taskID` at someone else's task, which the method then uses for its task-scoped work. Put the guard **first** in the method, before any repo/executor/agent-manager use — `TestSessionKeyedEntryPointsGuardBeforeDependencies` runs each entry point with nil dependencies and fails on a panic if a guard is placed too late. +- **Dispatched WS actions have a gateway backstop.** `Client.authorizeAction` (`internal/gateway/websocket/dispatch_scope.go`) checks any payload carrying `task_id` or `session_id` against the client's identity *before* dispatch, so a newly added action is scoped by default rather than only when its author remembers. Handler-level scoping is still required (defense in depth, and it covers non-WS callers) — do not delete a service-layer check because the backstop exists. The backstop reads `task_id`, `session_id` and `task_environment_id` — keep using those names for task-scoped actions, because an action that invents a new name for the same kind of resource silently opts out. (`user_shell.stop` was exactly that: it keys off `task_environment_id` with `task_id` optional, so a `task_id`-only backstop missed it.) Also avoid permissive types (`any`, `json.RawMessage`) for those fields: a payload that fails to unmarshal into the ref struct is treated as naming nothing. - **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. - **Self-authenticating webhooks** (automation, office channels, plugin webhooks) and `/health`, `/api/v1/features`, `/api/v1/app-state` stay public — the allowlist lives in `auth/httpmw/middleware.go` with a pinning test. diff --git a/apps/backend/internal/agent/handlers/passthrough_handlers.go b/apps/backend/internal/agent/handlers/passthrough_handlers.go index 366c1e05ed..ef43e9bfc4 100644 --- a/apps/backend/internal/agent/handlers/passthrough_handlers.go +++ b/apps/backend/internal/agent/handlers/passthrough_handlers.go @@ -49,6 +49,14 @@ func (h *PassthroughHandlers) wsAgentStdin(ctx context.Context, msg *ws.Message) return nil, fmt.Errorf("session_id is required") } + // Per-user scoping: this resolves the execution by a bare in-memory + // lookup, which skips the GetOrEnsure* chokepoint where the check runs. + // Unguarded, a caller with any live session ID could write bytes into + // another user's agent process. + if err := h.lifecycleMgr.CheckSessionAccess(ctx, req.SessionID); err != nil { + return nil, fmt.Errorf("session not found") + } + // Get the agent execution for this session execution, ok := h.lifecycleMgr.GetExecutionBySessionID(req.SessionID) if !ok { @@ -92,6 +100,14 @@ func (h *PassthroughHandlers) wsAgentResize(ctx context.Context, msg *ws.Message return nil, fmt.Errorf("session_id is required") } + // Per-user scoping: this resolves the execution by a bare in-memory + // lookup, which skips the GetOrEnsure* chokepoint where the check runs. + // Unguarded, a caller with any live session ID could write bytes into + // another user's agent process. + if err := h.lifecycleMgr.CheckSessionAccess(ctx, req.SessionID); err != nil { + return nil, fmt.Errorf("session not found") + } + if req.Cols == 0 || req.Rows == 0 { return nil, fmt.Errorf("cols and rows must be non-zero") } diff --git a/apps/backend/internal/agent/handlers/passthrough_scope_test.go b/apps/backend/internal/agent/handlers/passthrough_scope_test.go new file mode 100644 index 0000000000..0c062b5ce3 --- /dev/null +++ b/apps/backend/internal/agent/handlers/passthrough_scope_test.go @@ -0,0 +1,114 @@ +package handlers + +import ( + "context" + "encoding/json" + "errors" + "testing" + + "go.uber.org/zap" + + "github.com/kandev/kandev/internal/agent/runtime/lifecycle" + commonlogger "github.com/kandev/kandev/internal/common/logger" + ws "github.com/kandev/kandev/pkg/websocket" +) + +// agent.stdin and agent.resize resolve the execution with a bare in-memory +// lookup, which skips the GetOrEnsure* chokepoint where the lifecycle manager +// runs the per-user check. Unguarded, a caller holding any live session ID +// could write arbitrary bytes into another user's agent process — effectively +// running commands in their worktree — or resize its PTY. + +func passthroughTestLogger(t *testing.T) *commonlogger.Logger { + t.Helper() + log, err := commonlogger.NewFromZap(zap.NewNop()) + if err != nil { + t.Fatalf("logger: %v", err) + } + return log +} + +// TestPassthroughActionsDenyForeignSession asserts both actions refuse before +// touching the execution. The manager carries only the access checker, so the +// guard must fire before any other lifecycle state is consulted. +func TestPassthroughActionsDenyForeignSession(t *testing.T) { + mgr := &lifecycle.Manager{} + mgr.SetSessionAccessChecker(func(_ context.Context, sessionID string) error { + if sessionID == "sess-a" { + return nil + } + return errors.New("task not found") + }) + h := &PassthroughHandlers{lifecycleMgr: mgr, logger: passthroughTestLogger(t)} + + cases := map[string]struct { + action string + handler func(context.Context, *ws.Message) (*ws.Message, error) + payload string + }{ + "stdin": {ws.ActionAgentStdin, h.wsAgentStdin, `{"session_id":"sess-b","data":"rm -rf /\n"}`}, + "resize": {ws.ActionAgentResize, h.wsAgentResize, + `{"session_id":"sess-b","cols":80,"rows":24}`}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + msg := &ws.Message{ID: "1", Type: ws.MessageTypeRequest, Action: tc.action, + Payload: json.RawMessage(tc.payload)} + + resp, err := tc.handler(context.Background(), msg) + + if err == nil { + t.Fatalf("foreign session was accepted (resp=%+v)", resp) + } + // The refusal must not distinguish "not yours" from "no such session". + if err.Error() != "session not found" { + t.Errorf("error = %q, want \"session not found\"", err.Error()) + } + }) + } +} + +// TestPassthroughGuardIsNoOpWhenUnwired keeps single-user instances working. +// This asserts the guard's own predicate rather than driving the handler: with +// no checker installed the handler falls through to GetExecutionBySessionID, +// which a bare Manager cannot serve, so the property is only observable here. +func TestPassthroughGuardIsNoOpWhenUnwired(t *testing.T) { + if err := (&lifecycle.Manager{}).CheckSessionAccess(context.Background(), "sess-b"); err != nil { + t.Errorf("unwired checker denied the call (%v); pre-auth behavior broken", err) + } +} + +// TestUserShellStopDeniesForeignEnvironment covers the legacy fallback in +// wsUserShellStop: task_id is optional there and the fallback tears down the +// PTY through the interactive runner directly, never reaching +// GetOrEnsureExecutionForEnvironment where the environment check normally runs. +// With task_id empty and a foreign task_environment_id, it stopped another +// user's terminal. +func TestUserShellStopDeniesForeignEnvironment(t *testing.T) { + mgr := &lifecycle.Manager{} + mgr.SetEnvironmentAccessChecker(func(_ context.Context, envID string) error { + if envID == "env-a" { + return nil + } + return errors.New("task not found") + }) + h := &ShellHandlers{lifecycleMgr: mgr, logger: passthroughTestLogger(t)} + msg := &ws.Message{ID: "1", Type: ws.MessageTypeRequest, Action: ws.ActionUserShellStop, + Payload: json.RawMessage(`{"task_id":"","task_environment_id":"env-b","terminal_id":"shell-1"}`)} + + _, err := h.wsUserShellStop(context.Background(), msg) + + if err == nil { + t.Fatal("foreign task environment was accepted") + } + if err.Error() != "task environment not found" { + t.Errorf("error = %q, want \"task environment not found\"", err.Error()) + } +} + +// TestUserShellStopGuardIsNoOpWhenUnwired keeps single-user instances working. +func TestUserShellStopGuardIsNoOpWhenUnwired(t *testing.T) { + if err := (&lifecycle.Manager{}).CheckEnvironmentAccess(context.Background(), "env-b"); err != nil { + t.Errorf("unwired checker denied the call (%v); pre-auth behavior broken", err) + } +} diff --git a/apps/backend/internal/agent/handlers/shell_handlers.go b/apps/backend/internal/agent/handlers/shell_handlers.go index cc462d3c38..c04bb3fe8c 100644 --- a/apps/backend/internal/agent/handlers/shell_handlers.go +++ b/apps/backend/internal/agent/handlers/shell_handlers.go @@ -560,6 +560,16 @@ func (h *ShellHandlers) wsUserShellStop(ctx context.Context, msg *ws.Message) (* return nil, fmt.Errorf("terminal_id is required") } + // Per-user scoping. Guard on the environment, not task_id: task_id is + // optional here, and the legacy fallback below tears down the PTY through + // the interactive runner directly, never reaching + // GetOrEnsureExecutionForEnvironment where the check normally runs. Without + // this, an empty task_id plus a foreign task_environment_id stopped another + // user's terminal. + if err := h.lifecycleMgr.CheckEnvironmentAccess(ctx, req.TaskEnvironmentID); err != nil { + return nil, errors.New("task environment not found") + } + // Managed terminal — route through the service so the DB row is // deleted alongside the PTY tear-down. Destroy errors propagate so // the frontend doesn't optimistically remove a row that the backend diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager.go b/apps/backend/internal/agent/runtime/lifecycle/manager.go index 4a6cf77d95..52291ac79c 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager.go @@ -390,6 +390,18 @@ func (m *Manager) CheckSessionAccess(ctx context.Context, sessionID string) erro return m.sessionAccessCheck(ctx, sessionID) } +// CheckEnvironmentAccess authorizes an environment-scoped operation for the ctx +// identity. The environment-keyed sibling of CheckSessionAccess, for handlers +// that act on a task environment without going through +// GetOrEnsureExecutionForEnvironment (the user-shell tear-down path). +// No-op when no checker is set. +func (m *Manager) CheckEnvironmentAccess(ctx context.Context, taskEnvironmentID string) error { + if m.environmentAccessCheck == nil { + return nil + } + return m.environmentAccessCheck(ctx, taskEnvironmentID) +} + // SetWorkspaceInfoProvider sets the provider for workspace information. // // The WorkspaceInfoProvider interface allows the lifecycle manager to dynamically create diff --git a/apps/backend/internal/backendapp/auth.go b/apps/backend/internal/backendapp/auth.go index e11734d92b..a1307a24a1 100644 --- a/apps/backend/internal/backendapp/auth.go +++ b/apps/backend/internal/backendapp/auth.go @@ -60,6 +60,9 @@ func gatewayAuthPolicy(authSvc *auth.Service, taskSvc *taskservice.Service, task Session: taskSvc.AuthorizeSessionAccess, }, WorkspaceOwner: newWorkspaceOwnerResolver(taskRepo), + // The user-shell actions name a task environment and treat task_id as + // optional, so the dispatch backstop needs an environment-keyed check. + ActionEnvironment: taskSvc.AuthorizeEnvironmentAccess, } } diff --git a/apps/backend/internal/backendapp/orchestrator.go b/apps/backend/internal/backendapp/orchestrator.go index 8ef958290a..06b360d946 100644 --- a/apps/backend/internal/backendapp/orchestrator.go +++ b/apps/backend/internal/backendapp/orchestrator.go @@ -119,6 +119,12 @@ func provideOrchestrator( // boot payload and task.updated events. taskSvc.SetForegroundActivityProvider(orchestratorSvc) + // Per-user scoping for the session-keyed WS actions. The orchestrator + // resolves sessions through its own repo handle, so it does not inherit the + // task service's authorize* checks. + orchestratorSvc.SetSessionAccessChecker(taskSvc.AuthorizeSessionAccess) + orchestratorSvc.SetTaskAccessChecker(taskSvc.AuthorizeTaskAccess) + // Publish task.updated when the first session is marked primary so the // frontend receives primary_session_id for newly created tasks. orchestratorSvc.SetOnPrimarySessionSet(func(ctx context.Context, taskID, _ string) { diff --git a/apps/backend/internal/gateway/websocket/access.go b/apps/backend/internal/gateway/websocket/access.go index 90d75e95cc..ab8b9eb381 100644 --- a/apps/backend/internal/gateway/websocket/access.go +++ b/apps/backend/internal/gateway/websocket/access.go @@ -38,6 +38,11 @@ type AuthPolicy struct { Subscriptions SubscriptionAccessPolicy // WorkspaceOwner powers BroadcastToWorkspace owner routing. WorkspaceOwner WorkspaceOwnerResolver + // ActionEnvironment gates dispatched actions that name a task environment + // rather than a task or session — the user-shell actions, which treat + // task_id as optional. Kept off SubscriptionAccessPolicy because there is + // no environment subscription topic; this is dispatch-only. + ActionEnvironment func(ctx context.Context, taskEnvironmentID string) error } // SetAuthPolicy installs the auth hooks. Call during startup wiring, before diff --git a/apps/backend/internal/gateway/websocket/client.go b/apps/backend/internal/gateway/websocket/client.go index 263ffe0aa1..b97591e163 100644 --- a/apps/backend/internal/gateway/websocket/client.go +++ b/apps/backend/internal/gateway/websocket/client.go @@ -196,6 +196,19 @@ func (c *Client) handleMessage(msg *ws.Message) { // handler's work should run to completion so it doesn't leave partial // state behind. The dispatch ctx still cancels on server shutdown. dispatchCtx := c.dispatchContext() + + // Per-user scoping backstop: refuse before the handler runs if the payload + // names a task or session this client may not touch. See dispatch_scope.go + // for why this lives here and not only in each handler. + if err := c.authorizeAction(dispatchCtx, msg.Payload); err != nil { + c.logger.Debug("denied out-of-scope action", + zap.String("action", msg.Action), + zap.String("user_id", c.identity.UserID), + zap.Error(err)) + c.sendError(msg.ID, msg.Action, ws.ErrorCodeNotFound, "not found", nil) + return + } + response, err := c.hub.dispatcher.Dispatch(dispatchCtx, msg) if err != nil { c.logger.Error("Handler error", diff --git a/apps/backend/internal/gateway/websocket/dispatch_scope.go b/apps/backend/internal/gateway/websocket/dispatch_scope.go new file mode 100644 index 0000000000..b0ed74e898 --- /dev/null +++ b/apps/backend/internal/gateway/websocket/dispatch_scope.go @@ -0,0 +1,93 @@ +package websocket + +import ( + "context" + "encoding/json" + + "github.com/kandev/kandev/internal/auth/authn" +) + +// Per-user scoping for dispatched WS RPC actions. +// +// Subscriptions and HTTP routes were scoped when opt-in auth landed, but +// dispatched actions were left to authorize themselves at the service layer. +// That is per-method opt-in across ~74 actions in four packages, and it was +// missed repeatedly: session stop/delete/rename/launch, agent.cancel, +// permission.respond and agent.stdin all reached another user's session with +// nothing but a guessed ID. agent.stdin writes arbitrary bytes into the agent +// process, so the gap was equivalent to running commands in someone else's +// worktree. +// +// This is the structural backstop. Rather than trusting every present and +// future handler to remember, any action naming a task or session in its +// payload is checked here, before dispatch. Handlers still scope themselves +// (defense in depth, and they cover non-WS callers) — this makes a *newly +// added* action safe by default instead of only when its author remembers. + +// scopedActionRefs are the payload fields that name a per-user resource. +// +// All three names must stay in sync with what handlers actually send. The +// user-shell actions key off task_environment_id and treat task_id as +// optional, so checking only task_id let `user_shell.stop` tear down another +// user's terminal by naming their environment with task_id empty. An action +// that invents a fourth name for the same kind of resource silently opts out +// of this backstop — see AGENTS.md. +type scopedActionRefs struct { + TaskID string `json:"task_id"` + SessionID string `json:"session_id"` + TaskEnvironmentID string `json:"task_environment_id"` +} + +// authorizeAction reports why a dispatched action must be refused, or nil when +// the caller may proceed. +// +// No-op for connections without a real identity: identity-less internal +// clients and the synthetic identity injected while auth is disabled keep the +// pre-auth see-everything behavior, exactly as callerScope does server-side. +func (c *Client) authorizeAction(ctx context.Context, payload json.RawMessage) error { + if c.identity.UserID == "" || c.identity.Synthetic { + return nil + } + refs, ok := parseScopedActionRefs(payload) + if !ok { + return nil + } + policy := c.hub.authPolicy + if refs.SessionID != "" && policy.Subscriptions.Session != nil { + if err := policy.Subscriptions.Session(ctx, refs.SessionID); err != nil { + return err + } + } + if refs.TaskID != "" && policy.Subscriptions.Task != nil { + if err := policy.Subscriptions.Task(ctx, refs.TaskID); err != nil { + return err + } + } + if refs.TaskEnvironmentID != "" && policy.ActionEnvironment != nil { + if err := policy.ActionEnvironment(ctx, refs.TaskEnvironmentID); err != nil { + return err + } + } + return nil +} + +// parseScopedActionRefs pulls the scoping IDs out of a payload. ok=false when +// the payload carries neither (or is not a JSON object at all), which means +// there is nothing to authorize. +func parseScopedActionRefs(payload json.RawMessage) (scopedActionRefs, bool) { + if len(payload) == 0 { + return scopedActionRefs{}, false + } + var refs scopedActionRefs + if err := json.Unmarshal(payload, &refs); err != nil { + // Non-object payloads (arrays, bare values) name no resource. + return scopedActionRefs{}, false + } + return refs, refs.TaskID != "" || refs.SessionID != "" || refs.TaskEnvironmentID != "" +} + +// identityIsScoped reports whether this client's identity participates in +// per-user scoping. Exposed for tests and for symmetry with callerScope. +func identityIsScoped(identity authn.Identity) bool { + return identity.UserID != "" && !identity.Synthetic +} diff --git a/apps/backend/internal/gateway/websocket/dispatch_scope_test.go b/apps/backend/internal/gateway/websocket/dispatch_scope_test.go new file mode 100644 index 0000000000..3c0d100ee9 --- /dev/null +++ b/apps/backend/internal/gateway/websocket/dispatch_scope_test.go @@ -0,0 +1,269 @@ +package websocket + +import ( + "context" + "encoding/json" + "testing" + + "github.com/kandev/kandev/internal/auth/authn" + "github.com/kandev/kandev/internal/task/repository/repoerrors" + ws "github.com/kandev/kandev/pkg/websocket" +) + +// Dispatched WS actions were previously left to authorize themselves at the +// service layer, which was missed for a dozen session-mutating actions. These +// tests pin the gateway backstop: an action naming a foreign task or session is +// refused before its handler runs. + +// dispatchScopeFixture wires a hub whose auth policy grants only user-a's +// task-a / sess-a and denies everything else. +type dispatchScopeFixture struct { + client *Client + handled *bool +} + +func newDispatchScopeFixture(t *testing.T, identity authn.Identity) *dispatchScopeFixture { + t.Helper() + h := newTestHub(t) + dispatcher := ws.NewDispatcher() + h.dispatcher = dispatcher + h.mu.Lock() + h.dispatchCtx = context.Background() + h.mu.Unlock() + + // Only "sess-a" / "task-a" belong to user-a; everything else is denied. + h.setAuthPolicy(AuthPolicy{ + Enforced: func() bool { return true }, + Subscriptions: SubscriptionAccessPolicy{ + Session: func(_ context.Context, sessionID string) error { + if sessionID == "sess-a" { + return nil + } + return repoerrors.ErrTaskNotFound + }, + Task: func(_ context.Context, taskID string) error { + if taskID == "task-a" { + return nil + } + return repoerrors.ErrTaskNotFound + }, + }, + ActionEnvironment: func(_ context.Context, envID string) error { + if envID == "env-a" { + return nil + } + return repoerrors.ErrTaskNotFound + }, + }) + + handled := false + dispatcher.RegisterFunc("test.scoped", func(context.Context, *ws.Message) (*ws.Message, error) { + handled = true + return nil, nil + }) + + c := newTestClient("c-scope") + c.hub = h + c.identity = identity + return &dispatchScopeFixture{client: c, handled: &handled} +} + +func (f *dispatchScopeFixture) dispatch(t *testing.T, payload string) { + t.Helper() + f.client.handleMessage(&ws.Message{ + Action: "test.scoped", + Type: ws.MessageTypeRequest, + Payload: json.RawMessage(payload), + }) +} + +func realIdentity() authn.Identity { + return authn.Identity{UserID: "user-a", Role: authn.RoleMember} +} + +func TestDispatchScopeDeniesForeignSession(t *testing.T) { + f := newDispatchScopeFixture(t, realIdentity()) + + f.dispatch(t, `{"session_id":"sess-b"}`) + + if *f.handled { + t.Error("handler ran for a foreign session; the backstop did not fire") + } +} + +func TestDispatchScopeDeniesForeignTask(t *testing.T) { + f := newDispatchScopeFixture(t, realIdentity()) + + f.dispatch(t, `{"task_id":"task-b"}`) + + if *f.handled { + t.Error("handler ran for a foreign task") + } +} + +// TestDispatchScopeDeniesForeignSessionEvenWithOwnTask covers the mixed payload +// (session.launch / task session status carry both): owning the task must not +// license operating on someone else's session. +func TestDispatchScopeDeniesForeignSessionEvenWithOwnTask(t *testing.T) { + f := newDispatchScopeFixture(t, realIdentity()) + + f.dispatch(t, `{"task_id":"task-a","session_id":"sess-b"}`) + + if *f.handled { + t.Error("handler ran; a foreign session_id must deny even beside an owned task_id") + } +} + +func TestDispatchScopeAllowsOwnResources(t *testing.T) { + f := newDispatchScopeFixture(t, realIdentity()) + + f.dispatch(t, `{"task_id":"task-a","session_id":"sess-a"}`) + + if !*f.handled { + t.Error("handler did not run for the caller's own task and session") + } +} + +// TestDispatchScopeUnscopedIdentitiesPassThrough pins the compatibility +// contract: the synthetic identity injected while auth is disabled, and +// identity-less connections, keep the pre-auth see-everything behavior. +func TestDispatchScopeUnscopedIdentitiesPassThrough(t *testing.T) { + cases := map[string]authn.Identity{ + "synthetic": {UserID: "default-user", Role: authn.RoleAdmin, Synthetic: true}, + "no identity": {}, + } + for name, identity := range cases { + t.Run(name, func(t *testing.T) { + f := newDispatchScopeFixture(t, identity) + + f.dispatch(t, `{"session_id":"sess-b","task_id":"task-b"}`) + + if !*f.handled { + t.Errorf("%s caller was denied; pre-auth behavior must be preserved", name) + } + }) + } +} + +// TestDispatchScopeIgnoresPayloadsWithoutRefs makes sure the backstop does not +// interfere with actions that name no task or session, including non-object +// payloads (it must not panic or deny them). +func TestDispatchScopeIgnoresPayloadsWithoutRefs(t *testing.T) { + for _, payload := range []string{`{}`, `{"other":"x"}`, `[]`, `"str"`, `null`, ``} { + t.Run("payload="+payload, func(t *testing.T) { + f := newDispatchScopeFixture(t, realIdentity()) + + f.dispatch(t, payload) + + if !*f.handled { + t.Errorf("payload %q names no resource and must dispatch", payload) + } + }) + } +} + +// TestDispatchScopeNoOpWithoutPolicy keeps an unwired gateway permissive, so +// embedded/test setups behave as before. +func TestDispatchScopeNoOpWithoutPolicy(t *testing.T) { + h := newTestHub(t) + dispatcher := ws.NewDispatcher() + h.dispatcher = dispatcher + h.mu.Lock() + h.dispatchCtx = context.Background() + h.mu.Unlock() + handled := false + dispatcher.RegisterFunc("test.scoped", func(context.Context, *ws.Message) (*ws.Message, error) { + handled = true + return nil, nil + }) + c := newTestClient("c-nopolicy") + c.hub = h + c.identity = realIdentity() + + c.handleMessage(&ws.Message{ + Action: "test.scoped", + Type: ws.MessageTypeRequest, + Payload: json.RawMessage(`{"session_id":"sess-b"}`), + }) + + if !handled { + t.Error("no auth policy wired; the action must dispatch") + } +} + +func TestParseScopedActionRefs(t *testing.T) { + cases := []struct { + payload string + wantOK bool + wantTask string + wantSess string + wantEnv string + }{ + {`{"task_id":"t","session_id":"s"}`, true, "t", "s", ""}, + {`{"task_id":"t"}`, true, "t", "", ""}, + {`{"session_id":"s"}`, true, "", "s", ""}, + {`{"task_environment_id":"e"}`, true, "", "", "e"}, + {`{"task_id":"","task_environment_id":"e"}`, true, "", "", "e"}, + {`{}`, false, "", "", ""}, + {`{"task_id":""}`, false, "", "", ""}, + {`[1,2]`, false, "", "", ""}, + {``, false, "", "", ""}, + } + for _, tc := range cases { + refs, ok := parseScopedActionRefs(json.RawMessage(tc.payload)) + if ok != tc.wantOK { + t.Errorf("payload %q ok = %v, want %v", tc.payload, ok, tc.wantOK) + } + if refs.TaskID != tc.wantTask || refs.SessionID != tc.wantSess || refs.TaskEnvironmentID != tc.wantEnv { + t.Errorf("payload %q refs = %+v, want task=%q session=%q env=%q", + tc.payload, refs, tc.wantTask, tc.wantSess, tc.wantEnv) + } + } +} + +func TestIdentityIsScoped(t *testing.T) { + if identityIsScoped(authn.Identity{}) { + t.Error("empty identity must not be scoped") + } + if identityIsScoped(authn.Identity{UserID: "u", Synthetic: true}) { + t.Error("synthetic identity must not be scoped") + } + if !identityIsScoped(authn.Identity{UserID: "u"}) { + t.Error("real identity must be scoped") + } +} + +// TestDispatchScopeDeniesForeignEnvironment covers the user-shell actions, which +// name a task environment and treat task_id as optional. Checking only task_id +// let `user_shell.stop` tear down another user's terminal with task_id empty. +func TestDispatchScopeDeniesForeignEnvironment(t *testing.T) { + f := newDispatchScopeFixture(t, realIdentity()) + + f.dispatch(t, `{"task_environment_id":"env-b","terminal_id":"shell-1"}`) + + if *f.handled { + t.Error("handler ran for a foreign task environment") + } +} + +// TestDispatchScopeDeniesForeignEnvironmentWithEmptyTaskID is the exact bypass: +// omitting task_id must not skip the check. +func TestDispatchScopeDeniesForeignEnvironmentWithEmptyTaskID(t *testing.T) { + f := newDispatchScopeFixture(t, realIdentity()) + + f.dispatch(t, `{"task_id":"","task_environment_id":"env-b","terminal_id":"shell-1"}`) + + if *f.handled { + t.Error("empty task_id must not bypass the environment check") + } +} + +func TestDispatchScopeAllowsOwnEnvironment(t *testing.T) { + f := newDispatchScopeFixture(t, realIdentity()) + + f.dispatch(t, `{"task_environment_id":"env-a","terminal_id":"shell-1"}`) + + if !*f.handled { + t.Error("handler did not run for the caller's own task environment") + } +} diff --git a/apps/backend/internal/orchestrator/event_handlers_github.go b/apps/backend/internal/orchestrator/event_handlers_github.go index b3e63ae8d5..19d84ca34e 100644 --- a/apps/backend/internal/orchestrator/event_handlers_github.go +++ b/apps/backend/internal/orchestrator/event_handlers_github.go @@ -1122,6 +1122,19 @@ func (s *Service) associatePRFromPushScoped( // if found. This provides an on-demand alternative to the background poller, // allowing the frontend to trigger immediate PR detection. func (s *Service) CheckSessionPR(ctx context.Context, taskID, sessionID string) (bool, error) { + // Per-user scoping first, before any early return: this both reveals PR + // association and installs a PR watch, so it must be owner-only. Report + // "no PR" rather than an error so a foreign session leaks nothing about + // its existence. + // + // Both IDs, not just the session: everything below (GetTaskPR, + // resolveTaskRepo, EnsurePRWatch, resolvePrimaryTaskRepositoryID) is keyed + // off taskID, so authorizing only the session would let a caller pair one + // of their own sessions with another user's task. + if err := s.authorizeTaskSessionPair(ctx, taskID, sessionID); err != nil { + return false, nil + } + if s.githubService == nil { return false, nil } diff --git a/apps/backend/internal/orchestrator/event_handlers_transient.go b/apps/backend/internal/orchestrator/event_handlers_transient.go index 8b5643787f..3a6741d528 100644 --- a/apps/backend/internal/orchestrator/event_handlers_transient.go +++ b/apps/backend/internal/orchestrator/event_handlers_transient.go @@ -331,6 +331,16 @@ func (s *Service) cancelAllTransientRetries() { // and surfaces the manual recovery banner so they can Resume or Start fresh. // Returns true if a retry loop was active. func (s *Service) CancelTransientRetry(ctx context.Context, taskID, sessionID string) bool { + // Reports "nothing to cancel" on denial: the bool return carries no error + // channel, and a foreign session must not be distinguishable from an idle + // one. Guard first — resetTransientRetry below mutates retry state. + // + // Both IDs: taskID is handed to handleRecoverableFailure, which writes + // against that task, so the session check alone would leave it free to + // point at someone else's. + if err := s.authorizeTaskSessionPair(ctx, taskID, sessionID); err != nil { + return false + } _, active := s.transientRetries.Load(sessionID) s.resetTransientRetry(sessionID) if !active { diff --git a/apps/backend/internal/orchestrator/event_handlers_workflow.go b/apps/backend/internal/orchestrator/event_handlers_workflow.go index d0895e9e25..ba78565909 100644 --- a/apps/backend/internal/orchestrator/event_handlers_workflow.go +++ b/apps/backend/internal/orchestrator/event_handlers_workflow.go @@ -1975,6 +1975,10 @@ func (s *Service) clearSessionPlanMode(ctx context.Context, session *models.Task // Public entry point for client-driven plan-mode toggles (e.g. the "Implement plan" // affordance) so the change is server-authoritative and survives page refresh. func (s *Service) SetSessionPlanModeByID(ctx context.Context, sessionID string, enabled bool) error { + if err := s.authorizeSession(ctx, sessionID); err != nil { + return err + } + session, err := s.repo.GetTaskSession(ctx, sessionID) if err != nil { return err diff --git a/apps/backend/internal/orchestrator/service.go b/apps/backend/internal/orchestrator/service.go index d52e3adf85..995ff33c27 100644 --- a/apps/backend/internal/orchestrator/service.go +++ b/apps/backend/internal/orchestrator/service.go @@ -13,6 +13,7 @@ package orchestrator import ( "context" "errors" + "fmt" "regexp" "strings" "sync" @@ -265,6 +266,15 @@ type Service struct { // Task service owns the rich payload; orchestrator delegates. taskEvents TaskEventPublisher + // sessionAccessCheck enforces per-user workspace scoping on the + // session-keyed WS actions. Nil = unscoped. See SetSessionAccessChecker. + sessionAccessCheck func(ctx context.Context, sessionID string) error + + // taskAccessCheck is the task-keyed sibling of sessionAccessCheck, for + // entry points that name a task rather than a session (session.launch, + // session.ensure). Nil = unscoped. + taskAccessCheck func(ctx context.Context, taskID string) error + // Workflow step getter for prompt building workflowStepGetter WorkflowStepGetter @@ -736,6 +746,76 @@ func (s *Service) SetTaskEventPublisher(publisher TaskEventPublisher) { s.taskEvents = publisher } +// SetSessionAccessChecker installs the per-user workspace scoping check used by +// the session-keyed WS actions (task session status, session PR check). Those +// resolve sessions through the orchestrator's own repo handle rather than the +// task service, so they do not inherit its authorize* checks and must ask here. +// +// The checker must return nil for contexts without a request identity, so +// internal callers (event bus, schedulers) stay unscoped. Nil leaves every +// session-keyed action unscoped, which is the pre-auth behavior. +func (s *Service) SetSessionAccessChecker(check func(ctx context.Context, sessionID string) error) { + s.sessionAccessCheck = check +} + +// SetTaskAccessChecker installs the task-keyed sibling of +// SetSessionAccessChecker, used by the entry points that name a task rather +// than a session. Same contract: nil for identity-less internal callers. +func (s *Service) SetTaskAccessChecker(check func(ctx context.Context, taskID string) error) { + s.taskAccessCheck = check +} + +// authorizeSession applies the configured per-user session check. No-op when +// unwired. +func (s *Service) authorizeSession(ctx context.Context, sessionID string) error { + if s.sessionAccessCheck == nil || sessionID == "" { + return nil + } + return s.sessionAccessCheck(ctx, sessionID) +} + +// authorizeTask applies the configured per-user task check. No-op when unwired. +func (s *Service) authorizeTask(ctx context.Context, taskID string) error { + if s.taskAccessCheck == nil || taskID == "" { + return nil + } + return s.taskAccessCheck(ctx, taskID) +} + +// authorizeTaskSessionPair guards an entry point that accepts BOTH a task and a +// session ID. +// +// Checking only the session is not enough: the caller can pass one of their own +// sessions to satisfy that check while pointing taskID at another user's task, +// which the method then uses for its task-scoped work (GetTaskPR, repository +// resolution, PR-watch creation, recoverable-failure handling). Both IDs are +// authorized, and the pair is required to be consistent so the two arguments +// cannot describe different tasks. +// +// The pair check is skipped when the session cannot be loaded: by then both IDs +// are already authorized, so a mismatch is a caller bug rather than a +// cross-user leak, and failing here would change behavior for identity-less +// internal callers. +func (s *Service) authorizeTaskSessionPair(ctx context.Context, taskID, sessionID string) error { + if err := s.authorizeSession(ctx, sessionID); err != nil { + return err + } + if err := s.authorizeTask(ctx, taskID); err != nil { + return err + } + if taskID == "" || sessionID == "" || s.repo == nil { + return nil + } + session, err := s.repo.GetTaskSession(ctx, sessionID) + if err != nil || session == nil { + return nil //nolint:nilerr // both IDs authorized; consistency is best-effort + } + if session.TaskID != taskID { + return fmt.Errorf("session %s does not belong to task %s", sessionID, taskID) + } + return nil +} + // publishTaskUpdated forwards to the configured TaskEventPublisher. // No-op when the publisher isn't wired (tests, or before SetTaskEventPublisher // has been called during startup). Pass the pre-move workflow ID when the diff --git a/apps/backend/internal/orchestrator/session_ensure.go b/apps/backend/internal/orchestrator/session_ensure.go index 76d2e72058..b09bc6e0cc 100644 --- a/apps/backend/internal/orchestrator/session_ensure.go +++ b/apps/backend/internal/orchestrator/session_ensure.go @@ -56,6 +56,10 @@ func acquireEnsureLock(taskID string) func() { // not. This is used by the office advanced mode where one-off tasks may have // their execution torn down after completion. func (s *Service) EnsureSession(ctx context.Context, taskID string, opts ...EnsureSessionOptions) (*EnsureSessionResponse, error) { + if err := s.authorizeTask(ctx, taskID); err != nil { + return nil, err + } + if taskID == "" { return nil, fmt.Errorf("task_id is required") } diff --git a/apps/backend/internal/orchestrator/session_launch.go b/apps/backend/internal/orchestrator/session_launch.go index a3d9b29fa6..2c491a9f6a 100644 --- a/apps/backend/internal/orchestrator/session_launch.go +++ b/apps/backend/internal/orchestrator/session_launch.go @@ -85,6 +85,14 @@ func ResolveIntent(req *LaunchSessionRequest) SessionIntent { // LaunchSession is the unified entry point for all session operations. func (s *Service) LaunchSession(ctx context.Context, req *LaunchSessionRequest) (*LaunchSessionResponse, error) { + // Every intent funnels through here. SessionID is empty when creating, so + // that case is carried by the task check alone. + if err := s.authorizeTask(ctx, req.TaskID); err != nil { + return nil, err + } + if err := s.authorizeSession(ctx, req.SessionID); err != nil { + return nil, err + } intent := ResolveIntent(req) req.Prompt = strings.TrimSpace(req.Prompt) @@ -286,6 +294,14 @@ func (s *Service) launchRestoreWorkspace(ctx context.Context, req *LaunchSession // RecoverSession handles user-initiated recovery after an agent CLI failure. // action is "resume" (retry with existing ACP session) or "fresh_start" (clear token, start fresh). func (s *Service) RecoverSession(ctx context.Context, taskID, sessionID, action string) (*LaunchSessionResponse, error) { + // Guard before the switch: "fresh_start" clears the resume token, so an + // unauthorized call would mutate the session even if the launch failed. + if err := s.authorizeSession(ctx, sessionID); err != nil { + return nil, err + } + if err := s.authorizeTask(ctx, taskID); err != nil { + return nil, err + } switch action { case "fresh_start": s.clearResumeToken(ctx, sessionID) diff --git a/apps/backend/internal/orchestrator/session_scope_matrix_test.go b/apps/backend/internal/orchestrator/session_scope_matrix_test.go new file mode 100644 index 0000000000..afb1474465 --- /dev/null +++ b/apps/backend/internal/orchestrator/session_scope_matrix_test.go @@ -0,0 +1,245 @@ +package orchestrator + +import ( + "context" + "errors" + "testing" + + "github.com/kandev/kandev/internal/task/models" +) + +// pairStubRepo satisfies sessionExecutorStore with only GetTaskSession usable — +// the pair consistency check calls nothing else, so the embedded nil interface +// is never touched. +type pairStubRepo struct { + sessionExecutorStore + sessionTaskID string +} + +func (r *pairStubRepo) GetTaskSession(context.Context, string) (*models.TaskSession, error) { + return &models.TaskSession{ID: "sess-1", TaskID: r.sessionTaskID}, nil +} + +// Structural pin for the orchestrator's session-keyed entry points. +// +// These resolve sessions through the orchestrator's own repo/executor handles, +// so they inherit nothing from the task service's authorize* helpers. Each one +// was reachable straight from a WS action with a caller-supplied ID and no +// ownership check — stopping, deleting, renaming, relaunching or cancelling +// another user's live agent session, or answering their permission prompts. +// +// Every entry asserts the guard fires. Dependencies are left nil on purpose: a +// denial must short-circuit before the method touches repo, executor or agent +// manager, so a nil-pointer panic here means the guard is in the wrong place. + +var errDenied = errors.New("task not found") + +// deniedCase invokes one entry point and reports whether it refused. +type deniedCase struct { + name string + invoke func(*Service) error +} + +func sessionScopeCases() []deniedCase { + const ( + taskID = "task-b" + sessionID = "sess-b" + ) + return []deniedCase{ + {"StopSession", func(s *Service) error { + return s.StopSession(context.Background(), sessionID, "reason", false) + }}, + {"DeleteSession", func(s *Service) error { + return s.DeleteSession(context.Background(), sessionID) + }}, + {"SetPrimarySession", func(s *Service) error { + return s.SetPrimarySession(context.Background(), sessionID) + }}, + {"RenameSession", func(s *Service) error { + return s.RenameSession(context.Background(), sessionID, "hijacked") + }}, + {"ResetAgentContext", func(s *Service) error { + return s.ResetAgentContext(context.Background(), sessionID) + }}, + {"CancelAgent", func(s *Service) error { + return s.CancelAgent(context.Background(), sessionID) + }}, + {"RespondToPermission", func(s *Service) error { + return s.RespondToPermission(context.Background(), sessionID, "pending-1", "allow", false, false) + }}, + {"SetSessionPlanModeByID", func(s *Service) error { + return s.SetSessionPlanModeByID(context.Background(), sessionID, true) + }}, + {"RecoverSession", func(s *Service) error { + _, err := s.RecoverSession(context.Background(), taskID, sessionID, "resume") + return err + }}, + {"LaunchSession", func(s *Service) error { + _, err := s.LaunchSession(context.Background(), &LaunchSessionRequest{ + TaskID: taskID, SessionID: sessionID, + }) + return err + }}, + {"LaunchSession/create-no-session", func(s *Service) error { + // SessionID is empty when creating, so the task check must carry it. + _, err := s.LaunchSession(context.Background(), &LaunchSessionRequest{TaskID: taskID}) + return err + }}, + {"EnsureSession", func(s *Service) error { + _, err := s.EnsureSession(context.Background(), taskID) + return err + }}, + } +} + +// TestSessionKeyedEntryPointsDenyForeignResources is the matrix: with both +// checkers denying, every entry point must refuse. +func TestSessionKeyedEntryPointsDenyForeignResources(t *testing.T) { + for _, tc := range sessionScopeCases() { + t.Run(tc.name, func(t *testing.T) { + s := &Service{ + logger: scopeTestLogger(t), + sessionAccessCheck: func(context.Context, string) error { return errDenied }, + taskAccessCheck: func(context.Context, string) error { return errDenied }, + } + + if err := tc.invoke(s); err == nil { + t.Fatal("entry point did not deny a foreign resource") + } + }) + } +} + +// TestSessionKeyedEntryPointsGuardBeforeDependencies asserts the guard runs +// before any nil dependency is touched — the run above would panic, not fail, +// if a guard were placed after the first repo/executor call. Kept separate so +// the intent is explicit rather than implied by the absence of a panic. +func TestSessionKeyedEntryPointsGuardBeforeDependencies(t *testing.T) { + for _, tc := range sessionScopeCases() { + t.Run(tc.name, func(t *testing.T) { + defer func() { + if r := recover(); r != nil { + t.Fatalf("guard runs too late — panicked on a nil dependency: %v", r) + } + }() + s := &Service{ + logger: scopeTestLogger(t), + sessionAccessCheck: func(context.Context, string) error { return errDenied }, + taskAccessCheck: func(context.Context, string) error { return errDenied }, + } + _ = tc.invoke(s) + }) + } +} + +// TestCancelTransientRetryDeniesForeignSession is separate because it returns a +// bool rather than an error: a denial must read as "nothing to cancel" so a +// foreign session is indistinguishable from an idle one. +func TestCancelTransientRetryDeniesForeignSession(t *testing.T) { + s := &Service{ + logger: scopeTestLogger(t), + sessionAccessCheck: func(context.Context, string) error { return errDenied }, + } + + if s.CancelTransientRetry(context.Background(), "task-b", "sess-b") { + t.Error("CancelTransientRetry reported a cancellation for a foreign session") + } +} + +// TestSessionKeyedEntryPointsUnscopedWhenUnwired pins the compatibility +// contract for every entry point at once: with no checkers installed nothing is +// denied by scoping, so identity-less internal callers (schedulers, event bus, +// workflow engine) and pre-auth single-user instances are unaffected. +// +// The methods still fail on their nil dependencies — that is expected and not +// what this asserts. It asserts they get *past* the guard, which is why the +// denial sentinel must not appear. +func TestSessionKeyedEntryPointsUnscopedWhenUnwired(t *testing.T) { + for _, tc := range sessionScopeCases() { + t.Run(tc.name, func(t *testing.T) { + defer func() { + // Reaching a nil dependency proves the guard did not deny. + _ = recover() + }() + s := &Service{logger: scopeTestLogger(t)} + + if err := tc.invoke(s); errors.Is(err, errDenied) { + t.Fatal("unwired checker denied the call; pre-auth behavior broken") + } + }) + } +} + +// ownSessionForeignTask is the pairing attack: the caller supplies one of their +// own session IDs to satisfy the session check, while pointing taskID at another +// user's task. Entry points that accept both IDs then do their task-scoped work +// against the foreign task — for CheckSessionPR that is PR disclosure plus a +// PR-watch write, for CancelTransientRetry a recoverable-failure write. +func ownSessionForeignTask() *Service { + return &Service{ + sessionAccessCheck: func(_ context.Context, sessionID string) error { + if sessionID == "sess-mine" { + return nil + } + return errDenied + }, + taskAccessCheck: func(_ context.Context, taskID string) error { + if taskID == "task-mine" { + return nil + } + return errDenied + }, + } +} + +func TestCheckSessionPRDeniesOwnSessionPairedWithForeignTask(t *testing.T) { + s := ownSessionForeignTask() + s.logger = scopeTestLogger(t) + + found, err := s.CheckSessionPR(context.Background(), "task-victim", "sess-mine") + + if err != nil { + t.Fatalf("CheckSessionPR: %v", err) + } + if found { + t.Error("found = true; an owned session must not unlock another user's task") + } +} + +func TestCancelTransientRetryDeniesOwnSessionPairedWithForeignTask(t *testing.T) { + s := ownSessionForeignTask() + s.logger = scopeTestLogger(t) + + if s.CancelTransientRetry(context.Background(), "task-victim", "sess-mine") { + t.Error("an owned session must not unlock another user's task") + } +} + +// TestAuthorizeTaskSessionPairRejectsInconsistentPair covers the consistency +// half: both IDs authorized, but naming a session that belongs to a different +// task must still be refused. +func TestAuthorizeTaskSessionPairRejectsInconsistentPair(t *testing.T) { + s := &Service{ + logger: scopeTestLogger(t), + sessionAccessCheck: func(context.Context, string) error { return nil }, + taskAccessCheck: func(context.Context, string) error { return nil }, + repo: &pairStubRepo{sessionTaskID: "task-other"}, + } + + if err := s.authorizeTaskSessionPair(context.Background(), "task-mine", "sess-1"); err == nil { + t.Error("expected refusal for a session that belongs to a different task") + } +} + +func TestAuthorizeTaskSessionPairAcceptsConsistentPair(t *testing.T) { + s := &Service{ + logger: scopeTestLogger(t), + sessionAccessCheck: func(context.Context, string) error { return nil }, + taskAccessCheck: func(context.Context, string) error { return nil }, + repo: &pairStubRepo{sessionTaskID: "task-mine"}, + } + + if err := s.authorizeTaskSessionPair(context.Background(), "task-mine", "sess-1"); err != nil { + t.Errorf("consistent pair was refused: %v", err) + } +} diff --git a/apps/backend/internal/orchestrator/session_scope_test.go b/apps/backend/internal/orchestrator/session_scope_test.go new file mode 100644 index 0000000000..de038dad32 --- /dev/null +++ b/apps/backend/internal/orchestrator/session_scope_test.go @@ -0,0 +1,109 @@ +package orchestrator + +import ( + "context" + "errors" + "testing" + + "go.uber.org/zap" + + commonlogger "github.com/kandev/kandev/internal/common/logger" +) + +// The session-keyed WS actions resolve sessions through the orchestrator's own +// repo handle, so they do not inherit the task service's authorize* checks. +// Without SetSessionAccessChecker a caller supplying another user's +// (task_id, session_id) pair was served: GetTaskSessionStatus returned that +// session's live status, and CheckSessionPR revealed PR association and +// installed a PR watch. + +func scopeTestLogger(t *testing.T) *commonlogger.Logger { + t.Helper() + log, err := commonlogger.NewFromZap(zap.NewNop()) + if err != nil { + t.Fatalf("logger: %v", err) + } + return log +} + +// denyingChecker refuses every session and records that it was consulted. +func denyingChecker(called *bool) func(context.Context, string) error { + return func(context.Context, string) error { + *called = true + return errors.New("task not found") + } +} + +// TestGetTaskSessionStatusDeniesForeignSession also pins that the denial is +// reported in the same shape as a missing session, so the response does not +// distinguish "not yours" from "does not exist". repo stays nil: a denial must +// short-circuit before any session lookup. +func TestGetTaskSessionStatusDeniesForeignSession(t *testing.T) { + called := false + s := &Service{logger: scopeTestLogger(t), sessionAccessCheck: denyingChecker(&called)} + + resp, err := s.GetTaskSessionStatus(context.Background(), "task-b", "sess-b") + if err != nil { + t.Fatalf("GetTaskSessionStatus: %v", err) + } + + if !called { + t.Fatal("session access checker was not consulted") + } + if resp.Error != "session not found" { + t.Errorf("resp.Error = %q, want \"session not found\"", resp.Error) + } +} + +// TestCheckSessionPRDeniesForeignSession keeps the check ahead of the +// unconfigured-GitHub early return, so the gate applies regardless of setup. +func TestCheckSessionPRDeniesForeignSession(t *testing.T) { + called := false + s := &Service{logger: scopeTestLogger(t), sessionAccessCheck: denyingChecker(&called)} + + found, err := s.CheckSessionPR(context.Background(), "task-b", "sess-b") + if err != nil { + t.Fatalf("CheckSessionPR: %v", err) + } + + if !called { + t.Fatal("session access checker was not consulted before the early return") + } + if found { + t.Error("found = true; a denied session must not report a PR") + } +} + +// TestSessionScopeUnwiredStaysUnscoped pins the pre-auth behavior: with no +// checker installed nothing is denied, so single-user instances and tests are +// unaffected. GetTaskSessionStatus is exercised via CheckSessionPR, which +// returns cleanly without a repo. +func TestSessionScopeUnwiredStaysUnscoped(t *testing.T) { + s := &Service{logger: scopeTestLogger(t)} + + if err := s.authorizeSession(context.Background(), "any-session"); err != nil { + t.Fatalf("unwired checker must not deny: %v", err) + } + found, err := s.CheckSessionPR(context.Background(), "task-b", "sess-b") + if err != nil || found { + t.Fatalf("CheckSessionPR = (%v, %v), want (false, nil) with no github service", found, err) + } +} + +// TestSessionScopeCheckerReceivesSessionID guards against wiring the check to +// the task ID by mistake, which would authorize the wrong resource. +func TestSessionScopeCheckerReceivesSessionID(t *testing.T) { + var got string + s := &Service{logger: scopeTestLogger(t), sessionAccessCheck: func(_ context.Context, sessionID string) error { + got = sessionID + return nil + }} + + if _, err := s.CheckSessionPR(context.Background(), "task-b", "sess-b"); err != nil { + t.Fatalf("CheckSessionPR: %v", err) + } + + if got != "sess-b" { + t.Errorf("checker received %q, want the session ID sess-b", got) + } +} diff --git a/apps/backend/internal/orchestrator/task_operations.go b/apps/backend/internal/orchestrator/task_operations.go index 1c211a1f80..65c67856b4 100644 --- a/apps/backend/internal/orchestrator/task_operations.go +++ b/apps/backend/internal/orchestrator/task_operations.go @@ -1734,6 +1734,11 @@ func (s *Service) waitForSessionReady(ctx context.Context, sessionID string) err } } +// sessionNotFoundStatus is the status-response error used for both a missing +// session and one the caller does not own, so the response cannot be used to +// tell the two apart. +const sessionNotFoundStatus = "session not found" + // GetTaskSessionStatus returns the status of a task session including whether it's resumable func (s *Service) GetTaskSessionStatus(ctx context.Context, taskID, sessionID string) (dto.TaskSessionStatusResponse, error) { s.logger.Debug("checking task session status", @@ -1745,10 +1750,17 @@ func (s *Service) GetTaskSessionStatus(ctx context.Context, taskID, sessionID st TaskID: taskID, } + // Per-user scoping: a foreign session is indistinguishable from a missing + // one, so reuse the same response shape rather than a distinct error. + if err := s.authorizeSession(ctx, sessionID); err != nil { + resp.Error = sessionNotFoundStatus + return resp, nil + } + // 1. Load session from database session, err := s.repo.GetTaskSession(ctx, sessionID) if err != nil { - resp.Error = "session not found" + resp.Error = sessionNotFoundStatus return resp, nil } @@ -2174,6 +2186,10 @@ func (s *Service) CancelTaskExecution(ctx context.Context, taskID string, reason // StopSession stops agent execution for a specific session func (s *Service) StopSession(ctx context.Context, sessionID string, reason string, force bool) error { + if err := s.authorizeSession(ctx, sessionID); err != nil { + return err + } + s.logger.Info("stopping session execution", zap.String("session_id", sessionID), zap.String("reason", reason), @@ -2183,6 +2199,10 @@ func (s *Service) StopSession(ctx context.Context, sessionID string, reason stri // DeleteSession deletes a session that is not currently running. func (s *Service) DeleteSession(ctx context.Context, sessionID string) error { + if err := s.authorizeSession(ctx, sessionID); err != nil { + return err + } + session, err := s.repo.GetTaskSession(ctx, sessionID) if err != nil { return fmt.Errorf("session not found: %w", err) @@ -2265,6 +2285,10 @@ func (s *Service) promoteNextPrimaryAfterRemoval(ctx context.Context, taskID, de // SetPrimarySession marks a session as the primary session for its task // and broadcasts a task.updated event so the frontend reflects the change. func (s *Service) SetPrimarySession(ctx context.Context, sessionID string) error { + if err := s.authorizeSession(ctx, sessionID); err != nil { + return err + } + if err := s.repo.SetSessionPrimary(ctx, sessionID); err != nil { return fmt.Errorf("failed to set session as primary: %w", err) } @@ -2293,6 +2317,10 @@ const maxSessionNameLength = 120 // session.state_changed event (same state) so all clients update the tab label. // An empty name clears the custom label, falling back to the derived title. func (s *Service) RenameSession(ctx context.Context, sessionID, name string) error { + if err := s.authorizeSession(ctx, sessionID); err != nil { + return err + } + name = strings.TrimSpace(name) // Truncate by runes, not bytes — a byte slice could split a multi-byte // UTF-8 sequence and persist an invalid string. @@ -3063,6 +3091,10 @@ func (s *Service) trySwitchModel(ctx context.Context, taskID, sessionID, model, // RespondToPermission sends a response to a permission request for a session func (s *Service) RespondToPermission(ctx context.Context, sessionID, pendingID, optionID string, cancelled, rejected bool) error { + if err := s.authorizeSession(ctx, sessionID); err != nil { + return err + } + s.logger.Debug("responding to permission request", zap.String("session_id", sessionID), zap.String("pending_id", pendingID), @@ -3228,6 +3260,10 @@ func (s *Service) isCancelInFlight(sessionID string) bool { // turn) so the user can unstick a session whose agent subprocess crashed. Other errors // still fail the cancel. func (s *Service) CancelAgent(ctx context.Context, sessionID string) error { + if err := s.authorizeSession(ctx, sessionID); err != nil { + return err + } + s.logger.Debug("cancelling agent turn", zap.String("session_id", sessionID)) // Deduplicate concurrent retries. The UI's cancel button has no in-flight @@ -3583,6 +3619,10 @@ func (s *Service) CompleteTask(ctx context.Context, taskID string) error { // ResetAgentContext resets the agent's conversation context for a session, // clearing conversation history while preserving the workspace environment. func (s *Service) ResetAgentContext(ctx context.Context, sessionID string) error { + if err := s.authorizeSession(ctx, sessionID); err != nil { + return err + } + session, err := s.repo.GetTaskSession(ctx, sessionID) if err != nil { return fmt.Errorf("session not found: %w", err) diff --git a/apps/backend/internal/task/handlers/message_handlers.go b/apps/backend/internal/task/handlers/message_handlers.go index 4a95610d02..8f0c73adf8 100644 --- a/apps/backend/internal/task/handlers/message_handlers.go +++ b/apps/backend/internal/task/handlers/message_handlers.go @@ -99,12 +99,20 @@ func (h *MessageHandlers) registerHTTP(router *gin.Engine) { func (h *MessageHandlers) httpGetShellOutput(c *gin.Context) { message, err := h.service.GetMessage(c.Request.Context(), c.Param("message_id")) if err != nil { - if !errors.Is(err, sql.ErrNoRows) { - h.logger.Error("failed to get shell output message", zap.Error(err)) - c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to get shell output"}) + // A missing row surfaces as bare sql.ErrNoRows, which isNotFound does + // not classify (it matches typed sentinels and the "not found" substring, + // neither of which covers "sql: no rows in result set") — so keep that + // case explicit. + if errors.Is(err, sql.ErrNoRows) { + c.JSON(http.StatusNotFound, gin.H{"error": "message not found"}) return } - c.JSON(http.StatusNotFound, gin.H{"error": "message not found"}) + // Everything else goes through the shared mapper: GetMessage is now + // per-user scoped, so a denial arrives as ErrTaskNotFound. Answering 500 + // for a foreign message while a missing one gets 404 would be an + // existence signal, and inconsistent with the 404-everywhere rule the + // other session routes follow. + handleNotFound(c, h.logger, err, "message not found") return } output, ok := models.ExtractShellExecOutput(message.Metadata) diff --git a/apps/backend/internal/task/handlers/process_handlers.go b/apps/backend/internal/task/handlers/process_handlers.go index 0095895936..67ba20481d 100644 --- a/apps/backend/internal/task/handlers/process_handlers.go +++ b/apps/backend/internal/task/handlers/process_handlers.go @@ -345,6 +345,9 @@ func (h *ProcessHandlers) httpStopProcessByID(c *gin.Context) { c.JSON(http.StatusBadRequest, gin.H{"error": "session_id and process_id are required"}) return } + if h.denySessionAccess(c, sessionID) { + return + } h.logger.Debug("stop process request (path)", zap.String("session_id", sessionID), zap.String("process_id", processID), @@ -479,6 +482,22 @@ func resolveScriptCommand( } } +// denySessionAccess reports whether the caller may not touch sessionID, having +// already written the 404 response. +// +// The session-keyed routes in this file resolve their execution with a bare +// in-memory lookup (GetExecutionBySessionID / *BySessionID), which skips the +// GetOrEnsure* paths where the lifecycle manager runs the per-user check +// internally — so they have to ask explicitly. Routes that call +// service.GetTaskSession first are already covered by its scoping. +func (h *ProcessHandlers) denySessionAccess(c *gin.Context, sessionID string) bool { + if err := h.service.AuthorizeSessionAccess(c.Request.Context(), sessionID); err != nil { + c.JSON(http.StatusNotFound, gin.H{"error": "session not found"}) + return true + } + return false +} + func (h *ProcessHandlers) applyIfSessionReady(sessionID string, allowPassthrough bool, apply func() error) error { execution, running := h.lifecycleMgr.GetExecutionBySessionID(sessionID) if !running { @@ -500,6 +519,9 @@ func (h *ProcessHandlers) applyIfSessionReady(sessionID string, allowPassthrough // httpSetSessionMode applies the session mode now or records it for the next launch. func (h *ProcessHandlers) httpSetSessionMode(c *gin.Context) { sessionID := c.Param("id") + if h.denySessionAccess(c, sessionID) { + return + } var req struct { ModeID string `json:"mode_id"` } @@ -532,6 +554,9 @@ func (h *ProcessHandlers) httpSetSessionMode(c *gin.Context) { // httpSetSessionModel applies the session model now or records it for the next launch. func (h *ProcessHandlers) httpSetSessionModel(c *gin.Context) { sessionID := c.Param("id") + if h.denySessionAccess(c, sessionID) { + return + } var req struct { ModelID string `json:"model_id"` } @@ -564,6 +589,9 @@ func (h *ProcessHandlers) httpSetSessionModel(c *gin.Context) { // httpSetSessionConfigOption applies an option now or records it for the next launch. func (h *ProcessHandlers) httpSetSessionConfigOption(c *gin.Context) { sessionID := c.Param("id") + if h.denySessionAccess(c, sessionID) { + return + } var req struct { ConfigID string `json:"config_id"` Value string `json:"value"` @@ -597,6 +625,9 @@ func (h *ProcessHandlers) httpSetSessionConfigOption(c *gin.Context) { // httpAuthenticate triggers authentication for a given auth method on a running agent. func (h *ProcessHandlers) httpAuthenticate(c *gin.Context) { sessionID := c.Param("id") + if h.denySessionAccess(c, sessionID) { + return + } var req struct { MethodID string `json:"method_id"` } diff --git a/apps/backend/internal/task/handlers/session_scope_http_test.go b/apps/backend/internal/task/handlers/session_scope_http_test.go new file mode 100644 index 0000000000..4aa0359101 --- /dev/null +++ b/apps/backend/internal/task/handlers/session_scope_http_test.go @@ -0,0 +1,229 @@ +package handlers + +import ( + "context" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/gin-gonic/gin" + + "github.com/kandev/kandev/internal/auth/authn" + "github.com/kandev/kandev/internal/task/models" + "github.com/kandev/kandev/internal/task/service" +) + +// foreignSessionRepo owns everything as "user-b", so a request made as +// "user-a" exercises the per-user scoping denial. +type foreignSessionRepo struct { + mockRepository +} + +func (r *foreignSessionRepo) GetTaskSession(context.Context, string) (*models.TaskSession, error) { + return &models.TaskSession{ID: "sess-b", TaskID: "task-b"}, nil +} + +func (r *foreignSessionRepo) GetTask(context.Context, string) (*models.Task, error) { + return &models.Task{ID: "task-b", WorkspaceID: "ws-b"}, nil +} + +func (r *foreignSessionRepo) GetWorkspace(context.Context, string) (*models.Workspace, error) { + return &models.Workspace{ID: "ws-b", OwnerID: "user-b"}, nil +} + +func newForeignSessionHandlers(t *testing.T) *TaskHandlers { + t.Helper() + log := newTestLogger(t) + repo := &foreignSessionRepo{} + svc := service.NewService(service.Repos{ + Workspaces: repo, Tasks: repo, TaskRepos: repo, + Workflows: repo, Messages: repo, Turns: repo, + Sessions: repo, GitSnapshots: repo, RepoEntities: repo, + Executors: repo, Environments: repo, TaskEnvironments: repo, + Reviews: repo, + }, nil, log, service.RepositoryDiscoveryConfig{}) + return &TaskHandlers{service: svc, logger: log} +} + +// requestAs builds a gin context for sessionID carrying userID's identity. +func requestAs(t *testing.T, userID, sessionID string) (*gin.Context, *httptest.ResponseRecorder) { + t.Helper() + rec := httptest.NewRecorder() + c, _ := gin.CreateTestContext(rec) + c.Request = httptest.NewRequest(http.MethodPost, "/api/v1/sessions/"+sessionID+"/approve", nil) + c.Request = c.Request.WithContext( + authn.WithIdentity(c.Request.Context(), authn.Identity{UserID: userID, Role: authn.RoleMember}), + ) + c.Params = gin.Params{{Key: "id", Value: sessionID}} + return c, rec +} + +// TestHTTPApproveSessionDeniesForeignSessionWith404 pins the status mapping for +// the approve route. Approving advances a workflow step, so a foreign session +// must be refused — and it must answer 404 like every other session route +// rather than a 500, which would tell the caller the session exists. +func TestHTTPApproveSessionDeniesForeignSessionWith404(t *testing.T) { + gin.SetMode(gin.TestMode) + h := newForeignSessionHandlers(t) + c, rec := requestAs(t, "user-a", "sess-b") + + h.httpApproveSession(c) + + if rec.Code != http.StatusNotFound { + t.Fatalf("status = %d, want 404 (body: %s)", rec.Code, rec.Body.String()) + } + + // Prove the 404 is the scoping denial and not some unrelated error path: + // the owner reaches past the check and fails later, on the workflow-step + // fixtures this test deliberately does not build. + ownerCtx, ownerRec := requestAs(t, "user-b", "sess-b") + h.httpApproveSession(ownerCtx) + if ownerRec.Code == http.StatusNotFound { + t.Fatal("owner got 404 too — the 404 above is not proving authorization") + } +} + +// TestProcessRoutesDenyForeignSession covers the session-keyed routes in +// process_handlers.go. They resolve their execution with a bare in-memory +// lookup, which skips the lifecycle manager's internal per-user check, so each +// must deny explicitly. lifecycleMgr is nil on purpose: the check has to +// short-circuit before anything touches it, and a nil-pointer panic here would +// mean the guard is in the wrong place. +func TestProcessRoutesDenyForeignSession(t *testing.T) { + gin.SetMode(gin.TestMode) + log := newTestLogger(t) + repo := &foreignSessionRepo{} + svc := service.NewService(service.Repos{ + Workspaces: repo, Tasks: repo, TaskRepos: repo, + Workflows: repo, Messages: repo, Turns: repo, + Sessions: repo, GitSnapshots: repo, RepoEntities: repo, + Executors: repo, Environments: repo, TaskEnvironments: repo, + Reviews: repo, + }, nil, log, service.RepositoryDiscoveryConfig{}) + h := &ProcessHandlers{service: svc, logger: log} + + routes := map[string]func(*gin.Context){ + "set-mode": h.httpSetSessionMode, + "set-model": h.httpSetSessionModel, + "set-config-option": h.httpSetSessionConfigOption, + "authenticate": h.httpAuthenticate, + "stop-process": h.httpStopProcessByID, + } + for name, handler := range routes { + t.Run(name, func(t *testing.T) { + rec := httptest.NewRecorder() + c, _ := gin.CreateTestContext(rec) + c.Request = httptest.NewRequest(http.MethodPost, "/x", strings.NewReader(`{}`)) + c.Request.Header.Set("Content-Type", "application/json") + c.Request = c.Request.WithContext(authn.WithIdentity( + c.Request.Context(), + authn.Identity{UserID: "user-a", Role: authn.RoleMember}, + )) + c.Params = gin.Params{{Key: "id", Value: "sess-b"}, {Key: "processId", Value: "proc-1"}} + + handler(c) + + if rec.Code != http.StatusNotFound { + t.Fatalf("status = %d, want 404 (body: %s)", rec.Code, rec.Body.String()) + } + }) + } +} + +// TestHTTPGetTaskSessionDeniesForeignSessionWith404 covers the plain read, +// whose DTO carries session metadata, the host worktree path, and branch names. +func TestHTTPGetTaskSessionDeniesForeignSessionWith404(t *testing.T) { + gin.SetMode(gin.TestMode) + h := newForeignSessionHandlers(t) + c, rec := requestAs(t, "user-a", "sess-b") + + h.httpGetTaskSession(c) + + if rec.Code != http.StatusNotFound { + t.Fatalf("status = %d, want 404 (body: %s)", rec.Code, rec.Body.String()) + } + if body := rec.Body.String(); len(body) > 0 && rec.Code == http.StatusOK { + t.Fatalf("foreign session body leaked: %s", body) + } +} + +// TestHTTPGetShellOutputDeniesForeignMessageWith404 pins the status mapping for +// the shell-output route. GetMessage became per-user scoped, so a denial now +// arrives as ErrTaskNotFound rather than sql.ErrNoRows; matching only the latter +// answered 500 for a foreign message and 404 for a missing one, which is an +// existence signal. +func TestHTTPGetShellOutputDeniesForeignMessageWith404(t *testing.T) { + gin.SetMode(gin.TestMode) + + foreign := shellOutputRequestAs(t, "user-a") + if foreign.Code != http.StatusNotFound { + t.Fatalf("status = %d, want 404 (body: %s)", foreign.Code, foreign.Body.String()) + } + + // In-test witness that the 404 above comes from authorization and not from + // the route's post-service `!ok` gate. This test was previously vacuous for + // exactly that reason: with an empty metadata fixture it passed even with + // the guard removed. The owner must get past the check, so if this ever + // starts returning 404 too, the assertion above has stopped proving + // anything — which is cheaper to notice here than in a mutation run. + owner := shellOutputRequestAs(t, "user-b") + if owner.Code == http.StatusNotFound { + t.Fatal("owner got 404 too — the 404 above is not proving authorization") + } +} + +// shellOutputRequestAs issues the shell-output request for the foreign fixture's +// message as userID, and returns the recorded response. +func shellOutputRequestAs(t *testing.T, userID string) *httptest.ResponseRecorder { + t.Helper() + log := newTestLogger(t) + repo := &foreignMessageRepo{} + svc := service.NewService(service.Repos{ + Workspaces: repo, Tasks: repo, TaskRepos: repo, + Workflows: repo, Messages: repo, Turns: repo, + Sessions: repo, GitSnapshots: repo, RepoEntities: repo, + Executors: repo, Environments: repo, TaskEnvironments: repo, + Reviews: repo, + }, nil, log, service.RepositoryDiscoveryConfig{}) + h := &MessageHandlers{service: svc, logger: log} + + rec := httptest.NewRecorder() + c, _ := gin.CreateTestContext(rec) + c.Request = httptest.NewRequest(http.MethodGet, + "/api/v1/task-sessions/sess-b/messages/msg-b/shell-output", nil) + c.Request = c.Request.WithContext(authn.WithIdentity( + c.Request.Context(), authn.Identity{UserID: userID, Role: authn.RoleMember})) + c.Params = gin.Params{{Key: "id", Value: "sess-b"}, {Key: "message_id", Value: "msg-b"}} + + h.httpGetShellOutput(c) + return rec +} + +// foreignMessageRepo owns the message's session as "user-b". +// +// The metadata must be a VALID shell-output payload. httpGetShellOutput has a +// second 404 gate after the service call — +// `if !ok || message.TaskSessionID != c.Param("id")` — and +// ExtractShellExecOutput returns ok=false for nil metadata. With an empty +// fixture the route answers 404 through that gate whether or not scoping +// denied it, so the test would pass even with the authorization removed. +// Populating it means a scope regression reaches the 200 path and fails. +type foreignMessageRepo struct { + foreignSessionRepo +} + +func (r *foreignMessageRepo) GetMessage(context.Context, string) (*models.Message, error) { + return &models.Message{ + ID: "msg-b", + TaskSessionID: "sess-b", + Metadata: map[string]any{ + "status": "completed", + "normalized": map[string]any{ + "shell_exec": map[string]any{ + "output": map[string]any{"stdout": "victim shell output"}, + }, + }, + }, + }, nil +} diff --git a/apps/backend/internal/task/handlers/task_http_handlers.go b/apps/backend/internal/task/handlers/task_http_handlers.go index a11fceae82..dda0610234 100644 --- a/apps/backend/internal/task/handlers/task_http_handlers.go +++ b/apps/backend/internal/task/handlers/task_http_handlers.go @@ -500,8 +500,10 @@ func (h *TaskHandlers) httpListSessionTurns(c *gin.Context) { func (h *TaskHandlers) httpApproveSession(c *gin.Context) { result, err := h.service.ApproveSession(c.Request.Context(), c.Param("id")) if err != nil { - h.logger.Error("failed to approve session", zap.String("session_id", c.Param("id")), zap.Error(err)) - c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) + // Route not-found through the shared mapper so a per-user scoping + // denial answers 404 like every other session route, rather than + // surfacing as a 500 that distinguishes "yours" from "someone else's". + handleNotFound(c, h.logger, err, "task session not found") return } diff --git a/apps/backend/internal/task/service/service_messages.go b/apps/backend/internal/task/service/service_messages.go index ac014c94ce..a0712e8e04 100644 --- a/apps/backend/internal/task/service/service_messages.go +++ b/apps/backend/internal/task/service/service_messages.go @@ -265,7 +265,19 @@ func (s *Service) createMessageWithRetry(ctx context.Context, message *models.Me // GetMessage retrieves a message by ID func (s *Service) GetMessage(ctx context.Context, id string) (*models.Message, error) { - return s.messages.GetMessage(ctx, id) + message, err := s.messages.GetMessage(ctx, id) + if err != nil { + return nil, err + } + // Scope like ListMessages: the shell-output route reaches a message by ID, + // so without this a caller holding someone else's (session_id, message_id) + // pair could read their command output. + if message.TaskSessionID != "" { + if err := s.AuthorizeSessionAccess(ctx, message.TaskSessionID); err != nil { + return nil, err + } + } + return message, nil } // ListMessages returns all messages for a session. diff --git a/apps/backend/internal/task/service/service_sessions.go b/apps/backend/internal/task/service/service_sessions.go index 97674d7386..b6790b56d1 100644 --- a/apps/backend/internal/task/service/service_sessions.go +++ b/apps/backend/internal/task/service/service_sessions.go @@ -55,12 +55,26 @@ func (s *Service) WaitForSessionReady(ctx context.Context, sessionID string) err // ListTaskSessions returns all sessions for a task. func (s *Service) ListTaskSessions(ctx context.Context, taskID string) ([]*models.TaskSession, error) { + if err := s.authorizeTaskID(ctx, taskID); err != nil { + return nil, err + } return s.sessions.ListTaskSessions(ctx, taskID) } // GetTaskSession returns a single session by ID. +// +// Scoping authorizes off the row we just read rather than calling +// AuthorizeSessionAccess, which would re-read the same session: this is a hot +// path (every session-keyed HTTP route and several MCP tools land here). func (s *Service) GetTaskSession(ctx context.Context, sessionID string) (*models.TaskSession, error) { - return s.sessions.GetTaskSession(ctx, sessionID) + session, err := s.sessions.GetTaskSession(ctx, sessionID) + if err != nil { + return nil, err + } + if err := s.authorizeTaskID(ctx, session.TaskID); err != nil { + return nil, err + } + return session, nil } // GetExecutorRunningBySessionID returns the live executor row for sessionID, @@ -82,6 +96,9 @@ func (s *Service) DismissLastAgentError(ctx context.Context, sessionID, stamp st if err != nil { return nil, err } + if err := s.authorizeTaskID(ctx, session.TaskID); err != nil { + return nil, err + } lastErr, ok := models.LoadLastAgentError(session.Metadata) if !ok { return session, nil @@ -102,6 +119,9 @@ func (s *Service) DismissLastAgentError(ctx context.Context, sessionID, stamp st // GetPrimarySession returns the primary session for a task. func (s *Service) GetPrimarySession(ctx context.Context, taskID string) (*models.TaskSession, error) { + if err := s.authorizeTaskID(ctx, taskID); err != nil { + return nil, err + } return s.sessions.GetPrimarySessionByTaskID(ctx, taskID) } diff --git a/apps/backend/internal/task/service/service_sessions_access_test.go b/apps/backend/internal/task/service/service_sessions_access_test.go new file mode 100644 index 0000000000..d1b4a3d8c1 --- /dev/null +++ b/apps/backend/internal/task/service/service_sessions_access_test.go @@ -0,0 +1,229 @@ +package service + +import ( + "context" + "errors" + "testing" + + "github.com/kandev/kandev/internal/task/models" + "github.com/kandev/kandev/internal/task/repository/repoerrors" + v1 "github.com/kandev/kandev/pkg/api/v1" +) + +// Session-keyed reads and mutations must apply the same per-user scoping as the +// task surface. They did not: GetTaskSession, ListTaskSessions and +// GetPrimarySession returned rows for any ID, and DismissLastAgentError / +// ApproveSession mutated any session — so knowing another user's session or +// task ID was enough. The session DTO these feed carries Metadata, the host +// worktree path, and branch names, and ApproveSession advances a workflow step. + +type sessionScopeRepo interface { + CreateWorkspace(context.Context, *models.Workspace) error + CreateWorkflow(context.Context, *models.Workflow) error + CreateTask(context.Context, *models.Task) error + CreateTaskSession(context.Context, *models.TaskSession) error +} + +// seedSessionScopeFixture gives every owner a task with a primary session so +// both halves of the matrix (denied for foreign, allowed for own) are covered, +// plus a pre-auth unowned workspace that must stay visible to everyone. +func seedSessionScopeFixture(t *testing.T, repo sessionScopeRepo) { + t.Helper() + ctx := context.Background() + owners := []struct{ suffix, owner string }{ + {"a", "user-a"}, + {"b", "user-b"}, + {"legacy", ""}, + } + for _, o := range owners { + if err := repo.CreateWorkspace(ctx, &models.Workspace{ + ID: "ws-" + o.suffix, Name: "ws " + o.suffix, OwnerID: o.owner, + }); err != nil { + t.Fatalf("create workspace %s: %v", o.suffix, err) + } + if err := repo.CreateWorkflow(ctx, &models.Workflow{ + ID: "wf-" + o.suffix, WorkspaceID: "ws-" + o.suffix, Name: "flow " + o.suffix, + }); err != nil { + t.Fatalf("create workflow %s: %v", o.suffix, err) + } + if err := repo.CreateTask(ctx, &models.Task{ + ID: "task-" + o.suffix, WorkspaceID: "ws-" + o.suffix, WorkflowID: "wf-" + o.suffix, + WorkflowStepID: "step-1", Title: "task " + o.suffix, + State: v1.TaskStateCreated, Priority: "medium", + }); err != nil { + t.Fatalf("create task %s: %v", o.suffix, err) + } + if err := repo.CreateTaskSession(ctx, &models.TaskSession{ + ID: "sess-" + o.suffix, TaskID: "task-" + o.suffix, + IsPrimary: true, State: models.TaskSessionStateCreated, + }); err != nil { + t.Fatalf("create session %s: %v", o.suffix, err) + } + } +} + +func TestSessionScopingGetTaskSession(t *testing.T) { + svc, _, repo := createTestService(t) + seedSessionScopeFixture(t, repo) + + if _, err := svc.GetTaskSession(ctxAs("user-a"), "sess-b"); !errors.Is(err, repoerrors.ErrTaskNotFound) { + t.Fatalf("get foreign session = %v, want ErrTaskNotFound", err) + } + if _, err := svc.GetTaskSession(ctxAs("user-a"), "sess-a"); err != nil { + t.Fatalf("owner must still read own session: %v", err) + } + if _, err := svc.GetTaskSession(ctxAs("user-a"), "sess-legacy"); err != nil { + t.Fatalf("pre-auth unowned session must stay visible: %v", err) + } +} + +func TestSessionScopingListTaskSessions(t *testing.T) { + svc, _, repo := createTestService(t) + seedSessionScopeFixture(t, repo) + + if _, err := svc.ListTaskSessions(ctxAs("user-a"), "task-b"); !errors.Is(err, repoerrors.ErrTaskNotFound) { + t.Fatalf("list foreign task's sessions = %v, want ErrTaskNotFound", err) + } + own, err := svc.ListTaskSessions(ctxAs("user-a"), "task-a") + if err != nil { + t.Fatalf("owner must still list own sessions: %v", err) + } + if len(own) != 1 || own[0].ID != "sess-a" { + t.Fatalf("owner list = %+v, want [sess-a]", own) + } +} + +func TestSessionScopingGetPrimarySession(t *testing.T) { + svc, _, repo := createTestService(t) + seedSessionScopeFixture(t, repo) + + if _, err := svc.GetPrimarySession(ctxAs("user-a"), "task-b"); !errors.Is(err, repoerrors.ErrTaskNotFound) { + t.Fatalf("get foreign primary session = %v, want ErrTaskNotFound", err) + } + own, err := svc.GetPrimarySession(ctxAs("user-a"), "task-a") + if err != nil { + t.Fatalf("owner must still read own primary session: %v", err) + } + if own.ID != "sess-a" { + t.Fatalf("owner primary = %s, want sess-a", own.ID) + } +} + +// TestSessionScopingDismissLastAgentError covers a mutation: dismissing the +// banner on someone else's session. +func TestSessionScopingDismissLastAgentError(t *testing.T) { + svc, _, repo := createTestService(t) + seedSessionScopeFixture(t, repo) + + if _, err := svc.DismissLastAgentError(ctxAs("user-a"), "sess-b", ""); !errors.Is(err, repoerrors.ErrTaskNotFound) { + t.Fatalf("dismiss on foreign session = %v, want ErrTaskNotFound", err) + } + if _, err := svc.DismissLastAgentError(ctxAs("user-a"), "sess-a", ""); err != nil { + t.Fatalf("owner must still dismiss on own session: %v", err) + } +} + +// TestSessionScopingApproveSession covers the highest-impact mutation: approving +// a workflow review step on another user's session. The owner path is asserted +// only as "not denied" — a full approval needs workflow-step fixtures this test +// deliberately does not build, so any other error is fine here. +func TestSessionScopingApproveSession(t *testing.T) { + svc, _, repo := createTestService(t) + seedSessionScopeFixture(t, repo) + + if _, err := svc.ApproveSession(ctxAs("user-a"), "sess-b"); !errors.Is(err, repoerrors.ErrTaskNotFound) { + t.Fatalf("approve foreign session = %v, want ErrTaskNotFound", err) + } + if _, err := svc.ApproveSession(ctxAs("user-a"), "sess-a"); errors.Is(err, repoerrors.ErrTaskNotFound) { + t.Fatal("owner must not be denied access to their own session") + } +} + +// TestSessionScopingGetMessage covers the by-ID message read behind the +// shell-output route. ListMessages was already scoped; GetMessage was not, so a +// caller holding someone else's (session_id, message_id) pair could read their +// command output. +func TestSessionScopingGetMessage(t *testing.T) { + svc, _, repo := createTestService(t) + seedSessionScopeFixture(t, repo) + ctx := context.Background() + foreign, err := svc.CreateMessage(ctx, &CreateMessageRequest{ + TaskSessionID: "sess-b", TaskID: "task-b", + AuthorType: "agent", Content: "secret shell output", + }) + if err != nil { + t.Fatalf("seed message: %v", err) + } + own, err := svc.CreateMessage(ctx, &CreateMessageRequest{ + TaskSessionID: "sess-a", TaskID: "task-a", + AuthorType: "agent", Content: "my shell output", + }) + if err != nil { + t.Fatalf("seed own message: %v", err) + } + + if _, err := svc.GetMessage(ctxAs("user-a"), foreign.ID); !errors.Is(err, repoerrors.ErrTaskNotFound) { + t.Fatalf("get foreign message = %v, want ErrTaskNotFound", err) + } + if _, err := svc.GetMessage(ctxAs("user-a"), own.ID); err != nil { + t.Fatalf("owner must still read own message: %v", err) + } + if _, err := svc.GetMessage(context.Background(), foreign.ID); err != nil { + t.Fatalf("internal caller must stay unscoped: %v", err) + } +} + +// TestSessionScopingInternalAndSyntheticCallersUnscoped pins the compatibility +// contract: identity-less internal callers (pollers, event bus, office +// schedulers) and the synthetic identity used while auth is disabled keep +// seeing everything. +func TestSessionScopingInternalAndSyntheticCallersUnscoped(t *testing.T) { + svc, _, repo := createTestService(t) + seedSessionScopeFixture(t, repo) + + for name, ctx := range map[string]context.Context{ + "internal": context.Background(), + "synthetic": ctxSynthetic(), + } { + if _, err := svc.GetTaskSession(ctx, "sess-b"); err != nil { + t.Errorf("%s caller GetTaskSession: %v", name, err) + } + if _, err := svc.ListTaskSessions(ctx, "task-b"); err != nil { + t.Errorf("%s caller ListTaskSessions: %v", name, err) + } + if _, err := svc.GetPrimarySession(ctx, "task-b"); err != nil { + t.Errorf("%s caller GetPrimarySession: %v", name, err) + } + if _, err := svc.DismissLastAgentError(ctx, "sess-b", ""); err != nil { + t.Errorf("%s caller DismissLastAgentError: %v", name, err) + } + } +} + +// TestSessionScopingAttachWorkspaceSources covers POST +// /tasks/:id/workspace-sources and the add_workspace_sources_kandev MCP tool. +// The route arrived on main while this branch was in review with no ownership +// check, so a caller could attach repositories and folders to another user's +// task and have them materialized into that task's workspace. +func TestSessionScopingAttachWorkspaceSources(t *testing.T) { + svc, _, repo := createTestService(t) + seedSessionScopeFixture(t, repo) + + _, err := svc.AttachWorkspaceSources(ctxAs("user-a"), AttachWorkspaceSourcesRequest{ + TaskID: "task-b", + Sources: []WorkspaceSourceInput{{Kind: WorkspaceSourceFolder, LocalPath: t.TempDir()}}, + }) + if !errors.Is(err, repoerrors.ErrTaskNotFound) { + t.Fatalf("attach to foreign task = %v, want ErrTaskNotFound", err) + } + + // The owner is not denied: it gets past the guard and fails (or succeeds) + // on the source-materialization logic this test does not set up. + _, ownErr := svc.AttachWorkspaceSources(ctxAs("user-a"), AttachWorkspaceSourcesRequest{ + TaskID: "task-a", + Sources: []WorkspaceSourceInput{{Kind: WorkspaceSourceFolder, LocalPath: t.TempDir()}}, + }) + if errors.Is(ownErr, repoerrors.ErrTaskNotFound) { + t.Error("owner was denied access to their own task") + } +} diff --git a/apps/backend/internal/task/service/service_workflow.go b/apps/backend/internal/task/service/service_workflow.go index 3bfaa190bd..12908c61a0 100644 --- a/apps/backend/internal/task/service/service_workflow.go +++ b/apps/backend/internal/task/service/service_workflow.go @@ -31,6 +31,10 @@ func (s *Service) ApproveSession(ctx context.Context, sessionID string) (*Approv if err != nil { return nil, fmt.Errorf("failed to get session: %w", err) } + // Approving advances the task's workflow step, so this must be owner-only. + if err := s.authorizeTaskID(ctx, session.TaskID); err != nil { + return nil, err + } result.Session = session // Get the task to find its current workflow step diff --git a/apps/backend/internal/task/service/service_workspace_sources.go b/apps/backend/internal/task/service/service_workspace_sources.go index a97f177a06..9e4f577d07 100644 --- a/apps/backend/internal/task/service/service_workspace_sources.go +++ b/apps/backend/internal/task/service/service_workspace_sources.go @@ -61,6 +61,13 @@ func (s *Service) AttachWorkspaceSources(ctx context.Context, req AttachWorkspac if req.TaskID == "" || len(req.Sources) == 0 { return nil, fmt.Errorf("%w: task_id and at least one source are required", ErrInvalidWorkspaceSource) } + // Per-user scoping: this attaches repositories and folders to the task and + // materializes them into its workspace, so it must be owner-only. Guard + // before taking the per-task lock. Denial surfaces as ErrTaskNotFound, + // which the HTTP layer already maps to 404. + if err := s.authorizeTaskID(ctx, req.TaskID); err != nil { + return nil, err + } unlock := s.lockWorkspaceSources(req.TaskID) defer unlock() task, err := s.tasks.GetTask(ctx, req.TaskID)