Conversation
There was a problem hiding this comment.
looplj has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
Code Review
This pull request enhances header filtering in MergeHTTPHeaders by blocking standard hop-by-hop headers and dynamically identified connection-specific headers listed in Connection or Proxy-Connection fields. Feedback highlights a potential security vulnerability where non-canonical header keys could bypass existing filters, and suggests improving efficiency by lazily initializing the header map only when necessary.
| connectionSpecificHeaders := make(map[string]bool) | ||
| for _, headerKey := range []string{"Connection", "Proxy-Connection"} { | ||
| for _, headerValue := range src.Values(headerKey) { | ||
| for _, token := range strings.Split(headerValue, ",") { | ||
| token = strings.TrimSpace(token) | ||
| if token == "" { | ||
| continue | ||
| } | ||
|
|
||
| connectionSpecificHeaders[http.CanonicalHeaderKey(token)] = true | ||
| } | ||
| } | ||
| } | ||
|
|
||
| for k, v := range src { | ||
| if sensitiveHeaders[k] || libManagedHeaders[k] || isBlockedHeader(k) { | ||
| if sensitiveHeaders[k] || libManagedHeaders[k] || isBlockedHeader(k) || connectionSpecificHeaders[k] { | ||
| continue |
There was a problem hiding this comment.
The current implementation has two areas for improvement:
- Efficiency: The
connectionSpecificHeadersmap is allocated on every call toMergeHTTPHeaders. It is more efficient to initialize it lazily only if connection-specific headers (likeConnectionorProxy-Connection) are actually present in the source. - Correctness & Security: The loop iterates over
srcusing raw keys. If the source headers contain non-canonical keys (e.g., lowercaseauthorization), they will bypass the filters for sensitive, blocked, and connection-specific headers. Canonicalizing the key at the start of the loop ensures that all security and hop-by-hop filters are applied reliably and that the destination header map is normalized.
There was a problem hiding this comment.
looplj has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Uh oh!
There was an error while loading. Please reload this page.