| name | security-review |
| description | Security review of the diff — the third review axis alongside Standards and Spec. Reviews for injection, auth, secrets, deserialization, SSRF, path traversal, unsafe operations, and dependency confusion. Use as a parallel sub-agent with fresh context (diff-only, anti-anchored) during the review phase. |
| provenance | auto-pi-original |
Security Review
The third review axis in auto-pi's review phase — alongside Standards (does the code follow the repo's conventions?) and Spec (does the code match what was asked for?). This axis answers: does the diff introduce security vulnerabilities?
Runs as a parallel sub-agent with fresh context — only the diff, not the builder's reasoning — so the security review is not anchored by the implementer's assumptions.
Smell catalog
Each smell reads what it is → how to spot it in the diff → how to fix. Match against the diff hunks:
-
Injection — user-controlled input reaches a dangerous sink without sanitization.
- SQL injection — string-concatenated queries, unparameterized
query(), ORM raw queries with user input. → parameterize; use prepared statements / parameterized query builders.
- Command injection — user input in
exec(), execSync(), spawn() shell mode, backticks, subprocess.run(shell=True). → use argument arrays (execFile, spawn without shell: true); never interpolate.
- Template injection — user input in template engines (
eval, ejs, jinja2, nunjucks) that execute code. → use logic-less templates or auto-escaping; never eval user content.
- XSS — unescaped user input rendered in HTML (
innerHTML, dangerouslySetInnerHTML, |safe in Django, {!! !!} in Blade). → escape on output; use framework auto-escaping; CSP headers.
-
Authentication / authorization — missing or bypassable access control.
- Missing auth checks — a route, endpoint, or function that should require authentication but doesn't. → add the auth middleware/guard.
- Privilege escalation — a user can perform actions above their role (horizontal or vertical). → check role/permission at the action site, not just the route.
- IDOR (Insecure Direct Object Reference) — user can access another user's resource by changing an ID in the URL/params. → verify ownership:
where("user_id = ?", currentUser.id).
- Missing ownership checks — the code loads a resource by ID without checking the requester owns it. → add ownership scoping.
-
Secrets — credentials exposed in code or config.
- Hardcoded credentials — API keys, passwords, tokens, private keys in source. → move to env vars / secret manager; rotate if committed.
- API keys in code —
const API_KEY = "sk-...". → env var; .env in .gitignore.
- Secret logging — tokens, passwords, PII written to logs. → redact before logging; never log secrets.
- Secrets in committed config —
.env not in .gitignore, config.json with prod keys. → .gitignore + secret manager.
-
Unsafe deserialization — untrusted data parsed into executable or object form.
eval() / Function() on user input. → never; use JSON.parse (no code execution).pickle.loads() / yaml.load() (unsafe) on untrusted input. → yaml.safe_load; avoid pickle for untrusted data.JSON.parse of attacker-controlled data with prototype pollution (__proto__). → use Object.create(null) or a safe schema; validate keys.unserialize() in PHP, readObject() in Java. → use safe alternatives; whitelist classes.
-
SSRF (Server-Side Request Forgery) — the server makes requests to attacker-controlled URLs.
- Server-side fetch/HTTP to a user-supplied URL (webhooks, image fetching, URL previews). → validate and restrict: allowlist domains, block internal IPs (127.0.0.1, 169.254.169.254, 10.x, 192.168.x), follow-redirect limits.
- Internal endpoint access from external input. → same restrictions.
-
Path traversal — user input in file paths allows escaping the intended directory.
../ sequences in file paths, path.join(userInput, ...) without normalization. → normalize and verify the result is within the allowed root: path.resolve() + check startsWith(allowedDir).- Symlink attacks — following symlinks created by an attacker. → use
fs.realpath; check the resolved path.
-
Unsafe operations — dangerous actions without safeguards.
- Destructive git —
push --force, reset --hard, branch deletion without confirmation. → require explicit user confirmation; never automate.
- Shell exec with user input —
exec(userInput). → see Injection; use argument arrays.
- Unvalidated redirects —
redirect(userInput) (open redirect). → allowlist redirect targets; reject absolute URLs.
- ReDoS — regex that can cause catastrophic backtracking on user input (
(a+)+). → use safe regex patterns; test with long inputs; consider re2.
-
Dependency confusion / untrusted packages — supply-chain risks in the diff.
- New dependencies from untrusted sources, missing integrity checks. → verify package source, check for known vulnerabilities (
npm audit, pip-audit), pin versions.
- Dependency confusion (package exists in multiple registries with different content). → use scoped registries; verify
package-lock.json / requirements.txt integrity.
- Importing from URLs / git repos instead of registries. → prefer registry packages; verify checksums.
Review brief (sub-agent prompt)
Run the security review as a parallel sub-agent with fresh context. The sub-agent gets only the diff — not the builder's reasoning, not the spec, not the plan — so the review is anti-anchored.
Security sub-agent prompt — include:
- The full diff command and commit list (same as Standards/Spec sub-agents).
- The smell catalog above, pasted in full — the sub-agent has no other access to it.
- The brief: "Review this diff for security vulnerabilities ONLY. For each finding: (a) name the smell category, (b) cite the file:line, (c) quote the vulnerable hunk, (d) explain the attack scenario, (e) state severity (CRITICAL / HIGH / LOW), (f) suggest the fix. If a category has no findings, state it is clean. Skip anything tooling already enforces (e.g. a linter that blocks
eval). Under 400 words."
Anti-anchored rules:
- Do NOT read the PR description, the commit message rationale, or the builder's plan — these are self-grading and may downplay security issues.
- Do NOT trust comments in the code that say "safe" or "validated" — verify the validation exists in the diff.
- Do NOT skip a category because "this repo doesn't do X" — if the diff touches X, review it.
Severity guidance
Map each security finding to one of three severities:
- CRITICAL — the vulnerability allows unauthenticated remote code execution, authentication bypass, or mass data exfiltration. The system is exploitable as-is. → must fix before merge; blocks ship.
- HIGH — the vulnerability allows authenticated privilege escalation, injection with limited impact, or exposure of sensitive data to a subset of users. Exploitable but requires access or specific conditions. → should fix before merge; blocks ship unless explicitly accepted by the user.
- LOW — the vulnerability requires unusual conditions, has limited impact, or is a defense-in-depth concern (e.g. missing rate limiting, verbose error messages leaking internal paths). → nice to have; can ship with a follow-up ticket.
No self-grading downgrade: An implementer's stated rationale ("it's an internal tool", "the input is trusted") is self-grading, not external evidence. It must NOT downgrade a finding's severity. If the rationale points at the deployment context rather than the code, that's a PLAN_DEFECT (route to the planner), not a reason to soften the security finding.
Aggregation
Security findings are added to the review findings: list alongside Standards and Spec findings. Each finding has:
<severity> <file>:<line> <smell category> — <description>
If no security findings, state security: clean with the categories checked (so the reviewer didn't just skip them).
The findings flow into the review-disposition phase — the orchestration layer dispositions each finding (verified-fix, verified-defer, rejected, needs-user-decision). Security CRITICAL findings that are verified-fix block ship until resolved.
Provenance
This skill is an auto-pi original — no Matt Pocock upstream. It exists because ask-matt's code-review is two-axis (Standards + Spec) with no security methodology; auto-pi adds the third axis with a real smell catalog and sub-agent brief.
