Node-jsonwebtoken: Remove Dependencies

[vokeio]

Are there any objections towards removing some of the package dependence? Particularly the Lodash ones, they are easy to write. I don't mind contributing to do this myself I just wanted to verify that this would be acceptable.

[ziluvatar]

Hi! What is the motivation for that? Even if it is easy to write it means: more code not related to the business of this library to maintain + add possible errors in code.

[vokeio]

I find value in packages which strive to be dependencies less. I especially think when the package is utilized for security or authorization/authentication it is most valuable. To an extent and with exception I think the Application/Program (the end goal) is the only one that should have dependencies. At least that is how I think or try to write my code. I like to look at my node_modules folder and know exactly what is in there.

Also before I utilize any package in my project I like to read the project dependencies. I do this for multiple reasons but the primary reason is security every dependencies should be considered a potential security threat.

I am not suggesting the removal of all dependencies just those that are foundation JavaScript knowledge and that have builtin utilities for such handling. And the potential for error is not necessarily removed it is just passed on to someone else. The risk of error for something like type check in my opinion is very low.

[ziluvatar]

every dependencies should be considered a potential security threat.

Agree.

And the potential for error is not necessarily removed it is just passed on to someone else.

I think this is the tradeoff you need to make when you choose a library, you need to be sure that the library is well maintained with a good community behind it. In this case lodash is well known library, I'm not saying there will be no errors, but I'm sure they will be found faster since it is their business logic (validations in this case).

In any case, @MitMaro is working improving the tests and adding ton of edge case test validations. Whenever that's ready, if you want you could pick one dependency and create a PR as proof of concept.

[paulocoghi]

Would you consider it helpful to use Rollup to bundle the code? If this is interesting for you, I can make a PR with an example.

This doesn't solve the point of @AlexanderElias , but we possibly have some advantages when bundling everything into a unique library file with only the parts used from each dependency (Rollup does this automatically).

Some interesting points:

• when someone is just using the library, no dependencies are downloaded, which is fast and nice

• all the dependencies becomes "devDependencies", and are only downloaded when someone is developing on node-jsonwebtoken

• it could, theoretically, reduce the possibilities of errors, vulnerabilities or exploitations, since just part of each dependency (the used part) is included. Less code, less problems.

• the library remains usable and downloadable to newcomers even when someone removes a package from npm that's a dependency, giving time to find a solution/alternative

:)

[ziluvatar]

@paulocoghi thanks for sharing, I didn't know about it. I'll take a look, although, I don't think we would need to add that complexity since I see that more useful for browser ecosystem than nodeJS one.

[worc]

i understand wanting to reduce the amount of maintenance on the project, but looking at what you guys are using from lodash (https://github.com/auth0/node-jsonwebtoken/blob/master/sign.js#L3-L9), you're taking on extra overhead (versioning, auditing, worrying about rollup) for some pretty basic functionality.

lodash's isBoolean, isString, isNumber are pretty straightforward:

isString and isNumber are both type-coerced typeofs, isBoolean uses strict equality against... booleans, and the only extras from lodash is a small fallback for each of the primitives' object forms.

after events like eslint or left-pad it seems like unnecessary risk and unnecessary auditing overhead to support a whole library for a few lines of duck typing.

[callum-p]

Additionally, the lodash used by this project has a vulnerability:

[19/34] lodash.isboolean 3.0.3  [VULNERABLE]   1 known vulnerabilities affecting installed version
[CVE-2018-3721] lodash node module before 4.17.5 suffers from a Modification of Assumed-Immutabl...
lodash node module before 4.17.5 suffers from a Modification of Assumed-Immutable Data (MAID) vulnerability via defaultsDeep, merge, and mergeWith functions, which allows a malicious user to modify the prototype of "Object" via __proto__, causing the addition or modification of an existing property that will exist on all objects.
ID: 12e63c9c-b3f9-42d3-8541-dca1b72cad69
Details: https://ossindex.sonatype.org/vuln/12e63c9c-b3f9-42d3-8541-dca1b72cad69
Dependency path: /jsonwebtoken/lodash.isboolean
------------------------------------------------------------

[ziluvatar]

Not sure how that vulnerability report made that relation 🤔

lodash.isboolean does not use any of defaultsDeep, merge, or mergeWith functions.

The vulnerability is on the main package (lodash) that is only used as a dependency for one of our dev dependencies (where we actually have some more https://github.com/auth0/node-jsonwebtoken/pull/481)

┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Low           │ Prototype Pollution                                          │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ lodash                                                       │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ >=4.17.5                                                     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ cost-of-modules [dev]                                        │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ cost-of-modules > cli-table2 > lodash                        │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://nodesecurity.io/advisories/577                       │
└───────────────┴──────────────────────────────────────────────────────────────┘

Some of your statements make perfect sense, however, answering the original question, we will stick to the current lodash dependencies until an unknown-yet event happens that would make us reevaluate it (similar happened to Joi, we used to have it too).

[worc]

we will stick to the current lodash dependencies until an unknown-yet event happens

i mean, that's a perfectly fine stance to have if you're not involved with authentication and verification, but isn't this the same thing as saying we'll worry about it AFTER we get hit by a zero-day?

and, to clarify, i feel like if you're responsible for authentication and verification your stance should be more constantly reassess our risk and reduce our exposure wherever we can.

from there, i think you can make a stronger argument for keeping outside dependencies. you can argue that you trust them more than you trust yourselves to handle potential vulnerabilities or to implement error-prone features correctly. that's why encoding and encrypting rely on the standard library and not a homegrown solution. and what's being said here is we're not sure isBoolean, isString and isNumber fall into that category.

[ziluvatar]

ziluvatar: we will stick to the current lodash dependencies until an unknown-yet event happens

worc: isn't this the same thing as saying we'll worry about it AFTER we get hit by a zero-day?

You extrapolated my comment beyond its intent. I meant things like, ie: lodash get unmaintained somehow, not compatible with new version of node and we need to upgrade to next version of node, then you need to look for alternatives (other lib? internal implementation?)

if you're responsible for authentication and verification your stance should be more constantly reassess our risk and reduce our exposure wherever we can.

Each layer on that chain needs to consider that statement, this library is consumer of "jws" and that one a consumer of "jwa", and any consumer of us is providing some kind of verification to their final consumers probably.

When I compare both repos:

|Library|Watchers|Stars|Forks|Contributors|
|-------|---------|------|-----|------------|
|Lodash|860|35,168|3,657|271|
|This one|273|8,691|632|69|

I can not stop thinking that Lodash will have more eyes looking for fixes than here to our internal functions.

[worc]

i don't think the concern is that you'd need more eyes on fixes for isBoolean or isString or isNumber. i think the concern is that by having lodash as a dependency, you're bringing in those 271 contributors in on your project, plus any dependencies and contributors lodash itself pulls in. the deep and murky dependency chain is the risk-factor here, not anything specific about the duck-typing checkers.

i know that it's reactive, but i think it's important to remember that the eslint exploit wasn't something that could be stopped by listing it as a devDependency or only using parts of the dependency or by not using eslint. the exfiltration wasn't in the code meant for consumption, it was part of the installation step, and the exploit was in the implicit trust we put into the things we list in package.json, not in any technical flaw in a type-checker.

[worc]

since this is a point i feel needs re-re-re-underlining, the issue is not the source code itself, it's not the implementations, it's not any technical workaround or hack or clever bit of code that we should be worried about. event-stream was not clever code, it was a user extending their publishing rights to a third, malicious party. the root of the problem is not the code within the dependency tree, the problem is the large network of relatively unknown actors that you bring in with each package.

[forty]

Why wouldn't you simply depends on lodash directly? The more everyone use the same packages, the more secure it is (more eyes are watching, big projects have less risk of being unmaintained or given to the first guy asking for it without anyone noticing) and this avoid duplicating packages / feature for nothing. Virtually everyone is using some of the lodash functions or some equivalent. If everyone just used lodash instead, I would have have to have isobj, isobject, is-object, lodash.isobject, isarray, isboolean, lodash.isboolean, is-boolean, ... (you see my point) as sub dependencies.
It is sad the the nodejs standard lib is so poor, and everyone seems to need to depends on 100 packages to do anything without duplicating useless code, but the only thing we can do here is to have de facto standard libs that everyone uses and maintain

[worc]

Why wouldn't you simply depends on lodash directly?

because two most recent news-making exploits were designed around the trust we put into libraries and the imports from lodash are not exactly necessary

[forty]

Lodash is fine, it's a well known library. Removing the dependency works as well, but most big project is going to depend on lodash directly or indirectly anyway, so it's not a big deal to have it even it's it's not that useful

[worc]

Lodash is fine, it's a well known library.

i don't think you're following. popularity isn't security. there's an argument there that an active, contributing community might be in a better position to police publishing rights and catch bad actors. but by depending on lodash, node-jsonwebtoken is implicitly and indirectly extending trust to those 271 contributors to lodash.

that's not a trivial amount of risk for three very basic duck-typing functions.

[forty]

I said well known, which in npmland is very different from popular :)

[worc]

@forty i think my point still stands. or were you implying something else?

@MitMaro did you have something to add?

[MitMaro]

@worc, I don't have much to add that others haven't already said, hence my support for some comments, but I can reiterate.

Security is about risk management and in this situation there are two avenues of risk.

The first avenue of risk in this project is the inclusion of Lodash in the project. As you have correctly identified, this adds the inherent risk of depending on the development and release management of the Lodash project. This risk is lessened by the vast number of developers and security experts who review and look for security issues in the Lodash project. The case of eslint-scope that you mentioned is a great example of this. When eslint-scope was compromised on July 12 it was discovered within an hour of the compromised package being published, and the compromise disabled within a couple hours (source). This is the strength of large open source projects. To counter that point, there are cases that take much longer to discover, such as the recent event-stream compromise that did take longer to discover and fix.

The second avenue is replacing dependencies by writing the functionality into the library. As you correctly point out, this removes the security concerns with using an external dependency. This, however, does add another security concern, which is that developers make mistakes and write insecure code. If I, or any developer, on this project were to write a replacement to the Lodash functionality there is a very good chance that we would introduce the same or similar security issues that the developers of Lodash introduced. An example of that occurred with this library, which had a serious security flaw for several months to years before it was discovered and fixed (source).

Security is never black and white, there are tradeoffs and compromises that need to be made. There is rarely a perfect solution that is perfectly secure. The maintainer of this library has made the decision that trusting in the community that supports Lodash is the avenue they wish to follow. On a personal note, and as a developer who uses this library, I fully agree and support that decision. If there was a dependency on a lesser known and supported third-party dependency, I would have been behind you in replacing that dependency with either something better supported or with a homebrew solution.

With that said, there are some Lodash dependencies that I think could be removed, not for security reasons, but because JavaScript provides the same functionality. Right now there are a lot of missing and broken tests, that I have been volunteering my time to try to add and fix. I think that making unnecessary changes to the project before the test suite is updated would be unwise and add additional risk.

I fully understand your point of view, but I agree with the maintainer of this project that as the project currently stands the Lodash dependencies are not a security problem and should not be removed.

[worc]

well said. i still strongly disagree that there is a "very good chance" of introducing security issues by vendoring your duck-typing. but i think if that's the direction, and if there's this level of thought put into accepting the risks, then this issue should be closed.