mirror of
https://github.com/n8n-io/n8n.git
synced 2026-10-29 19:10:43 +08:00
fix(core): Shorten private-credential OAuth authorization links (#32584)
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
811a4dfe83
commit
c1582a62a0
+123
@@ -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<Request>({ params: { id: 'cred-1' }, query: {} });
|
||||
const res = mock<Response>();
|
||||
|
||||
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<Request>({ params: { id: 'cred-1' }, query: { token: 'gone' } });
|
||||
const res = mock<Response>();
|
||||
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<Request>({ params: { id: 'cred-1' }, query: { token: 'tok' } });
|
||||
const res = mock<Response>();
|
||||
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<Request>({ params: { id: 'cred-1' }, query: { token: 'tok' } });
|
||||
const res = mock<Response>();
|
||||
const mockCredential = mock<CredentialsEntity>({ 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<Request>({ params: { id: 'cred-1' }, query: { token: 'tok' } });
|
||||
const res = mock<Response>();
|
||||
const mockCredential = mock<CredentialsEntity>({ 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<Request>({ params: { id: 'cred-1' }, query: { token: 'tok' } });
|
||||
const res = mock<Response>();
|
||||
const mockCredential = mock<CredentialsEntity>({ 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',
|
||||
|
||||
@@ -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<void> {
|
||||
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
|
||||
*
|
||||
|
||||
+51
@@ -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<CacheService>();
|
||||
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();
|
||||
});
|
||||
});
|
||||
+41
-38
@@ -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<CredentialResolverWorkflowService>;
|
||||
let mockExecutionContextService: jest.Mocked<ExecutionContextService>;
|
||||
let mockOauthService: jest.Mocked<OauthService>;
|
||||
let mockEnterpriseCredentialsService: jest.Mocked<EnterpriseCredentialsService>;
|
||||
let mockAuthorizeIntentService: jest.Mocked<AuthorizeIntentService>;
|
||||
let mockUrlService: jest.Mocked<UrlService>;
|
||||
|
||||
const executionContext: IExecutionContext = {
|
||||
version: 1,
|
||||
@@ -64,20 +67,27 @@ describe('CredentialCheckProxyService', () => {
|
||||
decryptExecutionContext: jest.fn().mockResolvedValue(plaintextContext),
|
||||
} as unknown as jest.Mocked<ExecutionContextService>;
|
||||
|
||||
mockOauthService = {
|
||||
generateAOauth2AuthUri: jest.fn(),
|
||||
generateAOauth1AuthUri: jest.fn(),
|
||||
} as unknown as jest.Mocked<OauthService>;
|
||||
|
||||
mockEnterpriseCredentialsService = {
|
||||
getOne: jest.fn(),
|
||||
} as unknown as jest.Mocked<EnterpriseCredentialsService>;
|
||||
|
||||
mockAuthorizeIntentService = {
|
||||
create: jest.fn().mockResolvedValue('intent-token'),
|
||||
} as unknown as jest.Mocked<AuthorizeIntentService>;
|
||||
|
||||
mockUrlService = {
|
||||
getInstanceBaseUrl: jest.fn().mockReturnValue('http://localhost:5678'),
|
||||
} as unknown as jest.Mocked<UrlService>;
|
||||
|
||||
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 () => {
|
||||
|
||||
@@ -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<string, unknown>;
|
||||
};
|
||||
|
||||
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<string> {
|
||||
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<AuthorizeIntent | undefined> {
|
||||
return await this.cacheService.get<AuthorizeIntent>(this.cacheKey(token));
|
||||
}
|
||||
|
||||
private cacheKey(token: string): string {
|
||||
return `${CACHE_PREFIX}${token}`;
|
||||
}
|
||||
}
|
||||
+24
-20
@@ -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}`;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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';
|
||||
|
||||
@@ -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<CredentialsEntity>({ id: 'credential-id' });
|
||||
credentialsFinderService.findCredentialForUser.mockResolvedValue(mockCredential);
|
||||
const req = mock<AuthenticatedRequest>({
|
||||
user: mock<User>({ 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<AuthenticatedRequest>({
|
||||
@@ -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<string, unknown>;
|
||||
};
|
||||
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),
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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<OauthFlowState | undefined> {
|
||||
return await this.cacheService.get<OauthFlowState>(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<CreateCsrfStateData>(
|
||||
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<CreateCsrfStateData>(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 ?? '',
|
||||
});
|
||||
|
||||
|
||||
@@ -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 = {
|
||||
|
||||
@@ -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<OauthFlowState>(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')
|
||||
|
||||
@@ -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<OauthFlowState>(
|
||||
`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<OauthFlowState>(cacheKey);
|
||||
flowState!.stateData!.userId = sharee.id;
|
||||
await cacheService.set(cacheKey, flowState, MAX_CSRF_AGE);
|
||||
const reencodedState = ownerState;
|
||||
|
||||
nock('https://test.domain')
|
||||
.post('/oauth2/token')
|
||||
|
||||
@@ -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();
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
Reference in New Issue
Block a user