From 9122d6aeea3cf714843a714fd19687c1880e9452 Mon Sep 17 00:00:00 2001 From: Backend Engineer Date: Wed, 15 Apr 2026 17:25:09 +0000 Subject: [PATCH] fix(security): gate GET /approvals/pending behind AdminAuth (#180) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GET /approvals/pending was registered on the open router with no middleware, allowing any unauthenticated caller to enumerate all pending approvals across every workspace on the platform. Fix: add inline middleware.AdminAuth(db.DB) to the route registration, matching the pattern used in PR #167 for bundles, events, and viewport. The three workspace-scoped approvals routes (POST/GET /approvals, POST /approvals/:id/decide) were already correctly behind WorkspaceAuth inside the wsAuth group — no change needed there. Tests: two new regression tests in wsauth_middleware_test.go — TestAdminAuth_Issue180_ApprovalsListing_NoBearer_Returns401 TestAdminAuth_Issue180_ApprovalsListing_FailOpen_NoTokens Co-Authored-By: Claude Sonnet 4.6 --- .../middleware/wsauth_middleware_test.go | 76 +++++++++++++++++++ platform/internal/router/router.go | 7 +- 2 files changed, 81 insertions(+), 2 deletions(-) diff --git a/platform/internal/middleware/wsauth_middleware_test.go b/platform/internal/middleware/wsauth_middleware_test.go index 95b79bbb..5d677170 100644 --- a/platform/internal/middleware/wsauth_middleware_test.go +++ b/platform/internal/middleware/wsauth_middleware_test.go @@ -486,6 +486,82 @@ func TestAdminAuth_InvalidBearer_Returns401(t *testing.T) { // requests — the route-level fix in router.go is the enforcement point. // ──────────────────────────────────────────────────────────────────────────── +// ──────────────────────────────────────────────────────────────────────────── +// Issue #180 regression — unauthenticated GET /approvals/pending +// +// GET /approvals/pending was registered on the open router (no middleware) +// and returned all pending approvals across every workspace to any caller, +// with no token required. +// Attack vector confirmed by Security Auditor: +// curl http://host/approvals/pending → 200 with full cross-workspace list +// +// Fixed by adding inline AdminAuth to the route registration in router.go. +// This test asserts the gate blocks unauthenticated reads. +// ──────────────────────────────────────────────────────────────────────────── + +// TestAdminAuth_Issue180_ApprovalsListing_NoBearer_Returns401 documents the #180 +// attack vector and verifies that AdminAuth returns 401 for GET without a token. +func TestAdminAuth_Issue180_ApprovalsListing_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 router.go fix: GET /approvals/pending is behind AdminAuth. + r.GET("/approvals/pending", AdminAuth(mockDB), func(c *gin.Context) { + c.JSON(http.StatusOK, gin.H{"approvals": []interface{}{}}) + }) + + w := httptest.NewRecorder() + // #180 attack: no Authorization header on GET /approvals/pending. + req, _ := http.NewRequest(http.MethodGet, "/approvals/pending", nil) + r.ServeHTTP(w, req) + + if w.Code != http.StatusUnauthorized { + t.Errorf("#180 GET /approvals/pending 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) + } +} + +// TestAdminAuth_Issue180_ApprovalsListing_FailOpen_NoTokens documents the +// fail-open contract: on a fresh install (no tokens anywhere), the middleware +// must not block the canvas from polling /approvals/pending. +func TestAdminAuth_Issue180_ApprovalsListing_FailOpen_NoTokens(t *testing.T) { + mockDB, mock, err := sqlmock.New() + if err != nil { + t.Fatalf("sqlmock.New: %v", err) + } + defer mockDB.Close() + + // HasAnyLiveTokenGlobal returns 0 — fresh install, no tokens yet. + mock.ExpectQuery(hasAnyLiveTokenGlobalQuery). + WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(0)) + + r := gin.New() + r.GET("/approvals/pending", AdminAuth(mockDB), func(c *gin.Context) { + c.JSON(http.StatusOK, gin.H{"approvals": []interface{}{}}) + }) + + w := httptest.NewRecorder() + req, _ := http.NewRequest(http.MethodGet, "/approvals/pending", nil) + r.ServeHTTP(w, req) + + if w.Code != http.StatusOK { + t.Errorf("#180 fail-open (no tokens): expected 200, got %d: %s", w.Code, w.Body.String()) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations: %v", err) + } +} + // 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) { diff --git a/platform/internal/router/router.go b/platform/internal/router/router.go index e83cfd24..58ce2fd2 100644 --- a/platform/internal/router/router.go +++ b/platform/internal/router/router.go @@ -166,8 +166,11 @@ func Setup(hub *ws.Hub, broadcaster *events.Broadcaster, prov *provisioner.Provi wsAuth.POST("/approvals", apph.Create) wsAuth.GET("/approvals", apph.List) wsAuth.POST("/approvals/:approvalId/decide", apph.Decide) - // /approvals/pending is a cross-workspace admin path; keep on root router outside wsAuth. - r.GET("/approvals/pending", apph.ListAll) + // /approvals/pending is a cross-workspace admin path; WorkspaceAuth cannot + // be used here (no workspace scope), but it still needs auth so an + // unauthenticated caller cannot enumerate all pending approvals across the + // entire platform. Gated behind AdminAuth (issue #180). + r.GET("/approvals/pending", middleware.AdminAuth(db.DB), apph.ListAll) // Team Expansion teamh := handlers.NewTeamHandler(broadcaster, prov, platformURL, configsDir)