Skip to content

refactor(sdk): harden HTTP reads and improve code quality#18

Open
appleboy wants to merge 5 commits intomainfrom
worktree-sdk
Open

refactor(sdk): harden HTTP reads and improve code quality#18
appleboy wants to merge 5 commits intomainfrom
worktree-sdk

Conversation

@appleboy
Copy link
Copy Markdown
Member

Summary

  • Cap all HTTP response body reads with io.LimitReader (1 MB) to prevent OOM from unbounded server responses
  • Guard empty client_secret in ClientCredentials() and Introspect() to avoid sending client_secret= in form body
  • Share a single retry.Client between discovery and OAuth in authgate.New() to eliminate redundant HTTP client allocation
  • Simplify duplicated flow dispatch switch/case into a single useBrowser boolean check
  • Extract magic endpoint paths (/oauth/device/code, /oauth/introspect, /oauth/tokeninfo) and subject prefix (client:) into named constants
  • Use string concatenation in Error.Error() for perfsprint linter consistency
  • Add Scope and IDToken fields to credstore.Token so cached tokens preserve full information across round-trips
  • Write a response for duplicate auth code callback requests instead of returning blank
  • Remove unnecessary empty slice allocation for default scopes

Test plan

  • make lint passes with 0 issues
  • make fmt clean
  • make test all tests pass across all packages
  • Verify no import cycles introduced by shared HTTP client change
  • Manual test: auth code flow callback duplicate request shows "Already processed" message

🤖 Generated with Claude Code

- Cap all HTTP response body reads with io.LimitReader (1 MB) to
  prevent OOM from unbounded server responses
- Guard empty client_secret in ClientCredentials and Introspect to
  avoid sending client_secret= in form body
- Share a single retry.Client between discovery and OAuth in
  authgate.New() to eliminate redundant HTTP client allocation
- Simplify duplicated flow dispatch switch/case into a single
  useBrowser boolean check
- Extract magic endpoint paths and subject prefix into named constants
- Use string concatenation in Error.Error() for perfsprint consistency
- Add Scope and IDToken fields to credstore.Token so cached tokens
  preserve full information across round-trips
- Write a response for duplicate auth code callback requests
- Remove unnecessary empty slice allocation for default scopes

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings March 23, 2026 13:40
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 SDK’s HTTP handling and refactors auth/token-flow code to reduce risk from unbounded responses, improve request construction, and preserve more token data across cached round-trips.

Changes:

  • Cap HTTP response body reads/decodes to 1 MB in OAuth and discovery to mitigate unbounded-body DoS risk.
  • Avoid sending client_secret= when the secret is empty for client-credentials and introspection requests.
  • Refactor authgate initialization/flow selection and improve token persistence to include scope and id_token, plus add a duplicate-callback response.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
oauth/oauth.go Adds response-size limits, omits empty client_secret, tweaks error string formatting, and limits error-body reads.
middleware/middleware.go Extracts the client: subject prefix into a named constant for introspection subject typing.
discovery/discovery.go Adds response-size limit for discovery decode; extracts AuthGate-specific endpoint paths into constants.
credstore/store.go Extends persisted token model with Scope and IDToken for round-trip fidelity.
authgate.go Creates a shared retry client for discovery+oauth, simplifies flow selection, removes default empty scopes allocation.
authflow/authflow.go Responds to duplicate auth-code callbacks and maps new token fields between oauth↔credstore.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- Defer default HTTP client creation in oauth.NewClient and
  discovery.NewClient so WithHTTPClient avoids redundant allocation
- Add clear error message when response body exceeds 1 MB limit
  instead of confusing "unexpected EOF" from truncated JSON
- Add tests for empty client_secret in ClientCredentials and
  Introspect to verify no client_secret= field is sent

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

oauth/oauth.go:457

  • parseErrorResponse now caps reads to 1MB, but if the server returns an error body larger than the cap, io.ReadAll will return a truncated body with nil error and the SDK may surface up to 1MB of arbitrary text in Error.Description. Consider detecting lr.N <= 0 after ReadAll and returning a dedicated "response too large" error (or a short placeholder description) to avoid propagating huge/truncated error bodies.
func parseErrorResponse(resp *http.Response) error {
	lr := limitedBody(resp.Body)
	body, err := io.ReadAll(lr)
	if err != nil {
		return &Error{
			Code:        "server_error",
			Description: "failed to read error response",
			StatusCode:  resp.StatusCode,
		}
	}

	var oauthErr Error
	if json.Unmarshal(body, &oauthErr) == nil && oauthErr.Code != "" {
		oauthErr.StatusCode = resp.StatusCode
		return &oauthErr
	}

	return &Error{
		Code:        http.StatusText(resp.StatusCode),
		Description: string(body),
		StatusCode:  resp.StatusCode,

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- Wrap errResponseTooLarge with the original decode error context so
  oversized-body failures remain actionable
- Detect truncated error bodies in parseErrorResponse and return a
  dedicated error instead of propagating huge truncated text
- Add tests for >1MB response body limit (success and error paths)
- Add test for duplicate auth code callback response

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- Use maxResponseBytes+1 in LimitedReader so responses exactly at
  the limit are accepted, only triggering the error when truly exceeded
- Check lr.N == 0 instead of lr.N <= 0 consistently across oauth
  and discovery packages

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Add lr.N == 0 check after successful JSON decode to catch cases
where the body exceeds the limit but the decoder succeeds on a
truncated stream.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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.

2 participants