diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index a0cdfb89..df70a84e 100644 --- a/.github/workflows/test.yml +++ b/.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 diff --git a/nextcloud_mcp_server/auth/oauth_routes.py b/nextcloud_mcp_server/auth/oauth_routes.py index 1b00706a..53c804ea 100644 --- a/nextcloud_mcp_server/auth/oauth_routes.py +++ b/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( diff --git a/nextcloud_mcp_server/auth/token_utils.py b/nextcloud_mcp_server/auth/token_utils.py index 77bd0b2a..b13f411d 100644 --- a/nextcloud_mcp_server/auth/token_utils.py +++ b/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 - present (e.g. BasicAuth mode where this should not be called). + user_id from the verified token, or ``"default_user"`` when no + 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 diff --git a/tests/unit/test_oauth_callback_session_cleanup.py b/tests/unit/test_oauth_callback_session_cleanup.py new file mode 100644 index 00000000..3d908896 --- /dev/null +++ b/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 diff --git a/tests/unit/test_token_utils_user_id.py b/tests/unit/test_token_utils_user_id.py new file mode 100644 index 00000000..1ddcc97b --- /dev/null +++ b/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())