fix(isms): serialize approve/decline to prevent concurrent double-publish (CS-701) (#3366)

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.


Claude-Session: https://claude.ai/code/session_012CweXoSSEP89mX93u3Bdcp

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: tofikwest

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,

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,

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,

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,

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();
+  });
 });

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);
+    });
   });
 });

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 });
   }
 
   /**

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<void> {