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

msw seems to depend on vulnerable versions of path-to-regexp #2270

Closed
4 tasks done
corneliusroemer opened this issue Sep 9, 2024 · 13 comments · Fixed by #2285
Closed
4 tasks done

msw seems to depend on vulnerable versions of path-to-regexp #2270

corneliusroemer opened this issue Sep 9, 2024 · 13 comments · Fixed by #2285
Labels
bug Something isn't working needs:triage Issues that have not been investigated yet. scope:node Related to MSW running in Node

Comments

@corneliusroemer
Copy link

corneliusroemer commented Sep 9, 2024

Prerequisites

Environment check

  • I'm using the latest msw version
  • I'm using Node.js version 18 or higher

Node.js version

NA

Reproduction repository

loculus-project/loculus

Reproduction steps

npm audit

Current behavior

❯ npm audit
# npm audit report

path-to-regexp  0.2.0 - 7.1.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 [email protected], which is a breaking change
node_modules/path-to-regexp
  astro  <=0.0.0-xray-20231129021231 || >=0.18.0-collections.1
  Depends on vulnerable versions of path-to-regexp
  node_modules/astro
    @astrojs/mdx  <=0.0.0-vercel-upgrade-20230905174957 || >=1.0.0-beta.0
    Depends on vulnerable versions of astro
    node_modules/@astrojs/mdx
    @astrojs/node  <=0.0.2-next.0 || >=3.0.0
    Depends on vulnerable versions of astro
    node_modules/@astrojs/node
    @astrojs/tailwind  <=0.0.0-wasm-20220915005725 || >=3.0.0-beta.0
    Depends on vulnerable versions of astro
    node_modules/@astrojs/tailwind
  msw  <=0.0.1 || 0.2.2 - 0.4.2 || >=0.36.0
  Depends on vulnerable versions of path-to-regexp
  node_modules/msw

6 high severity vulnerabilities

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

Expected behavior

The latest version of msw doesn't use a vulnerable version of path-to-regexp

@corneliusroemer corneliusroemer added bug Something isn't working needs:triage Issues that have not been investigated yet. scope:node Related to MSW running in Node labels Sep 9, 2024
di0nys1us added a commit to navikt/hotsak-frontend that referenced this issue Sep 10, 2024
vi har dette problemet
mswjs/msw#2270

ser ut som vi må vente på at msw oppdaterer sin versjon av path-to-regexp før denne audit feilen blir borte
@mehradrishty
Copy link

facing the same issue. It still exists.

@marekdano
Copy link

yes, I'm facing the same issue in v1.3.4

@Eric-Arellano
Copy link

Eric-Arellano commented Sep 10, 2024

facing the same issue. It still exists.
yes, I'm facing the same issue in v1.3.4

Gentle open source etiquette tip: it's not helpful to reply with a message like "same". It creates noise for everyone subscribed to the issue. Instead, the best thing to do is upvote the original message.

@krimple
Copy link

krimple commented Sep 10, 2024

With VERY limited testing, I think I may have a short-term workaround. Not sure what the far reaching effects are, but, adding this to my package.json file tells the msw library to use v8.0.0 of the path-to-regexp library instead of the other version.

  "overrides": {
    "msw": {
     "path-to-regexp": "^8.0.0"
     }
  },

Then an npm audit doesn't find the vulnerability anymore.

That would imply a breaking change, so maybe we need to just wait on a fix. So far with a few routes it looks like it works for me, but YMMV.

@direisc
Copy link

direisc commented Sep 10, 2024

With VERY limited testing, I think I may have a short-term workaround. Not sure what the far reaching effects are, but, adding this to my package.json file tells the msw library to use v8.0.0 of the path-to-regexp library instead of the other version.

I tried that approach, for my project stop works.
Same for version 7.2.x but that one still throw errors on audit.

@shotanue
Copy link

shotanue commented Sep 11, 2024

The current version of msw depends on [email protected], which in turn depends on [email protected]. This older version of path-to-regex has known security vulnerabilities.

Current msw pnpm-lock.yaml

[email protected] > [email protected]

path-to-regexp: 0.1.7

The new version of [email protected], which was released on yesterday(2024/09/10), depends on [email protected], which resolves the security vulnerabilities present in the older version.

To update express to 4.20.0 may fix this issue.
I overlooked that msw itself depends on path-to-regexp. I didn't read this issue well, sorry.

path-to-regexp:

Release: [email protected]
https://github.com/expressjs/express/releases/tag/4.20.0

PR
expressjs/express#5902

@j-seixas
Copy link

j-seixas commented Sep 11, 2024

Yeah, it needs to update both path-to-regexp and express packages so that this vulnerability is patched

@fwg-dev-butter
Copy link

As MSW is only a dev dependency for my company I updated our pipeline audit check to include '--omit=dev'

This may help others until a fix is deployed

@agadzinski93
Copy link

To the devs: I forked msw and ran the tests and it seems the breaking change for path-to-regexp v6 and v8 is the match function provided by path-to-regexp which was changed to no longer support certain characters.

Seems the function coercePath in src/core/utils/matching/matchRequestUrl.ts needs to be updated with how it modifies strings

Here is a link addressing this from the devs of path-to-regexp regarding the unexpected characters - https://github.com/pillarjs/path-to-regexp?tab=readme-ov-file#unexpected-----etc

@svozza
Copy link

svozza commented Sep 12, 2024

The fix has also been backported to the v6 version of path-to-regexp so it might be enough to just bump the dependency to 6.3.0 rather than have to go straight to 8.0.0.

Nvm, just saw in the package.json that the version is ^6.2.0 so it would automatically get the new version but the vulnerability is still being reported by npm audit.

JeffJacobson added a commit to WSDOT-GIS/wsdot-mp-map that referenced this issue Sep 12, 2024
See [msw seems to depend on vulnerable versions of path-to-regexp #2270](mswjs/msw#2270)
@kettanaito
Copy link
Member

kettanaito commented Sep 15, 2024

@agadzinski93, thank you for doing that research. There are also some type changes across major version updates, but those should be manageable.

A great reminder from @fwg-dev-butter that MSW is a development dependency. Even if it, or its transitive dependencies have vulnerabilities, that is not the same as a production-grade dependency shipping vulnerabilities to your users. You don't ship MSW to your users, you use it on your machine. That doesn't negate the vulnerability, but it severely minimizes its scope to being pretty much non-existing.

Configuring your reporting property will save you, your company, and open source maintainers a lot of time and stress. So do that.

I am all for updating dependencies though. Pull requests are welcome with this one!

@markmssd
Copy link
Contributor

I've opened #2285 which upgrades express to v5 (not a breaking change because it's only used in tests) and path-to-regexp to 6.3.0 (which also fixes the vulnerability, just need to wait for the audit to acknowledge it). I can try to upgrade path-to-regexp to v8 in a few days as well.

@kettanaito
Copy link
Member

Released: v2.4.8 🎉

This has been released in v2.4.8!

Make sure to always update to the latest version (npm i msw@latest) to get the newest features and bug fixes.


Predictable release automation by @ossjs/release.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working needs:triage Issues that have not been investigated yet. scope:node Related to MSW running in Node
Projects
None yet
Development

Successfully merging a pull request may close this issue.