Tags: go-chi/chi
Tags
fix(middleware): reject catch-all compress wildcards "/*" and "*/*" (#… …1156) NewCompressor(level, "/*") passed validation but stored an empty wildcard key that never matched any Content-Type, silently compressing nothing. Instead of turning it into a compress-everything catch-all, reject both "/*" and "*/*" at construction: compressing every response wastes CPU on already-compressed types (zip, jpeg, png), which is why the middleware keeps a curated default list. Users should pass explicit content types.
ci: pin GitHub Actions to full commit SHAs (#1116) Pin unpinned action references to immutable commit SHAs. Version tags retained as inline comments. See: https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions#using-third-party-actions Co-authored-by: KiloClaw Security <security@kiloclaw.ai>
feat: middleware.ClientIP, a replacement for middleware.RealIP (#967) * ClientIP middleware proposal, intended to replace RealIP * ClientIP middleware: address PR feedback and security advisories Rework the new ClientIP* middlewares per reviewer feedback (c2h5oh, adam-p, rezmoss, Saku0512) and align with the documented attack patterns in GHSA-3fxj-6jh8-hvhx, GHSA-rjr7-jggh-pgcp, GHSA-9g5q-2w5x-hmxf. API: - Rename ClientIPFromXFFHeader -> ClientIPFromXFF. - Add ClientIPFromXFFTrustedProxies(n) for dynamic proxy pools where enumerating CIDRs isn't practical (autoscaling, ephemeral containers). - Add GetClientIPAddr alongside GetClientIP; both back to a single netip.Addr stored in context. r.RemoteAddr is never mutated. - Drop IsLoopback/IsPrivate filtering: the user's explicit trust configuration is authoritative (k8s nginx-ingress and similar legitimately surface those values). - Merge multiple X-Forwarded-For header instances before walking (RFC 2616), defeating duplicate-header attacks. - Deprecate middleware.RealIP with citations to all three advisories and guidance pointing at the new API. Docs: - Per-function godoc explains exactly when each variant applies. - Example_clientIP is a single, consolidated decision guide rendered on pkg.go.dev. Tests: 56 subtests including explicit PoC reproductions of each advisory, /24 boundary cases (Saku0512), IPv6, multi-header merging, spoofing prevention, and middleware chaining (rezmoss). Co-authored-by: Cursor <cursoragent@cursor.com> Co-Authored-By: Claude Sonnet 4.7 <noreply@anthropic.com> * Drop first-write-wins semantics, we want users to use exactly one middleware * ClientIPFromXFFTrustedProxies: drop fallback-chaining hint from godoc Saku0512 correctly flagged in #967 that the godoc told users to register ClientIPFromRemoteAddr *after* ClientIPFromXFFTrustedProxies for fallback behavior. With last-write-wins semantics (482e855) that recipe is actively wrong: on the happy path RemoteAddr would silently overwrite the legitimate XFF-derived client IP with the immediate proxy's address — reintroducing the spoofing-prone behavior this API was written to prevent. Rather than just reverse the order in the example, drop the chaining hint entirely. The whole point of 482e855 was to push users toward picking exactly one ClientIPFrom* middleware; documenting a chaining recipe in one specific function works against that. The honest statement is "no client IP is set and GetClientIP returns \"\"" — what the caller does about it is on them. Also rename TestClientIPChaining -> TestClientIPLastWriteWins and reword its subtests to describe the property being locked down rather than endorsing a fallback recipe. Same coverage, no implicit recommendation. * ClientIP: add failing regression tests for v4-mapped / zoned IP bypasses Per PR #967 review, demonstrate three concrete gaps that the rightmost-untrusted XFF algorithm and ClientIPFromRemoteAddr don't catch: 1. v4-mapped IPv6 in XFF escapes a v4 trusted prefix (netip.Prefix.Contains is false for ::ffff:a.b.c.d vs a.b.c.0/N). Internal-address spoof and loopback spoof variants. 2. Zoned IPv6 in XFF escapes a v6 trusted prefix (netip.Prefix.Contains is false for any zoned address). 3. v4-mapped IPv6 RemoteAddr is returned in its v6 form, splitting one logical client across two rate-limit buckets / log keys / prefix checks depending on dual-stack listener configuration. These tests are EXPECTED TO FAIL on this commit; the follow-up commit will land the fixes (Unmap() + zone-strip at parse time). The previously discussed "empties don't shift TrustedProxies slot" property is not a bypass — current behavior is already correct — so it will be added as a regression-pin test alongside the fixes rather than committed here as failing. Co-authored-by: Cursor <cursoragent@cursor.com> * ClientIP: normalize v4-mapped IPv6 and strip header zones; README migration Fixes three gaps surfaced by the PR #967 review (failing tests added in the previous commit now pass): 1. v4-mapped IPv6 in XFF escaping a v4 trusted prefix. netip.Prefix.Contains returns false for ::ffff:a.b.c.d checked against a.b.c.0/N, so an attacker able to inject the v4-mapped form of an internal address could escape the trust list. Fix: a single parseHeaderAddr helper Unmap()s before the prefix check and before storage. Applied uniformly in ClientIPFromHeader, the rightmost-untrusted XFF walk, and the ClientIPFromXFFTrustedProxies slot lookup. 2. Zoned IPv6 in XFF escaping a v6 trusted prefix. netip.Prefix.Contains returns false for any zoned IP, and zone identifiers carry no meaning across the network, so an attacker-injected zone suffix bypassed the trust check. Fix: parseHeaderAddr strips zones via WithZone("") at parse time; zones are by definition not valid in header-sourced IPs. 3. ClientIPFromRemoteAddr returning ::ffff:a.b.c.d for v4 clients on dual-stack listeners, splitting one logical client into two rate-limit / log / ACL keys. Fix: Unmap() before storing. Zone is preserved here because RemoteAddr can legitimately be link-local. Also: - Pre-canonicalize the user-supplied header name in ClientIPFromHeader at construction time, matching the realip.go pattern and saving a per-request canonicalization on the hot path. A package-level xForwardedForHeader constant documents the canonical XFF spelling. - Per-function godoc explicitly notes the v4-mapped folding and zone stripping so downstream callers understand the normalization contract. - New regression-pin tests: empties don't shift the ClientIPFromXFFTrustedProxies slot index; positive normalization tests for v4-mapped and zoned inputs in each entry point. - README: deprecate the RealIP row, add ClientIPFrom* entries, and introduce a "Choosing a ClientIP middleware" section with copy-paste recipes for the common deployments (direct internet, Cloudflare, CloudFront, dynamic-IP proxy pools). Co-authored-by: Cursor <cursoragent@cursor.com> * Improve comments and README * Update middleware.Logger to work with ClientIP * ClientIPFromXFF: zero-allocation lazy right-to-left walk middleware.ClientIPFromXFF sits in the request hot path of any chi server running behind a proxy fleet. Per the #967 review, "each allocation counts" — at high RPS the per-request mergeXFF allocations are small individually but show up in GC pressure across many handlers. The previous implementation built a fully materialized merged XFF slice via mergeXFF, then walked it right-to-left and returned at the first non-trusted entry. In the common case — one trusted hop, the rightmost XFF entry IS the client — we paid to allocate and populate the whole chain only to inspect the last cell. Replace it with a lazy walker that iterates headers right-to-left and, within each header, iterates comma-separated entries right-to-left via strings.LastIndexByte. Substrings alias the original header storage, no copies. The walker returns at the first parseable non-trusted entry without ever materializing a slice. Per-request allocation impact (XFF present, typical "1 header, 3 IPs, 1 trusted hop"): Before: ~4 small allocations - initial mergeXFF backing array - strings.Split's []string per header value - 2 regrowths of the result slice as it fills After: 0 allocations - returns on the first iteration; nothing materialized Worst case (entire chain inside trusted prefixes, walker scans all entries): still 0 allocations. Correctness validated three independent ways: - Algorithm conformance: every requirement of the PR's rightmost- untrusted spec maps line-by-line to the new walker — multi-header merge, empty/whitespace drop, parse-failure skip, trust-prefix skip, normalization via parseHeaderAddr (Unmap + zone strip). - Iteration-order equivalence: walking headers in reverse and each header's entries in reverse via LastIndexByte yields exactly the same sequence as mergeXFF's flat list walked right-to-left. - All existing ClientIPFromXFF tests pass: multi-header merge, rightmost-untrusted spoofing scenarios, CIDR boundary membership, v4-mapped folding, IPv6 zone stripping, security regression pins. - Brute-force equivalence fuzz against the original mergeXFF-based walker over 200k random inputs (valid IPs, garbage, v4-mapped, zoned, whitespace, leading/trailing/double commas, empty headers): zero divergences. mergeXFF is unchanged and still used by ClientIPFromXFFTrustedProxies, which genuinely needs the total count for slot indexing. Co-authored-by: Cursor <cursoragent@cursor.com> * ClientIPFromXFF: add failing tests for fail-closed on unparseable XFF PR #967 review issue #6 (adam-p): a netip.ParseAddr failure mid-chain in the rightmost-untrusted walk currently triggers a silent continue. With that behaviour, a hostile or buggy hop emitting garbage between the client and a trusted proxy is indistinguishable from no entry at all, and the walker happily skips past it to return a more-leftward IP — a spoofable result. Pin the desired fail-closed contract with TestXFF_FailClosedOnUnparseable covering three failing cases: 1. garbage_rightmost_no_prefixes XFF "1.1.1.1, garbage" with no trusted prefixes. Today returns 1.1.1.1 (skip-and-continue); must return "" (fail-closed). 2. garbage_between_client_and_trusted_proxy XFF "203.0.113.7, garbage, 10.0.0.1", trusted 10.0.0.0/8. Today skips the trusted hop and the garbage, returns 203.0.113.7. Must return "" — we cannot tell whether the entry to the left of garbage is the client or another forged hop. 3. garbage_past_trusted_chain Same shape with multiple trusted hops. Identical contract. Plus one negative case that MUST keep passing (proves fail-closed does not over-trigger when the walker returns before reaching garbage): 4. garbage_in_unreachable_left_header Headers ["garbage", "203.0.113.7, 10.0.0.1"], trusted 10.0.0.0/8. Walker returns 203.0.113.7 from the right header; garbage in the left header is never inspected. Already passes today, must still pass after the fix. Also remove the now-obsolete "unparseable_rightmost_skipped" case from TestClientIPFromXFF_NoTrustedPrefixes — it asserts the old skip-and- continue behaviour and no longer fits that table's "rightmost-first" theme. The "weird_with_empties_then_valid_rightmost" comment is updated to call out that empty/whitespace entries are trimmed before parsing and so do not trip fail-closed. Cases 1-3 are EXPECTED TO FAIL on this commit; the follow-up flips rightmostUntrustedXFF from silent-skip to fail-closed. Co-authored-by: Cursor <cursoragent@cursor.com> * ClientIPFromXFF: fail closed on unparseable XFF entries PR #967 review issue #6 (adam-p): the rightmost-untrusted walk previously treated a netip.ParseAddr failure the same as a trusted-prefix match — silently skip and keep walking left. That is unsafe. Rightmost-untrusted only works because we trust ourselves to be able to read every hop to the right of the client. An unparseable hop is indistinguishable from a hostile or missing one. Walking past it can let us "find" a client IP that is actually any prepended value an attacker chose to put further left in the chain — exactly the spoofing shape this middleware was written to prevent. Flip the !ok branch in rightmostUntrustedXFF from continue to return (zero, false). Trusted-prefix still continues, empties/whitespace still skip via trim, fail-closed only fires on actual parse failures. ClientIPFromXFFTrustedProxies and ClientIPFromHeader are unaffected: they are single-value reads and already produce no IP on parse failure by construction. Godoc: - ClientIPFromXFF gets one concise line stating the fail-closed contract (visible to library users). - rightmostUntrustedXFF's internal comment is expanded to explain the security rationale (visible to chi maintainers). The four cases pinned in the previous commit's TestXFF_FailClosedOnUnparseable now pass; full middleware test suite stays green. Co-authored-by: Cursor <cursoragent@cursor.com> * ClientIPFromXFFTrustedProxies: zero-allocation lazy nthFromRightXFF walk After 09ddca9 swapped rightmostUntrustedXFF to a lazy right-to-left walk, mergeXFF was kept only because ClientIPFromXFFTrustedProxies needs positional indexing — xff[len-N] — into the merged chain. The slice was allocated and populated every request, even though only one position in it was ever read. The same right-to-left iteration that powers rightmostUntrustedXFF serves this call site with a different stop condition: count down N non-empty entries from the right, return the Nth. nthFromRightXFF is that helper. ClientIPFromXFFTrustedProxies's handler becomes a single call against it; mergeXFF is deleted. Both XFF middlewares now share one iteration shape (right-to-left lazy walk over comma-separated entries across all header values, with substrings aliasing the original header storage) parameterised by stop condition: "first non-trusted" for rightmostUntrustedXFF, "Nth from right" for nthFromRightXFF. One less helper to reason about; both paths become zero-allocation. Allocation impact for ClientIPFromXFFTrustedProxies (per request with XFF present, typical "3 IPs, N=2"): Before: ~4 small allocations - initial mergeXFF backing array - strings.Split's []string per header value - 2 regrowths of the result slice After: 0 allocations - returns at the Nth non-empty entry from the right; nothing materialized Security properties preserved end-to-end — all 12 subtests of TestClientIPFromXFFTrustedProxies pass unchanged, including the trickiest pin (empties_dont_shift_slot) which is exactly the property "do not count empty entries toward N" that nthFromRightXFF's drop-then- decrement loop guarantees by construction. Co-authored-by: Cursor <cursoragent@cursor.com> * ClientIPFromXFF: unify both XFF walkers behind a single walkXFF primitive After the prior two commits, both XFF middlewares had near-identical right-to-left scan loops differing only in stop condition (first non-trusted entry vs Nth non-empty entry from the right). nthFromRightXFF and rightmostUntrustedXFF together carried ~30 lines of duplicated loop boilerplate. Collapse to one primitive: walkXFF(headers []string, visit func(entry string) bool) iterates the merged X-Forwarded-For chain right-to-left and calls visit on each trimmed non-empty entry; visit returns true to stop the walk. Both middleware handlers now share this primitive and supply their own stop condition as a small inline visitor closure (~7 lines each). Net: -21 lines in middleware/client_ip.go, one fewer concept to reason about, single source of truth for the (security-critical) walk order and the empty-entry drop rule. Zero-allocation property is preserved -- benchmarked side-by-side against the previous two-helpers implementation: BenchmarkRightmost_TwoHelpers 36 ns/op 0 B/op 0 allocs/op BenchmarkRightmost_Callback 37 ns/op 0 B/op 0 allocs/op BenchmarkNth_TwoHelpers 11 ns/op 0 B/op 0 allocs/op BenchmarkNth_Callback 13 ns/op 0 B/op 0 allocs/op Go's escape analysis keeps the visitor closures (and their captured &found / &entry / &n locals) on the stack; only cost is one indirect call per visited entry (~1-2 ns). All tests still pass (advisory pins, fail-closed pins, multi-header merge, normalization, boundary CIDR, last-write-wins, panic conditions). Stale comments in client_ip_test.go that referenced the removed helpers by name are updated; test names/comments uniformly use "position" (matching the godoc) for the algorithmic concept and "entry" for an individual chain value. Co-authored-by: Cursor <cursoragent@cursor.com> * client_ip_test: scrub stale "EXPECTED TO FAIL" notes on regression pins Four test godocs still said "this test is EXPECTED TO FAIL until X lands" even though X landed in a follow-up commit on this branch. Misleading to future readers (Copilot PR review flagged 3 of 4). The tests themselves are unchanged and still pass; they are now regression pins for behavior the PR already enforces. Reword the relevant paragraphs from future-tense ("the expected post-fix behavior is...") to present-tense ("pinned post-fix behavior: parseHeaderAddr does X") so the doc matches what the test actually pins. Affected tests: - TestXFF_V4MappedIPv6BypassesTrustedV4Prefix - TestXFF_IPv6ZoneIDBypassesTrustedV6Prefix - TestClientIPFromRemoteAddr_V4MappedIsUnmapped - TestXFF_FailClosedOnUnparseable Co-authored-by: Cursor <cursoragent@cursor.com> * fix(review): minor README fix * ClientIPFromHeader: add failing tests for multi-value header attack PR #967 Copilot review: ClientIPFromHeader uses r.Header.Get(header) to read the trusted single-IP header. Get returns only the FIRST value Go saw on the wire. If the trusted header reaches us with two values (a client-supplied "spoofed" plus a proxy-added "real"), we surface the client's value -- exactly the bypass shape this middleware was written to prevent. The trusted hop is the one CLOSEST to us in the chain, which is the LAST value, not the first. Same rightmost-untrusted spirit as ClientIPFromXFF: values from hops further from us are by definition less trustworthy than what our nearest hop produced. So the post-fix contract is: - Read r.Header.Values(header) (all instances). - Take the last entry. - parseHeaderAddr it; if it doesn't parse (garbage, empty), no IP is set -- do NOT fall back to earlier values (those came from less-trusted hops). TestClientIPFromHeader_MultiValueLastWins pins five cases: 1. single_value_unchanged (regression pin) 2. attacker_then_proxy -- today returns attacker's value 3. three_values_last_wins -- today returns attacker's value 4. last_unparseable_no_fallback -- today falls back to earlier value 5. last_empty_no_fallback -- today falls back to earlier value Cases 2-5 are EXPECTED TO FAIL on this commit; the follow-up flips ClientIPFromHeader from Get to Values()[len-1]. Co-authored-by: Cursor <cursoragent@cursor.com> * ClientIPFromHeader: take last value, not first (defense-in-depth) PR #967 Copilot review: the previous implementation read the trusted single-IP header with r.Header.Get(header), which returns only the first value. If the trusted header reaches us with multiple values -- a client-supplied "spoofed" entry plus the proxy's real entry, or two proxy-set entries because of upstream chain quirks -- we surfaced the attacker-controlled first value. Switch to r.Header.Values(header) and take the LAST entry. The last value is the one added by the hop closest to us in the chain, which is by definition the most trusted (the rightmost-untrusted principle we already apply to X-Forwarded-For). Fail-closed on garbage at the last position: we do NOT fall back to earlier values, as those came from less-trusted hops further from us. Correctly configured proxies (single value, proxy overwrites instead of appends) are unaffected -- len(values)==1, so first == last == that value. The four cases pinned in the previous commit's TestClientIPFromHeader_MultiValueLastWins now pass: - attacker_then_proxy (last value wins) - three_values_last_wins (last value wins) - last_unparseable_no_fallback (fail-closed) - last_empty_no_fallback (fail-closed) Existing ClientIPFromHeader tests stay green. Godoc updated with one short paragraph naming the multi-value contract. Co-authored-by: Cursor <cursoragent@cursor.com> * client_ip: add walkXFF benchmark as O(M) regression pin PR #967 Copilot review claimed walkXFF is O(n^2) in the number of XFF entries because it calls strings.LastIndexByte on progressively shorter substrings, and suggested a DoS-prone CPU cost on attacker-supplied large XFF headers. Benchmark refutes the claim. walkXFF is strictly O(M) in total chain length M -- linear in the entry count n, near-constant for the single-trusted-hop case (visitor returns true on the first entry). BenchmarkWalkXFF (visitor walks every entry, worst case for ClientIPFromXFF when all entries are inside trusted prefixes): n=1 5.6 ns/op n=10 57.6 ns/op (5.8 ns/entry) n=100 541.1 ns/op (5.4 ns/entry) n=1000 5228 ns/op (5.2 ns/entry) n=10000 55148 ns/op (5.5 ns/entry) BenchmarkWalkXFF_RightmostStop (visitor stops at the first entry, the common case for ClientIPFromXFF with no trusted prefixes and ClientIPFromXFFTrustedProxies(1)): n=1 5.2 ns/op n=10 6.0 ns/op n=100 6.1 ns/op n=1000 6.1 ns/op n=10000 6.1 ns/op -- truly constant Why the analysis is O(M): strings.LastIndexByte scans BACKWARD from the end of its input and stops at the first match. After we slice off the rightmost entry, the next call only scans up to the next comma -- O(L) where L is that entry's length. Sum over n iterations is O(n*L) = O(M). On arm64/amd64 it's also SIMD-optimized in the Go runtime, so a hand-rolled byte loop would lose, not win. Benchmark stays in the codebase as a regression pin: any future refactor that accidentally drops back to O(n^2) gets caught here. Zero allocations preserved across both shapes. Co-authored-by: Cursor <cursoragent@cursor.com> * ClientIPFromXFF: doc fix -- no-args case is fail-closed, not skip-and-continue PR #967 Copilot review: the godoc said "Calling with no arguments returns the rightmost parseable XFF IP", implying skip-and-continue past garbage. That phrasing predates the fail-closed change in 5dd2243; the actual behavior is now "try the rightmost; if it doesn't parse, no IP is set". Update the line to match. Co-authored-by: Cursor <cursoragent@cursor.com> * ClientIPFromXFF*: skip Header.Values canonicalization on the hot path PR #967 fresh review: r.Header.Values(key) re-canonicalizes the key through textproto.CanonicalMIMEHeaderKey on every call. xForwardedForHeader is already a const in canonical form, so the canonicalization is redundant work in the request hot path. Replace r.Header.Values(xForwardedForHeader) with the direct map read r.Header[xForwardedForHeader] in both XFF middleware handlers. Safe because: - Headers received by net/http are stored under canonical keys. - Headers set programmatically via r.Header.Set/Add are also stored under canonical keys. - r.Header[non-canonical-key] only misses if user code bypasses the canonicalization via direct map writes; that's a test-only foot-gun and not the regime this middleware runs in. Per-lookup cost measured side by side on an M4 Pro: Header.Values 16.0 ns/op 0 B/op 0 allocs/op DirectMap 4.3 ns/op 0 B/op 0 allocs/op ~12 ns saved per request per XFF middleware on the hot path. Test suite (~70 tests) stays green; walkXFF semantics unchanged. Co-authored-by: Cursor <cursoragent@cursor.com> * Example_clientIP: mention fail-closed in the XFF comment PR #967 fresh review: the inline comment summarizing the XFF middleware said it "walks right-to-left, skipping trusted entries" but stopped short of mentioning the fail-closed-on-garbage contract that landed in 5dd2243. Add three words to match the godoc. Co-authored-by: Cursor <cursoragent@cursor.com> * TestClientIPFromHeader_MultiValueLastWins: regression-pin wording PR #967 Copilot review: the godoc on this test still described the pre-fix behavior in present tense ("Today we surface values[0]"), even though a8e3dbf already switched the implementation to read r.Header.Values() and use the last entry. Reword the godoc to frame the test as a regression pin for the gap fixed in a8e3dbf -- past-tense bug description, present-tense post-fix contract. Same body, same test cases, same assertions. Co-authored-by: Cursor <cursoragent@cursor.com> * BenchmarkWalkXFF: rename stale rightmostUntrustedXFF reference PR #967 Copilot review: the BenchmarkWalkXFF godoc referenced rightmostUntrustedXFF, an internal helper that was consolidated into the single walkXFF primitive back in 1863339 and no longer exists in the package. Rename the reference to ClientIPFromXFF (the public caller whose worst case this benchmark actually exercises). No code change. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Claude Sonnet 4.7 <noreply@anthropic.com>
fix(middleware): add missing return in RouteHeaders empty check (#1045) The RouteHeaders middleware was missing a return statement after calling next.ServeHTTP when the router had no routes configured. This caused the next handler to be called twice - once in the empty check and again at the end of the function. Also adds comprehensive test coverage for the RouteHeaders middleware and Pattern matching functionality.
middleware: add Discard method to WrapResponseWriter (#926) * middleware: add Discard method to WrapResponseWriter * resolve review comments * use ioutil.Discard and deprecate the public interface * move the Discard method back to the public interface * discard calls to WriteHeader too
PreviousNext