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

[Snyk] Security upgrade del from 6.1.1 to 8.0.0 #8031

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

JJediny
Copy link
Member

@JJediny JJediny commented Oct 11, 2024

Snyk has created this PR to fix one or more vulnerable packages in the `npm` dependencies of this project.

Changes included in this PR

  • Changes to the following files to upgrade the vulnerable dependencies to a fixed version:
    • package.json
    • package-lock.json

Vulnerabilities that will be fixed

With an upgrade:
Severity Priority Score (*) Issue Breaking Change Exploit Maturity
medium severity 631/1000
Why? Proof of Concept exploit, Has a fix available, CVSS 6.2
Missing Release of Resource after Effective Lifetime
SNYK-JS-INFLIGHT-6095116
Yes Proof of Concept

(*) Note that the real score may have changed since the PR was raised.

Commit messages
Package name: del The new version differs by 10 commits.

See the full diff

Check the changes in this PR to ensure they won't cause issues with your project.


Note: You are seeing this because you or someone else with access to this repository has authorized Snyk to open fix PRs.

For more information:
🧐 View latest project report

🛠 Adjust project settings

📚 Read more about Snyk's upgrade and patch logic


Learn how to fix vulnerabilities with free interactive lessons:

🦉 Learn about vulnerability in an interactive lesson of Snyk Learn.

Copy link

🔍 Preview in Federalist

@nick-mon1
Copy link
Contributor

nick-mon1 commented Nov 19, 2024

@RileySeaburg the del package has breaking changes from using commonJS requires to ES Module imports. I've remove del and use native node fs methods to do the directory removal work.

What I Tested

  • tested uploads with npx gulp upload and works
  • tested both images and files
  • folders are removed
  • tested npx gulp buildSprite and works
  • removed del package.

@RileySeaburg
Copy link
Member

Let's wait until #8188 is merged to merge this.

@nick-mon1
Copy link
Contributor

@RileySeaburg npm audit runs fine on my local but it still failing on circleci.

nick-mon1
nick-mon1 previously approved these changes Nov 20, 2024
RileySeaburg

This comment was marked as outdated.

@RileySeaburg RileySeaburg self-requested a review November 21, 2024 15:54
Copy link
Member

@RileySeaburg RileySeaburg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nick-mon1 Please remove npm-run-all It is no longer maintained.

Replace it with Concurrently

@@ -21,7 +21,7 @@
"prettier:js:fix": "npx prettier -w 'themes/digital.gov/src/js/*.js' 'config/typescript/**/*.ts'",
"prettier:templates": "npx prettier -c ./themes/**/*.html",
"prettier:templates:fix": "npx prettier -w ./themes/**/*.html",
"start": "npm-run-all --parallel local-build hugo workflow",
"start": "concurrently \"npm run local-build\" \"npm run hugo\" \"npm run workflow\"",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RileySeaburg Replaced with concurrently.

@@ -44,7 +44,7 @@
"@prantlf/jsonlint": "^14.0.3",
"@uswds/uswds": "^3.9.0",
"autoprefixer": "^10.4.16",
"del": "^6.1.1",
"concurrently": "^9.1.0",
"dotenv": "^16.3.1",
"eslint": "^8.56.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RileySeaburg We should update to v9 during the next POAM. Made a note of this.

@nick-mon1 nick-mon1 self-assigned this Nov 25, 2024
@nick-mon1 nick-mon1 added the NPM Fix Usually to fix an `NPM Audit` message label Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✋ HOLD NPM Fix Usually to fix an `NPM Audit` message
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants