Merge pull request #549 from junyuanz1/fix/oauth-token-error-diagnostics
fix(client/oauth): preserve token endpoint error diagnostics
This commit is contained in:
commit
7d7a5669ef
3 changed files with 218 additions and 17 deletions
|
|
@ -159,6 +159,27 @@ provider = MCP::Client::OAuth::Provider.new(
|
|||
)
|
||||
```
|
||||
|
||||
### Token Endpoint Errors
|
||||
|
||||
When a token exchange or refresh fails, `MCP::Client::OAuth::Flow::AuthorizationError` includes the HTTP status and
|
||||
the authorization server's `error` and `error_description` from [RFC 6749 Section 5.2](https://www.rfc-editor.org/rfc/rfc6749#section-5.2).
|
||||
For example:
|
||||
|
||||
```text
|
||||
Token endpoint returned status 400. invalid_request: Client must not use multiple authentication methods
|
||||
```
|
||||
|
||||
The exception exposes `http_status`, `error`, and `error_description` readers for structured diagnostics. Missing or non-string
|
||||
diagnostic fields are `nil`; non-JSON responses retain the status-only message. Other authorization failures have `nil` readers.
|
||||
An `invalid_grant` response still raises `Flow::InvalidGrantError`, a subclass of `Flow::AuthorizationError`, so refresh-token
|
||||
recovery behavior is unchanged.
|
||||
|
||||
Diagnostic fields are limited to 128 characters for `error` and 512 for `error_description`, including a trailing `...` when
|
||||
truncated. Characters outside the RFC's printable ASCII set are replaced with spaces, and surrounding whitespace is removed.
|
||||
The SDK excludes all other response fields, including `error_uri`, and does not include the raw response body in these errors.
|
||||
Descriptions are provider-controlled text, not guaranteed to be free of sensitive information; apply your application's logging
|
||||
and redaction policy before persisting them or displaying them to users.
|
||||
|
||||
### Client Credentials Grant
|
||||
|
||||
For a confidential machine-to-machine client (no user, no browser redirect), use `MCP::Client::OAuth::ClientCredentialsProvider` instead of `Provider`.
|
||||
|
|
|
|||
|
|
@ -14,7 +14,19 @@ module MCP
|
|||
# `Provider`; this class consumes a Provider plus signal data extracted from
|
||||
# the failing response (resource_metadata URL, scope challenge).
|
||||
class Flow
|
||||
class AuthorizationError < StandardError; end
|
||||
TOKEN_ENDPOINT_ERROR_MAX_LENGTH = 128
|
||||
TOKEN_ENDPOINT_ERROR_DESCRIPTION_MAX_LENGTH = 512
|
||||
|
||||
class AuthorizationError < StandardError
|
||||
attr_reader :http_status, :error, :error_description
|
||||
|
||||
def initialize(message = nil, http_status: nil, error: nil, error_description: nil)
|
||||
super(message)
|
||||
@http_status = http_status
|
||||
@error = error
|
||||
@error_description = error_description
|
||||
end
|
||||
end
|
||||
|
||||
# Raised specifically when the token endpoint rejects a grant with
|
||||
# `error: "invalid_grant"` (RFC 6749 §5.2). Callers use this to
|
||||
|
|
@ -1062,11 +1074,7 @@ module MCP
|
|||
end
|
||||
|
||||
if response.status < 200 || response.status >= 300
|
||||
if token_endpoint_error_code(response) == "invalid_grant"
|
||||
raise InvalidGrantError, "Token endpoint rejected the grant: invalid_grant."
|
||||
end
|
||||
|
||||
raise AuthorizationError, "Token endpoint returned status #{response.status}."
|
||||
raise token_endpoint_error(response)
|
||||
end
|
||||
|
||||
parsed = begin
|
||||
|
|
@ -1087,17 +1095,35 @@ module MCP
|
|||
parsed
|
||||
end
|
||||
|
||||
# Extracts the `error` code from an RFC 6749 §5.2 error response body
|
||||
# when one is parseable. Returns nil on any parse failure or when
|
||||
# the body is not JSON.
|
||||
def token_endpoint_error_code(response)
|
||||
body = response_body_string(response).to_s
|
||||
return if body.empty?
|
||||
# Surface only RFC 6749 §5.2 diagnostic fields, never the raw response,
|
||||
# which may contain tokens or other credentials. Classify the original
|
||||
# code so sanitization cannot turn malformed input into invalid_grant.
|
||||
def token_endpoint_error(response)
|
||||
message = "Token endpoint returned status #{response.status}."
|
||||
error_class = AuthorizationError
|
||||
parsed = JSON.parse(response_body_string(response))
|
||||
parsed = {} unless parsed.is_a?(Hash)
|
||||
|
||||
parsed = JSON.parse(body)
|
||||
parsed["error"] if parsed.is_a?(Hash)
|
||||
rescue JSON::ParserError
|
||||
nil
|
||||
error_class = parsed["error"] == "invalid_grant" ? InvalidGrantError : AuthorizationError
|
||||
error = token_endpoint_diagnostic(parsed["error"], limit: TOKEN_ENDPOINT_ERROR_MAX_LENGTH)
|
||||
description = token_endpoint_diagnostic(parsed["error_description"], limit: TOKEN_ENDPOINT_ERROR_DESCRIPTION_MAX_LENGTH)
|
||||
message += " #{[error, description].compact.join(": ")}" if error || description
|
||||
|
||||
error_class.new(message, http_status: response.status, error: error, error_description: description)
|
||||
rescue StandardError
|
||||
# Diagnostics must not mask the endpoint failure or change refresh recovery.
|
||||
error_class.new("Token endpoint returned status #{response.status}.", http_status: response.status)
|
||||
end
|
||||
|
||||
def token_endpoint_diagnostic(value, limit:)
|
||||
return unless value.is_a?(String)
|
||||
|
||||
# RFC 6749 permits printable ASCII except double quotes and backslashes.
|
||||
# Replace other characters to keep provider text on one log line.
|
||||
value = value.scrub(" ").gsub(/[^\x20-\x21\x23-\x5B\x5D-\x7E]/, " ").strip
|
||||
return if value.empty?
|
||||
|
||||
value.length > limit ? "#{value[0, limit - 3]}..." : value
|
||||
end
|
||||
|
||||
# Per RFC 6749 Section 2.3.1, the `client_id` and `client_secret` MUST be
|
||||
|
|
|
|||
|
|
@ -2731,7 +2731,161 @@ module MCP
|
|||
provider.save_client_information("client_id" => "test-client")
|
||||
provider.save_tokens("access_token" => "stale-at", "refresh_token" => "revoked-rt")
|
||||
|
||||
assert_raises(Flow::InvalidGrantError) do
|
||||
error = assert_raises(Flow::InvalidGrantError) do
|
||||
Flow.new(provider: provider).refresh!(server_url: @server_url, resource_metadata_url: @prm_url)
|
||||
end
|
||||
assert_equal(400, error.http_status)
|
||||
assert_equal("invalid_grant", error.error)
|
||||
assert_equal("refresh token expired", error.error_description)
|
||||
assert_equal("Token endpoint returned status 400. invalid_grant: refresh token expired", error.message)
|
||||
end
|
||||
|
||||
def test_token_exchange_preserves_oauth_diagnostics
|
||||
stub_request(:post, "#{@auth_base}/token").to_return(
|
||||
status: 400,
|
||||
body: JSON.generate(
|
||||
error: "invalid_request",
|
||||
error_description: "Client must not use multiple authentication methods",
|
||||
access_token: "do-not-log-access-token",
|
||||
refresh_token: "do-not-log-refresh-token",
|
||||
client_secret: "do-not-log-client-secret",
|
||||
error_uri: "https://auth.example.com/error?secret=do-not-log",
|
||||
),
|
||||
)
|
||||
|
||||
error = assert_raises(Flow::AuthorizationError) do
|
||||
capture_authorization_scope(grant_types: ["authorization_code"])
|
||||
end
|
||||
|
||||
assert_equal(400, error.http_status)
|
||||
assert_equal("invalid_request", error.error)
|
||||
assert_equal("Client must not use multiple authentication methods", error.error_description)
|
||||
assert_equal(
|
||||
"Token endpoint returned status 400. invalid_request: Client must not use multiple authentication methods",
|
||||
error.message,
|
||||
)
|
||||
end
|
||||
|
||||
def test_refresh_preserves_oauth_diagnostics
|
||||
error = refresh_token_endpoint_error(
|
||||
JSON.generate(error: "invalid_client", error_description: "Client authentication failed"),
|
||||
status: 401,
|
||||
)
|
||||
|
||||
assert_instance_of(Flow::AuthorizationError, error)
|
||||
assert_equal(401, error.http_status)
|
||||
assert_equal("invalid_client", error.error)
|
||||
assert_equal("Client authentication failed", error.error_description)
|
||||
assert_equal("Token endpoint returned status 401. invalid_client: Client authentication failed", error.message)
|
||||
end
|
||||
|
||||
def test_token_endpoint_errors_fall_back_for_malformed_bodies
|
||||
["", "<html>secret</html>", "{broken", "null", "[]", '"secret"', "42"].each do |body|
|
||||
error = refresh_token_endpoint_error(body)
|
||||
|
||||
assert_instance_of(Flow::AuthorizationError, error)
|
||||
assert_equal(400, error.http_status)
|
||||
assert_nil(error.error)
|
||||
assert_nil(error.error_description)
|
||||
assert_equal("Token endpoint returned status 400.", error.message)
|
||||
end
|
||||
end
|
||||
|
||||
def test_token_endpoint_errors_ignore_non_string_and_empty_fields
|
||||
[nil, 42, true, [], { secret: "hidden" }, "", " \r\n\t"].each do |value|
|
||||
error = refresh_token_endpoint_error(JSON.generate(error: value, error_description: value))
|
||||
|
||||
assert_nil(error.error)
|
||||
assert_nil(error.error_description)
|
||||
assert_equal("Token endpoint returned status 400.", error.message)
|
||||
end
|
||||
end
|
||||
|
||||
def test_token_endpoint_errors_preserve_optional_fields_independently
|
||||
error = refresh_token_endpoint_error(JSON.generate(error: "provider_extension"))
|
||||
assert_equal("provider_extension", error.error)
|
||||
assert_nil(error.error_description)
|
||||
assert_equal("Token endpoint returned status 400. provider_extension", error.message)
|
||||
|
||||
error = refresh_token_endpoint_error(JSON.generate(error_description: "Details without a code"))
|
||||
assert_nil(error.error)
|
||||
assert_equal("Details without a code", error.error_description)
|
||||
assert_equal("Token endpoint returned status 400. Details without a code", error.message)
|
||||
end
|
||||
|
||||
def test_token_endpoint_errors_sanitize_without_changing_grant_classification
|
||||
error = refresh_token_endpoint_error(
|
||||
JSON.generate(error: "invalid_grant\n", error_description: "expired\r\n\t\e\u0000\"\\\u2028token"),
|
||||
)
|
||||
|
||||
assert_instance_of(Flow::AuthorizationError, error)
|
||||
assert_equal("invalid_grant", error.error)
|
||||
assert_equal("expired token", error.error_description)
|
||||
assert_equal("Token endpoint returned status 400. invalid_grant: expired token", error.message)
|
||||
end
|
||||
|
||||
def test_invalid_grant_with_invalid_utf8_description_preserves_classification
|
||||
error = refresh_token_endpoint_error(
|
||||
"{\"error\":\"invalid_grant\",\"error_description\":\"bad \xFF byte\"}".b,
|
||||
)
|
||||
|
||||
assert_instance_of(Flow::InvalidGrantError, error)
|
||||
assert_equal(400, error.http_status)
|
||||
assert_equal("invalid_grant", error.error)
|
||||
assert_equal("bad byte", error.error_description)
|
||||
assert_equal("Token endpoint returned status 400. invalid_grant: bad byte", error.message)
|
||||
end
|
||||
|
||||
def test_token_endpoint_errors_scrub_invalid_utf8_in_both_fields
|
||||
error = refresh_token_endpoint_error(
|
||||
"{\"error\":\"invalid_grant\xFF\",\"error_description\":\"bad \xFF byte\"}".b,
|
||||
)
|
||||
|
||||
assert_instance_of(Flow::AuthorizationError, error)
|
||||
assert_equal(400, error.http_status)
|
||||
assert_equal("invalid_grant", error.error)
|
||||
assert_equal("bad byte", error.error_description)
|
||||
assert_equal("Token endpoint returned status 400. invalid_grant: bad byte", error.message)
|
||||
end
|
||||
|
||||
def test_token_endpoint_errors_fall_back_when_diagnostic_extraction_raises
|
||||
Flow.any_instance.stubs(:token_endpoint_diagnostic).raises(ArgumentError, "sensitive provider text")
|
||||
|
||||
{ "invalid_grant" => Flow::InvalidGrantError, "invalid_client" => Flow::AuthorizationError }.each do |code, klass|
|
||||
error = refresh_token_endpoint_error(JSON.generate(error: code, error_description: "details"))
|
||||
|
||||
assert_instance_of(klass, error)
|
||||
assert_equal(400, error.http_status)
|
||||
assert_nil(error.error)
|
||||
assert_nil(error.error_description)
|
||||
assert_equal("Token endpoint returned status 400.", error.message)
|
||||
end
|
||||
end
|
||||
|
||||
def test_token_endpoint_errors_bound_diagnostic_lengths
|
||||
error = refresh_token_endpoint_error(JSON.generate(error: "e" * 200, error_description: "d" * 1000))
|
||||
|
||||
assert_equal("#{"e" * 125}...", error.error)
|
||||
assert_equal("#{"d" * 509}...", error.error_description)
|
||||
assert_equal("Token endpoint returned status 400. #{error.error}: #{error.error_description}", error.message)
|
||||
end
|
||||
|
||||
def test_other_authorization_errors_have_no_token_endpoint_diagnostics
|
||||
error = Flow::AuthorizationError.new("Discovery failed")
|
||||
|
||||
assert_equal("Discovery failed", error.message)
|
||||
assert_nil(error.http_status)
|
||||
assert_nil(error.error)
|
||||
assert_nil(error.error_description)
|
||||
end
|
||||
|
||||
def refresh_token_endpoint_error(body, status: 400)
|
||||
stub_request(:post, "#{@auth_base}/token").to_return(status: status, body: body)
|
||||
provider = ssrf_test_provider
|
||||
provider.save_client_information("client_id" => "test-client")
|
||||
provider.save_tokens("access_token" => "stale-at", "refresh_token" => "saved-rt")
|
||||
|
||||
assert_raises(Flow::AuthorizationError) do
|
||||
Flow.new(provider: provider).refresh!(server_url: @server_url, resource_metadata_url: @prm_url)
|
||||
end
|
||||
end
|
||||
|
|
|
|||
Loading…
Reference in a new issue