fix(main): address medium-severity security findings#15
Conversation
- Bound HTTP response reads to 1 MB via io.LimitReader to prevent unbounded memory allocation - Replace configInitialized bool with sync.Once to eliminate potential data race - Warn when client secret is passed via CLI flag as it may be visible in process listings Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR hardens the CLI against medium-severity security findings by bounding HTTP response reads, removing a theoretical config init data race, and adding user-facing guidance about insecure secret handling.
Changes:
- Cap all
io.ReadAll(resp.Body)calls to 1 MiB viaio.LimitReaderto prevent unbounded memory allocation. - Replace
configInitializedwithsync.Onceto makeinitConfig()concurrency-safe. - Warn when
-client-secretis provided via CLI flags due to potential exposure via process listings.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
main.go
Outdated
| tokenExchangeTimeout = 10 * time.Second | ||
| tokenVerificationTimeout = 10 * time.Second | ||
| refreshTokenTimeout = 10 * time.Second | ||
| maxResponseSize = 1 << 20 // 1 MB |
There was a problem hiding this comment.
maxResponseSize is defined as 1<<20 (1,048,576 bytes), which is 1 MiB rather than 1 MB. Either adjust the comment to say 1 MiB, or use a decimal value (e.g. 1_000_000) if you intend an SI MB limit.
| maxResponseSize = 1 << 20 // 1 MB | |
| maxResponseSize = 1 << 20 // 1 MiB |
main.go
Outdated
| body, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseSize)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to read response: %w", err) | ||
| } |
There was a problem hiding this comment.
Using io.ReadAll(io.LimitReader(..., maxResponseSize)) will silently truncate oversized responses; the resulting error will likely be a JSON parse failure rather than a clear "response too large" signal. Consider reading maxResponseSize+1 bytes and returning an explicit error when the limit is exceeded.
| body, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseSize)) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to read response: %w", err) | |
| } | |
| body, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseSize+1)) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to read response: %w", err) | |
| } | |
| if int64(len(body)) > maxResponseSize { | |
| return nil, fmt.Errorf("response too large") | |
| } |
main.go
Outdated
| body, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseSize)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to read response: %w", err) | ||
| } |
There was a problem hiding this comment.
Using io.ReadAll(io.LimitReader(..., maxResponseSize)) will silently truncate oversized responses; the resulting error will likely be a JSON parse failure rather than a clear "response too large" signal. Consider reading maxResponseSize+1 bytes and returning an explicit error when the limit is exceeded.
| body, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseSize)) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to read response: %w", err) | |
| } | |
| body, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseSize+1)) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to read response: %w", err) | |
| } | |
| if int64(len(body)) > maxResponseSize { | |
| return nil, fmt.Errorf("response too large: limit %d bytes", maxResponseSize) | |
| } |
main.go
Outdated
| body, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseSize)) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to read response: %w", err) |
There was a problem hiding this comment.
Using io.ReadAll(io.LimitReader(..., maxResponseSize)) will silently truncate oversized responses; the resulting error will likely be a JSON parse failure rather than a clear "response too large" signal. Consider reading maxResponseSize+1 bytes and returning an explicit error when the limit is exceeded.
main.go
Outdated
| body, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseSize)) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to read response: %w", err) |
There was a problem hiding this comment.
Using io.ReadAll(io.LimitReader(..., maxResponseSize)) will silently truncate oversized responses; the resulting error will likely be a JSON parse failure rather than a clear "response too large" signal. Consider reading maxResponseSize+1 bytes and returning an explicit error when the limit is exceeded.
main.go
Outdated
| body, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseSize)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to read response: %w", err) | ||
| } |
There was a problem hiding this comment.
New security behavior (capping response reads) is introduced here, but there are currently no unit tests covering the oversized-response case. Consider adding httptest-based tests that return >maxResponseSize bodies and assert you get a deterministic "response too large"-style error (rather than a JSON parse error).
| body, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseSize)) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to read response: %w", err) | |
| } | |
| body, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseSize+1)) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to read response: %w", err) | |
| } | |
| if int64(len(body)) > maxResponseSize { | |
| return nil, fmt.Errorf("failed to read response: response body exceeds maximum allowed size of %d bytes", maxResponseSize) | |
| } |
| func doInitConfig() { | ||
| flag.Parse() | ||
|
|
||
| serverURL = getConfig(*flagServerURL, "SERVER_URL", "http://localhost:8080") | ||
| clientID = getConfig(*flagClientID, "CLIENT_ID", "") | ||
| clientSecret = getConfig(*flagClientSecret, "CLIENT_SECRET", "") | ||
| if *flagClientSecret != "" { | ||
| configWarnings = append(configWarnings, | ||
| "Client secret passed via command-line flag. "+ | ||
| "This may be visible in process listings. "+ | ||
| "Consider using CLIENT_SECRET env var or .env file instead.") | ||
| } |
There was a problem hiding this comment.
A new warning is added when -client-secret is provided, but there are no tests validating that configWarnings includes this message when the flag is set. Consider factoring the warning decision into a small helper (or resetting flag.CommandLine in tests) so this behavior is covered and doesn’t regress.
| func doInitConfig() { | |
| flag.Parse() | |
| serverURL = getConfig(*flagServerURL, "SERVER_URL", "http://localhost:8080") | |
| clientID = getConfig(*flagClientID, "CLIENT_ID", "") | |
| clientSecret = getConfig(*flagClientSecret, "CLIENT_SECRET", "") | |
| if *flagClientSecret != "" { | |
| configWarnings = append(configWarnings, | |
| "Client secret passed via command-line flag. "+ | |
| "This may be visible in process listings. "+ | |
| "Consider using CLIENT_SECRET env var or .env file instead.") | |
| } | |
| // computeConfigWarningsFromFlags returns configuration warnings derived solely | |
| // from command-line flag values. This is separated from doInitConfig so that | |
| // the warning behavior can be unit tested without relying on global flag state. | |
| func computeConfigWarningsFromFlags(clientSecretFlag string) []string { | |
| var warnings []string | |
| if clientSecretFlag != "" { | |
| warnings = append(warnings, | |
| "Client secret passed via command-line flag. "+ | |
| "This may be visible in process listings. "+ | |
| "Consider using CLIENT_SECRET env var or .env file instead.") | |
| } | |
| return warnings | |
| } | |
| func doInitConfig() { | |
| flag.Parse() | |
| serverURL = getConfig(*flagServerURL, "SERVER_URL", "http://localhost:8080") | |
| clientID = getConfig(*flagClientID, "CLIENT_ID", "") | |
| clientSecret = getConfig(*flagClientSecret, "CLIENT_SECRET", "") | |
| configWarnings = append(configWarnings, computeConfigWarningsFromFlags(*flagClientSecret)...) |
The lint job referenced `make generate` which has no corresponding Makefile target, causing CI to fail. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Address Copilot review feedback: - Fix comment from "1 MB" to "1 MiB" (1<<20 is a mebibyte) - Read maxResponseSize+1 bytes and return a clear "response too large" error instead of silently truncating (which caused confusing JSON parse errors) - Extract readResponseBody helper to deduplicate the pattern - Add tests for readResponseBody (within limit, at limit, exceeds limit) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
io.ReadAll(resp.Body)calls withio.LimitReader(1 MB cap) to prevent unbounded memory allocation from malicious/oversized responsesconfigInitializedbool withsync.Onceto eliminate a theoretical data race on concurrentinitConfig()calls-client-secretCLI flag, as it may be visible in process listings (ps,/proc)Test plan
make test— all tests passmake lint— no issues🤖 Generated with Claude Code