Skip to content

Fix basic auth options type issue #584

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

Merged
thornjad merged 1 commit into from
Mar 24, 2020
Merged

Fix basic auth options type issue #584

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 5 additions & 2 deletions lib/http-server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,11 @@ function HttpServer(options) {
// an attacker knowledge of whether the username is correct via a timing
// attack.
if (credentials) {
var usernameEqual = secureCompare(options.username, credentials.name);
var passwordEqual = secureCompare(options.password, credentials.pass);
// since the `name` and `pass` attributes of `credentials` are always string type
// https://github.com/DefinitelyTyped/DefinitelyTyped/blob/HEAD/types/basic-auth/index.d.ts#L15-L16
// so we use `.toString()` to fix https://github.com/http-party/http-server/issues/583
var usernameEqual = secureCompare(options.username.toString(), credentials.name);
var passwordEqual = secureCompare(options.password.toString(), credentials.pass);
Comment on lines +114 to +115
Copy link
Member

@thornjad thornjad Dec 13, 2019

Choose a reason for hiding this comment

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

Should we also convert the credentials to strings? If the server were given a numeric password, as in the tests you added, the user will likely expect numeric to work. Or are we guaranteed to receive a string as credentials?

Copy link
Contributor Author

@swr1bm86 swr1bm86 Dec 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

from the definition of basic-auth which we use here to parse credentials, I think, yes, the credentials are guaranteed to be string :)

Copy link
Member

@thornjad thornjad Dec 16, 2019

Choose a reason for hiding this comment

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

Okay, perfect, that looks like it will always be a string! Could we just add in a comment here saying the same and/or why we need to use .toString() here? It could be added to the comment above the if, or a new comment within the if block?

Copy link
Contributor Author

@swr1bm86 swr1bm86 Dec 25, 2019

Choose a reason for hiding this comment

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

ping @thornjad , sorry for the late replay, the comment is added, pls review again :)

Copy link
Member

@thornjad thornjad Dec 26, 2019

Choose a reason for hiding this comment

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

Coule we also mention in the comment that credentials.name and credentials.pass will be strings, so we don't need to convert them to strings?

if (usernameEqual && passwordEqual) {
return res.emit('next');
}
Expand Down
135 changes: 135 additions & 0 deletions test/http-server-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,5 +379,140 @@ vows.describe('http-server').addBatch({
teardown: function (server) {
server.close();
}
},
'When http-server is listening on 8086 with username "good_username" and Number type password 123456': {
topic: function () {
var server = httpServer.createServer({
root: root,
robots: true,
headers: {
'Access-Control-Allow-Origin': '*',
'Access-Control-Allow-Credentials': 'true'
},
username: 'good_username',
password: 123456
});

server.listen(8086);
this.callback(null, server);
},
'and the user requests an existent file with no auth details': {
topic: function () {
request('http://127.0.0.1:8086/file', this.callback);
},
'status code should be 401': function (res) {
assert.equal(res.statusCode, 401);
},
'and file content': {
topic: function (res, body) {
var self = this;
fs.readFile(path.join(root, 'file'), 'utf8', function (err, data) {
self.callback(err, data, body);
});
},
'should be a forbidden message': function (err, file, body) {
assert.equal(body, 'Access denied');
}
}
},
'and the user requests an existent file with incorrect username': {
topic: function () {
request('http://127.0.0.1:8086/file', {
auth: {
user: 'wrong_username',
pass: '123456'
}
}, this.callback);
},
'status code should be 401': function (res) {
assert.equal(res.statusCode, 401);
},
'and file content': {
topic: function (res, body) {
var self = this;
fs.readFile(path.join(root, 'file'), 'utf8', function (err, data) {
self.callback(err, data, body);
});
},
'should be a forbidden message': function (err, file, body) {
assert.equal(body, 'Access denied');
}
}
},
'and the user requests an existent file with incorrect password': {
topic: function () {
request('http://127.0.0.1:8086/file', {
auth: {
user: 'good_username',
pass: '654321'
}
}, this.callback);
},
'status code should be 401': function (res) {
assert.equal(res.statusCode, 401);
},
'and file content': {
topic: function (res, body) {
var self = this;
fs.readFile(path.join(root, 'file'), 'utf8', function (err, data) {
self.callback(err, data, body);
});
},
'should be a forbidden message': function (err, file, body) {
assert.equal(body, 'Access denied');
}
}
},
'and the user requests a non-existent file with incorrect password': {
topic: function () {
request('http://127.0.0.1:8086/404', {
auth: {
user: 'good_username',
pass: '654321'
}
}, this.callback);
},
'status code should be 401': function (res) {
assert.equal(res.statusCode, 401);
},
'and file content': {
topic: function (res, body) {
var self = this;
fs.readFile(path.join(root, 'file'), 'utf8', function (err, data) {
self.callback(err, data, body);
});
},
'should be a forbidden message': function (err, file, body) {
assert.equal(body, 'Access denied');
}
}
},
'and the user requests an existent file with correct auth details': {
topic: function () {
request('http://127.0.0.1:8086/file', {
auth: {
user: 'good_username',
pass: '123456'
}
}, this.callback);
},
'status code should be 200': function (res) {
assert.equal(res.statusCode, 200);
},
'and file content': {
topic: function (res, body) {
var self = this;
fs.readFile(path.join(root, 'file'), 'utf8', function (err, data) {
self.callback(err, data, body);
});
},
'should match content of served file': function (err, file, body) {
assert.equal(body.trim(), file.trim());
}
}
},
teardown: function (server) {
server.close();
}
}
}).export(module);