Skip to content

PR #391

PR #391 #1122

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

codeql

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

Annotations

12 warnings
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 (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.
Discovered test total bumped from 52 to 57 without matching item additions: src/snapshot-tests/__fixtures__/json/device/test--failure.json#L34
The fixture changes `discovered.total` from 52 to 57, but the surrounding hunk and context only show the same item list as before with no five new test entries appended. If the underlying test discovery genuinely added five tests, the `items` array should also list them; otherwise this looks like a fixture being patched to make tests pass rather than reflecting an intentional behavior change, which violates the guardrail that fixture updates must map to intentional behavior changes and stay aligned across MCP/CLI/JSON outputs.
TOCTOU race in ensureSocketDir when daemon dir lives under /tmp: src/daemon/socket-path.ts#L84
ensureSocketDir uses existsSync followed by mkdirSync, then validateSocketDir runs lstatSync, statSync and chmodSync as separate syscalls. With the daemon directory now under tmpdir() (typically world-writable /tmp), a local attacker who can predict the 12-hex-char workspace hash can race between these calls — e.g. swap the directory for a symlink after the lstat check but before chmodSync, which follows symlinks. Ownership checks mitigate cross-user attacks, but the non-atomic validate-then-chmod sequence still weakens the previous guarantees provided when the directory lived under the user's home (~/.xcodebuildmcp). Consider opening the directory with O_NOFOLLOW/O_DIRECTORY via fs.openSync and operating on the file descriptor (fchmod/fstat) to make validation atomic.
EPERM from process.kill incorrectly reports live PID as dead: src/utils/process-liveness.ts#L11
When `process.kill(pid, 0)` raises EPERM, the target process exists but is owned by another user — the PID is alive. This implementation returns `false` in that case (`code !== 'ESRCH' && code !== 'EPERM'`), so a live PID owned by another user will be treated as dead. Given this helper is used for ownership checks during workspace filesystem cleanup, a stale-looking-but-live process could have its artifacts (daemon files, OSLog helpers, logs) deleted out from under it, which is exactly the multi-process boundary the PR is trying to protect.
Forced shutdown timer and server.close callback can concurrently run cleanupArtifacts and race process.exit: src/daemon.ts#L336
When the 5s forced shutdown timer fires it sets forcedShutdownTimer=null and then invokes cleanupArtifacts() asynchronously. If server.close() completes during that async cleanup, its callback sees forcedShutdownTimer===null, skips clearTimeout, and starts a second concurrent cleanupArtifacts() call. Both paths then race to call process.exit with different exit codes (1 vs exitCode), and two concurrent cleanups of workspace-owned artifacts run against the same workspaceKey/socketPath — exactly the multi-process cleanup boundary this PR is trying to protect.
[K8M-WKN] Forced shutdown timer and server.close callback can concurrently run cleanupArtifacts and race process.exit (additional location): src/daemon.ts#L345
When the 5s forced shutdown timer fires it sets forcedShutdownTimer=null and then invokes cleanupArtifacts() asynchronously. If server.close() completes during that async cleanup, its callback sees forcedShutdownTimer===null, skips clearTimeout, and starts a second concurrent cleanupArtifacts() call. Both paths then race to call process.exit with different exit codes (1 vs exitCode), and two concurrent cleanups of workspace-owned artifacts run against the same workspaceKey/socketPath — exactly the multi-process cleanup boundary this PR is trying to protect.
server 'error' handler only releases lock and never shuts down or exits the daemon: src/daemon.ts#L420
The new `server.on('error', releaseStartupRegistryLock)` handler at line 420 only releases the startup registry lock when the underlying net server emits an error (e.g., EADDRINUSE on the socket path, or any later server-level error). It does not call `shutdown`, log the error, propagate to Sentry, or exit the process. If `server.listen(socketPath, ...)` fails, the listen callback never runs, the outer `main()` Promise has already resolved, and the process is left alive with no listener and no exit path. The user-visible effect is a silently broken daemon process that holds no socket but never exits.
runStartupLifecycleSweep rejection becomes unhandledRejection and crashes the daemon: src/daemon.ts#L458
At line 458, `void runStartupLifecycleSweep()` is fire-and-forget with no `.catch` handler, unlike `enrichSentryMetadata()` immediately above which is wrapped with `.catch`. Because `process.on('unhandledRejection', handleCrash)` is registered (line 475) and `handleCrash` calls `shutdown(1)`, any rejection from the startup lifecycle sweep (e.g., transient filesystem errors during orphan reconciliation or log retention) will crash the just-started daemon with exit code 1. The comment at lines 450-452 states the sweep is intentionally fire-and-forget so it cannot delay request serving, but the missing rejection handler means a sweep failure does the opposite — it terminates the daemon.
Recovery without owner.json can delete a freshly acquired lock (TOCTOU): src/utils/fs-lock-sync.ts#L76
When `shouldRecoverLockDir` returns `{ recover: true, owner: null }` (lock dir exists, has no owner.json, and is older than the lease), `tryRecoverExpiredLockDir` renames the lock dir to a quarantine path and unconditionally `rmSync`s it without any post-rename verification. If another process atomically created a valid lock dir at the same path between the `shouldRecoverLockDir` check and the `renameSync`, that fresh dir (along with whatever owner.json/contents it now has) is what gets quarantined and deleted. Owner equality is only validated when `recovery.owner` is non-null, so the no-owner branch silently destroys a possibly-live lock. This race is most reachable through `tryAcquireGuard` because guard recovery does not run under any outer lock.
normalizeWorkspaceKey allows '..' and other traversal-prone keys: src/utils/log-paths.ts#L39
normalizeWorkspaceKey only rejects '/' and '\\' but allows values like '..', '.', or strings containing null bytes. Because the normalized key is joined directly into a filesystem path under the workspaces dir, a caller passing '..' would resolve to the parent (XcodeBuildMCP app dir) and subsequent cleanup/retention logic could operate on or delete files outside the intended workspace sandbox. Given the PR's emphasis on ownership boundaries between workspaces, this weakens that boundary if any caller ever derives the key from untrusted input.
isPidAlive returns false for live processes owned by another user (EPERM): src/utils/process-liveness.ts#L11
When `process.kill(pid, 0)` fails with `EPERM`, the target PID exists but belongs to another user/process the caller cannot signal. The current logic returns `code !== 'ESRCH' && code !== 'EPERM'`, which treats EPERM as 'not alive'. Given this helper is used to gate cleanup of workspace-owned artifacts (daemons, simulator OSLog helpers), treating a live-but-foreign PID as dead can cause the process to delete or reclaim files belonging to another active process — exactly the multi-process safety boundary this PR is trying to enforce.