diff --git a/server/internal/api/serverscope.go b/server/internal/api/serverscope.go index aca638d..8cadf38 100644 --- a/server/internal/api/serverscope.go +++ b/server/internal/api/serverscope.go @@ -187,11 +187,16 @@ var serverScopedRoutes = map[string]scopeDecl{ "GET /api/workflows/:id": scoped, // createWorkflow/updateWorkflow validate target_server_ids through - // services.validateTargetServers, which resolves each named ID with - // GetServerScoped — so a restricted token can neither save a workflow - // targeting a host outside its scope (which the scheduler, firing as the - // system, would otherwise run there) nor learn which IDs exist by the - // difference between "target server not found" and a successful save. + // services.validateTargetServers (GetServerScoped per ID) and separately + // validate the ID-union-tags target set as a whole through + // services.validateWorkflowTargetScope, which resolves the workflow's + // targets both unscoped and scoped and refuses to save unless they match + // — the same all-or-nothing rule the MCP create_workflow tool applies. + // Together these mean a restricted token can neither save a workflow + // targeting a host or tag outside its scope (which the scheduler, firing + // as the system, would otherwise run there) nor learn which IDs or tags + // resolve to something by the difference between a refusal and a + // successful save. "POST /api/workflows": scoped, "PUT /api/workflows/:id": scoped, @@ -233,12 +238,18 @@ var serverScopedRoutes = map[string]scopeDecl{ // 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. + // rule the MCP create_workflow tool applies. So a workflow written after + // this check existed had its targets constrained to whichever scope wrote + // it, and the scheduler firing it later with a nil token scope — acting + // as the system, not as any caller — reaches nothing that write didn't + // already allow. + // + // This holds only for workflows written after the check was added. Rows + // already in the database were saved under the old, unvalidated rule and + // are never re-validated — neither this route nor the writers re-check an + // existing row's targets after the fact. A workflow saved before this fix + // with an out-of-scope tag selector still schedules and fires exactly as + // it did before. "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 index e43a174..d616b92 100644 --- a/server/internal/services/workflow_target_scope_test.go +++ b/server/internal/services/workflow_target_scope_test.go @@ -1,98 +1,165 @@ package services import ( + "errors" "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) -} +// decideWorkflowTargetScope is the pure core validateWorkflowTargetScope +// calls (internal/services/workflows.go), which CreateWorkflow and +// UpdateWorkflow in turn call. Testing it directly, rather than a +// reimplementation of the rule, means these tests exercise the exact +// decision the shipped code makes. -// 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{ + // A tag selector reaching a server outside scope: resolves to something + // fleet-wide, but the scoped view comes up short. + all := []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"} + scoped := []models.Server{all[0]} // only stg-1 visible to a staging token - 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") + if err := decideWorkflowTargetScope(all, scoped); !errors.Is(err, ErrWorkflowTargetOutOfScope) { + t.Errorf("out-of-scope target set: got %v, want ErrWorkflowTargetOutOfScope", err) } } 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"}}, + // validateWorkflowTargetScope short-circuits before ever resolving the + // fleet when tokenScope is nil, so an unrestricted caller keeps today's + // behaviour exactly — including saving a tag selector matching nothing. + // listServersForScope is stubbed to fail the test if called at all, so + // this proves the short-circuit, not just that the decision would allow + // it. + restore := listServersForScope + listServersForScope = func(instanceID string) ([]models.Server, error) { + t.Fatal("listServersForScope called for an unrestricted (nil scope) caller") + return nil, nil } + defer func() { listServersForScope = restore }() - if !allowSaveTargets(fleet, nil, map[string]string{"env": "production"}, nil) { - t.Error("nil (unrestricted) token scope was blocked from a valid target selector") + if err := validateWorkflowTargetScope("inst-1", nil, map[string]string{"env": "production"}, nil); err != nil { + t.Errorf("unrestricted caller refused: %v", err) } - if !allowSaveTargets(fleet, []string{"prod-1"}, nil, nil) { - t.Error("nil (unrestricted) token scope was blocked from naming a server by ID") + if err := validateWorkflowTargetScope("inst-1", []string{"anything"}, nil, nil); err != nil { + t.Errorf("unrestricted caller refused: %v", err) } } func TestSaveTargetsEqualOrNarrowerSelectorAllowed(t *testing.T) { - fleet := []models.Server{ + all := []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") + // Equal: the scoped resolution matches the unscoped one exactly. + if err := decideWorkflowTargetScope(all, all); err != nil { + t.Errorf("equal selector refused: %v", err) } - // 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") + // Narrower: same story, a stricter selector still returns everything the + // unscoped resolution does when every match is in scope. + narrower := []models.Server{all[0]} + if err := decideWorkflowTargetScope(all, narrower); err != nil { + t.Errorf("narrower selector refused: %v", err) } } -// 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. +// The time-of-write/time-of-fire gap: a restricted caller naming IDs or tags +// that match no server at all today must be refused, not passed through — +// otherwise the caller could save a selector for an environment that does +// not exist yet, arm the schedule, and have it fire the moment a server picks +// up the tag. This is distinct from "no targets at all" below. +func TestSaveTargetsMatchingNothingIsRefusedForRestrictedCaller(t *testing.T) { + if err := decideWorkflowTargetScope(nil, nil); !errors.Is(err, ErrWorkflowTargetOutOfScope) { + t.Errorf("targets specified but resolving to nothing: got %v, want ErrWorkflowTargetOutOfScope", err) + } +} + +// --- Binding to the shipped call sites --- +// +// The cases above test decideWorkflowTargetScope's rule in isolation. These +// two prove CreateWorkflow and UpdateWorkflow actually invoke it: they call +// the real functions with a restricted token and an out-of-scope tag +// selector and an empty server ID list, so validateTargetServers (which +// walks TargetServerIDs) never touches the database and the only way the +// call can fail before reaching db.Col(...).InsertOne/UpdateOne is through +// validateWorkflowTargetScope. listServersForScope is swapped for a stub so +// no live database is needed. +// +// Removing either call site in workflows.go makes these fail: without a +// database connection, CreateWorkflow/UpdateWorkflow then fall through to +// the real db.Col(...), whose *mongo.Database is nil in this test binary, and +// the call panics instead of returning ErrWorkflowTargetOutOfScope. That was +// verified by hand: deleting the validateWorkflowTargetScope call in +// CreateWorkflow and running this test produces +// +// panic: runtime error: invalid memory address or nil pointer dereference +// ... +// FAIL .../internal/services 0.006s +// +// rather than a clean assertion failure, which is still a failure — the test +// no longer passes silently once the enforcement is removed. +func TestCreateAndUpdateWorkflowBindToTargetScopeCheck(t *testing.T) { + restore := listServersForScope + listServersForScope = func(instanceID string) ([]models.Server, error) { + return []models.Server{ + {ServerID: "stg-1", Tags: map[string]string{"env": "staging"}}, + {ServerID: "prod-1", Tags: map[string]string{"env": "production"}}, + }, nil + } + defer func() { listServersForScope = restore }() + + staging := map[string]string{"env": "staging"} + w := models.Workflow{ + Name: "escalate", + Steps: []models.WorkflowStepRef{}, + TargetTags: map[string]string{"env": "production"}, + } + + if _, err := CreateWorkflow("inst-1", w, staging); !errors.Is(err, ErrWorkflowTargetOutOfScope) { + t.Errorf("CreateWorkflow with out-of-scope tags: got %v, want ErrWorkflowTargetOutOfScope", err) + } + if err := UpdateWorkflow("inst-1", "wf-1", w, staging); !errors.Is(err, ErrWorkflowTargetOutOfScope) { + t.Errorf("UpdateWorkflow with out-of-scope tags: got %v, want ErrWorkflowTargetOutOfScope", err) + } +} + +// A workflow with no targets at all (no IDs, no tags) must stay creatable +// for a restricted caller — there is nothing to escalate through, and this +// must not become collateral damage from the fix above. func TestSaveTargetsNoTargetsAtAllIsUnaffected(t *testing.T) { - fleet := []models.Server{ - {ServerID: "stg-1", Tags: map[string]string{"env": "staging"}}, + restore := listServersForScope + listServersForScope = func(instanceID string) ([]models.Server, error) { + t.Fatal("listServersForScope called for a workflow with no targets at all") + return nil, nil } - staging := map[string]string{"env": "staging"} + defer func() { listServersForScope = restore }() - if !allowSaveTargets(fleet, nil, nil, staging) { - t.Error("workflow with no targets at all was refused for a restricted caller") + staging := map[string]string{"env": "staging"} + w := models.Workflow{Name: "no-targets", Steps: []models.WorkflowStepRef{}} + + if err := validateWorkflowTargetScope("inst-1", w.TargetServerIDs, w.TargetTags, staging); err != nil { + t.Errorf("workflow with no targets at all was refused for a restricted caller: %v", err) } - if !allowSaveTargets(fleet, nil, nil, nil) { - t.Error("workflow with no targets at all was refused for an unrestricted caller") + if err := validateWorkflowTargetScope("inst-1", w.TargetServerIDs, w.TargetTags, nil); err != nil { + t.Errorf("workflow with no targets at all was refused for an unrestricted caller: %v", err) + } +} + +// A database error while resolving the fleet must surface as an error, not +// fold into a silent pass. +func TestSaveTargetsDatabaseErrorIsNotSwallowed(t *testing.T) { + restore := listServersForScope + wantErr := errors.New("boom: database unavailable") + listServersForScope = func(instanceID string) ([]models.Server, error) { + return nil, wantErr + } + defer func() { listServersForScope = restore }() + + staging := map[string]string{"env": "staging"} + err := validateWorkflowTargetScope("inst-1", nil, map[string]string{"env": "production"}, staging) + if !errors.Is(err, wantErr) { + t.Errorf("database error during scope validation: got %v, want it surfaced as an error, not a pass", err) } } diff --git a/server/internal/services/workflows.go b/server/internal/services/workflows.go index 2f8df33..590dac4 100644 --- a/server/internal/services/workflows.go +++ b/server/internal/services/workflows.go @@ -319,6 +319,20 @@ func validateTargetServers(instanceID string, serverIDs []string, tokenScope map return nil } +// ErrWorkflowTargetOutOfScope is returned by validateWorkflowTargetScope's +// two refusal cases: a restricted caller's target set reaching outside its +// scope, and (see below) a restricted caller specifying targets that resolve +// to nothing at all. Both fold into this single sentinel and message so the +// two remain indistinguishable to the caller. +var ErrWorkflowTargetOutOfScope = errors.New("target tags match no servers visible to this token") + +// listServersForScope is ListServers, indirected so +// validateWorkflowTargetScope's database read can be swapped out in tests +// without a live database. The decision logic itself +// (decideWorkflowTargetScope) is pure and takes no database dependency at +// all; this seam exists only for the read that feeds it. +var listServersForScope = ListServers + // validateWorkflowTargetScope refuses to save a workflow whose combined // targets (IDs union tags) reach outside the acting credential's scope. // @@ -330,27 +344,55 @@ func validateTargetServers(instanceID string, serverIDs []string, tokenScope map // 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. +// A nil tokenScope is unrestricted and always passes: an unrestricted caller +// may save any selector, including one matching nothing today, exactly as +// before this fix. A workflow with no targets at all (empty IDs and empty +// tags) is also left alone regardless of scope — there is nothing for it to +// fire on, and refusing it would break the existing, unrelated ability to +// save a workflow before wiring up its targets. // -// 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. +// What IS refused, for a restricted caller only, is a workflow that names IDs +// or tags which resolve to no server at all. Without this, a token restricted +// to env=staging could save target_tags {env: production} while no server yet +// carries that pair — a not-yet-provisioned environment, a tag rollout in +// progress, a guessed value — pass validation on an empty set, arm the +// schedule, and have the scheduler execute on those hosts the moment someone +// tags them. That is the same escalation as the out-of-scope case, just +// deferred to whenever the fleet catches up to the selector, so it is +// refused the same way and with the same message. +// +// A database error while fetching the fleet is returned as-is, not folded +// into a pass: this check exists to refuse, and a transient failure to read +// the fleet must not silently become permission to save. func validateWorkflowTargetScope(instanceID string, serverIDs []string, tags map[string]string, tokenScope map[string]string) error { - if tokenScope == nil { + hasTargets := len(serverIDs) > 0 || len(tags) > 0 + if !hasTargets || tokenScope == nil { return nil } - allTargets, err := ResolveTargets(instanceID, serverIDs, tags) + all, err := listServersForScope(instanceID) if err != nil { - // No targets at all, or none matched — nothing to escalate through. - return nil + return err } - 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") + allTargets := UnionTargets(all, serverIDs, tags) + visible := make([]models.Server, 0, len(all)) + for _, s := range all { + if ServerInTokenScope(s, tokenScope) { + visible = append(visible, s) + } + } + scopedTargets := UnionTargets(visible, serverIDs, tags) + return decideWorkflowTargetScope(allTargets, scopedTargets) +} + +// decideWorkflowTargetScope is the pure core of validateWorkflowTargetScope: +// given the workflow's already-resolved unscoped and scoped target sets (both +// computed by the same UnionTargets used at run time), it decides whether a +// restricted caller may save them. It takes no database dependency, so it +// exercises exactly the same rule the shipped code applies without needing a +// database to test it. +func decideWorkflowTargetScope(allTargets, scopedTargets []models.Server) error { + if len(allTargets) == 0 || len(scopedTargets) != len(allTargets) { + return ErrWorkflowTargetOutOfScope } return nil }