From dff3668a250db7f88e1f8cd6a3a0d77b82e8aa1d Mon Sep 17 00:00:00 2001 From: mrhid6 Date: Wed, 22 Jul 2026 09:28:43 +0100 Subject: [PATCH] fix(server): validate cross-org resource ownership Review of the org-scoping pass found that org_id on a query filter protects the row you look up, but does nothing when a handler accepts a foreign resource ID as data and a downstream unscoped query consumes it. - AssignKey: verify key and server both belong to the org - BuildAuthorizedKeys: resolve server first, scope assignments and keys to that server's org (was honouring foreign assignment rows) - Workflows: validate TargetServerIDs on create/update and re-check at trigger time - Monitor incidents/uptime handlers: gate on org-scoped GetMonitor - GetChannels: take orgID; validate channel_ids on monitor create/update - Secret and default-step unique indexes: scope to org_id so a second org no longer hits E11000 - DeleteServer/DeleteMonitor: scope cascading deletes --- server/internal/api/monitors.go | 18 +++++++++++++++ server/internal/services/channels.go | 21 ++++++++++++++--- server/internal/services/keys.go | 9 ++++++++ server/internal/services/monitors.go | 18 +++++++++++++-- server/internal/services/secrets.go | 21 +++++++++++++++-- server/internal/services/servers.go | 2 +- server/internal/services/sync.go | 10 ++++++++- server/internal/services/workflow_runner.go | 5 +++++ server/internal/services/workflows.go | 25 ++++++++++++++++++++- 9 files changed, 119 insertions(+), 10 deletions(-) diff --git a/server/internal/api/monitors.go b/server/internal/api/monitors.go index 4e50cbc..7dac179 100644 --- a/server/internal/api/monitors.go +++ b/server/internal/api/monitors.go @@ -121,6 +121,15 @@ func deleteMonitor(c *gin.Context) { } func getMonitorIncidents(c *gin.Context) { + m, err := services.GetMonitor(auth.OrgID(c), c.Param("id")) + if err != nil { + c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) + return + } + if m == nil { + c.JSON(http.StatusNotFound, gin.H{"error": "monitor not found"}) + return + } incidents, err := services.ListIncidents(c.Param("id"), 50) if err != nil { c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) @@ -130,6 +139,15 @@ func getMonitorIncidents(c *gin.Context) { } func getMonitorUptime(c *gin.Context) { + m, err := services.GetMonitor(auth.OrgID(c), c.Param("id")) + if err != nil { + c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) + return + } + if m == nil { + c.JSON(http.StatusNotFound, gin.H{"error": "monitor not found"}) + return + } since := time.Now().Add(-30 * 24 * time.Hour) rollups, err := services.UptimeRollups(c.Param("id"), since) if err != nil { diff --git a/server/internal/services/channels.go b/server/internal/services/channels.go index 3f91795..beaaff7 100644 --- a/server/internal/services/channels.go +++ b/server/internal/services/channels.go @@ -41,14 +41,14 @@ func GetChannel(orgID, channelID string) (*models.NotificationChannel, error) { return &ch, nil } -// GetChannels loads multiple channels by ID, skipping any not found. -func GetChannels(channelIDs []string) ([]models.NotificationChannel, error) { +// GetChannels loads multiple channels by ID within an org, skipping any not found. +func GetChannels(orgID string, channelIDs []string) ([]models.NotificationChannel, error) { if len(channelIDs) == 0 { return nil, nil } ctx, cancel := monCtx() defer cancel() - cur, err := db.Col("notification_channels").Find(ctx, bson.M{"channel_id": bson.M{"$in": channelIDs}}) + cur, err := db.Col("notification_channels").Find(ctx, bson.M{"org_id": orgID, "channel_id": bson.M{"$in": channelIDs}}) if err != nil { return nil, err } @@ -59,6 +59,21 @@ func GetChannels(channelIDs []string) ([]models.NotificationChannel, error) { return out, nil } +// validateChannelIDs rejects any channel that does not belong to the org. +// Channel IDs arrive from the client as data on monitor writes. +func validateChannelIDs(orgID string, channelIDs []string) error { + for _, id := range channelIDs { + ch, err := GetChannel(orgID, id) + if err != nil { + return err + } + if ch == nil { + return errors.New("channel " + id + " not found") + } + } + return nil +} + func CreateChannel(orgID string, ch *models.NotificationChannel) (*models.NotificationChannel, error) { ctx, cancel := monCtx() defer cancel() diff --git a/server/internal/services/keys.go b/server/internal/services/keys.go index a233fa5..9f3900f 100644 --- a/server/internal/services/keys.go +++ b/server/internal/services/keys.go @@ -175,6 +175,15 @@ func AssignKey(orgID, keyID, serverID string) (*models.Assignment, error) { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() + // Both sides must belong to the caller's org — the IDs arrive from the + // client as data and are consumed by unscoped agent-path queries later. + if _, err := GetKey(orgID, keyID); err != nil { + return nil, fmt.Errorf("key not found") + } + if _, err := GetServer(orgID, serverID); err != nil { + return nil, fmt.Errorf("server not found") + } + // Check if already assigned and active var existing models.Assignment err := db.Col("assignments").FindOne(ctx, bson.M{ diff --git a/server/internal/services/monitors.go b/server/internal/services/monitors.go index aaf196a..4de9ec6 100644 --- a/server/internal/services/monitors.go +++ b/server/internal/services/monitors.go @@ -101,6 +101,9 @@ func getMonitorByID(monitorID string) (*models.Monitor, error) { func CreateMonitor(orgID string, m *models.Monitor) (*models.Monitor, error) { ctx, cancel := monCtx() defer cancel() + if err := validateChannelIDs(orgID, m.ChannelIDs); err != nil { + return nil, err + } m.OrgID = orgID m.MonitorID = uuid.NewString() m.CreatedAt = time.Now() @@ -123,6 +126,11 @@ 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 { + if err := validateChannelIDs(orgID, ids); err != nil { + return err + } + } _, err := db.Col("monitors").UpdateOne(ctx, bson.M{"monitor_id": monitorID, "org_id": orgID}, bson.M{"$set": upd}) return err } @@ -130,9 +138,15 @@ func UpdateMonitor(orgID, monitorID string, upd bson.M) error { func DeleteMonitor(orgID, monitorID string) error { ctx, cancel := monCtx() defer cancel() - if _, err := db.Col("monitors").DeleteOne(ctx, bson.M{"monitor_id": monitorID, "org_id": orgID}); err != nil { + res, err := db.Col("monitors").DeleteOne(ctx, bson.M{"monitor_id": monitorID, "org_id": orgID}) + if err != nil { return err } + // Only cascade when the org-scoped delete actually removed a monitor — + // incidents/rollups carry no org_id of their own. + if res.DeletedCount == 0 { + return nil + } db.Col("incidents").DeleteMany(ctx, bson.M{"monitor_id": monitorID}) db.Col("monitor_rollups").DeleteMany(ctx, bson.M{"monitor_id": monitorID}) return nil @@ -263,7 +277,7 @@ func notifyTransition(m *models.Monitor, newStatus, message string) { if len(m.ChannelIDs) == 0 { return } - channels, err := GetChannels(m.ChannelIDs) + channels, err := GetChannels(m.OrgID, m.ChannelIDs) if err != nil { log.Printf("notify: load channels for %s: %v", m.MonitorID, err) return diff --git a/server/internal/services/secrets.go b/server/internal/services/secrets.go index a1a0e97..ab6718c 100644 --- a/server/internal/services/secrets.go +++ b/server/internal/services/secrets.go @@ -2,6 +2,7 @@ package services import ( "context" + "errors" "fmt" "sort" "time" @@ -13,18 +14,34 @@ import ( "go.mongodb.org/mongo-driver/v2/mongo/options" ) -// EnsureSecretIndexes creates the unique compound index on (group, key). +// EnsureSecretIndexes creates the unique compound index on (org_id, group, key). +// The pre-multi-tenant index was on (group, key) alone, which made a second org +// collide on the same group/key — drop it if a live DB still carries it. func EnsureSecretIndexes() error { ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() + if err := db.Col("secrets").Indexes().DropOne(ctx, "group_1_key_1"); err != nil && !isIndexNotFound(err) { + return err + } + _, err := db.Col("secrets").Indexes().CreateOne(ctx, mongo.IndexModel{ - Keys: bson.D{{Key: "group", Value: 1}, {Key: "key", Value: 1}}, + Keys: bson.D{{Key: "org_id", Value: 1}, {Key: "group", Value: 1}, {Key: "key", Value: 1}}, Options: options.Index().SetUnique(true), }) return err } +// isIndexNotFound reports whether err is Mongo's IndexNotFound (27), returned +// when dropping an index that was never created. +func isIndexNotFound(err error) bool { + var ce mongo.CommandError + if errors.As(err, &ce) { + return ce.Code == 27 || ce.Name == "IndexNotFound" + } + return false +} + // ListSecretGroups returns a summary of every group with its key count and // most recent update time. func ListSecretGroups(orgID string) ([]models.GroupSummary, error) { diff --git a/server/internal/services/servers.go b/server/internal/services/servers.go index 3900836..86b5f9f 100644 --- a/server/internal/services/servers.go +++ b/server/internal/services/servers.go @@ -257,7 +257,7 @@ func DeleteServer(orgID, serverID string) error { return err } // Also remove assignments - _, err = db.Col("assignments").DeleteMany(ctx, bson.M{"server_id": serverID}) + _, err = db.Col("assignments").DeleteMany(ctx, bson.M{"server_id": serverID, "org_id": orgID}) return err } diff --git a/server/internal/services/sync.go b/server/internal/services/sync.go index d3add76..d4bd523 100644 --- a/server/internal/services/sync.go +++ b/server/internal/services/sync.go @@ -13,7 +13,15 @@ func BuildAuthorizedKeys(serverID string) ([]string, error) { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() + // Agent path — no session, so the org comes from the server record itself + // and both follow-up queries are scoped to it. + srv, err := getServerByID(serverID) + if err != nil { + return nil, err + } + cursor, err := db.Col("assignments").Find(ctx, bson.M{ + "org_id": srv.OrgID, "server_id": serverID, "revoked_at": nil, }) @@ -30,7 +38,7 @@ func BuildAuthorizedKeys(serverID string) ([]string, error) { var lines []string for _, a := range assignments { var key models.Key - err := db.Col("keys").FindOne(ctx, bson.M{"key_id": a.KeyID}).Decode(&key) + err := db.Col("keys").FindOne(ctx, bson.M{"key_id": a.KeyID, "org_id": srv.OrgID}).Decode(&key) if err != nil { continue } diff --git a/server/internal/services/workflow_runner.go b/server/internal/services/workflow_runner.go index 23f3bda..7ab91b0 100644 --- a/server/internal/services/workflow_runner.go +++ b/server/internal/services/workflow_runner.go @@ -30,6 +30,11 @@ func TriggerWorkflow(orgID, workflowID, actor string) (string, error) { if len(wf.Steps) == 0 { return "", fmt.Errorf("workflow has no steps") } + // Re-check ownership at trigger time — targets may predate validation or a + // server may have been removed since the workflow was saved. + if err := validateTargetServers(orgID, wf.TargetServerIDs); err != nil { + return "", err + } // Reject a concurrent run of the same workflow. ctx, cancel := wfCtx() diff --git a/server/internal/services/workflows.go b/server/internal/services/workflows.go index f3f1820..01f7c8a 100644 --- a/server/internal/services/workflows.go +++ b/server/internal/services/workflows.go @@ -25,8 +25,13 @@ func EnsureWorkflowIndexes() error { }); err != nil { return err } + // The pre-multi-tenant index was on slug alone, so seeding defaults for a + // second org collided — drop it if a live DB still carries it. + if err := db.Col("workflow_steps").Indexes().DropOne(ctx, "slug_1"); err != nil && !isIndexNotFound(err) { + return err + } if _, err := db.Col("workflow_steps").Indexes().CreateOne(ctx, mongo.IndexModel{ - Keys: bson.D{{Key: "slug", Value: 1}}, + Keys: bson.D{{Key: "org_id", Value: 1}, {Key: "slug", Value: 1}}, Options: options.Index().SetUnique(true). SetPartialFilterExpression(bson.M{"source": "default"}), }); err != nil { @@ -219,6 +224,9 @@ func CreateWorkflow(orgID string, w models.Workflow) (*models.Workflow, error) { if err := ValidateWorkflow(w); err != nil { return nil, err } + if err := validateTargetServers(orgID, w.TargetServerIDs); err != nil { + return nil, err + } normalizeInlineSteps(&w) if _, err := db.Col("workflows").InsertOne(ctx, w); err != nil { return nil, err @@ -232,6 +240,9 @@ func UpdateWorkflow(orgID, id string, w models.Workflow) error { if err := ValidateWorkflow(w); err != nil { return err } + if err := validateTargetServers(orgID, w.TargetServerIDs); err != nil { + return err + } normalizeInlineSteps(&w) _, err := db.Col("workflows").UpdateOne(ctx, bson.M{"workflow_id": id, "org_id": orgID}, bson.M{"$set": bson.M{ "name": w.Name, @@ -242,6 +253,18 @@ func UpdateWorkflow(orgID, id string, w models.Workflow) error { return err } +// validateTargetServers rejects any target server that does not belong to the +// org. The IDs are client-supplied and are later consumed by the runner's +// unscoped lookups, so ownership has to be proven at the write boundary. +func validateTargetServers(orgID string, serverIDs []string) error { + for _, sid := range serverIDs { + if _, err := GetServer(orgID, sid); err != nil { + return fmt.Errorf("target server %s not found", sid) + } + } + return nil +} + // normalizeInlineSteps derives outputs for inline steps and strips fields that // only belong to library steps. func normalizeInlineSteps(w *models.Workflow) {