Repository navigation
[v1.x] fix(auth): reject non-loopback http and fragment redirect URIs at registration - #3143
Open
samuelmbabhazi wants to merge 1 commit into
Conversation
…istration The registration handler validated redirect_uris only with SafeUrlSchema, so it registered http://attacker.example.com/callback and URIs with a fragment. The MCP authorization spec requires HTTPS or a loopback host, and RFC 6749 §3.1.2 forbids a fragment. Both now get 400 invalid_client_metadata, which is what the hosting:auth:as:redirect-uri-scheme requirement already expected, so its known failure is removed and the scenario also covers a fragment. The loopback hosts are the ones the authorize handler already relaxes ports for (localhost, 127.0.0.1, [::1]). Private-use schemes stay accepted. Fixes modelcontextprotocol#3142
🦋 Changeset detectedLatest commit: b65b1ae The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3142.
The registration handler of
mcpAuthRoutervalidatedredirect_urisonly withSafeUrlSchema, so it registeredhttp://attacker.example.com/callbackandhttps://client.example.com/callback#fragmentwith 201. The spec requires HTTPS or a loopback host for redirect URIs (Communication Security), and RFC 6749 §3.1.2 forbids a fragment.The handler now answers both with
400 invalid_client_metadata. That is the responsehosting:auth:as:redirect-uri-schemeintest/e2e/requirements.tsalready expected, so this removes its known failure, and the scenario now also registers a URI with a fragment.OAuthClientMetadataSchema, which clients also use to parse metadata.localhost,127.0.0.1,[::1]), soLOOPBACK_HOSTSis exported fromauthorize.tsand reused rather than defined twice.com.example.app:/callbackare still accepted. No new option.The changeset says what breaks: re-registering a non-loopback
http:or fragment URI now fails, existing registrations are untouched.Unit tests in
register.test.tscover the rejected cases, loopbackhttp:on any port, and a private-use scheme. Without the change, the two rejection tests fail and the e2e scenario fails once the known failure is removed.