From 2d340a5a6b24cca780325040e6bfd0ce308641e6 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sat, 2 May 2026 18:59:34 +0200 Subject: [PATCH] fix(auth): address PR #758 follow-up review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six findings from the latest claude-bot review on PR #758: - JWKS cache had no kid-miss refresh path (Medium): on IdP key rotation every login failed for up to _OIDC_CACHE_TTL. Evict and refetch once before raising, per OIDC core §10.1.1. - _should_use_secure_cookies fell back to nextcloud_host scheme, but the cookie is issued by the MCP server. Switch to settings.nextcloud_mcp_server_url so split-scheme deployments get the right Secure flag. - _origin_matches_self compared raw netloc strings, which include the port. Browsers omit default ports per RFC 6454 §6.2; an mcp_server_url like :443 falsely 403'd every legitimate logout. Normalise (scheme, host, port) tuples with default ports stripped. - delete_oauth_session exists in storage.py — drop the stale "we don't have this method" comment and call it eagerly so replays can't be processed and the table doesn't accumulate completed-but-not-yet-expired browser-login rows. - extract_user_id_from_token's unused ctx param renamed to _ctx to signal "intentionally unused" at the signature level. - provisioning_decorator instantiated RefreshTokenStorage per call. Switch to get_shared_storage() for the lock-protected process-wide singleton. Plus pre-push self-review catch: lazy-logging on the unchanged except arm in session_backend.py. Adds 5 regression tests: - JWKS rotation: success on refetch - JWKS rotation: still-missing-kid surfaces original error - JWKS rotation: network error during refresh wrapped as IdTokenVerificationError - default-port CSRF: explicit :443 in config + portless Origin - default-port CSRF: portless config + explicit :443 in Origin - scheme-mismatch CSRF: same host, different scheme rejected Co-Authored-By: Claude Opus 4.7 (1M context) --- .../auth/browser_oauth_routes.py | 48 +++-- .../auth/provisioning_decorator.py | 13 +- nextcloud_mcp_server/auth/session_backend.py | 2 +- nextcloud_mcp_server/auth/token_utils.py | 29 ++- tests/unit/test_id_token_verification.py | 191 ++++++++++++++++++ tests/unit/test_oauth_logout.py | 68 +++++++ 6 files changed, 324 insertions(+), 27 deletions(-) diff --git a/nextcloud_mcp_server/auth/browser_oauth_routes.py b/nextcloud_mcp_server/auth/browser_oauth_routes.py index 76c6949b..3d2c6186 100644 --- a/nextcloud_mcp_server/auth/browser_oauth_routes.py +++ b/nextcloud_mcp_server/auth/browser_oauth_routes.py @@ -32,6 +32,23 @@ from ..http import nextcloud_httpx_client logger = logging.getLogger(__name__) +def _normalise_origin(raw: str) -> tuple[str, str, int | None]: + """Return (scheme, hostname, port) with default HTTP/HTTPS ports stripped. + + Browsers omit default ports in Origin headers (RFC 6454 §6.2), so a + raw netloc string comparison falsely rejects requests whenever + ``mcp_server_url`` is configured with an explicit ``:443`` / ``:80`` + (or vice versa). + """ + parsed = parse_url(raw) + scheme = parsed.scheme.lower() + hostname = (parsed.hostname or "").lower() + port = parsed.port + if (scheme == "https" and port == 443) or (scheme == "http" and port == 80): + port = None + return (scheme, hostname, port) + + def _origin_matches_self(request: Request, oauth_ctx: dict) -> bool: """Return True when Origin/Referer is missing or matches our own host. @@ -39,8 +56,11 @@ def _origin_matches_self(request: Request, oauth_ctx: dict) -> bool: (PR #758 finding 5). 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 netloc of the first present header must equal the - netloc of the configured ``mcp_server_url``. + - Otherwise, the (scheme, hostname, port) tuple of the first present + header must equal the same tuple of the configured + ``mcp_server_url``. Default ports (80/443) are normalised away + before comparison so RFC-6454-compliant browsers — which omit + default ports in Origin — aren't rejected. """ cfg = oauth_ctx.get("config") or oauth_ctx mcp_server_url = cfg.get("mcp_server_url") @@ -48,11 +68,11 @@ def _origin_matches_self(request: Request, oauth_ctx: dict) -> bool: # Mis-configured deployment — fail open rather than break logout. return True - expected = parse_url(mcp_server_url).netloc.lower() + expected = _normalise_origin(mcp_server_url) raw = request.headers.get("origin") or request.headers.get("referer") if not raw: return True - return parse_url(raw).netloc.lower() == expected + return _normalise_origin(raw) == expected def _safe_next_url(raw: str | None, default: str) -> str: @@ -78,19 +98,19 @@ def _should_use_secure_cookies() -> bool: """Determine if cookies should have the Secure flag. Reads ``settings.cookie_secure`` first (set via the ``COOKIE_SECURE`` - env var). Falls back to auto-detect from the ``nextcloud_host`` scheme - when unset. - - Returns: - True if cookies should be secure (HTTPS), False otherwise + env var). Falls back to auto-detecting from the MCP server's own URL + scheme — the cookie is issued by THIS server, so the Secure flag must + reflect THIS server's transport, not Nextcloud's. (Split-scheme + deployments — HTTPS Nextcloud + plain-HTTP MCP sidecar, or vice + versa — would otherwise get the wrong answer.) """ settings = get_settings() if settings.cookie_secure is not None: # Dynaconf auto-coerces "true"/"false" → bool but "1"/"0" → int; # bool() normalises both. return bool(settings.cookie_secure) - nextcloud_host = settings.nextcloud_host or "" - return nextcloud_host.startswith("https://") + mcp_server_url = settings.nextcloud_mcp_server_url or "" + return mcp_server_url.startswith("https://") async def oauth_login(request: Request) -> RedirectResponse | JSONResponse: @@ -327,8 +347,10 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo # read-time as defense-in-depth (issue #758 finding 3). The session # row could have been written by an older code path or reused. next_url = _safe_next_url(oauth_session.get("client_redirect_uri"), "/app") - # Clean up the temporary session - # Note: We don't have delete_oauth_session method, but it will expire after TTL + # One-time-use session: delete eagerly so a replayed callback can't + # be processed and so the oauth_sessions table doesn't accumulate + # completed-but-not-yet-expired browser-login rows. + await storage.delete_oauth_session(state) # Exchange authorization code for tokens mcp_server_url = oauth_config["mcp_server_url"] diff --git a/nextcloud_mcp_server/auth/provisioning_decorator.py b/nextcloud_mcp_server/auth/provisioning_decorator.py index f9534e31..b28eecc3 100644 --- a/nextcloud_mcp_server/auth/provisioning_decorator.py +++ b/nextcloud_mcp_server/auth/provisioning_decorator.py @@ -14,7 +14,7 @@ from mcp.server.fastmcp import Context from mcp.shared.exceptions import McpError from mcp.types import ErrorData -from nextcloud_mcp_server.auth.storage import RefreshTokenStorage +from nextcloud_mcp_server.auth.storage import get_shared_storage logger = logging.getLogger(__name__) @@ -80,9 +80,9 @@ def require_provisioning(func: Callable) -> Callable: ) ) - # Check provisioning status - storage = RefreshTokenStorage.from_env() - await storage.initialize() + # Check provisioning status — share the process-wide singleton + # rather than initialising a new sqlite handle per tool call. + storage = await get_shared_storage() refresh_data = await storage.get_refresh_token(user_id) @@ -149,9 +149,8 @@ def require_provisioning_or_suggest(func: Callable) -> Callable: user_id = access_token.resource if access_token else None if user_id: - # Check provisioning status - storage = RefreshTokenStorage.from_env() - await storage.initialize() + # Check provisioning status using the shared singleton. + storage = await get_shared_storage() refresh_data = await storage.get_refresh_token(user_id) diff --git a/nextcloud_mcp_server/auth/session_backend.py b/nextcloud_mcp_server/auth/session_backend.py index f4654b64..69371c02 100644 --- a/nextcloud_mcp_server/auth/session_backend.py +++ b/nextcloud_mcp_server/auth/session_backend.py @@ -94,5 +94,5 @@ class SessionAuthBackend(AuthenticationBackend): return AuthCredentials(["authenticated"]), SimpleUser(user_id) except Exception as e: - logger.warning(f"Session validation error: {e}") + logger.warning("Session validation error: %s", e) return None diff --git a/nextcloud_mcp_server/auth/token_utils.py b/nextcloud_mcp_server/auth/token_utils.py index a204810e..77bd0b2a 100644 --- a/nextcloud_mcp_server/auth/token_utils.py +++ b/nextcloud_mcp_server/auth/token_utils.py @@ -115,10 +115,24 @@ async def verify_id_token( raise IdTokenVerificationError("ID token header missing 'kid'") try: signing_key = jwks[kid] - except KeyError as e: - raise IdTokenVerificationError( - f"No JWKS key matches ID token kid {kid!r}" - ) from e + except KeyError: + # Cache miss may indicate IdP key rotation. Refresh JWKS once + # before giving up, per OIDC core §10.1.1: when an unrecognised + # `kid` arrives the relying party should refetch the JWKS rather + # than waiting for cache TTL to elapse. + _jwks_cache.pop(jwks_uri, None) + try: + jwks_data = await _get_cached(_jwks_cache, jwks_uri) + jwks = PyJWKSet.from_dict(jwks_data) + signing_key = jwks[kid] + except KeyError as e: + raise IdTokenVerificationError( + f"No JWKS key matches ID token kid {kid!r}" + ) from e + except Exception as e: + raise IdTokenVerificationError( + f"Failed to refresh JWKS after kid miss: {e}" + ) from e # PyJWT verifies the JWT with the algorithm declared in its header, # cross-checked against this allowlist (so an attacker can't downgrade @@ -158,7 +172,7 @@ async def verify_id_token( return payload -async def extract_user_id_from_token(ctx: Context) -> str: +async def extract_user_id_from_token(_ctx: Context) -> str: """Extract user_id from the verified MCP access token. Reads the `sub` claim from `AccessToken.resource`, which is populated by @@ -168,7 +182,10 @@ async def extract_user_id_from_token(ctx: Context) -> str: identity claim. Args: - ctx: MCP context with access token (unused — kept for the public API) + _ctx: MCP context with access token. Intentionally unused — kept on + the public signature so call sites can pass the FastMCP Context + they already hold without rewriting; identity is read from the + verifier-populated AccessToken via get_access_token(). Returns: user_id from the verified token, or "default_user" when no token is diff --git a/tests/unit/test_id_token_verification.py b/tests/unit/test_id_token_verification.py index ee43d027..669a55d4 100644 --- a/tests/unit/test_id_token_verification.py +++ b/tests/unit/test_id_token_verification.py @@ -244,6 +244,197 @@ async def test_verify_id_token_missing_token_rejected(): ) +async def test_verify_id_token_recovers_after_kid_rotation(): + """Unknown kid → JWKS is refetched once and verification succeeds. + + Pins the fix for the PR #758 follow-up review: previously a kid-miss + raised immediately, so every login failed for up to _OIDC_CACHE_TTL + after the IdP rotated its signing key. + """ + rotated_key = rsa.generate_private_key(public_exponent=65537, key_size=2048) + rotated_pem = rotated_key.private_bytes( + encoding=serialization.Encoding.PEM, + format=serialization.PrivateFormat.TraditionalOpenSSL, + encryption_algorithm=serialization.NoEncryption(), + ) + + def _build_rotated_jwks() -> dict: + pub = rotated_key.public_key().public_numbers() + return { + "keys": [ + { + "kty": "RSA", + "use": "sig", + "kid": "rotated-key", + "alg": "RS256", + "n": _b64u_uint(pub.n), + "e": _b64u_uint(pub.e), + } + ] + } + + jwks_fetches = {"count": 0} + + def rotation_handler(request: httpx.Request) -> httpx.Response: + url = str(request.url) + if url == DISCOVERY_URL: + return httpx.Response(200, json={"issuer": ISSUER, "jwks_uri": JWKS_URI}) + if url == JWKS_URI: + jwks_fetches["count"] += 1 + # First fetch: stale JWKS (without rotated kid). + # Subsequent fetches: post-rotation JWKS (with rotated kid). + jwks = ( + _build_jwks() if jwks_fetches["count"] == 1 else _build_rotated_jwks() + ) + return httpx.Response( + 200, + content=json.dumps(jwks).encode(), + headers={"content-type": "application/json"}, + ) + return httpx.Response(404) + + transport = httpx.MockTransport(rotation_handler) + + def fake_client(**kwargs): + kwargs["transport"] = transport + return httpx.AsyncClient(**kwargs) + + now = int(time.time()) + token = jwt.encode( + { + "iss": ISSUER, + "aud": "test-client", + "sub": "alice", + "iat": now, + "exp": now + 60, + }, + rotated_pem, + algorithm="RS256", + headers={"kid": "rotated-key"}, + ) + + with patch( + "nextcloud_mcp_server.auth.token_utils.nextcloud_httpx_client", + side_effect=fake_client, + ): + # Prime the cache with the stale JWKS by triggering a verification + # that misses on the rotated kid. + payload = await verify_id_token( + token, discovery_url=DISCOVERY_URL, expected_audience="test-client" + ) + + assert payload["sub"] == "alice" + assert jwks_fetches["count"] == 2, ( + "JWKS should be refetched once on kid miss " + f"(actual fetches: {jwks_fetches['count']})" + ) + + +async def test_verify_id_token_rotation_retry_still_misses(): + """Refresh that still doesn't include the kid surfaces the original error.""" + fetches = {"count": 0} + + def handler(request: httpx.Request) -> httpx.Response: + url = str(request.url) + if url == DISCOVERY_URL: + return httpx.Response(200, json={"issuer": ISSUER, "jwks_uri": JWKS_URI}) + if url == JWKS_URI: + fetches["count"] += 1 + return httpx.Response( + 200, + content=json.dumps(_build_jwks()).encode(), + headers={"content-type": "application/json"}, + ) + return httpx.Response(404) + + transport = httpx.MockTransport(handler) + + def fake_client(**kwargs): + kwargs["transport"] = transport + return httpx.AsyncClient(**kwargs) + + now = int(time.time()) + token = _sign( + { + "iss": ISSUER, + "aud": "test-client", + "sub": "alice", + "iat": now, + "exp": now + 60, + }, + kid="never-existed", + ) + + with patch( + "nextcloud_mcp_server.auth.token_utils.nextcloud_httpx_client", + side_effect=fake_client, + ): + with pytest.raises(IdTokenVerificationError, match="No JWKS key matches"): + await verify_id_token( + token, discovery_url=DISCOVERY_URL, expected_audience="test-client" + ) + + assert fetches["count"] == 2, "JWKS should be refetched once before raising" + + +async def test_verify_id_token_rotation_retry_network_error_wraps(): + """A 500 on the kid-miss refresh fetch surfaces as IdTokenVerificationError. + + Pins the fail-closed branch in the new refresh block: a network error + during JWKS refetch must not bubble out as a bare exception — it has + to be wrapped in IdTokenVerificationError so the caller's existing + error handling stays correct. + """ + fetches = {"jwks": 0} + + def handler(request: httpx.Request) -> httpx.Response: + url = str(request.url) + if url == DISCOVERY_URL: + return httpx.Response(200, json={"issuer": ISSUER, "jwks_uri": JWKS_URI}) + if url == JWKS_URI: + fetches["jwks"] += 1 + # First fetch: stale-but-valid JWKS. Second (refresh): 500. + if fetches["jwks"] == 1: + return httpx.Response( + 200, + content=json.dumps(_build_jwks()).encode(), + headers={"content-type": "application/json"}, + ) + return httpx.Response(500, content=b"upstream broke") + return httpx.Response(404) + + transport = httpx.MockTransport(handler) + + def fake_client(**kwargs): + kwargs["transport"] = transport + return httpx.AsyncClient(**kwargs) + + now = int(time.time()) + token = _sign( + { + "iss": ISSUER, + "aud": "test-client", + "sub": "alice", + "iat": now, + "exp": now + 60, + }, + kid="not-cached-yet", + ) + + with patch( + "nextcloud_mcp_server.auth.token_utils.nextcloud_httpx_client", + side_effect=fake_client, + ): + with pytest.raises( + IdTokenVerificationError, match="Failed to refresh JWKS after kid miss" + ): + await verify_id_token( + token, discovery_url=DISCOVERY_URL, expected_audience="test-client" + ) + + assert fetches["jwks"] == 2 + + async def test_verify_id_token_caches_discovery_and_jwks(): """Discovery + JWKS must be cached: two verifications, one fetch each. diff --git a/tests/unit/test_oauth_logout.py b/tests/unit/test_oauth_logout.py index d036c491..b8629ace 100644 --- a/tests/unit/test_oauth_logout.py +++ b/tests/unit/test_oauth_logout.py @@ -192,6 +192,74 @@ async def test_logout_allows_same_origin_post(storage): assert await storage.get_browser_session_user("sid-Y") is None +async def test_logout_allows_same_origin_post_with_explicit_default_port(storage): + """mcp_server_url has explicit :443; browser Origin omits the port. + + RFC 6454 §6.2: browsers omit default ports in Origin headers. The + netloc string ``mcp.example.com:443`` would never match ``mcp.example.com`` + without port normalisation, blocking every legitimate logout. + """ + await storage.create_browser_session(session_id="sid-PE", user_id="alice") + + request = _build_request( + cookie="sid-PE", + oauth_context={ + "storage": storage, + "config": { + "mcp_server_url": "https://mcp.example.com:443", + "discovery_url": None, + }, + }, + headers={"origin": "https://mcp.example.com"}, + ) + + response = await oauth_logout(request) + assert response.status_code == 302 + assert await storage.get_browser_session_user("sid-PE") is None + + +async def test_logout_allows_same_origin_post_with_default_port_in_origin(storage): + """Symmetric case: config omits port, Origin includes :443.""" + await storage.create_browser_session(session_id="sid-PI", user_id="alice") + + request = _build_request( + cookie="sid-PI", + oauth_context={ + "storage": storage, + "config": { + "mcp_server_url": "https://mcp.example.com", + "discovery_url": None, + }, + }, + headers={"origin": "https://mcp.example.com:443"}, + ) + + response = await oauth_logout(request) + assert response.status_code == 302 + assert await storage.get_browser_session_user("sid-PI") is None + + +async def test_logout_blocks_scheme_mismatch(storage): + """Same hostname but different scheme must be treated as cross-origin.""" + await storage.create_browser_session(session_id="sid-SC", user_id="alice") + + request = _build_request( + cookie="sid-SC", + oauth_context={ + "storage": storage, + "config": { + "mcp_server_url": "https://mcp.example.com", + "discovery_url": None, + }, + }, + headers={"origin": "http://mcp.example.com"}, + ) + + response = await oauth_logout(request) + assert response.status_code == 403 + assert await storage.get_browser_session_user("sid-SC") == "alice" + + async def test_logout_allows_referer_when_origin_missing(storage): """Some browsers strip Origin on POST; Referer is the fallback signal.""" await storage.create_browser_session(session_id="sid-Z", user_id="alice")