Closed mpadge closed 5 years ago
Hey @mpadge. Thanks for raising this, anything with security even false alarms are worth my attention. The link to the "alert" is dead but I assume it is one like this
my_app git:(rproj2) npm audit
=== npm audit security report ===
# Run npm install react-scripts@2.1.3 to resolve 3 vulnerabilities
SEMVER WARNING: Recommended action is a potentially breaking change
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ High │ Missing Origin Validation │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package │ webpack-dev-server │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ react-scripts │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path │ react-scripts > webpack-dev-server │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info │ https://nodesecurity.io/advisories/725 │
└───────────────┴──────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Low │ Prototype pollution │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package │ merge │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ react-scripts │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path │ react-scripts > jest > jest-cli > jest-haste-map > sane > │
│ │ exec-sh > merge │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info │ https://nodesecurity.io/advisories/722 │
└───────────────┴──────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Low │ Prototype pollution │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package │ merge │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ react-scripts │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path │ react-scripts > jest > jest-cli > jest-runtime > │
│ │ jest-haste-map > sane > exec-sh > merge │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info │ https://nodesecurity.io/advisories/722 │
└───────────────┴──────────────────────────────────────────────────────────────┘
found 3 vulnerabilities (2 low, 1 high) in 14061 scanned packages
3 vulnerabilities require semver-major dependency updates.
webpack is indeed included in CRA. I think I just need to update the "template" min version of the above.
Will reference this ticket, thanks @mpadge.
OK. Next commit should remove this but for current apps we just need a couple of more steps just to be safe.
So to fix @silberzwiebel's current app:
➜ code npm -g list create-react-app
/usr/lib
└── create-react-app@2.0.3
If not please install it sudo npm i -g create-react-app
there are ways of doing this without sudo
that is a documentation issue for npm/yarn to worry about.
Remove the local version and save it by running npm remove -S create-react-app
no harm if it is already removed. From a gp
app, this should be the same:
➜ my_app git:(master) ✗ npm list create-react-app
my_app@0.1.0 .../my_app
└── (empty)
Really the only step required as per above npm audit
output, we can just run npm i react-scripts@2.1.3.1
Finally, permanent fix is in package.json
(within a geoplumber app) update the line for react-scripts
to "react-scripts": "^2.1.3"
Then I just removed the node_modules
folder and clean installed npm i
for my geoplumber app and I ran the audit again
npm audit
=== npm audit security report ===
found 0 vulnerabilities
in 36167 scanned packages
Boring background stuff: somehow a "caret" got lost in that inst
packag.json file and caused this. So apologies there.
Also, perhaps this is something that needs to go into the documentation somewhere, that I have made it a rule/convention that we rely on a -g
global installation of create-react-app
for now (till others come onboard or I find a better way of doing things). As the gp_create
function uses it on a clean directory, related to #59 in a sense, we must have create-react-app
before we create a gp app.
Therefore, this line should have never been there. Its redundant.
What does this mean? It means the global
npm package is relied upon, our package.json
gets slimmer. All this in the next commit/PR.
Seems this seemingly small issue has led to lots of genuinely useful changes. Hard for people new to it to understand the JS ecosystem. The documentation should link to the relevant places.
Yes, and documentation needs "research funding" time :)
Hope to push sometime soon but may do some extra work on this which is the root of missing the caret there.
Come on github, "fix" != "close"
Thanks, @layik! I'll have a look at your steps to fix the vulnerability next week. (BTW I disagree with "fix" != "close" :P )
here:
Didn’t know about this feature? You can use any of these keywords to close an issue via commit message:
close, closes, closed, fixes, fixed
All of them work the same, including this behavior.
But seriously, thanks @layik for taking it so seriously and fixing - great stuff!
Too Boring DR;
For the record and interested parties, so far what we have been doing is copy lines such as "react": "^16.7.0",
during gp_create()
. Then let user gp_build()
. But system
commands which is the "only"/current way of talking to npm
is asynchronous
. That means R needs to know when that has finished for geoplumber
to issue subsequent commands.
And so, a line such as "react-scripts": "2.1.3"
will become a potential (not necessarily) security vulnerability
despite the fact that on npx create-react-app
command installing the newer version.
Therefore, we fall back to current "not ideal" way of installing npm
dependencies from R. Code for future change is in the next commit and hope to work this out between R and npm
sooner than later.
PS: I never missed the caret
, it looks like npx create-react-app
creates package.json
with "react-scripts": "2.1.3"
, noticed it after pasting it here actually, but that is even more boring reading.
Morning @layik! @silberzwiebel is currently developing a
gp
framework formoveability
which encounters this alert. The offendingjs
tool iswebpack-dev-server
, which is loaded inpackage-lock.json
under genericreact-scripts
. That currently loads2.9.4
, yet the security alert says this can be solved by insitingwebpack-dev-server >= 3.1.11
. I'm not sure where best to put that as it's simply loaded as a generic part ofreact
. Can you solve this for us please?