diff --git a/packages/core/src/execution-engine/__tests__/routing-node.test.ts b/packages/core/src/execution-engine/__tests__/routing-node.test.ts index f43ac27be45..ba27460b41b 100644 --- a/packages/core/src/execution-engine/__tests__/routing-node.test.ts +++ b/packages/core/src/execution-engine/__tests__/routing-node.test.ts @@ -2531,11 +2531,16 @@ describe('RoutingNode', () => { position: [0, 0], }; - const buildNodeType = (): INodeType => { + // `null` means the node declares no base URL; a default parameter cannot express that, + // since an explicit `undefined` triggers the default. + const buildNodeType = ( + baseURL: string | null = 'https://api.example.com', + routedUrl = 'https://other-host.example.com/path', + ): INodeType => { const routingNodeType = nodeTypes.getByNameAndVersion(baseNode.type); routingNodeType.description = { credentials: [{ name: 'testCredential', required: true }], - requestDefaults: { baseURL: 'https://api.example.com' }, + requestDefaults: { baseURL: baseURL ?? undefined }, properties: [ { displayName: 'Endpoint', @@ -2544,7 +2549,7 @@ describe('RoutingNode', () => { default: '', routing: { request: { - url: 'https://attacker.com/exfiltrate', + url: routedUrl, }, }, }, @@ -2553,9 +2558,12 @@ describe('RoutingNode', () => { return routingNodeType; }; - const runWithCredential = async (data: Record) => { + const runWithCredential = async ( + data: Record, + options: { baseURL?: string | null; routedUrl?: string } = {}, + ) => { const credentialData = data as unknown as ICredentialDataDecryptedObject; - const nodeType = buildNodeType(); + const nodeType = buildNodeType(options.baseURL, options.routedUrl); const workflow = new Workflow({ nodes: [baseNode], connections: {}, @@ -2628,32 +2636,69 @@ describe('RoutingNode', () => { expect(requestOptions.allowedDomains).toBeUndefined(); }); - test("throws when mode is 'none'", async () => { - await expect( - runWithCredential({ - apiKey: 'testApiKey', - allowedHttpRequestDomains: 'none', - }), - ).rejects.toThrow('This credential is configured to prevent use within an HTTP Request node'); + test("adds the node's own host when mode is 'domains' and the list omits it", async () => { + const result = await runWithCredential({ + apiKey: 'testApiKey', + allowedHttpRequestDomains: 'domains', + allowedDomains: 'other.example.com', + }); + + const requestOptions = (result?.[0]?.[0]?.json as { requestOptions: IHttpRequestOptions }) + .requestOptions; + expect(requestOptions.allowedDomains).toBe('api.example.com, other.example.com'); }); - test("throws when mode is 'domains' but the list is empty", async () => { - await expect( - runWithCredential({ + test.each([ + ['the node declares no base URL', null], + ['the base URL resolves to an empty string', ''], + ])('does not widen the allowlist from the routed URL when %s', async (_label, baseURL) => { + // The routed URL can interpolate a node parameter, so its host is the user's choice. + const result = await runWithCredential( + { apiKey: 'testApiKey', allowedHttpRequestDomains: 'domains', - allowedDomains: ' ', - }), - ).rejects.toThrow('No allowed domains specified'); + allowedDomains: 'other.example.com', + }, + { baseURL, routedUrl: 'https://user-chosen.example.net/path' }, + ); + + const requestOptions = (result?.[0]?.[0]?.json as { requestOptions: IHttpRequestOptions }) + .requestOptions; + expect(requestOptions.allowedDomains).toBe('other.example.com'); }); - test("throws when mode is 'domains' but the list is missing", async () => { - await expect( - runWithCredential({ - apiKey: 'testApiKey', - allowedHttpRequestDomains: 'domains', - }), - ).rejects.toThrow('No allowed domains specified'); + test("does not block a declarative node when mode is 'none'", async () => { + const result = await runWithCredential({ + apiKey: 'testApiKey', + allowedHttpRequestDomains: 'none', + }); + + const requestOptions = (result?.[0]?.[0]?.json as { requestOptions: IHttpRequestOptions }) + .requestOptions; + expect(requestOptions.allowedDomains).toBeUndefined(); + }); + + test("falls back to the node's own host when the 'domains' list is empty", async () => { + const result = await runWithCredential({ + apiKey: 'testApiKey', + allowedHttpRequestDomains: 'domains', + allowedDomains: ' ', + }); + + const requestOptions = (result?.[0]?.[0]?.json as { requestOptions: IHttpRequestOptions }) + .requestOptions; + expect(requestOptions.allowedDomains).toBe('api.example.com'); + }); + + test("falls back to the node's own host when the 'domains' list is missing", async () => { + const result = await runWithCredential({ + apiKey: 'testApiKey', + allowedHttpRequestDomains: 'domains', + }); + + const requestOptions = (result?.[0]?.[0]?.json as { requestOptions: IHttpRequestOptions }) + .requestOptions; + expect(requestOptions.allowedDomains).toBe('api.example.com'); }); test('does not set allowedDomains when restriction field is absent', async () => { diff --git a/packages/core/src/execution-engine/routing-node.ts b/packages/core/src/execution-engine/routing-node.ts index 1042a9673e9..3a976b6ff29 100644 --- a/packages/core/src/execution-engine/routing-node.ts +++ b/packages/core/src/execution-engine/routing-node.ts @@ -227,8 +227,16 @@ export class RoutingNode { itemContext[itemIndex].requestData.options.timeout = 300_000; } + // A declarative node's URL comes from its own routing, not from the user. Only + // `baseURL` is safe to widen the allowlist with: a per-operation `url` can + // interpolate a node parameter, and that host is the user's choice, not the node's. const allowedDomains = credentials - ? getCredentialAllowedDomains({ node, credentialData: credentials }) + ? getCredentialAllowedDomains({ + node, + credentialData: credentials, + credentialOwnedSurface: true, + nodeEndpointUrl: itemContext[itemIndex].requestData.options.baseURL, + }) : undefined; if (allowedDomains) { itemContext[itemIndex].requestData.options.allowedDomains = allowedDomains; diff --git a/packages/workflow/src/credential-domain-restrictions.ts b/packages/workflow/src/credential-domain-restrictions.ts index f11a0299e18..8295bddba45 100644 --- a/packages/workflow/src/credential-domain-restrictions.ts +++ b/packages/workflow/src/credential-domain-restrictions.ts @@ -105,14 +105,7 @@ export function isDomainAllowed(options: { url: string; allowedDomains: string } return true; } - let url: URL; - try { - url = new URL(urlString); - } catch { - return false; - } - - const hostname = url.hostname.toLowerCase().replace(/\.$/, ''); + const hostname = toHostname(urlString); if (!hostname) return false; const allowedDomainsList = allowedDomains @@ -148,6 +141,16 @@ function toDisplayHost(url: string): string { } } +/** Hostname of an absolute URL, normalised for matching. `undefined` when there is none. */ +function toHostname(url: string | undefined): string | undefined { + if (!url) return undefined; + try { + return new URL(url).hostname.toLowerCase().replace(/\.$/, '') || undefined; + } catch { + return undefined; + } +} + /** Throws `UserError` when `node` is omitted, so callers without an `INode` (axios helper) get a wrappable error. */ export function assertUrlAllowed(options: { url: string; @@ -162,31 +165,61 @@ export function assertUrlAllowed(options: { throw node ? new NodeOperationError(node, message) : new UserError(message); } -/** Returns the allowlist for forwarding to per-hop redirect checks; `undefined` means allow-all. */ +/** + * Returns the allowlist for forwarding to per-hop redirect checks; `undefined` means allow-all. + * + * `'none'` blocks the caller outright, unless `credentialOwnedSurface` says the URL comes from + * a node definition rather than from the user — set that only for such callers, never for a + * URL that arrives as request input. + * + * On such a surface, pass `nodeEndpointUrl` and its host joins the `'domains'` allowlist, so a + * list the user wrote with the HTTP Request node in mind does not stop the credential working in + * the node it belongs to. A node that decides its endpoint later — in `preSend`, or in the + * credential's own `authenticate` — has no host to offer here and stays subject to the list. + */ export function getCredentialAllowedDomains(options: { node: INode; credentialData: ICredentialDataDecryptedObject; surface?: string; + credentialOwnedSurface?: boolean; + nodeEndpointUrl?: string; }): string | undefined { - const { node, credentialData, surface = DEFAULT_SURFACE } = options; + const { + node, + credentialData, + surface = DEFAULT_SURFACE, + credentialOwnedSurface, + nodeEndpointUrl, + } = options; const mode = readMode(credentialData); + // Guarded on the flag: a surface where the user picks the URL must never widen its allowlist. + const endpointHost = credentialOwnedSurface ? toHostname(nodeEndpointUrl) : undefined; + // A comma is a legal host character and would split into extra allowlist entries. + const ownHost = endpointHost?.includes(',') ? undefined : endpointHost; if (mode === 'none') { - throw new NodeOperationError( - node, - `This credential is configured to prevent use within an ${surface} node`, - ); + if (!credentialOwnedSurface) { + throw new NodeOperationError( + node, + `This credential is configured to prevent use within an ${surface} node`, + ); + } + return undefined; } if (mode === 'domains') { const allowedDomains = readAllowedDomainsField(credentialData); if (!allowedDomains) { + if (ownHost) return ownHost; throw new NodeOperationError( node, 'No allowed domains specified. Configure allowed domains or change restriction setting.', ); } - return allowedDomains; + if (!ownHost || isDomainAllowed({ url: `https://${ownHost}`, allowedDomains })) { + return allowedDomains; + } + return `${ownHost}, ${allowedDomains}`; } return undefined; diff --git a/packages/workflow/test/credential-domain-restrictions.test.ts b/packages/workflow/test/credential-domain-restrictions.test.ts index 9212533e034..4a4b4d577af 100644 --- a/packages/workflow/test/credential-domain-restrictions.test.ts +++ b/packages/workflow/test/credential-domain-restrictions.test.ts @@ -278,6 +278,227 @@ describe('getCredentialAllowedDomains', () => { }), ).toThrow('No allowed domains specified'); }); + + describe('credentialOwnedSurface', () => { + const noneCredential = { allowedHttpRequestDomains: 'none' }; + + it("adds the node's own host to a 'domains' allowlist that omits it", () => { + expect( + getCredentialAllowedDomains({ + node, + credentialData: { + allowedHttpRequestDomains: 'domains', + allowedDomains: 'other.example.com', + }, + credentialOwnedSurface: true, + nodeEndpointUrl: 'https://api.example.com/v2', + }), + ).toBe('api.example.com, other.example.com'); + }); + + it("does not duplicate the node's own host when the allowlist already covers it", () => { + expect( + getCredentialAllowedDomains({ + node, + credentialData: { + allowedHttpRequestDomains: 'domains', + allowedDomains: 'other.example.com, API.Example.com', + }, + credentialOwnedSurface: true, + nodeEndpointUrl: 'https://api.example.com/v2', + }), + ).toBe('other.example.com, API.Example.com'); + }); + + it('strips the port from the node host it adds', () => { + expect( + getCredentialAllowedDomains({ + node, + credentialData: { + allowedHttpRequestDomains: 'domains', + allowedDomains: 'other.example.com', + }, + credentialOwnedSurface: true, + nodeEndpointUrl: 'https://api.example.com:8443/v2', + }), + ).toBe('api.example.com, other.example.com'); + }); + + it('does not add a host containing a comma, which would split the list', () => { + expect( + getCredentialAllowedDomains({ + node, + credentialData: { + allowedHttpRequestDomains: 'domains', + allowedDomains: 'other.example.com', + }, + credentialOwnedSurface: true, + nodeEndpointUrl: 'https://one,two.example.com/v2', + }), + ).toBe('other.example.com'); + }); + + it('treats a wildcard entry as already covering the node host', () => { + expect( + getCredentialAllowedDomains({ + node, + credentialData: { + allowedHttpRequestDomains: 'domains', + allowedDomains: '*.example.com', + }, + credentialOwnedSurface: true, + nodeEndpointUrl: 'https://api.example.com/v2', + }), + ).toBe('*.example.com'); + }); + + it("falls back to the node's own host when the 'domains' list is empty", () => { + expect( + getCredentialAllowedDomains({ + node, + credentialData: { allowedHttpRequestDomains: 'domains', allowedDomains: ' ' }, + credentialOwnedSurface: true, + nodeEndpointUrl: 'https://api.example.com/v2', + }), + ).toBe('api.example.com'); + }); + + it("still rejects an empty 'domains' list when there is no node host to fall back on", () => { + expect(() => + getCredentialAllowedDomains({ + node, + credentialData: { allowedHttpRequestDomains: 'domains', allowedDomains: '' }, + credentialOwnedSurface: true, + }), + ).toThrow('No allowed domains specified'); + }); + + it.each([ + ['explicitly false', false], + ['undefined', undefined], + ])( + 'never widens the allowlist when credentialOwnedSurface is %s', + (_label, credentialOwnedSurface) => { + expect( + getCredentialAllowedDomains({ + node, + credentialData: { + allowedHttpRequestDomains: 'domains', + allowedDomains: 'other.example.com', + }, + credentialOwnedSurface, + nodeEndpointUrl: 'https://api.example.com/v2', + }), + ).toBe('other.example.com'); + }, + ); + + it.each([ + ['an unresolved expression', '={{$credentials.baseUrl}}'], + ['a relative path', '/api/v2'], + ['a scheme with no host', 'https://'], + ['an empty string', ''], + ['undefined', undefined], + ])('widens nothing when the node endpoint is %s', (_label, nodeEndpointUrl) => { + expect( + getCredentialAllowedDomains({ + node, + credentialData: { + allowedHttpRequestDomains: 'domains', + allowedDomains: 'other.example.com', + }, + credentialOwnedSurface: true, + nodeEndpointUrl, + }), + ).toBe('other.example.com'); + }); + + it("ignores nodeEndpointUrl for 'none'", () => { + expect( + getCredentialAllowedDomains({ + node, + credentialData: { allowedHttpRequestDomains: 'none' }, + credentialOwnedSurface: true, + nodeEndpointUrl: 'https://api.example.com/v2', + }), + ).toBeUndefined(); + }); + + it("ignores nodeEndpointUrl for 'all'", () => { + expect( + getCredentialAllowedDomains({ + node, + credentialData: { allowedHttpRequestDomains: 'all' }, + credentialOwnedSurface: true, + nodeEndpointUrl: 'https://api.example.com/v2', + }), + ).toBeUndefined(); + }); + + it("leaves 'none' unrestricted for a credential-owned surface", () => { + expect( + getCredentialAllowedDomains({ + node, + credentialData: noneCredential, + credentialOwnedSurface: true, + }), + ).toBeUndefined(); + }); + + it.each([ + ['explicitly false', false], + ['undefined', undefined], + ])( + "still blocks 'none' when credentialOwnedSurface is %s", + (_label, credentialOwnedSurface) => { + expect(() => + getCredentialAllowedDomains({ + node, + credentialData: noneCredential, + credentialOwnedSurface, + }), + ).toThrow('This credential is configured to prevent use within an HTTP Request node'); + }, + ); + + it("still blocks 'none' when the flag is omitted entirely", () => { + expect(() => getCredentialAllowedDomains({ node, credentialData: noneCredential })).toThrow( + 'This credential is configured to prevent use within an HTTP Request node', + ); + }); + + it("still enforces the 'domains' allowlist on a credential-owned surface", () => { + expect( + getCredentialAllowedDomains({ + node, + credentialData: { + allowedHttpRequestDomains: 'domains', + allowedDomains: 'api.example.com', + }, + credentialOwnedSurface: true, + }), + ).toBe('api.example.com'); + }); + + it("still rejects an empty 'domains' allowlist on a credential-owned surface", () => { + expect(() => + getCredentialAllowedDomains({ + node, + credentialData: { allowedHttpRequestDomains: 'domains', allowedDomains: ' ' }, + credentialOwnedSurface: true, + }), + ).toThrow('No allowed domains specified'); + }); + + it.each([ + ["'all'", { allowedHttpRequestDomains: 'all' }], + ['an absent mode', {}], + ])('returns undefined for %s on a credential-owned surface', (_label, credentialData) => { + expect( + getCredentialAllowedDomains({ node, credentialData, credentialOwnedSurface: true }), + ).toBeUndefined(); + }); + }); }); describe('assertUrlAllowed', () => {