fix(auth): address PR #758 round-6 medium/low review

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) <noreply@anthropic.com>

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

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(

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

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

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