mirror of
https://github.com/n8n-io/n8n
synced 2026-04-21 15:47:20 +00:00
fix(core): Omit empty scope from OAuth2 client credentials token request and improve error messaging (#28159)
This commit is contained in:
parent
aa6c322059
commit
3db52dca22
3 changed files with 255 additions and 2 deletions
|
|
@ -0,0 +1,111 @@
|
|||
import { mockDeep } from 'jest-mock-extended';
|
||||
import type { IAllExecuteFunctions, INode, IWorkflowExecuteAdditionalData } from 'n8n-workflow';
|
||||
|
||||
const mockGetToken = jest.fn();
|
||||
const mockSign = jest.fn();
|
||||
const mockCreateToken = jest.fn();
|
||||
const MockClientOAuth2 = jest.fn();
|
||||
|
||||
jest.mock('@n8n/client-oauth2', () => ({
|
||||
ClientOAuth2: MockClientOAuth2,
|
||||
}));
|
||||
|
||||
import { requestOAuth2 } from '../request-helper-functions';
|
||||
|
||||
describe('createOAuth2Client - scope handling', () => {
|
||||
const mockThis = mockDeep<IAllExecuteFunctions>();
|
||||
const mockNode = mockDeep<INode>();
|
||||
const mockAdditionalData = mockDeep<IWorkflowExecuteAdditionalData>();
|
||||
|
||||
const baseCredentials = {
|
||||
clientId: 'client-id',
|
||||
clientSecret: 'client-secret',
|
||||
grantType: 'clientCredentials',
|
||||
accessTokenUrl: 'https://auth.example.com/token',
|
||||
authentication: 'body',
|
||||
oauthTokenData: undefined,
|
||||
};
|
||||
|
||||
beforeEach(() => {
|
||||
jest.resetAllMocks();
|
||||
mockNode.name = 'test-node';
|
||||
mockNode.credentials = { testOAuth2: { id: 'cred-id', name: 'cred-name' } };
|
||||
|
||||
mockGetToken.mockResolvedValue({ data: { access_token: 'mock-token' } });
|
||||
mockSign.mockImplementation((opts: object) => ({
|
||||
...opts,
|
||||
headers: { Authorization: 'Bearer mock-token' },
|
||||
}));
|
||||
mockCreateToken.mockReturnValue({ sign: mockSign, accessToken: 'mock-token' });
|
||||
|
||||
MockClientOAuth2.mockImplementation(() => ({
|
||||
credentials: { getToken: mockGetToken },
|
||||
createToken: mockCreateToken,
|
||||
}));
|
||||
|
||||
mockThis.helpers.httpRequest.mockResolvedValue({ success: true });
|
||||
});
|
||||
|
||||
const call = async () =>
|
||||
await requestOAuth2.call(
|
||||
mockThis,
|
||||
'testOAuth2',
|
||||
{ method: 'GET', url: 'https://api.example.com/data' },
|
||||
mockNode,
|
||||
mockAdditionalData,
|
||||
undefined,
|
||||
true,
|
||||
);
|
||||
|
||||
test('should pass undefined scopes when scope is undefined', async () => {
|
||||
mockThis.getCredentials.mockResolvedValue({ ...baseCredentials, scope: undefined });
|
||||
|
||||
await call();
|
||||
|
||||
expect(MockClientOAuth2).toHaveBeenCalledWith(expect.objectContaining({ scopes: undefined }));
|
||||
});
|
||||
|
||||
test('should pass undefined scopes when scope is null', async () => {
|
||||
mockThis.getCredentials.mockResolvedValue({ ...baseCredentials, scope: null });
|
||||
|
||||
await call();
|
||||
|
||||
expect(MockClientOAuth2).toHaveBeenCalledWith(expect.objectContaining({ scopes: undefined }));
|
||||
});
|
||||
|
||||
test('should pass undefined scopes when scope is an empty string', async () => {
|
||||
mockThis.getCredentials.mockResolvedValue({ ...baseCredentials, scope: '' });
|
||||
|
||||
await call();
|
||||
|
||||
expect(MockClientOAuth2).toHaveBeenCalledWith(expect.objectContaining({ scopes: undefined }));
|
||||
});
|
||||
|
||||
test('should pass undefined scopes when scope contains only spaces', async () => {
|
||||
mockThis.getCredentials.mockResolvedValue({ ...baseCredentials, scope: ' ' });
|
||||
|
||||
await call();
|
||||
|
||||
expect(MockClientOAuth2).toHaveBeenCalledWith(expect.objectContaining({ scopes: undefined }));
|
||||
});
|
||||
|
||||
test('should pass a trimmed scopes array for a valid scope string', async () => {
|
||||
mockThis.getCredentials.mockResolvedValue({ ...baseCredentials, scope: 'read write' });
|
||||
|
||||
await call();
|
||||
|
||||
expect(MockClientOAuth2).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ scopes: ['read', 'write'] }),
|
||||
);
|
||||
});
|
||||
|
||||
test('should trim and filter extra whitespace between scopes', async () => {
|
||||
mockThis.getCredentials.mockResolvedValue({ ...baseCredentials, scope: ' read write ' });
|
||||
|
||||
await call();
|
||||
|
||||
expect(MockClientOAuth2).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ scopes: ['read', 'write'] }),
|
||||
);
|
||||
});
|
||||
});
|
||||
|
|
@ -1396,6 +1396,134 @@ describe('Request Helper Functions', () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe('requestOAuth2 - client credentials initial token fetch', () => {
|
||||
const baseUrl = 'https://api.example.com';
|
||||
const tokenUrl = 'https://auth.example.com';
|
||||
const mockThis = mockDeep<IAllExecuteFunctions>();
|
||||
const mockNode = mockDeep<INode>();
|
||||
const mockAdditionalData = mockDeep<IWorkflowExecuteAdditionalData>();
|
||||
|
||||
beforeEach(() => {
|
||||
nock.cleanAll();
|
||||
jest.resetAllMocks();
|
||||
mockNode.name = 'test-node';
|
||||
mockNode.credentials = {
|
||||
testOAuth2: { id: 'cred-id', name: 'cred-name' },
|
||||
};
|
||||
});
|
||||
|
||||
test('should not send scope parameter when scope is empty', async () => {
|
||||
mockThis.getCredentials.mockResolvedValue({
|
||||
clientId: 'client-id',
|
||||
clientSecret: 'client-secret',
|
||||
grantType: 'clientCredentials',
|
||||
accessTokenUrl: `${tokenUrl}/token`,
|
||||
authentication: 'body',
|
||||
scope: '',
|
||||
oauthTokenData: undefined,
|
||||
});
|
||||
|
||||
// Token endpoint must NOT receive scope in body
|
||||
nock(tokenUrl)
|
||||
.post('/token', (body) => !('scope' in body) || body.scope === undefined)
|
||||
.reply(
|
||||
200,
|
||||
{ access_token: 'new-token', token_type: 'bearer' },
|
||||
{ 'content-type': 'application/json' },
|
||||
);
|
||||
|
||||
nock(baseUrl).get('/data').reply(200, { success: true });
|
||||
|
||||
mockThis.helpers.httpRequest.mockResolvedValueOnce({ success: true });
|
||||
|
||||
await requestOAuth2.call(
|
||||
mockThis,
|
||||
'testOAuth2',
|
||||
{ method: 'GET', url: `${baseUrl}/data` },
|
||||
mockNode,
|
||||
mockAdditionalData,
|
||||
undefined,
|
||||
true, // isN8nRequest
|
||||
);
|
||||
|
||||
expect(mockThis.helpers.httpRequest).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
headers: expect.objectContaining({ Authorization: 'Bearer new-token' }),
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
test('should send scope parameter when scope is set', async () => {
|
||||
mockThis.getCredentials.mockResolvedValue({
|
||||
clientId: 'client-id',
|
||||
clientSecret: 'client-secret',
|
||||
grantType: 'clientCredentials',
|
||||
accessTokenUrl: `${tokenUrl}/token`,
|
||||
authentication: 'body',
|
||||
scope: 'read write',
|
||||
oauthTokenData: undefined,
|
||||
});
|
||||
|
||||
nock(tokenUrl)
|
||||
.post('/token', (body) => body.scope === 'read write')
|
||||
.reply(
|
||||
200,
|
||||
{ access_token: 'scoped-token', token_type: 'bearer' },
|
||||
{ 'content-type': 'application/json' },
|
||||
);
|
||||
|
||||
mockThis.helpers.httpRequest.mockResolvedValueOnce({ data: 'ok' });
|
||||
|
||||
await requestOAuth2.call(
|
||||
mockThis,
|
||||
'testOAuth2',
|
||||
{ method: 'GET', url: `${baseUrl}/data` },
|
||||
mockNode,
|
||||
mockAdditionalData,
|
||||
undefined,
|
||||
true,
|
||||
);
|
||||
|
||||
expect(mockThis.helpers.httpRequest).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
headers: expect.objectContaining({ Authorization: 'Bearer scoped-token' }),
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
test('should throw ApplicationError with clear message when token acquisition fails', async () => {
|
||||
mockThis.getCredentials.mockResolvedValue({
|
||||
clientId: 'client-id',
|
||||
clientSecret: 'wrong-secret',
|
||||
grantType: 'clientCredentials',
|
||||
accessTokenUrl: `${tokenUrl}/token`,
|
||||
authentication: 'body',
|
||||
scope: '',
|
||||
oauthTokenData: undefined,
|
||||
});
|
||||
|
||||
nock(tokenUrl)
|
||||
.post('/token')
|
||||
.reply(
|
||||
400,
|
||||
{ error: 'invalid_client', error_description: 'Invalid client credentials' },
|
||||
{ 'content-type': 'application/json' },
|
||||
);
|
||||
|
||||
await expect(
|
||||
requestOAuth2.call(
|
||||
mockThis,
|
||||
'testOAuth2',
|
||||
{ method: 'GET', url: `${baseUrl}/data` },
|
||||
mockNode,
|
||||
mockAdditionalData,
|
||||
undefined,
|
||||
true,
|
||||
),
|
||||
).rejects.toThrow('Failed to acquire OAuth2 access token');
|
||||
});
|
||||
});
|
||||
|
||||
describe('SSRF protection wiring', () => {
|
||||
const baseUrl = 'https://example.com';
|
||||
const workflow = mock<Workflow>();
|
||||
|
|
|
|||
|
|
@ -713,11 +713,16 @@ export function applyPaginationRequestData(
|
|||
}
|
||||
|
||||
function createOAuth2Client(credentials: OAuth2CredentialData): ClientOAuth2 {
|
||||
// Split and trim scopes; empty scope tokens are not RFC 6749-compliant and may be rejected by authorization servers
|
||||
const scopes = credentials.scope
|
||||
?.split(' ')
|
||||
.map((s) => s.trim())
|
||||
.filter(Boolean);
|
||||
return new ClientOAuth2({
|
||||
clientId: credentials.clientId,
|
||||
clientSecret: credentials.clientSecret,
|
||||
accessTokenUri: credentials.accessTokenUrl,
|
||||
scopes: (credentials.scope ?? '').split(' '),
|
||||
scopes: scopes?.length ? scopes : undefined,
|
||||
ignoreSSLIssues: credentials.ignoreSSLIssues,
|
||||
authentication: credentials.authentication ?? 'header',
|
||||
...(credentials.additionalBodyProperties && {
|
||||
|
|
@ -823,7 +828,16 @@ export async function requestOAuth2(
|
|||
Object.keys(oauthTokenData).length === 0 ||
|
||||
oauthTokenData.access_token === '') // stub
|
||||
) {
|
||||
const { data } = await oAuthClient.credentials.getToken();
|
||||
let tokenResult: Awaited<ReturnType<typeof oAuthClient.credentials.getToken>>;
|
||||
try {
|
||||
tokenResult = await oAuthClient.credentials.getToken();
|
||||
} catch (error) {
|
||||
const message = error instanceof Error ? error.message : String(error);
|
||||
throw new ApplicationError(`Failed to acquire OAuth2 access token: ${message}`, {
|
||||
cause: error,
|
||||
});
|
||||
}
|
||||
const { data } = tokenResult;
|
||||
// Find the credentials
|
||||
if (!node.credentials?.[credentialsType]) {
|
||||
throw new ApplicationError('Node does not have credential type', {
|
||||
|
|
|
|||
Loading…
Reference in a new issue