ipv6 support - #4895
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 32 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughHost handling is reworked across proxy dialing, Pinecone store setup, OAuth2 redirect validation, CORS origin checks, and websocket origin checks to use ChangesIPv6-aware loopback/localhost detection
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client as Client/Browser
participant Utils as isLocalhostOrigin / isLocalhost
participant OAuth as isLoopbackRedirectHost
participant NetPkg as net package
participant Proxy as dialAddrHost / shouldBypassProxy
Client->>Utils: request with Origin/host
Utils->>NetPkg: url.Parse, SplitHostPort, ParseIP
NetPkg-->>Utils: host, loopback status
Utils-->>Client: allowed/denied
Client->>OAuth: redirect URI
OAuth->>NetPkg: ParseIP(hostname)
NetPkg-->>OAuth: IsLoopback result
OAuth-->>Client: scheme/match decision
Client->>Proxy: outbound address
Proxy->>NetPkg: SplitHostPort(addr)
NetPkg-->>Proxy: extracted host
Proxy-->>Client: proxy bypass decision
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
|
![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}
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 (1)
transports/bifrost-http/handlers/mcpoauth2issuance.go (1)
687-691: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winComment/code mismatch: host is not actually compared for loopback matches. The inline comment states "match scheme + host (without port) + path", but the condition only checks
parsed.Scheme == rParsed.Scheme && parsed.Path == rParsed.Path— the host is ignored, so a registeredhttp://127.0.0.1/cbwill match a candidatehttp://[::1]/cb. Treating loopback variants as interchangeable is defensible per RFC 8252 §7.3 (loopback host/port flexibility), but please align the comment with the behavior (or add the host check if cross-loopback matching is unintended).🤖 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 `@transports/bifrost-http/handlers/mcpoauth2issuance.go` around lines 687 - 691, The loopback redirect check in mcpoauth2issuance.go is not comparing the host even though the comment says it does; update the logic around the loopback branch in the redirect matching helper to either add an explicit host comparison between parsed and rParsed or revise the inline comment to match the intended cross-loopback behavior. Use the existing loopback matching code near isLoopbackRedirectHost and the parsed/rParsed scheme-path comparison to keep the behavior and comment consistent.
🤖 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 `@transports/bifrost-http/handlers/mcpoauth2issuance.go`:
- Around line 654-657: Move the doc comments so each one is directly attached to
its matching function: place the `matchRedirectURI` comment immediately above
`matchRedirectURI`, and move the `isAllowedRedirectScheme` comment so it sits
immediately above `isAllowedRedirectScheme` in `mcpoauth2issuance.go`. Keep the
descriptions unchanged, just reorder them to match their symbols.
In `@transports/Dockerfile`:
- Line 109: The healthcheck probe in the Dockerfile is using localhost, which
can resolve to IPv6 first and make the check flaky in this image. Update the CMD
used for the healthcheck to probe 127.0.0.1 instead, or otherwise force IPv4, so
the check is consistent with the container listening on APP_HOST via 0.0.0.0.
In `@transports/Dockerfile.local`:
- Line 112: The healthcheck command in the Dockerfile.local uses localhost,
which can resolve to ::1 and make the probe depend on resolver order. Update the
healthcheck in the CMD line to target 127.0.0.1 instead, keeping the same
APP_PORT and /health path so the check is consistently bound to IPv4 loopback.
---
Outside diff comments:
In `@transports/bifrost-http/handlers/mcpoauth2issuance.go`:
- Around line 687-691: The loopback redirect check in mcpoauth2issuance.go is
not comparing the host even though the comment says it does; update the logic
around the loopback branch in the redirect matching helper to either add an
explicit host comparison between parsed and rParsed or revise the inline comment
to match the intended cross-loopback behavior. Use the existing loopback
matching code near isLoopbackRedirectHost and the parsed/rParsed scheme-path
comparison to keep the behavior and comment consistent.
🪄 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: CHILL
Plan: Pro Plus
Run ID: b697e786-6f5c-467b-a132-dd674a63101d
📒 Files selected for processing (9)
core/network/http.goframework/vectorstore/pinecone.gohelm-charts/bifrost/values.yamltransports/Dockerfiletransports/Dockerfile.localtransports/bifrost-http/handlers/mcpoauth2issuance.gotransports/bifrost-http/handlers/utils.gotransports/bifrost-http/handlers/websocket.gotransports/bifrost-http/server/server.go
1394ee1 to
e7b88af
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@transports/bifrost-http/handlers/localhostcheck_test.go`:
- Around line 5-31: The localhost check currently treats an empty host as
trusted, which allows a malformed WebSocket origin check to pass. Update
isLocalhost in the localhost-check logic to fail closed by returning false for
an empty Host value, and adjust TestIsLocalhost in localhostcheck_test.go to
expect false for the empty-string case while keeping the existing localhost/IP
cases unchanged.
🪄 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: CHILL
Plan: Pro Plus
Run ID: efc8aea5-2e06-46cf-8235-18e6ca2bfe95
📒 Files selected for processing (12)
core/network/dialaddrhost_test.gocore/network/http.goframework/vectorstore/pinecone.goframework/vectorstore/pineconehost_test.gohelm-charts/bifrost/values.yamltransports/Dockerfiletransports/Dockerfile.localtransports/bifrost-http/handlers/localhostcheck_test.gotransports/bifrost-http/handlers/mcpoauth2issuance.gotransports/bifrost-http/handlers/utils.gotransports/bifrost-http/handlers/websocket.gotransports/bifrost-http/server/server.go
✅ Files skipped from review due to trivial changes (3)
- helm-charts/bifrost/values.yaml
- transports/Dockerfile.local
- transports/bifrost-http/server/server.go
🚧 Files skipped from review as they are similar to previous changes (6)
- transports/Dockerfile
- transports/bifrost-http/handlers/websocket.go
- framework/vectorstore/pinecone.go
- core/network/http.go
- transports/bifrost-http/handlers/mcpoauth2issuance.go
- transports/bifrost-http/handlers/utils.go
e7b88af to
b9c5b2c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@transports/bifrost-http/handlers/websocket.go`:
- Around line 78-84: The host validation in the WebSocket upgrade path is too
permissive because the current bracket removal in the loopback check can
normalize malformed bracketed hosts into valid IPs. Update the host handling in
the websocket.go logic around the loopback/upgrade validation to only accept
exactly paired brackets, and treat bracketed values as IP literals before
permitting the upgrade. Keep the behavior fail-closed for malformed inputs by
rejecting cases like partially bracketed hosts rather than trimming them into a
valid address.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 19b5e17d-03a0-4bf8-af9f-4d748a19a091
📒 Files selected for processing (10)
core/network/dialaddrhost_test.gocore/network/http.goframework/vectorstore/pinecone.goframework/vectorstore/pineconehost_test.gohelm-charts/bifrost/values.yamltransports/bifrost-http/handlers/localhostcheck_test.gotransports/bifrost-http/handlers/mcpoauth2issuance.gotransports/bifrost-http/handlers/utils.gotransports/bifrost-http/handlers/websocket.gotransports/bifrost-http/server/server.go
✅ Files skipped from review due to trivial changes (2)
- helm-charts/bifrost/values.yaml
- transports/bifrost-http/server/server.go
🚧 Files skipped from review as they are similar to previous changes (7)
- core/network/dialaddrhost_test.go
- framework/vectorstore/pineconehost_test.go
- core/network/http.go
- transports/bifrost-http/handlers/localhostcheck_test.go
- transports/bifrost-http/handlers/utils.go
- transports/bifrost-http/handlers/mcpoauth2issuance.go
- framework/vectorstore/pinecone.go
b9c5b2c to
252ecd8
Compare
Merge activity
|
* 'dev' of https://github.com/maximhq/bifrost: ipv6 support (maximhq#4895) docs: add virtual key expiry support docs (maximhq#4889) test: add Postman e2e collection and runner for virtual key expiry validation and enforcement (maximhq#4888) feat: add expiry field to virtual keys (maximhq#4887) fix: converts thinking to disabled if tool choice is required for deepseek (maximhq#4861) chore: adds docs for deepseek provider (maximhq#4854) chore: adds tests for deepseek provider (maximhq#4853) feat: adds deepseek provider (maximhq#4852) fix: cost for image generation or image edit streaming (maximhq#4802) feat: add `BedrockMantleKeyConfig` support to key hashing, schema/table mapping, and sensitive field clearing (maximhq#4886) fix: skip O(N) reference refresh on request-time rate-limit/budget reset (maximhq#4883) refactor: simplify Responses lifecycle permissions to require explicit per-verb flags and expose them in UI (maximhq#4880) fix: append datasheet models for incomplete list models call (maximhq#4879) # Conflicts: # ui/app/workspace/providers/fragments/allowedRequestsFields.tsx # ui/app/workspace/virtual-keys/views/virtualKeyDetailsSheet.tsx # ui/app/workspace/virtual-keys/views/virtualKeySheet.tsx # ui/app/workspace/virtual-keys/views/virtualKeysTable.tsx # ui/components/ui/datePickerWithRange.tsx
2 participants
Summary
This PR fixes IPv6 handling across several components where naive string splitting on
:or hardcoded checks for only127.0.0.1would mangle IPv6 literals or miss IPv6 loopback addresses like::1. It also consolidates loopback detection logic into reusable helpers and corrects the startup log message to use the actual bound address.Changes
core/network/http.go: Replacedstrings.Split(addr, ":")[0]withnet.SplitHostPortfor proxy bypass host extraction, correctly unwrapping IPv6 bracket notation (e.g.,[::1]:8080→::1).framework/vectorstore/pinecone.go: Extracted ahostWithLocalSchemehelper that usesnet.SplitHostPortandnet.ParseIP().IsLoopback()to detect loopback addresses, replacing hardcodedlocalhost/127.0.0.1prefix checks. This now correctly handles IPv6 loopback ([::1]) for Pinecone Local connections.transports/bifrost-http/handlers/mcpoauth2issuance.go: IntroducedisLoopbackRedirectHostusingnet.ParseIP().IsLoopback(), replacing inline string comparisons. OAuth2 redirect URI matching and scheme validation now correctly recognize[::1]as a loopback per RFC 8252 §7.3.transports/bifrost-http/handlers/utils.go: RewroteisLocalhostOriginto parse the origin URL and usenet.ParseIPwithIsLoopback()/IsUnspecified(), covering IPv6 literals and bracketed addresses instead of a series ofstrings.HasPrefixchecks.transports/bifrost-http/handlers/websocket.go: Replacedstrings.LastIndex(host, ":")port stripping withnet.SplitHostPortinisLocalhost, and usesnet.ParseIP().IsLoopback()instead of an explicit::1string comparison.transports/bifrost-http/server/server.go: Fixed the startup log to print the actual boundserverAddrrather than reconstructing it froms.Hostands.Portseparately, which could produce a malformed URL for IPv6 hosts.transports/Dockerfile/transports/Dockerfile.local: Changed theHEALTHCHECKcommand fromhttp://127.0.0.1:${APP_PORT}/healthtohttp://localhost:${APP_PORT}/healthfor compatibility with IPv6-only or dual-stack environments.helm-charts/bifrost/values.yaml: Added a comment clarifying thathost: 0.0.0.0binds IPv4 interfaces only, and that::should be used for dual-stack or IPv6-only clusters.Type of change
Affected areas
How to test
To validate IPv6 loopback behavior specifically:
[::1]and confirmhttp://is correctly prepended.redirect_uriusing[::1]and confirm it is accepted as a loopback address.[::1]origin and confirm it is treated as localhost.Breaking changes
Security considerations
The OAuth2 redirect URI loopback detection now correctly includes
::1per RFC 8252 §7.3, which allows IPv6 loopback redirect URIs withhttp://scheme. This is intentional and spec-compliant. No previously rejected addresses are newly permitted beyond the IPv6 loopback literal.Checklist
docs/contributing/README.mdand followed the guidelines