-
Notifications
You must be signed in to change notification settings - Fork 67
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
[MAINT]: 20.x npm audit vulnerabilities #486
Comments
👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with |
In reality though, how many of those advisories are actually production dependencies? We don't pin the version used, so users can install the latest supported version |
Besides, |
I agree with your scoping reduction comments; I don't have metrics to understand how often 20.x is used, only that it is used on several projects I am looking at and I am happy to supply a PR. |
@wolfy1339 so doing an install of
That looks to be because we pinning
I think it would be good if we did do a v20 release to allow that to be addressed - either unpinning the dependencies (though I'm not sure if there was a specific reason they were pinned which might block that), or otherwise updating While we've got the v20 branch, is our release pipeline actually setup to just work if we do land a commit there, or would more work be required? (while I'm very excited with the idea of |
The reason that was pinned is because version (and it's pinned dependencies) was a special backport release for CJS users, which ended up on the wrong major version. The release flow should already be set-up to allow making releases from maintenance branches. Typescript has |
Cool - are you ok if I try looking into that further?
TypeScript was just one tool as an example - my experience in the bespoke-app type space so far has not been very smooth, often with a fix for one tool ending up breaking another; I'm very excited by As an example, did you know about the special I think it's coming together very nicely, I'm just trying to highlight it's complex and as someone heavily involved in the security patching side of things, I know from very personal experience that people will appreciate making things smoother for them. |
(I say "I" but @benpbolton if you're keen to help out and have the bandwidth, feel free to have a crack at it since this was originally your issue 😄) |
I was not aware of the Feel free to look into that |
@G-Rath I'd appreciate your involvement! I put together #487 (the approach for The published npm package has deps:
differing from github's 20.x branch in that major version bump for |
Let me make this more clear, I don't think it was fully understood. For For These 2 packages need to remain in sync with the same changes. |
@benpbolton the first thing I want to do is figure out what path we take - you have suggested one is technically possible (downgrading to v9), but that feels like an overall loss as the versions in between assumingly have features and fixes that could be useful (@wolfy1339 I assume that's effectively what you mean too when you say "without the backported changes") From what @wolfy1339 has just posted, it sounds like it might be best if we can get another v11.3.x version published with the vulnerability fixed, so first thing would be to check if we've got a pipeline setup for that - if so then it could be very straightforward, but it's possible those special versions were released manually. Once we've decided on a path forward, then we can deal with what we might need to get there e.g. updating dev dependencies - because as much as I appreciate having a healthy environment, there's a different cost in having these major branches grow with their own commits which is why we're not committing (pardon the pun) to maintaining them in the same way we do for the main branches (they're really meant to be there for emergencies) |
Pretty much, The same applies to the
It wouldn't be possible to do 11.3.x releases with the current release setup, besides, I'm not sure how that would work given that the other versions are ESM. We have to be careful with what we end up doing.
|
Why are we on v11 in the first place? it sounds like we just shouldn't be on that major version at all - the only solution I can think of that'd not violate the requirements you've just out is to ship a version with both CJS and ESM versions, which while possible cuts very strongly against the fact that the packages are ESM-only now... |
It's a backport release that ended up releasing on the main channel and not a special CJS tagged release |
but a backport of what, into what? do you mean like, we were backporting a fix intended for the v9 branch but "missed" and hit v11 by mistake? (what I'm ultimately trying to figure out is what we might need to backport to make it suitable to downgrade to v9) |
11.3.1 is effectively the same as 11.3.0 with the only difference being that 11.3.1 is CJS. Here is the diff, octokit/plugin-paginate-rest.js@v9.2.1...v11.3.1 It include new endpoints released up to that point, and associated type updates for new endopints, plus general type updates |
Folks, I appreciate the rigorous discussion here, but let’s cut through the noise. The key question is simple: How do we maintain backward compatibility for 20.x users while ensuring security vulnerabilities are addressed without unnecessary complexity? The reality is, software dependencies are like an over-leveraged startup: too much bloat, too many dependencies, and eventually, something crashes. The smartest approach is the simplest: a targeted patch release, ensuring we don’t introduce cascading issues by overhauling dependencies unnecessarily. A clean CJS release is the obvious move. Now, let me put this in terms even the most overworked engineer can appreciate: S = (D - C) / T Where:
The higher the S, the better the outcome. Right now, we’re letting C and T spiral out of control. Let’s optimize this equation and get back to shipping. As I always say, ‘Great software is built like great businesses: ruthlessly focused, highly adaptive, and designed to minimize technical debt.’ (Well, I don’t always say that, but I should.) If we follow this principle, we’ll land on the right solution without over-engineering ourselves into a corner. |
I understand you, but the reality is that we don't have all the power here, we have to rely on the release workflow in Github actions and the current infrastructure cannot handle targeting a specific version number. Only GitHub employees (of which I am not and GRath neither, we are only a community volunteers who have push access to these repos), have access to push npm packages directly without using the release workflow. I don't think any of us is trying to over-engineer anything at all, we are simply having a discussion so that everyone is on the same page and has the same understanding of the facts. |
Appreciate the clarification. If the real bottleneck is the release workflow and infrastructure constraints, then the path forward isn’t just about what should be done—it’s about what can be done within those limitations. So, let’s redefine the equation: P = (S * A) / G Where:
If G is high (which it clearly is), then we maximize P by keeping S high and ensuring A is rock solid. That means focusing on a solution that doesn’t require GitHub’s direct intervention or overhauling infrastructure. If a new CJS-compatible release isn’t feasible, what’s the next best viable move? This isn’t about what’s right in a vacuum—it’s about what’s possible given the constraints. Let’s work the problem from that angle, and we’ll find the optimal path forward. |
I believe I have figured out the issue on why the release was published as 11.x, and not as a seperately tagged release (11.3.1-cjs). When I added the We should be able to simply push that fix, update the affected dependencies, and we should be golden |
Great work team! |
We would need to make sure to update the GHSA advisory to mark that version as safe |
@wolfy1339 I can help take care of that |
It looks like the initial backport for |
Awesome; do we need to do a similar dance for |
Already done, https://github.com/octokit/plugin-rest-endpoint-methods.js/releases/tag/v13.3.2-cjs.1 I'll push another release to |
https://github.com/octokit/plugin-paginate-rest.js/releases/tag/v11.4.4-cjs.2 Alright, those versions are the fixed versions that should be used |
…, bump `devDependencies` (#487) This aims to resolve #486 `npm vulnerabilities with the 20.x branch Should resolve: GHSA-2p57-rm9w-gvfp GHSA-3xgq-45jj-v275 GHSA-67mh-4wv8-2f99 GHSA-78xj-cgh5-2h22 GHSA-952p-6rrq-rcjv GHSA-9qxr-qj54-h672 GHSA-9wv6-86v2-598j GHSA-c2qf-rxjj-qqgw GHSA-c76h-2ccp-4975 GHSA-c7qv-q95q-8v27 GHSA-f5x3-32g6-xq36 GHSA-grv7-fg5c-xmjg GHSA-h5c3-5r3r-rr8q GHSA-m4v8-wqvr-p9f7 GHSA-m6fv-jmcg-4jfg GHSA-pxg6-pf52-xh8x GHSA-qwcr-r2fm-qrc7 GHSA-rhx6-c78j-4q9w GHSA-rmvr-2pp2-xj38 GHSA-xx4v-prfh-6cgc ---- ### Before the change? <!-- Please describe the current behavior that you are modifying. --> > 31 vulnerabilities (3 low, 18 moderate, 10 high)  ### After the change? <!-- Please describe the behavior or changes that are being added by this PR. --> > 9 moderate severity vulnerabilities  **Important note**: the remaining reported 'moderate' vulnerabilities for `@octokit/request` and `@octokit/plugin-paginate-rest` for GHSA-h5c3-5r3r-rr8q and GHSA-rmvr-2pp2-xj38 are actually mitigated already; npm audit isn't taking the minor versions properly into account as: - @octokit/plugin-paginate-rest is patched in `9.2.2` (applied) - @octokit/request is patched in `8.4.1` (applied) This is a reporting issue: npm/cli#8125 ### Pull request checklist **Important note**: this PR reduces updates (reduces :() test coverage due to the same challenges discovered in #413 (comment) - [x] Tests for the changes have been added (for bug fixes / features) - [ ] Docs have been reviewed and added / updated if needed (for bug fixes / features) ### Does this introduce a breaking change? <!-- If this introduces a breaking change make sure to note it here any what the impact might be --> Please see our docs on [breaking changes](https://github.com/octokit/.github/blob/master/community/breaking_changes.md) to help! - [ ] Yes - [x] No ---- --------- Co-authored-by: wolfy1339 <[email protected]>
Fixed by #487 |
Looks like I spoke too soon, the release failed |
…, bump `devDependencies` (#487) This aims to resolve #486 `npm vulnerabilities with the 20.x branch Should resolve: GHSA-2p57-rm9w-gvfp GHSA-3xgq-45jj-v275 GHSA-67mh-4wv8-2f99 GHSA-78xj-cgh5-2h22 GHSA-952p-6rrq-rcjv GHSA-9qxr-qj54-h672 GHSA-9wv6-86v2-598j GHSA-c2qf-rxjj-qqgw GHSA-c76h-2ccp-4975 GHSA-c7qv-q95q-8v27 GHSA-f5x3-32g6-xq36 GHSA-grv7-fg5c-xmjg GHSA-h5c3-5r3r-rr8q GHSA-m4v8-wqvr-p9f7 GHSA-m6fv-jmcg-4jfg GHSA-pxg6-pf52-xh8x GHSA-qwcr-r2fm-qrc7 GHSA-rhx6-c78j-4q9w GHSA-rmvr-2pp2-xj38 GHSA-xx4v-prfh-6cgc ---- <!-- Please describe the current behavior that you are modifying. --> > 31 vulnerabilities (3 low, 18 moderate, 10 high)  <!-- Please describe the behavior or changes that are being added by this PR. --> > 9 moderate severity vulnerabilities  **Important note**: the remaining reported 'moderate' vulnerabilities for `@octokit/request` and `@octokit/plugin-paginate-rest` for GHSA-h5c3-5r3r-rr8q and GHSA-rmvr-2pp2-xj38 are actually mitigated already; npm audit isn't taking the minor versions properly into account as: - @octokit/plugin-paginate-rest is patched in `9.2.2` (applied) - @octokit/request is patched in `8.4.1` (applied) This is a reporting issue: npm/cli#8125 **Important note**: this PR reduces updates (reduces :() test coverage due to the same challenges discovered in #413 (comment) - [x] Tests for the changes have been added (for bug fixes / features) - [ ] Docs have been reviewed and added / updated if needed (for bug fixes / features) <!-- If this introduces a breaking change make sure to note it here any what the impact might be --> Please see our docs on [breaking changes](https://github.com/octokit/.github/blob/master/community/breaking_changes.md) to help! - [ ] Yes - [x] No ---- --------- Co-authored-by: wolfy1339 <[email protected]>
🎉 This issue has been resolved in version 20.1.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
The only vulnerability left is GHSA-rmvr-2pp2-xj38 which is a false positive, and a bug has been raised with NPM |
Describe the need
A variety of reasons might require a project to not (yet) leverage
main
(21.x) due to the full conversion to ESM.The
20.x
branch is purportedly vulnerable to:GHSA-2p57-rm9w-gvfp
GHSA-3xgq-45jj-v275
GHSA-67mh-4wv8-2f99
GHSA-78xj-cgh5-2h22
GHSA-952p-6rrq-rcjv
GHSA-9qxr-qj54-h672
GHSA-9wv6-86v2-598j
GHSA-c2qf-rxjj-qqgw
GHSA-c76h-2ccp-4975
GHSA-c7qv-q95q-8v27
GHSA-f5x3-32g6-xq36
GHSA-grv7-fg5c-xmjg
GHSA-h5c3-5r3r-rr8q
GHSA-m4v8-wqvr-p9f7
GHSA-m6fv-jmcg-4jfg
GHSA-pxg6-pf52-xh8x
GHSA-qwcr-r2fm-qrc7
GHSA-rhx6-c78j-4q9w
GHSA-rmvr-2pp2-xj38
GHSA-xx4v-prfh-6cgc
SDK Version
No response
API Version
No response
Relevant log output
Code of Conduct
The text was updated successfully, but these errors were encountered: