From 15dbb263490b5f71e8837d4e8c81645c388d4d0f Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sat, 2 May 2026 17:03:57 +0200 Subject: [PATCH 01/14] fix(auth): harden OAuth/session for hosted multi-tenant deployment (#626) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pre-launch hardening for the hosted Astrolabe Cloud offering. Addresses all five findings raised in #626 (Tim Kaufmann, code review of v0.65.0). Re-verified against master before fixing. Finding 3 (LLM-controllable user_id) — drop user_id from the public signatures of provision_nextcloud_access, revoke_nextcloud_access, check_provisioning_status, check_logged_in. Tool wrappers now always derive identity from the verified AccessToken; user_id is no longer accepted as MCP input. Adds parameterized CI-guard test that locks the schema. Finding 2 (predictable session cookie) — replace mcp_session= cookie with a cryptographically random session_id mapped server-side (new browser_sessions table, alembic 005). Cookie value is opaque, expires, revocable. SessionAuthBackend looks up user_id via the new mapping and additionally requires a refresh token to fail closed. Finding 4 (logout doesn't revoke refresh token) — oauth_logout now calls the IdP revocation_endpoint (RFC 7009) when advertised, deletes the stored refresh token regardless, and clears the browser_sessions row. Cleanup is best-effort: logout always 302s. Finding 1 (unverified ID token decodes) — verify_id_token helper does JWKS signature + issuer + audience + exp + nonce checks per OIDC core 3.1.3.7. Used by both OAuth callback handlers (browser + MCP). Removes the four "verify_signature: False" decodes that previously trusted IdP claims unconditionally. Drops dead-code _validate_token_audience in token_broker. Refactors token_utils + provisioning_decorator to read user_id from the verified AccessToken instead of re-decoding the JWT. Finding 5 (hardcoded Fernet keys in docker-compose.yml) — replace the three inline TOKEN_ENCRYPTION_KEY values with required env var interpolation; document in env.sample. Test coverage: 4 new unit test modules (signature pinning, browser sessions, ID-token verification, logout + revoke + session backend). 693 unit tests pass; ruff/format/ty clean. Migration note: existing browser admin-UI sessions become invalid on rollout (cookies are looked up against the new browser_sessions table, which starts empty). Users re-login. MCP API access is unaffected. Tracked on Astrolabe Cloud POC board card #37. Co-Authored-By: Claude Opus 4.7 (1M context) --- docker-compose.yml | 20 +- env.sample | 9 + .../20260502_1500_005_add_browser_sessions.py | 49 +++ .../auth/browser_oauth_routes.py | 156 +++++++-- nextcloud_mcp_server/auth/oauth_routes.py | 43 ++- .../auth/provisioning_decorator.py | 26 +- nextcloud_mcp_server/auth/session_backend.py | 38 +- nextcloud_mcp_server/auth/storage.py | 84 +++++ nextcloud_mcp_server/auth/token_broker.py | 30 -- nextcloud_mcp_server/auth/token_utils.py | 180 +++++++--- nextcloud_mcp_server/server/oauth_tools.py | 78 ++--- tests/unit/test_browser_sessions.py | 75 ++++ tests/unit/test_id_token_verification.py | 233 ++++++++++++ tests/unit/test_oauth_logout.py | 331 ++++++++++++++++++ tests/unit/test_oauth_tools_signatures.py | 56 +++ tests/unit/test_token_broker.py | 18 - 16 files changed, 1184 insertions(+), 242 deletions(-) create mode 100644 nextcloud_mcp_server/alembic/versions/20260502_1500_005_add_browser_sessions.py create mode 100644 tests/unit/test_browser_sessions.py create mode 100644 tests/unit/test_id_token_verification.py create mode 100644 tests/unit/test_oauth_logout.py create mode 100644 tests/unit/test_oauth_tools_signatures.py diff --git a/docker-compose.yml b/docker-compose.yml index 8d993c6d..3a1c6c0e 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -153,10 +153,10 @@ services: - ENABLE_MULTI_USER_BASIC_AUTH=true - ENABLE_BACKGROUND_OPERATIONS=true - # Token storage (required for middleware initialization) - # DEVELOPMENT ONLY - generate a fresh key for production: - # python -c "from cryptography.fernet import Fernet; print(Fernet.generate_key().decode())" - - TOKEN_ENCRYPTION_KEY=fqqI4G51yBCOcu9cvv6wCUJB7sf_CK2za5ClC6b86yY= + # Token storage (required for middleware initialization). + # Source the key from .env — see env.sample. To generate a fresh key: + # python -c "from cryptography.fernet import Fernet; print(Fernet.generate_key().decode())" + - TOKEN_ENCRYPTION_KEY=${TOKEN_ENCRYPTION_KEY:?TOKEN_ENCRYPTION_KEY must be set in .env (see env.sample)} - TOKEN_STORAGE_DB=/app/data/tokens.db - ENABLE_SEMANTIC_SEARCH=true @@ -230,9 +230,9 @@ services: - NEXTCLOUD_RESOURCE_URI=nextcloud # ADR-005: Keycloak uses client IDs as audiences, not URLs - NEXTCLOUD_PUBLIC_ISSUER_URL=http://localhost:8888/realms/nextcloud-mcp - # Refresh token storage (ADR-002 Tier 1 & 2) + # Refresh token storage (ADR-002 Tier 1 & 2). Source from .env. - ENABLE_BACKGROUND_OPERATIONS=true - - TOKEN_ENCRYPTION_KEY=ESF1BvEQdGYsCluwMx9Cxvw3uh5pFowPH7Rg_nIliyo= + - TOKEN_ENCRYPTION_KEY=${TOKEN_ENCRYPTION_KEY:?TOKEN_ENCRYPTION_KEY must be set in .env (see env.sample)} - TOKEN_STORAGE_DB=/app/data/tokens.db # ADR-005: Token exchange mode (RFC 8693) @@ -278,10 +278,10 @@ services: # Login Flow v2 (ADR-022) - ENABLE_LOGIN_FLOW=true - # Token storage (required for app password + session persistence) - # DEVELOPMENT ONLY - generate a fresh key for production: - # python -c "from cryptography.fernet import Fernet; print(Fernet.generate_key().decode())" - - TOKEN_ENCRYPTION_KEY=rxJvkBf7ZBjZZDL4a1sSqjhmjawhmbRMSOGfK8HDyKU= + # Token storage (required for app password + session persistence). + # Source the key from .env — see env.sample. To generate a fresh key: + # python -c "from cryptography.fernet import Fernet; print(Fernet.generate_key().decode())" + - TOKEN_ENCRYPTION_KEY=${TOKEN_ENCRYPTION_KEY:?TOKEN_ENCRYPTION_KEY must be set in .env (see env.sample)} - TOKEN_STORAGE_DB=/app/data/tokens.db # Semantic search diff --git a/env.sample b/env.sample index 652d9435..bae9d96e 100644 --- a/env.sample +++ b/env.sample @@ -15,6 +15,15 @@ # Your Nextcloud instance URL (without trailing slash) NEXTCLOUD_HOST= +# Fernet key for encrypting refresh tokens / app passwords / browser +# sessions in SQLite. Required by every docker-compose profile that runs +# the MCP server (single-user, multi-user-basic, keycloak, login-flow). +# Generate one with: +# python -c "from cryptography.fernet import Fernet; print(Fernet.generate_key().decode())" +# NEVER commit a real key. Each environment (dev / staging / prod) needs +# its own key. +TOKEN_ENCRYPTION_KEY= + # ============================================ # SINGLE-USER BASICAUTH MODE # ============================================ diff --git a/nextcloud_mcp_server/alembic/versions/20260502_1500_005_add_browser_sessions.py b/nextcloud_mcp_server/alembic/versions/20260502_1500_005_add_browser_sessions.py new file mode 100644 index 00000000..b85f20d1 --- /dev/null +++ b/nextcloud_mcp_server/alembic/versions/20260502_1500_005_add_browser_sessions.py @@ -0,0 +1,49 @@ +"""Add browser_sessions table for random-id browser cookie auth. + +Replaces the prior `mcp_session=` cookie pattern (issue #626 +finding 2) with a server-side mapping from a cryptographically random +session id to the authenticated user_id. The cookie value is now opaque +and revocable. + +Revision ID: 005 +Revises: 004 +Create Date: 2026-05-02 15:00:00.000000 +""" + +from alembic import op + +revision = "005" +down_revision = "004" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + op.execute( + """ + CREATE TABLE IF NOT EXISTS browser_sessions ( + session_id TEXT PRIMARY KEY, + user_id TEXT NOT NULL, + created_at INTEGER NOT NULL, + expires_at INTEGER NOT NULL + ) + """ + ) + op.execute( + """ + CREATE INDEX IF NOT EXISTS idx_browser_sessions_user + ON browser_sessions(user_id) + """ + ) + op.execute( + """ + CREATE INDEX IF NOT EXISTS idx_browser_sessions_expires + ON browser_sessions(expires_at) + """ + ) + + +def downgrade() -> None: + op.execute("DROP INDEX IF EXISTS idx_browser_sessions_expires") + op.execute("DROP INDEX IF EXISTS idx_browser_sessions_user") + op.execute("DROP TABLE IF EXISTS browser_sessions") diff --git a/nextcloud_mcp_server/auth/browser_oauth_routes.py b/nextcloud_mcp_server/auth/browser_oauth_routes.py index c1050532..577c3024 100644 --- a/nextcloud_mcp_server/auth/browser_oauth_routes.py +++ b/nextcloud_mcp_server/auth/browser_oauth_routes.py @@ -10,14 +10,18 @@ import os import secrets import time from base64 import urlsafe_b64encode +from html import escape as html_escape from urllib.parse import urlencode from urllib.parse import urlparse as parse_url import httpx -import jwt from starlette.requests import Request from starlette.responses import HTMLResponse, JSONResponse, RedirectResponse +from nextcloud_mcp_server.auth.token_utils import ( + IdTokenVerificationError, + verify_id_token, +) from nextcloud_mcp_server.auth.userinfo_routes import ( _get_userinfo_endpoint, _query_idp_userinfo, @@ -383,16 +387,44 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo logger.info(f"Refresh token present: {refresh_token is not None}") logger.info(f"ID token present: {id_token is not None}") - # Decode ID token to get user info + # Resolve the discovery URL + audience used for THIS auth request so + # we can verify the ID token signature + claims (issue #626 finding 1). + if oauth_client: + # External IdP path + verification_audience = oauth_client.client_id + verification_discovery_url = getattr(oauth_client, "discovery_url", None) + else: + # Integrated Nextcloud OIDC path + verification_audience = oauth_config["client_id"] + verification_discovery_url = oauth_config.get("discovery_url") + + if not verification_discovery_url: + logger.error("Cannot verify ID token: no discovery_url available") + return HTMLResponse( + "

Login Failed

OIDC discovery URL not configured

", + status_code=500, + ) + try: - userinfo = jwt.decode(id_token, options={"verify_signature": False}) - user_id = userinfo.get("sub") - username = userinfo.get("preferred_username") or userinfo.get("email") - logger.info(f"Browser login successful: {username} (sub={user_id})") - except Exception as e: - logger.warning(f"Failed to decode ID token: {e}") - user_id = f"user-{secrets.token_hex(8)}" - username = "unknown" + userinfo = await verify_id_token( + id_token, + discovery_url=verification_discovery_url, + expected_audience=verification_audience, + ) + except IdTokenVerificationError as e: + logger.error("ID token verification failed: %s", e) + # html_escape: defense-in-depth. The exception text is currently + # server-constructed, but escape on the success path too so any + # future error wrapping that includes IdP response text can't + # smuggle markup into the login-failure page. + return HTMLResponse( + f"

Login Failed

ID token failed verification: {html_escape(str(e))}

", + status_code=400, + ) + + user_id = userinfo["sub"] + username = userinfo.get("preferred_username") or userinfo.get("email") + logger.info("Browser login successful: %s (sub=%s)", username, user_id) # Calculate refresh token expiration from token response refresh_expires_in = token_data.get("refresh_expires_in") @@ -455,40 +487,118 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo logger.error(f"Error caching user profile: {e}") # Continue anyway - profile cache is optional for browser UI - # Create response and set session cookie - # Redirect to stored next_url (from OAuth session) or /app as default + # Create a server-side browser session: a random opaque session_id is + # mapped to the verified user_id in `browser_sessions`. The cookie value + # is the session_id (never the raw user_id — see issue #626 finding 2). + session_id = secrets.token_urlsafe(32) + session_ttl = 86400 * 30 # 30 days + await storage.create_browser_session( + session_id=session_id, user_id=user_id, ttl_seconds=session_ttl + ) + response = RedirectResponse(next_url, status_code=302) response.set_cookie( key="mcp_session", - value=user_id, - max_age=86400 * 30, # 30 days + value=session_id, + max_age=session_ttl, httponly=True, secure=_should_use_secure_cookies(), samesite="lax", ) - logger.info(f"Session cookie set for user: {username}") + logger.info("Session cookie set for user %s (sid=%s…)", username, session_id[:8]) return response async def oauth_logout(request: Request) -> RedirectResponse: - """Browser OAuth logout - clears session cookie. + """Browser OAuth logout — invalidate session and revoke refresh token. + + Issue #626 finding 4: prior implementation only cleared the cookie, + leaving the refresh token in storage (valid up to 90 days). This now: + 1. Resolves the user_id for the current browser session_id. + 2. Calls the IdP `revocation_endpoint` for the stored refresh token + when the IdP advertises one. + 3. Deletes the stored refresh token regardless of revocation success. + 4. Deletes the browser_sessions row so the cookie is unusable even + if it leaks. + 5. Clears the cookie on the response. Query parameters: next: Optional URL to redirect to after logout (default: /oauth/login) - - Returns: - 302 redirect with cleared session cookie """ next_url = request.query_params.get("next", "/oauth/login") + session_id = request.cookies.get("mcp_session") - # TODO: Optionally revoke refresh token from storage - # session_id = request.cookies.get("mcp_session") - # if session_id: - # await storage.delete_refresh_token(session_id) + oauth_ctx = getattr(request.app.state, "oauth_context", None) + storage = oauth_ctx.get("storage") if oauth_ctx else None + + if session_id and storage and oauth_ctx: + try: + user_id = await storage.get_browser_session_user(session_id) + if user_id: + token_data = await storage.get_refresh_token(user_id) + refresh_token = token_data.get("refresh_token") if token_data else None + + if refresh_token: + await _revoke_refresh_token_at_idp(oauth_ctx, refresh_token) + await storage.delete_refresh_token(user_id) + logger.info("Refresh token revoked + deleted for user %s", user_id) + + await storage.delete_browser_session(session_id) + except Exception as e: + # Logout must always succeed locally; log and continue. + logger.warning("Logout cleanup failed (continuing): %s", e) response = RedirectResponse(next_url, status_code=302) response.delete_cookie("mcp_session") logger.info("User logged out, session cookie cleared") return response + + +async def _revoke_refresh_token_at_idp(oauth_ctx: dict, refresh_token: str) -> None: + """Best-effort RFC 7009 revocation against the IdP. + + Silent on failure: revoking remotely is a defense-in-depth step on top + of deleting the local copy, and we don't want logout to error if the + IdP is unreachable or doesn't advertise a revocation endpoint. + """ + try: + discovery_url = oauth_ctx.get("discovery_url") or os.getenv( + "OIDC_DISCOVERY_URL", + f"{os.getenv('NEXTCLOUD_HOST', '')}/.well-known/openid-configuration", + ) + if not discovery_url: + return + + async with nextcloud_httpx_client() as http_client: + discovery_response = await http_client.get(discovery_url) + discovery_response.raise_for_status() + discovery = discovery_response.json() + revocation_endpoint = discovery.get("revocation_endpoint") + if not revocation_endpoint: + logger.debug("IdP advertises no revocation_endpoint; skipping") + return + + client_id = oauth_ctx.get("client_id") or os.getenv("OIDC_CLIENT_ID") + client_secret = oauth_ctx.get("client_secret") or os.getenv( + "OIDC_CLIENT_SECRET" + ) + if not (client_id and client_secret): + logger.debug("No OIDC client credentials available for revocation") + return + + response = await http_client.post( + revocation_endpoint, + data={ + "token": refresh_token, + "token_type_hint": "refresh_token", + }, + auth=(client_id, client_secret), + ) + if response.status_code >= 400: + logger.warning( + "Refresh token revocation returned HTTP %s", response.status_code + ) + except Exception as e: + logger.warning("Refresh token revocation failed: %s", e) diff --git a/nextcloud_mcp_server/auth/oauth_routes.py b/nextcloud_mcp_server/auth/oauth_routes.py index 1eb39286..1ea81551 100644 --- a/nextcloud_mcp_server/auth/oauth_routes.py +++ b/nextcloud_mcp_server/auth/oauth_routes.py @@ -30,13 +30,16 @@ from typing import Any from urllib.parse import unquote, urlencode from urllib.parse import urlparse as parse_url -import jwt from starlette.requests import Request from starlette.responses import HTMLResponse, JSONResponse, RedirectResponse from nextcloud_mcp_server.auth.browser_oauth_routes import oauth_login_callback from nextcloud_mcp_server.auth.client_registry import get_client_registry from nextcloud_mcp_server.auth.storage import RefreshTokenStorage +from nextcloud_mcp_server.auth.token_utils import ( + IdTokenVerificationError, + verify_id_token, +) from nextcloud_mcp_server.config import get_settings from ..http import nextcloud_httpx_client @@ -643,23 +646,29 @@ async def oauth_callback_nextcloud(request: Request): refresh_token = token_data.get("refresh_token") id_token = token_data.get("id_token") - # Decode ID token to get user info - logger.info("=" * 60) - logger.info("oauth_callback_nextcloud: Extracting user_id from ID token") - logger.info("=" * 60) + # Verify ID token signature + claims (issue #626 finding 1). + logger.info("oauth_callback_nextcloud: Verifying ID token") try: - userinfo = jwt.decode(id_token, options={"verify_signature": False}) - user_id = userinfo.get("sub") - username = userinfo.get("preferred_username") or userinfo.get("email") - logger.info(" ✓ ID token decode SUCCESSFUL") - logger.info(f" Extracted user_id: {user_id}") - logger.info(f" Username: {username}") - logger.info(f" ID token payload keys: {list(userinfo.keys())}") - logger.info(f"Flow 2: User {username} provisioned resource access") - except Exception as e: - logger.error(f" ✗ ID token decode FAILED: {type(e).__name__}: {e}") - user_id = "unknown" - logger.error(f" Using fallback user_id: {user_id}") + userinfo = await verify_id_token( + id_token, + discovery_url=discovery_url, + expected_audience=mcp_server_client_id, + ) + except IdTokenVerificationError as e: + logger.error("ID token verification failed: %s", e) + return JSONResponse( + { + "error": "invalid_token", + "error_description": "ID token failed verification", + }, + status_code=400, + ) + + user_id = userinfo["sub"] + username = userinfo.get("preferred_username") or userinfo.get("email") + logger.info( + "Flow 2: User %s (sub=%s) provisioned resource access", username, user_id + ) # Store master refresh token for Flow 2 if refresh_token: diff --git a/nextcloud_mcp_server/auth/provisioning_decorator.py b/nextcloud_mcp_server/auth/provisioning_decorator.py index 585b6ff2..f9534e31 100644 --- a/nextcloud_mcp_server/auth/provisioning_decorator.py +++ b/nextcloud_mcp_server/auth/provisioning_decorator.py @@ -9,7 +9,7 @@ import functools import logging from typing import Callable -import jwt +from mcp.server.auth.middleware.auth_context import get_access_token from mcp.server.fastmcp import Context from mcp.shared.exceptions import McpError from mcp.types import ErrorData @@ -65,16 +65,12 @@ def require_provisioning(func: Callable) -> Callable: return await func(*args, **kwargs) # Offline access mode - check if user has completed Flow 2 provisioning - # Get user_id from authorization token - user_id = None - if hasattr(ctx, "authorization") and ctx.authorization: - try: - token = ctx.authorization.token - payload = jwt.decode(token, options={"verify_signature": False}) - user_id = payload.get("sub") - logger.debug(f"Checking provisioning for user: {user_id}") - except Exception as e: - logger.warning(f"Failed to extract user_id from token: {e}") + # Read user_id from the verified AccessToken populated by + # UnifiedTokenVerifier; no second decode of the raw JWT here. + access_token = get_access_token() + user_id = access_token.resource if access_token else None + if user_id: + logger.debug("Checking provisioning for user: %s", user_id) if not user_id: raise McpError( @@ -149,12 +145,8 @@ def require_provisioning_or_suggest(func: Callable) -> Callable: if ctx: # Try to check provisioning status try: - # Get user_id from authorization token - user_id = None - if hasattr(ctx, "authorization") and ctx.authorization: - token = ctx.authorization.token - payload = jwt.decode(token, options={"verify_signature": False}) - user_id = payload.get("sub") + access_token = get_access_token() + user_id = access_token.resource if access_token else None if user_id: # Check provisioning status diff --git a/nextcloud_mcp_server/auth/session_backend.py b/nextcloud_mcp_server/auth/session_backend.py index 1a3dc714..f4654b64 100644 --- a/nextcloud_mcp_server/auth/session_backend.py +++ b/nextcloud_mcp_server/auth/session_backend.py @@ -52,44 +52,46 @@ class SessionAuthBackend(AuthenticationBackend): username = os.getenv("NEXTCLOUD_USERNAME", "admin") return AuthCredentials(["authenticated", "admin"]), SimpleUser(username) - # OAuth mode: Check for session cookie + # OAuth mode: opaque random session_id cookie -> user_id mapping. + # Replaces the prior `mcp_session=` cookie pattern (issue + # #626 finding 2). The cookie value is no longer the user identity; + # we look it up server-side and reject unknown / expired sessions. session_id = conn.cookies.get("mcp_session") - logger.info( - f"Session authentication check - cookie present: {session_id is not None}, path: {conn.url.path}" - ) if not session_id: logger.info("No session cookie found - redirecting to login") return None - logger.info(f"Found session cookie: {session_id[:16]}...") - - # Get OAuth context from app state oauth_context = getattr(conn.app.state, "oauth_context", None) if not oauth_context: logger.warning("OAuth context not available in app state") return None - # Validate session storage = oauth_context.get("storage") if not storage: logger.warning("OAuth storage not available") return None try: - # Check if user has refresh token (indicates logged-in session) - logger.info(f"Looking up refresh token for session: {session_id[:16]}...") - token_data = await storage.get_refresh_token(session_id) - if not token_data: - logger.warning( - f"No refresh token found for session {session_id[:16]}..." + user_id = await storage.get_browser_session_user(session_id) + if not user_id: + logger.info( + "Browser session not found or expired (sid=%s…)", session_id[:8] ) return None - # Session is valid - use session_id (which is user_id from ID token) as username - username = session_id - logger.info(f"✓ Session authenticated successfully: {username[:16]}...") + # Defense-in-depth: only authenticate sessions for users that + # actually have a refresh token persisted. Logout deletes both, + # so an expired/revoked user state will fail closed here. + token_data = await storage.get_refresh_token(user_id) + if not token_data: + logger.warning( + "Session %s… has no refresh token for user %s; rejecting", + session_id[:8], + user_id, + ) + return None - return AuthCredentials(["authenticated"]), SimpleUser(username) + return AuthCredentials(["authenticated"]), SimpleUser(user_id) except Exception as e: logger.warning(f"Session validation error: {e}") diff --git a/nextcloud_mcp_server/auth/storage.py b/nextcloud_mcp_server/auth/storage.py index ed6675df..38da1d11 100644 --- a/nextcloud_mcp_server/auth/storage.py +++ b/nextcloud_mcp_server/auth/storage.py @@ -1133,6 +1133,90 @@ class RefreshTokenStorage: return deleted + # ============================================================================ + # Browser Sessions (OAuth admin UI) + # ============================================================================ + # + # Maps a cryptographically random `session_id` (cookie value) to the + # authenticated user_id. Replaces the prior `mcp_session=` + # cookie pattern (issue #626 finding 2). Cookie value is opaque, expires, + # and can be revoked server-side without forcing the user to roll their + # IdP `sub`. + + async def create_browser_session( + self, + session_id: str, + user_id: str, + ttl_seconds: int = 86400 * 30, + ) -> None: + """Persist a random session_id → user_id mapping for browser auth.""" + if not self._initialized: + await self.initialize() + + now = int(time.time()) + expires_at = now + ttl_seconds + + async with aiosqlite.connect(self.db_path) as db: + await db.execute( + """ + INSERT OR REPLACE INTO browser_sessions + (session_id, user_id, created_at, expires_at) + VALUES (?, ?, ?, ?) + """, + (session_id, user_id, now, expires_at), + ) + await db.commit() + + logger.debug( + "Stored browser session %s for user %s (expires in %ss)", + session_id[:8], + user_id, + ttl_seconds, + ) + + async def get_browser_session_user(self, session_id: str) -> Optional[str]: + """Look up the user_id bound to a browser session_id, or None. + + Returns None when the session is unknown or expired. Expired rows + are deleted on encounter to keep the table small. + """ + if not self._initialized: + await self.initialize() + + async with aiosqlite.connect(self.db_path) as db: + db.row_factory = aiosqlite.Row + async with db.execute( + "SELECT user_id, expires_at FROM browser_sessions WHERE session_id = ?", + (session_id,), + ) as cursor: + row = await cursor.fetchone() + + if not row: + return None + + if row["expires_at"] < time.time(): + logger.debug("Browser session %s expired", session_id[:8]) + await self.delete_browser_session(session_id) + return None + + return row["user_id"] + + async def delete_browser_session(self, session_id: str) -> bool: + """Delete a browser session row. Returns True when a row was removed.""" + if not self._initialized: + await self.initialize() + + async with aiosqlite.connect(self.db_path) as db: + cursor = await db.execute( + "DELETE FROM browser_sessions WHERE session_id = ?", (session_id,) + ) + await db.commit() + deleted = cursor.rowcount > 0 + + if deleted: + logger.debug("Deleted browser session %s", session_id[:8]) + return deleted + # ============================================================================ # Webhook Registration Tracking (both BasicAuth and OAuth modes) # ============================================================================ diff --git a/nextcloud_mcp_server/auth/token_broker.py b/nextcloud_mcp_server/auth/token_broker.py index c4729edb..9885246f 100644 --- a/nextcloud_mcp_server/auth/token_broker.py +++ b/nextcloud_mcp_server/auth/token_broker.py @@ -20,7 +20,6 @@ from typing import Dict, Optional, Tuple import anyio import httpx -import jwt from nextcloud_mcp_server.auth.storage import RefreshTokenStorage @@ -489,35 +488,6 @@ class TokenBrokerService: ) return access_token, expires_in - async def _validate_token_audience(self, token: str, expected_audience: str): - """ - Validate that token has correct audience claim. - - Args: - token: JWT token to validate - expected_audience: Expected audience value - - Raises: - ValueError: If audience doesn't match - """ - try: - # Decode without verification to check claims - # In production, should verify signature - claims = jwt.decode(token, options={"verify_signature": False}) - - audience = claims.get("aud", []) - if isinstance(audience, str): - audience = [audience] - - if expected_audience not in audience: - raise ValueError( - f"Token audience {audience} doesn't include {expected_audience}" - ) - - except jwt.DecodeError as e: - # Token might be opaque, skip validation - logger.debug(f"Cannot decode token for audience validation: {e}") - async def refresh_master_token(self, user_id: str) -> bool: """ Refresh the master refresh token (periodic rotation). diff --git a/nextcloud_mcp_server/auth/token_utils.py b/nextcloud_mcp_server/auth/token_utils.py index d9a4cb4f..51020ec1 100644 --- a/nextcloud_mcp_server/auth/token_utils.py +++ b/nextcloud_mcp_server/auth/token_utils.py @@ -5,81 +5,155 @@ between server/ and auth/ layers. """ import logging -import os +from typing import Any import jwt +from jwt import PyJWKSet from mcp.server.auth.middleware.auth_context import get_access_token from mcp.server.auth.provider import AccessToken from mcp.server.fastmcp import Context -from nextcloud_mcp_server.auth.userinfo_routes import _query_idp_userinfo - from ..http import nextcloud_httpx_client logger = logging.getLogger(__name__) -async def extract_user_id_from_token(ctx: Context) -> str: - """Extract user_id from the MCP access token (Flow 1). +class IdTokenVerificationError(Exception): + """Raised when an OIDC ID token fails signature or claim verification.""" - Handles both JWT and opaque tokens: - - JWT: Decode and extract 'sub' claim - - Opaque: Call userinfo endpoint to get 'sub' + +async def verify_id_token( + id_token: str, + *, + discovery_url: str, + expected_audience: str, + expected_nonce: str | None = None, +) -> dict[str, Any]: + """Verify an OIDC ID token's signature and standard claims. + + Implements the verification steps required by OIDC core spec section + 3.1.3.7 (ID Token Validation) for the authorization-code flow: + - Signature against JWKS (RS256) + - Issuer matches the OP that issued the token + - Audience contains the expected client_id + - Token is not expired (`exp`) + - `iat` is well-formed (PyJWT default) + - `nonce` matches when one was included in the auth request + + Replaces the prior `jwt.decode(id_token, options={"verify_signature": False})` + pattern (issue #626 finding 1) on the OAuth callback paths. Args: - ctx: MCP context with access token + id_token: Raw ID token (JWT) string. + discovery_url: OIDC `.well-known/openid-configuration` URL of the IdP. + expected_audience: The MCP-server-side OAuth client_id used for this + authorization request. + expected_nonce: When the auth request included a nonce, the same value + so it can be checked here. None disables the nonce check (callers + that didn't bind a nonce in the auth request). Returns: - user_id extracted from token, or "default_user" as fallback + Decoded, verified ID-token claims. + + Raises: + IdTokenVerificationError: On any verification failure. """ - # Use MCP SDK's get_access_token() which uses contextvars - access_token: AccessToken | None = get_access_token() + if not id_token: + raise IdTokenVerificationError("ID token missing from token response") - if not access_token or not access_token.token: - logger.warning(" ✗ No access token found via get_access_token()") - return "default_user" - - token = access_token.token - is_jwt = "." in token and token.count(".") >= 2 - logger.info(f" Token type: {'JWT' if is_jwt else 'Opaque'}") - - # Try JWT decode first - if is_jwt: - try: - payload = jwt.decode(token, options={"verify_signature": False}) - user_id = payload.get("sub", "unknown") - logger.info(f" ✓ JWT decode successful: user_id={user_id}") - return user_id - except Exception as e: - logger.error(f" ✗ JWT decode failed: {type(e).__name__}: {e}") - - # Opaque token - call userinfo endpoint - logger.info(" Opaque token detected, calling userinfo endpoint...") try: - # Get userinfo endpoint from OIDC discovery - oidc_discovery_uri = os.getenv( - "OIDC_DISCOVERY_URI", - "http://localhost:8080/.well-known/openid-configuration", - ) async with nextcloud_httpx_client() as http_client: - discovery_response = await http_client.get(oidc_discovery_uri) + discovery_response = await http_client.get(discovery_url) discovery_response.raise_for_status() discovery = discovery_response.json() - userinfo_endpoint = discovery.get("userinfo_endpoint") - if userinfo_endpoint: - userinfo = await _query_idp_userinfo(token, userinfo_endpoint) - if userinfo: - user_id = userinfo.get("sub", "unknown") - logger.info(f" ✓ Userinfo query successful: user_id={user_id}") - return user_id - else: - logger.error(" ✗ Userinfo query failed") - else: - logger.error(" ✗ No userinfo_endpoint available") + issuer = discovery.get("issuer") + jwks_uri = discovery.get("jwks_uri") + if not issuer or not jwks_uri: + raise IdTokenVerificationError( + "OIDC discovery response missing issuer or jwks_uri" + ) + + jwks_response = await http_client.get(jwks_uri) + jwks_response.raise_for_status() + jwks_data = jwks_response.json() + except IdTokenVerificationError: + raise except Exception as e: - logger.error(f" ✗ Userinfo query failed: {type(e).__name__}: {e}") + raise IdTokenVerificationError( + f"Failed to fetch OIDC discovery / JWKS: {e}" + ) from e - # Fallback - logger.warning(" Using fallback user_id: default_user") - return "default_user" + try: + jwks = PyJWKSet.from_dict(jwks_data) + unverified_header = jwt.get_unverified_header(id_token) + kid = unverified_header.get("kid") + if not kid: + 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 + + payload: dict[str, Any] = jwt.decode( + id_token, + signing_key.key, + algorithms=["RS256"], + audience=expected_audience, + issuer=issuer, + options={ + "verify_signature": True, + "verify_exp": True, + "verify_iat": True, + "verify_aud": True, + "verify_iss": True, + "require": ["sub", "iss", "aud", "exp", "iat"], + }, + ) + except IdTokenVerificationError: + raise + except jwt.PyJWTError as e: + raise IdTokenVerificationError(f"ID token verification failed: {e}") from e + except Exception as e: + raise IdTokenVerificationError( + f"Unexpected error verifying ID token: {e}" + ) from e + + if expected_nonce is not None and payload.get("nonce") != expected_nonce: + raise IdTokenVerificationError("ID token nonce does not match request nonce") + + return payload + + +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 + `UnifiedTokenVerifier` after JWT signature verification (or token + introspection for opaque tokens). We never re-decode the raw token here: + the verifier has already validated the signature and extracted the + identity claim. + + Args: + ctx: MCP context with access token (unused — kept for the public API) + + Returns: + user_id from the verified token, or "default_user" when no token is + present (e.g. BasicAuth mode where this should not be called). + """ + access_token: AccessToken | None = get_access_token() + + if not access_token: + logger.warning("No access token found via get_access_token()") + return "default_user" + + user_id = access_token.resource + if not user_id: + logger.error( + "Access token has no resource (sub) claim — verifier should have rejected it" + ) + return "default_user" + + return user_id diff --git a/nextcloud_mcp_server/server/oauth_tools.py b/nextcloud_mcp_server/server/oauth_tools.py index 3698aa82..90769bf5 100644 --- a/nextcloud_mcp_server/server/oauth_tools.py +++ b/nextcloud_mcp_server/server/oauth_tools.py @@ -198,9 +198,7 @@ def generate_oauth_url_for_flow2( return f"{auth_endpoint}?{urlencode(params)}" -async def provision_nextcloud_access( - ctx: Context, user_id: Optional[str] = None -) -> ProvisioningResult: +async def provision_nextcloud_access(ctx: Context, user_id: str) -> ProvisioningResult: """ MCP Tool: Provision offline access to Nextcloud resources. @@ -211,16 +209,13 @@ async def provision_nextcloud_access( Args: ctx: MCP context with user's Flow 1 token - user_id: Optional user identifier (extracted from token if not provided) + user_id: Authenticated user identifier (must be derived from the + verified access token by the caller; never accept from MCP input). Returns: ProvisioningResult with Astrolabe settings URL or status """ try: - # Extract user ID from the MCP access token (Flow 1 token) - if not user_id: - user_id = await extract_user_id_from_token(ctx) - # Check if already provisioned status = await get_provisioning_status(ctx, user_id) if status.is_provisioned: @@ -271,9 +266,7 @@ async def provision_nextcloud_access( ) -async def revoke_nextcloud_access( - ctx: Context, user_id: Optional[str] = None -) -> RevocationResult: +async def revoke_nextcloud_access(ctx: Context, user_id: str) -> RevocationResult: """ MCP Tool: Revoke offline access to Nextcloud resources. @@ -281,19 +274,14 @@ async def revoke_nextcloud_access( that was granted via Flow 2. Args: - mcp: MCP context - user_id: Optional user identifier + ctx: MCP context + user_id: Authenticated user identifier (must be derived from the + verified access token by the caller; never accept from MCP input). Returns: RevocationResult with status """ try: - # Get user ID from token if not provided - if not user_id: - logger.info("Extracting user_id from access token for revoke...") - user_id = await extract_user_id_from_token(ctx) - logger.info(f" Revoke using user_id: {user_id}") - # Check current status status = await get_provisioning_status(ctx, user_id) if not status.is_provisioned: @@ -350,9 +338,7 @@ async def revoke_nextcloud_access( ) -async def check_provisioning_status( - ctx: Context, user_id: Optional[str] = None -) -> ProvisioningStatus: +async def check_provisioning_status(ctx: Context, user_id: str) -> ProvisioningStatus: """ MCP Tool: Check the current provisioning status. @@ -360,24 +346,17 @@ async def check_provisioning_status( Nextcloud access and see details about their current authorization. Args: - mcp: MCP context - user_id: Optional user identifier + ctx: MCP context + user_id: Authenticated user identifier (must be derived from the + verified access token by the caller; never accept from MCP input). Returns: ProvisioningStatus with current state """ - # Get user ID from context if not provided - if not user_id: - user_id = ( - ctx.context.get("user_id", "default_user") # type: ignore - if hasattr(ctx, "context") - else "default_user" - ) - return await get_provisioning_status(ctx, user_id) -async def check_logged_in(ctx: Context, user_id: Optional[str] = None) -> str: +async def check_logged_in(ctx: Context, user_id: str) -> str: """ MCP Tool: Check if user is logged in and elicit login if needed. @@ -387,23 +366,13 @@ async def check_logged_in(ctx: Context, user_id: Optional[str] = None) -> str: Args: ctx: MCP context with user's Flow 1 token - user_id: Optional user identifier (extracted from token if not provided) + user_id: Authenticated user identifier (must be derived from the + verified access token by the caller; never accept from MCP input). Returns: "yes" if logged in, or elicitation prompting for login """ try: - # Extract user ID from the MCP access token (Flow 1 token) - logger.info("=" * 60) - logger.info("check_logged_in: Starting user_id extraction") - logger.info("=" * 60) - - if not user_id: - user_id = await extract_user_id_from_token(ctx) - logger.info(f" Final user_id for check_logged_in: {user_id}") - else: - logger.info(f" user_id provided as argument: {user_id}") - # Check if already logged in logger.info(f"Checking provisioning status for user_id: {user_id}") status = await get_provisioning_status(ctx, user_id) @@ -591,10 +560,8 @@ def register_oauth_tools(mcp): ), ) @require_scopes("openid") - async def tool_provision_access( - ctx: Context, - user_id: Optional[str] = None, - ) -> ProvisioningResult: + async def tool_provision_access(ctx: Context) -> ProvisioningResult: + user_id = await extract_user_id_from_token(ctx) return await provision_nextcloud_access(ctx, user_id) @mcp.tool( @@ -608,9 +575,8 @@ def register_oauth_tools(mcp): ), ) @require_scopes("openid") - async def tool_revoke_access( - ctx: Context, user_id: Optional[str] = None - ) -> RevocationResult: + async def tool_revoke_access(ctx: Context) -> RevocationResult: + user_id = await extract_user_id_from_token(ctx) return await revoke_nextcloud_access(ctx, user_id) @mcp.tool( @@ -623,9 +589,8 @@ def register_oauth_tools(mcp): ), ) @require_scopes("openid") - async def tool_check_status( - ctx: Context, user_id: Optional[str] = None - ) -> ProvisioningStatus: + async def tool_check_status(ctx: Context) -> ProvisioningStatus: + user_id = await extract_user_id_from_token(ctx) return await check_provisioning_status(ctx, user_id) @mcp.tool( @@ -641,5 +606,6 @@ def register_oauth_tools(mcp): ), ) @require_scopes("openid") - async def tool_check_logged_in(ctx: Context, user_id: Optional[str] = None) -> str: + async def tool_check_logged_in(ctx: Context) -> str: + user_id = await extract_user_id_from_token(ctx) return await check_logged_in(ctx, user_id) diff --git a/tests/unit/test_browser_sessions.py b/tests/unit/test_browser_sessions.py new file mode 100644 index 00000000..7851d933 --- /dev/null +++ b/tests/unit/test_browser_sessions.py @@ -0,0 +1,75 @@ +"""Unit tests for browser_sessions storage (issue #626 finding 2). + +The browser admin UI no longer uses the raw user_id as the cookie value +— it uses a cryptographically random session_id mapped server-side to +user_id. These tests pin the storage contract. +""" + +import secrets +import tempfile +import time +from pathlib import Path + +import pytest +from cryptography.fernet import Fernet + +from nextcloud_mcp_server.auth.storage import RefreshTokenStorage + +pytestmark = pytest.mark.unit + + +@pytest.fixture +async def storage(): + with tempfile.TemporaryDirectory() as tmpdir: + db_path = Path(tmpdir) / "test_browser_sessions.db" + s = RefreshTokenStorage( + db_path=str(db_path), encryption_key=Fernet.generate_key().decode() + ) + await s.initialize() + yield s + + +async def test_create_and_get_browser_session(storage): + sid = secrets.token_urlsafe(32) + await storage.create_browser_session(session_id=sid, user_id="alice") + + user_id = await storage.get_browser_session_user(sid) + assert user_id == "alice" + + +async def test_get_browser_session_unknown_returns_none(storage): + assert await storage.get_browser_session_user("does-not-exist") is None + + +async def test_delete_browser_session(storage): + sid = secrets.token_urlsafe(32) + await storage.create_browser_session(session_id=sid, user_id="alice") + + deleted = await storage.delete_browser_session(sid) + assert deleted is True + assert await storage.get_browser_session_user(sid) is None + + +async def test_expired_browser_session_rejected_and_deleted(storage): + sid = secrets.token_urlsafe(32) + # ttl_seconds=0 so the row is immediately expired (now == expires_at) + await storage.create_browser_session(session_id=sid, user_id="alice", ttl_seconds=0) + # Make sure clock advances past expires_at + time.sleep(0.01) + + assert await storage.get_browser_session_user(sid) is None + # Expired row should be deleted on encounter + assert await storage.delete_browser_session(sid) is False + + +async def test_replace_existing_session_id(storage): + """INSERT OR REPLACE so re-using a session_id rebinds the user. + + Not a recommended call pattern (session_ids are random), but the + storage layer must not raise UNIQUE constraint errors if it happens. + """ + sid = secrets.token_urlsafe(32) + await storage.create_browser_session(session_id=sid, user_id="alice") + await storage.create_browser_session(session_id=sid, user_id="bob") + + assert await storage.get_browser_session_user(sid) == "bob" diff --git a/tests/unit/test_id_token_verification.py b/tests/unit/test_id_token_verification.py new file mode 100644 index 00000000..84ed854a --- /dev/null +++ b/tests/unit/test_id_token_verification.py @@ -0,0 +1,233 @@ +"""Unit tests for OIDC ID token verification (issue #626 finding 1). + +The OAuth callback handlers used to call +`jwt.decode(id_token, options={"verify_signature": False})` and trust the +result. They now go through `verify_id_token`, which checks signature +against JWKS and validates issuer / audience / exp / nonce per OIDC core +spec §3.1.3.7. +""" + +import json +import time +from base64 import urlsafe_b64encode +from unittest.mock import patch + +import httpx +import jwt +import pytest +from cryptography.hazmat.primitives import serialization +from cryptography.hazmat.primitives.asymmetric import rsa + +from nextcloud_mcp_server.auth.token_utils import ( + IdTokenVerificationError, + verify_id_token, +) + +pytestmark = pytest.mark.unit + + +# Generated once per process — RSA keypair generation is slow. +_KEY = rsa.generate_private_key(public_exponent=65537, key_size=2048) +_PRIVATE_PEM = _KEY.private_bytes( + encoding=serialization.Encoding.PEM, + format=serialization.PrivateFormat.TraditionalOpenSSL, + encryption_algorithm=serialization.NoEncryption(), +) +_OTHER_KEY = rsa.generate_private_key(public_exponent=65537, key_size=2048) +_OTHER_PRIVATE_PEM = _OTHER_KEY.private_bytes( + encoding=serialization.Encoding.PEM, + format=serialization.PrivateFormat.TraditionalOpenSSL, + encryption_algorithm=serialization.NoEncryption(), +) + +ISSUER = "https://idp.example.com" +DISCOVERY_URL = f"{ISSUER}/.well-known/openid-configuration" +JWKS_URI = f"{ISSUER}/jwks" + + +def _b64u_uint(n: int) -> str: + raw = n.to_bytes((n.bit_length() + 7) // 8, "big") + return urlsafe_b64encode(raw).rstrip(b"=").decode("ascii") + + +def _build_jwks() -> dict: + pub = _KEY.public_key().public_numbers() + return { + "keys": [ + { + "kty": "RSA", + "use": "sig", + "kid": "test-key-1", + "alg": "RS256", + "n": _b64u_uint(pub.n), + "e": _b64u_uint(pub.e), + } + ] + } + + +def _sign( + claims: dict, *, kid: str = "test-key-1", key_pem: bytes = _PRIVATE_PEM +) -> str: + return jwt.encode(claims, key_pem, algorithm="RS256", headers={"kid": kid}) + + +def _idp_handler(request: httpx.Request) -> httpx.Response: + if str(request.url) == DISCOVERY_URL: + return httpx.Response(200, json={"issuer": ISSUER, "jwks_uri": JWKS_URI}) + if str(request.url) == JWKS_URI: + return httpx.Response( + 200, + content=json.dumps(_build_jwks()).encode(), + headers={"content-type": "application/json"}, + ) + return httpx.Response(404) + + +@pytest.fixture +def mock_idp(): + """Patch nextcloud_httpx_client used inside token_utils.verify_id_token.""" + transport = httpx.MockTransport(_idp_handler) + + def fake_client(**kwargs): + kwargs["transport"] = transport + return httpx.AsyncClient(**kwargs) + + with patch( + "nextcloud_mcp_server.auth.token_utils.nextcloud_httpx_client", + side_effect=fake_client, + ): + yield + + +async def test_verify_id_token_accepts_valid_token(mock_idp): + now = int(time.time()) + token = _sign( + { + "iss": ISSUER, + "aud": "test-client", + "sub": "alice", + "iat": now, + "exp": now + 60, + } + ) + payload = await verify_id_token( + token, discovery_url=DISCOVERY_URL, expected_audience="test-client" + ) + assert payload["sub"] == "alice" + + +async def test_verify_id_token_rejects_wrong_audience(mock_idp): + now = int(time.time()) + token = _sign( + { + "iss": ISSUER, + "aud": "other-client", + "sub": "alice", + "iat": now, + "exp": now + 60, + } + ) + with pytest.raises(IdTokenVerificationError): + await verify_id_token( + token, discovery_url=DISCOVERY_URL, expected_audience="test-client" + ) + + +async def test_verify_id_token_rejects_expired_token(mock_idp): + now = int(time.time()) + token = _sign( + { + "iss": ISSUER, + "aud": "test-client", + "sub": "alice", + "iat": now - 120, + "exp": now - 60, + } + ) + with pytest.raises(IdTokenVerificationError): + await verify_id_token( + token, discovery_url=DISCOVERY_URL, expected_audience="test-client" + ) + + +async def test_verify_id_token_rejects_wrong_issuer(mock_idp): + now = int(time.time()) + token = _sign( + { + "iss": "https://evil.example.com", + "aud": "test-client", + "sub": "alice", + "iat": now, + "exp": now + 60, + } + ) + with pytest.raises(IdTokenVerificationError): + await verify_id_token( + token, discovery_url=DISCOVERY_URL, expected_audience="test-client" + ) + + +async def test_verify_id_token_rejects_wrong_signature(mock_idp): + """Token signed with a different key but matching kid header must fail.""" + now = int(time.time()) + forged = _sign( + { + "iss": ISSUER, + "aud": "test-client", + "sub": "alice", + "iat": now, + "exp": now + 60, + }, + key_pem=_OTHER_PRIVATE_PEM, + ) + with pytest.raises(IdTokenVerificationError): + await verify_id_token( + forged, discovery_url=DISCOVERY_URL, expected_audience="test-client" + ) + + +async def test_verify_id_token_rejects_unknown_kid(mock_idp): + now = int(time.time()) + token = _sign( + { + "iss": ISSUER, + "aud": "test-client", + "sub": "alice", + "iat": now, + "exp": now + 60, + }, + kid="not-in-jwks", + ) + with pytest.raises(IdTokenVerificationError, match="No JWKS key matches"): + await verify_id_token( + token, discovery_url=DISCOVERY_URL, expected_audience="test-client" + ) + + +async def test_verify_id_token_nonce_mismatch_rejected(mock_idp): + now = int(time.time()) + token = _sign( + { + "iss": ISSUER, + "aud": "test-client", + "sub": "alice", + "iat": now, + "exp": now + 60, + "nonce": "actual", + } + ) + with pytest.raises(IdTokenVerificationError, match="nonce"): + await verify_id_token( + token, + discovery_url=DISCOVERY_URL, + expected_audience="test-client", + expected_nonce="expected", + ) + + +async def test_verify_id_token_missing_token_rejected(): + with pytest.raises(IdTokenVerificationError, match="missing"): + await verify_id_token( + "", discovery_url=DISCOVERY_URL, expected_audience="test-client" + ) diff --git a/tests/unit/test_oauth_logout.py b/tests/unit/test_oauth_logout.py new file mode 100644 index 00000000..57925581 --- /dev/null +++ b/tests/unit/test_oauth_logout.py @@ -0,0 +1,331 @@ +"""Unit tests for OAuth logout (issue #626 finding 4) and the +SessionAuthBackend (finding 2). + +These cover the new server-side session lifecycle: + - logout deletes refresh token + browser session + - logout calls IdP revocation_endpoint when available + - logout still succeeds when IdP/storage errors + - SessionAuthBackend resolves random session_id -> user_id, fails + closed when the session is unknown / expired / has no refresh token +""" + +import json +import tempfile +from pathlib import Path +from types import SimpleNamespace +from unittest.mock import AsyncMock, MagicMock, patch + +import httpx +import pytest +from cryptography.fernet import Fernet +from starlette.requests import HTTPConnection + +from nextcloud_mcp_server.auth.browser_oauth_routes import ( + _revoke_refresh_token_at_idp, + oauth_logout, +) +from nextcloud_mcp_server.auth.session_backend import SessionAuthBackend +from nextcloud_mcp_server.auth.storage import RefreshTokenStorage + +pytestmark = pytest.mark.unit + + +# --------------------------------------------------------------------------- +# storage fixture (real SQLite backend; lighter than mocking every call) +# --------------------------------------------------------------------------- + + +@pytest.fixture +async def storage(): + with tempfile.TemporaryDirectory() as tmpdir: + db_path = Path(tmpdir) / "test_logout.db" + s = RefreshTokenStorage( + db_path=str(db_path), encryption_key=Fernet.generate_key().decode() + ) + await s.initialize() + yield s + + +def _build_request(*, cookie: str | None, oauth_context: dict | None): + """Build a minimal Starlette-style request stub for oauth_logout.""" + request = MagicMock() + request.query_params = {} + request.cookies = {"mcp_session": cookie} if cookie else {} + request.app.state.oauth_context = oauth_context + return request + + +# --------------------------------------------------------------------------- +# oauth_logout +# --------------------------------------------------------------------------- + + +async def test_logout_deletes_refresh_token_and_session(storage): + """Happy path: logout removes the refresh token and the browser session.""" + await storage.create_browser_session(session_id="sid-1", user_id="alice") + await storage.store_refresh_token( + user_id="alice", refresh_token="rt-abc", flow_type="browser" + ) + + request = _build_request( + cookie="sid-1", + oauth_context={"storage": storage, "discovery_url": None}, + ) + + with patch( + "nextcloud_mcp_server.auth.browser_oauth_routes._revoke_refresh_token_at_idp", + new=AsyncMock(), + ): + response = await oauth_logout(request) + + assert response.status_code == 302 + assert await storage.get_refresh_token("alice") is None + assert await storage.get_browser_session_user("sid-1") is None + + +async def test_logout_calls_revocation_when_refresh_token_present(storage): + """The IdP revocation helper is called with the stored refresh token.""" + await storage.create_browser_session(session_id="sid-2", user_id="bob") + await storage.store_refresh_token( + user_id="bob", refresh_token="rt-xyz", flow_type="browser" + ) + + revoke = AsyncMock() + request = _build_request( + cookie="sid-2", + oauth_context={"storage": storage, "discovery_url": "http://idp/.well-known"}, + ) + + with patch( + "nextcloud_mcp_server.auth.browser_oauth_routes._revoke_refresh_token_at_idp", + new=revoke, + ): + await oauth_logout(request) + + revoke.assert_awaited_once() + args = revoke.await_args.args + # Second arg is the refresh token string + assert args[1] == "rt-xyz" + + +async def test_logout_no_session_cookie_returns_302(storage): + """Without a cookie, logout still 302s and doesn't touch storage.""" + request = _build_request( + cookie=None, oauth_context={"storage": storage, "discovery_url": None} + ) + response = await oauth_logout(request) + assert response.status_code == 302 + + +async def test_logout_swallows_storage_errors(storage): + """Logout is best-effort — a storage failure must not 500 the response.""" + await storage.create_browser_session(session_id="sid-3", user_id="carol") + broken_storage = MagicMock() + broken_storage.get_browser_session_user = AsyncMock( + side_effect=RuntimeError("db down") + ) + broken_storage.delete_browser_session = AsyncMock() + + request = _build_request( + cookie="sid-3", + oauth_context={"storage": broken_storage, "discovery_url": None}, + ) + response = await oauth_logout(request) + assert response.status_code == 302 # logout still succeeds + + +async def test_logout_handles_session_with_no_refresh_token(storage): + """Cookie + session row exist but refresh token already gone — logout is idempotent.""" + await storage.create_browser_session(session_id="sid-4", user_id="dave") + + revoke = AsyncMock() + request = _build_request( + cookie="sid-4", + oauth_context={"storage": storage, "discovery_url": None}, + ) + with patch( + "nextcloud_mcp_server.auth.browser_oauth_routes._revoke_refresh_token_at_idp", + new=revoke, + ): + await oauth_logout(request) + + # Revoke not called — no token to revoke + revoke.assert_not_called() + # Browser session still cleared + assert await storage.get_browser_session_user("sid-4") is None + + +# --------------------------------------------------------------------------- +# _revoke_refresh_token_at_idp +# --------------------------------------------------------------------------- + + +def _httpx_handler(routes: dict[str, httpx.Response]): + def handler(request: httpx.Request) -> httpx.Response: + return routes.get(str(request.url), httpx.Response(404)) + + return handler + + +async def test_revoke_helper_posts_to_revocation_endpoint(): + discovery_url = "http://idp.example/.well-known" + revocation_url = "http://idp.example/revoke" + + received: list[httpx.Request] = [] + + def handler(request: httpx.Request) -> httpx.Response: + if str(request.url) == discovery_url: + return httpx.Response( + 200, + content=json.dumps({"revocation_endpoint": revocation_url}).encode(), + headers={"content-type": "application/json"}, + ) + if str(request.url) == revocation_url: + received.append(request) + return httpx.Response(200) + return httpx.Response(404) + + transport = httpx.MockTransport(handler) + + def fake_client(**kwargs): + kwargs["transport"] = transport + return httpx.AsyncClient(**kwargs) + + with patch( + "nextcloud_mcp_server.auth.browser_oauth_routes.nextcloud_httpx_client", + side_effect=fake_client, + ): + await _revoke_refresh_token_at_idp( + { + "discovery_url": discovery_url, + "client_id": "test-client", + "client_secret": "test-secret", + }, + "rt-secret", + ) + + assert len(received) == 1 + body = received[0].content.decode() + assert "token=rt-secret" in body + assert "token_type_hint=refresh_token" in body + + +async def test_revoke_helper_skips_when_no_revocation_endpoint(): + """IdPs without a revocation_endpoint advertised: helper must no-op silently.""" + discovery_url = "http://idp.example/.well-known" + + def handler(request: httpx.Request) -> httpx.Response: + if str(request.url) == discovery_url: + return httpx.Response(200, json={}) # no revocation_endpoint + return httpx.Response(404) + + transport = httpx.MockTransport(handler) + + def fake_client(**kwargs): + kwargs["transport"] = transport + return httpx.AsyncClient(**kwargs) + + with patch( + "nextcloud_mcp_server.auth.browser_oauth_routes.nextcloud_httpx_client", + side_effect=fake_client, + ): + # Returns None and does not raise + result = await _revoke_refresh_token_at_idp( + { + "discovery_url": discovery_url, + "client_id": "x", + "client_secret": "y", + }, + "rt", + ) + assert result is None + + +async def test_revoke_helper_silent_on_idp_error(): + """If the IdP 500s, the helper must not raise — caller treats it as best-effort.""" + + def handler(request: httpx.Request) -> httpx.Response: + return httpx.Response(500, content=b"boom") + + transport = httpx.MockTransport(handler) + + def fake_client(**kwargs): + kwargs["transport"] = transport + return httpx.AsyncClient(**kwargs) + + with patch( + "nextcloud_mcp_server.auth.browser_oauth_routes.nextcloud_httpx_client", + side_effect=fake_client, + ): + result = await _revoke_refresh_token_at_idp( + { + "discovery_url": "http://x/.well-known", + "client_id": "x", + "client_secret": "y", + }, + "rt", + ) + assert result is None + + +# --------------------------------------------------------------------------- +# SessionAuthBackend +# --------------------------------------------------------------------------- + + +def _build_conn(*, cookie: str | None, oauth_context: dict | None): + conn = MagicMock(spec=HTTPConnection) + conn.cookies = {"mcp_session": cookie} if cookie else {} + conn.url = SimpleNamespace(path="/app") + conn.app = MagicMock() + conn.app.state.oauth_context = oauth_context + return conn + + +async def test_session_backend_authenticates_known_session_with_token(storage): + await storage.create_browser_session(session_id="sid-A", user_id="alice") + await storage.store_refresh_token( + user_id="alice", refresh_token="rt", flow_type="browser" + ) + + backend = SessionAuthBackend(oauth_enabled=True) + conn = _build_conn(cookie="sid-A", oauth_context={"storage": storage}) + + result = await backend.authenticate(conn) + assert result is not None + creds, user = result + assert "authenticated" in creds.scopes + assert user.username == "alice" + + +async def test_session_backend_rejects_unknown_session(storage): + backend = SessionAuthBackend(oauth_enabled=True) + conn = _build_conn(cookie="not-a-real-sid", oauth_context={"storage": storage}) + assert await backend.authenticate(conn) is None + + +async def test_session_backend_rejects_session_without_refresh_token(storage): + """Defense-in-depth: session row exists but user has no refresh token.""" + await storage.create_browser_session(session_id="sid-B", user_id="bob") + # Note: NO refresh token stored for bob + + backend = SessionAuthBackend(oauth_enabled=True) + conn = _build_conn(cookie="sid-B", oauth_context={"storage": storage}) + assert await backend.authenticate(conn) is None + + +async def test_session_backend_rejects_when_no_cookie(storage): + backend = SessionAuthBackend(oauth_enabled=True) + conn = _build_conn(cookie=None, oauth_context={"storage": storage}) + assert await backend.authenticate(conn) is None + + +async def test_session_backend_basicauth_mode_short_circuits(monkeypatch, storage): + """In BasicAuth mode (oauth_enabled=False) the backend never touches storage.""" + monkeypatch.setenv("NEXTCLOUD_USERNAME", "admin-user") + backend = SessionAuthBackend(oauth_enabled=False) + conn = _build_conn(cookie=None, oauth_context=None) + result = await backend.authenticate(conn) + assert result is not None + _, user = result + assert user.username == "admin-user" diff --git a/tests/unit/test_oauth_tools_signatures.py b/tests/unit/test_oauth_tools_signatures.py new file mode 100644 index 00000000..cbeba12b --- /dev/null +++ b/tests/unit/test_oauth_tools_signatures.py @@ -0,0 +1,56 @@ +"""Unit tests for OAuth tool input-schema hardening (issue #626 finding 3). + +These tools must derive `user_id` from the verified MCP access token and +must never accept it as an MCP-level input. Otherwise an LLM (or any MCP +client) could supply an arbitrary user_id and reach cross-user revoke or +status-disclosure operations. +""" + +import pytest +from mcp.server.fastmcp import FastMCP + +from nextcloud_mcp_server.server.oauth_tools import register_oauth_tools + +pytestmark = pytest.mark.unit + + +HARDENED_TOOLS = ( + "provision_nextcloud_access", + "revoke_nextcloud_access", + "check_provisioning_status", + "check_logged_in", +) + + +@pytest.fixture +def registered_tools(): + """Register the OAuth tools against a fresh FastMCP and return them by name. + + Uses FastMCP's `_tool_manager.list_tools()`; flagged as internal and may + break on SDK upgrades, but this is the supported way to inspect a tool's + JSON input schema in unit tests (see tests/unit/test_stdio.py). + """ + mcp = FastMCP("test-oauth-tools") + register_oauth_tools(mcp) + tools = mcp._tool_manager.list_tools() + return {t.name: t for t in tools} + + +def test_oauth_tools_registered(registered_tools): + for name in HARDENED_TOOLS: + assert name in registered_tools, f"{name} should be registered" + + +@pytest.mark.parametrize("tool_name", HARDENED_TOOLS) +def test_oauth_tool_schema_does_not_accept_user_id(tool_name, registered_tools): + """user_id must not appear in the tool's JSON input schema.""" + tool = registered_tools[tool_name] + properties = tool.parameters.get("properties", {}) + required = tool.parameters.get("required", []) + + assert "user_id" not in properties, ( + f"{tool_name} accepts user_id as an MCP input — must be derived from " + f"the verified access token (issue #626 finding 3). " + f"properties={list(properties.keys())}" + ) + assert "user_id" not in required diff --git a/tests/unit/test_token_broker.py b/tests/unit/test_token_broker.py index 32105b2e..8aebcd1a 100644 --- a/tests/unit/test_token_broker.py +++ b/tests/unit/test_token_broker.py @@ -10,7 +10,6 @@ from datetime import datetime, timedelta, timezone from unittest.mock import AsyncMock, MagicMock, patch import httpx -import jwt import pytest from cryptography.fernet import Fernet @@ -283,23 +282,6 @@ class TestTokenBrokerService: # Verify cache was cleared assert await token_broker.cache.get("user1") is None - async def test_validate_token_audience(self, token_broker): - """Test token audience validation.""" - # Create test token with audience - test_payload = { - "sub": "user1", - "aud": ["nextcloud", "other-service"], - "exp": datetime.now(timezone.utc) + timedelta(hours=1), - } - test_token = jwt.encode(test_payload, "secret", algorithm="HS256") - - # Should not raise for correct audience - await token_broker._validate_token_audience(test_token, "nextcloud") - - # Should raise for wrong audience - with pytest.raises(ValueError, match="doesn't include wrong-audience"): - await token_broker._validate_token_audience(test_token, "wrong-audience") - async def test_token_refresh_with_network_error(self, token_broker, mock_storage): """Test handling network errors during token refresh.""" # Storage returns already-decrypted refresh token From 931ee602ebd4deacf50bf49a828df6fac9eb77f0 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sat, 2 May 2026 18:26:39 +0200 Subject: [PATCH 02/14] =?UTF-8?q?fix(auth):=20address=20PR=20#758=20review?= =?UTF-8?q?=20=E2=80=94=20XSS,=20CSRF,=20open=20redirect,=20JWKS=20cache?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses all 9 findings from the review on PR #758: Blocking: - _revoke_refresh_token_at_idp now reads config from oauth_ctx["config"] (the production-shaped nested dict). Previously read flat keys, causing IdP revocation to silently no-op in production. Test fixtures rebuilt to the realistic nested shape so the bug can't regress unnoticed. - HTML error responses in oauth_login_callback now wrap IdP-controlled error_body, str(e), and the attacker-controlled error/error_description query params in html_escape. New test_browser_oauth_xss.py pins this. Important: - New _safe_next_url helper validates the ?next= query param at write time (oauth_login), in oauth_logout, and on read from the session row in oauth_login_callback. Blocks https://, // (protocol-relative), and CRLF/whitespace injection. - verify_id_token now caches discovery + JWKS (5-min TTL) using the same pattern as oauth_routes._get_cached_discovery. New caching regression test pins to one fetch per URL across multiple calls. - /oauth/logout is now POST-only at the route layer (defeats passive CSRF via ). oauth_logout also validates Origin/Referer against the configured mcp_server_url. Logout UI in user_info.html converted from to
. - New storage.cleanup_expired_browser_sessions() called from the hourly cleanup loop in app.py — previously these rows accumulated for users who never explicitly logged out. Nits: - Demoted INFO logs that leaked oauth_config.keys() / client_id / token-storage state to DEBUG. Operator-relevant outcome lines (login successful, refresh token stored, logged out) stay INFO. - verify_id_token algorithms widened to RS256, PS256, ES256 — covers Azure AD (PS256) and Cognito/some Keycloak realms (ES256). Symmetric and "none" remain off the allowlist. - Migrated all Optional[X] usages in auth/storage.py to X | None per CLAUDE.md. Breaking change: GET /oauth/logout now returns 405. The in-tree logout UI was migrated to a POST form; any external bookmark or curl-based caller that relied on GET will need to switch. Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/app.py | 11 +- .../auth/browser_oauth_routes.py | 161 +++++++++++++----- nextcloud_mcp_server/auth/storage.py | 81 ++++++--- .../auth/templates/user_info.html | 20 ++- nextcloud_mcp_server/auth/token_utils.py | 59 +++++-- tests/unit/test_browser_oauth_xss.py | 121 +++++++++++++ tests/unit/test_browser_sessions.py | 24 +++ tests/unit/test_id_token_verification.py | 56 ++++++ tests/unit/test_oauth_logout.py | 114 +++++++++++-- tests/unit/test_safe_next_url.py | 44 +++++ 10 files changed, 581 insertions(+), 110 deletions(-) create mode 100644 tests/unit/test_browser_oauth_xss.py create mode 100644 tests/unit/test_safe_next_url.py diff --git a/nextcloud_mcp_server/app.py b/nextcloud_mcp_server/app.py index 68fd08bd..c32ba904 100644 --- a/nextcloud_mcp_server/app.py +++ b/nextcloud_mcp_server/app.py @@ -1354,13 +1354,16 @@ def get_app(transport: str = "streamable-http", enabled_apps: list[str] | None = storage = await get_shared_storage() count = await storage.delete_expired_login_flow_sessions() if count: - logger.info(f"Cleaned up {count} expired login flow sessions") + logger.info("Cleaned up %s expired login flow sessions", count) + # Browser session rows are otherwise only cleaned up lazily + # when a user revisits — PR #758 finding 6. + await storage.cleanup_expired_browser_sessions() # Also clean up expired AS proxy codes/sessions _cleanup_expired_proxy_codes() # Clean up expired web provision sessions _cleanup_expired_provision_sessions() except Exception as e: - logger.warning(f"Login flow cleanup error: {e}") + logger.warning("Login flow cleanup error: %s", e) await anyio.sleep(3600) # Every hour @asynccontextmanager @@ -2242,8 +2245,10 @@ def get_app(transport: str = "streamable-http", enabled_apps: list[str] | None = name="oauth_login_callback", ) ) + # POST-only: defends against passive CSRF (e.g. ) + # — see PR #758 finding 5. routes.append( - Route("/oauth/logout", oauth_logout, methods=["GET"], name="oauth_logout") + Route("/oauth/logout", oauth_logout, methods=["POST"], name="oauth_logout") ) logger.info( "Browser OAuth routes enabled: /oauth/login, /oauth/login-callback (legacy), /oauth/logout" diff --git a/nextcloud_mcp_server/auth/browser_oauth_routes.py b/nextcloud_mcp_server/auth/browser_oauth_routes.py index 577c3024..cc957242 100644 --- a/nextcloud_mcp_server/auth/browser_oauth_routes.py +++ b/nextcloud_mcp_server/auth/browser_oauth_routes.py @@ -32,6 +32,48 @@ from ..http import nextcloud_httpx_client logger = logging.getLogger(__name__) +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: + - 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``. + """ + cfg = oauth_ctx.get("config") or oauth_ctx + mcp_server_url = cfg.get("mcp_server_url") + if not mcp_server_url: + # Mis-configured deployment — fail open rather than break logout. + return True + + expected = parse_url(mcp_server_url).netloc.lower() + raw = request.headers.get("origin") or request.headers.get("referer") + if not raw: + return True + return parse_url(raw).netloc.lower() == expected + + +def _safe_next_url(raw: str | None, default: str) -> str: + """Return a path-only redirect target, falling back to *default*. + + Blocks open-redirect abuse via the ``?next=`` query parameter on + ``/oauth/login`` and ``/oauth/logout`` (and the round-tripped + ``client_redirect_uri`` stored on the oauth_session). A safe target: + - starts with a single ``/`` (so it's a path on this server) + - does NOT start with ``//`` (which would be protocol-relative) + - has no whitespace or control characters that could trick browsers + + Anything else returns *default*. + """ + if not raw or not raw.startswith("/") or raw.startswith("//"): + return default + if any(c.isspace() or ord(c) < 0x20 for c in raw): + return default + return raw + + def _should_use_secure_cookies() -> bool: """Determine if cookies should have secure flag. @@ -73,14 +115,17 @@ async def oauth_login(request: Request) -> RedirectResponse | JSONResponse: oauth_client = oauth_ctx["oauth_client"] oauth_config = oauth_ctx["config"] - # Debug: Log oauth_config contents - logger.info(f"oauth_login called - oauth_config keys: {oauth_config.keys()}") - logger.info(f"oauth_login called - client_id: {oauth_config.get('client_id')}") - logger.info(f"oauth_login called - oauth_client: {oauth_client is not None}") + # Demoted to DEBUG (PR #758 nit a) — these previously leaked the + # full set of config keys + the client_id at INFO on every login. + logger.debug("oauth_login called - oauth_config keys: %s", oauth_config.keys()) + logger.debug("oauth_login called - client_id: %s", oauth_config.get("client_id")) + logger.debug("oauth_login called - oauth_client: %s", oauth_client is not None) - # Get redirect URL from query params (default to /app) - next_url = request.query_params.get("next", "/app") - logger.info(f"oauth_login - next_url: {next_url}") + # Get redirect URL from query params (default to /app). Validated at + # write-time so we never store an attacker-controlled absolute URL on + # the oauth_session row (issue #758 finding 3). + next_url = _safe_next_url(request.query_params.get("next"), "/app") + logger.debug("oauth_login - next_url: %s", next_url) # Generate state for CSRF protection state = secrets.token_urlsafe(32) @@ -142,7 +187,7 @@ async def oauth_login(request: Request) -> RedirectResponse | JSONResponse: } auth_url = f"{oauth_client.authorization_endpoint}?{urlencode(idp_params)}" - logger.info(f"Redirecting to external IdP login: {auth_url.split('?')[0]}") + logger.debug("Redirecting to external IdP login: %s", auth_url.split("?")[0]) else: # Integrated mode (Nextcloud OIDC) discovery_url = oauth_config.get("discovery_url") @@ -199,11 +244,10 @@ async def oauth_login(request: Request) -> RedirectResponse | JSONResponse: "resource": nextcloud_resource_uri, # Request tokens for Nextcloud API access } - # Debug: Log full parameters - logger.info(f"Building Nextcloud OIDC auth URL with params: {idp_params}") + logger.debug("Building Nextcloud OIDC auth URL with params: %s", idp_params) auth_url = f"{authorization_endpoint}?{urlencode(idp_params)}" - logger.info(f"Redirecting to Nextcloud OIDC login: {auth_url}") + logger.debug("Redirecting to Nextcloud OIDC login: %s", auth_url) return RedirectResponse(auth_url, status_code=302) @@ -228,8 +272,10 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo error_description = request.query_params.get( "error_description", "Authorization failed" ) - logger.error(f"OAuth login error: {error} - {error_description}") + logger.error("OAuth login error: %s - %s", error, error_description) login_url = str(request.url_for("oauth_login")) + # html_escape: error / error_description come from attacker-controlled + # query parameters and would otherwise reflect into the failure page. return HTMLResponse( f""" @@ -237,9 +283,9 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo Login Failed

Login Failed

-

Error: {error}

-

{error_description}

-

Try again

+

Error: {html_escape(error)}

+

{html_escape(error_description)}

+

Try again

""", @@ -278,8 +324,10 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo if oauth_session: # code_verifier was stored in mcp_authorization_code field code_verifier = oauth_session.get("mcp_authorization_code", "") - # next_url was stored in client_redirect_uri field - next_url = oauth_session.get("client_redirect_uri", "/app") + # next_url was stored in client_redirect_uri field — re-validate at + # 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 @@ -347,8 +395,10 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo e.response.text if hasattr(e.response, "text") else str(e.response.content) ) logger.error( - f"Token exchange failed: HTTP {e.response.status_code} - {error_body}" + "Token exchange failed: HTTP %s - %s", e.response.status_code, error_body ) + # html_escape: error_body originates from the IdP and could contain + # markup that would be reflected into the failure page otherwise. return HTMLResponse( f""" @@ -357,14 +407,14 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo

Login Failed

Failed to exchange authorization code for tokens

-

HTTP {e.response.status_code}: {error_body}

+

HTTP {e.response.status_code}: {html_escape(error_body)}

""", status_code=500, ) except Exception as e: - logger.error(f"Token exchange failed: {e}") + logger.error("Token exchange failed: %s", e) return HTMLResponse( f""" @@ -373,7 +423,7 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo

Login Failed

Failed to exchange authorization code for tokens

-

Error: {e}

+

Error: {html_escape(str(e))}

""", @@ -383,9 +433,11 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo refresh_token = token_data.get("refresh_token") id_token = token_data.get("id_token") - logger.info(f"Token exchange response keys: {token_data.keys()}") - logger.info(f"Refresh token present: {refresh_token is not None}") - logger.info(f"ID token present: {id_token is not None}") + # Demoted to DEBUG (PR #758 nit a) — these were previously logged at + # INFO on every login. + logger.debug("Token exchange response keys: %s", token_data.keys()) + logger.debug("Refresh token present: %s", refresh_token is not None) + logger.debug("ID token present: %s", id_token is not None) # Resolve the discovery URL + audience used for THIS auth request so # we can verify the ID token signature + claims (issue #626 finding 1). @@ -431,8 +483,10 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo refresh_expires_at = None if refresh_expires_in: refresh_expires_at = int(time.time()) + refresh_expires_in - logger.info( - f"Refresh token expires in {refresh_expires_in}s (at timestamp {refresh_expires_at})" + logger.debug( + "Refresh token expires in %ss (at timestamp %s)", + refresh_expires_in, + refresh_expires_at, ) # Extract granted scopes @@ -442,10 +496,13 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo # Store refresh token (for background jobs ONLY) if refresh_token: - logger.info(f"Storing refresh token for user_id: {user_id}") - logger.info(f" State parameter (provisioning_client_id): {state[:16]}...") - logger.info(f" Granted scopes: {granted_scopes}") - logger.info(f" Expires at: {refresh_expires_at}") + logger.debug( + "Storing refresh token for user_id=%s state=%s... scopes=%s expires_at=%s", + user_id, + state[:16], + granted_scopes, + refresh_expires_at, + ) await storage.store_refresh_token( user_id=user_id, refresh_token=refresh_token, @@ -454,9 +511,10 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo provisioning_client_id=state, # Store state for unified session lookup scopes=granted_scopes, ) - logger.info(f"✓ Refresh token stored successfully for user_id: {user_id}") logger.info( - f" Token can now be found via provisioning_client_id={state[:16]}..." + "Refresh token stored for user %s (lookup key: %s...)", + user_id, + state[:16], ) else: logger.warning("No refresh token in token response - cannot store session") @@ -478,13 +536,13 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo if profile_data: # Cache profile for browser UI (no token needed to display) await storage.store_user_profile(user_id, profile_data) - logger.info(f"✓ User profile cached for {user_id}") + logger.debug("User profile cached for %s", user_id) else: - logger.warning(f"Failed to query userinfo endpoint for {user_id}") + logger.warning("Failed to query userinfo endpoint for %s", user_id) else: logger.warning("Could not determine userinfo endpoint") except Exception as e: - logger.error(f"Error caching user profile: {e}") + logger.error("Error caching user profile: %s", e) # Continue anyway - profile cache is optional for browser UI # Create a server-side browser session: a random opaque session_id is @@ -510,7 +568,7 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo return response -async def oauth_logout(request: Request) -> RedirectResponse: +async def oauth_logout(request: Request) -> RedirectResponse | JSONResponse: """Browser OAuth logout — invalidate session and revoke refresh token. Issue #626 finding 4: prior implementation only cleared the cookie, @@ -523,13 +581,30 @@ async def oauth_logout(request: Request) -> RedirectResponse: if it leaks. 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. + Query parameters: next: Optional URL to redirect to after logout (default: /oauth/login) """ - next_url = request.query_params.get("next", "/oauth/login") + next_url = _safe_next_url(request.query_params.get("next"), "/oauth/login") session_id = request.cookies.get("mcp_session") oauth_ctx = getattr(request.app.state, "oauth_context", None) + + # CSRF check: when Origin or Referer is present, host must match the + # MCP server's own host. Per OWASP CSRF cheat sheet, we allow the + # request through when neither header is present (some user agents + # strip both for privacy on same-origin POST). + if oauth_ctx and not _origin_matches_self(request, oauth_ctx): + logger.warning( + "Logout blocked: cross-origin request from %s", + request.headers.get("origin") or request.headers.get("referer"), + ) + return JSONResponse({"error": "forbidden"}, status_code=403) + storage = oauth_ctx.get("storage") if oauth_ctx else None if session_id and storage and oauth_ctx: @@ -563,8 +638,12 @@ async def _revoke_refresh_token_at_idp(oauth_ctx: dict, refresh_token: str) -> N of deleting the local copy, and we don't want logout to error if the IdP is unreachable or doesn't advertise a revocation endpoint. """ + # Production oauth_context nests config under "config" (see app.py + # starlette_lifespan). A flat shape is also accepted for tests and + # historical callers. + cfg = oauth_ctx.get("config") or oauth_ctx try: - discovery_url = oauth_ctx.get("discovery_url") or os.getenv( + discovery_url = cfg.get("discovery_url") or os.getenv( "OIDC_DISCOVERY_URL", f"{os.getenv('NEXTCLOUD_HOST', '')}/.well-known/openid-configuration", ) @@ -580,10 +659,8 @@ async def _revoke_refresh_token_at_idp(oauth_ctx: dict, refresh_token: str) -> N logger.debug("IdP advertises no revocation_endpoint; skipping") return - client_id = oauth_ctx.get("client_id") or os.getenv("OIDC_CLIENT_ID") - client_secret = oauth_ctx.get("client_secret") or os.getenv( - "OIDC_CLIENT_SECRET" - ) + client_id = cfg.get("client_id") or os.getenv("OIDC_CLIENT_ID") + client_secret = cfg.get("client_secret") or os.getenv("OIDC_CLIENT_SECRET") if not (client_id and client_secret): logger.debug("No OIDC client credentials available for revocation") return diff --git a/nextcloud_mcp_server/auth/storage.py b/nextcloud_mcp_server/auth/storage.py index 38da1d11..a9fc9a33 100644 --- a/nextcloud_mcp_server/auth/storage.py +++ b/nextcloud_mcp_server/auth/storage.py @@ -31,7 +31,7 @@ import os import socket import time from pathlib import Path -from typing import Any, Optional +from typing import Any import aiosqlite import anyio @@ -205,11 +205,11 @@ class RefreshTokenStorage: self, user_id: str, refresh_token: str, - expires_at: Optional[int] = None, + expires_at: int | None = None, flow_type: str = "hybrid", token_audience: str = "nextcloud", - provisioning_client_id: Optional[str] = None, - scopes: Optional[list[str]] = None, + provisioning_client_id: str | None = None, + scopes: list[str] | None = None, ) -> None: """ Store encrypted refresh token for user. @@ -313,7 +313,7 @@ class RefreshTokenStorage: logger.debug(f"Cached user profile for {user_id}") - async def get_user_profile(self, user_id: str) -> Optional[dict[str, Any]]: + async def get_user_profile(self, user_id: str) -> dict[str, Any] | None: """ Retrieve cached user profile data. @@ -351,7 +351,7 @@ class RefreshTokenStorage: return profile_data - async def get_refresh_token(self, user_id: str) -> Optional[dict]: + async def get_refresh_token(self, user_id: str) -> dict | None: """ Retrieve and decrypt refresh token for user. @@ -444,7 +444,7 @@ class RefreshTokenStorage: async def get_refresh_token_by_provisioning_client_id( self, provisioning_client_id: str - ) -> Optional[dict]: + ) -> dict | None: """ Retrieve and decrypt refresh token by provisioning_client_id (state parameter). @@ -617,8 +617,8 @@ class RefreshTokenStorage: client_id_issued_at: int, client_secret_expires_at: int, redirect_uris: list[str], - registration_access_token: Optional[str] = None, - registration_client_uri: Optional[str] = None, + registration_access_token: str | None = None, + registration_client_uri: str | None = None, ) -> None: """ Store encrypted OAuth client credentials. @@ -689,7 +689,7 @@ class RefreshTokenStorage: auth_method="oauth", ) - async def get_oauth_client(self) -> Optional[dict]: + async def get_oauth_client(self) -> dict | None: """ Retrieve and decrypt OAuth client credentials. @@ -827,9 +827,9 @@ class RefreshTokenStorage: self, event: str, user_id: str, - resource_type: Optional[str] = None, - resource_id: Optional[str] = None, - auth_method: Optional[str] = None, + resource_type: str | None = None, + resource_id: str | None = None, + auth_method: str | None = None, ) -> None: """ Log operation to audit log. @@ -866,8 +866,8 @@ class RefreshTokenStorage: async def get_audit_logs( self, - user_id: Optional[str] = None, - since: Optional[int] = None, + user_id: str | None = None, + since: int | None = None, limit: int = 100, ) -> list[dict]: """ @@ -909,14 +909,14 @@ class RefreshTokenStorage: self, session_id: str, client_redirect_uri: str, - state: Optional[str] = None, - code_challenge: Optional[str] = None, - code_challenge_method: Optional[str] = None, - mcp_authorization_code: Optional[str] = None, - client_id: Optional[str] = None, + state: str | None = None, + code_challenge: str | None = None, + code_challenge_method: str | None = None, + mcp_authorization_code: str | None = None, + client_id: str | None = None, flow_type: str = "hybrid", is_provisioning: bool = False, - requested_scopes: Optional[str] = None, + requested_scopes: str | None = None, ttl_seconds: int = 600, # 10 minutes ) -> None: """ @@ -969,7 +969,7 @@ class RefreshTokenStorage: logger.debug(f"Stored OAuth session {session_id} (expires in {ttl_seconds}s)") - async def get_oauth_session(self, session_id: str) -> Optional[dict]: + async def get_oauth_session(self, session_id: str) -> dict | None: """ Retrieve OAuth session by session ID. @@ -1001,7 +1001,7 @@ class RefreshTokenStorage: async def get_oauth_session_by_mcp_code( self, mcp_authorization_code: str - ) -> Optional[dict]: + ) -> dict | None: """ Retrieve OAuth session by MCP authorization code. @@ -1037,9 +1037,9 @@ class RefreshTokenStorage: async def update_oauth_session( self, session_id: str, - user_id: Optional[str] = None, - idp_access_token: Optional[str] = None, - idp_refresh_token: Optional[str] = None, + user_id: str | None = None, + idp_access_token: str | None = None, + idp_refresh_token: str | None = None, ) -> bool: """ Update OAuth session with IdP token data. @@ -1174,7 +1174,7 @@ class RefreshTokenStorage: ttl_seconds, ) - async def get_browser_session_user(self, session_id: str) -> Optional[str]: + async def get_browser_session_user(self, session_id: str) -> str | None: """Look up the user_id bound to a browser session_id, or None. Returns None when the session is unknown or expired. Expired rows @@ -1217,6 +1217,31 @@ class RefreshTokenStorage: logger.debug("Deleted browser session %s", session_id[:8]) return deleted + async def cleanup_expired_browser_sessions(self) -> int: + """Remove expired ``browser_sessions`` rows. + + Returns the number of rows deleted. Called by the periodic cleanup + task in ``app.py``. Without this users who never explicitly log out + leave session rows behind that only get deleted lazily on lookup + (PR #758 finding 6). + """ + if not self._initialized: + await self.initialize() + + now = int(time.time()) + + async with aiosqlite.connect(self.db_path) as db: + cursor = await db.execute( + "DELETE FROM browser_sessions WHERE expires_at < ?", (now,) + ) + await db.commit() + deleted = cursor.rowcount + + if deleted > 0: + logger.info("Cleaned up %s expired browser session(s)", deleted) + + return deleted + # ============================================================================ # Webhook Registration Tracking (both BasicAuth and OAuth modes) # ============================================================================ @@ -1396,7 +1421,7 @@ class RefreshTokenStorage: auth_method="app_password", ) - async def get_app_password(self, user_id: str) -> Optional[str]: + async def get_app_password(self, user_id: str) -> str | None: """ Retrieve and decrypt app password for a user. diff --git a/nextcloud_mcp_server/auth/templates/user_info.html b/nextcloud_mcp_server/auth/templates/user_info.html index 25f527b6..39b8826a 100644 --- a/nextcloud_mcp_server/auth/templates/user_info.html +++ b/nextcloud_mcp_server/auth/templates/user_info.html @@ -285,14 +285,18 @@
  • - - - - - - - Logout - + {# Logout is POST-only to defeat CSRF (PR #758 finding 5). + Style this +
diff --git a/nextcloud_mcp_server/auth/token_utils.py b/nextcloud_mcp_server/auth/token_utils.py index 51020ec1..a204810e 100644 --- a/nextcloud_mcp_server/auth/token_utils.py +++ b/nextcloud_mcp_server/auth/token_utils.py @@ -5,6 +5,7 @@ between server/ and auth/ layers. """ import logging +import time from typing import Any import jwt @@ -18,10 +19,37 @@ from ..http import nextcloud_httpx_client logger = logging.getLogger(__name__) +# OIDC discovery + JWKS caches keyed by URL → (expires_at, data). Mirrors the +# pattern in oauth_routes._get_cached_discovery so that ID-token verification +# during the OAuth callback doesn't make two extra round-trips per login (PR +# #758 finding 4). 5-minute TTL matches oauth_routes. +_discovery_cache: dict[str, tuple[float, dict[str, Any]]] = {} +_jwks_cache: dict[str, tuple[float, dict[str, Any]]] = {} +_OIDC_CACHE_TTL = 300 + + class IdTokenVerificationError(Exception): """Raised when an OIDC ID token fails signature or claim verification.""" +async def _get_cached( + cache: dict[str, tuple[float, dict[str, Any]]], url: str +) -> dict[str, Any]: + """Return cached JSON response for *url* or fetch + cache on miss/expiry.""" + 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() 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 + + async def verify_id_token( id_token: str, *, @@ -62,21 +90,16 @@ async def verify_id_token( raise IdTokenVerificationError("ID token missing from token response") try: - async with nextcloud_httpx_client() as http_client: - discovery_response = await http_client.get(discovery_url) - discovery_response.raise_for_status() - discovery = discovery_response.json() + discovery = await _get_cached(_discovery_cache, discovery_url) - issuer = discovery.get("issuer") - jwks_uri = discovery.get("jwks_uri") - if not issuer or not jwks_uri: - raise IdTokenVerificationError( - "OIDC discovery response missing issuer or jwks_uri" - ) + issuer = discovery.get("issuer") + jwks_uri = discovery.get("jwks_uri") + if not issuer or not jwks_uri: + raise IdTokenVerificationError( + "OIDC discovery response missing issuer or jwks_uri" + ) - jwks_response = await http_client.get(jwks_uri) - jwks_response.raise_for_status() - jwks_data = jwks_response.json() + jwks_data = await _get_cached(_jwks_cache, jwks_uri) except IdTokenVerificationError: raise except Exception as e: @@ -97,10 +120,18 @@ async def verify_id_token( f"No JWKS key matches ID token kid {kid!r}" ) from e + # PyJWT verifies the JWT with the algorithm declared in its header, + # cross-checked against this allowlist (so an attacker can't downgrade + # to ``none`` or HMAC). The allowlist covers the OIDC algorithms + # most cloud IdPs ship by default: + # - RS256: Nextcloud user_oidc, Keycloak default, Auth0, Google. + # - PS256: Azure AD on newer keys. + # - ES256: some Keycloak realms, AWS Cognito user pools. + # Symmetric (HSxxx) and ``none`` are intentionally absent. payload: dict[str, Any] = jwt.decode( id_token, signing_key.key, - algorithms=["RS256"], + algorithms=["RS256", "PS256", "ES256"], audience=expected_audience, issuer=issuer, options={ diff --git a/tests/unit/test_browser_oauth_xss.py b/tests/unit/test_browser_oauth_xss.py new file mode 100644 index 00000000..13903dcf --- /dev/null +++ b/tests/unit/test_browser_oauth_xss.py @@ -0,0 +1,121 @@ +"""Regression tests for HTML XSS in browser OAuth error responses. + +The reviewer on PR #758 flagged that ``oauth_login_callback`` interpolated +IdP-controlled and query-parameter-controlled text into HTMLResponse bodies +without escaping. These tests pin the html_escape behavior so the +vulnerability cannot regress silently. +""" + +import json +import tempfile +from pathlib import Path +from unittest.mock import MagicMock, patch + +import httpx +import pytest +from cryptography.fernet import Fernet + +from nextcloud_mcp_server.auth.browser_oauth_routes import oauth_login_callback +from nextcloud_mcp_server.auth.storage import RefreshTokenStorage + +pytestmark = pytest.mark.unit + + +XSS_PAYLOAD = "" + + +@pytest.fixture +async def storage(): + with tempfile.TemporaryDirectory() as tmpdir: + db_path = Path(tmpdir) / "xss.db" + s = RefreshTokenStorage( + db_path=str(db_path), encryption_key=Fernet.generate_key().decode() + ) + await s.initialize() + yield s + + +def _build_request(*, query_params: dict, oauth_context: dict | None = None): + request = MagicMock() + request.query_params = query_params + request.cookies = {} + request.app.state.oauth_context = oauth_context + request.url_for = MagicMock(return_value="/oauth/login") + return request + + +async def test_callback_escapes_error_query_params(storage): + """`error` and `error_description` are attacker-controlled — must be escaped.""" + request = _build_request( + query_params={ + "error": XSS_PAYLOAD, + "error_description": XSS_PAYLOAD, + }, + oauth_context={"storage": storage, "config": {}}, + ) + + response = await oauth_login_callback(request) + body = response.body.decode() + + assert XSS_PAYLOAD not in body + assert "<script>alert(1)</script>" in body + + +async def test_callback_escapes_idp_http_error_body(storage): + """IdP-returned HTTPError body must be HTML-escaped before reflection.""" + discovery = {"token_endpoint": "http://idp.example/token"} + + def handler(request: httpx.Request) -> httpx.Response: + if request.url.path.endswith("/.well-known/openid-configuration"): + return httpx.Response( + 200, + content=json.dumps(discovery).encode(), + headers={"content-type": "application/json"}, + ) + if str(request.url) == "http://idp.example/token": + return httpx.Response(400, content=XSS_PAYLOAD.encode()) + return httpx.Response(404) + + transport = httpx.MockTransport(handler) + + def fake_client(**kwargs): + kwargs["transport"] = transport + return httpx.AsyncClient(**kwargs) + + # Pre-populate the oauth_session row that the callback expects + await storage.store_oauth_session( + session_id="state-xss", + client_id="browser-ui", + client_redirect_uri="/app", + state="state-xss", + code_challenge="cc", + code_challenge_method="S256", + mcp_authorization_code="cv", + flow_type="browser", + ttl_seconds=600, + ) + + request = _build_request( + query_params={"code": "abc", "state": "state-xss"}, + oauth_context={ + "storage": storage, + "oauth_client": None, + "config": { + "discovery_url": "http://idp.example/.well-known/openid-configuration", + "client_id": "test", + "client_secret": "secret", + "mcp_server_url": "http://localhost", + }, + }, + ) + + with patch( + "nextcloud_mcp_server.auth.browser_oauth_routes.nextcloud_httpx_client", + side_effect=fake_client, + ): + response = await oauth_login_callback(request) + + body = response.body.decode() + assert response.status_code == 500 + assert XSS_PAYLOAD not in body + assert "<script>alert(1)</script>" in body diff --git a/tests/unit/test_browser_sessions.py b/tests/unit/test_browser_sessions.py index 7851d933..65ac5db8 100644 --- a/tests/unit/test_browser_sessions.py +++ b/tests/unit/test_browser_sessions.py @@ -73,3 +73,27 @@ async def test_replace_existing_session_id(storage): await storage.create_browser_session(session_id=sid, user_id="bob") assert await storage.get_browser_session_user(sid) == "bob" + + +async def test_cleanup_expired_browser_sessions(storage): + """Periodic cleanup removes expired rows but leaves fresh ones (PR #758 finding 6).""" + fresh_sid = secrets.token_urlsafe(32) + expired_sid = secrets.token_urlsafe(32) + + await storage.create_browser_session( + session_id=fresh_sid, user_id="alice", ttl_seconds=3600 + ) + # ttl_seconds=-2 → expires_at strictly in the past (cleanup uses < now, + # so it must be actually less, not equal). + await storage.create_browser_session( + session_id=expired_sid, user_id="bob", ttl_seconds=-2 + ) + + deleted = await storage.cleanup_expired_browser_sessions() + assert deleted == 1 + + # Fresh row survives, expired row is gone + assert await storage.get_browser_session_user(fresh_sid) == "alice" + assert await storage.get_browser_session_user(expired_sid) is None + # Calling again should be a no-op + assert await storage.cleanup_expired_browser_sessions() == 0 diff --git a/tests/unit/test_id_token_verification.py b/tests/unit/test_id_token_verification.py index 84ed854a..ee43d027 100644 --- a/tests/unit/test_id_token_verification.py +++ b/tests/unit/test_id_token_verification.py @@ -18,6 +18,7 @@ import pytest from cryptography.hazmat.primitives import serialization from cryptography.hazmat.primitives.asymmetric import rsa +from nextcloud_mcp_server.auth import token_utils from nextcloud_mcp_server.auth.token_utils import ( IdTokenVerificationError, verify_id_token, @@ -26,6 +27,16 @@ from nextcloud_mcp_server.auth.token_utils import ( pytestmark = pytest.mark.unit +@pytest.fixture(autouse=True) +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() + yield + token_utils._discovery_cache.clear() + token_utils._jwks_cache.clear() + + # Generated once per process — RSA keypair generation is slow. _KEY = rsa.generate_private_key(public_exponent=65537, key_size=2048) _PRIVATE_PEM = _KEY.private_bytes( @@ -231,3 +242,48 @@ async def test_verify_id_token_missing_token_rejected(): await verify_id_token( "", discovery_url=DISCOVERY_URL, expected_audience="test-client" ) + + +async def test_verify_id_token_caches_discovery_and_jwks(): + """Discovery + JWKS must be cached: two verifications, one fetch each. + + Pins the fix for PR #758 finding 4 — every login previously made two + extra HTTP round-trips to the IdP for the same metadata. + """ + fetches: dict[str, int] = {} + + def counting_handler(request: httpx.Request) -> httpx.Response: + url = str(request.url) + fetches[url] = fetches.get(url, 0) + 1 + return _idp_handler(request) + + transport = httpx.MockTransport(counting_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, + } + ) + + with patch( + "nextcloud_mcp_server.auth.token_utils.nextcloud_httpx_client", + side_effect=fake_client, + ): + await verify_id_token( + token, discovery_url=DISCOVERY_URL, expected_audience="test-client" + ) + await verify_id_token( + token, discovery_url=DISCOVERY_URL, expected_audience="test-client" + ) + + assert fetches.get(DISCOVERY_URL) == 1, "discovery fetched more than once" + assert fetches.get(JWKS_URI) == 1, "JWKS fetched more than once" diff --git a/tests/unit/test_oauth_logout.py b/tests/unit/test_oauth_logout.py index 57925581..d036c491 100644 --- a/tests/unit/test_oauth_logout.py +++ b/tests/unit/test_oauth_logout.py @@ -46,12 +46,20 @@ async def storage(): yield s -def _build_request(*, cookie: str | None, oauth_context: dict | None): +def _build_request( + *, + cookie: str | None, + oauth_context: dict | None, + headers: dict | None = None, +): """Build a minimal Starlette-style request stub for oauth_logout.""" request = MagicMock() request.query_params = {} request.cookies = {"mcp_session": cookie} if cookie else {} request.app.state.oauth_context = oauth_context + # Headers default to empty so the CSRF check sees neither Origin nor + # Referer (allowed by policy — see _origin_matches_self). + request.headers = headers or {} return request @@ -69,7 +77,7 @@ async def test_logout_deletes_refresh_token_and_session(storage): request = _build_request( cookie="sid-1", - oauth_context={"storage": storage, "discovery_url": None}, + oauth_context={"storage": storage, "config": {"discovery_url": None}}, ) with patch( @@ -93,7 +101,10 @@ async def test_logout_calls_revocation_when_refresh_token_present(storage): revoke = AsyncMock() request = _build_request( cookie="sid-2", - oauth_context={"storage": storage, "discovery_url": "http://idp/.well-known"}, + oauth_context={ + "storage": storage, + "config": {"discovery_url": "http://idp/.well-known"}, + }, ) with patch( @@ -111,7 +122,8 @@ async def test_logout_calls_revocation_when_refresh_token_present(storage): async def test_logout_no_session_cookie_returns_302(storage): """Without a cookie, logout still 302s and doesn't touch storage.""" request = _build_request( - cookie=None, oauth_context={"storage": storage, "discovery_url": None} + cookie=None, + oauth_context={"storage": storage, "config": {"discovery_url": None}}, ) response = await oauth_logout(request) assert response.status_code == 302 @@ -128,12 +140,78 @@ async def test_logout_swallows_storage_errors(storage): request = _build_request( cookie="sid-3", - oauth_context={"storage": broken_storage, "discovery_url": None}, + oauth_context={ + "storage": broken_storage, + "config": {"discovery_url": None}, + }, ) response = await oauth_logout(request) assert response.status_code == 302 # logout still succeeds +async def test_logout_blocks_cross_origin_post(storage): + """POST from a foreign Origin must be rejected with 403 (PR #758 finding 5).""" + await storage.create_browser_session(session_id="sid-X", user_id="alice") + + request = _build_request( + cookie="sid-X", + oauth_context={ + "storage": storage, + "config": { + "mcp_server_url": "https://mcp.example.com", + "discovery_url": None, + }, + }, + headers={"origin": "https://evil.example.com"}, + ) + + response = await oauth_logout(request) + assert response.status_code == 403 + # Session row must NOT have been deleted. + assert await storage.get_browser_session_user("sid-X") == "alice" + + +async def test_logout_allows_same_origin_post(storage): + """POST with matching Origin proceeds normally.""" + await storage.create_browser_session(session_id="sid-Y", user_id="alice") + + request = _build_request( + cookie="sid-Y", + oauth_context={ + "storage": storage, + "config": { + "mcp_server_url": "https://mcp.example.com", + "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-Y") is None + + +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") + + request = _build_request( + cookie="sid-Z", + oauth_context={ + "storage": storage, + "config": { + "mcp_server_url": "https://mcp.example.com", + "discovery_url": None, + }, + }, + headers={"referer": "https://mcp.example.com/app"}, + ) + + response = await oauth_logout(request) + assert response.status_code == 302 + + async def test_logout_handles_session_with_no_refresh_token(storage): """Cookie + session row exist but refresh token already gone — logout is idempotent.""" await storage.create_browser_session(session_id="sid-4", user_id="dave") @@ -141,7 +219,7 @@ async def test_logout_handles_session_with_no_refresh_token(storage): revoke = AsyncMock() request = _build_request( cookie="sid-4", - oauth_context={"storage": storage, "discovery_url": None}, + oauth_context={"storage": storage, "config": {"discovery_url": None}}, ) with patch( "nextcloud_mcp_server.auth.browser_oauth_routes._revoke_refresh_token_at_idp", @@ -197,9 +275,11 @@ async def test_revoke_helper_posts_to_revocation_endpoint(): ): await _revoke_refresh_token_at_idp( { - "discovery_url": discovery_url, - "client_id": "test-client", - "client_secret": "test-secret", + "config": { + "discovery_url": discovery_url, + "client_id": "test-client", + "client_secret": "test-secret", + } }, "rt-secret", ) @@ -232,9 +312,11 @@ async def test_revoke_helper_skips_when_no_revocation_endpoint(): # Returns None and does not raise result = await _revoke_refresh_token_at_idp( { - "discovery_url": discovery_url, - "client_id": "x", - "client_secret": "y", + "config": { + "discovery_url": discovery_url, + "client_id": "x", + "client_secret": "y", + } }, "rt", ) @@ -259,9 +341,11 @@ async def test_revoke_helper_silent_on_idp_error(): ): result = await _revoke_refresh_token_at_idp( { - "discovery_url": "http://x/.well-known", - "client_id": "x", - "client_secret": "y", + "config": { + "discovery_url": "http://x/.well-known", + "client_id": "x", + "client_secret": "y", + } }, "rt", ) diff --git a/tests/unit/test_safe_next_url.py b/tests/unit/test_safe_next_url.py new file mode 100644 index 00000000..056054e3 --- /dev/null +++ b/tests/unit/test_safe_next_url.py @@ -0,0 +1,44 @@ +"""Tests for _safe_next_url, the open-redirect guard for ``?next=`` params. + +Pins the contract that any non-path target falls back to the default, +preventing the open-redirect issue flagged on PR #758. +""" + +import pytest + +from nextcloud_mcp_server.auth.browser_oauth_routes import _safe_next_url + +pytestmark = pytest.mark.unit + + +@pytest.mark.parametrize( + "raw, expected", + [ + # Valid path-only targets pass through. + ("/app", "/app"), + ("/app/foo", "/app/foo"), + ("/oauth/login", "/oauth/login"), + ("/app?x=1&y=2", "/app?x=1&y=2"), + ("/app#frag", "/app#frag"), + # Empty / missing → default. + ("", "/default"), + (None, "/default"), + # Absolute URLs → default. + ("https://evil.example.com", "/default"), + ("http://evil.example.com/path", "/default"), + # Protocol-relative → default. Browser would treat as cross-origin. + ("//evil.example.com", "/default"), + ("//evil.example.com/path", "/default"), + # No leading slash → default. + ("relative/path", "/default"), + ("app", "/default"), + # Whitespace / control chars → default. Defends against tab/space + # injection that some browsers historically tolerated. + ("/app\nfoo", "/default"), + ("/app\tfoo", "/default"), + ("/app\x00foo", "/default"), + ("/app foo", "/default"), + ], +) +def test_safe_next_url(raw, expected): + assert _safe_next_url(raw, "/default") == expected From af25c281bfd7839ce13788f0d01f8ffab55b4289 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sat, 2 May 2026 18:32:19 +0200 Subject: [PATCH 03/14] fix(auth): use Settings for OIDC env vars in token revocation helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After merging master, _should_use_secure_cookies was refactored to read from Settings instead of os.getenv, which dropped `import os` from browser_oauth_routes.py — leaving _revoke_refresh_token_at_idp's four remaining os.getenv() calls undefined (CI ruff F821). Migrate the helper to the same Settings-based pattern: - oidc_discovery_url → settings.oidc_discovery_url - OIDC_CLIENT_ID → settings.oidc_client_id - OIDC_CLIENT_SECRET → settings.oidc_client_secret - NEXTCLOUD_HOST → settings.nextcloud_host Drive-by: the previous fallback read OIDC_CLIENT_ID, but the canonical env var per env.sample / docker-compose is NEXTCLOUD_OIDC_CLIENT_ID. The Settings layer handles this mapping via dynaconf, so the corrected name is now used automatically. Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/auth/browser_oauth_routes.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/nextcloud_mcp_server/auth/browser_oauth_routes.py b/nextcloud_mcp_server/auth/browser_oauth_routes.py index b603fa05..76c6949b 100644 --- a/nextcloud_mcp_server/auth/browser_oauth_routes.py +++ b/nextcloud_mcp_server/auth/browser_oauth_routes.py @@ -641,11 +641,13 @@ async def _revoke_refresh_token_at_idp(oauth_ctx: dict, refresh_token: str) -> N # starlette_lifespan). A flat shape is also accepted for tests and # historical callers. cfg = oauth_ctx.get("config") or oauth_ctx + settings = get_settings() try: - discovery_url = cfg.get("discovery_url") or os.getenv( - "OIDC_DISCOVERY_URL", - f"{os.getenv('NEXTCLOUD_HOST', '')}/.well-known/openid-configuration", - ) + discovery_url = cfg.get("discovery_url") or settings.oidc_discovery_url + if not discovery_url and settings.nextcloud_host: + discovery_url = ( + f"{settings.nextcloud_host}/.well-known/openid-configuration" + ) if not discovery_url: return @@ -658,8 +660,8 @@ async def _revoke_refresh_token_at_idp(oauth_ctx: dict, refresh_token: str) -> N logger.debug("IdP advertises no revocation_endpoint; skipping") return - client_id = cfg.get("client_id") or os.getenv("OIDC_CLIENT_ID") - client_secret = cfg.get("client_secret") or os.getenv("OIDC_CLIENT_SECRET") + client_id = cfg.get("client_id") or settings.oidc_client_id + client_secret = cfg.get("client_secret") or settings.oidc_client_secret if not (client_id and client_secret): logger.debug("No OIDC client credentials available for revocation") return From 2d340a5a6b24cca780325040e6bfd0ce308641e6 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sat, 2 May 2026 18:59:34 +0200 Subject: [PATCH 04/14] 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") From 2ef4bfc4afd91fe30547ff9b948be748a030030d Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sat, 2 May 2026 19:46:36 +0200 Subject: [PATCH 05/14] fix(auth): fail closed on missing sub claim, delete Flow 2 callback session MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the two remaining 🟡 findings from the PR #758 follow-up review: 1. extract_user_id_from_token previously fell back to "default_user" when the verified access token had no sub claim. In a multi-tenant deployment a malformed IdP token could have bucketed every request under a single sentinel user, risking cross-tenant data exposure. The function now raises McpError on that branch; the BasicAuth no-token sentinel path is preserved. 2. oauth_callback_nextcloud (Flow 2) read the PKCE code_verifier from oauth_sessions but never deleted the row, leaving the verifier valid for the full 10-minute TTL. The row is now deleted eagerly inside the same branch, mirroring oauth_login_callback in browser_oauth_routes. Also wires TOKEN_ENCRYPTION_KEY through the docker-compose step in the CI test workflow so the integration matrix can boot — every job had been failing fast on the ${TOKEN_ENCRYPTION_KEY:?...} interpolation guard added in PR #758 finding 5. Tests pin both fixes (test_token_utils_user_id.py, test_oauth_callback_session_cleanup.py). Co-Authored-By: Claude Opus 4.7 (1M context) --- .github/workflows/test.yml | 5 + nextcloud_mcp_server/auth/oauth_routes.py | 5 + nextcloud_mcp_server/auth/token_utils.py | 22 ++- .../test_oauth_callback_session_cleanup.py | 172 ++++++++++++++++++ tests/unit/test_token_utils_user_id.py | 83 +++++++++ 5 files changed, 284 insertions(+), 3 deletions(-) create mode 100644 tests/unit/test_oauth_callback_session_cleanup.py create mode 100644 tests/unit/test_token_utils_user_id.py diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index a0cdfb89..df70a84e 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -139,6 +139,11 @@ jobs: env: MCP_SERVER_URL: ${{ matrix.mcp-internal-url }} NEXTCLOUD_IMAGE: ${{ matrix.nextcloud_image }} + # Required by docker-compose.yml since PR #758 finding 5 (no more + # hardcoded Fernet keys). Generated once and stored as a repo + # secret; the CI tokens.db is ephemeral so a single shared key + # across services is acceptable. + TOKEN_ENCRYPTION_KEY: ${{ secrets.TOKEN_ENCRYPTION_KEY }} - name: Install the latest version of uv uses: astral-sh/setup-uv@37802adc94f370d6bfd71619e3f0bf239e1f3b78 # v7.6.0 diff --git a/nextcloud_mcp_server/auth/oauth_routes.py b/nextcloud_mcp_server/auth/oauth_routes.py index 1b00706a..53c804ea 100644 --- a/nextcloud_mcp_server/auth/oauth_routes.py +++ b/nextcloud_mcp_server/auth/oauth_routes.py @@ -597,6 +597,11 @@ async def oauth_callback_nextcloud(request: Request): logger.info( f"Retrieved code_verifier for Flow 2 callback (state={state[:16]}...)" ) + # One-time-use session: delete eagerly so the stored code_verifier + # can't be replayed for the remainder of the oauth_sessions TTL. + # Mirrors browser_oauth_routes.oauth_login_callback (PR #758 + # follow-up review). + await storage.delete_oauth_session(state) # Exchange code for tokens mcp_server_client_id = os.getenv( diff --git a/nextcloud_mcp_server/auth/token_utils.py b/nextcloud_mcp_server/auth/token_utils.py index 77bd0b2a..b13f411d 100644 --- a/nextcloud_mcp_server/auth/token_utils.py +++ b/nextcloud_mcp_server/auth/token_utils.py @@ -13,6 +13,8 @@ from jwt import PyJWKSet from mcp.server.auth.middleware.auth_context import get_access_token from mcp.server.auth.provider import AccessToken from mcp.server.fastmcp import Context +from mcp.shared.exceptions import McpError +from mcp.types import ErrorData from ..http import nextcloud_httpx_client @@ -188,8 +190,17 @@ async def extract_user_id_from_token(_ctx: Context) -> str: verifier-populated AccessToken via get_access_token(). Returns: - user_id from the verified token, or "default_user" when no token is - present (e.g. BasicAuth mode where this should not be called). + user_id from the verified token, or ``"default_user"`` when no + access token is present at all (BasicAuth mode — there is no + OAuth identity to extract, so the sentinel is returned and the + caller's BasicAuth branch handles it). + + Raises: + McpError: An access token was present but had no ``sub`` claim + (``access_token.resource`` empty). Failing closed prevents a + malformed IdP token from silently bucketing every request + under the ``"default_user"`` key in SQLite, which would risk + cross-tenant data exposure (PR #758 follow-up review). """ access_token: AccessToken | None = get_access_token() @@ -202,6 +213,11 @@ async def extract_user_id_from_token(_ctx: Context) -> str: logger.error( "Access token has no resource (sub) claim — verifier should have rejected it" ) - return "default_user" + raise McpError( + ErrorData( + code=-1, + message="Cannot determine user identity from access token", + ) + ) return user_id diff --git a/tests/unit/test_oauth_callback_session_cleanup.py b/tests/unit/test_oauth_callback_session_cleanup.py new file mode 100644 index 00000000..3d908896 --- /dev/null +++ b/tests/unit/test_oauth_callback_session_cleanup.py @@ -0,0 +1,172 @@ +"""Pin one-time-use semantics on the Flow-2 callback's oauth_session row. + +The PR #758 follow-up review flagged that +``oauth_callback_nextcloud`` reads ``code_verifier`` from the +``oauth_sessions`` table but never deletes the row, leaving the verifier +valid for the rest of the 10-minute TTL. This test exercises the real +storage layer to confirm the row is gone after the callback runs. + +We mock everything *after* the deletion (discovery + token exchange + +ID token verification) so the test focuses on the cleanup contract, +not the OAuth wire protocol. +""" + +import tempfile +from pathlib import Path +from unittest.mock import AsyncMock, MagicMock, patch + +import httpx +import pytest +from cryptography.fernet import Fernet + +from nextcloud_mcp_server.auth.oauth_routes import oauth_callback_nextcloud +from nextcloud_mcp_server.auth.storage import RefreshTokenStorage + +pytestmark = pytest.mark.unit + + +@pytest.fixture +async def storage(): + with tempfile.TemporaryDirectory() as tmpdir: + db_path = Path(tmpdir) / "test_callback_cleanup.db" + s = RefreshTokenStorage( + db_path=str(db_path), encryption_key=Fernet.generate_key().decode() + ) + await s.initialize() + yield s + + +def _build_request(*, code: str, state: str, storage: RefreshTokenStorage): + request = MagicMock() + request.query_params = {"code": code, "state": state} + request.app.state.oauth_context = { + "storage": storage, + "config": { + "discovery_url": "https://idp.example.com/.well-known/openid-configuration", + "mcp_server_url": "https://mcp.example.com", + "client_id": "mcp-server", + "client_secret": "mcp-secret", + }, + } + return request + + +async def test_callback_deletes_oauth_session_after_reading_verifier(storage): + """After a successful callback exchange the row is gone. + + Pins the PR #758 follow-up review fix: previously the row stayed + until the 10-minute TTL elapsed, leaving the stored ``code_verifier`` + valid for replay if ``state`` leaked. + """ + state = "state-abc-123" + await storage.store_oauth_session( + session_id=state, + client_redirect_uri="http://localhost:9999/callback", + state=state, + mcp_authorization_code="verifier-pkce-secret", + flow_type="flow2", + ) + # Sanity check: row exists before the callback runs. + assert await storage.get_oauth_session(state) is not None + + request = _build_request(code="idp-auth-code", state=state, storage=storage) + + # Stub everything after the deletion: discovery, token exchange, ID + # token verification, and the user_oidc UserInfo round-trip. The + # exact responses don't matter — we only care that the deletion has + # happened by the time these are invoked. + fake_discovery = { + "token_endpoint": "https://idp.example.com/token", + "userinfo_endpoint": "https://idp.example.com/userinfo", + "issuer": "https://idp.example.com", + } + fake_userinfo = {"sub": "alice", "email": "alice@example.com"} + fake_token_response = MagicMock() + fake_token_response.json.return_value = { + "access_token": "ac-tok", + "refresh_token": "rf-tok", + "id_token": "id-tok", + "expires_in": 3600, + } + fake_token_response.raise_for_status = MagicMock() + + fake_http = MagicMock() + fake_http.post = AsyncMock(return_value=fake_token_response) + fake_http.__aenter__ = AsyncMock(return_value=fake_http) + fake_http.__aexit__ = AsyncMock(return_value=None) + + with ( + patch( + "nextcloud_mcp_server.auth.oauth_routes._get_cached_discovery", + new=AsyncMock(return_value=fake_discovery), + ), + patch( + "nextcloud_mcp_server.auth.oauth_routes.nextcloud_httpx_client", + return_value=fake_http, + ), + patch( + "nextcloud_mcp_server.auth.oauth_routes.verify_id_token", + new=AsyncMock(return_value=fake_userinfo), + ), + ): + # The callback may go on to do extra work (storing tokens, redirecting, + # rendering HTML); we don't care about the response body, only the + # storage-level side effect. + try: + await oauth_callback_nextcloud(request) + except Exception: + # Any error past the deletion point is fine for this test. + pass + + assert await storage.get_oauth_session(state) is None, ( + "oauth_callback_nextcloud must delete the oauth_sessions row " + "after reading code_verifier (PR #758 follow-up review)" + ) + + +async def test_callback_no_session_row_does_not_crash(storage): + """If the row is already gone (e.g. expired), the callback proceeds.""" + state = "state-missing" + # No store_oauth_session call — the row never existed. + + request = _build_request(code="idp-auth-code", state=state, storage=storage) + + fake_discovery = { + "token_endpoint": "https://idp.example.com/token", + "userinfo_endpoint": "https://idp.example.com/userinfo", + "issuer": "https://idp.example.com", + } + fake_token_response = MagicMock() + fake_token_response.json.return_value = {"access_token": "ac"} + fake_token_response.raise_for_status = MagicMock( + side_effect=httpx.HTTPStatusError( + "boom", + request=MagicMock(), + response=MagicMock(status_code=400), + ) + ) + + fake_http = MagicMock() + fake_http.post = AsyncMock(return_value=fake_token_response) + fake_http.__aenter__ = AsyncMock(return_value=fake_http) + fake_http.__aexit__ = AsyncMock(return_value=None) + + with ( + patch( + "nextcloud_mcp_server.auth.oauth_routes._get_cached_discovery", + new=AsyncMock(return_value=fake_discovery), + ), + patch( + "nextcloud_mcp_server.auth.oauth_routes.nextcloud_httpx_client", + return_value=fake_http, + ), + ): + # We don't care what happens past the deletion — just that the + # missing-row branch doesn't try to delete a nonexistent session. + try: + await oauth_callback_nextcloud(request) + except Exception: + pass + + # No crash, no row, no surprises. + assert await storage.get_oauth_session(state) is None diff --git a/tests/unit/test_token_utils_user_id.py b/tests/unit/test_token_utils_user_id.py new file mode 100644 index 00000000..1ddcc97b --- /dev/null +++ b/tests/unit/test_token_utils_user_id.py @@ -0,0 +1,83 @@ +"""Unit tests for ``extract_user_id_from_token`` (PR #758 follow-up review). + +The function used to silently fall back to ``"default_user"`` whenever the +verified access token had no ``sub`` claim. In a multi-tenant deployment +that would let a malformed IdP token bucket every request under a single +sentinel user, risking cross-tenant data exposure. The fix is to keep the +no-token fallback (BasicAuth mode legitimately calls this without an +OAuth identity) but raise ``McpError`` whenever an access token is +present and ``resource`` is empty. +""" + +import time +from unittest.mock import MagicMock, patch + +import pytest +from mcp.server.auth.provider import AccessToken +from mcp.shared.exceptions import McpError + +from nextcloud_mcp_server.auth.token_utils import extract_user_id_from_token + +pytestmark = pytest.mark.unit + + +def _token(resource: str | None = "alice") -> AccessToken: + return AccessToken( + token="t", + client_id="test-client", + scopes=["openid"], + expires_at=int(time.time() + 3600), + resource=resource, + ) + + +async def test_returns_user_id_when_token_has_sub(): + """Happy path: verified access token with sub → returns the sub.""" + with patch( + "nextcloud_mcp_server.auth.token_utils.get_access_token", + return_value=_token("alice"), + ): + user_id = await extract_user_id_from_token(MagicMock()) + + assert user_id == "alice" + + +async def test_returns_default_user_when_no_access_token(): + """BasicAuth mode: get_access_token() returns None → sentinel. + + BasicAuth deployments don't issue OAuth tokens; the sentinel lets + BasicAuth-aware callers branch on it. Removing this fallback would + break the BasicAuth path. + """ + with patch( + "nextcloud_mcp_server.auth.token_utils.get_access_token", + return_value=None, + ): + user_id = await extract_user_id_from_token(MagicMock()) + + assert user_id == "default_user" + + +async def test_raises_when_token_present_but_resource_empty(): + """Token present but ``resource`` empty → fail closed with McpError. + + Pins the PR #758 follow-up review fix: a malformed IdP token must + not silently funnel users into a shared ``"default_user"`` SQLite + bucket. + """ + with patch( + "nextcloud_mcp_server.auth.token_utils.get_access_token", + return_value=_token(""), + ): + with pytest.raises(McpError, match="Cannot determine user identity"): + await extract_user_id_from_token(MagicMock()) + + +async def test_raises_when_resource_is_none(): + """Same fail-closed behaviour when ``resource`` is None rather than ''.""" + with patch( + "nextcloud_mcp_server.auth.token_utils.get_access_token", + return_value=_token(None), + ): + with pytest.raises(McpError, match="Cannot determine user identity"): + await extract_user_id_from_token(MagicMock()) From 4c84d829848b026abf9fbbefa3caefa72e074e48 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sat, 2 May 2026 20:48:25 +0200 Subject: [PATCH 06/14] fix(auth): address PR #758 auto-review (id-token verify, nonce, CI key) Blocking: - AS proxy callback now calls verify_id_token before caching the proxy code so a tampered IdP response can't smuggle identity claims. Important: - Browser OAuth flow generates and verifies an OIDC nonce; new alembic migration 006 adds the nonce column to oauth_sessions. - _origin_matches_self logs a warning when CSRF check is bypassed. - oauth_tools.py uses get_shared_storage instead of fresh handles. Nits: - New token_utils.get_oidc_discovery shares the 5-minute cache with verify_id_token; oauth_login (integrated) and _revoke_refresh_token_at_idp now use it instead of issuing fresh discovery fetches. - Drop typing.Optional from oauth_tools.py in favour of X | None. CI: - test.yml generates an ephemeral Fernet TOKEN_ENCRYPTION_KEY per run with openssl, removing the dependency on a missing repo secret. Co-Authored-By: Claude Opus 4.7 (1M context) --- .github/workflows/test.yml | 19 +++- ...02_1600_006_add_nonce_to_oauth_sessions.py | 29 ++++++ .../auth/browser_oauth_routes.py | 69 ++++++++----- nextcloud_mcp_server/auth/oauth_routes.py | 22 +++++ nextcloud_mcp_server/auth/storage.py | 8 +- nextcloud_mcp_server/auth/token_utils.py | 11 +++ nextcloud_mcp_server/server/oauth_tools.py | 24 ++--- .../test_oauth_callback_session_cleanup.py | 96 ++++++++++++++++++- tests/unit/test_oauth_logout.py | 53 ++++++++-- 9 files changed, 277 insertions(+), 54 deletions(-) create mode 100644 nextcloud_mcp_server/alembic/versions/20260502_1600_006_add_nonce_to_oauth_sessions.py diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index df70a84e..f85eca78 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -129,6 +129,18 @@ jobs: # npm ci # npm run build + # Generate an ephemeral Fernet key per CI run. docker-compose.yml + # requires TOKEN_ENCRYPTION_KEY (PR #758 finding 5 removed the + # hardcoded default), but the CI tokens.db is destroyed at the end of + # the job so there is no value in persisting the key as a repo secret. + # ``openssl rand -base64 32`` produces 32 bytes encoded as 44 base64 + # chars; ``tr '+/' '-_'`` converts to URL-safe base64, which is + # exactly what Fernet expects. + - name: Generate ephemeral TOKEN_ENCRYPTION_KEY + run: | + KEY=$(openssl rand -base64 32 | tr '+/' '-_') + echo "TOKEN_ENCRYPTION_KEY=${KEY}" >> "$GITHUB_ENV" + # Start services with the appropriate profile - name: Run docker compose uses: hoverkraft-tech/compose-action@4894d2492015c1774ee5a13a95b1072093087ec3 # v2.5.0 @@ -139,11 +151,8 @@ jobs: env: MCP_SERVER_URL: ${{ matrix.mcp-internal-url }} NEXTCLOUD_IMAGE: ${{ matrix.nextcloud_image }} - # Required by docker-compose.yml since PR #758 finding 5 (no more - # hardcoded Fernet keys). Generated once and stored as a repo - # secret; the CI tokens.db is ephemeral so a single shared key - # across services is acceptable. - TOKEN_ENCRYPTION_KEY: ${{ secrets.TOKEN_ENCRYPTION_KEY }} + # Inherited from $GITHUB_ENV via the previous step. + TOKEN_ENCRYPTION_KEY: ${{ env.TOKEN_ENCRYPTION_KEY }} - name: Install the latest version of uv uses: astral-sh/setup-uv@37802adc94f370d6bfd71619e3f0bf239e1f3b78 # v7.6.0 diff --git a/nextcloud_mcp_server/alembic/versions/20260502_1600_006_add_nonce_to_oauth_sessions.py b/nextcloud_mcp_server/alembic/versions/20260502_1600_006_add_nonce_to_oauth_sessions.py new file mode 100644 index 00000000..8489e38f --- /dev/null +++ b/nextcloud_mcp_server/alembic/versions/20260502_1600_006_add_nonce_to_oauth_sessions.py @@ -0,0 +1,29 @@ +"""Add nonce column to oauth_sessions for OIDC ID-token binding. + +PR #758 finding 2: the browser OAuth flow generated PKCE + state but no +``nonce``. Without a nonce, an attacker who obtains a valid ID token for +another user (e.g. from a parallel auth request) could replay it inside +this flow because the token isn't cryptographically tied to the +authorization request. The nonce is generated in ``oauth_login``, +forwarded to the IdP in the auth URL, and verified on the way back. + +Revision ID: 006 +Revises: 005 +Create Date: 2026-05-02 16:00:00.000000 +""" + +from alembic import op + +revision = "006" +down_revision = "005" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + op.execute("ALTER TABLE oauth_sessions ADD COLUMN nonce TEXT") + + +def downgrade() -> None: + # SQLite < 3.35 cannot DROP COLUMN; leave the column on downgrade. + pass diff --git a/nextcloud_mcp_server/auth/browser_oauth_routes.py b/nextcloud_mcp_server/auth/browser_oauth_routes.py index 3d2c6186..cb0b33e9 100644 --- a/nextcloud_mcp_server/auth/browser_oauth_routes.py +++ b/nextcloud_mcp_server/auth/browser_oauth_routes.py @@ -19,6 +19,7 @@ from starlette.responses import HTMLResponse, JSONResponse, RedirectResponse from nextcloud_mcp_server.auth.token_utils import ( IdTokenVerificationError, + get_oidc_discovery, verify_id_token, ) from nextcloud_mcp_server.auth.userinfo_routes import ( @@ -65,7 +66,15 @@ def _origin_matches_self(request: Request, oauth_ctx: dict) -> bool: cfg = oauth_ctx.get("config") or oauth_ctx mcp_server_url = cfg.get("mcp_server_url") if not mcp_server_url: - # Mis-configured deployment — fail open rather than break logout. + # Mis-configured deployment — fail open rather than break logout, but + # log loudly so the operator can see this is happening (PR #758 + # finding 3). Other OAuth code paths require ``mcp_server_url`` and + # KeyError if it's absent, so this branch should never fire in a + # correctly configured deployment. + logger.warning( + "CSRF check bypassed on /oauth/logout: mcp_server_url not " + "configured in oauth_context — set NEXTCLOUD_MCP_SERVER_URL" + ) return True expected = _normalise_origin(mcp_server_url) @@ -149,6 +158,12 @@ async def oauth_login(request: Request) -> RedirectResponse | JSONResponse: # Generate state for CSRF protection state = secrets.token_urlsafe(32) + # Generate OIDC nonce so the ID token returned on callback can be bound + # to THIS auth request (PR #758 finding 2). Without a nonce, an attacker + # who acquired a separate valid ID token could replay it inside this + # flow. + nonce = secrets.token_urlsafe(32) + # Build OAuth authorization URL mcp_server_url = oauth_config["mcp_server_url"] callback_uri = f"{mcp_server_url}/oauth/callback" @@ -164,7 +179,8 @@ async def oauth_login(request: Request) -> RedirectResponse | JSONResponse: digest = hashlib.sha256(code_verifier.encode()).digest() code_challenge = urlsafe_b64encode(digest).decode().rstrip("=") - # Store code_verifier in session for retrieval during callback (using state as key) + # Store code_verifier + nonce in session for retrieval during callback + # (using state as key) await storage.store_oauth_session( session_id=state, # Use state as session ID client_id="browser-ui", @@ -173,6 +189,7 @@ async def oauth_login(request: Request) -> RedirectResponse | JSONResponse: code_challenge=code_challenge, code_challenge_method="S256", mcp_authorization_code=code_verifier, # Store code_verifier here temporarily + nonce=nonce, flow_type="browser", ttl_seconds=600, # 10 minutes ) @@ -199,6 +216,7 @@ async def oauth_login(request: Request) -> RedirectResponse | JSONResponse: "response_type": "code", "scope": scopes, "state": state, + "nonce": nonce, "code_challenge": code_challenge, "code_challenge_method": "S256", "prompt": "consent", # Ensure refresh token @@ -219,12 +237,11 @@ async def oauth_login(request: Request) -> RedirectResponse | JSONResponse: status_code=500, ) - # Fetch authorization endpoint - async with nextcloud_httpx_client() as http_client: - response = await http_client.get(discovery_url) - response.raise_for_status() - discovery = response.json() - authorization_endpoint = discovery["authorization_endpoint"] + # Fetch authorization endpoint via the shared 5-minute discovery + # cache (PR #758 nit 5) so each browser login doesn't hit the IdP's + # discovery endpoint. + discovery = await get_oidc_discovery(discovery_url) + authorization_endpoint = discovery["authorization_endpoint"] # Include offline_access only if the IdP advertises it (or if # scopes_supported is absent from the discovery document). @@ -257,6 +274,7 @@ async def oauth_login(request: Request) -> RedirectResponse | JSONResponse: "response_type": "code", "scope": scopes, "state": state, + "nonce": nonce, "code_challenge": code_challenge, "code_challenge_method": "S256", "prompt": "consent", # Ensure refresh token @@ -336,13 +354,17 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo oauth_client = oauth_ctx["oauth_client"] oauth_config = oauth_ctx["config"] - # Retrieve code_verifier and redirect URL from session storage + # Retrieve code_verifier, nonce, and redirect URL from session storage code_verifier = "" + nonce: str | None = None next_url = "/app" # Default redirect oauth_session = await storage.get_oauth_session(state) if oauth_session: # code_verifier was stored in mcp_authorization_code field code_verifier = oauth_session.get("mcp_authorization_code", "") + # nonce bound to this auth request — verified against the ID token + # below (PR #758 finding 2). + nonce = oauth_session.get("nonce") # next_url was stored in client_redirect_uri field — re-validate at # read-time as defense-in-depth (issue #758 finding 3). The session # row could have been written by an older code path or reused. @@ -483,6 +505,7 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo id_token, discovery_url=verification_discovery_url, expected_audience=verification_audience, + expected_nonce=nonce, ) except IdTokenVerificationError as e: logger.error("ID token verification failed: %s", e) @@ -673,21 +696,21 @@ async def _revoke_refresh_token_at_idp(oauth_ctx: dict, refresh_token: str) -> N if not discovery_url: return + # Re-use the shared 5-minute discovery cache (PR #758 nit 6) so a + # burst of logouts doesn't hammer the IdP's discovery endpoint. + discovery = await get_oidc_discovery(discovery_url) + revocation_endpoint = discovery.get("revocation_endpoint") + if not revocation_endpoint: + logger.debug("IdP advertises no revocation_endpoint; skipping") + return + + client_id = cfg.get("client_id") or settings.oidc_client_id + client_secret = cfg.get("client_secret") or settings.oidc_client_secret + if not (client_id and client_secret): + logger.debug("No OIDC client credentials available for revocation") + return + async with nextcloud_httpx_client() as http_client: - discovery_response = await http_client.get(discovery_url) - discovery_response.raise_for_status() - discovery = discovery_response.json() - revocation_endpoint = discovery.get("revocation_endpoint") - if not revocation_endpoint: - logger.debug("IdP advertises no revocation_endpoint; skipping") - return - - client_id = cfg.get("client_id") or settings.oidc_client_id - client_secret = cfg.get("client_secret") or settings.oidc_client_secret - if not (client_id and client_secret): - logger.debug("No OIDC client credentials available for revocation") - return - response = await http_client.post( revocation_endpoint, data={ diff --git a/nextcloud_mcp_server/auth/oauth_routes.py b/nextcloud_mcp_server/auth/oauth_routes.py index 53c804ea..2f6de955 100644 --- a/nextcloud_mcp_server/auth/oauth_routes.py +++ b/nextcloud_mcp_server/auth/oauth_routes.py @@ -954,6 +954,28 @@ async def _oauth_callback_as_proxy( f"(token_type={nc_token_response.get('token_type')})" ) + # Verify the ID token signature + claims before caching the response + # (PR #758 finding 1). Without this, a compromised IdP or tampered + # transport could plant arbitrary identity claims into the proxy code + # entry that gets handed back to the MCP client. Mirrors the + # verification done in oauth_callback_nextcloud. + id_token = nc_token_response.get("id_token") + try: + await verify_id_token( + id_token, + discovery_url=discovery_url, + expected_audience=mcp_server_client_id, + ) + except IdTokenVerificationError as e: + logger.error("AS proxy: ID token verification failed: %s", e) + return JSONResponse( + { + "error": "invalid_token", + "error_description": "ID token failed verification", + }, + status_code=400, + ) + # Generate a proxy authorization code for the client proxy_code = secrets.token_urlsafe(32) _proxy_codes[proxy_code] = ProxyCodeEntry( diff --git a/nextcloud_mcp_server/auth/storage.py b/nextcloud_mcp_server/auth/storage.py index a9fc9a33..691c1cc2 100644 --- a/nextcloud_mcp_server/auth/storage.py +++ b/nextcloud_mcp_server/auth/storage.py @@ -917,6 +917,7 @@ class RefreshTokenStorage: flow_type: str = "hybrid", is_provisioning: bool = False, requested_scopes: str | None = None, + nonce: str | None = None, ttl_seconds: int = 600, # 10 minutes ) -> None: """ @@ -933,6 +934,8 @@ class RefreshTokenStorage: flow_type: Type of flow ('hybrid', 'flow1', 'flow2') is_provisioning: Whether this is a Flow 2 provisioning session requested_scopes: Requested OAuth scopes + nonce: OIDC ``nonce`` value bound to this auth request, returned + in the ID token and verified on callback (PR #758 finding 2). ttl_seconds: Session TTL in seconds """ if not self._initialized: @@ -947,8 +950,8 @@ class RefreshTokenStorage: INSERT INTO oauth_sessions (session_id, client_id, client_redirect_uri, state, code_challenge, code_challenge_method, mcp_authorization_code, flow_type, - is_provisioning, requested_scopes, created_at, expires_at) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + is_provisioning, requested_scopes, nonce, created_at, expires_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) """, ( session_id, @@ -961,6 +964,7 @@ class RefreshTokenStorage: flow_type, is_provisioning, requested_scopes, + nonce, now, expires_at, ), diff --git a/nextcloud_mcp_server/auth/token_utils.py b/nextcloud_mcp_server/auth/token_utils.py index b13f411d..7903ac34 100644 --- a/nextcloud_mcp_server/auth/token_utils.py +++ b/nextcloud_mcp_server/auth/token_utils.py @@ -52,6 +52,17 @@ async def _get_cached( return data +async def get_oidc_discovery(discovery_url: str) -> dict[str, Any]: + """Return the cached OIDC discovery document for *discovery_url*. + + Shares the 5-minute discovery cache used by `verify_id_token`, so a + callback that does discovery → token-exchange → ID-token verification + reuses one HTTP round-trip instead of three. Public alias for `_get_cached` + against `_discovery_cache` (PR #758 nits 5 & 6). + """ + return await _get_cached(_discovery_cache, discovery_url) + + async def verify_id_token( id_token: str, *, diff --git a/nextcloud_mcp_server/server/oauth_tools.py b/nextcloud_mcp_server/server/oauth_tools.py index 90769bf5..1ae62e79 100644 --- a/nextcloud_mcp_server/server/oauth_tools.py +++ b/nextcloud_mcp_server/server/oauth_tools.py @@ -9,7 +9,6 @@ import logging import os import secrets from datetime import datetime, timezone -from typing import Optional from urllib.parse import urlencode from mcp.server.fastmcp import Context @@ -18,7 +17,7 @@ from pydantic import BaseModel, Field from nextcloud_mcp_server.auth import require_scopes from nextcloud_mcp_server.auth.astrolabe_client import AstrolabeClient -from nextcloud_mcp_server.auth.storage import RefreshTokenStorage +from nextcloud_mcp_server.auth.storage import get_shared_storage from nextcloud_mcp_server.auth.token_broker import TokenBrokerService # Re-export for backward compatibility — canonical location is auth.token_utils @@ -34,17 +33,17 @@ class ProvisioningStatus(BaseModel): """Status of Nextcloud provisioning for a user.""" is_provisioned: bool = Field(description="Whether Nextcloud access is provisioned") - provisioned_at: Optional[str] = Field( + provisioned_at: str | None = Field( None, description="ISO timestamp when provisioned" ) - credential_type: Optional[str] = Field( + credential_type: str | None = Field( None, description="Type of credential ('refresh_token' or 'app_password')" ) - client_id: Optional[str] = Field( + client_id: str | None = Field( None, description="Client ID that initiated the original Flow 1" ) - scopes: Optional[list[str]] = Field(None, description="Granted scopes") - flow_type: Optional[str] = Field( + scopes: list[str] | None = Field(None, description="Granted scopes") + flow_type: str | None = Field( None, description="Type of flow used ('hybrid', 'flow1', 'flow2')" ) @@ -53,7 +52,7 @@ class ProvisioningResult(BaseModel): """Result of provisioning attempt.""" success: bool = Field(description="Whether provisioning was initiated") - provisioning_url: Optional[str] = Field( + provisioning_url: str | None = Field( None, description="URL to Astrolabe settings for provisioning background sync" ) message: str = Field(description="Status message for the user") @@ -122,8 +121,7 @@ async def get_provisioning_status(ctx: Context, user_id: str) -> ProvisioningSta logger.info( f" get_provisioning_status: Looking up refresh token for user_id={user_id}" ) - storage = RefreshTokenStorage.from_env() - await storage.initialize() + storage = await get_shared_storage() token_data = await storage.get_refresh_token(user_id) @@ -291,8 +289,7 @@ async def revoke_nextcloud_access(ctx: Context, user_id: str) -> RevocationResul ) # Initialize Token Broker to handle revocation - storage = RefreshTokenStorage.from_env() - await storage.initialize() + storage = await get_shared_storage() # Get OAuth client credentials from storage client_creds = await storage.get_oauth_client() @@ -420,8 +417,7 @@ async def check_logged_in(ctx: Context, user_id: str) -> str: state = secrets.token_urlsafe(32) # Store state in session for validation on callback - storage = RefreshTokenStorage.from_env() - await storage.initialize() + storage = await get_shared_storage() # Create OAuth session for Flow 2 session_id = f"flow2_{user_id}_{secrets.token_hex(8)}" diff --git a/tests/unit/test_oauth_callback_session_cleanup.py b/tests/unit/test_oauth_callback_session_cleanup.py index 3d908896..1f84f678 100644 --- a/tests/unit/test_oauth_callback_session_cleanup.py +++ b/tests/unit/test_oauth_callback_session_cleanup.py @@ -9,6 +9,11 @@ storage layer to confirm the row is gone after the callback runs. We mock everything *after* the deletion (discovery + token exchange + ID token verification) so the test focuses on the cleanup contract, not the OAuth wire protocol. + +Also pins the AS-proxy callback's ID-token verification rejection path +introduced in PR #758 finding 1 (auto-review): a forged or unsigned +id_token must surface as a 400 ``invalid_token`` JSONResponse and must +not register a proxy code. """ import tempfile @@ -19,8 +24,15 @@ import httpx import pytest from cryptography.fernet import Fernet -from nextcloud_mcp_server.auth.oauth_routes import oauth_callback_nextcloud +from nextcloud_mcp_server.auth.oauth_routes import ( + ASProxySession, + _as_proxy_sessions, + _oauth_callback_as_proxy, + _proxy_codes, + oauth_callback_nextcloud, +) from nextcloud_mcp_server.auth.storage import RefreshTokenStorage +from nextcloud_mcp_server.auth.token_utils import IdTokenVerificationError pytestmark = pytest.mark.unit @@ -170,3 +182,85 @@ async def test_callback_no_session_row_does_not_crash(storage): # No crash, no row, no surprises. assert await storage.get_oauth_session(state) is None + + +# --------------------------------------------------------------------------- +# AS proxy callback (PR #758 finding 1): ID-token verification rejection +# --------------------------------------------------------------------------- + + +def _build_as_proxy_request(*, code: str, state: str): + request = MagicMock() + request.query_params = {"code": code, "state": state} + request.app.state.oauth_context = { + "config": { + "discovery_url": "https://idp.example.com/.well-known/openid-configuration", + "mcp_server_url": "https://mcp.example.com", + "client_id": "mcp-server", + "client_secret": "mcp-secret", + } + } + return request + + +async def test_as_proxy_rejects_invalid_id_token(): + """Forged/unsigned id_token in the IdP token response → 400 invalid_token. + + Pins PR #758 finding 1. Without verification a compromised IdP or + tampered transport could plant arbitrary identity claims into the + cached ProxyCodeEntry that downstream clients pick up. + """ + server_state = "as-proxy-state-rejected" + _as_proxy_sessions[server_state] = ASProxySession( + client_id="mcp-client", + client_redirect_uri="http://127.0.0.1:9999/callback", + client_state="client-state-xyz", + code_challenge="challenge", + code_challenge_method="S256", + requested_scopes="openid", + ) + _proxy_codes.clear() + + request = _build_as_proxy_request(code="auth-code", state=server_state) + + fake_discovery = { + "token_endpoint": "https://idp.example.com/token", + "issuer": "https://idp.example.com", + } + fake_token_response = MagicMock(status_code=200) + fake_token_response.json.return_value = { + "access_token": "ac-tok", + "refresh_token": "rf-tok", + "id_token": "forged.id.token", + "token_type": "Bearer", + } + + fake_http = MagicMock() + fake_http.post = AsyncMock(return_value=fake_token_response) + fake_http.__aenter__ = AsyncMock(return_value=fake_http) + fake_http.__aexit__ = AsyncMock(return_value=None) + + with ( + patch( + "nextcloud_mcp_server.auth.oauth_routes._get_cached_discovery", + new=AsyncMock(return_value=fake_discovery), + ), + patch( + "nextcloud_mcp_server.auth.oauth_routes.nextcloud_httpx_client", + return_value=fake_http, + ), + patch( + "nextcloud_mcp_server.auth.oauth_routes.verify_id_token", + new=AsyncMock(side_effect=IdTokenVerificationError("bad signature")), + ), + ): + response = await _oauth_callback_as_proxy(request, server_state) + + assert response.status_code == 400 + body = bytes(response.body).decode() + assert "invalid_token" in body + # Critical: the proxy code store must not have grown — a rejected + # callback must not be turned into a redeemable proxy code. + assert _proxy_codes == {} + # And the session has been popped (one-time use). + assert server_state not in _as_proxy_sessions diff --git a/tests/unit/test_oauth_logout.py b/tests/unit/test_oauth_logout.py index b8629ace..095f81c1 100644 --- a/tests/unit/test_oauth_logout.py +++ b/tests/unit/test_oauth_logout.py @@ -20,6 +20,7 @@ import pytest from cryptography.fernet import Fernet from starlette.requests import HTTPConnection +from nextcloud_mcp_server.auth import token_utils from nextcloud_mcp_server.auth.browser_oauth_routes import ( _revoke_refresh_token_at_idp, oauth_logout, @@ -30,6 +31,20 @@ from nextcloud_mcp_server.auth.storage import RefreshTokenStorage pytestmark = pytest.mark.unit +@pytest.fixture(autouse=True) +def _clear_oidc_discovery_cache(): + """Reset the shared discovery cache so tests don't see each other's fetches. + + ``_revoke_refresh_token_at_idp`` was changed (PR #758 nit 6) to use + ``token_utils.get_oidc_discovery`` which caches for 5 minutes — without + this clear, the second test in the file would see the first test's + discovery doc and skip the MockTransport call. + """ + token_utils._discovery_cache.clear() + yield + token_utils._discovery_cache.clear() + + # --------------------------------------------------------------------------- # storage fixture (real SQLite backend; lighter than mocking every call) # --------------------------------------------------------------------------- @@ -337,9 +352,17 @@ async def test_revoke_helper_posts_to_revocation_endpoint(): kwargs["transport"] = transport return httpx.AsyncClient(**kwargs) - with patch( - "nextcloud_mcp_server.auth.browser_oauth_routes.nextcloud_httpx_client", - side_effect=fake_client, + # Discovery now goes through token_utils.get_oidc_discovery (PR #758 nit + # 6); revocation POST still uses browser_oauth_routes' httpx client. + with ( + patch( + "nextcloud_mcp_server.auth.browser_oauth_routes.nextcloud_httpx_client", + side_effect=fake_client, + ), + patch( + "nextcloud_mcp_server.auth.token_utils.nextcloud_httpx_client", + side_effect=fake_client, + ), ): await _revoke_refresh_token_at_idp( { @@ -373,9 +396,15 @@ async def test_revoke_helper_skips_when_no_revocation_endpoint(): kwargs["transport"] = transport return httpx.AsyncClient(**kwargs) - with patch( - "nextcloud_mcp_server.auth.browser_oauth_routes.nextcloud_httpx_client", - side_effect=fake_client, + with ( + patch( + "nextcloud_mcp_server.auth.browser_oauth_routes.nextcloud_httpx_client", + side_effect=fake_client, + ), + patch( + "nextcloud_mcp_server.auth.token_utils.nextcloud_httpx_client", + side_effect=fake_client, + ), ): # Returns None and does not raise result = await _revoke_refresh_token_at_idp( @@ -403,9 +432,15 @@ async def test_revoke_helper_silent_on_idp_error(): kwargs["transport"] = transport return httpx.AsyncClient(**kwargs) - with patch( - "nextcloud_mcp_server.auth.browser_oauth_routes.nextcloud_httpx_client", - side_effect=fake_client, + with ( + patch( + "nextcloud_mcp_server.auth.browser_oauth_routes.nextcloud_httpx_client", + side_effect=fake_client, + ), + patch( + "nextcloud_mcp_server.auth.token_utils.nextcloud_httpx_client", + side_effect=fake_client, + ), ): result = await _revoke_refresh_token_at_idp( { From c33d52ea9139f5d4daa892a5b28ff158f0c0d858 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sat, 2 May 2026 21:56:08 +0200 Subject: [PATCH 07/14] fix(auth): address PR #758 round-2 review - oauth_login_callback's integrated-mode token-exchange branch now reuses the shared discovery cache via get_oidc_discovery (round-2 finding 1). - AS proxy flow now generates an OIDC nonce in oauth_authorize, stores it on ASProxySession, forwards it to the IdP, and passes it as expected_nonce to verify_id_token in _oauth_callback_as_proxy (round-2 finding 2). - Consolidate the two parallel discovery caches: oauth_routes' local _discovery_cache and _get_cached_discovery are removed; all callers now go through token_utils.get_oidc_discovery, which acquires the follow_redirects=True knob it needs for Nextcloud installs without pretty URLs (round-2 finding 3). - Demote per-user INFO logs in oauth_tools.py (check_logged_in, get_provisioning_status) to DEBUG; the elicitation auth URL is no longer logged because it contains a sensitive state token (round-2 finding 4). Also pin nonce binding behaviour with a new unit test that asserts _oauth_callback_as_proxy forwards session.nonce to verify_id_token, and update test mocks to track the cache consolidation. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../auth/browser_oauth_routes.py | 10 +- nextcloud_mcp_server/auth/oauth_routes.py | 52 ++++---- nextcloud_mcp_server/auth/token_utils.py | 36 ++++-- nextcloud_mcp_server/server/oauth_tools.py | 114 ++++++++++-------- tests/unit/test_browser_oauth_xss.py | 24 +++- tests/unit/test_dcr_proxy.py | 2 +- .../test_oauth_callback_session_cleanup.py | 69 ++++++++++- tests/unit/test_oidc_discovery.py | 17 +-- 8 files changed, 210 insertions(+), 114 deletions(-) diff --git a/nextcloud_mcp_server/auth/browser_oauth_routes.py b/nextcloud_mcp_server/auth/browser_oauth_routes.py index cb0b33e9..58bb3d95 100644 --- a/nextcloud_mcp_server/auth/browser_oauth_routes.py +++ b/nextcloud_mcp_server/auth/browser_oauth_routes.py @@ -407,11 +407,11 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo else: # Integrated mode (Nextcloud OIDC) discovery_url = oauth_config.get("discovery_url") - async with nextcloud_httpx_client() as http_client: - response = await http_client.get(discovery_url) - response.raise_for_status() - discovery = response.json() - token_endpoint = discovery["token_endpoint"] + # Use the shared 5-minute discovery cache; oauth_login() above + # has already populated it for this discovery_url so the + # callback should hit the cache rather than re-fetching. + discovery = await get_oidc_discovery(discovery_url) + token_endpoint = discovery["token_endpoint"] token_params = { "grant_type": "authorization_code", diff --git a/nextcloud_mcp_server/auth/oauth_routes.py b/nextcloud_mcp_server/auth/oauth_routes.py index 2f6de955..db55cb3f 100644 --- a/nextcloud_mcp_server/auth/oauth_routes.py +++ b/nextcloud_mcp_server/auth/oauth_routes.py @@ -38,6 +38,7 @@ from nextcloud_mcp_server.auth.client_registry import get_client_registry from nextcloud_mcp_server.auth.storage import RefreshTokenStorage from nextcloud_mcp_server.auth.token_utils import ( IdTokenVerificationError, + get_oidc_discovery, verify_id_token, ) from nextcloud_mcp_server.config import get_settings @@ -91,6 +92,7 @@ class ASProxySession: code_challenge: str code_challenge_method: str requested_scopes: str + nonce: str = "" created_at: float = field(default_factory=time.time) expires_at: float = field(default_factory=lambda: time.time() + 600) @@ -103,10 +105,6 @@ class ASProxySession: _proxy_codes: dict[str, ProxyCodeEntry] = {} _as_proxy_sessions: dict[str, ASProxySession] = {} -# OIDC discovery document cache (URL → (expires_at, data)) -_discovery_cache: dict[str, tuple[float, dict[str, Any]]] = {} -_DISCOVERY_CACHE_TTL = 300 # 5 minutes - # DCR rate limiting (IP → [timestamps]) _dcr_rate_limit: dict[str, list[float]] = {} _DCR_RATE_LIMIT_MAX = 10 # max requests @@ -138,26 +136,6 @@ def _transform_scopes_for_idp(scopes: str, resource_server_id: str) -> str: ) -async def _get_cached_discovery(url: str) -> dict[str, Any]: - """Fetch OIDC discovery document with caching (5-minute TTL). - - Follows redirects so the configured discovery URL works against Nextcloud - instances without pretty URLs enabled, where ``/.well-known/openid-configuration`` - issues a 301 to ``/index.php/.well-known/openid-configuration``. - """ - now = time.time() - if url in _discovery_cache: - expires_at, data = _discovery_cache[url] - if now < expires_at: - return data - async with nextcloud_httpx_client(follow_redirects=True) as http_client: - response = await http_client.get(url) - response.raise_for_status() - data = response.json() - _discovery_cache[url] = (now + _DISCOVERY_CACHE_TTL, data) - return data - - def _cleanup_expired_proxy_codes() -> None: """Remove expired proxy codes and sessions.""" now = time.time() @@ -316,6 +294,10 @@ async def oauth_authorize(request: Request) -> RedirectResponse | JSONResponse: # We do NOT forward PKCE to Nextcloud — the MCP server is a confidential client. server_state = secrets.token_urlsafe(32) + # OIDC nonce binds the IdP's ID token to THIS authorization request, + # blocking ID-token replay across flows (PR #758 round-2 finding 2). + server_nonce = secrets.token_urlsafe(32) + requested_scope = request.query_params.get("scope", "") default_scopes = "openid profile email" resource_scopes = oauth_config.get("scopes", "") @@ -333,6 +315,7 @@ async def oauth_authorize(request: Request) -> RedirectResponse | JSONResponse: code_challenge=code_challenge, code_challenge_method=code_challenge_method, requested_scopes=scopes, + nonce=server_nonce, ) # Use MCP server's own client_id with Nextcloud @@ -359,7 +342,7 @@ async def oauth_authorize(request: Request) -> RedirectResponse | JSONResponse: status_code=500, ) - discovery = await _get_cached_discovery(discovery_url) + discovery = await get_oidc_discovery(discovery_url) authorization_endpoint = discovery["authorization_endpoint"] # Replace internal Docker hostname with public URL for browser access @@ -395,6 +378,7 @@ async def oauth_authorize(request: Request) -> RedirectResponse | JSONResponse: "response_type": "code", "scope": idp_scope_str, "state": server_state, + "nonce": server_nonce, "prompt": "consent", "resource": f"{mcp_server_url}/mcp", # MCP server audience } @@ -469,7 +453,7 @@ async def oauth_authorize_nextcloud( # supporting the offline_access scope. discovery_url = oauth_config.get("discovery_url") if discovery_url: - disc = await _get_cached_discovery(discovery_url) + disc = await get_oidc_discovery(discovery_url) scopes_supported = disc.get("scopes_supported") if scopes_supported is None or "offline_access" in scopes_supported: scopes += " offline_access" @@ -506,7 +490,7 @@ async def oauth_authorize_nextcloud( status_code=500, ) - discovery = await _get_cached_discovery(discovery_url) + discovery = await get_oidc_discovery(discovery_url) authorization_endpoint = discovery["authorization_endpoint"] # Fix internal hostname for browser access @@ -623,7 +607,7 @@ async def oauth_callback_nextcloud(request: Request): status_code=500, ) - discovery = await _get_cached_discovery(discovery_url) + discovery = await get_oidc_discovery(discovery_url) token_endpoint = discovery["token_endpoint"] # Build token exchange params @@ -917,7 +901,7 @@ async def _oauth_callback_as_proxy( status_code=500, ) - discovery = await _get_cached_discovery(discovery_url) + discovery = await get_oidc_discovery(discovery_url) token_endpoint = discovery["token_endpoint"] # Exchange auth code with Nextcloud (server-side, confidential client, no PKCE) @@ -959,12 +943,18 @@ async def _oauth_callback_as_proxy( # transport could plant arbitrary identity claims into the proxy code # entry that gets handed back to the MCP client. Mirrors the # 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. id_token = nc_token_response.get("id_token") try: await verify_id_token( id_token, discovery_url=discovery_url, expected_audience=mcp_server_client_id, + expected_nonce=session.nonce or None, ) except IdTokenVerificationError as e: logger.error("AS proxy: ID token verification failed: %s", e) @@ -1252,7 +1242,7 @@ async def _token_refresh(request: Request, form) -> JSONResponse: status_code=500, ) - discovery = await _get_cached_discovery(discovery_url) + discovery = await get_oidc_discovery(discovery_url) token_endpoint = discovery["token_endpoint"] # Proxy refresh request to Nextcloud @@ -1341,7 +1331,7 @@ async def oauth_register_proxy(request: Request) -> JSONResponse: registration_endpoint = None if discovery_url: try: - discovery = await _get_cached_discovery(discovery_url) + discovery = await get_oidc_discovery(discovery_url) registration_endpoint = discovery.get("registration_endpoint") except Exception: logger.warning("Failed to fetch OIDC discovery for DCR endpoint") diff --git a/nextcloud_mcp_server/auth/token_utils.py b/nextcloud_mcp_server/auth/token_utils.py index 7903ac34..95364ab0 100644 --- a/nextcloud_mcp_server/auth/token_utils.py +++ b/nextcloud_mcp_server/auth/token_utils.py @@ -21,10 +21,11 @@ from ..http import nextcloud_httpx_client logger = logging.getLogger(__name__) -# OIDC discovery + JWKS caches keyed by URL → (expires_at, data). Mirrors the -# pattern in oauth_routes._get_cached_discovery so that ID-token verification -# during the OAuth callback doesn't make two extra round-trips per login (PR -# #758 finding 4). 5-minute TTL matches oauth_routes. +# OIDC discovery + JWKS caches keyed by URL → (expires_at, data). Single +# source of truth for the codebase: oauth_routes / browser_oauth_routes both +# go through ``get_oidc_discovery`` which reads/writes _discovery_cache, so +# the first discovery fetch primes the cache for all later callers (PR #758 +# round-2 nit 3). 5-minute TTL. _discovery_cache: dict[str, tuple[float, dict[str, Any]]] = {} _jwks_cache: dict[str, tuple[float, dict[str, Any]]] = {} _OIDC_CACHE_TTL = 300 @@ -35,16 +36,26 @@ class IdTokenVerificationError(Exception): async def _get_cached( - cache: dict[str, tuple[float, dict[str, Any]]], url: str + cache: dict[str, tuple[float, dict[str, Any]]], + url: str, + *, + follow_redirects: bool = False, ) -> dict[str, Any]: - """Return cached JSON response for *url* or fetch + cache on miss/expiry.""" + """Return cached JSON response for *url* or fetch + cache on miss/expiry. + + ``follow_redirects`` is forwarded to ``nextcloud_httpx_client``: discovery + fetches against Nextcloud without pretty URLs need it (the configured + ``/.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. + """ 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() as http_client: + 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() @@ -57,10 +68,13 @@ async def get_oidc_discovery(discovery_url: str) -> dict[str, Any]: Shares the 5-minute discovery cache used by `verify_id_token`, so a callback that does discovery → token-exchange → ID-token verification - reuses one HTTP round-trip instead of three. Public alias for `_get_cached` - against `_discovery_cache` (PR #758 nits 5 & 6). + reuses one HTTP round-trip instead of three. The fetch follows + redirects because Nextcloud without pretty URLs returns 301 from + ``/.well-known/openid-configuration`` to ``/index.php/.well-known/...``. + Single source of truth for OIDC discovery in the codebase + (PR #758 round-2 nit 3). """ - return await _get_cached(_discovery_cache, discovery_url) + return await _get_cached(_discovery_cache, discovery_url, follow_redirects=True) async def verify_id_token( @@ -103,7 +117,7 @@ async def verify_id_token( raise IdTokenVerificationError("ID token missing from token response") try: - discovery = await _get_cached(_discovery_cache, discovery_url) + discovery = await get_oidc_discovery(discovery_url) issuer = discovery.get("issuer") jwks_uri = discovery.get("jwks_uri") diff --git a/nextcloud_mcp_server/server/oauth_tools.py b/nextcloud_mcp_server/server/oauth_tools.py index 1ae62e79..ea7ee0b0 100644 --- a/nextcloud_mcp_server/server/oauth_tools.py +++ b/nextcloud_mcp_server/server/oauth_tools.py @@ -105,8 +105,12 @@ async def get_provisioning_status(ctx: Context, user_id: str) -> ProvisioningSta status = await astrolabe.get_background_sync_status(user_id) if status.get("has_access"): - logger.info( - f" get_provisioning_status: ✓ App password FOUND for user_id={user_id}" + # Demoted to debug (PR #758 round-2 nit 4): user_id ends up + # in log aggregation on every call, which is noise in a + # multi-tenant deployment. + logger.debug( + " get_provisioning_status: app password FOUND for user_id=%s", + user_id, ) provisioned_at_str = status.get("provisioned_at") return ProvisioningStatus( @@ -115,28 +119,28 @@ async def get_provisioning_status(ctx: Context, user_id: str) -> ProvisioningSta credential_type="app_password", ) except Exception as e: - logger.debug(f" App password check failed for {user_id}: {e}") + logger.debug(" App password check failed for %s: %s", user_id, e) # Check for OAuth refresh token (fallback) - logger.info( - f" get_provisioning_status: Looking up refresh token for user_id={user_id}" + logger.debug( + " get_provisioning_status: looking up refresh token for user_id=%s", user_id ) storage = await get_shared_storage() token_data = await storage.get_refresh_token(user_id) if not token_data: - logger.info( - f" get_provisioning_status: ✗ No credentials found for user_id={user_id}" + logger.debug( + " get_provisioning_status: no credentials found for user_id=%s", user_id ) return ProvisioningStatus(is_provisioned=False) - logger.info( - f" get_provisioning_status: ✓ Refresh token FOUND for user_id={user_id}" - ) - logger.info(f" flow_type: {token_data.get('flow_type')}") - logger.info( - f" provisioning_client_id: {token_data.get('provisioning_client_id', 'N/A')}" + logger.debug( + " get_provisioning_status: refresh token FOUND for user_id=%s " + "flow_type=%s provisioning_client_id=%s", + user_id, + token_data.get("flow_type"), + token_data.get("provisioning_client_id", "N/A"), ) # Convert timestamp to ISO format if present @@ -370,18 +374,22 @@ async def check_logged_in(ctx: Context, user_id: str) -> str: "yes" if logged in, or elicitation prompting for login """ try: - # Check if already logged in - logger.info(f"Checking provisioning status for user_id: {user_id}") + # Demoted to debug (PR #758 round-2 nit 4): per-user logging at INFO + # ends up in log aggregation on every check_logged_in call, which is + # noise in a hosted multi-tenant deployment. + logger.debug("Checking provisioning status for user_id=%s", user_id) status = await get_provisioning_status(ctx, user_id) - logger.info(f" Provisioning status: is_provisioned={status.is_provisioned}") + logger.debug( + " Provisioning status for %s: is_provisioned=%s", + user_id, + status.is_provisioned, + ) if status.is_provisioned: - logger.info(f"✓ User {user_id} is already logged in - returning 'yes'") - logger.info("=" * 60) + logger.debug("User %s already logged in", user_id) return "yes" - logger.info(f"✗ User {user_id} is NOT logged in - triggering elicitation") - logger.info("=" * 60) + logger.debug("User %s NOT logged in — triggering elicitation", user_id) # Not logged in - generate OAuth URL for Flow 2 # Use settings (handles both ENABLE_BACKGROUND_OPERATIONS and ENABLE_OFFLINE_ACCESS) @@ -462,8 +470,11 @@ async def check_logged_in(ctx: Context, user_id: str) -> str: scopes=scopes, ) - # Use elicitation to prompt user to login - logger.info(f"Eliciting login for user {user_id} with URL: {auth_url}") + # Use elicitation to prompt user to login. Logged at debug (PR #758 + # round-2 nit 4): the auth URL contains the per-request ``state`` + # token, which is sensitive enough that it shouldn't land in + # multi-tenant log aggregation by default. + logger.debug("Eliciting login for user %s (URL omitted)", user_id) result = await ctx.elicit( message=f"Please log in to Nextcloud at the following URL:\n\n{auth_url}\n\nAfter completing the login, check the box below and click OK.", @@ -472,10 +483,15 @@ async def check_logged_in(ctx: Context, user_id: str) -> str: if result.action == "accept": # Check if login was successful by looking for refresh token - # Strategy: Try multiple lookup methods to handle both flows - logger.info("User accepted login prompt, checking for refresh token") - logger.info(f" State parameter: {state[:16]}...") - logger.info(f" User ID: {user_id}") + # Strategy: Try multiple lookup methods to handle both flows. + # Demoted to debug (PR #758 round-2 nit 4): user_id + state + # appear here on every elicitation accept. + logger.debug( + "User accepted login prompt; looking up refresh token " + "(user_id=%s state=%s...)", + user_id, + state[:16], + ) # First, try to find token by provisioning_client_id (Flow 2 from elicitation) refresh_token_data = ( @@ -483,45 +499,39 @@ async def check_logged_in(ctx: Context, user_id: str) -> str: ) if refresh_token_data: - logger.info("✓ Refresh token found via provisioning_client_id lookup") - logger.info( - f" Flow type: {refresh_token_data.get('flow_type', 'unknown')}" - ) - logger.info( - f" Provisioned at: {refresh_token_data.get('provisioned_at', 'unknown')}" + logger.debug( + "Refresh token found via provisioning_client_id lookup " + "(flow_type=%s provisioned_at=%s)", + refresh_token_data.get("flow_type", "unknown"), + refresh_token_data.get("provisioned_at", "unknown"), ) return "yes" # Fallback: Try to find token by user_id (browser login or any other flow) - logger.info(f"✗ No token found with provisioning_client_id={state[:16]}...") - logger.info(f" Trying fallback lookup by user_id: {user_id}") + logger.debug( + "No token via provisioning_client_id=%s...; falling back to user_id=%s", + state[:16], + user_id, + ) refresh_token_data = await storage.get_refresh_token(user_id) if refresh_token_data: - logger.info("✓ Refresh token found via user_id lookup") - logger.info( - f" Flow type: {refresh_token_data.get('flow_type', 'unknown')}" - ) - logger.info( - f" Provisioned at: {refresh_token_data.get('provisioned_at', 'unknown')}" - ) - logger.info( - f" Provisioning client ID: {refresh_token_data.get('provisioning_client_id', 'NULL')}" - ) - logger.info( - " Note: This token was created via browser login or different flow" + logger.debug( + "Refresh token found via user_id lookup " + "(flow_type=%s provisioned_at=%s provisioning_client_id=%s)", + refresh_token_data.get("flow_type", "unknown"), + refresh_token_data.get("provisioned_at", "unknown"), + refresh_token_data.get("provisioning_client_id", "NULL"), ) return "yes" # No token found by either method - logger.warning(f"✗ No refresh token found for user {user_id}") logger.warning( - f" Checked provisioning_client_id={state[:16]}... - NOT FOUND" - ) - logger.warning(f" Checked user_id={user_id} - NOT FOUND") - logger.warning( - " This may indicate the user completed login but token wasn't stored" + "No refresh token found for user_id=%s (checked provisioning_client_id=%s... and user_id) — " + "user completed elicitation but token wasn't stored", + user_id, + state[:16], ) return ( diff --git a/tests/unit/test_browser_oauth_xss.py b/tests/unit/test_browser_oauth_xss.py index 13903dcf..8b82b483 100644 --- a/tests/unit/test_browser_oauth_xss.py +++ b/tests/unit/test_browser_oauth_xss.py @@ -15,6 +15,7 @@ import httpx import pytest from cryptography.fernet import Fernet +from nextcloud_mcp_server.auth import token_utils from nextcloud_mcp_server.auth.browser_oauth_routes import oauth_login_callback from nextcloud_mcp_server.auth.storage import RefreshTokenStorage @@ -24,6 +25,14 @@ pytestmark = pytest.mark.unit XSS_PAYLOAD = "" +@pytest.fixture(autouse=True) +def _clear_oidc_discovery_cache(): + """Reset the shared discovery cache between tests.""" + token_utils._discovery_cache.clear() + yield + token_utils._discovery_cache.clear() + + @pytest.fixture async def storage(): with tempfile.TemporaryDirectory() as tmpdir: @@ -109,9 +118,18 @@ async def test_callback_escapes_idp_http_error_body(storage): }, ) - with patch( - "nextcloud_mcp_server.auth.browser_oauth_routes.nextcloud_httpx_client", - side_effect=fake_client, + # Discovery now goes through token_utils.get_oidc_discovery (PR #758 + # round-2 nit 3); token-exchange POST still uses browser_oauth_routes' + # httpx client. + with ( + patch( + "nextcloud_mcp_server.auth.browser_oauth_routes.nextcloud_httpx_client", + side_effect=fake_client, + ), + patch( + "nextcloud_mcp_server.auth.token_utils.nextcloud_httpx_client", + side_effect=fake_client, + ), ): response = await oauth_login_callback(request) diff --git a/tests/unit/test_dcr_proxy.py b/tests/unit/test_dcr_proxy.py index 07eed461..621ef299 100644 --- a/tests/unit/test_dcr_proxy.py +++ b/tests/unit/test_dcr_proxy.py @@ -42,7 +42,7 @@ async def test_registration_not_supported_when_no_endpoint(): } with patch( - "nextcloud_mcp_server.auth.oauth_routes._get_cached_discovery", + "nextcloud_mcp_server.auth.oauth_routes.get_oidc_discovery", new_callable=AsyncMock, return_value=discovery_doc, ): diff --git a/tests/unit/test_oauth_callback_session_cleanup.py b/tests/unit/test_oauth_callback_session_cleanup.py index 1f84f678..7ad2efbe 100644 --- a/tests/unit/test_oauth_callback_session_cleanup.py +++ b/tests/unit/test_oauth_callback_session_cleanup.py @@ -109,7 +109,7 @@ async def test_callback_deletes_oauth_session_after_reading_verifier(storage): with ( patch( - "nextcloud_mcp_server.auth.oauth_routes._get_cached_discovery", + "nextcloud_mcp_server.auth.oauth_routes.get_oidc_discovery", new=AsyncMock(return_value=fake_discovery), ), patch( @@ -165,7 +165,7 @@ async def test_callback_no_session_row_does_not_crash(storage): with ( patch( - "nextcloud_mcp_server.auth.oauth_routes._get_cached_discovery", + "nextcloud_mcp_server.auth.oauth_routes.get_oidc_discovery", new=AsyncMock(return_value=fake_discovery), ), patch( @@ -242,7 +242,7 @@ async def test_as_proxy_rejects_invalid_id_token(): with ( patch( - "nextcloud_mcp_server.auth.oauth_routes._get_cached_discovery", + "nextcloud_mcp_server.auth.oauth_routes.get_oidc_discovery", new=AsyncMock(return_value=fake_discovery), ), patch( @@ -264,3 +264,66 @@ async def test_as_proxy_rejects_invalid_id_token(): assert _proxy_codes == {} # And the session has been popped (one-time use). assert server_state not in _as_proxy_sessions + + +async def test_as_proxy_passes_session_nonce_to_verify_id_token(): + """The session-bound nonce must be forwarded as ``expected_nonce``. + + Pins PR #758 round-2 finding 2: ``oauth_authorize`` generates a nonce + and stores it on the ``ASProxySession``; the callback must pass it to + ``verify_id_token`` so an ID token harvested from a parallel auth + request can't be replayed inside the AS-proxy flow. + """ + server_state = "as-proxy-state-with-nonce" + server_nonce = "nonce-bound-to-this-request" + _as_proxy_sessions[server_state] = ASProxySession( + client_id="mcp-client", + client_redirect_uri="http://127.0.0.1:9999/callback", + client_state="client-state", + code_challenge="challenge", + code_challenge_method="S256", + requested_scopes="openid", + nonce=server_nonce, + ) + _proxy_codes.clear() + + request = _build_as_proxy_request(code="auth-code", state=server_state) + + fake_discovery = { + "token_endpoint": "https://idp.example.com/token", + "issuer": "https://idp.example.com", + } + fake_token_response = MagicMock(status_code=200) + fake_token_response.json.return_value = { + "access_token": "ac", + "id_token": "id-tok", + "token_type": "Bearer", + } + fake_http = MagicMock() + fake_http.post = AsyncMock(return_value=fake_token_response) + fake_http.__aenter__ = AsyncMock(return_value=fake_http) + fake_http.__aexit__ = AsyncMock(return_value=None) + + verify_mock = AsyncMock(return_value={"sub": "alice"}) + + with ( + patch( + "nextcloud_mcp_server.auth.oauth_routes.get_oidc_discovery", + new=AsyncMock(return_value=fake_discovery), + ), + patch( + "nextcloud_mcp_server.auth.oauth_routes.nextcloud_httpx_client", + return_value=fake_http, + ), + patch( + "nextcloud_mcp_server.auth.oauth_routes.verify_id_token", + new=verify_mock, + ), + ): + await _oauth_callback_as_proxy(request, server_state) + + verify_mock.assert_awaited_once() + kwargs = verify_mock.await_args.kwargs + assert kwargs.get("expected_nonce") == server_nonce, ( + "AS-proxy callback must forward session.nonce to verify_id_token" + ) diff --git a/tests/unit/test_oidc_discovery.py b/tests/unit/test_oidc_discovery.py index c2459f25..0212e318 100644 --- a/tests/unit/test_oidc_discovery.py +++ b/tests/unit/test_oidc_discovery.py @@ -1,12 +1,12 @@ -"""Unit tests for OIDC discovery fetch in oauth_routes.""" +"""Unit tests for the shared OIDC discovery fetch in token_utils.""" from unittest.mock import patch import httpx import pytest -from nextcloud_mcp_server.auth import oauth_routes -from nextcloud_mcp_server.auth.oauth_routes import _get_cached_discovery +from nextcloud_mcp_server.auth import token_utils +from nextcloud_mcp_server.auth.token_utils import get_oidc_discovery pytestmark = pytest.mark.unit @@ -14,9 +14,9 @@ pytestmark = pytest.mark.unit @pytest.fixture(autouse=True) def _clear_discovery_cache(): """Reset the in-memory discovery cache between tests.""" - oauth_routes._discovery_cache.clear() + token_utils._discovery_cache.clear() yield - oauth_routes._discovery_cache.clear() + token_utils._discovery_cache.clear() async def test_discovery_follows_redirect_to_index_php(): @@ -26,7 +26,8 @@ async def test_discovery_follows_redirect_to_index_php(): redirect ``/.well-known/openid-configuration`` to ``/index.php/.well-known/openid-configuration``. Without follow_redirects the OAuth authorize handler raises HTTPStatusError and returns 500 - (see oauth_routes._get_cached_discovery). + (PR #758 round-2 nit 3 consolidated the discovery cache; see + ``token_utils.get_oidc_discovery``). """ pretty_url = "https://nx.example.com/.well-known/openid-configuration" @@ -51,10 +52,10 @@ async def test_discovery_follows_redirect_to_index_php(): return httpx.AsyncClient(**kwargs) with patch( - "nextcloud_mcp_server.auth.oauth_routes.nextcloud_httpx_client", + "nextcloud_mcp_server.auth.token_utils.nextcloud_httpx_client", side_effect=fake_client, ) as factory: - result = await _get_cached_discovery(pretty_url) + result = await get_oidc_discovery(pretty_url) assert result == discovery_doc factory.assert_called_once() From 9d0e7dcebe0f6df2acdf6e01aeab6d11ed0109ff Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sat, 2 May 2026 22:40:06 +0200 Subject: [PATCH 08/14] fix(auth): address PR #758 round-3 review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Flow 2 (oauth_authorize_nextcloud) now generates a nonce, stores it on the oauth_session row, forwards it to the IdP, and verifies it via expected_nonce in oauth_callback_nextcloud — closes the last replay- protection gap (round-3 finding 1). - _origin_matches_self fails closed when mcp_server_url is missing instead of allowing the logout, and the diagnostic log is promoted from warning to error so the misconfiguration is monitorable (round-3 finding 2). New regression test pins the new behaviour. - The five user_id-accepting helpers in oauth_tools.py (get_provisioning_status, provision_nextcloud_access, revoke_nextcloud_access, check_provisioning_status, check_logged_in) are renamed with leading underscores to make the trust boundary structural rather than documentary (round-3 finding 3). - create_browser_session and delete_browser_session now emit audit_log rows so session establishment / teardown match the pattern used by the rest of the security-relevant storage operations (round-3 nit 5). delete_browser_session selects user_id before delete so the audit row is attributable. - oauth_login_callback no longer reflects raw IdP-error text or exception strings into the HTML failure page; users see a generic "internal error occurred" message + a correlation ID, with the detail logged server-side keyed by the same ID (round-3 nit 6). The XSS regression test is updated to pin the stricter contract. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../auth/browser_oauth_routes.py | 58 ++++++++++++------- nextcloud_mcp_server/auth/oauth_routes.py | 18 +++++- nextcloud_mcp_server/auth/storage.py | 26 +++++++++ nextcloud_mcp_server/server/oauth_tools.py | 39 +++++++------ tests/unit/test_browser_oauth_xss.py | 17 +++++- tests/unit/test_oauth_logout.py | 54 +++++++++++++++-- 6 files changed, 163 insertions(+), 49 deletions(-) diff --git a/nextcloud_mcp_server/auth/browser_oauth_routes.py b/nextcloud_mcp_server/auth/browser_oauth_routes.py index 58bb3d95..18dc12b5 100644 --- a/nextcloud_mcp_server/auth/browser_oauth_routes.py +++ b/nextcloud_mcp_server/auth/browser_oauth_routes.py @@ -66,16 +66,16 @@ def _origin_matches_self(request: Request, oauth_ctx: dict) -> bool: cfg = oauth_ctx.get("config") or oauth_ctx mcp_server_url = cfg.get("mcp_server_url") if not mcp_server_url: - # Mis-configured deployment — fail open rather than break logout, but - # log loudly so the operator can see this is happening (PR #758 - # finding 3). Other OAuth code paths require ``mcp_server_url`` and - # KeyError if it's absent, so this branch should never fire in a - # correctly configured deployment. - logger.warning( - "CSRF check bypassed on /oauth/logout: mcp_server_url not " + # Fail closed (PR #758 round-3 finding 2): a future code path that + # leaves ``mcp_server_url`` unset would otherwise silently disable + # CSRF protection on /oauth/logout. Blocking the logout is + # recoverable — the user just re-logs-in once the misconfiguration + # is fixed — and the error log makes the cause monitorable. + logger.error( + "CSRF check failed on /oauth/logout: mcp_server_url not " "configured in oauth_context — set NEXTCLOUD_MCP_SERVER_URL" ) - return True + return False expected = _normalise_origin(mcp_server_url) raw = request.headers.get("origin") or request.headers.get("referer") @@ -434,14 +434,19 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo token_data = response.json() except httpx.HTTPStatusError as e: + # Correlation IDs let the user reference a specific failure in the + # server logs without us having to reflect raw exception/IdP text + # back into the HTML page (PR #758 round-3 nit 6). + correlation_id = secrets.token_hex(8) error_body = ( e.response.text if hasattr(e.response, "text") else str(e.response.content) ) logger.error( - "Token exchange failed: HTTP %s - %s", e.response.status_code, error_body + "Token exchange failed (correlation_id=%s): HTTP %s - %s", + correlation_id, + e.response.status_code, + error_body, ) - # html_escape: error_body originates from the IdP and could contain - # markup that would be reflected into the failure page otherwise. return HTMLResponse( f""" @@ -449,15 +454,17 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo Login Failed

Login Failed

-

Failed to exchange authorization code for tokens

-

HTTP {e.response.status_code}: {html_escape(error_body)}

+

An internal error occurred while exchanging the authorization code.

+

Correlation ID: {html_escape(correlation_id)}

+

Please try again, or contact your administrator if the problem persists.

""", status_code=500, ) except Exception as e: - logger.error("Token exchange failed: %s", e) + correlation_id = secrets.token_hex(8) + logger.error("Token exchange failed (correlation_id=%s): %s", correlation_id, e) return HTMLResponse( f""" @@ -465,8 +472,9 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo Login Failed

Login Failed

-

Failed to exchange authorization code for tokens

-

Error: {html_escape(str(e))}

+

An internal error occurred while exchanging the authorization code.

+

Correlation ID: {html_escape(correlation_id)}

+

Please try again, or contact your administrator if the problem persists.

""", @@ -508,13 +516,19 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo expected_nonce=nonce, ) except IdTokenVerificationError as e: - logger.error("ID token verification failed: %s", e) - # html_escape: defense-in-depth. The exception text is currently - # server-constructed, but escape on the success path too so any - # future error wrapping that includes IdP response text can't - # smuggle markup into the login-failure page. + # Same correlation-ID pattern as token-exchange failures + # (PR #758 round-3 nit 6) — log the detail server-side and only + # show a generic message + correlation ID in the browser. + correlation_id = secrets.token_hex(8) + logger.error( + "ID token verification failed (correlation_id=%s): %s", + correlation_id, + e, + ) return HTMLResponse( - f"

Login Failed

ID token failed verification: {html_escape(str(e))}

", + f"

Login Failed

" + f"

The ID token failed verification.

" + f"

Correlation ID: {html_escape(correlation_id)}

", status_code=400, ) diff --git a/nextcloud_mcp_server/auth/oauth_routes.py b/nextcloud_mcp_server/auth/oauth_routes.py index db55cb3f..72a62cc5 100644 --- a/nextcloud_mcp_server/auth/oauth_routes.py +++ b/nextcloud_mcp_server/auth/oauth_routes.py @@ -465,7 +465,12 @@ async def oauth_authorize_nextcloud( digest = hashlib.sha256(code_verifier.encode()).digest() code_challenge = urlsafe_b64encode(digest).decode().rstrip("=") - # Store code_verifier in session for retrieval during callback + # OIDC nonce binds the IdP-returned ID token to THIS auth request + # (PR #758 round-3 finding 1). Browser flow + AS proxy already do + # this; Flow 2 is the third path and was missing it. + nonce = secrets.token_urlsafe(32) + + # Store code_verifier + nonce in session for retrieval during callback storage = oauth_ctx["storage"] await storage.store_oauth_session( session_id=state, @@ -475,6 +480,7 @@ async def oauth_authorize_nextcloud( code_challenge=code_challenge, code_challenge_method="S256", mcp_authorization_code=code_verifier, # Store code_verifier here temporarily + nonce=nonce, flow_type="flow2", ttl_seconds=600, # 10 minutes ) @@ -512,6 +518,7 @@ async def oauth_authorize_nextcloud( "response_type": "code", "scope": scopes, "state": state, + "nonce": nonce, "code_challenge": code_challenge, "code_challenge_method": "S256", "prompt": "consent", # Force consent to show resource access @@ -572,12 +579,15 @@ async def oauth_callback_nextcloud(request: Request): storage: RefreshTokenStorage = oauth_ctx["storage"] oauth_config = oauth_ctx["config"] - # Retrieve code_verifier from session storage (PKCE required by Nextcloud OIDC) + # Retrieve code_verifier + nonce from session storage (PKCE + OIDC + # nonce binding both required for Flow 2 — round-3 finding 1). code_verifier = "" + nonce: str | None = None oauth_session = await storage.get_oauth_session(state) if oauth_session: # code_verifier was stored in mcp_authorization_code field code_verifier = oauth_session.get("mcp_authorization_code", "") + nonce = oauth_session.get("nonce") logger.info( f"Retrieved code_verifier for Flow 2 callback (state={state[:16]}...)" ) @@ -636,12 +646,16 @@ async def oauth_callback_nextcloud(request: Request): id_token = token_data.get("id_token") # Verify ID token signature + claims (issue #626 finding 1). + # ``expected_nonce`` is the per-request nonce stored on the + # oauth_session row (PR #758 round-3 finding 1); falsy → skip nonce + # check for sessions written before the column existed. logger.info("oauth_callback_nextcloud: Verifying ID token") try: userinfo = await verify_id_token( id_token, discovery_url=discovery_url, expected_audience=mcp_server_client_id, + expected_nonce=nonce or None, ) except IdTokenVerificationError as e: logger.error("ID token verification failed: %s", e) diff --git a/nextcloud_mcp_server/auth/storage.py b/nextcloud_mcp_server/auth/storage.py index 691c1cc2..517538a3 100644 --- a/nextcloud_mcp_server/auth/storage.py +++ b/nextcloud_mcp_server/auth/storage.py @@ -1178,6 +1178,16 @@ class RefreshTokenStorage: ttl_seconds, ) + # Audit log to match the pattern used by the other security-relevant + # storage operations (PR #758 round-3 nit 5). Browser session + # establishment is a security-relevant event. + await self._audit_log( + event="create_browser_session", + user_id=user_id, + resource_type="browser_session", + resource_id=session_id[:8], + ) + async def get_browser_session_user(self, session_id: str) -> str | None: """Look up the user_id bound to a browser session_id, or None. @@ -1210,7 +1220,16 @@ 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). async with aiosqlite.connect(self.db_path) as db: + async with db.execute( + "SELECT user_id FROM browser_sessions WHERE session_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,) ) @@ -1219,6 +1238,13 @@ class RefreshTokenStorage: if deleted: logger.debug("Deleted browser session %s", session_id[:8]) + if user_id: + await self._audit_log( + event="delete_browser_session", + user_id=user_id, + resource_type="browser_session", + resource_id=session_id[:8], + ) return deleted async def cleanup_expired_browser_sessions(self) -> int: diff --git a/nextcloud_mcp_server/server/oauth_tools.py b/nextcloud_mcp_server/server/oauth_tools.py index ea7ee0b0..f5057736 100644 --- a/nextcloud_mcp_server/server/oauth_tools.py +++ b/nextcloud_mcp_server/server/oauth_tools.py @@ -77,10 +77,15 @@ class LoginConfirmation(BaseModel): ) -async def get_provisioning_status(ctx: Context, user_id: str) -> ProvisioningStatus: +async def _get_provisioning_status(ctx: Context, user_id: str) -> ProvisioningStatus: """ Check the provisioning status for Nextcloud access. + Internal helper — leading underscore signals that ``user_id`` is a + trusted identity claim that callers MUST derive from the verified + access token. The MCP tool wrappers in ``register_oauth_tools`` are + the only legitimate callers (PR #758 round-3 finding 3). + Checks for both credential types: 1. App password from Astrolabe (works today) 2. OAuth refresh token from storage (for future) @@ -200,9 +205,9 @@ def generate_oauth_url_for_flow2( return f"{auth_endpoint}?{urlencode(params)}" -async def provision_nextcloud_access(ctx: Context, user_id: str) -> ProvisioningResult: +async def _provision_nextcloud_access(ctx: Context, user_id: str) -> ProvisioningResult: """ - MCP Tool: Provision offline access to Nextcloud resources. + Internal helper for the ``provision_nextcloud_access`` MCP tool. Returns URL to Astrolabe settings page where users can provision background sync access using either: @@ -219,7 +224,7 @@ async def provision_nextcloud_access(ctx: Context, user_id: str) -> Provisioning """ try: # Check if already provisioned - status = await get_provisioning_status(ctx, user_id) + status = await _get_provisioning_status(ctx, user_id) if status.is_provisioned: return ProvisioningResult( success=True, @@ -268,9 +273,9 @@ async def provision_nextcloud_access(ctx: Context, user_id: str) -> Provisioning ) -async def revoke_nextcloud_access(ctx: Context, user_id: str) -> RevocationResult: +async def _revoke_nextcloud_access(ctx: Context, user_id: str) -> RevocationResult: """ - MCP Tool: Revoke offline access to Nextcloud resources. + Internal helper for the ``revoke_nextcloud_access`` MCP tool. This tool removes the stored refresh token and revokes access that was granted via Flow 2. @@ -285,7 +290,7 @@ async def revoke_nextcloud_access(ctx: Context, user_id: str) -> RevocationResul """ try: # Check current status - status = await get_provisioning_status(ctx, user_id) + status = await _get_provisioning_status(ctx, user_id) if not status.is_provisioned: return RevocationResult( success=True, @@ -339,9 +344,9 @@ async def revoke_nextcloud_access(ctx: Context, user_id: str) -> RevocationResul ) -async def check_provisioning_status(ctx: Context, user_id: str) -> ProvisioningStatus: +async def _check_provisioning_status(ctx: Context, user_id: str) -> ProvisioningStatus: """ - MCP Tool: Check the current provisioning status. + Internal helper for the ``check_provisioning_status`` MCP tool. This tool allows users to check whether they have provisioned Nextcloud access and see details about their current authorization. @@ -354,12 +359,12 @@ async def check_provisioning_status(ctx: Context, user_id: str) -> ProvisioningS Returns: ProvisioningStatus with current state """ - return await get_provisioning_status(ctx, user_id) + return await _get_provisioning_status(ctx, user_id) -async def check_logged_in(ctx: Context, user_id: str) -> str: +async def _check_logged_in(ctx: Context, user_id: str) -> str: """ - MCP Tool: Check if user is logged in and elicit login if needed. + Internal helper for the ``check_logged_in`` MCP tool. This tool checks whether the user has completed Flow 2 (resource provisioning) to grant offline access to Nextcloud. If not logged in, it uses MCP elicitation @@ -378,7 +383,7 @@ async def check_logged_in(ctx: Context, user_id: str) -> str: # ends up in log aggregation on every check_logged_in call, which is # noise in a hosted multi-tenant deployment. logger.debug("Checking provisioning status for user_id=%s", user_id) - status = await get_provisioning_status(ctx, user_id) + status = await _get_provisioning_status(ctx, user_id) logger.debug( " Provisioning status for %s: is_provisioned=%s", user_id, @@ -568,7 +573,7 @@ def register_oauth_tools(mcp): @require_scopes("openid") async def tool_provision_access(ctx: Context) -> ProvisioningResult: user_id = await extract_user_id_from_token(ctx) - return await provision_nextcloud_access(ctx, user_id) + return await _provision_nextcloud_access(ctx, user_id) @mcp.tool( name="revoke_nextcloud_access", @@ -583,7 +588,7 @@ def register_oauth_tools(mcp): @require_scopes("openid") async def tool_revoke_access(ctx: Context) -> RevocationResult: user_id = await extract_user_id_from_token(ctx) - return await revoke_nextcloud_access(ctx, user_id) + return await _revoke_nextcloud_access(ctx, user_id) @mcp.tool( name="check_provisioning_status", @@ -597,7 +602,7 @@ def register_oauth_tools(mcp): @require_scopes("openid") async def tool_check_status(ctx: Context) -> ProvisioningStatus: user_id = await extract_user_id_from_token(ctx) - return await check_provisioning_status(ctx, user_id) + return await _check_provisioning_status(ctx, user_id) @mcp.tool( name="check_logged_in", @@ -614,4 +619,4 @@ def register_oauth_tools(mcp): @require_scopes("openid") async def tool_check_logged_in(ctx: Context) -> str: user_id = await extract_user_id_from_token(ctx) - return await check_logged_in(ctx, user_id) + return await _check_logged_in(ctx, user_id) diff --git a/tests/unit/test_browser_oauth_xss.py b/tests/unit/test_browser_oauth_xss.py index 8b82b483..8411eb36 100644 --- a/tests/unit/test_browser_oauth_xss.py +++ b/tests/unit/test_browser_oauth_xss.py @@ -70,8 +70,14 @@ async def test_callback_escapes_error_query_params(storage): assert "<script>alert(1)</script>" in body -async def test_callback_escapes_idp_http_error_body(storage): - """IdP-returned HTTPError body must be HTML-escaped before reflection.""" +async def test_callback_does_not_reflect_idp_http_error_body(storage): + """IdP-returned HTTPError body must not appear in the user-visible HTML. + + Updated for PR #758 round-3 nit 6: the callback now logs the IdP + response server-side and shows the user only a generic message + a + correlation ID, eliminating reflection of attacker-controllable text + into the error page entirely. + """ discovery = {"token_endpoint": "http://idp.example/token"} def handler(request: httpx.Request) -> httpx.Response: @@ -135,5 +141,10 @@ async def test_callback_escapes_idp_http_error_body(storage): body = response.body.decode() assert response.status_code == 500 + # Strict: neither the raw payload nor an HTML-escaped form of the + # IdP body should appear — the page must show only the generic + # message + correlation ID. assert XSS_PAYLOAD not in body - assert "<script>alert(1)</script>" in body + assert "<script>alert(1)</script>" not in body + assert "An internal error occurred" in body + assert "Correlation ID" in body diff --git a/tests/unit/test_oauth_logout.py b/tests/unit/test_oauth_logout.py index 095f81c1..0aad07e0 100644 --- a/tests/unit/test_oauth_logout.py +++ b/tests/unit/test_oauth_logout.py @@ -92,7 +92,13 @@ async def test_logout_deletes_refresh_token_and_session(storage): request = _build_request( cookie="sid-1", - oauth_context={"storage": storage, "config": {"discovery_url": None}}, + oauth_context={ + "storage": storage, + "config": { + "mcp_server_url": "https://mcp.example.com", + "discovery_url": None, + }, + }, ) with patch( @@ -118,7 +124,10 @@ async def test_logout_calls_revocation_when_refresh_token_present(storage): cookie="sid-2", oauth_context={ "storage": storage, - "config": {"discovery_url": "http://idp/.well-known"}, + "config": { + "mcp_server_url": "https://mcp.example.com", + "discovery_url": "http://idp/.well-known", + }, }, ) @@ -138,7 +147,13 @@ async def test_logout_no_session_cookie_returns_302(storage): """Without a cookie, logout still 302s and doesn't touch storage.""" request = _build_request( cookie=None, - oauth_context={"storage": storage, "config": {"discovery_url": None}}, + oauth_context={ + "storage": storage, + "config": { + "mcp_server_url": "https://mcp.example.com", + "discovery_url": None, + }, + }, ) response = await oauth_logout(request) assert response.status_code == 302 @@ -157,7 +172,10 @@ async def test_logout_swallows_storage_errors(storage): cookie="sid-3", oauth_context={ "storage": broken_storage, - "config": {"discovery_url": None}, + "config": { + "mcp_server_url": "https://mcp.example.com", + "discovery_url": None, + }, }, ) response = await oauth_logout(request) @@ -295,6 +313,26 @@ async def test_logout_allows_referer_when_origin_missing(storage): assert response.status_code == 302 +async def test_logout_blocked_when_mcp_server_url_missing(storage): + """Fail-closed CSRF (PR #758 round-3 finding 2): missing ``mcp_server_url`` + in oauth_ctx must reject the logout, not allow it. + + A future code path that leaves ``mcp_server_url`` unset would + otherwise silently disable CSRF protection. Blocking is recoverable. + """ + await storage.create_browser_session(session_id="sid-MM", user_id="alice") + + request = _build_request( + cookie="sid-MM", + oauth_context={"storage": storage, "config": {"discovery_url": None}}, + ) + + response = await oauth_logout(request) + assert response.status_code == 403 + # Session must NOT have been deleted. + assert await storage.get_browser_session_user("sid-MM") == "alice" + + async def test_logout_handles_session_with_no_refresh_token(storage): """Cookie + session row exist but refresh token already gone — logout is idempotent.""" await storage.create_browser_session(session_id="sid-4", user_id="dave") @@ -302,7 +340,13 @@ async def test_logout_handles_session_with_no_refresh_token(storage): revoke = AsyncMock() request = _build_request( cookie="sid-4", - oauth_context={"storage": storage, "config": {"discovery_url": None}}, + oauth_context={ + "storage": storage, + "config": { + "mcp_server_url": "https://mcp.example.com", + "discovery_url": None, + }, + }, ) with patch( "nextcloud_mcp_server.auth.browser_oauth_routes._revoke_refresh_token_at_idp", From 3a4fa8adc89059cab82ef21f070ec2327c914d37 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sat, 2 May 2026 23:14:07 +0200 Subject: [PATCH 09/14] fix(auth): address PR #758 round-3 final review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seven findings from the latest review on #758, plus a regression test catching the substance of the cache-stampede fix: - verify_id_token: widen id_token annotation to str | None to match callers passing nc_token_response.get("id_token") - extract_user_id_from_token: use JSON-RPC reserved error code -32001 instead of -1 - _get_cached: per-URL anyio.Lock dict + meta-lock coalesces concurrent cache misses into a single IdP fetch (mirrors token_broker.py idiom) - delete_browser_session: collapse SELECT+DELETE into atomic DELETE ... RETURNING user_id (SQLite >= 3.35) - new test_origin_normalise.py: parametrized port/scheme/host equivalence cases for the CSRF Origin guard - browser_oauth_routes: correct misleading "PR #758 finding 5" cross- references (finding 5 was Fernet-key hardening, not CSRF) - ASProxySession.nonce: make required, drop spurious "legacy session" default; reword the in-flight `or None` comment to reflect that ASProxySession is purely in-memory - new test_get_cached_coalesces_concurrent_misses: pins the cache-stampede protection — fires 10 concurrent _get_cached calls and asserts exactly one HTTP fetch Co-Authored-By: Claude Opus 4.7 (1M context) --- .../auth/browser_oauth_routes.py | 9 +-- nextcloud_mcp_server/auth/oauth_routes.py | 9 +-- nextcloud_mcp_server/auth/storage.py | 17 +++--- nextcloud_mcp_server/auth/token_utils.py | 55 ++++++++++++++----- tests/unit/test_id_token_verification.py | 50 +++++++++++++++++ .../test_oauth_callback_session_cleanup.py | 1 + tests/unit/test_origin_normalise.py | 41 ++++++++++++++ 7 files changed, 152 insertions(+), 30 deletions(-) create mode 100644 tests/unit/test_origin_normalise.py 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 From b696541918a3621ff3afbdffee17e5a94d0fd0c5 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sun, 3 May 2026 00:57:12 +0200 Subject: [PATCH 10/14] 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)}" + ) From e2955e8246db17ec94e12dcb40711875d89b86b7 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sun, 3 May 2026 01:37:38 +0200 Subject: [PATCH 11/14] fix(auth): address PR #758 round-5 medium/low review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings from the latest review on #758 (1 medium, 2 low): Medium: - browser_oauth_routes.oauth_logout: move delete_browser_session into a finally block so an error from delete_refresh_token can no longer leave an orphan browser_sessions row. The orphan was not exploitable (SessionAuthBackend rejects sessions without a live refresh token), but it lingered until the hourly cleanup cron — a correctness gap. New regression test pins the fix. Low: - oauth_callback_nextcloud: drop redundant ``or None`` from ``expected_nonce=nonce``. ``nonce`` is already ``str | None`` and ``secrets.token_urlsafe`` never produces an empty string, so the coercion was a no-op that could mislead future readers into thinking empty-string was a valid skip-the-check path. - storage.RefreshTokenStorage.initialize: fail fast at startup when SQLite < 3.35, since ``DELETE ... RETURNING`` (used in ``delete_browser_session``) needs that minimum. Ubuntu 20.04 ships 3.31 and would otherwise hit OperationalError on every logout. Prerequisite also documented in docs/installation.md. Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/installation.md | 1 + .../auth/browser_oauth_routes.py | 15 +++++- nextcloud_mcp_server/auth/oauth_routes.py | 9 ++-- nextcloud_mcp_server/auth/storage.py | 16 ++++++ tests/unit/test_oauth_logout.py | 50 +++++++++++++++++++ 5 files changed, 86 insertions(+), 5 deletions(-) diff --git a/docs/installation.md b/docs/installation.md index 0f99af62..d855fc5a 100644 --- a/docs/installation.md +++ b/docs/installation.md @@ -5,6 +5,7 @@ This guide covers installing the Nextcloud MCP server on your system. ## Prerequisites - **Python 3.11+** - Check with `python3 --version` +- **SQLite 3.35+** - Check with `python3 -c "import sqlite3; print(sqlite3.sqlite_version)"`. The OAuth session storage uses `DELETE ... RETURNING`, which is only available from SQLite 3.35 (March 2021). Ubuntu 20.04 ships SQLite 3.31 and is **not** supported; upgrade the host or run from the Docker image, which bundles a newer libsqlite3. - **Access to a Nextcloud instance** - Self-hosted or cloud-hosted - **Administrator access** *(optional)* - Only needed to customise app-password policies in Nextcloud settings; not required for any deployment mode (single-user, multi-user BasicAuth, or Login Flow v2) diff --git a/nextcloud_mcp_server/auth/browser_oauth_routes.py b/nextcloud_mcp_server/auth/browser_oauth_routes.py index 3da0864a..cccb79c9 100644 --- a/nextcloud_mcp_server/auth/browser_oauth_routes.py +++ b/nextcloud_mcp_server/auth/browser_oauth_routes.py @@ -683,11 +683,22 @@ async def oauth_logout(request: Request) -> RedirectResponse | JSONResponse: await _revoke_refresh_token_at_idp(oauth_ctx, refresh_token) await storage.delete_refresh_token(user_id) logger.info("Refresh token revoked + deleted for user %s", user_id) - - await storage.delete_browser_session(session_id) except Exception as e: # Logout must always succeed locally; log and continue. logger.warning("Logout cleanup failed (continuing): %s", e) + finally: + # Always drop the browser_sessions row, even when the + # refresh-token cleanup above failed — otherwise an orphan + # row lingers until the hourly cleanup cron (PR #758 round-5 + # review medium 1). Not exploitable (SessionAuthBackend + # already rejects sessions without a live refresh token), but + # a correctness gap worth closing here. + try: + await storage.delete_browser_session(session_id) + except Exception as e: + logger.warning( + "Failed to delete browser session %s…: %s", session_id[:8], e + ) response = RedirectResponse(next_url, status_code=302) response.delete_cookie("mcp_session") diff --git a/nextcloud_mcp_server/auth/oauth_routes.py b/nextcloud_mcp_server/auth/oauth_routes.py index 0ee6a697..d28e3e67 100644 --- a/nextcloud_mcp_server/auth/oauth_routes.py +++ b/nextcloud_mcp_server/auth/oauth_routes.py @@ -647,15 +647,18 @@ async def oauth_callback_nextcloud(request: Request): # Verify ID token signature + claims (issue #626 finding 1). # ``expected_nonce`` is the per-request nonce stored on the - # oauth_session row (PR #758 round-3 finding 1); falsy → skip nonce - # check for sessions written before the column existed. + # oauth_session row (PR #758 round-3 finding 1). ``nonce`` is already + # ``str | None`` and ``secrets.token_urlsafe`` never produces an empty + # string, so passing it directly is correct — pre-migration-006 rows + # surface as ``None`` from ``oauth_session.get("nonce")``, which + # ``verify_id_token`` already treats as "skip the check". logger.info("oauth_callback_nextcloud: Verifying ID token") try: userinfo = await verify_id_token( id_token, discovery_url=discovery_url, expected_audience=mcp_server_client_id, - expected_nonce=nonce or None, + expected_nonce=nonce, ) except IdTokenVerificationError as e: logger.error("ID token verification failed: %s", e) diff --git a/nextcloud_mcp_server/auth/storage.py b/nextcloud_mcp_server/auth/storage.py index f26cb068..e7e8d0b3 100644 --- a/nextcloud_mcp_server/auth/storage.py +++ b/nextcloud_mcp_server/auth/storage.py @@ -29,6 +29,7 @@ import json import logging import os import socket +import sqlite3 import time from pathlib import Path from typing import Any @@ -139,10 +140,25 @@ class RefreshTokenStorage: 1. New database: Run migrations from scratch 2. Pre-Alembic database: Stamp with initial revision (no changes) 3. Alembic-managed database: Upgrade to latest version + + Raises: + RuntimeError: when the underlying SQLite library is older than + 3.35, which is required for ``DELETE ... RETURNING`` used by + ``delete_browser_session`` (PR #758 round-5 review low 2). + Ubuntu 20.04 ships SQLite 3.31, so deployers on that + baseline must upgrade or use a newer Python image. """ if self._initialized: return + if sqlite3.sqlite_version_info < (3, 35): + raise RuntimeError( + "SQLite >= 3.35 is required (DELETE ... RETURNING is used " + "by delete_browser_session); detected " + f"{sqlite3.sqlite_version}. Upgrade SQLite or use a Python " + "image with a newer bundled libsqlite3." + ) + # Ensure directory exists db_dir = Path(self.db_path).parent db_dir.mkdir(parents=True, exist_ok=True) diff --git a/tests/unit/test_oauth_logout.py b/tests/unit/test_oauth_logout.py index 0aad07e0..9eee223d 100644 --- a/tests/unit/test_oauth_logout.py +++ b/tests/unit/test_oauth_logout.py @@ -182,6 +182,56 @@ async def test_logout_swallows_storage_errors(storage): assert response.status_code == 302 # logout still succeeds +async def test_logout_deletes_session_when_refresh_token_delete_fails(storage): + """Browser session row must be removed even if delete_refresh_token raises. + + Pins PR #758 round-5 review medium 1: previously the two deletes lived + in the same try-block, so an error on ``delete_refresh_token`` left an + orphan ``browser_sessions`` row that lingered until the cleanup cron. + """ + await storage.create_browser_session(session_id="sid-orphan", user_id="dave") + await storage.store_refresh_token( + user_id="dave", refresh_token="rt-dave", flow_type="browser" + ) + + real_delete_refresh_token = storage.delete_refresh_token + real_delete_browser_session = storage.delete_browser_session + + storage.delete_refresh_token = AsyncMock(side_effect=RuntimeError("boom")) + delete_browser_session_calls: list[str] = [] + + async def tracking_delete_browser_session(session_id: str) -> bool: + delete_browser_session_calls.append(session_id) + return await real_delete_browser_session(session_id) + + storage.delete_browser_session = tracking_delete_browser_session + + request = _build_request( + cookie="sid-orphan", + oauth_context={ + "storage": storage, + "config": { + "mcp_server_url": "https://mcp.example.com", + "discovery_url": None, + }, + }, + ) + + try: + response = await oauth_logout(request) + finally: + storage.delete_refresh_token = real_delete_refresh_token + storage.delete_browser_session = real_delete_browser_session + + assert response.status_code == 302 + assert delete_browser_session_calls == ["sid-orphan"], ( + "delete_browser_session must run even after delete_refresh_token raised" + ) + assert await storage.get_browser_session_user("sid-orphan") is None, ( + "browser_sessions row must be gone — finally branch failed to fire" + ) + + async def test_logout_blocks_cross_origin_post(storage): """POST from a foreign Origin must be rejected with 403 (PR #758 finding 5).""" await storage.create_browser_session(session_id="sid-X", user_id="alice") From ec9b9b2a75d052174b5cbbbbb85d5f3d67f8af59 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sun, 3 May 2026 13:03:49 +0200 Subject: [PATCH 12/14] fix(auth): address PR #758 round-6 medium/low review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five findings from the latest review on #758 (2 medium, 3 nit): Medium: - browser_oauth_routes.oauth_login_callback + oauth_routes.oauth_callback_nextcloud: fail closed with 400 when the oauth_session row is unknown/expired. Previously both callbacks fell through with code_verifier="" and expected_nonce=None, silently bypassing the PKCE + nonce protections introduced in earlier rounds. Symmetric unit tests pin both contracts. - token_utils.verify_id_token: use secrets.compare_digest for the nonce check instead of short-circuit !=. Mirrors the sibling PKCE verifier comparison; closes the last secret-equality timing-side-channel surface in the auth path. Nit: - Tighten the comment at all 4 mcp_authorization_code/code_verifier store + retrieve sites so a future refactor sees the field reuse immediately (renaming the column requires a schema migration). - _should_use_secure_cookies: explicit string normalisation instead of bool(settings.cookie_secure). Dynaconf normally coerces but tests / direct settings.set calls can leave the raw string in place — bool("false") is True. New parametrized unit tests cover the coercion matrix + http/https fallback. - oauth_routes.py:591 f-string log converted to lazy %s formatting (folded into the Flow 2 callback rewrite). Co-Authored-By: Claude Opus 4.7 (1M context) --- .../auth/browser_oauth_routes.py | 59 ++++++++------ nextcloud_mcp_server/auth/oauth_routes.py | 42 ++++++---- nextcloud_mcp_server/auth/token_utils.py | 9 ++- tests/unit/test_browser_oauth_routes.py | 73 ++++++++++++++++++ .../test_oauth_callback_session_cleanup.py | 76 +++++++++---------- 5 files changed, 182 insertions(+), 77 deletions(-) create mode 100644 tests/unit/test_browser_oauth_routes.py diff --git a/nextcloud_mcp_server/auth/browser_oauth_routes.py b/nextcloud_mcp_server/auth/browser_oauth_routes.py index cccb79c9..7e3d78fa 100644 --- a/nextcloud_mcp_server/auth/browser_oauth_routes.py +++ b/nextcloud_mcp_server/auth/browser_oauth_routes.py @@ -115,10 +115,15 @@ def _should_use_secure_cookies() -> bool: 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) + raw = settings.cookie_secure + if raw is not None: + # Dynaconf normally coerces "true"/"false"/"1"/"0", but tests or + # direct ``settings.set`` calls can bypass that — bool("false") is + # True. Normalise explicitly so an unexpected string never flips + # cookies to Secure on plain HTTP (round-6 review). + if isinstance(raw, bool): + return raw + return str(raw).strip().lower() not in ("0", "false", "no", "off", "") mcp_server_url = settings.nextcloud_mcp_server_url or "" return mcp_server_url.startswith("https://") @@ -189,7 +194,10 @@ async def oauth_login(request: Request) -> RedirectResponse | JSONResponse: state=state, code_challenge=code_challenge, code_challenge_method="S256", - mcp_authorization_code=code_verifier, # Store code_verifier here temporarily + # `mcp_authorization_code` field reused to store the PKCE + # code_verifier (one-time-use). Renaming the column requires a + # schema migration. + mcp_authorization_code=code_verifier, nonce=nonce, flow_type="browser", ttl_seconds=600, # 10 minutes @@ -355,25 +363,30 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo oauth_client = oauth_ctx["oauth_client"] oauth_config = oauth_ctx["config"] - # Retrieve code_verifier, nonce, and redirect URL from session storage - code_verifier = "" - nonce: str | None = None - next_url = "/app" # Default redirect + # Retrieve code_verifier, nonce, and redirect URL from session storage. + # Fail closed when the row is missing/expired: otherwise PKCE + + # nonce verification silently degrade to no-ops (round-6 review). oauth_session = await storage.get_oauth_session(state) - if oauth_session: - # code_verifier was stored in mcp_authorization_code field - code_verifier = oauth_session.get("mcp_authorization_code", "") - # nonce bound to this auth request — verified against the ID token - # below (PR #758 finding 2). - nonce = oauth_session.get("nonce") - # next_url was stored in client_redirect_uri field — re-validate at - # 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") - # 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) + if not oauth_session: + logger.warning("OAuth callback received unknown/expired state=%s", state[:16]) + return HTMLResponse( + "Unknown or expired session — please try logging in again.", + status_code=400, + ) + # `mcp_authorization_code` field reused to store the PKCE code_verifier + # (one-time-use). Renaming the column requires a schema migration. + code_verifier = oauth_session.get("mcp_authorization_code", "") + # nonce bound to this auth request — verified against the ID token + # below (PR #758 finding 2). + nonce = oauth_session.get("nonce") + # next_url was stored in client_redirect_uri field — re-validate at + # 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") + # 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/oauth_routes.py b/nextcloud_mcp_server/auth/oauth_routes.py index d28e3e67..7dbb56ef 100644 --- a/nextcloud_mcp_server/auth/oauth_routes.py +++ b/nextcloud_mcp_server/auth/oauth_routes.py @@ -479,7 +479,10 @@ async def oauth_authorize_nextcloud( state=state, code_challenge=code_challenge, code_challenge_method="S256", - mcp_authorization_code=code_verifier, # Store code_verifier here temporarily + # `mcp_authorization_code` field reused to store the PKCE + # code_verifier (one-time-use). Renaming the column requires a + # schema migration. + mcp_authorization_code=code_verifier, nonce=nonce, flow_type="flow2", ttl_seconds=600, # 10 minutes @@ -580,22 +583,31 @@ async def oauth_callback_nextcloud(request: Request): oauth_config = oauth_ctx["config"] # Retrieve code_verifier + nonce from session storage (PKCE + OIDC - # nonce binding both required for Flow 2 — round-3 finding 1). - code_verifier = "" - nonce: str | None = None + # nonce binding both required for Flow 2 — round-3 finding 1). Fail + # closed when the row is missing/expired so PKCE + nonce verification + # are not silently bypassed (round-6 review). oauth_session = await storage.get_oauth_session(state) - if oauth_session: - # code_verifier was stored in mcp_authorization_code field - code_verifier = oauth_session.get("mcp_authorization_code", "") - nonce = oauth_session.get("nonce") - logger.info( - f"Retrieved code_verifier for Flow 2 callback (state={state[:16]}...)" + if not oauth_session: + logger.warning("Flow 2 callback received unknown/expired state=%s", state[:16]) + return JSONResponse( + { + "error": "invalid_request", + "error_description": ( + "Unknown or expired session — please retry the OAuth flow" + ), + }, + status_code=400, ) - # One-time-use session: delete eagerly so the stored code_verifier - # can't be replayed for the remainder of the oauth_sessions TTL. - # Mirrors browser_oauth_routes.oauth_login_callback (PR #758 - # follow-up review). - await storage.delete_oauth_session(state) + # `mcp_authorization_code` field reused to store the PKCE code_verifier + # (one-time-use). Renaming the column requires a schema migration. + code_verifier = oauth_session.get("mcp_authorization_code", "") + nonce = oauth_session.get("nonce") + logger.info("Retrieved code_verifier for Flow 2 callback (state=%s…)", state[:16]) + # One-time-use session: delete eagerly so the stored code_verifier + # can't be replayed for the remainder of the oauth_sessions TTL. + # Mirrors browser_oauth_routes.oauth_login_callback (PR #758 + # follow-up review). + await storage.delete_oauth_session(state) # Exchange code for tokens mcp_server_client_id = os.getenv( diff --git a/nextcloud_mcp_server/auth/token_utils.py b/nextcloud_mcp_server/auth/token_utils.py index db1a492a..c7edb960 100644 --- a/nextcloud_mcp_server/auth/token_utils.py +++ b/nextcloud_mcp_server/auth/token_utils.py @@ -5,6 +5,7 @@ between server/ and auth/ layers. """ import logging +import secrets import time from typing import Any @@ -232,7 +233,13 @@ async def verify_id_token( f"Unexpected error verifying ID token: {e}" ) from e - if expected_nonce is not None and payload.get("nonce") != expected_nonce: + # Constant-time comparison mirrors the PKCE verifier check + # (oauth_routes.py:1029) — short-circuit `!=` is avoided in + # security-sensitive equality even when the secret is server-generated + # (round-6 review). + if expected_nonce is not None and not secrets.compare_digest( + payload.get("nonce", "") or "", expected_nonce + ): raise IdTokenVerificationError("ID token nonce does not match request nonce") return payload diff --git a/tests/unit/test_browser_oauth_routes.py b/tests/unit/test_browser_oauth_routes.py new file mode 100644 index 00000000..0c86bb04 --- /dev/null +++ b/tests/unit/test_browser_oauth_routes.py @@ -0,0 +1,73 @@ +"""Unit tests for ``browser_oauth_routes`` helpers. + +Pins the round-6 review fix that ``_should_use_secure_cookies`` must not +trust ``bool(settings.cookie_secure)`` — Dynaconf normally coerces but +tests / direct ``settings.set`` calls can leave the raw string in place, +and ``bool("false")`` is ``True``. +""" + +import pytest + +from nextcloud_mcp_server.auth import browser_oauth_routes + +pytestmark = pytest.mark.unit + + +def _fake_settings(*, cookie_secure, mcp_server_url=""): + return type( + "S", + (), + { + "cookie_secure": cookie_secure, + "nextcloud_mcp_server_url": mcp_server_url, + }, + )() + + +@pytest.mark.parametrize( + "value,expected", + [ + (True, True), + (False, False), + ("true", True), + ("false", False), + ("True", True), + ("FALSE", False), + ("0", False), + ("1", True), + ("no", False), + ("yes", True), + ("off", False), + ("on", True), + ("", False), + ], +) +def test_should_use_secure_cookies_string_coercion(monkeypatch, value, expected): + monkeypatch.setattr( + browser_oauth_routes, + "get_settings", + lambda: _fake_settings(cookie_secure=value), + ) + assert browser_oauth_routes._should_use_secure_cookies() is expected + + +def test_should_use_secure_cookies_falls_back_to_https_scheme(monkeypatch): + monkeypatch.setattr( + browser_oauth_routes, + "get_settings", + lambda: _fake_settings( + cookie_secure=None, mcp_server_url="https://mcp.example.com" + ), + ) + assert browser_oauth_routes._should_use_secure_cookies() is True + + +def test_should_use_secure_cookies_falls_back_to_http_scheme(monkeypatch): + monkeypatch.setattr( + browser_oauth_routes, + "get_settings", + lambda: _fake_settings( + cookie_secure=None, mcp_server_url="http://localhost:8000" + ), + ) + assert browser_oauth_routes._should_use_secure_cookies() is False diff --git a/tests/unit/test_oauth_callback_session_cleanup.py b/tests/unit/test_oauth_callback_session_cleanup.py index 8b54fc8c..7b666a2c 100644 --- a/tests/unit/test_oauth_callback_session_cleanup.py +++ b/tests/unit/test_oauth_callback_session_cleanup.py @@ -20,10 +20,10 @@ import tempfile from pathlib import Path from unittest.mock import AsyncMock, MagicMock, patch -import httpx import pytest from cryptography.fernet import Fernet +from nextcloud_mcp_server.auth.browser_oauth_routes import oauth_login_callback from nextcloud_mcp_server.auth.oauth_routes import ( ASProxySession, _as_proxy_sessions, @@ -136,51 +136,51 @@ async def test_callback_deletes_oauth_session_after_reading_verifier(storage): ) -async def test_callback_no_session_row_does_not_crash(storage): - """If the row is already gone (e.g. expired), the callback proceeds.""" +async def test_callback_unknown_state_returns_400(storage): + """Unknown/expired state must fail closed with 400. + + Pins the PR #758 round-6 review fix: previously the callback fell + through with empty ``code_verifier`` / ``expected_nonce=None``, + silently bypassing the PKCE + nonce protections introduced in earlier + rounds. The handler now returns 400 before any token exchange. + """ state = "state-missing" # No store_oauth_session call — the row never existed. request = _build_request(code="idp-auth-code", state=state, storage=storage) - fake_discovery = { - "token_endpoint": "https://idp.example.com/token", - "userinfo_endpoint": "https://idp.example.com/userinfo", - "issuer": "https://idp.example.com", + response = await oauth_callback_nextcloud(request) + + assert response.status_code == 400 + assert await storage.get_oauth_session(state) is None + + +async def test_browser_callback_unknown_state_returns_400(storage): + """Symmetric unknown-state contract for the browser-flow callback. + + Mirrors ``test_callback_unknown_state_returns_400`` for + ``oauth_login_callback`` — both callbacks must fail closed when the + oauth_session row is missing/expired (PR #758 round-6 review). + """ + state = "state-missing-browser" + + request = MagicMock() + request.query_params = {"code": "idp-auth-code", "state": state} + request.cookies = {} + request.url_for = MagicMock(return_value="/oauth/login") + request.app.state.oauth_context = { + "storage": storage, + "oauth_client": None, # Nextcloud-integrated mode + "config": { + "mcp_server_url": "https://mcp.example.com", + "client_id": "mcp-server", + "client_secret": "mcp-secret", + }, } - fake_token_response = MagicMock() - fake_token_response.json.return_value = {"access_token": "ac"} - fake_token_response.raise_for_status = MagicMock( - side_effect=httpx.HTTPStatusError( - "boom", - request=MagicMock(), - response=MagicMock(status_code=400), - ) - ) - fake_http = MagicMock() - fake_http.post = AsyncMock(return_value=fake_token_response) - fake_http.__aenter__ = AsyncMock(return_value=fake_http) - fake_http.__aexit__ = AsyncMock(return_value=None) + response = await oauth_login_callback(request) - with ( - patch( - "nextcloud_mcp_server.auth.oauth_routes.get_oidc_discovery", - new=AsyncMock(return_value=fake_discovery), - ), - patch( - "nextcloud_mcp_server.auth.oauth_routes.nextcloud_httpx_client", - return_value=fake_http, - ), - ): - # We don't care what happens past the deletion — just that the - # missing-row branch doesn't try to delete a nonexistent session. - try: - await oauth_callback_nextcloud(request) - except Exception: - pass - - # No crash, no row, no surprises. + assert response.status_code == 400 assert await storage.get_oauth_session(state) is None From 27fcf05d3a9e1a1fe49ebcff927b8353678792bd Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sun, 3 May 2026 13:36:36 +0200 Subject: [PATCH 13/14] fix(auth): address PR #758 round-7 important review - Coerce refresh_expires_in to int before arithmetic in both callback paths so IdPs that serialize the field as a JSON string (e.g. AWS Cognito) don't trigger an unhandled TypeError 500. - Drop the orphaned oauth_session row written by _check_logged_in. The canonical Flow 2 row is created by generate_oauth_url_for_flow2 keyed by `state`, which is what the unified callback looks up; the flow2_ session_id was never matched and just churned the table for 10 minutes per call. - Match delete_cookie attributes (httponly, secure, samesite) to the set_cookie call on logout so browsers reliably evict the cookie even on implementations that consider security flags during deletion. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../auth/browser_oauth_routes.py | 14 ++++++++++++-- nextcloud_mcp_server/auth/oauth_routes.py | 4 +++- nextcloud_mcp_server/server/oauth_tools.py | 17 ++++------------- 3 files changed, 19 insertions(+), 16 deletions(-) diff --git a/nextcloud_mcp_server/auth/browser_oauth_routes.py b/nextcloud_mcp_server/auth/browser_oauth_routes.py index 7e3d78fa..c155c168 100644 --- a/nextcloud_mcp_server/auth/browser_oauth_routes.py +++ b/nextcloud_mcp_server/auth/browser_oauth_routes.py @@ -554,7 +554,9 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo refresh_expires_in = token_data.get("refresh_expires_in") refresh_expires_at = None if refresh_expires_in: - refresh_expires_at = int(time.time()) + refresh_expires_in + # Some IdPs (e.g. AWS Cognito) return refresh_expires_in as a JSON + # string rather than an int; coerce to be safe. + refresh_expires_at = int(time.time()) + int(refresh_expires_in) logger.debug( "Refresh token expires in %ss (at timestamp %s)", refresh_expires_in, @@ -714,7 +716,15 @@ async def oauth_logout(request: Request) -> RedirectResponse | JSONResponse: ) response = RedirectResponse(next_url, status_code=302) - response.delete_cookie("mcp_session") + # Match the attributes from set_cookie so browsers reliably evict the + # cookie even on edge-case implementations that consider security flags + # when matching for deletion. + response.delete_cookie( + "mcp_session", + httponly=True, + secure=_should_use_secure_cookies(), + samesite="lax", + ) logger.info("User logged out, session cookie cleared") return response diff --git a/nextcloud_mcp_server/auth/oauth_routes.py b/nextcloud_mcp_server/auth/oauth_routes.py index 7dbb56ef..919b04d8 100644 --- a/nextcloud_mcp_server/auth/oauth_routes.py +++ b/nextcloud_mcp_server/auth/oauth_routes.py @@ -699,7 +699,9 @@ async def oauth_callback_nextcloud(request: Request): refresh_expires_in = token_data.get("refresh_expires_in") refresh_expires_at = None if refresh_expires_in: - refresh_expires_at = int(time.time()) + refresh_expires_in + # Some IdPs (e.g. AWS Cognito) return refresh_expires_in as a JSON + # string rather than an int; coerce to be safe. + refresh_expires_at = int(time.time()) + int(refresh_expires_in) logger.info(" refresh_expires_in: %ss", refresh_expires_in) logger.info(" refresh_expires_at: %s", refresh_expires_at) diff --git a/nextcloud_mcp_server/server/oauth_tools.py b/nextcloud_mcp_server/server/oauth_tools.py index cb143b94..48cfae62 100644 --- a/nextcloud_mcp_server/server/oauth_tools.py +++ b/nextcloud_mcp_server/server/oauth_tools.py @@ -432,21 +432,12 @@ 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. 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)}" + # The canonical Flow 2 oauth_session row is written inside + # generate_oauth_url_for_flow2 (keyed by `state`, with the PKCE + # verifier and nonce); the unified callback looks it up by `state`. + # No additional row is needed here. redirect_uri = f"{os.getenv('NEXTCLOUD_MCP_SERVER_URL', 'http://localhost:8000')}/oauth/callback" - await storage.store_oauth_session( - session_id=session_id, - client_redirect_uri="", # No client redirect for Flow 2 - state=state, - flow_type="flow2", - is_provisioning=True, - ttl_seconds=600, # 10 minute TTL - ) - # Define scopes for Nextcloud access # Note: offline_access is only included when enabled in settings. # The actual scope sent to the IdP is determined by From b875eaf0690c41c930fa5cefd9e9f7721e2c7a52 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sun, 3 May 2026 14:21:12 +0200 Subject: [PATCH 14/14] fix(auth): address PR #758 round-7 medium/minor review - Gate browser session creation on a successful refresh token. When the IdP returns no refresh token, SessionAuthBackend would silently reject every subsequent request and bounce the user back to /oauth/login in a loop. The callback now bails with a 400 + correlation ID + actionable hint about offline_access *before* writing browser_sessions or setting the cookie. Pinned by a new end-to-end unit test. - Evict orphaned browser_sessions rows in SessionAuthBackend when the associated refresh token is gone, instead of letting them accumulate until TTL cleanup. Best-effort; deletion errors stay non-fatal. - Demote identity-bearing logs in the Flow 2 OAuth callback (user_id, scopes, audience, expires_at) from INFO to DEBUG so they don't leak into multi-tenant log aggregation on every provision. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../auth/browser_oauth_routes.py | 59 ++++--- nextcloud_mcp_server/auth/oauth_routes.py | 25 +-- nextcloud_mcp_server/auth/session_backend.py | 11 ++ tests/unit/test_browser_oauth_routes.py | 151 +++++++++++++++++- tests/unit/test_oauth_logout.py | 10 +- 5 files changed, 221 insertions(+), 35 deletions(-) diff --git a/nextcloud_mcp_server/auth/browser_oauth_routes.py b/nextcloud_mcp_server/auth/browser_oauth_routes.py index c155c168..0152c5e1 100644 --- a/nextcloud_mcp_server/auth/browser_oauth_routes.py +++ b/nextcloud_mcp_server/auth/browser_oauth_routes.py @@ -568,30 +568,47 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo token_data.get("scope", "").split() if token_data.get("scope") else None ) - # Store refresh token (for background jobs ONLY) - if refresh_token: - logger.debug( - "Storing refresh token for user_id=%s state=%s... scopes=%s expires_at=%s", + # Store refresh token (for background jobs ONLY). The browser session + # itself is gated on this — without a refresh token, ``SessionAuthBackend`` + # would reject every subsequent request and silently bounce the user back + # to ``/oauth/login`` (PR #758 round-7 medium 1). + if not refresh_token: + correlation_id = secrets.token_urlsafe(8) + logger.error( + "No refresh token in token response — cannot establish browser " + "session (correlation_id=%s, user_id=%s)", + correlation_id, user_id, - state[:16], - granted_scopes, - refresh_expires_at, ) - await storage.store_refresh_token( - user_id=user_id, - refresh_token=refresh_token, - expires_at=refresh_expires_at, - flow_type="browser", # Browser-based login flow - provisioning_client_id=state, # Store state for unified session lookup - scopes=granted_scopes, + return HTMLResponse( + f"

Login Failed

" + f"

The identity provider did not return a refresh token, so a " + f"persistent session could not be established. Make sure " + f"offline_access is granted in the IdP configuration.

" + f"

Correlation ID: {html_escape(correlation_id)}

", + status_code=400, ) - logger.info( - "Refresh token stored for user %s (lookup key: %s...)", - user_id, - state[:16], - ) - else: - logger.warning("No refresh token in token response - cannot store session") + + logger.debug( + "Storing refresh token for user_id=%s state=%s... scopes=%s expires_at=%s", + user_id, + state[:16], + granted_scopes, + refresh_expires_at, + ) + await storage.store_refresh_token( + user_id=user_id, + refresh_token=refresh_token, + expires_at=refresh_expires_at, + flow_type="browser", # Browser-based login flow + provisioning_client_id=state, # Store state for unified session lookup + scopes=granted_scopes, + ) + logger.info( + "Refresh token stored for user %s (lookup key: %s...)", + user_id, + state[:16], + ) # Query and cache user profile (for browser UI display) access_token = token_data.get("access_token") diff --git a/nextcloud_mcp_server/auth/oauth_routes.py b/nextcloud_mcp_server/auth/oauth_routes.py index 919b04d8..92ac6268 100644 --- a/nextcloud_mcp_server/auth/oauth_routes.py +++ b/nextcloud_mcp_server/auth/oauth_routes.py @@ -702,16 +702,19 @@ async def oauth_callback_nextcloud(request: Request): # Some IdPs (e.g. AWS Cognito) return refresh_expires_in as a JSON # string rather than an int; coerce to be safe. refresh_expires_at = int(time.time()) + int(refresh_expires_in) - logger.info(" refresh_expires_in: %ss", refresh_expires_in) - logger.info(" refresh_expires_at: %s", refresh_expires_at) + logger.debug(" refresh_expires_in: %ss", refresh_expires_in) + logger.debug(" refresh_expires_at: %s", refresh_expires_at) - logger.info("Storing refresh token:") - logger.info(" user_id: %s", user_id) - logger.info(" flow_type: flow2") - logger.info(" token_audience: nextcloud") - logger.info(" provisioning_client_id: %s...", state[:16]) - logger.info(" scopes: %s", granted_scopes) - logger.info(" expires_at: %s", refresh_expires_at) + # Identity-bearing fields stay at DEBUG so they don't reach + # multi-tenant log aggregation on every Flow 2 provision (PR #758 + # round-7 minor). + logger.debug("Storing refresh token:") + logger.debug(" user_id: %s", user_id) + logger.debug(" flow_type: flow2") + logger.debug(" token_audience: nextcloud") + logger.debug(" provisioning_client_id: %s...", state[:16]) + logger.debug(" scopes: %s", granted_scopes) + logger.debug(" expires_at: %s", refresh_expires_at) await storage.store_refresh_token( user_id=user_id, @@ -722,8 +725,8 @@ async def oauth_callback_nextcloud(request: Request): scopes=granted_scopes, expires_at=refresh_expires_at, ) - logger.info("✓ Stored Flow 2 master refresh token for user %s", user_id) - logger.info("=" * 60) + logger.debug("✓ Stored Flow 2 master refresh token for user %s", user_id) + logger.debug("=" * 60) # Return success HTML page success_html = """ diff --git a/nextcloud_mcp_server/auth/session_backend.py b/nextcloud_mcp_server/auth/session_backend.py index 70c45146..87364f09 100644 --- a/nextcloud_mcp_server/auth/session_backend.py +++ b/nextcloud_mcp_server/auth/session_backend.py @@ -101,6 +101,17 @@ class SessionAuthBackend(AuthenticationBackend): session_id[:8], user_id, ) + # Proactively evict the orphan so the table doesn't accumulate + # rows that the auth check will keep rejecting until TTL + # cleanup (PR #758 round-7 minor). + try: + await storage.delete_browser_session(session_id) + except Exception as e: + logger.warning( + "Failed to delete orphaned browser session %s…: %s", + session_id[:8], + e, + ) return None return AuthCredentials(["authenticated"]), SimpleUser(user_id) diff --git a/tests/unit/test_browser_oauth_routes.py b/tests/unit/test_browser_oauth_routes.py index 0c86bb04..6f3ae7f5 100644 --- a/tests/unit/test_browser_oauth_routes.py +++ b/tests/unit/test_browser_oauth_routes.py @@ -6,9 +6,18 @@ tests / direct ``settings.set`` calls can leave the raw string in place, and ``bool("false")`` is ``True``. """ -import pytest +import json +import tempfile +from pathlib import Path +from unittest.mock import AsyncMock, MagicMock, patch -from nextcloud_mcp_server.auth import browser_oauth_routes +import httpx +import pytest +from cryptography.fernet import Fernet + +from nextcloud_mcp_server.auth import browser_oauth_routes, token_utils +from nextcloud_mcp_server.auth.browser_oauth_routes import oauth_login_callback +from nextcloud_mcp_server.auth.storage import RefreshTokenStorage pytestmark = pytest.mark.unit @@ -71,3 +80,141 @@ def test_should_use_secure_cookies_falls_back_to_http_scheme(monkeypatch): ), ) assert browser_oauth_routes._should_use_secure_cookies() is False + + +# --------------------------------------------------------------------------- +# oauth_login_callback: missing refresh_token must NOT create a session +# --------------------------------------------------------------------------- +# +# Pins PR #758 round-7 medium 1: when the IdP returns no refresh_token, +# ``SessionAuthBackend`` would silently reject every subsequent request +# (because ``get_refresh_token`` returns None), bouncing the user back to +# ``/oauth/login`` in a loop. The callback now bails with a 400 error page +# *before* any browser_sessions row or Set-Cookie header is created. + + +@pytest.fixture +def _clear_oidc_caches(): + 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() + + +@pytest.fixture +async def _no_refresh_storage(): + with tempfile.TemporaryDirectory() as tmpdir: + s = RefreshTokenStorage( + db_path=str(Path(tmpdir) / "norefresh.db"), + encryption_key=Fernet.generate_key().decode(), + ) + await s.initialize() + yield s + + +async def test_callback_rejects_token_response_without_refresh_token( + _clear_oidc_caches, _no_refresh_storage +): + storage = _no_refresh_storage + state = "state-norefresh" + + await storage.store_oauth_session( + session_id=state, + client_id="browser-ui", + client_redirect_uri="/app", + state=state, + code_challenge="cc", + code_challenge_method="S256", + mcp_authorization_code="cv", + flow_type="browser", + ttl_seconds=600, + ) + + discovery = { + "issuer": "http://idp.example", + "token_endpoint": "http://idp.example/token", + } + + def handler(request: httpx.Request) -> httpx.Response: + if request.url.path.endswith("/.well-known/openid-configuration"): + return httpx.Response( + 200, + content=json.dumps(discovery).encode(), + headers={"content-type": "application/json"}, + ) + if str(request.url) == "http://idp.example/token": + # Successful token exchange but no refresh_token (e.g. IdP + # config without offline_access). + return httpx.Response( + 200, + content=json.dumps( + { + "access_token": "at", + "id_token": "id-token-stub", + "token_type": "Bearer", + } + ).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) + + request = MagicMock() + request.query_params = {"code": "abc", "state": state} + request.cookies = {} + request.app.state.oauth_context = { + "storage": storage, + "oauth_client": None, + "config": { + "discovery_url": "http://idp.example/.well-known/openid-configuration", + "client_id": "test", + "client_secret": "secret", + "mcp_server_url": "http://localhost", + }, + } + request.url_for = MagicMock(return_value="/oauth/login") + + fake_userinfo = {"sub": "alice", "preferred_username": "alice"} + + with ( + patch( + "nextcloud_mcp_server.auth.browser_oauth_routes.nextcloud_httpx_client", + side_effect=fake_client, + ), + patch( + "nextcloud_mcp_server.auth.token_utils.nextcloud_httpx_client", + side_effect=fake_client, + ), + patch( + "nextcloud_mcp_server.auth.browser_oauth_routes.verify_id_token", + new=AsyncMock(return_value=fake_userinfo), + ), + patch( + "nextcloud_mcp_server.auth.browser_oauth_routes._get_userinfo_endpoint", + new=AsyncMock(return_value=None), + ), + ): + response = await oauth_login_callback(request) + + assert response.status_code == 400 + body = response.body.decode() + assert "Login Failed" in body + assert "refresh token" in body.lower() + + # No browser session row may have been created. + assert await storage.get_browser_session_user("ignored") is None + # Nothing under the verified user_id either. + assert await storage.get_refresh_token("alice") is None + + # No Set-Cookie header — the user must not walk away with an unusable + # session cookie. + set_cookie = response.headers.get("set-cookie", "") + assert "mcp_session" not in set_cookie diff --git a/tests/unit/test_oauth_logout.py b/tests/unit/test_oauth_logout.py index 9eee223d..c27960f6 100644 --- a/tests/unit/test_oauth_logout.py +++ b/tests/unit/test_oauth_logout.py @@ -586,7 +586,12 @@ async def test_session_backend_rejects_unknown_session(storage): async def test_session_backend_rejects_session_without_refresh_token(storage): - """Defense-in-depth: session row exists but user has no refresh token.""" + """Defense-in-depth: session row exists but user has no refresh token. + + PR #758 round-7 minor: rejection now also evicts the orphaned + ``browser_sessions`` row so the table doesn't accumulate dead entries + that the auth check will keep rejecting until TTL cleanup. + """ await storage.create_browser_session(session_id="sid-B", user_id="bob") # Note: NO refresh token stored for bob @@ -594,6 +599,9 @@ async def test_session_backend_rejects_session_without_refresh_token(storage): conn = _build_conn(cookie="sid-B", oauth_context={"storage": storage}) assert await backend.authenticate(conn) is None + # Orphan must be evicted on rejection. + assert await storage.get_browser_session_user("sid-B") is None + async def test_session_backend_rejects_when_no_cookie(storage): backend = SessionAuthBackend(oauth_enabled=True)