From 2be8cc693ebef1edec7863b1cfb2e4907cc2917f Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Thu, 23 Apr 2026 12:35:03 +0100 Subject: [PATCH 1/2] Return *oauth2.RetrieveError from tokenexchange MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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". --- pkg/auth/tokenexchange/exchange.go | 74 +++++++++++-------------- pkg/auth/tokenexchange/exchange_test.go | 65 +++++++++++++--------- 2 files changed, 73 insertions(+), 66 deletions(-) diff --git a/pkg/auth/tokenexchange/exchange.go b/pkg/auth/tokenexchange/exchange.go index 430a4dff9b..2c57c97352 100644 --- a/pkg/auth/tokenexchange/exchange.go +++ b/pkg/auth/tokenexchange/exchange.go @@ -7,7 +7,6 @@ package tokenexchange import ( "context" "encoding/json" - "errors" "fmt" "io" "log/slog" @@ -67,34 +66,6 @@ func NormalizeTokenType(tokenType string) (string, error) { } } -// oAuthError represents an OAuth 2.0 error response as defined in RFC 6749 Section 5.2. -type oAuthError struct { - Error string `json:"error"` - ErrorDescription string `json:"error_description,omitempty"` - ErrorURI string `json:"error_uri,omitempty"` - StatusCode int `json:"-"` -} - -func (e *oAuthError) String() string { - if e.ErrorURI != "" { - return fmt.Sprintf("OAuth error %q (status %d): see %s", e.Error, e.StatusCode, e.ErrorURI) - } - return fmt.Sprintf("OAuth error %q (status %d)", e.Error, e.StatusCode) -} - -// parseOAuthError attempts to parse an OAuth error response from the given response body. -func parseOAuthError(statusCode int, body []byte) *oAuthError { - var oauthErr oAuthError - if err := json.Unmarshal(body, &oauthErr); err != nil { - return nil - } - if oauthErr.Error == "" { - return nil - } - oauthErr.StatusCode = statusCode - return &oauthErr -} - // defaultHTTPClient is the default HTTP client used for token exchange requests. var defaultHTTPClient = &http.Client{ Timeout: defaultHTTPTimeout, @@ -502,7 +473,7 @@ func executeTokenExchangeRequest(client *http.Client, req *http.Request) ([]byte return nil, fmt.Errorf("failed to read token exchange response: %w", err) } - if err := validateResponseStatus(resp.StatusCode, body); err != nil { + if err := validateResponseStatus(resp, body); err != nil { return nil, err } @@ -510,21 +481,42 @@ func executeTokenExchangeRequest(client *http.Client, req *http.Request) ([]byte } // validateResponseStatus checks the HTTP status code and returns an error if not successful. -func validateResponseStatus(statusCode int, body []byte) error { - if statusCode >= 200 && statusCode <= 299 { +// On non-2xx responses it returns a *oauth2.RetrieveError; when the body is non-conformant +// (no RFC 6749 §5.2 "error" field), the raw body is moved to debug logs and cleared from +// the error so it cannot be interpolated into log messages by callers. +func validateResponseStatus(resp *http.Response, body []byte) error { + if resp.StatusCode >= 200 && resp.StatusCode <= 299 { return nil } - // Try to parse as OAuth error first - if oauthErr := parseOAuthError(statusCode, body); oauthErr != nil { - //nolint:gosec // G706: OAuth error codes are standard protocol values, not user input - slog.Debug("Token exchange OAuth error", "oauth_error_code", oauthErr.Error, "description", oauthErr.ErrorDescription) - return errors.New(oauthErr.String()) + retrieveErr := &oauth2.RetrieveError{ + Response: resp, + Body: body, + } + + // Best-effort parse of the RFC 6749 Section 5.2 error response. Non-JSON or + // non-error-shaped bodies leave ErrorCode/ErrorDescription/ErrorURI empty. + var oauthErr struct { + Error string `json:"error"` + ErrorDescription string `json:"error_description,omitempty"` + ErrorURI string `json:"error_uri,omitempty"` + } + if err := json.Unmarshal(body, &oauthErr); err == nil { + retrieveErr.ErrorCode = oauthErr.Error + retrieveErr.ErrorDescription = oauthErr.ErrorDescription + retrieveErr.ErrorURI = oauthErr.ErrorURI + } + + if retrieveErr.ErrorCode != "" { + slog.Debug("Token exchange OAuth error", + "oauth_error_code", retrieveErr.ErrorCode, + "description", retrieveErr.ErrorDescription) + } else { + slog.Debug("Token exchange failed", "status", resp.StatusCode, "body_length", len(body), "body", string(body)) + retrieveErr.Body = nil } - //nolint:gosec // G706: status code and body length are safe diagnostic values - slog.Debug("Token exchange failed", "status", statusCode, "body_length", len(body)) - return fmt.Errorf("token exchange failed with status %d", statusCode) + return retrieveErr } // parseTokenExchangeResponse parses the token exchange response body. @@ -532,7 +524,7 @@ func parseTokenExchangeResponse(body []byte) (*response, error) { var tokenResp response if err := json.Unmarshal(body, &tokenResp); err != nil { slog.Debug("Failed to parse token exchange response", "error", err) - return nil, errors.New("failed to parse token exchange response") + return nil, fmt.Errorf("failed to parse token exchange response: %w", err) } return &tokenResp, nil diff --git a/pkg/auth/tokenexchange/exchange_test.go b/pkg/auth/tokenexchange/exchange_test.go index 419731aba7..c9151c86e8 100644 --- a/pkg/auth/tokenexchange/exchange_test.go +++ b/pkg/auth/tokenexchange/exchange_test.go @@ -371,40 +371,44 @@ func TestExchangeToken_HTTPErrorResponses(t *testing.T) { t.Parallel() tests := []struct { - name string - statusCode int - responseBody string - expectedError string + name string + statusCode int + responseBody string + expectedErrorCode string + expectedDescription string + expectedBodyNil bool }{ { - name: "400 Bad Request", - statusCode: http.StatusBadRequest, - responseBody: `{"error":"invalid_request","error_description":"Missing required parameter"}`, - expectedError: "OAuth error \"invalid_request\" (status 400)", + name: "400 Bad Request", + statusCode: http.StatusBadRequest, + responseBody: `{"error":"invalid_request","error_description":"Missing required parameter"}`, + expectedErrorCode: "invalid_request", + expectedDescription: "Missing required parameter", }, { - name: "401 Unauthorized", - statusCode: http.StatusUnauthorized, - responseBody: `{"error":"invalid_client"}`, - expectedError: "OAuth error \"invalid_client\" (status 401)", + name: "401 Unauthorized", + statusCode: http.StatusUnauthorized, + responseBody: `{"error":"invalid_client"}`, + expectedErrorCode: "invalid_client", }, { - name: "403 Forbidden", - statusCode: http.StatusForbidden, - responseBody: `{"error":"access_denied"}`, - expectedError: "OAuth error \"access_denied\" (status 403)", + name: "403 Forbidden", + statusCode: http.StatusForbidden, + responseBody: `{"error":"access_denied"}`, + expectedErrorCode: "access_denied", }, { - name: "500 Internal Server Error", - statusCode: http.StatusInternalServerError, - responseBody: `{"error":"server_error"}`, - expectedError: "OAuth error \"server_error\" (status 500)", + name: "500 Internal Server Error", + statusCode: http.StatusInternalServerError, + responseBody: `{"error":"server_error"}`, + expectedErrorCode: "server_error", }, { - name: "503 Service Unavailable", - statusCode: http.StatusServiceUnavailable, - responseBody: "Service temporarily unavailable", - expectedError: "token exchange failed with status 503", + name: "503 Service Unavailable", + statusCode: http.StatusServiceUnavailable, + responseBody: "Service temporarily unavailable", + expectedBodyNil: true, + // Non-JSON body: ErrorCode stays empty, body cleared to prevent info leak. }, } @@ -434,7 +438,18 @@ func TestExchangeToken_HTTPErrorResponses(t *testing.T) { require.Error(t, err) assert.Nil(t, resp) - assert.Contains(t, err.Error(), tt.expectedError) + + var retrieveErr *oauth2.RetrieveError + require.ErrorAs(t, err, &retrieveErr) + require.NotNil(t, retrieveErr.Response) + assert.Equal(t, tt.statusCode, retrieveErr.Response.StatusCode) + assert.Equal(t, tt.expectedErrorCode, retrieveErr.ErrorCode) + assert.Equal(t, tt.expectedDescription, retrieveErr.ErrorDescription) + if tt.expectedBodyNil { + assert.Nil(t, retrieveErr.Body) + } else if tt.expectedErrorCode != "" { + assert.Equal(t, []byte(tt.responseBody), retrieveErr.Body) + } }) } } From e8109ccac83c24fb0b31018b5f2f5b4d99f00bc4 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Wed, 29 Apr 2026 16:23:53 +0100 Subject: [PATCH 2/2] Always clear RetrieveError.Body in tokenexchange MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- pkg/auth/tokenexchange/exchange.go | 10 ++++++---- pkg/auth/tokenexchange/exchange_test.go | 14 ++++---------- 2 files changed, 10 insertions(+), 14 deletions(-) diff --git a/pkg/auth/tokenexchange/exchange.go b/pkg/auth/tokenexchange/exchange.go index 2c57c97352..b6a8e3fb68 100644 --- a/pkg/auth/tokenexchange/exchange.go +++ b/pkg/auth/tokenexchange/exchange.go @@ -481,9 +481,10 @@ func executeTokenExchangeRequest(client *http.Client, req *http.Request) ([]byte } // validateResponseStatus checks the HTTP status code and returns an error if not successful. -// On non-2xx responses it returns a *oauth2.RetrieveError; when the body is non-conformant -// (no RFC 6749 §5.2 "error" field), the raw body is moved to debug logs and cleared from -// the error so it cannot be interpolated into log messages by callers. +// On non-2xx responses it extracts RFC 6749 §5.2 fields (error, error_description, error_uri) +// onto the structured fields of the returned *oauth2.RetrieveError. Body is always cleared so +// callers cannot interpolate raw upstream content into error strings — matching the pattern used +// by Ory Hydra, which never surfaces raw error bodies through its public error type. func validateResponseStatus(resp *http.Response, body []byte) error { if resp.StatusCode >= 200 && resp.StatusCode <= 299 { return nil @@ -513,9 +514,10 @@ func validateResponseStatus(resp *http.Response, body []byte) error { "description", retrieveErr.ErrorDescription) } else { slog.Debug("Token exchange failed", "status", resp.StatusCode, "body_length", len(body), "body", string(body)) - retrieveErr.Body = nil } + retrieveErr.Body = nil + return retrieveErr } diff --git a/pkg/auth/tokenexchange/exchange_test.go b/pkg/auth/tokenexchange/exchange_test.go index c9151c86e8..abc5a0fe01 100644 --- a/pkg/auth/tokenexchange/exchange_test.go +++ b/pkg/auth/tokenexchange/exchange_test.go @@ -376,7 +376,6 @@ func TestExchangeToken_HTTPErrorResponses(t *testing.T) { responseBody string expectedErrorCode string expectedDescription string - expectedBodyNil bool }{ { name: "400 Bad Request", @@ -404,10 +403,9 @@ func TestExchangeToken_HTTPErrorResponses(t *testing.T) { expectedErrorCode: "server_error", }, { - name: "503 Service Unavailable", - statusCode: http.StatusServiceUnavailable, - responseBody: "Service temporarily unavailable", - expectedBodyNil: true, + name: "503 Service Unavailable", + statusCode: http.StatusServiceUnavailable, + responseBody: "Service temporarily unavailable", // Non-JSON body: ErrorCode stays empty, body cleared to prevent info leak. }, } @@ -445,11 +443,7 @@ func TestExchangeToken_HTTPErrorResponses(t *testing.T) { assert.Equal(t, tt.statusCode, retrieveErr.Response.StatusCode) assert.Equal(t, tt.expectedErrorCode, retrieveErr.ErrorCode) assert.Equal(t, tt.expectedDescription, retrieveErr.ErrorDescription) - if tt.expectedBodyNil { - assert.Nil(t, retrieveErr.Body) - } else if tt.expectedErrorCode != "" { - assert.Equal(t, []byte(tt.responseBody), retrieveErr.Body) - } + assert.Nil(t, retrieveErr.Body) }) } }