From 481b5cfb1af4e432e46dc27325516d6a1f83af6f Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Sun, 19 Apr 2026 01:28:13 -0700 Subject: [PATCH] =?UTF-8?q?fix(security):=20C4=20=E2=80=94=20close=20Admin?= =?UTF-8?q?Auth=20fail-open=20race=20on=20hosted-SaaS=20fresh=20install?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pre-launch review blocker. AdminAuth's Tier-1 fail-open fired whenever the workspace_auth_tokens table was empty — including the window between a hosted tenant EC2 booting and the first workspace being created. In that window, every admin-gated route (POST /org/import, POST /workspaces, POST /bundles/import, etc.) was reachable without a bearer, letting an attacker pre-empt the first real user by importing a hostile workspace into a freshly provisioned instance. Fix: fail-open is now ONLY applied when ADMIN_TOKEN is unset (self- hosted dev with zero auth configured). Hosted SaaS always sets ADMIN_TOKEN at provision time, so the branch never fires in prod and requests with no bearer get 401 even before the first token is minted. Tier-2 / Tier-3 paths unchanged. The old TestAdminAuth_684_FailOpen_AdminTokenSet_NoGlobalTokens test was codifying exactly this bug (asserting 200 on fresh install with ADMIN_TOKEN set). Renamed and flipped to TestAdminAuth_C4_AdminTokenSet_FreshInstall_FailsClosed asserting 401. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../internal/middleware/wsauth_middleware.go | 17 +++++++++++++---- .../middleware/wsauth_middleware_test.go | 18 ++++++++++++------ 2 files changed, 25 insertions(+), 10 deletions(-) diff --git a/workspace-server/internal/middleware/wsauth_middleware.go b/workspace-server/internal/middleware/wsauth_middleware.go index aed59f33..2759586d 100644 --- a/workspace-server/internal/middleware/wsauth_middleware.go +++ b/workspace-server/internal/middleware/wsauth_middleware.go @@ -93,6 +93,7 @@ func WorkspaceAuth(database *sql.DB) gin.HandlerFunc { func AdminAuth(database *sql.DB) gin.HandlerFunc { return func(c *gin.Context) { ctx := c.Request.Context() + adminSecret := os.Getenv("ADMIN_TOKEN") hasLive, err := wsauth.HasAnyLiveTokenGlobal(ctx, database) if err != nil { @@ -101,9 +102,17 @@ func AdminAuth(database *sql.DB) gin.HandlerFunc { return } if !hasLive { - // Tier 1: fail-open on fresh install / pre-Phase-30 upgrade. - c.Next() - return + // Tier 1: fail-open is ONLY safe when ADMIN_TOKEN is unset + // (self-hosted dev, pre-Phase-30 upgrade). Hosted SaaS always + // sets ADMIN_TOKEN at provision time, and C4 (SaaS-launch + // blocker) showed that without this guard an attacker can + // pre-empt the first user by POSTing /org/import before any + // token gets minted. When ADMIN_TOKEN is set we fall through + // into the same bearer-check path Tier-2 uses below. + if adminSecret == "" { + c.Next() + return + } } // Bearer token is the ONLY accepted credential for admin routes. @@ -115,7 +124,7 @@ func AdminAuth(database *sql.DB) gin.HandlerFunc { // Tier 2 (#684 fix): dedicated ADMIN_TOKEN — workspace bearer tokens // must not grant access to admin routes. - if adminSecret := os.Getenv("ADMIN_TOKEN"); adminSecret != "" { + if adminSecret != "" { if subtle.ConstantTimeCompare([]byte(tok), []byte(adminSecret)) != 1 { c.AbortWithStatusJSON(http.StatusUnauthorized, gin.H{"error": "invalid admin auth token"}) return diff --git a/workspace-server/internal/middleware/wsauth_middleware_test.go b/workspace-server/internal/middleware/wsauth_middleware_test.go index e5b157ed..205efd2c 100644 --- a/workspace-server/internal/middleware/wsauth_middleware_test.go +++ b/workspace-server/internal/middleware/wsauth_middleware_test.go @@ -1137,9 +1137,14 @@ func TestAdminAuth_684_AdminTokenNotSet_FallsBackToWorkspaceToken(t *testing.T) } // TestAdminAuth_684_FailOpen_AdminTokenSet_NoGlobalTokens — even when -// ADMIN_TOKEN is set, a fresh install (no tokens globally) must still -// fail-open (tier-1 contract unchanged). -func TestAdminAuth_684_FailOpen_AdminTokenSet_NoGlobalTokens(t *testing.T) { +// Regression for SaaS-launch blocker C4: when ADMIN_TOKEN is set, a +// fresh install (zero live workspace tokens) MUST fail closed. Hosted +// SaaS tenants boot with ADMIN_TOKEN set but an empty tokens table — +// without this guard, an anonymous caller can POST /org/import or +// /workspaces before the first real user and pre-empt the instance. +// Fail-open is only acceptable when ADMIN_TOKEN is also unset +// (self-hosted dev with zero auth configured). +func TestAdminAuth_C4_AdminTokenSet_FreshInstall_FailsClosed(t *testing.T) { mockDB, mock, err := sqlmock.New() if err != nil { t.Fatalf("sqlmock.New: %v", err) @@ -1159,11 +1164,12 @@ func TestAdminAuth_684_FailOpen_AdminTokenSet_NoGlobalTokens(t *testing.T) { w := httptest.NewRecorder() req, _ := http.NewRequest(http.MethodGet, "/admin/secrets", nil) - // No bearer — but fail-open should still pass. + // No bearer — ADMIN_TOKEN is set so the no-tokens tier-1 escape + // no longer applies; the request must be rejected. r.ServeHTTP(w, req) - if w.Code != http.StatusOK { - t.Errorf("#684 fail-open w/ ADMIN_TOKEN set (no global tokens): expected 200, got %d: %s", + if w.Code != http.StatusUnauthorized { + t.Errorf("C4 fresh-install w/ ADMIN_TOKEN set: expected 401, got %d: %s", w.Code, w.Body.String()) } if err := mock.ExpectationsWereMet(); err != nil {