-
-
Notifications
You must be signed in to change notification settings - Fork 373
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
base: main
Are you sure you want to change the base?
Conversation
- 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.
danger-js also pulls in This package will need updating in order to prevent danger-js pulling in the CVE. |
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.
@mrginglymus @chris-griffin: I've provided a PR for Now I think we're stuck until @orta gets a chance to look at the upstream library. |
@chris-griffin I went to test this PR locally (and update
When I ran $ 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 |
I made an alternative PR here: #1481 (including @chris-griffin's initial changes) |
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.