fix(auth): scope the workflow-step surface to the workspace owner (#3031)

Co-authored-by: Kandev Agent <agent@kandev.dev>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
João Salavisa
2026-08-25 13:34:48 +01:00
committed by GitHub
parent c3b5408f19
commit 7d9f81be84
18 changed files with 2162 additions and 21 deletions
+1
View File
@@ -176,6 +176,7 @@ Standalone agentctl is launched in its own process group so terminal Ctrl+C is h
- **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. 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.
- **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.