From b696541918a3621ff3afbdffee17e5a94d0fd0c5 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sun, 3 May 2026 00:57:12 +0200 Subject: [PATCH] fix(auth): address PR #758 round-4 review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seven findings from the latest review on #758 (3 medium, 4 low/nit): Medium: - storage.py: replace 5 ``assert self.cipher is not None`` sites with explicit ``RuntimeError`` so missing TOKEN_ENCRYPTION_KEY can't silently become an AttributeError under ``python -O`` - session_backend.py: document the silent-invalidation invariant — refresh-token TTL expiry without explicit logout deliberately makes the browser session unusable; future readers must not relax it - server/oauth_tools.py: drop user_id from the Flow 2 session_id identifier — use ``flow2_{secrets.token_hex(16)}`` so audit logs and DB rows don't carry user_id in the session_id field Low / nit: - token_utils.py: drop _fetch_locks dict entry in finally so a probed deployment can't grow the lock dict without bound; coalescing test now pins the invariant with len(_fetch_locks) == 0 - browser_oauth_routes.py: strip trailing slash from settings.nextcloud_host before constructing the well-known URL so a host configured as ``https://cloud.example.com/`` doesn't produce a double-slash - browser_oauth_routes.py: add comment explaining the three-layer CSRF policy on the mcp_session cookie set (SameSite=Lax + POST-only logout + Origin/Referer check) - oauth_routes.py: convert all 23 f-string log calls to lazy %-style per the CLAUDE.md / memory feedback_lazy_logging convention Co-Authored-By: Claude Opus 4.7 (1M context) --- .../auth/browser_oauth_routes.py | 12 ++++- nextcloud_mcp_server/auth/oauth_routes.py | 46 ++++++++--------- nextcloud_mcp_server/auth/session_backend.py | 12 +++++ nextcloud_mcp_server/auth/storage.py | 50 +++++++++++++++---- nextcloud_mcp_server/auth/token_utils.py | 39 +++++++++------ nextcloud_mcp_server/server/oauth_tools.py | 6 ++- tests/unit/test_id_token_verification.py | 7 +++ 7 files changed, 122 insertions(+), 50 deletions(-) diff --git a/nextcloud_mcp_server/auth/browser_oauth_routes.py b/nextcloud_mcp_server/auth/browser_oauth_routes.py index 340b542c..3da0864a 100644 --- a/nextcloud_mcp_server/auth/browser_oauth_routes.py +++ b/nextcloud_mcp_server/auth/browser_oauth_routes.py @@ -614,6 +614,12 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo ) response = RedirectResponse(next_url, status_code=302) + # CSRF protection is layered: ``SameSite=Lax`` blocks cross-site POSTs + # in modern browsers; ``oauth_logout`` is POST-only with an Origin / + # Referer check (``_origin_matches_self``) to cover older browsers and + # non-browser clients. ``HttpOnly`` blocks JS exfiltration on XSS; + # ``Secure`` is gated to non-HTTP hosts in dev (PR #758 round-4 review + # nit 6). response.set_cookie( key="mcp_session", value=session_id, @@ -705,8 +711,12 @@ async def _revoke_refresh_token_at_idp(oauth_ctx: dict, refresh_token: str) -> N try: discovery_url = cfg.get("discovery_url") or settings.oidc_discovery_url if not discovery_url and settings.nextcloud_host: + # Strip trailing slash so a host configured as + # ``https://cloud.example.com/`` doesn't produce a double-slash + # in the well-known URL (PR #758 round-4 review nit 5). discovery_url = ( - f"{settings.nextcloud_host}/.well-known/openid-configuration" + f"{settings.nextcloud_host.rstrip('/')}" + "/.well-known/openid-configuration" ) if not discovery_url: return diff --git a/nextcloud_mcp_server/auth/oauth_routes.py b/nextcloud_mcp_server/auth/oauth_routes.py index 85b1fc5a..0ee6a697 100644 --- a/nextcloud_mcp_server/auth/oauth_routes.py +++ b/nextcloud_mcp_server/auth/oauth_routes.py @@ -267,7 +267,7 @@ async def oauth_authorize(request: Request) -> RedirectResponse | JSONResponse: ) if not is_valid: - logger.warning(f"Client validation failed: {error_msg}") + logger.warning("Client validation failed: %s", error_msg) return JSONResponse( { "error": "unauthorized_client", @@ -326,10 +326,10 @@ async def oauth_authorize(request: Request) -> RedirectResponse | JSONResponse: callback_uri = f"{mcp_server_url}/oauth/callback" logger.info("AS Proxy: Intermediary authorization flow") - logger.info(f" Client: {client_id}") - logger.info(f" MCP server client_id: {mcp_server_client_id}") - logger.info(f" Server callback: {callback_uri}") - logger.info(f" Scopes: {scopes}") + logger.info(" Client: %s", client_id) + logger.info(" MCP server client_id: %s", mcp_server_client_id) + logger.info(" Server callback: %s", callback_uri) + logger.info(" Scopes: %s", scopes) # Discover Nextcloud authorization endpoint discovery_url = oauth_config.get("discovery_url") @@ -369,7 +369,7 @@ async def oauth_authorize(request: Request) -> RedirectResponse | JSONResponse: ) idp_scope_str = _transform_scopes_for_idp(scopes, resource_server_id) if resource_server_id: - logger.info(f" IdP scopes (prefixed): {idp_scope_str}") + logger.info(" IdP scopes (prefixed): %s", idp_scope_str) # Redirect to Nextcloud with MCP server's own client_id (no PKCE — confidential client) idp_params = { @@ -384,7 +384,7 @@ async def oauth_authorize(request: Request) -> RedirectResponse | JSONResponse: } auth_url = f"{authorization_endpoint}?{urlencode(idp_params)}" - logger.info(f"Redirecting to Nextcloud OIDC: {auth_url.split('?')[0]}") + logger.info("Redirecting to Nextcloud OIDC: %s", auth_url.split("?")[0]) return RedirectResponse(auth_url, status_code=302) @@ -553,7 +553,7 @@ async def oauth_callback_nextcloud(request: Request): error_description = request.query_params.get( "error_description", "Authorization failed" ) - logger.error(f"Flow 2 authorization error: {error} - {error_description}") + logger.error("Flow 2 authorization error: %s - %s", error, error_description) return JSONResponse( { "error": error, @@ -685,16 +685,16 @@ async def oauth_callback_nextcloud(request: Request): refresh_expires_at = None if refresh_expires_in: refresh_expires_at = int(time.time()) + refresh_expires_in - logger.info(f" refresh_expires_in: {refresh_expires_in}s") - logger.info(f" refresh_expires_at: {refresh_expires_at}") + logger.info(" refresh_expires_in: %ss", refresh_expires_in) + logger.info(" refresh_expires_at: %s", refresh_expires_at) logger.info("Storing refresh token:") - logger.info(f" user_id: {user_id}") + logger.info(" user_id: %s", user_id) logger.info(" flow_type: flow2") logger.info(" token_audience: nextcloud") - logger.info(f" provisioning_client_id: {state[:16]}...") - logger.info(f" scopes: {granted_scopes}") - logger.info(f" expires_at: {refresh_expires_at}") + logger.info(" provisioning_client_id: %s...", state[:16]) + logger.info(" scopes: %s", granted_scopes) + logger.info(" expires_at: %s", refresh_expires_at) await storage.store_refresh_token( user_id=user_id, @@ -705,7 +705,7 @@ async def oauth_callback_nextcloud(request: Request): scopes=granted_scopes, expires_at=refresh_expires_at, ) - logger.info(f"✓ Stored Flow 2 master refresh token for user {user_id}") + logger.info("✓ Stored Flow 2 master refresh token for user %s", user_id) logger.info("=" * 60) # Return success HTML page @@ -787,7 +787,7 @@ async def oauth_callback(request: Request): oauth_session.get("flow_type", "browser") if oauth_session else "browser" ) - logger.info(f"Unified callback: flow_type={flow_type} (from session lookup)") + logger.info("Unified callback: flow_type=%s (from session lookup)", flow_type) if flow_type == "flow2": # Flow 2: Resource Provisioning - MCP server gets delegated Nextcloud access @@ -801,7 +801,7 @@ async def oauth_callback(request: Request): else: # Unknown flow type - logger.warning(f"Unknown flow_type in OAuth session: {flow_type}") + logger.warning("Unknown flow_type in OAuth session: %s", flow_type) return JSONResponse( { "error": "invalid_request", @@ -831,7 +831,7 @@ async def _oauth_callback_as_proxy( error_description = request.query_params.get( "error_description", "Authorization failed" ) - logger.error(f"AS proxy callback error: {error} - {error_description}") + logger.error("AS proxy callback error: %s - %s", error, error_description) # Retrieve session to redirect back to client with error session = _as_proxy_sessions.pop(server_state, None) @@ -1186,7 +1186,7 @@ async def _token_authorization_code(request: Request, form) -> JSONResponse: ) if not _verify_pkce_s256(code_verifier, entry.code_challenge): - logger.warning(f"PKCE verification failed for client {entry.client_id}") + logger.warning("PKCE verification failed for client %s", entry.client_id) return JSONResponse( { "error": "invalid_grant", @@ -1196,7 +1196,7 @@ async def _token_authorization_code(request: Request, form) -> JSONResponse: ) logger.info( - f"AS proxy token: Returning Nextcloud token for client {entry.client_id}" + "AS proxy token: Returning Nextcloud token for client %s", entry.client_id ) # Return the stored Nextcloud token response directly @@ -1329,7 +1329,7 @@ async def oauth_register_proxy(request: Request) -> JSONResponse: # Remove timestamps outside the window timestamps = [t for t in timestamps if now - t < _DCR_RATE_LIMIT_WINDOW] if len(timestamps) >= _DCR_RATE_LIMIT_MAX: - logger.warning(f"DCR rate limit exceeded for {client_ip}") + logger.warning("DCR rate limit exceeded for %s", client_ip) return JSONResponse( { "error": "too_many_requests", @@ -1365,7 +1365,7 @@ async def oauth_register_proxy(request: Request) -> JSONResponse: status_code=400, ) - logger.info(f"DCR proxy: Forwarding registration to {registration_endpoint}") + logger.info("DCR proxy: Forwarding registration to %s", registration_endpoint) async with nextcloud_httpx_client() as http_client: response = await http_client.post( @@ -1401,7 +1401,7 @@ async def oauth_register_proxy(request: Request) -> JSONResponse: redirect_uris=redirect_uris, name=client_name, ) - logger.info(f"DCR proxy: Registered client {new_client_id} in local registry") + logger.info("DCR proxy: Registered client %s in local registry", new_client_id) return JSONResponse(nc_response, status_code=response.status_code) diff --git a/nextcloud_mcp_server/auth/session_backend.py b/nextcloud_mcp_server/auth/session_backend.py index 69371c02..70c45146 100644 --- a/nextcloud_mcp_server/auth/session_backend.py +++ b/nextcloud_mcp_server/auth/session_backend.py @@ -22,6 +22,18 @@ class SessionAuthBackend(AuthenticationBackend): For BasicAuth mode: Always authenticates as the configured user. For OAuth mode: Checks for valid session cookie with stored refresh token. + + Behavior note — silent invalidation on refresh-token TTL expiry: + The OAuth path requires *both* a live ``browser_sessions`` row and a + live ``refresh_tokens`` row for the resolved user. Logout deletes + both atomically, so a logged-out user always fails closed here. + However, if the refresh token expires by TTL (without an explicit + logout) the row is removed by ``get_refresh_token`` and the browser + session becomes unusable — the user simply gets redirected to + ``/oauth/login``. This is intentional defense-in-depth: the + refresh-token check is what makes a leaked or stale browser cookie + unusable after revocation. Do not relax this without first removing + the cleanup invariant on logout (PR #758 round-4 review medium 2). """ def __init__(self, oauth_enabled: bool = False): diff --git a/nextcloud_mcp_server/auth/storage.py b/nextcloud_mcp_server/auth/storage.py index f97e6cff..f26cb068 100644 --- a/nextcloud_mcp_server/auth/storage.py +++ b/nextcloud_mcp_server/auth/storage.py @@ -227,8 +227,14 @@ class RefreshTokenStorage: if not self._initialized: await self.initialize() - # Type narrowing: cipher is set after initialize() - assert self.cipher is not None + # ``assert`` is stripped under ``python -O``, which would silently + # turn a missing TOKEN_ENCRYPTION_KEY into an ``AttributeError`` on + # the next ``self.cipher.encrypt(...)``. Raise explicitly instead + # (PR #758 round-4 review medium 1). + if self.cipher is None: + raise RuntimeError( + "TOKEN_ENCRYPTION_KEY is not set — token storage operations unavailable" + ) encrypted_token = self.cipher.encrypt(refresh_token.encode()) now = int(time.time()) scopes_json = json.dumps(scopes) if scopes else None @@ -374,8 +380,14 @@ class RefreshTokenStorage: if not self._initialized: await self.initialize() - # Type narrowing: cipher is set after initialize() - assert self.cipher is not None + # ``assert`` is stripped under ``python -O``, which would silently + # turn a missing TOKEN_ENCRYPTION_KEY into an ``AttributeError`` on + # the next ``self.cipher.encrypt(...)``. Raise explicitly instead + # (PR #758 round-4 review medium 1). + if self.cipher is None: + raise RuntimeError( + "TOKEN_ENCRYPTION_KEY is not set — token storage operations unavailable" + ) start_time = time.time() try: @@ -461,8 +473,14 @@ class RefreshTokenStorage: if not self._initialized: await self.initialize() - # Type narrowing: cipher is set after initialize() - assert self.cipher is not None + # ``assert`` is stripped under ``python -O``, which would silently + # turn a missing TOKEN_ENCRYPTION_KEY into an ``AttributeError`` on + # the next ``self.cipher.encrypt(...)``. Raise explicitly instead + # (PR #758 round-4 review medium 1). + if self.cipher is None: + raise RuntimeError( + "TOKEN_ENCRYPTION_KEY is not set — token storage operations unavailable" + ) async with aiosqlite.connect(self.db_path) as db: async with db.execute( @@ -635,8 +653,14 @@ class RefreshTokenStorage: if not self._initialized: await self.initialize() - # Type narrowing: cipher is set after initialize() - assert self.cipher is not None + # ``assert`` is stripped under ``python -O``, which would silently + # turn a missing TOKEN_ENCRYPTION_KEY into an ``AttributeError`` on + # the next ``self.cipher.encrypt(...)``. Raise explicitly instead + # (PR #758 round-4 review medium 1). + if self.cipher is None: + raise RuntimeError( + "TOKEN_ENCRYPTION_KEY is not set — token storage operations unavailable" + ) # Encrypt sensitive data encrypted_secret = self.cipher.encrypt(client_secret.encode()) @@ -708,8 +732,14 @@ class RefreshTokenStorage: if not self._initialized: await self.initialize() - # Type narrowing: cipher is set after initialize() - assert self.cipher is not None + # ``assert`` is stripped under ``python -O``, which would silently + # turn a missing TOKEN_ENCRYPTION_KEY into an ``AttributeError`` on + # the next ``self.cipher.encrypt(...)``. Raise explicitly instead + # (PR #758 round-4 review medium 1). + if self.cipher is None: + raise RuntimeError( + "TOKEN_ENCRYPTION_KEY is not set — token storage operations unavailable" + ) async with aiosqlite.connect(self.db_path) as db: async with db.execute( diff --git a/nextcloud_mcp_server/auth/token_utils.py b/nextcloud_mcp_server/auth/token_utils.py index d2bdf944..db1a492a 100644 --- a/nextcloud_mcp_server/auth/token_utils.py +++ b/nextcloud_mcp_server/auth/token_utils.py @@ -75,20 +75,31 @@ async def _get_cached( 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 + try: + 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 + finally: + # Drop the dict entry so a misconfigured deployment hitting + # arbitrary URLs can't grow ``_fetch_locks`` without bound (PR #758 + # round-4 review nit 4). Already-queued waiters share our local + # ``lock`` reference and remain coalesced; new arrivals lazily + # recreate a lock — by which time the cache is populated, so they + # short-circuit before reaching the lock anyway. + async with _fetch_locks_lock: + if _fetch_locks.get(url) is lock: + del _fetch_locks[url] async def get_oidc_discovery(discovery_url: str) -> dict[str, Any]: diff --git a/nextcloud_mcp_server/server/oauth_tools.py b/nextcloud_mcp_server/server/oauth_tools.py index f5057736..cb143b94 100644 --- a/nextcloud_mcp_server/server/oauth_tools.py +++ b/nextcloud_mcp_server/server/oauth_tools.py @@ -432,8 +432,10 @@ async def _check_logged_in(ctx: Context, user_id: str) -> str: # Store state in session for validation on callback storage = await get_shared_storage() - # Create OAuth session for Flow 2 - session_id = f"flow2_{user_id}_{secrets.token_hex(8)}" + # Create OAuth session for Flow 2. Identifier is purely random + # so audit-log entries / DB rows don't carry the user_id in the + # session_id field (PR #758 round-4 review medium 3). + session_id = f"flow2_{secrets.token_hex(16)}" redirect_uri = f"{os.getenv('NEXTCLOUD_MCP_SERVER_URL', 'http://localhost:8000')}/oauth/callback" await storage.store_oauth_session( diff --git a/tests/unit/test_id_token_verification.py b/tests/unit/test_id_token_verification.py index f71c6de8..c17159b8 100644 --- a/tests/unit/test_id_token_verification.py +++ b/tests/unit/test_id_token_verification.py @@ -528,3 +528,10 @@ async def test_get_cached_coalesces_concurrent_misses(): assert all(r == results[0] for r in results), ( "concurrent callers received divergent cached data" ) + # Pin the round-4 cleanup invariant: _fetch_locks must drain after the + # fetch completes so a probed deployment can't accumulate locks for + # arbitrary URLs. + assert len(token_utils._fetch_locks) == 0, ( + "expected _fetch_locks to be empty after fetch, " + f"found {list(token_utils._fetch_locks)}" + )