Prepend system bin dirs to PATH when allowed_non_write_users is set#1208
Prepend system bin dirs to PATH when allowed_non_write_users is set#1208OctavianGuzu merged 1 commit intomainfrom
Conversation
1ac9d90 to
347a6be
Compare
347a6be to
869ee18
Compare
There was a problem hiding this comment.
LGTM — straightforward PATH fix following existing patterns.
Extended reasoning...
Overview
Adds two steps to action.yml that write /usr/bin and /bin to $GITHUB_PATH: once before the main Claude step and once after (with always()). Both are gated on allowed_non_write_users \!= '', matching the adjacent pin-bun step pattern.
Security risks
None meaningful. The change only adds standard, trusted system directories to PATH. It does not introduce any new code execution paths, secret exposure, or auth bypass risks. Gating on allowed_non_write_users is appropriate and consistent with adjacent steps.
Level of scrutiny
Low — this is a config-only change to a CI action definition. The mechanism (writing to $GITHUB_PATH) is standard GitHub Actions practice; entries written there are prepended to PATH for subsequent steps, so the naming is accurate despite using >>.
Other factors
No bugs were found, no prior review comments exist, and the change is small (8 lines), self-contained, and follows established patterns in the file. continue-on-error: true makes both steps safe to fail without breaking the workflow.
869ee18 to
e558b02
Compare
There was a problem hiding this comment.
LGTM — straightforward PATH fix and improved security docs.
Extended reasoning...
Overview
Two files changed: adds two small steps that prepend and to GITHUB_PATH (one before the main Claude step, one after in the always-run cleanup phase), and strengthens the PAT warning for allowed_non_write_users.
Security risks
None introduced. The path entries are hardcoded system directories, not user-controlled. The doc change is purely additive security guidance, warning against static PATs and recommending minimal allowedTools.
Level of scrutiny
Low. This is a mechanical fix for a known PATH ordering issue (setup actions like setup-bun can prepend their own dirs, pushing system tools lower). The new steps follow the exact same pattern as the adjacent "Pin bun binary" step (same if condition, same continue-on-error: true).
Other factors
No logic changes, no new inputs, no security-sensitive code touched. The post-step re-prepend correctly uses always() to cover cleanup steps. Bug hunting found no issues.
f7fcc61 to
360857f
Compare
360857f to
da6f220
Compare
da6f220 to
b3500f6
Compare
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
action.yml:350-376— The Re-prepend step's env: block (lines 357-362) clears BASH_ENV, LD_PRELOAD, and related variables but omits GITHUB_ENV, leaving the write target for the env-clearing at line 372 vulnerable to prompt injection: if the Claude run step is coerced into writing GITHUB_ENV=/dev/null to the real env file, the Re-prepend step's} >> "$GITHUB_ENV"silently discards all clearings to /dev/null, and malicious values like BASH_ENV=/tmp/evil.sh survive into the Cleanup SSH signing key and Post buffered inline comments steps (which invoke bash without --noprofile --norc). Fix by capturing the authoritative GITHUB_ENV path before the Claude step (via a step output) and using that pinned path in the Re-prepend run block.Extended reasoning...
What the bug is and how it manifests
The Re-prepend step was added to sanitize the environment after Claude processes untrusted input (when allowed_non_write_users is set). It clears dangerous variables by writing zeroed values to
$GITHUB_ENVat line 372. However, the step's own env: block (lines 357-362) enumerates BASH_ENV, LD_PRELOAD, NODE_OPTIONS, DYLD_INSERT_LIBRARIES, and DYLD_PRELOAD — but conspicuously omits GITHUB_ENV itself. This means if the GITHUB_ENV environment variable has been overridden by an attacker-injected value (e.g., GITHUB_ENV=/dev/null) from the preceding Claude step, the Re-prepend step inherits that overridden value and all its clearing writes are silently discarded.The specific code path that triggers it
During the Run Claude Code Action step, Claude has access to a bash tool that can write arbitrary content to $GITHUB_ENV. A prompt injection — e.g., a hidden instruction in a user-submitted issue — could cause Claude to execute:
echo 'GITHUB_ENV=/dev/null' >> "$GITHUB_ENV"andecho 'BASH_ENV=/tmp/evil.sh' >> "$GITHUB_ENV". The GitHub Actions runner reads the env file after the Claude step and propagates both values. When the Re-prepend step runs, it has GITHUB_ENV=/dev/null in its environment (set from the accumulated env). The env: block does NOT reset GITHUB_ENV, so the run block's} >> "$GITHUB_ENV"expands to} >> /dev/null— the BASH_ENV= clearing is written nowhere. The runner then reads the canonical env file (its internal reference), finds no clearings, and BASH_ENV=/tmp/evil.sh persists into subsequent steps.Why existing code does not prevent it
The env: block correctly overrides BASH_ENV and the other variables for the step's own bash invocation, which protects the Re-prepend step itself. But the purpose of the GITHUB_ENV writes is to protect SUBSEQUENT steps. Since GITHUB_ENV is not in the env: block, its value during the run: block is whatever the accumulated environment says — and that can be /dev/null if Claude was injected. The continue-on-error: true flag silences any error from writing to /dev/null (on Linux, writes to /dev/null succeed without error anyway), so there is no signal that the clearing failed.
The refutation's counter-argument addressed
One verifier argued that the GitHub Actions runner always creates a fresh temp file for GITHUB_ENV at the start of each step, overriding any accumulated GITHUB_ENV value. If this is correct, the runner would inject the correct GITHUB_ENV path into the Re-prepend step despite any injected override, and the clearing would succeed. This is a legitimate uncertainty: GitHub's documentation describes GITHUB_ENV as runner-managed, and the runner may indeed inject its authoritative path per step. However, this behavior is implementation-dependent and not explicitly guaranteed against injection overrides in the runner specification. At minimum, the design gap (GITHUB_ENV absent from the env: block) makes the step's protection fragile and dependent on an undocumented runner guarantee. The safe fix does not rely on this behavior.
Impact
If the attack works (runner does not override accumulated GITHUB_ENV), BASH_ENV=/tmp/evil.sh persists into the Cleanup SSH signing key step (line 374-383) and Post buffered inline comments step (lines 385+), both of which use shell: bash without --noprofile --norc. When bash initializes, it sources the file at BASH_ENV, giving the attacker arbitrary code execution in the post-Claude cleanup steps. These steps also have access to secrets (GITHUB_TOKEN, ANTHROPIC_API_KEY) passed in their env: blocks, making this high-value for exfiltration.
How to fix it
Before the Run Claude Code Action step, capture the authoritative GITHUB_ENV path: add a step with
id: pin-envandrun: echo "github_env=$GITHUB_ENV" >> "$GITHUB_OUTPUT". In the Re-prepend step's run block, replace$GITHUB_ENVwith${{ steps.pin-env.outputs.github_env }}. Step outputs are set by the runner at step completion and cannot be overridden by env injection, so this pins the write target to the known-good path regardless of what accumulated env vars say.Step-by-step proof
- Attacker submits an issue containing hidden instruction: execute
echo 'GITHUB_ENV=/dev/null' >> "$GITHUB_ENV" && echo 'BASH_ENV=/tmp/evil.sh' >> "$GITHUB_ENV"via bash tool. - Claude (with allowed_non_write_users set) processes it and runs the bash command, writing both entries to the legitimate GITHUB_ENV temp file.
- Runner reads the temp file after the Claude step; accumulates GITHUB_ENV=/dev/null and BASH_ENV=/tmp/evil.sh.
- Runner sets up the Re-prepend step, propagating accumulated env. If runner does not override GITHUB_ENV: step environment has GITHUB_ENV=/dev/null.
- Re-prepend step runs: env: block overrides BASH_ENV="" protecting the step's own bash (good). Run block executes
} >> "$GITHUB_ENV"=} >> /dev/null— clearings are discarded. - Runner reads its canonical env file after Re-prepend; finds no new clearings. BASH_ENV=/tmp/evil.sh from step 3 remains.
- Cleanup SSH signing key step: shell: bash initializes, sources /tmp/evil.sh via BASH_ENV. Attacker code executes with access to the step's environment including any secrets.
- Attacker submits an issue containing hidden instruction: execute
Ensures later steps resolve standard tools like git and tar from /usr/bin regardless of what setup actions added earlier in the job. Also strengthens the PAT guidance in security.md. :house: Remote-Dev: homespace
b3500f6 to
a15bf9d
Compare
wolffiex
left a comment
There was a problem hiding this comment.
LGTM with one small fix before merge, plus a scope note.
Fix: NODE_OPTIONS asymmetry in the re-prepend step
The step's env: block clears 8 variables for the step itself — including NODE_OPTIONS — but the $GITHUB_ENV persistence block clears only 7. NODE_OPTIONS is missing:
env:
BASH_ENV: ""
LD_PRELOAD: ""
LD_LIBRARY_PATH: ""
NODE_OPTIONS: "" # cleared for THIS step
DYLD_INSERT_LIBRARIES: ""
DYLD_PRELOAD: ""
DYLD_LIBRARY_PATH: ""
DYLD_FRAMEWORK_PATH: ""
run: |
echo "/usr/bin" >> "$GITHUB_PATH"
echo "/bin" >> "$GITHUB_PATH"
{
echo "BASH_ENV="
echo "LD_PRELOAD="
echo "LD_LIBRARY_PATH="
# NODE_OPTIONS missing — add it
echo "DYLD_INSERT_LIBRARIES="
echo "DYLD_PRELOAD="
echo "DYLD_LIBRARY_PATH="
echo "DYLD_FRAMEWORK_PATH="
} >> "$GITHUB_ENV"If Claude emits echo "NODE_OPTIONS=--require=/tmp/evil.js" >> "$GITHUB_ENV" during the action run, that setting survives into later steps. The action's own cleanup steps use bun (which ignores NODE_OPTIONS), so this doesn't compromise the action itself — but any downstream node/npm/npx step in the same job picks it up. Either add echo "NODE_OPTIONS=" to the $GITHUB_ENV block, or drop NODE_OPTIONS: "" from the step-level env: so the two lists match.
Suggestion: scope of PATH hardening
Only /usr/bin and /bin are prepended. Binaries commonly installed elsewhere remain exposed to earlier PATH entries: gh (/usr/local/bin on most runners), node, npm, docker, aws, kubectl, language toolchains. The PR framing ("git, tar, zstd") is accurate for what's defended, but a brief comment in action.yml at the pre-step would help a future reader not mistake this for comprehensive PATH lockdown — e.g. "defends standard POSIX tooling; tools in /usr/local/bin and language toolchains remain first-match from earlier PATH entries."
Notes (non-blocking)
continue-on-error: trueon the re-prepend matches the adjacent pin-bun pattern, but the failure mode is different: pin-bun failing → loud downstream. This failing → silent loss of a security boundary. Worth a comment, or droppingcontinue-on-erroron just this step.$GITHUB_PATHappend order:/usr/binwritten first,/binsecond — GitHub prepends each line, so/binends up ahead of/usr/binin final PATH. Harmless (both trusted), but the PR description reads as if/usr/binwins.- Custom shell
/bin/bash --noprofile --norc -e -o pipefail {0}correctly bypasses/etc/profileand~/.bashrc. Good hardening. docs/security.mdchange is well-calibrated — the PAT warning is genuinely sharper.
Approving on the assumption the NODE_OPTIONS gap gets closed in a follow-up commit before merge.
|
Thanks for the review! On the NODE_OPTIONS asymmetry: it's intentional — the runner has rejected Ack on PATH scope (only |
Prepends
/usr/binand/bintoGITHUB_PATH(before and after the Claude step) so later steps in the job resolve standard tools (git, tar, zstd) from the system locations regardless of what setup actions added to PATH earlier. Gated onallowed_non_write_users, same as the adjacent pin-bun step.