Repository navigation
fix(mcp): make browser admission exact and byte-bounded #286
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
202094c
b339880
54fa2a2
56625df
c0cf0bb
0914883
6463ea3
0208932
c6d549e
6035aeb
5121917
f12dcf4
2c82e87
7903303
95c873d
53bfd63
34f1d5e
1291505
e1d2bd7
aa0307e
d0df01d
b80729a
1393aca
3db763c
5b139f3
16277d2
d2c2a6b
2b86ec2
1175cd9
65a461d
ca84b63
510d7fc
f277ea2
8be7fbb
040aabb
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 |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| # 2.13.1 — MCP browser and request-byte admission | ||
|
|
||
| ## Fixed | ||
|
|
||
| - Browser MCP clients can complete an exact-Origin CORS preflight before OAuth | ||
| while wildcard, prefix, suffix, `null`, and unrelated Origins fail closed. | ||
| - Unsafe configured Origins now prevent startup instead of weakening the exact | ||
| allowlist into wildcard, credential-bearing, or path-bearing CORS. | ||
| - Origin-sensitive MCP responses vary by Origin and expose only the protocol, | ||
| session, and `WWW-Authenticate` headers needed for Streamable HTTP and OAuth | ||
| protected-resource discovery. | ||
| - MCP POST bodies are bounded by actual streamed bytes before OAuth and SDK JSON | ||
| decoding. Ambiguous framing, declared/actual mismatches, malformed body | ||
| streams, and over-limit requests return stable no-store errors without | ||
| echoing request content. | ||
| - Non-browser MCP clients may continue to omit `Origin`. | ||
|
|
||
| ## Operations | ||
|
|
||
| - Added `MCP_MAX_REQUEST_BYTES` with a 65,536-byte default and an explicit | ||
| 8,192–1,048,576-byte startup-validated range. | ||
| - Docker Compose, `.env.example`, integration guidance, ADR 0119, and MCP | ||
| standards traceability now share the same browser and body-admission contract. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,132 @@ | ||
| """Bound MCP request bodies before OAuth and JSON decoding.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import json | ||
| from collections.abc import Sequence | ||
|
|
||
| from starlette.types import ASGIApp, Message, Receive, Scope, Send | ||
|
|
||
|
|
||
| class BoundedRequestBodyApp: | ||
| """Reject ambiguous or oversized MCP POST bodies before downstream parsing. | ||
|
|
||
| ``Content-Length`` is useful for an early rejection but is never trusted as | ||
| the only resource boundary. Requests without it are read incrementally into | ||
| a buffer capped at ``maximum_bytes``. A successful request is replayed once | ||
| to the downstream ASGI application without changing its bytes. | ||
| """ | ||
|
|
||
| def __init__(self, app: ASGIApp, *, maximum_bytes: int) -> None: | ||
| """Wrap ``app`` with one positive finite body-size limit.""" | ||
| if maximum_bytes <= 0: | ||
| raise ValueError("maximum_bytes must be positive") | ||
| self._app = app | ||
| self._maximum_bytes = maximum_bytes | ||
|
|
||
| async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: | ||
| """Apply admission to HTTP POST and pass other ASGI traffic unchanged.""" | ||
| if scope["type"] != "http" or scope.get("method") != "POST": | ||
| await self._app(scope, receive, send) | ||
| return | ||
|
|
||
| content_lengths = _header_values(scope, b"content-length") | ||
| transfer_encodings = _header_values(scope, b"transfer-encoding") | ||
| declared_length = _parse_content_length(content_lengths, transfer_encodings) | ||
| if declared_length is _INVALID_LENGTH: | ||
| await _send_error(send, 400, "mcp_invalid_content_length") | ||
| return | ||
| if isinstance(declared_length, int) and declared_length > self._maximum_bytes: | ||
| await _send_error(send, 413, "mcp_request_too_large") | ||
| return | ||
|
|
||
| body = bytearray() | ||
| while True: | ||
| message = await receive() | ||
| message_type = message.get("type") | ||
| if message_type == "http.disconnect": | ||
| await _send_error(send, 400, "mcp_request_disconnected") | ||
| return | ||
| if message_type != "http.request": | ||
| await _send_error(send, 400, "mcp_invalid_request_body") | ||
| return | ||
| chunk = message.get("body", b"") | ||
| if not isinstance(chunk, bytes): | ||
| await _send_error(send, 400, "mcp_invalid_request_body") | ||
| return | ||
| if len(body) + len(chunk) > self._maximum_bytes: | ||
| await _send_error(send, 413, "mcp_request_too_large") | ||
| return | ||
| body.extend(chunk) | ||
| if not message.get("more_body", False): | ||
| break | ||
|
|
||
| if isinstance(declared_length, int) and declared_length != len(body): | ||
| await _send_error(send, 400, "mcp_content_length_mismatch") | ||
| return | ||
|
|
||
| replayed = False | ||
|
|
||
| async def replay_receive() -> Message: | ||
| nonlocal replayed | ||
| if replayed: | ||
| return {"type": "http.disconnect"} | ||
| replayed = True | ||
| return {"type": "http.request", "body": bytes(body), "more_body": False} | ||
|
|
||
| await self._app(scope, replay_receive, send) | ||
|
|
||
|
|
||
| class _InvalidLength: | ||
| """Sentinel distinguishing an invalid length from an absent one.""" | ||
|
|
||
|
|
||
| _INVALID_LENGTH = _InvalidLength() | ||
|
|
||
|
|
||
| def _header_values(scope: Scope, name: bytes) -> tuple[bytes, ...]: | ||
| """Return every raw value for one case-insensitive ASGI request header.""" | ||
| return tuple( | ||
| value | ||
| for header_name, value in scope.get("headers", []) | ||
| if header_name.lower() == name | ||
| ) | ||
|
|
||
|
|
||
| def _parse_content_length( | ||
| content_lengths: Sequence[bytes], | ||
| transfer_encodings: Sequence[bytes], | ||
| ) -> int | None | _InvalidLength: | ||
| """Return an unambiguous nonnegative length, absence, or invalid sentinel.""" | ||
| if len(content_lengths) > 1: | ||
| return _INVALID_LENGTH | ||
| if content_lengths and transfer_encodings: | ||
| return _INVALID_LENGTH | ||
| if not content_lengths: | ||
| return None | ||
| try: | ||
| decoded = content_lengths[0].decode("ascii") | ||
| except UnicodeDecodeError: | ||
| return _INVALID_LENGTH | ||
| if not decoded or not decoded.isdecimal(): | ||
| return _INVALID_LENGTH | ||
| try: | ||
| return int(decoded, 10) | ||
| except ValueError: | ||
| return _INVALID_LENGTH | ||
|
Comment on lines
+107
to
+116
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. 📝 Info: Oversized Content-Length relies on Python 3.12 int-string digit limit
Was this helpful? React with 👍 or 👎 to provide feedback. |
||
|
|
||
|
|
||
| async def _send_error(send: Send, status_code: int, error_code: str) -> None: | ||
| """Send one bounded payload-safe admission error response.""" | ||
| body = json.dumps( | ||
| {"error_code": error_code}, | ||
| ensure_ascii=True, | ||
| separators=(",", ":"), | ||
| ).encode("ascii") | ||
| headers = [ | ||
| (b"content-type", b"application/json"), | ||
| (b"content-length", str(len(body)).encode("ascii")), | ||
| (b"cache-control", b"no-store"), | ||
| ] | ||
| await send({"type": "http.response.start", "status": status_code, "headers": headers}) | ||
| await send({"type": "http.response.body", "body": body}) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ | |
| from contextlib import asynccontextmanager | ||
| from dataclasses import asdict, dataclass | ||
| from typing import Annotated, Any, Literal | ||
| from urllib.parse import urlsplit | ||
|
|
||
| from mcp.server import MCPServer | ||
| from mcp.server.auth.middleware.auth_context import get_access_token | ||
|
|
@@ -19,6 +20,7 @@ | |
| ) | ||
| from mcp.types import CallToolResult, ImageContent, TextContent, ToolAnnotations | ||
| from pydantic import AnyHttpUrl, BaseModel, Field | ||
| from starlette.middleware.cors import CORSMiddleware | ||
| from starlette.requests import Request | ||
| from starlette.types import ASGIApp, Receive, Scope, Send | ||
|
|
||
|
|
@@ -33,6 +35,7 @@ | |
| NullGlobalAskExternalVerifier, | ||
| SearxngOrchestratorGlobalAskVerifier, | ||
| ) | ||
| from backend.app.mcp_admission import BoundedRequestBodyApp | ||
| from backend.app.mcp_auth import KeycloakMcpTokenVerifier | ||
| from lineageweave.image_content import orchestrator_vision_client | ||
| from lineageweave.post_chat import ( | ||
|
|
@@ -107,6 +110,8 @@ async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: | |
| is_post=request.method == "POST", | ||
| ) | ||
| if rejection is not None: | ||
| if request.headers.get("origin") is not None: | ||
| rejection.headers.add_vary_header("Origin") | ||
| await rejection(scope, receive, send) | ||
| return | ||
|
Comment on lines
112
to
116
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. 🔍 Transport security validates disallowed Origins on preflight before CORS short-circuits The disallowed-origin preflight test (test_mcp_cors_preflight.py) expects HTTP 403 with Was this helpful? React with 👍 or 👎 to provide feedback. |
||
| await self._app(scope, receive, send) | ||
|
|
@@ -118,6 +123,27 @@ async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: | |
| AccessTokenProvider = Callable[[], AccessToken | None] | ||
|
|
||
|
|
||
| def _validate_mcp_allowed_origins(origins: list[str]) -> None: | ||
| """Require browser allowlist entries to be exact HTTP(S) origins.""" | ||
| for origin in origins: | ||
| parsed = urlsplit(origin) | ||
| invalid = ( | ||
| origin in {"*", "null"} | ||
| or parsed.scheme not in {"http", "https"} | ||
| or not parsed.netloc | ||
| or parsed.username is not None | ||
| or parsed.password is not None | ||
| or parsed.path != "" | ||
| or parsed.query != "" | ||
| or parsed.fragment != "" | ||
| ) | ||
| if invalid: | ||
| raise ValueError( | ||
| "MCP_ALLOWED_ORIGINS entries must be exact HTTP(S) origins " | ||
| "without wildcard, credentials, path, query, or fragment" | ||
| ) | ||
|
|
||
|
|
||
| def _chat_client(settings: Settings) -> PostChatClient: | ||
| """Build the existing contextual-orchestrator chat channel or its null client.""" | ||
| if not (settings.orchestrator_base_url and settings.orchestrator_api_key): | ||
|
|
@@ -267,14 +293,39 @@ def build_mcp_http_app( | |
| server: MCPServer[McpAppContext], | ||
| settings: Settings, | ||
| ) -> ASGIApp: | ||
| """Build the Streamable HTTP app with transport checks outside OAuth.""" | ||
| """Build exact-origin, byte-bounded Streamable HTTP outside OAuth.""" | ||
| _validate_mcp_allowed_origins(settings.mcp_allowed_origins) | ||
| transport_security = TransportSecuritySettings( | ||
| enable_dns_rebinding_protection=True, | ||
| allowed_hosts=settings.mcp_allowed_hosts, | ||
| allowed_origins=settings.mcp_allowed_origins, | ||
| ) | ||
| sdk_app = server.streamable_http_app(transport_security=transport_security) | ||
| return PreAuthTransportSecurityApp(sdk_app, transport_security) | ||
| cors_app = CORSMiddleware( | ||
| sdk_app, | ||
| allow_origins=settings.mcp_allowed_origins, | ||
| allow_methods=["GET", "POST", "DELETE"], | ||
| allow_headers=[ | ||
| "Accept", | ||
| "Authorization", | ||
| "Content-Type", | ||
| "Last-Event-ID", | ||
| "MCP-Protocol-Version", | ||
| "Mcp-Session-Id", | ||
| ], | ||
| expose_headers=[ | ||
| "MCP-Protocol-Version", | ||
| "Mcp-Session-Id", | ||
| "WWW-Authenticate", | ||
| ], | ||
| allow_credentials=False, | ||
| max_age=600, | ||
| ) | ||
| bounded_app = BoundedRequestBodyApp( | ||
|
Comment on lines
+320
to
+324
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. 🔍 Body-admission errors bypass CORS, so browsers can't read 400/413 error bodies The middleware layering is Was this helpful? React with 👍 or 👎 to provide feedback. |
||
| cors_app, | ||
| maximum_bytes=settings.mcp_max_request_bytes, | ||
| ) | ||
| return PreAuthTransportSecurityApp(bounded_app, transport_security) | ||
|
|
||
|
|
||
| _settings = load_settings() | ||
|
|
||
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.
📝 Info: Body is fully buffered even when downstream OAuth will reject it
For every POST,
BoundedRequestBodyApp.__call__reads the entire request body into a bytearray (mcp_admission.py) before delegating to CORS/OAuth. An unauthenticated POST (no/invalid bearer) still gets its full body (up tomcp_max_request_bytes, max 1 MiB) buffered before the 401 is returned. This is bounded and intentional per ADR 0119 (byte admission precedes OAuth), so it is not a resource bug, but it does mean unauthenticated callers can cause up to the configured limit of memory buffering per request before authentication.Was this helpful? React with 👍 or 👎 to provide feedback.