security-review-deep

name

security-review-deep

description

Scanner-grounded security audit, distinct from the built-in /security-review: runs the right scanners (semgrep, osv-scanner, trivy, gitleaks, bandit), queries live CVEs via MCP, walks a 24-category threat-model checklist, and produces a triaged report. Trigger on "deep security review", "full security audit", "check for CVEs", "scan my dependencies", or proactively after changes touching auth, crypto, deserialization, or dependency upgrades. Routine file/network I/O does NOT trigger; offer it in a sentence instead.

Security Review

A disciplined, repeatable process for catching vulnerabilities in a code change. Designed to make any model — large or small — review like a senior security engineer by combining deterministic scanners, live advisory data, and a structured threat-model walkthrough.

When to use this skill

• After a commit, in a pre-push hook, or in CI
• When reviewing a PR or diff
• When auditing a dependency upgrade
• When the user asks "is this safe?" or anything in the same neighborhood
• Proactively after the user finishes work on genuinely sensitive surface area: auth, crypto, deserialization or parsing of untrusted input, dependency upgrades

Nearly all code touches files or the network; that alone is not a trigger — this is a full multi-scanner audit, not a lint. If the work merely brushes sensitive territory, offer the review in one sentence and let the user decide.

The process

Always work in this order. Do not skip to LLM analysis without running the scanners first — that's how you end up hallucinating CVE numbers and missing real issues.

Step 1 — Scope the change

Determine what to review:

# For a post-commit / pre-push review:
git diff HEAD~1 HEAD --stat
git diff HEAD~1 HEAD

# For a PR review (against main):
git diff origin/main...HEAD --stat
git diff origin/main...HEAD

# For an audit of the whole repo:
# scope is everything; note this and warn the user it will be slower

Capture:

• list of changed files

• any dependency manifests touched (package.json, package-lock.json, requirements.txt, pyproject.toml, Cargo.toml, go.mod, pom.xml, Gemfile.lock, etc.)

• whether the change touches any sensitive surface (see checklist below)

• languages and frameworks present in the diff. This decides which Step 2 scanners run. Read the file extensions and a few imports, then list them explicitly, e.g.:

  languages: Python (Flask), Go, TypeScript (React), shell, Dockerfile
  iac:       Terraform, GitHub Actions
  

  Languages absent from the diff skip their Step 2 scanner; record "ran": false, "reason": "language not in diff" in the run summary. This is what stops the model from blindly invoking gosec on a Python-only change.

Step 2 — Run the deterministic scanners

Run all available scanners on the scoped change. Don't try to be the scanner — use them. Route by file type: only run a scanner if its target language / surface appears in the diff (or, for whole-repo audits, the repo).

Always-on (regardless of language)

WORKDIR="${TMPDIR:-/tmp}/security-review-$$"
mkdir -p "$WORKDIR"

# Wrap every scanner in a 300s budget so one hung tool can't block the
# whole review. Falls back to running bare if no `timeout` is on PATH
# (on macOS install GNU coreutils: `brew install coreutils`).
TIMEOUT=$(command -v timeout || command -v gtimeout || true)
TO() { [[ -n "$TIMEOUT" ]] && "$TIMEOUT" 300 "$@" || "$@"; }
# Convention: a scanner that hit the budget exits 124; record
# {"ran": true, "timed_out": true} for that tool in the run summary
# rather than dropping the layer silently.

# SAST — pattern-based vuln detection, multi-language
TO semgrep --config=p/default --config=p/security-audit \
        --config=p/owasp-top-ten --config=p/cwe-top-25 \
        --json --quiet <changed-files-or-dir> > "$WORKDIR/semgrep.json"

# Dependency CVEs (Google's OSV aggregator)
TO osv-scanner scan source -r --format=json --output="$WORKDIR/osv.json" .

# Filesystem / IaC / container vulns; also includes lockfile scanning
TO trivy fs --severity HIGH,CRITICAL --format=json --output="$WORKDIR/trivy.json" .

# Secrets — fast regex layer
TO gitleaks detect --no-banner --report-format=json --report-path="$WORKDIR/gitleaks.json"

# Secrets — live-credential verification (different findings than gitleaks; run both)
TO trufflehog filesystem . --json --no-update > "$WORKDIR/trufflehog.json"

The per-language and CI/IaC scanner blocks live in references/scanner-catalog.md, along with install commands, the scheduled CodeQL setup, and the tested-version table. Load the catalog once the diff's languages are known, run only the blocks whose surface appears in the diff, and wrap each invocation in the same TO helper.

Gitleaks catches secret-shaped strings via regex; TruffleHog verifies whether a detected secret is live against 20+ providers. The two find different things; run both and union the results.

Language-specific and CI / IaC scanners

Gated by what's in the diff: the per-language commands (bandit, gosec, njsscan, brakeman, shellcheck, spotbugs, cargo-*, flawfinder / cppcheck / clang-tidy, security-scan, psalm) and the CI / build / infra commands (zizmor, hadolint, checkov, kube-score, syft + grype) are in references/scanner-catalog.md.

If any tool is missing, note it and continue with the rest. Record the absence in the run-summary JSON (Step 6) as "ran": false, "reason": "not installed". Do not refuse to proceed.

Read each JSON output and build a unified findings list with: file, line, rule, severity, message, raw tool source.

Cross-cutting passes

In addition to the file-scoped scanners, run these on every review:

# Secrets in history, not just the current diff. Gitleaks/trufflehog
# default to "what's in the working tree;" attacker access to git
# objects exposes everything previously committed. Look back at least
# 30 days, more if the repo's young.
trufflehog git "file://$(pwd)" --since-commit="HEAD~200" \
  --json --no-update > "$WORKDIR/trufflehog-history.json"

# Regression check: if a previous review exists for this repo, diff
# the new findings against the old. Any finding that is NEW (was not
# present in the previous report) is a regression and earns a tag in
# the report. Findings that existed before are still flagged but
# de-emphasized.
PREV=".security-reviews/last.json"
[[ -f "$PREV" ]] && comm -13 \
    <(jq -r '.findings[].id' "$PREV" | sort) \
    <(jq -r '.findings[].id' "$WORKDIR/report.json" 2>/dev/null | sort) \
    > "$WORKDIR/regressions.txt" || true

# Test coverage on changed security-relevant lines. Run the project's
# coverage tool (pytest --cov / jest --coverage / cargo tarpaulin) and
# cross-reference the uncovered lines against the diff. A new auth
# check or input validator with no test is a real risk.

For each of these, omit gracefully if the tool isn't installed or the artifact doesn't exist; record in the run summary.

Heavier passes for a schedule (CodeQL)

CodeQL (database-backed, interprocedural taint tracking) is too slow for the per-PR path: minutes to 30+ minutes of build time. Run it scheduled, ingest it. The GitHub Actions workflow YAML and rationale live in references/scanner-catalog.md. If .security-reviews/codeql-latest.sarif (or wherever the artifact lands) exists, read it in Step 2 alongside the per-PR scanners and tag those findings source: codeql, age: <days-since-job> so the reader knows they're from the nightly pass, not the live diff.

Step 3 — Pull live advisory context (MCP first, scanners as fallback)

If the security-advisories MCP server is registered, use it before any other dependency-scanning approach. It's faster, doesn't need a venv, and returns structured data Claude can cite without paraphrasing.

Order of preference for dependency CVE lookups:

1. MCP for everything in this order per finding:
   • check_dependencies_bulk for a whole lockfile, check_package for one bumped dep.
   • composite_risk(cve_id) for any CVE that surfaces. This rolls CVSS (NVD) + EPSS (FIRST.org) + KEV membership (CISA) into a single 0-100 score and triage bucket. Use this to rank findings instead of re-deriving the math in prose.
   • kev_lookup(cve_id) if you only need the "actively exploited?" bit.
   • epss_score(cve_id) if you only need the exploit-probability bit.
   • latest_safe_version(ecosystem, name, current_version) for an upgrade target. Returns each advisory's fix versions and a best-effort smallest version that clears all of them. Cite this verbatim in the "Suggested fix" block.
   • ghsa_get(ghsa_id) / lookup_cve(cve_id) for full advisory text.
   • package_health(ecosystem, name) for maintenance signals (last release date, total releases, maintainer count, archived / yanked status). Run this on any new dependency the diff adds — abandonware and typosquats are senior-reviewer concerns OSV won't surface. PyPI + npm supported natively.
2. osv-scanner if the MCP isn't registered.
3. Hand-rolled OSV.dev API calls (curl, Python urllib) only as a last resort if both above fail.

Do NOT fall back to "recall the CVE from memory" — that's how CVE IDs get hallucinated. If the tools aren't available, say so explicitly in the report.

NVD enrichment has been unreliable since its analysis backlog began in 2024, so lookup_cve results may be sparse. Treat OSV + GHSA + KEV as the primary trio and NVD as supplementary.

Reachability gate (apply to every dep CVE before flagging)

Most OSV findings are NOT reachable from your code. Industry estimates put unreachable CVEs at ~90% of raw output. Before listing a dep CVE in the report, do one of:

1. Direct check: grep the codebase for the vulnerable function/class named in the advisory's details. If it's not imported or called, mark reachable: no and demote to the "Dependency hygiene" section instead of "Critical / High."
2. Tool check if the language supports it:
   • Go: govulncheck ./... (reports reachability against Go's vuln DB)
   • Python: pip-audit --strict (limited reachability heuristics)
   • Rust: cargo-audit + manual import check
3. Explicit unknown: mark reachable: unknown and explain why you couldn't verify (vulnerable symbol not named in advisory, deep transitive dep, etc.).

Every dep finding in the report must carry a reachable: yes/no/unknown tag. Don't silently drop unreachable findings; surface them as "known vulnerable version present but no call-graph evidence of use."

Step 4 — Apply the threat-model checklist to the diff

This is where the LLM adds value beyond scanners. For every changed file, walk the checklist below and write down what you find. Do not just say "looks good" — name the specific function, line, and reason for each item you clear or flag.

Read references/threat-checklist.md for the full list. The top categories:

1. Trust boundaries — does this code cross one (user → server, low-priv → high-priv, network → process)? What data flows across?
2. Input validation — every external input typed, length-checked, content-validated before use
3. Injection sinks — SQL, command, LDAP, XPath, template, log, header, NoSQL, ORM-raw
4. AuthN / AuthZ — is every new endpoint/handler protected? Is the check before the side effect? Are object-level permissions enforced (IDOR)?
5. Secrets / credentials — hardcoded, logged, sent to telemetry, embedded in URLs, committed to repo?
6. Crypto — algorithm choice, key length, IV/nonce reuse, MAC-then-encrypt errors, custom crypto (red flag)
7. Deserialization — pickle, yaml.load (not safe_load), Java/PHP unserialize, anything that turns bytes into objects
8. SSRF / file inclusion — URL or path constructed from user input and then fetched/opened
9. Race conditions / TOCTOU — check-then-use patterns on files, tokens, balances
10. Error handling — stack traces leaked, exceptions swallowed silently, fail-open patterns
11. Logging & PII — secrets, tokens, full PII in logs; unbounded log growth
12. Dependencies — covered by Step 3, but also: typosquats, abandoned packages, suspicious new maintainers
13. Supply chain — install/build scripts, postinstall hooks, dynamic code loading
14. Concurrency — shared mutable state, locking
15. Resource exhaustion — unbounded loops, allocations, recursion, regex (ReDoS)
16. Web-app cross-cutting — CSRF, CORS, open redirect, security headers, cookie flags, cache poisoning, host header injection
17. Protocol-specific — GraphQL (introspection, depth, complexity, batching), WebSocket (origin, auth on connect), gRPC, webhook signature verification + replay protection
18. Authorization matrix — for every new endpoint write a one-line caller / side_effect / lesser_role_result row and verify the answer. IDOR lives here; scanners do not catch it.
19. Business logic — payment / coupon / refund abuse, workflow step skipping, race-on-balance, signed-message replay, state-machine bypass, mass assignment
20. LLM / AI agent / MCP — direct + indirect prompt injection, insecure output handling, excessive agency, system-prompt leakage, RAG poisoning, unbounded consumption; MCP-specific tool-arg validation, no shell from args, SSRF in fetch tools, auth on connect
21. Negative space — for each addition, ask what wasn't added: route missing from auth allowlist, PII field missing from log scrubber, weakened control in the deletion side of the diff, new background job using elevated creds
22. AI-generated code red flags — if the diff looks AI-shaped, do an extra pass on input handling, error paths, and duplicate helpers; studies consistently find AI-generated code carries substantially higher vuln density than human-written code. Also walk the "looks done but isn't" sub-list: validators that don't validate, tests that test nothing, dead branches, half-refactored identifiers, TODO/FIXME left in production paths, feature flags that don't kill, retry loops with no upper bound.
23. Memory safety and unsafe constructs — only when Rust / C / C++ / Go is in the diff. Rust: unsafe blocks need // SAFETY: justification; no .unwrap() / .expect() / panic!() / todo!() on reachable paths; integer overflow handling explicit. C/C++: bounded buffer ops, no format-string holes, UAF / double-free / uninitialized memory, integer overflow on size arithmetic. Go: errors checked at every call site, context.Context propagated, no goroutine leaks, no defer in loops.
24. Context-specific surfaces (when relevant) — Mobile (cert pinning, deep-link validation, exported components, Keychain/Keystore, WebView hardening). Cloud IAM (no *:*, OIDC trust-policy wildcards, AssumeRole chains, KMS key policies, VPC SGs, audit-log integrity). Skip the sub-block that doesn't apply.

For each item: ✅ cleared (with brief reason), ⚠️ flagged (with file:line + explanation), or N/A.

Step 5 — Triage and prioritize

You will likely have more findings than the user wants to act on. Rank by exploitability × impact, not by scanner severity alone:

• Critical — exploitable now, no auth required, leads to RCE / data exfil / auth bypass
• High — exploitable with some condition (auth'd user, specific input), or undermines a security boundary
• Medium — defense-in-depth issues, hardening gaps, theoretical-but-real
• Low — informational, style, "should fix eventually"
• Noise — scanner false positives. Explicitly mark these and explain why it's a false positive — don't just drop them.

Triage rules baked into the workflow

For every CVE that appears (in OSV output, dep findings, or anywhere else), call composite_risk(cve_id) and apply these rules:

1. CISA KEV → Critical, regardless of CVSS. KEV membership means the bug is being actively weaponized in the wild; that beats any abstract severity score.
2. EPSS >= 0.5 → minimum High. FIRST.org's exploit-prediction model is 0.0-1.0 for "exploited in the next 30 days." Half-likely is more dangerous than most CVSS-8 issues that nobody has weaponized.
3. CVSS alone → bucket by score × 10 (so CVSS 9.0 → 90, "Critical").
4. Reachability beats severity. A CVSS 9.8 in an unreachable transitive dep is Medium hygiene at best; a CVSS 6.5 in a hot code path on a user-facing route is High.

The composite_score returned by composite_risk already encodes rules 1-3. Use it as the default order within each bucket.

Step 6 — Output the report

Use the format in references/report-template.md. Key principles:

• Lead with what's actually exploitable. Don't bury a CRITICAL under 40 LOW findings.
• Every finding gets a fix. Suggested patch, not just "be more careful."
• Cite the source for every claim — which scanner, which CVE, which line of code.
• Distinguish what you ran from what you didn't. "Semgrep + OSV + Trivy on the diff. Did not run dynamic analysis or fuzzing."
• No padding. No "in conclusion, security is important." End when the work ends.

Required at the top of the report (banner):

• Counts: KEV findings, EPSS >= 0.5 findings, other.
• For example: 3 KEV · 7 EPSS≥0.5 · 41 other.

Required at the bottom of the report (run summary):

{
  "scanners": {
    "semgrep":     {"ran": true, "files": 87, "findings": 12, "took_ms": 4200},
    "osv-scanner": {"ran": true, "deps": 312, "findings": 5, "took_ms": 1800},
    "trivy":       {"ran": true, "findings": 0,  "took_ms": 6100},
    "gitleaks":    {"ran": true, "findings": 0,  "took_ms": 400},
    "bandit":      {"ran": true, "files": 14, "findings": 1, "took_ms": 900}
  },
  "mcp": {
    "calls": 23,
    "kev_hits": 3,
    "epss_high_hits": 7
  }
}

A sloppy run and a thorough run should look obviously different. If a scanner was unavailable, set "ran": false and "reason": "...".

Every dependency finding must carry a reachable: yes/no/unknown tag (per Step 3's reachability gate).

Every finding (code OR dep) carries a Confidence tag:

• High — confirmed by a deterministic scanner rule, demonstrated exploit, or direct call-graph evidence
• Medium — pattern match or heuristic; verification recommended before acting
• Low — speculative; flag for awareness, may be a false positive

If a finding is Low confidence it generally belongs in "Medium / hardening" or below, not "Critical / High." See references/report-template.md for the full convention.

If the caller asks for CI-friendly output (or passes --sarif / --json), also emit a sidecar alongside the markdown report:

• report.sarif (SARIF 2.1.0) — for GitHub Advanced Security, Azure DevOps, generic SCA tools, and the built-in /code-review --comment machinery
• report.json — flat finding list with {file, line, rule, severity, composite, reachable, confidence, source, suggested_fix, message, kev, epss, category} and an optional top-level summary string for the banner

The markdown report is for humans; the sidecar is for tools. Both must reference the same findings; do not silently drop entries when serializing.

Posting findings as PR review comments

scripts/post-findings.sh reads report.json and posts one batched GitHub review, each finding rendered as an inline comment at its file:line. Severity threshold and max-comments configurable; defaults are --min-severity medium --max-comments 50. Usage, the sidecar contract, and a sample CI step live in scripts/README.md. Requires gh (logged in) and jq.

If everything passes, the report can be three lines. If it doesn't, it can be ten pages. Length follows findings, never the other way around.

Step 7 — Capture project-specific patterns for next time

Before signing off, ask the question that closes the learning loop:

Did this review surface a pattern specific to this codebase that should become a deterministic check next time?

Common candidates:

• A project-internal helper that's unsafe by construction (e.g. an in-house run_command that always passes shell=True) — write a semgrep rule that flags any call.
• A wrapper that LOOKS like a safe helper but isn't (e.g. db.execute_safe(...) that string-formats internally) — write a rule that flags calls.
• A field name that should always be redacted in logs (e.g. customer.ssn, token.value) — write a rule that flags any log statement referencing it.
• A convention the team agreed on but only humans enforce (e.g. "money goes through Money.from_decimal, never float") — write a rule that flags float(...) near monetary identifiers.

If you find one, draft a semgrep rule and append it to .semgrep/project-rules.yml (create the file if absent). Then add --config=.semgrep/project-rules.yml to the Step 2 semgrep invocation so the next review picks it up automatically.

Minimum useful rule template:

rules:
  - id: project.unsafe-run-command
    message: |
      Internal `run_command(...)` always uses `shell=True`. Use
      `subprocess.run([...], shell=False)` directly, or rework the
      helper to take a list.
    severity: WARNING
    languages: [python]
    pattern: run_command(...)

Include the new rule in the report's "Suggested next steps" so the user knows what was added. One rule per review is plenty — chasing more than that is bikeshedding.

Failure modes to actively avoid

• Hallucinating CVE numbers. Only cite CVEs returned by lookup_cve or present in scanner output. Never reconstruct from memory.
• "It looks fine." If you haven't run the scanners or walked the checklist, you don't know that.
• Over-trusting a single scanner. Semgrep misses things OSV catches and vice versa. Run multiple.
• Treating scanner severity as final. A CRITICAL Semgrep finding on dead code is less important than a MEDIUM one in a live auth path. Re-rank based on context.
• Burying lede. Top of the report is the worst finding, always.
• Dropping findings without explanation. If you decide a finding is a false positive, say so and say why. Never silently omit.
• Never cite a CVE ID that didn't come directly from a tool output. Every CVE-XXXX-NNNN in the report must be traceable to a specific tool invocation (MCP lookup_cve, OSV-Scanner JSON, GHSA advisory result). If you find yourself typing a CVE ID from memory or inference, stop — call lookup_cve to verify it exists and matches your claim, or remove it.

Lightweight mode

If the user wants something fast (pre-commit hook, sub-30-second turnaround):

1. Run only semgrep --config=p/security-audit and gitleaks on the diff. Add zizmor if any .github/workflows/*.yml is touched and hadolint if any Dockerfile* is touched — both are fast and catch a class of issues nothing else does.
2. Skip the MCP live-CVE pass unless a dependency manifest was touched
3. Apply only the top 5 checklist categories (1, 2, 3, 4, 5)
4. Output a compact one-screen report

Tell the user this is the lightweight pass and a fuller review can be run separately.

When you're stuck

If a scanner errors out, capture the error verbatim, note which scanner, and continue with the others. The review is still valuable even if one tool is broken.

If the diff is enormous (>2000 lines changed), tell the user up front: full review will take time, and offer to either (a) batch by directory, or (b) focus on the most sensitive files first based on filename heuristics (anything matching auth, login, password, crypto, admin, payment, upload, parse, exec, *.sql, etc.).

Tested versions

The scanner versions table lives in references/scanner-catalog.md (update it there when a known-good upgrade lands, not on every release).