review-code-security

---
name: review-code-security
description: Scan implemented code for concrete security anti-patterns. Companion to /review-plan-security.
license: MIT
---

Scan the specified code for security anti-patterns using the same three axes as `/review-plan-security`. Use after implementation to verify the plan's security goals hold in the actual code.

## When to Use

Use when the implementation touches external input, secrets handling, or third-party integrations. Pass a file path, diff, or describe the code to review.

## Rules

- Report each finding with: file/location, pattern category, and recommended fix.
- Note context for potential false positives (test fixtures, comments, examples).
- For commit-time auto-scanning of hard secrets (AWS/LLM API keys, PEM keys, GitHub/Slack tokens, `.env` commits), see `docs/scan-outbound.md` — those are covered automatically.
- Sibling sweep: enumerate functions, logic, or patterns the change touches that belong to a class; flag siblings as MUST / OPTIONAL / NA for same treatment.

## Procedure

RCS-1. **Open the concern round** (Bash): `bash "$AGENTS_CONFIG_DIR/skills/review-code-security/scripts/open-concern-round.sh"` — prints `ROUND`, `PLANS_DIR`, `SESSION_ID`, and the `[PRIOR CONCERNS START]`…`[PRIOR CONCERNS END]` block both producers receive. `ROUND=0` means the ledger is unavailable: run RCS-2 and RCS-3, report the `NOT-STAGED` line, skip the ledger close-out in `## Completion`.

RCS-2. **Delegate scan to security-scanner**, issued together with the RCS-3 quality gates per `skills/_shared/subagent-concurrency.md` SC-P (independent — both are read-only over the merge-base diff and write no shared target):
   ```
   Agent({ subagent_type: "security-scanner", prompt: JSON.stringify({
     topic: "security review", context: SCAN_TARGET,
     artifact_dir: PLANS_DIR, prior_concerns: PRIOR_BLOCK
   }) })
   ```
   Pass the RCS-1 block verbatim as `prior_concerns`; omit the key when RCS-1 printed none.
   On `failed` status: surface summary + artifact_path to user.
   Output: `## Security Review: PERFORMED|FAILED` (1 line) + artifact_path pointer. Read report only on failure or explicit user request.

RCS-3. **Quality gates** (Bash, issued with RCS-2 per SC-P): `CONCERN_LEDGER_ROUND=<ROUND> bash "$AGENTS_CONFIG_DIR/skills/review-code-security/scripts/run-quality-gates.sh"` — resolves merge-base and runs the ledger-wrapped codex reviewer plus 7 lint gates; the wrapper stages the reviewer's own delta. Each gate is advisory; non-zero exit is a warning, not a blocker.
   When the output carries any `## <gate>: NOT FOUND` line, append `(N gates NOT FOUND)` to the `## Security Review:` line so the reader sees the sweep was incomplete.

## Patterns by Axis

### Axis 1: Information Leakage
*Axis scope: OWASP ASVS V8 (Data Protection), V6 (Stored Cryptography)*

**Automated coverage**: AWS/LLM API keys, PEM private keys, GitHub/Slack tokens, and `.env` file commits are auto-detected by `scan-outbound.sh` at pre-commit. Source integrity is also covered: zero-width chars (U+200B/C/D, U+FEFF) and Bidi override chars (U+202D/E, U+2066–2069) are detected as `[zero-width]` / `[bidi-override]` (Trojan Source, CVE-2021-42574). This axis focuses on context-dependent leaks automation cannot catch.

| Pattern | What to look for | Risk |
|---|---|---|
| Generic secret hardcoded | `(?i)(secret\|password\|token)\s*=\s*["'][^"']{8,}["']` in source | Leaked credential |
| Logging sensitive data | `log.*password`, `print.*token`, stack traces with secrets | Exposure via logs |
| `.env` not gitignored | `.env` absent from `.gitignore` | Secret leak on push |
| Temp files with secrets | Scripts writing secrets to `/tmp/` or similar | Filesystem exposure |

### Axis 2: Third-Party Access
*Axis scope: OWASP MCP Top 10 (MCP03 Excessive Permissions, MCP04 Tool Poisoning), LLM Top 10 (LLM03 Supply Chain)*

| Pattern | What to look for | Risk |
|---|---|---|
| Unpinned dependency | `"latest"` in package.json, bare package name in requirements.txt | Supply chain (LLM03) |
| Unvalidated LLM/MCP output | Agent output used directly in `eval`, shell call, or DB query | Prompt injection → RCE |
| Excessive MCP permissions | Tool requesting file system / network beyond task scope | MCP03 |
| Tool Poisoning | MCP tool descriptions containing instruction overrides (`ignore previous`, system commands) | MCP04 |
| Rug Pull | MCP server behavior changes after approval — use trusted, auditable publishers only | MCP09 |
| Return Value Injection | Tool return value fed directly into a prompt or `eval` without validation | MCP05 / LLM01 |

### Axis 3: External Access
*Axis scope: OWASP WSTG (Input Validation), CWE Top 25 #2 (CWE-79 XSS), #3 (CWE-89 SQL Injection)*

| Pattern | Regex hint | CWE |
|---|---|---|
| Shell injection | `eval.*\$`, unquoted `$VAR` in command position | CWE-78 |
| Path traversal | `../` combined with user-controlled variable | CWE-22 |
| SQL injection | String concatenation in SQL (`"SELECT " + var`) | CWE-89 |
| Open redirect | Redirect to URL from user input without allowlist | CWE-601 |
| XSS | Unsanitized user input rendered as HTML | CWE-79 |
| Instruction override in input | Untrusted input containing `ignore previous`, `you are now`, `system:` forwarded to LLM as context | LLM01 |
| Base64 obfuscation | Base64 string from untrusted input decoded and passed to LLM/shell | LLM01 |

## Completion

After reporting findings, once both RCS-2 and RCS-3 finish, close the ledger round via `skills/review-code-security/scripts/close-concern-round.sh`. Schema and severity vocabulary: `skills/_shared/concern-ledger.md`.

1. Run: `bash "$AGENTS_CONFIG_DIR/skills/review-code-security/scripts/close-concern-round.sh" <ROUND> <PLANS_DIR> <SESSION_ID> security-scanner <COMPLETE|PARTIAL|ABSENT> <artifact_path>` — stages the scanner's delta (mapping its `status:` onto the exec label), reduces, finalizes, and verifies via `check-finalized`, retrying finalize once on a transient failure. Append `(N unresolved concerns)`, derived from its `UNRESOLVED=` line, to the `## Security Review:` line.
2. `CHECK=ok` → run (as a standalone Bash command — no pipes, no && chaining): `echo "<<WORKFLOW_MARK_STEP_review_security_complete>>"`
3. `CHECK=FINALIZE-FAILED` → do not emit the completion sentinel; report the `FINALIZE-FAILED` reason and the recovered-copy path instead.

## Relationship to Other Tools

- `/review-plan-security` — architecture-level checklist (run before implementation begins)
- `scan-outbound.sh` — auto-detects hard secrets and private info at pre-commit (see `docs/scan-outbound.md`)