Skip to content

[bugfix] code_review.yml: replace PR's .claude/ instead of nesting (GHSA-f9x3-9rgg-92p7)#515

Merged
tiankongdeguiji merged 2 commits into
alibaba:masterfrom
tiankongdeguiji:fix/codereview-claude-override
May 20, 2026
Merged

[bugfix] code_review.yml: replace PR's .claude/ instead of nesting (GHSA-f9x3-9rgg-92p7)#515
tiankongdeguiji merged 2 commits into
alibaba:masterfrom
tiankongdeguiji:fix/codereview-claude-override

Conversation

@tiankongdeguiji
Copy link
Copy Markdown
Collaborator

Summary

  • .github/workflows/code_review.yml: the override that was supposed to neutralize a PR-supplied .claude/ directory was a no-op — cp -r SRC DST nests into DST when DST already exists, so the PR's pr-code/.claude/settings.json was never replaced and a SessionStart hook in the PR could still fire on the self-hosted runner. Added rm -rf pr-code/.claude before the copy.
  • .github/workflows/deprecated/code_review.yml: dropped — fully superseded by the active workflow.

The trigger (pull_request_target + types: [labeled] gated on the claude-review label) still requires a maintainer to label the PR, so this is not exploitable by a drive-by forker; risk is a maintainer labeling a PR whose .claude/ slipped past review.

Reference: GHSA-f9x3-9rgg-92p7.

Verification

Reproduced the nesting bug and the fix in /tmp:

# before fix:
$ cp -r trusted-claude/.claude pr-code/.claude
pr-code/.claude/settings.json        → "MALICIOUS"   ← still there
pr-code/.claude/.claude/settings.json → "TRUSTED"    ← nested, ignored

# after fix:
$ rm -rf pr-code/.claude
$ cp -r trusted-claude/.claude pr-code/.claude
pr-code/.claude/settings.json        → "TRUSTED"

Test plan

  • Add the claude-review label to a PR and confirm the review job still runs (no functional change beyond replacing the .claude/ contents).

…HSA-f9x3-9rgg-92p7)

The Code Review workflow tried to neutralize a PR-supplied `.claude/`
directory by overlaying the base-branch version on top:

    cp -r trusted-claude/.claude pr-code/.claude

But `cp -r SRC DST` nests into DST when DST already exists, producing
`pr-code/.claude/.claude/` and leaving the PR's
`pr-code/.claude/settings.json` in place. Claude Code reads the
working-directory `settings.json`, so a `SessionStart` hook in the PR
would still fire on the self-hosted runner — defeating the override.

The workflow trigger (`pull_request_target` with `types: [labeled]`
gated on the `claude-review` label) still requires a maintainer to label
the PR before `claude -p` runs, so this is not exploitable by a drive-by
forker. The risk is a maintainer labeling a PR whose `.claude/` slipped
past review.

Fix: `rm -rf pr-code/.claude` before the copy so the trusted version
fully replaces the PR's directory.
The active code_review.yml has fully replaced the
anthropics/claude-code-action v1-based variant. No reason to keep the
workflow_dispatch stub around — `git log` is the archive.
@tiankongdeguiji tiankongdeguiji merged commit a19c8c0 into alibaba:master May 20, 2026
7 checks passed
@juniperbevensee
Copy link
Copy Markdown

Just flagging that i was the one who privately submitted the disclosure that led to this PR. :)

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