Fix highest CVEs - #228
Conversation
a56cc6f to
f262fdf
Compare
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates dependency tooling and build configuration across multiple packages, primarily to address security advisories and remove obsolete lint/build dependencies.
Changes:
- Removed legacy ESLint/Babel/Airbnb config dependencies from several package manifests.
- Upgraded PostCSS to
8.5.25across affected packages and through Rush/PNPM overrides. - Added additional
pnpmglobalOverridesand introduced Rush change files for impacted packages.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/modules/storybook-auth-addon/package.json | Drops eslint-config-airbnb from devDependencies. |
| packages/modules/manage-versions/package.json | Drops eslint-config-airbnb; bumps postcss version. |
| packages/modules/imodel-browser/rollup.config.js | Removes @svgr/rollup import and plugin usage from Rollup build. |
| packages/modules/imodel-browser/package.json | Removes @svgr/rollup, babel-eslint, eslint-config-airbnb; bumps postcss. |
| packages/modules/delete-itwin/package.json | Drops eslint-config-airbnb; bumps postcss version. |
| packages/modules/delete-imodel/package.json | Drops eslint-config-airbnb; bumps postcss version. |
| packages/modules/create-imodel/package.json | Drops eslint-config-airbnb; bumps postcss version. |
| packages/apps/storybook/package.json | Removes legacy Bentley deps and babel-eslint/eslint-config-airbnb. |
| common/scripts/package.json | Removes babel-eslint and eslint-config-airbnb from script deps. |
| common/config/rush/pnpm-config.json | Updates postcss override and adds several additional security-related overrides. |
| common/changes/@itwin/manage-versions-react/lk-fix-highest-cves_2026-08-05-22-00.json | Adds Rush change file (type none). |
| common/changes/@itwin/imodel-browser-react/lk-fix-highest-cves_2026-08-05-22-00.json | Adds Rush change file (type none). |
| common/changes/@itwin/delete-itwin-react/lk-fix-highest-cves_2026-08-05-22-00.json | Adds Rush change file (type none). |
| common/changes/@itwin/delete-imodel-react/lk-fix-highest-cves_2026-08-05-22-00.json | Adds Rush change file (type none). |
| common/changes/@itwin/create-imodel-react/lk-fix-highest-cves_2026-08-05-22-00.json | Adds Rush change file (type none). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
f262fdf to
9383b83
Compare
Remove two dead devDependency trees and pin the vulnerable transitive packages that have no fix reachable through a direct dependency. pnpm audit over the lockfile goes from 84 advisories (4 critical, 51 high) to 45 (0 critical, 25 high), with none newly introduced. - Drop eslint-config-airbnb from all eight projects. It is never extended by any .eslintrc, and was the sole source of lodash@3.10.1, json5@0.4.0 and minimatch@2.0.10. Drop babel-eslint alongside it. - Drop @svgr/rollup and its svgr() call from imodel-browser's rollup config. The repo contains no .svg files and the @stratakit/icons imports stay external, so it never transformed anything; it was the sole source of svgo@1.3.2 and nth-check@1.0.2. - Override form-data, flatted, lodash, picomatch and svgo, and alias xmldom to its maintained successor @xmldom/xmldom. - Move the existing postcss pin from 8.4.31 to 8.5.25. No consumer-visible contract changes: dependencies, peerDependencies, files, exports and version are untouched in all five published packages.
89f742a to
005fd5b
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Six of the security overrides were unnecessary. Every consumer's declared range already admitted the patched version -- form-data ^3.0.0 and ^4.0.0, flatted ^3.2.9, lodash ^4.0.0, picomatch ^2.0.4 through ^2.3.1, svgo ^2.7.0 -- so regenerating the lockfile reaches the same resolutions on its own and the overrides only restated what pnpm would pick anyway. Removing form-data@3, form-data@4, flatted, lodash, picomatch@2 and svgo@2 moves the lockfile by 8 lines, all of them the overrides block echoing the config; not one resolved version changes. Audited against the same live advisory data, both lockfiles report 47 vulnerabilities over an identical set of 37 GHSAs. The xmldom entry stays. It is an alias rather than a version bump, and no published xmldom is unaffected -- every version up to and including 0.6.0 is vulnerable -- so no lockfile refresh can resolve it. It is now pinned exactly instead of with a caret, matching the other three entries, so a security-driven override cannot drift.
Nine more package copies were vulnerable only because the lockfile held an older resolution than the declared ranges already allowed. Bumping the direct rollup floor and refreshing the rest clears them without any new override: - @babel/plugin-transform-modules-systemjs 7.25.9 -> 7.29.8, via @babel/preset-env 7.29.7 which requires the patched plugin - fast-uri 3.0.6 -> 3.1.5 (ajv wants ^3.0.1) - immutable 5.1.1 -> 5.1.9 (sass 1.86.3 wants ^5.0.2, so sass itself is untouched -- newer sass requires Node 20 and this repo still supports 18) - js-yaml 3.14.1 -> 3.15.1 and 4.1.0 -> 4.3.1 - minimatch 3.1.2 -> 3.1.5, leaving the already-clean 10.2.5 copy alone - rollup 2.79.2 -> 2.80.0, raising the direct devDependency floor from ^2.42.4, the only change here that touches a package manifest - serialize-javascript 6.0.2 removed outright by terser-webpack-plugin 5.6.1, which no longer depends on it - ws 8.18.1 -> 8.21.3 Audited against the same live advisory data, main reports 88 vulnerabilities across 71 GHSAs and this branch reports 22 across 17; 54 GHSAs are cleared and none introduced. No package gained a Node 20 engine requirement that main did not already have. brace-expansion is deliberately left alone: the patched 5.0.9 requires Node 20 or newer, and rush.json still declares support for ^18.12.0.
|
Good thinking about the remanining findings @ben-polinsky. I went on and did an additional pass, and this dropped the vulns to 22, so about 20 more vulns removed. Thanks! |
I have much experience with rush and cves thanks to itwinjs-core :) |
|
Just to recap: 8 remaining high vulns? |
Seems like it after my last audit yesterday 🙏🏻 |
Alright sorry to be a stickler, but looks like these should be relatively easy to resolve? Add Rush-wide pnpm overrides in common/config/rush/pnpm-config.json :
• serialize-javascript → 7.0.3
• tmp → 0.2.7
• brace-expansion 1.x → 1.1.18
• brace-expansion 5.x → 5.0.9 |
No worries, I'll check it out. I think I refrained about brace-expansion because it moved node's base version to 20, but I'll check again to be sure. |
No one should be using node 18.x. It (and node 20.x) are EOL (and 18.x has been EOL since Mar 27, 2025.) |
Two more overrides, both of which keep the repo's declared Node floor intact: - tmp 0.0.33 -> 0.2.7, reached through external-editor <- inquirer <- msw. Its only call site is tmpNameSync, which 0.2.x still provides, and 0.2.7 needs only node >=14.14. - brace-expansion 1.x -> 1.1.18, scoped so the 5.x copy is untouched. 1.1.18 declares no engines constraint. Unlike the resolutions refreshed in the previous commit, neither of these is reachable by regenerating the lockfile: external-editor pins tmp at ^0.0.33 and there is no patched 0.0.x, so an override is the only lever. Audit goes from 22 vulnerabilities across 17 GHSAs to 15 across 13. Published output stays byte-identical to main across all 200 files in the five packages. Two high findings are deliberately left for a follow-up because their patched versions require Node 20 or newer, and rush.json still declares ^18.12.0: serialize-javascript, whose 4.0.0 copy comes from the deprecated rollup-plugin-terser and whose fix needs 7.x (node >=20), and brace-expansion 5.x, whose 5.0.9 needs node 20 || >=22.
The two remaining high findings both need a patched version that requires Node 20 or newer, so this also raises the repo's supported Node floor: - serialize-javascript -> 7.1.0, clearing the RCE on the 4.0.0 copy that the deprecated rollup-plugin-terser pins at ^4.0.0. Note 7.0.3 is not enough: the CPU-exhaustion advisory covers >=5.0.0 <7.0.5. - brace-expansion 5.x -> 5.0.9, clearing three DoS advisories on 5.0.6. nodeSupportedVersionRange drops ^18.12.0 and becomes ^20.9.0 || ^22.11.0 || ^24.11.0. Node 18 has been end-of-life since April 2025, and this costs consumers nothing: none of the five published packages declares an engines field, and they are browser-targeted React components, so Node is only ever a build-time requirement here. All three workflows already run Node 20 or newer (publish 20.x, ci 24.x, deploy-storybook 20), and both new floors accept Node 20. Audit now reports 11 vulnerabilities across 9 GHSAs, down from 88 across 71 on main: 62 advisories cleared, none introduced, and no critical or high findings left. Published output remains byte-identical to main across all 200 files in the five packages.
If only it were true that no one IS using it 😅 . |
|
Here are the supported platforms for iTwin.js, FWIW: https://www.itwinjs.org/learning/supportedplatforms/ iTwin.js 4 - Node >=18.12 Perhaps a question for @aruniverse if it's ok to drop Node 18 support |
iTwin.js 4.x is at EOL:
https://www.itwinjs.org/learning/api-support-policies/#version-support-status |
|
Hey @aruniverse 👋🏻 |
|
Go ahead and drop node 18, thats been eol for years now. Also all the pkgs in here are client side, they shouldnt need node to begin with except for testing. You need an approval from @williamkbentley |
TL;DR
pnpm auditovercommon/config/rush/pnpm-lock.yamldrops from 88 vulnerabilities to 11, with zero newly introduced. No critical or high findings remain.Those are per-finding counts, so an advisory hitting two copies of a package counts twice. Deduplicated, 62 distinct GHSA advisories are eliminated (3 critical, 44 high, 13 moderate, 2 low), leaving 9.
Nothing published from this repo is affected either way — every one of these is a transitive dependency of a devDependency. The production closure of the five published packages is 34 distinct names and carries no advisories at all.
Solves AB#2119495
How
1. Delete devDependencies the repo no longer uses
eslint-config-airbnb(all 8 projects) — a 2013-era package never extended by any.eslintrc; the real chain iscommon/scripts/.eslintrc.ts.json->.eslintrc.ts.base.json. It was dragging ineslint@0.21.2,babel-core@5.8.38,lodash@3.10.1,json5@0.4.0andminimatch@2.0.10.babel-eslintgoes with it, also unused.@svgr/rollupand itssvgr()call inimodel-browser/rollup.config.js— the repo contains zero.svgfiles, and the only.svgimports are bare@stratakit/iconsspecifiers that stay external and are consumed as URL strings via<Icon href={...}>. Building with and without it produces byte-identical output, so it was transforming nothing. Sole source ofsvgo@1.3.2->nth-check@1.0.2.2. Refresh stale lockfile resolutions (no override needed)
Most findings needed no override: the declared ranges already admitted the patched versions and the lockfile was simply holding older resolutions.
form-datasuperagent^3.0.0,jsdom@20^4.0.0flattedflat-cache^3.2.9lodash^4.0.0picomatchanymatch^2.0.4,@rollup/pluginutils^2.2.2svgopostcss-svgo^2.7.0fast-uriajv^3.0.1immutablesass@1.86.3^5.0.2js-yaml^3.13.1,^4.1.0minimatchfork-ts-checker-webpack-plugin^3.0.4ws@babel/plugin-transform-modules-systemjs@babel/preset-env@7.29.7serialize-javascriptterser-webpack-plugin@5.6.1no longer depends on it3. Two manifest changes
rollupdirect devDependency floor^2.42.4->^2.80.0in the five published packages, the only way past 2.79.2.postcsspin8.4.31->8.5.25, inglobalOverridesand the five packages that pin it directly. Verified output-neutral: all 18 SCSS files compile to byte-identical CSS under both versions.4. Overrides that are genuinely required
None of these is reachable by regenerating the lockfile — in each case the parent pins a range with no patched release inside it:
xmldomis abandoned at 0.6.0 and every published version is vulnerable, so it is aliased to its maintained successor. Its only consumer is@bentley/itwin-client'sSamlToken.js, which the addon's OIDCBrowserAuthorizationClientpath never reaches; the alias is verified to return identical results for that file's exact xpath expressions.tmpis pinned at^0.0.33byexternal-editor(<-inquirer<-msw); no patched0.0.xexists. Its only call site istmpNameSync, which0.2.xstill provides.serialize-javascriptis pinned at^4.0.0by the deprecatedrollup-plugin-terser.7.0.3is not sufficient — the CPU-exhaustion advisory covers>=5.0.0 <7.0.5— hence7.1.0. Migrating to@rollup/plugin-terserwould remove the need for this override.brace-expansionis scoped per major so each line gets its own patched release.All are pinned exactly rather than with a caret, so a security override cannot drift.
Node 18
serialize-javascript@7.xrequiresnode >=20.0.0andbrace-expansion@5.0.9declaresnode: 20 || >=22, so clearing the last four high findings means dropping^18.12.0fromnodeSupportedVersionRange.This costs consumers nothing: none of the five published packages declares an
enginesfield, and they are browser-targeted React components, so Node is only ever a build-time requirement here. The change constrains contributors building this repo, not anyone installing the packages. Node 18 has been end-of-life since April 2025, and all three workflows already run Node 20 or newer (publish20.x,ci24.x,deploy-storybook20) — both new floors accept Node 20.Advisories removed (62)
form-data3.0.3,4.0.23.0.5/4.0.6lodash3.10.14.18.1xmldom0.1.31@xmldom/xmldom@0.8.13@babel/plugin-transform-modules-systemjs7.25.97.29.8brace-expansion1.1.11,5.0.61.1.18/5.0.9brace-expansion1.1.11,5.0.61.1.18/5.0.9brace-expansion1.1.11,5.0.61.1.18/5.0.9fast-uri3.0.63.1.5fast-uri3.0.63.1.5fast-uri3.0.63.1.5fast-uri3.0.63.1.5fast-uri3.0.63.1.5flatted3.3.33.4.4flatted3.3.33.4.4form-data3.0.3,4.0.23.0.5/4.0.6immutableList32-bit trie overflow → unrecoverable DoS5.1.15.1.9immutable5.1.15.1.9immutable5.1.15.1.9js-yaml3.14.1,4.1.03.15.1/4.3.1js-yaml3.14.1,4.1.03.15.1/4.3.1json50.4.0lodash3.10.14.18.1lodash3.10.14.18.1lodash3.10.14.18.1lodash_.templateimports key names4.17.214.18.1minimatch2.0.10,3.1.23.1.5minimatch2.0.10,3.1.23.1.5minimatch2.0.10,3.1.23.1.5minimatch2.0.103.1.5minimatch2.0.103.1.5nanoid3.3.113.3.11nanoid3.3.113.3.11nth-check1.0.2picomatch2.3.12.3.2postcss8.4.318.5.25postcss8.4.318.5.25rollup2.79.22.80.0serialize-javascript4.0.0,6.0.27.1.0svgo1.3.2,2.8.02.8.3svgo2.8.02.8.3tmp0.0.330.2.7ws8.18.18.21.3xmldom0.1.31@xmldom/xmldom@0.8.13xmldom0.1.31@xmldom/xmldom@0.8.13xmldom0.1.31@xmldom/xmldom@0.8.13xmldom0.1.31@xmldom/xmldom@0.8.13xmldom0.1.31@xmldom/xmldom@0.8.13brace-expansion1.1.111.1.18/5.0.9js-yaml3.14.1,4.1.03.15.1/4.3.1js-yaml3.14.1,4.1.03.15.1/4.3.1lodash_.unsetand_.omit3.10.1,4.17.214.18.1lodash3.10.14.18.1lodash_.unsetand_.omitfunctions4.17.214.18.1picomatch2.3.12.3.2postcssfromis unset8.4.318.5.25postcss8.4.318.5.25serialize-javascript6.0.27.1.0ws8.18.18.21.3xmldom0.1.31@xmldom/xmldom@0.8.13xmldom0.1.31@xmldom/xmldom@0.8.13brace-expansion1.1.111.1.18/5.0.9tmpdirparameter0.0.330.2.7Verification
rush buildandrush test: 193 tests, 0 failures; Storybook static build succeedsrush change -vpasseseslint-config-airbnbgone: 0 errorsmain's across all five packages — 200 files, every SHA-256 matching, built frommain's dependency set and from this branch's. That holds even though the bundler itself moved (rollup2.79.2 -> 2.80.0) alongside the babel and postcss bumps, so consumers receive exactly the bytes they do today. Additionally smoke-tested by overriding Studio's copy with this build and running the Visualizer.dependencies,peerDependencies,files,exportsandversionunchanged in all five published packages — onlydevDependenciesmovedNot addressed (9)
ajv6.12.6copyqsyaml1.10.2via tooling, no in-range fixcookiemsw@0.27; needs anmswmajor upgradewebpackbuildHttpallow-list bypass; that feature is not used@babel/coresourceMappingURLfile read in a build-only toolAll 9 are dev-only — none appears in the production closure, so none reaches a consumer of these packages.