fix(engine): accept bracketed IPv6 and port-suffixed entries in X-Forwarded-For#4634
fix(engine): accept bracketed IPv6 and port-suffixed entries in X-Forwarded-For#4634BootstrapperSBL wants to merge 1 commit intogin-gonic:masterfrom
Conversation
…warded-For `validateHeader` called `net.ParseIP` directly on each comma-split item, so anything with brackets or a `:port` suffix got rejected silently and `ClientIP()` fell through to `RemoteAddr` — which means a client coming in through IIS/ARR or certain cloud LBs would show up as the reverse proxy instead of the real caller. The four forms called out in gin-gonic#4572 are all normal real-world outputs: - "192.168.8.39" - "240e:318:2f4a:de56::240" - "[240e:318:2f4a:de56::240]" - "192.168.8.39:38792" - "[240e:318:2f4a:de56::240]:38792" Extract a small `parseForwardedForItem` helper that tries `net.SplitHostPort` first (handles the two `:port` variants and strips brackets in the process) and falls back to bracket-stripping + `net.ParseIP` for bare `[ipv6]`. The returned `clientIP` is now always the bare IP regardless of which proxy produced the header, which keeps the shape of `ClientIP()` stable. Table tests cover all four reporter-listed forms, plus a chain with a port on the last entry and a couple of garbage inputs. Closes gin-gonic#4572
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4634 +/- ##
==========================================
- Coverage 99.21% 98.37% -0.84%
==========================================
Files 42 48 +6
Lines 3182 3148 -34
==========================================
- Hits 3157 3097 -60
- Misses 17 42 +25
- Partials 8 9 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Heads up on the failing |
|
Hey, just checking if you've had a chance to look at this — happy to adjust anything if needed. |
|
Hi, parseForwardedForItem’s doc comment says it accepts “four” common forms but lists five, which is internally inconsistent and can confuse maintainers. Severity: informational | Category: maintainability How to fix: Fix docstring count Agent prompt to fix - you can give this to your LLM of choice:
Found by Qodo code review |
2 participants
What
Context.ClientIP()now parsesX-Forwarded-Forentries in the bracketed and port-suffixed forms produced by IIS/ARR and several cloud load balancers.Closes #4572.
Why
validateHeaderingin.gohanded each comma-split item straight tonet.ParseIP, which silently rejects anything other than a bare1.2.3.4or2001:db8::1. In practice proxies often emit:ClientIP()X-Forwarded-For: 192.168.8.39192.168.8.39192.168.8.39(unchanged)X-Forwarded-For: 240e:318:2f4a:de56::240240e:318:2f4a:de56::240240e:318:2f4a:de56::240(unchanged)X-Forwarded-For: [240e:318:2f4a:de56::240]RemoteAddr240e:318:2f4a:de56::240X-Forwarded-For: 192.168.8.39:38792RemoteAddr192.168.8.39X-Forwarded-For: [240e:318:2f4a:de56::240]:38792RemoteAddr240e:318:2f4a:de56::240The last three rows are the failure modes from the report.
How
A small
parseForwardedForItemhelper:net.SplitHostPortfirst — this handlesip:portand[ipv6]:portand already strips the brackets for us.[]and callingnet.ParseIP— covers the bare[ipv6]case.ClientIP()'s output shape is identical across all proxy flavours.validateHeader's trusted-proxy reverse-walk logic is untouched — only the per-item parse changes.Test plan
Table-driven test
TestValidateHeaderForwardedForFormscovers:[::1](bracketed loopback)[garbage]negative casesAlso verified:
go build ./...go vet ./...go test -race ./...(whole tree green, includingTestContextClientIP*)golangci-lint run --verbose(v2.11.4, matches CI) — 0 issues