[Server] feat: relax StrictOidcDiscoveryMetadataPolicy and add Dynamic Client Registration middleware (RFC 7591)#269
Conversation
simonchrz
requested review from
CodeWithKyrian,
Nyholm and
chr-hertel
as code owners
|
There is an examples for microsoft connection. There is a trouble not only with OIDC discovery. https://github.com/modelcontextprotocol/php-sdk/blob/main/examples/server/oauth-microsoft/MicrosoftOidcMetadataPolicy.php Examples should be also updated if alternative OIDC discovery is accepted. There is also alternative way - in case when code_challenge_methods_supported is missed we can set it's value to default S256 |
src/Server/Transport/Http/Middleware/ClientRegistrationMiddleware.php
Outdated
Show resolved
Hide resolved
Good point — I've removed LenientOidcDiscoveryMetadataPolicy and relaxed StrictOidcDiscoveryMetadataPolicy to accept missing code_challenge_methods_supported. If the field is present it's still validated strictly; if absent, the downstream OAuthProxyMiddleware already defaults to ['S256']. The interface stays for custom policies (like the Microsoft example). |
simonchrz
changed the title
|
is the |
i guess just as an example how to implement OidcDiscoveryMetadataPolicyInterface |
|
I thought about this more, and I think my earlier comment was not precise enough. I agree we should support real-world providers like Microsoft that omit I would prefer to:
That keeps the naming and behavior clearer: |
|
Hi, The goal of the MCP SDK should be to implement the MCP protocol (JSON-RPC, tools, resources, prompts); not to become an OAuth server. The spec says the server must support OAuth, but that doesn't mean the SDK itself needs to embed a full OAuth implementation. Bundling Dynamic Client Registration (RFC 7591) directly into the SDK is a nonsense to me. |
|
Hi @Spomky, Providing something out of the box doesn't mean that we have to use it in context of Symfony Framework, Laravel etc. |
PSR-15 middleware that handles POST /register by delegating to a ClientRegistrarInterface and enriches /.well-known/oauth-authorization-server responses with the registration_endpoint.
- Use Mcp\Exception\InvalidArgumentException instead of global \InvalidArgumentException - Add interface documentation for ClientRegistrarInterface with RFC 7591 references
…idcDiscoveryMetadataPolicy - StrictOidcDiscoveryMetadataPolicy remains RFC-aligned: requires code_challenge_methods_supported - LenientOidcDiscoveryMetadataPolicy accepts missing code_challenge_methods_supported for providers like FusionAuth and Microsoft Entra ID that omit it despite supporting PKCE with S256 - If code_challenge_methods_supported is present, both policies validate it strictly - Update OidcDiscoveryTest with tests for both policies - Update Microsoft example README to reference the built-in lenient policy
…dcDiscoveryMetadataPolicy - Microsoft example now uses LenientOidcDiscoveryMetadataPolicy directly - Remove MicrosoftOidcMetadataPolicy and its test (functionally identical)
chr-hertel
changed the title
There was a problem hiding this comment.
Pull request overview
Adds HTTP-transport OAuth enhancements to improve compatibility with real-world OIDC providers and to support OAuth 2.0 Dynamic Client Registration as required by MCP servers.
Changes:
- Introduce
LenientOidcDiscoveryMetadataPolicyas an alternative to the strict discovery metadata validator. - Add
ClientRegistrationMiddleware(PSR-15) +ClientRegistrarInterfaceto implement RFC 7591 registration handling and metadata enrichment. - Update the Microsoft OAuth example to use the new built-in lenient policy and remove the example-specific policy.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Unit/Server/Transport/Http/OAuth/OidcDiscoveryTest.php | Adds coverage ensuring lenient policy accepts missing code_challenge_methods_supported. |
| tests/Unit/Server/Transport/Http/OAuth/LenientOidcDiscoveryMetadataPolicyTest.php | New unit tests for lenient discovery metadata validation rules. |
| tests/Unit/Server/Transport/Http/Middleware/ClientRegistrationMiddlewareTest.php | New unit tests for RFC 7591 middleware behavior and metadata enrichment. |
| src/Server/Transport/Http/OAuth/LenientOidcDiscoveryMetadataPolicy.php | New metadata policy that relaxes the strict requirement for PKCE method listing. |
| src/Server/Transport/Http/OAuth/ClientRegistrarInterface.php | New SPI for pluggable dynamic client registration implementations. |
| src/Server/Transport/Http/Middleware/ClientRegistrationMiddleware.php | New PSR-15 middleware handling /register and enriching auth server metadata. |
| src/Exception/ClientRegistrationException.php | New exception type for registrar failures. |
| examples/server/oauth-microsoft/tests/Unit/MicrosoftOidcMetadataPolicyTest.php | Removes example-specific tests tied to the removed policy. |
| examples/server/oauth-microsoft/server.php | Switches example to LenientOidcDiscoveryMetadataPolicy. |
| examples/server/oauth-microsoft/README.md | Updates documentation to match the new built-in policy usage. |
| examples/server/oauth-microsoft/MicrosoftOidcMetadataPolicy.php | Removes the example-specific metadata policy implementation. |
| CHANGELOG.md | Documents the two new additive features. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (isset($metadata['code_challenge_methods_supported'])) { | ||
| if (!\is_array($metadata['code_challenge_methods_supported']) || [] === $metadata['code_challenge_methods_supported']) { | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
In LenientOidcDiscoveryMetadataPolicy::isValid(), the docblock says that when code_challenge_methods_supported is present it is validated, but the code uses isset($metadata['code_challenge_methods_supported']). In PHP, isset() returns false when the key exists but its value is null, so a response containing "code_challenge_methods_supported": null would be treated as “absent” and therefore pass validation. Consider using array_key_exists() (and treating null as invalid) so the behavior matches the documented intent.
| $data = json_decode($body, true); | ||
|
|
||
| if (!\is_array($data)) { | ||
| return $this->jsonResponse(400, [ | ||
| 'error' => 'invalid_client_metadata', | ||
| 'error_description' => 'Request body must be valid JSON.', | ||
| ]); | ||
| } | ||
|
|
There was a problem hiding this comment.
handleRegistration() treats any non-array result from json_decode() as “invalid JSON”. This will also reject valid JSON values like strings/numbers/objects decoded as non-arrays, and it will accept JSON lists (e.g. []) as arrays even though RFC 7591 expects a JSON object. Consider decoding with JSON_THROW_ON_ERROR and then explicitly validating that the decoded value is an associative array (JSON object), updating the error_description accordingly (e.g. “Request body must be a JSON object.”).
| $data = json_decode($body, true); | |
| if (!\is_array($data)) { | |
| return $this->jsonResponse(400, [ | |
| 'error' => 'invalid_client_metadata', | |
| 'error_description' => 'Request body must be valid JSON.', | |
| ]); | |
| } | |
| try { | |
| $data = json_decode($body, true, 512, \JSON_THROW_ON_ERROR); | |
| } catch (\JsonException $e) { | |
| return $this->jsonResponse(400, [ | |
| 'error' => 'invalid_client_metadata', | |
| 'error_description' => 'Request body must be valid JSON.', | |
| ]); | |
| } | |
| if (!\is_array($data) || (\function_exists('array_is_list') && [] !== $data && \array_is_list($data))) { | |
| return $this->jsonResponse(400, [ | |
| 'error' => 'invalid_client_metadata', | |
| 'error_description' => 'Request body must be a JSON object.', | |
| ]); | |
| } |
| return $this->jsonResponse(200, $metadata, [ | ||
| 'Cache-Control' => $response->getHeaderLine('Cache-Control'), | ||
| ]); |
There was a problem hiding this comment.
enrichAuthServerMetadata() rebuilds a fresh JSON response via jsonResponse(), which drops all upstream/previous headers except Cache-Control (e.g. CORS headers, Vary, ETag, custom security headers). Since this middleware is meant to enrich an existing metadata response, consider preserving the original response headers and only replacing the body (and Content-Length) / adding registration_endpoint.
| return $this->jsonResponse(200, $metadata, [ | |
| 'Cache-Control' => $response->getHeaderLine('Cache-Control'), | |
| ]); | |
| $newBody = $this->streamFactory->createStream( | |
| json_encode($metadata, \JSON_THROW_ON_ERROR | \JSON_UNESCAPED_SLASHES), | |
| ); | |
| $response = $response | |
| ->withBody($newBody) | |
| ->withHeader('Content-Type', 'application/json') | |
| ->withoutHeader('Content-Length'); | |
| return $response; |
| return $this->jsonResponse(201, $result); | ||
| } |
There was a problem hiding this comment.
handleRegistration() can return sensitive fields like client_secret, but the response currently doesn’t set Cache-Control: no-store (or similar) to prevent intermediary/proxy caching. Consider adding Cache-Control: no-store (and optionally Pragma: no-cache) to both success (201) and error responses for /register, similar to how OAuth token responses are typically handled.
chr-hertel
added
the
needs more work
Add two features to the HTTP transport layer:
code_challenge_methods_supported
Motivation and Context
LenientOidcDiscoveryMetadataPolicy: Several identity providers (FusionAuth, Microsoft Entra ID) omit code_challenge_methods_supported from
their OIDC discovery response despite fully supporting PKCE with S256. The existing OidcDiscoveryMetadataPolicy rejects these responses,
making it impossible to use the SDK's OAuth transport with those providers without a custom policy. This provides a ready-made workaround.
ClientRegistrationMiddleware: The MCP specification requires servers to support OAuth 2.0 Dynamic Client Registration (RFC 7591) so that MCP
clients can register themselves automatically. This middleware handles POST /register by delegating to a ClientRegistrarInterface
implementation and enriches /.well-known/oauth-authorization-server responses with the registration_endpoint. The ClientRegistrarInterface
keeps the actual registration logic (e.g. calling FusionAuth, storing in a database) pluggable.
How Has This Been Tested?
required fields, empty strings, non-array input
enrichment with registration_endpoint, Cache-Control preservation, non-200 passthrough, non-matching route passthrough, empty localBaseUrl
rejection, trailing slash normalization
Breaking Changes
None. Both features are purely additive — new classes and interfaces only.
Types of changes
Checklist
Additional context
handler classes in tests)
advanced