fix(auth): close per-user session isolation gaps (service, WS, orchestrator) (#1939)
* fix(auth): scope session-keyed reads and mutations to the workspace owner Session-keyed service entry points did not apply per-user scoping, so knowing another user's session or task ID was enough to reach it: - GetTaskSession, ListTaskSessions, GetPrimarySession returned rows for any ID. These feed GET /task-sessions/:id and GET /tasks/:id/sessions, whose DTO carries session Metadata, the host worktree path, the branch, and container/executor IDs. - DismissLastAgentError mutated any session. - ApproveSession approved a workflow review step on any session, which advances the task's workflow — the highest-impact of the set. Authorize via authorizeTaskID off the row already read, rather than AuthorizeSessionAccess, which would re-read the session on what is a hot path. Internal identity-less callers (pollers, event bus, office schedulers, office agent JWTs) and the synthetic identity used while auth is disabled stay unscoped, and unowned pre-auth rows stay visible. Batch helpers that take ID lists are deliberately left alone: their callers derive IDs from an already-authorized task list, so per-ID checks would add N queries to list views for no gain. Recorded in AGENTS.md. Also route httpApproveSession through handleNotFound so a denial answers 404 like every other session route instead of a 500 that would confirm the session exists. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(auth): scope the session-keyed paths that bypass the task service An adversarial sweep of the previous commit found session-keyed surfaces reachable with a caller-supplied ID and no ownership check. They bypass the task service, so scoping its methods did not cover them: - POST /task-sessions/:id/{set-mode,set-model,set-config-option, authenticate} and .../processes/:processId/stop resolve their execution with a bare in-memory lookup, which skips the GetOrEnsure* chokepoint where the lifecycle per-user check runs. A non-owner could switch another user's model or mode, re-authenticate their agent, or kill their process. Guarded by a shared denySessionAccess helper. - The orchestrator reads sessions through its own repo handle, so GetTaskSessionStatus and CheckSessionPR inherited nothing from the task service. CheckSessionPR also installs a PR watch as a side effect. Added a SetSessionAccessChecker hook mirroring the lifecycle manager's, wired to taskSvc.AuthorizeSessionAccess. The check runs before CheckSessionPR's unconfigured-GitHub early return so it applies regardless of setup, and denials reuse the "session not found" shape so they cannot be told apart from a missing session. - GetMessage had no scoping while ListMessages beside it did, so a caller holding another user's (session_id, message_id) pair could read their shell output. This does NOT close the whole surface. A second sweep found the orchestrator's session-mutating WS actions (session.stop/delete/rename/ set_primary/set_plan_mode/reset_context/recover/launch/ensure, agent.cancel, permission.respond) and agent.stdin / agent.resize also unscoped — agent.stdin writes arbitrary input into another user's agent process. Those touch live session lifecycle and need their own change; recorded in AGENTS.md so the gap is not mistaken for covered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(auth): scope every session-keyed WS action, plus a gateway backstop Completes the sweep. Dispatched WS actions were left to authorize themselves at the service layer, and a dozen never did — a caller with a guessed session ID could stop, delete, rename, relaunch or reset another user's live agent session, cancel their in-flight turn, answer their permission prompts, or write arbitrary bytes into their agent process. Two layers, deliberately both: 1. Gateway backstop. Client.authorizeAction checks any dispatched payload carrying task_id or session_id against the client's identity before dispatch. Per-method opt-in across ~74 actions in four packages had been missed repeatedly; this makes a newly added action scoped by default. Verified safe to apply generically: session_id means a task session everywhere, and user-shell/terminal actions key off task_id / task_environment_id, so nothing is misread. 2. Explicit guards, for defense in depth and for non-WS callers: - orchestrator: StopSession, DeleteSession, SetPrimarySession, RenameSession, ResetAgentContext, CancelAgent, RespondToPermission, SetSessionPlanModeByID, RecoverSession, LaunchSession, EnsureSession, CancelTransientRetry. Added SetTaskAccessChecker for the task-keyed ones. LaunchSession guards every intent at the single funnel; SessionID is empty on create, so the task check carries that case. - agent.stdin / agent.resize now call lifecycleMgr.CheckSessionAccess, the helper already documented for bare-lookup handlers. Guards go first in each method, before any repo/executor use. TestSessionKeyedEntryPointsGuardBeforeDependencies runs all twelve with nil dependencies and fails on a panic if one is placed too late. Denials are indistinguishable from a missing resource: WS returns not-found, CancelTransientRetry reports "nothing cancelled", and GetTaskSessionStatus reuses its "session not found" shape. Unwired checkers stay permissive throughout, so identity-less internal callers (schedulers, event bus, workflow engine) and pre-auth single-user instances are unaffected — pinned per entry point. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(auth): scope environment-keyed user-shell actions Third sweep finding, in surface the previous commit's own comment claimed was covered. The user-shell actions key off task_environment_id and treat task_id as optional, so a task_id-only backstop missed them: user_shell.stop with an empty task_id and a foreign task_environment_id fell through to the legacy runner path and tore down another user's terminal PTY. That path never reaches GetOrEnsureExecutionForEnvironment, where the environment check normally runs, so neither layer caught it. - Gateway backstop now also reads task_environment_id, gated by a new AuthPolicy.ActionEnvironment hook wired to taskSvc.AuthorizeEnvironmentAccess. This covers all three environment-keyed actions generically, not just the reported one. - wsUserShellStop guards explicitly via a new lifecycleMgr.CheckEnvironmentAccess, mirroring CheckSessionAccess, so the fallback is safe independent of the gateway. Corrected the comment that wrongly asserted task_environment_id was already covered, and recorded in AGENTS.md both the exact field names the backstop reads and why a permissive type on one of them would silently opt out. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(auth): authorize both IDs on task+session entry points Review finding (P1). Entry points taking both a task and a session ID only checked the session, so a caller could satisfy that check with one of their own sessions while pointing taskID at another user's task: - CheckSessionPR then used the unchecked taskID for GetTaskPR, resolveTaskRepo, resolvePrimaryTaskRepositoryID and EnsurePRWatch — cross-workspace PR disclosure plus a PR-watch write. - CancelTransientRetry handed it to handleRecoverableFailure, which writes against that task. Both now use a new authorizeTaskSessionPair, which authorizes each ID and additionally requires the pair to be consistent, so the two arguments cannot describe different tasks. GetTaskSessionStatus already validated the pair after loading the session and is unchanged. The consistency half is skipped when the session cannot be loaded: both IDs are authorized by then, so a mismatch is a caller bug rather than a cross-user leak, and failing there would change behavior for identity-less internal callers. Recorded the rule in AGENTS.md — a two-ID entry point must never use authorizeSession alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(auth): scope AttachWorkspaceSources Found while rebasing onto main. POST /tasks/:id/workspace-sources and the add_workspace_sources_kandev MCP tool landed on main during this PR's 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. Same class as the rest of this PR, so fixed here rather than left for a follow-up. The WS dispatch backstop added earlier already covered the browser path (the payload carries task_id); this closes the HTTP route and the MCP tool, which do not go through it. ErrTaskNotFound already maps to 404 in workspaceSourceHTTPStatus, so the denial surfaces correctly without touching the handler. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(auth): map denied shell-output reads to 404 Review finding. GetMessage became per-user scoped earlier in this PR, so a denial now arrives as ErrTaskNotFound. httpGetShellOutput only mapped sql.ErrNoRows to 404, sending everything else to the 500 branch — so a foreign message answered 500 while a missing one answered 404, which is an existence signal and inconsistent with the 404-everywhere rule the other session routes follow. Kept the sql.ErrNoRows branch explicit rather than replacing it outright: isNotFound classifies typed sentinels and the "not found" substring, and "sql: no rows in result set" matches neither, so routing that case through handleNotFound alone regressed the missing-message response to 500 (caught by TestHTTPShellOutputSnapshot). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(auth): make the shell-output scoping test actually exercise the guard Both reviewers caught that TestHTTPGetShellOutputDeniesForeignMessageWith404 was vacuous. 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. The fixture had none, so the route answered 404 through that gate whether or not scoping denied the read. Confirmed by mutation rather than inspection: with the authorization removed from GetMessage the test still passed. With valid shell-output metadata in the fixture it returns 200 and the victim's stdout, so the test fails as intended; restoring the guard makes it pass again. Also mutation-tested the sibling denial tests, since this is a defect class rather than a one-off: - TestHTTPGetTaskSessionDeniesForeignSessionWith404 — sound, returns 200 with the session DTO when the guard is removed. - TestProcessRoutesDenyForeignSession — catches removal, though via the documented nil-lifecycleMgr panic rather than a status assertion. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(auth): add an in-test witness to the shell-output scoping test Review suggestion. The fixture fix made the test exercise the guard, but nothing in the test proved the 404 came from authorization rather than the route's post-service `!ok` gate — which is exactly how it went vacuous in the first place. The evidence lived in a commit message describing a manual mutation run, not in the test. Now asserts both directions, matching the approve-route test: the non-owner gets 404, and the owner does NOT. If the assertion ever stops proving authorization, the owner case fails and says so. Re-verified by mutation: removing the guard from GetMessage fails the test with 200 and the victim's stdout; restoring it passes. 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>
This commit is contained in:
@@ -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.
|
||||
|
||||
|
||||
@@ -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")
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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,
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -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
|
||||
}
|
||||
@@ -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")
|
||||
}
|
||||
}
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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")
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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)
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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"`
|
||||
}
|
||||
|
||||
@@ -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
|
||||
}
|
||||
@@ -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
|
||||
}
|
||||
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
|
||||
|
||||
@@ -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")
|
||||
}
|
||||
}
|
||||
@@ -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
|
||||
|
||||
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user