withastro / astro

The web framework for content-driven websites. ⭐️ Star to support our work!
https://astro.build
Other
46.81k stars 2.49k forks source link

Astro relies on vulnerable `path-to-regexp` #11956

Closed corneliusroemer closed 1 month ago

corneliusroemer commented 2 months ago

Astro Info

Astro                    v4.15.4
Node                     v22.8.0
System                   macOS (arm64)
Package Manager          npm
Output                   server
Adapter                  @astrojs/node
Integrations             @astrojs/tailwind
                         @astrojs/react
                         @astrojs/mdx

If this issue only occurs in one browser, which browser is a problem?

No response

Describe the Bug

npm audit reports that astro relies on vulnerable versions of path-to-regexp

❯ 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 msw@0.35.0, 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

What's the expected result?

No reliance on vulnerable version

Link to Minimal Reproducible Example

NA

Participation

ematipico commented 2 months ago

PRs are welcome

hkbertoson commented 2 months ago

I just opened #11965 for this.

delucis commented 2 months ago

Just for reassurance for anyone seeing this, this vulnerability will only impact you if you “have two parameters within a single segment, separated by something that is not a period (.)” (source) in a dynamic route that is on-demand rendered.

As an example, a server-rendered site with the following file structure would be vulnerable due to the multiple parameters within a single segment:

src/
  pages/
    [color]-[animal].astro

That’s mostly pretty rare in Astro sites, but a malicious actor in theory could tie up your server by making requests with very long matching URL segments when using a pattern like this.

Patterns like [color]/[animal].astro with segments separated by a / are not impacted.

Static sites are also not really vulnerable. The worst case scenario would be a slower static build if you were using unsanitized user input as params from getStaticPaths(), but the ReDoS attack vector generally depends on making many slow requests, so it seems like an improbable risk (even if theoretically possible).

uwej711 commented 2 months ago

As far as I can see, the library is only used to generate routes, astro does not use the matching or regex generation, so this may not apply to astro at all.

uwej711 commented 2 months ago

After trying to update and adjust the existing code to the latest version of path-to-regexp I opted for removing it completely, see https://github.com/withastro/astro/pull/11981

uwej711 commented 2 months ago

Had to change the target branch, now https://github.com/withastro/astro/pull/11983

matiboux commented 2 months ago

FYI, the GitHub Advisory Database was updated for path-to-regexp. A fix in version 6 (6.3.0) is now available, which matches the version required by Astro (^6.2.2).

Latest patch table for path-to-regexp: Affected versions Patched versions
< 0.1.10 0.1.10
>= 0.2.0, < 1.9.0 1.9.0
>= 2.0.0, < 3.3.0 3.3.0
>= 4.0.0, < 6.3.0 6.3.0
>= 7.0.0, < 8.0.0 8.0.0

While the work in PRs to upgrade to 8.x.x or get rid of the dependency continues, Please fix your projects to upgrade to 6.3.0.

To fix quickly, I'll be opening a PR to update the version required by Astro to ^6.3.0.

uwej711 commented 2 months ago

I think we need to reopen that again (until I finish a correct implementation without path-to-regexp).

corneliusroemer commented 2 months ago

Indeed must be reopened due to original PR closing this being reverted in https://github.com/withastro/astro/pull/11993 @delucis

matiboux commented 2 months ago

Reminding you of #11985 in case that short term solution may become relevant again. Renovate bot could have picked up on it, but the path-to-regexp is currently frozen in the package.json file.

delucis commented 2 months ago

Thanks @matiboux! 6.3.0 includes breaking changes (see the failing tests in #11985 — https://github.com/withastro/astro/actions/runs/10847513941) so it’s not quite that simple either unfortunately.

Princesseuh commented 1 month ago

Fixed by https://github.com/withastro/astro/pull/12001