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

Replace wait-on dependency #9537

Closed
3 of 7 tasks
NickGerleman opened this issue Nov 12, 2023 · 3 comments · Fixed by #9547
Closed
3 of 7 tasks

Replace wait-on dependency #9537

NickGerleman opened this issue Nov 12, 2023 · 3 comments · Fixed by #9547
Labels
domain: dependencies Proposal to upgrade a dependency across major versions

Comments

@NickGerleman
Copy link
Contributor

NickGerleman commented Nov 12, 2023

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

@docusaurus/core depends on wait-on@^7.0.1, which in turn depends on axios@^0.27.2. This version will now trigger GitHub vulnerability warnings due to axios/axios#6006 effecting axios before 1.6.0.

The newest version of wait-on still depends on old version of axios. Docusaurus only uses it in a single place, so it seems reasonable to remove or replace the dependency with something else.

Reproducible demo

No response

Steps to reproduce

yarn audit Docusaurus app

Expected behavior

Audit is clean

Actual behavior

Audit shows vulns from axios

Your environment

No response

Self-service

  • I'd be willing to fix this bug myself.
@NickGerleman NickGerleman 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 Nov 12, 2023
@Josh-Cena
Copy link
Collaborator

Please see #6394 (comment) for our stance on security bugs. This one in particular has exactly zero impact because our usage of wait-on does not even use axios. So we are not going to invest time finding another package and changing our code because of a "bug" that has no impact. If you have some security policy that requires absolutely no CVE, feel free to use your package manager's resolution to force resolving axios to a newer version.

@Josh-Cena Josh-Cena closed this as not planned Won't fix, can't repro, duplicate, stale Nov 12, 2023
@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 Nov 12, 2023
@NickGerleman
Copy link
Contributor Author

NickGerleman commented Nov 13, 2023

@Josh-Cena would you accept a contribution changing the dep?

I fully agree with your assessment on the lack of a real-world security risk associated with this warning (or most other JS CVEs). Though some teams and companies do still try to keep clean regardless. I think I could avoid work for those folks if I changed this at the library level, instead of for a single repo’s resolutions consuming Docusarus.

E.g. Metas infrastructure pinged me as RN oncall about even a medium level GitHub vulnerability on React Native Website. It will ping other owners of OSS Docusaurus repoes at Meta at the same time to fix it, so N people now get separate issues to fix this, because the policy is set at a greater organizational level.

@Josh-Cena
Copy link
Collaborator

Sure, if you want to work on a fix, PRs are definitely welcome. I'll re-open the issue for now so you could track it. I certainly would like to see a library that's more maintained and significantly smaller (without all the observables/HTTP dependencies).

@Josh-Cena Josh-Cena reopened this Nov 13, 2023
@Josh-Cena Josh-Cena added domain: dependencies Proposal to upgrade a dependency across major versions and removed 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. labels Nov 13, 2023
@Josh-Cena Josh-Cena changed the title Docusaurus pulls in axios < 1.6.0 triggering GitHub vulnerability warnings Replace wait-on dependency Nov 13, 2023
NickGerleman added a commit to NickGerleman/docusaurus that referenced this issue Nov 14, 2023
Fixes facebook#9537

This change removes usage of `wait-on`, and replaces it with an effective copy of the algorithm it ends up taking for our use case.

1. `wait-on` sees a file as present when `fs.stat` on the path stops throwing
2. It polls on a timer (which WaitPlugin sets to 300ms)
3. It waits until a time has passed without file size changing (defaults to 750ms)
4. `wait-on` defaults to no timout, so we poll forever.

See https://github.com/jeffbski/wait-on/blob/master/lib/wait-on.js for reference
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain: dependencies Proposal to upgrade a dependency across major versions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants