From 9191cf3fb07ce6bbb7b2217592ce56372b52f6c2 Mon Sep 17 00:00:00 2001 From: Guillaume Jacquart Date: Wed, 7 Oct 2026 19:16:11 +0000 Subject: [PATCH] fix(core): Apply the credential type policy to credential tests and probes (#39927) Co-authored-by: Claude Opus 5.5 (1M context) --- .../__tests__/credentials.service.test.ts | 150 +++++++++++++++ .../src/credentials/credentials.service.ts | 55 +++++- .../instance-ai.adapter.service.test.ts | 55 ++++++ .../instance-ai.adapter.service.ts | 6 +- .../cli/src/policy/policy-violation.error.ts | 13 +- ...dential-type-policy.credential-use.test.ts | 182 ++++++++++++++++++ .../__tests__/authentication.test.ts | 43 +++++ .../utils/request-helpers/authentication.ts | 12 +- packages/workflow/src/errors/index.ts | 1 + .../workflow/src/errors/policy-refusal.ts | 14 ++ .../test/errors/policy-refusal.test.ts | 24 +++ 11 files changed, 534 insertions(+), 21 deletions(-) create mode 100644 packages/cli/test/integration/policy/credential-type-policy.credential-use.test.ts create mode 100644 packages/workflow/src/errors/policy-refusal.ts create mode 100644 packages/workflow/test/errors/policy-refusal.test.ts diff --git a/packages/cli/src/credentials/__tests__/credentials.service.test.ts b/packages/cli/src/credentials/__tests__/credentials.service.test.ts index 70d199bc13a..378b6421f63 100644 --- a/packages/cli/src/credentials/__tests__/credentials.service.test.ts +++ b/packages/cli/src/credentials/__tests__/credentials.service.test.ts @@ -1239,6 +1239,82 @@ describe('CredentialsService', () => { }); }); + const credentialUseRefusal = () => + new PolicyViolationError([ + { kind: 'test', checkId: 'test', message: 'Credential type "githubApi" is blocked' }, + ]); + + describe('decryptForUse', () => { + const storedCredential = mock({ id: 'cred-id', type: 'githubApi' }); + + it('checks the policy against the owning project, then returns the raw data', async () => { + sharedCredentialsRepository.findCredentialOwningProject.mockResolvedValue( + mock({ id: 'project-1' }), + ); + vi.spyOn(service, 'decrypt').mockResolvedValue({ accessToken: 'secret' }); + + await expect(service.decryptForUse(storedCredential, ownerActor)).resolves.toEqual({ + accessToken: 'secret', + }); + expect(policyEnforcementService.enforceCredentialDecrypt).toHaveBeenCalledExactlyOnceWith( + { + credentialType: 'githubApi', + credentialId: 'cred-id', + consumer: null, + projectId: 'project-1', + }, + ownerActor, + ); + expect(service.decrypt).toHaveBeenCalledWith(storedCredential, true); + }); + + it('judges on the given project without looking up the owner', async () => { + vi.spyOn(service, 'decrypt').mockResolvedValue({}); + + await service.decryptForUse(storedCredential, ownerActor, 'project-2'); + + expect(sharedCredentialsRepository.findCredentialOwningProject).not.toHaveBeenCalled(); + expect(policyEnforcementService.enforceCredentialDecrypt).toHaveBeenCalledWith( + expect.objectContaining({ projectId: 'project-2' }), + ownerActor, + ); + }); + + it('skips the owner lookup when the caller already found no owner', async () => { + vi.spyOn(service, 'decrypt').mockResolvedValue({}); + + await service.decryptForUse(storedCredential, ownerActor, null); + + expect(sharedCredentialsRepository.findCredentialOwningProject).not.toHaveBeenCalled(); + expect(policyEnforcementService.enforceCredentialDecrypt).toHaveBeenCalledWith( + expect.objectContaining({ projectId: null }), + ownerActor, + ); + }); + + it('judges on instance policy only when the credential has no owning project', async () => { + sharedCredentialsRepository.findCredentialOwningProject.mockResolvedValue(undefined); + vi.spyOn(service, 'decrypt').mockResolvedValue({}); + + await service.decryptForUse(storedCredential, ownerActor); + + expect(policyEnforcementService.enforceCredentialDecrypt).toHaveBeenCalledWith( + expect.objectContaining({ projectId: null }), + ownerActor, + ); + }); + + it('does not decrypt when the policy refuses', async () => { + policyEnforcementService.enforceCredentialDecrypt.mockRejectedValue(credentialUseRefusal()); + const decrypt = vi.spyOn(service, 'decrypt'); + + await expect( + service.decryptForUse(storedCredential, ownerActor, 'project-1'), + ).rejects.toThrow(PolicyViolationError); + expect(decrypt).not.toHaveBeenCalled(); + }); + }); + describe('testById', () => { it('throws CredentialNotFoundError when the user cannot use the credential', async () => { credentialsFinderService.findCredentialForUser.mockResolvedValue(null); @@ -1298,6 +1374,18 @@ describe('CredentialsService', () => { ); expect(result).toEqual(testResult); }); + + it('refuses a blocked credential before the tester runs', async () => { + credentialsFinderService.findCredentialForUser.mockResolvedValue( + mock({ id: 'credential-id', type: 'githubApi', usageScope: 'project' }), + ); + policyEnforcementService.enforceCredentialDecrypt.mockRejectedValue(credentialUseRefusal()); + + await expect(service.testById(ownerUser, 'credential-id')).rejects.toThrow( + PolicyViolationError, + ); + expect(credentialsTester.testCredentials).not.toHaveBeenCalled(); + }); }); describe('getOne', () => { @@ -2079,6 +2167,60 @@ describe('CredentialsService', () => { }); describe('testWithCredentials', () => { + it('refuses a stored credential of a blocked type before the tester runs', async () => { + credentialsFinderService.findCredentialForUser.mockResolvedValue( + mock({ id: 'credential-id', type: 'githubApi', isManaged: false }), + ); + policyEnforcementService.enforceCredentialDecrypt.mockRejectedValue(credentialUseRefusal()); + + await expect( + service.testWithCredentials(ownerUser, { + id: 'credential-id', + name: 'GitHub account', + type: 'githubApi', + data: {}, + }), + ).rejects.toThrow(PolicyViolationError); + expect(credentialsTester.testCredentials).not.toHaveBeenCalled(); + }); + + it('rejects a posted type that differs from the stored one before decrypting', async () => { + // A managed credential ignores the posted payload, so this needs an unmanaged one. + credentialsFinderService.findCredentialForUser.mockResolvedValue( + mock({ id: 'credential-id', type: 'githubApi', isManaged: false }), + ); + + await expect( + service.testWithCredentials(ownerUser, { + id: 'credential-id', + name: 'GitHub account', + type: 'slackApi', + data: {}, + }), + ).rejects.toThrow(BadRequestError); + expect(policyEnforcementService.enforceCredentialDecrypt).not.toHaveBeenCalled(); + expect(credentialsTester.testCredentials).not.toHaveBeenCalled(); + }); + + it('refuses an unsaved provider connection of a blocked type on instance policy', async () => { + credentialsFinderService.findCredentialForUser.mockResolvedValue(null); + policyEnforcementService.enforceCredentialDecrypt.mockRejectedValue(credentialUseRefusal()); + + await expect( + service.testWithCredentials(ownerUser, { + id: '', + name: 'n8n Assistant model', + type: 'openAiApi', + data: { apiKey: 'key' }, + }), + ).rejects.toThrow(PolicyViolationError); + expect(policyEnforcementService.enforceCredentialDecrypt).toHaveBeenCalledWith( + { credentialType: 'openAiApi', credentialId: '', consumer: null, projectId: null }, + { kind: 'user', user: ownerUser }, + ); + expect(credentialsTester.testCredentials).not.toHaveBeenCalled(); + }); + it('tests an unsaved provider connection for an instance credential manager', async () => { const testResult = { status: 'OK', message: 'Credential tested successfully' } as const; credentialsFinderService.findCredentialForUser.mockResolvedValue(null); @@ -4929,6 +5071,14 @@ describe('CredentialsService', () => { ); }); + it('should refuse a blocked credential before probing it', async () => { + credentialsFinderService.findCredentialForUser.mockResolvedValue(storedCredential); + policyEnforcementService.enforceCredentialDecrypt.mockRejectedValue(credentialUseRefusal()); + + await expect(service.probeById(ownerUser, 'cred-id')).rejects.toThrow(PolicyViolationError); + expect(credentialsTester.probeCredentialAuth).not.toHaveBeenCalled(); + }); + it('should throw when the credential has no test URL', async () => { credentialsFinderService.findCredentialForUser.mockResolvedValue(storedCredential); mockDecryptedData({ template: '{}' }); diff --git a/packages/cli/src/credentials/credentials.service.ts b/packages/cli/src/credentials/credentials.service.ts index 3f3e3e5b09d..fc83809a0ce 100644 --- a/packages/cli/src/credentials/credentials.service.ts +++ b/packages/cli/src/credentials/credentials.service.ts @@ -1028,6 +1028,36 @@ export class CredentialsService { await this.update(credential.id, newCredentialData, actor, decryptedData); } + /** + * Decrypts a stored credential whose secret is about to leave n8n, such as a test call to its + * provider. Refuses what the credential policy blocks; `decrypt` stays for display and rewrite. + * Omit `projectId` to judge on the owning project. Pass `null` when the caller already found + * no owner: only instance policy applies, and the lookup is skipped. + */ + async decryptForUse( + credential: CredentialsEntity, + actor: PolicyActor, + projectId?: string | null, + ): Promise { + const judgedProjectId = + projectId === undefined + ? ((await this.findCredentialOwningProject(credential.id))?.id ?? null) + : projectId; + await this.enforceCredentialUse(credential, actor, judgedProjectId); + return await this.decrypt(credential, true); + } + + private async enforceCredentialUse( + credential: Pick, + actor: PolicyActor, + projectId: string | null, + ) { + await this.policyEnforcementService.enforceCredentialDecrypt( + { credentialType: credential.type, credentialId: credential.id, consumer: null, projectId }, + actor, + ); + } + /** * Decrypts the credentials data and redacts the content by default. * @@ -1488,7 +1518,7 @@ export class CredentialsService { throw new CredentialNotFoundError(credentialId); } - const credentials = await this.prepareCredentialsForTest({ storedCredential }); + const credentials = await this.prepareCredentialsForTest({ storedCredential, user }); return await this.test(user.id, credentials); } @@ -1503,6 +1533,8 @@ export class CredentialsService { if (!storedCredential) { if (credentials.id === '' && hasGlobalScope(user, 'credential:manageInstance')) { this.validateInstanceCredentialData(credentials.data ?? {}); + // Nothing stored is decrypted, but the test still calls the blocked type's provider. + await this.enforceCredentialUse(credentials, { kind: 'user', user }, null); return await this.test(user.id, credentials); } throw new CredentialNotFoundError(credentials.id); @@ -1534,7 +1566,7 @@ export class CredentialsService { throw new CredentialNotFoundError(credentialId); } - const data = await this.decrypt(storedCredential, true); + const data = await this.decryptForUse(storedCredential, { kind: 'user', user }); // Expressions and non-HTTP values are refused, not resolved. const testTarget = parseHttpUrl(data.testUrl); @@ -2399,10 +2431,21 @@ export class CredentialsService { credentialsToTest, }: { storedCredential: CredentialsEntity; - user?: User; + user: User; credentialsToTest?: ICredentialsDecrypted; }): Promise { - const decryptedData = await this.decrypt(storedCredential, true); + // The tester picks its test from the posted type, so it must match the stored secrets. + if (credentialsToTest && credentialsToTest.type !== storedCredential.type) { + throw new BadRequestError('The credential type does not match the stored credential'); + } + + // Find the owning project to prevent leakage of other project data. + const owningProject = await this.findCredentialOwningProject(storedCredential.id); + const decryptedData = await this.decryptForUse( + storedCredential, + { kind: 'user', user }, + owningProject?.id ?? null, + ); const mergedCredentials: ICredentialsDecrypted = credentialsToTest ? deepCopy(credentialsToTest) : { @@ -2412,8 +2455,6 @@ export class CredentialsService { data: decryptedData, }; - // Find the owning project to prevent leakage of other project data. - const owningProject = await this.findCredentialOwningProject(storedCredential.id); if (!owningProject) { mergedCredentials.homeProject = undefined; } else { @@ -2427,7 +2468,7 @@ export class CredentialsService { }; } - if (user && credentialsToTest) { + if (credentialsToTest) { await this.replaceCredentialContentsForSharee( user, storedCredential, diff --git a/packages/cli/src/modules/instance-ai/__tests__/instance-ai.adapter.service.test.ts b/packages/cli/src/modules/instance-ai/__tests__/instance-ai.adapter.service.test.ts index 81c1fe3a746..e11b6703e27 100644 --- a/packages/cli/src/modules/instance-ai/__tests__/instance-ai.adapter.service.test.ts +++ b/packages/cli/src/modules/instance-ai/__tests__/instance-ai.adapter.service.test.ts @@ -8074,6 +8074,61 @@ describe('createContext: aiPreferenceService', () => { }); describe('createCredentialAdapter', () => { + describe('test', () => { + const storedCredential = { id: 'cred-1', name: 'Slack account', type: 'slackApi' }; + + const adapterWith = (decryptForUse: ReturnType, test: ReturnType) => + createNodeAdapterServiceForTests([], { + credentialsFinderService: { + findCredentialForUser: vi.fn().mockResolvedValue(storedCredential), + }, + credentialsService: { decryptForUse, test }, + }); + + it('decrypts through the policy-checked path as the user', async () => { + const decryptForUse = vi.fn().mockResolvedValue({ accessToken: 'secret' }); + const test = vi.fn().mockResolvedValue({ status: 'OK', message: 'ok' }); + const { credentialService, mockUser } = adapterWith(decryptForUse, test); + + await expect(credentialService.test('cred-1')).resolves.toEqual({ + success: true, + message: 'ok', + }); + expect(decryptForUse).toHaveBeenCalledWith( + storedCredential, + { kind: 'user', user: mockUser }, + undefined, + ); + }); + + it('judges the policy on the project the thread is bound to', async () => { + const decryptForUse = vi.fn().mockResolvedValue({}); + const test = vi.fn().mockResolvedValue({ status: 'OK', message: 'ok' }); + const { service, mockUser } = adapterWith(decryptForUse, test); + const { credentialService } = service.createContext(mockUser, { + projectId: 'team-project-1', + }); + + await credentialService.test('cred-1'); + + expect(decryptForUse).toHaveBeenCalledWith( + storedCredential, + { kind: 'user', user: mockUser }, + 'team-project-1', + ); + }); + + it('does not run the test when the policy refuses the decrypt', async () => { + const refusal = new Error('Credential type "slackApi" is blocked by an instance policy'); + const decryptForUse = vi.fn().mockRejectedValue(refusal); + const test = vi.fn(); + const { credentialService } = adapterWith(decryptForUse, test); + + await expect(credentialService.test('cred-1')).rejects.toBe(refusal); + expect(test).not.toHaveBeenCalled(); + }); + }); + describe('getCredentialFillState', () => { /** An adapter over a credential type declaring `properties` and holding `data`. */ const adapterFor = ( diff --git a/packages/cli/src/modules/instance-ai/instance-ai.adapter.service.ts b/packages/cli/src/modules/instance-ai/instance-ai.adapter.service.ts index c6f3bdafd15..0f4c0a5ce6c 100644 --- a/packages/cli/src/modules/instance-ai/instance-ai.adapter.service.ts +++ b/packages/cli/src/modules/instance-ai/instance-ai.adapter.service.ts @@ -2846,7 +2846,11 @@ export class InstanceAiAdapterService { id: credential.id, name: credential.name, type: credential.type, - data: await credentialsService.decrypt(credential, true), + data: await credentialsService.decryptForUse( + credential, + { kind: 'user', user }, + boundProjectId, + ), }; const result = await credentialsService.test(user.id, credentialsToTest); diff --git a/packages/cli/src/policy/policy-violation.error.ts b/packages/cli/src/policy/policy-violation.error.ts index 23e5bac0532..59df5d96c73 100644 --- a/packages/cli/src/policy/policy-violation.error.ts +++ b/packages/cli/src/policy/policy-violation.error.ts @@ -1,5 +1,5 @@ import type { PolicyViolation } from '@n8n/decorators'; -import { UserError } from 'n8n-workflow'; +import { hasPolicyRefusalMarker, UserError } from 'n8n-workflow'; /** `enforce*` only throws when something objected, so an empty list is a bug, not a case. */ export type NonEmptyViolations = [PolicyViolation, ...PolicyViolation[]]; @@ -61,14 +61,5 @@ export class PolicyViolationError extends UserError { * is needed, e.g. to read {@link PolicyViolationError.violations}. */ export function isPolicyRefusal(error: unknown): boolean { - if (error instanceof PolicyViolationError) return true; - - // `hasOwn`, not `in`: the marker is an own property by contract, and an inherited - // one would let an unrelated error suppress cleanup and retries. - return ( - typeof error === 'object' && - error !== null && - Object.hasOwn(error, 'isPolicyRefusal') && - (error as { isPolicyRefusal?: unknown }).isPolicyRefusal === true - ); + return error instanceof PolicyViolationError || hasPolicyRefusalMarker(error); } diff --git a/packages/cli/test/integration/policy/credential-type-policy.credential-use.test.ts b/packages/cli/test/integration/policy/credential-type-policy.credential-use.test.ts new file mode 100644 index 00000000000..562186fdb84 --- /dev/null +++ b/packages/cli/test/integration/policy/credential-type-policy.credential-use.test.ts @@ -0,0 +1,182 @@ +/** + * Pins the credential policy on the routes that decrypt a stored credential to call its provider + * without going through a workflow run: the test, probe and public API test routes. + */ +import { createTeamProject, testDb } from '@n8n/backend-test-utils'; +import { LICENSE_FEATURES } from '@n8n/constants'; +import type { CredentialsEntity, Project, User } from '@n8n/db'; +import { Container } from '@n8n/di'; +import { mock } from 'vitest-mock-extended'; + +import { CredentialsTester } from '@/services/credentials-tester.service'; + +import { saveCredential } from '../shared/db/credentials'; +import { createOwnerWithApiKey } from '../shared/db/users'; +import type { SuperAgentTest } from '../shared/types'; +import * as utils from '../shared/utils/'; +import { denyRule, putCredentialTypePolicy } from './shared/credential-type-policy'; +import { clearPolicyCache } from './shared/policy-cache'; + +const GITHUB_API = 'githubApi'; + +const testServer = utils.setupTestServer({ + endpointGroups: ['credentials', 'publicApi', 'type-availability-policies'], + modules: ['policy-infrastructure', 'type-availability-policies'], + enabledFeatures: [LICENSE_FEATURES.TYPE_AVAILABILITY_POLICIES], +}); + +const credentialsTester = mock(); +Container.set(CredentialsTester, credentialsTester); + +let owner: User; +let ownerAgent: SuperAgentTest; +let publicApiAgent: SuperAgentTest; + +const PROBEABLE_DATA = { + accessToken: 'secret', + testUrl: 'https://api.example.com/me', + serviceOrigin: 'https://api.example.com', +}; + +async function saveGithubCredential(owner: { user: User } | { project: Project }) { + return await saveCredential( + { name: 'GitHub account', type: GITHUB_API, data: PROBEABLE_DATA }, + { ...owner, role: 'credential:owner' }, + ); +} + +const denyGithub = async (projectId: string | null) => + await putCredentialTypePolicy(ownerAgent, projectId, { + rules: [denyRule('deny-github', GITHUB_API)], + }); + +const testRoute = async (credential: CredentialsEntity) => + await ownerAgent.post('/credentials/test').send({ + credentials: { id: credential.id, name: credential.name, type: credential.type, data: {} }, + }); + +beforeAll(async () => { + owner = await createOwnerWithApiKey(); + ownerAgent = testServer.authAgentFor(owner); + publicApiAgent = testServer.publicApiAgentFor(owner); + + await utils.initCredentialsTypes(); +}); + +beforeEach(() => { + credentialsTester.testCredentials.mockResolvedValue({ status: 'OK', message: 'Tested' }); + credentialsTester.probeCredentialAuth.mockResolvedValue({ + status: 'OK', + message: 'Tested', + outcome: 'accepted', + }); +}); + +afterEach(async () => { + vi.clearAllMocks(); + await testDb.truncate([ + 'TypeAvailabilityPolicyAttachment', + 'TypeAvailabilityPolicyScope', + 'TypeAvailabilityPolicy', + 'SharedCredentials', + 'CredentialsEntity', + ]); + await clearPolicyCache(); +}); + +describe('a credential of a type an instance policy blocks', () => { + test('POST /credentials/test is refused before the tester runs', async () => { + const credential = await saveGithubCredential({ user: owner }); + await denyGithub(null); + + const response = await testRoute(credential); + + expect(response.statusCode).toBe(403); + expect(response.body.message).toBe( + `Credential type "${GITHUB_API}" is blocked by an instance policy`, + ); + expect(credentialsTester.testCredentials).not.toHaveBeenCalled(); + }); + + test('POST /credentials/:id/probe is refused before the probe runs', async () => { + const credential = await saveGithubCredential({ user: owner }); + await denyGithub(null); + + const response = await ownerAgent.post(`/credentials/${credential.id}/probe`); + + expect(response.statusCode).toBe(403); + expect(credentialsTester.probeCredentialAuth).not.toHaveBeenCalled(); + }); + + test('public API POST /credentials/:id/test is refused before the tester runs', async () => { + const credential = await saveGithubCredential({ user: owner }); + await denyGithub(null); + + const response = await publicApiAgent.post(`/credentials/${credential.id}/test`); + + expect(response.statusCode).toBe(403); + expect(credentialsTester.testCredentials).not.toHaveBeenCalled(); + }); + + test('an unsaved instance credential test is refused', async () => { + await denyGithub(null); + + const response = await ownerAgent.post('/credentials/test').send({ + credentials: { id: '', name: 'GitHub account', type: GITHUB_API, data: {} }, + }); + + expect(response.statusCode).toBe(403); + expect(credentialsTester.testCredentials).not.toHaveBeenCalled(); + }); +}); + +describe('a credential of a type a project policy blocks', () => { + test('is refused when the blocking project owns it', async () => { + const project = await createTeamProject('Blocking project', owner); + const credential = await saveGithubCredential({ project }); + await denyGithub(project.id); + + const response = await testRoute(credential); + + expect(response.statusCode).toBe(403); + expect(credentialsTester.testCredentials).not.toHaveBeenCalled(); + }); + + test('still tests when another project owns it', async () => { + const blockingProject = await createTeamProject('Blocking project', owner); + await denyGithub(blockingProject.id); + const credential = await saveGithubCredential({ user: owner }); + + const response = await testRoute(credential); + + expect(response.statusCode).toBe(200); + expect(credentialsTester.testCredentials).toHaveBeenCalledOnce(); + }); +}); + +describe('a test whose posted type differs from the stored one', () => { + test('is rejected before the tester runs', async () => { + const credential = await saveGithubCredential({ user: owner }); + + const response = await ownerAgent.post('/credentials/test').send({ + credentials: { id: credential.id, name: credential.name, type: 'httpHeaderAuth', data: {} }, + }); + + expect(response.statusCode).toBe(400); + expect(credentialsTester.testCredentials).not.toHaveBeenCalled(); + }); +}); + +describe('a credential of a type no rule denies', () => { + test('tests and probes as before', async () => { + const credential = await saveGithubCredential({ user: owner }); + await putCredentialTypePolicy(ownerAgent, null, { + rules: [denyRule('deny-slack', 'slackApi')], + }); + + expect((await testRoute(credential)).statusCode).toBe(200); + expect((await ownerAgent.post(`/credentials/${credential.id}/probe`)).statusCode).toBe(200); + expect(credentialsTester.testCredentials).toHaveBeenCalledOnce(); + expect(credentialsTester.probeCredentialAuth).toHaveBeenCalledOnce(); + }); +}); diff --git a/packages/core/src/execution-engine/node-execution-context/utils/request-helpers/__tests__/authentication.test.ts b/packages/core/src/execution-engine/node-execution-context/utils/request-helpers/__tests__/authentication.test.ts index 0de762e4e87..1a8d9c489ea 100644 --- a/packages/core/src/execution-engine/node-execution-context/utils/request-helpers/__tests__/authentication.test.ts +++ b/packages/core/src/execution-engine/node-execution-context/utils/request-helpers/__tests__/authentication.test.ts @@ -22,6 +22,12 @@ vi.mock('../legacy-request-adapter', () => ({ proxyRequestToAxios: vi.fn(), })); +/** Shaped like cli's `PolicyViolationError`, which core cannot import. */ +const policyRefusal = () => + Object.assign(new Error('Credential type "slackApi" is blocked by an instance policy'), { + isPolicyRefusal: true, + }); + describe('httpRequestWithAuthentication', () => { const baseUrl = 'https://api.example.com'; const tokenUrl = 'https://auth.example.com'; @@ -374,6 +380,25 @@ describe('httpRequestWithAuthentication', () => { expect(result).toEqual({ ok: true }); expect(request).toHaveBeenCalledTimes(1); }); + + test('rethrows a policy refusal from the credential read unchanged, with no request sent', async () => { + mockAdditionalData.credentialsHelper.getParentTypes.mockReturnValue([]); + const refusal = policyRefusal(); + mockThis.getCredentials.mockRejectedValue(refusal); + + await expect( + httpRequestWithAuthentication.call( + mockThis, + 'slackApi', + { method: 'GET', url: `${baseUrl}/items` }, + mockWorkflow, + mockNode, + mockAdditionalData, + ), + ).rejects.toBe(refusal); + + expect(request).not.toHaveBeenCalled(); + }); }); describe('requestWithAuthentication (legacy) — preAuthentication retry', () => { @@ -508,4 +533,22 @@ describe('requestWithAuthentication (legacy) — preAuthentication retry', () => ); }, ); + + test('rethrows a policy refusal from the credential read unchanged, with no request sent', async () => { + const refusal = policyRefusal(); + mockThis.getCredentials.mockRejectedValue(refusal); + + await expect( + requestWithAuthentication.call( + mockThis, + 'slackApi', + { method: 'GET', uri: 'https://api.example.com/items' }, + mockWorkflow, + mockNode, + mockAdditionalData, + ), + ).rejects.toBe(refusal); + + expect(proxyRequestToAxiosMock).not.toHaveBeenCalled(); + }); }); diff --git a/packages/core/src/execution-engine/node-execution-context/utils/request-helpers/authentication.ts b/packages/core/src/execution-engine/node-execution-context/utils/request-helpers/authentication.ts index d19923d5c3a..61366b3e33a 100644 --- a/packages/core/src/execution-engine/node-execution-context/utils/request-helpers/authentication.ts +++ b/packages/core/src/execution-engine/node-execution-context/utils/request-helpers/authentication.ts @@ -13,7 +13,12 @@ import type { IWorkflowExecuteAdditionalData, Workflow, } from 'n8n-workflow'; -import { ExecutionBaseError, NodeApiError, NodeOperationError } from 'n8n-workflow'; +import { + ExecutionBaseError, + hasPolicyRefusalMarker, + NodeApiError, + NodeOperationError, +} from 'n8n-workflow'; import { callEvalMockHandler, normalizeLegacyRequest } from '@/execution-engine/eval-mock-helpers'; @@ -137,6 +142,9 @@ export async function httpRequestWithAuthentication( requestSent = true; return await Container.get(OutboundHttp).requests().request(requestOptions); } catch (error) { + // Keep the policy reason visible instead of reporting it as a failed API call. + if (hasPolicyRefusalMarker(error)) throw error; + // if there is a pre authorization method defined and // the method failed due to unauthorized request if ( @@ -316,7 +324,7 @@ export async function requestWithAuthentication( } throw error; } catch (error) { - if (error instanceof ExecutionBaseError) throw error; + if (error instanceof ExecutionBaseError || hasPolicyRefusalMarker(error)) throw error; throw new NodeApiError(this.getNode(), error); } diff --git a/packages/workflow/src/errors/index.ts b/packages/workflow/src/errors/index.ts index 8856d44f6ef..3d4dab6a7f3 100644 --- a/packages/workflow/src/errors/index.ts +++ b/packages/workflow/src/errors/index.ts @@ -46,4 +46,5 @@ export { ExpressionClassExtensionError } from './expression-class-extension.erro export { ExpressionReservedVariableError } from './expression-reserved-variable.error'; export { ExpressionWithStatementError } from './expression-with-statement.error'; export { DbConnectionTimeoutError } from './db-connection-timeout-error'; +export { hasPolicyRefusalMarker } from './policy-refusal'; export { IsolateError } from '@n8n/errors'; diff --git a/packages/workflow/src/errors/policy-refusal.ts b/packages/workflow/src/errors/policy-refusal.ts new file mode 100644 index 00000000000..602a93fec3b --- /dev/null +++ b/packages/workflow/src/errors/policy-refusal.ts @@ -0,0 +1,14 @@ +/** + * Whether an error carries the own `isPolicyRefusal` marker that a policy refusal sets. + * Lets packages that cannot import the refusal class recognise one, also after serialization. + */ +export function hasPolicyRefusalMarker(error: unknown): boolean { + // `hasOwn`, not `in` alone: an inherited marker must not count. + return ( + typeof error === 'object' && + error !== null && + Object.hasOwn(error, 'isPolicyRefusal') && + 'isPolicyRefusal' in error && + error.isPolicyRefusal === true + ); +} diff --git a/packages/workflow/test/errors/policy-refusal.test.ts b/packages/workflow/test/errors/policy-refusal.test.ts new file mode 100644 index 00000000000..c11a57b261e --- /dev/null +++ b/packages/workflow/test/errors/policy-refusal.test.ts @@ -0,0 +1,24 @@ +import { hasPolicyRefusalMarker } from '../../src/errors'; + +describe('hasPolicyRefusalMarker', () => { + it('recognises an error that carries the marker as an own property', () => { + const refusal = Object.assign(new Error('Blocked'), { isPolicyRefusal: true }); + + expect(hasPolicyRefusalMarker(refusal)).toBe(true); + }); + + it('does not recognise an inherited marker', () => { + const inherited = Object.create({ isPolicyRefusal: true }) as object; + + expect(hasPolicyRefusalMarker(inherited)).toBe(false); + }); + + it.each([ + ['an ordinary error', new Error('boom')], + ['a non-true marker', { isPolicyRefusal: 'yes' }], + ['null', null], + ['undefined', undefined], + ])('does not recognise %s', (_label, value) => { + expect(hasPolicyRefusalMarker(value)).toBe(false); + }); +});