Skip to content

PR #391

PR #391 #1130

Triggered via dynamic May 4, 2026 19:03
Status Success
Total duration 1m 34s
Artifacts

codeql

on: dynamic
Matrix: analyze
Fit to window
Zoom out
Zoom in

Annotations

2 errors and 11 warnings
TOCTOU race between lstatSync symlink check and subsequent statSync/chmodSync allows symlink attacks in shared tmpdir: src/daemon/socket-path.ts#L68
validateSocketDir uses lstatSync to confirm the directory is not a symlink, then calls statSync and chmodSync on the same path. Because the daemon directory lives under tmpdir() (a world-writable location like /tmp), a local attacker can replace the directory with a symlink between the lstat and the follow-up calls. statSync and chmodSync follow symlinks, so chmodSync 0o700 could be redirected to modify permissions on an arbitrary file owned by the current user. This is a classic TOCTOU symlink vulnerability on a security-sensitive socket directory.
[SYD-QKZ] TOCTOU race between lstatSync symlink check and subsequent statSync/chmodSync allows symlink attacks in shared tmpdir (additional location): src/daemon/socket-path.ts#L89
validateSocketDir uses lstatSync to confirm the directory is not a symlink, then calls statSync and chmodSync on the same path. Because the daemon directory lives under tmpdir() (a world-writable location like /tmp), a local attacker can replace the directory with a symlink between the lstat and the follow-up calls. statSync and chmodSync follow symlinks, so chmodSync 0o700 could be redirected to modify permissions on an arbitrary file owned by the current user. This is a classic TOCTOU symlink vulnerability on a security-sensitive socket directory.
Discovered test total bumped from 52 to 57 without matching item list expansion: src/snapshot-tests/__fixtures__/json/device/test--failure.json#L34
The fixture changes `discovered.total` from 52 to 57, but the visible diff only shows the same item list as before. If the underlying test discovery genuinely added 5 tests, the `items` array should grow accordingly; if not, this looks like a fixture being patched to make tests pass rather than reflecting an intentional behavior change. This risks violating the guardrail that fixture updates must map to intentional behavior changes and stay aligned across MCP/CLI/JSON outputs.
isPidAlive returns true for dead PIDs when kill returns EPERM in some cases, but more critically returns false for ESRCH correctly — however EPERM handling is inverted for liveness semantics: src/utils/process-liveness.ts#L11
When `process.kill(pid, 0)` throws `EPERM`, the process exists but the caller lacks permission to signal it — this means the PID *is* alive. The current code returns `code !== 'ESRCH' && code !== 'EPERM'`, which returns `false` for EPERM, incorrectly reporting a live process as dead. This can cause ownership/cleanup logic to delete artifacts belonging to live processes owned by another user, defeating the multi-process cleanup boundaries this PR is meant to enforce.
shutdown() does not release startup registry lock if invoked before listen callback: src/daemon.ts#L484
The startup registry lock is only released inside the `server.listen` callback's `finally` (line 456), inside the synchronous catch (line 492), or in `handleStartupServerError` (line 426). If `shutdown()` is invoked between `acquireDaemonRegistryMutationLock` (line 186) and the listen callback executing — for example via SIGTERM/SIGINT after handlers register at lines 487-488, the idle timer firing, or an uncaughtException — the daemon exits without calling `releaseStartupRegistryLock`. The registry lock then remains held for up to its 30-second lease (DAEMON_REGISTRY_LOCK_LEASE_MS), preventing another daemon from starting in that workspace until the lease expires.
[G6N-XZ5] shutdown() does not release startup registry lock if invoked before listen callback (additional location): src/daemon.ts#L425
The startup registry lock is only released inside the `server.listen` callback's `finally` (line 456), inside the synchronous catch (line 492), or in `handleStartupServerError` (line 426). If `shutdown()` is invoked between `acquireDaemonRegistryMutationLock` (line 186) and the listen callback executing — for example via SIGTERM/SIGINT after handlers register at lines 487-488, the idle timer firing, or an uncaughtException — the daemon exits without calling `releaseStartupRegistryLock`. The registry lock then remains held for up to its 30-second lease (DAEMON_REGISTRY_LOCK_LEASE_MS), preventing another daemon from starting in that workspace until the lease expires.
[G6N-XZ5] shutdown() does not release startup registry lock if invoked before listen callback (additional location): src/daemon.ts#L441
The startup registry lock is only released inside the `server.listen` callback's `finally` (line 456), inside the synchronous catch (line 492), or in `handleStartupServerError` (line 426). If `shutdown()` is invoked between `acquireDaemonRegistryMutationLock` (line 186) and the listen callback executing — for example via SIGTERM/SIGINT after handlers register at lines 487-488, the idle timer firing, or an uncaughtException — the daemon exits without calling `releaseStartupRegistryLock`. The registry lock then remains held for up to its 30-second lease (DAEMON_REGISTRY_LOCK_LEASE_MS), preventing another daemon from starting in that workspace until the lease expires.
TOCTOU race between lstatSync, statSync, and chmodSync in /tmp socket directory validation: src/daemon/socket-path.ts#L68
validateSocketDir performs lstatSync, statSync, and chmodSync as separate syscalls on a path under tmpdir() (typically world-writable /tmp). Between these calls, a local attacker on the same host can replace the directory with a symlink or attacker-owned directory, defeating the symlink/ownership/mode checks. Because the daemon socket lives in this directory, a successful race could let another local user influence the daemon's socket placement or permissions. Using fd-based operations (openSync with O_NOFOLLOW + fstatSync + fchmodSync) would close the window.
[KPC-7UW] TOCTOU race between lstatSync, statSync, and chmodSync in /tmp socket directory validation (additional location): src/daemon/socket-path.ts#L89
validateSocketDir performs lstatSync, statSync, and chmodSync as separate syscalls on a path under tmpdir() (typically world-writable /tmp). Between these calls, a local attacker on the same host can replace the directory with a symlink or attacker-owned directory, defeating the symlink/ownership/mode checks. Because the daemon socket lives in this directory, a successful race could let another local user influence the daemon's socket placement or permissions. Using fd-based operations (openSync with O_NOFOLLOW + fstatSync + fchmodSync) would close the window.
TOCTOU race in release() can delete another process's lock: src/utils/fs-lock.ts#L142
release() reads the owner file and verifies the token matches before calling fs.rm on lockDir. Between the readLockOwner check and the fs.rm call, another process could observe our lease as expired (if release runs after expiresAtMs), quarantine our directory, and create a fresh lock at the same path. The original holder's fs.rm then recursively deletes the new holder's lock directory. While this requires the original holder's release to run after lease expiration, leaseMs is configurable per call and there is no monotonic check that our lease is still valid before deletion. Result: a slow releaser can destroy a legitimate successor's lock, breaking mutual exclusion guarantees the rest of the system relies on.
isPidAlive returns false for EPERM, treating live processes as dead: src/utils/process-liveness.ts#L10
`process.kill(pid, 0)` throws EPERM when the target PID exists but is owned by another user (i.e., the process IS alive). The current logic `code !== 'ESRCH' && code !== 'EPERM'` returns false for EPERM, incorrectly classifying live processes owned by other users as dead. Since this function gates ownership checks for daemon/simulator helper cleanup across multi-process workspaces, a workspace process running as a different uid could be considered dead and have its artifacts (logs, sockets, OSLog outputs) deleted while still active — exactly the multi-process cleanup boundary issue the PR description warns about.
scheduleWorkspaceFilesystemLifecycleSweep never records preKey timestamp on first run, causing redundant scheduling: src/utils/workspace-filesystem-lifecycle.ts#L389
buildSchedulePreKey-based throttling uses lastScheduledAtByPreKey, but the map is only updated inside the setTimeout callback after the sweep completes (and only when not skipped). Between the time scheduleWorkspaceFilesystemLifecycleSweep is invoked and the sweep finishes (SCHEDULE_DELAY_MS + sweep duration), repeated calls with the same preKey will all bypass the cooldown gate because lastScheduledAtByPreKey has no entry yet. They will be deduplicated by runningScheduledSweeps only if the scheduleKey (workspaceKey:logDir) matches; with different logDir values for the same workspaceKey, this can lead to many concurrent timers/sweeps before the cooldown takes effect.
[2PY-QCL] scheduleWorkspaceFilesystemLifecycleSweep never records preKey timestamp on first run, causing redundant scheduling (additional location): src/utils/workspace-filesystem-lifecycle.ts#L405
buildSchedulePreKey-based throttling uses lastScheduledAtByPreKey, but the map is only updated inside the setTimeout callback after the sweep completes (and only when not skipped). Between the time scheduleWorkspaceFilesystemLifecycleSweep is invoked and the sweep finishes (SCHEDULE_DELAY_MS + sweep duration), repeated calls with the same preKey will all bypass the cooldown gate because lastScheduledAtByPreKey has no entry yet. They will be deduplicated by runningScheduledSweeps only if the scheduleKey (workspaceKey:logDir) matches; with different logDir values for the same workspaceKey, this can lead to many concurrent timers/sweeps before the cooldown takes effect.