From e5cc1b5255640cfce6abbbf6af4de919d3636f84 Mon Sep 17 00:00:00 2001 From: Abdessamad Derraz <3028866+Abdess@users.noreply.github.com> Date: Tue, 27 Oct 2026 11:48:22 +0100 Subject: [PATCH] refactor: keep tokens out of autorun job steps Every step, gate and recomputation runs with an allowlisted environment (PATH, HOME, LANG, LC_*, TZ, TMPDIR and the variables its env: key names). Names holding TOKEN, SECRET or PASS are dropped and refused in the registry. Only the push and the gh calls that read CI get GITHUB_TOKEN. --- automation/jobs.yml | 12 +++++-- scripts/autorun.py | 76 ++++++++++++++++++++++++++++++++----------- tests/test_autorun.py | 40 +++++++++++++++++++++++ 3 files changed, 106 insertions(+), 22 deletions(-) diff --git a/automation/jobs.yml b/automation/jobs.yml index f78cb7eca..94f49bb95 100644 --- a/automation/jobs.yml +++ b/automation/jobs.yml @@ -4,9 +4,12 @@ # Commands run in the automation worktree (.cache/autorun/worktree) unless # `worktree: false`, which runs them in the main checkout and forbids # commits. `python` names the interpreter running autorun.py. A step is a -# command list, or a mapping with `run`, `ok_codes` and `stdout` (a file -# the command's output is written to). `twice` recomputes an artifact the -# gate wrote into `{out}` and compares both, timestamps aside. Each commit +# command list, or a mapping with `run`, `ok_codes`, `stdout` (a file the +# command's output is written to) and `env` (variables passed through +# beside PATH, HOME, LANG, LC_*, TZ and TMPDIR). A step never sees a token +# or a password: only the push and gh receive GITHUB_TOKEN. `twice` +# recomputes an artifact the gate wrote into `{out}` and compares both, +# timestamps aside. Each commit # takes only the changed files its `paths` globs match, and a job that # leaves any other change behind fails. `report` names a JSON file a step # writes, kept in the job's status. @@ -94,6 +97,9 @@ jobs: - run: [python, scripts/campaign.py, run, --budget, 150m] ok_codes: [0, 75] stdout: tmp/autorun/campaign.json + env: [RETROBIOS_ROMFETCH, RETROBIOS_AUTO_LOCK, ROMFETCH_CONTENT_DIR, ROMFETCH_CACHE_DIR, + ROMFETCH_DATS_DIR, ROMFETCH_RETOOL_DIR, ROMFETCH_MIN_INTERVAL, ROMFETCH_MAX_SIZE, + ROMFETCH_DISABLED_SOURCES, XDG_CONFIG_HOME] gate: - [python, scripts/campaign.py, gate] commits: diff --git a/scripts/autorun.py b/scripts/autorun.py index 693bba54e..5921e46ae 100644 --- a/scripts/autorun.py +++ b/scripts/autorun.py @@ -102,6 +102,8 @@ class Step: argv: tuple[str, ...] ok_codes: tuple[int, ...] = (0,) stdout: str = "" + # Variables passed through beside the allowlist (romfetch's settings). + env: tuple[str, ...] = () @dataclass(frozen=True) @@ -220,13 +222,18 @@ def _step(raw: object, where: str) -> Step: if isinstance(raw, list): return Step(argv=tuple(str(a) for a in raw)) if isinstance(raw, dict) and isinstance(raw.get("run"), list): - unknown = set(raw) - {"run", "ok_codes", "stdout"} + unknown = set(raw) - {"run", "ok_codes", "stdout", "env"} if unknown: raise ValueError(f"{where}: unknown step key {sorted(unknown)}") + names = tuple(str(n) for n in raw.get("env") or ()) + secret = [n for n in names if is_secret(n)] + if secret: + raise ValueError(f"{where}: a step never receives {', '.join(secret)}") return Step( argv=tuple(str(a) for a in raw["run"]), ok_codes=tuple(int(c) for c in raw.get("ok_codes", (0,))), stdout=str(raw.get("stdout") or ""), + env=names, ) raise ValueError(f"{where}: a step is a command list or a mapping with 'run'") @@ -463,9 +470,10 @@ def twice( # Git -def git(cwd: Path, *args: str, check: bool = True) -> subprocess.CompletedProcess: +def git(cwd: Path, *args: str, check: bool = True, + env: dict[str, str] | None = None) -> subprocess.CompletedProcess: proc = subprocess.run( - ["git", "-C", str(cwd), *args], capture_output=True, text=True, check=False, + ["git", "-C", str(cwd), *args], capture_output=True, text=True, check=False, env=env, ) if check and proc.returncode != 0: raise AutorunError( @@ -590,7 +598,7 @@ def push_batch(worktree: Path, gate_base: str, regate: Callable[[], bool]) -> st raise AutorunError(BLOCKED, f"rebase onto {upstream} conflicts, batch kept") if git(worktree, "rev-parse", upstream).stdout.strip() != gate_base and not regate(): raise AutorunError(FAILED, f"gate failed on the batch rebased onto {upstream}") - git(worktree, "push", "--quiet", REMOTE, f"HEAD:{TARGET}") + git(worktree, "push", "--quiet", REMOTE, f"HEAD:{TARGET}", env=token_env()) return git(worktree, "rev-parse", "HEAD").stdout.strip() @@ -602,6 +610,7 @@ def gh_json(args: list[str]) -> object: try: proc = subprocess.run( ["gh", *args], capture_output=True, text=True, timeout=120, check=False, + env=token_env(), ) except (OSError, subprocess.TimeoutExpired): return None @@ -724,20 +733,51 @@ def set_alert(state: dict, name: str, active: bool, detail: str, now: str) -> No # Running a job -def job_env(job: Job) -> dict[str, str]: - env = os.environ.copy() - env["RETROBIOS_AUTO"] = "1" +# What a job step inherits. A step can run third-party code (a buildbot +# core, a scraped page, a downloaded tool), so it gets the variables a +# program needs to run and nothing it could leak: no token, no password. +SAFE_ENV = frozenset({"PATH", "HOME", "LANG", "TZ", "TMPDIR"}) +SECRET_NAMES = frozenset({"GITHUB_TOKEN", "GH_TOKEN", "ROMSTATION_USER", "ROMSTATION_PASS"}) +SECRET_MARKERS = ("TOKEN", "SECRET", "PASS") + + +def is_secret(name: str) -> bool: + upper = name.upper() + return name in SECRET_NAMES or any(marker in upper for marker in SECRET_MARKERS) + + +def safe_env(names: Sequence[str] = (), add: dict[str, str] | None = None, + source: dict[str, str] | None = None) -> dict[str, str]: + """The allowlisted environment, plus the variables a step names + explicitly. A secret is dropped even when named.""" + source = dict(os.environ) if source is None else source + wanted = set(names) + env = {key: value for key, value in source.items() + if (key in SAFE_ENV or key.startswith("LC_") or key in wanted) and not is_secret(key)} + env.update({key: value for key, value in (add or {}).items() if not is_secret(key)}) + return env + + +def token_env() -> dict[str, str]: + """The environment of the phases that need GitHub: the push and gh.""" + sys.path.insert(0, str(REPO_ROOT / "scripts")) + from common import github_token + + env = dict(os.environ) + token = github_token() + if token: + env["GITHUB_TOKEN"] = token + return env + + +def job_env(job: Job, step: Step | None = None) -> dict[str, str]: + """A step's environment: the allowlist, the runner's own markers and the + variables the step names. Never a token, network job or not.""" + add = {"RETROBIOS_AUTO": "1"} if job.lock: # A step that takes the host lock itself (campaign.py) runs under ours. - env[LOCK_HELD_ENV] = "1" - if job.network: - sys.path.insert(0, str(REPO_ROOT / "scripts")) - from common import github_token - - token = github_token() - if token: - env["GITHUB_TOKEN"] = token - return env + add[LOCK_HELD_ENV] = "1" + return safe_env(step.env if step else (), add) def _remove_scratch(root: Path, rel: str) -> None: @@ -772,7 +812,6 @@ class Runner: self.cwd = ctx.worktree if job.worktree else ctx.repo self.log = ctx.home / "logs" / job.name / f"{dt.date.today().isoformat()}.log" self.deadline = ctx.host.clock() + job.timeout - self.env: dict[str, str] = {} self.commits: list[str] = [] self.retried: list[str] = [] self.report: object = None @@ -794,7 +833,7 @@ class Runner: left = self.deadline - self.ctx.host.clock() if left <= 0: return Outcome(124, "job timeout reached") - return execute(argv, self.cwd, self.log, self.env, left, stdout) + return execute(argv, self.cwd, self.log, job_env(self.job, step), left, stdout) outcome = run_with_retry(once, step.ok_codes, self.wait_idle) if outcome.retried: @@ -884,7 +923,6 @@ class Runner: refresh_breaker(self.state, ctx.gh, now_iso()) if job.commits and self.state.get("breaker", {}).get("open"): return BLOCKED, "circuit breaker open: a pushed automation commit failed CI" - self.env = job_env(job) base = "" if job.worktree: ensure_worktree(ctx.repo, ctx.worktree) diff --git a/tests/test_autorun.py b/tests/test_autorun.py index 0edaf2388..b65b3a260 100644 --- a/tests/test_autorun.py +++ b/tests/test_autorun.py @@ -19,6 +19,7 @@ import time import unittest import unittest.mock from pathlib import Path +from types import MappingProxyType REPO_ROOT = Path(__file__).resolve().parents[1] sys.path.insert(0, str(REPO_ROOT / "scripts")) @@ -196,6 +197,45 @@ class Guards(Scratch): self.assertIsNone(record["last_success"]) +class StepEnvironment(Scratch): + """A step can run third-party code: it never inherits a token or a password.""" + + HOST = MappingProxyType({ + "PATH": "/usr/bin", "HOME": "/home/u", "LANG": "C.UTF-8", "LC_ALL": "C", "TZ": "UTC", + "TMPDIR": "/t", "GITHUB_TOKEN": "t1", "GH_TOKEN": "t2", "ROMSTATION_USER": "u", + "ROMSTATION_PASS": "p", "NPM_TOKEN": "t3", "AWS_SECRET_ACCESS_KEY": "s", + "DB_PASSWORD": "p2", "ROMFETCH_CONTENT_DIR": "/c", "EDITOR": "vi"}) + + def test_the_allowlist(self): + env = ar.safe_env(["ROMFETCH_CONTENT_DIR", "GITHUB_TOKEN"], {"RETROBIOS_AUTO": "1"}, + source=dict(self.HOST)) + self.assertEqual(env, {"PATH": "/usr/bin", "HOME": "/home/u", "LANG": "C.UTF-8", + "LC_ALL": "C", "TZ": "UTC", "TMPDIR": "/t", + "ROMFETCH_CONTENT_DIR": "/c", "RETROBIOS_AUTO": "1"}) + + def test_a_step_naming_a_secret_is_refused_at_load(self): + for name in ("GITHUB_TOKEN", "ROMSTATION_PASS", "MY_SECRET"): + with self.subTest(name=name), self.assertRaises(ValueError): + ar.parse_jobs({"jobs": {"x": {"steps": [{"run": [PY, "-V"], "env": [name]}]}}}) + + def test_a_network_job_step_sees_no_token(self): + dump = "import json, os; print(json.dumps(dict(os.environ)))" + ctx = self.context({"x": {"worktree": False, "min_free_gb": 0, "network": True, + "steps": [{"run": [PY, "-c", dump], "stdout": "env.json", + "env": ["ROMFETCH_CONTENT_DIR"]}]}}) + with unittest.mock.patch.dict(os.environ, {"GITHUB_TOKEN": "t1", "GH_TOKEN": "t2", + "ROMSTATION_PASS": "p", + "ROMFETCH_CONTENT_DIR": "/c", "EDITOR": "vi"}): + self.assertEqual(ar.run_job(ctx, "x")[0], ar.OK) + given = ar.token_env() + seen = json.loads((ctx.repo / "env.json").read_text()) + self.assertFalse({"GITHUB_TOKEN", "GH_TOKEN", "ROMSTATION_PASS", "EDITOR"} & set(seen)) + self.assertEqual((seen["ROMFETCH_CONTENT_DIR"], seen["RETROBIOS_AUTO"]), ("/c", "1")) + self.assertEqual(seen[ar.LOCK_HELD_ENV], "1") + # The push and gh are the phases that get it. + self.assertEqual(given["GITHUB_TOKEN"], "t1") + + class Retry(Scratch): def _runs(self, *outcomes: ar.Outcome): queue = list(outcomes)