lorentz/wulf-pulse
1
0
Fork 0
Code Issues 1 Pull requests Projects Releases Packages Wiki Activity Actions

docs(18): research phase domain

Browse source
This commit is contained in:
lorentz 2026-07-15 16:22:25 -04:00
parent 0ec690b261
commit 4ccd379dc3
1 changed files with 719 additions and 0 deletions
Download patch file Download diff file Expand all files Collapse all files

719
.planning/phases/18-campaign-grouping-phishing-analysis-api/18-RESEARCH.md Normal file
View file

@ -0,0 +1,719 @@
# Phase 18: Campaign Grouping & Phishing Analysis API - Research
**Researched:** 2026-07-15
**Domain:** Postgres-backed dedupe/grouping logic + Next.js 16 App Router API routes on an existing internal PSA codebase (Pulse)
**Confidence:** HIGH (all findings below are `[VERIFIED: codebase]` — read directly from the files that exist in this repo today, not from training data or external docs)
<user_constraints>
## User Constraints (from CONTEXT.md)
### Locked Decisions
**Grouping Trigger Point**
- **D-01:** Campaign grouping runs automatically as part of the same
detection path Phase 15's `phishing-detector.ts` already uses (webhook
fire-and-forget `ticket.created` handler + the scheduled cron sweep) — the
moment a `reports` row is created or updated, grouping runs against it so a
campaign forms/accumulates without waiting for anyone to call the API. The
new `POST /api/phishing/tickets/{ticket_id}/analyze` endpoint (DETECT-03)
calls the SAME shared grouping function on-demand for one ticket — not a
separate/duplicate implementation. Rationale: success criterion #2
("accumulates... as new duplicate reports arrive over time") implies
grouping must happen automatically, not only when explicitly requested.
**Grouping Parameters**
- **D-02:** Time window for the fallback tiers (attachment-hash/URL-domain +
subject + sender, and sender + normalized-subject + client) is **24
hours**. Rationale: catches same-day mass-phishing blasts (the realistic
case — one attacker campaign, multiple people reporting the same day)
without over-grouping unrelated reports that happen to share sender+subject
weeks apart.
- **D-03:** Subject normalization for the fallback tiers: strip leading
`Re:`/`Fwd:`/`Fw:` prefixes (case-insensitive, repeated occurrences),
lowercase, trim whitespace. Standard email-threading normalization, nothing
more elaborate.
**Campaign Merge Behavior**
- **D-04:** Campaigns are never merged into each other. Each new report
attaches to at most one existing campaign (the single best/first match by
the tiered key) or creates a new one. If a report's tiered key
theoretically matches two distinct existing open campaigns, this is
treated as a rare edge case — do NOT implement multi-campaign-merge logic
(transactional reassignment of reports/messages/indicators/classifications
across campaigns) in this phase. Attach to the first/best match found and
move on; revisit only if this proves to be a real, recurring problem in
practice.
**Permission Model**
- **D-05:** Add a new `phishing` resource to `lib/permissions.ts`'s
`statement` with the FULL action set from the milestone spec now —
`phishing: ["read", "analyze", "approve", "remediate"]` — but this phase
only GRANTS `read` and `analyze` in role definitions:
- `superAdminRole` / `adminRole`: `phishing: ["read", "analyze"]`
- `userRole`: `phishing: ["read"]` (can browse campaigns, cannot trigger
on-demand analysis)
`approve`/`remediate` are declared in the statement (so the vocabulary
exists) but ungranted to any role until Phase 20 wires them up. Rationale:
establishes the full permission vocabulary once so Phase 20 only adds role
grants, not new statement keys — avoids touching the same statement block
twice across phases.
**Route Auth Granularity**
- **D-06:** Every `/api/phishing/*` route uses the fine-grained
`requirePermission()` check from day one, not plain `requireAuth()`:
- `GET /api/phishing/campaigns` and `GET /api/phishing/campaigns/{id}` →
`requirePermission('phishing', 'read')`
- `POST /api/phishing/tickets/{ticket_id}/analyze` →
`requirePermission('phishing', 'analyze')`
This establishes the exact auth convention (per-route, per-action
permission check) that Phase 19-21's endpoints must copy. Since `userRole`
gets `read` granted per D-05, this doesn't restrict any current user from
browsing — it wires the fine-grained check through now rather than
retrofitting it later.
### Claude's Discretion
- **Exact SQL/query shape for `GET /api/phishing/campaigns/{id}`'s nested
response** (linked reports, messages, indicators, classification history)
— single JOIN-heavy query vs. multiple queries assembled in application
code. Follow whatever the existing `entity-sync.ts` / DetailModal-backing
API routes in this codebase already do for similarly-shaped nested detail
responses.
- **Exact shape of the tiered-key matching queries** (how "attachment-hash/
URL-domain" is queried against the `indicators` table's `indicator_type`
values `'attachment_hash'`/`'url'`/`'sender'` from Phase 16, how URL-domain
is extracted from a full URL string) — implementation detail, resolve via
research against the actual `indicators` schema and data shapes Phase 16
produces.
- **Response body shape for `POST /analyze`** — should return the resulting
campaign linkage (campaign ID, grouping method used, whether a new
campaign was created vs. an existing one was matched) — exact field names
are planner's call, following the project's camelCase API response
convention.
- **Where the shared grouping function lives** (new file e.g.
`lib/services/campaign-grouping-service.ts` vs. extending
`phishing-detector.ts`) — planner's call, following whatever composition
pattern is cleanest given `phishing-detector.ts`'s current size and
responsibilities.
- **Whether `campaigns.status` transitions in this phase** (e.g., does
grouping ever set/change `status` beyond the migration's default `'open'`)
— not mentioned in success criteria; likely out of scope for this phase
(status transitions are Phase 19/20's classification/remediation concern),
but confirm during planning.
### Deferred Ideas (OUT OF SCOPE)
- **Multi-campaign merge logic** — deferred per D-04. If a report's tiered
key ever legitimately matches two distinct existing campaigns in practice
(not just in theory), building proper merge/reassignment logic is future
work, not blocking this phase.
- **`campaigns.status` transition logic** (open → closed/resolved etc.) —
out of scope for this phase; belongs to Phase 19 (classification) or
Phase 20 (remediation).
- **approve/remediate permission grants** — the `phishing` resource's
`approve`/`remediate` actions are declared in the statement (D-05) but not
granted to any role until Phase 20.
None — discussion otherwise stayed within phase scope.
</user_constraints>
<phase_requirements>
## Phase Requirements
| ID | Description | Research Support |
|----|-------------|------------------|
| CAMP-01 | Reports are grouped into a campaign using original Message-ID first, then attachment-hash/URL-domain + subject + sender + time-window, then sender + normalized subject + client + time-window as fallback keys | See Architecture Patterns (Pattern 1/2), Common Pitfalls 1/2/4 — tier key sources confirmed against actual `indicators`/`messages`/`reports`/`contacts` columns; 24h window and subject normalization specified per D-02/D-03 with a code example |
| CAMP-02 | A campaign can accumulate many linked ticket reports and recipients over time as duplicates are detected | See Common Pitfalls 3 (no `recipients` column — must be derived via join), Architecture Patterns Pattern 2 (transaction-wrapped find-or-create, `report_count`/`last_seen_at` update) |
| CAMP-03 | An operator can list campaigns and view a single campaign's full detail (linked reports, messages, indicators, classification history) via API | See Architecture Patterns Pattern 3 (bulk-fetch nested detail, exact analog `app/api/admin/device-link-conflicts/route.ts`) |
| DETECT-03 | An operator can trigger analysis of one specific ticket by ID on demand (`POST /api/phishing/tickets/{ticket_id}/analyze`) | See Architecture Patterns Pattern 1 and 4 (shared function call chain: detect → parseAndStoreMessage → group; Next.js 16 `params: Promise<>` convention confirmed) |
| ACCESS-01 | All `/api/phishing/*` endpoints enforce existing Pulse auth conventions (`requireAuth`/`requirePermission`), with approve/remediate requiring elevated permission beyond plain read access | See Code Examples (`requirePermission` call shape, `lib/permissions.ts` diff), Security Domain (V4 Access Control row) |
</phase_requirements>
## Summary
This phase adds no new libraries, no new infra, and no UI. Everything needed already
exists in the repo: `postgresClient` singleton, `requirePermission()`, the
`reports`/`messages`/`indicators`/`campaigns` tables (migrations 097/099), and two
already-wired trigger points (`webhook-service.ts`'s `triggerPhishingDetection()` and
`sync-scheduler.ts`'s `phishing-sweep` cron branch → `phishing-sweep-service.ts`). The
work is: (1) a new pure grouping function that computes a tiered campaign key and
finds-or-creates a `campaigns` row, called from the two existing trigger points plus a
new on-demand route; (2) three new `/api/phishing/*` routes gated by a new `phishing`
permission resource.
**The single most important finding requiring a planning decision:** `parseAndStoreMessage()`
(the function that creates `messages`/`indicators` rows — the only source of Message-ID
and attachment-hash/URL data) is **not called anywhere in the automatic webhook/cron
path today**. It has zero callers outside its own test file. Per Phase 16's own docstring,
it was deliberately left uncalled, waiting for "the live on-demand trigger
(`POST /api/phishing/tickets/{id}/analyze`)" to arrive in this phase. This means: **as
currently wired, the automatic grouping path (webhook `ticket.created` + cron sweep) will
only ever have `reports` + `contacts` table data available — never `messages`/`indicators` —
so Tier 1 (Message-ID) and Tier 2 (attachment-hash/URL-domain) can only ever match for a
ticket that someone has already run through `POST /analyze` at least once.** This is
flagged in Open Questions below with a recommendation; it materially affects whether
success criterion #1's tiered matching is fully live automatically or only via manual
trigger for tiers 1-2.
**Primary recommendation:** Build one new pure service (e.g.
`lib/services/campaign-grouping-service.ts`) exporting a single
`groupReportIntoCampaign(reportId)` function that: looks up the report row, looks up any
linked `messages`/`indicators` rows (if `parseAndStoreMessage` has already run for this
report), tries Tier 1 → Tier 2 → Tier 3 matching queries in order inside a
`postgresClient.transaction()`, and either links `reports.campaign_id` to an existing
campaign (bumping `report_count`/`last_seen_at`) or inserts a new `campaigns` row. Call
this same function from `webhook-service.ts` (after `detectPhishingTicket`),
`phishing-sweep-service.ts` (after `detectPhishingTicket`, per-ticket), and the new
`POST /api/phishing/tickets/{ticket_id}/analyze` route (after `detectPhishingTicket` +
`parseAndStoreMessage`).
## Architectural Responsibility Map
| Capability | Primary Tier | Secondary Tier | Rationale |
|------------|-------------|----------------|-----------|
| Campaign key computation (tiered match logic) | API/Backend (`lib/services/`) | — | Pure business logic, no UI, testable in isolation like `phishing-detector.ts` |
| Campaign find-or-create + accumulation | Database/Storage (Postgres via `postgresClient`) | API/Backend | `campaigns`/`reports.campaign_id` are the durable state; service layer owns the transaction |
| On-demand analyze trigger | API/Backend (`app/api/phishing/tickets/[ticket_id]/analyze/route.ts`) | — | New route, orchestrates existing services, no new infra |
| Campaign list/detail read | API/Backend (`app/api/phishing/campaigns/*`) | Database/Storage | Read-only JSON assembly over existing tables; no caching layer needed at this scale |
| Auth/permission gate | API/Backend (`requirePermission()` in each route) | Browser/Client (none — no UI this phase) | `middleware.ts` only checks session-cookie presence; role/permission check is always in the route handler per this codebase's convention |
| Automatic grouping trigger | API/Backend (webhook handler + cron sweep, both already server-side) | — | Extends Phase 15's existing dual-path wiring; no new trigger mechanism |
## Standard Stack
No new packages required. This phase is 100% additive Postgres queries + Next.js route
handlers using infrastructure already present in `package.json`.
### Core (existing, reused)
| Library | Version (installed) | Purpose | Why Standard |
|---------|---------|---------|--------------|
| `pg` (via `postgresClient` singleton) | 8.11.0 `[VERIFIED: package.json]` | All DB access | Project convention — no ORM |
| Next.js App Router route handlers | 16.1.1 `[VERIFIED: package.json]` | `/api/phishing/*` | Only backend pattern used in this repo |
| Node built-in `crypto` | n/a (built-in) | Content hashing, if a composite-key hash is used | Already the pattern in `phishing-detector.ts`'s `computePhishingContentHash` |
| Node built-in `URL` | n/a (built-in) | Domain extraction from indicator `value` (URL string) for Tier 2 | Precedent: `lib/services/personal-channels.ts:35` already does `new URL(input)` |
### Alternatives Considered
| Instead of | Could Use | Tradeoff |
|------------|-----------|----------|
| Pure-JS tiered key computation + targeted parameterized queries | One giant SQL `JOIN`/`OR` query encoding all three tiers | JS approach matches this codebase's existing style (small pure functions + explicit queries, see `computePhishingContentHash`); a mega-query would be harder to test and reason about, and this phase's data volumes (single-tenant MSP phishing reports) don't need query-level optimization |
**Installation:** None — no `npm install` needed for this phase.
## Package Legitimacy Audit
Not applicable — this phase introduces zero new dependencies. Table omitted per the
package-legitimacy-gate's own scope (only required "whenever this phase installs
external packages").
## Architecture Patterns
### System Architecture Diagram
```
┌─────────────────────────────┐
│ Autotask webhook │
│ ticket.created │
└──────────────┬────────────────┘
│ fire-and-forget
▼
webhook-service.ts: triggerPhishingDetection(payload)
│
▼
detectPhishingTicket(ticket) ──► upserts `reports` row
│
▼
groupReportIntoCampaign(reportId) [NEW, this phase]
│
┌──────────────┼───────────────────────┐
▼ ▼ ▼
Tier 1: Message-ID Tier 2: attach-hash/ Tier 3: sender+
(via messages/ URL-domain+subject+ subject+client+
indicators, if sender+24h window 24h window
parseAndStoreMessage (indicators, (reports +
already ran) 24h window) contacts join)
│ │ │
└──────────────┴───────────┬───────────┘
▼
find-or-create in `campaigns`
(postgresClient.transaction)
│
▼
UPDATE reports SET campaign_id = ...
┌─────────────────────────────┐
│ sync-scheduler.ts cron │
│ sync_type='phishing-sweep' │──► sweepPhishingTickets() ──► same detectPhishingTicket
└─────────────────────────────┘ + groupReportIntoCampaign
loop, per ticket
┌─────────────────────────────────────────────┐
│ POST /api/phishing/tickets/{id}/analyze │ requirePermission('phishing','analyze')
└──────────────────┬────────────────────────────┘
▼
detectPhishingTicket(ticket)
▼
parseAndStoreMessage({reportId, ticketId}) [existing, Phase 16 — currently uncalled elsewhere]
▼
groupReportIntoCampaign(reportId) [NEW — same shared function as above]
▼
returns { campaignId, groupMethod, created: boolean }
┌─────────────────────────────────────────────┐
│ GET /api/phishing/campaigns │ requirePermission('phishing','read')
│ GET /api/phishing/campaigns/{id} │ requirePermission('phishing','read')
└──────────────────┬────────────────────────────┘
▼
SELECT campaigns [+ bulk-fetch linked reports/messages/
indicators/classifications, assembled in application code
— see Pattern: Bulk-Fetch Nested Detail below]
```
### Recommended Project Structure
```
lib/services/
├── phishing-detector.ts # existing — unchanged
├── phishing-eml-service.ts # existing — unchanged (gains a caller: the new /analyze route)
├── phishing-sweep-service.ts # existing — gains one new call per ticket (groupReportIntoCampaign)
├── campaign-grouping-service.ts # NEW — groupReportIntoCampaign(reportId), tier key helpers, normalizeSubject()
app/api/phishing/
├── tickets/[ticket_id]/analyze/route.ts # NEW — POST, requirePermission('phishing','analyze')
├── campaigns/route.ts # NEW — GET (list), requirePermission('phishing','read')
├── campaigns/[id]/route.ts # NEW — GET (detail), requirePermission('phishing','read')
lib/permissions.ts # MODIFIED — add `phishing` resource + role grants (D-05)
lib/services/webhook-service.ts # MODIFIED — triggerPhishingDetection() gains one call
lib/services/sync-scheduler.ts # unchanged (phishing-sweep branch already calls sweepPhishingTickets() — no scheduler.ts edit needed, only phishing-sweep-service.ts)
```
### Pattern 1: Shared pure detector/grouping function, called from 3 sites
**What:** One function (`groupReportIntoCampaign`) with zero duplicated logic, called
from the webhook handler, the cron sweep, and the on-demand API route.
**When to use:** Exactly this phase's D-01 requirement.
**Example (existing precedent to follow exactly — `phishing-detector.ts`'s
`detectPhishingTicket` is called identically from all 3 sites already):**
```typescript
// lib/services/webhook-service.ts:488-489 (existing, unmodified pattern to extend)
console.log(`[WEBHOOK] Triggering phishing detection for ticket ${payload.entityId}`);
await detectPhishingTicket(ticket);
// ADD: await groupReportIntoCampaign(result.reportId) when result.flagged
```
```typescript
// lib/services/phishing-sweep-service.ts:78-84 (existing loop to extend)
const detection = await detectPhishingTicket(ticket);
if (detection.skippedUnchanged) {
result.skippedUnchanged += 1;
} else if (detection.flagged) {
result.flagged += 1;
// ADD: await groupReportIntoCampaign(detection.reportId)
}
```
### Pattern 2: Tiered find-or-create inside a transaction
**What:** Because `campaigns.campaign_key` has **no UNIQUE constraint** (migration 097
only creates a non-unique `idx_campaigns_campaign_key` index — `[VERIFIED: migrations/097_phishing_triage_schema.sql]`), a naive `SELECT` then `INSERT`-if-not-found
race is possible under concurrent webhook+cron execution. Wrap the whole find-or-create
in `postgresClient.transaction()` (existing method, `lib/services/postgres-client.ts:96-111`).
**When to use:** Every call to `groupReportIntoCampaign`.
**Example:**
```typescript
// Source: lib/services/postgres-client.ts:96-111 (existing transaction() method)
await postgresClient.transaction(async (client) => {
// Tier 1: Message-ID (only reachable if messages row exists — see Open Questions)
const tier1 = await client.query(
`SELECT r.campaign_id::text AS campaign_id
FROM messages m
JOIN reports r ON r.id = m.report_id
WHERE m.message_id = $1 AND m.message_id IS NOT NULL AND r.campaign_id IS NOT NULL
ORDER BY r.created_at ASC LIMIT 1`,
[messageId]
);
// ... Tier 2 / Tier 3 queries follow the same shape, each gated on the
// previous tier finding nothing ...
// Then either UPDATE campaigns SET report_count = report_count + 1,
// last_seen_at = NOW() WHERE id = $1, or INSERT INTO campaigns (...) RETURNING id.
});
```
### Pattern 3: Bulk-Fetch Nested Detail (the exact analog to follow for GET /campaigns/{id})
**What:** This codebase's established pattern for assembling a parent + nested-children
JSON response is NOT a single mega-JOIN — it's (1) one query for the parent rows, (2) one
bulk query for all related child rows keyed by an array of parent-derived IDs, (3) a
`Map` for O(1) lookup, (4) a final `.map()` that manually builds the camelCase response
shape.
**When to use:** `GET /api/phishing/campaigns` (list, with per-campaign counts) and
`GET /api/phishing/campaigns/{id}` (detail, with linked reports/messages/indicators/classifications).
**Example — read directly from the codebase (exact pattern to replicate):**
```typescript
// Source: app/api/admin/device-link-conflicts/route.ts:37-133 (existing, full file read)
export async function GET(request: NextRequest) {
const { error } = await requirePermission('admin', 'access');
if (error) return error;
// 1. Parent query with LEFT JOINs for 1:1 relations
const reviews = await postgresClient.query<ReviewRow>(`SELECT ... FROM device_link_review r JOIN ... `, params);
// 2. Bulk-fetch all related child rows keyed by an ID array (ANY($1::bigint[]))
const ciDetails = new Map<string, CiRow>();
const ciRes = await postgresClient.query<CiRow>(`SELECT ... WHERE ci.id = ANY($1::bigint[])`, [Array.from(allCiIds)]);
for (const ci of ciRes.rows) ciDetails.set(ci.id, ci);
// 3. Manual camelCase assembly in a final .map()
const items = reviews.rows.map((r) => ({ id: r.id, detectedAt: r.detected_at, xref: { ... }, candidates: [...] }));
return NextResponse.json({ items, total, limit, offset });
}
```
For the simpler paginated list (`GET /api/phishing/campaigns`), the shorter
`workflow/executions/route.ts` pattern (`{ data, total, limit, offset }`, dynamic
WHERE-clause building from query params) is also a valid, simpler precedent — but note
it does **not** camelCase-transform its output (it spreads raw snake_case rows). Follow
`device-link-conflicts`'s manual-camelCase version instead, since CLAUDE.md's convention
is explicit ("API responses are camelCase — handlers transform manually").
### Pattern 4: Next.js 16 dynamic route param convention (CONFIRMED)
**What:** `params` is a `Promise` that must be `await`ed — this is the actual, current
convention in this repo, confirmed by reading 3 existing dynamic routes.
**Example:**
```typescript
// Source: app/api/tickets/[id]/route.ts:18-23 and app/api/workflow/executions/[id]/route.ts:4-9 (both existing, both this exact shape)
export async function GET(
request: NextRequest,
{ params }: { params: Promise<{ id: string }> }
) {
const { id } = await params;
// ...
}
```
For `POST /api/phishing/tickets/{ticket_id}/analyze`, the folder path is
`app/api/phishing/tickets/[ticket_id]/analyze/route.ts` and the param type is
`{ params: Promise<{ ticket_id: string }> }` — use `Number(ticket_id)` before passing to
`DetectableTicket.id` (matches `tickets/[id]/route.ts:27`'s `parseInt(id)` pattern; `tickets.id`
is `BIGINT`).
### Anti-Patterns to Avoid
- **Duplicating detection/grouping logic in the route handler:** the route must call the
shared service function, not reimplement matching inline (this is the entire point of
D-01 and mirrors how `detectPhishingTicket` is already the single source of truth).
- **Multi-campaign merge machinery:** explicitly out of scope per D-04. Attach to the
first/best match found; do not build transactional reassignment across campaigns.
- **SQL-encoded fuzzy tier logic:** don't try to encode "strip Re:/Fwd:, 24h window,
domain-from-URL" all inside one WHERE clause — compute keys in JS (matching
`computePhishingContentHash`'s style), then run targeted parameterized queries.
## Don't Hand-Roll
| Problem | Don't Build | Use Instead | Why |
|---------|-------------|-------------|-----|
| URL domain extraction from indicator `value` | Regex-based hostname parser | Node built-in `new URL(value).hostname` | Already the precedent in `lib/services/personal-channels.ts:35`; regex-based URL parsing is a classic source of bugs (userinfo, ports, IDN, trailing slashes) |
| Permission/role definitions | A separate phishing-specific auth module | `lib/permissions.ts`'s existing `statement`/`superAdminRole`/`adminRole`/`userRole` + `requirePermission()` from `lib/auth-utils.ts` | This IS the project's only auth mechanism (Better Auth 1.4's `createAccessControl`); adding a parallel one would violate CLAUDE.md's "Better Auth is final" |
| Transaction/locking primitives | Custom advisory-lock or optimistic-concurrency code | `postgresClient.transaction()` (existing, `postgres-client.ts:96-111`) | Already wraps BEGIN/COMMIT/ROLLBACK correctly; Postgres row-level locking inside the callback (if needed) is a plain `SELECT ... FOR UPDATE` inside that same transaction |
**Key insight:** every piece of infrastructure this phase needs (hashing helper style,
transaction wrapper, permission system, route+auth pattern, nested-detail JSON assembly)
already has an exact precedent somewhere in this repo. There is no case in this phase
where reaching for an external library is justified.
## Common Pitfalls
### Pitfall 1: Automatic path has no Message-ID/indicator data to key on (see Open Questions)
**What goes wrong:** Tier 1/2 grouping silently never fires for tickets that arrive via
webhook/cron and are never explicitly `/analyze`d, even though CAMP-01 lists Message-ID
as the "primary" tier.
**Why it happens:** `parseAndStoreMessage()` (the only writer of `messages`/`indicators`)
has zero callers today outside its own test — confirmed via
`grep -rn "parseAndStoreMessage" --include=*.ts` returning only the definition file and
`phishing-eml-service.test.ts`. Phase 16's own plan doc (`16-03-PLAN.md:59`) states the
"live on-demand trigger... arrives in Phase 18" as the intended first caller.
**How to avoid:** Decide explicitly during planning (see Open Questions) whether this
phase also wires `parseAndStoreMessage` into the webhook/cron path, or whether Tier 1/2
are accepted as "only reachable after an explicit `/analyze` call" for v1.
**Warning signs:** If a test/manual check creates two tickets with the same `.eml`
Message-ID via webhook only (no `/analyze` call) and they land in two different
campaigns instead of one, this is the expected (if surprising) behavior under the
"accept as-is" option.
### Pitfall 2: `campaigns.campaign_key` has no UNIQUE constraint
**What goes wrong:** A naive check-then-insert (`SELECT ... campaign_key = $1` then
`INSERT INTO campaigns`) run concurrently by webhook + cron sweep for two reports that
should be the same campaign can create two campaign rows for one campaign.
**Why it happens:** Migration 097 only creates `idx_campaigns_campaign_key` (a plain
non-unique index), not a unique constraint — `[VERIFIED: migrations/097_phishing_triage_schema.sql:36]`.
**How to avoid:** Wrap find-or-create in `postgresClient.transaction()`. Given this
project's own precedent for "rare edge case, document don't over-engineer" (per
`17-mimecast-blast-radius-lookup/17-CONTEXT.md` D-05, cited in this phase's own
CONTEXT.md), a transaction without row-level locking is likely sufficient — true webhook/cron
concurrency on the *same* ticket is rare. Flag as a known limitation rather than adding a
`SELECT ... FOR UPDATE` advisory-lock scheme, unless the planner judges the race
realistic enough to matter (e.g. bulk backfill sweep + webhook firing together).
### Pitfall 3: `campaigns` table has no `recipients` column
**What goes wrong:** Assuming there's a `recipients` array/JSONB column to append to on
each new report (CAMP-02 says "accumulate... recipients").
**Why it happens:** Migration 097's `campaigns` table columns are exactly:
`id, campaign_key, group_method, first_seen_at, last_seen_at, report_count, status,
created_at, updated_at` — no recipients field. `[VERIFIED: migrations/097_phishing_triage_schema.sql:24-34]`
**How to avoid:** "Recipients" for a campaign must be computed at read time (in
`GET /api/phishing/campaigns/{id}`) by joining all linked `reports` rows (via
`reports.campaign_id`) to their `requester_contact_id` → `contacts.email_address`/`first_name`/`last_name`
(contacts table confirmed at `migrations/001_initial_schema.sql:83-103`, has
`email_address`). Do not add a new denormalized column for this in this phase unless the
planner has a specific performance reason to.
### Pitfall 4: `indicators.value` for `indicator_type='url'` is a bare URL string, not a domain
**What goes wrong:** Querying `indicators.value = $domain` directly for Tier 2 matching
returns nothing, because the stored value is the full URL (e.g.
`https://evil.example.com/path?x=1`), not `evil.example.com`.
**Why it happens:** `phishing-eml-service.ts:186-193` inserts
`[messageId, 'url', url, ...]` where `url` is the raw string from
`normalized.urls` (`eml-parser.ts`'s `extractUrls`) — no domain extraction happens at
write time. `[VERIFIED: lib/services/phishing-eml-service.ts:184-193]`
**How to avoid:** Extract the domain in the grouping service at *read* time
(`new URL(indicatorRow.value).hostname`), not by querying `indicators.value` directly for
an exact domain match. Guard `new URL()` in a try/catch — malformed/relative URLs are
possible in real-world phishing emails.
### Pitfall 5: `DetectableTicket.contact_id` vs `reports.requester_contact_id` naming mismatch
**What goes wrong:** Assuming a `sender`/`requester` field exists on the `reports` row
under an obvious name.
**Why it happens:** The TS interface field is `contact_id` (optional) but the DB column
and the actual reports insert map it to `requester_contact_id` —
`[VERIFIED: lib/services/phishing-detector.ts:77, 234]`.
**How to avoid:** When joining `reports` → `contacts` for Tier 3's "sender", join on
`reports.requester_contact_id = contacts.id`, not a column literally named `sender` or
`contact_id`.
## Code Examples
### Subject normalization (D-03) — style matches `computePhishingContentHash`
```typescript
// New helper, following the existing pure-function style in phishing-detector.ts
export function normalizeSubject(subject: string | null): string {
let s = (subject ?? '').trim();
const prefixRe = /^(re|fwd|fw):\s*/i;
while (prefixRe.test(s)) {
s = s.replace(prefixRe, '').trim();
}
return s.toLowerCase();
}
```
### requirePermission call-and-early-return (exact syntax to copy 3x)
```typescript
// Source: app/api/admin/rmm/settings/route.ts:30-31 and app/api/rmm/executions/route.ts:84
const { error } = await requirePermission('phishing', 'read');
if (error) return error;
// or, for the analyze route (needs session.user.id for future audit trail in Phase 20):
const { session, error } = await requirePermission('phishing', 'analyze');
if (error) return error;
```
### `lib/permissions.ts` diff shape (D-05) — closest analog is `rmm: ["read", "execute"]`
```typescript
// statement (add one line, mirrors existing rmm entry at lib/permissions.ts:30)
phishing: ["read", "analyze", "approve", "remediate"],
// superAdminRole / adminRole (add one line each, at lib/permissions.ts:46 and :59)
phishing: ["read", "analyze"],
// userRole (add one line, at lib/permissions.ts:72)
phishing: ["read"],
```
Note: `lib/permissions.ts` (the file read in full for this research) is a **different,
older/parallel permission module** from `lib/auth-utils.ts`'s `requirePermission()` — both
exist in the repo (`[VERIFIED: lib/permissions.ts` full read`]`). `lib/auth-utils.ts`
imports `hasPermission`/`Permission`/`statement` presumably from this same file (confirm
the exact import path during planning — `lib/auth-utils.ts:51-74`'s `requirePermission`
signature matches `Permission["resource"]`/`statement` shapes 1:1 with `lib/permissions.ts`,
so they are the same system, not two competing ones).
## State of the Art
Not applicable — no external library/framework version drift to track. All patterns
above are read directly from this repo's current `master` branch as of 2026-07-15.
## Assumptions Log
| # | Claim | Section | Risk if Wrong |
|---|-------|---------|---------------|
| A1 | `lib/auth-utils.ts`'s `hasPermission`/`Permission`/`statement` imports resolve to `lib/permissions.ts` (not a second, hidden permissions file) | Code Examples | Low — trivially confirmed by opening `lib/auth-utils.ts`'s import block during planning; if wrong, the `phishing` resource must be added to whichever file is actually imported |
| A2 | No true webhook+cron concurrency race on the same ticket is realistic enough to require `SELECT ... FOR UPDATE` locking (Pitfall 2) | Common Pitfalls | Low-medium — if wrong, two campaigns could be created for one real campaign in rare timing windows; recoverable manually since D-04 already accepts "rare edge case" duplication as out of scope for merge-logic |
**Everything else in this research is `[VERIFIED: codebase]`** — read directly from
migration files, service files, route files, and config, not from training-data
assumptions about how this codebase "probably" works.
## Open Questions
1. **Should this phase also wire `parseAndStoreMessage()` into the automatic
webhook/cron path, or leave it reachable only via the on-demand `/analyze` endpoint?**
- What we know: `parseAndStoreMessage` has zero callers today outside its test file.
Phase 16's own plan doc explicitly earmarked "the live on-demand trigger" (this
phase's `/analyze` route) as its first real caller. CONTEXT.md's D-01 says grouping
runs automatically on webhook/cron using whatever the `reports` row has — it does
NOT explicitly say eml-parsing also becomes automatic.
- What's unclear: Whether the project intends Tier 1/Message-ID grouping to be a
realistic automatic behavior (requiring eml parsing on every ticket.created event —
an extra Autotask attachment-content fetch + optional B2 upload per ticket, a
meaningfully bigger automatic-path cost) or an "only when someone has looked at this
ticket" behavior.
- Recommendation: Default to **NOT** wiring `parseAndStoreMessage` into the automatic
path in this phase (matches Phase 16's explicit intent and keeps the automatic path
cheap — no extra Autotask/B2 calls per ticket.created event). Document this
explicitly as a known limitation: automatic grouping effectively only achieves Tier 3
(sender+subject+client+time-window) until an operator calls `/analyze` for a given
ticket, after which Tier 1/2 become available for that ticket's future duplicates.
Surface this tradeoff to the user/planner explicitly rather than silently building
around it — it changes what "automatically grouped" means in practice for success
criterion #1.
2. **Does `groupReportIntoCampaign` re-run for a report already linked to a campaign?**
- What we know: `detectPhishingTicket`'s idempotency guard (content-hash unchanged)
still refreshes the `evidence` JSONB on every call — so it's called repeatedly over
a ticket's lifetime (every webhook re-fire, every cron sweep pass within the 7-day
window).
- What's unclear: Whether `groupReportIntoCampaign` should short-circuit immediately
if `reports.campaign_id IS NOT NULL` (cheap, avoids re-running tier queries for
already-grouped reports on every sweep), or should still re-run to catch a late-
arriving `.eml` upgrading a Tier-3-only report to a Tier-1 match.
- Recommendation: Short-circuit if already linked (`campaign_id IS NOT NULL`) for the
automatic webhook/cron path (cheap, avoids redundant work every 7-day sweep pass),
but let the `/analyze` route always re-run grouping unconditionally (it's an explicit
operator request and may follow a fresh `/analyze` call that just populated
`messages`/`indicators` for the first time, deserving a chance to upgrade to Tier 1).
3. **Does `lib/auth-utils.ts` import `statement`/`hasPermission` from `lib/permissions.ts`
directly?** (See Assumption A1.) Confirm the import path when adding the `phishing`
resource so it lands in the file `requirePermission()` actually reads from.
## Environment Availability
Skipped — this phase has no new external tool/service/runtime dependencies. All
infrastructure (Postgres, the existing Autotask client, B2 client) is already used by
Phase 15/16 code this phase calls into, and none of it is newly required by this phase
specifically.
## Validation Architecture
### Test Framework
| Property | Value |
|----------|-------|
| Framework | vitest 4.1.5 `[VERIFIED: package.json]` |
| Config file | `vitest.config.ts` — `include: ['lib/**/*.test.ts']` only `[VERIFIED: vitest.config.ts]` |
| Quick run command | `npm test -- campaign-grouping-service` (or `npx vitest run lib/services/campaign-grouping-service.test.ts`) |
| Full suite command | `npm test` |
**Critical confirmed finding:** `vitest.config.ts`'s `include` glob is
`lib/**/*.test.ts` — it does **not** include `app/**`. There is no precedent anywhere in
this repo for testing a `route.ts` handler directly (`find app/api -iname "*.test.ts"`
returns zero results), and the current config would not even discover such a test file if
one were added. **Test the service layer only** (`campaign-grouping-service.ts`'s pure
functions and DB-touching functions, mocking `postgresClient` the same way
`phishing-eml-service.test.ts` mocks it) — do not plan a task to add route-handler tests;
it would be inconsistent with every other API route in this codebase and outside the
current test runner's scope.
### Phase Requirements → Test Map
| Req ID | Behavior | Test Type | Automated Command | File Exists? |
|--------|----------|-----------|-------------------|-------------|
| CAMP-01 | Tiered key computation (Message-ID > attachment-hash/URL-domain+subject+sender+24h > sender+normalized-subject+client+24h) | unit | `npx vitest run lib/services/campaign-grouping-service.test.ts` | ❌ Wave 0 |
| CAMP-01 | `normalizeSubject()` strips Re:/Fwd:/Fw: (repeated, case-insensitive), lowercases, trims | unit | same file | ❌ Wave 0 |
| CAMP-02 | Second report with matching key increments `report_count`, updates `last_seen_at`, links `campaign_id`, never creates a second campaign | unit (mocked postgresClient) | same file | ❌ Wave 0 |
| DETECT-03 | `POST /api/phishing/tickets/{id}/analyze` orchestrates detect→parse→group and returns campaign linkage | manual / integration — no route-handler test precedent exists in this repo (see Validation Architecture note above) | manual curl/Postman against a dev server, or a service-layer test of the orchestration function if the route delegates to one | n/a |
| CAMP-03 | `GET /api/phishing/campaigns` and `/{id}` return expected nested shape | manual / integration (same reasoning as above) | manual verification | n/a |
| ACCESS-01 | Every `/api/phishing/*` route rejects unauthenticated/unauthorized requests with 401/403 | manual — no existing precedent for testing `requirePermission()` wiring via automated route tests in this repo | manual curl without/with session cookie, or code-review checklist during `/gsd:verify-work` | n/a |
### Sampling Rate
- **Per task commit:** `npx vitest run lib/services/campaign-grouping-service.test.ts`
- **Per wave merge:** `npm test` (full suite — note pre-existing unrelated failures in
`lib/services/analyzer/itglue-search.test.ts` per Phase 16's `deferred-items.md`; do not
attempt to fix those as part of this phase)
- **Phase gate:** Full suite green (modulo the known pre-existing failure) +
`npx tsc --noEmit --pretty` clean + manual curl verification of all 3 new routes'
auth behavior (since no automated route-handler test precedent exists to rely on)
### Wave 0 Gaps
- [ ] `lib/services/campaign-grouping-service.test.ts` — new file, covers CAMP-01/CAMP-02
pure-function and mocked-DB behavior (mirror `phishing-detector.test.ts` and
`phishing-eml-service.test.ts`'s `vi.mock('./postgres-client')` approach)
- [ ] No new fixtures needed — reuse `eml-parser.fixtures.ts` synthetic fixtures if
Message-ID matching needs a fixture value (per Out-of-Scope: "Real customer `.eml`
fixtures in tests" is banned; synthetic only)
- [ ] Framework install: none — vitest already configured and used extensively
## Security Domain
### Applicable ASVS Categories
| ASVS Category | Applies | Standard Control |
|---------------|---------|-----------------|
| V2 Authentication | No (unchanged) | Better Auth 1.4 session cookie — no change this phase |
| V3 Session Management | No (unchanged) | Better Auth 1.4 — no change this phase |
| V4 Access Control | **Yes** | `requirePermission('phishing', 'read'|'analyze')` per route (D-06) — new `phishing` resource in `lib/permissions.ts`'s `statement`/role definitions (D-05) |
| V5 Input Validation | Yes | `ticket_id`/`id` path params: validate as numeric/UUID before querying (mirror `tickets/[id]/route.ts`'s `parseInt(id)`); campaign `id` is a UUID (`campaigns.id UUID PRIMARY KEY`) — validate format before querying to avoid a malformed-UUID Postgres error leaking as a 500 |
| V6 Cryptography | No | No crypto operations beyond the existing `computePhishingContentHash`-style hashing (not security-sensitive — it's a dedupe key, not an auth/integrity control) |
### Known Threat Patterns for this stack
| Pattern | STRIDE | Standard Mitigation |
|---------|--------|---------------------|
| Broken access control — a `user`-role account calling `POST /analyze` (which only `read` is granted to per D-05) | Elevation of Privilege | `requirePermission('phishing', 'analyze')` returns 403 via `hasPermission()`'s role-statement lookup — already the exact mechanism used for `rmm.execute` today (`app/api/rmm/executions/route.ts:84`) |
| IDOR via `ticket_id`/campaign `id` path param (guessing another company's ticket ID) | Information Disclosure | This codebase's existing routes (e.g. `tickets/[id]/route.ts`) do not currently scope by company/tenant for authenticated users — this is a single-tenant internal MSP tool (Wulf Consulting staff only), so this is consistent with the existing security model, not a new gap introduced by this phase. No additional scoping logic needed beyond matching existing route conventions. |
| SQL injection via unvalidated query params in the list route's dynamic WHERE-clause building | Tampering | Follow `device-link-conflicts/route.ts`'s pattern exactly: parameterized `$n` placeholders built alongside a `params: unknown[]` array — never string-interpolate a user-supplied value into SQL text (the `sourceFilter` string in that file only ever contains the placeholder index, never the value itself) |
| Never-fetch invariant regression (EVID-04/SC#3) — a future contributor "helpfully" adding a URL-preview feature that fetches an indicator's URL | Tampering / SSRF | Not this phase's job to re-verify (Phase 16 already covers it), but note for the planner: this phase's grouping logic only ever reads `indicators.value` as a string for domain-extraction (`new URL(value).hostname`) — never a network fetch. Keep it that way. |
## Sources
### Primary (HIGH confidence — direct codebase reads, this session)
- `migrations/097_phishing_triage_schema.sql` — full read, all 7 table definitions
- `migrations/099_indicators_metadata.sql` — full read, `indicators.metadata` addition
- `migrations/098_phishing_sweep_schedule.sql` — full read, cron schedule seed
- `migrations/001_initial_schema.sql` — `contacts` table columns (lines 83-103)
- `lib/services/phishing-detector.ts` — full read (250 lines)
- `lib/services/phishing-eml-service.ts` — full read (225 lines)
- `lib/services/phishing-sweep-service.ts` — full read (102 lines)
- `lib/services/webhook-service.ts` — `triggerPhishingDetection()` and
`triggerWorkflowEngine()`, lines 395-491
- `lib/services/sync-scheduler.ts` — `phishing-sweep` sync_type branch (lines 25, 303-309, 472-477)
- `lib/services/eml-parser.ts` — `NormalizedMessage` interface, `extractUrls`,
`messageId`/`domain` fields (lines 93-272)
- `lib/permissions.ts` — full read (110 lines), `statement`/roles/`hasPermission`
- `lib/auth-utils.ts` — `requireAuth`/`requirePermission`/`requireAdmin`/`requireSuperAdmin`
(lines 31-124)
- `lib/services/postgres-client.ts` — `transaction()`, `upsert()`, `bulkUpsert()` signatures
(lines 96-238)
- `app/api/admin/rmm/settings/route.ts` — full read, `requirePermission` call shape
- `app/api/tickets/[id]/route.ts` — full read, dynamic-route `params: Promise<>` convention
- `app/api/rmm/executions/route.ts` — full read, list+POST pattern with `requirePermission`
- `app/api/workflow/executions/route.ts` and `[id]/route.ts` — full reads, simpler
list/detail pattern (no camelCase transform — noted as the weaker analog)
- `app/api/admin/device-link-conflicts/route.ts` — full read, bulk-fetch + Map +
manual-camelCase nested-detail pattern (the recommended analog)
- `middleware.ts` — full read of `publicRoutes` array, confirms `/api/phishing/*` is not
public and gets the default session-cookie check
- `vitest.config.ts` — full read, confirms `include: ['lib/**/*.test.ts']` only
- `lib/services/phishing-detector.test.ts` — read, confirms mocking-free pure-function
test style for exported helpers
- `.planning/phases/16-eml-mime-evidence-parser/16-03-PLAN.md` and `16-03-SUMMARY.md` —
confirm `parseAndStoreMessage` was deliberately left uncalled pending this phase
- `.planning/phases/18-campaign-grouping-phishing-analysis-api/18-CONTEXT.md` — full read
- `.planning/REQUIREMENTS.md` — full read
- `.planning/STATE.md` — full read
- `.planning/config.json` — full read (`nyquist_validation: true`, no
`security_enforcement` key present → treated as enabled per instructions)
### Secondary (MEDIUM confidence)
None — no external sources were needed for this phase; every fact is derived from
reading this repository directly.
### Tertiary (LOW confidence)
None.
## Metadata
**Confidence breakdown:**
- Standard stack: HIGH — no new packages, all existing infra confirmed by direct file reads
- Architecture: HIGH — patterns pulled from 3+ existing analogous route files, transaction
method read from source
- Pitfalls: HIGH — all 5 pitfalls are concrete schema/code facts (missing constraint,
missing column, raw-URL-not-domain, field-name mismatch, uncalled function), not
speculative
**Research date:** 2026-07-15
**Valid until:** Stable — this research is tied to the current state of `master` in this
repo, not to any external library version. Re-verify only if `migrations/097`/`099`,
`phishing-detector.ts`, `phishing-eml-service.ts`, `lib/permissions.ts`, or
`lib/auth-utils.ts` change before this phase is planned/executed.