fix(server): close fail-open paths in monitor org checks

Review hardening on e2f5f1f. None of these were live bypasses; all were
one bad row or one new caller away from becoming one.

- Replace the empty-orgID sentinel with explicit scheduler entry points.
  The sentinel meant "skip the org check" and was keyed on a value read
  from a DB record on the agent path, so a server doc with a blank org_id
  would silently disable isolation. The exported agent-facing functions
  now reject an empty orgID outright.
- ValidateAgentToken errors when the resolved server has no org.
- UpdateMonitor's runner and channel_ids type assertions were fail-open:
  a wrong-typed value skipped validation while the $set still ran. Now a
  hard error.
- Normalise an empty runner on update to the server runner, matching
  create. Previously it matched no runner at all, so the monitor silently
  stopped being checked and stopped alerting.
- IngestResult returns an error for an unknown monitor, so probing an
  unknown ID looks the same as probing a foreign one.

server/internal/monitorsched/scheduler.go

@@ -35,7 +35,7 @@ func loop(ctx context.Context) {
 	var mu sync.Mutex
 
 	sync := func() {
-		monitors, err := services.ListMonitorsForRunner("", models.RunnerServer)
+		monitors, err := services.ListServerScheduledMonitors()
 		if err != nil {
 			log.Printf("monitorsched: list monitors: %v", err)
 			return
@@ -88,7 +88,7 @@ func runMonitor(ctx context.Context, m models.Monitor) {
 
 	run := func() {
 		res := checker.Run(ctx, spec)
-		if err := services.IngestResult("", models.RunnerServer, m.MonitorID, res); err != nil {
+		if err := services.IngestServerScheduledResult(m.MonitorID, res); err != nil {
 			log.Printf("monitorsched: ingest %s: %v", m.MonitorID, err)
 		}
 	}

server/internal/services/monitors.go

@@ -51,13 +51,30 @@ func ListMonitors(orgID string) ([]models.Monitor, error) {
 	return out, nil
 }
 
-// ListMonitorsForRunner returns enabled monitors whose Runner matches runner.
-// Runner is client-supplied at write time, so an agent fetching its own work
-// must scope by the org of its authenticated server record — otherwise another
-// org could point a monitor at that server_id and have it run their checks.
-// An empty orgID means the cross-org server-scheduler sweep (mirrors
-// MarkOfflineServers) and is only ever passed with runner == RunnerServer.
+// ListMonitorsForRunner returns enabled monitors whose Runner matches runner,
+// scoped to orgID. Runner is client-supplied at write time, so an agent fetching
+// its own work must scope by the org of its authenticated server record —
+// otherwise another org could point a monitor at that server_id and have it run
+// their checks. An empty orgID is rejected: it would silently widen the query to
+// every org.
 func ListMonitorsForRunner(orgID, runner string) ([]models.Monitor, error) {
+	if orgID == "" {
+		return nil, errors.New("org id required")
+	}
+	return listMonitorsForRunner(orgID, runner)
+}
+
+// ListServerScheduledMonitors returns every enabled server-run monitor across
+// all orgs. This is the in-process scheduler's entry point (mirrors the cross-org
+// MarkOfflineServers sweep) and must never be called from a request-driven path —
+// it performs no org scoping at all.
+func ListServerScheduledMonitors() ([]models.Monitor, error) {
+	return listMonitorsForRunner("", models.RunnerServer)
+}
+
+// listMonitorsForRunner is the shared query. An empty orgID means no org filter
+// and is only reachable via ListServerScheduledMonitors.
+func listMonitorsForRunner(orgID, runner string) ([]models.Monitor, error) {
 	ctx, cancel := monCtx()
 	defer cancel()
 	filter := bson.M{"runner": runner, "enabled": true}
@@ -151,15 +168,31 @@ func CreateMonitor(orgID string, m *models.Monitor) (*models.Monitor, error) {
 func UpdateMonitor(orgID, monitorID string, upd bson.M) error {
 	ctx, cancel := monCtx()
 	defer cancel()
-	if ids, ok := upd["channel_ids"].([]string); ok {
+	// A present-but-wrong-type value is a hard error: silently skipping the
+	// check would still let the unvalidated value through to the $set.
+	if raw, present := upd["channel_ids"]; present {
+		ids, ok := raw.([]string)
+		if !ok {
+			return fmt.Errorf("channel_ids must be a string array")
+		}
 		if err := validateChannelIDs(orgID, ids); err != nil {
 			return err
 		}
 	}
-	if runner, ok := upd["runner"].(string); ok {
+	if raw, present := upd["runner"]; present {
+		runner, ok := raw.(string)
+		if !ok {
+			return fmt.Errorf("runner must be a string")
+		}
 		if err := validateRunner(orgID, runner); err != nil {
 			return err
 		}
+		// Match CreateMonitor: an empty runner means the server scheduler.
+		// Storing "" would match no runner at all and silently stop the
+		// monitor being checked.
+		if runner == "" {
+			upd["runner"] = models.RunnerServer
+		}
 	}
 	_, err := db.Col("monitors").UpdateOne(ctx, bson.M{"monitor_id": monitorID, "org_id": orgID}, bson.M{"$set": upd})
 	return err
@@ -225,16 +258,39 @@ func UptimeRollups(monitorID string, since time.Time) ([]models.Rollup, error) {
 // monitorID is client-supplied on the agent path, so the caller passes the org
 // and runner it is authenticated as: orgID is the reporting agent's server org
 // and runner is its server_id. A result is only applied to a monitor owned by
-// that org and assigned to that runner. An empty orgID is the in-process server
-// scheduler, which passes runner == RunnerServer.
+// that org and assigned to that runner. An empty orgID is rejected — it would
+// skip the ownership check entirely.
 func IngestResult(orgID, runner, monitorID string, res checker.Result) error {
+	if orgID == "" {
+		return errors.New("org id required")
+	}
+	return ingestResult(orgID, runner, monitorID, res)
+}
+
+// IngestServerScheduledResult applies a result produced by the in-process server
+// scheduler, which has no org context of its own. This is the scheduler's entry
+// point and must never be called from a request-driven path — it skips the org
+// ownership check.
+func IngestServerScheduledResult(monitorID string, res checker.Result) error {
+	return ingestResult("", models.RunnerServer, monitorID, res)
+}
+
+// ingestResult is the shared implementation. An empty orgID skips the org
+// ownership check and is only reachable via IngestServerScheduledResult.
+func ingestResult(orgID, runner, monitorID string, res checker.Result) error {
 	ctx, cancel := monCtx()
 	defer cancel()
 
 	m, err := getMonitorByID(monitorID)
-	if err != nil || m == nil {
+	if err != nil {
 		return err
 	}
+	// Report not-found the same way as a cross-org hit, so probing an unknown
+	// monitor_id is no quieter than probing a foreign one and stale monitors
+	// stay visible to operators.
+	if m == nil {
+		return fmt.Errorf("monitor %s not found", monitorID)
+	}
 	if orgID != "" && m.OrgID != orgID {
 		return fmt.Errorf("monitor %s belongs to another org", monitorID)
 	}

server/internal/services/servers.go

@@ -181,6 +181,11 @@ func ValidateAgentToken(serverID, agentToken string) (*models.Server, error) {
 	if err != nil {
 		return nil, fmt.Errorf("invalid agent token")
 	}
+	// Defence in depth: every agent-path caller scopes its work by this OrgID,
+	// so a blank one would widen those queries instead of narrowing them.
+	if s.OrgID == "" {
+		return nil, fmt.Errorf("server %s has no org", serverID)
+	}
 	return &s, nil
 }