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

Spectacle: Update dependencies to remediate trim vulnerability #1328

Open
Burnett2k opened this issue Mar 13, 2024 · 3 comments
Open

Spectacle: Update dependencies to remediate trim vulnerability #1328

Burnett2k opened this issue Mar 13, 2024 · 3 comments

Comments

@Burnett2k
Copy link
Member

This is a complicated topic, so I will do my best to summarize all of the issues at play here. Primarily, this is on our radar because dependabot alerted us that sub dependencies reference a vulnerable package of trim. Unfortunately, there is not a clear or easy upgrade path to make this happen. Additionally, it's somewhat debatable whether or not this vulnerability should be considered as high priority as dependabot claims as it's a simple DoS regex. Since this is a presentation library, there's not much Denial of Service things you can do other than mess up your own decks which you can already do anyways ;)

Anyways, to get us off of trim completely, here's what I think we need to do. I might have missed a few things.

upgrade mxs-js

  1. dependabot warned of an issue within trim
  2. We don’t directly use trim, but sub dependencies do. All instances of the vulnerable trim version come from remark-parse@8.0.3
    1. mdx-js/mdx (used in spectacle mdx loader) ⇒ Remark-parse ⇒ trim
      1. will need to upgrade to v2 to get away from the trim dependency
      2. new version is ESM and will potentially require some re-work
      3. Recommended that we release this as a major version upgrade
    2. docusaurus/core (used for the documentation website) ⇒ remark-parse ⇒ trim
    3. remark-parse(from package.json within spectacle) ⇒ trim
      1. This can be fixed relatively easily by going to v9
      2. The notes make it seem as if this is not a big change ,no changes to the actual api from what I can tell
  3. to fix the trim issue, mdx-js and remark-parse need major upgrades. Upgrading mdx-js will likely give us access to newer and better markdown parsing, so we would get some benefit from doing that anyways.

relevant docs:

output of pnpm why (excluding the dev-dependency docusaurus)

❯ pnpm why trim --filter '!@docusaurus' -P

../packages/create-spectacle             |  WARN  The field "resolutions" was found in /Users/sawyerburnett/git-repos/formidable/spectacle/packages/create-spectacle/package.json. This will not take effect. You should configure "resolutions" at the root of the workspace instead.
No projects matched the filters "@docusaurus" in "/Users/sawyerburnett/git-repos/formidable/spectacle"
Legend: production dependency, optional only, dev only

spectacle-example-js /Users/sawyerburnett/git-repos/formidable/spectacle/examples/js

dependencies:
spectacle link:../../packages/spectacle
└─┬ remark-parse 8.0.3
  └── trim 0.0.1

spectacle-example-md /Users/sawyerburnett/git-repos/formidable/spectacle/examples/md

dependencies:
spectacle link:../../packages/spectacle
└─┬ remark-parse 8.0.3
  └── trim 0.0.1

spectacle-example-mdx /Users/sawyerburnett/git-repos/formidable/spectacle/examples/mdx

dependencies:
spectacle link:../../packages/spectacle
└─┬ remark-parse 8.0.3
  └── trim 0.0.1

spectacle-example-ts /Users/sawyerburnett/git-repos/formidable/spectacle/examples/typescript

dependencies:
spectacle link:../../packages/spectacle
└─┬ remark-parse 8.0.3
  └── trim 0.0.1

spectacle@10.1.7 /Users/sawyerburnett/git-repos/formidable/spectacle/packages/spectacle

dependencies:
remark-parse 8.0.3
└── trim 0.0.1

spectacle-mdx-loader@0.1.1 /Users/sawyerburnett/git-repos/formidable/spectacle/packages/spectacle-mdx-loader

dependencies:
@mdx-js/mdx 1.6.22
├─┬ remark-mdx 1.6.22
│ └─┬ remark-parse 8.0.3
│   └── trim 0.0.1
└─┬ remark-parse 8.0.3
  └── trim 0.0.1
@ryan-roemer
Copy link
Member

v9 is fine for docs / core spectacle.

Making mdx loader ESM is a bigger deal because we're cutting off CJS users.

@Burnett2k
Copy link
Member Author

@ryan-roemer In case the ESM / CommonJS issue comes up again, what is Formidable's stance? Sounds like we plan to support CJS users as long as possible?

If we released this upgrade as a major version upgrade, it wouldn't quite be cutting off users. But I do see how it could be perceived that way by the community.

It does seem if we want to continue to modernize the package with new features, new markdown syntax support, and keep it up-to-date, we will eventually need to upgrade or look for alternative dependencies which don't force ESM.

This all stems from some fairly low priority security issues and since we aren't pushing for new features within spectacle, I'm quite fine to just punt on this indefinitely until we find a more pressing need to upgrade.

@ryan-roemer
Copy link
Member

I'm interested in @carbonrobot 's input here, but my thoughts are that we don't have a strict "stance" as much as want to follow what the communities of our various OSS can best be served by. For example:

  • For an application, it's whatever the team wants as you're all the upstream consumers
  • For a frontend-only library that will always be consumed by a bundler, it's whatever the team wants.
  • For a Node library, you want to consider what your community actually uses.

Let's look at one example -- the popular chalk library from Sindre, who's an early ESM-only converter: https://www.npmjs.com/package/chalk?activeTab=versions (I'm ignoring small numbered versions)

The overwhelming usage is still CJS despite original 5.0.0 version published over two years ago.

For us here for spectacle, we could consider it a special case as Spectacle is consumed as a frontend library. spectacle-mdx-loader is a Node library, but it's limited to just the build. So not a completely clear cut case, but I'd probably favor sticking with CJS rather than forcing all folks who want to create MDX decks to switch to ESM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants