🔒 security: harden proxy middleware#4405
Conversation
|
Need the big picture first? Review this PR in Change Stack to see what changed before going file by file. WalkthroughThis PR adds client-side HTTPS→HTTP redirect downgrade rejection and implements a proxy SecurityPolicy with SSRF protections, hop-by-hop header stripping, TLS1.2 enforcement, safe upstream path joining, config fields for policy and response-size limits, extensive tests, fuzzers, and documentation/audit updates. ChangesSecurity Hardening for Proxy and Client Redirects
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
gaby
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
| currentURL := string(req.URI().FullURI()) | ||
| for redirects := 0; ; redirects++ { | ||
| req.SetRequestURI(currentURL) |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a comprehensive security hardening policy for the proxy middleware and HTTP client, introducing SSRF protection (blocking private/loopback IPs), scheme validation, HTTPS-to-HTTP redirect downgrade prevention, RFC 7230 hop-by-hop header stripping, and path sanitization to prevent network-path reference injection. The review feedback highlights several critical security and robustness improvements: addressing a potential TOCTOU DNS rebinding vulnerability in upstream validation, enforcing a timeout on DNS lookups to prevent DoS, stripping sensitive headers during cross-origin redirects, resolving a bug in path joining that strips base URL path prefixes, handling startup DNS failures gracefully instead of panicking, and replacing a custom string comparison helper with the standard library's bytes.EqualFold.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // validateUpstream parses raw, enforces the scheme allowlist, and unless | ||
| // the policy permits private addresses, resolves the hostname and | ||
| // rejects responses that include any blocked address. Rejecting on a | ||
| // single blocked answer mitigates DNS rebinding attempts in which the | ||
| // resolver returns a mix of public and private IPs. | ||
| func validateUpstream(raw string, policy SecurityPolicy) (*url.URL, error) { |
There was a problem hiding this comment.
The SSRF protection is vulnerable to a Time-of-Check to Time-of-Use (TOCTOU) DNS Rebinding attack. While validateUpstream resolves the hostname to validate the IP, the actual HTTP request made by cli.Do(req, res) will perform its own DNS resolution. A malicious DNS server can return a safe public IP during the validation check and then return a blocked private IP (e.g., 127.0.0.1) when the client connects.
To fully mitigate DNS rebinding, consider pinning the resolved IP address in the request URL (while preserving the original host for SNI and the Host header), or validating the IP address within a custom Dial function of the fasthttp.Client.
| } | ||
| return nil | ||
| } | ||
| addrs, err := net.LookupIP(host) |
There was a problem hiding this comment.
Calling net.LookupIP(host) without a timeout or context can cause the request to hang indefinitely (or up to the OS default timeout of 30 seconds) if the DNS server is slow or unresponsive. This can easily lead to Denial of Service (DoS) and resource exhaustion under load.
Use context.WithTimeout and net.DefaultResolver.LookupIP to enforce a reasonable timeout (e.g., 2 seconds) for DNS resolution. Note that you will need to add "context" to the imports of this file.
ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
defer cancel()
addrs, err := net.DefaultResolver.LookupIP(ctx, "ip", host)| // followRedirects implements a redirect loop that re-validates each | ||
| // target against policy before issuing the next request. It replaces | ||
| // fasthttp.Client.DoRedirects so we can reject HTTPS→HTTP downgrades and | ||
| // reapply SSRF checks to caller-controlled Location headers. | ||
| func followRedirects(cli *fasthttp.Client, req *fasthttp.Request, resp *fasthttp.Response, maxRedirects int, policy SecurityPolicy) error { |
There was a problem hiding this comment.
When following redirects, followRedirects does not strip sensitive headers like Authorization or Cookie when redirecting to a different host/origin. This can leak credentials to untrusted third-party upstreams.
Compare the host of currentURL and nextURL. If they differ, strip sensitive headers from req before making the next request.
| if parsed.Path != "" { | ||
| out.Path = parsed.Path | ||
| out.RawPath = parsed.RawPath | ||
| } |
There was a problem hiding this comment.
If the configured upstream base URL contains a path prefix (e.g., http://upstream.example/api), joinUpstreamPath will completely overwrite out.Path with parsed.Path (e.g., /foo), silently stripping the /api prefix. This breaks proxying to sub-paths.
To preserve the base path, join base.Path and parsed.Path correctly. Note that the fuzz test FuzzJoinUpstreamPath did not catch this because it used a base URL without any path prefix (http://upstream.invalid).
| if parsed.Path != "" { | |
| out.Path = parsed.Path | |
| out.RawPath = parsed.RawPath | |
| } | |
| if parsed.Path != "" { | |
| out.Path = strings.TrimSuffix(base.Path, "/") + "/" + strings.TrimPrefix(parsed.Path, "/") | |
| if parsed.RawPath != "" { | |
| out.RawPath = strings.TrimSuffix(base.RawPath, "/") + "/" + strings.TrimPrefix(parsed.RawPath, "/") | |
| } | |
| } |
| for _, server := range cfg.Servers { | ||
| if !strings.HasPrefix(server, "http") { | ||
| server = "http://" + server | ||
| } | ||
|
|
||
| u, err := url.Parse(server) | ||
| u, err := validateUpstream(server, policy) | ||
| if err != nil { | ||
| panic(err) | ||
| } |
There was a problem hiding this comment.
Performing live DNS lookups during Balancer initialization means any transient DNS failure or network lag at application startup will cause the application to panic and crash. In containerized environments (like Kubernetes) where services start in parallel, this can lead to crash loops. Consider handling DNS lookup failures gracefully at startup (e.g., logging a warning and retrying, or deferring IP validation to request time) rather than panicking immediately.
| if err != nil { | ||
| return "", err | ||
| } | ||
| if !policy.AllowHTTPSDowngrade && equalFoldASCII(previousScheme, []byte("https")) && target.Scheme == "http" { |
There was a problem hiding this comment.
The custom equalFoldASCII function can be replaced with the standard library's highly optimized bytes.EqualFold function, which is already used in the client package. This improves maintainability and performance. You can also delete the equalFoldASCII helper function entirely.
| if !policy.AllowHTTPSDowngrade && equalFoldASCII(previousScheme, []byte("https")) && target.Scheme == "http" { | |
| if !policy.AllowHTTPSDowngrade && bytes.EqualFold(previousScheme, []byte("https")) && target.Scheme == "http" { |
gaby
force-pushed
the
claude/proxy-middleware-security-audit-q45Ya
branch
from
95cacf8 to
9f6f1e6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
middleware/proxy/proxy.go (1)
74-113:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRun hop-by-hop stripping after the modifier hooks too.
Line 74 strips the outbound headers before
ModifyRequest, and Line 100 strips the inbound headers beforeModifyResponse. That lets either hook addConnection,Proxy-Authorization,Transfer-Encoding, or aConnection-listed custom header back after the security filter has already run.Suggested fix
- if !policy.KeepHopByHopHeaders { - if cfg.KeepConnectionHeader { - stripHopByHopRequestHeaders(req, fiber.HeaderConnection) - } else { - stripHopByHopRequestHeaders(req) - } - } - // Modify request if cfg.ModifyRequest != nil { if err := cfg.ModifyRequest(c); err != nil { return err } } + + if !policy.KeepHopByHopHeaders { + if cfg.KeepConnectionHeader { + stripHopByHopRequestHeaders(req, fiber.HeaderConnection) + } else { + stripHopByHopRequestHeaders(req) + } + } @@ - if !policy.KeepHopByHopHeaders { - if cfg.KeepConnectionHeader { - stripHopByHopResponseHeaders(res, fiber.HeaderConnection) - } else { - stripHopByHopResponseHeaders(res) - } - } - // Modify response if cfg.ModifyResponse != nil { if err := cfg.ModifyResponse(c); err != nil { return err } } + + if !policy.KeepHopByHopHeaders { + if cfg.KeepConnectionHeader { + stripHopByHopResponseHeaders(res, fiber.HeaderConnection) + } else { + stripHopByHopResponseHeaders(res) + } + }🤖 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 74 - 113, The hop-by-hop header stripping is currently done before cfg.ModifyRequest and before cfg.ModifyResponse, which allows modifier hooks to reintroduce forbidden headers; move the stripHopByHopRequestHeaders call to immediately after cfg.ModifyRequest (but before setting the RequestURI and lbc.Do) and move stripHopByHopResponseHeaders to immediately after cfg.ModifyResponse (and before returning the response), preserving the existing conditionals using policy.KeepHopByHopHeaders and cfg.KeepConnectionHeader so the same header lists are used when you call stripHopByHopRequestHeaders and stripHopByHopResponseHeaders.
🧹 Nitpick comments (1)
docs/middleware/proxy.md (1)
54-54: 💤 Low valueOptional: Consider simplifying "by accident" to "accidentally".
The phrase "cannot be negotiated by accident" could be more concise as "cannot be negotiated accidentally" or "are not negotiated by default".
✏️ Suggested simplification
-`Config.TLSConfig` is cloned with `MinVersion: tls.VersionTLS12` if no minimum is configured, so deprecated TLS versions cannot be negotiated by accident. +`Config.TLSConfig` is cloned with `MinVersion: tls.VersionTLS12` if no minimum is configured, so deprecated TLS versions cannot be negotiated accidentally.🤖 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 `@docs/middleware/proxy.md` at line 54, Change the wording in the sentence that mentions `Config.TLSConfig` and `MinVersion: tls.VersionTLS12` to use "accidentally" (e.g., "cannot be negotiated accidentally") or "are not negotiated by default" for conciseness; update the phrase that currently reads "cannot be negotiated by accident" to one of these alternatives around the `MinVersion: tls.VersionTLS12` description.
🤖 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/fuzz_test.go`:
- Around line 94-96: Update the failing test message to fix the cspell typo:
replace "unparseable" with "unparsable" in the t.Fatalf call that currently
reads t.Fatalf("unparseable output %q for input %q: %v", out, requestPath, err)
(refer to the t.Fatalf invocation and the variables out, requestPath, err in
fuzz_test.go) so the CI spell-check passes.
In `@middleware/proxy/security_test.go`:
- Around line 318-320: The test currently reads the response body with a single
call to resp.Body.Read (variable body and n) which can return a partial payload
and cause flaky require.Contains assertions; replace that partial read with a
full read using io.ReadAll (or ioutil.ReadAll) on resp.Body, check the error
with require.NoError, close the body, and then assert require.Contains(t,
string(fullBody), "HTTPS to HTTP redirect blocked") so the assertion always sees
the complete response.
In `@middleware/proxy/security.go`:
- Around line 91-99: When normalizing and installing a SecurityPolicy in
WithSecurityPolicy (and the similar helper later), ensure you deep-copy the
AllowedSchemes slice instead of reusing the backing array: if
len(policy.AllowedSchemes)==0 and you assign
DefaultSecurityPolicy().AllowedSchemes or when you assign policy into
activePolicy, replace the slice with a freshly allocated copy (e.g. copy into
append([]string(nil), ...)) so callers cannot mutate the global
activePolicy/resolvePolicy/balancer view via a shared backing array; do this for
every place that assigns or returns a SecurityPolicy with AllowedSchemes to
guarantee immutability outside policyLock.
---
Outside diff comments:
In `@middleware/proxy/proxy.go`:
- Around line 74-113: The hop-by-hop header stripping is currently done before
cfg.ModifyRequest and before cfg.ModifyResponse, which allows modifier hooks to
reintroduce forbidden headers; move the stripHopByHopRequestHeaders call to
immediately after cfg.ModifyRequest (but before setting the RequestURI and
lbc.Do) and move stripHopByHopResponseHeaders to immediately after
cfg.ModifyResponse (and before returning the response), preserving the existing
conditionals using policy.KeepHopByHopHeaders and cfg.KeepConnectionHeader so
the same header lists are used when you call stripHopByHopRequestHeaders and
stripHopByHopResponseHeaders.
---
Nitpick comments:
In `@docs/middleware/proxy.md`:
- Line 54: Change the wording in the sentence that mentions `Config.TLSConfig`
and `MinVersion: tls.VersionTLS12` to use "accidentally" (e.g., "cannot be
negotiated accidentally") or "are not negotiated by default" for conciseness;
update the phrase that currently reads "cannot be negotiated by accident" to one
of these alternatives around the `MinVersion: tls.VersionTLS12` description.
🪄 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: f5aaf748-e0fa-4332-9046-d19b2e112917
📒 Files selected for processing (11)
client/errors.goclient/transport.goclient/transport_test.godocs/middleware/proxy.mdmiddleware/proxy/SECURITY_AUDIT.mdmiddleware/proxy/config.gomiddleware/proxy/fuzz_test.gomiddleware/proxy/proxy.gomiddleware/proxy/proxy_test.gomiddleware/proxy/security.gomiddleware/proxy/security_test.go
| body := make([]byte, 512) | ||
| n, _ := resp.Body.Read(body) //nolint:errcheck // bounded buffer, EOF is fine | ||
| require.Contains(t, string(body[:n]), "HTTPS to HTTP redirect blocked") |
There was a problem hiding this comment.
Use full body read to avoid partial-read assertion flakes
A single Read is not guaranteed to return the full response body; this can intermittently miss the expected substring.
Suggested fix
+import "io"- body := make([]byte, 512)
- n, _ := resp.Body.Read(body) //nolint:errcheck // bounded buffer, EOF is fine
- require.Contains(t, string(body[:n]), "HTTPS to HTTP redirect blocked")
+ body, err := io.ReadAll(resp.Body)
+ require.NoError(t, err)
+ require.Contains(t, string(body), "HTTPS to HTTP redirect blocked")📝 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.
| body := make([]byte, 512) | |
| n, _ := resp.Body.Read(body) //nolint:errcheck // bounded buffer, EOF is fine | |
| require.Contains(t, string(body[:n]), "HTTPS to HTTP redirect blocked") | |
| body, err := io.ReadAll(resp.Body) | |
| require.NoError(t, err) | |
| require.Contains(t, string(body), "HTTPS to HTTP redirect blocked") |
🤖 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_test.go` around lines 318 - 320, The test currently
reads the response body with a single call to resp.Body.Read (variable body and
n) which can return a partial payload and cause flaky require.Contains
assertions; replace that partial read with a full read using io.ReadAll (or
ioutil.ReadAll) on resp.Body, check the error with require.NoError, close the
body, and then assert require.Contains(t, string(fullBody), "HTTPS to HTTP
redirect blocked") so the assertion always sees the complete response.
| func WithSecurityPolicy(policy SecurityPolicy) SecurityPolicy { | ||
| if len(policy.AllowedSchemes) == 0 { | ||
| policy.AllowedSchemes = DefaultSecurityPolicy().AllowedSchemes | ||
| } | ||
| policyLock.Lock() | ||
| defer policyLock.Unlock() | ||
| prev := activePolicy | ||
| activePolicy = policy | ||
| return prev |
There was a problem hiding this comment.
Clone AllowedSchemes when normalizing a policy.
These helpers copy the struct but keep the same slice backing array. If a caller mutates the slice they passed to WithSecurityPolicy, the active global allowlist changes outside policyLock, and resolvePolicy can hand the same shared slice to balancers that thought they captured an immutable snapshot.
Suggested fix
+func normalizePolicy(policy SecurityPolicy) SecurityPolicy {
+ if len(policy.AllowedSchemes) == 0 {
+ policy.AllowedSchemes = []string{"http", "https"}
+ } else {
+ policy.AllowedSchemes = append([]string(nil), policy.AllowedSchemes...)
+ }
+ return policy
+}
+
func WithSecurityPolicy(policy SecurityPolicy) SecurityPolicy {
- if len(policy.AllowedSchemes) == 0 {
- policy.AllowedSchemes = DefaultSecurityPolicy().AllowedSchemes
- }
+ policy = normalizePolicy(policy)
policyLock.Lock()
defer policyLock.Unlock()
- prev := activePolicy
+ prev := normalizePolicy(activePolicy)
activePolicy = policy
return prev
}
func currentSecurityPolicy() SecurityPolicy {
policyLock.RLock()
defer policyLock.RUnlock()
- return activePolicy
+ return normalizePolicy(activePolicy)
}
func resolvePolicy(override *SecurityPolicy) SecurityPolicy {
if override != nil {
- policy := *override
- if len(policy.AllowedSchemes) == 0 {
- policy.AllowedSchemes = DefaultSecurityPolicy().AllowedSchemes
- }
- return policy
+ return normalizePolicy(*override)
}
return currentSecurityPolicy()
}Also applies to: 103-121
🤖 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.go` around lines 91 - 99, When normalizing and
installing a SecurityPolicy in WithSecurityPolicy (and the similar helper
later), ensure you deep-copy the AllowedSchemes slice instead of reusing the
backing array: if len(policy.AllowedSchemes)==0 and you assign
DefaultSecurityPolicy().AllowedSchemes or when you assign policy into
activePolicy, replace the slice with a freshly allocated copy (e.g. copy into
append([]string(nil), ...)) so callers cannot mutate the global
activePolicy/resolvePolicy/balancer view via a shared backing array; do this for
every place that assigns or returns a SecurityPolicy with AllowedSchemes to
guarantee immutability outside policyLock.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4405 +/- ##
==========================================
+ Coverage 91.29% 91.45% +0.16%
==========================================
Files 132 133 +1
Lines 13193 13418 +225
==========================================
+ Hits 12044 12272 +228
Misses 729 729
+ Partials 420 417 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
gaby
force-pushed
the
claude/proxy-middleware-security-audit-q45Ya
branch
from
9f6f1e6 to
2ce39b9
Compare
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (2)
middleware/proxy/security_test.go (1)
315-320:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRead the full response body before asserting the downgrade error.
A single
Readis not guaranteed to return the complete payload, so this assertion can miss the expected message intermittently. Useio.ReadAll(resp.Body)and assert on the full body string.Suggested fix
- body := make([]byte, 512) - n, _ := resp.Body.Read(body) //nolint:errcheck // bounded buffer, EOF is fine - require.Contains(t, string(body[:n]), "HTTPS to HTTP redirect blocked") + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + require.Contains(t, string(body), "HTTPS to HTTP redirect blocked")Also add
ioto the import block.🤖 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_test.go` around lines 315 - 320, The test currently reads the response body with a single resp.Body.Read which may not return the entire payload; change the assertion to read the full response body using io.ReadAll(resp.Body) (and add io to the imports), then assert that the returned string contains "HTTPS to HTTP redirect blocked" using the complete body; update the code that references resp.Body.Read in the test function in security_test.go to use io.ReadAll and the same require.Contains assertion on the full body string.middleware/proxy/fuzz_test.go (1)
96-98:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the cspell-blocking typo in this failure message.
unparseableis already failing spell-check CI here;unparsableclears it without changing the test behavior.Based on pipeline failures, cspell is already failing on `unparseable` at this line.Suggested fix
- t.Fatalf("unparseable output %q for input %q: %v", out, requestPath, err) + t.Fatalf("unparsable output %q for input %q: %v", out, requestPath, err)🤖 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/fuzz_test.go` around lines 96 - 98, The test failure message contains a cspell-blocking typo: replace the word "unparseable" with "unparsable" in the t.Fatalf call inside the fuzz test that verifies joinUpstreamPath; locate the t.Fatalf("unparseable output %q for input %q: %v", out, requestPath, err) and update the literal to "unparsable output %q for input %q: %v" so the test behavior remains identical but the spelling check passes.Source: Pipeline failures
🤖 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 `@docs/middleware/proxy.md`:
- Around line 254-259: The docs show proxy.DefaultSecurityPolicy as a variable
but the code exposes DefaultSecurityPolicy() as a function; update the
documentation snippet to call the function and use its returned
proxy.SecurityPolicy (e.g., assign the result of proxy.DefaultSecurityPolicy()
or call it inline) or alternatively change the implementation to export a
variable named DefaultSecurityPolicy of type proxy.SecurityPolicy; reference the
symbol DefaultSecurityPolicy() and the type proxy.SecurityPolicy when making the
change so examples compile and match the API.
In `@middleware/proxy/config.go`:
- Around line 51-54: The comments for MaxConnsPerHost (and the related
MaxResponseBodySize comment) currently imply the proxy will always enforce those
limits, but they are no-ops when Config.Client is set; update the struct field
docs in middleware/proxy/config.go to explicitly state that these limits are
only applied when the proxy creates internal fasthttp.HostClient instances
(i.e., when Config.Client is nil) and are ignored when a custom Config.Client is
provided; reference the Config.Client field and the code path that builds
fasthttp.HostClient in proxy.go to make the behavior clear.
In `@middleware/proxy/proxy_test.go`:
- Around line 107-109: Add t.Parallel() as the first statement inside the
Test_Proxy_DefaultClient_MaxConnsPerHost test so it runs concurrently with other
tests; update the Test_Proxy_DefaultClient_MaxConnsPerHost function (which
asserts defaultMaxConnsPerHost == client.MaxConnsPerHost) to call t.Parallel()
at the top before any assertions.
In `@middleware/proxy/proxy.go`:
- Around line 74-80: The hop-by-hop header stripping
(stripHopByHopRequestHeaders) is currently executed before calling the
policy.ModifyRequest hook, allowing ModifyRequest to re-add banned headers; move
the stripping logic so it runs immediately after the ModifyRequest call
(preserve the cfg.KeepConnectionHeader conditional and existing call signature),
and apply the same change for the response-side stripping block (the analogous
stripHopByHopResponseHeaders/response stripping currently around lines 82-87) so
headers are removed after policy.ModifyResponse as well.
In `@middleware/proxy/SECURITY_AUDIT.md`:
- Around line 22-23: The audit entry incorrectly states the per-host connection
cap fix as “Fixed (opt-in)” with a 0 -> 512 fallback; update the
SECURITY_AUDIT.md entries (rows referencing per-host cap and the same note at
lines 154-156) to reflect the actual shipped default of MaxConnsPerHost = 1024
and clearly state the migration/opt-in semantics (e.g., that the default is
1024, how to opt into prior behavior if any, and any config key name
MaxConnsPerHost used in docs/config). Ensure references to “per-host cap” and
“MaxConnsPerHost” are consistent across both locations.
- Around line 175-186: The verification steps in the SECURITY_AUDIT.md changes
run Go tests and linting but omit Markdown linting; add a step invoking the
repository’s Markdown linter (run `make markdown`) into the verification block
so changes to Markdown files are validated. Locate the verification command
block that contains the test and lint commands (the shell snippet with `go test
./middleware/proxy/ ./client/ -count=1 -skip IPv6` and the `golangci-lint` line)
and insert a `make markdown` command among those steps to ensure `**/*.md` is
linted when docs are modified.
In `@middleware/proxy/security_test.go`:
- Around line 266-313: The test mutates the package-global security policy via
WithSecurityPolicy, so remove t.Parallel() from
Test_Security_DoRedirects_BlocksDowngrade (and the other affected test) and
replace the manual policy override block with the scoped helper
withSecurityPolicyForTest(...) so the override is applied only for this test and
automatically restored; specifically, instead of calling policy :=
DefaultSecurityPolicy(); policy.AllowPrivateIPs = true; prev :=
WithSecurityPolicy(policy); defer WithSecurityPolicy(prev) use
withSecurityPolicyForTest(t, func() { set the AllowPrivateIPs on a copy of
DefaultSecurityPolicy and run the DoRedirects call inside that closure }) to
ensure exclusive, non-parallel access to the global activePolicy.
- Around line 182-187: The handler passed to createProxyTestServerIPv4 uses
require.Empty inside the Fiber handler (calling require.Empty on c.Get for
headers/hops), which violates the go-require rule; replace those in-handler
require calls by capturing the header values (e.g., collect c.Get(h) results and
any custom header) and either call t.Errorf/t.Fatalf from inside the handler
instead of require.* or better return the values and perform require assertions
after invoking app.Test/createProxyTestServerIPv4 so assertions run in the test
goroutine; update both occurrences around the handler that iterates over hops
and the check for "X-Custom-Hop" accordingly.
In `@middleware/proxy/security.go`:
- Around line 355-357: joinUpstreamPath currently overwrites the upstream base
path when parsed.Path is set, causing configured base paths (e.g. /v1) to be
dropped; change the logic in joinUpstreamPath so that when parsed.Path is
present you join it with the upstream/base path instead of replacing it
(preserve leading/trailing slashes correctly and avoid duplicate slashes), and
apply the same join to RawPath; refer to the joinUpstreamPath function and the
parsed/ out.Path and out.RawPath assignments (used by
DomainForward/BalancerForward) to locate and update the code.
- Around line 222-239: validateUpstream currently validates the hostname via
validateHostForSSRF but then returns the original URL (u) which proxy.go later
dials using u.Host/u.String, leaving a TOCTOU hole; modify
validateUpstream/consumer so that either (a) the vetted IP addresses are pinned
into the dial step (provide a custom DialContext/Dial function that uses the
validated IPs instead of resolving the hostname), or (b) perform the
private-range/SSRF check again inside a custom dial path immediately before
connect; update the code paths that call validateUpstream (and the transport in
proxy.go where it dials u.Host/u.String) to use the custom dialer or IP-pinned
address to ensure the same vetted IPs are used for the actual TCP connect (refer
to symbols validateUpstream, validateHostForSSRF, u.Host, u.String and the dial
hooks in transport/proxy).
- Around line 188-190: Replace strings.Split with strings.SplitSeq in
connectionListedHeaders: in the loop over values (function
connectionListedHeaders) use strings.SplitSeq(string(v), ",") and keep the
existing TrimSpace + non-empty append logic to satisfy the linter. In
joinUpstreamPath ensure you do not overwrite configured base.Path when setting
out.Path — preserve or prepend base.Path to the parsed.Path instead of replacing
it (adjust logic around out.Path = parsed.Path in joinUpstreamPath). For
validateHostForSSRF, eliminate the TOCTOU by resolving the hostname to IP(s) and
pinning those addresses for the subsequent dial (or perform the equivalent
dial-time validation) so that the fasthttp.HostClient dial uses the verified
IPs/addresses rather than re-resolving u.Host; update validateHostForSSRF and
the caller in proxy.go (where fasthttp.HostClient{Addr: u.Host} is created) to
accept/use resolved IPs.
---
Duplicate comments:
In `@middleware/proxy/fuzz_test.go`:
- Around line 96-98: The test failure message contains a cspell-blocking typo:
replace the word "unparseable" with "unparsable" in the t.Fatalf call inside the
fuzz test that verifies joinUpstreamPath; locate the t.Fatalf("unparseable
output %q for input %q: %v", out, requestPath, err) and update the literal to
"unparsable output %q for input %q: %v" so the test behavior remains identical
but the spelling check passes.
In `@middleware/proxy/security_test.go`:
- Around line 315-320: The test currently reads the response body with a single
resp.Body.Read which may not return the entire payload; change the assertion to
read the full response body using io.ReadAll(resp.Body) (and add io to the
imports), then assert that the returned string contains "HTTPS to HTTP redirect
blocked" using the complete body; update the code that references resp.Body.Read
in the test function in security_test.go to use io.ReadAll and the same
require.Contains assertion on the full body string.
🪄 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: 52ed3e7c-7866-4828-9705-f8a6640e84a4
📒 Files selected for processing (11)
client/errors.goclient/transport.goclient/transport_test.godocs/middleware/proxy.mdmiddleware/proxy/SECURITY_AUDIT.mdmiddleware/proxy/config.gomiddleware/proxy/fuzz_test.gomiddleware/proxy/proxy.gomiddleware/proxy/proxy_test.gomiddleware/proxy/security.gomiddleware/proxy/security_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- client/errors.go
- client/transport_test.go
- client/transport.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 11
♻️ Duplicate comments (2)
middleware/proxy/security_test.go (1)
315-320:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRead the full response body before asserting the downgrade error.
A single
Readis not guaranteed to return the complete payload, so this assertion can miss the expected message intermittently. Useio.ReadAll(resp.Body)and assert on the full body string.Suggested fix
- body := make([]byte, 512) - n, _ := resp.Body.Read(body) //nolint:errcheck // bounded buffer, EOF is fine - require.Contains(t, string(body[:n]), "HTTPS to HTTP redirect blocked") + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + require.Contains(t, string(body), "HTTPS to HTTP redirect blocked")Also add
ioto the import block.🤖 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_test.go` around lines 315 - 320, The test currently reads the response body with a single resp.Body.Read which may not return the entire payload; change the assertion to read the full response body using io.ReadAll(resp.Body) (and add io to the imports), then assert that the returned string contains "HTTPS to HTTP redirect blocked" using the complete body; update the code that references resp.Body.Read in the test function in security_test.go to use io.ReadAll and the same require.Contains assertion on the full body string.middleware/proxy/fuzz_test.go (1)
96-98:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the cspell-blocking typo in this failure message.
unparseableis already failing spell-check CI here;unparsableclears it without changing the test behavior.Based on pipeline failures, cspell is already failing on `unparseable` at this line.Suggested fix
- t.Fatalf("unparseable output %q for input %q: %v", out, requestPath, err) + t.Fatalf("unparsable output %q for input %q: %v", out, requestPath, err)🤖 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/fuzz_test.go` around lines 96 - 98, The test failure message contains a cspell-blocking typo: replace the word "unparseable" with "unparsable" in the t.Fatalf call inside the fuzz test that verifies joinUpstreamPath; locate the t.Fatalf("unparseable output %q for input %q: %v", out, requestPath, err) and update the literal to "unparsable output %q for input %q: %v" so the test behavior remains identical but the spelling check passes.Source: Pipeline failures
🤖 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 `@docs/middleware/proxy.md`:
- Around line 254-259: The docs show proxy.DefaultSecurityPolicy as a variable
but the code exposes DefaultSecurityPolicy() as a function; update the
documentation snippet to call the function and use its returned
proxy.SecurityPolicy (e.g., assign the result of proxy.DefaultSecurityPolicy()
or call it inline) or alternatively change the implementation to export a
variable named DefaultSecurityPolicy of type proxy.SecurityPolicy; reference the
symbol DefaultSecurityPolicy() and the type proxy.SecurityPolicy when making the
change so examples compile and match the API.
In `@middleware/proxy/config.go`:
- Around line 51-54: The comments for MaxConnsPerHost (and the related
MaxResponseBodySize comment) currently imply the proxy will always enforce those
limits, but they are no-ops when Config.Client is set; update the struct field
docs in middleware/proxy/config.go to explicitly state that these limits are
only applied when the proxy creates internal fasthttp.HostClient instances
(i.e., when Config.Client is nil) and are ignored when a custom Config.Client is
provided; reference the Config.Client field and the code path that builds
fasthttp.HostClient in proxy.go to make the behavior clear.
In `@middleware/proxy/proxy_test.go`:
- Around line 107-109: Add t.Parallel() as the first statement inside the
Test_Proxy_DefaultClient_MaxConnsPerHost test so it runs concurrently with other
tests; update the Test_Proxy_DefaultClient_MaxConnsPerHost function (which
asserts defaultMaxConnsPerHost == client.MaxConnsPerHost) to call t.Parallel()
at the top before any assertions.
In `@middleware/proxy/proxy.go`:
- Around line 74-80: The hop-by-hop header stripping
(stripHopByHopRequestHeaders) is currently executed before calling the
policy.ModifyRequest hook, allowing ModifyRequest to re-add banned headers; move
the stripping logic so it runs immediately after the ModifyRequest call
(preserve the cfg.KeepConnectionHeader conditional and existing call signature),
and apply the same change for the response-side stripping block (the analogous
stripHopByHopResponseHeaders/response stripping currently around lines 82-87) so
headers are removed after policy.ModifyResponse as well.
In `@middleware/proxy/SECURITY_AUDIT.md`:
- Around line 22-23: The audit entry incorrectly states the per-host connection
cap fix as “Fixed (opt-in)” with a 0 -> 512 fallback; update the
SECURITY_AUDIT.md entries (rows referencing per-host cap and the same note at
lines 154-156) to reflect the actual shipped default of MaxConnsPerHost = 1024
and clearly state the migration/opt-in semantics (e.g., that the default is
1024, how to opt into prior behavior if any, and any config key name
MaxConnsPerHost used in docs/config). Ensure references to “per-host cap” and
“MaxConnsPerHost” are consistent across both locations.
- Around line 175-186: The verification steps in the SECURITY_AUDIT.md changes
run Go tests and linting but omit Markdown linting; add a step invoking the
repository’s Markdown linter (run `make markdown`) into the verification block
so changes to Markdown files are validated. Locate the verification command
block that contains the test and lint commands (the shell snippet with `go test
./middleware/proxy/ ./client/ -count=1 -skip IPv6` and the `golangci-lint` line)
and insert a `make markdown` command among those steps to ensure `**/*.md` is
linted when docs are modified.
In `@middleware/proxy/security_test.go`:
- Around line 266-313: The test mutates the package-global security policy via
WithSecurityPolicy, so remove t.Parallel() from
Test_Security_DoRedirects_BlocksDowngrade (and the other affected test) and
replace the manual policy override block with the scoped helper
withSecurityPolicyForTest(...) so the override is applied only for this test and
automatically restored; specifically, instead of calling policy :=
DefaultSecurityPolicy(); policy.AllowPrivateIPs = true; prev :=
WithSecurityPolicy(policy); defer WithSecurityPolicy(prev) use
withSecurityPolicyForTest(t, func() { set the AllowPrivateIPs on a copy of
DefaultSecurityPolicy and run the DoRedirects call inside that closure }) to
ensure exclusive, non-parallel access to the global activePolicy.
- Around line 182-187: The handler passed to createProxyTestServerIPv4 uses
require.Empty inside the Fiber handler (calling require.Empty on c.Get for
headers/hops), which violates the go-require rule; replace those in-handler
require calls by capturing the header values (e.g., collect c.Get(h) results and
any custom header) and either call t.Errorf/t.Fatalf from inside the handler
instead of require.* or better return the values and perform require assertions
after invoking app.Test/createProxyTestServerIPv4 so assertions run in the test
goroutine; update both occurrences around the handler that iterates over hops
and the check for "X-Custom-Hop" accordingly.
In `@middleware/proxy/security.go`:
- Around line 355-357: joinUpstreamPath currently overwrites the upstream base
path when parsed.Path is set, causing configured base paths (e.g. /v1) to be
dropped; change the logic in joinUpstreamPath so that when parsed.Path is
present you join it with the upstream/base path instead of replacing it
(preserve leading/trailing slashes correctly and avoid duplicate slashes), and
apply the same join to RawPath; refer to the joinUpstreamPath function and the
parsed/ out.Path and out.RawPath assignments (used by
DomainForward/BalancerForward) to locate and update the code.
- Around line 222-239: validateUpstream currently validates the hostname via
validateHostForSSRF but then returns the original URL (u) which proxy.go later
dials using u.Host/u.String, leaving a TOCTOU hole; modify
validateUpstream/consumer so that either (a) the vetted IP addresses are pinned
into the dial step (provide a custom DialContext/Dial function that uses the
validated IPs instead of resolving the hostname), or (b) perform the
private-range/SSRF check again inside a custom dial path immediately before
connect; update the code paths that call validateUpstream (and the transport in
proxy.go where it dials u.Host/u.String) to use the custom dialer or IP-pinned
address to ensure the same vetted IPs are used for the actual TCP connect (refer
to symbols validateUpstream, validateHostForSSRF, u.Host, u.String and the dial
hooks in transport/proxy).
- Around line 188-190: Replace strings.Split with strings.SplitSeq in
connectionListedHeaders: in the loop over values (function
connectionListedHeaders) use strings.SplitSeq(string(v), ",") and keep the
existing TrimSpace + non-empty append logic to satisfy the linter. In
joinUpstreamPath ensure you do not overwrite configured base.Path when setting
out.Path — preserve or prepend base.Path to the parsed.Path instead of replacing
it (adjust logic around out.Path = parsed.Path in joinUpstreamPath). For
validateHostForSSRF, eliminate the TOCTOU by resolving the hostname to IP(s) and
pinning those addresses for the subsequent dial (or perform the equivalent
dial-time validation) so that the fasthttp.HostClient dial uses the verified
IPs/addresses rather than re-resolving u.Host; update validateHostForSSRF and
the caller in proxy.go (where fasthttp.HostClient{Addr: u.Host} is created) to
accept/use resolved IPs.
---
Duplicate comments:
In `@middleware/proxy/fuzz_test.go`:
- Around line 96-98: The test failure message contains a cspell-blocking typo:
replace the word "unparseable" with "unparsable" in the t.Fatalf call inside the
fuzz test that verifies joinUpstreamPath; locate the t.Fatalf("unparseable
output %q for input %q: %v", out, requestPath, err) and update the literal to
"unparsable output %q for input %q: %v" so the test behavior remains identical
but the spelling check passes.
In `@middleware/proxy/security_test.go`:
- Around line 315-320: The test currently reads the response body with a single
resp.Body.Read which may not return the entire payload; change the assertion to
read the full response body using io.ReadAll(resp.Body) (and add io to the
imports), then assert that the returned string contains "HTTPS to HTTP redirect
blocked" using the complete body; update the code that references resp.Body.Read
in the test function in security_test.go to use io.ReadAll and the same
require.Contains assertion on the full body string.
🪄 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: 52ed3e7c-7866-4828-9705-f8a6640e84a4
📒 Files selected for processing (11)
client/errors.goclient/transport.goclient/transport_test.godocs/middleware/proxy.mdmiddleware/proxy/SECURITY_AUDIT.mdmiddleware/proxy/config.gomiddleware/proxy/fuzz_test.gomiddleware/proxy/proxy.gomiddleware/proxy/proxy_test.gomiddleware/proxy/security.gomiddleware/proxy/security_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- client/errors.go
- client/transport_test.go
- client/transport.go
🛑 Comments failed to post (11)
docs/middleware/proxy.md (1)
254-259:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winDefault policy snippet shows a non-existent exported variable.
proxy.DefaultSecurityPolicyis documented here as a variable, but the implementation exposesDefaultSecurityPolicy()as a function. This doc/API mismatch will mislead users and produce invalid usage examples.Proposed doc fix
-var DefaultSecurityPolicy = proxy.SecurityPolicy{ +var defaultPolicy = proxy.DefaultSecurityPolicy() +// defaultPolicy is: +// proxy.SecurityPolicy{ AllowedSchemes: []string{"http", "https"}, AllowPrivateIPs: false, AllowHTTPSDowngrade: false, KeepHopByHopHeaders: false, -} +//}🤖 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 `@docs/middleware/proxy.md` around lines 254 - 259, The docs show proxy.DefaultSecurityPolicy as a variable but the code exposes DefaultSecurityPolicy() as a function; update the documentation snippet to call the function and use its returned proxy.SecurityPolicy (e.g., assign the result of proxy.DefaultSecurityPolicy() or call it inline) or alternatively change the implementation to export a variable named DefaultSecurityPolicy of type proxy.SecurityPolicy; reference the symbol DefaultSecurityPolicy() and the type proxy.SecurityPolicy when making the change so examples compile and match the API.middleware/proxy/config.go (1)
51-54:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument that these limits are ignored when
Config.Clientis set.
proxy.goonly appliesMaxConnsPerHostandMaxResponseBodySizewhile building internalfasthttp.HostClients in Line 26-Line 57. With a customConfig.Client, both knobs are no-ops today, so the current comments overpromise the cap/limit behavior.Also applies to: 91-97
🤖 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/config.go` around lines 51 - 54, The comments for MaxConnsPerHost (and the related MaxResponseBodySize comment) currently imply the proxy will always enforce those limits, but they are no-ops when Config.Client is set; update the struct field docs in middleware/proxy/config.go to explicitly state that these limits are only applied when the proxy creates internal fasthttp.HostClient instances (i.e., when Config.Client is nil) and are ignored when a custom Config.Client is provided; reference the Config.Client field and the code path that builds fasthttp.HostClient in proxy.go to make the behavior clear.middleware/proxy/proxy_test.go (1)
107-109:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
t.Parallel()at the start of this test.This new top-level test is read-only and doesn't participate in the global-client override flow, so it should follow the repo's
*_test.goconvention.As per coding guidelines,
**/*_test.go: Always invoket.Parallel()at the start of each test and subtest to maximize concurrency in Go tests.🤖 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_test.go` around lines 107 - 109, Add t.Parallel() as the first statement inside the Test_Proxy_DefaultClient_MaxConnsPerHost test so it runs concurrently with other tests; update the Test_Proxy_DefaultClient_MaxConnsPerHost function (which asserts defaultMaxConnsPerHost == client.MaxConnsPerHost) to call t.Parallel() at the top before any assertions.Source: Coding guidelines
middleware/proxy/proxy.go (1)
74-80:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winStrip hop-by-hop headers after
ModifyRequest, not before it.A
ModifyRequesthook can currently re-addConnection,Proxy-Authorization,TE, or Connection-listed headers after the security pass, so the “secure by default” stripping is bypassed on the final outbound request.Suggested fix
- if !policy.KeepHopByHopHeaders { - if cfg.KeepConnectionHeader { - stripHopByHopRequestHeaders(req, fiber.HeaderConnection) - } else { - stripHopByHopRequestHeaders(req) - } - } - // Modify request if cfg.ModifyRequest != nil { if err := cfg.ModifyRequest(c); err != nil { return err } } + + if !policy.KeepHopByHopHeaders { + if cfg.KeepConnectionHeader { + stripHopByHopRequestHeaders(req, fiber.HeaderConnection) + } else { + stripHopByHopRequestHeaders(req) + } + }Also applies to: 82-87
🤖 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 74 - 80, The hop-by-hop header stripping (stripHopByHopRequestHeaders) is currently executed before calling the policy.ModifyRequest hook, allowing ModifyRequest to re-add banned headers; move the stripping logic so it runs immediately after the ModifyRequest call (preserve the cfg.KeepConnectionHeader conditional and existing call signature), and apply the same change for the response-side stripping block (the analogous stripHopByHopResponseHeaders/response stripping currently around lines 82-87) so headers are removed after policy.ModifyResponse as well.middleware/proxy/SECURITY_AUDIT.md (2)
22-23:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winConnection-cap finding is inconsistent with the documented defaults.
This audit says per-host cap is “Fixed (opt-in)” and frames
0 -> 512fallback as the fix behavior, but the updated proxy docs/config describeMaxConnsPerHostdefaulting to1024. Please align this section with the actual shipped default so migration impact is accurately documented.Also applies to: 154-156
🤖 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_AUDIT.md` around lines 22 - 23, The audit entry incorrectly states the per-host connection cap fix as “Fixed (opt-in)” with a 0 -> 512 fallback; update the SECURITY_AUDIT.md entries (rows referencing per-host cap and the same note at lines 154-156) to reflect the actual shipped default of MaxConnsPerHost = 1024 and clearly state the migration/opt-in semantics (e.g., that the default is 1024, how to opt into prior behavior if any, and any config key name MaxConnsPerHost used in docs/config). Ensure references to “per-host cap” and “MaxConnsPerHost” are consistent across both locations.
175-186:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInclude markdown lint in verification steps.
Since this PR modifies Markdown docs, add
make markdownto the verification block to match repo review requirements.Proposed addition
# Linting golangci-lint run ./middleware/proxy/... ./client/... +make markdownAs per coding guidelines: “
**/*.md: Runmake markdownto lint all Markdown files when modifying code”.📝 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.🤖 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_AUDIT.md` around lines 175 - 186, The verification steps in the SECURITY_AUDIT.md changes run Go tests and linting but omit Markdown linting; add a step invoking the repository’s Markdown linter (run `make markdown`) into the verification block so changes to Markdown files are validated. Locate the verification command block that contains the test and lint commands (the shell snippet with `go test ./middleware/proxy/ ./client/ -count=1 -skip IPv6` and the `golangci-lint` line) and insert a `make markdown` command among those steps to ensure `**/*.md` is linted when docs are modified.Source: Coding guidelines
middleware/proxy/security_test.go (2)
182-187:
⚠️ Potential issue | 🟠 Major🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # 1) Inspect the referenced test section sed -n '150,260p' middleware/proxy/security_test.go | nl -ba | sed -n '1,140p' # 2) Find all require.* calls inside this file rg -n --fixed-string 'require.' middleware/proxy/security_test.go # 3) Look for other instances of require.* inside Fiber handler funcs across the repo rg -n --glob='*_test.go' 'require\.(Empty|NotEmpty|True|False|NoError|Error|Equal|NotEqual|Len|Contains|NotContains|Nil|NotNil|Panics|NotPanics)' . # 4) Check if there is lint/config explicitly banning require in handlers ls -la fd -a 'golangci\.yml|.golangci\.yml|golangci\.yaml|.golangci\.yaml' . 2>/dev/null || true rg -n 'require\.' .golangci* **/*.yml **/*yaml **/*toml **/*json 2>/dev/null || true rg -n 'require.*handler|handler.*require|forbid.*require|forbid.*assert' . 2>/dev/null || true rg -n 'forbidigo|revive|stylecheck|gocritic|gosec|testify' . 2>/dev/null || trueRepository: gofiber/fiber
Length of output: 100
🏁 Script executed:
#!/bin/bash set -euo pipefail # 1) Show the referenced region with line numbers (cat -n is available) cat -n middleware/proxy/security_test.go | sed -n '150,260p' # 2) List all require.* usages in this file with a bit of surrounding context rg -n --context 3 'require\.' middleware/proxy/security_test.go # 3) Find other require.* in any *_test.go that appear inside handlers: # - Heuristic: handler signature includes "fiber.Ctx" or "func(c fiber.Ctx)". # - Also capture net/http handler style to see if any relevant pattern exists. rg -n --glob='*_test.go' 'require\.' . | head -n 200 rg -n --glob='*_test.go' 'func\s*\(\s*c\s*fiber\.Ctx\s*\)\s*error|func\s*\(\s*ctx\s*fiber\.Ctx\s*\)\s*error|func\s*\(\s*c\s*fiber\.Ctx\s*\)\s*\{' rg -n --glob='*_test.go' --context 3 'func\s*\(\s*c\s*fiber\.Ctx\s*\)\s*error' . | head -n 120 # 4) Inspect linter config(s) for rules referencing forbid/require/assertions # Search common config filenames and contents for 'require' or 'testify' fd -a -t f 'golangci\.yml|\.golangci\.yml|golangci\.yaml|\.golangci\.yaml|golangci\.toml|\.golangci\.toml' . 2>/dev/null || true rg -n --hidden --glob='*.yml' --glob='*.yaml' --glob='*.toml' --glob='*.json' 'require\.|testify\.|forbid.*require|revive|gocritic|forbidigo|testify' . 2>/dev/null || trueRepository: gofiber/fiber
Length of output: 50369
🏁 Script executed:
#!/bin/bash set -euo pipefail # Locate golangci-lint config files (and any linter config that might mention require/testify/handlers) fd -a -t f 'golangci\.yml|\.golangci\.yml|golangci\.yaml|\.golangci\.yaml|golangci\.toml|\.golangci\.toml|\.golangci\.toml' . 2>/dev/null || true fd -a -t f '.*revive.*\.ya?ml|.*gocritic.*\.ya?ml|.*gofmt.*|.*staticcheck.*|.*errcheck.*|.*testify.*' . 2>/dev/null || true # If we found golangci config(s), search them for rules mentioning require/testify/handlers for cfg in $(fd -a -t f 'golangci\.yml|\.golangci\.yml|golangci\.yaml|\.golangci\.yaml|golangci\.toml|\.golangci\.toml' . 2>/dev/null || true); do echo "---- $cfg ----" rg -n 'require\.|testify\.|forbid.*require|revive|gocritic|forbidigo|testify|handler' "$cfg" || true done # Also search for any custom lint rule/docs mentioning "require" + "handler" anywhere in repo, but keep output bounded rg -n --max-count=50 'require\.(' . || true rg -n --max-count=50 'require.*handler|handler.*require|forbid.*require|assert.*handler|testing\.T' . || trueRepository: gofiber/fiber
Length of output: 50459
🏁 Script executed:
#!/bin/bash set -euo pipefail # Show the exact handler regions for the two require.* call sites echo "=== middleware/proxy/security_test.go (relevant lines) ===" sed -n '160,210p' middleware/proxy/security_test.go | nl -ba 2>/dev/null || cat -n middleware/proxy/security_test.go | sed -n '160,210p' sed -n '220,250p' middleware/proxy/security_test.go | nl -ba 2>/dev/null || cat -n middleware/proxy/security_test.go | sed -n '220,250p' # Find golangci-lint configs and grep for anything mentioning require/testify/handler assertions echo "=== linter configs ===" configs="$(fd -a -t f 'golangci\.yml|\.golangci\.yml|golangci\.yaml|\.golangci\.yaml|golangci\.toml|\.golangci\.toml' . 2>/dev/null || true)" if [ -z "$configs" ]; then echo "No golangci config found by filename." else echo "$configs" for cfg in $configs; do echo "---- grep require/testify/handler in $cfg ----" rg -n 'require\.|testify\.|forbid.*require|handler.*require|require.*handler|assert.*handler|FailNow|runtime\.Goexit' "$cfg" || true done fi # Search for custom analyzers/doc text in repo (best-effort, limited output) echo "=== repo-wide search for handler+require rules/doc ===" rg -n --max-count=80 'require\.(Empty|Equal|True|False|NoError|Error|NotNil|Nil|Len|Contains|NotContains|Panics|PanicsWith|PanicsWithValue|NotPanics)|require\..*handler|handler.*require|assert.*handler' . || true # Search for testifylint / custom tool names that might enforce this echo "=== search for testifylint / custom rule names ===" rg -n --max-count=80 'testifylint|require.*lint|assert.*lint|FailNow.*lint|Goexit.*lint|handler.*assert' . || true # As a fallback, find any direct mention of "require" + "handler" in comments/docs echo "=== search for wording ===" rg -n --max-count=80 '(forbid|disallow|invalid).*(require|testify)|require.*(disallow|forbid|invalid)' . || trueRepository: gofiber/fiber
Length of output: 50370
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "=== Find CI/lint config referencing golangci-lint or custom analyzers ===" rg -n --hidden --no-ignore -S 'golangci-lint|golangci_lint|testifylint|staticcheck|revive|gocritic|forbidigo|gofumpt|errcheck' .github . 2>/dev/null || true ls -la .github 2>/dev/null || true fd -a -t f '.*lint.*|.*golangci.*|.*static.*|.*revive.*|.*gocritic.*|.*forbid.*|Makefile|magefile\.go' . 2>/dev/null || true echo "=== Search for require assertions that use fiber ctx (c.) in *_test.go ===" # Heuristic: require.*(..., c.<...>) is very likely inside a fiber handler. rg -n --glob='*_test.go' --hidden -S 'require\.[A-Za-z0-9_]+\([^,]*,\s*c\.[A-Za-z0-9_]+\b' . echo "=== Search for require assertions inside fiber handler literal bodies (multiline heuristic) ===" # Limited scope to reduce noise: middleware/proxy tests first rg -n --glob='*_test.go' --hidden -U --pcre2 '(?s)func\s*\(\s*\w+\s*fiber\.Ctx\s*\)\s*error\s*\{.{0,400}?require\.' middleware 2>/dev/null || true rg -n --glob='*_test.go' --hidden -U --pcre2 '(?s)func\s*\(\s*\w+\s*fiber\.Ctx\s*\)\s*error\s*\{.{0,400}?require\.' . 2>/dev/null | head -n 80 || true echo "=== Search repo text for explicit wording about require.* in handlers ===" rg -n --hidden --no-ignore -S 'require.*(handler|handlers|fiber\.Ctx|http\.Handler|t\.Parallel)|disallow.*require|forbid.*require' . 2>/dev/null | head -n 80 || trueRepository: gofiber/fiber
Length of output: 50370
🏁 Script executed:
#!/bin/bash set -euo pipefail # 1) See CI lint command/config usage echo "=== .github/workflows/lint.yml ===" sed -n '1,120p' .github/workflows/lint.yml | cat # 2) Search for golangci-lint config files (some repos use default name) echo "=== Find golangci config by content ===" fd -a -t f '.*golangci.*(yml|yaml|toml|json)$' . 2>/dev/null || true fd -a -t f '(.*lint.*(yml|yaml|toml|json)$)' . 2>/dev/null || true # 3) Search for testifylint rule implementation/config echo "=== Search for testifylint ===" rg -n --hidden -S 'testifylint' . || true # 4) If custom analyzer is configured in CI flags, find it echo "=== Search CI for 'testifylint' / 'no require' strings ===" rg -n --hidden -S 'testifylint|require.*handler|handler.*require|require.*assert' .github workflows config.yml . 2>/dev/null || true # 5) Inspect root config.yml (often holds golangci/gofmt/etc settings) if [ -f config.yml ]; then echo "=== config.yml (head) ===" head -n 200 config.yml fi # 6) Search for any nolint/testifylint disable in handler contexts (spot-check) rg -n --hidden -S 'nolint:testifylint' . | head -n 50 || trueRepository: gofiber/fiber
Length of output: 13916
🌐 Web query:
gofiber go-lint-single.yml require.* handler testifylint💡 Result:
In the GoFiber project, the
testifylintlinter is used to enforce idiomatic usage of thestretchr/testifylibrary [1][2]. The specific behavior you are inquiring about involves thego-requirechecker withintestifylint, which warns against the use ofrequireassertions inside HTTP handlers [3][4]. Because HTTP handlers run in separate goroutines that service incoming connections, usingrequire(which typically terminates a test execution) inside these handlers can lead to undefined behavior, difficulty in debugging, and premature test termination [3][4]. To manage this in the Fiber codebase and other Go projects: 1. Configuration: In your.golangci.yml, you can enabletestifylintwith all checkers enabled [1]: testifylint: enable-all: true 2. Handling HTTP Handlers: If you need to intentionally use these patterns or encounter false positives, the check can be disabled specifically for HTTP handlers using the following flag in your linter configuration or via command-line arguments [3]: --go-require.ignore-http-handlers In the GoFiber repository, this linter has been integrated as part of efforts to improve test code quality and ensure adherence to best practices [5][6][7]. If you encounter linter violations, the recommended fix is typically to refactor the test to ensure assertions run in the main test goroutine or to use more deterministic patterns, such as channels, to coordinate asynchronous tests [5][4].Citations:
- 1: https://github.com/gofiber/fiber/blob/ce4202e5f47222defe6aa3f4ccc765b7abbdf64c/.golangci.yml
- 2: #2816
- 3: https://pkg.go.dev/github.com/Antonboom/testifylint
- 4: #2891
- 5: #3775
- 6: #2874
- 7: #2805
Don’t use
require.*inside Fiber handler functions
middleware/proxy/security_test.gocallsrequire.Emptyfrom within thefunc(c fiber.Ctx) error { ... }handler passed tocreateProxyTestServerIPv4(182-187). The repo’stestifylint(go-require) flags this handler-context usage. Switch tot.Errorf/t.Fatalf(or capture values and assert afterapp.Test).Current code at 182-187
_, addr := createProxyTestServerIPv4(t, func(c fiber.Ctx) error { for _, h := range hops { require.Empty(t, c.Get(h), "expected hop-by-hop %q stripped", h) } require.Empty(t, c.Get("X-Custom-Hop"), "Connection-listed header should be stripped") return c.SendString("ok") })Also applies to: 233-236
🤖 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_test.go` around lines 182 - 187, The handler passed to createProxyTestServerIPv4 uses require.Empty inside the Fiber handler (calling require.Empty on c.Get for headers/hops), which violates the go-require rule; replace those in-handler require calls by capturing the header values (e.g., collect c.Get(h) results and any custom header) and either call t.Errorf/t.Fatalf from inside the handler instead of require.* or better return the values and perform require assertions after invoking app.Test/createProxyTestServerIPv4 so assertions run in the test goroutine; update both occurrences around the handler that iterates over hops and the check for "X-Custom-Hop" accordingly.Source: Learnings
266-313:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep tests that mutate
WithSecurityPolicyserial.
WithSecurityPolicyrewrites the package-global active policy inmiddleware/proxy/security.go, sot.Parallel()here can leakAllowPrivateIPs=trueinto unrelated tests and make the suite nondeterministic. Scope the override withwithSecurityPolicyForTest(...)at the test level and keep these policy-mutating tests non-parallel.Based on
middleware/proxy/security.go,WithSecurityPolicymutates the package-globalactivePolicy, so these tests need exclusive access to it.Also applies to: 354-367
🧰 Tools
🪛 OpenGrep (1.22.0)
[ERROR] 301-301: TLS certificate verification is disabled via InsecureSkipVerify. This allows man-in-the-middle attacks. Remove InsecureSkipVerify or set it to false.
(coderabbit.tls.go-insecure-skip-verify)
[ERROR] 301-301: TLS certificate verification is disabled via InsecureSkipVerify. This allows man-in-the-middle attacks. Remove InsecureSkipVerify or set it to false.
(coderabbit.tls.go-insecure-skip-verify)
🤖 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_test.go` around lines 266 - 313, The test mutates the package-global security policy via WithSecurityPolicy, so remove t.Parallel() from Test_Security_DoRedirects_BlocksDowngrade (and the other affected test) and replace the manual policy override block with the scoped helper withSecurityPolicyForTest(...) so the override is applied only for this test and automatically restored; specifically, instead of calling policy := DefaultSecurityPolicy(); policy.AllowPrivateIPs = true; prev := WithSecurityPolicy(policy); defer WithSecurityPolicy(prev) use withSecurityPolicyForTest(t, func() { set the AllowPrivateIPs on a copy of DefaultSecurityPolicy and run the DoRedirects call inside that closure }) to ensure exclusive, non-parallel access to the global activePolicy.middleware/proxy/security.go (3)
188-190:
⚠️ Potential issue | 🔴 Critical🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== Declared Go/toolchain files ==" fd -HI '^(go\.mod|go\.work|\.tool-versions|\.go-version)$' -x sh -c ' echo "--- $1 ---" sed -n "1,80p" "$1" ' sh {} echo echo "== Existing SplitSeq usage in the repo ==" rg -n --glob '*.go' '\bSplitSeq\s*\(' || trueRepository: gofiber/fiber
Length of output: 2871
🏁 Script executed:
#!/bin/bash set -euo pipefail file="middleware/proxy/security.go" echo "== File exists ==" test -f "$file" && echo "found $file" || (echo "missing $file" && exit 1) echo echo "== Show lines around 188-190 ==" nl -ba "$file" | sed -n '160,230p' echo echo "== Locate joinUpstreamPath ==" rg -n "func\s+joinUpstreamPath|joinUpstreamPath\(" "$file" nl -ba "$file" | sed -n '230,330p' || true echo echo "== Locate validateHostForSSRF ==" rg -n "validateHostForSSRF" "$file" nl -ba "$file" | sed -n '330,430p' || true echo echo "== Check for strings.Split usage in this file ==" rg -n "strings\.Split\(" "$file"Repository: gofiber/fiber
Length of output: 205
🏁 Script executed:
#!/bin/bash set -euo pipefail file="middleware/proxy/security.go" echo "== Show lines around 188-190 (line-numbered) ==" awk 'NR>=160 && NR<=230 {printf "%d\t%s\n", NR, $0}' "$file" echo echo "== Locate joinUpstreamPath (definitions/calls) ==" rg -n "func\s+joinUpstreamPath|joinUpstreamPath\(" "$file" rg -n "joinUpstreamPath" -S "$file" echo echo "== Show joinUpstreamPath body (approx range) ==" # show larger window for context awk 'NR>=200 && NR<=320 {printf "%d\t%s\n", NR, $0}' "$file" echo echo "== Locate validateHostForSSRF ==" rg -n "validateHostForSSRF" "$file" awk 'NR>=320 && NR<=520 {printf "%d\t%s\n", NR, $0}' "$file" echo echo "== Check for strings.Split usage in this file ==" rg -n "strings\.Split\(" "$file"Repository: gofiber/fiber
Length of output: 8927
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n --hidden --no-ignore-vcs "validateUpstream\(" middleware/proxy -S rg -n --hidden --no-ignore-vcs "validateHostForSSRF\(" middleware/proxy -S rg -n --hidden --no-ignore-vcs "joinUpstreamPath\(" middleware/proxy -S echo echo "== Show callers of validateUpstream (context) ==" rg -n "validateUpstream\(" middleware/proxy -n -S -C 3 echo echo "== Show callers of joinUpstreamPath (context) ==" rg -n "joinUpstreamPath\(" middleware/proxy -n -S -C 3Repository: gofiber/fiber
Length of output: 9410
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n --hidden --no-ignore-vcs "validateUpstream\(" middleware/proxy -S rg -n --hidden --no-ignore-vcs "joinUpstreamPath\(" middleware/proxy -S echo echo "== Callers of validateUpstream (with context) ==" rg -n "validateUpstream\(" middleware/proxy -S -C 3 echo echo "== Callers of joinUpstreamPath (with context) ==" rg -n "joinUpstreamPath\(" middleware/proxy -S -C 3Repository: gofiber/fiber
Length of output: 9239
🏁 Script executed:
#!/bin/bash set -euo pipefail file_test="middleware/proxy/security_test.go" file="middleware/proxy/security.go" echo "== Search for joinUpstreamPath tests involving base paths ==" rg -n "joinUpstreamPath\(" "$file_test" rg -n "parseUpstream\(\"http://.*\/" "$file_test" || true rg -n "parseUpstream\(\"https?://.*\/" "$file_test" || true echo echo "== Inspect WithSecurityPolicy / resolvePolicy / shallow copies ==" rg -n "WithSecurityPolicy|resolvePolicy|AllowedSchemes" "$file" echo echo "== Show surrounding code for those matches ==" rg -n "WithSecurityPolicy|resolvePolicy" "$file" -n # print relevant ranges around matches (best-effort with awk line numbers) awk 'NR>=1 && NR<=260 {print NR":"$0}' "$file" | sed -n '1,260p' >/dev/null # For precise context, just print around key line numbers if we can extract them python3 - <<'PY' import re,sys,subprocess,os file="middleware/proxy/security.go" text=open(file,'r',encoding='utf-8').read().splitlines() # find lines containing key funcs keys=["WithSecurityPolicy","resolvePolicy","AllowedSchemes"] for i,line in enumerate(text,1): if any(k in line for k in keys): print(i,line.strip()) PYRepository: gofiber/fiber
Length of output: 2572
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== security.go: show policy helpers around WithSecurityPolicy/resolvePolicy ==" sed -n '1,170p' middleware/proxy/security.go echo echo "== security.go: show joinUpstreamPath comment + function ==" sed -n '300,420p' middleware/proxy/security.go echo echo "== proxy.go: show request building around joinUpstreamPath ==" sed -n '250,460p' middleware/proxy/proxy.go echo echo "== proxy.go: search where base URL host/scheme are applied to fasthttp request ==" rg -n "URI\.|SetRequestURI|SetHost|u\.Host|u\.Hostname|Host:" middleware/proxy/proxy.go echo echo "== security_test.go: search for base path / non-empty upstream paths ==" rg -n "parseUpstream\(|joinUpstreamPath\(|http://.*\/|https://.*\/" middleware/proxy/security_test.goRepository: gofiber/fiber
Length of output: 13885
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== Inspect middleware/proxy/proxy.go around the request setup ==" sed -n '1,260p' middleware/proxy/proxy.go echo echo "== Search fasthttp client/dialer/resolver configuration ==" rg -n "Dial|Resolver|Lookup|DialContext|net\.Resolver|CustomDial|ResolveIP|Dialer" middleware/proxy/proxy.go middleware/proxy/*.go echo echo "== Inspect targetURL / SetRequestURI / request host usage ==" rg -n "targetURL|SetRequestURI\(|SetRequestURIBytes|Addr:|Hostname\(|Host:" middleware/proxy/proxy.go sed -n '200,320p' middleware/proxy/proxy.goRepository: gofiber/fiber
Length of output: 12830
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== Go toolchain (go.mod) ==" rg -n '^go [0-9]+\.[0-9]+' go.mod echo echo "== Existing strings.SplitSeq usage ==" rg -n --glob '*.go' '\bSplitSeq\s*\(' .Repository: gofiber/fiber
Length of output: 1976
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== security_test.go: show joinUpstreamPath test section ==" sed -n '300,380p' middleware/proxy/security_test.go echo echo "== security_test.go: search for parseUpstream( with non-empty path ==" rg -n "parseUpstream\\(\"https?://[^\"/]+/[^\"']*\"" middleware/proxy/security_test.go || true rg -n "parseUpstream\\(\"https?://[^\"/]+/[^\"']*\"" middleware/proxy/fuzz_test.go || trueRepository: gofiber/fiber
Length of output: 3045
🌐 Web query:
fasthttp HostClient Addr resolution at dial time DNS rebinding hostname vs IP💡 Result:
In fasthttp, DNS resolution behavior is primarily handled by the TCPDialer, which features a built-in DNS cache to improve performance [1]. Understanding how this interacts with HostClient and dial-time resolution is critical for security and reliability. DNS Resolution and Caching fasthttp's TCPDialer manages DNS resolution and includes a configurable DNSCacheDuration [1]. When a connection is requested, the dialer checks its internal tcpAddrsMap to see if the host has been recently resolved [1]. If the cached entry is expired, it performs a new DNS resolution [1]. This caching mechanism is designed to reduce latency but can introduce risks related to DNS rebinding if not managed correctly [1]. HostClient and Dialing HostClient is designed to balance requests among a list of upstream hosts provided in its Addr field [2][3]. These addresses are passed to the dialer, which performs resolution as needed [2][3]. Because HostClient manages its own connection pools and target addresses, developers must be careful when separating the Host header from the actual connection target [4]. DNS Rebinding and Security DNS rebinding occurs when an attacker controls a domain and manipulates its DNS TTL to switch the resolved IP address between an initial check (validation) and a subsequent request (execution) [5]. In the context of fasthttp: 1. Impact of Host Header: Developers often want to override the HTTP Host header for virtual hosting while keeping the connection target fixed (e.g., dialing a specific IP) [4][6]. If not handled explicitly, fasthttp may use the Host header in a way that triggers unintended DNS resolution [4]. 2. Mitigation: To prevent DNS rebinding and related SSRF vulnerabilities, it is recommended to explicitly set the connection target (IP) and use the Request.Header.SetHost method to override the Host header [7][4][6]. By decoupling the dial address from the Host header, you ensure the connection goes to a trusted, static IP while still providing the necessary header for the target server [7][4]. 3. Security Awareness: When using user-supplied URLs, always validate the hostname and consider using a custom Dial function to enforce connections to known-safe IP addresses rather than relying on dynamic DNS resolution [7][5]. Summary of Best Practices - To avoid DNS resolution issues, use a custom Dial function in your Client or HostClient to map specific hostnames to hardcoded IPs when necessary [7]. - If you need to send a specific Host header without triggering DNS for that domain, use Request.Header.SetHost and ensure your client dials the intended destination IP directly [4][6]. - Be cautious of user-supplied input in URLs, as malformed input can sometimes bypass simple validation logic and lead to unintended DNS requests [5].
Citations:
- 1: https://github.com/valyala/fasthttp/blob/master/tcpdialer.go
- 2: https://github.com/valyala/fasthttp/blob/master/client.go
- 3: https://github.com/valyala/fasthttp/blob/v1.69.0/client.go
- 4: valyala/fasthttp#2106
- 5: valyala/fasthttp#1761
- 6: valyala/fasthttp#318
- 7: valyala/fasthttp#841
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== go.mod toolchain ==" rg -n '^go [0-9]+\.[0-9]+' go.mod echo echo "== Existing strings.SplitSeq usage ==" rg -n --glob '*.go' '\bSplitSeq\s*\(' .Repository: gofiber/fiber
Length of output: 1971
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== Declared Go/toolchain files ==" fd -HI '^(go\.mod|go\.work|\.tool-versions|\.go-version)$' -x sh -c ' echo "--- $1 ---" sed -n "1,80p" "$1" ' sh {} echo echo "== Existing SplitSeq usage in the repo ==" rg -n --glob '*.go' '\bSplitSeq\s*\(' || trueRepository: gofiber/fiber
Length of output: 2871
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== go.mod toolchain ==" rg -n '^go [0-9]+\.[0-9]+' go.mod echo echo "== Existing strings.SplitSeq usage ==" rg -n --glob '*.go' '\bSplitSeq\s*\(' .Repository: gofiber/fiber
Length of output: 1971
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== go toolchain ==" rg -n '^go [0-9]+\.[0-9]+' go.mod echo echo "== strings.Split at the lint site (should become SplitSeq) ==" rg -n "connectionListedHeaders|strings\.Split\(" middleware/proxy/security.go echo echo "== joinUpstreamPath overwrites base.Path ==" rg -n "func joinUpstreamPath|out\.Path = parsed\.Path" middleware/proxy/security.go echo echo "== Dial still uses u.Host/hostname after validation ==" rg -n "validateUpstream\(|validateHostForSSRF\(|Addr:\s*u\.Host" middleware/proxy/proxy.go middleware/proxy/security.goRepository: gofiber/fiber
Length of output: 1462
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n '^go [0-9]+\.[0-9]+' go.mod rg -n --glob '*.go' '\bSplitSeq\s*\(' .Repository: gofiber/fiber
Length of output: 1909
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== go.mod toolchain ==" rg -n '^go [0-9]+\.[0-9]+' go.mod echo echo "== Existing strings.SplitSeq usage ==" rg -n --glob '*.go' '\bSplitSeq\s*\(' .Repository: gofiber/fiber
Length of output: 1971
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n '^go [0-9]+\.[0-9]+' go.mod echo rg -n --glob '*.go' '\bSplitSeq\s*\(' .Repository: gofiber/fiber
Length of output: 1910
Fix CI lint in
connectionListedHeadersand address upstream path + SSRF hardening
CI:
middleware/proxy/security.go(connectionListedHeaders, 188-190) — replacestrings.Splitwithstrings.SplitSeq.Suggested change
for _, name := range strings.SplitSeq(string(v), ",") { name = strings.TrimSpace(name) if name != "" { out = append(out, name) } }Functional regression:
joinUpstreamPath(middleware/proxy/security.go, 328-361) overwritesout.Path = parsed.Path, dropping any configuredbase.Pathprefix.Security gap:
validateHostForSSRF(middleware/proxy/security.go, 262-288) DNS-checksu.Hostname(), butfasthttp.HostClient{Addr: u.Host}(middleware/proxy/proxy.go, 40-75) dials the hostname/port—DNS can change between validation and connection (DNS rebinding/TOCTOU). Pin resolved IPs for dialing or enforce dial-time validation.🧰 Tools
🪛 GitHub Actions: Linter / 0_lint _ lint.txt
[error] 189-189: golangci-lint reported 'stringsseq: Ranging over SplitSeq is more efficient (modernize)'. Step failed in 'golangci-lint run'.
🪛 GitHub Actions: Linter / lint _ lint
[error] 189-189: golangci-lint reported issue by rule 'stringsseq': Ranging over SplitSeq is more efficient (modernize).
🪛 GitHub Check: lint / lint
[failure] 189-189:
stringsseq: Ranging over SplitSeq is more efficient (modernize)🤖 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.go` around lines 188 - 190, Replace strings.Split with strings.SplitSeq in connectionListedHeaders: in the loop over values (function connectionListedHeaders) use strings.SplitSeq(string(v), ",") and keep the existing TrimSpace + non-empty append logic to satisfy the linter. In joinUpstreamPath ensure you do not overwrite configured base.Path when setting out.Path — preserve or prepend base.Path to the parsed.Path instead of replacing it (adjust logic around out.Path = parsed.Path in joinUpstreamPath). For validateHostForSSRF, eliminate the TOCTOU by resolving the hostname to IP(s) and pinning those addresses for the subsequent dial (or perform the equivalent dial-time validation) so that the fasthttp.HostClient dial uses the verified IPs/addresses rather than re-resolving u.Host; update validateHostForSSRF and the caller in proxy.go (where fasthttp.HostClient{Addr: u.Host} is created) to accept/use resolved IPs.Sources: Linters/SAST tools, Pipeline failures
222-239:
⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftThe hostname SSRF check is still TOCTOU.
validateHostForSSRFvets one DNS answer set, but the proxy later hands the original hostname back to the transport.proxy.gothen dialsu.Host/u.String()again at Line 40, Line 96, Line 240, Line 364, and Line 409, so a rebinding resolver can return a public IP during validation and a blocked IP during connect. To actually enforce the policy, pin the vetted IPs into dialing or re-run the private-range check inside a custom dial path immediately before connect.🤖 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.go` around lines 222 - 239, validateUpstream currently validates the hostname via validateHostForSSRF but then returns the original URL (u) which proxy.go later dials using u.Host/u.String, leaving a TOCTOU hole; modify validateUpstream/consumer so that either (a) the vetted IP addresses are pinned into the dial step (provide a custom DialContext/Dial function that uses the validated IPs instead of resolving the hostname), or (b) perform the private-range/SSRF check again inside a custom dial path immediately before connect; update the code paths that call validateUpstream (and the transport in proxy.go where it dials u.Host/u.String) to use the custom dialer or IP-pinned address to ensure the same vetted IPs are used for the actual TCP connect (refer to symbols validateUpstream, validateHostForSSRF, u.Host, u.String and the dial hooks in transport/proxy).
355-357:
⚠️ Potential issue | 🟠 Major | ⚡ Quick win
joinUpstreamPathdrops the upstream’s configured path prefix.When
baseishttps://api.example.com/v1and the client requests/users, this now returnshttps://api.example.com/users, not/v1/users. Sinceproxy.goLine 364 and Line 409 routeDomainForward/BalancerForwardthrough this helper, any non-root upstream base path breaks after this change.🤖 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.go` around lines 355 - 357, joinUpstreamPath currently overwrites the upstream base path when parsed.Path is set, causing configured base paths (e.g. /v1) to be dropped; change the logic in joinUpstreamPath so that when parsed.Path is present you join it with the upstream/base path instead of replacing it (preserve leading/trailing slashes correctly and avoid duplicate slashes), and apply the same join to RawPath; refer to the joinUpstreamPath function and the parsed/ out.Path and out.RawPath assignments (used by DomainForward/BalancerForward) to locate and update the code.
…, and hop-by-hop smuggling Security audit of middleware/proxy and the redirect path in client/ identified ten findings. This change implements secure-by-default behavior for all of them. See middleware/proxy/SECURITY_AUDIT.md for the full report and severity table. New API - proxy.SecurityPolicy (AllowedSchemes, AllowPrivateIPs, AllowHTTPSDowngrade, KeepHopByHopHeaders) - proxy.DefaultSecurityPolicy, proxy.WithSecurityPolicy - proxy.Config.SecurityPolicy, MaxResponseBodySize, MaxConnsPerHost - Sentinel errors: ErrUpstreamSchemeNotAllowed, ErrUpstreamHostInvalid, ErrUpstreamHostBlocked, ErrRedirectDowngrade - client.ErrRedirectDowngrade Breaking changes (defaults) - Upstream targets resolving to loopback, RFC 1918, link-local (including 169.254.169.254), multicast, unspecified, or RFC 6598 CGNAT addresses are rejected. Set AllowPrivateIPs to opt in. - Only http/https schemes are accepted; file://, gopher://, ftp://, etc. are rejected. - DoRedirects rejects HTTPS→HTTP redirect downgrades. - RFC 7230 §6.1 hop-by-hop headers (Connection, Keep-Alive, Proxy-Authenticate, Proxy-Authorization, TE, Trailer, Transfer-Encoding, Upgrade) are stripped both ways, plus every header listed in the Connection field. - TLSConfig is cloned with MinVersion: tls.VersionTLS12 when no minimum is set; caller's struct is not mutated. - Fiber HTTP client (client.composeRedirectURL) rejects HTTPS→HTTP redirect downgrades. Bug fixes - DomainForward/BalancerForward previously concatenated `addr + c.OriginalURL()`. A leading `//` in the request path could form a network-path reference that, when re-parsed, changed the upstream host. joinUpstreamPath now rebuilds the URL safely. - Snapshot scheme/target into freshly allocated strings before SetRequestURI to fix an aliasing regression where a caller-supplied addr that was itself a slice of the request buffer got clobbered mid-request (produced "unsupported protocol ttp:" errors). Tests - middleware/proxy/security_test.go: scheme allowlist, private-IP blocking + opt-in, hop-by-hop stripping (request and response, including Connection-listed), file:// rejection, HTTPS→HTTP redirect blocking with a real TLS handshake, path-injection protection, and TLS minimum version cloning. - middleware/proxy/fuzz_test.go: FuzzValidateUpstream, FuzzJoinUpstreamPath, FuzzConnectionListedHeaders (all pass with -fuzztime=10s). - TestMain in proxy_test.go relaxes the policy for loopback tests so the rest of the suite continues to work; security_test.go installs the strict policy where it asserts the new defaults. - client/transport_test.go: TestComposeRedirectURL_RejectsHTTPSDowngrade. Docs - docs/middleware/proxy.md gains a full Security section covering every default, the SecurityPolicy fields, and migration notes for the breaking defaults. https://claude.ai/code/session_01FhTTWhze2oLocDUWAkEet2
gaby
force-pushed
the
claude/proxy-middleware-security-audit-q45Ya
branch
from
2ce39b9 to
14c5317
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
middleware/proxy/proxy.go (2)
74-87:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRe-strip hop-by-hop headers after the modify callbacks.
Line 74 strips request hop-by-hop headers before
ModifyRequest, and Line 100 strips response hop-by-hop headers beforeModifyResponse. Either hook can addConnection,Upgrade, or other hop-by-hop fields back onto the wire, which bypasses the secure-by-default behavior this PR is trying to guarantee. Run the stripping step again immediately beforelbc.Doand again afterModifyResponse(or move the existing strips to those final boundaries).Also applies to: 100-113
🤖 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 74 - 87, The current code strips hop-by-hop headers before cfg.ModifyRequest and before cfg.ModifyResponse, but ModifyRequest/ModifyResponse callbacks can reintroduce hop-by-hop headers; ensure hop-by-hop headers are stripped again immediately before performing the proxied request (before calling lbc.Do) and again immediately after running cfg.ModifyResponse (or move the existing strip calls to those final boundaries). Concretely, call stripHopByHopRequestHeaders(req, fiber.HeaderConnection) or stripHopByHopRequestHeaders(req) as appropriate right before lbc.Do, and call stripHopByHopResponseHeaders(res) right after cfg.ModifyResponse returns (or relocate the existing stripHopByHopRequestHeaders/stripHopByHopResponseHeaders calls to those spots), keeping the existing cfg.KeepConnectionHeader / policy.KeepHopByHopHeaders checks intact and referencing the same helper functions and cfg.ModifyRequest/ cfg.ModifyResponse / lbc.Do symbols.
29-46:⚠️ Potential issue | 🟠 MajorPin the vetted upstream address into the dial target (close DNS-rebinding SSRF window).
Line 32 and line 217 validate
u.Hostname()via DNS lookups, but the proxy still dials using the original hostname:
- Balancer:
fasthttp.HostClient{ Addr: u.Host }(line 40), whereu.Hostremains the input hostname.- Runtime forwarding:
req.SetRequestURI(targetURL)built fromu.String()(lines 234-235), sofasthttpre-resolves at connect time.Since
validateHostForSSRFonly checks the validation-time DNS answers (and doesn’t pin the resolved IP), DNS can change between validation and dial to blocked targets (loopback/RFC1918) and bypass the SSRF check.Use the resolved, allowed IP(s) as the actual dial destination (e.g., via
HostClient.Addrpinning or a custom dialer/TCPDialer) while preserving the original hostname for HTTPHostand TLS SNI (tls.Config.ServerName).🤖 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 29 - 46, The code validates upstreams via validateUpstream/validateHostForSSRF but still dials using the original hostname (fasthttp.HostClient{Addr: u.Host} and req.SetRequestURI(u.String())), leaving a DNS-rebinding SSRF gap; fix by pinning the resolved, allowed IP: in validateUpstream return the vetted IP (or list) and set the HostClient dialing target to that IP:port (or use a custom Dial/TCPDialer that dials the resolved IP) while preserving the original hostname for the HTTP Host header and TLS SNI (set tls.Config.ServerName via secureTLSConfig to u.Hostname()); ensure req.SetRequestURI still uses the original URI/host so Host header and SNI remain unchanged while network connection goes to the vetted IP.
🤖 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_AUDIT.md`:
- Around line 173-186: The Verification checklist in SECURITY_AUDIT.md is
missing the Markdown lint step; update the "Verification" section to include
running "make markdown" (or an equivalent "Run markdown linter" entry) alongside
the existing test/lint commands so Markdown files are linted when docs are
modified; look for the "Verification" heading and the block that lists go test
and golangci-lint commands and add the markdown lint instruction there.
In `@middleware/proxy/security_test.go`:
- Around line 182-187: The Fiber handler passed into createProxyTestServerIPv4
uses require.* assertions (e.g., require.Empty) which run inside the request
handler goroutine and can abort that goroutine instead of failing the test;
replace those require calls with plain conditional checks that call t.Errorf or
t.Fatalf (as appropriate) from within the handler (for example, check c.Get(h)
and if non-empty call t.Errorf("expected hop-by-hop %q stripped", h) or
t.Fatalf(...) ), and do the same for the "X-Custom-Hop" check; apply the same
replacement for the other handler assertions noted at lines 233-236 so only the
test goroutine uses require.* while handlers use testing.T methods.
- Around line 266-313: Test_Security_DoRedirects_BlocksDowngrade mutates
package-global policy via WithSecurityPolicy inside a parallel test; remove
t.Parallel() and instead install the temporary policy before exercising the
server using the helper withSecurityPolicyForTest (or call WithSecurityPolicy
and restore immediately) so the global change does not run in parallel or inside
the request handler; apply the same change to the other test referenced (lines
~354-366) and ensure the policy setup is done in the test body before calling
app.Test/DoRedirects so the temporary policy is active only for the test scope.
---
Outside diff comments:
In `@middleware/proxy/proxy.go`:
- Around line 74-87: The current code strips hop-by-hop headers before
cfg.ModifyRequest and before cfg.ModifyResponse, but
ModifyRequest/ModifyResponse callbacks can reintroduce hop-by-hop headers;
ensure hop-by-hop headers are stripped again immediately before performing the
proxied request (before calling lbc.Do) and again immediately after running
cfg.ModifyResponse (or move the existing strip calls to those final boundaries).
Concretely, call stripHopByHopRequestHeaders(req, fiber.HeaderConnection) or
stripHopByHopRequestHeaders(req) as appropriate right before lbc.Do, and call
stripHopByHopResponseHeaders(res) right after cfg.ModifyResponse returns (or
relocate the existing stripHopByHopRequestHeaders/stripHopByHopResponseHeaders
calls to those spots), keeping the existing cfg.KeepConnectionHeader /
policy.KeepHopByHopHeaders checks intact and referencing the same helper
functions and cfg.ModifyRequest/ cfg.ModifyResponse / lbc.Do symbols.
- Around line 29-46: The code validates upstreams via
validateUpstream/validateHostForSSRF but still dials using the original hostname
(fasthttp.HostClient{Addr: u.Host} and req.SetRequestURI(u.String())), leaving a
DNS-rebinding SSRF gap; fix by pinning the resolved, allowed IP: in
validateUpstream return the vetted IP (or list) and set the HostClient dialing
target to that IP:port (or use a custom Dial/TCPDialer that dials the resolved
IP) while preserving the original hostname for the HTTP Host header and TLS SNI
(set tls.Config.ServerName via secureTLSConfig to u.Hostname()); ensure
req.SetRequestURI still uses the original URI/host so Host header and SNI remain
unchanged while network connection goes to the vetted IP.
🪄 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: 89555a1b-442b-4b8a-96e1-ed610259598c
📒 Files selected for processing (12)
client/errors.goclient/transport.goclient/transport_test.godocs/middleware/proxy.mdmiddleware/proxy/SECURITY_AUDIT.mdmiddleware/proxy/config.gomiddleware/proxy/fuzz_test.gomiddleware/proxy/proxy.gomiddleware/proxy/proxy_test.gomiddleware/proxy/security.gomiddleware/proxy/security_branches_test.gomiddleware/proxy/security_test.go
✅ Files skipped from review due to trivial changes (1)
- client/errors.go
🚧 Files skipped from review as they are similar to previous changes (6)
- middleware/proxy/config.go
- client/transport.go
- client/transport_test.go
- middleware/proxy/proxy_test.go
- middleware/proxy/fuzz_test.go
- middleware/proxy/security.go
| ## Verification | ||
|
|
||
| ```sh | ||
| # Run the proxy and client test suites | ||
| go test ./middleware/proxy/ ./client/ -count=1 -skip IPv6 | ||
|
|
||
| # Run the fuzz seeds and a short fuzz session | ||
| go test ./middleware/proxy/ -run 'Fuzz' -count=1 | ||
| go test ./middleware/proxy/ -run '^$' -fuzz FuzzValidateUpstream -fuzztime=10s | ||
| go test ./middleware/proxy/ -run '^$' -fuzz FuzzJoinUpstreamPath -fuzztime=10s | ||
|
|
||
| # Linting | ||
| golangci-lint run ./middleware/proxy/... ./client/... | ||
| ``` |
There was a problem hiding this comment.
Add markdown linting to the verification checklist.
The Verification section should include make markdown since this PR modifies Markdown docs.
As per coding guidelines, “**/*.md: Run make markdown to lint all Markdown files when modifying code.”
🤖 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_AUDIT.md` around lines 173 - 186, The Verification
checklist in SECURITY_AUDIT.md is missing the Markdown lint step; update the
"Verification" section to include running "make markdown" (or an equivalent "Run
markdown linter" entry) alongside the existing test/lint commands so Markdown
files are linted when docs are modified; look for the "Verification" heading and
the block that lists go test and golangci-lint commands and add the markdown
lint instruction there.
Source: Coding guidelines
| _, addr := createProxyTestServerIPv4(t, func(c fiber.Ctx) error { | ||
| for _, h := range hops { | ||
| require.Empty(t, c.Get(h), "expected hop-by-hop %q stripped", h) | ||
| } | ||
| require.Empty(t, c.Get("X-Custom-Hop"), "Connection-listed header should be stripped") | ||
| return c.SendString("ok") |
There was a problem hiding this comment.
Avoid require inside Fiber handlers.
These assertions run from request-handler goroutines, so require.* can abort the goroutine instead of failing the test cleanly. Use plain if checks with t.Errorf/t.Fatalf in the handler and keep require in the test goroutine. Based on learnings, "In the Fiber codebase, the linter does not allow require assertions from within HTTP handlers (including net/http-style handlers). Use t.Fatalf, t.Errorf, or similar testing.T methods for error handling inside handler functions instead."
Also applies to: 233-236
🤖 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_test.go` around lines 182 - 187, The Fiber handler
passed into createProxyTestServerIPv4 uses require.* assertions (e.g.,
require.Empty) which run inside the request handler goroutine and can abort that
goroutine instead of failing the test; replace those require calls with plain
conditional checks that call t.Errorf or t.Fatalf (as appropriate) from within
the handler (for example, check c.Get(h) and if non-empty call
t.Errorf("expected hop-by-hop %q stripped", h) or t.Fatalf(...) ), and do the
same for the "X-Custom-Hop" check; apply the same replacement for the other
handler assertions noted at lines 233-236 so only the test goroutine uses
require.* while handlers use testing.T methods.
Source: Learnings
| func Test_Security_DoRedirects_BlocksDowngrade(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| // Start an HTTPS server that 302-redirects to a plaintext HTTP URL. | ||
| cert, err := generateLocalhostCert(t) | ||
| require.NoError(t, err) | ||
|
|
||
| httpsLn, err := tls.Listen(fiber.NetworkTCP4, "127.0.0.1:0", &tls.Config{Certificates: []tls.Certificate{cert}, MinVersion: tls.VersionTLS12}) | ||
| require.NoError(t, err) | ||
| t.Cleanup(func() { httpsLn.Close() }) //nolint:errcheck // best effort | ||
|
|
||
| plaintextLn, err := net.Listen(fiber.NetworkTCP4, "127.0.0.1:0") | ||
| require.NoError(t, err) | ||
| t.Cleanup(func() { plaintextLn.Close() }) //nolint:errcheck // best effort | ||
|
|
||
| plaintextAddr := plaintextLn.Addr().String() | ||
|
|
||
| httpsApp := fiber.New() | ||
| httpsApp.Get("/", func(c fiber.Ctx) error { | ||
| c.Location("http://" + plaintextAddr + "/secret") | ||
| return c.SendStatus(fiber.StatusFound) | ||
| }) | ||
| startServer(httpsApp, httpsLn) | ||
|
|
||
| plainApp := fiber.New() | ||
| plainApp.Get("/secret", func(c fiber.Ctx) error { | ||
| return c.SendString("LEAKED") | ||
| }) | ||
| startServer(plainApp, plaintextLn) | ||
|
|
||
| // Use a custom proxy client that trusts the self-signed cert; otherwise | ||
| // the TLS dial fails before we get to evaluate the redirect. | ||
| tlsClient := &fasthttp.Client{ | ||
| NoDefaultUserAgentHeader: true, | ||
| DisablePathNormalizing: true, | ||
| TLSConfig: &tls.Config{InsecureSkipVerify: true, MinVersion: tls.VersionTLS12}, | ||
| } | ||
|
|
||
| app := fiber.New() | ||
| app.Get("/test", func(c fiber.Ctx) error { | ||
| // Allow private IPs for the test loopback servers but keep the | ||
| // downgrade protection enabled. | ||
| policy := DefaultSecurityPolicy() | ||
| policy.AllowPrivateIPs = true | ||
| prev := WithSecurityPolicy(policy) | ||
| defer WithSecurityPolicy(prev) | ||
| return DoRedirects(c, "https://"+httpsLn.Addr().String(), 1, tlsClient) | ||
| }) |
There was a problem hiding this comment.
Keep the global-policy tests serial.
WithSecurityPolicy mutates package-global state. Running these tests with t.Parallel() lets their temporary policy bleed into other proxy tests that call currentSecurityPolicy()/Do/Balancer at runtime, and Test_Security_DoRedirects_BlocksDowngrade makes that worse by flipping the global inside the handler itself. Please keep both tests non-parallel and install the temporary policy in the test body with withSecurityPolicyForTest before app.Test.
Also applies to: 354-366
🧰 Tools
🪛 OpenGrep (1.22.0)
[ERROR] 301-301: TLS certificate verification is disabled via InsecureSkipVerify. This allows man-in-the-middle attacks. Remove InsecureSkipVerify or set it to false.
(coderabbit.tls.go-insecure-skip-verify)
[ERROR] 301-301: TLS certificate verification is disabled via InsecureSkipVerify. This allows man-in-the-middle attacks. Remove InsecureSkipVerify or set it to false.
(coderabbit.tls.go-insecure-skip-verify)
🤖 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_test.go` around lines 266 - 313,
Test_Security_DoRedirects_BlocksDowngrade mutates package-global policy via
WithSecurityPolicy inside a parallel test; remove t.Parallel() and instead
install the temporary policy before exercising the server using the helper
withSecurityPolicyForTest (or call WithSecurityPolicy and restore immediately)
so the global change does not run in parallel or inside the request handler;
apply the same change to the other test referenced (lines ~354-366) and ensure
the policy setup is done in the test body before calling app.Test/DoRedirects so
the temporary policy is active only for the test scope.
4 participants
Security audit of middleware/proxy and the redirect path in client/
identified ten findings. This change implements secure-by-default
behavior for all of them. See middleware/proxy/SECURITY_AUDIT.md for
the full report and severity table.
New API
AllowHTTPSDowngrade, KeepHopByHopHeaders)
ErrUpstreamHostBlocked, ErrRedirectDowngrade
Breaking changes (defaults)
(including 169.254.169.254), multicast, unspecified, or RFC 6598
CGNAT addresses are rejected. Set AllowPrivateIPs to opt in.
etc. are rejected.
Proxy-Authenticate, Proxy-Authorization, TE, Trailer,
Transfer-Encoding, Upgrade) are stripped both ways, plus every
header listed in the Connection field.
minimum is set; caller's struct is not mutated.
redirect downgrades.
Bug fixes
addr + c.OriginalURL(). A leading//in the request path could form anetwork-path reference that, when re-parsed, changed the upstream
host. joinUpstreamPath now rebuilds the URL safely.
SetRequestURI to fix an aliasing regression where a caller-supplied
addr that was itself a slice of the request buffer got clobbered
mid-request (produced "unsupported protocol ttp:" errors).
Tests
blocking + opt-in, hop-by-hop stripping (request and response,
including Connection-listed), file:// rejection, HTTPS→HTTP
redirect blocking with a real TLS handshake, path-injection
protection, and TLS minimum version cloning.
FuzzJoinUpstreamPath, FuzzConnectionListedHeaders (all pass with
-fuzztime=10s).
the rest of the suite continues to work; security_test.go installs
the strict policy where it asserts the new defaults.
Docs
every default, the SecurityPolicy fields, and migration notes for
the breaking defaults.
https://claude.ai/code/session_01FhTTWhze2oLocDUWAkEet2