Skip to content

Strip GitHub auth from runner job environment#141

Closed
jmcte wants to merge 1 commit into
mainfrom
codex/fix-vulnerability-in-public-runner-allowlist
Closed

Strip GitHub auth from runner job environment#141
jmcte wants to merge 1 commit into
mainfrom
codex/fix-vulnerability-in-public-runner-allowlist

Conversation

@jmcte

@jmcte jmcte commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Motivation

  • Public/selected Synology runner containers previously inherit long-lived GitHub credentials (GITHUB_PAT and GitHub App envs) which job-level processes can read, creating a credential-exposure vulnerability.

Description

  • Add a new sanitized job launch helper run_runner_job_bash in docker/runner-entrypoint.sh that launches ./run.sh with env -u to unset GITHUB_PAT, GITHUB_APP_ID, GITHUB_APP_INSTALLATION_ID, and GITHUB_APP_PRIVATE_KEY before executing the job process.
  • Switch the runner job start path to use the sanitized run_runner_job_bash so registration/cleanup still use the existing credential-bearing path while jobs run without inherited GitHub auth.
  • Extend the smoke runner stub scripts/smoke/actions-runner/run.sh to fail if any GitHub auth env vars are present and to record the sanitized state, and add an assertion to scripts/smoke-test.sh to verify the job process sees github auth: unset.
  • Key files changed: docker/runner-entrypoint.sh, scripts/smoke/actions-runner/run.sh, and scripts/smoke-test.sh.

Testing

  • Ran the TypeScript build and unit tests with pnpm --silent build && pnpm --silent test, and all tests passed (48 test files, 263 tests succeeded).
  • Performed a shell syntax check bash -n on modified shell scripts which succeeded.
  • Executed the smoke harness pnpm --silent smoke-test but it was blocked in this environment because Docker is not installed (docker: command not found), so full smoke execution could not complete here.

Codex Task

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c8a7aacc07

ℹ️ 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".

Comment on lines +31 to +35
env \
-u GITHUB_PAT \
-u GITHUB_APP_ID \
-u GITHUB_APP_INSTALLATION_ID \
-u GITHUB_APP_PRIVATE_KEY \

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Keep root-mode runner secrets out of parent processes

In root execution mode, this only removes the GitHub auth variables from the child ./run.sh environment; the entrypoint shell that started the pipeline still keeps its original environment while the runner is active. For supported root-mode runners such as RUNNER_EXEC_MODE_OVERRIDE=root on the Linux Docker plane or Synology root fallback, workflow processes also run as root and can read another root process's /proc/<pid>/environ, so they can still recover GITHUB_PAT or the App credentials despite this sanitization.

Useful? React with 👍 / 👎.

@athena-omt athena-omt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Athena review: approves. The change is focused, tests cover the new guard, and I don't see a blocker in the diff.

@pheidon pheidon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pheidon review: approves. The change is focused, tests cover the new guard, and I don't see a blocker in the diff.

@pheidon

pheidon commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Closing as duplicate overlap with #142. PR #142 carries the same runner job auth scrub from this PR plus the later cleanup-safe process-environment scrub, so #142 should remain the canonical branch for this fix.

@pheidon pheidon closed this Jun 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants