Secure Code Review

---
id: secure-code-review
version: "2.0.0"
title: "Secure Code Review"
description: "The sinks and boundaries that belong to no framework: passing input to a shell or an evaluator, letting input choose a filesystem path, validating at a boundary that is not an HTTP request, and handling regulated data (PII, PHI, cardholder data) so it does not spread into paths nobody reviews. Use when generating or reviewing code that shells out, touches the filesystem, reads a queue message, CLI argument, or parsed file, or when a change crosses a PII, PHI, or PCI boundary."
category: prevention
severity: high
applies_to:
  - "when generating or reviewing code that executes a command or evaluates a string"
  - "when code builds a filesystem path from input"
  - "when validating input that did not arrive as an HTTP request — CLI, queue, env, file"
  - "when a change touches PII, PHI, or cardholder data"
languages: ["*"]
token_budget:
  minimal: 1000
  compact: 1400
  full: 1800
rules_path: "checklists/"
related_skills: ["api-security", "database-security", "crypto-misuse", "file-upload-security", "frontend-security"]
last_updated: "2026-08-14"
sources:
  - "CWE-78 — OS Command Injection"
  - "CWE-22 — Path Traversal"
  - "CWE-20 — Improper Input Validation"
  - "OWASP Top 10 2021"
  - "SEI CERT Coding Standards"
external_tools:
  - name: semgrep
    purpose: "multi-language SAST (OWASP/CWE rulesets)"
    command: "semgrep scan --config auto"
---

# Secure Code Review

## Rules (for AI agents)

### ALWAYS
- Validate input at **every** trust boundary, not only the HTTP one. A CLI argument, an
  environment variable, a queue message, a filename, a parsed config or spreadsheet
  cell all arrive from outside and none of them pass through the request-validation
  layer. Check type, length, allowed characters and allowed range, and reject before
  processing. `api-security` owns the HTTP request body and its schema; this rule is
  every other way input gets in.
- Build a command as an **argument vector**, never as a string a shell will parse:
  `subprocess.run([...])` without `shell=True`, `execFile`/`spawn` with an array,
  `exec.Command(name, args...)`. Passing an array is what removes the shell, and
  removing the shell is what removes the injection — quoting and escaping are the
  fallback for when you cannot.
- **Canonicalize then check** any path built from input: resolve it to an absolute real
  path, then confirm it is inside the intended directory. Checking the raw string first
  misses `..`, symlinks, absolute-path replacement, and encoded separators.
  `file-upload-security` owns stored uploads and archive extraction; this rule covers
  a static-file handler, a template loader, a log path, an export filename.
- Tag any function or module handling **PII, PHI or cardholder data** with an explicit
  classification comment (`// classification: PII`), and keep that handling in
  clearly-named modules rather than in general utility code. An untagged helper is how
  regulated data spreads into paths nobody reviews for it, and the tag is what makes
  the later review possible.
- Consult the owner for anything with one: `database-security` when code builds a
  query, `crypto-misuse` when it hashes, encrypts, signs or compares a secret,
  `frontend-security` for output encoding and response headers, `auth-security` for
  who the caller is, `iam-best-practices` for the privileges a process runs with.
  Each of those states its rule with the API names and the false positives this skill
  would have to leave out.

### NEVER
- Pass input to a shell or an evaluator: `system`, `popen`, `os.system`,
  `subprocess.run(..., shell=True)`, `child_process.exec` / `execSync`, backticks,
  `eval`, `new Function()`, `vm.runInNewContext`, `pickle.loads` on a request body.
  The dangerous property is that the callee parses the string; a value that is only
  ever an argument is not the same risk.
- Let input reach a path without canonicalization — including as a *segment* joined
  onto a trusted base, which is where `../` still works.
- Treat client-side validation as a control. Re-validate on the server, and treat the
  client check as what it is: a message to the user, not a boundary.
- Store a **payment card number, CVV, or full track data** in your own database. Send
  the card to a tokenization provider and persist only the opaque token plus the last
  four digits. The CVV is never persisted at all, not even encrypted, not even briefly
  — retention is the violation, not exposure.
- Copy real regulated data into a fixture, a seed script, a local dump, or a bug
  report to reproduce something. The copy inherits the classification and leaves the
  reviewed path, which is the whole failure this skill's tagging rule exists to
  prevent.

### KNOWN FALSE POSITIVES
- Obviously fake fixture data is not regulated data: `4111-1111-1111-1111`,
  `555-0100`, `John Doe`, `test@example.com`. The classification rules apply to real
  data paths.
- A command built entirely from **constants**, or one whose only variable parts are
  passed as separate argv elements, is not command injection. The finding is a shell
  parsing a string that input reached — not the presence of a subprocess call.
- A path joined from values the application itself generated — a UUID, a hash, a
  database id — is not traversal. The finding is a caller-supplied component.
- A regulated-data module that is *correctly* tagged and narrowly scoped is the rule
  working. The finding is untagged handling in general-purpose code.

## Context (for humans)

This skill covers what is left when every framework-shaped concern has an owner. A
query has `database-security`. A response header has `frontend-security`. An endpoint
has `api-security`. What has no owner is the plain script: something shells out,
something opens a file whose name came from somewhere, something reads a queue message
and believes it. Those are not web vulnerabilities and no framework defends against
them, which is why this is the skill that fires on `languages: ["*"]` and on code with
no framework at all.

The command-execution rule is worth stating as a property rather than a list, because
the list is always incomplete. The property is: does the callee **parse** the string? A
shell does. `eval` does. A deserializer does. An argv array does not, and that is why
switching to one is a fix rather than a mitigation — there is no remaining parser for
an attacker to target.

The regulated-data rules came from a separate compliance skill and are deliberately
narrow. They are not a compliance programme; they are the two decisions that get made
in code and are expensive to reverse — where the classification is written down, and
the fact that cardholder data does not live in your database.

## References

- `references/verifying-findings.md` — confirm or refute a finding, then lock it
- `references/verifying-compliance-findings.md` — tie a flagged control gap to a
  specific clause, then look for the evidence that the control actually fired
- `checklists/owasp_top10.yaml`
- `checklists/injection_patterns.yaml`
- [CWE-78](https://cwe.mitre.org/data/definitions/78.html) · [CWE-22](https://cwe.mitre.org/data/definitions/22.html) · [CWE-20](https://cwe.mitre.org/data/definitions/20.html).
- [OWASP Top 10 2021](https://owasp.org/Top10/).