Skip to content

Commit

Permalink
Throw if request is accepted or rejected more than once. Closes #149
Browse files Browse the repository at this point in the history
  • Loading branch information
theturtle32 committed Nov 25, 2014
1 parent 2a5076b commit a9e7866
Show file tree
Hide file tree
Showing 5 changed files with 114 additions and 31 deletions.
2 changes: 1 addition & 1 deletion gulpfile.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ var gulp = require('gulp');
var jshint = require('gulp-jshint');

gulp.task('lint', function() {
return gulp.src(['gulpfile.js', 'lib/**/*.js'])
return gulp.src(['gulpfile.js', 'lib/**/*.js', 'test/unit/*.js'])
.pipe(jshint('.jshintrc'))
.pipe(jshint.reporter('jshint-stylish', {verbose: true}))
.pipe(jshint.reporter('fail'));
Expand Down
13 changes: 13 additions & 0 deletions lib/WebSocketRequest.js
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,8 @@ function WebSocketRequest(socket, httpRequest, serverConfig) {
this._socketCloseHandler = this._handleSocketCloseBeforeAccept.bind(this);
this.socket.on('end', this._socketCloseHandler);
this.socket.on('close', this._socketCloseHandler);

this._resolved = false;
}

util.inherits(WebSocketRequest, EventEmitter);
Expand Down Expand Up @@ -245,6 +247,8 @@ WebSocketRequest.prototype.parseCookies = function(str) {
};

WebSocketRequest.prototype.accept = function(acceptedProtocol, allowedOrigin, cookies) {
this._verifyResolution();

// TODO: Handle extensions

var protocolFullCase;
Expand Down Expand Up @@ -454,6 +458,8 @@ WebSocketRequest.prototype.accept = function(acceptedProtocol, allowedOrigin, co
};

WebSocketRequest.prototype.reject = function(status, reason, extraHeaders) {
this._verifyResolution();

if (typeof(status) !== 'number') {
status = 403;
}
Expand Down Expand Up @@ -488,6 +494,13 @@ WebSocketRequest.prototype._removeSocketCloseListeners = function() {
this.socket.removeListener('close', this._socketCloseHandler);
};

WebSocketRequest.prototype._verifyResolution = function() {
if (this._resolved) {
throw new Error('WebSocketRequest may only be accepted or rejected one time.');
}
this._resolved = true;

This comment has been minimized.

Copy link
@hesalx

hesalx Nov 29, 2014

Omg, WebSocketRequest#accept() will throw error in at least 14 places after this

This comment has been minimized.

Copy link
@ibc

ibc Nov 29, 2014

Collaborator

What do you mean?

This comment has been minimized.

Copy link
@hesalx

hesalx Nov 29, 2014

A request is marked as resolved at the top of the accept() and reject(). But there are 14 reject() calls inside the accept() after the request is resolved. Thus there are 14 conditions when request fails without any response to client ever. Because an error is thrown before actual accept()/reject() is done.
Actually, as for me, it causes the error at protocols mismatch. I get "WebSocketRequest may only be accepted or rejected one time." instead of an actual error.

This comment has been minimized.

Copy link
@theturtle32

theturtle32 Nov 29, 2014

Author Owner

And that's why I'm starting to move toward having more automated tests. :)

};

function cleanupFailedConnection(connection) {
// Since we have to return a connection object even if the socket is
// already dead in order not to break the API, we schedule a 'close'
Expand Down
43 changes: 43 additions & 0 deletions test/shared/test-server.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
var http = require('http');
var WebSocketServer = require('../../lib/WebSocketServer');

var server;
var wsServer;

function prepare(callback) {
if (typeof(callback) !== 'function') { callback = function(){}; }
server = http.createServer(function(request, response) {
response.writeHead(404);
response.end();
});

wsServer = new WebSocketServer({
httpServer: server,
autoAcceptConnections: false,
maxReceivedFrameSize: 64*1024*1024, // 64MiB
maxReceivedMessageSize: 64*1024*1024, // 64MiB
fragmentOutgoingMessages: false,
keepalive: false,
disableNagleAlgorithm: false
});

server.listen(64321, function(err) {
if (err) {
return callback(err);
}
callback(null, wsServer);
});
}

function stopServer() {
try {
wsServer.shutDown();
server.close();
}
catch(e) { /* do nothing */ }
}

module.exports = {
prepare: prepare,
stopServer: stopServer
};
33 changes: 3 additions & 30 deletions test/unit/dropBeforeAccept.js
Original file line number Diff line number Diff line change
@@ -1,42 +1,15 @@
#!/usr/bin/env node

var test = require('tape');
var http = require('http');

var WebSocketServer = require('../../lib/WebSocketServer');
var WebSocketClient = require('../../lib/WebSocketClient');

var server;
var wsServer;

function prepare(callback) {
server = http.createServer(function(request, response) {
response.writeHead(404);
response.end();
});

wsServer = new WebSocketServer({
httpServer: server,
autoAcceptConnections: false,
maxReceivedFrameSize: 64*1024*1024, // 64MiB
maxReceivedMessageSize: 64*1024*1024, // 64MiB
fragmentOutgoingMessages: false,
keepalive: false,
disableNagleAlgorithm: false
});

server.listen(64321, callback);
}

function stopServer() {
wsServer.shutDown();
server.close();
}
var server = require('../shared/test-server');
var stopServer = server.stopServer;

test('Drop TCP Connection Before server accepts the request', function(t) {
t.plan(5);

prepare(function(err) {
server.prepare(function(err, wsServer) {
if (err) {
t.fail('Unable to start test server');
return t.end();
Expand Down
54 changes: 54 additions & 0 deletions test/unit/request.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
var test = require('tape');

var WebSocketClient = require('../../lib/WebSocketClient');
var server = require('../shared/test-server');
var stopServer = server.stopServer;

var testCase = test('Request can only be rejected or accepted once.', function(t) {
t.plan(6);

server.prepare(function(err, wsServer) {
if (err) {
t.fail('Unable to start test server');
return t.end();
}

wsServer.once('request', firstReq);
connect(2);

function firstReq(request) {
var accept = request.accept.bind(request, request.requestedProtocols[0], request.origin);
var reject = request.reject.bind(request);

t.doesNotThrow(accept, 'First call to accept() should succeed.');
t.throws(accept, 'Second call to accept() should throw.');
t.throws(reject, 'Call to reject() after accept() should throw.');

wsServer.once('request', secondReq);
}

function secondReq(request) {
var accept = request.accept.bind(request, request.requestedProtocols[0], request.origin);
var reject = request.reject.bind(request);

t.doesNotThrow(reject, 'First call to reject() should succeed.');
t.throws(reject, 'Second call to reject() should throw.');
t.throws(accept, 'Call to accept() after reject() should throw.');

t.end();
}

function connect(numTimes) {
var client;
for (var i=0; i < numTimes; i++) {
client = new WebSocketClient();
client.connect('ws://localhost:64321/', 'foo');
client.on('connect', function(connection) { connection.close(); });
}
}
});
});

testCase.on('end', function() {
stopServer();
});

0 comments on commit a9e7866

Please sign in to comment.