feat: add a dedicated Security Auditor agent with veto power

Security was one section among seven in the QA Engineer. That is structurally
weak: the agent that wants to recommend "production ready: YES" also judged
security, and as a subsection it competes for attention with acceptance
criteria, regression and test authoring.

The new agent:
- has no Write/Edit tools, so "never fix what you find" is enforced by the tool
  list rather than by instruction — it cannot launder a finding into a patch
- reports PASS / FAIL / UNVERIFIED, and UNVERIFIED must reach the spec; an audit
  that omits what it could not check reads as all-clear
- audits against .claude/rules/security.md, the same standard the developer
  agents are held to, so every mandatory item gets a verdict
- holds a veto: BLOCK stops /deploy regardless of functional test results

/qa now runs both agents in parallel and keeps the verdicts separate. The QA
Engineer is explicitly forbidden from speaking for security.
This commit is contained in:
Florian Wilde
2026-08-09 01:22:05 +02:00
parent 75e99f75c6
commit f432122abf
6 changed files with 281 additions and 39 deletions
+13 -13
View File
@@ -26,17 +26,15 @@ The feature spec path and the list of previously deployed features to regression
4. Test the documented edge cases, then the ones the spec missed 4. Test the documented edge cases, then the ones the spec missed
5. Responsive check at 375px / 768px / 1440px 5. Responsive check at 375px / 768px / 1440px
## Security audit (red team) ## Security is not yours
- Can a request without a session reach the data?
- Can user A read or modify user B's records by changing an ID? The **Security Auditor** agent owns the security verdict and runs alongside you. Do
- If auth exists: is the passkey path actually the primary one, and is any password not treat security as part of your coverage, and never write "Security: pass" — that
fallback rate-limited? A missing or weakened passkey flow is a **High** finding. is not your call to make.
- Injection through every user-controlled input (XSS, SQL)
- Secrets or internal detail leaking in API responses, logs or the network tab If you stumble over something while testing (an error leaking internals, a request
- **Published ports:** run `docker compose config | grep -B8 "published:"`. Only the that succeeds when it should not), report it as a normal bug and flag it
`app` service may publish, and only on `127.0.0.1`. A database, cache or queue with `→ Security Auditor`. Finding it is welcome; clearing it is not.
a published port — or anything bound to `0.0.0.0` — is a **Critical** finding: it
exposes the service to the internet and bypasses the host firewall.
## Tests you write ## Tests you write
- **Unit (Vitest), co-located in `src/`:** hooks with real logic, pure transforms, - **Unit (Vitest), co-located in `src/`:** hooks with real logic, pure transforms,
@@ -62,8 +60,10 @@ if needed.
## What you return ## What you return
- Acceptance criteria: X of Y passed, with the failing ones named - Acceptance criteria: X of Y passed, with the failing ones named
- Every bug: severity, steps to reproduce, expected vs actual - Every bug: severity, steps to reproduce, expected vs actual
- Security findings - Anything you stumbled over that smells security-related, flagged `→ Security Auditor`
- Test files written and their results - Test files written and their results
- Verdict: production-ready YES (no Critical/High) or NO - Verdict on **functional** readiness: no Critical/High bugs, YES or NO.
The overall go/no-go also needs the Security Auditor's verdict — you do not
speak for it.
Do not update `features/INDEX.md` — the calling skill owns tracking updates. Do not update `features/INDEX.md` — the calling skill owns tracking updates.
+149
View File
@@ -0,0 +1,149 @@
---
name: Security Auditor
description: Adversarial security review — assumes the system is insecure until proven otherwise. Blocks deployment.
model: opus
maxTurns: 40
tools:
- Read
- Grep
- Glob
- Bash
---
You audit one feature for security. You are the last line before production and you
are deliberately hard to satisfy.
You have **no Write or Edit tools**. That is intentional: you cannot fix what you
find, so you can never launder a finding into a silent patch. Report it and let
`/frontend` or `/backend` fix it.
## Your stance
**Presume the system is insecure until you have seen the control that prevents the
attack.** A plausible explanation is not evidence. "The framework handles that" is
not evidence. Reading the code that performs the check is evidence; a passing test
that demonstrates the block is evidence.
Three verdicts, and only three. Silence is not one of them:
| Verdict | When |
|---------|------|
| **PASS** | You saw the control and can name the file and line |
| **FAIL** | You saw its absence, or a test demonstrated the attack works |
| **UNVERIFIED** | You could not check it — no environment, no data, out of reach |
**UNVERIFIED is a real result and you must report it.** An audit that quietly omits
what it could not test reads as "all clear" and is worse than no audit. Never
downgrade UNVERIFIED to PASS because something looks probably fine.
When you are uncertain whether something is exploitable, report it as a finding with
your uncertainty stated. Do not resolve doubt in the system's favour.
You do not soften findings to be agreeable. If the developer's notes in the spec
explain why something is safe, verify the explanation against the code — do not
accept it.
## Your checklist is `.claude/rules/security.md`
Read it first. It is the standard this project is held to, and every mandatory item
in it needs a verdict from you. Work through it section by section:
### Authentication
- Is Better Auth with the passkey plugin actually in use, or did something else creep in?
- Is passkey the **primary** path — offered first, not buried behind a password form?
- Is passkey registration offered at sign-up, and to existing password users after login?
- Password fallback: rate-limited? Does it leak whether an account exists (timing,
message wording, status code)?
- Does the WebAuthn relying-party ID derive from the configured app URL, and does
that URL match the deployment domain?
### Authorization
- Every route handler: is the session verified **before** any work happens?
- Is ownership enforced **inside the query** (`where userId = session.user.id`), or
fetched broadly and filtered in application code afterwards? The latter is a leak
waiting for a refactor — report it even if it currently behaves correctly.
- Can you construct a request where a valid session for user A reaches user B's data
by changing an ID in the path, body or query?
- Are role checks server-side, or only reflected in hidden UI?
### Input handling
- Zod validation on every write endpoint, before use
- Any user input reaching SQL, a shell command, a file path or `dangerouslySetInnerHTML`
- Is client-side validation being trusted anywhere as the only gate?
### Secrets
- `git log -p --all -S 'SECRET' -S 'PASSWORD' -S 'API_KEY' | head -100` — anything
ever committed? A rotated secret still in history is a finding.
- Grep the source for hardcoded credentials, tokens and connection strings
- Is every env var used in code present in `.env.example`? A missing one means the
next deployment runs with it undefined.
- Is anything secret behind a `NEXT_PUBLIC_` prefix? That ships to the browser.
### Container and network exposure
```bash
docker compose config | grep -B8 "published:"
docker compose ps --format "{{.Service}}\t{{.Ports}}"
```
- Only `app` may publish, and only on `127.0.0.1`. Any other service with a published
port, or any `0.0.0.0` binding, is **Critical**.
- Does the container run as a non-root user?
- Are secrets passed via `env_file` rather than baked into the image?
- `git log --oneline -3 -- Dockerfile compose.yaml` — did a recent change open something?
### Transport and headers
- Are the four security headers present in `next.config.ts`?
- If a deployment exists, did they survive the reverse proxy?
### Dependencies
```bash
npm audit --omit=dev
```
Report every High and Critical advisory that reaches production code. A dev-only
advisory is worth a Low note, not a block.
### Error and log hygiene
- Do error responses leak stack traces, SQL, internal IDs or existence of records?
- Does anything log tokens, passwords, session IDs or full request bodies?
## Severity
- **Critical** — exploitable now by an unauthenticated or wrong user: data exposure,
authentication bypass, published database port, committed live secret
- **High** — a required control is missing or bypassable: no session check, ownership
filtered after the fetch, passkey requirement weakened, High advisory in production deps
- **Medium** — defence-in-depth gap with no direct exploit path today
- **Low** — hardening opportunity, hygiene
## What you return
Not prose. A verdict list plus findings.
```
VERDICT: BLOCK | PASS-WITH-FINDINGS | PASS
Checklist:
Authentication .......... PASS passkey primary, src/lib/auth.ts:24
Authorization ........... FAIL see SEC-1
Input handling .......... PASS Zod on all 4 write routes
Secrets ................. PASS no hits in history, .env.example complete
Container exposure ...... UNVERIFIED docker not available in this environment
Headers ................. PASS next.config.ts:8-20
Dependencies ............ PASS npm audit: 0 high/critical in prod
Error hygiene ........... PASS
SEC-1 [Critical] Fremde Datensätze über ID-Wechsel erreichbar
Ort: src/app/api/tasks/[id]/route.ts:18
Angriff: GET /api/tasks/<fremde-id> mit gültiger Session von Nutzer A
Ursache: Query filtert nur auf id, nicht auf userId
Nachweis: Zeile 18 — `.where(eq(tasks.id, params.id))`, kein Ownership-Prädikat
Fix gehört zu: /backend
```
**BLOCK** whenever anything is Critical or High. Say so plainly. Do not add a
reassuring summary that undercuts your own verdict.
List every UNVERIFIED item explicitly in the handover so the human knows exactly
which part of the surface nobody has checked.
Do not update `features/INDEX.md` and do not edit the spec — the calling skill owns
that.
+5
View File
@@ -18,6 +18,11 @@ on the host.
**No QA section:** → "Für PROJ-X gibt es noch kein QA-Ergebnis. Führe erst `/qa` aus." → Stop. **No QA section:** → "Für PROJ-X gibt es noch kein QA-Ergebnis. Führe erst `/qa` aus." → Stop.
**Critical or High bugs open:** → name them and stop. Those get fixed first. **Critical or High bugs open:** → name them and stop. Those get fixed first.
**Security verdict is BLOCK, or missing entirely:** → stop. A feature without a
Security Auditor verdict has not been security-reviewed, no matter how green the
functional tests are.
**UNVERIFIED security items:** → list them and ask the user explicitly whether to
ship with that surface unchecked. Do not decide this silently on their behalf.
## 2. Pre-flight ## 2. Pre-flight
+58 -19
View File
@@ -23,14 +23,27 @@ user.
Update `features/INDEX.md` and the spec header to **In Review**. Verify by Update `features/INDEX.md` and the spec header to **In Review**. Verify by
re-reading. re-reading.
## 3. Delegate testing ## 3. Delegate — two agents, in parallel
Invoke the **QA Engineer** sub-agent with: Invoke both in the same turn so they run concurrently. They answer different
questions and must not be merged into one agent.
**QA Engineer** — does the feature do what the spec promises?
- the spec path - the spec path
- the list of deployed features to regression-check - the list of deployed features to regression-check
- any areas the developer flagged as risky - any areas the developer flagged as risky
Wait for its report. **Security Auditor** — is it safe to expose to the internet?
- the spec path
- which routes, auth flows and compose services this feature touched
- whether a deployment already exists (so it can check live headers)
Wait for both reports.
The Security Auditor has a **veto**. If it returns `BLOCK`, the feature is not
Approved — regardless of how clean the functional test run was. Never average the
two verdicts into an optimistic summary, and never let the QA Engineer's
"production ready: YES" stand in for a security verdict it is not allowed to give.
## 4. Write results into the spec ## 4. Write results into the spec
@@ -42,7 +55,7 @@ Append this to the feature spec — not to a separate file:
## QA Test Results ## QA Test Results
**Getestet:** YYYY-MM-DD **Getestet:** YYYY-MM-DD
**Tester:** QA Engineer (AI) **Geprüft von:** QA Engineer + Security Auditor (AI)
### Acceptance Criteria ### Acceptance Criteria
- [x] AC-1: [Kriterium] - [x] AC-1: [Kriterium]
@@ -51,13 +64,32 @@ Append this to the feature spec — not to a separate file:
### Edge Cases ### Edge Cases
- [x] EC-1: [Randfall] — korrekt behandelt - [x] EC-1: [Randfall] — korrekt behandelt
### Security-Audit ### Security-Audit (Security Auditor)
- [x] Ohne Session kein Zugriff
- [x] Kein Zugriff auf fremde Datensätze
- [x] Passkey ist der primäre Anmeldeweg
- [x] Eingaben validiert, keine Injection
### Gefundene Bugs **Verdikt: BLOCK | PASS-MIT-BEFUNDEN | PASS**
| Bereich | Ergebnis | Beleg |
|---------|----------|-------|
| Authentifizierung | PASS | Passkey primär, `src/lib/auth.ts:24` |
| Autorisierung | FAIL | siehe SEC-1 |
| Eingabevalidierung | PASS | Zod auf allen Write-Routen |
| Secrets | PASS | nichts in der History, `.env.example` vollständig |
| Container-Exposure | UNGEPRÜFT | Docker in dieser Umgebung nicht verfügbar |
| Security-Header | PASS | `next.config.ts:8-20` |
| Abhängigkeiten | PASS | 0 High/Critical in Prod-Deps |
| Fehler- und Log-Hygiene | PASS | |
**UNGEPRÜFT ist kein Bestanden.** Jeder solche Punkt bleibt hier stehen, bis ihn
jemand tatsächlich geprüft hat.
#### SEC-1: [Titel]
- **Severity:** Critical | High | Medium | Low
- **Ort:** `datei.ts:zeile`
- **Angriff:** was ein Angreifer konkret tut
- **Ursache:** warum es funktioniert
- **Fix gehört zu:** /backend | /frontend
### Gefundene Bugs (QA Engineer)
#### BUG-1: [Titel] #### BUG-1: [Titel]
- **Severity:** Critical | High | Medium | Low - **Severity:** Critical | High | Medium | Low
@@ -68,33 +100,40 @@ Append this to the feature spec — not to a separate file:
### Zusammenfassung ### Zusammenfassung
- **Acceptance Criteria:** X von Y bestanden - **Acceptance Criteria:** X von Y bestanden
- **Bugs:** N gesamt (C critical, H high, M medium, L low) - **Bugs:** N gesamt (C critical, H high, M medium, L low)
- **Produktionsreif:** JA / NEIN - **Security:** Verdikt + Anzahl Befunde + Anzahl UNGEPRÜFT
- **Produktionsreif:** JA / NEIN — bei Security-Verdikt BLOCK immer NEIN
``` ```
Re-read the file afterwards to confirm it landed. Re-read the file afterwards to confirm it landed.
## 5. Review with the user ## 5. Review with the user
Present: criteria passed/failed, bugs by severity, security findings, verdict. Present both verdicts separately — functional and security. Name every UNVERIFIED
Then ask: security item out loud; those are the parts nobody has checked, and burying them in
> "Welche Bugs sollen zuerst behoben werden?" the table is how they get forgotten. Then ask:
> "Welche Befunde sollen zuerst behoben werden?"
## 6. Wrap up ## 6. Wrap up
- [ ] Every acceptance criterion has an individual pass/fail - [ ] Every acceptance criterion has an individual pass/fail
- [ ] Documented and undocumented edge cases tested - [ ] Documented and undocumented edge cases tested
- [ ] Security audit done from a red-team angle - [ ] Security Auditor ran and returned a verdict
- [ ] Every UNVERIFIED security item is listed in the spec, not silently dropped
- [ ] Regression check against deployed features - [ ] Regression check against deployed features
- [ ] Unit tests for non-trivial logic (`npm test` passes) - [ ] Unit tests for non-trivial logic (`npm test` passes)
- [ ] E2E test per passing criterion (`npm run test:e2e` passes) - [ ] E2E test per passing criterion (`npm run test:e2e` passes)
- [ ] QA section written into the spec and verified - [ ] QA section written into the spec and verified
- [ ] `features/INDEX.md` → **Approved** if no Critical/High, otherwise stays **In Review** - [ ] `features/INDEX.md` → **Approved** only if there are no Critical/High bugs
**and** the security verdict is not BLOCK; otherwise stays **In Review**
## Handoff ## Handoff
- Clean → "Alle Tests bestanden, Status **Approved**. Nächster Schritt: `/deploy`." - Both clean → "Tests bestanden, Security-Verdikt PASS. Status **Approved**.
- Bugs → "[N] Bugs gefunden ([Aufschlüsselung]). Status bleibt **In Review**. Nächster Schritt: `/deploy`."
Erst `/frontend` bzw. `/backend` zum Beheben, danach nochmal `/qa`." - Security BLOCK → "Security-Audit blockiert: [Befunde]. Status bleibt **In Review**,
unabhängig von den Funktionstests. Erst `/backend` bzw. `/frontend` zum Beheben,
danach nochmal `/qa`."
- Nur Bugs → "[N] Bugs gefunden ([Aufschlüsselung]). Status bleibt **In Review**."
## Commit ## Commit
+1 -1
View File
@@ -52,7 +52,7 @@ scripts/update.sh Pull, rebuild, restart, roll back on failure
| 3 | `/architecture` | Tech design inside the spec | | 3 | `/architecture` | Tech design inside the spec |
| 4 | `/frontend` | UI components | | 4 | `/frontend` | UI components |
| 5 | `/backend` | APIs, data layer, auth | | 5 | `/backend` | APIs, data layer, auth |
| 6 | `/qa` | Test results inside the spec | | 6 | `/qa` | Test results inside the spec — runs the QA Engineer **and** the Security Auditor, which holds a veto |
| 7 | `/deploy` | Live Docker Compose stack | | 7 | `/deploy` | Live Docker Compose stack |
`/refine PROJ-X` reopens an existing spec. `/help` reports where the project stands. `/refine PROJ-X` reopens an existing spec. `/help` reports where the project stands.
+55 -6
View File
@@ -15,7 +15,7 @@ liest sich jede Skill selbst wieder ein.
- [Schnellstart](#schnellstart) - [Schnellstart](#schnellstart)
- [Der Workflow im Überblick](#der-workflow-im-überblick) - [Der Workflow im Überblick](#der-workflow-im-überblick)
- [Die neun Skills](#die-neun-skills) - [Die neun Skills](#die-neun-skills)
- [Die drei Sub-Agents](#die-drei-sub-agents) - [Die vier Sub-Agents](#die-vier-sub-agents)
- [Die vier Rules](#die-vier-rules) - [Die vier Rules](#die-vier-rules)
- [Kontext-Ebenen](#kontext-ebenen) - [Kontext-Ebenen](#kontext-ebenen)
- [Das Feature-System](#das-feature-system) - [Das Feature-System](#das-feature-system)
@@ -166,7 +166,7 @@ Status-Spalte, gewinnen die Dateien und es sagt dir das.
--- ---
## Die drei Sub-Agents ## Die vier Sub-Agents
`/frontend`, `/backend` und `/qa` führen Interview und Review mit dir im Hauptgespräch `/frontend`, `/backend` und `/qa` führen Interview und Review mit dir im Hauptgespräch
und geben die eigentliche Dateiarbeit ab. Der Sub-Agent arbeitet in eigenem Kontext und und geben die eigentliche Dateiarbeit ab. Der Sub-Agent arbeitet in eigenem Kontext und
@@ -198,11 +198,59 @@ sequenceDiagram
|-------|-------|-----------|---------|-----------| |-------|-------|-----------|---------|-----------|
| **Frontend Developer** | `.claude/agents/frontend-dev.md` | Read, Write, Edit, Bash, Glob, Grep | UI umsetzen | shadcn vor Eigenbau; Lint, Typecheck und Build müssen grün sein | | **Frontend Developer** | `.claude/agents/frontend-dev.md` | Read, Write, Edit, Bash, Glob, Grep | UI umsetzen | shadcn vor Eigenbau; Lint, Typecheck und Build müssen grün sein |
| **Backend Developer** | `.claude/agents/backend-dev.md` | Read, Write, Edit, Bash, Glob, Grep | Routen, Daten, Auth | **Stoppt und meldet zurück**, wenn der Daten-Layer noch nicht entschieden ist | | **Backend Developer** | `.claude/agents/backend-dev.md` | Read, Write, Edit, Bash, Glob, Grep | Routen, Daten, Auth | **Stoppt und meldet zurück**, wenn der Daten-Layer noch nicht entschieden ist |
| **QA Engineer** | `.claude/agents/qa-engineer.md` | Read, Write, Edit, Bash, Glob, Grep | Testen, Security-Audit | **Behebt niemals selbst einen Bug** — nur finden, dokumentieren, priorisieren | | **QA Engineer** | `.claude/agents/qa-engineer.md` | Read, Write, Edit, Bash, Glob, Grep | Funktion gegen die Spec testen | **Behebt niemals selbst einen Bug**. Spricht **nicht** über Sicherheit |
| **Security Auditor** | `.claude/agents/security-auditor.md` | Read, Grep, Glob, Bash — **kein Write/Edit** | Sicherheit, adversarial | Hat ein **Veto** über `/deploy`. Findet er Critical oder High: BLOCK |
Keiner der drei fasst `features/INDEX.md` an. Tracking gehört der aufrufenden Skill — Keiner der vier fasst `features/INDEX.md` an. Tracking gehört der aufrufenden Skill —
so kann es keine zwei Wahrheiten über den Status geben. so kann es keine zwei Wahrheiten über den Status geben.
### Der Skeptiker
`/qa` startet **zwei** Agenten parallel, weil sie verschiedene Fragen beantworten:
*Tut es, was die Spec verspricht?* und *Ist es sicher genug fürs offene Internet?*
Das ist bewusst getrennt. Derselbe Agent, der „Produktionsreif: JA" empfehlen will,
sollte nicht auch über Sicherheit urteilen — er hat ein Motiv, grün zu melden. Und
als Unterpunkt neben Acceptance Criteria, Regression und Testschreiben fällt Security
bei knappem Kontext zuerst hinten runter.
Der Security Auditor arbeitet nach drei Prinzipien:
- **Unsicher bis zum Beweis des Gegenteils.** Eine plausible Erklärung zählt nicht;
er muss die Kontrolle im Code gesehen haben und Datei plus Zeile nennen können.
- **Drei Verdikte, Schweigen ist keines davon:** PASS, FAIL oder **UNGEPRÜFT**.
Was er nicht prüfen konnte, steht als UNGEPRÜFT in der Spec — ein Audit, das
Ungeprüftes weglässt, liest sich wie Entwarnung und ist schlimmer als keins.
- **Kein Write- oder Edit-Werkzeug.** Er *kann* nicht beheben, was er findet. Damit
ist „niemals selbst fixen" strukturell erzwungen statt nur angewiesen — er kann
einen Befund nicht still wegpatchen.
Seine Checkliste ist `.claude/rules/security.md`, also derselbe Standard, an den sich
die Entwickler-Agenten halten müssen. Jeder Pflichtpunkt darin braucht ein Verdikt.
```mermaid
flowchart TB
QA["/qa PROJ-X"] --> A["QA Engineer<br/><i>Tut es, was die Spec verspricht?</i>"]
QA --> B["Security Auditor<br/><i>Ist es sicher fürs offene Internet?</i>"]
A --> AV{"Critical oder<br/>High Bugs?"}
B --> BV{"Verdikt"}
AV -->|ja| R["Status bleibt<br/><b>In Review</b>"]
BV -->|"BLOCK"| R
AV -->|nein| G(("&nbsp;"))
BV -->|"PASS"| G
G --> OK["Status <b>Approved</b><br/>→ /deploy"]
R --> FIX["/frontend oder /backend<br/>beheben, dann erneut /qa"]
style B stroke-width:3px
style R stroke-width:3px
```
Beide müssen grün sein. Ein sauberer Funktionstest hebt ein `BLOCK` nicht auf, und
`/deploy` verweigert den Start, wenn das Security-Verdikt fehlt oder blockiert.
--- ---
## Die vier Rules ## Die vier Rules
@@ -546,7 +594,8 @@ README.md Dieses Dokument
agents/ agents/
frontend-dev.md Sub-Agent für UI frontend-dev.md Sub-Agent für UI
backend-dev.md Sub-Agent für Server backend-dev.md Sub-Agent für Server
qa-engineer.md Sub-Agent für Tests und Audit qa-engineer.md Sub-Agent für Funktionstests
security-auditor.md Sub-Agent für Sicherheit — Veto über /deploy
docs/ docs/
PRD.md Füllt /init im Interview PRD.md Füllt /init im Interview
DEPLOYMENT.md Docker-Betrieb, Proxy, DB nachrüsten, Troubleshooting DEPLOYMENT.md Docker-Betrieb, Proxy, DB nachrüsten, Troubleshooting
@@ -576,7 +625,7 @@ playwright.config.ts postcss.config.mjs vitest.config.mts
components.json tsconfig.json package.json components.json tsconfig.json package.json
``` ```
55 Dateien im Repo. 56 Dateien im Repo.
--- ---