From 2741f5d53b1e0261b15e6b78b80a6e0fa641d913 Mon Sep 17 00:00:00 2001 From: Dev Lead Agent Date: Wed, 15 Apr 2026 08:40:06 +0000 Subject: [PATCH] =?UTF-8?q?test(security):=20add=20#120=20regression=20tes?= =?UTF-8?q?ts=20=E2=80=94=20PATCH=20auth=20+=20workspace=20existence=20gua?= =?UTF-8?q?rd?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two gaps identified by Security Auditor in PR #125 review cycle: 1. handlers_extended_test.go: - Fix TestExtended_WorkspaceUpdate: add SELECT EXISTS mock expectation so the test correctly reflects the #120 existence guard now running first. - Add TestExtended_WorkspaceUpdate_NotFound: verifies PATCH returns 404 (not 200) for a nonexistent workspace ID — the core #120 behaviour fix. 2. wsauth_middleware_test.go: - Add TestAdminAuth_Issue120_PatchWorkspace_NoBearer_Returns401: documents the confirmed attack vector (PATCH without token must return 401) and asserts AdminAuth is applied to PATCH /workspaces/:id per the router.go change. Co-Authored-By: Claude Sonnet 4.6 --- .../handlers/handlers_extended_test.go | 47 +++++++++++++++++++ .../middleware/wsauth_middleware_test.go | 47 +++++++++++++++++++ 2 files changed, 94 insertions(+) diff --git a/platform/internal/handlers/handlers_extended_test.go b/platform/internal/handlers/handlers_extended_test.go index 68df80ed..1e6f3a53 100644 --- a/platform/internal/handlers/handlers_extended_test.go +++ b/platform/internal/handlers/handlers_extended_test.go @@ -73,6 +73,11 @@ func TestExtended_WorkspaceUpdate(t *testing.T) { broadcaster := newTestBroadcaster() handler := NewWorkspaceHandler(broadcaster, nil, "http://localhost:8080", "/tmp/configs") + // #120 fix: existence check runs first — workspace must be found before updates proceed. + mock.ExpectQuery("SELECT EXISTS"). + WithArgs("ws-upd"). + WillReturnRows(sqlmock.NewRows([]string{"exists"}).AddRow(true)) + // Expect name update mock.ExpectExec("UPDATE workspaces SET name"). WithArgs("ws-upd", "New Name"). @@ -110,6 +115,48 @@ func TestExtended_WorkspaceUpdate(t *testing.T) { } } +// TestExtended_WorkspaceUpdate_NotFound verifies the #120 fix: PATCH /workspaces/:id +// returns 404 (not 200) when the workspace does not exist in the DB. +// +// Before PR #125, the handler ran blind UPDATEs that matched zero rows and still +// returned {"status":"updated"} HTTP 200 — allowing an attacker to probe and +// speculatively modify workspace attributes (name, tier, parent_id, runtime, +// workspace_dir) without any observable error. The existence guard must fire +// and return 404 before any UPDATE is attempted. +func TestExtended_WorkspaceUpdate_NotFound(t *testing.T) { + mock := setupTestDB(t) + setupTestRedis(t) + broadcaster := newTestBroadcaster() + handler := NewWorkspaceHandler(broadcaster, nil, "http://localhost:8080", "/tmp/configs") + + // Existence check returns false — workspace does not exist. + mock.ExpectQuery("SELECT EXISTS"). + WithArgs("00000000-0000-0000-0000-000000000000"). + WillReturnRows(sqlmock.NewRows([]string{"exists"}).AddRow(false)) + + // No UPDATE or INSERT should follow — the handler must short-circuit at 404. + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: "00000000-0000-0000-0000-000000000000"}} + + body := `{"name":"probe"}` + c.Request = httptest.NewRequest("PATCH", + "/workspaces/00000000-0000-0000-0000-000000000000", + bytes.NewBufferString(body)) + c.Request.Header.Set("Content-Type", "application/json") + + handler.Update(c) + + if w.Code != http.StatusNotFound { + t.Errorf("#120 regression: expected 404 for nonexistent workspace, got %d: %s", + w.Code, w.Body.String()) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations: %v", err) + } +} + // ---------- TestWorkspaceRestart (Extended) ---------- func TestExtended_WorkspaceRestart_NoProvisioner(t *testing.T) { diff --git a/platform/internal/middleware/wsauth_middleware_test.go b/platform/internal/middleware/wsauth_middleware_test.go index 50ba5b91..95b79bbb 100644 --- a/platform/internal/middleware/wsauth_middleware_test.go +++ b/platform/internal/middleware/wsauth_middleware_test.go @@ -472,3 +472,50 @@ func TestAdminAuth_InvalidBearer_Returns401(t *testing.T) { t.Errorf("unmet sqlmock expectations: %v", err) } } + +// ──────────────────────────────────────────────────────────────────────────── +// Issue #120 regression — unauthenticated PATCH /workspaces/:id +// +// Before PR #125, PATCH /workspaces/:id was registered outside the wsAdmin +// group and did NOT enforce AdminAuth. An attacker could change workspace +// name, tier, parent_id, runtime, or workspace_dir without any token. +// Security Auditor confirmed the live exploit: +// curl -X PATCH .../workspaces/00000000-.../ -d '{"name":"probe"}' → 200 +// +// This test asserts AdminAuth applied to the PATCH route blocks unauthenticated +// requests — the route-level fix in router.go is the enforcement point. +// ──────────────────────────────────────────────────────────────────────────── + +// TestAdminAuth_Issue120_PatchWorkspace_NoBearer_Returns401 documents the #120 +// attack vector and verifies that AdminAuth returns 401 for PATCH without a token. +func TestAdminAuth_Issue120_PatchWorkspace_NoBearer_Returns401(t *testing.T) { + mockDB, mock, err := sqlmock.New() + if err != nil { + t.Fatalf("sqlmock.New: %v", err) + } + defer mockDB.Close() + + // HasAnyLiveTokenGlobal returns 1 — at least one workspace is token-enrolled. + mock.ExpectQuery(hasAnyLiveTokenGlobalQuery). + WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(1)) + + r := gin.New() + // Mirror the PR #125 router change: PATCH is inside the wsAdmin AdminAuth group. + r.PATCH("/workspaces/:id", AdminAuth(mockDB), func(c *gin.Context) { + c.JSON(http.StatusOK, gin.H{"status": "updated"}) + }) + + w := httptest.NewRecorder() + // #120 attack: no Authorization header on PATCH. + req, _ := http.NewRequest(http.MethodPatch, + "/workspaces/00000000-0000-0000-0000-000000000000", + nil) + r.ServeHTTP(w, req) + + if w.Code != http.StatusUnauthorized { + t.Errorf("#120 PATCH no-bearer: expected 401, got %d: %s", w.Code, w.Body.String()) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations: %v", err) + } +}