From 65b7f66a5bcd9b04758709e24801a056c9fa2d32 Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Wed, 12 Aug 2026 23:17:10 +0200 Subject: [PATCH] verify-action-build: detect the tarball root instead of assuming package/ DefinitelyTyped roots @types/* tarballs at the bare package name (estree/index.d.ts), not package/. _tarball_files stripped only a literal "package/", so no extracted path lined up with node_modules/@types//, nothing was accounted for, and every file of every vendored @types package was reported as injected code -- even though the tarball had already passed integrity verification. Surfaced by #1171 (github-pages-deploy-action v4.9.0), whose yarn -> npm migration added node_modules/.package-lock.json and so switched this check on for the first time. With the fix that commit verifies clean: 116 packages, 0 extra. Generated-by: Claude Code (Opus 5) --- .../test_npm_registry_verify.py | 83 ++++++++++++++++++- .../npm_registry_verify.py | 29 +++++-- 2 files changed, 101 insertions(+), 11 deletions(-) diff --git a/utils/tests/verify_action_build/test_npm_registry_verify.py b/utils/tests/verify_action_build/test_npm_registry_verify.py index 6c6660a3..dab9b77d 100644 --- a/utils/tests/verify_action_build/test_npm_registry_verify.py +++ b/utils/tests/verify_action_build/test_npm_registry_verify.py @@ -36,12 +36,17 @@ ) -def _make_tgz(files: dict[str, bytes]) -> bytes: - """Build an npm-style ``.tgz`` (everything under ``package/``).""" +def _make_tgz(files: dict[str, bytes], root: str = "package") -> bytes: + """Build an npm-style ``.tgz``. + + ``root`` is ``package`` by convention; DefinitelyTyped publishes under + the bare package name instead, so it is parameterised. Pass ``""`` to + emit member names verbatim. + """ buf = io.BytesIO() with tarfile.open(fileobj=buf, mode="w:gz") as tf: for rel, content in files.items(): - info = tarfile.TarInfo(name=f"package/{rel}") + info = tarfile.TarInfo(name=f"{root}/{rel}" if root else rel) info.size = len(content) tf.addfile(info, io.BytesIO(content)) return buf.getvalue() @@ -60,6 +65,41 @@ def _integrity(data: bytes) -> str: PKG_TGZ = _make_tgz(PKG_FILES) PKG_URL = "https://registry.npmjs.org/foo/-/foo-1.0.0.tgz" +# DefinitelyTyped roots its tarballs at the bare package name rather than +# ``package/`` — this is the real @types/estree@1.0.9 member layout. +TYPES_FILES = { + "LICENSE": b"MIT License\n", + "README.md": b"# Installation\n", + "flow.d.ts": b"// flow types\n", + "index.d.ts": b"export interface Node {}\n", + "package.json": b'{"name":"@types/estree","version":"1.0.9"}\n', +} +TYPES_TGZ = _make_tgz(TYPES_FILES, root="estree") +TYPES_URL = "https://registry.npmjs.org/@types/estree/-/estree-1.0.9.tgz" + + +def _types_tree(extra: dict[str, str] | None = None) -> dict[str, str]: + tree = {"node_modules/.package-lock.json": "abc123"} + for rel, content in TYPES_FILES.items(): + tree[f"node_modules/@types/estree/{rel}"] = _git_blob_sha1(content) + if extra: + tree.update(extra) + return tree + + +def _types_lock() -> bytes: + return json.dumps({ + "lockfileVersion": 3, + "packages": { + "": {"name": "root"}, + "node_modules/@types/estree": { + "version": "1.0.9", + "resolved": TYPES_URL, + "integrity": _integrity(TYPES_TGZ), + }, + }, + }).encode() + def _tree_for(files: dict[str, bytes], extra: dict[str, str] | None = None) -> dict[str, str]: tree = {"node_modules/.package-lock.json": "abc123"} @@ -124,6 +164,24 @@ def test_tarball_files_strips_package_prefix(self): out = _tarball_files(PKG_TGZ) assert out == PKG_FILES + def test_tarball_files_strips_definitelytyped_bare_name_root(self): + # @types/* tarballs root at the bare package name; assuming + # "package/" left every path prefixed with the root, so none of + # them matched node_modules/@types//. + assert _tarball_files(TYPES_TGZ) == TYPES_FILES + + def test_tarball_files_strips_dot_slash_prefixed_root(self): + assert _tarball_files(_make_tgz(PKG_FILES, root="./package")) == PKG_FILES + + def test_tarball_files_leaves_multi_root_tarball_untouched(self): + # No single shared root → nothing is safe to strip. + files = {"a/one.js": b"1\n", "b/two.js": b"2\n"} + assert _tarball_files(_make_tgz(files, root="")) == files + + def test_tarball_files_leaves_root_level_files_untouched(self): + files = {"one.js": b"1\n"} + assert _tarball_files(_make_tgz(files, root="")) == files + def test_strip_npm_install_metadata_drops_underscore_keys(self): # npm's install bookkeeping is _-prefixed by convention; the exact # set has varied across npm versions, so match the prefix. @@ -254,6 +312,25 @@ def test_extra_file_in_verified_package_fails(self): assert result.ok is False assert "node_modules/foo/sneaky.js" in result.extra + def test_definitelytyped_package_verifies_clean(self): + # apache/infrastructure-actions#1171: github-pages-deploy-action v4.9.0 + # migrated yarn → npm, which added node_modules/.package-lock.json and + # so switched this check on for the first time. Every file of every + # vendored @types package was then reported as injected code, even + # though each tarball had already passed integrity verification. + result = _run(_types_tree(), _types_lock(), tarballs={TYPES_URL: TYPES_TGZ}) + assert result.ok is True + assert result.verified == ["@types/estree"] + assert not result.extra and not result.mismatched and not result.errors + + def test_injected_file_in_definitelytyped_package_still_flagged(self): + # Precision guard: detecting the root must not stop real extra files + # inside a bare-name-rooted package from being caught. + tree = _types_tree(extra={"node_modules/@types/estree/evil.js": "deadbeef00"}) + result = _run(tree, _types_lock(), tarballs={TYPES_URL: TYPES_TGZ}) + assert result.ok is False + assert "node_modules/@types/estree/evil.js" in result.extra + def test_noisy_bin_files_not_flagged_as_extra(self): tree = _tree_for(PKG_FILES, extra={"node_modules/.bin/foo": "shimsha00"}) result = _run(tree, _lock()) diff --git a/utils/verify_action_build/npm_registry_verify.py b/utils/verify_action_build/npm_registry_verify.py index 65b2db9d..6200d3cf 100644 --- a/utils/verify_action_build/npm_registry_verify.py +++ b/utils/verify_action_build/npm_registry_verify.py @@ -261,20 +261,33 @@ def _download_tarball(url: str) -> bytes | None: def _tarball_files(data: bytes) -> dict[str, bytes]: """Extract a ``.tgz`` into ``{relative_path: bytes}``. - npm tarballs root everything under ``package/``; that prefix is - stripped so paths line up with ``node_modules//``. + npm tarballs root everything under a single top-level directory; that + prefix is stripped so paths line up with ``node_modules//``. + The directory is ``package/`` by convention, but DefinitelyTyped + publishes ``@types/*`` under the bare package name instead + (``estree/index.d.ts``, ``json-schema/LICENSE``), so the root is + detected rather than assumed — hardcoding ``package/`` left every file + of every vendored ``@types`` package unaccounted for, and the caller + reports unaccounted files as injected code. + + A tarball whose files do not all share one root is left untouched. """ files: dict[str, bytes] = {} with tarfile.open(fileobj=io.BytesIO(data), mode="r:gz") as tf: - for member in tf.getmembers(): - if not member.isfile(): - continue - name = member.name - rel = name[len("package/"):] if name.startswith("package/") else name + # Some publishers emit "./package/..." member names. + members = [ + (m.name[2:] if m.name.startswith("./") else m.name, m) + for m in tf.getmembers() + if m.isfile() + ] + roots = {name.split("/", 1)[0] for name, _ in members if "/" in name} + nested = all("/" in name for name, _ in members) + strip = f"{next(iter(roots))}/" if nested and len(roots) == 1 else "" + for name, member in members: extracted = tf.extractfile(member) if extracted is None: continue - files[rel] = extracted.read() + files[name[len(strip):] if strip else name] = extracted.read() return files