fix: [security] Replace colors with chalk. #166
Conversation
|
+1 The maintainer of the colors package has purposely deployed a corrupted breaking change in protest of not getting funding. https://snyk.io/blog/open-source-maintainer-pulls-the-plug-on-npm-packages-colors-and-faker-now-what/ This issue has run-on effects with Firebase-tools (which uses cli-tables as a dependancy) And with github actions firebase-hosting-deploy ( which uses firebase-tools as a dependancy ) See image below ( garbage output from dependancy "colors" ) |
| @@ -11,7 +11,7 @@ | |||
| }, | |||
| "keywords": ["cli", "colors", "table"], | |||
| "dependencies": { | |||
| "colors": "1.0.3" | |||
| "chalk": "^4.1.2" | |||
There was a problem hiding this comment.
Why not pin the chalk version, just in case
There was a problem hiding this comment.
Pinning in a library is generally a bad idea (present circumstances excluded). If we pinned this to chalk 4.1.2, and a bug fix came out in 4.1.3, we’d have to merge a patch to this library and then all consumers of this library would have to upgrade this and chalk. A critical security vulnerability could take a long time to propagate its way down the dependency tree.
It also means if you have this dependency pinned at 4.1.2, and you use some other library that pins it at 4.1.1, then you needlessly end up with two copies of chalk in your node_modules.
Of course, if you’re consuming cli-table in an application, you absolutely should pin this dependency via a package-lock.json or yarn.lock.
There was a problem hiding this comment.
I see pinning as an interim solution.
Moving to a new package requires planning and testing, and in the meanwhile pinning the dependency to a specific, non-broken version can be a good idea. I think that will be our plan here.
There was a problem hiding this comment.
It's true that pinning can slow down deployments of fixes in scenarios like this (installed software like a CLI), but it also protects against these things just as much. As evidenced here, package maintainers can be unstable, or accounts can get compromised. If things had been fully pinned, we wouldn't be scrambling to address the issue from colors.
There was a problem hiding this comment.
If you have a lockfile, you’re not scrambling to fix anything related to colors. Only new installs should be affected by the mess with colors.
(As an aside, If you’re curious to know what kind of a world we’d be living in if libraries pinned all their dependencies in package.json directly, I’d invite you to have a look at this PR. Not quite this exact scenario, but this is the sort of thing we’d all be doing on a regular basis:
There was a problem hiding this comment.
Will we have the merge of this PR?
In our projects we are using the cli-table. Otherwise, we will have to change it here.
Can we proceed with this PR?
The maintainer of colors has purposely introduced an infinite loop in the latest version of colors. Therefore we need to ditch this dependency. cli-table must also be updated in order to get rid of the colors dependency entirely. There is a PR: Automattic/cli-table#166 Or maybe an alternative table lib should be considered as well?
|
Hey folks -- I'm grateful the community has been proactive here :) I opened #169 which allows |
Description
This replaces the
colorslibrary withchalkafter the author of colors added an infinite loop to colors/lib/index.js as a protest.cli-table is actually already safe, because it had colors.js pinned to an old version, but I'm doing my best to remove the author's code from popular packages because it makes it easier to audit my toolchain.
Checklist
Please make sure the items below have been covered before requesting a review:
Steps to Test or Reproduce
Existing color test cases are passing.