docs(15): add code review report
This commit is contained in:
parent
35339a62b8
commit
e94482bd0b
1 changed files with 184 additions and 0 deletions
|
|
@ -0,0 +1,184 @@
|
|||
---
|
||||
phase: 15-data-model-detection-ticket-evidence
|
||||
reviewed: 2026-07-15T00:00:00Z
|
||||
depth: standard
|
||||
files_reviewed: 7
|
||||
files_reviewed_list:
|
||||
- migrations/097_phishing_triage_schema.sql
|
||||
- migrations/098_phishing_sweep_schedule.sql
|
||||
- lib/services/phishing-detector.ts
|
||||
- lib/services/phishing-detector.test.ts
|
||||
- lib/services/phishing-sweep-service.ts
|
||||
- lib/services/webhook-service.ts
|
||||
- lib/services/sync-scheduler.ts
|
||||
findings:
|
||||
critical: 1
|
||||
warning: 2
|
||||
info: 2
|
||||
total: 5
|
||||
status: issues_found
|
||||
---
|
||||
|
||||
# Phase 15: Code Review Report
|
||||
|
||||
**Reviewed:** 2026-07-15T00:00:00Z
|
||||
**Depth:** standard
|
||||
**Files Reviewed:** 7
|
||||
**Status:** issues_found
|
||||
|
||||
## Summary
|
||||
|
||||
Reviewed the new phishing-triage schema (7 tables, `reports` fully designed and 6 stubs for later phases), the shared `phishing-detector.ts` core (pattern matcher + content-hash idempotency + evidence gathering + upsert), the `phishing-sweep-service.ts` cron reconciliation, and the two hook-in points (webhook-service.ts's new `triggerPhishingDetection`, sync-scheduler.ts's new `phishing-sweep` dispatch branch + default schedule).
|
||||
|
||||
The good news: the pattern matcher is exactly what it claims to be (plain `.toLowerCase()` + `.includes()`, no regex, no ReDoS surface), the content hash is scoped to `title`+`description` only (correctly excludes status/assignee churn per D-04), all `reports`/`companies`/`ticket_notes`/`time_entries` queries are fully parameterized, and the sync-scheduler dispatch branch is fire-and-forget-safe and wrapped in the same try/catch/status-update scaffolding as every other schedule type.
|
||||
|
||||
The bad news: tracing the webhook hook-in across `app/api/webhooks/autotask/route.ts` → `lib/types/webhook.ts` → `lib/services/webhook-service.ts` shows `triggerPhishingDetection` reads from `payload.entity`, a field that is **never populated** anywhere in the real webhook flow (Autotask's actual payload only carries `Action/Guid/EntityType/Id/Fields/EventTime`, and `normalizeWebhookPayload` never sets `.entity`). As a result the "primary near-real-time trigger" described in `phishing-sweep-service.ts`'s own docstring never actually flags a ticket — every webhook-triggered call to `detectPhishingTicket` runs with `title: null, description: null`, which can never match any pattern. This is a functional regression that should block ship; see CR-01.
|
||||
|
||||
## Critical Issues
|
||||
|
||||
### CR-01: Webhook-triggered phishing detection never actually detects anything (`payload.entity` is always `undefined`)
|
||||
|
||||
**File:** `lib/services/webhook-service.ts:449-480` (root cause spans `lib/types/webhook.ts:97-157` and `app/api/webhooks/autotask/route.ts:41-53`)
|
||||
|
||||
**Issue:**
|
||||
`triggerPhishingDetection` branches on `payload.entity`:
|
||||
|
||||
```ts
|
||||
if (payload.entity) {
|
||||
ticket = {
|
||||
id: payload.entityId,
|
||||
ticket_number: payload.entity.ticketNumber || null,
|
||||
title: payload.entity.title || null,
|
||||
description: payload.entity.description || null,
|
||||
...
|
||||
};
|
||||
} else {
|
||||
ticket = { id: payload.entityId, ticket_number: null, title: null, description: null, ... };
|
||||
}
|
||||
```
|
||||
|
||||
But `payload.entity` is structurally never populated:
|
||||
- Autotask's real webhook payload (`AutotaskRawWebhookPayload`, `lib/types/webhook.ts:83-92`) only contains `Action, Guid, EntityType, Id, Fields, EventTime, SequenceNumber, PersonId` — there is no embedded entity body. `app/api/webhooks/autotask/route.ts`'s own header comment confirms this is the *actual* payload shape.
|
||||
- `normalizeWebhookPayload()` (`lib/types/webhook.ts:137-157`) constructs `AutotaskWebhookPayload` from the raw payload and never assigns `.entity` — it's left `undefined`.
|
||||
- `handleCreateOrUpdate()` (`webhook-service.ts:157-180`) fetches the full ticket separately (either from `payload.entity` — always absent — or via `client.getEntityById(...)`) and upserts it into Postgres, but never writes the fetched entity back onto `payload.entity`. By the time `triggerPhishingDetection(payload)` runs (same `payload` object, fire-and-forget after the response path), `payload.entity` is still `undefined`.
|
||||
|
||||
So every real invocation takes the `else` branch: `title: null, description: null`. `matchesPhishingPatterns(null, null)` computes `haystack = ' '` (empty + space + empty) and `.includes()` against all 8 locked patterns is always `false`. `detectPhishingTicket` therefore returns `{ flagged: false }` immediately, before ever reaching the content-hash / evidence-gathering / upsert logic. **No `reports` row is ever created via the webhook path.**
|
||||
|
||||
This is worse than a no-op: the code logs `[WEBHOOK] Triggering phishing detection for ticket ${payload.entityId}` on every ticket creation, which reads as confirmation the pipeline ran, while it silently never matches. The only thing that will ever populate `reports` is the nightly `phishing-sweep` cron (disabled by default, `0 5 * * *`), directly contradicting the stated design ("webhook path... is the primary near-real-time trigger; this sweep catches anything the webhook missed" — `phishing-sweep-service.ts:5-9`).
|
||||
|
||||
**Fix:** Don't rely on `payload.entity`. Either fetch the ticket row from Postgres (already upserted earlier in the same request by `handleCreateOrUpdate`) or reuse the entity `handleCreateOrUpdate` already fetched from Autotask, and thread it through to `triggerPhishingDetection`:
|
||||
|
||||
```ts
|
||||
private async triggerPhishingDetection(payload: AutotaskWebhookPayload): Promise<void> {
|
||||
const row = await postgresClient.query<{
|
||||
id: string; ticket_number: string | null; title: string | null;
|
||||
description: string | null; company_id: number | null;
|
||||
contact_id: number | null; created_by_contact_id: number | null;
|
||||
}>(
|
||||
`SELECT id, ticket_number, title, description, company_id, contact_id, created_by_contact_id
|
||||
FROM tickets WHERE id = $1`,
|
||||
[payload.entityId]
|
||||
);
|
||||
const r = row.rows[0];
|
||||
if (!r) {
|
||||
console.warn(`[WEBHOOK] Skipping phishing detection — ticket ${payload.entityId} not found in Postgres yet`);
|
||||
return;
|
||||
}
|
||||
const ticket: DetectableTicket = {
|
||||
id: Number(r.id),
|
||||
ticket_number: r.ticket_number,
|
||||
title: r.title,
|
||||
description: r.description,
|
||||
company_id: r.company_id,
|
||||
contact_id: r.contact_id,
|
||||
created_by_contact_id: r.created_by_contact_id,
|
||||
};
|
||||
await detectPhishingTicket(ticket);
|
||||
}
|
||||
```
|
||||
|
||||
This runs after `handleCreateOrUpdate` has already upserted the ticket, so the row is guaranteed to exist and have current `title`/`description`.
|
||||
|
||||
## Warnings
|
||||
|
||||
### WR-01: `phishing-detector.ts` duplicates the Autotask client factory and drops its config validation
|
||||
|
||||
**File:** `lib/services/phishing-detector.ts:112-123`
|
||||
|
||||
**Issue:** The codebase already has a shared factory, `lib/services/autotask-factory.ts::getAutotaskClient()`, which validates all four required env vars and **throws** a clear error when Autotask isn't configured. `phishing-detector.ts` instead hand-rolls its own private module-level singleton with the same shape but no validation:
|
||||
|
||||
```ts
|
||||
let _autotaskClient: AutotaskClient | null = null;
|
||||
function getAutotaskClient(): AutotaskClient {
|
||||
if (!_autotaskClient) {
|
||||
_autotaskClient = new AutotaskClient({
|
||||
apiUrl: process.env.AUTOTASK_API_URL || '',
|
||||
username: process.env.AUTOTASK_USERNAME || '',
|
||||
password: process.env.AUTOTASK_SECRET || '',
|
||||
apiIntegrationCode: process.env.AUTOTASK_API_INTEGRATION_CODE || '',
|
||||
});
|
||||
}
|
||||
return _autotaskClient;
|
||||
}
|
||||
```
|
||||
|
||||
Since `gatherTicketEvidence`'s call to `getAttachments()` is wrapped in a try/catch that swallows all errors to `attachments = []`, a misconfigured Autotask integration will silently produce empty attachment evidence forever, with no distinguishable signal from "ticket genuinely has no attachments." It also creates a second, independently-cached `AutotaskClient` instance parallel to the one in `autotask-factory.ts` and the one duplicated again in `webhook-service.ts`.
|
||||
|
||||
**Fix:** Import and use the shared factory instead of re-implementing it:
|
||||
```ts
|
||||
import { getAutotaskClient } from './autotask-factory';
|
||||
```
|
||||
and drop the local `_autotaskClient`/`getAutotaskClient` in `phishing-detector.ts`.
|
||||
|
||||
### WR-02: Evidence snapshot goes stale — the content-hash gate that (correctly) excludes status/assignee churn also silently skips re-gathering evidence
|
||||
|
||||
**File:** `lib/services/phishing-detector.ts:195-207`
|
||||
|
||||
**Issue:** `detectPhishingTicket` short-circuits before evidence gathering whenever `content_hash` (over `title`+`description` only) is unchanged:
|
||||
|
||||
```ts
|
||||
if (existing.rowCount && existing.rowCount > 0 && existing.rows[0].content_hash === contentHash) {
|
||||
return { flagged: true, skippedUnchanged: true };
|
||||
}
|
||||
const evidence = await gatherTicketEvidence(ticket);
|
||||
```
|
||||
|
||||
This correctly avoids reprocessing on pure status/assignee churn (the D-04 design goal), but it also means `reports.evidence` (notes, time entries, attachment metadata) is frozen at whatever it was captured on the first detection. If an analyst adds a new ticket note, logs time, or an attachment lands on the ticket later — all common during triage — and the title/description text doesn't change, the stored evidence snapshot never refreshes. Later phases (16+) that read `reports.evidence` for message/indicator extraction will be working from a stale snapshot unless something re-triggers full reprocessing (e.g. a title/description edit).
|
||||
|
||||
**Fix:** Consider decoupling "should we re-run pattern matching + rewrite content_hash" from "should we refresh the evidence snapshot" — e.g. re-gather evidence on every sweep pass for still-open reports regardless of content_hash, or hash evidence inputs (notes/time-entries/attachment count) separately and refresh when that changes. At minimum, document this as an accepted limitation if it's intentional, since it isn't currently called out anywhere in the code comments (only the status/assignee-churn exclusion is documented).
|
||||
|
||||
## Info
|
||||
|
||||
### IN-01: Non-parameterized SQL interpolation of a sweep-window constant
|
||||
|
||||
**File:** `lib/services/phishing-sweep-service.ts:48`
|
||||
|
||||
**Issue:** The sweep query builds part of its `WHERE` clause via template-literal interpolation rather than a bound parameter:
|
||||
```ts
|
||||
WHERE is_deleted = false
|
||||
AND last_activity_date > NOW() - INTERVAL '${SWEEP_WINDOW_DAYS} days'
|
||||
ORDER BY last_activity_date DESC
|
||||
LIMIT $1
|
||||
```
|
||||
`SWEEP_WINDOW_DAYS` is a hardcoded module constant (`7`), not attacker-controlled, so this isn't currently exploitable. But it's a pattern this phase was explicitly asked to be careful about, and it's an easy thing to regress if `SWEEP_WINDOW_DAYS` is ever made configurable (e.g. via an admin-editable setting) without someone remembering to re-parameterize it.
|
||||
|
||||
**Fix:** Use `make_interval(days => $2)` (or similar) with a bound parameter instead of string interpolation, even though today it's a constant:
|
||||
```ts
|
||||
AND last_activity_date > NOW() - make_interval(days => $2)
|
||||
...
|
||||
const candidates = await postgresClient.query(candidatesQuery, [SCAN_LIMIT, SWEEP_WINDOW_DAYS]);
|
||||
```
|
||||
|
||||
### IN-02: No coverage for the orchestration path (`detectPhishingTicket`, `gatherTicketEvidence`) despite an established mocking precedent
|
||||
|
||||
**File:** `lib/services/phishing-detector.test.ts`, `lib/services/phishing-detector.ts:130-248`
|
||||
|
||||
**Issue:** The test file only covers the two pure functions (`matchesPhishingPatterns`, `computePhishingContentHash`). The stateful orchestration — the idempotency check, the upsert, and evidence gathering, which is where CR-01/WR-02-adjacent bugs live — has zero test coverage. This isn't out of line with most of the codebase (per CLAUDE.md, most services have no tests), but the project does have a working precedent for mocking `postgresClient` in a service test (`lib/services/pax8-sync-service.test.ts`, `lib/services/pax8-company-matcher.test.ts`), so a mocked-DB test for the idempotency branch (unchanged hash → `skippedUnchanged: true` without a second evidence fetch) would have been feasible and would have caught regressions in this exact logic cheaply.
|
||||
|
||||
**Fix:** Add a `postgresClient`-mocked test asserting that a second `detectPhishingTicket` call with an identical title/description does not re-invoke `gatherTicketEvidence`/Autotask, following the `pax8-sync-service.test.ts` mocking pattern.
|
||||
|
||||
---
|
||||
|
||||
_Reviewed: 2026-07-15T00:00:00Z_
|
||||
_Reviewer: Claude (gsd-code-reviewer)_
|
||||
_Depth: standard_
|
||||
Loading…
Add table
Add a link
Reference in a new issue