security-review

name	security-review
description	Complete a security review of pending changes on the current branch. Focuses on injection, authn/authz, secrets, SSRF, deserialisation, insecure defaults, and supply-chain risk. Use when the user asks to "security review", "security audit", "check for vulns", or before raising a PR.

Security Review Skill

You are conducting a security review of the changes on this branch. The goal is to find real, exploitable issues — not produce a generic OWASP checklist.

Step 1 — Determine review scope

1. If the user names a PR, fetch via gh pr diff <n>.
2. Otherwise review branch vs main:

   base=$(git merge-base HEAD main)
   git diff "$base"...HEAD --name-only
   git diff "$base"...HEAD
   

3. If neither produces a diff, fall back to staged changes (git diff --cached).

State the scope in one sentence.

Step 2 — Gather context

• Read each changed file in full — security bugs hide in surrounding context (where input enters, how output is used, what privileges the process holds).
• Read any CLAUDE.md and .env-template to understand secrets handling and trust boundaries.
• Note any new dependencies added to pyproject.toml, requirements.txt, package.json, etc.

Step 3 — Threat focus

Review for the following classes. For each finding, you must be able to point to a specific line and describe a plausible attacker action. Vague concerns are not findings.

Injection

• SQL: string concatenation / f-strings into queries instead of parameterised statements.
• Command: subprocess with shell=True, os.system, unvetted Popen args built from user input.
• Code: eval, exec, pickle.loads, yaml.load (without SafeLoader), marshal.loads on untrusted data.
• Template: server-side template injection (Jinja render_template_string with user input).
• Log injection / log forging from unsanitised user input.

AuthN / AuthZ

• New endpoints or CLI commands lacking auth checks present on siblings.
• Authorisation decisions made on client-supplied IDs without ownership verification (IDOR).
• Token / session handling: weak generation, missing expiry, logged tokens.
• Hardcoded credentials, API keys, or default passwords.

Secrets & data exposure

• Secrets committed to the repo (look for high-entropy strings, AKIA*, ghp_*, xoxb-*, private keys).
• Secrets written to logs, error messages, or exception traces.
• Sensitive data returned in responses where it shouldn't be (PII, tokens, internal IDs).

SSRF / outbound requests

• requests.get(user_supplied_url) without allowlist / scheme check.
• URL parsing used for auth decisions (classic SSRF bypass via @, IPv6, DNS rebinding).
• Webhooks or callbacks that fetch arbitrary URLs.

Deserialisation & parsing

• pickle, cPickle, shelve, dill on untrusted bytes.
• XML parsers without XXE protection (defusedxml not used).
• ZIP / tarfile extraction without path traversal protection (zipslip).

Path traversal & file handling

• Joining user input into filesystem paths without os.path.realpath containment check.
• open() / file writes whose path includes request data.

Crypto

• MD5 / SHA1 for security purposes.
• ECB mode, static IVs, hardcoded keys.
• random instead of secrets for tokens.
• TLS verification disabled (verify=False, InsecureRequestWarning suppressed).

Supply chain

• New dependencies: are they pinned? Do they resolve via the org's Nexus proxy (per CLAUDE.md / org policy) rather than public PyPI?
• Typosquat-prone names (requets, urllib4).

Insecure defaults

• debug=True, permissive CORS (*), open bind addresses (0.0.0.0) without justification.
• Disabled CSRF, SameSite=None without rationale.

Race conditions & TOCTOU

• File checks followed by file use without atomicity.
• Auth check / state change separated by I/O.

Step 4 — Report

Use CVSS-flavoured severity (Critical / High / Medium / Low / Info), but rank by exploitability in this codebase, not the abstract worst case.

Output format:

## Security Review: <one-line scope>

### Critical
- **`file.py:42` — <vuln class>**
  Attack: <what an attacker does, concretely>
  Fix: <one-line remediation>

### High
- ...

### Medium
- ...

### Low / Info
- ...

### Out of scope / accepted
<things you considered but ruled out — keep terse, only if non-obvious>

### Verdict
<one sentence: safe to ship / fix blockers first / needs broader threat modelling>

Rules:

• Always cite file:line. No findings without a concrete location.
• A "finding" needs a plausible attacker and a plausible impact. If you can't write the Attack line, it's not a finding — at most an Info note.
• Do NOT pad with generic OWASP advice. The reader knows what XSS is.
• If the diff is security-clean, say so in one line and end with the Verdict.
• For new dependencies, name them explicitly and note whether they resolved via the approved internal registry.

Step 5 — If invoked from /pr

When the caller is /pr, your output is embedded in the PR body's Security notes section. Keep the report self-contained — anyone reading the PR should understand each finding without running the skill themselves.