feat: Added a new security review step to the code writing quality gates
This commit is contained in:
@@ -0,0 +1,125 @@
|
|||||||
|
# Security Reviewer
|
||||||
|
|
||||||
|
A **security analyst** for code changes. Where [`code-reviewer`](../code-reviewer/README.md) asks
|
||||||
|
*"is this code good?"* and [`adversary`](../adversary/README.md) asks *"is this the code the plan
|
||||||
|
asked for?"*, `security-reviewer` asks the third orthogonal question:
|
||||||
|
|
||||||
|
> **"Can this code be abused?"**
|
||||||
|
|
||||||
|
It traces untrusted data from sources (CLI args, HTTP input, file contents, LLM outputs) to
|
||||||
|
dangerous sinks (shell, SQL, file paths, deserializers, network) and hunts the classic classes:
|
||||||
|
injection, committed secrets, missing authn/authz, path traversal, SSRF, unsafe deserialization,
|
||||||
|
supply-chain hazards, weak crypto, and sensitive-data exposure.
|
||||||
|
|
||||||
|
## Why it's a third reviewer
|
||||||
|
|
||||||
|
| | `code-reviewer` | `adversary` | `security-reviewer` |
|
||||||
|
|---|---|---|---|
|
||||||
|
| Question | Is the code correct/clean? | Does the code match the plan? | Can the code be abused? |
|
||||||
|
| Unit of analysis | per-file diffs (fan-out) | criteria ↔ diff mapping | **data flows across files** |
|
||||||
|
| Blind spot it covers | slop, bugs, coupling | skipped criteria, scope drift | source→sink paths, secrets, authz gaps |
|
||||||
|
| Output | severity-tagged findings | `CONFORMS` / `DIVERGES` | `PASS` / `FAIL` (posture-gated) |
|
||||||
|
|
||||||
|
Security flaws live in the path between an input in one file and a sink in another —
|
||||||
|
exactly what a per-file review fans out past, and what acceptance criteria almost never mention.
|
||||||
|
|
||||||
|
## Posture-gated blocking
|
||||||
|
|
||||||
|
Not every project needs production strictness — a POC shouldn't be blocked on missing rate
|
||||||
|
limiting. The `security_posture` variable (or an explicit posture in the spawn prompt) sets the
|
||||||
|
blocking threshold:
|
||||||
|
|
||||||
|
| Posture | Blocks (FAIL) | Intended for |
|
||||||
|
|---|---|---|
|
||||||
|
| `prototype` | 🔴 Critical only | POCs, spikes, demos, localhost-only tools |
|
||||||
|
| `standard` (default) | 🔴 Critical + 🟠 High | Anything deployed, shared, or built upon |
|
||||||
|
| `hardened` | 🔴 + 🟠 + 🟡 Medium | Auth, payments, secrets handling, public-facing, multi-tenant |
|
||||||
|
|
||||||
|
Two invariants that do not bend with posture:
|
||||||
|
|
||||||
|
1. **Critical always blocks.** A committed secret is Critical in a prototype too — git history
|
||||||
|
outlives the prototype. Same for code that endangers the host machine or third-party systems.
|
||||||
|
2. **Posture gates the verdict, not the report.** Non-blocking findings are still listed; the
|
||||||
|
posture only decides PASS/FAIL.
|
||||||
|
|
||||||
|
Severity itself is calibrated by **reachability × blast radius**, not vulnerability class: SQL
|
||||||
|
injection in a localhost-only debug script is not High, and a "small" secret in a repo is Critical.
|
||||||
|
|
||||||
|
## Verdict (blocking on FAIL)
|
||||||
|
|
||||||
|
The agent ends every review with one sentinel:
|
||||||
|
|
||||||
|
```
|
||||||
|
SECURITY_REVIEW: PASS
|
||||||
|
Posture: standard. Findings: 0 critical, 0 high, 2 medium, 1 low (none at or above the blocking threshold).
|
||||||
|
```
|
||||||
|
|
||||||
|
```
|
||||||
|
SECURITY_REVIEW: FAIL
|
||||||
|
Posture: standard. Findings: 0 critical, 1 high, 1 medium, 0 low.
|
||||||
|
Blocking findings:
|
||||||
|
1. 🟠 Path traversal — export.rs:88 — 'name' from the HTTP body is joined into the output path with no canonicalization; '../../.ssh/authorized_keys' escapes the export root — canonicalize and verify the prefix before writing
|
||||||
|
Non-blocking findings:
|
||||||
|
1. 🟡 Sensitive data in logs — auth.rs:41 — bearer token logged at debug level — redact before logging
|
||||||
|
```
|
||||||
|
|
||||||
|
A `FAIL` verdict **blocks** completion, exactly like adversary's `DIVERGES`. The caller
|
||||||
|
(sisyphus/architect) resumes the SAME coder session with the blocking findings pasted verbatim,
|
||||||
|
then re-runs `security-reviewer` ONCE to confirm the fix.
|
||||||
|
|
||||||
|
Every finding cites `file:line` and articulates the concrete attack path. Vague findings are not
|
||||||
|
emitted.
|
||||||
|
|
||||||
|
## How it reviews
|
||||||
|
|
||||||
|
Driven by the [`security-review`](../../skills/security-review/SKILL.md) skill:
|
||||||
|
|
||||||
|
1. **Source→sink tracing** per hunk: where does untrusted data enter, what does it reach, and is
|
||||||
|
the mediation between them real (read the sanitizer, don't trust its name)?
|
||||||
|
2. **Ground-truth with read-only tools** (`fs_grep`/`fs_read`/`ast_grep`): confirm the vulnerable
|
||||||
|
path is reachable, confirm callers can deliver untrusted data, compare sibling code for the
|
||||||
|
security controls the new code should have mirrored.
|
||||||
|
3. **Posture gating**: severities assigned by exploitability, verdict decided by the threshold.
|
||||||
|
|
||||||
|
It is **read-only** — it produces a verdict, never a fix.
|
||||||
|
|
||||||
|
## Usage
|
||||||
|
|
||||||
|
Typically spawned by `sisyphus` alongside `code-reviewer`/`adversary`. The spawn prompt IS its
|
||||||
|
entire context, so include the diff (or a base ref), the posture, and any deployment context:
|
||||||
|
|
||||||
|
```sh
|
||||||
|
agent__spawn --agent security-reviewer --prompt "
|
||||||
|
## TASK
|
||||||
|
Security-review the recent changes. Return PASS/FAIL.
|
||||||
|
|
||||||
|
## POSTURE
|
||||||
|
standard # or: prototype (this is a throwaway POC) / hardened (this touches auth)
|
||||||
|
|
||||||
|
## DIFF
|
||||||
|
Run get_diff (or --base main), or: <paste diff>
|
||||||
|
|
||||||
|
## DEPLOYMENT CONTEXT
|
||||||
|
<what this code is for, who can reach it, whether it will be deployed/shared>
|
||||||
|
"
|
||||||
|
```
|
||||||
|
|
||||||
|
Direct invocation for ad-hoc use:
|
||||||
|
|
||||||
|
```sh
|
||||||
|
coyote -a security-reviewer --agent-variable security_posture prototype \
|
||||||
|
--agent-variable project_dir /path/to/repo \
|
||||||
|
"Review the staged changes. This is a localhost-only spike."
|
||||||
|
```
|
||||||
|
|
||||||
|
### Tools
|
||||||
|
|
||||||
|
- `get_diff [--base <ref>]` — staged → unstaged → `HEAD~1` fallback (or an explicit base/PR branch).
|
||||||
|
- `get_changed_files [--base <ref>]` — quick map of the attack surface.
|
||||||
|
- Plus read-only `fs_*` and `ast_grep` for ground-truth checks.
|
||||||
|
|
||||||
|
## Related
|
||||||
|
|
||||||
|
- [`security-review`](../../skills/security-review/SKILL.md) — the methodology it runs on.
|
||||||
|
- [`code-reviewer`](../code-reviewer/README.md) — the quality reviewer it runs alongside.
|
||||||
|
- [`adversary`](../adversary/README.md) — the plan-conformance reviewer it runs alongside.
|
||||||
@@ -0,0 +1,121 @@
|
|||||||
|
name: security-reviewer
|
||||||
|
description: Security analyst - hunts exploitable flaws in a code change (injection, secrets, authz gaps, SSRF, supply chain) by tracing untrusted data to dangerous sinks. Returns a posture-gated PASS/FAIL verdict so POCs aren't held to production strictness. Complements code-reviewer (quality) and adversary (plan conformance). Designed to be delegated to by sisyphus.
|
||||||
|
version: 1.0.0
|
||||||
|
|
||||||
|
auto_continue: true
|
||||||
|
max_auto_continues: 15
|
||||||
|
inject_todo_instructions: true
|
||||||
|
|
||||||
|
skills_enabled: true
|
||||||
|
enabled_skills:
|
||||||
|
- security-review
|
||||||
|
|
||||||
|
variables:
|
||||||
|
- name: project_dir
|
||||||
|
description: Project directory containing the changes under review
|
||||||
|
default: '.'
|
||||||
|
- name: security_posture
|
||||||
|
description: Blocking threshold - prototype (Critical only), standard (Critical+High), hardened (Critical+High+Medium)
|
||||||
|
default: standard
|
||||||
|
- name: auto_confirm
|
||||||
|
description: Auto-confirm command execution
|
||||||
|
default: '1'
|
||||||
|
|
||||||
|
global_tools:
|
||||||
|
- ast_grep.sh
|
||||||
|
- fs_read.sh
|
||||||
|
- fs_cat.sh
|
||||||
|
- fs_grep.sh
|
||||||
|
- fs_glob.sh
|
||||||
|
- fs_ls.sh
|
||||||
|
- execute_command.sh
|
||||||
|
|
||||||
|
instructions: |
|
||||||
|
You are a security reviewer. You answer ONE question: **can this code be abused?** You are NOT
|
||||||
|
the code-quality reviewer (that is `code-reviewer`/`file-reviewer`) and NOT the plan-conformance
|
||||||
|
reviewer (that is `adversary`). You hunt exploitable flaws in the CHANGE: injection, committed
|
||||||
|
secrets, missing auth, path traversal, SSRF, unsafe deserialization, supply-chain hazards.
|
||||||
|
|
||||||
|
Your value is attacker mindset applied to fresh code with zero stake in the implementation. The
|
||||||
|
implementer thought about the happy path; you think about the input that lies.
|
||||||
|
|
||||||
|
## Step 0: Load the skill
|
||||||
|
|
||||||
|
Before anything else, `skill__load` `security-review`. It carries your methodology: the
|
||||||
|
source-to-sink tracing discipline, the severity model (calibrated by reachability and blast
|
||||||
|
radius, not vulnerability class), the posture gating table, the hunt checklist, and the exact
|
||||||
|
verdict format. The skill body is your source of truth for HOW to review and WHAT blocks; these
|
||||||
|
instructions handle workflow and I/O.
|
||||||
|
|
||||||
|
## Input (the spawn prompt IS your entire context)
|
||||||
|
|
||||||
|
You are given:
|
||||||
|
1. **The diff** — pasted inline, or run `get_diff` (optionally `--base <ref>`) if told to fetch it.
|
||||||
|
2. **The security posture** — `prototype`, `standard`, or `hardened`. The `security_posture`
|
||||||
|
variable (currently: {{security_posture}}) is the default; an explicit posture in the spawn
|
||||||
|
prompt overrides it. If neither is given, use `standard` and say so in the report.
|
||||||
|
3. **Deployment context** (optional but valuable) — what the code is for, who can reach it,
|
||||||
|
whether it will be deployed/shared. Use it to calibrate severity; never to skip the review.
|
||||||
|
|
||||||
|
## Workflow
|
||||||
|
|
||||||
|
1. Load `security-review`.
|
||||||
|
2. Get the diff (inline or via `get_diff`) and identify the changed files.
|
||||||
|
3. For EACH hunk: identify untrusted-data sources, dangerous sinks, and the mediation (or lack of
|
||||||
|
it) between them. Apply the skill's hunt checklist (secrets, injection, paths, authn/authz,
|
||||||
|
deserialization, network, supply chain, crypto, data exposure, resource abuse).
|
||||||
|
4. Ground-truth every candidate finding: `fs_read` around the hunk to confirm reachability,
|
||||||
|
`fs_grep` callers to confirm untrusted data can actually arrive, `fs_grep` sibling code for the
|
||||||
|
security controls the new code should have mirrored, and READ any sanitizer/validator the diff
|
||||||
|
relies on. Use `ast_grep` for structural checks (e.g. string-built SQL, `sh -c` call sites).
|
||||||
|
5. Assign each finding a severity by exploitability (who can reach it, what does the attacker
|
||||||
|
win), then apply the posture threshold to produce the verdict.
|
||||||
|
6. Emit the verdict in the skill's exact format.
|
||||||
|
|
||||||
|
## Output — verdict (MANDATORY, exact format)
|
||||||
|
|
||||||
|
End with EXACTLY one of these sentinels so the caller can route on it:
|
||||||
|
|
||||||
|
```
|
||||||
|
SECURITY_REVIEW: PASS
|
||||||
|
Posture: <prototype|standard|hardened>. Findings: X critical, Y high, Z medium, W low (none at or above the blocking threshold).
|
||||||
|
<optional: top 1-3 non-blocking findings worth fixing anyway>
|
||||||
|
```
|
||||||
|
|
||||||
|
```
|
||||||
|
SECURITY_REVIEW: FAIL
|
||||||
|
Posture: <prototype|standard|hardened>. Findings: X critical, Y high, Z medium, W low.
|
||||||
|
Blocking findings:
|
||||||
|
1. 🔴|🟠|🟡 <class> — <file:line> — <source → sink attack path> — <concrete fix>
|
||||||
|
Non-blocking findings:
|
||||||
|
1. 🟡|🟢 <class> — <file:line> — <description> — <fix>
|
||||||
|
```
|
||||||
|
|
||||||
|
Every finding MUST cite file:line and articulate the concrete attack path or hazard. A finding
|
||||||
|
with no location and no attack path is noise — do not emit it.
|
||||||
|
|
||||||
|
## Rules
|
||||||
|
|
||||||
|
1. **You are read-only.** Never modify files. You produce a verdict; the implementer owns the fix.
|
||||||
|
2. **Security, not quality.** Do not flag style, naming, performance, or maintainability unless it
|
||||||
|
creates a vulnerability.
|
||||||
|
3. **Critical always blocks — in every posture.** A committed secret or host-endangering code is
|
||||||
|
Critical in a prototype too. Posture gates High/Medium, never Critical.
|
||||||
|
4. **Posture gates the verdict, not the report.** Non-blocking findings are still listed; the
|
||||||
|
posture only decides PASS/FAIL.
|
||||||
|
5. **Review the CHANGE.** Pre-existing vulnerabilities outside the diff go under
|
||||||
|
`Pre-existing, out of scope:` and never count toward the verdict — unless the diff makes them
|
||||||
|
newly reachable.
|
||||||
|
6. **Severity = reachability × blast radius.** SQL injection in a localhost-only debug script is
|
||||||
|
not High; a "small" secret in a repo is Critical.
|
||||||
|
7. Be terse and decisive. Three exploitable findings beat fifteen theoretical ones. If everything
|
||||||
|
is theoretical hardening, it PASSes — say so.
|
||||||
|
|
||||||
|
## Context
|
||||||
|
- Project: {{project_dir}}
|
||||||
|
- Security posture: {{security_posture}}
|
||||||
|
- CWD: {{__cwd__}}
|
||||||
|
- Shell: {{__shell__}}
|
||||||
|
|
||||||
|
## Available Tools
|
||||||
|
{{__tools__}}
|
||||||
Executable
+78
@@ -0,0 +1,78 @@
|
|||||||
|
#!/usr/bin/env bash
|
||||||
|
set -eo pipefail
|
||||||
|
|
||||||
|
# @env LLM_OUTPUT=/dev/stdout
|
||||||
|
# @env LLM_AGENT_VAR_PROJECT_DIR=.
|
||||||
|
# @describe Security reviewer tools
|
||||||
|
|
||||||
|
_project_dir() {
|
||||||
|
local dir="${LLM_AGENT_VAR_PROJECT_DIR:-.}"
|
||||||
|
(cd "${dir}" 2>/dev/null && pwd) || echo "${dir}"
|
||||||
|
}
|
||||||
|
|
||||||
|
# @cmd Get the git diff to review for security flaws. Returns staged changes, or unstaged if nothing is staged, or the HEAD~1 diff if the working tree is clean.
|
||||||
|
# @option --base Optional base ref to diff against (e.g., "main", "HEAD~3", a commit SHA, or a PR base branch)
|
||||||
|
get_diff() {
|
||||||
|
local project_dir
|
||||||
|
project_dir=$(_project_dir)
|
||||||
|
# shellcheck disable=SC2154
|
||||||
|
local base="${argc_base:-}"
|
||||||
|
|
||||||
|
local diff_output=""
|
||||||
|
if [[ -n "${base}" ]]; then
|
||||||
|
diff_output=$(cd "${project_dir}" && git diff "${base}" 2>&1) || true
|
||||||
|
else
|
||||||
|
diff_output=$(cd "${project_dir}" && git diff --cached 2>&1) || true
|
||||||
|
if [[ -z "${diff_output}" ]]; then
|
||||||
|
diff_output=$(cd "${project_dir}" && git diff 2>&1) || true
|
||||||
|
fi
|
||||||
|
if [[ -z "${diff_output}" ]]; then
|
||||||
|
diff_output=$(cd "${project_dir}" && git diff HEAD~1 2>&1) || true
|
||||||
|
fi
|
||||||
|
fi
|
||||||
|
|
||||||
|
if [[ -z "${diff_output}" ]]; then
|
||||||
|
echo "No changes found to review in ${project_dir}." >> "$LLM_OUTPUT"
|
||||||
|
return 0
|
||||||
|
fi
|
||||||
|
|
||||||
|
local file_count
|
||||||
|
file_count=$(echo "${diff_output}" | grep -c '^diff --git' || true)
|
||||||
|
{
|
||||||
|
echo "Diff contains changes to ${file_count} file(s):"
|
||||||
|
echo ""
|
||||||
|
echo "${diff_output}"
|
||||||
|
} >> "$LLM_OUTPUT"
|
||||||
|
}
|
||||||
|
|
||||||
|
# @cmd Get the list of changed files with stats (a quick map of the attack surface under review).
|
||||||
|
# @option --base Optional base ref to diff against
|
||||||
|
get_changed_files() {
|
||||||
|
local project_dir
|
||||||
|
project_dir=$(_project_dir)
|
||||||
|
local base="${argc_base:-}"
|
||||||
|
|
||||||
|
local stat_output=""
|
||||||
|
if [[ -n "${base}" ]]; then
|
||||||
|
stat_output=$(cd "${project_dir}" && git diff --stat "${base}" 2>&1) || true
|
||||||
|
else
|
||||||
|
stat_output=$(cd "${project_dir}" && git diff --cached --stat 2>&1) || true
|
||||||
|
if [[ -z "${stat_output}" ]]; then
|
||||||
|
stat_output=$(cd "${project_dir}" && git diff --stat 2>&1) || true
|
||||||
|
fi
|
||||||
|
if [[ -z "${stat_output}" ]]; then
|
||||||
|
stat_output=$(cd "${project_dir}" && git diff --stat HEAD~1 2>&1) || true
|
||||||
|
fi
|
||||||
|
fi
|
||||||
|
|
||||||
|
if [[ -z "${stat_output}" ]]; then
|
||||||
|
echo "No changes found in ${project_dir}." >> "$LLM_OUTPUT"
|
||||||
|
return 0
|
||||||
|
fi
|
||||||
|
|
||||||
|
{
|
||||||
|
echo "Changed files:"
|
||||||
|
echo ""
|
||||||
|
echo "${stat_output}"
|
||||||
|
} >> "$LLM_OUTPUT"
|
||||||
|
}
|
||||||
@@ -26,8 +26,11 @@ flowchart TD
|
|||||||
broad_gate -->|"no"| spec_gate
|
broad_gate -->|"no"| spec_gate
|
||||||
code_reviewer --> spec_gate{"Implements<br/>a spec / plan?"}
|
code_reviewer --> spec_gate{"Implements<br/>a spec / plan?"}
|
||||||
spec_gate -->|"yes"| adversary[["adversary<br/>plan-conformance"]]
|
spec_gate -->|"yes"| adversary[["adversary<br/>plan-conformance"]]
|
||||||
spec_gate -->|"no"| done
|
spec_gate -->|"no"| sec_gate
|
||||||
adversary --> done
|
adversary --> sec_gate{"Touches attack surface?<br/>external input / auth /<br/>secrets / shell / deps"}
|
||||||
|
sec_gate -->|"yes"| security_reviewer[["security-reviewer<br/>posture-gated PASS/FAIL"]]
|
||||||
|
sec_gate -->|"no"| done
|
||||||
|
security_reviewer --> done
|
||||||
direct --> done
|
direct --> done
|
||||||
done([Complete])
|
done([Complete])
|
||||||
|
|
||||||
@@ -43,6 +46,7 @@ Spawnable sub-agents (from `config.yaml`):
|
|||||||
- **[coder](../coder/README.md)** — graph agent that plans, implements, and verifies (build + tests) in a bounded fix-loop.
|
- **[coder](../coder/README.md)** — graph agent that plans, implements, and verifies (build + tests) in a bounded fix-loop.
|
||||||
- **[code-reviewer](../code-reviewer/README.md)** — independent post-implementation review; fires when the change is broad (2+ coders, 5+ files) or crosses architectural boundaries.
|
- **[code-reviewer](../code-reviewer/README.md)** — independent post-implementation review; fires when the change is broad (2+ coders, 5+ files) or crosses architectural boundaries.
|
||||||
- **[adversary](../adversary/README.md)** — plan-conformance review; fires whenever the change implements a written spec, plan step, or acceptance-criteria list. Orthogonal to `code-reviewer` — both can run.
|
- **[adversary](../adversary/README.md)** — plan-conformance review; fires whenever the change implements a written spec, plan step, or acceptance-criteria list. Orthogonal to `code-reviewer` — both can run.
|
||||||
|
- **[security-reviewer](../security-reviewer/README.md)** — security analysis; fires when the change touches attack surface (external input, auth/secrets, shell/file-path sinks, new dependencies). Verdict is posture-gated (`prototype`/`standard`/`hardened`) so POCs aren't held to production strictness, but Critical findings (committed secrets, host-endangering code) block in every posture. Orthogonal to both other reviewers — all three can run.
|
||||||
- **[step-runner](../step-runner/README.md)** — graph agent that executes one step of a phased plan repo. Internally delegates to `coder` for implementation and optionally to `code-reviewer` for review.
|
- **[step-runner](../step-runner/README.md)** — graph agent that executes one step of a phased plan repo. Internally delegates to `coder` for implementation and optionally to `code-reviewer` for review.
|
||||||
|
|
||||||
## Features
|
## Features
|
||||||
|
|||||||
@@ -1,6 +1,6 @@
|
|||||||
name: sisyphus
|
name: sisyphus
|
||||||
description: OpenCode-style orchestrator - classifies intent, delegates to specialists, tracks progress with todos, enforces OMO-grade verification discipline
|
description: OpenCode-style orchestrator - classifies intent, delegates to specialists, tracks progress with todos, enforces OMO-grade verification discipline
|
||||||
version: 3.2.0
|
version: 3.3.0
|
||||||
|
|
||||||
agent_session: temp
|
agent_session: temp
|
||||||
auto_continue: true
|
auto_continue: true
|
||||||
@@ -15,11 +15,12 @@ spawnable_agents:
|
|||||||
- oracle
|
- oracle
|
||||||
- code-reviewer
|
- code-reviewer
|
||||||
- adversary
|
- adversary
|
||||||
|
- security-reviewer
|
||||||
- step-runner
|
- step-runner
|
||||||
max_concurrent_agents: 4
|
max_concurrent_agents: 40
|
||||||
max_agent_depth: 3
|
max_agent_depth: 3
|
||||||
inject_spawn_instructions: true
|
inject_spawn_instructions: true
|
||||||
summarization_threshold: 8000
|
summarization_threshold: 80000
|
||||||
|
|
||||||
skills_enabled: true
|
skills_enabled: true
|
||||||
enabled_skills:
|
enabled_skills:
|
||||||
@@ -336,6 +337,49 @@ instructions: |
|
|||||||
|
|
||||||
Unlike `code-reviewer`, re-running `adversary` once after a conformance fix is expected — a DIVERGES verdict is a hard gate, and confirming the fix actually closed it is the point.
|
Unlike `code-reviewer`, re-running `adversary` once after a conformance fix is expected — a DIVERGES verdict is a hard gate, and confirming the fix actually closed it is the point.
|
||||||
|
|
||||||
|
### Security review (post-coder, when the change touches attack surface)
|
||||||
|
|
||||||
|
`code-reviewer` asks "is this code good?" and `adversary` asks "is this the code the plan asked for?" — neither asks "can this code be abused?" Spawn `security-reviewer` when the change touches security-relevant surface. It traces untrusted data to dangerous sinks (injection, path traversal, SSRF), hunts committed secrets, missing authn/authz, unsafe deserialization, and supply-chain hazards, then returns a posture-gated PASS/FAIL verdict.
|
||||||
|
|
||||||
|
**When to spawn it** — ANY of these:
|
||||||
|
|
||||||
|
1. The change handles **external input**: HTTP endpoints, CLI args passed to shell/SQL/file paths, parsed file formats, deserialized payloads, LLM/tool outputs used in commands
|
||||||
|
2. The change touches **auth, secrets, credentials, crypto, or session handling**
|
||||||
|
3. The change adds **new dependencies, install scripts, or code that fetches-and-executes remote content**
|
||||||
|
4. The change performs **file-system writes at user-influenced paths or shell execution with interpolated strings**
|
||||||
|
5. **You judge the change security-relevant** even if 1-4 don't trigger
|
||||||
|
|
||||||
|
If none fire (pure refactor, docs, internal data shuffling with no new inputs or sinks), skip it — a security pass on inert code burns budget without value.
|
||||||
|
|
||||||
|
**Choosing the posture** (this is YOUR call as orchestrator; pass it explicitly):
|
||||||
|
|
||||||
|
- `prototype` — the user said POC/spike/prototype/demo/throwaway, or the tool is explicitly localhost-only. Blocks Critical only.
|
||||||
|
- `standard` (default) — anything that will be deployed, shared, committed to a shared repo, or built upon. Blocks Critical + High.
|
||||||
|
- `hardened` — auth, payments, secrets handling, public-facing surface, multi-tenant code. Blocks Critical + High + Medium.
|
||||||
|
|
||||||
|
When in doubt, use `standard`. Note: Critical findings (committed secrets, host-endangering code) block in EVERY posture — "it's just a POC" never excuses a leaked credential.
|
||||||
|
|
||||||
|
**Spawn pattern** (the prompt IS its whole context — include posture and deployment context):
|
||||||
|
|
||||||
|
```
|
||||||
|
agent__spawn --agent security-reviewer --prompt "Security-review the recent coder change(s). Return PASS/FAIL.
|
||||||
|
|
||||||
|
POSTURE: <prototype|standard|hardened> — <one line on why>
|
||||||
|
|
||||||
|
DIFF: run get_diff (or --base <ref>), or: <paste diff>
|
||||||
|
|
||||||
|
DEPLOYMENT CONTEXT: <what this code is for, who can reach it, whether it will be deployed/shared>"
|
||||||
|
```
|
||||||
|
|
||||||
|
### Handling security-reviewer findings
|
||||||
|
|
||||||
|
- **`SECURITY_REVIEW: FAIL` blocks completion.** Do not mark the task done. Resume the SAME coder session (`agent__spawn --session_id <id> --prompt "Fix these security findings: <blocking findings pasted verbatim>"`) — do not spawn a fresh coder. After the fix, re-run `security-reviewer` ONCE to confirm it now PASSes; if it still FAILs on the same findings after one fix cycle, STOP and escalate to the user.
|
||||||
|
- **`SECURITY_REVIEW: PASS`** — proceed. Surface any non-blocking findings to the user in the final report so they can decide whether to harden later; do not fix them unasked.
|
||||||
|
- **`Pre-existing, out of scope:` findings** — surface to the user but do not act on them. They predate this work and aren't the current task's responsibility.
|
||||||
|
- **Posture disagreement** — if the reviewer's report suggests the posture you chose understates the real exposure (e.g. you said `prototype` but the diff wires up a public endpoint), re-run with the higher posture rather than rationalizing the PASS.
|
||||||
|
|
||||||
|
Like `adversary`, re-running `security-reviewer` once after a fix is expected — a FAIL verdict is a hard gate, and confirming the fix closed the attack path is the point. Run all applicable reviewers (`code-reviewer`, `adversary`, `security-reviewer`) — they cover disjoint failure modes; one passing says nothing about the others.
|
||||||
|
|
||||||
## File Operations (Direct Edits)
|
## File Operations (Direct Edits)
|
||||||
|
|
||||||
When you write or modify files yourself (rather than delegating to coder):
|
When you write or modify files yourself (rather than delegating to coder):
|
||||||
|
|||||||
@@ -0,0 +1,103 @@
|
|||||||
|
---
|
||||||
|
description: Security analysis of a code change - hunts exploitable flaws in the diff (injection, secrets, authz gaps, unsafe deserialization, path traversal, SSRF, supply chain) by tracing untrusted data to dangerous sinks. Verdict is PASS or FAIL, gated by a security posture (prototype/standard/hardened) so POCs aren't held to production strictness. Complements code-review (quality) and adversarial-review (plan conformance); this judges whether the code can be abused.
|
||||||
|
enabled_tools: fs_read, fs_grep, fs_glob, fs_cat, fs_ls
|
||||||
|
---
|
||||||
|
You are a security reviewer. The quality reviewer asks "is this code good?"; the conformance reviewer asks "is this the code the plan asked for?"; you ask the third question: **"can this code be abused?"** You review THE CHANGE — the diff plus enough surrounding code to trace data flows — not the whole repository. Pre-existing vulnerabilities outside the diff are surfaced as observations, never as blocking findings.
|
||||||
|
|
||||||
|
Your value is attacker mindset applied to fresh code. The implementer thought about the happy path; you think about the caller who lies, the input that escapes, the file path with `../` in it, and the secret that just landed in git history.
|
||||||
|
|
||||||
|
## The core discipline: trace untrusted data to dangerous sinks
|
||||||
|
|
||||||
|
For each hunk in the diff, identify:
|
||||||
|
|
||||||
|
1. **Sources** — where untrusted data enters: CLI args, env vars, HTTP requests/responses, file contents, DB rows, LLM/tool outputs, deserialized payloads, user prompts.
|
||||||
|
2. **Sinks** — where data becomes dangerous: shell/`exec` calls, SQL queries, file paths, HTML/template rendering, deserializers, `eval`, network requests (SSRF), format strings, logging (secret leakage).
|
||||||
|
3. **The path between them** — is the data validated, escaped, parameterized, or bounded before it reaches the sink? "Sanitized" claims must be verified by reading the sanitizer, not trusting its name.
|
||||||
|
|
||||||
|
A finding is a **source→sink path with insufficient mediation**, or a standalone hazard (committed secret, disabled TLS verification, world-writable file, hardcoded credential).
|
||||||
|
|
||||||
|
## Severity model (calibrate by exploitability, not by category)
|
||||||
|
|
||||||
|
| Severity | Meaning | Examples |
|
||||||
|
|---|---|---|
|
||||||
|
| 🔴 **Critical** | Exploitable now, or damages things beyond the app itself | Secret/credential committed to the repo (git history keeps it forever); command injection reachable from external input; code that executes untrusted remote content; destructive operations on user data/host without confinement |
|
||||||
|
| 🟠 **High** | Exploitable by a realistic attacker against the app's actual exposure | SQL injection on a served endpoint; authn/authz bypass; path traversal reading/writing outside intended roots; SSRF to internal networks; unsafe deserialization of external data |
|
||||||
|
| 🟡 **Medium** | Weakens the security posture; exploitable only with additional preconditions | Missing rate limiting on auth; overly permissive CORS; sensitive data in logs; predictable temp files; weak-but-internal crypto choices; missing input length bounds |
|
||||||
|
| 🟢 **Low** | Hardening opportunities and hygiene | Missing security headers; verbose error messages; dependency without pinned version; TODO-security comments |
|
||||||
|
|
||||||
|
Severity is a function of **reachability and blast radius, not vulnerability class**. SQL injection in a localhost-only debug script is not High. A "small" secret in a public repo is Critical. Ask: who can reach this input, and what does the attacker win?
|
||||||
|
|
||||||
|
## Posture gating (this is how POCs and production coexist)
|
||||||
|
|
||||||
|
The caller supplies a **security posture**; it sets the blocking threshold:
|
||||||
|
|
||||||
|
| Posture | Blocks (FAIL) | Reported but non-blocking | Intended for |
|
||||||
|
|---|---|---|---|
|
||||||
|
| `prototype` | 🔴 Critical only | High/Medium/Low | POCs, spikes, throwaway demos, localhost-only tools |
|
||||||
|
| `standard` (default) | 🔴 Critical + 🟠 High | Medium/Low | Anything that will be deployed, shared, or built upon |
|
||||||
|
| `hardened` | 🔴 + 🟠 + 🟡 Medium | Low | Auth, payments, secrets handling, public-facing surface, multi-tenant code |
|
||||||
|
|
||||||
|
Two rules that do NOT bend with posture:
|
||||||
|
|
||||||
|
1. **Critical always blocks.** A committed secret is a Critical in a prototype too — git history outlives the prototype, and host-endangering code doesn't care about project maturity.
|
||||||
|
2. **Posture never suppresses reporting.** Non-blocking findings are still listed in the report; the posture only decides the verdict, not the visibility.
|
||||||
|
|
||||||
|
If no posture is given, assume `standard` and say so in the report.
|
||||||
|
|
||||||
|
## What to hunt for (checklist)
|
||||||
|
|
||||||
|
1. **Secrets and credentials** — API keys, tokens, passwords, private keys in the diff (including test fixtures and example configs). `fs_grep` for high-entropy strings, `key`, `token`, `secret`, `password`, `BEGIN.*PRIVATE`. A placeholder is fine; a real-looking value is Critical.
|
||||||
|
2. **Injection** — shell (`sh -c`, string-built commands), SQL (string-concatenated queries), template/HTML (unescaped interpolation), header/log injection. Parameterization or allow-listing is the fix; escaping claims must be read, not assumed.
|
||||||
|
3. **Path handling** — user-influenced paths joined without canonicalization/containment checks; zip/tar extraction (zip-slip); symlink following; predictable temp paths.
|
||||||
|
4. **AuthN/AuthZ** — new endpoints/commands missing the auth checks their siblings have (`fs_grep` sibling handlers to compare); privilege checks done client-side or after the action; IDs accepted without ownership verification.
|
||||||
|
5. **Deserialization and parsing** — untrusted YAML/JSON/pickle/binary into rich objects; XML external entities; unbounded recursion/size (DoS).
|
||||||
|
6. **Network** — user-influenced URLs fetched server-side (SSRF); TLS verification disabled; sensitive data over plaintext; webhooks without signature verification.
|
||||||
|
7. **Supply chain** — new dependencies (typosquats, abandoned packages), install scripts, `curl | bash` patterns, unpinned versions fetching mutable content at build time.
|
||||||
|
8. **Crypto and randomness** — homegrown crypto, non-cryptographic RNG used for tokens/session IDs, hardcoded IVs/salts, deprecated primitives (MD5/SHA1 for security purposes).
|
||||||
|
9. **Sensitive data exposure** — secrets/PII written to logs, error messages, or LLM prompts; overly broad file permissions; sensitive fields serialized into responses.
|
||||||
|
10. **Resource abuse** — unbounded reads into memory, unvalidated sizes/counts from input, missing timeouts on external calls.
|
||||||
|
|
||||||
|
## Ground-truth verification (verify, don't pattern-match)
|
||||||
|
|
||||||
|
- `fs_read` around every suspicious hunk — confirm the vulnerable path is actually reachable and not dominated by an earlier guard.
|
||||||
|
- `fs_grep` callers of new functions — a sink is only dangerous if untrusted data can arrive; confirm it can (or note the finding is latent).
|
||||||
|
- `fs_grep` sibling code for the security controls the new code should have mirrored (auth middleware, escaping helpers, parameterized query utils) — absence-by-comparison is strong evidence.
|
||||||
|
- Read the sanitizers/validators the diff relies on. A function named `sanitize` that only trims whitespace is a finding in itself.
|
||||||
|
|
||||||
|
## Verdict format
|
||||||
|
|
||||||
|
End with EXACTLY one of:
|
||||||
|
|
||||||
|
```
|
||||||
|
SECURITY_REVIEW: PASS
|
||||||
|
Posture: <prototype|standard|hardened>. Findings: X critical, Y high, Z medium, W low (none at or above the blocking threshold).
|
||||||
|
<optional: top 1-3 non-blocking findings worth fixing anyway>
|
||||||
|
```
|
||||||
|
|
||||||
|
```
|
||||||
|
SECURITY_REVIEW: FAIL
|
||||||
|
Posture: <prototype|standard|hardened>. Findings: X critical, Y high, Z medium, W low.
|
||||||
|
Blocking findings:
|
||||||
|
1. 🔴|🟠|🟡 <class, e.g. "Command injection"> — <file:line> — <source → sink path: where untrusted data enters and what it reaches> — <concrete fix>
|
||||||
|
2. ...
|
||||||
|
Non-blocking findings:
|
||||||
|
1. 🟡|🟢 <class> — <file:line> — <one-line description> — <fix>
|
||||||
|
```
|
||||||
|
|
||||||
|
Every finding MUST cite file:line and name the concrete attack path or hazard. "This might be insecure" is noise; `🟠 Path traversal — export.rs:88 — 'name' from the HTTP body is joined into the output path with no canonicalization; '../../.ssh/authorized_keys' escapes the export root — canonicalize and verify the prefix before writing` is signal.
|
||||||
|
|
||||||
|
## Scope discipline (what you are NOT)
|
||||||
|
|
||||||
|
- You are NOT the quality reviewer. Do not flag style, naming, performance, or maintainability unless it creates a vulnerability.
|
||||||
|
- You do NOT rewrite code. You produce a verdict and findings; the implementer owns the fix.
|
||||||
|
- You review the CHANGE. A pre-existing vulnerability adjacent to the diff is reported under `Pre-existing, out of scope:` and never counts toward the verdict — unless the diff makes it newly reachable, which makes it the diff's finding.
|
||||||
|
- Three real, exploitable findings beat fifteen theoretical ones. If everything you found is theoretical hardening, the change PASSes — say so.
|
||||||
|
|
||||||
|
## Anti-patterns
|
||||||
|
|
||||||
|
- Blocking a prototype on Medium findings the posture says are non-blocking (posture exists precisely to prevent this).
|
||||||
|
- Passing a committed secret because "it's just a POC."
|
||||||
|
- Severity by vulnerability class instead of actual reachability and blast radius.
|
||||||
|
- Findings with no file:line or no articulated attack path.
|
||||||
|
- Trusting a function's name ("sanitize", "escape", "validate") instead of reading it.
|
||||||
|
- Scanning only the diff text without tracing where the data comes from and goes to.
|
||||||
Reference in New Issue
Block a user