diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index a0cdfb89..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,6 +151,8 @@ jobs: env: MCP_SERVER_URL: ${{ matrix.mcp-internal-url }} NEXTCLOUD_IMAGE: ${{ matrix.nextcloud_image }} + # 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/docker-compose.yml b/docker-compose.yml index 8d993c6d..3a1c6c0e 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -153,10 +153,10 @@ services: - ENABLE_MULTI_USER_BASIC_AUTH=true - ENABLE_BACKGROUND_OPERATIONS=true - # Token storage (required for middleware initialization) - # DEVELOPMENT ONLY - generate a fresh key for production: - # python -c "from cryptography.fernet import Fernet; print(Fernet.generate_key().decode())" - - TOKEN_ENCRYPTION_KEY=fqqI4G51yBCOcu9cvv6wCUJB7sf_CK2za5ClC6b86yY= + # Token storage (required for middleware initialization). + # Source the key from .env — see env.sample. To generate a fresh key: + # python -c "from cryptography.fernet import Fernet; print(Fernet.generate_key().decode())" + - TOKEN_ENCRYPTION_KEY=${TOKEN_ENCRYPTION_KEY:?TOKEN_ENCRYPTION_KEY must be set in .env (see env.sample)} - TOKEN_STORAGE_DB=/app/data/tokens.db - ENABLE_SEMANTIC_SEARCH=true @@ -230,9 +230,9 @@ services: - NEXTCLOUD_RESOURCE_URI=nextcloud # ADR-005: Keycloak uses client IDs as audiences, not URLs - NEXTCLOUD_PUBLIC_ISSUER_URL=http://localhost:8888/realms/nextcloud-mcp - # Refresh token storage (ADR-002 Tier 1 & 2) + # Refresh token storage (ADR-002 Tier 1 & 2). Source from .env. - ENABLE_BACKGROUND_OPERATIONS=true - - TOKEN_ENCRYPTION_KEY=ESF1BvEQdGYsCluwMx9Cxvw3uh5pFowPH7Rg_nIliyo= + - TOKEN_ENCRYPTION_KEY=${TOKEN_ENCRYPTION_KEY:?TOKEN_ENCRYPTION_KEY must be set in .env (see env.sample)} - TOKEN_STORAGE_DB=/app/data/tokens.db # ADR-005: Token exchange mode (RFC 8693) @@ -278,10 +278,10 @@ services: # Login Flow v2 (ADR-022) - ENABLE_LOGIN_FLOW=true - # Token storage (required for app password + session persistence) - # DEVELOPMENT ONLY - generate a fresh key for production: - # python -c "from cryptography.fernet import Fernet; print(Fernet.generate_key().decode())" - - TOKEN_ENCRYPTION_KEY=rxJvkBf7ZBjZZDL4a1sSqjhmjawhmbRMSOGfK8HDyKU= + # Token storage (required for app password + session persistence). + # Source the key from .env — see env.sample. To generate a fresh key: + # python -c "from cryptography.fernet import Fernet; print(Fernet.generate_key().decode())" + - TOKEN_ENCRYPTION_KEY=${TOKEN_ENCRYPTION_KEY:?TOKEN_ENCRYPTION_KEY must be set in .env (see env.sample)} - TOKEN_STORAGE_DB=/app/data/tokens.db # Semantic search diff --git a/docs/installation.md b/docs/installation.md index 0f99af62..d855fc5a 100644 --- a/docs/installation.md +++ b/docs/installation.md @@ -5,6 +5,7 @@ This guide covers installing the Nextcloud MCP server on your system. ## Prerequisites - **Python 3.11+** - Check with `python3 --version` +- **SQLite 3.35+** - Check with `python3 -c "import sqlite3; print(sqlite3.sqlite_version)"`. The OAuth session storage uses `DELETE ... RETURNING`, which is only available from SQLite 3.35 (March 2021). Ubuntu 20.04 ships SQLite 3.31 and is **not** supported; upgrade the host or run from the Docker image, which bundles a newer libsqlite3. - **Access to a Nextcloud instance** - Self-hosted or cloud-hosted - **Administrator access** *(optional)* - Only needed to customise app-password policies in Nextcloud settings; not required for any deployment mode (single-user, multi-user BasicAuth, or Login Flow v2) diff --git a/env.sample b/env.sample index 652d9435..bae9d96e 100644 --- a/env.sample +++ b/env.sample @@ -15,6 +15,15 @@ # Your Nextcloud instance URL (without trailing slash) NEXTCLOUD_HOST= +# Fernet key for encrypting refresh tokens / app passwords / browser +# sessions in SQLite. Required by every docker-compose profile that runs +# the MCP server (single-user, multi-user-basic, keycloak, login-flow). +# Generate one with: +# python -c "from cryptography.fernet import Fernet; print(Fernet.generate_key().decode())" +# NEVER commit a real key. Each environment (dev / staging / prod) needs +# its own key. +TOKEN_ENCRYPTION_KEY= + # ============================================ # SINGLE-USER BASICAUTH MODE # ============================================ diff --git a/nextcloud_mcp_server/alembic/versions/20260502_1500_005_add_browser_sessions.py b/nextcloud_mcp_server/alembic/versions/20260502_1500_005_add_browser_sessions.py new file mode 100644 index 00000000..b85f20d1 --- /dev/null +++ b/nextcloud_mcp_server/alembic/versions/20260502_1500_005_add_browser_sessions.py @@ -0,0 +1,49 @@ +"""Add browser_sessions table for random-id browser cookie auth. + +Replaces the prior `mcp_session=` cookie pattern (issue #626 +finding 2) with a server-side mapping from a cryptographically random +session id to the authenticated user_id. The cookie value is now opaque +and revocable. + +Revision ID: 005 +Revises: 004 +Create Date: 2026-05-02 15:00:00.000000 +""" + +from alembic import op + +revision = "005" +down_revision = "004" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + op.execute( + """ + CREATE TABLE IF NOT EXISTS browser_sessions ( + session_id TEXT PRIMARY KEY, + user_id TEXT NOT NULL, + created_at INTEGER NOT NULL, + expires_at INTEGER NOT NULL + ) + """ + ) + op.execute( + """ + CREATE INDEX IF NOT EXISTS idx_browser_sessions_user + ON browser_sessions(user_id) + """ + ) + op.execute( + """ + CREATE INDEX IF NOT EXISTS idx_browser_sessions_expires + ON browser_sessions(expires_at) + """ + ) + + +def downgrade() -> None: + op.execute("DROP INDEX IF EXISTS idx_browser_sessions_expires") + op.execute("DROP INDEX IF EXISTS idx_browser_sessions_user") + op.execute("DROP TABLE IF EXISTS browser_sessions") 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/app.py b/nextcloud_mcp_server/app.py index b79e87dd..3fb9194f 100644 --- a/nextcloud_mcp_server/app.py +++ b/nextcloud_mcp_server/app.py @@ -1354,13 +1354,16 @@ def get_app(transport: str = "streamable-http", enabled_apps: list[str] | None = storage = await get_shared_storage() count = await storage.delete_expired_login_flow_sessions() if count: - logger.info(f"Cleaned up {count} expired login flow sessions") + logger.info("Cleaned up %s expired login flow sessions", count) + # Browser session rows are otherwise only cleaned up lazily + # when a user revisits — PR #758 finding 6. + await storage.cleanup_expired_browser_sessions() # Also clean up expired AS proxy codes/sessions _cleanup_expired_proxy_codes() # Clean up expired web provision sessions _cleanup_expired_provision_sessions() except Exception as e: - logger.warning(f"Login flow cleanup error: {e}") + logger.warning("Login flow cleanup error: %s", e) await anyio.sleep(3600) # Every hour @asynccontextmanager @@ -2242,8 +2245,10 @@ def get_app(transport: str = "streamable-http", enabled_apps: list[str] | None = name="oauth_login_callback", ) ) + # POST-only: defends against passive CSRF (e.g. ) + # — see PR #758 finding 5. routes.append( - Route("/oauth/logout", oauth_logout, methods=["GET"], name="oauth_logout") + Route("/oauth/logout", oauth_logout, methods=["POST"], name="oauth_logout") ) logger.info( "Browser OAuth routes enabled: /oauth/login, /oauth/login-callback (legacy), /oauth/logout" diff --git a/nextcloud_mcp_server/auth/browser_oauth_routes.py b/nextcloud_mcp_server/auth/browser_oauth_routes.py index 8b33b0fb..0152c5e1 100644 --- a/nextcloud_mcp_server/auth/browser_oauth_routes.py +++ b/nextcloud_mcp_server/auth/browser_oauth_routes.py @@ -9,14 +9,19 @@ import logging import secrets import time from base64 import urlsafe_b64encode +from html import escape as html_escape from urllib.parse import urlencode from urllib.parse import urlparse as parse_url import httpx -import jwt from starlette.requests import Request 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 ( _get_userinfo_endpoint, _query_idp_userinfo, @@ -28,23 +33,99 @@ from ..http import nextcloud_httpx_client logger = logging.getLogger(__name__) +def _normalise_origin(raw: str) -> tuple[str, str, int | None]: + """Return (scheme, hostname, port) with default HTTP/HTTPS ports stripped. + + Browsers omit default ports in Origin headers (RFC 6454 §6.2), so a + raw netloc string comparison falsely rejects requests whenever + ``mcp_server_url`` is configured with an explicit ``:443`` / ``:80`` + (or vice versa). + """ + parsed = parse_url(raw) + scheme = parsed.scheme.lower() + hostname = (parsed.hostname or "").lower() + port = parsed.port + if (scheme == "https" and port == 443) or (scheme == "http" and port == 80): + port = None + return (scheme, hostname, port) + + +def _origin_matches_self(request: Request, oauth_ctx: dict) -> bool: + """Return True when Origin/Referer is missing or matches our own host. + + Used to gate POST /oauth/logout against cross-origin form submissions + (PR #758 round-3 review hardening). Per OWASP CSRF cheat sheet, the + policy is: + - If neither Origin nor Referer is set, allow (same-origin POST in + privacy-conscious browsers may strip both). + - Otherwise, the (scheme, hostname, port) tuple of the first present + header must equal the same tuple of the configured + ``mcp_server_url``. Default ports (80/443) are normalised away + before comparison so RFC-6454-compliant browsers — which omit + default ports in Origin — aren't rejected. + """ + cfg = oauth_ctx.get("config") or oauth_ctx + mcp_server_url = cfg.get("mcp_server_url") + if not mcp_server_url: + # Fail closed (PR #758 round-3 finding 2): a future code path that + # leaves ``mcp_server_url`` unset would otherwise silently disable + # CSRF protection on /oauth/logout. Blocking the logout is + # recoverable — the user just re-logs-in once the misconfiguration + # is fixed — and the error log makes the cause monitorable. + logger.error( + "CSRF check failed on /oauth/logout: mcp_server_url not " + "configured in oauth_context — set NEXTCLOUD_MCP_SERVER_URL" + ) + return False + + expected = _normalise_origin(mcp_server_url) + raw = request.headers.get("origin") or request.headers.get("referer") + if not raw: + return True + return _normalise_origin(raw) == expected + + +def _safe_next_url(raw: str | None, default: str) -> str: + """Return a path-only redirect target, falling back to *default*. + + Blocks open-redirect abuse via the ``?next=`` query parameter on + ``/oauth/login`` and ``/oauth/logout`` (and the round-tripped + ``client_redirect_uri`` stored on the oauth_session). A safe target: + - starts with a single ``/`` (so it's a path on this server) + - does NOT start with ``//`` (which would be protocol-relative) + - has no whitespace or control characters that could trick browsers + + Anything else returns *default*. + """ + if not raw or not raw.startswith("/") or raw.startswith("//"): + return default + if any(c.isspace() or ord(c) < 0x20 for c in raw): + return default + return raw + + def _should_use_secure_cookies() -> bool: """Determine if cookies should have the Secure flag. Reads ``settings.cookie_secure`` first (set via the ``COOKIE_SECURE`` - env var). Falls back to auto-detect from the ``nextcloud_host`` scheme - when unset. - - Returns: - True if cookies should be secure (HTTPS), False otherwise + env var). Falls back to auto-detecting from the MCP server's own URL + scheme — the cookie is issued by THIS server, so the Secure flag must + reflect THIS server's transport, not Nextcloud's. (Split-scheme + deployments — HTTPS Nextcloud + plain-HTTP MCP sidecar, or vice + versa — would otherwise get the wrong answer.) """ settings = get_settings() - if settings.cookie_secure is not None: - # Dynaconf auto-coerces "true"/"false" → bool but "1"/"0" → int; - # bool() normalises both. - return bool(settings.cookie_secure) - nextcloud_host = settings.nextcloud_host or "" - return nextcloud_host.startswith("https://") + raw = settings.cookie_secure + if raw is not None: + # Dynaconf normally coerces "true"/"false"/"1"/"0", but tests or + # direct ``settings.set`` calls can bypass that — bool("false") is + # True. Normalise explicitly so an unexpected string never flips + # cookies to Secure on plain HTTP (round-6 review). + if isinstance(raw, bool): + return raw + return str(raw).strip().lower() not in ("0", "false", "no", "off", "") + mcp_server_url = settings.nextcloud_mcp_server_url or "" + return mcp_server_url.startswith("https://") async def oauth_login(request: Request) -> RedirectResponse | JSONResponse: @@ -68,18 +149,27 @@ async def oauth_login(request: Request) -> RedirectResponse | JSONResponse: oauth_client = oauth_ctx["oauth_client"] oauth_config = oauth_ctx["config"] - # Debug: Log oauth_config contents - logger.info(f"oauth_login called - oauth_config keys: {oauth_config.keys()}") - logger.info(f"oauth_login called - client_id: {oauth_config.get('client_id')}") - logger.info(f"oauth_login called - oauth_client: {oauth_client is not None}") + # Demoted to DEBUG (PR #758 nit a) — these previously leaked the + # full set of config keys + the client_id at INFO on every login. + logger.debug("oauth_login called - oauth_config keys: %s", oauth_config.keys()) + logger.debug("oauth_login called - client_id: %s", oauth_config.get("client_id")) + logger.debug("oauth_login called - oauth_client: %s", oauth_client is not None) - # Get redirect URL from query params (default to /app) - next_url = request.query_params.get("next", "/app") - logger.info(f"oauth_login - next_url: {next_url}") + # Get redirect URL from query params (default to /app). Validated at + # write-time so we never store an attacker-controlled absolute URL on + # the oauth_session row (issue #758 finding 3). + next_url = _safe_next_url(request.query_params.get("next"), "/app") + logger.debug("oauth_login - next_url: %s", next_url) # 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" @@ -95,7 +185,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", @@ -103,7 +194,11 @@ async def oauth_login(request: Request) -> RedirectResponse | JSONResponse: state=state, code_challenge=code_challenge, code_challenge_method="S256", - mcp_authorization_code=code_verifier, # Store code_verifier here temporarily + # `mcp_authorization_code` field reused to store the PKCE + # code_verifier (one-time-use). Renaming the column requires a + # schema migration. + mcp_authorization_code=code_verifier, + nonce=nonce, flow_type="browser", ttl_seconds=600, # 10 minutes ) @@ -130,6 +225,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 @@ -137,7 +233,7 @@ async def oauth_login(request: Request) -> RedirectResponse | JSONResponse: } auth_url = f"{oauth_client.authorization_endpoint}?{urlencode(idp_params)}" - logger.info(f"Redirecting to external IdP login: {auth_url.split('?')[0]}") + logger.debug("Redirecting to external IdP login: %s", auth_url.split("?")[0]) else: # Integrated mode (Nextcloud OIDC) discovery_url = oauth_config.get("discovery_url") @@ -150,12 +246,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). @@ -188,17 +283,17 @@ 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 "resource": nextcloud_resource_uri, # Request tokens for Nextcloud API access } - # Debug: Log full parameters - logger.info(f"Building Nextcloud OIDC auth URL with params: {idp_params}") + logger.debug("Building Nextcloud OIDC auth URL with params: %s", idp_params) auth_url = f"{authorization_endpoint}?{urlencode(idp_params)}" - logger.info(f"Redirecting to Nextcloud OIDC login: {auth_url}") + logger.debug("Redirecting to Nextcloud OIDC login: %s", auth_url) return RedirectResponse(auth_url, status_code=302) @@ -223,8 +318,10 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo error_description = request.query_params.get( "error_description", "Authorization failed" ) - logger.error(f"OAuth login error: {error} - {error_description}") + logger.error("OAuth login error: %s - %s", error, error_description) login_url = str(request.url_for("oauth_login")) + # html_escape: error / error_description come from attacker-controlled + # query parameters and would otherwise reflect into the failure page. return HTMLResponse( f""" @@ -232,9 +329,9 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo Login Failed

Login Failed

-

Error: {error}

-

{error_description}

-

Try again

+

Error: {html_escape(error)}

+

{html_escape(error_description)}

+

Try again

""", @@ -266,17 +363,30 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo oauth_client = oauth_ctx["oauth_client"] oauth_config = oauth_ctx["config"] - # Retrieve code_verifier and redirect URL from session storage - code_verifier = "" - next_url = "/app" # Default redirect + # Retrieve code_verifier, nonce, and redirect URL from session storage. + # Fail closed when the row is missing/expired: otherwise PKCE + + # nonce verification silently degrade to no-ops (round-6 review). oauth_session = await storage.get_oauth_session(state) - if oauth_session: - # code_verifier was stored in mcp_authorization_code field - code_verifier = oauth_session.get("mcp_authorization_code", "") - # next_url was stored in client_redirect_uri field - next_url = oauth_session.get("client_redirect_uri", "/app") - # Clean up the temporary session - # Note: We don't have delete_oauth_session method, but it will expire after TTL + if not oauth_session: + logger.warning("OAuth callback received unknown/expired state=%s", state[:16]) + return HTMLResponse( + "Unknown or expired session — please try logging in again.", + status_code=400, + ) + # `mcp_authorization_code` field reused to store the PKCE code_verifier + # (one-time-use). Renaming the column requires a schema migration. + code_verifier = oauth_session.get("mcp_authorization_code", "") + # nonce bound to this auth request — verified against the ID token + # below (PR #758 finding 2). + nonce = oauth_session.get("nonce") + # next_url was stored in client_redirect_uri field — re-validate at + # read-time as defense-in-depth (issue #758 finding 3). The session + # row could have been written by an older code path or reused. + next_url = _safe_next_url(oauth_session.get("client_redirect_uri"), "/app") + # One-time-use session: delete eagerly so a replayed callback can't + # be processed and so the oauth_sessions table doesn't accumulate + # completed-but-not-yet-expired browser-login rows. + await storage.delete_oauth_session(state) # Exchange authorization code for tokens mcp_server_url = oauth_config["mcp_server_url"] @@ -311,11 +421,11 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo else: # Integrated mode (Nextcloud OIDC) discovery_url = oauth_config.get("discovery_url") - async with nextcloud_httpx_client() as http_client: - response = await http_client.get(discovery_url) - response.raise_for_status() - discovery = response.json() - token_endpoint = discovery["token_endpoint"] + # Use the shared 5-minute discovery cache; oauth_login() above + # has already populated it for this discovery_url so the + # callback should hit the cache rather than re-fetching. + discovery = await get_oidc_discovery(discovery_url) + token_endpoint = discovery["token_endpoint"] token_params = { "grant_type": "authorization_code", @@ -338,11 +448,18 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo token_data = response.json() except httpx.HTTPStatusError as e: + # Correlation IDs let the user reference a specific failure in the + # server logs without us having to reflect raw exception/IdP text + # back into the HTML page (PR #758 round-3 nit 6). + correlation_id = secrets.token_hex(8) error_body = ( e.response.text if hasattr(e.response, "text") else str(e.response.content) ) logger.error( - f"Token exchange failed: HTTP {e.response.status_code} - {error_body}" + "Token exchange failed (correlation_id=%s): HTTP %s - %s", + correlation_id, + e.response.status_code, + error_body, ) return HTMLResponse( f""" @@ -351,15 +468,17 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo Login Failed

Login Failed

-

Failed to exchange authorization code for tokens

-

HTTP {e.response.status_code}: {error_body}

+

An internal error occurred while exchanging the authorization code.

+

Correlation ID: {html_escape(correlation_id)}

+

Please try again, or contact your administrator if the problem persists.

""", status_code=500, ) except Exception as e: - logger.error(f"Token exchange failed: {e}") + correlation_id = secrets.token_hex(8) + logger.error("Token exchange failed (correlation_id=%s): %s", correlation_id, e) return HTMLResponse( f""" @@ -367,8 +486,9 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo Login Failed

Login Failed

-

Failed to exchange authorization code for tokens

-

Error: {e}

+

An internal error occurred while exchanging the authorization code.

+

Correlation ID: {html_escape(correlation_id)}

+

Please try again, or contact your administrator if the problem persists.

""", @@ -378,28 +498,69 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo refresh_token = token_data.get("refresh_token") id_token = token_data.get("id_token") - logger.info(f"Token exchange response keys: {token_data.keys()}") - logger.info(f"Refresh token present: {refresh_token is not None}") - logger.info(f"ID token present: {id_token is not None}") + # Demoted to DEBUG (PR #758 nit a) — these were previously logged at + # INFO on every login. + logger.debug("Token exchange response keys: %s", token_data.keys()) + logger.debug("Refresh token present: %s", refresh_token is not None) + logger.debug("ID token present: %s", id_token is not None) + + # Resolve the discovery URL + audience used for THIS auth request so + # we can verify the ID token signature + claims (issue #626 finding 1). + if oauth_client: + # External IdP path + verification_audience = oauth_client.client_id + verification_discovery_url = getattr(oauth_client, "discovery_url", None) + else: + # Integrated Nextcloud OIDC path + verification_audience = oauth_config["client_id"] + verification_discovery_url = oauth_config.get("discovery_url") + + if not verification_discovery_url: + logger.error("Cannot verify ID token: no discovery_url available") + return HTMLResponse( + "

Login Failed

OIDC discovery URL not configured

", + status_code=500, + ) - # Decode ID token to get user info try: - userinfo = jwt.decode(id_token, options={"verify_signature": False}) - user_id = userinfo.get("sub") - username = userinfo.get("preferred_username") or userinfo.get("email") - logger.info(f"Browser login successful: {username} (sub={user_id})") - except Exception as e: - logger.warning(f"Failed to decode ID token: {e}") - user_id = f"user-{secrets.token_hex(8)}" - username = "unknown" + userinfo = await verify_id_token( + id_token, + discovery_url=verification_discovery_url, + expected_audience=verification_audience, + expected_nonce=nonce, + ) + except IdTokenVerificationError as e: + # Same correlation-ID pattern as token-exchange failures + # (PR #758 round-3 nit 6) — log the detail server-side and only + # show a generic message + correlation ID in the browser. + correlation_id = secrets.token_hex(8) + logger.error( + "ID token verification failed (correlation_id=%s): %s", + correlation_id, + e, + ) + return HTMLResponse( + f"

Login Failed

" + f"

The ID token failed verification.

" + f"

Correlation ID: {html_escape(correlation_id)}

", + status_code=400, + ) + + user_id = userinfo["sub"] + username = userinfo.get("preferred_username") or userinfo.get("email") + logger.info("Browser login successful: %s (sub=%s)", username, user_id) # Calculate refresh token expiration from token response refresh_expires_in = token_data.get("refresh_expires_in") refresh_expires_at = None if refresh_expires_in: - refresh_expires_at = int(time.time()) + refresh_expires_in - logger.info( - f"Refresh token expires in {refresh_expires_in}s (at timestamp {refresh_expires_at})" + # Some IdPs (e.g. AWS Cognito) return refresh_expires_in as a JSON + # string rather than an int; coerce to be safe. + refresh_expires_at = int(time.time()) + int(refresh_expires_in) + logger.debug( + "Refresh token expires in %ss (at timestamp %s)", + refresh_expires_in, + refresh_expires_at, ) # Extract granted scopes @@ -407,26 +568,47 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo token_data.get("scope", "").split() if token_data.get("scope") else None ) - # Store refresh token (for background jobs ONLY) - if refresh_token: - logger.info(f"Storing refresh token for user_id: {user_id}") - logger.info(f" State parameter (provisioning_client_id): {state[:16]}...") - logger.info(f" Granted scopes: {granted_scopes}") - logger.info(f" Expires at: {refresh_expires_at}") - await storage.store_refresh_token( - user_id=user_id, - refresh_token=refresh_token, - expires_at=refresh_expires_at, - flow_type="browser", # Browser-based login flow - provisioning_client_id=state, # Store state for unified session lookup - scopes=granted_scopes, + # Store refresh token (for background jobs ONLY). The browser session + # itself is gated on this — without a refresh token, ``SessionAuthBackend`` + # would reject every subsequent request and silently bounce the user back + # to ``/oauth/login`` (PR #758 round-7 medium 1). + if not refresh_token: + correlation_id = secrets.token_urlsafe(8) + logger.error( + "No refresh token in token response — cannot establish browser " + "session (correlation_id=%s, user_id=%s)", + correlation_id, + user_id, ) - logger.info(f"✓ Refresh token stored successfully for user_id: {user_id}") - logger.info( - f" Token can now be found via provisioning_client_id={state[:16]}..." + return HTMLResponse( + f"

Login Failed

" + f"

The identity provider did not return a refresh token, so a " + f"persistent session could not be established. Make sure " + f"offline_access is granted in the IdP configuration.

" + f"

Correlation ID: {html_escape(correlation_id)}

", + status_code=400, ) - else: - logger.warning("No refresh token in token response - cannot store session") + + logger.debug( + "Storing refresh token for user_id=%s state=%s... scopes=%s expires_at=%s", + user_id, + state[:16], + granted_scopes, + refresh_expires_at, + ) + await storage.store_refresh_token( + user_id=user_id, + refresh_token=refresh_token, + expires_at=refresh_expires_at, + flow_type="browser", # Browser-based login flow + provisioning_client_id=state, # Store state for unified session lookup + scopes=granted_scopes, + ) + logger.info( + "Refresh token stored for user %s (lookup key: %s...)", + user_id, + state[:16], + ) # Query and cache user profile (for browser UI display) access_token = token_data.get("access_token") @@ -445,49 +627,177 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo if profile_data: # Cache profile for browser UI (no token needed to display) await storage.store_user_profile(user_id, profile_data) - logger.info(f"✓ User profile cached for {user_id}") + logger.debug("User profile cached for %s", user_id) else: - logger.warning(f"Failed to query userinfo endpoint for {user_id}") + logger.warning("Failed to query userinfo endpoint for %s", user_id) else: logger.warning("Could not determine userinfo endpoint") except Exception as e: - logger.error(f"Error caching user profile: {e}") + logger.error("Error caching user profile: %s", e) # Continue anyway - profile cache is optional for browser UI - # Create response and set session cookie - # Redirect to stored next_url (from OAuth session) or /app as default + # Create a server-side browser session: a random opaque session_id is + # mapped to the verified user_id in `browser_sessions`. The cookie value + # is the session_id (never the raw user_id — see issue #626 finding 2). + session_id = secrets.token_urlsafe(32) + session_ttl = 86400 * 30 # 30 days + await storage.create_browser_session( + session_id=session_id, user_id=user_id, ttl_seconds=session_ttl + ) + response = RedirectResponse(next_url, status_code=302) + # CSRF protection is layered: ``SameSite=Lax`` blocks cross-site POSTs + # in modern browsers; ``oauth_logout`` is POST-only with an Origin / + # Referer check (``_origin_matches_self``) to cover older browsers and + # non-browser clients. ``HttpOnly`` blocks JS exfiltration on XSS; + # ``Secure`` is gated to non-HTTP hosts in dev (PR #758 round-4 review + # nit 6). response.set_cookie( key="mcp_session", - value=user_id, - max_age=86400 * 30, # 30 days + value=session_id, + max_age=session_ttl, httponly=True, secure=_should_use_secure_cookies(), samesite="lax", ) - logger.info(f"Session cookie set for user: {username}") + logger.info("Session cookie set for user %s (sid=%s…)", username, session_id[:8]) return response -async def oauth_logout(request: Request) -> RedirectResponse: - """Browser OAuth logout - clears session cookie. +async def oauth_logout(request: Request) -> RedirectResponse | JSONResponse: + """Browser OAuth logout — invalidate session and revoke refresh token. + + Issue #626 finding 4: prior implementation only cleared the cookie, + leaving the refresh token in storage (valid up to 90 days). This now: + 1. Resolves the user_id for the current browser session_id. + 2. Calls the IdP `revocation_endpoint` for the stored refresh token + when the IdP advertises one. + 3. Deletes the stored refresh token regardless of revocation success. + 4. Deletes the browser_sessions row so the cookie is unusable even + if it leaks. + 5. Clears the cookie on the response. + + Method is POST-only at the route layer to defeat passive CSRF (PR #758 + round-3 review hardening). Origin / Referer headers are also validated + against the configured ``mcp_server_url`` when present, blocking + same-method-but-cross-origin form submissions. Query parameters: next: Optional URL to redirect to after logout (default: /oauth/login) - - Returns: - 302 redirect with cleared session cookie """ - next_url = request.query_params.get("next", "/oauth/login") + next_url = _safe_next_url(request.query_params.get("next"), "/oauth/login") + session_id = request.cookies.get("mcp_session") - # TODO: Optionally revoke refresh token from storage - # session_id = request.cookies.get("mcp_session") - # if session_id: - # await storage.delete_refresh_token(session_id) + oauth_ctx = getattr(request.app.state, "oauth_context", None) + + # CSRF check: when Origin or Referer is present, host must match the + # MCP server's own host. Per OWASP CSRF cheat sheet, we allow the + # request through when neither header is present (some user agents + # strip both for privacy on same-origin POST). + if oauth_ctx and not _origin_matches_self(request, oauth_ctx): + logger.warning( + "Logout blocked: cross-origin request from %s", + request.headers.get("origin") or request.headers.get("referer"), + ) + return JSONResponse({"error": "forbidden"}, status_code=403) + + storage = oauth_ctx.get("storage") if oauth_ctx else None + + if session_id and storage and oauth_ctx: + try: + user_id = await storage.get_browser_session_user(session_id) + if user_id: + token_data = await storage.get_refresh_token(user_id) + refresh_token = token_data.get("refresh_token") if token_data else None + + if refresh_token: + await _revoke_refresh_token_at_idp(oauth_ctx, refresh_token) + await storage.delete_refresh_token(user_id) + logger.info("Refresh token revoked + deleted for user %s", user_id) + except Exception as e: + # Logout must always succeed locally; log and continue. + logger.warning("Logout cleanup failed (continuing): %s", e) + finally: + # Always drop the browser_sessions row, even when the + # refresh-token cleanup above failed — otherwise an orphan + # row lingers until the hourly cleanup cron (PR #758 round-5 + # review medium 1). Not exploitable (SessionAuthBackend + # already rejects sessions without a live refresh token), but + # a correctness gap worth closing here. + try: + await storage.delete_browser_session(session_id) + except Exception as e: + logger.warning( + "Failed to delete browser session %s…: %s", session_id[:8], e + ) response = RedirectResponse(next_url, status_code=302) - response.delete_cookie("mcp_session") + # Match the attributes from set_cookie so browsers reliably evict the + # cookie even on edge-case implementations that consider security flags + # when matching for deletion. + response.delete_cookie( + "mcp_session", + httponly=True, + secure=_should_use_secure_cookies(), + samesite="lax", + ) logger.info("User logged out, session cookie cleared") return response + + +async def _revoke_refresh_token_at_idp(oauth_ctx: dict, refresh_token: str) -> None: + """Best-effort RFC 7009 revocation against the IdP. + + Silent on failure: revoking remotely is a defense-in-depth step on top + of deleting the local copy, and we don't want logout to error if the + IdP is unreachable or doesn't advertise a revocation endpoint. + """ + # Production oauth_context nests config under "config" (see app.py + # starlette_lifespan). A flat shape is also accepted for tests and + # historical callers. + cfg = oauth_ctx.get("config") or oauth_ctx + settings = get_settings() + try: + discovery_url = cfg.get("discovery_url") or settings.oidc_discovery_url + if not discovery_url and settings.nextcloud_host: + # Strip trailing slash so a host configured as + # ``https://cloud.example.com/`` doesn't produce a double-slash + # in the well-known URL (PR #758 round-4 review nit 5). + discovery_url = ( + f"{settings.nextcloud_host.rstrip('/')}" + "/.well-known/openid-configuration" + ) + 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: + response = await http_client.post( + revocation_endpoint, + data={ + "token": refresh_token, + "token_type_hint": "refresh_token", + }, + auth=(client_id, client_secret), + ) + if response.status_code >= 400: + logger.warning( + "Refresh token revocation returned HTTP %s", response.status_code + ) + except Exception as e: + logger.warning("Refresh token revocation failed: %s", e) diff --git a/nextcloud_mcp_server/auth/oauth_routes.py b/nextcloud_mcp_server/auth/oauth_routes.py index f19d9b50..92ac6268 100644 --- a/nextcloud_mcp_server/auth/oauth_routes.py +++ b/nextcloud_mcp_server/auth/oauth_routes.py @@ -30,13 +30,17 @@ from typing import Any from urllib.parse import unquote, urlencode from urllib.parse import urlparse as parse_url -import jwt from starlette.requests import Request from starlette.responses import HTMLResponse, JSONResponse, RedirectResponse from nextcloud_mcp_server.auth.browser_oauth_routes import oauth_login_callback from nextcloud_mcp_server.auth.client_registry import get_client_registry from nextcloud_mcp_server.auth.storage import RefreshTokenStorage +from nextcloud_mcp_server.auth.token_utils import ( + IdTokenVerificationError, + get_oidc_discovery, + verify_id_token, +) from nextcloud_mcp_server.config import get_settings from ..http import nextcloud_httpx_client @@ -88,6 +92,7 @@ class ASProxySession: code_challenge: str code_challenge_method: str requested_scopes: str + nonce: str created_at: float = field(default_factory=time.time) expires_at: float = field(default_factory=lambda: time.time() + 600) @@ -100,10 +105,6 @@ class ASProxySession: _proxy_codes: dict[str, ProxyCodeEntry] = {} _as_proxy_sessions: dict[str, ASProxySession] = {} -# OIDC discovery document cache (URL → (expires_at, data)) -_discovery_cache: dict[str, tuple[float, dict[str, Any]]] = {} -_DISCOVERY_CACHE_TTL = 300 # 5 minutes - # DCR rate limiting (IP → [timestamps]) _dcr_rate_limit: dict[str, list[float]] = {} _DCR_RATE_LIMIT_MAX = 10 # max requests @@ -135,26 +136,6 @@ def _transform_scopes_for_idp(scopes: str, resource_server_id: str) -> str: ) -async def _get_cached_discovery(url: str) -> dict[str, Any]: - """Fetch OIDC discovery document with caching (5-minute TTL). - - Follows redirects so the configured discovery URL works against Nextcloud - instances without pretty URLs enabled, where ``/.well-known/openid-configuration`` - issues a 301 to ``/index.php/.well-known/openid-configuration``. - """ - now = time.time() - if url in _discovery_cache: - expires_at, data = _discovery_cache[url] - if now < expires_at: - return data - async with nextcloud_httpx_client(follow_redirects=True) as http_client: - response = await http_client.get(url) - response.raise_for_status() - data = response.json() - _discovery_cache[url] = (now + _DISCOVERY_CACHE_TTL, data) - return data - - def _cleanup_expired_proxy_codes() -> None: """Remove expired proxy codes and sessions.""" now = time.time() @@ -286,7 +267,7 @@ async def oauth_authorize(request: Request) -> RedirectResponse | JSONResponse: ) if not is_valid: - logger.warning(f"Client validation failed: {error_msg}") + logger.warning("Client validation failed: %s", error_msg) return JSONResponse( { "error": "unauthorized_client", @@ -313,6 +294,10 @@ async def oauth_authorize(request: Request) -> RedirectResponse | JSONResponse: # We do NOT forward PKCE to Nextcloud — the MCP server is a confidential client. server_state = secrets.token_urlsafe(32) + # OIDC nonce binds the IdP's ID token to THIS authorization request, + # blocking ID-token replay across flows (PR #758 round-2 finding 2). + server_nonce = secrets.token_urlsafe(32) + requested_scope = request.query_params.get("scope", "") default_scopes = "openid profile email" resource_scopes = oauth_config.get("scopes", "") @@ -330,6 +315,7 @@ async def oauth_authorize(request: Request) -> RedirectResponse | JSONResponse: code_challenge=code_challenge, code_challenge_method=code_challenge_method, requested_scopes=scopes, + nonce=server_nonce, ) # Use MCP server's own client_id with Nextcloud @@ -340,10 +326,10 @@ async def oauth_authorize(request: Request) -> RedirectResponse | JSONResponse: callback_uri = f"{mcp_server_url}/oauth/callback" logger.info("AS Proxy: Intermediary authorization flow") - logger.info(f" Client: {client_id}") - logger.info(f" MCP server client_id: {mcp_server_client_id}") - logger.info(f" Server callback: {callback_uri}") - logger.info(f" Scopes: {scopes}") + logger.info(" Client: %s", client_id) + logger.info(" MCP server client_id: %s", mcp_server_client_id) + logger.info(" Server callback: %s", callback_uri) + logger.info(" Scopes: %s", scopes) # Discover Nextcloud authorization endpoint discovery_url = oauth_config.get("discovery_url") @@ -356,7 +342,7 @@ async def oauth_authorize(request: Request) -> RedirectResponse | JSONResponse: status_code=500, ) - discovery = await _get_cached_discovery(discovery_url) + discovery = await get_oidc_discovery(discovery_url) authorization_endpoint = discovery["authorization_endpoint"] # Replace internal Docker hostname with public URL for browser access @@ -383,7 +369,7 @@ async def oauth_authorize(request: Request) -> RedirectResponse | JSONResponse: ) idp_scope_str = _transform_scopes_for_idp(scopes, resource_server_id) if resource_server_id: - logger.info(f" IdP scopes (prefixed): {idp_scope_str}") + logger.info(" IdP scopes (prefixed): %s", idp_scope_str) # Redirect to Nextcloud with MCP server's own client_id (no PKCE — confidential client) idp_params = { @@ -392,12 +378,13 @@ async def oauth_authorize(request: Request) -> RedirectResponse | JSONResponse: "response_type": "code", "scope": idp_scope_str, "state": server_state, + "nonce": server_nonce, "prompt": "consent", "resource": f"{mcp_server_url}/mcp", # MCP server audience } auth_url = f"{authorization_endpoint}?{urlencode(idp_params)}" - logger.info(f"Redirecting to Nextcloud OIDC: {auth_url.split('?')[0]}") + logger.info("Redirecting to Nextcloud OIDC: %s", auth_url.split("?")[0]) return RedirectResponse(auth_url, status_code=302) @@ -466,7 +453,7 @@ async def oauth_authorize_nextcloud( # supporting the offline_access scope. discovery_url = oauth_config.get("discovery_url") if discovery_url: - disc = await _get_cached_discovery(discovery_url) + disc = await get_oidc_discovery(discovery_url) scopes_supported = disc.get("scopes_supported") if scopes_supported is None or "offline_access" in scopes_supported: scopes += " offline_access" @@ -478,7 +465,12 @@ async def oauth_authorize_nextcloud( digest = hashlib.sha256(code_verifier.encode()).digest() code_challenge = urlsafe_b64encode(digest).decode().rstrip("=") - # Store code_verifier in session for retrieval during callback + # OIDC nonce binds the IdP-returned ID token to THIS auth request + # (PR #758 round-3 finding 1). Browser flow + AS proxy already do + # this; Flow 2 is the third path and was missing it. + nonce = secrets.token_urlsafe(32) + + # Store code_verifier + nonce in session for retrieval during callback storage = oauth_ctx["storage"] await storage.store_oauth_session( session_id=state, @@ -487,7 +479,11 @@ async def oauth_authorize_nextcloud( state=state, code_challenge=code_challenge, code_challenge_method="S256", - mcp_authorization_code=code_verifier, # Store code_verifier here temporarily + # `mcp_authorization_code` field reused to store the PKCE + # code_verifier (one-time-use). Renaming the column requires a + # schema migration. + mcp_authorization_code=code_verifier, + nonce=nonce, flow_type="flow2", ttl_seconds=600, # 10 minutes ) @@ -503,7 +499,7 @@ async def oauth_authorize_nextcloud( status_code=500, ) - discovery = await _get_cached_discovery(discovery_url) + discovery = await get_oidc_discovery(discovery_url) authorization_endpoint = discovery["authorization_endpoint"] # Fix internal hostname for browser access @@ -525,6 +521,7 @@ async def oauth_authorize_nextcloud( "response_type": "code", "scope": scopes, "state": state, + "nonce": nonce, "code_challenge": code_challenge, "code_challenge_method": "S256", "prompt": "consent", # Force consent to show resource access @@ -559,7 +556,7 @@ async def oauth_callback_nextcloud(request: Request): error_description = request.query_params.get( "error_description", "Authorization failed" ) - logger.error(f"Flow 2 authorization error: {error} - {error_description}") + logger.error("Flow 2 authorization error: %s - %s", error, error_description) return JSONResponse( { "error": error, @@ -585,15 +582,32 @@ async def oauth_callback_nextcloud(request: Request): storage: RefreshTokenStorage = oauth_ctx["storage"] oauth_config = oauth_ctx["config"] - # Retrieve code_verifier from session storage (PKCE required by Nextcloud OIDC) - code_verifier = "" + # Retrieve code_verifier + nonce from session storage (PKCE + OIDC + # nonce binding both required for Flow 2 — round-3 finding 1). Fail + # closed when the row is missing/expired so PKCE + nonce verification + # are not silently bypassed (round-6 review). oauth_session = await storage.get_oauth_session(state) - if oauth_session: - # code_verifier was stored in mcp_authorization_code field - code_verifier = oauth_session.get("mcp_authorization_code", "") - logger.info( - f"Retrieved code_verifier for Flow 2 callback (state={state[:16]}...)" + if not oauth_session: + logger.warning("Flow 2 callback received unknown/expired state=%s", state[:16]) + return JSONResponse( + { + "error": "invalid_request", + "error_description": ( + "Unknown or expired session — please retry the OAuth flow" + ), + }, + status_code=400, ) + # `mcp_authorization_code` field reused to store the PKCE code_verifier + # (one-time-use). Renaming the column requires a schema migration. + code_verifier = oauth_session.get("mcp_authorization_code", "") + nonce = oauth_session.get("nonce") + logger.info("Retrieved code_verifier for Flow 2 callback (state=%s…)", state[:16]) + # One-time-use session: delete eagerly so the stored code_verifier + # can't be replayed for the remainder of the oauth_sessions TTL. + # Mirrors browser_oauth_routes.oauth_login_callback (PR #758 + # follow-up review). + await storage.delete_oauth_session(state) # Exchange code for tokens mcp_server_client_id = os.getenv( @@ -615,7 +629,7 @@ async def oauth_callback_nextcloud(request: Request): status_code=500, ) - discovery = await _get_cached_discovery(discovery_url) + discovery = await get_oidc_discovery(discovery_url) token_endpoint = discovery["token_endpoint"] # Build token exchange params @@ -643,23 +657,36 @@ async def oauth_callback_nextcloud(request: Request): refresh_token = token_data.get("refresh_token") id_token = token_data.get("id_token") - # Decode ID token to get user info - logger.info("=" * 60) - logger.info("oauth_callback_nextcloud: Extracting user_id from ID token") - logger.info("=" * 60) + # Verify ID token signature + claims (issue #626 finding 1). + # ``expected_nonce`` is the per-request nonce stored on the + # oauth_session row (PR #758 round-3 finding 1). ``nonce`` is already + # ``str | None`` and ``secrets.token_urlsafe`` never produces an empty + # string, so passing it directly is correct — pre-migration-006 rows + # surface as ``None`` from ``oauth_session.get("nonce")``, which + # ``verify_id_token`` already treats as "skip the check". + logger.info("oauth_callback_nextcloud: Verifying ID token") try: - userinfo = jwt.decode(id_token, options={"verify_signature": False}) - user_id = userinfo.get("sub") - username = userinfo.get("preferred_username") or userinfo.get("email") - logger.info(" ✓ ID token decode SUCCESSFUL") - logger.info(f" Extracted user_id: {user_id}") - logger.info(f" Username: {username}") - logger.info(f" ID token payload keys: {list(userinfo.keys())}") - logger.info(f"Flow 2: User {username} provisioned resource access") - except Exception as e: - logger.error(f" ✗ ID token decode FAILED: {type(e).__name__}: {e}") - user_id = "unknown" - logger.error(f" Using fallback user_id: {user_id}") + userinfo = await verify_id_token( + id_token, + discovery_url=discovery_url, + expected_audience=mcp_server_client_id, + expected_nonce=nonce, + ) + except IdTokenVerificationError as e: + logger.error("ID token verification failed: %s", e) + return JSONResponse( + { + "error": "invalid_token", + "error_description": "ID token failed verification", + }, + status_code=400, + ) + + user_id = userinfo["sub"] + username = userinfo.get("preferred_username") or userinfo.get("email") + logger.info( + "Flow 2: User %s (sub=%s) provisioned resource access", username, user_id + ) # Store master refresh token for Flow 2 if refresh_token: @@ -672,17 +699,22 @@ async def oauth_callback_nextcloud(request: Request): refresh_expires_in = token_data.get("refresh_expires_in") refresh_expires_at = None if refresh_expires_in: - refresh_expires_at = int(time.time()) + refresh_expires_in - logger.info(f" refresh_expires_in: {refresh_expires_in}s") - logger.info(f" refresh_expires_at: {refresh_expires_at}") + # Some IdPs (e.g. AWS Cognito) return refresh_expires_in as a JSON + # string rather than an int; coerce to be safe. + refresh_expires_at = int(time.time()) + int(refresh_expires_in) + logger.debug(" refresh_expires_in: %ss", refresh_expires_in) + logger.debug(" refresh_expires_at: %s", refresh_expires_at) - logger.info("Storing refresh token:") - logger.info(f" user_id: {user_id}") - logger.info(" flow_type: flow2") - logger.info(" token_audience: nextcloud") - logger.info(f" provisioning_client_id: {state[:16]}...") - logger.info(f" scopes: {granted_scopes}") - logger.info(f" expires_at: {refresh_expires_at}") + # Identity-bearing fields stay at DEBUG so they don't reach + # multi-tenant log aggregation on every Flow 2 provision (PR #758 + # round-7 minor). + logger.debug("Storing refresh token:") + logger.debug(" user_id: %s", user_id) + logger.debug(" flow_type: flow2") + logger.debug(" token_audience: nextcloud") + logger.debug(" provisioning_client_id: %s...", state[:16]) + logger.debug(" scopes: %s", granted_scopes) + logger.debug(" expires_at: %s", refresh_expires_at) await storage.store_refresh_token( user_id=user_id, @@ -693,8 +725,8 @@ async def oauth_callback_nextcloud(request: Request): scopes=granted_scopes, expires_at=refresh_expires_at, ) - logger.info(f"✓ Stored Flow 2 master refresh token for user {user_id}") - logger.info("=" * 60) + logger.debug("✓ Stored Flow 2 master refresh token for user %s", user_id) + logger.debug("=" * 60) # Return success HTML page success_html = """ @@ -775,7 +807,7 @@ async def oauth_callback(request: Request): oauth_session.get("flow_type", "browser") if oauth_session else "browser" ) - logger.info(f"Unified callback: flow_type={flow_type} (from session lookup)") + logger.info("Unified callback: flow_type=%s (from session lookup)", flow_type) if flow_type == "flow2": # Flow 2: Resource Provisioning - MCP server gets delegated Nextcloud access @@ -789,7 +821,7 @@ async def oauth_callback(request: Request): else: # Unknown flow type - logger.warning(f"Unknown flow_type in OAuth session: {flow_type}") + logger.warning("Unknown flow_type in OAuth session: %s", flow_type) return JSONResponse( { "error": "invalid_request", @@ -819,7 +851,7 @@ async def _oauth_callback_as_proxy( error_description = request.query_params.get( "error_description", "Authorization failed" ) - logger.error(f"AS proxy callback error: {error} - {error_description}") + logger.error("AS proxy callback error: %s - %s", error, error_description) # Retrieve session to redirect back to client with error session = _as_proxy_sessions.pop(server_state, None) @@ -903,7 +935,7 @@ async def _oauth_callback_as_proxy( status_code=500, ) - discovery = await _get_cached_discovery(discovery_url) + discovery = await get_oidc_discovery(discovery_url) token_endpoint = discovery["token_endpoint"] # Exchange auth code with Nextcloud (server-side, confidential client, no PKCE) @@ -940,6 +972,35 @@ 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. + # + # ``expected_nonce`` is the per-request nonce we forwarded to the IdP + # in oauth_authorize (PR #758 round-2 finding 2). ASProxySession is + # in-memory only and ``nonce`` is now a required field, so for any + # session created via the current code path this is always set; the + # ``or None`` is defence-in-depth and a no-op in practice. + 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, + expected_nonce=session.nonce or None, + ) + 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( @@ -1145,7 +1206,7 @@ async def _token_authorization_code(request: Request, form) -> JSONResponse: ) if not _verify_pkce_s256(code_verifier, entry.code_challenge): - logger.warning(f"PKCE verification failed for client {entry.client_id}") + logger.warning("PKCE verification failed for client %s", entry.client_id) return JSONResponse( { "error": "invalid_grant", @@ -1155,7 +1216,7 @@ async def _token_authorization_code(request: Request, form) -> JSONResponse: ) logger.info( - f"AS proxy token: Returning Nextcloud token for client {entry.client_id}" + "AS proxy token: Returning Nextcloud token for client %s", entry.client_id ) # Return the stored Nextcloud token response directly @@ -1216,7 +1277,7 @@ async def _token_refresh(request: Request, form) -> JSONResponse: status_code=500, ) - discovery = await _get_cached_discovery(discovery_url) + discovery = await get_oidc_discovery(discovery_url) token_endpoint = discovery["token_endpoint"] # Proxy refresh request to Nextcloud @@ -1288,7 +1349,7 @@ async def oauth_register_proxy(request: Request) -> JSONResponse: # Remove timestamps outside the window timestamps = [t for t in timestamps if now - t < _DCR_RATE_LIMIT_WINDOW] if len(timestamps) >= _DCR_RATE_LIMIT_MAX: - logger.warning(f"DCR rate limit exceeded for {client_ip}") + logger.warning("DCR rate limit exceeded for %s", client_ip) return JSONResponse( { "error": "too_many_requests", @@ -1305,7 +1366,7 @@ async def oauth_register_proxy(request: Request) -> JSONResponse: registration_endpoint = None if discovery_url: try: - discovery = await _get_cached_discovery(discovery_url) + discovery = await get_oidc_discovery(discovery_url) registration_endpoint = discovery.get("registration_endpoint") except Exception: logger.warning("Failed to fetch OIDC discovery for DCR endpoint") @@ -1324,7 +1385,7 @@ async def oauth_register_proxy(request: Request) -> JSONResponse: status_code=400, ) - logger.info(f"DCR proxy: Forwarding registration to {registration_endpoint}") + logger.info("DCR proxy: Forwarding registration to %s", registration_endpoint) async with nextcloud_httpx_client() as http_client: response = await http_client.post( @@ -1360,7 +1421,7 @@ async def oauth_register_proxy(request: Request) -> JSONResponse: redirect_uris=redirect_uris, name=client_name, ) - logger.info(f"DCR proxy: Registered client {new_client_id} in local registry") + logger.info("DCR proxy: Registered client %s in local registry", new_client_id) return JSONResponse(nc_response, status_code=response.status_code) diff --git a/nextcloud_mcp_server/auth/provisioning_decorator.py b/nextcloud_mcp_server/auth/provisioning_decorator.py index 585b6ff2..b28eecc3 100644 --- a/nextcloud_mcp_server/auth/provisioning_decorator.py +++ b/nextcloud_mcp_server/auth/provisioning_decorator.py @@ -9,12 +9,12 @@ import functools import logging from typing import Callable -import jwt +from mcp.server.auth.middleware.auth_context import get_access_token from mcp.server.fastmcp import Context from mcp.shared.exceptions import McpError from mcp.types import ErrorData -from nextcloud_mcp_server.auth.storage import RefreshTokenStorage +from nextcloud_mcp_server.auth.storage import get_shared_storage logger = logging.getLogger(__name__) @@ -65,16 +65,12 @@ def require_provisioning(func: Callable) -> Callable: return await func(*args, **kwargs) # Offline access mode - check if user has completed Flow 2 provisioning - # Get user_id from authorization token - user_id = None - if hasattr(ctx, "authorization") and ctx.authorization: - try: - token = ctx.authorization.token - payload = jwt.decode(token, options={"verify_signature": False}) - user_id = payload.get("sub") - logger.debug(f"Checking provisioning for user: {user_id}") - except Exception as e: - logger.warning(f"Failed to extract user_id from token: {e}") + # Read user_id from the verified AccessToken populated by + # UnifiedTokenVerifier; no second decode of the raw JWT here. + access_token = get_access_token() + user_id = access_token.resource if access_token else None + if user_id: + logger.debug("Checking provisioning for user: %s", user_id) if not user_id: raise McpError( @@ -84,9 +80,9 @@ def require_provisioning(func: Callable) -> Callable: ) ) - # Check provisioning status - storage = RefreshTokenStorage.from_env() - await storage.initialize() + # Check provisioning status — share the process-wide singleton + # rather than initialising a new sqlite handle per tool call. + storage = await get_shared_storage() refresh_data = await storage.get_refresh_token(user_id) @@ -149,17 +145,12 @@ def require_provisioning_or_suggest(func: Callable) -> Callable: if ctx: # Try to check provisioning status try: - # Get user_id from authorization token - user_id = None - if hasattr(ctx, "authorization") and ctx.authorization: - token = ctx.authorization.token - payload = jwt.decode(token, options={"verify_signature": False}) - user_id = payload.get("sub") + access_token = get_access_token() + user_id = access_token.resource if access_token else None if user_id: - # Check provisioning status - storage = RefreshTokenStorage.from_env() - await storage.initialize() + # Check provisioning status using the shared singleton. + storage = await get_shared_storage() refresh_data = await storage.get_refresh_token(user_id) diff --git a/nextcloud_mcp_server/auth/session_backend.py b/nextcloud_mcp_server/auth/session_backend.py index 1a3dc714..87364f09 100644 --- a/nextcloud_mcp_server/auth/session_backend.py +++ b/nextcloud_mcp_server/auth/session_backend.py @@ -22,6 +22,18 @@ class SessionAuthBackend(AuthenticationBackend): For BasicAuth mode: Always authenticates as the configured user. For OAuth mode: Checks for valid session cookie with stored refresh token. + + Behavior note — silent invalidation on refresh-token TTL expiry: + The OAuth path requires *both* a live ``browser_sessions`` row and a + live ``refresh_tokens`` row for the resolved user. Logout deletes + both atomically, so a logged-out user always fails closed here. + However, if the refresh token expires by TTL (without an explicit + logout) the row is removed by ``get_refresh_token`` and the browser + session becomes unusable — the user simply gets redirected to + ``/oauth/login``. This is intentional defense-in-depth: the + refresh-token check is what makes a leaked or stale browser cookie + unusable after revocation. Do not relax this without first removing + the cleanup invariant on logout (PR #758 round-4 review medium 2). """ def __init__(self, oauth_enabled: bool = False): @@ -52,45 +64,58 @@ class SessionAuthBackend(AuthenticationBackend): username = os.getenv("NEXTCLOUD_USERNAME", "admin") return AuthCredentials(["authenticated", "admin"]), SimpleUser(username) - # OAuth mode: Check for session cookie + # OAuth mode: opaque random session_id cookie -> user_id mapping. + # Replaces the prior `mcp_session=` cookie pattern (issue + # #626 finding 2). The cookie value is no longer the user identity; + # we look it up server-side and reject unknown / expired sessions. session_id = conn.cookies.get("mcp_session") - logger.info( - f"Session authentication check - cookie present: {session_id is not None}, path: {conn.url.path}" - ) if not session_id: logger.info("No session cookie found - redirecting to login") return None - logger.info(f"Found session cookie: {session_id[:16]}...") - - # Get OAuth context from app state oauth_context = getattr(conn.app.state, "oauth_context", None) if not oauth_context: logger.warning("OAuth context not available in app state") return None - # Validate session storage = oauth_context.get("storage") if not storage: logger.warning("OAuth storage not available") return None try: - # Check if user has refresh token (indicates logged-in session) - logger.info(f"Looking up refresh token for session: {session_id[:16]}...") - token_data = await storage.get_refresh_token(session_id) - if not token_data: - logger.warning( - f"No refresh token found for session {session_id[:16]}..." + user_id = await storage.get_browser_session_user(session_id) + if not user_id: + logger.info( + "Browser session not found or expired (sid=%s…)", session_id[:8] ) return None - # Session is valid - use session_id (which is user_id from ID token) as username - username = session_id - logger.info(f"✓ Session authenticated successfully: {username[:16]}...") + # Defense-in-depth: only authenticate sessions for users that + # actually have a refresh token persisted. Logout deletes both, + # so an expired/revoked user state will fail closed here. + token_data = await storage.get_refresh_token(user_id) + if not token_data: + logger.warning( + "Session %s… has no refresh token for user %s; rejecting", + session_id[:8], + user_id, + ) + # Proactively evict the orphan so the table doesn't accumulate + # rows that the auth check will keep rejecting until TTL + # cleanup (PR #758 round-7 minor). + try: + await storage.delete_browser_session(session_id) + except Exception as e: + logger.warning( + "Failed to delete orphaned browser session %s…: %s", + session_id[:8], + e, + ) + return None - return AuthCredentials(["authenticated"]), SimpleUser(username) + return AuthCredentials(["authenticated"]), SimpleUser(user_id) except Exception as e: - logger.warning(f"Session validation error: {e}") + logger.warning("Session validation error: %s", e) return None diff --git a/nextcloud_mcp_server/auth/storage.py b/nextcloud_mcp_server/auth/storage.py index ed6675df..e7e8d0b3 100644 --- a/nextcloud_mcp_server/auth/storage.py +++ b/nextcloud_mcp_server/auth/storage.py @@ -29,9 +29,10 @@ import json import logging import os import socket +import sqlite3 import time from pathlib import Path -from typing import Any, Optional +from typing import Any import aiosqlite import anyio @@ -139,10 +140,25 @@ class RefreshTokenStorage: 1. New database: Run migrations from scratch 2. Pre-Alembic database: Stamp with initial revision (no changes) 3. Alembic-managed database: Upgrade to latest version + + Raises: + RuntimeError: when the underlying SQLite library is older than + 3.35, which is required for ``DELETE ... RETURNING`` used by + ``delete_browser_session`` (PR #758 round-5 review low 2). + Ubuntu 20.04 ships SQLite 3.31, so deployers on that + baseline must upgrade or use a newer Python image. """ if self._initialized: return + if sqlite3.sqlite_version_info < (3, 35): + raise RuntimeError( + "SQLite >= 3.35 is required (DELETE ... RETURNING is used " + "by delete_browser_session); detected " + f"{sqlite3.sqlite_version}. Upgrade SQLite or use a Python " + "image with a newer bundled libsqlite3." + ) + # Ensure directory exists db_dir = Path(self.db_path).parent db_dir.mkdir(parents=True, exist_ok=True) @@ -205,11 +221,11 @@ class RefreshTokenStorage: self, user_id: str, refresh_token: str, - expires_at: Optional[int] = None, + expires_at: int | None = None, flow_type: str = "hybrid", token_audience: str = "nextcloud", - provisioning_client_id: Optional[str] = None, - scopes: Optional[list[str]] = None, + provisioning_client_id: str | None = None, + scopes: list[str] | None = None, ) -> None: """ Store encrypted refresh token for user. @@ -227,8 +243,14 @@ class RefreshTokenStorage: if not self._initialized: await self.initialize() - # Type narrowing: cipher is set after initialize() - assert self.cipher is not None + # ``assert`` is stripped under ``python -O``, which would silently + # turn a missing TOKEN_ENCRYPTION_KEY into an ``AttributeError`` on + # the next ``self.cipher.encrypt(...)``. Raise explicitly instead + # (PR #758 round-4 review medium 1). + if self.cipher is None: + raise RuntimeError( + "TOKEN_ENCRYPTION_KEY is not set — token storage operations unavailable" + ) encrypted_token = self.cipher.encrypt(refresh_token.encode()) now = int(time.time()) scopes_json = json.dumps(scopes) if scopes else None @@ -313,7 +335,7 @@ class RefreshTokenStorage: logger.debug(f"Cached user profile for {user_id}") - async def get_user_profile(self, user_id: str) -> Optional[dict[str, Any]]: + async def get_user_profile(self, user_id: str) -> dict[str, Any] | None: """ Retrieve cached user profile data. @@ -351,7 +373,7 @@ class RefreshTokenStorage: return profile_data - async def get_refresh_token(self, user_id: str) -> Optional[dict]: + async def get_refresh_token(self, user_id: str) -> dict | None: """ Retrieve and decrypt refresh token for user. @@ -374,8 +396,14 @@ class RefreshTokenStorage: if not self._initialized: await self.initialize() - # Type narrowing: cipher is set after initialize() - assert self.cipher is not None + # ``assert`` is stripped under ``python -O``, which would silently + # turn a missing TOKEN_ENCRYPTION_KEY into an ``AttributeError`` on + # the next ``self.cipher.encrypt(...)``. Raise explicitly instead + # (PR #758 round-4 review medium 1). + if self.cipher is None: + raise RuntimeError( + "TOKEN_ENCRYPTION_KEY is not set — token storage operations unavailable" + ) start_time = time.time() try: @@ -444,7 +472,7 @@ class RefreshTokenStorage: async def get_refresh_token_by_provisioning_client_id( self, provisioning_client_id: str - ) -> Optional[dict]: + ) -> dict | None: """ Retrieve and decrypt refresh token by provisioning_client_id (state parameter). @@ -461,8 +489,14 @@ class RefreshTokenStorage: if not self._initialized: await self.initialize() - # Type narrowing: cipher is set after initialize() - assert self.cipher is not None + # ``assert`` is stripped under ``python -O``, which would silently + # turn a missing TOKEN_ENCRYPTION_KEY into an ``AttributeError`` on + # the next ``self.cipher.encrypt(...)``. Raise explicitly instead + # (PR #758 round-4 review medium 1). + if self.cipher is None: + raise RuntimeError( + "TOKEN_ENCRYPTION_KEY is not set — token storage operations unavailable" + ) async with aiosqlite.connect(self.db_path) as db: async with db.execute( @@ -617,8 +651,8 @@ class RefreshTokenStorage: client_id_issued_at: int, client_secret_expires_at: int, redirect_uris: list[str], - registration_access_token: Optional[str] = None, - registration_client_uri: Optional[str] = None, + registration_access_token: str | None = None, + registration_client_uri: str | None = None, ) -> None: """ Store encrypted OAuth client credentials. @@ -635,8 +669,14 @@ class RefreshTokenStorage: if not self._initialized: await self.initialize() - # Type narrowing: cipher is set after initialize() - assert self.cipher is not None + # ``assert`` is stripped under ``python -O``, which would silently + # turn a missing TOKEN_ENCRYPTION_KEY into an ``AttributeError`` on + # the next ``self.cipher.encrypt(...)``. Raise explicitly instead + # (PR #758 round-4 review medium 1). + if self.cipher is None: + raise RuntimeError( + "TOKEN_ENCRYPTION_KEY is not set — token storage operations unavailable" + ) # Encrypt sensitive data encrypted_secret = self.cipher.encrypt(client_secret.encode()) @@ -689,7 +729,7 @@ class RefreshTokenStorage: auth_method="oauth", ) - async def get_oauth_client(self) -> Optional[dict]: + async def get_oauth_client(self) -> dict | None: """ Retrieve and decrypt OAuth client credentials. @@ -708,8 +748,14 @@ class RefreshTokenStorage: if not self._initialized: await self.initialize() - # Type narrowing: cipher is set after initialize() - assert self.cipher is not None + # ``assert`` is stripped under ``python -O``, which would silently + # turn a missing TOKEN_ENCRYPTION_KEY into an ``AttributeError`` on + # the next ``self.cipher.encrypt(...)``. Raise explicitly instead + # (PR #758 round-4 review medium 1). + if self.cipher is None: + raise RuntimeError( + "TOKEN_ENCRYPTION_KEY is not set — token storage operations unavailable" + ) async with aiosqlite.connect(self.db_path) as db: async with db.execute( @@ -827,9 +873,9 @@ class RefreshTokenStorage: self, event: str, user_id: str, - resource_type: Optional[str] = None, - resource_id: Optional[str] = None, - auth_method: Optional[str] = None, + resource_type: str | None = None, + resource_id: str | None = None, + auth_method: str | None = None, ) -> None: """ Log operation to audit log. @@ -866,8 +912,8 @@ class RefreshTokenStorage: async def get_audit_logs( self, - user_id: Optional[str] = None, - since: Optional[int] = None, + user_id: str | None = None, + since: int | None = None, limit: int = 100, ) -> list[dict]: """ @@ -909,14 +955,15 @@ class RefreshTokenStorage: self, session_id: str, client_redirect_uri: str, - state: Optional[str] = None, - code_challenge: Optional[str] = None, - code_challenge_method: Optional[str] = None, - mcp_authorization_code: Optional[str] = None, - client_id: Optional[str] = None, + state: str | None = None, + code_challenge: str | None = None, + code_challenge_method: str | None = None, + mcp_authorization_code: str | None = None, + client_id: str | None = None, flow_type: str = "hybrid", is_provisioning: bool = False, - requested_scopes: Optional[str] = None, + requested_scopes: str | None = None, + nonce: str | None = None, ttl_seconds: int = 600, # 10 minutes ) -> None: """ @@ -933,6 +980,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 +996,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 +1010,7 @@ class RefreshTokenStorage: flow_type, is_provisioning, requested_scopes, + nonce, now, expires_at, ), @@ -969,7 +1019,7 @@ class RefreshTokenStorage: logger.debug(f"Stored OAuth session {session_id} (expires in {ttl_seconds}s)") - async def get_oauth_session(self, session_id: str) -> Optional[dict]: + async def get_oauth_session(self, session_id: str) -> dict | None: """ Retrieve OAuth session by session ID. @@ -1001,7 +1051,7 @@ class RefreshTokenStorage: async def get_oauth_session_by_mcp_code( self, mcp_authorization_code: str - ) -> Optional[dict]: + ) -> dict | None: """ Retrieve OAuth session by MCP authorization code. @@ -1037,9 +1087,9 @@ class RefreshTokenStorage: async def update_oauth_session( self, session_id: str, - user_id: Optional[str] = None, - idp_access_token: Optional[str] = None, - idp_refresh_token: Optional[str] = None, + user_id: str | None = None, + idp_access_token: str | None = None, + idp_refresh_token: str | None = None, ) -> bool: """ Update OAuth session with IdP token data. @@ -1133,6 +1183,140 @@ class RefreshTokenStorage: return deleted + # ============================================================================ + # Browser Sessions (OAuth admin UI) + # ============================================================================ + # + # Maps a cryptographically random `session_id` (cookie value) to the + # authenticated user_id. Replaces the prior `mcp_session=` + # cookie pattern (issue #626 finding 2). Cookie value is opaque, expires, + # and can be revoked server-side without forcing the user to roll their + # IdP `sub`. + + async def create_browser_session( + self, + session_id: str, + user_id: str, + ttl_seconds: int = 86400 * 30, + ) -> None: + """Persist a random session_id → user_id mapping for browser auth.""" + if not self._initialized: + await self.initialize() + + now = int(time.time()) + expires_at = now + ttl_seconds + + async with aiosqlite.connect(self.db_path) as db: + await db.execute( + """ + INSERT OR REPLACE INTO browser_sessions + (session_id, user_id, created_at, expires_at) + VALUES (?, ?, ?, ?) + """, + (session_id, user_id, now, expires_at), + ) + await db.commit() + + logger.debug( + "Stored browser session %s for user %s (expires in %ss)", + session_id[:8], + user_id, + ttl_seconds, + ) + + # Audit log to match the pattern used by the other security-relevant + # storage operations (PR #758 round-3 nit 5). Browser session + # establishment is a security-relevant event. + await self._audit_log( + event="create_browser_session", + user_id=user_id, + resource_type="browser_session", + resource_id=session_id[:8], + ) + + async def get_browser_session_user(self, session_id: str) -> str | None: + """Look up the user_id bound to a browser session_id, or None. + + Returns None when the session is unknown or expired. Expired rows + are deleted on encounter to keep the table small. + """ + if not self._initialized: + await self.initialize() + + async with aiosqlite.connect(self.db_path) as db: + db.row_factory = aiosqlite.Row + async with db.execute( + "SELECT user_id, expires_at FROM browser_sessions WHERE session_id = ?", + (session_id,), + ) as cursor: + row = await cursor.fetchone() + + if not row: + return None + + if row["expires_at"] < time.time(): + logger.debug("Browser session %s expired", session_id[:8]) + await self.delete_browser_session(session_id) + return None + + return row["user_id"] + + async def delete_browser_session(self, session_id: str) -> bool: + """Delete a browser session row. Returns True when a row was removed.""" + if not self._initialized: + await self.initialize() + + # DELETE ... RETURNING (SQLite ≥ 3.35) reads ``user_id`` atomically + # with the delete itself, so the audit log can't race against a + # concurrent delete that empties the row between SELECT and DELETE + # (PR #758 round-3 review). + user_id: str | None = None + async with aiosqlite.connect(self.db_path) as db: + async with db.execute( + "DELETE FROM browser_sessions WHERE session_id = ? RETURNING user_id", + (session_id,), + ) as cursor: + row = await cursor.fetchone() + await db.commit() + + deleted = row is not None + if deleted: + user_id = row[0] + logger.debug("Deleted browser session %s", session_id[:8]) + if user_id: + await self._audit_log( + event="delete_browser_session", + user_id=user_id, + resource_type="browser_session", + resource_id=session_id[:8], + ) + return deleted + + async def cleanup_expired_browser_sessions(self) -> int: + """Remove expired ``browser_sessions`` rows. + + Returns the number of rows deleted. Called by the periodic cleanup + task in ``app.py``. Without this users who never explicitly log out + leave session rows behind that only get deleted lazily on lookup + (PR #758 finding 6). + """ + if not self._initialized: + await self.initialize() + + now = int(time.time()) + + async with aiosqlite.connect(self.db_path) as db: + cursor = await db.execute( + "DELETE FROM browser_sessions WHERE expires_at < ?", (now,) + ) + await db.commit() + deleted = cursor.rowcount + + if deleted > 0: + logger.info("Cleaned up %s expired browser session(s)", deleted) + + return deleted + # ============================================================================ # Webhook Registration Tracking (both BasicAuth and OAuth modes) # ============================================================================ @@ -1312,7 +1496,7 @@ class RefreshTokenStorage: auth_method="app_password", ) - async def get_app_password(self, user_id: str) -> Optional[str]: + async def get_app_password(self, user_id: str) -> str | None: """ Retrieve and decrypt app password for a user. diff --git a/nextcloud_mcp_server/auth/templates/user_info.html b/nextcloud_mcp_server/auth/templates/user_info.html index 25f527b6..39b8826a 100644 --- a/nextcloud_mcp_server/auth/templates/user_info.html +++ b/nextcloud_mcp_server/auth/templates/user_info.html @@ -285,14 +285,18 @@
  • - - - - - - - Logout - + {# Logout is POST-only to defeat CSRF (PR #758 finding 5). + Style this +
diff --git a/nextcloud_mcp_server/auth/token_broker.py b/nextcloud_mcp_server/auth/token_broker.py index c4729edb..9885246f 100644 --- a/nextcloud_mcp_server/auth/token_broker.py +++ b/nextcloud_mcp_server/auth/token_broker.py @@ -20,7 +20,6 @@ from typing import Dict, Optional, Tuple import anyio import httpx -import jwt from nextcloud_mcp_server.auth.storage import RefreshTokenStorage @@ -489,35 +488,6 @@ class TokenBrokerService: ) return access_token, expires_in - async def _validate_token_audience(self, token: str, expected_audience: str): - """ - Validate that token has correct audience claim. - - Args: - token: JWT token to validate - expected_audience: Expected audience value - - Raises: - ValueError: If audience doesn't match - """ - try: - # Decode without verification to check claims - # In production, should verify signature - claims = jwt.decode(token, options={"verify_signature": False}) - - audience = claims.get("aud", []) - if isinstance(audience, str): - audience = [audience] - - if expected_audience not in audience: - raise ValueError( - f"Token audience {audience} doesn't include {expected_audience}" - ) - - except jwt.DecodeError as e: - # Token might be opaque, skip validation - logger.debug(f"Cannot decode token for audience validation: {e}") - async def refresh_master_token(self, user_id: str) -> bool: """ Refresh the master refresh token (periodic rotation). diff --git a/nextcloud_mcp_server/auth/token_utils.py b/nextcloud_mcp_server/auth/token_utils.py index d9a4cb4f..c7edb960 100644 --- a/nextcloud_mcp_server/auth/token_utils.py +++ b/nextcloud_mcp_server/auth/token_utils.py @@ -5,81 +5,291 @@ between server/ and auth/ layers. """ import logging -import os +import secrets +import time +from typing import Any +import anyio import jwt +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 nextcloud_mcp_server.auth.userinfo_routes import _query_idp_userinfo +from mcp.shared.exceptions import McpError +from mcp.types import ErrorData from ..http import nextcloud_httpx_client logger = logging.getLogger(__name__) -async def extract_user_id_from_token(ctx: Context) -> str: - """Extract user_id from the MCP access token (Flow 1). +# OIDC discovery + JWKS caches keyed by URL → (expires_at, data). Single +# source of truth for the codebase: oauth_routes / browser_oauth_routes both +# go through ``get_oidc_discovery`` which reads/writes _discovery_cache, so +# the first discovery fetch primes the cache for all later callers (PR #758 +# round-2 nit 3). 5-minute TTL. +_discovery_cache: dict[str, tuple[float, dict[str, Any]]] = {} +_jwks_cache: dict[str, tuple[float, dict[str, Any]]] = {} +_OIDC_CACHE_TTL = 300 - Handles both JWT and opaque tokens: - - JWT: Decode and extract 'sub' claim - - Opaque: Call userinfo endpoint to get 'sub' +# Per-URL fetch locks coalesce concurrent cache misses into a single HTTP +# request, preventing thundering-herd against the IdP at cache expiry +# (PR #758 round-3 review). Mirrors the lock-dict + meta-lock idiom from +# token_broker.py. +_fetch_locks: dict[str, anyio.Lock] = {} +_fetch_locks_lock = anyio.Lock() + + +async def _get_fetch_lock(url: str) -> anyio.Lock: + """Return the per-URL lock used to serialise cache-miss fetches.""" + async with _fetch_locks_lock: + lock = _fetch_locks.get(url) + if lock is None: + lock = anyio.Lock() + _fetch_locks[url] = lock + return lock + + +class IdTokenVerificationError(Exception): + """Raised when an OIDC ID token fails signature or claim verification.""" + + +async def _get_cached( + cache: dict[str, tuple[float, dict[str, Any]]], + url: str, + *, + follow_redirects: bool = False, +) -> dict[str, Any]: + """Return cached JSON response for *url* or fetch + cache on miss/expiry. + + ``follow_redirects`` is forwarded to ``nextcloud_httpx_client``: discovery + fetches against Nextcloud without pretty URLs need it (the configured + ``/.well-known/openid-configuration`` path issues a 301), but JWKS + fetches deliberately stay strict — the URL came from the discovery + document we already trust, so a redirect there would be suspicious. + + Concurrent callers seeing the same cache miss are coalesced via a + per-URL ``anyio.Lock``: only one fetch runs, the rest wait and read the + populated cache. + """ + entry = cache.get(url) + if entry is not None and time.time() < entry[0]: + return entry[1] + lock = await _get_fetch_lock(url) + try: + async with lock: + # Re-check inside the lock — a concurrent waiter may have already + # populated the cache before we acquired it. + entry = cache.get(url) + if entry is not None and time.time() < entry[0]: + return entry[1] + async with nextcloud_httpx_client( + follow_redirects=follow_redirects + ) as http_client: + response = await http_client.get(url) + response.raise_for_status() + data = response.json() + cache[url] = (time.time() + _OIDC_CACHE_TTL, data) + return data + finally: + # Drop the dict entry so a misconfigured deployment hitting + # arbitrary URLs can't grow ``_fetch_locks`` without bound (PR #758 + # round-4 review nit 4). Already-queued waiters share our local + # ``lock`` reference and remain coalesced; new arrivals lazily + # recreate a lock — by which time the cache is populated, so they + # short-circuit before reaching the lock anyway. + async with _fetch_locks_lock: + if _fetch_locks.get(url) is lock: + del _fetch_locks[url] + + +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. The fetch follows + redirects because Nextcloud without pretty URLs returns 301 from + ``/.well-known/openid-configuration`` to ``/index.php/.well-known/...``. + Single source of truth for OIDC discovery in the codebase + (PR #758 round-2 nit 3). + """ + return await _get_cached(_discovery_cache, discovery_url, follow_redirects=True) + + +async def verify_id_token( + id_token: str | None, + *, + discovery_url: str, + expected_audience: str, + expected_nonce: str | None = None, +) -> dict[str, Any]: + """Verify an OIDC ID token's signature and standard claims. + + Implements the verification steps required by OIDC core spec section + 3.1.3.7 (ID Token Validation) for the authorization-code flow: + - Signature against JWKS (RS256) + - Issuer matches the OP that issued the token + - Audience contains the expected client_id + - Token is not expired (`exp`) + - `iat` is well-formed (PyJWT default) + - `nonce` matches when one was included in the auth request + + Replaces the prior `jwt.decode(id_token, options={"verify_signature": False})` + pattern (issue #626 finding 1) on the OAuth callback paths. Args: - ctx: MCP context with access token + id_token: Raw ID token (JWT) string. + discovery_url: OIDC `.well-known/openid-configuration` URL of the IdP. + expected_audience: The MCP-server-side OAuth client_id used for this + authorization request. + expected_nonce: When the auth request included a nonce, the same value + so it can be checked here. None disables the nonce check (callers + that didn't bind a nonce in the auth request). Returns: - user_id extracted from token, or "default_user" as fallback + Decoded, verified ID-token claims. + + Raises: + IdTokenVerificationError: On any verification failure. + """ + if not id_token: + raise IdTokenVerificationError("ID token missing from token response") + + try: + discovery = await get_oidc_discovery(discovery_url) + + issuer = discovery.get("issuer") + jwks_uri = discovery.get("jwks_uri") + if not issuer or not jwks_uri: + raise IdTokenVerificationError( + "OIDC discovery response missing issuer or jwks_uri" + ) + + jwks_data = await _get_cached(_jwks_cache, jwks_uri) + except IdTokenVerificationError: + raise + except Exception as e: + raise IdTokenVerificationError( + f"Failed to fetch OIDC discovery / JWKS: {e}" + ) from e + + try: + jwks = PyJWKSet.from_dict(jwks_data) + unverified_header = jwt.get_unverified_header(id_token) + kid = unverified_header.get("kid") + if not kid: + raise IdTokenVerificationError("ID token header missing 'kid'") + try: + signing_key = jwks[kid] + except KeyError: + # Cache miss may indicate IdP key rotation. Refresh JWKS once + # before giving up, per OIDC core §10.1.1: when an unrecognised + # `kid` arrives the relying party should refetch the JWKS rather + # than waiting for cache TTL to elapse. + _jwks_cache.pop(jwks_uri, None) + try: + jwks_data = await _get_cached(_jwks_cache, jwks_uri) + jwks = PyJWKSet.from_dict(jwks_data) + signing_key = jwks[kid] + except KeyError as e: + raise IdTokenVerificationError( + f"No JWKS key matches ID token kid {kid!r}" + ) from e + except Exception as e: + raise IdTokenVerificationError( + f"Failed to refresh JWKS after kid miss: {e}" + ) from e + + # PyJWT verifies the JWT with the algorithm declared in its header, + # cross-checked against this allowlist (so an attacker can't downgrade + # to ``none`` or HMAC). The allowlist covers the OIDC algorithms + # most cloud IdPs ship by default: + # - RS256: Nextcloud user_oidc, Keycloak default, Auth0, Google. + # - PS256: Azure AD on newer keys. + # - ES256: some Keycloak realms, AWS Cognito user pools. + # Symmetric (HSxxx) and ``none`` are intentionally absent. + payload: dict[str, Any] = jwt.decode( + id_token, + signing_key.key, + algorithms=["RS256", "PS256", "ES256"], + audience=expected_audience, + issuer=issuer, + options={ + "verify_signature": True, + "verify_exp": True, + "verify_iat": True, + "verify_aud": True, + "verify_iss": True, + "require": ["sub", "iss", "aud", "exp", "iat"], + }, + ) + except IdTokenVerificationError: + raise + except jwt.PyJWTError as e: + raise IdTokenVerificationError(f"ID token verification failed: {e}") from e + except Exception as e: + raise IdTokenVerificationError( + f"Unexpected error verifying ID token: {e}" + ) from e + + # Constant-time comparison mirrors the PKCE verifier check + # (oauth_routes.py:1029) — short-circuit `!=` is avoided in + # security-sensitive equality even when the secret is server-generated + # (round-6 review). + if expected_nonce is not None and not secrets.compare_digest( + payload.get("nonce", "") or "", expected_nonce + ): + raise IdTokenVerificationError("ID token nonce does not match request nonce") + + return payload + + +async def extract_user_id_from_token(_ctx: Context) -> str: + """Extract user_id from the verified MCP access token. + + Reads the `sub` claim from `AccessToken.resource`, which is populated by + `UnifiedTokenVerifier` after JWT signature verification (or token + introspection for opaque tokens). We never re-decode the raw token here: + the verifier has already validated the signature and extracted the + identity claim. + + Args: + _ctx: MCP context with access token. Intentionally unused — kept on + the public signature so call sites can pass the FastMCP Context + they already hold without rewriting; identity is read from the + verifier-populated AccessToken via get_access_token(). + + Returns: + 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). """ - # Use MCP SDK's get_access_token() which uses contextvars access_token: AccessToken | None = get_access_token() - if not access_token or not access_token.token: - logger.warning(" ✗ No access token found via get_access_token()") + if not access_token: + logger.warning("No access token found via get_access_token()") return "default_user" - token = access_token.token - is_jwt = "." in token and token.count(".") >= 2 - logger.info(f" Token type: {'JWT' if is_jwt else 'Opaque'}") - - # Try JWT decode first - if is_jwt: - try: - payload = jwt.decode(token, options={"verify_signature": False}) - user_id = payload.get("sub", "unknown") - logger.info(f" ✓ JWT decode successful: user_id={user_id}") - return user_id - except Exception as e: - logger.error(f" ✗ JWT decode failed: {type(e).__name__}: {e}") - - # Opaque token - call userinfo endpoint - logger.info(" Opaque token detected, calling userinfo endpoint...") - try: - # Get userinfo endpoint from OIDC discovery - oidc_discovery_uri = os.getenv( - "OIDC_DISCOVERY_URI", - "http://localhost:8080/.well-known/openid-configuration", + user_id = access_token.resource + if not user_id: + logger.error( + "Access token has no resource (sub) claim — verifier should have rejected it" + ) + raise McpError( + ErrorData( + # JSON-RPC 2.0 reserves -32000..-32099 for application errors. + code=-32001, + message="Cannot determine user identity from access token", + ) ) - async with nextcloud_httpx_client() as http_client: - discovery_response = await http_client.get(oidc_discovery_uri) - discovery_response.raise_for_status() - discovery = discovery_response.json() - userinfo_endpoint = discovery.get("userinfo_endpoint") - if userinfo_endpoint: - userinfo = await _query_idp_userinfo(token, userinfo_endpoint) - if userinfo: - user_id = userinfo.get("sub", "unknown") - logger.info(f" ✓ Userinfo query successful: user_id={user_id}") - return user_id - else: - logger.error(" ✗ Userinfo query failed") - else: - logger.error(" ✗ No userinfo_endpoint available") - except Exception as e: - logger.error(f" ✗ Userinfo query failed: {type(e).__name__}: {e}") - - # Fallback - logger.warning(" Using fallback user_id: default_user") - return "default_user" + return user_id diff --git a/nextcloud_mcp_server/server/oauth_tools.py b/nextcloud_mcp_server/server/oauth_tools.py index 3698aa82..48cfae62 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") @@ -78,10 +77,15 @@ class LoginConfirmation(BaseModel): ) -async def get_provisioning_status(ctx: Context, user_id: str) -> ProvisioningStatus: +async def _get_provisioning_status(ctx: Context, user_id: str) -> ProvisioningStatus: """ Check the provisioning status for Nextcloud access. + Internal helper — leading underscore signals that ``user_id`` is a + trusted identity claim that callers MUST derive from the verified + access token. The MCP tool wrappers in ``register_oauth_tools`` are + the only legitimate callers (PR #758 round-3 finding 3). + Checks for both credential types: 1. App password from Astrolabe (works today) 2. OAuth refresh token from storage (for future) @@ -106,8 +110,12 @@ async def get_provisioning_status(ctx: Context, user_id: str) -> ProvisioningSta status = await astrolabe.get_background_sync_status(user_id) if status.get("has_access"): - logger.info( - f" get_provisioning_status: ✓ App password FOUND for user_id={user_id}" + # Demoted to debug (PR #758 round-2 nit 4): user_id ends up + # in log aggregation on every call, which is noise in a + # multi-tenant deployment. + logger.debug( + " get_provisioning_status: app password FOUND for user_id=%s", + user_id, ) provisioned_at_str = status.get("provisioned_at") return ProvisioningStatus( @@ -116,29 +124,28 @@ async def get_provisioning_status(ctx: Context, user_id: str) -> ProvisioningSta credential_type="app_password", ) except Exception as e: - logger.debug(f" App password check failed for {user_id}: {e}") + logger.debug(" App password check failed for %s: %s", user_id, e) # Check for OAuth refresh token (fallback) - logger.info( - f" get_provisioning_status: Looking up refresh token for user_id={user_id}" + logger.debug( + " get_provisioning_status: looking up refresh token for user_id=%s", user_id ) - storage = RefreshTokenStorage.from_env() - await storage.initialize() + storage = await get_shared_storage() token_data = await storage.get_refresh_token(user_id) if not token_data: - logger.info( - f" get_provisioning_status: ✗ No credentials found for user_id={user_id}" + logger.debug( + " get_provisioning_status: no credentials found for user_id=%s", user_id ) return ProvisioningStatus(is_provisioned=False) - logger.info( - f" get_provisioning_status: ✓ Refresh token FOUND for user_id={user_id}" - ) - logger.info(f" flow_type: {token_data.get('flow_type')}") - logger.info( - f" provisioning_client_id: {token_data.get('provisioning_client_id', 'N/A')}" + logger.debug( + " get_provisioning_status: refresh token FOUND for user_id=%s " + "flow_type=%s provisioning_client_id=%s", + user_id, + token_data.get("flow_type"), + token_data.get("provisioning_client_id", "N/A"), ) # Convert timestamp to ISO format if present @@ -198,11 +205,9 @@ def generate_oauth_url_for_flow2( return f"{auth_endpoint}?{urlencode(params)}" -async def provision_nextcloud_access( - ctx: Context, user_id: Optional[str] = None -) -> ProvisioningResult: +async def _provision_nextcloud_access(ctx: Context, user_id: str) -> ProvisioningResult: """ - MCP Tool: Provision offline access to Nextcloud resources. + Internal helper for the ``provision_nextcloud_access`` MCP tool. Returns URL to Astrolabe settings page where users can provision background sync access using either: @@ -211,18 +216,15 @@ async def provision_nextcloud_access( Args: ctx: MCP context with user's Flow 1 token - user_id: Optional user identifier (extracted from token if not provided) + user_id: Authenticated user identifier (must be derived from the + verified access token by the caller; never accept from MCP input). Returns: ProvisioningResult with Astrolabe settings URL or status """ try: - # Extract user ID from the MCP access token (Flow 1 token) - if not user_id: - user_id = await extract_user_id_from_token(ctx) - # Check if already provisioned - status = await get_provisioning_status(ctx, user_id) + status = await _get_provisioning_status(ctx, user_id) if status.is_provisioned: return ProvisioningResult( success=True, @@ -271,31 +273,24 @@ async def provision_nextcloud_access( ) -async def revoke_nextcloud_access( - ctx: Context, user_id: Optional[str] = None -) -> RevocationResult: +async def _revoke_nextcloud_access(ctx: Context, user_id: str) -> RevocationResult: """ - MCP Tool: Revoke offline access to Nextcloud resources. + Internal helper for the ``revoke_nextcloud_access`` MCP tool. This tool removes the stored refresh token and revokes access that was granted via Flow 2. Args: - mcp: MCP context - user_id: Optional user identifier + ctx: MCP context + user_id: Authenticated user identifier (must be derived from the + verified access token by the caller; never accept from MCP input). Returns: RevocationResult with status """ try: - # Get user ID from token if not provided - if not user_id: - logger.info("Extracting user_id from access token for revoke...") - user_id = await extract_user_id_from_token(ctx) - logger.info(f" Revoke using user_id: {user_id}") - # Check current status - status = await get_provisioning_status(ctx, user_id) + status = await _get_provisioning_status(ctx, user_id) if not status.is_provisioned: return RevocationResult( success=True, @@ -303,8 +298,7 @@ async def revoke_nextcloud_access( ) # 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() @@ -350,36 +344,27 @@ async def revoke_nextcloud_access( ) -async def check_provisioning_status( - ctx: Context, user_id: Optional[str] = None -) -> ProvisioningStatus: +async def _check_provisioning_status(ctx: Context, user_id: str) -> ProvisioningStatus: """ - MCP Tool: Check the current provisioning status. + Internal helper for the ``check_provisioning_status`` MCP tool. This tool allows users to check whether they have provisioned Nextcloud access and see details about their current authorization. Args: - mcp: MCP context - user_id: Optional user identifier + ctx: MCP context + user_id: Authenticated user identifier (must be derived from the + verified access token by the caller; never accept from MCP input). Returns: ProvisioningStatus with current state """ - # Get user ID from context if not provided - if not user_id: - user_id = ( - ctx.context.get("user_id", "default_user") # type: ignore - if hasattr(ctx, "context") - else "default_user" - ) - - return await get_provisioning_status(ctx, user_id) + return await _get_provisioning_status(ctx, user_id) -async def check_logged_in(ctx: Context, user_id: Optional[str] = None) -> str: +async def _check_logged_in(ctx: Context, user_id: str) -> str: """ - MCP Tool: Check if user is logged in and elicit login if needed. + Internal helper for the ``check_logged_in`` MCP tool. This tool checks whether the user has completed Flow 2 (resource provisioning) to grant offline access to Nextcloud. If not logged in, it uses MCP elicitation @@ -387,35 +372,29 @@ async def check_logged_in(ctx: Context, user_id: Optional[str] = None) -> str: Args: ctx: MCP context with user's Flow 1 token - user_id: Optional user identifier (extracted from token if not provided) + user_id: Authenticated user identifier (must be derived from the + verified access token by the caller; never accept from MCP input). Returns: "yes" if logged in, or elicitation prompting for login """ try: - # Extract user ID from the MCP access token (Flow 1 token) - logger.info("=" * 60) - logger.info("check_logged_in: Starting user_id extraction") - logger.info("=" * 60) - - if not user_id: - user_id = await extract_user_id_from_token(ctx) - logger.info(f" Final user_id for check_logged_in: {user_id}") - else: - logger.info(f" user_id provided as argument: {user_id}") - - # Check if already logged in - logger.info(f"Checking provisioning status for user_id: {user_id}") - status = await get_provisioning_status(ctx, user_id) - logger.info(f" Provisioning status: is_provisioned={status.is_provisioned}") + # Demoted to debug (PR #758 round-2 nit 4): per-user logging at INFO + # ends up in log aggregation on every check_logged_in call, which is + # noise in a hosted multi-tenant deployment. + logger.debug("Checking provisioning status for user_id=%s", user_id) + status = await _get_provisioning_status(ctx, user_id) + logger.debug( + " Provisioning status for %s: is_provisioned=%s", + user_id, + status.is_provisioned, + ) if status.is_provisioned: - logger.info(f"✓ User {user_id} is already logged in - returning 'yes'") - logger.info("=" * 60) + logger.debug("User %s already logged in", user_id) return "yes" - logger.info(f"✗ User {user_id} is NOT logged in - triggering elicitation") - logger.info("=" * 60) + logger.debug("User %s NOT logged in — triggering elicitation", user_id) # Not logged in - generate OAuth URL for Flow 2 # Use settings (handles both ENABLE_BACKGROUND_OPERATIONS and ENABLE_OFFLINE_ACCESS) @@ -451,22 +430,14 @@ async def check_logged_in(ctx: Context, user_id: Optional[str] = None) -> 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)}" + # The canonical Flow 2 oauth_session row is written inside + # generate_oauth_url_for_flow2 (keyed by `state`, with the PKCE + # verifier and nonce); the unified callback looks it up by `state`. + # No additional row is needed here. redirect_uri = f"{os.getenv('NEXTCLOUD_MCP_SERVER_URL', 'http://localhost:8000')}/oauth/callback" - await storage.store_oauth_session( - session_id=session_id, - client_redirect_uri="", # No client redirect for Flow 2 - state=state, - flow_type="flow2", - is_provisioning=True, - ttl_seconds=600, # 10 minute TTL - ) - # Define scopes for Nextcloud access # Note: offline_access is only included when enabled in settings. # The actual scope sent to the IdP is determined by @@ -497,8 +468,11 @@ async def check_logged_in(ctx: Context, user_id: Optional[str] = None) -> str: scopes=scopes, ) - # Use elicitation to prompt user to login - logger.info(f"Eliciting login for user {user_id} with URL: {auth_url}") + # Use elicitation to prompt user to login. Logged at debug (PR #758 + # round-2 nit 4): the auth URL contains the per-request ``state`` + # token, which is sensitive enough that it shouldn't land in + # multi-tenant log aggregation by default. + logger.debug("Eliciting login for user %s (URL omitted)", user_id) result = await ctx.elicit( message=f"Please log in to Nextcloud at the following URL:\n\n{auth_url}\n\nAfter completing the login, check the box below and click OK.", @@ -507,10 +481,15 @@ async def check_logged_in(ctx: Context, user_id: Optional[str] = None) -> str: if result.action == "accept": # Check if login was successful by looking for refresh token - # Strategy: Try multiple lookup methods to handle both flows - logger.info("User accepted login prompt, checking for refresh token") - logger.info(f" State parameter: {state[:16]}...") - logger.info(f" User ID: {user_id}") + # Strategy: Try multiple lookup methods to handle both flows. + # Demoted to debug (PR #758 round-2 nit 4): user_id + state + # appear here on every elicitation accept. + logger.debug( + "User accepted login prompt; looking up refresh token " + "(user_id=%s state=%s...)", + user_id, + state[:16], + ) # First, try to find token by provisioning_client_id (Flow 2 from elicitation) refresh_token_data = ( @@ -518,45 +497,39 @@ async def check_logged_in(ctx: Context, user_id: Optional[str] = None) -> str: ) if refresh_token_data: - logger.info("✓ Refresh token found via provisioning_client_id lookup") - logger.info( - f" Flow type: {refresh_token_data.get('flow_type', 'unknown')}" - ) - logger.info( - f" Provisioned at: {refresh_token_data.get('provisioned_at', 'unknown')}" + logger.debug( + "Refresh token found via provisioning_client_id lookup " + "(flow_type=%s provisioned_at=%s)", + refresh_token_data.get("flow_type", "unknown"), + refresh_token_data.get("provisioned_at", "unknown"), ) return "yes" # Fallback: Try to find token by user_id (browser login or any other flow) - logger.info(f"✗ No token found with provisioning_client_id={state[:16]}...") - logger.info(f" Trying fallback lookup by user_id: {user_id}") + logger.debug( + "No token via provisioning_client_id=%s...; falling back to user_id=%s", + state[:16], + user_id, + ) refresh_token_data = await storage.get_refresh_token(user_id) if refresh_token_data: - logger.info("✓ Refresh token found via user_id lookup") - logger.info( - f" Flow type: {refresh_token_data.get('flow_type', 'unknown')}" - ) - logger.info( - f" Provisioned at: {refresh_token_data.get('provisioned_at', 'unknown')}" - ) - logger.info( - f" Provisioning client ID: {refresh_token_data.get('provisioning_client_id', 'NULL')}" - ) - logger.info( - " Note: This token was created via browser login or different flow" + logger.debug( + "Refresh token found via user_id lookup " + "(flow_type=%s provisioned_at=%s provisioning_client_id=%s)", + refresh_token_data.get("flow_type", "unknown"), + refresh_token_data.get("provisioned_at", "unknown"), + refresh_token_data.get("provisioning_client_id", "NULL"), ) return "yes" # No token found by either method - logger.warning(f"✗ No refresh token found for user {user_id}") logger.warning( - f" Checked provisioning_client_id={state[:16]}... - NOT FOUND" - ) - logger.warning(f" Checked user_id={user_id} - NOT FOUND") - logger.warning( - " This may indicate the user completed login but token wasn't stored" + "No refresh token found for user_id=%s (checked provisioning_client_id=%s... and user_id) — " + "user completed elicitation but token wasn't stored", + user_id, + state[:16], ) return ( @@ -591,11 +564,9 @@ def register_oauth_tools(mcp): ), ) @require_scopes("openid") - async def tool_provision_access( - ctx: Context, - user_id: Optional[str] = None, - ) -> ProvisioningResult: - return await provision_nextcloud_access(ctx, user_id) + async def tool_provision_access(ctx: Context) -> ProvisioningResult: + user_id = await extract_user_id_from_token(ctx) + return await _provision_nextcloud_access(ctx, user_id) @mcp.tool( name="revoke_nextcloud_access", @@ -608,10 +579,9 @@ def register_oauth_tools(mcp): ), ) @require_scopes("openid") - async def tool_revoke_access( - ctx: Context, user_id: Optional[str] = None - ) -> RevocationResult: - return await revoke_nextcloud_access(ctx, user_id) + async def tool_revoke_access(ctx: Context) -> RevocationResult: + user_id = await extract_user_id_from_token(ctx) + return await _revoke_nextcloud_access(ctx, user_id) @mcp.tool( name="check_provisioning_status", @@ -623,10 +593,9 @@ def register_oauth_tools(mcp): ), ) @require_scopes("openid") - async def tool_check_status( - ctx: Context, user_id: Optional[str] = None - ) -> ProvisioningStatus: - return await check_provisioning_status(ctx, user_id) + async def tool_check_status(ctx: Context) -> ProvisioningStatus: + user_id = await extract_user_id_from_token(ctx) + return await _check_provisioning_status(ctx, user_id) @mcp.tool( name="check_logged_in", @@ -641,5 +610,6 @@ def register_oauth_tools(mcp): ), ) @require_scopes("openid") - async def tool_check_logged_in(ctx: Context, user_id: Optional[str] = None) -> str: - return await check_logged_in(ctx, user_id) + async def tool_check_logged_in(ctx: Context) -> str: + user_id = await extract_user_id_from_token(ctx) + return await _check_logged_in(ctx, user_id) diff --git a/tests/unit/test_browser_oauth_routes.py b/tests/unit/test_browser_oauth_routes.py new file mode 100644 index 00000000..6f3ae7f5 --- /dev/null +++ b/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 diff --git a/tests/unit/test_browser_oauth_xss.py b/tests/unit/test_browser_oauth_xss.py new file mode 100644 index 00000000..8411eb36 --- /dev/null +++ b/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 = "" + + +@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 "<script>alert(1)</script>" 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 "<script>alert(1)</script>" not in body + assert "An internal error occurred" in body + assert "Correlation ID" in body diff --git a/tests/unit/test_browser_sessions.py b/tests/unit/test_browser_sessions.py new file mode 100644 index 00000000..65ac5db8 --- /dev/null +++ b/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 diff --git a/tests/unit/test_dcr_proxy.py b/tests/unit/test_dcr_proxy.py index 07eed461..621ef299 100644 --- a/tests/unit/test_dcr_proxy.py +++ b/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, ): diff --git a/tests/unit/test_id_token_verification.py b/tests/unit/test_id_token_verification.py new file mode 100644 index 00000000..c17159b8 --- /dev/null +++ b/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)}" + ) 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..7b666a2c --- /dev/null +++ b/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" + ) diff --git a/tests/unit/test_oauth_logout.py b/tests/unit/test_oauth_logout.py new file mode 100644 index 00000000..c27960f6 --- /dev/null +++ b/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" diff --git a/tests/unit/test_oauth_tools_signatures.py b/tests/unit/test_oauth_tools_signatures.py new file mode 100644 index 00000000..cbeba12b --- /dev/null +++ b/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 diff --git a/tests/unit/test_oidc_discovery.py b/tests/unit/test_oidc_discovery.py index c2459f25..0212e318 100644 --- a/tests/unit/test_oidc_discovery.py +++ b/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() diff --git a/tests/unit/test_origin_normalise.py b/tests/unit/test_origin_normalise.py new file mode 100644 index 00000000..e7a5037d --- /dev/null +++ b/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 diff --git a/tests/unit/test_safe_next_url.py b/tests/unit/test_safe_next_url.py new file mode 100644 index 00000000..056054e3 --- /dev/null +++ b/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 diff --git a/tests/unit/test_token_broker.py b/tests/unit/test_token_broker.py index 32105b2e..8aebcd1a 100644 --- a/tests/unit/test_token_broker.py +++ b/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 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())