/security-review

name: security-review
model: sonnet
effort: standard
description: >
  AI-powered security analysis of code changes — traces data flow, detects
  injection, auth bypass, secrets exposure, and unsafe deserialization across
  files. Use when reviewing pending changes, before release-branch, during
  verify-work Phase 5, during build-epic Step 0 threat modeling, or when
  the user says "security review" or "scan for vulns".

story: e45s41

<!-- story: e45s18 -->

story: e26s01

story: e26s02

story: e26s03

story: e26s04

story: e26s05

story: e26s06

story: e26s07

story: e45s26

Security Review

> **HARD GATE** — Requires git context (branch with merge-base or diff). Never > writes files outside `specs/security/`. Findings below confidence 8/10 are > suppressed. Pre-flight: `git rev-parse HEAD >/dev/null 2>&1`

Parallel worktree mode (e45s18)

When running alongside `audit-code`, use isolated worktrees so scans do not race on the same index:

bash scripts/lib/parallel-review-worktrees.sh security-review

Each check gets a detached worktree at `.bigpowers/worktrees/review-<name>/`; reports still write only under `specs/security/`.

5-phase scan

| # | Phase | What | |---|-------|------| | 1 | **Scope Resolution** | Detect diff via `git diff --merge-base origin/HEAD`; resolve languages/frameworks from dependency files | | 2 | **Context Research** | Identify existing security patterns, sanitization, auth model in the codebase | | 3 | **Vulnerability Assessment** | Trace user input → sink; check auth boundaries, crypto, deserialization, path ops | | 4 | **False-Positive Filtering** | Cross-check each finding against exclusion rules; reject confidence < 8 | | 5 | **Report Generation** | Output structured markdown: file:line, severity, category, exploit scenario, fix |

Categories

Covered: SQLi, XSS, SSRF, command injection, auth bypass, unsafe deserialization, path traversal, IDOR, crypto flaws, secrets exposure, template injection, NoSQLi

CWE mapping mandate (e45s26)

Every **new detection rule** added to this skill MUST:

1. Map to a [CWE](https://cwe.mitre.org/) ID in `REFERENCE-vuln-categories.md` (e.g. SQLi → CWE-89, XSS → CWE-79). 2. Ship **two fixture pairs** under `skills/security-review/fixtures/`:

• **Positive** — minimal code the rule MUST flag (vulnerable pattern present).
• **Negative** — structurally similar code the rule MUST NOT flag (safe pattern / false-positive guard).

| Rule | CWE | Positive fixture | Negative fixture | |------|-----|------------------|------------------| | SQL injection | CWE-89 | `fixtures/CWE-89-sqli-positive.py` | `fixtures/CWE-89-sqli-negative.py` | | XSS (DOM) | CWE-79 | `fixtures/CWE-79-xss-positive.js` | `fixtures/CWE-79-xss-negative.js` | | Missing tenant scoping (IDOR) | CWE-639 | `fixtures/CWE-639-idor-positive.go` | `fixtures/CWE-639-idor-negative.go` | | Fail-open verify directive | CWE-754 | `fixtures/CWE-fail-open-verify-positive.sh` | `fixtures/CWE-fail-open-verify-negative.sh` |

Before merging a new category, run both fixtures through the detection guidance and confirm positive flags / negative passes.

SQL-safety doctrine (e45s41 — proven authorship)

Formal rule for SQL injection classification:

| SQL source | Attacker-reachable input? | Verdict | |------------|---------------------------|---------| | Hardcoded / compile-time constant string | N/A | **Safe** — proven authorship | | Developer-authored query with bound parameters only | No dynamic fragments from user input | **Safe** | | String concatenation / template with user-controlled values | Yes | **Unsafe** — report as SQLi | | ORM query builder with user input in WHERE/JOIN | Yes | **Unsafe** unless parameterized | | Stored procedure call with bound args | Args from trusted constants only | **Safe** | | Stored procedure with dynamic SQL inside | User input reaches EXEC | **Unsafe** |

**Provenance test:** If the agent cannot prove the query string was authored entirely by the developer (no attacker-reachable interpolation), treat as vulnerable. Hardcoded SQL in migrations, seeds, and admin scripts is safe; anything reachable from HTTP/CLI/user input is not.

BCP Plus Integration

This skill maps to **BCP Plus dimension 12 (Security & Compliance)**. When BCP Plus sizing is active, the threat model categories above correspond to sub-elements within dimension 12. The NFR Gate rule applies: standard-expectation items (e.g., "use HTTPS", "hash passwords") score 0 with a one-line rationale; only above-standard security requirements contribute to the dimension 12 count. See `docs/references/bcp-plus.md` for the full 13-dimension framework and NFR Gate pattern.

Integration points

| Skill | Touchpoint | |-------|------------| | `build-epic` | Step 0 — threat-model epic scope → `specs/security/epics/<id>/THREAT_MODEL.md` | | `plan-work` | `security:` field (none/low/medium/high) on story tasks | | `plan-release` | +2 WSJF risk boost for HIGH+ risk epics | | `audit-code` | Checklist: "diff scanned — no unaddressed HIGH findings" | | `request-review` | Inject threat model categories + false-positive rules into reviewer prompt | | `investigate-bug` | Security-impact assessment in RCA (NONE→CRITICAL) | | `validate-fix` | Recurrence hardening check for security bugs | | `verify-work` | Phase 5 — blocks on HIGH findings ≥ 8 confidence | | `release-branch` | Hard gate — blocks merge if unresolved HIGH findings |

Report format

Each finding: **`File:Line` — Severity — Category**

• Description: how the vulnerability manifests
• Exploit scenario: concrete attack path
• Recommendation: fix with code example

Reference files

• [Vuln categories](REFERENCE-vuln-categories.md) — detection guidance per vuln type
• [False positives](REFERENCE-false-positives.md) — hard exclusions + precedent
• [Confidence rubric](REFERENCE-confidence-rubric.md) — scoring methodology (0–10)

Verify

→ verify: `test -d specs/security && test -f scripts/