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

Closes #35714

🤖 Generated with Claude Code

[claude]

Claude finished @mbiuki's task in 1m 49s —— View job

---

Security Review — claude-security-review.yml

New Issues

• 🟠 High: .github/workflows/claude-security-review.yml:134-156 — The Run Claude security review step runs claude --dangerously-skip-permissions --allowedTools "Bash,..." over the attacker-controllable PR-head checkout, while the immediately-preceding aws-actions/configure-aws-credentials@v4 step (line 134) exports live AWS OIDC credentials (AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY/AWS_SESSION_TOKEN) into the runner environment. Those creds are readable by any Bash command Claude runs, so a prompt-injection payload committed into a reviewed PR could instruct Claude to use them. The checkout hardening (persist-credentials: false, no GITHUB_TOKEN in this step's env) protects the git token but does nothing for the AWS session. Assumption: the BEDROCK_ROLE_ARN role is scoped to bedrock:InvokeModel only. What to verify: the IAM policy on that role — if it grants anything beyond model invocation, the blast radius of a prompt injection is that entire policy. Consider scoping the role minimally and/or documenting this trust boundary in the header.

• 🟡 Medium: .github/workflows/claude-security-review.yml:65,104,115,135,188,216 — All third-party actions use mutable tags (actions/github-script@v7, actions/checkout@v4, aws-actions/configure-aws-credentials@v4). Already flagged by Semgrep (github-actions-mutable-action-tag, 6 findings) and still unpinned on the reviewed SHA. For a workflow that holds pull-requests: write, issues: write, checks: write, id-token: write and assumes an AWS role, a repointed tag is a direct supply-chain path. Pin each to a full 40-char commit SHA. Fix this →

• 🟡 Medium: .github/workflows/claude-security-review.yml:207,227 — Both the abstract-findings and the clean comment call github.rest.issues.createComment unconditionally, so a new comment is appended on every synchronize (every push). A PR that goes through several review cycles accumulates a stack of duplicate "Security Review" comments. Upsert instead: find the bot's prior ## Security Review comment via listComments and updateComment it, falling back to create. (Author acknowledged this as a follow-up, but it's in the diff being merged.)

• 🟡 Medium: .github/workflows/claude-security-review.yml:268-272 — Each Slack report chunk is wrapped in triple-backtick fences ("```\n" + c + "\n```"). If report_markdown itself contains a ``` fence (very likely — it's a markdown security report that will quote code), the fence closes early and the remainder renders as raw/garbled mrkdwn in Slack. Additionally the chunks[:8] cap (line 271) silently drops any content past ~22 KB with no indication in the message that it was truncated — for a security report that's a real information-loss risk. Consider `files.upload.v2` for the full report, and at minimum append a "truncated — N blocks omitted" marker when the cap is hit.

Notes (non-blocking)

• The Parse findings step (line 158) correctly fails closed on a missing/invalid security-findings.json — good, and the right call for a security gate.
• envsubst at line 129 will expand any $VAR in the prompt template, not just the three intended ones. The current template only references $BASE_SHA/$HEAD_SHA/$PR_NUMBER, so this is fine today, but a future $ in the prompt (e.g. a shell example) would be silently blanked. Consider envsubst '$BASE_SHA $HEAD_SHA $PR_NUMBER' to whitelist.

No critical issues. The fail-closed logic, persist-credentials: false, and the env:-not-interpolation handling of untrusted context values are all done correctly.

• 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.

[wezell]

Would it be better to run this repo wide once or twice a week? our front end tooling is vulnerable to xss and clickjacking and other issues. The fact that we are so limiting the code paths where this runs makes it less valuable imo.

[mbiuki]

@sfreudenthaler — thanks for the nudge to fix the bot review first. Done, and I also reworked the approach you and @wezell flagged.

Bot review (#issuecomment-4454797469) — the file had botched, half-applied Semgrep suggestions. All addressed:

• Removed duplicate/truncated Resolve PR details + Check for prior Semgrep review steps — the duplicate step ids meant the workflow wouldn't even parse.
• Closed the github-script injection — no context/step values are interpolated into any script: body; everything flows through env: + process.env.
• Fail closed: a missing findings file now fails the check instead of reporting a green "no findings."
• Dropped the hardcoded branch ref for a github.workflow_sha sparse-checkout; consolidated the split draft gate; added persist-credentials: false on the PR-head checkout.

Approach — you and @wezell were right that gating Claude on Semgrep was backwards: it skipped the logic-aware reviewer exactly when another tool was engaged. Removed the Semgrep-suppression — Claude now reviews every PR, ungated. Bumped the model to claude-opus-4-8 to match the repo's other AI checks.

Deliberately left as follow-ups (not this PR):

• Fork PRs don't receive secrets, so the review can't run on them — needs a separate strategy.
• @wezell's periodic repo-wide scan idea (frontend XSS/clickjacking surface) — I think that's complementary to this per-PR gate; happy to open an issue.
• Not proposing this into required branch-protection checks yet — I'd run it advisory for ~a week to gauge noise first.

Validated with actionlint + an independent review pass. Another look when you have a chance would be appreciated.

[github-actions]

Security Review

Automated security review flagged 1 issue(s) that require attention before this PR can merge.

For security reasons, details are not posted here. The PR author has been notified privately on Slack with the full report and remediation guidance.

If you did not receive a Slack notification, contact the security team.

This check will remain red until the findings are resolved and a new commit is pushed.

[github-actions]

Security Review

No high-confidence security findings on the changes in this PR.

[github-actions]

Security Review

No high-confidence security findings on the changes in this PR.

[sfreudenthaler]

don’t use anthropic API directly. Should go via bedrock AND ideally through dotcms/ai-workflows repo as the reusable action

[sfreudenthaler]

This shouldn't be using the antrhopic api. should be bedrock so that we include it into our controls and commit leverage.

Also consider using the dotcms/ai-workflows repo which allows for prompt injection to be passed across and gives you short lived OIDC and additional security controls AND the ability to pick from different models by variable.

[mbiuki]

Done in 99fd5d3a17 — the review now runs on Bedrock, not the Anthropic API. Added id-token: write + aws-actions/configure-aws-credentials@v4 to OIDC-assume BEDROCK_ROLE_ARN (no long-lived keys), set CLAUDE_CODE_USE_BEDROCK=1 / AWS_REGION / --model $BEDROCK_MODEL_ID, and dropped the ANTHROPIC_API_KEY secret. It reuses the exact vars (BEDROCK_ROLE_ARN / BEDROCK_MODEL_ID / BEDROCK_AWS_REGION) that issue_autodoc.yml and the ai_claude-* reviewer workflows already use, so it's in the same controls/billing path.

On the dotcms/ai-workflows suggestion: agreed that's the ideal end state. I kept it as a follow-up rather than folding it into this PR because this workflow has bespoke behavior the generic claude-orchestrator.yml doesn't express today — the Semgrep-dedup gate, parsing security-findings.json, the abstract-public-comment + private-Slack-DM split, and the merge-blocking fail-closed. Porting it means mapping all of that onto the reusable workflow's inputs/outputs, which is worth doing deliberately as its own change. Happy to open that as a tracked follow-up if you'd like.

[semgrep-dotcms]

Semgrep found 6 github-actions-mutable-action-tag findings:

• .github/workflows/claude-security-review.yml
  • L65 - Triage
  • L104 - Triage
  • L115 - Triage
  • L135 - Triage
  • L188 - Triage
  • L216 - Triage

GitHub Actions step uses a mutable tag or branch reference. Tags and branch names can be silently repointed by the action owner, enabling supply-chain attacks — as seen in the trivy-action and kics-github-action compromises. Pin the reference to a full 40-character commit SHA instead, e.g. uses: actions/checkout@8ade135a41bc03ea155e62e844d188df1ea18608.

If this is a critical or high severity finding, please also link this issue in the #security channel in Slack.

[mbiuki]

Pushed 99fd5d3a17 addressing the review:

@sfreudenthaler — use Bedrock: ✅ The review now runs through AWS Bedrock (OIDC-assumed BEDROCK_ROLE_ARN, CLAUDE_CODE_USE_BEDROCK=1, --model $BEDROCK_MODEL_ID), reusing the same vars as issue_autodoc/ai_claude-*. ANTHROPIC_API_KEY removed. Full port to dotcms/ai-workflows proposed as a follow-up (reply on the thread explains why it's separate).

Semgrep github-script-injection: ✅ Already resolved on the current branch — the flagged Check for prior Semgrep review step was removed when the workflow moved to always-review, and every remaining actions/github-script step passes step-output values via env: and reads them with process.env (never interpolated into the script: body). No ${{ ... }}-in-JS remains.

🤖 Generated with Claude Code

[mbiuki]

Security assessment

CVSS 4.0	N/A: CI tooling PR adding an automated security-review workflow; no product vulnerability.
Priority	P3 · due 2026-08-12 (one quarter, 90 days from 2026-05-14, past due) · project #7 (dotCMS - Product Planning): P3 - Medium

Why this priority: CI tooling PR adding an automated security-review workflow; no product vulnerability.

Labels: already correct, no change.

Automated triage by the dotCMS Security team (Claude), 2026-10-06. To override, change the project's Priority field or the CVSS label. The next refresh keeps a manual change.