Security Review

security-review

Focused security review of a diff, a file, or a small module. Not a comprehensive audit — that's an engagement. This finds the classes of bug that regularly ship because nobody looked, and it's specific about what it did and did not check.

When to use this

• "Review this for security"
• "Check for vulns"
• "Did I leak a secret in this diff"
• Before pushing / opening a PR that touches: auth, session, crypto, user input, SQL, shell exec, file paths from users, deserialization, eval, regex on untrusted input, or third-party API calls with secrets
• After bumping a dependency to a version with a known CVE (npm audit, pip-audit, cargo audit)

Not for: comprehensive audits of an entire codebase (too big for a skill; scope carefully), threat modeling, cryptographic protocol design, or regulated-compliance certification. Say so if asked.

Procedure

1. Establish scope

Ask for or infer the exact scope. Options in priority order:

1. A named file or files.
2. A branch diff: git diff <base>...HEAD.
3. Staged changes: git diff --cached.
4. A directory or module.

Do NOT try to review a whole repo in one pass. If the scope is too large (more than ~500 changed lines or ~10 files touching sensitive areas), say so and offer to review the most sensitive slice first.

2. Run the fast, cheap checks first

Before deep reading, do the mechanical scans. These are cheap and catch the highest-severity, dumbest bugs.

Secrets in diff:

• git diff <range> | grep -iE '(api[_-]?key|secret|token|password|bearer|BEGIN [A-Z ]+PRIVATE KEY|xox[baprs]-|ghp_|sk-|AKIA[0-9A-Z]{16}|AIza[0-9A-Za-z_-]{35})'
• Also check for .env-shaped strings: [A-Z_]+=[^ ]+ in non-config files
• If anything hits, halt and report the exact lines. Do not continue until the user confirms whether it's real.

Dependency CVEs:

• Detect package manager from lockfile
• Suggest running: npm audit, pip-audit, cargo audit, bundle audit, or equivalent — don't run automatically; the user does it
• If the diff bumps a dependency, check the version bump for a corresponding advisory

3. Read for the OWASP-shaped classic bugs

Look at each changed file for these patterns. Note anything that matches; don't guess when unsure.

Injection:

• SQL — string concatenation of user input into SQL, template literals with query fragments, missing parameterization. execute("SELECT * FROM u WHERE id = " + id) is a hit. Check for prepared statements / parameter binding / ORM usage.
• Command / shell — subprocess.run(..., shell=True) or backticks or os.system with user input. shell=False + list args is safe.
• NoSQL — find({field: userInput}) without validation, especially if the field allows $where or operators.
• XSS — innerHTML = userInput, unescaped template output, missing htmlspecialchars / escape. React auto-escapes; dangerouslySetInnerHTML reverses that.
• XXE / SSRF — user-controlled URLs passed to fetch / requests.get, XML parsers without external-entity restriction.

Auth / access control:

• Missing authorization checks on state-changing endpoints
• IDOR: GET /users/:id returning any user's data without checking whether the caller can view that ID
• Session fixation, missing CSRF tokens on state-changing routes
• Weak password hashing (MD5, SHA1, plain SHA256 without salt)
• JWT with alg: none, JWT verification skipped, JWT secret hardcoded

Crypto / data:

• Home-rolled crypto instead of stdlib primitives
• ECB mode block cipher, static IV, missing MAC
• Insecure randomness for security purposes (Math.random(), random.random())
• Timing-attack-vulnerable comparisons on secrets (== on tokens; should be constant-time compare)

Deserialization:

• pickle.load on untrusted input, yaml.load (not safe_load), unserialize, Java readObject on external data — all remote-code-exec waiting to happen

File & path:

• Path joining with user input without normalization → path traversal (../../etc/passwd)
• File upload endpoints that don't check extension AND content type AND filename length
• Symlink attacks if the code follows symlinks in user-controlled dirs

Race conditions on security-relevant state:

• TOCTOU: check-then-use with a gap (e.g. check file permissions, then read)
• Non-atomic "check if user exists, then create user" without unique constraint

4. Write the findings

For each real finding, structure it:

### <severity> · <one-line title>

**File / line:** path/to/file.py:42

**What:** <one-sentence description of the bug>

**Why it matters:** <what an attacker could do with this>

**Fix:** <concrete change — code or approach>

**Confidence:** <high | medium | low — how sure you are this is real>

Severity:

• Critical — direct compromise of secrets, data, or system integrity
• High — meaningful data exposure or bypassable auth
• Medium — exploitable under some conditions; defense-in-depth failure
• Low — style / hygiene / hardening opportunity

Order the findings most-severe first. If there are none, say so — do not manufacture findings. "No security issues found in the reviewed scope (). Not reviewed: <what you didn't cover>." is a valid and honest report.

5. Say what you did and did NOT check

End with an explicit scope reminder. Reviewers of your review need to know what you covered:

Reviewed: <files, diff range, or module>
Did not review: <what you deliberately skipped and why>
Not in scope: <things a security review can't verify without running
              the code / a specialist audit — e.g. cryptographic proof,
              side-channel analysis, business logic soundness>

Anti-patterns

• Do not run any exploit or PoC yourself. Describe how the bug would be exploited in the write-up; do not demonstrate.
• Do not report "you should use HTTPS" or other generic hygiene as a finding unless it's actually violated in the code you reviewed.
• Do not upgrade the confidence of a finding to make it sound scarier. "Low confidence, might be a bug" beats a false positive that erodes trust.
• Do not review your own code with less rigor. If you wrote the change earlier in the conversation, apply the same procedure.