docs(18): add code review report
This commit is contained in:
parent
95859660d6
commit
bd90cdcb30
1 changed files with 279 additions and 0 deletions
|
|
@ -0,0 +1,279 @@
|
|||
---
|
||||
phase: 18-campaign-grouping-phishing-analysis-api
|
||||
reviewed: 2026-07-15T23:41:39Z
|
||||
depth: standard
|
||||
files_reviewed: 8
|
||||
files_reviewed_list:
|
||||
- lib/services/campaign-grouping-service.ts
|
||||
- lib/services/campaign-grouping-service.test.ts
|
||||
- lib/permissions.ts
|
||||
- app/api/phishing/tickets/[ticket_id]/analyze/route.ts
|
||||
- lib/services/webhook-service.ts
|
||||
- lib/services/phishing-sweep-service.ts
|
||||
- app/api/phishing/campaigns/route.ts
|
||||
- app/api/phishing/campaigns/[id]/route.ts
|
||||
findings:
|
||||
critical: 1
|
||||
warning: 5
|
||||
info: 3
|
||||
total: 9
|
||||
status: issues_found
|
||||
---
|
||||
|
||||
# Phase 18: Code Review Report
|
||||
|
||||
**Reviewed:** 2026-07-15T23:41:39Z
|
||||
**Depth:** standard
|
||||
**Files Reviewed:** 8
|
||||
**Status:** issues_found
|
||||
|
||||
## Summary
|
||||
|
||||
The core tiered-matching design (`campaign-grouping-service.ts`) is well documented and its
|
||||
self-exclusion logic (own-row filtering in the Tier 1/2/3 queries) is correctly implemented and
|
||||
tested. The automatic paths (webhook, sweep) correctly use `skipIfAlreadyGrouped` to avoid
|
||||
redundant work. However, the one caller that intentionally re-runs grouping unconditionally —
|
||||
`POST /api/phishing/tickets/{id}/analyze` — exposes a real data-integrity bug: `report_count` is
|
||||
incremented every time a match is found, even when the match is the report's *own current*
|
||||
campaign, which is the common case once a campaign has two or more members. This is a BLOCKER
|
||||
because it silently corrupts the exact metric (`report_count`) analysts use to judge the size of
|
||||
a phishing campaign. Several secondary issues (an un-clamped negative `limit` query param, a
|
||||
duplicate-row risk in `messages`, an inconsistent JOIN type between two report-matching queries,
|
||||
and a latent race condition when concurrent reports contend for the same emerging campaign) round
|
||||
out the findings below.
|
||||
|
||||
## Critical Issues
|
||||
|
||||
### CR-01: `groupReportIntoCampaign` double-counts `report_count` on every re-run against an already-linked campaign
|
||||
|
||||
**File:** `lib/services/campaign-grouping-service.ts:156-165, 299-334`
|
||||
**Issue:**
|
||||
The transaction's own-report query never selects the report's *current* `campaign_id`:
|
||||
|
||||
```ts
|
||||
const ownReportRes = await client.query<OwnReportRow>(
|
||||
`SELECT title, requester_contact_id, company_id, created_at
|
||||
FROM reports
|
||||
WHERE id = $1`,
|
||||
[reportId]
|
||||
);
|
||||
```
|
||||
|
||||
When a match is found on any tier, the code unconditionally bumps the target campaign's counter
|
||||
and re-points the report at it:
|
||||
|
||||
```ts
|
||||
if (matchCampaignId && matchGroupMethod) {
|
||||
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 };
|
||||
}
|
||||
```
|
||||
|
||||
There is no check for `matchCampaignId === (report's existing campaign_id)`. This function is
|
||||
called *unconditionally* (no `skipIfAlreadyGrouped`) from
|
||||
`app/api/phishing/tickets/[ticket_id]/analyze/route.ts:75`, by design ("D-08: /analyze always
|
||||
re-runs grouping unconditionally"). Any campaign with two or more reports has, by construction, at
|
||||
least one *other* report already linked to the same campaign that satisfies Tier 1 (shared
|
||||
`message_id`), Tier 2 (shared attachment/URL + subject/sender), or Tier 3 (sender + subject +
|
||||
company) against the report being re-analyzed. So the very next time an analyst (or an automated
|
||||
retry, or a double-clicked "re-analyze" button) calls `/analyze` on any report belonging to a
|
||||
multi-report campaign, `groupReportIntoCampaign` will re-match that same campaign and increment
|
||||
`report_count` again for a report that was already counted — with no bound on how many times this
|
||||
can happen. `report_count` is the number surfaced verbatim by both
|
||||
`GET /api/phishing/campaigns` and `GET /api/phishing/campaigns/[id]` as the primary signal of a
|
||||
campaign's blast radius; this bug silently inflates that number every time `/analyze` is
|
||||
re-invoked on an already-grouped ticket, which is an explicitly supported, documented use case
|
||||
for this route (upgrading Tier 3 -> Tier 1/2 after EML parsing).
|
||||
|
||||
None of the existing tests cover this path — the "self-exclusion" test in
|
||||
`campaign-grouping-service.test.ts:305-335` only verifies the case where *nothing else* matches
|
||||
(so a fresh campaign is created); it does not cover the case where a *different, sibling* report
|
||||
in the same campaign is found again on re-run.
|
||||
|
||||
**Fix:**
|
||||
Select the report's current `campaign_id` in `ownReportRes` and short-circuit when the tiered
|
||||
match resolves to that same campaign:
|
||||
|
||||
```ts
|
||||
const ownReportRes = await client.query<OwnReportRow>(
|
||||
`SELECT title, requester_contact_id, company_id, created_at, campaign_id::text AS campaign_id
|
||||
FROM reports
|
||||
WHERE id = $1`,
|
||||
[reportId]
|
||||
);
|
||||
...
|
||||
if (matchCampaignId && matchGroupMethod) {
|
||||
if (matchCampaignId === ownReport.campaign_id) {
|
||||
// Already correctly linked — re-run found the same campaign again; no-op.
|
||||
return { campaignId: matchCampaignId, groupMethod: matchGroupMethod, created: false };
|
||||
}
|
||||
await client.query(`UPDATE campaigns SET report_count = report_count + 1, ... 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 };
|
||||
}
|
||||
```
|
||||
(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, so a moved report also leaves the origin campaign's count permanently
|
||||
inflated.)
|
||||
|
||||
## Warnings
|
||||
|
||||
### WR-01: No serialization around concurrent campaign matching — concurrent reports can create duplicate campaigns for the same lure
|
||||
|
||||
**File:** `lib/services/campaign-grouping-service.ts:140-389`, `lib/services/webhook-service.ts:458-495`
|
||||
**Issue:** Each `POST /api/webhooks/autotask` request that carries a new phishing-flagged ticket
|
||||
fires `triggerPhishingDetection()` fire-and-forget (`webhook-service.ts:119-121`), with no queue
|
||||
or lock serializing concurrent invocations. `groupReportIntoCampaign` reads candidate matches and
|
||||
then writes the result inside a single `postgresClient.transaction()` at the default
|
||||
(`READ COMMITTED`) isolation level, with no `SELECT ... FOR UPDATE` / advisory lock on the
|
||||
dedup key. If two reports of the *same* mass-phishing lure arrive close together (the scenario
|
||||
this whole feature exists to catch — many employees reporting the same email via the KnowBe4
|
||||
button within seconds of each other), both transactions can run their Tier 1/2/3 lookups before
|
||||
either commits, both see "no match", and both create separate campaigns for what should be one.
|
||||
This is a distinct failure mode from the already-documented "no merge logic" limitation (D-04) —
|
||||
that limitation is about *not undoing* a bad grouping after the fact; this is about *never
|
||||
grouping correctly in the first place* under concurrent load.
|
||||
**Fix:** Use a Postgres advisory lock keyed by a normalized dedup signal (e.g.
|
||||
`hashtext(sender || normalized_subject || company_id)`) around the tiered-matching block, or
|
||||
serialize phishing-ticket processing through a single-writer queue, so two concurrent reports of
|
||||
the same lure cannot both observe "no existing campaign" at once.
|
||||
|
||||
### WR-02: `GET /api/phishing/campaigns` — negative `limit` is not clamped and `limit=0` silently becomes 50
|
||||
|
||||
**File:** `app/api/phishing/campaigns/route.ts:31`
|
||||
**Issue:**
|
||||
```ts
|
||||
const limit = Math.min(parseInt(url.searchParams.get('limit') ?? '50', 10) || 50, 200);
|
||||
```
|
||||
- `?limit=0` → `parseInt` yields `0`, and `0 || 50` evaluates the fallback because `0` is falsy,
|
||||
silently overriding an explicit, valid request for zero rows with 50.
|
||||
- `?limit=-5` → `parseInt` yields `-5`, `-5 || 50` is `-5` (truthy, non-zero), and
|
||||
`Math.min(-5, 200)` stays `-5`. Unlike `offset` two lines below (which is correctly floored with
|
||||
`Math.max(..., 0)`), `limit` has no lower bound. `-5` is then passed straight into
|
||||
`LIMIT $1 OFFSET $2`, and Postgres raises `ERROR: LIMIT must not be negative`, surfacing as an
|
||||
unhandled 500 from this endpoint for a trivially-supplied query string.
|
||||
**Fix:**
|
||||
```ts
|
||||
const limitParam = parseInt(url.searchParams.get('limit') ?? '', 10);
|
||||
const limit = Math.min(Math.max(Number.isFinite(limitParam) ? limitParam : 50, 0), 200);
|
||||
```
|
||||
|
||||
### WR-03: Repeated `/analyze` calls insert duplicate `messages`/`indicators` rows, and `groupReportIntoCampaign` picks one non-deterministically
|
||||
|
||||
**File:** `app/api/phishing/tickets/[ticket_id]/analyze/route.ts:70`, `lib/services/campaign-grouping-service.ts:172-179`
|
||||
**Issue:** `parseAndStoreMessage` is invoked unconditionally on every `POST /analyze` call, and
|
||||
(per `lib/services/phishing-eml-service.ts`) inserts a new `messages` row plus new `indicators`
|
||||
rows with no existence check and no `ON CONFLICT` — there is no unique constraint on
|
||||
`messages.report_id`. Any repeat call to `/analyze` for the same ticket (a normal thing to do,
|
||||
since this route is explicitly meant to be re-run — see the file's own header comment) therefore
|
||||
accumulates duplicate `messages`/`indicators` rows for the same report. Downstream,
|
||||
`groupReportIntoCampaign`'s own-message lookup:
|
||||
```ts
|
||||
const ownMessageRes = await client.query<OwnMessageRow>(
|
||||
`SELECT id::text AS id, message_id FROM messages WHERE report_id = $1 LIMIT 1`,
|
||||
[reportId]
|
||||
);
|
||||
```
|
||||
has no `ORDER BY`, so which duplicate row is used for Tier 1/2 matching becomes arbitrary and can
|
||||
vary between calls. `GET /api/phishing/campaigns/[id]` (`app/api/phishing/campaigns/[id]/route.ts:100-107`)
|
||||
also has no dedup and will surface every duplicate `messages`/`indicators` row verbatim in its
|
||||
response.
|
||||
**Fix:** In `parseAndStoreMessage` (or at the call site in `analyze/route.ts`), check for an
|
||||
existing `messages` row for the `report_id` first and skip/short-circuit if one already exists
|
||||
(mirroring the `content_hash` idempotency guard already used in `phishing-detector.ts`), or add a
|
||||
unique constraint on `messages.report_id` and upsert.
|
||||
|
||||
### WR-04: Tier 3 matching query uses an `INNER JOIN` to `contacts` that filters out legitimate candidates
|
||||
|
||||
**File:** `lib/services/campaign-grouping-service.ts:300-312`
|
||||
**Issue:**
|
||||
```sql
|
||||
SELECT r.campaign_id::text AS campaign_id, r.title
|
||||
FROM reports r
|
||||
JOIN contacts c ON c.id = r.requester_contact_id
|
||||
WHERE r.requester_contact_id = $1
|
||||
AND r.company_id = $2
|
||||
...
|
||||
```
|
||||
No column from `contacts` is selected or filtered on — the join exists only to (silently) require
|
||||
that `requester_contact_id` currently resolves to a row in `contacts`. `reports.requester_contact_id`
|
||||
has no `FOREIGN KEY` constraint (see `migrations/097_phishing_triage_schema.sql:60`), so a contact
|
||||
that has since been deleted, merged, or not-yet-synced from Autotask will cause this `INNER JOIN`
|
||||
to silently drop an otherwise-valid Tier 3 candidate, understating campaign membership. Contrast
|
||||
with `app/api/phishing/campaigns/[id]/route.ts:91`, which correctly uses a `LEFT JOIN` for the
|
||||
identical `contacts` relationship.
|
||||
**Fix:** Either drop the join (nothing from `contacts` is used) or change it to a `LEFT JOIN` to
|
||||
match the read-path's handling of the same relationship.
|
||||
|
||||
### WR-05: Tier 2 sender-indicator comparison is case-sensitive with no normalization
|
||||
|
||||
**File:** `lib/services/campaign-grouping-service.ts:270-273`
|
||||
**Issue:**
|
||||
```ts
|
||||
const candidateSender = candidateIndicators.find((i) => i.indicator_type === 'sender')?.value;
|
||||
if (candidateSender !== ownSenderValue) continue;
|
||||
```
|
||||
Both values are raw email-address strings taken directly from `indicators.value` (populated in
|
||||
`phishing-eml-service.ts` from the parsed `From:` header with no case normalization).
|
||||
`normalizeSubject()` is used to guard against case drift in the subject, but there is no
|
||||
equivalent normalization for the sender email before this exact-string comparison, so two
|
||||
messages from literally the same sender with different casing in the address (a real possibility
|
||||
across different mail clients re-forwarding the same lure) will fail to Tier-2-match even when the
|
||||
attachment hash / URL domain otherwise line up.
|
||||
**Fix:** Compare with `candidateSender?.toLowerCase() !== ownSenderValue?.toLowerCase()`, and
|
||||
apply the same normalization when building `computeTier2Key`'s `senderValue` component so newly
|
||||
created campaign keys stay consistent.
|
||||
|
||||
## Info
|
||||
|
||||
### IN-01: Ticket-row-fetch query duplicated verbatim across three call sites
|
||||
|
||||
**File:** `app/api/phishing/tickets/[ticket_id]/analyze/route.ts:32-44`, `lib/services/webhook-service.ts:459-471`, `lib/services/phishing-sweep-service.ts:45-61`
|
||||
**Issue:** The exact same `SELECT id, ticket_number, title, description, company_id, contact_id,
|
||||
created_by_contact_id FROM tickets WHERE id = $1` query (and the matching `DetectableTicket`
|
||||
object construction) is duplicated near-verbatim in all three trigger sites. The module headers of
|
||||
`campaign-grouping-service.ts` and `phishing-detector.ts` explicitly call out "no duplicated
|
||||
matching logic between callers" as a design goal for the *matching* logic — this row-fetch/mapping
|
||||
step is the one piece that didn't get the same shared-helper treatment.
|
||||
**Fix:** Extract a small `loadDetectableTicket(ticketId): Promise<DetectableTicket | null>` helper
|
||||
(e.g. in `phishing-detector.ts`, alongside `DetectableTicket`) and call it from all three sites.
|
||||
|
||||
### IN-02: Campaign matching ignores `campaigns.status`
|
||||
|
||||
**File:** `lib/services/campaign-grouping-service.ts:184-318`
|
||||
**Issue:** None of the Tier 1/2/3 matching queries filter on `campaigns.status`. A new (or
|
||||
re-analyzed) report can silently attach to a campaign an analyst has already marked resolved/closed,
|
||||
bumping `report_count`/`last_seen_at` with no visible signal that a "closed" investigation just
|
||||
grew a new member. The code's own comments explicitly defer `status` transitions to Phase 19/20,
|
||||
so this may be intentional scope-narrowing rather than an oversight — flagging for visibility since
|
||||
it interacts with the `report_count` semantics analysts will rely on.
|
||||
**Fix (if in scope for a later phase):** Either exclude non-`open` campaigns from matching, or
|
||||
surface a `reopened` flag / audit event when a report attaches to a non-open campaign.
|
||||
|
||||
### IN-03: No test coverage for the CR-01 double-increment scenario
|
||||
|
||||
**File:** `lib/services/campaign-grouping-service.test.ts`
|
||||
**Issue:** The suite thoroughly covers first-time Tier 1/2/3 matches, campaign creation, and
|
||||
"self only, nothing else matches" self-exclusion (`campaign-grouping-service.test.ts:305-335`), but
|
||||
has no test for the case exercised by CR-01: a report that is already linked to campaign X, whose
|
||||
tiered re-match finds a *different* report also linked to X (i.e., re-matches its own current
|
||||
campaign via a sibling row, not via its own row).
|
||||
**Fix:** Add a test staging `ownReport.campaign_id` (once selected, per the CR-01 fix) equal to a
|
||||
tier match's `campaign_id`, asserting `UPDATE campaigns ... report_count = report_count + 1` is
|
||||
**not** called a second time.
|
||||
|
||||
---
|
||||
|
||||
_Reviewed: 2026-07-15T23:41:39Z_
|
||||
_Reviewer: Claude (gsd-code-reviewer)_
|
||||
_Depth: standard_
|
||||
Loading…
Add table
Add a link
Reference in a new issue