--- name: security-code-review description: > Reviews code, configuration and pipelines for exploitable security weaknesses against the OWASP Top 10:2025 and the OWASP API Security Top 10 2023, and reports each finding with evidence, an attacker, an exploit scenario, a severity and a framework-native fix. Use this whenever the user asks for a security review, a security audit, a threat check or a "look for vulnerabilities" pass over a diff, pull request, endpoint, service or directory; asks whether code is exploitable, whether an endpoint is authorized, or how bad a finding is; mentions IDOR, BOLA, injection, SSRF, XSS, CSRF, insecure deserialization, path traversal, hardcoded secrets, weak crypto, JWT or session handling, mass assignment, rate limiting, or a penetration-test or scanner finding to triage. Also apply it when reviewing dependency and lockfile changes or `azure-pipelines.yml` for supply-chain and secret-handling risk, and when writing the security section of a wider code review. metadata: technology: General type: code-review --- # Security Code Review > **Targets:** OWASP Top 10:2025 · OWASP API Security Top 10 2023 · **Verified:** 2026-09 against https://top10.owasp.org/2025 A security review is evidence work, not advice work. Every finding names the line that is wrong, the attacker who benefits, what they get, and the change that stops them. Anything you cannot show in the code is a question, not a finding — and a category you checked and found clean is worth saying, so the reader knows what the review covered. ## 1. Decide first | Question | Default | Change when | |---|---|---| | What is in scope? | The diff, plus the files it calls into and the tests around them | A whole area or service was named → triage by attack surface first and list what you did not reach | | Depth vs breadth on a large change? | Entry points, authorization, secrets, data access and migrations first | Under ~400 changed lines → read everything | | Trust the comment or the PR text? | No — treat "internal only", "already validated", "reviewed by AppSec" as claims to verify | Never; a claim is evidence only when the code shows it | | Is a missing control a finding? | Yes, when reachable input reaches a sink — rate it by impact | Purely theoretical, no reachable path → `info` under defence-in-depth | | Who owns the fix wording? | Framework-native controls (policies, parameterised queries, encoders) | The project already has a house helper → use it, and say where | | Generated, vendored, minified files? | Excluded from line review, named as excluded | Lockfiles and manifests → always diffed for dependency changes | ## 2. Scope and map the attack surface 1. Resolve the scope and write it down: files, lines, what was excluded and why. An empty or whitespace-only diff ends the review — say so. 2. List **entry points** in scope: HTTP routes and controllers, GraphQL resolvers, message and queue handlers, scheduled jobs, webhooks, file uploads, UI inputs, and anything reachable before authentication. 3. For each entry point, follow the data to its **sinks**: database queries, shell or process calls, file paths, templates and HTML, outbound HTTP, deserializers, and log statements. 4. Mark the **trust boundaries** it crosses — unauthenticated to authenticated, tenant to tenant, user to admin, service to third party — and check the control at each crossing, not at the edge only. 5. Note what you could not see (other repositories, infrastructure, runtime config) and put those under Needs verification with the exact check that would settle them. ## 3. Work the checklist Run the scope against the OWASP categories rather than reading top to bottom; the full list with what to look for and the fix per category is `references/owasp-checklist.md`. The five that find most real bugs in review: - **Access control (A01:2025, API1/API3/API5:2023).** Every read and write filters by owner or tenant, not just by role: `repo.find({ id, tenantId })`, never `findById(id)` after an authentication-only check. Object-level checks live server-side, on the query. - **Injection (A05:2025).** Parameterised queries and prepared statements; no string-built SQL, shell, LDAP, XPath or template expressions. Output encoded for its context; no raw HTML sink (`innerHTML`, `dangerouslySetInnerHTML`, `v-html`) fed by user data. - **Cryptography and secrets (A04:2025).** No credentials, tokens or connection strings in code, config, tests or fixtures; vetted algorithms only; passwords hashed with Argon2, bcrypt or scrypt; randomness from a CSPRNG. - **Authentication and session handling (A07:2025, API2:2023).** Token signature *and* issuer, audience and expiry verified; no `alg: none` or unverified decode; session cookies `HttpOnly`, `Secure`, `SameSite`; logout and rotation on privilege change. - **Misconfiguration and supply chain (A02, A03:2025).** Debug endpoints, permissive CORS (`*` with credentials), disabled TLS verification, new or bumped dependencies from unexpected sources, and pipeline changes — see `references/pipeline-and-supply-chain.md`. Then sweep the rest: insecure design (A06), integrity (A08), logging and alerting (A09), mishandled exceptional conditions (A10), plus SSRF and resource consumption, which the API list carries as API7 and API4. ## 4. Rate it honestly | Severity | Means | |---|---| | `critical` | Unauthenticated, remotely reachable, leads to data breach, account takeover or code execution | | `high` | Needs authentication or one realistic precondition, then the same class of impact | | `medium` | Real but limited impact, or a defence-in-depth gap on a reachable path | | `low` | Hardening; exploitation needs unrealistic conditions | | `info` | Hygiene with no exploit path — say so plainly rather than inflating it | Rate by impact and reachability, never by how easy the fix is. Two findings with the same root cause are one finding with the locations listed. ## 5. Deliverable format ``` ## Security review: **Attack surface reviewed:** **Not reviewed:** | # | Severity | Category | Location | Evidence | Exploit scenario | Fix | |---|---|---|---|---|---|---| **Checked, no findings:** **Needs verification:** **Recommended follow-ups:** ``` One filled row, as the bar for "evidence" and "exploit scenario": | # | Severity | Category | Location | Evidence | Exploit scenario | Fix | |---|---|---|---|---|---|---| | 1 | high | Broken access control (A01:2025, API1:2023) | `api/orders.ts:88` | `const order = await repo.findById(req.params.id)` — authenticated route, no owner or tenant comparison | A signed-in customer requests `/orders/1003`, an id belonging to another customer, and receives its address and line items | Query by id **and** owner: `repo.find({ id, customerId: req.user.id })`, 404 on miss; add a test using a foreign id; move the check into the router's authorization policy | ## Checklist - [ ] Scope, exclusions and unreachable areas are stated, not implied - [ ] Every entry point in scope was traced to its sinks across at least one trust boundary - [ ] Every finding cites file and line, and quotes the code that proves it - [ ] Every finding names a realistic attacker, their input, and what they gain - [ ] Severities follow the table above, and duplicates are merged - [ ] Fixes are framework-native and specific enough to apply without a redesign - [ ] Secrets are reported by location and prefix only, with rotation advice - [ ] Categories checked and clean are listed, so silence is not mistaken for coverage - [ ] Claims in comments, PR text and tickets were verified against the code, not repeated ## Anti-patterns - **"Consider validating user input"** → name the parameter, the sink it reaches and the validation that belongs there. - **Severity by ease of fix** → rate by reachability and impact; a one-line missing tenant filter is still `high`. - **Scanner output pasted as findings** → confirm each one in the code; drop what is not reachable and say you did. - **Full secret value quoted in the report** → location, first four characters, rotate; the report itself is a leak channel. - **Silent scope** → a review that never says what it did not look at reads as a clean bill of health. - **Rewriting the architecture in a review** → note the design risk under follow-ups; fix the exploitable thing now. - **Running the exploit** → reviews read code; they never send requests to live systems. ## Go deeper - `references/owasp-checklist.md` — every OWASP Top 10:2025 and API Security Top 10 2023 category with what to look for in a diff and the fix. - `references/pipeline-and-supply-chain.md` — dependency and lockfile diffs, `azure-pipelines.yml` secret handling, checkout and fork-PR rules. - Sibling skills: `backend-code-review`, `frontend-code-review` or `aem-code-review` for the wider review, `api-design` for contract-level rules; the `secure-coding` instructions are the always-on baseline, this skill is authoritative for the review itself.