docs(260716-n46): pre-dispatch plan for Mimecast blast-radius fixes
This commit is contained in:
parent
ced258bbda
commit
ad84885daa
1 changed files with 213 additions and 0 deletions
|
|
@ -0,0 +1,213 @@
|
|||
---
|
||||
phase: quick-260716-n46
|
||||
plan: 01
|
||||
type: execute
|
||||
wave: 1
|
||||
depends_on: []
|
||||
files_modified:
|
||||
- lib/services/mimecast-blast-radius.ts
|
||||
- lib/services/mimecast-blast-radius.test.ts
|
||||
- lib/services/mimecast-client.test.ts
|
||||
- app/api/phishing/campaigns/[id]/route.ts
|
||||
autonomous: true
|
||||
requirements: [BUG-1-future-end-date, BUG-2-multi-tenant-gap]
|
||||
|
||||
must_haves:
|
||||
truths:
|
||||
- "A freshly-detected campaign (primary report created <24h ago) no longer returns a false clean blast-radius reading — the future end-date is clamped to now before the Mimecast search."
|
||||
- "When the underlying delivered-messages search fails silently (returns an error field instead of throwing), getBlastRadius surfaces status: 'unavailable', reason: 'lookup_failed' instead of a confident zero-count 'ok'."
|
||||
- "A campaign whose reporting company has its own enabled mimecast_tenants row is queried against that company's own tenant credentials, not the global Wulf tenant."
|
||||
- "A campaign whose company has no registered tenant still falls back to the global env-configured client (existing behavior preserved)."
|
||||
- "Tenant client_secret / client_id are never written to logs."
|
||||
artifacts:
|
||||
- path: "lib/services/mimecast-blast-radius.ts"
|
||||
provides: "getBlastRadius with optional per-tenant client injection + swallowed-error detection"
|
||||
contains: "options?"
|
||||
- path: "app/api/phishing/campaigns/[id]/route.ts"
|
||||
provides: "clamped date window + per-company mimecast_tenants resolution threaded into getBlastRadius"
|
||||
contains: "mimecast_tenants"
|
||||
- path: "lib/services/mimecast-blast-radius.test.ts"
|
||||
provides: "coverage for injected-client path + swallowed-error-degrades-to-unavailable"
|
||||
- path: "lib/services/mimecast-client.test.ts"
|
||||
provides: "coverage for getMimecastClientForTenant building an independent tenant-scoped instance"
|
||||
key_links:
|
||||
- from: "app/api/phishing/campaigns/[id]/route.ts"
|
||||
to: "mimecast_tenants table"
|
||||
via: "SELECT by reports.company_id where enabled = true"
|
||||
pattern: "mimecast_tenants.*company_id"
|
||||
- from: "app/api/phishing/campaigns/[id]/route.ts"
|
||||
to: "getMimecastClientForTenant"
|
||||
via: "resolved tenant client passed as getBlastRadius option"
|
||||
pattern: "getMimecastClientForTenant"
|
||||
- from: "app/api/phishing/campaigns/[id]/route.ts"
|
||||
to: "getBlastRadius dateWindow.end"
|
||||
via: "Math.min clamp against Date.now()"
|
||||
pattern: "Math\\.min"
|
||||
---
|
||||
|
||||
<objective>
|
||||
Fix two blast-radius bugs found while live-testing ticket 699308 (Seubert & Associates) against the real Mimecast API.
|
||||
|
||||
**Bug 1 (false clean on fresh campaigns):** `app/api/phishing/campaigns/[id]/route.ts` builds `dateWindow.end = createdAt + 24h`. For any campaign whose primary report is <24h old — every freshly-detected campaign, exactly when an operator opens LiveLink review — `end` is in the future. Mimecast's real `/api/message-finder/search` rejects a future `end` with `err_track_and_trace_invalid_end_date`. `searchDeliveredMessages()` swallows that error internally (`return { messages: [], error }`), so `getBlastRadius()`'s outer try/catch never fires and it returns a confident zero-count `status: 'ok'` — a false "nothing happened."
|
||||
|
||||
**Bug 2 (multi-tenant gap, documented as D-05):** `getBlastRadius()` always uses the global env-configured `getMimecastClient()` (Wulf's tenant `CUSA13A95`). For a company with its own registered `mimecast_tenants` row (e.g. Seubert, `CUSA96A181`), querying the global tenant returns zero across delivered/held/threat searches; querying the company's own credentials finds the real message immediately.
|
||||
|
||||
Purpose: make the freshest, most operationally-relevant campaigns return the real answer.
|
||||
Output: clamped date window, swallowed-error surfacing, and per-company tenant resolution — all within the existing `BlastRadiusInput`/`BlastRadiusResult` contract.
|
||||
</objective>
|
||||
|
||||
<execution_context>
|
||||
@$HOME/.claude/get-shit-done/workflows/execute-plan.md
|
||||
@$HOME/.claude/get-shit-done/templates/summary.md
|
||||
</execution_context>
|
||||
|
||||
<context>
|
||||
@.planning/STATE.md
|
||||
@./CLAUDE.md
|
||||
|
||||
<interfaces>
|
||||
<!-- Confirmed from the codebase — executor should use these directly. -->
|
||||
|
||||
From lib/services/mimecast-client.ts:
|
||||
```typescript
|
||||
// Per-tenant factory — ALREADY IMPLEMENTED and exported. Builds a fresh,
|
||||
// independent MimecastClient (NOT the cached global). accountCode is '' —
|
||||
// no x-mc-account header is sent when authenticating directly as the tenant.
|
||||
export function getMimecastClientForTenant(tenant: {
|
||||
client_id: string;
|
||||
client_secret: string;
|
||||
base_url?: string;
|
||||
}): MimecastClient;
|
||||
|
||||
export function getMimecastClient(): MimecastClient; // global env-configured, cached
|
||||
export function isMimecastConfigured(): boolean; // checks global env vars only
|
||||
|
||||
// The swallow point Bug 1 exploits — returns error field, never throws:
|
||||
async searchDeliveredMessages(options): Promise<{ messages: MimecastDeliveredMessage[]; error?: string }>;
|
||||
```
|
||||
|
||||
From lib/services/mimecast-blast-radius.ts (current signature to extend):
|
||||
```typescript
|
||||
export async function getBlastRadius(input: BlastRadiusInput): Promise<BlastRadiusResult>;
|
||||
// Internally hardcodes: const client = getMimecastClient();
|
||||
// Internally: const deliveredResult = await client.searchDeliveredMessages(...);
|
||||
// -> deliveredResult.error is currently IGNORED (Bug 1 swallow).
|
||||
```
|
||||
|
||||
mimecast_tenants schema (migration 062 — snake_case columns):
|
||||
```
|
||||
id SERIAL, company_id BIGINT, account_code VARCHAR, account_name VARCHAR,
|
||||
client_id VARCHAR NOT NULL, client_secret VARCHAR NOT NULL,
|
||||
base_url VARCHAR DEFAULT 'https://api.services.mimecast.com',
|
||||
enabled BOOLEAN DEFAULT true, notes TEXT, created_at, updated_at
|
||||
```
|
||||
|
||||
reports table (migration 097): has `company_id BIGINT` and `company_name VARCHAR`.
|
||||
The route's reports SELECT currently does NOT include company_id — Task 3 adds it.
|
||||
</interfaces>
|
||||
</context>
|
||||
|
||||
<tasks>
|
||||
|
||||
<task type="auto" tdd="true">
|
||||
<name>Task 1: Add optional per-tenant client injection + swallowed-error surfacing to getBlastRadius</name>
|
||||
<files>lib/services/mimecast-blast-radius.ts, lib/services/mimecast-blast-radius.test.ts</files>
|
||||
<behavior>
|
||||
- Given a fake tenant client passed via a new options arg, getBlastRadius calls THAT client's methods and never calls getMimecastClient().
|
||||
- Given a tenant client is provided but isMimecastConfigured() returns false (global env unset), getBlastRadius still runs the fan-out (does NOT short-circuit to not_configured) — a per-tenant client is self-sufficient.
|
||||
- Given NO tenant client and isMimecastConfigured() false, still returns { status: 'unavailable', reason: 'not_configured' } (unchanged).
|
||||
- Given searchDeliveredMessages resolves { messages: [], error: 'err_track_and_trace_invalid_end_date' }, getBlastRadius returns { status: 'unavailable', reason: 'lookup_failed', error: 'err_track_and_trace_invalid_end_date' } — NOT a false-clean ok (Bug 1 defense-in-depth).
|
||||
- Given a cacheScope option, the cache key is namespaced by it so two tenants querying the same messageId do not collide.
|
||||
- All existing getBlastRadius tests still pass unchanged (existing mocks return no `error` field, so the swallow-detection path is inert for them).
|
||||
</behavior>
|
||||
<action>
|
||||
Extend getBlastRadius to accept an optional second parameter: `getBlastRadius(input, options?: { client?: MimecastClient; cacheScope?: string })`. Import the `MimecastClient` type from './mimecast-client'.
|
||||
|
||||
Rework the configuration gate: resolve the client as `options?.client ?? (isMimecastConfigured() ? getMimecastClient() : null)`. If the resolved client is null, return `{ status: 'unavailable', reason: 'not_configured' }`. This means an injected tenant client bypasses the global env check (the tenant carries its own credentials), while the no-injection path preserves today's exact behavior.
|
||||
|
||||
Namespace the cache key with the scope: prefix the existing cache key with `options?.cacheScope ?? 'global'` (e.g. `mimecast:blast-radius:${scope}:msgid:...` / `...:composite:...`). This prevents cross-tenant cache collisions on the same messageId/composite key.
|
||||
|
||||
After the Promise.all fan-out, before computing counts, honor the swallowed delivered-search error: if `deliveredResult.error` is truthy, throw a new Error(deliveredResult.error) so the existing outer catch converts it to `{ status: 'unavailable', reason: 'lookup_failed', error }`. This is the Bug 1 defense-in-depth: even if a future end-date (or any other Mimecast rejection) slips through, the operator sees "unavailable" not a false zero. Do NOT log the error separately here — the existing outer catch already logs err.message only (T-17-02); do not widen logging to response bodies or tenant secrets.
|
||||
|
||||
Update the module doc-comment item (c) "KNOWN LIMITATION — MULTI-TENANT GAP (D-05)": note that per-tenant resolution is now supported via the optional client injection, resolved by the caller (campaign detail route).
|
||||
|
||||
Add tests to mimecast-blast-radius.test.ts following the existing vi.mock() discipline: (1) a fake tenant client object (same shape as getMimecastClientMock's return: getMessageInfo/searchDeliveredMessages/getHeldMessages/getThreatEvents) passed via options.client — assert it is used and getMimecastClientMock is NOT called; (2) tenant client provided while isMimecastConfiguredMock returns false — assert fan-out still runs (result.status === 'ok'); (3) searchDeliveredMessagesMock resolves { messages: [], error: 'err_track_and_trace_invalid_end_date' } — assert result is unavailable/lookup_failed with that error string and setCachedData NOT called. Use only mocked/fake tenant data — never real credentials.
|
||||
</action>
|
||||
<verify>
|
||||
<automated>npx vitest run lib/services/mimecast-blast-radius.test.ts</automated>
|
||||
</verify>
|
||||
<done>All new and existing getBlastRadius tests pass; injected tenant client is exercised; swallowed delivered-search error degrades to unavailable/lookup_failed.</done>
|
||||
</task>
|
||||
|
||||
<task type="auto" tdd="true">
|
||||
<name>Task 2: Add getMimecastClientForTenant coverage</name>
|
||||
<files>lib/services/mimecast-client.test.ts</files>
|
||||
<behavior>
|
||||
- getMimecastClientForTenant returns a MimecastClient instance.
|
||||
- The returned instance is independent of the cached global (calling it does not populate/replace the getMimecastClient() singleton; a subsequent getMimecastClient() with env set still builds its own).
|
||||
- base_url defaults to the Mimecast API host when omitted from the tenant arg.
|
||||
</behavior>
|
||||
<action>
|
||||
Add a `describe('getMimecastClientForTenant', ...)` block to the existing mimecast-client.test.ts, following that file's beforeEach env-delete + _resetMimecastClient() pattern. Import getMimecastClientForTenant alongside the existing imports. Assert: passing a fake tenant object `{ client_id: 'tid', client_secret: 'tsecret', base_url: 'https://tenant.example' }` returns a MimecastClient (instanceof, or a truthy object exposing searchDeliveredMessages). Assert it returns a NEW instance each call (two calls are not `===`), confirming it is not the cached global. Assert omitting base_url does not throw. Use only fake credentials — never real mimecast_tenants values.
|
||||
</action>
|
||||
<verify>
|
||||
<automated>npx vitest run lib/services/mimecast-client.test.ts</automated>
|
||||
</verify>
|
||||
<done>getMimecastClientForTenant tests pass, confirming an independent tenant-scoped instance.</done>
|
||||
</task>
|
||||
|
||||
<task type="auto">
|
||||
<name>Task 3: Clamp date window (Bug 1) and resolve per-company tenant (Bug 2) in the campaign detail route</name>
|
||||
<files>app/api/phishing/campaigns/[id]/route.ts</files>
|
||||
<action>
|
||||
Bug 1 — clamp the end date: in the getBlastRadius call site (~line 262-265), change `end: new Date(createdAt.getTime() + 24 * 60 * 60 * 1000)` to clamp against now: `end: new Date(Math.min(createdAt.getTime() + 24 * 60 * 60 * 1000, Date.now()))`. Leave `start` unchanged. This is the primary Bug 1 fix (Task 1's swallow-detection is the backstop).
|
||||
|
||||
Bug 2 — per-company tenant resolution:
|
||||
1. Add `company_id` to the reports SELECT: add `r.company_id::text` to the column list in the reportsRes query, add `company_id: string | null` to the ReportRow interface, and map it to `companyId` in the `reports.rows.map(...)` block.
|
||||
2. Inside the `if (primaryReport)` branch, before calling getBlastRadius, resolve the reporting company's tenant: if `primaryReport.companyId` is non-null, query `SELECT client_id, client_secret, base_url FROM mimecast_tenants WHERE company_id = $1 AND enabled = true ORDER BY id LIMIT 1` with `[primaryReport.companyId]`. If a row is returned, build a tenant client via `getMimecastClientForTenant({ client_id, client_secret, base_url })` (import it from '@/lib/services/mimecast-client'; note snake_case columns map directly to that function's snake_case param shape) and pass it as the getBlastRadius second arg: `getBlastRadius({...}, { client: tenantClient, cacheScope: primaryReport.companyId })`. If no enabled tenant row exists, call getBlastRadius with no options (global fallback, cacheScope defaults to 'global').
|
||||
3. Never log client_id / client_secret. Do not add a redundant reports query — reuse the company_id added to the existing reportsRes.
|
||||
|
||||
Scope guard: touch only this route and its getBlastRadius call — do not refactor other Mimecast call sites (mimecast-sync-service.ts, etc.).
|
||||
</action>
|
||||
<verify>
|
||||
<automated>npx tsc --noEmit --pretty</automated>
|
||||
</verify>
|
||||
<done>Type check passes; dateWindow.end is clamped via Math.min against Date.now(); route resolves an enabled mimecast_tenants row by reports.company_id and threads a per-tenant client (via getMimecastClientForTenant) into getBlastRadius, falling back to the global client when none exists.</done>
|
||||
</task>
|
||||
|
||||
</tasks>
|
||||
|
||||
<threat_model>
|
||||
## Trust Boundaries
|
||||
|
||||
| Boundary | Description |
|
||||
|----------|-------------|
|
||||
| Postgres mimecast_tenants → app | Reads per-company OAuth client_id/client_secret into process memory |
|
||||
| app → Mimecast API | Authenticates as a specific tenant; credentials must not leak to logs/responses |
|
||||
|
||||
## STRIDE Threat Register
|
||||
|
||||
| Threat ID | Category | Component | Disposition | Mitigation Plan |
|
||||
|-----------|----------|-----------|-------------|-----------------|
|
||||
| T-N46-01 | Information Disclosure | tenant client_id/client_secret from mimecast_tenants | mitigate | Never console.log/echo tenant credentials; existing outer catch logs err.message only (T-17-02 unchanged); tests use fake credentials only |
|
||||
| T-N46-02 | Information Disclosure | cache key collision across tenants | mitigate | Namespace blast-radius cache key by cacheScope (company_id) so one company cannot read another's cached result |
|
||||
| T-N46-03 | Spoofing/Tampering | wrong-tenant query returning false clean | mitigate | Resolve tenant strictly by reports.company_id with enabled = true; fall back to global only when no per-company row exists |
|
||||
</threat_model>
|
||||
|
||||
<verification>
|
||||
- `npx vitest run lib/services/mimecast-blast-radius.test.ts lib/services/mimecast-client.test.ts` — all pass.
|
||||
- `npx tsc --noEmit --pretty` — clean (route.ts safety net, per project convention; vitest does not cover app/**).
|
||||
- Manual reasoning check: a report created 1h ago now produces `end = Date.now()` (not +24h future); a company with an enabled tenant row is queried against its own credentials.
|
||||
</verification>
|
||||
|
||||
<success_criteria>
|
||||
- Fresh campaigns (<24h primary report) no longer produce false-clean blast-radius readings — end date clamped, and any swallowed search error surfaces as unavailable/lookup_failed.
|
||||
- Companies with their own enabled mimecast_tenants row are queried against their own tenant; companies without one fall back to the global client.
|
||||
- Public BlastRadiusInput/BlastRadiusResult contract unchanged (only an additive optional second param to getBlastRadius).
|
||||
- No tenant credentials logged; cache keys are tenant-scoped.
|
||||
- All lib tests green; tsc clean.
|
||||
</success_criteria>
|
||||
|
||||
<output>
|
||||
Create `.planning/quick/260716-n46-fix-mimecast-blast-radius-date-window-fu/260716-n46-SUMMARY.md` when done.
|
||||
</output>
|
||||
Loading…
Add table
Add a link
Reference in a new issue