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

chore: Upgrade octokit/rest.js for CVE patch #1477

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

chris-griffin
Copy link

Just some cleanup for a transitive CVE: GHSA-h5c3-5r3r-rr8q

None of these breaking changes should impact v12 of danger-js as it requires node >= 18.

- v19 dropped support for node 10/12 (https://github.com/octokit/rest.js/releases/tag/v19.0.0)
- v20 dropped support for node 14/16, removed preview support for the REST API, and removed the agent option (https://github.com/octokit/rest.js/releases/tag/v20.0.0)
- v21 updated the package to ESM (https://github.com/octokit/rest.js/releases/tag/v21.0.0)

None of these breaking changes should impact v12 of danger-js as it requires node >= 18.
@mrginglymus
Copy link

danger-js also pulls in memfs-or-file-map-to-github-branch, which still relies on the old version of @octokit/rest:

https://github.com/orta/memfs-or-file-map-to-github-branch/blob/c293897e6b7cf80b23f16956ee2dc236953323e7/package.json#L19

This package will need updating in order to prevent danger-js pulling in the CVE.

fbartho added a commit to fbartho/memfs-or-file-map-to-github-branch that referenced this pull request Mar 11, 2025
Reference:
- https://github.com/octokit/rest.js/releases/tag/v21.1.1
- danger/danger-js#1477
- danger/danger-js#1479

I updated package.json dependencies, and reinstalled modules, deleted the `build` directory and then ran `yarn build` and there were no build errors, and no changes in the `build/` directory.

For good measure, I also opened `index.ts`, and VSCode didn't give me any warnings or TS Errors.
@fbartho
Copy link
Member

fbartho commented Mar 11, 2025

@mrginglymus @chris-griffin: I've provided a PR for memfs-or-file-map-to-github-branch here orta/memfs-or-file-map-to-github-branch#6

Now I think we're stuck until @orta gets a chance to look at the upstream library.

@fbartho
Copy link
Member

fbartho commented Mar 12, 2025

@chris-griffin I went to test this PR locally (and update devDependency release-it to a newer version, since that also depended on ancient @octokit/rest), and I ran into two unexpected issues:

  1. yarn.lock wasn't updated in this PR, did you use npm or something else?
  2. The above suggested you hadn't hooked in to run the pre-push scripts.

When I ran yarn build I got the following errors:

$ yarn build
…
source/platforms/GitHub.ts:6:39 - error TS1479: The current file is a CommonJS module whose imports will produce 'require' calls; however, the referenced file is an ECMAScript module and cannot be imported with 'require'. Consider writing a dynamic 'import("@octokit/rest")' call instead.
  To convert this file to an ECMAScript module, change its file extension to '.mts', or add the field `"type": "module"` to '~/Code/danger-js/package.json'.

6 import { Octokit as NodeGitHub } from "@octokit/rest"
                                        ~~~~~~~~~~~~~~~
# Many of the same type of error repeated for different files.

(These issues can also be viewed in the CI build: https://github.com/danger/danger-js/actions/runs/13427029750/job/37511812934?pr=1477)


Looks like we need to do some work to import from ESM dependencies. sigh
Any support would be appreciated!

@fbartho
Copy link
Member

fbartho commented Mar 12, 2025

I made an alternative PR here: #1481 (including @chris-griffin's initial changes)

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.

3 participants