Fix: reject zip-slip entries and non-bare skill names when loading zipped skills (adk-python parity)

[AmaadMartin]

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

1. Link to an existing issue (if applicable):
   No existing issue.
2. Or, if no issue exists, describe the change:
   Problem: loadSkillFromZipBuffer() (core/src/skills/loader.ts) is the trust boundary for Agent Skills downloaded from a remote registry — GCPSkillRegistry.getSkill() base64-decodes an API response and hands the raw bytes straight in, so the archive is authored by whoever published the skill, not by the user running the agent. It read SKILL.md and the references/ / assets/ / scripts/ trees out of the archive with no validation of member names, and never checked that the skill name was a bare path segment. The adk-python counterpart, _load_skill_from_zip_bytes in src/google/adk/skills/_utils.py, has both guards, so the identical archive was rejected by adk-python and accepted by adk-js.

entryName is not sanitised on read — verified empirically against the pinned adm-zip@0.5.17 (core/package.json): a zip written with the members ../evil.txt, /abs.txt, references/../../esc.txt round-trips through new AdmZip(buffer).getEntries() with those exact strings intact. The resource maps are keyed by relative path, so loadZipDir('references') turns a member named references/../../esc.txt into the map key ../../esc.txt, which escapes for any caller that writes Skill.resources to disk.

Solution: Port both adk-python guards into loadSkillFromZipBuffer, with the error strings copied character-for-character.

Condition (checked in this order)	Message
Any member name absolute / ../-prefixed / containing /../	Dangerous zip entry ignored: <entryName>
No SKILL.md (unchanged)	SKILL.md not found in zipped filesystem.
Frontmatter name missing or falsy	SKILL.md frontmatter must contain 'name'
name not a string, or not a bare path segment	Invalid skill name in SKILL.md: <name>

Three things worth calling out explicitly:

(a) Which strings were copied verbatim, and why one of them reads oddly. All four messages above are taken verbatim from src/google/adk/skills/_utils.py, including Dangerous zip entry ignored: — the word "ignored" is misleading (both implementations reject the entire archive rather than skipping the entry; adk-python raises at _utils.py, it does not skip-and-warn), but parity on the observable string wins over nicer prose, so it is not reworded.

(b) The name guard has to run before schema validation, which is why parseSkillMdContent was split. parseSkillMdContent() validated with zod internally and wrapped every failure as Invalid YAML in frontmatter: …, so a name of ../evil never reached a check placed after it — a guard added downstream would be unreachable dead code. It is now split into two module-private helpers mirroring the Python structure (_parse_skill_md_content returns an unvalidated dict; Frontmatter.model_validate runs after the name checks): parseFrontmatterYaml() returns the raw mapping, validateFrontmatter() applies FrontmatterSchema. parseSkillMdContent's exported signature, return value and every one of its error strings are unchanged — the five existing tests asserting them are untouched and still pass. The only behavioural addition inside it is Array.isArray(parsed) in the mapping check, needed so raw['name'] really is read off a mapping (it also matches Python's isinstance(parsed, dict)); it is thrown from inside the same try, so it still surfaces composed as Invalid YAML in frontmatter: SKILL.md frontmatter must be a YAML mapping.

(c) path.basename needs an explicit ./.. case to match pathlib. path.basename('..') === '..' whereas pathlib.Path('..').name === '', so a naive path.basename(name) === name check accepts .. where adk-python rejects it. isBareSkillName rejects . and .. explicitly. There is a test pinning exactly this (case name: ..), and it fails against the naive version — see the mutation log below.

Honest scope note on guard #2. I verified against the built schema that FrontmatterSchema already rejects every name this guard rejects (../evil, a/b, .., ., /etc/passwd, 123, missing) because SNAKE_OR_KEBAB_NAME_PATTERN admits only [a-z0-9] with single - or _. So guard #2 does not change which archives are accepted today; it changes the observable error message from a 150-character zod dump prefixed Invalid YAML in frontmatter: (which blames YAML for something that is not a YAML problem) to adk-python's string. It is kept as an independent check because that regex is a naming-style rule, not a traversal defence: if it is ever relaxed (dots for versioned names, uppercase, …) the traversal property would disappear silently. Guard #1 (zip slip) is not redundant — nothing else in the file rejects a traversal member name.

Breaking change: intentional and the point of the change. An archive whose SKILL.md name contains a path separator or is ./.. previously threw Invalid YAML in frontmatter: <zod dump> and now throws Invalid skill name in SKILL.md: <name>. No test asserted the old string. Archives with traversal member names previously loaded and now throw. Exported API surface, loadSkillFromDir, validateSkillDir, loadAllSkillsInDir and GCPSkillRegistry's signature are all unchanged; no new exports, no dependency or lockfile changes.

Collision check (run before writing any code, per pipeline policy): gh pr list --repo AmaadMartin/adk-js --state open --limit 100 returned 13 open skill-related PRs; I diffed the six that touch core/src/skills/loader.ts (#282, #262, #239, #284, #263, #242). None implements either guard, so this is not a duplicate. Three of them overlap textually: #239 and #262 remove the redundant second FrontmatterSchema.parse(...) in loadSkillFromZipBuffer (which disappears here too, as a consequence of calling validateFrontmatter(raw) directly), and #282 restructures the same try block in parseSkillMdContent. I branched from main rather than stacking because those three are mutually conflicting siblings — stacking on any one of them would be arbitrary and would pull an unrelated, unreviewed change into this security fix's base. Whichever lands first, the other diffs are a small textual rebase. I deliberately left loadSkillFile's identical double-parse alone for the same reason (it is claimed by #239/#262), and did not touch ALLOWED_FRONTMATTER_KEYS or validateSkillDir, which other siblings are editing.

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.

12 new cases in a describe('loadSkillFromZipBuffer', …) block in core/test/skills/loader_test.ts: benign archive loads all three resource maps; the three dangerous member names (/etc/passwd, ../evil.txt, references/../../esc.txt); a dangerous member in an archive with no SKILL.md (pins that the zip-slip loop runs before the SKILL.md lookup, matching _utils.py); the three non-bare names (../evil, a/b, ..); a non-string name (123); a missing name; a YAML-sequence frontmatter; and a regression guard that a benign archive without SKILL.md still reports SKILL.md not found in zipped filesystem..

Fixture note: AdmZip.addFile() canonicalises the name it is given ('../evil.txt' is stored as 'evil.txt'), so a test written the obvious way builds a benign archive and passes for the wrong reason. The helper adds a placeholder member and reassigns entryName afterwards, which round-trips through toBuffer() intact.

Commands run (targeted only, no full-suite run):

npx vitest run --project unit:core core/test/skills/loader_test.ts                     # 42 passed
npx vitest run --project unit:core core/test/tools/skills/skill_registry_test.ts       # 35 passed (untouched, still green)
npm run build                                                                          # clean
npm run docs:check                                                                     # clean (typedoc, warnings-as-errors)
npx tsc --noEmit                                                                       # no errors in either touched file
npx eslint core/src/skills/loader.ts core/test/skills/loader_test.ts                   # clean
npx prettier --check core/src/skills/loader.ts core/test/skills/loader_test.ts         # clean

Coverage: 100% line and branch coverage of the new code, measured with --coverage.include='core/src/skills/loader.ts' and checked per-line against the coverage JSON — every uncovered line in the file is pre-existing code the change does not touch (the loadDir walk, validateSkillDir's catch, and the dead catch around data.toString('utf-8') in loadZipDir). No coverage thresholds were changed and no coverage-tool suppressions were added.

Proof the tests can fail. Each new guard was mutated in core/src/skills/loader.ts and the suite re-run; every mutation was killed:

Mutation	Result
Delete the zip-slip loop entirely	4 failed — e.g. rejects the whole archive for the dangerous entry ../evil.txt: expected [Function] to throw an error; and reports the dangerous entry even when SKILL.md is absent: expected … 'Dangerous zip entry ignored: ../evil.…' but got 'SKILL.md not found in zipped filesyst…' (this one also pins the guard ordering)
isBareSkillName → naive path.basename(name) === name	1 failed — rejects the non-bare skill name ..: expected … 'Invalid skill name in SKILL.md: ..' but got 'Invalid YAML in frontmatter: [\n {\n…'
Delete both name guards	5 failed — all four name cases plus rejects frontmatter with no name: expected … 'SKILL.md frontmatter must contain 'n…' but got 'Invalid YAML in frontmatter: [\n {\n…'
Drop the typeof skillName !== 'string' disjunct	1 failed — rejects a skill name that is not a string: expected … 'Invalid skill name in SKILL.md: 123' but got 'Invalid YAML in frontmatter: [\n {\n…'
Drop Array.isArray(parsed) from the mapping check	1 failed — reports a YAML sequence as a non-mapping, not as a missing name: expected … 'Invalid YAML in frontmatter: SKILL.md…' but got 'SKILL.md frontmatter must contain 'n…'

Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.

No credentials or network needed. From the repo root, after npm ci && npm run build, run a scratch ESM script that imports from the public entry point:

import AdmZip from 'adm-zip';
import {loadSkillFromZipBuffer} from '@google/adk';

const skillMd =
  '---\nname: test-skill\ndescription: A test skill\n---\nInstruction body';

// (1) hostile archive: entryName must be reassigned, addFile() would sanitise it
const zip = new AdmZip();
zip.addFile('SKILL.md', Buffer.from(skillMd, 'utf-8'));
zip.addFile('placeholder.txt', Buffer.from('x', 'utf-8'));
zip.getEntries().find((e) => e.entryName === 'placeholder.txt').entryName =
  '../evil.txt';
loadSkillFromZipBuffer(zip.toBuffer()); // throws: Dangerous zip entry ignored: ../evil.txt

// (2) benign archive still loads all three resource maps
const ok = new AdmZip();
ok.addFile('SKILL.md', Buffer.from(skillMd, 'utf-8'));
ok.addFile('references/ref1.md', Buffer.from('ref content', 'utf-8'));
ok.addFile('assets/asset1.txt', Buffer.from('asset content', 'utf-8'));
ok.addFile('scripts/run.sh', Buffer.from('echo hello', 'utf-8'));
console.log(loadSkillFromZipBuffer(ok.toBuffer()).resources);

Observed output against the built package, matching adk-python's strings exactly:

throws: "Dangerous zip entry ignored: ../evil.txt"
throws: "Dangerous zip entry ignored: /etc/passwd"
throws: "Dangerous zip entry ignored: references/../../esc.txt"
throws: "Invalid skill name in SKILL.md: ../evil"
throws: "Invalid skill name in SKILL.md: a/b"
throws: "Invalid skill name in SKILL.md: .."
throws: "Invalid skill name in SKILL.md: 123"
benign: test-skill {"references":{"ref1.md":"ref content"},"assets":{"asset1.txt":"asset content"},"scripts":{"run.sh":{"src":"echo hello"}}}

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.

---

CI note: the first validation run failed on run-tests (macos-latest) with Error: Test timed out in 40000ms in tests/integration/app_loader/app_loader_test.ts > should discover apps vs agents across directories and standalone files (2521 passed, 1 failed), which cancelled the other two matrix legs. That is the known cold-start AgentLoader discovery flake already being addressed by open PRs on this fork (#260, #256, #247, #235) and is unrelated to this change — no skills test was involved. A re-run of the failed jobs passed on all three platforms: run-tests (ubuntu-latest) pass 5m27s, run-tests (macos-latest) pass 5m50s, run-tests (windows-latest) pass 8m1s.