Merge pull request #729 from Molecule-AI/fix/issue-684-adminauth-bearer-scope

fix(auth): AdminAuth rejects workspace bearer tokens when ADMIN_TOKEN is set (#684)

platform/internal/middleware/wsauth_middleware.go

@@ -1,6 +1,7 @@
 package middleware
 
 import (
+	"crypto/subtle"
 	"database/sql"
 	"log"
 	"net/http"
@@ -64,20 +65,31 @@ func WorkspaceAuth(database *sql.DB) gin.HandlerFunc {
 // AdminAuth returns a Gin middleware for global/admin routes (e.g.
 // /settings/secrets, /admin/secrets) that have no per-workspace scope.
 //
-// Same lazy-bootstrap contract as WorkspaceAuth: if no live token exists
-// anywhere on the platform (fresh install / pre-Phase-30 upgrade), requests
-// are let through so existing deployments keep working. Once any workspace
-// has a live token every request to these routes MUST present a valid bearer
-// token — no Origin-based bypass. (#623)
+// # Credential tier (evaluated in order)
 //
-// Any valid workspace bearer token is accepted — the route is not scoped to
-// a specific workspace so we only verify the token is live and unrevoked.
+//  1. Lazy-bootstrap fail-open: if no live workspace token exists anywhere on
+//     the platform (fresh install / pre-Phase-30 upgrade), every request passes
+//     through so existing deployments keep working.
+//
+//  2. ADMIN_TOKEN env var (recommended, closes #684): when set, the bearer
+//     MUST equal this value exactly (constant-time comparison). Workspace
+//     bearer tokens are intentionally rejected even if valid — a compromised
+//     workspace agent must not be able to read global secrets, steal GitHub App
+//     installation tokens, or enumerate pending approvals across the platform.
+//     Set ADMIN_TOKEN to a strong random secret (e.g. openssl rand -base64 32).
+//
+//  3. Fallback — workspace token (deprecated, backward-compat): when
+//     ADMIN_TOKEN is not set and workspace tokens do exist globally, any valid
+//     workspace bearer token is still accepted. This preserves existing
+//     behaviour for deployments that have not yet configured ADMIN_TOKEN, but
+//     it leaves the blast-radius isolation gap described in #684 open. Set
+//     ADMIN_TOKEN to eliminate this fallback.
 //
 // NOTE: canvasOriginAllowed / isSameOriginCanvas are intentionally NOT called
 // here.  The Origin header is trivially forgeable by any container on the
 // Docker network; using it as an auth bypass would let an attacker reach
 // /settings/secrets, /bundles/import, /events, etc. without a bearer token.
-// Those short-circuits belong ONLY in CanvasOrBearer (cosmetic routes).
+// Those short-circuits belong ONLY in CanvasOrBearer (cosmetic routes). (#623)
 func AdminAuth(database *sql.DB) gin.HandlerFunc {
 	return func(c *gin.Context) {
 		ctx := c.Request.Context()
@@ -88,18 +100,34 @@ func AdminAuth(database *sql.DB) gin.HandlerFunc {
 			c.AbortWithStatusJSON(http.StatusInternalServerError, gin.H{"error": "auth check failed"})
 			return
 		}
-		if hasLive {
-			// Bearer token is the ONLY accepted credential for admin routes.
-			tok := wsauth.BearerTokenFromHeader(c.GetHeader("Authorization"))
-			if tok != "" {
-				if err := wsauth.ValidateAnyToken(ctx, database, tok); err != nil {
-					c.AbortWithStatusJSON(http.StatusUnauthorized, gin.H{"error": "invalid admin auth token"})
-					return
-				}
-				c.Next()
+		if !hasLive {
+			// Tier 1: fail-open on fresh install / pre-Phase-30 upgrade.
+			c.Next()
+			return
+		}
+
+		// Bearer token is the ONLY accepted credential for admin routes.
+		tok := wsauth.BearerTokenFromHeader(c.GetHeader("Authorization"))
+		if tok == "" {
+			c.AbortWithStatusJSON(http.StatusUnauthorized, gin.H{"error": "admin auth required"})
+			return
+		}
+
+		// 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 subtle.ConstantTimeCompare([]byte(tok), []byte(adminSecret)) != 1 {
+				c.AbortWithStatusJSON(http.StatusUnauthorized, gin.H{"error": "invalid admin auth token"})
 				return
 			}
-			c.AbortWithStatusJSON(http.StatusUnauthorized, gin.H{"error": "admin auth required"})
+			c.Next()
+			return
+		}
+
+		// Tier 3 (deprecated): ADMIN_TOKEN not configured — fall back to any
+		// valid workspace token. Operators should set ADMIN_TOKEN to close #684.
+		if err := wsauth.ValidateAnyToken(ctx, database, tok); err != nil {
+			c.AbortWithStatusJSON(http.StatusUnauthorized, gin.H{"error": "invalid admin auth token"})
 			return
 		}
 		c.Next()

platform/internal/middleware/wsauth_middleware_test.go

@@ -940,3 +940,233 @@ func TestAdminAuth_623_ValidBearer_WithOrigin_Passes(t *testing.T) {
 		t.Errorf("unmet sqlmock expectations: %v", err)
 	}
 }
+
+// ── Issue #684 — AdminAuth accepts any workspace bearer as admin credential ──
+//
+// Root cause: AdminAuth called ValidateAnyToken which matched any live
+// workspace token.  A compromised workspace agent could present its own bearer
+// and reach /admin/github-installation-token, /approvals/pending, etc.
+//
+// Fix: when ADMIN_TOKEN env var is set the middleware verifies the bearer
+// against that secret exclusively (constant-time).  Workspace tokens are
+// rejected even if valid.  When ADMIN_TOKEN is not set the old behaviour is
+// preserved for backward-compat (deprecated fallback, tier 3).
+
+// TestAdminAuth_684_AdminTokenSet_WorkspaceTokenRejected — the primary
+// regression test: when ADMIN_TOKEN is configured, a valid workspace bearer
+// token MUST be rejected with 401 on admin routes (#684).
+func TestAdminAuth_684_AdminTokenSet_WorkspaceTokenRejected(t *testing.T) {
+	mockDB, mock, err := sqlmock.New()
+	if err != nil {
+		t.Fatalf("sqlmock.New: %v", err)
+	}
+	defer mockDB.Close()
+
+	t.Setenv("ADMIN_TOKEN", "super-secret-admin-token-xyz")
+
+	// Platform has live workspace tokens — AdminAuth is active.
+	mock.ExpectQuery(hasAnyLiveTokenGlobalQuery).
+		WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(1))
+
+	// ValidateAnyToken must NOT be called — workspace tokens must be rejected
+	// before any DB lookup when ADMIN_TOKEN is set.
+
+	r := gin.New()
+	r.GET("/admin/github-installation-token", AdminAuth(mockDB), func(c *gin.Context) {
+		c.JSON(http.StatusOK, gin.H{"token": "ghp_live_token"})
+	})
+
+	w := httptest.NewRecorder()
+	// #684 attack: compromised workspace agent sends its own bearer.
+	req, _ := http.NewRequest(http.MethodGet, "/admin/github-installation-token", nil)
+	req.Header.Set("Authorization", "Bearer some-valid-workspace-bearer-token")
+	r.ServeHTTP(w, req)
+
+	if w.Code != http.StatusUnauthorized {
+		t.Errorf("#684 workspace token w/ ADMIN_TOKEN set: expected 401, got %d: %s",
+			w.Code, w.Body.String())
+	}
+	if err := mock.ExpectationsWereMet(); err != nil {
+		t.Errorf("unmet sqlmock expectations: %v", err)
+	}
+}
+
+// TestAdminAuth_684_AdminTokenSet_CorrectAdminTokenAccepted — when ADMIN_TOKEN
+// is set, presenting the exact ADMIN_TOKEN value must grant access (200).
+func TestAdminAuth_684_AdminTokenSet_CorrectAdminTokenAccepted(t *testing.T) {
+	mockDB, mock, err := sqlmock.New()
+	if err != nil {
+		t.Fatalf("sqlmock.New: %v", err)
+	}
+	defer mockDB.Close()
+
+	const adminSecret = "super-secret-admin-token-xyz"
+	t.Setenv("ADMIN_TOKEN", adminSecret)
+
+	mock.ExpectQuery(hasAnyLiveTokenGlobalQuery).
+		WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(1))
+
+	// No DB token lookup — ADMIN_TOKEN check is env-only, no DB round-trip.
+
+	r := gin.New()
+	r.GET("/admin/github-installation-token", AdminAuth(mockDB), func(c *gin.Context) {
+		c.JSON(http.StatusOK, gin.H{"token": "ghp_live_token"})
+	})
+
+	w := httptest.NewRecorder()
+	req, _ := http.NewRequest(http.MethodGet, "/admin/github-installation-token", nil)
+	req.Header.Set("Authorization", "Bearer "+adminSecret)
+	r.ServeHTTP(w, req)
+
+	if w.Code != http.StatusOK {
+		t.Errorf("#684 correct ADMIN_TOKEN: expected 200, got %d: %s", w.Code, w.Body.String())
+	}
+	if err := mock.ExpectationsWereMet(); err != nil {
+		t.Errorf("unmet sqlmock expectations: %v", err)
+	}
+}
+
+// TestAdminAuth_684_AdminTokenSet_WrongAdminToken_Returns401 — when ADMIN_TOKEN
+// is set, presenting a different value must return 401.
+func TestAdminAuth_684_AdminTokenSet_WrongAdminToken_Returns401(t *testing.T) {
+	mockDB, mock, err := sqlmock.New()
+	if err != nil {
+		t.Fatalf("sqlmock.New: %v", err)
+	}
+	defer mockDB.Close()
+
+	t.Setenv("ADMIN_TOKEN", "correct-admin-secret")
+
+	mock.ExpectQuery(hasAnyLiveTokenGlobalQuery).
+		WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(1))
+
+	r := gin.New()
+	r.GET("/admin/liveness", AdminAuth(mockDB), func(c *gin.Context) {
+		c.JSON(http.StatusOK, gin.H{"subsystems": gin.H{}})
+	})
+
+	w := httptest.NewRecorder()
+	req, _ := http.NewRequest(http.MethodGet, "/admin/liveness", nil)
+	req.Header.Set("Authorization", "Bearer wrong-admin-secret")
+	r.ServeHTTP(w, req)
+
+	if w.Code != http.StatusUnauthorized {
+		t.Errorf("#684 wrong ADMIN_TOKEN: expected 401, got %d: %s", w.Code, w.Body.String())
+	}
+	if err := mock.ExpectationsWereMet(); err != nil {
+		t.Errorf("unmet sqlmock expectations: %v", err)
+	}
+}
+
+// TestAdminAuth_684_AdminTokenSet_NoBearer_Returns401 — when ADMIN_TOKEN is
+// set, a request with no bearer must still return 401.
+func TestAdminAuth_684_AdminTokenSet_NoBearer_Returns401(t *testing.T) {
+	mockDB, mock, err := sqlmock.New()
+	if err != nil {
+		t.Fatalf("sqlmock.New: %v", err)
+	}
+	defer mockDB.Close()
+
+	t.Setenv("ADMIN_TOKEN", "correct-admin-secret")
+
+	mock.ExpectQuery(hasAnyLiveTokenGlobalQuery).
+		WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(1))
+
+	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.StatusUnauthorized {
+		t.Errorf("#684 no bearer w/ ADMIN_TOKEN set: expected 401, got %d: %s",
+			w.Code, w.Body.String())
+	}
+	if err := mock.ExpectationsWereMet(); err != nil {
+		t.Errorf("unmet sqlmock expectations: %v", err)
+	}
+}
+
+// TestAdminAuth_684_AdminTokenNotSet_FallsBackToWorkspaceToken — when
+// ADMIN_TOKEN is NOT set, a valid workspace token is still accepted (deprecated
+// tier-3 fallback for backward compatibility).
+func TestAdminAuth_684_AdminTokenNotSet_FallsBackToWorkspaceToken(t *testing.T) {
+	mockDB, mock, err := sqlmock.New()
+	if err != nil {
+		t.Fatalf("sqlmock.New: %v", err)
+	}
+	defer mockDB.Close()
+
+	// ADMIN_TOKEN explicitly unset — tier-3 fallback active.
+	t.Setenv("ADMIN_TOKEN", "")
+
+	workspaceToken := "any-live-workspace-token"
+	tokenHash := sha256.Sum256([]byte(workspaceToken))
+
+	mock.ExpectQuery(hasAnyLiveTokenGlobalQuery).
+		WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(1))
+
+	mock.ExpectQuery(validateAnyTokenSelectQuery).
+		WithArgs(tokenHash[:]).
+		WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow("tok-ws-1"))
+
+	mock.ExpectExec(validateTokenUpdateQuery).
+		WithArgs("tok-ws-1").
+		WillReturnResult(sqlmock.NewResult(0, 1))
+
+	r := gin.New()
+	r.GET("/admin/secrets", AdminAuth(mockDB), func(c *gin.Context) {
+		c.JSON(http.StatusOK, gin.H{"ok": true})
+	})
+
+	w := httptest.NewRecorder()
+	req, _ := http.NewRequest(http.MethodGet, "/admin/secrets", nil)
+	req.Header.Set("Authorization", "Bearer "+workspaceToken)
+	r.ServeHTTP(w, req)
+
+	if w.Code != http.StatusOK {
+		t.Errorf("#684 fallback (no ADMIN_TOKEN): expected 200, got %d: %s",
+			w.Code, w.Body.String())
+	}
+	if err := mock.ExpectationsWereMet(); err != nil {
+		t.Errorf("unmet sqlmock expectations: %v", err)
+	}
+}
+
+// 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) {
+	mockDB, mock, err := sqlmock.New()
+	if err != nil {
+		t.Fatalf("sqlmock.New: %v", err)
+	}
+	defer mockDB.Close()
+
+	t.Setenv("ADMIN_TOKEN", "some-admin-secret")
+
+	// HasAnyLiveTokenGlobal returns 0 — fresh install.
+	mock.ExpectQuery(hasAnyLiveTokenGlobalQuery).
+		WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(0))
+
+	r := gin.New()
+	r.GET("/admin/secrets", AdminAuth(mockDB), func(c *gin.Context) {
+		c.JSON(http.StatusOK, gin.H{"ok": true})
+	})
+
+	w := httptest.NewRecorder()
+	req, _ := http.NewRequest(http.MethodGet, "/admin/secrets", nil)
+	// No bearer — but fail-open should still pass.
+	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",
+			w.Code, w.Body.String())
+	}
+	if err := mock.ExpectationsWereMet(); err != nil {
+		t.Errorf("unmet sqlmock expectations: %v", err)
+	}
+}