From 27fcf05d3a9e1a1fe49ebcff927b8353678792bd Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sun, 3 May 2026 13:36:36 +0200 Subject: [PATCH] fix(auth): address PR #758 round-7 important review - Coerce refresh_expires_in to int before arithmetic in both callback paths so IdPs that serialize the field as a JSON string (e.g. AWS Cognito) don't trigger an unhandled TypeError 500. - Drop the orphaned oauth_session row written by _check_logged_in. The canonical Flow 2 row is created by generate_oauth_url_for_flow2 keyed by `state`, which is what the unified callback looks up; the flow2_ session_id was never matched and just churned the table for 10 minutes per call. - Match delete_cookie attributes (httponly, secure, samesite) to the set_cookie call on logout so browsers reliably evict the cookie even on implementations that consider security flags during deletion. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../auth/browser_oauth_routes.py | 14 ++++++++++++-- nextcloud_mcp_server/auth/oauth_routes.py | 4 +++- nextcloud_mcp_server/server/oauth_tools.py | 17 ++++------------- 3 files changed, 19 insertions(+), 16 deletions(-) diff --git a/nextcloud_mcp_server/auth/browser_oauth_routes.py b/nextcloud_mcp_server/auth/browser_oauth_routes.py index 7e3d78fa..c155c168 100644 --- a/nextcloud_mcp_server/auth/browser_oauth_routes.py +++ b/nextcloud_mcp_server/auth/browser_oauth_routes.py @@ -554,7 +554,9 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo 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 + # 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, @@ -714,7 +716,15 @@ async def oauth_logout(request: Request) -> RedirectResponse | JSONResponse: ) 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 diff --git a/nextcloud_mcp_server/auth/oauth_routes.py b/nextcloud_mcp_server/auth/oauth_routes.py index 7dbb56ef..919b04d8 100644 --- a/nextcloud_mcp_server/auth/oauth_routes.py +++ b/nextcloud_mcp_server/auth/oauth_routes.py @@ -699,7 +699,9 @@ 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 + # 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.info(" refresh_expires_in: %ss", refresh_expires_in) logger.info(" refresh_expires_at: %s", refresh_expires_at) diff --git a/nextcloud_mcp_server/server/oauth_tools.py b/nextcloud_mcp_server/server/oauth_tools.py index cb143b94..48cfae62 100644 --- a/nextcloud_mcp_server/server/oauth_tools.py +++ b/nextcloud_mcp_server/server/oauth_tools.py @@ -432,21 +432,12 @@ async def _check_logged_in(ctx: Context, user_id: str) -> str: # Store state in session for validation on callback storage = await get_shared_storage() - # Create OAuth session for Flow 2. Identifier is purely random - # so audit-log entries / DB rows don't carry the user_id in the - # session_id field (PR #758 round-4 review medium 3). - session_id = f"flow2_{secrets.token_hex(16)}" + # 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