docs(18): create gap-closure plan 18-04
This commit is contained in:
parent
ea71579fb0
commit
20f3e1bbb5
2 changed files with 237 additions and 7 deletions
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -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"
|
||||
---
|
||||
|
||||
<objective>
|
||||
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.
|
||||
</objective>
|
||||
|
||||
<execution_context>
|
||||
@$HOME/.claude/get-shit-done/workflows/execute-plan.md
|
||||
@$HOME/.claude/get-shit-done/templates/summary.md
|
||||
</execution_context>
|
||||
|
||||
<context>
|
||||
@.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
|
||||
|
||||
<interfaces>
|
||||
<!-- Key contracts the executor needs — extracted from the codebase. No exploration required. -->
|
||||
|
||||
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<GroupReportResult | null>;
|
||||
```
|
||||
|
||||
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);
|
||||
```
|
||||
</interfaces>
|
||||
</context>
|
||||
|
||||
<tasks>
|
||||
|
||||
<task type="auto" tdd="true">
|
||||
<name>Task 1: Short-circuit own-campaign re-match in groupReportIntoCampaign + regression test</name>
|
||||
<files>lib/services/campaign-grouping-service.ts, lib/services/campaign-grouping-service.test.ts</files>
|
||||
<read_first>
|
||||
- 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)
|
||||
</read_first>
|
||||
<behavior>
|
||||
- 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).
|
||||
</behavior>
|
||||
<action>
|
||||
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.
|
||||
</action>
|
||||
<verify>
|
||||
<automated>npx vitest run lib/services/campaign-grouping-service.test.ts && npx tsc --noEmit --pretty</automated>
|
||||
</verify>
|
||||
<acceptance_criteria>
|
||||
- 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`.
|
||||
</acceptance_criteria>
|
||||
<done>
|
||||
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.
|
||||
</done>
|
||||
</task>
|
||||
|
||||
<task type="auto">
|
||||
<name>Task 2: Clamp the campaigns list limit query param (WR-02)</name>
|
||||
<files>app/api/phishing/campaigns/route.ts</files>
|
||||
<read_first>
|
||||
- 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)
|
||||
</read_first>
|
||||
<action>
|
||||
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.
|
||||
</action>
|
||||
<verify>
|
||||
<automated>npx tsc --noEmit --pretty && grep -q "Math.max" app/api/phishing/campaigns/route.ts</automated>
|
||||
</verify>
|
||||
<acceptance_criteria>
|
||||
- `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).
|
||||
</acceptance_criteria>
|
||||
<done>
|
||||
GET /api/phishing/campaigns?limit=-5 returns a clamped 200 response instead of an unhandled 500; explicit limit=0 is honored.
|
||||
</done>
|
||||
</task>
|
||||
|
||||
</tasks>
|
||||
|
||||
<threat_model>
|
||||
## 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 |
|
||||
</threat_model>
|
||||
|
||||
<verification>
|
||||
- `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)`.
|
||||
</verification>
|
||||
|
||||
<success_criteria>
|
||||
- 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.
|
||||
</success_criteria>
|
||||
|
||||
<output>
|
||||
Create `.planning/phases/18-campaign-grouping-phishing-analysis-api/18-04-SUMMARY.md` when done.
|
||||
</output>
|
||||
Loading…
Add table
Add a link
Reference in a new issue