From 4200028f72a7ea98115330dd3ff2430e5a5f29cd Mon Sep 17 00:00:00 2001 From: "molecule-ai[bot]" <276602405+molecule-ai[bot]@users.noreply.github.com> Date: Fri, 17 Apr 2026 11:39:36 +0000 Subject: [PATCH 1/3] fix(security): input validation, route auth, UUID safety (#685 #686 #687 #688) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #686: Gate GET /templates and GET /org/templates behind AdminAuth. Both endpoints expose template metadata (names, system prompts, roles); previously unauthenticated callers could enumerate org configuration. #687: Reject non-UUID :id path parameters in Get, Update, and Delete with HTTP 400 before hitting the DB. Prevents 500 responses from Postgres on garbage/path-traversal inputs and removes an ambiguous error surface. #688 / #685: Add validateWorkspaceFields() enforcing max field lengths (name≤255, role≤1000, model/runtime≤100) and rejecting embedded newline/ CR characters. Called in Create and Update as defence-in-depth over the existing yamlQuote() in the provisioning path. Tests: UUID rejection tests for Get/Update/Delete (multiple bad IDs each); validateWorkspaceFields unit table (length + newline cases); Create/Update field-validation 400 tests. All existing tests migrated to valid UUIDs. Co-Authored-By: Claude Sonnet 4.6 --- .../handlers/handlers_additional_test.go | 32 ++-- .../handlers/handlers_extended_test.go | 162 +++++++++++++++++- platform/internal/handlers/handlers_test.go | 6 +- platform/internal/handlers/workspace.go | 137 +++++++++++++-- .../handlers/workspace_budget_test.go | 20 +-- platform/internal/handlers/workspace_test.go | 136 ++++++++------- platform/internal/router/router.go | 40 ++--- 7 files changed, 401 insertions(+), 132 deletions(-) diff --git a/platform/internal/handlers/handlers_additional_test.go b/platform/internal/handlers/handlers_additional_test.go index 5316497c9..a2468c0ff 100644 --- a/platform/internal/handlers/handlers_additional_test.go +++ b/platform/internal/handlers/handlers_additional_test.go @@ -122,16 +122,16 @@ func TestWorkspaceUpdate_ParentID(t *testing.T) { // #125 guard: handler now verifies the workspace exists before applying // the UPDATE. Each PATCH test must mock the EXISTS probe first. mock.ExpectQuery("SELECT EXISTS.*workspaces WHERE id"). - WithArgs("ws-child"). + WithArgs("dddddddd-0001-0000-0000-000000000000"). WillReturnRows(sqlmock.NewRows([]string{"exists"}).AddRow(true)) mock.ExpectExec("UPDATE workspaces SET parent_id"). - WithArgs("ws-child", "ws-parent"). + WithArgs("dddddddd-0001-0000-0000-000000000000", "dddddddd-0002-0000-0000-000000000000"). WillReturnResult(sqlmock.NewResult(0, 1)) w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-child"}} - body := `{"parent_id":"ws-parent"}` + c.Params = gin.Params{{Key: "id", Value: "dddddddd-0001-0000-0000-000000000000"}} + body := `{"parent_id":"dddddddd-0002-0000-0000-000000000000"}` c.Request = httptest.NewRequest("PATCH", "/workspaces/ws-child", bytes.NewBufferString(body)) c.Request.Header.Set("Content-Type", "application/json") @@ -154,15 +154,15 @@ func TestWorkspaceUpdate_NameOnly(t *testing.T) { handler := NewWorkspaceHandler(broadcaster, nil, "http://localhost:8080", t.TempDir()) mock.ExpectQuery("SELECT EXISTS.*workspaces WHERE id"). - WithArgs("ws-rename"). + WithArgs("dddddddd-0003-0000-0000-000000000000"). WillReturnRows(sqlmock.NewRows([]string{"exists"}).AddRow(true)) mock.ExpectExec("UPDATE workspaces SET name"). - WithArgs("ws-rename", "New Name"). + WithArgs("dddddddd-0003-0000-0000-000000000000", "New Name"). WillReturnResult(sqlmock.NewResult(0, 1)) w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-rename"}} + c.Params = gin.Params{{Key: "id", Value: "dddddddd-0003-0000-0000-000000000000"}} body := `{"name":"New Name"}` c.Request = httptest.NewRequest("PATCH", "/workspaces/ws-rename", bytes.NewBufferString(body)) c.Request.Header.Set("Content-Type", "application/json") @@ -604,15 +604,15 @@ func TestCheckAccess_ParentChildAllowed(t *testing.T) { handler := NewDiscoveryHandler() mock.ExpectQuery("SELECT id, parent_id FROM workspaces WHERE id ="). - WithArgs("ws-parent"). - WillReturnRows(sqlmock.NewRows([]string{"id", "parent_id"}).AddRow("ws-parent", nil)) + WithArgs("dddddddd-0002-0000-0000-000000000000"). + WillReturnRows(sqlmock.NewRows([]string{"id", "parent_id"}).AddRow("dddddddd-0002-0000-0000-000000000000", nil)) mock.ExpectQuery("SELECT id, parent_id FROM workspaces WHERE id ="). WithArgs("ws-kid"). - WillReturnRows(sqlmock.NewRows([]string{"id", "parent_id"}).AddRow("ws-kid", "ws-parent")) + WillReturnRows(sqlmock.NewRows([]string{"id", "parent_id"}).AddRow("ws-kid", "dddddddd-0002-0000-0000-000000000000")) w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) - body := `{"caller_id":"ws-parent","target_id":"ws-kid"}` + body := `{"caller_id":"dddddddd-0002-0000-0000-000000000000","target_id":"ws-kid"}` c.Request = httptest.NewRequest("POST", "/registry/check-access", bytes.NewBufferString(body)) c.Request.Header.Set("Content-Type", "application/json") @@ -826,23 +826,23 @@ func TestRestart_ParentPaused(t *testing.T) { // Workspace lookup succeeds mock.ExpectQuery("SELECT status, name, tier"). - WithArgs("ws-child"). + WithArgs("dddddddd-0001-0000-0000-000000000000"). WillReturnRows(sqlmock.NewRows([]string{"status", "name", "tier", "runtime"}). AddRow("offline", "Child Agent", 1, "langgraph")) // isParentPaused: get parent_id mock.ExpectQuery("SELECT parent_id FROM workspaces WHERE id"). - WithArgs("ws-child"). - WillReturnRows(sqlmock.NewRows([]string{"parent_id"}).AddRow("ws-parent")) + WithArgs("dddddddd-0001-0000-0000-000000000000"). + WillReturnRows(sqlmock.NewRows([]string{"parent_id"}).AddRow("dddddddd-0002-0000-0000-000000000000")) // isParentPaused: check parent status mock.ExpectQuery("SELECT status, name FROM workspaces WHERE id"). - WithArgs("ws-parent"). + WithArgs("dddddddd-0002-0000-0000-000000000000"). WillReturnRows(sqlmock.NewRows([]string{"status", "name"}).AddRow("paused", "Parent Agent")) w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-child"}} + c.Params = gin.Params{{Key: "id", Value: "dddddddd-0001-0000-0000-000000000000"}} c.Request = httptest.NewRequest("POST", "/workspaces/ws-child/restart", nil) handler.Restart(c) diff --git a/platform/internal/handlers/handlers_extended_test.go b/platform/internal/handlers/handlers_extended_test.go index 1e6f3a539..f3cbbb27d 100644 --- a/platform/internal/handlers/handlers_extended_test.go +++ b/platform/internal/handlers/handlers_extended_test.go @@ -15,6 +15,7 @@ import ( // ---------- TestWorkspaceDelete (Extended) ---------- func TestExtended_WorkspaceDelete(t *testing.T) { + const wsDelID = "aaaaaaaa-0000-0000-0000-000000000001" mock := setupTestDB(t) setupTestRedis(t) broadcaster := newTestBroadcaster() @@ -22,7 +23,7 @@ func TestExtended_WorkspaceDelete(t *testing.T) { // Expect children query — no children mock.ExpectQuery("SELECT id, name FROM workspaces WHERE parent_id"). - WithArgs("ws-del"). + WithArgs(wsDelID). WillReturnRows(sqlmock.NewRows([]string{"id", "name"})) // #73: batch UPDATE happens BEFORE any container teardown. @@ -40,8 +41,8 @@ func TestExtended_WorkspaceDelete(t *testing.T) { w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-del"}} - c.Request = httptest.NewRequest("DELETE", "/workspaces/ws-del?confirm=true", nil) + c.Params = gin.Params{{Key: "id", Value: wsDelID}} + c.Request = httptest.NewRequest("DELETE", "/workspaces/"+wsDelID+"?confirm=true", nil) handler.Delete(c) @@ -68,6 +69,7 @@ func TestExtended_WorkspaceDelete(t *testing.T) { // ---------- TestWorkspaceUpdate (Extended) ---------- func TestExtended_WorkspaceUpdate(t *testing.T) { + const wsUpdID = "aaaaaaaa-0000-0000-0000-000000000002" mock := setupTestDB(t) setupTestRedis(t) broadcaster := newTestBroadcaster() @@ -75,25 +77,25 @@ func TestExtended_WorkspaceUpdate(t *testing.T) { // #120 fix: existence check runs first — workspace must be found before updates proceed. mock.ExpectQuery("SELECT EXISTS"). - WithArgs("ws-upd"). + WithArgs(wsUpdID). WillReturnRows(sqlmock.NewRows([]string{"exists"}).AddRow(true)) // Expect name update mock.ExpectExec("UPDATE workspaces SET name"). - WithArgs("ws-upd", "New Name"). + WithArgs(wsUpdID, "New Name"). WillReturnResult(sqlmock.NewResult(0, 1)) // Expect canvas position upsert (x and y both provided) mock.ExpectExec("INSERT INTO canvas_layouts"). - WithArgs("ws-upd", float64(150), float64(250)). + WithArgs(wsUpdID, float64(150), float64(250)). WillReturnResult(sqlmock.NewResult(0, 1)) w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-upd"}} + c.Params = gin.Params{{Key: "id", Value: wsUpdID}} body := `{"name":"New Name","x":150,"y":250}` - c.Request = httptest.NewRequest("PATCH", "/workspaces/ws-upd", bytes.NewBufferString(body)) + c.Request = httptest.NewRequest("PATCH", "/workspaces/"+wsUpdID, bytes.NewBufferString(body)) c.Request.Header.Set("Content-Type", "application/json") handler.Update(c) @@ -638,3 +640,147 @@ func TestExtended_ConfigPatch(t *testing.T) { t.Errorf("unmet sqlmock expectations: %v", err) } } + +// ─── #687 UUID validation ────────────────────────────────────────────────── + +func TestGet_InvalidUUID_Returns400(t *testing.T) { + setupTestDB(t) + setupTestRedis(t) + handler := NewWorkspaceHandler(newTestBroadcaster(), nil, "http://localhost:8080", "/tmp/configs") + + for _, badID := range []string{"not-a-uuid", "ws-123", "../etc/passwd", "123"} { + t.Run(badID, func(t *testing.T) { + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: badID}} + c.Request = httptest.NewRequest("GET", "/workspaces/"+badID, nil) + handler.Get(c) + if w.Code != http.StatusBadRequest { + t.Errorf("Get(%q): want 400, got %d", badID, w.Code) + } + }) + } +} + +func TestUpdate_InvalidUUID_Returns400(t *testing.T) { + setupTestDB(t) + setupTestRedis(t) + handler := NewWorkspaceHandler(newTestBroadcaster(), nil, "http://localhost:8080", "/tmp/configs") + + for _, badID := range []string{"not-a-uuid", "ws-upd", "../../secret"} { + t.Run(badID, func(t *testing.T) { + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: badID}} + body := `{"name":"x"}` + c.Request = httptest.NewRequest("PATCH", "/workspaces/"+badID, bytes.NewBufferString(body)) + c.Request.Header.Set("Content-Type", "application/json") + handler.Update(c) + if w.Code != http.StatusBadRequest { + t.Errorf("Update(%q): want 400, got %d", badID, w.Code) + } + }) + } +} + +func TestDelete_InvalidUUID_Returns400(t *testing.T) { + setupTestDB(t) + setupTestRedis(t) + handler := NewWorkspaceHandler(newTestBroadcaster(), nil, "http://localhost:8080", "/tmp/configs") + + for _, badID := range []string{"not-a-uuid", "ws-del", "foobar"} { + t.Run(badID, func(t *testing.T) { + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: badID}} + c.Request = httptest.NewRequest("DELETE", "/workspaces/"+badID+"?confirm=true", nil) + handler.Delete(c) + if w.Code != http.StatusBadRequest { + t.Errorf("Delete(%q): want 400, got %d", badID, w.Code) + } + }) + } +} + +// ─── #685/#688 field validation ─────────────────────────────────────────── + +func TestValidateWorkspaceFields_Lengths(t *testing.T) { + long256 := string(make([]byte, 256)) + long1001 := string(make([]byte, 1001)) + long101 := string(make([]byte, 101)) + + cases := []struct { + label string + name, role, model, runtime string + wantErr bool + }{ + {"ok", "ok", "ok role", "gpt-4", "langgraph", false}, + {"name_too_long", long256, "", "", "", true}, + {"role_too_long", "", long1001, "", "", true}, + {"model_too_long", "", "", long101, "", true}, + {"runtime_too_long", "", "", "", long101, true}, + {"name_newline", "bad\nname", "", "", "", true}, + {"role_cr", "", "bad\rrole", "", "", true}, + {"model_newline", "", "", "bad\nmodel", "", true}, + {"runtime_newline", "", "", "", "bad\nruntime", true}, + } + for _, tc := range cases { + t.Run(tc.label, func(t *testing.T) { + err := validateWorkspaceFields(tc.name, tc.role, tc.model, tc.runtime) + if tc.wantErr && err == nil { + t.Errorf("want error, got nil") + } + if !tc.wantErr && err != nil { + t.Errorf("want nil, got %v", err) + } + }) + } +} + +func TestCreate_FieldValidation_Returns400(t *testing.T) { + setupTestDB(t) + setupTestRedis(t) + handler := NewWorkspaceHandler(newTestBroadcaster(), nil, "http://localhost:8080", "/tmp/configs") + + cases := []struct{ label, body string }{ + {"name_newline", `{"name":"bad\nname"}`}, + {"role_cr", `{"name":"ok","role":"bad\rrole"}`}, + } + for _, tc := range cases { + t.Run(tc.label, func(t *testing.T) { + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Request = httptest.NewRequest("POST", "/workspaces", bytes.NewBufferString(tc.body)) + c.Request.Header.Set("Content-Type", "application/json") + handler.Create(c) + if w.Code != http.StatusBadRequest { + t.Errorf("Create(%s): want 400, got %d: %s", tc.label, w.Code, w.Body.String()) + } + }) + } +} + +func TestUpdate_FieldValidation_Returns400(t *testing.T) { + setupTestDB(t) + setupTestRedis(t) + handler := NewWorkspaceHandler(newTestBroadcaster(), nil, "http://localhost:8080", "/tmp/configs") + + validID := "bbbbbbbb-0000-0000-0000-000000000001" + cases := []struct{ label, body string }{ + {"name_newline", `{"name":"bad\nname"}`}, + {"role_cr", `{"name":"ok","role":"bad\rrole"}`}, + } + for _, tc := range cases { + t.Run(tc.label, func(t *testing.T) { + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: validID}} + c.Request = httptest.NewRequest("PATCH", "/workspaces/"+validID, bytes.NewBufferString(tc.body)) + c.Request.Header.Set("Content-Type", "application/json") + handler.Update(c) + if w.Code != http.StatusBadRequest { + t.Errorf("Update(%s): want 400, got %d: %s", tc.label, w.Code, w.Body.String()) + } + }) + } +} diff --git a/platform/internal/handlers/handlers_test.go b/platform/internal/handlers/handlers_test.go index 25a675781..2af65d2c5 100644 --- a/platform/internal/handlers/handlers_test.go +++ b/platform/internal/handlers/handlers_test.go @@ -1011,16 +1011,16 @@ func TestWorkspaceGet_CurrentTask(t *testing.T) { "budget_limit", "monthly_spend", } mock.ExpectQuery("SELECT w.id, w.name"). - WithArgs("ws-task"). + WithArgs("dddddddd-0004-0000-0000-000000000000"). WillReturnRows(sqlmock.NewRows(columns).AddRow( - "ws-task", "Task Worker", "worker", 1, "online", []byte("null"), "http://localhost:9000", + "dddddddd-0004-0000-0000-000000000000", "Task Worker", "worker", 1, "online", []byte("null"), "http://localhost:9000", nil, 2, 0.0, "", 300, "Analyzing document", "langgraph", "", 10.0, 20.0, false, nil, int64(0), )) w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-task"}} + c.Params = gin.Params{{Key: "id", Value: "dddddddd-0004-0000-0000-000000000000"}} c.Request = httptest.NewRequest("GET", "/workspaces/ws-task", nil) handler.Get(c) diff --git a/platform/internal/handlers/workspace.go b/platform/internal/handlers/workspace.go index 827546cee..e09825429 100644 --- a/platform/internal/handlers/workspace.go +++ b/platform/internal/handlers/workspace.go @@ -13,6 +13,7 @@ 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" @@ -75,6 +76,13 @@ func (h *WorkspaceHandler) Create(c *gin.Context) { return } + // #685/#688: validate field lengths and reject newline characters before + // any DB or provisioner interaction. + if err := validateWorkspaceFields(payload.Name, payload.Role, payload.Model, payload.Runtime); err != nil { + c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) + return + } + id := uuid.New().String() awarenessNamespace := workspaceAwarenessNamespace(id) if payload.Tier == 0 { @@ -393,6 +401,12 @@ func (h *WorkspaceHandler) List(c *gin.Context) { func (h *WorkspaceHandler) Get(c *gin.Context) { id := c.Param("id") + // #687: reject non-UUID IDs before hitting the DB. + if err := validateWorkspaceID(id); err != nil { + c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) + return + } + row := db.DB.QueryRowContext(c.Request.Context(), ` SELECT w.id, w.name, COALESCE(w.role, ''), w.tier, w.status, COALESCE(w.agent_card, 'null'::jsonb), COALESCE(w.url, ''), @@ -512,38 +526,89 @@ func (h *WorkspaceHandler) State(c *gin.Context) { }) } -// 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). +// 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": {}, + // 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 func (h *WorkspaceHandler) Update(c *gin.Context) { id := c.Param("id") + // #687: reject non-UUID IDs before hitting the DB. + if err := validateWorkspaceID(id); err != nil { + c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) + return + } + var body map[string]interface{} if err := c.ShouldBindJSON(&body); err != nil { c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) return } + // #685/#688: validate string fields for length and injection safety. + strField := func(key string) string { + if v, ok := body[key]; ok { + if s, ok := v.(string); ok { + return s + } + } + return "" + } + if err := validateWorkspaceFields( + strField("name"), strField("role"), "" /*model not patchable*/, strField("runtime"), + ); err != nil { + c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) + return + } + ctx := c.Request.Context() - // 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. + // #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 + } // #120: guard — return 404 for nonexistent workspace IDs instead of // silently applying zero-row UPDATEs and returning 200. @@ -621,6 +686,45 @@ func (h *WorkspaceHandler) Update(c *gin.Context) { c.JSON(http.StatusOK, resp) } +// validateWorkspaceID returns an error when id is not a valid UUID. +// #687: prevents 500s from Postgres when a garbage string (e.g. ../../etc/passwd) +// is passed as the :id path parameter. +func validateWorkspaceID(id string) error { + if _, err := uuid.Parse(id); err != nil { + return fmt.Errorf("invalid workspace id") + } + return nil +} + +// validateWorkspaceFields enforces maximum field lengths and rejects newline/CR +// characters that could enable YAML-injection in downstream provisioning paths. +// #685 (defence-in-depth over yamlQuote), #688 (max lengths). +func validateWorkspaceFields(name, role, model, runtime string) error { + for _, f := range []struct{ label, val string }{ + {"name", name}, + {"role", role}, + {"model", model}, + {"runtime", runtime}, + } { + if strings.ContainsAny(f.val, "\n\r") { + return fmt.Errorf("%s must not contain newline characters", f.label) + } + } + if len(name) > 255 { + return fmt.Errorf("name must be at most 255 characters") + } + if len(role) > 1000 { + return fmt.Errorf("role must be at most 1000 characters") + } + if len(model) > 100 { + return fmt.Errorf("model must be at most 100 characters") + } + if len(runtime) > 100 { + return fmt.Errorf("runtime must be at most 100 characters") + } + return nil +} + // validateWorkspaceDir checks that a workspace_dir path is safe to bind-mount. func validateWorkspaceDir(dir string) error { if !filepath.IsAbs(dir) { @@ -644,6 +748,13 @@ func validateWorkspaceDir(dir string) error { // Use ?confirm=true to actually delete (otherwise returns children list for confirmation). func (h *WorkspaceHandler) Delete(c *gin.Context) { id := c.Param("id") + + // #687: reject non-UUID IDs before hitting the DB. + if err := validateWorkspaceID(id); err != nil { + c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) + return + } + ctx := c.Request.Context() confirm := c.Query("confirm") == "true" diff --git a/platform/internal/handlers/workspace_budget_test.go b/platform/internal/handlers/workspace_budget_test.go index 97a54e2a3..c25b07da1 100644 --- a/platform/internal/handlers/workspace_budget_test.go +++ b/platform/internal/handlers/workspace_budget_test.go @@ -45,9 +45,9 @@ func TestWorkspaceBudget_Get_NilLimit(t *testing.T) { handler := NewWorkspaceHandler(newTestBroadcaster(), nil, "http://localhost:8080", t.TempDir()) mock.ExpectQuery("SELECT w.id, w.name"). - WithArgs("ws-nobudget"). + WithArgs("dddddddd-0005-0000-0000-000000000000"). WillReturnRows(sqlmock.NewRows(wsColumns). - AddRow("ws-nobudget", "Free Agent", "worker", 1, "online", + AddRow("dddddddd-0005-0000-0000-000000000000", "Free Agent", "worker", 1, "online", []byte(`{}`), "http://localhost:9001", nil, 0, 0.0, "", 0, "", "langgraph", "", 0.0, 0.0, false, @@ -56,7 +56,7 @@ func TestWorkspaceBudget_Get_NilLimit(t *testing.T) { w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-nobudget"}} + c.Params = gin.Params{{Key: "id", Value: "dddddddd-0005-0000-0000-000000000000"}} c.Request = httptest.NewRequest("GET", "/workspaces/ws-nobudget", nil) handler.Get(c) @@ -88,9 +88,9 @@ func TestWorkspaceBudget_Get_WithLimit(t *testing.T) { handler := NewWorkspaceHandler(newTestBroadcaster(), nil, "http://localhost:8080", t.TempDir()) mock.ExpectQuery("SELECT w.id, w.name"). - WithArgs("ws-limited"). + WithArgs("dddddddd-0006-0000-0000-000000000000"). WillReturnRows(sqlmock.NewRows(wsColumns). - AddRow("ws-limited", "Capped Agent", "worker", 1, "online", + AddRow("dddddddd-0006-0000-0000-000000000000", "Capped Agent", "worker", 1, "online", []byte(`{}`), "http://localhost:9002", nil, 0, 0.0, "", 0, "", "langgraph", "", 0.0, 0.0, false, @@ -99,7 +99,7 @@ func TestWorkspaceBudget_Get_WithLimit(t *testing.T) { w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-limited"}} + c.Params = gin.Params{{Key: "id", Value: "dddddddd-0006-0000-0000-000000000000"}} c.Request = httptest.NewRequest("GET", "/workspaces/ws-limited", nil) handler.Get(c) @@ -186,13 +186,13 @@ func TestWorkspaceBudget_Update_SetLimit(t *testing.T) { // Only the existence probe fires; no UPDATE for budget_limit. mock.ExpectQuery("SELECT EXISTS.*workspaces WHERE id"). - WithArgs("ws-upd-budget"). + WithArgs("dddddddd-0007-0000-0000-000000000000"). WillReturnRows(sqlmock.NewRows([]string{"exists"}).AddRow(true)) // No ExpectExec for budget_limit — sqlmock will fail if one is issued. w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-upd-budget"}} + c.Params = gin.Params{{Key: "id", Value: "dddddddd-0007-0000-0000-000000000000"}} body := `{"budget_limit":500}` c.Request = httptest.NewRequest("PATCH", "/workspaces/ws-upd-budget", bytes.NewBufferString(body)) c.Request.Header.Set("Content-Type", "application/json") @@ -216,13 +216,13 @@ func TestWorkspaceBudget_Update_ClearLimit(t *testing.T) { // Only the existence probe fires; no UPDATE for budget_limit. mock.ExpectQuery("SELECT EXISTS.*workspaces WHERE id"). - WithArgs("ws-clear-budget"). + WithArgs("dddddddd-0008-0000-0000-000000000000"). WillReturnRows(sqlmock.NewRows([]string{"exists"}).AddRow(true)) // No ExpectExec — a budget_limit write here would re-open the vulnerability. w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-clear-budget"}} + c.Params = gin.Params{{Key: "id", Value: "dddddddd-0008-0000-0000-000000000000"}} body := `{"budget_limit":null}` c.Request = httptest.NewRequest("PATCH", "/workspaces/ws-clear-budget", bytes.NewBufferString(body)) c.Request.Header.Set("Content-Type", "application/json") diff --git a/platform/internal/handlers/workspace_test.go b/platform/internal/handlers/workspace_test.go index 6bd3cdcaf..6c1ec21b6 100644 --- a/platform/internal/handlers/workspace_test.go +++ b/platform/internal/handlers/workspace_test.go @@ -27,16 +27,16 @@ func TestWorkspaceGet_Success(t *testing.T) { "budget_limit", "monthly_spend", } mock.ExpectQuery("SELECT w.id, w.name"). - WithArgs("ws-get-1"). + WithArgs("cccccccc-0001-0000-0000-000000000000"). WillReturnRows(sqlmock.NewRows(columns). - AddRow("ws-get-1", "My Agent", "worker", 1, "online", []byte(`{"name":"test"}`), + AddRow("cccccccc-0001-0000-0000-000000000000", "My Agent", "worker", 1, "online", []byte(`{"name":"test"}`), "http://localhost:8001", nil, 2, 0.05, "", 3600, "working", "langgraph", "", 10.0, 20.0, false, nil, 0)) w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-get-1"}} + c.Params = gin.Params{{Key: "id", Value: "cccccccc-0001-0000-0000-000000000000"}} c.Request = httptest.NewRequest("GET", "/workspaces/ws-get-1", nil) handler.Get(c) @@ -74,12 +74,12 @@ func TestWorkspaceGet_NotFound(t *testing.T) { handler := NewWorkspaceHandler(broadcaster, nil, "http://localhost:8080", t.TempDir()) mock.ExpectQuery("SELECT w.id, w.name"). - WithArgs("ws-nonexistent"). + WithArgs("cccccccc-0002-0000-0000-000000000000"). WillReturnError(sql.ErrNoRows) w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-nonexistent"}} + c.Params = gin.Params{{Key: "id", Value: "cccccccc-0002-0000-0000-000000000000"}} c.Request = httptest.NewRequest("GET", "/workspaces/ws-nonexistent", nil) handler.Get(c) @@ -100,12 +100,12 @@ func TestWorkspaceGet_DBError(t *testing.T) { handler := NewWorkspaceHandler(broadcaster, nil, "http://localhost:8080", t.TempDir()) mock.ExpectQuery("SELECT w.id, w.name"). - WithArgs("ws-dberr"). + WithArgs("cccccccc-0003-0000-0000-000000000000"). WillReturnError(sql.ErrConnDone) w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-dberr"}} + c.Params = gin.Params{{Key: "id", Value: "cccccccc-0003-0000-0000-000000000000"}} c.Request = httptest.NewRequest("GET", "/workspaces/ws-dberr", nil) handler.Get(c) @@ -406,7 +406,7 @@ func TestWorkspaceUpdate_BadJSON(t *testing.T) { w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-upd"}} + c.Params = gin.Params{{Key: "id", Value: "cccccccc-0004-0000-0000-000000000000"}} c.Request = httptest.NewRequest("PATCH", "/workspaces/ws-upd", bytes.NewBufferString("not json")) c.Request.Header.Set("Content-Type", "application/json") @@ -425,22 +425,22 @@ func TestWorkspaceUpdate_MultipleFields(t *testing.T) { // #125: existence probe fires once before any field update. mock.ExpectQuery("SELECT EXISTS.*workspaces WHERE id"). - WithArgs("ws-multi"). + WithArgs("cccccccc-0005-0000-0000-000000000000"). WillReturnRows(sqlmock.NewRows([]string{"exists"}).AddRow(true)) // Expect name, role, and tier updates mock.ExpectExec("UPDATE workspaces SET name"). - WithArgs("ws-multi", "Updated Agent"). + WithArgs("cccccccc-0005-0000-0000-000000000000", "Updated Agent"). WillReturnResult(sqlmock.NewResult(0, 1)) mock.ExpectExec("UPDATE workspaces SET role"). - WithArgs("ws-multi", "manager"). + WithArgs("cccccccc-0005-0000-0000-000000000000", "manager"). WillReturnResult(sqlmock.NewResult(0, 1)) mock.ExpectExec("UPDATE workspaces SET tier"). - WithArgs("ws-multi", float64(3)). + WithArgs("cccccccc-0005-0000-0000-000000000000", float64(3)). WillReturnResult(sqlmock.NewResult(0, 1)) w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-multi"}} + c.Params = gin.Params{{Key: "id", Value: "cccccccc-0005-0000-0000-000000000000"}} body := `{"name":"Updated Agent","role":"manager","tier":3}` c.Request = httptest.NewRequest("PATCH", "/workspaces/ws-multi", bytes.NewBufferString(body)) @@ -472,15 +472,15 @@ func TestWorkspaceUpdate_RuntimeField(t *testing.T) { handler := NewWorkspaceHandler(broadcaster, nil, "http://localhost:8080", t.TempDir()) mock.ExpectQuery("SELECT EXISTS.*workspaces WHERE id"). - WithArgs("ws-rt"). + WithArgs("cccccccc-0006-0000-0000-000000000000"). WillReturnRows(sqlmock.NewRows([]string{"exists"}).AddRow(true)) mock.ExpectExec("UPDATE workspaces SET runtime"). - WithArgs("ws-rt", "claude-code"). + WithArgs("cccccccc-0006-0000-0000-000000000000", "claude-code"). WillReturnResult(sqlmock.NewResult(0, 1)) w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-rt"}} + c.Params = gin.Params{{Key: "id", Value: "cccccccc-0006-0000-0000-000000000000"}} body := `{"runtime":"claude-code"}` c.Request = httptest.NewRequest("PATCH", "/workspaces/ws-rt", bytes.NewBufferString(body)) @@ -507,14 +507,14 @@ func TestWorkspaceDelete_ConfirmationRequired(t *testing.T) { // Children query returns 2 children mock.ExpectQuery("SELECT id, name FROM workspaces WHERE parent_id"). - WithArgs("ws-parent"). + WithArgs("cccccccc-0007-0000-0000-000000000000"). WillReturnRows(sqlmock.NewRows([]string{"id", "name"}). - AddRow("ws-child-1", "Child One"). - AddRow("ws-child-2", "Child Two")) + AddRow("cccccccc-0008-0000-0000-000000000000", "Child One"). + AddRow("cccccccc-0009-0000-0000-000000000000", "Child Two")) w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-parent"}} + c.Params = gin.Params{{Key: "id", Value: "cccccccc-0007-0000-0000-000000000000"}} // No ?confirm=true c.Request = httptest.NewRequest("DELETE", "/workspaces/ws-parent", nil) @@ -552,14 +552,14 @@ func TestWorkspaceDelete_CascadeWithChildren(t *testing.T) { // Children query returns 1 child mock.ExpectQuery("SELECT id, name FROM workspaces WHERE parent_id"). - WithArgs("ws-parent-del"). + WithArgs("cccccccc-000a-0000-0000-000000000000"). WillReturnRows(sqlmock.NewRows([]string{"id", "name"}). - AddRow("ws-child-del", "Child Agent")) + AddRow("cccccccc-000b-0000-0000-000000000000", "Child Agent")) // Descendant CTE query returns the recursive set (1 descendant: ws-child-del) mock.ExpectQuery("WITH RECURSIVE descendants"). - WithArgs("ws-parent-del"). - WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow("ws-child-del")) + WithArgs("cccccccc-000a-0000-0000-000000000000"). + WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow("cccccccc-000b-0000-0000-000000000000")) // #73: single batch UPDATE covering [self + descendants] BEFORE stopping // containers (prevents heartbeat/restart resurrection races). @@ -580,7 +580,7 @@ func TestWorkspaceDelete_CascadeWithChildren(t *testing.T) { w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-parent-del"}} + c.Params = gin.Params{{Key: "id", Value: "cccccccc-000a-0000-0000-000000000000"}} c.Request = httptest.NewRequest("DELETE", "/workspaces/ws-parent-del?confirm=true", nil) handler.Delete(c) @@ -612,12 +612,12 @@ func TestWorkspaceDelete_ChildrenQueryError(t *testing.T) { handler := NewWorkspaceHandler(broadcaster, nil, "http://localhost:8080", t.TempDir()) mock.ExpectQuery("SELECT id, name FROM workspaces WHERE parent_id"). - WithArgs("ws-err-del"). + WithArgs("cccccccc-000c-0000-0000-000000000000"). WillReturnError(sql.ErrConnDone) w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-err-del"}} + c.Params = gin.Params{{Key: "id", Value: "cccccccc-000c-0000-0000-000000000000"}} c.Request = httptest.NewRequest("DELETE", "/workspaces/ws-err-del?confirm=true", nil) handler.Delete(c) @@ -781,69 +781,83 @@ 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. -// 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) { +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"). + WithArgs("cccccccc-000d-0000-0000-000000000000"). WillReturnRows(sqlmock.NewRows([]string{"exists"}).AddRow(true)) mock.ExpectExec("UPDATE workspaces SET name"). - WithArgs("ws-cosmetic", "Cosmetic"). + WithArgs("cccccccc-000d-0000-0000-000000000000", "Cosmetic"). WillReturnResult(sqlmock.NewResult(0, 1)) w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-cosmetic"}} + c.Params = gin.Params{{Key: "id", Value: "cccccccc-000d-0000-0000-000000000000"}} 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: got %d, want 200: %s", w.Code, w.Body.String()) + t.Errorf("cosmetic PATCH (no bearer) should pass; got %d: %s", w.Code, w.Body.String()) } } -// 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) { +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()) - // No workspace_auth_tokens query expected — auth is middleware's responsibility. + // 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: "cccccccc-000e-0000-0000-000000000000"}} + 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-owned"). + WithArgs("cccccccc-000f-0000-0000-000000000000"). WillReturnRows(sqlmock.NewRows([]string{"exists"}).AddRow(true)) mock.ExpectExec("UPDATE workspaces SET tier"). - WithArgs("ws-owned", float64(3)). + WithArgs("cccccccc-000f-0000-0000-000000000000", float64(4)). WillReturnResult(sqlmock.NewResult(0, 1)) w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-owned"}} - c.Request = httptest.NewRequest("PATCH", "/workspaces/ws-owned", - bytes.NewBufferString(`{"tier":3}`)) + c.Params = gin.Params{{Key: "id", Value: "cccccccc-000f-0000-0000-000000000000"}} + c.Request = httptest.NewRequest("PATCH", "/workspaces/ws-bootstrap", + bytes.NewBufferString(`{"tier":4}`)) 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("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) + t.Errorf("bootstrap fail-open: got %d, want 200 (%s)", w.Code, w.Body.String()) } } @@ -866,16 +880,16 @@ func TestWorkspaceGet_FinancialFieldsStripped(t *testing.T) { } // Populate with non-zero financial values to confirm they are stripped. mock.ExpectQuery("SELECT w.id, w.name"). - WithArgs("ws-fin-1"). + WithArgs("cccccccc-0010-0000-0000-000000000000"). WillReturnRows(sqlmock.NewRows(columns). - AddRow("ws-fin-1", "Finance Test", "worker", 1, "online", []byte(`{}`), + AddRow("cccccccc-0010-0000-0000-000000000000", "Finance Test", "worker", 1, "online", []byte(`{}`), "http://localhost:9001", nil, 0, 0.0, "", 0, "", "langgraph", "", 0.0, 0.0, false, int64(50000), int64(12500))) // budget_limit=500 USD, spend=125 USD w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-fin-1"}} + c.Params = gin.Params{{Key: "id", Value: "cccccccc-0010-0000-0000-000000000000"}} c.Request = httptest.NewRequest("GET", "/workspaces/ws-fin-1", nil) handler.Get(c) @@ -917,16 +931,16 @@ func TestWorkspaceUpdate_BudgetLimitIgnored(t *testing.T) { // Only the existence probe fires — no UPDATE for budget_limit. mock.ExpectQuery("SELECT EXISTS.*workspaces WHERE id"). - WithArgs("ws-budget-test"). + WithArgs("cccccccc-0011-0000-0000-000000000000"). WillReturnRows(sqlmock.NewRows([]string{"exists"}).AddRow(true)) // name update is the only expected write mock.ExpectExec("UPDATE workspaces SET name"). - WithArgs("ws-budget-test", "Safe Name"). + WithArgs("cccccccc-0011-0000-0000-000000000000", "Safe Name"). WillReturnResult(sqlmock.NewResult(0, 1)) w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-budget-test"}} + c.Params = gin.Params{{Key: "id", Value: "cccccccc-0011-0000-0000-000000000000"}} // Send budget_limit alongside an innocuous field. body := `{"name":"Safe Name","budget_limit":null}` c.Request = httptest.NewRequest("PATCH", "/workspaces/ws-budget-test", @@ -954,13 +968,13 @@ func TestWorkspaceUpdate_BudgetLimitOnly_Ignored(t *testing.T) { handler := NewWorkspaceHandler(broadcaster, nil, "http://localhost:8080", t.TempDir()) mock.ExpectQuery("SELECT EXISTS.*workspaces WHERE id"). - WithArgs("ws-budget-only"). + WithArgs("cccccccc-0012-0000-0000-000000000000"). WillReturnRows(sqlmock.NewRows([]string{"exists"}).AddRow(true)) // No UPDATE expected — budget_limit must be silently skipped. w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) - c.Params = gin.Params{{Key: "id", Value: "ws-budget-only"}} + c.Params = gin.Params{{Key: "id", Value: "cccccccc-0012-0000-0000-000000000000"}} c.Request = httptest.NewRequest("PATCH", "/workspaces/ws-budget-only", bytes.NewBufferString(`{"budget_limit":999999}`)) c.Request.Header.Set("Content-Type", "application/json") diff --git a/platform/internal/router/router.go b/platform/internal/router/router.go index 5be4b3df2..27072bab1 100644 --- a/platform/internal/router/router.go +++ b/platform/internal/router/router.go @@ -110,6 +110,16 @@ 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: @@ -132,13 +142,6 @@ 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) @@ -317,16 +320,6 @@ func Setup(hub *ws.Hub, broadcaster *events.Broadcaster, prov *provisioner.Provi adminAuth.DELETE("/admin/secrets/:key", sechGlobal.DeleteGlobal) } - // Admin — cross-workspace schedule health monitoring (issue #618). - // Lets cron-audit agents and operators detect silent schedule failures - // across all workspaces without holding individual workspace bearer tokens. - // AdminAuth mirrors the /admin/liveness gate — fail-open on fresh install, - // strict bearer-only once any token exists. - { - asHealth := handlers.NewAdminSchedulesHealthHandler() - r.GET("/admin/schedules/health", middleware.AdminAuth(db.DB), asHealth.Health) - } - // Admin — test token minting (issue #6). Hidden in production via TestTokensEnabled(). // AdminAuth is a second defence-in-depth layer: on a fresh install with no tokens yet, // AdminAuth is fail-open (HasAnyLiveTokenGlobal == 0), so the bootstrap still works. @@ -370,11 +363,14 @@ func Setup(hub *ws.Hub, broadcaster *events.Broadcaster, prov *provisioner.Provi // Templates tmplh := handlers.NewTemplatesHandler(configsDir, dockerCli) - r.GET("/templates", tmplh.List) - // #190: POST /templates/import writes arbitrary files into configsDir. - // Must be admin-gated — same class as /bundles/import (#164) and /org/import. + // #686: GET /templates lists all template names+metadata from configsDir. + // Open access lets unauthenticated callers enumerate org configurations and + // installed plugins. AdminAuth-gate it alongside POST /templates/import. { tmplAdmin := r.Group("", middleware.AdminAuth(db.DB)) + tmplAdmin.GET("/templates", tmplh.List) + // #190: POST /templates/import writes arbitrary files into configsDir. + // Must be admin-gated — same class as /bundles/import (#164) and /org/import. tmplAdmin.POST("/templates/import", tmplh.Import) } wsAuth.GET("/shared-context", tmplh.SharedContext) @@ -427,7 +423,9 @@ func Setup(hub *ws.Hub, broadcaster *events.Broadcaster, prov *provisioner.Provi // Org Templates orgDir := findOrgDir(configsDir) orgh := handlers.NewOrgHandler(wh, broadcaster, prov, channelMgr, configsDir, orgDir) - r.GET("/org/templates", orgh.ListTemplates) + // #686: GET /org/templates exposes the org template catalogue (names, roles, + // configured system prompts). AdminAuth-gate to match /org/import. + r.GET("/org/templates", middleware.AdminAuth(db.DB), orgh.ListTemplates) // /org/import can create arbitrary workspaces from an uploaded YAML — it // must be an admin-gated route. The handler also path-sanitizes // `dir`/`template`/`files_dir` via resolveInsideRoot, but defence-in- From 4a3b747dffa5363dd765c562edb798c1251abd2c Mon Sep 17 00:00:00 2001 From: Molecule AI QA Engineer Date: Fri, 17 Apr 2026 11:52:57 +0000 Subject: [PATCH 2/3] test(security): regression suite for input validation fixes (#685 #686 #687 #688) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 30 test cases covering all four security fixes from PR #701: #686 — AdminAuth gate on GET /templates and GET /org/templates: - NoAuth returns 401 when tokens are enrolled - FreshInstall fails open (bootstraps correctly) #687 — UUID path param validation: - URL-encoded traversal (..%2f..%2fetc%2fpasswd) → 400 - Non-UUID strings (not-a-uuid, ws-123, XSS payloads) → 400 - Valid UUIDs pass through (regression check) #688 — Field length limits: - name=256, role=1001, model=101 chars → 400 - Exact-boundary values (255/1000/100) → pass (off-by-one guard) #685 — YAML injection via newline/CR: - Newline in name, CR in role → 400 - YAML multi-field injection payload "agent\nrole: injected" → 400 Co-Authored-By: Claude Sonnet 4.6 --- ...ecurity_regression_685_686_687_688_test.go | 477 ++++++++++++++++++ 1 file changed, 477 insertions(+) create mode 100644 platform/internal/handlers/security_regression_685_686_687_688_test.go diff --git a/platform/internal/handlers/security_regression_685_686_687_688_test.go b/platform/internal/handlers/security_regression_685_686_687_688_test.go new file mode 100644 index 000000000..f8d4fcb90 --- /dev/null +++ b/platform/internal/handlers/security_regression_685_686_687_688_test.go @@ -0,0 +1,477 @@ +package handlers + +// security_regression_685_686_687_688_test.go — regression suite for the +// input-validation security fixes shipped in PR #701. +// +// #686 — GET /templates and GET /org/templates now require AdminAuth +// #687 — UUID validation on workspace :id path params (invalid UUID → 400) +// #688 — Field length limits: name≤255, role≤1000, model/runtime≤100 +// #685 — YAML injection: newline/CR characters rejected in name/role/model/runtime +// +// These tests are intentionally kept at the handler layer (not full router) +// for fast CI execution. The template auth tests are the exception — they wire +// AdminAuth middleware into a mini gin router to verify the actual security gate +// rather than the handler's internal logic. + +import ( + "bytes" + "database/sql" + "net/http" + "net/http/httptest" + "strings" + "testing" + + sqlmock "github.com/DATA-DOG/go-sqlmock" + "github.com/Molecule-AI/molecule-monorepo/platform/internal/middleware" + "github.com/gin-gonic/gin" +) + +// authTokenQuery matches the SELECT issued by HasAnyLiveTokenGlobal inside AdminAuth. +const authTokenQuery = "SELECT COUNT.*workspace_auth_tokens" + +// newEnrolledAuthDB returns a sqlmock DB pre-loaded so that the next +// HasAnyLiveTokenGlobal call reports one enrolled workspace (i.e., auth is enforced). +// The returned Sqlmock lets the caller verify expectations afterwards. +func newEnrolledAuthDB(t *testing.T) (*sql.DB, sqlmock.Sqlmock) { + t.Helper() + d, m, err := sqlmock.New() + if err != nil { + t.Fatalf("sqlmock.New: %v", err) + } + t.Cleanup(func() { _ = d.Close() }) + m.ExpectQuery(authTokenQuery). + WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(1)) + return d, m +} + +// newFreshInstallAuthDB returns a sqlmock DB where HasAnyLiveTokenGlobal +// reports zero enrolled workspaces — the platform is in fail-open bootstrap mode. +func newFreshInstallAuthDB(t *testing.T) (*sql.DB, sqlmock.Sqlmock) { + t.Helper() + d, m, err := sqlmock.New() + if err != nil { + t.Fatalf("sqlmock.New: %v", err) + } + t.Cleanup(func() { _ = d.Close() }) + m.ExpectQuery(authTokenQuery). + WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(0)) + return d, m +} + +// ───────────────────────────────────────────────────────────────────────────── +// #686 — AdminAuth gate on GET /templates +// ───────────────────────────────────────────────────────────────────────────── + +// TestSecurity_GetTemplates_NoAuth_Returns401 verifies that once at least one +// workspace is enrolled (tokens exist), GET /templates without a bearer token +// is rejected with 401. Previously the route was unauthenticated (#686). +func TestSecurity_GetTemplates_NoAuth_Returns401(t *testing.T) { + setupTestDB(t) + setupTestRedis(t) + authDB, authMock := newEnrolledAuthDB(t) + + tmpDir := t.TempDir() + tmplh := NewTemplatesHandler(tmpDir, nil) + + r := gin.New() + r.GET("/templates", middleware.AdminAuth(authDB), tmplh.List) + + w := httptest.NewRecorder() + req, _ := http.NewRequest(http.MethodGet, "/templates", nil) + // Deliberately omit Authorization header — must be rejected. + r.ServeHTTP(w, req) + + if w.Code != http.StatusUnauthorized { + t.Errorf("#686 GET /templates no-auth: want 401, got %d body=%s", w.Code, w.Body.String()) + } + if err := authMock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet auth mock expectations: %v", err) + } +} + +// TestSecurity_GetTemplates_FreshInstall_FailsOpen verifies that GET /templates +// still succeeds on a fresh install (zero enrolled workspaces → AdminAuth fail-open). +// This is the regression check: the auth gate must not break new deployments. +func TestSecurity_GetTemplates_FreshInstall_FailsOpen(t *testing.T) { + setupTestDB(t) + setupTestRedis(t) + authDB, authMock := newFreshInstallAuthDB(t) + + tmpDir := t.TempDir() + tmplh := NewTemplatesHandler(tmpDir, nil) + + r := gin.New() + r.GET("/templates", middleware.AdminAuth(authDB), tmplh.List) + + w := httptest.NewRecorder() + req, _ := http.NewRequest(http.MethodGet, "/templates", nil) + r.ServeHTTP(w, req) + + if w.Code != http.StatusOK { + t.Errorf("#686 GET /templates fresh-install: want 200 (fail-open), got %d body=%s", w.Code, w.Body.String()) + } + if err := authMock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet auth mock expectations: %v", err) + } +} + +// ───────────────────────────────────────────────────────────────────────────── +// #686 — AdminAuth gate on GET /org/templates +// ───────────────────────────────────────────────────────────────────────────── + +// TestSecurity_GetOrgTemplates_NoAuth_Returns401 verifies that GET /org/templates +// requires a bearer token once the platform has enrolled workspaces. +// Previously the route was unauthenticated, exposing org structure details (#686). +func TestSecurity_GetOrgTemplates_NoAuth_Returns401(t *testing.T) { + setupTestDB(t) + setupTestRedis(t) + authDB, authMock := newEnrolledAuthDB(t) + + tmpDir := t.TempDir() + wh := NewWorkspaceHandler(newTestBroadcaster(), nil, "http://localhost:8080", tmpDir) + orgh := NewOrgHandler(wh, newTestBroadcaster(), nil, nil, tmpDir, tmpDir) + + r := gin.New() + r.GET("/org/templates", middleware.AdminAuth(authDB), orgh.ListTemplates) + + w := httptest.NewRecorder() + req, _ := http.NewRequest(http.MethodGet, "/org/templates", nil) + // No Authorization header — must be rejected. + r.ServeHTTP(w, req) + + if w.Code != http.StatusUnauthorized { + t.Errorf("#686 GET /org/templates no-auth: want 401, got %d body=%s", w.Code, w.Body.String()) + } + if err := authMock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet auth mock expectations: %v", err) + } +} + +// TestSecurity_GetOrgTemplates_FreshInstall_FailsOpen mirrors the /templates +// regression check for /org/templates — fresh installs must still work. +func TestSecurity_GetOrgTemplates_FreshInstall_FailsOpen(t *testing.T) { + setupTestDB(t) + setupTestRedis(t) + authDB, authMock := newFreshInstallAuthDB(t) + + tmpDir := t.TempDir() + wh := NewWorkspaceHandler(newTestBroadcaster(), nil, "http://localhost:8080", tmpDir) + orgh := NewOrgHandler(wh, newTestBroadcaster(), nil, nil, tmpDir, tmpDir) + + r := gin.New() + r.GET("/org/templates", middleware.AdminAuth(authDB), orgh.ListTemplates) + + w := httptest.NewRecorder() + req, _ := http.NewRequest(http.MethodGet, "/org/templates", nil) + r.ServeHTTP(w, req) + + if w.Code != http.StatusOK { + t.Errorf("#686 GET /org/templates fresh-install: want 200 (fail-open), got %d body=%s", w.Code, w.Body.String()) + } + if err := authMock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet auth mock expectations: %v", err) + } +} + +// ───────────────────────────────────────────────────────────────────────────── +// #687 — UUID validation on workspace :id path params +// ───────────────────────────────────────────────────────────────────────────── + +// TestSecurity_Get_URLEncodedTraversal_Returns400 verifies that a URL-encoded +// path traversal sequence — the type a browser or curl submits as +// /workspaces/..%252f..%252fetc%252fpasswd (double-encoded → decoded to +// ..%2f..%2fetc%2fpasswd by the HTTP layer) — is rejected 400 before any DB +// query. Previously a non-UUID id caused a Postgres syntax error → 500. +func TestSecurity_Get_URLEncodedTraversal_Returns400(t *testing.T) { + setupTestDB(t) + setupTestRedis(t) + handler := NewWorkspaceHandler(newTestBroadcaster(), nil, "http://localhost:8080", t.TempDir()) + + // gin decodes %25 → %, so the outer HTTP layer hands the handler this value. + traversalID := "..%2f..%2fetc%2fpasswd" + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: traversalID}} + c.Request = httptest.NewRequest(http.MethodGet, "/workspaces/"+traversalID, nil) + + handler.Get(c) + + if w.Code != http.StatusBadRequest { + t.Errorf("#687 URL-encoded traversal Get(%q): want 400, got %d body=%s", + traversalID, w.Code, w.Body.String()) + } +} + +// TestSecurity_Get_NotUUID_Returns400 checks the simplest non-UUID rejection. +func TestSecurity_Get_NotUUID_Returns400(t *testing.T) { + setupTestDB(t) + setupTestRedis(t) + handler := NewWorkspaceHandler(newTestBroadcaster(), nil, "http://localhost:8080", t.TempDir()) + + for _, badID := range []string{ + "not-a-uuid", + "ws-123", + "123", + "../etc/passwd", + "", + } { + t.Run(badID, func(t *testing.T) { + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: badID}} + c.Request = httptest.NewRequest(http.MethodGet, "/workspaces/"+badID, nil) + handler.Get(c) + if w.Code != http.StatusBadRequest { + t.Errorf("#687 Get(%q): want 400, got %d", badID, w.Code) + } + }) + } +} + +// TestSecurity_ValidUUID_PassesUUIDValidation verifies that a well-formed UUID +// passes the validateWorkspaceID guard — i.e., the fix doesn't false-positive +// on legitimate workspace IDs. +func TestSecurity_ValidUUID_PassesUUIDValidation(t *testing.T) { + if err := validateWorkspaceID("550e8400-e29b-41d4-a716-446655440000"); err != nil { + t.Errorf("regression: valid UUID rejected: %v", err) + } + if err := validateWorkspaceID("aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"); err != nil { + t.Errorf("regression: valid UUID rejected: %v", err) + } +} + +// ───────────────────────────────────────────────────────────────────────────── +// #688 — Field length limits on POST /workspaces +// ───────────────────────────────────────────────────────────────────────────── + +// TestSecurity_Create_NameTooLong_Returns400 verifies a 256-character name is +// rejected before any DB interaction. The limit is 255 characters (#688). +func TestSecurity_Create_NameTooLong_Returns400(t *testing.T) { + setupTestDB(t) + setupTestRedis(t) + handler := NewWorkspaceHandler(newTestBroadcaster(), nil, "http://localhost:8080", t.TempDir()) + + name256 := strings.Repeat("a", 256) + body := `{"name":"` + name256 + `"}` + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Request = httptest.NewRequest(http.MethodPost, "/workspaces", bytes.NewBufferString(body)) + c.Request.Header.Set("Content-Type", "application/json") + + handler.Create(c) + + if w.Code != http.StatusBadRequest { + t.Errorf("#688 name=256 chars: want 400, got %d body=%s", w.Code, w.Body.String()) + } +} + +// TestSecurity_Create_RoleTooLong_Returns400 verifies a 1001-character role is +// rejected. The limit is 1000 characters (#688). +func TestSecurity_Create_RoleTooLong_Returns400(t *testing.T) { + setupTestDB(t) + setupTestRedis(t) + handler := NewWorkspaceHandler(newTestBroadcaster(), nil, "http://localhost:8080", t.TempDir()) + + role1001 := strings.Repeat("r", 1001) + body := `{"name":"valid-name","role":"` + role1001 + `"}` + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Request = httptest.NewRequest(http.MethodPost, "/workspaces", bytes.NewBufferString(body)) + c.Request.Header.Set("Content-Type", "application/json") + + handler.Create(c) + + if w.Code != http.StatusBadRequest { + t.Errorf("#688 role=1001 chars: want 400, got %d body=%s", w.Code, w.Body.String()) + } +} + +// TestSecurity_Create_ModelTooLong_Returns400 verifies a 101-character model +// is rejected (#688). The limit is 100 characters. +func TestSecurity_Create_ModelTooLong_Returns400(t *testing.T) { + setupTestDB(t) + setupTestRedis(t) + handler := NewWorkspaceHandler(newTestBroadcaster(), nil, "http://localhost:8080", t.TempDir()) + + model101 := strings.Repeat("m", 101) + body := `{"name":"valid-name","model":"` + model101 + `"}` + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Request = httptest.NewRequest(http.MethodPost, "/workspaces", bytes.NewBufferString(body)) + c.Request.Header.Set("Content-Type", "application/json") + + handler.Create(c) + + if w.Code != http.StatusBadRequest { + t.Errorf("#688 model=101 chars: want 400, got %d body=%s", w.Code, w.Body.String()) + } +} + +// ───────────────────────────────────────────────────────────────────────────── +// #685 — YAML injection: newline/CR rejection +// ───────────────────────────────────────────────────────────────────────────── + +// TestSecurity_Create_NameWithNewline_Returns400 verifies that a workspace name +// containing a literal newline character is rejected before DB interaction. +// Newlines break YAML multi-line quoting even with yamlQuote escaping (#685). +func TestSecurity_Create_NameWithNewline_Returns400(t *testing.T) { + setupTestDB(t) + setupTestRedis(t) + handler := NewWorkspaceHandler(newTestBroadcaster(), nil, "http://localhost:8080", t.TempDir()) + + // JSON \n is a literal newline in the parsed string value. + body := `{"name":"bad\nname"}` + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Request = httptest.NewRequest(http.MethodPost, "/workspaces", bytes.NewBufferString(body)) + c.Request.Header.Set("Content-Type", "application/json") + + handler.Create(c) + + if w.Code != http.StatusBadRequest { + t.Errorf("#685 name with \\n: want 400, got %d body=%s", w.Code, w.Body.String()) + } +} + +// TestSecurity_Create_YAMLInjectionViaNewline_Returns400 verifies that a +// workspace name crafted to inject YAML fields via a newline is caught by the +// newline-rejection gate before reaching the provisioner. +// +// The attack string "agent\nrole: injected_value" would, if written unquoted +// into a YAML config, silently set the role field to "injected_value". The +// newline is the injection vector — it is rejected by #685. +// +// Note: curly-brace injection like "{inject: yaml}" does not contain newlines +// and is handled separately by yamlQuote escaping in the provisioner +// (defence-in-depth). That value is intentionally allowed through here and +// must be tested against the provisioner's yamlQuote output, not this gate. +func TestSecurity_Create_YAMLInjectionViaNewline_Returns400(t *testing.T) { + setupTestDB(t) + setupTestRedis(t) + handler := NewWorkspaceHandler(newTestBroadcaster(), nil, "http://localhost:8080", t.TempDir()) + + // The injected string breaks out of a YAML scalar via newline. + body := "{\"name\":\"agent\\nrole: injected_value\"}" + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Request = httptest.NewRequest(http.MethodPost, "/workspaces", bytes.NewBufferString(body)) + c.Request.Header.Set("Content-Type", "application/json") + + handler.Create(c) + + if w.Code != http.StatusBadRequest { + t.Errorf("#685 YAML injection via \\n: want 400, got %d body=%s", w.Code, w.Body.String()) + } +} + +// TestSecurity_Create_RoleWithCR_Returns400 verifies carriage-return rejection +// in the role field (#685). CR alone can also break YAML multi-line values. +func TestSecurity_Create_RoleWithCR_Returns400(t *testing.T) { + setupTestDB(t) + setupTestRedis(t) + handler := NewWorkspaceHandler(newTestBroadcaster(), nil, "http://localhost:8080", t.TempDir()) + + body := "{\"name\":\"ok\",\"role\":\"bad\\rrole\"}" + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Request = httptest.NewRequest(http.MethodPost, "/workspaces", bytes.NewBufferString(body)) + c.Request.Header.Set("Content-Type", "application/json") + + handler.Create(c) + + if w.Code != http.StatusBadRequest { + t.Errorf("#685 role with \\r: want 400, got %d body=%s", w.Code, w.Body.String()) + } +} + +// ───────────────────────────────────────────────────────────────────────────── +// Regression: validateWorkspaceFields direct unit coverage +// ───────────────────────────────────────────────────────────────────────────── + +// TestSecurity_ValidateWorkspaceFields_BoundaryValues exercises exact-boundary +// values for all four field limits to ensure the fence posts are correct. +// These are regression checks: fixing the upper limits must not accidentally +// tighten or loosen the constraint by ±1. +func TestSecurity_ValidateWorkspaceFields_BoundaryValues(t *testing.T) { + cases := []struct { + label string + name string + role string + model string + runtime string + wantErr bool + }{ + // Exact maximum lengths — must PASS. + {"name_at_255", strings.Repeat("a", 255), "", "", "", false}, + {"role_at_1000", "", strings.Repeat("r", 1000), "", "", false}, + {"model_at_100", "", "", strings.Repeat("m", 100), "", false}, + {"runtime_at_100", "", "", "", strings.Repeat("x", 100), false}, + // One over the limit — must FAIL. + {"name_at_256", strings.Repeat("a", 256), "", "", "", true}, + {"role_at_1001", "", strings.Repeat("r", 1001), "", "", true}, + {"model_at_101", "", "", strings.Repeat("m", 101), "", true}, + {"runtime_at_101", "", "", "", strings.Repeat("x", 101), true}, + // Newline/CR in each field — must FAIL. + {"name_newline", "a\nb", "", "", "", true}, + {"role_cr", "", "a\rb", "", "", true}, + {"model_newline", "", "", "a\nb", "", true}, + {"runtime_newline", "", "", "", "a\nb", true}, + // Fully valid — must PASS. + {"all_valid", "My Agent", "You are a helpful agent.", "claude-opus-4-7", "langgraph", false}, + } + + for _, tc := range cases { + t.Run(tc.label, func(t *testing.T) { + err := validateWorkspaceFields(tc.name, tc.role, tc.model, tc.runtime) + if tc.wantErr && err == nil { + t.Errorf("want error, got nil") + } + if !tc.wantErr && err != nil { + t.Errorf("want nil, got %v", err) + } + }) + } +} + +// TestSecurity_ValidateWorkspaceID_ValidUUIDs verifies that real workspace UUIDs +// (RFC 4122 v4) are accepted. Regression check: the fix must not reject valid IDs. +func TestSecurity_ValidateWorkspaceID_ValidUUIDs(t *testing.T) { + valid := []string{ + "550e8400-e29b-41d4-a716-446655440000", // RFC 4122 example + "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee", + "00000000-0000-0000-0000-000000000000", + "dddddddd-0001-0000-0000-000000000000", // used in other handler tests + } + for _, id := range valid { + if err := validateWorkspaceID(id); err != nil { + t.Errorf("regression: valid UUID %q rejected: %v", id, err) + } + } +} + +// TestSecurity_ValidateWorkspaceID_InvalidIDs checks that non-UUID strings all +// return errors from validateWorkspaceID. +func TestSecurity_ValidateWorkspaceID_InvalidIDs(t *testing.T) { + invalid := []string{ + "not-a-uuid", + "ws-abc", + "", + "../etc/passwd", + "..%2f..%2fetc%2fpasswd", + "