From 61e688070f48016b6d178cd04c02fcc2f33e19ad Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Sat, 17 Oct 2026 04:30:50 +0800 Subject: [PATCH] fix(adb): refuse transfers when destination existence is unknown Own ADB command failure facts once and interpret only the clean empty test false result as absence. Preserve unknown probe output and reuse parsed facts for package refusals. Related: #4574 --- docs/adb-device-analysis.md | 6 +- src/adb/AdbCommand.test.ts | 112 ++++++++++++++ src/adb/AdbCommand.ts | 244 ++++++++++++++++++++++++++++++ src/adb/AdbProvider.ts | 176 ++------------------- tests/boundary/cli/adbCli.test.ts | 69 +++++++++ 5 files changed, 440 insertions(+), 167 deletions(-) create mode 100644 src/adb/AdbCommand.test.ts create mode 100644 src/adb/AdbCommand.ts diff --git a/docs/adb-device-analysis.md b/docs/adb-device-analysis.md index b18e7b4fbe..7e10ac33d6 100644 --- a/docs/adb-device-analysis.md +++ b/docs/adb-device-analysis.md @@ -153,8 +153,10 @@ and during shutdown. - **Mutations are explicit.** `push_adb_file`, `install_adb_package`, and `uninstall_adb_package` declare `mutatesTarget` and never run implicitly: pushes check for existing device files before transfer and refuse them - unless `overwrite` is set. The check is not atomic, so a different process - can create the path before `adb push` writes it. When available, the device's + unless `overwrite` is set. A failed or unexpected existence probe stops + transfer even with `overwrite`; only a clean `test -e` exit of 1 with no + output is interpreted as absence. The check is not atomic, so a different + process can create the path before `adb push` writes it. When available, the device's `sha256sum` verifies the transfer; installs preserve the device's `Failure [...]` reason verbatim. - **Producer identity stays attached.** Every result embeds the producing diff --git a/src/adb/AdbCommand.test.ts b/src/adb/AdbCommand.test.ts new file mode 100644 index 0000000000..63bb5ab0aa --- /dev/null +++ b/src/adb/AdbCommand.test.ts @@ -0,0 +1,112 @@ +import { expect, it, vi } from "vitest"; +import { execFileBytes } from "../process/ExecFileOutput.js"; +import { probeAdbPathExistence } from "./AdbCommand.js"; + +vi.mock("../process/ExecFileOutput.js", async (importOriginal) => ({ + ...(await importOriginal()), + execFileBytes: vi.fn(), +})); + +const probe = (signal?: AbortSignal) => + probeAdbPathExistence( + "push_adb_file", + "device", + "/device/path", + "/host/adb", + { + timeoutMs: 100, + maxOutputBytes: 1024, + }, + signal, + ); + +it("recognizes only the completed empty false result as absence", async () => { + vi.mocked(execFileBytes).mockRejectedValueOnce( + Object.assign(new Error("test false"), { + code: 1, + stdout: "", + stderr: "", + signal: null, + killed: false, + }), + ); + await expect(probe()).resolves.toEqual({ ok: true, value: false }); +}); + +it.each([ + { code: 2 }, + { code: "ENOENT" }, + { signal: "SIGTERM" }, + { collectionFailure: true }, + { stdout: "unexpected" }, + { stderr: " " }, +])("keeps nonstandard false completion unknown: %j", async (extra) => { + vi.mocked(execFileBytes).mockRejectedValueOnce( + Object.assign(new Error("probe failed"), { + code: 1, + stdout: "", + stderr: "", + signal: null, + killed: false, + ...extra, + }), + ); + const result = await probe(); + expect(result).toMatchObject({ + ok: false, + error: { + _tag: "AnalysisProtocolError", + diagnostics: { destination_existence: "unknown" }, + }, + }); +}); + +it.each([ + [{ killed: true }, "AnalysisTimeoutError"], + [{ outputTruncated: true }, "AnalysisResourceConstraintError"], + [{ code: "ABORT_ERR" }, "AnalysisCancelledError"], +] as const)( + "preserves a stopped or incomplete probe: %j", + async (extra, tag) => { + vi.mocked(execFileBytes).mockRejectedValueOnce( + Object.assign(new Error("probe failed"), { + code: 1, + stdout: "", + stderr: "", + signal: null, + killed: false, + ...extra, + }), + ); + await expect(probe()).resolves.toMatchObject({ + ok: false, + error: { _tag: tag }, + }); + }, +); + +it("recognizes an empty successful probe as existence", async () => { + vi.mocked(execFileBytes).mockResolvedValueOnce({ + stdout: Buffer.alloc(0), + stderr: Buffer.alloc(0), + }); + await expect(probe()).resolves.toEqual({ ok: true, value: true }); +}); + +it.each(["stdout", "stderr"] as const)( + "retains unexpected successful %s as unknown", + async (stream) => { + vi.mocked(execFileBytes).mockResolvedValueOnce({ + stdout: Buffer.alloc(0), + stderr: Buffer.alloc(0), + [stream]: Buffer.from("unexpected"), + }); + await expect(probe()).resolves.toMatchObject({ + ok: false, + error: { + _tag: "AnalysisProtocolError", + capturedOutput: { [stream]: "unexpected", exit_code: 0 }, + }, + }); + }, +); diff --git a/src/adb/AdbCommand.ts b/src/adb/AdbCommand.ts new file mode 100644 index 0000000000..5ab13e4b57 --- /dev/null +++ b/src/adb/AdbCommand.ts @@ -0,0 +1,244 @@ +import { + AnalysisCancelledError, + AnalysisProtocolError, + AnalysisResourceConstraintError, + AnalysisTimeoutError, + AnalysisUnsupportedTargetError, +} from "../domain/analysisErrorCore.js"; +import type { AnalysisError } from "../domain/analysisErrorBase.js"; +import { err, ok, type Result } from "../domain/result.js"; +import { + execFileBytes, + execFileOutputFailure, + type ExecFileOutputFailure, +} from "../process/ExecFileOutput.js"; +import { packageChangeFailureReason } from "./AdbShellOutput.js"; + +const STDERR_EXCERPT_BYTES = 512; + +/** Parsed command failure owned by this adapter, separate from its public tag. */ +class AdbProtocolError extends AnalysisProtocolError { + constructor( + message: string, + readonly commandFailure: ExecFileOutputFailure, + cause: unknown, + ) { + super(message, { cause }); + } +} + +/** adb joins shell operands into a remote POSIX command; quote at that boundary. */ +const quoteShellOperand = (value: string): string => + /^[A-Za-z0-9_./:=@+,-]+$/u.test(value) + ? value + : "'" + value.replaceAll("'", "'\\''") + "'"; + +export const adbArguments = ( + serial: string | undefined, + arguments_: readonly string[], +): readonly string[] => { + const operands = + arguments_[0] === "shell" + ? ["shell", ...arguments_.slice(1).map(quoteShellOperand)] + : [...arguments_]; + return serial === undefined ? operands : ["-s", serial, ...operands]; +}; + +export interface AdbRunLimits { + readonly timeoutMs: number; + readonly maxOutputBytes: number; +} + +/** Run one bounded adb invocation and keep its exact failure shape. */ +export const runAdbCapture = async ( + operation: string, + targetPath: string, + binary: string, + arguments_: readonly string[], + limits: AdbRunLimits, + signal: AbortSignal | undefined, +): Promise> => { + if (signal?.aborted === true) + return err(new AnalysisCancelledError(operation, { cause: signal.reason })); + try { + return ok( + await execFileBytes(binary, arguments_, { + timeout: limits.timeoutMs, + maxBuffer: limits.maxOutputBytes, + stopSignal: "SIGTERM", + ...(signal === undefined ? {} : { signal }), + }), + ); + } catch (cause) { + const failure = execFileOutputFailure(cause); + if (signal?.aborted || failure?.code === "ABORT_ERR") + return err(new AnalysisCancelledError(operation, { cause })); + if (failure === undefined) + return err( + new AnalysisProtocolError(`adb failed for ${operation}`, { cause }), + ); + if (failure.outputTruncated) + return err( + new AnalysisResourceConstraintError( + operation, + "memory", + `adb output exceeded the ${String(limits.maxOutputBytes)}-byte capture budget`, + { max_output_bytes: limits.maxOutputBytes }, + { cause }, + ), + ); + const excerpt = failure.stderr.trim().slice(0, STDERR_EXCERPT_BYTES); + if (/unauthorized/iu.test(excerpt)) + return err( + new AnalysisUnsupportedTargetError(operation, targetPath, excerpt, { + cause, + }), + ); + if ( + /device[^\n]*not found|device offline|still authorizing|more than one device/iu.test( + excerpt, + ) + ) + return err( + new AnalysisUnsupportedTargetError(operation, targetPath, excerpt, { + cause, + }), + ); + if ( + /No such file or directory|Permission denied|read: Permission denied/iu.test( + excerpt, + ) + ) + return err( + new AnalysisUnsupportedTargetError(operation, targetPath, excerpt, { + cause, + }), + ); + if (failure.killed) + return err( + new AnalysisTimeoutError(operation, limits.timeoutMs, { cause }), + ); + return err( + new AdbProtocolError( + excerpt === "" + ? `adb exited with code ${String(failure.code)} for ${operation}` + : excerpt, + failure, + cause, + ), + ); + } +}; + +/** Decode textual adb protocols after the shared byte capture and error boundary. */ +export const runAdb = async ( + ...arguments_: Parameters +): Promise< + Result<{ stdout: string; stderr: string; stdoutBytes: number }, AnalysisError> +> => { + const captured = await runAdbCapture(...arguments_); + if (!captured.ok) return captured; + return ok({ + stdout: captured.value.stdout.toString("utf8"), + stderr: captured.value.stderr.toString("utf8"), + // Byte count of the captured output before UTF-8 decoding, so malformed + // sequences replaced by U+FFFD still report their true captured size. + stdoutBytes: captured.value.stdout.byteLength, + }); +}; + +/** + * adb exits nonzero when the device refuses a package change, printing the + * `Failure [...]` verdict to stderr (install) or stdout (uninstall). Read the + * refusal from that failed run so it is not reported as unreadable output. + */ +export const packageChangeRefusal = ( + operation: string, + serial: string, + error: AnalysisError, +): Result | undefined => { + if (!(error instanceof AdbProtocolError)) return undefined; + const failure = error.commandFailure; + const reason = packageChangeFailureReason( + `${failure.stdout}\n${failure.stderr}`, + ); + return reason === null + ? undefined + : err( + new AnalysisUnsupportedTargetError(operation, serial, reason, { + cause: error, + }), + ); +}; + +/** Only the clean false result of test -e establishes absence. */ +export const probeAdbPathExistence = async ( + operation: string, + serial: string, + path: string, + binary: string, + limits: AdbRunLimits, + signal: AbortSignal | undefined, +): Promise> => { + const probed = await runAdbCapture( + operation, + path, + binary, + adbArguments(serial, ["shell", "test", "-e", path]), + limits, + signal, + ); + if (probed.ok) { + if (probed.value.stdout.length === 0 && probed.value.stderr.length === 0) + return ok(true); + return err( + new AnalysisProtocolError( + "The ADB path existence probe returned unexpected output; destination existence is unknown", + { + diagnostics: { destination_existence: "unknown" }, + capturedOutput: { + stdout: probed.value.stdout.toString("utf8"), + stderr: probed.value.stderr.toString("utf8"), + stdout_bytes: probed.value.stdout.length, + stderr_bytes: probed.value.stderr.length, + exit_code: 0, + signal: null, + truncated: false, + }, + }, + ), + ); + } + const error = probed.error; + if (!(error instanceof AdbProtocolError)) return probed; + const failure = error.commandFailure; + if ( + failure.code === 1 && + failure.signal === null && + !failure.killed && + !failure.outputTruncated && + failure.collectionFailure !== true && + failure.stdout === "" && + failure.stderr === "" + ) + return ok(false); + return err( + new AnalysisProtocolError(error.message, { + cause: error.cause, + diagnostics: { destination_existence: "unknown" }, + capturedOutput: { + stdout: failure.stdout, + stderr: failure.stderr, + ...(failure.stdoutBytes === undefined + ? {} + : { stdout_bytes: failure.stdoutBytes }), + ...(failure.stderrBytes === undefined + ? {} + : { stderr_bytes: failure.stderrBytes }), + exit_code: typeof failure.code === "number" ? failure.code : null, + signal: failure.signal, + truncated: failure.outputTruncated, + }, + }), + ); +}; diff --git a/src/adb/AdbProvider.ts b/src/adb/AdbProvider.ts index db696e8994..60d7b3672e 100644 --- a/src/adb/AdbProvider.ts +++ b/src/adb/AdbProvider.ts @@ -70,9 +70,13 @@ import { import { ProviderCleanupError } from "../domain/providerCleanupError.js"; import { PrivateRuntimeRootUnavailableError } from "../process/PrivateRuntimeRootUnavailableError.js"; import { - execFileBytes, - execFileOutputFailure, -} from "../process/ExecFileOutput.js"; + adbArguments, + runAdb, + runAdbCapture, + packageChangeRefusal, + probeAdbPathExistence, + type AdbRunLimits, +} from "./AdbCommand.js"; import { AdbConfigurationFailure, adbConfigurationAvailability, @@ -102,7 +106,6 @@ import { parseBugreportCopiedPath, parseDumpsysPackageOutput, parseFeatureListOutput, - packageChangeFailureReason, parsePackageChangeOutput, parseProcessListOutput, parsePngDimensions, @@ -144,7 +147,6 @@ const SCREEN_TIMEOUT_MS = 60_000; const SCREEN_MAX_OUTPUT_BYTES = 32 * 1024 * 1024; const BUGREPORT_TIMEOUT_MS = 600_000; const BUGREPORT_MAX_OUTPUT_BYTES = 1024 * 1024; -const STDERR_EXCERPT_BYTES = 512; const UNPARSED_LINES_REPORTED = 10; /** Carry the exact digest owner through caller-specific partial-result mapping. */ @@ -364,150 +366,6 @@ const syncStagedAdbFile = async ( if (primary !== undefined) throw primary; }; -/** adb joins shell operands into a remote POSIX command; quote at that boundary. */ -const quoteShellOperand = (value: string): string => - /^[A-Za-z0-9_./:=@+,-]+$/u.test(value) - ? value - : "'" + value.replaceAll("'", "'\\''") + "'"; - -const adbArguments = ( - serial: string | undefined, - arguments_: readonly string[], -): readonly string[] => { - const operands = - arguments_[0] === "shell" - ? ["shell", ...arguments_.slice(1).map(quoteShellOperand)] - : [...arguments_]; - return serial === undefined ? operands : ["-s", serial, ...operands]; -}; - -interface AdbRunLimits { - readonly timeoutMs: number; - readonly maxOutputBytes: number; -} - -/** Run one bounded adb invocation and keep its exact failure shape. */ -const runAdbCapture = async ( - operation: string, - targetPath: string, - binary: string, - arguments_: readonly string[], - limits: AdbRunLimits, - signal: AbortSignal | undefined, -): Promise> => { - if (signal?.aborted === true) - return err(new AnalysisCancelledError(operation, { cause: signal.reason })); - try { - return ok( - await execFileBytes(binary, arguments_, { - timeout: limits.timeoutMs, - maxBuffer: limits.maxOutputBytes, - stopSignal: "SIGTERM", - ...(signal === undefined ? {} : { signal }), - }), - ); - } catch (cause) { - if (signal?.aborted || execFileOutputFailure(cause)?.code === "ABORT_ERR") - return err(new AnalysisCancelledError(operation, { cause })); - const failure = execFileOutputFailure(cause); - if (failure === undefined) - return err( - new AnalysisProtocolError(`adb failed for ${operation}`, { cause }), - ); - if (failure.outputTruncated) - return err( - new AnalysisResourceConstraintError( - operation, - "memory", - `adb output exceeded the ${String(limits.maxOutputBytes)}-byte capture budget`, - { max_output_bytes: limits.maxOutputBytes }, - { cause }, - ), - ); - const excerpt = failure.stderr.trim().slice(0, STDERR_EXCERPT_BYTES); - if (/unauthorized/iu.test(excerpt)) - return err( - new AnalysisUnsupportedTargetError(operation, targetPath, excerpt, { - cause, - }), - ); - if ( - /device[^\n]*not found|device offline|still authorizing|more than one device/iu.test( - excerpt, - ) - ) - return err( - new AnalysisUnsupportedTargetError(operation, targetPath, excerpt, { - cause, - }), - ); - if ( - /No such file or directory|Permission denied|read: Permission denied/iu.test( - excerpt, - ) - ) - return err( - new AnalysisUnsupportedTargetError(operation, targetPath, excerpt, { - cause, - }), - ); - if (failure.killed) - return err( - new AnalysisTimeoutError(operation, limits.timeoutMs, { cause }), - ); - return err( - new AnalysisProtocolError( - excerpt === "" - ? `adb exited with code ${String(failure.code)} for ${operation}` - : excerpt, - { cause }, - ), - ); - } -}; - -/** Decode textual adb protocols after the shared byte capture and error boundary. */ -const runAdb = async ( - ...arguments_: Parameters -): Promise< - Result<{ stdout: string; stderr: string; stdoutBytes: number }, AnalysisError> -> => { - const captured = await runAdbCapture(...arguments_); - if (!captured.ok) return captured; - return ok({ - stdout: captured.value.stdout.toString("utf8"), - stderr: captured.value.stderr.toString("utf8"), - // Byte count of the captured output before UTF-8 decoding, so malformed - // sequences replaced by U+FFFD still report their true captured size. - stdoutBytes: captured.value.stdout.byteLength, - }); -}; - -/** - * adb exits nonzero when the device refuses a package change, printing the - * `Failure [...]` verdict to stderr (install) or stdout (uninstall). Read the - * refusal from that failed run so it is not reported as unreadable output. - */ -const packageChangeRefusal = ( - operation: string, - serial: string, - error: AnalysisError, -): Result | undefined => { - if (!(error instanceof AnalysisProtocolError)) return undefined; - const failure = execFileOutputFailure(error.cause); - if (failure === undefined) return undefined; - const reason = packageChangeFailureReason( - `${failure.stdout}\n${failure.stderr}`, - ); - return reason === null - ? undefined - : err( - new AnalysisUnsupportedTargetError(operation, serial, reason, { - cause: error, - }), - ); -}; - const clientIdentityResult = ( selection: AdbBinarySelection, identity: AdbClientIdentity, @@ -2535,28 +2393,16 @@ export class AdbProvider { operation, signal, ); - // `test -e` exits nonzero with no adb diagnosis when the path is absent. - // Any other probe failure leaves existence unknown, so it is reported as - // such rather than treated as absence or followed by a push. - const existsProbe = await runAdb( + const existsProbe = await probeAdbPathExistence( operation, + input.serial, input.device_path, selection.binary, - adbArguments(input.serial, [ - "shell", - "test", - "-e", - input.device_path, - ]), { timeoutMs: SETTINGS_TIMEOUT_MS, maxOutputBytes: 1024 }, signal, ); - if ( - !existsProbe.ok && - !(existsProbe.error instanceof AnalysisProtocolError) - ) - return existsProbe; - const deviceExists = existsProbe.ok; + if (!existsProbe.ok) return existsProbe; + const deviceExists = existsProbe.value; if (deviceExists && !input.overwrite) return err( new AnalysisOutputError( diff --git a/tests/boundary/cli/adbCli.test.ts b/tests/boundary/cli/adbCli.test.ts index 69925565f7..e41aeb7e84 100644 --- a/tests/boundary/cli/adbCli.test.ts +++ b/tests/boundary/cli/adbCli.test.ts @@ -5,6 +5,10 @@ import { expect, it, onTestFinished } from "vitest"; import { z } from "zod"; import { toolContract } from "../../../src/contracts/toolContracts.js"; +import { + execFileOutput, + execFileOutputFailure, +} from "../../../src/process/ExecFileOutput.js"; import { expectConfigurationRefusal, serveCli as serve, @@ -14,6 +18,71 @@ import { removeTestWorkspace, } from "../../support/workspace/workspaceFixture.js"; +it.skipIf(process.platform === "win32").each([ + { exit: 1, stdout: "", stderr: "adb: error: closed", overwrite: false }, + { exit: 1, stdout: "", stderr: "adb: error: closed", overwrite: true }, + { exit: 1, stdout: "unexpected probe output", stderr: "", overwrite: false }, + { exit: 2, stdout: "", stderr: "", overwrite: false }, +])("refuses an ambiguous destination probe before push: %j", async (probe) => { + const workspace = await createTestWorkspace("rea-adb-probe-cli-"); + onTestFinished(() => removeTestWorkspace(workspace.root)); + const source = await workspace.write("payload.bin", "replacement"); + await workspace.write("device-file", "original"); + const adb = await workspace.write( + "adb", + `#!/bin/sh +root=$(CDPATH= cd -- "$(dirname -- "$0")" && pwd) +printf '%s\\n' "$3" >> "$root/calls" +if [ "$1" = version ]; then + printf 'Android Debug Bridge version 1.0.41\\nVersion 34.0.5-cli\\nInstalled as %s\\n' "$0" + exit 0 +fi +if [ "$3" = shell ] && [ "$4" = test ]; then + printf '%s' '${probe.stdout}' + printf '%s' '${probe.stderr}' >&2 + exit ${String(probe.exit)} +fi +if [ "$3" = push ]; then cp "$4" "$root/device-file"; exit 0; fi +exit 1 +`, + ); + await chmod(adb, 0o755); + const result = await execFileOutput( + process.execPath, + [ + "scripts/rea.mjs", + "push-adb-file", + "device", + source, + "/data/local/tmp/payload.bin", + ...(probe.overwrite ? ["--overwrite"] : []), + "--json", + ], + { + env: { ...process.env, REA_ADB_PATH: adb }, + timeout: 20_000, + stopSignal: "SIGTERM", + maxBuffer: 1024 * 1024, + }, + ).then( + (output) => ({ exitCode: 0, stdout: output.stdout }), + (cause: unknown) => { + const failure = execFileOutputFailure(cause); + if (failure === undefined) throw cause; + return { exitCode: failure.code, stdout: failure.stdout }; + }, + ); + expect(result.exitCode).toBe(1); + expect(JSON.parse(result.stdout)).toMatchObject({ + code: "unreadable_output", + }); + if (probe.stderr !== "") expect(result.stdout).toContain(probe.stderr); + expect(await readFile(workspace.path("device-file"), "utf8")).toBe( + "original", + ); + expect(await readFile(workspace.path("calls"), "utf8")).toBe("\nshell\n"); +}); + it.each([ [ ["inspect-adb-client", "--json"],