From 03a641efa0f702eefcac3c2052f74068a2bcf024 Mon Sep 17 00:00:00 2001 From: lorentz Date: Thu, 16 Jul 2026 09:56:55 -0400 Subject: [PATCH] docs(20): capture phase context --- .../20-CONTEXT.md | 221 ++++++++++++++++++ .../20-DISCUSSION-LOG.md | 79 +++++++ 2 files changed, 300 insertions(+) create mode 100644 .planning/phases/20-remediation-approval-audit-safety/20-CONTEXT.md create mode 100644 .planning/phases/20-remediation-approval-audit-safety/20-DISCUSSION-LOG.md diff --git a/.planning/phases/20-remediation-approval-audit-safety/20-CONTEXT.md b/.planning/phases/20-remediation-approval-audit-safety/20-CONTEXT.md new file mode 100644 index 0000000..008cdd0 --- /dev/null +++ b/.planning/phases/20-remediation-approval-audit-safety/20-CONTEXT.md @@ -0,0 +1,221 @@ +# Phase 20: Remediation, Approval & Audit Safety - Context + +**Gathered:** 2026-07-16 +**Status:** Ready for planning + + +## Phase Boundary + +Turns a campaign's classification (Phase 19 output) into governed action: +recommended remediation actions are persisted as `proposed`; an operator with +elevated permission can approve specific action(s) with exact params +(recording approver/timestamp); `POST /remediate` proceeds only for approved +actions and is idempotent on re-run; an operator can mark a campaign false +positive (blocked once remediation is already approved/completed); every +state-changing call (classify/approve/remediate/mark-false-positive) writes +an `audit_events` row. + +Does NOT cover actually wiring a real external remediation provider (Mimecast +block, MS Graph forwarding-rule toggle, RMM isolate, password reset) — those +are `REMEDEXEC-01..05`, explicitly deferred to v2. Does NOT cover the +Autotask triage note (Phase 21) or the LiveLink approval UI (Phase 22) — this +phase only builds the APIs those consume. + + + + +## Implementation Decisions + +### Remediate Execution Model +- **D-01:** Since no external provider (Mimecast block/purge, MS Graph + forwarding-rule toggle, RMM isolate, password reset) is wired in this + milestone — confirmed by direct inspection: `mimecast-client.ts` has no + block/purge method, `msgraph-client.ts` has no forwarding-rule method, no + RMM isolate/quarantine method exists anywhere in `lib/services/` — + `POST /remediate` uses a **simulated internal effect** for ALL 7 action + types uniformly (no provider distinction yet): it transitions + `remediation_actions.status` to `completed`, writes the `audit_events` row, + and returns success. No real external API call is made for any action + type in this phase. This is the "effect" that REMED-04's idempotency test + asserts does not duplicate (calling remediate twice on the same approved + action must produce exactly one `completed` transition / one audit entry, + not `not_implemented` twice). REMEDEXEC-01..05 (v2) is where a real + provider call would replace this simulated transition per action type. + +### Permission Grants +- **D-02:** `approve` and `remediate` (declared but ungranted in Phase 18 + D-05) are granted to **both `superAdminRole` and `adminRole`** in + `lib/permissions.ts` — parity with how `analyze` is already granted to + both. `userRole` still gets only `phishing: ["read"]`. Confirmed via direct + read of `lib/permissions.ts`: both roles currently have + `phishing: ["read", "analyze"]` — Phase 20 adds `"approve", "remediate"` to + both role definitions (the `statement` already has the full vocabulary from + Phase 18, so this is a role-grant change only, not a new statement key). + `mark-false-positive` uses the same elevated tier (`approve`-equivalent) — + see D-04 for why it can't be looser than `approve`. + +### Approval Granularity +- **D-03:** `POST /approve` request body specifies WHICH action(s) from the + classification's `recommended_actions` to approve, and MAY override/narrow + their params (e.g. `block_sender` scoped to one specific address rather + than the classifier's broader suggestion). This is NOT a bare + "approve everything the classifier suggested" call — REMED-02's "recording + ... the exact approved action parameters" implies operator input, not an + echo of the classifier's proposal. Each approved action becomes its own + `remediation_actions` row (`status: 'approved'`, `approved_by`, + `approved_at`, `params` = the operator-supplied/overridden params) — + unapproved recommended actions from the classification are simply never + materialized as rows. + +### Mark-False-Positive State Guard +- **D-04:** `POST /mark-false-positive` is **blocked** (explicit + conflict/error response, not silent success) if the campaign already has + any `remediation_actions` row with `status IN ('approved', 'completed')`. + Rationale: prevents a contradictory audit trail where a campaign is both + "remediated" and "false positive." If no approved/completed remediation + exists yet, mark-false-positive proceeds freely (this doesn't block + re-classification or a later un-false-positive correction — that's not in + scope here; planner's call whether false-positive is reversible). + Requires the same elevated permission tier as `approve` (D-02) — marking + false positive is a state-changing security decision, not a read action. + +### Claude's Discretion (explicitly deferred to research + planner) +- Exact idempotency detection mechanism for `POST /remediate` re-runs — + whether it's a status check (`status = 'completed'` short-circuits with + the same response) or a dedupe key/constraint — as long as calling + remediate twice on the same approved action produces exactly one + `completed` transition and one audit entry (D-01's contract). +- Exact `audit_events.actor` value format — likely session email, matching + the `disabled_by` convention already used for admin actions elsewhere in + the codebase (per CLAUDE.md's integration-disable section) — planner's + call, not re-litigated here. +- Whether `POST /approve` allows approving a subset now and a different + subset later (multiple approve calls against the same classification), or + is a single one-shot call — planner's call, no strong preference surfaced. + Must not conflict with D-04's block-after-approved guard. +- Exact JSON request/response shapes for approve/remediate/mark-false-positive + — follow the project's camelCase API convention; `remediation_actions` and + `audit_events` are already JSONB-backed tables from migration 097. +- Whether `not_implemented` still has any code path in this phase (e.g. as a + defensive fallback if a future action type is added without wiring D-01's + simulated-effect handling) — planner's call; REMED-03's exact wording is + satisfied by D-01's simulated-effect model since it always "takes or logs + an action" rather than silently succeeding. + + + + +## Canonical References + +**Downstream agents MUST read these before planning or implementing.** + +### Schema (this phase writes) +- `migrations/097_phishing_triage_schema.sql` — `remediation_actions` table + (`action_type`, `status` DEFAULT `'proposed'`, `params` JSONB, + `approved_by`, `approved_at`) and `audit_events` table (`actor`, + `event_type`, `payload` JSONB) — both currently stubs, this phase is their + first real writer. + +### Action vocabulary (locked upstream, not re-decided) +- `.planning/phases/19-classification-engine/19-CONTEXT.md` D-08 — the 7 + action types (`no_action`, `warn_user`, `disable_forwarding_rule` / + `block_sender`, `purge_message`, `reset_password`, `isolate_endpoint`) and + the OR'd `requires_approval` invariant this phase's approve logic must + respect. +- `lib/services/campaign-classifier.ts` — `classifyCampaign()` output shape + (`recommendedActions`, `requiresApproval`) that `POST /approve` reads from + the most recent `classifications` row for the campaign. + +### Auth/permissions (this phase grants; Phase 18 declared the vocabulary) +- `lib/permissions.ts` — `phishing` statement already has + `["read", "analyze", "approve", "remediate"]` (Phase 18 D-05); this phase + adds `"approve", "remediate"` to `superAdminRole` and `adminRole` (D-02). +- `lib/auth-utils.ts` — `requirePermission(resource, action)` — same + call-and-early-return pattern as every other `/api/phishing/*` route + (Phase 18 D-06 convention). +- `app/api/phishing/campaigns/[id]/classify/route.ts` (Phase 19) — the exact + route-shape precedent (UUID guard + `requirePermission` + delegation) this + phase's three new routes should copy. + +### Provider capability audit (confirms D-01's "no real provider" premise) +- `lib/services/mimecast-client.ts` — no block/purge/quarantine method + exists (`getHeldMessages`/`releaseHeldMessage` are read/release only, not + a send-time block). +- `lib/services/msgraph-client.ts` — no mailbox forwarding-rule or password- + reset method exists. +- No RMM isolate/quarantine method exists anywhere in `lib/services/`. + +### Prior phase decisions (for consistency) +- `.planning/phases/19-classification-engine/19-CONTEXT.md` (D-08 action + vocabulary; D-02 append-only classification history this phase reads from) +- `.planning/phases/18-campaign-grouping-phishing-analysis-api/18-CONTEXT.md` + (D-05 permission statement vocabulary; D-06 per-route `requirePermission` + convention) + + + + +## Existing Code Insights + +### Reusable Assets +- `app/api/phishing/campaigns/[id]/classify/route.ts` — exact structural + precedent (UUID param guard, `requirePermission`, service delegation, + error shape) for the three new routes in this phase. +- `lib/permissions.ts`'s existing per-role object literals — this phase adds + two array entries (`"approve"`, `"remediate"`) to two existing role + objects; no new statement keys needed (Phase 18 D-05 already declared + them). + +### Established Patterns +- Append-only history tables with `campaign_id` FK + `created_at` (same + shape `classifications` used in Phase 19) — `remediation_actions` and + `audit_events` should follow the same insert-only shape, no upsert. +- `requirePermission(resource, action)` early-return pattern for every + `/api/phishing/*` route (Phase 18 D-06) — this phase's three routes follow + the same shape with `'approve'` / `'remediate'` actions respectively. + +### Integration Points +- `POST /approve` reads the campaign's most recent `classifications` row + (Phase 19) for `recommendedActions`/`requiresApproval`, writes one or more + `remediation_actions` rows. +- `POST /remediate` reads approved `remediation_actions` rows for the + campaign, transitions eligible ones to `completed` (D-01), writes + `audit_events`. +- `POST /mark-false-positive` checks `remediation_actions` state (D-04) before + proceeding, writes `audit_events`. +- Phase 21 (Autotask triage note) and Phase 22 (LiveLink approval UI) both + read `remediation_actions`/`audit_events` written by this phase — the + status values and audit event shape are a direct contract with both. + + + + +## Specific Ideas + +No specific UI/reference examples surfaced — this is a pure API/backend +phase (`UI hint: no` per ROADMAP.md). Discussion focused entirely on the +approve/remediate/audit contract shape. + + + + +## Deferred Ideas + +- **Real remediation provider wiring** (Mimecast block, Graph forwarding-rule + toggle, RMM isolate, password reset) — `REMEDEXEC-01..05`, explicitly v2. + D-01's simulated-effect model is the placeholder until a v2 phase picks one + provider at a time. +- **False-positive reversibility** — whether an operator can un-mark a false + positive later — not discussed, no signal it's needed for this phase; + planner should treat it as out of scope unless it blocks a success + criterion. +- **Multi-round approval** (approving additional actions after an initial + approve call) — noted as Claude's discretion (see ``), not a + locked requirement either way. + + + +--- + +*Phase: 20-remediation-approval-audit-safety* +*Context gathered: 2026-07-16* diff --git a/.planning/phases/20-remediation-approval-audit-safety/20-DISCUSSION-LOG.md b/.planning/phases/20-remediation-approval-audit-safety/20-DISCUSSION-LOG.md new file mode 100644 index 0000000..20663db --- /dev/null +++ b/.planning/phases/20-remediation-approval-audit-safety/20-DISCUSSION-LOG.md @@ -0,0 +1,79 @@ +# Phase 20: Remediation, Approval & Audit Safety - Discussion Log + +> **Audit trail only.** Do not use as input to planning, research, or execution agents. +> Decisions are captured in CONTEXT.md — this log preserves the alternatives considered. + +**Date:** 2026-07-16 +**Phase:** 20-remediation-approval-audit-safety +**Areas discussed:** Remediate scope, Permission grant, Approval scope, FP state guard + +--- + +## Remediate scope + +| Option | Description | Selected | +|--------|-------------|----------| +| Simulated internal effect | No real external API call for any action type. Remediate transitions `remediation_actions.status` to `completed` internally, writes the audit event, and returns success — the "effect" REMED-04's idempotency test asserts doesn't duplicate. All 7 action types behave the same way. | ✓ | +| Always not_implemented | Every remediate call returns not_implemented/501 for every action type, since no provider is configured. Pure safety-contract stub. | | +| Wire one real action for real | Actually implement `disable_forwarding_rule` via MS Graph — real effect for one action, not_implemented for the other 6. More scope/risk. | | + +**User's choice:** Simulated internal effect (Recommended) +**Notes:** Confirmed no provider client (Mimecast, MS Graph, RMM) has a block/purge/forwarding-rule/isolate method today, before presenting this question. + +--- + +## Permission grant + +| Option | Description | Selected | +|--------|-------------|----------| +| Admin + Super-Admin | Same tier as `analyze` today — parity with how this milestone has treated admin/super-admin identically so far. | ✓ | +| Super-Admin only | Stricter tier — approve/remediate can affect real security posture, so only super-admin gets it. | | + +**User's choice:** Admin + Super-Admin (Recommended) +**Notes:** Confirmed via direct read of `lib/permissions.ts` that both roles currently have only `phishing: ["read", "analyze"]` before asking. + +--- + +## Approval scope + +| Option | Description | Selected | +|--------|-------------|----------| +| Operator selects a subset | Request body includes which action(s) to approve and can override params (e.g. narrowing `block_sender` to a specific address). Matches REMED-02's "exact approved action parameters" wording. | ✓ | +| Approve the full set verbatim | One approve call locks in ALL recommended_actions exactly as proposed — simpler, less operator control. | | + +**User's choice:** Operator selects a subset (Recommended) +**Notes:** None. + +--- + +## FP state guard + +| Option | Description | Selected | +|--------|-------------|----------| +| Block after approve/remediate | mark-false-positive returns an explicit conflict/error if the campaign already has an approved or completed remediation_actions row. | ✓ | +| Always allow, just label it | mark-false-positive is a pure label change regardless of remediation state — simpler but permits a contradictory audit trail. | | + +**User's choice:** Block after approve/remediate (Recommended) +**Notes:** None. + +--- + +## Wrap-up check + +Asked whether any further gray areas needed discussion before writing context. + +**User's choice:** I'm ready for context (Recommended) + +## Claude's Discretion + +- Exact idempotency detection mechanism for `POST /remediate` re-runs. +- Exact `audit_events.actor` value format (likely session email). +- Whether `POST /approve` allows multiple approve calls over time vs. one-shot. +- Exact JSON request/response shapes for the three new routes. +- Whether a `not_implemented` code path still exists as a defensive fallback. + +## Deferred Ideas + +- Real remediation provider wiring (Mimecast block, Graph forwarding-rule, RMM isolate, password reset) — `REMEDEXEC-01..05`, v2. +- False-positive reversibility — not discussed, no signal it's needed. +- Multi-round approval — noted as Claude's discretion, not locked.