Skip to content

verify-action-build: end run: block at sibling keys like env: - #1105

Open
potiuk wants to merge 1 commit into
mainfrom
fix/metadata-run-block-boundary
Open

verify-action-build: end run: block at sibling keys like env:#1105
potiuk wants to merge 1 commit into
mainfrom
fix/metadata-run-block-boundary

Conversation

@potiuk

@potiuk potiuk commented Jul 27, 2026

Copy link
Copy Markdown
Member

Follow-up from reviewing #1076.

  • analyze_action_metadata closed a run: block scalar only on keys indented less than 8 spaces, so a sibling env: (and everything after it) was 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 direct input interpolation (injection risk) check reported the mitigation as the flaw: carabiner-dev/actions/install/download-and-verify@60563b5 raised 19 shell findings, all false.
  • Now tracks the indent of the run: key and closes the block at any content indented no deeper. Single-line run: cmd is scanned on its own line instead of swallowing the keys that follow it.

Test plan

  • 3 regression tests in utils/tests/verify_action_build/test_security.py; the fixture is the real download-and-verify action.yml shape. Two of the three fail without the security.py change.
  • uv run pytest utils/tests/ - 310 passed.
  • prek run --all-files - clean.
  • verify_action_build --from-pr 1076 now reports 2 shell findings instead of 19 and no injection warning. The GITHUB_PATH write on the single-line run: at line 177 is newly detected, which the old boundary logic skipped.

Generated-by: Claude Opus 5 (1M context) via Claude Code

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@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
@potiuk

potiuk commented Jul 28, 2026

Copy link
Copy Markdown
Member Author

Fresh evidence for this one: the false positive fired again today on #1109
(carabiner-dev/actions/install/download-and-verify bumped to 4c23ca5), which
has since been merged.

The CI verify job reported:

action.yml line 80: direct input interpolation in shell (injection risk)

Line 80 of that action.yml is:

79:         env:
80:           INPUTS_INSTALL_DIR: ${{ inputs.install-dir }}

That is the env: mapping, i.e. exactly the mitigation this check is meant to
encourage, and the value is character-allowlisted by the case statement in the
run: block right above it. The warning is being reported against the safe
pattern.

The cause is the boundary logic this PR replaces: on main, security.py:983
closes the run: block only on keys indented less than 8 spaces, and the sibling
env: here is indented exactly 8, so the block stays open and the following line
is scanned as shell body.

Every carabiner-dev install/* and download-and-verify bump has this shape, so
this re-fires on each one and costs the reviewer a manual dismissal each time.

@potiuk

potiuk commented Jul 30, 2026

Copy link
Copy Markdown
Member Author

🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant