fix(auth): close auth bypass on /mcp/{server_id} virtual server endpoints#3812
Conversation
|
@junlin3012 - thanks for the contribution! Can you please resolve conflicts, fix lint and test issues? And make sure that you sign off on your commits. https://github.com/IBM/mcp-context-forge/pull/3812/checks?check_run_id=68296600601 |
bf01825 to
f4d0901
Compare
|
@junlin3012 - Can you please resolve conflicts? |
…ints
Root cause: two independent failures combined to completely bypass
authentication for /mcp/{server_id} URLs:
1. Auth gate only checked path.endswith("/mcp"). Requests to
/mcp/{server_id} end in the server ID, so authenticate() returned
True immediately — skipping ALL authentication.
2. _SERVER_ID_RE only matched /servers/{id}/mcp. The /mcp/{id} pattern
never matched, so server_id was None and RBAC was skipped.
Changes:
- Extend _SERVER_ID_RE to match both /servers/{id}/mcp and /mcp/{id}
- Add _extract_server_id() helper for the two named groups
- Update auth gate to recognize /mcp/{id} as an MCP path
- Reorder _auth_no_token: per-server OAuth check runs before global auth
so oauth_enabled servers return RFC 9728 resource_metadata in 401
Fixes IBM#3752
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Jun Lin <206301840+junlin3012@users.noreply.github.com>
Three tests that use /servers/1/mcp in strict mode now need _check_server_oauth_enforcement mocked because per-server OAuth enforcement runs before the global mcp_require_auth check. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Jun Lin <206301840+junlin3012@users.noreply.github.com>
- Add missing blank line after _extract_server_id function (ruff E305) - Mock _check_server_oauth_enforcement in test_proxy_auth test that validates strict-mode 401 behavior (no DB in test env) Signed-off-by: Jun Lin <206301840+junlin3012@users.noreply.github.com>
…th collision
The dual-pattern regex (^/mcp/(?P<mcp_server_id>[^/]+)) would match
/mcp/sse and /mcp/message as server IDs, causing _validate_server_id()
to reject legitimate MCP SDK sub-paths with 404.
The /mcp/{server_id} URL pattern is not a documented access pattern —
it's an accidental Starlette mount artifact. The auth bypass is fully
closed by the path.startswith("/mcp/") guard in authenticate().
Changes:
- Restore single-pattern _SERVER_ID_RE from IBM#3892
- Remove _extract_server_id() helper (only one named group now)
- Revert match.group calls to use "server_id" directly
The two security-relevant fixes remain:
1. Auth gate: path.startswith("/mcp/") closes the bypass
2. _auth_no_token: per-server OAuth before global strict-mode check
Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
…bypass
Add 5 new tests covering the auth bypass fix:
- /mcp/{server_id} must enforce auth in strict mode (was 200 OK before)
- /mcp/{server_id} gets public-only scope in permissive mode
- Arbitrary /mcp/* sub-paths must not skip auth (parametrized)
- OAuth servers return resource_metadata in 401 even in strict mode
Mock _check_server_oauth_enforcement in 3 additional strict-mode tests
that use /servers/{id}/mcp/sse and /mcp/message paths, since per-server
OAuth now runs before the global mcp_require_auth check.
Update .secrets.baseline for shifted line numbers.
Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
The Starlette mount at /mcp routes ALL sub-paths to the MCP handler.
After the auth gate fix (path.startswith("/mcp/")), these paths pass
authentication in permissive mode and get treated as global /mcp —
exposing all public tools via an undocumented route surface.
Only /mcp, /mcp/, /mcp/sse, and /mcp/message are valid direct-access
endpoints. Server-scoped access uses /servers/{id}/mcp (rewritten by
MCPPathRewriteMiddleware). All other /mcp/* paths now return 404.
Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
Defense-in-depth: Uvicorn percent-decodes scope["path"] before our code runs (e.g. /mcp/%2e%2e/admin → /mcp/../admin). Add parametrized tests verifying the allowlist check rejects decoded traversal sequences, double-slash variants, and trailing-space paths. Addresses Codex review suggestion for proxy/path normalization drift hardening. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
f4d0901 to
dd6ceb4
Compare
There was a problem hiding this comment.
Review: Approved
Rebased onto main, resolved conflicts with #3892, reviewed, tested, and hardened.
Root cause: the auth gate only checked path.endswith("/mcp"), so /mcp/abc123 (ending in the server ID) skipped all authentication.
Changes after rebase
Original PR (3 commits by @junlin3012):
- Auth gate fix: added
/mcp/*path detection _auth_no_tokenreordering: per-server OAuth enforcement before global strict-mode check (RFC 9728)- Test mocks for strict-mode tests affected by the reordering
Maintainer additions (4 commits):
- Removed
/mcp/{id}regex extension — the dual-pattern regex (/mcp/(?P<mcp_server_id>...)) would match/mcp/sseand/mcp/messageas server IDs, breaking legitimate MCP SDK sub-paths with 404. The auth bypass is fully closed by the simplerpath.startswith("/mcp/")guard. - Reject undocumented
/mcp/*sub-paths with 404 — the Starlette mount at/mcproutes ALL sub-paths. Without this, permissive mode let/mcp/foothrough as a public-only global MCP request (undocumented route surface). Only/mcp,/mcp/,/mcp/sse,/mcp/messageare valid. - Added 11 deny-path regression tests — undocumented paths rejected in strict+permissive modes, decoded path traversal variants (
../,./,//, trailing space), OAuthresource_metadatapreserved in strict mode. - Decoded path traversal tests — defense-in-depth for Uvicorn's percent-decoding (e.g.
/mcp/%2e%2e/admin→/mcp/../admin).
Test results
- 430/430 transport unit tests pass
- 24/24 proxy auth tests pass
- All encoding attack vectors verified on live deployment:
%2e%2e,%2F,%252F,//,%00, case variation — all return 404 or 400 - Authenticated
/mcp,/servers/{id}/mcp, and/healthwork correctly
Effective production diff: +23 lines in 1 file
Three changes in _StreamableHttpAuthHandler:
is_mcp_pathgainspath.startswith("/mcp/")(1 line)- Undocumented
/mcp/*sub-path rejection with 404 (5 lines) _auth_no_token: per-server OAuth beforemcp_require_auth(reordering, net +5 lines)
There was a problem hiding this comment.
Summary
Reviewed the complete changeset, including maintainer hardening commits. This PR correctly fixes the critical authentication bypass with comprehensive defence-in-depth measures.
Key Strengths
- Root cause properly addressed - Auth gate now catches
/mcp/*paths that previously bypassed all checks - RFC 9728 compliance fixed - Per-server OAuth enforcement runs before global strict-mode check, ensuring
resource_metadataURL is always included - Defense-in-depth hardening - Explicit 404 rejection of undocumented
/mcp/*sub-paths prevents permissive-mode bypass - Comprehensive test coverage - 11 new deny-path regression tests covering path traversal, encoding attacks, and OAuth metadata preservation
Security Impact
Before: /mcp/{anything} → 200 OK with full tool access (complete auth bypass)
After: /mcp/{anything} → 404 (undocumented path rejected)
After: /servers/{id}/mcp → 401 with OAuth resource_metadata (proper RFC 9728 flow)
LGTM 🚀
There was a problem hiding this comment.
Confirmed the three core changes in _StreamableHttpAuthHandler:
- Auth gate fix —
path.startswith("/mcp/")correctly catches/mcp/{server_id}paths that previously bypassed all authentication by not ending in/mcp. - 404 block for undocumented
/mcp/*sub-paths — agree with the maintainer's decision to block these entirely rather than route them as server aliases. Cleaner and reduces attack surface. _auth_no_tokenreordering — per-server OAuth enforcement now runs before the globalmcp_require_authcheck, ensuring RFC 9728resource_metadataURL is always included inWWW-Authenticatefor OAuth-enabled servers.
LGTM
…ints (#3812) * fix(auth): close auth bypass on /mcp/{server_id} virtual server endpoints Root cause: two independent failures combined to completely bypass authentication for /mcp/{server_id} URLs: 1. Auth gate only checked path.endswith("/mcp"). Requests to /mcp/{server_id} end in the server ID, so authenticate() returned True immediately — skipping ALL authentication. 2. _SERVER_ID_RE only matched /servers/{id}/mcp. The /mcp/{id} pattern never matched, so server_id was None and RBAC was skipped. Changes: - Extend _SERVER_ID_RE to match both /servers/{id}/mcp and /mcp/{id} - Add _extract_server_id() helper for the two named groups - Update auth gate to recognize /mcp/{id} as an MCP path - Reorder _auth_no_token: per-server OAuth check runs before global auth so oauth_enabled servers return RFC 9728 resource_metadata in 401 Fixes #3752 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Jun Lin <206301840+junlin3012@users.noreply.github.com> * test(auth): mock per-server OAuth check in strict-mode tests Three tests that use /servers/1/mcp in strict mode now need _check_server_oauth_enforcement mocked because per-server OAuth enforcement runs before the global mcp_require_auth check. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Jun Lin <206301840+junlin3012@users.noreply.github.com> * fix: resolve lint (E305) and test failures for CI - Add missing blank line after _extract_server_id function (ruff E305) - Mock _check_server_oauth_enforcement in test_proxy_auth test that validates strict-mode 401 behavior (no DB in test env) Signed-off-by: Jun Lin <206301840+junlin3012@users.noreply.github.com> * fix(auth): remove /mcp/{id} regex extension to prevent SSE/message path collision The dual-pattern regex (^/mcp/(?P<mcp_server_id>[^/]+)) would match /mcp/sse and /mcp/message as server IDs, causing _validate_server_id() to reject legitimate MCP SDK sub-paths with 404. The /mcp/{server_id} URL pattern is not a documented access pattern — it's an accidental Starlette mount artifact. The auth bypass is fully closed by the path.startswith("/mcp/") guard in authenticate(). Changes: - Restore single-pattern _SERVER_ID_RE from #3892 - Remove _extract_server_id() helper (only one named group now) - Revert match.group calls to use "server_id" directly The two security-relevant fixes remain: 1. Auth gate: path.startswith("/mcp/") closes the bypass 2. _auth_no_token: per-server OAuth before global strict-mode check Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> * test(auth): add deny-path regression tests for /mcp/{server_id} auth bypass Add 5 new tests covering the auth bypass fix: - /mcp/{server_id} must enforce auth in strict mode (was 200 OK before) - /mcp/{server_id} gets public-only scope in permissive mode - Arbitrary /mcp/* sub-paths must not skip auth (parametrized) - OAuth servers return resource_metadata in 401 even in strict mode Mock _check_server_oauth_enforcement in 3 additional strict-mode tests that use /servers/{id}/mcp/sse and /mcp/message paths, since per-server OAuth now runs before the global mcp_require_auth check. Update .secrets.baseline for shifted line numbers. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> * fix(auth): reject undocumented /mcp/* sub-paths with 404 The Starlette mount at /mcp routes ALL sub-paths to the MCP handler. After the auth gate fix (path.startswith("/mcp/")), these paths pass authentication in permissive mode and get treated as global /mcp — exposing all public tools via an undocumented route surface. Only /mcp, /mcp/, /mcp/sse, and /mcp/message are valid direct-access endpoints. Server-scoped access uses /servers/{id}/mcp (rewritten by MCPPathRewriteMiddleware). All other /mcp/* paths now return 404. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> * test(auth): add decoded path traversal variant tests Defense-in-depth: Uvicorn percent-decodes scope["path"] before our code runs (e.g. /mcp/%2e%2e/admin → /mcp/../admin). Add parametrized tests verifying the allowlist check rejects decoded traversal sequences, double-slash variants, and trailing-space paths. Addresses Codex review suggestion for proxy/path normalization drift hardening. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> --------- Signed-off-by: Jun Lin <206301840+junlin3012@users.noreply.github.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Mihai Criveti <crivetimihai@gmail.com>
Summary
Complete authentication bypass on
/mcp/{server_id}virtual server endpoints. Any request — no token, garbage token, forged JWT,alg:noneattack — receives 200 OK with full tool access. Theoauth_enabledflag andMCP_REQUIRE_AUTHsetting have no effect on this URL pattern.Root Cause
Two independent failures combine to bypass all authentication:
1. Auth gate skips
/mcp/{id}entirely (line 3027)/mcp/abc123does not end in/mcp— it ends in the server ID. Soauthenticate()returnsTrueimmediately. No JWT check, no proxy auth, no OAuth enforcement.2.
_SERVER_ID_REnever matches/mcp/{id}(line 100)Only matches
/servers/{id}/mcp. The/mcp/{id}pattern — which Starlette's mount actually routes — is invisible. Server ID is alwaysNone, RBAC is skipped,_check_server_oauth_enforcement()gets a null server.3. Strict mode hides
resource_metadataURLWhen
MCP_REQUIRE_AUTH=true, the global auth check runs before per-server OAuth enforcement, returning a genericWWW-Authenticate: Bearerwithout the RFC 9728resource_metadataURL. MCP clients cannot discover the OAuth server.Fix
4 changes, 2 files, +43/-15 lines:
_SERVER_ID_REto match both/servers/{id}/mcpand/mcp/{id}_extract_server_id()helper for the two named regex groups/mcp/{id}(and any/mcp/*path the Starlette mount routes)_auth_no_token: per-server OAuth check runs before globalmcp_require_authsoresource_metadataURL is always includedTest Evidence
Live Cloud Run testing (v1.0.0-RC2, PostgreSQL, Keycloak 26.x SSO)
/mcp/{id}alg:noneattack/mcp/test)/servers/{id}/mcp/mcp/{id}/health(no auth)Unit tests
Adversarial testing
Test environment
oauth_enabled=true,authorization_servers: [keycloak]Updated tests
3 existing tests that use
/servers/1/mcpin strict mode now mock_check_server_oauth_enforcementbecause per-server OAuth enforcement runs before the globalmcp_require_authcheck (the intended behavioral change).Related issues
How to reproduce
🤖 Generated with Claude Code