jankneumann Security Review
Run a reusable security review workflow across repositories. This skill auto-detects project profile(s), executes compatible scanners, normalizes findings, and applies a deterministic risk gate.
The skill operates in two complementary modes:
- Preventive mode (this section, below) — applied while writing or reviewing code, before scanners are invoked. Catches whole categories of vulnerability through three-tier boundary rules and an OWASP Top 10 checklist.
- Scanner-runner mode (Steps 1–5 below) — applied to a built artifact / running service. Runs OWASP Dependency-Check (SCA) and ZAP (DAST) and applies a deterministic risk gate.
Both modes are mutually reinforcing: the preventive checklist eliminates the easy bugs so the scanners surface the hard ones. Run preventive mode on every change; run scanner-runner mode at integration boundaries.
Preventive Mode
Adopted from the security-and-hardening reference skill. Use this section while authoring or reviewing code that touches authentication, authorization, input validation, secrets, network exposure, or data persistence.
Three-Tier Boundary System
The boundary system replaces ad-hoc judgment with a tiered policy. Map every security-sensitive operation to one of the tiers below.
Tier 1 — Always do (no permission needed)
These are always-correct defaults; deviating from them requires explicit justification in the change description.
- Parameterize every SQL query (use bind variables /
?placeholders). - Validate all external input at the system boundary (HTTP handler, CLI parser, MCP tool call) using a schema library (
pydantic/zod/ JSON Schema), not deep in business logic. - Use list-form
subprocess.run([...])— nevershell=Truewith concatenated user data. - Hash passwords with a slow KDF (bcrypt / argon2 / scrypt). Never store, log, or transmit plaintext passwords.
- Set HTTPS / TLS for all network egress; reject downgrades.
- Prefer the platform's secret manager (env-injected vault token → KV lookup) over reading raw files in source control.
- Apply allow-listing for redirects, file paths, and host headers; reject by default.
- Encode output for the consuming context: HTML escape for browsers, JSON-encode for APIs, log-encode for log lines.
Tier 2 — Ask first (requires explicit human approval)
Operations in this tier are sometimes correct but always risky. Pause and request confirmation before proceeding; document the approval in the PR.
- Disabling CSRF, CORS, or auth on an endpoint (even temporarily for debugging).
- Rolling your own crypto (custom hashing, encryption, signing). Default to a vetted library.
- Logging request/response bodies that may contain secrets, PII, tokens, or credentials.
- Granting write access to production data from a non-production environment.
- Adding a new dependency that handles authentication, sessions, or crypto — verify maintenance status and security advisories first.
- Bypassing rate-limiting or input size limits for a "trusted" client.
- Storing user-supplied data in a path constructed from that data (path traversal risk).
Tier 3 — Never do (no exceptions, no overrides)
These are absolute prohibitions. Treat any code that crosses these lines as broken and refuse to merge.
eval()/exec()/Function()on attacker-controlled or external input.- Hardcoded production secrets, API keys, or private keys in source code or config files.
- Disabling TLS certificate verification in production code (
verify=False,rejectUnauthorized: false). - SQL string concatenation with user input ("just this once").
- Storing passwords with reversible encryption (or any non-KDF hash like MD5/SHA-1).
- Logging raw passwords, MFA codes, session tokens, or credit-card numbers.
- Sending secrets via URL query parameters (they appear in access logs and Referer headers).
- Returning stack traces or framework error pages to unauthenticated users in production.
OWASP Top 10 Prevention Checklist
Apply this checklist to every meaningful change. The deeper, item-level guidance lives in references/security-checklist.md — use that file as the authoritative source when triaging an audit finding.
- A01 — Broken Access Control — Every endpoint enforces authentication AND authorization (role/ownership checks). Default deny; no IDOR by URL guessing; admin endpoints reject non-admin tokens with 403.
- A02 — Cryptographic Failures — TLS for all transport; KDF for passwords; AES-GCM (or equivalent AEAD) for at-rest encryption; no MD5/SHA-1 for security purposes; no hardcoded keys.
- A03 — Injection — Parameterized SQL; list-form subprocess; output encoding for HTML/log/template contexts; no
eval/ dynamic SQL /os.systemon user input. - A04 — Insecure Design — Threat-model the new feature; document the trust boundary; rate-limit by default; design for failure (denial-of-service safety, idempotency).
- A05 — Security Misconfiguration — Production has CSRF + CORS + secure cookies + HSTS + CSP enabled; debug and stack traces disabled; default admin accounts removed; no
.envin source control. - A06 — Vulnerable & Outdated Components — Dependency-check / Dependabot is wired in CI (the scanner-runner mode below covers this); pin major versions; review security advisories before upgrading auth/crypto libs.
- A07 — Identification & Authentication Failures — Slow KDF for passwords; account lockout / rate-limit on login; MFA available for high-privilege accounts; session tokens are random, server-side-revocable, and rotate on privilege change.
- A08 — Software & Data Integrity Failures — Verify checksums / signatures of downloaded artifacts; pin CI action versions to a SHA; no auto-update of code from external sources without review.
- A09 — Security Logging & Monitoring Failures — Log auth events (success, failure, lockout, privilege change) with structured fields; alert on anomalies; never log secrets; preserve logs across the incident-response window.
- A10 — Server-Side Request Forgery (SSRF) — Allow-list outbound destinations; reject internal/loopback/metadata IPs (169.254.169.254, 10.0.0.0/8, 127.0.0.1) for user-driven fetches; resolve DNS once and pin the resolved IP for the request.
For the deeper, language-specific checks behind each item, consult references/security-checklist.md.
Arguments
$ARGUMENTS - Optional flags:
--repo <path>(default: current directory)--out-dir <path>(default:<repo>/docs/security-review)--profile-override <profiles>(for examplepython,node)--fail-on <info|low|medium|high|critical>(default:high)--zap-target <url-or-spec>(required for ZAP execution)--zap-mode <baseline|api|full>(default:baseline)--change <change-id>(optional; writes OpenSpec artifact toopenspec/changes/<id>/security-review-report.md)--openspec-root <path>(optional override when resolving OpenSpec change directory)--bootstrap <auto|never>(default:auto)--apply-bootstrap(execute install commands, otherwise print-only)--allow-degraded-pass(allow pass when scanners are unavailable and no threshold findings exist)--dry-run
Script Location
Scripts live in <agent-skills-dir>/security-review/scripts/. Each agent runtime substitutes <agent-skills-dir> with its config directory:
- Claude:
.claude/skills - Codex:
.codex/skills - Gemini:
.gemini/skills
Installed skill copies are expected to include these scripts. If they are missing, reinstall the skill from its canonical distribution rather than invoking a repo-local path.
Prerequisites
- Python 3.11+
- Optional scanner runtime dependencies:
- Java 17+ for native OWASP Dependency-Check
- Podman Desktop (preferred) with Docker CLI compatibility enabled
- or another Docker-compatible container runtime
- For dependency setup/repair:
<agent-skills-dir>/security-review/scripts/install_deps.sh<agent-skills-dir>/security-review/docs/dependencies.md
Coordinator Integration (Optional)
Use docs/coordination-detection-template.md as the shared detection preamble.
- Detect transport and capability flags at skill start
- Execute hooks only when the matching
CAN_*flag istrue - If coordinator is unavailable, continue with standalone behavior
Steps
0. Detect Coordinator and Run Guardrail Pre-check (Informational)
At skill start, run the coordination detection preamble and set:
COORDINATOR_AVAILABLECOORDINATION_TRANSPORT(mcp|http|none)CAN_LOCK,CAN_QUEUE_WORK,CAN_HANDOFF,CAN_MEMORY,CAN_GUARDRAILS
If CAN_GUARDRAILS=true, run an informational guardrail pre-check before scanners:
- MCP path:
check_guardrails - HTTP path:
"<skill-base-dir>/../coordination-bridge/scripts/coordination_bridge.py"try_check_guardrails(...)
Pre-check target text should summarize intended scan commands and any write actions (report generation paths, temp outputs).
Phase 1 behavior:
- Report violations with category/severity details
- Continue execution without hard-blocking
1. Detect Project Profile
python3 <agent-skills-dir>/security-review/scripts/detect_profile.py --repo <path> --pretty
2. Build Scanner Plan
python3 <agent-skills-dir>/security-review/scripts/build_scan_plan.py \
--repo <path> \
--zap-target <url-or-spec> \
--zap-mode baseline \
--fail-on high \
--pretty
3. Check Prerequisites
<agent-skills-dir>/security-review/scripts/check_prereqs.sh --json
If requirements are missing:
# print-only by default
<agent-skills-dir>/security-review/scripts/install_deps.sh --components java,podman,dependency-check
# execute install commands
<agent-skills-dir>/security-review/scripts/install_deps.sh --apply --components java,podman,dependency-check
4. Run End-to-End Orchestrator
python3 <agent-skills-dir>/security-review/scripts/main.py \
--repo <path> \
--out-dir docs/security-review \
--fail-on high \
--zap-target <url-or-spec> \
--zap-mode baseline \
--change <change-id> \
--bootstrap auto
Outputs (default under <repo>/docs/security-review/):
aggregate.jsongate.jsonsecurity-review-report.jsonsecurity-review-report.md- optional OpenSpec artifact:
openspec/changes/<change-id>/security-review-report.md
Dry-run behavior:
--dry-runstill writes deterministic files underdocs/security-review/.--dry-rundoes not overwriteopenspec/changes/<change-id>/security-review-report.md.
5. Interpret Gate
PASS: no findings at/above threshold and scanner execution acceptable.FAIL: findings met/exceeded threshold.INCONCLUSIVE: scanner execution incomplete (unless degraded pass is explicitly allowed).- For DAST-capable profiles, omitting
--zap-targetmarks ZAP as unavailable and yieldsINCONCLUSIVEunless--allow-degraded-passis set. - A
PASSproduced under--allow-degraded-passis a DEGRADED pass, not a clean one.gate.jsonreasonsthen begins withDEGRADED — <scanner> NOT CHECKED …, naming every scanner that did not run. Propagate it: record the Security phase invalidation-report.mdas- **Status**: DEGRADEDwith that one-line reason, rather thanpass.gate_logic.pyblocks the pre-merge gate on a DEGRADED required phase unless--accept-degraded Securityis passed, and records the override in the gate summary.
Manual Scanner Commands (Optional)
Dependency-Check adapter (native or container fallback):
<agent-skills-dir>/security-review/scripts/run_dependency_check.sh --repo <path> --out <dir>
python3 <agent-skills-dir>/security-review/scripts/parse_dependency_check.py --input <dir>/dependency-check-report.json --pretty
ZAP adapter (container runtime):
<agent-skills-dir>/security-review/scripts/run_zap_scan.sh --target <url-or-spec> --out <dir> --mode baseline
python3 <agent-skills-dir>/security-review/scripts/parse_zap_results.py --input <dir>/zap-report.json --pretty
Quality Checks
python3 -m pytest <agent-skills-dir>/security-review/tests -q
openspec validate add-security-review-skill --strict
Common Rationalizations
| Rationalization | Why it's wrong |
|---|---|
| "Dependency-Check passed, so the change is secure" | SCA only catches known-CVE'd dependencies. It says nothing about your authn/authz logic, your input validation, or the shell injection you wrote yesterday. Run preventive mode AND scanner mode. |
| "I'll skip the OWASP checklist — this PR is small" | The most expensive vulnerabilities ship in small PRs (a one-line verify=False, a f"... {user_input} ..." SQL string). Small PRs are exactly when humans skip review; the checklist exists to compensate. |
| "We don't have a threat model, so I can't tell what tier this falls into" | When in doubt, treat the operation as Tier 2 (ask first). Tier-3 prohibitions (eval, hardcoded secrets, TLS bypass) are absolute and don't require a threat model to recognize. |
| "ZAP scan target isn't deployed yet — I'll skip the scan" | Run preventive mode anyway. ZAP-INCONCLUSIVE is fine for a PR that doesn't add new endpoints; it is not fine when the PR is the new endpoint. Wire ZAP into staging before merge. |
| "The scanner finding is a false positive — I'll suppress it without a note" | Suppressing without a justification (issue link, threat-model reference, compensating-control description) is how real findings get buried. Always document the suppression in the report. |
Red Flags
- A PR that touches authentication, sessions, or crypto without explicit reference to the OWASP A02/A07 checklist items in the description.
- Any new
subprocesscall withshell=Trueor any new SQL string that includesf"..."/+/%formatting with non-literal data — Tier 3 prohibition. verify=False,rejectUnauthorized: false, orInsecureRequestWarningsuppression appearing anywhere outside a clearly-labelled local-test fixture — Tier 3 prohibition.- A new dependency in
package.json/pyproject.toml/Cargo.tomlthat handles auth, crypto, or sessions, without a note in the PR explaining why and confirming it has recent commits and no open critical advisories. gate.jsonreportsINCONCLUSIVEand--allow-degraded-passwas applied without a justification in the PR description.- Logging that emits
request.body,headers["Authorization"],request.cookies, or password-bearing form fields — Tier 2 violation that often slips through review. - A
.envfile, private key, orkubeconfigappearing ingit statusfor the PR.
Verification
- Confirm preventive mode was applied: every Tier 3 prohibition listed above has been searched (e.g.,
rg "shell=True|verify=False|eval\(|exec\(") and zero violations remain in the PR diff. - Confirm the OWASP Top 10 checklist was walked: cite at least one item the PR exercises (e.g., "A03 Injection: all new SQL uses bound parameters at handlers/users.py:42") in the PR description.
- Confirm the scanner-runner mode produced
gate.jsonwithPASS(orINCONCLUSIVEwith--allow-degraded-passAND a written justification). - Confirm
references/security-checklist.mdwas consulted for the relevant item-level checks (cite the section heading) when the change touches an OWASP category. - Confirm any Tier 2 operation in the diff has explicit human approval recorded in the PR (review comment or commit trailer).