Skip to content

check_interface()'s token matching reads YAML block-scalar content as structure #949

Description

@ptr727

Found on PR #948 (CodeRabbit review) while adding a new requireTokensInJob/forbidTokensInJob entry for the deploy-site caller-stub interface contract, but the weakness is in check_interface()/_code_view() generally, not specific to that one entry.

The gap

_code_view() (spec/audit.py) strips comment-only lines (#-prefixed), but does not strip YAML block-scalar (|, >, and their chomp-indicator variants) body content. Every requireTokensInJob/forbidTokensInJob check in check_interface() is a plain substring search over that view, so a job whose name: (or any other string-valued key) is written as a block scalar containing text that happens to match a required or forbidden token, at a matching indent, can satisfy or trip the check without the actual YAML structure the token is meant to verify existing at all. Concretely, for the deploy-site.yml contract's new "with:\n environment:" token:

  deploy:
    name: |
      with:
      environment:

check_interface() reports this caller stub as carrying the required with.environment input. It does not - the job has no with: mapping at all, and would fail at GitHub Actions dispatch time with a missing-required-input error, exactly the failure mode #942's fix was meant to catch mechanically rather than only at runtime.

Scope

This is not unique to the deploy-site contract. Every requireTokensInJob/forbidTokensInJob entry across spec/files.json reads through the same _code_view() substring search, so the same construction (a block scalar crafted to contain a matching token at a matching indent) can fool any of them, not only the newly-added one.

Ask

Decide the proportionate fix. Two shapes, roughly by cost:

  1. Targeted mitigation: strip YAML block-scalar body lines (a key: |/key: > line followed by more-indented lines, until dedent) from _code_view()'s output, the same bounded-scope heuristic the function already uses for comments. Catches the realistic case without a general YAML parser.
  2. Structural validation: parse the job mapping properly (e.g. via a YAML library, scoped to just the job block) and check with.environment as an actual child key rather than by substring, which CodeRabbit's own review labeled a "heavy lift" - likely the right call only if _code_view()'s scope of false-positives/negatives turns out to be broader than block scalars alone.

Given every interface check across the file shares this weakness and the trigger requires a deliberately adversarial construction (not something ordinary caller-stub authoring produces by accident), I judged a full engine rewrite out of scope for #948's actual bug fix and filed this separately rather than block that PR on it.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions