verify-action-build: diff and scan the shell payload actions ship

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:

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

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

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)

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

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: