From 7c2cb75f8f4d433fa80d031b8117bc72e6954370 Mon Sep 17 00:00:00 2001 From: Tofik Hasanov Date: Fri, 24 Jul 2026 00:00:54 -0400 Subject: [PATCH 1/2] fix(risks): cs-727 deploy-review fixes - acceptance create runs in a transaction with a FOR UPDATE row-lock on the subject, so a concurrent residual edit can no longer produce an acceptance frozen at a superseded rating - approve() collects the drift baseline INSIDE the approval transaction (collectPlatformData gained an optional client), so a just-approved document can't immediately read as stale - useAcceptances surfaces ApiResponse.error (apiClient never throws); the acceptance card renders an error + retry state instead of misreporting a failed load as "no acceptance recorded" - acceptance dialog only preselects the owner when they are selectable - RTP submit stays blocked while readiness is loading or failed - methodology matrix intro states score bands AND raw-product bands (the previous wording misattributed raw ranges to the score) - readiness queries extracted to risk-treatment-readiness.ts - acceptance response schemas document authType/authenticatedUser + 500 - comment fixes: 10-column table note, ISO 27001 attribution --- apps/api/src/isms/documents/data-source.ts | 34 ++++--- .../documents/risk-methodology-defaults.ts | 2 +- .../src/isms/documents/risk-methodology.ts | 2 +- .../documents/risk-treatment-readiness.ts | 34 +++++++ .../api/src/isms/isms.service.approve.spec.ts | 3 + apps/api/src/isms/isms.service.ts | 52 +++-------- apps/api/src/isms/utils/pdf-renderer.ts | 2 +- .../risks/risk-acceptances.service.spec.ts | 22 +++++ .../api/src/risks/risk-acceptances.service.ts | 91 ++++++++++++------- .../schemas/risk-acceptances.responses.ts | 48 +++++++++- .../components/RiskLevelMatrixPreview.tsx | 5 +- .../components/RiskTreatmentPlanClient.tsx | 12 ++- .../acceptance/RecordAcceptanceDialog.tsx | 12 ++- .../acceptance/ResidualAcceptanceCard.tsx | 20 +++- apps/app/src/hooks/use-risk-acceptances.ts | 9 +- 15 files changed, 247 insertions(+), 101 deletions(-) create mode 100644 apps/api/src/isms/documents/risk-treatment-readiness.ts diff --git a/apps/api/src/isms/documents/data-source.ts b/apps/api/src/isms/documents/data-source.ts index 50c2577122..e76efba692 100644 --- a/apps/api/src/isms/documents/data-source.ts +++ b/apps/api/src/isms/documents/data-source.ts @@ -1,5 +1,6 @@ import { createHash } from 'node:crypto'; import { db } from '@db'; +import type { Prisma } from '@db'; import { parseStoredAnswers } from '../wizard/wizard-schema'; import type { IsmsPlatformData } from './types'; @@ -16,10 +17,19 @@ const HIGH_IMPACT = ['major', 'severe']; export async function collectPlatformData({ organizationId, frameworkId, + client, }: { organizationId: string; frameworkId: string; + /** + * Optional transaction client. The approval flow passes its transaction so + * the drift baseline is read at the SAME point in time as the rows frozen + * into the published version — otherwise a concurrent edit between the two + * reads makes a just-approved document immediately show as stale. + */ + client?: Prisma.TransactionClient; }): Promise { + const dbc = client ?? db; const [ organization, frameworkInstances, @@ -34,15 +44,15 @@ export async function collectPlatformData({ partiesRows, acceptanceRows, ] = await Promise.all([ - db.organization.findUnique({ + dbc.organization.findUnique({ where: { id: organizationId }, select: { name: true }, }), - db.frameworkInstance.findMany({ + dbc.frameworkInstance.findMany({ where: { organizationId }, select: { framework: { select: { name: true } } }, }), - db.vendor.findMany({ + dbc.vendor.findMany({ where: { organizationId }, select: { id: true, @@ -61,14 +71,14 @@ export async function collectPlatformData({ assignee: { select: { user: { select: { name: true, email: true } } } }, }, }), - db.member.count({ where: { organizationId, deactivated: false } }), - db.member.groupBy({ + dbc.member.count({ where: { organizationId, deactivated: false } }), + dbc.member.groupBy({ by: ['department'], where: { organizationId, deactivated: false }, _count: { _all: true }, }), - db.device.count({ where: { organizationId } }), - db.risk.findMany({ + dbc.device.count({ where: { organizationId } }), + dbc.risk.findMany({ where: { organizationId }, select: { id: true, @@ -86,18 +96,18 @@ export async function collectPlatformData({ assignee: { select: { user: { select: { name: true, email: true } } } }, }, }), - db.employeeTrainingVideoCompletion.count({ + dbc.employeeTrainingVideoCompletion.count({ where: { member: { organizationId } }, }), - db.frameworkEditorFramework.findUnique({ + dbc.frameworkEditorFramework.findUnique({ where: { id: frameworkId }, select: { name: true }, }), - db.ismsProfile.findUnique({ + dbc.ismsProfile.findUnique({ where: { organizationId_frameworkId: { organizationId, frameworkId } }, select: { answers: true }, }), - db.ismsInterestedParty.findMany({ + dbc.ismsInterestedParty.findMany({ where: { document: { organizationId, @@ -107,7 +117,7 @@ export async function collectPlatformData({ }, select: { id: true, name: true, category: true }, }), - db.riskAcceptance.findMany({ + dbc.riskAcceptance.findMany({ where: { organizationId }, select: { id: true, riskId: true, vendorId: true }, }), diff --git a/apps/api/src/isms/documents/risk-methodology-defaults.ts b/apps/api/src/isms/documents/risk-methodology-defaults.ts index 9f5200ab9b..380fce36b0 100644 --- a/apps/api/src/isms/documents/risk-methodology-defaults.ts +++ b/apps/api/src/isms/documents/risk-methodology-defaults.ts @@ -3,7 +3,7 @@ // behavior: the five-band risk level scale (very-low .. very-high, computed as // likelihood x impact in apps/api/src/risks/risk-level.ts) and the platform's // four treatment strategies (mitigate/avoid/transfer/accept) with their ISO -// 27002 option names (Modify/Avoid/Share/Retain). Keep text ASCII-safe: the +// 27001 option names (Modify/Avoid/Share/Retain). Keep text ASCII-safe: the // PDF renderer's standard fonts cannot render glyphs like ">=" ligatures. /** Platform likelihood levels, in ascending order (matches the Likelihood enum). */ diff --git a/apps/api/src/isms/documents/risk-methodology.ts b/apps/api/src/isms/documents/risk-methodology.ts index 4433b0388b..2ff5abd58c 100644 --- a/apps/api/src/isms/documents/risk-methodology.ts +++ b/apps/api/src/isms/documents/risk-methodology.ts @@ -168,7 +168,7 @@ export function buildRiskMethodologySections( { heading: 'Risk level matrix', intro: - 'The risk level is derived from the product of likelihood and impact (1-25), normalized to a 1-10 score and mapped to five bands: Very low (1-5), Low (6-10), Medium (11-15), High (16-20), and Very high (21-25). Each risk in the register carries its calculated level for both its inherent and residual states.', + 'The risk level is derived from the product of likelihood and impact (1-25), normalized to a 1-10 score (the product divided by 2.5, rounded up) and banded by score: Very low (score 1-2), Low (3-4), Medium (5-6), High (7-8), Very high (9-10). Expressed as the raw product, the bands are: Very low 1-5, Low 6-10, Medium 11-15, High 16-20, Very high 21-25. Each risk in the register carries its calculated level for both its inherent and residual states.', table: riskMatrixTable(), }, { diff --git a/apps/api/src/isms/documents/risk-treatment-readiness.ts b/apps/api/src/isms/documents/risk-treatment-readiness.ts new file mode 100644 index 0000000000..0cb7dfca89 --- /dev/null +++ b/apps/api/src/isms/documents/risk-treatment-readiness.ts @@ -0,0 +1,34 @@ +import { db } from '@db'; +import type { Prisma } from '@db'; +import { riskTreatmentValidationMessages } from './risk-treatment-plan'; + +/** + * Clause-6.1.3 readiness: the RTP renders from the platform Risk Register + + * Vendors (org-scoped), not from register rows of its own — so readiness reads + * those tables. Archived risks are out of the plan (see + * loadRiskTreatmentExtras). Shared by the submit gate and the page payload. + */ +export async function loadRiskTreatmentReadinessMessages({ + organizationId, + client, +}: { + organizationId: string; + client?: Prisma.TransactionClient; +}): Promise { + const dbc = client ?? db; + const [risks, vendors] = await Promise.all([ + dbc.risk.findMany({ + where: { organizationId, status: { not: 'archived' } }, + select: { assigneeId: true }, + }), + dbc.vendor.findMany({ + where: { organizationId }, + select: { assigneeId: true }, + }), + ]); + return riskTreatmentValidationMessages({ + riskCount: risks.length, + risksWithoutOwner: risks.filter((risk) => !risk.assigneeId).length, + vendorsWithoutOwner: vendors.filter((vendor) => !vendor.assigneeId).length, + }); +} diff --git a/apps/api/src/isms/isms.service.approve.spec.ts b/apps/api/src/isms/isms.service.approve.spec.ts index 3b1f19af1d..2ca36806e0 100644 --- a/apps/api/src/isms/isms.service.approve.spec.ts +++ b/apps/api/src/isms/isms.service.approve.spec.ts @@ -140,9 +140,12 @@ describe('IsmsService.approve (CS-701 versioning)', () => { await service.approve(args); + // The baseline is collected INSIDE the approval transaction (client + // threaded) so it reads the same point in time as the frozen rows. expect(mockCollect).toHaveBeenCalledWith({ organizationId: 'org_1', frameworkId: 'fw_1', + client: expect.anything(), }); // Re-derives in-tx from the same snapshot: persisted rows and the frozen // version come from one pass. diff --git a/apps/api/src/isms/isms.service.ts b/apps/api/src/isms/isms.service.ts index 0788a2761a..40cb758edb 100644 --- a/apps/api/src/isms/isms.service.ts +++ b/apps/api/src/isms/isms.service.ts @@ -26,7 +26,7 @@ import { import { defaultProcedureText } from './documents/management-review-defaults'; import { defaultRiskMethodologyNarrative } from './documents/risk-methodology'; import { loadRiskTreatmentExtras } from './documents/risk-treatment-export-data'; -import { riskTreatmentValidationMessages } from './documents/risk-treatment-plan'; +import { loadRiskTreatmentReadinessMessages } from './documents/risk-treatment-readiness'; import { updateDraftSnapshot } from './utils/draft-snapshot'; import { EXPORT_DOCUMENT_INCLUDE } from './utils/export-payload'; import { lockDocument } from './utils/document-lock'; @@ -450,10 +450,6 @@ export class IsmsService { const document = await this.requireDocument({ documentId, organizationId }); this.assertPendingApprovalBy({ document, member }); - const snapshot = await collectPlatformData({ - organizationId, - frameworkId: document.frameworkId, - }); const now = new Date(); // Freeze the draft into a new immutable published version and promote it to @@ -464,6 +460,16 @@ export class IsmsService { // same lock) on this document so they can't interleave and double-publish. await lockDocument(tx, documentId); + // Collect the drift baseline INSIDE the transaction so it reads the same + // point in time as the rows frozen into the published version below — a + // concurrent platform edit can no longer make a just-approved document + // immediately stale. + const snapshot = await collectPlatformData({ + organizationId, + frameworkId: document.frameworkId, + client: tx, + }); + // Atomically claim the approval: the check-then-act guard above runs before // the transaction, so under READ COMMITTED a racing approve/decline could // read the same stale `needs_review`. This conditional update only matches @@ -717,38 +723,6 @@ export class IsmsService { } } - /** - * Clause-6.1.3 readiness: the RTP renders from the platform Risk Register + - * Vendors (org-scoped), not from register rows of its own — so readiness - * reads those tables. Archived risks are out of the plan (see - * loadRiskTreatmentExtras). Shared by the submit gate and the page payload. - */ - private async riskTreatmentReadinessMessages({ - organizationId, - client, - }: { - organizationId: string; - client?: Prisma.TransactionClient; - }): Promise { - const dbc = client ?? db; - const [risks, vendors] = await Promise.all([ - dbc.risk.findMany({ - where: { organizationId, status: { not: 'archived' } }, - select: { assigneeId: true }, - }), - dbc.vendor.findMany({ - where: { organizationId }, - select: { assigneeId: true }, - }), - ]); - return riskTreatmentValidationMessages({ - riskCount: risks.length, - risksWithoutOwner: risks.filter((risk) => !risk.assigneeId).length, - vendorsWithoutOwner: vendors.filter((vendor) => !vendor.assigneeId) - .length, - }); - } - private async assertRiskTreatmentPlanComplete({ tx, organizationId, @@ -756,7 +730,7 @@ export class IsmsService { tx: Prisma.TransactionClient; organizationId: string; }) { - const messages = await this.riskTreatmentReadinessMessages({ + const messages = await loadRiskTreatmentReadinessMessages({ organizationId, client: tx, }); @@ -788,7 +762,7 @@ export class IsmsService { } const [extras, validationMessages] = await Promise.all([ loadRiskTreatmentExtras({ organizationId }), - this.riskTreatmentReadinessMessages({ organizationId }), + loadRiskTreatmentReadinessMessages({ organizationId }), ]); return { ...extras, validationMessages }; } diff --git a/apps/api/src/isms/utils/pdf-renderer.ts b/apps/api/src/isms/utils/pdf-renderer.ts index 73fa84e369..e920794315 100644 --- a/apps/api/src/isms/utils/pdf-renderer.ts +++ b/apps/api/src/isms/utils/pdf-renderer.ts @@ -115,7 +115,7 @@ export function renderIsmsPdf({ const threeCol = table.headers.length === 3; const cellFills = table.cellFills; // Never let a header word break mid-word ("Acceptanc/e"): floor each - // column at its header's longest word. Wide tables (the 6.1.3 plan has 9 + // column at its header's longest word. Wide tables (the 6.1.3 plan has 10 // columns) otherwise get squeezed below the header width by autotable's // content-proportional sizing. pdf.setFont('helvetica', 'bold'); diff --git a/apps/api/src/risks/risk-acceptances.service.spec.ts b/apps/api/src/risks/risk-acceptances.service.spec.ts index 5a8147dde2..5d2ccbeefb 100644 --- a/apps/api/src/risks/risk-acceptances.service.spec.ts +++ b/apps/api/src/risks/risk-acceptances.service.spec.ts @@ -5,6 +5,12 @@ const mockDb = { vendor: { findFirst: jest.fn() }, member: { findFirst: jest.fn() }, riskAcceptance: { findMany: jest.fn(), create: jest.fn() }, + // The create paths run inside a transaction with a subject row-lock; the + // callback receives this same mock as the transaction client. + $transaction: jest.fn( + (fn: (tx: typeof mockDb) => unknown): unknown => fn(mockDb), + ), + $queryRaw: jest.fn().mockResolvedValue([]), }; jest.mock('@db', () => ({ db: mockDb })); @@ -67,6 +73,22 @@ describe('RiskAcceptancesService', () => { expect(view.levelLabel).toBe('Very low'); }); + it('row-locks the risk and writes through the same transaction', async () => { + mockDb.risk.findFirst.mockResolvedValue(baseRisk); + mockDb.member.findFirst.mockResolvedValue(activeMember); + mockDb.riskAcceptance.create.mockResolvedValue(storedRow); + + await service.createForRisk('rsk_1', ORG, {}); + + // The subject lock serializes concurrent residual edits with the + // read-freeze-insert sequence, so a fresh acceptance can never be + // recorded against an already-superseded rating. + expect(mockDb.$transaction).toHaveBeenCalledTimes(1); + expect(mockDb.$queryRaw).toHaveBeenCalledTimes(1); + const rawQuery = mockDb.$queryRaw.mock.calls[0][0].join('?'); + expect(rawQuery).toContain('FOR UPDATE'); + }); + it('defaults the acceptor to the risk owner (assignee)', async () => { mockDb.risk.findFirst.mockResolvedValue(baseRisk); mockDb.member.findFirst.mockResolvedValue(activeMember); diff --git a/apps/api/src/risks/risk-acceptances.service.ts b/apps/api/src/risks/risk-acceptances.service.ts index a568037e29..c28f273815 100644 --- a/apps/api/src/risks/risk-acceptances.service.ts +++ b/apps/api/src/risks/risk-acceptances.service.ts @@ -3,7 +3,13 @@ import { Injectable, NotFoundException, } from '@nestjs/common'; -import { db, type Impact, type Likelihood, type RiskAcceptance } from '@db'; +import { + db, + type Impact, + type Likelihood, + type Prisma, + type RiskAcceptance, +} from '@db'; import { CreateRiskAcceptanceDto } from './dto/create-risk-acceptance.dto'; import { LEVEL_LABEL, ratingLevel, type RiskLevel } from './risk-level'; @@ -70,27 +76,34 @@ export class RiskAcceptancesService { */ assertAccess?: (risk: { assigneeId: string | null }) => void, ): Promise { - const risk = await db.risk.findFirst({ - where: { id: riskId, organizationId }, - select: { - assigneeId: true, - residualLikelihood: true, - residualImpact: true, - }, - }); - if (!risk) { - throw new NotFoundException( - `Risk with ID ${riskId} not found in organization ${organizationId}`, - ); - } - assertAccess?.(risk); - - return this.createAcceptance({ - organizationId, - subject: { riskId }, - dto, - ownerMemberId: risk.assigneeId, - current: risk, + // Row-lock the risk for the whole read-freeze-insert sequence: a + // concurrent residual PATCH blocks until this commits, so the frozen + // rating is always the rating at acceptance time (never instantly stale). + return db.$transaction(async (tx) => { + await tx.$queryRaw`SELECT id FROM "Risk" WHERE id = ${riskId} FOR UPDATE`; + const risk = await tx.risk.findFirst({ + where: { id: riskId, organizationId }, + select: { + assigneeId: true, + residualLikelihood: true, + residualImpact: true, + }, + }); + if (!risk) { + throw new NotFoundException( + `Risk with ID ${riskId} not found in organization ${organizationId}`, + ); + } + assertAccess?.(risk); + + return this.createAcceptance({ + tx, + organizationId, + subject: { riskId }, + dto, + ownerMemberId: risk.assigneeId, + current: risk, + }); }); } @@ -109,19 +122,28 @@ export class RiskAcceptancesService { organizationId: string, dto: CreateRiskAcceptanceDto, ): Promise { - const vendor = await this.findVendorRating(vendorId, organizationId); + // Same row-lock rationale as createForRisk. + return db.$transaction(async (tx) => { + await tx.$queryRaw`SELECT id FROM "Vendor" WHERE id = ${vendorId} FOR UPDATE`; + const vendor = await this.findVendorRating(vendorId, organizationId, tx); - return this.createAcceptance({ - organizationId, - subject: { vendorId }, - dto, - ownerMemberId: vendor.assigneeId, - current: vendor.rating, + return this.createAcceptance({ + tx, + organizationId, + subject: { vendorId }, + dto, + ownerMemberId: vendor.assigneeId, + current: vendor.rating, + }); }); } - private async findVendorRating(vendorId: string, organizationId: string) { - const vendor = await db.vendor.findFirst({ + private async findVendorRating( + vendorId: string, + organizationId: string, + client?: Prisma.TransactionClient, + ) { + const vendor = await (client ?? db).vendor.findFirst({ where: { id: vendorId, organizationId }, select: { assigneeId: true, @@ -160,13 +182,14 @@ export class RiskAcceptancesService { } private async createAcceptance(params: { + tx: Prisma.TransactionClient; organizationId: string; subject: { riskId: string } | { vendorId: string }; dto: CreateRiskAcceptanceDto; ownerMemberId: string | null; current: ResidualRating; }): Promise { - const { organizationId, subject, dto, ownerMemberId, current } = params; + const { tx, organizationId, subject, dto, ownerMemberId, current } = params; const acceptorId = dto.acceptedById ?? ownerMemberId; if (!acceptorId) { @@ -175,7 +198,7 @@ export class RiskAcceptancesService { ); } - const member = await db.member.findFirst({ + const member = await tx.member.findFirst({ where: { id: acceptorId, organizationId }, select: { deactivated: true, @@ -193,7 +216,7 @@ export class RiskAcceptancesService { ); } - const row = await db.riskAcceptance.create({ + const row = await tx.riskAcceptance.create({ data: { organizationId, ...subject, diff --git a/apps/api/src/risks/schemas/risk-acceptances.responses.ts b/apps/api/src/risks/schemas/risk-acceptances.responses.ts index 52ddbc5ece..c6fb7b62fe 100644 --- a/apps/api/src/risks/schemas/risk-acceptances.responses.ts +++ b/apps/api/src/risks/schemas/risk-acceptances.responses.ts @@ -81,6 +81,39 @@ const NOT_FOUND = (entity: string): ApiResponseOptions => ({ }, }); +// Auth fields the controllers spread into every response (sibling pattern: +// get-risk-by-id.responses.ts). +const AUTH_RESPONSE_PROPERTIES = { + authType: { + type: 'string', + enum: ['api-key', 'session'], + description: 'How the request was authenticated', + }, + authenticatedUser: { + type: 'object', + description: 'User information (only for session auth)', + properties: { + id: { type: 'string', example: 'usr_def456ghi789' }, + email: { type: 'string', example: 'user@example.com' }, + }, + }, +}; + +const INTERNAL_ERROR: ApiResponseOptions = { + status: 500, + description: 'Internal server error', + content: { + 'application/json': { + schema: { + type: 'object', + properties: { + message: { type: 'string', example: 'Internal server error' }, + }, + }, + }, + }, +}; + const UNAUTHORIZED: ApiResponseOptions = { status: 401, description: 'Unauthorized - Invalid authentication', @@ -128,6 +161,7 @@ export const LIST_RISK_ACCEPTANCES_RESPONSES: Record< type: 'object', properties: { data: { type: 'array', items: RISK_ACCEPTANCE_SCHEMA }, + ...AUTH_RESPONSE_PROPERTIES, }, }, }, @@ -136,6 +170,7 @@ export const LIST_RISK_ACCEPTANCES_RESPONSES: Record< 401: UNAUTHORIZED, 403: FORBIDDEN, 404: NOT_FOUND('Risk'), + 500: INTERNAL_ERROR, }; export const RECORD_RISK_ACCEPTANCE_RESPONSES: Record< @@ -145,7 +180,17 @@ export const RECORD_RISK_ACCEPTANCE_RESPONSES: Record< 201: { status: 201, description: 'Acceptance recorded successfully', - content: { 'application/json': { schema: RISK_ACCEPTANCE_SCHEMA } }, + content: { + 'application/json': { + schema: { + type: 'object', + properties: { + ...RISK_ACCEPTANCE_SCHEMA.properties, + ...AUTH_RESPONSE_PROPERTIES, + }, + }, + }, + }, }, 400: { status: 400, @@ -168,4 +213,5 @@ export const RECORD_RISK_ACCEPTANCE_RESPONSES: Record< 401: UNAUTHORIZED, 403: FORBIDDEN, 404: NOT_FOUND('Risk'), + 500: INTERNAL_ERROR, }; diff --git a/apps/app/src/app/(app)/[orgId]/documents/isms/components/RiskLevelMatrixPreview.tsx b/apps/app/src/app/(app)/[orgId]/documents/isms/components/RiskLevelMatrixPreview.tsx index 221275a7e8..083bf91173 100644 --- a/apps/app/src/app/(app)/[orgId]/documents/isms/components/RiskLevelMatrixPreview.tsx +++ b/apps/app/src/app/(app)/[orgId]/documents/isms/components/RiskLevelMatrixPreview.tsx @@ -59,8 +59,9 @@ export function RiskLevelMatrixPreview() { Computed from likelihood x impact (1-25), normalized to the same 1-10 score the risk - badges use: Very low (1-5), Low (6-10), Medium (11-15), High (16-20), Very high (21-25). - This table renders into the document and is not editable. + badges use (score bands: Very low 1-2, Low 3-4, Medium 5-6, High 7-8, Very high 9-10 — + i.e. raw products 1-5, 6-10, 11-15, 16-20, 21-25). This table renders into the document + and is not editable. ); diff --git a/apps/app/src/app/(app)/[orgId]/documents/isms/components/RiskTreatmentPlanClient.tsx b/apps/app/src/app/(app)/[orgId]/documents/isms/components/RiskTreatmentPlanClient.tsx index 6c06c1a34e..7d68482f90 100644 --- a/apps/app/src/app/(app)/[orgId]/documents/isms/components/RiskTreatmentPlanClient.tsx +++ b/apps/app/src/app/(app)/[orgId]/documents/isms/components/RiskTreatmentPlanClient.tsx @@ -29,10 +29,16 @@ export function RiskTreatmentPlanClient(props: RiskTreatmentPlanClientProps) { const { riskTreatment, error, isLoading, mutateRiskTreatment } = useIsmsRiskTreatment(documentId); - const blockedReason = - riskTreatment && riskTreatment.validationMessages.length > 0 + // null = ready to submit. While readiness is unknown (loading/failed) keep + // Submit blocked so the approval dialog can't be completed only to hit the + // server-side completeness error. + const blockedReason = riskTreatment + ? riskTreatment.validationMessages.length > 0 ? riskTreatment.validationMessages.join(' ') - : null; + : null + : error + ? 'The readiness check could not be loaded — refresh the page and try again.' + : 'Checking the plan is ready to submit...'; return ( option.id === defaultAcceptorId, + ) + ? (defaultAcceptorId ?? '') + : ''; const { control, handleSubmit, @@ -70,7 +78,7 @@ export function RecordAcceptanceDialog({ formState: { isSubmitting, errors }, } = useForm({ resolver: zodResolver(acceptanceSchema), - defaultValues: { acceptedById: defaultAcceptorId ?? '', notes: '' }, + defaultValues: { acceptedById: validDefaultId, notes: '' }, }); const acceptorId = watch('acceptedById'); @@ -78,7 +86,7 @@ export function RecordAcceptanceDialog({ acceptorOptions.find((option) => option.id === acceptorId)?.name ?? 'the selected member'; const handleClose = (next: boolean) => { - if (!next) reset({ acceptedById: defaultAcceptorId ?? '', notes: '' }); + if (!next) reset({ acceptedById: validDefaultId, notes: '' }); onOpenChange(next); }; diff --git a/apps/app/src/components/risks/acceptance/ResidualAcceptanceCard.tsx b/apps/app/src/components/risks/acceptance/ResidualAcceptanceCard.tsx index cc66f3e7e9..d0a84417d5 100644 --- a/apps/app/src/components/risks/acceptance/ResidualAcceptanceCard.tsx +++ b/apps/app/src/components/risks/acceptance/ResidualAcceptanceCard.tsx @@ -60,7 +60,10 @@ export function ResidualAcceptanceCard({ acceptorOptions, canUpdate, }: ResidualAcceptanceCardProps) { - const { acceptances, latest, isLoading, recordAcceptance } = useAcceptances(kind, subjectId); + const { acceptances, latest, isLoading, error, mutate, recordAcceptance } = useAcceptances( + kind, + subjectId, + ); const [dialogOpen, setDialogOpen] = useState(false); const currentLevelLabel = @@ -95,7 +98,9 @@ export function ResidualAcceptanceCard({

Residual risk acceptance

{latest && !latestStale && Accepted} {latest && latestStale && Stale} - {!latest && !isLoading && Awaiting acceptance} + {!latest && !isLoading && !error && ( + Awaiting acceptance + )}
The risk owner's formal, dated acceptance of the residual risk (ISO 27001 @@ -114,7 +119,16 @@ export function ResidualAcceptanceCard({ )}
- {isLoading && acceptances.length === 0 ? ( + {error ? ( + // A failed load must never read as "no acceptance recorded" — that + // could prompt a duplicate record of an existing formal acceptance. +
+ Couldn't load the acceptance history. + +
+ ) : isLoading && acceptances.length === 0 ? (
diff --git a/apps/app/src/hooks/use-risk-acceptances.ts b/apps/app/src/hooks/use-risk-acceptances.ts index 3f00ba1dbb..fa9f7e0ccc 100644 --- a/apps/app/src/hooks/use-risk-acceptances.ts +++ b/apps/app/src/hooks/use-risk-acceptances.ts @@ -48,7 +48,12 @@ export function useAcceptances(kind: AcceptanceSubjectKind, subjectId: string | const endpoint = endpointFor(kind, subjectId); const swr = useApiSWR(endpoint); - const acceptances = swr.data?.data?.data ?? []; + // apiClient resolves API failures into ApiResponse.error (the fetcher never + // throws), so swr.error alone misses forbidden/missing responses — surface + // both so callers can distinguish a failed load from an empty history. + const apiError = swr.data?.error ? new Error(swr.data.error) : null; + const error = swr.error ?? apiError; + const acceptances = error ? [] : (swr.data?.data?.data ?? []); const latest = acceptances[0] ?? null; const recordAcceptance = useCallback( @@ -68,7 +73,7 @@ export function useAcceptances(kind: AcceptanceSubjectKind, subjectId: string | acceptances, latest, isLoading: swr.isLoading, - error: swr.error, + error, mutate: swr.mutate, recordAcceptance, }; From 1b1a2a0506b101543718239f1de70a38d29fc3af Mon Sep 17 00:00:00 2001 From: Tofik Hasanov Date: Fri, 24 Jul 2026 00:16:58 -0400 Subject: [PATCH 2/2] fix(risks): address review round on the deploy-fix PR - RTP submit gating fails closed: a load/revalidation error blocks submission even when stale cached readiness data is present - approve() runs REPEATABLE READ (single MVCC snapshot), so the drift baseline and the frozen rows can never observe different data; one retry on a P2034 write conflict re-runs with a fresh snapshot - acceptance row locks are org-scoped, so a foreign-tenant id can never acquire another organization's row lock - 500 responses wired into all four acceptance endpoint decorators - authType documents the 'service' value HybridAuthGuard can return --- .../api/src/isms/isms.service.approve.spec.ts | 10 +++++++ apps/api/src/isms/isms.service.ts | 28 ++++++++++++++++--- .../src/risks/risk-acceptances.controller.ts | 2 ++ .../api/src/risks/risk-acceptances.service.ts | 8 ++++-- .../schemas/risk-acceptances.responses.ts | 2 +- .../vendors/vendor-acceptances.controller.ts | 2 ++ .../components/RiskTreatmentPlanClient.tsx | 19 +++++++------ 7 files changed, 55 insertions(+), 16 deletions(-) diff --git a/apps/api/src/isms/isms.service.approve.spec.ts b/apps/api/src/isms/isms.service.approve.spec.ts index 2ca36806e0..b76fe11da2 100644 --- a/apps/api/src/isms/isms.service.approve.spec.ts +++ b/apps/api/src/isms/isms.service.approve.spec.ts @@ -16,6 +16,16 @@ jest.mock('@db', () => ({ member: { findFirst: jest.fn() }, $transaction: jest.fn(), }, + Prisma: { + TransactionIsolationLevel: { RepeatableRead: 'RepeatableRead' }, + PrismaClientKnownRequestError: class PrismaClientKnownRequestError extends Error { + code: string; + constructor(message: string, { code }: { code: string }) { + super(message); + this.code = code; + } + }, + }, })); jest.mock('./documents/data-source', () => ({ collectPlatformData: jest.fn(), diff --git a/apps/api/src/isms/isms.service.ts b/apps/api/src/isms/isms.service.ts index 40cb758edb..a306b54d14 100644 --- a/apps/api/src/isms/isms.service.ts +++ b/apps/api/src/isms/isms.service.ts @@ -455,15 +455,17 @@ export class IsmsService { // Freeze the draft into a new immutable published version and promote it to // currentVersion. Editing afterwards reverts status to draft but leaves this // published version live and exportable (CS-701). - const published = await db.$transaction(async (tx) => { + const publish = async (tx: Prisma.TransactionClient) => { // Serialize concurrent approvals (and register-row creates, which take the // same lock) on this document so they can't interleave and double-publish. await lockDocument(tx, documentId); // Collect the drift baseline INSIDE the transaction so it reads the same - // point in time as the rows frozen into the published version below — a - // concurrent platform edit can no longer make a just-approved document - // immediately stale. + // point in time as the rows frozen into the published version below. + // The transaction runs REPEATABLE READ (one MVCC snapshot for every + // statement), so this baseline and the rows loaded further down can + // never observe different data — a concurrent platform edit commits + // entirely before or entirely after this approval. const snapshot = await collectPlatformData({ organizationId, frameworkId: document.frameworkId, @@ -518,6 +520,24 @@ export class IsmsService { data: { currentVersionId: result.versionId }, }); return result; + }; + + // REPEATABLE READ can abort with a write conflict (P2034) when a racing + // lifecycle write commits between our snapshot and our claim update — + // retry once; the rerun takes a fresh snapshot and either wins cleanly or + // fails the conditional claim with the friendly BadRequestException. + const runPublish = () => + db.$transaction(publish, { + isolationLevel: Prisma.TransactionIsolationLevel.RepeatableRead, + }); + const published = await runPublish().catch((error: unknown) => { + if ( + error instanceof Prisma.PrismaClientKnownRequestError && + error.code === 'P2034' + ) { + return runPublish(); + } + throw error; }); // Render + upload the frozen exports outside the transaction (Policies diff --git a/apps/api/src/risks/risk-acceptances.controller.ts b/apps/api/src/risks/risk-acceptances.controller.ts index 0cbe24c1e5..f9a43b0294 100644 --- a/apps/api/src/risks/risk-acceptances.controller.ts +++ b/apps/api/src/risks/risk-acceptances.controller.ts @@ -56,6 +56,7 @@ export class RiskAcceptancesController { @ApiResponse(LIST_RISK_ACCEPTANCES_RESPONSES[401]) @ApiResponse(LIST_RISK_ACCEPTANCES_RESPONSES[403]) @ApiResponse(LIST_RISK_ACCEPTANCES_RESPONSES[404]) + @ApiResponse(LIST_RISK_ACCEPTANCES_RESPONSES[500]) async listRiskAcceptances( @Param('id') riskId: string, @OrganizationId() organizationId: string, @@ -85,6 +86,7 @@ export class RiskAcceptancesController { @ApiResponse(RECORD_RISK_ACCEPTANCE_RESPONSES[401]) @ApiResponse(RECORD_RISK_ACCEPTANCE_RESPONSES[403]) @ApiResponse(RECORD_RISK_ACCEPTANCE_RESPONSES[404]) + @ApiResponse(RECORD_RISK_ACCEPTANCE_RESPONSES[500]) async recordRiskAcceptance( @Param('id') riskId: string, @Body() dto: CreateRiskAcceptanceDto, diff --git a/apps/api/src/risks/risk-acceptances.service.ts b/apps/api/src/risks/risk-acceptances.service.ts index c28f273815..336f3f83fe 100644 --- a/apps/api/src/risks/risk-acceptances.service.ts +++ b/apps/api/src/risks/risk-acceptances.service.ts @@ -80,7 +80,9 @@ export class RiskAcceptancesService { // concurrent residual PATCH blocks until this commits, so the frozen // rating is always the rating at acceptance time (never instantly stale). return db.$transaction(async (tx) => { - await tx.$queryRaw`SELECT id FROM "Risk" WHERE id = ${riskId} FOR UPDATE`; + // Org-scoped so a foreign-tenant id can never acquire (even briefly) + // another organization's row lock. + await tx.$queryRaw`SELECT id FROM "Risk" WHERE id = ${riskId} AND "organizationId" = ${organizationId} FOR UPDATE`; const risk = await tx.risk.findFirst({ where: { id: riskId, organizationId }, select: { @@ -124,7 +126,9 @@ export class RiskAcceptancesService { ): Promise { // Same row-lock rationale as createForRisk. return db.$transaction(async (tx) => { - await tx.$queryRaw`SELECT id FROM "Vendor" WHERE id = ${vendorId} FOR UPDATE`; + // Org-scoped so a foreign-tenant id can never acquire (even briefly) + // another organization's row lock. + await tx.$queryRaw`SELECT id FROM "Vendor" WHERE id = ${vendorId} AND "organizationId" = ${organizationId} FOR UPDATE`; const vendor = await this.findVendorRating(vendorId, organizationId, tx); return this.createAcceptance({ diff --git a/apps/api/src/risks/schemas/risk-acceptances.responses.ts b/apps/api/src/risks/schemas/risk-acceptances.responses.ts index c6fb7b62fe..6ebcf0d769 100644 --- a/apps/api/src/risks/schemas/risk-acceptances.responses.ts +++ b/apps/api/src/risks/schemas/risk-acceptances.responses.ts @@ -86,7 +86,7 @@ const NOT_FOUND = (entity: string): ApiResponseOptions => ({ const AUTH_RESPONSE_PROPERTIES = { authType: { type: 'string', - enum: ['api-key', 'session'], + enum: ['api-key', 'session', 'service'], description: 'How the request was authenticated', }, authenticatedUser: { diff --git a/apps/api/src/vendors/vendor-acceptances.controller.ts b/apps/api/src/vendors/vendor-acceptances.controller.ts index e1a2f39037..fde34ccc93 100644 --- a/apps/api/src/vendors/vendor-acceptances.controller.ts +++ b/apps/api/src/vendors/vendor-acceptances.controller.ts @@ -45,6 +45,7 @@ export class VendorAcceptancesController { @ApiResponse(LIST_VENDOR_ACCEPTANCES_RESPONSES[401]) @ApiResponse(LIST_VENDOR_ACCEPTANCES_RESPONSES[403]) @ApiResponse(LIST_VENDOR_ACCEPTANCES_RESPONSES[404]) + @ApiResponse(LIST_VENDOR_ACCEPTANCES_RESPONSES[500]) async listVendorAcceptances( @Param('id') vendorId: string, @OrganizationId() organizationId: string, @@ -72,6 +73,7 @@ export class VendorAcceptancesController { @ApiResponse(RECORD_VENDOR_ACCEPTANCE_RESPONSES[401]) @ApiResponse(RECORD_VENDOR_ACCEPTANCE_RESPONSES[403]) @ApiResponse(RECORD_VENDOR_ACCEPTANCE_RESPONSES[404]) + @ApiResponse(RECORD_VENDOR_ACCEPTANCE_RESPONSES[500]) async recordVendorAcceptance( @Param('id') vendorId: string, @Body() dto: CreateRiskAcceptanceDto, diff --git a/apps/app/src/app/(app)/[orgId]/documents/isms/components/RiskTreatmentPlanClient.tsx b/apps/app/src/app/(app)/[orgId]/documents/isms/components/RiskTreatmentPlanClient.tsx index 7d68482f90..0084975b2e 100644 --- a/apps/app/src/app/(app)/[orgId]/documents/isms/components/RiskTreatmentPlanClient.tsx +++ b/apps/app/src/app/(app)/[orgId]/documents/isms/components/RiskTreatmentPlanClient.tsx @@ -29,15 +29,16 @@ export function RiskTreatmentPlanClient(props: RiskTreatmentPlanClientProps) { const { riskTreatment, error, isLoading, mutateRiskTreatment } = useIsmsRiskTreatment(documentId); - // null = ready to submit. While readiness is unknown (loading/failed) keep - // Submit blocked so the approval dialog can't be completed only to hit the - // server-side completeness error. - const blockedReason = riskTreatment - ? riskTreatment.validationMessages.length > 0 - ? riskTreatment.validationMessages.join(' ') - : null - : error - ? 'The readiness check could not be loaded — refresh the page and try again.' + // null = ready to submit. Fail closed: a load/revalidation error blocks + // submission even when stale cached data is still present (SWR keeps the + // previous data on a failed revalidate), and loading blocks until the + // server has confirmed readiness. + const blockedReason = error + ? 'The readiness check could not be loaded — refresh the page and try again.' + : riskTreatment + ? riskTreatment.validationMessages.length > 0 + ? riskTreatment.validationMessages.join(' ') + : null : 'Checking the plan is ready to submit...'; return (