diff --git a/README.md b/README.md index a263a55..5c9e04b 100644 --- a/README.md +++ b/README.md @@ -188,7 +188,7 @@ docker compose up -d go test ./... ``` -> No test files exist in the repository yet — this is the standard command to run once tests are added. See `CONTRIBUTING.md` before submitting a PR that adds functionality without tests. +> Tests live beside the code they cover. See `CONTRIBUTING.md` before submitting a PR that adds functionality without tests. ### Configuration @@ -203,6 +203,54 @@ Edit `config.yaml` or use environment variables: | Max payload | `messages.max_payload_size` | `RELAY_MESSAGES_MAX_PAYLOAD_SIZE` | `524288` (500KB) | | Rate limit | `security.rate_limit` | `RELAY_SECURITY_RATE_LIMIT` | `30` req/min/IP | | API key | `security.api_key` | `RELAY_SECURITY_API_KEY` | `` (disabled) | +| CORS origins | `security.allowed_origins` | `RELAY_SECURITY_ALLOWED_ORIGINS` | `` (CORS disabled) | + +### CORS + +By default the relay serves **no CORS headers**, so a browser calling it from +another origin is blocked at the preflight. There are two ways to run a +browser client: + +**1. Reverse proxy (no relay configuration).** Put the relay behind a +same-origin path in your own app — a Vercel rewrite, an nginx `location`, a +Vite `server.proxy` entry. The request is never cross-origin, so CORS never +applies. This is the setup the ThruBox client docs assume. + +**2. Direct calls with an origin allowlist.** List the origins you want to +serve and browsers can call the relay directly, no proxy needed: + +```yaml +security: + allowed_origins: + - "https://app.example.com" + - "http://localhost:5173" +``` + +or via the environment, comma-separated: + +```bash +RELAY_SECURITY_ALLOWED_ORIGINS="https://app.example.com,http://localhost:5173" +``` + +When an origin is allowed, the relay answers preflights and returns +`Access-Control-Allow-Origin` for that origin, `Access-Control-Allow-Methods: +GET, POST, DELETE, OPTIONS`, and `Access-Control-Allow-Headers: Content-Type` +— plus `X-API-Key` when `security.api_key` is set. + +Notes: + +- Entries must be full origins (`https://host[:port]`), with no path. A bare + hostname or a URL with a path is rejected at startup rather than silently + never matching. +- `"*"` allows any origin. It cannot be combined with specific origins, and it + is a poor fit for a relay with no API key configured — anything on the web + can then read and write messages from a browser. +- Credentialed CORS is not supported. The relay authenticates with the + `X-API-Key` header, not cookies, so `Access-Control-Allow-Credentials` is + never sent. +- An allowlisted origin still has to satisfy `security.api_key` and the rate + limiter. CORS controls which origins a browser will let read a response; it + is not authentication. --- diff --git a/cmd/relay/main.go b/cmd/relay/main.go index 8801bef..83b09aa 100644 --- a/cmd/relay/main.go +++ b/cmd/relay/main.go @@ -34,6 +34,7 @@ func main() { "storage_path", cfg.Storage.Path, "ttl_days", cfg.Messages.TTLDays, "rate_limit", cfg.Security.RateLimit, + "allowed_origins", cfg.Security.AllowedOrigins, ) // Initialize storage @@ -70,13 +71,24 @@ func main() { mux.HandleFunc("GET /api/messages/{address}", msgHandler.HandleGetByAddress) mux.HandleFunc("DELETE /api/messages/{id}", msgHandler.HandleDelete) - // Apply middleware chain: API Key → Rate Limiter → Router + // Apply middleware chain: CORS → API Key → Rate Limiter → Router + // + // CORS is outermost on purpose. A browser preflight is an OPTIONS request + // with no custom headers, so it carries no X-API-Key; if APIKeyAuth ran + // first every preflight would 401 and the real request would never be + // sent. CORS answers the preflight itself and lets everything else fall + // through to authentication as normal. rateLimiter := middleware.NewRateLimiter(cfg.Security.RateLimit) defer rateLimiter.Stop() var h http.Handler = mux h = rateLimiter.Middleware(h) h = middleware.APIKeyAuth(cfg.Security.APIKey)(h) + h = middleware.CORS(cfg.Security.AllowedOrigins, cfg.Security.APIKey != "")(h) + + if len(cfg.Security.AllowedOrigins) > 0 { + slog.Info("CORS enabled", "allowed_origins", cfg.Security.AllowedOrigins) + } // Create HTTP server srv := &http.Server{ diff --git a/config.yaml b/config.yaml index 778fd0e..7fa48a8 100644 --- a/config.yaml +++ b/config.yaml @@ -13,3 +13,10 @@ messages: security: rate_limit: 30 # Requests per minute per IP api_key: "" # Optional: if set, all requests must include X-API-Key header + + # CORS allowlist for browsers calling the relay directly (no reverse proxy). + # Empty (the default) serves no CORS headers at all, unchanged from before. + # Use ["*"] to allow any origin. "*" cannot be mixed with specific origins. + # allowed_origins: + # - "https://app.example.com" + allowed_origins: [] diff --git a/internal/config/config.go b/internal/config/config.go index 46afad3..2bb063e 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -5,6 +5,7 @@ import ( "log" "os" "strconv" + "strings" "gopkg.in/yaml.v3" ) @@ -39,6 +40,9 @@ type MessageConfig struct { type SecurityConfig struct { RateLimit int `yaml:"rate_limit"` APIKey string `yaml:"api_key"` + // AllowedOrigins is the CORS allowlist. Empty (the default) serves no + // CORS headers at all. "*" allows any origin. + AllowedOrigins []string `yaml:"allowed_origins"` } // Default returns a Config with sensible defaults. @@ -60,6 +64,8 @@ func Default() *Config { Security: SecurityConfig{ RateLimit: 30, APIKey: "", + // No origins: CORS stays off unless explicitly configured. + AllowedOrigins: nil, }, } } @@ -144,6 +150,24 @@ func applyEnvOverrides(cfg *Config) { if v := os.Getenv("RELAY_SECURITY_API_KEY"); v != "" { cfg.Security.APIKey = v } + + if v := os.Getenv("RELAY_SECURITY_ALLOWED_ORIGINS"); v != "" { + cfg.Security.AllowedOrigins = splitOrigins(v) + } +} + +// splitOrigins parses a comma-separated origin list from an environment +// variable, trimming spaces and dropping empty entries so that values like +// "https://a.example, https://b.example," behave as expected. +func splitOrigins(v string) []string { + parts := strings.Split(v, ",") + origins := make([]string, 0, len(parts)) + for _, p := range parts { + if p = strings.TrimSpace(p); p != "" { + origins = append(origins, p) + } + } + return origins } // Validate checks the configuration values for validity. @@ -160,6 +184,50 @@ func (c *Config) Validate() error { if c.Security.RateLimit < 0 { return fmt.Errorf("invalid rate_limit: %d", c.Security.RateLimit) } + if err := validateAllowedOrigins(c.Security.AllowedOrigins); err != nil { + return err + } + return nil +} + +// validateAllowedOrigins rejects configurations that would not do what the +// operator expects: a wildcard mixed with specific origins (the wildcard +// silently wins, making the list misleading), and entries that are not +// scheme-qualified origins, which can never match a browser Origin header. +func validateAllowedOrigins(origins []string) error { + hasWildcard, hasSpecific := false, false + + for _, o := range origins { + o = strings.TrimSpace(o) + if o == "" { + continue + } + if o == "*" { + hasWildcard = true + continue + } + hasSpecific = true + + scheme, rest, found := strings.Cut(o, "://") + if !found || scheme == "" { + return fmt.Errorf( + "invalid allowed_origins entry %q: must be a full origin such as https://app.example.com", o) + } + // A single trailing slash is tolerated; anything more is a path. + host := strings.TrimSuffix(rest, "/") + if host == "" { + return fmt.Errorf( + "invalid allowed_origins entry %q: missing host", o) + } + if strings.Contains(host, "/") { + return fmt.Errorf( + "invalid allowed_origins entry %q: an origin has no path component", o) + } + } + + if hasWildcard && hasSpecific { + return fmt.Errorf(`invalid allowed_origins: "*" cannot be combined with specific origins`) + } return nil } diff --git a/internal/config/security_test.go b/internal/config/security_test.go new file mode 100644 index 0000000..2dfb6de --- /dev/null +++ b/internal/config/security_test.go @@ -0,0 +1,273 @@ +package config + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// resetOriginEnv neutralises the CORS-related environment so a test observes +// only what it sets. Empty is equivalent to unset for applyEnvOverrides, and +// t.Setenv restores whatever the developer's shell had. +func resetOriginEnv(t *testing.T) { + t.Helper() + t.Setenv("RELAY_SECURITY_ALLOWED_ORIGINS", "") +} + +func TestAllowedOrigins_DefaultIsEmpty(t *testing.T) { + resetOriginEnv(t) + + cfg := Default() + applyEnvOverrides(cfg) + + if len(cfg.Security.AllowedOrigins) != 0 { + t.Errorf("AllowedOrigins = %v, want empty so CORS stays off by default", + cfg.Security.AllowedOrigins) + } +} + +func TestAllowedOrigins_EnvOverride(t *testing.T) { + tests := []struct { + name string + env string + want []string + }{ + { + name: "single origin", + env: "https://app.example.com", + want: []string{"https://app.example.com"}, + }, + { + name: "comma separated", + env: "https://a.example.com,https://b.example.com", + want: []string{"https://a.example.com", "https://b.example.com"}, + }, + { + name: "spaces around entries are trimmed", + env: " https://a.example.com , https://b.example.com ", + want: []string{"https://a.example.com", "https://b.example.com"}, + }, + { + name: "trailing comma produces no empty entry", + env: "https://a.example.com,", + want: []string{"https://a.example.com"}, + }, + { + name: "wildcard", + env: "*", + want: []string{"*"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + resetOriginEnv(t) + t.Setenv("RELAY_SECURITY_ALLOWED_ORIGINS", tt.env) + + cfg := Default() + applyEnvOverrides(cfg) + + got := cfg.Security.AllowedOrigins + if len(got) != len(tt.want) { + t.Fatalf("AllowedOrigins = %v, want %v", got, tt.want) + } + for i := range got { + if got[i] != tt.want[i] { + t.Errorf("AllowedOrigins[%d] = %q, want %q", i, got[i], tt.want[i]) + } + } + }) + } +} + +func TestValidateAllowedOrigins(t *testing.T) { + tests := []struct { + name string + origins []string + wantErr string // substring; empty means the config must be accepted + }{ + {name: "nil is valid", origins: nil}, + {name: "empty slice is valid", origins: []string{}}, + {name: "wildcard alone is valid", origins: []string{"*"}}, + { + name: "specific origins are valid", + origins: []string{"https://a.example.com", "http://localhost:5173"}, + }, + { + name: "wildcard mixed with a specific origin is rejected", + origins: []string{"*", "https://a.example.com"}, + wantErr: "cannot be combined", + }, + { + name: "a bare hostname is rejected", + origins: []string{"app.example.com"}, + wantErr: "must be a full origin", + }, + { + name: "an origin with a path is rejected", + origins: []string{"https://app.example.com/api"}, + wantErr: "no path component", + }, + { + name: "an empty scheme is rejected", + origins: []string{"://app.example.com"}, + wantErr: "must be a full origin", + }, + { + name: "an empty host is rejected", + origins: []string{"https://"}, + wantErr: "missing host", + }, + { + name: "a scheme-only entry with a slash is rejected", + origins: []string{"https:///"}, + wantErr: "missing host", + }, + { + name: "a lone separator is rejected", + origins: []string{"://"}, + wantErr: "must be a full origin", + }, + { + name: "a trailing slash is tolerated", + origins: []string{"https://app.example.com/"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cfg := Default() + cfg.Security.AllowedOrigins = tt.origins + + err := cfg.Validate() + + if tt.wantErr == "" { + if err != nil { + t.Fatalf("Validate() error = %v, want nil", err) + } + return + } + if err == nil { + t.Fatalf("Validate() error = nil, want an error containing %q", tt.wantErr) + } + if !strings.Contains(err.Error(), tt.wantErr) { + t.Errorf("Validate() error = %q, want it to contain %q", err.Error(), tt.wantErr) + } + }) + } +} + +// TestAllowedOrigins_InvalidEnvIsRejectedByLoad makes sure a bad env value +// fails startup loudly rather than silently producing an allowlist that can +// never match. +func TestAllowedOrigins_InvalidEnvIsRejectedByLoad(t *testing.T) { + resetOriginEnv(t) + t.Setenv("RELAY_SECURITY_ALLOWED_ORIGINS", "app.example.com") + + if _, err := Load("does-not-exist.yaml"); err == nil { + t.Fatal("Load() error = nil, want a validation error for a scheme-less origin") + } +} + +// writeOriginConfig drops a YAML file in a temp dir and returns its path. +// Named distinctly from the helpers in the other config test files so the +// package still compiles once those land alongside this one. +func writeOriginConfig(t *testing.T, body string) string { + t.Helper() + path := filepath.Join(t.TempDir(), "config.yaml") + if err := os.WriteFile(path, []byte(body), 0o600); err != nil { + t.Fatalf("writing temp config: %v", err) + } + return path +} + +// TestAllowedOrigins_FromYAML covers the documented primary route. The +// environment variable is the fallback; config.yaml is what the README and the +// shipped config actually show, so it needs coverage of its own. +func TestAllowedOrigins_FromYAML(t *testing.T) { + resetOriginEnv(t) + + path := writeOriginConfig(t, ` +security: + allowed_origins: + - "https://a.example.com" + - "https://b.example.com" +`) + + cfg, err := Load(path) + if err != nil { + t.Fatalf("Load() error = %v", err) + } + + want := []string{"https://a.example.com", "https://b.example.com"} + if len(cfg.Security.AllowedOrigins) != len(want) { + t.Fatalf("AllowedOrigins = %v, want %v", cfg.Security.AllowedOrigins, want) + } + for i := range want { + if cfg.Security.AllowedOrigins[i] != want[i] { + t.Errorf("AllowedOrigins[%d] = %q, want %q", i, cfg.Security.AllowedOrigins[i], want[i]) + } + } +} + +// TestAllowedOrigins_EnvReplacesYAMLList guards a specific footgun: the +// environment override must REPLACE the YAML list, not append to it. +// Unmarshalling into a pre-populated struct makes slice merging an easy +// accident, and appending would silently keep serving an origin the operator +// thought they had removed. +func TestAllowedOrigins_EnvReplacesYAMLList(t *testing.T) { + resetOriginEnv(t) + + path := writeOriginConfig(t, ` +security: + allowed_origins: + - "https://from-file.example.com" +`) + t.Setenv("RELAY_SECURITY_ALLOWED_ORIGINS", "https://from-env.example.com") + + cfg, err := Load(path) + if err != nil { + t.Fatalf("Load() error = %v", err) + } + + if len(cfg.Security.AllowedOrigins) != 1 { + t.Fatalf("AllowedOrigins = %v, want exactly the env value (env must replace, not append)", + cfg.Security.AllowedOrigins) + } + if got, want := cfg.Security.AllowedOrigins[0], "https://from-env.example.com"; got != want { + t.Errorf("AllowedOrigins[0] = %q, want %q", got, want) + } +} + +// TestAllowedOrigins_YAMLDefaultsToEmpty confirms a config file that says +// nothing about CORS leaves it switched off. +func TestAllowedOrigins_YAMLDefaultsToEmpty(t *testing.T) { + resetOriginEnv(t) + + path := writeOriginConfig(t, "server:\n port: 3000\n") + + cfg, err := Load(path) + if err != nil { + t.Fatalf("Load() error = %v", err) + } + if len(cfg.Security.AllowedOrigins) != 0 { + t.Errorf("AllowedOrigins = %v, want empty", cfg.Security.AllowedOrigins) + } +} + +// TestAllowedOrigins_MalformedYAMLEntryFailsStartup pairs with the Validate +// table: a bad entry in the file must fail Load, not just Validate in isolation. +func TestAllowedOrigins_MalformedYAMLEntryFailsStartup(t *testing.T) { + resetOriginEnv(t) + + path := writeOriginConfig(t, ` +security: + allowed_origins: + - "https://" +`) + + if _, err := Load(path); err == nil { + t.Fatal("Load() error = nil, want a validation error for an origin with no host") + } +} diff --git a/internal/middleware/cors.go b/internal/middleware/cors.go new file mode 100644 index 0000000..c871144 --- /dev/null +++ b/internal/middleware/cors.go @@ -0,0 +1,142 @@ +package middleware + +import ( + "net/http" + "strconv" + "strings" +) + +// preflightMaxAge is how long a browser may cache a preflight result, in +// seconds. Ten minutes keeps preflight chatter down without making an origin +// allowlist change take long to propagate. +const preflightMaxAge = 600 + +// corsWildcard allows any origin. Configuring it is opt-in. +const corsWildcard = "*" + +// allowedMethods mirrors the routes registered in cmd/relay: GET /health, +// POST /api/messages, GET /api/messages/{address}, DELETE /api/messages/{id}. +var allowedMethods = []string{ + http.MethodGet, + http.MethodPost, + http.MethodDelete, + http.MethodOptions, +} + +// CORS returns an HTTP middleware that serves cross-origin headers for the +// origins in allowOrigins. +// +// If allowOrigins is empty the middleware is a no-op passthrough, which is the +// default: the relay serves no CORS headers and OPTIONS keeps 404ing from the +// router, exactly as before. Operators opt in by listing origins, or by +// listing "*" to allow any. +// +// requireAPIKey adds X-API-Key to Access-Control-Allow-Headers, so browsers +// are permitted to send it when the relay has an API key configured. +// +// This middleware must sit OUTERMOST in the chain, ahead of APIKeyAuth. +// Browsers never attach custom headers to a preflight, so an OPTIONS request +// carries no X-API-Key; if authentication ran first every preflight would 401 +// and the real request would never be sent. +func CORS(allowOrigins []string, requireAPIKey bool) func(http.Handler) http.Handler { + allowed, wildcard := normalizeOrigins(allowOrigins) + + allowHeaders := "Content-Type" + if requireAPIKey { + allowHeaders = "Content-Type, X-API-Key" + } + allowMethods := strings.Join(allowedMethods, ", ") + + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Not configured — behave exactly as the relay did before. + if !wildcard && len(allowed) == 0 { + next.ServeHTTP(w, r) + return + } + + origin := r.Header.Get("Origin") + + // Same-origin and non-browser callers (curl, the Go SDK, + // server-to-server) send no Origin. Leave them untouched. + if origin == "" { + next.ServeHTTP(w, r) + return + } + + // The response now varies by request origin. Set this even when + // the origin is rejected, so a shared cache cannot serve one + // origin's response to another. + w.Header().Add("Vary", "Origin") + + if !originAllowed(allowed, wildcard, origin) { + // A preflight we will not honour is answered directly, so the + // caller gets a clear status instead of a confusing 404 from + // the router. No CORS headers are emitted, so the browser + // blocks the real request either way. + if isPreflight(r) { + http.Error(w, "origin not allowed by CORS policy", http.StatusForbidden) + return + } + // A non-preflight request from an unlisted origin is still + // served; without the header the browser hides the response. + next.ServeHTTP(w, r) + return + } + + // Echo the concrete origin rather than "*". It is required if a + // caller ever uses credentials, and it keeps the response honest + // about which origin it was built for. + w.Header().Set("Access-Control-Allow-Origin", origin) + + if isPreflight(r) { + w.Header().Set("Access-Control-Allow-Methods", allowMethods) + w.Header().Set("Access-Control-Allow-Headers", allowHeaders) + w.Header().Set("Access-Control-Max-Age", strconv.Itoa(preflightMaxAge)) + w.WriteHeader(http.StatusNoContent) + return + } + + next.ServeHTTP(w, r) + }) + } +} + +// isPreflight reports whether r is a CORS preflight: an OPTIONS request +// carrying Access-Control-Request-Method. A bare OPTIONS is not a preflight +// and is left to the router. +func isPreflight(r *http.Request) bool { + return r.Method == http.MethodOptions && + r.Header.Get("Access-Control-Request-Method") != "" +} + +// normalizeOrigins trims, lowercases and de-duplicates the configured origins, +// dropping empties and any trailing slash (a common config slip — an Origin +// header never has one). It reports whether the wildcard was present. +func normalizeOrigins(origins []string) (map[string]struct{}, bool) { + allowed := make(map[string]struct{}, len(origins)) + wildcard := false + + for _, o := range origins { + o = strings.TrimSpace(o) + if o == "" { + continue + } + if o == corsWildcard { + wildcard = true + continue + } + allowed[strings.ToLower(strings.TrimSuffix(o, "/"))] = struct{}{} + } + + return allowed, wildcard +} + +// originAllowed reports whether origin passes the allowlist. +func originAllowed(allowed map[string]struct{}, wildcard bool, origin string) bool { + if wildcard { + return true + } + _, ok := allowed[strings.ToLower(strings.TrimSuffix(origin, "/"))] + return ok +} diff --git a/internal/middleware/cors_test.go b/internal/middleware/cors_test.go new file mode 100644 index 0000000..014a66d --- /dev/null +++ b/internal/middleware/cors_test.go @@ -0,0 +1,354 @@ +package middleware + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// okHandler records whether the request reached the end of the chain. +func okHandler(reached *bool) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + *reached = true + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("ok")) + }) +} + +// preflightReq builds a browser-style CORS preflight for the given origin. +func preflightReq(origin, method string) *http.Request { + r := httptest.NewRequest(http.MethodOptions, "/api/messages", nil) + r.Header.Set("Origin", origin) + r.Header.Set("Access-Control-Request-Method", method) + return r +} + +// simpleReq builds an ordinary cross-origin request. +func simpleReq(origin string) *http.Request { + r := httptest.NewRequest(http.MethodGet, "/api/messages/alice", nil) + if origin != "" { + r.Header.Set("Origin", origin) + } + return r +} + +func TestCORS_DisabledByDefault(t *testing.T) { + reached := false + h := CORS(nil, false)(okHandler(&reached)) + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, simpleReq("https://app.example.com")) + + if !reached { + t.Error("request did not reach the next handler") + } + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "" { + t.Errorf("Access-Control-Allow-Origin = %q, want empty when CORS is off", got) + } + if got := rec.Header().Get("Vary"); got != "" { + t.Errorf("Vary = %q, want empty when CORS is off", got) + } +} + +func TestCORS_DisabledLeavesPreflightToTheRouter(t *testing.T) { + reached := false + h := CORS(nil, false)(okHandler(&reached)) + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, preflightReq("https://app.example.com", http.MethodPost)) + + if !reached { + t.Error("preflight was intercepted even though CORS is disabled") + } +} + +func TestCORS_AllowedOriginOnSimpleRequest(t *testing.T) { + reached := false + h := CORS([]string{"https://app.example.com"}, false)(okHandler(&reached)) + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, simpleReq("https://app.example.com")) + + if !reached { + t.Error("request did not reach the next handler") + } + if got, want := rec.Header().Get("Access-Control-Allow-Origin"), "https://app.example.com"; got != want { + t.Errorf("Access-Control-Allow-Origin = %q, want %q", got, want) + } + if got, want := rec.Header().Get("Vary"), "Origin"; got != want { + t.Errorf("Vary = %q, want %q", got, want) + } +} + +func TestCORS_PreflightIsAnsweredDirectly(t *testing.T) { + reached := false + h := CORS([]string{"https://app.example.com"}, false)(okHandler(&reached)) + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, preflightReq("https://app.example.com", http.MethodPost)) + + if reached { + t.Error("preflight reached the next handler; it should be answered by the middleware") + } + if got, want := rec.Code, http.StatusNoContent; got != want { + t.Errorf("status = %d, want %d", got, want) + } + if got, want := rec.Header().Get("Access-Control-Allow-Origin"), "https://app.example.com"; got != want { + t.Errorf("Access-Control-Allow-Origin = %q, want %q", got, want) + } + + methods := rec.Header().Get("Access-Control-Allow-Methods") + for _, m := range []string{"GET", "POST", "DELETE"} { + if !strings.Contains(methods, m) { + t.Errorf("Access-Control-Allow-Methods = %q, want it to include %s", methods, m) + } + } + if got := rec.Header().Get("Access-Control-Max-Age"); got == "" { + t.Error("Access-Control-Max-Age is not set") + } +} + +func TestCORS_AllowHeadersTracksAPIKey(t *testing.T) { + tests := []struct { + name string + requireAPIKey bool + wantAPIKey bool + }{ + {name: "no api key configured", requireAPIKey: false, wantAPIKey: false}, + {name: "api key configured", requireAPIKey: true, wantAPIKey: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + reached := false + h := CORS([]string{"https://app.example.com"}, tt.requireAPIKey)(okHandler(&reached)) + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, preflightReq("https://app.example.com", http.MethodPost)) + + got := rec.Header().Get("Access-Control-Allow-Headers") + if !strings.Contains(got, "Content-Type") { + t.Errorf("Access-Control-Allow-Headers = %q, want it to include Content-Type", got) + } + if hasKey := strings.Contains(got, "X-API-Key"); hasKey != tt.wantAPIKey { + t.Errorf("Access-Control-Allow-Headers = %q, X-API-Key present = %v, want %v", + got, hasKey, tt.wantAPIKey) + } + }) + } +} + +func TestCORS_DisallowedOriginPreflightIsRejected(t *testing.T) { + reached := false + h := CORS([]string{"https://app.example.com"}, false)(okHandler(&reached)) + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, preflightReq("https://evil.example.com", http.MethodPost)) + + if reached { + t.Error("rejected preflight reached the next handler") + } + if got, want := rec.Code, http.StatusForbidden; got != want { + t.Errorf("status = %d, want %d", got, want) + } + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "" { + t.Errorf("Access-Control-Allow-Origin = %q, want empty for a disallowed origin", got) + } + if got, want := rec.Header().Get("Vary"), "Origin"; got != want { + t.Errorf("Vary = %q, want %q even on rejection, to keep shared caches honest", got, want) + } +} + +func TestCORS_DisallowedOriginSimpleRequestGetsNoHeader(t *testing.T) { + reached := false + h := CORS([]string{"https://app.example.com"}, false)(okHandler(&reached)) + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, simpleReq("https://evil.example.com")) + + if !reached { + t.Error("non-preflight request should still be served") + } + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "" { + t.Errorf("Access-Control-Allow-Origin = %q, want empty; the browser must block the read", got) + } +} + +func TestCORS_NoOriginHeaderIsUntouched(t *testing.T) { + reached := false + h := CORS([]string{"https://app.example.com"}, false)(okHandler(&reached)) + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, simpleReq("")) + + if !reached { + t.Error("request without Origin did not reach the next handler") + } + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "" { + t.Errorf("Access-Control-Allow-Origin = %q, want empty for a non-browser caller", got) + } + if got := rec.Header().Get("Vary"); got != "" { + t.Errorf("Vary = %q, want empty when no Origin was sent", got) + } +} + +func TestCORS_Wildcard(t *testing.T) { + reached := false + h := CORS([]string{"*"}, false)(okHandler(&reached)) + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, simpleReq("https://anything.example.com")) + + if got, want := rec.Header().Get("Access-Control-Allow-Origin"), "https://anything.example.com"; got != want { + t.Errorf("Access-Control-Allow-Origin = %q, want the echoed origin %q", got, want) + } +} + +func TestCORS_NeverAllowsCredentials(t *testing.T) { + // Credentialed CORS is not supported: the relay authenticates with a + // header, not cookies, and advertising credentials alongside "*" would be + // a footgun. + reached := false + h := CORS([]string{"*"}, true)(okHandler(&reached)) + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, preflightReq("https://anything.example.com", http.MethodPost)) + + if got := rec.Header().Get("Access-Control-Allow-Credentials"); got != "" { + t.Errorf("Access-Control-Allow-Credentials = %q, want it never to be set", got) + } +} + +func TestCORS_OriginNormalization(t *testing.T) { + tests := []struct { + name string + configure string + request string + wantAllow bool + }{ + { + name: "exact match", configure: "https://app.example.com", + request: "https://app.example.com", wantAllow: true, + }, + { + name: "configured with a trailing slash", configure: "https://app.example.com/", + request: "https://app.example.com", wantAllow: true, + }, + { + name: "configured with surrounding spaces", configure: " https://app.example.com ", + request: "https://app.example.com", wantAllow: true, + }, + { + name: "mixed case in config", configure: "https://APP.example.com", + request: "https://app.example.com", wantAllow: true, + }, + { + name: "a different scheme is a different origin", configure: "https://app.example.com", + request: "http://app.example.com", wantAllow: false, + }, + { + name: "a different port is a different origin", configure: "https://app.example.com", + request: "https://app.example.com:8443", wantAllow: false, + }, + { + name: "suffix attack is not a match", configure: "https://app.example.com", + request: "https://app.example.com.evil.tld", wantAllow: false, + }, + { + name: "subdomains are not implicitly allowed", configure: "https://example.com", + request: "https://sub.example.com", wantAllow: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + reached := false + h := CORS([]string{tt.configure}, false)(okHandler(&reached)) + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, simpleReq(tt.request)) + + allowed := rec.Header().Get("Access-Control-Allow-Origin") != "" + if allowed != tt.wantAllow { + t.Errorf("origin %q against config %q: allowed = %v, want %v", + tt.request, tt.configure, allowed, tt.wantAllow) + } + }) + } +} + +func TestCORS_BareOptionsIsNotAPreflight(t *testing.T) { + // An OPTIONS request with no Access-Control-Request-Method is not a + // preflight; it belongs to the router, not to this middleware. + reached := false + h := CORS([]string{"https://app.example.com"}, false)(okHandler(&reached)) + + r := httptest.NewRequest(http.MethodOptions, "/api/messages", nil) + r.Header.Set("Origin", "https://app.example.com") + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, r) + + if !reached { + t.Error("bare OPTIONS was swallowed by the CORS middleware") + } +} + +func TestCORS_EmptyStringOriginsAreIgnored(t *testing.T) { + // A config like RELAY_SECURITY_ALLOWED_ORIGINS="," must not become an + // allowlist containing the empty origin. + reached := false + h := CORS([]string{"", " "}, false)(okHandler(&reached)) + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, simpleReq("https://evil.example.com")) + + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "" { + t.Errorf("Access-Control-Allow-Origin = %q, want empty", got) + } + if !reached { + t.Error("request should pass through when the allowlist is effectively empty") + } +} + +// TestCORS_PreflightSurvivesAPIKeyAuth is the middleware-ordering regression +// test. Browsers never attach X-API-Key to a preflight, so CORS must answer it +// before APIKeyAuth has a chance to reject it. +func TestCORS_PreflightSurvivesAPIKeyAuth(t *testing.T) { + reached := false + + h := okHandler(&reached) + h = APIKeyAuth("secret-key")(h) + h = CORS([]string{"https://app.example.com"}, true)(h) + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, preflightReq("https://app.example.com", http.MethodPost)) + + if got, want := rec.Code, http.StatusNoContent; got != want { + t.Errorf("preflight status = %d, want %d (401 means CORS is nested inside APIKeyAuth)", got, want) + } + if got, want := rec.Header().Get("Access-Control-Allow-Origin"), "https://app.example.com"; got != want { + t.Errorf("Access-Control-Allow-Origin = %q, want %q", got, want) + } +} + +// TestCORS_ActualRequestStillNeedsTheAPIKey confirms CORS has not become an +// authentication bypass for real (non-preflight) requests. +func TestCORS_ActualRequestStillNeedsTheAPIKey(t *testing.T) { + reached := false + + h := okHandler(&reached) + h = APIKeyAuth("secret-key")(h) + h = CORS([]string{"https://app.example.com"}, true)(h) + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, simpleReq("https://app.example.com")) + + if reached { + t.Error("request without an API key reached the handler") + } + if got, want := rec.Code, http.StatusUnauthorized; got != want { + t.Errorf("status = %d, want %d", got, want) + } +}