fix(sandbox): address review: leak FIXME, legal ACL fixture, stronger offset test, prose

packages/sandbox/sandbox-windows-acl/src/index.ts

@@ -249,8 +249,12 @@ export class AclSandbox {
     } catch (error) {
       // Best-effort close on the failure path (last error already captured in `error`).
       api.closeHandle(currentToken)
-      // Fail-closed cleanup: never leave a revocable (temp) grant or SID
-      // allocation behind a failed init. Standing workspace ACEs are NOT
+      // FIXME(windows-acl): a failure after createRestrictedToken leaks the restricted
+      // token handle and the parsed write SID — this.api stays undefined, so dispose()
+      // early-returns and cannot clean them up. Close the token and free the write SID
+      // here (the hardening-followup rework already does both).
+      // Fail-closed cleanup: revoke the revocable (temp) grants and free the init SID
+      // allocations a failed init left behind. Standing workspace ACEs are NOT
       // revoked — they are the intended end state (the reuse cache), not an
       // error artifact.
       const cleanupFailures: unknown[] = []
@@ -361,7 +365,7 @@ export class AclSandbox {
     }
     const token = this.token
     /* v8 ignore next -- init assigns this.api only after this.token, so an initialized instance always
-       has its token; the guard mirrors the write-SID guard's defensive shape. */
+       has its token; the guard mirrors the write-SID guard. */
     if (token !== undefined) {
       try {
         if (api.closeHandle(token) === 0) throwLastError(api, 'CloseHandle', 'restricted token')

packages/sandbox/sandbox-windows-acl/tests/acl-failure-paths.spec.ts

@@ -72,12 +72,12 @@ function craftSid(revision: number, count: number, authority: number[] = [0, 0,
 function craftAclWithGrant(sid: NativePtr, match: boolean): NativePtr {
   const acl = allocBytes(32)
   koffi.encode(acl, 'uint8', 2) // AclRevision
-  koffi.encode(acl, 2, 'uint16', 16) // AclSize: header + one 8-byte-SID ACE
+  koffi.encode(acl, 2, 'uint16', 24) // AclSize: 8-byte header + one 16-byte ACE
   koffi.encode(acl, 4, 'uint16', 1) // AceCount
   const ace = 8
   koffi.encode(acl, ace + 0, 'uint8', abi.ACCESS_ALLOWED_ACE_TYPE)
   koffi.encode(acl, ace + 1, 'uint8', abi.SUB_CONTAINERS_AND_OBJECTS_INHERIT)
-  koffi.encode(acl, ace + 2, 'uint16', 8)
+  koffi.encode(acl, ace + 2, 'uint16', 16) // AceSize: header + mask + inline 8-byte SID
   koffi.encode(acl, ace + 4, 'uint32', abi.GRANT_MASK)
   const inlineSid = ace + 8
   for (let offset = 0; offset < 8; offset++) {

packages/sandbox/sandbox-windows-acl/tests/ffi.spec.ts

@@ -182,9 +182,21 @@ describe('sameSidAt bounded comparison', () => {
     expect(sameSidAt(left, 0, right, 0)).toBe(false)
   })
 
-  it('accepts identical SIDs at nonzero offsets', () => {
-    const left = craftSid(1, 1, [0, 0, 0, 0, 0, 5], [42])
-    const right = craftSid(1, 1, [0, 0, 0, 0, 0, 5], [42])
+  it('accepts identical SIDs at nonzero offsets over differing leading bytes', () => {
+    const sid = craftSid(1, 1, [0, 0, 0, 0, 0, 5], [42])
+    // Embed the same SID bytes at offset 4 of two buffers whose first four
+    // bytes differ: an offset-ignoring comparison reads the differing
+    // prefixes and must reject.
+    const left = allocBytes(4 + 12)
+    const right = allocBytes(4 + 12)
+    koffi.encode(left, 0, 'uint32', 0x11111111)
+    koffi.encode(right, 0, 'uint32', 0x22222222)
+    for (let offset = 0; offset < 12; offset++) {
+      const byte = koffi.decode(sid, offset, 'uint8') as number
+      koffi.encode(left, 4 + offset, 'uint8', byte)
+      koffi.encode(right, 4 + offset, 'uint8', byte)
+    }
     expect(sameSidAt(left, 4, right, 4)).toBe(true)
+    expect(sameSidAt(left, 0, right, 0)).toBe(false) // the differing prefixes are not a matching SID
   })
 })

packages/sandbox/sandbox-windows-acl/tests/index-failure-paths.spec.ts

@@ -270,10 +270,11 @@ describe('AclSandbox init', () => {
     // fresh() hands out 1n to OpenProcess and 2n to OpenProcessToken; the
     // token-layer close of 1n succeeds and init's close of 2n fails.
     closeHandle.mockImplementation((handle: NativePtr) => (handle === 2n ? 0 : 1))
+    // The failure lands after this.token is stored but before this.api is
+    // assigned; the catch drains the SID allocations and rethrows the
+    // original error. (The stored restricted token and parsed write SID leak
+    // until process exit — see the FIXME in init's catch.)
     await expect(sandbox.init()).rejects.toMatchObject({ api: 'CloseHandle' })
-    // The failed init never stored a restricted token: dispose skips the
-    // token close and the already-drained allocations.
-    expect(() => { sandbox.dispose() }).not.toThrow()
   })
 
   it('revokes the revocable grants and aggregates cleanup failures when the token pipeline fails', async () => {