-
Notifications
You must be signed in to change notification settings - Fork 49
feat(mcp): wire up oauth2 token verification for streamable-http transport #284
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
base: main
Are you sure you want to change the base?
Changes from all commits
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 |
|---|---|---|
|
|
@@ -130,6 +130,17 @@ async def lifespan(app: FastAPI): | |
| app.include_router(memory_router) | ||
|
|
||
|
|
||
| @app.get("/.well-known/oauth-authorization-server") | ||
| async def oauth_authorization_server_metadata(): | ||
| """RFC 9728 Protected Resource Metadata for MCP OIDC discovery.""" | ||
| if not settings.oauth2_issuer_url: | ||
| return {"error": "OIDC not configured"} | ||
| return { | ||
| "resource": f"https://{settings.oauth2_resource_host or 'localhost'}", | ||
| "authorization_servers": [settings.oauth2_issuer_url], | ||
| } | ||
|
Comment on lines
+136
to
+141
|
||
|
|
||
|
|
||
| def on_start_logger(port: int): | ||
| """Log startup information""" | ||
| print("\n-----------------------------------") | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -3,6 +3,8 @@ | |||||||||||||||||||||
| from typing import Any | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| import ulid | ||||||||||||||||||||||
| from mcp.server.auth.provider import AccessToken, TokenVerifier | ||||||||||||||||||||||
| from mcp.server.auth.settings import AuthSettings | ||||||||||||||||||||||
| from mcp.server.fastmcp import FastMCP as _FastMCPBase | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| from agent_memory_server import working_memory as working_memory_core | ||||||||||||||||||||||
|
|
@@ -179,22 +181,16 @@ async def run_sse_async(self): | |||||||||||||||||||||
| ).serve() | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| def streamable_http_app(self): | ||||||||||||||||||||||
| """Return a Starlette app for streamable-http with namespace routing.""" | ||||||||||||||||||||||
| from mcp.server.streamable_http_manager import StreamableHTTPSessionManager | ||||||||||||||||||||||
| from starlette.applications import Starlette | ||||||||||||||||||||||
| """Return a Starlette app for streamable-http with namespace routing. | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Extends the parent's streamable_http_app() by adding a namespace-prefixed | ||||||||||||||||||||||
| route while preserving auth middleware and protected resource routes. | ||||||||||||||||||||||
| """ | ||||||||||||||||||||||
| from starlette.requests import Request | ||||||||||||||||||||||
| from starlette.routing import Route | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| if self._session_manager is None: | ||||||||||||||||||||||
| self._session_manager = StreamableHTTPSessionManager( | ||||||||||||||||||||||
| app=self._mcp_server, | ||||||||||||||||||||||
| event_store=self._event_store, | ||||||||||||||||||||||
| retry_interval=self._retry_interval, | ||||||||||||||||||||||
| json_response=self.settings.json_response, | ||||||||||||||||||||||
| stateless=self.settings.stateless_http, | ||||||||||||||||||||||
| security_settings=self.settings.transport_security, | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| # Get the parent's app which includes auth middleware and well-known routes | ||||||||||||||||||||||
| app = super().streamable_http_app() | ||||||||||||||||||||||
| session_manager = self._session_manager | ||||||||||||||||||||||
| mcp_instance = self | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
@@ -212,14 +208,9 @@ async def __call__(self, scope, receive, send): | |||||||||||||||||||||
| handler = _NamespaceAwareHandler() | ||||||||||||||||||||||
| path = self.settings.streamable_http_path | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| return Starlette( | ||||||||||||||||||||||
| debug=self.settings.debug, | ||||||||||||||||||||||
| routes=[ | ||||||||||||||||||||||
| Route(path, endpoint=handler), | ||||||||||||||||||||||
| Route(f"/{{namespace}}{path}", endpoint=handler), | ||||||||||||||||||||||
| ], | ||||||||||||||||||||||
| lifespan=lambda app: session_manager.run(), | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
| # Add namespace-prefixed route to the existing app | ||||||||||||||||||||||
| app.routes.append(Route(f"/{{namespace}}{path}", endpoint=handler)) | ||||||||||||||||||||||
| return app | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| async def run_streamable_http_async(self): | ||||||||||||||||||||||
| """Start streamable HTTP MCP server.""" | ||||||||||||||||||||||
|
|
@@ -255,12 +246,51 @@ async def run_stdio_async(self): | |||||||||||||||||||||
| """ | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| class JWTTokenVerifier(TokenVerifier): | ||||||||||||||||||||||
| """Verify JWT tokens from Hydra using the existing auth module.""" | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| async def verify_token(self, token: str) -> AccessToken | None: | ||||||||||||||||||||||
| from agent_memory_server.auth import verify_jwt | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| try: | ||||||||||||||||||||||
| user_info = verify_jwt(token) | ||||||||||||||||||||||
|
Comment on lines
+253
to
+256
|
||||||||||||||||||||||
| from agent_memory_server.auth import verify_jwt | |
| try: | |
| user_info = verify_jwt(token) | |
| from anyio import to_thread | |
| from agent_memory_server.auth import verify_jwt | |
| try: | |
| user_info = await to_thread.run_sync(verify_jwt, token) |
Copilot
AI
Apr 16, 2026
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.
verify_token() catches Exception and returns None, which will silently convert internal errors (e.g., misconfiguration, JWKS fetch failures) into an auth failure and make diagnosis difficult. Consider catching only expected auth-related exceptions (e.g., FastAPI HTTPException/JWT validation errors) and logging them; let unexpected exceptions propagate so they surface as 5xx with logs.
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.
Sync HTTP call blocks async event loop
Low Severity
JWTTokenVerifier.verify_token is an async method that calls the synchronous verify_jwt() directly. Internally, verify_jwt uses httpx.Client (synchronous) to fetch JWKS keys, which can block the event loop for up to 10 seconds during cache misses. In the REST API, FastAPI runs sync dependencies in a thread pool automatically, but here the sync call runs directly on the event loop. Using asyncio.to_thread or similar would avoid the blocking.
Reviewed by Cursor Bugbot for commit 2e0b5c7. Configure here.
Copilot
AI
Apr 16, 2026
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.
_build_mcp_auth_kwargs() gates MCP auth on auth_mode and OAuth2 settings, but it ignores settings.disable_auth. That can lead to the REST API running with auth disabled while MCP enforces OAuth (or vice versa), which is surprising given disable_auth is treated as the global kill switch in auth.get_current_user(). Consider including settings.disable_auth in this guard (or aligning on a single source of truth for enabling auth).


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.
Error response returns HTTP 200 instead of error status
Medium Severity
When
oauth2_issuer_urlis not configured,oauth_authorization_server_metadatareturns{"error": "OIDC not configured"}with an implicit HTTP 200 status. Clients will interpret this as valid metadata and attempt to parse theresourceandauthorization_serversfields, leading to confusing failures downstream. An appropriate HTTP error status (e.g., 404 or 501) would correctly signal that the metadata is unavailable.Reviewed by Cursor Bugbot for commit 2e0b5c7. Configure here.