diff --git a/.planning/phases/23-classification-disposition-per-client-automation-gate/23-REVIEW.md b/.planning/phases/23-classification-disposition-per-client-automation-gate/23-REVIEW.md new file mode 100644 index 0000000..4bdb06b --- /dev/null +++ b/.planning/phases/23-classification-disposition-per-client-automation-gate/23-REVIEW.md @@ -0,0 +1,293 @@ +--- +phase: 23-classification-disposition-per-client-automation-gate +reviewed: 2026-07-16T00:00:00Z +depth: standard +files_reviewed: 19 +files_reviewed_list: + - app/admin/page.tsx + - app/admin/phishing-automation/page.tsx + - app/api/admin/phishing-automation/[companyId]/route.ts + - app/api/admin/phishing-automation/route.ts + - components/phishing/action-area-card.tsx + - components/phishing/classification-card.tsx + - components/phishing/timeline-card.tsx + - lib/services/campaign-classifier.test.ts + - lib/services/campaign-classifier.ts + - lib/services/phishing-automation-gate.test.ts + - lib/services/phishing-automation-gate.ts + - lib/services/remediation-default-params.test.ts + - lib/services/remediation-default-params.ts + - lib/services/remediation-service.test.ts + - lib/services/remediation-service.ts + - lib/services/triage-note-format.ts + - lib/services/triage-note-service.test.ts + - lib/services/triage-note-service.ts + - lib/services/webhook-service.ts + - migrations/100_phishing_automation_gate.sql +findings: + critical: 1 + warning: 5 + info: 2 + total: 8 +status: issues_found +--- + +# Phase 23: Code Review Report + +**Reviewed:** 2026-07-16T00:00:00Z +**Depth:** standard +**Files Reviewed:** 19 +**Status:** issues_found + +## Summary + +Reviewed the per-client phishing automation gate (admin toggle UI + API), the +`phishing-automation-gate.ts` reader, and the new webhook stage-runner that +wires `parse -> classify -> report` into the auto-detection path, plus the +supporting campaign-classifier/remediation-service/triage-note-service files +that this phase's new code calls into. + +The gate table/reader/admin-UI plumbing is solid and well-tested. The one +serious defect is in the new webhook auto-report wiring: it calls +`generateAndPostAcknowledgment()` unconditionally on every `USER_AWARENESS` +verdict, with no check against remediation/audit history, so a campaign that +accumulates multiple reports (the normal case for a "campaign") will get a +duplicate customer-visible thank-you note re-sent to every already-notified +ticket each time a new report joins. Several secondary issues (stale-closure +race in the toggle UI, an inaccurate "never throws" doc contract, an `any` +cast, and a blast-radius lookup that's always run with empty sender/recipient +filters) are also flagged below. + +## Critical Issues + +### CR-01: Auto-report re-sends the acknowledge_user note to every already-notified ticket on each new report into a campaign + +**File:** `lib/services/webhook-service.ts:522-567` (specifically line 561), calling `lib/services/triage-note-service.ts:212-251` (`generateAndPostAcknowledgment`) + +**Issue:** `runGatedPhishingStages()` is invoked once per new ticket-create +webhook (`triggerPhishingDetection` -> `groupReportIntoCampaign(..., { skipIfAlreadyGrouped: true })` -> +`runGatedPhishingStages`), and `groupReportIntoCampaign` returns the same +`campaignId` whether the report just created a brand-new campaign or was +merged into an *existing* one (`created: false` case, +`campaign-grouping-service.ts` lines ~373/396). Every time this fires with +`gate.autoClassify` and `gate.autoReport` both on, it re-runs +`classifyCampaign()` (a legitimate new history row, D-02) and then, if the +fresh verdict is `USER_AWARENESS`, unconditionally calls +`generateAndPostAcknowledgment(campaignId)`. + +`generateAndPostAcknowledgment()` queries **every** report linked to the +campaign (`SELECT ... FROM reports WHERE campaign_id = $1`) and posts the +customer-visible "Thank you for reporting this email" note to **all** of +their tickets — including ones that were already notified on a previous pass. +There is no check against `remediation_actions` (no row is ever created for +the auto path — see WR-01) or any other record of "have we already sent this +note for this campaign" before firing. + +Since a "phishing campaign" by definition is usually multiple employees +reporting the same email, this is not an edge case — it is the normal shape +of the data the whole feature exists to handle. Any company with +`autoClassify` + `autoReport` enabled will have already-thanked employees +receive duplicate (and, over N additional reports, N-fold) customer-visible +notes on the same ticket thread. + +This also compounds with the pre-existing lack of webhook-event dedup +(`logWebhookEvent` inserts with `ON CONFLICT (event_id) DO NOTHING` but never +checks the insert actually happened before continuing to process — a +redelivered Autotask webhook for the same ticket will reprocess the entire +chain again). + +**Fix:** Before calling `generateAndPostAcknowledgment`, check whether an +`acknowledge_user` remediation/notification has already been recorded for +this campaign (e.g. query `remediation_actions` for an existing +`acknowledge_user` row, or track a `acknowledge_user_posted_at` column on +`campaigns`), and skip the post if one exists. Minimal fix: + +```ts +if (verdict === 'USER_AWARENESS') { + const already = await postgresClient.query<{ id: string }>( + `SELECT id FROM remediation_actions + WHERE campaign_id = $1 AND action_type = 'acknowledge_user' + LIMIT 1`, + [campaignId] + ); + if (already.rows.length === 0) { + await generateAndPostAcknowledgment(campaignId); + // also insert the remediation_actions/audit rows — see WR-01 + } +} +``` + +## Warnings + +### WR-01: Auto-posted acknowledge_user note leaves no remediation_actions or audit_events trail + +**File:** `lib/services/webhook-service.ts:550-566` + +**Issue:** The manual path (`approveRemediationActions` -> +`remediateApprovedActions`) always inserts a `remediation_actions` row and +writes `remediation_approved`/`remediation_completed` `audit_events` rows +(REMED-06's "every state change writes exactly one audit_events row" +invariant, `remediation-service.ts` lines 1-27). The new automatic path +bypasses both services entirely and calls +`generateAndPostAcknowledgment(campaignId)` directly, so: +- The Action Area UI (`components/phishing/action-area-card.tsx`) has no way + to know `acknowledge_user` was already handled for an auto-opted-in + company — the row would still show as an available, unchecked action. +- `TimelineCard` (`components/phishing/timeline-card.tsx`) never shows that + the note was sent, since it renders from `classification`/`report`/`audit` + timeline entries and no audit event is ever written for this path. +- There is no persisted record at all of the auto-post beyond a transient + `console.error` on failure (nothing on success). + +This was flagged as a discretionary decision in +`23-PATTERNS.md` ("Audit-event write on every classify/approve/remediate/ +mark-false-positive state change" section) but was not implemented, breaking +parity with every other phishing state-change in this codebase and directly +enabling CR-01 (no record to check against for idempotency). + +**Fix:** Insert a `remediation_actions` row (status `completed`, +`approved_by: null` or a sentinel like `'system:auto_report'`) and a matching +`writeAuditEvent({ eventType: 'remediation_completed', ... })` call when the +automatic path posts the note — mirroring `remediateApprovedActions`'s shape +so the UI and CR-01's idempotency check both work off the same table. + +### WR-02: Triage-note blast-radius lookup always uses empty sender/recipient filters + +**File:** `lib/services/triage-note-service.ts:126-143` + +**Issue:** `generateAndPostTriageNote()` calls `getBlastRadius()` with +`sender: ''` and `recipient: ''` unconditionally: + +```ts +blastRadius = await getBlastRadius({ + sender: '', + recipient: '', + subject: primaryReport.title ?? '', + ... +}); +``` + +`getBlastRadius()` passes these straight into Mimecast's +`searchDeliveredMessages({ to: input.recipient, from: input.sender, ... })` +and `getHeldMessages({ recipient: input.recipient })` +(`lib/services/mimecast-blast-radius.ts:124-131`). Searching with an empty +`to`/`from` will not reproduce the real fan-out for the reported message — +it will either return no rows (silently rendering a confident-looking +"Blast Radius: Matched: 0 / Delivered: 0 / Held: 0" section that looks like +"nothing happened" when the lookup was never actually scoped to this +message) or, depending on the Mimecast API's handling of blank filters, +unrelated/oversized result sets. This directly undermines the purpose of the +blast-radius section in a note whose entire point is to give staff an +accurate picture of exposure. Contrast with `campaign-classifier.ts`'s +`gatherCampaignEvidence()`, which correctly extracts the sender identity from +the parsed message/indicator rows before calling `getBlastRadius`. + +**Fix:** Join `messages`/`indicators` (as `gatherCampaignEvidence` already +does) to obtain a real sender email and the requester's email for the +primary report, and pass those into `getBlastRadius` instead of empty +strings. + +### WR-03: Lost-update race when toggling two stages on the same company in quick succession + +**File:** `app/admin/phishing-automation/page.tsx:86-118` + +**Issue:** `toggle(company, stage, next)` computes `nextFlags` from the +`company` object captured in the row's render closure, then PATCHes all +three flags. `toggling` is tracked per `${company.id}:${stage}`, so a second +switch on the *same row but different stage* is not disabled while the first +PATCH is in flight. If a user flips `autoParse` and then immediately flips +`autoClassify` before the first request's `setCompanies` update has landed, +both `toggle()` calls read the same stale `company` snapshot. The first +PATCH lands and updates `autoParse` in state; the second PATCH — built from +the pre-update snapshot — sends the *old* `autoParse` value alongside the new +`autoClassify` value, silently reverting the just-applied `autoParse` toggle +in the database. + +**Fix:** Either disable all three switches on a row while any one of that +row's toggles is in flight, or read the latest values from a ref/state getter +at PATCH-send time instead of the closed-over `company` argument, e.g.: + +```ts +setToggling(`${company.id}:${stage}`); +setCompanies(prev => { + const current = prev.find(c => c.id === company.id) ?? company; + const nextFlags = { ...current, [stage]: next }; + void fetch(...); // build the request off `nextFlags`, not the stale param + return prev; +}); +``` + +### WR-04: `getCompanyAutomationGate` docstring claims it "never throws" but does not guard its query + +**File:** `lib/services/phishing-automation-gate.ts:24-58` + +**Issue:** The function comment states: "Absent row, null, or NaN companyId +all resolve to all-false — never throws." The `null`/`NaN` cases are indeed +handled explicitly (lines 31-33), but the actual `postgresClient.query(...)` +call (lines 35-46) is not wrapped in a `try/catch` — a transient DB error +here throws, contradicting the documented contract. In the one real call +site (`webhook-service.ts:529`, `const gate = await +getCompanyAutomationGate(companyId);`), the call is *not* individually +wrapped in a `try/catch` the way each of the three stages below it is +(lines 532-537, 542-548, 551-566) — a DB hiccup here silently skips **all +three** stages for that ticket with no stage-specific log line, relying only +on the caller's fire-and-forget `.catch()` several levels up +(`processWebhook`). + +**Fix:** Either wrap the query in a `try/catch` returning all-false to make +the doc comment true, or fix the doc comment and wrap the call site in +`runGatedPhishingStages` in its own `try/catch` with a dedicated log line so +a DB failure here is distinguishable from "company opted out." + +### WR-05: `any` cast on `session.user` violates the project's explicit no-`any` convention + +**File:** `app/api/admin/phishing-automation/[companyId]/route.ts:40` + +**Issue:** `const userEmail = (session?.user as any)?.email ?? null;` uses +`any`, which CLAUDE.md explicitly prohibits ("Don't use `any` — use specific +types"). Every sibling component in this same phase +(`action-area-card.tsx:308`, `classification-card.tsx:65`) does this +correctly with `(session?.user as { role?: string } | undefined)`. + +**Fix:** +```ts +const userEmail = (session?.user as { email?: string } | undefined)?.email ?? null; +``` + +## Info + +### IN-01: `companyId` route param accepts partially-numeric strings + +**File:** `app/api/admin/phishing-automation/[companyId]/route.ts:24,71` + +**Issue:** `parseInt(companyId, 10)` on a value like `"123abc"` returns +`123` rather than `NaN`, so the `isNaN(id)` guard doesn't catch malformed but +partially-numeric path segments — the request silently targets company `123` +instead of rejecting the input. Low risk (admin-only surface, IDs are +generated by the UI itself, not user-typed), but worth tightening. + +**Fix:** Use a stricter check, e.g. `if (!/^\d+$/.test(companyId)) return +NextResponse.json(...)` before `parseInt`. + +### IN-02: `getWebhookStats` builds its SQL `INTERVAL` clause via string interpolation + +**File:** `lib/services/webhook-service.ts:340-351` (pre-existing, not +introduced by this phase, but present in the reviewed file) + +**Issue:** `INTERVAL '${hours} hours'` is interpolated directly into the SQL +string rather than passed as a bound parameter. The one current caller +(`app/api/webhooks/stats/route.ts`) sanitizes via `parseInt()` first, so this +isn't currently exploitable, but the pattern is fragile — a future caller +that forwards a raw string (or changes the `parseInt` call) would reintroduce +an injection vector, and a non-numeric `hours` value today produces an +uncaught-looking Postgres syntax error surfaced as a generic 500. + +**Fix:** Validate `hours` is a finite integer within a sane range before use, +or build the interval via `make_interval(hours => $1::int)` bound as a +parameter. + +--- + +_Reviewed: 2026-07-16T00:00:00Z_ +_Reviewer: Claude (gsd-code-reviewer)_ +_Depth: standard_