security-review

---
name: security-review
description: "OWASP-informed, code-level security review of delivered code: injection, authentication and authorization, data exposure, configuration, dependencies, cryptography and logging, with attack scenarios for Medium+ findings and compliance constraints (CC-XXX) treated as Critical. Use after a delivery, especially one touching input handling, auth, sensitive data or external integrations, such as 'security review TICKET-007' or 'check what this delivery shipped for injection or auth bypass before we merge'. Not for a whole-codebase OWASP posture assessment (owasp-top10-review) or prompt-injection risk in LLM features (owasp-llm-top10-review)."
argument-hint: "[capability area] [TICKET-XXX] [in change-slug] | [files]"
effort: xhigh
---

# Security Review Skill

**Invoked with:** `$ARGUMENTS`

If that is empty, ask the user which delivered ticket (or which files) to review before doing anything else. Never guess the target.

A ticket belongs to a change. When exactly one change is active (`ready` or `in_delivery` in `.gener8v/pipeline-state.yaml`), default to it; when several are active and the argument does not name one (`… in <change-slug>`), ask which change before reading anything.

## Purpose

Perform an OWASP-informed, code-level security review of implemented code. This skill looks for vulnerabilities, misconfigurations, and security anti-patterns in delivered code. It operates at the code level — examining actual implementation for injection vectors, authentication gaps, data exposure, and insecure defaults. When constraints or technical design are available, it cross-references compliance requirements and security architecture decisions.

## When to Use

Use this skill when:
- A ticket has been delivered, especially tickets involving user input, authentication, data handling, external integrations, or configuration
- The system handles sensitive data (PII, credentials, financial data, health records)
- Before deployment or release
- After or in parallel with Code Review and Quality Review
- When compliance constraints (CC-XXX) exist that require security verification
- When the technical design includes authentication, authorization, or data protection decisions

## Input

**Source:** Delivered code files, plus security-relevant pipeline artifacts
**Read from:**
- Delivery record: `.gener8v/changes/<change-slug>/delivery/<area-slug>-ticket-NNN-delivery.md` (for the file list)
- Ticket: `.gener8v/changes/<change-slug>/tickets/<area-slug>/TICKET-NNN.md` (one ticket, one file — for its Constraints and Known Hazards; also where a deferred finding's Known Hazard is appended)
- Actual code files listed in the delivery record's "Files Produced" section
- Constraints: `.gener8v/constraints/prd.md` and `.gener8v/constraints/<area-slug>.md` (whichever exist — compliance constraints CC-XXX at either level apply)
- Conventions: `.gener8v/CONVENTIONS.md`
- Technical Design: `.gener8v/technical-design/<area-slug>.md` or `.gener8v/technical-design/system-design.md` (if available — for auth/authz design decisions)
- System Context: `.gener8v/context.md` (if available — for deployment environment and infrastructure; its `## Repositories` table says which repository each root-relative path lives in when the root is a workspace)

**Expects:** Code files to exist. Does NOT require all pipeline artifacts — the skill adapts its coverage based on what is available.

**If input is missing or malformed:**
- If no delivery record exists, the user can point directly to code files to review
- If constraints are missing, compliance verification is skipped — note "Compliance constraints not available" in the report
- If technical design is missing, architecture-level security checks are limited — note in the report
- If code files do not exist, stop and flag the issue

## Output

**Produces:** A security review report with findings and interactive resolutions
**Write to:** `.gener8v/changes/<change-slug>/reviews/<area-slug>-ticket-NNN-security-review.md`
**Creates directory:** `.gener8v/changes/<change-slug>/reviews/` if it does not exist
**Naming convention:** Matches the delivery record naming with `-security-review` suffix, inside the same change's directory. The top-level `.gener8v/reviews/` holds system-level assessments only — never write a ticket review there.

The report is written as soon as findings are drafted (all `Open`) and updated finding by finding during resolution. Approved remediations are applied in the resolution phase, re-verified, and recorded in the delivery record's `## Post-Review Amendments`. Findings are referenced from other documents as `<change-slug>/<report-slug>/SEC-XXX` — e.g. `support-search/search-and-retrieval-ticket-002-security-review/SEC-001` (numbering restarts per report; see `CONVENTIONS.md` §4).

## Output Format

Write the report in the shape of `assets/security-review-report.md` — Read it before writing and follow it exactly, headings and status lines included. `gener8v-state.py` parses its `**Result:**` line for the verdict and counts its `### SEC-NNN` headings and `**Severity:**` lines for the metrics, so none of the three may be renamed or reformatted.

---

## Principles

### Severity Drives Priority
Use OWASP-aligned severity levels:
- **Critical**: Actively exploitable with high impact. Remote code execution, authentication bypass, SQL injection with data access. Must be remediated before deployment.
- **High**: Exploitable with moderate effort or significant impact. Privilege escalation, stored XSS, insecure direct object references. Should be remediated before deployment.
- **Medium**: Exploitable under specific conditions or with moderate impact. Reflected XSS, missing rate limiting, verbose error messages exposing internals. Should be addressed.
- **Low**: Minor concern or defense-in-depth gap. Missing security headers, overly permissive CORS in non-sensitive contexts. Address when practical.
- **Informational**: Best practice recommendation. Security improvement opportunity with no immediate risk.

Critical and High findings block approval unless the user explicitly accepts the risk with documented rationale.

### Attack Scenarios Are Required
Every Medium-severity-or-higher finding must include a plausible attack scenario: who is the attacker, what access do they have, what steps do they take, what do they achieve? This distinguishes real vulnerabilities from theoretical concerns. A finding without an attack scenario is an assertion, not evidence.

### Defense in Depth, Not Perfection
Security is layered. A missing validation at one layer is less severe if another layer catches it. Assess findings in the context of the full stack, not in isolation. A SQL injection vector behind an authentication wall and input sanitization middleware is lower severity than one in an unauthenticated public endpoint.

### Accepted Risk Is a Valid Outcome
Not every security finding must be fixed. Some are accepted risks: the likelihood is low, the mitigation cost is high, or compensating controls exist. The review records risk acceptance decisions explicitly with rationale. Critical findings require strong justification for acceptance — document why the risk is tolerable and what compensating controls exist. Acceptance is the Security role's decision (`CONVENTIONS.md` §7): every accepted-risk finding carries `**Risk accepted by:** Security — <name>, YYYY-MM-DD` next to its rationale, even when one person holds every role.

### Compliance Constraints Are Non-Negotiable
If the constraints analysis identifies compliance requirements (CC-XXX), violations of those constraints are automatically elevated to Critical severity regardless of exploitability. Compliance is not risk-based — it is requirement-based. A CC-XXX violation means the system does not meet its stated compliance obligations.

### Secrets in Code Are Always Critical
Hardcoded credentials, API keys, tokens, private keys, or connection strings in source code are Critical findings. No exceptions for "dev environments," "temporary values," or "will be changed later." If it is in the code and the code is committed, it is a secret exposure. The remediation is always: remove the secret, rotate it, use environment variables or a secrets manager.

### Dependencies Are Attack Surface
Third-party dependencies with known CVEs are findings. The review should check for outdated dependencies with known vulnerabilities where tooling makes this feasible (e.g., `npm audit`, `pip-audit`, `cargo audit`). The severity matches the CVE severity.

### Two Phases, Two Runtimes
**Findings** (steps 1–12) can run in a fresh context — the shipped `security-reviewer` agent, in parallel with the other two reviewers — because a reviewer who did not build the code has no reason to trust the builder's account of it. The findings phase writes the report with every finding `Status: Open` and a provisional verdict, and changes nothing else. **Resolution** (steps 13–15) runs in the main session with the user, one review at a time so three reviewers never edit the same file concurrently. Every approved change is re-verified and appended to the delivery record's `## Post-Review Amendments`; the report is updated per finding as it is resolved, and the final verdict is written last.

### Log Sensitive Data Never
Logging that includes PII, credentials, session tokens, full request/response bodies with sensitive fields, or stack traces with internal paths in production is a finding. Good logging is essential for security monitoring — but logging sensitive data creates a new exposure vector.

## Process

1. **Locate Code**: Read the delivery record to get the list of files produced. If no delivery record exists, use file paths provided directly by the user.

2. **Read All Code**: Read every delivered code file thoroughly.

3. **Read Security Context**: Read the ticket file (for its Constraints and any Known Hazards already recorded), constraints (for CC-XXX compliance requirements), technical design (for auth/authz patterns and security-related architecture decisions), and system context (for deployment environment).

4. **Check Input Validation**: Examine all entry points — function parameters from external input, API endpoints, form handlers, file uploads, URL parameters, headers. Check for:
   - Missing validation on user-controlled input
   - SQL injection vectors (string concatenation in queries)
   - NoSQL injection vectors
   - Command injection (shell commands with user input)
   - Template injection
   - XSS vectors (unescaped output of user input)
   - Path traversal (user input in file paths)

5. **Check Authentication & Authorization**: Verify auth patterns:
   - Are authentication checks present on protected endpoints?
   - Are authorization checks granular (not just "is logged in" but "has permission")?
   - Are sessions handled securely (expiration, invalidation, secure flags)?
   - Are passwords hashed with appropriate algorithms (bcrypt, argon2, scrypt)?
   - Are tokens validated properly (signature, expiration, audience)?

6. **Check Data Protection**: Look for sensitive data exposure:
   - PII in logs, error messages, or API responses
   - Credentials or secrets in source code, configuration files, or comments
   - Sensitive data in URLs (query parameters are logged by servers and proxies)
   - Missing encryption for sensitive data at rest or in transit
   - Overly broad data exposure in API responses (returning full objects when subsets suffice)

7. **Check Configuration Security**:
   - Hardcoded secrets, API keys, connection strings
   - Insecure defaults (debug mode enabled, verbose errors, open CORS)
   - Missing security headers (CSP, X-Frame-Options, HSTS)
   - Overly permissive file/directory permissions
   - Default credentials or accounts

8. **Check Dependencies**: Where tooling is available, check for known vulnerable dependencies. Note the tool used and findings.

9. **Check Cryptography**: If the code uses cryptographic operations:
   - Are algorithms current and appropriate (not MD5, SHA1 for security purposes)?
   - Is key management handled properly (not hardcoded)?
   - Are random numbers generated with cryptographically secure functions?

10. **Check Logging & Monitoring**: Verify that security-relevant events are logged (authentication attempts, authorization failures, input validation rejections) and that sensitive data is excluded from logs.

11. **Cross-Reference Compliance**: For each CC-XXX constraint from the constraints analysis, verify the code meets the requirement. Flag violations at Critical severity.

12. **Draft Findings and Write the Report**: Create findings with severity, OWASP reference, category, attack scenario (for Medium+), and specific remediation guidance. Write the full report to `.gener8v/changes/<change-slug>/reviews/` now, every finding `Open`, verdict provisional. *(End of the findings phase — when run as the `security-reviewer` agent, stop here and return the report path, verdict and counts.)*

13. **Present to User**: Share findings starting with Critical, then High, then Medium, then Low, then Informational. For Critical and High findings, emphasize the attack scenario and impact. Work through interactive resolution — the user may fix, defer, or accept risk — updating each finding in the report as it is decided. A finding deferred to a named ticket (`Deferred → TICKET-NNN`) also gets a Known Hazard appended to that ticket's own file — `.gener8v/changes/<change-slug>/tickets/<area-slug>/TICKET-NNN.md` — so the implementer sees it (`CONVENTIONS.md` §2). When a risk is accepted, write the `**Risk accepted by:** Security — <name>, YYYY-MM-DD` line and the rationale on the finding at that moment, and mark the Resolution Log row `Risk Accepted: Yes`.

14. **Apply Approved Remediations**: Update code files for findings the user approves, re-run the delivery record's Verification Run, and append each change to the delivery record's `## Post-Review Amendments`.

15. **Write the Verdict**: Set the final `**Result:**` and `**Accepted Risks:**` last.

## Example

A worked example — the findings-phase report for Search & Retrieval TICKET-002 (semantic index) in change `support-search`, with one Medium and one Informational finding, followed by the resolution excerpt that records an accepted risk — is in `references/example.md`. Read it before producing your first security review report.

---

## Troubleshooting

- **Orchestrate still lists the security review as missing after the report was written.** The state script opens exactly `.gener8v/changes/<change-slug>/reviews/<area-slug>-ticket-nnn-security-review.md` — area slug, lowercase `ticket-nnn`, inside the ticket's change. The top-level `.gener8v/reviews/` holds system assessments and is never searched for a ticket's review. Move or rename the file; never leave two copies.
- **No dependency audit tool can run** — `npm audit`, `pip-audit` or `cargo audit` is not installed, or there is no lockfile. Say so under the dependency check with the tool that was tried. Silence reads as a clean audit, and Orchestrate dates the next security re-check from the last dependency audit a review recorded.
- **A finding's severity turns on a control outside the workspace** — a gateway that authenticates, a WAF, network policy in another repository. Do not assume it exists. Rate from what the code shows, name the assumed control in the attack scenario, and let resolution record it: a control confirmed by the user becomes an accepted risk with its `**Risk accepted by:**` line, not a silently lowered severity.
- **The user wants a Critical or High accepted before resolution.** Acceptance belongs to the Security role at step 13, written on the finding with its rationale. The findings phase — and the `security-reviewer` agent — leaves every finding `Open`, however obvious the acceptance looks.
- **The `security-reviewer` agent returned without a report path.** The findings phase ends at step 12 by writing the report; an agent that stops at its turn limit returns partial output instead. Resume or re-launch it on the same ticket, or run steps 1–12 in the main session, before starting step 13.

## Integration with Other Skills

**Upstream:**
- **Ticket Breakdown Skill**: Provides the ticket file (`changes/<change-slug>/tickets/<area-slug>/TICKET-NNN.md`) — the review reads its Constraints and Known Hazards, and appends a Known Hazard to it when a finding is deferred there
- **Delivery Skill**: Provides the delivery record (`changes/<change-slug>/delivery/…`, with the file list) and the delivered code to review
- **Constraints Skill**: Provides compliance constraints (CC-XXX) for mandatory security requirements
- **Technical Design Skill**: Provides security-related architecture decisions (auth patterns, data protection approach)

**Downstream:**
- **Audit Skill**: Can include security review findings and risk acceptances in cross-stage assessments
- **OWASP Top 10 / OWASP LLM Top 10 / Architecture Review**: read every `changes/*/reviews/*-security-review.md` (and legacy `reviews/*-security-review.md`) and map its findings (qualified as `<change-slug>/<report-slug>/SEC-XXX`) onto their taxonomies
- **Delivery Skill** (later tickets): reads this report from `changes/<change-slug>/reviews/` for deferred findings and accepted risks on files it will touch
- **Orchestrate**: reads the `**Result:**` line; `Changes Required` holds the ticket at `changes_required`

**Parallel:**
- **Code Review Skill**: Reviews the same code for pipeline traceability — different concern, can run in parallel
- **Quality Review Skill**: Reviews the same code for engineering quality — different concern, can run in parallel

## Revisions

- Security review reports capture a point-in-time assessment — they do not auto-update when code changes or new CVEs are published
- After code modifications from other review findings, consider re-running the security review if the changes affect security-relevant code
- If new compliance constraints (CC-XXX) are added, re-run the security review to verify the code meets the updated requirements
- Dependency vulnerability checks become stale quickly — re-run periodically or when dependencies are updated

## Notes

- This skill reviews code, not architecture — for design-level security analysis (threat modeling, trust boundaries), use the Constraints skill with a security focus or create a dedicated threat model
- The findings phase runs after Delivery, in parallel with Code Review and Quality Review (as the reviewer agents); resolution phases run one at a time
- The OWASP Top 10:2025 is the primary reference framework (the same edition the OWASP Top 10 Review skill assesses against), but findings are not limited to it
- Verdict vocabulary is shared by all three reviews: Approved / Approved with Notes / Changes Required; accepted risks are counted separately
- Risk acceptance is a first-class outcome — the report explicitly records what risks were accepted, why, and who accepted them (`**Risk accepted by:** Security — …`)
- For dependency vulnerability checks, note which tool was used and when — results are time-sensitive
- If no security-relevant code is found (pure data transformation, formatting, etc.), the review can be brief with an "Approved" verdict and a note explaining the limited attack surface
- This skill can be used without a delivery record by pointing directly at code files — it degrades gracefully