-
-
Notifications
You must be signed in to change notification settings - Fork 39
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.
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
Bearer regular expression matching in authenticate handler #105
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems about right but we should check RFC for BasicAuth if Bearer can have preceeding whitespace or something like that.
@@ -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+)/); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets discuss this:
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about it again. Making it strikter is more important than backwards compatibility.
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