fix(claude-e2e-verify): decline the credential-coexistence acceptance; run PR code as an unprivileged user - #328
Conversation
…; run PR code as an unprivileged user Rules #288 as Option B (documented at #288, issuecomment-5138342003): the persisted-credential acceptance block is declined, not re-ratified, and the coexistence is closed structurally. All PR-controlled code (setup-command + app-start-command) now runs as a dedicated unprivileged user from a .git-free copy of the checkout with a sudo env_reset environment; the runner-owned checkout is chmod'd o-rwx. This removes the app's access to the persisted .git/config token, later steps' /proc environs, the OIDC request env vars, and the runner-ingested GITHUB_ENV/GITHUB_PATH files (#298's channel). A separate unprivileged JOB — the direction #288 recorded — cannot work here: jobs run on separate VMs and the agent must reach the live app over localhost, so the uid boundary lands in-job. The header's threat inventory gains the four-credential list, the corrected visibility:all premise, the corrected fork wording (the agent step executes and fails under continue-on-error; it is not skipped), a demoted strip-step rationale (artipacked compliance + self-hosted cross-job scrub only), and the indirect-prompt-injection entry (#299) with the recorded decision that the default claude-args deliberately carries no --allowedTools. Fails closed on runners without passwordless sudo/useradd rather than degrading to same-uid execution. Fixes #288 Fixes #298 Fixes #299 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01RhS3T7ShwJgKTrvk2Mvd3C Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 2m 36s —— View job Reviewing PR #328
This is the code-review lane; per Findings🔴 Important —
On GitHub-hosted runners this is invisible (the VM is destroyed after the job), but the Suggest adding a Everything else
|
|
Claude finished @kyle-sexton's task in 6m 15s —— View job Security review
Scope: only the two changed files, diffed against The core design is sound for the stated primary target (GitHub-hosted 1. IMPORTANT (confidence: PLAUSIBLE) —
The fail-closed check at Since this specific guarantee is load-bearing for one of the four listed credentials and the existing check doesn't cover it, consider hardening the isolate step to assert the property directly, e.g. 2. SUGGESTION (confidence: PLAUSIBLE) — workspace protection strips only "other" bits, not group bits
3. SUGGESTION (confidence: PLAUSIBLE) — the Teardown ( No findings within the checked-in |
|
Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e286de2620
ℹ️ 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".
…ead them
## Fix
The lane failed its own dogfood run: the app never came up, and the
application log carried the reason —
bash: /home/runner/work/_temp/app-start-command.sh: Permission denied
Both command scripts were composed into `$RUNNER_TEMP` and then executed
as `app-under-test`, but that uid cannot read runner-owned paths. The
runner-opened log fd already encodes half of this — it exists so the app
never needs WRITE access to a runner-owned path — the miss was that reads
are barred just the same.
Both scripts are now staged into a root-owned `/opt/claude-e2e-verify`
via `install -m 0555 -o root -g root`, readable and executable by the app
uid but not writable by it. Deliberately not the app user's own home: that
directory is app-owned, so a script placed there could be swapped by the
very code it is meant to launch. The staging directory is recreated fresh
each run.
This also repairs `setup-command`, which had the identical defect and
escaped the failure only because the dogfood caller passes none — it was
a latent break for any real consumer.
## Account provisioning is now repeatable (review finding, P1)
`useradd` ran unconditionally, so on a persistent self-hosted runner —
which `inputs.runner` permits — the first run left the account behind and
every later run died at that line under `set -e`.
Creation is now conditional, but reuse is *gated*, not assumed: the
account is stamped with a marker in its GECOS field at creation and the
marker is re-checked before reuse. An `app-under-test` this lane did not
provision belongs to someone else, and adopting it would run PR-controlled
code under whatever that account already carries — group memberships,
sudo rules, existing files — so that case fails closed, matching the
step's existing refusal to degrade to same-uid execution. A reused account
is reset first: processes under the uid killed, the app tree removed and
recopied.
## Ground rules no longer contradict the workflow (review finding, P2)
`CLAUDE.md`'s security section still called the credential coexistence
"accepted, not mitigated in-job", which is the posture this PR replaces —
guidance opposite to the implemented model invites a later change that
restores the weaker one. Only that clause moved; the bound (the
`pull_request`-only tripwire, the fork-PR no-secrets guarantee) and the
"do not widen it" prohibition are untouched, and the mitigation's details
stay owned by the workflow header rather than being restated.
## Verification
- `actionlint` exit 0 on the changed workflow, with `shellcheck` present
so the embedded `run:` scripts were checked too.
- `markdownlint-cli2` clean on `CLAUDE.md`.
- YAML parses; no whitespace damage.
- Marker gating simulated against synthetic passwd lines: this lane's own
stamp reuses; a foreign `adduser` GECOS (`Someone Else,,,`) and an empty
GECOS both fail closed.
The permission fix itself is proven by the dogfood lane, which path-filters
on this workflow and exercises it end-to-end against the fixture app.
## Related
Addresses both review findings on #328.
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 4m 25s —— View job Security review
Scope: this PR's two changed workflow files, focused on what changed since my last pass on this branch (commit Previously reported and now resolved: the 1. IMPORTANT (confidence: PLAUSIBLE) — the reused The new reuse path at L379-L390 only removes Everything else under This is compounded by the Both issues share a root cause: the isolation model treats the account/home/script-dir as ambient shared state rather than scoping it per job. Since the isolation credibility here rests specifically on the self-hosted-reuse case (the
2. IMPORTANT (confidence: PLAUSIBLE) — still open from my previous pass: Unchanged by the new commit. L353 and the header both credit 3. SUGGESTION (confidence: PLAUSIBLE) — still open from my previous pass: Unchanged. L400 is sound on the documented GitHub-hosted target (per-user private groups by default), but implicitly assumes that No new findings within the trigger boundary, credential/permission scoping, or template-injection discipline — those are unchanged by this commit and still read correctly. The new script-staging mechanism itself ( |
1 participant
Summary
Implements the #288 ruling (Option B — decline re-ratification, posted under this session's delegated decision authority; human veto stands). The persisted-credential acceptance block at
claude-e2e-verify.yml:65-92is rewritten from accepted to declined-and-mitigated, and the coexistence is closed structurally rather than re-worded:setup-commandandapp-start-command— now runs as a dedicated unprivileged user (app-under-test) from a.git-free copy of the checkout, undersudoenv-reset. The runner-owned checkout ischmod o-rwxbefore any PR-controlled code runs. Closed channels: the persisted.git/configtoken read, same-uid/procenviron reads of later steps (including the agent'sCLAUDE_CODE_OAUTH_TOKENand minted App token), theACTIONS_ID_TOKEN_REQUEST_*inheritance at spawn, and$GITHUB_ENV/$GITHUB_PATHappends (claude-e2e-verify: $GITHUB_ENV / $GITHUB_PATH hijack by the concurrently-running app (follow-up to #288) #298's execution-path hijack — a different uid cannot write runner-owned files, which also moots the step-boundary re-read question claude-e2e-verify: $GITHUB_ENV / $GITHUB_PATH hijack by the concurrently-running app (follow-up to #288) #298 flagged as unverified).:90-92: jobs run on separate VMs and the agent must reach the live app overlocalhost, so a second job's app is unreachable by design. The uid boundary delivers the same security property in-job; the header documents this.sudo/useradd(therunnerinput allows self-hosted) errors out instead of degrading to same-uid execution. GitHub-hosted Ubuntu images provide both.visibility: allpremise fixed, the false "fork PRs skip the agent entirely" claim replaced with the verified behavior (the agent step executes and fails undercontinue-on-error; the jobif:has no fork guard), and theStrip persisted git credentialsstep demoted out of the coexistence rationale to its own merits (zizmorartipackedcompliance; cross-job scrub on self-hosted runners). The#1236strip-ordering constraint is preserved unchanged.e2e-spec'sgh pr diff, and browsing an app rendering PR-authored content), with the explicit recorded decision claude-e2e-verify: indirect prompt injection is absent from the lane's threat inventory (follow-up to #288) #299 demands: the defaultclaude-argsdeliberately carries no--allowedTools— the lane requires the agent to author/run Playwright scripts and post its ownghcomment, soBashcannot be excluded, and an allowlist includingBashbounds nothing while implying it does. The honest boundary (advisory posture, minimal token permissions, orgcan_approve_pull_request_reviews: false, turn budget, the privilege separation above) is documented instead; consumers can pass their ownclaude-args.setup-command/app-start-commandno longer inherit the runner environment or run in the writable checkout; input descriptions updated. The only live consumer is this repo's own dogfood caller (org-wide code search verified: zero external callers), whose static-fixture command is unaffected.The teardown mechanism changes from setsid/PID-group signalling to
pkill -uof the app uid (strictly broader coverage); the self-caller's inaccurate "degrades to a warning" fork comment is corrected in passing.Verification
actionlintclean on both changed workflows (baseline main: clean).zizmorvisible findings identical to main's baseline (1 lowartipackedat the checkout step — the documented #1236 constraint; 1 mediumadhoc-packagesfor the pinned global Playwright install — both pre-existing).yqYAML parse clean.claude-e2e-verify-self.yml) path-filters on both changed files, so this PR's own CI exercises the isolated lane end-to-end against the fixture app.Related
Ruling and full rationale, including the settled
id-token/fork fact with GitHub-doc citations and the block-scoped embargo correction: #288 (comment)Fixes #288
Fixes #298
Fixes #299
🤖 Generated with Claude Code
https://claude.ai/code/session_01RhS3T7ShwJgKTrvk2Mvd3C