Remove Dependencies

[xeaone]

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.

[xeaone]

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 #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