🐛 fix(proxy): guard runtime helpers against DNS-rebinding

[gaby]

Description

middleware/proxy's SecurityPolicy (default AllowPrivateIPs: false) is documented as blocking SSRF against loopback, RFC 1918, link-local, CGNAT, and cloud-metadata addresses. In practice that guarantee only held for Balancer, which installs a dial-time guard (newSSRFDialer) that re-validates the resolved IP atomically with the connection.

Every other public entry point — Do, DoRedirects, DoTimeout, DoDeadline, Forward, DomainForward, BalancerForward — validated the host only once, up front, then dispatched through a shared/user-supplied *fasthttp.Client whose own dialer re-resolved the hostname independently, with no validation. A rebinding resolver that answers with a public IP on the first lookup and a private IP on the second passed the up-front check and then connected to the private address anyway (classic DNS rebinding / check-use gap).

This PR closes that gap for all runtime helpers by giving them the same dial-time re-validation Balancer already has, so AllowPrivateIPs=false genuinely blocks rebinding for every entry point.

Fixes the DNS-rebinding SSRF in proxy.Do/Forward/DoRedirects/DoTimeout/DoDeadline/DomainForward/BalancerForward (☢️ Bug).

How the guard is installed

The guard rides on fasthttp's Client.ConfigureClient hook, which runs once per HostClient at creation — so it is present before the first dial to each host and, crucially, covers both dial code paths (callDialFunc prefers DialTimeout over Dial, so guarding only Dial would leave a DialTimeout-configured client unvalidated).

• installHostClientGuard wraps both hc.DialTimeout (preserving its per-request timeout) and hc.Dial.
• guardedDial (shared core) + newGuardedClientDialer / newGuardedClientDialerWithTimeout: when AllowPrivateIPs is true the guard delegates unchanged; otherwise it resolves the host, rejects the whole resolution on any blocked IP, and dials the exact validated IP (through the caller's dialer when set). Policy is read fresh on every dial, since a client outlives any single policy snapshot. Reuses resolveAndValidateHost and dialValidatedIPs.
• ensureClientGuarded installs the hook idempotently, serialized under a mutex so the guard is in place before any concurrent guarder observes the client. It composes with a user's existing ConfigureClient (running the user hook first, then the guard, so a hook that sets its own hc.Dial cannot overwrite the guard). Wired into init (default client) and WithClient before first use, and on the dispatch path only for a per-call variadic client (the global client is already guarded, so the common request path stays lock-free). Clients with a nil ConfigureClient are matched by identity and never retained.

Because a pooled keep-alive connection is always bound to an already-validated IP, connection reuse cannot be rebound; only a fresh dial reaches a new IP, and every fresh dial runs the guard. The only path left to the caller is a custom Balancer Config.Client (*fasthttp.LBClient), whose underlying dialers are not reachable for wrapping — documented on SecurityPolicy.AllowPrivateIPs.

As defense-in-depth, isBlockedIP now also unwraps IPv4-compatible (::a.b.c.d) and NAT64 (64:ff9b::a.b.c.d) IPv6 literals to the embedded IPv4 and applies the same blocklist (the IPv4-mapped ::ffff: form was already covered), so a private target can't be smuggled past as an IPv6 literal.

Changes introduced

• middleware/proxy/security.go: installHostClientGuard, guardedDial, newGuardedClientDialer, newGuardedClientDialerWithTimeout; harden isBlockedIP with embeddedIPv4/allZero; update SecurityPolicy.AllowPrivateIPs docs.
• middleware/proxy/proxy.go: guardConfigureClient/ensureClientGuarded plumbing (mutex-serialized, identity-idempotent), wired into init/WithClient/doActionWithPolicy; refresh the per-helper SSRF doc comments and the DomainForward construction-validation note.
• Tests (security_rebinding_test.go, security_test.go):
  • DNS-rebinding regression via an in-process fake resolver (public answer first, loopback after) asserting the internal server is never reached — for the default client, a per-call custom client, a client configured with DialTimeout, and DoRedirects; plus a DNS A-query counter asserting the block happens at dial time, not up front.
  • Unit tests for the dialers (block loopback, delegate when private allowed, compose with a caller dialer, validated-hostname dials through to the resolved IP), ConfigureClient compose/ordering + idempotency, a -race concurrency regression test, and extended isBlockedIP coverage (IPv4-mapped / IPv4-compatible / NAT64, private blocked, public allowed).
• Benchmarks: none added. The common no-variadic request path takes no new lock; the guard adds one resolve + validated dial per new connection when AllowPrivateIPs=false (mirrors the existing Balancer guard).
• Documentation Update: doc comments in the changed files only; no /docs/ changes.
• Changelog/What's New: security fix — SSRF/DNS-rebinding protection now applies to all proxy runtime helpers, not just Balancer.
• Migration Guide: none — behavior only changes for previously-unprotected SSRF cases under the secure default.
• API Alignment with Express: n/a.
• API Longevity: no public API/signature changes; the guard is internal.
• Examples: n/a.

Type of change

• Code consistency (non-breaking change which improves code reliability and robustness) — security bug fix, no public API change.

Checklist

• Conducted a self-review of the code and provided comments for complex or critical parts.
• Added or updated unit tests to validate the effectiveness of the changes.
• Ensured that new and existing unit tests pass locally (go test -race ./middleware/proxy/..., go vet, gofmt).
• No new dependencies (golang.org/x/net/dns/dnsmessage, used only in tests, was already a direct module dependency).
• Updated the documentation in /docs/ (not applicable — no user-facing docs affected).
• Provided benchmarks for the new code.

🤖 Generated with Claude Code

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Dial-time SSRF protection now guards shared and per-call fasthttp clients, validates host resolutions during connection establishment, handles wrapped IPv4 forms, and adds DNS-rebinding, composition, concurrency, and policy regression tests.

Changes

SSRF and DNS-rebinding protection

Layer / File(s)	Summary
Policy-aware dial validation middleware/proxy/security.go	Host-client dialing revalidates resolved addresses under strict policy, preserves custom dialers, and unwraps IPv4-compatible and NAT64 addresses for blocklist checks.
Client guard installation and dispatch middleware/proxy/proxy.go	Global and per-call fasthttp clients receive idempotent guards, while proxy dispatch uses dial-time validation for upstream requests and redirects.
Rebinding and guard regression coverage middleware/proxy/security_rebinding_test.go, middleware/proxy/security_test.go	Tests cover DNS rebinding, guard composition, concurrency, dial variants, private-IP policy behavior, and wrapped-IP blocklist cases.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant ProxyDispatch
  participant FasthttpClient
  participant HostClient
  participant Resolver
  ProxyDispatch->>FasthttpClient: Dispatch upstream request
  FasthttpClient->>HostClient: Open connection
  HostClient->>Resolver: Resolve hostname at dial time
  Resolver-->>HostClient: Return current IP addresses
  HostClient->>HostClient: Validate resolved IP
  HostClient-->>FasthttpClient: Block or dial validated IP
  FasthttpClient-->>ProxyDispatch: Return response or error

Loading

Possibly related PRs

• gofiber/fiber#4405: Both changes address redirect downgrade handling in proxy redirect flows, with this PR also adding dial-time SSRF and DNS-rebinding protection.

Suggested reviewers: sixcolors, efectn, ReneWerner87

Poem

I’m a rabbit guarding every dial,
Rebinding tricks now hit a wall.
Public paths may safely flow,
Private hops are stopped below.
With ears up high and tests in flight,
I burrow through the DNS night.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is concise and accurately summarizes the main change: adding DNS-rebinding guards to proxy runtime helpers.
Description check	✅ Passed	The description covers the problem, solution, changes, type of change, and checklist sections required by the template.
Docstring Coverage	✅ Passed	Docstring coverage is 81.82% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/dns-rebinding-ssrf-proxy-cfb0ko

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.2)

level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[codecov]

Codecov Report

❌ Patch coverage is 98.61111% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 93.22%. Comparing base (a5a8690) to head (4cfd4da).
⚠️ Report is 11 commits behind head on main.

Files with missing lines	Patch %	Lines
middleware/proxy/security.go	98.18%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4518      +/-   ##
==========================================
+ Coverage   93.13%   93.22%   +0.09%     
==========================================
  Files         140      140              
  Lines       14275    14421     +146     
==========================================
+ Hits        13295    13444     +149     
+ Misses        611      608       -3     
  Partials      369      369              

Flag	Coverage Δ
unittests	93.22% <98.61%> (+0.09%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Copilot]

Pull request overview

This PR hardens middleware/proxy against DNS-rebinding SSRF by ensuring the runtime helper paths (e.g., Do, Forward, DoRedirects) re-validate resolved IPs at dial time (not only at initial validation), aligning their security guarantees with the Balancer path.

Changes:

• Add a policy-aware, dial-time SSRF guard that wraps fasthttp.HostClient dialing (Dial and DialTimeout) and install it via fasthttp.Client.ConfigureClient, with idempotent client guarding.
• Extend the upstream IP blocklist to also catch IPv4-compatible and NAT64 IPv6 wrapper literals by unwrapping embedded IPv4s before evaluation.
• Add regression/unit tests covering DNS rebinding behavior and guarded dialer composition (plus expanded isBlockedIP cases).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File	Description
middleware/proxy/security.go	Implements dial-time guarded dialing helpers and enhances blocked-IP detection for IPv6 wrapper forms.
middleware/proxy/proxy.go	Ensures all runtime-helper-dispatched *fasthttp.Clients get guarded via ConfigureClient, including WithClient and per-call overrides.
middleware/proxy/security_test.go	Expands isBlockedIP test coverage for IPv4-mapped, IPv4-compatible, and NAT64 literals.
middleware/proxy/security_rebinding_test.go	Adds end-to-end rebinding regression and unit tests for the new guarded client dialer and guard installation semantics.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (3)

middleware/proxy/security_rebinding_test.go (3)

347-355: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Simplify goroutine spawning with WaitGroup.Go (modernize/revive).

♻️ Proposed fix

 			var wg sync.WaitGroup
-			for range 32 {
-				wg.Add(1)
-				go func() {
-					defer wg.Done()
-					ensureClientGuarded(cli)
-				}()
-			}
+			for range 32 {
+				wg.Go(func() {
+					ensureClientGuarded(cli)
+				})
+			}
 			wg.Wait()

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@middleware/proxy/security_rebinding_test.go` around lines 347 - 355, Replace
the manual wg.Add(1), goroutine wrapper, and defer wg.Done() around
ensureClientGuarded(cli) with the WaitGroup.Go helper, while preserving the loop
count and waiting behavior with wg.Wait().

Source: Linters/SAST tools

---

163-170: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Replace the DialTimeout wrapper lambda with a direct reference (unlambda).

fasthttp.DialTimeout already matches the DialTimeout field signature, so the wrapping closure is flagged by lint here and again at Line 374.

♻️ Proposed fix

 			cli := &fasthttp.Client{
-				DialTimeout: func(addr string, timeout time.Duration) (net.Conn, error) {
-					return fasthttp.DialTimeout(addr, timeout)
-				},
+				DialTimeout: fasthttp.DialTimeout,
 			}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@middleware/proxy/security_rebinding_test.go` around lines 163 - 170, Replace
the redundant DialTimeout closure in the “custom client with DialTimeout” test
case with a direct fasthttp.DialTimeout function reference, and apply the same
unlambda change to the corresponding occurrence near the other flagged location.

Source: Linters/SAST tools

---

93-104: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

buildDNSResponse lint failures: hugeParam (Line 93) and unwrapped errors (Lines 101, 104).

q is a 260-byte value; passing by pointer avoids the copy. The StartQuestions()/Question(q) returns are also flagged by wrapcheck. Consider passing q by pointer and wrapping (or //nolint-ing) the builder errors to clear the lint job.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@middleware/proxy/security_rebinding_test.go` around lines 93 - 104, Update
buildDNSResponse to accept *dnsmessage.Question and pass the pointer through to
the builder, updating callers as needed; wrap the errors returned by
StartQuestions and Question with contextual messages (or add narrowly scoped
nolint directives) to satisfy hugeParam and wrapcheck linting.

Source: Linters/SAST tools

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@middleware/proxy/security_rebinding_test.go`:
- Line 35: Add justified //nolint:errcheck comments with reasons to the
pc.Close(), pc.WriteTo(...), and ln.Close() calls in the relevant test cleanup
and write paths, preserving the existing behavior while satisfying errcheck.

---

Nitpick comments:
In `@middleware/proxy/security_rebinding_test.go`:
- Around line 347-355: Replace the manual wg.Add(1), goroutine wrapper, and
defer wg.Done() around ensureClientGuarded(cli) with the WaitGroup.Go helper,
while preserving the loop count and waiting behavior with wg.Wait().
- Around line 163-170: Replace the redundant DialTimeout closure in the “custom
client with DialTimeout” test case with a direct fasthttp.DialTimeout function
reference, and apply the same unlambda change to the corresponding occurrence
near the other flagged location.
- Around line 93-104: Update buildDNSResponse to accept *dnsmessage.Question and
pass the pointer through to the builder, updating callers as needed; wrap the
errors returned by StartQuestions and Question with contextual messages (or add
narrowly scoped nolint directives) to satisfy hugeParam and wrapcheck linting.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 831deb5d-f29d-44a9-b247-9f5e2321fd08

📥 Commits

Reviewing files that changed from the base of the PR and between c6f4afb and e32dc6f.

📒 Files selected for processing (4)

• middleware/proxy/proxy.go
• middleware/proxy/security.go
• middleware/proxy/security_rebinding_test.go
• middleware/proxy/security_test.go

[github-actions]

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.

Benchmark suite	Current: 4cfd4da	Previous: f280569	Ratio
BenchmarkStripHopByHop_NoConnection (github.com/gofiber/fiber/v3/middleware/proxy)	1474 ns/op 0 B/op 0 allocs/op	929.7 ns/op 0 B/op 0 allocs/op	1.59
BenchmarkStripHopByHop_NoConnection (github.com/gofiber/fiber/v3/middleware/proxy) - ns/op	1474 ns/op	929.7 ns/op	1.59

This comment was automatically generated by workflow using github-action-benchmark.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

[coderabbitai]

🧹 Nitpick comments (1)

middleware/proxy/proxy.go (1)

178-213: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Avoid using reflect.Value.Pointer() as the idempotency key — compare against an explicit sentinel on guardedConfigureClient instead; Go doesn’t guarantee func-pointer identity for bound method values, so this check can misclassify an already-guarded client.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@middleware/proxy/proxy.go` around lines 178 - 213, Replace the
reflect.Value.Pointer-based check in ensureClientGuarded with an explicit
sentinel identifying guardedConfigureClient hooks. Update guardedConfigureClient
and its run hook so the sentinel is preserved or detectable for composed hooks,
then use that sentinel to return early when the client is already guarded;
remove the guardHookPtr dependency.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@middleware/proxy/proxy.go`:
- Around line 178-213: Replace the reflect.Value.Pointer-based check in
ensureClientGuarded with an explicit sentinel identifying guardedConfigureClient
hooks. Update guardedConfigureClient and its run hook so the sentinel is
preserved or detectable for composed hooks, then use that sentinel to return
early when the client is already guarded; remove the guardHookPtr dependency.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: f0d64a9d-7704-4890-9358-17c6e8db0f27

📥 Commits

Reviewing files that changed from the base of the PR and between 3ab95c8 and 4cfd4da.

📒 Files selected for processing (2)

• middleware/proxy/proxy.go
• middleware/proxy/security.go

🚧 Files skipped from review as they are similar to previous changes (1)

• middleware/proxy/security.go

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.