Skip to content

Code Quality: PR #391 #1120

Code Quality: PR #391

Code Quality: PR #391 #1120

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

codeql

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

Annotations

9 warnings
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.
Discovered test total changed from 52 to 57 without corresponding item list expansion: src/snapshot-tests/__fixtures__/json/device/test--failure.json#L34
The `discovered.total` field increased from 52 to 57, but the visible `items` array context shows the same enumerated tests. If the total no longer matches the items list length, the JSON fixture's structured output envelope may be inconsistent. Per the skill's guardrail that JSON fixtures preserve stable structured output envelopes and that fixture updates map to intentional behavior changes, this numeric change should be verified to reflect a real behavior change (e.g., new tests added) rather than an ad hoc patch to make tests pass.
release() can delete another process's lock after lease expiry due to TOCTOU: src/utils/fs-lock.ts#L142
release() in createLock reads the current owner.json, compares the token, and then calls fs.rm on the lock directory as two non-atomic steps. If the original holder's lease has expired and a concurrent contender used tryRecoverExpiredLockDir to quarantine and recreate the lock between the readLockOwner check and the fs.rm call, release() will still pass the token check (because the token was read before recovery), then unconditionally rm the directory now belonging to the new owner. Because expiresAtMs is the only liveness gate other than isPidAlive, any holder that is slow to call release() after its own lease elapses can wipe out a peer's freshly acquired lock, breaking mutual exclusion across multiple MCP servers/daemons.
[F8E-D9X] release() can delete another process's lock after lease expiry due to TOCTOU (additional location): src/daemon.ts#L336
release() in createLock reads the current owner.json, compares the token, and then calls fs.rm on the lock directory as two non-atomic steps. If the original holder's lease has expired and a concurrent contender used tryRecoverExpiredLockDir to quarantine and recreate the lock between the readLockOwner check and the fs.rm call, release() will still pass the token check (because the token was read before recovery), then unconditionally rm the directory now belonging to the new owner. Because expiresAtMs is the only liveness gate other than isPidAlive, any holder that is slow to call release() after its own lease elapses can wipe out a peer's freshly acquired lock, breaking mutual exclusion across multiple MCP servers/daemons.
PID reuse blocks lock recovery indefinitely: src/utils/fs-lock.ts#L90
shouldRecoverLockDir refuses to recover an expired lock whenever isPidAlive(staleOwner.pid) returns true, even though expiresAtMs is already in the past. Because operating systems recycle PIDs, an unrelated long-lived process can occupy the original holder's PID after a crash, making the lock appear live forever. Combined with the lease-expiry-only fallback being gated by '&&' (purpose mismatch OR not-yet-expired OR pid alive), there is no path to recover when PID is reused, leading to permanent lock starvation for the workspace artifact path.
[5QX-HMZ] PID reuse blocks lock recovery indefinitely (additional location): src/utils/workspace-filesystem-lifecycle.ts#L404
shouldRecoverLockDir refuses to recover an expired lock whenever isPidAlive(staleOwner.pid) returns true, even though expiresAtMs is already in the past. Because operating systems recycle PIDs, an unrelated long-lived process can occupy the original holder's PID after a crash, making the lock appear live forever. Combined with the lease-expiry-only fallback being gated by '&&' (purpose mismatch OR not-yet-expired OR pid alive), there is no path to recover when PID is reused, leading to permanent lock starvation for the workspace artifact path.
Socket directory creation in shared tmpdir is vulnerable to TOCTOU symlink/ownership attack: src/daemon/socket-path.ts#L89
`ensureSocketDir` calls `existsSync(dir)` then `mkdirSync(dir, { recursive: true, mode: 0o700 })` and only validates ownership/symlink status afterward. Because `daemonRunDir()` now defaults to `tmpdir()` (a world-writable shared directory) instead of `~/.xcodebuildmcp/daemons/`, a local attacker can race the daemon: pre-create the parent path or a component of it (e.g. an intermediate directory under `/tmp/xcodebuildmcp-<hash>/`) so that subsequent socket bind/connect operations occur in attacker-controlled storage. While `validateSocketDir` rejects symlinks and foreign-owned directories at the leaf, `mkdirSync({ recursive: true })` will silently accept pre-existing intermediate path components that the attacker could have created, and the validation is only applied to the leaf `dir`. On multi-user macOS/Linux hosts, this could allow a co-tenant to disrupt or hijack the daemon socket.
isPidAlive returns false for live processes owned by other users (EPERM): src/utils/process-liveness.ts#L11
On POSIX systems, `process.kill(pid, 0)` throws `EPERM` when the target PID exists but is owned by another user — this actually indicates the process IS alive. By treating `EPERM` as 'not alive', the function will incorrectly report live processes owned by other users as dead. Given this utility is used for ownership checks during multi-process cleanup (per the PR description), a false 'dead' result could cause one process to delete another live process's artifacts, which is exactly the regression the PR is trying to prevent.