diff --git a/packages/cli/src/modules/dynamic-credentials.ee/__tests__/dynamic-credentials.controller.test.ts b/packages/cli/src/modules/dynamic-credentials.ee/__tests__/dynamic-credentials.controller.test.ts index 4a67943bd3f..ca3f4bca746 100644 --- a/packages/cli/src/modules/dynamic-credentials.ee/__tests__/dynamic-credentials.controller.test.ts +++ b/packages/cli/src/modules/dynamic-credentials.ee/__tests__/dynamic-credentials.controller.test.ts @@ -12,6 +12,7 @@ import { EventService } from '@/events/event.service'; import { OauthService } from '@/oauth/oauth.service'; import { DynamicCredentialResolverRepository } from '@/modules/dynamic-credentials.ee/database/repositories/credential-resolver.repository'; import { + AuthorizeIntentService, CredentialConnectionStatusService, DynamicCredentialResolverRegistry, } from '@/modules/dynamic-credentials.ee/services'; @@ -31,6 +32,7 @@ describe('DynamicCredentialsController', () => { const resolverRegistry = mockInstance(DynamicCredentialResolverRegistry); const dynamicCredentialWebService = mockInstance(DynamicCredentialWebService); const cipher = mockInstance(Cipher); + const authorizeIntentService = mockInstance(AuthorizeIntentService); mockInstance(CredentialsFinderService); mockInstance(CredentialConnectionStatusService); mockInstance(EventService); @@ -325,6 +327,127 @@ describe('DynamicCredentialsController', () => { }); }); + describe('authorizeCredentialRedirect', () => { + it('renders an error and does not redirect when the token is missing', async () => { + const req = mock({ params: { id: 'cred-1' }, query: {} }); + const res = mock(); + + await controller.authorizeCredentialRedirect(req, res); + + expect(oauthService.renderCallbackError).toHaveBeenCalledWith( + res, + 'Missing authorization token.', + ); + expect(res.redirect).not.toHaveBeenCalled(); + }); + + it('renders an error when the intent is expired or unknown', async () => { + const req = mock({ params: { id: 'cred-1' }, query: { token: 'gone' } }); + const res = mock(); + authorizeIntentService.get.mockResolvedValue(undefined); + + await controller.authorizeCredentialRedirect(req, res); + + expect(oauthService.renderCallbackError).toHaveBeenCalledWith( + res, + 'This authorization link is invalid or has expired. Please request a new one.', + ); + expect(res.redirect).not.toHaveBeenCalled(); + }); + + it('renders an error when the intent credential does not match the path id', async () => { + const req = mock({ params: { id: 'cred-1' }, query: { token: 'tok' } }); + const res = mock(); + authorizeIntentService.get.mockResolvedValue({ + credentialId: 'a-different-credential', + resolverId: 'resolver-123', + identity: 'token123', + metadata: {}, + }); + + await controller.authorizeCredentialRedirect(req, res); + + expect(oauthService.renderCallbackError).toHaveBeenCalled(); + expect(res.redirect).not.toHaveBeenCalled(); + }); + + it('materializes the OAuth2 flow and redirects to the provider for a valid intent', async () => { + const req = mock({ params: { id: 'cred-1' }, query: { token: 'tok' } }); + const res = mock(); + const mockCredential = mock({ id: 'cred-1', type: 'googleOAuth2Api' }); + + authorizeIntentService.get.mockResolvedValue({ + credentialId: 'cred-1', + resolverId: 'resolver-123', + identity: 'bearer-jwt', + metadata: { source: 'n8n-oauth' }, + }); + enterpriseCredentialsService.getOne.mockResolvedValue(mockCredential); + oauthService.generateAOauth2AuthUri.mockResolvedValue( + 'https://accounts.google.com/o/oauth2/auth?x=1', + ); + + await controller.authorizeCredentialRedirect(req, res); + + expect(oauthService.generateAOauth2AuthUri).toHaveBeenCalledWith( + mockCredential, + expect.objectContaining({ + cid: 'cred-1', + origin: 'dynamic-credential', + authorizationHeader: 'Bearer bearer-jwt', + credentialResolverId: 'resolver-123', + authMetadata: { source: 'n8n-oauth' }, + }), + req, + res, + ); + expect(res.redirect).toHaveBeenCalledWith('https://accounts.google.com/o/oauth2/auth?x=1'); + }); + + it('materializes the OAuth1 flow for an OAuth1 credential', async () => { + const req = mock({ params: { id: 'cred-1' }, query: { token: 'tok' } }); + const res = mock(); + const mockCredential = mock({ id: 'cred-1', type: 'twitterOAuth1Api' }); + + authorizeIntentService.get.mockResolvedValue({ + credentialId: 'cred-1', + resolverId: 'resolver-123', + identity: 'bearer-jwt', + metadata: {}, + }); + enterpriseCredentialsService.getOne.mockResolvedValue(mockCredential); + oauthService.generateAOauth1AuthUri.mockResolvedValue( + 'https://api.twitter.com/oauth/authorize?x=1', + ); + + await controller.authorizeCredentialRedirect(req, res); + + expect(oauthService.generateAOauth1AuthUri).toHaveBeenCalled(); + expect(oauthService.generateAOauth2AuthUri).not.toHaveBeenCalled(); + expect(res.redirect).toHaveBeenCalledWith('https://api.twitter.com/oauth/authorize?x=1'); + }); + + it('renders an error when materializing the provider URL fails', async () => { + const req = mock({ params: { id: 'cred-1' }, query: { token: 'tok' } }); + const res = mock(); + const mockCredential = mock({ id: 'cred-1', type: 'googleOAuth2Api' }); + + authorizeIntentService.get.mockResolvedValue({ + credentialId: 'cred-1', + resolverId: 'resolver-123', + identity: 'bearer-jwt', + metadata: {}, + }); + enterpriseCredentialsService.getOne.mockResolvedValue(mockCredential); + oauthService.generateAOauth2AuthUri.mockRejectedValue(new Error('discovery failed')); + + await controller.authorizeCredentialRedirect(req, res); + + expect(oauthService.renderCallbackError).toHaveBeenCalledWith(res, 'discovery failed'); + expect(res.redirect).not.toHaveBeenCalled(); + }); + }); + describe('revokeCredential', () => { const mockResolverEntity: DynamicCredentialResolver = { id: 'resolver-123', diff --git a/packages/cli/src/modules/dynamic-credentials.ee/dynamic-credentials.controller.ts b/packages/cli/src/modules/dynamic-credentials.ee/dynamic-credentials.controller.ts index cebc7d7a506..6fdccc9e887 100644 --- a/packages/cli/src/modules/dynamic-credentials.ee/dynamic-credentials.controller.ts +++ b/packages/cli/src/modules/dynamic-credentials.ee/dynamic-credentials.controller.ts @@ -1,11 +1,11 @@ import { Time } from '@n8n/constants'; -import { Delete, Options, Param, Post, RestController } from '@n8n/decorators'; +import { Delete, Get, Options, Param, Post, RestController } from '@n8n/decorators'; import { CredentialsEntity, AuthenticatedRequest, isAuthenticatedRequest, User } from '@n8n/db'; import type { Scope } from '@n8n/permissions'; import { Container } from '@n8n/di'; import { Request, Response } from 'express'; import { Cipher } from 'n8n-core'; -import { jsonParse } from 'n8n-workflow'; +import { ensureError, jsonParse } from 'n8n-workflow'; import { CredentialsFinderService } from '@/credentials/credentials-finder.service'; import { EnterpriseCredentialsService } from '@/credentials/credentials.service.ee'; @@ -16,7 +16,11 @@ import { CreateCsrfStateData, OauthService } from '@/oauth/oauth.service'; import { DynamicCredentialResolverRepository } from './database/repositories/credential-resolver.repository'; import { DynamicCredentialsConfig } from './dynamic-credentials.config'; -import { CredentialConnectionStatusService, DynamicCredentialResolverRegistry } from './services'; +import { + AuthorizeIntentService, + CredentialConnectionStatusService, + DynamicCredentialResolverRegistry, +} from './services'; import { DynamicCredentialCorsService } from './services/dynamic-credential-cors.service'; import { DynamicCredentialWebService } from './services/dynamic-credential-web.service'; import { getDynamicCredentialMiddlewares } from './utils'; @@ -36,6 +40,7 @@ export class DynamicCredentialsController { private readonly credentialsFinderService: CredentialsFinderService, private readonly credentialConnectionStatusService: CredentialConnectionStatusService, private readonly eventService: EventService, + private readonly authorizeIntentService: AuthorizeIntentService, ) {} private async findCredentialToUse( @@ -186,6 +191,62 @@ export class DynamicCredentialsController { throw new BadRequestError('Credential type not supported'); } + /** + * GET /credentials/:id/authorize?token=... + * + * Browser-clickable counterpart to the POST authorize flow. A credential gate hands + * back this short link instead of a large provider authorization URL; opening it + * materializes the OAuth flow (deferred discovery / client registration happens here) + * and redirects to the provider. The unguessable, short-lived `token` is the + * authorization — like the OAuth callback, no session or endpoint auth token is + * required, since the caller identity was captured server-side when the link was + * issued. + */ + @Get('/:id/authorize', { + allowUnauthenticated: true, + usesTemplates: true, + ipRateLimit: { + limit: dynamicCredentialsConfig.rateLimitAuthorizePerMinute, + windowMs: 1 * Time.minutes.toMilliseconds, + }, + }) + async authorizeCredentialRedirect(req: Request, res: Response): Promise { + const token = typeof req.query.token === 'string' ? req.query.token : undefined; + if (!token) { + this.oauthService.renderCallbackError(res, 'Missing authorization token.'); + return; + } + + const intent = await this.authorizeIntentService.get(token); + if (!intent || intent.credentialId !== req.params.id) { + this.oauthService.renderCallbackError( + res, + 'This authorization link is invalid or has expired. Please request a new one.', + ); + return; + } + + try { + const credential = await this.findCredentialToUse(intent.credentialId); + + const csrfData: CreateCsrfStateData = { + cid: credential.id, + origin: 'dynamic-credential', + authorizationHeader: intent.identity ? `Bearer ${intent.identity}` : '', + authMetadata: intent.metadata, + credentialResolverId: intent.resolverId, + }; + + const authorizationUrl = credential.type.toLowerCase().includes('oauth2') + ? await this.oauthService.generateAOauth2AuthUri(credential, csrfData, req, res) + : await this.oauthService.generateAOauth1AuthUri(credential, csrfData, req, res); + + res.redirect(authorizationUrl); + } catch (e) { + this.oauthService.renderCallbackError(res, ensureError(e).message); + } + } + /** * DELETE /credentials/:credentialId/my-connection * diff --git a/packages/cli/src/modules/dynamic-credentials.ee/services/__tests__/authorize-intent.service.test.ts b/packages/cli/src/modules/dynamic-credentials.ee/services/__tests__/authorize-intent.service.test.ts new file mode 100644 index 00000000000..21c4d32492b --- /dev/null +++ b/packages/cli/src/modules/dynamic-credentials.ee/services/__tests__/authorize-intent.service.test.ts @@ -0,0 +1,51 @@ +import { mock } from 'jest-mock-extended'; + +import type { CacheService } from '@/services/cache/cache.service'; + +import { AuthorizeIntentService, type AuthorizeIntent } from '../authorize-intent.service'; + +describe('AuthorizeIntentService', () => { + const cacheService = mock(); + const service = new AuthorizeIntentService(cacheService); + + const intent: AuthorizeIntent = { + credentialId: 'cred-1', + resolverId: 'resolver-1', + identity: 'bearer-jwt', + metadata: { source: 'n8n-oauth' }, + }; + + beforeEach(() => jest.clearAllMocks()); + + it('stores the intent under a prefixed key and returns an opaque token', async () => { + const token = await service.create(intent); + + expect(typeof token).toBe('string'); + expect(token.length).toBeGreaterThan(20); + // URL-safe (base64url) so it can ride in a query string without encoding. + expect(token).toMatch(/^[A-Za-z0-9_-]+$/); + expect(cacheService.set).toHaveBeenCalledWith( + `dynamic-credentials:authorize-intent:${token}`, + intent, + expect.any(Number), + ); + }); + + it('reads the intent back without deleting it (retryable within TTL)', async () => { + cacheService.get.mockResolvedValue(intent); + + const result = await service.get('some-token'); + + expect(result).toBe(intent); + expect(cacheService.get).toHaveBeenCalledWith( + 'dynamic-credentials:authorize-intent:some-token', + ); + expect(cacheService.delete).not.toHaveBeenCalled(); + }); + + it('returns undefined for an unknown or expired token', async () => { + cacheService.get.mockResolvedValue(undefined); + + expect(await service.get('gone')).toBeUndefined(); + }); +}); diff --git a/packages/cli/src/modules/dynamic-credentials.ee/services/__tests__/credential-check-proxy.service.test.ts b/packages/cli/src/modules/dynamic-credentials.ee/services/__tests__/credential-check-proxy.service.test.ts index 27f016d9ca4..2a65224fcbb 100644 --- a/packages/cli/src/modules/dynamic-credentials.ee/services/__tests__/credential-check-proxy.service.test.ts +++ b/packages/cli/src/modules/dynamic-credentials.ee/services/__tests__/credential-check-proxy.service.test.ts @@ -1,10 +1,12 @@ +import type { GlobalConfig } from '@n8n/config'; import type { IExecutionContext, PlaintextExecutionContext } from 'n8n-workflow'; import type { EnterpriseCredentialsService } from '@/credentials/credentials.service.ee'; -import type { OauthService } from '@/oauth/oauth.service'; +import type { UrlService } from '@/services/url.service'; import type { ExecutionContextService } from 'n8n-core'; import { CredentialsEntity } from '@n8n/db'; +import type { AuthorizeIntentService } from '../authorize-intent.service'; import type { CredentialResolverWorkflowService } from '../credential-resolver-workflow.service'; import { CredentialCheckProxyService } from '../credential-check-proxy.service'; @@ -32,8 +34,9 @@ describe('CredentialCheckProxyService', () => { let service: CredentialCheckProxyService; let mockCredentialResolverWorkflowService: jest.Mocked; let mockExecutionContextService: jest.Mocked; - let mockOauthService: jest.Mocked; let mockEnterpriseCredentialsService: jest.Mocked; + let mockAuthorizeIntentService: jest.Mocked; + let mockUrlService: jest.Mocked; const executionContext: IExecutionContext = { version: 1, @@ -64,20 +67,27 @@ describe('CredentialCheckProxyService', () => { decryptExecutionContext: jest.fn().mockResolvedValue(plaintextContext), } as unknown as jest.Mocked; - mockOauthService = { - generateAOauth2AuthUri: jest.fn(), - generateAOauth1AuthUri: jest.fn(), - } as unknown as jest.Mocked; - mockEnterpriseCredentialsService = { getOne: jest.fn(), } as unknown as jest.Mocked; + mockAuthorizeIntentService = { + create: jest.fn().mockResolvedValue('intent-token'), + } as unknown as jest.Mocked; + + mockUrlService = { + getInstanceBaseUrl: jest.fn().mockReturnValue('http://localhost:5678'), + } as unknown as jest.Mocked; + + const globalConfig = { endpoints: { rest: 'rest' } } as unknown as GlobalConfig; + service = new CredentialCheckProxyService( mockCredentialResolverWorkflowService, mockExecutionContextService, - mockOauthService, mockEnterpriseCredentialsService, + mockAuthorizeIntentService, + mockUrlService, + globalConfig, ); }); @@ -99,9 +109,10 @@ describe('CredentialCheckProxyService', () => { expect(result.credentials).toHaveLength(1); expect(result.credentials[0].status).toBe('configured'); expect(result.credentials[0].authorizationUrl).toBeUndefined(); + expect(mockAuthorizeIntentService.create).not.toHaveBeenCalled(); }); - it('should return readyToExecute:false with OAuth URLs when credentials are missing', async () => { + it('should return a short authorize link and capture an intent when credentials are missing', async () => { mockCredentialResolverWorkflowService.getWorkflowStatus.mockResolvedValue([ { credentialId: 'cred-1', @@ -114,9 +125,6 @@ describe('CredentialCheckProxyService', () => { const mockCredential = createMockCredentialEntity({ id: 'cred-1', type: 'oauth2Api' }); mockEnterpriseCredentialsService.getOne.mockResolvedValue(mockCredential); - mockOauthService.generateAOauth2AuthUri.mockResolvedValue( - 'https://accounts.google.com/o/oauth2/auth?...', - ); const result = await service.checkCredentialStatus('workflow-1', executionContext); @@ -124,19 +132,19 @@ describe('CredentialCheckProxyService', () => { expect(result.credentials).toHaveLength(1); expect(result.credentials[0].status).toBe('missing'); expect(result.credentials[0].authorizationUrl).toBe( - 'https://accounts.google.com/o/oauth2/auth?...', - ); - expect(mockOauthService.generateAOauth2AuthUri).toHaveBeenCalledWith( - mockCredential, - expect.objectContaining({ - cid: 'cred-1', - origin: 'dynamic-credential', - credentialResolverId: 'resolver-1', - }), + 'http://localhost:5678/rest/credentials/cred-1/authorize?token=intent-token', ); + // The provider URL is built lazily at click-time, so the intent carries the + // caller identity rather than a fully-formed authorization URL. + expect(mockAuthorizeIntentService.create).toHaveBeenCalledWith({ + credentialId: 'cred-1', + resolverId: 'resolver-1', + identity: 'token-123', + metadata: {}, + }); }); - it('should generate OAuth1 URL for OAuth1 credentials', async () => { + it('should also return a short link for OAuth1 credentials', async () => { mockCredentialResolverWorkflowService.getWorkflowStatus.mockResolvedValue([ { credentialId: 'cred-1', @@ -152,17 +160,13 @@ describe('CredentialCheckProxyService', () => { type: 'twitterOAuth1Api', }); mockEnterpriseCredentialsService.getOne.mockResolvedValue(mockCredential); - mockOauthService.generateAOauth1AuthUri.mockResolvedValue( - 'https://api.twitter.com/oauth/authorize?...', - ); const result = await service.checkCredentialStatus('workflow-1', executionContext); expect(result.credentials[0].authorizationUrl).toBe( - 'https://api.twitter.com/oauth/authorize?...', + 'http://localhost:5678/rest/credentials/cred-1/authorize?token=intent-token', ); - expect(mockOauthService.generateAOauth1AuthUri).toHaveBeenCalled(); - expect(mockOauthService.generateAOauth2AuthUri).not.toHaveBeenCalled(); + expect(mockAuthorizeIntentService.create).toHaveBeenCalledTimes(1); }); it('should throw when no credential context in execution context', async () => { @@ -198,7 +202,6 @@ describe('CredentialCheckProxyService', () => { const mockCredential = createMockCredentialEntity({ id: 'cred-2', type: 'oauth2Api' }); mockEnterpriseCredentialsService.getOne.mockResolvedValue(mockCredential); - mockOauthService.generateAOauth2AuthUri.mockResolvedValue('https://auth.example.com'); const result = await service.checkCredentialStatus('workflow-1', executionContext); @@ -207,7 +210,9 @@ describe('CredentialCheckProxyService', () => { expect(result.credentials[0].status).toBe('configured'); expect(result.credentials[0].authorizationUrl).toBeUndefined(); expect(result.credentials[1].status).toBe('missing'); - expect(result.credentials[1].authorizationUrl).toBe('https://auth.example.com'); + expect(result.credentials[1].authorizationUrl).toBe( + 'http://localhost:5678/rest/credentials/cred-2/authorize?token=intent-token', + ); }); it('should return undefined authorizationUrl when credential is not found', async () => { @@ -227,9 +232,10 @@ describe('CredentialCheckProxyService', () => { expect(result.readyToExecute).toBe(false); expect(result.credentials[0].authorizationUrl).toBeUndefined(); + expect(mockAuthorizeIntentService.create).not.toHaveBeenCalled(); }); - it('should pass empty authorizationHeader when identity is missing', async () => { + it('should capture an empty identity in the intent when identity is missing', async () => { mockExecutionContextService.decryptExecutionContext.mockResolvedValue({ version: 1, establishedAt: Date.now(), @@ -252,17 +258,15 @@ describe('CredentialCheckProxyService', () => { const mockCredential = createMockCredentialEntity({ id: 'cred-1', type: 'oauth2Api' }); mockEnterpriseCredentialsService.getOne.mockResolvedValue(mockCredential); - mockOauthService.generateAOauth2AuthUri.mockResolvedValue('https://auth.example.com'); await service.checkCredentialStatus('workflow-1', executionContext); - expect(mockOauthService.generateAOauth2AuthUri).toHaveBeenCalledWith( - mockCredential, - expect.objectContaining({ authorizationHeader: '' }), + expect(mockAuthorizeIntentService.create).toHaveBeenCalledWith( + expect.objectContaining({ identity: '' }), ); }); - it('should not generate authorizationUrl for non-OAuth credential types', async () => { + it('should not generate an authorize link for non-OAuth credential types', async () => { mockCredentialResolverWorkflowService.getWorkflowStatus.mockResolvedValue([ { credentialId: 'cred-1', @@ -279,8 +283,7 @@ describe('CredentialCheckProxyService', () => { const result = await service.checkCredentialStatus('workflow-1', executionContext); expect(result.credentials[0].authorizationUrl).toBeUndefined(); - expect(mockOauthService.generateAOauth2AuthUri).not.toHaveBeenCalled(); - expect(mockOauthService.generateAOauth1AuthUri).not.toHaveBeenCalled(); + expect(mockAuthorizeIntentService.create).not.toHaveBeenCalled(); }); it('should return readyToExecute:true for empty credentials list', async () => { diff --git a/packages/cli/src/modules/dynamic-credentials.ee/services/authorize-intent.service.ts b/packages/cli/src/modules/dynamic-credentials.ee/services/authorize-intent.service.ts new file mode 100644 index 00000000000..1067ada8b6b --- /dev/null +++ b/packages/cli/src/modules/dynamic-credentials.ee/services/authorize-intent.service.ts @@ -0,0 +1,49 @@ +import { Service } from '@n8n/di'; +import { randomBytes } from 'node:crypto'; + +import { MAX_CSRF_AGE } from '@/oauth/types'; +import { CacheService } from '@/services/cache/cache.service'; + +/** + * A pending dynamic-credential authorization, captured at credential-gate time and + * materialized into a real OAuth flow only when the user opens the short link. Holds + * just enough to rebuild the flow at click-time — notably the caller's identity, so + * the resolved credential is bound to the right subject regardless of who clicks. + */ +export type AuthorizeIntent = { + credentialId: string; + resolverId: string; + /** Caller identity (the bearer/subject) the credential connection must be bound to. */ + identity: string; + metadata?: Record; +}; + +const CACHE_PREFIX = 'dynamic-credentials:authorize-intent:'; + +/** + * Stores short-lived "authorize intents" in the shared cache so a credential gate can + * hand back a tiny n8n link (`/credentials/:id/authorize?token=…`) instead of a large + * provider authorization URL. The cache is shared across mains, so the link can be + * opened on a different main than the one that issued it. + */ +@Service() +export class AuthorizeIntentService { + constructor(private readonly cacheService: CacheService) {} + + async create(intent: AuthorizeIntent): Promise { + const token = randomBytes(32).toString('base64url'); + // Matches the OAuth CSRF state lifetime: the link is only useful for the + // duration of a handshake, and a short window limits the value of a leaked token. + await this.cacheService.set(this.cacheKey(token), intent, MAX_CSRF_AGE); + return token; + } + + /** Read an intent without consuming it, so a failed click can be retried within the TTL. */ + async get(token: string): Promise { + return await this.cacheService.get(this.cacheKey(token)); + } + + private cacheKey(token: string): string { + return `${CACHE_PREFIX}${token}`; + } +} diff --git a/packages/cli/src/modules/dynamic-credentials.ee/services/credential-check-proxy.service.ts b/packages/cli/src/modules/dynamic-credentials.ee/services/credential-check-proxy.service.ts index d36310e63f4..b0bffe24648 100644 --- a/packages/cli/src/modules/dynamic-credentials.ee/services/credential-check-proxy.service.ts +++ b/packages/cli/src/modules/dynamic-credentials.ee/services/credential-check-proxy.service.ts @@ -1,4 +1,4 @@ -import { CredentialsEntity } from '@n8n/db'; +import { GlobalConfig } from '@n8n/config'; import { Service } from '@n8n/di'; import type { CredentialCheckResult, @@ -8,9 +8,10 @@ import type { } from 'n8n-workflow'; import { EnterpriseCredentialsService } from '@/credentials/credentials.service.ee'; -import { CreateCsrfStateData, OauthService } from '@/oauth/oauth.service'; +import { UrlService } from '@/services/url.service'; import { ExecutionContextService } from 'n8n-core'; +import { AuthorizeIntentService } from './authorize-intent.service'; import { CredentialResolverWorkflowService } from './credential-resolver-workflow.service'; @Service() @@ -18,8 +19,10 @@ export class CredentialCheckProxyService implements DynamicCredentialCheckProxyP constructor( private readonly credentialResolverWorkflowService: CredentialResolverWorkflowService, private readonly executionContextService: ExecutionContextService, - private readonly oauthService: OauthService, private readonly enterpriseCredentialsService: EnterpriseCredentialsService, + private readonly authorizeIntentService: AuthorizeIntentService, + private readonly urlService: UrlService, + private readonly globalConfig: GlobalConfig, ) {} async checkCredentialStatus( @@ -66,6 +69,13 @@ export class CredentialCheckProxyService implements DynamicCredentialCheckProxyP return { readyToExecute, credentials }; } + /** + * Returns a short n8n link that, when opened, redirects to the provider's OAuth + * authorization page. The heavy work of building the provider URL (OAuth discovery / + * dynamic client registration) is deferred to click-time, so the gate response stays + * fast and small. The caller identity is captured in a server-side intent so the + * connection binds to the right subject regardless of who opens the link. + */ private async generateAuthorizationUrl( credentialId: string, resolverId: string, @@ -74,24 +84,18 @@ export class CredentialCheckProxyService implements DynamicCredentialCheckProxyP const credential = await this.enterpriseCredentialsService.getOne(credentialId); if (!credential) return undefined; - const csrfData: CreateCsrfStateData = { - cid: credential.id, - origin: 'dynamic-credential', - authorizationHeader: credentialContext.identity ? `Bearer ${credentialContext.identity}` : '', - authMetadata: credentialContext.metadata, - credentialResolverId: resolverId, - }; + const type = credential.type.toLowerCase(); + if (!type.includes('oauth2') && !type.includes('oauth1')) return undefined; - const callerData: [CredentialsEntity, CreateCsrfStateData] = [credential, csrfData]; + const token = await this.authorizeIntentService.create({ + credentialId: credential.id, + resolverId, + identity: credentialContext.identity ?? '', + metadata: credentialContext.metadata, + }); - let authorizationUrl: string | undefined; - - if (credential.type.toLowerCase().includes('oauth2')) { - authorizationUrl = await this.oauthService.generateAOauth2AuthUri(...callerData); - } else if (credential.type.toLowerCase().includes('oauth1')) { - authorizationUrl = await this.oauthService.generateAOauth1AuthUri(...callerData); - } - - return authorizationUrl; + const basePath = this.urlService.getInstanceBaseUrl(); + const restPath = this.globalConfig.endpoints.rest; + return `${basePath}/${restPath}/credentials/${credential.id}/authorize?token=${token}`; } } diff --git a/packages/cli/src/modules/dynamic-credentials.ee/services/index.ts b/packages/cli/src/modules/dynamic-credentials.ee/services/index.ts index 3a0c1c580f8..bc1bbeba0d8 100644 --- a/packages/cli/src/modules/dynamic-credentials.ee/services/index.ts +++ b/packages/cli/src/modules/dynamic-credentials.ee/services/index.ts @@ -4,5 +4,6 @@ export * from './dynamic-credential.service'; export * from './dynamic-credential-storage.service'; export * from './credential-resolver-workflow.service'; export * from './credential-check-proxy.service'; +export * from './authorize-intent.service'; export * from './n8n-resolver-seeder.service'; export * from './credential-connection-status.service'; diff --git a/packages/cli/src/oauth/__tests__/oauth.service.test.ts b/packages/cli/src/oauth/__tests__/oauth.service.test.ts index a43a96003fb..48427088e5a 100644 --- a/packages/cli/src/oauth/__tests__/oauth.service.test.ts +++ b/packages/cli/src/oauth/__tests__/oauth.service.test.ts @@ -100,6 +100,10 @@ describe('OauthService', () => { beforeEach(() => { jest.setSystemTime(new Date(timestamp)); jest.clearAllMocks(); + // clearAllMocks() does not reset implementations set via mockResolvedValue, so + // pin the per-flow cache to "empty" by default. Tests that exercise the cache + // path opt in explicitly; the rest fall back to the legacy URL-encoded state. + cacheService.get.mockResolvedValue(undefined); credentialsHelper.getCredentialsProperties.mockReturnValue([]); globalConfig.endpoints = { rest: 'rest' } as any; @@ -401,55 +405,31 @@ describe('OauthService', () => { }); describe('createCsrfState', () => { - it('should create CSRF state with correct structure', async () => { - const data = { - cid: 'credential-id', - userId: 'user-id', - origin: 'static-credential' as const, - }; + it('should create CSRF state with only the signed token and timestamp', async () => { jest.setSystemTime(new Date(timestamp)); - const [csrfSecret, base64State] = await service.createCsrfState(data); + const [csrfSecret, base64State, stateToken] = await service.createCsrfState(); expect(typeof csrfSecret).toBe('string'); expect(csrfSecret.length).toBeGreaterThan(0); - expect(cipher.encryptV2).toHaveBeenCalled(); + expect(typeof stateToken).toBe('string'); + expect(stateToken.length).toBeGreaterThan(0); // Verify base64State is a valid base64 string expect(typeof base64State).toBe('string'); const base64Decoded = JSON.parse(Buffer.from(base64State, 'base64').toString()); - expect(base64Decoded.token).toBeDefined(); + expect(base64Decoded.token).toBe(stateToken); expect(base64Decoded.createdAt).toBe(timestamp); - expect(base64Decoded.data).toBeDefined(); - - // Decrypt the data field to verify CSRF data - const decryptedData = JSON.parse(await cipher.decryptV2(base64Decoded.data)); - expect(decryptedData.cid).toBe('credential-id'); - expect(decryptedData.userId).toBe('user-id'); - expect(decryptedData.origin).toBe('static-credential'); }); - it('should include additional data in state', async () => { - const data = { - cid: 'credential-id', - customField: 'custom-value', - origin: 'static-credential' as const, - }; - jest.setSystemTime(new Date(timestamp)); + it('does not embed the CSRF payload in the URL state', async () => { + const [, base64State] = await service.createCsrfState(); - const [, base64State] = await service.createCsrfState(data); - - expect(cipher.encryptV2).toHaveBeenCalled(); - - // Verify base64State structure + // The payload now lives server-side in the per-flow cache, so the state + // carries no encrypted blob and never touches the cipher. + expect(cipher.encryptV2).not.toHaveBeenCalled(); const base64Decoded = JSON.parse(Buffer.from(base64State, 'base64').toString()); - expect(base64Decoded.token).toBeDefined(); - expect(base64Decoded.createdAt).toBeDefined(); - expect(base64Decoded.data).toBeDefined(); - - // Decrypt and verify the customField is in the encrypted data - const decryptedData = JSON.parse(await cipher.decryptV2(base64Decoded.data)); - expect(decryptedData.customField).toBe('custom-value'); + expect(base64Decoded.data).toBeUndefined(); }); }); @@ -496,6 +476,36 @@ describe('OauthService', () => { ); }); + it('reads the CSRF payload from the per-flow cache when the URL state carries no blob', async () => { + const csrfData = { + cid: 'credential-id', + userId: 'user-id', + origin: 'static-credential' as const, + }; + // New-style state: token + timestamp only, no encrypted `data` in the URL. + const state = { token: 'token', createdAt: timestamp }; + const encodedState = Buffer.from(JSON.stringify(state)).toString('base64'); + cacheService.get.mockResolvedValue({ csrfSecret: 'secret', stateData: csrfData }); + const mockCredential = mock({ id: 'credential-id' }); + credentialsFinderService.findCredentialForUser.mockResolvedValue(mockCredential); + const req = mock({ + user: mock({ id: 'user-id' }), + }); + + const [decodedState, credential] = await (service as any).decodeCsrfState(encodedState, req); + + // Payload came from the cache, so the cipher is never consulted. + expect(cipher.decryptV2).not.toHaveBeenCalled(); + expect(cacheService.get).toHaveBeenCalledWith('oauth:flow:token'); + expect(decodedState).toMatchObject({ + token: 'token', + cid: 'credential-id', + userId: 'user-id', + origin: 'static-credential', + }); + expect(credential).toBe(mockCredential); + }); + it('should throw error when state format is invalid', async () => { const invalidState = 'not-base64-json'; const req = mock({ @@ -2260,6 +2270,9 @@ describe('OauthService', () => { jest .spyOn(service, 'createCsrfState') .mockResolvedValue(['csrf-secret', 'base64-state', 'state-token']); + const storeOauthFlowState = jest + .spyOn(service, 'storeOauthFlowState') + .mockResolvedValue(undefined); await service.generateAOauth2AuthUri(credential, { cid: credential.id, @@ -2267,10 +2280,12 @@ describe('OauthService', () => { userId: 'user-id', }); - // Verify createCsrfState was called with cid - expect(service.createCsrfState).toHaveBeenCalledWith( + // The CSRF payload (incl. cid) is stashed server-side in the per-flow + // cache instead of being encrypted into the state URL parameter. + expect(storeOauthFlowState).toHaveBeenCalledWith( + 'state-token', expect.objectContaining({ - cid: '1', + stateData: expect.objectContaining({ cid: '1' }), }), ); }); @@ -4309,8 +4324,13 @@ describe('OauthService', () => { const url = new URL(authUri); expect(url.searchParams.get('resource')).toBe('https://mcp.example.com/mcp'); - expect(cipher.encryptV2).toHaveBeenLastCalledWith( - expect.stringContaining('"resource":"https://mcp.example.com/mcp"'), + // The resolved resource is stashed in the per-flow cache payload, not the URL state. + expect(cacheService.set).toHaveBeenLastCalledWith( + expect.any(String), + expect.objectContaining({ + stateData: expect.objectContaining({ resource: 'https://mcp.example.com/mcp' }), + }), + expect.any(Number), ); }); @@ -4330,7 +4350,10 @@ describe('OauthService', () => { }); expect(new URL(authUri).searchParams.has('resource')).toBe(false); - expect(cipher.encryptV2).toHaveBeenLastCalledWith(expect.not.stringContaining('resource')); + const storedFlowState = jest.mocked(cacheService.set).mock.lastCall?.[1] as { + stateData: Record; + }; + expect(storedFlowState.stateData.resource).toBeUndefined(); }); it('should include normalized user-supplied resource URL when it matches discovery', async () => { @@ -4416,8 +4439,12 @@ describe('OauthService', () => { const url = new URL(authUri); expect(url.origin + url.pathname).toBe('https://auth.example.com/oauth2/auth'); expect(url.searchParams.get('resource')).toBe('https://mcp.example.com/mcp'); - expect(cipher.encryptV2).toHaveBeenLastCalledWith( - expect.stringContaining('"resource":"https://mcp.example.com/mcp"'), + expect(cacheService.set).toHaveBeenLastCalledWith( + expect.any(String), + expect.objectContaining({ + stateData: expect.objectContaining({ resource: 'https://mcp.example.com/mcp' }), + }), + expect.any(Number), ); }); }); diff --git a/packages/cli/src/oauth/oauth.service.ts b/packages/cli/src/oauth/oauth.service.ts index 67f7513eb5f..69b717a148d 100644 --- a/packages/cli/src/oauth/oauth.service.ts +++ b/packages/cli/src/oauth/oauth.service.ts @@ -65,6 +65,14 @@ import { Time } from '@n8n/constants'; */ export type OauthFlowState = { csrfSecret: string; + /** + * The CSRF state payload (credential id, origin, user identity, browser + * binding hash, and for dynamic credentials the caller's bearer token). + * Held here instead of encrypted in the `state` URL parameter so the + * authorization URL stays small. Optional so flows started before this + * change (which carried the payload in the URL) still resolve. + */ + stateData?: CreateCsrfStateData; /** OAuth2 PKCE verifier, needed to exchange the code in the callback. */ codeVerifier?: string; /** OAuth1 request-token secret, needed to sign the access-token request in the callback. */ @@ -413,14 +421,19 @@ export class OauthService { return await this.credentialsRepository.findOneBy({ id: credentialId }); } - async createCsrfState(data: CreateCsrfStateData): Promise<[string, string, string]> { + /** + * Mint a CSRF secret and the matching `state` URL parameter. The state carries + * only the signed token and a timestamp — the rest of the flow payload is stashed + * server-side in the per-flow cache (see {@link storeOauthFlowState}) so the + * authorization URL does not balloon with an encrypted blob. + */ + async createCsrfState(): Promise<[string, string, string]> { const token = new Csrf(); const csrfSecret = token.secretSync(); const stateToken = token.create(csrfSecret); const state: CsrfState = { token: stateToken, createdAt: Date.now(), - data: await this.cipher.encryptV2(JSON.stringify(data)), }; const base64State = Buffer.from(JSON.stringify(state)).toString('base64'); @@ -437,6 +450,15 @@ export class OauthService { await this.cacheService.set(this.oauthFlowCacheKey(stateToken), flowState, MAX_CSRF_AGE); } + /** + * Read the per-flow OAuth state without consuming it. Used while decoding the + * callback `state` to recover the payload that used to live in the URL; the + * entry is still consumed (deleted) later by {@link consumeOauthFlowState}. + */ + protected async peekOauthFlowState(stateToken: string): Promise { + return await this.cacheService.get(this.oauthFlowCacheKey(stateToken)); + } + /** * Read and consume the per-flow OAuth state. Consume-once: the entry is deleted * after a successful read to prevent replay. @@ -459,12 +481,25 @@ export class OauthService { errorMessage, }); - const decryptedState = jsonParse( - await this.cipher.decryptV2(decoded.data), - { - errorMessage, - }, - ); + // The CSRF state payload now lives server-side in the per-flow cache (keyed + // by the state token). Fall back to the encrypted URL blob for flows that + // were initiated before this change and are still in flight. + const flowState = + typeof decoded.token === 'string' ? await this.peekOauthFlowState(decoded.token) : undefined; + const decryptedState = + flowState?.stateData ?? + (typeof decoded.data === 'string' + ? jsonParse(await this.cipher.decryptV2(decoded.data), { + errorMessage, + }) + : undefined); + + // A parseable token with no recoverable payload means the per-flow entry is + // gone — the flow was already consumed (replay) or expired out of the cache. + // Surface that as an invalid callback state rather than a generic format error. + if (!decryptedState) { + throw new UnexpectedError('The OAuth callback state is invalid!'); + } if (typeof decryptedState.cid !== 'string' || typeof decoded.token !== 'string') { throw new UnexpectedError(errorMessage); @@ -835,7 +870,7 @@ export class OauthService { this.validateOAuthUrlOrThrow(oauthCredentials.accessTokenUrl ?? ''); // Generate a CSRF prevention token and send it as an OAuth2 state string - const [csrfSecret, state, stateToken] = await this.createCsrfState(csrfData); + const [csrfSecret, state, stateToken] = await this.createCsrfState(); const oAuthOptions = { ...this.convertCredentialToOptions(oauthCredentials), @@ -848,7 +883,7 @@ export class OauthService { await this.externalHooks.run('oauth2.authenticate', [oAuthOptions]); - const flowState: OauthFlowState = { csrfSecret }; + const flowState: OauthFlowState = { csrfSecret, stateData: csrfData }; if (oauthCredentials.grantType === 'pkce') { const { code_verifier, code_challenge } = await pkceChallenge(); oAuthOptions.query = { @@ -1030,7 +1065,7 @@ export class OauthService { this.validateOAuthUrlOrThrow(oauthCredentials.requestTokenUrl ?? ''); this.validateOAuthUrlOrThrow(oauthCredentials.accessTokenUrl ?? ''); - const [csrfSecret, state, stateToken] = await this.createCsrfState(csrfData); + const [csrfSecret, state, stateToken] = await this.createCsrfState(); const signatureMethod = oauthCredentials.signatureMethod; @@ -1096,6 +1131,7 @@ export class OauthService { // concurrent flows by different users don't clobber each other's secret. await this.storeOauthFlowState(stateToken, { csrfSecret, + stateData: csrfData, oauthTokenSecret: responseJson.oauth_token_secret ?? '', }); diff --git a/packages/cli/src/oauth/types.ts b/packages/cli/src/oauth/types.ts index 76a61bca3c1..eee84232f48 100644 --- a/packages/cli/src/oauth/types.ts +++ b/packages/cli/src/oauth/types.ts @@ -5,8 +5,13 @@ export type CsrfStateRequired = { token: string; /** Creation timestamp of the CSRF state. Used for expiration. */ createdAt: number; - /** Encrypted stringified CSRF state data */ - data: string; + /** + * Encrypted stringified CSRF state data. Legacy field: the payload now lives + * server-side in the per-flow cache (keyed by the state token) instead of in + * the URL, so newly minted states omit it. Kept optional to keep in-flight + * flows started before this change resolvable at the callback. + */ + data?: string; }; export type CreateCsrfStateData = { diff --git a/packages/cli/test/integration/controllers/oauth/oauth1.api.test.ts b/packages/cli/test/integration/controllers/oauth/oauth1.api.test.ts index 48ebb4a02c9..8642b7ae9dc 100644 --- a/packages/cli/test/integration/controllers/oauth/oauth1.api.test.ts +++ b/packages/cli/test/integration/controllers/oauth/oauth1.api.test.ts @@ -5,7 +5,9 @@ import { response as Response } from 'express'; import nock from 'nock'; import { CredentialsHelper } from '@/credentials-helper'; -import { OauthService } from '@/oauth/oauth.service'; +import { OauthService, type OauthFlowState } from '@/oauth/oauth.service'; +import { MAX_CSRF_AGE } from '@/oauth/types'; +import { CacheService } from '@/services/cache/cache.service'; import { decryptCredentialData, getCredentialById, @@ -195,11 +197,10 @@ describe('OAuth1 API', () => { return this; }); - // Build a callback state whose decrypted userId equals the requesting member, - // so the userId equality check inside decodeCsrfState passes and the credential - // scope check is the only remaining gate. The owner-initiated /auth call below - // produces a valid encrypted state; we then re-encrypt its contents with the - // member's userId before driving the callback as the member. + // Make the flow's stored userId equal the requesting member, so the userId + // equality check inside decodeCsrfState passes and the credential scope check + // is the only remaining gate. The CSRF payload lives server-side in the + // per-flow cache now, so we rewrite the cached stateData (rather than the URL). const csrfSpy = jest.spyOn(oauthService, 'createCsrfState').mockClear(); mockRequestTokenEndpoint(); @@ -212,13 +213,12 @@ describe('OAuth1 API', () => { const [, ownerState] = await csrfSpy.mock.results[0].value; const decoded = JSON.parse(Buffer.from(ownerState, 'base64').toString()); - const decryptedData = JSON.parse(oauthService['cipher'].decrypt(decoded.data)) as Record< - string, - unknown - >; - decryptedData.userId = sharee.id; - decoded.data = oauthService['cipher'].encrypt(JSON.stringify(decryptedData)); - const reencodedState = Buffer.from(JSON.stringify(decoded)).toString('base64'); + const cacheService = Container.get(CacheService); + const cacheKey = `oauth:flow:${decoded.token}`; + const flowState = await cacheService.get(cacheKey); + flowState!.stateData!.userId = sharee.id; + await cacheService.set(cacheKey, flowState, MAX_CSRF_AGE); + const reencodedState = ownerState; nock('https://test.domain') .post('/oauth1/access_token') diff --git a/packages/cli/test/integration/controllers/oauth/oauth2.api.test.ts b/packages/cli/test/integration/controllers/oauth/oauth2.api.test.ts index eb579d81a9e..7a46ed30c93 100644 --- a/packages/cli/test/integration/controllers/oauth/oauth2.api.test.ts +++ b/packages/cli/test/integration/controllers/oauth/oauth2.api.test.ts @@ -7,7 +7,9 @@ import { parse as parseQs } from 'querystring'; import { CredentialsHelper } from '@/credentials-helper'; import { ExternalHooks } from '@/external-hooks'; -import { OauthService } from '@/oauth/oauth.service'; +import { OauthService, type OauthFlowState } from '@/oauth/oauth.service'; +import { MAX_CSRF_AGE } from '@/oauth/types'; +import { CacheService } from '@/services/cache/cache.service'; import { decryptCredentialData, getCredentialById, @@ -79,19 +81,20 @@ describe('OAuth2 API', () => { scope: 'openid', }); - // Verify state is base64-encoded and contains expected structure + // Verify state is base64-encoded and contains expected structure. The CSRF + // payload now lives server-side in the per-flow cache, so the URL state carries + // only the signed token and timestamp. expect(queryParams.state).toBeDefined(); const decodedState = JSON.parse(Buffer.from(queryParams.state as string, 'base64').toString()); expect(decodedState).toMatchObject({ token: expect.any(String), createdAt: expect.any(Number), - data: expect.any(String), // Encrypted CSRF data }); + expect(decodedState.data).toBeUndefined(); }); it('should allow external hook to modify oAuthOptions and state', async () => { const externalHooks = Container.get(ExternalHooks); - const oauthService = Container.get(OauthService); // Mock the external hook to modify both redirectUri and state const hookSpy = jest.fn(async function (oAuthOptions) { @@ -129,12 +132,15 @@ describe('OAuth2 API', () => { expect(decodedState.host).toBe('custom.host.com'); expect(decodedState.token).toBeDefined(); expect(decodedState.createdAt).toBeDefined(); - expect(decodedState.data).toBeDefined(); + // The CSRF payload is no longer carried in the URL. + expect(decodedState.data).toBeUndefined(); - // Decrypt the data field and verify original CSRF data is preserved - const decryptedData = JSON.parse(oauthService['cipher'].decrypt(decodedState.data)); - expect(decryptedData.cid).toBe(credential.id); - expect(decryptedData.userId).toBe(owner.id); + // The original CSRF data is preserved server-side in the per-flow cache. + const flowState = await Container.get(CacheService).get( + `oauth:flow:${decodedState.token}`, + ); + expect(flowState?.stateData?.cid).toBe(credential.id); + expect(flowState?.stateData?.userId).toBe(owner.id); }); it('should fail on auth when callback is called as another user', async () => { @@ -439,11 +445,10 @@ describe('OAuth2 API', () => { return this; }); - // Build a callback state whose decrypted userId equals the requesting member, - // so the userId equality check inside decodeCsrfState passes and the credential - // scope check is the only remaining gate. The owner-initiated /auth call below - // produces a valid encrypted state; we then re-encrypt its contents with the - // member's userId before driving the callback as the member. + // Make the flow's stored userId equal the requesting member, so the userId + // equality check inside decodeCsrfState passes and the credential scope check + // is the only remaining gate. The CSRF payload lives server-side in the + // per-flow cache now, so we rewrite the cached stateData (rather than the URL). const ownerAgentForSetup = testServer.authAgentFor(owner); const csrfSpy = jest.spyOn(oauthService, 'createCsrfState').mockClear(); await ownerAgentForSetup @@ -453,13 +458,12 @@ describe('OAuth2 API', () => { const [, ownerState] = await csrfSpy.mock.results[0].value; const decoded = JSON.parse(Buffer.from(ownerState, 'base64').toString()); - const decryptedData = JSON.parse(oauthService['cipher'].decrypt(decoded.data)) as Record< - string, - unknown - >; - decryptedData.userId = sharee.id; - decoded.data = oauthService['cipher'].encrypt(JSON.stringify(decryptedData)); - const reencodedState = Buffer.from(JSON.stringify(decoded)).toString('base64'); + const cacheService = Container.get(CacheService); + const cacheKey = `oauth:flow:${decoded.token}`; + const flowState = await cacheService.get(cacheKey); + flowState!.stateData!.userId = sharee.id; + await cacheService.set(cacheKey, flowState, MAX_CSRF_AGE); + const reencodedState = ownerState; nock('https://test.domain') .post('/oauth2/token') diff --git a/packages/cli/test/integration/controllers/oauth/oauth2.skip-auth.api.test.ts b/packages/cli/test/integration/controllers/oauth/oauth2.skip-auth.api.test.ts index 0fb189ffd4f..1363e91f9fe 100644 --- a/packages/cli/test/integration/controllers/oauth/oauth2.skip-auth.api.test.ts +++ b/packages/cli/test/integration/controllers/oauth/oauth2.skip-auth.api.test.ts @@ -173,7 +173,8 @@ describe('OAuth2 API with skipAuthOnOAuthCallback enabled', () => { scope: 'openid', }); - // Verify state is base64-encoded and contains expected structure + // Verify state is base64-encoded and contains expected structure. The CSRF + // payload now lives server-side in the per-flow cache, not in the URL state. expect(queryParams.state).toBeDefined(); const decodedState = JSON.parse( Buffer.from(queryParams.state as string, 'base64').toString(), @@ -181,8 +182,8 @@ describe('OAuth2 API with skipAuthOnOAuthCallback enabled', () => { expect(decodedState).toMatchObject({ token: expect.any(String), createdAt: expect.any(Number), - data: expect.any(String), // Encrypted CSRF data }); + expect(decodedState.data).toBeUndefined(); }); });