From 786e3904745a5d9f33febc25b4a2d04d085bec9b Mon Sep 17 00:00:00 2001 From: kzoeps Date: Wed, 3 Jun 2026 12:25:33 +0600 Subject: [PATCH] fix(cors): split public and admin GraphQL origins --- .../unreleased/split-graphql-admin-cors.yaml | 5 + .env.example | 16 ++- AGENTS.md | 2 +- README.md | 15 +- cmd/hyperindex/main.go | 120 ++++++++-------- cmd/hyperindex/main_test.go | 128 +++++++++++++++++- internal/config/config.go | 92 ++++++++++++- internal/config/config_test.go | 96 +++++++++++++ internal/graphql/admin/handler.go | 2 +- internal/graphql/handler.go | 2 +- internal/graphql/handler_test.go | 4 +- internal/graphql/subscription/handler.go | 55 +++++--- internal/graphql/subscription/handler_test.go | 15 +- internal/server/cors.go | 100 ++++++++++---- internal/server/cors_test.go | 100 ++++++++++++++ 15 files changed, 628 insertions(+), 124 deletions(-) create mode 100644 .changes/unreleased/split-graphql-admin-cors.yaml create mode 100644 internal/server/cors_test.go diff --git a/.changes/unreleased/split-graphql-admin-cors.yaml b/.changes/unreleased/split-graphql-admin-cors.yaml new file mode 100644 index 00000000..48a11411 --- /dev/null +++ b/.changes/unreleased/split-graphql-admin-cors.yaml @@ -0,0 +1,5 @@ +kind: fixed +body: Split CORS configuration so public GraphQL allows browser clients by default while admin GraphQL requires explicit trusted origins. +time: 2026-06-03T11:42:23.428959884+06:00 +custom: + Affects: operator diff --git a/.env.example b/.env.example index d5cf1be2..685f17af 100644 --- a/.env.example +++ b/.env.example @@ -35,10 +35,18 @@ SECRET_KEY_BASE=CHANGE_ME_TO_A_RANDOM_64_CHARACTER_STRING_USE_OPENSSL_RAND # Replace this placeholder with a real random secret before production use. ADMIN_API_KEY=replace-with-a-random-secret -# Allowed origins for CORS and WebSocket connections (comma-separated) -# Empty or unset = allow all origins. Set explicit origins in production. -# Examples: https://myapp.com,https://admin.myapp.com -# ALLOWED_ORIGINS= +# Browser origins for public GraphQL HTTP and subscription APIs. +# Empty or "*" allows every origin so independent frontends can query public indexed data. +# Set a comma-separated list only if you need to restrict public browser access. +PUBLIC_ALLOWED_ORIGINS=* + +# Browser origins for admin GraphQL. Empty keeps admin GraphQL same-origin only. +# Set explicit trusted admin frontend origins when the admin UI is hosted separately. +# Wildcard "*" is rejected for admin routes. +# ADMIN_ALLOWED_ORIGINS=https://admin.example.com + +# Deprecated: ALLOWED_ORIGINS is kept only as a compatibility fallback for explicit admin origins. +# Use PUBLIC_ALLOWED_ORIGINS and ADMIN_ALLOWED_ORIGINS instead. # Admin DIDs (comma-separated) - users with admin access to the dashboard # This is the backend source of truth and is read-only in the admin UI. diff --git a/AGENTS.md b/AGENTS.md index 34bfadf4..a3d454a1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -87,7 +87,7 @@ Run verification based on what changed. - `TAP_ENABLED=true` switches record ingestion to Tap mode. - `LABELER_SUBSCRIBE_ENABLED=true` with `LABELER_SUBSCRIBE_URLS` starts optional external `com.atproto.label.subscribeLabels` ingestion. - Migrations run automatically on startup. -- Be careful with `ALLOWED_ORIGINS`: current code allows all origins when unset, even if older prose suggests stricter defaults. +- CORS origin config is split by route group: `PUBLIC_ALLOWED_ORIGINS` controls public GraphQL/OAuth browser access and defaults to `*`; `ADMIN_ALLOWED_ORIGINS` must list trusted admin frontend origins explicitly and rejects wildcard `*`. Deprecated `ALLOWED_ORIGINS` is only a compatibility fallback for explicit admin origins. ## Changie fragments diff --git a/README.md b/README.md index d5d00458..883af4f9 100644 --- a/README.md +++ b/README.md @@ -392,9 +392,18 @@ SECRET_KEY_BASE=your-secret-key-at-least-64-characters-long-generate-with-openss # Example: openssl rand -base64 32 ADMIN_API_KEY=replace-with-a-random-secret -# WebSocket origins — comma-separated allowed origins for subscriptions. -# Unset or empty allows all origins. Set a comma-separated list to restrict origins; "*" also allows all origins. -# ALLOWED_ORIGINS=https://your-frontend.vercel.app +# Browser origins for public GraphQL HTTP and subscription APIs. +# Empty or "*" allows every origin so independent browser frontends can query public indexed data. +# Set a comma-separated list only if you need to restrict public browser access. +PUBLIC_ALLOWED_ORIGINS=* + +# Browser origins for admin GraphQL. Empty keeps admin GraphQL same-origin only. +# Set explicit trusted admin frontend origins when the admin UI is hosted separately. +# Wildcard "*" is rejected for admin routes. +# ADMIN_ALLOWED_ORIGINS=https://admin.hypercerts.dev + +# Deprecated: ALLOWED_ORIGINS is kept only as a compatibility fallback for explicit admin origins. +# Use PUBLIC_ALLOWED_ORIGINS and ADMIN_ALLOWED_ORIGINS instead. # Tap record ingestion (recommended) # TAP_ENABLED=true diff --git a/cmd/hyperindex/main.go b/cmd/hyperindex/main.go index f7fec34a..85434295 100644 --- a/cmd/hyperindex/main.go +++ b/cmd/hyperindex/main.go @@ -262,19 +262,6 @@ func setupRouter(cfg *config.Config, svc *services, bg *backgroundServices) *chi r.Use(middleware.Recoverer) r.Use(middleware.Timeout(60 * time.Second)) - // CORS — uses AllowedOrigins from config; defaults to "*" if not set - var allowedOrigins []string - if cfg.AllowedOrigins != "" { - for _, o := range strings.Split(cfg.AllowedOrigins, ",") { - allowedOrigins = append(allowedOrigins, strings.TrimSpace(o)) - } - } - allowAdminAPIKeyAuth := cfg.AdminAPIKey != "" - r.Use(server.CORSMiddleware(server.CORSConfig{ - AllowedOrigins: allowedOrigins, - AllowAdminAPIKeyAuth: allowAdminAPIKeyAuth, - })) - // Health check reports process liveness only. Dependency readiness and // deterministic labeler cursor failures are exposed via GET /ready. r.Get("/health", func(w http.ResponseWriter, req *http.Request) { @@ -535,34 +522,59 @@ func setupOAuth(r *chi.Mux, cfg *config.Config, svc *services, bg *backgroundSer AuthorizationCodeExpiration: 600, // 10 minutes }, svc.db) - // Discovery endpoints - r.Get("/.well-known/oauth-authorization-server", oauthHandlers.HandleAuthorizationServerMetadata) - r.Get("/.well-known/oauth-protected-resource", oauthHandlers.HandleProtectedResourceMetadata) - - // Client metadata (this server as an OAuth client) - r.Get("/oauth-client-metadata.json", server.HandleClientMetadata(server.ClientMetadataConfig{ - ExternalBaseURL: cfg.ExternalBaseURL, - ClientName: "Hyperindex", - Scope: "atproto transition:generic", - })) - - // OAuth flow endpoints - r.Get("/oauth/authorize", oauthHandlers.HandleAuthorize) - r.Post("/oauth/authorize", oauthHandlers.HandleAuthorize) - r.Get("/oauth/callback", oauthHandlers.HandleCallback) - r.Post("/oauth/token", oauthHandlers.HandleToken) - r.Get("/oauth/jwks", oauthHandlers.HandleJWKS) - r.Post("/oauth/revoke", oauthHandlers.HandleRevoke) - - // Additional OAuth endpoints - registerHandler := server.NewOAuthRegisterHandler(svc.db) - r.Post("/oauth/register", registerHandler.HandleRegister) - - parHandler := server.NewOAuthPARHandler(svc.db) - r.Post("/oauth/par", parHandler.HandlePAR) + publicCORS := server.CORSMiddleware(server.CORSConfig{ + AllowedOrigins: cfg.PublicAllowedOriginList(), + }) - r.Get("/oauth/dpop/nonce", server.HandleDPoPNonce) - r.Post("/oauth/dpop/nonce", server.HandleDPoPNonce) + r.Group(func(r chi.Router) { + r.Use(publicCORS) + + // Discovery endpoints + r.Get("/.well-known/oauth-authorization-server", oauthHandlers.HandleAuthorizationServerMetadata) + r.Get("/.well-known/oauth-protected-resource", oauthHandlers.HandleProtectedResourceMetadata) + + // Client metadata (this server as an OAuth client) + r.Get("/oauth-client-metadata.json", server.HandleClientMetadata(server.ClientMetadataConfig{ + ExternalBaseURL: cfg.ExternalBaseURL, + ClientName: "Hyperindex", + Scope: "atproto transition:generic", + })) + + // OAuth flow endpoints + r.Get("/oauth/authorize", oauthHandlers.HandleAuthorize) + r.Post("/oauth/authorize", oauthHandlers.HandleAuthorize) + r.Get("/oauth/callback", oauthHandlers.HandleCallback) + r.Post("/oauth/token", oauthHandlers.HandleToken) + r.Get("/oauth/jwks", oauthHandlers.HandleJWKS) + r.Post("/oauth/revoke", oauthHandlers.HandleRevoke) + + // Additional OAuth endpoints + registerHandler := server.NewOAuthRegisterHandler(svc.db) + r.Post("/oauth/register", registerHandler.HandleRegister) + + parHandler := server.NewOAuthPARHandler(svc.db) + r.Post("/oauth/par", parHandler.HandlePAR) + + r.Get("/oauth/dpop/nonce", server.HandleDPoPNonce) + r.Post("/oauth/dpop/nonce", server.HandleDPoPNonce) + + corsPreflight := func(w http.ResponseWriter, r *http.Request) {} + for _, path := range []string{ + "/.well-known/oauth-authorization-server", + "/.well-known/oauth-protected-resource", + "/oauth-client-metadata.json", + "/oauth/authorize", + "/oauth/callback", + "/oauth/token", + "/oauth/jwks", + "/oauth/revoke", + "/oauth/register", + "/oauth/par", + "/oauth/dpop/nonce", + } { + r.Options(path, corsPreflight) + } + }) // Start cleanup worker oauthCleanupCtx, oauthCleanupCancel := context.WithCancel(context.Background()) @@ -614,9 +626,14 @@ func setupAdmin(r *chi.Mux, cfg *config.Config, svc *services) *admin.Handler { // Wire up backfill callbacks for the admin UI configureBackfillCallbacks(adminHandler, cfg, svc) - // Admin endpoint with optional auth (allows introspection without auth) - r.Handle("/admin/graphql", adminHandler.OptionalAuth()) - r.Handle("/admin/graphql/", adminHandler.OptionalAuth()) + // Admin endpoint with optional auth (allows introspection without auth). + // Cross-origin browser access is restricted to explicit admin origins. + adminGraphQLHandler := server.CORSMiddleware(server.CORSConfig{ + AllowedOrigins: cfg.AdminAllowedOriginList(), + AllowAdminAPIKeyAuth: cfg.AdminAPIKey != "", + })(adminHandler.OptionalAuth()) + r.Handle("/admin/graphql", adminGraphQLHandler) + r.Handle("/admin/graphql/", adminGraphQLHandler) slog.Info("Admin GraphQL endpoint enabled", "path", "/admin/graphql") // GraphiQL playgrounds @@ -775,19 +792,16 @@ func setupGraphQL(r *chi.Mux, cfg *config.Config, svc *services, pubsub *subscri if err != nil { slog.Error("Failed to create GraphQL handler", "error", err) } else { - r.Handle("/graphql", graphqlHandler) - r.Handle("/graphql/", graphqlHandler) + publicOrigins := cfg.PublicAllowedOriginList() + publicGraphQLHandler := server.CORSMiddleware(server.CORSConfig{ + AllowedOrigins: publicOrigins, + })(graphqlHandler) + r.Handle("/graphql", publicGraphQLHandler) + r.Handle("/graphql/", publicGraphQLHandler) slog.Info("GraphQL endpoint enabled", "path", "/graphql") // WebSocket subscription endpoint - var allowedOrigins []string - if cfg.AllowedOrigins != "" { - allowedOrigins = strings.Split(cfg.AllowedOrigins, ",") - for i := range allowedOrigins { - allowedOrigins[i] = strings.TrimSpace(allowedOrigins[i]) - } - } - subscriptionHandler := subscription.NewHandler(graphqlHandler.Schema(), pubsub, allowedOrigins) + subscriptionHandler := subscription.NewHandler(graphqlHandler.Schema(), pubsub, publicOrigins) r.Handle("/graphql/ws", subscriptionHandler) slog.Info("GraphQL subscriptions enabled", "path", "/graphql/ws") } diff --git a/cmd/hyperindex/main_test.go b/cmd/hyperindex/main_test.go index 87b2413e..924a8bb5 100644 --- a/cmd/hyperindex/main_test.go +++ b/cmd/hyperindex/main_test.go @@ -13,6 +13,7 @@ import ( "github.com/GainForest/hyperindex/internal/buildinfo" "github.com/GainForest/hyperindex/internal/config" "github.com/GainForest/hyperindex/internal/database/repositories" + "github.com/GainForest/hyperindex/internal/graphql/subscription" "github.com/GainForest/hyperindex/internal/testutil" ) @@ -42,6 +43,115 @@ func TestRootEndpointReturnsBuildInfoVersion(t *testing.T) { } } +func TestAdminGraphQLCORSUsesRestrictedOrigins(t *testing.T) { + db := testutil.SetupTestDB(t) + cfg := &config.Config{ + ExternalBaseURL: "https://api.example", + AdminAPIKey: "admin-secret-123", + AdminAllowedOrigins: "https://admin.example", + } + r := setupRouter(cfg, labelerTestServices(db), &backgroundServices{}) + setupAdmin(r, cfg, labelerTestServices(db)) + + t.Run("allowed admin origin", func(t *testing.T) { + req := httptest.NewRequest(http.MethodOptions, "/admin/graphql", nil) + req.Header.Set("Origin", "https://admin.example") + req.Header.Set("Access-Control-Request-Method", http.MethodPost) + rec := httptest.NewRecorder() + + r.ServeHTTP(rec, req) + + if rec.Code != http.StatusNoContent { + t.Fatalf("OPTIONS /admin/graphql status = %d, want %d", rec.Code, http.StatusNoContent) + } + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "https://admin.example" { + t.Fatalf("Access-Control-Allow-Origin = %q, want admin origin", got) + } + if got := rec.Header().Get("Access-Control-Allow-Headers"); !strings.Contains(got, "X-Admin-API-Key") { + t.Fatalf("Access-Control-Allow-Headers = %q, want X-Admin-API-Key", got) + } + }) + + t.Run("unknown admin origin", func(t *testing.T) { + req := httptest.NewRequest(http.MethodOptions, "/admin/graphql", nil) + req.Header.Set("Origin", "https://evil.example") + req.Header.Set("Access-Control-Request-Method", http.MethodPost) + rec := httptest.NewRecorder() + + r.ServeHTTP(rec, req) + + if rec.Code != http.StatusNoContent { + t.Fatalf("OPTIONS /admin/graphql status = %d, want %d", rec.Code, http.StatusNoContent) + } + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "" { + t.Fatalf("Access-Control-Allow-Origin = %q, want empty", got) + } + }) +} + +func TestPublicGraphQLCORSAllowsAllOriginsByDefault(t *testing.T) { + db := testutil.SetupTestDB(t) + cfg := &config.Config{ExternalBaseURL: "https://api.example"} + r := setupRouter(cfg, labelerTestServices(db), &backgroundServices{}) + setupGraphQL(r, cfg, labelerTestServices(db), subscription.NewPubSub()) + + req := httptest.NewRequest(http.MethodOptions, "/graphql", nil) + req.Header.Set("Origin", "https://any-frontend.example") + req.Header.Set("Access-Control-Request-Method", http.MethodPost) + rec := httptest.NewRecorder() + + r.ServeHTTP(rec, req) + + if rec.Code != http.StatusNoContent { + t.Fatalf("OPTIONS /graphql status = %d, want %d", rec.Code, http.StatusNoContent) + } + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "*" { + t.Fatalf("Access-Control-Allow-Origin = %q, want *", got) + } +} + +func TestOAuthEndpointsUsePublicCORS(t *testing.T) { + db := testutil.SetupTestDB(t) + cfg := &config.Config{ExternalBaseURL: "https://api.example"} + r := setupRouter(cfg, labelerTestServices(db), &backgroundServices{}) + setupOAuth(r, cfg, labelerTestServices(db), &backgroundServices{}) + + t.Run("preflight for token endpoint", func(t *testing.T) { + req := httptest.NewRequest(http.MethodOptions, "/oauth/token", nil) + req.Header.Set("Origin", "https://frontend.example") + req.Header.Set("Access-Control-Request-Method", http.MethodPost) + req.Header.Set("Access-Control-Request-Headers", "content-type,dpop") + rec := httptest.NewRecorder() + + r.ServeHTTP(rec, req) + + if rec.Code != http.StatusNoContent { + t.Fatalf("OPTIONS /oauth/token status = %d, want %d", rec.Code, http.StatusNoContent) + } + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "*" { + t.Fatalf("Access-Control-Allow-Origin = %q, want *", got) + } + if got := rec.Header().Get("Access-Control-Allow-Headers"); !strings.Contains(got, "DPoP") { + t.Fatalf("Access-Control-Allow-Headers = %q, want DPoP", got) + } + }) + + t.Run("actual discovery response", func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, "/.well-known/oauth-authorization-server", nil) + req.Header.Set("Origin", "https://frontend.example") + rec := httptest.NewRecorder() + + r.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("GET discovery status = %d, want %d", rec.Code, http.StatusOK) + } + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "*" { + t.Fatalf("Access-Control-Allow-Origin = %q, want *", got) + } + }) +} + func TestHealthIgnoresLabelerReadiness(t *testing.T) { db := testutil.SetupTestDB(t) url := "wss://labeler.example/xrpc/com.atproto.label.subscribeLabels" @@ -309,12 +419,18 @@ func labelerTestConfig(url string) *config.Config { func labelerTestServices(db *testutil.TestDB) *services { return &services{ - db: db.Executor, - records: db.Records, - actors: db.Actors, - lexicons: db.Lexicons, - config: db.Config, - externalLabels: db.ExternalLabels, + db: db.Executor, + records: db.Records, + actors: db.Actors, + lexicons: db.Lexicons, + config: db.Config, + activity: db.Activity, + oauthClients: db.OAuthClients, + labels: db.Labels, + externalLabels: db.ExternalLabels, + labelDefinitions: db.LabelDefinitions, + labelPreferences: db.LabelPreferences, + reports: db.Reports, } } diff --git a/internal/config/config.go b/internal/config/config.go index 857422e4..baa8eecc 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -24,8 +24,16 @@ type Config struct { DatabaseURL string // Security - SecretKeyBase string - AllowedOrigins string // Comma-separated allowed WebSocket/CORS origins (empty or "*" = allow all) + SecretKeyBase string + // AllowedOrigins is a deprecated comma-separated origin list kept as a fallback for admin CORS. + // Use PublicAllowedOrigins and AdminAllowedOrigins for new deployments. + AllowedOrigins string + // PublicAllowedOrigins is a comma-separated origin list for public GraphQL HTTP and WebSocket APIs. + // Empty or "*" allows all browser origins so independent frontends can query public data. + PublicAllowedOrigins string + // AdminAllowedOrigins is a comma-separated explicit origin list for admin GraphQL browser clients. + // Empty keeps admin GraphQL same-origin only unless deprecated AllowedOrigins has explicit origins. + AdminAllowedOrigins string // OAuth ExternalBaseURL string @@ -88,8 +96,10 @@ func Load() (*Config, error) { DatabaseURL: getEnv("DATABASE_URL", "sqlite:data/hyperindex.db"), // Security - SecretKeyBase: getEnv("SECRET_KEY_BASE", ""), - AllowedOrigins: getEnv("ALLOWED_ORIGINS", ""), + SecretKeyBase: getEnv("SECRET_KEY_BASE", ""), + AllowedOrigins: getEnv("ALLOWED_ORIGINS", ""), + PublicAllowedOrigins: getEnv("PUBLIC_ALLOWED_ORIGINS", "*"), + AdminAllowedOrigins: getEnv("ADMIN_ALLOWED_ORIGINS", ""), // OAuth ExternalBaseURL: getEnv("EXTERNAL_BASE_URL", ""), @@ -152,6 +162,10 @@ func Load() (*Config, error) { slog.Info("ADMIN_API_KEY enabled for admin API access", "admin_api_key_set", true) } + if strings.TrimSpace(cfg.AllowedOrigins) != "" { + slog.Warn("ALLOWED_ORIGINS is deprecated; use PUBLIC_ALLOWED_ORIGINS for public GraphQL and ADMIN_ALLOWED_ORIGINS for admin GraphQL") + } + // Set default external base URL if not provided cfg.ExternalBaseURL = strings.TrimSpace(cfg.ExternalBaseURL) if cfg.ExternalBaseURL == "" { @@ -186,6 +200,10 @@ func (c *Config) Validate() error { return fmt.Errorf("SECRET_KEY_BASE must be at least 64 characters") } + if containsWildcardOrigin(ParseAllowedOrigins(c.AdminAllowedOrigins)) { + return fmt.Errorf("ADMIN_ALLOWED_ORIGINS must list explicit trusted origins; wildcard '*' is only supported by PUBLIC_ALLOWED_ORIGINS") + } + if c.Port < 1 || c.Port > 65535 { return fmt.Errorf("PORT must be between 1 and 65535") } @@ -225,7 +243,9 @@ func (c *Config) LogConfig() { "jetstream_collections", c.JetstreamCollections, "jetstream_disable_cursor", c.JetstreamDisableCursor, "backfill_on_start", c.BackfillOnStart, - "allowed_origins", c.AllowedOrigins, + "allowed_origins_deprecated", c.AllowedOrigins, + "public_allowed_origins", c.PublicAllowedOrigins, + "admin_allowed_origins", c.AdminAllowedOrigins, "tap_enabled", c.TapEnabled, "tap_url", c.TapURL, "tap_admin_password_set", c.TapAdminPassword != "", @@ -237,6 +257,68 @@ func (c *Config) LogConfig() { ) } +// PublicAllowedOriginList returns the origins allowed to call public GraphQL routes from browsers. +// Public GraphQL is intentionally open by default; set PUBLIC_ALLOWED_ORIGINS to restrict it. +func (c *Config) PublicAllowedOriginList() []string { + origins := ParseAllowedOrigins(c.PublicAllowedOrigins) + if len(origins) == 0 { + return []string{"*"} + } + return origins +} + +// AdminAllowedOriginList returns the origins allowed to call admin GraphQL routes from browsers. +// Admin GraphQL is same-origin only by default. When ADMIN_ALLOWED_ORIGINS is unset, explicit +// origins from deprecated ALLOWED_ORIGINS are used as a compatibility fallback; wildcard values +// from ALLOWED_ORIGINS are ignored for admin routes. +func (c *Config) AdminAllowedOriginList() []string { + origins := ParseAllowedOrigins(c.AdminAllowedOrigins) + if len(origins) > 0 { + return origins + } + + return removeWildcardOrigins(ParseAllowedOrigins(c.AllowedOrigins)) +} + +// ParseAllowedOrigins parses a comma-separated browser origin list, trimming whitespace and duplicates. +func ParseAllowedOrigins(raw string) []string { + parts := strings.Split(raw, ",") + origins := make([]string, 0, len(parts)) + seen := make(map[string]struct{}, len(parts)) + for _, part := range parts { + trimmed := strings.TrimSpace(part) + if trimmed == "" { + continue + } + if _, ok := seen[trimmed]; ok { + continue + } + seen[trimmed] = struct{}{} + origins = append(origins, trimmed) + } + return origins +} + +func containsWildcardOrigin(origins []string) bool { + for _, origin := range origins { + if strings.TrimSpace(origin) == "*" { + return true + } + } + return false +} + +func removeWildcardOrigins(origins []string) []string { + explicitOrigins := make([]string, 0, len(origins)) + for _, origin := range origins { + if strings.TrimSpace(origin) == "*" { + continue + } + explicitOrigins = append(explicitOrigins, origin) + } + return explicitOrigins +} + // LabelerSubscribeURLList returns trimmed labeler subscription URLs. func (c *Config) LabelerSubscribeURLList() []string { return ParseLabelerSubscribeURLs(c.LabelerSubscribeURLs) diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 7efab6fa..bd5fb24f 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -347,6 +347,102 @@ func TestConfigValidate(t *testing.T) { } } +func TestParseAllowedOrigins(t *testing.T) { + got := ParseAllowedOrigins(" https://one.example, ,https://two.example,https://one.example ") + want := []string{"https://one.example", "https://two.example"} + + if len(got) != len(want) { + t.Fatalf("ParseAllowedOrigins() len = %d, want %d: %#v", len(got), len(want), got) + } + for i := range want { + if got[i] != want[i] { + t.Fatalf("ParseAllowedOrigins()[%d] = %q, want %q", i, got[i], want[i]) + } + } +} + +func TestPublicAllowedOriginListDefaultsToWildcard(t *testing.T) { + cfg := Config{} + + got := cfg.PublicAllowedOriginList() + if len(got) != 1 || got[0] != "*" { + t.Fatalf("PublicAllowedOriginList() = %#v, want [*]", got) + } +} + +func TestAdminAllowedOriginListUsesExplicitOriginsOnly(t *testing.T) { + t.Run("admin env wins", func(t *testing.T) { + cfg := Config{ + AllowedOrigins: "https://legacy.example", + AdminAllowedOrigins: "https://admin.example", + } + + got := cfg.AdminAllowedOriginList() + if len(got) != 1 || got[0] != "https://admin.example" { + t.Fatalf("AdminAllowedOriginList() = %#v, want explicit admin origin", got) + } + }) + + t.Run("legacy explicit origins fallback", func(t *testing.T) { + cfg := Config{AllowedOrigins: "https://legacy.example"} + + got := cfg.AdminAllowedOriginList() + if len(got) != 1 || got[0] != "https://legacy.example" { + t.Fatalf("AdminAllowedOriginList() = %#v, want legacy origin", got) + } + }) + + t.Run("legacy wildcard does not open admin", func(t *testing.T) { + cfg := Config{AllowedOrigins: "*"} + + if got := cfg.AdminAllowedOriginList(); len(got) != 0 { + t.Fatalf("AdminAllowedOriginList() = %#v, want empty", got) + } + }) + + t.Run("legacy wildcard is ignored but explicit origins remain", func(t *testing.T) { + cfg := Config{AllowedOrigins: "*,https://legacy.example"} + + got := cfg.AdminAllowedOriginList() + if len(got) != 1 || got[0] != "https://legacy.example" { + t.Fatalf("AdminAllowedOriginList() = %#v, want explicit legacy origin", got) + } + }) +} + +func TestConfigValidateRejectsAdminWildcardOrigin(t *testing.T) { + cfg := Config{ + SecretKeyBase: "this_is_a_very_long_secret_key_that_is_definitely_more_than_64_characters_long_for_testing", + Port: 8080, + AdminAPIKey: "admin-secret-123", + AdminAllowedOrigins: "*", + } + + err := cfg.Validate() + if err == nil || !strings.Contains(err.Error(), "ADMIN_ALLOWED_ORIGINS") { + t.Fatalf("Validate() error = %v, want ADMIN_ALLOWED_ORIGINS error", err) + } +} + +func TestLoadCORSOriginConfig(t *testing.T) { + t.Setenv("ADMIN_API_KEY", "admin-secret-123") + t.Setenv("SECRET_KEY_BASE", "this_is_a_very_long_secret_key_that_is_definitely_more_than_64_characters_long_for_testing") + t.Setenv("PUBLIC_ALLOWED_ORIGINS", "") + t.Setenv("ADMIN_ALLOWED_ORIGINS", "https://admin.example") + + cfg, err := Load() + if err != nil { + t.Fatalf("Load() error = %v", err) + } + + if cfg.PublicAllowedOrigins != "*" { + t.Fatalf("PublicAllowedOrigins = %q, want *", cfg.PublicAllowedOrigins) + } + if cfg.AdminAllowedOrigins != "https://admin.example" { + t.Fatalf("AdminAllowedOrigins = %q, want https://admin.example", cfg.AdminAllowedOrigins) + } +} + func TestLoadAdminAPIKey(t *testing.T) { os.Setenv("ADMIN_API_KEY", "admin-secret") os.Setenv("SECRET_KEY_BASE", "this_is_a_very_long_secret_key_that_is_definitely_more_than_64_characters_long_for_testing") diff --git a/internal/graphql/admin/handler.go b/internal/graphql/admin/handler.go index c6b395bd..a3869340 100644 --- a/internal/graphql/admin/handler.go +++ b/internal/graphql/admin/handler.go @@ -102,7 +102,7 @@ func logSafeURLVariableValue(value interface{}) interface{} { } // ServeHTTP handles admin GraphQL HTTP requests. -// CORS is handled by the router-level middleware; not duplicated here. +// CORS is handled by route-level middleware; not duplicated here. func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { // Parse the request var params struct { diff --git a/internal/graphql/handler.go b/internal/graphql/handler.go index 4ccba319..247c79c2 100644 --- a/internal/graphql/handler.go +++ b/internal/graphql/handler.go @@ -30,7 +30,7 @@ func NewHandler(registry *lexicon.Registry, repos *resolver.Repositories) (*Hand } // ServeHTTP handles GraphQL HTTP requests. -// CORS is handled by the router-level middleware; not duplicated here. +// CORS is handled by route-level middleware; not duplicated here. func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { // Parse the request var params struct { diff --git a/internal/graphql/handler_test.go b/internal/graphql/handler_test.go index e075c5d2..f9f538b4 100644 --- a/internal/graphql/handler_test.go +++ b/internal/graphql/handler_test.go @@ -36,7 +36,7 @@ func createMinimalSchema() (*graphqlgo.Schema, error) { } func TestHandler_ServeHTTP_NoCORSInHandler(t *testing.T) { - // CORS is handled by the router-level CORSMiddleware, not the handler. + // CORS is handled by route-level CORSMiddleware, not the handler. // Verify the handler does NOT set CORS headers directly. schema, err := createMinimalSchema() if err != nil { @@ -55,7 +55,7 @@ func TestHandler_ServeHTTP_NoCORSInHandler(t *testing.T) { handler.ServeHTTP(w, req) if w.Header().Get("Access-Control-Allow-Origin") != "" { - t.Error("handler should not set Access-Control-Allow-Origin (CORS is middleware's job)") + t.Error("handler should not set Access-Control-Allow-Origin (CORS is route middleware's job)") } }) } diff --git a/internal/graphql/subscription/handler.go b/internal/graphql/subscription/handler.go index 320ad1f3..682957bc 100644 --- a/internal/graphql/subscription/handler.go +++ b/internal/graphql/subscription/handler.go @@ -5,6 +5,8 @@ import ( "encoding/json" "log/slog" "net/http" + "net/url" + "strings" "sync" "time" @@ -51,8 +53,8 @@ type Handler struct { // NewHandler creates a new subscription handler. // allowedOrigins controls which origins may open WebSocket connections. -// Pass []string{"*"} to allow all origins (development only). -// Pass nil or empty slice to enforce same-origin policy. +// Pass []string{"*"} to allow all origins for public subscription APIs. +// Pass nil or an empty slice to accept same-origin browser connections only. func NewHandler(schema *graphql.Schema, pubsub *PubSub, allowedOrigins []string) *Handler { return &Handler{ schema: schema, @@ -66,15 +68,9 @@ func NewHandler(schema *graphql.Schema, pubsub *PubSub, allowedOrigins []string) // makeOriginChecker returns a CheckOrigin function based on the allowed origins list. func makeOriginChecker(allowedOrigins []string) func(r *http.Request) bool { - // No origins configured or explicitly set to "*": allow all origins. - // This matches the CORS middleware default behavior. To restrict origins, - // set ALLOWED_ORIGINS to a comma-separated list of specific origins. - if len(allowedOrigins) == 0 || (len(allowedOrigins) == 1 && allowedOrigins[0] == "*") { - if len(allowedOrigins) == 0 { - slog.Warn("WebSocket CheckOrigin allows all origins (ALLOWED_ORIGINS not configured)") - } else { - slog.Warn("WebSocket CheckOrigin allows all origins (ALLOWED_ORIGINS=\"*\")") - } + allowedSet, allowAll := normalizeWebSocketOrigins(allowedOrigins) + if allowAll { + slog.Warn("WebSocket CheckOrigin allows all origins") return func(r *http.Request) bool { return true } @@ -82,13 +78,11 @@ func makeOriginChecker(allowedOrigins []string) func(r *http.Request) bool { return func(r *http.Request) bool { origin := r.Header.Get("Origin") - if origin == "" { - return true // Same-origin requests don't send Origin header + if origin == "" || isSameOriginWebSocketRequest(r) { + return true } - for _, allowed := range allowedOrigins { - if origin == allowed { - return true - } + if _, ok := allowedSet[origin]; ok { + return true } slog.Warn("WebSocket connection rejected: origin not allowed", "origin", origin, @@ -97,6 +91,33 @@ func makeOriginChecker(allowedOrigins []string) func(r *http.Request) bool { } } +func normalizeWebSocketOrigins(origins []string) (map[string]struct{}, bool) { + allowedSet := make(map[string]struct{}, len(origins)) + for _, origin := range origins { + trimmed := strings.TrimSpace(origin) + if trimmed == "" { + continue + } + if trimmed == "*" { + return nil, true + } + allowedSet[trimmed] = struct{}{} + } + return allowedSet, false +} + +func isSameOriginWebSocketRequest(r *http.Request) bool { + origin := r.Header.Get("Origin") + if origin == "" { + return true + } + parsedOrigin, err := url.Parse(origin) + if err != nil { + return false + } + return parsedOrigin.Host == r.Host +} + // ServeHTTP upgrades HTTP to WebSocket and handles subscriptions. func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { conn, err := h.upgrader.Upgrade(w, r, nil) diff --git a/internal/graphql/subscription/handler_test.go b/internal/graphql/subscription/handler_test.go index ca39305c..5ef04895 100644 --- a/internal/graphql/subscription/handler_test.go +++ b/internal/graphql/subscription/handler_test.go @@ -2,6 +2,7 @@ package subscription import ( "net/http" + "net/http/httptest" "testing" ) @@ -13,15 +14,21 @@ func TestMakeOriginChecker(t *testing.T) { want bool }{ { - name: "nil origins allows all", + name: "nil origins reject cross-origin", allowedOrigins: nil, requestOrigin: "https://example.com", - want: true, + want: false, }, { - name: "empty origins allows all", + name: "empty origins reject cross-origin", allowedOrigins: []string{}, requestOrigin: "https://example.com", + want: false, + }, + { + name: "empty origins allow same-origin", + allowedOrigins: []string{}, + requestOrigin: "https://api.example", want: true, }, { @@ -65,7 +72,7 @@ func TestMakeOriginChecker(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { checker := makeOriginChecker(tt.allowedOrigins) - req, _ := http.NewRequest("GET", "/graphql/ws", nil) + req := httptest.NewRequest(http.MethodGet, "https://api.example/graphql/ws", nil) if tt.requestOrigin != "" { req.Header.Set("Origin", tt.requestOrigin) } diff --git a/internal/server/cors.go b/internal/server/cors.go index 642fc8ba..2ee5f004 100644 --- a/internal/server/cors.go +++ b/internal/server/cors.go @@ -6,37 +6,28 @@ import ( "strings" ) -// CORSConfig holds CORS middleware configuration. +// CORSConfig holds CORS middleware configuration for one route group. type CORSConfig struct { - // AllowedOrigins is a list of origins that are allowed to make cross-origin requests. - // If empty, defaults to "*" (all origins allowed — suitable for development only). + // AllowedOrigins is the list of browser origins that may make cross-origin requests. + // Use []string{"*"} to allow every origin. An empty list sends no CORS origin + // headers, which keeps browser access same-origin unless another middleware adds them. AllowedOrigins []string - // AllowedHeaders is the list of request headers allowed in CORS requests. - // "Content-Type" and "Authorization" are always included. + // AllowedHeaders is the list of additional request headers allowed in CORS requests. + // "Content-Type", "Authorization", and "DPoP" are always included. AllowedHeaders []string - // AllowAdminAPIKeyAuth controls whether X-User-DID is included in allowed headers. + // AllowAdminAPIKeyAuth includes admin proxy headers in preflight responses. + // Enable this only for admin routes that intentionally accept X-Admin-API-Key + // and X-User-DID from browser clients. AllowAdminAPIKeyAuth bool } -// CORSMiddleware returns an HTTP middleware that handles CORS headers and preflight requests. -// It uses the configured allowed origins instead of hardcoding "*". +// CORSMiddleware returns an HTTP middleware that handles CORS headers and +// preflight requests for a single route group. func CORSMiddleware(cfg CORSConfig) func(http.Handler) http.Handler { - // Build allowed origins set for O(1) lookup - allowedSet := make(map[string]bool, len(cfg.AllowedOrigins)) - for _, origin := range cfg.AllowedOrigins { - allowedSet[strings.TrimSpace(origin)] = true - } - allowAll := len(cfg.AllowedOrigins) == 0 - - // Build allowed headers - headers := []string{"Content-Type", "Authorization", "DPoP"} - headers = append(headers, cfg.AllowedHeaders...) - if cfg.AllowAdminAPIKeyAuth { - headers = append(headers, "X-User-DID") - } - allowedHeaders := strings.Join(headers, ", ") + allowedSet, allowAll := normalizeAllowedOrigins(cfg.AllowedOrigins) + allowedHeaders := strings.Join(allowedRequestHeaders(cfg), ", ") return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -44,17 +35,18 @@ func CORSMiddleware(cfg CORSConfig) func(http.Handler) http.Handler { if allowAll { w.Header().Set("Access-Control-Allow-Origin", "*") - } else if origin != "" && allowedSet[origin] { - w.Header().Set("Access-Control-Allow-Origin", origin) - w.Header().Set("Vary", "Origin") + } else if origin != "" { + addVaryHeader(w.Header(), "Origin") + if _, ok := allowedSet[origin]; ok { + w.Header().Set("Access-Control-Allow-Origin", origin) + } } w.Header().Set("Access-Control-Allow-Methods", "POST, GET, OPTIONS") w.Header().Set("Access-Control-Allow-Headers", allowedHeaders) w.Header().Set("Access-Control-Max-Age", "86400") - // Handle preflight - if r.Method == "OPTIONS" { + if r.Method == http.MethodOptions { w.WriteHeader(http.StatusNoContent) return } @@ -63,3 +55,57 @@ func CORSMiddleware(cfg CORSConfig) func(http.Handler) http.Handler { }) } } + +func normalizeAllowedOrigins(origins []string) (map[string]struct{}, bool) { + allowedSet := make(map[string]struct{}, len(origins)) + for _, origin := range origins { + trimmed := strings.TrimSpace(origin) + if trimmed == "" { + continue + } + if trimmed == "*" { + return nil, true + } + allowedSet[trimmed] = struct{}{} + } + return allowedSet, false +} + +func allowedRequestHeaders(cfg CORSConfig) []string { + headers := []string{"Content-Type", "Authorization", "DPoP"} + headers = append(headers, cfg.AllowedHeaders...) + if cfg.AllowAdminAPIKeyAuth { + headers = append(headers, "X-Admin-API-Key", "X-User-DID") + } + return uniqueHeaderNames(headers) +} + +func uniqueHeaderNames(headers []string) []string { + unique := make([]string, 0, len(headers)) + seen := make(map[string]struct{}, len(headers)) + for _, header := range headers { + trimmed := strings.TrimSpace(header) + if trimmed == "" { + continue + } + key := strings.ToLower(trimmed) + if _, ok := seen[key]; ok { + continue + } + seen[key] = struct{}{} + unique = append(unique, trimmed) + } + return unique +} + +func addVaryHeader(header http.Header, value string) { + current := header.Values("Vary") + for _, entry := range current { + for _, part := range strings.Split(entry, ",") { + if strings.EqualFold(strings.TrimSpace(part), value) { + return + } + } + } + header.Add("Vary", value) +} diff --git a/internal/server/cors_test.go b/internal/server/cors_test.go new file mode 100644 index 00000000..629fca8f --- /dev/null +++ b/internal/server/cors_test.go @@ -0,0 +1,100 @@ +package server + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +func TestCORSMiddlewareAllowsAllOriginsWithWildcard(t *testing.T) { + handler := CORSMiddleware(CORSConfig{AllowedOrigins: []string{"*"}})(okHandler()) + + req := httptest.NewRequest(http.MethodPost, "/graphql", nil) + req.Header.Set("Origin", "https://example.app") + rec := httptest.NewRecorder() + + handler.ServeHTTP(rec, req) + + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "*" { + t.Fatalf("Access-Control-Allow-Origin = %q, want *", got) + } +} + +func TestCORSMiddlewareRestrictsSpecificOrigins(t *testing.T) { + handler := CORSMiddleware(CORSConfig{AllowedOrigins: []string{"https://admin.example"}})(okHandler()) + + t.Run("allowed origin", func(t *testing.T) { + req := httptest.NewRequest(http.MethodPost, "/admin/graphql", nil) + req.Header.Set("Origin", "https://admin.example") + rec := httptest.NewRecorder() + + handler.ServeHTTP(rec, req) + + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "https://admin.example" { + t.Fatalf("Access-Control-Allow-Origin = %q, want allowed origin", got) + } + if got := rec.Header().Get("Vary"); got != "Origin" { + t.Fatalf("Vary = %q, want Origin", got) + } + }) + + t.Run("rejected origin", func(t *testing.T) { + req := httptest.NewRequest(http.MethodPost, "/admin/graphql", nil) + req.Header.Set("Origin", "https://evil.example") + rec := httptest.NewRecorder() + + handler.ServeHTTP(rec, req) + + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "" { + t.Fatalf("Access-Control-Allow-Origin = %q, want empty", got) + } + if got := rec.Header().Get("Vary"); got != "Origin" { + t.Fatalf("Vary = %q, want Origin", got) + } + }) +} + +func TestCORSMiddlewareEmptyOriginsDoNotAllowCrossOrigin(t *testing.T) { + handler := CORSMiddleware(CORSConfig{})(okHandler()) + + req := httptest.NewRequest(http.MethodPost, "/admin/graphql", nil) + req.Header.Set("Origin", "https://example.app") + rec := httptest.NewRecorder() + + handler.ServeHTTP(rec, req) + + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "" { + t.Fatalf("Access-Control-Allow-Origin = %q, want empty", got) + } +} + +func TestCORSMiddlewareAdminPreflightHeaders(t *testing.T) { + handler := CORSMiddleware(CORSConfig{ + AllowedOrigins: []string{"https://admin.example"}, + AllowAdminAPIKeyAuth: true, + })(okHandler()) + + req := httptest.NewRequest(http.MethodOptions, "/admin/graphql", nil) + req.Header.Set("Origin", "https://admin.example") + req.Header.Set("Access-Control-Request-Method", http.MethodPost) + rec := httptest.NewRecorder() + + handler.ServeHTTP(rec, req) + + if rec.Code != http.StatusNoContent { + t.Fatalf("status = %d, want %d", rec.Code, http.StatusNoContent) + } + allowedHeaders := rec.Header().Get("Access-Control-Allow-Headers") + for _, header := range []string{"Content-Type", "Authorization", "DPoP", "X-Admin-API-Key", "X-User-DID"} { + if !strings.Contains(allowedHeaders, header) { + t.Fatalf("Access-Control-Allow-Headers = %q, want it to contain %q", allowedHeaders, header) + } + } +} + +func okHandler() http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + }) +}