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 node_module vulnerable transitive dependencies "nth-check" and "trim". #6394

Closed
2 of 7 tasks
VictorGFM opened this issue Jan 18, 2022 · 2 comments
Closed
2 of 7 tasks
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

@VictorGFM
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

The current version of docusaurus (2.0.0-beta.14) relies on some transitive dependencies that are vulnerable. The GitHub already reported the CVEs related to those dependencies but the docusaurus still using them, so would be good if they could get upgraded.

Steps to reproduce

Verify the dependencies described in the description.

Expected behavior

Use the updated versions of the described dependencies.

Actual behavior

The versions of the described dependencies are vulnerable.

Your environment

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

Reproducible demo

No response

Self-service

  • I'd be willing to fix this bug myself.
@VictorGFM VictorGFM 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 Jan 18, 2022
@Josh-Cena Josh-Cena added closed: wontfix A fix will bring significant overhead, or is out of scope (for feature requests) 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 Jan 18, 2022
@Josh-Cena
Copy link
Collaborator

Josh-Cena commented Jan 19, 2022

Hi! If we could upgrade we would already, we regularly regenerate our lockfile to make our dependencies are the latest version possible (#6341). However, these dependencies are transitively depended on, so the upgrade doesn't depend on us.

Security auditing is broken in the front end, because much of the time, these CVEs don't actually impact anyone. I recommend you to read this Dan Abramov blog post: https://overreacted.io/npm-audit-broken-by-design/ And to back up this assertion, here's why we have these two dependencies:

=> Found "[email protected]"
info Reasons this module exists
   - "_project_#remark-parse" depends on it

=> Found "cheerio#[email protected]"
info Reasons this module exists
   - "_project_#@docusaurus#core#@slorber#static-site-generator-webpack-plugin#cheerio#css-select" depends on it

remark-parse is used to generate JSX from MDX. The static site generator plugin is also only a tool during build. Both of them are only used during the build phase and aren't sent down to the client. You shouldn't worry about security during the build phase, because the people who wrote the code and the people who run the build are the same group of people.

In short, upgrading involves unnecessary trouble (big architectural changes across major versions) while it doesn't actually fix any security issues. I'm going to close this as wontfix. Thanks for taking interest in this.

@VictorGFM
Copy link
Author

I see, thanks for the explanation!

@Josh-Cena Josh-Cena mentioned this issue Apr 5, 2022
7 tasks
@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 closed: wontfix A fix will bring significant overhead, or is out of scope (for feature requests) labels Apr 30, 2022
ozamosi pushed a commit to weaveworks/weave-gitops that referenced this issue May 9, 2022
This has an API change for algolia where the App ID needs to be
specified. See
https://docusaurus.io/blog/2021/11/21/algolia-docsearch-migration and
in particular note that they say these parameters are safe to put into
git. How did I know what parameters to enter? They're plain text
parameters as part of the request, I just grabbed them.

This also has an API change that causes the inlinetoc comoponent to be
a flat array instead of a tree - done in facebook/docusaurus#729 - so
this replaces the hard-coded index with a function that looks for the
start and end of the relevant section.

This gets rid of all security vulnerability warnings except for two,
which aren't relevant - see facebook/docusaurus#6394.
ozamosi pushed a commit to weaveworks/weave-gitops that referenced this issue May 9, 2022
This has an API change for algolia where the App ID needs to be
specified. See
https://docusaurus.io/blog/2021/11/21/algolia-docsearch-migration and
in particular note that they say these parameters are safe to put into
git. How did I know what parameters to enter? They're plain text
parameters as part of the request, I just grabbed them.

This also has an API change that causes the inlinetoc comoponent to be
a flat array instead of a tree - done in facebook/docusaurus#729 - so
this replaces the hard-coded index with a function that looks for the
start and end of the relevant section.

This gets rid of all security vulnerability warnings except for two,
which aren't relevant - see facebook/docusaurus#6394.
ozamosi pushed a commit to weaveworks/weave-gitops that referenced this issue May 9, 2022
This has an API change for algolia where the App ID needs to be
specified. See
https://docusaurus.io/blog/2021/11/21/algolia-docsearch-migration and
in particular note that they say these parameters are safe to put into
git. How did I know what parameters to enter? They're plain text
parameters as part of the request, I just grabbed them.

This also has an API change that causes the TOCInline component to be
a flat array instead of a tree - done in facebook/docusaurus#729 - so
this replaces the hard-coded index with a function that looks for the
start and end of the relevant section.

This gets rid of all security vulnerability warnings except for two,
which aren't relevant - see facebook/docusaurus#6394.
ozamosi pushed a commit to weaveworks/weave-gitops that referenced this issue May 9, 2022
This has an API change for algolia where the App ID needs to be
specified. See
https://docusaurus.io/blog/2021/11/21/algolia-docsearch-migration and
in particular note that they say these parameters are safe to put into
git. How did I know what parameters to enter? They're plain text
parameters as part of the request, I just grabbed them.

This also has an API change that causes the TOCInline component to be
a flat array instead of a tree - done in facebook/docusaurus#729 - so
this replaces the hard-coded index with a function that looks for the
start and end of the relevant section.

This gets rid of all security vulnerability warnings except for two,
which aren't relevant - see facebook/docusaurus#6394.
ozamosi pushed a commit to weaveworks/weave-gitops that referenced this issue May 9, 2022
This has an API change for algolia where the App ID needs to be
specified. See
https://docusaurus.io/blog/2021/11/21/algolia-docsearch-migration and
in particular note that they say these parameters are safe to put into
git. How did I know what parameters to enter? They're plain text
parameters as part of the request, I just grabbed them.

This also has an API change that causes the TOCInline component to be
a flat array instead of a tree - done in facebook/docusaurus#729 - so
this replaces the hard-coded index with a function that looks for the
start and end of the relevant section.

This gets rid of all security vulnerability warnings except for two,
which aren't relevant - see facebook/docusaurus#6394.
ozamosi pushed a commit to weaveworks/weave-gitops that referenced this issue May 11, 2022
This has an API change for algolia where the App ID needs to be
specified. See
https://docusaurus.io/blog/2021/11/21/algolia-docsearch-migration and
in particular note that they say these parameters are safe to put into
git. How did I know what parameters to enter? They're plain text
parameters as part of the request, I just grabbed them.

This also has an API change that causes the TOCInline component to be
a flat array instead of a tree - done in facebook/docusaurus#729 - so
this replaces the hard-coded index with a function that looks for the
start and end of the relevant section.

This gets rid of all security vulnerability warnings except for two,
which aren't relevant - see facebook/docusaurus#6394.
danielspofford added a commit to peridio/peridio-docs that referenced this issue Sep 2, 2022
Yarn audit and GitHub dependabot warn about got, nth-check, and trim.
This isn't actually really an issue according to
facebook/docusaurus#6394, but we will force
usage of newer version anyway to quiet the warnings.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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

2 participants