From f432122abf538d59273613efacaab65e04a6311e Mon Sep 17 00:00:00 2001 From: Florian Wilde Date: Sun, 9 Aug 2026 01:22:05 +0200 Subject: [PATCH] feat: add a dedicated Security Auditor agent with veto power MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .claude/agents/qa-engineer.md | 26 ++--- .claude/agents/security-auditor.md | 149 +++++++++++++++++++++++++++++ .claude/skills/deploy/SKILL.md | 5 + .claude/skills/qa/SKILL.md | 77 +++++++++++---- CLAUDE.md | 2 +- README.md | 61 ++++++++++-- 6 files changed, 281 insertions(+), 39 deletions(-) create mode 100644 .claude/agents/security-auditor.md diff --git a/.claude/agents/qa-engineer.md b/.claude/agents/qa-engineer.md index 53f32b5..71d4ed2 100644 --- a/.claude/agents/qa-engineer.md +++ b/.claude/agents/qa-engineer.md @@ -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 5. Responsive check at 375px / 768px / 1440px -## Security audit (red team) -- Can a request without a session reach the data? -- Can user A read or modify user B's records by changing an ID? -- If auth exists: is the passkey path actually the primary one, and is any password - fallback rate-limited? A missing or weakened passkey flow is a **High** finding. -- Injection through every user-controlled input (XSS, SQL) -- Secrets or internal detail leaking in API responses, logs or the network tab -- **Published ports:** run `docker compose config | grep -B8 "published:"`. Only the - `app` service may publish, and only on `127.0.0.1`. A database, cache or queue with - 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. +## Security is not yours + +The **Security Auditor** agent owns the security verdict and runs alongside you. Do +not treat security as part of your coverage, and never write "Security: pass" — that +is not your call to make. + +If you stumble over something while testing (an error leaking internals, a request +that succeeds when it should not), report it as a normal bug and flag it +`→ Security Auditor`. Finding it is welcome; clearing it is not. ## Tests you write - **Unit (Vitest), co-located in `src/`:** hooks with real logic, pure transforms, @@ -62,8 +60,10 @@ if needed. ## What you return - Acceptance criteria: X of Y passed, with the failing ones named - 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 -- 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. diff --git a/.claude/agents/security-auditor.md b/.claude/agents/security-auditor.md new file mode 100644 index 0000000..c891c8f --- /dev/null +++ b/.claude/agents/security-auditor.md @@ -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/ 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. diff --git a/.claude/skills/deploy/SKILL.md b/.claude/skills/deploy/SKILL.md index 2006b45..7592827 100644 --- a/.claude/skills/deploy/SKILL.md +++ b/.claude/skills/deploy/SKILL.md @@ -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. **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 diff --git a/.claude/skills/qa/SKILL.md b/.claude/skills/qa/SKILL.md index 8bd06e3..032fbce 100644 --- a/.claude/skills/qa/SKILL.md +++ b/.claude/skills/qa/SKILL.md @@ -23,14 +23,27 @@ user. Update `features/INDEX.md` and the spec header to **In Review**. Verify by 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 list of deployed features to regression-check - 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 @@ -42,7 +55,7 @@ Append this to the feature spec — not to a separate file: ## QA Test Results **Getestet:** YYYY-MM-DD -**Tester:** QA Engineer (AI) +**Geprüft von:** QA Engineer + Security Auditor (AI) ### Acceptance Criteria - [x] AC-1: [Kriterium] @@ -51,13 +64,32 @@ Append this to the feature spec — not to a separate file: ### Edge Cases - [x] EC-1: [Randfall] — korrekt behandelt -### Security-Audit -- [x] Ohne Session kein Zugriff -- [x] Kein Zugriff auf fremde Datensätze -- [x] Passkey ist der primäre Anmeldeweg -- [x] Eingaben validiert, keine Injection +### Security-Audit (Security Auditor) -### 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] - **Severity:** Critical | High | Medium | Low @@ -68,33 +100,40 @@ Append this to the feature spec — not to a separate file: ### Zusammenfassung - **Acceptance Criteria:** X von Y bestanden - **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. ## 5. Review with the user -Present: criteria passed/failed, bugs by severity, security findings, verdict. -Then ask: -> "Welche Bugs sollen zuerst behoben werden?" +Present both verdicts separately — functional and security. Name every UNVERIFIED +security item out loud; those are the parts nobody has checked, and burying them in +the table is how they get forgotten. Then ask: +> "Welche Befunde sollen zuerst behoben werden?" ## 6. Wrap up - [ ] Every acceptance criterion has an individual pass/fail - [ ] 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 - [ ] Unit tests for non-trivial logic (`npm test` passes) - [ ] E2E test per passing criterion (`npm run test:e2e` passes) - [ ] 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 -- Clean → "Alle Tests bestanden, Status **Approved**. Nächster Schritt: `/deploy`." -- Bugs → "[N] Bugs gefunden ([Aufschlüsselung]). Status bleibt **In Review**. - Erst `/frontend` bzw. `/backend` zum Beheben, danach nochmal `/qa`." +- Both clean → "Tests bestanden, Security-Verdikt PASS. Status **Approved**. + Nächster Schritt: `/deploy`." +- 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 diff --git a/CLAUDE.md b/CLAUDE.md index f27d6c1..2b445dd 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -52,7 +52,7 @@ scripts/update.sh Pull, rebuild, restart, roll back on failure | 3 | `/architecture` | Tech design inside the spec | | 4 | `/frontend` | UI components | | 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 | `/refine PROJ-X` reopens an existing spec. `/help` reports where the project stands. diff --git a/README.md b/README.md index d8a3446..2d86983 100644 --- a/README.md +++ b/README.md @@ -15,7 +15,7 @@ liest sich jede Skill selbst wieder ein. - [Schnellstart](#schnellstart) - [Der Workflow im Überblick](#der-workflow-im-überblick) - [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) - [Kontext-Ebenen](#kontext-ebenen) - [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 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 | | **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. +### 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
Tut es, was die Spec verspricht?"] + QA --> B["Security Auditor
Ist es sicher fürs offene Internet?"] + + A --> AV{"Critical oder
High Bugs?"} + B --> BV{"Verdikt"} + + AV -->|ja| R["Status bleibt
In Review"] + BV -->|"BLOCK"| R + AV -->|nein| G((" ")) + BV -->|"PASS"| G + + G --> OK["Status Approved
→ /deploy"] + R --> FIX["/frontend oder /backend
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 @@ -546,7 +594,8 @@ README.md Dieses Dokument agents/ frontend-dev.md Sub-Agent für UI 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/ PRD.md Füllt /init im Interview 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 ``` -55 Dateien im Repo. +56 Dateien im Repo. ---