fix(core): Simplify Websocket origin security checks (#15761)

Co-authored-by: Danny Martini <danny@n8n.io>

packages/cli/src/push/__tests__/index.test.ts

@@ -144,121 +144,85 @@ describe('Push', () => {
 
 			describe('should throw on invalid origin', () => {
 				test.each([
+					{
+						name: 'origin is undefined',
+						origin: undefined,
+						xForwardedHost: undefined,
+					},
 					{
 						name: 'origin does not match host',
-						origin: 'https://subdomain.example.com',
+						origin: 'https://123example.com',
-						xForwardedProto: undefined,
 						xForwardedHost: undefined,
 					},
 					{
 						name: 'origin does not match host (subdomain)',
-						origin: 'https://123example.com',
+						origin: `https://subdomain.${host}`,
-						xForwardedProto: undefined,
 						xForwardedHost: undefined,
 					},
 					{
-						name: 'origin is not defined',
+						name: 'origin does not match x-forwarded-host',
-						origin: undefined,
+						origin: `https://${host}`, // this is correct
-						xForwardedProto: undefined,
+						xForwardedHost: 'https://123example.com', // this is not
-						xForwardedHost: undefined,
 					},
 					{
-						name: 'only one of the forward headers is defined',
+						name: 'origin does not match x-forwarded-host (subdomain)',
-						origin: 'https://123example.com',
+						origin: `https://${host}`, // this is correct
-						xForwardedProto: 'https',
+						xForwardedHost: `https://subdomain.${host}`, // this is not
-						xForwardedHost: undefined,
 					},
+				])('$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` is set correctly', () => {
+				test.each([
 					{
-						name: 'only one of the forward headers is defined',
+						name: 'origin matches forward headers (https)',
-						origin: 'https://123example.com',
+						origin: `https://${host}`,
-						xForwardedProto: undefined,
-						xForwardedHost: '123example.com',
-					},
-					{
-						name: 'protocol mismatch',
-						// correct origin, but forward headers take precedence
-						origin: 'https://example.com',
-						xForwardedProto: 'http',
 						xForwardedHost: host,
 					},
 					{
-						name: 'origin is undefined',
+						name: 'origin matches forward headers (http)',
-						origin: undefined,
+						origin: `http://${host}`,
-						xForwardedProto: undefined,
+						xForwardedHost: host,
-						xForwardedHost: undefined,
-					},
-				])(
-					'$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();
-					},
-				);
-			});
-
-			describe('should not throw on invalid origin if `X-Forwarded-Host` and `X-Forwarded-Proto` are 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 but protocol has different case',
-						origin: 'HTTPS://example.com',
-						xForwardedProto: undefined,
-						xForwardedHost: undefined,
 					},
 					{
 						name: 'origin matches host (https)',
-						origin: 'https://example.com',
+						origin: `https://${host}`,
-						xForwardedProto: undefined,
 						xForwardedHost: undefined,
 					},
 					{
 						name: 'origin matches host (http)',
-						origin: 'http://example.com',
+						origin: `http://${host}`,
-						xForwardedProto: undefined,
 						xForwardedHost: undefined,
 					},
-				])(
+				])('$name', ({ origin, xForwardedHost }) => {
-					'$name (origin: $origin, x-forwarded-proto: $xForwardedProto, x-forwarded-host: $xForwardedHost)',
+					// ARRANGE
-					({ origin, xForwardedProto, xForwardedHost }) => {
+					req.headers.origin = origin;
-						// ARRANGE
+					req.headers['x-forwarded-host'] = xForwardedHost;
-						req.headers.origin = origin;
-						req.headers['x-forwarded-proto'] = xForwardedProto;
-						req.headers['x-forwarded-host'] = xForwardedHost;
 
-						const emitSpy = jest.spyOn(push, 'emit');
+					const emitSpy = jest.spyOn(push, 'emit');
-						const connection = backendName === 'sse' ? { req, res } : ws;
+					const connection = backendName === 'sse' ? { req, res } : ws;
 
-						// ACT
+					// ACT
-						push.handleRequest(req, res);
+					push.handleRequest(req, res);
 
-						// ASSERT
+					// ASSERT
-						expect(backend.add).toHaveBeenCalledWith(pushRef, user.id, connection);
+					expect(backend.add).toHaveBeenCalledWith(pushRef, user.id, connection);
-						expect(emitSpy).toHaveBeenCalledWith('editorUiConnected', pushRef);
+					expect(emitSpy).toHaveBeenCalledWith('editorUiConnected', pushRef);
-					},
+				});
-				);
 			});
 
 			test('should throw if pushRef is invalid', () => {

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<PushEvents> {
 		}
 	}
 
-	/** 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<PushEvents> {
 		);
 	}
 
-	/**
-	 * 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<PushEvents> {
 
 		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 (
+		} else if (inProduction) {
-			inProduction &&
+			const expectedHost =
-			headers.origin?.toLowerCase() !== expectedOriginResult.expectedOrigin.toLowerCase()
+				typeof headers['x-forwarded-host'] === 'string'
-		) {
+					? headers['x-forwarded-host']
-			this.logger.warn(
+					: headers.host;
-				`Origin header does NOT match the expected origin. (Origin: "${headers.origin}", Expected: "${expectedOriginResult.expectedOrigin}")`,
+			if (expectedHost !== originHost) {
-				{
+				this.logger.warn(
-					expectedOrigin: expectedOriginResult.expectedOrigin,
+					`Origin header does NOT match the expected origin. (Origin: "${originHost}", Expected: "${expectedHost}")`,
-					headers: {
+					{ headers: pick(headers, ['host', 'origin', 'x-forwarded-proto', 'x-forwarded-host']) },
-						host: req.headers.host,
+				);
-						origin: req.headers.origin,
+				connectionError = 'Invalid origin!';
-						['x-forwarded-proto']: req.headers['x-forwarded-proto'],
+			}
-						['x-forwarded-host']: req.headers['x-forwarded-host'],
-					},
-				},
-			);
-			connectionError = 'Invalid origin!';
 		}
 
 		if (connectionError) {