Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
appleboy merged 3 commits into from
Mar 11, 2026
Merged

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

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1f87a77
fix(main): address medium-severity security findings
appleboy Mar 11, 2026
55ac4f0
ci(workflows): remove non-existent make generate step
appleboy Mar 11, 2026
cc5f47a
fix(main): return explicit error on oversized responses
appleboy Mar 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 27 additions & 20 deletions main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"os/signal"
"strconv"
"strings"
"sync"
"syscall"
"time"

Expand All @@ -28,18 +29,18 @@ import (
)

var (
serverURL string
clientID string
clientSecret string
redirectURI string
callbackPort int
scope string
tokenFile string
tokenStoreMode string
tokenStore credstore.Store[credstore.Token]
configInitialized bool
retryClient *retry.Client
configWarnings []string
serverURL string
clientID string
clientSecret string
redirectURI string
callbackPort int
scope string
tokenFile string
tokenStoreMode string
tokenStore credstore.Store[credstore.Token]
configOnce sync.Once
retryClient *retry.Client
configWarnings []string

flagServerURL *string
flagClientID *string
Expand All @@ -55,6 +56,7 @@ const (
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.
)

func init() {
Expand Down Expand Up @@ -96,16 +98,21 @@ func init() {

// initConfig parses flags and initializes all configuration.
func initConfig() {
if configInitialized {
return
}
configInitialized = true
configOnce.Do(doInitConfig)
}

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.")
}
Comment on lines +104 to +115
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.
scope = getConfig(*flagScope, "SCOPE", "read write")
tokenFile = getConfig(*flagTokenFile, "TOKEN_FILE", ".authgate-tokens.json")

Expand Down Expand Up @@ -341,7 +348,7 @@ func exchangeCode(ctx context.Context, code, codeVerifier string) (*tui.TokenSto
}
defer resp.Body.Close()

body, err := io.ReadAll(resp.Body)
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.
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.
Expand Down Expand Up @@ -405,7 +412,7 @@ func refreshAccessToken(ctx context.Context, refreshToken string) (*tui.TokenSto
}
defer resp.Body.Close()

body, err := io.ReadAll(resp.Body)
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.
Expand Down Expand Up @@ -472,7 +479,7 @@ func verifyToken(ctx context.Context, accessToken string) (string, error) {
}
defer resp.Body.Close()

body, err := io.ReadAll(resp.Body)
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.
}
Expand Down Expand Up @@ -531,7 +538,7 @@ func makeAPICallWithAutoRefresh(ctx context.Context, storage *tui.TokenStorage)
defer resp.Body.Close()
}

body, err := io.ReadAll(resp.Body)
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.
}
Expand Down
Loading