Skip to content

chore(ci): finish the #1641 sweep - 159 ${{ }} interpolations remain in run: blocks across 7 workflows #155

Description

@cristim

Summary

LeanerCloud/cloud-commitments-cli#1641 established the rule that untrusted values must reach a run: block through env:, never through ${{ }} interpolation, because interpolation splices the value into the shell script as source text before bash ever sees it. The rule has been applied file by file. 159 interpolations remain inside run: blocks across 7 workflow files on main.

This issue is the census, so whoever finishes the sweep can see the whole surface and reproduce the count.

Current state on main

count workflow notes
44 database-migration.yml
26 deploy-all.yml release-triggered
24 deploy-azure.yml
23 deploy-aws-lambda.yml fixed by LeanerCloud/cloud-commitments-cli#1657 (open at time of writing)
19 deploy-aws-fargate.yml
18 deploy-gcp.yml
5 ci.yml
159 7 of 16 workflow files

Already clean, for reference: rollback.yml and cleanup-staging.yml were fixed by LeanerCloud/cloud-commitments-cli#1641 itself (5f850f214), and destroy-fargate-dev.yml, aws_sanity.yml, azure_sanity.yml and the remaining files have none.

Once LeanerCloud/cloud-commitments-cli#1657 merges this drops to 136 across 6 files.

How the count was produced

Deliberately not grep '\${{'. A grep cannot distinguish a run: body from an env: value, a with: input, an if: condition, or a comment, and most ${{ }} in these files are in positions where it is correct and expected. The count comes from parsing the YAML and walking only jobs.*.steps[].run scalars:

import yaml, re
INTERP = re.compile(r"\$\{\{")
doc = yaml.safe_load(open(path))
for job_name, job in (doc.get("jobs") or {}).items():
    for step in (job.get("steps") or []):
        run = step.get("run")
        if isinstance(run, str):
            for lineno, line in enumerate(run.splitlines(), 1):
                if INTERP.search(line):
                    print(job_name, step.get("name"), lineno, line.strip())

Negative control (worth keeping when re-running this): point the same parser at a known-bad file before trusting a 0. Run against deploy-aws-lambda.yml at LeanerCloud/cloud-commitments-cli#1657's merge-base it reports 23 hits; against LeanerCloud/cloud-commitments-cli#1657's head it reports 0. A parser that reports 0 everywhere because it silently failed to find jobs looks identical to a clean repo.

Why this is worth finishing

Not every one of the 159 is exploitable. Many interpolate values that are not attacker-influenced (github.sha, needs.*.result). The reason to finish the sweep anyway is that the safe ones and the dangerous ones are indistinguishable at a glance, so the invariant "no interpolation in run:" is checkable while "no interpolation of untrusted values in run:" requires re-deriving the trust of every value on every read. LeanerCloud/cloud-commitments-cli#1657's own diff makes this point in a comment: relying on "an upstream job validated this" makes the rule "raw interpolation is fine when someone else checked", which breaks silently the moment the upstream guard moves.

Two properties make the difference concrete:

  1. A value interpolated into a run: block is shell source, so a $(...), backtick, quote, or newline in it is executed or breaks parsing. Via env: it is inert data.
  2. Several of these jobs hold id-token: write and can assume the AWS deploy role, so code execution in them is credential access, not just a broken build.

Useful context for urgency

The release path has never fired. The repository has zero remote tags and zero GitHub releases (git ls-remote --tags origin is empty; gh release list is empty). The four tags that exist are local-only backup tags that were never pushed. So release-triggered interpolation paths are latent rather than exercised, which argues for doing this properly rather than urgently.

Suggested approach

One PR per workflow file, matching how LeanerCloud/cloud-commitments-cli#1641 and LeanerCloud/cloud-commitments-cli#1657 have done it. Each is mechanical and independently reviewable: move each interpolated value to a job- or step-level env: entry and reference it quoted ("$VAR"). Nothing else.

In particular, do not add set -euo pipefail to blocks that lack it as part of the same change. It is worth doing, but it alters whether a step fails on a non-zero intermediate command, so it turns a diff a reviewer can check by inspection into one that needs the workflow exercised, on workflows that deploy. Separate pass, separate PR.

Remedy simplified (2026-08-03): the set -euo pipefail addition was split out of the sweep. It is a behaviour change riding along with a mechanical one, and it is the part that would stop each PR from being reviewable by inspection.

A lint step that runs the parser above over .github/workflows/ and fails on any hit would keep the count at zero once the sweep lands. That is probably worth a follow-up issue rather than part of this one.

Related

No activity

Activity on this issue will appear here.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions