-
Notifications
You must be signed in to change notification settings - Fork 0
fix: bound HTTP response reads and use sync.Once for config init #16
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,21 +28,40 @@ import ( | |
| ) | ||
|
|
||
| var ( | ||
| serverURL string | ||
| clientID string | ||
| tokenFile string | ||
| tokenStoreMode string | ||
| flagServerURL *string | ||
| flagClientID *string | ||
| flagTokenFile *string | ||
| flagTokenStore *string | ||
| configInitialized bool | ||
| retryClient *retry.Client | ||
| tokenStore credstore.Store[credstore.Token] | ||
| serverURL string | ||
| clientID string | ||
| tokenFile string | ||
| tokenStoreMode string | ||
| flagServerURL *string | ||
| flagClientID *string | ||
| flagTokenFile *string | ||
| flagTokenStore *string | ||
| configOnce sync.Once | ||
| retryClient *retry.Client | ||
| tokenStore credstore.Store[credstore.Token] | ||
| ) | ||
|
|
||
| const defaultKeyringService = "authgate-device-cli" | ||
|
|
||
| // maxResponseBodySize limits HTTP response body reads to prevent memory exhaustion (DoS). | ||
| const maxResponseBodySize = 1 << 20 // 1 MB | ||
|
|
||
| // errResponseTooLarge indicates the server returned an oversized response body. | ||
| var errResponseTooLarge = errors.New("response body exceeds maximum allowed size") | ||
|
|
||
| // readResponseBody reads the response body up to maxResponseBodySize. | ||
| // Returns errResponseTooLarge if the body exceeds the limit. | ||
| func readResponseBody(body io.Reader) ([]byte, error) { | ||
| data, err := io.ReadAll(io.LimitReader(body, maxResponseBodySize+1)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to read response: %w", err) | ||
| } | ||
| if int64(len(data)) > maxResponseBodySize { | ||
| return nil, errResponseTooLarge | ||
| } | ||
| return data, nil | ||
| } | ||
|
|
||
| // Timeout configuration for different operations | ||
| const ( | ||
| deviceCodeRequestTimeout = 10 * time.Second | ||
|
|
@@ -107,11 +126,12 @@ func init() { | |
| // initConfig parses flags and initializes configuration | ||
| // Separated from init() to avoid conflicts with test flag parsing | ||
| func initConfig() { | ||
| if configInitialized { | ||
| return | ||
| } | ||
| configInitialized = true | ||
| configOnce.Do(func() { | ||
| doInitConfig() | ||
| }) | ||
| } | ||
|
|
||
| func doInitConfig() { | ||
| flag.Parse() | ||
|
|
||
| // Priority: flag > env > default | ||
|
|
@@ -438,7 +458,7 @@ func requestDeviceCode(ctx context.Context) (*oauth2.DeviceAuthResponse, error) | |
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| body, err := io.ReadAll(resp.Body) | ||
| body, err := readResponseBody(resp.Body) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to read response: %w", err) | ||
| } | ||
|
|
@@ -638,7 +658,7 @@ func exchangeDeviceCode( | |
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| body, err := io.ReadAll(resp.Body) | ||
| body, err := readResponseBody(resp.Body) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to read response: %w", err) | ||
| } | ||
|
|
@@ -696,7 +716,7 @@ func verifyToken(ctx context.Context, accessToken string, d tui.Displayer) error | |
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| body, err := io.ReadAll(resp.Body) | ||
| body, err := readResponseBody(resp.Body) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to read response: %w", err) | ||
| } | ||
|
|
@@ -746,7 +766,7 @@ func refreshAccessToken( | |
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| body, err := io.ReadAll(resp.Body) | ||
| body, err := readResponseBody(resp.Body) | ||
| if err != nil { | ||
| return credstore.Token{}, fmt.Errorf("failed to read response: %w", err) | ||
| } | ||
|
|
@@ -871,7 +891,7 @@ func makeAPICallWithAutoRefresh( | |
| defer resp.Body.Close() | ||
| } | ||
|
|
||
| body, err := io.ReadAll(resp.Body) | ||
| body, err := readResponseBody(resp.Body) | ||
| if err != nil { | ||
|
Comment on lines
891
to
895
|
||
| return fmt.Errorf("failed to read response: %w", err) | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change introduces a new security boundary (
maxResponseBodySize) but there are no tests exercising the oversized-response path (e.g., server returns >1MB, client should fail in a well-defined way). Since this file already has extensive HTTP/flow tests, consider adding a test case that returns a payload just over the limit and asserts the resulting error (ideally a dedicated "response too large" error if you add explicit truncation detection).