diff --git a/scripts/tests/test_workflow_persist_credentials.py b/scripts/tests/test_workflow_persist_credentials.py index 55b6b84c8..430f9db7c 100644 --- a/scripts/tests/test_workflow_persist_credentials.py +++ b/scripts/tests/test_workflow_persist_credentials.py @@ -390,11 +390,43 @@ _SECRET_REF = re.compile( # `secret_refs` below rather than in the patterns, which cannot express "within the enclosing span". # Applied to the whole string instead, `_SECRET_REF` reads the English `# We pass no secrets. Then # the pull is anonymous.` as a reference to a secret named `Then` — a fabricated name, faulting a -# PR-route job for a comment, on this branch's own subject. A reference the runner actually resolves -# is always inside an expression, so the scoping costs no real spelling. +# PR-route job for a comment, on this branch's own subject. +# +# THE SCOPING COSTS EXACTLY ONE REAL SPELLING, AND IT IS NOT NOTHING: an `if:` value is an expression +# WHETHER OR NOT it is wrapped, so `if: secrets.REGISTRY_PASSWORD != ''` names a stored secret in a +# document that contains no `${{` at all. That an unwrapped condition is evaluated rather than read +# as text is demonstrated by this repo's own `docker-build.yml` and not inferred — `build` carries +# `if: github.event_name != 'pull_request'` bare, and `PR_EXCLUDING_IFS` below pins that exact +# unwrapped string. `if:` is the only key whose value the grammar lets omit the delimiters, so the +# scoping is repaired AT THAT KEY rather than abandoned: `condition_refs` reads an `if:` value as one +# span, and `secret_name_counts` routes the value there instead of through `secret_refs`. Everywhere +# else the scoping stands, and the English sentence above still costs nothing. _EXPRESSION = re.compile(r"\$\{\{(.*?)\}\}", re.S) _SECRETS_TOKEN = re.compile(r"(? list[str]: + """Every `secrets` reference inside ONE expression span — the resolved names, then the residue. + + Shared by both entry points, so that widening a spelling widens the wrapped and the unwrapped + reading together: a second copy of this resolution would be free to drift from the one the + assertion runs on, which is the shape this guard exists to catch. + """ + resolved = [next(group for group in m.groups() if group is not None) for m in _SECRET_REF.finditer(expression)] + # The residue: `secrets` tokens in this span that resolved to no literal name. + residue = len(_SECRETS_TOKEN.findall(expression)) - len(resolved) + return resolved + [WHOLE_SECRETS_CONTEXT] * residue + def secret_refs(text: str) -> list[str]: """Every `secrets` reference in one string — occurrences, not names. @@ -405,13 +437,22 @@ def secret_refs(text: str) -> list[str]: """ found: list[str] = [] for expression in _EXPRESSION.findall(text): - resolved = [next(group for group in m.groups() if group is not None) for m in _SECRET_REF.finditer(expression)] - found.extend(resolved) - # The residue: `secrets` tokens in this span that resolved to no literal name. - found.extend([WHOLE_SECRETS_CONTEXT] * (len(_SECRETS_TOKEN.findall(expression)) - len(resolved))) + found.extend(_refs_in_expression(expression)) return found +def condition_refs(condition: str) -> list[str]: + """Every `secrets` reference in an `if:` value, which is an expression with or without `${{ }}`. + + The WHOLE value is read as one span, with the delimiters neutralised rather than honoured, so + that a condition mixing the two forms — `${{ true }} && secrets.X != ''` — is covered by the same + read as the bare one, and a fully wrapped condition still counts each reference exactly once. + This is strictly more demanding than `secret_refs` on the same string and never less: an `if:` is + never prose, so the over-match the span scoping exists to avoid cannot arise here. + """ + return _refs_in_expression(_EXPRESSION_DELIMITER.sub(" ", condition)) + + # The ONLY job-level `if:` in this repo that takes a job OFF the `pull_request` route. This is a PIN, # not an expression parser, and the direction is the point: an `if:` that is not in this set leaves # the job IN the population, so an unrecognised guard reddens rather than exempting. Parsing @@ -483,6 +524,12 @@ def secret_name_counts(node: object) -> Counter: only this direction is lossless: `secret_names` is derived from it below. A second traversal with a different accumulator would be a copy of a mechanism, free to drift from the one the assertion runs on — the shape this guard exists to catch, in the guard itself. + + One key is read differently, and it is a KEY rather than a place in the document: the value of an + `if:` is an expression with or without `${{ }}`, so it goes through `condition_refs`. Read as an + ordinary string it would be scoped to its `${{ }}` spans and a bare `if: secrets.X != ''` would + be invisible, on the route where the head writes the file. The value is routed there INSTEAD of + onto the stack, so a wrapped condition is counted once rather than twice. """ found: Counter = Counter() stack: list[object] = [node] @@ -491,7 +538,10 @@ def secret_name_counts(node: object) -> Counter: if isinstance(item, dict): for key, value in item.items(): stack.append(key) - stack.append(value) + if isinstance(key, str) and key.lower() == IF_KEY and isinstance(value, str): + found.update(condition_refs(value)) + else: + stack.append(value) elif isinstance(item, list): stack.extend(item) elif isinstance(item, str): @@ -682,6 +732,13 @@ def test_the_DOCUMENT_walk_finds_every_secret_reference_the_TEXT_does() -> None: A YAML anchor/alias would also redden benignly (the walk visits the aliased node once per reference, the text carries `*alias`); measured 2026-09-05 no tracked workflow uses one. + A THIRD asymmetry, which is not the strip's: an `if:` value is read by `condition_refs`, so an + UNWRAPPED `if: secrets.X != ''` is counted by the walk and not by the text half, which has no key + to read it by and is scoped to `${{ }}` spans. The walk's count is then the larger one, which + reddens — the safe direction, and on a document that is already faulting the stored-secret + assertion for the same reference. Measured 2026-09-05, no tracked workflow names a secret in an + `if:` at all, so nothing in the tree reaches it. + WHAT THIS CROSS-CHECK STRUCTURALLY CANNOT REPORT, since it is the reason `secret_refs` has to be widened rather than leaned on: both halves read through that one function, so a spelling IT does not recognise is invisible to both and they agree at zero. The dot/index/whole-context spellings @@ -905,6 +962,57 @@ def test_the_collector_sees_every_SPELLING_of_a_secret_reference() -> None: as_text = f"jobs:\n j:\n steps:\n - run: |\n {sentence}\n true\n" assert walk_versus_text_faults("synthetic.yml", as_text) == [], sentence + # AND THE SPELLING THAT SCOPING COSTS: an `if:` value is an expression whether or not it is + # wrapped, so a condition naming a stored secret contains no `${{` and the span scoping cannot + # see it. `docker-build.yml`'s own `build` job carries an unwrapped `if:`, so this is the shape + # the repo already writes and not a hypothetical. Driven against the REAL predecessor: reading + # the value as an ordinary string — which is how `secret_name_counts` routed it before + # `condition_refs` — is `secret_refs`, and it is asserted empty on every row. + condition = "secrets.REGISTRY_PASSWORD != ''" + assert secret_refs(condition) == [], ( + "an unwrapped condition is supposed to be invisible to the span-scoped reader — if it is " + "not, these rows prove nothing about `condition_refs`." + ) + assert condition_refs(condition) == ["REGISTRY_PASSWORD"] + + job_level = {True: {"pull_request": None}, "jobs": {"j": {"if": condition, "steps": [{"run": "true"}]}}} + step_level = { + True: {"pull_request": None}, + "jobs": {"j": {"steps": [{"if": "secrets.RENOVATE_TOKEN != ''", "run": "true"}]}}, + } + for doc, name in ((job_level, "REGISTRY_PASSWORD"), (step_level, "RENOVATE_TOKEN")): + faults = stored_secret_faults("synthetic.yml", doc) + assert len(faults) == 1, (name, faults) + assert name in faults[0], (name, faults) + + # A WRAPPED condition counts once, not twice — the delimiters are neutralised rather than read as + # a second span, so the cross-check still agrees with the text half on the shape workflows write. + wrapped = "jobs:\n j:\n if: ${{ secrets.REGISTRY_PASSWORD != '' }}\n steps:\n - run: true\n" + assert secret_name_counts(yaml.safe_load(wrapped)) == Counter({"REGISTRY_PASSWORD": 1}) + assert walk_versus_text_faults("synthetic.yml", wrapped) == [] + + # A condition MIXING the two forms is read whole, so the unwrapped half is not lost behind the + # wrapped one. + assert condition_refs("${{ true }} && secrets.RENOVATE_TOKEN != ''") == ["RENOVATE_TOKEN"] + # And two wrapped references in one condition stay two: the delimiters become a SPACE, so the + # names cannot collapse into one identifier. + assert condition_refs("${{ secrets.REGISTRY_USER }}${{ secrets.REGISTRY_PASSWORD }}") == [ + "REGISTRY_USER", + "REGISTRY_PASSWORD", + ] + + # The clause is on the `if:` KEY, so the English sentences above are untouched by it: they are + # `run:` scalars, where the word is prose and the scoping still costs nothing. + still_clean = { + True: {"pull_request": None}, + "jobs": {"j": {"if": "github.event_name == 'push'", "steps": [{"run": "# We pass no secrets. Then true"}]}}, + } + assert stored_secret_faults("synthetic.yml", still_clean) == [] + + # The pinned exclusion is itself an unwrapped condition and must stay clean — it names no secret, + # and reading conditions must not start faulting every gated job. + assert condition_refs("github.event_name != 'pull_request'") == [] + def test_a_SECRETS_HANDOVER_naming_nothing_is_reported_under_the_WHOLE_CONTEXT_sentinel() -> None: """`secrets: inherit` hands the WHOLE store to a `uses:` job while naming no secret at all.