-
Notifications
You must be signed in to change notification settings - Fork 12
feat(mcp-server): configurable basePath to avoid host OAuth route collisions #1737
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
58ecac5
d872b93
8f18b83
c293175
77cd41e
5d1e2c8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| import type { HttpCallback } from './types'; | ||
| import type { HttpCallback, McpRouteMatcher } from './types'; | ||
| import type Koa from 'koa'; | ||
|
|
||
| import expressToKoa from './utils/express-to-koa'; | ||
|
|
@@ -22,12 +22,11 @@ function isMcpRoute(url: string): boolean { | |
| */ | ||
| export default class McpMiddleware { | ||
| private mcpHttpCallback: HttpCallback | null = null; | ||
| private routeMatcher: McpRouteMatcher = isMcpRoute; | ||
|
|
||
| /** | ||
| * Set the MCP HTTP callback. Call this before using the middleware. | ||
| */ | ||
| setCallback(callback: HttpCallback | null): void { | ||
| setCallback(callback: HttpCallback | null, routeMatcher?: McpRouteMatcher): void { | ||
| this.mcpHttpCallback = callback; | ||
| this.routeMatcher = routeMatcher ?? isMcpRoute; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Claude Fable 5 — [Preferential] The local fallback
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Kept as-is (preferential). The local |
||
| } | ||
|
|
||
| /** | ||
|
|
@@ -64,8 +63,7 @@ export default class McpMiddleware { | |
| next(); | ||
| } | ||
| }, | ||
| // MCP routes are at root: /.well-known/*, /oauth/*, /mcp | ||
| isMcpRoute, | ||
| url => this.routeMatcher(url), | ||
| ); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| import McpMiddleware from '../src/mcp-middleware'; | ||
|
|
||
| describe('McpMiddleware', () => { | ||
| function makeCtx(url: string) { | ||
| return { url, req: {}, res: { once: jest.fn() }, respond: true } as any; | ||
| } | ||
|
|
||
| test('getKoaMiddleware gates requests with the custom route matcher', async () => { | ||
| const middleware = new McpMiddleware(); | ||
| const callback = jest.fn((req, res, next) => next()); | ||
| middleware.setCallback(callback, url => url.startsWith('/custom')); | ||
|
|
||
| const koaMiddleware = middleware.getKoaMiddleware(); | ||
|
|
||
| await koaMiddleware(makeCtx('/mcp'), jest.fn().mockResolvedValue(undefined) as any); | ||
| expect(callback).not.toHaveBeenCalled(); | ||
|
|
||
| await koaMiddleware(makeCtx('/custom/x'), jest.fn().mockResolvedValue(undefined) as any); | ||
| expect(callback).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| test('falls back to the default root matcher when none is provided', async () => { | ||
| const middleware = new McpMiddleware(); | ||
| const callback = jest.fn((req, res, next) => next()); | ||
| middleware.setCallback(callback); | ||
|
|
||
| const koaMiddleware = middleware.getKoaMiddleware(); | ||
|
|
||
| await koaMiddleware(makeCtx('/api/other'), jest.fn().mockResolvedValue(undefined) as any); | ||
| expect(callback).not.toHaveBeenCalled(); | ||
|
|
||
| await koaMiddleware(makeCtx('/mcp'), jest.fn().mockResolvedValue(undefined) as any); | ||
| expect(callback).toHaveBeenCalledTimes(1); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,56 @@ | ||
| /** MCP-specific paths that should be handled by the MCP server */ | ||
| export const MCP_PATHS = ['/.well-known/', '/oauth/', '/mcp']; | ||
| export type McpRouteMatcher = (url: string) => boolean; | ||
|
|
||
| /** Check if a URL is an MCP route */ | ||
| export function isMcpRoute(url: string): boolean { | ||
| return MCP_PATHS.some(p => url === p || url.startsWith(p)); | ||
| export function normalizeMountPath(input?: string): string { | ||
| if (!input) return ''; | ||
|
|
||
| const trimmed = input.trim(); | ||
| if (trimmed === '' || trimmed === '/') return ''; | ||
|
|
||
| const withLeadingSlash = trimmed.startsWith('/') ? trimmed : `/${trimmed}`; | ||
| const collapsed = withLeadingSlash.replace(/\/+/g, '/').replace(/\/+$/, ''); | ||
|
|
||
| // Allow only unreserved path characters per segment. The prefix is interpolated raw into | ||
| // Express/Koa route patterns (path-to-regexp) and resolved through new URL() for the OAuth | ||
| // metadata; an allowlist keeps those two interpretations identical and future-proof. | ||
| if (!/^(\/[A-Za-z0-9_-]+)+$/.test(collapsed)) { | ||
| throw new Error( | ||
| `Invalid MCP mount path "${input}": use a plain path prefix like "/mcp" ` + | ||
| `(letters, digits, "-" and "_" only).`, | ||
| ); | ||
| } | ||
|
|
||
| return collapsed; | ||
| } | ||
|
|
||
| /** | ||
| * Well-known paths stay anchored at the origin root (per RFC 8414/9728) but carry the prefix | ||
| * as a suffix, so a host's own root OAuth metadata is not claimed. | ||
| */ | ||
| export function buildMcpPaths(prefix = ''): string[] { | ||
| const normalized = normalizeMountPath(prefix); | ||
|
|
||
| const wellKnown = normalized | ||
| ? [ | ||
| `/.well-known/oauth-authorization-server${normalized}`, | ||
| `/.well-known/oauth-protected-resource${normalized}`, | ||
| ] | ||
| : ['/.well-known/']; | ||
|
|
||
| return [...wellKnown, `${normalized}/oauth/`, `${normalized}/mcp`]; | ||
| } | ||
|
|
||
| export function makeIsMcpRoute(prefix = ''): McpRouteMatcher { | ||
|
macroscopeapp[bot] marked this conversation as resolved.
|
||
| const paths = buildMcpPaths(prefix); | ||
|
|
||
| // Match on the pathname (req.url carries the query string) and on a segment boundary, so | ||
| // '/mcp?x=1' still matches and '/ai/mcp' does not shadow '/ai/mcp-dashboard'. | ||
| return (url: string) => { | ||
| const [pathname] = url.split(/[?#]/, 1); | ||
|
|
||
| return paths.some(p => pathname === p || pathname.startsWith(p.endsWith('/') ? p : `${p}/`)); | ||
| }; | ||
| } | ||
|
|
||
| export const MCP_PATHS = buildMcpPaths(''); | ||
|
|
||
| export const isMcpRoute = makeIsMcpRoute(''); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Claude Fable 5 — [Preferential] The matcher is rebuilt here from the raw string while the callback's internal filter builds its own from the normalized field (server.ts:646) — they agree only because both call the same function, and
{ mcpHttpCallback?; mcpIsMcpRoute? }makes callback-without-matcher representable (silently falling back to the root-path matcher inMcpMiddleware). HavinggetHttpCallback()return its matcher — or passing the pair as one object tosetMcpCallback— makes the mismatch unrepresentable, and would let agent/types.ts reuseMcpRouteMatcherviaimport typeinstead of duplicating it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acknowledged (preferential). The two matchers agree by construction — both are
makeIsMcpRoutefed the samebasePath(normalized identically), so they can't diverge without editing the function itself. Returning the matcher fromgetHttpCallbackwould change its return contract and ripple into the standalonerun()path and its tests; I kept the threading (which mirrors the existinghttpCallbackchannel) to keep the diff contained. Happy to do the object-return refactor as a follow-up if you prefer it.