docs(23): add code review report
This commit is contained in:
parent
dadac5180d
commit
c243cbc41f
1 changed files with 293 additions and 0 deletions
|
|
@ -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_
|
||||
Loading…
Add table
Add a link
Reference in a new issue