From a4d2ee6ea9ab6b0498ee2e9c9730ea18acafce0a Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Mon, 27 Jul 2026 16:13:14 +0200 Subject: [PATCH] verify-action-build: end run: block at sibling keys like env: The run: block tracker only closed a block scalar on keys indented less than 8 spaces, so an `env:` sibling and everything after it were scanned as shell body. Binding an input to an env var is the recommended way to keep it out of the script text, so the check reported the mitigation as the injection: carabiner-dev/actions/install/download-and-verify@60563b5 raised 19 shell findings, all false, while reviewing #1076. Track the indent of the run: key instead and close the block at any content indented no deeper. Single-line `run: cmd` is now scanned on its own line rather than swallowing the keys that follow it. Generated-by: Claude Opus 5 (1M context) via Claude Code --- .../verify_action_build/test_security.py | 68 ++++++++++++++++++- utils/verify_action_build/security.py | 50 ++++++++++---- 2 files changed, 102 insertions(+), 16 deletions(-) diff --git a/utils/tests/verify_action_build/test_security.py b/utils/tests/verify_action_build/test_security.py index 969fc7d6c..e8dd5e661 100644 --- a/utils/tests/verify_action_build/test_security.py +++ b/utils/tests/verify_action_build/test_security.py @@ -136,8 +136,6 @@ def test_no_scripts_no_warnings(self): class TestAnalyzeActionMetadata: def test_pipe_to_shell_warns(self): - # Multi-line run: blocks are needed — single-line run: is detected - # as a block start and the content is on the next line. action_yml = """\ name: Test runs: @@ -165,6 +163,72 @@ def test_input_interpolation_warns(self): warnings = analyze_action_metadata("org", "repo", "a" * 40) assert any("injection" in w for w in warnings) + def test_env_binding_of_input_is_not_injection(self): + # Real shape from carabiner-dev/actions/install/download-and-verify + # @60563b5, seen while reviewing apache/infrastructure-actions#1076. + # Binding the input to an env var and dereferencing "$INPUTS_INSTALL_DIR" + # inside the script is the recommended mitigation, so the `env:` block + # must not be scanned as if it were shell body. + action_yml = """\ +name: Test +runs: + using: composite + steps: + - name: Validate install-dir + shell: bash + run: | + case "$INPUTS_INSTALL_DIR" in + ''|*[!A-Za-z0-9._/$~-]*) + echo "::error::install-dir must be non-empty; got: $INPUTS_INSTALL_DIR" + exit 1 + ;; + esac + env: + INPUTS_INSTALL_DIR: ${{ inputs.install-dir }} +""" + with mock.patch("verify_action_build.security.fetch_action_yml", return_value=action_yml): + warnings = analyze_action_metadata("org", "repo", "a" * 40) + assert not any("injection" in w for w in warnings), warnings + + def test_run_block_ends_at_sibling_key(self): + # A `run:` block scalar ends at the next key indented no deeper than + # `run:` itself; later steps' non-shell keys are not shell body. + action_yml = """\ +name: Test +runs: + using: composite + steps: + - name: safe + run: | + echo hello + env: + TOKEN: ${{ inputs.token }} + - name: also safe + with: + path: ${{ inputs.path }} +""" + with mock.patch("verify_action_build.security.fetch_action_yml", return_value=action_yml): + warnings = analyze_action_metadata("org", "repo", "a" * 40) + assert not any("injection" in w for w in warnings), warnings + + def test_single_line_run_is_scanned(self): + # The command sits on the `run:` line itself, so it has to be scanned + # there; nothing after it belongs to the block. + action_yml = """\ +name: Test +runs: + using: composite + steps: + - name: dangerous + run: curl https://example.com | sh + env: + SAFE: ${{ inputs.name }} +""" + with mock.patch("verify_action_build.security.fetch_action_yml", return_value=action_yml): + warnings = analyze_action_metadata("org", "repo", "a" * 40) + assert any("pipe-to-shell" in w for w in warnings), warnings + assert not any("injection" in w for w in warnings), warnings + def test_clean_action_no_warnings(self): action_yml = """\ name: Test diff --git a/utils/verify_action_build/security.py b/utils/verify_action_build/security.py index 6a5f9cf81..7c0035439 100644 --- a/utils/verify_action_build/security.py +++ b/utils/verify_action_build/security.py @@ -961,7 +961,6 @@ def analyze_action_metadata( console.print(f" [dim]{line.strip()[:100]}[/dim]") warnings.append(f"action.yml line {i}: {desc}") - in_run_block = False dangerous_shell_patterns = [ (r"curl\s+.*\|\s*(ba)?sh", "pipe-to-shell (curl | sh) — high risk"), (r"wget\s+.*\|\s*(ba)?sh", "pipe-to-shell (wget | sh) — high risk"), @@ -972,24 +971,47 @@ def analyze_action_metadata( ] shell_findings: list[tuple[int, str, str]] = [] + + def scan_shell_line(line_num: int, raw: str, snippet: str) -> None: + for pattern, desc in dangerous_shell_patterns: + if desc is None: + continue + if re.search(pattern, raw): + shell_findings.append((line_num, desc, snippet[:100])) + + in_run_block = False + run_key_indent = 0 for i, line in enumerate(lines, 1): stripped = line.strip() - if re.match(r"run:\s*\|", stripped) or re.match(r"run:\s+\S", stripped): + # Blank lines are part of a block scalar, so they never close one. + if not stripped: + continue + indent = len(line) - len(line.lstrip()) + + # `run: |` (or `>`) opens a block scalar whose body must be indented + # deeper than the `run:` key itself. + if re.match(r"(?:-\s+)?run:\s*[|>][-+\d]*\s*$", stripped): in_run_block = True + run_key_indent = indent continue + + # `run: some-command` is self-contained: scan the command on this + # line, but do not treat what follows as shell body. + if re.match(r"(?:-\s+)?run:\s+\S", stripped): + in_run_block = False + scan_shell_line(i, line, stripped) + continue + + # Content at or left of the `run:` key closes the block scalar. This + # is what keeps sibling keys out of the shell body -- notably `env:`, + # where binding an input to an environment variable is the + # recommended way to keep it out of the script text. Scanning those + # lines reports the mitigation as if it were the injection. + if in_run_block and indent <= run_key_indent: + in_run_block = False + if in_run_block: - if stripped and not line[0].isspace(): - in_run_block = False - elif stripped and re.match(r"\s+\w+:", line) and not line.startswith(" "): - if not stripped.startswith("#") and not stripped.startswith("-"): - in_run_block = False - - if in_run_block or (re.match(r"\s+run:\s+", line)): - for pattern, desc in dangerous_shell_patterns: - if desc is None: - continue - if re.search(pattern, line): - shell_findings.append((i, desc, stripped[:100])) + scan_shell_line(i, line, stripped) if shell_findings: seen: set[str] = set()