Bearer regular expression matching in authenticate handler

[FStefanni]

Summary

This pr fixes the regex for checking the bearer token.

Linked issue(s)

Issue #89, point 15, original pr oauthjs/node-oauth2-server#614

Added tests?

Yes

OAuth2 standard

RFC 6750 | OAuth 2.0 Bearer Token Usage | Section 2.1
https://tools.ietf.org/html/rfc6750#section-2.1

[Uzlopak]

seems about right but we should check RFC for BasicAuth if Bearer can have preceeding whitespace or something like that.

[Uzlopak]

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

[jankapunkt]

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

[jorenvandeweyer]

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.

[jorenvandeweyer]

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

[jankapunkt]

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

[Uzlopak]

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.

[jankapunkt]

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

[Uzlopak]

@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.

[FStefanni]

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

[Uzlopak]

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.

[Uzlopak]

I thought about it again. Making it strikter is more important than backwards compatibility.