diff --git a/nextcloud_mcp_server/auth/browser_oauth_routes.py b/nextcloud_mcp_server/auth/browser_oauth_routes.py index 58bb3d95..18dc12b5 100644 --- a/nextcloud_mcp_server/auth/browser_oauth_routes.py +++ b/nextcloud_mcp_server/auth/browser_oauth_routes.py @@ -66,16 +66,16 @@ def _origin_matches_self(request: Request, oauth_ctx: dict) -> bool: cfg = oauth_ctx.get("config") or oauth_ctx mcp_server_url = cfg.get("mcp_server_url") if not mcp_server_url: - # Mis-configured deployment — fail open rather than break logout, but - # log loudly so the operator can see this is happening (PR #758 - # finding 3). Other OAuth code paths require ``mcp_server_url`` and - # KeyError if it's absent, so this branch should never fire in a - # correctly configured deployment. - logger.warning( - "CSRF check bypassed on /oauth/logout: mcp_server_url not " + # 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 True + return False expected = _normalise_origin(mcp_server_url) raw = request.headers.get("origin") or request.headers.get("referer") @@ -434,14 +434,19 @@ 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( - "Token exchange failed: HTTP %s - %s", e.response.status_code, error_body + "Token exchange failed (correlation_id=%s): HTTP %s - %s", + correlation_id, + e.response.status_code, + error_body, ) - # html_escape: error_body originates from the IdP and could contain - # markup that would be reflected into the failure page otherwise. return HTMLResponse( f""" @@ -449,15 +454,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}: {html_escape(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("Token exchange failed: %s", e) + correlation_id = secrets.token_hex(8) + logger.error("Token exchange failed (correlation_id=%s): %s", correlation_id, e) return HTMLResponse( f""" @@ -465,8 +472,9 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo Login Failed

Login Failed

-

Failed to exchange authorization code for tokens

-

Error: {html_escape(str(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.

""", @@ -508,13 +516,19 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo expected_nonce=nonce, ) except IdTokenVerificationError as e: - logger.error("ID token verification failed: %s", e) - # html_escape: defense-in-depth. The exception text is currently - # server-constructed, but escape on the success path too so any - # future error wrapping that includes IdP response text can't - # smuggle markup into the login-failure page. + # 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

ID token failed verification: {html_escape(str(e))}

", + f"

Login Failed

" + f"

The ID token failed verification.

" + f"

Correlation ID: {html_escape(correlation_id)}

", status_code=400, ) diff --git a/nextcloud_mcp_server/auth/oauth_routes.py b/nextcloud_mcp_server/auth/oauth_routes.py index db55cb3f..72a62cc5 100644 --- a/nextcloud_mcp_server/auth/oauth_routes.py +++ b/nextcloud_mcp_server/auth/oauth_routes.py @@ -465,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, @@ -475,6 +480,7 @@ async def oauth_authorize_nextcloud( code_challenge=code_challenge, code_challenge_method="S256", mcp_authorization_code=code_verifier, # Store code_verifier here temporarily + nonce=nonce, flow_type="flow2", ttl_seconds=600, # 10 minutes ) @@ -512,6 +518,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 @@ -572,12 +579,15 @@ 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) + # Retrieve code_verifier + nonce from session storage (PKCE + OIDC + # nonce binding both required for Flow 2 — round-3 finding 1). code_verifier = "" + nonce: str | None = None oauth_session = await storage.get_oauth_session(state) if oauth_session: # code_verifier was stored in mcp_authorization_code field code_verifier = oauth_session.get("mcp_authorization_code", "") + nonce = oauth_session.get("nonce") logger.info( f"Retrieved code_verifier for Flow 2 callback (state={state[:16]}...)" ) @@ -636,12 +646,16 @@ async def oauth_callback_nextcloud(request: Request): id_token = token_data.get("id_token") # 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); falsy → skip nonce + # check for sessions written before the column existed. logger.info("oauth_callback_nextcloud: Verifying ID token") try: userinfo = await verify_id_token( id_token, discovery_url=discovery_url, expected_audience=mcp_server_client_id, + expected_nonce=nonce or None, ) except IdTokenVerificationError as e: logger.error("ID token verification failed: %s", e) diff --git a/nextcloud_mcp_server/auth/storage.py b/nextcloud_mcp_server/auth/storage.py index 691c1cc2..517538a3 100644 --- a/nextcloud_mcp_server/auth/storage.py +++ b/nextcloud_mcp_server/auth/storage.py @@ -1178,6 +1178,16 @@ class RefreshTokenStorage: 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. @@ -1210,7 +1220,16 @@ class RefreshTokenStorage: if not self._initialized: await self.initialize() + # SELECT the row before DELETE so we can attribute the audit log + # entry to the right user (PR #758 round-3 nit 5). async with aiosqlite.connect(self.db_path) as db: + async with db.execute( + "SELECT user_id FROM browser_sessions WHERE session_id = ?", + (session_id,), + ) as cursor: + row = await cursor.fetchone() + user_id = row[0] if row else None + cursor = await db.execute( "DELETE FROM browser_sessions WHERE session_id = ?", (session_id,) ) @@ -1219,6 +1238,13 @@ class RefreshTokenStorage: if deleted: 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: diff --git a/nextcloud_mcp_server/server/oauth_tools.py b/nextcloud_mcp_server/server/oauth_tools.py index ea7ee0b0..f5057736 100644 --- a/nextcloud_mcp_server/server/oauth_tools.py +++ b/nextcloud_mcp_server/server/oauth_tools.py @@ -77,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) @@ -200,9 +205,9 @@ def generate_oauth_url_for_flow2( return f"{auth_endpoint}?{urlencode(params)}" -async def provision_nextcloud_access(ctx: Context, user_id: str) -> 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: @@ -219,7 +224,7 @@ async def provision_nextcloud_access(ctx: Context, user_id: str) -> Provisioning """ try: # 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, @@ -268,9 +273,9 @@ async def provision_nextcloud_access(ctx: Context, user_id: str) -> Provisioning ) -async def revoke_nextcloud_access(ctx: Context, user_id: str) -> 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. @@ -285,7 +290,7 @@ async def revoke_nextcloud_access(ctx: Context, user_id: str) -> RevocationResul """ try: # 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, @@ -339,9 +344,9 @@ async def revoke_nextcloud_access(ctx: Context, user_id: str) -> RevocationResul ) -async def check_provisioning_status(ctx: Context, user_id: str) -> 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. @@ -354,12 +359,12 @@ async def check_provisioning_status(ctx: Context, user_id: str) -> ProvisioningS Returns: ProvisioningStatus with current state """ - 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: str) -> 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 @@ -378,7 +383,7 @@ async def check_logged_in(ctx: Context, user_id: str) -> str: # 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) + status = await _get_provisioning_status(ctx, user_id) logger.debug( " Provisioning status for %s: is_provisioned=%s", user_id, @@ -568,7 +573,7 @@ def register_oauth_tools(mcp): @require_scopes("openid") 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) + return await _provision_nextcloud_access(ctx, user_id) @mcp.tool( name="revoke_nextcloud_access", @@ -583,7 +588,7 @@ def register_oauth_tools(mcp): @require_scopes("openid") 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) + return await _revoke_nextcloud_access(ctx, user_id) @mcp.tool( name="check_provisioning_status", @@ -597,7 +602,7 @@ def register_oauth_tools(mcp): @require_scopes("openid") 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) + return await _check_provisioning_status(ctx, user_id) @mcp.tool( name="check_logged_in", @@ -614,4 +619,4 @@ def register_oauth_tools(mcp): @require_scopes("openid") 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) + return await _check_logged_in(ctx, user_id) diff --git a/tests/unit/test_browser_oauth_xss.py b/tests/unit/test_browser_oauth_xss.py index 8b82b483..8411eb36 100644 --- a/tests/unit/test_browser_oauth_xss.py +++ b/tests/unit/test_browser_oauth_xss.py @@ -70,8 +70,14 @@ async def test_callback_escapes_error_query_params(storage): assert "<script>alert(1)</script>" in body -async def test_callback_escapes_idp_http_error_body(storage): - """IdP-returned HTTPError body must be HTML-escaped before reflection.""" +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: @@ -135,5 +141,10 @@ async def test_callback_escapes_idp_http_error_body(storage): 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>" 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_oauth_logout.py b/tests/unit/test_oauth_logout.py index 095f81c1..0aad07e0 100644 --- a/tests/unit/test_oauth_logout.py +++ b/tests/unit/test_oauth_logout.py @@ -92,7 +92,13 @@ async def test_logout_deletes_refresh_token_and_session(storage): request = _build_request( cookie="sid-1", - oauth_context={"storage": storage, "config": {"discovery_url": None}}, + oauth_context={ + "storage": storage, + "config": { + "mcp_server_url": "https://mcp.example.com", + "discovery_url": None, + }, + }, ) with patch( @@ -118,7 +124,10 @@ async def test_logout_calls_revocation_when_refresh_token_present(storage): cookie="sid-2", oauth_context={ "storage": storage, - "config": {"discovery_url": "http://idp/.well-known"}, + "config": { + "mcp_server_url": "https://mcp.example.com", + "discovery_url": "http://idp/.well-known", + }, }, ) @@ -138,7 +147,13 @@ 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": {"discovery_url": 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 @@ -157,7 +172,10 @@ async def test_logout_swallows_storage_errors(storage): cookie="sid-3", oauth_context={ "storage": broken_storage, - "config": {"discovery_url": None}, + "config": { + "mcp_server_url": "https://mcp.example.com", + "discovery_url": None, + }, }, ) response = await oauth_logout(request) @@ -295,6 +313,26 @@ async def test_logout_allows_referer_when_origin_missing(storage): 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") @@ -302,7 +340,13 @@ async def test_logout_handles_session_with_no_refresh_token(storage): revoke = AsyncMock() request = _build_request( cookie="sid-4", - oauth_context={"storage": storage, "config": {"discovery_url": None}}, + 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",