fix: prevent data races from shared header value slices#213
Conversation
Package-level slices (headerVaryOrigin, headerOriginAll, headerTrue) and struct field slices (preflightVary, exposedHeaders, maxAge) were assigned directly to response header maps. If a downstream handler mutates response headers (e.g. via append or sort), it could modify the shared backing array, causing data races under concurrent access. Replace shared mutable slices with fresh slice literals created at each call site, ensuring each request gets its own backing array. Fixes rs#198
There was a problem hiding this comment.
Pull request overview
Updates the CORS middleware to eliminate shared mutable []string backing arrays being assigned into http.Header, preventing downstream handlers from mutating shared slices and triggering data races under concurrent requests (Fixes #198).
Changes:
- Replaces package-level
[]stringvars for common header values withconststrings. - Assigns fresh
[]string{...}slices into response headers at each write site to avoid shared backing arrays. - Avoids assigning internal precomputed slices (
preflightVary,exposedHeaders,maxAge) directly intohttp.Header.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if vary, found := headers["Vary"]; found { | ||
| headers["Vary"] = append(vary, c.preflightVary[0]) | ||
| } else { | ||
| headers["Vary"] = c.preflightVary | ||
| headers["Vary"] = []string{c.preflightVary[0]} | ||
| } |
There was a problem hiding this comment.
headers["Vary"] = []string{c.preflightVary[0]} assumes c.preflightVary is non-nil and non-empty. Previously, when Vary was not already present, a zero-value Cors{} would not panic here (it would assign nil), but this change will always panic on preflight requests if preflightVary is unset. Consider cloning the slice instead (e.g., append([]string(nil), c.preflightVary...)) so it remains safe for zero-values and future changes where preflightVary may contain more than one element.
| if c.allowedOriginsAll { | ||
| headers["Access-Control-Allow-Origin"] = headerOriginAll | ||
| headers["Access-Control-Allow-Origin"] = []string{headerOriginAllValue} | ||
| } else { | ||
| headers["Access-Control-Allow-Origin"] = r.Header["Origin"] | ||
| } | ||
| if len(c.exposedHeaders) > 0 { | ||
| headers["Access-Control-Expose-Headers"] = c.exposedHeaders | ||
| headers["Access-Control-Expose-Headers"] = []string{c.exposedHeaders[0]} | ||
| } | ||
| if c.allowCredentials { | ||
| headers["Access-Control-Allow-Credentials"] = headerTrue | ||
| headers["Access-Control-Allow-Credentials"] = []string{headerTrueValue} | ||
| } |
There was a problem hiding this comment.
This change is fixing a concurrency/data-race footgun by avoiding reuse of backing arrays in http.Header values, but there isn’t a regression test asserting that handlers cannot mutate shared header value slices (and/or that concurrent requests don’t race under -race). Adding a focused test that mutates w.Header()[...] inside a wrapped handler and runs many concurrent requests would help prevent reintroducing the issue.
|
append would be thread safe since the slices are at capacity. Sorting or setting headers by index is unrealistic use cases. @Yanhu007 did you trigger this bug in your own code? |
Fixes #198
Problem
Package-level slice variables (
headerVaryOrigin,headerOriginAll,headerTrue) and struct field slices (preflightVary,exposedHeaders,maxAge) are assigned directly tohttp.Headermaps. Sincehttp.Headervalues are[]string, downstream handlers can mutate the backing array (e.g., viaappend,slices.Sort, or direct index writes), causing data races when the middleware serves concurrent requests.Fix
varslice declarations withconststring values[]string{...}literals at each assignment site, so each request gets its own backing arraypreflightVary,exposedHeaders,maxAge)This eliminates the shared mutable state while keeping the same observable behavior. The allocation cost is negligible — one small slice per CORS header per request.
Testing
All existing tests pass, including with
-race.