From d62335bc8d6e95cd800325b77e7c9a66c67c0991 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 5 Sep 2026 08:53:43 +0200 Subject: [PATCH] =?UTF-8?q?fix(885):=20an=20English=20full=20stop=20is=20n?= =?UTF-8?q?ot=20a=20secret=20name=20=E2=80=94=20the=20detector=20reads=20`?= =?UTF-8?q?secrets`=20only=20inside=20an=20expression=20span?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_SECRET_REF` ran over the whole string while the comment above it claimed every pattern was confined to `${{ }}` spans. Executed against the shipped module, `secret_refs("# We deliberately pass no secrets. Then the pull is anonymous.")` returned `['Then']`, and fed through the real collector that is one fault reading "job `j` names stored secret(s) on the pull_request route: Then" — a fabricated name, on a PR-route job, for its own comment. The same comment separately reddened the text-versus-walk cross-check, because the line-level strip removes a `#` line from the text half only. The existing negative control passed for a reason that does not generalise: no `.` follows the word in `"no secrets are used here"`. A sentence ENDING in "secrets." is the likeliest thing to be written into a PR-route `run:` block on this branch's own subject, so the trap was self-inflicted. `secret_refs` now resolves names per `${{ }}` span, so every spelling is scoped the way the residue counter already was. The added rows drive the real predecessor — `_SECRET_REF` applied to the whole string — and assert it read a name where the scoped reader reads none, so reverting the scoping reddens them. The `INJECTED_SECRETS` comment stops calling the injected `GITEA_TOKEN` "bounded by the workflow's own `permissions:`": on this route the head supplies that file and can delete the block. Allow-listing it is a claim about the store it is not in, not about a bound. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV --- .../test_workflow_persist_credentials.py | 49 +++++++++++++++---- 1 file changed, 40 insertions(+), 9 deletions(-) diff --git a/scripts/tests/test_workflow_persist_credentials.py b/scripts/tests/test_workflow_persist_credentials.py index 2509a7efc..2ff80f83b 100644 --- a/scripts/tests/test_workflow_persist_credentials.py +++ b/scripts/tests/test_workflow_persist_credentials.py @@ -333,9 +333,11 @@ def test_no_workflow_lives_under_dot_GITHUB() -> None: # The repo's Actions secret store, read 2026-09-04: GH_COM_TOKEN, REGISTRY_PASSWORD, REGISTRY_USER, # RENOVATE_TOKEN, SERVERMGMT_DEPLOY_KEY. `GITEA_TOKEN` is deliberately NOT one of them — it is the -# per-run token Gitea injects, bounded by the workflow's own `permissions:` block, and a head-authored -# run receives it whether or not any job names it. Allow-listing it is therefore a statement about a -# MECHANISM (injected, scoped, unavoidable) and not an exemption for a site, which is why it is a +# per-run token Gitea injects, and a head-authored run receives it whether or not any job names it. +# Its `permissions:` block narrows it for the COMMITTED file only — on this route the head supplies +# that file and can delete the block — so allow-listing it is NOT a claim that it is bounded. It is a +# statement about a MECHANISM (injected, unavoidable, out of the store this guard is about) and not +# an exemption for a site, which is why it is a # closed one-member set rather than a list that can grow: a second entry would be an exemption, and # an exemption outlives its reason silently. INJECTED_SECRETS = frozenset({"GITEA_TOKEN"}) @@ -378,9 +380,13 @@ _SECRET_REF = re.compile( # grammar: any spelling of the context that does not yield a literal name is reported, including one # nobody has written yet. # -# Both patterns are confined to `${{ }}` spans, because a bare `secrets` is otherwise ordinary -# English — this file and four workflows discuss "secrets" in prose, and matching that would fault -# every job carrying a comment. Inside an expression the word is the context and nothing else. +# EVERY pattern here reads only inside `${{ }}` spans, `_SECRET_REF` included, because outside one +# the word is ordinary English and `secrets.` is a sentence boundary. That scoping is enforced in +# `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. _EXPRESSION = re.compile(r"\$\{\{(.*?)\}\}", re.S) _SECRETS_TOKEN = re.compile(r"(? list[str]: spelling widens both. A list rather than a set for the reason `secret_name_counts` is a Counter: only the lossless direction can be narrowed afterwards. """ - found = [next(group for group in m.groups() if group is not None) for m in _SECRET_REF.finditer(text)] + found: list[str] = [] for expression in _EXPRESSION.findall(text): - unresolved = len(_SECRETS_TOKEN.findall(expression)) - len(_SECRET_REF.findall(expression)) - found.extend([WHOLE_SECRETS_CONTEXT] * unresolved) + 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))) return found @@ -794,6 +802,29 @@ def test_the_collector_sees_every_SPELLING_of_a_secret_reference() -> None: prose = {True: {"pull_request": None}, "jobs": {"j": {"steps": [{"run": "echo 'no secrets here'"}]}}} assert stored_secret_faults("synthetic.yml", prose) == [] + # The prose row above passes for a reason that does not generalise — no `.` follows the word. An + # English sentence ENDING in "secrets." is the shape that reached the unscoped predecessor, and + # it is the likeliest sentence to be written into a PR-route `run:` block on this branch's own + # subject. Driven against that predecessor: `unscoped` is `_SECRET_REF` applied to the whole + # string, which is how `secret_refs` read before the span scoping. + for sentence in ( + "# We deliberately pass no secrets. Then the pull is anonymous.", + "echo 'this job holds no secrets. Anonymous pull only'", + ): + unscoped = [next(group for group in m.groups() if group is not None) for m in _SECRET_REF.finditer(sentence)] + assert unscoped, ( + f"{sentence!r} is supposed to be a sentence the unscoped predecessor read as a secret " + "name — if it is not, this row proves nothing about the scoping." + ) + assert secret_refs(sentence) == [], sentence + commented = {True: {"pull_request": None}, "jobs": {"j": {"steps": [{"run": f"{sentence}\ntrue"}]}}} + assert stored_secret_faults("synthetic.yml", commented) == [], sentence + # The same comment reddened the text-versus-walk cross-check separately, and for a different + # mechanism: the strip is LINE-level, so a fabricated name inside a `run:` scalar was removed + # from the text half only and the two halves disagreed on a name no workflow ever held. + 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 + def test_the_collector_reports_a_WORKFLOW_SCOPE_reference_no_job_if_can_reach() -> None: """The workflow scope is judged even when every job is gated OFF the route.