Skip to content

fix(main): address medium-severity security findings#15

Merged
appleboy merged 3 commits intomainfrom
worktree-roktree
Mar 11, 2026
Merged

fix(main): address medium-severity security findings#15
appleboy merged 3 commits intomainfrom
worktree-roktree

Conversation

@appleboy
Copy link
Copy Markdown
Member

Summary

  • Bound all 4 io.ReadAll(resp.Body) calls with io.LimitReader (1 MB cap) to prevent unbounded memory allocation from malicious/oversized responses
  • Replace configInitialized bool with sync.Once to eliminate a theoretical data race on concurrent initConfig() calls
  • Add a warning when client secret is passed via -client-secret CLI flag, as it may be visible in process listings (ps, /proc)

Test plan

  • make test — all tests pass
  • make lint — no issues

🤖 Generated with Claude Code

- 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>
Copilot AI review requested due to automatic review settings March 11, 2026 14:53
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Mar 11, 2026

Codecov Report

❌ Patch coverage is 27.77778% with 13 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
main.go 27.77% 12 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 via io.LimitReader to prevent unbounded memory allocation.
  • Replace configInitialized with sync.Once to make initConfig() concurrency-safe.
  • Warn when -client-secret is 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
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
maxResponseSize = 1 << 20 // 1 MB
maxResponseSize = 1 << 20 // 1 MiB

Copilot uses AI. Check for mistakes.
main.go Outdated
Comment on lines 351 to 354
body, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseSize))
if err != nil {
return nil, fmt.Errorf("failed to read response: %w", err)
}
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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")
}

Copilot uses AI. Check for mistakes.
main.go Outdated
Comment on lines 415 to 418
body, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseSize))
if err != nil {
return nil, fmt.Errorf("failed to read response: %w", err)
}
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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)
}

Copilot uses AI. Check for mistakes.
main.go Outdated
Comment on lines 482 to 484
body, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseSize))
if err != nil {
return "", fmt.Errorf("failed to read response: %w", err)
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
main.go Outdated
Comment on lines 541 to 543
body, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseSize))
if err != nil {
return fmt.Errorf("failed to read response: %w", err)
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
main.go Outdated
Comment on lines 351 to 354
body, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseSize))
if err != nil {
return nil, fmt.Errorf("failed to read response: %w", err)
}
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
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)
}

Copilot uses AI. Check for mistakes.
Comment on lines +104 to +115
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.")
}
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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)...)

Copilot uses AI. Check for mistakes.
appleboy and others added 2 commits March 11, 2026 22:59
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>
@appleboy appleboy merged commit 65b54f1 into main Mar 11, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants