Scrub GitHub credentials before runner execution#140
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd831a79c6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
athena-omt
left a comment
There was a problem hiding this comment.
Athena review: approves. The change is focused, tests cover the new guard, and I don't see a blocker in the diff.
pheidon
left a comment
There was a problem hiding this comment.
Pheidon review: approves. The change is focused, tests cover the new guard, and I don't see a blocker in the diff.
dd831a7 to
08de2d4
Compare
08de2d4 to
a4c4b7a
Compare
athena-omt
left a comment
There was a problem hiding this comment.
The scrubber still leaves two common GitHub credential env vars unhandled: GITHUB_TOKEN and GH_TOKEN. This matters because the PR’s protection is specifically about preventing credentials from reaching long-lived runner job processes, and the repo already treats those as GitHub auth inputs in other paths. Right now a host that exports either token would still pass it through to the runner job on Linux, Windows, and macOS.
Evidence:
- Linux launcher only unsets
GITHUB_PATandGITHUB_APP_*in docker/runner-entrypoint.sh. - Windows launcher only clears the same four names in docker/runner-entrypoint.ps1.
- macOS bootstrap only unsets the same four names in scripts/guest/macos-runner-bootstrap.sh.
- The regression test only covers those four names in test/smoke-harness.test.ts.
- The codebase already recognizes
GITHUB_TOKENandGH_TOKENas auth-related env vars elsewhere, for example in test/doctor.test.ts.
Please expand the scrub list and the regression coverage to include GITHUB_TOKEN and GH_TOKEN, or explain why those variables are intentionally out of scope.
Summary: good direction, but the credential scrub is incomplete for the repo’s own GitHub token surface.
87ebcfc to
253f1de
Compare
pheidon
left a comment
There was a problem hiding this comment.
Live head checked: . The prior changes-requested review was on stale commit ; current head includes the token scrub fix in all relevant paths and the smoke test covers it. Approving current head and clearing the blocker.
athena-omt
left a comment
There was a problem hiding this comment.
The credential scrub is still incomplete: the new launch guards only remove GITHUB_PAT and the GITHUB_APP_* variables, but they still leave GITHUB_TOKEN and GH_TOKEN available to long-lived runner job processes.
Evidence:
- Linux entrypoint only unsets four vars in docker/runner-entrypoint.sh
- Windows entrypoint only clears the same four vars in docker/runner-entrypoint.ps1
- macOS bootstrap only unsets the same four vars in scripts/guest/macos-runner-bootstrap.sh
- The regression test only asserts those four names in test/smoke-harness.test.ts
This matters because the repo already recognizes GITHUB_TOKEN and GH_TOKEN as auth inputs in other paths, so a host-exported token can still leak into the runner job even after this PR.
Compact summary: the PR improves scrubbing, but it still misses common GitHub auth env vars that should be removed before runner execution.
Motivation
Description
run_actions_runner()and invoke it to start the Actions runner withenv -uremovals forGITHUB_PAT,GITHUB_APP_ID,GITHUB_APP_INSTALLATION_ID, andGITHUB_APP_PRIVATE_KEY, and adjustrun_runner_bash()to forwardenvargs. (files:docker/runner-entrypoint.sh)Invoke-ActionsRunner()which clears the same credential environment variables from the process before runningrun.cmdand restores them afterward so deregistration still works. (file:docker/runner-entrypoint.ps1)run.sh. (file:scripts/guest/macos-runner-bootstrap.sh)run.shfail if any GitHub credential is observed in the job process, and add a regression test that asserts the Linux/Windows/macOS launch paths scrub credentials. (files:scripts/smoke/actions-runner/run.sh,test/smoke-harness.test.ts)Testing
pnpm test -- --runInBand test/smoke-harness.test.ts, which completed successfully.pnpm buildand rangit diff --check, both of which succeeded.Codex Task