[PR #3575] [MERGED] fix(contracts): reject NUL bytes in Frida, Rizin, and GDB local path inputs #5834

Closed
opened 2026-10-31 01:09:47 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

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

Base: main ← Head: fix/local-path-schema-frida-rizin-gdb


📝 Commits (1)

  • 08193b0 fix(contracts): reject NUL bytes in Frida, Rizin, and GDB local path inputs

📊 Changes

4 files changed (+42 additions, -9 deletions)

View changed files

📝 src/contracts/fridaToolContracts.ts (+5 -4)
📝 src/contracts/gdbToolContracts.ts (+2 -3)
📝 src/contracts/rizinDebugToolContracts.ts (+3 -2)
📝 tests/acceptance/tools/localToolContracts.test.ts (+32 -0)

📄 Description

Summary

Fixes a contract gap where several caller-selected local path inputs on the Frida, Rizin, and GDB tools were typed as plain strings instead of the shared local-path schema, so an embedded NUL byte passed advertised and runtime validation and only failed later inside a provider or a filesystem call.

Every other local-input tool in REA already refuses NUL at the contract boundary through localPathStringSchema in src/domain/localPath.ts. These seven fields were added after the earlier NUL sweeps (#1873, #1899, #1925) and were not included, so they drifted from the rest of the surface. This change routes them through the same shared schema and extends the existing OS-invalid-local-input acceptance harness to the tools that were missing from it.

Problem and expected behavior

The smallest trigger is calling start_frida_session with {"mode":"spawn","device_id":"local","program":"/tmp/selected\u0000filename"}. The advertised JSON Schema only required minLength: 1, so the value was accepted, and the NUL was carried into the Frida spawn path where it surfaced as an opaque capability_unavailable error rather than a named input constraint. The expected behavior is a typed schema rejection that names the NUL byte, consistent with every other local path tool.

The violated invariant is that caller-selected local filesystem paths are validated once at the owning contract boundary. A NUL byte is never a valid path component on any supported platform, so admitting it defers a deterministic, caller-visible mistake to a deeper layer, where it reads as a provider failure instead of a request error. The same gap applied to load_frida_script.path, both instrument_with_frida path and program fields, start_rizin_debug_session.path, start_rizin_debug_session.inferior_tty, and start_gdb_session.inferior_tty.

Change and scope

  • Route start_frida_session.program, load_frida_script.path, instrument_with_frida.program, and instrument_with_frida.path in src/contracts/fridaToolContracts.ts through localPathStringSchema.
  • Route start_rizin_debug_session.path and start_rizin_debug_session.inferior_tty in src/contracts/rizinDebugToolContracts.ts through the same schema.
  • Route start_gdb_session.inferior_tty in src/contracts/gdbToolContracts.ts through the same schema, keeping its existing description.
  • Extend tests/acceptance/tools/localToolContracts.test.ts with one case per field so the shared harness covers the Frida, Rizin, and GDB tools.

The approach reuses the existing schema owner rather than adding a new validation layer, which is how the earlier fixes in this class landed. The .regex(/^[^\x00]*$/u) pattern on nulFreeStringSchema survives z.toJSONSchema, so the constraint is advertised to clients and enforced at runtime from the same definition. GDB already rejected a NUL inferior_tty at runtime in GdbSessionManager, so its change is contract consistency rather than a behavior change; Frida and Rizin had no service-level NUL check for these fields.

Intentionally not included: no changes to the path values themselves, no new absolute-path requirement (that would be a separate, breaking contract change), and no changes to any tool outside the Frida, Rizin, and GDB contracts.

Contract and boundary impact

  • Semantic owner and earliest changed stage: the tool contracts in src/contracts/ own input admission; the change moves the earliest NUL rejection to the advertised contract boundary.
  • Contribution scope fit and linked maintainer decision: ordinary fix within existing boundaries; not applicable, no new provider or capability.
  • CLI and MCP/tool-catalog contract: the affected tool input schemas now advertise a NUL-free pattern in addition to minLength: 1. The generated MCP tool catalog is regenerated by CI and validated; no committed catalog file changes.
  • Provider, bridge, target-format, or platform compatibility: none. Frida, Rizin, and GDB are bring-your-own providers; the change only rejects an already-invalid input earlier.
  • Evidence, artifact, provenance, or reconstruction contract: none.
  • Target selection, authority, upstream behavior, and cleanup details (if applicable): none. The change does not alter target selection, execution, or cleanup.
  • Generated metadata (docs/public/product-catalog.json), package, or installation impact (CI generates and validates ignored catalogs and skills/; edit authored instructions in .agents/skills/reverse-engineer-anything/ and never commit build outputs): none.

Expected effects (check known effects; use documentation-only on its own). Unchecked items do not establish absence; leave uncertainties visible.

  • Read existing files or captured data.
  • Launch or attach to a process.
  • Inject instrumentation or run scripts/code.
  • Change process memory, registers, files, or configuration.
  • Contact network services.
  • Recover protected code or work around protections.
  • Documentation or usability only; no target interaction.
  • Not sure yet; investigation or maintainer guidance needed.

Evidence and regression coverage

  • Tests added or updated: seven cases added to tests/acceptance/tools/localToolContracts.test.ts, which drives the real advertised MCP contracts through the built server and asserts each request is rejected with a message containing NUL.
  • Base reproduction or other evidence: on the pre-change revision the same harness fails on start_frida_session, showing the NUL input passed schema validation and reached the provider, where it surfaced as capability_unavailable with the message Unable to find executable at '/tmp/selected' (the value truncated at the NUL). After the change the harness passes and the response is a schema error naming the NUL byte.
  • User-visible CLI/MCP output (if applicable): an MCP call with a NUL path now returns a validation error at the contract boundary instead of a provider error.
  • Remaining proof gaps: the added cases exercise the shared acceptance harness through the built server; there is no separate per-provider live run for these specific rejected inputs, because the refusal happens before any provider contact.

For evidence-bearing changes:

  • Observed, derived, and inferred claims remain distinguishable.
  • Artifact identity, source provenance, and failed attempts remain preserved.
  • Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.

Validation performed

  • npm run test:focused -- tests/acceptance/tools/localToolContracts.test.ts — 1 passed (the OS-invalid-local-input matrix now includes the Frida, Rizin, and GDB cases); fails on the base revision for the intended reason.
  • npm run test:focused -- src/contracts/fridaToolContracts.test.ts src/contracts/sessionPathInputs.test.ts src/contracts/advertisedInputExamples.test.ts tests/boundary/mcp/fridaLifecycleMcp.test.ts tests/boundary/mcp/rizinDebuggerCancellationMcp.test.ts tests/boundary/mcp/gdbCancellationMcp.test.ts — 49 passed, 5 skipped.
  • npm run test:focused -- src/contracts/compactInputSchema.test.ts src/contracts/toolInputReferences.test.ts src/contracts/toolOutputReferences.test.ts src/contracts/workflowInputContracts.test.ts — 43 passed.
  • npm run typecheck — clean.
  • npx oxlint on the four changed files — 0 warnings, 0 errors.
  • npx oxfmt --check on the four changed files — clean.
  • npm run verify:module-boundaries — checked 1886 source files, clean.
  • npm run mcp-catalog:check — passed.
  • npm run docs:check — 5 passed.

Compatibility, safety, and release

  • Breaking changes or migration steps: none. The change only rejects inputs that were never valid paths, so no correct caller is affected.
  • Real Hopper/Ghidra, browser, or OS coverage: not applicable; no provider engine behavior changes. Frida, Rizin, and GDB remain bring-your-own and are not started by these tests.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: this hardens input admission at the contract boundary for local path inputs. It does not expand any authority, and the refusal happens before any provider or filesystem effect.

Author readiness

Human owner: @DeryFerd

  • I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work.
  • I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps.
  • I declare this PR ready for initial maintainer review.

Before merge

  • This PR addresses a concrete problem or an agreed enhancement.
  • Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff.
  • 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/3575 **Author:** [@DeryFerd](https://github.com/DeryFerd) **Created:** 10/14/2026 **Status:** ✅ Merged **Merged:** 10/14/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/local-path-schema-frida-rizin-gdb` --- ### 📝 Commits (1) - [`08193b0`](https://github.com/morluto/rea/commit/08193b0acf7d89818d77f1881d1c4078ffae1daf) fix(contracts): reject NUL bytes in Frida, Rizin, and GDB local path inputs ### 📊 Changes **4 files changed** (+42 additions, -9 deletions) <details> <summary>View changed files</summary> 📝 `src/contracts/fridaToolContracts.ts` (+5 -4) 📝 `src/contracts/gdbToolContracts.ts` (+2 -3) 📝 `src/contracts/rizinDebugToolContracts.ts` (+3 -2) 📝 `tests/acceptance/tools/localToolContracts.test.ts` (+32 -0) </details> ### 📄 Description ## Summary Fixes a contract gap where several caller-selected local path inputs on the Frida, Rizin, and GDB tools were typed as plain strings instead of the shared local-path schema, so an embedded NUL byte passed advertised and runtime validation and only failed later inside a provider or a filesystem call. Every other local-input tool in REA already refuses NUL at the contract boundary through `localPathStringSchema` in `src/domain/localPath.ts`. These seven fields were added after the earlier NUL sweeps (#1873, #1899, #1925) and were not included, so they drifted from the rest of the surface. This change routes them through the same shared schema and extends the existing OS-invalid-local-input acceptance harness to the tools that were missing from it. ## Problem and expected behavior The smallest trigger is calling `start_frida_session` with `{"mode":"spawn","device_id":"local","program":"/tmp/selected\u0000filename"}`. The advertised JSON Schema only required `minLength: 1`, so the value was accepted, and the NUL was carried into the Frida spawn path where it surfaced as an opaque `capability_unavailable` error rather than a named input constraint. The expected behavior is a typed schema rejection that names the NUL byte, consistent with every other local path tool. The violated invariant is that caller-selected local filesystem paths are validated once at the owning contract boundary. A NUL byte is never a valid path component on any supported platform, so admitting it defers a deterministic, caller-visible mistake to a deeper layer, where it reads as a provider failure instead of a request error. The same gap applied to `load_frida_script.path`, both `instrument_with_frida` path and program fields, `start_rizin_debug_session.path`, `start_rizin_debug_session.inferior_tty`, and `start_gdb_session.inferior_tty`. ## Change and scope - Route `start_frida_session.program`, `load_frida_script.path`, `instrument_with_frida.program`, and `instrument_with_frida.path` in `src/contracts/fridaToolContracts.ts` through `localPathStringSchema`. - Route `start_rizin_debug_session.path` and `start_rizin_debug_session.inferior_tty` in `src/contracts/rizinDebugToolContracts.ts` through the same schema. - Route `start_gdb_session.inferior_tty` in `src/contracts/gdbToolContracts.ts` through the same schema, keeping its existing description. - Extend `tests/acceptance/tools/localToolContracts.test.ts` with one case per field so the shared harness covers the Frida, Rizin, and GDB tools. The approach reuses the existing schema owner rather than adding a new validation layer, which is how the earlier fixes in this class landed. The `.regex(/^[^\x00]*$/u)` pattern on `nulFreeStringSchema` survives `z.toJSONSchema`, so the constraint is advertised to clients and enforced at runtime from the same definition. GDB already rejected a NUL `inferior_tty` at runtime in `GdbSessionManager`, so its change is contract consistency rather than a behavior change; Frida and Rizin had no service-level NUL check for these fields. Intentionally not included: no changes to the path values themselves, no new absolute-path requirement (that would be a separate, breaking contract change), and no changes to any tool outside the Frida, Rizin, and GDB contracts. ## Contract and boundary impact - Semantic owner and earliest changed stage: the tool contracts in `src/contracts/` own input admission; the change moves the earliest NUL rejection to the advertised contract boundary. - Contribution scope fit and linked maintainer decision: ordinary fix within existing boundaries; not applicable, no new provider or capability. - CLI and MCP/tool-catalog contract: the affected tool input schemas now advertise a NUL-free pattern in addition to `minLength: 1`. The generated MCP tool catalog is regenerated by CI and validated; no committed catalog file changes. - Provider, bridge, target-format, or platform compatibility: none. Frida, Rizin, and GDB are bring-your-own providers; the change only rejects an already-invalid input earlier. - Evidence, artifact, provenance, or reconstruction contract: none. - Target selection, authority, upstream behavior, and cleanup details (if applicable): none. The change does not alter target selection, execution, or cleanup. - Generated metadata (`docs/public/product-catalog.json`), package, or installation impact (CI generates and validates ignored catalogs and `skills/`; edit authored instructions in `.agents/skills/reverse-engineer-anything/` and never commit build outputs): none. Expected effects (check known effects; use documentation-only on its own). Unchecked items do not establish absence; leave uncertainties visible. - [x] Read existing files or captured data. - [ ] Launch or attach to a process. - [ ] Inject instrumentation or run scripts/code. - [ ] Change process memory, registers, files, or configuration. - [ ] Contact network services. - [ ] Recover protected code or work around protections. - [ ] Documentation or usability only; no target interaction. - [ ] Not sure yet; investigation or maintainer guidance needed. ## Evidence and regression coverage - Tests added or updated: seven cases added to `tests/acceptance/tools/localToolContracts.test.ts`, which drives the real advertised MCP contracts through the built server and asserts each request is rejected with a message containing `NUL`. - Base reproduction or other evidence: on the pre-change revision the same harness fails on `start_frida_session`, showing the NUL input passed schema validation and reached the provider, where it surfaced as `capability_unavailable` with the message `Unable to find executable at '/tmp/selected'` (the value truncated at the NUL). After the change the harness passes and the response is a schema error naming the NUL byte. - User-visible CLI/MCP output (if applicable): an MCP call with a NUL path now returns a validation error at the contract boundary instead of a provider error. - Remaining proof gaps: the added cases exercise the shared acceptance harness through the built server; there is no separate per-provider live run for these specific rejected inputs, because the refusal happens before any provider contact. For evidence-bearing changes: - [x] Observed, derived, and inferred claims remain distinguishable. - [x] Artifact identity, source provenance, and failed attempts remain preserved. - [x] Unsupported, incomplete, unavailable, or uncertain outcomes remain visible. ## Validation performed - `npm run test:focused -- tests/acceptance/tools/localToolContracts.test.ts` — 1 passed (the OS-invalid-local-input matrix now includes the Frida, Rizin, and GDB cases); fails on the base revision for the intended reason. - `npm run test:focused -- src/contracts/fridaToolContracts.test.ts src/contracts/sessionPathInputs.test.ts src/contracts/advertisedInputExamples.test.ts tests/boundary/mcp/fridaLifecycleMcp.test.ts tests/boundary/mcp/rizinDebuggerCancellationMcp.test.ts tests/boundary/mcp/gdbCancellationMcp.test.ts` — 49 passed, 5 skipped. - `npm run test:focused -- src/contracts/compactInputSchema.test.ts src/contracts/toolInputReferences.test.ts src/contracts/toolOutputReferences.test.ts src/contracts/workflowInputContracts.test.ts` — 43 passed. - `npm run typecheck` — clean. - `npx oxlint` on the four changed files — 0 warnings, 0 errors. - `npx oxfmt --check` on the four changed files — clean. - `npm run verify:module-boundaries` — checked 1886 source files, clean. - `npm run mcp-catalog:check` — passed. - `npm run docs:check` — 5 passed. ## Compatibility, safety, and release - Breaking changes or migration steps: none. The change only rejects inputs that were never valid paths, so no correct caller is affected. - Real Hopper/Ghidra, browser, or OS coverage: not applicable; no provider engine behavior changes. Frida, Rizin, and GDB remain bring-your-own and are not started by these tests. - Package or release metadata impact: none. - Security, privacy, process, or containment review: this hardens input admission at the contract boundary for local path inputs. It does not expand any authority, and the refusal happens before any provider or filesystem effect. ## Author readiness Human owner: @DeryFerd - [x] I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work. - [x] I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps. - [x] I declare this PR ready for initial maintainer review. ## Before merge - [ ] This PR addresses a concrete problem or an agreed enhancement. - [ ] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. - [ ] 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. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
bsakaguchi 2026-10-31 01:09:47 +00:00
Sign in to join this conversation.
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#5834
No description provided.