[codex] fix CodeQL URL and Cursor JSON hardening#963
Conversation
Co-authored-by: Codex <noreply@openai.com>
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 19 minutes and 42 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
Note
|
codeant-ai
Bot
added
the
size:L
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| } | ||
| httpClient.Transport = h.apiCallTransport(auth) | ||
|
|
||
| httpClient := h.apiCallHTTPClient(auth) |
There was a problem hiding this comment.
Suggestion: Since this PR modifies a function that is far longer than 40 lines, refactor APICall by extracting cohesive sections (such as request parsing, header/token preparation, upstream execution, and response formatting) into small helper functions to keep the handler within the size rule. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The final APICall function is much longer than 40 lines, so it violates the stated function-length rule. The suggestion to extract request parsing, header/token handling, upstream execution, and response formatting into helpers directly addresses that real violation.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** pkg/llmproxy/api/handlers/management/api_call.go
**Line:** 204:204
**Comment:**
*Custom Rule: Since this PR modifies a function that is far longer than 40 lines, refactor `APICall` by extracting cohesive sections (such as request parsing, header/token preparation, upstream execution, and response formatting) into small helper functions to keep the handler within the size rule.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| openaiResp, errMarshal := cursorCompletionJSON(id, created, parsed.Model, fullText.String()) | ||
| if errMarshal != nil { | ||
| return resp, fmt.Errorf("cursor: failed to encode response: %w", errMarshal) |
There was a problem hiding this comment.
Suggestion: Extract the response-encoding and post-processing block into a small helper so this modified function stays within the 40-line function-length rule. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This added block is inside the already-large Execute function, and the suggested custom rule is about keeping modified functions under 40 lines. The new response-encoding/post-processing logic contributes to that length violation, so the suggestion matches a real issue.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** pkg/llmproxy/executor/cursor_executor.go
**Line:** 336:338
**Comment:**
*Custom Rule: Extract the response-encoding and post-processing block into a small helper so this modified function stays within the 40-line function-length rule.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| openaiJSON, errMarshal := cursorChunkJSON(chatId, created, parsed.Model, json.RawMessage(delta), finishReason) | ||
| if errMarshal != nil { | ||
| log.Warnf("cursor: failed to encode stream chunk: %v", errMarshal) | ||
| return | ||
| } | ||
| openaiJSON := fmt.Sprintf(`{"id":"%s","object":"chat.completion.chunk","created":%d,"model":"%s","choices":[{"index":0,"delta":%s,"finish_reason":%s}]}`, | ||
| chatId, created, parsed.Model, delta, fr) | ||
| sseLine := []byte("data: " + openaiJSON + "\n") | ||
| sseLine := append([]byte("data: "), openaiJSON...) | ||
| sseLine = append(sseLine, '\n') |
There was a problem hiding this comment.
Suggestion: Move stream chunk encoding/emission logic into a dedicated helper to reduce the size of this modified function body below the 40-line limit. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This is newly added logic inside ExecuteStream, which is already a very large function. The suggestion is aimed at the 40-line function-length rule, and this block is part of the extra code that makes the modified function exceed that limit.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** pkg/llmproxy/executor/cursor_executor.go
**Line:** 541:547
**Comment:**
*Custom Rule: Move stream chunk encoding/emission logic into a dedicated helper to reduce the size of this modified function body below the 40-line limit.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| if len(resolved) == 0 { | ||
| return nil, fmt.Errorf("target host resolution failed") | ||
| } | ||
| dialer := &net.Dialer{} | ||
| return dialer.DialContext(ctx, network, net.JoinHostPort(resolved[0].IP.String(), port)) |
There was a problem hiding this comment.
Suggestion: The dial guard always connects to only the first resolved IP address, so requests can fail unnecessarily when that single address is unreachable or not compatible with the requested network family while other resolved addresses are valid. Iterate through the allowed addresses (or use a fallback strategy) instead of hard-pinning to index 0. [logic error]
Severity Level: Major ⚠️
- ❌ /v0/management/api-call intermittently fails for multi-IP targets.
- ⚠️ Management integrations see 502 errors though upstream is reachable.
- ⚠️ Requires retries or manual reruns for affected API calls.Steps of Reproduction ✅
1. Start the server so the management API handler `APICall` is exposed at POST
`/v0/management/api-call` (see `pkg/llmproxy/api/handlers/management/api_call.go:32-38`,
`api_call.go:86`).
2. From a management client, send a POST to `/v0/management/api-call` with JSON body
targeting an external URL whose hostname resolves to multiple public IPs, where the first
resolved IP is unreachable but a later one is reachable (e.g., host has both IPv6 and IPv4
records but the environment has no IPv6 route). The request is parsed and sanitized in
`APICall` (url handling at `api_call.go:117-123`, `sanitizeAPICallURL` at
`api_call_url.go:37-60`, and `validateResolvedHostIPs` at `api_call_url.go:62-89`).
3. `APICall` builds an outbound request and constructs an HTTP client via `httpClient :=
h.apiCallHTTPClient(auth)` (see `api_call.go:180-189`, `api_call.go:25`).
`apiCallHTTPClient` creates an `http.Client` using `Transport:
apiCallGuardedRoundTripper{base: h.apiCallTransport(auth)}` (see
`http_transport.go:53-56`), and `apiCallTransport` uses a cloned `http.Transport` with
`clone.DialContext = guardedAPICallDialContext` when no proxy is configured (see
`http_transport.go:43-50`).
4. During `httpClient.Do(req)` (see `api_call.go:25-30`), the HTTP transport calls
`guardedAPICallDialContext` (implemented at `api_call_guard.go:11-24`), which resolves
allowed IPs via `resolveAllowedAPICallHostIPs(host)` and then dials only `resolved[0].IP`
(lines `api_call_guard.go:16-24`). If that first IP is unreachable while another allowed
IP in `resolved[1:]` is reachable, the dial fails and `APICall` returns `HTTP 502` with
`"request failed"` (see `api_call.go:25-30`), even though a working address for the same
hostname exists—an avoidable failure caused by pinning to the first resolved IP instead of
iterating or falling back.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** pkg/llmproxy/api/handlers/management/api_call_guard.go
**Line:** 20:24
**Comment:**
*Logic Error: The dial guard always connects to only the first resolved IP address, so requests can fail unnecessarily when that single address is unreachable or not compatible with the requested network family while other resolved addresses are valid. Iterate through the allowed addresses (or use a fallback strategy) instead of hard-pinning to index 0.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| return nil, fmt.Errorf("invalid url host") | ||
| } | ||
| resolved, errLookup := net.LookupIP(trimmed) | ||
| resolved, errLookup := net.DefaultResolver.LookupIPAddr(context.Background(), trimmed) |
There was a problem hiding this comment.
Suggestion: DNS resolution is performed with context.Background(), which ignores request cancellation and client timeout propagation. A slow or stuck resolver can block the handler path even after the request is canceled, causing request hangs and potential resource exhaustion under load. Thread the request/dial context into this resolver call and use it for lookup. [possible bug]
Severity Level: Major ⚠️
- ❌ Management API /v0/management/api-call can hang on DNS.
- ⚠️ Copilot quota enrichment pauses when resolver is slow.
- ⚠️ Guarded dialer DNS check ignores caller cancellation context.Steps of Reproduction ✅
1. Start the service so the management API handler `APICall` in
`pkg/llmproxy/api/handlers/management/api_call.go:86-255` is serving `POST
/v0/management/api-call` (documented in the comment block at lines 32-83).
2. From a client, send a `POST /v0/management/api-call` request with a valid JSON body
pointing `url` to an HTTPS endpoint whose hostname resolves slowly or hangs via the system
DNS (e.g., misconfigured or unreachable resolver); this causes `APICall` to call
`sanitizeAPICallURL` in `api_call_url.go:37-59`, which in turn leads to
`validateResolvedHostIPs(parsedURL.Hostname())` at `api_call.go:127`.
3. While the handler is blocked in `validateResolvedHostIPs` (implemented at
`api_call_url.go:62-65`), the function calls `resolveAllowedAPICallHostIPs` at
`api_call_url.go:67-89`, which performs
`net.DefaultResolver.LookupIPAddr(context.Background(), trimmed)` at line 72; now abort
the client request (e.g., `curl` Ctrl-C or closing the HTTP connection) so
`c.Request.Context()` in `APICall` is canceled.
4. Observe that despite the request context being canceled, the goroutine remains blocked
until the DNS lookup completes or times out at OS level because `LookupIPAddr` is invoked
with `context.Background()` (line 72) instead of the request/dial context; the same
uncancelable DNS resolution is also used in the guarded dial path
`guardedAPICallDialContext` in `api_call_guard.go:11-25`, which calls
`resolveAllowedAPICallHostIPs` at line 16, so both direct management calls and guarded
transports can hang under DNS issues.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** pkg/llmproxy/api/handlers/management/api_call_url.go
**Line:** 72:72
**Comment:**
*Possible Bug: DNS resolution is performed with `context.Background()`, which ignores request cancellation and client timeout propagation. A slow or stuck resolver can block the handler path even after the request is canceled, causing request hangs and potential resource exhaustion under load. Thread the request/dial context into this resolver call and use it for lookup.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
CodeAnt AI finished reviewing your PR. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is ON, but it could not run because the branch was deleted or merged before autofix could start.
Reviewed by Cursor Bugbot for commit 5d88d7d. Configure here.
| data = []byte(`{"choices":[{"index":0,"delta":{},"finish_reason":null}]}`) | ||
| } | ||
| return cliproxyexecutor.StreamChunk{ | ||
| Payload: []byte(data), |
There was a problem hiding this comment.
Modified function sseChunk is never called anywhere
Low Severity
The sseChunk function was refactored in this PR to use cursorChunkJSON, but a codebase-wide grep confirms it has zero callers — it is dead code. The new cursorChunkJSON call, the error-handling fallback, and the log.Warnf path inside it are all unreachable.
Reviewed by Cursor Bugbot for commit 5d88d7d. Configure here.
| if len(allowed) == 0 { | ||
| return nil, fmt.Errorf("target host resolution failed") | ||
| } | ||
| return allowed, nil |
There was a problem hiding this comment.
Dial guard context not propagated to DNS resolution
Low Severity
resolveAllowedAPICallHostIPs hard-codes context.Background() for DNS resolution. guardedAPICallDialContext receives a ctx (carrying the request deadline/cancellation) but never forwards it, so DNS lookups cannot be canceled when the HTTP client timeout fires or the request is aborted. This could cause the dial to block on slow DNS even after the request context is done.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 5d88d7d. Configure here.
|
CodeAnt AI is running the review. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
size:L
Sequence DiagramThis PR hardens the management API call path by routing outbound requests through a guarded HTTP client that validates each URL and redirect, resolves hosts to safe IPs, and blocks private or local targets before dialing. sequenceDiagram
participant AdminClient as Admin client
participant ManagementAPI as Management API
participant GuardedClient as Guarded HTTP client
participant DNSResolver as DNS resolver
participant TargetAPI as Target API
AdminClient->>ManagementAPI: Submit management API call request
ManagementAPI->>GuardedClient: Send outbound request through guarded client
GuardedClient->>DNSResolver: Validate URL and redirects and resolve host to safe IPs
DNSResolver-->>GuardedClient: Allowed IPs or error
alt Safe target
GuardedClient->>TargetAPI: Send HTTP request to validated IP
TargetAPI-->>GuardedClient: HTTP response
GuardedClient-->>ManagementAPI: Proxied response
ManagementAPI-->>AdminClient: Return target response
else Unsafe or invalid target
GuardedClient-->>ManagementAPI: URL or host validation error
ManagementAPI-->>AdminClient: Return request failed error
end
Generated by CodeAnt AI |
| resolved, errResolve := resolveAllowedAPICallHostIPs(host) | ||
| if errResolve != nil { | ||
| return nil, errResolve | ||
| } | ||
| if len(resolved) == 0 { | ||
| return nil, fmt.Errorf("target host resolution failed") | ||
| } | ||
| dialer := &net.Dialer{} | ||
| return dialer.DialContext(ctx, network, net.JoinHostPort(resolved[0].IP.String(), port)) |
There was a problem hiding this comment.
Suggestion: The dial guard always connects to resolved[0] and never retries other validated IPs for the same host. On multi-record/dual-stack DNS responses, this can fail requests when the first address is unreachable even though another allowed address would succeed. Iterate through resolved IPs (or use a dial strategy with fallback) instead of pinning to only the first entry. [logic error]
Severity Level: Major ⚠️
- ❌ /v0/management/api-call fails for partially reachable hosts.
- ⚠️ Copilot quota fetch can fail despite usable IPs.
- ⚠️ Reduces robustness against common dual-stack DNS setups.Steps of Reproduction ✅
1. Start cliproxyapi++ so the management handler `APICall` in
`pkg/llmproxy/api/handlers/management/api_call.go:86` is serving `POST
/v0/management/api-call`.
2. Configure DNS for some allowed external host (e.g. `api.example.com`) so
`net.DefaultResolver.LookupIPAddr` returns multiple IPs where the first is unreachable
(firewalled or blackholed) and a later one is reachable.
3. From a management client, call `POST /v0/management/api-call` with JSON body
`{"method":"GET","url":"https://api.example.com/ping"}` so the handler builds an HTTP
client via `apiCallHTTPClient` (`http_transport.go:53-56`), which uses
`guardedAPICallDialContext` as its `DialContext` (`http_transport.go:47-50`).
4. When the client performs `httpClient.Do(req)` (`api_call.go:25-27`), the dial path
`guardedAPICallDialContext` (`api_call_guard.go:11-24`) resolves all allowed IPs but
unconditionally dials only `resolved[0].IP` (`api_call_guard.go:16-24`); the connection
fails even though a later allowed IP would succeed, causing the management API call to
fail unnecessarily.
5. The same single-IP dial behavior is exercised by the Copilot quota enrichment path,
which constructs an `http.Client` using `h.apiCallTransport(auth)` (with
`clone.DialContext = guardedAPICallDialContext` in `http_transport.go:43-50`) in
`copilot_quota.go:32-35`, so quota fetches also fail when the first IP is bad but others
are good.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** pkg/llmproxy/api/handlers/management/api_call_guard.go
**Line:** 16:24
**Comment:**
*Logic Error: The dial guard always connects to `resolved[0]` and never retries other validated IPs for the same host. On multi-record/dual-stack DNS responses, this can fail requests when the first address is unreachable even though another allowed address would succeed. Iterate through resolved IPs (or use a dial strategy with fallback) instead of pinning to only the first entry.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| func validateAPICallRequestURL(reqURL *url.URL) error { | ||
| if errValidate := validateAPICallURL(reqURL); errValidate != nil { | ||
| return errValidate | ||
| } | ||
| return validateResolvedHostIPs(reqURL.Hostname()) | ||
| } |
There was a problem hiding this comment.
Suggestion: URL validation in the round-tripper triggers host resolution through helpers that use background context, so DNS lookup is not tied to request cancellation or client timeout. Under slow/hanging DNS, requests can block longer than intended. Plumb req.Context() into host-resolution validation so cancellation and deadlines are respected. [possible bug]
Severity Level: Major ⚠️
- ❌ /v0/management/api-call may hang on slow DNS.
- ⚠️ Copilot quota enrichment can exceed caller deadlines.
- ⚠️ Request timeout configuration not fully enforced for DNS.Steps of Reproduction ✅
1. Start cliproxyapi++ so the management handler `APICall` in
`pkg/llmproxy/api/handlers/management/api_call.go:86` is serving `POST
/v0/management/api-call`, and note the HTTP client timeout `defaultAPICallTimeout` used in
`apiCallHTTPClient` (`http_transport.go:53-55`).
2. Misconfigure system DNS (e.g., `/etc/resolv.conf`) or the target hostname so lookups
for some allowed host (e.g. `https://safe.example.com/ping`) hang or are extremely slow
when `net.DefaultResolver.LookupIPAddr` is called.
3. From a management client, issue `POST /v0/management/api-call` with a JSON body whose
`url` points to `https://safe.example.com/ping`; the handler sanitizes the URL and already
calls `validateResolvedHostIPs(parsedURL.Hostname())` (`api_call.go:13-20`), which
internally calls `resolveAllowedAPICallHostIPs` using `context.Background()`
(`api_call_url.go:62-73`), so this first DNS pass is already uncancelable by the request
context.
4. Next, `APICall` builds an HTTP client via `apiCallHTTPClient`
(`http_transport.go:53-56`); when `httpClient.Do(req)` runs, the transport
`apiCallGuardedRoundTripper` executes `RoundTrip` (`api_call_guard.go:31-42`), which again
calls `validateAPICallRequestURL` (`api_call_guard.go:45-49`) and therefore
`validateResolvedHostIPs(reqURL.Hostname())` (`api_call_guard.go:49`,
`api_call_url.go:62-73`), performing DNS resolution with `context.Background()` inside
`resolveAllowedAPICallHostIPs`.
5. Because `resolveAllowedAPICallHostIPs` uses `context.Background()` rather than
`req.Context()` or the client's timeout context, the APICall goroutine blocks inside
`LookupIPAddr` past the configured `defaultAPICallTimeout`, causing the
`/v0/management/api-call` request to hang until OS-level DNS timeouts elapse instead of
respecting the request deadline.
6. The same uncancelable DNS behavior appears in other call sites like Copilot quota
enrichment (`copilot_quota.go:15-19`), which also calls
`validateResolvedHostIPs(parsedQuotaURL.Hostname())` and thus is vulnerable to hanging
beyond its provided `ctx` deadline.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** pkg/llmproxy/api/handlers/management/api_call_guard.go
**Line:** 45:50
**Comment:**
*Possible Bug: URL validation in the round-tripper triggers host resolution through helpers that use background context, so DNS lookup is not tied to request cancellation or client timeout. Under slow/hanging DNS, requests can block longer than intended. Plumb `req.Context()` into host-resolution validation so cancellation and deadlines are respected.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| } | ||
| clone := transport.Clone() | ||
| clone.Proxy = nil | ||
| clone.DialContext = guardedAPICallDialContext |
There was a problem hiding this comment.
Suggestion: Routing all direct connections through guardedAPICallDialContext introduces a single-address dial path (it resolves and dials only one IP), which can fail on multi-record hosts when the first returned address is unreachable (common with dual-stack IPv6/IPv4 setups). This causes intermittent request failures even when other resolved addresses are healthy; iterate through allowed resolved IPs or preserve fallback dialing behavior. [logic error]
Severity Level: Major ⚠️
- ❌ Management APICall endpoint may fail on dual-stack hosts.
- ⚠️ Copilot quota enrichment requests can intermittently fail outbound.
- ⚠️ Loss of Go net/http multi-address fallback behavior.Steps of Reproduction ✅
1. Run cliproxyapi++ so the management endpoint `POST /v0/management/api-call` is served
by `Handler.APICall` in `pkg/llmproxy/api/handlers/management/api_call.go:86-130`.
2. From a client, issue a `POST /v0/management/api-call` request (as in
`api_tools_cbor_test.go:20-39`) with a JSON body whose `"url"` points to an HTTPS API host
that resolves to multiple IPs (A/AAAA) where the first returned IP is unreachable from
this environment (e.g. IPv6-only address on an IPv4-only network).
3. The handler builds an `http.Client` via `h.apiCallHTTPClient(auth)` at
`api_call.go:204`, which uses `apiCallTransport` in `http_transport.go:17-50`; for the
direct-connect path (no proxy), `apiCallTransport` clones `http.DefaultTransport`,
disables `Proxy`, and sets `clone.DialContext = guardedAPICallDialContext` at
`http_transport.go:47-49`.
4. `guardedAPICallDialContext` in `api_call_guard.go:11-25` resolves allowed IPs with
`resolveAllowedAPICallHostIPs` and then always dials only `resolved[0]`
(`net.JoinHostPort(resolved[0].IP.String(), port)`), so if that first IP is unreachable
the dial fails immediately with no attempt to try `resolved[1:]`; `httpClient.Do(req)` at
`api_call.go:204-209` returns an error and the endpoint responds `502` (`"request
failed"`), even though other resolved IPs could have succeeded.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** pkg/llmproxy/api/handlers/management/http_transport.go
**Line:** 49:49
**Comment:**
*Logic Error: Routing all direct connections through `guardedAPICallDialContext` introduces a single-address dial path (it resolves and dials only one IP), which can fail on multi-record hosts when the first returned address is unreachable (common with dual-stack IPv6/IPv4 setups). This causes intermittent request failures even when other resolved addresses are healthy; iterate through allowed resolved IPs or preserve fallback dialing behavior.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
CodeAnt AI finished running the review. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
CodeAnt AI is running the review. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
size:L
Sequence DiagramThis PR hardens the management API outbound call path by validating each request and redirect URL and pinning dials to vetted IPs, and it updates the Cursor executor to build completion and stream JSON payloads via a JSON encoder to prevent injection. sequenceDiagram
participant Client
participant LLMProxy
participant GuardedHTTPClient
participant ExternalAPI
participant CursorExecutor
== Management API call hardening ==
Client->>LLMProxy: Management API call request
LLMProxy->>GuardedHTTPClient: Send outbound request via guarded client
GuardedHTTPClient->>GuardedHTTPClient: Validate request URL and each redirect
GuardedHTTPClient->>GuardedHTTPClient: Resolve host and reject unsafe IPs
GuardedHTTPClient->>ExternalAPI: Send request to validated IP
ExternalAPI-->>GuardedHTTPClient: Return response
GuardedHTTPClient-->>LLMProxy: Forward validated response
LLMProxy-->>Client: Return API call result
== Cursor JSON encoding hardening ==
Client->>LLMProxy: Cursor chat or stream request
LLMProxy->>CursorExecutor: Execute Cursor session
CursorExecutor->>CursorExecutor: Build completion and stream JSON with JSON encoder
CursorExecutor-->>Client: Return safely encoded JSON payloads
Generated by CodeAnt AI |
| resolved, errResolve := resolveAllowedAPICallHostIPs(host) | ||
| if errResolve != nil { | ||
| return nil, errResolve | ||
| } | ||
| if len(resolved) == 0 { | ||
| return nil, fmt.Errorf("target host resolution failed") | ||
| } | ||
| dialer := &net.Dialer{} | ||
| return dialer.DialContext(ctx, network, net.JoinHostPort(resolved[0].IP.String(), port)) |
There was a problem hiding this comment.
🟠 Architect Review — HIGH
The guarded dialer resolves all allowed IPs for a host but always dials only resolved[0], assuming the first DNS result is reachable. On common dual-stack or multi-address setups (e.g., unreachable IPv6 first, reachable IPv4 second), management API calls will fail even though the target host is valid and reachable, regressing the default transport's multi-address fallback behavior.
Suggestion: Keep the SSRF/IP-allowlist enforcement but iterate (or otherwise perform happy-eyeballs over) the full allowed IP list, attempting connection to each in turn or with parallel race, instead of hard-pinning the dial to only the first resolved address.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** pkg/llmproxy/api/handlers/management/api_call_guard.go
**Line:** 16:24
**Comment:**
*HIGH: The guarded dialer resolves all allowed IPs for a host but always dials only resolved[0], assuming the first DNS result is reachable. On common dual-stack or multi-address setups (e.g., unreachable IPv6 first, reachable IPv4 second), management API calls will fail even though the target host is valid and reachable, regressing the default transport's multi-address fallback behavior.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
CodeAnt AI finished running the review. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
CodeAnt AI is running the review. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
size:L
Sequence DiagramThis diagram shows how the Cursor executor now uses JSON helpers to safely encode responses and how the management API uses a guarded HTTP client to validate each outbound request URL and target IP before dialing. sequenceDiagram
participant CursorClient
participant CursorExecutor
participant AdminClient
participant ManagementAPI
participant GuardedHTTPClient
participant ExternalAPI
CursorClient->>CursorExecutor: Send chat request
CursorExecutor->>CursorExecutor: Build JSON completion and chunks with helpers
CursorExecutor-->>CursorClient: Return safely encoded Cursor response
AdminClient->>ManagementAPI: Send management API call
ManagementAPI->>GuardedHTTPClient: Forward request via guarded transport
GuardedHTTPClient->>GuardedHTTPClient: Validate URL, redirects and resolved host IP
GuardedHTTPClient->>ExternalAPI: Send request to validated address
ExternalAPI-->>AdminClient: Response via ManagementAPI
Generated by CodeAnt AI |
|
CodeAnt AI finished running the review. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
CodeAnt AI is running the review. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
size:L
Sequence DiagramThis PR hardens the management API call path by validating outbound URLs, DNS resolution, and redirects, and updates the Cursor executor to build completion and streaming JSON responses with structured encoding to prevent injection. sequenceDiagram
participant Client
participant ManagementAPI
participant GuardedHTTPClient
participant TargetService
participant CursorClient
participant CursorExecutor
participant CursorJSONHelper
Client->>ManagementAPI: Send management API call
ManagementAPI->>GuardedHTTPClient: Execute outbound request
GuardedHTTPClient->>TargetService: Resolve host validate IP and redirects then send request
TargetService-->>GuardedHTTPClient: Return response
GuardedHTTPClient-->>Client: Proxied response via management API
CursorClient->>CursorExecutor: Request completion or stream
CursorExecutor->>CursorJSONHelper: Build completion and stream chunks with JSON encoder
CursorJSONHelper-->>CursorExecutor: Escaped JSON payloads
CursorExecutor-->>CursorClient: Return completion or SSE stream
Generated by CodeAnt AI |
|
CodeAnt AI finished running the review. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
CodeAnt AI is running the review. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
size:L
Sequence DiagramThis diagram shows how the Cursor executor now builds JSON responses with helpers to prevent injection, and how management API calls use a guarded HTTP client that validates each outbound URL and pins requests to safe host IPs. sequenceDiagram
participant CursorClient
participant CursorExecutor
participant ManagementClient
participant ManagementAPI
participant GuardedHTTPClient
participant ExternalService
CursorClient->>CursorExecutor: Send completion or stream request
CursorExecutor->>CursorExecutor: Build JSON responses with cursor helpers
CursorExecutor-->>CursorClient: Return safe cursor response
ManagementClient->>ManagementAPI: Send management API call with target URL
ManagementAPI->>GuardedHTTPClient: Execute request via guarded client
GuardedHTTPClient->>GuardedHTTPClient: Validate URL and host IP
GuardedHTTPClient->>ExternalService: Send request to validated IP
ExternalService-->>GuardedHTTPClient: Response
GuardedHTTPClient-->>ManagementAPI: Forward response or validation error
ManagementAPI-->>ManagementClient: Return result
Generated by CodeAnt AI |
| allowed := make([]net.IPAddr, 0, len(resolved)) | ||
| for _, ip := range resolved { | ||
| if ip == nil { | ||
| if ip.IP == nil { | ||
| continue | ||
| } | ||
| if ip.IsLoopback() || ip.IsPrivate() || ip.IsUnspecified() || ip.IsMulticast() || ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() { | ||
| return fmt.Errorf("target host is not allowed") | ||
| if ip.IP.IsLoopback() || ip.IP.IsPrivate() || ip.IP.IsUnspecified() || ip.IP.IsMulticast() || ip.IP.IsLinkLocalUnicast() || ip.IP.IsLinkLocalMulticast() { | ||
| return nil, fmt.Errorf("target host is not allowed") | ||
| } | ||
| allowed = append(allowed, ip) | ||
| } | ||
| return nil | ||
| if len(allowed) == 0 { | ||
| return nil, fmt.Errorf("target host resolution failed") | ||
| } | ||
| return allowed, nil |
There was a problem hiding this comment.
Suggestion: This helper preserves resolver order and returns all allowed IPs as-is, but the dial guard consumes only the first returned address; on dual-stack hosts this can select an unreachable family (commonly IPv6-first), causing intermittent connection failures even when another resolved address is healthy. Return a preferred IP for the requested network or provide ordered/fallback candidates that the dial path can iterate. [logic error]
Severity Level: Major ⚠️
- ❌ /v0/management/api-call may fail for dual-stack hosts.
- ⚠️ Upstream services reachable via IPv4-only become intermittently unreachable.
- ⚠️ Admin tooling relying on api-call experiences avoidable connection errors.Steps of Reproduction ✅
1. Configure DNS so that a target hostname (e.g. `api.example.com`) used with the
management endpoint `POST /v0/management/api-call` (documented and implemented in
`pkg/llmproxy/api/handlers/management/api_call.go:32-85`) resolves to multiple IPs, such
as an unreachable IPv6 address followed by a reachable IPv4 address (dual-stack,
IPv6-first).
2. Send a management request to `/v0/management/api-call` with
`"url":"https://api.example.com/path"` so that `APICall` builds an outbound HTTP request
and HTTP client via `httpClient := h.apiCallHTTPClient(auth)` at
`pkg/llmproxy/api/handlers/management/api_call.go:25-26`.
3. `apiCallHTTPClient` in `pkg/llmproxy/api/handlers/management/http_transport.go:53-63`
constructs an `http.Client` whose `Transport` is an `apiCallGuardedRoundTripper` wrapping
a transport whose `DialContext` has been overridden to `guardedAPICallDialContext` at
`http_transport.go:47-50`.
4. When `httpClient.Do(req)` runs, the transport calls `guardedAPICallDialContext` in
`pkg/llmproxy/api/handlers/management/api_call_guard.go:11-25`, which resolves the
hostname via `resolveAllowedAPICallHostIPs(host)` (in `api_call_url.go:67-90`) and then
always dials `resolved[0].IP` at `api_call_guard.go:23-24`, ignoring any additional
resolved addresses.
5. On a dual-stack host where the first resolved IP (e.g. IPv6) is unreachable but a later
address (e.g. IPv4) is reachable, the dial attempt using only `resolved[0].IP` fails,
causing `httpClient.Do(req)` to return an error and `APICall` to respond with `HTTP 502`
`"request failed"` at `api_call.go:25-30`, even though a healthy alternative IP address
was available in the DNS response.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** pkg/llmproxy/api/handlers/management/api_call_url.go
**Line:** 76:89
**Comment:**
*Logic Error: This helper preserves resolver order and returns all allowed IPs as-is, but the dial guard consumes only the first returned address; on dual-stack hosts this can select an unreachable family (commonly IPv6-first), causing intermittent connection failures even when another resolved address is healthy. Return a preferred IP for the requested network or provide ordered/fallback candidates that the dial path can iterate.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
CodeAnt AI finished running the review. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
1 participant
User description
Summary
Fixes the critical CodeQL findings for unsafe JSON construction in Cursor responses and request-forgery hardening in the management API call path.
Root Cause
Changes
Validation
Full package validation remains blocked by existing unrelated compile errors:
Note
High Risk
Touches security-sensitive outbound request handling and response encoding, adding DNS pinning and redirect validation that could block previously-working destinations or change network behavior. Also changes Cursor streaming/completion JSON construction, which could affect client compatibility if payload shapes differ.
Overview
Hardens the management
api-callpath against SSRF by introducing a guarded HTTP client that re-validates URLs on every request and redirect, and a dial guard that resolves hosts, rejects private/local/link-local IPs, and dials a validated IP (reducing DNS-rebinding risk).Replaces Cursor executor JSON payload construction (completions, stream chunks, tool-call deltas, usage chunks) from
fmt.Sprintfstring interpolation toencoding/jsonhelpers, with added regression tests to ensure untrusted fields can’t inject malformed/top-level JSON.Reviewed by Cursor Bugbot for commit 5d88d7d. Bugbot is set up for automated code reviews on this repo. Configure here.
CodeAnt-AI Description
Harden Cursor responses and management API requests against malformed and unsafe URLs
What Changed
Impact
✅ Fewer broken Cursor responses✅ Lower risk of JSON injection in streamed output✅ Fewer SSRF-style management API requests✅ Safer redirects to external APIs🔄 Retrigger CodeAnt AI Review
Details
💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.