# Secure Code Review > Review a change or a codebase for security defects with a deliberate reading order — where bugs cluster, trace source to sink, confirm reachability, and write a finding someone can actually fix. Use when asked to security-review a pull request, feature or repository, or before shipping anything touching auth, money, tenancy, uploads or secrets. Not a scanner run (its output is leads), not design-stage threat modeling, and not a penetration test — nothing here authorizes touching a running system. - Skill: `nahid-sparktales/secure-code-review` (Agent Skill, multi-file: 2 files) - Install (CLI): `npx skillmds@latest add nahid-sparktales/secure-code-review` - Raw SKILL.md: https://api.skillmd.com/api/skills/nahid-sparktales/secure-code-review/raw - Safety review: pending - Works with: Claude Code, Claude.ai, OpenAI Codex - Category: Security - Author: nahid-sparktales (https://skillmd.com/u/nahid-sparktales) - Updated: 2026-09-22 - Page: https://skillmd.com/skills/nahid-sparktales/secure-code-review --- # Secure code review A reviewer who reads files in the order the diff lists them finds what a linter finds. The value is in reading in the order defects cluster, following untrusted input to where it does damage, and writing findings precise enough that a fix lands in the right place. ## When this fires A change is about to ship and it touches authentication, authorization, money, personal data, tenancy, uploads, deserialization, URLs or secrets; or a repository needs a security pass. Also on a report that someone reached something they should not have. ## Procedure 1. **Fix the scope and the ground rules in writing.** What is in scope, what the code is supposed to do, and — explicitly — that this is a reading exercise. Running the application, sending payloads, scanning a host or touching production is a separate, authorized act: propose it and **stop and ask**, naming the target and the environment. 2. **Get the ground truth.** For a change: the actual diff, plus the callers of anything it changed — a guard removed in one file is a defect in every file that relied on it. For a repository: the route table, the entry points, and the dependency manifest. 3. **Read in the order defects cluster, not in file order.** New or changed entry points; anything touching identity or permissions; anything building a query, command, path, URL or template; anything handling money, credentials, personal data or tenant scope; configuration and middleware (ordering, CORS, cookie flags, content security policy, dependency bumps); and last, everything labelled temporary, commented out, or added late under pressure. 4. **Trace each candidate both directions.** Backwards to an untrusted source, forwards to the dangerous sink, and check what the path in between actually enforces. A finding is a source, a sink, and a path. Without the path it is a hardening suggestion — still worth saying, but say which it is. Use the `owasp-web` skill for the class-by-class patterns. 5. **Check the cases the author did not write a test for.** The other tenant, the anonymous caller, the expired or replayed token, the second identical request, the bulk endpoint, the id belonging to a different scope, the empty and the enormous input. 6. **Ask what changed about the rules, not only about the lines.** A relaxed database constraint, a middleware registered earlier or later, a default that flipped, a dependency major version, a feature flag now on by default. These are invisible in a diff read line by line and they move the security properties of code nobody touched. 7. **Do not turn into a scanner.** A tool hit you cannot connect to a reachable path is a lead to triage, not a finding to report. Noise trains the reader to skim the one finding that mattered. 8. **Say exactly what you did, per finding.** Read, executed, tested, reproduced — these are different claims. "Reachable by inspection" is honest; "exploitable" requires having run it, in an environment you were authorized to run it in, and you name that environment. 9. **Group by root cause, not by occurrence.** One unscoped shared query reachable from four handlers is one finding with four locations and one fix — at the choke point every caller goes through. Four findings invite four local patches and leave the fifth caller open. 10. **Write each finding so it can be fixed without you**: file and line, the untrusted input, the path to the sink, the concrete consequence to a named asset, the smallest correct fix, and your confidence with what evidence would settle it. 11. **Rank by consequence and reachability.** Unauthenticated, no preconditions, hits credentials or the whole dataset, at the top. A category name is not a severity: not every injection outranks every access-control gap, and usually the reverse. 12. **Report coverage and blind spots in the same document.** What was read, what was skipped, what could not be assessed and why. A review with no stated limits reads as a clean bill of health it did not earn. 13. **Fixing and disclosing are separate decisions.** Proposing a patch is part of the review; committing it, pushing it, or writing an unfixed vulnerability into a public issue or PR comment is outward-facing — **stop and ask** before any of those. ## Checklist - [ ] Scope agreed, and no running system touched without an explicit ask - [ ] Diff read together with the callers of everything it changed - [ ] Clustered reading order followed, config and middleware included - [ ] Each candidate traced source → path → sink; hardening labelled separately - [ ] Negative cases considered: other tenant, anonymous, replay, bulk, expired - [ ] Rule changes (constraints, defaults, ordering, dependency majors) examined - [ ] Tool output triaged for reachability, not pasted - [ ] Read / executed / tested / reproduced distinguished in every claim - [ ] Findings grouped by root cause, with the fix at the shared choke point - [ ] Severity argued from consequence and reachability - [ ] Coverage and blind spots stated - [ ] Nothing committed, pushed or published without asking ## Failure handling - **You cannot tell whether input is trusted** — say so and name what would settle it (the caller, the middleware, the schema). An assumed-safe input is how the interesting bugs survive review. - **The path runs through code you cannot follow** — dynamic dispatch, generated code, a vendored binary — report the trace as far as it goes and mark it unresolved. Unresolved is not safe. - **The fix is architectural and large** — size it and say so rather than proposing a local patch that adds a second enforcement point. Two checks that can disagree are worse than one badly placed. - **You find something serious in unrelated code** — report it; do not quietly widen the change. Scope is about what you touch, not about what you are allowed to notice. - **Nothing was found** — say what was read and what was not. "No findings in the six handlers and the auth middleware; the queue consumers and the infrastructure config were not reviewed" is a result. "Looks fine" is not. - **The change is already merged or deployed** — that changes urgency and sequencing, not the review. Report first, and let the people who own the system decide on rollback or a fix forward. ## Evidence to report The scope and what was read, by file or area; per finding, the location, the source-to-sink trace, the consequence, the fix and the confidence; the severity ranking with its reasoning; tool output with its triage; the explicit list of what was not reviewed and what could not be assessed; and, for anything you ran, exactly what you ran and against which environment.