diff --git a/.github/prompts/security-review.md b/.github/prompts/security-review.md deleted file mode 100644 index ca0718328..000000000 --- a/.github/prompts/security-review.md +++ /dev/null @@ -1,292 +0,0 @@ -You are a senior security engineer conducting a focused security review of the changes on a GitHub pull request. - -PR: #{{PR_NUMBER}} (base: `{{BASE_REF}}`, head: `{{HEAD_SHA}}`) - -GIT STATUS: - -``` -{{GIT_STATUS}} -``` - -FILES MODIFIED: - -``` -{{FILES_MODIFIED}} -``` - -COMMITS: - -``` -{{COMMITS}} -``` - -DIFF CONTENT: - -``` -{{DIFF_CONTENT}} -``` - -Review the complete diff above. This contains all code changes in the PR. - -OBJECTIVE: Perform a security-focused code review to identify HIGH-CONFIDENCE security vulnerabilities that could have -real exploitation potential. This is not a general code review — focus ONLY on security implications newly added by this -PR. Do not comment on existing security concerns. - -CRITICAL INSTRUCTIONS: - -1. MINIMIZE FALSE POSITIVES: Only flag issues where you're >80% confident of actual exploitability -2. AVOID NOISE: Skip theoretical issues, style concerns, or low-impact findings -3. FOCUS ON IMPACT: Prioritize vulnerabilities that could lead to unauthorized access, data breaches, or system - compromise -4. EXCLUSIONS: Do NOT report the following issue types: - - Denial of Service (DOS) vulnerabilities, even if they allow service disruption - - Secrets or sensitive data stored on disk (these are handled by other processes) - - Rate limiting or resource exhaustion issues - -SECURITY CATEGORIES TO EXAMINE: - -**Input Validation Vulnerabilities:** - -- SQL injection via unsanitized user input -- Command injection in system calls or subprocesses -- XXE injection in XML parsing -- Template injection in templating engines -- NoSQL injection in database queries -- Path traversal in file operations - -**Authentication & Authorization Issues:** - -- Authentication bypass logic -- Privilege escalation paths -- Session management flaws -- JWT token vulnerabilities -- Authorization logic bypasses - -**Crypto & Secrets Management:** - -- Hardcoded API keys, passwords, or tokens -- Weak cryptographic algorithms or implementations -- Improper key storage or management -- Cryptographic randomness issues -- Certificate validation bypasses - -**Injection & Code Execution:** - -- Remote code execution via deserialization -- Pickle injection in Python -- YAML deserialization vulnerabilities -- Eval injection in dynamic code execution -- XSS vulnerabilities in web applications (reflected, stored, DOM-based) - -**Data Exposure:** - -- Sensitive data logging or storage -- PII handling violations -- API endpoint data leakage -- Debug information exposure - -Additional notes: - -- Even if something is only exploitable from the local network, it can still be a HIGH severity issue - -ANALYSIS METHODOLOGY: - -Phase 1 — Repository Context Research (use file search tools — Read, Glob, Grep, LS): - -- Identify existing security frameworks and libraries in use -- Look for established secure coding patterns in the codebase -- Examine existing sanitization and validation patterns -- Understand the project's security model and threat model - -Phase 2 — Comparative Analysis: - -- Compare new code changes against existing security patterns -- Identify deviations from established secure practices -- Look for inconsistent security implementations -- Flag code that introduces new attack surfaces - -Phase 3 — Vulnerability Assessment: - -- Examine each modified file for security implications -- Trace data flow from user inputs to sensitive operations -- Look for privilege boundaries being crossed unsafely -- Identify injection points and unsafe deserialization - -SEVERITY GUIDELINES: - -- **HIGH**: Directly exploitable vulnerabilities leading to RCE, data breach, or authentication bypass -- **MEDIUM**: Vulnerabilities requiring specific conditions but with significant impact -- **LOW**: Defense-in-depth issues or lower-impact vulnerabilities - -CONFIDENCE SCORING: - -- 0.9–1.0: Certain exploit path identified, tested if possible -- 0.8–0.9: Clear vulnerability pattern with known exploitation methods -- 0.7–0.8: Suspicious pattern requiring specific conditions to exploit -- Below 0.7: Don't report (too speculative) - -FINAL REMINDER: Focus on HIGH and MEDIUM findings only. Better to miss some theoretical issues than flood the report -with false positives. Each finding should be something a security engineer would confidently raise in a PR review. - -FALSE POSITIVE FILTERING: - -> You do not need to run commands to reproduce the vulnerability — just read the code to determine if it is a real -> vulnerability. Do not write to any files. -> -> HARD EXCLUSIONS — Automatically exclude findings matching these patterns: -> -> 1. Denial of Service (DOS) vulnerabilities or resource exhaustion attacks. -> 2. Secrets or credentials stored on disk if they are otherwise secured. -> 3. Rate limiting concerns or service overload scenarios. -> 4. Memory consumption or CPU exhaustion issues. -> 5. Lack of input validation on non-security-critical fields without proven security impact. -> 6. Input sanitization concerns for GitHub Action workflows unless they are clearly triggerable via untrusted input. -> 7. A lack of hardening measures. Code is not expected to implement all security best practices, only flag concrete -> vulnerabilities. -> 8. Race conditions or timing attacks that are theoretical rather than practical issues. Only report a race condition -> if it is concretely problematic. -> 9. Vulnerabilities related to outdated third-party libraries. These are managed separately and should not be reported -> here. -> 10. Memory safety issues such as buffer overflows or use-after-free vulnerabilities are impossible in Rust. Do not -> report memory safety issues in Rust or any other memory-safe languages. -> 11. Files that are only unit tests or only used as part of running tests. -> 12. Log spoofing concerns. Outputting unsanitized user input to logs is not a vulnerability. -> 13. SSRF vulnerabilities that only control the path. SSRF is only a concern if it can control the host or protocol. -> 14. Including user-controlled content in AI system prompts is not a vulnerability. -> 15. Regex injection. Injecting untrusted content into a regex is not a vulnerability. -> 16. Regex DOS concerns. -> 17. Insecure documentation. Do not report any findings in documentation files such as markdown files. -> 18. A lack of audit logs is not a vulnerability. -> -> PRECEDENTS: -> -> 1. Logging high-value secrets in plaintext is a vulnerability. Logging URLs is assumed to be safe. -> 2. UUIDs can be assumed to be unguessable and do not need to be validated. -> 3. Environment variables and CLI flags are trusted values. Attackers are generally not able to modify them in a secure -> environment. Any attack that relies on controlling an environment variable is invalid. -> 4. Resource management issues such as memory or file descriptor leaks are not valid. -> 5. Subtle or low-impact web vulnerabilities such as tabnabbing, XS-Leaks, prototype pollution, and open redirects -> should not be reported unless they are extremely high confidence. -> 6. React and Angular are generally secure against XSS. These frameworks do not need to sanitize or escape user input -> unless they are using `dangerouslySetInnerHTML`, `bypassSecurityTrustHtml`, or similar methods. Do not report XSS -> vulnerabilities in React or Angular components or `.tsx` files unless they are using unsafe methods. -> 7. Most vulnerabilities in GitHub Action workflows are not exploitable in practice. Before validating a GitHub Action -> workflow vulnerability ensure it is concrete and has a very specific attack path. -> 8. A lack of permission checking or authentication in client-side JS/TS code is not a vulnerability. Client-side code -> is not trusted and does not need to implement these checks; they are handled on the server side. The same applies -> to all flows that send untrusted data to the backend — the backend is responsible for validating and sanitizing all -> inputs. -> 9. Only include MEDIUM findings if they are obvious and concrete issues. -> 10. Most vulnerabilities in IPython notebooks (`*.ipynb` files) are not exploitable in practice. Before validating a -> notebook vulnerability ensure it is concrete and has a very specific attack path where untrusted input can trigger -> the vulnerability. -> 11. Logging non-PII data is not a vulnerability even if the data may be sensitive. Only report logging vulnerabilities -> if they expose sensitive information such as secrets, passwords, or PII. -> 12. Command injection vulnerabilities in shell scripts are generally not exploitable in practice since shell scripts -> generally do not run with untrusted user input. Only report command injection vulnerabilities in shell scripts if -> they are concrete and have a very specific attack path for untrusted input. -> -> SIGNAL QUALITY CRITERIA — for remaining findings, assess: -> -> 1. Is there a concrete, exploitable vulnerability with a clear attack path? -> 2. Does this represent a real security risk vs. theoretical best practice? -> 3. Are there specific code locations and reproduction steps? -> 4. Would this finding be actionable for a security team? -> -> For each finding, assign a confidence score from 1–10: -> -> - 1–3: Low confidence, likely false positive or noise -> - 4–6: Medium confidence, needs investigation -> - 7–10: High confidence, likely true vulnerability - -WORKFLOW (executed in 3 steps): - -1. Use a sub-task to identify vulnerabilities. Use the repository exploration tools to understand the codebase context, - then analyze the PR changes for security implications. In the prompt for this sub-task, include all of the above. -2. Then for each vulnerability identified by the above sub-task, create a new sub-task to filter out false positives. - Launch these sub-tasks as parallel sub-tasks. In the prompt for these sub-tasks, include everything in the "FALSE - POSITIVE FILTERING" instructions. -3. Filter out any vulnerabilities where the sub-task reported a confidence less than 8. - -POSTING RESULTS — read this section CAREFULLY before posting anything: - -You have access to one MCP tool provided by the `anthropics/claude-code-action` GitHub Action. This is the ONLY correct -way to publish findings — do NOT call the GitHub REST API (`gh`, `octokit`, `curl`) and do NOT attempt to use -`POST /pulls/{n}/reviews` directly. Direct REST calls are not available as tools in this environment. Do NOT attempt to -post a top-level summary comment yourself; the workflow posts the summary after you exit. - -**Tool — `mcp__github_inline_comment__create_inline_comment`.** Posts an inline review comment on a specific file and -line. Each call buffers one comment; the action's post-step posts them all to the PR after this session ends. Call it -once per finding. - -Parameters (all you need for almost every case): - -- `path` (string, required): repo-relative file path, e.g. `".github/workflows/pr-security-review.yml"`. -- `line` (number, required for single-line findings): the diff line number (RIGHT side / new file) where the issue - lives. -- `startLine` (number, optional): pair with `line` for a multi-line range (`startLine` ≤ `line`). -- `body` (string, required): markdown comment body (see format below). -- `side` (`"LEFT"` | `"RIGHT"`, default `"RIGHT"`): leave at default unless commenting on a deleted line. -- Do NOT pass `confirmed`. Letting it default lets the action's classifier filter test/probe-style comments. - -A. **Re-review awareness.** Before posting, READ the existing PR comments included in the diff/git context above. If -your own bot identity has already commented on this PR (look for review comments authored by the bot whose token is -running this workflow), treat this run as a re-review: - -- For each of your prior findings, examine the current diff and explicitly determine whether the issue is now resolved, - still outstanding, or no longer applicable. -- Skip re-posting findings that are already resolved. -- Skip re-posting findings that are still on a line you've already commented on (don't duplicate yourself). - -If you cannot directly read prior comments from the diff context, default to assuming this is a fresh review and post -all current findings. - -B. **Other reviewers.** Where review comments left by other reviewers are visible in the context, factor them into your -analysis: do not contradict an accepted resolution, and do not duplicate a finding another reviewer has already raised. - -C. **Inline review comments.** For each new or still-outstanding finding, call -`mcp__github_inline_comment__create_inline_comment` once with: - -```json -{ - "path": "", - "line": , - "body": "" -} -``` - -The `body` MUST follow this format exactly: - -``` -**Severity:** -**Category:** -**Confidence:** <0.7-1.0> - - - -**Exploit scenario:** - -**Recommendation:** -``` - -When suggesting a code fix, optionally include a GitHub suggestion block at the end of the body: - -```` -```suggestion - -``` -```` - -DO NOT post a single summary comment that lists all findings inline. Each finding gets its own `create_inline_comment` -call. - -D. **No top-level summary comment from you.** Do NOT call any other tool to post a summary or status comment — the -workflow posts a top-level summary comment automatically after this run finishes, based on how many findings you -buffered. If you have no findings, simply exit without calling any tool; the workflow will post the "no findings" -summary. - -E. **Final assistant message.** Your final assistant message should be a one-line plain-text status only, e.g. -`Posted N inline finding(s).` or `No high-confidence findings.` — do NOT include the per-finding markdown in this -message. The inline comments carry the detail. - -START ANALYSIS now. diff --git a/.github/workflows/pr-security-review.yml b/.github/workflows/pr-security-review.yml index daf9fc176..86d08ddf6 100644 --- a/.github/workflows/pr-security-review.yml +++ b/.github/workflows/pr-security-review.yml @@ -3,8 +3,6 @@ name: Claude Security Review on: pull_request_target: types: [opened, reopened, synchronize, labeled] - pull_request_review: - types: [submitted] workflow_dispatch: inputs: pr_number: @@ -22,48 +20,40 @@ permissions: contents: read concurrency: - group: - pr-security-review-${{ github.event.pull_request.number || github.event.review.pull_request_url || inputs.pr_number - }} - cancel-in-progress: true + # Don't cancel-in-progress: a cancelled run that has already started its labels/checkout + # but not the actual review still triggers always() steps and ends up posting a misleading + # "no findings" summary (since the inline-comment buffer is empty when the analysis step + # was skipped due to cancellation). Letting both runs complete is the safer default. + group: pr-security-review-${{ github.event.pull_request.number || inputs.pr_number }} + cancel-in-progress: false jobs: authorize: runs-on: ubuntu-latest - # Filter event subtypes early: - # - pull_request_review: only the 'approved' state authorizes (comment/changes-requested are ignored). - # - pull_request_target labeled: only the 'safe-to-review' label triggers (other label changes are ignored). + # On 'labeled' events, only proceed when the label is exactly 'safe-to-review'. + # Other labels (e.g. size/m) are filtered out so we don't spawn API calls. if: | - (github.event_name != 'pull_request_review' || github.event.review.state == 'approved') && - (github.event_name != 'pull_request_target' || github.event.action != 'labeled' || - github.event.label.name == 'safe-to-review') + github.event_name != 'pull_request_target' || + github.event.action != 'labeled' || + github.event.label.name == 'safe-to-review' outputs: authorized: ${{ steps.auth.outputs.authorized || steps.dispatch-auth.outputs.authorized }} steps: - name: Check authorization id: auth - if: github.event_name == 'pull_request_target' || github.event_name == 'pull_request_review' + if: github.event_name == 'pull_request_target' uses: actions/github-script@v9 with: script: | - // pull_request_target opened/reopened/synchronize: gate on the PR author. - // pull_request_target labeled (safe-to-review): gate on the labeler (sender). - // pull_request_review (approved): gate on the reviewer who approved. - const isApproval = context.eventName === 'pull_request_review'; - const isLabel = - context.eventName === 'pull_request_target' && context.payload.action === 'labeled'; - let user; - let reason; - if (isApproval) { - user = context.payload.review.user.login; - reason = `approver ${user}`; - } else if (isLabel) { - user = context.payload.sender.login; - reason = `labeler ${user}`; - } else { - user = context.payload.pull_request.user.login; - reason = `PR author ${user}`; - } + // pull_request_target opened/reopened/synchronize: gate on the PR author + // (auto-runs on maintainer-authored PRs; community PRs need the label path below). + // pull_request_target labeled (safe-to-review): gate on the labeler (sender) + // so a maintainer applying the label authorizes the run on a community PR. + const isLabel = context.payload.action === 'labeled'; + const user = isLabel + ? context.payload.sender.login + : context.payload.pull_request.user.login; + const reason = isLabel ? `labeler ${user}` : `PR author ${user}`; try { await github.rest.teams.getMembershipForUserInOrg({ org: context.repo.owner, @@ -173,62 +163,35 @@ jobs: uses: actions/checkout@v6 with: ref: ${{ steps.pr.outputs.head_sha }} + # The bundled /security-review skill runs `git diff origin/HEAD...` so we need + # the base branch locally too. fetch-depth: 0 grabs the full history. fetch-depth: 0 - - name: Build security review prompt - id: build-prompt - env: - PR_NUMBER: ${{ steps.pr.outputs.number }} - BASE_REF: ${{ steps.pr.outputs.base_ref }} - HEAD_SHA: ${{ steps.pr.outputs.head_sha }} - run: | - set -euo pipefail - # Ensure we have the base branch locally so origin/ resolves. - git fetch --no-tags origin "$BASE_REF" - BASE="origin/$BASE_REF" - - export GIT_STATUS="$(git status)" - export FILES_MODIFIED="$(git diff --name-only "$BASE"...)" - export COMMITS="$(git log --no-decorate "$BASE"...)" - # Cap diff at ~512KB to keep the prompt under model context limits. - export DIFF_CONTENT="$(git diff "$BASE"... | head -c 524288)" - - OUT="${RUNNER_TEMP}/security-review-prompt.md" - python3 - > "$OUT" <<'PY' - import os, sys - tpl = open(".github/prompts/security-review.md").read() - for k in ("PR_NUMBER","BASE_REF","HEAD_SHA","GIT_STATUS","FILES_MODIFIED","COMMITS","DIFF_CONTENT"): - tpl = tpl.replace("{{"+k+"}}", os.environ.get(k, "")) - sys.stdout.write(tpl) - PY - - echo "prompt_file=$OUT" >> "$GITHUB_OUTPUT" - echo "Prompt size: $(wc -c < "$OUT") bytes" - - name: Configure AWS credentials (OIDC) uses: aws-actions/configure-aws-credentials@v6 with: role-to-assume: ${{ secrets.BEDROCK_SECURITY_REVIEW_ROLE_ARN }} aws-region: us-west-2 - - name: Read prompt file - id: prompt - env: - PROMPT_FILE: ${{ steps.build-prompt.outputs.prompt_file }} - run: | - set -euo pipefail - { - echo "prompt<<__SECREVIEW_EOF__" - cat "$PROMPT_FILE" - echo "__SECREVIEW_EOF__" - } >> "$GITHUB_OUTPUT" - - name: Run Claude Code security review + id: review uses: anthropics/claude-code-action@v1 with: github_token: ${{ steps.app-token.outputs.token }} use_bedrock: 'true' - prompt: ${{ steps.prompt.outputs.prompt }} + # The Claude Code SDK that ships with the action has /security-review bundled + # as a slash command. Invoking it directly lets the skill drive its own + # `git diff origin/HEAD...`, sub-task fan-out, and false-positive filtering + # without us re-implementing any of that. We append a short tail telling the + # action to use the inline-comment MCP tool for findings. + prompt: | + /security-review + + For each finding, call mcp__github_inline_comment__create_inline_comment with + { path, line, body } pointing at the exact file and line in the diff. Do NOT + post a single summary comment listing all findings — the workflow handles a + top-level summary after this run completes. If there are no findings, exit + without calling any tool. show_full_output: 'true' # Allow-listing this MCP tool name is what tells the action to register the # github_inline_comment MCP server. See anthropics/claude-code-action @@ -239,7 +202,10 @@ jobs: - name: Count buffered findings id: findings - if: always() + # Only count if the review step actually ran (success or failure - both produce + # a meaningful buffer state). Skip on cancellation/skip so we don't lie about + # "no findings" when Bedrock was never invoked. + if: steps.review.conclusion == 'success' || steps.review.conclusion == 'failure' run: | set -euo pipefail BUFFER=/tmp/inline-comments-buffer.jsonl @@ -252,22 +218,37 @@ jobs: echo "Buffered findings: $COUNT" - name: Post security review summary comment + # Always post some kind of summary so the PR shows the run happened, but branch on + # the review step's conclusion so a cancelled/skipped run doesn't get reported as + # "no findings". if: always() uses: actions/github-script@v9 env: PR_NUMBER: ${{ steps.pr.outputs.number }} FINDING_COUNT: ${{ steps.findings.outputs.count }} + REVIEW_CONCLUSION: ${{ steps.review.conclusion }} RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} with: github-token: ${{ steps.app-token.outputs.token }} script: | const prNumber = parseInt(process.env.PR_NUMBER, 10); const count = parseInt(process.env.FINDING_COUNT || '0', 10); + const conclusion = process.env.REVIEW_CONCLUSION || 'skipped'; const runUrl = process.env.RUN_URL; - const body = - count > 0 - ? `**Claude Security Review:** posted ${count} inline finding${count === 1 ? '' : 's'} on this PR. ([run](${runUrl}))` - : `**Claude Security Review:** no high-confidence findings. ([run](${runUrl}))`; + + let body; + if (conclusion === 'success') { + body = + count > 0 + ? `**Claude Security Review:** posted ${count} inline finding${count === 1 ? '' : 's'} on this PR. ([run](${runUrl}))` + : `**Claude Security Review:** no high-confidence findings. ([run](${runUrl}))`; + } else if (conclusion === 'failure') { + body = `**Claude Security Review:** the review run failed before completing. See the [run](${runUrl}) for details.`; + } else { + // cancelled / skipped — analysis didn't run, do NOT claim "no findings" + body = `**Claude Security Review:** the review run was ${conclusion} before the analysis could complete (likely superseded or interrupted). See the [run](${runUrl}); a later run on this PR will replace this status.`; + } + await github.rest.issues.createComment({ owner: context.repo.owner, repo: context.repo.repo, diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 296863c11..c46a37610 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -50,18 +50,19 @@ wanted' issues is a great place to start. ## Maintainer notes: Claude security review on community PRs -The `Claude Security Review` workflow runs automatically on maintainer-authored PRs (opened/reopened) and on community -PRs once a maintainer either submits an **approving review** _or_ applies the **`safe-to-review`** label. PRs from -non-collaborators are otherwise skipped — that explicit signal is the gate, so a maintainer must manually review the -diff before the automated reviewer runs. - -The label flow is convenient when you want to kick the security review without committing to an approval: drop the label -on the PR and the workflow fires once. Removing and re-applying the label re-triggers the review. - -To re-run the review on a later commit, submit another approving review, re-apply the label, or trigger the -`Claude Security Review` workflow manually from the Actions tab with the PR number. Note that manual dispatch can verify -the analysis and prompt plumbing but cannot post inline comments — the action's inline-comment MCP server only attaches -on PR-context events (`pull_request_target`, `pull_request_review`). +The `Claude Security Review` workflow runs automatically on maintainer-authored PRs (opened/reopened/synchronize) and on +community PRs once a maintainer applies the **`safe-to-review`** label. PRs from non-collaborators are otherwise skipped +— the label is the gate, so a maintainer must manually review the diff before the automated reviewer runs. + +> **Why the label and not an approving review?** GitHub does not inject repo/org secrets into workflows triggered by +> `pull_request_review` events when the PR head is a fork, regardless of the reviewer's access level. The Bedrock OIDC +> role and GitHub App credentials this workflow needs are only available on `pull_request_target` events, so the label +> path is the only one that works end-to-end for fork PRs. + +To re-run the review on a later commit, remove and re-apply the label, or trigger the `Claude Security Review` workflow +manually from the Actions tab with the PR number. Note that manual dispatch can verify the analysis and prompt plumbing +but cannot post inline comments — the action's inline-comment MCP server only attaches on PR-context events +(`pull_request_target`). ## Code of Conduct