fix(auth): scope the SSR terminal-listing routes to the caller (#3029)
Co-authored-by: Kandev Agent <agent@kandev.dev>
This commit is contained in:
@@ -177,7 +177,7 @@ Standalone agentctl is launched in its own process group so terminal Ctrl+C is h
|
||||
- **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.
|
||||
- **The workflow-step surface scopes itself in `internal/workflow`.** Steps, workspace step lists, export and import all reach a workflow or workspace by caller-supplied ID, and none of those IDs is a name the WS dispatch backstop parses, so the guard has to be in this package. `internal/workflow/service/access.go` holds `AuthorizeWorkflow` / `AuthorizeWorkspace` / `AuthorizeStep`, backed by `SetWorkflowAccessChecker` / `SetWorkspaceAccessChecker` (wired to `taskSvc.AuthorizeWorkflowAccess` / `AuthorizeWorkspaceAccess` in `backendapp/services.go`, next to the older `SetSessionAccessChecker`). Step CRUD authorizes in the **controller**, because REST, WS and the MCP tools all share it; `ListStepsByWorkspaceID` and the export/import trio authorize in the **service**, because the boot-payload builder and the MCP config tools call those directly. A denial and a genuine miss both surface as `service.ErrNotVisible` → one 404, so a foreign step and a nonexistent one are indistinguishable. Templates are deliberately unscoped (install-global, ownerless). Note that an empty ID **fails closed** here rather than being passed to `AuthorizeWorkspaceAccess`, which reads `""` as "no scoping applies". A step also *contains* IDs — `move_to_step` names a step, `queue_run` names a task — and the engine dereferences them later on the event bus, with no identity to check against, so they are validated at write time: `models.CollectStepEventReferences` enumerates them (keep it in sync with `RemapStepEvents` when adding a trigger), a transition target must resolve to a step in the same workflow, and a task target is authorized like any other. A reorder's step IDs must belong to the workflow named in the URL, which is also what keeps a read-only workflow's steps from being reordered through a mutable workflow's route.
|
||||
- **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`, plus `id` on **top-level `task.<verb>` actions only** (`task.state`, `task.move`, `task.get`, … — `task.` and exactly one more segment). 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. `task.state` and `task.move` were the second instance: they name the task `id`, so the backstop parsed no refs and let one user mutate any other user's task — hence the `id` rule.) Deeper `task.*` namespaces are deliberately excluded: in `task.plan.revision.get` and `task.review.finding.update` the `id` names a revision or a finding, so those actions must carry `task_id` when they are task-scoped. 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.
|
||||
- **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`, plus `id` on **top-level `task.<verb>` actions only** (`task.state`, `task.move`, `task.get`, … — `task.` and exactly one more segment). 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. `task.state` and `task.move` were the second instance: they name the task `id`, so the backstop parsed no refs and let one user mutate any other user's task — hence the `id` rule.) Deeper `task.*` namespaces are deliberately excluded: in `task.plan.revision.get` and `task.review.finding.update` the `id` names a revision or a finding, so those actions must carry `task_id` when they are task-scoped. 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. **The backstop does not cover HTTP.** The dispatch backstop only sees WS payloads, so an HTTP route that names a task or environment has to guard itself, and only the third-party integration prefixes have a global middleware. The two SSR terminal lists (`GET /api/v1/environments/:id/terminals`, `GET /api/v1/tasks/:id/terminals`) were the gap: they read straight from the interactive runner and the terminal service (which has no authorization of its own), leaking another user's terminal display names and initial command lines. They now mount `authorizeEnvironmentRoute` / `authorizeTaskRoute` (`internal/agent/handlers/shell_handlers.go`) as gin route guards calling `lifecycleMgr.CheckEnvironmentAccess` / `CheckTaskAccess`, so the check runs before any terminal state is read. Guard **every** ID the handler consumes, not just the path param: `/tasks/:id/terminals` also takes `?task_environment_id=`, which is a second way into a foreign environment. When a handler merges state keyed by two IDs, authorize them as a **pair** (`AuthorizeTaskEnvironmentAccess`, the task/environment sibling of `AuthorizeTaskSessionAccess`) rather than independently: both single-ID checks pass for a caller who holds each ID separately, and the handler then merges two unrelated lists. Pair on the binding, not on row ownership: `inherit_parent` binds a subtask's session to the parent task's environment and `shared_group` binds every group member to one canonical environment, so `env.TaskID == taskID` is the wrong predicate and would empty the terminal panel for both modes.
|
||||
- **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. Subscription actions (`*.subscribe`) `return` before reaching the dispatch backstop (`Client.authorizeAction`), so scoping a new subscribe topic means adding a hook to `SubscriptionAccessPolicy` (`internal/gateway/websocket/access.go`) *and* calling it explicitly inside that action's own handler — the backstop's `scopedActionRefs` cannot reach it. `run.subscribe`'s `Subscriptions.Run` hook (added for WO-02, resolving the run's workspace via `runs.GetRunWorkspaceID`) is the worked example.
|
||||
- **Task overview vs. session detail:** Keep cross-task state in the bounded, persisted `TaskStatusSummary` projection and `task.status_summary.updated`; keep unbounded transcript and execution data session-scoped. The [bounded task-status spec](../../docs/specs/platform/requirements/bounded-task-status-delivery.md) and [accepted ADR](../../docs/decisions/2026-08-01-separate-task-summary-session-stream-traffic.md) define the shared contract.
|
||||
- **Port-proxy subtree capability:** with auth enforced, `/port-proxy/:sessionId/:port/*` needs a credential on *every* request, but browser subresource fetches cannot always carry one (the `<link rel="manifest">` fetch never sends cookies; sandboxed iframes and `?token=`-opened previews drop them elsewhere). `PortProxyHandler` mints a short-lived (15 min, sliding), path-scoped, HMAC-signed capability after the document authenticates and propagates it as a cookie scoped to the proxy subtree plus a `kandev_cap` query parameter appended to rewritten asset URLs. `requireConnectionAuth` (`internal/gateway/websocket/access.go`) accepts either form only for the exact session:port it was minted for and restores the issuing identity, so `CheckSessionAccess` still gates on the real owner. Synthetic identities (auth disabled) skip minting — proxied bodies stay byte-identical to the pre-auth behavior.
|
||||
|
||||
Reference in New Issue
Block a user