From ebfe98e601b46d8e6f77dc2a52d4a19d296e43fa Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Thu, 27 Aug 2026 20:34:13 +0200 Subject: [PATCH] verify-action-build: diff and scan the shell payload actions ship The source diff only copied JS/TS/JSON/YAML, and script analysis ran only for composite/docker actions and only for scripts named in action.yml or a Dockerfile. A node action that shells out to a committed script therefore had that script reviewed by nobody: uraimo/run-on-arch-action declares main: src/run-on-arch.js, which exec()s src/run-on-arch.sh, so PR #1196's +12/-1 change to its docker run invocation never reached the reviewer. Add shell/interpreter extensions and Dockerfile* to the source diff, discover committed scripts from the repo tree, and run script analysis for JS actions too (informational there - the verdict is unchanged). Generated-by: Claude Code (Opus 5) --- README.md | 2 + .../verify_action_build/test_diff_source.py | 61 ++++++++++++++ .../verify_action_build/test_security.py | 83 +++++++++++++++++-- utils/verify_action_build/diff_source.py | 27 +++++- utils/verify_action_build/security.py | 38 +++++++++ utils/verify_action_build/verification.py | 14 ++++ 6 files changed, 217 insertions(+), 8 deletions(-) create mode 100644 utils/tests/verify_action_build/test_diff_source.py diff --git a/README.md b/README.md index b03f70dfc..581c05cd3 100644 --- a/README.md +++ b/README.md @@ -258,6 +258,8 @@ Non-minified compiled JS (e.g. Deno `deno task bundle` output, Dart `dart compil Files that appear **only in the rebuild** are reported as informational rather than as a failure. The action does not publish them, so they never reach a consumer's runner and cannot be a supply-chain vector. In practice they are intermediate build output from a multi-stage build that upstream deliberately does not commit — for example `JetBrains/qodana-action` declares `main: scan/dist/index.js`, so the output directory resolves to the whole `scan/` sub-project and the rebuild's gitignored `scan/lib/*.js` (stage one of its `tsc` → `esbuild` build) lands inside the compared tree. The inverse remains a hard failure: JS present in the published tree but *absent* from the rebuild is unaccounted-for shipped code, as is a published tree with no compiled JS at all when the rebuild produced some — there is then nothing to reconcile the rebuild against. +The **source diff vs approved** and the **Script analysis** check both cover more than the language the entrypoint is written in. A node action is free to shell out to a script committed beside it — `uraimo/run-on-arch-action` declares `main: src/run-on-arch.js`, which then `exec()`s `src/run-on-arch.sh` — so shell/interpreter scripts (`.sh`, `.bash`, `.ps1`, `.py`, `.rb`, `.pl`) and `Dockerfile*` are diffed alongside the JS/TS sources, and committed shell scripts are discovered from the repo tree rather than only from the files `action.yml` or a `Dockerfile` happen to name. Script analysis runs for every action type; for JavaScript actions its findings are reported in the summary but do not change the pass/fail verdict. + #### Security Review Checklist When reviewing an action (new or updated), watch for these potential issues in the source diff between the approved and new versions: diff --git a/utils/tests/verify_action_build/test_diff_source.py b/utils/tests/verify_action_build/test_diff_source.py new file mode 100644 index 000000000..3923380ef --- /dev/null +++ b/utils/tests/verify_action_build/test_diff_source.py @@ -0,0 +1,61 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# +from pathlib import Path + +from verify_action_build.diff_source import is_source_file + + +class TestIsSourceFile: + def test_js_and_ts_sources(self): + for name in ("src/main.ts", "index.js", "bundle.mjs", "helper.cjs"): + assert is_source_file(Path(name)), name + + def test_metadata_files(self): + for name in ("action.yml", "action.yaml", "package.json"): + assert is_source_file(Path(name)), name + + def test_shell_script_is_source(self): + # uraimo/run-on-arch-action ships src/run-on-arch.sh, which + # src/run-on-arch.js exec()s. Skipping it hid a +12/-1 change to the + # docker run invocation from the reviewer. + assert is_source_file(Path("src/run-on-arch.sh")) + assert is_source_file(Path("scripts/install.bash")) + assert is_source_file(Path("setup.ps1")) + + def test_interpreter_scripts_are_source(self): + assert is_source_file(Path("scripts/publish.py")) + assert is_source_file(Path("bin/release.rb")) + assert is_source_file(Path("tools/gen.pl")) + + def test_plain_dockerfile(self): + assert is_source_file(Path("Dockerfile")) + + def test_suffixed_dockerfile(self): + # Dockerfile.aarch64.trixie has a .trixie suffix, so only the name + # prefix identifies it. + assert is_source_file(Path("Dockerfiles/Dockerfile.aarch64.trixie")) + assert is_source_file(Path("Dockerfile.loongarch64.alpine_latest")) + + def test_non_source_files_excluded(self): + for name in ("README.md", "LICENSE", "logo.png", "notes.txt", "core_bg.wasm"): + assert not is_source_file(Path(name)), name + + def test_name_containing_dockerfile_not_prefixed(self): + # Only a leading "Dockerfile" counts; a mention elsewhere does not. + assert not is_source_file(Path("docs/about-Dockerfile.md")) diff --git a/utils/tests/verify_action_build/test_security.py b/utils/tests/verify_action_build/test_security.py index 0816af03f..f970f697c 100644 --- a/utils/tests/verify_action_build/test_security.py +++ b/utils/tests/verify_action_build/test_security.py @@ -102,6 +102,11 @@ def fetch(org, repo, commit, path): return files.get(path) return fetch + def _mock_tree(self, paths=()): + return mock.patch( + "verify_action_build.security._list_repo_files", return_value=list(paths) + ) + def test_detects_eval(self): action_yml = """\ name: Test @@ -113,9 +118,10 @@ def test_detects_eval(self): files = { "script.py": 'eval("malicious code")\n', } - with mock.patch("verify_action_build.security.fetch_action_yml", return_value=action_yml): - with mock.patch("verify_action_build.security.fetch_file_from_github", side_effect=self._mock_fetch_file(files)): - warnings = analyze_scripts("org", "repo", "a" * 40) + with mock.patch("verify_action_build.security.fetch_action_yml", return_value=action_yml), \ + self._mock_tree(), \ + mock.patch("verify_action_build.security.fetch_file_from_github", side_effect=self._mock_fetch_file(files)): + warnings = analyze_scripts("org", "repo", "a" * 40) # Script analysis finds suspicious patterns (eval is in findings) # Warnings list may be empty since script analysis only logs to console # but doesn't add to warnings for all patterns @@ -128,11 +134,76 @@ def test_no_scripts_no_warnings(self): using: node20 main: dist/index.js """ - with mock.patch("verify_action_build.security.fetch_action_yml", return_value=action_yml): - with mock.patch("verify_action_build.security.fetch_file_from_github", return_value=None): - warnings = analyze_scripts("org", "repo", "a" * 40) + with mock.patch("verify_action_build.security.fetch_action_yml", return_value=action_yml), \ + self._mock_tree(), \ + mock.patch("verify_action_build.security.fetch_file_from_github", return_value=None): + warnings = analyze_scripts("org", "repo", "a" * 40) assert warnings == [] + def test_scans_shell_script_only_reachable_from_js_entrypoint(self): + # uraimo/run-on-arch-action: action.yml names only the JS entrypoint, + # which then exec()s src/run-on-arch.sh. Nothing in action.yml or a + # Dockerfile mentions the script, so it has to come from the tree. + action_yml = """\ +name: Run on architecture +runs: + using: node24 + main: 'src/run-on-arch.js' +""" + files = {"src/run-on-arch.sh": "#!/bin/bash\ndocker run --rm ubuntu\n"} + with mock.patch("verify_action_build.security.fetch_action_yml", return_value=action_yml), \ + self._mock_tree(["src/run-on-arch.js", "src/run-on-arch.sh", "README.md"]), \ + mock.patch( + "verify_action_build.security.fetch_file_from_github", + side_effect=self._mock_fetch_file(files), + ) as fetch: + analyze_scripts("org", "repo", "a" * 40) + fetched = {call.args[3] for call in fetch.call_args_list} + assert "src/run-on-arch.sh" in fetched + + def test_tree_discovery_skips_vendored_and_test_dirs(self): + action_yml = """\ +name: Test +runs: + using: node24 + main: 'dist/index.js' +""" + tree = [ + "node_modules/foo/install.sh", + "__tests__/fixture.sh", + "docs/example.sh", + "src/entry.sh", + ] + with mock.patch("verify_action_build.security.fetch_action_yml", return_value=action_yml), \ + self._mock_tree(tree), \ + mock.patch( + "verify_action_build.security.fetch_file_from_github", + side_effect=self._mock_fetch_file({"src/entry.sh": "echo hi\n"}), + ) as fetch: + analyze_scripts("org", "repo", "a" * 40) + fetched = {call.args[3] for call in fetch.call_args_list} + assert "src/entry.sh" in fetched + assert not any(p.startswith(("node_modules/", "__tests__/", "docs/")) for p in fetched) + + def test_tree_discovery_respects_sub_path(self): + action_yml = """\ +name: Test +runs: + using: node24 + main: 'index.js' +""" + tree = ["other/setup.sh", "sub/action/src/setup.sh"] + with mock.patch("verify_action_build.security.fetch_action_yml", return_value=action_yml), \ + self._mock_tree(tree), \ + mock.patch( + "verify_action_build.security.fetch_file_from_github", + side_effect=self._mock_fetch_file({"sub/action/src/setup.sh": "echo hi\n"}), + ) as fetch: + analyze_scripts("org", "repo", "a" * 40, sub_path="sub/action") + fetched = {call.args[3] for call in fetch.call_args_list} + assert "sub/action/src/setup.sh" in fetched + assert "other/setup.sh" not in fetched + class TestAnalyzeActionMetadata: def test_pipe_to_shell_warns(self): diff --git a/utils/verify_action_build/diff_source.py b/utils/verify_action_build/diff_source.py index bd1c19528..8515e4add 100644 --- a/utils/verify_action_build/diff_source.py +++ b/utils/verify_action_build/diff_source.py @@ -27,6 +27,30 @@ from .diff_display import show_colored_diff from .diff_js import beautify_js +# Extensions compared between the approved and the new version. Generated +# output (dist/, node_modules/) and test fixtures are filtered out separately, +# so this set only has to answer "is this a file the action can execute on a +# runner". That is wider than the language the entrypoint is written in: a +# node action is free to shell out to a committed .sh (uraimo/run-on-arch-action +# names only src/run-on-arch.js in action.yml and then exec()s +# src/run-on-arch.sh), and a docker action's Dockerfile is itself part of what +# runs. Anything left out here is a change the reviewer never sees. +SOURCE_EXTENSIONS = { + ".js", ".ts", ".mjs", ".cjs", ".mts", ".cts", ".json", ".yml", ".yaml", + ".sh", ".bash", ".ps1", ".py", ".rb", ".pl", +} + +# Runtime files carrying no extension of their own. Dockerfiles are commonly +# suffixed per target (Dockerfile.aarch64.trixie), so match on the prefix. +SOURCE_FILENAME_PREFIXES = ("Dockerfile",) + + +def is_source_file(rel: Path) -> bool: + """Whether *rel* is compared by the approved-vs-new source diff.""" + if rel.suffix in SOURCE_EXTENSIONS: + return True + return rel.name.startswith(SOURCE_FILENAME_PREFIXES) + def diff_approved_vs_new( org: str, repo: str, approved_hash: str, new_hash: str, work_dir: Path, @@ -57,7 +81,6 @@ def diff_approved_vs_new( "package-lock.json", "yarn.lock", "pnpm-lock.yaml", "bun.lockb", "shrinkwrap.json", "npm-shrinkwrap.json", } - source_extensions = {".js", ".ts", ".mjs", ".cjs", ".mts", ".cts", ".json", ".yml", ".yaml"} with console.status("[bold blue]Fetching source from both versions...[/bold blue]"): clone_dir = work_dir / "repo-clone" @@ -85,7 +108,7 @@ def diff_approved_vs_new( if matched and rel not in include_dist_files: skipped_dirs.update(matched) continue - if rel.suffix in source_extensions: + if is_source_file(rel): dest = out_dir / rel dest.parent.mkdir(parents=True, exist_ok=True) shutil.copy2(f, dest) diff --git a/utils/verify_action_build/security.py b/utils/verify_action_build/security.py index 1936f1f73..5552633b7 100644 --- a/utils/verify_action_build/security.py +++ b/utils/verify_action_build/security.py @@ -569,6 +569,10 @@ def analyze_scripts( if clean: script_files.add(clean) + # Scripts nothing references from action.yml or a Dockerfile still ship + # with the action and still run when the entrypoint shells out to them. + script_files.update(_discover_shell_script_files(org, repo, commit_hash, sub_path)) + if not script_files: return warnings @@ -1255,6 +1259,7 @@ def scan_shell_line(line_num: int, raw: str, snippet: str) -> None: ] _JS_SOURCE_EXTENSIONS = (".ts", ".js", ".mjs", ".cjs") +_SHELL_SCRIPT_EXTENSIONS = (".sh", ".bash", ".ps1") _JS_SCAN_DIR_PREFIXES = ("src/", "lib/", "source/", "sources/", "scripts/") _JS_EXCLUDE_DIR_PREFIXES = ( "dist/", "build/", "out/", "node_modules/", "coverage/", @@ -1380,6 +1385,39 @@ def _discover_js_source_files( return files +def _discover_shell_script_files( + org: str, repo: str, commit_hash: str, sub_path: str, +) -> list[str]: + """Return repo paths of committed shell/interpreter scripts worth scanning. + + ``analyze_scripts`` otherwise only learns about a script when action.yml or + a Dockerfile names it. A node action's entrypoint is free to shell out to + a script that neither file mentions -- uraimo/run-on-arch-action declares + ``main: src/run-on-arch.js``, which then ``exec()``s ``src/run-on-arch.sh`` + -- so the script that actually invokes docker went unscanned. Discover + them from the tree instead, using the same directory filters as the JS + discovery so vendored and test material stays out. + """ + paths: list[str] = [] + all_paths = _list_repo_files(org, repo, commit_hash) + if not all_paths: + return paths + + prefix = f"{sub_path.rstrip('/')}/" if sub_path else "" + for path in all_paths: + if prefix and not path.startswith(prefix): + continue + rel = path[len(prefix):] if prefix else path + if not rel.endswith(_SHELL_SCRIPT_EXTENSIONS): + continue + if any(rel.startswith(d) for d in _JS_EXCLUDE_DIR_PREFIXES): + continue + if "/" in rel and not any(rel.startswith(d) for d in _JS_SCAN_DIR_PREFIXES): + continue + paths.append(rel) + return paths + + def _find_binary_downloads(content: str) -> list[tuple[int, str]]: """Find lines that download binaries or scripts over HTTP(S). diff --git a/utils/verify_action_build/verification.py b/utils/verify_action_build/verification.py index 1291e57b7..6d4348bf2 100644 --- a/utils/verify_action_build/verification.py +++ b/utils/verify_action_build/verification.py @@ -558,6 +558,20 @@ def verify_single_action( js_status, js_detail = "fail", "DIFFERENCES DETECTED" checks_performed.append(("JS build verification", js_status, js_detail)) + # Script analysis used to run only for composite/docker actions, so + # a node action's shell payload was never looked at — even though + # its entrypoint is free to shell out to a committed script that + # neither action.yml nor a Dockerfile names. Findings here are + # informational: they are not added to non_js_warnings, so the + # pass/fail verdict for a JS action is unchanged. + js_script_warnings = analyze_scripts(org, repo, commit_hash, sub_path) + checks_performed.append(( + "Script analysis", + "warn" if js_script_warnings else "pass", + f"{len(js_script_warnings)} warning(s)" if js_script_warnings + else "no suspicious patterns", + )) + # Check for previously approved versions and offer to diff # (reuse the list fetched earlier for the approved_hash build arg) if approved: