diff --git a/apps/backend/AGENTS.md b/apps/backend/AGENTS.md index 2aacbb2dc4..18ca37d796 100644 --- a/apps/backend/AGENTS.md +++ b/apps/backend/AGENTS.md @@ -180,7 +180,7 @@ and can leak ACP subprocesses. - **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. -- **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. +- **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.` 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. - **Self-authenticating webhooks** (automation, office channels, plugin webhooks) and `/health`, `/api/v1/features`, `/api/v1/app-state` stay public — the allowlist lives in `auth/httpmw/middleware.go` with a pinning test. diff --git a/apps/backend/internal/backendapp/helpers.go b/apps/backend/internal/backendapp/helpers.go index d9bead270a..9f3becb9d2 100644 --- a/apps/backend/internal/backendapp/helpers.go +++ b/apps/backend/internal/backendapp/helpers.go @@ -599,6 +599,9 @@ func registerRoutes(p routeParams) { // the kanban board doesn't react to subtree archive/delete until a // full reload. handoffSvc.SetTaskEventPublisher(p.taskSvc) + // Per-user scoping for the cascade is installed by + // TaskHandlers.SetHandoffService, which is the call that makes the archive / + // delete routes prefer the cascade over the guarded Service methods. if p.services.Office != nil { p.services.Office.SetWorkspaceGroupCleaner(handoffSvc) } diff --git a/apps/backend/internal/gateway/websocket/client.go b/apps/backend/internal/gateway/websocket/client.go index c5005aef34..b0635afe75 100644 --- a/apps/backend/internal/gateway/websocket/client.go +++ b/apps/backend/internal/gateway/websocket/client.go @@ -237,7 +237,7 @@ func (c *Client) handleMessage(msg *ws.Message) { // 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 { + if err := c.authorizeAction(dispatchCtx, msg.Action, msg.Payload); err != nil { c.logger.Debug("denied out-of-scope action", zap.String("action", msg.Action), zap.String("user_id", c.identity.UserID), diff --git a/apps/backend/internal/gateway/websocket/dispatch_scope.go b/apps/backend/internal/gateway/websocket/dispatch_scope.go index b0ed74e898..10ce6aacc2 100644 --- a/apps/backend/internal/gateway/websocket/dispatch_scope.go +++ b/apps/backend/internal/gateway/websocket/dispatch_scope.go @@ -3,6 +3,7 @@ package websocket import ( "context" "encoding/json" + "strings" "github.com/kandev/kandev/internal/auth/authn" ) @@ -26,16 +27,22 @@ import ( // scopedActionRefs are the payload fields that name a per-user resource. // -// All three names must stay in sync with what handlers actually send. The +// All four 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 +// that invents a fifth name for the same kind of resource silently opts out // of this backstop — see AGENTS.md. +// +// ID is the same escape hatch found a second time: task.state and task.move +// name their task `id`, so a task_id-only backstop parsed no refs and let one +// user mutate any other user's task. It is read only for top-level task. +// actions — see isTopLevelTaskAction. type scopedActionRefs struct { TaskID string `json:"task_id"` SessionID string `json:"session_id"` TaskEnvironmentID string `json:"task_environment_id"` + ID string `json:"id"` } // authorizeAction reports why a dispatched action must be refused, or nil when @@ -44,7 +51,7 @@ type scopedActionRefs struct { // 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 { +func (c *Client) authorizeAction(ctx context.Context, action string, payload json.RawMessage) error { if c.identity.UserID == "" || c.identity.Synthetic { return nil } @@ -52,14 +59,18 @@ func (c *Client) authorizeAction(ctx context.Context, payload json.RawMessage) e if !ok { return nil } + taskID := refs.TaskID + if taskID == "" && isTopLevelTaskAction(action) { + taskID = refs.ID + } 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 { + if taskID != "" && policy.Subscriptions.Task != nil { + if err := policy.Subscriptions.Task(ctx, taskID); err != nil { return err } } @@ -83,7 +94,25 @@ func parseScopedActionRefs(payload json.RawMessage) (scopedActionRefs, bool) { // Non-object payloads (arrays, bare values) name no resource. return scopedActionRefs{}, false } - return refs, refs.TaskID != "" || refs.SessionID != "" || refs.TaskEnvironmentID != "" + return refs, refs.TaskID != "" || refs.SessionID != "" || refs.TaskEnvironmentID != "" || refs.ID != "" +} + +// isTopLevelTaskAction reports whether an action's payload `id` names a task. +// +// True for "task." plus exactly one more segment — task.get, task.update, +// task.delete, task.move, task.state, task.archive, and any future sibling. +// Deliberately a rule about namespace depth rather than a list of action names: +// a list has the same "someone forgot to add it" failure mode this backstop +// exists to remove. +// +// Deeper namespaces are excluded because their `id` names something else: +// task.plan.revision.get carries a revision ID and task.review.finding.update a +// finding ID, so checking either as a task ID would deny legitimate calls. +// Those actions carry task_id when they are task-scoped. +func isTopLevelTaskAction(action string) bool { + const prefix = "task." + rest, found := strings.CutPrefix(action, prefix) + return found && !strings.Contains(rest, ".") } // identityIsScoped reports whether this client's identity participates in diff --git a/apps/backend/internal/gateway/websocket/dispatch_scope_test.go b/apps/backend/internal/gateway/websocket/dispatch_scope_test.go index 3c0d100ee9..0aaedbfefe 100644 --- a/apps/backend/internal/gateway/websocket/dispatch_scope_test.go +++ b/apps/backend/internal/gateway/websocket/dispatch_scope_test.go @@ -18,8 +18,9 @@ import ( // 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 + client *Client + handled *bool + dispatcher *ws.Dispatcher } func newDispatchScopeFixture(t *testing.T, identity authn.Identity) *dispatchScopeFixture { @@ -65,7 +66,22 @@ func newDispatchScopeFixture(t *testing.T, identity authn.Identity) *dispatchSco c := newTestClient("c-scope") c.hub = h c.identity = identity - return &dispatchScopeFixture{client: c, handled: &handled} + return &dispatchScopeFixture{client: c, handled: &handled, dispatcher: dispatcher} +} + +// dispatchAs sends payload under a real action name, so tests can exercise the +// rules that key off the action rather than only the payload. +func (f *dispatchScopeFixture) dispatchAs(t *testing.T, action, payload string) { + t.Helper() + f.dispatcher.RegisterFunc(action, func(context.Context, *ws.Message) (*ws.Message, error) { + *f.handled = true + return nil, nil + }) + f.client.handleMessage(&ws.Message{ + Action: action, + Type: ws.MessageTypeRequest, + Payload: json.RawMessage(payload), + }) } func (f *dispatchScopeFixture) dispatch(t *testing.T, payload string) { @@ -198,25 +214,30 @@ func TestParseScopedActionRefs(t *testing.T) { wantTask string wantSess string wantEnv string + wantID 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, "", "", ""}, + {`{"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", ""}, + {`{"id":"t"}`, true, "", "", "", "t"}, + {`{"id":"t","state":"COMPLETED"}`, true, "", "", "", "t"}, + {`{}`, false, "", "", "", ""}, + {`{"task_id":""}`, false, "", "", "", ""}, + {`{"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) + if refs.TaskID != tc.wantTask || refs.SessionID != tc.wantSess || + refs.TaskEnvironmentID != tc.wantEnv || refs.ID != tc.wantID { + t.Errorf("payload %q refs = %+v, want task=%q session=%q env=%q id=%q", + tc.payload, refs, tc.wantTask, tc.wantSess, tc.wantEnv, tc.wantID) } } } @@ -267,3 +288,107 @@ func TestDispatchScopeAllowsOwnEnvironment(t *testing.T) { t.Error("handler did not run for the caller's own task environment") } } + +// A top-level task. action names its task `id`, not `task_id` — which is +// how task.state and task.move mutated any user's task with the backstop finding +// no refs to check and allowing the dispatch. +// +// The rule keys off namespace depth rather than a list of action names, so a +// future task. is covered without an edit. Deeper namespaces are excluded +// on purpose: their `id` names a plan, revision or finding, and checking it as a +// task ID would deny legitimate reads — see the allow cases below. +func TestDispatchScopeDeniesForeignTaskByIDOnTopLevelTaskActions(t *testing.T) { + for _, action := range []string{ws.ActionTaskState, ws.ActionTaskMove, ws.ActionTaskArchive, ws.ActionTaskGet} { + t.Run(action, func(t *testing.T) { + f := newDispatchScopeFixture(t, realIdentity()) + + f.dispatchAs(t, action, `{"id":"task-b","state":"COMPLETED"}`) + + if *f.handled { + t.Errorf("handler ran for %s naming a foreign task by id", action) + } + }) + } +} + +func TestDispatchScopeAllowsOwnTaskByIDOnTopLevelTaskActions(t *testing.T) { + f := newDispatchScopeFixture(t, realIdentity()) + + f.dispatchAs(t, ws.ActionTaskState, `{"id":"task-a","state":"COMPLETED"}`) + + if !*f.handled { + t.Error("the owner's own task.state was denied; the rule must not be a blanket deny") + } +} + +// TestDispatchScopeIgnoresIDOnNestedTaskActions is the regression this rule +// could plausibly cause: in task.plan.* and task.review.* the `id` is a plan, +// revision or finding ID, and treating it as a task ID would refuse every read. +func TestDispatchScopeIgnoresIDOnNestedTaskActions(t *testing.T) { + nested := map[string]string{ + ws.ActionTaskPlanRevisionGet: `{"id":"revision-7"}`, + ws.ActionTaskReviewFindingUpdate: `{"id":"finding-3","status":"resolved"}`, + ws.ActionTaskPlanGet: `{"id":"plan-1"}`, + } + for action, payload := range nested { + t.Run(action, func(t *testing.T) { + f := newDispatchScopeFixture(t, realIdentity()) + + f.dispatchAs(t, action, payload) + + if !*f.handled { + t.Errorf("%s was denied: its id names a nested resource, not a task", action) + } + }) + } +} + +// TestDispatchScopeIgnoresIDOnNonTaskActions keeps every other namespace out of +// the rule — `id` there is a workflow, executor or agent profile. +func TestDispatchScopeIgnoresIDOnNonTaskActions(t *testing.T) { + for _, action := range []string{ws.ActionWorkflowGet, "executor.get", "tasks.something"} { + t.Run(action, func(t *testing.T) { + f := newDispatchScopeFixture(t, realIdentity()) + + f.dispatchAs(t, action, `{"id":"task-b"}`) + + if !*f.handled { + t.Errorf("%s was denied; its id does not name a task", action) + } + }) + } +} + +// task.create / task.list carry no id, so the rule is a no-op for them. +func TestDispatchScopeAllowsTopLevelTaskActionsWithoutID(t *testing.T) { + f := newDispatchScopeFixture(t, realIdentity()) + + f.dispatchAs(t, ws.ActionTaskCreate, `{"workspace_id":"ws-a","title":"New"}`) + + if !*f.handled { + t.Error("task.create names no task id and must dispatch") + } +} + +func TestIsTopLevelTaskAction(t *testing.T) { + cases := map[string]bool{ + ws.ActionTaskState: true, + ws.ActionTaskMove: true, + ws.ActionTaskGet: true, + ws.ActionTaskArchive: true, + ws.ActionTaskPlanGet: false, + ws.ActionTaskPlanRevisionGet: false, + ws.ActionTaskReviewFindingUpdate: false, + ws.ActionTaskSessionList: false, + ws.ActionWorkflowGet: false, + "task": false, + "task.": true, + "tasks.get": false, + "": false, + } + for action, want := range cases { + if got := isTopLevelTaskAction(action); got != want { + t.Errorf("isTopLevelTaskAction(%q) = %v, want %v", action, got, want) + } + } +} diff --git a/apps/backend/internal/task/handlers/task_delete_authz_test.go b/apps/backend/internal/task/handlers/task_delete_authz_test.go new file mode 100644 index 0000000000..afdfc665a0 --- /dev/null +++ b/apps/backend/internal/task/handlers/task_delete_authz_test.go @@ -0,0 +1,256 @@ +package handlers + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "sync" + "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" + ws "github.com/kandev/kandev/pkg/websocket" +) + +// DELETE /api/v1/tasks/:id built its context with context.WithTimeout( +// context.Background(), …). context.Background() carries no identity, so the +// service's authorizeTaskID saw an identity-less internal caller and permitted +// deleting anyone's task. The sibling archive route uses c.Request.Context() and +// answers 404, which is the shape of the fix — but the request context is +// cancelled when the client disconnects, and this route detaches from that on +// purpose, so the fix is context.WithoutCancel: keep the values, drop the cancel. +// +// Symbols here are authz-prefixed so they cannot collide with the fixtures PR +// #2500 adds to this package while it is unmerged. + +// authzDeleteRepo serves one task owned by user-b and records every delete the +// handler managed to issue. +type authzDeleteRepo struct { + mockRepository + + mu sync.Mutex + // deleted records each delete alongside the liveness of the context it + // arrived with. A cancelled context means the caller handed us one that a + // client disconnect had already killed. + deleted []string + deleteErr []error + archived []string +} + +func (r *authzDeleteRepo) GetTask(_ context.Context, id string) (*models.Task, error) { + return &models.Task{ID: id, WorkspaceID: "ws-b", Title: "Victim"}, nil +} + +func (r *authzDeleteRepo) GetWorkspace(_ context.Context, id string) (*models.Workspace, error) { + return &models.Workspace{ID: id, Name: "B's", OwnerID: "user-b"}, nil +} + +func (r *authzDeleteRepo) ArchiveTaskIfActive(_ context.Context, id, cascadeID string) (bool, error) { + r.mu.Lock() + defer r.mu.Unlock() + r.archived = append(r.archived, id+"/"+cascadeID) + return true, nil +} + +func (r *authzDeleteRepo) archives() []string { + r.mu.Lock() + defer r.mu.Unlock() + return append([]string(nil), r.archived...) +} + +func (r *authzDeleteRepo) DeleteTask(ctx context.Context, id string) error { + r.mu.Lock() + defer r.mu.Unlock() + r.deleted = append(r.deleted, id) + r.deleteErr = append(r.deleteErr, ctx.Err()) + return nil +} + +func (r *authzDeleteRepo) ListChildren(context.Context, string) ([]*models.Task, error) { + return nil, nil +} + +func (r *authzDeleteRepo) ListChildrenIncludingArchived(context.Context, string) ([]*models.Task, error) { + return nil, nil +} + +func (r *authzDeleteRepo) CountToolCallMessagesBySession(context.Context, []string) (map[string]int, error) { + return nil, nil +} + +func (r *authzDeleteRepo) deletes() []string { + r.mu.Lock() + defer r.mu.Unlock() + return append([]string(nil), r.deleted...) +} + +// deliveredLive reports whether every recorded delete arrived on a context that +// was still alive. A real repository would abort on a cancelled one; the fake +// cannot, so the liveness is asserted directly. +func (r *authzDeleteRepo) deliveredLive() bool { + r.mu.Lock() + defer r.mu.Unlock() + for _, err := range r.deleteErr { + if err != nil { + return false + } + } + return len(r.deleteErr) > 0 +} + +func newAuthzTaskHandlers(t *testing.T, repo *authzDeleteRepo) *TaskHandlers { + t.Helper() + log := newTestLogger(t) + 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, repo: repo, logger: log} +} + +// authzDeleteRequest builds a DELETE for taskID carrying userID's identity. +func authzDeleteRequest(t *testing.T, userID, taskID string) (*gin.Context, *httptest.ResponseRecorder) { + t.Helper() + gin.SetMode(gin.TestMode) + rec := httptest.NewRecorder() + c, _ := gin.CreateTestContext(rec) + req := httptest.NewRequest(http.MethodDelete, "/api/v1/tasks/"+taskID, nil) + req = req.WithContext(authn.WithIdentity( + req.Context(), authn.Identity{UserID: userID, Role: authn.RoleMember})) + c.Request = req + c.Params = gin.Params{{Key: "id", Value: taskID}} + return c, rec +} + +func TestHTTPDeleteTaskDeniesForeignTask(t *testing.T) { + repo := &authzDeleteRepo{} + h := newAuthzTaskHandlers(t, repo) + + c, rec := authzDeleteRequest(t, "user-a", "task-b") + h.httpDeleteTask(c) + + if rec.Code != http.StatusNotFound { + t.Fatalf("foreign delete: status = %d body = %s, want 404", rec.Code, rec.Body.String()) + } + if got := repo.deletes(); len(got) != 0 { + t.Fatalf("a denied delete reached the repository: %v", got) + } + + // The owner must still be able to delete, otherwise a 404 for everyone + // would pass this test just as well. + ownerCtx, ownerRec := authzDeleteRequest(t, "user-b", "task-b") + h.httpDeleteTask(ownerCtx) + + if ownerRec.Code != http.StatusOK { + t.Fatalf("owner delete: status = %d body = %s, want 200", ownerRec.Code, ownerRec.Body.String()) + } + if got := repo.deletes(); len(got) != 1 || got[0] != "task-b" { + t.Fatalf("owner delete did not reach the repository: %v", got) + } +} + +// TestHTTPDeleteTaskDeniesForeignTaskThroughCascade covers the shape backendapp +// actually ships: with a HandoffService wired the handler never calls +// Service.DeleteTask, so the guard that carries this route lives on the cascade. +// +// SetHandoffService is called exactly as production calls it — nothing here +// installs the access checker by hand — so removing that wiring fails this test. +func TestHTTPDeleteTaskDeniesForeignTaskThroughCascade(t *testing.T) { + repo := &authzDeleteRepo{} + h := newAuthzTaskHandlers(t, repo) + h.SetHandoffService(service.NewHandoffService(repo, nil, nil, nil, nil, nil)) + + c, rec := authzDeleteRequest(t, "user-a", "task-b") + h.httpDeleteTask(c) + + if rec.Code != http.StatusNotFound { + t.Fatalf("foreign cascade delete: status = %d body = %s, want 404", rec.Code, rec.Body.String()) + } + if got := repo.deletes(); len(got) != 0 { + t.Fatalf("a denied cascade delete reached the repository: %v", got) + } + + ownerCtx, ownerRec := authzDeleteRequest(t, "user-b", "task-b") + h.httpDeleteTask(ownerCtx) + + if ownerRec.Code != http.StatusOK { + t.Fatalf("owner cascade delete: status = %d body = %s, want 200", ownerRec.Code, ownerRec.Body.String()) + } + if got := repo.deletes(); len(got) != 1 || got[0] != "task-b" { + t.Fatalf("owner cascade delete did not reach the repository: %v", got) + } +} + +// TestWSArchiveTaskDeniesForeignTaskThroughCascade covers the third route the +// cascade substitution unscoped: wsArchiveTask also prefers HandoffService when +// one is wired, so it never reaches Service.ArchiveTask's guard either. +func TestWSArchiveTaskDeniesForeignTaskThroughCascade(t *testing.T) { + repo := &authzDeleteRepo{} + h := newAuthzTaskHandlers(t, repo) + h.SetHandoffService(service.NewHandoffService(repo, nil, nil, nil, nil, nil)) + + msg := &ws.Message{ + ID: "msg-1", + Action: ws.ActionTaskArchive, + Payload: json.RawMessage(`{"id":"task-b"}`), + } + + denied, err := h.wsArchiveTask(authzWSContext("user-a"), msg) + if err != nil { + t.Fatalf("wsArchiveTask: %v", err) + } + if denied.Type != ws.MessageTypeError { + t.Fatalf("foreign archive: type = %s payload = %s, want error", denied.Type, denied.Payload) + } + if got := repo.archives(); len(got) != 0 { + t.Fatalf("a denied archive reached the repository: %v", got) + } + + owner, err := h.wsArchiveTask(authzWSContext("user-b"), msg) + if err != nil { + t.Fatalf("owner archive: %v", err) + } + if owner.Type == ws.MessageTypeError { + t.Fatalf("owner archive was refused: %s", owner.Payload) + } + if got := repo.archives(); len(got) != 1 { + t.Fatalf("owner archive did not reach the repository: %v", got) + } +} + +// TestHTTPDeleteTaskSurvivesClientDisconnect is why the fix is +// context.WithoutCancel rather than a plain c.Request.Context() swap: a delete +// tears down worktrees, containers and subtree rows, and a browser that +// navigates away mid-request must not abort it half-done. +func TestHTTPDeleteTaskSurvivesClientDisconnect(t *testing.T) { + repo := &authzDeleteRepo{} + h := newAuthzTaskHandlers(t, repo) + + c, rec := authzDeleteRequest(t, "user-b", "task-b") + gone, disconnect := context.WithCancel(c.Request.Context()) + c.Request = c.Request.WithContext(gone) + disconnect() + + h.httpDeleteTask(c) + + if rec.Code != http.StatusOK { + t.Fatalf("disconnected delete: status = %d body = %s, want 200", rec.Code, rec.Body.String()) + } + if got := repo.deletes(); len(got) != 1 || got[0] != "task-b" { + t.Fatalf("a client disconnect aborted the delete: %v", got) + } + // The status code alone proves nothing here — this fake repository ignores + // its context, so a cancelled one would still "succeed". Assert the context + // the delete was actually handed is live, which is what a real repository, + // worktree teardown or container removal depends on. + if !repo.deliveredLive() { + t.Fatal("the delete was handed a cancelled context; a client disconnect can abort it half-done") + } +} diff --git a/apps/backend/internal/task/handlers/task_handlers.go b/apps/backend/internal/task/handlers/task_handlers.go index 8584186187..68f8be1010 100644 --- a/apps/backend/internal/task/handlers/task_handlers.go +++ b/apps/backend/internal/task/handlers/task_handlers.go @@ -66,8 +66,20 @@ type taskCreateLastUsedRecorder interface { // Kanban subtask path to attach workspace-group membership and the // sequential blocker chain (handoffs phase 5). Optional — nil disables // post-create attachment, matching the pre-handoffs behaviour. +// +// Wiring a HandoffService also re-installs the per-user task guard on it. That +// is not a convenience: this setter is what makes the archive / delete / unarchive +// routes call the cascade *instead of* Service.ArchiveTask / DeleteTask, and the +// cascade walks the task repository directly, so it inherits none of their +// authorizeTaskID checks. Installing the checker here means the substitution +// cannot silently unscope those routes. The checker is a no-op for identity-less +// internal callers (the integration watch-reset paths), exactly as it is +// everywhere else. func (h *TaskHandlers) SetHandoffService(svc *service.HandoffService) { h.handoffSvc = svc + if svc != nil && h.service != nil { + svc.SetTaskAccessChecker(h.service.AuthorizeTaskAccess) + } } func (h *TaskHandlers) SetWorkspaceQuarantineRestorer(restorer WorkspaceQuarantineRestorer) { diff --git a/apps/backend/internal/task/handlers/task_http_handlers.go b/apps/backend/internal/task/handlers/task_http_handlers.go index 6a78443543..f23797e137 100644 --- a/apps/backend/internal/task/handlers/task_http_handlers.go +++ b/apps/backend/internal/task/handlers/task_http_handlers.go @@ -1652,7 +1652,14 @@ func (h *TaskHandlers) httpMoveTask(c *gin.Context) { } func (h *TaskHandlers) httpDeleteTask(c *gin.Context) { - deleteCtx, cancel := context.WithTimeout(context.Background(), constants.TaskDeleteTimeout) + // WithoutCancel, not Background: a delete tears down worktrees, containers + // and subtree rows, so it must survive the client navigating away — but + // context.Background() also dropped the caller identity the auth middleware + // attached, and an identity-free context reads as an internal caller, which + // authorizes everything. WithoutCancel keeps the request's values and drops + // only its cancellation. + deleteCtx, cancel := context.WithTimeout( + context.WithoutCancel(c.Request.Context()), constants.TaskDeleteTimeout) defer cancel() taskID := c.Param("id") cascade := cascadeQueryParam(c) diff --git a/apps/backend/internal/task/handlers/task_ws_authz_test.go b/apps/backend/internal/task/handlers/task_ws_authz_test.go new file mode 100644 index 0000000000..26b5532306 --- /dev/null +++ b/apps/backend/internal/task/handlers/task_ws_authz_test.go @@ -0,0 +1,180 @@ +package handlers + +import ( + "context" + "encoding/json" + "strings" + "sync" + "testing" + + "github.com/kandev/kandev/internal/auth/authn" + "github.com/kandev/kandev/internal/task/models" + taskrepo "github.com/kandev/kandev/internal/task/repository/sqlite" + "github.com/kandev/kandev/internal/task/service" + v1 "github.com/kandev/kandev/pkg/api/v1" + ws "github.com/kandev/kandev/pkg/websocket" +) + +// The WS half of the same gaps. task.state and task.move name their task `id`, +// which is what let them past the gateway backstop; these assert the handlers +// refuse regardless, since the service guard is the actual fix and the handler +// is also what a non-gateway caller reaches. +// +// The denial assertion is deliberately a *comparison* rather than a fixed error +// code: what matters is that a foreign task is indistinguishable from one that +// does not exist, which is what these handlers already emit for any service +// error. + +// authzWSRepo serves exactly one task — user-b's — and records every mutating +// write. Any other ID is genuinely absent, so "foreign" and "missing" can be +// compared against each other. +type authzWSRepo struct { + mockRepository + + mu sync.Mutex + states []string + positions []string +} + +func (r *authzWSRepo) GetTask(_ context.Context, id string) (*models.Task, error) { + if id != "task-b" { + return nil, taskrepo.ErrTaskNotFound + } + return &models.Task{ + ID: "task-b", WorkspaceID: "ws-b", Title: "Victim", + WorkflowID: "wf-b", WorkflowStepID: "step-b-1", State: v1.TaskStateTODO, + }, nil +} + +func (r *authzWSRepo) GetWorkspace(_ context.Context, id string) (*models.Workspace, error) { + return &models.Workspace{ID: id, Name: "B's", OwnerID: "user-b"}, nil +} + +func (r *authzWSRepo) UpdateTaskState(_ context.Context, id string, state v1.TaskState) error { + r.mu.Lock() + defer r.mu.Unlock() + r.states = append(r.states, id+"="+string(state)) + return nil +} + +func (r *authzWSRepo) UpdateTask(_ context.Context, task *models.Task) error { + r.mu.Lock() + defer r.mu.Unlock() + r.positions = append(r.positions, task.ID+"@"+task.WorkflowStepID) + return nil +} + +func (r *authzWSRepo) writes() int { + r.mu.Lock() + defer r.mu.Unlock() + return len(r.states) + len(r.positions) +} + +func (r *authzWSRepo) CountToolCallMessagesBySession(context.Context, []string) (map[string]int, error) { + return nil, nil +} + +func (r *authzWSRepo) ListChildren(context.Context, string) ([]*models.Task, error) { + return nil, nil +} + +func newAuthzWSHandlers(t *testing.T, repo *authzWSRepo) *TaskHandlers { + t.Helper() + log := newTestLogger(t) + 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, repo: repo, logger: log} +} + +func authzWSContext(userID string) context.Context { + return authn.WithIdentity(context.Background(), + authn.Identity{UserID: userID, Role: authn.RoleMember}) +} + +func authzWSMessage(t *testing.T, action string, payload map[string]any) *ws.Message { + t.Helper() + raw, err := json.Marshal(payload) + if err != nil { + t.Fatalf("marshal payload: %v", err) + } + return &ws.Message{ID: "msg-1", Action: action, Payload: raw} +} + +// authzWSCall names one entry point and how to invoke it against a task ID. +type authzWSCall struct { + action string + invoke func(*TaskHandlers, context.Context, string) (*ws.Message, error) +} + +func authzWSCalls(t *testing.T) []authzWSCall { + t.Helper() + return []authzWSCall{ + {ws.ActionTaskState, func(h *TaskHandlers, ctx context.Context, id string) (*ws.Message, error) { + return h.wsUpdateTaskState(ctx, authzWSMessage(t, ws.ActionTaskState, + map[string]any{"id": id, "state": string(v1.TaskStateCompleted)})) + }}, + {ws.ActionTaskMove, func(h *TaskHandlers, ctx context.Context, id string) (*ws.Message, error) { + return h.wsMoveTask(ctx, authzWSMessage(t, ws.ActionTaskMove, map[string]any{ + "id": id, "workflow_id": "wf-b", "workflow_step_id": "step-b-2", "position": 3, + })) + }}, + } +} + +func TestWSTaskStateAndMoveDenyForeignTask(t *testing.T) { + for _, tc := range authzWSCalls(t) { + t.Run(tc.action, func(t *testing.T) { + repo := &authzWSRepo{} + h := newAuthzWSHandlers(t, repo) + + denied, err := tc.invoke(h, authzWSContext("user-a"), "task-b") + if err != nil { + t.Fatalf("%s: %v", tc.action, err) + } + if denied.Type != ws.MessageTypeError { + t.Fatalf("foreign %s: type = %s payload = %s, want error", + tc.action, denied.Type, denied.Payload) + } + if strings.Contains(string(denied.Payload), "Victim") { + t.Fatalf("the foreign task leaked into the response: %s", denied.Payload) + } + if n := repo.writes(); n != 0 { + t.Fatalf("a denied %s reached the repository (%d writes)", tc.action, n) + } + + // A denial must read exactly like a task that does not exist. + missing, err := tc.invoke(h, authzWSContext("user-a"), "task-nonexistent") + if err != nil { + t.Fatalf("%s (unknown task): %v", tc.action, err) + } + if string(missing.Payload) != string(denied.Payload) { + t.Fatalf("denial is distinguishable from a missing task:\n denied = %s\n missing = %s", + denied.Payload, missing.Payload) + } + }) + } +} + +// TestWSTaskStateAllowsOwner is the witness for the denial above: without it the +// test would still pass if the guard refused everyone. +func TestWSTaskStateAllowsOwner(t *testing.T) { + repo := &authzWSRepo{} + h := newAuthzWSHandlers(t, repo) + + resp, err := h.wsUpdateTaskState(authzWSContext("user-b"), authzWSMessage(t, ws.ActionTaskState, + map[string]any{"id": "task-b", "state": string(v1.TaskStateCompleted)})) + if err != nil { + t.Fatalf("owner task.state: %v", err) + } + if resp.Type == ws.MessageTypeError { + t.Fatalf("owner task.state was refused: %s", resp.Payload) + } + if repo.writes() == 0 { + t.Fatal("owner task.state did not reach the repository") + } +} diff --git a/apps/backend/internal/task/service/handoff_cascade.go b/apps/backend/internal/task/service/handoff_cascade.go index 9b92198624..e9961a4721 100644 --- a/apps/backend/internal/task/service/handoff_cascade.go +++ b/apps/backend/internal/task/service/handoff_cascade.go @@ -176,6 +176,9 @@ type CascadeOutcome struct { // archived, stamping the cascade ID on the released row. // 5. Evaluate cleanup once per affected group. func (s *HandoffService) ArchiveTaskTree(ctx context.Context, rootID string, cascade bool) (*CascadeOutcome, error) { + if err := s.authorizeTask(ctx, rootID); err != nil { + return nil, err + } if rootID == "" { return nil, errors.New("rootID is required") } @@ -278,6 +281,9 @@ func (s *HandoffService) ArchiveTaskTree(ctx context.Context, rootID string, cas // because deletion is unconditional; the cascade ID is stamped only // for symmetry with archive. func (s *HandoffService) DeleteTaskTree(ctx context.Context, rootID string, cascade bool) (*CascadeOutcome, error) { + if err := s.authorizeTask(ctx, rootID); err != nil { + return nil, err + } if rootID == "" { return nil, errors.New("rootID is required") } @@ -628,6 +634,9 @@ func (s *HandoffService) cancelActiveRuns(ctx context.Context, taskIDs []string, // archived (empty cascade id) or that belong to an earlier cascade are // left archived — fixing the resurrection bug from the original review. func (s *HandoffService) UnarchiveTaskTree(ctx context.Context, rootID string) (*CascadeOutcome, error) { + if err := s.authorizeTask(ctx, rootID); err != nil { + return nil, err + } if rootID == "" { return nil, errors.New("rootID is required") } diff --git a/apps/backend/internal/task/service/handoff_cascade_authz_test.go b/apps/backend/internal/task/service/handoff_cascade_authz_test.go new file mode 100644 index 0000000000..7611db5e2e --- /dev/null +++ b/apps/backend/internal/task/service/handoff_cascade_authz_test.go @@ -0,0 +1,124 @@ +package service + +import ( + "context" + "errors" + "testing" +) + +// Per-user scoping for the cascade entry points. +// +// backendapp always wires a HandoffService into TaskHandlers, so in a shipped +// instance DELETE /tasks/:id, POST /tasks/:id/archive and the task.archive WS +// action never reach the guarded Service.DeleteTask / Service.ArchiveTask — they +// land here instead, and this type had no identity awareness at all. Restoring +// the caller identity on the HTTP delete handler is inert without these guards. + +var errCascadeDenied = errors.New("task not found") + +// cascadeAuthzCase invokes one cascade entry point against a task ID. +type cascadeAuthzCase struct { + name string + invoke func(*HandoffService, string) error +} + +func cascadeAuthzCases() []cascadeAuthzCase { + return []cascadeAuthzCase{ + {"ArchiveTaskTree", func(s *HandoffService, id string) error { + _, err := s.ArchiveTaskTree(context.Background(), id, true) + return err + }}, + {"DeleteTaskTree", func(s *HandoffService, id string) error { + _, err := s.DeleteTaskTree(context.Background(), id, true) + return err + }}, + {"UnarchiveTaskTree", func(s *HandoffService, id string) error { + _, err := s.UnarchiveTaskTree(context.Background(), id) + return err + }}, + } +} + +// cascadeAuthzChecker admits task-mine and refuses everything else, which is +// what Service.AuthorizeTaskAccess does for a caller who owns one workspace. +func cascadeAuthzChecker(_ context.Context, taskID string) error { + if taskID == "task-mine" { + return nil + } + return errCascadeDenied +} + +func TestCascadeEntryPointsDenyForeignTask(t *testing.T) { + for _, tc := range cascadeAuthzCases() { + t.Run(tc.name, func(t *testing.T) { + tasks := newFakeTaskRepo() + tasks.addTask("task-mine", "", "ws-a") + tasks.addArchivedTask("task-b", "", "ws-b", "") + repo := &fakeDeleteRepo{fakeCascadeRepo: newCascadeRepo(tasks)} + svc := NewHandoffService(repo, nil, nil, nil, newCascadeWSGroupRepo(), nil) + svc.SetTaskAccessChecker(cascadeAuthzChecker) + + if err := tc.invoke(svc, "task-b"); !errors.Is(err, errCascadeDenied) { + t.Fatalf("foreign cascade: err = %v, want denial", err) + } + + tasks.mu.Lock() + victim, present := tasks.tasks["task-b"] + stillArchived := present && victim.ArchivedAt != nil + tasks.mu.Unlock() + if !present { + t.Fatal("a denied cascade deleted the foreign task") + } + if !stillArchived { + t.Fatal("a denied cascade unarchived the foreign task") + } + + // The owner must get past the guard, otherwise the denial above + // would also pass with the checker refusing everything. + if err := tc.invoke(svc, "task-mine"); errors.Is(err, errCascadeDenied) { + t.Fatalf("owner cascade was denied: %v", err) + } + }) + } +} + +// TestCascadeEntryPointsGuardBeforeDependencies pins guard placement: with the +// task repo nil, a guard placed after the first repo read panics instead of +// returning the denial. +func TestCascadeEntryPointsGuardBeforeDependencies(t *testing.T) { + for _, tc := range cascadeAuthzCases() { + 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) + } + }() + svc := NewHandoffService(nil, nil, nil, nil, nil, nil) + svc.SetTaskAccessChecker(cascadeAuthzChecker) + + if err := tc.invoke(svc, "task-b"); !errors.Is(err, errCascadeDenied) { + t.Fatalf("err = %v, want denial", err) + } + }) + } +} + +// TestCascadeEntryPointsUnscopedWhenUnwired pins the compatibility contract: no +// checker installed means nothing is denied by scoping, so identity-less +// internal callers and every existing bare-NewHandoffService fixture are +// unaffected. The calls may still fail on their own dependencies — this asserts +// only that the denial sentinel does not appear. +func TestCascadeEntryPointsUnscopedWhenUnwired(t *testing.T) { + for _, tc := range cascadeAuthzCases() { + t.Run(tc.name, func(t *testing.T) { + tasks := newFakeTaskRepo() + tasks.addArchivedTask("task-b", "", "ws-b", "") + repo := &fakeDeleteRepo{fakeCascadeRepo: newCascadeRepo(tasks)} + svc := NewHandoffService(repo, nil, nil, nil, newCascadeWSGroupRepo(), nil) + + if err := tc.invoke(svc, "task-b"); errors.Is(err, errCascadeDenied) { + t.Fatal("unwired checker denied the call; pre-auth behavior broken") + } + }) + } +} diff --git a/apps/backend/internal/task/service/handoff_service.go b/apps/backend/internal/task/service/handoff_service.go index 6fc618d5fc..0c8cbb014b 100644 --- a/apps/backend/internal/task/service/handoff_service.go +++ b/apps/backend/internal/task/service/handoff_service.go @@ -200,6 +200,7 @@ type HandoffService struct { runCanceller RunCanceller eventPublisher TaskEventPublisher resourceCleaner TaskResourceCleaner + taskAccessCheck func(ctx context.Context, taskID string) error logger *logger.Logger parentLock parentMutex workspaceGroupLock parentMutex @@ -278,6 +279,28 @@ func (s *HandoffService) SetTaskResourceCleaner(c TaskResourceCleaner) { s.resourceCleaner = c } +// SetTaskAccessChecker installs the per-user task check applied by the cascade +// entry points. Same contract as orchestrator.Service.SetTaskAccessChecker: the +// checker returns nil for contexts without a request identity, so internal +// callers (event bus, schedulers, workflow engine) stay unscoped, and leaving it +// unwired keeps the pre-auth see-everything behavior. +// +// The cascade methods walk the task repository directly and never reach +// Service.DeleteTask / Service.ArchiveTask, so they inherit nothing from the +// task service's authorize* helpers — without this they are the unguarded half +// of every archive and delete route. +func (s *HandoffService) SetTaskAccessChecker(check func(ctx context.Context, taskID string) error) { + s.taskAccessCheck = check +} + +// authorizeTask applies the configured per-user task check. No-op when unwired. +func (s *HandoffService) authorizeTask(ctx context.Context, taskID string) error { + if s.taskAccessCheck == nil || taskID == "" { + return nil + } + return s.taskAccessCheck(ctx, taskID) +} + // NewHandoffService creates a HandoffService. blockers and wsGroups may be // nil — when nil the corresponding methods return empty results / // no-ops, mirroring the optional wiring used by Service.SetBlockerRepository. diff --git a/apps/backend/internal/task/service/service_branch_update.go b/apps/backend/internal/task/service/service_branch_update.go index a652b21c3f..3344224338 100644 --- a/apps/backend/internal/task/service/service_branch_update.go +++ b/apps/backend/internal/task/service/service_branch_update.go @@ -50,6 +50,12 @@ func (s *Service) UpdateRepositoryBaseBranch(ctx context.Context, req UpdateRepo if err != nil { return nil, err } + // Ahead of every repository read. The WS action names task_id so the gateway + // backstop already covers that transport, but this method is also reachable + // over HTTP and MCP, and a backstop is not a substitute for a service guard. + if err := s.authorizeTaskID(ctx, req.TaskID); err != nil { + return nil, err + } taskRepo, err := s.loadTaskRepositoryForUpdate(ctx, req.TaskID, req.TaskRepositoryID) if err != nil { return nil, err diff --git a/apps/backend/internal/task/service/service_workflow.go b/apps/backend/internal/task/service/service_workflow.go index 3bfb5839c7..8d78bd9ed6 100644 --- a/apps/backend/internal/task/service/service_workflow.go +++ b/apps/backend/internal/task/service/service_workflow.go @@ -161,6 +161,9 @@ func (s *Service) resolveApprovalNextStep(ctx context.Context, step *wfmodels.Wo // UpdateTaskState updates the state of a task, moves it to the matching column, // and publishes a task.state_changed event func (s *Service) UpdateTaskState(ctx context.Context, id string, state v1.TaskState) (*models.Task, error) { + if err := s.authorizeTaskID(ctx, id); err != nil { + return nil, err + } task, err := s.tasks.GetTask(ctx, id) if err != nil { return nil, err @@ -430,6 +433,9 @@ func (s *Service) MoveTaskWithOptions( position int, opts MoveTaskOptions, ) (*MoveTaskResult, error) { + if err := s.authorizeTaskID(ctx, id); err != nil { + return nil, err + } task, err := s.tasks.GetTask(ctx, id) if err != nil { return nil, err @@ -684,6 +690,19 @@ func (s *Service) promoteFeederQueuedTask(ctx context.Context, candidate *models s.publishTaskMovedEvent(ctx, candidate, oldWorkflowID, fromStepID, targetStep.ID, "") return true } + // ctx here still carries the identity of whoever triggered the move that + // freed the slot, so MoveTaskWithOptions' authorizeTaskID applies to the + // promoted candidate too. That is safe by construction: a step's + // PullFromStepID resolves within the same workflow, a workflow belongs to + // one workspace, and a workspace has one owner — so the candidate always + // belongs to the caller who just passed the same check. + // + // If a future configuration ever allowed cross-workspace feeder pulls, this + // would refuse and log below rather than promote. Leave it that way: do not + // strip the identity to "fix" it. Promoting another user's task into a step + // they cannot see is the worse outcome, and the atomic promoter above (which + // the SQLite repository implements, so it is the only path production takes) + // does not go through a guarded method at all. if _, err := s.MoveTaskWithOptions(ctx, candidate.ID, targetStep.WorkflowID, targetStep.ID, position, MoveTaskOptions{PreserveDeferredLaunch: true}); err != nil { skipped[candidate.ID] = struct{}{} s.logger.Warn("skipping queued task that could not be promoted", zap.String("task_id", candidate.ID), zap.String("to_step_id", targetStep.ID), zap.Error(err)) diff --git a/apps/backend/internal/task/service/service_workflow_authz_test.go b/apps/backend/internal/task/service/service_workflow_authz_test.go new file mode 100644 index 0000000000..fd9c186913 --- /dev/null +++ b/apps/backend/internal/task/service/service_workflow_authz_test.go @@ -0,0 +1,247 @@ +package service + +import ( + "context" + "errors" + "testing" + + sqliterepo "github.com/kandev/kandev/internal/task/repository/sqlite" + + "github.com/kandev/kandev/internal/task/models" + "github.com/kandev/kandev/internal/task/repository/repoerrors" + wfmodels "github.com/kandev/kandev/internal/workflow/models" + v1 "github.com/kandev/kandev/pkg/api/v1" +) + +// Per-user scoping for the task lifecycle mutations that used to start at +// s.tasks.GetTask with no authorization step: state changes, workflow moves and +// base-branch edits. Their WS actions name the task `id` rather than `task_id`, +// so the gateway backstop parsed no refs and let them through — these guards are +// what actually refuses the call. +// +// Every case runs the identical request as the OWNER afterwards and asserts it +// succeeds. Without that witness a denial test still passes when the fixture is +// broken (wrong ID, unseeded row) rather than when the guard works. + +// seedAuthzWorkflowFixture builds one workspace owned by user-b holding a +// workflow with two steps and a task parked on the first, plus a repository the +// task checks out. Returns the task-repository row ID for the base-branch case. +func seedAuthzWorkflowFixture(t *testing.T, svc *Service, repo *sqliterepo.Repository) string { + t.Helper() + ctx := context.Background() + svc.SetWorkflowStepGetter(&fakeWorkflowStepGetter{steps: map[string]*wfmodels.WorkflowStep{ + "step-b-1": {ID: "step-b-1", WorkflowID: "wf-b", Name: "First", Position: 0}, + "step-b-2": {ID: "step-b-2", WorkflowID: "wf-b", Name: "Second", Position: 1}, + }}) + must(t, repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws-b", Name: "B's", OwnerID: "user-b"})) + must(t, repo.CreateWorkflow(ctx, &models.Workflow{ID: "wf-b", WorkspaceID: "ws-b", Name: "B flow"})) + must(t, repo.CreateRepository(ctx, &models.Repository{ + ID: "repo-b", WorkspaceID: "ws-b", Name: "backend", DefaultBranch: "main", + })) + must(t, repo.CreateTask(ctx, &models.Task{ + ID: "task-b", WorkspaceID: "ws-b", WorkflowID: "wf-b", WorkflowStepID: "step-b-1", + Title: "Victim", State: v1.TaskStateTODO, Priority: priorityMedium, + })) + must(t, repo.CreateTaskRepository(ctx, &models.TaskRepository{ + ID: "task-repo-b", TaskID: "task-b", RepositoryID: "repo-b", + BaseBranch: "main", CheckoutBranch: "feature/x", + })) + return "task-repo-b" +} + +func TestUpdateTaskStateDeniesForeignTask(t *testing.T) { + svc, _, repo := createTestService(t) + seedAuthzWorkflowFixture(t, svc, repo) + + task, err := svc.UpdateTaskState(ctxAs("user-a"), "task-b", v1.TaskStateInProgress) + if !errors.Is(err, repoerrors.ErrTaskNotFound) { + t.Fatalf("foreign state change: err = %v, want ErrTaskNotFound", err) + } + if task != nil { + t.Fatalf("denial leaked the task: %+v", task) + } + + stored, err := repo.GetTask(context.Background(), "task-b") + if err != nil { + t.Fatalf("GetTask: %v", err) + } + if stored.State != v1.TaskStateTODO { + t.Fatalf("a denied state change reached the repository: state = %s", stored.State) + } + + // The owner must still get through, otherwise the denial above proves nothing. + owned, err := svc.UpdateTaskState(ctxAs("user-b"), "task-b", v1.TaskStateInProgress) + if err != nil { + t.Fatalf("owner state change: %v", err) + } + if owned.State != v1.TaskStateInProgress { + t.Fatalf("owner state change did not apply: state = %s", owned.State) + } +} + +func TestMoveTaskDeniesForeignTask(t *testing.T) { + svc, _, repo := createTestService(t) + seedAuthzWorkflowFixture(t, svc, repo) + + result, err := svc.MoveTask(ctxAs("user-a"), "task-b", "wf-b", "step-b-2", 3) + if !errors.Is(err, repoerrors.ErrTaskNotFound) { + t.Fatalf("foreign move: err = %v, want ErrTaskNotFound", err) + } + if result != nil { + t.Fatalf("denial leaked the move result: %+v", result) + } + + stored, err := repo.GetTask(context.Background(), "task-b") + if err != nil { + t.Fatalf("GetTask: %v", err) + } + if stored.WorkflowStepID != "step-b-1" || stored.Position != 0 { + t.Fatalf("a denied move reached the repository: step = %s position = %d", + stored.WorkflowStepID, stored.Position) + } + + owned, err := svc.MoveTask(ctxAs("user-b"), "task-b", "wf-b", "step-b-2", 3) + if err != nil { + t.Fatalf("owner move: %v", err) + } + if owned.Task.WorkflowStepID != "step-b-2" { + t.Fatalf("owner move did not apply: step = %s", owned.Task.WorkflowStepID) + } +} + +func TestUpdateRepositoryBaseBranchDeniesForeignTask(t *testing.T) { + svc, _, repo := createTestService(t) + taskRepoID := seedAuthzWorkflowFixture(t, svc, repo) + svc.SetAgentBaseBranchPusher(&fakeBaseBranchPusher{}) + + req := UpdateRepositoryBaseBranchRequest{ + TaskID: "task-b", TaskRepositoryID: taskRepoID, BaseBranch: "staging", + } + + updated, err := svc.UpdateRepositoryBaseBranch(ctxAs("user-a"), req) + if !errors.Is(err, repoerrors.ErrTaskNotFound) { + t.Fatalf("foreign base-branch update: err = %v, want ErrTaskNotFound", err) + } + if updated != nil { + t.Fatalf("denial leaked the task repository: %+v", updated) + } + + rows, err := repo.ListTaskRepositories(context.Background(), "task-b") + if err != nil { + t.Fatalf("ListTaskRepositories: %v", err) + } + if rows[0].BaseBranch != "main" { + t.Fatalf("a denied base-branch update reached the repository: %s", rows[0].BaseBranch) + } + + owned, err := svc.UpdateRepositoryBaseBranch(ctxAs("user-b"), req) + if err != nil { + t.Fatalf("owner base-branch update: %v", err) + } + if owned.BaseBranch != "staging" { + t.Fatalf("owner base-branch update did not apply: %s", owned.BaseBranch) + } +} + +// TestScopedMoveStillPromotesFeederQueuedTask covers the follow-on work flagged +// in review: the move guards run on a ctx that internal continuations inherit, +// and feeder queue promotion is the one that can reach a guarded method. Freeing +// a slot in a WIP-limited step as the owner must still pull the task queued in +// the feeder step behind it. +// +// What this pins, precisely: promotion still happens end to end under a real +// identity. It does NOT exercise the guard, because the SQLite repository +// implements workflowQueuedTaskPromoter, so promotion takes the atomic branch and +// never calls MoveTaskWithOptions at all — the guarded fallback in +// promoteFeederQueuedTask is unreachable in production and only runs for repos +// without that method. Verified by mutation: breaking the feeder pull fails this +// test, while forcing a denying identity into the promotion path does not. +// +// The invariant that keeps the fallback safe is documented at that call site: a +// step's PullFromStepID resolves within the same workflow, a workflow belongs to +// one workspace, and a workspace has one owner. +func TestScopedMoveStillPromotesFeederQueuedTask(t *testing.T) { + svc, _, repo := createTestService(t) + ctx := context.Background() + svc.SetWorkflowStepGetter(&fakeWorkflowStepGetter{steps: map[string]*wfmodels.WorkflowStep{ + "step-b-feed": {ID: "step-b-feed", WorkflowID: "wf-b", Name: "Backlog", Position: 0}, + "step-b-wip": { + ID: "step-b-wip", WorkflowID: "wf-b", Name: "Doing", Position: 1, + WIPLimit: 1, PullFromStepID: "step-b-feed", + }, + "step-b-done": {ID: "step-b-done", WorkflowID: "wf-b", Name: "Done", Position: 2}, + }}) + must(t, repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws-b", Name: "B's", OwnerID: "user-b"})) + must(t, repo.CreateWorkflow(ctx, &models.Workflow{ID: "wf-b", WorkspaceID: "ws-b", Name: "B flow"})) + + // occupant fills the WIP-limited step; candidate waits in the feeder step. + must(t, repo.CreateTask(ctx, &models.Task{ + ID: "task-occupant", WorkspaceID: "ws-b", WorkflowID: "wf-b", WorkflowStepID: "step-b-wip", + Title: "Occupant", State: v1.TaskStateTODO, Priority: priorityMedium, WIPAdmitted: true, + })) + must(t, repo.CreateTask(ctx, &models.Task{ + ID: "task-queued", WorkspaceID: "ws-b", WorkflowID: "wf-b", WorkflowStepID: "step-b-feed", + Title: "Queued", State: v1.TaskStateTODO, Priority: priorityMedium, + QueuedForStepID: "step-b-wip", + })) + + // The owner moves the occupant out, freeing the slot — under their identity, + // so every guard on the promotion path is live. + if _, err := svc.MoveTask(ctxAs("user-b"), "task-occupant", "wf-b", "step-b-done", 0); err != nil { + t.Fatalf("owner move out of the WIP step: %v", err) + } + + promoted, err := repo.GetTask(ctx, "task-queued") + if err != nil { + t.Fatalf("GetTask: %v", err) + } + if promoted.WorkflowStepID != "step-b-wip" || !promoted.WIPAdmitted { + t.Fatalf("a scoped move did not pull the feeder-queued task: step=%s admitted=%v", + promoted.WorkflowStepID, promoted.WIPAdmitted) + } +} + +// TestTaskWorkflowGuardsPrecedeRepositoryUse pins guard placement. The service +// carries only the two repositories the guard itself reads; a denial that ran +// after the first unrelated repo call would nil-panic instead of returning. +func TestTaskWorkflowGuardsPrecedeRepositoryUse(t *testing.T) { + svc, _, repo := createTestService(t) + seedAuthzWorkflowFixture(t, svc, repo) + + guarded := &Service{ + tasks: svc.tasks, + workspaces: svc.workspaces, + logger: svc.logger, + } + + cases := map[string]func() error{ + "UpdateTaskState": func() error { + _, err := guarded.UpdateTaskState(ctxAs("user-a"), "task-b", v1.TaskStateInProgress) + return err + }, + "MoveTaskWithOptions": func() error { + _, err := guarded.MoveTaskWithOptions( + ctxAs("user-a"), "task-b", "wf-b", "step-b-2", 0, MoveTaskOptions{}) + return err + }, + "UpdateRepositoryBaseBranch": func() error { + _, err := guarded.UpdateRepositoryBaseBranch(ctxAs("user-a"), UpdateRepositoryBaseBranchRequest{ + TaskID: "task-b", TaskRepositoryID: "task-repo-b", BaseBranch: "staging", + }) + return err + }, + } + + for name, invoke := range cases { + t.Run(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) + } + }() + if err := invoke(); !errors.Is(err, repoerrors.ErrTaskNotFound) { + t.Fatalf("err = %v, want ErrTaskNotFound", err) + } + }) + } +} diff --git a/docs/plans/task-lifecycle-authz-gaps/plan.md b/docs/plans/task-lifecycle-authz-gaps/plan.md new file mode 100644 index 0000000000..6dc445de17 --- /dev/null +++ b/docs/plans/task-lifecycle-authz-gaps/plan.md @@ -0,0 +1,145 @@ +--- +spec: docs/specs/auth/spec.md +created: 2026-08-10 +status: building +--- + +# Implementation Plan: Task Lifecycle Authorization Gaps + +## Overview + +Four per-user authorization holes let one authenticated user mutate another user's +task by naming its ID. All four share one root cause: the entry point never asks +`authorizeTaskID`, and the structural backstops that would have caught it do not +see the request. + +- **Gaps 1 & 2** — `Service.UpdateTaskState` and `Service.MoveTaskWithOptions` start + at `s.tasks.GetTask` with no authorize call. Their WS actions (`task.state`, + `task.move`) name the task `id`, and the gateway backstop only parses `task_id`, + `session_id` and `task_environment_id`, so it finds no refs and allows the action. +- **Gap 3** — `TaskHandlers.httpDeleteTask` builds its context from + `context.Background()`. The identity the auth middleware put on the request is + dropped, so `authorizeTaskID` sees an identity-less internal caller and permits + everything. +- **Gap 4** (found while verifying gap 3's fix) — the production delete/archive path + does not reach `Service.DeleteTask` / `Service.ArchiveTask` at all. When a + `HandoffService` is wired — which `backendapp` always does — the handlers call + `HandoffService.DeleteTaskTree` / `ArchiveTaskTree`, and `HandoffService` has no + identity awareness whatsoever. Fixing gap 3's context alone would leave the + shipped delete route wide open; the fixture in which gap 3 was reproduced has + `handoffSvc == nil`, which is why the sibling archive route looked correct. + +Layering matters here. The service guards are the fix; the gateway backstop is a +structural net that makes the *next* `task.*` action safe by default rather than +only when its author remembers. Per `apps/backend/CLAUDE.md` neither substitutes for +the other, so both land. + +Four tasks. 01 and 04 are independent; 02 is meaningless without 03 (the production +route bypasses the guarded service), so they land together in that order. + +--- + +## Backend + +### Service layer (`internal/task/service/service_workflow.go`) + +`authorizeTaskID` first, before any repo, executor or event-bus use — matching +`UpdateTaskMetadata` (`service_tasks.go:1333`) and `deleteTaskWithReason` +(`service_tasks.go:1911`): + +- `UpdateTaskState` — `if err := s.authorizeTaskID(ctx, id); err != nil { return nil, err }` +- `MoveTaskWithOptions` — same, ahead of `s.tasks.GetTask`. The destination + workflow needs no separate check: `validateTaskMove` already refuses a move + whose target workflow belongs to a different workspace. +- `UpdateRepositoryBaseBranch` — same, on `req.TaskID`. Its WS payload uses + `task_id`, so the gateway backstop does cover it today; the check lands anyway + because a backstop is not a substitute for a service-level guard and the method + is reachable from HTTP and MCP too. + +Denials return the existing `repoerrors.ErrTaskNotFound` sentinel. No handler +change: `wsUpdateTaskState` and `wsMoveTask` already collapse every service error +to a generic `ErrorCodeInternalError` message, which is exactly what a nonexistent +task produces — the denial is indistinguishable from "no such task". + +### Cascade layer (`internal/task/service/handoff_service.go`, `handoff_cascade.go`) + +Mirror the orchestrator's checker pattern (`internal/orchestrator/service.go:982-1021`) +rather than inventing a second one: + +```go +func (s *HandoffService) SetTaskAccessChecker(check func(ctx context.Context, taskID string) error) +func (s *HandoffService) authorizeTask(ctx context.Context, taskID string) error // no-op when unwired +``` + +Called first in `ArchiveTaskTree`, `DeleteTaskTree` and `UnarchiveTaskTree` — before +the `rootID == ""` and `s.tasks == nil` guards do their repo work. Installed by +`TaskHandlers.SetHandoffService`, which is the call that makes those routes prefer +the cascade over the guarded `Service` methods, so the substitution cannot happen +without the guard. + +Unwired stays unscoped, so every existing fixture that builds a bare +`NewHandoffService(repo, nil, nil, nil, nil, nil)` is unaffected. + +### HTTP layer (`internal/task/handlers/task_http_handlers.go`) + +`httpDeleteTask` becomes: + +```go +deleteCtx, cancel := context.WithTimeout( + context.WithoutCancel(c.Request.Context()), constants.TaskDeleteTimeout) +``` + +`WithoutCancel` keeps the request's values — the identity among them — while +detaching from the request's cancellation, so a client that disconnects mid-delete +cannot abort a half-finished subtree teardown. Swapping in `c.Request.Context()` +directly would fix the identity and reintroduce that abort. + +### WS gateway backstop (`internal/gateway/websocket/dispatch_scope.go`) + +`authorizeAction` gains the action name and one rule: for an action of the form +`task.` — exactly two dot-separated segments — a payload `id` is a task ID. +That covers `task.get/update/delete/move/state/archive` and any future sibling, +without a hand-maintained list to fall out of date. + +Deeper namespaces are deliberately excluded: `task.plan.revision.get` and +`task.review.finding.update` also carry `id`, but it names a revision or a finding, +and treating it as a task ID would deny legitimate reads. `task.create` and +`task.list` carry no `id` and are unaffected. + +--- + +## Docs + +- `docs/specs/auth/spec.md` — the behavioral guarantee (lifecycle mutations are + ownership-checked below the transport; the backstop's `id` rule) plus two + scenarios. +- `apps/backend/CLAUDE.md` — the scoping-model section gains the `id`-vs-`task_id` + rule so the next author knows which payload names are covered. + +--- + +## Testing + +TDD throughout: denial test first, watched fail, then the guard. + +Every denial test also issues the identical request **as the owner** and asserts it +succeeds. Without that witness a denial test passes for the wrong reason — a broken +fixture, a typo'd ID, a missing seed row — and several tests in this area were +vacuous for exactly that reason. Every denied mutation additionally asserts the +repository recorded no write; a guard that refuses the response after committing +the write is still data loss. + +New symbols in `internal/task/handlers` are `authz`-prefixed so they cannot collide +with the fixtures PR #2500 adds to the same package while it is unmerged. + +Mutation check: each guard is reverted one at a time and the new test must fail and +name itself. + +## Tasks + +| # | Task | Depends on | +|---|------|-----------| +| 01 | [Service-layer guards for state, move and base-branch](task-01-service-guards.md) | — | +| 02 | [HandoffService cascade authorization](task-02-handoff-cascade-authz.md) | — | +| 03 | [HTTP delete keeps the caller identity](task-03-http-delete-identity.md) | 02 | +| 04 | [WS backstop reads `id` on top-level `task.*` actions](task-04-ws-backstop-id.md) | — | diff --git a/docs/plans/task-lifecycle-authz-gaps/task-01-service-guards.md b/docs/plans/task-lifecycle-authz-gaps/task-01-service-guards.md new file mode 100644 index 0000000000..697767163b --- /dev/null +++ b/docs/plans/task-lifecycle-authz-gaps/task-01-service-guards.md @@ -0,0 +1,54 @@ +# Task 01 — Service-layer guards for state, move and base-branch + +**Status:** done +**Depends on:** — + +## Goal + +`Service.UpdateTaskState`, `Service.MoveTaskWithOptions` and +`Service.UpdateRepositoryBaseBranch` refuse a task the caller cannot see, before +they read or write anything. + +## Changes + +`internal/task/service/service_workflow.go` + +- `UpdateTaskState` — `authorizeTaskID(ctx, id)` as the first statement, ahead of + `s.tasks.GetTask`. +- `MoveTaskWithOptions` — same. `MoveTask` delegates here, so both entry points are + covered by one guard. +- `UpdateRepositoryBaseBranch` — `authorizeTaskID(ctx, req.TaskID)` first. Defense in + depth: the WS action names `task_id` so the gateway backstop already covers that + transport, but the method is also reachable from HTTP and MCP, and + `apps/backend/CLAUDE.md` requires the service-level check regardless. + +No handler changes. `wsUpdateTaskState` and `wsMoveTask` map every service error to +a generic `ErrorCodeInternalError` message, which is byte-identical to what a +nonexistent task already produces — so the denial leaks no existence. + +## Tests + +`internal/task/service/service_workflow_authz_test.go` (new), using the +`ctxAs` / `seedScopedWorkspaces` / `createTestService` fixtures already in +`service_access_test.go`: + +- `TestUpdateTaskStateDeniesForeignTask` — user-a is refused with + `repoerrors.ErrTaskNotFound`, the returned task is nil, and the repository state + is unchanged; **then user-b runs the identical call and it succeeds** and the + state actually changes. +- `TestMoveTaskDeniesForeignTask` — same shape; asserts the task's + workflow/step/position are untouched after the denial, and that the owner's move + lands. +- `TestUpdateRepositoryBaseBranchDeniesForeignTask` — same shape. +- `TestTaskWorkflowGuardsPrecedeRepositoryUse` — table over the three entry points + with a `Service` whose repos are nil except the two the guard itself needs; a + panic means the guard is placed after the first repo use. + +## Verification + +``` +cd apps/backend && CGO_ENABLED=1 go test -tags fts5 -race -count=1 ./internal/task/service/... +``` + +Mutation: remove each `authorizeTaskID` line in turn; the matching test must fail +and name itself. diff --git a/docs/plans/task-lifecycle-authz-gaps/task-02-handoff-cascade-authz.md b/docs/plans/task-lifecycle-authz-gaps/task-02-handoff-cascade-authz.md new file mode 100644 index 0000000000..4ff2a05a92 --- /dev/null +++ b/docs/plans/task-lifecycle-authz-gaps/task-02-handoff-cascade-authz.md @@ -0,0 +1,67 @@ +# Task 02 — HandoffService cascade authorization + +**Status:** done +**Depends on:** — + +## Why + +`backendapp` always wires a `HandoffService` into `TaskHandlers`, so in a shipped +instance `DELETE /tasks/:id`, `POST /tasks/:id/archive` and the `task.archive` WS +action never reach the guarded `Service.DeleteTask` / `Service.ArchiveTask` — they +go to `HandoffService.DeleteTaskTree` / `ArchiveTaskTree`, which have no identity +awareness at all. Task 03's identity fix is inert without this. + +This is why the sibling archive route looked correctly scoped: it was reproduced +against a fixture with `handoffSvc == nil`. + +## Changes + +`internal/task/service/handoff_service.go` + +- `taskAccessCheck func(ctx context.Context, taskID string) error` field. +- `SetTaskAccessChecker(...)` — same contract as + `orchestrator.Service.SetTaskAccessChecker`: the checker returns nil for + identity-less contexts, and leaving it unwired keeps the pre-auth behavior. +- `authorizeTask(ctx, taskID)` — no-op when unwired or when taskID is empty. + +`internal/task/service/handoff_cascade.go` + +- `authorizeTask` as the first statement of `ArchiveTaskTree`, `DeleteTaskTree` and + `UnarchiveTaskTree`. + +`internal/task/handlers/task_handlers.go` + +- `TaskHandlers.SetHandoffService` installs the checker + (`svc.SetTaskAccessChecker(h.service.AuthorizeTaskAccess)`). + +The wiring lives there rather than in `backendapp` on purpose: `SetHandoffService` +is the call that makes archive/delete/unarchive use the cascade *instead of* the +guarded `Service` methods, and every user-facing cascade call site is in this +package. Installing the guard as part of that substitution means it cannot be +wired without it, and it is reachable from a package test — a separate +`backendapp` line would have had no pin short of booting the whole route table. +The integration watch-reset callers share the same instance and are identity-free, +so the checker no-ops for them. + +## Tests + +`internal/task/service/handoff_cascade_authz_test.go` (new): + +- `TestCascadeEntryPointsDenyForeignTask` — table over the three entry points with a + checker that admits `task-mine` and refuses `task-b`. Each asserts the denial, that + the outcome is nil, and that the recording repo saw **no** archive/delete/unarchive + call; **then re-runs the same entry point on the owned task** and asserts it + proceeds past the guard. +- `TestCascadeEntryPointsGuardBeforeDependencies` — the same table with `s.tasks` + nil; a panic means the guard runs too late. +- `TestCascadeEntryPointsUnscopedWhenUnwired` — no checker installed, the denial + sentinel must not appear, so every existing fixture that builds a bare + `NewHandoffService(...)` keeps working. + +## Verification + +``` +cd apps/backend && CGO_ENABLED=1 go test -tags fts5 -race -count=1 ./internal/task/... ./internal/backendapp/... +``` + +Mutation: remove each `authorizeTask` call in turn. diff --git a/docs/plans/task-lifecycle-authz-gaps/task-03-http-delete-identity.md b/docs/plans/task-lifecycle-authz-gaps/task-03-http-delete-identity.md new file mode 100644 index 0000000000..aa28dc37ed --- /dev/null +++ b/docs/plans/task-lifecycle-authz-gaps/task-03-http-delete-identity.md @@ -0,0 +1,52 @@ +# Task 03 — HTTP delete keeps the caller identity + +**Status:** done +**Depends on:** 02 + +## Goal + +`DELETE /api/v1/tasks/:id` authorizes the caller, without reintroducing the +client-disconnect abort that `context.Background()` was there to prevent. + +## Changes + +`internal/task/handlers/task_http_handlers.go` — `httpDeleteTask`: + +```go +deleteCtx, cancel := context.WithTimeout( + context.WithoutCancel(c.Request.Context()), constants.TaskDeleteTimeout) +``` + +`context.WithoutCancel` keeps the request context's *values* — the identity the auth +middleware attached is one of them — while dropping its cancellation and deadline. +A browser that navigates away mid-delete no longer aborts a half-finished subtree +teardown, which is what plain `c.Request.Context()` would have reintroduced. + +## Tests + +`internal/task/handlers/task_delete_authz_test.go` (new). Symbols are `authz`-prefixed +so they cannot collide with the fixtures PR #2500 adds to this package while it is +unmerged. + +- `TestHTTPDeleteTaskDeniesForeignTask` — user-a gets 404 with the sanitized body, + the recording repo saw no delete; **then user-b issues the identical request and + gets 200**, with the delete recorded. +- `TestHTTPDeleteTaskDeniesForeignTaskThroughCascade` — the same pair with a + `HandoffService` wired (the production shape), proving task 02's guard is what + carries this route. +- `TestWSArchiveTaskDeniesForeignTaskThroughCascade` — the third route the cascade + substitution unscoped. +- `TestHTTPDeleteTaskSurvivesClientDisconnect` — the request context is cancelled + before the handler runs; the delete must still reach the repository **on a live + context**. Asserting the status code alone was vacuous (the fake repository + ignores its context, so a cancelled one still "succeeds") — the mutation run + caught that, and the test now records `ctx.Err()` at the repository boundary. + +## Verification + +``` +cd apps/backend && CGO_ENABLED=1 go test -tags fts5 -race -count=1 ./internal/task/handlers/... +``` + +Mutation: restore `context.Background()`; the denial tests must fail. Replace with a +bare `c.Request.Context()`; the disconnect test must fail. diff --git a/docs/plans/task-lifecycle-authz-gaps/task-04-ws-backstop-id.md b/docs/plans/task-lifecycle-authz-gaps/task-04-ws-backstop-id.md new file mode 100644 index 0000000000..70a99a807e --- /dev/null +++ b/docs/plans/task-lifecycle-authz-gaps/task-04-ws-backstop-id.md @@ -0,0 +1,58 @@ +# Task 04 — WS backstop reads `id` on top-level `task.*` actions + +**Status:** done +**Depends on:** — + +## Why + +`dispatch_scope.go` exists so a *newly added* WS action is scoped by default rather +than only when its author remembers. It reads `task_id`, `session_id` and +`task_environment_id`. `task.state` and `task.move` name their task `id`, so the +backstop parsed no refs and allowed them — the file's own comment predicted this +("an action that invents a fourth name … silently opts out"). + +The service guards in task 01 are the fix. This closes the class. + +## Changes + +`internal/gateway/websocket/dispatch_scope.go` + +- `authorizeAction(ctx, action, payload)` — takes the action name. +- `scopedActionRefs` gains `ID string \`json:"id"\``, used **only** when + `isTopLevelTaskAction(action)`: the action is `task.` + exactly one more segment. +- Deeper namespaces stay excluded on purpose. `task.plan.revision.get` and + `task.review.finding.update` also carry `id`, but it names a revision or a finding; + treating it as a task ID would deny legitimate reads. Rather than a hand-maintained + action list — which has the same "someone forgot" failure mode the backstop exists + to remove — the rule keys off the namespace depth, so a future `task.` is + covered without an edit. +- `parseScopedActionRefs` returns ok when any of the four names is present. + +`internal/gateway/websocket/client.go` — pass `msg.Action` through. + +## Tests + +`internal/gateway/websocket/dispatch_scope_test.go`: + +- `task.state` / `task.move` / `task.archive` with `{"id": "task-b"}` are refused, + and the task checker is called with `task-b`. +- The owner's identical action is allowed (the checker admits it), so the rule is not + a blanket deny. +- `task.plan.revision.get` and `task.review.finding.update` with `{"id": …}` are + **allowed** — the regression this rule could plausibly cause. +- `task.create` / `task.list` (no `id`) are unaffected. +- A synthetic or identity-less client is unaffected. + +## Docs + +`apps/backend/CLAUDE.md` — the scoping-model section records which payload names the +backstop reads, including the `task.` + `id` rule, so the next author knows +what is and is not covered. + +## Verification + +``` +cd apps/backend && CGO_ENABLED=1 go test -tags fts5 -race -count=1 ./internal/gateway/... +``` + +Mutation: drop the `isTopLevelTaskAction` branch; the `task.state` case must fail. diff --git a/docs/specs/auth/spec.md b/docs/specs/auth/spec.md index bb82fd52a3..73841ce497 100644 --- a/docs/specs/auth/spec.md +++ b/docs/specs/auth/spec.md @@ -51,6 +51,19 @@ the local single-user install with a login screen it never asked for. from another user's workspace does not grant access. Admins do NOT see other users' workspaces (hard privacy; admin is a management role, not a visibility role). Pre-auth data is claimed by the admin at setup. +- **Task lifecycle mutations are ownership-checked at the service layer.** + Changing a task's state, moving it between workflows/steps, updating a task + repository's base branch, and archiving or deleting a task (including the + cascade that walks its subtree) all refuse a foreign task before any read or + write reaches the repository. This holds on every transport — HTTP, WebSocket + and MCP — because the check lives below them, and it holds for long-running + deletes, which detach from request cancellation but keep the caller identity. +- **The WebSocket backstop reads `id` on top-level `task.*` actions.** In + addition to `task_id` / `session_id` / `task_environment_id`, an action named + `task.` (exactly two segments — `task.state`, `task.move`, `task.get`, …) + has its payload `id` treated as a task ID. Deeper namespaces (`task.plan.*`, + `task.review.*`) are untouched: their `id` names a plan, revision or finding, + not a task. - **Shared surfaces (instance-global by design).** Executors, executor profiles, environment definitions, agent profiles/agents, editors, prompts, sprites, voice, notification providers, and system pages remain shared @@ -99,6 +112,15 @@ the local single-user install with a login screen it never asked for. ## Scenarios +- **GIVEN** two users A and B and a task owned by B, **WHEN** A sends + `task.state` or `task.move` over the WebSocket naming that task by `id`, + **THEN** the call is refused, B's task DTO is not serialized into the + response, and no state, position or workflow write reaches the repository. +- **GIVEN** the same pair, **WHEN** A sends `DELETE /api/v1/tasks/{B's task}` + or `POST /api/v1/tasks/{B's task}/archive`, **THEN** the response is 404 and + no task row, subtree row, or workspace-group membership is touched — while + the owner's own delete still runs to completion if the owner's browser + disconnects mid-request. - **GIVEN** an authenticated Kandev browser session, **WHEN** a protected API request receives a Kandev session challenge, **THEN** the browser clears the stale Kandev identity and navigates to `/login`.