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

Dependency security updates #76

Merged
merged 3 commits into from
May 29, 2020
Merged

Dependency security updates #76

merged 3 commits into from
May 29, 2020

Conversation

byrond
Copy link
Collaborator

@byrond byrond commented May 14, 2020

Updates the following dependencies in order to upgrade vulnerable versions of acorn and minimist:

  • eslint
  • react-scripts
  • node-sass-chokidar
  • cypress

Testing

@byrond byrond self-assigned this May 14, 2020
@byrond byrond added the Work in-Progress Do not merge PR, currently being worked on. label May 15, 2020
@byrond
Copy link
Collaborator Author

byrond commented May 15, 2020

I tried yarn start and got the following:

There might be a problem with the project dependency tree.
It is likely not a bug in Create React App, but something you need to fix locally.

The react-scripts package provided by Create React App requires a dependency:

  "babel-loader": "8.1.0"

Don't try to install it manually: your package manager does it automatically.
However, a different version of babel-loader was detected higher up in the tree:

  /Users/byronduvall/federated-search-react/node_modules/babel-loader (version: 8.0.6) 

Manually installing incompatible versions is known to cause hard-to-debug issues.

If you would prefer to ignore this check, add SKIP_PREFLIGHT_CHECK=true to an .env file in your project.
That will permanently disable this message but you might encounter other issues.

@byrond
Copy link
Collaborator Author

byrond commented May 15, 2020

I removed babel-loader from package.json (since it's already a dependency of react-scripts, ran yarn install and got:

**ERROR** Failed to apply patch for package react-scripts at path
  
    node_modules/react-scripts

  This error was caused because react-scripts has changed since you
  made the patch file for it. This introduced conflicts with your patch,
  just like a merge conflict in Git when separate incompatible changes are
  made to the same piece of code.

  Maybe this means your patch file is no longer necessary, in which case
  hooray! Just delete it!

  Otherwise, you need to generate a new patch file.

@byrond
Copy link
Collaborator Author

byrond commented May 15, 2020

I manually fixed the line number in the react-scripts patch to match the location in the new version and updated the patch filename. yarn install completed, and yarn start now gives:

There might be a problem with the project dependency tree.
It is likely not a bug in Create React App, but something you need to fix locally.

The react-scripts package provided by Create React App requires a dependency:

  "webpack": "4.42.0"

@byrond
Copy link
Collaborator Author

byrond commented May 15, 2020

I removed webpack from package.json, removed node_modules, re-ran yarn install, and `yarn start completed successfully!

@byrond
Copy link
Collaborator Author

byrond commented May 15, 2020

I tested this with the Fed Search Demo D8 site, as well as ran the Cypress test. Seems like everything still works!

@biz123 biz123 self-requested a review May 27, 2020 18:43
@biz123 biz123 self-assigned this May 27, 2020
@biz123
Copy link
Contributor

biz123 commented May 29, 2020

@byrond this is working as expected. One thing that is odd is that it appears Github is no longer flagging those versions of Acorn and Minimist, but I see the previously flagged versions from their email and I have verified those are no longer in the lock file:
Screen Shot 2020-05-29 at 8 04 20 AM

@biz123
Copy link
Contributor

biz123 commented May 29, 2020

The alerts were withdrawn:
GHSA-7fhm-mqm4-2wp7

@biz123
Copy link
Contributor

biz123 commented May 29, 2020

Since this cleans out packages that are not needed this is good cleanup even though the security alerts were withdrawn.

@biz123
Copy link
Contributor

biz123 commented May 29, 2020

I also confirmed Cypress tests run as expected.

@byrond byrond merged commit d8259bf into 3.x May 29, 2020
@byrond byrond deleted the dependency-security-updates branch May 29, 2020 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Work in-Progress Do not merge PR, currently being worked on.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants