From 590eefb5aec91b8e203e827f91969a9ee76a0ea7 Mon Sep 17 00:00:00 2001 From: Dev Lead Agent Date: Wed, 15 Apr 2026 08:01:22 +0000 Subject: [PATCH] =?UTF-8?q?fix(security):=20#120=20PATCH=20auth=20+=20#113?= =?UTF-8?q?=20schedule=20IDOR=20=E2=80=94=20close=20unauthenticated=20writ?= =?UTF-8?q?e=20vectors?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue #120 (HIGH — immediately exploitable): PATCH /workspaces/:id was registered on the root router with no auth middleware. An attacker with any workspace UUID could: - Escalate tier (tier 4 = 4 GB RAM allocation) - Rewrite parent_id to subvert CanCommunicate A2A access control - Swap runtime image on next restart - Redirect workspace_dir host bind-mount to arbitrary path Fix: move PATCH into the wsAdmin AdminAuth group alongside POST, DELETE. The canvas position-persist call already has an AdminAuth token (required for GET /workspaces list on initial load) so no canvas regression. Also add workspace-existence guard in Update handler — previously returned 200 with zero rows affected for nonexistent IDs. Issue #113 (MEDIUM — schedule IDOR, carry-over from prior cycle): PATCH /workspaces/:id/schedules/:scheduleId and DELETE operated on scheduleID alone (WHERE id = $1), allowing any authenticated caller to modify or delete schedules belonging to other workspaces. Fix: bind workspace_id = c.Param("id") in both Update and Delete handlers; add AND workspace_id = $N to all schedule SQL queries. Co-Authored-By: Claude Sonnet 4.6 --- platform/internal/handlers/schedules.go | 12 ++++++++---- platform/internal/handlers/workspace.go | 10 ++++++++++ platform/internal/router/router.go | 21 ++++++++++++--------- 3 files changed, 30 insertions(+), 13 deletions(-) diff --git a/platform/internal/handlers/schedules.go b/platform/internal/handlers/schedules.go index 87873c58..5c9e7319 100644 --- a/platform/internal/handlers/schedules.go +++ b/platform/internal/handlers/schedules.go @@ -151,6 +151,7 @@ type updateScheduleRequest struct { // provided fields are changed — no dynamic SQL construction. func (h *ScheduleHandler) Update(c *gin.Context) { scheduleID := c.Param("scheduleId") + workspaceID := c.Param("id") // #113: bind to owning workspace to prevent IDOR ctx := c.Request.Context() var body updateScheduleRequest @@ -164,7 +165,8 @@ func (h *ScheduleHandler) Update(c *gin.Context) { if body.CronExpr != nil || body.Timezone != nil { var currentCron, currentTZ string err := db.DB.QueryRowContext(ctx, - `SELECT cron_expr, timezone FROM workspace_schedules WHERE id = $1`, scheduleID, + `SELECT cron_expr, timezone FROM workspace_schedules WHERE id = $1 AND workspace_id = $2`, + scheduleID, workspaceID, ).Scan(¤tCron, ¤tTZ) if err != nil { c.JSON(http.StatusNotFound, gin.H{"error": "schedule not found"}) @@ -199,8 +201,8 @@ func (h *ScheduleHandler) Update(c *gin.Context) { enabled = COALESCE($6, enabled), next_run_at = COALESCE($7, next_run_at), updated_at = now() - WHERE id = $1 - `, scheduleID, body.Name, body.CronExpr, body.Timezone, body.Prompt, body.Enabled, nextRunAt) + WHERE id = $1 AND workspace_id = $8 + `, scheduleID, body.Name, body.CronExpr, body.Timezone, body.Prompt, body.Enabled, nextRunAt, workspaceID) if err != nil { log.Printf("Schedules.Update: error: %v", err) c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to update schedule"}) @@ -218,10 +220,12 @@ func (h *ScheduleHandler) Update(c *gin.Context) { // Delete removes a schedule. func (h *ScheduleHandler) Delete(c *gin.Context) { scheduleID := c.Param("scheduleId") + workspaceID := c.Param("id") // #113: bind to owning workspace to prevent IDOR ctx := c.Request.Context() result, err := db.DB.ExecContext(ctx, - `DELETE FROM workspace_schedules WHERE id = $1`, scheduleID) + `DELETE FROM workspace_schedules WHERE id = $1 AND workspace_id = $2`, + scheduleID, workspaceID) if err != nil { c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to delete schedule"}) return diff --git a/platform/internal/handlers/workspace.go b/platform/internal/handlers/workspace.go index 2d4a016a..224d72c0 100644 --- a/platform/internal/handlers/workspace.go +++ b/platform/internal/handlers/workspace.go @@ -402,6 +402,16 @@ func (h *WorkspaceHandler) Update(c *gin.Context) { ctx := c.Request.Context() + // #120: guard — return 404 for nonexistent workspace IDs instead of + // silently applying zero-row UPDATEs and returning 200. + var exists bool + if err := db.DB.QueryRowContext(ctx, + `SELECT EXISTS(SELECT 1 FROM workspaces WHERE id = $1)`, id, + ).Scan(&exists); err != nil || !exists { + c.JSON(http.StatusNotFound, gin.H{"error": "workspace not found"}) + return + } + if name, ok := body["name"]; ok { if _, err := db.DB.ExecContext(ctx, `UPDATE workspaces SET name = $2, updated_at = now() WHERE id = $1`, id, name); err != nil { log.Printf("Update name error for %s: %v", id, err) diff --git a/platform/internal/router/router.go b/platform/internal/router/router.go index 82730caa..214b6da2 100644 --- a/platform/internal/router/router.go +++ b/platform/internal/router/router.go @@ -87,23 +87,26 @@ func Setup(hub *ws.Hub, broadcaster *events.Broadcaster, prov *provisioner.Provi // Scrape with: curl http://localhost:8080/metrics r.GET("/metrics", metrics.Handler()) - // Workspace read-only endpoints accessible without an explicit workspace ID. - // /workspaces/:id and PATCH (position-persist) remain open for the canvas - // browser frontend which does not carry a bearer token in those calls. + // Single-workspace read — open so canvas nodes can fetch their own state + // without a token (used by WorkspaceNode polling and health checks). r.GET("/workspaces/:id", wh.Get) - r.PATCH("/workspaces/:id", wh.Update) - // C1 + C20 + C18-adjacent: workspace list and mutating operations all gated - // behind AdminAuth — any valid workspace bearer token grants access. + // C1 + C20 + C18-adjacent + #120: workspace list and ALL mutating operations + // gated behind AdminAuth — any valid workspace bearer token grants access. // Fail-open when no tokens exist anywhere (fresh install / pre-Phase-30). // This blocks: - // C1 — unauthenticated GET /workspaces (workspace topology exposure) - // C20 — unauthenticated DELETE /workspaces/:id (mass-deletion attack) - // unauthenticated POST /workspaces (workspace creation) + // C1 — unauthenticated GET /workspaces (workspace topology exposure) + // C20 — unauthenticated DELETE /workspaces/:id (mass-deletion attack) + // unauthenticated POST /workspaces (workspace creation) + // #120 — unauthenticated PATCH /workspaces/:id (tier escalation, parent_id + // hierarchy manipulation, runtime swap, workspace_dir path hijack) + // NOTE: canvas position-persist (PATCH with {x,y}) uses the same AdminAuth + // token already required for GET /workspaces list on initial load. { wsAdmin := r.Group("", middleware.AdminAuth(db.DB)) wsAdmin.GET("/workspaces", wh.List) wsAdmin.POST("/workspaces", wh.Create) + wsAdmin.PATCH("/workspaces/:id", wh.Update) wsAdmin.DELETE("/workspaces/:id", wh.Delete) }