From a51e55e2b1ac7ee503516b69cc18877aa1a42c8a Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Wed, 26 Aug 2026 14:16:10 -0400 Subject: [PATCH] fix(tests): the signing pins assert the guarantee where #558 moved it, and one they missed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `main` is red because I merged #558 without running the suite. Three tests in `test_desktop_packaging.py` assert the signing gates by looking for `secrets.X != ''` inside each step's `if:` -- the exact expression #558 had to remove, because GitHub rejects `secrets` in an `if:` at parse time. The GUARANTEE is unchanged and is still pinned: all five Apple credentials or none, both Windows credentials or none, and each skip notice firing on the exact complement of its sign step. What moved is where that is decided -- from four `if:` conditions into one `signing` step whose outputs the conditions read -- so the assertions move with it. ── AND A PIN FOR THE FAILURE THESE TESTS COULD NOT SEE ──────────────────────── The old tests asserted that a string appeared in a condition. That says nothing about whether GitHub can PARSE the file, which is why they were green for the three days the workflow was undispatchable. Two new pins close that: * no `secrets` in any `if:`, in any job or step -- the thing that made the whole workflow invalid; * no signing credential in a job-level `env:` -- the obvious fix for the first one, and the wrong one, because it exposes every certificate to `uv sync`, the PyInstaller build and every other step in the job. All three are mutation-checked: restoring a secret to an `if:` fails two tests, dropping one credential from the gate fails a third, and hoisting a secret to job-level env fails the fourth. ── THE PROCESS FAILURE, RECORDED ────────────────────────────────────────────── I validated #558's YAML, checked its structure, and let GitHub's own parser confirm the fix -- and did not run `pytest`, because it was "only a workflow file". This repository pins its workflows WITH tests; that is the whole reason those three existed. The suite is not optional on a workflow change here. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01T6yA5khYnJ2qzheArRToQ2 --- tests/test_desktop_packaging.py | 120 ++++++++++++++++++++++++++------ 1 file changed, 98 insertions(+), 22 deletions(-) diff --git a/tests/test_desktop_packaging.py b/tests/test_desktop_packaging.py index a0c5d6e..de88514 100644 --- a/tests/test_desktop_packaging.py +++ b/tests/test_desktop_packaging.py @@ -419,39 +419,116 @@ def test_the_signing_secrets_live_on_a_protected_environment(desktop_job: dict) ) +def test_the_signing_gate_is_a_step_and_not_an_if_condition(workflow: dict) -> None: + """**`secrets` is not a context GitHub allows in a step-level `if:`.** + + The four signing steps were gated as `if: ... && secrets.MACOS_CERT_P12_BASE64 != ''` from + #512 until #558. GitHub rejects that at PARSE time -- "Unrecognized named-value: 'secrets'" -- + so the whole workflow was invalid and `Release` could not be dispatched at all for three days. + It hid well: an unparseable workflow cannot report its own name, so every failure was filed + under the file path rather than `name: Release`, with zero jobs, on a `push` event, for a + workflow that is `workflow_dispatch`-only. + + These tests did not catch it, and could not have as written: they asserted the presence of a + string in a condition, which says nothing about whether GitHub can parse the file. So this one + pins the SHAPE that keeps it parseable -- no `secrets` in any `if:`, anywhere in the workflow. + """ + offenders = [] + for job_name, job in workflow["jobs"].items(): + if "secrets." in str(job.get("if", "")): + offenders.append(f"job {job_name}") + for step in job.get("steps", []): + if "secrets." in str(step.get("if", "")): + offenders.append(f"{job_name} / {step.get('name', '?')}") + assert not offenders, ( + "`secrets` in an `if:` makes the whole workflow unparseable and undispatchable: " + f"{offenders}. Compute the gate in a step and read its output instead." + ) + + +def test_the_signing_secrets_are_not_exposed_to_every_step(desktop_job: dict) -> None: + """The obvious fix for the above is a job-level `env:` block, because `env` IS allowed in + `if:`. It is the wrong one here. + + That would put every signing certificate and notarisation key into the environment of EVERY + step in this job -- `uv sync`, the PyInstaller build, the smoke test -- which is precisely the + exposure the `signing` environment exists to prevent. A certificate reachable by a compromised + build dependency is not protected by having been fetched from a reviewed environment. + """ + job_env = desktop_job.get("env") or {} + leaked = [k for k, v in job_env.items() if "secrets." in str(v)] + assert not leaked, ( + f"job-level env exposes {leaked} to every step in the desktop job; scope signing " + "credentials to the one step that reads them" + ) + + +def test_the_signing_gate_requires_every_credential(desktop_job: dict) -> None: + """**All FIVE or none.** A signed-but-un-notarised app is the worst state on macOS -- it still + trips Gatekeeper, and now looks like it tried not to -- so the gate is the whole Apple set: + the Developer ID Application `.p12` to sign, and the App Store Connect API key trio + `notarytool` needs. #402's lesson is measured per component, never by one token. + + Asserted on the gate STEP since #558, which is where that decision now lives. Every secret + must be readable by the step (its own `env`) and tested by it (`-n "$SECRET"`), so a partial + credential set produces `no` and the sign step is skipped. + """ + gate = _step(desktop_job, "Can this leg sign?") + assert gate.get("id") == "signing", "the gate step must be addressable as `steps.signing`" + assert gate.get("shell") == "bash", ( + "the gate runs on macOS AND Windows runners, where the default shell is pwsh" + ) + + env = gate.get("env") or {} + run = str(gate.get("run", "")) + for secret in _MACOS_SIGNING_SECRETS + _WINDOWS_SIGNING_SECRETS: + assert f"secrets.{secret}" in str(env.get(secret, "")), ( + f"the gate step cannot read {secret}, so it cannot require it" + ) + assert f'-n "${secret}"' in run, ( + f"the gate must test {secret} -- a partial credential set must skip, not half-sign" + ) + + # Only a boolean leaves the step. An output is visible in the run's API payload, so nothing + # derived from a certificate may be written to one. + assert 'echo "macos=$macos" >> "$GITHUB_OUTPUT"' in run + assert 'echo "windows=$windows" >> "$GITHUB_OUTPUT"' in run + for secret in _MACOS_SIGNING_SECRETS + _WINDOWS_SIGNING_SECRETS: + assert f'{secret}" >> "$GITHUB_OUTPUT' not in run, f"{secret} is written to an output" + + def test_macos_signing_runs_only_when_every_apple_credential_exists(desktop_job: dict) -> None: - """All FIVE or none: a signed-but-un-notarised app is the worst state on macOS (it - still trips Gatekeeper, and now looks like it tried not to), so the gate is the whole - Apple set -- the Developer ID Application .p12 to sign, and the App Store Connect API - key trio notarytool needs. #402's lesson is measured per component, not by one token.""" + """The sign step reads the gate's verdict and nothing else.""" condition = str(_step(desktop_job, "Sign, notarise and staple (macOS)").get("if", "")) assert "runner.os == 'macOS'" in condition - for secret in _MACOS_SIGNING_SECRETS: - assert f"secrets.{secret} != ''" in condition, ( - f"the macOS sign step must require {secret} -- a partial credential set must " - "skip, not half-sign" - ) + assert "steps.signing.outputs.macos == 'yes'" in condition, ( + "the macOS sign step must gate on the credential check in `steps.signing`" + ) def test_windows_signing_runs_only_when_the_certificate_exists(desktop_job: dict) -> None: condition = str(_step(desktop_job, "Sign the installer (Windows)").get("if", "")) assert "runner.os == 'Windows'" in condition - for secret in _WINDOWS_SIGNING_SECRETS: - assert f"secrets.{secret} != ''" in condition + assert "steps.signing.outputs.windows == 'yes'" in condition def test_each_skip_notice_names_every_missing_secret_and_the_price_of_fixing_it( desktop_job: dict, ) -> None: - """The honest skip: each notice fires on the exact COMPLEMENT of its sign step's gate, - and says what to buy, which secrets to create, and where the checklist is -- so a - reader of a green run learns signing was SKIPPED, never believes it happened, and - knows the purchase that would turn it on (#438's signing table, restated as text).""" + """The honest skip: each notice fires on the exact COMPLEMENT of its sign step's gate, and + says what to buy, which secrets to create, and where the checklist is -- so a reader of a + green run learns signing was SKIPPED, never believes it happened, and knows the purchase that + would turn it on (#438's signing table, restated as text). + + The COMPLEMENT is asserted literally: `!= 'yes'` against the sign step's `== 'yes'`. Written + as two independent conditions they could drift into a state where neither fires and a release + goes out silently unsigned, which is the failure this pairing exists to make impossible. + """ cases = [ - ("Notice: macOS signing skipped", _MACOS_SIGNING_SECRETS, "$99"), - ("Notice: Windows signing skipped", _WINDOWS_SIGNING_SECRETS, "SmartScreen"), + ("Notice: macOS signing skipped", _MACOS_SIGNING_SECRETS, "$99", "macos"), + ("Notice: Windows signing skipped", _WINDOWS_SIGNING_SECRETS, "SmartScreen", "windows"), ] - for name, secrets, product in cases: + for name, secrets, product, leg in cases: notice = _step(desktop_job, name) condition = str(notice.get("if", "")) run = str(notice.get("run", "")) @@ -461,11 +538,10 @@ def test_each_skip_notice_names_every_missing_secret_and_the_price_of_fixing_it( f"{name} must name the paid product that unlocks signing -- the reader is " "being asked to accept an unsigned binary, and the price is the context" ) + assert f"steps.signing.outputs.{leg} != 'yes'" in condition, ( + f"{name} must fire on the exact complement of the sign step's gate" + ) for secret in secrets: - assert f"secrets.{secret} == ''" in condition, ( - f"{name} must fire when {secret} is missing -- every gap in the gate " - "needs its explanation" - ) assert secret in run, ( f"{name} must NAME {secret} -- a notice that says only 'not configured' " "has already been failed by code-quality.yml's preflight prose (#402)"