security-pr-reviewer

Security PR Reviewer

Purpose

Give a change a security verdict backed by evidence: severity-ranked findings anchored to file:line in the real diff, each high-severity finding carrying an exploit path or abuse scenario, and an approve / request-changes decision with the blocking findings named. This is the security lens on a specific diff — narrower and deeper on security than code-reviewer, but working on the same artifact (an obtained diff), never on an imagined change. On SaaS data paths, tenant isolation and object-level authorization are mandatory review dimensions, checked whether or not the PR description mentions them.

Use When

• Use when: asked to security-review a PR, branch-against-base, commit, or the current staged/working diff.
• Use when: gating a change on security before merge, or a change touches auth, data access, uploads, external calls, or config.
• Do NOT use when: the request is a general correctness/quality review — that is code-reviewer (this skill is the security-specialized pass).
• Do NOT use when: there is no diff yet and the ask is design-time threat enumeration — threat-modeler.
• Do NOT use when: the risk is dependency/CI supply chain — supply-chain-security-reviewer.
• Do NOT use when: the change is a database migration whose safety is the question — secure-migration-reviewer.
• Do NOT use when: the input is SAST/CodeQL/SARIF output to triage — static-analysis-reviewer.
• Never security-review from a description of a change: no diff, no review.

Inputs to Inspect

1. The actual diff and its base: gh pr diff <n> / git diff <base>...<branch> / git diff + git diff --staged. Record which was reviewed. No diff → Stop Conditions.
2. The stated intent (PR description, linked issue) — but the review judges what the code does, not what the description claims.
3. Surrounding unchanged code of each hunk: the authz middleware the new route relies on, the query the new input flows into, the caller that sets scope — security bugs live in the contract between changed and unchanged code.
4. The security context: tenant model and authorization matrix for the data paths touched; existing validators/sanitizers the change should use.
5. New tests (or their absence) for the security-relevant behavior — a security fix without a negative test is unverified.

Workflow

1. Obtain the real diff and base; note size and which files carry security weight (auth, data access, config, uploads, external calls).
2. Map the change to boundaries. For each hunk, identify the trust boundary it touches (client→server, tenant→tenant, server→DB, app→3rd party) and review against that boundary's threats.
3. Authn/authz pass: new/changed endpoints authenticated? Authorization checked at the RESOURCE (object-level), not just the route or UI? On SaaS data access, is tenant scope server-derived and applied to every query the diff adds? (mandatory rows — see Security Rules).
4. Injection & input pass: untrusted input reaching SQL/NoSQL/shell/ template/deserialization; parameterization and contextual encoding present; mass assignment of restricted fields (role, tenant_id, owner_id, price).
5. Secrets & config pass: secrets added to code/config/logs; server secrets exposed to the client bundle; unsafe logging of tokens/PII; SSRF or open-redirect via new user-supplied URLs.
6. Security-relevant change pass: does the diff weaken an existing control (loosened policy, removed check, broadened CORS/permissions)? Flag any control removal explicitly.
7. Rank and evidence findings. Each finding: severity, file:line, and for HIGH+ a concrete exploit path or abuse scenario (persona → steps → payoff). No exploit path → cap at medium and label "needs verification".
8. Judge test adequacy: are the security behaviors pinned by tests that would fail if the control were reverted? Name the gaps.
9. Deliver the verdict (approve / approve-with-nits / request-changes) with blockers listed and what was NOT reviewed (generated files, areas out of the diff) so coverage is honest.

Output Format

SECURITY PR REVIEW — <PR/branch/diff id> (base: <ref>, N files)
Reviewed diff via: <command>
Verdict: approve | approve-with-nits | request-changes
Boundaries touched: <client→server | tenant→tenant | server→DB | app→3rd party>
Findings (severity-ranked):
  [CRITICAL|HIGH] <file:line> — <vuln>; exploit: <persona → steps → payoff>; fix: <remediation>
  [MEDIUM|LOW]    <file:line> — <issue>; fix: <remediation>
SaaS checks: tenant scope <server-derived? applied to new queries?> | object-level authz <present?>
Control changes: <any existing control weakened/removed — explicit>
Tests: <security behaviors pinned? gaps — would a revert be caught?>
Not reviewed: <exclusions + why>

Validation Checklist

• Reviewed an actual obtained diff; command recorded; no review from prose.
• Every hunk mapped to the trust boundary it touches.
• Object-level authorization and tenant-scope checks assessed on every SaaS data path the diff adds — even if the PR didn't mention them.
• Injection, mass-assignment, secrets, and SSRF/redirect passes done.
• Any weakening/removal of an existing control is flagged explicitly.
• Every HIGH+ finding has an exploit path/abuse scenario; path-less claims capped at medium and labeled.
• Test adequacy judged by "would a revert of the control be caught?".
• Verdict names blockers; not-reviewed list present.

Security Rules

• No diff, no review — never infer a change's security from its description.
• HIGH/CRITICAL findings require an exploit path or abuse scenario (master-prompt §6); without one, rank medium and mark needs-verification.
• Tenant isolation and object-level authorization are mandatory review dimensions on SaaS data paths, not optional, regardless of the PR's stated scope.
• A change that weakens or removes a security control is a finding on its own, even if the new state still "works".
• Findings are never suppressed to unblock a merge; accepted risk needs the human's written rationale via human-approval-boundary.
• This skill reviews; it does not fix. Fixes are a separate change (appsec-implementer).

Gotchas

• The vulnerability is often in the unchanged code the diff now reaches — a new caller feeding an old unparameterized query; review the contract, not the hunk alone.
• Authorization at the route or in the UI is not object-level authorization — the new endpoint may check "is a member" but not "owns THIS record".
• A diff that adds a second route to existing data (GraphQL beside REST) can bypass the isolation the first route enforced — check for parallel paths.
• Secrets slip in via config/test fixtures/log statements, not just source — scan the whole diff, including non-src files.
• A green CI run means existing tests pass, not that the new security behavior is tested — read the test diff, not the badge.
• Large or generated-heavy diffs induce rubber-stamping; say where scrutiny thinned rather than implying uniform coverage.

Stop Conditions

• No diff can be obtained → stop; do not review from a verbal description.
• The diff changes security policy, RLS, or auth flows in ways whose implications are unclear → flag for human security review via human-approval-boundary; do not approve on inference.
• The security-relevant change is a DB migration or RLS policy → hand the policy/migration portion to rls-policy-auditor / secure-migration-reviewer.
• The finding is a dependency/CI supply-chain issue rather than app code → hand to supply-chain-security-reviewer.
• Asked to both review and silently fix in one pass → review first; fixes are a separate, explicitly-approved change.

Supporting Files

• references/security-review-passes.md — the per-pass checklists (authz, injection, secrets, control-change) with boundary-specific prompts and the exploit-path template for high-severity findings.
• evals/evals.json — trigger + behavior cases.
• evals/trigger-evals.json — discrimination against secure-migration-reviewer, static-analysis-reviewer, supply-chain-security-reviewer, and the shipped code-reviewer (security-review cluster).