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

@@ -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:

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)

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)