diff --git a/packages/cli/src/push/__tests__/index.test.ts b/packages/cli/src/push/__tests__/index.test.ts index afce756abf82a..16e321f694322 100644 --- a/packages/cli/src/push/__tests__/index.test.ts +++ b/packages/cli/src/push/__tests__/index.test.ts @@ -145,120 +145,84 @@ describe('Push', () => { describe('should throw on invalid origin', () => { test.each([ { - name: 'origin does not match host', - origin: 'https://subdomain.example.com', - xForwardedProto: undefined, - xForwardedHost: undefined, - }, - { - name: 'origin does not match host (subdomain)', - origin: 'https://123example.com', - xForwardedProto: undefined, - xForwardedHost: undefined, - }, - { - name: 'origin is not defined', + name: 'origin is undefined', origin: undefined, - xForwardedProto: undefined, xForwardedHost: undefined, }, { - name: 'only one of the forward headers is defined', + name: 'origin does not match host', origin: 'https://123example.com', - xForwardedProto: 'https', xForwardedHost: undefined, }, { - name: 'only one of the forward headers is defined', - origin: 'https://123example.com', - xForwardedProto: undefined, - xForwardedHost: '123example.com', + name: 'origin does not match host (subdomain)', + origin: `https://subdomain.${host}`, + xForwardedHost: undefined, }, { - name: 'protocol mismatch', - // correct origin, but forward headers take precedence - origin: 'https://example.com', - xForwardedProto: 'http', - xForwardedHost: host, + name: 'origin does not match x-forwarded-host', + origin: `https://${host}`, // this is correct + xForwardedHost: 'https://123example.com', // this is not }, { - name: 'origin is undefined', - origin: undefined, - xForwardedProto: undefined, - xForwardedHost: undefined, + name: 'origin does not match x-forwarded-host (subdomain)', + origin: `https://${host}`, // this is correct + xForwardedHost: `https://subdomain.${host}`, // this is not }, - ])( - '$name (origin: $origin, x-forwarded-proto: $xForwardedProto, x-forwarded-host: $xForwardedHost)', - ({ origin, xForwardedProto, xForwardedHost }) => { - req.headers.origin = origin; - req.headers['x-forwarded-proto'] = xForwardedProto; - req.headers['x-forwarded-host'] = xForwardedHost; - - if (backendName === 'sse') { - expect(() => push.handleRequest(req, res)).toThrow( - new BadRequestError('Invalid origin!'), - ); - } else { - push.handleRequest(req, res); - expect(ws.send).toHaveBeenCalledWith('Invalid origin!'); - expect(ws.close).toHaveBeenCalledWith(1008); - } - expect(backend.add).not.toHaveBeenCalled(); - }, - ); + ])('$name', ({ origin, xForwardedHost }) => { + req.headers.origin = origin; + req.headers['x-forwarded-host'] = xForwardedHost; + + if (backendName === 'sse') { + expect(() => push.handleRequest(req, res)).toThrow( + new BadRequestError('Invalid origin!'), + ); + } else { + push.handleRequest(req, res); + expect(ws.send).toHaveBeenCalledWith('Invalid origin!'); + expect(ws.close).toHaveBeenCalledWith(1008); + } + expect(backend.add).not.toHaveBeenCalled(); + }); }); - describe('should not throw on invalid origin if `X-Forwarded-Host` and `X-Forwarded-Proto` are set correctly', () => { + describe('should not throw on invalid origin if `X-Forwarded-Host` is set correctly', () => { test.each([ { - name: 'origin matches forward headers', - origin: 'https://example.com', - xForwardedProto: 'https', - xForwardedHost: 'example.com', - }, - { - name: 'origin matches forward headers but has different case', - origin: 'https://example.com', - xForwardedProto: 'https', - xForwardedHost: 'EXAMPLE.com', + name: 'origin matches forward headers (https)', + origin: `https://${host}`, + xForwardedHost: host, }, { - name: 'origin matches forward headers but protocol has different case', - origin: 'HTTPS://example.com', - xForwardedProto: undefined, - xForwardedHost: undefined, + name: 'origin matches forward headers (http)', + origin: `http://${host}`, + xForwardedHost: host, }, { name: 'origin matches host (https)', - origin: 'https://example.com', - xForwardedProto: undefined, + origin: `https://${host}`, xForwardedHost: undefined, }, { name: 'origin matches host (http)', - origin: 'http://example.com', - xForwardedProto: undefined, + origin: `http://${host}`, xForwardedHost: undefined, }, - ])( - '$name (origin: $origin, x-forwarded-proto: $xForwardedProto, x-forwarded-host: $xForwardedHost)', - ({ origin, xForwardedProto, xForwardedHost }) => { - // ARRANGE - req.headers.origin = origin; - req.headers['x-forwarded-proto'] = xForwardedProto; - req.headers['x-forwarded-host'] = xForwardedHost; - - const emitSpy = jest.spyOn(push, 'emit'); - const connection = backendName === 'sse' ? { req, res } : ws; - - // ACT - push.handleRequest(req, res); + ])('$name', ({ origin, xForwardedHost }) => { + // ARRANGE + req.headers.origin = origin; + req.headers['x-forwarded-host'] = xForwardedHost; - // ASSERT - expect(backend.add).toHaveBeenCalledWith(pushRef, user.id, connection); - expect(emitSpy).toHaveBeenCalledWith('editorUiConnected', pushRef); - }, - ); + const emitSpy = jest.spyOn(push, 'emit'); + const connection = backendName === 'sse' ? { req, res } : ws; + + // ACT + push.handleRequest(req, res); + + // ASSERT + expect(backend.add).toHaveBeenCalledWith(pushRef, user.id, connection); + expect(emitSpy).toHaveBeenCalledWith('editorUiConnected', pushRef); + }); }); test('should throw if pushRef is invalid', () => { diff --git a/packages/cli/src/push/index.ts b/packages/cli/src/push/index.ts index 84b7d598284d3..2107cb4609284 100644 --- a/packages/cli/src/push/index.ts +++ b/packages/cli/src/push/index.ts @@ -6,6 +6,7 @@ import { Container, Service } from '@n8n/di'; import type { Application } from 'express'; import { ServerResponse } from 'http'; import type { Server } from 'http'; +import { pick } from 'lodash'; import { InstanceSettings, Logger } from 'n8n-core'; import { deepCopy } from 'n8n-workflow'; import { parse as parseUrl } from 'url'; @@ -89,7 +90,7 @@ export class Push extends TypedEmitter { } } - /** Sets up the push endppoint that the frontend connects to. */ + /** Sets up the push endpoint that the frontend connects to. */ setupPushHandler(restEndpoint: string, app: Application) { app.use( `/${restEndpoint}/push`, @@ -100,35 +101,6 @@ export class Push extends TypedEmitter { ); } - /** - * Construct the expected origin out of the host and forward headers. - * If `x-forwarded-host` and `x-forwarded-proto` are both defined they take - * precedence over `host`. - * If they are not both defined then `host` is used and the protocol is - * inferred from `origin`. - */ - private constructExpectedOrigin(req: SSEPushRequest | WebSocketPushRequest) { - const headers = req.headers; - - if (headers.origin) { - const forwardedHost = - typeof headers['x-forwarded-host'] === 'string' ? headers['x-forwarded-host'] : undefined; - const forwardedProto = - typeof headers['x-forwarded-proto'] === 'string' ? headers['x-forwarded-proto'] : undefined; - const allForwardHeadersAreDefined = forwardedHost && forwardedProto; - const host = allForwardHeadersAreDefined ? forwardedHost : headers.host; - const proto = allForwardHeadersAreDefined - ? forwardedProto - : headers.origin?.toLowerCase().startsWith('https://') - ? 'https' - : 'http'; - - return { success: true, expectedOrigin: `${proto}://${host}` } as const; - } else { - return { success: false } as const; - } - } - handleRequest(req: SSEPushRequest | WebSocketPushRequest, res: PushResponse) { const { ws, @@ -139,31 +111,27 @@ export class Push extends TypedEmitter { let connectionError = ''; - const expectedOriginResult = this.constructExpectedOrigin(req); + // Extract host domain from origin + const originHost = headers.origin?.replace(/^https?:\/\//, ''); if (!pushRef) { connectionError = 'The query parameter "pushRef" is missing!'; - } else if (!expectedOriginResult.success) { + } else if (!originHost) { this.logger.warn('Origin header is missing'); connectionError = 'Invalid origin!'; - } else if ( - inProduction && - headers.origin?.toLowerCase() !== expectedOriginResult.expectedOrigin.toLowerCase() - ) { - this.logger.warn( - `Origin header does NOT match the expected origin. (Origin: "${headers.origin}", Expected: "${expectedOriginResult.expectedOrigin}")`, - { - expectedOrigin: expectedOriginResult.expectedOrigin, - headers: { - host: req.headers.host, - origin: req.headers.origin, - ['x-forwarded-proto']: req.headers['x-forwarded-proto'], - ['x-forwarded-host']: req.headers['x-forwarded-host'], - }, - }, - ); - connectionError = 'Invalid origin!'; + } else if (inProduction) { + const expectedHost = + typeof headers['x-forwarded-host'] === 'string' + ? headers['x-forwarded-host'] + : headers.host; + if (expectedHost !== originHost) { + this.logger.warn( + `Origin header does NOT match the expected origin. (Origin: "${originHost}", Expected: "${expectedHost}")`, + { headers: pick(headers, ['host', 'origin', 'x-forwarded-proto', 'x-forwarded-host']) }, + ); + connectionError = 'Invalid origin!'; + } } if (connectionError) {