slackapi / bolt-js

A framework to build Slack apps using JavaScript
https://tools.slack.dev/bolt-js/
MIT License
2.75k stars 393 forks source link

package vulnerabilities with dependency "path-to-regexp" and "send" #2242

Closed nirh1989 closed 1 month ago

nirh1989 commented 1 month ago

Hi Team,

I am using "@slack/bolt": "^3.21.2" and getting vulnerabilities notification.

Can you please fix the following vulnerabilities?

# npm audit report

path-to-regexp  0.2.0 - 7.2.0
Severity: high
path-to-regexp outputs backtracking regular expressions - https://github.com/advisories/GHSA-9wv6-86v2-598j
fix available via `npm audit fix --force`
Will install @slack/bolt@3.12.2, which is a breaking change
node_modules/path-to-regexp
  @slack/bolt  *
  Depends on vulnerable versions of express
  Depends on vulnerable versions of path-to-regexp
  node_modules/@slack/bolt

send  <0.19.0
Severity: moderate
send vulnerable to template injection that can lead to XSS - https://github.com/advisories/GHSA-m6fv-jmcg-4jfg
fix available via `npm audit fix --force`
Will install @slack/bolt@3.12.2, which is a breaking change
node_modules/serve-static/node_modules/send
  serve-static  <=1.16.0
  Depends on vulnerable versions of send
  node_modules/serve-static
    express  4.0.0-rc1 - 5.0.0-beta.3
    Depends on vulnerable versions of serve-static
    node_modules/express

5 vulnerabilities (3 moderate, 2 high)

To address all issues (including breaking changes), run:
  npm audit fix --force
npm ls send
myProject
└─┬ @slack/bolt@3.21.2
  └─┬ express@4.20.0
    ├── send@0.19.0
    └─┬ serve-static@1.16.0
      └── send@0.18.0
npm ls path-to-regexp
myProject
└─┬ @slack/bolt@3.21.2
  ├─┬ express@4.20.0
  │ └── path-to-regexp@0.1.10
  └── path-to-regexp@6.2.2
seratch commented 1 month ago

Hi @nirh1989, thank you so much for flagging this. The "express" dependency and its sub-dependencies are only used by ExpressReceiver, so if you don't use ExpressReceiver (note that the default one is HTTPReceiver, which uses only Node standard modules), there is no actual security risk with this alert.

With that being said, this project will take necessary actions to eliminate the high-severity audit error when feasible. At this moment, there is nothing we can do (if I understand correctly).

nirh1989 commented 1 month ago

Hi @seratch

Thank you for your comment, these are all the dependencies i have in my project

"dependencies": {
    "@slack/bolt": "^3.21.2",
    "pino": "^9.4.0",
    "pino-abstract-transport": "^2.0.0"
  },
  "devDependencies": {
    "@types/node": "^20.3.2",
    "pino-pretty": "^11.2.2",
    "typescript": "^5.1.3"
  }

and this is what i get what i run npm audit

# npm audit report

path-to-regexp  0.2.0 - 7.2.0
Severity: high
path-to-regexp outputs backtracking regular expressions - https://github.com/advisories/GHSA-9wv6-86v2-598j
fix available via `npm audit fix --force`
Will install @slack/bolt@3.12.2, which is a breaking change
node_modules/path-to-regexp
  @slack/bolt  *
  Depends on vulnerable versions of express
  Depends on vulnerable versions of path-to-regexp
  node_modules/@slack/bolt

send  <0.19.0
Severity: moderate
send vulnerable to template injection that can lead to XSS - https://github.com/advisories/GHSA-m6fv-jmcg-4jfg
fix available via `npm audit fix --force`
Will install @slack/bolt@3.12.2, which is a breaking change
node_modules/serve-static/node_modules/send
  serve-static  <=1.16.0
  Depends on vulnerable versions of send
  node_modules/serve-static
    express  4.0.0-rc1 - 5.0.0-beta.3
    Depends on vulnerable versions of serve-static
    node_modules/express

5 vulnerabilities (3 moderate, 2 high)

To address all issues (including breaking changes), run:
  npm audit fix --force

everything in the message seems to come from @slack/bolt

nirh1989 commented 1 month ago

Hi @seratch

I created the same in Stackblitz: https://stackblitz.com/edit/stackblitz-starters-w8jubh?description=Starter%20project%20for%20Node.js,%20a%20JavaScript%20runtime%20built%20on%20Chrome%27s%20V8%20JavaScript%20engine&file=package.json&title=node.new%20Starter

you go in, run npm install and then npm audit and see the same

seratch commented 1 month ago

I do understand this is happening. Let me clarify a bit more.

The actual dependency tree is @slack/bolt -> express -> path-to-regexp. This project started only with ExpressReceiver, and then we switched the default receiver to HTTPReceiver later. So, most users today no longer use the express dependency at all. However, we still keep express as a required dependency for smoother major release migration and backward-compatibility. In the future, we may make it optional, but this is how it is right now. Also, apart from ExpressReceiver, @slack/bolt does not directly use any functionalities of path-to-regexp.

Regarding the short-term actions this project can take, the express dependency sets the version range of path-to-regexp and blocks major upgrades. Therefore, until these third-party libraries release new versions eliminating the possibility of the vulnerability, we have to wait. Hope this clarifies.

nirh1989 commented 1 month ago

Hi @seratch,

I understand what you are saying, so i dig further. I checked your package.json: https://github.com/slackapi/bolt-js/blob/main/package.json you are using: "path-to-regexp": "^6.2.1" that's where the vulnerability comes from. Can you update this version to "path-to-regexp": "^8.0.0"?

This will solve it

seratch commented 1 month ago

Actually I already tried it but it didn't work out. Even with it, any version of express 4.x still uses 6.x. It seems the only solution at this moment seems to be upgrading express to ^5.0. If it's inevitable to upgrade express, we will do so.

By the way, let me correct this:

@slack/bolt does not directly use any functionalities of path-to-regexp.

Since https://github.com/slackapi/bolt-js/pull/1785 was merged, SocketModeReceiver and a few others use match fucntion provided by the library. So, the upgrade must be done not only for Express users but also for Socket Mode users. But we're still blocked from a quick ugprade at this moment. My business hours will end soon, but our team will continue monitoring the progress on 3rd party side and decide what actions to take.

seratch commented 1 month ago

Update: I created a draft pull request (https://github.com/slackapi/bolt-js/pull/2244) to upgrade Express to v5, which was released just a day ago. This resolves the issue, but I'd like to avoid doing this as much as possible because it requires existing ExpressReceiver users to immediately migrate to Express v5. If we have to do this, the release version must be either minor or major.

nirh1989 commented 1 month ago

Thank you @seratch

When i should expect this to be merged?

seratch commented 1 month ago

I cannot tell the exact timing but if it'll be merged, it won't take long (meaning it won't be next week); thank you so much for being patient with this

nirh1989 commented 1 month ago

thank you very much @seratch for being responsive and handle this issue :)

filmaj commented 1 month ago

We are still investigating the scope of addressing this. It may require a major new version. Your patience is appreciated.

filmaj commented 1 month ago

To resolve the issue, we would require upgrading express from v4 to v5, as @seratch has explored in #2244. Because bolt-js supports, as a first-class option, different kinds of 'receivers' (that is, different mechanisms of receiving events from Slack, described in more detail here), and one of the supported receivers in this project is the ExpressReceiver, any breaking changes introduced by upgrading express implicitly forwards these breaking changes to consumers of bolt-js. At least, to consumers of bolt-js that are using the ExpressReceiver.

Given that, I think the only way to address this security vulnerability is to release a new major version of bolt that moves from express v4 to v5.

seratch commented 1 month ago

@filmaj Thank you for looking into this. Regarding the plans for bolt v4 including express v5 and dropping EOLed runtimes, it looks great to me too.

At the same time, I came to think that we should quickly release 3.21.3 3.21.4 with top-level path-to-regexp upgrade to 8.1 today. Still, express depends on an older version. Also, its send dependency's moderate vul won't be resolved by that. Due to this, it may not resolve the alert for all customers. But there is no risk for us to do so, plus now our code directly use the code for Socket Mode etc. So, having a high-severity vul package version in our package.json should be avoided in any case. What do you think?

filmaj commented 1 month ago

Sounds good. I'll release a patch with upgraded path-to-regexp shortly, and in the mean time plan for a short-term bolt v4 that includes upgrading to express v5 (as well as a bolt v5 plan for the longer term).

I will update this issue once 3.21.4 is live.

filmaj commented 1 month ago

This may not be so easy and still support node 12... because the latest path-to-regexp uses language features that are not supported in node 12 (optional chaining, the ?. operator), it seems the mocha tests are having trouble running on node 12. See https://github.com/slackapi/bolt-js/pull/2251

seratch commented 1 month ago

@filmaj Hmm, if there is no workaround, I think it's okay to drop Node 12 tests this time. It's not great in principle, but the version was EOLed 2.5 years ago. I believe no one will complain about it.

filmaj commented 1 month ago

OK, https://github.com/slackapi/bolt-js/pull/2251 is ready for review.

seratch commented 1 month ago

Reopened this because the complete resolution for both path-to-regexp and send will be done when we release a new major version w/ Express v5 requirement. No blocker for 3.21.4 release.

filmaj commented 1 month ago

bolt v3.21.4 is live on npm and at least upgrades path-to-regexp that Bolt consumes, which should address the vulnerability for Bolt consumers building apps using either the SocketModeReceiver or the HTTPReceiver.

ExpressReceiver requires an upgrade to express to v5 from v4, which is a breaking change, which will also require a bolt new major version. I think I will work on that in the near term.

MeiravShimelman commented 1 month ago

Hi, what about @nestjs/platform-express ? seems like its also breaking

seratch commented 1 month ago

It seems serve-static's latest version no longer has the vul issue (meaning no send@0.18 dependency via the package):

$ npm ls send
@slack/bolt@3.21.4 /path-to-project/bolt-js
└─┬ express@4.21.0
  ├── send@0.19.0
  └─┬ serve-static@1.16.2
    └── send@0.19.0 deduped
seratch commented 1 month ago

@MeiravShimelman Sorry, I don't understand your question yet, but express's latest version no longer has the "send" package's vulnerability issue. Also, in general, bolt-js does not work properly within a Nest.js app due to bolt-js's restriction. If you have follow-up questions, please feel free to create a new issue for them.

seratch commented 1 month ago

bolt-js 3.x does not have this issue anymore, so let me close this issue now

lessquo commented 1 month ago

Hello, it appears that @slack/bolt@3.21.4 still requires express@4.16.4. Upgrading to express@4.21.0 should fix the issue.

seratch commented 1 month ago

We don't lock express version. 4.16.4 is the oldest version we support: https://github.com/slackapi/bolt-js/blob/%40slack/bolt%403.21.4/package.json#L53

Indeed, future releases in bolt-js 3.x should upgrade the oldest version, but for now, you can quickly upgrade express by npm i express@latest.

lessquo commented 1 month ago

I see, thanks for the kind response.