Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
mrwensveen opened this issue Jun 14, 2021 · 16 comments

Comments

@mrwensveen
Copy link

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:

  • The distinction does not make sense. Nonetheless, tools like npm audit are dependent on this distinction.
  • Apparently some people build on the production server? They shouldn't.
  • An actual run-time dependency in the form of a polyfill is included. This could be a separate (and optional?) dependency.
  • Eject crashes in some situations (issue Ejecting crashes if no devDependencies exists in package.json #2655)
  • Possibly other issues...

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
Copy link

stahlmanDesign commented Jun 21, 2021

All of these issues are duplicates or are related:
#11136
#11131
#11118
#11116
#11109
#11092
#11081
#11077
#11067
#11054

and could be solved by #11102

@rvramesh
Copy link

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
Copy link
Author

@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
Copy link

@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
Copy link

illume commented Jun 28, 2021

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
Copy link
Contributor

gaearon commented Jul 2, 2021

Yes please. Want to send a PR?

karrui added a commit to opengovsg/FormSG that referenced this issue Jul 6, 2021
note that react-scripts is also moved to devDependencies. CRA is a build tool and thus do not truly belong in dependencies.
See their own argument: facebook/create-react-app#11102

this allows npm audit --production to raise any real vulnerabilities
karrui added a commit to opengovsg/FormSG that referenced this issue Jul 6, 2021
…devDependencies (#2313)

* chore: update dependabot to also run on form-v2/develop branch

* chore: run npm audit and npm update

* chore: move most tooling packages to devDependencies

note that react-scripts is also moved to devDependencies. CRA is a build tool and thus do not truly belong in dependencies.
See their own argument: facebook/create-react-app#11102

this allows npm audit --production to raise any real vulnerabilities

* chore: add cache flag to eslint command in .lintstagedrc
@mrwensveen
Copy link
Author

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
Copy link
Contributor

gaearon commented Jul 7, 2021

The broader discussion is happening in #11174.

tomwalsh96 added a commit to JFfarrell/summer_project that referenced this issue Jul 12, 2021
- false positives being alerted on npm audit
- fixed by moving reacts-scriptsto devDependencies as per; facebook/create-react-app#11102
tomwalsh96 added a commit to JFfarrell/summer_project that referenced this issue Jul 12, 2021
* reorganise file locations

* fixed npm audit dependencies issue

- false positives being alerted on npm audit
- fixed by moving reacts-scriptsto devDependencies as per; facebook/create-react-app#11102

* Basic Google Map Implementation 🗺

- Includes Initial unstyled Marker and Popup
@rolanddo8
Copy link

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

@moraespaulolucas
Copy link

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
Copy link

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
Copy link

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

@yk-fl-aw
Copy link

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
Copy link

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
Copy link

reuzel commented May 19, 2022

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:

  • move all dependencies that are not required in production to devDependencies, which are almost all dependencies
  • update package.json to include the build folder only in the npm package (using the files field)
  • add react-scripts build to the prepare script in package.json to ensure the app is build before publishing

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

  • add a small locator.js (and associated d.ts) to the project that has a default export the current location of the build folder on disk (after installation)
  • set the package.main field to locator.js and types to the locator.d.ts files
  • include the locator in the files export

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

julian916 added a commit to julian916/web-react-code-exercise that referenced this issue Sep 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

12 participants