Skip to content

Return *oauth2.RetrieveError from tokenexchange#5082

Merged
jhrozek merged 2 commits intomainfrom
oauth-refactor-prep-2
May 5, 2026
Merged

Return *oauth2.RetrieveError from tokenexchange#5082
jhrozek merged 2 commits intomainfrom
oauth-refactor-prep-2

Conversation

@jhrozek
Copy link
Copy Markdown
Contributor

@jhrozek jhrozek commented Apr 28, 2026

Summary

  • Why: Token exchange currently returns a private oAuthError whose only output is a formatted string. Callers cannot inspect the RFC 6749 §5.2 fields without parsing the message. This PR aligns the error shape with the upcoming JWT Bearer grant, both of which will share helpers in pkg/oauth*oauth2.RetrieveError from golang.org/x/oauth2 is the library-standard surface for non-2xx token endpoint responses.
  • What: Replace oAuthError with *oauth2.RetrieveError. validateResponseStatus now takes *http.Response so it can attach the full response and best-effort-parse the body. On non-conformant bodies (no error field, e.g. proxy HTML 5xx), the raw body is logged at debug level and cleared from the returned error so RetrieveError.Error() cannot interpolate arbitrary upstream content (HTML, hostnames, stack traces) into wrapped log strings — same two-tier pattern already used by formatOAuth2Error in pkg/authserver. parseTokenExchangeResponse wraps json.Unmarshal failures with %w.

Type of change

  • Refactoring (no behavior change)

(Note: the body-redaction defense above is a deliberate hardening of an information-disclosure path the previous code already had via the implicit fmt error string; user-visible structured behavior is unchanged.)

Test plan

  • Unit tests (task test) — pkg/auth/tokenexchange passes; two pre-existing failures on main in unrelated packages (pkg/skills/client hardcoded port, pkg/workloads empty-group filter) are not introduced by this PR.
  • Linting (task lint-fix) — 0 issues.

TestExchangeToken_HTTPErrorResponses was migrated from substring matching on err.Error() to errors.As(err, &*oauth2.RetrieveError{}) and now asserts on Response.StatusCode, ErrorCode, ErrorDescription, and Body (preserved on conformant responses, cleared on the 503 non-JSON case).

API Compatibility

  • This PR does not break the v1beta1 API.

Does this introduce a user-facing change?

No. Internal error type change. Operators who scrape the existing slog.Debug log fields (oauth_error_code, description, status, body_length) will see identical field names; the body-content debug field is new and additive.

Special notes for reviewers

  • The commit is intentionally narrow and isolated from the pkg/oauth shared-helpers migration (subsequent commits 5b/5c) so a future bisect can distinguish "error shape regressed" from "plumbing regressed."
  • Upstream caller pkg/vmcp/auth/strategies/tokenexchange.go:142 wraps with %w, so errors.As continues to work and the leading "token exchange failed: " prefix is preserved.
  • The body-redaction behavior was added in response to review feedback on the original commit — see commit message for rationale and the formatOAuth2Error precedent.

Generated with Claude Code

@github-actions github-actions Bot added the size/S Small PR: 100-299 lines changed label Apr 28, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 28, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.19%. Comparing base (a98c991) to head (e8109cc).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5082      +/-   ##
==========================================
- Coverage   67.19%   67.19%   -0.01%     
==========================================
  Files         598      598              
  Lines       60258    60260       +2     
==========================================
+ Hits        40493    40494       +1     
- Misses      16691    16696       +5     
+ Partials     3074     3070       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jhrozek jhrozek requested a review from tgrunnagle April 28, 2026 22:26
tgrunnagle
tgrunnagle previously approved these changes Apr 29, 2026
Comment thread pkg/auth/tokenexchange/exchange.go Outdated
jhrozek added 2 commits April 29, 2026 16:24
Replace the private oAuthError type with *oauth2.RetrieveError from
golang.org/x/oauth2 so token exchange errors expose RFC 6749 §5.2
fields (error, error_description, error_uri) as structured data via
errors.As. This is the library-standard surface for non-2xx token
endpoint responses, and aligns the error shape with the JWT Bearer
grant that will share helpers in pkg/oauth.

Behavior changes:

- validateResponseStatus takes *http.Response so it can attach the
  full response to the returned error and parse the body as RFC
  6749 §5.2 best-effort.

- When the body is non-conformant (no "error" field, e.g. a proxy
  HTML 5xx), the raw body is logged at debug level and cleared from
  the returned error. This prevents oauth2.RetrieveError.Error()
  from interpolating arbitrary upstream content (HTML, hostnames,
  stack traces) into wrapped error strings — same two-tier pattern
  used by formatOAuth2Error in pkg/authserver.

- parseTokenExchangeResponse wraps json.Unmarshal failures with %w.

The error type change is isolated from code movement so a future
bisect can distinguish "error shape regressed" from "plumbing
regressed".
The previous commit cleared the body only when the response was
non-conformant (no RFC 6749 §5.2 "error" field), on the theory that
a structured-error body is bounded and harmless. PR review pointed
out the asymmetry, and the simpler answer is to clear Body in both
branches:

- The structured fields (ErrorCode, ErrorDescription, ErrorURI) are
  already extracted onto *oauth2.RetrieveError, so callers using
  errors.As lose nothing.

- Full body content is preserved in slog.Debug for ops, regardless
  of which branch is taken.

- No caller in this repo reads retrieveErr.Body for any non-debug
  purpose (verified by grep on .RetrieveError\b).

- Removes a special case future maintainers would have to re-derive.

This matches Ory Hydra, which never surfaces raw upstream error
bodies through its public error type — see the JWKS fetcher in
fosite/client_authentication_jwks_strategy.go and the token-hook
client in oauth2/token_hook.go, both of which discard the body and
return only the upstream status code on non-2xx responses.
@jhrozek jhrozek force-pushed the oauth-refactor-prep-2 branch from 1f32fdf to e8109cc Compare April 29, 2026 15:35
@github-actions github-actions Bot added size/S Small PR: 100-299 lines changed and removed size/S Small PR: 100-299 lines changed labels Apr 29, 2026
@jhrozek jhrozek merged commit 37fcb1a into main May 5, 2026
46 checks passed
@jhrozek jhrozek deleted the oauth-refactor-prep-2 branch May 5, 2026 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S Small PR: 100-299 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants