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/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,
 

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)
 	}
 }

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
 }