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

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

Closed
nirh1989 opened this issue Sep 11, 2024 · 26 comments · Fixed by #2251 or #2254
Closed

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

nirh1989 opened this issue Sep 11, 2024 · 26 comments · Fixed by #2251 or #2254
Labels
dependencies Pull requests that update a dependency file security semver:major
Milestone

Comments

@nirh1989
Copy link

nirh1989 commented Sep 11, 2024

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/[email protected], 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/[email protected], 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/[email protected]
  └─┬ [email protected]
    ├── [email protected]
    └─┬ [email protected]
      └── [email protected]
npm ls path-to-regexp
myProject
└─┬ @slack/[email protected]
  ├─┬ [email protected]
  │ └── [email protected]
  └── [email protected]
@seratch seratch added security dependencies Pull requests that update a dependency file and removed untriaged labels Sep 11, 2024
@seratch seratch added this to the 3.21.3 milestone Sep 11, 2024
@seratch
Copy link
Member

seratch commented Sep 11, 2024

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
Copy link
Author

nirh1989 commented Sep 11, 2024

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/[email protected], 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/[email protected], 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
Copy link
Author

@seratch
Copy link
Member

seratch commented Sep 11, 2024

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
Copy link
Author

nirh1989 commented Sep 11, 2024

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
Copy link
Member

seratch commented Sep 11, 2024

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 #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
Copy link
Member

seratch commented Sep 11, 2024

Update: I created a draft pull request (#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
Copy link
Author

Thank you @seratch

When i should expect this to be merged?

@seratch
Copy link
Member

seratch commented Sep 11, 2024

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
Copy link
Author

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

@filmaj filmaj removed this from the 3.21.3 milestone Sep 11, 2024
@filmaj
Copy link
Contributor

filmaj commented Sep 11, 2024

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

@filmaj
Copy link
Contributor

filmaj commented Sep 11, 2024

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.

@filmaj filmaj added this to the 4.0.0 milestone Sep 11, 2024
@seratch
Copy link
Member

seratch commented Sep 11, 2024

@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
Copy link
Contributor

filmaj commented Sep 11, 2024

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
Copy link
Contributor

filmaj commented Sep 11, 2024

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 #2251

@seratch
Copy link
Member

seratch commented Sep 11, 2024

@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
Copy link
Contributor

filmaj commented Sep 11, 2024

OK, #2251 is ready for review.

@seratch seratch modified the milestones: 3.21.4, 4.0.0 Sep 11, 2024
@seratch seratch reopened this Sep 11, 2024
@seratch
Copy link
Member

seratch commented Sep 11, 2024

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
Copy link
Contributor

filmaj commented Sep 11, 2024

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.

@filmaj filmaj mentioned this issue Sep 13, 2024
6 tasks
@MeiravShimelman
Copy link

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

@seratch
Copy link
Member

seratch commented Sep 17, 2024

It seems serve-static's latest version no longer has the vul issue (meaning no [email protected] dependency via the package):

$ npm ls send
@slack/[email protected] /path-to-project/bolt-js
└─┬ [email protected]
  ├── [email protected]
  └─┬ [email protected]
    └── [email protected] deduped

@seratch seratch modified the milestones: 4.0.0, 3.21.4 Sep 17, 2024
@seratch
Copy link
Member

seratch commented Sep 17, 2024

@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
Copy link
Member

seratch commented Sep 17, 2024

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

@seratch seratch closed this as completed Sep 17, 2024
@lessquo
Copy link

lessquo commented Sep 19, 2024

Hello, it appears that @slack/[email protected] still requires [email protected]. Upgrading to [email protected] should fix the issue.

@seratch
Copy link
Member

seratch commented Sep 19, 2024

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
Copy link

lessquo commented Sep 19, 2024

I see, thanks for the kind response.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file security semver:major
Projects
None yet
5 participants