Skip to content

Test: pin both halves of the zip-slip entry-name contract - #762

Open
AmaadMartin wants to merge 1 commit into
mainfrom
fix/skills-zip-entry-name-contract-tests
Open

Test: pin both halves of the zip-slip entry-name contract#762
AmaadMartin wants to merge 1 commit into
mainfrom
fix/skills-zip-entry-name-contract-tests

Conversation

@AmaadMartin

Copy link
Copy Markdown
Owner

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):

Follow-up to google#621 (the zip-slip fix this pins). Original port: google#584.

  1. Or, if no issue exists, describe the change:

Problem: The isDangerousZipEntryName suite asserts only rejection. Nothing asserts that a member name which merely contains .. inside a segment is still accepted, so an over-broad change stays green while it breaks archives with a benign dot-prefixed member. The docstring also still says the function mirrors adk-python's _load_skill_from_zip_bytes, which stopped being true at #621.

Solution: I pinned the accepted half of the contract with three new cases, and added the two bare dangerous shapes the committed list only covered in prefixed form. I rewrote the docstring to name the divergence and its reason, in the house style of the neighbouring isBareSkillName comment. No runtime behaviour changes: the source diff is comment lines only.

Notes for the reviewer:

  • Parity rule applied. Parity wins for anything observable across the boundary; here the divergence is a rejection set, and adk-js rejects a strict superset of what adk-python rejects. No archive that adk-python accepts and extracts safely is broken by adk-js. adk-python is out of scope for this PR.
  • The .. cases were appended to the existing it.each array. The assertion body, the test name and all five existing entries are unchanged, so no regression signal is lost.
  • I did not add a/../../evil.txt. The committed references/../../esc.txt is the same shape and exercises no new branch.
  • isDangerousZipEntryName stays module-private. The new tests drive it through loadSkillFromZipBuffer, as the existing cases do.
  • Base sync. This fork's main was 13 commits behind google/adk-js main and did not contain Fix: reject a non-object Reasoning Engine raw body instead of crashing the dev server #621, so the new cases could not pass. I fast-forwarded the fork's main to upstream and rebased. The PR diff is two files.
  • Collision check. gh pr list --repo AmaadMartin/adk-js --state open --limit 100 plus gh pr diff --name-only on every skills, zip and loader PR. Only Fix: key directory-loaded skill resources by POSIX relative paths #728 touches these two files. It rewrites resource keys in loadDir and adds tests in the loadSkillFromDir block; this PR touches isDangerousZipEntryName and the loadSkillFromZipBuffer block. There is no dependency between them, and Fix: key directory-loaded skill resources by POSIX relative paths #728 is based on a main that predates Fix: reject a non-object Reasoning Engine raw body instead of crashing the dev server #621, so I did not stack on it.

Testing Plan

Unit Tests:

  • I have added or updated unit tests for my change.
  • All unit tests pass locally.

npx vitest run --project unit:core core/test/skills/loader_test.ts — 52 passed, up from 47 on main.

Mutation proof. I ran each mutation against core/src/skills/loader.ts and confirmed the expected tests turn red.

# Mutation Result
1 segment === '..' -> segment.includes('..') 3 failed, 49 passed. The three new benign cases fail with Error: Dangerous zip entry ignored: references/..hidden.txt, ... assets/v1..2.bin, ... scripts/a..b.sh.
2 split(/[/\\]/) -> split('/') 2 failed, 50 passed: new case '..\evil.txt' and existing 'scripts\..\..\pwned.txt', both AssertionError: expected [Function] to throw an error.
3 some((segment) => segment === '..') -> startsWith('../') 5 failed, 47 passed, including the new case '..', all AssertionError: expected [Function] to throw an error.

Mutation 1 is the regression the new negative test exists to catch. It is green against main today.

Manual End-to-End (E2E) Tests:

No E2E surface. The change is a comment plus unit cases against an in-memory archive.

Other local checks on the pushed commit:

  • npm run build — passes.
  • npm run lint — clean.
  • npm run format:check — clean.
  • git diff main -- core/src/skills/loader.ts — every added and removed line is a comment line.

Checklist

  • I have read the CONTRIBUTING.md document.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.

The suite only asserted rejection. A change that swapped the exact-segment
test for a substring test stayed green while it broke every archive with a
benign dot-prefixed member such as references/..hidden.txt.

Add three cases that assert such a member still loads, and add the two bare
dangerous shapes ('..' and a leading Windows traversal) that the committed
list only covered in prefixed form.

Correct the docstring: adk-js no longer mirrors adk-python's
_load_skill_from_zip_bytes, it is deliberately stricter.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant