security-review

name	security-review
description	Use when reviewing code for vulnerabilities, checking diffs for injection/XSS/auth/crypto issues. Invokes on code changes (.go/.py/.js/.ts/.java/.rs/.php/.rb), not docs-only diffs.
user-invocable	true
disable-model-invocation	false
allowed-tools	Read Grep Glob Bash Task
license	Apache-2.0

Security Review Skill

Identify exploitable security vulnerabilities in code. Investigate before flagging. Report findings with confidence levels and severity.

Skip condition: If the diff contains no executable code (docs-only, JSON-only config, comments-only), skip the review and record skipped — diff is docs-only (or similar) in the **Security:** field. If the diff touches any .go/.py/.js/.ts/.tsx/.jsx/.java/.rs/.php/.rb file, this skill MUST be invoked — no skip.

Scope: Research vs. Reporting

CRITICAL DISTINCTION:

• Report on: Only the specific file, diff, or code provided
• Research: The ENTIRE codebase to build confidence before reporting

Before flagging any issue, research the codebase to understand:

• Where does this input actually come from? (Trace data flow)
• Is there validation/sanitization elsewhere?
• How is this configured? (Check settings, config files, middleware)
• What framework protections exist?

Do NOT report issues based solely on pattern matching. Investigate first, then report.

Confidence Levels + NEW/MODIFIED Doctrine (adapted for yesloop)

The Sentry original says "Do not report LOW". That is rejected here. We also reject the "fix if <2 min" time-estimate rule from earlier Phase 5 doctrine. Instead we use a bright-line NEW vs MODIFIED distinction — a fact about the diff, not an estimate.

Why NEW vs MODIFIED instead of time estimates

1. Time estimates are unreliable for agents and humans. "NEW vs MODIFIED" is a fact determined from git diff.
2. Greenfield has zero compat constraints — no API contract, no migration, no downstream callers that can break.
3. Training signal — shipping LOW findings trains future agents that LOW is acceptable. yesloop builds production code.
4. Compounding debt — 50 LOW findings = 4+ hours of tech-debt cleanup later plus re-orientation per finding.

Action Matrix

Classify each finding by whether the vulnerable lines are added by this diff (NEW) or in existing code the diff touches (MODIFIED):

Code Type in Diff	HIGH	MEDIUM	LOW
NEW (added file / added function / added logic block)	Fix immediately + document	Investigate dataflow + fix (or document "not exploitable because X")	Fix — no exception
MODIFIED (existing function touched)	Fix introduced issues	Fix introduced issues	Fix introduced issues; pre-existing = document "OUT OF SCOPE, Learning #TBD"
Docs/config only	n/a — skip entire review	n/a	n/a

Pre-existing issues in surrounding code (not introduced by this diff) are OUT OF SCOPE — document them with a Learning reference (yesmem_remember) so they surface later, but do not fix them here (avoids scope creep).

Severity Definitions (for describing findings)

Severity	Impact	Examples
Critical	Direct exploit, severe impact, no auth required	RCE, SQL injection to data, auth bypass, hardcoded secrets
High	Exploitable with conditions, significant impact	Stored XSS, SSRF to metadata, IDOR to sensitive data
Medium	Specific conditions required, moderate impact	Reflected XSS, CSRF on state-changing actions, path traversal
Low	Defense-in-depth, minimal direct impact	Missing headers, verbose errors, weak algorithms in non-critical context

Do Not Flag

General Rules

• Test files (unless explicitly reviewing test security)
• Dead code, commented code, documentation strings
• Patterns using constants or server-controlled configuration
• Code paths that require prior authentication to reach (note the auth requirement instead)

Server-Controlled Values (NOT Attacker-Controlled)

These are configured by operators, not controlled by attackers:

Source	Example	Why It's Safe
Process env / config	os.Getenv("DATABASE_URL"), os.LookupEnv	Set at deployment
Config files	config.yaml, viper.GetString, app.config['KEY']	Server-side files
Framework constants	http.StatusOK, default timeouts	Compile-time constants
Hardcoded values	baseURL := "https://api.internal"	Compile-time constants
Internal service URLs from config	cfg.UpstreamURL	Operator-controlled

SSRF Example — NOT a vulnerability:

// SAFE: URL comes from config (server-controlled)
resp, err := http.Get(cfg.SeerAutofixURL + path)

SSRF Example — IS a vulnerability:

// VULNERABLE: URL comes from request (attacker-controlled)
resp, err := http.Get(r.URL.Query().Get("url"))

Framework-Mitigated Patterns

Check before flagging. Common false positives:

Pattern	Why It's Usually Safe
Go html/template {{.Field}}	Auto-escaped by default (context-aware)
Go text/template {{.Field}}	NOT auto-escaped (flag for HTML output)
React {variable}	Auto-escaped by default
Django {{ variable }}	Auto-escaped by default
Parameterized SQL db.Query("...$1", input)	Parameterized query
ORM db.Where("id = ?", input)	Parameterized

Only flag these when:

• Go templates: template.HTML(userInput), template.URL(userInput), template.JS(...) — explicit bypass of escaping
• React: dangerouslySetInnerHTML={{__html: userInput}}
• Django: {{ var|safe }}, {% autoescape off %}, mark_safe(user_input)
• SQL: string interpolation fmt.Sprintf("SELECT ... WHERE id=%s", input), db.Raw(stringWithInput), db.Exec with concatenated SQL

Review Process

1. Detect Context

What type of code am I reviewing?

Code Type	What to check
API endpoints, routes	auth, authz, injection, IDOR, mass assignment
Frontend, templates	XSS, CSRF
File handling, uploads	path traversal, uploads, XXE
Crypto, secrets, tokens	algorithms, key management, randomness
Data serialization	pickle/yaml/JSON deserialization
External requests	SSRF
Business workflows	race conditions, workflow bypass
Config, headers, CORS	misconfiguration
CI/CD, dependencies	supply chain
Error handling	fail-open, information disclosure
Logging	log injection, secret leakage

2. Identify Language

Based on file extension or imports:

Indicators	Apply patterns from
.go, go.mod	Go stdlib + popular pkgs (net/http, database/sql, os/exec, html/template, crypto/*)
.py, django, flask, fastapi	Python: subprocess, pickle, yaml, eval/exec, Django/Flask/FastAPI
.js, .ts, express, react, vue, next	JavaScript: child_process, eval, innerHTML, dangerouslySetInnerHTML
.rs, Cargo.toml	Rust: unsafe, FFI, Command::new with user input
.java, spring, @Controller	Java: ObjectInputStream, Runtime.exec, Spring templates
.php, .rb	PHP/ Ruby: unserialize, eval, shell exec

3. Research Before Flagging

For each potential issue, research the codebase to build confidence:

• Where does this value actually come from? Trace the data flow with graph_traverse / grep for callers.
• Is it configured at deployment (env, config) or from user input (request)?
• Is there validation, sanitization, or allowlisting upstream?
• What framework protections apply (auto-escaping, parameterization)?

Only flag issues where you have at least MEDIUM confidence after understanding the broader context.

4. Verify Exploitability

For each finding, confirm:

Is the input attacker-controlled?

Attacker-Controlled (Investigate)	Server-Controlled (Usually Safe)
r.URL.Query(), r.FormValue(), r.Header.Get(...) (most headers)	os.Getenv, viper.GetString
request.GET, request.POST, request.args (Python)	Hardcoded constants
req.body, req.json (Node)	Internal service URLs from config
request.cookies (unsigned)	Database content from admin/system
URL path segments: /users/{id}	Signed session data
File uploads (content and names)	Framework settings
Database content from other users
WebSocket messages

Does the framework mitigate this?

• Check for auto-escaping, parameterization
• Check for middleware/decorators that sanitize

Is there validation upstream?

• Input validation before this code
• Sanitization libraries

5. Report Findings

All three confidence levels land in the **Security:** field — never silently dropped.

---

Quick Patterns Reference

Always Flag (Critical)

eval(user_input)                       # Any language
exec(user_input)                       # Python
pickle.loads(user_data)                # Python
yaml.load(user_data)                   # Python (not safe_load)
unserialize($user_data)                # PHP
ObjectInputStream.readObject(user)     # Java
shell=True + user_input                # Python subprocess
child_process.exec(user)               # Node.js
exec.Command(userInput...)             # Go os/exec

Always Flag (High)

innerHTML = userInput                  # DOM XSS
dangerouslySetInnerHTML={user}         # React XSS
v-html="userInput"                     # Vue XSS
template.HTML(userInput)               # Go template escape bypass
fmt.Sprintf("SELECT ... WHERE id=%s", user)  # SQL injection
`SELECT * FROM x WHERE ${user}`        # SQL injection (JS)
os.system(fmt.Sprintf("cmd %s", user)) # Command injection (Go)

Always Flag (Secrets)

password = "hardcoded"
api_key = "sk-..."
AWS_SECRET_ACCESS_KEY = "..."
private_key = "-----BEGIN"

Check Context First (MUST Investigate Before Flagging)

# SSRF - ONLY if URL is from user input, NOT from settings/config
http.Get(r.URL.Query().Get("url"))     # FLAG: User-controlled URL
http.Get(cfg.UpstreamURL)              # SAFE: Server-controlled config
http.Get(cfg.Base + "/" + path)        # CHECK: Is 'path' user input?

# Path traversal - ONLY if path is from user input
os.Open(r.URL.Query().Get("file"))     # FLAG: User-controlled path
os.Open(cfg.LogPath)                   # SAFE: Server-controlled config
filepath.Join(baseDir, filename)       # CHECK: Is 'filename' user input? Note: filepath.Join cleans `..` — verify

# Open redirect - ONLY if URL is from user input
http.Redirect(w, r, r.URL.Query().Get("next"), 302)  # FLAG
http.Redirect(w, r, cfg.LoginURL, 302)               # SAFE

# Weak crypto - ONLY if used for security purposes
md5.Sum(fileContent)                   # SAFE: File checksums, caching
md5.Sum([]byte(password))              # FLAG: Password hashing
math/rand.Intn(N)                      # SAFE: Non-security uses (UI, sampling)
math/rand.Intn(N) for token            # FLAG: Security tokens need crypto/rand

---

Output Format

Write findings to the **Security:** field of the Phase 5 scratchpad block. Distinguish NEW (diff-added code) from MODIFIED (existing code touched by diff) findings. Every finding must carry either a fix-commit-hash OR an "OUT OF SCOPE" annotation with Learning reference OR the block is "skipped" with a reason.

**Security:** reviewed via security-review skill
- NEW findings (in diff-added code):
  - [HIGH] injection in foo.go:42 — FIXED in commit abc123
  - [LOW] missing input length check in bar.go:10 — FIXED in commit def456
- MODIFIED findings:
  - [MEDIUM] SSRF risk introduced by baz.go:78 — FIXED in commit 789abc
  - [LOW pre-existing] weak hashing in qux.go:100 — OUT OF SCOPE (pre-existing, surrounding code; Learning #TBD)
- Skipped: none | OR "skipped — diff is docs-only (.md files only)"

Or if no findings at all:

**Security:** reviewed via security-review skill — none, diff reviewed, no HIGH/MEDIUM/LOW findings.

Or if skipped:

**Security:** skipped — diff is docs-only (.md files only).

Finding-line grammar

Each finding line carries:

• [SEVERITY] — Critical / High / Medium / Low
• (file:line) — location
• short description — what the vulnerability is
• disposition — FIXED in commit <hash> | not exploitable because <evidence> | OUT OF SCOPE (pre-existing; Learning #<id>)

NEW findings never use "OUT OF SCOPE" — they are in the diff, they are in scope. Only MODIFIED findings can carry pre-existing OUT-OF-SCOPE annotations, and only for issues in surrounding code the diff did not introduce.

---

What NOT to Do

• Do NOT skip investigation — always trace dataflow before flagging.
• Do NOT report on test files unless reviewing test security specifically.
• Do NOT flag server-controlled configuration as attacker-controlled.
• Do NOT silently drop LOW findings — fix NEW LOWs, document MODIFIED pre-existing LOWs as OUT OF SCOPE.
• Do NOT mark findings as exploitable without confirming attacker-controlled input.
• Do NOT review code you didn't actually read.
• Do NOT apply the "fix if <2 min" rule — that's deprecated. Use the NEW/MODIFIED bright line.
• Do NOT scope-creep into fixing pre-existing issues in surrounding MODIFIED code — document them, do not fix.