Override adm-zip to 0.6.0 to clear GHSA-xcpc-8h2w-3j85 - #2243
Merged
Conversation
`onnxruntime-node`, added in #2235, depends on `adm-zip: ^0.5.16`, and the fix for GHSA-xcpc-8h2w-3j85 landed in 0.6.0. That range cannot reach it, so npm keeps resolving a vulnerable version and Dependabot keeps flagging it. An override pulls 0.6.0 in anyway. `onnxruntime-node` only uses `new AdmZip(path)` and `getEntry`, which are unchanged in 0.6.0, and its Node requirement moves from >=12 to >=14, which this project already exceeds. npm audit goes from 2 high advisories to none.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Clears Dependabot alert #32 (GHSA-xcpc-8h2w-3j85, high, "crafted ZIP file triggers 4GB memory allocation") by overriding
adm-zipto 0.6.0.onnxruntime-node, added in #2235, declaresadm-zip: ^0.5.16. The advisory is fixed in 0.6.0, which that range cannot reach, so npm keeps resolving 0.5.18 and the alert cannot be closed by a normal dependency bump. Anoverridesentry pulls 0.6.0 in anyway.npm auditgoes from 2 high advisories to zero.Why the override is safe
onnxruntime-nodeusesadm-zipin exactly one place,script/install-utils.js, and only throughnew AdmZip(packageFilePath)andzip.getEntry(...). Both behave the same in 0.6.0, verified directly:The only other change is the engine requirement moving from
node >=12tonode >=14, and this project already requiresnode >=22.The override is unscoped rather than nested under
onnxruntime-node, so it keeps applying if another dependency pullsadm-zipin later.onnxruntime-nodeis the only consumer today.Exposure, for context
Worth knowing while assessing urgency: this code path is not reached by a normal install.
adm-zipis used byonnxruntime-node's postinstall script to unpack binaries that are not bundled in the npm package, chiefly the CUDA execution provider. On a default CPU install the script exits early and never opens an archive, and the archive it would open comes from Microsoft's NuGet feed rather than from user input.So this is housekeeping to get the alert closed and keep
npm auditclean, not an exploitable path in MiniSearch. The proper long-term fix is upstream relaxing the range inonnxruntime-node; the override can be dropped once that ships.Type of Change
How to test
npm install, thennpm ls adm-zipshowsadm-zip@0.6.0underonnxruntime-node@1.27.0.npm auditreportsfound 0 vulnerabilities(2 high before this change).npm run testpasses (288 tests) andnpm run lintpasses.npx vitest run --config vitest.integration.config.tspasses (6 tests), which loads the real ONNX model and so provesonnxruntime-nodestill works under the override.I could not test the CUDA install path, since that needs Linux x64 with an NVIDIA GPU. It is the only place
adm-zipactually runs, so the API check quoted above stands in for it.Checklist
npm run lintpassesnpm run test), with tests added where it made senseNo new tests: the change is a dependency resolution constraint, and the existing integration test already covers the consumer still working.
Security, performance, or breaking changes
Security: closes one high-severity advisory. No behavior change for users, and nothing to migrate.