fix(fetch): forward request headers to the outgoing HTTP request - #270
fix(fetch): forward request headers to the outgoing HTTP request#270Sonic-Y3k wants to merge 2 commits into
Conversation
WalkthroughThis pull request updates fetch request handling to forward stored headers onto the outgoing HTTP request and to normalize appended header names before forbidden-header checks. It also adds tests that verify custom headers are forwarded and 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Request.createHttpReq built the *http.Request but never copied the request's Headers onto it, so any header set via fetch's RequestInit (Content-Type, Authorization, custom API headers, ...) was silently dropped. Servers that require those headers would reject the request.
abfda9a to
f33b56b
Compare
|
Lol, I'm surprised this was never implemented, as I could remember spending some time dealing with headers; but that was apparently only for responses :D |
| // Forward the request headers to the outgoing HTTP request. Without this, | ||
| // headers set via fetch's RequestInit (e.g. Content-Type, Authorization, or | ||
| // custom API headers) were silently dropped, causing servers that require | ||
| // them to reject the request. |
There was a problem hiding this comment.
This is one of the things I dislike about AI-generated code, excessive comments. This comments tells nothing that isn't immediately readable from from code.
Same goes for function documentation, doesn't IHMO add anything.
There was a problem hiding this comment.
Yeah, I do agree with this. Will remove the doc comment.
| // headers set via fetch's RequestInit (e.g. Content-Type, Authorization, or | ||
| // custom API headers) were silently dropped, causing servers that require | ||
| // them to reject the request. | ||
| for name, value := range r.Headers.All() { |
There was a problem hiding this comment.
Looking at Headers.All() there's some special handling of Set-Cookie. And since headers were only implemented (to my surprise) for responses, I'm unsure if cookies should have special handling in request headers.
Actual handling of cookies in real requests is handled by Go's http module using a CookieJar.
And that makes me think that perhaps there should be a dedicated test of how adding Set-Cookie headers affects the actual HTTP request, not the returned *http.Request value, but the resulting request sent from the http.Client to the http.Roundtripper.
There was a problem hiding this comment.
Yeah... this turned up a bug. Headers.Append checked the forbidden-header list before lower-casing the name, and that list is lower-cased, so a capitalised Cookie/Set-Cookie bypassed the filter and got forwarded. I made Append normalise the name first (forbidden header names match case-insensitively per the Fetch spec). cookie/set-cookie are already in invalidRequestHeaders, and real request cookies go through the http.Client cookie jar, so the forwarding loop needs no Set-Cookie handling.
Added TestFetchDoesNotForwardCookieHeaders to cover it. On testing the actual sent request: TestFetchForwardsHeaders already asserts against the *http.Request the recorder captures inside the handler - i.e. after the client -> round-tripper, so it's the resulting request, not the pre-flight value.
| fetch.WithMethod("POST"), | ||
| fetch.WithHeaders([][2]types.ByteString{ | ||
| {"Content-Type", "application/json+protobuf"}, | ||
| {"X-Goog-Api-Key", "secret-key"}, |
There was a problem hiding this comment.
very nitpicky, but I'd use something more anonymous, like X-Example-Header - that something that smells like a specific service
There was a problem hiding this comment.
Not nitpicky at all ;-) switched to X-Example-Header (and swapped application/json+protobuf for a plain application/json while here)
Match forbidden request header names case-insensitively so Cookie and Set-Cookie set on a fetch Request are no longer forwarded to the outgoing request; real request cookies are managed by the http.Client cookie jar. Also drop the redundant code comments flagged in review and use a non-service-specific header name in the test.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 307bd1d3-cc87-47aa-876f-ef9fc27495d6
📒 Files selected for processing (3)
internal/fetch/fetch.gointernal/fetch/headers.gointernal/fetch/request_headers_test.go
| if slices.Index(h.invalidHeaders, name) != -1 { | ||
| return | ||
| } | ||
| h.headers = insertSorted(h.headers, Header{key: name.ToLower(), val: val}, compareHeaders) | ||
| h.headers = insertSorted(h.headers, Header{key: name, val: val}, compareHeaders) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Handle forbidden header prefixes here, not just exact names.
name = name.ToLower() fixes case-insensitive exact matches, but slices.Index(h.invalidHeaders, name) still treats the list as exact names only. That means Sec-Fetch-Site/Sec-CH-UA and Proxy-Authorization will bypass this check because "sec-fetch-site" and "proxy-authorization" do not equal the sentinel entries "sec-"/"proxy-" shown in invalidRequestHeaders below. With Request.createHttpReq() now forwarding every stored header, those forbidden prefixes will still leak onto the outbound request.
Suggested fix
func (h *Headers) Append(name, val types.ByteString) {
// Forbidden header names are matched case-insensitively, so normalise the
// name before both the check and insertion. Keeping the list sorted is
// imperative for correct iteration behaviour.
name = name.ToLower()
- if slices.Index(h.invalidHeaders, name) != -1 {
+ if isForbiddenHeader(name, h.invalidHeaders) {
return
}
h.headers = insertSorted(h.headers, Header{key: name, val: val}, compareHeaders)
}
+
+func isForbiddenHeader(name types.ByteString, invalid []types.ByteString) bool {
+ if slices.Index(invalid, name) != -1 {
+ return true
+ }
+ s := string(name)
+ return strings.HasPrefix(s, "proxy-") || strings.HasPrefix(s, "sec-")
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if slices.Index(h.invalidHeaders, name) != -1 { | |
| return | |
| } | |
| h.headers = insertSorted(h.headers, Header{key: name.ToLower(), val: val}, compareHeaders) | |
| h.headers = insertSorted(h.headers, Header{key: name, val: val}, compareHeaders) | |
| if isForbiddenHeader(name, h.invalidHeaders) { | |
| return | |
| } | |
| h.headers = insertSorted(h.headers, Header{key: name, val: val}, compareHeaders) | |
| } | |
| func isForbiddenHeader(name types.ByteString, invalid []types.ByteString) bool { | |
| if slices.Index(invalid, name) != -1 { | |
| return true | |
| } | |
| s := string(name) | |
| return strings.HasPrefix(s, "proxy-") || strings.HasPrefix(s, "sec-") | |
| } |
2 participants
Request.createHttpReqbuilds the*http.Requestbut never copies the request'sHeadersonto it. As a result, any header set via fetch'sRequestInit(Content-Type,Authorization, custom API headers, …) is silently dropped, and servers that require those headers reject the request.This forwards the request headers to the outgoing request.
Testing: added
TestFetchForwardsHeaders;go test ./internal/fetch/passes.AI disclosure: This change was developed with the help of an AI coding assistant. I've reviewed and tested it myself; it follows the existing conventions and the full test suite (main module,
v8engine,sobekengine) passes locally.