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")