From cd9dfc395416e6ffae2700e1bce0b36fa8b40f0c Mon Sep 17 00:00:00 2001 From: lorentz Date: Thu, 16 Jul 2026 06:56:03 -0400 Subject: [PATCH] docs(18): update code review after CR-03/CR-02 gap closure --- .../18-REVIEW.md | 289 +++--------------- 1 file changed, 44 insertions(+), 245 deletions(-) diff --git a/.planning/phases/18-campaign-grouping-phishing-analysis-api/18-REVIEW.md b/.planning/phases/18-campaign-grouping-phishing-analysis-api/18-REVIEW.md index 8b5badf..c49bdca 100644 --- a/.planning/phases/18-campaign-grouping-phishing-analysis-api/18-REVIEW.md +++ b/.planning/phases/18-campaign-grouping-phishing-analysis-api/18-REVIEW.md @@ -1,285 +1,84 @@ --- phase: 18-campaign-grouping-phishing-analysis-api -reviewed: 2026-07-16T02:32:12Z +reviewed: 2026-07-16T10:54:20Z depth: standard -files_reviewed: 3 +files_reviewed: 2 files_reviewed_list: - lib/services/campaign-grouping-service.ts - lib/services/campaign-grouping-service.test.ts - - app/api/phishing/campaigns/route.ts findings: - critical: 2 + critical: 0 warning: 2 info: 3 - total: 7 + total: 5 status: issues_found --- -# Phase 18: Code Review Report (re-review, post gap-closure fix 18-04) +# Phase 18: Code Review Report (re-review, supersedes prior 18-REVIEW.md for these 2 files) -**Reviewed:** 2026-07-16T02:32:12Z +**Reviewed:** 2026-07-16T10:54:20Z **Depth:** standard -**Files Reviewed:** 3 -**Status:** issues_found +**Files Reviewed:** 2 +**Status:** issues_found (no Critical findings; both gap-closure fixes confirmed correct) ## Summary -This is a re-review of `campaign-grouping-service.ts`, its test file, and the -`/api/phishing/campaigns` route after plan 18-04's gap-closure fix. Both -originally-flagged issues are **verified fixed**: +Re-reviewed `campaign-grouping-service.ts` and its test file fresh, focused on the two gap-closure fixes from plan 18-05 (CR-03 own-campaign revalidation guard, CR-02 `decrementOriginCampaign` helper) and the shared tier-key-computation refactor that supports both. This review scope is 2 files (`campaign-grouping-service.ts`/`.test.ts`) — the prior `18-REVIEW.md`'s `app/api/phishing/campaigns/route.ts` findings (its own CR-01/CR-02) are out of scope here and not re-verified in this pass; this report replaces the prior document's content for the 2 files listed above only. -- **Prior CR-01 (double-increment on sibling re-match)** — fixed. - `groupReportIntoCampaign` now selects the report's own `campaign_id` - (`campaign-grouping-service.ts:158`) and compares it against - `matchCampaignId` (`:332-334`), short-circuiting with a no-op - `{ created: false }` result when a tier match resolves back to the - report's own current campaign. Verified against the new regression test at - `campaign-grouping-service.test.ts:337-362`, which asserts zero - `UPDATE campaigns`, zero `INSERT INTO campaigns`, and zero - `UPDATE reports` calls for exactly the sibling-re-match scenario the - original finding described. -- **Prior WR-02 (unclamped `limit` → Postgres 500 on negative input)** — - fixed. `route.ts:31-33` now clamps `limit` through `Number.isFinite` + - `Math.max(...,0)` + `Math.min(...,200)`, so a negative, NaN, or oversized - `limit` can no longer reach the SQL `LIMIT` clause unclamped. +**CR-03 (own-campaign revalidation guard, `campaign-grouping-service.ts:399-438`):** traced correctly. Every tier query self-excludes the report's own row, so a single-report campaign (no sibling exists yet) always reaches this block with `matchCampaignId === null`. The guard recomputes `currentKeys` (tier1/tier2/tier3 + a `report:` fallback, always deterministic per report — filtered non-null at `:357-359`) and checks whether the report's *own* stored `campaign_key` is still one of them. If so, it returns the existing campaign with zero mutating queries. Verified against Test A (Tier-1 stored key reused), Test B (Tier-3 stored key reused), Test C (never-grouped, guard correctly doesn't fire), and Test D (signal diverged, correctly falls through to create-new). Hand-traced additional edge cases not in the suite — empty signal falling back to the `report:` key on both sides (always self-consistent since it's derived from `reportId` alone), and a stale/diverged stored key always being excluded from `currentKeys` by construction (so the diverged-fallthrough path can never collide with the abandoned key) — both consistent, no hole found. -However, this pass surfaced **two new, material bugs** in the same code -paths: +**CR-02 (`decrementOriginCampaign`, `:122-138`):** correctly shared by both abandonment sites — the sibling-migration branch (`:393-395`) and the CR-03 signal-diverged fall-through (`:437`) — and correctly gated so it never fires on a same-campaign no-op match (the CR-01 guard at `:372-374` returns before either call site is reached) or on a never-grouped report (both call sites are guarded by `ownReport.campaign_id` being truthy). Verified against Test E (cross-campaign migration: decrement origin + increment destination, exactly one each), Test F (never-grouped upgrade: increment only, no decrement), Test G (same-campaign re-match: no mutation at all — confirms the guard interposes correctly ahead of the decrement), and Test H (signal-diverged create-new: decrement origin + insert new campaign, no increment). All four scenarios traced correctly through the code and match the described production confirmation (ticket 627088 analyzed twice, single `campaigns` row, unchanged `report_count`). -1. A parameter-binding bug in the campaigns list route (unrelated to the - `limit`/`offset` fix) that makes every status-filtered request 500. -2. A gap in the CR-01 fix itself: the added guard only covers "tier match - resolves to the report's own *current* campaign." It does not cover "tier - match resolves to a *different, already-existing* campaign" — which is - the exact Tier-3→Tier-1 upgrade scenario this phase's own docstring (D-08) - describes as expected, and which the **prior review's own suggested fix - explicitly warned about** ("If the intent is also to support a report - *moving* from one campaign to another, the old campaign's `report_count` - must be decremented at the same time — currently there is no code path - that does this at all"). That caveat was not addressed by the 18-04 fix; - the gap is still open today, just narrower than before. - -## Critical Issues - -### CR-01: `/api/phishing/campaigns?status=...` throws a Postgres parameter-binding error (500) on every status-filtered request - -**File:** `app/api/phishing/campaigns/route.ts:36-56` -**Issue:** `statusFilter` is built once, against the parameter list used by -the *first* query (`params = [limit, offset, status]`, so the placeholder is -computed as `$${params.length}` = `$3`): - -```ts -const params: unknown[] = [limit, offset]; -let statusFilter = ''; -if (status) { - params.push(status); - statusFilter = `WHERE status = $${params.length}`; // "WHERE status = $3" -} -``` - -That same `statusFilter` string (still referencing `$3`) is then reused -verbatim for the **total count** query, but with a completely different, -much shorter params array: - -```ts -const totalRes = await postgresClient.query<{ count: string }>( - `SELECT COUNT(*)::text AS count FROM campaigns ${statusFilter}`, - status ? [status] : [] // only one bind value supplied, but the SQL text references $3 -); -``` - -Postgres's extended query protocol requires every placeholder referenced in -the SQL text to have a bound value at that ordinal position. With `status` -set, the total-count query text contains `$3` but only one bind parameter is -supplied — `pg` throws a bind-mismatch error, which is caught by the route's -`catch` block and returned as a `500`. **Every** call to this endpoint with a -`status` filter (e.g. `?status=open`, exactly what a campaigns dashboard -would call by default) fails outright. - -**Fix:** Give the total-count query its own independent placeholder numbering -instead of reusing `statusFilter`: - -```ts -const totalRes = await postgresClient.query<{ count: string }>( - `SELECT COUNT(*)::text AS count FROM campaigns ${status ? 'WHERE status = $1' : ''}`, - status ? [status] : [] -); -``` - -### CR-02: CR-01's same-campaign guard doesn't cover cross-campaign migration — old campaign's `report_count` is never decremented - -**File:** `lib/services/campaign-grouping-service.ts:324-346` -**Issue:** The gap-closure fix added exactly one guard: - -```ts -if (matchCampaignId === ownReport.campaign_id) { - return { campaignId: matchCampaignId, groupMethod: matchGroupMethod, created: false }; -} -``` - -This correctly no-ops when a tier match resolves back to the report's own -*current* campaign (the originally-flagged scenario). But it does nothing -for the case where `ownReport.campaign_id` is non-null and `matchCampaignId` -resolves to a **different, already-existing** campaign. That case is not -hypothetical — it's exactly what the file's own docstring describes as -expected behavior: - -> D-08: ... it may upgrade a Tier-3-only report to Tier 1 after -> `parseAndStoreMessage` has just populated `messages`/`indicators` for the -> first time. - -Concretely: report X is created, Tier 3 matches nothing, and a new campaign -A is created for it (`report_count = 1`, `reports.campaign_id = A`). Later, -`/analyze` is re-run on X after `parseAndStoreMessage` has populated its -`messages`/`indicators` row. Tier 1 now finds a *different*, pre-existing -campaign B (formed by another report sharing the same `message_id`). Since -`B !== A`, the new guard doesn't trigger, and the code falls through to: - -```ts -await client.query( - `UPDATE campaigns SET report_count = report_count + 1, last_seen_at = NOW(), updated_at = NOW() WHERE id = $1`, - [matchCampaignId] // increments B -); -await client.query( - `UPDATE reports SET campaign_id = $1, updated_at = NOW() WHERE id = $2`, - [matchCampaignId, reportId] // moves X from A to B -); -``` - -Campaign A's `report_count` is **never decremented**. A now has -`report_count = 1` with zero actually-linked reports — a permanently -stale/inflated count that the exact endpoint under review -(`GET /api/phishing/campaigns`) surfaces to users indefinitely, with no -reconciliation path anywhere in the codebase. - -This is not a newly-introduced regression — the prior review's suggested fix -for the original bug explicitly flagged this exact risk ("a moved report -also leaves the origin campaign's count permanently inflated") — but the -18-04 fix implemented only the narrower same-campaign guard and left this -caveat unaddressed. It should be treated as still-open, not resolved. - -**Fix:** Decrement the outgoing campaign when the report is moving to a -genuinely different campaign, not just skip when it's the same one: - -```ts -if (matchCampaignId && matchGroupMethod) { - if (matchCampaignId === ownReport.campaign_id) { - return { campaignId: matchCampaignId, groupMethod: matchGroupMethod, created: false }; - } - if (ownReport.campaign_id) { - await client.query( - `UPDATE campaigns - SET report_count = GREATEST(report_count - 1, 0), updated_at = NOW() - WHERE id = $1`, - [ownReport.campaign_id] - ); - } - await client.query( - `UPDATE campaigns - SET report_count = report_count + 1, last_seen_at = NOW(), updated_at = NOW() - WHERE id = $1`, - [matchCampaignId] - ); - await client.query( - `UPDATE reports SET campaign_id = $1, updated_at = NOW() WHERE id = $2`, - [matchCampaignId, reportId] - ); - return { campaignId: matchCampaignId, groupMethod: matchGroupMethod, created: false }; -} -``` -(A campaign left at `report_count = 0` raises a separate follow-up question -— whether to soft-delete/hide it from the list endpoint — but that's a -smaller design decision than leaving the count permanently wrong.) +No Critical-severity defects were introduced by this round's changes. Two Warnings and three Info-level items below are worth addressing for robustness/precision but do not block shipping given the already-documented scope limits (D-04, Pitfall 2, and the CR-02 scope note in the code itself). ## Warnings -### WR-01: Tier 2 candidate query has no `ORDER BY` — non-deterministic campaign selection among multiple qualifying siblings +### WR-01: `decrementOriginCampaign` has no floor — `report_count` can go negative -**File:** `lib/services/campaign-grouping-service.ts:241-251` -**Issue:** Tier 1 (`campaign-grouping-service.ts:192-201`, `ORDER BY -r.created_at ASC LIMIT 1`) and Tier 3 (`:301-311`, `ORDER BY r.created_at -ASC`) both deterministically pick the earliest-created match. The Tier 2 -candidate query has no `ORDER BY` at all: - -```ts -const tier2Candidates = await client.query( - `SELECT r.id::text AS report_id, r.campaign_id::text AS campaign_id, r.title, - m.id::text AS message_id - FROM messages m - JOIN reports r ON r.id = m.report_id - WHERE r.campaign_id IS NOT NULL - AND r.id != $1 - AND r.created_at BETWEEN $2::timestamptz - INTERVAL '24 hours' - AND $2::timestamptz + INTERVAL '24 hours'`, - [reportId, ownReport.created_at] -); +**File:** `lib/services/campaign-grouping-service.ts:131-138` +**Issue:** The decrement is unconditional (`report_count = report_count - 1`) with no `GREATEST(...)` clamp, and there is no DB-level `CHECK (report_count >= 0)` constraint (`migrations/097_phishing_triage_schema.sql:32` — plain `INTEGER NOT NULL DEFAULT 0`). Every path in this file that reaches `decrementOriginCampaign` is, by hand-trace, reachable at most once per abandonment under single-instance, non-concurrent execution — so this is safe today. But that guarantee rests entirely on there being no concurrent double-fire of the same grouping call (see WR-02). If a race — or a future caller — ever invokes this twice for the same origin campaign, the count silently goes negative with no defensive floor and nothing downstream would catch it. +**Fix:** +```sql +UPDATE campaigns + SET report_count = GREATEST(report_count - 1, 0), updated_at = NOW() + WHERE id = $1 ``` -If two distinct existing campaigns both have a sibling report matching by -attachment-hash/URL-domain + subject + sender within the 24h window (plausible -during a large phishing wave, where multiple independent campaigns can share -a lure template), which one gets attached is whatever order Postgres happens -to return rows in — not guaranteed stable across query plans/analyze cycles, -and inconsistent with Tier 1/Tier 3's explicit earliest-wins ordering. -**Fix:** Add `ORDER BY r.created_at ASC` to match the Tier 1/Tier 3 -convention. +### WR-02: Pre-transaction `skipIfAlreadyGrouped` check is a TOCTOU race, and the new decrement logic raises its stakes -### WR-02: No regression test covers the same-campaign guard for Tier 1/Tier 2, nor the CR-02 cross-campaign-migration gap - -**File:** `lib/services/campaign-grouping-service.test.ts:337-362` -**Issue:** The regression test added by 18-04 -(`'re-analyzing a report already linked to a campaign that a sibling report -re-matches...'`) only exercises the same-campaign guard via a **Tier 3** -match. The guard is generic — applied once after all three tiers resolve — -so this is a coverage gap rather than proof the fix generalizes, and nothing -in the current suite would have caught CR-02 above. -**Fix:** Add two more cases: (1) a Tier 1 sibling match that resolves to the -report's own current campaign (should still no-op — currently unverified for -this tier), and (2) a case where `ownReport.campaign_id = 'campaign-A'` and -the tier match resolves to a *different* `'campaign-B'` — asserting the fix -for CR-02 (old campaign decremented, new campaign incremented, report moved) -once implemented, so this doesn't regress silently again. +**File:** `lib/services/campaign-grouping-service.ts:165-175` +**Issue:** The `skipIfAlreadyGrouped` short-circuit reads `reports.campaign_id` via a plain `postgresClient.query` *outside* the transaction, before `BEGIN` (transaction body starts at `:175`). If the webhook or cron sweep fires twice in quick succession for the same report (webhook redelivery, or overlapping cron sweep ticks — both real call sites: `webhook-service.ts:493`, `phishing-sweep-service.ts:91`), both invocations can observe `campaign_id IS NULL` and both proceed into their own transaction. Neither transaction takes a row lock on the `reports` row or the candidate `campaigns` row, so both can independently reach the same tier match. This race pre-dates this round (already flagged as a known limitation via the "Pitfall 2 — no UNIQUE constraint on `campaign_key`" comment), but the new `decrementOriginCampaign` logic makes a concrete double-fire strictly worse than before: previously a double-fire risked only a duplicate `campaigns` row (visible/annoying); now a double-fire on the migration or CR-03-diverged branch can double-decrement an origin campaign's count, silently under-reporting it (compounded by WR-01's missing floor). +**Fix:** Out of scope to fully close here (would need `SELECT ... FOR UPDATE` on the `reports` row inside the transaction, or an advisory lock keyed on `reportId`), but worth a follow-up given the new decrement path raises the impact of the existing race. At minimum, consider re-reading `reports.campaign_id` with `FOR UPDATE` as the first statement inside the transaction (`:176-181`) so a concurrent second call blocks until the first commits, instead of relying solely on the non-transactional outer pre-check. ## Info -### IN-01: `limit=0` or negative is silently clamped to `0` (empty page) rather than falling back to the documented default of 50 +### IN-01: Unchecked cast from DB `TEXT` to the `groupMethod` union -**File:** `app/api/phishing/campaigns/route.ts:32` -**Issue:** `Math.min(Math.max(Number.isFinite(rawLimit) ? rawLimit : 50, 0), 200)` -correctly prevents the Postgres 500 that the prior WR-02 flagged, but a -request like `?limit=-5` now silently returns an empty `items` array with -`limit: 0` in the response rather than an error or the documented default. -Callers may interpret an empty page as "no campaigns" instead of "your -`limit` param was invalid." Not a crash, just a silent-success footgun. -**Fix:** Consider treating `<= 0` the same as "unparseable" and falling back -to `50`, or explicitly document that `limit=0` is a valid "return nothing" -request. +**File:** `lib/services/campaign-grouping-service.ts:414, 426` +**Issue:** `ownCampaignRes` is typed `{ campaign_key: string; group_method: string }`, and the return value casts it with `ownCampaign.group_method as GroupReportResult['groupMethod']` — no runtime check that the stored value is actually one of `'message_id' | 'attachment_or_url' | 'sender_subject_client'`. In practice the column is only ever written by this same function's own `INSERT` (`:460-465`) with a value drawn from the union, so it's safe today, but it's an unchecked type assertion rather than a validated narrowing — the kind of blind cast CLAUDE.md's "don't use `any` — use specific types" guidance is meant to discourage in spirit. +**Fix:** +```ts +const VALID_GROUP_METHODS = new Set(['message_id', 'attachment_or_url', 'sender_subject_client']); +function asGroupMethod(v: string): GroupReportResult['groupMethod'] { + if (!VALID_GROUP_METHODS.has(v)) throw new Error(`Unexpected group_method value: ${v}`); + return v as GroupReportResult['groupMethod']; +} +``` -### IN-02: `status` query param is not validated against known campaign statuses +### IN-02: CR-02 comment describes an "own campaign row is gone" scenario that the current schema makes unreachable -**File:** `app/api/phishing/campaigns/route.ts:34` -**Issue:** `status` is read straight from the query string and used directly -in the parameterized `WHERE` clause (safe from injection) but never checked -against the actual `campaigns.status` enum/values. A typo (`?status=opne`) -silently returns zero rows with a `200` rather than a `400`. Consistent with -the project's "no Zod in route handlers unless it matters" convention, but -worth a one-line comment noting it's intentional. +**File:** `lib/services/campaign-grouping-service.ts:430-436` +**Issue:** The comment states "own campaign row is gone, or the report's signal has genuinely diverged" as two alternative reasons the guard's `if (ownCampaign && ...)` can fail. Checked against the schema: `reports.campaign_id` is `UUID REFERENCES campaigns(id)` with no `ON DELETE` clause (`migrations/097_phishing_triage_schema.sql:67`), and there is no `DELETE FROM campaigns` anywhere in the codebase today (confirmed via repo-wide grep) — so a referenced `campaigns` row cannot currently be deleted out from under a report, and `ownCampaignRes.rows[0]` will always be present whenever `ownReport.campaign_id` is non-null. The defensive `ownCampaign &&` check is harmless to keep (cheap insurance against a future admin/cleanup script), but the comment overstates a currently-live code path as an expected runtime scenario. +**Fix:** Tighten the comment to note the "row is gone" branch is currently unreachable defensive code (no cascade delete, nothing deletes `campaigns` rows yet) rather than implying there's a live repro path for it — avoids a future reader assuming this needs its own test/repro. -### IN-03: Documented concurrency gap (`campaign_key` has no UNIQUE constraint) remains unaddressed — pre-existing, not touched by this fix +### IN-03: Test suite doesn't cover the CR-01/CR-03 guards via a Tier 2 match, only Tier 1 and Tier 3 -**File:** `lib/services/campaign-grouping-service.ts:125-126` -**Issue:** The file's own comment ("Pitfall 2 — `campaigns.campaign_key` has -no UNIQUE constraint") acknowledges that concurrent calls to -`groupReportIntoCampaign` (e.g. webhook fire + cron sweep landing at nearly -the same time for two reports that would both create/join the same new -campaign) can race and create duplicate campaign rows or double-apply -increments, since there's no row-level locking (`SELECT ... FOR UPDATE`) or -DB-level uniqueness backing the find-or-create. Unrelated to the CR-01/WR-02 -fixes verified in this pass — listed here only to confirm it's still open, -not silently resolved. +**File:** `lib/services/campaign-grouping-service.test.ts:342-424` +**Issue:** Test G (CR-01/CAMP-02 same-campaign no-op guard) and Tests A/B (CR-03 own-campaign revalidation) are covered for the Tier-3 same-campaign case (Test G) and the Tier-1/Tier-3 stored-key cases (A/B) respectively, but no test exercises either guard when the match/stored key is the Tier-2 (`attachment_or_url`) key. Both guards are generic (plain string/reference equality independent of which tier produced the value), so this is a low-risk coverage gap rather than a suspected bug — the existing 12-test suite simply doesn't touch this one branch combination. +**Fix:** Optional: add a Tier-2 variant of Test G and Test A for completeness, reusing the Tier-2 indicator fixture shape already present in the "extracts URL-domain" test. --- -_Reviewed: 2026-07-16T02:32:12Z_ +_Reviewed: 2026-07-16T10:54:20Z_ _Reviewer: Claude (gsd-code-reviewer)_ _Depth: standard_