diff --git a/.changeset/fix-3120-secure-phase-empty-register.md b/.changeset/fix-3120-secure-phase-empty-register.md new file mode 100644 index 000000000..1f8ac211c --- /dev/null +++ b/.changeset/fix-3120-secure-phase-empty-register.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3142 +--- +**`secure-phase` no longer rubber-stamps SECURITY.md for legacy phases with no `` blocks** — Step 3's short-circuit previously exited to Step 6 (write clean SECURITY.md) whenever `threats_open: 0`, regardless of whether zero threats meant "all mitigated" or "none were ever written". Legacy phases authored before `` blocks became canonical now trigger **retroactive-STRIDE mode** in Step 5: the auditor builds a register from implementation files before verifying mitigations. Step 2c now tracks `register_authored_at_plan_time` and Step 3 gates the skip on both `threats_open: 0 AND register_authored_at_plan_time: true`. Closes #3120. diff --git a/get-shit-done/workflows/secure-phase.md b/get-shit-done/workflows/secure-phase.md index f2ec84c80..1a306ed83 100644 --- a/get-shit-done/workflows/secure-phase.md +++ b/get-shit-done/workflows/secure-phase.md @@ -58,6 +58,8 @@ Read SUMMARY.md — extract `## Threat Flags` entries. Per threat: `{ threat_id, category, component, disposition, mitigation_pattern, files_to_check }` +Also set `register_authored_at_plan_time: true` if **at least one** PLAN file contained a parseable `` block; `false` if no PLAN files had any `` block (legacy phase authored before formal threat modelling was standard). + ## 3. Threat Classification Classify each threat: @@ -69,7 +71,10 @@ Classify each threat: Build: `{ threat_id, category, component, disposition, status, evidence }` -If `threats_open: 0` → skip to Step 6 directly. +**Short-circuit rule:** +- If `threats_open: 0 AND register_authored_at_plan_time: true` → skip to Step 6 directly. All plan-time threats are verified CLOSED. +- If `threats_open: 0 AND register_authored_at_plan_time: false` → **do NOT skip**. Empty-by-no-planning must not rubber-stamp a clean SECURITY.md. Proceed to Step 5 in **retroactive-STRIDE mode** — the auditor builds a register from implementation files first, then verifies mitigations. +- If `threats_open > 0` → proceed to Step 4 (present threat plan to user). ## 4. Present Threat Plan @@ -82,6 +87,11 @@ Call AskUserQuestion with threat table and options: ## 5. Spawn gsd-security-auditor +**Auditor constraint — varies by register origin:** + +- `register_authored_at_plan_time: true` — **Verify mitigations exist** — do not scan for new threats. The register is complete; verify each threat's mitigation is present in the implementation. +- `register_authored_at_plan_time: false` (retroactive-STRIDE mode) — **Retroactive-STRIDE: build a STRIDE register from implementation files first, then verify mitigations.** The phase was authored before formal threat modelling; the auditor must construct the register from scratch before verifying. + ``` Task( prompt="Read ~/.claude/agents/gsd-security-auditor.md for instructions.\n\n" + @@ -158,7 +168,8 @@ Display `/clear` reminder. - [ ] Input state detected (A/B/C) — state C exits cleanly - [ ] PLAN.md threat model parsed, register built - [ ] SUMMARY.md threat flags incorporated -- [ ] threats_open: 0 → skip directly to Step 6 +- [ ] threats_open: 0 AND register_authored_at_plan_time: true → skip directly to Step 6 +- [ ] threats_open: 0 AND register_authored_at_plan_time: false → retroactive-STRIDE mode (Step 5), not skipped - [ ] User gate with threat table presented - [ ] Auditor spawned with complete context - [ ] All three return formats (SECURED/OPEN_THREATS/ESCALATE) handled diff --git a/tests/bug-3120-secure-phase-empty-register.test.cjs b/tests/bug-3120-secure-phase-empty-register.test.cjs new file mode 100644 index 000000000..1900f03e3 --- /dev/null +++ b/tests/bug-3120-secure-phase-empty-register.test.cjs @@ -0,0 +1,58 @@ +'use strict'; +// allow-test-rule: reads product workflow markdown (secure-phase.md) to verify structural guard contract — not a source-grep test + +// Regression guard for bug #3120. +// +// secure-phase.md Step 3 short-circuited to Step 6 (write SECURITY.md) +// whenever threats_open: 0, without distinguishing between: +// Case A: All plan-time threat_model threats are CLOSED (legitimate skip) +// Case B: No threat_model blocks were written at plan time (legacy phases) +// → rubber-stamps a clean SECURITY.md with zero audit performed +// +// Fix: Step 2c tracks `register_authored_at_plan_time` (true iff ≥1 PLAN +// file contained a parseable block). Step 3 now requires BOTH +// threats_open: 0 AND register_authored_at_plan_time to skip. If only +// threats_open: 0 and NOT register_authored_at_plan_time, Step 5 runs in +// retroactive-STRIDE mode. + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const ROOT = path.join(__dirname, '..'); +const src = fs.readFileSync( + path.join(ROOT, 'get-shit-done', 'workflows', 'secure-phase.md'), + 'utf8', +); + +describe('bug #3120: secure-phase short-circuit guards', () => { + test('Step 2c tracks register_authored_at_plan_time', () => { + assert.ok( + src.includes('register_authored_at_plan_time'), + 'secure-phase.md does not track register_authored_at_plan_time in Step 2c', + ); + }); + + test('Step 3 short-circuit requires both conditions', () => { + assert.ok( + src.includes('threats_open: 0 AND register_authored_at_plan_time'), + 'Step 3 short-circuit does not gate on both threats_open:0 AND register_authored_at_plan_time', + ); + }); + + test('retroactive-STRIDE mode is documented for legacy phases', () => { + assert.ok( + src.includes('retroactive') || src.includes('Retroactive'), + 'secure-phase.md does not document retroactive-STRIDE mode for legacy phases (no blocks)', + ); + }); + + test('Step 5 auditor constraint varies by mode', () => { + assert.ok( + (src.includes('Verify mitigations') || src.includes('verify mitigations')) && + (src.includes('Retroactive') || src.includes('retroactive')), + 'Step 5 does not distinguish planned vs retroactive-STRIDE auditor constraint', + ); + }); +});