Skip to content
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.

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

fix(server): stricter headers security check #2092

Merged
merged 2 commits into from Jul 3, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 8 additions & 1 deletion lib/Server.js
Expand Up @@ -685,7 +685,14 @@ class Server {
return;
}

if (headers && (!this.checkHost(headers) || !this.checkOrigin(headers))) {
if (!headers) {
this.log.warn(
'serverMode implementation must pass headers to the callback of onConnection(f) ' +
'via f(connection, headers) in order for clients to pass a headers security check'
);
}

if (!headers || !this.checkHost(headers) || !this.checkOrigin(headers)) {
this.sockWrite([connection], 'error', 'Invalid Host/Origin header');

this.socketServer.close(connection);
Expand Down
2 changes: 1 addition & 1 deletion lib/servers/SockJSServer.js
Expand Up @@ -57,7 +57,7 @@ module.exports = class SockJSServer extends BaseServer {
connection.close();
}

// f should return the resulting connection and, optionally, the connection headers
// f should be passed the resulting connection and the connection headers
onConnection(f) {
this.socket.on('connection', (connection) => {
f(connection, connection.headers);
Expand Down
2 changes: 1 addition & 1 deletion lib/servers/WebsocketServer.js
Expand Up @@ -27,7 +27,7 @@ module.exports = class WebsocketServer extends BaseServer {
connection.close();
}

// f should return the resulting connection
// f should be passed the resulting connection and the connection headers
onConnection(f) {
this.wsServer.on('connection', (connection, req) => {
f(connection, req.headers);
Expand Down
8 changes: 8 additions & 0 deletions test/server/__snapshots__/serverMode-option.test.js.snap
Expand Up @@ -7,3 +7,11 @@ Array [
"close",
]
`;

exports[`serverMode option without a header results in an error 1`] = `
Array [
"open",
"{\\"type\\":\\"error\\",\\"data\\":\\"Invalid Host/Origin header\\"}",
"close",
]
`;
91 changes: 91 additions & 0 deletions test/server/serverMode-option.test.js
Expand Up @@ -223,4 +223,95 @@ describe('serverMode option', () => {
}, 5000);
});
});

describe('without a header', () => {
let mockWarn;
beforeAll((done) => {
server = testServer.start(
config,
{
port,
serverMode: class MySockJSServer extends BaseServer {
constructor(serv) {
super(serv);
this.socket = sockjs.createServer({
// Use provided up-to-date sockjs-client
sockjs_url: '/__webpack_dev_server__/sockjs.bundle.js',
// Limit useless logs
log: (severity, line) => {
if (severity === 'error') {
this.server.log.error(line);
} else {
this.server.log.debug(line);
}
},
});

this.socket.installHandlers(this.server.listeningApp, {
prefix: this.server.sockPath,
});
}

send(connection, message) {
connection.write(message);
}

close(connection) {
connection.close();
}

onConnection(f) {
this.socket.on('connection', (connection) => {
f(connection);
});
}

onConnectionClose(connection, f) {
connection.on('close', f);
}
},
},
done
);

mockWarn = jest.spyOn(server.log, 'warn').mockImplementation(() => {});
});

it('results in an error', (done) => {
const data = [];
const client = new SockJS(`http://localhost:${port}/sockjs-node`);

client.onopen = () => {
data.push('open');
};

client.onmessage = (e) => {
data.push(e.data);
};

client.onclose = () => {
data.push('close');
};

setTimeout(() => {
expect(data).toMatchSnapshot();
const calls = mockWarn.mock.calls;
mockWarn.mockRestore();

let foundWarning = false;
const regExp = /serverMode implementation must pass headers to the callback of onConnection\(f\)/;
calls.every((call) => {
if (regExp.test(call)) {
foundWarning = true;
return false;
}
return true;
});

expect(foundWarning).toBeTruthy();

done();
}, 5000);
});
});
});