Skip to content

PR #391

PR #391 #1128

Triggered via dynamic May 4, 2026 18:25
Status Success
Total duration 1m 21s
Artifacts

codeql

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

Annotations

5 errors and 11 warnings
server 'error' handler only releases startup lock and never shuts down or exits: src/daemon.ts#L420
When `server.listen(socketPath, ...)` fails (e.g., EADDRINUSE, EACCES, permission errors creating the unix socket), the registered error handler is `releaseStartupRegistryLock` which only releases the registry lock. It does not call `shutdown()`, exit the process, flush Sentry, or surface the error to the user. The daemon process will remain alive without a listening socket, and the parent CLI/user will receive a misleading 'Daemon started' line only if listen succeeded—if it failed, neither writeLine nor any error path runs, leaving an orphan process and a confused caller.
[LZ8-9PH] server 'error' handler only releases startup lock and never shuts down or exits (additional location): src/daemon.ts#L336
When `server.listen(socketPath, ...)` fails (e.g., EADDRINUSE, EACCES, permission errors creating the unix socket), the registered error handler is `releaseStartupRegistryLock` which only releases the registry lock. It does not call `shutdown()`, exit the process, flush Sentry, or surface the error to the user. The daemon process will remain alive without a listening socket, and the parent CLI/user will receive a misleading 'Daemon started' line only if listen succeeded—if it failed, neither writeLine nor any error path runs, leaving an orphan process and a confused caller.
[LZ8-9PH] server 'error' handler only releases startup lock and never shuts down or exits (additional location): src/daemon.ts#L426
When `server.listen(socketPath, ...)` fails (e.g., EADDRINUSE, EACCES, permission errors creating the unix socket), the registered error handler is `releaseStartupRegistryLock` which only releases the registry lock. It does not call `shutdown()`, exit the process, flush Sentry, or surface the error to the user. The daemon process will remain alive without a listening socket, and the parent CLI/user will receive a misleading 'Daemon started' line only if listen succeeded—if it failed, neither writeLine nor any error path runs, leaving an orphan process and a confused caller.
TOCTOU race when recovering lock without owner file lets fresh locks be deleted: src/utils/fs-lock.ts#L109
In tryRecoverExpiredLockDir, when shouldRecoverLockDir returns recovery.owner === null (age-based fallback for missing/invalid owner.json), the post-quarantine verification block is skipped entirely. Between shouldRecoverLockDir reading the directory and quarantineLockDir renaming it, another process can create a brand-new, valid lock at lockDir. The rename then captures that fresh lock, the null-owner branch removes it without checking the quarantined contents, and the legitimate lock holder's directory is silently destroyed. This breaks the multi-process cleanup boundaries that this PR is intended to enforce.
[N7J-Y3R] TOCTOU race when recovering lock without owner file lets fresh locks be deleted (additional location): src/utils/fs-lock.ts#L211
In tryRecoverExpiredLockDir, when shouldRecoverLockDir returns recovery.owner === null (age-based fallback for missing/invalid owner.json), the post-quarantine verification block is skipped entirely. Between shouldRecoverLockDir reading the directory and quarantineLockDir renaming it, another process can create a brand-new, valid lock at lockDir. The rename then captures that fresh lock, the null-owner branch removes it without checking the quarantined contents, and the legitimate lock holder's directory is silently destroyed. This breaks the multi-process cleanup boundaries that this PR is intended to enforce.
Analyze (actions)
Starting April 2026, the CodeQL Action will skip computing file coverage information on pull requests to improve analysis performance. File coverage information will still be computed on non-PR analyses. To opt out of this change, create a custom repository property with the name `github-codeql-file-coverage-on-prs` and the type "True/false", then set this property to `true` in the repository's settings.
Analyze (python)
Starting April 2026, the CodeQL Action will skip computing file coverage information on pull requests to improve analysis performance. File coverage information will still be computed on non-PR analyses. To opt out of this change, create a custom repository property with the name `github-codeql-file-coverage-on-prs` and the type "True/false", then set this property to `true` in the repository's settings.
Analyze (javascript-typescript)
Starting April 2026, the CodeQL Action will skip computing file coverage information on pull requests to improve analysis performance. File coverage information will still be computed on non-PR analyses. To opt out of this change, create a custom repository property with the name `github-codeql-file-coverage-on-prs` and the type "True/false", then set this property to `true` in the repository's settings.
Test fixture 'discovered.total' bumped from 52 to 57 without corresponding items: src/snapshot-tests/__fixtures__/json/device/test--failure.json#L34
The fixture's `discovered.total` was changed from 52 to 57, but the visible `items` array does not appear to grow by 5 entries to match. If the total no longer matches the items list, the JSON fixture's structured output envelope is inconsistent, suggesting the fixture may have been updated solely to make tests pass rather than to reflect an intentional, coherent behavior change. This violates the guardrail that fixture updates map to intentional behavior changes and that JSON fixtures preserve stable structured output envelopes.
Predictable socket directory in world-writable tmpdir enables pre-creation attacks: src/daemon/socket-path.ts#L89
The daemon socket directory is now placed in `tmpdir()` (typically `/tmp`, world-writable) at a path derived from a 12-hex-char (48-bit) hash of the workspace root. On multi-user systems, an attacker who can guess the victim's workspace path can pre-create `/tmp/xcodebuildmcp-<hash>` before the victim's daemon starts. While `validateSocketDir` does check ownership via `stat.uid !== uid` after `mkdirSync`, the `mkdirSync({ recursive: true })` call silently succeeds if the directory already exists, so the protection relies entirely on the uid check. Any later TOCTOU between `lstatSync`/`statSync` and the subsequent socket bind in `ensureSocketDir`'s caller is not protected by file-handle-based validation (e.g., `O_NOFOLLOW` open of a dirfd).
normalizeWorkspaceKey allows '.' and '..' enabling path traversal: src/utils/log-paths.ts#L38
normalizeWorkspaceKey only rejects empty strings and path separators ('/' and '\\'), but does not reject '.', '..', null bytes, or other path-traversal sequences. A workspace key of '..' would resolve to the parent of the workspaces directory, allowing getWorkspaceFilesystemLayout to return paths outside the intended workspaces root. Since these layouts drive cleanup, lock, and DerivedData operations, a maliciously- or accidentally-crafted workspace key could cause file operations (including deletions during cleanup) to occur outside the intended workspace tree.
isPidAlive treats EPERM as alive but ESRCH as dead — inverted logic risk for unrelated errors: src/utils/process-liveness.ts#L9
The function returns true when `process.kill(pid, 0)` throws with code EPERM (correct: process exists but we lack permission) and false on ESRCH (correct: no such process). However, for any other error code (e.g. EINVAL, or undefined code), it currently returns true, falsely reporting a process as alive. In the workspace cleanup context this means stale helper/daemon artifacts owned by dead PIDs could be preserved indefinitely if an unexpected error occurs, defeating the reconciliation logic described in the PR.
Scheduled sweep cooldown can be bypassed by concurrent schedule calls: src/utils/workspace-filesystem-lifecycle.ts#L408
`scheduleWorkspaceFilesystemLifecycleSweep` checks `lastScheduledAtByScope`/`lastScheduledAtByPreKey` and `runningScheduledSweeps` but only updates `lastScheduledAt*` after the sweep completes inside the timer callback. Multiple synchronous calls within the same tick (before the timer fires and before `runningScheduledSweeps.add` is observed by another caller in another async context) — or repeated calls between the timer's `setTimeout` firing and the cooldown maps being updated — will all pass the cooldown check and queue redundant work. While `runningScheduledSweeps` partially mitigates this, it is only added synchronously after both checks, so concurrent invocations from different async contexts may schedule duplicate timers before the running flag suppresses them. This can lead to repeated cleanup sweeps running back-to-back instead of being throttled by the cooldown.
TOCTOU race in validateSocketDir between symlink check and chmodSync allows permission change on attacker target: src/daemon/socket-path.ts#L84
validateSocketDir performs lstatSync to ensure the directory is not a symlink, then later calls chmodSync(dir, 0o700) when permissions are too loose. Because the directory now lives under tmpdir() (e.g. /tmp, world-writable with sticky bit), a local attacker on shared systems can replace the directory with a symlink in the window between the lstat/stat checks and chmodSync. chmodSync follows symlinks, so this can change permissions of an attacker-chosen target file owned by the current user.
[29F-9QC] TOCTOU race in validateSocketDir between symlink check and chmodSync allows permission change on attacker target (additional location): src/daemon/socket-path.ts#L89
validateSocketDir performs lstatSync to ensure the directory is not a symlink, then later calls chmodSync(dir, 0o700) when permissions are too loose. Because the directory now lives under tmpdir() (e.g. /tmp, world-writable with sticky bit), a local attacker on shared systems can replace the directory with a symlink in the window between the lstat/stat checks and chmodSync. chmodSync follows symlinks, so this can change permissions of an attacker-chosen target file owned by the current user.
isPidAlive returns true for ESRCH (dead process) and false for EPERM (live process): src/utils/process-liveness.ts#L11
The error handling logic is inverted. When process.kill returns ESRCH, the process does not exist (dead), but `code !== 'ESRCH'` evaluates to false correctly. However, when EPERM is returned, the process IS alive (we just lack permission to signal it), yet this code returns false, incorrectly reporting a live process as dead. This will cause ownership checks to treat live processes owned by other users as dead, potentially leading to deletion of artifacts belonging to active processes — exactly the multi-process safety boundary this PR aims to protect.