Fix CORS preflight Access-Control-Allow-Headers when wildcard configured
#37
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes CORS preflight handling to properly echo back requested headers when a wildcard configuration is used, rather than always returning "*" in the Access-Control-Allow-Headers response header.
Changes:
- Modified the CORS middleware to detect wildcard header configuration and echo back the browser's
Access-Control-Request-Headersvalue in preflight responses - Computed
allow_headersdynamically based on whether wildcards are configured and what headers the client requests - Maintained backward compatibility for non-wildcard configurations
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let allow_headers = if self.headers.iter().any(|value| value == "*") { | ||
| req.headers() | ||
| .get(header::ACCESS_CONTROL_REQUEST_HEADERS) | ||
| .and_then(|value| value.to_str().ok()) | ||
| .filter(|value| !value.trim().is_empty()) | ||
| .map(str::to_string) | ||
| .unwrap_or_else(|| "*".to_string()) | ||
| } else { | ||
| self.headers_header_value() | ||
| }; |
There was a problem hiding this comment.
The allow_headers variable is computed for all requests (lines 198-207), but it's only used during preflight requests (line 268). This adds unnecessary overhead for normal requests.
Consider moving this computation inside the if is_preflight block (after line 236) so it only executes when needed.
| let allow_headers = if self.headers.iter().any(|value| value == "*") { | ||
| req.headers() | ||
| .get(header::ACCESS_CONTROL_REQUEST_HEADERS) | ||
| .and_then(|value| value.to_str().ok()) | ||
| .filter(|value| !value.trim().is_empty()) | ||
| .map(str::to_string) | ||
| .unwrap_or_else(|| "*".to_string()) | ||
| } else { | ||
| self.headers_header_value() | ||
| }; |
There was a problem hiding this comment.
This CORS functionality lacks test coverage. The change modifies critical CORS preflight behavior but includes no tests to verify the new wildcard header echoing logic works correctly or that it doesn't break existing functionality.
Many other modules in this crate (e.g., jwt, rate_limit, api_key) have comprehensive test coverage. Consider adding tests that verify:
- Wildcard headers echo the request headers correctly
- Empty or whitespace-only request headers fall back to "*"
- Non-wildcard configurations still work as before
- Edge cases like missing Access-Control-Request-Headers are handled properly
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Motivation
"*"for allowed headers should reflect the browser's requested headers on preflight so the response matches what the client expects.Access-Control-Allow-Headers: *, which can cause issues when the browser sends specificAccess-Control-Request-Headersthat must be echoed back.Description
"*", the middleware now reads the incomingAccess-Control-Request-Headersheader and uses that value forAccess-Control-Allow-Headersif present, otherwise falls back to"*".allow_headerscomputed value and uses it when building the preflight response instead of the static header string.Testing
Codex Task