Conversation
Fixes an issue where the runtime MCP proxy router strictly matched /mcp/proxy/:serverId without a wildcard, causing requests with subpaths (e.g. /messages, /sse) to fail with 404. Also ensures toUpstreamProxyUrl safely appends the subpath onto the resolved upstream URL instead of truncating it.
|
All contributors on this pull request have signed the CLA. |
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
Yevanchen
left a comment
There was a problem hiding this comment.
Review: MCP proxy subpaths at 18de54f
Passed
just test-file apps/api/tests/driver-mcp-proxy-route.test.ts: 5 passed.just test-file apps/api/tests/driver-llm-proxy-route.test.ts: 33 passed.just tc-package @mosoo/api: passed.just fmt-check-path apps/api/tests/driver-mcp-proxy-route.test.ts: passed.- A local HTTP fixture exercising the actual Driver
executeRemoteHttpMcpCommand, pinned MCP SDK, grant verifier, and Hono route successfully initialized and executedtools/callon both the parent route and this PR. All requests used the same proxy URL and the same configured upstream MCP endpoint.
Findings
- [P1] The wildcard expands use of the stored upstream credential to arbitrary descendant paths and methods. See the inline comment at the new route registration.
- [P2] Subpath normalization removes significant trailing slashes and changes the target endpoint. See the inline comment at URL construction.
Verification gaps and issue scope
The transport premise in the PR description needs correction. Streamable HTTP uses one MCP endpoint; tools/call is a JSON-RPC method. The checked-in Driver custom executor explicitly uses StreamableHTTPClientTransport, and the other backend configurations receive the complete proxyUrl.
Legacy HTTP+SSE does use a separately advertised POST URI. That compatibility gap remains after this change: with upstream /base/sse sending event: endpoint\ndata: /messages?sessionId=review-session, the pinned SSEClientTransport sends its next POST to /messages?sessionId=review-session on the API origin, receives 404, and never reaches the wildcard proxy. This also fails on the parent. The new tests manually choose proxy subpaths and return JSON for the supposed SSE response, so they do not exercise this negotiation.
The local reproduction uses fixture target/vault resolution and a local upstream. It establishes routing behavior, not full Sandbox/provider or production acceptance. I did not run the full just check gate. GitHub's PR Check run is currently action_required, so the metadata checks alone do not establish full CI success.
Verdict
NEEDS CHANGES. Please preserve the credential boundary and exact endpoint path, and provide the actual failing MCP client/server/transport trace from #626 before settling the extra routing contract.
| }; | ||
|
|
||
| driver.all("/mcp/proxy/:serverId", handleRuntimeMcpProxy); | ||
| driver.all("/mcp/proxy/:serverId/*", handleRuntimeMcpProxy); |
There was a problem hiding this comment.
[P1] Keep credential-bearing requests within admitted MCP transport targets
This wildcard accepts every HTTP method and arbitrary descendants, while resolveRuntimeMcpProxyTarget receives no subpath to authorize and proxyRuntimeMcpRequest injects the stored upstream bearer credential. With a registered URL ending in /base, a valid runtime grant now permits DELETE /api/driver/mcp/proxy/<id>/admin/config to become an authenticated DELETE /base/admin/config. I reproduced the boundary change against a local HTTP fixture: the parent returns 404 without contacting upstream; this commit forwards the credential and returns 200. If the configured prefix contains privileged non-MCP endpoints, or the registered URL is the origin root, Sandbox code gains access to those operations using the owner's credential. Keep Streamable HTTP bound to the configured endpoint; any additional legacy/custom transport targets need an explicit validated mapping and method policy. Please add a denial test proving unrelated descendants cannot receive the upstream credential.
| const trimmedSubPath = subPath.replace(/^\/+|\/+$/g, ""); | ||
| if (trimmedSubPath.length > 0) { | ||
| target.pathname = target.pathname.replace(/\/+$/, "") + `/${trimmedSubPath}`; |
There was a problem hiding this comment.
[P2] Preserve the downstream endpoint's trailing slash
subPath.replace(/^\/+|\/+$/g, "") removes a significant trailing slash as well as the join separator. With upstream /base, a request to /api/driver/mcp/proxy/<id>/messages/ is forwarded to /base/messages, although /base/messages/ and /base/messages can be different endpoints. A local upstream accepting only /base/messages/ returns 200 directly and 404 through this PR. Normalize the base/suffix join without removing the validated suffix's trailing slash, including a suffix of /, and add coverage for that case.
Summary
/mcp/proxy/:serverId/*alongside/mcp/proxy/:serverId).extractMcpProxySubPathwith percent-encoding and path traversal guards (..,\,%25,%2f,%5c, and malformed encoding).toUpstreamProxyUrlto correctly append subpaths to upstream target URLs while normalizing trailing slashes and preserving query parameters.grantparameter stripping.Why
/sse,/messages,/tools/call).toUpstreamProxyUrlunconditionally dropped subpaths even if received.Verification
bun test apps/api/tests/driver-mcp-proxy-route.test.ts(5 pass, 0 fail, 17 expects)bun test apps/api/tests/driver-llm-proxy-route.test.ts(33 pass, 0 fail)bun x tsc -p apps/api/tsconfig.json --noEmit(0 errors)vp fmt --check(100% matched)bun scripts/validate-commit-range.ts upstream/main HEAD(1 commit passed)Impact
Review
apps/api/src/adapters/http/routes/driver-route.tsextractMcpProxySubPathandtoUpstreamProxyUrlinternal/private to align with the existingextractLlmProxySubPathpattern and avoid unnecessary public API surface expansion.Design (UI changes only, otherwise N/A)