From 68df7f49beffba49720ad5943117d90f34245968 Mon Sep 17 00:00:00 2001 From: Tofik Hasanov Date: Tue, 7 Jul 2026 16:15:11 -0400 Subject: [PATCH] fix(isms): serialize approve/decline to prevent concurrent double-publish (CS-701) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Flagged on the production deploy PR (#3364): assertPendingApprovalBy runs before the transaction, so two racing approve() calls could both pass the guard. Root cause (verified): under READ COMMITTED a plain in-transaction status re-read does NOT serialize — the loser reads the still-committed `needs_review`. The @@unique([documentId, version]) constraint only turns the perfectly-simultaneous case into an error; the staggered case silently creates a second published version and overwrites currentVersionId. Fix: - approve() now takes a per-document advisory lock and then claims the approval atomically via updateMany({ where: status='needs_review' + approverId }) inside the transaction, aborting when count !== 1. Exactly one caller freezes a version; losers abort before any derivation/version creation. The pre-transaction guard is kept for the normal-case specific error messages. - decline() gets the same atomic claim, so an approve/decline race can no longer leave a "declined" document that still owns a live currentVersionId. - Renamed the shared advisory-lock helper lockDocumentForPositions -> lockDocument (it now serializes approval too, not just register-row position allocation); updated the four register-create callers. Adversarially audited the whole approve/publish/version/export flow for related concurrency + IDOR issues; getVersionExport org-scoping, S3 keys, snapshot parsing, currentVersionId integrity and the migration paths verified safe. The narrow edit-during-approve window for register update/delete is low-severity/self-healing and left as a follow-up. Tests: 321 api ISMS tests passing (incl. new approve + decline race tests); typecheck clean. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_012CweXoSSEP89mX93u3Bdcp --- .../src/isms/isms-context-issue.service.ts | 6 +-- .../src/isms/isms-interested-party.service.ts | 6 +-- apps/api/src/isms/isms-objective.service.ts | 6 +-- apps/api/src/isms/isms-requirement.service.ts | 6 +-- .../api/src/isms/isms.service.approve.spec.ts | 50 ++++++++++++++++--- .../src/isms/isms.service.lifecycle.spec.ts | 29 ++++++++--- apps/api/src/isms/isms.service.ts | 48 +++++++++++++++--- apps/api/src/isms/utils/document-lock.ts | 11 ++-- 8 files changed, 125 insertions(+), 37 deletions(-) diff --git a/apps/api/src/isms/isms-context-issue.service.ts b/apps/api/src/isms/isms-context-issue.service.ts index 7d2fa066be..daeb45c23b 100644 --- a/apps/api/src/isms/isms-context-issue.service.ts +++ b/apps/api/src/isms/isms-context-issue.service.ts @@ -2,7 +2,7 @@ import { Injectable, NotFoundException } from '@nestjs/common'; import { db } from '@db'; import type { Prisma } from '@db'; import { invalidateApprovalIfNeeded } from './utils/approval'; -import { lockDocumentForPositions } from './utils/document-lock'; +import { lockDocument } from './utils/document-lock'; import type { CreateContextIssueInput, UpdateContextIssueInput, @@ -28,7 +28,7 @@ export class IsmsContextIssueService { await this.requireDocument({ documentId, organizationId }); return db.$transaction(async (tx) => { - await lockDocumentForPositions(tx, documentId); + await lockDocument(tx, documentId); const position = dto.position ?? (await this.nextPosition({ tx, documentId })); await invalidateApprovalIfNeeded({ tx, documentId }); @@ -92,7 +92,7 @@ export class IsmsContextIssueService { /** * Next position uses max(position)+1 so it survives deletes. Runs on the * transaction client; the create first takes a per-document advisory lock - * (lockDocumentForPositions) so concurrent creates can't read the same max. + * (lockDocument) so concurrent creates can't read the same max. */ private async nextPosition({ tx, diff --git a/apps/api/src/isms/isms-interested-party.service.ts b/apps/api/src/isms/isms-interested-party.service.ts index 58b3dcbf8e..70141f275f 100644 --- a/apps/api/src/isms/isms-interested-party.service.ts +++ b/apps/api/src/isms/isms-interested-party.service.ts @@ -2,7 +2,7 @@ import { Injectable, NotFoundException } from '@nestjs/common'; import { db } from '@db'; import type { Prisma } from '@db'; import { invalidateApprovalIfNeeded } from './utils/approval'; -import { lockDocumentForPositions } from './utils/document-lock'; +import { lockDocument } from './utils/document-lock'; import type { CreateInterestedPartyInput, UpdateInterestedPartyInput, @@ -28,7 +28,7 @@ export class IsmsInterestedPartyService { await this.requireDocument({ documentId, organizationId }); return db.$transaction(async (tx) => { - await lockDocumentForPositions(tx, documentId); + await lockDocument(tx, documentId); const position = dto.position ?? (await this.nextPosition({ tx, documentId })); await invalidateApprovalIfNeeded({ tx, documentId }); @@ -89,7 +89,7 @@ export class IsmsInterestedPartyService { /** * Next position uses max(position)+1 so it survives deletes. Runs on the * transaction client; the create first takes a per-document advisory lock - * (lockDocumentForPositions) so concurrent creates can't read the same max. + * (lockDocument) so concurrent creates can't read the same max. */ private async nextPosition({ tx, diff --git a/apps/api/src/isms/isms-objective.service.ts b/apps/api/src/isms/isms-objective.service.ts index 98a5986930..512232ec5d 100644 --- a/apps/api/src/isms/isms-objective.service.ts +++ b/apps/api/src/isms/isms-objective.service.ts @@ -2,7 +2,7 @@ import { Injectable, NotFoundException } from '@nestjs/common'; import { db } from '@db'; import type { Prisma } from '@db'; import { invalidateApprovalIfNeeded } from './utils/approval'; -import { lockDocumentForPositions } from './utils/document-lock'; +import { lockDocument } from './utils/document-lock'; import type { CreateObjectiveInput, UpdateObjectiveInput, @@ -31,7 +31,7 @@ export class IsmsObjectiveService { }); return db.$transaction(async (tx) => { - await lockDocumentForPositions(tx, documentId); + await lockDocument(tx, documentId); const position = dto.position ?? (await this.nextPosition({ tx, documentId })); await invalidateApprovalIfNeeded({ tx, documentId }); @@ -118,7 +118,7 @@ export class IsmsObjectiveService { /** * Next position uses max(position)+1 so it survives deletes. Runs on the * transaction client; the create first takes a per-document advisory lock - * (lockDocumentForPositions) so concurrent creates can't read the same max. + * (lockDocument) so concurrent creates can't read the same max. */ private async nextPosition({ tx, diff --git a/apps/api/src/isms/isms-requirement.service.ts b/apps/api/src/isms/isms-requirement.service.ts index 568219d3bf..9a16673bc5 100644 --- a/apps/api/src/isms/isms-requirement.service.ts +++ b/apps/api/src/isms/isms-requirement.service.ts @@ -6,7 +6,7 @@ import { import { db } from '@db'; import type { Prisma } from '@db'; import { invalidateApprovalIfNeeded } from './utils/approval'; -import { lockDocumentForPositions } from './utils/document-lock'; +import { lockDocument } from './utils/document-lock'; import type { CreateRequirementInput, UpdateRequirementInput, @@ -37,7 +37,7 @@ export class IsmsRequirementService { } return db.$transaction(async (tx) => { - await lockDocumentForPositions(tx, documentId); + await lockDocument(tx, documentId); const position = dto.position ?? (await this.nextPosition({ tx, documentId })); await invalidateApprovalIfNeeded({ tx, documentId }); @@ -123,7 +123,7 @@ export class IsmsRequirementService { /** * Next position uses max(position)+1 so it survives deletes. Runs on the * transaction client; the create first takes a per-document advisory lock - * (lockDocumentForPositions) so concurrent creates can't read the same max. + * (lockDocument) so concurrent creates can't read the same max. */ private async nextPosition({ tx, diff --git a/apps/api/src/isms/isms.service.approve.spec.ts b/apps/api/src/isms/isms.service.approve.spec.ts index cdedf89f6e..fc6e203815 100644 --- a/apps/api/src/isms/isms.service.approve.spec.ts +++ b/apps/api/src/isms/isms.service.approve.spec.ts @@ -122,7 +122,9 @@ describe('IsmsService.approve (CS-701 versioning)', () => { .mockResolvedValueOnce({ id: 'doc_1', status: 'approved' }); mockCollect.mockResolvedValue(platformData); const tx = { + $executeRaw: jest.fn().mockResolvedValue(0), ismsDocument: { + updateMany: jest.fn().mockResolvedValue({ count: 1 }), findUniqueOrThrow: jest.fn().mockResolvedValue(reloaded), update: jest.fn().mockResolvedValue({}), }, @@ -155,18 +157,26 @@ describe('IsmsService.approve (CS-701 versioning)', () => { expect(mockUpdateDraft).toHaveBeenCalledWith( expect.objectContaining({ tx, documentId: 'doc_1' }), ); + // The approval is claimed atomically (serializes concurrent approvals): only + // matches while still awaiting this member's review. + expect(tx.ismsDocument.updateMany).toHaveBeenCalledWith({ + where: { + id: 'doc_1', + organizationId: 'org_1', + status: 'needs_review', + approverId: 'mem_1', + }, + data: expect.objectContaining({ status: 'approved', declinedAt: null }), + }); // A published version is created from the reloaded (re-derived) document. expect(versionService.createPublishedVersion).toHaveBeenCalledWith( expect.objectContaining({ tx, document: reloaded, memberId: 'mem_1' }), ); - // The document is promoted to the freshly-created version. + // The document is promoted to the freshly-created version (status was already + // set by the atomic claim above). expect(tx.ismsDocument.update).toHaveBeenCalledWith({ where: { id: 'doc_1' }, - data: expect.objectContaining({ - status: 'approved', - declinedAt: null, - currentVersionId: 'isms_ver_1', - }), + data: { currentVersionId: 'isms_ver_1' }, }); // Renders + uploads happen AFTER the transaction (Policies pattern). expect(versionService.publishRenders).toHaveBeenCalledWith( @@ -178,4 +188,32 @@ describe('IsmsService.approve (CS-701 versioning)', () => { }), ); }); + + it('aborts without creating a version when a concurrent approval already won the claim', async () => { + (mockDb.member.findFirst as jest.Mock).mockResolvedValue({ id: 'mem_1' }); + (mockDb.ismsDocument.findFirst as jest.Mock).mockResolvedValue({ + id: 'doc_1', + status: 'needs_review', + approverId: 'mem_1', + frameworkId: 'fw_1', + type: 'context_of_organization', + }); + mockCollect.mockResolvedValue(platformData); + const tx = { + $executeRaw: jest.fn().mockResolvedValue(0), + ismsDocument: { + // The racing approval already flipped status, so the claim matches 0 rows. + updateMany: jest.fn().mockResolvedValue({ count: 0 }), + findUniqueOrThrow: jest.fn(), + update: jest.fn(), + }, + }; + (mockDb.$transaction as jest.Mock).mockImplementation((cb) => cb(tx)); + + await expect(service.approve(args)).rejects.toThrow(BadRequestException); + // No derivation, no version creation once the claim fails. + expect(mockRunDerivation).not.toHaveBeenCalled(); + expect(versionService.createPublishedVersion).not.toHaveBeenCalled(); + expect(tx.ismsDocument.findUniqueOrThrow).not.toHaveBeenCalled(); + }); }); diff --git a/apps/api/src/isms/isms.service.lifecycle.spec.ts b/apps/api/src/isms/isms.service.lifecycle.spec.ts index 80e1311c5e..2defb6ba8f 100644 --- a/apps/api/src/isms/isms.service.lifecycle.spec.ts +++ b/apps/api/src/isms/isms.service.lifecycle.spec.ts @@ -14,6 +14,7 @@ jest.mock('@db', () => ({ ismsDocument: { findFirst: jest.fn(), update: jest.fn(), + updateMany: jest.fn(), }, member: { findFirst: jest.fn() }, $transaction: jest.fn(), @@ -151,23 +152,39 @@ describe('IsmsService document lifecycle', () => { await expect(service.decline(args)).rejects.toThrow(ForbiddenException); }); - it('sets declined status and declinedAt', async () => { + it('sets declined status and declinedAt via an atomic claim', async () => { (mockDb.member.findFirst as jest.Mock).mockResolvedValue({ id: 'mem_1' }); (mockDb.ismsDocument.findFirst as jest.Mock).mockResolvedValue({ id: 'doc_1', status: 'needs_review', approverId: 'mem_1', }); - (mockDb.ismsDocument.update as jest.Mock).mockResolvedValue({ - id: 'doc_1', - status: 'declined', - }); + (mockDb.ismsDocument.updateMany as jest.Mock).mockResolvedValue({ count: 1 }); await service.decline(args); - const call = (mockDb.ismsDocument.update as jest.Mock).mock.calls[0][0]; + const call = (mockDb.ismsDocument.updateMany as jest.Mock).mock.calls[0][0]; + // Guarded transition: only matches while awaiting this member's review. + expect(call.where).toEqual({ + id: 'doc_1', + organizationId: 'org_1', + status: 'needs_review', + approverId: 'mem_1', + }); expect(call.data.status).toBe('declined'); expect(call.data.declinedAt).toBeInstanceOf(Date); }); + + it('aborts when a concurrent approve already won (claim matches 0 rows)', async () => { + (mockDb.member.findFirst as jest.Mock).mockResolvedValue({ id: 'mem_1' }); + (mockDb.ismsDocument.findFirst as jest.Mock).mockResolvedValue({ + id: 'doc_1', + status: 'needs_review', + approverId: 'mem_1', + }); + (mockDb.ismsDocument.updateMany as jest.Mock).mockResolvedValue({ count: 0 }); + + await expect(service.decline(args)).rejects.toThrow(BadRequestException); + }); }); }); diff --git a/apps/api/src/isms/isms.service.ts b/apps/api/src/isms/isms.service.ts index e0a73497ff..39777c115c 100644 --- a/apps/api/src/isms/isms.service.ts +++ b/apps/api/src/isms/isms.service.ts @@ -11,6 +11,7 @@ import { collectPlatformData } from './documents/data-source'; import { runDerivation } from './documents/generate'; import { updateDraftSnapshot } from './utils/draft-snapshot'; import { EXPORT_DOCUMENT_INCLUDE } from './utils/export-payload'; +import { lockDocument } from './utils/document-lock'; import { IsmsVersionService } from './isms-version.service'; /** @@ -224,6 +225,29 @@ export class IsmsService { // currentVersion. Editing afterwards reverts status to draft but leaves this // published version live and exportable (CS-701). const published = await db.$transaction(async (tx) => { + // 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); + + // 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 + // while the document is still awaiting THIS member's review, so exactly one + // caller proceeds; a loser matches zero rows and aborts before creating a + // version. (A plain in-transaction re-read would not serialize.) + const claim = await tx.ismsDocument.updateMany({ + where: { + id: documentId, + organizationId, + status: 'needs_review', + approverId: member.id, + }, + data: { status: 'approved', approvedAt: now, declinedAt: null }, + }); + if (claim.count !== 1) { + throw new BadRequestException('Document is not pending your approval'); + } + // Re-derive in the same transaction so the persisted rows and the frozen // snapshot come from one pass (otherwise the approved content can drift). await runDerivation({ @@ -250,12 +274,7 @@ export class IsmsService { await tx.ismsDocument.update({ where: { id: documentId }, - data: { - status: 'approved', - approvedAt: now, - declinedAt: null, - currentVersionId: result.versionId, - }, + data: { currentVersionId: result.versionId }, }); return result; }); @@ -286,10 +305,23 @@ export class IsmsService { const document = await this.requireDocument({ documentId, organizationId }); this.assertPendingApprovalBy({ document, member }); - return db.ismsDocument.update({ - where: { id: documentId }, + // Atomically claim the decline so it can't race an approve (both pass the + // pre-transaction guard). Only matches while still awaiting this member's + // review, so a concurrent approve that already won leaves zero rows here. + const claim = await db.ismsDocument.updateMany({ + where: { + id: documentId, + organizationId, + status: 'needs_review', + approverId: member.id, + }, data: { status: 'declined', declinedAt: new Date() }, }); + if (claim.count !== 1) { + throw new BadRequestException('Document is not pending your approval'); + } + + return this.getDocument({ documentId, organizationId }); } /** diff --git a/apps/api/src/isms/utils/document-lock.ts b/apps/api/src/isms/utils/document-lock.ts index 22fef4f602..1ffa527ccd 100644 --- a/apps/api/src/isms/utils/document-lock.ts +++ b/apps/api/src/isms/utils/document-lock.ts @@ -1,13 +1,14 @@ import type { Prisma } from '@db'; /** - * Serialize register-row position allocation for a single document. The Postgres + * Serialize concurrent writes for a single ISMS document. The Postgres * transaction-scoped advisory lock (keyed on the document id) is held until the - * surrounding transaction commits, so two concurrent creates can't both read the - * same max(position) and persist duplicate ordering keys. Call this inside the - * create transaction, before computing the next position. + * surrounding transaction commits, so two transactions touching the same document + * run one at a time. Used by register-row creates (so concurrent creates can't + * read the same max(position)) and by approve (so concurrent approvals can't both + * freeze a published version). Call this first, inside the transaction. */ -export async function lockDocumentForPositions( +export async function lockDocument( tx: Prisma.TransactionClient, documentId: string, ): Promise {