Get rid of dependencies?

[jankapunkt]

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.

[jwerre]

I'm going to look more into this today. At first glance think we should definitely remove basic-auth but I agree type-is would be more difficult.

[HappyZombies]

It seems that the reason for basic-auth was for safe buffers support so it could support older versions of Node. What would we simply parse the basic auth headers ourselves then?

[jwerre]

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

[Uzlopak]

I would prefer that we remove those dependencies. Just simply by the fact, that it reduces the surface for potential attack vectors.

[Uzlopak]

@jwerre

You mentioned that integrating or resolving those dependencies would be a major version. I kind of disagree. It would be more of a minor change, as the interfaces would not be changed.

[jwerre]

@Uzlopak I think what you did would be fine for a minor change but I don't think we should throw the errors. They should be returned.

[Uzlopak]

Hmm, we could return Errors, but would this not result in a deoptimization by V8 as the function gets polymorphic?

[jankapunkt]

These decisions (throw vs return) should also be consulted by the OAuth 2.0 standard RFC(s) in order to see what it states. We need to be compliant before being performant.

[jwerre]

First off @Uzlopak, thank you for taking the time to do this. I'm fine with throwing errors but I'm not a fan of throwing frivolous errors. Let's not crash the process when ultimately this is all that's true:

if (typeof string !== 'string') {
    return undefined;
}

getClientCredentials should only throw an invalid_client which it does here.

The errors thrown on lines 53, 57, and 83 should also be invalid_client errors. It is my option that it's better to remove them entirely and return undefined or {} and allow the invalid_client to throw.

[jwerre]

Can you also add the grant type check after line 176 similar to the one on line 202

[Uzlopak]

I actually think that your suggested code is much better than taking that old stuff from basic-auth package.
I mean, the low complexity of the original package is not worth the potential supply chain attack vector for this package.

[jorenvandeweyer]

@jankapunkt any update on you want to approach these dependencies?

[jankapunkt]

@jorenvandeweyer I think we keep the basic-auth and the type-is dependencies. However, we should keep this issue open in order to mark this topic as important. There may be the time we may have to fork or drop these deps.