diff --git a/src/aegis_gitea_mcp/oauth/callback.py b/src/aegis_gitea_mcp/oauth/callback.py index b5f17d0..4def927 100644 --- a/src/aegis_gitea_mcp/oauth/callback.py +++ b/src/aegis_gitea_mcp/oauth/callback.py @@ -16,14 +16,21 @@ import html from typing import Any from urllib.parse import urlsplit +from mcp.server.transport_security import RequestBodyLimitMiddleware +from starlette.datastructures import Headers from starlette.requests import Request from starlette.responses import HTMLResponse, RedirectResponse, Response -from starlette.routing import Route +from starlette.routing import Route, request_response +from starlette.types import ASGIApp, Receive, Scope, Send from aegis_gitea_mcp.oauth.provider import CONSENT_PATH, GiteaOAuthProvider from aegis_gitea_mcp.oauth.upstream import CALLBACK_PATH COOKIE_TTL_SECONDS = 600 +#: Largest request body the consent form may send (three short fields). +MAX_CONSENT_BODY_BYTES = 16 * 1024 +_MAX_FORM_FIELDS = 8 +_MAX_FORM_PART_BYTES = 4096 _INVALID_MESSAGE = ( "This sign-in request is invalid or has expired. Start the sign-in again from your MCP client." ) @@ -100,6 +107,28 @@ def _text(value: Any, limit: int = 200) -> str: return value[:limit] if isinstance(value, str) else "" +class _UrlencodedFormOnly: + """ASGI guard for the consent POST, applied before any body is read. + + The route is reachable without authentication, so it must not parse + arbitrary bodies: anything but ``application/x-www-form-urlencoded`` gets + 415 without reading a byte, and the body is capped at + :data:`MAX_CONSENT_BODY_BYTES` (413 beyond it). + """ + + def __init__(self, app: ASGIApp) -> None: + self._limited = RequestBodyLimitMiddleware(app, MAX_CONSENT_BODY_BYTES) + + async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: + if scope["type"] == "http": + content_type = Headers(scope=scope).get("content-type", "") + if content_type.split(";")[0].strip().lower() != "application/x-www-form-urlencoded": + response = Response("Unsupported media type", status_code=415) + await response(scope, receive, send) + return + await self._limited(scope, receive, send) + + def build_browser_routes(provider: GiteaOAuthProvider) -> list[Route]: """Return the consent and callback routes for ``provider``.""" cookies = _Cookies(provider.base_url) @@ -138,7 +167,12 @@ def build_browser_routes(provider: GiteaOAuthProvider) -> list[Route]: origin = request.headers.get("origin") if origin is not None and origin != expected_origin: return _invalid() - form = await request.form() + try: + form = await request.form( + max_files=0, max_fields=_MAX_FORM_FIELDS, max_part_size=_MAX_FORM_PART_BYTES + ) + except Exception: + return _invalid() decision = provider.finish_consent( _text(form.get("request")), _text(form.get("csrf")), @@ -168,6 +202,12 @@ def build_browser_routes(provider: GiteaOAuthProvider) -> list[Route]: return [ Route(CONSENT_PATH, consent_page, methods=["GET"]), - Route(CONSENT_PATH, consent_submit, methods=["POST"]), - Route(CALLBACK_PATH, callback, methods=["GET"]), + Route( + CONSENT_PATH, _UrlencodedFormOnly(request_response(consent_submit)), methods=["POST"] + ), + Route( + CALLBACK_PATH, + RequestBodyLimitMiddleware(request_response(callback), MAX_CONSENT_BODY_BYTES), + methods=["GET"], + ), ] diff --git a/tests/security/__init__.py b/tests/security/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/tests/security/helpers.py b/tests/security/helpers.py new file mode 100644 index 0000000..4d9618e --- /dev/null +++ b/tests/security/helpers.py @@ -0,0 +1,77 @@ +"""Helpers shared by the security tests.""" + +from __future__ import annotations + +import json +from collections.abc import Iterable +from dataclasses import dataclass, field +from typing import Any + +from starlette.types import ASGIApp, Message, Scope + + +@dataclass +class AsgiResult: + """What one raw ASGI call produced and how much of the request body it consumed.""" + + status: int = 0 + body: bytes = b"" + headers: dict[str, str] = field(default_factory=dict) + bytes_read: int = 0 + receive_calls: int = 0 + + def json(self) -> Any: + return json.loads(self.body) + + +async def asgi_request( + app: ASGIApp, + method: str, + path: str, + *, + headers: dict[str, str] | None = None, + chunks: Iterable[bytes] = (), + host: str = "mcp.example.com", + scheme: str = "https", + client: tuple[str, int] = ("198.51.100.7", 4000), +) -> AsgiResult: + """Drive ``app`` with a counting ``receive`` so tests can assert how much body was read.""" + result = AsgiResult() + pending = list(chunks) + raw_headers = [(b"host", host.encode())] + for name, value in (headers or {}).items(): + raw_headers.append((name.lower().encode(), value.encode())) + scope: Scope = { + "type": "http", + "asgi": {"version": "3.0"}, + "http_version": "1.1", + "method": method, + "scheme": scheme, + "path": path, + "raw_path": path.encode(), + "query_string": b"", + "root_path": "", + "headers": raw_headers, + "client": client, + "server": (host, 443), + } + + async def receive() -> Message: + result.receive_calls += 1 + if not pending: + return {"type": "http.disconnect"} + chunk = pending.pop(0) + result.bytes_read += len(chunk) + return {"type": "http.request", "body": chunk, "more_body": bool(pending)} + + async def send(message: Message) -> None: + if message["type"] == "http.response.start": + result.status = message["status"] + result.headers = { + key.decode().lower(): value.decode() for key, value in message["headers"] + } + elif message["type"] == "http.response.body": + result.body += message.get("body", b"") + + await app(scope, receive, send) + return result diff --git a/tests/security/test_oauth_input_bounds.py b/tests/security/test_oauth_input_bounds.py new file mode 100644 index 0000000..b13d776 --- /dev/null +++ b/tests/security/test_oauth_input_bounds.py @@ -0,0 +1,89 @@ +"""Input bounds of the unauthenticated OAuth routes (SEC-01, SEC-09).""" + +from __future__ import annotations + +import pytest + +from aegis_gitea_mcp.oauth.callback import MAX_CONSENT_BODY_BYTES +from tests.security.helpers import asgi_request +from tests.test_oauth_helpers import BASE, build_oauth_app + +CHUNK = 64 * 1024 + + +@pytest.fixture(autouse=True) +def _env(mock_env: None) -> None: + """The audit logger and the rate limiter read the process-wide settings.""" + + +def chunks_of(total: int) -> list[bytes]: + return [b"a" * CHUNK for _ in range(total // CHUNK)] + + +async def test_consent_post_over_the_cap_is_413_without_reading_the_body() -> None: + app = build_oauth_app().app + result = await asgi_request( + app, + "POST", + "/oauth/consent", + headers={ + "content-type": "application/x-www-form-urlencoded", + "content-length": str(1024 * 1024), + "origin": BASE, + }, + chunks=chunks_of(1024 * 1024), + ) + assert result.status == 413 + assert result.bytes_read == 0 + + +async def test_consent_post_without_declared_length_stops_reading_at_the_cap() -> None: + app = build_oauth_app().app + result = await asgi_request( + app, + "POST", + "/oauth/consent", + headers={"content-type": "application/x-www-form-urlencoded", "origin": BASE}, + chunks=chunks_of(1024 * 1024), + ) + assert result.status == 413 + assert result.bytes_read <= MAX_CONSENT_BODY_BYTES + CHUNK + + +async def test_consent_multipart_is_415_without_reading_any_body() -> None: + app = build_oauth_app().app + result = await asgi_request( + app, + "POST", + "/oauth/consent", + headers={"content-type": "multipart/form-data; boundary=x", "origin": BASE}, + chunks=chunks_of(512 * 1024), + ) + assert result.status == 415 + assert result.bytes_read == 0 + + +async def test_consent_json_body_is_415() -> None: + app = build_oauth_app().app + result = await asgi_request( + app, + "POST", + "/oauth/consent", + headers={"content-type": "application/json", "origin": BASE}, + chunks=[b"{}"], + ) + assert result.status == 415 + + +async def test_small_urlencoded_consent_post_still_reaches_the_handler() -> None: + app = build_oauth_app().app + result = await asgi_request( + app, + "POST", + "/oauth/consent", + headers={"content-type": "application/x-www-form-urlencoded", "origin": BASE}, + chunks=[b"request=unknown&csrf=x&decision=approve"], + ) + # Reaches the handler, which answers its fixed 400 page for an unknown request. + assert result.status == 400 + assert b"invalid or has expired" in result.body