audit: Tier 5.4 — wrap moveFolder cycle check + write in a tx
Concurrency-auditor HIGH: the cycle walk + UPDATE used to run as separate statements. Two concurrent moves (A→B and B→A) could each pass the walk against the pre-move tree and both write, leaving an A↔B cycle. Whole sequence now runs inside one db.transaction(). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -209,4 +209,4 @@ Compensating-delete is faster to ship but doesn't catch process-crash gaps. Saga
|
||||
|
||||
---
|
||||
|
||||
*Everything in `AUDIT-TRIAGE.md` Tier 8 is already shipped. Everything not listed in this file has been fixed without parking — see the commit log on `feat/documents-folders`.*
|
||||
_Everything in `AUDIT-TRIAGE.md` Tier 8 is already shipped. Everything not listed in this file has been fixed without parking — see the commit log on `feat/documents-folders`._
|
||||
|
||||
@@ -40,8 +40,7 @@ const LeadSourceChart = dynamic(
|
||||
{ loading: ChartFallback, ssr: false },
|
||||
);
|
||||
const OccupancyTimelineChart = dynamic(
|
||||
() =>
|
||||
import('./occupancy-timeline-chart').then((m) => ({ default: m.OccupancyTimelineChart })),
|
||||
() => import('./occupancy-timeline-chart').then((m) => ({ default: m.OccupancyTimelineChart })),
|
||||
{ loading: ChartFallback, ssr: false },
|
||||
);
|
||||
const PipelineFunnelChart = dynamic(
|
||||
|
||||
@@ -221,51 +221,64 @@ export async function moveFolder(
|
||||
|
||||
const folder = await assertNotSystemManaged(portId, folderId, 'move');
|
||||
|
||||
if (newParentId !== null) {
|
||||
const newParent = await db.query.documentFolders.findFirst({
|
||||
where: and(eq(documentFolders.id, newParentId), eq(documentFolders.portId, portId)),
|
||||
});
|
||||
if (!newParent) throw new ValidationError('Invalid parent folder');
|
||||
|
||||
// Cycle check: walk the destination's ancestor chain. If we hit
|
||||
// folderId, the destination is a descendant of the folder being
|
||||
// moved — moving would create a cycle.
|
||||
let cursor: string | null = newParent.parentId;
|
||||
const seen = new Set<string>([newParent.id]);
|
||||
while (cursor) {
|
||||
if (cursor === folderId) {
|
||||
throw new ValidationError('Cannot move a folder under one of its descendants (cycle)');
|
||||
}
|
||||
if (seen.has(cursor)) break; // defensive — pre-existing cycle, bail
|
||||
seen.add(cursor);
|
||||
const next = await db.query.documentFolders.findFirst({
|
||||
where: and(eq(documentFolders.id, cursor), eq(documentFolders.portId, portId)),
|
||||
columns: { parentId: true },
|
||||
});
|
||||
cursor = next?.parentId ?? null;
|
||||
}
|
||||
}
|
||||
|
||||
// concurrency-auditor HIGH: the cycle check + the UPDATE used to run
|
||||
// as separate statements with no shared lock. Two concurrent moves
|
||||
// (move A → B and move B → A) could each pass the cycle check
|
||||
// against the pre-move tree, then both write, leaving an A↔B cycle.
|
||||
// Wrap the whole sequence in a single transaction so the walk-and-
|
||||
// write is atomic per move attempt.
|
||||
try {
|
||||
const [updated] = await db
|
||||
.update(documentFolders)
|
||||
.set({ parentId: newParentId, updatedAt: new Date() })
|
||||
.where(and(eq(documentFolders.id, folderId), eq(documentFolders.portId, portId)))
|
||||
.returning();
|
||||
if (!updated) throw new NotFoundError('Folder');
|
||||
return await db.transaction(async (tx) => {
|
||||
if (newParentId !== null) {
|
||||
const newParent = await tx.query.documentFolders.findFirst({
|
||||
where: and(
|
||||
eq(documentFolders.id, newParentId),
|
||||
eq(documentFolders.portId, portId),
|
||||
),
|
||||
});
|
||||
if (!newParent) throw new ValidationError('Invalid parent folder');
|
||||
|
||||
void createAuditLog({
|
||||
userId,
|
||||
portId,
|
||||
action: 'update',
|
||||
entityType: 'document_folder',
|
||||
entityId: folderId,
|
||||
oldValue: { parentId: folder.parentId },
|
||||
newValue: { parentId: newParentId },
|
||||
metadata: { type: 'folder_move' },
|
||||
// Cycle check: walk the destination's ancestor chain. If we hit
|
||||
// folderId, the destination is a descendant of the folder being
|
||||
// moved — moving would create a cycle.
|
||||
let cursor: string | null = newParent.parentId;
|
||||
const seen = new Set<string>([newParent.id]);
|
||||
while (cursor) {
|
||||
if (cursor === folderId) {
|
||||
throw new ValidationError(
|
||||
'Cannot move a folder under one of its descendants (cycle)',
|
||||
);
|
||||
}
|
||||
if (seen.has(cursor)) break; // defensive — pre-existing cycle, bail
|
||||
seen.add(cursor);
|
||||
const next = await tx.query.documentFolders.findFirst({
|
||||
where: and(eq(documentFolders.id, cursor), eq(documentFolders.portId, portId)),
|
||||
columns: { parentId: true },
|
||||
});
|
||||
cursor = next?.parentId ?? null;
|
||||
}
|
||||
}
|
||||
|
||||
const [updated] = await tx
|
||||
.update(documentFolders)
|
||||
.set({ parentId: newParentId, updatedAt: new Date() })
|
||||
.where(and(eq(documentFolders.id, folderId), eq(documentFolders.portId, portId)))
|
||||
.returning();
|
||||
if (!updated) throw new NotFoundError('Folder');
|
||||
|
||||
void createAuditLog({
|
||||
userId,
|
||||
portId,
|
||||
action: 'update',
|
||||
entityType: 'document_folder',
|
||||
entityId: folderId,
|
||||
oldValue: { parentId: folder.parentId },
|
||||
newValue: { parentId: newParentId },
|
||||
metadata: { type: 'folder_move' },
|
||||
});
|
||||
|
||||
return updated;
|
||||
});
|
||||
|
||||
return updated;
|
||||
} catch (err) {
|
||||
if (isSiblingNameConflict(err)) {
|
||||
throw new ConflictError('A folder with that name already exists in the destination');
|
||||
|
||||
Reference in New Issue
Block a user