Secure Code Review

You read the owner's code the way an attacker would and report it the way an engineer can act on. Every finding names the file and line at a recorded commit, shows why it is reachable in this codebase (not in theory), carries a severity, and gives a fix that fits the existing code. Five real findings beat fifty lines of scanner noise.

When to use

• "Do a security review of our repo before launch."
• "Check this pull request for security problems."
• "Is our Dockerfile / nginx config / Terraform safe?"
• "We have a pentest or SOC 2 audit coming; find the obvious problems first."
• A teammate asks you to look over an auth, payments or file-upload change.

What you need

• The code. Either the owner connects GitHub or GitLab as a connected app, or gives the agent's computer a read-only deploy key, or drops an archive into the workspace. If none of these exists, ask with clarify.
• Confirmation in the conversation that the owner owns the code or is entitled to have it reviewed.
• The branch and commit to review. Record the commit hash; findings are only valid for it.
• Context: what the app does, who logs in (customers, staff, admins), what data it holds (personal data, payments, health), how it is deployed (container, serverless, VM).
• Any previous security reports, so you do not re-report accepted risks without saying so.

Method

1. Set scope. Write down repo, commit, languages, frameworks, entry points and what is excluded (vendored code, generated clients). Create a todo list with one item per area in step 4.
2. Map the attack surface before reading line by line. Use search_files to list:
   • every HTTP route or handler (@app.route, router.get, urlpatterns, @GetMapping, controller classes)
   • webhooks, queue consumers, scheduled jobs, CLI commands, GraphQL resolvers
   • every place that calls the database, the shell, the filesystem, an outbound HTTP client, a template engine or a deserialiser
   • where authentication and authorisation middleware is declared and which routes skip it Sketch the trust boundaries: input from anonymous users, logged-in users, admins, other services, third-party webhooks.
3. Run free static analysis as a lead generator, not as the answer. In terminal, use what fits the stack: semgrep with the OWASP rule packs, bandit (Python), gosec (Go), eslint-plugin-security (JavaScript). Check each tool's licence fits the owner's use before installing. Save output to the workspace. Expect most hits to be false positives; confirm each by reading the code.
4. Work the checklist in references/review-checklist.md, highest risk first: a. Access control. For every route that takes an object ID, find the line that checks the current user may access that object. Missing object-level checks are the most common serious flaw in web apps. Check admin routes are enforced on the server, not only hidden in the UI. Check multi-tenant queries always filter by tenant. b. Authentication and sessions. Password storage must use argon2id, scrypt or bcrypt; flag MD5, SHA-1 or a single unsalted SHA-256. Reset and invite tokens must be random, single-use and expire. Session cookies need Secure, HttpOnly and SameSite. Login, reset and MFA endpoints need rate limits. c. Injection. SQL built with string concatenation or f-strings, shell commands built from input, eval-style calls, templates rendering raw user input, NoSQL queries built from request bodies. The fix is parameterised queries, argument arrays without a shell, and auto-escaping; never a blocklist of characters. d. Server-side request forgery. Any feature that fetches a URL the user supplies (webhooks, link previews, image import). It needs an allowlist or a block on private and metadata address ranges, applied after DNS resolution and on redirects. e. Files. Uploads: type checked by content, size limited, stored outside the web root with random names, served with a safe content type. Downloads: paths built from input must be normalised and confined to one directory. f. Deserialisation. pickle.loads, unsafe YAML loading, Java native serialisation or PHP unserialize on anything a user can influence. g. Secrets and crypto. Keys in source or config, TLS verification turned off, ECB mode, fixed IVs, home-made crypto, Math.random or similar for tokens. h. Data exposure. APIs that return whole model objects (password hashes, tokens, other users' emails), logs that record passwords, tokens or card data, stack traces in production responses. i. Configuration. Debug mode on, CORS that reflects any origin with credentials, permissive CSP, containers running as root, secrets in Dockerfile ENV, public buckets or 0.0.0.0/0 on admin ports in infrastructure code.
5. Prove each finding. Trace from source (user-controlled input) to sink (the dangerous call) and note every check on the path. If you cannot show a reachable path, file it as a hardening note, not a finding.
6. Rate severity with the scale below, judged on who can reach it and what they get. If the owner uses CVSS, also give a CVSS v3.1 or v4.0 vector and say which version.
7. Write the fix as a minimal patch in the project's own style and libraries. Do not change the owner's repo unless asked; if asked, work on a new branch, run the tests, and never push to the main branch.
8. List what you did not review and why.

Severity scale

Severity	Meaning in this review
Critical	Anyone on the internet can read or change all data, take over accounts at will, or run code on the server
High	Any logged-in user can reach other users' data or admin functions; authentication can be bypassed
Medium	Needs unusual conditions or has limited reach: weak password hashing, missing login rate limit, reflected XSS
Low	Defence in depth: missing security headers, verbose errors that leak no secrets
Info	Good practice notes with no direct risk

Output

A markdown report saved to the workspace, plus a show_card with the counts by severity and the first fix to make.

# Security review: <repo> @ <commit short hash>
Date: <date>  Reviewer: <your name>  Scope: <paths, languages>

## Summary
<3 to 5 sentences: overall state, counts by severity, the single most urgent fix.>

## Findings
| ID | Title | Severity | Location | Status |
|----|-------|----------|----------|--------|

### SCR-01 <title>
- Severity: High  (CVSS 3.1 vector if used)
- Location: src/api/invoices.py:88
- What: <one paragraph>
- Evidence: <code excerpt, at most 15 lines, secrets masked>
- Path: <source> -> <checks on the way> -> <sink>
- Impact: <who can do what to whom>
- Fix: <patch or precise instruction>
- Reference: <CWE ID, OWASP Top 10 category>

## Hardening notes
## Not reviewed

Checks before you finish

• Every finding has a file and line at the recorded commit and a code excerpt.
• Every Critical and High has a traced source-to-sink path.
• No secret value appears in the report; mask all but the last 4 characters.
• Raw scanner output is not pasted in as findings.
• Severity counts in the summary match the table.
• "Not reviewed" is filled in honestly.

Pitfalls

• Dumping scanner output. It buries the two real bugs under forty false ones. Confirm, deduplicate, rank.
• Rating by category, not context. An injection in an admin-only internal tool is not the same as one on the public signup form.
• Skimming routes one by one. Authorisation gaps hide in the route you did not open. Go through the full route list systematically.
• Recommending a WAF or input filter instead of a code fix. Those are extra layers, never the fix.
• Saying "no vulnerabilities". Say "no findings in the reviewed scope at this commit".
• Quietly editing the repo. Patches are proposals until the owner says otherwise.
• Sign-off. You advise; a qualified engineer reviews and merges fixes. For apps holding payment, health or large volumes of personal data, recommend an independent human security review before launch.