Skip to content

Bearer regular expression matching in authenticate handler #105

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
merged 1 commit into from
Feb 1, 2022
Merged

Bearer regular expression matching in authenticate handler #105

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
2 changes: 1 addition & 1 deletion lib/handlers/authenticate-handler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ AuthenticateHandler.prototype.getTokenFromRequest = function(request) {

AuthenticateHandler.prototype.getTokenFromRequestHeader = function(request) {
const token = request.get('Authorization');
const matches = token.match(/Bearer\s(\S+)/);
const matches = token.match(/^Bearer\s(\S+)/);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lets discuss this:

Suggested change
const matches = token.match(/^Bearer\s(\S+)/);
const matches = token.match(/^ *(?:[Bb][Ee][Aa][Rr][Ee][Rr]) +([A-Za-z0-9._~+/-]+=*) *$/);

Inspired by https://github.com/jshttp/basic-auth/blob/e8a29f94dc7c05b5858b08090386338af010ce49/index.js#L35

Copy link
Member

Choose a reason for hiding this comment

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

I agree, what's the improvement of this change @FStefanni

Copy link
Member

Choose a reason for hiding this comment

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

In the spec it is defined as it should start with Bearer followed with a whitespace. That's what the additional ^ in the regex is for. I think we should not support BeArEr @Uzlopak

b64token    = 1*( ALPHA / DIGIT /
                  "-" / "." / "_" / "~" / "+" / "/" ) *"="
credentials = "Bearer" 1*SP b64token

But I do like the check provided for the second part. But that should be a different issue/pull request. Because that check should also be done on the result of generateAccessToken and generateRefreshToken.

Copy link
Member

Choose a reason for hiding this comment

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

I suggest we approve & merge this pull request and create a new issue for this.

Copy link
Member

Choose a reason for hiding this comment

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

@jorenvandeweyer I am no regex expert but do you know if the new introduced regex can be overloaded to create a DoS?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The issue is more, that there can be implementations of clients, where the Bearer is preceded by whitespace. Now it doesnt matter, but with this PR a previous working OAuth2 server could have a breaking change. Atleast I would suggest to add \s* in front of Bearer.

Copy link
Member

Choose a reason for hiding this comment

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

In conflict between standard compliance and backward compatibility I will always vote for standard compliance. If preceeding whitespace is not against the standard and poses no security risks then I think it's okay to go for this kind of backward compatiblity

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jankapunkt
Trick 17b

https://npm.runkit.com/safe-regex

var safeRegex = require("safe-regex")

console.log(safeRegex(/^Bearer\s(\S+)/))

result is true, so the regex is safe.

Copy link
Contributor Author

@FStefanni FStefanni Dec 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Hi,

sorry but in the end I have not understand what is the conclusion...
Do we want to support the initial white spaces? (sorry but I do not know exactly if it is standard or not to allow such white spaces)

About the safe-regex, I do not think we need it here: we are just going to match some spaces.

Regards

Copy link
Collaborator

Choose a reason for hiding this comment

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

The regex thing: you should always check if a regex is safe from catastrophic backtracking. So jankapunkt was asking if the regex is safe. Complete normal phenomenon.


if (!matches) {
throw new InvalidRequestError('Invalid request: malformed authorization header');
Expand Down
28 changes: 28 additions & 0 deletions test/unit/handlers/authenticate-handler_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/

const AuthenticateHandler = require('../../../lib/handlers/authenticate-handler');
const InvalidRequestError = require('../../../lib/errors/invalid-request-error');
const Request = require('../../../lib/request');
const sinon = require('sinon');
const should = require('chai').should();
Expand All @@ -16,6 +17,33 @@ const ServerError = require('../../../lib/errors/server-error');

describe('AuthenticateHandler', function() {
describe('getTokenFromRequest()', function() {
describe('with bearer token in the request authorization header', function() {
it('should throw an error if the token is malformed', () => {
const handler = new AuthenticateHandler({
model: { getAccessToken() {} },
});
const request = new Request({
body: {},
headers: {
Authorization: 'foo Bearer bar',
},
method: 'ANY',
query: {},
});

try {
handler.getTokenFromRequestHeader(request);

should.fail('should.fail', '');
} catch (e) {
e.should.be.an.instanceOf(InvalidRequestError);
e.message.should.equal(
'Invalid request: malformed authorization header',
);
}
});
});

describe('with bearer token in the request authorization header', function() {
it('should call `getTokenFromRequestHeader()`', function() {
const handler = new AuthenticateHandler({ model: { getAccessToken: function() {} } });
Expand Down