From 156c5354de2113d2bcbf60455d033dbb4be0dee1 Mon Sep 17 00:00:00 2001 From: mrhid6 Date: Wed, 22 Jul 2026 10:05:37 +0100 Subject: [PATCH] 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 | 4 +- server/internal/services/monitors.go | 78 +++++++++++++++++++---- server/internal/services/servers.go | 5 ++ 3 files changed, 74 insertions(+), 13 deletions(-) diff --git a/server/internal/monitorsched/scheduler.go b/server/internal/monitorsched/scheduler.go index 214f4c9..884e3ae 100644 --- a/server/internal/monitorsched/scheduler.go +++ b/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) } } diff --git a/server/internal/services/monitors.go b/server/internal/services/monitors.go index 38427bc..45dfb74 100644 --- a/server/internal/services/monitors.go +++ b/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) } diff --git a/server/internal/services/servers.go b/server/internal/services/servers.go index 75dcb45..39d0098 100644 --- a/server/internal/services/servers.go +++ b/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 }