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 @@ 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