feat: bypass proxy for private/LAN IPs in management api-call#1960
feat: bypass proxy for private/LAN IPs in management api-call#1960thebtf wants to merge 1 commit intorouter-for-me:mainfrom
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Summary of ChangesHello, 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 enhances network request handling by introducing intelligent proxy bypass logic. It addresses a critical usability issue where the management API's api-call handler, when a global proxy is active, would incorrectly route requests to internal network resources through the proxy, leading to connection timeouts. By detecting private and local IP ranges, the system can now correctly route these requests directly, ensuring seamless interaction with LAN-hosted services like model providers in the Management UI. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
![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 introduces a feature to bypass the configured proxy for requests to private/LAN IP addresses in the management API call handler. The security review agent noted that no specific changes were provided for review, preventing a detailed security critique. The implementation correctly modifies the apiCallTransport function to check the target host and conditionally skip proxying, and the changes in refreshGeminiOAuthAccessToken and refreshAntigravityOAuthAccessToken ensure external OAuth requests continue to use the proxy. A suggestion has been made to improve the efficiency and robustness of the new isNoProxyHost function by pre-parsing the CIDR ranges at startup.
| var defaultNoProxyCIDRs = []string{ | ||
| "127.0.0.0/8", | ||
| "10.0.0.0/8", | ||
| "172.16.0.0/12", | ||
| "192.168.0.0/16", | ||
| "169.254.0.0/16", | ||
| "::1/128", | ||
| "fc00::/7", | ||
| "fe80::/10", | ||
| } | ||
|
|
||
| // isNoProxyHost returns true when targetHost should bypass the proxy. | ||
| func isNoProxyHost(targetHost string) bool { | ||
| host := targetHost | ||
| if h, _, err := net.SplitHostPort(host); err == nil { | ||
| host = h | ||
| } | ||
| ip := net.ParseIP(host) | ||
| if ip == nil { | ||
| return false | ||
| } | ||
| for _, cidr := range defaultNoProxyCIDRs { | ||
| _, network, err := net.ParseCIDR(cidr) | ||
| if err != nil { | ||
| continue | ||
| } | ||
| if network.Contains(ip) { | ||
| return true | ||
| } | ||
| } | ||
| if h != nil && h.cfg != nil { | ||
| if proxyStr := strings.TrimSpace(h.cfg.ProxyURL); proxyStr != "" { | ||
| proxyCandidates = append(proxyCandidates, proxyStr) | ||
| return false | ||
| } |
There was a problem hiding this comment.
The current implementation of isNoProxyHost parses the hardcoded CIDR strings on every function call. This is inefficient as it can be done once at application startup.
Additionally, if one of the hardcoded CIDRs is invalid, net.ParseCIDR will return an error which is silently ignored with continue. For hardcoded values, it's better to fail fast at startup if they are invalid.
I suggest pre-parsing the CIDRs into a slice of *net.IPNet during initialization. This improves performance and makes the logic more robust.
var defaultNoProxyCIDRs = []string{
"127.0.0.0/8",
"10.0.0.0/8",
"172.16.0.0/12",
"192.168.0.0/16",
"169.254.0.0/16",
"::1/128",
"fc00::/7",
"fe80::/10",
}
var parsedNoProxyCIDRs = func() []*net.IPNet {
cidrs := make([]*net.IPNet, 0, len(defaultNoProxyCIDRs))
for _, cidr := range defaultNoProxyCIDRs {
_, network, err := net.ParseCIDR(cidr)
if err != nil {
// Panic on startup if hardcoded CIDRs are invalid.
panic(fmt.Sprintf("invalid hardcoded CIDR in defaultNoProxyCIDRs: %q: %v", cidr, err))
}
cidrs = append(cidrs, network)
}
return cidrs
}()
// isNoProxyHost returns true when targetHost should bypass the proxy.
func isNoProxyHost(targetHost string) bool {
if targetHost == "" {
return false
}
host := targetHost
if h, _, err := net.SplitHostPort(host); err == nil {
host = h
}
ip := net.ParseIP(host)
if ip == nil {
return false
}
for _, network := range parsedNoProxyCIDRs {
if network.Contains(ip) {
return true
}
}
return false
}
xkonjin
left a comment
xkonjin
left a comment
There was a problem hiding this comment.
Code Review
Overall: ✅ Approvable — correct approach, one robustness suggestion.
What's good
- Bypassing proxy for RFC 1918 / loopback / link-local addresses is the right fix. These ranges are unreachable via most external proxies and routing them through one silently fails.
- Passing
targetHostexplicitly rather than re-parsing inside the transport is clean. - Correctly passing
""for OAuth refresh calls (so they always use proxy — OAuth endpoints are always external). - IPv6 ranges (::1/128, fc00::/7, fe80::/10) included alongside IPv4 — thorough.
Issue: CIDR parsing on every request
isNoProxyHost calls net.ParseCIDR inside a loop on every invocation. With 8 CIDR strings this is fast but unnecessary work. Pre-parse defaultNoProxyCIDRs to []*net.IPNet at init time (package-level var with init() or a sync.Once) and just iterate the pre-parsed networks. This is what gemini-code-assist flagged and it's worth doing.
Minor notes
isNoProxyHostonly matches IP addresses — hostname-based LAN targets (e.g.mymachine.local,raspberrypi) returnfalseand will be proxied. This is probably acceptable for now but worth documenting as a known limitation in a comment.- No test for
isNoProxyHostitself. Given it's a security boundary (bypasses proxy), a small table-driven test covering private IPs, public IPs, IPv6, and hostnames would be valuable.
xkonjin
left a comment
xkonjin
left a comment
There was a problem hiding this comment.
Nice direction, but there is still a gap in the no-proxy detection that will break common LAN setups.
isNoProxyHost() only bypasses the proxy when targetHost is already a literal IP (net.ParseIP(host) succeeds). For typical management traffic to local devices, the target is often a hostname that resolves to a private address (nas.local, router.lan, mDNS names, internal DNS names, etc.). In those cases this PR still sends the request through the proxy, which is exactly the path this change is trying to avoid.
Please either:
- resolve hostnames before the CIDR check, or
- document and test that only literal IP targets are supported here.
I’d strongly prefer the former, plus a regression test covering a hostname that resolves to RFC1918 space.
luispater
left a comment
luispater
left a comment
There was a problem hiding this comment.
Summary:
The overall direction makes sense, but there is still one correctness gap before this is safe to merge.
Key findings:
- Blocking:
isNoProxyHost()only recognizes literal IPs. Requests tohttp://localhost:...still go through the configured proxy becausenet.ParseIP("localhost")returns nil, and bracketed IPv6 loopback URLs such ashttp://[::1]/...are missed as well. That means a common local-provider setup is still broken even though the PR claims to bypass loopback/LAN targets. - Non-blocking: there is no regression coverage for the new bypass logic, so this kind of hole is easy to miss in CI.
Test plan:
- go test ./...
Follow-up:
- Please normalize the host with URL hostname semantics (or explicitly special-case
localhost) and add regression coverage forlocalhost, literal private IPv4, and IPv6 loopback.
thebtf
force-pushed
the
fix/no-proxy-private-ips
branch
from
76210ef to
2c226dd
Compare
Management UI "Pick Models" requests to LAN targets (e.g. LM Studio
at 172.20.23.32) timed out because apiCallTransport unconditionally
routed every call through the global proxy, and most proxies cannot
reach the local network.
Add isNoProxyHost() which detects RFC 1918 / loopback / link-local
targets and bypasses the proxy for them. The helper accepts:
- literal IPv4 / IPv6 (with or without port)
- bracketed IPv6 ("[::1]", "[::1]:443")
- zone-suffixed IPv6 ("fe80::1%eth0")
- "localhost" as a universal loopback alias
- arbitrary hostnames — resolved via the default resolver with a
500ms timeout; every resolved address must fall inside the
no-proxy ranges, so a single public address disables the bypass
and LAN DNS poisoning cannot hijack external traffic
- unresolvable hostnames — bypass disabled (proxy retains a chance
to reach the target)
Defaults: 127.0.0.0/8, 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16,
169.254.0.0/16, ::1/128, fc00::/7, fe80::/10. Overridable via
`no-proxy-cidrs` in config.yaml (full replacement, not append).
apiCallTransport now takes an explicit targetHost argument. The
Pick-Models path passes the parsed URL host; callers that do not
know the target yet pass "" and retain the previous always-proxy
behaviour.
Includes regression tests for private/public IPv4, IPv6 loopback,
link-local with zone, bracketed forms, localhost, and LAN-vs.-public
bypass via the transport.
Upstream PR: router-for-me#1960
|
@luispater @xkonjin thanks for the review — both gaps are now fixed, force-pushed to FixedHostname resolution —
IPv6 edge cases:
Also fixed: the initial commit added Merge with the new API-key proxy resolutionRebased on top of
Regression tests
Existing tests for the API-key candidate list continue to pass and call the new two-arg signature with |
thebtf
force-pushed
the
fix/no-proxy-private-ips
branch
from
2c226dd to
8eb3cb6
Compare
Management UI "Pick Models" requests to LAN targets (e.g. LM Studio
at 172.20.23.32) timed out because apiCallTransport unconditionally
routed every call through the global proxy, and most proxies cannot
reach the local network.
Add isNoProxyHost() which detects RFC 1918 / loopback / link-local
targets and bypasses the proxy for them. The helper accepts:
- literal IPv4 / IPv6 (with or without port)
- bracketed IPv6 ("[::1]", "[::1]:443")
- zone-suffixed IPv6 ("fe80::1%eth0")
- "localhost" as a universal loopback alias
- arbitrary hostnames — resolved via the default resolver with a
500ms timeout; every resolved address must fall inside the
no-proxy ranges, so a single public address disables the bypass
and LAN DNS poisoning cannot hijack external traffic
- unresolvable hostnames — bypass disabled (proxy retains a chance
to reach the target)
Defaults: 127.0.0.0/8, 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16,
169.254.0.0/16, ::1/128, fc00::/7, fe80::/10. Overridable via
`no-proxy-cidrs` in config.yaml (full replacement, not append).
apiCallTransport now takes an explicit targetHost argument. The
Pick-Models path passes the parsed URL host; callers that do not
know the target yet pass "" and retain the previous always-proxy
behaviour.
Includes regression tests for private/public IPv4, IPv6 loopback,
link-local with zone, bracketed forms, localhost, and LAN-vs.-public
bypass via the transport.
Upstream PR: router-for-me#1960
Management UI "Pick Models" requests to LAN targets (e.g. LM Studio at
172.20.23.32) timed out because apiCallTransport unconditionally routed
every call through the global proxy, and most proxies cannot reach the
local network.
Add isNoProxyHost() which detects RFC 1918 / loopback / link-local
targets and bypasses the proxy for them. The helper accepts:
- literal IPv4 / IPv6 (with or without port)
- bracketed IPv6 ("[::1]", "[::1]:443")
- zone-suffixed IPv6 ("fe80::1%eth0")
- "localhost" as a universal loopback alias
- arbitrary hostnames — resolved via the default resolver with a
500ms timeout; every resolved address must fall inside the
no-proxy ranges, so a single public address disables the bypass
and LAN DNS poisoning cannot hijack external traffic
- unresolvable hostnames — bypass disabled (proxy retains a chance
to reach the target)
Defaults: 127.0.0.0/8, 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16,
169.254.0.0/16, ::1/128, fc00::/7, fe80::/10. Overridable via
`no-proxy-cidrs` in config.yaml (full replacement, not append).
apiCallTransport now takes an explicit targetHost argument. The
Pick-Models path passes the parsed URL host; callers that do not
know the target yet (OAuth token refresh to Google endpoints) pass
"" and retain the previous always-proxy behaviour.
Regression tests cover private/public IPv4, IPv6 loopback, link-local
with zone, bracketed forms, localhost variants, and LAN-vs.-public
bypass through the transport.
thebtf
force-pushed
the
fix/no-proxy-private-ips
branch
from
8eb3cb6 to
96780b2
Compare
Management UI "Pick Models" requests to LAN targets (e.g. LM Studio
at 172.20.23.32) timed out because apiCallTransport unconditionally
routed every call through the global proxy, and most proxies cannot
reach the local network.
Add isNoProxyHost() which detects RFC 1918 / loopback / link-local
targets and bypasses the proxy for them. The helper accepts:
- literal IPv4 / IPv6 (with or without port)
- bracketed IPv6 ("[::1]", "[::1]:443")
- zone-suffixed IPv6 ("fe80::1%eth0")
- "localhost" as a universal loopback alias
- arbitrary hostnames — resolved via the default resolver with a
500ms timeout; every resolved address must fall inside the
no-proxy ranges, so a single public address disables the bypass
and LAN DNS poisoning cannot hijack external traffic
- unresolvable hostnames — bypass disabled (proxy retains a chance
to reach the target)
Defaults: 127.0.0.0/8, 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16,
169.254.0.0/16, ::1/128, fc00::/7, fe80::/10. Overridable via
`no-proxy-cidrs` in config.yaml (full replacement, not append).
apiCallTransport now takes an explicit targetHost argument. The
Pick-Models path passes the parsed URL host; callers that do not
know the target yet pass "" and retain the previous always-proxy
behaviour.
Includes regression tests for private/public IPv4, IPv6 loopback,
link-local with zone, bracketed forms, localhost, and LAN-vs.-public
bypass via the transport.
Upstream PR: router-for-me#1960
3 participants
Summary
api-callhandler routes all requests through it — including requests to LAN targets (e.g. LM Studio at172.x.x.x). Since proxies typically cannot reach private networks, these requests time out.isNoProxyHost()that detects RFC 1918, loopback, and link-local addresses and skips the proxy for them.Default bypass ranges
127.0.0.0/8,10.0.0.0/8,172.16.0.0/12,192.168.0.0/16,169.254.0.0/16,::1/128,fc00::/7,fe80::/10Files changed
internal/api/handlers/management/api_tools.goTest plan
http://192.168.1.100:1234/v1/models)