Files
software-mansion__argent/packages/tool-server/test/native-profiler-ios-capture-strategy-shell-safety.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

47 lines
1.9 KiB
TypeScript

import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
// Records how the capture-strategy selector probes the active Xcode version, so
// the test can assert it never reaches /bin/sh (execSync). Part of keeping the
// whole iOS-profiler subsystem uniformly shell-free.
const rec = vi.hoisted(() => ({
calls: [] as { fn: string; file: string; args: string[] }[],
}));
vi.mock("child_process", () => ({
execFileSync: (file: string, args: string[]) => {
rec.calls.push({ fn: "execFileSync", file, args });
return "Xcode 26.5\nBuild version 17F42\n";
},
execSync: (cmd: string) => {
rec.calls.push({ fn: "execSync", file: String(cmd), args: [] });
throw new Error(`execSync must not be used here (shell risk): ${cmd}`);
},
}));
import { selectIosCaptureStrategy } from "../src/utils/ios-profiler/capture-strategy/select";
describe("iOS capture-strategy: xcodebuild probe is shell-free", () => {
beforeEach(() => {
rec.calls.length = 0;
delete process.env.ARGENT_IOS_CAPTURE;
});
afterEach(() => {
delete process.env.ARGENT_IOS_CAPTURE;
});
it("reads the Xcode version via execFileSync(argv), never execSync", () => {
// With no env override, selection falls through to the xcodebuild version
// probe, exercising the sink. Assert the probe is shell-free regardless of
// which strategy the version policy then picks — decoupled from isDegraded()
// so narrowing that bound later (when Apple fixes the deadlock) can't
// spuriously break this shell-safety test.
const strategy = selectIosCaptureStrategy();
expect(["device", "all-processes"]).toContain(strategy.name);
expect(rec.calls.every((c) => c.fn === "execFileSync")).toBe(true);
const probe = rec.calls.find((c) => c.file === "xcodebuild");
expect(probe).toBeDefined();
expect(probe?.args).toEqual(["-version"]);
});
});