RDFLib / prez-ui

BSD 3-Clause "New" or "Revised" License
10 stars 7 forks source link

Security audit, review and update of node dependencies #79

Closed edmondchuc closed 1 year ago

edmondchuc commented 1 year ago

Fixes https://github.com/RDFLib/prez-ui/issues/76.

Branched off main on commit https://github.com/RDFLib/prez-ui/tree/ed9543588cdcd385214498b8c5fe508c1282edd9.

This PR fixes the vite security issue detailed in the npm audit command. It fixes it by bumping the minor and patch versions of the affected packages in package-lock.json.

npm audit ``` # npm audit report semver <7.5.2 Severity: moderate semver vulnerable to Regular Expression Denial of Service - https://github.com/advisories/GHSA-c2qf-rxjj-qqgw fix available via `npm audit fix --force` Will install npm-run-all@4.1.2, which is a breaking change node_modules/semver node_modules/superagent/node_modules/semver cross-spawn 6.0.0 - 6.0.5 Depends on vulnerable versions of semver node_modules/cross-spawn npm-run-all >=1.8.0 Depends on vulnerable versions of cross-spawn Depends on vulnerable versions of read-pkg node_modules/npm-run-all normalize-package-data <=2.5.0 Depends on vulnerable versions of semver node_modules/normalize-package-data read-pkg <=5.2.0 Depends on vulnerable versions of normalize-package-data node_modules/read-pkg vite 3.0.2 - 3.2.6 Severity: high Vite Server Options (server.fs.deny) can be bypassed using double forward-slash (//) - https://github.com/advisories/GHSA-353f-5xf4-qw67 fix available via `npm audit fix` node_modules/vite word-wrap * Severity: moderate word-wrap vulnerable to Regular Expression Denial of Service - https://github.com/advisories/GHSA-j8xg-fqg3-53r7 fix available via `npm audit fix` node_modules/word-wrap optionator 0.8.3 - 0.9.1 Depends on vulnerable versions of word-wrap node_modules/optionator 8 vulnerabilities (7 moderate, 1 high) To address issues that do not require attention, run: npm audit fix To address all issues (including breaking changes), run: npm audit fix --force ```

semver

Running the npm explain semver command shows a set of dev and prod dependencies using the semver package. There's nothing to do with the dev dependencies as these vulnerabilities do not exist once we perform a production build.

However, the prod dependencies using affected semver package are all from the @triply family of YASQUI packages.

Details of the vulnerability is here https://github.com/advisories/GHSA-c2qf-rxjj-qqgw, and the vulnerability description is:

Versions of the package semver before 7.5.2 are vulnerable to Regular Expression Denial of Service (ReDoS) via the function new Range, when untrusted user data is provided as a range.

The YASGUI packages use semver via superagent, which does not use the Range function. See https://github.com/search?q=repo%3Aladjs%2Fsuperagent+semver&type=code.

Action: Conclusion for now is, it is safe to use YASGUI with this vulnerability there, as long as superagent does not use the Range function.

npm explain semver ``` semver@5.7.1 dev node_modules/semver semver@"^5.5.0" from cross-spawn@6.0.5 node_modules/cross-spawn cross-spawn@"^6.0.5" from npm-run-all@4.1.5 node_modules/npm-run-all dev npm-run-all@"^4.1.5" from the root project semver@"2 || 3 || 4 || 5" from normalize-package-data@2.5.0 node_modules/normalize-package-data normalize-package-data@"^2.3.2" from read-pkg@3.0.0 node_modules/read-pkg read-pkg@"^3.0.0" from npm-run-all@4.1.5 node_modules/npm-run-all dev npm-run-all@"^4.1.5" from the root project semver@7.5.3 node_modules/superagent/node_modules/semver semver@"^7.3.2" from superagent@5.3.1 node_modules/superagent superagent@"5.3.1" from @triply/yasgui@4.2.28 node_modules/@triply/yasgui peer @triply/yasgui@"4.x" from @triply/yasqe@4.2.28 node_modules/@triply/yasqe @triply/yasqe@"^4.2.28" from the root project @triply/yasqe@"^4.2.28" from @triply/yasgui@4.2.28 @triply/yasqe@"^4.2.28" from @triply/yasr@4.2.28 node_modules/@triply/yasr @triply/yasr@"^4.2.28" from the root project @triply/yasr@"^4.2.28" from @triply/yasgui@4.2.28 peer @triply/yasgui@"4.x" from @triply/yasr@4.2.28 node_modules/@triply/yasr @triply/yasr@"^4.2.28" from the root project @triply/yasr@"^4.2.28" from @triply/yasgui@4.2.28 superagent@"5.3.1" from @triply/yasqe@4.2.28 node_modules/@triply/yasqe @triply/yasqe@"^4.2.28" from the root project @triply/yasqe@"^4.2.28" from @triply/yasgui@4.2.28 @triply/yasqe@"^4.2.28" from @triply/yasr@4.2.28 node_modules/@triply/yasr @triply/yasr@"^4.2.28" from the root project @triply/yasr@"^4.2.28" from @triply/yasgui@4.2.28 ```

word-wrap

Running the npm explain word-wrap command shows a set of dev packages depending on word-wrap. Since these are dev dependencies, they are not an issue in Prez UI in the production build.

Action: Nothing to do as word-wrap is used by dev dependencies.

npm explain word-wrap ``` word-wrap@1.2.3 dev node_modules/word-wrap word-wrap@"~1.2.3" from optionator@0.8.3 node_modules/optionator optionator@"^0.8.1" from escodegen@2.0.0 node_modules/escodegen escodegen@"^2.0.0" from jsdom@20.0.3 node_modules/jsdom dev jsdom@"^20.0.3" from the root project peerOptional jsdom@"*" from vitest@0.25.5 node_modules/vitest dev vitest@"^0.25.3" from the root project ```
jamiefeiss commented 1 year ago

If we're updating packages because of security concerns, shouldn't this PR also include updating the packages in package.json? If the yasqe & yasr packages are exposing a vulnerability, should they be updated as well? Also, one of the warnings (high severity) from npm audit includes updating Vite

edmondchuc commented 1 year ago

If we're updating packages because of security concerns, shouldn't this PR also include updating the packages in package.json? Also, one of the warnings (high severity) from npm audit includes updating Vite

Yes, I thought it'd update package.json directly too, if it's a direct dependency in this project, but running npm audit fix seemed to only update the package-lock.json. I'm guessing it's another dependency that has vite as a peer dependency on an older version with a vulnerability.

The vite high severity issue has been fixed by updating the peer dependencies for the dependent packages in package-lock.json.

Running npm explain vite.

vite@3.2.7
node_modules/vite
  dev vite@"^3.2.4" from the root project
  peer vite@"^3.0.0" from @vitejs/plugin-vue@3.2.0
  node_modules/@vitejs/plugin-vue
    dev @vitejs/plugin-vue@"^3.2.0" from the root project
  peer vite@"^2.0.0 || ^3.0.0 || ^4.0.0" from vite-plugin-rewrite-all@1.0.1
  node_modules/vite-plugin-rewrite-all
    vite-plugin-rewrite-all@"^1.0.1" from the root project
  peer vite@"^3.0.0" from vite-plugin-vue-type-imports@0.2.4
  node_modules/vite-plugin-vue-type-imports
    dev vite-plugin-vue-type-imports@"^0.2.4" from the root project
  vite@"^3.0.0" from vitest@0.25.5
  node_modules/vitest
    dev vitest@"^0.25.3" from the root project

There is no issue with vite now when running npm audit.

# npm audit report

semver  <7.5.2
Severity: moderate
semver vulnerable to Regular Expression Denial of Service - https://github.com/advisories/GHSA-c2qf-rxjj-qqgw
fix available via `npm audit fix --force`
Will install npm-run-all@4.1.2, which is a breaking change
node_modules/semver
  cross-spawn  6.0.0 - 6.0.5
  Depends on vulnerable versions of semver
  node_modules/cross-spawn
    npm-run-all  >=1.8.0
    Depends on vulnerable versions of cross-spawn
    Depends on vulnerable versions of read-pkg
    node_modules/npm-run-all
  normalize-package-data  <=2.5.0
  Depends on vulnerable versions of semver
  node_modules/normalize-package-data
    read-pkg  <=5.2.0
    Depends on vulnerable versions of normalize-package-data
    node_modules/read-pkg

word-wrap  *
Severity: moderate
word-wrap vulnerable to Regular Expression Denial of Service - https://github.com/advisories/GHSA-j8xg-fqg3-53r7
fix available via `npm audit fix`
node_modules/word-wrap
  optionator  0.8.3 - 0.9.1
  Depends on vulnerable versions of word-wrap
  node_modules/optionator

7 moderate severity vulnerabilities

To address issues that do not require attention, run:
  npm audit fix

To address all issues (including breaking changes), run:
  npm audit fix --force

YASQE and YASR are exposing a vulnerability through using a much older version of superagent (5.3.1) while the latest is on 8.0.9. In saying that, it looks like 8.0.9 is still using a version of semver (7.3.8) that has the vulnerability. The vulnerability for semver is fixed in the latest version (7.5.3) released last week.

jamiefeiss commented 1 year ago

Ah okay then, so updating yasqe & yasr won't address this vulnerability anyway. Alright, I'm happy with this change. Maybe soon we can update Vite regardless, as we should be keeping our dependencies up to date (as long as they don't break things!)

edmondchuc commented 1 year ago

Perfect, thanks for the review!