diff --git a/.planning/phases/18-campaign-grouping-phishing-analysis-api/18-05-PLAN.md b/.planning/phases/18-campaign-grouping-phishing-analysis-api/18-05-PLAN.md new file mode 100644 index 0000000..bca0671 --- /dev/null +++ b/.planning/phases/18-campaign-grouping-phishing-analysis-api/18-05-PLAN.md @@ -0,0 +1,243 @@ +--- +phase: 18-campaign-grouping-phishing-analysis-api +plan: 05 +type: execute +wave: 1 +depends_on: ["18-01", "18-04"] +files_modified: + - lib/services/campaign-grouping-service.ts + - lib/services/campaign-grouping-service.test.ts +autonomous: false +gap_closure: true +requirements: [CAMP-01, CAMP-02] +tags: [postgres, vitest, campaign-grouping, phishing, idempotency] + +must_haves: + truths: + - "Re-analyzing a single-report campaign (no sibling report exists yet) returns the report's OWN existing campaign_id with created:false — no duplicate campaigns row with the same campaign_key is ever created (CR-03 closed)." + - "On that re-analyze, the report's existing campaign report_count is unchanged (no increment) and no second campaigns row is inserted and the report is not re-linked away from its valid campaign." + - "A report that gains a genuinely BETTER sibling match on re-analyze still upgrades to that sibling's campaign (D-08 preserved) — the own-campaign guard runs only when NO sibling matched, so it never suppresses a legitimate upgrade." + - "First-time grouping of a genuinely new report (no existing campaign_id, no sibling) still creates exactly one new campaign — no regression to the create-new path." + - "(CR-02, bundled) When a report migrates from campaign A to a DIFFERENT existing campaign B on re-analyze, campaign A's report_count is decremented so A no longer shows a stale count for a report it no longer holds." + - "(plan-checker warning, closed) When the own-campaign guard falls through to create-new because the report's signal genuinely diverged from its stored campaign_key (not just 'no sibling found'), the abandoned origin campaign's report_count is also decremented — this is the same abandonment as CR-02's cross-campaign migration, just landing on a brand-new campaign instead of an existing sibling's, so it must not be left stale either." + artifacts: + - path: "lib/services/campaign-grouping-service.ts" + provides: "own-campaign revalidation guard before the create-new fall-through; origin-campaign decrement on cross-campaign migration" + contains: "groupReportIntoCampaign" + - path: "lib/services/campaign-grouping-service.test.ts" + provides: "regression tests for CR-03 (single-report re-analyze), no-regression create-new, D-08 sibling upgrade, and CR-02 origin decrement" + key_links: + - from: "lib/services/campaign-grouping-service.ts groupReportIntoCampaign" + to: "campaigns row for ownReport.campaign_id" + via: "SELECT campaign_key, group_method WHERE id = ownReport.campaign_id, compared against freshly-recomputed current tier keys before falling through to create" + pattern: "SELECT campaign_key" + coverage_caveat: "Mocked-only unit coverage was INSUFFICIENT last time — the 18-04 mocked regression test passed while CR-03 shipped and reproduced live in production. This codebase has NO real-Postgres integration test harness (all tests mock postgresClient; vitest include is lib/**/*.test.ts). Therefore the automated tests here are still mocked, and a BLOCKING human live-verification checkpoint (Task 3) is REQUIRED to prove no-duplicate-on-reanalyze against a real database before this gap is considered closed." +--- + + +Close CR-03 (BLOCKING): re-analyzing a single-report campaign — the common case for any brand-new phishing lure before a second person reports it — creates a DUPLICATE `campaigns` row with the identical `campaign_key` instead of reusing the report's own existing campaign. Confirmed live against production (`POST /api/phishing/tickets/627088/analyze` run twice → two campaigns with the same `campaign_key`). + +Root cause: all three tiers in `groupReportIntoCampaign` search only for SIBLING reports that already have a campaign (`r.id != $x` self-exclusion on every tier query). None check whether the calling report's OWN current campaign already satisfies its freshly-computed tier key. For a one-report campaign, every tier returns zero rows, `matchCampaignId` stays null, and control falls through to "create new campaign", abandoning the report's still-valid campaign. The 18-04 no-op guard (`matchCampaignId === ownReport.campaign_id`) can never fire in this path because it only runs when a sibling WAS found. + +This plan also bundles CR-02 (companion, lower-severity): a genuine cross-campaign migration (report moves from campaign A to a different existing campaign B) never decrements A's `report_count`, leaving A permanently stale. Both stem from the same missing "check my own existing campaign" step; the CR-02 decrement is a small, well-understood, closely-coupled addition and is bundled here per the gap author's guidance. Per plan-checker review, the CR-02 decrement is applied in BOTH places a report can abandon its origin campaign: the existing sibling-migration branch, and Task 1's own signal-diverged create-new fall-through (a new campaign, not a sibling's, but the same abandonment). + +Purpose: restore the core CAMP-01/CAMP-02 guarantee — "never create a second campaign for the same key" — and keep `report_count` accurate under repeat `/analyze`, which Phase 19/20 will rely on for classification/triage weighting. +Output: a revalidation guard + origin decrement in `campaign-grouping-service.ts`, mocked regression tests that stage the exact previously-missed scenarios, and a live human verification against a real database. + + + +@$HOME/.claude/get-shit-done/workflows/execute-plan.md +@$HOME/.claude/get-shit-done/templates/summary.md + + + +@.planning/PROJECT.md +@.planning/ROADMAP.md +@.planning/STATE.md + +@.planning/phases/18-campaign-grouping-phishing-analysis-api/18-CONTEXT.md +@.planning/phases/18-campaign-grouping-phishing-analysis-api/18-VERIFICATION.md +@lib/services/campaign-grouping-service.ts +@lib/services/campaign-grouping-service.test.ts + + + + +From lib/services/campaign-grouping-service.ts: + export interface GroupReportResult { + campaignId: string; + groupMethod: 'message_id' | 'attachment_or_url' | 'sender_subject_client'; + created: boolean; + } + + interface OwnReportRow { // already includes campaign_id: string | null (added by 18-04) + title, requester_contact_id, company_id, created_at, campaign_id + } + + // Pure key builders already exist and are used in the create-new block: + computeTier1Key(messageId): string | null // `message_id:` or null + computeTier2Key(hashes, domains, subj, sender): string | null // `attachment_or_url:...` or null + computeTier3Key(contactId, subj, companyId): string | null // `sender_subject_client:...` or null + // Fallback key when no signal exists: `report:` + +campaigns table columns (migration 097): id, campaign_key, group_method, first_seen_at, last_seen_at, report_count, status. + +Test mock discipline (lib/services/campaign-grouping-service.test.ts): the fake transaction client routes each query to staged rows by a DISTINGUISHING SQL SUBSTRING (e.g. `WHERE m.message_id = $1`, `BETWEEN $4::timestamptz`, `UPDATE campaigns`, `INSERT INTO campaigns`). Any NEW query added to the service MUST have a matching new branch in `makeClient()` or the test throws `Unstaged query in test mock`. + + + + + + + Task 1: Own-campaign revalidation guard before create-new (CR-03 — BLOCKING) + lib/services/campaign-grouping-service.ts, lib/services/campaign-grouping-service.test.ts + + - lib/services/campaign-grouping-service.ts lines 320-395 (the match branch at 324-346 and the create-new fall-through at 348-394) — this is exactly where the guard is inserted. + - lib/services/campaign-grouping-service.ts lines 93-119 (computeTier1Key/computeTier2Key/computeTier3Key — reused, not reimplemented). + - lib/services/campaign-grouping-service.test.ts lines 79-117 (makeClient SQL-substring router) and 337-362 (the 18-04 regression test that PASSED yet missed CR-03 — the new tests must cover the single-report path it did not stage). + - .planning/phases/18-campaign-grouping-phishing-analysis-api/18-VERIFICATION.md lines 127-141 (the CR-03 section — authoritative bug description). + + + - Test A (CR-03 core, single-report re-analyze, must FAIL before the fix): stage ownReport with campaign_id='campaign-own', an ownMessage with a message_id, tier1/tier2Candidates/tier3 all empty (no sibling exists), and a NEW staged query returning the report's own campaign row {campaign_key: 'message_id:', group_method: 'message_id'}. Assert result === {campaignId:'campaign-own', groupMethod:'message_id', created:false}; assert ZERO `INSERT INTO campaigns`, ZERO `UPDATE campaigns`, ZERO `UPDATE reports SET campaign_id`. + - Test B (CR-03 Tier-3-only variant): stage ownReport with campaign_id='campaign-own', ownMessage empty, tier3 empty (no sibling), own campaign row {campaign_key:'sender_subject_client:5:invoice alert:10', group_method:'sender_subject_client'}. Assert existing campaign returned, created:false, zero mutating calls. + - Test C (no-regression, genuinely new report): stage ownReport with campaign_id=null, all tiers empty. Assert exactly one `INSERT INTO campaigns`, created:true, zero `UPDATE campaigns` — the own-campaign guard must NOT run when campaign_id is null. + - Test D (signal genuinely diverged): stage ownReport campaign_id='campaign-own', all tiers empty, own campaign row whose campaign_key matches NONE of the freshly-computed current keys (e.g. stored 'sender_subject_client:5:old subject:10' but current subject differs). Assert it falls through to create a new campaign (created:true) — the guard only reuses when the stored key still matches a current tier key. (The origin-decrement for this fall-through is added in Task 2's Test H, which reuses this same staged scenario — Task 1 only needs to prove the create-new fall-through itself still fires correctly here.) + + + Add the CR-03 revalidation guard in groupReportIntoCampaign. First refactor so the three key builders run ONCE before both the own-campaign check and the existing create-new block: compute tier1Key from ownMessage.message_id, tier2Key from ownAttachmentHashes/ownUrlDomains/normalizedSubject/ownSenderValue, tier3Key from ownReport fields, and the fallback `report:${reportId}` — reusing computeTier1Key/computeTier2Key/computeTier3Key. Build a `currentKeys` string array from the non-null values of [tier1Key, tier2Key, tier3Key, `report:${reportId}`]. + + Then, AFTER the existing `if (matchCampaignId && matchGroupMethod)` migration branch and BEFORE the create-new INSERT, insert a new guard that runs only when `matchCampaignId` is null (no sibling matched) AND `ownReport.campaign_id` is non-null: SELECT campaign_key, group_method FROM campaigns WHERE id = ownReport.campaign_id (parameterized, `$1`). If a row is returned AND its campaign_key is included in `currentKeys`, the report is still validly linked to its own campaign — return { campaignId: ownReport.campaign_id, groupMethod: (the stored group_method cast to GroupReportResult['groupMethod']), created: false } WITHOUT any INSERT/UPDATE. If no row is returned (campaign was deleted) or the stored campaign_key matches none of currentKeys (the report's signal genuinely diverged), fall through to the existing create-new logic unchanged. + + Reuse the tierNKey variables already computed for the create-new block (do not compute them twice). Add a comment referencing CR-03 and why the guard sits outside the sibling-match branch. Do NOT weaken the existing `r.id != $x` self-exclusion on any tier query and do NOT change the sibling-match/migration branch in this task (CR-02 is Task 2). + + In the test file, add a new branch to makeClient() that routes the new own-campaign SELECT (distinguishing substring `SELECT campaign_key` / `FROM campaigns` + `WHERE id = $1`) to a staged `ownCampaign` rows array on the MockRows interface. Add Tests A-D from the behavior block. Follow the existing SQL-substring routing and callsContaining() assertion style exactly. + + TDD: write Tests A and B first, run vitest, confirm they FAIL (the current code creates a duplicate campaign / falls through to INSERT), then implement the guard, then confirm all tests green. + + + npx vitest run lib/services/campaign-grouping-service.test.ts + npx tsc --noEmit --pretty + + + - New Tests A-D exist and pass; Tests A/B demonstrably fail against the pre-fix code (verified during RED). + - Single-report re-analyze returns the report's own existing campaign_id with created:false and issues zero INSERT/UPDATE calls. + - campaign_id=null path still creates exactly one campaign (no regression). + - Signal-diverged path still falls through to create. + - `npx tsc --noEmit --pretty` exits 0; full suite (existing 17 + new tests) green. + + Single-report re-analyze returns the report's own existing campaign_id with created:false and zero mutating queries; no duplicate campaigns row is ever created for the same campaign_key; create-new and signal-diverged paths unregressed; full suite + tsc green. + + + + Task 2: Decrement origin campaign report_count on cross-campaign migration AND on signal-diverged create-new (CR-02 + plan-checker-flagged divergence case) + D-08 upgrade regression + lib/services/campaign-grouping-service.ts, lib/services/campaign-grouping-service.test.ts + + - lib/services/campaign-grouping-service.ts lines 324-346 (the sibling-match/migration branch — where a report is re-linked from ownReport.campaign_id to a different matchCampaignId without decrementing the origin). + - .planning/phases/18-campaign-grouping-phishing-analysis-api/18-VERIFICATION.md lines 48-66 (CR-02 Escalation — the authoritative description and the intended D-08 upgrade flow it must not break). + - 18-CONTEXT.md D-08 (lines ~107-115) — the /analyze route intentionally allows upgrading a Tier-3-only grouping to a Tier-1 sibling match; that upgrade MUST keep working. + - Task 1's create-new fall-through (both "no own-campaign row found" and "stored campaign_key matches none of currentKeys" branches) — this is the SECOND place a report can be abandoned from its origin campaign, and it needs the same decrement added here. + + + - Test E (CR-02 migration decrements origin): stage ownReport with campaign_id='campaign-A', a sibling match via Tier 1 or Tier 3 resolving to a DIFFERENT 'campaign-B'. Assert: one `UPDATE campaigns` incrementing B (report_count+1) AND one `UPDATE campaigns` decrementing A (report_count-1); one `UPDATE reports SET campaign_id` to B; created:false; result.campaignId==='campaign-B'. + - Test F (D-08 upgrade preserved, no origin when none): stage ownReport with campaign_id=null (never grouped) and a sibling Tier 1 match to 'campaign-B'. Assert increment of B only, NO decrement UPDATE (there is no origin to decrement), report re-linked to B, created:false. + - Test G (same-campaign re-match still a pure no-op): the existing 18-04 scenario (ownReport.campaign_id === matchCampaignId) still returns created:false with ZERO UPDATE campaigns / INSERT / re-link — the CR-02 decrement must NOT fire when the match resolves back to the report's own campaign. + - Test H (signal-diverged create-new also decrements origin — closes the plan-checker warning): reuse Task 1's Test D staging (ownReport.campaign_id='campaign-A', all tiers empty, own campaign row whose campaign_key matches none of currentKeys). Assert: one `INSERT INTO campaigns` (new campaign, created:true) AND one `UPDATE campaigns` decrementing campaign-A (report_count-1); report re-linked to the new campaign id. + + + In the sibling-match/migration branch (the code path where `matchCampaignId && matchGroupMethod` is true and `matchCampaignId !== ownReport.campaign_id`): after incrementing matchCampaignId's report_count/last_seen_at and re-linking the report, add — only when `ownReport.campaign_id` is non-null — an `UPDATE campaigns SET report_count = report_count - 1, updated_at = NOW() WHERE id = $1` against `ownReport.campaign_id` to correct the origin's now-stale count. Keep the existing `matchCampaignId === ownReport.campaign_id` early-return no-op AHEAD of the increment/decrement so the same-campaign case still issues zero mutations. + + ALSO apply the identical decrement in Task 1's create-new fall-through: both when no own-campaign row is found for `ownReport.campaign_id`, and when the stored `campaign_key` matches none of `currentKeys` (the signal-diverged case) — whenever `ownReport.campaign_id` is non-null and control is about to INSERT a brand-new campaign, decrement the origin first. This is the same "abandoning an origin campaign" situation as the migration branch, just landing on a new row instead of an existing sibling's; leaving it undecremented would reproduce the same class of staleness the CR-02 fix exists to close. Factor the decrement into one small shared helper/inline block if that reads cleanly, or duplicate the single UPDATE statement — either is fine, just don't skip it in either location. + + Scope note (explicit): DECREMENT ONLY — do NOT delete the origin campaign even if its report_count reaches 0. Deleting rows introduces FK-cascade and detail-route risk that is out of scope for this gap fix; an empty (report_count=0) campaign is a strictly lesser problem than a stale count and can be filtered/cleaned in a later phase. Record this scoping in a code comment referencing CR-02. + + The origin decrement UPDATE uses the SAME `UPDATE campaigns` SQL prefix as the increment/create paths, so the test router already matches it — Tests E/F/G/H distinguish by inspecting the SQL body (`report_count + 1` vs `report_count - 1`) and bound params, following the existing callsContaining() pattern. Add Tests E-H. + + TDD: write Test E first, confirm it FAILS (no decrement today), implement the migration-branch decrement, confirm green. Then write Test H, confirm it FAILS (create-new fall-through still doesn't decrement), implement the create-new-path decrement, confirm green. Then confirm F and G pass (guard against over-firing). + + + npx vitest run lib/services/campaign-grouping-service.test.ts + npx tsc --noEmit --pretty + + + - Cross-campaign migration decrements the origin campaign's report_count exactly once; the destination is incremented exactly once. + - Signal-diverged create-new ALSO decrements the origin campaign's report_count exactly once (Test H) — this closes the plan-checker's warning that Task 1 alone would leave this path stale. + - No decrement fires when there is no origin (campaign_id=null) or when the match resolves to the report's own campaign. + - D-08 sibling upgrade path (Tier-3-only → Tier-1 sibling) still re-links correctly. + - Full test suite green; `npx tsc --noEmit --pretty` exits 0. + + Cross-campaign migration AND signal-diverged create-new both decrement the origin campaign's report_count exactly once; the decrement never fires with a null origin or a same-campaign re-match; D-08 upgrade preserved; suite + tsc green. + + + + Task 3: Live database verification of no-duplicate-on-reanalyze (mocked coverage was insufficient last time) + none (live verification only — no file changes) + + - .planning/phases/18-campaign-grouping-phishing-analysis-api/18-VERIFICATION.md lines 127-141 (CR-03 reproduction steps used against production — mirror them, then clean up). + + + Tasks 1-2 add an own-campaign revalidation guard (CR-03) and an origin-count decrement (CR-02) to `groupReportIntoCampaign`, covered by new MOCKED vitest regression tests. Mocked-only coverage is explicitly NOT sufficient to close this gap: the 18-04 mocked regression test passed while CR-03 shipped and reproduced live in production. This codebase has no real-Postgres integration harness, so a human must confirm the fix against an actual database. + + + This is a blocking human-verification checkpoint — no code changes. The executor pauses here and the human performs the live database verification described in how-to-verify, then reports the outcome via the resume signal. Do not auto-approve; the legitimacy of the fix depends on a real Postgres round-trip because mocked-only coverage missed this exact bug once. + + + Run this worktree's code against a real database — prefer a local dev instance (`npm run dev`, port 3100) pointed at a dev/test Postgres. If only production is available, use a known throwaway phishing ticket and CLEAN UP afterward (delete the test campaign(s), the report, its message/indicator rows), exactly as the verifier did for ticket 627088. + + 1. Pick a phishing ticket that currently has NO campaign (or create/reset test data for one). Call `POST /api/phishing/tickets/{ticket_id}/analyze` with an admin session cookie. Note the returned campaignId (call it C1) and that `created:true`. + 2. Immediately call the IDENTICAL `POST /api/phishing/tickets/{ticket_id}/analyze` again (same ticket, no new sibling report). Expected: response returns the SAME campaignId C1 with `created:false` — NOT a new campaign id, NOT created:true. + 3. Query the database directly: `SELECT id, campaign_key, report_count FROM campaigns WHERE campaign_key = (SELECT campaign_key FROM campaigns WHERE id = '');` Expected: exactly ONE row (no duplicate campaign_key), report_count unchanged from step 1 (not incremented), and `reports.campaign_id` for the ticket still points at C1. + 4. (D-08 spot-check, optional but recommended) If two tickets share a Message-ID, confirm that /analyze on the second one still links both into one campaign and the first campaign's count does not go stale. + 5. Clean up any test rows created (campaigns, report, messages, indicators) so the database returns to its pre-test state. + + Confirm: step 2 returned the same id with created:false, and step 3 shows exactly one campaigns row for that key with a stable report_count. + + + A second consecutive /analyze on the same single-report ticket returns the same campaignId with created:false, and a direct DB query shows exactly one campaigns row for that campaign_key with an unchanged report_count. + + + - A second consecutive /analyze on the same single-report ticket returns the same campaignId with created:false. + - Direct DB query shows exactly one campaigns row for that campaign_key (no duplicate) and an unchanged report_count. + - No orphaned/abandoned campaign with a stale report_count remains for the tested ticket. + - Any test data created is cleaned up. + + Live re-analyze of a single-report ticket produced no duplicate campaign and a stable report_count, confirmed by a direct database query; human typed "approved". + Type "approved" once the live re-analyze produces no duplicate campaign and a stable report_count, or describe the observed divergence. + + + + + +## Trust Boundaries + +| Boundary | Description | +|----------|-------------| +| operator → POST /api/phishing/tickets/{id}/analyze | Authenticated (requirePermission('phishing','analyze')) request repeatedly triggers grouping; already gated upstream, unchanged this plan | +| service → campaigns/reports tables | groupReportIntoCampaign writes campaign rows inside a transaction; the defect under fix is data-integrity, not access | + +## STRIDE Threat Register + +| Threat ID | Category | Component | Disposition | Mitigation Plan | +|-----------|----------|-----------|-------------|-----------------| +| T-18-01 | Denial of Service / data integrity | groupReportIntoCampaign create-new fall-through | mitigate | CR-03 fix: revalidate the report's own existing campaign before creating a new one, so repeat /analyze can no longer spawn unbounded duplicate campaigns rows sharing one campaign_key | +| T-18-02 | Tampering (integrity) | campaigns.report_count | mitigate | CR-02 fix: decrement the origin campaign on cross-campaign migration so report_count is not left stale/inflated | +| T-18-03 | Tampering | concurrent find-or-create without UNIQUE constraint on campaign_key | accept | Pre-existing (IN-03, carried forward); out of scope for this gap fix. Grouping already runs inside postgresClient.transaction(); a UNIQUE constraint + row-lock hardening is tracked separately, not introduced here | +| T-18-SC | Tampering | npm/pip/cargo installs | N/A | No package installs in this plan — pure edits to an existing service + test file | + + + +- `npx vitest run lib/services/campaign-grouping-service.test.ts` — all existing tests plus new CR-03 (A-D) and CR-02 (E-H) regression tests pass; Tests A/B, E, and H demonstrably failed against pre-fix code during RED. +- `npx tsc --noEmit --pretty` exits 0. +- Live human verification (Task 3): a second consecutive /analyze on a single-report ticket returns the same campaignId with created:false, and a direct DB query shows exactly one campaigns row for that campaign_key with an unchanged report_count. +- Scope confirmation: WR-01 (Tier 2 missing ORDER BY), WR-04/WR-05, and IN-* findings from 18-REVIEW.md are NOT addressed here (out of scope for this gap-closure plan). + + + +- CR-03 closed: re-analyzing a single-report campaign reuses the report's own existing campaign (created:false) and never creates a duplicate campaigns row with the same campaign_key — proven by mocked regression tests AND a live database check. +- CR-02 closed: cross-campaign migration decrements the origin campaign's report_count, AND the signal-diverged create-new fall-through does the same (no abandoned origin is ever left stale, whether the report lands on an existing sibling's campaign or a brand-new one). +- D-08 preserved: a report with a genuinely better sibling match still upgrades to that sibling's campaign; the own-campaign guard runs only when no sibling matched. +- No regression: first-time grouping of a new report still creates exactly one campaign; the 18-04 same-campaign re-match no-op still fires with zero mutations. + + + +Create `.planning/phases/18-campaign-grouping-phishing-analysis-api/18-05-SUMMARY.md` when done. +