diff --git a/docs/adb-device-analysis.md b/docs/adb-device-analysis.md index 59ab98017..416e78161 100644 --- a/docs/adb-device-analysis.md +++ b/docs/adb-device-analysis.md @@ -109,12 +109,12 @@ existing analysis families directly, for example - **Device paths are not host paths.** `base_apk_device_path` values live on the device filesystem. Only pulls and captures produce host-local paths, each paired with byte size and SHA-256 digest. A pulled file's host name is - the device path's last segment, accepted only when every host can store it: - a `pull_adb_file` whose device path ends in a separator, `.`/`..`, a - control or reserved character, a Windows reserved device name, a trailing - dot or space, or a `:` alternate-data-stream marker is refused as - `invalid_request` before any `adb pull` runs, and REA never renames a - device-reported file. + the device path's final POSIX segment. REA preserves the original device + path and filename; a missing final segment or `.`/`..` is refused before + output creation. Windows hosts follow [Windows filename rules](https://learn.microsoft.com/en-us/windows/win32/fileio/naming-a-file), + while POSIX hosts retain valid POSIX filenames such as names with spaces or + colons. A package pull checks reported names against the destination + filesystem before pulling, and refuses names that would share a local path. - **Splits are part of the answer.** `pm path` enumerates the base APK and every installed split; pulls cover the whole set. System packages whose APKs keep other names report the `unknown` role rather than guessing. diff --git a/src/adb/AdbDeviceOutput.test.ts b/src/adb/AdbDeviceOutput.test.ts index b85061cae..fe11c2a26 100644 --- a/src/adb/AdbDeviceOutput.test.ts +++ b/src/adb/AdbDeviceOutput.test.ts @@ -239,4 +239,35 @@ describe("parsePmPathOutput", () => { role: "unknown", }); }); + + it("preserves whitespace and the exact POSIX device path", () => { + const devicePath = "/data/app/My App/résumé:final\\draft*.apk"; + const { paths, rejectedPaths, unparsedLines } = parsePmPathOutput( + `package:${devicePath}\r\n`, + ); + expect(paths).toEqual([ + { + device_path: devicePath, + file_name: "résumé:final\\draft*.apk", + role: "unknown", + }, + ]); + expect(rejectedPaths).toEqual([]); + expect(unparsedLines).toEqual([]); + }); + + it("rejects an empty final component without normalizing the source", () => { + const devicePath = "/data/app/pkg/base.apk/"; + const parsed = parsePmPathOutput(`package:${devicePath}\n`); + expect(parsed.paths).toEqual([]); + expect(parsed.rejectedPaths).toEqual([ + { device_path: devicePath, reason: "empty_file_name" }, + ]); + }); + + it("retains malformed lines including surrounding whitespace", () => { + expect( + parsePmPathOutput(" package:/data/app/pkg/base.apk \n").unparsedLines, + ).toEqual([" package:/data/app/pkg/base.apk "]); + }); }); diff --git a/src/adb/AdbDeviceOutput.ts b/src/adb/AdbDeviceOutput.ts index 44bbb53ba..3380de608 100644 --- a/src/adb/AdbDeviceOutput.ts +++ b/src/adb/AdbDeviceOutput.ts @@ -6,6 +6,11 @@ * their evidence basis. */ +import { + adbHostFileName, + type AdbHostFileNameReason, +} from "./AdbHostFileName.js"; + /** One device exactly as `adb devices -l` reports it. */ export interface AdbDeviceEntry { readonly serial: string; @@ -205,11 +210,6 @@ export interface AdbPackagePathEntry { readonly role: "base" | "split" | "unknown"; } -const fileNameOf = (devicePath: string): string => { - const parts = devicePath.split(/\//u); - return parts[parts.length - 1] ?? devicePath; -}; - /** * Parse `pm path PACKAGE` lines. The base/split role is derived from the file * name (`base.apk`, `split_*.apk`), which is the only signal the output @@ -219,20 +219,35 @@ export const parsePmPathOutput = ( stdout: string, ): { paths: readonly AdbPackagePathEntry[]; + rejectedPaths: readonly { + device_path: string; + reason: AdbHostFileNameReason; + }[]; unparsedLines: readonly string[]; } => { const paths: AdbPackagePathEntry[] = []; + const rejectedPaths: { + device_path: string; + reason: AdbHostFileNameReason; + }[] = []; const unparsedLines: string[] = []; for (const rawLine of stdout.split(/\r?\n/u)) { - const line = rawLine.trim(); - if (line === "") continue; - const match = /^package:(\S+)$/u.exec(line); + if (rawLine === "") continue; + const match = /^package:(.*)$/u.exec(rawLine); if (match === null) { - unparsedLines.push(line); + unparsedLines.push(rawLine); continue; } const devicePath = match[1]!; - const fileName = fileNameOf(devicePath); + const validated = adbHostFileName(devicePath); + if (!validated.ok) { + rejectedPaths.push({ + device_path: devicePath, + reason: validated.reason, + }); + continue; + } + const fileName = validated.fileName; paths.push({ device_path: devicePath, file_name: fileName, @@ -244,5 +259,5 @@ export const parsePmPathOutput = ( : "unknown", }); } - return { paths, unparsedLines }; + return { paths, rejectedPaths, unparsedLines }; }; diff --git a/src/adb/AdbHostFileName.test.ts b/src/adb/AdbHostFileName.test.ts index 74a87bf4b..973fcafbb 100644 --- a/src/adb/AdbHostFileName.test.ts +++ b/src/adb/AdbHostFileName.test.ts @@ -14,28 +14,50 @@ describe("adbHostFileName", () => { ], ["/data/local/tmp/résumé.xml", "résumé.xml"], ["/data/local/tmp/a-b_c.d", "a-b_c.d"], - ["/data/local/tmp/", "tmp"], - ])("accepts the portable device path %s", (devicePath, fileName) => { + ["/data/local/tmp/a\\b", "a\\b"], + ["/data/local/tmp/report:2024", "report:2024"], + ["/data/local/tmp/a*b.txt", "a*b.txt"], + ["/data/local/tmp/re\u007fport", "re\u007fport"], + ])("accepts the POSIX device path %s", (devicePath, fileName) => { expect(adbHostFileName(devicePath)).toEqual({ ok: true, fileName }); }); it.each([ ["/", "empty_file_name"], + ["data/local/tmp/file", "not_absolute_path"], + ["/data/local/tmp/", "empty_file_name"], ["/data/local/tmp/..", "dot_segment"], ["/data/local/tmp/.", "dot_segment"], - ["/data/local/tmp/a\\b", "path_separator"], - ["/data/local/tmp/re\u007fport", "control_character"], - ["/data/local/tmp/ta\tb", "control_character"], + ["/data\0/local/tmp/file", "unsupported_character"], + ["/data/local/tmp/a\0b", "unsupported_character"], + ] as const)("refuses %s as %s", (devicePath, reason) => { + expect(adbHostFileName(devicePath)).toEqual({ ok: false, reason }); + }); + + it.each([ + ["/data/local/tmp/a\\b", "unsupported_character"], + ["/data/local/tmp/ta\tb", "unsupported_character"], ["/data/local/tmp/CON", "reserved_name"], ["/data/local/tmp/nul.txt", "reserved_name"], ["/data/local/tmp/com1", "reserved_name"], ["/data/local/tmp/LPT9", "reserved_name"], - ["/data/local/tmp/report:2024", "alternate_data_stream"], + ["/data/local/tmp/COM¹.txt", "reserved_name"], + ["/data/local/tmp/lpt³", "reserved_name"], + ["/data/local/tmp/report:2024", "unsupported_character"], ["/data/local/tmp/report.", "trailing_dot_or_space"], ["/data/local/tmp/report ", "trailing_dot_or_space"], ["/data/local/tmp/a*b.txt", "unsupported_character"], ["/data/local/tmp/a?b.txt", "unsupported_character"], - ] as const)("refuses %s as %s", (devicePath, reason) => { - expect(adbHostFileName(devicePath)).toEqual({ ok: false, reason }); + ] as const)("rejects %s on Windows as %s", (devicePath, reason) => { + expect(adbHostFileName(devicePath, "win32")).toEqual({ ok: false, reason }); + }); + + it("does not impose non-Windows control restrictions on Windows filenames", () => { + expect(adbHostFileName("/data/local/tmp/name\u007f.txt", "win32").ok).toBe( + true, + ); + expect(adbHostFileName("/data/local/tmp/name\u0085.txt", "win32").ok).toBe( + true, + ); }); }); diff --git a/src/adb/AdbHostFileName.ts b/src/adb/AdbHostFileName.ts index ea4d526f2..4ffd7cec1 100644 --- a/src/adb/AdbHostFileName.ts +++ b/src/adb/AdbHostFileName.ts @@ -1,61 +1,53 @@ -import { posix } from "node:path"; - /** Why a device-reported path cannot be used as a host file name. */ export type AdbHostFileNameReason = | "empty_file_name" + | "not_absolute_path" | "dot_segment" - | "path_separator" - | "control_character" | "unsupported_character" | "reserved_name" - | "trailing_dot_or_space" - | "alternate_data_stream"; + | "trailing_dot_or_space"; -/** A portable host file name, or the exact reason the device name was refused. */ export type AdbHostFileNameResult = | { readonly ok: true; readonly fileName: string } | { readonly ok: false; readonly reason: AdbHostFileNameReason }; -/** - * Win32 resolves these basenames to a device regardless of case and of any - * extension (`NUL`, `nul.txt`), so a portable host file name must avoid them on - * every host. `(?:\.|$)` keeps ordinary names such as `console.txt` admissible. - */ const WINDOWS_RESERVED_BASENAME = - /^(?:con|prn|aux|nul|com[1-9]|lpt[1-9])(?:\.|$)/iu; - -/** Characters that are not portable in a host file name on Windows or POSIX. */ -const UNSUPPORTED_CHARACTER = /[*?"<>|]/u; - -/** Control characters (`Cc`), including C0 controls and DEL. */ -const CONTROL_CHARACTER = /\p{Cc}/u; + /^(?:con|prn|aux|nul|com[1-9¹²³]|lpt[1-9¹²³])(?:\.|$)/iu; /** - * Derive a portable host file name from a device-reported path. - * - * The device path is a POSIX path on the device filesystem, so its last - * `/`-separated segment is taken with POSIX semantics rather than the host - * `basename` (which would vary between POSIX and Windows hosts). A name is - * accepted only when every host can store it and address it back unambiguously: - * no separator, control, or reserved-device characters, no trailing dot or - * space, no `:` alternate-data-stream marker, and no `.`/`..` segment. + * Derive the final component of a device POSIX path without normalizing the + * source. A trailing slash has no filename and must not silently become the + * preceding component as `basename` would make it. */ -export const adbHostFileName = (devicePath: string): AdbHostFileNameResult => { - const fileName = posix.basename(devicePath); - if (fileName === "") return { ok: false, reason: "empty_file_name" }; +export const adbHostFileName = ( + devicePath: string, + platform: NodeJS.Platform = process.platform, +): AdbHostFileNameResult => { + if (devicePath === "" || devicePath.endsWith("/")) + return { ok: false, reason: "empty_file_name" }; + if (devicePath.includes("\0")) + return { ok: false, reason: "unsupported_character" }; + if (!devicePath.startsWith("/")) + return { ok: false, reason: "not_absolute_path" }; + const fileName = devicePath.slice(devicePath.lastIndexOf("/") + 1); if (fileName === "." || fileName === "..") return { ok: false, reason: "dot_segment" }; - if (fileName.includes("\\") || fileName.includes("/")) - return { ok: false, reason: "path_separator" }; - if (CONTROL_CHARACTER.test(fileName)) - return { ok: false, reason: "control_character" }; - if (fileName.includes(":")) - return { ok: false, reason: "alternate_data_stream" }; - if (UNSUPPORTED_CHARACTER.test(fileName)) - return { ok: false, reason: "unsupported_character" }; - if (WINDOWS_RESERVED_BASENAME.test(fileName)) - return { ok: false, reason: "reserved_name" }; - if (/[. ]$/u.test(fileName)) - return { ok: false, reason: "trailing_dot_or_space" }; + if (platform === "win32") { + if (/[\\:*?"<>|]/u.test(fileName) || hasWindowsControl(fileName)) + return { ok: false, reason: "unsupported_character" }; + if (WINDOWS_RESERVED_BASENAME.test(fileName)) + return { ok: false, reason: "reserved_name" }; + if (/[. ]$/u.test(fileName)) + return { ok: false, reason: "trailing_dot_or_space" }; + } + return { ok: true, fileName }; }; + +const hasWindowsControl = (fileName: string): boolean => { + for (const character of fileName) { + const code = character.codePointAt(0)!; + if (code > 0 && code < 0x20) return true; + } + return false; +}; diff --git a/src/adb/AdbProvider.test.ts b/src/adb/AdbProvider.test.ts index 1ac912b27..9ea0f026b 100644 --- a/src/adb/AdbProvider.test.ts +++ b/src/adb/AdbProvider.test.ts @@ -1,6 +1,7 @@ import { createHash } from "node:crypto"; import { chmod, + lstat, mkdir, mkdtemp, readFile, @@ -52,6 +53,28 @@ const refusedBy = async ( return projectAnalysisError(result.error); }; +const namesAliasOnVolume = async ( + parent: string, + firstName: string, + secondName: string, +): Promise => { + const probe = join(parent, "destination-name-probe"); + await mkdir(probe); + try { + await mkdir(join(probe, firstName)); + try { + await mkdir(join(probe, secondName)); + return false; + } catch (cause) { + if (cause instanceof Error && "code" in cause && cause.code === "EEXIST") + return true; + throw cause; + } + } finally { + await rm(probe, { recursive: true, force: true }); + } +}; + it("inspects the adb client without contacting the server", async () => { const { binary } = await writeAdbStub( () => `if [ "$1" = "version" ]; then ${VERSION_BLOCK}; fi`, @@ -283,6 +306,82 @@ it("refuses to reuse an existing package output directory", async () => { expect(result.error.message).toContain("already exists"); }); +it.each([ + ["an empty filename", "package:/data/app/x/"], + [ + "duplicate local filenames", + "package:/data/app/x/base.apk\npackage:/data/app/y/base.apk", + ], +])("refuses %s before creating output or pulling", async (_, pmOutput) => { + const { binary, root } = await writeAdbStub( + (root) => `if [ "$1" = "version" ]; then ${VERSION_BLOCK}; exit 0; fi +if [ "$1" = "-s" ] && [ "$4" = "pm" ]; then printf '%b' '${pmOutput.replaceAll("'", "'\\''").replaceAll("\n", "\\n")}\\n'; exit 0; fi +if [ "$1" = "-s" ] && [ "$3" = "pull" ]; then : > '${join(root, "pull-ran")}'; exit 0; fi +exit 1`, + ); + const output = join(root, "pulls"); + const result = await providerOver(binary).execute({ + operation: "pull_adb_package", + input: { + serial: "emulator-5554", + package: "com.example", + output_directory: output, + }, + }); + expect(result.ok).toBe(false); + if (result.ok) return; + expect(result.error.capturedOutput?.stdout).toContain( + pmOutput.split("\n")[0], + ); + await expect(readFile(join(root, "pull-ran"))).rejects.toThrow(); + await expect(lstat(output)).rejects.toMatchObject({ code: "ENOENT" }); +}); + +it.each([ + ["case-only names", "base.apk", "BASE.APK"], + ["normalization-equivalent names", "caf\u00e9.apk", "cafe\u0301.apk"], +])( + "checks %s against the actual destination filesystem before pulling", + async (_, firstName, secondName) => { + const { binary, root } = await writeAdbStub((root) => + pullStub( + `'package:/data/app/x/${firstName}' 'package:/data/app/y/${secondName}'`, + `printf 'pulled\\n' >> '${join(root, "pull.log")}' + printf 'apk' > "$5"`, + ), + ); + const aliases = await namesAliasOnVolume(root, firstName, secondName); + const result = await providerOver(binary).execute({ + operation: "pull_adb_package", + input: { + serial: "emulator-5554", + package: "com.example", + output_directory: root, + }, + }); + const pullLog = join(root, "pull.log"); + const packageDirectory = join(root, "com.example"); + if (aliases) { + expect(result.ok).toBe(false); + if (result.ok) return; + expect(result.error.message).toContain("unique local destinations"); + await expect(readFile(pullLog)).rejects.toThrow(); + await expect(lstat(packageDirectory)).rejects.toThrow(); + return; + } + expect(result.ok).toBe(true); + if (!result.ok) throw new Error(result.error.message); + expect( + ( + result.value.result as { artifacts: { file_name: string }[] } + ).artifacts.map((artifact) => artifact.file_name), + ).toEqual([firstName, secondName]); + expect((await readFile(pullLog, "utf8")).trim().split("\n")).toHaveLength( + 2, + ); + }, +); + it("reports an uninstalled package from empty pm path output", async () => { const { binary } = await writeAdbStub( () => `if [ "$1" = "version" ]; then ${VERSION_BLOCK}; exit 0; fi @@ -465,14 +564,14 @@ exit 1`, operation: "pull_adb_file", input: { serial: "emulator-5554", - device_path: "/data/local/tmp/notes.txt", + device_path: "/data/local/tmp/My Notes:r*sumé.txt", output_directory: output, }, }); const digest = createHash("sha256").update("file-bytes").digest("hex"); expect(result).toMatchObject({ - device_path: "/data/local/tmp/notes.txt", - local_path: join(output, "notes.txt"), + device_path: "/data/local/tmp/My Notes:r*sumé.txt", + local_path: join(output, "My Notes:r*sumé.txt"), bytes: 10, sha256: digest, }); @@ -480,10 +579,7 @@ exit 1`, it.each([ ["/data/local/tmp/..", "dot_segment"], - ["/data/local/tmp/a\\b", "path_separator"], - ["/data/local/tmp/CON", "reserved_name"], - ["/data/local/tmp/report.", "trailing_dot_or_space"], - ["/data/local/tmp/report:2024", "alternate_data_stream"], + ["/data/local/tmp/", "empty_file_name"], ] as const)( "refuses the unsafe device path %s before pulling", async (devicePath, reason) => { @@ -510,13 +606,19 @@ exit 0`, { path: ["device_path"], reason: "invalid_value", - message: expect.stringContaining(reason), + message: expect.stringContaining( + `${JSON.stringify(devicePath)} does not name`, + ), }, ], }); + expect( + (projected.details as { issues: { message: string }[] }).issues[0]! + .message, + ).toContain(reason); // No host directory and no adb invocation are effects of a refused pull. await expect(readFile(join(root, "pull-ran"))).rejects.toThrow(); - await expect(readFile(output)).rejects.toThrow(); + await expect(lstat(output)).rejects.toMatchObject({ code: "ENOENT" }); }, ); diff --git a/src/adb/AdbProvider.ts b/src/adb/AdbProvider.ts index 82b43b08f..d4e353e30 100644 --- a/src/adb/AdbProvider.ts +++ b/src/adb/AdbProvider.ts @@ -1,7 +1,15 @@ import { execFile } from "node:child_process"; import { createHash } from "node:crypto"; import { constants as fsConstants } from "node:fs"; -import { access, lstat, mkdir, rm, stat, writeFile } from "node:fs/promises"; +import { + access, + lstat, + mkdir, + rmdir, + rm, + stat, + writeFile, +} from "node:fs/promises"; import { join, resolve } from "node:path"; import type { @@ -388,6 +396,68 @@ const removeOwnedPackageDirectory = async ( await rm(path, { recursive: true, force: true }); }; +const reservePackageDestinations = async ( + operation: string, + packageDirectory: string, + entries: readonly { readonly file_name: string }[], + capturedOutput: { + readonly stdout: string; + readonly stderr: string; + readonly truncated: boolean; + }, +): Promise => { + const reservations: { + readonly path: string; + readonly identity: PackageDirectoryIdentity; + }[] = []; + for (const entry of entries) { + const path = join(packageDirectory, entry.file_name); + try { + await mkdir(path); + const created = await lstat(path); + if (!created.isDirectory() || created.isSymbolicLink()) + throw new Error(`Could not reserve local filename ${entry.file_name}`); + reservations.push({ + path, + identity: { + dev: created.dev, + ino: created.ino, + mode: created.mode, + }, + }); + } catch (cause: unknown) { + return new AnalysisOutputError( + operation, + `Package Manager paths cannot be written to unique local destinations at ${JSON.stringify(entry.file_name)}; refusing before pulling any files`, + { cause, capturedOutput }, + ); + } + } + for (const reservation of reservations) { + try { + const current = await lstat(reservation.path); + if ( + !current.isDirectory() || + current.isSymbolicLink() || + current.dev !== reservation.identity.dev || + current.ino !== reservation.identity.ino || + current.mode !== reservation.identity.mode + ) + throw new Error( + `The local filename reservation changed before release: ${reservation.path}`, + ); + await rmdir(reservation.path); + } catch (cause: unknown) { + return new AnalysisOutputError( + operation, + `Could not release a local filename reservation before pulling: ${reservation.path}`, + { cause, capturedOutput }, + ); + } + } + return undefined; +}; + /** * ADB device inspection and acquisition over a caller-selected adb binary. * @@ -1156,6 +1226,19 @@ export class AdbProvider { ); if (!paths.ok) return paths; const parsed = parsePmPathOutput(paths.value.stdout); + const capturedOutput = { + stdout: paths.value.stdout, + stderr: paths.value.stderr, + truncated: false, + }; + if (parsed.rejectedPaths.length > 0) + return err( + new AnalysisOutputError( + operation, + `Package Manager reported paths that cannot be represented as host files: ${parsed.rejectedPaths.map((entry) => `${JSON.stringify(entry.device_path)} (${entry.reason})`).join(", ")}`, + { capturedOutput }, + ), + ); if (parsed.paths.length === 0 && parsed.unparsedLines.length === 0) return err( new AnalysisUnsupportedTargetError( @@ -1168,8 +1251,21 @@ export class AdbProvider { return err( new AnalysisProtocolError( `pm path reported an unrecognized line: ${parsed.unparsedLines[0]}`, + { capturedOutput }, ), ); + const destinationNames = new Set(); + for (const entry of parsed.paths) { + if (destinationNames.has(entry.file_name)) + return err( + new AnalysisOutputError( + operation, + `Package Manager reported multiple paths with the same local destination ${JSON.stringify(entry.file_name)}; refusing before pulling any files`, + { capturedOutput }, + ), + ); + destinationNames.add(entry.file_name); + } const directory = await prepareOutputDirectory( operation, input.output_directory, @@ -1228,6 +1324,43 @@ export class AdbProvider { ); return err(analysisErrorWithCleanupFailure(primary, cleanup, operation)); } + const failOwnedPackage = async ( + primary: AnalysisError, + ): Promise> => { + const owner: PendingPackageDirectoryCleanup = { + path: packageDirectory, + identity: packageDirectoryIdentity, + primary, + remove: () => + removeOwnedPackageDirectory( + packageDirectory, + packageDirectoryIdentity, + ), + }; + try { + await owner.remove(); + } catch (cause: unknown) { + this.#pendingPackageDirectoryCleanup.add(owner); + const cleanup = new ProviderCleanupError( + ADB_PROVIDER_IDENTITY.id, + [packageDirectory], + { reason: cause instanceof Error ? cause.message : String(cause) }, + { operation, cause }, + ); + return err( + analysisErrorWithCleanupFailure(primary, cleanup, operation), + ); + } + return err(primary); + }; + const destinationError = await reservePackageDestinations( + operation, + packageDirectory, + parsed.paths, + capturedOutput, + ); + if (destinationError !== undefined) + return failOwnedPackage(destinationError); const artifacts: { device_path: string; file_name: string; @@ -1273,17 +1406,7 @@ export class AdbProvider { }); return err(error); }; - const seenFileNames = new Set(); for (const entry of parsed.paths) { - if (seenFileNames.has(entry.file_name)) { - failures.push({ - device_path: entry.device_path, - stage: "pull", - message: `Duplicate file name ${entry.file_name} in the APK set; refusing to overwrite`, - }); - continue; - } - seenFileNames.add(entry.file_name); const localPath = join(packageDirectory, entry.file_name); const pulled = await runAdb( operation, @@ -1338,33 +1461,7 @@ export class AdbProvider { const primary = new AnalysisProtocolError( `Every APK pull failed for ${packageName} on ${serial}: ${failures[0]!.message}`, ); - const owner: PendingPackageDirectoryCleanup = { - path: packageDirectory, - identity: packageDirectoryIdentity, - primary, - remove: () => - removeOwnedPackageDirectory( - packageDirectory, - packageDirectoryIdentity, - ), - }; - try { - await owner.remove(); - } catch (cause: unknown) { - this.#pendingPackageDirectoryCleanup.add(owner); - const cleanup = new ProviderCleanupError( - ADB_PROVIDER_IDENTITY.id, - [packageDirectory], - { - reason: cause instanceof Error ? cause.message : String(cause), - }, - { operation: operation, cause }, - ); - return err( - analysisErrorWithCleanupFailure(primary, cleanup, operation), - ); - } - return err(primary); + return failOwnedPackage(primary); } return ok( createAnalysisExecution( @@ -1407,7 +1504,7 @@ export class AdbProvider { { path: ["device_path"], reason: "invalid_value", - message: `The device path does not name a portable host file (${derived.reason}); REA never renames a device-reported file.`, + message: `The device path ${JSON.stringify(input.device_path)} does not name a file that can be written on this host (${derived.reason}); REA never renames a device-reported file.`, }, ]), ); diff --git a/tests/boundary/cli/adbCli.test.ts b/tests/boundary/cli/adbCli.test.ts index 88d20af9b..71309e878 100644 --- a/tests/boundary/cli/adbCli.test.ts +++ b/tests/boundary/cli/adbCli.test.ts @@ -1,9 +1,14 @@ -import { expect, it } from "vitest"; +import { chmod, lstat, readFile } from "node:fs/promises"; +import { expect, it, onTestFinished } from "vitest"; import { expectConfigurationRefusal, serveCli as serve, } from "./cliServeHarness.js"; +import { + createTestWorkspace, + removeTestWorkspace, +} from "../../support/workspace/workspaceFixture.js"; it.each([ [ @@ -46,3 +51,40 @@ it("requires the output directory option on pull", async () => { ]); expect(result.exitCode).not.toBe(0); }); + +it.skipIf(process.platform === "win32")( + "refuses a trailing device separator through the CLI before adb or output effects", + async () => { + const workspace = await createTestWorkspace("rea-adb-cli-pull-"); + onTestFinished(() => removeTestWorkspace(workspace.root)); + const adb = await workspace.write( + "adb", + `#!/bin/sh +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 +: > '${workspace.path("invoked")}' +exit 0 +`, + ); + await chmod(adb, 0o755); + const output = workspace.path("pulls"); + const result = await serve( + [ + "pull-adb-file", + "emulator-5554", + "/data/local/tmp/", + "--output-directory", + output, + "--json", + ], + { REA_ADB_PATH: adb }, + ); + expect(JSON.parse(result.stdout)).toMatchObject({ + code: "invalid_request", + }); + await expect(readFile(workspace.path("invoked"))).rejects.toThrow(); + await expect(lstat(output)).rejects.toMatchObject({ code: "ENOENT" }); + }, +);