d3-geo
d3-geo copied to clipboard
d3
Update tar to avoid high severity security alert
My local mirror of this project receieves dependbot alerts
https://github.com/martinfrances107/d3-geo/security/dependabot
and there is a high severity alert associated with our use of the tar package.
https://github.com/advisories/GHSA-5955-9wpr-37jh
To our project tar is pull in as canvas in pulled.
Here is a copy of npm why tar .. after the fix.
I am about to post a PR.
[email protected] dev
node_modules/tar
tar@"^6.1.0" from @mapbox/[email protected]
node_modules/@mapbox/node-pre-gyp
@mapbox/node-pre-gyp@"^1.0.0" from [email protected]
node_modules/canvas
dev canvas@"2.8" from the root project
Sorry for the confusion this is not a duplicate of #248
The automated PR supplied by the bot is incorrect.
The other patch just modified the yarn.lock file .. This PR updates the canvas dependancy .. .which in turn pulls in the correct version of tar
I can't reopen this issue... and I don't think correcting the bots patch ok
What would you like to happen?
I think https://github.com/d3/d3-geo/commit/0c27527d4f9f500698b28d33289ee1104ee22990 is correct? You just have to run yarn again on the project.
I think 0c27527 is correct?
No I don't think it is correct.
.lock files are the output, the conclusion of all the dependancy checking.
The lock files are manifest of all the finalized pulled in modules.
package.json is the place where the user specifies the desired packages.
but modfiying ONLY the lock files .. I suggest what has happened is the we have created a maintenece hazard.
Specifically future developers updating the package.json will acidentitly see the conditions on tar that we have specified only in the lock file wiped way in the update. Bringing back the problem.
So I have advanced several line of logic
a) in the issue ...I discussed the output of "npm why" which details why canvas must be modified
b) In the comment above I show the step by which future modifications to the repo which reintroduce the problem..
c) In terms of reviewing my own work .. there is maybe a conern that blindly bumping the minor version of canvas will affect users by the thousands if it breaks their code.( minor versions are not supposed have breaking changes by convention... but that is not a guareentee. )
When you say I am not convinced... which part is bothering you .. a, b or c? I mean this in a friendly .. we are having a discussion, sort of way. I need a little more info, to be of service.
My understanding is that when you do a fresh yarn or npm install on the current repo (with a package.json that has canvas: "2"), and remove/ignore the yarn.lock file and previous node_modules, it will install the most current version of each package that matches the requirements—in this case it will be:
canvas@2:
resolved "https://registry.yarnpkg.com/canvas/-/canvas-2.8.0.tgz#f99ca7f25e6e26686661ffa4fec1239bbef74461"
tar@^6.1.0:
resolved "https://registry.yarnpkg.com/tar/-/tar-6.1.11.tgz#6760a38f003afa1b2ffd0ffe9e9abbd0eab3d621"
If you don't disregard the yarn.lock file, then it will load the versions that are indicated (which are the same). So, in all the cases we're safe.
Removing the yarn.lock (and node_,modules) and running yarn is equivalent to running yarn upgrade, FYI.
Ok so here is the line of reasoning which says the suggestion from above is seemingly equivalent but not strictly so..
When yarn upgrade is running and all the dependencies are recalulated.
It works in this instance because the package reconciliation algorithm :-
A) Find the version number ranges for each package the will provide a solution. B) Pick the highest number, the lastest version, in those ranges.
The maintaince hazrad occur that it is not certain in general to pull a known safe version of tar.
If the next developer comming down the road want to adjust a seemingly unrelated package depenecy in package.json then as the complex dependencies are re-evaluated canvas maybe downgraded to meet the new enviroment. We can't always rely on mechamism B to do the right thing.
My patch is different in that by locking down canvas it locks down tar to a secure version.
I see, thanks for the detailed write-up. I did not say that the two approaches were equivalent :) But, I think yarn upgrade is "correct", in the sense that it works, and is enough in this case.
Note that there is no certainty that tar is safe in its current version—we just happen to know that the previous version wasn't. We have to rely on recurring inspection of the security databases, for which we have dependabot set up; a cron running yarn audit at regular intervals would be fine too.
It could be a good idea to add yarn audit in the CI tests, by the way. (We wouldn't want to add them to local unit tests since they don't work offline—and they're slow.)
Please forgive me but that sounds like a really bad approach to the hazard