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

Upgrade Front-end Dependencies #3187

Merged
merged 7 commits into from Oct 25, 2019
Merged

Conversation

@rajadain
Copy link
Member

rajadain commented Oct 24, 2019

Overview

Upgrades a number of dependencies to resolve security issues. Nunjucks has not been upgraded and is relegated to #3140.

Connects #3182

Testing Instructions

  • Check out this branch
  • Remove your node_modules $ rm -rf src/mmw/node_modules
  • Destroy and recreate app VM $ vagrant destroy -f app && vagrant up
  • Rebundle everything $ ./scripts/bundle.sh --debug --vendor --tests
  • Ensure tests run successfully $ ./scripts/test.sh
  • Ensure project still works correctly by clicking around
  • Ensure bundle watching still works by running $ ./scripts/bundle.sh --watch and making changes to the codebase (maybe change the title here) and saving the file, and reloading the page to see your changes
  • Ensure linting still works $ vagrant ssh app -c 'cd /opt/app && yarn run lint'
  • Do a security audit and ensure there's no issues outside of Nunjucks $ vagrant ssh app -c 'cd /opt/app && yarn audit'
rajadain added 5 commits Oct 24, 2019
Transitively upgrading diff, growl, minimatch
@rajadain rajadain added the PA DEP label Oct 24, 2019
@rajadain rajadain requested a review from mmcfarland Oct 24, 2019
Ansible fails to install a version of Firefox matching `6*` due to more
recent releases becoming available in the repository. Upgrading prevents
provisining errors.
Copy link
Member

mmcfarland left a comment

I upgraded FF in 88e91e7 in order to get the app VM to provision. Once I did that, tests pass. However, I do see one low security issue outside of Nunjucks:

 low           │ Regular Expression Denial of Service                         │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ clean-css                                                    │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ >=4.1.11                                                     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ jstify                                                       │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ jstify > html-minifier > clean-css                           │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://www.npmjs.com/advisories/785                    

App and build tools continue to function correctly.

@rajadain

This comment has been minimized.

Copy link
Member Author

rajadain commented Oct 25, 2019

We are on the most recent release of jstify, which is from almost four years ago. I'll see if upgrading clean-css separately helps, but sometimes packages pull in specific versions of dependencies, making it impossible unless jstify is removed.

Since we now use yarn for managing front-end dependencies.
Also update README with new instructions.
@rajadain

This comment has been minimized.

Copy link
Member Author

rajadain commented Oct 25, 2019

I updated clean-css manually but it did not removing the infringing version:

$ kj yarn why clean-css

yarn why v1.19.1
[1/4] Why do we have the module "clean-css"...?
[2/4] Initialising dependency graph...
[3/4] Finding dependency...
[4/4] Calculating file sizes...
=> Found "clean-css@4.2.1"
info Has been hoisted to "clean-css"
info This module exists because it's specified in "dependencies".
info Disk size without dependencies: "119.23MB"
info Disk size with unique dependencies: "138.27MB"
info Disk size with transitive dependencies: "138.27MB"
info Number of shared dependencies: 1
=> Found "html-minifier#clean-css@3.4.28"
info This module exists because "jstify#html-minifier" depends on it.
info Disk size without dependencies: "71.22MB"
info Disk size with unique dependencies: "96.27MB"
info Disk size with transitive dependencies: "96.27MB"
info Number of shared dependencies: 2
Done in 1.07s.

I think we should mark it same as Nunjucks.

@mmcfarland mmcfarland self-requested a review Oct 25, 2019
Copy link
Member

mmcfarland left a comment

Sounds good to me. Everything looks good if my FF commit is 👍 .

@mmcfarland mmcfarland assigned rajadain and unassigned mmcfarland Oct 25, 2019
@rajadain

This comment has been minimized.

Copy link
Member Author

rajadain commented Oct 25, 2019

Yes, thanks for adding that!

@rajadain rajadain merged commit 51367b8 into develop Oct 25, 2019
2 checks passed
2 checks passed
default Build finished.
Details
model-my-watershed-pull-requests Build #4107 succeeded in 8 min 48 sec
Details
@rajadain rajadain deleted the tt/upgrade-front-end-dependencies branch Oct 25, 2019
@rajadain

This comment has been minimized.

Copy link
Member Author

rajadain commented Oct 25, 2019

Also snuck in a commit that replaces npm.sh with a yarn.sh, and updated the README instructions for installing new dependencies. Thanks for reviewing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.