node-oauth/node-oauth2-server

🚀 The successor to oauthjs/oauth2-server. 🔒 Complete, compliant, maintained and well tested OAuth2 Server for node.js. Includes native async await and PKCE.

node-oauth2-server's Issues

Update crypto hash to use sha256

This line in token-utils.

Uses sha1 for the hash, we should update this to be sha256 and update our unit tests too.

getters of Model, expected return value?

Well. When I read #6 i realized:

The Model Implementations for the getters (e.g. getAccessToken) return strictly IToken.

So do we expect that model functions throw errors when the requested data is not existing?

The original documentation does not say anything regarding that
https://oauth2-server.readthedocs.io/en/latest/model/spec.html#model-getauthorizationcode

And what about the code examples in the documentation do not help at all as they are broken, as they never have returning any value (not tested, but by only reading the code). LOL

https://oauth2-server.readthedocs.io/en/latest/model/spec.html#model-getauthorizationcode

function getRefreshToken(refreshToken) {
  // imaginary DB queries
  db.queryRefreshToken({refresh_token: refreshToken})
    .then(function(token) {
      return Promise.all([
        token,
        db.queryClient({id: token.client_id}),
        db.queryUser({id: token.user_id})
      ]);
    })
    .spread(function(token, client, user) {
      return {
        refreshToken: token.refresh_token,
        refreshTokenExpiresAt: token.expires_at,
        scope: token.scope,
        client: client, // with 'id' property
        user: user
      };
    });
}

Security status and option to disable service

In order to provide additional security enhancements for versions with unsafe status need to notice update process particularities:

• users cannot continuously monitor the security status of all packages and this package,
• automatic update question is also not easy and may meet some delays
• in some cases, users, even having information about the vulnerability, may postpone the update for some reason and/or for some time (say, before the start of the working day)

In cases where the absence of a vulnerability is essential, it may be justified to block the operation of the server or part of the functionality until the updates.

I suggest considering:

• have a system for monitoring the presence of vulnerabilities in the server (and a web page of the version status)
• have the option to block the system if there is a negative status for this version
• simplify as much as possible and take steps to make updates easier

Remove promisify-any dependency

Just use util.promisify

Get rid of dependencies?

I just checked a bit the current dependencies we have.

Regarding bluebird and promise I think we are good to remove them and they are covered by these issues already: #19 #20 #48

The other dependencies are the following:

type-is

npm: https://www.npmjs.com/package/type-is
github: https://github.com/jshttp/type-is

dependencies:

• media-typer: https://www.npmjs.com/package/media-typer
• mime-types: https://www.npmjs.com/package/mime-types

usage:

This project uses type-is to parse and classify request types (like text/html; charset=utf-8 or application/json).

See in code: https://github.com/node-oauth/node-oauth2-server/blob/development/lib/request.js#L8

analysis:

While mime-types is updated regularly, the type-is and media-typer have not been released in a few years but are at the same time downloaded in the ten-millions. This can mean they are quite mature but it could also mean that the owners may have not much time anymore to maintain.

The may also have no 2FA setup yet (because a few years ago nobody cared so much but a few people who were security-aware).
We should ask the owners (directly via Email) if they have setup 2FA in order to prevent supply chain attacks on their repos, since their combination (yeaqrs ago released + 20 million downloads) would be targets for SCA in my view.

remove or not

I am not sure if removing makes things easier. Parsing is often a complicated thing and request types are variying a lot. Would need more analysis, if we need only a few types exclusively.

basic-auth

npm: https://www.npmjs.com/package/basic-auth
github: https://github.com/jshttp/basic-auth

dependencies:

• safe-buffer: https://www.npmjs.com/package/safe-buffer

usage:

This project uses basic-auth in order to retrieve and parse the auth header of a request.

See in code:

• https://github.com/node-oauth/node-oauth2-server/blob/development/lib/handlers/token-handler.js#L20
• https://github.com/node-oauth/node-oauth2-server/blob/development/lib/handlers/token-handler.js#L175

analysis

Both packages are not published in 2 years. Their usage is also in the millions. While I can't say about the activity for the owner of basic-auth I know at least for the author of safe-buffer that he is actively maintaing his repos. The same thing applies regarding 2FA, we should ask the owners (directly via Email).

Removing basic-auth would be a major version change since it throws errors. These errors may be being caught in peoples apis.

remove or not

This is a more interesting issue, since parsing the auth header is way simpler than parsing thousands of combinations of requests. We also could get rid of both deps, since the node-native buffer should be sufficient.

Security process

As already mentioned in the old discussion this project is highly security relevant:

What you gonna do? Issue is also, that this is a security relevant product. If you have a not trustworthy contributor/maintainer which puts malicious code into the product, then alot of companies will be hackable. But on the other hand, we can have a critical community and make it necessary to have x approvals before the maintainers can actually merge into master. I would also agree that new maintainers need to disclose their identities and who their employers are, so that making them to be maintainers does not mean to make a malicious anonymous able to taint the code.

I agree a lot with this. How do we want to ensure a high security in this project at all stages?

Also: assuming there is funding, do we want to get an independent security audit for certain releases?

Replace UnauthorizedRequestError with InvalidRequestError

The UnauthorizedRequestError is not a standard error code. According to the reference in the comment https://datatracker.ietf.org/doc/html/rfc6750#section-3.1 there is no unauthorized_request error.

UnauthorizedRequestError is used in the AuthenticateHandler for indicating that there was no token in body and header.
According to the Spec it should be an InvalidRequestError, as the token is clearly a missing parameter.

The request is missing a required parameter, includes an
unsupported parameter or parameter value, repeats the same
parameter, uses more than one method for including an access
token, or is otherwise malformed. The resource server SHOULD
respond with the HTTP 400 (Bad Request) status code.

Replace should with chai

Is discussed is issue #5 should.js has been archived. Replacing should with chai will make the project more future-proof.

Discord? Slack? A place to chat?

Can we have a place to chat?
Slack or Discord would be cool. Then I could annoy you constantly with OAuth2-related stuff.

Examples

The barrier to entry on this package isn't insurmountable but it's not easy either. I'd love to see some examples using the following databases:

• SQL
• MongoDB
• Redis

Add security consideration section to integration tests

We should add tests to fully cover section 10 - security considerations and cover as much as possible that can be covered without going into implementation details.

Add sha512sum to package.json

To improve trust I thought we may add the sha512sum (the one which is added during npm pubklish to the package.json) to every commit or at least every commit that is used when publishing a new version.

Alternatively, is there a better or more "best practice" way to give people the chance to easily (accessible) verify the packge's code integrity with the code on GitHub?

SonarQube: Weak Cryptography in generateRandomToken

I am currently running node-oauth2-server through sonarqube.

I know this part of the code was touched in #38.

Sonarqube shows for generateRandomToken following Security Hotspot
Cryptographic hash algorithms such as MD2, MD4, MD5, MD6, HAVAL-128, HMAC-MD5, DSA (which uses SHA-1), RIPEMD, RIPEMD-128, RIPEMD-160, HMACRIPEMD160 and SHA-1 are no longer considered secure, because it is possible to have collisions (little computational effort is enough to find two or more different inputs that produce the same hash).

We actually just want to create a random token. So this finding is not a security issue anyway.

But when I read the code it makes for me no sense, why the output of randomBytes, which outputs a cryptographical secure random value should be hashed again with sha256. We could replace this by simply doing

  generateRandomToken: function() {
    return Promise.resolve(crypto.randomBytes(32).toString('hex'));
  }

and all tests would pass. SonarQube would also not remark this as potential security issue.

Conformance tests

Maybe there is something useful here https://www.certification.openid.net/

Seems actually be OIDC specific. But maybe there is something we could use.

Remove lodash dependency

Just did a quick search for "_." and realized lodash is a dependency almost exclusively because of assign. We could easily remove lodash (and all the security concerns with it) by using the Spread operator or Object.assign.

_.includes and _.has are also used a couple of times and could easily be replaced with Array.includes and hasOwnProperty.

Scope in refresh_token grant type is ignored.

While checking the compliance of the refresh_token grant. I discovered that the optional scope parameter in the body is ignored.

node-oauth2-server/lib/grant-types/refresh-token-grant-type.js

Lines 64 to 73 in 1b91ddc

	return Promise.bind(this)
	.then(function() {
	return this.getRefreshToken(request, client);
	})
	.tap(function(token) {
	return this.revokeToken(token);
	})
	.then(function(token) {
	return this.saveToken(token.user, client, token.scope);
	});

https://datatracker.ietf.org/doc/html/rfc6749#section-6

Remove statuses dependency

Use http.STATUS_CODES instead.

Don't repeat tests when they fail.

Let's change this to 0... why repeat failed tests?

node-oauth2-server/.mocharc.yml

Line 3 in 485147f

retries: 1

Solving appropriate the Client Side RefreshToken Race Issue

I wrote a very nice text and now I am just sad because my Chrome freeze and every well formulated word is gone.

So again...but shorter

In native applications we dont have a Race Condition when refreshing a Access Token. But in a Browser-App we have the issue, that we will use multiple Tabs. So if an Access Token expires or is going to expire, all the Tabs start to Refresh the Token. Even if we use one central datastore (cookiestore, localStorage). I once solved this by allowing the creation of multiple access token by one refresh token and disabling Refresh Token Rotation.

But this is actually an Anti-Pattern.

So I thought how we can implement it in a save and stable way:

I think the desired behavior is that in a specific and very very short timeframe the RefreshToken can be used multiple times but MUST return always the same AccessToken. So I propose to add an option next to alwaysIssueNewRefreshToken named refreshTokenExpirationDelay (name pending).

It works like this:

1. Clients in Browser realize that their Access Token is going to expire and each one of the start the refreshToken Grant
2. Authorization Server does the usual validation stuff
3. New Access Token and new Refresh Token (alwaysIssueNewRefreshToken is set) are generated
4. old RefreshToken gets the value or a reference of the new Access Token, the value or the reference of the new Refresh Token and the expiration of the old RefreshToken is set to now + refreshTokenExpirationDelay (now + 5000 => 5 seconds in the future), but with the condition that it will only set those fields if the new AccessToken value or reference was not set before.
5. Check if our DB Operation in step 4 changed an entry or not
   5.1 We successfully changed old Refresh Token => we won the race.
   5.2 We did not change anything. Load the new Access Token and the new RefreshToken from the Database and delete the unnecessary generated Access and Refresh Token in Step 3
6. Return new AccessToken and RefreshToken

Any Optimizations you would suggest?

This would be possible breaking the saveToken-method in the Model.

Documentation

We need to run the documentation. We can use the free js.org domain on this stage, for example, obviously:

node-oauth.js.org

I have experience to set it up it for my open source projects, along with the GitBook documentation engine. Both also used by the well-known Redux (or at least Redux used GitBook before did not check for now). Thereat it was easy, but now I would prefer to use something more interesting. Most likely including but not limited to JSDoc which is mentioned in the discussion #5

Implement PKCE

See https://snyk.io/vuln/npm:oauth2-server
oauthjs/node-oauth2-server#452

Remove dependencies

In the old thread discussion there was a mention on some deps that could be removed or replaced:

• bluebird -> Native Node Promise
• promisify-any -> utilis.promisify
• statuses -> http.STATUS_CODES

Move validateScope outside promise.all

When there is an invalid scope passed, there is still an accessToken and refreshToken generated.

Code

node-oauth2-server/lib/grant-types/password-grant-type.js

Lines 105 to 127 in 0154165

	PasswordGrantType.prototype.saveToken = function(user, client, scope) {
	var fns = [
	this.validateScope(user, client, scope),
	this.generateAccessToken(client, user, scope),
	this.generateRefreshToken(client, user, scope),
	this.getAccessTokenExpiresAt(),
	this.getRefreshTokenExpiresAt()
	];
	return Promise.all(fns)
	.bind(this)
	.spread(function(scope, accessToken, refreshToken, accessTokenExpiresAt, refreshTokenExpiresAt) {
	var token = {
	accessToken: accessToken,
	accessTokenExpiresAt: accessTokenExpiresAt,
	refreshToken: refreshToken,
	refreshTokenExpiresAt: refreshTokenExpiresAt,
	scope: scope
	};
	return promisify(this.model.saveToken, 3).call(this.model, token, client, user);
	});
	};

Suggestion

Move this.validateScope(user, client, scope) out of the array and check this before generating the tokens.

Use case

We use JWT's and only an internal token id is saved instead of the full JWT string. This means generating the token automatically means saving the token. So we are not actually using the saveToken function.

Remove util.inherits

Use ES6 Class syntax with extends instead of utils.inherits

node-oauth2-server/lib/grant-types/authorization-code-grant-type.js

Line 47 in 485147f

util.inherits(AuthorizationCodeGrantType, AbstractGrantType);

node-oauth2-server/lib/grant-types/client-credentials-grant-type.js

Line 40 in 485147f

util.inherits(ClientCredentialsGrantType, AbstractGrantType);

node-oauth2-server/lib/grant-types/password-grant-type.js

Line 42 in 485147f

util.inherits(PasswordGrantType, AbstractGrantType);

node-oauth2-server/lib/grant-types/refresh-token-grant-type.js

Line 47 in 485147f

util.inherits(RefreshTokenGrantType, AbstractGrantType);

This also occurs in all the errors at lib/errors

Dependabot vulnerabilities -- what we are doing

Dependabot is giving us a few PRs to update dependencies, thankfully most of these are dev dependencies, but given how two of them are major versions -- we gotta be a little careful here because tests may break.

What I am going to do is create a PR that not only updates the packages, but also removes the statuses module and attempt to replace it with http-status-codes .

auth0 mfa-otp

Did somebody ever implemented a mfa-otp grant flow like auth0 did?

Missing tests

When running the code coverage there are still a few lines uncovered:

-------------------------------------|---------|----------|---------|---------|-------------------
File                                 | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
-------------------------------------|---------|----------|---------|---------|-------------------
All files                            |   98.62 |    94.48 |     100 |   98.62 |                   
 lib                                 |     100 |    88.88 |     100 |     100 |                   
  request.js                         |     100 |       90 |     100 |     100 | 15,36             
  response.js                        |     100 |       90 |     100 |     100 | 16                
  server.js                          |     100 |    83.33 |     100 |     100 | 18                
 lib/errors                          |   99.09 |    91.66 |     100 |   99.09 |                   
  access-denied-error.js             |     100 |      100 |     100 |     100 |                   
  insufficient-scope-error.js        |     100 |      100 |     100 |     100 |                   
  invalid-argument-error.js          |     100 |      100 |     100 |     100 |                   
  invalid-client-error.js            |     100 |      100 |     100 |     100 |                   
  invalid-grant-error.js             |     100 |      100 |     100 |     100 |                   
  invalid-request-error.js           |     100 |      100 |     100 |     100 |                   
  invalid-scope-error.js             |     100 |      100 |     100 |     100 |                   
  invalid-token-error.js             |     100 |      100 |     100 |     100 |                   
  oauth-error.js                     |      95 |    91.66 |     100 |      95 | 18                
  server-error.js                    |     100 |      100 |     100 |     100 |                   
  unauthorized-client-error.js       |     100 |      100 |     100 |     100 |                   
  unauthorized-request-error.js      |     100 |      100 |     100 |     100 |                   
  unsupported-grant-type-error.js    |     100 |      100 |     100 |     100 |                   
  unsupported-response-type-error.js |     100 |      100 |     100 |     100 |                   
 lib/grant-types                     |   99.61 |    97.14 |     100 |   99.61 |                   
  abstract-grant-type.js             |   97.56 |       85 |     100 |   97.56 | 101               
  authorization-code-grant-type.js   |     100 |    97.72 |     100 |     100 | 147               
  client-credentials-grant-type.js   |     100 |      100 |     100 |     100 |                   
  password-grant-type.js             |     100 |      100 |     100 |     100 |                   
  refresh-token-grant-type.js        |     100 |      100 |     100 |     100 |                   
 lib/handlers                        |    98.6 |    96.38 |     100 |    98.6 |                   
  authenticate-handler.js            |   98.97 |    98.38 |     100 |   98.97 | 63                
  authorize-handler.js               |   98.59 |    96.55 |     100 |   98.59 | 43,215            
  token-handler.js                   |   98.31 |    94.44 |     100 |   98.31 | 120,124           
 lib/models                          |   82.14 |    73.07 |     100 |   82.14 |                   
  token-model.js                     |   82.14 |    73.07 |     100 |   82.14 | 19,23,27,31,35    
 lib/response-types                  |     100 |      100 |     100 |     100 |                   
  code-response-type.js              |     100 |      100 |     100 |     100 |                   
 lib/token-types                     |     100 |    91.66 |     100 |     100 |                   
  bearer-token-type.js               |     100 |    91.66 |     100 |     100 | 51                
 lib/utils                           |     100 |      100 |     100 |     100 |                   
  token-util.js                      |     100 |      100 |     100 |     100 |                   
 lib/validator                       |     100 |      100 |     100 |     100 |                   
  is.js                              |     100 |      100 |     100 |     100 |                   
-------------------------------------|---------|----------|---------|---------|-------------------

I checked the files and all of them were missing checks for thrown Errors/Exceptions. Since throwing specific errors in specific situations is a crucial part of the standard we should at least check, whether these errors are correct / compliant or not.

Code Quality

Some things I'd like to discuss and also work on is code quality and documentation:

• Tests and coverage (mocha, chai, sinon, istanbul)
• Linter
• API Docs
• Wiki, if desired
• README and badges
• GitHub Actions for CI and CD
• static analysis like semmle / lgtm for non-dependency vulnerabilities: https://github.com/node-oauth/node-oauth2-server/security/code-scanning

There are a few things to decide, like

• what linter do we want to use (I suggest something with zero config like prettier or standard)
• what doc generation (JSDoc) we want to use (I suggest also zero config like JSDoc)
• should master / main be push-protected?
• what should be required for CI to pass a basic pull request?
  • linter?
  • unit tests?
  • minimal required test coverage?
  • dependency audit via npm audit?
  • security analyses?

What else?

Remove Bluebird dependency

We should be able to remove Bluebird as a dependency in favor of standard ES6 Promises.

InvalidClientError should return 401

https://datatracker.ietf.org/doc/html/rfc6749#section-5.2

 Client authentication failed (e.g., unknown client, no
               client authentication included, or unsupported
               authentication method).  The authorization server MAY
               return an HTTP 401 (Unauthorized) status code to indicate
               which HTTP authentication schemes are supported.  If the
               client attempted to authenticate via the "Authorization"
               request header field, the authorization server MUST
               respond with an HTTP 401 (Unauthorized) status code and
               include the "WWW-Authenticate" response header field
               matching the authentication scheme used by the client.

Missing distinction between public and confidential clients

Currently there is not distinction between confidential and public clients as it is needed by RFC6749

We should add an attribute "type" for the Client-Object.
In the Authorization Grant Flow Access Token is used, we need not only the client_id in the payload but also the client credentials in the authoriation header if it is a confidential client. see RFC 6749 4.1.3

Currently it is not really based on the Client but on the grant_type and the requireClientAuthentication option. As it lacks distinction theoretically a public client has to send client_secret.

[meta] list of original project pr

Hi,

this is a list of original project pr still open, to be analyzed and possibly integrated in this code base.

• 1 2.x support: oauth.token promise, returns token object instead of sending response - 424
  • This is referring to a very old version, no more relevant
• 2 PKCE - 452
  • Addressed in issue #76, so we do not need to track it here
• 3 Don't fail on vschar check on state if it is empty and allowEmptyState=1 - 473
  • see #93
• 4 Implement validateRedirectUri model function - 482
  • This is a feature-adding pr. Since it seems fine, I just added it in pr #97. The code is slightly changed w.r.t. original pr (just a little cleaner imho).
• 5 Update extension-grants.rst with example - 530
  • Fixed by pr #92
• 6 Allow "allowed" parameter to be fetched from body - 532
  • see #94
• 7 Pass scope to model on refresh_token request - 540
  • The original pr is fine w.r.t. the RFC6749 Section 6. I believe the missing part is to ensure the passed scope does not contains non-validated scopes, and this missing check could lead to potential security risks. So I suggest to implement a check to ensure the passed request scope object is a "subset" of validated scopes (or, as alternative, a method which removes invalid scopes).
• 8 Update README.md - 556
  • Fixed by pr #91
• 9 rokenToken is not required if alwaysIssueNewRefreshToken = false - 558
  • The pr is right, w.r.t. the rest of the code base. So it is is just a little improvement, to make the model.revokeToken() method implementation optional. So the pr could be accepted, even if it is not strictly useful (it is possible to implement the method with an empty body, or maybe with a default implementation inside a "model" base class). Therefore I am not proposing a pr for this.
• 10 Add return statements for more clarity - 561
  • Pr proposed: #98. This is a simple "doc" improvement. The original pr states that is for clarity, but it is not: the doc examples are actually bugged, so this pr is required.
• 11 RFC 6749#4.1.2.1 - fix order of error handling - 565
  • Pr proposed: #112. The original pr fixes the order of some checks, according with the standard, which seems fine to me.
• 12 The object returned by the response.getHeaders() method does not prot… - 567
  • This pr proposes a fix already present in the code. So it is useless.
• 13 Update authorize-handler.js - 576
  • This pr is an alternative implementation of pr oauthjs/node-oauth2-server#649. See comments below (point number 20).
• 14 v5: add codecoverage and upgrade packages - 610
  • This targets branch v5-dev (typescript). Probably no more useful.
• 15 fix: Bearer regular expression matching in authenticate handler - 614
  • The original pr is fine. Proposed the pr #105
• 16 Update refresh-token-grant-type.js - 615
  • This pr is wrong. The change introduce a bug.
• 17 Passing validated scope into generateAccessToken. - 620
  • The pr targets branch v5-dev, and claims it is already fixed in other branches. So it is useless.
• 18 Set WWW-Authenticate header for invalid requests - 646
  • The original pr is right, so I just re-proposed it here: #96
• 19 Remove scope validation - 647
  • Proposed pr #103
  • The implementation is slightly different from the original pr. Details given in the pr description
• 20 Fix user denial and missing state parameter on error redirect - 649
  • This is a different implementation of point 13
  • Propose pr #99
• 21 Update refresh-token-grant-type.js - 657
  • The pr does not looks correct. Checking also the v5-dev branch, the client object seems to have a id member, and not a clientId member. Written into the original pr to have feedback.
• 22 Add PKCE support - 658
  • Addressed in issue #76, so we do not need to track it here
• 23 Add PKCE support (for v5-dev Typescript) - 659
  • Addressed in issue #76, so we do not need to track it here
• 24 Fix model spec to use code.authorizationCode instead of code.code - 660
  • The original pr just upgrades the doc, according with the original dev branch. Even if the pr seems not strictly useful right now, it shows that the original dev branch could contain some interesting improvements, worth to merge in this code base. To be further analyzed.
• 25 dependencies updated #662
  • This targets branch v5-dev (typescript). Probably no more useful.
• 26 Bump statuses from 1.5.0 to 2.0.1 - 670
  • No more relevant: package has been removed
• 27 Update token-util.js - 678
  • Already fixed
• 28 Bump mocha from 5.2.0 to 8.4.0 - 689
  • Fixed by the following n.32
• 29 Bump sinon from 7.5.0 to 11.1.1 - 694
  • Fixed by the following n.33
• 30 Bump jshint from 2.13.0 to 2.13.1 - 708
  • No more relevant: jshint has been replaced with eslint
• 31 Only require 'redirect_uri' in token/authorization_code when one was provided in authorize/code - 709
  • The original pr question is correct. But I am wondering whether the proposed code is fine. In fact I am not 100% sure about the getRedirectUri() method implementation. Probably, just removing || client.redirectUris[0] will fix all the issues. But I am not completely sure about this: I do not know the standard and the code so well...
• 32 Bump mocha from 5.2.0 to 9.1.3 - 716
  • Fixed by pr #90
• 33 Bump sinon from 7.5.0 to 12.0.1 - 718
  • Already fixed

The list will be upgraded to track the ongoing process of integrating the pr's.

Regards.

RFC-Conformity depends on Implementation

We should document, that some conformity rules can only be implemented by the express/fastify/koa-etc. layer.

Maybe we should collect the MUST rules for meeting the conformity requirements but are (currently?) out of scope of the oauth2-server.

• the authorization endpoint MUST support the GET method. probably alot of people only implement the post but not the get method.

   The authorization server MUST support the use of the HTTP "GET"
   method [RFC2616] for the authorization endpoint and MAY support the
   use of the "POST" method as well.

https://datatracker.ietf.org/doc/html/rfc6749#section-3.1

• TLS is also necessary, which is not enforced by the oauth2-server

   The authorization server MUST require the use of TLS as described in
   Section 1.6 when sending requests using password authentication.

https://datatracker.ietf.org/doc/html/rfc6749#section-2.3.1

• Brute-Force Protection for the endpoints is also a MUST regarding the RFC.

   Since this client authentication method involves a password, the
   authorization server MUST protect any endpoint utilizing it against
   brute force attacks.

ResourceOwnerPasswordGrant: getUser should use request instead of username, password as parameters

I think, it is kind bad, that we can not access directly the request. It enforces the implementor to only have the fields username and password available.

I remember, that I once had to use more information in the getUser-function and had to implement an ugly hack (concat the fields into the userfield and split them again in the implementation) just be able to process the the data then in the getUser-function. I think it was necessary to configure the generated refreshToken as long lived (4 weeks) or short lived (1 day).

So by using the request as parameter, it is possible to name the fields of the login-form differently and also be able to have access to all fields of the login-Form.

AuthorizationCode not revoked early enough

This is a small security issue if I understood bluebirds tap method correctly.

See also:
oauthjs/node-oauth2-server#637

I think we should revoke the AuthorizationCode much much earlier, by moving the revokeAuthorizationCode call above the redirectUri check.

node-oauth2-server/lib/grant-types/authorization-code-grant-type.js

Line 68 in 485147f

.tap(function(code) {

An attack on the authorizationCode grant is usually based on a malicious redirect_uri. So if the authorization code is valid, we load the token, then tap on validateRedirectUri, and if the redirect_uri is invalid, we error and we are not calling revokeAuthorizationCode. So the authorizationCode is still existing despite it was used already.

Supporting Node12 and above

Apparently, we (master) support Node 4 and above. We should, however, consider following the Node LTS and basically, support Node 12 and above.

The simplest change would be updating the package.json to update the engines to node >= 12.0.

The question is now What and where else would we need to make changes to show we support only Node12 and above? (obviously CI/CD testing, but that convo is for another issue).

Some things that we are merging to development only support Node 12 and above (example, mocha). So would this change be backwards breaking?

A fun little tidbit is that in the previous repository we forked, the travis.yml file actually was failing on Node 4 - Node 10...so maybe we already support Node 12 but just need to update the docs and package.json? https://travis-ci.org/github/oauthjs/node-oauth2-server

Matching @types/node-oauth2-server

In the event that @node-oauth/node-oauth2-server does not match @types/oauth2-server

Remove support for callbacks in models

Callback support adds unneeded complexity. Models should be exclusively promise based version ^4.1.0. #20 (comment)

Replace InvalidArgumentError with ServerError

InvalidArgumentError or invalid_argument is not a oauth2-error. It is used by oauth2-server to indicate errors like that specific internal implementation is wrong. E.g. Request is missing header. Also the HttpStatusCode of 500 clearly indicates that it is a ServerError and not something what the enduser can control.

https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.2.1
https://datatracker.ietf.org/doc/html/rfc6749#section-4.2.2.1

Welcome, let's get started with version 4 and the new organization -- a new starting point

This repo is an attempt to continue on with the work that has been done on oauthjs/node-oauth2-server

The NPM module will be called @node-oauth/oauth2-server.

From oauthjs/node-oauth2-server#621, the project has basically, from what it seems, been abandoned. And we understand that life get's busy and what not, but due to difficulty of getting a new maintainer on board the project, I've decided to just get it going on my own.

Mini Roadmap

Version 4 will be released now and will essentially be what's on oauthjs/node-oauth2-server master branch, a hotfix that contains some module updates to fix vulnerability, and some minor package.json and README.md updates. From there we will take the work done for version 4 and make that version 5 here.

Version 4 will primarily be ONLY for bug fixes and misc. clean ups as we transition over to oauth2-server2.
We will also begin updating things for GitHub actions to support CI/CD and automate security fixes.

No new features (unless seriously necessary and needed ASAP) will be added; all other features will go into version 5

Helping out

Really will need helping sort out version 5 since there appears to be several change. We def should reference the old project and see what was happening in there.

We can also start setting up GitHub actions as well as the overall organization that this project is now under.

Thank you and please leave thoughts or concerns on this issue, let's get started!

If you have any questions relating to anything specific to the project in itself (ex, a bug) come here and start a new issue and/or link a previous PR/issue.

Modernize code to ES6+

I think we have a huge potential to transform as many variables as possible to const and let which allows us to have better scoping (block-scoped variables) and let's us detect unintended mutations early.

We already replaced many lodash functions with native ES6 alternatives so I think it might be good to consequentially do this for the rest of the code, too.

This issue is weakly related to #48

validateScope and veryifyScope are anti-patterns?!

The model says, we should provide the functions validateScope and verifyScope. The implementor has to write a correct function, which filters out invalid or not allowed scopes. This can result in a bad implementation, as the implementor could mess it up.

Despite burdening the implementor with the task to write a correct function, the framework should ask for a getScopesOfUser (name disputable), which returns all scopes of the user and the framework will have (well-tested?!) validateScopes and verifyScopes methods which filters out invalid or not allowed scopes.

Feel free to join our Discord for live chatting

https://discord.gg/4HTUAcTvqV

The Discord is not a replacement for the issue board, please do not use the discord for bug reporting.

Merge strategy `rebase` or `merge`

To minimize and simplify discussion of code quality #5, let us discuss merge strategy here

The result slice of code will be the same. The question is only about git history and how to work with it. This is part of history of this project where merge approach is used:

This requires more attention at navigate through - merge commits has more than one parents.

There are at least two commit in history which has different features and does not include both.

Search commit that has an error become more complicated, it may appear and disappear in linear order (for example image above commit 558 appears at af6 and ff0 but not appears between them at b4d). Fortunately git has a tool to simplify search.

By the way GitHub hides this branches from user, in opposite bitbucket shows them.

If we try to take a look to merge commit it is empty in most cases but may contain conflict resolution or some else (what? the question for discuss also). Empty merge commit is correct due to changes actually present in parent branches but little confusing and involve to navigate to other commits in parent branches.

From the other side branches shows how works moves and they represent itself some sort of formatting or division for fragments the works.

This the part of linear history identical rebase approach (taken also from this project):

However top commits a285 and 88a6 may be was made by merge strategy but actually has one parent commit (but actually created by rebase or just empty generic commits), and they are empty. It is possible to create empty generic single parent commit also. But what is the reason and how that commits actually created it is not clean for now. But the topic is not about that coimmis but about merge strategy.

That is the thought I would like to share in addition to what may be found in web.

Make is.js it's own independent module

is.js really slows down the tests. What do you guys think about moving it to it's own repository and adding it as a dependency?

Maintaining adapter packages?

Do we want to maintain adapters under this org, too? At least for express I can provide support. One argument for it is, that it will build much more trust and we can ensure this will live on. Argument against is that we have more to maintain overall.

SonarQube: Control Character in UNICODECHARNOCRLF in is.js

SonarQube remarks that it is most likely that the Control Character \u0009 is a bug.

0x9 is Tab-Control Character.

is.uchar is used in password-grant-type.js for validating the content of username and password.

The question here is: Is it possible to have Tab as a character of a username and/or password? I say: No

I think this is a valid finding.

I recommend to remove the Tab-Control-Character.

Lets Discuss.

Release strategy

How do we want to release a new version?

• who should release? -> human vs GitHub Actions?
• when to release?
• how to prepare a release? -> one release branch with tags or one branch for each major version? like release-4.x, release-5.x
• do we want additional tests that run in GitHub actions when merging to a release branch? GH Actions is so flexible I could even clone another repo and install the intended release package into it and run tests, so we can test adapter-compliance and even the example repo if we want. There are no limits :-)
• who is responsible for release management and the deployment keys, npm repo etc.?
• do we also want to release to GitHub packages as a redundancy factor?
• what else....?
• do we want to hold these information in a `MAINTAINERS.md? ?

Scope Bug still existing?

I have the bad feeling, that this bug was never fixed.

oauthjs/node-oauth2-server#550

New linter

As discussed per #5 ,we are looking into implementing ESLint as our linter.

In the original project, it appeared that ESLint was going to be part of some version.

https://github.com/oauthjs/node-oauth2-server/blob/dev/.eslintrc

However this never got merged into master.

The goal here is to

1. Define a eslint config we all can agree on
2. Then create a PR that contains the ESLint config along with all additional fixes / formatting we implement (if possible).