From 3c447b42a7f019f8fa24cf4b1c599929fc9abb85 Mon Sep 17 00:00:00 2001 From: Ricardo Espinoza Date: Thu, 15 Oct 2026 11:43:45 +0000 Subject: [PATCH] fix(core): Key each OAuth authorization request to its own session (#39930) Co-authored-by: Claude Opus 5 --- ...chat-trigger-resource.resolver.api.test.ts | 24 +-- .../oauth-consent.controller.api.test.ts | 109 ++++++------ .../__tests__/oauth-flow-test-utils.ts | 28 +++ .../__tests__/oauth-server.service.test.ts | 19 +- .../__tests__/oauth-session.service.test.ts | 168 +++++++++++++++++- .../__tests__/oauth.controller.api.test.ts | 166 ++++++++++++++++- .../__tests__/oauth.helpers.test.ts | 34 ++++ ...flow-mcp-trigger-authorization.api.test.ts | 13 +- .../dto/approve-consent-request.dto.ts | 2 + .../oauth-server/oauth-consent.controller.ts | 45 +++-- .../oauth-server/oauth-server.service.ts | 12 +- .../oauth-server/oauth-session.service.ts | 106 +++++++++-- .../modules/oauth-server/oauth.controller.ts | 36 ++++ .../src/modules/oauth-server/oauth.helpers.ts | 20 +++ .../webhook-request-sanitizer.test.ts | 34 +++- .../src/webhooks/webhook-request-sanitizer.ts | 14 +- .../public-api/oauth-consent-scopes.test.ts | 8 +- .../@n8n/rest-api-client/src/api/consent.ts | 15 +- .../src/app/stores/consent.store.test.ts | 16 +- .../editor-ui/src/app/stores/consent.store.ts | 13 +- .../src/app/views/OAuthConsentView.test.ts | 23 ++- .../src/app/views/OAuthConsentView.vue | 40 ++++- .../services/mcp-oauth-api-helper.ts | 44 +++-- .../tests/e2e/mcp/mcp-oauth.spec.ts | 3 +- 24 files changed, 823 insertions(+), 169 deletions(-) create mode 100644 packages/cli/src/modules/oauth-server/__tests__/oauth-flow-test-utils.ts diff --git a/packages/cli/src/modules/oauth-server/__tests__/chat-trigger-resource.resolver.api.test.ts b/packages/cli/src/modules/oauth-server/__tests__/chat-trigger-resource.resolver.api.test.ts index 7c503ac2f70..ff2487263c6 100644 --- a/packages/cli/src/modules/oauth-server/__tests__/chat-trigger-resource.resolver.api.test.ts +++ b/packages/cli/src/modules/oauth-server/__tests__/chat-trigger-resource.resolver.api.test.ts @@ -16,6 +16,8 @@ import request from 'supertest'; import { createMember, createOwner } from '@test-integration/db/users'; import { setupTestServer } from '@test-integration/utils'; +import { pendingFlowFrom } from './oauth-flow-test-utils'; + import { AuthService } from '@/auth/auth.service'; import { AUTH_COOKIE_NAME } from '@/constants'; import { OAuthTokenService } from '@/modules/oauth-server/oauth-token.service'; @@ -451,13 +453,9 @@ describe('consent reuse on a second visit', () => { .get('/oauth/authorize') .query(authorizeQuery(resourceUrl, cc1, 'state-1')); expect(first.statusCode).toBe(302); - expect(first.headers.location).toBe('/oauth/consent'); + expect(first.headers.location).toContain('/oauth/consent'); - const rawSetCookie: string | string[] = first.headers['set-cookie'] ?? []; - const setCookies = Array.isArray(rawSetCookie) ? rawSetCookie : [rawSetCookie]; - const sessionCookie = setCookies - .map((cookie) => cookie.split(';')[0]) - .find((cookie) => cookie.startsWith('n8n-oauth-session=')); + const { flowId, cookie: sessionCookie } = pendingFlowFrom(first); expect(sessionCookie).toBeDefined(); // `/consent/approve` lives under the `/rest` prefix, unlike the root-level @@ -468,7 +466,7 @@ describe('consent reuse on a second visit', () => { const approve = await consentAgent .post('/consent/approve') - .send({ approved: true, scopes: [] }); + .send({ approved: true, scopes: [], flow: flowId }); expect(approve.statusCode).toBe(200); const { codeChallenge: cc2 } = await pkce(); @@ -477,7 +475,7 @@ describe('consent reuse on a second visit', () => { .query(authorizeQuery(resourceUrl, cc2, 'state-2')); expect(second.statusCode).toBe(302); - expect(second.headers.location).not.toBe('/oauth/consent'); + expect(second.headers.location).not.toContain('/oauth/consent'); expect(second.headers.location).toContain(resourceUrl); }); @@ -515,13 +513,9 @@ describe('consent reuse on a second visit', () => { .get('/oauth/authorize') .query(authorizeQuery(resourceUrl, codeChallenge, 'state-1')); expect(first.statusCode).toBe(302); - expect(first.headers.location).toBe('/oauth/consent'); + expect(first.headers.location).toContain('/oauth/consent'); - const rawSetCookie: string | string[] = first.headers['set-cookie'] ?? []; - const setCookies = Array.isArray(rawSetCookie) ? rawSetCookie : [rawSetCookie]; - const sessionCookie = setCookies - .map((cookie) => cookie.split(';')[0]) - .find((cookie) => cookie.startsWith('n8n-oauth-session=')); + const { flowId, cookie: sessionCookie } = pendingFlowFrom(first); expect(sessionCookie).toBeDefined(); // The visitor now logs in (this is the "just logged in" step from the ticket), @@ -529,7 +523,7 @@ describe('consent reuse on a second visit', () => { const consentAgent = testServer.authAgentFor(owner); consentAgent.jar.setCookie(sessionCookie ?? ''); - const details = await consentAgent.get('/consent/details'); + const details = await consentAgent.get(`/consent/details?flow=${flowId}`); // Desired: already consented, so this should signal auto-approval instead of // requiring another manual click. diff --git a/packages/cli/src/modules/oauth-server/__tests__/oauth-consent.controller.api.test.ts b/packages/cli/src/modules/oauth-server/__tests__/oauth-consent.controller.api.test.ts index d57474ad8a8..8c3476b82c9 100644 --- a/packages/cli/src/modules/oauth-server/__tests__/oauth-consent.controller.api.test.ts +++ b/packages/cli/src/modules/oauth-server/__tests__/oauth-consent.controller.api.test.ts @@ -11,6 +11,7 @@ import { setupTestServer } from '@test-integration/utils'; import type { OAuthClient } from '../database/entities/oauth-client.entity'; import { OAuthClientRepository } from '../database/repositories/oauth-client.repository'; import type { OAuthSessionPayload } from '../oauth-session.service'; +import { sessionCookieFor, TEST_FLOW_ID } from './oauth-flow-test-utils'; const testServer = setupTestServer({ endpointGroups: ['mcp'], modules: ['oauth-server', 'mcp'] }); @@ -74,8 +75,8 @@ describe('GET /rest/consent/details', () => { const response = await testServer .authAgentFor(owner) - .get('/consent/details') - .set('Cookie', `n8n-oauth-session=${sessionToken}`); + .get(`/consent/details?flow=${TEST_FLOW_ID}`) + .set('Cookie', sessionCookieFor(TEST_FLOW_ID, sessionToken)); expect(response.statusCode).toBe(200); expect(response.body.data).toEqual({ @@ -120,8 +121,8 @@ describe('GET /rest/consent/details', () => { const response = await testServer .authAgentFor(owner) - .get('/consent/details') - .set('Cookie', `n8n-oauth-session=${sessionToken}`); + .get(`/consent/details?flow=${TEST_FLOW_ID}`) + .set('Cookie', sessionCookieFor(TEST_FLOW_ID, sessionToken)); expect(response.statusCode).toBe(200); expect(response.body.data).toEqual({ @@ -153,8 +154,8 @@ describe('GET /rest/consent/details', () => { const response = await testServer .authAgentFor(owner) - .get('/consent/details') - .set('Cookie', `n8n-oauth-session=${sessionToken}`); + .get(`/consent/details?flow=${TEST_FLOW_ID}`) + .set('Cookie', sessionCookieFor(TEST_FLOW_ID, sessionToken)); expect(response.statusCode).toBe(422); expect(response.body).toEqual({ @@ -168,7 +169,9 @@ describe('GET /rest/consent/details', () => { }); test('should return 400 when session cookie is missing', async () => { - const response = await testServer.authAgentFor(owner).get('/consent/details'); + const response = await testServer + .authAgentFor(owner) + .get(`/consent/details?flow=${TEST_FLOW_ID}`); expect(response.statusCode).toBeGreaterThanOrEqual(400); expect(response.body).toEqual({ @@ -180,8 +183,8 @@ describe('GET /rest/consent/details', () => { test('should return 400 when session token is invalid', async () => { const response = await testServer .authAgentFor(owner) - .get('/consent/details') - .set('Cookie', 'n8n-oauth-session=invalid-token'); + .get(`/consent/details?flow=${TEST_FLOW_ID}`) + .set('Cookie', sessionCookieFor(TEST_FLOW_ID, 'invalid-token')); expect(response.statusCode).toBeGreaterThanOrEqual(400); expect(response.body).toEqual({ @@ -202,8 +205,8 @@ describe('GET /rest/consent/details', () => { const response = await testServer .authAgentFor(owner) - .get('/consent/details') - .set('Cookie', `n8n-oauth-session=${sessionToken}`); + .get(`/consent/details?flow=${TEST_FLOW_ID}`) + .set('Cookie', sessionCookieFor(TEST_FLOW_ID, sessionToken)); expect(response.statusCode).toBeGreaterThanOrEqual(400); expect(response.body).toEqual({ @@ -215,18 +218,18 @@ describe('GET /rest/consent/details', () => { test('should clear session cookie on invalid token', async () => { const response = await testServer .authAgentFor(owner) - .get('/consent/details') - .set('Cookie', 'n8n-oauth-session=invalid-token'); + .get(`/consent/details?flow=${TEST_FLOW_ID}`) + .set('Cookie', sessionCookieFor(TEST_FLOW_ID, 'invalid-token')); expect(response.statusCode).toBeGreaterThanOrEqual(400); const setCookieHeader = response.headers['set-cookie']; expect(setCookieHeader).toBeDefined(); - expect(setCookieHeader[0]).toContain('n8n-oauth-session='); + expect(setCookieHeader[0]).toContain(sessionCookieFor(TEST_FLOW_ID, '')); expect(setCookieHeader[0]).toMatch(/Max-Age=0|Expires=Thu, 01 Jan 1970/); }); test('should require authentication', async () => { - const response = await testServer.authlessAgent.get('/consent/details'); + const response = await testServer.authlessAgent.get(`/consent/details?flow=${TEST_FLOW_ID}`); expect(response.statusCode).toBe(401); }); @@ -251,16 +254,16 @@ describe('GET /rest/consent/details', () => { const ownerResponse = await testServer .authAgentFor(owner) - .get('/consent/details') - .set('Cookie', `n8n-oauth-session=${sessionToken}`); + .get(`/consent/details?flow=${TEST_FLOW_ID}`) + .set('Cookie', sessionCookieFor(TEST_FLOW_ID, sessionToken)); expect(ownerResponse.statusCode).toBe(200); expect(ownerResponse.body.data.clientName).toBe('Test Client 2'); const memberResponse = await testServer .authAgentFor(member) - .get('/consent/details') - .set('Cookie', `n8n-oauth-session=${sessionToken}`); + .get(`/consent/details?flow=${TEST_FLOW_ID}`) + .set('Cookie', sessionCookieFor(TEST_FLOW_ID, sessionToken)); expect(memberResponse.statusCode).toBe(200); expect(memberResponse.body.data.clientName).toBe('Test Client 2'); @@ -294,8 +297,8 @@ describe('POST /rest/consent/approve', () => { const response = await testServer .authAgentFor(owner) .post('/consent/approve') - .set('Cookie', `n8n-oauth-session=${sessionToken}`) - .send({ approved: true, scopes: ['workflow:read'] }); + .set('Cookie', sessionCookieFor(TEST_FLOW_ID, sessionToken)) + .send({ approved: true, scopes: ['workflow:read'], flow: TEST_FLOW_ID }); expect(response.statusCode).toBe(200); expect(response.body.data).toEqual({ @@ -316,8 +319,8 @@ describe('POST /rest/consent/approve', () => { const response = await testServer .authAgentFor(owner) .post('/consent/approve') - .set('Cookie', `n8n-oauth-session=${sessionToken}`) - .send({ approved: true, scopes: ['workflow:read', 'workflow:read'] }); + .set('Cookie', sessionCookieFor(TEST_FLOW_ID, sessionToken)) + .send({ approved: true, scopes: ['workflow:read', 'workflow:read'], flow: TEST_FLOW_ID }); expect(response.statusCode).toBe(400); }); @@ -326,8 +329,8 @@ describe('POST /rest/consent/approve', () => { const response = await testServer .authAgentFor(owner) .post('/consent/approve') - .set('Cookie', `n8n-oauth-session=${sessionToken}`) - .send({ approved: false }); + .set('Cookie', sessionCookieFor(TEST_FLOW_ID, sessionToken)) + .send({ approved: false, flow: TEST_FLOW_ID }); expect(response.statusCode).toBe(200); expect(response.body.data).toEqual({ @@ -348,14 +351,14 @@ describe('POST /rest/consent/approve', () => { const response = await testServer .authAgentFor(owner) .post('/consent/approve') - .set('Cookie', `n8n-oauth-session=${sessionToken}`) - .send({ approved: true, scopes: ['workflow:read'] }); + .set('Cookie', sessionCookieFor(TEST_FLOW_ID, sessionToken)) + .send({ approved: true, scopes: ['workflow:read'], flow: TEST_FLOW_ID }); expect(response.statusCode).toBe(200); const setCookieHeader = response.headers['set-cookie']; expect(setCookieHeader).toBeDefined(); - expect(setCookieHeader[0]).toContain('n8n-oauth-session='); + expect(setCookieHeader[0]).toContain(sessionCookieFor(TEST_FLOW_ID, '')); expect(setCookieHeader[0]).toMatch(/Max-Age=0|Expires=Thu, 01 Jan 1970/); }); @@ -363,8 +366,8 @@ describe('POST /rest/consent/approve', () => { const response = await testServer .authAgentFor(owner) .post('/consent/approve') - .set('Cookie', `n8n-oauth-session=${sessionToken}`) - .send({}); + .set('Cookie', sessionCookieFor(TEST_FLOW_ID, sessionToken)) + .send({ flow: TEST_FLOW_ID }); expect(response.statusCode).toBeGreaterThanOrEqual(400); expect(response.body).toMatchObject({ @@ -378,8 +381,8 @@ describe('POST /rest/consent/approve', () => { const response = await testServer .authAgentFor(owner) .post('/consent/approve') - .set('Cookie', `n8n-oauth-session=${sessionToken}`) - .send({ approved: 'yes' }); + .set('Cookie', sessionCookieFor(TEST_FLOW_ID, sessionToken)) + .send({ approved: 'yes', flow: TEST_FLOW_ID }); expect(response.statusCode).toBeGreaterThanOrEqual(400); expect(response.body).toMatchObject({ @@ -394,7 +397,7 @@ describe('POST /rest/consent/approve', () => { const response = await testServer .authAgentFor(owner) .post('/consent/approve') - .send({ approved: true, scopes: ['workflow:read'] }); + .send({ approved: true, scopes: ['workflow:read'], flow: TEST_FLOW_ID }); expect(response.statusCode).toBeGreaterThanOrEqual(400); expect(response.body).toEqual({ @@ -407,8 +410,8 @@ describe('POST /rest/consent/approve', () => { const response = await testServer .authAgentFor(owner) .post('/consent/approve') - .set('Cookie', 'n8n-oauth-session=invalid-token') - .send({ approved: true, scopes: ['workflow:read'] }); + .set('Cookie', sessionCookieFor(TEST_FLOW_ID, 'invalid-token')) + .send({ approved: true, scopes: ['workflow:read'], flow: TEST_FLOW_ID }); expect(response.statusCode).toBeGreaterThanOrEqual(400); expect(response.body.status).toBe('error'); @@ -418,20 +421,20 @@ describe('POST /rest/consent/approve', () => { const response = await testServer .authAgentFor(owner) .post('/consent/approve') - .set('Cookie', 'n8n-oauth-session=invalid-token') - .send({ approved: true, scopes: ['workflow:read'] }); + .set('Cookie', sessionCookieFor(TEST_FLOW_ID, 'invalid-token')) + .send({ approved: true, scopes: ['workflow:read'], flow: TEST_FLOW_ID }); const setCookieHeader = response.headers['set-cookie']; expect(setCookieHeader).toBeDefined(); - expect(setCookieHeader[0]).toContain('n8n-oauth-session='); + expect(setCookieHeader[0]).toContain(sessionCookieFor(TEST_FLOW_ID, '')); expect(setCookieHeader[0]).toMatch(/Max-Age=0|Expires=Thu, 01 Jan 1970/); }); test('should require authentication', async () => { const response = await testServer.authlessAgent .post('/consent/approve') - .set('Cookie', `n8n-oauth-session=${sessionToken}`) - .send({ approved: true, scopes: ['workflow:read'] }); + .set('Cookie', sessionCookieFor(TEST_FLOW_ID, sessionToken)) + .send({ approved: true, scopes: ['workflow:read'], flow: TEST_FLOW_ID }); expect(response.statusCode).toBe(401); }); @@ -440,8 +443,8 @@ describe('POST /rest/consent/approve', () => { const response = await testServer .authAgentFor(owner) .post('/consent/approve') - .set('Cookie', `n8n-oauth-session=${sessionToken}`) - .send({ approved: true, scopes: ['workflow:read'] }); + .set('Cookie', sessionCookieFor(TEST_FLOW_ID, sessionToken)) + .send({ approved: true, scopes: ['workflow:read'], flow: TEST_FLOW_ID }); expect(response.statusCode).toBe(200); @@ -463,8 +466,8 @@ describe('POST /rest/consent/approve', () => { const response = await testServer .authAgentFor(owner) .post('/consent/approve') - .set('Cookie', `n8n-oauth-session=${sessionToken}`) - .send({ approved: false }); + .set('Cookie', sessionCookieFor(TEST_FLOW_ID, sessionToken)) + .send({ approved: false, flow: TEST_FLOW_ID }); expect(response.statusCode).toBe(200); @@ -483,8 +486,8 @@ describe('POST /rest/consent/approve', () => { const ownerResponse = await testServer .authAgentFor(owner) .post('/consent/approve') - .set('Cookie', `n8n-oauth-session=${sessionToken}`) - .send({ approved: true, scopes: ['workflow:read'] }); + .set('Cookie', sessionCookieFor(TEST_FLOW_ID, sessionToken)) + .send({ approved: true, scopes: ['workflow:read'], flow: TEST_FLOW_ID }); expect(ownerResponse.statusCode).toBe(200); expect(ownerResponse.body.data.redirectUrl).toContain('code='); @@ -499,8 +502,8 @@ describe('POST /rest/consent/approve', () => { const memberResponse = await testServer .authAgentFor(member) .post('/consent/approve') - .set('Cookie', `n8n-oauth-session=${newSessionToken}`) - .send({ approved: false }); + .set('Cookie', sessionCookieFor(TEST_FLOW_ID, newSessionToken)) + .send({ approved: false, flow: TEST_FLOW_ID }); expect(memberResponse.statusCode).toBe(200); expect(memberResponse.body.data.redirectUrl).toContain('error=access_denied'); @@ -528,8 +531,8 @@ describe('Consent Flow - End-to-End', () => { const detailsResponse = await testServer .authAgentFor(owner) - .get('/consent/details') - .set('Cookie', `n8n-oauth-session=${sessionToken}`); + .get(`/consent/details?flow=${TEST_FLOW_ID}`) + .set('Cookie', sessionCookieFor(TEST_FLOW_ID, sessionToken)); expect(detailsResponse.statusCode).toBe(200); expect(detailsResponse.body.data.clientName).toBe('End-to-End Test Client'); @@ -537,8 +540,8 @@ describe('Consent Flow - End-to-End', () => { const approvalResponse = await testServer .authAgentFor(owner) .post('/consent/approve') - .set('Cookie', `n8n-oauth-session=${sessionToken}`) - .send({ approved: true, scopes: ['workflow:read'] }); + .set('Cookie', sessionCookieFor(TEST_FLOW_ID, sessionToken)) + .send({ approved: true, scopes: ['workflow:read'], flow: TEST_FLOW_ID }); expect(approvalResponse.statusCode).toBe(200); expect(approvalResponse.body.data.status).toBe('success'); @@ -547,7 +550,7 @@ describe('Consent Flow - End-to-End', () => { const setCookieHeader = approvalResponse.headers['set-cookie']; expect(setCookieHeader).toBeDefined(); - expect(setCookieHeader[0]).toContain('n8n-oauth-session='); + expect(setCookieHeader[0]).toContain(sessionCookieFor(TEST_FLOW_ID, '')); expect(setCookieHeader[0]).toMatch(/Max-Age=0|Expires=Thu, 01 Jan 1970/); }); }); diff --git a/packages/cli/src/modules/oauth-server/__tests__/oauth-flow-test-utils.ts b/packages/cli/src/modules/oauth-server/__tests__/oauth-flow-test-utils.ts new file mode 100644 index 00000000000..fec78427570 --- /dev/null +++ b/packages/cli/src/modules/oauth-server/__tests__/oauth-flow-test-utils.ts @@ -0,0 +1,28 @@ +import { OAUTH_SESSION_COOKIE_PREFIX } from '../oauth-session.service'; + +type ResponseWithHeaders = { headers: Record }; + +/** + * Read the pending authorization flow out of an /authorize redirect: the flow + * id it sends the browser to the consent screen with, and the session cookie it + * set for that flow. + */ +export const pendingFlowFrom = (response: ResponseWithHeaders) => { + const location = String(response.headers.location ?? ''); + const flowId = new URL(location, 'http://localhost').searchParams.get('flow') ?? ''; + + const rawSetCookie = response.headers['set-cookie'] ?? []; + const setCookies = Array.isArray(rawSetCookie) ? rawSetCookie : [rawSetCookie]; + const cookie = setCookies + .map((setCookie) => setCookie.split(';')[0]) + .find((setCookie) => setCookie.startsWith(`${OAUTH_SESSION_COOKIE_PREFIX}${flowId}=`)); + + return { flowId, cookie }; +}; + +/** Cookie header carrying one flow's session token. */ +export const sessionCookieFor = (flowId: string, sessionToken: string) => + `${OAUTH_SESSION_COOKIE_PREFIX}${flowId}=${sessionToken}`; + +/** A well-formed flow id for tests that mint their own session instead of calling /authorize. */ +export const TEST_FLOW_ID = 'ktest00-0123456789abcdef01234567'; diff --git a/packages/cli/src/modules/oauth-server/__tests__/oauth-server.service.test.ts b/packages/cli/src/modules/oauth-server/__tests__/oauth-server.service.test.ts index 168b8223676..0cc336d493a 100644 --- a/packages/cli/src/modules/oauth-server/__tests__/oauth-server.service.test.ts +++ b/packages/cli/src/modules/oauth-server/__tests__/oauth-server.service.test.ts @@ -31,6 +31,8 @@ import { OAuthTokenService } from '../oauth-token.service'; const SUPPORTED_SCOPES = ['tool:listWorkflows', 'tool:getWorkflowDetails']; const TEST_RESOURCE_URL = 'https://n8n.example.com/mcp-server/http'; +/** Flow id the mocked session service hands back for each pending authorization. */ +const FLOW_ID = 'ktest00-0123456789abcdef01234567'; let logger: Mocked; let oauthSessionService: Mocked; @@ -100,6 +102,7 @@ describe('OAuthServerService', () => { vi.clearAllMocks(); getAllowedRedirectUris.mockResolvedValue([]); isResourceAvailable.mockResolvedValue(true); + oauthSessionService.createSession.mockReturnValue(FLOW_ID); }); describe('clientsStore', () => { @@ -675,7 +678,7 @@ describe('OAuthServerService', () => { state: 'state-xyz', resource: 'https://n8n.example.com/mcp-server/http', }); - expect(res.redirect).toHaveBeenCalledWith('/oauth/consent'); + expect(res.redirect).toHaveBeenCalledWith(`/oauth/consent?flow=${FLOW_ID}`); }); it('should reject with invalid_target when the named resource is unavailable', async () => { @@ -795,7 +798,7 @@ describe('OAuthServerService', () => { await service.authorize(client, params, res); expect(oauthSessionService.createSession).toHaveBeenCalled(); - expect(res.redirect).toHaveBeenCalledWith('/oauth/consent'); + expect(res.redirect).toHaveBeenCalledWith(`/oauth/consent?flow=${FLOW_ID}`); }); }); @@ -902,7 +905,7 @@ describe('OAuthServerService', () => { res, expect.objectContaining({ redirectUri: 'http://localhost:52680/callback' }), ); - expect(res.redirect).toHaveBeenCalledWith('/oauth/consent'); + expect(res.redirect).toHaveBeenCalledWith(`/oauth/consent?flow=${FLOW_ID}`); }); it('should reject a loopback redirect URI whose path differs from the allowlist entry', async () => { @@ -997,7 +1000,7 @@ describe('OAuthServerService', () => { state: 'state-xyz', resource: undefined, }); - expect(res.redirect).toHaveBeenCalledWith('/oauth/consent'); + expect(res.redirect).toHaveBeenCalledWith(`/oauth/consent?flow=${FLOW_ID}`); expect(logger.warn).not.toHaveBeenCalled(); }); @@ -1027,7 +1030,7 @@ describe('OAuthServerService', () => { await service.authorize(client, params, res); expect(oauthSessionService.createSession).not.toHaveBeenCalled(); - expect(oauthSessionService.clearSession).toHaveBeenCalledWith(res); + expect(oauthSessionService.clearSession).not.toHaveBeenCalled(); expect(res.status).toHaveBeenCalledWith(400); expect(res.json).toHaveBeenCalledWith({ error: 'invalid_target', @@ -1067,10 +1070,10 @@ describe('OAuthServerService', () => { state: null, resource: 'https://n8n.example.com/mcp-server/http', }); - expect(res.redirect).toHaveBeenCalledWith('/oauth/consent'); + expect(res.redirect).toHaveBeenCalledWith(`/oauth/consent?flow=${FLOW_ID}`); }); - it('should handle errors and clear session', async () => { + it('should handle errors without touching other pending flows', async () => { const client = { client_id: 'client-123', client_name: 'Test Client', @@ -1105,7 +1108,7 @@ describe('OAuthServerService', () => { error, clientId: 'client-123', }); - expect(oauthSessionService.clearSession).toHaveBeenCalledWith(res); + expect(oauthSessionService.clearSession).not.toHaveBeenCalled(); expect(res.status).toHaveBeenCalledWith(500); expect(res.json).toHaveBeenCalledWith({ error: 'server_error', diff --git a/packages/cli/src/modules/oauth-server/__tests__/oauth-session.service.test.ts b/packages/cli/src/modules/oauth-server/__tests__/oauth-session.service.test.ts index ec38e2269c4..6faa7e30d9f 100644 --- a/packages/cli/src/modules/oauth-server/__tests__/oauth-session.service.test.ts +++ b/packages/cli/src/modules/oauth-server/__tests__/oauth-session.service.test.ts @@ -1,18 +1,25 @@ import type { GlobalConfig } from '@n8n/config'; +import type { Request, Response } from 'express'; import { JsonWebTokenError, TokenExpiredError } from 'jsonwebtoken'; import { mock } from 'vitest-mock-extended'; import { ZodError } from 'zod'; import { JwtService } from '@n8n/backend-services'; -import { OAuthSessionService, type OAuthSessionPayload } from '../oauth-session.service'; +import { + OAUTH_SESSION_COOKIE_PREFIX, + OAuthSessionService, + type OAuthSessionPayload, +} from '../oauth-session.service'; describe('OAuthSessionService', () => { const globalConfig = mock({ userManagement: { jwtSecret: 'random-secret' }, + auth: { cookie: { secure: true, samesite: 'lax' } }, + endpoints: { rest: 'rest' }, }); const jwtService = new JwtService(mock(), globalConfig, mock()); - const oauthSessionService = new OAuthSessionService(jwtService); + const oauthSessionService = new OAuthSessionService(jwtService, globalConfig); const sessionPayload: OAuthSessionPayload = { clientId: 'client-id', @@ -21,6 +28,163 @@ describe('OAuthSessionService', () => { state: 'client-state', }; + /** A response whose request carries the given pending flow cookies. */ + const responseWith = (cookies: Record = {}) => + mock({ req: mock({ cookies }) }); + + describe('createSession', () => { + it('should store the session under a flow id of its own', () => { + const res = responseWith(); + + const flowId = oauthSessionService.createSession(res, sessionPayload); + + expect(flowId).toMatch(/^[0-9a-z]{6,12}-[0-9a-f]{24}$/); + // Exact, so a `path` narrower than the default cannot be added back: + // it would hide the pending flows from /authorize, where they are + // evicted, or from the consent endpoints, where they are read. + expect(res.cookie).toHaveBeenCalledWith( + `${OAUTH_SESSION_COOKIE_PREFIX}${flowId}`, + expect.any(String), + { + httpOnly: true, + secure: true, + sameSite: 'lax', + maxAge: 10 * 60 * 1000, + }, + ); + }); + + // `strict` would drop the cookie when another site hands the browser to + // /authorize, which is how every flow starts. + it('should relax a strict sameSite setting to lax', () => { + globalConfig.auth.cookie.samesite = 'strict'; + const res = responseWith(); + + try { + oauthSessionService.createSession(res, sessionPayload); + } finally { + globalConfig.auth.cookie.samesite = 'lax'; + } + + expect(res.cookie).toHaveBeenCalledWith( + expect.any(String), + expect.any(String), + expect.objectContaining({ sameSite: 'lax' }), + ); + }); + + it('should pass a none sameSite setting through', () => { + globalConfig.auth.cookie.samesite = 'none'; + const res = responseWith(); + + try { + oauthSessionService.createSession(res, sessionPayload); + } finally { + globalConfig.auth.cookie.samesite = 'lax'; + } + + expect(res.cookie).toHaveBeenCalledWith( + expect.any(String), + expect.any(String), + expect.objectContaining({ sameSite: 'none' }), + ); + }); + + it('should give two authorization requests separate cookies', () => { + const first = responseWith(); + const firstFlowId = oauthSessionService.createSession(first, sessionPayload); + + // The browser now holds the first flow's cookie and starts a second flow. + const second = responseWith({ + [`${OAUTH_SESSION_COOKIE_PREFIX}${firstFlowId}`]: 'first-token', + }); + const secondFlowId = oauthSessionService.createSession(second, { + ...sessionPayload, + codeChallenge: 'second-challenge', + }); + + expect(secondFlowId).not.toBe(firstFlowId); + expect(second.clearCookie).not.toHaveBeenCalled(); + expect(second.cookie).toHaveBeenCalledWith( + `${OAUTH_SESSION_COOKIE_PREFIX}${secondFlowId}`, + expect.any(String), + expect.anything(), + ); + }); + + it('should evict the oldest pending flow once the cap is reached', () => { + // Three already pending, so the new one takes the oldest one's slot. Flow ids + // carry a timestamp prefix, so sorting the names sorts them oldest first. + const pending = ['a00000', 'b00000', 'c00000'].map( + (prefix) => `${OAUTH_SESSION_COOKIE_PREFIX}${prefix}-${'0'.repeat(24)}`, + ); + const res = responseWith(Object.fromEntries(pending.map((name) => [name, 'token']))); + + oauthSessionService.createSession(res, sessionPayload); + + expect(res.clearCookie).toHaveBeenCalledTimes(1); + expect(res.clearCookie).toHaveBeenCalledWith(pending[0], expect.anything()); + }); + + it('should leave cookies that are not pending flows alone', () => { + const res = responseWith({ 'n8n-auth': 'auth-token' }); + + oauthSessionService.createSession(res, sessionPayload); + + expect(res.clearCookie).not.toHaveBeenCalled(); + }); + }); + + describe('getSessionToken', () => { + it('should return the token of the named flow', () => { + const flowId = `ktest00-${'a'.repeat(24)}`; + const cookies = { [`${OAUTH_SESSION_COOKIE_PREFIX}${flowId}`]: 'session-token' }; + + expect(oauthSessionService.getSessionToken(cookies, flowId)).toBe('session-token'); + }); + + it('should not return the token of a different flow', () => { + const cookies = { [`${OAUTH_SESSION_COOKIE_PREFIX}ktest00-${'a'.repeat(24)}`]: 'other' }; + + expect( + oauthSessionService.getSessionToken(cookies, `ktest01-${'b'.repeat(24)}`), + ).toBeUndefined(); + }); + + // Keyed by the malformed id, so only the guard can make these pass: an + // implementation that looked the name up regardless would return the token. + it.each(['', 'not-a-flow-id', '../n8n-auth', '__proto__'])( + 'should reject the malformed flow id %s', + (flowId) => { + const cookies = { [`${OAUTH_SESSION_COOKIE_PREFIX}${flowId}`]: 'stale-token' }; + + expect(oauthSessionService.getSessionToken(cookies, flowId)).toBeUndefined(); + }, + ); + }); + + describe('clearSession', () => { + it('should clear only the named flow', () => { + const res = responseWith(); + const flowId = `ktest00-${'a'.repeat(24)}`; + + oauthSessionService.clearSession(res, flowId); + + expect(res.clearCookie).toHaveBeenCalledWith( + `${OAUTH_SESSION_COOKIE_PREFIX}${flowId}`, + expect.anything(), + ); + }); + + it('should do nothing for a malformed flow id', () => { + const res = responseWith(); + + oauthSessionService.clearSession(res, 'not-a-flow-id'); + + expect(res.clearCookie).not.toHaveBeenCalled(); + }); + }); + describe('verifySession', () => { it('should return the payload of an authorization session token', () => { const token = jwtService.sign('oauthSession', sessionPayload, { expiresIn: '10m' }); diff --git a/packages/cli/src/modules/oauth-server/__tests__/oauth.controller.api.test.ts b/packages/cli/src/modules/oauth-server/__tests__/oauth.controller.api.test.ts index a0d8fa8ca58..577ce4d8a9a 100644 --- a/packages/cli/src/modules/oauth-server/__tests__/oauth.controller.api.test.ts +++ b/packages/cli/src/modules/oauth-server/__tests__/oauth.controller.api.test.ts @@ -3,15 +3,21 @@ import { GlobalConfig } from '@n8n/config'; import type { User } from '@n8n/db'; import { ControllerRegistryMetadata, type Controller } from '@n8n/decorators'; import { Container } from '@n8n/di'; +import { randomUUID } from 'node:crypto'; +import request from 'supertest'; +import { AuthService } from '@/auth/auth.service'; +import { AUTH_COOKIE_NAME } from '@/constants'; import { McpSettingsService } from '@/modules/mcp/mcp.settings.service'; import { UrlService } from '@n8n/backend-services'; import { ProtectedResourceRegistry } from '@n8n/backend-services/auth'; import { createOwner } from '@test-integration/db/users'; import { setupTestServer } from '@test-integration/utils'; +import { OAuthClientRepository } from '../database/repositories/oauth-client.repository'; import { OAuthServerConfig } from '../oauth-server.config'; import type { OAuthController as OAuthControllerClass } from '../oauth.controller'; +import { pendingFlowFrom } from './oauth-flow-test-utils'; const testServer = setupTestServer({ modules: ['oauth-server', 'mcp'], endpointGroups: ['mcp'] }); @@ -715,13 +721,11 @@ describe('Full authorization-code flow (PKCE)', () => { state: 'flow-state', }); expect(authorizeResponse.statusCode).toBe(302); - expect(authorizeResponse.headers.location).toBe('/oauth/consent'); - const rawSetCookie: string | string[] = authorizeResponse.headers['set-cookie'] ?? []; - const setCookies = Array.isArray(rawSetCookie) ? rawSetCookie : [rawSetCookie]; - const sessionCookie = setCookies - .map((cookie) => cookie.split(';')[0]) - .find((cookie) => cookie.startsWith('n8n-oauth-session=')); + // The redirect names this authorization request, so the consent screen can + // ask for the one it was opened for. + const { flowId, cookie: sessionCookie } = pendingFlowFrom(authorizeResponse); + expect(authorizeResponse.headers.location).toBe(`/oauth/consent?flow=${flowId}`); expect(sessionCookie).toBeDefined(); // 3. Consent approval as an authenticated user. @@ -739,7 +743,7 @@ describe('Full authorization-code flow (PKCE)', () => { authAgent.jar.setCookie(sessionCookie ?? ''); const consentResponse = await authAgent .post('/consent/approve') - .send({ approved: true, scopes: grantedScopes }); + .send({ approved: true, scopes: grantedScopes, flow: flowId }); expect(consentResponse.statusCode).toBe(200); const redirectUrl = new URL(consentResponse.body.data.redirectUrl); @@ -783,6 +787,154 @@ describe('Full authorization-code flow (PKCE)', () => { expect(refreshResponse.body.access_token).toEqual(expect.any(String)); expect(refreshResponse.body.access_token).not.toBe(tokenResponse.body.access_token); }); + + // A client that starts two authorization requests from one browser (a desktop + // app that launches its bridge twice, or a user who retries) used to lose the + // first one: both requests shared a single session cookie, so the approval + // minted a code bound to the second request's PKCE challenge while the process + // waiting on the callback held the first request's verifier. + test('should keep two authorization requests from one browser independent', async () => { + const { createHash, randomBytes } = await import('node:crypto'); + const pkcePair = () => { + const verifier = randomBytes(32).toString('base64url'); + return { verifier, challenge: createHash('sha256').update(verifier).digest('base64url') }; + }; + const first = pkcePair(); + const second = pkcePair(); + + const registerResponse = await testServer.restlessAgent.post('/mcp-oauth/register').send({ + client_name: 'Concurrent Flow Client', + redirect_uris: ['https://example.com/callback'], + grant_types: ['authorization_code'], + token_endpoint_auth_method: 'none', + }); + const clientId = registerResponse.body.client_id; + + // One browser, so one cookie jar for both authorization requests. Its own + // agent, because the flow cookies outlive each request and the shared + // agents are reused by every other test in this file. + const browser = request.agent(testServer.app); + const authorize = async (codeChallenge: string, state: string) => { + const response = await browser.get('/mcp-oauth/authorize').query({ + client_id: clientId, + redirect_uri: 'https://example.com/callback', + response_type: 'code', + code_challenge: codeChallenge, + code_challenge_method: 'S256', + state, + }); + expect(response.statusCode).toBe(302); + const flow = pendingFlowFrom(response); + expect(flow.cookie).toBeDefined(); + return flow; + }; + + // Both requests are pending at once, each with its own flow id and cookie. + const firstFlow = await authorize(first.challenge, 'state-first'); + const secondFlow = await authorize(second.challenge, 'state-second'); + expect(firstFlow.flowId).not.toBe(secondFlow.flowId); + + // The consent screen carries both cookies and asks for one flow by id. + const consentAgent = testServer.authAgentFor(owner); + consentAgent.jar.setCookie(firstFlow.cookie ?? ''); + consentAgent.jar.setCookie(secondFlow.cookie ?? ''); + + const details = await consentAgent.get(`/consent/details?flow=${firstFlow.flowId}`); + expect(details.body).toMatchObject({ data: { clientId } }); + + // Approve the first request, granting exactly what its screen offered. + const consentResponse = await consentAgent + .post('/consent/approve') + .send({ approved: true, scopes: details.body.data.scopes, flow: firstFlow.flowId }); + expect(consentResponse.body).toMatchObject({ data: { status: 'success' } }); + + // The code carries the first request's state, and only the first request's + // verifier redeems it. + const redirectUrl = new URL(consentResponse.body.data.redirectUrl); + expect(redirectUrl.searchParams.get('state')).toBe('state-first'); + const code = redirectUrl.searchParams.get('code'); + + const exchange = async (codeVerifier: string) => + await testServer.restlessAgent.post('/mcp-oauth/token').type('form').send({ + grant_type: 'authorization_code', + code: code!, + client_id: clientId, + code_verifier: codeVerifier, + redirect_uri: 'https://example.com/callback', + }); + + const wrongVerifier = await exchange(second.verifier); + expect(wrongVerifier.statusCode).toBe(400); + expect(wrongVerifier.body.error).toBe('invalid_grant'); + + const rightVerifier = await exchange(first.verifier); + expect(rightVerifier.body).toMatchObject({ access_token: expect.any(String) }); + + // The second request survived the first one's approval and is still decidable. + const secondConsent = await consentAgent + .post('/consent/approve') + .send({ approved: true, scopes: details.body.data.scopes, flow: secondFlow.flowId }); + expect(secondConsent.body).toMatchObject({ data: { status: 'success' } }); + expect(new URL(secondConsent.body.data.redirectUrl).searchParams.get('state')).toBe( + 'state-second', + ); + }); + + // Eviction depends on the browser offering its pending flow cookies back to + // /authorize, which is a property of how the cookies are scoped, not of the + // service. A mock request cannot show that: it is handed the cookies it is + // asked about. So this drives a real cookie jar past the cap and reads the + // result through the consent endpoint. + test('should evict the oldest pending flow once a browser passes the cap', async () => { + // Written straight to the table: the DCR endpoint is IP rate limited, and + // this test is about pending flows rather than registration. + const clientId = `pending-flow-cap-${randomUUID()}`; + await Container.get(OAuthClientRepository).save({ + id: clientId, + name: 'Pending Flow Cap Client', + redirectUris: ['https://example.com/callback'], + grantTypes: ['authorization_code'], + tokenEndpointAuthMethod: 'none', + }); + + // One signed-in browser: no path prefix, so it reaches both the + // /mcp-oauth/authorize routes and the /rest/consent ones out of one jar, + // offering each the cookies that request's path is entitled to. + const browser = request.agent(testServer.app); + browser.jar.setCookie( + `${AUTH_COOKIE_NAME}=${Container.get(AuthService).issueJWT(owner, owner.mfaEnabled, 'test-browser-id')}`, + ); + + const authorize = async (state: string) => { + const response = await browser.get('/mcp-oauth/authorize').query({ + client_id: clientId, + redirect_uri: 'https://example.com/callback', + response_type: 'code', + code_challenge: 'E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM', + code_challenge_method: 'S256', + state, + }); + expect(response.statusCode).toBe(302); + return pendingFlowFrom(response); + }; + + const flows = []; + for (const state of ['first', 'second', 'third', 'fourth']) { + flows.push(await authorize(state)); + } + expect(new Set(flows.map((flow) => flow.flowId)).size).toBe(4); + + // The cap is three, so the fourth request pushed the first one out and the + // browser no longer holds its cookie. + const evicted = await browser.get(`/rest/consent/details?flow=${flows[0].flowId}`); + expect(evicted.statusCode).toBe(400); + + // The three newest survived, each still readable under its own flow id. + for (const flow of flows.slice(1)) { + const details = await browser.get(`/rest/consent/details?flow=${flow.flowId}`); + expect(details.body).toMatchObject({ data: { clientId } }); + } + }); }); describe('Neutral /oauth/* endpoint aliases', () => { diff --git a/packages/cli/src/modules/oauth-server/__tests__/oauth.helpers.test.ts b/packages/cli/src/modules/oauth-server/__tests__/oauth.helpers.test.ts index aa8d5bc68f6..536baade34a 100644 --- a/packages/cli/src/modules/oauth-server/__tests__/oauth.helpers.test.ts +++ b/packages/cli/src/modules/oauth-server/__tests__/oauth.helpers.test.ts @@ -218,4 +218,38 @@ describe('OAuthHelpers', () => { expect(OAuthHelpers.setIssuerParam('/oauth/consent', issuer)).toBe('/oauth/consent'); }); }); + + describe('basicAuthClientId', () => { + const basicHeader = (credentials: string) => + `Basic ${Buffer.from(credentials, 'utf8').toString('base64')}`; + + it('should return the client id from the username half', () => { + expect(OAuthHelpers.basicAuthClientId(basicHeader('client-123:s3cret'))).toBe('client-123'); + }); + + it('should not return the secret, even when the secret holds a colon', () => { + const result = OAuthHelpers.basicAuthClientId(basicHeader('client-123:s3:cret')); + + expect(result).toBe('client-123'); + expect(result).not.toContain('s3'); + }); + + it('should form-decode the client id', () => { + expect(OAuthHelpers.basicAuthClientId(basicHeader('client%20123:secret'))).toBe('client 123'); + }); + + it('should return an id that is not form-encoded as sent', () => { + expect(OAuthHelpers.basicAuthClientId(basicHeader('client%id:secret'))).toBe('client%id'); + }); + + it.each([ + undefined, + '', + 'Bearer abc123', + 'Basic', + 'Basic ' + Buffer.from(':s').toString('base64'), + ])('should return undefined for the header %s', (header) => { + expect(OAuthHelpers.basicAuthClientId(header)).toBeUndefined(); + }); + }); }); diff --git a/packages/cli/src/modules/oauth-server/__tests__/workflow-mcp-trigger-authorization.api.test.ts b/packages/cli/src/modules/oauth-server/__tests__/workflow-mcp-trigger-authorization.api.test.ts index e2c9fa0a24d..edcf9238706 100644 --- a/packages/cli/src/modules/oauth-server/__tests__/workflow-mcp-trigger-authorization.api.test.ts +++ b/packages/cli/src/modules/oauth-server/__tests__/workflow-mcp-trigger-authorization.api.test.ts @@ -20,6 +20,7 @@ import { CacheService, UrlService, JwtService } from '@n8n/backend-services'; import { OAuthClientRepository } from '../database/repositories/oauth-client.repository'; import type { OAuthSessionPayload } from '../oauth-session.service'; +import { sessionCookieFor, TEST_FLOW_ID } from './oauth-flow-test-utils'; /** * Authorization for OAuth-protected MCP Server Triggers. A workflow whose MCP @@ -194,8 +195,8 @@ describe('consent gate: workflow name and authorization code require execute acc const response = await testServer .authAgentFor(memberNoAccess) - .get('/consent/details') - .set('Cookie', `n8n-oauth-session=${sessionToken}`); + .get(`/consent/details?flow=${TEST_FLOW_ID}`) + .set('Cookie', sessionCookieFor(TEST_FLOW_ID, sessionToken)); expect(JSON.stringify(response.body)).not.toContain(workflowName); }); @@ -207,8 +208,8 @@ describe('consent gate: workflow name and authorization code require execute acc const response = await testServer .authAgentFor(owner) - .get('/consent/details') - .set('Cookie', `n8n-oauth-session=${sessionToken}`); + .get(`/consent/details?flow=${TEST_FLOW_ID}`) + .set('Cookie', sessionCookieFor(TEST_FLOW_ID, sessionToken)); expect(response.body.data.resourceName).toBe(workflowName); }); @@ -220,8 +221,8 @@ describe('consent gate: workflow name and authorization code require execute acc const response = await testServer .authAgentFor(memberNoAccess) .post('/consent/approve') - .set('Cookie', `n8n-oauth-session=${sessionToken}`) - .send({ approved: true }) + .set('Cookie', sessionCookieFor(TEST_FLOW_ID, sessionToken)) + .send({ approved: true, flow: TEST_FLOW_ID }) .expect(403); expect(String(response.body.data?.redirectUrl ?? '')).not.toContain('code='); diff --git a/packages/cli/src/modules/oauth-server/dto/approve-consent-request.dto.ts b/packages/cli/src/modules/oauth-server/dto/approve-consent-request.dto.ts index 66f1adc815a..5ed94f6f66a 100644 --- a/packages/cli/src/modules/oauth-server/dto/approve-consent-request.dto.ts +++ b/packages/cli/src/modules/oauth-server/dto/approve-consent-request.dto.ts @@ -11,4 +11,6 @@ export class ApproveConsentRequestDto extends Z.class({ message: 'scopes must not contain duplicates', }) .optional(), + /** Id of the pending authorization request to decide, from the /authorize redirect. */ + flow: z.string().max(64), }) {} diff --git a/packages/cli/src/modules/oauth-server/oauth-consent.controller.ts b/packages/cli/src/modules/oauth-server/oauth-consent.controller.ts index fce3e5259e0..d23da115a39 100644 --- a/packages/cli/src/modules/oauth-server/oauth-consent.controller.ts +++ b/packages/cli/src/modules/oauth-server/oauth-consent.controller.ts @@ -9,6 +9,9 @@ import { OAuthConsentService } from './oauth-consent.service'; import { OAuthSessionService } from './oauth-session.service'; import { ForbiddenError } from '@n8n/errors'; +/** `flow` names the pending authorization request to read, set by the /authorize redirect. */ +type ConsentDetailsRequest = AuthenticatedRequest<{}, {}, {}, { flow?: string }>; + @RestController('/consent') export class OAuthConsentController { constructor( @@ -18,20 +21,22 @@ export class OAuthConsentController { ) {} @Get('/details', { usesTemplates: true }) - async getConsentDetails(req: AuthenticatedRequest, res: Response) { + async getConsentDetails(req: ConsentDetailsRequest, res: Response) { + const flowId = typeof req.query.flow === 'string' ? req.query.flow : ''; + try { - const sessionToken = this.getAndValidateSessionToken(req, res); + const sessionToken = this.getAndValidateSessionToken(req, res, flowId); if (!sessionToken) return; const consentDetails = await this.consentService.getConsentDetails(sessionToken, req.user); if (!consentDetails) { - this.sendInvalidSessionError(res, true); + this.sendInvalidSessionError(res, flowId, true); return; } if (!consentDetails.ok) { - this.oauthSessionService.clearSession(res); + this.oauthSessionService.clearSession(res, flowId); if (consentDetails.reason === 'forbidden') { this.sendErrorResponse( res, @@ -46,7 +51,7 @@ export class OAuthConsentController { if (consentDetails.autoApproved) { // The session's decision is already made — consume it, same as a manual approval. - this.oauthSessionService.clearSession(res); + this.oauthSessionService.clearSession(res, flowId); res.json({ data: { autoApproved: true, @@ -72,7 +77,7 @@ export class OAuthConsentController { }); } catch (error) { this.logger.error('Failed to get consent details', { error }); - this.oauthSessionService.clearSession(res); + this.oauthSessionService.clearSession(res, flowId); this.sendErrorResponse(res, 500, 'Failed to load authorization details'); } } @@ -84,7 +89,7 @@ export class OAuthConsentController { @Body payload: ApproveConsentRequestDto, ) { try { - const sessionToken = this.getAndValidateSessionToken(req, res); + const sessionToken = this.getAndValidateSessionToken(req, res, payload.flow); if (!sessionToken) return; const result = await this.consentService.handleConsentDecision( @@ -94,7 +99,7 @@ export class OAuthConsentController { payload.scopes, ); - this.oauthSessionService.clearSession(res); + this.oauthSessionService.clearSession(res, payload.flow); res.json({ data: { @@ -104,7 +109,7 @@ export class OAuthConsentController { }); } catch (error) { this.logger.error('Failed to process consent', { error }); - this.oauthSessionService.clearSession(res); + this.oauthSessionService.clearSession(res, payload.flow); if (error instanceof ForbiddenError) { this.sendErrorResponse( res, @@ -126,17 +131,27 @@ export class OAuthConsentController { }); } - private sendInvalidSessionError(res: Response, clearCookie = false): void { + private sendInvalidSessionError(res: Response, flowId: string, clearCookie = false): void { if (clearCookie) { - this.oauthSessionService.clearSession(res); + this.oauthSessionService.clearSession(res, flowId); } this.sendErrorResponse(res, 400, 'Invalid or expired authorization session'); } - private getAndValidateSessionToken(req: AuthenticatedRequest, res: Response): string | null { - const sessionToken = this.oauthSessionService.getSessionToken(req.cookies); + /** + * Resolve the pending authorization request the caller names. An unknown + * flow id reads as an expired session: the browser only reaches this screen + * through an /authorize redirect carrying the id, and the first decision + * made on a flow consumes it. + */ + private getAndValidateSessionToken( + req: AuthenticatedRequest, + res: Response, + flowId: string, + ): string | null { + const sessionToken = this.oauthSessionService.getSessionToken(req.cookies, flowId); if (!sessionToken) { - this.sendInvalidSessionError(res); + this.sendInvalidSessionError(res, flowId); return null; } @@ -145,7 +160,7 @@ export class OAuthConsentController { return sessionToken; } catch (error) { this.logger.debug('Invalid session token', { error }); - this.sendInvalidSessionError(res, true); + this.sendInvalidSessionError(res, flowId, true); return null; } } diff --git a/packages/cli/src/modules/oauth-server/oauth-server.service.ts b/packages/cli/src/modules/oauth-server/oauth-server.service.ts index 306813bf555..368457f4e74 100644 --- a/packages/cli/src/modules/oauth-server/oauth-server.service.ts +++ b/packages/cli/src/modules/oauth-server/oauth-server.service.ts @@ -473,9 +473,8 @@ export class OAuthServerService implements OAuthServerProvider { return; } - this.oauthSessionService.createSession(res, sessionPayload); - - res.redirect('/oauth/consent'); + const flowId = this.oauthSessionService.createSession(res, sessionPayload); + res.redirect(`/oauth/consent?flow=${flowId}`); } catch (error) { if (error instanceof InvalidResourceIndicatorError) { this.logger.warn('Rejecting OAuth authorization request with invalid resource', { @@ -483,7 +482,6 @@ export class OAuthServerService implements OAuthServerProvider { resource: error.resource, expectedResource: error.expectedResource, }); - this.oauthSessionService.clearSession(res); res.status(400).json({ error: 'invalid_target', error_description: 'Invalid resource indicator', @@ -492,7 +490,6 @@ export class OAuthServerService implements OAuthServerProvider { } this.logger.error('Error in authorize method', { error, clientId: client.client_id }); - this.oauthSessionService.clearSession(res); res.status(500).json({ error: 'server_error', error_description: 'Internal server error' }); } } @@ -579,6 +576,11 @@ export class OAuthServerService implements OAuthServerProvider { }); } + this.logger.debug('OAuth token exchange succeeded', { + clientId: client.client_id, + userId: authRecord.userId, + }); + return { access_token: accessToken, token_type: 'Bearer', diff --git a/packages/cli/src/modules/oauth-server/oauth-session.service.ts b/packages/cli/src/modules/oauth-server/oauth-session.service.ts index 30d5b4c5bcb..95b191bc193 100644 --- a/packages/cli/src/modules/oauth-server/oauth-session.service.ts +++ b/packages/cli/src/modules/oauth-server/oauth-session.service.ts @@ -1,6 +1,8 @@ +import { GlobalConfig } from '@n8n/config'; import { Time } from '@n8n/constants'; import { Service } from '@n8n/di'; -import { Response } from 'express'; +import type { CookieOptions, Request, Response } from 'express'; +import { randomBytes } from 'node:crypto'; import { z } from 'zod'; import { JwtService } from '@n8n/backend-services'; @@ -17,31 +19,56 @@ const oauthSessionPayloadSchema = z.object({ export type OAuthSessionPayload = z.infer; -export const OAUTH_SESSION_COOKIE_NAME = 'n8n-oauth-session'; +/** Each pending authorization request owns a cookie, named by its flow id. */ +export const OAUTH_SESSION_COOKIE_PREFIX = 'n8n-oauth-session-'; const SESSION_EXPIRY_MS = 10 * Time.minutes.toMilliseconds; // 10 minutes +/** + * Pending flows one browser may hold at once, which is what keeps the request + * header bounded: the consent screen is sent every pending flow's cookie. + * + * Two /authorize calls that overlap both read the same cookie snapshot, so a + * burst can hold more than this for a moment. The next /authorize sees them all + * and prunes back to the cap, so an overshoot decays instead of accumulating. + */ +const MAX_PENDING_FLOWS = 3; + +/** The shape `newFlowId` mints. Checked before a flow id reaches a cookie name. */ +const FLOW_ID_PATTERN = /^[0-9a-z]{6,12}-[0-9a-f]{24}$/; + /** * Manages OAuth authorization session state using JWT-based cookies * Stores temporary session data during the authorization flow */ @Service() export class OAuthSessionService { - constructor(private readonly jwtService: JwtService) {} + constructor( + private readonly jwtService: JwtService, + private readonly globalConfig: GlobalConfig, + ) {} /** - * Create OAuth session token and set it as a cookie + * Store one authorization request under a new flow id, and return that id + * for the consent screen to ask for its own session by. + * + * One cookie per /authorize call, so a second request cannot replace the one + * a consent screen is already showing: the parameters the user approves stay + * the ones they were shown. */ - createSession(res: Response, payload: OAuthSessionPayload): void { + createSession(res: Response, payload: OAuthSessionPayload): string { + const flowId = this.newFlowId(); const sessionToken = this.jwtService.sign('oauthSession', payload, { expiresIn: '10m', }); - res.cookie(OAUTH_SESSION_COOKIE_NAME, sessionToken, { - httpOnly: true, - secure: process.env.NODE_ENV === 'production', - sameSite: 'lax', + this.evictOldestFlows(res, MAX_PENDING_FLOWS - 1); + + res.cookie(OAUTH_SESSION_COOKIE_PREFIX + flowId, sessionToken, { + ...this.cookieOptions(), maxAge: SESSION_EXPIRY_MS, }); + + return flowId; } /** @@ -56,17 +83,62 @@ export class OAuthSessionService { return oauthSessionPayloadSchema.parse(payload); } - /** - * Clear OAuth session cookie - */ - clearSession(res: Response): void { - res.clearCookie(OAUTH_SESSION_COOKIE_NAME); + /** Clear one flow's cookie, leaving the browser's other pending flows. */ + clearSession(res: Response, flowId: string): void { + if (!FLOW_ID_PATTERN.test(flowId)) return; + res.clearCookie(OAUTH_SESSION_COOKIE_PREFIX + flowId, this.cookieOptions()); + } + + getSessionToken(cookies: Record, flowId: string): string | undefined { + if (!FLOW_ID_PATTERN.test(flowId)) return undefined; + return cookies[OAUTH_SESSION_COOKIE_PREFIX + flowId]; } /** - * Extract session token from request cookies + * A base36 millisecond prefix plus 96 random bits. The prefix makes the + * cookie names sort by age, which is all `evictOldestFlows` needs; the + * random half is what makes a flow id unguessable. */ - getSessionToken(cookies: Record): string | undefined { - return cookies[OAUTH_SESSION_COOKIE_NAME]; + private newFlowId(): string { + return `${Date.now().toString(36)}-${randomBytes(12).toString('hex')}`; + } + + /** + * Drop the oldest pending flows, leaving room for `keep` of them. Flow ids + * minted by one instance share a prefix length, so sorting the cookie names + * sorts the flows oldest first. + */ + private evictOldestFlows(res: Response, keep: number): void { + const pending = Object.keys(this.parsedCookies(res.req)) + .filter((name) => name.startsWith(OAUTH_SESSION_COOKIE_PREFIX)) + .sort(); + + for (const name of pending.slice(0, Math.max(0, pending.length - keep))) { + res.clearCookie(name, this.cookieOptions()); + } + } + + /** + * Attributes come from the instance's cookie config, the same source as the + * auth cookie, with `sameSite` clamped to a `lax` minimum because `strict` + * would drop the cookie on a hand-off into /authorize from another site. + * + * The default path is deliberate. /authorize and the consent endpoints sit + * under different roots, so scoping to either one hides the pending flows + * from the other, and `evictOldestFlows` would never see a cookie to evict. + */ + private cookieOptions(): CookieOptions { + const { secure, samesite } = this.globalConfig.auth.cookie; + return { + httpOnly: true, + secure, + sameSite: samesite === 'strict' ? 'lax' : samesite, + }; + } + + private parsedCookies(req: Request): Record { + const cookies: unknown = req.cookies; + if (cookies === null || typeof cookies !== 'object') return {}; + return { ...cookies }; } } diff --git a/packages/cli/src/modules/oauth-server/oauth.controller.ts b/packages/cli/src/modules/oauth-server/oauth.controller.ts index ee6788eac57..6012c91b151 100644 --- a/packages/cli/src/modules/oauth-server/oauth.controller.ts +++ b/packages/cli/src/modules/oauth-server/oauth.controller.ts @@ -14,6 +14,7 @@ import { } from '@n8n/decorators'; import { Container } from '@n8n/di'; import { AuthenticationService } from '@n8n/inbound-auth'; +import { isRecord } from '@n8n/utils/is-record'; import type { Response, Request, RequestHandler, Router } from 'express'; import { UrlService } from '@n8n/backend-services'; @@ -77,6 +78,40 @@ const rfc9207IssuerParam: RequestHandler = (_req, res, next) => { next(); }; +/** + * The SDK's token handler serializes every rejection (unknown client, expired + * or reused code, PKCE mismatch, ...) straight into the HTTP response without + * logging, which leaves a failed token exchange with no trace in the server + * logs at all. Wrap `res.json` so the token route reports what it rejected and + * for which client. + */ +const logTokenFailures: RequestHandler = (req, res, next) => { + const originalJson = res.json.bind(res); + res.json = (body?: unknown) => { + if (res.statusCode >= 400) { + const errorBody = isRecord(body) ? body : {}; + // The SDK's handler parses the form body before it responds, so the + // request body is populated by the time this runs. + const requestBody = isRecord(req.body) ? req.body : {}; + // A `client_secret_basic` request has no `client_id` field; its id is the + // Basic username. + const clientId = + typeof requestBody.client_id === 'string' + ? requestBody.client_id + : OAuthHelpers.basicAuthClientId(req.headers.authorization); + logger.warn('OAuth token request failed', { + statusCode: res.statusCode, + error: errorBody.error, + errorDescription: errorBody.error_description, + grantType: requestBody.grant_type, + clientId, + }); + } + return originalJson(body); + }; + next(); +}; + // Built once and mounted under both the legacy `/mcp-oauth/*` paths (existing // DCR clients hold them in their stored discovery metadata) and the neutral // `/oauth/*` paths that future, non-MCP protected resources will advertise. @@ -119,6 +154,7 @@ const sharedEndpointRouters = (basePath: '/mcp-oauth' | '/oauth'): StaticRouterM path: `${basePath}/token`, router: tokenRouter, skipAuth: true, + middlewares: [logTokenFailures], ipRateLimit: createIpRateLimit( oauthServerConfig.rateLimitToken, 5 * Time.minutes.toMilliseconds, diff --git a/packages/cli/src/modules/oauth-server/oauth.helpers.ts b/packages/cli/src/modules/oauth-server/oauth.helpers.ts index fd2f531e0da..045b24a9c9b 100644 --- a/packages/cli/src/modules/oauth-server/oauth.helpers.ts +++ b/packages/cli/src/modules/oauth-server/oauth.helpers.ts @@ -62,4 +62,24 @@ export class OAuthHelpers { targetUrl.searchParams.set('iss', issuer); return targetUrl.toString(); } + + /** + * Client id from a `client_secret_basic` Authorization header, which carries + * it as the form-encoded username (RFC 6749 §2.3.1). Returns the username + * only, so the secret never reaches a caller. + */ + static basicAuthClientId(header?: string): string | undefined { + const encoded = /^Basic (\S+)$/i.exec(header ?? '')?.[1]; + if (!encoded) return undefined; + + const [username] = Buffer.from(encoded, 'base64').toString('utf8').split(':'); + if (!username) return undefined; + + try { + return decodeURIComponent(username); + } catch { + // A client that did not form-encode its id; report it as sent. + return username; + } + } } diff --git a/packages/cli/src/webhooks/__tests__/webhook-request-sanitizer.test.ts b/packages/cli/src/webhooks/__tests__/webhook-request-sanitizer.test.ts index bcb5bf20722..e502964bda5 100644 --- a/packages/cli/src/webhooks/__tests__/webhook-request-sanitizer.test.ts +++ b/packages/cli/src/webhooks/__tests__/webhook-request-sanitizer.test.ts @@ -2,7 +2,7 @@ import type { Request } from 'express'; import { mock } from 'vitest-mock-extended'; import { OIDC_NONCE_COOKIE_NAME, OIDC_STATE_COOKIE_NAME } from '@/constants'; -import { OAUTH_SESSION_COOKIE_NAME } from '@/modules/oauth-server/oauth-session.service'; +import { OAUTH_SESSION_COOKIE_PREFIX } from '@/modules/oauth-server/oauth-session.service'; import { OIDC_ID_TOKEN_COOKIE_NAME } from '@/modules/sso-oidc/constants'; import { OAUTH_BINDING_COOKIE_NAME } from '@/oauth/oauth-browser-binding.service'; import { sanitizeWebhookRequest } from '@/webhooks/webhook-request-sanitizer'; @@ -342,7 +342,10 @@ describe('webhookRequestSanitizer', () => { describe('cookies n8n issues for its own flows', () => { const N8N_ISSUED_COOKIES = [ - OAUTH_SESSION_COOKIE_NAME, + // One authorization flow's session cookie, named by its flow id. + `${OAUTH_SESSION_COOKIE_PREFIX}ktest00-0123456789abcdef01234567`, + // The name the authorization session used before it was keyed per flow. + 'n8n-oauth-session', OAUTH_BINDING_COOKIE_NAME, OIDC_ID_TOKEN_COOKIE_NAME, OIDC_STATE_COOKIE_NAME, @@ -364,6 +367,33 @@ describe('webhookRequestSanitizer', () => { expect(mockRequest.cookies).toEqual({ 'other-cookie': 'value' }); }); + + // A browser holds one authorization-session cookie for each pending flow, so + // matching the prefix has to take all of them, not one known name. + it('should remove every pending authorization flow cookie', () => { + const first = `${OAUTH_SESSION_COOKIE_PREFIX}ktest00-0123456789abcdef01234567`; + const second = `${OAUTH_SESSION_COOKIE_PREFIX}ktest01-89abcdef0123456789abcdef`; + mockRequest.headers = { cookie: `${first}=a; ${second}=b; other-cookie=value` }; + mockRequest.cookies = { [first]: 'a', [second]: 'b', 'other-cookie': 'value' }; + + sanitizeWebhookRequest(mockRequest); + + expect(mockRequest.headers.cookie).toBe('other-cookie=value'); + expect(mockRequest.cookies).toEqual({ 'other-cookie': 'value' }); + }); + + it('should leave a cookie that begins with the session name but not the separator', () => { + mockRequest.headers = { cookie: 'n8n-oauth-sessions=abc123; other-cookie=value' }; + mockRequest.cookies = { 'n8n-oauth-sessions': 'abc123', 'other-cookie': 'value' }; + + sanitizeWebhookRequest(mockRequest); + + expect(mockRequest.headers.cookie).toBe('n8n-oauth-sessions=abc123; other-cookie=value'); + expect(mockRequest.cookies).toEqual({ + 'n8n-oauth-sessions': 'abc123', + 'other-cookie': 'value', + }); + }); }); // The chat endpoints skip sanitizing for their own node type, so the hosted page diff --git a/packages/cli/src/webhooks/webhook-request-sanitizer.ts b/packages/cli/src/webhooks/webhook-request-sanitizer.ts index 3b5c81f7dcd..6e0bb5dbaf8 100644 --- a/packages/cli/src/webhooks/webhook-request-sanitizer.ts +++ b/packages/cli/src/webhooks/webhook-request-sanitizer.ts @@ -9,7 +9,7 @@ import { OIDC_NONCE_COOKIE_NAME, OIDC_STATE_COOKIE_NAME, } from '@/constants'; -import { OAUTH_SESSION_COOKIE_NAME } from '@/modules/oauth-server/oauth-session.service'; +import { OAUTH_SESSION_COOKIE_PREFIX } from '@/modules/oauth-server/oauth-session.service'; import { OIDC_ID_TOKEN_COOKIE_NAME } from '@/modules/sso-oidc/constants'; import { OAUTH_BINDING_COOKIE_NAME } from '@/oauth/oauth-browser-binding.service'; @@ -31,7 +31,9 @@ const BROWSER_ID_COOKIE_NAME = 'n8n-browserId'; const DISALLOWED_COOKIES = new Set([ AUTH_COOKIE_NAME, BROWSER_ID_COOKIE_NAME, - OAUTH_SESSION_COOKIE_NAME, + // The OAuth authorization session used to be one cookie with this exact + // name. A browser can still hold one for the 10 minutes after an upgrade. + 'n8n-oauth-session', OAUTH_BINDING_COOKIE_NAME, OIDC_ID_TOKEN_COOKIE_NAME, OIDC_STATE_COOKIE_NAME, @@ -44,9 +46,13 @@ const DISALLOWED_COOKIES = new Set([ // The form auth cookie's name appends the workflow or execution it was minted // for (`-…`), so it is matched by prefix rather than listed above. The // separator is required so an unrelated cookie that merely begins with the -// prefix (e.g. `n8n-form-authentic`) passes through untouched. +// prefix (e.g. `n8n-form-authentic`) passes through untouched. The OAuth +// authorization session appends its flow id the same way, and its prefix +// already ends in the separator. const isDisallowedCookie = (name: string) => - DISALLOWED_COOKIES.has(name) || name.startsWith(`${FORM_AUTH_COOKIE_PREFIX}-`); + DISALLOWED_COOKIES.has(name) || + name.startsWith(`${FORM_AUTH_COOKIE_PREFIX}-`) || + name.startsWith(OAUTH_SESSION_COOKIE_PREFIX); /** * Removes a cookie with the given name from the request header diff --git a/packages/cli/test/integration/public-api/oauth-consent-scopes.test.ts b/packages/cli/test/integration/public-api/oauth-consent-scopes.test.ts index c84e035cf01..90d49a8f4ed 100644 --- a/packages/cli/test/integration/public-api/oauth-consent-scopes.test.ts +++ b/packages/cli/test/integration/public-api/oauth-consent-scopes.test.ts @@ -16,6 +16,10 @@ import { PublicApiProtectedResource } from '@/modules/inbound-auth-core/public-a import { TrustedSourceDiscoveryService } from '@/modules/inbound-auth-core/trusted-source-discovery.service'; import { TrustedSourceDbStore } from '@/modules/inbound-auth-core/trusted-source.store'; import { OAuthClientRepository } from '@/modules/oauth-server/database/repositories/oauth-client.repository'; +import { + sessionCookieFor, + TEST_FLOW_ID, +} from '@/modules/oauth-server/__tests__/oauth-flow-test-utils'; import type { OAuthSessionPayload } from '@/modules/oauth-server/oauth-session.service'; import { createMember, createOwner } from '@test-integration/db/users'; import { setupTestServer } from '@test-integration/utils'; @@ -105,8 +109,8 @@ async function consentAndExchange(user: User, scopes: string[], requestedScopes? const approval = await testServer .authAgentFor(user) .post('/consent/approve') - .set('Cookie', `n8n-oauth-session=${session}`) - .send({ approved: true, scopes }); + .set('Cookie', sessionCookieFor(TEST_FLOW_ID, session)) + .send({ approved: true, scopes, flow: TEST_FLOW_ID }); if (approval.statusCode !== 200) return { approval, token: undefined }; const code = new URL(approval.body.data.redirectUrl as string).searchParams.get('code'); diff --git a/packages/frontend/@n8n/rest-api-client/src/api/consent.ts b/packages/frontend/@n8n/rest-api-client/src/api/consent.ts index c06aae3fc42..7fef5c9fb98 100644 --- a/packages/frontend/@n8n/rest-api-client/src/api/consent.ts +++ b/packages/frontend/@n8n/rest-api-client/src/api/consent.ts @@ -42,14 +42,23 @@ export interface ConsentApprovalResponse { redirectUrl: string; } -export async function getConsentDetails(context: IRestApiContext): Promise { - return await makeRestApiRequest(context, 'GET', '/consent/details'); +/** + * `flow` is the id the /authorize redirect put in this page's URL. It names the + * pending authorization request, so the screen reads and decides the one + * request it was opened for, even when the browser holds several. + */ +export async function getConsentDetails( + context: IRestApiContext, + flow: string, +): Promise { + return await makeRestApiRequest(context, 'GET', '/consent/details', { flow }); } export async function approveConsent( context: IRestApiContext, + flow: string, approved: boolean, scopes?: string[], ): Promise { - return await makeRestApiRequest(context, 'POST', '/consent/approve', { approved, scopes }); + return await makeRestApiRequest(context, 'POST', '/consent/approve', { flow, approved, scopes }); } diff --git a/packages/frontend/editor-ui/src/app/stores/consent.store.test.ts b/packages/frontend/editor-ui/src/app/stores/consent.store.test.ts index 186aeb8e821..6e5f08c0bb4 100644 --- a/packages/frontend/editor-ui/src/app/stores/consent.store.test.ts +++ b/packages/frontend/editor-ui/src/app/stores/consent.store.test.ts @@ -19,6 +19,9 @@ vi.mock('@n8n/stores/useRootStore', () => ({ })), })); +/** Id of the pending authorization request the screen was opened for. */ +const FLOW_ID = 'ktest00-0123456789abcdef01234567'; + describe('useConsentStore', () => { let consentStore: ReturnType; @@ -32,9 +35,10 @@ describe('useConsentStore', () => { const details = { clientName: 'Test Client', clientId: 'client-1', scopes: [] }; getConsentDetails.mockResolvedValue(details); - const result = await consentStore.fetchConsentDetails(); + const result = await consentStore.fetchConsentDetails(FLOW_ID); expect(result).toEqual(details); + expect(getConsentDetails).toHaveBeenCalledWith(expect.anything(), FLOW_ID); expect(consentStore.consentDetails).toEqual(details); expect(consentStore.isLoading).toBe(false); }); @@ -42,7 +46,7 @@ describe('useConsentStore', () => { it('sets a resource_unavailable error code on a 422', async () => { getConsentDetails.mockRejectedValue(new ResponseError('gone', { httpStatusCode: 422 })); - await expect(consentStore.fetchConsentDetails()).rejects.toThrow('gone'); + await expect(consentStore.fetchConsentDetails(FLOW_ID)).rejects.toThrow('gone'); expect(consentStore.errorCode).toBe('resource_unavailable'); expect(consentStore.error).toBe('gone'); @@ -59,11 +63,11 @@ describe('useConsentStore', () => { resolveStale = resolve; }), ); - const staleCall = consentStore.fetchConsentDetails(); + const staleCall = consentStore.fetchConsentDetails(FLOW_ID); const freshDetails = { clientName: 'Fresh Client', clientId: 'client-2', scopes: [] }; getConsentDetails.mockResolvedValueOnce(freshDetails); - await consentStore.fetchConsentDetails(); + await consentStore.fetchConsentDetails(FLOW_ID); expect(consentStore.consentDetails).toEqual(freshDetails); @@ -80,11 +84,11 @@ describe('useConsentStore', () => { rejectStale = reject; }), ); - const staleCall = consentStore.fetchConsentDetails().catch(() => {}); + const staleCall = consentStore.fetchConsentDetails(FLOW_ID).catch(() => {}); const freshDetails = { clientName: 'Fresh Client', clientId: 'client-2', scopes: [] }; getConsentDetails.mockResolvedValueOnce(freshDetails); - await consentStore.fetchConsentDetails(); + await consentStore.fetchConsentDetails(FLOW_ID); rejectStale(new Error('stale failure')); await staleCall; diff --git a/packages/frontend/editor-ui/src/app/stores/consent.store.ts b/packages/frontend/editor-ui/src/app/stores/consent.store.ts index 154d935034b..7b4405e6c72 100644 --- a/packages/frontend/editor-ui/src/app/stores/consent.store.ts +++ b/packages/frontend/editor-ui/src/app/stores/consent.store.ts @@ -20,14 +20,14 @@ export const useConsentStore = defineStore(STORES.CONSENT, () => { // the most recently issued one. let latestRequestId = 0; - const fetchConsentDetails = async () => { + const fetchConsentDetails = async (flow: string) => { const requestId = ++latestRequestId; isLoading.value = true; error.value = null; errorCode.value = null; try { - const response = await consentApi.getConsentDetails(rootStore.restApiContext); + const response = await consentApi.getConsentDetails(rootStore.restApiContext, flow); if (requestId !== latestRequestId) return consentDetails.value; consentDetails.value = response; return consentDetails.value; @@ -48,12 +48,17 @@ export const useConsentStore = defineStore(STORES.CONSENT, () => { } }; - const approveConsent = async (approved: boolean, scopes?: string[]) => { + const approveConsent = async (flow: string, approved: boolean, scopes?: string[]) => { isLoading.value = true; error.value = null; try { - const response = await consentApi.approveConsent(rootStore.restApiContext, approved, scopes); + const response = await consentApi.approveConsent( + rootStore.restApiContext, + flow, + approved, + scopes, + ); return response; } catch (err) { error.value = err instanceof Error ? err.message : 'Failed to process consent'; diff --git a/packages/frontend/editor-ui/src/app/views/OAuthConsentView.test.ts b/packages/frontend/editor-ui/src/app/views/OAuthConsentView.test.ts index a7a66ff1e2d..be5422d84b6 100644 --- a/packages/frontend/editor-ui/src/app/views/OAuthConsentView.test.ts +++ b/packages/frontend/editor-ui/src/app/views/OAuthConsentView.test.ts @@ -9,6 +9,14 @@ import { nextTick } from 'vue'; vi.mock('@n8n/rest-api-client/api/consent'); +/** The /authorize redirect puts the pending request's flow id in the page URL. */ +const FLOW_ID = 'ktest00-0123456789abcdef01234567'; + +vi.mock('vue-router', async (importOriginal) => ({ + ...(await importOriginal()), + useRoute: () => ({ query: { flow: FLOW_ID } }), +})); + const renderComponent = createComponentRenderer(OAuthConsentView); let locationHrefSpy: ReturnType; @@ -49,6 +57,15 @@ describe('OAuthConsentView', () => { locationHrefSpy?.mockRestore(); }); + // The URL is the only place the flow id comes from, so the screen has to read + // and show the request the /authorize redirect named. + it('should load the flow named in the URL', async () => { + renderComponent(); + await waitAllPromises(); + + expect(consentStore.fetchConsentDetails).toHaveBeenCalledWith(FLOW_ID); + }); + it('should show the workflow name and hide the permission list when a resource is named', async () => { consentStore.consentDetails = { clientName: 'Test MCP Client', @@ -209,7 +226,7 @@ describe('OAuthConsentView', () => { await userEvent.click(denyButton); await waitAllPromises(); - expect(consentStore.approveConsent).toHaveBeenCalledWith(false); + expect(consentStore.approveConsent).toHaveBeenCalledWith(FLOW_ID, false); expect(window.location.href).toBe(window.BASE_PATH ?? '/'); }); @@ -229,7 +246,7 @@ describe('OAuthConsentView', () => { await userEvent.click(allowButton); await waitAllPromises(); - expect(consentStore.approveConsent).toHaveBeenCalledWith(true, undefined); + expect(consentStore.approveConsent).toHaveBeenCalledWith(FLOW_ID, true, undefined); expect(window.location.href).toBe(redirectUrl); }); @@ -424,7 +441,7 @@ describe('OAuthConsentView', () => { await userEvent.click(getByTestId('consent-allow-button')); await waitAllPromises(); - expect(consentStore.approveConsent).toHaveBeenCalledWith(true, [ + expect(consentStore.approveConsent).toHaveBeenCalledWith(FLOW_ID, true, [ 'workflow:read', 'workflow:write', 'execution:read', diff --git a/packages/frontend/editor-ui/src/app/views/OAuthConsentView.vue b/packages/frontend/editor-ui/src/app/views/OAuthConsentView.vue index 9932d952dcd..7823fc4aaaa 100644 --- a/packages/frontend/editor-ui/src/app/views/OAuthConsentView.vue +++ b/packages/frontend/editor-ui/src/app/views/OAuthConsentView.vue @@ -4,6 +4,7 @@ import { useDocumentTitle } from '@/app/composables/useDocumentTitle'; import { useI18n } from '@n8n/i18n'; import type { BaseTextKey } from '@n8n/i18n'; import { onMounted, onUnmounted, computed, ref, watch } from 'vue'; +import { useRoute } from 'vue-router'; import type { ConsentDetailsPicker } from '@n8n/rest-api-client/api/consent'; import { N8nButton, @@ -23,6 +24,14 @@ import ScopesSelector from '@/app/components/scopes/ScopesSelector.vue'; const consentStore = useConsentStore(); +const route = useRoute(); +// Id of the authorization request this screen was opened for, put in the URL by +// the /authorize redirect. Without it the server has no session to show. +const flowId = computed(() => (typeof route.query.flow === 'string' ? route.query.flow : '')); +// The flow the details on screen belong to. Decisions name this one, not the +// current URL, so the request the user approves is the request they were shown. +const shownFlowId = ref(''); + const i18n = useI18n(); const documentTitle = useDocumentTitle(); const toast = useToast(); @@ -115,6 +124,7 @@ watch( const handleAllow = async () => { try { const response = await consentStore.approveConsent( + shownFlowId.value, true, hasScopes.value ? selectedScopes.value : undefined, ); @@ -133,7 +143,7 @@ const handleAllow = async () => { const handleDeny = async () => { try { - await consentStore.approveConsent(false); + await consentStore.approveConsent(shownFlowId.value, false); telemetry.track('User denied MCP consent', { client_name: clientDetails.value?.clientName, }); @@ -149,11 +159,11 @@ const handleClose = () => { window.location.href = window.BASE_PATH ?? '/'; }; -onMounted(async () => { - documentTitle.set(i18n.baseText('oauth.consentView.title')); +const loadDetails = async (flow: string) => { try { - const details = await consentStore.fetchConsentDetails(); - if (!isActive) return; + const details = await consentStore.fetchConsentDetails(flow); + if (!isActive || flow !== flowId.value) return; + shownFlowId.value = flow; detailsResolved.value = true; if (details?.autoApproved && details.redirectUrl) { autoApprovedRedirect.value = true; @@ -165,10 +175,28 @@ onMounted(async () => { available_scopes_count: availableScopes.value.length, }); } catch (err) { - if (!isActive) return; + if (!isActive || flow !== flowId.value) return; + shownFlowId.value = flow; toast.showError(err, i18n.baseText('oauth.consentView.error.fetchDetails')); } +}; + +onMounted(() => { + documentTitle.set(i18n.baseText('oauth.consentView.title')); }); + +// A second /authorize can land on this route with a different flow while the +// component stays mounted, which would leave one request on screen and another +// under the buttons. Reload whenever the id changes. +watch( + flowId, + async (flow) => { + detailsResolved.value = false; + shownFlowId.value = ''; + await loadDetails(flow); + }, + { immediate: true }, +);