callstack / callstack/agent-device

Typed device-shell execution boundary (ShellSafe) — mechanism + complete migration, one PR

Open
#2,026 1 comment 0 reactions 0 assignees View on GitHub
ready-for-agent
Dominant language
TypeScript
Stars
4.6k
Forks
299
Avg merge
10h 17m
Merged PRs (30d)
515

Description

> **Scope decision.** This lands as **one dedicated PR** — the `ShellSafe` types, the funnels, the guards, the backstop gate, the planted-red tests, **and every production/test call-site migration together**. #2023 stays MCP-focused and must not carry a partially enforced shell interface: a typed seam that existing callers bypass is false confidence, enabling guards before migrating callers risks runtime breakage, and a split leaves two execution models that worsen the ADR 0019 migration.
>
> **WIP branch with the mechanism already built and green in isolation:** `claude/shell-safe-boundary-wip` (commit `eb1449f`). Reuse it as the starting point — do not merge it as-is.

## Threat

`adb shell`, `adb exec-out `, and `hdc shell` join their argv **after the subcommand** into a single string that the *device's* `sh` evaluates. The host never runs a shell (`runSpawnedCommand` spawns with `shell: false`, `src/utils/exec.ts:132,140`), so the injection surface is purely device-side: any unquoted dynamic element in a `['shell', …]` argv is a potential argv/command injection. The audit found this on `input text` and `cmd clipboard set text`; both already quote through `shellQuoteIfNeeded` on `main` (`src/platforms/android/text-input.ts:216`, `device-input-state.ts:312`), so there is no known live exposure — this is about making the invariant structural.

Why not a static gate: the removed `check:shell-argv` inventory allowed `--update` self-approval and its literal-first AST heuristic was blind to indirect argv (`const s = 'shell'; adb([s, 'input', 'text', text])` produced no finding). Verified by reproduction.

## Mechanism (built on the WIP branch)

`packages/kernel/src/shell-safe.ts` — branded `ShellSafe` (mirrors the `src/utils/screenshot-geometry.ts:5-16` idiom), in kernel so `platform-android`, `platform-harmonyos`, and `provider-limrun` can all import it within the ADR 0019 direction:

- `sh.lit(token)` / `sh.lits(...)` — validated bare command words; throws on a metacharacter (fail-closed).
- `sh.arg(value)` — any dynamic value, shell-quoted. **Identity on the safe charset**, so a migrated argv is byte-identical to the pre-migration one and only genuine injection vectors change (pinned by a parity test).
- `sh.num(n)` — numeric argument.
- `sh.raw(fragment)` — narrow escape hatch for an author-written, self-quoted shell fragment; requires a `// shell-safe-approved: ` comment.

Funnels (shell path takes `ShellArgv`, non-shell subcommands keep plain `string[]`):
- `runAndroidShell` / `runAndroidExecOut` — `src/platforms/android/adb.ts`
- `runHarmonyShell` — `src/platforms/harmonyos/hdc.ts`
- `runLimrunAndroidShell` — `packages/provider-limrun/src/android.ts`

Guards reject a raw `['shell'|'exec-out', …]` at the adb executor / hdc / limrun boundaries — a **runtime value check**, so variable-built and indirect argv are caught where a static gate cannot.

Backstop: `scripts/shell-safe` (AST, di-seams-style comment approval) holds every `sh.raw(...)` and `as ShellSafe` cast to an approved, reviewed set — the one place a static check is sound, guarding the escape hatch rather than enumerating values.

Planted-red tests (`src/platforms/android/__tests__/device-shell-boundary.test.ts`): raw literal argv rejected, `exec-out` rejected, **variable-built argv rejected**, resolved-executor path rejected, `@ts-expect-error` proving raw strings do not typecheck, and `sh.arg` quoting an injection vector.

## Commit structure for the PR

1. Android local funnel and callers.
2. HarmonyOS funnel and callers.
3. Limrun/provider funnel and callers.
4. Injected host-port / package callers.
5. Delete every legacy raw-shell route and enable the final completeness gate.

The final gate must prove there is **no bypass**, including variable-built argv and casts. Prefer compatibility with the ADR 0019 destination packages over temporary seams in paths already being retired.

## Known obstacles (found while building the WIP)

- **Injected-executor helpers.** `app-control.ts`, `app-helpers.ts`, `device-input-state.ts` expose `...WithAdb(adb: AndroidAdbExecutor, …)` functions with no `DeviceInfo` in scope, exported to `src/sdk/limrun-runtime-dependencies.ts` and tests. A `runAdbShell(adb, ShellArgv)` helper resolves this without changing the executor contract (preferred over threading `DeviceInfo`).
- **`emulator-lifecycle.ts:157`** calls `runCmd('adb', ['-s', serial, 'shell', 'getprop', …])` directly with only a serial — needs its own funnel path.
- **`packages/platform-*` sites** (`app-state.ts`, `deployment/*`, `inventory.ts`, `logs/runtime.ts`, `network/runtime.ts`, `readiness/runtime.ts`) go through the injected host capability (`host.run` / `runAdb`), a second boundary that needs the same typing on the port's argv.
- **Per-call `options.adb` injection** is not honored by the current funnels; the daemon establishes provider scope (`src/daemon/request-platform-providers.ts:170`) so production is equivalent, but tests injecting via `options.adb` must move to `withAndroidAdbProvider`.
- **~30 test files** mock the funnels and assert the legacy `['shell', …]` shape; they migrate with their source (`noUnusedParameters` also bites when an `options.adb` read disappears).
- **`perf-native-process.ts`** interpolated `tool` unquoted into a fragment (typed union, not attacker-controlled) — the WIP shellQuote-escapes it; keep that fix.

Site checklist: `git show 9cc6642:scripts/shell-argv/inventory.json` (~188 dynamic elements across ~40 files).

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.