Security Review Skill

---
name: security-review
description: Review code for security defects. Use before merging changes that touch authentication, authorization, user input, file handling, secrets, or external requests, or when the user asks for a security review.
---

# Security Review

Find defects an attacker could actually use. A finding without an attack path is noise, and a review full of noise trains people to skip the next one.

## Scope first

Establish before reading: what changed, what data this code touches, who can reach it (anonymous internet, authenticated user, another tenant, internal only), and what the worst outcome would be. A missing check on an internal admin tool and the same check on a public endpoint are different findings.

## Follow the input

For every piece of data that originates outside the trust boundary (request parameters, headers, uploaded files, webhook payloads, third-party API responses, database rows written by users), trace it to where it is used. The vulnerability is at the destination, not the source.

Check each destination:
- **A query**: is it parameterized, or is user data concatenated into it
- **A shell or process call**: is any user data in the command, and is a shell involved at all
- **A file path**: can it escape the intended directory
- **A URL fetched by the server**: can it be pointed at internal addresses
- **A template or the DOM**: is it escaped for that context
- **Deserialization, XML parsing, or archive extraction**: are dangerous features disabled

## Check authorization on every object

For each resource this code returns or modifies: what proves the caller is allowed to touch that specific object, not merely that they are logged in. Broken object-level authorization is the most common serious flaw in web applications and the least visible in a diff, because the code reads correctly when you assume the caller owns the object.

Also check: privilege changes, whether the role check happens before or after the effect, and mass-assignment of fields the caller should not control.

## Then check

- **Secrets**: hardcoded credentials, keys in logs or error messages, secrets in client-side code or committed config
- **Authentication**: session fixation, missing expiry, tokens in URLs, weak reset flows, missing rate limits on credential endpoints
- **Crypto**: home-rolled algorithms, weak hashing for passwords, static IVs, predictable randomness for security purposes
- **Errors**: stack traces or internal identifiers returned to users
- **Dependencies**: new packages, unpinned versions, typosquatted names
- **Prompt injection**, where an LLM is involved: untrusted content reaching an instruction position, tool calls triggered by document content

## Report

Per finding, most severe first:

**[critical|high|medium|low] Claim**
- Where: `file:line`
- Attack path: what the attacker starts with, the steps, and what they get
- Impact: what is exposed, changed, or destroyed
- Fix: the smallest change that closes it, preferring a fix that removes the class of bug over one that catches this instance

## Rules

- Report only what you can defend with an attack path. Say "no exploitable finding" and list what you checked when that is the truth; an honest empty review is more useful than a padded one.
- Do not report a missing defense-in-depth control as a vulnerability. Note it separately.
- Never write or run an exploit against anything outside this codebase.
- Where you could not verify a control because it lives elsewhere (a gateway, middleware, a proxy), say so rather than assuming it exists or assuming it does not.