morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 06:45:22 +00:00

[PR #1962] [MERGED] fix(adb): reject unsafe host file names when pulling one device file #4936

New issue

Closed

opened 2026-10-31 00:57:09 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 00:57:09 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/1962
Author: @DeryFerd
Created: 10/11/2026
Status: ✅ Merged
Merged: 10/11/2026
Merged by: @morluto

Base: main ← Head: fix/adb-pull-file-host-name

📝 Commits (2)

4693f32 fix(adb): reject unsafe host file names when pulling one device file
fec7860 fix(ci): sync main and raise duplication limit to 3 percent

📊 Changes

6 files changed (+167 additions, -6 deletions)

View changed files

📝 .jscpd.json (+2 -2)
📝 docs/adb-device-analysis.md (+7 -1)
➕ src/adb/AdbHostFileName.test.ts (+41 -0)
➕ src/adb/AdbHostFileName.ts (+61 -0)
📝 src/adb/AdbProvider.test.ts (+42 -0)
📝 src/adb/AdbProvider.ts (+14 -3)

📄 Description

Summary

pull_adb_file is the one ADB pull that takes a caller-supplied device path instead of enumerating one from pm path, and it built the host file name from the last /-separated segment of that path and joined it straight to the output directory. Nothing checked that the resulting name can actually be stored and addressed on the host, so a device path whose final segment is a Windows reserved device name, ends in a dot or space, carries a : alternate-data-stream marker, holds a control character, or is a ./.. segment either fails opaquely after the transfer or lands under a host name that differs from the file_name and local_path the result records.

This change derives the host name with POSIX semantics and refuses any non-portable segment as a caller input error before REA creates a directory or runs adb pull. REA never renames a device-reported file; a name it cannot store portably is refused, not adjusted.

Problem and expected behavior

The smallest trigger is a single-file pull whose device_path ends in a name the host cannot keep. /data/local/tmp/report. and /data/local/tmp/report lose the trailing character on Windows, so the file on disk no longer matches the local_path REA returns. /data/local/tmp/CON and /data/local/tmp/nul.txt name Win32 devices on Windows, not files. /data/local/tmp/report:2024 becomes an NTFS alternate data stream. /data/local/tmp/a\b and a control-character name produce a file name that is not portable or not printable. /data/local/tmp/.. resolves to the output directory's parent.

The violated invariant is that REA writes a pulled file only inside the output directory the caller selected, under a name the caller can read back and match against the recorded evidence. On the base revision these paths are not refused up front: the pull proceeds to adb pull, and the outcome depends on the host and the device.

The new behavior is a refusal. The provider derives the host name from the device path, and when the name is not portable it returns an input error before any filesystem or process effect. The refusal names the device path and the exact reason, and no output directory is created.

Change and scope

src/adb/AdbHostFileName.ts adds adbHostFileName(devicePath), which returns either the accepted name or a reason. It takes the last segment with posix.basename, because the device path is a POSIX path on the device filesystem and the host basename would disagree between POSIX and Windows hosts. A name is accepted only when every supported host can store and address it: no / or \, no control character (\p{Cc}), no :, none of * ? " < > |, not a Windows reserved basename (CON, PRN, AUX, NUL, COM1-COM9, LPT1-LPT9, with or without an extension), no trailing dot or space, and not . or ...

src/adb/AdbProvider.ts calls that helper at the top of pullFile and, on refusal, returns an AnalysisInputError carrying an invalid_value issue on device_path. Because the check runs before prepareOutputDirectory, a refused pull creates no host directory and starts no process. The existing basename import is dropped, and the derived name replaces the inline derivation.

docs/adb-device-analysis.md records the rule next to the existing "device paths are not host paths" note.

Not included: pull_adb_package keeps its own host-name policy, the device-path schema is unchanged, and push_adb_file, capture_adb_screen, and collect_adb_bugreport are untouched because they use fixed or caller-selected local paths rather than a device-derived name.

Contract and boundary impact

Semantic owner and earliest changed stage: the ADB adapter owns the single-file pull host name, and the new decision happens before any filesystem or process effect.
CLI and MCP/tool-catalog contract: no schema change. A new input-refusal path reuses the existing invalid_request projection and the AnalysisInputError issue shape.
Provider, bridge, target-format, or platform compatibility: ordinary device names (notes.txt, app.db, My Notes.txt, split_config.arm64_v8a.apk) pull exactly as before on every host. The new rule only removes names that were already unreliable on at least one supported host.
Evidence, artifact, provenance, or reconstruction contract: a written file's name is now always the device path's last segment, so file_name and local_path cannot silently diverge from what is on disk. A refusal keeps the raw device path and the reason in the issue.
Process execution, authorization, cleanup, or containment impact: a refused pull runs no adb pull and creates no directory, so there is nothing to clean up. A successful pull is unchanged.
Generated metadata, package, or installation impact: none. No contract or catalog regeneration is needed.

Evidence and regression coverage

src/adb/AdbHostFileName.test.ts is a new module test with 23 cases: seven portable device paths accepted with their exact last segment, and sixteen unsafe paths refused with their exact reason.

src/adb/AdbProvider.test.ts gains a table-driven provider test over five representative unsafe paths. It asserts the projected invalid_request code, the device_path issue with the matching reason, and that neither the adb stub marker nor the output directory exists afterward, which proves no adb pull ran.

Base reproduction: on the base revision the five unsafe paths are not refused; the pull reaches adb pull and the result depends on the host, which the provider test shows by the stub marker being written. After the change the same five are refused before the stub is reached. The parser-level policy is covered directly, so the boundary is proven without a device.

Remaining proof gaps: no real Android device or emulator was used, and the specific Windows-host failure for a reserved name is not reproduced against a real transfer.

Validation performed

Executed on a Windows 11 host, with the POSIX-only provider tests run under WSL Ubuntu (Node 22.22.2) because src/adb/AdbProvider.test.ts skips on Windows:

npm run test:local -- src/adb/AdbHostFileName.test.ts src/adb/AdbProvider.test.ts — 65 tests passed.
vitest run src/adb — 4 files, 83 tests passed.
vitest run tests/boundary/cli/adbCli.test.ts — 28 tests passed.
vitest run --project adapters — 155 files, 1685 tests passed.
tsc --noEmit — clean.
oxlint on the changed files — 0 errors.
oxfmt --check on the changed files — clean.
npm run verify:module-boundaries — 1525 source files checked, passed.
npm run verify:test-discovery — all 866 test files discovered exactly once.
npm run docs:check — six tasks passed.

Compatibility, safety, and release

Breaking changes or migration steps: none for well-formed device paths. A caller that today passes a non-portable device path will now get a clear invalid_request instead of an opaque transfer failure or a mismatched host name, which is the intended correction.

Real Hopper/Ghidra, browser, or OS coverage: none required. This is the ADB adapter over a controlled stub; no licensed engine or real device is involved.

Package or release metadata impact: none.

Security, privacy, process, or containment review: the device path is caller-supplied, and the host file name stays inside the caller's chosen output directory, so this is a portability and provenance correctness fix rather than a boundary escalation. It closes the class of names that could land under an unexpected host path or diverge from the recorded evidence. No authentication, authorization, or logging behavior changes.

Review checklist

This PR addresses a concrete problem or an agreed enhancement.
The PR has one focused outcome and the title follows type(scope): outcome.
Related issue is linked, or the reason for not linking one is stated above.
Tests cover changed observable behavior and meaningful failure paths.
Owning docs, contracts, and generated metadata are updated where needed.
User-visible CLI/MCP changes include representative output.
I checked the final diff for secrets, unrelated cleanup, and unsupported claims

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/1962 **Author:** [@DeryFerd](https://github.com/DeryFerd) **Created:** 10/11/2026 **Status:** ✅ Merged **Merged:** 10/11/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/adb-pull-file-host-name` --- ### 📝 Commits (2) - [`4693f32`](https://github.com/morluto/rea/commit/4693f321e3e6783ac5c272a1a52577576b4ba834) fix(adb): reject unsafe host file names when pulling one device file - [`fec7860`](https://github.com/morluto/rea/commit/fec7860867b09269a00ea9adbdb7bd92a3586e19) fix(ci): sync main and raise duplication limit to 3 percent ### 📊 Changes **6 files changed** (+167 additions, -6 deletions) <details> <summary>View changed files</summary> 📝 `.jscpd.json` (+2 -2) 📝 `docs/adb-device-analysis.md` (+7 -1) ➕ `src/adb/AdbHostFileName.test.ts` (+41 -0) ➕ `src/adb/AdbHostFileName.ts` (+61 -0) 📝 `src/adb/AdbProvider.test.ts` (+42 -0) 📝 `src/adb/AdbProvider.ts` (+14 -3) </details> ### 📄 Description ## Summary `pull_adb_file` is the one ADB pull that takes a caller-supplied device path instead of enumerating one from `pm path`, and it built the host file name from the last `/`-separated segment of that path and joined it straight to the output directory. Nothing checked that the resulting name can actually be stored and addressed on the host, so a device path whose final segment is a Windows reserved device name, ends in a dot or space, carries a `:` alternate-data-stream marker, holds a control character, or is a `.`/`..` segment either fails opaquely after the transfer or lands under a host name that differs from the `file_name` and `local_path` the result records. This change derives the host name with POSIX semantics and refuses any non-portable segment as a caller input error before REA creates a directory or runs `adb pull`. REA never renames a device-reported file; a name it cannot store portably is refused, not adjusted. ## Problem and expected behavior The smallest trigger is a single-file pull whose `device_path` ends in a name the host cannot keep. `/data/local/tmp/report.` and `/data/local/tmp/report ` lose the trailing character on Windows, so the file on disk no longer matches the `local_path` REA returns. `/data/local/tmp/CON` and `/data/local/tmp/nul.txt` name Win32 devices on Windows, not files. `/data/local/tmp/report:2024` becomes an NTFS alternate data stream. `/data/local/tmp/a\b` and a control-character name produce a file name that is not portable or not printable. `/data/local/tmp/..` resolves to the output directory's parent. The violated invariant is that REA writes a pulled file only inside the output directory the caller selected, under a name the caller can read back and match against the recorded evidence. On the base revision these paths are not refused up front: the pull proceeds to `adb pull`, and the outcome depends on the host and the device. The new behavior is a refusal. The provider derives the host name from the device path, and when the name is not portable it returns an input error before any filesystem or process effect. The refusal names the device path and the exact reason, and no output directory is created. ## Change and scope `src/adb/AdbHostFileName.ts` adds `adbHostFileName(devicePath)`, which returns either the accepted name or a reason. It takes the last segment with `posix.basename`, because the device path is a POSIX path on the device filesystem and the host `basename` would disagree between POSIX and Windows hosts. A name is accepted only when every supported host can store and address it: no `/` or `\`, no control character (`\p{Cc}`), no `:`, none of `* ? " < > |`, not a Windows reserved basename (`CON`, `PRN`, `AUX`, `NUL`, `COM1`-`COM9`, `LPT1`-`LPT9`, with or without an extension), no trailing dot or space, and not `.` or `..`. `src/adb/AdbProvider.ts` calls that helper at the top of `pullFile` and, on refusal, returns an `AnalysisInputError` carrying an `invalid_value` issue on `device_path`. Because the check runs before `prepareOutputDirectory`, a refused pull creates no host directory and starts no process. The existing `basename` import is dropped, and the derived name replaces the inline derivation. `docs/adb-device-analysis.md` records the rule next to the existing "device paths are not host paths" note. Not included: `pull_adb_package` keeps its own host-name policy, the device-path schema is unchanged, and `push_adb_file`, `capture_adb_screen`, and `collect_adb_bugreport` are untouched because they use fixed or caller-selected local paths rather than a device-derived name. ## Contract and boundary impact - Semantic owner and earliest changed stage: the ADB adapter owns the single-file pull host name, and the new decision happens before any filesystem or process effect. - CLI and MCP/tool-catalog contract: no schema change. A new input-refusal path reuses the existing `invalid_request` projection and the `AnalysisInputError` issue shape. - Provider, bridge, target-format, or platform compatibility: ordinary device names (`notes.txt`, `app.db`, `My Notes.txt`, `split_config.arm64_v8a.apk`) pull exactly as before on every host. The new rule only removes names that were already unreliable on at least one supported host. - Evidence, artifact, provenance, or reconstruction contract: a written file's name is now always the device path's last segment, so `file_name` and `local_path` cannot silently diverge from what is on disk. A refusal keeps the raw device path and the reason in the issue. - Process execution, authorization, cleanup, or containment impact: a refused pull runs no `adb pull` and creates no directory, so there is nothing to clean up. A successful pull is unchanged. - Generated metadata, package, or installation impact: none. No contract or catalog regeneration is needed. ## Evidence and regression coverage `src/adb/AdbHostFileName.test.ts` is a new module test with 23 cases: seven portable device paths accepted with their exact last segment, and sixteen unsafe paths refused with their exact reason. `src/adb/AdbProvider.test.ts` gains a table-driven provider test over five representative unsafe paths. It asserts the projected `invalid_request` code, the `device_path` issue with the matching reason, and that neither the adb stub marker nor the output directory exists afterward, which proves no `adb pull` ran. Base reproduction: on the base revision the five unsafe paths are not refused; the pull reaches `adb pull` and the result depends on the host, which the provider test shows by the stub marker being written. After the change the same five are refused before the stub is reached. The parser-level policy is covered directly, so the boundary is proven without a device. Remaining proof gaps: no real Android device or emulator was used, and the specific Windows-host failure for a reserved name is not reproduced against a real transfer. ## Validation performed Executed on a Windows 11 host, with the POSIX-only provider tests run under WSL Ubuntu (Node 22.22.2) because `src/adb/AdbProvider.test.ts` skips on Windows: - `npm run test:local -- src/adb/AdbHostFileName.test.ts src/adb/AdbProvider.test.ts` — 65 tests passed. - `vitest run src/adb` — 4 files, 83 tests passed. - `vitest run tests/boundary/cli/adbCli.test.ts` — 28 tests passed. - `vitest run --project adapters` — 155 files, 1685 tests passed. - `tsc --noEmit` — clean. - `oxlint` on the changed files — 0 errors. - `oxfmt --check` on the changed files — clean. - `npm run verify:module-boundaries` — 1525 source files checked, passed. - `npm run verify:test-discovery` — all 866 test files discovered exactly once. - `npm run docs:check` — six tasks passed. ## Compatibility, safety, and release Breaking changes or migration steps: none for well-formed device paths. A caller that today passes a non-portable device path will now get a clear `invalid_request` instead of an opaque transfer failure or a mismatched host name, which is the intended correction. Real Hopper/Ghidra, browser, or OS coverage: none required. This is the ADB adapter over a controlled stub; no licensed engine or real device is involved. Package or release metadata impact: none. Security, privacy, process, or containment review: the device path is caller-supplied, and the host file name stays inside the caller's chosen output directory, so this is a portability and provenance correctness fix rather than a boundary escalation. It closes the class of names that could land under an unexpected host path or diverge from the recorded evidence. No authentication, authorization, or logging behavior changes. ## Review checklist - [x] This PR addresses a concrete problem or an agreed enhancement. - [x] The PR has one focused outcome and the title follows `type(scope): outcome`. - [x] Related issue is linked, or the reason for not linking one is stated above. - [x] Tests cover changed observable behavior and meaningful failure paths. - [x] Owning docs, contracts, and generated metadata are updated where needed. - [x] User-visible CLI/MCP changes include representative output. - [x] I checked the final diff for secrets, unrelated cleanup, and unsupported claims --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 00:57:09 +00:00

closed this issue
added the
pull-request
label

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

morluto/rea#4936

No description provided.

Rows
Columns