Skip to content

Gracefully handle invalid status codes #3143

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

Closed
wants to merge 2 commits into from
+38 −10
Closed

Gracefully handle invalid status codes #3143

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
61d4afe
Gracefully handle invalid status codes
nickclaw Nov 23, 2016
ae38ac6
Ensure that statusCode is a number
nickclaw Nov 27, 2016
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
25 changes: 15 additions & 10 deletions lib/response.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,14 @@ var charsetRegExp = /;\s*charset\s*=/;
* @public
*/

res.status = function status(code) {
this.statusCode = code;
res.status = function status(statusCode) {
// check that status code is valid
statusCode = parseInt(statusCode, 10);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should avoid reassignment of function arguments, please

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I don't think we should parse at all. Throw if it is not a number primitive

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Node/express currently works with any numeric string as a status code (node does statusCode |= 0) under the hood, seems like only accepting numbers could be an annoying change for anyone relying on it. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK that was done so they could add the restriction in a minor release instead of a major. Unless you want to make sure that this.status('500 cats') does not silently work, then I think it should just require a number. In fact, it should require that number to be an integer, so this.status(500.1) should throw as well.

The purpose is to make it obvious when there is a programming error, not to silently correct mistakes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically, if the purpose of this isn't to catch obviously invalid input up front, then there doesn't seem a point to the PR to me, as we can just let Node.js catch it as it does today.

if (Number.isNaN(statusCode) || statusCode < 100 || statusCode > 999) {
throw new TypeError('Invalid status code.');
}

this.statusCode = statusCode;
return this;
};

Expand Down Expand Up @@ -110,7 +116,7 @@ res.send = function send(body) {
// support res.send(status, body)
if (arguments.length === 2) {
deprecate('res.send(status, body): Use res.status(status).send(body) instead');
this.statusCode = arguments[0];
this.status(arguments[0]);
chunk = arguments[1];
}

Expand Down Expand Up @@ -169,7 +175,7 @@ res.send = function send(body) {
}

// freshness
if (req.fresh) this.statusCode = 304;
if (req.fresh) this.status(304);

// strip irrelevant headers
if (204 === this.statusCode || 304 === this.statusCode) {
Expand Down Expand Up @@ -208,7 +214,7 @@ res.json = function json(obj) {
// support res.json(status, obj)
if (arguments.length === 2) {
deprecate('res.json(status, obj): Use res.status(status).json(obj) instead');
this.statusCode = arguments[0];
this.status(arguments[0]);
val = arguments[1];
}

Expand Down Expand Up @@ -244,7 +250,7 @@ res.jsonp = function jsonp(obj) {
// support res.jsonp(status, obj)
if (arguments.length === 2) {
deprecate('res.jsonp(status, obj): Use res.status(status).jsonp(obj) instead');
this.statusCode = arguments[0];
this.status(arguments[0]);
val = arguments[1];
}

Expand Down Expand Up @@ -304,11 +310,10 @@ res.jsonp = function jsonp(obj) {
*/

res.sendStatus = function sendStatus(statusCode) {
var body = statusCodes[statusCode] || String(statusCode);

this.statusCode = statusCode;
this.status(statusCode);
this.type('txt');

var body = statusCodes[statusCode] || String(statusCode);
return this.send(body);
};

Expand Down Expand Up @@ -788,7 +793,7 @@ res.redirect = function redirect(url) {
});

// Respond
this.statusCode = status;
this.status(status);
this.set('Content-Length', Buffer.byteLength(body));

if (this.req.method === 'HEAD') {
Expand Down
36 changes: 36 additions & 0 deletions test/res.status.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,5 +16,41 @@ describe('res', function(){
.expect('Created')
.expect(201, done);
})

it('should throw a TypeError if invalid', function(done){
var app = express();

app.use(function(req, res){
res.status(10000).end();
});

request(app)
.get('/')
.expect(500, /Invalid status code/, done);
})

it('should handle numeric strings', function(done) {
var app = express();

app.use(function(req, res){
res.status('400').end();
});

request(app)
.get('/')
.expect(400, done);
})

it('should handle floats as integers', function(done) {
var app = express();

app.use(function(req, res){
res.status(404.04).end();
});

request(app)
.get('/')
.expect(404, done);
})
})
})