facebook / create-react-app

Set up a modern web app by running one command.
https://create-react-app.dev
MIT License
102.72k stars 26.85k forks source link

Rethink decision to make react-scripts a dependency instead of a devDependency #11102

Open mrwensveen opened 3 years ago

mrwensveen commented 3 years ago

Is your proposal related to a problem?

The decision to make react-scripts a dependency causes a lot of issues regarding perceived security vulnerabilities. Even though the issues themselves are technically harmless, these issues often break CI/CD flows and/or end up being reported here as actual issues.

Describe the solution you'd like

The decision to make react-scripts a dependency was in my opinion ill-conceived. There are issues with having react-scrips a devDependency as stated in the original pull request:

In my opinion, a shortcut was taken to work around some problems that should have been fixed separately.

Describe alternatives you've considered

It's possible to move react-scripts to devDependencies by hand, or to eject your React application. Both solve the problem (afaict) but a lot of people are not willing to do this or are unaware of the possibility.

Another possibility would be for the developers to update dependencies whenever a vulnerability pops up as fast as possible, and/or to help developers of dependent package to fix their dependencies. Maybe get Facebook to throw some money behind a React Vulnerability Strike Team (tm) or something.

Additional context

One way or another a solution is needed. Every time some vulnerability pops up I see a lot of frustration in the comments. Even though someone always politely explains the actual security implications, there are always a few people saying (and probably more people thinking) that you don't actually care that much about security. Rationally I know this not to be true, but I get frustrated as well sometimes (I'm only human after all).

stahlmanDesign commented 3 years ago

All of these issues are duplicates or are related: https://github.com/facebook/create-react-app/issues/11136 https://github.com/facebook/create-react-app/issues/11131 https://github.com/facebook/create-react-app/issues/11118 https://github.com/facebook/create-react-app/issues/11116 https://github.com/facebook/create-react-app/issues/11109 https://github.com/facebook/create-react-app/issues/11092 https://github.com/facebook/create-react-app/issues/11081 https://github.com/facebook/create-react-app/issues/11077 https://github.com/facebook/create-react-app/issues/11067 https://github.com/facebook/create-react-app/issues/11054

and could be solved by #11102

rvramesh commented 3 years ago

I moved react-scripts to devDependencies from the default dependencies section and still got the same vulnerabilities in npm audit. Am I doing anything wrong?

npm version { 'go-portal': '0.1.0', npm: '6.14.13', ares: '1.17.1', brotli: '1.0.9', cldr: '39.0', icu: '69.1', llhttp: '2.1.3', modules: '83', napi: '8', nghttp2: '1.42.0', node: '14.17.1', openssl: '1.1.1k', tz: '2021a', unicode: '13.0', uv: '1.41.0', v8: '8.4.371.23-node.67', zlib: '1.2.11' }

mrwensveen commented 3 years ago

@rvramesh You're correct. The vulnerabilities will only be ignored when you run npm audit --production, which is usually the case when audit is integrated in your CI/CD setup. Without the option the output will show the vulnerability but with a [dev] tag.

rvramesh commented 3 years ago

@rvramesh You're correct. The vulnerabilities will only be ignored when you run npm audit --production, which is usually the case when audit is integrated in your CI/CD setup. Without the option the output will show the vulnerability but with a [dev] tag.

Thank you for the clarification. It helps

illume commented 3 years ago

Good proposal IMHO.

However I guess dev environments are also considered for security related issues. It's sometimes harder to use exploits on dev environments, but it still happens.

npm audit will still show problems, but maybe the issue-reporting-noise will only be reduced a little bit. I think most people just see the results printed after using npm - not from using npm audit --production in CI.

Seems faster security related updates would be a better fix, but moving react-scripts to devDependencies would still be an improvement.

gaearon commented 3 years ago

Yes please. Want to send a PR?

mrwensveen commented 3 years ago

I want to, but it's probably a little more involving than just rolling back PR 2657. If the original rationale is still valid, these issues should be fixed first, or at least be opened here. This could take some time (away for summer vacation).

gaearon commented 3 years ago

The broader discussion is happening in https://github.com/facebook/create-react-app/issues/11174.

rolanddo8 commented 3 years ago

yea, I got those vulnerabilities too! Anyone knows how to fix it ?

moraespaulolucas commented 3 years ago

I have same issues. When I create a react project by "create-react-app", it shows 58 vulnerabilities (16 moderate, 40 high, 2 critical)

Please let me know how to fix it.

Same here. I moved react-scripts from dependencies to devDependencies in package.json and still have the same vulnerabilities when running npm audit fix.

marcelala commented 3 years ago

I have same issues. When I create a react project by "create-react-app", it shows 58 vulnerabilities (16 moderate, 40 high, 2 critical) Please let me know how to fix it.

Same here. I moved react-scripts from dependencies to devDependencies in package.json and still have the same vulnerabilities when running npm audit fix.

samesies, 58

moraespaulolucas commented 3 years ago

Well, I ran npm audit --production like discussed in #11174 and it found 0 vulnerabilities. I think it's solved

yk-fl-aw commented 3 years ago

Though "npm audit --production" shows 0 vulnerability, I can't run scripts from command line such as "npm start", "npm run build" and etc.

ramziosta commented 2 years ago

I ran npm audit --production and moved my react-scripts to devDependencies and still getting the error. This is all new, My files were opening fine a few days ago

reuzel commented 2 years ago

Besides the audit, there may be another reason to move all(!) dependencies to devDependencies.

In my monorepo I have distinct packages for frontend and backend, where the frontend is a react-app (obviously!). Through a package dependency the front-end is included into the backend. The benefit of this approach is that the CICD pipeline knows (leveraging multi-semantic-release) that when the client package version is bumped, the server version needs to be bumped as well (which bump triggers an actual deployment down the line).

In the end, the server is only interested in the output in the build folder. However installing the app as dependency will also install all the specified app-dependencies which are not necessary in production. So my suggestion:

To make the package easier to use I would recommend the following:

This will allow people to simple do something like import clientfolder from 'my-react-app' and use that location in for example express.static to server the files.

Hope this makes sense, Joost