Merge pull request #692 from Molecule-AI/fix/issue-680-681-workspace-auth
fix(security): auth+ownership on PATCH /workspaces/:id (#680 #681)
This commit is contained in:
molecule-ai[bot]
GitHub
commit
92a28341fb
@ -13,7 +13,6 @@ import (
|
||||
"github.com/Molecule-AI/molecule-monorepo/platform/internal/crypto"
|
||||
"github.com/Molecule-AI/molecule-monorepo/platform/internal/db"
|
||||
"github.com/Molecule-AI/molecule-monorepo/platform/internal/events"
|
||||
"github.com/Molecule-AI/molecule-monorepo/platform/internal/middleware"
|
||||
"github.com/Molecule-AI/molecule-monorepo/platform/internal/models"
|
||||
"github.com/Molecule-AI/molecule-monorepo/platform/internal/provisioner"
|
||||
"github.com/Molecule-AI/molecule-monorepo/platform/internal/wsauth"
|
||||
@ -513,22 +512,19 @@ 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.
|
||||
// sensitiveUpdateFields documents fields that carry elevated risk — kept as
|
||||
// an explicit list for code readability and future audits. Auth is now fully
|
||||
// enforced at the router layer (WorkspaceAuth middleware, #680 IDOR fix);
|
||||
// this map is no longer used for in-handler gate logic but is preserved to
|
||||
// surface the risk classification clearly.
|
||||
//
|
||||
// budget_limit is intentionally NOT here — the dedicated PATCH
|
||||
// /workspaces/:id/budget (AdminAuth) is the only write path (#611).
|
||||
var sensitiveUpdateFields = map[string]struct{}{
|
||||
"tier": {},
|
||||
"parent_id": {},
|
||||
"runtime": {},
|
||||
"workspace_dir": {},
|
||||
// budget_limit is intentionally NOT here. The dedicated
|
||||
// PATCH /workspaces/:id/budget (AdminAuth) is the only write path.
|
||||
// Accepting it here — even behind ValidateAnyToken — lets workspace agents
|
||||
// self-clear their own spending ceiling. (#611 Security Auditor finding)
|
||||
}
|
||||
|
||||
// Update handles PATCH /workspaces/:id
|
||||
@ -543,37 +539,11 @@ 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 == "" {
|
||||
if middleware.IsSameOriginCanvas(c) {
|
||||
break // tenant canvas — trusted same-origin
|
||||
}
|
||||
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
|
||||
}
|
||||
// Auth is fully enforced at the router layer (WorkspaceAuth middleware, #680).
|
||||
// WorkspaceAuth validates that the caller holds a valid bearer token for this
|
||||
// specific workspace — no additional auth gate is needed here. The
|
||||
// sensitiveUpdateFields map above documents the risk classification for
|
||||
// auditors but is no longer used as a runtime gate.
|
||||
|
||||
// #120: guard — return 404 for nonexistent workspace IDs instead of
|
||||
// silently applying zero-row UPDATEs and returning 200.
|
||||
|
||||
@ -781,13 +781,15 @@ func TestWorkspaceState_ValidTokenReturnsStatus(t *testing.T) {
|
||||
// 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) {
|
||||
// TestWorkspaceUpdate_CosmeticField_Passthrough verifies that a cosmetic-field
|
||||
// PATCH (name, role, x, y) is processed by the handler without any DB auth query.
|
||||
// Auth is fully enforced by WorkspaceAuth middleware before the handler runs (#680).
|
||||
func TestWorkspaceUpdate_CosmeticField_Passthrough(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))
|
||||
@ -804,60 +806,44 @@ func TestWorkspaceUpdate_CosmeticField_NoBearer_FailOpen_NoTokens(t *testing.T)
|
||||
handler.Update(c)
|
||||
|
||||
if w.Code != http.StatusOK {
|
||||
t.Errorf("cosmetic PATCH (no bearer) should pass; got %d: %s", w.Code, w.Body.String())
|
||||
t.Errorf("cosmetic PATCH: got %d, want 200: %s", w.Code, w.Body.String())
|
||||
}
|
||||
}
|
||||
|
||||
func TestWorkspaceUpdate_SensitiveField_NoBearer_TokensExist_Rejected(t *testing.T) {
|
||||
// TestWorkspaceUpdate_SensitiveField_AuthEnforcedByMiddleware documents the #680 fix:
|
||||
// auth for PATCH /workspaces/:id is now enforced by WorkspaceAuth middleware (router
|
||||
// layer), not inside the handler. The handler processes sensitive fields (tier,
|
||||
// parent_id, runtime, workspace_dir) directly — WorkspaceAuth has already verified
|
||||
// the caller holds a valid bearer token for this specific workspace before the handler
|
||||
// runs. No in-handler wsauth DB probe fires.
|
||||
func TestWorkspaceUpdate_SensitiveField_AuthEnforcedByMiddleware(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))
|
||||
// No workspace_auth_tokens query expected — auth is middleware's responsibility.
|
||||
mock.ExpectQuery("SELECT EXISTS.*workspaces WHERE id").
|
||||
WithArgs("ws-bootstrap").
|
||||
WithArgs("ws-owned").
|
||||
WillReturnRows(sqlmock.NewRows([]string{"exists"}).AddRow(true))
|
||||
mock.ExpectExec("UPDATE workspaces SET tier").
|
||||
WithArgs("ws-bootstrap", float64(4)).
|
||||
WithArgs("ws-owned", float64(3)).
|
||||
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.Params = gin.Params{{Key: "id", Value: "ws-owned"}}
|
||||
c.Request = httptest.NewRequest("PATCH", "/workspaces/ws-owned",
|
||||
bytes.NewBufferString(`{"tier":3}`))
|
||||
c.Request.Header.Set("Content-Type", "application/json")
|
||||
// WorkspaceAuth middleware would have validated the bearer before this runs.
|
||||
handler.Update(c)
|
||||
|
||||
if w.Code != http.StatusOK {
|
||||
t.Errorf("bootstrap fail-open: got %d, want 200 (%s)", w.Code, w.Body.String())
|
||||
t.Errorf("sensitive PATCH (auth at middleware): got %d, want 200: %s", w.Code, w.Body.String())
|
||||
}
|
||||
if err := mock.ExpectationsWereMet(); err != nil {
|
||||
t.Errorf("unmet sqlmock expectations: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@ -110,16 +110,6 @@ 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)
|
||||
|
||||
// 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).
|
||||
// Blocks:
|
||||
@ -142,6 +132,13 @@ func Setup(hub *ws.Hub, broadcaster *events.Broadcaster, prov *provisioner.Provi
|
||||
// Legacy workspaces (no token) are grandfathered to allow rolling upgrades.
|
||||
wsAuth := r.Group("/workspaces/:id", middleware.WorkspaceAuth(db.DB))
|
||||
{
|
||||
// #680: PATCH /workspaces/:id moved under WorkspaceAuth (#680 IDOR fix).
|
||||
// WorkspaceAuth enforces that the caller holds a valid bearer token for
|
||||
// this specific workspace — both auth AND ownership in one check. Cosmetic
|
||||
// updates (x/y drag-reposition, inline rename) from the combined tenant
|
||||
// image canvas still pass via the isSameOriginCanvas bypass in WorkspaceAuth.
|
||||
wsAuth.PATCH("", wh.Update)
|
||||
|
||||
// Lifecycle
|
||||
wsAuth.GET("/state", wh.State)
|
||||
wsAuth.POST("/restart", wh.Restart)
|
||||
|
||||
Loading…
Reference in New Issue
Block a user