From bdd56b14893a8d2384ab3a82697fe4682b8ed416 Mon Sep 17 00:00:00 2001 From: "molecule-ai[bot]" <276602405+molecule-ai[bot]@users.noreply.github.com> Date: Fri, 17 Apr 2026 12:13:44 +0000 Subject: [PATCH] =?UTF-8?q?fix(security):=20rebase=20#685-688=20onto=20mai?= =?UTF-8?q?n=20=E2=80=94=20preserve=20wsAuth=20PATCH,=20add=20yamlSpecialC?= =?UTF-8?q?hars?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Rebased onto 350288f1 (main HEAD, post-#692 IDOR fix) - PATCH /workspaces/:id remains under wsAuth group (not open router) - Added validateWorkspaceID (uuid.Parse check) in Get/Update/Delete - Added validateWorkspaceFields: rejects \n\r in all fields, yamlSpecialChars {}[]|>*&! in name/role only, enforces max lengths - Template endpoints (GET /templates, GET /org/templates) now require AdminAuth - Replaced stale in-handler sensitiveUpdateFields gate tests with TestWorkspaceUpdate_SensitiveField_AuthEnforcedByMiddleware Closes #685 #686 #687 #688 --- .../handlers/handlers_additional_test.go | 32 ++-- .../handlers/handlers_extended_test.go | 162 +++++++++++++++++- platform/internal/handlers/handlers_test.go | 6 +- platform/internal/handlers/workspace.go | 98 +++++++++++ .../handlers/workspace_budget_test.go | 20 +-- platform/internal/handlers/workspace_test.go | 92 +++++----- platform/internal/router/router.go | 9 +- 7 files changed, 333 insertions(+), 86 deletions(-) diff --git a/platform/internal/handlers/handlers_additional_test.go b/platform/internal/handlers/handlers_additional_test.go index 5316497c..a2468c0f 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 1e6f3a53..f3cbbb27 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 25a67578..2af65d2c 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 827546ce..d5e8117c 100644 --- a/platform/internal/handlers/workspace.go +++ b/platform/internal/handlers/workspace.go @@ -75,6 +75,13 @@ func (h *WorkspaceHandler) Create(c *gin.Context) { return } + // #685/#688: validate field lengths and reject injection 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 +400,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, ''), @@ -531,12 +544,34 @@ var sensitiveUpdateFields = map[string]struct{}{ 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). @@ -647,6 +682,12 @@ func (h *WorkspaceHandler) Delete(c *gin.Context) { ctx := c.Request.Context() confirm := c.Query("confirm") == "true" + // #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 + } + // Check for children rows, err := db.DB.QueryContext(ctx, `SELECT id, name FROM workspaces WHERE parent_id = $1 AND status != 'removed'`, id) @@ -773,3 +814,60 @@ func (h *WorkspaceHandler) Delete(c *gin.Context) { c.JSON(http.StatusOK, gin.H{"status": "removed", "cascade_deleted": len(descendantIDs)}) } + +// 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 +} + +// yamlSpecialChars is the set of YAML-special characters banned from workspace +// name and role. Newlines are handled separately below (same error message for +// all four fields); these additional characters target YAML block indicators, +// flow-sequence/mapping delimiters, and shell-expansion metacharacters that +// yamlQuote does NOT escape inside a double-quoted scalar (#685). +const yamlSpecialChars = "{}[]|>*&!" + +// validateWorkspaceFields enforces maximum field lengths and rejects characters +// that could enable YAML-injection in downstream provisioning paths. +// #685 (defence-in-depth over yamlQuote — newline + YAML-special chars in name/role), +// #688 (max field lengths). +func validateWorkspaceFields(name, role, model, runtime string) error { + // All four fields: reject newline / carriage-return. + 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) + } + } + // name and role only: reject YAML-special characters (#685). + for _, f := range []struct{ label, val string }{ + {"name", name}, + {"role", role}, + } { + if strings.ContainsAny(f.val, yamlSpecialChars) { + return fmt.Errorf("%s contains invalid 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 +} diff --git a/platform/internal/handlers/workspace_budget_test.go b/platform/internal/handlers/workspace_budget_test.go index 97a54e2a..c25b07da 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 6bd3cdca..42576dfc 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,32 +781,30 @@ 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()) } } @@ -824,16 +822,16 @@ func TestWorkspaceUpdate_SensitiveField_AuthEnforcedByMiddleware(t *testing.T) { // No workspace_auth_tokens query expected — auth is middleware's responsibility. mock.ExpectQuery("SELECT EXISTS.*workspaces WHERE id"). - WithArgs("ws-owned"). + WithArgs("cccccccc-000e-0000-0000-000000000000"). WillReturnRows(sqlmock.NewRows([]string{"exists"}).AddRow(true)) mock.ExpectExec("UPDATE workspaces SET tier"). - WithArgs("ws-owned", float64(3)). + WithArgs("cccccccc-000e-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-owned"}} - c.Request = httptest.NewRequest("PATCH", "/workspaces/ws-owned", + c.Params = gin.Params{{Key: "id", Value: "cccccccc-000e-0000-0000-000000000000"}} + c.Request = httptest.NewRequest("PATCH", "/workspaces/cccccccc-000e-0000-0000-000000000000", bytes.NewBufferString(`{"tier":3}`)) c.Request.Header.Set("Content-Type", "application/json") // WorkspaceAuth middleware would have validated the bearer before this runs. @@ -866,16 +864,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 +915,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 +952,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 5be4b3df..daa1572f 100644 --- a/platform/internal/router/router.go +++ b/platform/internal/router/router.go @@ -370,11 +370,14 @@ func Setup(hub *ws.Hub, broadcaster *events.Broadcaster, prov *provisioner.Provi // Templates tmplh := handlers.NewTemplatesHandler(configsDir, dockerCli) - r.GET("/templates", tmplh.List) + // #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. // #190: POST /templates/import writes arbitrary files into configsDir. // Must be admin-gated — same class as /bundles/import (#164) and /org/import. { tmplAdmin := r.Group("", middleware.AdminAuth(db.DB)) + tmplAdmin.GET("/templates", tmplh.List) tmplAdmin.POST("/templates/import", tmplh.Import) } wsAuth.GET("/shared-context", tmplh.SharedContext) @@ -427,7 +430,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-