From a63e88e7c1922c53dff0839cde2b5227ce71ae44 Mon Sep 17 00:00:00 2001 From: Pierantonio Cangianiello <10360978+pcan@users.noreply.github.com> Date: Thu, 10 Sep 2026 07:39:19 +0000 Subject: [PATCH] refactor(engine): Make the control plane token scope a parameter on both sides (no-changelog) (#37978) --- ...gine-control-plane-auth.middleware.test.ts | 33 ++++- .../engine-control-plane-client.test.ts | 77 ++---------- .../engine-control-plane-server.test.ts | 4 + .../engine-control-plane-transport.test.ts | 118 ++++++++++++++++++ .../engine-control-plane-auth.middleware.ts | 13 +- .../engine-v2/engine-control-plane-client.ts | 21 +--- .../engine-v2/engine-control-plane-server.ts | 18 +-- .../engine-control-plane-transport.ts | 37 ++++++ 8 files changed, 219 insertions(+), 102 deletions(-) create mode 100644 packages/cli/src/modules/engine-v2/__tests__/engine-control-plane-transport.test.ts create mode 100644 packages/cli/src/modules/engine-v2/engine-control-plane-transport.ts diff --git a/packages/cli/src/modules/engine-v2/__tests__/engine-control-plane-auth.middleware.test.ts b/packages/cli/src/modules/engine-v2/__tests__/engine-control-plane-auth.middleware.test.ts index dd718af5c92..b2612eabe2a 100644 --- a/packages/cli/src/modules/engine-v2/__tests__/engine-control-plane-auth.middleware.test.ts +++ b/packages/cli/src/modules/engine-v2/__tests__/engine-control-plane-auth.middleware.test.ts @@ -1,5 +1,6 @@ import type { Logger } from '@n8n/backend-common'; import type { EngineConfig } from '@n8n/config'; +import type { ActionScope } from '@n8n/engine'; import { ACTION_TOKEN, mintActionToken, mintIdentityToken } from '@n8n/engine'; import type { NextFunction, Request, Response } from 'express'; import jwt from 'jsonwebtoken'; @@ -29,11 +30,16 @@ describe('createEngineControlPlaneAuthMiddleware', () => { ), }) as unknown as Request; - const authenticate = (authorization?: string, secret = authSecret) => { + const authenticate = ( + authorization?: string, + secret = authSecret, + requiredScope: ActionScope = 'lifecycle-events:write', + ) => { const res = newResponse(); const middleware = createEngineControlPlaneAuthMiddleware( mock({ authSecret: secret }), logger, + requiredScope, ); middleware(newRequest(authorization), res, next); return res; @@ -60,7 +66,11 @@ describe('createEngineControlPlaneAuthMiddleware', () => { it('reads the secret per request, so one generated after startup still works', () => { // The secret is set after construction. const engineConfig = mock({ authSecret: '' }); - const middleware = createEngineControlPlaneAuthMiddleware(engineConfig, logger); + const middleware = createEngineControlPlaneAuthMiddleware( + engineConfig, + logger, + 'lifecycle-events:write', + ); engineConfig.authSecret = authSecret; middleware( @@ -87,7 +97,24 @@ describe('createEngineControlPlaneAuthMiddleware', () => { expectRejected(authenticate(`Bearer ${token}`)); }); - it('rejects a token carrying a different scope', () => { + it('accepts a credential read token on a route that requires that scope', () => { + const token = mintActionToken(authSecret, 'credentials:read'); + const res = authenticate(`Bearer ${token}`, authSecret, 'credentials:read'); + + expect(next).toHaveBeenCalled(); + expect(res.status).not.toHaveBeenCalled(); + }); + + it.each<[ActionScope, ActionScope]>([ + ['credentials:read', 'lifecycle-events:write'], + ['lifecycle-events:write', 'credentials:read'], + ])('rejects a %s token on a route that requires %s', (minted, required) => { + const token = mintActionToken(authSecret, minted); + + expectRejected(authenticate(`Bearer ${token}`, authSecret, required)); + }); + + it('rejects a token with a scope the engine does not know', () => { const token = jwt.sign({ scope: 'credential:read' }, authSecret, { algorithm: 'HS256', issuer: ACTION_TOKEN.issuer, diff --git a/packages/cli/src/modules/engine-v2/__tests__/engine-control-plane-client.test.ts b/packages/cli/src/modules/engine-v2/__tests__/engine-control-plane-client.test.ts index 0d977951f17..b44d2bc1614 100644 --- a/packages/cli/src/modules/engine-v2/__tests__/engine-control-plane-client.test.ts +++ b/packages/cli/src/modules/engine-v2/__tests__/engine-control-plane-client.test.ts @@ -1,17 +1,10 @@ -import type { - HttpRequestClient, - HttpRequestClientOptions, - OutboundHttp, -} from '@n8n/backend-network'; -import type { EngineConfig } from '@n8n/config'; -import type { LifecycleEvent } from '@n8n/engine'; -import { InvalidActionTokenError, verifyActionToken } from '@n8n/engine'; +import type { HttpRequestClient } from '@n8n/backend-network'; +import type { ActionScope, LifecycleEvent } from '@n8n/engine'; import { OperationalError } from 'n8n-workflow'; import { mock } from 'vitest-mock-extended'; import { EngineControlPlaneClient } from '../engine-control-plane-client'; - -const authSecret = 'a'.repeat(32); +import type { EngineControlPlaneTransport } from '../engine-control-plane-transport'; const events: LifecycleEvent[] = [ { @@ -24,7 +17,7 @@ const events: LifecycleEvent[] = [ describe('EngineControlPlaneClient', () => { let http: HttpRequestClient; - let clientOptions: HttpRequestClientOptions | undefined; + let requestedScope: ActionScope | undefined; let client: EngineControlPlaneClient; let signal: AbortSignal; @@ -32,29 +25,15 @@ describe('EngineControlPlaneClient', () => { vi.mocked(http.request).mockResolvedValue({ statusCode, body: '', headers: {} }); }; - /** Rebuilds the client so each test can vary the config. */ - const newClient = (engineConfig: Partial = {}) => { + beforeEach(() => { http = mock(); - const outboundHttp = mock({ - requests: vi.fn((options?: HttpRequestClientOptions) => { - clientOptions = options; + const transport = mock({ + forScope: vi.fn((scope: ActionScope) => { + requestedScope = scope; return http; }), }); - - return new EngineControlPlaneClient( - mock({ - controlPlaneBaseUrl: '', - authSecret, - controlPlanePort: 3001, - ...engineConfig, - }), - outboundHttp, - ); - }; - - beforeEach(() => { - client = newClient(); + client = new EngineControlPlaneClient(transport); signal = new AbortController().signal; }); @@ -84,23 +63,8 @@ describe('EngineControlPlaneClient', () => { ); }); - it('dials the control plane server on the loopback, not n8n main', () => { - expect(clientOptions?.baseURL).toBe('http://127.0.0.1:3001'); - }); - - it('dials the configured base URL when the control plane answers elsewhere', () => { - newClient({ controlPlaneBaseUrl: 'https://cp.internal:8443' }); - - expect(clientOptions?.baseURL).toBe('https://cp.internal:8443'); - }); - - it('opts out of SSRF protection for the n8n-controlled host', () => { - expect(clientOptions?.useDefaultSsrfPolicy).toBe('unsafe'); - }); - - it('leaves the send deadline to the engine, which owns it', () => { - // A client timeout would fire first and hide the engine's deadline. - expect(clientOptions?.timeout).toBeUndefined(); + it('asks the transport for a client scoped to lifecycle-event writes', () => { + expect(requestedScope).toBe('lifecycle-events:write'); }); it("forwards the engine's abort signal, so an abandoned batch cancels its request", async () => { @@ -111,25 +75,6 @@ describe('EngineControlPlaneClient', () => { expect(http.request).toHaveBeenCalledWith(expect.objectContaining({ abortSignal: signal })); }); - it('mints a fresh token per request', () => { - expect(typeof clientOptions?.headers).toBe('function'); - }); - - it('sends an action token scoped to lifecycle-event writes that the control plane accepts', () => { - const headers = clientOptions?.headers; - const resolved = typeof headers === 'function' ? headers() : headers; - const authorization = resolved?.authorization ?? ''; - - expect(authorization).toMatch(/^Bearer .+/); - - const token = authorization.replace('Bearer ', ''); - - expect(() => verifyActionToken(authSecret, token, 'lifecycle-events:write')).not.toThrow(); - expect(() => verifyActionToken('b'.repeat(32), token, 'lifecycle-events:write')).toThrow( - InvalidActionTokenError, - ); - }); - it.each([302, 400, 401, 500])( 'rejects a batch the control plane answered %s', async (statusCode) => { diff --git a/packages/cli/src/modules/engine-v2/__tests__/engine-control-plane-server.test.ts b/packages/cli/src/modules/engine-v2/__tests__/engine-control-plane-server.test.ts index b8d182e9977..2ba7336b196 100644 --- a/packages/cli/src/modules/engine-v2/__tests__/engine-control-plane-server.test.ts +++ b/packages/cli/src/modules/engine-v2/__tests__/engine-control-plane-server.test.ts @@ -108,6 +108,10 @@ describe('EngineControlPlaneServer', () => { 'a token signed with a different secret', mintActionToken('b'.repeat(32), 'lifecycle-events:write'), ], + [ + 'an action token minted for credential reads', + mintActionToken(authSecret, 'credentials:read'), + ], ])('rejects %s', async (_label, token) => { const response = await post({ events }, token); diff --git a/packages/cli/src/modules/engine-v2/__tests__/engine-control-plane-transport.test.ts b/packages/cli/src/modules/engine-v2/__tests__/engine-control-plane-transport.test.ts new file mode 100644 index 00000000000..ba4ee299f98 --- /dev/null +++ b/packages/cli/src/modules/engine-v2/__tests__/engine-control-plane-transport.test.ts @@ -0,0 +1,118 @@ +import type { + HttpRequestClient, + HttpRequestClientOptions, + OutboundHttp, +} from '@n8n/backend-network'; +import type { EngineConfig } from '@n8n/config'; +import type { ActionScope } from '@n8n/engine'; +import { InvalidActionTokenError, verifyActionToken } from '@n8n/engine'; +import { mock } from 'vitest-mock-extended'; + +import { EngineControlPlaneTransport } from '../engine-control-plane-transport'; + +const authSecret = 'a'.repeat(32); + +describe('EngineControlPlaneTransport', () => { + let http: HttpRequestClient; + let clientOptions: HttpRequestClientOptions | undefined; + + let engineConfig: EngineConfig; + + /** Rebuilds the transport so each test can vary the config. */ + const newTransport = (overrides: Partial = {}) => { + http = mock(); + const outboundHttp = mock({ + requests: vi.fn((options?: HttpRequestClientOptions) => { + clientOptions = options; + return http; + }), + }); + engineConfig = mock({ + controlPlaneBaseUrl: '', + authSecret, + controlPlanePort: 3001, + ...overrides, + }); + + return new EngineControlPlaneTransport(engineConfig, outboundHttp); + }; + + /** Resolves the authorization header the way the HTTP client does per request. */ + const mintedToken = () => { + const headers = clientOptions?.headers; + const resolved = typeof headers === 'function' ? headers() : headers; + const authorization = resolved?.authorization ?? ''; + + expect(authorization).toMatch(/^Bearer .+/); + + return authorization.replace('Bearer ', ''); + }; + + describe('forScope', () => { + it('dials the control plane server on the loopback, not n8n main', () => { + newTransport().forScope('lifecycle-events:write'); + + expect(clientOptions?.baseURL).toBe('http://127.0.0.1:3001'); + }); + + it('dials the configured base URL when the control plane answers elsewhere', () => { + newTransport({ controlPlaneBaseUrl: 'https://cp.internal:8443' }).forScope( + 'lifecycle-events:write', + ); + + expect(clientOptions?.baseURL).toBe('https://cp.internal:8443'); + }); + + it('opts out of SSRF protection for the n8n-controlled host', () => { + newTransport().forScope('lifecycle-events:write'); + + expect(clientOptions?.useDefaultSsrfPolicy).toBe('unsafe'); + }); + + it('sets no client timeout, so the caller owns the deadline', () => { + newTransport().forScope('lifecycle-events:write'); + + expect(clientOptions?.timeout).toBeUndefined(); + }); + + it('mints a fresh token per request', () => { + newTransport().forScope('lifecycle-events:write'); + + expect(typeof clientOptions?.headers).toBe('function'); + }); + + it.each(['lifecycle-events:write', 'credentials:read'])( + 'mints a token with the requested scope %s and no other', + (scope) => { + newTransport().forScope(scope); + const token = mintedToken(); + const otherScope: ActionScope = + scope === 'credentials:read' ? 'lifecycle-events:write' : 'credentials:read'; + + expect(() => verifyActionToken(authSecret, token, scope)).not.toThrow(); + expect(() => verifyActionToken(authSecret, token, otherScope)).toThrow( + InvalidActionTokenError, + ); + }, + ); + + it('signs the token with the configured secret', () => { + newTransport().forScope('lifecycle-events:write'); + const token = mintedToken(); + + expect(() => verifyActionToken('b'.repeat(32), token, 'lifecycle-events:write')).toThrow( + InvalidActionTokenError, + ); + }); + + it('reads the secret at request time, after the module generated it', () => { + newTransport({ authSecret: '' }).forScope('lifecycle-events:write'); + + engineConfig.authSecret = authSecret; + + expect(() => + verifyActionToken(authSecret, mintedToken(), 'lifecycle-events:write'), + ).not.toThrow(); + }); + }); +}); diff --git a/packages/cli/src/modules/engine-v2/engine-control-plane-auth.middleware.ts b/packages/cli/src/modules/engine-v2/engine-control-plane-auth.middleware.ts index b776d2def89..b73933447a4 100644 --- a/packages/cli/src/modules/engine-v2/engine-control-plane-auth.middleware.ts +++ b/packages/cli/src/modules/engine-v2/engine-control-plane-auth.middleware.ts @@ -1,5 +1,6 @@ import type { Logger } from '@n8n/backend-common'; import type { EngineConfig } from '@n8n/config'; +import type { ActionScope } from '@n8n/engine'; import { InvalidActionTokenError, verifyActionToken } from '@n8n/engine'; import type { NextFunction, Request, Response } from 'express'; @@ -11,12 +12,14 @@ const BEARER_PREFIX = /^bearer /i; type SyncRequestHandler = (req: Request, res: Response, next: NextFunction) => void; /** - * Rejects a caller the shared secret does not vouch for. Reads the secret per - * request, because it is generated after this is constructed. + * Rejects a caller the shared secret does not vouch for, and a caller whose + * token was minted for another scope. Reads the secret per request, because it + * is generated after this is constructed. */ export function createEngineControlPlaneAuthMiddleware( engineConfig: EngineConfig, logger: Logger, + requiredScope: ActionScope, ): SyncRequestHandler { const reject = (req: Request, res: Response, reason: string): void => { // Logged, never returned: an operator needs the reason, a caller must not. @@ -33,11 +36,7 @@ export function createEngineControlPlaneAuthMiddleware( } try { - verifyActionToken( - engineConfig.authSecret, - header.replace(BEARER_PREFIX, ''), - 'lifecycle-events:write', - ); + verifyActionToken(engineConfig.authSecret, header.replace(BEARER_PREFIX, ''), requiredScope); } catch (error) { if (!(error instanceof InvalidActionTokenError)) throw error; reject(req, res, 'token rejected'); diff --git a/packages/cli/src/modules/engine-v2/engine-control-plane-client.ts b/packages/cli/src/modules/engine-v2/engine-control-plane-client.ts index cda0ec7f876..ed68a42b224 100644 --- a/packages/cli/src/modules/engine-v2/engine-control-plane-client.ts +++ b/packages/cli/src/modules/engine-v2/engine-control-plane-client.ts @@ -1,11 +1,9 @@ import type { HttpRequestClient } from '@n8n/backend-network'; -import { OutboundHttp } from '@n8n/backend-network'; -import { EngineConfig } from '@n8n/config'; import { Service } from '@n8n/di'; import type { LifecycleEvent } from '@n8n/engine'; -import { mintActionToken } from '@n8n/engine'; import { OperationalError } from 'n8n-workflow'; +import { EngineControlPlaneTransport } from './engine-control-plane-transport'; import { STATUS_CALLBACK_PATH } from './engine-v2.constants'; /** @@ -16,21 +14,8 @@ import { STATUS_CALLBACK_PATH } from './engine-v2.constants'; export class EngineControlPlaneClient { private readonly http: HttpRequestClient; - constructor( - private readonly engineConfig: EngineConfig, - outboundHttp: OutboundHttp, - ) { - this.http = outboundHttp.requests({ - // Fixed, n8n-controlled host. - useDefaultSsrfPolicy: 'unsafe', - // A bind address is not dialable, so default to loopback. - baseURL: - engineConfig.controlPlaneBaseUrl || `http://127.0.0.1:${engineConfig.controlPlanePort}`, - // A factory: each request needs a fresh token, and the secret is set later. - headers: () => ({ - authorization: `Bearer ${mintActionToken(this.engineConfig.authSecret, 'lifecycle-events:write')}`, - }), - }); + constructor(transport: EngineControlPlaneTransport) { + this.http = transport.forScope('lifecycle-events:write'); } /** Throws on a refused batch; the engine decides what a failed delivery costs. */ diff --git a/packages/cli/src/modules/engine-v2/engine-control-plane-server.ts b/packages/cli/src/modules/engine-v2/engine-control-plane-server.ts index c6fed0d3b33..e1de122ade3 100644 --- a/packages/cli/src/modules/engine-v2/engine-control-plane-server.ts +++ b/packages/cli/src/modules/engine-v2/engine-control-plane-server.ts @@ -9,7 +9,7 @@ import { bodyParser, rawBodyReader } from '@/middlewares'; import { send } from '@/response-helper'; import { createEngineControlPlaneAuthMiddleware } from './engine-control-plane-auth.middleware'; -import { CONTROL_PLANE_PREFIX, STATUS_CALLBACK_PATH } from './engine-v2.constants'; +import { STATUS_CALLBACK_PATH } from './engine-v2.constants'; import { EngineLifecycleEventController } from './engine-lifecycle-event.controller'; /** @@ -86,16 +86,18 @@ export class EngineControlPlaneServer { res.status(200).json({ status: 'ok' }); }); - // On the prefix, not the route, so a later route cannot forget either. - app.use( - CONTROL_PLANE_PREFIX, - createEngineControlPlaneAuthMiddleware(this.engineConfig, this.logger), - ); + // Auth is per route, because each route requires its own token scope. It + // runs before the body parser, so an unauthenticated body is never read. // n8n's parser bounds the body by `N8N_PAYLOAD_SIZE_MAX`. - app.use(CONTROL_PLANE_PREFIX, rawBodyReader, bodyParser); - app.post( STATUS_CALLBACK_PATH, + createEngineControlPlaneAuthMiddleware( + this.engineConfig, + this.logger, + 'lifecycle-events:write', + ), + rawBodyReader, + bodyParser, send( async (req, res) => await this.lifecycleEventController.receiveLifecycleEvents(req, res), ), diff --git a/packages/cli/src/modules/engine-v2/engine-control-plane-transport.ts b/packages/cli/src/modules/engine-v2/engine-control-plane-transport.ts new file mode 100644 index 00000000000..e992c0d1bae --- /dev/null +++ b/packages/cli/src/modules/engine-v2/engine-control-plane-transport.ts @@ -0,0 +1,37 @@ +import type { HttpRequestClient } from '@n8n/backend-network'; +import { OutboundHttp } from '@n8n/backend-network'; +import { EngineConfig } from '@n8n/config'; +import { Service } from '@n8n/di'; +import type { ActionScope } from '@n8n/engine'; +import { mintActionToken } from '@n8n/engine'; + +/** + * Builds the HTTP clients that call the control plane server. + * + * Every caller needs the same base URL, SSRF policy and token minting, and + * differs only in the scope of the token. Configuring this once keeps the + * callers identical in everything but their scope. + */ +@Service() +export class EngineControlPlaneTransport { + constructor( + private readonly engineConfig: EngineConfig, + private readonly outboundHttp: OutboundHttp, + ) {} + + /** Returns a client that sends a fresh action token with `scope` on every request. */ + forScope(scope: ActionScope): HttpRequestClient { + return this.outboundHttp.requests({ + // Fixed, n8n-controlled host. + useDefaultSsrfPolicy: 'unsafe', + // A bind address is not dialable, so default to loopback. + baseURL: + this.engineConfig.controlPlaneBaseUrl || + `http://127.0.0.1:${this.engineConfig.controlPlanePort}`, + // A factory: each request needs a fresh token, and the secret is set later. + headers: () => ({ + authorization: `Bearer ${mintActionToken(this.engineConfig.authSecret, scope)}`, + }), + }); + } +}