[codex] fix CodeQL URL and Cursor JSON hardening #963
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| package management | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
| "net" | ||
| "net/http" | ||
| "net/url" | ||
| ) | ||
|
|
||
| func guardedAPICallDialContext(ctx context.Context, network string, addr string) (net.Conn, error) { | ||
| host, port, errSplit := net.SplitHostPort(addr) | ||
| if errSplit != nil { | ||
| return nil, fmt.Errorf("invalid dial address: %w", errSplit) | ||
| } | ||
| 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)) | ||
|
Comment on lines
+20
to
+24
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
|
||
| } | ||
|
|
||
| type apiCallGuardedRoundTripper struct { | ||
| base http.RoundTripper | ||
| } | ||
|
|
||
| func (t apiCallGuardedRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { | ||
| if req == nil { | ||
| return nil, fmt.Errorf("request is nil") | ||
| } | ||
| if errValidate := validateAPICallRequestURL(req.URL); errValidate != nil { | ||
| return nil, errValidate | ||
| } | ||
| base := t.base | ||
| if base == nil { | ||
| base = http.DefaultTransport | ||
| } | ||
| return base.RoundTrip(req) | ||
| } | ||
|
|
||
| func validateAPICallRequestURL(reqURL *url.URL) error { | ||
| if errValidate := validateAPICallURL(reqURL); errValidate != nil { | ||
| return errValidate | ||
| } | ||
| return validateResolvedHostIPs(reqURL.Hostname()) | ||
| } | ||
|
Comment on lines
+45
to
+50
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 Severity Level: Major
|
||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| package management | ||
|
|
||
| import ( | ||
| "errors" | ||
| "net/http" | ||
| "net/url" | ||
| "testing" | ||
| ) | ||
|
|
||
| type apiCallRoundTripFunc func(*http.Request) (*http.Response, error) | ||
|
|
||
| func (f apiCallRoundTripFunc) RoundTrip(req *http.Request) (*http.Response, error) { | ||
| return f(req) | ||
| } | ||
|
|
||
| func TestAPICallGuardedRoundTripperRejectsUnsafeRequestURL(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| called := false | ||
| transport := apiCallGuardedRoundTripper{ | ||
| base: apiCallRoundTripFunc(func(*http.Request) (*http.Response, error) { | ||
| called = true | ||
| return nil, errors.New("base transport should not run") | ||
| }), | ||
| } | ||
|
|
||
| req, err := http.NewRequest(http.MethodGet, "http://127.0.0.1:8317/ping", nil) | ||
| if err != nil { | ||
| t.Fatalf("new request: %v", err) | ||
| } | ||
| if _, err := transport.RoundTrip(req); err == nil { | ||
| t.Fatalf("RoundTrip error = nil, want unsafe target rejection") | ||
| } | ||
| if called { | ||
| t.Fatal("base transport ran for unsafe URL") | ||
| } | ||
| } | ||
|
|
||
| func TestAPICallRequestURLValidationRejectsUnsafeRedirectURL(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| redirectURL, err := url.Parse("http://localhost:8317/ping") | ||
| if err != nil { | ||
| t.Fatalf("parse redirect url: %v", err) | ||
| } | ||
| req := &http.Request{URL: redirectURL} | ||
| if err := validateAPICallRequestURL(req.URL); err == nil { | ||
| t.Fatalf("validation error = nil, want unsafe redirect rejection") | ||
| } | ||
| } |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| package management | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
| "net" | ||
| "net/url" | ||
|
|
@@ -59,23 +60,33 @@ func sanitizeAPICallURL(raw string) (string, *url.URL, error) { | |
| } | ||
|
|
||
| func validateResolvedHostIPs(host string) error { | ||
| _, err := resolveAllowedAPICallHostIPs(host) | ||
| return err | ||
| } | ||
|
|
||
| func resolveAllowedAPICallHostIPs(host string) ([]net.IPAddr, error) { | ||
| trimmed := strings.TrimSpace(host) | ||
| if trimmed == "" { | ||
| return nil, fmt.Errorf("invalid url host") | ||
| } | ||
| resolved, errLookup := net.DefaultResolver.LookupIPAddr(context.Background(), trimmed) | ||
|
There was a problem hiding this comment. Suggestion: DNS resolution is performed with Severity Level: Major
|
||
| if errLookup != nil { | ||
| return nil, fmt.Errorf("target host resolution failed") | ||
| } | ||
| allowed := make([]net.IPAddr, 0, len(resolved)) | ||
| for _, ip := range resolved { | ||
| if ip.IP == nil { | ||
| continue | ||
| } | ||
| 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) | ||
| } | ||
| 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 resolutionLow Severity
Additional Locations (1)  Reviewed by Cursor Bugbot for commit 5d88d7d. Configure here.
Comment on lines
+76
to
+89
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
|
||
| } | ||
|
|
||
| func isAllowedHostOverride(parsedURL *url.URL, override string) bool { | ||
|
|
||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,9 +46,23 @@ func (h *Handler) apiCallTransport(auth *coreauth.Auth) http.RoundTripper { | |
| } | ||
| clone := transport.Clone() | ||
| clone.Proxy = nil | ||
| clone.DialContext = guardedAPICallDialContext | ||
|
There was a problem hiding this comment. Suggestion: Routing all direct connections through Severity Level: Major
|
||
| return clone | ||
| } | ||
|
|
||
| func (h *Handler) apiCallHTTPClient(auth *coreauth.Auth) *http.Client { | ||
| return &http.Client{ | ||
| Timeout: defaultAPICallTimeout, | ||
| Transport: apiCallGuardedRoundTripper{base: h.apiCallTransport(auth)}, | ||
| CheckRedirect: func(req *http.Request, via []*http.Request) error { | ||
| if len(via) >= 10 { | ||
| return fmt.Errorf("stopped after 10 redirects") | ||
| } | ||
| return validateAPICallRequestURL(req.URL) | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| func buildProxyTransportWithError(proxyStr string) (*http.Transport, error) { | ||
| proxyStr = strings.TrimSpace(proxyStr) | ||
| if proxyStr == "" { | ||
|
|
||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -333,11 +333,13 @@ func (e *CursorExecutor) Execute(ctx context.Context, auth *cliproxyauth.Auth, r | |
|
|
||
| id := "chatcmpl-" + uuid.New().String()[:28] | ||
| created := time.Now().Unix() | ||
| openaiResp, errMarshal := cursorCompletionJSON(id, created, parsed.Model, fullText.String()) | ||
| if errMarshal != nil { | ||
| return resp, fmt.Errorf("cursor: failed to encode response: %w", errMarshal) | ||
|
Comment on lines
+336
to
+338
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 |
||
| } | ||
|
|
||
| // Translate response back to source format if needed | ||
| result := openaiResp | ||
| if from.String() != "" && from.String() != "openai" { | ||
| var param any | ||
| result = sdktranslator.TranslateNonStream(ctx, to, from, req.Model, bytes.Clone(opts.OriginalRequest), payload, result, ¶m) | ||
|
|
@@ -536,21 +538,21 @@ func (e *CursorExecutor) ExecuteStream(ctx context.Context, auth *cliproxyauth.A | |
|
|
||
| // Wrap sendChunk/sendDone to use emitToOut | ||
| sendChunkSwitchable := func(delta string, finishReason string) { | ||
| 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 | ||
| } | ||
| sseLine := append([]byte("data: "), openaiJSON...) | ||
| sseLine = append(sseLine, '\n') | ||
|
Comment on lines
+541
to
+547
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 needsTranslate { | ||
| translated := sdktranslator.TranslateStream(ctx, to, from, req.Model, originalPayload, payload, sseLine, &streamParam) | ||
| for _, t := range translated { | ||
| emitToOut(cliproxyexecutor.StreamChunk{Payload: bytes.Clone(t)}) | ||
| } | ||
| } else { | ||
| emitToOut(cliproxyexecutor.StreamChunk{Payload: openaiJSON}) | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -595,25 +597,24 @@ func (e *CursorExecutor) ExecuteStream(ctx context.Context, auth *cliproxyauth.A | |
| thinkingActive = true | ||
| sendChunkSwitchable(`{"role":"assistant","content":"<think>"}`, "") | ||
| } | ||
| sendChunkSwitchable(cursorContentDeltaJSON(text), "") | ||
| } else { | ||
| if thinkingActive { | ||
| thinkingActive = false | ||
| sendChunkSwitchable(`{"content":"</think>"}`, "") | ||
| } | ||
| sendChunkSwitchable(cursorContentDeltaJSON(text), "") | ||
| } | ||
| }, | ||
| func(exec pendingMcpExec) { | ||
| if thinkingActive { | ||
| thinkingActive = false | ||
| sendChunkSwitchable(`{"content":"</think>"}`, "") | ||
| } | ||
| toolCallJSON := cursorToolCallDeltaJSON(toolCallIndex, exec.ToolCallId, exec.ToolName, exec.Args) | ||
| toolCallIndex++ | ||
| sendChunkSwitchable(toolCallJSON, "") | ||
| sendChunkSwitchable(`{}`, "tool_calls") | ||
| sendDoneSwitchable() | ||
|
|
||
| // Close current output to end the current HTTP SSE response | ||
|
|
@@ -701,23 +702,22 @@ func (e *CursorExecutor) ExecuteStream(ctx context.Context, auth *cliproxyauth.A | |
| } | ||
| // Include token usage in the final stop chunk | ||
| inputTok, outputTok := usage.get() | ||
| openaiJSON, errMarshal := cursorUsageChunkJSON(chatId, created, parsed.Model, inputTok, outputTok) | ||
| if errMarshal != nil { | ||
| log.Warnf("cursor: failed to encode usage chunk: %v", errMarshal) | ||
| openaiJSON = []byte(`{"choices":[{"index":0,"delta":{},"finish_reason":"stop"}]}`) | ||
| } | ||
| sseLine := append([]byte("data: "), openaiJSON...) | ||
| sseLine = append(sseLine, '\n') | ||
| if needsTranslate { | ||
| translated := sdktranslator.TranslateStream(ctx, to, from, req.Model, originalPayload, payload, sseLine, &streamParam) | ||
| for _, t := range translated { | ||
| emitToOut(cliproxyexecutor.StreamChunk{Payload: bytes.Clone(t)}) | ||
| } | ||
| } else { | ||
| emitToOut(cliproxyexecutor.StreamChunk{Payload: openaiJSON}) | ||
| } | ||
| sendDoneSwitchable() | ||
|
|
||
| // Close whatever output channel is still active | ||
| outMu.Lock() | ||
|
|
@@ -1436,16 +1436,15 @@ func deriveSessionKey(clientKey string, model string, messages []gjson.Result) s | |
| } | ||
|
|
||
| func sseChunk(id string, created int64, model string, delta string, finishReason string) cliproxyexecutor.StreamChunk { | ||
| // Note: the framework's WriteChunk adds "data: " prefix and "\n\n" suffix, | ||
| // so we only output the raw JSON here. | ||
| data, err := cursorChunkJSON(id, created, model, json.RawMessage(delta), finishReason) | ||
| if err != nil { | ||
| log.Warnf("cursor: failed to encode sse chunk: %v", err) | ||
| data = []byte(`{"choices":[{"index":0,"delta":{},"finish_reason":null}]}`) | ||
| } | ||
| return cliproxyexecutor.StreamChunk{ | ||
|
There was a problem hiding this comment. Modified function
|
||
| Payload: data, | ||
| } | ||
| } | ||
|
|
||
|
|
||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| package executor | ||
|
|
||
| import ( | ||
| "encoding/json" | ||
| "testing" | ||
| ) | ||
|
|
||
| func TestCursorCompletionJSONEscapesModelAndContent(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| payload, err := cursorCompletionJSON("chatcmpl-test", 1700000000, `x","pwned":true,"y":"`, `hi "there"`) | ||
| if err != nil { | ||
| t.Fatalf("cursorCompletionJSON: %v", err) | ||
| } | ||
|
|
||
| var got map[string]any | ||
| if err := json.Unmarshal(payload, &got); err != nil { | ||
| t.Fatalf("unmarshal payload: %v; payload=%s", err, payload) | ||
| } | ||
| if got["model"] != `x","pwned":true,"y":"` { | ||
| t.Fatalf("model = %q", got["model"]) | ||
| } | ||
| if _, ok := got["pwned"]; ok { | ||
| t.Fatalf("payload allowed model to inject top-level field: %s", payload) | ||
| } | ||
| } | ||
|
|
||
| func TestCursorChunkJSONEscapesModelAndFinishReason(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| payload, err := cursorChunkJSON( | ||
| "chatcmpl-test", | ||
| 1700000000, | ||
| `x","pwned":true,"y":"`, | ||
| json.RawMessage(`{"content":"ok"}`), | ||
| `stop","pwned":true,"x":"`, | ||
| ) | ||
| if err != nil { | ||
| t.Fatalf("cursorChunkJSON: %v", err) | ||
| } | ||
|
|
||
| var got struct { | ||
| Model string `json:"model"` | ||
| Pwned bool `json:"pwned"` | ||
| Choices []struct { | ||
| FinishReason string `json:"finish_reason"` | ||
| } `json:"choices"` | ||
| } | ||
| if err := json.Unmarshal(payload, &got); err != nil { | ||
| t.Fatalf("unmarshal payload: %v; payload=%s", err, payload) | ||
| } | ||
| if got.Model != `x","pwned":true,"y":"` { | ||
| t.Fatalf("model = %q", got.Model) | ||
| } | ||
| if got.Pwned { | ||
| t.Fatalf("payload allowed model to inject top-level field: %s", payload) | ||
| } | ||
| if got.Choices[0].FinishReason != `stop","pwned":true,"x":"` { | ||
| t.Fatalf("finish_reason = %q", got.Choices[0].FinishReason) | ||
| } | ||
| } | ||
|
|
||
| func TestCursorToolCallDeltaJSONEscapesToolIdentifiers(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| payload := cursorToolCallDeltaJSON( | ||
| 0, | ||
| `call_1","pwned":true,"x":"`, | ||
| `tool","pwned":true,"x":"`, | ||
| `{"ok":true}`, | ||
| ) | ||
| var got map[string]any | ||
| if err := json.Unmarshal([]byte(payload), &got); err != nil { | ||
| t.Fatalf("unmarshal payload: %v; payload=%s", err, payload) | ||
| } | ||
| if _, ok := got["pwned"]; ok { | ||
| t.Fatalf("payload allowed tool metadata to inject top-level field: %s", payload) | ||
| } | ||
| } |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Since this PR modifies a function that is far longer than 40 lines, refactor
APICallby 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
APICallfunction 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 🤖