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

fix: resolve dependency audit issue #3226

Closed
wants to merge 1 commit into from

Conversation

jaranin-b
Copy link

To solve dependency audit issue: https://www.npmjs.com/advisories/1080923

Screen Shot 2565-07-06 at 20 53 20

Description

Motivation and Context

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (change that has absolutely no effect on users)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Copy link
Member

@JamesHenry JamesHenry left a comment

Choose a reason for hiding this comment

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

Hi @jaranin-b, thanks for this, when updating dependencies you need to make sure you run an npm install at the root of the repo so that the lock file gets updated

@jaranin-b
Copy link
Author

Thanks @JamesHenry. Updated the lock file.

@JamesHenry
Copy link
Member

@jaranin-b it seems extremely unlikely that this is correct:

image

Please revert and try again. You should simply need to run npm install at the root and be using an appropriate version of npm. Please try with npm 8.12.1 to be safe.

ThisIsMissEm
ThisIsMissEm previously approved these changes Jul 6, 2022
Copy link
Contributor

@ThisIsMissEm ThisIsMissEm left a comment

Choose a reason for hiding this comment

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

This change looks good and matches my own investigation into this issue (I've reviewed the changelogs of git-up, git-url-parse and parse-url to see if there's any breaking changes that would affect lerna, and as far as I can tell, there is not)

Note: the package-lock still needs to be fixed.

@jaranin-b
Copy link
Author

Thanks @ThisIsMissEm @JamesHenry I've updated the lock file that generated using npm@8.12.1

@JamesHenry
Copy link
Member

@jaranin-b I'm sorry but you are still generating 1000s of lines of change in the lockfile which is definitely unexpected:

image

I have recreated the change in #3231 and the change is appropriately small:

image

In order to land this today, I am going to press ahead with #3231 but thank you very much for taking the initiative to submit this PR!

@JamesHenry JamesHenry closed this Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants