From 20f3e1bbb53bf886e637d9bd24d012ccfe78752c Mon Sep 17 00:00:00 2001 From: lorentz Date: Wed, 15 Jul 2026 22:22:17 -0400 Subject: [PATCH] docs(18): create gap-closure plan 18-04 --- .planning/STATE.md | 14 +- .../18-04-PLAN.md | 230 ++++++++++++++++++ 2 files changed, 237 insertions(+), 7 deletions(-) create mode 100644 .planning/phases/18-campaign-grouping-phishing-analysis-api/18-04-PLAN.md diff --git a/.planning/STATE.md b/.planning/STATE.md index 8d10a8e..524654b 100644 --- a/.planning/STATE.md +++ b/.planning/STATE.md @@ -4,13 +4,13 @@ milestone: v3.0 milestone_name: Phishing Triage Automation status: executing stopped_at: Phase 18 planned, 3 plans ready -last_updated: "2026-07-15T23:14:09.528Z" -last_activity: 2026-07-15 -- Phase 18 execution started +last_updated: "2026-07-16T02:22:05.897Z" +last_activity: 2026-07-16 -- Phase 18 planning complete progress: total_phases: 7 completed_phases: 3 - total_plans: 10 - completed_plans: 7 + total_plans: 11 + completed_plans: 10 percent: 43 --- @@ -27,8 +27,8 @@ See: .planning/PROJECT.md (updated 2026-07-14) Phase: 18 (campaign-grouping-phishing-analysis-api) — EXECUTING Plan: 1 of 3 -Status: Executing Phase 18 -Last activity: 2026-07-15 -- Phase 18 execution started +Status: Ready to execute +Last activity: 2026-07-16 -- Phase 18 planning complete Progress: [░░░░░░░░░░] 0% @@ -96,7 +96,7 @@ None yet. ### Blockers/Concerns -None yet. +- 2026-07-15 — Phase 18 gap closure (18-04): decision-coverage gate flagged D-02/D-03/D-04 (grouping-parameter decisions from original discuss-phase) as not literally cited in any plan's `must_haves`/`truths`. Overridden and proceeded — these decisions were already implemented in 18-01 (24h window, subject normalization, no-merge behavior) and independently confirmed correct by both 18-REVIEW.md and 18-VERIFICATION.md. Citation gap only, not an implementation gap. ### Quick Tasks Completed diff --git a/.planning/phases/18-campaign-grouping-phishing-analysis-api/18-04-PLAN.md b/.planning/phases/18-campaign-grouping-phishing-analysis-api/18-04-PLAN.md new file mode 100644 index 0000000..3118eb0 --- /dev/null +++ b/.planning/phases/18-campaign-grouping-phishing-analysis-api/18-04-PLAN.md @@ -0,0 +1,230 @@ +--- +phase: 18-campaign-grouping-phishing-analysis-api +plan: 04 +type: execute +wave: 1 +depends_on: ["18-01", "18-03"] +files_modified: + - lib/services/campaign-grouping-service.ts + - lib/services/campaign-grouping-service.test.ts + - app/api/phishing/campaigns/route.ts +autonomous: true +gap_closure: true +requirements: [CAMP-02, CAMP-03] + +must_haves: + truths: + - "Re-running POST /analyze on a report already linked to a multi-report campaign does NOT increment that campaign's report_count again" + - "First-time grouping and campaign creation continue to work exactly as before (no regression in the 16 existing tests)" + - "GET /api/phishing/campaigns?limit=-5 returns 200 with a clamped limit instead of an unhandled 500" + artifacts: + - path: "lib/services/campaign-grouping-service.ts" + provides: "groupReportIntoCampaign short-circuits when a tiered match resolves to the report's own current campaign_id" + contains: "ownReport.campaign_id" + - path: "lib/services/campaign-grouping-service.test.ts" + provides: "Regression test proving report_count is not double-incremented on sibling re-match" + contains: "report_count" + - path: "app/api/phishing/campaigns/route.ts" + provides: "limit query param clamped to [0, 200] with no falsy-zero fallthrough" + contains: "Math.max" + key_links: + - from: "lib/services/campaign-grouping-service.ts groupReportIntoCampaign" + to: "ownReport.campaign_id short-circuit before UPDATE campaigns" + via: "matchCampaignId === ownReport.campaign_id no-op guard" + pattern: "matchCampaignId === ownReport.campaign_id" +--- + + +Close the single blocking gap from 18-VERIFICATION.md (CAMP-02), independently +flagged as CR-01 in 18-REVIEW.md: `groupReportIntoCampaign` double-increments +`campaigns.report_count` every time `POST /api/phishing/tickets/{id}/analyze` is +re-run on a report that already belongs to a multi-report campaign. Because the +tier queries exclude the report's OWN row (`r.id != $x`) but NOT its sibling +rows, a re-analyze finds a sibling already in the same campaign, treats it as a +fresh match, and bumps `report_count` again — with no upper bound. `report_count` +is the exact blast-radius number surfaced by `GET /api/phishing/campaigns` and +`GET /api/phishing/campaigns/{id}`, so this makes CAMP-03's read API report an +untrustworthy campaign size. + +Also fixes the secondary WR-02 finding (in scope, low risk, same read API): the +`limit` query param in the campaigns list route is not clamped — `limit=0` +silently becomes 50 and `limit=-5` produces an unhandled Postgres 500. + +Purpose: make campaign `report_count` accurate and idempotent under repeat +`/analyze` calls, which is the explicitly supported, documented use case for the +route (Tier-3 → Tier-1/2 upgrade after EML parsing). +Output: fixed grouping service + regression test + clamped limit param. + + + +@$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-VERIFICATION.md +@.planning/phases/18-campaign-grouping-phishing-analysis-api/18-REVIEW.md + + + + +From lib/services/campaign-grouping-service.ts (the function being fixed): +```typescript +export interface GroupReportResult { + campaignId: string; + groupMethod: 'message_id' | 'attachment_or_url' | 'sender_subject_client'; + created: boolean; +} + +// Current OwnReportRow — MISSING campaign_id (the root of the bug) +interface OwnReportRow { + title: string | null; + requester_contact_id: number | null; + company_id: number | null; + created_at: string; +} + +export async function groupReportIntoCampaign( + reportId: string, + opts?: { skipIfAlreadyGrouped?: boolean } +): Promise; +``` + +The buggy code region (lib/services/campaign-grouping-service.ts): +- Lines 156-161: `ownReportRes` SELECT — selects `title, requester_contact_id, company_id, created_at` but NOT `campaign_id`. +- Lines 323-335: the match branch — unconditionally runs `UPDATE campaigns SET report_count = report_count + 1 ...` then `UPDATE reports SET campaign_id = ...`, with no check for `matchCampaignId === ownReport.campaign_id`. + +From the test mock (lib/services/campaign-grouping-service.test.ts): +- The fake client routes the own-report SELECT via `sql.includes('requester_contact_id, company_id, created_at')`. Adding `campaign_id::text AS campaign_id` AFTER `created_at` preserves this substring, so the existing routing keeps working. +- `REPORT_ROW` (line 136) is the shared fixture; existing tests leave `campaign_id` undefined, which stays `!== matchCampaignId`, so they keep incrementing as before (no regression). +- `callsContaining('UPDATE campaigns')` is the existing helper used to assert increment behavior. + +From app/api/phishing/campaigns/route.ts (WR-02 site): +```typescript +// Line 31 — buggy: `|| 50` swallows an explicit 0, and negative values are never floored: +const limit = Math.min(parseInt(url.searchParams.get('limit') ?? '50', 10) || 50, 200); +// Line 32 — offset is already correctly floored, use as the pattern to mirror: +const offset = Math.max(parseInt(url.searchParams.get('offset') ?? '0', 10) || 0, 0); +``` + + + + + + + Task 1: Short-circuit own-campaign re-match in groupReportIntoCampaign + regression test + lib/services/campaign-grouping-service.ts, lib/services/campaign-grouping-service.test.ts + + - lib/services/campaign-grouping-service.ts (the whole file — small, ~390 lines; focus lines 64-69 OwnReportRow, 156-165 own-report SELECT, 320-335 match branch) + - lib/services/campaign-grouping-service.test.ts (the whole file — study makeClient routing at lines 79-117, REPORT_ROW at 136, and the Tier 3 increment test at 143-169 as the template for the new regression test) + - .planning/phases/18-campaign-grouping-phishing-analysis-api/18-REVIEW.md (CR-01 remediation, lines 47-127 — the fix is spelled out there) + + + - Regression (the gap): a report already linked to campaign X whose tiered re-match resolves to campaign X (via a SIBLING report, not its own excluded row) must NOT run `UPDATE campaigns ... report_count + 1` a second time; it returns `{ campaignId: X, groupMethod, created: false }` as a no-op. + - No regression: a report with no current campaign that tier-matches campaign Y (Y !== own campaign_id, including the case where own campaign_id is null/undefined) still increments Y's report_count and links the report — exactly as the existing 16 tests assert. + - No regression: when nothing matches, a new campaign is still created (existing "self-exclusion" test at line 305 stays green). + + + Root cause (per CR-01): `ownReportRes` never selects the report's current `campaign_id`, and the match branch has no guard for the case where the tiered match resolves to the campaign the report is ALREADY in. Since `/analyze` re-runs grouping unconditionally (D-08, by design) and tier queries only exclude the report's own row (not its siblings), any multi-report campaign gets re-matched and double-counted on every repeat call. + + Fix in lib/services/campaign-grouping-service.ts: + 1. Add `campaign_id: string | null` to the `OwnReportRow` interface (after `created_at`). + 2. In the `ownReportRes` SELECT (currently `SELECT title, requester_contact_id, company_id, created_at FROM reports WHERE id = $1`), add `, campaign_id::text AS campaign_id` at the END of the column list — after `created_at` — so the existing test-mock routing substring `requester_contact_id, company_id, created_at` is preserved. Cast to text to match how campaign_id is compared elsewhere (`::text`). + 3. In the match branch (the `if (matchCampaignId && matchGroupMethod)` block, ~line 323), BEFORE the `UPDATE campaigns` call, add a guard: if `matchCampaignId === ownReport.campaign_id`, short-circuit as a no-op — return `{ campaignId: matchCampaignId, groupMethod: matchGroupMethod, created: false }` WITHOUT running either UPDATE. Add a brief comment explaining this is the "already correctly linked, re-run found the same campaign again" no-op that prevents report_count inflation (CR-01/CAMP-02). + 4. Do NOT implement report-moves-between-campaigns / origin-count-decrement — that path does not exist today and is explicitly out of scope for this gap (VERIFICATION.md lists it only as a conditional "if ever supported"). Leave the existing behavior for a genuinely different-campaign match (Y !== own) unchanged. + + Then add a regression test in lib/services/campaign-grouping-service.test.ts modeled on the existing Tier 3 test (lines 143-169): + - Stage `ownReport` as a row that includes `campaign_id: 'campaign-1'` (spread REPORT_ROW and add campaign_id), `ownMessage: []`, and `tier3: [{ campaign_id: 'campaign-1', title: 'Invoice Alert' }]` — i.e. the tiered match resolves to the report's OWN current campaign via a sibling. + - Call `groupReportIntoCampaign('report-self')`. + - Assert the result equals `{ campaignId: 'campaign-1', groupMethod: 'sender_subject_client', created: false }`. + - Assert `callsContaining('UPDATE campaigns')` has length 0 (the critical assertion — report_count NOT bumped a second time). + - Assert `callsContaining('INSERT INTO campaigns')` has length 0 (no new campaign either). + - Optionally also assert `callsContaining('UPDATE reports SET campaign_id')` has length 0 (no redundant re-link). + Name the test to describe the sibling-re-match scenario (e.g. "re-analyzing a report already linked to a campaign that a sibling report re-matches does not increment report_count a second time"). + + Do NOT touch the Tier 1/2/3 self-exclusion logic, the skipIfAlreadyGrouped path, or the new-campaign creation path — they are verified correct. + + + npx vitest run lib/services/campaign-grouping-service.test.ts && npx tsc --noEmit --pretty + + + - All existing tests plus the new regression test pass (17+ tests green). + - The new test fails if the short-circuit guard is removed (it asserts 0 `UPDATE campaigns` calls on a same-campaign sibling re-match). + - `npx tsc --noEmit --pretty` exits 0. + - `OwnReportRow` has a `campaign_id` field and the own-report SELECT fetches `campaign_id::text`. + + + Re-matching the report's own current campaign is a no-op: report_count is not incremented a second time, closing the CAMP-02 gap. First-time grouping and campaign creation are unchanged. + + + + + Task 2: Clamp the campaigns list limit query param (WR-02) + app/api/phishing/campaigns/route.ts + + - app/api/phishing/campaigns/route.ts (whole file — 77 lines; the fix is line 31, mirror the offset handling on line 32) + - .planning/phases/18-campaign-grouping-phishing-analysis-api/18-REVIEW.md (WR-02, lines 151-169 — remediation given verbatim) + + + Fix WR-02 on line 31 of app/api/phishing/campaigns/route.ts. The current + `const limit = Math.min(parseInt(url.searchParams.get('limit') ?? '50', 10) || 50, 200);` + has two bugs: `|| 50` treats an explicit valid `limit=0` as falsy and overrides it with 50, and there is no lower bound so `limit=-5` flows straight into `LIMIT $1` and Postgres raises "LIMIT must not be negative" as an unhandled 500. + + Replace with a parse-then-clamp that floors at 0 and caps at 200, using the same defensive shape as the `offset` line directly below it: + - Parse the raw `limit` param once (default to 50 when absent or non-numeric — use `Number.isFinite` on the parsed value rather than `|| 50` so an explicit 0 survives). + - Clamp with `Math.min(Math.max(parsed, 0), 200)`. + + Do NOT change the `offset`, `status`, pagination response shape, or the auth gate. This is a one-line-region fix. + + + npx tsc --noEmit --pretty && grep -q "Math.max" app/api/phishing/campaigns/route.ts + + + - `limit` is floored at 0 and capped at 200; an explicit `limit=0` is no longer silently replaced by 50; a negative `limit` can no longer reach the SQL. + - `npx tsc --noEmit --pretty` exits 0. + - No other behavior in the route changed (auth gate, offset, status filter, response shape intact). + + + GET /api/phishing/campaigns?limit=-5 returns a clamped 200 response instead of an unhandled 500; explicit limit=0 is honored. + + + + + + +## Trust Boundaries + +| Boundary | Description | +|----------|-------------| +| client → GET /api/phishing/campaigns | Untrusted `limit`/`offset`/`status` query params cross into a SQL LIMIT/OFFSET | +| repeat /analyze → campaign-grouping-service | Idempotency boundary: the same report re-processed must not corrupt aggregate state | + +## STRIDE Threat Register + +| Threat ID | Category | Component | Disposition | Mitigation Plan | +|-----------|----------|-----------|-------------|-----------------| +| T-18-01 | Tampering | groupReportIntoCampaign report_count aggregate | mitigate | Short-circuit no-op when a tiered match resolves to the report's own current campaign_id (Task 1) — prevents unbounded inflation of the integrity-critical count | +| T-18-02 | Denial of Service | GET /api/phishing/campaigns `limit` param | mitigate | Clamp `limit` to [0,200] so a negative value cannot trigger a Postgres error / unhandled 500 (Task 2) | +| T-18-SC | Tampering | npm/pip/cargo installs | accept | No package installs in this gap-closure plan — no new dependencies added | + + + +- `npx vitest run lib/services/campaign-grouping-service.test.ts` — all tests pass including the new sibling-re-match regression test. +- `npx tsc --noEmit --pretty` — exits 0 across the repo. +- Manual code read confirms `ownReport.campaign_id` guard sits before the `UPDATE campaigns` call in the match branch. +- Manual code read confirms `limit` is clamped with `Math.max(..., 0)` and `Math.min(..., 200)`. + + + +- CAMP-02 gap from 18-VERIFICATION.md is closed: re-analyzing an already-grouped report does not double-increment `report_count`. +- WR-02 read-API bug is closed: negative/zero `limit` handled safely. +- No regression: the existing 16 grouping tests remain green. +- Type check clean. + + + +Create `.planning/phases/18-campaign-grouping-phishing-analysis-api/18-04-SUMMARY.md` when done. +