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]: Bump package to fix CVE-2022-23541 #430

Closed
1 task done
vavsab opened this issue Dec 28, 2022 · 6 comments · Fixed by #433
Closed
1 task done

[MAINT]: Bump package to fix CVE-2022-23541 #430

vavsab opened this issue Dec 28, 2022 · 6 comments · Fixed by #433
Labels
released Status: Blocked Blocked by GitHub's API or other external factors Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR

Comments

@vavsab
Copy link

vavsab commented Dec 28, 2022

Describe the need

Please upgrade universal-github-app-jwt package.
They have recently fixed CVE related to jsonwebtoken
image

Link to CVE

SDK Version

No response

API Version

No response

Relevant log output

No response

Code of Conduct

  • I agree to follow this project's Code of Conduct
@vavsab vavsab added Status: Triage This is being looked at and prioritized Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR labels Dec 28, 2022
@vavsab vavsab changed the title [MAINT]: [MAINT]: Bump package to fix CVE-2022-23541 Dec 28, 2022
@wolfy1339
Copy link
Member

This isn't possible at this time.

The universal-github-app-jwt package has switched to ESM, and thus is incompatible with this package.

I don't think the CVE even affects us at all

@wolfy1339 wolfy1339 added Status: Blocked Blocked by GitHub's API or other external factors Priority: Low and removed Status: Triage This is being looked at and prioritized labels Dec 28, 2022
@nickfloyd nickfloyd moved this from 🆕 Triage to 🔖 Ready in 🧰 Octokit Active Jan 3, 2023
@nickfloyd nickfloyd moved this from 🔖 Ready to 🔥 Backlog in 🧰 Octokit Active Jan 3, 2023
@ebickle
Copy link

ebickle commented Jan 4, 2023

Are you sure ESM is incompatible with this package? The package.json for this library indicates it has a minimum requirement of Node.js 14.x, which has support for ESM. Although not directly supported, the browser .js file exported by cdn.skypack.dev is already ESM.

The company I work for has a 100's of millions of dependencies to manage, so all CVEs are relevant. There are just too many to look into whether each and every package is vulnerable for each and every version based on the package maintainer's statement. There are just too many 😁 - so a fix for this would be great.

@wolfy1339
Copy link
Member

Are you sure ESM is incompatible with this package?

This package is currently built as a CommonJS package for NodeJS, and including ESM dependencies would not go well. We have done so by accident on other Octokit packages

The package.json for this library indicates it has a minimum requirement of Node.js 14.x, which has support for ESM

It's not a question of Node versions, it's just that this package isn't ESM yet.
We are rewriting the code into ESM native over at https://github.com/octokit/octokit-next.js

Although not directly supported, the browser .js file exported by cdn.skypack.dev is already ESM.

The only ESM right now, is for browsers.

The company I work for has a 100's of millions of dependencies to manage, so all CVEs are relevant. There are just too many to look into whether each and every package is vulnerable for each and every version based on the package maintainer's statement. There are just too many 😁 - so a fix for this would be great.

I understand that fixing CVE's are important. Whenever a backport of the fix will be made, I will upgrade to it as soon as possible.

@wolfy1339 wolfy1339 moved this from 🔥 Backlog to 🛑 Blocked/Awaiting Response in 🧰 Octokit Active Jan 4, 2023
@kfcampbell
Copy link
Member

@ebickle for a concrete take, if this is possible, I'm not sure how to fix it. I'm able to update the version in package.json to v2.x.x:

diff --git a/package.json b/package.json
index 7491b91..0a8ecd6 100644
--- a/package.json
+++ b/package.json
@@ -31,7 +31,7 @@
     "@types/lru-cache": "^5.1.0",
     "deprecation": "^2.3.1",
     "lru-cache": "^6.0.0",
-    "universal-github-app-jwt": "^1.0.1",
+    "universal-github-app-jwt": "^2.0.1",
     "universal-user-agent": "^6.0.0"
   },

and after running npm i, see corresponding changes in the package-lock.json:

diff --git a/package-lock.json b/package-lock.json
index 85bdd62..2bdfb53 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -17,7 +17,7 @@
         "@types/lru-cache": "^5.1.0",
         "deprecation": "^2.3.1",
         "lru-cache": "^6.0.0",
-        "universal-github-app-jwt": "^1.0.1",
+        "universal-github-app-jwt": "^2.0.1",
         "universal-user-agent": "^6.0.0"
       },

However, at this point npm run build fails with:

 sh$ npm run build

> @octokit/auth-app@0.0.0-development build
> pika-pack build

@pika/pack build v0.3.7
[1/6] Validating source...
[2/6] Preparing pipeline...
      ❇️  pkg/
      » tsconfig.json [compilerOptions.target] should be "ES2020", but found "ES2018". You may encounter problems building.
[3/6] Running @pika/plugin-ts-standard-pkg...
src/get-app-authentication.ts(1,10): error TS2614: Module '"universal-github-app-jwt"' has no exported member 'githubAppJwt'. Did you mean to use 'import githubAppJwt from "universal-github-app-jwt"' instead?

Upon making the suggested change, npm run build succeeds but npm run test fails with three versions of this error:

    Details:

    /home/kfcampbell/github/dev/auth-app.js/node_modules/universal-github-app-jwt/index.js:4
    import { getToken } from "#get-token";
    ^^^^^^

    SyntaxError: Cannot use import statement outside a module

    > 1 | import githubAppJwt from "universal-github-app-jwt";
        | ^
      2 |
      3 | import { AppAuthentication, State } from "./types";
      4 |

      at Runtime.createScriptFromCode (node_modules/jest-runtime/build/index.js:1449:14)
      at Object.<anonymous> (src/get-app-authentication.ts:1:1)
      at Object.<anonymous> (src/auth.ts:18:1)
      at Object.<anonymous> (src/index.ts:5:1)
      at Object.<anonymous> (test/index.test.ts:6:1)

There's some discussion about a backport to v1 to fix the CVE as well over at universal-github-app-jwt#66.

@oscard0m oscard0m moved this from 🛑 Blocked/Awaiting Response to 👀 In review in 🧰 Octokit Active Jan 6, 2023
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in 🧰 Octokit Active Jan 6, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jan 6, 2023

🎉 This issue has been resolved in version 4.0.8 🎉

The release is available on:

Your semantic-release bot 📦🚀

@ebickle
Copy link

ebickle commented Jan 6, 2023

I really appreciate all the help solving this, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Status: Blocked Blocked by GitHub's API or other external factors Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants