Merge pull request #758 from cbcoutinho/security/oauth-session-hardening-626

fix(auth): harden OAuth/session for hosted multi-tenant deployment (#626)

tests/unit/test_browser_oauth_routes.py

@@ -0,0 +1,220 @@
+"""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 json
+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 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
+
+
+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
+
+
+# ---------------------------------------------------------------------------
+# 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

tests/unit/test_browser_oauth_xss.py

@@ -0,0 +1,150 @@
+"""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 import 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
+
+
+XSS_PAYLOAD = "<script>alert(1)</script>"
+
+
+@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:
+        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 "&lt;script&gt;alert(1)&lt;/script&gt;" in body
+
+
+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:
+        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",
+            },
+        },
+    )
+
+    # 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)
+
+    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 "&lt;script&gt;alert(1)&lt;/script&gt;" not in body
+    assert "An internal error occurred" in body
+    assert "Correlation ID" in body

tests/unit/test_browser_sessions.py

@@ -0,0 +1,99 @@
+"""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"
+
+
+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

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,
     ):

tests/unit/test_id_token_verification.py

@@ -0,0 +1,537 @@
+"""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 anyio
+import httpx
+import jwt
+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,
+)
+
+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()
+    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.
+_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"
+        )
+
+
+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.
+
+    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"
+
+
+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"
+    )
+    # 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)}"
+    )

tests/unit/test_oauth_callback_session_cleanup.py

@@ -0,0 +1,330 @@
+"""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.
+
+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
+from pathlib import Path
+from unittest.mock import AsyncMock, MagicMock, patch
+
+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,
+    _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
+
+
+@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_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=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_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)
+
+    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",
+        },
+    }
+
+    response = await oauth_login_callback(request)
+
+    assert response.status_code == 400
+    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",
+        nonce="nonce-rejected",
+    )
+    _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_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=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
+
+
+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"
+    )

tests/unit/test_oauth_logout.py

@@ -0,0 +1,620 @@
+"""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 import token_utils
+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
+
+
+@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)
+# ---------------------------------------------------------------------------
+
+
+@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,
+    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
+
+
+# ---------------------------------------------------------------------------
+# 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,
+            "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",
+        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,
+            "config": {
+                "mcp_server_url": "https://mcp.example.com",
+                "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,
+            "config": {
+                "mcp_server_url": "https://mcp.example.com",
+                "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,
+            "config": {
+                "mcp_server_url": "https://mcp.example.com",
+                "discovery_url": None,
+            },
+        },
+    )
+    response = await oauth_logout(request)
+    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")
+
+    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_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")
+
+    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_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")
+
+    revoke = AsyncMock()
+    request = _build_request(
+        cookie="sid-4",
+        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",
+        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)
+
+    # 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(
+            {
+                "config": {
+                    "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,
+        ),
+        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(
+            {
+                "config": {
+                    "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,
+        ),
+        patch(
+            "nextcloud_mcp_server.auth.token_utils.nextcloud_httpx_client",
+            side_effect=fake_client,
+        ),
+    ):
+        result = await _revoke_refresh_token_at_idp(
+            {
+                "config": {
+                    "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.
+
+    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
+
+    backend = SessionAuthBackend(oauth_enabled=True)
+    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)
+    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"

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

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

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

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

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

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