diff --git a/apps/backend/AGENTS.md b/apps/backend/AGENTS.md index 74c0be533f..bfefa7015f 100644 --- a/apps/backend/AGENTS.md +++ b/apps/backend/AGENTS.md @@ -176,6 +176,7 @@ Standalone agentctl is launched in its own process group so terminal Ctrl+C is h - **Workspaces are per-user** (`workspaces.owner_id`); the task service filters/denies at the service layer with `*NotFound` sentinels (no existence leak). Unowned rows (`owner_id=''`) stay visible until the setup wizard claims them for the admin. - **New user-facing service entry points must apply scoping** — call the `authorize*` helpers in `task/service/service_access.go` (or the same pattern) when adding routes that read or mutate workspace-scoped data. This includes **session-keyed** entry points: any service method taking a caller-supplied `sessionID` (or a `taskID` used to reach sessions) authorizes via `authorizeTaskID(session.TaskID)`. Authorize off the row you already read rather than calling `AuthorizeSessionAccess`, which re-reads the session. Batch helpers that take ID *lists* (`BatchGetSessionsForTasks`, `GetPrimarySessionInfoForTasks`, …) are exempt by convention: their callers derive the IDs from an already-authorized task list, so per-ID checks would add N queries to list views for no gain — keep it that way, and don't hand them caller-supplied IDs. - **Two session paths bypass the task service and must guard themselves.** (1) Handlers that resolve an execution by a bare in-memory lookup (`GetExecutionBySessionID`, `*BySessionID`) skip the `GetOrEnsure*` chokepoint where the lifecycle check runs — call `service.AuthorizeSessionAccess` (see `ProcessHandlers.denySessionAccess`) or `lifecycleMgr.CheckSessionAccess` first. (2) The orchestrator reads sessions through its own repo handle, so it inherits nothing from the task service; session-keyed entry points there call `authorizeSession` / `authorizeTask`, wired by `SetSessionAccessChecker` / `SetTaskAccessChecker`. An entry point that accepts **both** a task and a session ID must use `authorizeTaskSessionPair`, never `authorizeSession` alone: the caller can satisfy a session-only check with one of their own sessions while pointing `taskID` at someone else's task, which the method then uses for its task-scoped work. Put the guard **first** in the method, before any repo/executor/agent-manager use — `TestSessionKeyedEntryPointsGuardBeforeDependencies` runs each entry point with nil dependencies and fails on a panic if a guard is placed too late. +- **The workflow-step surface scopes itself in `internal/workflow`.** Steps, workspace step lists, export and import all reach a workflow or workspace by caller-supplied ID, and none of those IDs is a name the WS dispatch backstop parses, so the guard has to be in this package. `internal/workflow/service/access.go` holds `AuthorizeWorkflow` / `AuthorizeWorkspace` / `AuthorizeStep`, backed by `SetWorkflowAccessChecker` / `SetWorkspaceAccessChecker` (wired to `taskSvc.AuthorizeWorkflowAccess` / `AuthorizeWorkspaceAccess` in `backendapp/services.go`, next to the older `SetSessionAccessChecker`). Step CRUD authorizes in the **controller**, because REST, WS and the MCP tools all share it; `ListStepsByWorkspaceID` and the export/import trio authorize in the **service**, because the boot-payload builder and the MCP config tools call those directly. A denial and a genuine miss both surface as `service.ErrNotVisible` → one 404, so a foreign step and a nonexistent one are indistinguishable. Templates are deliberately unscoped (install-global, ownerless). Note that an empty ID **fails closed** here rather than being passed to `AuthorizeWorkspaceAccess`, which reads `""` as "no scoping applies". A step also *contains* IDs — `move_to_step` names a step, `queue_run` names a task — and the engine dereferences them later on the event bus, with no identity to check against, so they are validated at write time: `models.CollectStepEventReferences` enumerates them (keep it in sync with `RemapStepEvents` when adding a trigger), a transition target must resolve to a step in the same workflow, and a task target is authorized like any other. A reorder's step IDs must belong to the workflow named in the URL, which is also what keeps a read-only workflow's steps from being reordered through a mutable workflow's route. - **Dispatched WS actions have a gateway backstop.** `Client.authorizeAction` (`internal/gateway/websocket/dispatch_scope.go`) checks any payload carrying `task_id` or `session_id` against the client's identity *before* dispatch, so a newly added action is scoped by default rather than only when its author remembers. Handler-level scoping is still required (defense in depth, and it covers non-WS callers) — do not delete a service-layer check because the backstop exists. The backstop reads `task_id`, `session_id` and `task_environment_id`, plus `id` on **top-level `task.` actions only** (`task.state`, `task.move`, `task.get`, … — `task.` and exactly one more segment). Keep using those names for task-scoped actions, because an action that invents a new name for the same kind of resource silently opts out. (`user_shell.stop` was exactly that: it keys off `task_environment_id` with `task_id` optional, so a `task_id`-only backstop missed it. `task.state` and `task.move` were the second instance: they name the task `id`, so the backstop parsed no refs and let one user mutate any other user's task — hence the `id` rule.) Deeper `task.*` namespaces are deliberately excluded: in `task.plan.revision.get` and `task.review.finding.update` the `id` names a revision or a finding, so those actions must carry `task_id` when they are task-scoped. Also avoid permissive types (`any`, `json.RawMessage`) for those fields: a payload that fails to unmarshal into the ref struct is treated as naming nothing. - **WS**: clients carry their identity; dispatched actions and subscriptions are scoped; workspace-carrying events route via `Hub.BroadcastToWorkspace`. A new `hub.Broadcast` (global) call site needs a `//ws:global` justification comment. Subscription actions (`*.subscribe`) `return` before reaching the dispatch backstop (`Client.authorizeAction`), so scoping a new subscribe topic means adding a hook to `SubscriptionAccessPolicy` (`internal/gateway/websocket/access.go`) *and* calling it explicitly inside that action's own handler — the backstop's `scopedActionRefs` cannot reach it. `run.subscribe`'s `Subscriptions.Run` hook (added for WO-02, resolving the run's workspace via `runs.GetRunWorkspaceID`) is the worked example. - **Task overview vs. session detail:** Keep cross-task state in the bounded, persisted `TaskStatusSummary` projection and `task.status_summary.updated`; keep unbounded transcript and execution data session-scoped. The [bounded task-status spec](../../docs/specs/platform/requirements/bounded-task-status-delivery.md) and [accepted ADR](../../docs/decisions/2026-08-01-separate-task-summary-session-stream-traffic.md) define the shared contract. diff --git a/apps/backend/internal/backendapp/services.go b/apps/backend/internal/backendapp/services.go index e996b5bcbd..bb11470ada 100644 --- a/apps/backend/internal/backendapp/services.go +++ b/apps/backend/internal/backendapp/services.go @@ -174,6 +174,14 @@ func provideServices(cfg *config.Config, log *logger.Logger, repos *Repositories // Session history is owned by workflow service, but access is owned by the // task service. Keep the authorization check at the service boundary. workflowSvc.SetSessionAccessChecker(taskSvc.AuthorizeSessionAccess) + // Same split for the rest of the workflow-step surface: the workflow + // package owns steps, templates and export/import, but a workflow's owner + // is its workspace's owner, which only the task service can resolve. + workflowSvc.SetWorkflowAccessChecker(taskSvc.AuthorizeWorkflowAccess) + workflowSvc.SetWorkspaceAccessChecker(taskSvc.AuthorizeWorkspaceAccess) + // A step's queue_run action names the task it starts work on, so the + // step-write API accepts task IDs as well. + workflowSvc.SetTaskAccessChecker(taskSvc.AuthorizeTaskAccess) // Wire the ADR 0015 audit-trail writer for manual step transitions. // workflowSvc.CreateStepTransition already matches diff --git a/apps/backend/internal/backendapp/workflow_access_wiring_test.go b/apps/backend/internal/backendapp/workflow_access_wiring_test.go new file mode 100644 index 0000000000..dbc406f1ce --- /dev/null +++ b/apps/backend/internal/backendapp/workflow_access_wiring_test.go @@ -0,0 +1,92 @@ +package backendapp + +import ( + "fmt" + "go/ast" + "go/parser" + "go/token" + "testing" +) + +// TestWorkflowAccessCheckersAreWired guards the three lines that make the +// workflow-step surface's per-user scoping real in the shipped binary. +// +// The workflow package cannot resolve a workflow's owner on its own, so its +// guards call out through checkers that default to open when nothing is wired +// (the same shape SetSessionAccessChecker has always had, so a Service built +// without the task domain still works). Every test in internal/workflow wires +// its own fake, so deleting one of these lines here leaves that whole suite +// green while the real backend hands one user another user's workflow steps +// again — the defect the guards exist to close, reintroduced with no failing +// test. A rebase resolving a conflict in this DI function by dropping a line +// is exactly how that would happen. +func TestWorkflowAccessCheckersAreWired(t *testing.T) { + want := map[string]string{ + "SetWorkflowAccessChecker": "AuthorizeWorkflowAccess", + "SetWorkspaceAccessChecker": "AuthorizeWorkspaceAccess", + "SetTaskAccessChecker": "AuthorizeTaskAccess", + "SetSessionAccessChecker": "AuthorizeSessionAccess", + } + + provideFn := findFuncDecl(t, "services.go", "provideServices") + got := map[string]string{} + ast.Inspect(provideFn, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok || len(call.Args) != 1 { + return true + } + if _, tracked := want[sel.Sel.Name]; !tracked { + return true + } + // Only the workflow service's wiring counts; the orchestrator's + // identically-named setters are guarded by their own test. + if receiver, ok := sel.X.(*ast.Ident); !ok || receiver.Name != "workflowSvc" { + return true + } + got[sel.Sel.Name] = argumentSelector(call.Args[0]) + return true + }) + + for setter, checker := range want { + switch arg, called := got[setter]; { + case !called: + t.Errorf("provideServices never calls workflowSvc.%s; without it the workflow-step "+ + "surface falls open to any authenticated user", setter) + case arg != "taskSvc."+checker: + t.Errorf("workflowSvc.%s(%s), want taskSvc.%s", setter, arg, checker) + } + } +} + +// findFuncDecl parses filename and returns the named top-level function. +func findFuncDecl(t *testing.T, filename, name string) *ast.FuncDecl { + t.Helper() + file, err := parser.ParseFile(token.NewFileSet(), filename, nil, 0) + if err != nil { + t.Fatalf("parse %s: %v", filename, err) + } + for _, decl := range file.Decls { + if fn, ok := decl.(*ast.FuncDecl); ok && fn.Name.Name == name { + return fn + } + } + t.Fatalf("%s not found in %s; re-point this guard at the DI function", name, filename) + return nil +} + +// argumentSelector renders a `receiver.Method` argument for comparison. +func argumentSelector(arg ast.Expr) string { + sel, ok := arg.(*ast.SelectorExpr) + if !ok { + return fmt.Sprintf("%T", arg) + } + ident, ok := sel.X.(*ast.Ident) + if !ok { + return fmt.Sprintf("%T.%s", sel.X, sel.Sel.Name) + } + return ident.Name + "." + sel.Sel.Name +} diff --git a/apps/backend/internal/mcp/handlers/config_workflow_handlers.go b/apps/backend/internal/mcp/handlers/config_workflow_handlers.go index afd9a45a31..3b5e8409b3 100644 --- a/apps/backend/internal/mcp/handlers/config_workflow_handlers.go +++ b/apps/backend/internal/mcp/handlers/config_workflow_handlers.go @@ -8,6 +8,7 @@ import ( "github.com/kandev/kandev/internal/task/service" workflowctrl "github.com/kandev/kandev/internal/workflow/controller" wfmodels "github.com/kandev/kandev/internal/workflow/models" + workflowsvc "github.com/kandev/kandev/internal/workflow/service" "github.com/kandev/kandev/internal/workflow/stepevents" ws "github.com/kandev/kandev/pkg/websocket" "go.uber.org/zap" @@ -94,7 +95,7 @@ func (h *Handlers) handleExportWorkflow(ctx context.Context, msg *ws.Message) (* export, err := h.workflowSvc.ExportWorkflow(ctx, workflowID) if err != nil { h.logger.Error("failed to export workflow", zap.Error(err)) - return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeInternalError, "Failed to export workflow", nil) + return h.workflowScopeError(msg, err, "Failed to export workflow") } return ws.NewResponse(msg.ID, msg.Action, export) } @@ -141,7 +142,7 @@ func (h *Handlers) handleImportWorkflow(ctx context.Context, msg *ws.Message) (* result, err := h.workflowSvc.ImportWorkflows(ctx, req.WorkspaceID, &export) if err != nil { h.logger.Error("failed to import workflows", zap.Error(err)) - return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeInternalError, "Failed to import workflows", nil) + return h.workflowScopeError(msg, err, "Failed to import workflows") } return ws.NewResponse(msg.ID, msg.Action, result) } @@ -193,7 +194,7 @@ func (h *Handlers) handleCreateWorkflowStep(ctx context.Context, msg *ws.Message resp, err := h.workflowCtrl.CreateStep(ctx, createReq) if err != nil { h.logger.Error("failed to create workflow step", zap.Error(err)) - return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeInternalError, "Failed to create workflow step", nil) + return h.workflowScopeError(msg, err, "Failed to create workflow step") } h.publishWorkflowStepEvents(ctx, events.WorkflowStepUpdated, resp.DemotedStartSteps) h.publishWorkflowStepEvent(ctx, events.WorkflowStepCreated, resp.Step) @@ -242,7 +243,7 @@ func (h *Handlers) handleUpdateWorkflowStep(ctx context.Context, msg *ws.Message resp, err := h.workflowCtrl.UpdateStep(ctx, updateReq) if err != nil { h.logger.Error("failed to update workflow step", zap.Error(err)) - return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeInternalError, "Failed to update workflow step", nil) + return h.workflowScopeError(msg, err, "Failed to update workflow step") } h.publishWorkflowStepEvents(ctx, events.WorkflowStepUpdated, resp.DemotedStartSteps) h.publishWorkflowStepEvent(ctx, events.WorkflowStepUpdated, resp.Step) @@ -263,7 +264,7 @@ func (h *Handlers) handleDeleteWorkflowStep(ctx context.Context, msg *ws.Message if err := h.workflowCtrl.DeleteStep(ctx, stepID); err != nil { h.logger.Error("failed to delete workflow step", zap.Error(err)) - return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeInternalError, "Failed to delete workflow step", nil) + return h.workflowScopeError(msg, err, "Failed to delete workflow step") } if stepResp != nil { h.publishWorkflowStepEvent(ctx, events.WorkflowStepDeleted, stepResp.Step) @@ -291,11 +292,23 @@ func (h *Handlers) handleReorderWorkflowSteps(ctx context.Context, msg *ws.Messa StepIDs: req.StepIDs, }); err != nil { h.logger.Error("failed to reorder workflow steps", zap.Error(err)) - return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeInternalError, "Failed to reorder workflow steps", nil) + return h.workflowScopeError(msg, err, "Failed to reorder workflow steps") } return ws.NewResponse(msg.ID, msg.Action, map[string]interface{}{"success": true}) } +// workflowScopeError answers a workflow-surface failure. A resource the +// caller may not see, and one that does not exist, share the single not-found +// reply the workflow package produces for both; anything else is a genuine +// server failure. Without this, an agent that named a workflow, step or task +// it cannot reach was told the backend broke. +func (h *Handlers) workflowScopeError(msg *ws.Message, err error, clientErrMsg string) (*ws.Message, error) { + if workflowsvc.IsNotFound(err) { + return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeNotFound, clientErrMsg, nil) + } + return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeInternalError, clientErrMsg, nil) +} + // workflowStepEventSource labels workflow-step events published by this surface. const workflowStepEventSource = "mcp-handlers" diff --git a/apps/backend/internal/mcp/handlers/workflow_scope_error_test.go b/apps/backend/internal/mcp/handlers/workflow_scope_error_test.go new file mode 100644 index 0000000000..35b487a3a8 --- /dev/null +++ b/apps/backend/internal/mcp/handlers/workflow_scope_error_test.go @@ -0,0 +1,93 @@ +package handlers + +import ( + "context" + "encoding/json" + "errors" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/kandev/kandev/internal/auth/authn" + "github.com/kandev/kandev/internal/task/repository/repoerrors" + workflowctrl "github.com/kandev/kandev/internal/workflow/controller" + wfmodels "github.com/kandev/kandev/internal/workflow/models" + ws "github.com/kandev/kandev/pkg/websocket" +) + +// The workflow-step guards deny by returning the one not-found error the +// workflow package uses for both "not yours" and "not there". These handlers +// turned every error into INTERNAL_ERROR, so an agent that named a workflow, +// step or task it cannot reach was told the backend had broken — while the +// REST and WS surfaces answered not-found for the same request. + +// scopedMCPCtx carries a real (non-synthetic) identity, the way +// internal/mcp/scope attaches the task owner's identity to an in-session call. +func scopedMCPCtx() context.Context { + return authn.WithIdentity(context.Background(), authn.Identity{UserID: "user-a", Role: authn.RoleMember}) +} + +// setupScopedWorkflowHandlers builds the MCP handlers over a real workflow +// service whose task-domain checker answers with the supplied error. +func setupScopedWorkflowHandlers(t *testing.T, checkerErr error) (*Handlers, string) { + t.Helper() + h, _, repo := setupImportHandlers(t) + h.workflowCtrl = workflowctrl.NewController(h.workflowSvc) + h.workflowSvc.SetWorkflowAccessChecker(func(context.Context, string) error { return checkerErr }) + h.workflowSvc.SetWorkspaceAccessChecker(func(context.Context, string) error { return checkerErr }) + + step := &wfmodels.WorkflowStep{WorkflowID: "wf-other", Name: "Backlog", Color: "#111111"} + require.NoError(t, h.workflowSvc.CreateStep(context.Background(), step)) + // Keep the repo handle used, so the fixture stays honest about writing a + // real row rather than a fake. + stored, err := repo.GetStep(context.Background(), step.ID) + require.NoError(t, err) + return h, stored.ID +} + +func requireErrorCode(t *testing.T, msg *ws.Message, want string) { + t.Helper() + require.Equal(t, ws.MessageTypeError, msg.Type, "payload: %s", msg.Payload) + var payload ws.ErrorPayload + require.NoError(t, json.Unmarshal(msg.Payload, &payload)) + require.Equal(t, want, payload.Code, "message: %s", payload.Message) +} + +func TestMCPWorkflowHandlersReportDenialsAsNotFound(t *testing.T) { + h, stepID := setupScopedWorkflowHandlers(t, repoerrors.ErrWorkspaceNotFound) + ctx := scopedMCPCtx() + + cases := []struct { + name string + handler func(context.Context, *ws.Message) (*ws.Message, error) + payload map[string]any + }{ + {"delete step", h.handleDeleteWorkflowStep, map[string]any{"step_id": stepID}}, + {"update step", h.handleUpdateWorkflowStep, map[string]any{"step_id": stepID, "name": "Renamed"}}, + {"create step", h.handleCreateWorkflowStep, map[string]any{"workflow_id": "wf-other", "name": "New"}}, + {"reorder steps", h.handleReorderWorkflowSteps, + map[string]any{"workflow_id": "wf-other", "step_ids": []string{stepID}}}, + {"export workflow", h.handleExportWorkflow, map[string]any{"workflow_id": "wf-other"}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + raw, err := json.Marshal(tc.payload) + require.NoError(t, err) + resp, err := tc.handler(ctx, &ws.Message{ID: "req-1", Action: "test", Payload: raw}) + require.NoError(t, err) + requireErrorCode(t, resp, ws.ErrorCodeNotFound) + }) + } +} + +// A failure is still a failure: masking a database error as not-found would +// tell an agent its workflow is gone and invite it to recreate one. +func TestMCPWorkflowHandlersKeepRealFailuresInternal(t *testing.T) { + h, _ := setupScopedWorkflowHandlers(t, errors.New("database is locked")) + + raw, err := json.Marshal(map[string]any{"workflow_id": "wf-other"}) + require.NoError(t, err) + resp, err := h.handleExportWorkflow(scopedMCPCtx(), &ws.Message{ID: "req-1", Action: "test", Payload: raw}) + require.NoError(t, err) + requireErrorCode(t, resp, ws.ErrorCodeInternalError) +} diff --git a/apps/backend/internal/task/service/service_access.go b/apps/backend/internal/task/service/service_access.go index 4b55d4e8ea..feb48d241a 100644 --- a/apps/backend/internal/task/service/service_access.go +++ b/apps/backend/internal/task/service/service_access.go @@ -2,6 +2,7 @@ package service import ( "context" + "errors" "github.com/kandev/kandev/internal/auth/authn" "github.com/kandev/kandev/internal/task/models" @@ -93,8 +94,16 @@ func (s *Service) authorizeWorkflowID(ctx context.Context, workflowID string) er return nil } workspace, err := s.workspaces.GetWorkspace(ctx, workflow.WorkspaceID) - if err != nil { - return nil //nolint:nilerr // visibility fallback, not an operation failure + switch { + case errors.Is(err, repoerrors.ErrWorkspaceNotFound): + // A dangling workspace reference should not hide the workflow from the + // single user who can already see everything else about it. + return nil + case err != nil: + // Anything else is a failed lookup, not an answer. Treating it as + // "authorized" would let a transient database error hand a guessed + // workflow ID to whoever asked, so it fails closed by propagating. + return err } if !workspaceVisibleTo(workspace, userID) { return repoerrors.ErrWorkspaceNotFound @@ -108,6 +117,13 @@ func (s *Service) AuthorizeTaskAccess(ctx context.Context, taskID string) error return s.authorizeTaskID(ctx, taskID) } +// AuthorizeWorkflowAccess is the public form of authorizeWorkflowID, consumed +// by the workflow service, whose step/export/import surface reaches workflows +// by ID but does not own workspace permissions. +func (s *Service) AuthorizeWorkflowAccess(ctx context.Context, workflowID string) error { + return s.authorizeWorkflowID(ctx, workflowID) +} + // AuthorizeWorkspaceAccess is the public form of authorizeWorkspaceID, // consumed by the office route-scoping middleware. func (s *Service) AuthorizeWorkspaceAccess(ctx context.Context, workspaceID string) error { diff --git a/apps/backend/internal/task/service/service_access_test.go b/apps/backend/internal/task/service/service_access_test.go index 8666b6bed0..6ec1c460d4 100644 --- a/apps/backend/internal/task/service/service_access_test.go +++ b/apps/backend/internal/task/service/service_access_test.go @@ -10,6 +10,7 @@ import ( "github.com/kandev/kandev/internal/auth/authn" commonlogger "github.com/kandev/kandev/internal/common/logger" "github.com/kandev/kandev/internal/task/models" + "github.com/kandev/kandev/internal/task/repository" "github.com/kandev/kandev/internal/task/repository/repoerrors" v1 "github.com/kandev/kandev/pkg/api/v1" ) @@ -179,6 +180,61 @@ func TestCreateWorkspaceStampsOwner(t *testing.T) { } } +// TestAuthorizeWorkflowAccess covers the public wrapper the workflow service +// authorizes its step, export and import surface through. The workflow package +// owns those routes but not the permission: a workflow is visible exactly when +// its workspace is. +func TestAuthorizeWorkflowAccess(t *testing.T) { + svc, _, repo := createTestService(t) + seedScopedWorkspaces(t, repo) + if err := repo.CreateWorkflow(context.Background(), &models.Workflow{ + ID: "wf-legacy", WorkspaceID: "ws-legacy", Name: "Pre-auth flow", + }); err != nil { + t.Fatalf("create legacy workflow: %v", err) + } + + if err := svc.AuthorizeWorkflowAccess(ctxAs("user-a"), "wf-b"); !errors.Is(err, repoerrors.ErrWorkspaceNotFound) { + t.Fatalf("foreign workflow access: %v", err) + } + if err := svc.AuthorizeWorkflowAccess(ctxAs("user-b"), "wf-b"); err != nil { + t.Fatalf("owner workflow access: %v", err) + } + // Identity-less internal callers and the synthetic auth-disabled identity + // keep the pre-auth see-everything behavior. + if err := svc.AuthorizeWorkflowAccess(context.Background(), "wf-b"); err != nil { + t.Fatalf("internal workflow access: %v", err) + } + if err := svc.AuthorizeWorkflowAccess(ctxSynthetic(), "wf-b"); err != nil { + t.Fatalf("synthetic workflow access: %v", err) + } + // An unowned workspace stays visible until the setup wizard claims it. + if err := svc.AuthorizeWorkflowAccess(ctxAs("user-a"), "wf-legacy"); err != nil { + t.Fatalf("legacy workflow access: %v", err) + } + // A failed workspace lookup is not an answer. The dangling-reference + // fallback below it returns nil, so a transient database error used to + // read as "authorized" and hand a guessed workflow ID to whoever asked. + broken := errors.New("database is locked") + svc.workspaces = &brokenWorkspaceReader{WorkspaceRepository: repo, err: broken} + if err := svc.AuthorizeWorkflowAccess(ctxAs("user-a"), "wf-b"); !errors.Is(err, broken) { + t.Fatalf("failed workspace lookup: %v, want the lookup error", err) + } + // A workspace row that is genuinely gone keeps the documented fallback: + // the workflow stays visible rather than disappearing from its owner. + svc.workspaces = &brokenWorkspaceReader{WorkspaceRepository: repo, err: repoerrors.ErrWorkspaceNotFound} + if err := svc.AuthorizeWorkflowAccess(ctxAs("user-a"), "wf-b"); err != nil { + t.Fatalf("dangling workspace reference: %v, want the visibility fallback", err) + } + svc.workspaces = repo + + // A workflow that does not exist is reported as the repository reports it, + // which the workflow service classifies as not-visible so a missing + // workflow and a foreign one answer identically. + if err := svc.AuthorizeWorkflowAccess(ctxAs("user-a"), "wf-missing"); err == nil { + t.Fatal("missing workflow access: want an error") + } +} + func TestAuthorizeSessionAccess(t *testing.T) { svc, _, repo := createTestService(t) seedScopedWorkspaces(t, repo) @@ -332,3 +388,14 @@ func TestRepositoryScriptScoping(t *testing.T) { t.Fatalf("foreign GetRepositoryScript: %v", err) } } + +// brokenWorkspaceReader answers every workspace lookup with a fixed error, +// standing in for a database that is failing rather than empty. +type brokenWorkspaceReader struct { + repository.WorkspaceRepository + err error +} + +func (r *brokenWorkspaceReader) GetWorkspace(context.Context, string) (*models.Workspace, error) { + return nil, r.err +} diff --git a/apps/backend/internal/workflow/controller/controller.go b/apps/backend/internal/workflow/controller/controller.go index 2f9d69c44d..6d3716396a 100644 --- a/apps/backend/internal/workflow/controller/controller.go +++ b/apps/backend/internal/workflow/controller/controller.go @@ -9,6 +9,17 @@ import ( "github.com/kandev/kandev/internal/workflow/service" ) +// Per-user scoping lives here rather than in the HTTP handlers because the +// controller is the single entry point every user-facing transport shares: +// the REST routes, the WebSocket actions (which the gateway's dispatch +// backstop cannot cover — it parses no workflow_id or step id) and the MCP +// tool handlers. A guard added one layer up would leave the other two open. +// The checks themselves belong to the task domain and reach it through the +// service's Authorize* helpers (internal/workflow/service/access.go). +// +// Workflow templates are deliberately unscoped: they are install-global, +// read-only definitions with no owner. + // Controller handles workflow-related requests type Controller struct { svc *service.Service @@ -66,6 +77,9 @@ type CreateStepsFromTemplateRequest struct { } func (c *Controller) ListStepsByWorkflow(ctx context.Context, req ListStepsRequest) (*ListStepsResponse, error) { + if err := c.svc.AuthorizeWorkflow(ctx, req.WorkflowID); err != nil { + return nil, err + } steps, err := c.svc.ListStepsByWorkflow(ctx, req.WorkflowID) if err != nil { return nil, err @@ -75,6 +89,9 @@ func (c *Controller) ListStepsByWorkflow(ctx context.Context, req ListStepsReque // ListStepsByWorkspace returns all workflow steps for all workflows in a workspace. func (c *Controller) ListStepsByWorkspace(ctx context.Context, workspaceID string) (*ListStepsResponse, error) { + // Authorized inside ListStepsByWorkspaceID rather than here: the + // boot-payload builder calls that method without passing through this + // controller, so the service is the layer both entry points share. steps, err := c.svc.ListStepsByWorkspaceID(ctx, workspaceID) if err != nil { return nil, err @@ -83,6 +100,9 @@ func (c *Controller) ListStepsByWorkspace(ctx context.Context, workspaceID strin } func (c *Controller) GetStep(ctx context.Context, id string) (*GetStepResponse, error) { + if err := c.svc.AuthorizeStep(ctx, id); err != nil { + return nil, err + } step, err := c.svc.GetStep(ctx, id) if err != nil { return nil, err @@ -91,6 +111,9 @@ func (c *Controller) GetStep(ctx context.Context, id string) (*GetStepResponse, } func (c *Controller) CreateStepsFromTemplate(ctx context.Context, req CreateStepsFromTemplateRequest) error { + if err := c.svc.AuthorizeWorkflow(ctx, req.WorkflowID); err != nil { + return err + } if err := c.svc.EnsureWorkflowMutable(ctx, req.WorkflowID); err != nil { return err } @@ -117,6 +140,11 @@ type CreateStepRequest struct { // CreateStep creates a new workflow step. func (c *Controller) CreateStep(ctx context.Context, req CreateStepRequest) (*GetStepResponse, error) { + // The new step names its workflow in the body; authorize that workflow + // before anything else reads or writes. + if err := c.svc.AuthorizeWorkflow(ctx, req.WorkflowID); err != nil { + return nil, err + } if err := c.svc.EnsureWorkflowMutable(ctx, req.WorkflowID); err != nil { return nil, err } @@ -157,7 +185,7 @@ func (c *Controller) CreateStep(ctx context.Context, req CreateStepRequest) (*Ge if req.PullFromStepID != nil { step.PullFromStepID = strings.TrimSpace(*req.PullFromStepID) } - if err := c.validatePullFromStep(ctx, step); err != nil { + if err := c.validateStepReferences(ctx, step); err != nil { return nil, err } demotedStartSteps, err := c.svc.CreateStepWithStartStepUpdates(ctx, step) @@ -189,6 +217,9 @@ type UpdateStepRequest struct { // UpdateStep updates an existing workflow step. func (c *Controller) UpdateStep(ctx context.Context, req UpdateStepRequest) (*GetStepResponse, error) { + if err := c.svc.AuthorizeStep(ctx, req.ID); err != nil { + return nil, err + } step, err := c.svc.GetStep(ctx, req.ID) if err != nil { return nil, err @@ -244,7 +275,7 @@ func (c *Controller) UpdateStep(ctx context.Context, req UpdateStepRequest) (*Ge if req.PullFromStepID != nil { step.PullFromStepID = strings.TrimSpace(*req.PullFromStepID) } - if err := c.validatePullFromStep(ctx, step); err != nil { + if err := c.validateStepReferences(ctx, step); err != nil { return nil, err } demotedStartSteps, err := c.svc.UpdateStepWithStartStepUpdates(ctx, step) @@ -254,6 +285,55 @@ func (c *Controller) UpdateStep(ctx context.Context, req UpdateStepRequest) (*Ge return &GetStepResponse{Step: step, DemotedStartSteps: demotedStartSteps}, nil } +// validateStepReferences checks every ID the step carries that names another +// resource: its pull source, its move_to_step transition targets, and the +// tasks its queue_run actions would start work on. +// +// Two different rejections, deliberately. A reference the caller cannot see is +// not-found, indistinguishable from one that does not exist. A step the caller +// *can* see but that lives in another workflow is a validation error naming +// the reason, because there is nothing to hide from them and the editor has to +// be able to explain it. +func (c *Controller) validateStepReferences(ctx context.Context, step *models.WorkflowStep) error { + if err := c.validatePullFromStep(ctx, step); err != nil { + return err + } + refs := models.CollectStepEventReferences(step.Events) + for _, targetID := range refs.StepIDs { + if err := c.validateEventStepTarget(ctx, step, targetID); err != nil { + return err + } + } + for _, taskID := range refs.TaskIDs { + if err := c.svc.AuthorizeTask(ctx, taskID); err != nil { + return err + } + } + return nil +} + +// validateEventStepTarget keeps a transition inside its own workflow. The +// engine dereferences the target when the trigger fires and drives the task +// with whatever it finds there — that step's prompt and agent profile — so a +// target in somebody else's workflow both parks the task outside its board and +// runs it under their configuration. +func (c *Controller) validateEventStepTarget(ctx context.Context, step *models.WorkflowStep, targetID string) error { + if targetID == step.ID { + return nil // a self-transition names no other step + } + if err := c.svc.AuthorizeStep(ctx, targetID); err != nil { + return err + } + target, err := c.svc.GetStep(ctx, targetID) + if err != nil { + return fmt.Errorf("move_to_step target is invalid: %w", err) + } + if target.WorkflowID != step.WorkflowID { + return fmt.Errorf("move_to_step must reference a step in the same workflow") + } + return nil +} + func (c *Controller) validatePullFromStep(ctx context.Context, step *models.WorkflowStep) error { if step.PullFromStepID == "" { return nil @@ -261,6 +341,12 @@ func (c *Controller) validatePullFromStep(ctx context.Context, step *models.Work if step.ID != "" && step.PullFromStepID == step.ID { return fmt.Errorf("pull_from_step_id cannot reference the same step") } + // Authorizing the pull source keeps a foreign step from being told apart + // from a nonexistent one: without this, "must reference a step in the same + // workflow" would confirm that somebody else's step ID exists. + if err := c.svc.AuthorizeStep(ctx, step.PullFromStepID); err != nil { + return fmt.Errorf("pull_from_step_id is invalid: %w", err) + } source, err := c.svc.GetStep(ctx, step.PullFromStepID) if err != nil { return fmt.Errorf("pull_from_step_id is invalid: %w", err) @@ -301,6 +387,9 @@ func (c *Controller) validatePullFromStepAcyclic(ctx context.Context, stepID str // DeleteStep deletes a workflow step. func (c *Controller) DeleteStep(ctx context.Context, id string) error { + if err := c.svc.AuthorizeStep(ctx, id); err != nil { + return err + } step, err := c.svc.GetStep(ctx, id) if err != nil { return err @@ -319,6 +408,11 @@ type ReorderStepsRequest struct { // ReorderSteps reorders workflow steps for a workflow. func (c *Controller) ReorderSteps(ctx context.Context, req ReorderStepsRequest) error { + // The workflow is authorized here; the individual step IDs are authorized + // by the service, which is where the reorder actually resolves them. + if err := c.svc.AuthorizeWorkflow(ctx, req.WorkflowID); err != nil { + return err + } if err := c.svc.EnsureWorkflowMutable(ctx, req.WorkflowID); err != nil { return err } @@ -352,6 +446,8 @@ type ImportWorkflowsRequest struct { } // ExportWorkflow exports a single workflow. +// The export/import trio authorizes inside the service: the MCP config tools +// call those methods directly instead of coming through this controller. func (c *Controller) ExportWorkflow(ctx context.Context, workflowID string) (*models.WorkflowExport, error) { return c.svc.ExportWorkflow(ctx, workflowID) } diff --git a/apps/backend/internal/workflow/handlers/handlers.go b/apps/backend/internal/workflow/handlers/handlers.go index fbe7b809e8..110bdec7a8 100644 --- a/apps/backend/internal/workflow/handlers/handlers.go +++ b/apps/backend/internal/workflow/handlers/handlers.go @@ -117,6 +117,9 @@ func (h *Handlers) httpListStepsByWorkflow(c *gin.Context) { }) if err != nil { h.logger.Error("failed to list steps", zap.Error(err)) + if writeNotVisible(c, err, "Workflow not found") { + return + } c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to list steps"}) return } @@ -127,6 +130,9 @@ func (h *Handlers) httpListStepsByWorkspace(c *gin.Context) { resp, err := h.controller.ListStepsByWorkspace(c.Request.Context(), c.Param("id")) if err != nil { h.logger.Error("failed to list steps by workspace", zap.Error(err)) + if writeNotVisible(c, err, "Workspace not found") { + return + } c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to list steps"}) return } @@ -205,11 +211,26 @@ func (h *Handlers) httpUpdateStep(c *gin.Context) { c.JSON(http.StatusOK, resp.Step) } +// writeNotVisible answers 404 for a workflow, workspace or step the caller may +// not see, and reports whether it handled the error. The same 404 covers a +// resource that does not exist, so neither response reveals which it was. +func writeNotVisible(c *gin.Context, err error, message string) bool { + if !errors.Is(err, service.ErrNotVisible) { + return false + } + c.JSON(http.StatusNotFound, gin.H{"error": message}) + return true +} + func (h *Handlers) writeStepMutationError(c *gin.Context, err error) { if errors.Is(err, service.ErrWorkflowReadOnly) || errors.Is(err, service.ErrWorkflowWorkspaceReadOnly) { c.JSON(http.StatusConflict, gin.H{"error": err.Error()}) return } + // A step the caller may not see reads exactly like one that is not there. + if writeNotVisible(c, err, "Step not found") { + return + } msg := strings.ToLower(err.Error()) switch { case isStepValidationError(msg): @@ -232,7 +253,10 @@ func (h *Handlers) httpDeleteStep(c *gin.Context) { ctx := c.Request.Context() stepID := c.Param("id") stepResp, getErr := h.controller.GetStep(ctx, stepID) - if getErr != nil { + // A step the caller may not see is the ordinary rejection below, not a + // failure to read one — warning about it would file every unauthorized + // delete under infrastructure trouble. + if getErr != nil && !errors.Is(getErr, service.ErrNotVisible) { h.logger.Warn("failed to fetch step before delete; workflow_step.deleted event will not be published", zap.String("step_id", stepID), zap.Error(getErr)) } @@ -305,6 +329,9 @@ func (h *Handlers) httpExportWorkflow(c *gin.Context) { resp, err := h.controller.ExportWorkflow(c.Request.Context(), c.Param("id")) if err != nil { h.logger.Error("failed to export workflow", zap.Error(err)) + if writeNotVisible(c, err, "Workflow not found") { + return + } c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to export workflow"}) return } @@ -315,6 +342,9 @@ func (h *Handlers) httpExportWorkflows(c *gin.Context) { resp, err := h.controller.ExportWorkflows(c.Request.Context(), c.Param("id"), parseExportIDs(c)) if err != nil { h.logger.Error("failed to export workflows", zap.Error(err)) + if writeNotVisible(c, err, "Workspace not found") { + return + } c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to export workflows"}) return } @@ -357,6 +387,9 @@ func (h *Handlers) httpImportWorkflows(c *gin.Context) { resp, err := h.controller.ImportWorkflows(c.Request.Context(), req) if err != nil { h.logger.Error("failed to import workflows", zap.Error(err)) + if writeNotVisible(c, err, "Workspace not found") { + return + } c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) return } @@ -435,6 +468,9 @@ func (h *Handlers) wsCreateStepsFromTemplate(ctx context.Context, msg *ws.Messag TemplateID: req.TemplateID, }); err != nil { h.logger.Error("failed to create steps", zap.Error(err)) + if errors.Is(err, service.ErrNotVisible) { + return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeNotFound, notFoundMessage, nil) + } return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeInternalError, "Failed to create steps", nil) } return ws.NewResponse(msg.ID, msg.Action, map[string]bool{"success": true}) diff --git a/apps/backend/internal/workflow/handlers/scope_harness_test.go b/apps/backend/internal/workflow/handlers/scope_harness_test.go new file mode 100644 index 0000000000..dd28c74fcb --- /dev/null +++ b/apps/backend/internal/workflow/handlers/scope_harness_test.go @@ -0,0 +1,390 @@ +package handlers + +import ( + "context" + "database/sql" + "database/sql/driver" + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "sync" + "testing" + + sqlite3 "github.com/mattn/go-sqlite3" + + "github.com/kandev/kandev/internal/auth/authn" + "github.com/kandev/kandev/internal/common/logger" + taskmodels "github.com/kandev/kandev/internal/task/models" + "github.com/kandev/kandev/internal/task/repository/repoerrors" + ws "github.com/kandev/kandev/pkg/websocket" +) + +// The workflow-step surface reaches any workflow, workspace or step by ID. +// Scoping it means the guard has to run *before* the repository, so these +// tests need to see the statements the request actually issued, not just its +// status code: a guard that rejects after reading the row still read the row, +// and a guard deleted from one path still answers 404 on the sibling paths. +// +// stepQueryLog is that instrument. A wrapper driver records every statement +// naming workflow_steps, so a test can assert "denied, and the table was never +// touched" and can tell a real delete from a rejected one. + +const countingDriverName = "sqlite3-workflow-scope" + +// stepQueries is process-global because a database/sql driver is registered by +// name, not per pool. The tests in this package run sequentially (none call +// t.Parallel), and each scoped harness resets the log at setup. +var stepQueries = &stepQueryLog{} + +func init() { + sql.Register(countingDriverName, &countingDriver{inner: &sqlite3.SQLiteDriver{}, log: stepQueries}) +} + +type stepQueryLog struct { + mu sync.Mutex + statements []string +} + +func (l *stepQueryLog) record(query string) { + if !strings.Contains(query, "workflow_steps") { + return + } + l.mu.Lock() + defer l.mu.Unlock() + l.statements = append(l.statements, query) +} + +func (l *stepQueryLog) reset() { + l.mu.Lock() + defer l.mu.Unlock() + l.statements = nil +} + +func (l *stepQueryLog) all() []string { + l.mu.Lock() + defer l.mu.Unlock() + return append([]string(nil), l.statements...) +} + +// writes returns the statements that would have changed the table. +func (l *stepQueryLog) writes() []string { + var out []string + for _, stmt := range l.all() { + switch strings.ToUpper(strings.Fields(strings.TrimSpace(stmt))[0]) { + case "INSERT", "UPDATE", "DELETE": + out = append(out, stmt) + } + } + return out +} + +type countingDriver struct { + inner driver.Driver + log *stepQueryLog +} + +func (d *countingDriver) Open(name string) (driver.Conn, error) { + conn, err := d.inner.Open(name) + if err != nil { + return nil, err + } + return &countingConn{Conn: conn, log: d.log}, nil +} + +type countingConn struct { + driver.Conn + log *stepQueryLog +} + +func (c *countingConn) QueryContext(ctx context.Context, query string, args []driver.NamedValue) (driver.Rows, error) { + c.log.record(query) + return c.Conn.(driver.QueryerContext).QueryContext(ctx, query, args) +} + +func (c *countingConn) ExecContext(ctx context.Context, query string, args []driver.NamedValue) (driver.Result, error) { + c.log.record(query) + return c.Conn.(driver.ExecerContext).ExecContext(ctx, query, args) +} + +func (c *countingConn) PrepareContext(ctx context.Context, query string) (driver.Stmt, error) { + c.log.record(query) + return c.Conn.(driver.ConnPrepareContext).PrepareContext(ctx, query) +} + +func (c *countingConn) BeginTx(ctx context.Context, opts driver.TxOptions) (driver.Tx, error) { + return c.Conn.(driver.ConnBeginTx).BeginTx(ctx, opts) +} + +// scopedOwnership is the task-domain seam the workflow service authorizes +// through. It mirrors internal/task/service's rule exactly: no identity, or a +// synthetic one, means unscoped; a workspace with no owner stays visible to +// everyone; a denial is reported as not-found so existence does not leak. +// +// internal/task/service's own tests cover the real implementation +// (TestAuthorizeWorkflowAccess); this fake exists so the handler tests can +// assert *when* the workflow service calls out, not just the outcome. +type scopedOwnership struct { + workflowWorkspace map[string]string + taskWorkspace map[string]string + workspaceOwner map[string]string + + mu sync.Mutex + workflowCalls []string + workspaceCalls []string + taskCalls []string +} + +func (o *scopedOwnership) authorizeWorkflow(ctx context.Context, workflowID string) error { + o.mu.Lock() + o.workflowCalls = append(o.workflowCalls, workflowID) + o.mu.Unlock() + userID, scoped := scopeOf(ctx) + if !scoped { + return nil + } + workspaceID, ok := o.workflowWorkspace[workflowID] + if !ok { + // The task repository reports a missing workflow as a formatted error, + // not a sentinel (internal/task/repository/sqlite/workflow.go). + return fmt.Errorf("workflow not found: %s", workflowID) + } + return o.visible(userID, workspaceID) +} + +func (o *scopedOwnership) authorizeWorkspace(ctx context.Context, workspaceID string) error { + o.mu.Lock() + o.workspaceCalls = append(o.workspaceCalls, workspaceID) + o.mu.Unlock() + userID, scoped := scopeOf(ctx) + if !scoped { + return nil + } + return o.visible(userID, workspaceID) +} + +func (o *scopedOwnership) authorizeTask(ctx context.Context, taskID string) error { + o.mu.Lock() + o.taskCalls = append(o.taskCalls, taskID) + o.mu.Unlock() + userID, scoped := scopeOf(ctx) + if !scoped { + return nil + } + workspaceID, ok := o.taskWorkspace[taskID] + if !ok { + // The task service reports a foreign or missing task with the task + // sentinel, not the workspace one. + return repoerrors.ErrTaskNotFound + } + if err := o.visible(userID, workspaceID); err != nil { + return repoerrors.ErrTaskNotFound + } + return nil +} + +func (o *scopedOwnership) visible(userID, workspaceID string) error { + owner, ok := o.workspaceOwner[workspaceID] + if !ok { + return repoerrors.ErrWorkspaceNotFound + } + if owner != "" && owner != userID { + return repoerrors.ErrWorkspaceNotFound + } + return nil +} + +func (o *scopedOwnership) calls() (workflows, workspaces, tasks []string) { + o.mu.Lock() + defer o.mu.Unlock() + return append([]string(nil), o.workflowCalls...), + append([]string(nil), o.workspaceCalls...), + append([]string(nil), o.taskCalls...) +} + +func (o *scopedOwnership) resetCalls() { + o.mu.Lock() + defer o.mu.Unlock() + o.workflowCalls, o.workspaceCalls, o.taskCalls = nil, nil, nil +} + +func scopeOf(ctx context.Context) (string, bool) { + identity, ok := authn.IdentityFromContext(ctx) + if !ok || identity.Synthetic { + return "", false + } + return identity.UserID, true +} + +// scopedHarness is the workflow harness with the ownership checkers wired, two +// workspaces owned by different users, and a step in each. +type scopedHarness struct { + *workflowHarness + owner *scopedOwnership + provider *fakeWorkflowProvider + queries *stepQueryLog + stepA string + stepB string + stepB2 string +} + +const ( + userA = "user-a" + userB = "user-b" +) + +func setupScopedRouter(t *testing.T, logOverride ...*logger.Logger) *scopedHarness { + t.Helper() + h := setupStepRouterWithDriver(t, countingDriverName, logOverride...) + if _, err := h.db.Exec( + `INSERT INTO workflows (id, workspace_id, name, created_at, updated_at) + VALUES ('wf-a','ws-a','A Flow',CURRENT_TIMESTAMP,CURRENT_TIMESTAMP), + ('wf-b','ws-b','B Flow',CURRENT_TIMESTAMP,CURRENT_TIMESTAMP), + ('wf-b2','ws-b','B Second Flow',CURRENT_TIMESTAMP,CURRENT_TIMESTAMP)`); err != nil { + t.Fatalf("seed workflows: %v", err) + } + // Seed the steps before the checkers are wired: an identity-less request is + // unscoped either way, but this keeps the seed out of the call log. + stepA := createStepViaHTTP(t, h.router, map[string]interface{}{ + "workflow_id": "wf-a", "name": "A Backlog", "position": 0, + }) + stepB := createStepViaHTTP(t, h.router, map[string]interface{}{ + "workflow_id": "wf-b", "name": "B Backlog", "position": 0, + }) + // A second workflow for the same owner, so "not yours" and "not this + // workflow" can be told apart. + stepB2 := createStepViaHTTP(t, h.router, map[string]interface{}{ + "workflow_id": "wf-b2", "name": "B Second Backlog", "position": 0, + }) + + provider := &fakeWorkflowProvider{workflows: []*taskmodels.Workflow{ + {ID: "wf-a", WorkspaceID: "ws-a", Name: "A Flow"}, + {ID: "wf-b", WorkspaceID: "ws-b", Name: "B Flow"}, + }} + h.service.SetWorkflowProvider(provider) + + owner := &scopedOwnership{ + workflowWorkspace: map[string]string{"wf-a": "ws-a", "wf-b": "ws-b", "wf-b2": "ws-b"}, + taskWorkspace: map[string]string{"task-a": "ws-a", "task-b": "ws-b"}, + workspaceOwner: map[string]string{"ws-a": userA, "ws-b": userB}, + } + h.service.SetWorkflowAccessChecker(owner.authorizeWorkflow) + h.service.SetWorkspaceAccessChecker(owner.authorizeWorkspace) + h.service.SetTaskAccessChecker(owner.authorizeTask) + + stepQueries.reset() + return &scopedHarness{ + workflowHarness: h, owner: owner, provider: provider, queries: stepQueries, + stepA: stepA.ID, stepB: stepB.ID, stepB2: stepB2.ID, + } +} + +// asUser attaches a real (non-synthetic) identity, the way the auth middleware +// does for an authenticated request. +func asUser(userID string) context.Context { + return authn.WithIdentity(context.Background(), authn.Identity{UserID: userID, Role: authn.RoleMember}) +} + +// asSyntheticUser attaches the implicit identity injected when authentication +// is disabled. +func asSyntheticUser() context.Context { + return authn.WithIdentity(context.Background(), authn.Identity{UserID: userA, Synthetic: true}) +} + +// doAs issues an HTTP request carrying ctx's identity. +func doAs(t *testing.T, h *scopedHarness, ctx context.Context, method, path string, body any) *httptest.ResponseRecorder { + t.Helper() + var payload []byte + if body != nil { + var err error + payload, err = json.Marshal(body) + if err != nil { + t.Fatalf("marshal body: %v", err) + } + } + return doRawAs(t, h, ctx, method, path, string(payload)) +} + +// doRawAs issues a request with a raw (possibly non-JSON) body under ctx's identity. +func doRawAs(t *testing.T, h *scopedHarness, ctx context.Context, method, path, body string) *httptest.ResponseRecorder { + t.Helper() + req := httptest.NewRequest(method, path, strings.NewReader(body)).WithContext(ctx) + req.Header.Set("Content-Type", "application/json") + rec := httptest.NewRecorder() + h.router.ServeHTTP(rec, req) + return rec +} + +// dispatchAs routes one WS message through the registered handlers under ctx's +// identity, the way the gateway dispatches an authenticated client's action. +func dispatchAs(t *testing.T, h *scopedHarness, ctx context.Context, action string, payload any) *ws.Message { + t.Helper() + raw, err := json.Marshal(payload) + if err != nil { + t.Fatalf("marshal payload: %v", err) + } + msg := &ws.Message{ID: "req-1", Type: ws.MessageTypeRequest, Action: action, Payload: raw} + resp, err := h.dispatcher.Dispatch(ctx, msg) + if err != nil { + t.Fatalf("dispatch %s: %v", action, err) + } + if resp == nil { + t.Fatalf("dispatch %s returned no message", action) + } + return resp +} + +// requireStepUntouched asserts the workflow_steps table was never read or +// written while serving the request under test. +func requireStepTableUntouched(t *testing.T, h *scopedHarness) { + t.Helper() + if stmts := h.queries.all(); len(stmts) != 0 { + t.Fatalf("guard let %d workflow_steps statement(s) through: %v", len(stmts), stmts) + } +} + +// requireNoStepWrites asserts nothing mutated the table. Step-keyed routes +// resolve the step's owning workflow through the repository on purpose, so +// only writes can be forbidden there. +func requireNoStepWrites(t *testing.T, h *scopedHarness) { + t.Helper() + if stmts := h.queries.writes(); len(stmts) != 0 { + t.Fatalf("denied request wrote to workflow_steps: %v", stmts) + } +} + +// stepNames returns the persisted step names for a workflow, so a test can +// prove a rejected mutation left the rows alone. +func stepNames(t *testing.T, h *scopedHarness, workflowID string) []string { + t.Helper() + steps, err := h.repo.ListStepsByWorkflow(context.Background(), workflowID) + if err != nil { + t.Fatalf("list steps: %v", err) + } + names := make([]string, 0, len(steps)) + for _, step := range steps { + names = append(names, step.Name) + } + return names +} + +// decodeJSON decodes a successful response body into out. +func decodeJSON(t *testing.T, rec *httptest.ResponseRecorder, out any) { + t.Helper() + if err := json.Unmarshal(rec.Body.Bytes(), out); err != nil { + t.Fatalf("decode %s: %v", rec.Body.String(), err) + } +} + +func requireStatus(t *testing.T, rec *httptest.ResponseRecorder, want int) { + t.Helper() + if rec.Code != want { + t.Fatalf("status = %d, want %d; body = %s", rec.Code, want, rec.Body.String()) + } +} + +func requireNotFound(t *testing.T, rec *httptest.ResponseRecorder) { + t.Helper() + requireStatus(t, rec, http.StatusNotFound) +} diff --git a/apps/backend/internal/workflow/handlers/step_events_test.go b/apps/backend/internal/workflow/handlers/step_events_test.go index 13445e78e7..213b16088e 100644 --- a/apps/backend/internal/workflow/handlers/step_events_test.go +++ b/apps/backend/internal/workflow/handlers/step_events_test.go @@ -116,13 +116,23 @@ func (h *workflowHarness) dispatch(t *testing.T, action string, payload any) *ws func setupStepRouter(t *testing.T) *workflowHarness { t.Helper() - rawDB, err := sql.Open("sqlite3", ":memory:") + return setupStepRouterWithDriver(t, "sqlite3") +} + +// setupStepRouterWithDriver builds the harness over a named SQL driver, so the +// scoping tests can substitute a statement-counting driver and prove a guard +// rejected a request without the repository ever being queried. +func setupStepRouterWithDriver(t *testing.T, driverName string, logOverride ...*logger.Logger) *workflowHarness { + t.Helper() + rawDB, err := sql.Open(driverName, ":memory:") if err != nil { t.Fatalf("open sqlite: %v", err) } // Pin the pool to one connection: every connection to an in-memory SQLite // DB gets its own database, so a second one would not see the schema. rawDB.SetMaxOpenConns(1) + // The sqlx driver name only selects the bindvar style, so it stays + // "sqlite3" even when the pool was opened through a wrapper driver. db := sqlx.NewDb(rawDB, "sqlite3") t.Cleanup(func() { _ = db.Close() }) @@ -141,6 +151,9 @@ func setupStepRouter(t *testing.T) *workflowHarness { t.Fatalf("new repository: %v", err) } log := logger.Default() + if len(logOverride) > 0 { + log = logOverride[0] + } eventBus := bus.NewMemoryEventBus(log) workflowSvc := service.NewService(repo, log) t.Cleanup(func() { _ = workflowSvc.Close() }) diff --git a/apps/backend/internal/workflow/handlers/step_reference_scope_test.go b/apps/backend/internal/workflow/handlers/step_reference_scope_test.go new file mode 100644 index 0000000000..8ca7eb1583 --- /dev/null +++ b/apps/backend/internal/workflow/handlers/step_reference_scope_test.go @@ -0,0 +1,336 @@ +package handlers + +import ( + "context" + "fmt" + "net/http" + "strings" + "testing" + + taskmodels "github.com/kandev/kandev/internal/task/models" +) + +// A workflow step stores caller-supplied IDs *inside* its payload: a +// `move_to_step` action names the step to transition to, and a `queue_run` +// action names the task to queue work on. Authorizing the step being written +// says nothing about those, and the engine dereferences them later with no +// request identity to check against — so write time is the only place they can +// be checked. +// +// `pull_from_step_id` already had the same-workflow rule; these are the +// references that did not. + +// moveToStepBody builds a step payload whose on_turn_complete transitions to +// the named step. +func moveToStepBody(workflowID, name, targetStepID string) map[string]any { + return map[string]any{ + "workflow_id": workflowID, + "name": name, + "position": 5, + "events": map[string]any{ + "on_turn_complete": []map[string]any{ + {"type": "move_to_step", "config": map[string]any{"step_id": targetStepID}}, + }, + }, + } +} + +// queueRunBody builds a step payload whose on_comment queues a run against the +// named task. +func queueRunBody(workflowID, name, taskID string) map[string]any { + return map[string]any{ + "workflow_id": workflowID, + "name": name, + "position": 6, + "events": map[string]any{ + "on_comment": []map[string]any{ + {"type": "queue_run", "config": map[string]any{"task_id": taskID, "target": "primary"}}, + }, + }, + } +} + +// TestMoveToStepTargetCannotLeaveTheWorkflow covers the transition target on +// both the create and the update path, over HTTP. +func TestMoveToStepTargetCannotLeaveTheWorkflow(t *testing.T) { + t.Run("create cannot target another user's step", func(t *testing.T) { + h := setupScopedRouter(t) + h.queries.reset() + rec := doAs(t, h, asUser(userB), http.MethodPost, "/api/v1/workflow/steps", + moveToStepBody("wf-b", "Smuggler", h.stepA)) + requireNotFound(t, rec) + requireNoStepWrites(t, h) + }) + + t.Run("update cannot target another user's step", func(t *testing.T) { + h := setupScopedRouter(t) + h.queries.reset() + rec := doAs(t, h, asUser(userB), http.MethodPut, "/api/v1/workflow/steps/"+h.stepB, + map[string]any{"events": map[string]any{ + "on_turn_complete": []map[string]any{ + {"type": "move_to_step", "config": map[string]any{"step_id": h.stepA}}, + }, + }}) + requireNotFound(t, rec) + requireNoStepWrites(t, h) + }) + + t.Run("a foreign target answers exactly like a nonexistent one", func(t *testing.T) { + h := setupScopedRouter(t) + foreign := doAs(t, h, asUser(userB), http.MethodPost, "/api/v1/workflow/steps", + moveToStepBody("wf-b", "Smuggler", h.stepA)) + missing := doAs(t, h, asUser(userB), http.MethodPost, "/api/v1/workflow/steps", + moveToStepBody("wf-b", "Smuggler", "no-such-step")) + if foreign.Code != missing.Code || foreign.Body.String() != missing.Body.String() { + t.Fatalf("foreign target %d %s differs from missing target %d %s", + foreign.Code, foreign.Body.String(), missing.Code, missing.Body.String()) + } + }) + + // A step in the caller's *other* workflow is visible to them, so there is + // nothing to hide: this is an integrity rejection, and it must read as one + // rather than as a 404, or the UI cannot tell the user what is wrong. + t.Run("a step in the caller's other workflow is a validation error", func(t *testing.T) { + h := setupScopedRouter(t) + h.queries.reset() + rec := doAs(t, h, asUser(userB), http.MethodPost, "/api/v1/workflow/steps", + moveToStepBody("wf-b", "Crosser", h.stepB2)) + requireStatus(t, rec, http.StatusBadRequest) + if !strings.Contains(rec.Body.String(), "same workflow") { + t.Fatalf("body = %s, want the same-workflow reason", rec.Body.String()) + } + requireNoStepWrites(t, h) + }) + + // The invariant is not an authentication feature: a single-user install + // must not be able to point a transition at another workflow's step + // either, exactly as it cannot for pull_from_step_id. + t.Run("the same-workflow rule holds with auth disabled", func(t *testing.T) { + h := setupScopedRouter(t) + rec := doAs(t, h, asSyntheticUser(), http.MethodPost, "/api/v1/workflow/steps", + moveToStepBody("wf-b", "Crosser", h.stepA)) + requireStatus(t, rec, http.StatusBadRequest) + }) + + t.Run("a target in the same workflow is accepted", func(t *testing.T) { + h := setupScopedRouter(t) + rec := doAs(t, h, asUser(userB), http.MethodPost, "/api/v1/workflow/steps", + moveToStepBody("wf-b", "Neighbour", h.stepB)) + requireStatus(t, rec, http.StatusCreated) + }) +} + +// TestQueueRunTaskTargetIsAuthorized covers the other embedded ID: a +// `queue_run` action naming a literal task starts agent work on that task when +// the trigger fires, so an unchecked one is a write into another user's task. +func TestQueueRunTaskTargetIsAuthorized(t *testing.T) { + t.Run("a foreign task is refused", func(t *testing.T) { + h := setupScopedRouter(t) + h.queries.reset() + rec := doAs(t, h, asUser(userB), http.MethodPost, "/api/v1/workflow/steps", + queueRunBody("wf-b", "Queuer", "task-a")) + requireNotFound(t, rec) + requireNoStepWrites(t, h) + if _, _, tasks := h.owner.calls(); len(tasks) == 0 { + t.Fatal("the task-domain check was never consulted") + } + }) + + t.Run("the caller's own task is accepted", func(t *testing.T) { + h := setupScopedRouter(t) + rec := doAs(t, h, asUser(userB), http.MethodPost, "/api/v1/workflow/steps", + queueRunBody("wf-b", "Queuer", "task-b")) + requireStatus(t, rec, http.StatusCreated) + }) + + // "this" and an absent task_id both mean "the task the trigger fired on", + // which is the shape every built-in template uses. Neither names a task, + // so neither may be sent to the task service (an empty ID there means "no + // scoping applies"). + t.Run("the this sentinel names no task", func(t *testing.T) { + h := setupScopedRouter(t) + h.owner.resetCalls() + requireStatus(t, doAs(t, h, asUser(userB), http.MethodPost, "/api/v1/workflow/steps", + queueRunBody("wf-b", "Sentinel", "this")), http.StatusCreated) + requireStatus(t, doAs(t, h, asUser(userB), http.MethodPost, "/api/v1/workflow/steps", + queueRunBody("wf-b", "Empty", "")), http.StatusCreated) + if _, _, tasks := h.owner.calls(); len(tasks) != 0 { + t.Fatalf("sentinel task ids were sent to the task service: %v", tasks) + } + }) +} + +// TestReorderRequiresStepsToBelongToTheWorkflow closes the gap between the ID +// that is authorized (the workflow in the URL) and the IDs that are written +// (the step list in the body). Naming a step of a different workflow moved +// that step's position, and pointing the URL at a mutable workflow while +// naming a read-only workflow's steps walked straight past the read-only +// guard as well. +func TestReorderRequiresStepsToBelongToTheWorkflow(t *testing.T) { + t.Run("a step from the caller's other workflow is refused", func(t *testing.T) { + h := setupScopedRouter(t) + h.queries.reset() + rec := doAs(t, h, asUser(userB), http.MethodPut, "/api/v1/workflows/wf-b/workflow/steps/reorder", + map[string]any{"step_ids": []string{h.stepB, h.stepB2}}) + requireNotFound(t, rec) + requireNoStepWrites(t, h) + }) + + t.Run("the rule holds with auth disabled", func(t *testing.T) { + h := setupScopedRouter(t) + h.queries.reset() + rec := doAs(t, h, asSyntheticUser(), http.MethodPut, "/api/v1/workflows/wf-b/workflow/steps/reorder", + map[string]any{"step_ids": []string{h.stepB2}}) + requireNotFound(t, rec) + requireNoStepWrites(t, h) + }) + + t.Run("the workflow's own steps still reorder", func(t *testing.T) { + h := setupScopedRouter(t) + second := doAs(t, h, asUser(userB), http.MethodPost, "/api/v1/workflow/steps", + map[string]any{"workflow_id": "wf-b", "name": "Second", "position": 1}) + requireStatus(t, second, http.StatusCreated) + var created map[string]any + decodeJSON(t, second, &created) + secondID, _ := created["id"].(string) + + rec := doAs(t, h, asUser(userB), http.MethodPut, "/api/v1/workflows/wf-b/workflow/steps/reorder", + map[string]any{"step_ids": []string{secondID, h.stepB}}) + requireStatus(t, rec, http.StatusOK) + steps, err := h.repo.ListStepsByWorkflow(context.Background(), "wf-b") + if err != nil { + t.Fatalf("list steps: %v", err) + } + if len(steps) != 2 || steps[0].ID != secondID || steps[1].ID != h.stepB { + t.Fatalf("stored order = %#v, want the reversed order", steps) + } + }) +} + +// TestReadOnlyWorkflowDoesNotLeakToAForeignCaller pins the order of the two +// guards. EnsureWorkflowMutable answers 409 with a message naming *why* the +// workflow is read-only, so running it before the ownership check would tell +// one user that another user's workflow exists and is GitHub-synced. +func TestReadOnlyWorkflowDoesNotLeakToAForeignCaller(t *testing.T) { + h := setupScopedRouter(t) + h.service.SetWorkflowProvider(&fakeWorkflowProvider{workflows: []*taskmodels.Workflow{ + {ID: "wf-a", WorkspaceID: "ws-a", Name: "A Flow", Source: taskmodels.WorkflowSourceGitHub}, + }}) + + for _, tc := range []struct { + name string + method string + path string + body any + }{ + {"create", http.MethodPost, "/api/v1/workflow/steps", + map[string]any{"workflow_id": "wf-a", "name": "Nope", "position": 1}}, + {"template", http.MethodPost, "/api/v1/workflows/wf-a/workflow/steps", + map[string]any{"template_id": "template-1"}}, + {"reorder", http.MethodPut, "/api/v1/workflows/wf-a/workflow/steps/reorder", + map[string]any{"step_ids": []string{h.stepA}}}, + {"update", http.MethodPut, "/api/v1/workflow/steps/" + h.stepA, map[string]any{"name": "Nope"}}, + {"delete", http.MethodDelete, "/api/v1/workflow/steps/" + h.stepA, nil}, + } { + t.Run(tc.name, func(t *testing.T) { + rec := doAs(t, h, asUser(userB), tc.method, tc.path, tc.body) + requireNotFound(t, rec) + if strings.Contains(rec.Body.String(), "GitHub") { + t.Fatalf("read-only reason leaked to a foreign caller: %s", rec.Body.String()) + } + }) + } +} + +// TestImportCannotSmuggleAForeignTaskReference covers the payload half of the +// same defect. +// +// Transition targets are already constrained on this path: an export names +// them by position, WorkflowExport.Validate rejects a raw `step_id` under +// on_turn_start/on_turn_complete, and the importer maps each position onto a +// step it just created. A queue_run task target has no positional form, and an +// on_enter action survives the conversion verbatim, so a hand-written document +// can name any task in the install. +func TestImportCannotSmuggleAForeignTaskReference(t *testing.T) { + const queueRunYAML = `version: 1 +type: kandev_workflow +workflows: + - name: Smuggled + steps: + - name: Backlog + position: 0 + color: "#111111" + events: + on_enter: + - type: queue_run + config: + task_id: "%s" +` + const positionalYAML = `version: 1 +type: kandev_workflow +workflows: + - name: Positional + steps: + - name: Backlog + position: 0 + color: "#111111" + events: + on_turn_complete: + - type: move_to_step + config: + step_position: 1 + - name: Doing + position: 1 + color: "#222222" +` + + t.Run("another user's task is refused and nothing is written", func(t *testing.T) { + h := setupScopedRouter(t) + h.queries.reset() + rec := doRawAs(t, h, asUser(userB), http.MethodPost, "/api/v1/workspaces/ws-b/workflows/import", + fmt.Sprintf(queueRunYAML, "task-a")) + if rec.Code == http.StatusOK { + t.Fatalf("import accepted a foreign task reference: %s", rec.Body.String()) + } + requireNoStepWrites(t, h) + if len(h.provider.created) != 0 { + t.Fatalf("refused import created %v", h.provider.created) + } + }) + + t.Run("the caller's own task still imports", func(t *testing.T) { + h := setupScopedRouter(t) + rec := doRawAs(t, h, asUser(userB), http.MethodPost, "/api/v1/workspaces/ws-b/workflows/import", + fmt.Sprintf(queueRunYAML, "task-b")) + requireStatus(t, rec, http.StatusOK) + steps, err := h.repo.ListStepsByWorkflow(context.Background(), "created-Smuggled") + if err != nil { + t.Fatalf("list steps: %v", err) + } + if len(steps) != 1 || len(steps[0].Events.OnEnter) != 1 { + t.Fatalf("imported step = %#v, want the on_enter action preserved", steps) + } + }) + + t.Run("position-based transition targets still import", func(t *testing.T) { + h := setupScopedRouter(t) + rec := doRawAs(t, h, asUser(userB), http.MethodPost, "/api/v1/workspaces/ws-b/workflows/import", positionalYAML) + requireStatus(t, rec, http.StatusOK) + steps, err := h.repo.ListStepsByWorkflow(context.Background(), "created-Positional") + if err != nil { + t.Fatalf("list steps: %v", err) + } + if len(steps) != 2 { + t.Fatalf("steps = %#v, want both imported", steps) + } + target := "" + for _, action := range steps[0].Events.OnTurnComplete { + if id, ok := action.Config["step_id"].(string); ok { + target = id + } + } + if target != steps[1].ID { + t.Fatalf("remapped target = %q, want the second imported step %q", target, steps[1].ID) + } + }) +} diff --git a/apps/backend/internal/workflow/handlers/workflow_scope_test.go b/apps/backend/internal/workflow/handlers/workflow_scope_test.go new file mode 100644 index 0000000000..2d82ac9d4d --- /dev/null +++ b/apps/backend/internal/workflow/handlers/workflow_scope_test.go @@ -0,0 +1,364 @@ +package handlers + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "strings" + "testing" + + "go.uber.org/zap" + "go.uber.org/zap/zapcore" + "go.uber.org/zap/zaptest/observer" + + "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/workflow/controller" + ws "github.com/kandev/kandev/pkg/websocket" +) + +// importYAML is a minimal valid export document, used to prove an import into +// somebody else's workspace writes nothing. +const importYAML = `version: 1 +type: kandev_workflow +workflows: + - name: Smuggled + description: Imported by the wrong user + steps: + - name: Backlog + position: 0 + color: "#111111" +` + +// TestWorkflowKeyedRoutesDenyForeignOwner covers every route that names a +// workflow or a workspace by ID. User B is authenticated and guesses user A's +// IDs; each route must answer 404 and must not reach the workflow_steps table +// at all — a guard that runs after the read has already leaked (list/export) +// or already written (create/reorder/import). +func TestWorkflowKeyedRoutesDenyForeignOwner(t *testing.T) { + cases := []struct { + name string + method string + path string + body any + }{ + {"list steps by workflow", http.MethodGet, "/api/v1/workflows/wf-a/workflow/steps", nil}, + {"list steps by workspace", http.MethodGet, "/api/v1/workspaces/ws-a/workflow-steps", nil}, + {"export workflow", http.MethodGet, "/api/v1/workflows/wf-a/export", nil}, + {"export workspace workflows", http.MethodGet, "/api/v1/workspaces/ws-a/workflows/export", nil}, + { + "create step", http.MethodPost, "/api/v1/workflow/steps", + map[string]any{"workflow_id": "wf-a", "name": "Smuggled", "position": 9}, + }, + { + "create steps from template", http.MethodPost, "/api/v1/workflows/wf-a/workflow/steps", + map[string]any{"template_id": "template-1"}, + }, + { + "reorder steps", http.MethodPut, "/api/v1/workflows/wf-a/workflow/steps/reorder", + map[string]any{"step_ids": []string{"unknown"}}, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + h := setupScopedRouter(t) + seedTemplate(t, h.workflowHarness, "template-1", "Seeded Flow") + h.queries.reset() + + rec := doAs(t, h, asUser(userB), tc.method, tc.path, tc.body) + requireNotFound(t, rec) + requireStepTableUntouched(t, h) + if got := stepNames(t, h, "wf-a"); len(got) != 1 || got[0] != "A Backlog" { + t.Fatalf("workflow steps = %v, want the untouched seed", got) + } + }) + } + + t.Run("import into a foreign workspace", func(t *testing.T) { + h := setupScopedRouter(t) + h.queries.reset() + + rec := doRawAs(t, h, asUser(userB), http.MethodPost, "/api/v1/workspaces/ws-a/workflows/import", importYAML) + requireNotFound(t, rec) + requireStepTableUntouched(t, h) + if len(h.provider.created) != 0 { + t.Fatalf("denied import created %v", h.provider.created) + } + }) +} + +// TestStepKeyedRoutesDenyForeignOwner covers the routes keyed by step ID. The +// step's owning workflow has to be resolved before it can be authorized, so +// the read of that one row is expected; what must not happen is a write, a +// disclosure of the step, or a response that distinguishes "not yours" from +// "does not exist". +func TestStepKeyedRoutesDenyForeignOwner(t *testing.T) { + t.Run("get", func(t *testing.T) { + h := setupScopedRouter(t) + foreign := doAs(t, h, asUser(userB), http.MethodGet, "/api/v1/workflow/steps/"+h.stepA, nil) + missing := doAs(t, h, asUser(userB), http.MethodGet, "/api/v1/workflow/steps/no-such-step", nil) + + requireNotFound(t, foreign) + if foreign.Code != missing.Code || foreign.Body.String() != missing.Body.String() { + t.Fatalf("foreign step %d %s differs from missing step %d %s", + foreign.Code, foreign.Body.String(), missing.Code, missing.Body.String()) + } + if strings.Contains(foreign.Body.String(), "A Backlog") { + t.Fatalf("denied get leaked the step: %s", foreign.Body.String()) + } + }) + + t.Run("update", func(t *testing.T) { + h := setupScopedRouter(t) + h.queries.reset() + rec := doAs(t, h, asUser(userB), http.MethodPut, "/api/v1/workflow/steps/"+h.stepA, + map[string]any{"name": "Hijacked"}) + requireNotFound(t, rec) + requireNoStepWrites(t, h) + if got := stepNames(t, h, "wf-a"); len(got) != 1 || got[0] != "A Backlog" { + t.Fatalf("workflow steps = %v, want the untouched seed", got) + } + }) + + t.Run("delete", func(t *testing.T) { + h := setupScopedRouter(t) + h.queries.reset() + rec := doAs(t, h, asUser(userB), http.MethodDelete, "/api/v1/workflow/steps/"+h.stepA, nil) + requireNotFound(t, rec) + requireNoStepWrites(t, h) + if got := stepNames(t, h, "wf-a"); len(got) != 1 { + t.Fatalf("workflow steps = %v, want the step to survive", got) + } + }) + + t.Run("delete of a missing step answers exactly like a foreign one", func(t *testing.T) { + h := setupScopedRouter(t) + foreign := doAs(t, h, asUser(userB), http.MethodDelete, "/api/v1/workflow/steps/"+h.stepA, nil) + missing := doAs(t, h, asUser(userB), http.MethodDelete, "/api/v1/workflow/steps/no-such-step", nil) + if foreign.Code != missing.Code || foreign.Body.String() != missing.Body.String() { + t.Fatalf("foreign delete %d %s differs from missing delete %d %s", + foreign.Code, foreign.Body.String(), missing.Code, missing.Body.String()) + } + }) + + t.Run("reorder cannot name a foreign step from an owned workflow", func(t *testing.T) { + h := setupScopedRouter(t) + h.queries.reset() + rec := doAs(t, h, asUser(userB), http.MethodPut, "/api/v1/workflows/wf-b/workflow/steps/reorder", + map[string]any{"step_ids": []string{h.stepA}}) + requireNotFound(t, rec) + requireNoStepWrites(t, h) + }) + + // pull_from_step_id names a second step, and the validator used to answer + // "must reference a step in the same workflow" for one that exists and + // "invalid: workflow step not found" for one that does not — a working + // existence oracle over every step in the install, reachable from an + // ordinary edit of the caller's own step. + t.Run("a foreign pull source answers exactly like a nonexistent one", func(t *testing.T) { + h := setupScopedRouter(t) + h.queries.reset() + foreign := doAs(t, h, asUser(userB), http.MethodPut, "/api/v1/workflow/steps/"+h.stepB, + map[string]any{"pull_from_step_id": h.stepA}) + missing := doAs(t, h, asUser(userB), http.MethodPut, "/api/v1/workflow/steps/"+h.stepB, + map[string]any{"pull_from_step_id": "no-such-step"}) + requireNotFound(t, foreign) + if foreign.Code != missing.Code || foreign.Body.String() != missing.Body.String() { + t.Fatalf("foreign pull source %d %s differs from missing one %d %s", + foreign.Code, foreign.Body.String(), missing.Code, missing.Body.String()) + } + if strings.Contains(foreign.Body.String(), "same workflow") { + t.Fatalf("denied pull source disclosed that the step exists: %s", foreign.Body.String()) + } + requireNoStepWrites(t, h) + }) +} + +// TestWorkflowScopeWSActionsDenyForeignOwner is the half the gateway backstop +// cannot cover: these payloads name a workflow_id or a step id, neither of +// which dispatch_scope.go parses, so the WS surface is only as scoped as the +// handlers behind it. +func TestWorkflowScopeWSActionsDenyForeignOwner(t *testing.T) { + t.Run("workflow.step.list", func(t *testing.T) { + h := setupScopedRouter(t) + h.queries.reset() + msg := dispatchAs(t, h, asUser(userB), ws.ActionWorkflowStepList, map[string]any{"workflow_id": "wf-a"}) + requireWSError(t, msg, ws.ErrorCodeNotFound) + requireStepTableUntouched(t, h) + }) + + t.Run("workflow.step.get", func(t *testing.T) { + h := setupScopedRouter(t) + foreign := dispatchAs(t, h, asUser(userB), ws.ActionWorkflowStepGet, map[string]any{"id": h.stepA}) + missing := dispatchAs(t, h, asUser(userB), ws.ActionWorkflowStepGet, map[string]any{"id": "no-such-step"}) + requireWSError(t, foreign, ws.ErrorCodeNotFound) + if string(foreign.Payload) != string(missing.Payload) { + t.Fatalf("foreign step frame %s differs from missing step frame %s", foreign.Payload, missing.Payload) + } + }) + + t.Run("workflow.step.create", func(t *testing.T) { + h := setupScopedRouter(t) + seedTemplate(t, h.workflowHarness, "template-1", "Seeded Flow") + h.queries.reset() + msg := dispatchAs(t, h, asUser(userB), ws.ActionWorkflowStepCreate, + map[string]any{"workflow_id": "wf-a", "template_id": "template-1"}) + requireWSError(t, msg, ws.ErrorCodeNotFound) + requireStepTableUntouched(t, h) + }) +} + +// TestWorkflowScopeOwnerKeepsFullAccess is the other half of the guard: every +// route the previous tests deny must still work for the workspace's owner. +func TestWorkflowScopeOwnerKeepsFullAccess(t *testing.T) { + h := setupScopedRouter(t) + seedTemplate(t, h.workflowHarness, "template-1", "Seeded Flow") + ctx := asUser(userA) + + var listed controller.ListStepsResponse + byWorkflow := doAs(t, h, ctx, http.MethodGet, "/api/v1/workflows/wf-a/workflow/steps", nil) + requireStatus(t, byWorkflow, http.StatusOK) + if err := json.Unmarshal(byWorkflow.Body.Bytes(), &listed); err != nil { + t.Fatalf("decode list: %v", err) + } + if len(listed.Steps) != 1 || listed.Steps[0].ID != h.stepA { + t.Fatalf("steps = %#v, want the owner's step", listed.Steps) + } + + requireStatus(t, doAs(t, h, ctx, http.MethodGet, "/api/v1/workspaces/ws-a/workflow-steps", nil), http.StatusOK) + requireStatus(t, doAs(t, h, ctx, http.MethodGet, "/api/v1/workflow/steps/"+h.stepA, nil), http.StatusOK) + requireStatus(t, doAs(t, h, ctx, http.MethodGet, "/api/v1/workflows/wf-a/export", nil), http.StatusOK) + requireStatus(t, doAs(t, h, ctx, http.MethodGet, "/api/v1/workspaces/ws-a/workflows/export", nil), http.StatusOK) + + created := doAs(t, h, ctx, http.MethodPost, "/api/v1/workflow/steps", + map[string]any{"workflow_id": "wf-a", "name": "Doing", "position": 1}) + requireStatus(t, created, http.StatusCreated) + var step map[string]any + if err := json.Unmarshal(created.Body.Bytes(), &step); err != nil { + t.Fatalf("decode created step: %v", err) + } + newStepID, _ := step["id"].(string) + + requireStatus(t, doAs(t, h, ctx, http.MethodPut, "/api/v1/workflow/steps/"+newStepID, + map[string]any{"name": "In Progress"}), http.StatusOK) + requireStatus(t, doAs(t, h, ctx, http.MethodPut, "/api/v1/workflows/wf-a/workflow/steps/reorder", + map[string]any{"step_ids": []string{newStepID, h.stepA}}), http.StatusOK) + requireStatus(t, doAs(t, h, ctx, http.MethodPost, "/api/v1/workflows/wf-a/workflow/steps", + map[string]any{"template_id": "template-1"}), http.StatusCreated) + requireStatus(t, doAs(t, h, ctx, http.MethodDelete, "/api/v1/workflow/steps/"+newStepID, nil), http.StatusOK) + + requireStatus(t, doRawAs(t, h, ctx, http.MethodPost, + "/api/v1/workspaces/ws-a/workflows/import", importYAML), http.StatusOK) + if len(h.provider.created) != 1 || h.provider.created[0] != "Smuggled" { + t.Fatalf("owner import created %v, want the imported workflow", h.provider.created) + } + + var wsListed controller.ListStepsResponse + requireWSResponse(t, dispatchAs(t, h, ctx, ws.ActionWorkflowStepList, + map[string]any{"workflow_id": "wf-a"}), &wsListed) + if len(wsListed.Steps) == 0 { + t.Fatal("ws list returned no steps for the owner") + } + var wsStep controller.GetStepResponse + requireWSResponse(t, dispatchAs(t, h, ctx, ws.ActionWorkflowStepGet, map[string]any{"id": h.stepA}), &wsStep) + if wsStep.Step == nil || wsStep.Step.ID != h.stepA { + t.Fatalf("ws get = %#v, want the owner's step", wsStep.Step) + } +} + +// TestWorkflowScopeNoOpsWithAuthDisabled pins the single-user contract: the +// synthetic identity injected when authentication is off must reach every +// workspace exactly as it did before this guard existed, and must not even +// consult the task domain. +func TestWorkflowScopeNoOpsWithAuthDisabled(t *testing.T) { + for _, tc := range []struct { + name string + ctx context.Context + }{ + {"synthetic identity", asSyntheticUser()}, + {"no identity at all", context.Background()}, + } { + t.Run(tc.name, func(t *testing.T) { + h := setupScopedRouter(t) + seedTemplate(t, h.workflowHarness, "template-1", "Seeded Flow") + h.owner.resetCalls() + + // ws-b/wf-b belong to user-b; unscoped callers see everything. + requireStatus(t, doAs(t, h, tc.ctx, http.MethodGet, "/api/v1/workflows/wf-b/workflow/steps", nil), http.StatusOK) + requireStatus(t, doAs(t, h, tc.ctx, http.MethodGet, "/api/v1/workspaces/ws-b/workflow-steps", nil), http.StatusOK) + requireStatus(t, doAs(t, h, tc.ctx, http.MethodGet, "/api/v1/workflow/steps/"+h.stepB, nil), http.StatusOK) + requireStatus(t, doAs(t, h, tc.ctx, http.MethodGet, "/api/v1/workflows/wf-b/export", nil), http.StatusOK) + requireStatus(t, doAs(t, h, tc.ctx, http.MethodGet, "/api/v1/workspaces/ws-b/workflows/export", nil), http.StatusOK) + requireStatus(t, doAs(t, h, tc.ctx, http.MethodPost, "/api/v1/workflow/steps", + map[string]any{"workflow_id": "wf-b", "name": "Doing", "position": 1}), http.StatusCreated) + requireStatus(t, doAs(t, h, tc.ctx, http.MethodPut, "/api/v1/workflow/steps/"+h.stepB, + map[string]any{"name": "Renamed"}), http.StatusOK) + requireStatus(t, doAs(t, h, tc.ctx, http.MethodDelete, "/api/v1/workflow/steps/"+h.stepB, nil), http.StatusOK) + requireStatus(t, doRawAs(t, h, tc.ctx, http.MethodPost, + "/api/v1/workspaces/ws-b/workflows/import", importYAML), http.StatusOK) + + var wsListed controller.ListStepsResponse + requireWSResponse(t, dispatchAs(t, h, tc.ctx, ws.ActionWorkflowStepList, + map[string]any{"workflow_id": "wf-b"}), &wsListed) + + // An unresolved-owner failure would be invisible above if the guard + // short-circuited on a nil checker, so assert it never called out. + if workflows, workspaces, tasks := h.owner.calls(); len(workflows) != 0 || len(workspaces) != 0 || len(tasks) != 0 { + t.Fatalf("unscoped caller consulted the task domain: workflows=%v workspaces=%v tasks=%v", + workflows, workspaces, tasks) + } + }) + } +} + +// TestWorkflowTemplatesStayUnscoped pins the deliberate exception: templates +// are install-global read-only definitions with no owner, so any authenticated +// user may read them. +func TestWorkflowTemplatesStayUnscoped(t *testing.T) { + h := setupScopedRouter(t) + seedTemplate(t, h.workflowHarness, "template-1", "Seeded Flow") + + requireStatus(t, doAs(t, h, asUser(userB), http.MethodGet, "/api/v1/workflow/templates", nil), http.StatusOK) + requireStatus(t, doAs(t, h, asUser(userB), http.MethodGet, "/api/v1/workflow/templates/template-1", nil), http.StatusOK) + + var listed controller.ListTemplatesResponse + requireWSResponse(t, dispatchAs(t, h, asUser(userB), ws.ActionWorkflowTemplateList, map[string]any{}), &listed) + if !containsTemplate(listed.Templates, "template-1") { + t.Fatalf("templates = %#v, want the seeded row visible to any user", listed.Templates) + } +} + +// TestDeniedRequestsDoNotLogAsFailures pins the difference between a rejection +// and a fault. The delete route pre-reads the step so it can publish the +// deleted event, and that read is now authorized too — so every unauthorized +// delete used to file a Warn saying the step could not be fetched, which reads +// as infrastructure trouble in an operator's logs. +func TestDeniedRequestsDoNotLogAsFailures(t *testing.T) { + core, logs := observer.New(zapcore.WarnLevel) + log, err := logger.NewFromZap(zap.New(core)) + if err != nil { + t.Fatalf("logger: %v", err) + } + h := setupScopedRouter(t, log) + + requireNotFound(t, doAs(t, h, asUser(userB), http.MethodDelete, "/api/v1/workflow/steps/"+h.stepA, nil)) + + if entries := logs.FilterLevelExact(zapcore.WarnLevel).All(); len(entries) != 0 { + t.Fatalf("denied delete logged %d warning(s): %+v", len(entries), entries) + } +} + +// TestWSGetStepSeparatesDeniedFromBroken is the WS twin of the HTTP history +// route's rule: an unreachable step is not-found, a failing lookup is not. +// wsGetByID answered NOT_FOUND for everything, and authorizing the step added +// a database read to that path, so an outage would have reported every step as +// missing. +func TestWSGetStepSeparatesDeniedFromBroken(t *testing.T) { + h := setupScopedRouter(t) + h.service.SetWorkflowAccessChecker(func(context.Context, string) error { + return errors.New("database is locked") + }) + + msg := dispatchAs(t, h, asUser(userB), ws.ActionWorkflowStepGet, map[string]any{"id": h.stepA}) + requireWSError(t, msg, ws.ErrorCodeInternalError) +} diff --git a/apps/backend/internal/workflow/handlers/ws_helpers.go b/apps/backend/internal/workflow/handlers/ws_helpers.go index c0e832eaf6..f33ab012fe 100644 --- a/apps/backend/internal/workflow/handlers/ws_helpers.go +++ b/apps/backend/internal/workflow/handlers/ws_helpers.go @@ -2,11 +2,18 @@ package handlers import ( "context" + "errors" + "github.com/kandev/kandev/internal/workflow/service" ws "github.com/kandev/kandev/pkg/websocket" "go.uber.org/zap" ) +// notFoundMessage is the single reply for anything the caller may not see and +// anything that does not exist: one wording, so the frame itself cannot be +// used to tell the two apart. +const notFoundMessage = "Not found" + func (h *Handlers) wsGetByID( ctx context.Context, msg *ws.Message, logErrMsg, clientErrMsg string, @@ -24,7 +31,14 @@ func (h *Handlers) wsGetByID( resp, err := fn(ctx, req.ID) if err != nil { h.logger.Error(logErrMsg, zap.Error(err)) - return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeNotFound, clientErrMsg, nil) + // A denial and a genuine miss share one reply. Anything else is a + // failure and must not be dressed up as a missing resource — the + // authorization step reads the repository, so a database error can + // reach here now. + if service.IsNotFound(err) { + return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeNotFound, clientErrMsg, nil) + } + return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeInternalError, clientErrMsg, nil) } return ws.NewResponse(msg.ID, msg.Action, resp) } @@ -40,6 +54,10 @@ func (h *Handlers) wsHandleStringField( resp, err := fn(ctx, fieldValue) if err != nil { h.logger.Error(logErrMsg, zap.Error(err)) + if errors.Is(err, service.ErrNotVisible) { + return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeNotFound, notFoundMessage, nil) + } + return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeInternalError, clientErrMsg, nil) } return ws.NewResponse(msg.ID, msg.Action, resp) diff --git a/apps/backend/internal/workflow/models/models.go b/apps/backend/internal/workflow/models/models.go index 1c2c384ecc..f41d71ec5b 100644 --- a/apps/backend/internal/workflow/models/models.go +++ b/apps/backend/internal/workflow/models/models.go @@ -2,6 +2,7 @@ package models import ( "maps" + "strings" "time" ) @@ -345,6 +346,83 @@ type SessionStepHistory struct { CreatedAt time.Time `json:"created_at"` } +// StepEventReferences are the IDs a step's event actions name, and that +// something else dereferences later: a move_to_step action names the step to +// transition to, a queue_run action names the task to queue work on. Both are +// caller-supplied through the step-write API and both are read by the engine, +// which runs on the event bus with no request identity of its own — so the +// write is the only place they can be checked against a caller. +// +// Every trigger that RemapStepEvents rewrites must also be collected here. +type StepEventReferences struct { + // StepIDs are move_to_step targets. + StepIDs []string + // TaskIDs are queue_run targets, excluding the "this" sentinel and the + // empty default, which both mean "the task the trigger fired on". + TaskIDs []string +} + +// CollectStepEventReferences gathers every step and task ID named by a step's +// event actions. +func CollectStepEventReferences(events StepEvents) StepEventReferences { + var refs StepEventReferences + for _, a := range events.OnEnter { + if a.Type == OnEnterQueueRun { + refs.addTask(a.Config) + } + } + for _, a := range events.OnTurnStart { + if a.Type == OnTurnStartMoveToStep { + refs.addStep(a.Config) + } + } + for _, a := range events.OnTurnComplete { + if a.Type == OnTurnCompleteMoveToStep { + refs.addStep(a.Config) + } + } + for _, actions := range [][]GenericAction{ + events.OnComment, events.OnBlockerResolved, events.OnChildrenCompleted, + events.OnApprovalResolved, events.OnHeartbeat, events.OnBudgetAlert, events.OnAgentError, + } { + for _, a := range actions { + switch a.Type { + case GenericActionMoveToStep: + refs.addStep(a.Config) + case GenericActionQueueRun: + refs.addTask(a.Config) + } + } + } + return refs +} + +func (r *StepEventReferences) addStep(config map[string]any) { + if id := configID(config, "step_id"); id != "" { + r.StepIDs = append(r.StepIDs, id) + } +} + +// addTask skips the sentinels the engine resolves to the current task +// (internal/workflow/engine.resolveTaskID), which name no task at all. +func (r *StepEventReferences) addTask(config map[string]any) { + if id := configID(config, "task_id"); id != "" && id != QueueRunTaskIDThis { + r.TaskIDs = append(r.TaskIDs, id) + } +} + +// QueueRunTaskIDThis mirrors engine.TaskIDThis. The constant is repeated +// rather than imported because the engine imports this package. +const QueueRunTaskIDThis = "this" + +func configID(config map[string]any, key string) string { + if config == nil { + return "" + } + value, _ := config[key].(string) + return strings.TrimSpace(value) +} + // RemapStepEvents returns a copy of events with all step_id references // in move_to_step actions replaced using the provided ID mapping. func RemapStepEvents(events StepEvents, idMap map[string]string) StepEvents { diff --git a/apps/backend/internal/workflow/service/access.go b/apps/backend/internal/workflow/service/access.go new file mode 100644 index 0000000000..561bca1cc5 --- /dev/null +++ b/apps/backend/internal/workflow/service/access.go @@ -0,0 +1,201 @@ +package service + +import ( + "context" + "errors" + "fmt" + "strings" + + "github.com/kandev/kandev/internal/auth/authn" + "github.com/kandev/kandev/internal/task/repository/repoerrors" +) + +// Per-user scoping for the workflow-step surface (opt-in authentication). +// +// Workflows, workspaces and steps are all owned by the task domain — a +// workflow belongs to a workspace, and a workspace belongs to a user — so the +// permission itself is not this package's to decide. The checkers below are +// the same seam SetSessionAccessChecker already uses for step history: the +// task service's authorize* helpers are wired in at startup +// (internal/backendapp/services.go) and this package only decides *which* +// resource each entry point has to authorize. +// +// Scoping keys off the request-context identity exactly as the task service +// does: no identity means an internal caller (event bus, orchestrator, +// pollers), and a synthetic identity means authentication is disabled. Both +// are unscoped, so with auth off nothing here runs at all. +// +// Denials are reported as ErrNotVisible, which is also what a genuinely +// missing workflow, workspace or step produces. That is deliberate: a +// resource belonging to somebody else must be indistinguishable from one that +// does not exist, or the 404/500 split becomes an existence oracle. +// +// Workflow *templates* are exempt: they are install-global, read-only +// definitions with no owner (see the embedded YAML in config/workflows), so +// every authenticated user may read them. + +// ErrNotVisible reports that the caller may not see the requested workflow, +// workspace or step — or that it does not exist. The message carries +// "not found" because the handler layer classifies mutation errors by that +// substring (see writeStepMutationError). +var ErrNotVisible = errors.New("workflow resource not found") + +// An unwired checker means the workflow service was built without the task +// domain (unit tests, standalone tools) and leaves the corresponding check +// open, the same shape SetSessionAccessChecker has always had. Production +// always wires all four, which backendapp's TestWorkflowAccessCheckersAreWired +// pins. The fail-closed branches above it still run either way: an empty ID is +// refused before the checker is consulted, wired or not. + +// SetWorkflowAccessChecker wires the task-domain check for a workflow ID. +// Production passes taskservice.Service.AuthorizeWorkflowAccess. +func (s *Service) SetWorkflowAccessChecker(checker func(context.Context, string) error) { + s.workflowAccessChecker = checker +} + +// SetWorkspaceAccessChecker wires the task-domain check for a workspace ID. +// Production passes taskservice.Service.AuthorizeWorkspaceAccess. +func (s *Service) SetWorkspaceAccessChecker(checker func(context.Context, string) error) { + s.workspaceAccessChecker = checker +} + +// SetTaskAccessChecker wires the task-domain check for a task ID. A step's +// queue_run action can name a task to start work on, so the step-write API +// accepts task IDs too. Production passes +// taskservice.Service.AuthorizeTaskAccess. +func (s *Service) SetTaskAccessChecker(checker func(context.Context, string) error) { + s.taskAccessChecker = checker +} + +// callerIsScoped mirrors internal/task/service's callerScope: false means the +// caller is an internal one or authentication is disabled, and no scoping +// applies. The workflow package repeats the three lines rather than importing +// the task service, which would be a dependency cycle. +func callerIsScoped(ctx context.Context) bool { + identity, ok := authn.IdentityFromContext(ctx) + return ok && !identity.Synthetic +} + +// AuthorizeWorkflow checks that the caller may see a workflow. +// +// An empty workflow ID fails closed: every route reaching this helper names a +// workflow, so an empty one means the owner could not be resolved, and +// handing "" to the task-domain checker would be read as "no scoping applies" +// (the trap runSubscriptionCheck documents in backendapp/auth.go). +func (s *Service) AuthorizeWorkflow(ctx context.Context, workflowID string) error { + if !callerIsScoped(ctx) { + return nil + } + if workflowID == "" { + return ErrNotVisible + } + if s.workflowAccessChecker == nil { + return nil + } + return normalizeAccessError(s.workflowAccessChecker(ctx, workflowID)) +} + +// AuthorizeWorkspace checks that the caller may see a workspace. An empty +// workspace ID fails closed for the same reason as AuthorizeWorkflow. +func (s *Service) AuthorizeWorkspace(ctx context.Context, workspaceID string) error { + if !callerIsScoped(ctx) { + return nil + } + if workspaceID == "" { + return ErrNotVisible + } + if s.workspaceAccessChecker == nil { + return nil + } + return normalizeAccessError(s.workspaceAccessChecker(ctx, workspaceID)) +} + +// AuthorizeTask checks that the caller may see a task named inside a step's +// event actions. An empty ID fails closed for the same reason as +// AuthorizeWorkflow; callers strip the "this" sentinel before calling. +func (s *Service) AuthorizeTask(ctx context.Context, taskID string) error { + if !callerIsScoped(ctx) { + return nil + } + if taskID == "" { + return ErrNotVisible + } + if s.taskAccessChecker == nil { + return nil + } + return normalizeAccessError(s.taskAccessChecker(ctx, taskID)) +} + +// AuthorizeStep checks that the caller may see a workflow step, resolving the +// step's owning workflow (and through it, its workspace) first. A step whose +// workflow cannot be resolved fails closed. +// +// Reading the step row before deciding is unavoidable — the step ID is the +// only thing the caller supplied — but nothing is disclosed: every rejection, +// including a step that does not exist at all, returns ErrNotVisible. +func (s *Service) AuthorizeStep(ctx context.Context, stepID string) error { + if !callerIsScoped(ctx) || s.workflowAccessChecker == nil { + return nil + } + step, err := s.repo.GetStep(ctx, stepID) + if err != nil { + if isNotFoundError(err) { + return ErrNotVisible + } + return err + } + if step == nil { + // The repository reports a miss as an error rather than a nil row; this + // only keeps the dereference below safe. + return ErrNotVisible + } + // A step whose workflow_id is empty has no resolvable owner and lands in + // AuthorizeWorkflow's fail-closed branch. + if err := s.AuthorizeWorkflow(ctx, step.WorkflowID); err != nil { + if errors.Is(err, ErrNotVisible) { + return ErrNotVisible + } + return err + } + return nil +} + +// IsNotFound reports whether err is the one answer this package gives for +// both a resource the caller may not see and one that does not exist. Handler +// layers use it to pick a not-found reply without having to re-derive the +// classification (and without accidentally reporting a genuine failure as a +// miss, which would hide an outage behind an empty board). +func IsNotFound(err error) bool { + return isNotFoundError(err) +} + +// normalizeAccessError collapses a denial into ErrNotVisible and lets a +// genuine failure (a database error, say) through as a server error. +func normalizeAccessError(err error) error { + if err == nil { + return nil + } + if isNotFoundError(err) { + return fmt.Errorf("%w", ErrNotVisible) + } + return err +} + +// isNotFoundError recognizes both the task domain's sentinels and its +// formatted misses. The task repository reports a missing workflow or +// workspace as fmt.Errorf("workflow not found: %s", id) rather than a +// sentinel, and the task service returns those verbatim from its authorize* +// helpers, so a sentinel-only test would classify "this workflow does not +// exist" as a server error while "this workflow is not yours" answered 404 — +// which is the existence leak this guard exists to close. +func isNotFoundError(err error) bool { + if err == nil { + return false + } + if errors.Is(err, ErrNotVisible) || + errors.Is(err, repoerrors.ErrWorkspaceNotFound) || + errors.Is(err, repoerrors.ErrTaskNotFound) { + return true + } + return strings.Contains(strings.ToLower(err.Error()), "not found") +} diff --git a/apps/backend/internal/workflow/service/access_test.go b/apps/backend/internal/workflow/service/access_test.go new file mode 100644 index 0000000000..40bcf0c18b --- /dev/null +++ b/apps/backend/internal/workflow/service/access_test.go @@ -0,0 +1,252 @@ +package service + +import ( + "context" + "errors" + "fmt" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/kandev/kandev/internal/auth/authn" + taskmodels "github.com/kandev/kandev/internal/task/models" + "github.com/kandev/kandev/internal/task/repository/repoerrors" + "github.com/kandev/kandev/internal/workflow/models" +) + +// scopedCtx is a request context carrying a real (non-synthetic) identity. +func scopedCtx() context.Context { + return authn.WithIdentity(context.Background(), authn.Identity{UserID: "user-a", Role: authn.RoleMember}) +} + +// syntheticCtx is what the auth middleware injects while authentication is +// disabled. +func syntheticCtx() context.Context { + return authn.WithIdentity(context.Background(), authn.Identity{UserID: "user-a", Synthetic: true}) +} + +// denyAll stands in for a task service that answers "not yours" for everything. +func denyAll(context.Context, string) error { return repoerrors.ErrWorkspaceNotFound } + +// callRecorder records the IDs a checker was asked about. +type callRecorder struct { + ids []string + err error +} + +func (r *callRecorder) check(_ context.Context, id string) error { + r.ids = append(r.ids, id) + return r.err +} + +func TestAuthorizeWorkflowAndWorkspaceFailClosedOnAnEmptyID(t *testing.T) { + svc, _ := setupTestService(t) + recorder := &callRecorder{} + svc.SetWorkflowAccessChecker(recorder.check) + svc.SetWorkspaceAccessChecker(recorder.check) + svc.SetTaskAccessChecker(recorder.check) + + // An empty ID means the owner could not be resolved. The task service + // reads workspaceID=="" as "no scoping applies" and would allow it, which + // is why this never reaches the checker (see backendapp/auth.go's + // runSubscriptionCheck for the same trap). + require.ErrorIs(t, svc.AuthorizeWorkflow(scopedCtx(), ""), ErrNotVisible) + require.ErrorIs(t, svc.AuthorizeWorkspace(scopedCtx(), ""), ErrNotVisible) + require.ErrorIs(t, svc.AuthorizeTask(scopedCtx(), ""), ErrNotVisible) + require.Empty(t, recorder.ids, "an unresolvable owner was passed to the task service") +} + +func TestAuthorizeHelpersNoOpWhenAuthIsDisabled(t *testing.T) { + svc, _ := setupTestService(t) + recorder := &callRecorder{err: repoerrors.ErrWorkspaceNotFound} + svc.SetWorkflowAccessChecker(recorder.check) + svc.SetWorkspaceAccessChecker(recorder.check) + svc.SetTaskAccessChecker(recorder.check) + + for name, ctx := range map[string]context.Context{ + "synthetic identity": syntheticCtx(), + "no identity": context.Background(), + } { + t.Run(name, func(t *testing.T) { + require.NoError(t, svc.AuthorizeWorkflow(ctx, "wf-1")) + require.NoError(t, svc.AuthorizeWorkspace(ctx, "ws-1")) + require.NoError(t, svc.AuthorizeStep(ctx, "step-1")) + require.NoError(t, svc.AuthorizeTask(ctx, "task-1")) + require.NoError(t, svc.AuthorizeWorkflow(ctx, ""), "an empty ID must not fail closed for an unscoped caller") + }) + } + require.Empty(t, recorder.ids, "unscoped callers must not reach the task service at all") +} + +func TestAuthorizeStepResolvesItsOwningWorkflow(t *testing.T) { + t.Run("a step in a visible workflow is allowed", func(t *testing.T) { + svc, _ := setupTestService(t) + recorder := &callRecorder{} + svc.SetWorkflowAccessChecker(recorder.check) + step := &models.WorkflowStep{WorkflowID: "wf-1", Name: "Backlog", Color: "#111111"} + require.NoError(t, svc.CreateStep(context.Background(), step)) + + require.NoError(t, svc.AuthorizeStep(scopedCtx(), step.ID)) + require.Equal(t, []string{"wf-1"}, recorder.ids) + }) + + t.Run("a step in a foreign workflow is not found", func(t *testing.T) { + svc, _ := setupTestService(t) + svc.SetWorkflowAccessChecker(denyAll) + step := &models.WorkflowStep{WorkflowID: "wf-1", Name: "Backlog", Color: "#111111"} + require.NoError(t, svc.CreateStep(context.Background(), step)) + + require.ErrorIs(t, svc.AuthorizeStep(scopedCtx(), step.ID), ErrNotVisible) + }) + + // A step row with no workflow has no resolvable owner. Failing open here + // would hand every scoped caller a step nobody can be shown to own, so the + // branch is pinned with a row inserted straight into the table (the model + // validator rejects one through the service). + t.Run("a step with no owning workflow fails closed", func(t *testing.T) { + svc, db := setupTestService(t) + svc.SetWorkflowAccessChecker(func(context.Context, string) error { return nil }) + _, err := db.Exec( + `INSERT INTO workflow_steps (id, workflow_id, name, position, wip_limit, created_at, updated_at) + VALUES ('orphan-step', '', 'Orphan', 0, 0, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)`) + require.NoError(t, err) + + require.ErrorIs(t, svc.AuthorizeStep(scopedCtx(), "orphan-step"), ErrNotVisible) + }) + + t.Run("a nonexistent step is the same error", func(t *testing.T) { + svc, _ := setupTestService(t) + svc.SetWorkflowAccessChecker(denyAll) + require.ErrorIs(t, svc.AuthorizeStep(scopedCtx(), "no-such-step"), ErrNotVisible) + }) +} + +// TestAuthorizeWorkflowNormalizesAMissingWorkflow pins the classification the +// no-existence-leak rule depends on. The task repository reports a missing +// workflow as a formatted error rather than a sentinel, so treating only +// sentinels as denials would answer 500 for "does not exist" and 404 for "not +// yours" — telling the two apart. +func TestAuthorizeWorkflowNormalizesAMissingWorkflow(t *testing.T) { + svc, _ := setupTestService(t) + svc.SetWorkflowAccessChecker(func(context.Context, string) error { + return fmt.Errorf("workflow not found: %s", "wf-1") + }) + require.ErrorIs(t, svc.AuthorizeWorkflow(scopedCtx(), "wf-1"), ErrNotVisible) +} + +// TestAuthorizeWorkflowPropagatesRealFailures is the other side of that coin: a +// database failure must not be laundered into a 404, which would hide an +// outage behind an empty board. +func TestAuthorizeWorkflowPropagatesRealFailures(t *testing.T) { + svc, _ := setupTestService(t) + broken := errors.New("database is locked") + svc.SetWorkflowAccessChecker(func(context.Context, string) error { return broken }) + + err := svc.AuthorizeWorkflow(scopedCtx(), "wf-1") + require.ErrorIs(t, err, broken) + require.NotErrorIs(t, err, ErrNotVisible) +} + +// TestServiceEntryPointsAuthorizeThemselves covers the methods reached without +// passing through the workflow controller: the boot-payload builder calls +// ListStepsByWorkspaceID directly, and the MCP config tools call +// ExportWorkflow/ImportWorkflows directly. +func TestServiceEntryPointsAuthorizeThemselves(t *testing.T) { + newDeniedService := func(t *testing.T) *Service { + t.Helper() + svc, _ := setupTestService(t) + svc.SetWorkflowAccessChecker(denyAll) + svc.SetWorkspaceAccessChecker(denyAll) + // A provider that would panic-free serve data if the guard let the call + // through, so a missing guard shows up as a nil error, not a crash. + svc.SetWorkflowProvider(&stubWorkflowProvider{}) + return svc + } + + t.Run("list steps by workspace", func(t *testing.T) { + svc := newDeniedService(t) + _, err := svc.ListStepsByWorkspaceID(scopedCtx(), "ws-a") + require.ErrorIs(t, err, ErrNotVisible) + }) + + t.Run("export one workflow", func(t *testing.T) { + svc := newDeniedService(t) + _, err := svc.ExportWorkflow(scopedCtx(), "wf-a") + require.ErrorIs(t, err, ErrNotVisible) + }) + + t.Run("export a workspace", func(t *testing.T) { + svc := newDeniedService(t) + _, err := svc.ExportWorkflows(scopedCtx(), "ws-a", nil) + require.ErrorIs(t, err, ErrNotVisible) + }) + + t.Run("import into a workspace", func(t *testing.T) { + svc := newDeniedService(t) + _, err := svc.ImportWorkflows(scopedCtx(), "ws-a", &models.WorkflowExport{ + Version: models.ExportVersion, + Type: models.ExportType, + Workflows: []models.WorkflowPortable{{Name: "Smuggled"}}, + }) + require.ErrorIs(t, err, ErrNotVisible) + require.Empty(t, svc.workflowProvider.(*stubWorkflowProvider).created) + }) + + // ReorderSteps is the layer that writes the positions, so it authorizes + // the workflow itself rather than trusting a caller to have done it. The + // membership check below cannot stand in for this one: steps that really + // do belong to a workflow the caller cannot see would otherwise reorder. + t.Run("reorder authorizes the workflow it is asked to reorder", func(t *testing.T) { + svc, _ := setupTestService(t) + step := &models.WorkflowStep{WorkflowID: "wf-a", Name: "Backlog", Color: "#111111", Position: 7} + require.NoError(t, svc.CreateStep(context.Background(), step)) + svc.SetWorkflowAccessChecker(denyAll) + + require.ErrorIs(t, svc.ReorderSteps(scopedCtx(), "wf-a", []string{step.ID}), ErrNotVisible) + stored, err := svc.GetStep(context.Background(), step.ID) + require.NoError(t, err) + require.Equal(t, 7, stored.Position, "a denied reorder wrote anyway") + }) + + t.Run("reorder rejects a step from another workflow", func(t *testing.T) { + svc, _ := setupTestService(t) + // Position 7, so the reorder this test denies would visibly move the + // step to 0 if the guard were missing. wf-b is authorized: the step + // belonging to wf-a is the only reason to refuse. + step := &models.WorkflowStep{WorkflowID: "wf-a", Name: "Backlog", Color: "#111111", Position: 7} + require.NoError(t, svc.CreateStep(context.Background(), step)) + svc.SetWorkflowAccessChecker(func(context.Context, string) error { return nil }) + + require.ErrorIs(t, svc.ReorderSteps(scopedCtx(), "wf-b", []string{step.ID}), ErrNotVisible) + + stored, err := svc.GetStep(context.Background(), step.ID) + require.NoError(t, err) + require.Equal(t, 7, stored.Position, "a denied reorder wrote anyway") + }) +} + +// stubWorkflowProvider is a permissive task-domain stand-in: every method +// succeeds, so any assertion that fails is the guard's absence rather than a +// missing fixture. +type stubWorkflowProvider struct { + created []string +} + +func (p *stubWorkflowProvider) ListWorkflows(context.Context, string, bool) ([]*taskmodels.Workflow, error) { + return nil, nil +} + +func (p *stubWorkflowProvider) GetWorkflow(_ context.Context, id string) (*taskmodels.Workflow, error) { + return &taskmodels.Workflow{ID: id, WorkspaceID: "ws-a", Name: "A Flow"}, nil +} + +func (p *stubWorkflowProvider) CreateWorkflow( + _ context.Context, workspaceID, name, description string, +) (*taskmodels.Workflow, error) { + p.created = append(p.created, name) + return &taskmodels.Workflow{ID: "created-" + name, WorkspaceID: workspaceID, Name: name, Description: description}, nil +} + +func (p *stubWorkflowProvider) UpdateWorkflow(context.Context, *taskmodels.Workflow) error { + return nil +} diff --git a/apps/backend/internal/workflow/service/service.go b/apps/backend/internal/workflow/service/service.go index 6d2498fd14..0ff7b10d37 100644 --- a/apps/backend/internal/workflow/service/service.go +++ b/apps/backend/internal/workflow/service/service.go @@ -35,12 +35,17 @@ type Service struct { matchProfile models.AgentProfileMatcher syncOps SyncWorkflowOps sessionAccessChecker func(context.Context, string) error - historyQueue chan historyWrite - historyStop chan struct{} - historyDone chan struct{} - historyCloseOnce sync.Once - historyMu sync.RWMutex - historyClosed bool + // workflowAccessChecker / workspaceAccessChecker carry the task domain's + // per-user visibility rule into this package (see access.go). + workflowAccessChecker func(context.Context, string) error + workspaceAccessChecker func(context.Context, string) error + taskAccessChecker func(context.Context, string) error + historyQueue chan historyWrite + historyStop chan struct{} + historyDone chan struct{} + historyCloseOnce sync.Once + historyMu sync.RWMutex + historyClosed bool } type historyWrite struct { @@ -240,6 +245,9 @@ func (s *Service) ListStepsByWorkflow(ctx context.Context, workflowID string) ([ // ListStepsByWorkspaceID returns all workflow steps for all workflows in a workspace. func (s *Service) ListStepsByWorkspaceID(ctx context.Context, workspaceID string) ([]*models.WorkflowStep, error) { + if err := s.AuthorizeWorkspace(ctx, workspaceID); err != nil { + return nil, err + } steps, err := s.repo.ListStepsByWorkspaceID(ctx, workspaceID) if err != nil { s.logger.Error("failed to list steps by workspace", zap.String("workspace_id", workspaceID), zap.Error(err)) @@ -548,15 +556,36 @@ func (s *Service) DeleteStep(ctx context.Context, stepID string) error { // ReorderSteps reorders workflow steps for a workflow. func (s *Service) ReorderSteps(ctx context.Context, workflowID string, stepIDs []string) error { - for i, stepID := range stepIDs { + if err := s.AuthorizeWorkflow(ctx, workflowID); err != nil { + return err + } + // Resolve and check every step before writing any of them. The step IDs + // are caller-supplied and are not required by anything upstream to belong + // to workflowID, so a reorder could set positions on another workflow's + // steps — including a read-only one, whose guard only ever saw the + // workflow named in the URL. A rejection found halfway through the write + // loop would also leave a half-applied reorder behind. + steps := make([]*models.WorkflowStep, 0, len(stepIDs)) + for _, stepID := range stepIDs { step, err := s.repo.GetStep(ctx, stepID) if err != nil { s.logger.Error("failed to get step for reorder", zap.String("step_id", stepID), zap.Error(err)) return err } + if step == nil || step.WorkflowID != workflowID { + // Same answer as a step that does not exist: the caller named an + // ID this workflow does not own, and whether it exists elsewhere + // is not theirs to learn. + s.logger.Warn("refused to reorder a step from another workflow", + zap.String("step_id", stepID), zap.String("workflow_id", workflowID)) + return ErrNotVisible + } + steps = append(steps, step) + } + for i, step := range steps { step.Position = i if err := s.repo.UpdateStep(ctx, step); err != nil { - s.logger.Error("failed to update step position", zap.String("step_id", stepID), zap.Error(err)) + s.logger.Error("failed to update step position", zap.String("step_id", step.ID), zap.Error(err)) return err } } @@ -629,6 +658,9 @@ type ImportResult struct { // ExportWorkflow exports a single workflow with its steps as portable JSON. func (s *Service) ExportWorkflow(ctx context.Context, workflowID string) (*models.WorkflowExport, error) { + if err := s.AuthorizeWorkflow(ctx, workflowID); err != nil { + return nil, err + } wf, err := s.workflowProvider.GetWorkflow(ctx, workflowID) if err != nil { return nil, fmt.Errorf("failed to get workflow: %w", err) @@ -653,6 +685,9 @@ func (s *Service) ExportWorkflow(ctx context.Context, workflowID string) (*model // passes explicit IDs; the back-compat "export everything" path leaves them out // so a bare GET of the export endpoint can't leak system flows. func (s *Service) ExportWorkflows(ctx context.Context, workspaceID string, workflowIDs []string) (*models.WorkflowExport, error) { + if err := s.AuthorizeWorkspace(ctx, workspaceID); err != nil { + return nil, err + } includeHidden := workflowIDs != nil workflows, err := s.workflowProvider.ListWorkflows(ctx, workspaceID, includeHidden) if err != nil { @@ -690,6 +725,9 @@ func filterWorkflowsByID(workflows []*taskmodels.Workflow, ids []string) []*task // ImportWorkflows imports workflows into a workspace. Deduplicates by name. func (s *Service) ImportWorkflows(ctx context.Context, workspaceID string, export *models.WorkflowExport) (*ImportResult, error) { + if err := s.AuthorizeWorkspace(ctx, workspaceID); err != nil { + return nil, err + } if err := export.Validate(); err != nil { return nil, fmt.Errorf("invalid export data: %w", err) } @@ -737,6 +775,9 @@ func (s *Service) importSingleWorkflow(ctx context.Context, workspaceID string, if err := models.ValidateWorkflowStep(step); err != nil { return nil, fmt.Errorf("validate step %q: %w", sp.Name, err) } + if err := s.validateImportedStepReferences(ctx, step, sp.Name); err != nil { + return nil, err + } steps = append(steps, step) } @@ -772,6 +813,32 @@ func (s *Service) importSingleWorkflow(ctx context.Context, workspaceID string, return wf, nil } +// validateImportedStepReferences authorizes the tasks an imported step would +// queue work on. +// +// Transition targets need no check here. An export names them by position: +// WorkflowExport.Validate rejects a raw `step_id` under on_turn_start and +// on_turn_complete and requires each position to exist in the document, and +// ConvertPositionToStepID then maps every one onto a step this import just +// created. The Phase 2 triggers carry no reference at all, because that +// converter drops those lists. If either of those stops being true, the +// same-workflow check for step targets belongs here. +// +// A queue_run task target has no positional form, and an on_enter action is +// copied through the conversion verbatim, so a hand-written document can name +// any task in the install — authorize it against the importing caller exactly +// as the step-write API does. +func (s *Service) validateImportedStepReferences( + ctx context.Context, step *models.WorkflowStep, name string, +) error { + for _, taskID := range models.CollectStepEventReferences(step.Events).TaskIDs { + if err := s.AuthorizeTask(ctx, taskID); err != nil { + return fmt.Errorf("step %q: %w", name, err) + } + } + return nil +} + // stepFromPortable builds a WorkflowStep from its portable form, remapping // position-based references to the step IDs in posToID and matching the // step-level agent profile when a matcher is wired. existingProfileID is the