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