forked from molecule-ai/molecule-core
fix(security): remove canvasOriginAllowed from AdminAuth middleware (#623)
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 <noreply@anthropic.com>
This commit is contained in:
parent
0dd7437fef
commit
4810863a40
@ -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
|
||||
}
|
||||
|
||||
@ -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)
|
||||
}
|
||||
}
|
||||
|
||||
Loading…
Reference in New Issue
Block a user