From 71803c1cd19881d2d24dfe9737173ce09d150f82 Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Mon, 3 Aug 2026 03:12:08 +0200 Subject: [PATCH] verify-action-build: normalize package.json before comparing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A vendored node_modules holds the manifest npm rewrote at install time, while the registry tarball and a fresh npm ci hold the published one. Comparing bytes reported the difference as tampering. Two rewrites are involved: npm's _-prefixed install bookkeeping (the enumerated list missed _args and _location), and normalize-package-data expanding shorthand — author string to a person object, bugs to {url}, and a git+ prefix on repository.url. reactivecircus/android-emulator-runner ships exactly this shape, and one file, node_modules/tunnel/package.json, was the only difference across 217 — enough to fail the whole JS build check and the registry check. Also exempts @actions/tool-cache's bundled 7zdec.exe from the in-tree binary check, and fixes the registry mismatch line, which passed the offending path as the URL and so never named the failing file. Generated-by: Claude Opus 5 (1M context) via Claude Code --- .../test_diff_node_modules.py | 63 +++++++++ .../test_npm_registry_verify.py | 111 ++++++++++++++++ .../verify_action_build/test_security.py | 20 +++ .../verify_action_build/diff_node_modules.py | 11 +- .../npm_registry_verify.py | 121 +++++++++++++++++- utils/verify_action_build/security.py | 10 ++ 6 files changed, 326 insertions(+), 10 deletions(-) diff --git a/utils/tests/verify_action_build/test_diff_node_modules.py b/utils/tests/verify_action_build/test_diff_node_modules.py index b45715367..81b57f80d 100644 --- a/utils/tests/verify_action_build/test_diff_node_modules.py +++ b/utils/tests/verify_action_build/test_diff_node_modules.py @@ -183,3 +183,66 @@ def test_package_json_install_fields_ignored(self, tmp_path): orig, rebuilt, "test", "repo", "a" * 40, ) assert result is True + + def test_package_json_npm6_install_metadata_ignored(self, tmp_path): + # Real shape from reactivecircus/android-emulator-runner@a421e438: + # node_modules/tunnel/package.json was installed with npm v6-era + # tooling, which writes _args/_location (among others) that a modern + # npm ci rebuild does not. An enumerated field list missed exactly + # those two and reported the package as modified — the only "content + # difference" in 217 files, which failed the whole JS build check. + import json + orig = tmp_path / "original" + rebuilt = tmp_path / "rebuilt" + orig.mkdir() + rebuilt.mkdir() + (orig / "tunnel").mkdir() + (rebuilt / "tunnel").mkdir() + + published = { + "name": "tunnel", + "version": "0.0.6", + "main": "./index.js", + "license": "MIT", + } + installed = { + **published, + "_args": [["tunnel@0.0.6", "."]], + "_from": "tunnel@0.0.6", + "_id": "tunnel@0.0.6", + "_inBundle": False, + "_integrity": "sha512-1h/Lnq9yajKY2PEbBadPXj3VxsDDu844OnaAo52UVmIzIvwwtBPIuNvkjuzBlTWpfJyUbG3ez0KSBibQkj4ojg==", + "_location": "/tunnel", + "_phantomChildren": {}, + "_requested": {"type": "version", "registry": True}, + "_requiredBy": ["/@actions/http-client"], + "_resolved": "https://registry.npmjs.org/tunnel/-/tunnel-0.0.6.tgz", + "_spec": "0.0.6", + "_where": ".", + } + + (orig / "tunnel" / "package.json").write_text(json.dumps(installed)) + (rebuilt / "tunnel" / "package.json").write_text(json.dumps(published)) + + assert diff_node_modules(orig, rebuilt, "test", "repo", "a" * 40) is True + + def test_package_json_real_field_change_still_flagged(self, tmp_path): + # Precision guard: stripping _-prefixed keys must not mask a change + # to a field that actually affects what runs. + import json + orig = tmp_path / "original" + rebuilt = tmp_path / "rebuilt" + orig.mkdir() + rebuilt.mkdir() + (orig / "tunnel").mkdir() + (rebuilt / "tunnel").mkdir() + + (orig / "tunnel" / "package.json").write_text( + json.dumps({"name": "tunnel", "version": "0.0.6", "main": "./evil.js", + "_resolved": "https://registry.npmjs.org/tunnel/-/tunnel-0.0.6.tgz"}) + ) + (rebuilt / "tunnel" / "package.json").write_text( + json.dumps({"name": "tunnel", "version": "0.0.6", "main": "./index.js"}) + ) + + assert diff_node_modules(orig, rebuilt, "test", "repo", "a" * 40) is False 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 1362c8f23..6c6660a36 100644 --- a/utils/tests/verify_action_build/test_npm_registry_verify.py +++ b/utils/tests/verify_action_build/test_npm_registry_verify.py @@ -30,6 +30,8 @@ _git_blob_sha1, _integrity_matches, _tarball_files, + normalize_package_json, + strip_npm_install_metadata, verify_vendored_node_modules, ) @@ -91,6 +93,18 @@ def _run(tree, lockfile_bytes, tarballs=None, truncated=False): return verify_vendored_node_modules("org", "repo", "deadbeef") +def _run_with_files(tree, files, tarballs=None): + """Like :func:`_run`, but serves committed file bytes per path. + + Needed once a check fetches a committed file (not just the lockfile). + """ + tarballs = tarballs or {PKG_URL: PKG_TGZ} + with mock.patch.object(nrv, "_fetch_tree_with_sha", return_value=(tree, False)), \ + mock.patch.object(nrv, "_fetch_lockfile", side_effect=lambda o, r, c, p: files.get(p)), \ + mock.patch.object(nrv, "_download_tarball", side_effect=lambda url: tarballs.get(url)): + return verify_vendored_node_modules("org", "repo", "deadbeef") + + class TestHelpers: def test_git_blob_sha1_known_value(self): # git hash-object of an empty blob is well-known. @@ -110,6 +124,63 @@ def test_tarball_files_strips_package_prefix(self): out = _tarball_files(PKG_TGZ) assert out == PKG_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. + assert strip_npm_install_metadata({ + "name": "tunnel", "version": "0.0.6", + "_args": [["tunnel@0.0.6", "."]], "_location": "/tunnel", + "_resolved": "https://registry.npmjs.org/tunnel/-/tunnel-0.0.6.tgz", + }) == {"name": "tunnel", "version": "0.0.6"} + # Nothing else is touched. + assert strip_npm_install_metadata({"name": "x"}) == {"name": "x"} + + def test_normalize_package_json_shorthand_fields(self): + # Exact shapes from tunnel@0.0.6 as vendored by + # reactivecircus/android-emulator-runner@a421e438 vs the published + # tarball. npm's normalize-package-data expands author/bugs and + # prefixes repository.url with "git+" at install time. + published = { + "name": "tunnel", + "version": "0.0.6", + "author": "Koichi Kobayashi ", + "bugs": "https://github.com/koichik/node-tunnel/issues", + "repository": { + "type": "git", + "url": "https://github.com/koichik/node-tunnel.git", + }, + } + installed = { + "name": "tunnel", + "version": "0.0.6", + "author": {"name": "Koichi Kobayashi", "email": "koichik@improvement.jp"}, + "bugs": {"url": "https://github.com/koichik/node-tunnel/issues"}, + "repository": { + "type": "git", + "url": "git+https://github.com/koichik/node-tunnel.git", + }, + "_location": "/tunnel", + } + assert normalize_package_json(installed) == normalize_package_json(published) + + def test_normalize_package_json_leaves_runtime_fields_strict(self): + # Fields that decide what actually runs are compared as-is. + base = {"name": "foo", "version": "1.0.0"} + assert normalize_package_json({**base, "main": "./index.js"}) != \ + normalize_package_json({**base, "main": "./evil.js"}) + assert normalize_package_json({**base, "scripts": {"postinstall": "x"}}) != \ + normalize_package_json(base) + assert normalize_package_json({**base, "dependencies": {"a": "1"}}) != \ + normalize_package_json({**base, "dependencies": {"a": "2"}}) + + def test_normalize_person_handles_name_only_and_url(self): + assert normalize_package_json({"author": "Jane Doe"})["author"] == {"name": "Jane Doe"} + assert normalize_package_json( + {"author": "Jane Doe (https://example.com)"} + )["author"] == { + "name": "Jane Doe", "email": "j@example.com", "url": "https://example.com", + } + class TestVerify: def test_no_vendored_lockfile_returns_none(self): @@ -123,6 +194,46 @@ def test_clean_match_passes(self): assert result.verified == ["foo"] assert not result.mismatched and not result.extra and not result.errors + def test_package_json_install_metadata_is_not_a_mismatch(self): + # reactivecircus/android-emulator-runner@a421e438 vendors a + # node_modules installed with npm v6-era tooling, so every + # package.json carries _args/_location/... that the registry tarball + # never had. Byte comparison alone reported the package modified. + installed = json.dumps({ + "name": "foo", "version": "1.0.0", + "_args": [["foo@1.0.0", "."]], + "_location": "/foo", + "_resolved": PKG_URL, + "_integrity": _integrity(PKG_TGZ), + }).encode() + tree = _tree_for(PKG_FILES) + tree["node_modules/foo/package.json"] = _git_blob_sha1(installed) + + result = _run_with_files(tree, { + "node_modules/.package-lock.json": _lock(), + "node_modules/foo/package.json": installed, + }) + assert result.ok is True + assert result.verified == ["foo"] + assert not result.mismatched + + def test_package_json_real_change_still_mismatches(self): + # Precision guard: normalising _-prefixed keys must not hide an edit + # to a field that changes what actually runs. + tampered = json.dumps({ + "name": "foo", "version": "1.0.0", "main": "./evil.js", + "_resolved": PKG_URL, + }).encode() + tree = _tree_for(PKG_FILES) + tree["node_modules/foo/package.json"] = _git_blob_sha1(tampered) + + result = _run_with_files(tree, { + "node_modules/.package-lock.json": _lock(), + "node_modules/foo/package.json": tampered, + }) + assert result.ok is False + assert "node_modules/foo/package.json" in result.mismatched + def test_content_mismatch_fails(self): tree = _tree_for(PKG_FILES) tree["node_modules/foo/index.js"] = _git_blob_sha1(b"EVIL();\n") # tampered diff --git a/utils/tests/verify_action_build/test_security.py b/utils/tests/verify_action_build/test_security.py index 969fc7d6c..9409ce9ca 100644 --- a/utils/tests/verify_action_build/test_security.py +++ b/utils/tests/verify_action_build/test_security.py @@ -1491,6 +1491,26 @@ def test_gradle_wrapper_jar_exempt(self): assert _looks_like_in_tree_binary("dist/gradle-wrapper.jar") is True assert _looks_like_in_tree_binary("Library.jar") is True + def test_actions_tool_cache_7zdec_exempt(self): + # 7zdec.exe ships inside the first-party @actions/tool-cache npm + # package (it backs tc.extractZip on Windows), so every action that + # vendors node_modules carries it verbatim from the published + # tarball. reactivecircus/android-emulator-runner was false-flagged + # for it. + assert _looks_like_in_tree_binary( + "node_modules/@actions/tool-cache/scripts/externals/7zdec.exe" + ) is False + # Exempt under a nested action sub-path too. + assert _looks_like_in_tree_binary( + "subdir/node_modules/@actions/tool-cache/scripts/externals/7zdec.exe" + ) is False + # Precision: the same name dropped anywhere else is still caught — + # the suffix match needs the canonical package path. + assert _looks_like_in_tree_binary("dist/7zdec.exe") is True + assert _looks_like_in_tree_binary( + "node_modules/evil-pkg/scripts/externals/7zdec.exe" + ) is True + def test_matlab_platform_dir_naming(self): # MATLAB's launcher convention: dist/bin//run-matlab-command # where is MATLAB's own arch identifier and the file has diff --git a/utils/verify_action_build/diff_node_modules.py b/utils/verify_action_build/diff_node_modules.py index 55ef48820..b6082f7ab 100644 --- a/utils/verify_action_build/diff_node_modules.py +++ b/utils/verify_action_build/diff_node_modules.py @@ -24,6 +24,7 @@ from .console import console, link from .diff_display import show_colored_diff +from .npm_registry_verify import normalize_package_json def diff_node_modules( @@ -129,15 +130,9 @@ def collect_files(base: Path) -> dict[Path, str]: if rel_path.name == "package.json": orig_text = (original_dir / rel_path).read_text(errors="replace") rebuilt_text = (rebuilt_dir / rel_path).read_text(errors="replace") - install_fields = {"_resolved", "_integrity", "_from", "_where", "_id", - "_requested", "_requiredBy", "_shasum", "_spec", - "_phantomChildren", "_inBundle"} try: - orig_json = json.loads(orig_text) - rebuilt_json = json.loads(rebuilt_text) - for field in install_fields: - orig_json.pop(field, None) - rebuilt_json.pop(field, None) + orig_json = normalize_package_json(json.loads(orig_text)) + rebuilt_json = normalize_package_json(json.loads(rebuilt_text)) if orig_json == rebuilt_json: continue except (json.JSONDecodeError, ValueError): diff --git a/utils/verify_action_build/npm_registry_verify.py b/utils/verify_action_build/npm_registry_verify.py index 9b06389fb..65b2db9d1 100644 --- a/utils/verify_action_build/npm_registry_verify.py +++ b/utils/verify_action_build/npm_registry_verify.py @@ -49,6 +49,7 @@ import io import json import os +import re import tarfile import requests @@ -95,6 +96,90 @@ def ok(self) -> bool: ) +def strip_npm_install_metadata(obj: dict) -> dict: + """Drop npm's install-time metadata keys from a parsed ``package.json``. + + npm writes bookkeeping into each installed package's ``package.json`` + that is absent from the registry tarball and from a modern ``npm ci`` + rebuild: ``_args``, ``_from``, ``_id``, ``_inBundle``, ``_integrity``, + ``_location``, ``_phantomChildren``, ``_requested``, ``_requiredBy``, + ``_resolved``, ``_shasum``, ``_spec``, ``_where``. The exact set has + varied across npm versions, so match the reserved ``_`` prefix rather + than enumerating — an enumeration missed ``_args`` and ``_location`` + and false-flagged reactivecircus/android-emulator-runner, whose + vendored tree was installed with npm v6-era tooling. + + Only ``_``-prefixed keys are dropped; npm ignores unknown fields at + runtime, so this cannot mask a behavioural difference. + """ + return {k: v for k, v in obj.items() if not k.startswith("_")} + + +# "Name (url)", npm's shorthand for a person object. +_PERSON_RE = re.compile( + r"^\s*(?P[^<(]*?)\s*" + r"(?:<(?P[^>]*)>)?\s*" + r"(?:\((?P[^)]*)\))?\s*$" +) + + +def _normalize_person(value: object) -> object: + """Expand npm's person shorthand string into its object form.""" + if not isinstance(value, str): + return value + match = _PERSON_RE.match(value) + if not match: + return value + out = {k: v for k, v in match.groupdict().items() if v} + return out or value + + +def _normalize_repository(value: object) -> object: + """Canonicalise a ``repository`` field. + + npm records the URL with a ``git+`` scheme prefix that the published + manifest often omits. + """ + if isinstance(value, str): + value = {"url": value} + if isinstance(value, dict) and isinstance(value.get("url"), str): + value = dict(value) + url = value["url"] + if url.startswith("git+"): + value["url"] = url[len("git+"):] + return value + + +def normalize_package_json(obj: dict) -> dict: + """Normalise a ``package.json`` for comparison across install states. + + On install, npm runs the published manifest through + ``normalize-package-data``, which rewrites shorthand forms in place: + ``author``/``contributors``/``maintainers`` strings become person + objects, ``bugs`` becomes ``{"url": ...}``, and ``repository.url`` + gains a ``git+`` prefix. A vendored ``node_modules`` therefore holds + the *normalised* manifest while the registry tarball and a fresh + rebuild hold the *published* one — a difference in representation, not + in what the package does. + + Combined with :func:`strip_npm_install_metadata`. Fields this does + not recognise are left untouched and still compared strictly, so an + edit to ``main``, ``bin``, ``scripts`` or a dependency still fails. + """ + out = strip_npm_install_metadata(obj) + for key in ("author",): + if key in out: + out[key] = _normalize_person(out[key]) + for key in ("contributors", "maintainers"): + if isinstance(out.get(key), list): + out[key] = [_normalize_person(p) for p in out[key]] + if "bugs" in out and isinstance(out["bugs"], str): + out["bugs"] = {"url": out["bugs"]} + if "repository" in out: + out["repository"] = _normalize_repository(out["repository"]) + return out + + def _git_blob_sha1(data: bytes) -> str: """Git's blob object id: sha1 of ``blob \\0``. @@ -193,6 +278,27 @@ def _tarball_files(data: bytes) -> dict[str, bytes]: return files +def _package_json_equivalent( + org: str, repo: str, commit_hash: str, path: str, published: bytes, +) -> bool: + """Return True if the committed ``package.json`` at ``path`` differs + from the registry tarball's copy only by npm install metadata. + + Fetched lazily — only once a byte comparison has already failed. + """ + raw = _fetch_lockfile(org, repo, commit_hash, path) + if raw is None: + return False + try: + committed_json = json.loads(raw) + published_json = json.loads(published) + except ValueError: + return False + if not isinstance(committed_json, dict) or not isinstance(published_json, dict): + return False + return normalize_package_json(committed_json) == normalize_package_json(published_json) + + def _is_noisy(rel_path: str) -> bool: parts = rel_path.split("/") return parts[-1] in _NOISY_NAMES or any(p in _NOISY_DIRS for p in parts) @@ -302,6 +408,10 @@ def verify_vendored_node_modules( if committed_sha is None: continue # tarball ships a file the repo omits — benign if committed_sha != _git_blob_sha1(content): + if rel.split("/")[-1] == "package.json" and _package_json_equivalent( + org, repo, commit_hash, prefix + committed_path, content, + ): + continue # differs only by npm's install-time metadata result.mismatched.append(committed_path) pkg_ok = False if pkg_ok: @@ -323,7 +433,9 @@ def verify_vendored_node_modules( def _render(result: NpmRegistryResult, org: str, repo: str, commit_hash: str) -> None: """Print a per-category summary of the registry check.""" - blob = f"https://github.com/{org}/{repo}/tree/{commit_hash}/node_modules" + # Paths in ``mismatched`` / ``extra`` already start with ``node_modules/``, + # so the base stops at the commit tree root. + blob = f"https://github.com/{org}/{repo}/tree/{commit_hash}" if result.verified: console.print( f" [green]✓[/green] {len(result.verified)} package(s) match " @@ -339,7 +451,12 @@ def _render(result: NpmRegistryResult, org: str, repo: str, commit_hash: str) -> + (" …" if len(result.skipped) > 8 else "") ) for path in result.mismatched: - console.print(f" [red]✗[/red] {link(path, f'{blob}')} — content differs from registry tarball") + # link() takes (url, text) — naming the offending file matters more + # than linking the tree root, which told a reviewer nothing. + console.print( + f" [red]✗[/red] {link(f'{blob}/{path}', path)}" + f" — content differs from registry tarball" + ) for path in result.extra: console.print(f" [red]✗[/red] {path} — present in repo but not in the verified package tarball") for err in result.errors: diff --git a/utils/verify_action_build/security.py b/utils/verify_action_build/security.py index 6a5f9cf81..62a858058 100644 --- a/utils/verify_action_build/security.py +++ b/utils/verify_action_build/security.py @@ -1789,6 +1789,16 @@ def analyze_repo_metadata( # gradle/wrapper-validation-action). Seen on JetBrains/qodana-action, # a node24 action that never shells out to Gradle at runtime. "gradle/wrapper/gradle-wrapper.jar", + # 7-Zip's standalone decoder, bundled inside the first-party + # @actions/tool-cache npm package (it backs ``tc.extractZip`` on + # Windows). It is not the action's own code: any action that vendors + # its node_modules ships it verbatim from the published tarball, and + # the Vendored npm Registry Check verifies that package's bytes against + # the lockfile integrity hash. Seen on reactivecircus/android-emulator- + # runner. The general fix — crediting any binary under a node_modules + # package the registry check already verified — needs the two checks + # reordered, so this keeps the narrow, reviewable form used above. + "node_modules/@actions/tool-cache/scripts/externals/7zdec.exe", )