feat(mcp-server): configurable basePath to avoid host OAuth route collisions#1737
Conversation
…lisions The embedded MCP server registers its OAuth, .well-known and /mcp routes at the host root, which overrides a host app's own /oauth/authorize and /oauth/token routes. Add an opt-in basePath option to mountAiMcpServer() that scopes every MCP route under a prefix (e.g. /mcp), so the MCP server coexists with the host's OAuth. Default (no basePath) is byte-identical to before. - normalizeMountPath rejects inputs that new URL() rewrites differently from the raw-string route mounts (backslash, .., %-escapes, path-to-regexp metacharacters), preventing a silent advertised-vs-mounted route desync. - authorization-server metadata is mounted manually at the RFC 8414 suffixed path, since the SDK's mcpAuthMetadataRouter only serves it at the root. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Coverage Impact ⬆️ Merging this pull request will increase total coverage on Modified Files with Diff Coverage (6)
🛟 Help
|
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
Address review findings on the basePath feature: - Preserve a path on the base URL (e.g. https://host/agent) by resolving the prefix against a trailing-slash base, instead of dropping the segment. - Match claimed routes on a segment boundary so a prefix like /ai/mcp no longer shadows unrelated host routes such as /ai/mcp-dashboard. - Reject Express-reserved '[' and ']' in normalizeMountPath. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
buildMcpPaths, normalizeMountPath and the McpRouteMatcher type are only used inside the package (server.ts and tests import them from ./mcp-paths). Only makeIsMcpRoute is consumed externally (by the agent). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
hercemer42
left a comment
hercemer42
left a comment
There was a problem hiding this comment.
I burn my tokens for you ! 😆
| // Match on a segment boundary so a claimed path like '/ai/mcp' does not shadow an | ||
| // unrelated host route like '/ai/mcp-dashboard'. | ||
| return (url: string) => | ||
| paths.some(p => url === p || url.startsWith(p.endsWith('/') ? p : `${p}/`)); |
There was a problem hiding this comment.
Claude Fable 5 — [Should fix] The matcher receives the raw req.url/ctx.url including the query string, so exact-match paths no longer match when one is present: isMcpRoute('/mcp?foo=1') → false (was true on main via startsWith('/mcp')); same for /mcp/mcp?x and the suffixed well-known paths under a prefix. That's a second default-mode behavior change beyond the documented /mcp-dashboard one — a query-string request to the MCP endpoint now silently falls through to the host. Match on the pathname (split at ?/#, or accept ${p}? as a boundary too) and add a '/mcp?foo=1' case to the matcher tests.
There was a problem hiding this comment.
Fixed in c293175 — makeIsMcpRoute now matches on the pathname (url.split(/[?#]/, 1)), so /mcp?foo=1 and the suffixed well-known paths match again. Added a /mcp?foo=1 case to the matcher tests.
| '', | ||
| ); | ||
|
|
||
| if (/[?#\s\\:*()+%[\]]/.test(collapsed) || urlPath !== collapsed) { |
There was a problem hiding this comment.
Claude Fable 5 — [Should fix] The denylist misses !, which path-to-regexp v8 (Express 5) treats as reserved: normalizeMountPath('/a!b') passes, then app.use('/a!b/oauth/token', …) throws Unexpected ! at index 2 … at startup — the cryptic failure this validator exists to prevent (verified against express 5.2.1). A per-segment allowlist would be more robust across path-to-regexp versions than a denylist, and also closes |/^ (WHATWG-legal, so they survive the new URL() round-trip, but RFC 3986-illegal — a conforming client may percent-encode them and miss the byte-exact matcher). Minimum fix: add ! to the character class.
There was a problem hiding this comment.
Fixed in c293175 — switched normalizeMountPath to a per-segment allowlist (/^(\/[A-Za-z0-9_-]+)+$/), which rejects !, |, ^ and every other path-to-regexp/URL-reserved char in one rule (verified /a!b now throws). This also let me drop the new URL() round-trip check.
| const baseWithSlash = effectiveBaseUrl.href.endsWith('/') | ||
| ? effectiveBaseUrl | ||
| : new URL(`${effectiveBaseUrl.href}/`); | ||
| const mountBase = prefix ? new URL(`${prefix.slice(1)}/`, baseWithSlash) : baseWithSlash; |
There was a problem hiding this comment.
Claude Fable 5 — [Should fix] When effectiveBaseUrl carries a path (e.g. https://host/agent) and a prefix is set, only the advertised URLs include the base path: the issuer becomes https://host/agent/mcp, so an RFC 8414 client fetches /.well-known/oauth-authorization-server/agent/mcp — but that route is mounted (L564) and claimed by makeIsMcpRoute at /.well-known/oauth-authorization-server/mcp only. Likewise the advertised token endpoint path is /agent/mcp/oauth/token while Express mounts /mcp/oauth/token. This works only if a proxy strips the base path — which can't happen for the root-anchored well-known path. The test "preserves a base path on the origin" (server.test.ts:2630) pins the metadata half of this desync as expected behavior. Either derive the well-known mount + matcher from base path + prefix, or reject/warn loudly on a pathful base URL when mountPath is set.
There was a problem hiding this comment.
Fixed in c293175 — rather than half-supporting a path-based base URL, buildExpressApp now throws when basePath is set and the base URL has a non-root path (OAuth discovery must live at the origin root). Removed the baseWithSlash preservation and the test that pinned the desync; added a rejection test.
| * Opt-in path prefix under which all MCP routes (OAuth, .well-known, /mcp) are mounted, | ||
| * e.g. '/mcp'. Defaults to the origin root. Use it to avoid colliding with a host app's | ||
| * own OAuth routes when the server is embedded in an agent. | ||
| */ |
There was a problem hiding this comment.
Claude Fable 5 — [Should fix] This says all MCP routes including .well-known are mounted under the prefix, but buildMcpPaths and L564 anchor the well-known routes at the origin root with the prefix as a suffix (/.well-known/oauth-authorization-server/mcp), per RFC 8414/9728. A user who reads this and proxies only /mcp/* to the agent breaks OAuth discovery. The same caveat is missing from the basePath example on mountAiMcpServer (agent.ts:249-256). One line on each: discovery metadata stays at the origin root, so root .well-known traffic must still reach the agent.
There was a problem hiding this comment.
Fixed in c293175 — JSDoc on the option now states discovery metadata stays at the origin root (prefix-suffixed), and the mountAiMcpServer example carries the same note. (Docs PR ForestAdmin/docs#10 updated too.)
| * e.g. '/mcp'. Defaults to the origin root. Use it to avoid colliding with a host app's | ||
| * own OAuth routes when the server is embedded in an agent. | ||
| */ | ||
| mountPath?: string; |
There was a problem hiding this comment.
Claude Fable 5 — [Should fix] One concept, two public names: the agent option is basePath (agent.ts:255, and the PR title/body) while the exported ForestMCPServerOptions calls it mountPath. Both ship as public API in this PR and renaming later is a breaking change on mcp-server — align on one name now (or cross-reference each in the JSDoc if the split is deliberate).
There was a problem hiding this comment.
Fixed in c293175 — renamed the mcp-server option mountPath → basePath (and the internal field) so it matches the agent's public mountAiMcpServer({ basePath }). One name across both packages now.
| ); | ||
| }); | ||
|
|
||
| test('should pass basePath to ForestMCPServer as mountPath', async () => { |
There was a problem hiding this comment.
Claude Fable 5 — [Should fix] This asserts the constructor arg only; no test pins that basePath reaches the Koa route gate (makeIsMcpRoute(this.mcpBasePath) → setMcpCallback(cb, matcher) → McpMiddleware.routeMatcher). Dropping the matcher argument at the start() call site keeps every test in this PR green, while Koa-mounted agents with a basePath would never route /<prefix>/mcp to the MCP server. Add an assertion that the matcher passed to setCallback claims /<prefix>/mcp and rejects root /oauth/token.
There was a problem hiding this comment.
Fixed in c293175 — added a test that spies setMcpCallback and asserts the threaded matcher claims /ai/mcp and rejects root /oauth/token.
| }); | ||
|
|
||
| it('serves protected-resource metadata under the prefixed resource path', async () => { | ||
| const response = await request(app).get('/.well-known/oauth-protected-resource/mcp/mcp'); |
There was a problem hiding this comment.
Claude Fable 5 — [Should fix] The prefix-mode metadata tests assert resource but never authorization_servers (the field the client actually follows to reach the issuer) nor registration_endpoint (which must remain the unprefixed Forest server URL, server.ts:515). If mcpAuthMetadataRouter were fed the wrong issuer, discovery would break with all tests green. Two one-line assertions close this.
There was a problem hiding this comment.
Fixed in c293175 — the prefix metadata tests now assert authorization_servers (the issuer the client follows) and registration_endpoint (unprefixed Forest server URL).
| setCallback(callback: HttpCallback | null): void { | ||
| setCallback(callback: HttpCallback | null, routeMatcher?: McpRouteMatcher): void { | ||
| this.mcpHttpCallback = callback; | ||
| this.routeMatcher = routeMatcher ?? isMcpRoute; |
There was a problem hiding this comment.
Claude Fable 5 — [Preferential] The local fallback isMcpRoute (L9-16) keeps the old bare-startsWith semantics — it still claims /mcp-dashboard, which makeIsMcpRoute deliberately stopped matching — and the class JSDoc above still presents the root paths as unconditional. The fallback is nearly dead (null callback ⇒ pass-through), but nothing marks it as fallback-only, so a future reader may trust the stale semantics. Align it with the segment-boundary matcher or mark it explicitly as the null-callback fallback.
There was a problem hiding this comment.
Kept as-is (preferential). The local isMcpRoute is only the default value before a matcher is set / when the callback is null — a pass-through case. When MCP is enabled the agent always passes the prefix-aware matcher via setCallback(cb, matcher), so the bare-startsWith semantics are never exercised in a live MCP mount. Importing the real matcher from @forestadmin/mcp-server here would defeat the dynamic-import laziness (non-MCP users must not load the package), which is why the local copy exists.
| }); | ||
|
|
||
| const httpCallback = await mcpServer.getHttpCallback(); | ||
| const isMcpRoute = makeIsMcpRoute(this.mcpBasePath); |
There was a problem hiding this comment.
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 in McpMiddleware). Having getHttpCallback() return its matcher — or passing the pair as one object to setMcpCallback — makes the mismatch unrepresentable, and would let agent/types.ts reuse McpRouteMatcher via import type instead of duplicating it.
There was a problem hiding this comment.
Acknowledged (preferential). The two matchers agree by construction — both are makeIsMcpRoute fed the same basePath (normalized identically), so they can't diverge without editing the function itself. Returning the matcher from getHttpCallback would change its return contract and ripple into the standalone run() path and its tests; I kept the threading (which mirrors the existing httpCallback channel) to keep the diff contained. Happy to do the object-return refactor as a follow-up if you prefer it.
| */ | ||
| async getHttpCallback(baseUrl?: URL): Promise<HttpCallback> { | ||
| const app = await this.buildExpressApp(baseUrl); | ||
| const isMcpRoute = makeIsMcpRoute(this.mountPath); |
There was a problem hiding this comment.
Claude Fable 5 — [Code style] The getHttpCallback docblock above still says the callback handles /.well-known/*, /oauth/*, /mcp — with mountPath set it handles the prefixed/suffixed set only, and root /oauth/* passes through (as the new pass-through test asserts). Worth updating while touching this.
There was a problem hiding this comment.
Fixed in c293175 — updated the getHttpCallback docblock to say it handles the OAuth/.well-known/protocol routes scoped under basePath, and passes through everything else.
- Match on the pathname in the route matcher so query strings (e.g. /mcp?x=1)
no longer fall through to the host app.
- Validate mount paths with an unreserved-character allowlist, rejecting
path-to-regexp metacharacters (including '!') that crash Express 5 at boot.
- Reject a basePath when the base URL is not at the domain root, instead of
advertising discovery URLs the mounted routes don't serve.
- Rename the mcp-server option mountPath -> basePath to match the agent's
public mountAiMcpServer({ basePath }) option.
- Clarify in JSDoc that .well-known discovery stays at the origin root.
- Add tests: query-string matching, '!' rejection, non-root base rejection,
authorization_servers/registration_endpoint metadata, and matcher threading
to the Koa middleware.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks for the thorough review 🙏 — addressed in c293175. Summary: Should-fix (all done):
Preferential (kept, rationale in-thread): the agent's local fallback matcher (dynamic-import laziness) and the twin-matcher threading vs returning it from mcp-server: 602 tests green, lint clean. Docs updated in ForestAdmin/docs#10. |
… basePath In prefix mode, mount both discovery documents ourselves at the RFC 8414/9728 suffixed paths and skip mcpAuthMetadataRouter, which would otherwise also mount the authorization-server metadata at the bare origin root and shadow a host app's own root discovery. Non-prefix mode is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| }); | ||
| }); | ||
|
|
||
| describe('mountPath prefix', () => { |
…th prefix' Follow-up to the mountPath -> basePath option rename. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
All review comments addressed 🙏 @hercemer42 — thanks for the deep pass, approved ✅. Every thread answered inline; the substantive ones landed in Macroscope — all findings auto-resolved:
Docs (ForestAdmin/docs#10) updated in lockstep — the four doc comments (blast-radius wording, CI is re-running on the latest commits. Ready to merge once green — noting the docs PR should merge only after this ships in a release. |
# @forestadmin/mcp-server [1.17.0](https://github.com/ForestAdmin/agent-nodejs/compare/@forestadmin/mcp-server@1.16.0...@forestadmin/mcp-server@1.17.0) (2026-07-06) ### Features * **mcp-server:** configurable basePath to avoid host OAuth route collisions ([#1737](#1737)) ([a815fc4](a815fc4))
# @forestadmin/agent [1.85.0](https://github.com/ForestAdmin/agent-nodejs/compare/@forestadmin/agent@1.84.1...@forestadmin/agent@1.85.0) (2026-07-06) ### Features * **mcp-server:** configurable basePath to avoid host OAuth route collisions ([#1737](#1737)) ([a815fc4](a815fc4)) ### Dependencies * **@forestadmin/mcp-server:** upgraded to 1.17.0
2 participants
Context
When the MCP server is embedded via
agent.mountAiMcpServer(), it registers its OAuth (/oauth/authorize,/oauth/token),.well-known/*and/mcproutes at the host root. If the host app already does OAuth, the MCP server overrides its routes and breaks the app's authentication (reported by a customer). The only workaround today is running the MCP server standalone.This PR adds an opt-in
basePathoption that scopes every MCP route under a prefix, so the MCP server coexists with the host's OAuth.How it works
/ai/oauth/authorize,/ai/oauth/token,/ai/mcp) and, per RFC 8414/9728, as a suffix to the root-anchored well-known paths (/.well-known/oauth-authorization-server/ai). The MCP client discovers all endpoints dynamically from the advertised metadata, so no path is hardcoded on the client side.mcpAuthMetadataRouteronly serves it at the root.basePath) is byte-identical to the previous behavior — zero breaking change for deployed MCP servers.basePath: '/mcp'yields the MCP endpoint at/mcp/mcp(the prefix applies to all routes). A distinct prefix like/aiavoids the doubling — to be documented.Validation
normalizeMountPathrejects any input thatnew URL()would rewrite differently from the raw-string route mounts (backslash,..,%-escapes, path-to-regexp metacharacters), preventing a silent advertised-vs-mounted route desync at this user-facing trust boundary.Tests
mcp-paths.test.ts(new): normalization, validation rejects, prefixed/nested path building, idempotenceserver.test.ts: prefix-mode metadata + endpoints +WWW-Authenticate,getHttpCallbackroute filtering (host routes pass through, prefixed routes delegated), nested/api/mcp, base-path preservationmcp-middleware.test.ts(new): Koa matcher gatingagent.test.ts:basePaththreadingmcp-server: 593 tests pass. Reviewed with the pr-review-toolkit agents + skeptic pass; findings addressed.
Definition of Done
General
Security
basePathtrust boundary; default behavior unchanged.🤖 Generated with Claude Code
Note
Add configurable
basePathtoForestMCPServerto avoid OAuth route collisions with the host appmountPathtoForestMCPServerOptionsthat scopes all MCP routes (/oauth,/.well-known,/mcp) under a URL prefix, avoiding collisions with host app routes.normalizeMountPath,buildMcpPaths, andmakeIsMcpRoutein mcp-paths.ts to validate prefixes, construct prefixed route lists, and match routes at segment boundaries.ForestMCPServer.buildExpressAppmounts OAuth and MCP endpoints under the prefix and serves RFC 8414 authorization-server metadata at the prefixed.well-knownpath instead of the host root.McpMiddlewareandFrameworkMounterare updated to accept a custom route matcher so that only prefixed MCP routes are intercepted by the middleware.isMcpRoutenow uses segment-boundary semantics, so paths like/mcp-dashboardno longer match as MCP routes.Changes since #1737 opened
buildMcpPaths,normalizeMountPath, andMcpRouteMatchertype frommcp-serverpackage [8f18b83]ForestMCPServer.buildExpressAppthat throws an error when abasePathis configured but the effective base URL contains a non-root pathname [c293175]normalizeMountPathfunction to only allow letters, digits, hyphens, and underscores per segment using regex pattern^(\/[A-Za-z0-9_-]+)+$[c293175]makeIsMcpRoutefunction to extract pathname by splitting on query string and fragment delimiters before performing route matching [c293175]mountPathoption tobasePaththroughoutmcp-serverpackage inForestMCPServerOptionsinterface,ForestMCPServerclass field, and all call sites inagentpackage [c293175]ForestMCPServerOptionsandAgent.mountAiMcpServerJSDoc to clarify that well-known OAuth discovery endpoints remain at origin root while OAuth and MCP protocol routes are mounted under thebasePath[c293175]ForestMCPServer.buildExpressAppto conditionally mount endpoints based on prefix presence [77cd41e]mcp-servertest suite from 'mountPath prefix' to 'basePath prefix' [5d1e2c8]Macroscope summarized d872b93.