-
Notifications
You must be signed in to change notification settings - Fork 116
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
chore(npm-dependency): upgrades npm to 8.11.0 to resolve security vulnerability. #501
base: master
Are you sure you want to change the base?
chore(npm-dependency): upgrades npm to 8.11.0 to resolve security vulnerability. #501
Conversation
db2bb4d
to
0d40668
Compare
this link does not seem to point to the advisory that you meant for it to point to. could you provide more information? normally we get PRs from renovate and dependabot when a vulnerability it able to be fixed with an upgrade, so i'm wondering why we haven't gotten a PR from either in this case. |
Here's the direct link to the GH advisory GHSA-hj9c-8jmm-8c52 (the PR description has been updated as well). Sorry for the lack of details on this PR yet, I primarily marked it "Ready for review" because its draft status seemed to be preventing checks from running. I've been unable to get tests to pass locally, but I get the same failures on
|
while we're certainly open to raising the minimum to force the tools to install a patched version, the version that this PR updates npm to is within the existing range, so you should be able to resolve the audit alert by updating your lockfile. even better, we recommend running semantic-release using
we have some known problems with our test suite that need attention. are you using an M1 Mac, by chance? if so, you are likely running into the problems described in semantic-release/semantic-release#2478, which also impact this project.
since you are a first time contributor, github requires a maintainer to approve execution of the github actions in order to reduce abuse of compute time on their infrastructure. unfortunately, that complicates feedback for legitimate contributors. we do need to get the verification all passing in order to merge this change, though. i'm happy to keep approving, so feel free to ping me if i dont notice for a while. in the meantime, try uninstalling semantic-release and reinstalling to rebuild that part of your lockfile or migrate to using npx for execution. |
regarding this particular vulnerability, while i understand it may not be impacting your project specifically, this is a good time to at least mention for future readers... we always recommend defining |
since it looks like the current failure is in the matrix build for node v17, it could be worth removing that non-lts node version from our matrix and replacing it with the upcoming LTS v18 |
i notice that the lockfile only is updated for npm itself. that seems very unlikely if the lockfile was updated with the npm cli since dependencies of npm are almost certainly bumped as part of the npm update. did you updated the lockfile manually, by chance? |
No, Windows 10 on a Dell.
Historically, we've been averse to
No, I updated the |
thanks for confirming. that does make sense since we were already up to date within the range |
it appears that the failure that is happening for this PR is also happening for v17 and v18 in the mainline branch, even though the commit originally passed when it was added to the mainline. while i can reproduce the problem, i do not understand why this would be happening. while that means that the failure was not caused by this change, we still need to fix it before merging. i'm not sure i will have a chance to fix it right away, so help in investigating would be appreciated. it may make sense to fix that problem in a separate PR, but fixing it here would be fine too. |
knowing that and looking back at your output, i'm wondering if your local failures are related to line endings, especially since the line ending is shown in your output. i wonder if there is a good way to update the tests to be more forgiving of that |
As far back as v7.1.3, these tests fail with the same error on my machine. It doesn't seem to be line-ending related, as the test output and some debugging indicates that the failures are legitimate, and the pre-existing |
yarn audit
, it does not show up in our dependabot alerts, which may be way it has not received an automatic PR from dependabot in this repo