Docusaurus depends on trim@0.0.1 with CVE-2020-7753 vulnerability

[octogonz]

Have you read the Contributing Guidelines on issues?

• I have 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

@docusaurus/mdx-loader@2.0.0-beta.18 depends on @mdx-js/mdx@1.6.22 which depends on remark-parse@8.0.3 which is breaking our CI builds with this security scanner failure:

[INFO] _________________________________________________________________________________________________________________________________________________ 
[INFO] |Security Alerts                                                                                                                                | 
[INFO] |_______________________________________________________________________________________________________________________________________________| 
[INFO] |Alert title                             |Affected component                      |Severity                      |Due date                      | 
[INFO] |________________________________________|________________________________________|______________________________|______________________________| 
[INFO] |CVE-2020-7753                           |trim 0.0.1                              |High                          |                              | 
[INFO] |________________________________________|________________________________________|______________________________|______________________________| 
[INFO]  
##[warning]Component Governance detected 1 security related alerts at or above 'High' severity. Microsoft’s Open Source policy requires that all high and critical security vulnerabilities found by this task be addressed by upgrading vulnerable components. Vulnerabilities in indirect dependencies should be addressed by upgrading the root dependency.
[INFO] To change the severity threshold or build result, either dismiss the alerts in Component Governance or update the settings of this build task. 
[INFO] Please contact OpenSourceEngSupport@microsoft.com with questions. 
[INFO] Took 1.3847636 seconds to query alerts. 

Reproducible demo

N/A

Steps to reproduce

Here's an easy way to repro the issue:

1. npm init
2. npm install @docusaurus/mdx-loader
3. npm audit

                       === npm audit security report ===


                                 Manual Review
             Some vulnerabilities require your attention to resolve

          Visit https://go.npm.me/audit-guide for additional guidance


  High            Regular Expression Denial of Service in trim

  Package         trim

  Patched in      >=0.0.3

  Dependency of   @docusaurus/mdx-loader

  Path            @docusaurus/mdx-loader > @mdx-js/mdx > remark-parse > trim

  More info       https://github.com/advisories/GHSA-w5p7-h5w8-2hfq

Expected behavior

Docusaurus should not depend on packages with high severity security vulnerabilities.

Actual behavior

Security vulnerability

Your environment

• Docusaurus version used: 2.0.0-beta.18
• Environment name and version (e.g. Chrome 89, Node.js 16.4): Node v14.17.0
• Operating system and version (e.g. Ubuntu 20.04.2 LTS): Windows 10

Self-service

• I'd be willing to fix this bug myself.

[Josh-Cena]

Please see #6394

[Josh-Cena]

If your CI requires zero audit warning, your CI is broken. DO NOT rely on security audit for front end build pipelines.

[octogonz]

DO NOT rely on security audit for front end build pipelines.

@slorber Could we get a second opinion on this advice?

• I understand that the way an NPM package is used may make it impossible for a security vulnerability to be exploited
• I also understand that in many cases, this impossibility is immediately "obvious" to a knowledgeable person

However in a large code base, the way an NPM package is used is difficult to keep track of. Code evolves constantly. The analysis itself can be tricky. For example Docusaurus's hybrid client/server rendering makes it non-obvious whether a given NPM package will be executed by the browser runtime or Node.js tooling.

• For these reasons, corporate security departments prefer to simply upgrade every vulnerable package rather than carefully reviewing each case. I think I agree with this mentality.
• A categorical recommendation "DO NOT rely on security audit" seems... not wise

[Josh-Cena]

Apparently, the same issue has been responded to by multiple maintainers:

• Docusaurus does not allow for a passing npm audit in CI/CD pipelines #5501
• a vulnerability is introduced in docusaurus #5348
• npm vulnerability notices #5051

My response represents the response you will get from every single maintainer, and generally most maintainers in the front-end ecosystem. Docusaurus builds a static site; there's no way a user could be exploited by any security vulnerability whatsoever, unless they actively XSS themselves.

[Josh-Cena]

The data flow of Docusaurus is always one-way: developer -> build pipeline -> deployment env -> user. The user never sends data back to the server. When we say "Docusaurus has a server-side", this server-side is not the same as Next.js: it's build-time only, and once deployed, it's only a bunch of static assets.

The only viable attack vector that we are aware of is if you are outsourcing translations and injecting them via dangerouslySetInnerHtml, which may cause an XSS attack from a malicious translation contributor if there's no proofreading process—due to this consideration, all of our HTML text labels are untranslatable.

The lesson is that security audits are broken—it's not my word, it's the consensus from every OSS maintainer. Whenever you open a security issue somewhere, don't just say "there's a known CVE"—prove to us how this CVE can be actually exploited as an attack vector. Even better, please report this through the security portal for responsible disclosure.

[octogonz]

The user never sends data back to the server.

Take a look at our Docusaurus site:

https://rushstack.io/community/events/

Users do login-with-GitHub and then interact with a Node.js service via web pages that are rendered by the Docusaurus engine. You're really making a lot of assumptions about how a particular Docusaurus site might work.

[Josh-Cena]

That's your own code. Unless it's wired through some official Docusaurus API, we can't help with that. Since you are reporting it to upstream, and you have traced it down to exactly remark-parse, I'll assume you know whether any security threat comes in that chain😅

But anyways, I have spent enough time demonstrating to you why (a) it doesn't matter and (b) how to responsibly report security bugs in the front-end ecosystem. It's now your responsibility to abide to that.

[octogonz]

we can't help with that

Upgrading your @mdx-js/mdx dependency would fix this issue.

Since you are reporting it to upstream, and you have traced it down to exactly remark-parse, I'll assume you know whether any security threat comes in that chain

Yes, but I'm not the person who is a concern. It is the not-knowledgeable engineer who later adds another project to the same monorepo.

For example, let's say this new project is an Express service that happens to invoke trim() on a potentially malicious input, resulting in a real denial-of-service. Our security scanner should flag that, but oops! -- CVE-2020-7753 is exempted because at the time it seemed irrelevant for the Docusaurus project. (If I understand correctly, you're actually arguing to go further and disable every security check for the entire repo; I can't imagine any corporate compliance group tolerating that approach!)

See, security vulnerabilities by nature are tricky to analyze. This is why it's simpler just to work to get them eliminated from the dependency tree, rather than getting into armchair debates about each vulnerability that comes along.

it's not my word, it's the consensus from every OSS maintainer

As a maintainer of OSS myself, I would certainly give a lower priority to fixing an irrelevant-seeming vulnerability. But I would not dismiss it or argue that we shouldn't bother fixing the vulnerability at all, particularly for a high profile project like Docusaurus.

[Josh-Cena]

Upgrading your @mdx-js/mdx dependency would fix this issue.

That's the whole point of every issue I've been pointing to—right? We bump dependencies every week, there's no reason why we won't upgrade it if we can. However, upgrading to MDX v2 has been postponed to Docusaurus v3 because it would be a huge breaking change that will influence every single user, and it also uses ESM which we are not compatible with yet. We have #4029 and #6520 to track them.

If this audit warning really bothers you, consider persuading the remark maintainers to release a patch version for the particular version of remark-parse used by MDX v1. (Alert: I'm pretty sure they won't like the idea)

If I understand correctly, you're actually arguing to go further and disable every security check for the entire repo

I'm saying you should ignore it for Docusaurus because it's a front-end build tool, repeat "build tool". It doesn't offer any runtime features. Your comparison to Express is inadequate and there's a world of difference in between, which is why I said the "single direction of data flow" matters. Express is neither front-end nor "build pipeline". I don't see why my carefully-crafted statement that "DO NOT rely on security audit for front end build pipelines" can be stretched so much.

security vulnerabilities by nature are tricky to analyze

Totally agree. That's why I'm spending so much time demonstrating to you it doesn't matter, instead of instantly dismissing it as "it doesn't matter; now go".

particularly for a high profile project like Docusaurus.

Well, there are much higher-profile projects that take literally the same stance: facebook/create-react-app#11174

Again, we don't work on your team, we can't control your security policy. All we can ask you is, when you open a bug report, please please provide a reproduction. Maintainers have limited time (I have relatively much more time than @slorber but I'm volunteering), so it's important for you to demonstrate yourself how a particular bug is Docusaurus' issue, and for CVEs, how it causes tangible harm. It takes you and your team probably less than 5 minutes to run npm audit and write that report, but had we not been aware of this issue before, it would take us much more time than that to actually verify it's a security threat. The burden is on you, if you want to see it fixed, to demonstrate end-to-end why it's a problem and how we can go about fixing it. Security bugs are no exception to the reproduction → cause track-down → solution proposal workflow.

[slorber]

See, security vulnerabilities by nature are tricky to analyze.

Not all of them IMHO.

We are a build tool that runs in your CI to produce a static site output, based on inputs you commit yourself to Git.

To be affected by such Denial-of-Service you'd have to want to hack yourself and commit malicious content on Git.

And it would only DOS your CI (potentially blocking or slowing down the deployment), not really affecting real site visitors.

---

I don't think it's good to disable/ignore all security warnings, but I guess in our context you can just ignore all those Regular Expression Denial of Service errors by default.

I believe those shouldn't have Severity: Hight in the first place 🤷‍♂️ and in our particular context it's probably not even Medium.

See also https://overreacted.io/npm-audit-broken-by-design/

---

However in a large code base, the way an NPM package is used is difficult to keep track of. Code evolves constantly. The analysis itself can be tricky. For example Docusaurus's hybrid client/server rendering makes it non-obvious whether a given NPM package will be executed by the browser runtime or Node.js tooling.

We keep the browser runtime very small in core and theme-classic so there's only a limited number of dependencies being used there.

If there's a DOS vulnerability in a browser package that could affect real users, we'll look into it as a priority.

Now if you don't know if it's server or browser code, you can ask (like in this issue) and we'll tell you.

---

We don't have the bandwidth to fix all those harmless vulnerabilities ourselves, but I understand if your company remains afraid of such vulnerabilities.

It remains possible for you:

• to fork the project and fix those vulnerabilities (some version bumps are not so easy)
• force resolution of trim to a newer version, hoping it will keep working (eventually use patch-package to adapt some code that doesn't work)
• help us fund the project so that we have more bandwidth to fix such vulnerabilities.