feat: url validation - #3947
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCentralizes localhost/private-IP checks in core/network, updates ValidateExternalURL to use them, hardens ConfigureDialer to resolve and filter private addresses before connecting, adds unit tests for classifications and dialer SSRF behavior, and validates provider BaseURL inputs in handlers. ChangesSSRF Protection and DNS Rebinding Mitigation
Sequence DiagramsequenceDiagram
participant HTTPHandler
participant ValidateExternalURL
participant network_IsLocalhost
participant DNSResolver
participant network_IsPrivateIP
participant ConfigureDialer
HTTPHandler->>ValidateExternalURL: BaseURL
ValidateExternalURL->>network_IsLocalhost: check hostname
alt hostname is localhost
ValidateExternalURL-->>HTTPHandler: error
else
ValidateExternalURL->>DNSResolver: resolve hostname
DNSResolver-->>ValidateExternalURL: []IP
loop for each resolved IP
ValidateExternalURL->>network_IsPrivateIP: IP
alt IP is private
ValidateExternalURL-->>HTTPHandler: error
end
end
ValidateExternalURL-->>HTTPHandler: OK
end
HTTPHandler->>ConfigureDialer: request to connect
ConfigureDialer->>DNSResolver: resolve host (bounded)
DNSResolver-->>ConfigureDialer: []IP
ConfigureDialer->>network_IsPrivateIP: filter IPs
ConfigureDialer->>ConfigureDialer: dial first allowed IP
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
|
tejas ghatte seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
TejasGhatte
marked this pull request as ready for review
June 1, 2026 09:29
Contributor
Confidence Score: 5/5Safe to merge; the DNS-at-dial-time approach correctly eliminates the rebinding window, and all three changed layers (network utility, dialer, HTTP handlers) are consistent with each other. The core security fix is sound: dialing IP literals after per-connection DNS resolution removes the TOCTOU gap. The private-IP check is interleaved with the retry loop but safely — a private IP in any position in the DNS response is rejected before its dial attempt, or never reached because a prior public IP already connected. CIDRs are parsed once at init. Tests cover the key attack vectors. No correctness regressions were identified. No files require special attention. Important Files Changed
Reviews (5): Last reviewed commit: "feat: url validation" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/providers/utils/utils.go (1)
273-311:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftProxied and custom dialers still bypass the rebinding guard.
This protection only runs in the
defaultbranch. IfConfigureProxyor any other caller has already setclient.Dial/client.DialTimeout, we immediately delegate to that dialer with the original hostname, so DNS can still rebind betweenValidateExternalURLand connect time for those deployments.🤖 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 `@core/providers/utils/utils.go` around lines 273 - 311, ExistingDial and existingDialTimeout branches bypass the DNS-rebinding guard; change those branches so they first SplitHostPort(addr), LookupIP(host), reject network.IsLocalhost(host) and any network.IsPrivateIP(ip), pick a non-private ip and construct dialAddr via net.JoinHostPort(ip.String(), port), then call existingDial(dialAddr) or existingDialTimeout(dialAddr, client.ReadTimeout) instead of passing the original hostname; reuse the same validation/resolution logic used in the default branch (net.SplitHostPort, net.LookupIP, network.IsLocalhost, network.IsPrivateIP, net.JoinHostPort) so client.Dial / client.DialTimeout cannot be used to bypass the rebinding checks.
🤖 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 `@core/providers/utils/utils.go`:
- Around line 295-310: The current logic builds a single dialAddr from the first
non-private IP and dials it, which breaks fallback across multiple A/AAAA
records; change it to try each resolved public IP in order: loop over ips, skip
private ones using network.IsPrivateIP(ip), for each build dialAddr =
net.JoinHostPort(ip.String(), port) and attempt dialing with
(&net.Dialer{Timeout: client.ReadTimeout, KeepAliveConfig:
keepAliveCfg}).Dial("tcp", dialAddr) until a dial succeeds, returning the
successful conn; if all attempts fail return a consolidated error indicating
none of the resolved addresses for host succeeded (include last error or
aggregate errors) so callers know all attempts failed.
---
Outside diff comments:
In `@core/providers/utils/utils.go`:
- Around line 273-311: ExistingDial and existingDialTimeout branches bypass the
DNS-rebinding guard; change those branches so they first SplitHostPort(addr),
LookupIP(host), reject network.IsLocalhost(host) and any
network.IsPrivateIP(ip), pick a non-private ip and construct dialAddr via
net.JoinHostPort(ip.String(), port), then call existingDial(dialAddr) or
existingDialTimeout(dialAddr, client.ReadTimeout) instead of passing the
original hostname; reuse the same validation/resolution logic used in the
default branch (net.SplitHostPort, net.LookupIP, network.IsLocalhost,
network.IsPrivateIP, net.JoinHostPort) so client.Dial / client.DialTimeout
cannot be used to bypass the rebinding checks.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c57da3c1-bc51-4b00-afe1-0c21ba10ea90
📒 Files selected for processing (5)
core/network/utils.gocore/providers/utils/utils.gocore/utils.gocore/utils_test.gotransports/bifrost-http/handlers/providers.go
TejasGhatte
force-pushed
the
06-01-feat_url_validation
branch
from
June 1, 2026 11:15
b16b486 to
95d38fb
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/providers/utils/utils.go (1)
274-280:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftProxy/custom dialers bypass SSRF host/IP validation in ConfigureDialer.
ConfigureDialeronly applies the localhost/private-IP checks in thedefaultbranch; whenclient.Dialorclient.DialTimeoutis already set (e.g., byConfigureProxy), it forwards directly to the existing dialer without performing any of this validation, reintroducing the SSRF/rebinding gap.🤖 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 `@core/providers/utils/utils.go` around lines 274 - 280, The existingDial and existingDialTimeout branches in ConfigureDialer skip the SSRF/private-host validation, so wrap calls to any preexisting client.Dial or client.DialTimeout (symbols: existingDial, existingDialTimeout, client.Dial, client.DialTimeout, ConfigureProxy, ConfigureDialer) with the same host/IP validation used in the default branch before invoking the underlying dialer; preserve behavior like keepalive and timeout (use client.ReadTimeout for existingDialTimeout) and return the validation error if the host is disallowed. Ensure you perform the validation at the start of the case handlers and only call existingDial/existingDialTimeout when validation passes.
🤖 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 `@core/providers/utils/utils.go`:
- Around line 291-297: Replace the blocking net.LookupIP(host) call in
ConfigureDialer with a context-bounded resolver call: create a
context.WithTimeout using the appropriate timeout (client.ReadTimeout or
client.DialTimeout as the review suggests), call (&net.Resolver{}).LookupIP(ctx,
"ip", host), cancel the context after the lookup, and handle/return the lookup
error; do this lookup before constructing the net.Dialer so DNS resolution
cannot exceed the configured timeout.
In `@core/utils_test.go`:
- Around line 18-38: The test uses real DNS names (api.openai.com) which makes
ValidateExternalURL tests flaky; update code/tests to avoid live DNS by stubbing
the host lookup: modify ValidateExternalURL (or add a package-level lookup hook
used by it) to accept/invoke a lookup function (e.g., lookupHost or Resolver
hook) and then in core/utils_test.go replace the table cases to use that
injected stubbed lookup returning deterministic results for the test hosts (or
use localhost names) so the table-driven cases (the entries referencing
api.openai.com) no longer perform real network/DNS resolution.
---
Outside diff comments:
In `@core/providers/utils/utils.go`:
- Around line 274-280: The existingDial and existingDialTimeout branches in
ConfigureDialer skip the SSRF/private-host validation, so wrap calls to any
preexisting client.Dial or client.DialTimeout (symbols: existingDial,
existingDialTimeout, client.Dial, client.DialTimeout, ConfigureProxy,
ConfigureDialer) with the same host/IP validation used in the default branch
before invoking the underlying dialer; preserve behavior like keepalive and
timeout (use client.ReadTimeout for existingDialTimeout) and return the
validation error if the host is disallowed. Ensure you perform the validation at
the start of the case handlers and only call existingDial/existingDialTimeout
when validation passes.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 26871fbe-987b-46e9-97aa-2ab272207e0e
📒 Files selected for processing (5)
core/network/utils.gocore/providers/utils/utils.gocore/utils.gocore/utils_test.gotransports/bifrost-http/handlers/providers.go
TejasGhatte
force-pushed
the
06-01-feat_url_validation
branch
from
June 1, 2026 13:01
95d38fb to
06a457c
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/providers/utils/utils.go (1)
269-323:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply the host/IP gate before delegating to proxy or custom dialers.
These SSRF checks only run in the
defaultbranch. IfConfigureProxyor another caller has already populatedclient.Dial/client.DialTimeout, this wrapper goes throughexistingDial/existingDialTimeoutwithout validatingaddr, so proxied/custom-dial providers still bypass the new dial-time localhost/private-IP guard. Move theSplitHostPort+ resolve +IsLocalhost/IsPrivateIPvalidation ahead of theswitchso every dial mode shares the same gate.🤖 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 `@core/providers/utils/utils.go` around lines 269 - 323, The SSRF host/IP checks are only applied in the default dial branch, so custom/proxy dialers (existingDial, existingDialTimeout on client.Dial) bypass them; refactor the wrapper so you parse addr with net.SplitHostPort and perform the network.IsLocalhost and network.IsPrivateIP checks (and DNS resolution when needed) before the switch that chooses between existingDial, existingDialTimeout and the default logic, then pass the validated host/port or resolved IPs into the chosen dialing path (ensure you still use client.ReadTimeout and keepAliveCfg where appropriate).
♻️ Duplicate comments (1)
core/utils.go (1)
478-512:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake hostname resolution injectable so this validator can be tested deterministically.
ValidateExternalURLstill hard-codesnet.LookupIP, so the new table tests have to hit live DNS for hosts likeapi.openai.com. That makes this security regression coverage flaky in CI and offline environments. Please route lookup through a package-level hook or resolver interface and stub it incore/utils_test.go.Suggested minimal change
+var lookupIP = net.LookupIP + func ValidateExternalURL(urlStr string) error { ... - ips, err := net.LookupIP(hostname) + ips, err := lookupIP(hostname) if err != nil { return fmt.Errorf("failed to resolve hostname: %w", err) }As per coding guidelines,
**/*.go: "Apply standard Go review practices: ... deterministic tests, and table-driven coverage for behavior changes."🤖 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 `@core/utils.go` around lines 478 - 512, ValidateExternalURL currently calls net.LookupIP directly which prevents deterministic tests; introduce an injectable resolver by replacing direct net.LookupIP usage with a package-level variable or interface (e.g., var lookupIP = net.LookupIP or a Resolver type with LookupIP method) and use that in ValidateExternalURL to resolve hostnames; ensure the package-level hook is documented and resettable so core/utils_test.go can stub lookupIP to return controlled IPs for table-driven tests and restore the default after tests.
🤖 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.
Outside diff comments:
In `@core/providers/utils/utils.go`:
- Around line 269-323: The SSRF host/IP checks are only applied in the default
dial branch, so custom/proxy dialers (existingDial, existingDialTimeout on
client.Dial) bypass them; refactor the wrapper so you parse addr with
net.SplitHostPort and perform the network.IsLocalhost and network.IsPrivateIP
checks (and DNS resolution when needed) before the switch that chooses between
existingDial, existingDialTimeout and the default logic, then pass the validated
host/port or resolved IPs into the chosen dialing path (ensure you still use
client.ReadTimeout and keepAliveCfg where appropriate).
---
Duplicate comments:
In `@core/utils.go`:
- Around line 478-512: ValidateExternalURL currently calls net.LookupIP directly
which prevents deterministic tests; introduce an injectable resolver by
replacing direct net.LookupIP usage with a package-level variable or interface
(e.g., var lookupIP = net.LookupIP or a Resolver type with LookupIP method) and
use that in ValidateExternalURL to resolve hostnames; ensure the package-level
hook is documented and resettable so core/utils_test.go can stub lookupIP to
return controlled IPs for table-driven tests and restore the default after
tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 11d7ba2d-7b08-4ab6-beaf-db86bd90cf1a
📒 Files selected for processing (5)
core/network/utils.gocore/providers/utils/utils.gocore/utils.gocore/utils_test.gotransports/bifrost-http/handlers/providers.go
TejasGhatte
force-pushed
the
06-01-feat_url_validation
branch
from
June 1, 2026 13:15
06a457c to
6805390
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
core/utils_test.go (1)
20-37:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove live DNS dependency from the “valid URL” test fixtures.
Line 21, Line 26, Line 31, and Line 36 use
api.openai.com, which makes this unit test nondeterministic when DNS/network is unavailable. Use deterministic public IP literals (or a resolver stub hook) for these “valid” cases.🔧 Suggested deterministic fixture change
{ name: "valid https URL", - url: "https://api.openai.com", + url: "https://1.1.1.1", wantErr: false, }, { name: "valid http URL", - url: "http://api.openai.com", + url: "http://8.8.8.8", wantErr: false, }, { name: "valid https URL with path", - url: "https://api.openai.com/v1", + url: "https://1.1.1.1/v1", wantErr: false, }, { name: "valid https URL with port", - url: "https://api.openai.com:443", + url: "https://1.1.1.1:443", wantErr: false, },As per coding guidelines,
**/*.go: “Apply standard Go review practices: ... deterministic tests, and table-driven coverage for behavior changes.”🤖 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 `@core/utils_test.go` around lines 20 - 37, The table-driven test in core/utils_test.go uses live hostnames (api.openai.com) in the test cases (the slice of test cases with fields name, url, wantErr), making tests dependent on DNS; update those url entries to deterministic IP literals (e.g., replace "https://api.openai.com" with "https://93.184.216.34", "http://api.openai.com" with "http://93.184.216.34", "https://api.openai.com/v1" with "https://93.184.216.34/v1", and "https://api.openai.com:443" with "https://93.184.216.34:443") so the Test (the table-driven test using the url field) no longer relies on network/DNS.
🤖 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.
Duplicate comments:
In `@core/utils_test.go`:
- Around line 20-37: The table-driven test in core/utils_test.go uses live
hostnames (api.openai.com) in the test cases (the slice of test cases with
fields name, url, wantErr), making tests dependent on DNS; update those url
entries to deterministic IP literals (e.g., replace "https://api.openai.com"
with "https://93.184.216.34", "http://api.openai.com" with
"http://93.184.216.34", "https://api.openai.com/v1" with
"https://93.184.216.34/v1", and "https://api.openai.com:443" with
"https://93.184.216.34:443") so the Test (the table-driven test using the url
field) no longer relies on network/DNS.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d78e7406-87f0-4c41-9cc5-341b38e790ca
📒 Files selected for processing (6)
core/network/utils.gocore/providers/utils/dialer_test.gocore/providers/utils/utils.gocore/utils.gocore/utils_test.gotransports/bifrost-http/handlers/providers.go
TejasGhatte
force-pushed
the
06-01-feat_url_validation
branch
from
June 2, 2026 06:31
6805390 to
42a7b32
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/utils.go (1)
498-508:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPut a timeout on the validation-side DNS lookup.
ValidateExternalURLnow runs on the add/update-provider HTTP path, butnet.LookupIP(hostname)can block indefinitely. A slow or broken resolver can pin the handler goroutine before validation fails closed. Switch this to a boundednet.DefaultResolver.LookupIPcall, and preferably hide it behind a small package-level hook so tests can stub resolution.Suggested direction
+const externalURLResolveTimeout = 5 * time.Second + +var lookupExternalURLIPs = func(ctx context.Context, host string) ([]net.IP, error) { + return net.DefaultResolver.LookupIP(ctx, "ip", host) +} + func ValidateExternalURL(urlStr string) error { @@ - ips, err := net.LookupIP(hostname) + resolveCtx, cancel := context.WithTimeout(context.Background(), externalURLResolveTimeout) + defer cancel() + ips, err := lookupExternalURLIPs(resolveCtx, hostname) if err != nil { return fmt.Errorf("failed to resolve hostname: %w", err) }As per coding guidelines,
{transports,core}/**/*.go: "Apply HTTP/API security review: authentication and authorization checks, fail-closed behavior, input size limits, request validation, SSRF/path traversal/header injection defenses, CORS/cookie safety, and secret redaction in logs/errors."🤖 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 `@core/utils.go` around lines 498 - 508, The DNS lookup in ValidateExternalURL uses net.LookupIP which can block; replace it with a context-aware lookup using net.DefaultResolver.LookupIP(ctx, "ip", hostname) with a short timeout context (e.g., context.WithTimeout) so validation cannot hang the add/update-provider HTTP handler; also introduce a package-level resolver hook (e.g., var lookupIP = func(ctx, network, host) ([]net.IP, error) {...}) or a resolver interface used by ValidateExternalURL so tests can stub DNS resolution and unit tests can inject deterministic behavior; ensure error wrapping remains consistent when returning failures from ValidateExternalURL.
🤖 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.
Outside diff comments:
In `@core/utils.go`:
- Around line 498-508: The DNS lookup in ValidateExternalURL uses net.LookupIP
which can block; replace it with a context-aware lookup using
net.DefaultResolver.LookupIP(ctx, "ip", hostname) with a short timeout context
(e.g., context.WithTimeout) so validation cannot hang the add/update-provider
HTTP handler; also introduce a package-level resolver hook (e.g., var lookupIP =
func(ctx, network, host) ([]net.IP, error) {...}) or a resolver interface used
by ValidateExternalURL so tests can stub DNS resolution and unit tests can
inject deterministic behavior; ensure error wrapping remains consistent when
returning failures from ValidateExternalURL.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b1fe036e-3372-4da5-9acc-fa9ab6377e08
📒 Files selected for processing (6)
core/network/utils.gocore/providers/utils/dialer_test.gocore/providers/utils/utils.gocore/utils.gocore/utils_test.gotransports/bifrost-http/handlers/providers.go
Contributor
Merge activity
|
18 tasks
akshaydeo
pushed a commit
that referenced
this pull request
Jun 2, 2026
## Summary This PR closes a DNS rebinding vulnerability that existed between the time `ValidateExternalURL` validated a URL and the time the HTTP client actually opened a TCP connection. An attacker could exploit this window by having a hostname resolve to a public IP during validation and then to a private/internal IP at dial time, bypassing SSRF protections. ## Changes - Extracted `IsLocalhost` and `IsPrivateIP` into a new `core/network` package so they can be shared across validation and dialing layers. - Updated `ConfigureDialer` to perform its own DNS resolution at dial time, reject any resolved private or loopback IPs, and dial the resolved IP literal directly — eliminating the TOCTOU window between URL validation and connection. - Added `ValidateExternalURL` calls in the HTTP transport's `addProvider` and `updateProvider` handlers to reject private or loopback `BaseURL` values at the API boundary. - Added `core/utils_test.go` with comprehensive tests covering `ValidateExternalURL`, `IsLocalhost`, and `IsPrivateIP`, including RFC 1918 ranges, link-local addresses, the AWS metadata endpoint (`169.254.169.254`), IPv6 private ranges, and query-parameter injection vectors. ## Type of change - [x] Bug fix ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [x] Providers/Integrations ## How to test ```sh go version go test ./... ``` Key scenarios validated by the test suite: - `http://169.254.169.254/latest/meta-data/` → rejected as private IP - `http://10.0.0.1/path?x=` → rejected as private IP - `http://localhost:8080` → rejected as loopback - `https://api.openai.com` → allowed - IPv6 loopback (`::1`), link-local (`fe80::1`), and unique-local (`fc00::/7`) → all rejected ## Breaking changes - [ ] Yes - [x] No ## Security considerations This change directly addresses an SSRF / DNS rebinding attack vector. Previously, a malicious actor could register a hostname that resolved to a public IP during `ValidateExternalURL` and then switch the DNS record to an internal address (e.g., `169.254.169.254`, `10.x.x.x`) before the dialer connected. The dialer now resolves DNS independently, validates every returned IP against private ranges, and dials the IP literal, closing this window entirely. The `BaseURL` field on provider add/update endpoints is also now validated at the API layer. ## Checklist - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Security Improvements** * Strengthened SSRF protections: centralized hostname/IP checks now block localhost, link-local, unspecified, and private IPs when validating external URLs and provider Base URLs. * Provider create/update endpoints validate Base URLs and return clear Bad Request responses for unsafe targets. * Network dialing now resolves hosts and rejects disallowed/private addresses early to prevent unsafe connections. * **Tests** * Added extensive unit tests for URL validation, network address classification, and dialer SSRF behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
akshaydeo
pushed a commit
that referenced
this pull request
Jun 4, 2026
This PR closes a DNS rebinding vulnerability that existed between the time `ValidateExternalURL` validated a URL and the time the HTTP client actually opened a TCP connection. An attacker could exploit this window by having a hostname resolve to a public IP during validation and then to a private/internal IP at dial time, bypassing SSRF protections. - Extracted `IsLocalhost` and `IsPrivateIP` into a new `core/network` package so they can be shared across validation and dialing layers. - Updated `ConfigureDialer` to perform its own DNS resolution at dial time, reject any resolved private or loopback IPs, and dial the resolved IP literal directly — eliminating the TOCTOU window between URL validation and connection. - Added `ValidateExternalURL` calls in the HTTP transport's `addProvider` and `updateProvider` handlers to reject private or loopback `BaseURL` values at the API boundary. - Added `core/utils_test.go` with comprehensive tests covering `ValidateExternalURL`, `IsLocalhost`, and `IsPrivateIP`, including RFC 1918 ranges, link-local addresses, the AWS metadata endpoint (`169.254.169.254`), IPv6 private ranges, and query-parameter injection vectors. - [x] Bug fix - [x] Core (Go) - [x] Transports (HTTP) - [x] Providers/Integrations ```sh go version go test ./... ``` Key scenarios validated by the test suite: - `http://169.254.169.254/latest/meta-data/` → rejected as private IP - `http://10.0.0.1/path?x=` → rejected as private IP - `http://localhost:8080` → rejected as loopback - `https://api.openai.com` → allowed - IPv6 loopback (`::1`), link-local (`fe80::1`), and unique-local (`fc00::/7`) → all rejected - [ ] Yes - [x] No This change directly addresses an SSRF / DNS rebinding attack vector. Previously, a malicious actor could register a hostname that resolved to a public IP during `ValidateExternalURL` and then switch the DNS record to an internal address (e.g., `169.254.169.254`, `10.x.x.x`) before the dialer connected. The dialer now resolves DNS independently, validates every returned IP against private ranges, and dials the IP literal, closing this window entirely. The `BaseURL` field on provider add/update endpoints is also now validated at the API layer. - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable <!-- This is an auto-generated comment: release notes by coderabbit.ai --> * **Security Improvements** * Strengthened SSRF protections: centralized hostname/IP checks now block localhost, link-local, unspecified, and private IPs when validating external URLs and provider Base URLs. * Provider create/update endpoints validate Base URLs and return clear Bad Request responses for unsafe targets. * Network dialing now resolves hosts and rejects disallowed/private addresses early to prevent unsafe connections. * **Tests** * Added extensive unit tests for URL validation, network address classification, and dialer SSRF behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
18 tasks
akshaydeo
added a commit
that referenced
this pull request
Jun 6, 2026
## Summary This PR bumps the Go toolchain version from `1.26.3` to `1.26.4` across all modules and CI workflows, and cuts a new release (`core` v1.5.17, `framework` v1.3.17, `transports` v1.5.9, `plugins/compat` v0.1.16, `plugins/governance` v1.5.17, and associated plugin versions) incorporating a large batch of features and fixes accumulated since the previous release. ## Changes - **Go 1.26.4** — Updated `go-version` in all GitHub Actions workflows (`e2e-tests`, `helm-release`, `pr-tests`, `release-cli`, `release-pipeline`, `snyk`) and all `go.mod` files (core, framework, transports, cli, all plugins, examples, and test modules). - **Core (v1.5.17)** — OpenAI compaction support, multi-customer logs and usage tracking, multiple team/business unit support, `request_headers` wildcard pattern capture for OTel and Maxim plugins, xAI `x_search` tool, fetch URL validation with SSRF hardening, `file://` pricing URL scheme, virtual key provider fan-out filtering, and a broad set of fixes including Anthropic prompt cache key, empty thinking block stripping, OpenAI stream usage event cleanup, Gemini numeric schema constraints, stale connection retries, Azure Claude diagnostic strip, and passthrough budget handling. - **Framework (v1.3.17)** — Scope-aware budgets and limits wired from model configs, provider-level governance, multiple customer budget support with `calendar_aligned` windows, paginated virtual key fetch, `config.json` source-of-truth flow, FTS index cap reduction, sync worker drift fix, cascade deletes for model configs, and high-scale virtual key flow improvements. - **Transports (v1.5.9)** — Full changelog covering all of the above plus UI improvements (log navigation, customer detail sheet, `BudgetDisplay` component, inline loading shell, materialized view alias), SCIM provisioning fields, Helm/config schema additions (`roles`, `per_user_oauth`), client IP resolution from forwarded headers, and dependency upgrades (`recharts` to 3.8.1, `golang.org/x` CVE remediation). - **Plugins** — `governance` v1.5.17 adds team budget/rate-limit exporters, ghost node reconciliation fix, and VK double usage counting fix; `logging` v1.5.17 adds wildcard header capture and file attachment rendering; `otel` v1.2.17 adds `disable_content_logging` and multiple collectors support; `maxim` v1.6.17 adds `request_headers` wildcard capture; `compat` v0.1.16 fixes `max_tokens` preservation during param filtering. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [x] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [x] Providers/Integrations - [x] Plugins - [x] UI (React) - [ ] Docs ## How to test ```sh # Verify Go version go version # should report go1.26.4 # Run core tests cd core && go test ./... # Run framework tests cd framework && go test ./... # Run transports tests cd transports && go test ./... # Run plugin tests cd plugins/governance && go test ./... cd plugins/logging && go test ./... cd plugins/otel && go test ./... # UI cd ui pnpm i pnpm build pnpm test ``` ## Breaking changes - [ ] Yes - [x] No ## Related issues #4053, #4066, #4041, #4012, #3976, #3947, #3991, #4045, #3957, #3938, #3937, #3939, #3981, #3998, #3997, #4092, #4091, #4079, #4080, #4086, #3929, #3994, #4028, #3970, #3919, #3861, #3664, #3999, #4088, #4070, #4051, #4043, #4057, #4023, #3941, #3955, #4024, #3956, #3967, #3925, #3992, #3900 ## Security considerations - Fetch URL validation hardened against SSRF by tightening IP checks for private networks and link-local addresses (#4092, #3947, #3991). - Transitive `golang.org/x` dependencies (crypto, net, sys, text) bumped to address Docker Scout CVEs (#3900). ## Checklist - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [x] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [x] I verified the CI pipeline passes locally if applicable <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * OpenAI compaction, multi-customer/team logstore support, request-header wildcard capture, enhanced governance (provider-level & scope-aware limits), disable-content-logging option, support for multiple OpenTelemetry collectors, SSRF hardening and URL validation. * **Chores** * Bumped Go toolchain across modules and updated component/plugin version releases. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Merged
akshaydeo
pushed a commit
that referenced
this pull request
Jun 7, 2026
This PR closes a DNS rebinding vulnerability that existed between the time `ValidateExternalURL` validated a URL and the time the HTTP client actually opened a TCP connection. An attacker could exploit this window by having a hostname resolve to a public IP during validation and then to a private/internal IP at dial time, bypassing SSRF protections. - Extracted `IsLocalhost` and `IsPrivateIP` into a new `core/network` package so they can be shared across validation and dialing layers. - Updated `ConfigureDialer` to perform its own DNS resolution at dial time, reject any resolved private or loopback IPs, and dial the resolved IP literal directly — eliminating the TOCTOU window between URL validation and connection. - Added `ValidateExternalURL` calls in the HTTP transport's `addProvider` and `updateProvider` handlers to reject private or loopback `BaseURL` values at the API boundary. - Added `core/utils_test.go` with comprehensive tests covering `ValidateExternalURL`, `IsLocalhost`, and `IsPrivateIP`, including RFC 1918 ranges, link-local addresses, the AWS metadata endpoint (`169.254.169.254`), IPv6 private ranges, and query-parameter injection vectors. - [x] Bug fix - [x] Core (Go) - [x] Transports (HTTP) - [x] Providers/Integrations ```sh go version go test ./... ``` Key scenarios validated by the test suite: - `http://169.254.169.254/latest/meta-data/` → rejected as private IP - `http://10.0.0.1/path?x=` → rejected as private IP - `http://localhost:8080` → rejected as loopback - `https://api.openai.com` → allowed - IPv6 loopback (`::1`), link-local (`fe80::1`), and unique-local (`fc00::/7`) → all rejected - [ ] Yes - [x] No This change directly addresses an SSRF / DNS rebinding attack vector. Previously, a malicious actor could register a hostname that resolved to a public IP during `ValidateExternalURL` and then switch the DNS record to an internal address (e.g., `169.254.169.254`, `10.x.x.x`) before the dialer connected. The dialer now resolves DNS independently, validates every returned IP against private ranges, and dials the IP literal, closing this window entirely. The `BaseURL` field on provider add/update endpoints is also now validated at the API layer. - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable <!-- This is an auto-generated comment: release notes by coderabbit.ai --> * **Security Improvements** * Strengthened SSRF protections: centralized hostname/IP checks now block localhost, link-local, unspecified, and private IPs when validating external URLs and provider Base URLs. * Provider create/update endpoints validate Base URLs and return clear Bad Request responses for unsafe targets. * Network dialing now resolves hosts and rejects disallowed/private addresses early to prevent unsafe connections. * **Tests** * Added extensive unit tests for URL validation, network address classification, and dialer SSRF behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
akshaydeo
added a commit
that referenced
this pull request
Jun 7, 2026
This PR bumps the Go toolchain version from `1.26.3` to `1.26.4` across all modules and CI workflows, and cuts a new release (`core` v1.5.17, `framework` v1.3.17, `transports` v1.5.9, `plugins/compat` v0.1.16, `plugins/governance` v1.5.17, and associated plugin versions) incorporating a large batch of features and fixes accumulated since the previous release. - **Go 1.26.4** — Updated `go-version` in all GitHub Actions workflows (`e2e-tests`, `helm-release`, `pr-tests`, `release-cli`, `release-pipeline`, `snyk`) and all `go.mod` files (core, framework, transports, cli, all plugins, examples, and test modules). - **Core (v1.5.17)** — OpenAI compaction support, multi-customer logs and usage tracking, multiple team/business unit support, `request_headers` wildcard pattern capture for OTel and Maxim plugins, xAI `x_search` tool, fetch URL validation with SSRF hardening, `file://` pricing URL scheme, virtual key provider fan-out filtering, and a broad set of fixes including Anthropic prompt cache key, empty thinking block stripping, OpenAI stream usage event cleanup, Gemini numeric schema constraints, stale connection retries, Azure Claude diagnostic strip, and passthrough budget handling. - **Framework (v1.3.17)** — Scope-aware budgets and limits wired from model configs, provider-level governance, multiple customer budget support with `calendar_aligned` windows, paginated virtual key fetch, `config.json` source-of-truth flow, FTS index cap reduction, sync worker drift fix, cascade deletes for model configs, and high-scale virtual key flow improvements. - **Transports (v1.5.9)** — Full changelog covering all of the above plus UI improvements (log navigation, customer detail sheet, `BudgetDisplay` component, inline loading shell, materialized view alias), SCIM provisioning fields, Helm/config schema additions (`roles`, `per_user_oauth`), client IP resolution from forwarded headers, and dependency upgrades (`recharts` to 3.8.1, `golang.org/x` CVE remediation). - **Plugins** — `governance` v1.5.17 adds team budget/rate-limit exporters, ghost node reconciliation fix, and VK double usage counting fix; `logging` v1.5.17 adds wildcard header capture and file attachment rendering; `otel` v1.2.17 adds `disable_content_logging` and multiple collectors support; `maxim` v1.6.17 adds `request_headers` wildcard capture; `compat` v0.1.16 fixes `max_tokens` preservation during param filtering. - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [x] Chore/CI - [x] Core (Go) - [x] Transports (HTTP) - [x] Providers/Integrations - [x] Plugins - [x] UI (React) - [ ] Docs ```sh go version # should report go1.26.4 cd core && go test ./... cd framework && go test ./... cd transports && go test ./... cd plugins/governance && go test ./... cd plugins/logging && go test ./... cd plugins/otel && go test ./... cd ui pnpm i pnpm build pnpm test ``` - [ ] Yes - [x] No - Fetch URL validation hardened against SSRF by tightening IP checks for private networks and link-local addresses (#4092, #3947, #3991). - Transitive `golang.org/x` dependencies (crypto, net, sys, text) bumped to address Docker Scout CVEs (#3900). - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [x] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [x] I verified the CI pipeline passes locally if applicable <!-- This is an auto-generated comment: release notes by coderabbit.ai --> * **New Features** * OpenAI compaction, multi-customer/team logstore support, request-header wildcard capture, enhanced governance (provider-level & scope-aware limits), disable-content-logging option, support for multiple OpenTelemetry collectors, SSRF hardening and URL validation. * **Chores** * Bumped Go toolchain across modules and updated component/plugin version releases. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
akshaydeo
added a commit
that referenced
this pull request
Jun 7, 2026
## ✨ Features - **OpenAI Compaction** — Added OpenAI conversation compaction support across core, framework, logging, and the API surface (#4053) - **Multi-Customer & Org Hierarchy** — Logs and usage tracking now support multiple customers, teams, and business units, including business unit CRUD, team assignment, and governance endpoints in the OpenAPI spec (#4066, #4041, #4082) - **Provider-Level Governance** — Budgets & limits are now scope-aware and can be applied at the virtual-key top level and per provider, wired from the model configs table, with UI filters for scope and providers (#3938, #3937, #3939, #3981, #3962) - **Customer Budgets** — Customers support multiple budgets and `calendar_aligned` budget windows (#3998, #3997) - **Virtual Key Attribution & Controls** — Added a `created_by` user attribution column and a `blacklisted_models` column for virtual key provider configs (#3672, #3653) - **Request Header Capture** — OTel and Maxim observability plugins capture `request_headers` by pattern, with wildcard support (e.g. `x-custom-*`); logging gained the same wildcard header capture (#4012, #3958) - **OTel Content Controls & Collectors** — New `disable_content_logging` option drops message/tool content from exported spans, plus support for multiple OTel collectors (#4064, #3894) - **xAI x_search** — Added xAI `x_search` tool support (#3976) - **URL Validation** — Added fetch URL validation with private-network configuration and link-local blocking (#3947, #3991) - **File Scheme Pricing URLs** — Pricing source URLs now accept the `file://` scheme for air-gapped and self-hosted deployments (#4045) - **Paginated Virtual Keys** — Virtual key fetching is paginated to handle deployments with very large numbers of keys (#3957) - **Client IP Resolution** — Resolve client IP from `X-Forwarded-For`/`X-Real-IP` headers - **SCIM Provisioning** — Added `attributeType`/`attributeValue` SCIM provisioning fields - **Helm/Config Schema** — Added `roles` RBAC governance config and `per_user_oauth` MCP auth to the Helm chart and config schema (#4004, #4009) - **Log Navigation UI** — Added a "View logs" menu item to customer, team, and virtual key tables, clickable links in log detail views, a customer detail sheet, and a reusable `BudgetDisplay` component (#4073, #4054, #4026, #4055) - **Faster First Paint** — Added an inline loading shell to `#root` before React mounts (#4063) - **Materialized View Alias** — Added an `alias` column to the materialized view with filter support (#4078) ## 🐞 Fixed - **Fetch URL IP Checks** — Hardened fetch URL IP checks against SSRF (#4092) - **Mantle Model Matching** — Broadened Mantle model matching to all `gpt` variants (#4091) - **Empty Thinking Blocks** — Strip thinking blocks when the signature is empty (#4079) - **OpenAI Stream Usage** — Removed usage from the `responses.created` event in the OpenAI stream (#4080) - **Prompt Cache Key** — Set the prompt cache key from the Anthropic integration (#4086) - **Upstream Failure Status** — Map upstream connection failures to 502 instead of 400 (#3929) (thanks [@chris-colinsky](https://github.com/chris-colinsky)!) - **Gemini Schema Constraints** — Accept numeric schema integer constraints for Gemini (#3994) (thanks [@yanhao98](https://github.com/yanhao98)!) - **Files Provider Param** — Accept the `?provider=` query param on `GET /v1/files` (#3971) (thanks [@alexef](https://github.com/alexef)!) - **Optional Batch Model** — Made the `model` field optional on `POST /v1/batches` (#3973) (thanks [@alexef](https://github.com/alexef)!) - **Helm Azure Config** — Added missing `azure_key_config` fields to the Helm schema (#3996) (thanks [@axelray-dev](https://github.com/axelray-dev)!) - **Text Completion Chunk Model** — Added the missing `Model` field to `TextCompletionChunkResponse` (#3970) (thanks [@kuishou68](https://github.com/kuishou68)!) - **MCP Inline stdio Env** — MCP stdio server configs accept inline environment variable assignments (#3861) (thanks [@Shushmitaaaa](https://github.com/Shushmitaaaa)!) - **Orphaned Tool Results** — Orphaned tool results in the OpenAI to Anthropic conversion flow are no longer rejected by the Anthropic API (#3919) - **Node Usage Reconciliation** — Added a monotonic `inc_number` log cursor so node usage reconciliation does not skip late async log writes (#3664) - **Bedrock Output Assessments** — Corrected the type of `outputAssessments` in Bedrock responses (#4028) - **Model Pool Pricing Reloads** — Preserve non-pricing model pool entries across pricing reloads (#3999) - **Ghost Node Reconciliation** — Replicate the VK hierarchy flow for ghost node reconciliation (#4088) - **VK Double Usage Counting** — Fixed double usage counting when creating a virtual key (#4070) - **Model Config Lifecycle** — Cascade deletes for model configs and removal of stale in-memory model configs (#4051, #4043) - **FTS Index Cap** — Reduced the FTS index `left()` cap from 800k to 250k chars to stay within the tsvector limit (#4057) - **Sync Worker Drift** — Reduced the sync worker ticker period to 5m to prevent threshold drift (#4023) - **Passthrough** — Fixed passthrough budgets, gated passthrough models per VK, model extraction for Azure passthrough, and restricted fallbacks/provider selection to the VK boundary (#3941, #3988, #3983, #3924) - **Provider Response Headers** — Strip provider response headers and add a content-type filter (#3955, #4024) - **Stream Handling** — Drain non-SSE stream readers and retry stale connections (#3956, #3967) - **Azure Claude** — Strip Azure diagnostic property for Claude models (#3925) - **Compat max_tokens** — Preserve chat `max_tokens` during param filtering (#3992) - **Raw Request Flag** — Removed the raw request flag from providers that don't support it (#4058) - **UI Fixes** — Standardized page container layout, virtual key model configs UI, and dashboard chart tooltips (#4046, #4052, #4044) ## 🔧 Maintenance - **Dependency Upgrades** — Bumped transitive `golang.org/x` dependencies (crypto, net, sys, text) for Docker Scout CVE remediation and `recharts` to 3.8.1; cascaded version bumps across all modules (#3900, #4003)
2 tasks
soby
added a commit
to soby/bifrost
that referenced
this pull request
Jun 10, 2026
Upstream maximhq#3947's dial-time private-IP block broke dynamic deployments that route per-request BaseURLs at in-cluster services (gateway smoketests failed with 'connection to private IP 10.48.x.x is not allowed' on the first deploy carrying the uplift). The shared per-type client cannot carry per-tenant dial policy, so tenant SSRF policy for caller-supplied BaseURLs belongs at config-write time in the layer that owns tenant configs — the auto-init client opens AllowPrivateNetwork instead, restoring the pre-uplift posture. Link-local (cloud metadata endpoints: 169.254.x, fe80::) and unspecified addresses remain ALWAYS blocked in ConfigureDialer regardless of the flag. The regression test binds its mock upstream to the host's real RFC 1918 address — loopback is always allowed by the dialer, which is why every httptest-on-127.0.0.1 test sailed past this. Verified red/green: without the flag the test reproduces the exact production error.
soby
added a commit
to soby/bifrost
that referenced
this pull request
Jun 10, 2026
…licy for dynamic providers Restores per-request timeout injection for dynamic providers (removed as a silent no-op in 981371b) and fixes the dial policy upstream maximhq#3947 broke for dynamic deployments. Timeout: ProviderNetworkConfigOverride gains RequestTimeoutInSeconds, enforced in MakeRequestWithContext via fasthttp Request.SetTimeout so client.Do itself returns at the deadline (504) and closes the upstream connection — a context deadline would leave the caller blocked on the abandoned background Do via the wait() rendezvous until the upstream or transport ceiling let go. Auto-initialised (dynamic) providers build their shared client with DynamicProviderTransportTimeoutCeilingInSeconds (600s): the construction-time timeout is only a transport safety ceiling; each tenant's real timeout rides the request. Streaming calls are deliberately not bounded by this field — SSE handlers drive the streaming client directly and per-chunk progress is governed by the existing StreamIdleTimeoutInSeconds override. Dial policy: auto-initialised providers set AllowPrivateNetwork=true. The shared per-type client cannot carry per-tenant dial policy, and dynamic deployments legitimately route per-request BaseURLs at RFC 1918 addresses (in-cluster inference services). Link-local (cloud metadata endpoints: 169.254.x, fe80::) and unspecified addresses remain ALWAYS blocked in ConfigureDialer regardless of the flag. Tenant SSRF policy for caller-supplied BaseURLs belongs at config-write time in the layer that owns tenant configs. Memory stays O(#built-in provider types): no per-tenant clients, queues, or worker pools are ever created. Tests: TestPerRequestTimeoutOverride_TwoProvidersDistinctTimeouts drives two providers (openai@1s, anthropic@2s) concurrently through one Bifrost instance — each request must 504 at ITS OWN provider's deadline. Mock upstreams hang until client disconnect (drain r.Body first or net/http never cancels r.Context()), proving the deadline actively releases upstream connections. TestAutoInitDynamicProviderAllowsPrivateNetworkUpstreams binds its mock to the host's real RFC 1918 address — loopback is always allowed by the dialer, so httptest-on-127.0.0.1 never exercises the private-IP block. Both verified red/green.
soby
added a commit
to soby/bifrost
that referenced
this pull request
Jun 10, 2026
…dynamic providers Merges upstream/main @ 9399212 (~420 commits: key rotation, direct-key bypass, Azure v1 API, CompactionRequest, SSRF dial hardening, OTel attrs) into feat/provider-override, combined with the fork-side uplift work: Pre-merge cleanup (Phase B): - Auto-init config pollution fix: per-request NetworkConfig overrides no longer leak into the permanent provider config stored by prepareProvider. - Dead alias machinery removed (BaseProviderType, resolveQueueProviderKey, queue retargeting) — the gateway resolves aliases itself. - StreamIdleTimeout override made read-time (override > ctx > config), no ctx persistence, so fallback attempts don't inherit the primary's value. - Clone deep-copies inner-request Params pointers (MCP write-through fix). - ClearValue reverted to upstream tombstone semantics. - PreLLMHook idempotency expectation documented on prepareFallbackRequest. Merge resolution decisions: - Plugin override key takes precedence over caller-supplied direct-key at all three key-selection sites. - effectiveConfig (per-request MaxRetries/backoff) re-applied onto upstream's restructured executeRequestWithRetries. - validateRequest deferral + transport relaxations kept (empty provider is validated post-plugin so plugins can set it). - CompactionRequest added to SetProvider/SetModel/Clone with a reflection exhaustiveness test over all request types. Post-merge additions: - ProviderNetworkConfigOverride.RequestTimeoutInSeconds: per-request timeout enforced via fasthttp Request.SetTimeout (504 at deadline, conn closed). Auto-init dynamic providers build their shared client with DynamicProviderTransportTimeoutCeilingInSeconds (600s) as a transport safety ceiling only; each tenant's timeout rides the request. Streaming is governed by StreamIdleTimeoutInSeconds instead. Memory stays O(#built-in provider types). - Auto-init dynamic providers set AllowPrivateNetwork=true: upstream maximhq#3947 dial-time private-IP blocking broke routing to in-cluster RFC 1918 upstreams; link-local/metadata (169.254.x, fe80::) stays always-blocked. Tenant SSRF policy belongs at config-write time. - Tests: TestPerRequestTimeoutOverride_TwoProvidersDistinctTimeouts (openai@1s vs anthropic@2s concurrently, disconnect-aware hanging mocks), TestAutoInitDynamicProviderAllowsPrivateNetworkUpstreams (binds the host's real RFC 1918 address — loopback never exercises the block), override-key dead-key 502 path, Clone/SetProvider/SetModel exhaustiveness over all 45 request types.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR closes a DNS rebinding vulnerability that existed between the time
ValidateExternalURLvalidated a URL and the time the HTTP client actually opened a TCP connection. An attacker could exploit this window by having a hostname resolve to a public IP during validation and then to a private/internal IP at dial time, bypassing SSRF protections.Changes
IsLocalhostandIsPrivateIPinto a newcore/networkpackage so they can be shared across validation and dialing layers.ConfigureDialerto perform its own DNS resolution at dial time, reject any resolved private or loopback IPs, and dial the resolved IP literal directly — eliminating the TOCTOU window between URL validation and connection.ValidateExternalURLcalls in the HTTP transport'saddProviderandupdateProviderhandlers to reject private or loopbackBaseURLvalues at the API boundary.core/utils_test.gowith comprehensive tests coveringValidateExternalURL,IsLocalhost, andIsPrivateIP, including RFC 1918 ranges, link-local addresses, the AWS metadata endpoint (169.254.169.254), IPv6 private ranges, and query-parameter injection vectors.Type of change
Affected areas
How to test
go version go test ./...Key scenarios validated by the test suite:
http://169.254.169.254/latest/meta-data/→ rejected as private IPhttp://10.0.0.1/path?x=→ rejected as private IPhttp://localhost:8080→ rejected as loopbackhttps://api.openai.com→ allowed::1), link-local (fe80::1), and unique-local (fc00::/7) → all rejectedBreaking changes
Security considerations
This change directly addresses an SSRF / DNS rebinding attack vector. Previously, a malicious actor could register a hostname that resolved to a public IP during
ValidateExternalURLand then switch the DNS record to an internal address (e.g.,169.254.169.254,10.x.x.x) before the dialer connected. The dialer now resolves DNS independently, validates every returned IP against private ranges, and dials the IP literal, closing this window entirely. TheBaseURLfield on provider add/update endpoints is also now validated at the API layer.Checklist
docs/contributing/README.mdand followed the guidelinesSummary by CodeRabbit
Security Improvements
Tests