Skip to content

Code Quality: PR #391 #1140

Code Quality: PR #391

Code Quality: PR #391 #1140

Triggered via dynamic May 5, 2026 08:20
Status Success
Total duration 1m 30s
Artifacts

codeql

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

Annotations

1 error and 11 warnings
Daemon socket directory in shared tmpdir is vulnerable to symlink pre-creation attack: src/daemon/socket-path.ts#L89
`daemonDirForWorkspaceKey` places the socket directory under `tmpdir()` (typically `/tmp`), which is writable by any local user. The workspace hash is deterministic from a path that may be predictable (e.g., `/Users/<name>/projects/<name>`), so a local attacker can pre-create `xcodebuildmcp-<hash>` as a symlink targeting an attacker-controlled directory before the daemon starts. `mkdirSync` with `recursive: true` silently succeeds when the path already exists, including when it is a symlink to an existing directory. Although `validateSocketDir` rejects symlinks afterwards via `lstatSync`, the daemon will refuse to start while the attacker has effectively performed a denial-of-service; if symlink target is owned by the user and not a symlink itself (e.g., the user's `~/.ssh`), the ownership check passes and `chmodSync` may then alter permissions on that target.
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.total bumped from 52 to 57 without matching items list expansion: src/snapshot-tests/__fixtures__/json/device/test--failure.json#L34
The hunk increases `tests.discovered.total` from 52 to 57, but the visible items list in the surrounding context shows no corresponding additions of five new test entries. If the underlying test count did not actually grow by five, this fixture change would be a snapshot-only patch unrelated to the workspace filesystem cleanup behavior change, which violates the guardrail that fixture updates must map to intentional behavior changes and stay aligned with structured output. Reviewers should confirm the items array elsewhere in the fixture grew by exactly 5 entries; otherwise this is a fixture patched only to make tests pass.
Stale socket no longer removed when registry metadata is missing: src/cli/daemon-control.ts#L90
The previous implementation always called removeStaleSocket(socketPath) at the end of forceStopDaemon, ensuring an orphaned socket file would be cleaned up even when no registry entry existed. The new implementation throws early when findDaemonRegistryEntryBySocketPath returns null, so stale sockets without a matching registry entry are never cleaned up by this path. This can leave orphaned socket files on disk that prevent subsequent daemon startup from binding to the expected path.
[YSU-WDD] Stale socket no longer removed when registry metadata is missing (additional location): src/cli/daemon-control.ts#L91
The previous implementation always called removeStaleSocket(socketPath) at the end of forceStopDaemon, ensuring an orphaned socket file would be cleaned up even when no registry entry existed. The new implementation throws early when findDaemonRegistryEntryBySocketPath returns null, so stale sockets without a matching registry entry are never cleaned up by this path. This can leave orphaned socket files on disk that prevent subsequent daemon startup from binding to the expected path.
Daemon socket directory placed in shared tmpdir with predictable name enables pre-creation DoS: src/daemon/socket-path.ts#L21
`daemonRunDir()` defaults to `os.tmpdir()` (typically `/tmp`, shared/world-writable with sticky bit), and `daemonDirForWorkspaceKey` produces a deterministic name `xcodebuildmcp-<12-hex>` derived from the workspace path hash. A local unprivileged attacker who can predict or enumerate workspace hashes can pre-create that directory under their own UID before the daemon starts. `ensureSocketDir` will then skip `mkdirSync` (because `existsSync` is true), and `validateSocketDir` will throw on the uid mismatch, preventing the daemon from ever starting for that workspace. The validation correctly defends against symlink/uid hijacking of an existing dir, but the predictable name in a shared directory still allows trivial denial-of-service against any user on a multi-tenant host.
[55B-9B7] Daemon socket directory placed in shared tmpdir with predictable name enables pre-creation DoS (additional location): src/daemon/socket-path.ts#L89
`daemonRunDir()` defaults to `os.tmpdir()` (typically `/tmp`, shared/world-writable with sticky bit), and `daemonDirForWorkspaceKey` produces a deterministic name `xcodebuildmcp-<12-hex>` derived from the workspace path hash. A local unprivileged attacker who can predict or enumerate workspace hashes can pre-create that directory under their own UID before the daemon starts. `ensureSocketDir` will then skip `mkdirSync` (because `existsSync` is true), and `validateSocketDir` will throw on the uid mismatch, preventing the daemon from ever starting for that workspace. The validation correctly defends against symlink/uid hijacking of an existing dir, but the predictable name in a shared directory still allows trivial denial-of-service against any user on a multi-tenant host.
chmodSync after statSync allows TOCTOU permission change on attacker-swapped path: src/daemon/socket-path.ts#L84
`validateSocketDir` calls `statSync(dir)` then `chmodSync(dir, 0o700)` if permissions are too loose. Between these calls, a local attacker who can win the race could swap the path to a symlink (or replace the directory) so the chmod applies to a different filesystem object owned by the same user. Combined with the predictable path under `tmpdir()`, this enables tampering with the permissions of arbitrary user-owned directories. Using `fchmod` on an opened file descriptor (with `O_NOFOLLOW`) would close the window.
[HZS-JCC] chmodSync after statSync allows TOCTOU permission change on attacker-swapped path (additional location): src/utils/fs-lock.ts#L113
`validateSocketDir` calls `statSync(dir)` then `chmodSync(dir, 0o700)` if permissions are too loose. Between these calls, a local attacker who can win the race could swap the path to a symlink (or replace the directory) so the chmod applies to a different filesystem object owned by the same user. Combined with the predictable path under `tmpdir()`, this enables tampering with the permissions of arbitrary user-owned directories. Using `fchmod` on an opened file descriptor (with `O_NOFOLLOW`) would close the window.
Pre-key cooldown skip can starve scheduling when sweeps never complete: src/utils/workspace-filesystem-lifecycle.ts#L401
buildSchedulePreKey-based skip in scheduleWorkspaceFilesystemLifecycleSweep reads lastScheduledAtByPreKey but that map is only updated after a non-skipped sweep completes. Because the entry is set at completedAt (after the schedule delay plus sweep duration), and the same value is also used to short-circuit future schedule calls, this is benign — but the pre-key check uses options.now ?? Date.now() while the post-completion write uses Date.now(); if a caller passes a fixed options.now in the past for tests, the cooldown check can incorrectly skip indefinitely. This can cause scheduled sweeps to be silently dropped in test or time-controlled environments.
[BVG-EGW] Pre-key cooldown skip can starve scheduling when sweeps never complete (additional location): src/utils/workspace-filesystem-lifecycle.ts#L426
buildSchedulePreKey-based skip in scheduleWorkspaceFilesystemLifecycleSweep reads lastScheduledAtByPreKey but that map is only updated after a non-skipped sweep completes. Because the entry is set at completedAt (after the schedule delay plus sweep duration), and the same value is also used to short-circuit future schedule calls, this is benign — but the pre-key check uses options.now ?? Date.now() while the post-completion write uses Date.now(); if a caller passes a fixed options.now in the past for tests, the cooldown check can incorrectly skip indefinitely. This can cause scheduled sweeps to be silently dropped in test or time-controlled environments.