Skip to content

Updated Security Audit Fixes - #9211

Open
danrossi wants to merge 3 commits into
videojs:mainfrom
danrossi:audit-fix-new
Open

Updated Security Audit Fixes#9211
danrossi wants to merge 3 commits into
videojs:mainfrom
danrossi:audit-fix-new

Conversation

@danrossi

@danrossi danrossi commented Jul 1, 2026

Copy link
Copy Markdown

Description

Related #9166 #9210

This fixes the current security vulnerabilities minus the browserify which is not used to build anything so could be removed.

Specific Changes proposed

This updates packages with overrides to fix the security audit issues

The access-sniff accessibility test has been changed to a pa11y accessibility test report. So that old outdated package can be removed. Some overrides wouldn't fix it's problem so safe to remove.

Requirements Checklist

  • [ x] Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
    • Change has been verified in an actual browser (Chrome, Firefox, IE)
    • Unit Tests updated or fixed
    • Docs/guides updated
    • Example created (starter template on JSBin)
    • Has no DOM changes which impact accessiblilty or trigger warnings (e.g. Chrome issues tab)
    • Has no changes to JSDoc which cause npm run docs:api to error
  • Reviewed by Two Core Contributors

@welcome

welcome Bot commented Jul 1, 2026

Copy link
Copy Markdown

💖 Thanks for opening this pull request! 💖

Things that will help get your PR across the finish line:

  • Run npm run lint -- --errors locally to catch formatting errors earlier.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@Essk

Essk commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Thanks for putting this together. The audit cleanup is a worthwhile direction and I'd like to see it land in some form, but as it stands the changes require a newer Node than the project runs.

The repo pins Node 14 via .nvmrc, and I've run CI so we can see what that means in practice (Node 14.21.3 / npm 6.14.18):

  • npm 6 can't use the lockfileVersion: 3 lockfile in this PR. The install step warned package-lock.json was generated for lockfileVersion@3. I'll try to do my best with it! and fell back to re-resolving from package.json, effectively discarding the lockfile.
  • Because npm below 8.3 also ignores overrides, the vulnerability pins silently don't apply. You can see this in the install log: it installed glob@7.2.3 (with its security deprecation warning) despite this PR pinning glob to 13. So under the project toolchain the PR doesn't deliver its intended effect even where install appears to succeed.
  • The run then failed before exercising any of the new tooling: lint (eol-last, build/test-pa11y.js is missing a trailing newline) took down the unit and coverage jobs, and the conventional commit check failed on the commit messages.
  • Beyond what this run reached, several of the updated or pinned packages require Node 18+ themselves (pa11y 9, postcss-cli 11, remark-cli 12, minimatch 10, glob 13, update-notifier 7, @octokit/rest 22), so later stages would hit runtime incompatibilities even once lint passes.

So the package's Node version would need to be updated first, with all the build tooling tested, updated, and fixed as necessary, before a change of this shape is acceptable. That's a deliberate decision with a much wider blast radius (karma/BrowserStack runs, webpack 1 to 5 for the test builds, release tooling) and should be its own PR rather than riding along with an audit pass.

If you want to keep this moving, two shapes would work:

  1. A toolchain PR first: bump .nvmrc, get CI green end to end on the new Node, then rebase this on top, at which point the lockfile migration becomes correct rather than accidental. This is not a small job; possible hazards include:
    • build:test:webpack runs webpack 1, which uses md4 hashing and hard-fails on Node 17+ under OpenSSL 3 (ERR_OSSL_EVP_UNSUPPORTED). So the webpack 5 move in this PR is actually a prerequisite of the Node bump, not a passenger, and it brings config/CLI changes with it (--hide-modules is gone, webpack-cli is a separate package, output syntax changed).
    • npm 7+ auto-installs peer dependencies and hard-fails on conflicts that npm 6 only warned about. The dev tree here is old (deprecated rollup-plugin-* packages, the karma stack), so expect a round of resolution failures needing --legacy-peer-deps or dep bumps.
    • husky is on ^1.3.1, whose postinstall-based hook model predates modern npm; hooks may silently stop installing and it likely needs the husky 5+ migration.
    • karma still works on Node 20 but is archived/EOL, so the ChromeHeadless and BrowserStack launcher paths need re-verifying rather than assuming.
    • The lockfile regeneration is contributor-facing: once it's v3, everyone must be on the new Node/npm together, so .nvmrc, CONTRIBUTING, and the CI matrix should move in the same PR (and the Netlify build image is pinned separately from .nvmrc).
  2. A reduced version compatible with Node 14: pin npm 8.x (which still supports Node 14) to get overrides support, restricted to the pins that don't require newer Node (elliptic, underscore, crypto-browserify, browserify-sign) plus the in-range bumps.

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

Successfully merging this pull request may close these issues.

2 participants