Repository navigation
[v1.x] fix(auth): reject non-loopback http and fragment redirect URIs at registration - #3143
samuelmbabhazi wants to merge 2 commits 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: 353427c 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: |
JosephDoUrden
left a comment
JosephDoUrden
left a comment
There was a problem hiding this comment.
pulled b65b1ae onto the v1.x tip and ran it: register.test.ts 17/17, server auth + client auth + shared 616/616 across 20 files, hosting-express e2e 17/17 with the known failure gone. eslint, prettier and tsc clean. merge against v1.x is a no-op.
also poked the handler with a few extra inputs. HTTP://attacker.example/cb, http://localhost.evil.com/cb, http://[::ffff:127.0.0.1]/cb and myapp://cb#frag all get 400. http://127.0.0.1:0/cb, http://[::1]:8080/cb, http://0x7f.0.0.1/cb (the url parser folds it to 127.0.0.1) and com.example.app:/cb get 201. that matches rfc 8252 §7.3 loopback and the mcp communication-security rule.
one thing on the error code. src/server/auth/handlers/register.ts:92 throws InvalidClientMetadataError, so the body says invalid_client_metadata. rfc 7591 §3.2.2 has a dedicated code for this case, invalid_redirect_uri ("the value of one or more redirection URIs is invalid"), and #3142 asks for that one.
the parallel patch for server-legacy in the issue thread uses it too, so v1.x and main would answer the same bad input with different codes.
the e2e requirement already expects invalid_client_metadata, so this is a maintainer call rather than a blocker. if they want 7591, it is a small error class in errors.ts plus the three assertions.
looks good to me otherwise.
|
Kept |
RFC 7591 section 3.2.2 has a dedicated code for this case, and modelcontextprotocol#3142 asks for it. CustomOAuthError keeps the change free of new public API on v1.x.
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_redirect_uri(RFC 7591 §3.2.2). This removes the known failure ofhosting:auth:as:redirect-uri-schemeintest/e2e/requirements.ts, and the scenario now asserts that code and 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.