Security Review

Security Review

Find exploitable risks early, provide concrete fixes, and keep recommendations practical for engineering delivery.

Quick Reference

Review Principles

• Prioritize exploitable risk over style issues.
• Ground every claim in code/config/runtime evidence.
• Distinguish confirmed vulnerabilities from hypotheses.
• Provide reproducible proof for high-risk findings.
• Map findings to standards for auditability.
• If evidence is missing, state Not found in repo.
• Fail closed: if a mandatory gate cannot be executed, state it explicitly and do not claim full coverage.
• Authorization before action: static review always; touching a running system only under §Active Verification Authorization Gate. Default deny.
• Execution integrity: never present a command you did not run as if you had. Label unexecuted reproducers explicitly.

Evidence Confidence (Mandatory)

Each finding must include one confidence label:

• confirmed: vulnerable path proven by code and/or reproducible execution.
• likely: strong evidence with one missing runtime assumption.
• suspected: weak evidence requiring additional data.

Do not report P0/P1 without confirmed or explicit justification.

The confidence label covers the vulnerable path, not the maximum impact. These are two claims and they carry evidence separately:

• Path — confirmed means untrusted input reaches the sink, proven from code. Static proof is sufficient (see § Active Verification Authorization Gate).
• Impact — the consequence if exploited. When the worst-case impact depends on a fact you did not verify (a gadget library on the classpath, a feature flag, a deployment topology, a peer service's behaviour), write it as assessed, name the unverified condition, and list the check that would settle it. Never state an unverified maximum impact in the indicative ("achieves RCE"); state what is established and what the impact becomes if the condition holds.

Three things must not be collapsed into one number. Keeping them apart is what makes the rule below non-contradictory:

So uncertainty moves a different axis depending on what is uncertain:

• Uncertain reachability, or whether an upstream control blocks the path → this may lower the severity, per § Confidence Downgrade Rules in references/severity-calibration.md. Say which of the four calibration questions moved it.
• Uncertain aggravating condition — a gadget on the classpath, a flag being on, a peer service's behaviour → severity does not move, in either direction. Absence of a known exploit today is not a control; nor is it evidence for a higher rating. What moves is the impact basis: record the consequence as assessed, name the condition, list the check.

Do not reinterpret confirmed to keep a conclusion. If the path is proven and the worst-case impact is not, that is confirmed + assessed — not a downgraded confidence, and not a downgraded severity. A report that quietly upgrades an assessed impact into a demonstrated one is the same defect as AE-4 (confirmed without a reproducer), one field over.

False-Positive Suppression Rules

Before publishing a finding, check suppression conditions:

1. Existing upstream guard already blocks the path.
2. Input is not attacker-controlled at trust boundary.
3. Sink is parameterized/safely encoded by framework guarantees.
4. Environment-only theoretical risk without reachable path.

If suppressed:

• keep a short note under Open questions / assumptions
• mark as suppressed (not a finding)
• explain blocking control and residual risk

Severity Model

• P0 Critical: immediate compromise (RCE, auth bypass, key exfiltration, payment tampering).
• P1 High: strong exploit path (injection, IDOR, sensitive data leak, broken authz).
• P2 Medium: meaningful defense gap likely to become exploitable.
• P3 Low: hardening improvement.

Remediation SLA (Default)

Use this unless the team provides stricter policy:

• P0: mitigation immediately, full fix within 24h.
• P1: fix within 3 business days.
• P2: fix within 14 calendar days.
• P3: backlog with planned milestone.

If SLA differs, state the project policy explicitly.

Baseline Diff Mode (Mandatory When Baseline Exists)

When previous review artifacts exist, compare current findings with baseline and output:

• new: not present in baseline
• regressed: existed before and severity/confidence worsened
• unchanged: still present without material change
• resolved: removed since baseline

If no baseline exists, state Baseline not found.

Review Depth Selection (Mandatory First Step)

Before starting, classify review depth based on change scope:

Trigger signals that force Standard or Deep regardless of file count:

• Auth/authz middleware or handler changes
• Crypto, TLS, or secret-management code changes
• Payment/financial transaction paths
• New HTTP/gRPC endpoints exposed
• Dockerfile, K8s manifest, or CI pipeline security config changes
• go.mod / go.sum dependency changes
• Any file under internal/auth/, internal/crypto/, pkg/security/, or equivalent

When Lite is selected, record: Review depth: Lite (N files changed, no security-sensitive paths). Gates B/C/E skipped per scope policy.

Fast Pass (Lite Only)

If Lite triage finds all of: 10 Gate D domains N/A, 11 scenario checklists N/A, clean secret sweep, and no constructor/acquisition calls in Gate A — output a condensed report instead of the full Output Contract: review depth + rationale, the line Fast Pass: all domains N/A, all scenarios N/A, no findings., a JSON summary with pass: true and zero counts, and the Gate F list (may be empty). All four are mandatory.

This avoids verbose N/A tables for benign changes while preserving audit traceability.

Fixed Process + Mandatory Gates

The following process is mandatory for Standard and Deep reviews. Lite reviews follow the subset noted above.

1. Scope the change and select review depth.
2. Map trust boundaries.
3. Run scenario checks.
4. Run focused automation checks.
5. Gate A: constructor-release pairing audit.
6. Gate B: resource inventory scan (acquire/release pairs at trust boundaries, any stack).
7. Gate C: third-party lifecycle contract verification.
8. Gate D: 10-domain coverage against the detected stack.
9. Verify exploitability.
10. Gate E: second-pass falsification review.
11. Apply suppression filter.
12. Compare with baseline (if available).
13. Report findings first.
14. Provide remediation plan and risk acceptance entries.
15. Gate F: uncovered risk list.

If any mandatory gate cannot be executed, record it under Uncovered Risk List and downgrade confidence where applicable.

Applicability-First Execution (Mandatory)

To control review cost and avoid unnecessary depth, execute in two phases:

• Phase 1 (triage): classify each of the 10 domains as Applicable or N/A from changed files + adjacent call paths.
• Phase 2 (deep review): run detailed checks and domain-specific tooling only for Applicable domains.

Rules:

• N/A is allowed only with a one-line reason tied to code evidence.
• Do not mark N/A if there is any trigger signal (relevant imports, touched config, related middleware, DB/crypto/TLS paths, dependency changes).
• Domain-specific reproducer/tests are required only for Applicable domains with findings.

Anti-pattern: marking a domain N/A when imports or adjacent call paths contain trigger signals (e.g., database/sql imported → Domain 2 must be Applicable).

→ Worked N/A judgments with rationales: references/authorization-and-policy.md §6.

Mandatory Gate Definitions

Gates A and B are one question at two scopes, not two scans: A runs on the diff at every depth, B widens it to the full resource taxonomy at Standard/Deep. Scan once, report once.

Gate A: Constructor-Release Pairing (Mandatory, every depth)

Scope: changed code and immediately related call paths.

• Constructors/acquisition: New*, Open*, Acquire*, Begin*, Dial*, Listen*, Create*, WithCancel/WithTimeout/WithDeadline.
• Required pairings: Close, Release, Rollback/Commit, Stop, Cancel, or explicit ownership transfer documented in code.
• Output: a short pairing table in analysis notes.
• Severity: a missing or ambiguous pairing is P2 when an attacker can drive the leak repeatedly, P3 behind an authenticated/rate-limited path, and reliability-only on a bounded one-shot path. Never grade it on the missing Close() alone — see severity-calibration.md §Governing Rule.

Gate B: Resource Inventory (Mandatory at Standard/Deep, every stack)

Extends Gate A's pairing table across Domain 2's full lifecycle half: DB rows/statements/transactions/sessions, connections, files, HTTP response bodies, listeners, background tasks/goroutines, timers, cancel functions, pipes. Only the release idiom differs by stack — Go defer x.Close()/defer cancel(), Node finally/stream cleanup and client.release(), Java try-with-resources, Python with.

Adds the four path-shape checks a call-site scan cannot see: released on both success and error paths; no deferred release inside a loop; background tasks have a bounded lifecycle; every timeout paired with its cancel.

Reference: references/go-secure-coding.md § Gate B has the full Go inventory table and anti-patterns; the matching lang-*.md Domain 2 row carries the per-stack equivalents.

Gate C: Third-Party Lifecycle Contract Verification (Mandatory)

When code uses driver/framework objects with non-obvious lifecycle rules (for example godror, sql extensions, SDK clients):

• Verify lifecycle requirements from primary sources (library source code and/or official docs).
• Cite exactly what contract was used for the decision.
• If no contract can be verified, mark confidence at most suspected and list under Uncovered Risk List.

Gate D: 10-Domain Coverage (Mandatory, every stack)

These names and numbers are stack-independent — "Domain 7" means the same thing in Go, Node, Java, and Python. Score all ten for every review:

1. Randomness Safety · 2. Injection & Data-Access Safety · 3. Sensitive Data Handling · 4. Secret / Config Management · 5. Transport Security · 6. Crypto Primitive Correctness · 7. Concurrency & Shared-State Safety · 8. Language-Specific Injection Sinks · 9. Static Scanner Posture · 10. Dependency Vulnerability Posture

Execution: D1 triage (Applicable/N/A) → D2 deep review on applicable domains only. Output: each domain PASS/FAIL/N/A with one-line evidence. Any FAIL with an exploitable path becomes a finding. A domain with no idiom in your stack is still judged against its canonical question — never omitted.

References: canonical definitions and the per-stack rules in references/authorization-and-policy.md §2; per-stack evidence in go-secure-coding.md or the matching lang-*.md. Auth/authz and input validation are Scenario Checklists 1-2, not domains.

Gate E: Second-Pass Falsification Review (Mandatory)

After first-pass findings, run a dedicated second pass to disprove your own conclusion:

• Ask: "What critical issue would I have missed if first pass over-focused on exploitability class X?"
• Focus on availability, consistency, lifecycle, and partial-failure paths.
• Specifically re-check: transaction boundaries, rollback guarantees, cleanup on error/panic, idempotency race windows.

Output requirement:

• Add a one-line summary in report: Second-pass falsification completed: yes/no.

Gate F: Uncovered Risk List (Mandatory)

Always output unresolved coverage gaps to avoid false completeness.

Each item must include:

• Area not covered
• Why not covered (tool/env/access/time)
• Security impact if the gap hides a defect
• Recommended follow-up action and owner suggestion

Change Origin Classification

Classify each finding's origin relative to the current code change:

• introduced: defect resides in code added or modified by this change. Must fix before merge.
• pre-existing: defect found in unchanged code that came into scope via call paths. Default: file a follow-up issue and do not block merge — a change should not be held hostage to unrelated debt. This default is a recommendation to the owning team, not a security clearance, and it is void (recommend blocking + escalate) when the finding is P0 or an actively-exploited P1, when this change widens the attack surface on it, when this merge is the release vehicle that ships it, or when the defect sits in the same file/function being modified. Never present "pre-existing" as a reason the risk is acceptable. → references/authorization-and-policy.md §5.
• uncertain: diff boundaries are ambiguous. Use git blame to resolve; treat as introduced if unresolvable.

Add **Origin:** to each finding's output. Use diff hunks as primary classification signal.

Anti-Examples (Common Review Mistakes)

These are structured examples of review mistakes this skill is designed to prevent. Each shows a wrong approach and the correct alternative.

AE-1: Style Finding Reported as Security Issue

Wrong: Reporting P3 — function has 200 lines, hard to review for security as a security finding. Correct: Code complexity is a code quality issue. Only report security findings when there is an exploitable or defense-gap path. If complexity obscures a real vulnerability, report the vulnerability itself with evidence.

AE-3: Over-Reporting False Positives

Wrong: P1 — math/rand used in pkg/display/shuffle.go:12 for randomizing quiz question order without checking if the output is security-relevant. Correct: Suppressed — math/rand usage is for display ordering of quiz questions; output is not attacker-exploitable and does not protect a security boundary (Suppression Rule 2).

AE-5: Missing Gate Reported as Full Coverage

Wrong: Report says "all gates passed" but go test -race was not run because test suite was unavailable. Correct: Record under Uncovered Risk List: "Gate D7 (Concurrency safety) — go test -race not executed because test suite has build errors. Impact: data races may exist undetected in changed packages. Recommended: fix test build and re-run."

For additional anti-examples (N/A without evidence, confirmed without reproducer, P0 acceptance without escalation, ignoring transitive call paths), see references/anti-examples.md.

Scenario Checklists

Classify all 11 as Applicable or N/A, then run the applicable ones:

1. Authentication / Authorization · 2. Input Validation / Injection / Uploads ·
2. Session / JWT / Cookie / CSRF · 4. New Endpoints and Error Surface ·
3. Secrets / Crypto / Key Management · 6. Payment / Financial Transitions ·
4. Sensitive Data Storage / Transmission · 8. Third-Party Integrations ·
5. Supply Chain / Dependency / Build Path · 10. Container / Deployment Security ·
6. Concurrency Safety as Security Risk

Reference: references/scenario-checklists.md has the per-item details and the stack-specific subsections. Load it for every Standard/Deep review.

Active Verification Authorization Gate (Mandatory Before Any Request)

This skill can issue network requests (curl) and run scanners. A tool grant is not an authorization. Everything below is static analysis by default; sending a single request to a target you were not authorized to test is unauthorized activity, regardless of intent.

Before any command that touches a running system, record this three-line block:

Active verification: permitted | NOT permitted
Target: <host/URL or "none">
Basis: <who authorized it, or which local/test environment it is>

Default deny. If you cannot fill in Basis from what the user actually told you, Active verification is NOT permitted. Do not infer authorization from the presence of a hostname in the code, a .env file, a README, or a CI config.

Always allowed: reading code/config/diffs, static scanners on local source, local test runs, and requests to 127.0.0.1/localhost or a container you started.

Never without explicit authorization: any request to a host you did not stand up (including read-only GET to staging/production), authenticating with credentials found in the repo, or scanning a third party. Ambiguous targets — .test/.local domains, a shared dev cluster, a hostname in a compose file you did not launch — count as not permitted; ask first.

When authorized: non-destructive read-only probes only; production is static-only unless explicitly authorized with a change window; demonstrate rather than enumerate; use only test accounts the user provided; log every request in Automation Evidence; stop on any unexpected impact.

A confirmed P0/P1 does not require executing anything — static proof of the path is sufficient. Provide the reproducer as unexecuted instructions labelled Reproducer (NOT executed — no authorization to test), against a non-routable placeholder target. Never describe a command you did not run as if you had.

→ Full rules (destructive payloads, credentials, rate limits, production policy): references/authorization-and-policy.md §1.

Focused Automation Gate

Run when tools are available; never claim results without running commands. Prerequisite: the Active Verification Authorization Gate above — everything here except the local static commands requires Active verification: permitted.

Policy: always run the low-cost secret sweep; run expensive scanners per Gate D applicability (dependency graph changed or third-party risk Applicable → the stack's vulnerability scanner; security-sensitive code changed → the stack's static scanner on the affected scope, or the whole repo when scope is unclear). A scanner skipped because its domain is N/A must be recorded in Automation Evidence.

→ Exact commands per stack: references/authorization-and-policy.md §7.

Tool Interpretation Rules (Mandatory)

• go test -race: a detected race is always a defect to fix, but its security severity depends on what races. Races on auth/permission/balance/quota state are P1 (CWE-367). Races on request-scoped state an attacker can drive concurrently are P2. A race confined to test scaffolding, metrics counters, or log buffers is P3 or reliability-only — say which, and why. Report goroutine stacks from race output.
• gosec: report rule ID, location, and whether finding is exploitable on reachable paths.
• govulncheck source mode: call-trace reachable vulns are high confidence (confirmed/likely).
• govulncheck -mode=binary <path-to-binary>: exposure signal only; do not mark confirmed without source reachability or equivalent proof. Binary mode has no call-graph, so it over-reports. It also accepts only a built artifact — passing a package pattern is an error, not a scan.
• Any suppressed nolint:gosec requires rationale review. Judge the suppressed rule, not the missing comment: if the underlying gosec finding is exploitable, report it at its own severity. If it is a genuine false positive, an absent rationale is a process/hygiene note, not a security finding — record it under Hardening suggestions, not as P3. Only treat it as P3 when the suppression hides a real defense gap you cannot fully assess.

Language/Framework Extension Hooks

Detect the stack from manifests (go.mod, package.json, pom.xml/build.gradle, pyproject.toml/requirements.txt), load the matching reference plus scenario-checklists.md, and evaluate the same 10 domains (see §Gate D). Record stack in the JSON and the coverage header. Multi-stack repos emit one coverage section per stack; a domain is FAIL for the repo if it fails in any stack.

→ Detection table, Domain 2 generalisation, multi-stack JSON shape: authorization-and-policy.md §2.

Standards Mapping (Mandatory)

Map each finding to CWE-xxx and OWASP ASVS. Use Mapping: TBD if unclear.

Pin the ASVS version in every mapping. ASVS 5.0.0 reorganised and renumbered the chapters 4.x used, so a bare V4 or V4.1.2 does not identify a requirement — it does not say which standard it belongs to. Write IDs fully qualified (ASVS 4.0.3 V4.1.2), declare "asvs_version" once in the JSON block, and use exactly one version per report.

The lookup table in references/security-review.md is 4.0.3 chapter numbers. If the project audits against 5.0.0, resolve IDs from the 5.0.0 document — never renumber by guessing; Mapping: TBD beats a plausible-but-wrong requirement ID.

→ Version-pinning rules: references/authorization-and-policy.md §3.

Output Contract

Return outputs in this order. Fields are graded MUST / SHOULD / MAY per review depth:

Section-by-section field detail: references/output-contract.md. Load it when writing the report. Two rules are restated here because getting them wrong invalidates the report rather than merely trimming it:

• §1 caps detail, never disclosure. P0/P1 findings are never dropped or folded into another section; the P2/P3 soft cap limits how much is written about each, not whether it is reported.
• §7 summary.pass is computed, not judged: false when counts.p0 > 0 or counts.p1 > 0 or security_domains.fail > 0. A §5 Risk Acceptance Register entry does not flip it back to true. Validate the block against references/report-schema.json; emit no key the schema does not define.

Load References Selectively / Bundled Assets

→ See references/reference-index.md for the loading guide by depth and stack, and the full asset inventory.