Merge pull request #561 from koic/propagate_network_errors_from_protected_resource_metadata_discovery

Surface unreachable Protected Resource Metadata instead of falling back to legacy discovery

Author: koic

docs/_client/authorization.md

@@ -39,6 +39,8 @@ pass an `MCP::Client::OAuth::Provider` to the transport instead of a static `Aut
 - Fall back to the legacy 2025-03-26 discovery when the server publishes no Protected Resource Metadata, matching the TypeScript and Python SDKs: the MCP server's origin acts
   as the authorization base URL, its metadata is fetched from `<origin>/.well-known/oauth-authorization-server` and must name that origin as its `issuer` (RFC 8414 Section 3.3),
   and when even that is absent the spec's default endpoints `/authorize`, `/token`, and `/register` at the origin are used with PKCE S256 assumed.
+  When no Protected Resource Metadata candidate serves a JSON object, a request that could not reach the server, or that returned a `5xx` or `429`,
+  raises `Flow::MetadataUnreachableError` instead of triggering that fallback, and a body over the response cap is refused outright.
 - On subsequent 401s with a saved `refresh_token`, exchange it at the token endpoint before falling back to the full interactive flow (RFC 6749 Section 6).
   `ClientCredentialsProvider` and `CrossAppAccessProvider` refresh the same way and fall back to their own grant instead;
   their refresh also requires the `issuer` the SDK records on the tokens, so tokens stored without it run the grant again.

lib/mcp/client/oauth/discovery.rb

@@ -336,6 +336,29 @@ def canonicalize_origin_and_path(url)
             uri.to_s
           end
 
+          # Drops userinfo, query and fragment from `url` and leaves the host and path spelled as given, for reporting
+          # a URL next to the request that failed, where it must still match the server's access log; `URI#to_s`
+          # still lowercases the scheme and drops an explicit default port. Unlike `canonicalize_origin_and_path`
+          # it neither normalizes nor resolves dot segments, so its cost stays linear in the URL length, which matters
+          # for a URL the server chose. A URL that does not parse is not echoed, since the raw value could carry
+          # the very credentials being dropped.
+          def redact_url(url)
+            uri = URI.parse(url.to_s)
+
+            uri.fragment = nil
+            uri.query = nil
+            # `URI::Generic#userinfo=` is a no-op on Ruby 2.7 (the project's minimum supported version),
+            # so clear the components individually.
+            if uri.respond_to?(:user) && (uri.user || uri.password)
+              uri.user = nil
+              uri.password = nil
+            end
+
+            uri.to_s
+          rescue URI::Error
+            "[unparseable URL]"
+          end
+
           # Returns true when `prm` (a PRM `resource` URL) covers `server` (the MCP endpoint URL):
           # same scheme/host/port, with PRM's path being a prefix of the server's path. When PRM
           # also advertises a query string, the server's query MUST be identical to it (otherwise

lib/mcp/client/oauth/flow.rb

@@ -16,6 +16,8 @@ module OAuth
       class Flow
         TOKEN_ENDPOINT_ERROR_MAX_LENGTH = 128
         TOKEN_ENDPOINT_ERROR_DESCRIPTION_MAX_LENGTH = 512
+        METADATA_DIAGNOSTIC_MAX_LENGTH = 128
+        METADATA_URL_MAX_LENGTH = 2048
 
         # Token request parameters the flow sets itself. Its values win over a provider's `token_request_params`,
         # so a provider naming one of these is refused rather than left believing its value was sent.
@@ -57,6 +59,17 @@ class InvalidGrantError < AuthorizationError; end
         # or authorization server metadata failure by rescuing a class rather than by matching the message text.
         class AuthorizationRefusedError < AuthorizationError; end
 
+        # Raised by metadata discovery when every candidate URL answered that nothing usable is published there
+        # (a `4xx` other than `429`, a redirect that was not followed, or a body that is not a JSON object).
+        # The only discovery failure that may select the legacy 2025-03-26 path.
+        class MetadataNotPublishedError < AuthorizationError; end
+
+        # Raised by metadata discovery when the answer says nothing about what is published: the request failed to
+        # reach the server, or a candidate answered `5xx` or `429`. Falling back on this would move
+        # the flow to a different authorization server because of a transient failure, so it is surfaced instead,
+        # as the TypeScript SDK does for network errors outside browsers and the Python SDK does for both.
+        class MetadataUnreachableError < AuthorizationError; end
+
         # Raised for a `token_request_params` value the SDK refuses: a reserved key, a Hash comparing keys by identity,
         # or anything but a Hash of Strings. An `ArgumentError` because the value is a configuration mistake,
         # not a failed authorization, and deliberately outside `AuthorizationError`, which `MCP::Client::HTTP` treats on
@@ -470,15 +483,20 @@ def fetch_protected_resource_metadata(server_url:, resource_metadata_url:)
         #
         # Legacy path (2025-03-26 backwards compatibility): when the server publishes no PRM, `prm` is nil
         # and the MCP server's own origin acts as the authorization base URL, matching the TypeScript and Python SDKs.
-        # Any PRM discovery failure (404s, network errors, malformed documents) selects the legacy path, mirroring both SDKs' behavior.
+        # Only a discovery answer saying that nothing usable is published (`MetadataNotPublishedError`: a `4xx` other than `429`,
+        # a redirect that was not followed, or a body that is not a JSON object) selects the legacy path.
+        # A request that failed to reach the server, or returned a `5xx` or `429`, says nothing about what the server publishes,
+        # so once no candidate has served a usable document it is surfaced instead (`MetadataUnreachableError`),
+        # as both SDKs do for network errors (the TypeScript SDK outside browsers) and the Python SDK does for server errors.
+        # A body over the response cap is refused outright and never reaches the fallback either.
         # https://modelcontextprotocol.io/specification/2025-03-26/basic/authorization#fallbacks-for-servers-without-metadata-discovery
         def locate_authorization_server(server_url:, resource_metadata_url:)
           prm = begin
             fetch_protected_resource_metadata(
               server_url: server_url,
               resource_metadata_url: resource_metadata_url,
             )
-          rescue AuthorizationError
+          rescue MetadataNotPublishedError
             nil
           end
 
@@ -599,44 +617,63 @@ def first_authorization_server(prm)
           first
         end
 
-        # Walks candidate metadata URLs and returns the parsed JSON body of
-        # the first 2xx response. Raises `AuthorizationError` for transport
-        # failures (`Faraday::Error`) and malformed bodies (`JSON::ParserError`)
-        # so callers do not have to handle raw Faraday/JSON exceptions.
+        # Walks candidate metadata URLs and returns the parsed body of the first 2xx response that is a JSON object;
+        # the caller checks its fields. Candidates are tried until one serves such a body, since a later one may still
+        # be usable when an earlier one is broken or down (the URL from `WWW-Authenticate` against the well-known path,
+        # or the OAuth document against the OpenID one). Once the candidates are exhausted, an answer that said nothing
+        # about what is published (a network error, a `5xx`, or a `429`) outranks the rest and raises
+        # `MetadataUnreachableError`; otherwise (any other status, such as a `4xx` other than `429` or a redirect that
+        # was not followed, a body that is not JSON, or not a JSON object) `MetadataNotPublishedError`.
+        # A body over the cap is refused outright by `bounded_request` with a plain `AuthorizationError`,
+        # before any classification. Each failure is listed with its URL stripped of userinfo, query and fragment
+        # and cut to `METADATA_URL_MAX_LENGTH`, but otherwise spelled as requested, so it can be matched against
+        # a server's access log, and with exception text bounded, since the message lands in every log destination
+        # the error passes through.
         def fetch_metadata_json(urls, label:)
-          last_error = nil
+          failures = []
+          inconclusive = false
           urls.each do |url|
             response = begin
               http_get(url)
             rescue Faraday::Error => e
-              last_error = "GET #{url} raised #{e.class}: #{e.message}"
+              detail = bounded_diagnostic(e.message, limit: METADATA_DIAGNOSTIC_MAX_LENGTH)
+              failures << "GET #{reported_url(url)} raised #{[e.class, detail].compact.join(": ")}"
+              inconclusive = true
               next
             end
 
-            if response.status >= 200 && response.status < 300
-              parsed = begin
-                JSON.parse(response_body_string(response))
-              rescue JSON::ParserError => e
-                raise AuthorizationError, "Failed to parse #{label} from #{url}: #{e.message}."
-              end
+            unless response.status >= 200 && response.status < 300
+              failures << "GET #{reported_url(url)} returned #{response.status}"
+              inconclusive = true if response.status >= 500 || response.status == 429
+              next
+            end
 
-              # Even valid JSON can be the wrong shape (a top-level array,
-              # a bare `null`, a string, ...). The discovery callers index by
-              # name (`prm["authorization_servers"]`, etc.), so anything that
-              # is not a Hash would raise `TypeError` / `NoMethodError`
-              # downstream. Surface that as `AuthorizationError` instead so
-              # callers see a single, documented error type.
-              unless parsed.is_a?(Hash)
-                raise AuthorizationError,
-                  "#{label} from #{url} is not a JSON object (got #{parsed.class})."
-              end
+            parsed = begin
+              JSON.parse(response_body_string(response))
+            rescue JSON::ParserError => e
+              detail = bounded_diagnostic(e.message, limit: METADATA_DIAGNOSTIC_MAX_LENGTH) || e.class.name
+              failures << "GET #{reported_url(url)} returned a body that is not JSON: #{detail}"
+              next
+            end
 
-              return parsed
+            # Even valid JSON can be the wrong shape (a top-level array, a bare `null`, a string, ...).
+            # The discovery callers index by name (`prm["authorization_servers"]`, etc.), so anything that
+            # is not a Hash would raise `TypeError` / `NoMethodError` downstream.
+            unless parsed.is_a?(Hash)
+              failures << "GET #{reported_url(url)} returned a body that is not a JSON object (got #{parsed.class})"
+              next
             end
 
-            last_error = "GET #{url} returned #{response.status}"
+            return parsed
+          end
+
+          message = "Failed to fetch #{label}: #{failures.join("; ")}."
+
+          if inconclusive
+            raise MetadataUnreachableError, message
+          else
+            raise MetadataNotPublishedError, message
           end
-          raise AuthorizationError, "Failed to fetch #{label}: #{last_error}."
         end
 
         def ensure_pkce_supported!(as_metadata)
@@ -1304,8 +1341,8 @@ def token_endpoint_error(response)
           parsed = {} unless parsed.is_a?(Hash)
 
           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)
+          error = bounded_diagnostic(parsed["error"], limit: TOKEN_ENDPOINT_ERROR_MAX_LENGTH)
+          description = bounded_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)
@@ -1314,17 +1351,23 @@ def token_endpoint_error(response)
           error_class.new("Token endpoint returned status #{response.status}.", http_status: response.status)
         end
 
-        def token_endpoint_diagnostic(value, limit:)
+        def bounded_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.
+          # RFC 6749 permits printable ASCII except double quotes and backslashes in token endpoint error fields.
+          # Replace other characters to keep text received off the network 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
 
+        # A candidate URL as it goes into a failure string: redacted, and cut so a URL the server chose cannot
+        # grow the message without limit.
+        def reported_url(url)
+          bounded_diagnostic(Discovery.redact_url(url), limit: METADATA_URL_MAX_LENGTH)
+        end
+
         # Per RFC 6749 Section 2.3.1, the `client_id` and `client_secret` MUST be
         # `application/x-www-form-urlencoded` encoded before they are joined with
         # `:` and base64-encoded for the `Authorization: Basic` header. This is

test/mcp/client/oauth/discovery_test.rb

@@ -259,6 +259,19 @@ def test_canonicalize_url_drops_userinfo
           )
         end
 
+        def test_redact_url_drops_credentials_query_and_fragment_but_keeps_the_spelling
+          # Reported next to a failed request, the URL must still match the server's access log, so the host
+          # and path are left alone: no lowercasing of the host, no dot-segment resolution, no decoding.
+          assert_equal(
+            "https://Srv.Example.COM:8443/tenant/%2e%2e/prm%2Ejson",
+            Discovery.redact_url("https://user:pass@Srv.Example.COM:8443/tenant/%2e%2e/prm%2Ejson?token=abc#frag"),
+          )
+        end
+
+        def test_redact_url_does_not_echo_a_url_it_cannot_parse
+          assert_equal("[unparseable URL]", Discovery.redact_url("https://user:pass@srv.example.com/pr m.json?token=abc"))
+        end
+
         def test_canonicalize_url_normalizes_query_to_match_faraday
           # Faraday rewrites `env.url` before sending a request: it sorts
           # parameters by name, uppercases percent-encoded hex, and drops