lorentz
672f17b7f9
Bundles several in-progress efforts that were sitting uncommitted: - User queue-preferences (migration 087, API route, popover component) - QBO invoice soft-delete (migration 088) and AR diagnostics route - Dashboard/mobile engagement route and page adjustments - Docker Compose log-rotation config - One-off ticket/RMM investigation scripts (scripts/) - Planning docs: phase verification/pattern notes, mobile shell design spec - .gitignore: exclude local scratch financial/inventory data and Claude Code worktree/local-settings runtime state (never meant for version control) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W6RuWdiUiXrPK6FLBHjtpY
105 lines
11 KiB
Markdown
105 lines
11 KiB
Markdown
---
|
|
phase: 20-remediation-approval-audit-safety
|
|
verified: 2026-07-16T11:00:00Z
|
|
status: passed
|
|
score: 6/6 must-haves verified
|
|
overrides_applied: 0
|
|
---
|
|
|
|
# Phase 20: Remediation Approval & Audit Safety Verification Report
|
|
|
|
**Phase Goal:** Deliver a remediation/approval/audit-safety layer where recommended
|
|
remediation actions are proposed-only until an operator explicitly approves them,
|
|
remediation execution is idempotent and gated, false-positive marking is
|
|
conflict-guarded, and every state-changing action (classify/approve/remediate/
|
|
mark-false-positive) is audit-logged and permission-gated.
|
|
|
|
**Verified:** 2026-07-16T11:00:00Z
|
|
**Status:** passed
|
|
**Re-verification:** No — initial verification
|
|
|
|
## Goal Achievement
|
|
|
|
### Observable Truths
|
|
|
|
| # | Truth | Status | Evidence |
|
|
|---|-------|--------|----------|
|
|
| 1 | REMED-01: Recommended actions are `proposed`, never auto-executed | VERIFIED | `lib/services/campaign-classifier.ts` has no INSERT into `remediation_actions`; `remediation_actions.status` defaults to `'proposed'` in migration 097; only `approveRemediationActions` creates `'approved'` rows, only on explicit operator call. |
|
|
| 2 | REMED-02: Operator can approve via `POST /approve`, recording approver, timestamp, exact params | VERIFIED | `app/api/phishing/campaigns/[id]/approve/route.ts` gates `requirePermission('phishing','approve')`, captures actor from `session.user.email` (never request body), delegates to `approveRemediationActions` which inserts `approved_by=actor, approved_at=NOW(), params=$3::jsonb` per action, validated against latest `classifications.recommended_actions`. Unit tests confirm reject-if-not-recommended and reject-if-no-classification. |
|
|
| 3 | REMED-03: `POST /remediate` proceeds only for approved actions, non-destructive-by-default (simulated), never silently succeeds | VERIFIED | `remediateApprovedActions` throws `RemediationValidationError` when zero `remediation_actions` rows exist (route maps to 400). Only `status='approved'` rows transition; effect is simulated (`UPDATE ... status='completed'`, no external provider call per D-01, confirmed via grep — 0 matches for `not_implemented` outside comments and no HTTP/client calls to Mimecast/other providers in this file). |
|
|
| 4 | REMED-04: Re-running remediation is idempotent | VERIFIED | `remediateApprovedActions` selects `FOR UPDATE` and only acts on `status='approved'` rows; already-`completed` rows are skipped (no UPDATE, no audit write). Test `is idempotent: a second call transitions nothing and writes no second audit row` asserts 0 additional UPDATE calls and audit-write count stays at 2 across two calls. |
|
|
| 5 | REMED-05: Operator can mark false positive via API, blocked when remediation approved/completed exists | VERIFIED | `mark-false-positive/route.ts` gated `requirePermission('phishing','approve')`, delegates to `markCampaignFalsePositive`, which runs a `SELECT ... WHERE status IN ('approved','completed') FOR UPDATE` guard before any write; a hit throws `RemediationConflictError` -> route returns 409. Test confirms guard trips and confirms happy-path sets `campaigns.status='false_positive'`. |
|
|
| 6 | REMED-06: Every state-changing action (classify/approve/remediate/mark-false-positive) writes an audit event | VERIFIED | `writeAuditEvent` is the single INSERT path into `audit_events` (grep confirms no other file inserts into this table). All three remediation-service functions call it inside the same `postgresClient.transaction` as their state write (atomic — rollback discards both). `classify/route.ts` was edited to call `writeAuditEvent({..., eventType:'campaign_classified'})` after `classifyCampaign()` succeeds, closing the fourth action. |
|
|
|
|
**Score:** 6/6 truths verified
|
|
|
|
### Required Artifacts
|
|
|
|
| Artifact | Expected | Status | Details |
|
|
|----------|----------|--------|---------|
|
|
| `lib/services/phishing-audit.ts` | `writeAuditEvent(input, client?)` single audit insert path | VERIFIED | 55 lines, parameterized INSERT (`$1`-`$4`), returns `RETURNING id::text`, accepts optional transaction client. |
|
|
| `lib/services/remediation-service.ts` | approve/remediate/mark-false-positive + typed errors | VERIFIED | 279 lines. Exports `approveRemediationActions`, `remediateApprovedActions`, `markCampaignFalsePositive`, `RemediationValidationError`, `RemediationConflictError` — all present and match plan signatures. |
|
|
| `lib/services/remediation-service.test.ts` | idempotency/audit/D-04/recommended-only proofs | VERIFIED | 259 lines, 8 real behavioral test cases (not tautological — fake transaction client records actual SQL calls and asserts on them). |
|
|
| `lib/services/phishing-audit.test.ts` | writer proof | VERIFIED | 3 tests: standalone insert, injected-client routing. |
|
|
| `lib/permissions.ts` | grants approve+remediate to admin/super-admin only | VERIFIED | `superAdminRole` and `adminRole` both `phishing: ["read","analyze","approve","remediate"]`; `userRole` remains `phishing: ["read"]`. `hasPermission()` enforces this at runtime (not just declared) — verified by reading `lib/auth-utils.ts` `requirePermission()` which calls `hasPermission(userRole, resource, action)` and returns 403 on failure. |
|
|
| `app/api/phishing/campaigns/[id]/approve/route.ts` | POST approve endpoint | VERIFIED | 77 lines. Permission gate, UUID guard, JSON body validation, actor from session, error mapping (400/409/500), delegates to service. |
|
|
| `app/api/phishing/campaigns/[id]/remediate/route.ts` | POST remediate endpoint | VERIFIED | 64 lines. Same shell, `phishing:remediate` gate, no body required. |
|
|
| `app/api/phishing/campaigns/[id]/mark-false-positive/route.ts` | POST mark-false-positive endpoint | VERIFIED | 76 lines. `phishing:approve` gate, optional-body tolerant JSON parse, 409 on conflict. |
|
|
| `app/api/phishing/campaigns/[id]/classify/route.ts` | audit event wiring | VERIFIED | Edited to destructure `session`, import `writeAuditEvent`, call it post-classify with `eventType:'campaign_classified'`. |
|
|
|
|
### Key Link Verification
|
|
|
|
| From | To | Via | Status | Details |
|
|
|------|-----|-----|--------|---------|
|
|
| `remediation-service.ts` | `audit_events` | `writeAuditEvent(client)` inside `postgresClient.transaction` | WIRED | All three functions call `writeAuditEvent(..., client)` using the same transaction client as their state write — atomic. |
|
|
| `remediation-service.ts` | `classifications.recommended_actions` | latest-classification read validates approvable types | WIRED | `approveRemediationActions` selects `ORDER BY created_at DESC LIMIT 1` and rejects non-recommended action types before any insert. |
|
|
| `remediation-service.ts` | `remediation_actions.status` | `FOR UPDATE` status filter drives idempotent completion | WIRED | Confirmed via grep + test (`FOR UPDATE` present in both `remediateApprovedActions` and the D-04 guard in `markCampaignFalsePositive`). |
|
|
| `approve/route.ts` | `approveRemediationActions` | `requirePermission('phishing','approve')` -> service delegation | WIRED | Route imports and calls the function directly with `(id, actions, actor)`; not a stub — errors from the service propagate to typed HTTP status mapping. |
|
|
| `remediate/route.ts` | `remediateApprovedActions` | `requirePermission('phishing','remediate')` -> service delegation | WIRED | Same pattern confirmed. |
|
|
| `mark-false-positive/route.ts` | `markCampaignFalsePositive` | `requirePermission('phishing','approve')` -> service delegation | WIRED | Same pattern confirmed. |
|
|
| `classify/route.ts` | `writeAuditEvent` | post-classify audit write | WIRED | Call is inside the existing try block after `classifyCampaign(id)` succeeds. |
|
|
| routes | `middleware.ts` | auth gate | WIRED | `/api/phishing/*` is NOT in `middleware.ts`'s public-route allowlist (confirmed via grep — zero matches), so unauthenticated requests are redirected/blocked before reaching route-level `requirePermission`. |
|
|
|
|
### Behavioral Spot-Checks
|
|
|
|
| Behavior | Command | Result | Status |
|
|
|----------|---------|--------|--------|
|
|
| Service unit tests (11 assertions across approve/remediate/mark-fp/audit-writer) | `npx vitest run lib/services/phishing-audit.test.ts lib/services/remediation-service.test.ts` | 2 files, 11 tests, all passed | PASS |
|
|
| Type check | `npx tsc --noEmit --pretty` | clean, no output | PASS |
|
|
| Full test suite regression | `npx vitest run` | 369/371 passing; 2 failures isolated to `lib/services/analyzer/itglue-search.test.ts` | PASS (pre-existing, out-of-scope failure confirmed — last touched by commit `8f8b5ab`, an unrelated earlier commit, not part of this phase's diff) |
|
|
| No auto-execution of remediation on classify | `grep -n "remediation_actions" lib/services/campaign-classifier.ts` | no matches | PASS |
|
|
| Single audit insert path | `grep -rn "INSERT INTO audit_events"` across `lib/` and `app/` | only in `phishing-audit.ts` | PASS |
|
|
|
|
### Requirements Coverage
|
|
|
|
| Requirement | Source Plan | Description | Status | Evidence |
|
|
|-------------|------------|-------------|--------|----------|
|
|
| REMED-01 | 20-01 | Proposed-only, never auto-executed | SATISFIED | See Truth 1 |
|
|
| REMED-02 | 20-01, 20-02 | Approve via API, records approver/timestamp/params | SATISFIED | See Truth 2 |
|
|
| REMED-03 | 20-01, 20-02 | Remediate only proceeds for approved, non-destructive, never silent success | SATISFIED | See Truth 3 |
|
|
| REMED-04 | 20-01 | Idempotent re-run | SATISFIED | See Truth 4 |
|
|
| REMED-05 | 20-01, 20-02 | Mark false positive via API | SATISFIED | See Truth 5 |
|
|
| REMED-06 | 20-01, 20-02 | Every state-changing action audited | SATISFIED | See Truth 6 |
|
|
|
|
Note: `.planning/REQUIREMENTS.md` still lists REMED-01..06 as "Pending" in its coverage table (lines 198-203) and unchecked (`[ ]`) in the requirement list — this is a tracking-document staleness issue, not a code gap. Recommend updating REQUIREMENTS.md status table as a follow-up, but it does not block phase goal achievement since the underlying code is verified.
|
|
|
|
### Anti-Patterns Found
|
|
|
|
None found in the phase's modified/created files. No `TODO`/`FIXME`/`HACK`/`PLACEHOLDER` markers, no empty handlers, no hardcoded empty returns feeding into rendering, no `not_implemented` code paths (only descriptive prose in comments, explicitly called out in 20-01-SUMMARY.md as a deliberate wording fix to avoid tripping the D-01 grep check).
|
|
|
|
### Human Verification Required
|
|
|
|
None. This phase is service + API layer only (no new UI in this phase — Phase 22 covers the approval UI). All behaviors are verifiable via code inspection, unit tests, and static grep checks.
|
|
|
|
### Gaps Summary
|
|
|
|
No gaps found. All six REMED requirements are backed by real, non-stub implementations:
|
|
- The service layer (`phishing-audit.ts`, `remediation-service.ts`) performs real parameterized SQL, real transactional atomicity between state writes and audit writes, and real validation logic (not pass-through stubs).
|
|
- The route layer enforces real permission checks (`requirePermission` calls `hasPermission` against actual role definitions, returning 403 on failure — not just declaring permissions without enforcing them).
|
|
- Idempotency and the D-04 conflict guard are proven by unit tests that assert on actual SQL call counts and audit-write counts, not just "does not throw."
|
|
- Actor identity is derived from the server session in all three new routes and in the classify route edit — never from request body, closing the spoofing/repudiation threat noted in the phase's own threat model.
|
|
|
|
---
|
|
|
|
_Verified: 2026-07-16T11:00:00Z_
|
|
_Verifier: Claude (gsd-verifier)_
|