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

Update dependencies to non-vulnerable versions #10491

Closed
2 of 7 tasks
rebekaburnett opened this issue Sep 11, 2024 · 15 comments
Closed
2 of 7 tasks

Update dependencies to non-vulnerable versions #10491

rebekaburnett opened this issue Sep 11, 2024 · 15 comments
Labels
closed: please-fix-this-cve This issue is asking for fixing a CVE in a build-only dep which doesn't pose any real threat.

Comments

@rebekaburnett
Copy link

Have you read the Contributing Guidelines on issues?

Prerequisites

  • I'm using the latest version of Docusaurus.
  • I have tried the npm run clear or yarn clear command.
  • I have tried rm -rf node_modules yarn.lock package-lock.json and re-installing packages.
  • I have tried creating a repro with https://new.docusaurus.io.
  • I have read the console error message carefully (if applicable).

Description

Two packages required by docusaurus are outdated and are throwing security warnings in dependabot.
Please update:

  • path-to-regexp to 0.1.10
  • ws to 8.17.1 (ws)

image
image
image

Reproducible demo

No response

Steps to reproduce

This is a dependabot alert. It tries to upgrade dependent packages to non-vulnerable versions.

Expected behavior

Docusaurus packages would be secure and updated.

Actual behavior

Security alerts

Your environment

  • Public source code:
  • Public site URL:
  • Docusaurus version used: 3.5.2
  • Environment name and version (e.g. Chrome 89, Node.js 16.4):
  • Operating system and version (e.g. Ubuntu 20.04.2 LTS):

Self-service

  • I'd be willing to fix this bug myself.
@rebekaburnett rebekaburnett added bug An error in the Docusaurus core causing instability or issues with its execution status: needs triage This issue has not been triaged by maintainers labels Sep 11, 2024
@Josh-Cena
Copy link
Collaborator

When evaluating whether something is a vulnerability, you have to look at the attack vector and the respective cost of upgrading.

The first one, path-to-regexp.

$ yarn why path-to-regexp
yarn why v1.22.15
[1/4] 🤔  Why do we have the module "path-to-regexp"...?
[2/4] 🚚  Initialising dependency graph...
[3/4] 🔍  Finding dependency...
[4/4] 🚡  Calculating file sizes...
=> Found "[email protected]"
info Reasons this module exists
   - "_project_#@docusaurus#core#serve-handler" depends on it
   - Hoisted from "_project_#@docusaurus#core#serve-handler#path-to-regexp"
info Disk size without dependencies: "40KB"
info Disk size with unique dependencies: "40KB"
info Disk size with transitive dependencies: "40KB"
info Number of shared dependencies: 0
=> Found "react-router#[email protected]"
info This module exists because "_project_#@docusaurus#core#react-router" depends on it.
info Disk size without dependencies: "40KB"
info Disk size with unique dependencies: "64KB"
info Disk size with transitive dependencies: "64KB"
info Number of shared dependencies: 1
=> Found "express#[email protected]"
info This module exists because "_project_#@docusaurus#core#webpack-dev-server#express" depends on it.
info Disk size without dependencies: "16KB"
info Disk size with unique dependencies: "16KB"
info Disk size with transitive dependencies: "16KB"
info Number of shared dependencies: 0
✨  Done in 0.64s.

It's depended on by three paths. serve-handler and webpack-dev-server are both dev-only dependencies, so they will never compromise your production environment.

react-router is a production dependency, but if you look at the attack vector, you have to craft your path pattern in a specific way to trigger this ReDos attack. Docusaurus does not output wildcard paths in this format, so it's not a problem for any Docusaurus site unless you also have XSS, in which case you have other problems to worry about.

We do intend to upgrade to react-router v6, but that's a separate project and not done in the name of security. It's a much bigger project too and we haven't gotten to it, but it has always been on our roadmap.

Similarly, ws.

$ yarn why ws
yarn why v1.22.15
[1/4] 🤔  Why do we have the module "ws"...?
[2/4] 🚚  Initialising dependency graph...
[3/4] 🔍  Finding dependency...
[4/4] 🚡  Calculating file sizes...
=> Found "[email protected]"
info Has been hoisted to "ws"
info Reasons this module exists
   - "workspace-aggregator-12ee0c5d-4f4d-4ef3-b8ab-8f0789939fa5" depends on it
   - Hoisted from "_project_#jest-environment-jsdom#jsdom#ws"
   - Hoisted from "_project_#@docusaurus#core#webpack-dev-server#ws"
   - Hoisted from "_project_#website#@rsdoctor#webpack-plugin#@rsdoctor#graph#socket.io#engine.io#ws"
   - Hoisted from "_project_#website#@rsdoctor#webpack-plugin#@rsdoctor#graph#socket.io#socket.io-adapter#ws"
info Disk size without dependencies: "180KB"
info Disk size with unique dependencies: "180KB"
info Disk size with transitive dependencies: "180KB"
info Number of shared dependencies: 0
=> Found "webpack-bundle-analyzer#[email protected]"
info This module exists because "_project_#@docusaurus#core#webpack-bundle-analyzer" depends on it.
info Disk size without dependencies: "168KB"
info Disk size with unique dependencies: "168KB"
info Disk size with transitive dependencies: "168KB"
info Number of shared dependencies: 0
✨  Done in 1.15s.

Not going to explain it further but all of these are dev dependencies.

As a closing remark, https://overreacted.io/npm-audit-broken-by-design/ is worth a read on why you should generally not worry about dependabot alerts in Docusaurus projects. We build a static website and the attack vector is extremely narrow, if it even exists.

@Josh-Cena Josh-Cena closed this as not planned Won't fix, can't repro, duplicate, stale Sep 11, 2024
@Josh-Cena Josh-Cena added closed: please-fix-this-cve This issue is asking for fixing a CVE in a build-only dep which doesn't pose any real threat. and removed bug An error in the Docusaurus core causing instability or issues with its execution status: needs triage This issue has not been triaged by maintainers labels Sep 11, 2024
@johnxba
Copy link

johnxba commented Sep 12, 2024

Partial output of npm audit fix

path-to-regexp  2.0.0 - 3.2.0
Severity: high
path-to-regexp outputs backtracking regular expressions - https://github.com/advisories/GHSA-9wv6-86v2-598j
No fix available
node_modules/serve-handler/node_modules/path-to-regexp
  serve-handler  *
  Depends on vulnerable versions of path-to-regexp

Note No fix available.

advisory GHSA-9wv6-86v2-598j

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, < 8.0.0 8.0.0

This advisory states that an update to version 3.3.0 is available. Apparently, npm does not know that this version is available.

A manual install can be performed:
npm --prefix ./tmp install [email protected]

After this, the newly installed files need to be moved to node_modules/serve-handler/node_modules/path-to-regexp.

Once this is done for several vulnerable packages npm audit shows no more vulnerability notifications. A build showed no errors and the site works ok.

@rebekaburnett
Copy link
Author

@Josh-Cena Thank you for your thorough reply. 😺

@wellwelwel
Copy link

wellwelwel commented Sep 16, 2024

It's depended on by three paths. serve-handler and webpack-dev-server are both dev-only dependencies, so they will never compromise your production environment.

I'd like to bring up a different side, even though I understand that it's an intentional decision not to update development dependencies.


This issue in question has caused all my projects that depend on a cron (not an external bot), to stop working, because npm ci or npm i ends with exit code 1 when detecting vulnerabilities that cannot be fixed by npm audit fix.

For now, I think the only way is to ignore the exit code, but it could allow automatic commits for different errors.

@Josh-Cena, although this issue was closed as unplanned, if PRs are welcome, it would be great to know 🙋🏻‍♂️

Regardless, thanks in advance for your attention and for Docusaurus 🦖💙


For context:

Screenshot 2024-09-16 at 00 36 11
Screenshot 2024-09-16 at 00 35 23

@slorber
Copy link
Collaborator

slorber commented Sep 16, 2024

@wellwelwel we do accept PRs to fix warnings yes. Just don't overestimate the work it might require just to fix an harmless warning. If we don't fix it immediately, the reason is usually because it's not trivial at all, likely because the dependency is transitive and can't be upgraded without handling breaking changes of another larger library. You want to fix a small warning and often end up upgrading Webpack or something similar to the next major, requiring a Docusaurus breaking change and a new major version.

@steph-wang
Copy link

It's depended on by three paths. serve-handler and webpack-dev-server are both dev-only dependencies, so they will never compromise your production environment.

@Josh-Cena
I'm looking at the code on npm, and it seems like serve-handler and webpack-dev-server are both dependencies instead of dev-only dependencies?

@Josh-Cena
Copy link
Collaborator

They are dev dependencies to you—that is they won't be executed in the production environment. But they are real dependencies to us—that is they need to be transitively installed when you install the package. Dev dependencies declared in package.json won't be installed when published.

@slorber
Copy link
Collaborator

slorber commented Sep 25, 2024

If serve-handler is used in docusaurus serve and vulnerable to a RegExp DOS attack.

First, we don't particularly recommend using docusaurus serve in production, so at best you are more likely to DOS yourself in dev if only you found a way to exploit this.

Second, path-to-regexp is only used for the serve-handler rewrites/redirects feature, that we don't even use.

So it's technically impossible that even if you tried to DOS yourself using this vulnerability you'd succeed to do it without modifying our internal code.

For this reason, we don't consider this kind of vulnerability to be worth fixing. At least we don't prioritize them unless proven dangerous.

DOS vulnerabilities for a SSG are usually harmless because there's no runtime once the build succeeded. At best your CI job will fail due to the vulnerability.

@ryanv-knack
Copy link

Hi @slorber - I hope this message finds you well. First, I would like to express our gratitude for your hard work in maintaining docusaurus.

I completely understand your point about the low risk posed by the vulnerability in question, especially given that it's a "dev" dependency and should not be deployed in production environments. However, I would like to offer some perspective from our end, as an organization that must adhere to strict compliance and security standards.

Like many organizations today, we rely heavily on automated security and compliance tooling to ensure the integrity of our software supply chain. These tools often flag any vulnerabilities in our dependencies, whether dev or production, without allowing for much distinction in risk level. Unfortunately, this can place us in a difficult position, as we are obligated to address these flags to meet internal and external security policies.

While the actual risk of this particular issue might be minimal, having a clean security slate across all dependencies—even those only used in development—makes it significantly easier for us to maintain our compliance posture, which is becoming an increasingly important requirement for SaaS platforms like ours.

I kindly ask if you might reconsider updating the sub dependency in question. While I understand this may seem like a minor or low-priority task, this change could make a meaningful difference for organizations in heavily regulated sectors like ours.

Thank you again for your hard work, and I hope you understand our position. We look forward to continuing to use and support docusaurus.

@Josh-Cena
Copy link
Collaborator

Consider either resolving the dependency to a different version and hoping it works, patching the package locally, or sending a PR to us to update it. As we said, we have priorities and we are not going to invest extra time ourselves simply because some companies have bad security models. We will get to it one day, but the driving force of fixing issues caused by your workflow is ultimately you.

@slorber
Copy link
Collaborator

slorber commented Oct 3, 2024

@ryanv-knack I understand your point of view, but we have limited bandwidth and have to arbitrate between shipping meaningful improvements for all Docusaurus users, or investing time fixing warnings for those companies that have strict security requirements like yours.

Your company has strict security requirements and inflexible policies that do not acknowledge the fact a vulnerability could actually be harmless. That's your company's problem, not ours.

Instead of penalizing the Docusaurus ecosystem by reducing our bandwidth to implement new features and useful bug fixes, you should advocate for your company to change its security policies.

I understand that's not what you want to hear, but we don't plan to redirect our efforts to fixing those harmless warnings. Don't underestimate the cost for us to fix all the security warnings we have. Even if they are harmless, fixing all of them could take months, freeze the development of Docusaurus, and lead us to release breaking changes.

If you still want to use Docusaurus and don't plan to fix the inflexible security policies, there are options:

  • Send us good quality PRs fixing these warnings
  • Send us money or sponsor a contributor to work on it

It turns out that companies in regulated sectors that have such security policies are usually well-funded and can afford to contribute to the OSS software they use.

@yonas
Copy link

yonas commented Oct 4, 2024

It's depended on by three paths. serve-handler and webpack-dev-server are both dev-only dependencies, so they will never compromise your production environment.

Caution

What if the dev environment got compromised, which lead to a compromise of the developer's laptop, which further lead to the compromise of the production environment.

High profile cases like this have already happened. FreeBSD comes to mind.

If you still want to use Docusaurus and don't plan to fix the inflexible security policies, there are options:

Send us good quality PRs fixing these warnings
Send us money or sponsor a contributor to work on it

That's probably the attitude that leads to these type of break-ins.

And it doesn't make sense coming from a company with a $1.5 trillion dollar market capitalization.

Perhaps it's time to bring this topic up at the next major security-related event.

@slorber
Copy link
Collaborator

slorber commented Oct 4, 2024

Caution

What if the dev environment got compromised, which lead to a compromise of the developer's laptop, which further lead to the compromise of the production environment.

High profile cases like this have already happened. FreeBSD comes to mind.

If a dev environment can be compromised, we'll consider it a top priority to fix. That's not the case here, unless proven otherwise.

As far as I know, the reported vulnerabilities are RegExp DOS that can at best slow down / freeze the dev environment, not compromising it.

And in most cases you'd have to DDOS yourself or have a malicious teammate trying to annoy you. If you have a malicious teammate with Git access, this is more scary than having a local DDOS.

If you still want to use Docusaurus and don't plan to fix the inflexible security policies, there are options:
Send us good quality PRs fixing these warnings
Send us money or sponsor a contributor to work on it

That's probably the attitude that leads to these type of break-ins.

And it doesn't make sense coming from a company with a $1.5 trillion dollar market capitalization.

Perhaps it's time to bring this topic up at the next major security-related event.

You misunderstand how Meta allocates budget to OSS projects, and what it means to adopt MIT-licensed projects.

Let's remind you: https://github.com/facebook/docusaurus/blob/main/LICENSE

THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
SOFTWARE.

If these terms do not satisfy you, try another software with paid support and SLAs.

@facebook facebook locked and limited conversation to collaborators Oct 4, 2024
@slorber
Copy link
Collaborator

slorber commented Oct 17, 2024

Note serve-handler published v6.1.6 which upgrades path-to-regexp from v2 to v3 and remove one of those "vulnerable" dependencies (path-to-regexp v2).

We'll force all projects to upgrade in Docusaurus v3.6 but you can still upgrade this transitive dep independently.

See also #10587

@slorber
Copy link
Collaborator

slorber commented Oct 17, 2024

npx create-docusaurus@latest my-website classic --package-manager npm --javascript
cd my-website
npm audit

CleanShot 2024-10-17 at 13 51 45@2x

If you still have npm audit warnings under v3.5 it's because your transitive dependencies are not resolved to the latest versions. You can regenerate your lockfile.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
closed: please-fix-this-cve This issue is asking for fixing a CVE in a build-only dep which doesn't pose any real threat.
Projects
None yet
Development

No branches or pull requests

8 participants