fix: bind scope tests to real code, close time-of-write gap, fail closed

The previous tests reimplemented the target-scope rule instead of calling
validateWorkflowTargetScope, so they proved nothing about CreateWorkflow
or UpdateWorkflow's actual enforcement. Split the check into a pure
decideWorkflowTargetScope (tested directly, no database) and a thin
wrapper behind an overridable listServersForScope seam, so tests can
invoke the real CreateWorkflow/UpdateWorkflow without a live database and
fail if the call sites are removed.

Close the time-of-write/time-of-fire gap: a restricted caller could
previously save target_tags matching no server today (a selector aimed
at hosts not yet provisioned or not yet tagged), pass validation on an
empty set, and have the scheduler fire on those hosts the moment they
appeared. Now a restricted caller specifying targets that resolve to
nothing is refused with the same message as an out-of-scope match; a
workflow with no targets at all, and an unrestricted caller, are
unaffected.

A database error while resolving the fleet now surfaces as an error
instead of folding into a pass.

Correct three comments that overstated what the code does: the
create/update route comment now mentions the tag-scope check, not only
validateTargetServers; the schedule route comment explains its safety
holds only for workflows written after this check existed, not for rows
already in the database under the old rule.

server/internal/api/scopes.go

@@ -210,6 +210,17 @@ func RequireScopes() gin.HandlerFunc {
 	}
 }
 
+// routesOutsideAPIGroup lists routes matching the "/api/" path prefix that
+// are nonetheless registered on the root router rather than the authenticated
+// /api group, so they carry no session or API-token auth and are exempt from
+// routeScopes. scopes.go and serverscope_test.go both read this set so the
+// exception is stated once.
+var routesOutsideAPIGroup = map[string]bool{
+	// The ESO endpoint keeps its own bearer scheme and is deliberately
+	// outside the token vocabulary.
+	"GET /api/secrets/:group/values": true,
+}
+
 // AssertScopeMapComplete fails boot when a registered /api route has no scope.
 //
 // Without it, adding a route silently makes it unreachable by every token, and
@@ -220,9 +231,9 @@ func AssertScopeMapComplete(r *gin.Engine) error {
 		if !strings.HasPrefix(route.Path, "/api/") {
 			continue
 		}
-		// The ESO endpoint keeps its own bearer scheme and is deliberately
-		// outside the token vocabulary.
-		if route.Path == "/api/secrets/:group/values" {
+		// Routes registered outside the authenticated /api group (their own
+		// bearer scheme, not the token vocabulary) are deliberately excluded.
+		if routesOutsideAPIGroup[route.Method+" "+route.Path] {
 			continue
 		}
 		if _, ok := routeScopes[route.Method+" "+route.Path]; !ok {

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,
 
@@ -216,18 +221,35 @@ 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 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,
 

server/internal/api/serverscope_test.go

@@ -13,9 +13,13 @@ func TestServerScopeMapCoversEveryScopedRoute(t *testing.T) {
 		}
 	}
 	for r := range serverScopedRoutes {
-		if _, ok := routeScopes[r]; !ok {
-			t.Errorf("route %q is declared in serverScopedRoutes but is not a registered route", r)
+		if _, ok := routeScopes[r]; ok {
+			continue
 		}
+		if routesOutsideAPIGroup[r] {
+			continue
+		}
+		t.Errorf("route %q is declared in serverScopedRoutes but is not a registered route", r)
 	}
 }
 

server/internal/services/workflow_target_scope_test.go

@@ -0,0 +1,165 @@
+package services
+
+import (
+	"errors"
+	"testing"
+
+	"gitea.hostxtra.co.uk/mrhid6/vantage/server/internal/models"
+)
+
+// 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.
+
+func TestSaveTargetsRestrictedCallerCannotReachOutsideScope(t *testing.T) {
+	// 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"}},
+	}
+	scoped := []models.Server{all[0]} // only stg-1 visible to a staging token
+
+	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) {
+	// 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 err := validateWorkflowTargetScope("inst-1", nil, map[string]string{"env": "production"}, nil); err != nil {
+		t.Errorf("unrestricted caller refused: %v", err)
+	}
+	if err := validateWorkflowTargetScope("inst-1", []string{"anything"}, nil, nil); err != nil {
+		t.Errorf("unrestricted caller refused: %v", err)
+	}
+}
+
+func TestSaveTargetsEqualOrNarrowerSelectorAllowed(t *testing.T) {
+	all := []models.Server{
+		{ServerID: "stg-1", Tags: map[string]string{"env": "staging", "team": "core"}},
+	}
+	// Equal: the scoped resolution matches the unscoped one exactly.
+	if err := decideWorkflowTargetScope(all, all); err != nil {
+		t.Errorf("equal selector refused: %v", err)
+	}
+	// 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)
+	}
+}
+
+// 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) {
+	restore := listServersForScope
+	listServersForScope = func(instanceID string) ([]models.Server, error) {
+		t.Fatal("listServersForScope called for a workflow with no targets at all")
+		return nil, nil
+	}
+	defer func() { listServersForScope = restore }()
+
+	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 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)
+	}
+}

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,84 @@ 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.
+//
+// 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: 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.
+//
+// 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 {
+	hasTargets := len(serverIDs) > 0 || len(tags) > 0
+	if !hasTargets || tokenScope == nil {
+		return nil
+	}
+	all, err := listServersForScope(instanceID)
+	if err != nil {
+		return err
+	}
+	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
+}
+
 func normalizeInlineSteps(w *models.Workflow) {
 	for i := range w.Steps {
 		in := w.Steps[i].Inline