🐛 bug: fix Unix-socket support in IsProxyTrusted#4088
Conversation
Summary of ChangesHello @gaby, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a potential runtime panic in the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
No actionable comments were generated in the recent review. 🎉 WalkthroughAdds Unix-domain socket trust support: new Changes
Sequence DiagramsequenceDiagram
participant TestRunner as Test Runner
participant Client as Unix Socket Client
participant App as Fiber App
participant Handler as /ip Handler
TestRunner->>App: create app (TrustProxy=true, TrustProxyConfig{UnixSocket/Loopback})
TestRunner->>Client: connect to unix socket and send HTTP request with X-Forwarded-For
Client->>App: deliver request (RemoteAddr = net.UnixAddr)
App->>Handler: invoke /ip handler
Handler->>Handler: call IsProxyTrusted() (type-switch on RemoteAddr)
alt UnixAddr path
Handler->>Handler: consult TrustProxyConfig.UnixSocket and extract IP from header
else non-Unix path
Handler->>Handler: run TCP/UDP IP checks (loopback/private/CIDR)
end
Handler-->>TestRunner: return "IsProxyTrusted|IP" response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4088 +/- ##
=======================================
Coverage 91.01% 91.01%
=======================================
Files 119 119
Lines 11302 11320 +18
=======================================
+ Hits 10286 10303 +17
- Misses 643 644 +1
Partials 373 373
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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 effectively addresses a potential panic in IsProxyTrusted by adding a nil check for the remote IP. The introduction of explicit handling for Unix sockets, tying their trust status to the Loopback configuration, is a logical improvement. The accompanying test using a real Unix socket is a great addition that validates the new behavior and improves test coverage. I have one suggestion to make the new test more robust by replacing the fixed sleep with a retry mechanism.
There was a problem hiding this comment.
Pull request overview
This PR prevents runtime panics in the proxy trust determination logic by adding defensive nil checks and explicit Unix socket handling. It introduces Unix socket-aware proxy trust semantics controlled by the TrustProxyConfig.Loopback setting and adds test coverage for Unix socket scenarios.
Changes:
- Added nil guard for
RemoteIP()inIsProxyTrusted()to prevent panics - Implemented Unix socket detection using type switch on
RemoteAddr()with trust controlled byLoopbackconfig - Added comprehensive Unix socket test with both enabled and disabled loopback scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| req.go | Added type switch to detect Unix sockets and return trust based on Loopback config; added nil check for RemoteIP() to prevent panic |
| ctx_test.go | Added Test_Ctx_ProxyTrust_UnixRemoteAddr test and helper function to verify Unix socket proxy trust behavior in real runtime scenarios |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@ctx_test.go`:
- Around line 3012-3036: The fixed 300ms sleep in the goroutine makes the
unix-socket test flaky; replace the sleep with a readiness probe that retries
dialing the unix socket (sock) before creating the fasthttp.HostClient and
calling client.Do. Implement a short loop with a total timeout (e.g., 1s) that
repeatedly attempts net.Dial or net.DialTimeout (using NetworkUnix) and breaks
when a connection succeeds (closing the probe conn immediately), returning a
failure via errCh if the probe times out; then proceed to construct the
HostClient, perform client.Do(req, resp) and send results/errors as before
(preserve result, errCh, and app.Shutdown usage).
|
@copilot open a new pull request to apply changes based on the comments in this thread |
…est race condition Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
🐛 bug: address PR review comments for Unix-socket proxy trust
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@req.go`:
- Around line 1120-1125: The IsFromLocal method currently returns false for Unix
socket connections because it only checks ip.IsLoopback() on
r.c.fasthttp.RemoteIP(); keep the nil guard, but add an explicit type assertion
for *net.UnixAddr (as done in IsProxyTrusted()) and return true for Unix domain
sockets; in DefaultReq.IsFromLocal, check if remote := r.c.fasthttp.RemoteIP();
if remote == nil return false, else if _, ok := remote.(*net.UnixAddr); ok
return true, otherwise return remote.IsLoopback().
🧹 Nitpick comments (1)
req.go (1)
1084-1098: Core Unix-socket trust logic looks correct.The type switch cleanly separates address families: Unix sockets are controlled by the new
UnixSocketflag, TCP/UDP falls through to existing IP checks, and unknown types default to untrusted. The nil guard onRemoteIP()(line 1096) is good defensive coding even though TCP/UDP addresses should always yield a non-nil IP.One minor note:
RemoteAddr()is called on line 1084 andRemoteIP()is called again on line 1095. You could avoid the second virtual method call by extracting the IP directly from the type assertion:♻️ Optional: extract IP from the type switch to avoid a second call
remoteAddr := r.c.fasthttp.RemoteAddr() - switch remoteAddr.(type) { + var ip net.IP + switch addr := remoteAddr.(type) { case *net.UnixAddr: return config.TrustProxyConfig.UnixSocket - case *net.TCPAddr, *net.UDPAddr: - // Keep existing RemoteIP/IP-map/CIDR checks for TCP/UDP paths as-is. + case *net.TCPAddr: + ip = addr.IP + case *net.UDPAddr: + ip = addr.IP default: // Unknown address type: do not trust by default. return false } - ip := r.c.fasthttp.RemoteIP() if ip == nil { return false }
Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
Co-authored-by: gaby <835733+gaby@users.noreply.github.com>
🐛 bug: Handle Unix sockets explicitly in IsFromLocal()
This pull request addresses a potential runtime panic in the IsProxyTrusted function by adding a nil check for remote IPs and refines proxy trust logic to correctly handle Unix socket connections. It ensures that Unix socket remotes are explicitly controlled by loopback trust settings, preventing unexpected behavior and enhancing the robustness of proxy trust evaluations without impacting existing TCP/UDP remote handling
Fixes #4086