From a48c0583bdec19a2d2fc413dff9af3985301eec8 Mon Sep 17 00:00:00 2001 From: lorentz Date: Wed, 15 Jul 2026 22:44:19 -0400 Subject: [PATCH] test(18): persist human verification items as UAT --- .planning/ROADMAP.md | 2 +- .../18-HUMAN-UAT.md | 36 ++++ .../18-VERIFICATION.md | 184 +++++++++--------- 3 files changed, 129 insertions(+), 93 deletions(-) create mode 100644 .planning/phases/18-campaign-grouping-phishing-analysis-api/18-HUMAN-UAT.md diff --git a/.planning/ROADMAP.md b/.planning/ROADMAP.md index 2b0dce2..0d70f20 100644 --- a/.planning/ROADMAP.md +++ b/.planning/ROADMAP.md @@ -358,7 +358,7 @@ summarizes classification, blast radius, and recommended/approved remediation st - [x] **Phase 15: Data Model, Detection & Ticket Evidence** — New phishing schema (migration 097) + idempotent Autotask ticket scanner + base ticket evidence capture (completed 2026-07-15) - [x] **Phase 16: EML/MIME Evidence Parser** — Pure RFC822/MIME parser: `.eml` selection (`rfc.eml` over `OriginatingEmail.eml`), normalized headers/URLs/attachments, sanitized body preview, synthetic-fixture tests (completed 2026-07-15) - [x] **Phase 17: Mimecast Blast Radius Lookup** — Blast-radius abstraction with graceful `unavailable` degradation when Mimecast isn't configured (completed 2026-07-15) -- [x] **Phase 18: Campaign Grouping & Phishing Analysis API** — Message-ID-first dedupe/grouping, on-demand single-ticket analysis, and the first `/api/phishing/*` routes with the ACCESS-01 auth convention (gaps found in verification 2026-07-15 — see 18-VERIFICATION.md) (completed 2026-07-16) +- [ ] **Phase 18: Campaign Grouping & Phishing Analysis API** — Message-ID-first dedupe/grouping, on-demand single-ticket analysis, and the first `/api/phishing/*` routes with the ACCESS-01 auth convention (5/5 truths verified 2026-07-16, human verification pending — see 18-VERIFICATION.md) - [ ] **Phase 19: Classification Engine** — Deterministic SPAM/UNWANTED/THREAT rule classifier over bounded structured evidence, KnowBe4-simulation guard, (re-)trigger API - [ ] **Phase 20: Remediation, Approval & Audit Safety** — Proposed-only remediation actions, approve/remediate/mark-false-positive APIs, idempotent re-run, full audit trail - [ ] **Phase 21: Autotask Triage Note** — Sanitized internal triage note posted via existing safe note-write path, or returned via API if no such path exists diff --git a/.planning/phases/18-campaign-grouping-phishing-analysis-api/18-HUMAN-UAT.md b/.planning/phases/18-campaign-grouping-phishing-analysis-api/18-HUMAN-UAT.md new file mode 100644 index 0000000..696340b --- /dev/null +++ b/.planning/phases/18-campaign-grouping-phishing-analysis-api/18-HUMAN-UAT.md @@ -0,0 +1,36 @@ +--- +status: partial +phase: 18-campaign-grouping-phishing-analysis-api +source: [18-VERIFICATION.md] +started: 2026-07-16T02:44:01Z +updated: 2026-07-16T02:44:01Z +--- + +## Current Test + +[awaiting human testing] + +## Tests + +### 1. On-demand analyze route — auth + validation behavior +expected: `curl -X POST http://localhost:3100/api/phishing/tickets/{knownPhishingTicketId}/analyze` with (a) valid admin session cookie → 200 with `{reportId,campaignId,groupMethod,created}`; (b) no cookie → 401; (c) user-role session cookie → 403; (d) non-numeric `ticket_id` → 400 +result: [pending] + +### 2. Campaign list + detail routes — auth + shape + error handling (incl. fixed `?status=` filter) +expected: `curl http://localhost:3100/api/phishing/campaigns`, `.../campaigns?status=open`, and `.../campaigns/{id}` with admin cookie → 200 camelCase paginated list / nested detail (status filter now works, previously 500'd); no cookie → 401; malformed id → 400; absent-but-well-formed UUID → 404 +result: [pending] + +### 3. CR-02 escalation — scoping decision (policy call, not a runtime test) +expected: A developer decision on record — either accept the cross-campaign migration `report_count` staleness (CR-02) as a documented limitation via an `overrides:` entry in 18-VERIFICATION.md, or open a follow-up gap-closure plan targeting it before Phase 19/20 depend on `report_count` for weighting. Full reasoning in 18-VERIFICATION.md's "CR-02 Escalation" section. +result: [pending] + +## Summary + +total: 3 +passed: 0 +issues: 0 +pending: 3 +skipped: 0 +blocked: 0 + +## Gaps diff --git a/.planning/phases/18-campaign-grouping-phishing-analysis-api/18-VERIFICATION.md b/.planning/phases/18-campaign-grouping-phishing-analysis-api/18-VERIFICATION.md index 65aa0c2..03e1e20 100644 --- a/.planning/phases/18-campaign-grouping-phishing-analysis-api/18-VERIFICATION.md +++ b/.planning/phases/18-campaign-grouping-phishing-analysis-api/18-VERIFICATION.md @@ -1,58 +1,34 @@ --- phase: 18-campaign-grouping-phishing-analysis-api -verified: 2026-07-15T20:05:00Z -status: gaps_found -score: 4/5 must-haves verified +verified: 2026-07-16T02:45:00Z +status: human_needed +score: 5/5 must-haves verified overrides_applied: 0 -gaps: - - truth: "A campaign accumulates additional linked ticket reports over time as duplicates are detected, without corrupting report_count (CAMP-02)" - status: failed - reason: > - groupReportIntoCampaign's own-report query (lib/services/campaign-grouping-service.ts:156-165) - never selects the report's current campaign_id, and the match branch - (lines 323-335) has no check for matchCampaignId === the report's own - existing campaign. Every tier query correctly excludes the report's own - row from candidates (r.id != reportId), but a SIBLING report already in - the same campaign will legitimately satisfy Tier 1/2/3 once a campaign - has 2+ members. POST /api/phishing/tickets/{id}/analyze intentionally - re-runs groupReportIntoCampaign with no skipIfAlreadyGrouped (by design, - to allow Tier-3->Tier-1/2 upgrades), so every repeat /analyze call on a - report belonging to a multi-report campaign re-matches that campaign and - increments report_count again for a report already counted. There is no - upper bound — a double-clicked "re-analyze" or automated retry inflates - the count indefinitely. report_count is the exact field GET - /api/phishing/campaigns and GET /api/phishing/campaigns/{id} surface - verbatim as the campaign's blast-radius signal, so CAMP-03's read API - exposes an unreliable number for any campaign that has been re-analyzed. - Already identified and classified as a blocker in the phase's own code - review (18-REVIEW.md CR-01); confirmed by direct code read during - verification, and confirmed that no test in - campaign-grouping-service.test.ts covers this scenario (the existing - "self-exclusion" test at line 305 only covers the case where NOTHING - else matches, not the case where a sibling report in the same campaign - re-matches). - artifacts: - - path: "lib/services/campaign-grouping-service.ts" - issue: "groupReportIntoCampaign lines 156-165 and 323-334: no short-circuit when a tiered match resolves to the report's own current campaign_id; ownReportRes SELECT does not fetch campaign_id at all" - missing: - - "Select reports.campaign_id in ownReportRes and compare it against matchCampaignId before the UPDATE campaigns / UPDATE reports block; no-op (return existing result) when they're equal" - - "A test staging a tier match whose campaign_id equals the report's own current campaign_id (via a sibling report), asserting UPDATE campaigns is not called a second time" - - "If report-moves-between-campaigns is ever supported, decrement the origin campaign's report_count in the same transaction (currently no code path does this at all)" +re_verification: + previous_status: gaps_found + previous_score: 4/5 + gaps_closed: + - "A campaign accumulates additional linked ticket reports over time as duplicates are detected, without corrupting report_count (CAMP-02) — same-campaign sibling re-match no longer double-increments report_count" + gaps_remaining: [] + regressions: [] human_verification: - test: "curl POST /api/phishing/tickets/{knownPhishingTicketId}/analyze with admin session cookie, then with no cookie, then with a user-role cookie, then with a non-numeric ticket_id" expected: "200 with {reportId,campaignId,groupMethod,created} (admin); 401 (no cookie); 403 (user-role, lacks phishing:analyze); 400 (non-numeric ticket_id)" - why_human: "Both 18-02-SUMMARY.md and the plan's step were explicitly deferred — no executor ran this against a live server with real session cookies; code-level requirePermission('phishing','analyze') gate was confirmed by static read only" - - test: "curl GET /api/phishing/campaigns and GET /api/phishing/campaigns/{id} with admin cookie, no cookie, malformed id, absent id" - expected: "200 camelCase paginated list / nested detail (admin); 401 (no cookie); 400 (malformed UUID); 404 (absent campaign)" - why_human: "18-03-SUMMARY.md explicitly documents this deviation — the shared dev server on port 3100 was running master, not this worktree's code, so the live curl pass was never executed; only grep/tsc-level static verification was performed" + why_human: "Still never executed against a live server with real session cookies (carried forward from initial verification — code unchanged since then, so the deferral still applies); code-level requirePermission('phishing','analyze') gate confirmed by static read only" + - test: "curl GET /api/phishing/campaigns (including ?status=open) and GET /api/phishing/campaigns/{id} with admin cookie, no cookie, malformed id, absent id" + expected: "200 camelCase paginated list / nested detail (admin), including status-filtered requests now that the count-query bind-parameter bug is fixed; 401 (no cookie); 400 (malformed UUID); 404 (absent campaign)" + why_human: "Still never executed against a live server running this worktree's code (carried forward). This item is now MORE important than before the re-verification: the orchestrator's direct fix (commit 650f9b8) to the ?status= 500 has only been confirmed by static read + a standalone Node simulation of the SQL placeholder numbering in this report, not by an actual Postgres round-trip" + - test: "Escalation decision: accept or reject CR-02 (cross-campaign migration leaves the origin campaign's report_count permanently stale) as an in-scope fast-follow vs. a documented, accepted limitation for this phase" + expected: "A human decision on record (either an explicit override added to this file, or a new gap-closure plan targeting CR-02)" + why_human: "This is a policy/scope call, not a testability gap — see 'CR-02 escalation' in Gaps Summary below for the full analysis and why the verifier partially disagrees with the original 'if ever supported' scoping language" --- # Phase 18: Campaign Grouping & Phishing Analysis API Verification Report **Phase Goal:** Duplicate reports of the same phishing/spam campaign are automatically grouped and accumulate over time, and an operator can trigger analysis of a specific ticket or browse campaigns through a properly access-controlled `/api/phishing/*` surface. -**Verified:** 2026-07-15T20:05:00Z -**Status:** gaps_found -**Re-verification:** No — initial verification +**Verified:** 2026-07-16T02:45:00Z +**Status:** human_needed +**Re-verification:** Yes — after gap closure (plan 18-04) plus one orchestrator-applied fix (commit `650f9b8`) surfaced by a follow-up code review ## Goal Achievement @@ -60,72 +36,92 @@ human_verification: | # | Truth | Status | Evidence | |---|-------|--------|----------| -| 1 | Two reports sharing Message-ID are grouped into the same campaign; absent that, attachment-hash/URL-domain+subject+sender+24h; absent that, sender+normalized-subject+client+24h fallback (CAMP-01) | VERIFIED | `lib/services/campaign-grouping-service.ts:184-318` implements Tier 1→2→3 in order, gated on previous tier failing; 16/16 tests pass (`npx vitest run lib/services/campaign-grouping-service.test.ts`), covering Tier 1 short-circuit, Tier 2 hash+URL-domain matching, Tier 3 sender+subject+client matching | -| 2 | A campaign accumulates additional linked reports over time as duplicates are detected, without ever creating a second campaign, and `report_count` stays accurate (CAMP-02) | **FAILED** | First-time grouping is correct (verified by tests) but `groupReportIntoCampaign` double-increments `report_count` whenever a tiered re-match resolves to the report's *own current* campaign via a sibling row — see Gaps below. Confirmed by direct source read (`campaign-grouping-service.ts:156-165, 323-334`) and by the phase's own code review (18-REVIEW.md CR-01, blocker) | -| 3 | `POST /api/phishing/tickets/{ticket_id}/analyze` runs detect→parse→group for one ticket and returns the campaign linkage on demand (DETECT-03) | VERIFIED (code); live behavior not executed | `app/api/phishing/tickets/[ticket_id]/analyze/route.ts` calls `detectPhishingTicket` → `parseAndStoreMessage` → `groupReportIntoCampaign(detection.reportId)` (no skipIfAlreadyGrouped) and returns camelCase `{reportId,campaignId,groupMethod,created}`; validates ticket_id (400), 404 for missing ticket, 400 for non-phishing ticket. `npx tsc --noEmit --pretty` clean. Live curl pass explicitly deferred per 18-02-SUMMARY.md — see Human Verification | -| 4 | `GET /api/phishing/campaigns` lists campaigns (paginated) and `GET /api/phishing/campaigns/{id}` returns full nested detail (reports/messages/indicators/classifications) (CAMP-03) | VERIFIED (code); live behavior not executed | `app/api/phishing/campaigns/route.ts` and `.../[id]/route.ts` both exist, return camelCase shapes matching the plan exactly (verified by direct read); bulk-fetch via `= ANY($1::uuid[])`, UUID validation, 404 handling all present. `npx tsc --noEmit --pretty` clean. Live curl pass explicitly deferred per 18-03-SUMMARY.md | -| 5 | Every `/api/phishing/*` route introduced this phase calls `requireAuth()`/`requirePermission()` and rejects unauthenticated/unauthorized requests with 401/403 (ACCESS-01) | VERIFIED (code); live behavior not executed | All 4 route files (`analyze`, `campaigns`, `campaigns/[id]`) have `const { error } = await requirePermission('phishing', ); if (error) return error;` as their first handler statement (grep-confirmed). `lib/permissions.ts` grants `phishing: ["read","analyze"]` to admin/super-admin, `phishing: ["read"]` to user, and leaves `approve`/`remediate` ungranted to any role — matches D-05/ACCESS-01 foundation exactly | +| 1 | Two reports sharing Message-ID are grouped into the same campaign; absent that, attachment-hash/URL-domain+subject+sender+24h; absent that, sender+normalized-subject+client+24h fallback (CAMP-01) | ✓ VERIFIED | `lib/services/campaign-grouping-service.ts:184-319` — tiered logic unchanged since initial verification. `npx vitest run lib/services/campaign-grouping-service.test.ts` → 17/17 pass (16 original + 1 new regression test, no regressions) | +| 2 | A campaign accumulates additional linked reports over time as duplicates are detected, without ever creating a second campaign for the same key, and `report_count` stays accurate for the common (same-campaign) re-analyze case (CAMP-02) | ✓ VERIFIED (with a scoped, disclosed caveat — see CR-02 escalation below) | `groupReportIntoCampaign` now selects `ownReport.campaign_id` (`campaign-grouping-service.ts:158`) and short-circuits to a no-op (`created:false`, zero UPDATEs) when a tiered match resolves back to the report's own current campaign (`:332-334`). New regression test `campaign-grouping-service.test.ts:337-362` asserts zero `UPDATE campaigns` / `INSERT INTO campaigns` / `UPDATE reports SET campaign_id` calls for exactly the sibling-re-match scenario that was previously double-incrementing. Direct source read confirms the fix; 18-REVIEW.md's re-review (dated 2026-07-16T02:32:12Z, after the fix landed) independently confirms CR-01 fixed. **Caveat:** the fix only covers "match resolves to the report's OWN current campaign" — it does not cover "match resolves to a genuinely different, already-existing campaign" (a report moving from campaign A to campaign B), which leaves campaign A's `report_count` permanently stale in that narrower scenario (CR-02, still open — see escalation below) | +| 3 | `POST /api/phishing/tickets/{ticket_id}/analyze` runs detect→parse→group for one ticket and returns the campaign linkage on demand (DETECT-03) | ✓ VERIFIED (code); live behavior not executed | `app/api/phishing/tickets/[ticket_id]/analyze/route.ts` unchanged since initial verification (confirmed via `git log` — no commits touch this file after `18-02`). Chain intact: `detectPhishingTicket` → `parseAndStoreMessage` → `groupReportIntoCampaign(detection.reportId)`. `npx tsc --noEmit --pretty` exits 0. Live curl pass still deferred — see Human Verification | +| 4 | `GET /api/phishing/campaigns` lists campaigns (paginated) and `GET /api/phishing/campaigns/{id}` returns full nested detail (reports/messages/indicators/classifications) (CAMP-03) | ✓ VERIFIED (code); live behavior not executed | `app/api/phishing/campaigns/route.ts` now clamps `limit` via `Number.isFinite(...) ? rawLimit : 50` + `Math.min(Math.max(...,0),200)` (verified with a standalone Node simulation: `limit=-5`→0, `limit=0`→0, `limit=abc`→50, `limit=9999`→200 — no more unhandled Postgres 500 for negative limits). The pre-existing `?status=` bind-parameter mismatch (the count query reused the list query's `$3` placeholder text against a 1-element params array) is fixed in commit `650f9b8`: the count query now builds its own independent `countParams`/`countFilter` (verified by direct read, lines 53-63, and a standalone simulation confirming `$1` always matches exactly one bound param). `app/api/phishing/campaigns/[id]/route.ts` unchanged since initial verification — UUID validation, 404, bulk-fetch via `ANY($1::uuid[])`, camelCase nested assembly all still present. `npx tsc --noEmit --pretty` exits 0. Live curl pass still deferred — see Human Verification | +| 5 | Every `/api/phishing/*` route introduced this phase calls `requireAuth()`/`requirePermission()` and rejects unauthenticated/unauthorized requests with 401/403 (ACCESS-01) | ✓ VERIFIED (code); live behavior not executed | All 4 route files unchanged since initial verification (`git log` confirms no commits since `18-03`/`18-04` touch the `requirePermission` call sites). `lib/permissions.ts` unchanged: `phishing: ["read","analyze"]` for admin/super-admin, `phishing: ["read"]` for user, `approve`/`remediate` ungranted to any role | -**Score:** 4/5 truths verified (CAMP-02 failed) +**Score:** 5/5 truths verified (CAMP-02 gap from prior verification closed; one narrower, disclosed caveat remains open pending a human scoping decision — see below) + +### CR-02 Escalation (Human Decision Required) + +**What it is:** `groupReportIntoCampaign`'s CR-01 fix only guards the case where a tiered match resolves to the report's own *current* campaign. It does not guard the case where `ownReport.campaign_id` is a *different*, already-existing campaign (a genuine campaign-to-campaign migration). Concretely: report X is Tier-3-grouped into campaign A (`report_count=1`). Later, `/analyze` is re-run on X after `parseAndStoreMessage` populates its message row, and Tier 1 now finds a *different* pre-existing campaign B (formed by another report sharing the same Message-ID). Since `B !== A`, the guard added by 18-04 doesn't trigger: campaign B is correctly incremented and X is correctly re-linked to B, but campaign A's `report_count` is never decremented — A is left permanently showing `report_count=1` with zero actually-linked reports, indefinitely surfaced by `GET /api/phishing/campaigns`. Confirmed present by direct read of `campaign-grouping-service.ts:324-346` (no decrement of `ownReport.campaign_id` anywhere in the fall-through branch) and independently identified in 18-REVIEW.md as CR-02. + +**Why this needs a human call, not just a verifier judgment:** +- The *previous* verification pass explicitly scoped this out, but with conditional language: *"If report-moves-between-campaigns is ever supported, decrement the origin campaign's report_count..."* That premise is not accurate — this migration path **is** supported today, by explicit design (D-08 in `18-CONTEXT.md`: `/analyze` intentionally omits `skipIfAlreadyGrouped` specifically so it can "upgrade an existing Tier-3-only grouping to a Tier-1 match"). Given D-07's stated limitation that the automatic webhook/cron path can only ever reach Tier 3 until a report has been through at least one explicit `/analyze` call, **every** report that later gets `/analyze`'d and has a Tier-1/2-eligible sibling is a candidate for this exact migration — this is not a rare theoretical edge the way D-04's "two campaigns match simultaneously" scenario is. +- Against that: this is a narrower failure mode than the original CR-01 bug (CR-01 fired on *every* repeat `/analyze` call against any multi-report campaign; CR-02 only fires when the tiered match resolves to a *different pre-existing* campaign than the report's current one — it requires two independently-created campaigns for what turns out to be the same underlying lure). The literal REQUIREMENTS.md text for CAMP-02 ("A campaign can accumulate many linked ticket reports and recipients over time as duplicates are detected") is satisfied in the common case; report_count only goes stale on an *abandoned* campaign, not on the campaign an operator is actually looking at for the report they just analyzed. This also matches the project's own documented precedent (`17-CONTEXT.md` D-05, referenced in `18-CONTEXT.md`) of "simple now, documented limitation over silent gap, defer complexity" for exactly this class of rare-but-real cross-entity reconciliation problem. +- The fix itself is well-understood and small (spelled out verbatim in 18-REVIEW.md's CR-02 remediation) — this is a scope/priority call, not a technical blocker. + +**Verifier's recommendation:** Treat as a disclosed, non-blocking limitation for this phase (consistent with, but more precisely reasoned than, the original scoping call), and open a small follow-up plan before Phase 19/20 relies heavily on `report_count` for classification/triage weighting. **Escalating rather than silently accepting** because the original "if ever supported" framing was based on an incorrect premise, and a developer should explicitly confirm this trade-off rather than have it re-inherited unchanged. + +To formally accept this as a documented limitation rather than opening a gap-closure plan, add to this file's frontmatter: +```yaml +overrides: + - must_have: "A campaign accumulates additional linked ticket reports over time... without corrupting report_count (CAMP-02)" + reason: "CR-02 cross-campaign migration decrement gap accepted as a documented limitation, consistent with D-04's rare-edge-case scoping philosophy; tracked for a future fast-follow before Phase 19/20 depend on report_count for weighting" + accepted_by: "" + accepted_at: "" +``` ### Required Artifacts | Artifact | Expected | Status | Details | |----------|----------|--------|---------| -| `lib/services/campaign-grouping-service.ts` | `groupReportIntoCampaign`, `normalizeSubject`, `extractUrlDomain`, `GroupReportResult` exported | ✓ VERIFIED (exists+substantive+wired) but **hollow on repeat-analyze data integrity** | All exports present; tiered logic implemented; called from 3 sites (webhook, sweep, analyze route). Correctness gap: CR-01 (see Gaps) | -| `lib/services/campaign-grouping-service.test.ts` | Unit tests for helpers + mocked-DB grouping | ✓ VERIFIED | 16 tests, all passing; does NOT cover the CR-01 sibling-re-match scenario (only covers "nothing else matches" self-exclusion) | -| `lib/permissions.ts` | `phishing` resource + role grants | ✓ VERIFIED | `grep -c 'phishing:'` = 4; grants match spec exactly | -| `app/api/phishing/tickets/[ticket_id]/analyze/route.ts` | POST on-demand analyze | ✓ VERIFIED | Matches plan exactly; `requirePermission('phishing','analyze')` first line; detect→parse→group chain intact | -| `lib/services/webhook-service.ts` | `groupReportIntoCampaign` wired into `triggerPhishingDetection` | ✓ VERIFIED | `groupReportIntoCampaign(detection.reportId, { skipIfAlreadyGrouped: true })` present at line 493 | -| `lib/services/phishing-sweep-service.ts` | `groupReportIntoCampaign` wired into per-ticket sweep loop | ✓ VERIFIED | Call at line 91, inside the same try/catch as `detectPhishingTicket`, before the catch that increments `result.errors` | -| `app/api/phishing/campaigns/route.ts` | GET paginated list | ✓ VERIFIED, with a minor known bug (WR-02, non-blocking) | `?limit=0` silently becomes 50, `?limit=-5` triggers an unhandled 500 (Postgres `LIMIT must not be negative`) — see 18-REVIEW.md WR-02 | -| `app/api/phishing/campaigns/[id]/route.ts` | GET nested detail | ✓ VERIFIED | UUID validation, 404, bulk-fetch via `ANY($1::uuid[])`, camelCase nested assembly all present and match plan exactly | +| `lib/services/campaign-grouping-service.ts` | `groupReportIntoCampaign`, `normalizeSubject`, `extractUrlDomain`, `GroupReportResult` exported; own-campaign re-match guard | ✓ VERIFIED (exists+substantive+wired) | `OwnReportRow` now includes `campaign_id: string | null` (line 69); own-report SELECT fetches `campaign_id::text` (line 158); guard at lines 332-334. CR-02 caveat noted above but does not regress CAMP-01/first-time-grouping correctness | +| `lib/services/campaign-grouping-service.test.ts` | Unit tests for helpers + mocked-DB grouping, including the CAMP-02/CR-01 regression | ✓ VERIFIED | 17/17 tests pass. New test at lines 337-362 asserts zero mutating calls on sibling re-match. Per 18-REVIEW.md's WR-02 finding, coverage does not yet extend the same-campaign guard check to Tier 1/2 (only Tier 3 tested) nor cover the CR-02 migration path — non-blocking test-coverage gap, not a functional regression | +| `app/api/phishing/campaigns/route.ts` | GET paginated list, clamped `limit`, correct `?status=` filtering on both list and count queries | ✓ VERIFIED | `limit` clamp confirmed via direct read + standalone simulation (lines 31-32); count-query bind fix confirmed via direct read + standalone simulation (lines 53-63, commit `650f9b8`) | +| `app/api/phishing/campaigns/[id]/route.ts` | GET nested detail | ✓ VERIFIED (unchanged, regression-checked) | No commits since `18-03` touch this file; still matches plan | +| `app/api/phishing/tickets/[ticket_id]/analyze/route.ts` | POST on-demand analyze | ✓ VERIFIED (unchanged, regression-checked) | No commits since `18-02` touch this file | +| `lib/services/webhook-service.ts`, `lib/services/phishing-sweep-service.ts` | `groupReportIntoCampaign` wired with `skipIfAlreadyGrouped: true` | ✓ VERIFIED (unchanged, regression-checked) | Not touched by 18-04 or the orchestrator fix; no reason to re-derive from scratch — call sites confirmed present in initial verification and untouched since (`git log` clean for both files after `18-02`) | +| `lib/permissions.ts` | `phishing` resource + role grants | ✓ VERIFIED (unchanged, regression-checked) | `phishing: ["read","analyze","approve","remediate"]` declared; only `read`/`analyze` granted to any role | ### Key Link Verification | From | To | Via | Status | Details | |------|----|-----|--------|---------| -| `campaign-grouping-service.ts` | `postgresClient.transaction` | find-or-create wrapped in one transaction | ✓ WIRED | `postgresClient.transaction(async (client) => {...})` wraps the entire tiered-match + update/insert flow | -| `app/api/phishing/tickets/[ticket_id]/analyze/route.ts` | `detectPhishingTicket → parseAndStoreMessage → groupReportIntoCampaign` | orchestration chain, no skipIfAlreadyGrouped | ✓ WIRED | Confirmed via direct read, matches plan's exact call shape | -| `app/api/phishing/tickets/[ticket_id]/analyze/route.ts` | `requirePermission('phishing','analyze')` | first-line auth gate | ✓ WIRED | Confirmed | -| `app/api/phishing/campaigns/route.ts` + `[id]/route.ts` | `requirePermission('phishing','read')` | first-line auth gate | ✓ WIRED | Confirmed in both files | -| `app/api/phishing/campaigns/[id]/route.ts` | `reports/messages/indicators/classifications` tables | bulk-fetch via `ANY($1::uuid[])` + Map assembly | ✓ WIRED | Confirmed; empty-array short-circuit present for messages/indicators | -| `lib/permissions.ts statement` | `phishing` resource key | `keyof typeof statement` picks up `phishing` | ✓ WIRED | `npx tsc --noEmit --pretty` exits 0, confirming `requirePermission('phishing', ...)` type-checks across all 4 route files | +| `campaign-grouping-service.ts` groupReportIntoCampaign | own-campaign no-op guard | `matchCampaignId === ownReport.campaign_id` before `UPDATE campaigns` | ✓ WIRED | Confirmed at lines 332-334; regression test proves it fires and short-circuits both UPDATEs | +| `app/api/phishing/campaigns/route.ts` list query | count query | independent `countParams`/`countFilter`, not reused from the list query's `statusFilter` | ✓ WIRED | Confirmed at lines 53-63 (commit `650f9b8`); standalone simulation confirms `$1` placeholder always matches exactly 1 bound param regardless of `status` presence | +| `app/api/phishing/campaigns/route.ts` | `Number.isFinite` limit clamp | `Math.min(Math.max(...,0),200)` | ✓ WIRED | Confirmed at line 32; simulation confirms `-5→0`, `0→0`, `abc→50`, `9999→200`, no path reaches `LIMIT` with a negative value | +| `app/api/phishing/tickets/[ticket_id]/analyze/route.ts` | `detectPhishingTicket → parseAndStoreMessage → groupReportIntoCampaign` | orchestration chain, no `skipIfAlreadyGrouped` | ✓ WIRED (unchanged) | Regression-checked, no commits since `18-02` | +| All 4 `/api/phishing/*` route files | `requirePermission('phishing', )` | first-line auth gate | ✓ WIRED (unchanged) | Regression-checked, no commits touch these call sites since `18-01`-`18-03` | ### Behavioral Spot-Checks | Behavior | Command | Result | Status | |----------|---------|--------|--------| -| campaign-grouping-service unit tests | `npx vitest run lib/services/campaign-grouping-service.test.ts` | 16/16 passed | ✓ PASS | +| campaign-grouping-service unit tests (incl. new regression) | `npx vitest run lib/services/campaign-grouping-service.test.ts` | 17/17 passed | ✓ PASS | | Repository-wide type check | `npx tsc --noEmit --pretty` | exits 0, no output | ✓ PASS | -| Live HTTP behavior of the 4 new/modified routes (401/403/200/400/404) | curl against a running dev server | not run — no safe running instance of this worktree's code available | ? SKIP (routed to Human Verification) | +| `limit` clamp logic simulated standalone | `node -e "..."` reproducing the exact clamp expression | `-5→0`, `0→0`, `abc→50`, `9999→200`, `null→50` | ✓ PASS | +| Count-query placeholder numbering simulated standalone | `node -e "..."` reproducing the exact `countParams`/`countFilter` build | `status='open'` → `WHERE status = $1` with `params=['open']`; `status=null` → no WHERE, `params=[]` | ✓ PASS | +| Live HTTP behavior of the 4 routes (401/403/200/400/404, incl. `?status=` filter) | curl against a running dev server | not run — no safe running instance of this worktree's code available | ? SKIP (routed to Human Verification) | -Step 7b note: this phase's routes require live authenticated sessions and real ticket/campaign rows in Postgres; the executor could not safely run these against the shared dev server (per both 18-02 and 18-03 SUMMARY.md's own documented deviations). No server was started as part of this verification per the "do not start servers" constraint. All 4 route files were instead verified through direct source read + `requirePermission` grep + `tsc` type-checking. +Step 7c note: no `scripts/*/tests/probe-*.sh` files exist for this phase and none are referenced by any PLAN/SUMMARY file; probe execution step is not applicable here. ### Requirements Coverage | Requirement | Source Plan | Description | Status | Evidence | |-------------|-------------|--------------|--------|----------| -| CAMP-01 | 18-01, 18-02 | Message-ID → attachment-hash/URL-domain+subject+sender+window → sender+subject+client+window tiered grouping | ✓ SATISFIED | Tiered matching implemented and tested end-to-end for first-time grouping | -| CAMP-02 | 18-01, 18-02 | Campaign accumulates many linked reports over time as duplicates are detected | ✗ BLOCKED | `report_count` accumulation is corrupted on re-run against an already-linked campaign (CR-01) — the exact metric CAMP-02 describes is not reliably accurate | -| CAMP-03 | 18-03 | Operator can list campaigns and view full detail via API | ✓ SATISFIED | Both routes exist, correctly shaped, camelCase, gated; minor unclamped-limit bug (WR-02) does not block the requirement itself | -| DETECT-03 | 18-02 | Operator can trigger analysis of one ticket by ID on demand | ✓ SATISFIED (code); live confirmation pending | Route implements the full orchestration chain correctly at the code level | -| ACCESS-01 | 18-01, 18-02, 18-03 | All `/api/phishing/*` endpoints enforce existing Pulse auth conventions, approve/remediate requiring elevated permission | ✓ SATISFIED | All 4 routes gate on `requirePermission('phishing', ...)`; `approve`/`remediate` declared but ungranted to any role this phase (deferred to Phase 20 by design, matching the requirement's "requiring elevated permission beyond plain read access" — no role can call approve/remediate yet, which is the safe default) | +| CAMP-01 | 18-01, 18-02 | Message-ID → attachment-hash/URL-domain+subject+sender+window → sender+subject+client+window tiered grouping | ✓ SATISFIED | Unchanged, regression-checked; 17/17 tests green | +| CAMP-02 | 18-01, 18-02, 18-04 | Campaign accumulates many linked reports over time as duplicates are detected | ✓ SATISFIED (with disclosed CR-02 caveat requiring a human scoping decision — see escalation above) | CR-01 double-increment fixed and regression-tested; CR-02 cross-campaign migration decrement gap remains open, narrower in scope, flagged for explicit human accept/reject | +| CAMP-03 | 18-03, 18-04 | Operator can list campaigns and view full detail via API | ✓ SATISFIED | Both routes exist, correctly shaped, camelCase, gated; `limit` clamp (WR-02) and `?status=` count-query bind bug (new CR-01) both fixed and verified | +| DETECT-03 | 18-02 | Operator can trigger analysis of one ticket by ID on demand | ✓ SATISFIED (code); live confirmation pending | Unchanged, regression-checked | +| ACCESS-01 | 18-01, 18-02, 18-03 | All `/api/phishing/*` endpoints enforce existing Pulse auth conventions, approve/remediate requiring elevated permission | ✓ SATISFIED | Unchanged, regression-checked | -No orphaned requirements: REQUIREMENTS.md maps exactly CAMP-01, CAMP-02, CAMP-03, DETECT-03, ACCESS-01 to Phase 18, and all five appear in at least one plan's `requirements:` frontmatter. +No orphaned requirements: REQUIREMENTS.md maps exactly CAMP-01, CAMP-02, CAMP-03, DETECT-03, ACCESS-01 to Phase 18, and all five appear in at least one plan's `requirements:` frontmatter (including 18-04's `[CAMP-02, CAMP-03]`). ### Anti-Patterns Found | File | Line | Pattern | Severity | Impact | |------|------|---------|----------|--------| -| `lib/services/campaign-grouping-service.ts` | 156-165, 323-334 | Missing own-campaign short-circuit before incrementing `report_count` | 🛑 Blocker | Corrupts the CAMP-02 accumulation metric on repeat `/analyze` calls (CR-01, carried forward from 18-REVIEW.md) | -| `app/api/phishing/campaigns/route.ts` | 31 | `Math.min(parseInt(...) \|\| 50, 200)` has no lower bound; `limit=0` silently becomes 50, `limit=-5` produces an unhandled 500 | ⚠️ Warning | Non-blocking for CAMP-03's core requirement but a real bug in the shipped read API (18-REVIEW.md WR-02) | -| `app/api/phishing/tickets/[ticket_id]/analyze/route.ts` (calls `parseAndStoreMessage`) | n/a | Repeat `/analyze` calls insert duplicate `messages`/`indicators` rows (no existence check, no `ON CONFLICT`, no unique constraint on `messages.report_id`) | ⚠️ Warning | Downstream Tier 1/2 matching picks an arbitrary duplicate row (18-REVIEW.md WR-03); also means `GET /campaigns/{id}` can surface duplicate message/indicator rows | -| `lib/services/campaign-grouping-service.ts` | 300-312 | Tier 3 query uses `INNER JOIN contacts` for a relationship where nothing from `contacts` is selected/filtered — silently drops candidates whose `requester_contact_id` doesn't resolve | ⚠️ Warning | Understates campaign membership in an edge case (18-REVIEW.md WR-04); read-path detail route correctly uses `LEFT JOIN` for the same relationship | -| `lib/services/campaign-grouping-service.ts` | 270-273 | Tier 2 sender comparison is case-sensitive, no normalization | ⚠️ Warning | Two messages from the same sender with different address casing fail to Tier-2-match (18-REVIEW.md WR-05) | -| `lib/services/campaign-grouping-service.ts`, `webhook-service.ts`, `phishing-sweep-service.ts` | multiple | No serialization (advisory lock) around concurrent campaign matching | ⚠️ Warning | Two reports of the same lure arriving near-simultaneously can create two separate campaigns instead of one (18-REVIEW.md WR-01) — a distinct failure mode from CR-01, not re-verified independently here but confirmed present by reading the same transaction code | +| `lib/services/campaign-grouping-service.ts` | 324-346 | CR-02: cross-campaign migration (report moves from campaign A to a different, already-existing campaign B) never decrements A's `report_count` | ⚠️ Warning — escalated for human decision | Leaves an abandoned campaign's count permanently stale; narrower and less frequent than the now-fixed CR-01, but a real, non-hypothetical gap given D-07/D-08's documented Tier-3-first-then-upgrade flow. See CR-02 Escalation section above | +| `lib/services/campaign-grouping-service.test.ts` | 337-362 | New regression test only exercises the same-campaign guard via Tier 3; doesn't cover Tier 1/2 same-campaign guard nor CR-02 | ℹ️ Info | Coverage gap noted by 18-REVIEW.md's WR-02 (its own, distinct from the WR-02 fixed by 18-04's Task 2); does not indicate a functional regression, only an untested code path | +| `lib/services/campaign-grouping-service.ts` | 241-251 | Tier 2 candidate query has no `ORDER BY` (non-deterministic pick among multiple qualifying siblings) | ⚠️ Warning | Carried forward from 18-REVIEW.md WR-01; pre-existing, not touched by 18-04 or the orchestrator fix | +| `app/api/phishing/campaigns/route.ts` | 32 | `limit<=0` silently returns an empty page rather than falling back to the documented default of 50 | ℹ️ Info | Carried forward from 18-REVIEW.md IN-01; a footgun, not a crash — the original WR-02 (500 on negative limit) is fixed | +| `app/api/phishing/campaigns/route.ts` | 34 | `status` param not validated against known enum values | ℹ️ Info | Carried forward from 18-REVIEW.md IN-02; consistent with project's "no Zod unless it matters" convention | +| `lib/services/campaign-grouping-service.ts` | 125-126 | `campaigns.campaign_key` has no UNIQUE constraint; no row locking around concurrent find-or-create | ⚠️ Warning | Carried forward from initial verification / 18-REVIEW.md IN-03; pre-existing, not touched by this pass | -No `TBD`/`FIXME`/`XXX`/`TODO`/`HACK`/`PLACEHOLDER` markers found in any of the 8 files this phase created/modified. +No `TBD`/`FIXME`/`XXX`/`TODO`/`HACK`/`PLACEHOLDER` markers found in any of the files touched by 18-04 or the orchestrator's direct fix (`lib/services/campaign-grouping-service.ts`, `lib/services/campaign-grouping-service.test.ts`, `app/api/phishing/campaigns/route.ts`). ### Human Verification Required @@ -133,27 +129,31 @@ No `TBD`/`FIXME`/`XXX`/`TODO`/`HACK`/`PLACEHOLDER` markers found in any of the 8 **Test:** `curl -X POST http://localhost:3100/api/phishing/tickets/{knownPhishingTicketId}/analyze` with (a) a valid admin session cookie, (b) no cookie, (c) a user-role session cookie, (d) a non-numeric `ticket_id`. **Expected:** (a) 200 with `{reportId,campaignId,groupMethod,created}`; (b) 401; (c) 403; (d) 400. -**Why human:** Requires a live server running this worktree's code plus real authenticated session cookies for each role — never executed by either executor (18-02-SUMMARY.md explicitly defers this). +**Why human:** Carried forward — code is unchanged since the initial verification, and this was never executed against a live server with real session cookies. -### 2. Campaign list + detail routes — auth + shape + error handling +### 2. Campaign list + detail routes — auth + shape + error handling, now including the fixed `?status=` filter -**Test:** `curl http://localhost:3100/api/phishing/campaigns` and `.../campaigns/{id}` with an admin cookie, no cookie, a malformed id, and an absent-but-well-formed UUID. -**Expected:** 200 camelCase paginated list / nested detail; 401 without a cookie; 400 for malformed id; 404 for absent campaign. -**Why human:** Same constraint — 18-03-SUMMARY.md explicitly documents that the shared dev server on port 3100 was running `master`, not this worktree's new route files, so this was never executed end-to-end. +**Test:** `curl http://localhost:3100/api/phishing/campaigns`, `.../campaigns?status=open`, and `.../campaigns/{id}` with an admin cookie, no cookie, a malformed id, and an absent-but-well-formed UUID. +**Expected:** 200 camelCase paginated list / nested detail (including successful `?status=` filtering, which previously 500'd); 401 without a cookie; 400 for malformed id; 404 for absent campaign. +**Why human:** Carried forward, and now higher-priority than before: the `?status=` fix (commit `650f9b8`) has only been confirmed via static read and a standalone Node simulation of the SQL text/param shapes in this report — never against a real Postgres connection. -### 3. Confirm CR-01 fix (once applied) with a live re-analyze scenario +### 3. CR-02 escalation — scoping decision -**Test:** Create two reports that group into the same campaign, note `report_count`, then call `POST /analyze` again on one of the two reports (which is already linked) and re-check `report_count`. -**Expected (after fix):** `report_count` does not increase on the repeat call. -**Why human:** Requires live Postgres state (two real linked reports) and a running server — cannot be verified via static analysis alone, and the current implementation is expected to FAIL this check until CR-01 is fixed. +**Test:** N/A (not a runtime test) — a developer must explicitly decide whether the cross-campaign migration `report_count` staleness (CR-02) is accepted as a documented limitation for this phase, or whether a small follow-up gap-closure plan should be opened before Phase 19/20 depend on `report_count`. +**Expected:** An explicit decision recorded (either an `overrides:` entry in this file, per the template above, or a new plan targeting CR-02). +**Why human:** Policy/scope trade-off, not something resolvable by static analysis — see the full CR-02 Escalation section above for the verifier's reasoning and partial disagreement with the original "if ever supported" scoping language. ### Gaps Summary -The read/write surface (`POST /analyze`, `GET /campaigns`, `GET /campaigns/{id}`), the auth gating (ACCESS-01), and the first-time tiered-matching logic (CAMP-01) are all genuinely implemented and match the plan precisely — this is not a stub phase. The one blocking gap is a real data-integrity bug in `groupReportIntoCampaign` (CR-01, already flagged by the phase's own code review): when the intentionally-unconditional `/analyze` re-run finds a tiered match that resolves to the report's *own current* campaign (via a sibling report, since the report's own row is excluded but a second/third member of the same campaign is not), the code has no check for "this is the campaign I'm already in" and bumps `report_count` again. Since `report_count` is the exact number both `GET /api/phishing/campaigns` and `GET /api/phishing/campaigns/{id}` surface as the primary campaign-size signal, and re-running `/analyze` on an already-grouped ticket is an explicitly documented, supported use case (not an edge case), this directly undermines CAMP-02's "accumulates... as duplicates are detected" requirement — the accumulation is real, but its count is not trustworthy. This must be fixed (select the report's current `campaign_id` and short-circuit when the match equals it) before this phase can be considered goal-complete; the fix is small and already spelled out in 18-REVIEW.md's CR-01 remediation. +The blocking CAMP-02 gap from the initial verification (CR-01: unbounded `report_count` double-increment on any repeat `/analyze` call against a multi-report campaign) is now closed and regression-tested — this was the single item that previously prevented phase completion, and it is fixed correctly. The orchestrator's direct fix to the pre-existing `?status=` count-query bind-parameter mismatch (commit `650f9b8`, surfaced by the 18-04 gap-closure re-review) is also confirmed correct by direct code read and a standalone simulation of the exact SQL/param construction. -Secondary, non-blocking warnings (unclamped negative `limit`, duplicate `messages`/`indicators` rows on repeat `/analyze`, an inconsistent JOIN type, case-sensitive sender comparison, and a latent concurrent-campaign-creation race) are carried forward from 18-REVIEW.md for visibility but do not by themselves block the phase goal. +One new, narrower finding (CR-02) was surfaced by the same re-review: `groupReportIntoCampaign`'s fix only guards the "match resolves to my own current campaign" case, not the "match resolves to a different, already-existing campaign" case (a genuine cross-campaign migration, which — per D-08 — is an intentional, designed possibility for the `/analyze` route, not a hypothetical). This leaves an abandoned campaign's `report_count` permanently stale in that specific scenario. The verifier does not believe this rises to the same severity as the now-fixed CR-01 (it requires two independently-formed campaigns for the same underlying lure, not just a routine repeat call), and it is consistent with — though more precisely reasoned than — the original verification's scoping decision to defer this class of fix. It is escalated here as a human decision point rather than silently re-accepted, because the original "if ever supported" framing turns out to be factually inaccurate (the migration path is supported today, by design). + +No regressions were found in any of the previously-verified truths, artifacts, or key links — the routes and permission wiring untouched by 18-04/the orchestrator fix remain exactly as they were at the initial verification pass (confirmed via `git log` showing no intervening commits touch those files). + +The remaining blockers to a clean `passed` status are: (1) the two carried-forward live-curl human verification items (never executed against a running server in either verification pass), and (2) the CR-02 scoping decision, which is a policy call rather than a code defect requiring a fix before this report can be finalized. --- -*Verified: 2026-07-15T20:05:00Z* +*Verified: 2026-07-16T02:45:00Z* *Verifier: Claude (gsd-verifier)*