Skip to content

fix(mcp): allow loopback redirect_uri with any port per RFC 8252#1824

Merged
yusukebe merged 5 commits intohonojs:mainfrom
arjunkmrm:fix/rfc8252-loopback-redirect-uri
Mar 30, 2026
Merged

fix(mcp): allow loopback redirect_uri with any port per RFC 8252#1824
yusukebe merged 5 commits intohonojs:mainfrom
arjunkmrm:fix/rfc8252-loopback-redirect-uri

Conversation

@arjunkmrm
Copy link
Copy Markdown
Contributor

We ran into this while running Smithery as an MCP OAuth proxy using @hono/mcp. Claude Code registers http://localhost/callback in its client metadata but connects with an ephemeral port at runtime (e.g. http://localhost:58621/callback). The authorize handler rejects it because redirect_uris.includes() does a strict string comparison.

RFC 8252 Section 7.3 says loopback redirect URIs should match on scheme + host + path only, ignoring the port — native OAuth clients pick a random available port each time. Here's the excerpt:

The authorization server MUST allow any port to be specified at the
  time of the request for loopback IP redirect URIs, to accommodate
  clients that obtain an available ephemeral port from the operating
  system at the time of the request.

This adds a small isLoopbackRedirectAllowed() helper that kicks in only for localhost, 127.0.0.1, and [::1]. Everything else still uses strict matching. Four new tests cover the happy path, path mismatch rejection, and non-loopback strictness.

Fixes #1823

The author should do the following, if applicable

  • Add tests
  • Run tests
  • yarn changeset at the top of this repo and push the changeset
  • Follow the contribution guide

Note: haven't run yarn changeset yet — happy to add one if this approach looks right.

The authorize handler does strict string matching on redirect_uris,
rejecting localhost callbacks with ephemeral ports (e.g.
localhost:58621/callback) when the client registered localhost/callback
without a port.

Per RFC 8252 Section 7.3, loopback redirect URIs should match on
scheme+host+path only, ignoring port. This adds an
isLoopbackRedirectAllowed() check for localhost, 127.0.0.1, and [::1].

Non-loopback URIs still use strict matching.
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Mar 25, 2026

🦋 Changeset detected

Latest commit: dfddd05

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@hono/mcp Patch

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

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 26, 2026

Codecov Report

❌ Patch coverage is 89.47368% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.71%. Comparing base (d41b897) to head (dfddd05).
⚠️ Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
packages/mcp/src/auth/helpers/authorize.ts 89.47% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1824      +/-   ##
==========================================
- Coverage   91.73%   91.71%   -0.02%     
==========================================
  Files         113      113              
  Lines        3785     3803      +18     
  Branches      957      964       +7     
==========================================
+ Hits         3472     3488      +16     
- Misses        281      283       +2     
  Partials       32       32              
Flag Coverage Δ
mcp 90.48% <89.47%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@yusukebe
Copy link
Copy Markdown
Member

Hey @MathurAditya724 , Can you reivew this?

Copy link
Copy Markdown
Contributor

@MathurAditya724 MathurAditya724 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 🔥, just a minor change and then we can merge it

Comment thread packages/mcp/src/auth/helpers/authorize.ts Outdated
Addresses review feedback to also compare req.search in
isLoopbackRedirectAllowed, preventing loopback URIs with
extra query parameters from bypassing validation.
Copy link
Copy Markdown
Contributor

@MathurAditya724 MathurAditya724 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🔥

Copy link
Copy Markdown
Member

@yusukebe yusukebe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@yusukebe
Copy link
Copy Markdown
Member

@arjunkmrm @MathurAditya724 Thank you!

@yusukebe yusukebe merged commit 6bb1522 into honojs:main Mar 30, 2026
94 of 96 checks passed
@github-actions github-actions Bot mentioned this pull request Mar 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(mcp): authorize handler rejects localhost redirect_uri with different port (RFC 8252)

3 participants