From 4810863a4051f5e3379252e16fdaf2c200cd4528 Mon Sep 17 00:00:00 2001 From: Molecule AI Backend Engineer Date: Fri, 17 Apr 2026 06:00:45 +0000 Subject: [PATCH] fix(security): remove canvasOriginAllowed from AdminAuth middleware (#623) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Origin header is trivially forgeable by any container on the Docker network. Having canvasOriginAllowed() / isSameOriginCanvas() as auth bypass paths in AdminAuth let any curl/container without a bearer token reach /settings/secrets, /bundles/import, /bundles/export, /events, and all other AdminAuth-gated routes by forging Origin: http://localhost:3000. Fix: remove both Origin bypass branches from AdminAuth. Bearer token is now the only accepted credential. Lazy-bootstrap fail-open (zero tokens → pass-through) is preserved for fresh installs. CanvasOrBearer retains the Origin bypass because it is scoped exclusively to cosmetic routes (PUT /canvas/viewport) where a forged request has zero security impact — worst case is viewport position corruption. Added 3 regression tests: - TestAdminAuth_623_ForgedOrigin_Returns401 - TestAdminAuth_623_ForgedCORSOrigin_Returns401 - TestAdminAuth_623_ValidBearer_WithOrigin_Passes Closes #623, Closes #626 Co-Authored-By: Claude Sonnet 4.6 --- .../internal/middleware/wsauth_middleware.go | 21 ++-- .../middleware/wsauth_middleware_test.go | 113 ++++++++++++++++++ 2 files changed, 122 insertions(+), 12 deletions(-) diff --git a/platform/internal/middleware/wsauth_middleware.go b/platform/internal/middleware/wsauth_middleware.go index 5b06c576..5e24a745 100644 --- a/platform/internal/middleware/wsauth_middleware.go +++ b/platform/internal/middleware/wsauth_middleware.go @@ -67,10 +67,17 @@ func WorkspaceAuth(database *sql.DB) gin.HandlerFunc { // 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 one. +// has a live token every request to these routes MUST present a valid bearer +// token — no Origin-based bypass. (#623) // // 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. +// +// 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). func AdminAuth(database *sql.DB) gin.HandlerFunc { return func(c *gin.Context) { ctx := c.Request.Context() @@ -82,7 +89,7 @@ func AdminAuth(database *sql.DB) gin.HandlerFunc { return } if hasLive { - // Bearer token path — agents, CLI, and API clients. + // 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 { @@ -92,16 +99,6 @@ func AdminAuth(database *sql.DB) gin.HandlerFunc { c.Next() return } - // Canvas origin path — cross-origin canvas (CORS_ORIGINS match). - if canvasOriginAllowed(c.GetHeader("Origin")) { - c.Next() - return - } - // Same-origin canvas path — tenant image where canvas + API share a host. - if isSameOriginCanvas(c) { - c.Next() - return - } c.AbortWithStatusJSON(http.StatusUnauthorized, gin.H{"error": "admin auth required"}) return } diff --git a/platform/internal/middleware/wsauth_middleware_test.go b/platform/internal/middleware/wsauth_middleware_test.go index 2f062f41..7ee95ba7 100644 --- a/platform/internal/middleware/wsauth_middleware_test.go +++ b/platform/internal/middleware/wsauth_middleware_test.go @@ -778,3 +778,116 @@ func TestCanvasOriginAllowed_LocalhostDefault(t *testing.T) { t.Error("random origin should not be allowed") } } + +// ── Issue #623 regression ───────────────────────────────────────────────────── +// AdminAuth must NOT accept forged Origin headers. Any container on the Docker +// network can set Origin: http://localhost:3000 without a bearer token, which +// previously bypassed AdminAuth on ALL admin-gated routes. (#623, dup #626) + +// TestAdminAuth_623_ForgedOrigin_Returns401 — the main regression test: +// a request with a matching CORS origin but no bearer token must be rejected. +func TestAdminAuth_623_ForgedOrigin_Returns401(t *testing.T) { + mockDB, mock, err := sqlmock.New() + if err != nil { + t.Fatalf("sqlmock: %v", err) + } + defer mockDB.Close() + + // Platform has live tokens — AdminAuth is active. + mock.ExpectQuery(hasAnyLiveTokenGlobalQuery). + WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(1)) + + t.Setenv("CORS_ORIGINS", "http://localhost:3000") + + r := gin.New() + r.GET("/settings/secrets", AdminAuth(mockDB), func(c *gin.Context) { + c.JSON(http.StatusOK, gin.H{"secrets": []string{"OPENAI_API_KEY"}}) + }) + + w := httptest.NewRecorder() + // #623 attack: forge the canvas Origin header — no bearer token. + req, _ := http.NewRequest(http.MethodGet, "/settings/secrets", nil) + req.Header.Set("Origin", "http://localhost:3000") + r.ServeHTTP(w, req) + + if w.Code != http.StatusUnauthorized { + t.Errorf("#623 forged Origin bypass: expected 401, got %d: %s", w.Code, w.Body.String()) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations: %v", err) + } +} + +// TestAdminAuth_623_ForgedCORSOrigin_Returns401 — variant: attacker uses the +// tenant-domain CORS origin from CORS_ORIGINS (not just localhost). +func TestAdminAuth_623_ForgedCORSOrigin_Returns401(t *testing.T) { + mockDB, mock, err := sqlmock.New() + if err != nil { + t.Fatalf("sqlmock: %v", err) + } + defer mockDB.Close() + + mock.ExpectQuery(hasAnyLiveTokenGlobalQuery). + WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(1)) + + t.Setenv("CORS_ORIGINS", "https://acme.moleculesai.app") + + 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("Origin", "https://acme.moleculesai.app") + r.ServeHTTP(w, req) + + if w.Code != http.StatusUnauthorized { + t.Errorf("#623 forged tenant Origin: expected 401, got %d: %s", w.Code, w.Body.String()) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations: %v", err) + } +} + +// TestAdminAuth_623_ValidBearer_WithOrigin_Passes — bearer + matching Origin +// should still work (the Origin is irrelevant once the bearer validates). +func TestAdminAuth_623_ValidBearer_WithOrigin_Passes(t *testing.T) { + mockDB, mock, err := sqlmock.New() + if err != nil { + t.Fatalf("sqlmock: %v", err) + } + defer mockDB.Close() + + goodToken := "valid-bearer-token-xyz" + tokenHash := sha256.Sum256([]byte(goodToken)) + + mock.ExpectQuery(hasAnyLiveTokenGlobalQuery). + WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(1)) + mock.ExpectQuery(validateAnyTokenSelectQuery). + WithArgs(tokenHash[:]). + WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow("tok-1")) + mock.ExpectExec(validateTokenUpdateQuery). + WithArgs("tok-1"). + WillReturnResult(sqlmock.NewResult(0, 1)) + + t.Setenv("CORS_ORIGINS", "http://localhost:3000") + + r := gin.New() + r.GET("/settings/secrets", AdminAuth(mockDB), func(c *gin.Context) { + c.JSON(http.StatusOK, gin.H{"ok": true}) + }) + + w := httptest.NewRecorder() + req, _ := http.NewRequest(http.MethodGet, "/settings/secrets", nil) + req.Header.Set("Authorization", "Bearer "+goodToken) + req.Header.Set("Origin", "http://localhost:3000") // present but irrelevant + r.ServeHTTP(w, req) + + if w.Code != http.StatusOK { + t.Errorf("bearer+origin: expected 200, got %d: %s", w.Code, w.Body.String()) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations: %v", err) + } +}