From 0073831d02b03f27edf63813c87e7093d40629f9 Mon Sep 17 00:00:00 2001 From: Atharva0506 Date: Thu, 20 Aug 2026 12:58:24 +0530 Subject: [PATCH 1/2] feat: add configurable CORS support for direct browser clients The relay served no CORS headers, and http.ServeMux registers no OPTIONS route, so a browser calling it cross-origin failed its preflight with a 404 and no Access-Control-Allow-Origin. Consumers could only reach the relay through a same-origin reverse proxy. That is fine for anyone willing to run one, but ThruBox ships as a general-purpose relay and consumers without a proxy had no way to call it from a browser. Add a CORS middleware driven by a new security.allowed_origins list (RELAY_SECURITY_ALLOWED_ORIGINS, comma-separated). It is off by default: with no origins configured the middleware is a passthrough and the relay behaves exactly as before, so this is not a behaviour change for anyone already deployed. The middleware sits outermost, ahead of APIKeyAuth. Browsers never attach custom headers to a preflight, so an OPTIONS request carries no X-API-Key; nested inside authentication every preflight would 401 and the real request would never be sent. Actual requests still pass through APIKeyAuth and the rate limiter unchanged -- CORS is not a bypass. Details: - Preflights are answered directly with 204, Allow-Methods, Allow-Headers (Content-Type, plus X-API-Key when an API key is configured) and a 10 minute Max-Age. - The concrete origin is echoed, never "*", and Vary: Origin is always set once CORS is active so shared caches cannot cross origins. - Access-Control-Allow-Credentials is never sent; the relay authenticates with a header, not cookies. - Origins are matched exactly after trimming, lowercasing and dropping a trailing slash. Suffix and subdomain lookalikes do not match. - Malformed entries (no scheme, or a path component) and "*" mixed with specific origins are rejected by Validate at startup rather than silently never matching. Closes #32 --- README.md | 48 +++++ cmd/relay/main.go | 14 +- config.yaml | 7 + internal/config/config.go | 63 ++++++ internal/config/security_test.go | 149 +++++++++++++ internal/middleware/cors.go | 142 +++++++++++++ internal/middleware/cors_test.go | 354 +++++++++++++++++++++++++++++++ 7 files changed, 776 insertions(+), 1 deletion(-) create mode 100644 internal/config/security_test.go create mode 100644 internal/middleware/cors.go create mode 100644 internal/middleware/cors_test.go diff --git a/README.md b/README.md index a263a55..13a511d 100644 --- a/README.md +++ b/README.md @@ -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..bd8dc52 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,45 @@ 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 + if !strings.Contains(o, "://") { + return fmt.Errorf( + "invalid allowed_origins entry %q: must be a full origin such as https://app.example.com", o) + } + if strings.Contains(strings.TrimSuffix(o, "/"), "/") { + after := strings.SplitN(o, "://", 2)[1] + if strings.Contains(strings.TrimSuffix(after, "/"), "/") { + 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..a0fe408 --- /dev/null +++ b/internal/config/security_test.go @@ -0,0 +1,149 @@ +package config + +import ( + "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: "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") + } +} 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..e6e5212 --- /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 + + var h http.Handler = 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 + + var h http.Handler = 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) + } +} From 7ccc7fc8f449e0cd2a54e2e1877b297667453448 Mon Sep 17 00:00:00 2001 From: Atharva0506 Date: Thu, 20 Aug 2026 19:48:29 +0530 Subject: [PATCH 2/2] fix: reject allowed_origins entries with no scheme or no host validateAllowedOrigins accepted "://app.example.com", "https://", "http://", "://" and "https:///". Every one of them passes today and none can ever match a browser Origin header, which is exactly the class of typo the function exists to catch. Parse the entry with strings.Cut and require both a scheme and a host. That rewrite also removes a redundant guard: the outer path check could never be false, since an entry reaching it always contains "://" and therefore always contains "/". The inner check was doing all the work. Test coverage follows the config the README actually documents. The YAML allowed_origins key had no test at all despite being the primary route, and nothing asserted that the environment variable replaces a YAML list rather than appending to it -- a quiet way to keep serving an origin the operator believed they had removed. Also drop the redundant type on two var declarations in the middleware tests, and update the README note that still claimed the repository has no test files. Addresses CodeRabbit review feedback on #36. --- README.md | 2 +- internal/config/config.go | 19 +++-- internal/config/security_test.go | 124 +++++++++++++++++++++++++++++++ internal/middleware/cors_test.go | 4 +- 4 files changed, 139 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index 13a511d..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 diff --git a/internal/config/config.go b/internal/config/config.go index bd8dc52..2bb063e 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -207,16 +207,21 @@ func validateAllowedOrigins(origins []string) error { continue } hasSpecific = true - if !strings.Contains(o, "://") { + + 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) } - if strings.Contains(strings.TrimSuffix(o, "/"), "/") { - after := strings.SplitN(o, "://", 2)[1] - if strings.Contains(strings.TrimSuffix(after, "/"), "/") { - return fmt.Errorf( - "invalid allowed_origins entry %q: an origin has no path component", 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) } } diff --git a/internal/config/security_test.go b/internal/config/security_test.go index a0fe408..2dfb6de 100644 --- a/internal/config/security_test.go +++ b/internal/config/security_test.go @@ -1,6 +1,8 @@ package config import ( + "os" + "path/filepath" "strings" "testing" ) @@ -107,6 +109,26 @@ func TestValidateAllowedOrigins(t *testing.T) { 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/"}, @@ -147,3 +169,105 @@ func TestAllowedOrigins_InvalidEnvIsRejectedByLoad(t *testing.T) { 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_test.go b/internal/middleware/cors_test.go index e6e5212..014a66d 100644 --- a/internal/middleware/cors_test.go +++ b/internal/middleware/cors_test.go @@ -318,7 +318,7 @@ func TestCORS_EmptyStringOriginsAreIgnored(t *testing.T) { func TestCORS_PreflightSurvivesAPIKeyAuth(t *testing.T) { reached := false - var h http.Handler = okHandler(&reached) + h := okHandler(&reached) h = APIKeyAuth("secret-key")(h) h = CORS([]string{"https://app.example.com"}, true)(h) @@ -338,7 +338,7 @@ func TestCORS_PreflightSurvivesAPIKeyAuth(t *testing.T) { func TestCORS_ActualRequestStillNeedsTheAPIKey(t *testing.T) { reached := false - var h http.Handler = okHandler(&reached) + h := okHandler(&reached) h = APIKeyAuth("secret-key")(h) h = CORS([]string{"https://app.example.com"}, true)(h)