Files
software-mansion__argent/packages/tool-server/test/native-profiler-ios-run-with-timeout.test.ts
Ignacy Łątka 72fea93d7f fix(native-profiler): remove shell interpolation from iOS profiler (device_id + trace export) (#378)
## Summary
The iOS native profiler reached `/bin/sh` by string-interpolating values
into shell-executed commands — the classic command-injection shape. This
PR removes the shell from **every** such call site in the iOS-profiler
subsystem, converting them to argv-array `execFileSync`/`execFile` so no
caller- or path-derived value is ever re-parsed by a shell.

### 1. `device_id` interpolation — gated behind the UDID-shape
classifier (`native-profiler/platforms/ios.ts`)
The profiler interpolated the caller-supplied `device_id` (`udid`)
directly into shell-interpreted `execSync` template strings:
- `execSync(`xcrun simctl spawn ${udid} launchctl list`)`
- `execSync(`xcrun simctl listapps ${udid} | plutil -convert json -o -
-`)`

`device_id` is declared `z.string()` with no format constraint and
`resolveDevice()` returns it unchanged. **However, this sink is not
directly reachable with a hostile `device_id`** (thanks @hubgan for the
catch): before the iOS platform module runs, `classifyDevice()`
(`utils/device-info.ts`) routes by *shape* against an anchored hex-UDID
regex
`^[0-9A-Fa-f]{8}-[0-9A-Fa-f]{4}-[0-9A-Fa-f]{4}-[0-9A-Fa-f]{4}-[0-9A-Fa-f]{12}$`.
Any `device_id` carrying a shell metacharacter fails that (anchored,
hex-only) match, classifies as **Android**, and takes the argv-based
`adb -s <serial> shell …` path — it never reaches this iOS function.
Conversely, any value that *does* classify as iOS is, by the regex,
metacharacter-free. So on `main` the original `execSync` was **dormant /
defense-in-depth**, gated behind the classifier invariant, not an
exploitable path from a tool parameter.

The conversion is still worthwhile: it removes the code's reliance on
that classifier invariant (if the shape regex were ever loosened, or the
sink reached by a future refactor with a synthetic `platform: "ios"`,
the shell would be back). Fixed by converting both call sites to
`execFileSync` arg-arrays — the `simctl listapps | plutil` pipe is split
into two in-process stages so nothing is shell-interpreted.

This anti-pattern previously had a fix on an old branch that was lost
when the code moved from
`tools/profiler/ios-profiler/ios-profiler-start.ts` to
`native-profiler/platforms/ios.ts`.

### 2. Trace-export hardening — same anti-pattern
(`utils/ios-profiler/export.ts`)
Six `xctrace export` / `xctrace version` commands were built as
`/bin/sh` strings via `execSync`, interpolating the trace-file and
output paths:
- `execSync(`xctrace export --input "${traceFile}" --output "${outPath}"
--xpath '...'`)` (×5, incl. the CPU/leaks/hangs exports and `--toc`
discovery)
- `execSync("xctrace version 2>&1 || true")`

`traceFile` is internally generated today (a timestamped path under the
debug dir, only ever passed in by `native-profiler/platforms/ios.ts`),
so this is **not currently reachable from a tool parameter** — but it is
the identical anti-pattern sitting in the same subsystem, and #1's
interpolation was likewise dormant. Converted the shared
`execSyncWithTimeout` helper to an argv-based `execFileAsyncWithTimeout`
(`execFile`, no shell) and pass every `xctrace` invocation as a discrete
argv array.

### 3. `xcodebuild` version probe — same shell-string shape
(`capture-strategy/select.ts`)
`readActiveXcodeVersion()` ran `execSync("xcodebuild -version")`. The
command is a constant with no interpolated input, so this is **not an
injection** — but it is the same `/bin/sh`-string shape this PR removes
everywhere else in the subsystem. Converted to
`execFileSync("xcodebuild", ["-version"])` so the iOS-profiler subsystem
is uniformly shell-free.

## Audit
Swept the whole tool-server for the same class (caller-supplied tool
parameter → shell-interpreted sink). **No other reachable injection
found:**
- Host commands all use argv `execFile`/`spawn` (`adb`, `xctrace`,
`emulator`, `simctl`) — serial / udid / avdName / bundleId are discrete
argv elements.
- Device-shell commands that interpolate a caller value (`open-url` URL,
`launch-app` / `restart-app` bundleId+component) are escaped through
`shellQuote()` / single-quote escaping.
- The `command -v ${dep}` PATH probes in `check-deps.ts` /
`android-binary.ts` interpolate only the closed `ToolDependency` enum
(`adb | xcrun | emulator`), never a request value.

## Buffering note (post-review)
Splitting the `simctl listapps | plutil` shell pipe into two in-process
`execFileSync` stages moved stdout buffering from the OS to Node, where
each stage inherited Node's 1 MiB default `maxBuffer` — a large
`listapps` could throw `ENOBUFS` (`NATIVE_PROFILER_APP_LIST_FAILED`)
where the pipe streamed unbounded. All three iOS app-enumeration
captures now pass the subsystem's `DEFAULT_EXEC_MAX_BUFFER` (256 MiB,
from `run-with-timeout.ts`), matching how the exporter already avoids
this class.

## Verification
- `npm run build` (tsc) ✓ · `typecheck:tests` ✓ · `prettier --check` ✓ ·
`eslint --max-warnings 0` ✓
- Regression tests assert hostile values are only ever discrete argv
elements, that no `execSync` is used in the subsystem, and that every
app-enumeration stage passes the raised `maxBuffer`. Reverting a fix
back to `execSync` (or dropping a `maxBuffer`) makes its test fail —
confirming each is a real guard.
2026-07-03 16:45:29 +02:00

61 lines
2.3 KiB
TypeScript

import { describe, it, expect, vi } from "vitest";
// Records the options execFile is actually invoked with, so the test can assert
// the wrapper's timeout/maxBuffer guards cannot be silently weakened by a caller.
const rec = vi.hoisted(() => ({
calls: [] as { options?: { timeout?: number; maxBuffer?: number; encoding?: string } }[],
}));
vi.mock("child_process", () => ({
// promisify(execFile) calls this callback form: (file, args, options, cb).
execFile: (_file: string, _args: string[], ...rest: unknown[]) => {
const options = rest.find((r) => typeof r === "object" && r !== null) as
| { timeout?: number; maxBuffer?: number; encoding?: string }
| undefined;
rec.calls.push({ options });
const cb = rest.find((r) => typeof r === "function") as
| ((e: unknown, r: { stdout: string; stderr: string }) => void)
| undefined;
cb?.(null, { stdout: "", stderr: "" });
return undefined;
},
}));
import {
execFileAsyncWithTimeout,
DEFAULT_EXEC_MAX_BUFFER,
DEFAULT_EXEC_TIMEOUT_MS,
} from "../src/utils/ios-profiler/run-with-timeout";
describe("execFileAsyncWithTimeout: safety guards can't be silently weakened", () => {
it("applies the timeout and 256 MiB maxBuffer when the caller passes no options", async () => {
rec.calls.length = 0;
await execFileAsyncWithTimeout("xctrace", ["version"]);
expect(rec.calls).toHaveLength(1);
expect(rec.calls[0].options?.timeout).toBe(DEFAULT_EXEC_TIMEOUT_MS);
expect(rec.calls[0].options?.maxBuffer).toBe(DEFAULT_EXEC_MAX_BUFFER);
});
it("ignores a caller trying to shrink maxBuffer or drop the timeout", async () => {
rec.calls.length = 0;
// A caller that passed these would otherwise re-open the ENOBUFS /
// event-loop-freeze holes the wrapper exists to close.
await execFileAsyncWithTimeout("xctrace", ["version"], {
maxBuffer: 1024,
timeout: 0,
});
expect(rec.calls[0].options?.timeout).toBe(DEFAULT_EXEC_TIMEOUT_MS);
expect(rec.calls[0].options?.maxBuffer).toBe(DEFAULT_EXEC_MAX_BUFFER);
});
it("lets a caller RAISE maxBuffer above the guard (floor, not a cap)", async () => {
rec.calls.length = 0;
const bigger = 512 * 1024 * 1024;
await execFileAsyncWithTimeout("xctrace", ["version"], { maxBuffer: bigger });
expect(rec.calls[0].options?.maxBuffer).toBe(bigger);
});
});