Skip to content

Workflow artifact guard accepts a raw log copied over an allowed report name #609

Open

Description

@MaxGhenis

The gap

The workflow-artifact privacy check, policyengine_uk_data/tests/test_workflow_artifacts.py, allows six controlled report paths when their reviewed producer text appears before the upload. These are fit_summary.json, distribution_report.json and aggregate_result.json, each in the test and base-build jobs (lines 92, 100, 110, 134, 143 and 153; checked at 347–349).

The check proves the producer's text is present. It doesn't prove the uploaded file is what the producer wrote. So a later step that copies a raw calibration log over an allowed name still passes:

- run: cp constituency_calibration_log.csv fit_summary.json   # inserted before the upload

problems("pull_request.yaml", workflow) returns [] for all six names after this mutation. actions/upload-artifact uploads the bytes as they are. The downstream comparison would then fail to parse the file, but only after the artifact is uploaded.

Scope

This is not a current leak. The workflow at release head 21d5dbf9 (#608) has no such overwrite, and #603 documented this limit at test_workflow_artifacts.py:31–38: the patterns show text presence, not execution, order or content. The independent review of #608 found the gap and rated it P3. This issue asks for the stronger guarantee.

Options

  1. Content check before upload. A step validates each allowed file against its schema (for example policyengine-uk-data-fit-summary/3) and fails on anything else, including CSV bytes.
  2. Controlled staging. Producers write into a fresh directory that only they write to. The upload takes from there, and the guard rejects any other write to that directory.
  3. Make the guard reject writes to an allowed path by any step other than its producer.

Option 1 is the smallest change and also catches accidental mistakes.

Reproducer

This uses a synthetic CSV only: no microdata, no build, no network.

from pathlib import Path
import importlib.util, yaml

path = Path("policyengine_uk_data/tests/test_workflow_artifacts.py")
spec = importlib.util.spec_from_file_location("guard", path)
guard = importlib.util.module_from_spec(spec); spec.loader.exec_module(guard)
wf = yaml.safe_load(Path(".github/workflows/pull_request.yaml").read_text())
steps = wf["jobs"]["test"]["steps"]
i = next(i for i, s in enumerate(steps)
         if s.get("uses", "").startswith("actions/upload-artifact@")
         and s["with"]["path"] == "fit_summary.json")
steps.insert(i, {"run": "cp constituency_calibration_log.csv fit_summary.json"})
assert guard.problems("pull_request.yaml", yaml.safe_load(yaml.safe_dump(wf))) == []

A test for the fix: this mutation should produce a problem for each of the six names.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions