Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+66 −134
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2ca46a4
fix(core): Simplify Websocket origin security checks
netroy May 26, 2025
95a8e5c
added tests and updates names
despairblue May 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
134 changes: 49 additions & 85 deletions packages/cli/src/push/__tests__/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down
66 changes: 17 additions & 49 deletions packages/cli/src/push/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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`,
Expand All @@ -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,
Expand All @@ -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 (
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) {
Expand Down
Loading