Conversation
Size Report
Startup median (7 runs, lower is better):
|
|
Reviewed at 1392bf3. The funnel closes the injection paths #2026 describes, but relayed Android providers can start refusing commands after a long session. The limrun route is accepted by content, and that record expires. A design question: could the invalid state be unrepresentable instead of detected? If The new Moving HarmonyOS Live evidence is still missing for two changed routes: a relayed Android provider such as limrun, and HarmonyOS Smoke Tests was still running at review time. The diff touches every Android |
|
Reviewed at 1392bf3. One funnel for device-shell argv is the right fix for #2026, but the guard design has a failure mode, and a few pieces are duplicated.
There is no CHANGELOG entry, but users will see changes: HarmonyOS type/fill text is now quoted, an empty word renders as Would a smaller design do? If The live Android run in the description covers open/click/type/fill/screenshot. Could you also run a limrun or provider-scoped Android session, the native perf and screen-recording paths, and the snapshot-helper output-file read whose Smoke Tests is still running, and the diff sits on every Android shell call it makes. There are no conflicts. |
…package entries Review follow-ups on #2611. The content-keyed acceptance window is gone: a minted argv is accepted by identity only, and a relay that rebuilds the array (limrun serial prefix, provider-scope serial strip, the managed server-port rewrite, the host command port copy) carries the identity across with relayDeviceShellArgv, so a module-level constant stays accepted after any number of other mints and a plain copy is refused loudly instead of silently falling back to an evictable path. Executor argv is readonly end to end, so the observation host keeps its readonly contract with no cast, and observation mints its dumpsys argv at read time. runHarmonyShell is exported from the HarmonyOS entry and runAdbShell from Android mechanics, so the recording hosts use the package funnels instead of re-spelling them. deviceShellExecutableOf is the one adb/hdc normalizer for the host command port and the provider scope. The changelog names the refusal, the funnels, and the user-visible quoting changes.
|
Addressed at Eviction. The content-keyed window is gone. Acceptance is identity-only, and every relay that rebuilds the array carries the identity across with Branded type instead of a runtime guard. What blocked it: Funnels on the entries.
CHANGELOG has an Unreleased entry naming the refusal, the funnels, and the three visible quoting changes. Fragment count corrected to 6. Live evidence added to the PR body: record start/stop, perfetto, perf memory/frames, simpleperf (reaches the tool; the emulator refuses perf events), and the |
|
Addressed at
|
|
One more commit, |
… and hdc Closes #2026. adb shell, adb exec-out, and hdc shell join their argv after the subcommand into one string the device sh re-parses, so every unquoted dynamic element was a device-side command injection. Android quoted a handful of sites by hand; HarmonyOS quoted nothing (uitest uiInput text passed user text raw). @agent-device/kernel/device-shell is now the only producer of a device-shell argv: deviceShellArgv quotes every word (identity on the safe charset, so a migrated site is byte-identical unless the word was an injection vector), shellFragment is the one greppable escape hatch for genuine shell syntax, and assertDeviceShellArgv refuses a shell/exec-out argv the funnel did not build at every executor boundary: the handed-out adb executors and providers, runAndroidHostAdb, runHarmonyHdc, and the generic host command port for adb/hdc. Funnels: runAndroidShell/runAndroidExecOut, runAdbShell/runAdbExecOut for injected executors, runHarmonyShell. shellQuote moves from host-kit to the kernel module so the three platform packages share one implementation. Behavior changes: a word with shell metacharacters now arrives single-quoted (the HarmonyOS fix), an empty word renders as '' instead of vanishing, and sh -c script bodies reach sh as one argument.
The provider-scenario fixtures matched a whole command line held in one argv element; the funnel now emits words, and safe paths arrive unquoted.
…package entries Review follow-ups on #2611. The content-keyed acceptance window is gone: a minted argv is accepted by identity only, and a relay that rebuilds the array (limrun serial prefix, provider-scope serial strip, the managed server-port rewrite, the host command port copy) carries the identity across with relayDeviceShellArgv, so a module-level constant stays accepted after any number of other mints and a plain copy is refused loudly instead of silently falling back to an evictable path. Executor argv is readonly end to end, so the observation host keeps its readonly contract with no cast, and observation mints its dumpsys argv at read time. runHarmonyShell is exported from the HarmonyOS entry and runAdbShell from Android mechanics, so the recording hosts use the package funnels instead of re-spelling them. deviceShellExecutableOf is the one adb/hdc normalizer for the host command port and the provider scope. The changelog names the refusal, the funnels, and the user-visible quoting changes.
|
Rebased onto |
f97c77e to
a3a0ea2
Compare
|
|
Reviewed at a3a0ea2. Minting is now by identity in a WeakSet, which has no capacity, so the eviction failure from the last review cannot happen again, and every relay that rebuilds an argv carries the identity. Three problems remain. Two long-lived process routes skip the guard. CHANGELOG.md#L10 tells custom adb executors and providers to use inventory.ts#L243 adds a private Is The live Android runs in the comments were on 1006591, before the rebase and the inventory change. Please add a live Android run on a3a0ea2 that covers device listing, Smoke Tests failed on iOS only: the Settings replay The next step is guarding |
1 participant
Summary
Closes #2026.
adb shell,adb exec-out, andhdc shelljoin their argv into one string the deviceshre-parses, so any unquoted dynamic element was a device-side command injection. Android quoted a few sites by hand; HarmonyOS quoted nothing (uitest uiInput textpassed user text raw).@agent-device/kernel/device-shellis now the only producer of a device-shell argv:deviceShellArgv(subcommand, words, prefix?)quotes every word (identity on the safe charset, so migrated sites are byte-identical unless the word was an injection vector).shellFragment(script)is the one greppable escape hatch for genuine shell syntax (6 sites, each withshellQuote-d interpolations).assertDeviceShellArgvrefuses ashell/exec-outargv the funnel did not build, at every boundary: handed-out adb executors and providers,runAndroidHostAdb,runHarmonyHdc, and the host command port for adb/hdc. Acceptance is by identity only; a relay that rebuilds the array (limrun serial prefix, provider-scope serial strip, managed server-port rewrite, host command port) carries the identity across withrelayDeviceShellArgv. A plain copy is refused loudly, never silently downgraded.runAndroidShell/runAndroidExecOut,runAdbShell/runAdbExecOut(exported from Android mechanics for injected executors),runHarmonyShell(exported from the HarmonyOS entry).shellQuotemoves from host-kit to the kernel module (one implementation). Executor argv isreadonlyend to end; no casts. SDK note (in CHANGELOG): a custom executor annotated(args: string[])must widen toreadonly string[].Behavior changes (in CHANGELOG): metacharacter words arrive single-quoted (the HarmonyOS fix); an empty word renders as
''instead of vanishing;sh -cscript bodies reachshas one argument; a raw['shell', …]from a custom executor is refused withINVALID_ARGS(unguarded-device-shell-argv).152 files touched. Gross diff exceeds the 1,000-line budget by design: #2026's scope decision requires mechanism, guards, and every call site in one PR, since a typed seam existing callers bypass is false confidence.
Validation
Tested at
a3a0ea2163(rebased ontomain; the last two commits are the SDK doc snippet annotation and a size trim).pnpm check:tooling(format, lint, typecheck, layering, depgraph, build, package): pass.pnpm test:unit: 1306 files, 10085 tests pass. Provider-integration scenarios: 66 files pass.pnpm check:fallow: no issues in changed files.pnpm check:affected --run: onf97c77e169three tests hit their 5 s timeout under the bundle's contention (test-file-size-ratchet,ios-record-trace, applerunner-client); all three pass in isolation and the PR touches none of those paths. On1006591feethe only failure wasmutation-model, whoseownership.test.ts"a kernel is owned by tests that reach it indirectly" also fails at the merge-basead9b906140(verified in a clean checkout oforigin/main). The doc-snippet test (client-api-doc-snippets) passes on the final head.pnpm build+clean:daemon, isolated state dir), zerounguarded-device-shell-argvrefusals in the daemon log:open com.android.settings,clickthe search field,type/fillwithwifi; echo pwned $(id) 'x' `uname`landed literally (snapshot shows the exact string, no substitution);screenshotviaexec-outsucceeded.record start/record stop: 57 KB playable MP4 through thescreenrecord … & echo $!andprintf … > tmp && mvfragments.perf memory sampleandperf framessucceed;perf trace start/stop --kind perfettoproduces a 327 KB trace through the background-command fragment.perf cpu profile start --kind simpleperfreaches simpleperf, which the emulator refuses (failed to open perf event file … Permission denied); the fragment parsed correctly, the emulator lacks perf events.sh -c '<script>' name pathshape run over rawadb shellprints the file and removes it (exit 0); the old unquoted shape fails withsh: exit: too many argumentsand leaves the file, so that route was broken before this PR.limrun-runtime.test.tsdrivesam startthrough the real limrun serial-prefix relay intorunAndroidHostAdb, anddevice-shell.test.tssends a module constant through prefix and strip relays after 5,000 other mints. No limrun account is available here for a live run.hdc-shell.test.tsproves refusal and quoting at thehdcboundary).Not covered:
AndroidAdbSpawner(backgroundspawn) is not guarded; the only shell spawn sites (helper session, app-log transport) build their argv from the funnel already.