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

[MAINT]: 20.x npm audit vulnerabilities #486

Closed
1 task done
benpbolton opened this issue Feb 21, 2025 · 32 comments · Fixed by #487
Closed
1 task done

[MAINT]: 20.x npm audit vulnerabilities #486

benpbolton opened this issue Feb 21, 2025 · 32 comments · Fixed by #487
Labels
released on @20.x Status: Triage This is being looked at and prioritized Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR

Comments

@benpbolton
Copy link

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

Image

SDK Version

No response

API Version

No response

Relevant log output

npm audit | egrep -oe "https://github.com/advisories/.*" | sort -u
https://github.com/advisories/GHSA-2p57-rm9w-gvfp
https://github.com/advisories/GHSA-3xgq-45jj-v275
https://github.com/advisories/GHSA-67mh-4wv8-2f99
https://github.com/advisories/GHSA-78xj-cgh5-2h22
https://github.com/advisories/GHSA-952p-6rrq-rcjv
https://github.com/advisories/GHSA-9qxr-qj54-h672
https://github.com/advisories/GHSA-9wv6-86v2-598j
https://github.com/advisories/GHSA-c2qf-rxjj-qqgw
https://github.com/advisories/GHSA-c76h-2ccp-4975
https://github.com/advisories/GHSA-c7qv-q95q-8v27
https://github.com/advisories/GHSA-f5x3-32g6-xq36
https://github.com/advisories/GHSA-grv7-fg5c-xmjg
https://github.com/advisories/GHSA-h5c3-5r3r-rr8q
https://github.com/advisories/GHSA-m4v8-wqvr-p9f7
https://github.com/advisories/GHSA-m6fv-jmcg-4jfg
https://github.com/advisories/GHSA-pxg6-pf52-xh8x
https://github.com/advisories/GHSA-qwcr-r2fm-qrc7
https://github.com/advisories/GHSA-rhx6-c78j-4q9w
https://github.com/advisories/GHSA-rmvr-2pp2-xj38
https://github.com/advisories/GHSA-xx4v-prfh-6cgc

> 31 vulnerabilities (3 low, 18 moderate, 10 high)

Code of Conduct

  • I agree to follow this project's Code of Conduct
@benpbolton benpbolton added Status: Triage This is being looked at and prioritized Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR labels Feb 21, 2025
Copy link
Contributor

👋 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 Status: Up for grabs. You & others like you are the reason all of this works! So thank you & happy coding! 🚀

@wolfy1339
Copy link
Member

In reality though, how many of those advisories are actually production dependencies?
And how many of them relate to the docs website?

We don't pin the version used, so users can install the latest supported version

@wolfy1339
Copy link
Member

wolfy1339 commented Feb 21, 2025

Besides, require(esm) is a thing now in 20.x and 22.x (Both LTS releases)

@benpbolton
Copy link
Author

benpbolton commented Feb 21, 2025

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.

@G-Rath
Copy link
Member

G-Rath commented Feb 26, 2025

@wolfy1339 so doing an install of @octokit/rest@20, it looks like there is one vulnerability:

workspace/projects-scrap/octokit-check is 📦 v1.0.0 via  v20.11.0 took 2s
❯ osv-detector .
Loaded the following OSV databases:
  npm (23319 vulnerabilities, including withdrawn - last updated Wed, 26 Feb 2025 18:24:24 GMT)

package-lock.json: found 17 packages
  Using db npm (23319 vulnerabilities, including withdrawn - last updated Wed, 26 Feb 2025 18:24:24 GMT)

  @octokit/[email protected] is affected by the following vulnerabilities:
    GHSA-h5c3-5r3r-rr8q: @octokit/plugin-paginate-rest has a Regular Expression in iterator Leads to ReDoS Vulnerability Due to Catastrophic Backtracking (https://github.com/advisories/GHSA-h5c3-5r3r-rr8q)

  1 known vulnerability found in package-lock.json

That looks to be because we pinning @octokit/plugin-paginate-rest to an exact version:

  "dependencies": {
    "@octokit/core": "^5.0.2",
    "@octokit/plugin-paginate-rest": "11.3.1",
    "@octokit/plugin-request-log": "^4.0.0",
    "@octokit/plugin-rest-endpoint-methods": "13.2.2"
  },

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 @octokit/plugin-paginate-rest specifically.

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 require(esm) making a lot of things a non-issue, I'm not sure if it (/ the surrounding tooling like TypeScript) support it well enough to rely on it just yet)

@wolfy1339
Copy link
Member

wolfy1339 commented Feb 26, 2025

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 require(esm) support in --module nodenext, and it will get backported to --module node20 in Typescript 5.9

@G-Rath
Copy link
Member

G-Rath commented Feb 26, 2025

Cool - are you ok if I try looking into that further?

Typescript has require(esm) support in --module nodenext, and it will get backported to --module node20 in Typescript 5.9

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 require(esm) and pleasantly surprised by how fast it seems to be getting landed, but I think we've still got about another year before it can be relied on.

As an example, did you know about the special module.exports named export? Upstream packages need to ship that to make using require(esm) invisible for downstream consumers, and that requires es2022 because it's an arbitrary module namespace identifier...

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.

@G-Rath
Copy link
Member

G-Rath commented Feb 26, 2025

(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 😄)

@wolfy1339
Copy link
Member

I was not aware of the module.exports identifier.

Feel free to look into that

@benpbolton
Copy link
Author

(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 😄)

@G-Rath I'd appreciate your involvement!

I put together #487 (the approach for @octokit/plugin-paginate-rest there was to use the patched 9.2.2 version, not 11.4.1 ... as both appear to be patched: GHSA-h5c3-5r3r-rr8q)

The published npm package has deps:

  "dependencies": {
    "@octokit/core": "^5.0.2",
    "@octokit/plugin-paginate-rest": "11.3.1",
    "@octokit/plugin-request-log": "^4.0.0",
    "@octokit/plugin-rest-endpoint-methods": "13.2.2"
  },

differing from github's 20.x branch in that major version bump for @octokit/plugin-paginate-rest and in @octokit/plugin-rest-endpoint-methods ... I am awaiting more instruction on how to proceed over there, but if you've got a more atomic approach, as you're more aware/trusted in the ecosystem, happy to defer the solution to this issue to your PR! My primary goal is to ensure the 20.x branch remains viable for development and testing for users who cannot yet migrate to ESM.

@wolfy1339
Copy link
Member

Let me make this more clear, I don't think it was fully understood.

For @octokit/plugin-paginate-rest:
11.3.1 is CJS while 11.4.1 is ESM
9.x is CJS but without the backported changes

For @octokit/plugin-rest-endpoint-methods:
13.2.2 is CJS while 13.3.1 is ESM
10.x is CJS but without the backported changes

These 2 packages need to remain in sync with the same changes.

@G-Rath
Copy link
Member

G-Rath commented Feb 26, 2025

@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)

@wolfy1339
Copy link
Member

@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")

Pretty much,
For @ocotokit/plugin-paginate-rest, 11.3.1 is effectively the same as 11.3.0 with the only caveat that 11.3.1 is CJS while all other 11.x versions are ESM.

The same applies to the @octokit/plugin-rest-endpoint-methods versions

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.

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.

  • Make sure that the new version won't affect ESM users who have their versions set to the ~ operator
  • Make sure that the version is a proper CJS release
  • Make sure that both @octokit/plugin-paginate-rest and @octokit/plugin-rest-endpoint-methods receive the same update

@G-Rath
Copy link
Member

G-Rath commented Feb 26, 2025

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...

@wolfy1339
Copy link
Member

It's a backport release that ended up releasing on the main channel and not a special CJS tagged release
It really should have been 11.3.1-cjs or something similar.

@G-Rath
Copy link
Member

G-Rath commented Feb 26, 2025

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)

@wolfy1339
Copy link
Member

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

@jaredcobb
Copy link

jaredcobb commented Feb 26, 2025

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:

S is Stability
D is the number of Dependencies
C is the amount of Compatibility debt
T is the amount of Time wasted debating the perfect solution

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.

@wolfy1339
Copy link
Member

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.

@jaredcobb
Copy link

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:

P is Progress
S is the Simplicity of the solution
A is the Alignment of contributors and stakeholders
G is the amount of Gatekeeping we have to navigate

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.

@wolfy1339
Copy link
Member

wolfy1339 commented Feb 26, 2025

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 cjs tagged release, I forgot one crucial option, setting prerelease to true.

https://github.com/octokit/plugin-paginate-rest.js/blob/cc04a2a6d8c37acce7878371a1bd915db805e478/package.json#L82-L85

We should be able to simply push that fix, update the affected dependencies, and we should be golden

@jaredcobb
Copy link

Great work team!

@wolfy1339
Copy link
Member

We would need to make sure to update the GHSA advisory to mark that version as safe

@G-Rath
Copy link
Member

G-Rath commented Feb 26, 2025

@wolfy1339 I can help take care of that

@wolfy1339
Copy link
Member

It looks like the initial backport for @octokit/plugin-paginate-rest went well
https://github.com/octokit/plugin-paginate-rest.js/actions/runs/13553882633/job/37883694657

@benpbolton
Copy link
Author

Awesome; do we need to do a similar dance for @octokit/plugin-rest-endpoint-methods

@wolfy1339
Copy link
Member

Already done, https://github.com/octokit/plugin-rest-endpoint-methods.js/releases/tag/v13.3.2-cjs.1

I'll push another release to @octokit/plugin-paginate-rest to use that version

@wolfy1339
Copy link
Member

https://github.com/octokit/plugin-paginate-rest.js/releases/tag/v11.4.4-cjs.2
https://github.com/octokit/plugin-rest-endpoint-methods.js/releases/tag/v13.3.2-cjs.1

Alright, those versions are the fixed versions that should be used

wolfy1339 added a commit that referenced this issue Feb 26, 2025
…, 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)

![CleanShot 2025-02-21 at 12 06
39](https://github.com/user-attachments/assets/02abda17-8aee-46e3-b808-764672a18475)


### After the change?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

> 9 moderate severity vulnerabilities

![CleanShot 2025-02-21 at 12 12
49](https://github.com/user-attachments/assets/10d593d8-9de5-478e-8cde-b5fb81762706)

**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]>
@wolfy1339
Copy link
Member

Fixed by #487

@github-project-automation github-project-automation bot moved this from 🆕 Triage to ✅ Done in 🧰 Octokit Active Feb 26, 2025
@wolfy1339
Copy link
Member

Looks like I spoke too soon, the release failed
https://github.com/octokit/rest.js/actions/runs/13554345566/job/37885228642

wolfy1339 added a commit that referenced this issue Feb 26, 2025
…, 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)

![CleanShot 2025-02-21 at 12 06
39](https://github.com/user-attachments/assets/02abda17-8aee-46e3-b808-764672a18475)

<!-- Please describe the behavior or changes that are being added by
this PR. -->

> 9 moderate severity vulnerabilities

![CleanShot 2025-02-21 at 12 12
49](https://github.com/user-attachments/assets/10d593d8-9de5-478e-8cde-b5fb81762706)

**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]>
Copy link
Contributor

🎉 This issue has been resolved in version 20.1.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@wolfy1339
Copy link
Member

The only vulnerability left is GHSA-rmvr-2pp2-xj38 which is a false positive, and a bug has been raised with NPM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released on @20.x Status: Triage This is being looked at and prioritized Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR
Projects
None yet
4 participants