diff --git a/server/internal/api/serverscope.go b/server/internal/api/serverscope.go index 5a0de78..aca638d 100644 --- a/server/internal/api/serverscope.go +++ b/server/internal/api/serverscope.go @@ -216,18 +216,29 @@ var serverScopedRoutes = map[string]scopeDecl{ "GET /api/runs/:runId/servers/:serverId/logs": scoped, "GET /api/runs/:runId/servers/:serverId/logs/stream": scoped, - // Deleting a workflow, cancelling a run and arming a schedule all act on a - // definition rather than on a server, and none of them returns server - // data. Each can nevertheless reach a definition whose targets a - // restricted token cannot see — a cancel stops work on out-of-scope hosts, - // a schedule arms it there. That reach is real but bounded: the caller - // learns nothing about which hosts are involved (both /workflows listings - // are scoped), and a scope-narrowed variant of "cancel this run" would - // have to either half-cancel a run or refuse one whose targets are mixed, - // neither of which is a better answer than the current one. Recorded as a - // deliberate choice, not an oversight. - "DELETE /api/workflows/:id": fleetWide, - "POST /api/runs/:runId/cancel": fleetWide, + // Deleting a workflow and cancelling a run both act on a definition rather + // than on a server, and neither returns server data. Each can + // nevertheless reach a definition whose targets a restricted token cannot + // see — a cancel stops work on out-of-scope hosts. That reach is real but + // bounded: the caller learns nothing about which hosts are involved (both + // /workflows listings are scoped), and a scope-narrowed variant of + // "cancel this run" would have to either half-cancel a run or refuse one + // whose targets are mixed, neither of which is a better answer than the + // current one. Recorded as a deliberate choice, not an oversight. + "DELETE /api/workflows/:id": fleetWide, + "POST /api/runs/:runId/cancel": fleetWide, + + // Arming a schedule applies no scope check of its own, and that is safe + // only because it has nothing left to check: CreateWorkflow and + // UpdateWorkflow (internal/services/workflows.go) already refuse to save + // a workflow whose resolved targets — TargetServerIDs union TargetTags — + // reach outside the acting credential's scope, the same all-or-nothing + // rule the MCP create_workflow tool applies. So by the time a workflow + // exists to be scheduled, its targets were already constrained to + // whichever scope wrote them. The scheduler later fires it with a nil + // token scope, acting as the system rather than as any caller, and that + // is fine precisely because the targets were fixed at write time, not at + // fire time. "PUT /api/workflows/:id/schedule": fleetWide, "GET /api/workflows/:id/schedule/preview": exempt, diff --git a/server/internal/services/workflow_target_scope_test.go b/server/internal/services/workflow_target_scope_test.go new file mode 100644 index 0000000..e43a174 --- /dev/null +++ b/server/internal/services/workflow_target_scope_test.go @@ -0,0 +1,98 @@ +package services + +import ( + "testing" + + "gitea.hostxtra.co.uk/mrhid6/vantage/server/internal/models" +) + +// allowSaveTargets is the pure core of validateWorkflowTargetScope: the +// all-or-nothing rule that CreateWorkflow/UpdateWorkflow now enforce, and +// that create_workflow already enforced on the MCP surface. Proved here +// without a database by resolving over an in-memory fleet the same way +// ResolveTargets/ResolveTargetsScoped do. +func allowSaveTargets(all []models.Server, ids []string, tags, tokenScope map[string]string) bool { + allTargets := UnionTargets(all, ids, tags) + if len(allTargets) == 0 { + // No targets at all, or none matched — nothing to escalate through. + return true + } + if tokenScope == nil { + return true + } + visible := make([]models.Server, 0, len(all)) + for _, s := range all { + if ServerInTokenScope(s, tokenScope) { + visible = append(visible, s) + } + } + scopedTargets := UnionTargets(visible, ids, tags) + return len(scopedTargets) == len(allTargets) +} + +// This is the exact hole: an empty TargetServerIDs with a TargetTags selector +// reaching outside the caller's scope must be refused at save time, not left +// to surface later when the scheduler fires the workflow with no restriction +// of its own. +func TestSaveTargetsRestrictedCallerCannotReachOutsideScope(t *testing.T) { + fleet := []models.Server{ + {ServerID: "stg-1", Tags: map[string]string{"env": "staging"}}, + {ServerID: "prod-1", Tags: map[string]string{"env": "production"}}, + } + staging := map[string]string{"env": "staging"} + + if allowSaveTargets(fleet, nil, map[string]string{"env": "production"}, staging) { + t.Error("staging-scoped caller allowed to save a workflow targeting env=production by tag") + } + if allowSaveTargets(fleet, []string{"prod-1"}, nil, staging) { + t.Error("staging-scoped caller allowed to save a workflow naming an out-of-scope server ID") + } +} + +func TestSaveTargetsUnrestrictedCallerUnaffected(t *testing.T) { + fleet := []models.Server{ + {ServerID: "stg-1", Tags: map[string]string{"env": "staging"}}, + {ServerID: "prod-1", Tags: map[string]string{"env": "production"}}, + } + + if !allowSaveTargets(fleet, nil, map[string]string{"env": "production"}, nil) { + t.Error("nil (unrestricted) token scope was blocked from a valid target selector") + } + if !allowSaveTargets(fleet, []string{"prod-1"}, nil, nil) { + t.Error("nil (unrestricted) token scope was blocked from naming a server by ID") + } +} + +func TestSaveTargetsEqualOrNarrowerSelectorAllowed(t *testing.T) { + fleet := []models.Server{ + {ServerID: "stg-1", Tags: map[string]string{"env": "staging", "team": "core"}}, + {ServerID: "stg-2", Tags: map[string]string{"env": "staging", "team": "web"}}, + } + staging := map[string]string{"env": "staging"} + + // Equal selector: everything the caller can already see. + if !allowSaveTargets(fleet, nil, map[string]string{"env": "staging"}, staging) { + t.Error("equal selector refused") + } + // Narrower selector: a strict subset of the caller's scope. + if !allowSaveTargets(fleet, nil, map[string]string{"env": "staging", "team": "core"}, staging) { + t.Error("narrower selector refused") + } +} + +// An empty TargetTags selector matches nothing (MatchesTags is deliberately +// exclusive on empty), and a workflow with no targets at all must stay +// creatable — the scope check must not turn that into a refusal by accident. +func TestSaveTargetsNoTargetsAtAllIsUnaffected(t *testing.T) { + fleet := []models.Server{ + {ServerID: "stg-1", Tags: map[string]string{"env": "staging"}}, + } + staging := map[string]string{"env": "staging"} + + if !allowSaveTargets(fleet, nil, nil, staging) { + t.Error("workflow with no targets at all was refused for a restricted caller") + } + if !allowSaveTargets(fleet, nil, nil, nil) { + t.Error("workflow with no targets at all was refused for an unrestricted caller") + } +} diff --git a/server/internal/services/workflows.go b/server/internal/services/workflows.go index f907835..2f8df33 100644 --- a/server/internal/services/workflows.go +++ b/server/internal/services/workflows.go @@ -261,6 +261,9 @@ func CreateWorkflow(instanceID string, w models.Workflow, tokenScope map[string] if err := validateTargetServers(instanceID, w.TargetServerIDs, tokenScope); err != nil { return nil, err } + if err := validateWorkflowTargetScope(instanceID, w.TargetServerIDs, w.TargetTags, tokenScope); err != nil { + return nil, err + } normalizeInlineSteps(&w) if _, err := db.Col("workflows").InsertOne(ctx, w); err != nil { return nil, err @@ -280,6 +283,9 @@ func UpdateWorkflow(instanceID, id string, w models.Workflow, tokenScope map[str if err := validateTargetServers(instanceID, w.TargetServerIDs, tokenScope); err != nil { return err } + if err := validateWorkflowTargetScope(instanceID, w.TargetServerIDs, w.TargetTags, tokenScope); err != nil { + return err + } normalizeInlineSteps(&w) _, err := db.Col("workflows").UpdateOne(ctx, bson.M{"workflow_id": id, "instance_id": instanceID}, bson.M{"$set": bson.M{ "name": w.Name, @@ -313,6 +319,42 @@ func validateTargetServers(instanceID string, serverIDs []string, tokenScope map return nil } +// validateWorkflowTargetScope refuses to save a workflow whose combined +// targets (IDs union tags) reach outside the acting credential's scope. +// +// validateTargetServers only checks the ID half; a token restricted to +// env=staging could otherwise leave TargetServerIDs empty, set TargetTags to +// {env: production}, and reach those hosts later through the scheduler, which +// fires with no restriction of its own because it acts as the system rather +// than as any caller. This is the same all-or-nothing rule create_workflow +// enforces on the MCP surface: resolve the full target set unscoped and again +// scoped, and refuse unless they match exactly. +// +// A nil tokenScope is unrestricted and always passes. A workflow with no +// targets at all resolves to ErrNoTargets on both sides and is left alone — +// that is an existing, separate concern (validated elsewhere, or simply +// allowed), not a scope violation. +// +// The refusal message matches validateTargetServers' so a restricted caller +// cannot tell "these hosts exist but you cannot have them" apart from "your +// tag selector matches nothing"; it must not become a fleet enumeration +// oracle. +func validateWorkflowTargetScope(instanceID string, serverIDs []string, tags map[string]string, tokenScope map[string]string) error { + if tokenScope == nil { + return nil + } + allTargets, err := ResolveTargets(instanceID, serverIDs, tags) + if err != nil { + // No targets at all, or none matched — nothing to escalate through. + return nil + } + scopedTargets, err := ResolveTargetsScoped(instanceID, serverIDs, tags, tokenScope) + if err != nil || len(scopedTargets) != len(allTargets) { + return fmt.Errorf("target tags match no servers visible to this token") + } + return nil +} + func normalizeInlineSteps(w *models.Workflow) { for i := range w.Steps { in := w.Steps[i].Inline