fix(sandbox): address the ACL backend review findings — quoting, DACL merge+lock, failure paths

quoteArg doubles end-of-string backslashes (real CommandLineToArgvW round-trip test; first-token exemption noted); grantWrite merges into the current DACL instead of replacing it, and both grant/revoke hold a LockFileEx per-path lock under GetTempPathW/dsh-acl-locks (koffi crashes on NULL lpOverlapped — a zeroed OVERLAPPED is used); GRANT_MASK gains DELETE|FILE_DELETE_CHILD (never WRITE_DAC/WRITE_OWNER) though the win32 26200 second check only constrains the WRITE bit; failure paths close all handles (CreateProcessAsUserW pipe set, ResumeThread thread/process/job) with stub-api tests; getTempPathW refuses undersized buffers; drainPipe backs off; runner.spec pwshAvailable uses the resolvePwshPath probe; NTSTATUS exit codes mirror bit-exact (verified end-to-end); WinLocalSid JSDoc re-attributed to WinLocalLogonSid. All gates green: 42 passed/2 skipped, typecheck, oxlint, 0 clones, constraints, knip.
2026-08-08 14:32:01 +08:00
parent fa591b5ebd
commit 91d3ed6c5a
13 changed files with 805 additions and 83 deletions
--- a/packages/sandbox/sandbox-windows-acl/src/acl.ts
+++ b/packages/sandbox/sandbox-windows-acl/src/acl.ts
@@ -4,10 +4,19 @@
 * the failure handling the POC lacks). Every API call is checked and every
 * failure is reported with the API name, the exact Win32 code, the formatted
 * system text, and the affected path.
+ *
+ * Concurrency: grants are read-merge-write against the directory's CURRENT
+ * DACL, and the whole get-merge-set sequence runs under a per-path exclusive
+ * LockFileEx lock (see {@link withPathLock}) so concurrent sandbox instances
+ * cannot clobber each other's ACEs.
 * @module @deepseek-ai/dsh-sandbox-windows-acl/acl
 */

-import { allocPtrSlot, decodePtr, isNullPtr, ptrAddress, throwLastError, throwWin32 } from './ffi.ts'
+import { createHash } from 'node:crypto'
+import { mkdirSync } from 'node:fs'
+import { dirname, join } from 'node:path'
+
+import { allocOverlapped, allocPtrSlot, decodePtr, getTempPath, isInvalidHandle, isNullPtr, ptrAddress, throwLastError, throwWin32 } from './ffi.ts'
 import type { NativePtr, Win32Bindings } from './ffi.ts'
 import * as abi from './win32-abi.ts'

@@ -18,7 +27,7 @@ import * as abi from './win32-abi.ts'
 * `permissions` is the access mask; the POC passes 0 for REVOKE_ACCESS, which
 * removes every ACE for the trustee.
 */
-function buildExplicitAccess(sidPtr: NativePtr, mode: number, permissions: number): Buffer {
+export function buildExplicitAccess(sidPtr: NativePtr, mode: number, permissions: number): Buffer {
  const entry = Buffer.alloc(abi.EXPLICIT_ACCESS_W_SIZE)
  entry.writeUInt32LE(permissions, 0) // grfAccessPermissions
  entry.writeUInt32LE(mode, 4) // grfAccessMode
@@ -31,47 +40,82 @@ function buildExplicitAccess(sidPtr: NativePtr, mode: number, permissions: numbe
 }

 /**
- * Grant `FILE_GENERIC_WRITE & ~READ_CONTROL` (displays as "Write") to the
- * orphan SID on `path`, inheriting to subcontainers and objects. The directory
- * must be owned by the caller (owner implicit WRITE_DAC) — same precondition
- * as the POC.
+ * One lock file per protected path: `<GetTempPathW()>\dsh-acl-locks\<first 16
+ * hex of sha256(lowercased path)>.lock`. The lock root derives from
+ * GetTempPathW (never from runner argv or DSH_HOME), and the lowercasing
+ * maps Windows's case-insensitive path spellings onto one lock.
 * @param api - the binding table.
- * @param path - the directory whose DACL gains the grant (the workspace or temp root).
- * @param sidPtr - the orphan write SID the ACE names.
+ * @param path - the protected directory (absolute).
+ * @returns the lock file path for that directory.
 */
-export function grantWrite(api: Win32Bindings, path: string, sidPtr: NativePtr): void {
-  const newAclSlot = allocPtrSlot()
-  const mergeResult = api.setEntriesInAclW(1, buildExplicitAccess(sidPtr, abi.GRANT_ACCESS, abi.GRANT_MASK), null, newAclSlot)
-  if (mergeResult !== abi.ERROR_SUCCESS) throwWin32(api, 'SetEntriesInAclW', mergeResult, path)
-  const newAcl = decodePtr(newAclSlot)
-  if (newAcl === null) throwWin32(api, 'SetEntriesInAclW', api.getLastError(), `null ACL for ${path}`)
-
-  const applyResult = api.setNamedSecurityInfoW(
-    path, abi.SE_FILE_OBJECT, abi.DACL_SECURITY_INFORMATION,
-    null, null, newAcl, null,
-  )
-  // Free the LocalAlloc'd ACL before any throw; capture both outcomes first.
-  const freed = api.localFree(newAcl)
-  if (applyResult !== abi.ERROR_SUCCESS) throwWin32(api, 'SetNamedSecurityInfoW', applyResult, path)
-  if (!isNullPtr(freed)) throwLastError(api, 'LocalFree', `grantWrite(${path})`)
+export function lockFilePath(api: Win32Bindings, path: string): string {
+  const digest = createHash('sha256').update(path.toLowerCase()).digest('hex').slice(0, 16)
+  return join(getTempPath(api), 'dsh-acl-locks', `${digest}.lock`)
 }

 /**
- * Remove every ACE for the orphan SID from the directory DACL (REVOKE_ACCESS
- * merge — other entries are preserved). Returns whether an ACE removal was
- * attempted (false when the directory carries no DACL at all).
- *
- * Allocation contract (the POC's RevokeAccess, minus its missing checks):
- * GetNamedSecurityInfoW returns the DACL pointer INSIDE the security
- * descriptor allocation — only the descriptor may be LocalFree'd, and it must
- * not be freed before SetEntriesInAclW has consumed the ACL. Freeing the ACL
- * pointer itself corrupts the heap (verified the hard way).
+ * Run `action` holding the per-path exclusive lock: CreateFileW
+ * (OPEN_ALWAYS, shared read/write but NOT delete — a deletable lock file
+ * could be removed and recreated under the holder, letting two processes
+ * hold "the same" lock), then a one-byte LockFileEx
+ * (LOCKFILE_EXCLUSIVE_LOCK, zeroed OVERLAPPED = lock from offset 0 on the
+ * synchronous handle — see allocOverlapped for why not NULL), then
+ * UnlockFileEx + CloseHandle. Fail-closed: open/lock/unlock/close failures
+ * throw like every other Win32 call in this package; an `action` failure
+ * still unlocks (best-effort) and rethrows the original error.
 * @param api - the binding table.
- * @param path - the directory whose DACL loses the orphan-SID ACEs.
- * @param sidPtr - the orphan write SID whose ACEs are removed.
- * @returns whether an ACE removal was attempted (false when the directory carries no DACL at all).
+ * @param path - the protected directory (absolute).
+ * @param action - the get-merge-set sequence to serialize.
+ * @returns the action's result.
 */
-export function revokeWrite(api: Win32Bindings, path: string, sidPtr: NativePtr): boolean {
+export function withPathLock<T>(api: Win32Bindings, path: string, action: () => T): T {
+  const lockPath = lockFilePath(api, path)
+  mkdirSync(dirname(lockPath), { recursive: true })
+  const handle = api.createFileW(
+    lockPath,
+    abi.GENERIC_READ | abi.GENERIC_WRITE,
+    abi.FILE_SHARE_READ | abi.FILE_SHARE_WRITE,
+    null, abi.OPEN_ALWAYS, 0, null,
+  )
+  if (isInvalidHandle(handle)) throwLastError(api, 'CreateFileW', lockPath)
+  const overlapped = allocOverlapped() // stays zeroed: offset 0, hEvent NULL
+  if (api.lockFileEx(handle, abi.LOCKFILE_EXCLUSIVE_LOCK, 0, 1, 0, overlapped) === 0) {
+    const win32Code = api.getLastError()
+    api.closeHandle(handle) // best-effort on the lock-failure path
+    throwWin32(api, 'LockFileEx', win32Code, lockPath)
+  }
+
+  let result: T
+  try {
+    result = action()
+  } catch (error) {
+    // Best-effort release on the action-failure path: cleanup failures must
+    // not mask the action's error.
+    api.unlockFileEx(handle, 0, 1, 0, overlapped)
+    api.closeHandle(handle)
+    throw error
+  }
+  if (api.unlockFileEx(handle, 0, 1, 0, overlapped) === 0) {
+    const win32Code = api.getLastError()
+    api.closeHandle(handle) // best-effort on the unlock-failure path
+    throwWin32(api, 'UnlockFileEx', win32Code, lockPath)
+  }
+  if (api.closeHandle(handle) === 0) throwLastError(api, 'CloseHandle', `lock file ${lockPath}`)
+  return result
+}
+
+/**
+ * Read the directory's current explicit DACL via GetNamedSecurityInfoW.
+ * Allocation contract (the POC's RevokeAccess, minus its missing checks): the
+ * returned ACL pointer sits INSIDE the security descriptor allocation — only
+ * the descriptor may be LocalFree'd, and it must not be freed before
+ * SetEntriesInAclW has consumed the ACL. Freeing the ACL pointer itself
+ * corrupts the heap (verified the hard way).
+ * @param api - the binding table.
+ * @param path - the directory whose DACL is read.
+ * @returns the current explicit DACL (null when the directory carries none) and its owning descriptor.
+ */
+function readCurrentDacl(api: Win32Bindings, path: string): { oldAcl: NativePtr | null; descriptor: NativePtr | null } {
  const ownerSlot = allocPtrSlot()
  const groupSlot = allocPtrSlot()
  const daclSlot = allocPtrSlot()
@@ -82,27 +126,39 @@ export function revokeWrite(api: Win32Bindings, path: string, sidPtr: NativePtr)
    ownerSlot, groupSlot, daclSlot, saclSlot, descriptorSlot,
  )
  if (readResult !== abi.ERROR_SUCCESS) throwWin32(api, 'GetNamedSecurityInfoW', readResult, path)
-  const oldAcl = decodePtr(daclSlot)
-  const descriptor = decodePtr(descriptorSlot)
-
-  if (oldAcl === null) {
-    if (descriptor !== null) {
-      const freed = api.localFree(descriptor)
-      if (!isNullPtr(freed)) throwLastError(api, 'LocalFree', `revokeWrite(${path}) descriptor`)
-    }
-    return false
-  }
+  return { oldAcl: decodePtr(daclSlot), descriptor: decodePtr(descriptorSlot) }
+}

+/**
+ * Shared tail of grantWrite and revokeWrite: merge `entry` into `oldAcl`
+ * (null = no explicit DACL yet; SetEntriesInAclW builds one from scratch),
+ * free the descriptor before applying the merged ACL, apply it, then free the
+ * merged ACL — checking every call and reporting with the caller's label.
+ * @param api - the binding table.
+ * @param path - the directory the DACL edit applies to.
+ * @param entry - the EXPLICIT_ACCESS_W to merge (grant or revoke).
+ * @param oldAcl - the current explicit DACL (from {@link readCurrentDacl}).
+ * @param descriptor - the descriptor allocation owning `oldAcl`.
+ * @param label - the caller's name for error details.
+ */
+function mergeAndApply(
+  api: Win32Bindings,
+  path: string,
+  entry: Buffer,
+  oldAcl: NativePtr | null,
+  descriptor: NativePtr | null,
+  label: string,
+): void {
  const newAclSlot = allocPtrSlot()
-  const mergeResult = api.setEntriesInAclW(1, buildExplicitAccess(sidPtr, abi.REVOKE_ACCESS, 0), oldAcl, newAclSlot)
+  const mergeResult = api.setEntriesInAclW(1, entry, oldAcl, newAclSlot)
  if (mergeResult !== abi.ERROR_SUCCESS) {
    if (descriptor !== null) api.localFree(descriptor) // frees the ACL block too
-    throwWin32(api, 'SetEntriesInAclW', mergeResult, `revokeWrite(${path})`)
+    throwWin32(api, 'SetEntriesInAclW', mergeResult, `${label}(${path})`)
  }
  const newAcl = decodePtr(newAclSlot)
  if (newAcl === null) {
    if (descriptor !== null) api.localFree(descriptor)
-    throwWin32(api, 'SetEntriesInAclW', api.getLastError(), `revokeWrite(${path}): null new ACL`)
+    throwWin32(api, 'SetEntriesInAclW', api.getLastError(), `${label}(${path}): null new ACL`)
  }

  // The descriptor block (oldAcl included) is dead after the merge — free it
@@ -113,8 +169,52 @@ export function revokeWrite(api: Win32Bindings, path: string, sidPtr: NativePtr)
    null, null, newAcl, null,
  )
  const freedNew = api.localFree(newAcl)
-  if (applyResult !== abi.ERROR_SUCCESS) throwWin32(api, 'SetNamedSecurityInfoW', applyResult, `revokeWrite(${path})`)
-  if (freedDescriptor !== null && !isNullPtr(freedDescriptor)) throwLastError(api, 'LocalFree', `revokeWrite(${path}) descriptor`)
-  if (!isNullPtr(freedNew)) throwLastError(api, 'LocalFree', `revokeWrite(${path}) new ACL`)
-  return true
+  if (applyResult !== abi.ERROR_SUCCESS) throwWin32(api, 'SetNamedSecurityInfoW', applyResult, `${label}(${path})`)
+  if (freedDescriptor !== null && !isNullPtr(freedDescriptor)) throwLastError(api, 'LocalFree', `${label}(${path}) descriptor`)
+  if (!isNullPtr(freedNew)) throwLastError(api, 'LocalFree', `${label}(${path}) new ACL`)
+}
+
+/**
+ * Grant `GRANT_MASK` (Write+Delete, displays as "Modify") to the orphan SID
+ * on `path`, inheriting to subcontainers and objects. Read-merge-write: the
+ * new ACE merges into the directory's CURRENT explicit DACL (same shape as
+ * {@link revokeWrite}), so pre-existing explicit ACEs survive. Runs under the
+ * per-path lock. The directory must be owned by the caller (owner implicit
+ * WRITE_DAC) — same precondition as the POC.
+ * @param api - the binding table.
+ * @param path - the directory whose DACL gains the grant (the workspace or temp root).
+ * @param sidPtr - the orphan write SID the ACE names.
+ */
+export function grantWrite(api: Win32Bindings, path: string, sidPtr: NativePtr): void {
+  withPathLock(api, path, () => {
+    const { oldAcl, descriptor } = readCurrentDacl(api, path)
+    mergeAndApply(api, path, buildExplicitAccess(sidPtr, abi.GRANT_ACCESS, abi.GRANT_MASK), oldAcl, descriptor, 'grantWrite')
+  })
+}
+
+/**
+ * Remove every ACE for the orphan SID from the directory DACL (REVOKE_ACCESS
+ * merge — other entries are preserved). Returns whether an ACE removal was
+ * attempted (false when the directory carries no DACL at all).
+ *
+ * Runs under the per-path lock (the whole get-merge-set sequence); the
+ * descriptor/ACL allocation contract lives on {@link readCurrentDacl}.
+ * @param api - the binding table.
+ * @param path - the directory whose DACL loses the orphan-SID ACEs.
+ * @param sidPtr - the orphan write SID whose ACEs are removed.
+ * @returns whether an ACE removal was attempted (false when the directory carries no DACL at all).
+ */
+export function revokeWrite(api: Win32Bindings, path: string, sidPtr: NativePtr): boolean {
+  return withPathLock(api, path, () => {
+    const { oldAcl, descriptor } = readCurrentDacl(api, path)
+    if (oldAcl === null) {
+      if (descriptor !== null) {
+        const freed = api.localFree(descriptor)
+        if (!isNullPtr(freed)) throwLastError(api, 'LocalFree', `revokeWrite(${path}) descriptor`)
+      }
+      return false
+    }
+    mergeAndApply(api, path, buildExplicitAccess(sidPtr, abi.REVOKE_ACCESS, 0), oldAcl, descriptor, 'revokeWrite')
+    return true
+  })
 }
--- a/packages/sandbox/sandbox-windows-acl/src/ffi.ts
+++ b/packages/sandbox/sandbox-windows-acl/src/ffi.ts
@@ -26,6 +26,17 @@ export function isNullPtr(value: NativePtr | null | undefined): value is null |
  return value === null || value === undefined || (value as bigint) === 0n
 }

+/**
+ * True for CreateFileW's INVALID_HANDLE_VALUE failure marker (-1, which
+ * koffi hands back as the unsigned 64-bit all-ones pointer).
+ * @param handle - the handle CreateFileW returned.
+ * @returns whether the handle signals failure.
+ */
+export function isInvalidHandle(handle: NativePtr | null | undefined): boolean {
+  if (isNullPtr(handle)) return true
+  return (handle as bigint) === 0xFFFFFFFFFFFFFFFFn || (handle as bigint) === -1n
+}
+
 type Ptr = ReturnType<typeof koffi.pointer>

 /** Field subset written into a zeroed STARTUPINFOW (layout verified: size 104). */
@@ -86,6 +97,12 @@ export interface Win32Bindings {
  ): number
  // ---- environment / io ----------------------------------------------------
  getTempPathW(length: number, buffer: Buffer): number
+  createFileW(
+    fileName: string, desiredAccess: number, shareMode: number, attributes: null,
+    creationDisposition: number, flagsAndAttributes: number, templateFile: null,
+  ): NativePtr
+  lockFileEx(file: NativePtr, flags: number, reserved: number, bytesLow: number, bytesHigh: number, overlapped: NativePtr): number
+  unlockFileEx(file: NativePtr, reserved: number, bytesLow: number, bytesHigh: number, overlapped: NativePtr): number
  createPipe(readHandle: NativePtr, writeHandle: NativePtr, attributes: null, size: number): number
  setHandleInformation(handle: NativePtr, mask: number, flags: number): number
  createProcessAsUserW(
@@ -231,6 +248,18 @@ export function allocBytes(length: number): NativePtr {
  return value as NativePtr
 }

+/**
+ * Allocate one zeroed OVERLAPPED (32 bytes on x64: Internal@0, InternalHigh@8,
+ * Offset@16, OffsetHigh@20, hEvent@24). LockFileEx/UnlockFileEx receive this
+ * instead of a NULL lpOverlapped: koffi 3.1.1 crashes on NULL there, and a
+ * zeroed OVERLAPPED on a synchronous file handle is the documented equivalent
+ * (the byte range locks from offset 0, hEvent stays NULL).
+ * @returns the zeroed block pointer.
+ */
+export function allocOverlapped(): NativePtr {
+  return allocBytes(32)
+}
+
 /**
 * Decode a pointer VALUE stored in memory at `buffer[offset]` (e.g. TOKEN_GROUPS entries).
 * @param buffer - the buffer holding the pointer value.
@@ -313,6 +342,14 @@ function bindings(): Win32Bindings {
    setNamedSecurityInfoW: bind(advapi32, 'SetNamedSecurityInfoW', 'uint32', ['str16', 'int', 'uint32', PVOID, PVOID, PVOID, PVOID]),
    getNamedSecurityInfoW: bind(advapi32, 'GetNamedSecurityInfoW', 'uint32', ['str16', 'int', 'uint32', PPVOID, PPVOID, PPVOID, PPVOID, PPVOID]),
    getTempPathW: bind(kernel32, 'GetTempPathW', 'uint32', ['uint32', PVOID]),
+    // fileapi.h line ~64: HANDLE CreateFileW(LPCWSTR, DWORD, DWORD,
+    // LPSECURITY_ATTRIBUTES, DWORD, DWORD, HANDLE).
+    createFileW: bind(kernel32, 'CreateFileW', PVOID, ['str16', 'uint32', 'uint32', PVOID, 'uint32', 'uint32', PVOID]),
+    // fileapi.h lines ~177/~185: BOOL LockFileEx(HANDLE, DWORD, DWORD, DWORD,
+    // DWORD, LPOVERLAPPED); BOOL UnlockFileEx(HANDLE, DWORD, DWORD, DWORD,
+    // LPOVERLAPPED). lpOverlapped is NULL for synchronous locking.
+    lockFileEx: bind(kernel32, 'LockFileEx', 'int', [PVOID, 'uint32', 'uint32', 'uint32', 'uint32', PVOID]),
+    unlockFileEx: bind(kernel32, 'UnlockFileEx', 'int', [PVOID, 'uint32', 'uint32', 'uint32', PVOID]),
    createPipe: bind(kernel32, 'CreatePipe', 'int', [PPVOID, PPVOID, PVOID, 'uint32']),
    setHandleInformation: bind(kernel32, 'SetHandleInformation', 'int', [PVOID, 'uint32', 'uint32']),
    createProcessAsUserW: bind(advapi32, 'CreateProcessAsUserW', 'int', [
@@ -357,6 +394,25 @@ export function errorText(api: Win32Bindings, win32Code: number): string {
  return buffer.subarray(0, length * 2).toString('utf16le').trim()
 }

+/**
+ * Read the process temp directory via GetTempPathW (fileapi.h line ~188).
+ * Defensive against an overlong system temp path: GetTempPathW reports the
+ * REQUIRED length (including NUL) without writing the buffer when it is too
+ * small, so a reported length beyond the buffer's capacity means the buffer
+ * was never filled and must not be decoded.
+ * @param api - the binding table.
+ * @returns the NUL-terminated temp path decoded as a string.
+ */
+export function getTempPath(api: Win32Bindings): string {
+  const buffer = Buffer.alloc((abi.MAX_PATH + 1) * 2)
+  const length = api.getTempPathW(buffer.length / 2, buffer)
+  if (length === 0) throwLastError(api, 'GetTempPathW')
+  if (length > buffer.length / 2) {
+    throw new Win32Error('GetTempPathW', abi.ERROR_INSUFFICIENT_BUFFER, `required ${length} chars exceed the ${buffer.length / 2}-char buffer; nothing was written`)
+  }
+  return buffer.subarray(0, length * 2).toString('utf16le')
+}
+
 /**
 * Throw a Win32Error for a BOOL-style API failure. MUST be called immediately
 * after the failed call so GetLastError is not clobbered by other Win32 calls.
--- a/packages/sandbox/sandbox-windows-acl/src/index.ts
+++ b/packages/sandbox/sandbox-windows-acl/src/index.ts
@@ -29,7 +29,7 @@ import { resolve } from 'node:path'

 import { grantWrite, revokeWrite } from './acl.ts'
 import { Win32Error } from './errors.ts'
-import { allocPtrSlot, decodePtr, isNullPtr, throwLastError, win32 } from './ffi.ts'
+import { allocPtrSlot, decodePtr, getTempPath, isNullPtr, throwLastError, win32 } from './ffi.ts'
 import type { NativePtr, Win32Bindings } from './ffi.ts'
 import { drainPipe, spawnSandboxed, spawnSandboxedInherited, waitForExit } from './spawn.ts'
 import { createRestrictedToken, findLogonSid, makeWellKnownSid, openCurrentProcessToken } from './token.ts'
@@ -88,13 +88,6 @@ function randomWriteSid(): string {
  return `S-1-4-${randomInt(1, 2 ** 30)}-${randomInt(1, 2 ** 30)}`
 }

-function getTempPath(api: Win32Bindings): string {
-  const buffer = Buffer.alloc((abi.MAX_PATH + 1) * 2)
-  const length = api.getTempPathW(buffer.length / 2, buffer)
-  if (length === 0) throwLastError(api, 'GetTempPathW')
-  return buffer.subarray(0, length * 2).toString('utf16le')
-}
-
 /**
 * One write-restricted sandbox instance: token + orphan-SID grants + spawn.
 * `init()` is fail-closed — any Win32 failure revokes whatever was granted
--- a/packages/sandbox/sandbox-windows-acl/src/runner.ts
+++ b/packages/sandbox/sandbox-windows-acl/src/runner.ts
@@ -124,6 +124,15 @@ async function main(): Promise<number> {

 main().then(
  (exitCode) => {
+    // Exit-code mirroring is full-width on Windows, verified empirically on
+    // this machine (Windows 11 build 26200, Node 24): a child that exits
+    // with the NTSTATUS 0xC0000005 (STATUS_ACCESS_VIOLATION) is read back
+    // by GetExitCodeProcess as the uint32 3221225477, and after
+    // process.exitCode = 3221225477 the parent observes exactly
+    // 3221225477 (spawnSync status). PowerShell's $LASTEXITCODE and cmd
+    // print the signed view (-1073741819), but no truncation or masking
+    // happens anywhere in the chain — the mirror contract holds for the
+    // full 32-bit range, so no re-mapping is needed.
    process.exitCode = exitCode
  },
  (error: unknown) => {
--- a/packages/sandbox/sandbox-windows-acl/src/spawn.ts
+++ b/packages/sandbox/sandbox-windows-acl/src/spawn.ts
@@ -14,9 +14,12 @@ import type { NativePtr, Win32Bindings } from './ffi.ts'
 import * as abi from './win32-abi.ts'

 /**
- * Quote one argument per the CommandLineToArgvW parsing rules (backslash
- * escaping only before quotes; a trailing backslash before the closing quote
- * is doubled).
+ * Quote one argument per the CommandLineToArgvW parsing rules: backslashes
+ * are doubled only before a quote character — including the closing quote
+ * this function appends, so a trailing backslash run is doubled as well
+ * (otherwise an odd run would escape the closing quote into a literal
+ * character and corrupt the rest of the command line). Mirrors the CRT
+ * ArgvQuote behavior Microsoft documents for command-line arguments.
 * @param argument - one argv entry to quote.
 * @returns the quoted entry (bare when quoting is unnecessary).
 */
@@ -30,10 +33,13 @@ export function quoteArg(argument: string): string {
      backslashes++
      index++
    }
-    if (index < argument.length && argument.charAt(index) === '"') {
+    if (index === argument.length) {
+      // Trailing backslash run: doubled so it cannot escape the closing quote.
+      quoted += '\\'.repeat(backslashes * 2)
+    } else if (argument.charAt(index) === '"') {
      quoted += '\\'.repeat(backslashes * 2 + 1) + '"'
    } else {
-      quoted += '\\'.repeat(backslashes) + (index < argument.length ? argument.charAt(index) : '')
+      quoted += '\\'.repeat(backslashes) + argument.charAt(index)
    }
  }
  return quoted + '"'
@@ -119,8 +125,19 @@ export function spawnSandboxed(
    null, options.cwd,
    startupInfo, processInfo,
  )
-  // Capture the failure before CloseHandle calls clobber GetLastError.
-  if (created === 0) throwLastError(api, 'CreateProcessAsUserW', `command: ${options.command}, cwd: ${options.cwd}`)
+  // Capture the failure before CloseHandle calls clobber GetLastError, then
+  // close every pipe handle created so far — the six-close contract this test
+  // surface pins (tests/failure-paths.spec.ts).
+  if (created === 0) {
+    const win32Code = api.getLastError()
+    api.closeHandle(stdIn.read)
+    api.closeHandle(stdIn.write)
+    api.closeHandle(stdOut.read)
+    api.closeHandle(stdOut.write)
+    api.closeHandle(stdErr.read)
+    api.closeHandle(stdErr.write)
+    throwWin32(api, 'CreateProcessAsUserW', win32Code, `command: ${options.command}, cwd: ${options.cwd}`)
+  }

  const info = decodeProcessInfo(processInfo)
  const processHandle = info.hProcess
@@ -172,7 +189,9 @@ export async function drainPipe(api: Win32Bindings, handle: NativePtr): Promise<
      }
      chunks.push(chunk.subarray(0, decodeUint32(readSlot)))
    }
-    await new Promise<void>(resolve => setImmediate(resolve))
+    // Small backoff instead of setImmediate: a bare next-tick would busy-poll
+    // the pipe at full event-loop speed while the child produces no output.
+    await new Promise<void>(resolve => setTimeout(resolve, 1))
  }
  api.closeHandle(handle)
  return Buffer.concat(chunks)
@@ -314,7 +333,16 @@ export function spawnSandboxedInherited(
    api.closeHandle(job)
    throwWin32(api, 'AssignProcessToJobObject', win32Code, `pid ${info.dwProcessId}`)
  }
-  if (api.resumeThread(threadHandle) === 0xFFFFFFFF) throwLastError(api, 'ResumeThread', `pid ${info.dwProcessId}`)
+  if (api.resumeThread(threadHandle) === 0xFFFFFFFF) {
+    // Closing the job triggers kill-on-close, so the suspended child dies
+    // instead of hanging until this process exits; the process/thread handles
+    // must go too.
+    const win32Code = api.getLastError()
+    api.closeHandle(threadHandle)
+    api.closeHandle(processHandle)
+    api.closeHandle(job)
+    throwWin32(api, 'ResumeThread', win32Code, `pid ${info.dwProcessId}`)
+  }
  api.closeHandle(threadHandle)

  return { pid: info.dwProcessId, process: processHandle, job }
--- a/packages/sandbox/sandbox-windows-acl/src/win32-abi.ts
+++ b/packages/sandbox/sandbox-windows-acl/src/win32-abi.ts
@@ -49,13 +49,26 @@ export const SE_GROUP_LOGON_ID = 0xC0000000
 export const STANDARD_RIGHTS_WRITE = 0x00020000 // == READ_CONTROL
 /** FILE_GENERIC_WRITE: every file-write permission bit plus SYNCHRONIZE. */
 export const FILE_GENERIC_WRITE = 0x00120116
-// What the POC grants: FILE_GENERIC_WRITE minus READ_CONTROL; displays as
-// "Write" in Explorer/icacls (windows-acl-restrict-poc.cpp line 16).
+/** DELETE: remove or rename the object (winnt.h line ~3009). */
+export const DELETE = 0x00010000
+/** FILE_DELETE_CHILD: remove or rename a directory's children (winnt.h line ~5907). */
+export const FILE_DELETE_CHILD = 0x0040
+// The POC granted FILE_GENERIC_WRITE minus READ_CONTROL, which displays as
+// "Write" in Explorer/icacls (windows-acl-restrict-poc.cpp line 16). The
+// sandbox grant adds DELETE and FILE_DELETE_CHILD so confined
+// delete/rename/git operations inside the granted trees pass the token's
+// access check too; Write+DELETE displays as "Modify" in icacls.
+// WRITE_DAC/WRITE_OWNER stay OUT deliberately — granting them would let the
+// child take ownership or rewrite DACLs and escape the allowlist (the
+// security boundary).
 /**
- * GRANT_MASK: FILE_GENERIC_WRITE minus READ_CONTROL — the write-access mask
- * the orphan-SID ACEs grant (displays as "Write" in Explorer/icacls).
+ * GRANT_MASK: FILE_GENERIC_WRITE minus READ_CONTROL plus DELETE and
+ * FILE_DELETE_CHILD — the write+delete access mask the orphan-SID ACEs grant
+ * (displays as "Modify" in Explorer/icacls). WRITE_DAC/WRITE_OWNER are
+ * deliberately excluded: they would let the confined child take ownership or
+ * rewrite DACLs.
 */
-export const GRANT_MASK = FILE_GENERIC_WRITE & ~STANDARD_RIGHTS_WRITE // 0x00100116
+export const GRANT_MASK = (FILE_GENERIC_WRITE | DELETE | FILE_DELETE_CHILD) & ~STANDARD_RIGHTS_WRITE // 0x00110156

 // CreateRestrictedToken flags (winnt.h lines ~4284)
 /** DISABLE_MAX_PRIVILEGE: strip the token's maximum-privilege elevation so the confined child cannot escalate. */
@@ -68,7 +81,13 @@ export const WRITE_RESTRICTED = 0x8
 // WELL_KNOWN_SID_TYPE (winnt.h lines ~3369-3407)
 /** WinWorldSid: S-1-1-0 (Everyone). */
 export const WinWorldSid = 1
-/** WinLocalSid: S-1-2-0 (LOCAL); CreateWellKnownSid(WinLocalSid) fails with ERROR_INVALID_PARAMETER on Windows 11 build 26200. */
+/**
+ * WinLocalSid: S-1-2-0 (LOCAL) — safe, created successfully on every init
+ * (it sits in every restricted token's restricting list). The
+ * CreateWellKnownSid ERROR_INVALID_PARAMETER failure documented in this
+ * module's header comment belongs to WinLocalLogonSid (S-1-2-1), NOT to
+ * this type.
+ */
 export const WinLocalSid = 2
 /** WinInteractiveSid: S-1-5-4 (INTERACTIVE). */
 export const WinInteractiveSid = 11
@@ -155,6 +174,39 @@ export const ERROR_INSUFFICIENT_BUFFER = 122
 export const ERROR_BROKEN_PIPE = 109
 /** ERROR_NO_DATA: the pipe is being closed. */
 export const ERROR_NO_DATA = 232
+/** ERROR_LOCK_VIOLATION: a byte-range lock conflicts with an existing lock (winerror.h line ~78). */
+export const ERROR_LOCK_VIOLATION = 33
+
+// ---- lock files (fileapi.h / minwinbase.h / winnt.h) -----------------------
+
+// CreateFileW dwDesiredAccess for the ACL lock files: plain read+write is
+// enough to take byte-range locks.
+/** GENERIC_READ: generic read access (winnt.h line ~3028). */
+export const GENERIC_READ = 0x80000000
+/** GENERIC_WRITE: generic write access (winnt.h line ~3029). */
+export const GENERIC_WRITE = 0x40000000
+// CreateFileW dwShareMode: the lock file is shared for read/write but NOT
+// for delete — if a locked file could be deleted and recreated underneath the
+// lock holder, two processes could hold "the same" lock on different files.
+/** FILE_SHARE_READ: other opens may read (winnt.h line ~5949). */
+export const FILE_SHARE_READ = 0x00000001
+/** FILE_SHARE_WRITE: other opens may write (winnt.h line ~5950). */
+export const FILE_SHARE_WRITE = 0x00000002
+/** FILE_SHARE_DELETE: other opens may delete (winnt.h line ~5951) — deliberately NOT used for lock files. */
+export const FILE_SHARE_DELETE = 0x00000004
+/** OPEN_ALWAYS: create the lock file if absent, open it otherwise (fileapi.h line ~21). */
+export const OPEN_ALWAYS = 4
+// LockFileEx dwFlags (minwinbase.h lines ~180-181, included by winbase.h).
+/** LOCKFILE_EXCLUSIVE_LOCK: request an exclusive byte-range lock. */
+export const LOCKFILE_EXCLUSIVE_LOCK = 0x2
+/** LOCKFILE_FAIL_IMMEDIATELY: fail with ERROR_LOCK_VIOLATION instead of waiting. */
+export const LOCKFILE_FAIL_IMMEDIATELY = 0x1
+
+// ACE_HEADER.AceFlags (winnt.h lines ~3477-3524): inherited ACEs shown when
+// reading a DACL are marked with this bit and are not part of the explicit
+// DACL edits this module makes.
+/** INHERITED_ACE: the ACE was inherited from the parent object, not stored explicitly. */
+export const INHERITED_ACE = 0x10

 // ---- job object (winnt.h lines ~4859-4866, ~5138, ~5190-5199) --------------