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

Audit vulnerabilities in [email protected] ([email protected], [email protected]) #314

Open
globalexport opened this issue Oct 21, 2024 · 7 comments

Comments

@globalexport
Copy link

❯ pnpm audit
┌─────────────────────┬────────────────────────────────────────────────────────┐
│ low                 │ cookie accepts cookie name, path, and domain with out  │
│                     │ of bounds characters                                   │
├─────────────────────┼────────────────────────────────────────────────────────┤
│ Package             │ cookie                                                 │
├─────────────────────┼────────────────────────────────────────────────────────┤
│ Vulnerable versions │ <0.7.0                                                 │
├─────────────────────┼────────────────────────────────────────────────────────┤
│ Patched versions    │ >=0.7.0                                                │
├─────────────────────┼────────────────────────────────────────────────────────┤
│ Paths               │ . > [email protected] > [email protected]                        │
├─────────────────────┼────────────────────────────────────────────────────────┤
│ More info           │ https://github.com/advisories/GHSA-pxg6-pf52-xh8x      │
└─────────────────────┴────────────────────────────────────────────────────────┘
┌─────────────────────┬────────────────────────────────────────────────────────┐
│ low                 │ Valid ECDSA signatures erroneously rejected in         │
│                     │ Elliptic                                               │
├─────────────────────┼────────────────────────────────────────────────────────┤
│ Package             │ elliptic                                               │
├─────────────────────┼────────────────────────────────────────────────────────┤
│ Vulnerable versions │ <=6.5.7                                                │
├─────────────────────┼────────────────────────────────────────────────────────┤
│ Patched versions    │ <0.0.0                                                 │
├─────────────────────┼────────────────────────────────────────────────────────┤
│ Paths               │ . > [email protected] > [email protected] > [email protected]   │
├─────────────────────┼────────────────────────────────────────────────────────┤
│ More info           │ https://github.com/advisories/GHSA-fc9h-whq2-v747      │
└─────────────────────┴────────────────────────────────────────────────────────┘
2 vulnerabilities found
Severity: 2 low
@simov
Copy link
Owner

simov commented Oct 21, 2024

Thanks for the report, I just checked that myself.

For the cookie package I do see that there is a new version not covered by the caret range on 0.x release obviously, but for the jwk-to-pem package I'm actually surprised that it is still being listed as vulnerable since that issue got fixed in elliptic v6.5.7 indutny/elliptic#317 and then updated in jwk-to-pem v2.0.6 Brightspace/node-jwk-to-pem#189

@globalexport
Copy link
Author

Hi @simov!

I am confused, too. There was another approval 12 hours ago. Looks like it did not make it into the release? indutny/elliptic#317 (review)

@simov
Copy link
Owner

simov commented Oct 25, 2024

Oh there is another one indutny/elliptic#322, now it makes sense, though it's not patched yet.

@KidkArolis
Copy link
Contributor

@simov I can open a PR to upgrade the cookie pkg but it's blocked on the @curveball/session switching to ESM. Do you have a preferred approach?

  • Option 1: (my recommendation) - switch grant to be ESM package, supporting Node 14 and up when in ESM mode, or Node 23 and up seamlessly.
  • Option 2: switch grant to ESM, but add a build step to compile down to CJS to keep ~Node>14 support
  • Option 3: drop support for @curveball/session which is now an ESM-only module (at least for now)
  • Option 4: continue lazily requiring @curveball/session like grant does now, but this code path will only work in Node >23

@simov
Copy link
Owner

simov commented Feb 4, 2025

Hi @KidkArolis, thanks for the feedback. I just realized that I forgot to push back then 20c5a71

Maybe I'm missing something, but isn't [email protected] fixed?

This is what I am seeing locally with the above package.json file:

npm audit
# npm audit report                                                                                                                                                                                                                        

cookie  <0.7.0
cookie accepts cookie name, path, and domain with out of bounds characters - https://github.com/advisories/GHSA-pxg6-pf52-xh8x
fix available via `npm audit fix --force`
Will install @curveball/[email protected], which is a breaking change
node_modules/@curveball/session/node_modules/cookie
node_modules/grant/node_modules/cookie
  @curveball/session  <=1.0.0
  Depends on vulnerable versions of cookie
  node_modules/@curveball/session
  grant  >=5.3.0
  Depends on vulnerable versions of cookie
  node_modules/grant

So it seems like I only need to publish a new release as curveball is only a devDependency:

npm audit --production
npm warn config production Use `--omit=dev` instead.
found 0 vulnerabilities  

@KidkArolis
Copy link
Contributor

You're right, [email protected] is fixed. But if you update curveball (which is now ESM) the project won't work / tests won't run.
But you're right about the dev dep situation - updating cookie for grant itself will be huge help already!

@simov
Copy link
Owner

simov commented Feb 4, 2025

Thanks again, @KidkArolis, just published version 5.4.24 with the updated cookie dependency.

To your point though, Grant will definitely need some updates in the near feature, and I'm actually adding your proposals to my notes, but it's just that releasing the patch seemed easier for now.

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

No branches or pull requests

3 participants