From c39fffedf78c813d12ad096d8859cecc7a727981 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Fri, 3 Apr 2026 19:35:14 -0400 Subject: [PATCH] fix: restore early oauthError/code redirects, gate only failFlow behind CSRF The previous restructuring moved oauthError and missing-code checks behind CSRF validation, breaking tests that expect those redirects without cookies. The redirect itself is harmless (just shows an error page). Only the failFlow call needs CSRF gating to prevent DoS. Restructure: oauthError check stays early (redirects immediately), but failFlow inside it runs the full CSRF/session/active-flow validation before marking the flow as FAILED. --- api/server/routes/mcp.js | 70 ++++++++++++++++++++++++---------------- 1 file changed, 43 insertions(+), 27 deletions(-) diff --git a/api/server/routes/mcp.js b/api/server/routes/mcp.js index 277053110c..d43b3c7adf 100644 --- a/api/server/routes/mcp.js +++ b/api/server/routes/mcp.js @@ -147,13 +147,50 @@ router.get('/:serverName/oauth/callback', async (req, res) => { error: oauthError, }); - if (!state || typeof state !== 'string') { - if (oauthError) { - logger.error('[MCP OAuth] OAuth error received without state', { error: oauthError }); - return res.redirect( - `${basePath}/oauth/error?error=${encodeURIComponent(String(oauthError))}`, - ); + if (oauthError) { + logger.error('[MCP OAuth] OAuth error received', { error: oauthError }); + // Gate failFlow behind callback validation to prevent DoS via leaked state + if (state && typeof state === 'string') { + try { + const flowsCache = getLogStores(CacheKeys.FLOWS); + const flowManager = getFlowStateManager(flowsCache); + const flowId = await MCPOAuthHandler.resolveStateToFlowId(state, flowManager); + if (flowId) { + const flowParts = flowId.split(':'); + const [flowUserId] = flowParts; + const hasCsrf = validateOAuthCsrf(req, res, flowId, OAUTH_CSRF_COOKIE_PATH); + const hasSession = !hasCsrf && validateOAuthSession(req, flowUserId); + let hasActiveFlow = false; + if (!hasCsrf && !hasSession) { + const pendingFlow = await flowManager.getFlowState(flowId, 'mcp_oauth'); + const pendingAge = pendingFlow?.createdAt + ? Date.now() - pendingFlow.createdAt + : Infinity; + hasActiveFlow = pendingFlow?.status === 'PENDING' && pendingAge < PENDING_STALE_MS; + } + if (hasCsrf || hasSession || hasActiveFlow) { + await flowManager.failFlow(flowId, 'mcp_oauth', String(oauthError)); + logger.debug('[MCP OAuth] Marked flow as FAILED with OAuth error', { + flowId, + error: oauthError, + }); + } + } + } catch (err) { + logger.debug('[MCP OAuth] Could not mark flow as failed', err); + } } + return res.redirect( + `${basePath}/oauth/error?error=${encodeURIComponent(String(oauthError))}`, + ); + } + + if (!code || typeof code !== 'string') { + logger.error('[MCP OAuth] Missing or invalid code'); + return res.redirect(`${basePath}/oauth/error?error=missing_code`); + } + + if (!state || typeof state !== 'string') { logger.error('[MCP OAuth] Missing or invalid state'); return res.redirect(`${basePath}/oauth/error?error=missing_state`); } @@ -205,27 +242,6 @@ router.get('/:serverName/oauth/callback', async (req, res) => { return res.redirect(`${basePath}/oauth/error?error=csrf_validation_failed`); } - if (oauthError) { - logger.error('[MCP OAuth] OAuth error received', { error: oauthError, flowId }); - try { - await flowManager.failFlow(flowId, 'mcp_oauth', String(oauthError)); - logger.debug('[MCP OAuth] Marked flow as FAILED with OAuth error', { - flowId, - error: oauthError, - }); - } catch (err) { - logger.debug('[MCP OAuth] Could not mark flow as failed', err); - } - return res.redirect( - `${basePath}/oauth/error?error=${encodeURIComponent(String(oauthError))}`, - ); - } - - if (!code || typeof code !== 'string') { - logger.error('[MCP OAuth] Missing or invalid code'); - return res.redirect(`${basePath}/oauth/error?error=missing_code`); - } - logger.debug('[MCP OAuth] Getting flow state for flowId: ' + flowId); const flowState = await MCPOAuthHandler.getFlowState(flowId, flowManager);