From 50f48a8b6afece79779cf02f8f7d59d3bdb94db5 Mon Sep 17 00:00:00 2001 From: Matt Date: Tue, 12 May 2026 17:13:04 +0200 Subject: [PATCH] =?UTF-8?q?audit:=20Tier=202/3/4=20batch=20=E2=80=94=20rep?= =?UTF-8?q?orts=20math,=20portal=20copy,=20authz=20escalation=20guard?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tier 2.2: revenue PDF totalCompleted now filters on outcome='won' — setInterestOutcome forces stage='completed' for every outcome (incl. lost + cancelled), so the stage-only filter was including those toward "TOTAL COMPLETED REVENUE". Tier 2.3: fetchPipelineData stageCounts adds the missing .groupBy() — without it Postgres rejects the SELECT (per-stage breakdown was broken or coercing to ELSE-stage row). Tier 2.4: hot-deals widget rank ladder fixed two stage-name typos — 'in_comms' → 'in_communication', 'deposit_10' → 'deposit_10pct'. Both stages were collapsing to the ELSE 0 branch server-side AND rendering raw enum to the user in hot-deals-card.tsx. Tier 3.2: portal /portal/interests no longer renders raw enum to clients. New PORTAL_SIGNING_LABELS table maps every EOI/contract status to plain English (e.g. "waiting_for_signatures" → "Waiting for signatures"). Tier 4.1 (CRITICAL): permission-overrides PUT now requires caller- superset on every `true` write. Admins with only `admin.manage_users` could previously grant other users leaves they don't hold themselves (permanently_delete_clients, system_backup). Super-admins bypass. Tier 4.4: search graph-expansion re-gates every merged bucket by the destination's view permission. A user with berths.view but no interests.view searching "A12" no longer sees interest rows surfaced via expansion. Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/AUDIT-PARKED-QUESTIONS.md | 1 - src/app/(portal)/portal/interests/page.tsx | 30 ++++++++++++++- .../users/[id]/permission-overrides/route.ts | 18 +++++++++ src/app/api/v1/alerts/route.ts | 28 +++++++------- src/components/dashboard/hot-deals-card.tsx | 8 +++- src/lib/services/dashboard.service.ts | 8 +++- src/lib/services/report-generators.ts | 15 ++++++-- src/lib/services/search.service.ts | 37 ++++++++++++++++--- 8 files changed, 116 insertions(+), 29 deletions(-) diff --git a/docs/AUDIT-PARKED-QUESTIONS.md b/docs/AUDIT-PARKED-QUESTIONS.md index 8c8717c6..ea33c636 100644 --- a/docs/AUDIT-PARKED-QUESTIONS.md +++ b/docs/AUDIT-PARKED-QUESTIONS.md @@ -3,4 +3,3 @@ Items from the 33-agent audit that I deliberately left for you to decide on. Each one has the finding, why I parked it, and the proposed options. --- - diff --git a/src/app/(portal)/portal/interests/page.tsx b/src/app/(portal)/portal/interests/page.tsx index aa01c018..4a7b0b3d 100644 --- a/src/app/(portal)/portal/interests/page.tsx +++ b/src/app/(portal)/portal/interests/page.tsx @@ -9,6 +9,32 @@ import { stageLabel, safeStage, type PipelineStage } from '@/lib/constants'; export const metadata: Metadata = { title: 'Interests' }; +// Portal-friendly labels for signing-process status fields. The audit +// caught raw enum leak ("waiting_for_signatures" with underscores) at +// the client-facing surface. Map every known value to plain English; +// fall back to a Title-Case rendering for any new states. +const PORTAL_SIGNING_LABELS: Record = { + not_started: 'Not started', + draft: 'Drafted', + awaiting_them: 'Awaiting their signature', + awaiting_me: 'Awaiting your signature', + waiting_for_signatures: 'Waiting for signatures', + partially_signed: 'Partially signed', + sent: 'Sent for signing', + signed: 'Signed', + completed: 'Signed', + expired: 'Expired', + cancelled: 'Cancelled', + rejected: 'Rejected', +}; +function portalSigningLabel(status: string): string { + if (status in PORTAL_SIGNING_LABELS) return PORTAL_SIGNING_LABELS[status]!; + return status + .split('_') + .map((p) => (p ? p[0]!.toUpperCase() + p.slice(1) : p)) + .join(' '); +} + const STAGE_VARIANT: Record = { open: 'secondary', details_sent: 'secondary', @@ -77,10 +103,10 @@ export default async function PortalInterestsPage() { )} {interest.eoiStatus && ( - EOI: {interest.eoiStatus.replace(/_/g, ' ')} + EOI: {portalSigningLabel(interest.eoiStatus)} )} {interest.contractStatus && ( - Contract: {interest.contractStatus.replace(/_/g, ' ')} + Contract: {portalSigningLabel(interest.contractStatus)} )} diff --git a/src/app/api/v1/admin/users/[id]/permission-overrides/route.ts b/src/app/api/v1/admin/users/[id]/permission-overrides/route.ts index 93cc60bc..ccf348e5 100644 --- a/src/app/api/v1/admin/users/[id]/permission-overrides/route.ts +++ b/src/app/api/v1/admin/users/[id]/permission-overrides/route.ts @@ -181,6 +181,16 @@ export const PUT = withAuth( // here we drop unknown resources/actions so a malicious client // can't seed garbage keys that a future resolver might accidentally // honour. + // CALLER-SUPERSET (authz-auditor CRITICAL): an admin with only + // `admin.manage_users` previously could grant another user any + // permission leaf — including ones they don't hold themselves + // (e.g. `permanently_delete_clients`, `system_backup`). Require + // every `true` write to be a leaf the caller already has. + // Super-admins bypass (they hold all leaves by definition). + const callerPerms = ctx.permissions as Record< + string, + Record + > | null; const sanitized: Record> = {}; for (const [resource, actions] of Object.entries(overrides)) { const allowed = ALLOWED_RESOURCE_ACTIONS[resource]; @@ -193,6 +203,14 @@ export const PUT = withAuth( `permission override for ${resource}.${action} must be true or false`, ); } + if (value === true && !ctx.isSuperAdmin) { + const callerHas = callerPerms?.[resource]?.[action] === true; + if (!callerHas) { + throw new ForbiddenError( + `You don't hold ${resource}.${action} yourself, so you can't grant it.`, + ); + } + } cleaned[action] = value; } if (Object.keys(cleaned).length > 0) sanitized[resource] = cleaned; diff --git a/src/app/api/v1/alerts/route.ts b/src/app/api/v1/alerts/route.ts index b14ec219..415b41ab 100644 --- a/src/app/api/v1/alerts/route.ts +++ b/src/app/api/v1/alerts/route.ts @@ -10,22 +10,22 @@ type AlertStatus = 'open' | 'dismissed' | 'resolved'; // page uses. export const GET = withAuth( withPermission('admin', 'view_audit_log', async (req: NextRequest, ctx) => { - const url = new URL(req.url); - const status = (url.searchParams.get('status') ?? 'open') as AlertStatus; + const url = new URL(req.url); + const status = (url.searchParams.get('status') ?? 'open') as AlertStatus; - const rows = await listAlertsForPort(ctx.portId, { - includeDismissed: status !== 'open', - includeResolved: status !== 'open', - }); + const rows = await listAlertsForPort(ctx.portId, { + includeDismissed: status !== 'open', + includeResolved: status !== 'open', + }); - // Filter to the requested status bucket so callers don't see overlap. - const filtered = rows.filter((a) => { - if (status === 'open') return !a.dismissedAt && !a.resolvedAt; - if (status === 'dismissed') return Boolean(a.dismissedAt) && !a.resolvedAt; - if (status === 'resolved') return Boolean(a.resolvedAt); - return true; - }); + // Filter to the requested status bucket so callers don't see overlap. + const filtered = rows.filter((a) => { + if (status === 'open') return !a.dismissedAt && !a.resolvedAt; + if (status === 'dismissed') return Boolean(a.dismissedAt) && !a.resolvedAt; + if (status === 'resolved') return Boolean(a.resolvedAt); + return true; + }); - return NextResponse.json({ data: filtered }); + return NextResponse.json({ data: filtered }); }), ); diff --git a/src/components/dashboard/hot-deals-card.tsx b/src/components/dashboard/hot-deals-card.tsx index 2fdf170a..b292d571 100644 --- a/src/components/dashboard/hot-deals-card.tsx +++ b/src/components/dashboard/hot-deals-card.tsx @@ -23,13 +23,17 @@ interface HotDealsResponse { data: HotDeal[]; } +// Local label map intentionally narrowed to the stages this widget +// surfaces. Keys MUST match the canonical DB values (deposit_10pct + +// in_communication) — the reporting audit caught typos that broke the +// rank ladder server-side AND rendered raw enum to the user. const STAGE_LABELS: Record = { contract_signed: 'Contract Signed', contract_sent: 'Contract Sent', - deposit_10: 'Deposit 10%', + deposit_10pct: 'Deposit 10%', eoi_signed: 'EOI Signed', eoi_sent: 'EOI Sent', - in_comms: 'In Comms', + in_communication: 'In Comms', details_sent: 'Details Sent', open: 'Open', completed: 'Completed', diff --git a/src/lib/services/dashboard.service.ts b/src/lib/services/dashboard.service.ts index 0979ff81..12aae778 100644 --- a/src/lib/services/dashboard.service.ts +++ b/src/lib/services/dashboard.service.ts @@ -195,14 +195,18 @@ export async function getBerthStatusDistribution(portId: string) { */ export async function getHotDeals(portId: string, limit = 5) { // Stage rank: bigger = closer to closing. + // Reporting audit caught two stage-name typos: 'in_comms' and + // 'deposit_10' don't exist in the DB enum — canonical values are + // 'in_communication' and 'deposit_10pct'. Those two stages were + // silently collapsing to the ELSE 0 branch. const rank = sql`CASE ${interests.pipelineStage} WHEN 'completed' THEN 8 WHEN 'contract_signed' THEN 7 WHEN 'contract_sent' THEN 6 - WHEN 'deposit_10' THEN 5 + WHEN 'deposit_10pct' THEN 5 WHEN 'eoi_signed' THEN 4 WHEN 'eoi_sent' THEN 3 - WHEN 'in_comms' THEN 2 + WHEN 'in_communication' THEN 2 WHEN 'details_sent' THEN 1 ELSE 0 END`; diff --git a/src/lib/services/report-generators.ts b/src/lib/services/report-generators.ts index f1a7c8bf..467aeb9c 100644 --- a/src/lib/services/report-generators.ts +++ b/src/lib/services/report-generators.ts @@ -50,14 +50,18 @@ export async function fetchPipelineData( portId: string, _params: Record, ): Promise { - // Count interests per pipeline stage (non-archived) + // Count interests per pipeline stage (non-archived). + // The reporting audit caught the missing .groupBy() — without it, + // postgres rejects the SELECT or collapses every interest into a + // single ELSE-stage row. groupBy fixes the per-stage breakdown. const stageCounts = await db .select({ stage: interests.pipelineStage, count: count(), }) .from(interests) - .where(and(eq(interests.portId, portId), isNull(interests.archivedAt))); + .where(and(eq(interests.portId, portId), isNull(interests.archivedAt))) + .groupBy(interests.pipelineStage); const stageCountMap: Record = {}; for (const row of stageCounts) { @@ -122,7 +126,11 @@ export async function fetchRevenueData( stageRevenueMap[row.stage] = row.revenue ? String(row.revenue) : '0'; } - // Total revenue from completed interests (primary-berth link only). + // Total revenue from WON interests only. Reporting audit caught the + // gap: setInterestOutcome forces pipelineStage='completed' for lost + // AND cancelled outcomes too, so filtering by stage alone counted + // those toward "TOTAL COMPLETED REVENUE". The outcome='won' filter is + // the canonical money-changed-hands signal. const completedRevenue = await db .select({ total: sum(berths.price) }) .from(interests) @@ -135,6 +143,7 @@ export async function fetchRevenueData( and( eq(interests.portId, portId), eq(interests.pipelineStage, 'completed'), + eq(interests.outcome, 'won'), isNull(interests.archivedAt), ), ); diff --git a/src/lib/services/search.service.ts b/src/lib/services/search.service.ts index 59ea9a97..c20c43d3 100644 --- a/src/lib/services/search.service.ts +++ b/src/lib/services/search.service.ts @@ -1908,11 +1908,38 @@ export async function search( // Merge direct matches with expansion rows; direct rows always win // ties and sort first. Each bucket caps at `limit * 2` so reps still // see the full direct-match set plus a healthy expansion tail. - const mergedClients = mergeWithExpansion(clients, expanded.clients, limit * 2); - const mergedInterests = mergeWithExpansion(interests, expanded.interests, limit * 2); - const mergedYachts = mergeWithExpansion(yachts, expanded.yachts, limit * 2); - const mergedCompanies = mergeWithExpansion(companies, expanded.companies, limit * 2); - const mergedBerths = mergeWithExpansion(berths, expanded.berths, limit * 2); + // + // SECURITY (search-auditor H1): expandGraph runs unconditionally, + // but its results MUST be re-gated by the destination bucket's view + // permission. A user with berths.view but no interests.view searching + // "A12" was previously getting interest rows (client name + stage) + // surfaced via expansion. Gate each merge call so the expansion + // contributes empty rows for any bucket the caller can't see. + const mergedClients = mergeWithExpansion( + clients, + can(opts, 'clients.view') ? expanded.clients : [], + limit * 2, + ); + const mergedInterests = mergeWithExpansion( + interests, + can(opts, 'interests.view') ? expanded.interests : [], + limit * 2, + ); + const mergedYachts = mergeWithExpansion( + yachts, + can(opts, 'yachts.view') ? expanded.yachts : [], + limit * 2, + ); + const mergedCompanies = mergeWithExpansion( + companies, + can(opts, 'companies.view') ? expanded.companies : [], + limit * 2, + ); + const mergedBerths = mergeWithExpansion( + berths, + can(opts, 'berths.view') ? expanded.berths : [], + limit * 2, + ); const result: SearchResults = { clients: apply(mergedClients),