Skip to content

Commit

Permalink
fix(server): stricter headers security check (#2092)
Browse files Browse the repository at this point in the history
* fix(server): stricter headers security check

* fix(server): changed comments explaining how server impl works
  • Loading branch information
knagaitsev authored and hiroppy committed Jul 3, 2019
1 parent 56274e4 commit 078ddca
Show file tree
Hide file tree
Showing 5 changed files with 109 additions and 3 deletions.
9 changes: 8 additions & 1 deletion lib/Server.js
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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);
});
});
});

0 comments on commit 078ddca

Please sign in to comment.