diff --git a/nextcloud_mcp_server/auth/browser_oauth_routes.py b/nextcloud_mcp_server/auth/browser_oauth_routes.py index 18dc12b5..340b542c 100644 --- a/nextcloud_mcp_server/auth/browser_oauth_routes.py +++ b/nextcloud_mcp_server/auth/browser_oauth_routes.py @@ -54,7 +54,8 @@ def _origin_matches_self(request: Request, oauth_ctx: dict) -> bool: """Return True when Origin/Referer is missing or matches our own host. Used to gate POST /oauth/logout against cross-origin form submissions - (PR #758 finding 5). Per OWASP CSRF cheat sheet, the policy is: + (PR #758 round-3 review hardening). Per OWASP CSRF cheat sheet, the + policy is: - If neither Origin nor Referer is set, allow (same-origin POST in privacy-conscious browsers may strip both). - Otherwise, the (scheme, hostname, port) tuple of the first present @@ -640,9 +641,9 @@ async def oauth_logout(request: Request) -> RedirectResponse | JSONResponse: 5. Clears the cookie on the response. Method is POST-only at the route layer to defeat passive CSRF (PR #758 - finding 5). Origin / Referer headers are also validated against the - configured ``mcp_server_url`` when present, blocking same-method-but- - cross-origin form submissions. + round-3 review hardening). Origin / Referer headers are also validated + against the configured ``mcp_server_url`` when present, blocking + same-method-but-cross-origin form submissions. Query parameters: next: Optional URL to redirect to after logout (default: /oauth/login) diff --git a/nextcloud_mcp_server/auth/oauth_routes.py b/nextcloud_mcp_server/auth/oauth_routes.py index 72a62cc5..85b1fc5a 100644 --- a/nextcloud_mcp_server/auth/oauth_routes.py +++ b/nextcloud_mcp_server/auth/oauth_routes.py @@ -92,7 +92,7 @@ class ASProxySession: code_challenge: str code_challenge_method: str requested_scopes: str - nonce: str = "" + nonce: str created_at: float = field(default_factory=time.time) expires_at: float = field(default_factory=lambda: time.time() + 600) @@ -959,9 +959,10 @@ async def _oauth_callback_as_proxy( # verification done in oauth_callback_nextcloud. # # ``expected_nonce`` is the per-request nonce we forwarded to the IdP - # in oauth_authorize (PR #758 round-2 finding 2); falsy → skip nonce - # check for backward compatibility with sessions stored before the - # nonce field was added. + # in oauth_authorize (PR #758 round-2 finding 2). ASProxySession is + # in-memory only and ``nonce`` is now a required field, so for any + # session created via the current code path this is always set; the + # ``or None`` is defence-in-depth and a no-op in practice. id_token = nc_token_response.get("id_token") try: await verify_id_token( diff --git a/nextcloud_mcp_server/auth/storage.py b/nextcloud_mcp_server/auth/storage.py index 517538a3..f97e6cff 100644 --- a/nextcloud_mcp_server/auth/storage.py +++ b/nextcloud_mcp_server/auth/storage.py @@ -1220,23 +1220,22 @@ class RefreshTokenStorage: if not self._initialized: await self.initialize() - # SELECT the row before DELETE so we can attribute the audit log - # entry to the right user (PR #758 round-3 nit 5). + # DELETE ... RETURNING (SQLite ≥ 3.35) reads ``user_id`` atomically + # with the delete itself, so the audit log can't race against a + # concurrent delete that empties the row between SELECT and DELETE + # (PR #758 round-3 review). + user_id: str | None = None async with aiosqlite.connect(self.db_path) as db: async with db.execute( - "SELECT user_id FROM browser_sessions WHERE session_id = ?", + "DELETE FROM browser_sessions WHERE session_id = ? RETURNING user_id", (session_id,), ) as cursor: row = await cursor.fetchone() - user_id = row[0] if row else None - - cursor = await db.execute( - "DELETE FROM browser_sessions WHERE session_id = ?", (session_id,) - ) await db.commit() - deleted = cursor.rowcount > 0 + deleted = row is not None if deleted: + user_id = row[0] logger.debug("Deleted browser session %s", session_id[:8]) if user_id: await self._audit_log( diff --git a/nextcloud_mcp_server/auth/token_utils.py b/nextcloud_mcp_server/auth/token_utils.py index 95364ab0..d2bdf944 100644 --- a/nextcloud_mcp_server/auth/token_utils.py +++ b/nextcloud_mcp_server/auth/token_utils.py @@ -8,6 +8,7 @@ import logging import time from typing import Any +import anyio import jwt from jwt import PyJWKSet from mcp.server.auth.middleware.auth_context import get_access_token @@ -30,6 +31,23 @@ _discovery_cache: dict[str, tuple[float, dict[str, Any]]] = {} _jwks_cache: dict[str, tuple[float, dict[str, Any]]] = {} _OIDC_CACHE_TTL = 300 +# Per-URL fetch locks coalesce concurrent cache misses into a single HTTP +# request, preventing thundering-herd against the IdP at cache expiry +# (PR #758 round-3 review). Mirrors the lock-dict + meta-lock idiom from +# token_broker.py. +_fetch_locks: dict[str, anyio.Lock] = {} +_fetch_locks_lock = anyio.Lock() + + +async def _get_fetch_lock(url: str) -> anyio.Lock: + """Return the per-URL lock used to serialise cache-miss fetches.""" + async with _fetch_locks_lock: + lock = _fetch_locks.get(url) + if lock is None: + lock = anyio.Lock() + _fetch_locks[url] = lock + return lock + class IdTokenVerificationError(Exception): """Raised when an OIDC ID token fails signature or claim verification.""" @@ -48,19 +66,29 @@ async def _get_cached( ``/.well-known/openid-configuration`` path issues a 301), but JWKS fetches deliberately stay strict — the URL came from the discovery document we already trust, so a redirect there would be suspicious. + + Concurrent callers seeing the same cache miss are coalesced via a + per-URL ``anyio.Lock``: only one fetch runs, the rest wait and read the + populated cache. """ - now = time.time() entry = cache.get(url) - if entry is not None: - expires_at, data = entry - if now < expires_at: - return data - async with nextcloud_httpx_client(follow_redirects=follow_redirects) as http_client: - response = await http_client.get(url) - response.raise_for_status() - data = response.json() - cache[url] = (now + _OIDC_CACHE_TTL, data) - return data + if entry is not None and time.time() < entry[0]: + return entry[1] + lock = await _get_fetch_lock(url) + async with lock: + # Re-check inside the lock — a concurrent waiter may have already + # populated the cache before we acquired it. + entry = cache.get(url) + if entry is not None and time.time() < entry[0]: + return entry[1] + async with nextcloud_httpx_client( + follow_redirects=follow_redirects + ) as http_client: + response = await http_client.get(url) + response.raise_for_status() + data = response.json() + cache[url] = (time.time() + _OIDC_CACHE_TTL, data) + return data async def get_oidc_discovery(discovery_url: str) -> dict[str, Any]: @@ -78,7 +106,7 @@ async def get_oidc_discovery(discovery_url: str) -> dict[str, Any]: async def verify_id_token( - id_token: str, + id_token: str | None, *, discovery_url: str, expected_audience: str, @@ -240,7 +268,8 @@ async def extract_user_id_from_token(_ctx: Context) -> str: ) raise McpError( ErrorData( - code=-1, + # JSON-RPC 2.0 reserves -32000..-32099 for application errors. + code=-32001, message="Cannot determine user identity from access token", ) ) diff --git a/tests/unit/test_id_token_verification.py b/tests/unit/test_id_token_verification.py index 669a55d4..f71c6de8 100644 --- a/tests/unit/test_id_token_verification.py +++ b/tests/unit/test_id_token_verification.py @@ -12,6 +12,7 @@ import time from base64 import urlsafe_b64encode from unittest.mock import patch +import anyio import httpx import jwt import pytest @@ -32,9 +33,11 @@ def _clear_oidc_caches(): """Reset the discovery+JWKS caches so tests don't share fetched data.""" token_utils._discovery_cache.clear() token_utils._jwks_cache.clear() + token_utils._fetch_locks.clear() yield token_utils._discovery_cache.clear() token_utils._jwks_cache.clear() + token_utils._fetch_locks.clear() # Generated once per process — RSA keypair generation is slow. @@ -478,3 +481,50 @@ async def test_verify_id_token_caches_discovery_and_jwks(): assert fetches.get(DISCOVERY_URL) == 1, "discovery fetched more than once" assert fetches.get(JWKS_URI) == 1, "JWKS fetched more than once" + + +async def test_get_cached_coalesces_concurrent_misses(): + """Concurrent cache misses must collapse into a single HTTP fetch. + + PR #758 round-3 review: without the per-URL lock in ``_get_cached``, + N simultaneous callers at cache expiry would each fire their own + request to the IdP, potentially tripping rate limits. The async + handler yields with ``anyio.sleep(0.01)`` so all 10 callers reach + the cache-miss branch concurrently — without coalescing the count + would be 10. + """ + fetch_count = {"n": 0} + + async def slow_handler(request: httpx.Request) -> httpx.Response: + fetch_count["n"] += 1 + # Yield so concurrent waiters all reach the lock acquisition + # while the first holder is still mid-fetch. + await anyio.sleep(0.01) + return _idp_handler(request) + + transport = httpx.MockTransport(slow_handler) + + def fake_client(**kwargs): + kwargs["transport"] = transport + return httpx.AsyncClient(**kwargs) + + results: list[dict] = [] + + async def fetch_once(): + results.append(await token_utils._get_cached(token_utils._jwks_cache, JWKS_URI)) + + with patch( + "nextcloud_mcp_server.auth.token_utils.nextcloud_httpx_client", + side_effect=fake_client, + ): + async with anyio.create_task_group() as tg: + for _ in range(10): + tg.start_soon(fetch_once) + + assert fetch_count["n"] == 1, ( + f"expected exactly one fetch via lock coalescing, got {fetch_count['n']}" + ) + assert len(results) == 10 + assert all(r == results[0] for r in results), ( + "concurrent callers received divergent cached data" + ) diff --git a/tests/unit/test_oauth_callback_session_cleanup.py b/tests/unit/test_oauth_callback_session_cleanup.py index 7ad2efbe..8b54fc8c 100644 --- a/tests/unit/test_oauth_callback_session_cleanup.py +++ b/tests/unit/test_oauth_callback_session_cleanup.py @@ -218,6 +218,7 @@ async def test_as_proxy_rejects_invalid_id_token(): code_challenge="challenge", code_challenge_method="S256", requested_scopes="openid", + nonce="nonce-rejected", ) _proxy_codes.clear() diff --git a/tests/unit/test_origin_normalise.py b/tests/unit/test_origin_normalise.py new file mode 100644 index 00000000..e7a5037d --- /dev/null +++ b/tests/unit/test_origin_normalise.py @@ -0,0 +1,41 @@ +"""Tests for ``_normalise_origin`` port + scheme + host normalisation. + +The CSRF guard on POST /oauth/logout (PR #758 round-3 review hardening) +compares ``Origin`` / ``Referer`` against the configured ``mcp_server_url`` +via ``_normalise_origin``. RFC 6454 §6.2 says browsers omit default ports +(80 for http, 443 for https) from Origin headers, so the function strips +those before comparison. These tests pin that behaviour so it can't +silently regress. +""" + +import pytest + +from nextcloud_mcp_server.auth.browser_oauth_routes import _normalise_origin + +pytestmark = pytest.mark.unit + + +@pytest.mark.parametrize( + "left, right, equal", + [ + # Default ports are stripped — these MUST compare equal. + ("https://example.com", "https://example.com:443", True), + ("https://example.com:443", "https://example.com", True), + ("http://example.com", "http://example.com:80", True), + ("http://example.com:80", "http://example.com", True), + # Non-default ports are preserved. + ("https://example.com:8443", "https://example.com", False), + ("http://example.com:8080", "http://example.com", False), + ("https://example.com:8443", "https://example.com:443", False), + # Cross-scheme defaults don't collapse (https:443 != http:80 even + # though both ports get stripped, because the scheme differs). + ("https://example.com", "http://example.com", False), + ("https://example.com:443", "http://example.com:80", False), + # Hostname matters and is case-insensitive. + ("https://example.com", "https://other.com", False), + ("https://example.com", "https://EXAMPLE.COM", True), + ("https://Example.Com:443", "https://example.com", True), + ], +) +def test_normalise_origin_equivalence(left: str, right: str, equal: bool): + assert (_normalise_origin(left) == _normalise_origin(right)) is equal