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