security: fix CodeQL SSRF and path injection alerts#854
Conversation
Break taint propagation chains so CodeQL can verify sanitization: - SSRF (go/request-forgery): reconstruct URL from validated components instead of reusing parsed URL string; use literal allowlisted hostnames in copilotQuotaURLFromTokenURL instead of fmt.Sprintf with variable - Path injection (go/path-injection): apply filepath.Clean at call sites in token_storage.go and vertex_credentials.go so static analysis sees sanitization in the same scope as the filesystem operations Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
📝 WalkthroughWalkthroughSecurity hardening across three modules: API URL handling receives DNS resolution and URL reconstruction improvements, while file path operations in authentication modules incorporate explicit filepath cleaning steps after validation for enhanced sanitization. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
pkg/llmproxy/api/handlers/management/api_call_url.go (1)
66-69:⚠️ Potential issue | 🟠 MajorAdd timeout to DNS resolution and address TOCTOU rebinding window.
Line 66 calls
net.LookupIP()without timeout or context, risking request blocking. More critically, validating DNS upfront offers no protection against rebinding—the actual HTTP dial path (viah.apiCallTransport()) does not pin or verify that the connection uses one of the resolved IPs, leaving a TOCTOU window for an attacker to rebind the hostname between validation and dial.Add bounded DNS lookup with context timeout (suggest 2 seconds). To close the rebinding window, the transport's
DialContextmust validate that the resolved address matches one of the IPs returned from this check, or implement connection-time re-validation.DNS lookup fix (addressing timeout only)
import ( + "context" "fmt" "net" "net/url" "strings" + "time" ) func validateResolvedHostIPs(host string) error { trimmed := strings.TrimSpace(host) if trimmed == "" { return fmt.Errorf("invalid url host") } - resolved, errLookup := net.LookupIP(trimmed) + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + resolved, errLookup := net.DefaultResolver.LookupIP(ctx, "ip", trimmed) if errLookup != nil { return fmt.Errorf("target host resolution failed") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/llmproxy/api/handlers/management/api_call_url.go` around lines 66 - 69, Replace the blind net.LookupIP call with a context-bounded lookup (e.g., context.WithTimeout(ctx, 2*time.Second)) and use a resolver API that accepts context to produce the resolved IP set (the current variables are resolved/errLookup/trimmed). Then modify the transport returned by h.apiCallTransport() so its DialContext consults that IP set: after dialing, extract the remote IP from the connection's RemoteAddr and verify it is one of the previously resolved IPs (close and return an error if not) to eliminate the TOCTOU rebind window; alternatively perform the verification inside DialContext before returning the connection. Ensure the lookup result (set of IPs) is passed into apiCallTransport/DialContext so it can perform this validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/llmproxy/api/handlers/management/api_call_url.go`:
- Around line 66-69: Replace the blind net.LookupIP call with a context-bounded
lookup (e.g., context.WithTimeout(ctx, 2*time.Second)) and use a resolver API
that accepts context to produce the resolved IP set (the current variables are
resolved/errLookup/trimmed). Then modify the transport returned by
h.apiCallTransport() so its DialContext consults that IP set: after dialing,
extract the remote IP from the connection's RemoteAddr and verify it is one of
the previously resolved IPs (close and return an error if not) to eliminate the
TOCTOU rebind window; alternatively perform the verification inside DialContext
before returning the connection. Ensure the lookup result (set of IPs) is passed
into apiCallTransport/DialContext so it can perform this validation.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 67070422-e219-4d5d-b164-54b3e8efdb0d
📒 Files selected for processing (3)
pkg/llmproxy/api/handlers/management/api_call_url.gopkg/llmproxy/auth/base/token_storage.gopkg/llmproxy/auth/vertex/vertex_credentials.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: go-ci
- GitHub Check: quality-ci
- GitHub Check: golangci-lint
- GitHub Check: lint-test
- GitHub Check: Analyze (Go) (go)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: NEVER create a v2 file - refactor the original instead
NEVER create a new class if an existing one can be made generic
NEVER create custom implementations when an OSS library exists - search pkg.go.dev for existing libraries before writing code
Build generic building blocks (provider interface + registry) before application logic
Use chi for HTTP routing (NOT custom routers)
Use zerolog for logging (NOT fmt.Print)
Use viper for configuration (NOT manual env parsing)
Use go-playground/validator for validation (NOT manual if/else validation)
Use golang.org/x/time/rate for rate limiting (NOT custom limiters)
Use template strings for messages instead of hardcoded messages and config-driven logic instead of code-driven
Zero new lint suppressions without inline justification
All new code must pass: go fmt, go vet, golint
Maximum function length: 40 lines
No placeholder TODOs in committed code
Files:
pkg/llmproxy/auth/base/token_storage.gopkg/llmproxy/api/handlers/management/api_call_url.gopkg/llmproxy/auth/vertex/vertex_credentials.go
🔇 Additional comments (4)
pkg/llmproxy/auth/base/token_storage.go (1)
60-66: Good hardening: validated path is consistently used at filesystem call sites.Line 60/103/123 now validates first, and Line 65/107/127 uses the cleaned path for all file operations. This is a solid security/readability improvement and matches the PR’s static-analysis goal.
Also applies to: 103-107, 123-127
pkg/llmproxy/auth/vertex/vertex_credentials.go (1)
48-53: Path sanitization flow is clearer and consistent.Using
validatedPathand thencleanPathmakes the sanitize-then-use flow explicit at the call site, which helps security tooling and code readability.pkg/llmproxy/api/handlers/management/api_call_url.go (2)
48-58: URL reconstruction from parsed components is a strong SSRF hardening step.Line 48-58 cleanly breaks taint propagation while preserving validated scheme/host/path/query semantics.
123-126: Nice improvement: explicit host-to-quota URL mapping.Line 123-126 removes ambiguity and keeps the quota endpoint derivation deterministic for the two allowed hosts.
1 participant
Summary
go/request-forgery) by reconstructing URLs from validated components and using literal allowlisted hostnames instead of string interpolation with tainted variablesgo/path-injection) by applyingfilepath.Cleanat call sites so CodeQL can verify sanitization in the same scope as filesystem operationsTest plan
go build ./...passesgo vet ./...passes🤖 Generated with Claude Code
Summary by CodeRabbit