diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index df70a84e..f85eca78 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -129,6 +129,18 @@ jobs: # npm ci # npm run build + # Generate an ephemeral Fernet key per CI run. docker-compose.yml + # requires TOKEN_ENCRYPTION_KEY (PR #758 finding 5 removed the + # hardcoded default), but the CI tokens.db is destroyed at the end of + # the job so there is no value in persisting the key as a repo secret. + # ``openssl rand -base64 32`` produces 32 bytes encoded as 44 base64 + # chars; ``tr '+/' '-_'`` converts to URL-safe base64, which is + # exactly what Fernet expects. + - name: Generate ephemeral TOKEN_ENCRYPTION_KEY + run: | + KEY=$(openssl rand -base64 32 | tr '+/' '-_') + echo "TOKEN_ENCRYPTION_KEY=${KEY}" >> "$GITHUB_ENV" + # Start services with the appropriate profile - name: Run docker compose uses: hoverkraft-tech/compose-action@4894d2492015c1774ee5a13a95b1072093087ec3 # v2.5.0 @@ -139,11 +151,8 @@ 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 }} + # Inherited from $GITHUB_ENV via the previous step. + TOKEN_ENCRYPTION_KEY: ${{ env.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/alembic/versions/20260502_1600_006_add_nonce_to_oauth_sessions.py b/nextcloud_mcp_server/alembic/versions/20260502_1600_006_add_nonce_to_oauth_sessions.py new file mode 100644 index 00000000..8489e38f --- /dev/null +++ b/nextcloud_mcp_server/alembic/versions/20260502_1600_006_add_nonce_to_oauth_sessions.py @@ -0,0 +1,29 @@ +"""Add nonce column to oauth_sessions for OIDC ID-token binding. + +PR #758 finding 2: the browser OAuth flow generated PKCE + state but no +``nonce``. Without a nonce, an attacker who obtains a valid ID token for +another user (e.g. from a parallel auth request) could replay it inside +this flow because the token isn't cryptographically tied to the +authorization request. The nonce is generated in ``oauth_login``, +forwarded to the IdP in the auth URL, and verified on the way back. + +Revision ID: 006 +Revises: 005 +Create Date: 2026-05-02 16:00:00.000000 +""" + +from alembic import op + +revision = "006" +down_revision = "005" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + op.execute("ALTER TABLE oauth_sessions ADD COLUMN nonce TEXT") + + +def downgrade() -> None: + # SQLite < 3.35 cannot DROP COLUMN; leave the column on downgrade. + pass diff --git a/nextcloud_mcp_server/auth/browser_oauth_routes.py b/nextcloud_mcp_server/auth/browser_oauth_routes.py index 3d2c6186..cb0b33e9 100644 --- a/nextcloud_mcp_server/auth/browser_oauth_routes.py +++ b/nextcloud_mcp_server/auth/browser_oauth_routes.py @@ -19,6 +19,7 @@ from starlette.responses import HTMLResponse, JSONResponse, RedirectResponse from nextcloud_mcp_server.auth.token_utils import ( IdTokenVerificationError, + get_oidc_discovery, verify_id_token, ) from nextcloud_mcp_server.auth.userinfo_routes import ( @@ -65,7 +66,15 @@ def _origin_matches_self(request: Request, oauth_ctx: dict) -> bool: cfg = oauth_ctx.get("config") or oauth_ctx mcp_server_url = cfg.get("mcp_server_url") if not mcp_server_url: - # Mis-configured deployment — fail open rather than break logout. + # Mis-configured deployment — fail open rather than break logout, but + # log loudly so the operator can see this is happening (PR #758 + # finding 3). Other OAuth code paths require ``mcp_server_url`` and + # KeyError if it's absent, so this branch should never fire in a + # correctly configured deployment. + logger.warning( + "CSRF check bypassed on /oauth/logout: mcp_server_url not " + "configured in oauth_context — set NEXTCLOUD_MCP_SERVER_URL" + ) return True expected = _normalise_origin(mcp_server_url) @@ -149,6 +158,12 @@ async def oauth_login(request: Request) -> RedirectResponse | JSONResponse: # Generate state for CSRF protection state = secrets.token_urlsafe(32) + # Generate OIDC nonce so the ID token returned on callback can be bound + # to THIS auth request (PR #758 finding 2). Without a nonce, an attacker + # who acquired a separate valid ID token could replay it inside this + # flow. + nonce = secrets.token_urlsafe(32) + # Build OAuth authorization URL mcp_server_url = oauth_config["mcp_server_url"] callback_uri = f"{mcp_server_url}/oauth/callback" @@ -164,7 +179,8 @@ async def oauth_login(request: Request) -> RedirectResponse | JSONResponse: digest = hashlib.sha256(code_verifier.encode()).digest() code_challenge = urlsafe_b64encode(digest).decode().rstrip("=") - # Store code_verifier in session for retrieval during callback (using state as key) + # Store code_verifier + nonce in session for retrieval during callback + # (using state as key) await storage.store_oauth_session( session_id=state, # Use state as session ID client_id="browser-ui", @@ -173,6 +189,7 @@ async def oauth_login(request: Request) -> RedirectResponse | JSONResponse: code_challenge=code_challenge, code_challenge_method="S256", mcp_authorization_code=code_verifier, # Store code_verifier here temporarily + nonce=nonce, flow_type="browser", ttl_seconds=600, # 10 minutes ) @@ -199,6 +216,7 @@ async def oauth_login(request: Request) -> RedirectResponse | JSONResponse: "response_type": "code", "scope": scopes, "state": state, + "nonce": nonce, "code_challenge": code_challenge, "code_challenge_method": "S256", "prompt": "consent", # Ensure refresh token @@ -219,12 +237,11 @@ async def oauth_login(request: Request) -> RedirectResponse | JSONResponse: status_code=500, ) - # Fetch authorization endpoint - async with nextcloud_httpx_client() as http_client: - response = await http_client.get(discovery_url) - response.raise_for_status() - discovery = response.json() - authorization_endpoint = discovery["authorization_endpoint"] + # Fetch authorization endpoint via the shared 5-minute discovery + # cache (PR #758 nit 5) so each browser login doesn't hit the IdP's + # discovery endpoint. + discovery = await get_oidc_discovery(discovery_url) + authorization_endpoint = discovery["authorization_endpoint"] # Include offline_access only if the IdP advertises it (or if # scopes_supported is absent from the discovery document). @@ -257,6 +274,7 @@ async def oauth_login(request: Request) -> RedirectResponse | JSONResponse: "response_type": "code", "scope": scopes, "state": state, + "nonce": nonce, "code_challenge": code_challenge, "code_challenge_method": "S256", "prompt": "consent", # Ensure refresh token @@ -336,13 +354,17 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo oauth_client = oauth_ctx["oauth_client"] oauth_config = oauth_ctx["config"] - # Retrieve code_verifier and redirect URL from session storage + # Retrieve code_verifier, nonce, and redirect URL from session storage code_verifier = "" + nonce: str | None = None next_url = "/app" # Default redirect oauth_session = await storage.get_oauth_session(state) if oauth_session: # code_verifier was stored in mcp_authorization_code field code_verifier = oauth_session.get("mcp_authorization_code", "") + # nonce bound to this auth request — verified against the ID token + # below (PR #758 finding 2). + nonce = oauth_session.get("nonce") # next_url was stored in client_redirect_uri field — re-validate at # read-time as defense-in-depth (issue #758 finding 3). The session # row could have been written by an older code path or reused. @@ -483,6 +505,7 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo id_token, discovery_url=verification_discovery_url, expected_audience=verification_audience, + expected_nonce=nonce, ) except IdTokenVerificationError as e: logger.error("ID token verification failed: %s", e) @@ -673,21 +696,21 @@ async def _revoke_refresh_token_at_idp(oauth_ctx: dict, refresh_token: str) -> N if not discovery_url: return + # Re-use the shared 5-minute discovery cache (PR #758 nit 6) so a + # burst of logouts doesn't hammer the IdP's discovery endpoint. + discovery = await get_oidc_discovery(discovery_url) + revocation_endpoint = discovery.get("revocation_endpoint") + if not revocation_endpoint: + logger.debug("IdP advertises no revocation_endpoint; skipping") + return + + client_id = cfg.get("client_id") or settings.oidc_client_id + client_secret = cfg.get("client_secret") or settings.oidc_client_secret + if not (client_id and client_secret): + logger.debug("No OIDC client credentials available for revocation") + return + async with nextcloud_httpx_client() as http_client: - discovery_response = await http_client.get(discovery_url) - discovery_response.raise_for_status() - discovery = discovery_response.json() - revocation_endpoint = discovery.get("revocation_endpoint") - if not revocation_endpoint: - logger.debug("IdP advertises no revocation_endpoint; skipping") - return - - client_id = cfg.get("client_id") or settings.oidc_client_id - client_secret = cfg.get("client_secret") or settings.oidc_client_secret - if not (client_id and client_secret): - logger.debug("No OIDC client credentials available for revocation") - return - response = await http_client.post( revocation_endpoint, data={ diff --git a/nextcloud_mcp_server/auth/oauth_routes.py b/nextcloud_mcp_server/auth/oauth_routes.py index 53c804ea..2f6de955 100644 --- a/nextcloud_mcp_server/auth/oauth_routes.py +++ b/nextcloud_mcp_server/auth/oauth_routes.py @@ -954,6 +954,28 @@ async def _oauth_callback_as_proxy( f"(token_type={nc_token_response.get('token_type')})" ) + # Verify the ID token signature + claims before caching the response + # (PR #758 finding 1). Without this, a compromised IdP or tampered + # transport could plant arbitrary identity claims into the proxy code + # entry that gets handed back to the MCP client. Mirrors the + # verification done in oauth_callback_nextcloud. + id_token = nc_token_response.get("id_token") + try: + await verify_id_token( + id_token, + discovery_url=discovery_url, + expected_audience=mcp_server_client_id, + ) + except IdTokenVerificationError as e: + logger.error("AS proxy: ID token verification failed: %s", e) + return JSONResponse( + { + "error": "invalid_token", + "error_description": "ID token failed verification", + }, + status_code=400, + ) + # Generate a proxy authorization code for the client proxy_code = secrets.token_urlsafe(32) _proxy_codes[proxy_code] = ProxyCodeEntry( diff --git a/nextcloud_mcp_server/auth/storage.py b/nextcloud_mcp_server/auth/storage.py index a9fc9a33..691c1cc2 100644 --- a/nextcloud_mcp_server/auth/storage.py +++ b/nextcloud_mcp_server/auth/storage.py @@ -917,6 +917,7 @@ class RefreshTokenStorage: flow_type: str = "hybrid", is_provisioning: bool = False, requested_scopes: str | None = None, + nonce: str | None = None, ttl_seconds: int = 600, # 10 minutes ) -> None: """ @@ -933,6 +934,8 @@ class RefreshTokenStorage: flow_type: Type of flow ('hybrid', 'flow1', 'flow2') is_provisioning: Whether this is a Flow 2 provisioning session requested_scopes: Requested OAuth scopes + nonce: OIDC ``nonce`` value bound to this auth request, returned + in the ID token and verified on callback (PR #758 finding 2). ttl_seconds: Session TTL in seconds """ if not self._initialized: @@ -947,8 +950,8 @@ class RefreshTokenStorage: INSERT INTO oauth_sessions (session_id, client_id, client_redirect_uri, state, code_challenge, code_challenge_method, mcp_authorization_code, flow_type, - is_provisioning, requested_scopes, created_at, expires_at) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + is_provisioning, requested_scopes, nonce, created_at, expires_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) """, ( session_id, @@ -961,6 +964,7 @@ class RefreshTokenStorage: flow_type, is_provisioning, requested_scopes, + nonce, now, expires_at, ), diff --git a/nextcloud_mcp_server/auth/token_utils.py b/nextcloud_mcp_server/auth/token_utils.py index b13f411d..7903ac34 100644 --- a/nextcloud_mcp_server/auth/token_utils.py +++ b/nextcloud_mcp_server/auth/token_utils.py @@ -52,6 +52,17 @@ async def _get_cached( return data +async def get_oidc_discovery(discovery_url: str) -> dict[str, Any]: + """Return the cached OIDC discovery document for *discovery_url*. + + Shares the 5-minute discovery cache used by `verify_id_token`, so a + callback that does discovery → token-exchange → ID-token verification + reuses one HTTP round-trip instead of three. Public alias for `_get_cached` + against `_discovery_cache` (PR #758 nits 5 & 6). + """ + return await _get_cached(_discovery_cache, discovery_url) + + async def verify_id_token( id_token: str, *, diff --git a/nextcloud_mcp_server/server/oauth_tools.py b/nextcloud_mcp_server/server/oauth_tools.py index 90769bf5..1ae62e79 100644 --- a/nextcloud_mcp_server/server/oauth_tools.py +++ b/nextcloud_mcp_server/server/oauth_tools.py @@ -9,7 +9,6 @@ import logging import os import secrets from datetime import datetime, timezone -from typing import Optional from urllib.parse import urlencode from mcp.server.fastmcp import Context @@ -18,7 +17,7 @@ from pydantic import BaseModel, Field from nextcloud_mcp_server.auth import require_scopes from nextcloud_mcp_server.auth.astrolabe_client import AstrolabeClient -from nextcloud_mcp_server.auth.storage import RefreshTokenStorage +from nextcloud_mcp_server.auth.storage import get_shared_storage from nextcloud_mcp_server.auth.token_broker import TokenBrokerService # Re-export for backward compatibility — canonical location is auth.token_utils @@ -34,17 +33,17 @@ class ProvisioningStatus(BaseModel): """Status of Nextcloud provisioning for a user.""" is_provisioned: bool = Field(description="Whether Nextcloud access is provisioned") - provisioned_at: Optional[str] = Field( + provisioned_at: str | None = Field( None, description="ISO timestamp when provisioned" ) - credential_type: Optional[str] = Field( + credential_type: str | None = Field( None, description="Type of credential ('refresh_token' or 'app_password')" ) - client_id: Optional[str] = Field( + client_id: str | None = Field( None, description="Client ID that initiated the original Flow 1" ) - scopes: Optional[list[str]] = Field(None, description="Granted scopes") - flow_type: Optional[str] = Field( + scopes: list[str] | None = Field(None, description="Granted scopes") + flow_type: str | None = Field( None, description="Type of flow used ('hybrid', 'flow1', 'flow2')" ) @@ -53,7 +52,7 @@ class ProvisioningResult(BaseModel): """Result of provisioning attempt.""" success: bool = Field(description="Whether provisioning was initiated") - provisioning_url: Optional[str] = Field( + provisioning_url: str | None = Field( None, description="URL to Astrolabe settings for provisioning background sync" ) message: str = Field(description="Status message for the user") @@ -122,8 +121,7 @@ async def get_provisioning_status(ctx: Context, user_id: str) -> ProvisioningSta logger.info( f" get_provisioning_status: Looking up refresh token for user_id={user_id}" ) - storage = RefreshTokenStorage.from_env() - await storage.initialize() + storage = await get_shared_storage() token_data = await storage.get_refresh_token(user_id) @@ -291,8 +289,7 @@ async def revoke_nextcloud_access(ctx: Context, user_id: str) -> RevocationResul ) # Initialize Token Broker to handle revocation - storage = RefreshTokenStorage.from_env() - await storage.initialize() + storage = await get_shared_storage() # Get OAuth client credentials from storage client_creds = await storage.get_oauth_client() @@ -420,8 +417,7 @@ async def check_logged_in(ctx: Context, user_id: str) -> str: state = secrets.token_urlsafe(32) # Store state in session for validation on callback - storage = RefreshTokenStorage.from_env() - await storage.initialize() + storage = await get_shared_storage() # Create OAuth session for Flow 2 session_id = f"flow2_{user_id}_{secrets.token_hex(8)}" diff --git a/tests/unit/test_oauth_callback_session_cleanup.py b/tests/unit/test_oauth_callback_session_cleanup.py index 3d908896..1f84f678 100644 --- a/tests/unit/test_oauth_callback_session_cleanup.py +++ b/tests/unit/test_oauth_callback_session_cleanup.py @@ -9,6 +9,11 @@ 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 @@ -19,8 +24,15 @@ 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.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 @@ -170,3 +182,85 @@ async def test_callback_no_session_row_does_not_crash(storage): # No crash, no row, no surprises. 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", + ) + _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_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(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 diff --git a/tests/unit/test_oauth_logout.py b/tests/unit/test_oauth_logout.py index b8629ace..095f81c1 100644 --- a/tests/unit/test_oauth_logout.py +++ b/tests/unit/test_oauth_logout.py @@ -20,6 +20,7 @@ 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, @@ -30,6 +31,20 @@ 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) # --------------------------------------------------------------------------- @@ -337,9 +352,17 @@ async def test_revoke_helper_posts_to_revocation_endpoint(): kwargs["transport"] = transport return httpx.AsyncClient(**kwargs) - with patch( - "nextcloud_mcp_server.auth.browser_oauth_routes.nextcloud_httpx_client", - side_effect=fake_client, + # 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( { @@ -373,9 +396,15 @@ async def test_revoke_helper_skips_when_no_revocation_endpoint(): kwargs["transport"] = transport return httpx.AsyncClient(**kwargs) - with patch( - "nextcloud_mcp_server.auth.browser_oauth_routes.nextcloud_httpx_client", - side_effect=fake_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, + ), ): # Returns None and does not raise result = await _revoke_refresh_token_at_idp( @@ -403,9 +432,15 @@ async def test_revoke_helper_silent_on_idp_error(): kwargs["transport"] = transport return httpx.AsyncClient(**kwargs) - with patch( - "nextcloud_mcp_server.auth.browser_oauth_routes.nextcloud_httpx_client", - side_effect=fake_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, + ), ): result = await _revoke_refresh_token_at_idp( {