fix: address all high/medium/low issues from code review - #5
fix: address all high/medium/low issues from code review#5proxy-turkey wants to merge 3 commits into
Conversation
High: - Guard all 5 context type assertions with ccAPIKeyFromContext helper (handlers.go) — prevents panic if middleware ordering changes - Handle rand.Read error in randomHex, fall back to timestamp (converter.go) Medium: - Replace global consecutiveTimeouts with per-session sync.Map keyed by sessionID — prevents timeout counter leaking across users - Guard Choices[0] index access with length check (handlers.go) - Make CORS origin configurable instead of hardcoded "*" (middleware.go) - Build settings JSON via python3 json.dumps in claude-proxy.sh to prevent injection if token contains quotes/backslashes - Add config package tests: missing file, permission denied, invalid JSON, valid JSON, env overrides, validation (config_test.go) Low: - Merge double message iteration into single pass (converter.go) - Extract buildCommandCodeConfig helper to eliminate duplicated 14-field Config block across both converters - Fix snake_case cc_apiKey param to ccAPIKey across client.go/init.go - Replace header blocklist with allowlist (anthropic-* + standard) for safer upstream forwarding (client.go) - Use custom http.Transport with connection pooling instead of http.Client timeout that kills long streaming responses (client.go) - Fix modelRefreshIntervalMs JSON tag to modelRefreshInterval (was silently treated as nanoseconds instead of duration)
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several improvements, including safer JSON construction in claude-proxy.sh using Python, session-specific timeout tracking in HTTP handlers to prevent cross-user state leakage, and an allowlist-based header forwarding mechanism in the client. It also refactors configuration metadata building and adds a comprehensive test suite for configuration loading. Feedback on these changes highlights a few critical issues: the custom http.Transport should clone http.DefaultTransport to retain essential default settings like dial timeouts, and the new configuration tests should be updated to use t.TempDir() instead of hardcoded /tmp paths and platform-specific calls like os.Getuid() to ensure cross-platform compatibility on Windows.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| transport := &http.Transport{ | ||
| MaxIdleConns: 20, | ||
| MaxIdleConnsPerHost: 10, | ||
| IdleConnTimeout: 90 * time.Second, | ||
| } |
There was a problem hiding this comment.
Creating a raw &http.Transport{} from scratch without copying http.DefaultTransport or configuring a custom DialContext means you lose critical default settings. Specifically, you lose the default 30-second dial timeout, keep-alive settings, TLS handshake timeouts, and system proxy support. This can cause connection attempts to hang indefinitely under poor network conditions.
Instead, clone http.DefaultTransport and customize only the connection pooling fields.
transport := http.DefaultTransport.(*http.Transport).Clone()
transport.MaxIdleConns = 20
transport.MaxIdleConnsPerHost = 10
transport.IdleConnTimeout = 90 * time.Second| func TestLoadFailsOnPermissionDenied(t *testing.T) { | ||
| dir := t.TempDir() | ||
| path := filepath.Join(dir, "config.json") | ||
| if err := os.WriteFile(path, []byte(`{"port":3050}`), 0644); err != nil { | ||
| t.Fatalf("setup: %v", err) | ||
| } | ||
| if err := os.Chmod(path, 0000); err != nil { | ||
| t.Fatalf("chmod: %v", err) | ||
| } | ||
| t.Cleanup(func() { os.Chmod(path, 0644) }) | ||
|
|
||
| // Skip if running as root (root bypasses file permissions). | ||
| if os.Getuid() == 0 { | ||
| t.Skip("running as root, permission test not applicable") | ||
| } | ||
|
|
||
| _, err := Load(path) | ||
| if err == nil { | ||
| t.Fatal("Load() error = nil, want error for unreadable file") | ||
| } | ||
| } |
There was a problem hiding this comment.
os.Getuid() is not supported on Windows, which will cause this test file to fail compilation on Windows platforms. Additionally, os.Chmod(path, 0000) does not reliably make files unreadable on Windows.
To test the read failure path in a fully cross-platform and robust manner without needing root/administrator checks, you can pass a directory path (like t.TempDir()) to Load(). Attempting to read a directory as a file will fail with a read error on all operating systems.
func TestLoadFailsOnReadError(t *testing.T) {
dir := t.TempDir()
_, err := Load(dir)
if err == nil {
t.Fatal("Load() error = nil, want error when path is a directory")
}
}
|
@proxy-turkey gemini'nin önerilerini kontrol ettin mi? |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
2 participants
Summary
Addresses every high, medium, and low priority issue identified in the post-merge code review of v1.2.0.
High Severity (2 fixes)
handlers.go): All 5r.Context().Value("ccAPIKey").(string)calls replaced withccAPIKeyFromContext()helper using safe two-value assertion — prevents panic if middleware ordering changes.rand.Readerror (converter.go):randomHexnow checks the error and falls back to a timestamp-based ID instead of silently returning all-zeros.Medium Severity (5 fixes)
handlers.go): GlobalconsecutiveTimeoutsatomic replaced withsync.Mapkeyed by sessionID — prevents one user's timeout from triggering "reduce context" hints for a different user.Choices[0]access (handlers.go): Added length check before indexing — prevents panic if upstream returns zero choices.middleware.go):CORSnow accepts anallowedOriginparameter instead of hardcoding"*".claude-proxy.sh): Settings JSON now built viapython3 json.dumps()with bash escaping fallback — prevents injection if the token contains quotes or backslashes.config_test.go): 8 new test cases covering missing file, permission denied, invalid JSON, valid JSON, env overrides, validation, and defaults.Low Severity (5 fixes)
converter.go): OpenAI converter looped messages twice (convert + extract system); merged into single pass.buildCommandCodeConfighelper (converter.go): Eliminated ~30 lines of duplicated Config block across both OpenAI and Anthropic converters.client.go,init.go):cc_apiKey→ccAPIKeyacross 7 functions.client.go): Replaced blocklist with allowlist (anthropic-*,accept,user-agent) for safer upstream forwarding.client.go): Replacedhttp.Client{Timeout: 90s}with customhttp.Transport+ connection pooling — prevents long streaming responses from being killed.config.go):modelRefreshIntervalMs→modelRefreshInterval(was silently treated as nanoseconds).Verification
go build ./...✅go vet ./...✅go test ./... -count=1✅ (52 tests pass)ccpinteractive multi-turn tool use: ✅Files changed
internal/protocol/converter.gointernal/http/handlers.gointernal/http/middleware.gointernal/http/server.gointernal/http/handlers_test.gointernal/client/client.gointernal/client/init.gointernal/config/config.gointernal/config/config_test.goclaude-proxy.sh