Security review playbook

Security Review Playbook

Purpose

Review and harden a codebase proportionally to a WRITTEN threat model — not to a generic checklist of everything that could ever be insecure. Two failure modes cost equally: under-hardening a real boundary (webhook accepting unsigned payloads) and over-hardening a non-boundary (enterprise auth on a single-user local CLI). The threat model, written first, is what tells them apart.

When to Use / When NOT to Use

Use when:

• Code accepts input from outside the process: webhooks, HTTP endpoints, file uploads, CLI args from untrusted sources.
• Code deserializes anything (JSON, pickle, YAML, numpy blobs) or builds SQL/shell/URLs from variables.
• Code makes outbound requests whose destination could be influenced by input (SSRF surface).
• Asked to "do a security review", or a diff touches auth, secrets, or parsing.
• After ANY hardening change — to write the tests that lock it in.

Do NOT use when:

• The question is where secrets are stored/loaded → load configuration-management (come back here for how they're validated).
• A vulnerable dependency version is the issue → load dependency-management.
• You're writing the regression tests themselves and need test methodology → load validation-and-testing.
• The "security concern" is speculative hardening with no boundary behind it → load engineering-minimalism and re-read the threat model.
• Investigating how a past incident happened → load failure-archaeology.

Core Methodology

Step 1 — Write the threat model FIRST (mandatory, before any hardening)

Answer in writing, in the repo (README, SECURITY.md, or module docstring):

1. Who can send input to this system? (the whole internet? one GitHub webhook? only the local user?)
2. What is the worst thing malicious input could do? (RCE, data exfiltration, forged reviews, wasted compute?)
3. Where is the actual security boundary? Sometimes it is the code (public webhook endpoint). Sometimes it is explicitly NOT the code — e.g. a local agent framework that by design hands an LLM shell/filesystem access: there the boundary is the operator and the model they point it at, and the code can only offer guardrails.
4. What is explicitly out of scope? Write it down so future reviewers don't "fix" non-goals.

Step 2 — Inventory trust boundaries

Grep the codebase for every crossing (see Discovery Commands) and list them:

• Inbound: HTTP/webhook handlers, sockets, file reads of externally-writable paths.
• Deserialization: json.loads, pickle, yaml.load, np.load, JSON.parse on external bytes.
• Injection sinks: SQL string building, subprocess/os.system/exec, template rendering, eval.
• Outbound: any HTTP call whose host/path/params derive from input (SSRF).
• Filesystem: any path constructed from input (traversal).

Step 3 — Harden proportionally (decision tree)

For each boundary crossing from Step 2:
├── Is it inside the written threat model's scope?
│   ├── NO → do not harden it. Record WHY in the threat model
│   │        ("local user is trusted operator"). Over-hardening
│   │        non-boundaries is unpaid complexity.
│   └── YES ↓
├── Is untrusted data AUTHENTICATED before any expensive/dangerous work?
│   └── Order matters: check content-length/size limit → verify
│       signature (constant-time: hmac.compare_digest, never ==)
│       → ONLY THEN parse (json.loads etc.). Parsing before
│       authenticating = attacker-controlled CPU/memory.
├── Is the sink parameterized rather than concatenated?
│   ├── SQL → placeholders (?-params), never f-strings.
│   ├── Shell → avoid shell=True; pass argv lists; timeout everything.
│   ├── Outbound HTTP → FIXED hostname, input only as query PARAM,
│   │   never as any part of the URL host/path.
│   └── Deserialization → safest mode (allow_pickle=False,
│       yaml.safe_load); validate types after parse (isinstance guards).
├── Is load bounded? (size limits, concurrency caps like
│   asyncio.Semaphore, timeouts on shell/network/LLM calls)
└── Is failure behavior decided? (fatal vs recoverable — crash loudly
    on unavailable hard dependency; skip-and-log on per-item timeout)

Step 4 — Guardrail-vs-boundary honesty

Classify every protective measure as one of:

• Boundary: attacker cannot bypass it (HMAC verification, parameterized query, fixed outbound host).
• Guardrail: reduces accidents but a determined party bypasses it (shell-command blocklists, prompt instructions, regex filters on flexible syntax).

Document guardrails AS guardrails, with a known bypass example, in code comments or README. Claiming a guardrail is a boundary is the most dangerous documentation bug in security review. If a real boundary is needed where only a guardrail exists, the fix is isolation (container/VM, separate creds), not a longer blocklist.

Step 5 — Lock hardening in with tests

Every hardening change gets tests in the SAME PR or the immediately following commit:

• invalid/absent signature → rejected (and with what status);
• oversized payload → rejected BEFORE parsing;
• malformed identifiers (repo names, paths) → rejected;
• injection attempt strings → parameterized away, not executed. Untested hardening regresses silently on the next refactor.

Review checklist (run against every boundary)

• Threat model written and committed BEFORE hardening decisions.
• Signature/auth check uses constant-time comparison (hmac.compare_digest, crypto.timingSafeEqual).
• Size/content-length checked before any parse.
• All SQL parameterized; grep found zero query string-building.
• Deserialization in safest mode; allow_pickle=False; type-guards (isinstance) on parsed structures.
• Outbound calls: fixed host; input only in params.
• Timeouts on every shell/network/subprocess/LLM call; concurrency capped.
• Identifiers from input validated against a strict pattern (e.g. repo-name regex); file paths sanitized against traversal.
• Every guardrail labeled as guardrail with its known bypass.
• Tests cover each hardening behavior.
• No secrets in code or logs (cross-check configuration-management).

Discovery Commands

# Inbound boundaries
grep -rn "@app.post\|@app.get\|@router\.\|add_route" --include="*.py" .
grep -rn "app.post\|app.get\|createServer\|listen(" --include="*.ts" --include="*.js" . | grep -v node_modules

# Deserialization sinks
grep -rn "json.loads\|pickle\|yaml.load\|np.load\|fromstring\|JSON.parse" --include="*.py" --include="*.ts" --include="*.js" . | grep -v node_modules
grep -rn "allow_pickle" --include="*.py" .          # want: explicit =False

# Injection sinks
grep -rn "execute(\|executemany(" --include="*.py" . | grep -viE '\?|%s' | head   # candidates for string-built SQL
grep -rniE "f\".*(select|insert|update|delete)" --include="*.py" .
grep -rn "subprocess\|os.system\|shell=True\|child_process\|exec(" --include="*.py" --include="*.js" --include="*.ts" . | grep -v node_modules

# Outbound / SSRF surface
grep -rn "requests.get\|requests.post\|httpx\.\|urlopen\|fetch(" --include="*.py" --include="*.ts" . | grep -v node_modules
# Then inspect each hit: is the HOST a literal, or built from input?

# Auth / signature verification
grep -rn "hmac\|compare_digest\|timingSafeEqual\|X-Hub-Signature" --include="*.py" --include="*.ts" .
grep -rn "== signature\|signature ==" --include="*.py" .   # timing-unsafe compares

# Bounds and timeouts
grep -rn "content-length\|content_length\|Semaphore\|timeout" --include="*.py" .

# Path traversal
grep -rn "os.path.join\|Path(" --include="*.py" . | grep -iE "request|input|param|arg" | head

# History of security work (what was already considered)
git log --oneline | grep -iE "secur|harden|cve|vuln|threat"
ls SECURITY.md tests/test_security* 2>/dev/null

Failure Modes & Anti-patterns

Symptom	Mistake	Correction
Review produces 40 generic findings, none shipped	No threat model; checklist applied blindly	Write the threat model FIRST; harden only in-scope boundaries; record out-of-scope reasoning.
Signature bypass via timing	== comparison on HMAC digests	hmac.compare_digest / crypto.timingSafeEqual, always.
OOM/CPU burn from unauthenticated payloads	Parsing JSON before verifying signature/size	Order: size limit → signature → parse. Never invert.
SQL injection despite "we validate input"	String-built queries with upstream validation as the defense	Parameterize at the sink (?-placeholders); validation is depth, not the boundary.
RCE from a tampered data file	np.load/pickle with defaults on stored blobs	allow_pickle=False; treat your own DB/cache files as untrusted at load.
SSRF via "just a search tool"	Building the outbound URL from model/user input	Fix the host as a literal; pass input only as a query parameter.
Blocklist bypassed trivially ("rm -rf /" — two spaces)	Believing a blocklist is a security boundary	Label it a guardrail with its bypass documented; real isolation = container/VM/least-privilege creds.
Hardening silently removed in a refactor	No tests on security behavior	Every hardening commit is followed (or accompanied) by tests asserting the rejection paths.
Local single-user CLI grows auth/rate-limiting	Hardening a non-boundary	Threat model says the operator is trusted → out of scope; write that down instead of code.

Repository Examples

agentix — the threat model that says "not the code" (as of 2026-07-04)

~/agentix: ~700-line local ReAct agent framework on Ollama. Last commit: "Security review: harden embedding load + document threat model". README threat model (verbatim): agentix "hands an LLM the ability to run shell commands, execute Python, and read/write your filesystem... the security boundary is you and the model you point it at, not the code." Hardening is proportional to exactly that model:

• Parameterized SQL (?-placeholders) throughout.
• np.load(io.BytesIO(blob), allow_pickle=False) with the comment "a tampered DB blob must not be able to execute code on load" — the code treats its own ~/.agentix/memory.db blobs as untrusted at deserialization time.
• Web tool: FIXED host https://api.duckduckgo.com/, query passed only as a URL parameter — SSRF/injection prevented structurally, not by filtering.
• Shell blocklist ["rm -rf /", "sudo", "mkfs"] explicitly documented as a guardrail, NOT a boundary — the README acknowledges "rm -rf /" (two spaces) bypasses it, and advises container/VM isolation for a real boundary. Step 4 honesty, in production.
• Timeouts: shell 30s, python 15s, web 15s; tool exceptions returned as observation strings (fail-visible, not fail-crash).

prism — hardening a real network boundary, then testing it (as of 2026-07-04)

~/prism: FastAPI PR reviewer receiving GitHub webhooks — a genuine internet-facing boundary, so it gets the full treatment: HMAC-SHA256 webhook validation with hmac.compare_digest (constant-time) BEFORE any JSON parsing; content-length plus body-size checks before json.loads; repo-name regex validation; isinstance type-guards on all parsed JSON; asyncio.Semaphore(5) concurrency cap; failure taxonomy split into fatal (OllamaUnavailableError) vs recoverable (timeout-skip); graceful degradation when GITHUB_TOKEN is missing (warn + skip API call). Commit sequence: e264a72 → b7bbabb → 51b92c9 "security: add payload size limit, concurrency cap, repo name validation, Ollama timeout" → b011239 "tests: add security path coverage for signature, repo validation, size limit". The hardening commit was immediately locked in by tests (tests/test_security.py — the repo's only test file, which itself signals proportionality: the security paths are what MUST NOT regress). GitHub API version pinned in headers ("2022-11-28") for interface stability.

Proportionality across the two (as of 2026-07-04)

Same owner, same date, opposite hardening levels — and both correct: prism (internet-facing webhook) gets signatures, size limits, and caps; agentix (local operator-trusted tool) gets injection-proof sinks plus honestly-labeled guardrails, and skips auth entirely. The written threat model is the difference, not diligence.

Validation Criteria

You applied this skill correctly if:

1. A written threat model exists in the repo, dated, answering Step 1's four questions — and it predates the hardening diff.
2. Your boundary inventory lists every inbound/deserialization/injection/outbound/filesystem crossing, each marked in-scope or out-of-scope with a reason.
3. Every in-scope crossing passes the Step 3 checks (ordering, constant-time, parameterization, bounds, failure behavior).
4. Every guardrail is labeled as such WITH a known bypass documented; no guardrail is described as a boundary.
5. Tests exist that fail if any hardening is removed (invalid signature, oversized payload, injection strings).
6. You did NOT add security machinery outside the threat model's scope.

Provenance & Maintenance

• Sources: ~/agentix, ~/prism — investigated 2026-07-04. Both directories were read-restricted during authoring; all quoted facts (threat-model text, allow_pickle=False comment, commit hashes 51b92c9/b011239, semaphore value, timeout values) come from the 2026-07-04 verified fact pack ("verified 2026-07-04, not re-read").
• Assumptions: exact placement of prism's checks within request flow follows the fact pack's ordering description; line-level implementation details not re-read — "(hypothesis — requires verification)" for anything beyond the quoted facts.
• Re-verification commands:

  git -C ~/prism log --oneline
  grep -rn "compare_digest\|content.length\|Semaphore" ~/prism/prism
  ls ~/prism/tests/
  git -C ~/agentix log -1 --oneline
  grep -rn "allow_pickle\|duckduckgo\|rm -rf" ~/agentix --include="*.py"
  grep -A 5 -i "threat" ~/agentix/README.md
  

• Likely to drift: prism's tunables (Semaphore(5), size limits) and pinned GitHub API version; agentix's blocklist contents and timeout values; either repo adding endpoints (new boundaries requiring re-inventory).
• Maintenance checklist: re-run re-verification; re-read both threat models and confirm the quoted verbatim text still matches; re-inventory boundaries after any new handler/tool is added; confirm tests/test_security.py still covers the rejection paths cited here.