From cbf46a837b1fd019e94d535481d4f7b6bc029756 Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Wed, 15 Apr 2026 09:39:09 -0700 Subject: [PATCH] =?UTF-8?q?fix(auth):=20#138=20=E2=80=94=20field-level=20a?= =?UTF-8?q?uthz=20on=20PATCH=20/workspaces/:id?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #138. #125 moved PATCH /workspaces/:id into the wsAdmin AdminAuth group to close the #120 unauth vulnerability, but broke canvas drag- reposition and inline rename because canvas uses session cookies not bearer tokens. Multi-tenant deployments with any live token would have seen every canvas PATCH 401. Option A per #138 triage: PATCH goes back on the open router, but WorkspaceHandler.Update now enforces field-level authz: Cosmetic (no bearer required): name, role, x, y, canvas Sensitive (bearer required when any live token exists): tier — resource escalation parent_id — A2A hierarchy manipulation runtime — container image swap workspace_dir — host bind-mount redirection Fail-open bootstrap: HasAnyLiveTokenGlobal = 0 → pass-through (fresh install, pre-Phase-30 upgrade path). Matches the same lazy-bootstrap contract WorkspaceAuth and AdminAuth use elsewhere. 3 new tests cover all three branches of the matrix (cosmetic no-bearer, sensitive no-bearer-rejected, sensitive fail-open). Co-Authored-By: Claude Opus 4.6 (1M context) --- platform/internal/handlers/workspace.go | 43 ++++++++++ platform/internal/handlers/workspace_test.go | 85 ++++++++++++++++++++ platform/internal/router/router.go | 20 +++-- 3 files changed, 140 insertions(+), 8 deletions(-) diff --git a/platform/internal/handlers/workspace.go b/platform/internal/handlers/workspace.go index 54a2a9a3..84b23161 100644 --- a/platform/internal/handlers/workspace.go +++ b/platform/internal/handlers/workspace.go @@ -390,6 +390,20 @@ func (h *WorkspaceHandler) State(c *gin.Context) { }) } +// sensitiveUpdateFields gates the #120/#138 field-level auth check inside +// Update. Any key in this set requires a valid bearer token even when the +// rest of the route is open — tier is a resource-escalation vector, +// parent_id rewrites the A2A hierarchy, runtime swaps the container image +// on next restart, workspace_dir redirects host bind-mounts. Cosmetic +// fields (name, role, x, y, canvas) do not appear here and pass through +// unauthenticated so canvas drag-reposition and inline rename keep working. +var sensitiveUpdateFields = map[string]struct{}{ + "tier": {}, + "parent_id": {}, + "runtime": {}, + "workspace_dir": {}, +} + // Update handles PATCH /workspaces/:id func (h *WorkspaceHandler) Update(c *gin.Context) { id := c.Param("id") @@ -402,6 +416,35 @@ func (h *WorkspaceHandler) Update(c *gin.Context) { ctx := c.Request.Context() + // #138 field-level authz: PATCH /workspaces/:id is on the open router so + // canvas drag-reposition (cookie-based, no bearer token) keeps working, + // BUT the sensitive fields below require a valid bearer via the usual + // admin-token check. Lazy-bootstrap: if no live admin tokens exist at all + // (fresh install) the check is a no-op and everyone passes through. + for field := range body { + if _, sensitive := sensitiveUpdateFields[field]; !sensitive { + continue + } + hasLive, hlErr := wsauth.HasAnyLiveTokenGlobal(ctx, db.DB) + if hlErr != nil { + log.Printf("wsauth: Update HasAnyLiveTokenGlobal failed: %v — allowing request", hlErr) + break + } + if !hasLive { + break // fresh install — fail-open + } + tok := wsauth.BearerTokenFromHeader(c.GetHeader("Authorization")) + if tok == "" { + c.JSON(http.StatusUnauthorized, gin.H{"error": "admin auth required for field: " + field}) + return + } + if err := wsauth.ValidateAnyToken(ctx, db.DB, tok); err != nil { + c.JSON(http.StatusUnauthorized, gin.H{"error": "invalid admin auth token"}) + return + } + break // one successful validation covers the whole body + } + // #120: guard — return 404 for nonexistent workspace IDs instead of // silently applying zero-row UPDATEs and returning 200. var exists bool diff --git a/platform/internal/handlers/workspace_test.go b/platform/internal/handlers/workspace_test.go index a77851f1..924621f9 100644 --- a/platform/internal/handlers/workspace_test.go +++ b/platform/internal/handlers/workspace_test.go @@ -656,3 +656,88 @@ func TestWorkspaceState_ValidTokenReturnsStatus(t *testing.T) { t.Errorf("status should be 'degraded', got %v", body["status"]) } } + +// ── #138 field-level auth tests ───────────────────────────────────────────── +// Cosmetic PATCH (name/x/y/role) stays open so canvas drag-reposition works +// without a bearer token. Sensitive fields (tier/parent_id/runtime/ +// workspace_dir) require a valid admin bearer once any live token exists. + +func TestWorkspaceUpdate_CosmeticField_NoBearer_FailOpen_NoTokens(t *testing.T) { + mock := setupTestDB(t) + setupTestRedis(t) + broadcaster := newTestBroadcaster() + handler := NewWorkspaceHandler(broadcaster, nil, "http://localhost:8080", t.TempDir()) + + // Body contains only cosmetic field → no wsauth probe ever fires. + mock.ExpectQuery("SELECT EXISTS.*workspaces WHERE id"). + WithArgs("ws-cosmetic"). + WillReturnRows(sqlmock.NewRows([]string{"exists"}).AddRow(true)) + mock.ExpectExec("UPDATE workspaces SET name"). + WithArgs("ws-cosmetic", "Cosmetic"). + WillReturnResult(sqlmock.NewResult(0, 1)) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: "ws-cosmetic"}} + c.Request = httptest.NewRequest("PATCH", "/workspaces/ws-cosmetic", + bytes.NewBufferString(`{"name":"Cosmetic"}`)) + c.Request.Header.Set("Content-Type", "application/json") + handler.Update(c) + + if w.Code != http.StatusOK { + t.Errorf("cosmetic PATCH (no bearer) should pass; got %d: %s", w.Code, w.Body.String()) + } +} + +func TestWorkspaceUpdate_SensitiveField_NoBearer_TokensExist_Rejected(t *testing.T) { + mock := setupTestDB(t) + setupTestRedis(t) + broadcaster := newTestBroadcaster() + handler := NewWorkspaceHandler(broadcaster, nil, "http://localhost:8080", t.TempDir()) + + // HasAnyLiveTokenGlobal returns 1 — tokens exist on the platform. + mock.ExpectQuery("SELECT COUNT.*FROM workspace_auth_tokens"). + WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(1)) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: "ws-sensitive"}} + c.Request = httptest.NewRequest("PATCH", "/workspaces/ws-sensitive", + bytes.NewBufferString(`{"tier":4}`)) + c.Request.Header.Set("Content-Type", "application/json") + // No Authorization header — must fail closed. + handler.Update(c) + + if w.Code != http.StatusUnauthorized { + t.Errorf("sensitive PATCH without bearer: got %d, want 401 (%s)", w.Code, w.Body.String()) + } +} + +func TestWorkspaceUpdate_SensitiveField_NoTokensYet_FailOpen(t *testing.T) { + mock := setupTestDB(t) + setupTestRedis(t) + broadcaster := newTestBroadcaster() + handler := NewWorkspaceHandler(broadcaster, nil, "http://localhost:8080", t.TempDir()) + + // HasAnyLiveTokenGlobal returns 0 — fresh install, fail-open. + mock.ExpectQuery("SELECT COUNT.*FROM workspace_auth_tokens"). + WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(0)) + mock.ExpectQuery("SELECT EXISTS.*workspaces WHERE id"). + WithArgs("ws-bootstrap"). + WillReturnRows(sqlmock.NewRows([]string{"exists"}).AddRow(true)) + mock.ExpectExec("UPDATE workspaces SET tier"). + WithArgs("ws-bootstrap", float64(4)). + WillReturnResult(sqlmock.NewResult(0, 1)) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: "ws-bootstrap"}} + c.Request = httptest.NewRequest("PATCH", "/workspaces/ws-bootstrap", + bytes.NewBufferString(`{"tier":4}`)) + c.Request.Header.Set("Content-Type", "application/json") + handler.Update(c) + + if w.Code != http.StatusOK { + t.Errorf("bootstrap fail-open: got %d, want 200 (%s)", w.Code, w.Body.String()) + } +} diff --git a/platform/internal/router/router.go b/platform/internal/router/router.go index 613a5020..47c5a6c0 100644 --- a/platform/internal/router/router.go +++ b/platform/internal/router/router.go @@ -99,22 +99,26 @@ func Setup(hub *ws.Hub, broadcaster *events.Broadcaster, prov *provisioner.Provi // without a token (used by WorkspaceNode polling and health checks). r.GET("/workspaces/:id", wh.Get) - // C1 + C20 + C18-adjacent + #120: workspace list and ALL mutating operations - // gated behind AdminAuth — any valid workspace bearer token grants access. + // PATCH /workspaces/:id — back on the open router per #138. Canvas + // drag-reposition uses session cookies not bearer tokens; gating the + // whole route behind AdminAuth broke drag-to-reposition and inline + // rename. Field-level authz lives inside WorkspaceHandler.Update: + // - {x, y, canvas} only → passthrough (canvas position persist) + // - name / role → passthrough (inline rename) + // - tier / parent_id / runtime / workspace_dir → require bearer token + // The #120 escalation vectors stay locked; only cosmetic fields are open. + r.PATCH("/workspaces/:id", wh.Update) + + // C1 + C20: workspace list and life-cycle mutations gated behind AdminAuth. // Fail-open when no tokens exist anywhere (fresh install / pre-Phase-30). - // This blocks: + // Blocks: // 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) }