fix(auth): fail closed on missing sub claim, delete Flow 2 callback session

Addresses the two remaining 🟡 findings from the PR #758 follow-up review:

  1. extract_user_id_from_token previously fell back to "default_user" when
     the verified access token had no sub claim. In a multi-tenant deployment
     a malformed IdP token could have bucketed every request under a single
     sentinel user, risking cross-tenant data exposure. The function now
     raises McpError on that branch; the BasicAuth no-token sentinel path is
     preserved.

  2. oauth_callback_nextcloud (Flow 2) read the PKCE code_verifier from
     oauth_sessions but never deleted the row, leaving the verifier valid for
     the full 10-minute TTL. The row is now deleted eagerly inside the same
     branch, mirroring oauth_login_callback in browser_oauth_routes.

Also wires TOKEN_ENCRYPTION_KEY through the docker-compose step in the CI
test workflow so the integration matrix can boot — every job had been
failing fast on the ${TOKEN_ENCRYPTION_KEY:?...} interpolation guard added
in PR #758 finding 5.

Tests pin both fixes (test_token_utils_user_id.py,
test_oauth_callback_session_cleanup.py).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

.github/workflows/test.yml

@@ -139,6 +139,11 @@ jobs:
         env:
           MCP_SERVER_URL: ${{ matrix.mcp-internal-url }}
           NEXTCLOUD_IMAGE: ${{ matrix.nextcloud_image }}
+          # Required by docker-compose.yml since PR #758 finding 5 (no more
+          # hardcoded Fernet keys). Generated once and stored as a repo
+          # secret; the CI tokens.db is ephemeral so a single shared key
+          # across services is acceptable.
+          TOKEN_ENCRYPTION_KEY: ${{ secrets.TOKEN_ENCRYPTION_KEY }}
 
       - name: Install the latest version of uv
         uses: astral-sh/setup-uv@37802adc94f370d6bfd71619e3f0bf239e1f3b78 # v7.6.0

nextcloud_mcp_server/auth/oauth_routes.py

@@ -597,6 +597,11 @@ async def oauth_callback_nextcloud(request: Request):
         logger.info(
             f"Retrieved code_verifier for Flow 2 callback (state={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

@@ -13,6 +13,8 @@ from jwt import PyJWKSet
 from mcp.server.auth.middleware.auth_context import get_access_token
 from mcp.server.auth.provider import AccessToken
 from mcp.server.fastmcp import Context
+from mcp.shared.exceptions import McpError
+from mcp.types import ErrorData
 
 from ..http import nextcloud_httpx_client
 
@@ -188,8 +190,17 @@ async def extract_user_id_from_token(_ctx: Context) -> str:
             verifier-populated AccessToken via get_access_token().
 
     Returns:
-        user_id from the verified token, or "default_user" when no token is
+        user_id from the verified token, or ``"default_user"`` when no
-        present (e.g. BasicAuth mode where this should not be called).
+        access token is present at all (BasicAuth mode — there is no
+        OAuth identity to extract, so the sentinel is returned and the
+        caller's BasicAuth branch handles it).
+
+    Raises:
+        McpError: An access token was present but had no ``sub`` claim
+            (``access_token.resource`` empty). Failing closed prevents a
+            malformed IdP token from silently bucketing every request
+            under the ``"default_user"`` key in SQLite, which would risk
+            cross-tenant data exposure (PR #758 follow-up review).
     """
     access_token: AccessToken | None = get_access_token()
 
@@ -202,6 +213,11 @@ async def extract_user_id_from_token(_ctx: Context) -> str:
         logger.error(
             "Access token has no resource (sub) claim — verifier should have rejected it"
         )
-        return "default_user"
+        raise McpError(
+            ErrorData(
+                code=-1,
+                message="Cannot determine user identity from access token",
+            )
+        )
 
     return user_id

tests/unit/test_oauth_callback_session_cleanup.py

@@ -0,0 +1,172 @@
+"""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.
+"""
+
+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.oauth_routes import oauth_callback_nextcloud
+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_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_cached_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_no_session_row_does_not_crash(storage):
+    """If the row is already gone (e.g. expired), the callback proceeds."""
+    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",
+    }
+    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)
+
+    with (
+        patch(
+            "nextcloud_mcp_server.auth.oauth_routes._get_cached_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 await storage.get_oauth_session(state) is None

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