fix: patch d3 libraries with victory-vendor to fix security vulnerability #3120
Conversation
|
Built and tested code in https://github.com/ckifer/recharts/releases/tag/v2.3-beta.2 my fork. Installed and used in a next project with success. |
|
|
|
Do we have a reproducible example where the upgrade broke an application? |
|
@nikolasrieble yeah its pretty easy to reproduce. The broken build in the link I posted proves it breaks jest when upgrading from what we have today. If you install 2.1.13 in a next project or project with jest or next it will break the test run/build/dev server with something along the lines of: I'm not sure what the release process is for recharts or else I was going to produce a real alpha release. Otherwise I've been copying my built folders to replace what is in node_modules in a project. I've reproduced both success and fail but it's hard to give that reproduction to others. |
| "eventemitter3": "^4.0.1", | ||
| "lodash": "^4.17.19", | ||
| "react-is": "^16.10.2", | ||
| "react-resize-detector": "^7.1.2", | ||
| "react-smooth": "^2.0.1", | ||
| "recharts-scale": "^0.4.4", | ||
| "reduce-css-calc": "^2.1.8" | ||
| "reduce-css-calc": "^2.1.8", | ||
| "victory-vendor": "^36.6.8" |
There was a problem hiding this comment.
Could you please document the context of this workaround and the next steps?
You could write a comment in the package.json right above this line.
There was a problem hiding this comment.
Imo we might need a better more visible place for documenting some of the technical decisions we're making like this one. Not sure where though
There was a problem hiding this comment.
Comments are not permitted in json files, I will add my documentation steps in the thread instead of the review so they're easier to see
There was a problem hiding this comment.
Code wise not much is happening here, this is good to go.
What is missing is documentation on why this was done. This knowledge is right now distributed across the slack channel and multiple issues.
Please do write a summary comment about the motivation for this fix, and the next steps to take here (and when to take them).
|
since comments aren't permitted in json what I did was create an FAQ in the GitHub wiki then I added a few lines to the README I think we need a place to document technical decisions without needing to deploy the website, wiki fits this decently. README itself could get messy but we could add a smaller FAQ to the readme for more visibility. Lets discuss. This isn't pressing to merge since I can't release without Arthur |
|
Closed by #3167 |
Description
D3 moved to vending ESM only. Our consumers use jest, next, etc. which requires cjs - if we patch d3 security vulnerability in a patch fix we break our consumers. In order for v2.x.x to be secure we need a cjs version of patched d3.
victory-vendordoes this for us as per blog post https://formidable.com/blog/2022/victory-esm/change d3 references to
victory-vendor, remove direct d3 dependenciesRelated Issue
#3012 #3009, more
Motivation and Context
Fix dependency vulnerability that will not go away in 2.x.x due to d3 not wanting to upgrade
How Has This Been Tested?
See comment here #3012 (comment)
Created two duplicate branches. One I upgraded the dependencies directly, this one I replaced with
victory-vendor. Due to adding jest tests to our test suite we now have a failing build when directly upgrading the deps.Many references are also in our demo project - these continue to work as well. Tested manually.
npm run testnpm run buildnpm run demoScreenshots (if appropriate): N/A
Types of changes
Checklist: