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

Astro relies on vulnerable path-to-regexp #11956

Closed
1 task
corneliusroemer opened this issue Sep 9, 2024 · 12 comments · Fixed by #11983
Closed
1 task

Astro relies on vulnerable path-to-regexp #11956

corneliusroemer opened this issue Sep 9, 2024 · 12 comments · Fixed by #11983
Labels
- P4: important Violate documented behavior or significantly impacts performance (priority) ecosystem: upstream Upstream package has issue feat: routing Related to Astro routing (scope)

Comments

@corneliusroemer
Copy link

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 [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

What's the expected result?

No reliance on vulnerable version

Link to Minimal Reproducible Example

NA

Participation

  • I am willing to submit a pull request for this issue.
@github-actions github-actions bot added the needs triage Issue needs to be triaged label Sep 9, 2024
@ematipico
Copy link
Member

PRs are welcome

@hkbertoson
Copy link

I just opened #11965 for this.

@delucis
Copy link
Member

delucis commented Sep 11, 2024

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).

@ematipico ematipico added ecosystem: upstream Upstream package has issue - P4: important Violate documented behavior or significantly impacts performance (priority) feat: routing Related to Astro routing (scope) and removed needs triage Issue needs to be triaged labels Sep 11, 2024
@uwej711
Copy link
Contributor

uwej711 commented Sep 11, 2024

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

uwej711 commented Sep 12, 2024

After trying to update and adjust the existing code to the latest version of path-to-regexp I opted for removing it completely, see #11981

@uwej711
Copy link
Contributor

uwej711 commented Sep 13, 2024

Had to change the target branch, now #11983

@matiboux
Copy link
Contributor

matiboux commented Sep 13, 2024

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.

@delucis delucis linked a pull request Sep 13, 2024 that will close this issue
delucis pushed a commit that referenced this issue Sep 13, 2024
@uwej711
Copy link
Contributor

uwej711 commented Sep 15, 2024

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

@corneliusroemer
Copy link
Author

corneliusroemer commented Sep 15, 2024

Indeed must be reopened due to original PR closing this being reverted in #11993
@delucis

@delucis delucis reopened this Sep 15, 2024
@github-actions github-actions bot added the needs triage Issue needs to be triaged label Sep 15, 2024
@matiboux
Copy link
Contributor

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.

@Princesseuh Princesseuh removed the needs triage Issue needs to be triaged label Sep 16, 2024
@delucis
Copy link
Member

delucis commented Sep 16, 2024

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

@Princesseuh
Copy link
Member

Fixed by #12001

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P4: important Violate documented behavior or significantly impacts performance (priority) ecosystem: upstream Upstream package has issue feat: routing Related to Astro routing (scope)
Projects
None yet
7 participants