Fix: bump adm-zip to ^0.6.0 to clear GHSA-xcpc-8h2w-3j85 (CVE-2026-39244) - #663
Open
AmaadMartin wants to merge 1 commit into
Open
Fix: bump adm-zip to ^0.6.0 to clear GHSA-xcpc-8h2w-3j85 (CVE-2026-39244)#663AmaadMartin wants to merge 1 commit into
AmaadMartin wants to merge 1 commit into
Conversation
adm-zip < 0.6.0 is affected by GHSA-xcpc-8h2w-3j85 (CVE-2026-39244, High, CVSS 7.5): a crafted archive can declare a ~4GB uncompressed size that adm-zip allocated eagerly, before decompression and CRC validation. The fix is only available in adm-zip 0.6.0, a semver-major release that sits outside the declared ^0.5.17 range, so downstream consumers of @google/adk cannot reach it through resolution or overrides. Both manifests that declare adm-zip are bumped: core (runtime dependency, shipped) and dev (build-time devDependency). No source changes are required. The API surface used by core/src/skills/loader.ts (new AdmZip(buffer), getEntries, entryName, isDirectory, getData) and dev/build.js (new AdmZip(path), extractAllTo) is unchanged in 0.6.0. Fixes google#601
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.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
Closes: adm-zip range ^0.5.17 pins consumers to a version with an unpatched DoS (GHSA-xcpc-8h2w-3j85) google/adk-js#601
Related: adm-zip range ^0.5.17 pins consumers to a version with an unpatched DoS (GHSA-xcpc-8h2w-3j85) google/adk-js#601
Problem: Every published
@google/adkresolvesadm-zipto a version affected byGHSA-xcpc-8h2w-3j85 / CVE-2026-39244
— "Crafted ZIP file triggers 4GB memory allocation", CWE-400 + CWE-789, High, CVSS 3.1
score 7.5 (
AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:H).adm-zip'szipEntry.jscalledBuffer.alloc(_centralHeader.size)using the attacker-controlled uncompressed-size fieldfrom the ZIP central directory, before decompressing and before CRC validation, so a
~120-byte archive can force a ~4 GB allocation and crash the process.
The advisory range is
<0.6.0, and the fix only exists inadm-zip@0.6.0— asemver-major release that is unreachable from the
^0.5.17range this repo declared.That is why downstream consumers could not remediate it themselves:
overrides(flat andnested),
npm update,npm dedupeand a from-scratch lockfile resolve all leave0.5.xin the tree when the dependency crosses a workspace edge. The publisher has to bump it.
Solution: Declare
"adm-zip": "^0.6.0"in both manifests that require it andregenerate the lockfile. Nothing else changes.
core/package.json—dependencies(runtime, shipped in@google/adk; this is theedge that reaches consumers).
dev/package.json—devDependencies(build-time only, used bydev/build.js).Both are needed: leaving either behind keeps a
<0.6.0requirement in the workspace, andnpm would resolve a second, still-vulnerable
adm-zipnode to satisfy it — reintroducingthe advisory.
adm-zipstays in the section it was already in; neither declaration moved.Compatibility evidence — no source adaptation was required.
adm-ziphas exactly twocall sites in the repo (
grep -rn "adm-zip\|AdmZip" --include='*.ts' --include='*.js'):core/src/skills/loader.ts(loadSkillFromZipBuffer)new AdmZip(buffer),getEntries(),entry.entryName,entry.isDirectory,entry.getData()dev/build.js(unzipFile)new AdmZip(path),zip.extractAllTo(dir, true)Two test files also build fixtures with
new AdmZip(),addFile(),toBuffer()andassignment to
entry.entryName; all still behave identically.0.6.0additionally shipsits own
types.d.ts, and both call sites typecheck against it.Behavioural deltas in 0.6.0 that could have mattered, and why they do not here: CRC
validation for data-descriptor archives now trusts the central-directory CRC instead of
throwing
DESCRIPTOR_*(strictly more archives load; nothing in this repo asserts on thoseerrors), the entry table is now built with
Object.create(null)(prototype-pollutionhardening — a free win for the untrusted skill-loading path),
extractAllTono longeraborts when
utimesSyncfails (strictly more forgiving for the build script), andengines.nodemoves>=12.0→>=14.0(no effect:dev/build.jsalready targetsnode16).Reachability, stated honestly: the vulnerable path is only reached by consumers that
load skill bundles through
loadSkillFromZipBuffer, or by the build script unpacking theADK Web release archive. For most users this is supply-chain hygiene — a
npm auditfindingthey cannot clear — rather than a live exploit. That lowers the urgency; it does not remove
the need to fix, since the publisher is the only party who can.
Not a breaking change for
@google/adk.adm-zipis an internal implementationdetail: it is not re-exported and appears in no public type of
@google/adk. Asemver-major bump of an internal dependency is not a semver-major event for this package.
Collision check (required before implementing):
gh pr list --repo AmaadMartin/adk-js --state open --limit 1000returned 561 open PRs; none bumpsadm-zip. The three thattouch adjacent ground were inspected with
gh pr diff --name-only: #183 and #345 bothremove a stale
"dev": trueflag from thenode_modules/adm-ziplockfile entry — thatflag is already absent on current
main, so their hunk no longer applies and does notoverlap these three; #567 edits
dev/package.jsonto declare@types/js-yaml, a differentkey. No stacking was needed.
Deliberately out of scope (each would enlarge a security diff or conflict on the
lockfile): removing the now-redundant
@types/adm-zipdevDependency, the"dev"-flaglockfile hygiene above,
npm audit fix(the tree has 44 other advisories), any otherdependency bump, an
enginesfield, and a hand-written zip-bomb regression test — thatlast one would have to byte-patch the ZIP central-directory size field, coupling an adk-js
test to
adm-zipinternals for no signal the version pin does not already give.Testing Plan
Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.
Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.
No source lines changed (
git diff --statiscore/package.json | 2 +-,dev/package.json | 2 +-,package-lock.json | 12 +++---), so there is no new code tocover and therefore no new tests and no coverage delta — the repo-wide V8 thresholds in
vitest.config.ts(statements 86, branches 87, functions 88, lines 86) are untouchedbecause no
core/src/**ordev/src/**line moved. No existing test file was modified.The evidence that the bump is inert is the existing suites that perform a real in-memory
zip round trip through
adm-zip@0.6.0:npx vitest run --project unit:core core/test/skills/loader_test.tsnpx vitest run --project unit:core core/test/tools/skills/skill_registry_test.tsnpx vitest run --project unit:core(whole workspace)npx vitest run --project unit:devThe load-bearing cases inside
describe('loadSkillFromZipBuffer'):loads a benign archive with all resource trees— buildsSKILL.md+references/+assets/+scripts/withaddFile()+toBuffer(), reads it back and asserts theparsed frontmatter, instructions and all three resource maps.
rejects the whole archive for the dangerous entry %s(/etc/passwd,../evil.txt,references/../../esc.txt) — these assignentry.entryNameafteraddFile()(whichwould otherwise canonicalise the name away) and then
toBuffer(). They assert the thrownmessage contains the byte-exact name, so their passing is direct proof that 0.6.0 still
preserves a hand-written
entryNamethrough the write/parse round trip. Had 0.6.0changed name handling, this is where it would surface.
reports the dangerous entry even when SKILL.md is absent, andrejects the non-bare skill name %s(../evil,a/b,..) — the zip-slip and bare-nameguards live in adk-js (
isDangerousZipEntryName,isBareSkillName), not inadm-zip,and are unperturbed.
Proof the tests can fail (mutation testing against
adm-zip@0.6.0; both mutations werereverted and the working tree verified clean afterwards):
isDangerousZipEntryNamereplaced withreturn entryName === 'MUTATION_NEVER_MATCHES';→ 4 failures, e.g.AssertionError: expected [Function] to throw error including 'Dangerous zip entry ignored: ../evil.…' but got 'SKILL.md not found in zipped filesyst…'.const data = entry.getData();replaced withconst data = Buffer.from('MUTATED');inloadZipDir→ 2 failures across bothfiles:
AssertionError: expected 'MUTATED' to be 'ref content'.npm run ts:checkis not green on this branch, and is not green onmaineither — itreports 281 pre-existing errors of the form
core/dist/types/... is not assignable to core/src/...(duplicate type identity between thebuilt
core/distdeclarations andcore/src). Proven unrelated: swappingnode_modules/adm-zipback to0.5.17and re-running produces a byte-identical 281-errorset (
diffof the sorted error lines reports no difference).ts:checkis not part of theCI workflow. Both
adm-zipcall sites compile cleanly.One
unit:devcase,cli_create_test.ts > should handle Vertex AI selection with gcloud defaults, fails on a workstation that exportsGOOGLE_CLOUD_PROJECT/GOOGLE_CLOUD_LOCATION: the test mocksexecSyncbutcreateAgentprefers the realenvironment, so the assertion sees the developer's own project instead of
gcloud-project.Under
env -u GOOGLE_CLOUD_PROJECT -u GOOGLE_CLOUD_LOCATIONall 223 pass. That is anenvironment leak in an unrelated test, not a regression from this bump, and is left alone
here.
Manual End-to-End (E2E) Tests:
integritywas cross-checked againstnpm view adm-zip@0.6.0 dist.integrityand matches(
sha512-XleryMhbuksdKtofnWZ9Sk+…). Exactly one hoisted node, no duplicate tree, and theentry still carries no
"dev"flag (correct —coreneeds it at runtime).npm auditreads the lockfile; the acceptance criterion):one thing it claims to and nothing else.
npm run build— exercises the second call site for real.dev/build.jsunzipFile()is the only use of the path-string constructor and ofextractAllToin therepo. It downloaded the ADK Web v1.0.0 release archive and unpacked it with 0.6.0:
dev/dist/browser,.versionreadsv1.0.0.npm run lint,npm run format:checkandnpm run docs:checkall exit 0.CI:
run-testsis green on ubuntu-latest, macos-latest and windows-latest. The firstwindows run failed on
core/test/code_executors/unsafe_local_code_executor_test.ts > should execute shell code and return stdoutwithError: Test timed out in 5000ms— a known windows-only timing flake ina test unrelated to zip handling (the same commit passed on the other two runners, and 2689
of 2690 tests passed on windows). It passed on re-run with no code change.
Checklist
[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.