[PR #1899] [MERGED] fix(web): reject NUL bytes in web tool local path inputs #4893

Closed
opened 2026-10-31 00:56:30 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

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

Base: main ← Head: fix/web-tool-path-nul


📝 Commits (1)

  • 499ac18 fix(web): reject NUL bytes in web tool local path inputs

📊 Changes

7 files changed (+165 additions, -32 deletions)

View changed files

📝 src/domain/webModuleTrace.ts (+7 -8)
📝 src/domain/webNetworkCapture.ts (+4 -4)
📝 src/domain/webScriptArtifacts.ts (+4 -4)
📝 src/domain/webScriptExport.ts (+7 -12)
📝 src/domain/webSourceLocation.ts (+4 -4)
➕ src/domain/webToolPathSchemas.test.ts (+103 -0)
📝 tests/acceptance/tools/localToolContracts.test.ts (+36 -0)

📄 Description

Summary

Four web tools accepted caller-selected local paths as plain strings, so a NUL byte passed contract validation and reached the filesystem layer. The owning services only checked isAbsolute, and isAbsolute admits a NUL, so the byte was carried into a file operation and surfaced there as a raw ERR_INVALID_ARG_VALUE ("path must be a string... without null bytes") rather than as a named input constraint. Every other local-input tool in the repository already refuses NUL through the shared local-path schema, and the existing OS-invalid-local-input harness covers those tools but not these four. This PR routes the seven affected fields through the same shared schema so the refusal happens at the contract boundary, and extends the harness to the tools that were missing from it.

Problem and expected behavior

Smallest trigger: call inspect_web_network_capture with {"capture_path": "/tmp/selected\u0000filename", "format": "har"}, or any of the six sibling cases listed below. Prior behavior: the input schema accepted the value, the service's isAbsolute check passed, and the file read then threw a raw Node error instead of a typed AnalysisInputError, so the caller saw an internal error string rather than the shared "Local filesystem paths cannot contain NUL" constraint the other tools return. The violated invariant: a value the caller selects as a local filesystem path should be parsed once at the owning boundary, and NUL is never a valid filesystem path byte on any supported host, so it should be refused there. Expected behavior: all seven fields reject a NUL with the shared message, an ordinary absolute path is still accepted, and the accepted/rejected set for every other input is unchanged.

Change and scope

The seven caller-facing path fields now use localPathStringSchema instead of z.string().min(1): inspect_web_network_capture.capture_path (src/domain/webNetworkCapture.ts), export_web_scripts.capture_path and output_directory (src/domain/webScriptExport.ts), trace_web_source_location.manifest_path (shared selectedWebScriptInputSchema in src/domain/webScriptArtifacts.ts) and source_map.path (src/domain/webSourceLocation.ts), and trace_web_module_imports.manifest_path and import_map.path (src/domain/webModuleTrace.ts). Each field already required a nonempty string, so localPathStringSchema is a drop-in: the emitted pattern and minLength are identical and only the NUL refusal is added. Intentionally not included: output and derived path fields that REA builds internally from already-validated input are left as-is, no service-level isAbsolute check is removed, no URL field changes, and no tool is added or removed.

Contract and boundary impact

  • Semantic owner and earliest changed stage: domain input contracts for the four web tools, parsed at the tool-input boundary before any file is opened.
  • CLI and MCP/tool-catalog contract: the advertised JSON Schema for the seven fields is unchanged in shape; each keeps its existing minLength: 1 and gains the shared NUL-rejecting pattern already used by every other local-path tool. The catalog digest moves only because the contract moved, and no catalog file is committed.
  • Provider, bridge, target-format, or platform compatibility: none; the schema is provider-neutral and the NUL rule is host-independent.
  • Evidence, artifact, provenance, or reconstruction contract: none. Accepted inputs produce byte-identical Evidence; only inputs that were never valid on any host are now refused earlier.
  • Process execution, authorization, cleanup, or containment impact: none; this is schema validation and one harness extension.
  • Generated metadata (docs/public/product-catalog.json), package, or installation impact (CI generates and validates ignored catalogs and skills/; edit authored instructions in skill-src/ and never commit build outputs): none committed. .cache/mcp-tool-catalog.json is regenerated test metadata and is not part of the repository.

Evidence and regression coverage

  • Tests added or updated: a new src/domain/webToolPathSchemas.test.ts rejects a NUL in all seven fields and accepts ordinary absolute paths as a control; tests/acceptance/tools/localToolContracts.test.ts adds the four tools to its OS-invalid-local-input harness so they are exercised through the real advertised MCP contracts.
  • Base reproduction or other evidence: on the base revision the new schema case fails for all seven fields because each accepted a NUL, and the harness cases fail because the tools did not reject it; after the change both pass.
  • User-visible CLI/MCP output: a request carrying a NUL local path now fails at the boundary with the shared "Local filesystem paths cannot contain NUL" message instead of a raw filesystem error.
  • Remaining proof gaps: no real provider or browser run applies to this schema-only change; the behavior is fully exercised by the schema and MCP boundary suites listed below.

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

  • npx vitest run src/domain/webToolPathSchemas.test.ts — 8 passed, including the seven NUL-rejection cases and the accepted-path control.
  • npx vitest run tests/acceptance/tools/localToolContracts.test.ts — passed, including the four newly added tools through the real advertised MCP contracts.
  • npx vitest run tests/boundary/mcp/toolSchemaValidity.test.ts — 8 passed after npm run mcp-catalog:generate.
  • npx vitest run tests/boundary/filesystem/webScriptExport.test.ts — 2 failures that reproduce identically with this change stashed (a symlink EPERM and a private-runtime-root-authority-unavailable case on this Windows host), so they are environmental and pre-existing.
  • npm run typecheck — exit 0.
  • npx oxlint on the changed files — 0 warnings, 0 errors; oxfmt --check on the committed (LF) form — clean.
  • Real Hopper/Ghidra, browser, or OS lanes: not applicable; no provider, bridge, or platform-specific code is touched.

Compatibility, safety, and release

  • Breaking changes or migration steps: none for valid callers. A NUL path was already invalid on every host, so it now fails earlier with a named constraint; all previously accepted valid paths still parse identically and the advertised schema keeps the same shape.
  • Real Hopper/Ghidra, browser, or OS coverage: not applicable; the schema and MCP boundary suites above cover the changed contracts.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: the change closes a small boundary inconsistency where an OS-invalid byte could reach a filesystem call and surface as an untyped internal error; it adds no network, process, or authority surface.

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/1899 **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/web-tool-path-nul` --- ### 📝 Commits (1) - [`499ac18`](https://github.com/morluto/rea/commit/499ac180115de7c9de3f1adc55159ffa6e9bb40e) fix(web): reject NUL bytes in web tool local path inputs ### 📊 Changes **7 files changed** (+165 additions, -32 deletions) <details> <summary>View changed files</summary> 📝 `src/domain/webModuleTrace.ts` (+7 -8) 📝 `src/domain/webNetworkCapture.ts` (+4 -4) 📝 `src/domain/webScriptArtifacts.ts` (+4 -4) 📝 `src/domain/webScriptExport.ts` (+7 -12) 📝 `src/domain/webSourceLocation.ts` (+4 -4) ➕ `src/domain/webToolPathSchemas.test.ts` (+103 -0) 📝 `tests/acceptance/tools/localToolContracts.test.ts` (+36 -0) </details> ### 📄 Description ## Summary Four web tools accepted caller-selected local paths as plain strings, so a NUL byte passed contract validation and reached the filesystem layer. The owning services only checked `isAbsolute`, and `isAbsolute` admits a NUL, so the byte was carried into a file operation and surfaced there as a raw `ERR_INVALID_ARG_VALUE` ("path must be a string... without null bytes") rather than as a named input constraint. Every other local-input tool in the repository already refuses NUL through the shared local-path schema, and the existing OS-invalid-local-input harness covers those tools but not these four. This PR routes the seven affected fields through the same shared schema so the refusal happens at the contract boundary, and extends the harness to the tools that were missing from it. ## Problem and expected behavior Smallest trigger: call `inspect_web_network_capture` with `{"capture_path": "/tmp/selected\u0000filename", "format": "har"}`, or any of the six sibling cases listed below. Prior behavior: the input schema accepted the value, the service's `isAbsolute` check passed, and the file read then threw a raw Node error instead of a typed `AnalysisInputError`, so the caller saw an internal error string rather than the shared "Local filesystem paths cannot contain NUL" constraint the other tools return. The violated invariant: a value the caller selects as a local filesystem path should be parsed once at the owning boundary, and NUL is never a valid filesystem path byte on any supported host, so it should be refused there. Expected behavior: all seven fields reject a NUL with the shared message, an ordinary absolute path is still accepted, and the accepted/rejected set for every other input is unchanged. ## Change and scope The seven caller-facing path fields now use `localPathStringSchema` instead of `z.string().min(1)`: `inspect_web_network_capture.capture_path` (`src/domain/webNetworkCapture.ts`), `export_web_scripts.capture_path` and `output_directory` (`src/domain/webScriptExport.ts`), `trace_web_source_location.manifest_path` (shared `selectedWebScriptInputSchema` in `src/domain/webScriptArtifacts.ts`) and `source_map.path` (`src/domain/webSourceLocation.ts`), and `trace_web_module_imports.manifest_path` and `import_map.path` (`src/domain/webModuleTrace.ts`). Each field already required a nonempty string, so `localPathStringSchema` is a drop-in: the emitted `pattern` and `minLength` are identical and only the NUL refusal is added. Intentionally not included: output and derived path fields that REA builds internally from already-validated input are left as-is, no service-level `isAbsolute` check is removed, no URL field changes, and no tool is added or removed. ## Contract and boundary impact - Semantic owner and earliest changed stage: domain input contracts for the four web tools, parsed at the tool-input boundary before any file is opened. - CLI and MCP/tool-catalog contract: the advertised JSON Schema for the seven fields is unchanged in shape; each keeps its existing `minLength: 1` and gains the shared NUL-rejecting `pattern` already used by every other local-path tool. The catalog digest moves only because the contract moved, and no catalog file is committed. - Provider, bridge, target-format, or platform compatibility: none; the schema is provider-neutral and the NUL rule is host-independent. - Evidence, artifact, provenance, or reconstruction contract: none. Accepted inputs produce byte-identical Evidence; only inputs that were never valid on any host are now refused earlier. - Process execution, authorization, cleanup, or containment impact: none; this is schema validation and one harness extension. - Generated metadata (`docs/public/product-catalog.json`), package, or installation impact (CI generates and validates ignored catalogs and `skills/`; edit authored instructions in `skill-src/` and never commit build outputs): none committed. `.cache/mcp-tool-catalog.json` is regenerated test metadata and is not part of the repository. ## Evidence and regression coverage - Tests added or updated: a new `src/domain/webToolPathSchemas.test.ts` rejects a NUL in all seven fields and accepts ordinary absolute paths as a control; `tests/acceptance/tools/localToolContracts.test.ts` adds the four tools to its OS-invalid-local-input harness so they are exercised through the real advertised MCP contracts. - Base reproduction or other evidence: on the base revision the new schema case fails for all seven fields because each accepted a NUL, and the harness cases fail because the tools did not reject it; after the change both pass. - User-visible CLI/MCP output: a request carrying a NUL local path now fails at the boundary with the shared "Local filesystem paths cannot contain NUL" message instead of a raw filesystem error. - Remaining proof gaps: no real provider or browser run applies to this schema-only change; the behavior is fully exercised by the schema and MCP boundary suites listed below. 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 - `npx vitest run src/domain/webToolPathSchemas.test.ts` — 8 passed, including the seven NUL-rejection cases and the accepted-path control. - `npx vitest run tests/acceptance/tools/localToolContracts.test.ts` — passed, including the four newly added tools through the real advertised MCP contracts. - `npx vitest run tests/boundary/mcp/toolSchemaValidity.test.ts` — 8 passed after `npm run mcp-catalog:generate`. - `npx vitest run tests/boundary/filesystem/webScriptExport.test.ts` — 2 failures that reproduce identically with this change stashed (a symlink `EPERM` and a `private-runtime-root-authority-unavailable` case on this Windows host), so they are environmental and pre-existing. - `npm run typecheck` — exit 0. - `npx oxlint` on the changed files — 0 warnings, 0 errors; `oxfmt --check` on the committed (LF) form — clean. - Real Hopper/Ghidra, browser, or OS lanes: not applicable; no provider, bridge, or platform-specific code is touched. ## Compatibility, safety, and release - Breaking changes or migration steps: none for valid callers. A NUL path was already invalid on every host, so it now fails earlier with a named constraint; all previously accepted valid paths still parse identically and the advertised schema keeps the same shape. - Real Hopper/Ghidra, browser, or OS coverage: not applicable; the schema and MCP boundary suites above cover the changed contracts. - Package or release metadata impact: none. - Security, privacy, process, or containment review: the change closes a small boundary inconsistency where an OS-invalid byte could reach a filesystem call and surface as an untyped internal error; it adds no network, process, or authority surface. ## 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:56:30 +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#4893
No description provided.