fix(backend): scope task state, move and delete to the calling user (#2503)
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
@@ -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.<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.
|
||||
- **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.
|
||||
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
|
||||
@@ -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),
|
||||
|
||||
@@ -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.<verb>
|
||||
// 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
|
||||
|
||||
@@ -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.<verb> 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.<verb> 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)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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")
|
||||
}
|
||||
}
|
||||
@@ -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) {
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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")
|
||||
}
|
||||
}
|
||||
@@ -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")
|
||||
}
|
||||
|
||||
@@ -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")
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
@@ -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.
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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))
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
@@ -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.<verb>` — 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) | — |
|
||||
@@ -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.
|
||||
@@ -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.
|
||||
@@ -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.
|
||||
@@ -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.<verb>` 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.<verb>` + `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.
|
||||
@@ -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.<verb>` (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`.
|
||||
|
||||
Reference in New Issue
Block a user