ci: add Claude security-review workflow for sensitive-path PRs

[mbiuki]

Tracks #35714.

Summary

Adds .github/workflows/claude-security-review.yml which runs an AI-assisted security review on every PR touching a security-sensitive path. Designed to address the gap exposed by recent incidents (SI-75) where unauthenticated SQL injection landed via human review.

Behavior

• Trigger: pull_request (opened / synchronize / reopened / ready_for_review), filtered to security-sensitive paths only — REST resources, auth/login, servlets/filters, business impls (DB layer), push-publish, OSGi, web app, SQL, build files, Dockerfiles, workflows.
• On findings (confidence ≥ 8):
  1. PR gets an intentionally abstract comment — no vuln details (this repo is public). Just "N issues found, see Slack."
  2. PR author is DM'd on a private Slack channel with the full markdown report and remediation guidance.
  3. The check fails, so the PR is blocked from merge once this job is added to branch-protection's required checks.
• On clean: PR gets a short "no findings" comment, check passes.
• Concurrency: in-progress runs are cancelled on force-push.
• Timeout: 25 min.

Required configuration before this is useful

The workflow no-ops until these are configured (see header in the YAML for details):

Secrets (Settings → Secrets and variables → Actions → Secrets):

• `ANTHROPIC_API_KEY` — Anthropic API key
• `SLACK_BOT_TOKEN` — Slack bot token with `chat:write` scope; bot must be invited to the channel
• `SLACK_USER_MAP` — JSON mapping GitHub login → Slack user ID, e.g. `{"mbiuki":"U0123ABCDEF", ...}`

Variables (Settings → Secrets and variables → Actions → Variables):

• `SLACK_SECURITY_CHANNEL` — Slack channel ID (e.g. `C0XXXXXXX`) for the private security-comms channel

Branch protection (Settings → Branches → main):

• Add `Claude Security Review / security-review` to required status checks once we've validated noise levels.

Known limitations

• External-fork PRs: workflow currently uses `pull_request` (safe — no secret access), but that means it doesn't run on fork PRs. Follow-up to decide on a strategy for those (e.g. `pull_request_target` with read-only checkout, or run on the maintainer's re-push).
• Slack message size: report is chunked into ≤8 blocks of 2800 chars each. Very long reports may be truncated; a follow-up could upload the full report as a file via `files.upload.v2` instead.
• The Anthropic action used is `anthropics/claude-code-action@v1`. Pinning to a specific commit SHA is recommended once a stable revision is chosen.

Test plan

• Set the secrets and variables in a staging fork or this repo
• Open a test PR with a deliberate finding and confirm: abstract PR comment, Slack DM with details, failing check
• Open a test PR with no issues on a sensitive path and confirm: clean comment, passing check
• Open a PR on a non-sensitive path and confirm the workflow does not run
• Force-push a fix and confirm the previous run is cancelled and the new run goes green
• Once validated, add to branch protection's required checks

🤖 Generated with Claude Code

[github-actions]

❌ Issue Linking Required

This PR could not be linked to an issue. All PRs must be linked to an issue for tracking purposes.

How to fix this:

Option 1: Add keyword to PR body (Recommended - auto-removes this comment)
Edit this PR description and add one of these lines:

• This PR fixes #123 or Fixes: #123

• This PR closes #123 or Closes: #123

• This PR resolves #123 or Resolves: #123

• Other supported keywords: fix, fixed, close, closed, resolve, resolved
  Option 2: Link via GitHub UI (Note: won't clear the failed check)

1. Go to the PR → Development section (right sidebar)

2. Click "Link issue" and select an existing issue

3. Push a new commit or re-run the workflow to clear the failed check
   Option 3: Use branch naming
   Create a new branch with one of these patterns:

• 123-feature-description (number at start)

• issue-123-feature-description (issue-number at start)

• feature-issue-123 (issue-number anywhere)

Why is this required?

Issue linking ensures proper tracking, documentation, and helps maintain project history. It connects your code changes to the problem they solve.---

This comment was automatically generated by the issue linking workflow

[claude]

Claude finished @mbiuki's task in 2m 7s —— View job

---

PR Review

• Read the workflow file
• Analyze trigger / permissions / secret handling
• Analyze Semgrep precheck logic
• Analyze Claude action invocation and findings parsing
• Analyze PR comment + Slack notification logic
• Post review

---

Things I'd flag before this lands

1. AI-review step failures silently fail OPEN, not safe. .github/workflows/claude-security-review.yml:188-193 — if security-findings.json is missing (Claude action crashed, hit a rate limit, model wrote malformed output, network blip), the step prints "failing safe (no findings)" and the workflow posts a clean PR comment + passes the check. That's the opposite of fail-safe for a security gate. If the Claude step fails or the file is missing, this should post "review unavailable" and exit non-zero — otherwise transient failures wave through PRs the gate is supposed to block. Same problem if Claude returns markdown-wrapped JSON and jq blows up: nothing in the parse step guards against partial / non-JSON output. Fix this →

2. Prompt-injection blast radius via Bash + Agent tools. claude_args: --allowed-tools Read,Bash,Grep,Glob,Agent (line 180). The thing Claude is reading is attacker-controlled PR content. A diff containing `` is exactly the prompt-injection setup these tools warn about. ANTHROPIC_API_KEY is in the job env (passed to the action) and the `Bash` tool can almost certainly read it. Real-world risk is bounded because this is `pull_request` (no fork access to secrets) and internal collaborators already have many other paths to exfiltrate, but it's worth (a) dropping `Bash` and `Agent` unless they're actually needed for the review, (b) constraining `Bash` with an allowlist, or (c) noting this explicitly. At minimum drop `Agent` — there's no reason a top-level security-review prompt needs to spawn subagents, and it compounds both blast radius and cost.

3. "DM'd on Slack" is misleading — this is a channel post. The PR description and the comment at line 220 both say the author is "DM'd". chat.postMessage here posts to SLACK_SECURITY_CHANNEL with an <@user> mention (lines 279, 290). Anyone in that channel sees the full vuln report including the abstract finding count, code excerpts, and remediation. That may be intended (security team needs visibility) but the wording in the public PR comment ("notified privately on Slack") and in the PR body sets the wrong expectation. Either change the wording or actually DM via conversations.open → chat.postMessage to a 1:1 IM. Don't promise privacy you aren't delivering.

4. Reviewer feedback not addressed. sfreudenthaler explicitly asked: "the prompt should be moved to a standalone markdown file". The inline prompt (lines 154-178) is still inline. If you disagree, push back; if you agree, do it before requesting re-review.

5. Semgrep precheck is fairly easy to spoof. isSemgrep = s => /semgrep/i.test(s) (line 98) matches a substring case-insensitively on c.user.login, c.name, and c.app.slug. Anyone who can post comments to the repo with a GitHub username containing "semgrep" (e.g. semgrep-helper) can bypass the entire Claude review by leaving a single comment. Also matches any check run named/slugged with "semgrep" anywhere in the string. Tighten this to an exact match against the canonical Semgrep app slug (semgrep-app / semgrep) and ideally check c.user.type === 'Bot' so a random user account can't disable the check.

6. dotCMS/src/main/webapp/** and .github/workflows/** in paths will be noisy + expensive. webapp is huge and a static asset edit will trigger a full Opus review run. Workflow edits will trigger themselves on every infra/CI PR. Combined with no cost cap / max-turns and Agent enabled, this can rack up Anthropic spend fast. The PR description already lists cost cap as TODO — fine to ship without it, but at least put a --max-turns N (or whatever the action supports) on the Claude step now.

7. Disclosure leak via finding count. Line 216 posts "N issue(s)" publicly. Knowing "this PR has 3 security issues" is itself information — a watcher can deduce when a fix lands by waiting for the count to drop to 0, or guess at severity from medium/high split (which is in the JSON but not posted, good). For a public repo, even the number is a hint. Trivially fixable: drop the number and post "issues were found" instead.

8. Slack text fallback contains the count too (line 291). Most Slack clients render the text field as the notification preview / push notification, so the count leaks to anyone with the channel pinned. Minor, but if you're trying to keep details out of casual view, change the fallback to something generic.

9. Comment spam on each push. Already in the known-limitations list, but worth a follow-up issue with a specific implementation note: use github.rest.issues.updateComment against a sentinel comment (find by hidden HTML marker in body) rather than createComment every run. Otherwise busy PRs end up with 10+ duplicate "no findings" comments.

10. No artifact / audit trail. security-findings.json lives in the workspace and dies with the runner. Good for disclosure hygiene; bad for audits ("what did the AI flag on PR #X six months ago?"). Consider uploading it as a private artifact with restricted retention, or storing the report ID in the Slack message.

11. PR title is interpolated into Slack message text unsanitized (line 295). Not a security issue (Slack mrkdwn doesn't execute), but a PR title containing <@channel> will not actually broadcast — Slack only honors broadcasts in plain text and within message text. Still, a title with | or > breaks the hyperlink. Low priority; just be aware.

12. if: conditions cascade off steps.semgrep.outputs.found, not off Claude succeeding. Lines 184, 207, 232, 250, 320 all guard on the Semgrep skip flag but not on the Claude step's outcome. Coupled with #1, this is the same root cause: the workflow doesn't model "review failed to run" as distinct from "review found nothing". A single if: steps.review.conclusion == 'success' && steps.parse.outputs.has_findings == 'true' would tighten this up.

Clean parts: path filter is reasonable (and the SI-75 framing is right), concurrency: cancel-in-progress is correct, pull_request (not pull_request_target) is the right default given the Bash tool exposure, secrets aren't in env for the Claude step beyond ANTHROPIC_API_KEY, env-var pattern for the Python script avoids shell-injection from PR fields, and the abstract-comment design is the right instinct for a public repo.

The biggest things to fix before merging are #1 (fail-open on errors) and #3 (Slack channel vs DM wording), plus addressing sfreudenthaler's standalone-prompt request from his existing change-request review.

---

• Branch: security/add-claude-security-review-workflow

[sfreudenthaler]

if we go this route the prompt should be moved to a standalone markdown file. makes it easier to review and understand the code

[mbiuki]

Status update

Done

• Drafted .github/workflows/claude-security-review.yml (~330 lines, 9 steps)
• Trigger: pull_request filtered to security-sensitive paths (REST, auth, filters, business impls, push-publish, OSGi, web app, SQL, build, Dockerfiles, workflows)
• Concurrency cancel-in-progress + 25-min timeout
• Semgrep precheck: scans for prior Semgrep activity (check run, comment, review, inline review-comment, case-insensitive /semgrep/i match). If found, Claude review is skipped, a brief "skipped" comment is posted, and the check passes.
• If Semgrep absent → runs Claude /security-review via anthropics/claude-code-action@v1, writes findings to security-findings.json
• On findings → abstract PR comment (no vuln details — repo is public), detailed Slack DM to author on private channel, exit 1 to fail the check (which blocks merge once required-status is configured)
• On clean → "no findings" comment, check passes
• Tracking issue opened: Add automated /security-review workflow for security-sensitive PRs #35714
• End-to-end simulation against PR some-tests-for-review-tuning #35707 — produced the PR-comment text, Slack payload (6 blocks, full report chunked into 2x2800-char sections), and the exit 1 signal. Disclosure boundary held: nothing sensitive in the public comment.

To do (before this is live)

Configuration

• Add repo secret ANTHROPIC_API_KEY
• Add repo secret SLACK_BOT_TOKEN (Slack app with chat:write, bot invited to the chosen private channel)
• Add repo secret SLACK_USER_MAP — JSON mapping GitHub login → Slack user ID for every dotCMS engineer who can author PRs (e.g. {"mbiuki":"U0123ABCDEF", ...})
• Add repo variable SLACK_SECURITY_CHANNEL — Slack channel ID (e.g. C0XXXXXXX) for the private security-comms channel
• Pin anthropics/claude-code-action@v1 to a specific commit SHA once a stable revision is chosen

Validation

• Run on a deliberately-broken PR (e.g. some-tests-for-review-tuning #35707) and confirm: abstract comment, Slack DM, failing check
• Run on a clean PR on a sensitive path and confirm: "no findings" comment, passing check
• Run on a PR touching only non-sensitive paths and confirm: workflow does not trigger
• Run on a PR where Semgrep has commented and confirm: skip-comment + passing check
• Force-push a fix and confirm: in-progress run cancelled, new run goes green

Rollout

• Add Claude Security Review / security-review to main's required status checks (Settings → Branches → main → branch protection rule) — this is what actually blocks merges
• Decide policy for PRs from external forks (workflow currently uses pull_request, so secrets aren't exposed but the review also doesn't run on forks)
• Decide cost cap / throttling strategy — high PR volume × Opus reviews can rack up Anthropic spend
• Decide on per-finding private tracking (separate private issues vs Slack thread only)
• Tune path filters after a week of shadow runs

Known limitations to revisit

• Slack message currently chunks into ≤8 blocks of 2800 chars; very long reports may be truncated. Follow-up could upload full report as a file via files.upload.v2.
• Skip-comment fires once per PR push — repeated pushes could cause comment spam. Follow-up could edit the latest comment instead of appending.