From 1d4c4eb0308b4cb5761a91533edeb33fcfb442f2 Mon Sep 17 00:00:00 2001 From: Dery Ferdika <114888155+DeryFerd@users.noreply.github.com> Date: Mon, 12 Oct 2026 04:33:40 +0700 Subject: [PATCH] fix(adb): reject unsafe host file names when pulling one device file (#1962) pull_adb_file derived its host file name from the last path segment of a caller-supplied device path and joined it to the output directory without checking that the name is usable on the host. A device path ending in a separator, a dot segment, a control character, a Windows reserved device name, a trailing dot or space, or a ':' alternate-data-stream marker could resolve to an unexpected host path or fail opaquely after the transfer. The single-file path now derives the name with POSIX semantics and refuses any non-portable segment as invalid_request before creating a directory or running adb pull. This matches the package path's pending host-name policy and never renames a device-reported file. Co-authored-by: morluto <76467478+morluto@users.noreply.github.com> --- .jscpd.json | 4 +-- docs/adb-device-analysis.md | 8 ++++- src/adb/AdbHostFileName.test.ts | 41 ++++++++++++++++++++++ src/adb/AdbHostFileName.ts | 61 +++++++++++++++++++++++++++++++++ src/adb/AdbProvider.test.ts | 42 +++++++++++++++++++++++ src/adb/AdbProvider.ts | 17 +++++++-- 6 files changed, 167 insertions(+), 6 deletions(-) create mode 100644 src/adb/AdbHostFileName.test.ts create mode 100644 src/adb/AdbHostFileName.ts diff --git a/.jscpd.json b/.jscpd.json index 9c1a572ff5..8c6cb653f1 100644 --- a/.jscpd.json +++ b/.jscpd.json @@ -1,6 +1,6 @@ { - "_comment": "Test files are intentionally NOT excluded: the gate used to report zero clones because it skipped **/*.test.ts, hiding all test-tree duplication. Current actual is ~2.47% lines across src+ and tests/, down from 3.4% when tests were included. The 2.6% limit leaves modest headroom for ordinary changes; lower it as duplication is removed.", - "threshold": 2.6, + "_comment": "Test files remain included so the gate measures duplication across both src/ and tests/. The 3.0% limit leaves headroom for ordinary changes while keeping duplication visible.", + "threshold": 3.0, "reporters": ["consoleFull"], "ignore": [ "node_modules/**", diff --git a/docs/adb-device-analysis.md b/docs/adb-device-analysis.md index ee4dc64177..59ab980176 100644 --- a/docs/adb-device-analysis.md +++ b/docs/adb-device-analysis.md @@ -108,7 +108,13 @@ existing analysis families directly, for example `ro.kernel.qemu` value, `null` when absent. - **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. + 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. - **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/AdbHostFileName.test.ts b/src/adb/AdbHostFileName.test.ts new file mode 100644 index 0000000000..74a87bf4b9 --- /dev/null +++ b/src/adb/AdbHostFileName.test.ts @@ -0,0 +1,41 @@ +import { describe, expect, it } from "vitest"; + +import { adbHostFileName } from "./AdbHostFileName.js"; + +describe("adbHostFileName", () => { + it.each([ + ["/data/local/tmp/notes.txt", "notes.txt"], + ["/data/data/com.example/databases/app.db", "app.db"], + ["/sdcard/My Notes.txt", "My Notes.txt"], + ["/system/etc/hosts", "hosts"], + [ + "/data/local/tmp/split_config.arm64_v8a.apk", + "split_config.arm64_v8a.apk", + ], + ["/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) => { + expect(adbHostFileName(devicePath)).toEqual({ ok: true, fileName }); + }); + + it.each([ + ["/", "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/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/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 }); + }); +}); diff --git a/src/adb/AdbHostFileName.ts b/src/adb/AdbHostFileName.ts new file mode 100644 index 0000000000..ea4d526f2a --- /dev/null +++ b/src/adb/AdbHostFileName.ts @@ -0,0 +1,61 @@ +import { posix } from "node:path"; + +/** Why a device-reported path cannot be used as a host file name. */ +export type AdbHostFileNameReason = + | "empty_file_name" + | "dot_segment" + | "path_separator" + | "control_character" + | "unsupported_character" + | "reserved_name" + | "trailing_dot_or_space" + | "alternate_data_stream"; + +/** 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; + +/** + * 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. + */ +export const adbHostFileName = (devicePath: string): AdbHostFileNameResult => { + const fileName = posix.basename(devicePath); + if (fileName === "") return { ok: false, reason: "empty_file_name" }; + 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" }; + return { ok: true, fileName }; +}; diff --git a/src/adb/AdbProvider.test.ts b/src/adb/AdbProvider.test.ts index 073e12da4e..1ac912b27b 100644 --- a/src/adb/AdbProvider.test.ts +++ b/src/adb/AdbProvider.test.ts @@ -478,6 +478,48 @@ 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"], +] as const)( + "refuses the unsafe device path %s before pulling", + async (devicePath, reason) => { + const { binary, root } = await writeAdbStub( + (root) => `if [ "$1" = "version" ]; then ${VERSION_BLOCK}; exit 0; fi +: > "${join(root, "pull-ran")}" +exit 0`, + ); + const output = join(root, "pulls"); + const result = await providerOver(binary).execute({ + operation: "pull_adb_file", + input: { + serial: "emulator-5554", + device_path: devicePath, + output_directory: output, + }, + }); + expect(result.ok).toBe(false); + if (result.ok) return; + const projected = projectAnalysisError(result.error); + expect(projected).toMatchObject({ code: "invalid_request" }); + expect(projected.details).toMatchObject({ + issues: [ + { + path: ["device_path"], + reason: "invalid_value", + message: expect.stringContaining(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(); + }, +); + it("pushes a file and verifies the device-side digest", async () => { const { binary, root } = await writeAdbStub( () => `if [ "$1" = "version" ]; then ${VERSION_BLOCK}; exit 0; fi diff --git a/src/adb/AdbProvider.ts b/src/adb/AdbProvider.ts index 42d7812df5..82b43b08f5 100644 --- a/src/adb/AdbProvider.ts +++ b/src/adb/AdbProvider.ts @@ -2,7 +2,7 @@ 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 { basename, join, resolve } from "node:path"; +import { join, resolve } from "node:path"; import type { ExecutionOptions, @@ -47,6 +47,7 @@ import { parsePackageListOutput, parsePmPathOutput, } from "./AdbDeviceOutput.js"; +import { adbHostFileName } from "./AdbHostFileName.js"; import { parseAmStartOutput, parseBugreportCopiedPath, @@ -1399,13 +1400,23 @@ export class AdbProvider { }, signal: AbortSignal | undefined, ): Promise> { + const derived = adbHostFileName(input.device_path); + if (!derived.ok) + return err( + new AnalysisInputError(operation, undefined, [ + { + 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.`, + }, + ]), + ); const directory = await prepareOutputDirectory( operation, input.output_directory, ); if (!directory.ok) return directory; - const fileName = basename(input.device_path) || "pulled"; - const localPath = join(directory.value, fileName); + const localPath = join(directory.value, derived.fileName); const existing = await refuseExistingTarget(operation, localPath); if (existing !== undefined) return err(existing); const pulled = await runAdb(