Security Verdict and Report

Overview

A security reviewer that blocks on noise gets ignored; one that passes on a hunch ships a breach. The verdict follows a fixed policy so the developer can predict it and the human can trust it.

Core principle: block only what you can prove is exploitable and new; say nothing about what you cannot.

1. Severity

Severity	Meaning	Typical examples
CRITICAL	Exploitable now with little or no precondition; full compromise	RCE, auth bypass, SQL injection on a public endpoint, a live production secret committed, a malicious dependency
HIGH	Significant, reachable, one realistic precondition	IDOR on another user's data, stored XSS, SSRF to an internal service, path traversal reading server files, pull_request_target running PR code with secrets
MEDIUM	Needs specific conditions, or real but bounded impact	CSRF on a state change, reflected XSS behind an unusual flow, user enumeration that enables a further attack, verbose error leaking internal paths
LOW	Defence in depth, best practice	Missing header with no vulnerability, weak-but-unused config

Exposure moves severity, not existence: "only reachable from the internal network" can still be HIGH when internal callers are not trusted. Use the deployment shape from get_project_brief.

Anchors that keep two reviews of the same bug at the same level:

• CRITICAL — an unauthenticated actor gains code execution, full data-store access, or takeover of arbitrary accounts.
• HIGH — an actor fully defeats an explicit security control with real consequences: authentication bypass, cross-tenant read or write, stored script execution against other users, authenticated code execution, an unauthenticated remote stop of a shared service.
• MEDIUM — a real boundary violation with a limited blast radius, uncommon preconditions, or consequences confined to a narrow set of resources.
• LOW — disclosure of non-secret internals, or an effect that needs sustained effort for minimal gain. Dropped.

The HIGH/MEDIUM line: does the traced result fully defeat an explicit control for an action with real consequences, or only weaken it? Likelihood — preconditions, attacker position — moves severity down; nothing moves it above the impact the trace shows. If you cannot state the concrete damage, the severity is lower than it feels.

2. Confidence

Confidence is about exploitability in this code, not about whether the pattern looks bad:

• 0.9–1.0 — you traced source → sink end to end and can write the exploit input.
• 0.8–0.9 — a known vulnerable pattern; source and sink both read; no guard found on the path.
• 0.7–0.8 — suspicious, but it needs a condition you could not confirm. Not reported.
• < 0.7 — not reported.

3. The Decision

Evaluate every surviving finding, then apply the strongest outcome:

1. Unreviewable — part of the change could not be read (unreviewable-never-passes) → need_revision, naming exactly what could not be verified.
2. Block → need_revision when any holds:
   • a CRITICAL or HIGH finding introduced or enabled by the diff, confidence ≥ 0.8, surviving the exclusions and the self-refute pass;
   • a live secret in non-test code;
   • a new or changed dependency that is malicious, a likely typosquat, or carries a critical/high advisory with a fixed version available;
   • CI or agent escalation: pull_request_target with a PR-head checkout and secrets in reach, an agent permission-bypass flag, untrusted input reaching an LLM that holds tools.
3. Approve with notes → ready_for_qa plus one comment: MEDIUM findings at ≥ 0.8, at most three, highest first; and Needs validation notes, at most two (§6).
4. Clean → ready_for_qa, no comment.

LOW and < 0.8 findings are dropped: not "for awareness", not "minor", not mentioned.

4. Reject Template

One add_task_comment, before the move. The developer receives it alongside the architect's review, so it must stand alone.

Security review — changes requested

SEC-1 · HIGH · Broken object-level authorization (CWE-639, OWASP A01:2025) · internal/invoice/handler.go:57
Exploit: an attacker with any customer account can GET /api/invoices/{id} with another customer's id → reads their invoices (amounts, addresses).
Evidence: r.PathValue("id") (handler.go:52) → repo.Get(ctx, id) (repo.go:31, WHERE id = $1 only) → JSON response; no OrgID check on the path. Siblings use repo.GetForOrg.
Fix: use repo.GetForOrg(ctx, id, auth.OrgID(ctx)) and return 404 when it finds nothing; add a test where org B requests org A's invoice.

SEC-2 · CRITICAL · Hard-coded credential (CWE-798, OWASP A07:2025) · config/prod.go:12
Exploit: anyone with read access to the repository can use the Stripe live key → charges and refunds on the production account.
Evidence: const stripeKey = "sk_live_…" (prod.go:12), used by billing/client.go:20.
Fix: load it from the secret manager like the other keys; the key must also be rotated — removing it from the code does not un-leak it.

Reviewed: all 14 changed files; scanners: gitleaks (diff), semgrep p/default on changed files.

Per finding: severity · category (CWE-x, OWASP A0x:2025) · file:line; the exploit sentence ("an attacker with [capability] can [action] → [impact]"); the evidence trace with file:line; the fix. Redact secrets to a recognisable prefix — never paste the full value into a comment.

The fix is the smallest source change that enforces the invariant at the last trusted decision point — the repository method, check or parameter by name, plus the regression test that would fail if the hole reopened. "Sanitise input", "add validation" or "harden this" is not a fix. When one root cause appears at several locations, the fix names every location (coverage-and-variant-pass §4).

Needs validation notes, if any, follow the last SEC-n in the same comment (§6).

5. Approve-with-Notes Template

Security review — approved with notes

1. MEDIUM · CSRF (CWE-352, OWASP A01:2025) · web/routes/settings.ts:88 — POST /settings/email relies on the session cookie with SameSite=None and no CSRF token; a page the user visits can change their email. Add the csrf middleware its siblings use.

No praise, no list of what was fine, no LOW items, no more than three notes.

6. Needs-Validation Notes

For a lead whose source trace is complete, that would be HIGH or CRITICAL, and that exactly one fact outside the repository decides (source-visibility-needs-validation). It has no severity and never blocks on its own; at most two per review.

NV-1 · Identity header trusted behind the auth proxy · internal/api/middleware.go:31
Trace: X-Forwarded-User (middleware.go:31) → auth.SetUser (middleware.go:38) → every /api route; deploy/k8s/api-service.yaml:9 publishes the pod as type LoadBalancer.
Unresolved: whether the cluster's network policy or cloud firewall lets traffic reach that LoadBalancer without passing oauth2-proxy.
Owner check: confirm the LoadBalancer's source ranges or the firewall rule in the cloud console; if it is reachable, drop the header trust or make the Service ClusterIP.

On an otherwise clean review, the comment is headed Security review — approved with notes and carries only the NV items. Never phrase a needs-validation note as a confirmed vulnerability, and never ask anyone to test it against a live system.

7. Move and Stop

• Comment first, then move_board_task. A move without its comment hands the developer a rejection with no reason, and the one-word fallback verdict cannot add one.
• When the move tool says your verdict is recorded — the card waits for the other reviewer(s), or went to need_revision because another reviewer asked for changes — the run is over. Never move it again.
• Your run's final message is the run summary, not a second comment: one line with the verdict and the count of findings.

Common Mistakes

• Rating by category instead of by reachable impact: an "SQL injection" on a constant table name is not CRITICAL; it is not a finding.
• Blocking on a MEDIUM because there were several of them. Count does not raise severity.
• Pasting the full secret, the full scanner output or a stack of LOW items into the comment.
• Writing "possible", "might", "could potentially" in a blocking finding — if you would hedge it, it is below 0.8.
• Repeating the architect's points (tests, naming, design) to make the comment look thorough.

Red Flags

• A blocking finding with no exploit sentence or no file:line.
• A rejection whose fix is "sanitise input" with no named function, parameter or check.
• An approval while a file in the diff was never opened.
• A notes comment on a clean approval that only says "looks good".
• A needs-validation note with a severity, with no exact unresolved fact, or used for a lead whose trace is incomplete.

References (names and links only)

OWASP Top 10:2025 · CWE Top 25 (2025) · CVSS v4.0 (vocabulary only; the policy above decides)