[PR #1472] [CLOSED] fix(json): refuse evidence JSON that cannot decode before reading it #2910

Closed
opened 2026-10-17 21:11:43 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/1472
Author: @DeryFerd
Created: 10/10/2026
Status: ❌ Closed

Base: main ← Head: fix/json-undecodable-input-admission


📝 Commits (1)

  • 549b68b fix(json): refuse evidence JSON that cannot decode before reading it

📊 Changes

2 files changed (+64 additions, -4 deletions)

View changed files

📝 src/application/JsonFiles.ts (+33 -3)
📝 tests/boundary/filesystem/jsonUtf8Input.test.ts (+31 -1)

📄 Description

Summary

readJsonFile backs evidence bundle imports and analysis snapshot loads, and it used to pull the caller-selected file into memory before anything checked whether the file could ever be accepted. This PR adds an admission check on the stat that the file handle already provides: an input larger than three times the runtime string limit cannot decode under any content, so it is refused immediately with the same typed resource_constraint error the decode path already produces, now carrying the failed bound as max_decodable_input_bytes. No issue is linked because this is a standalone boundary hardening found while reading the JSON input path, not a report-driven fix.

Problem and expected behavior

Smallest trigger: call readJsonFile on a regular file whose size exceeds 3 * constants.MAX_STRING_LENGTH, roughly 1.5 GiB with Node's current ~512 MiB string limit. Prior behavior: the whole file was read into a Buffer first, and only then did parseUtf8Json chunk-decode it and raise AnalysisResourceConstraintError once the decoded length crossed the limit. The violated invariant: a caller should learn that input can never be accepted without the host first paying for every byte of it. On a machine that cannot hold the file, the process can die inside readFile() and the typed error, with its path, byte count, and remediation, never appears. Expected behavior: refuse from the admission stat, report the observed size and both limits, and never read the file's bytes.

Change and scope

The check lives in readJsonFile (src/application/JsonFiles.ts), which now goes through withRegularFile so the size is stat-ed on the same open handle that performs the read; a file that grows after admission still falls back to the existing decode classification, so the refusal can never be the only guard. The bound is 3 * constants.MAX_STRING_LENGTH because UTF-8's smallest ratio is one UTF-16 code unit per three bytes (three-byte BMP sequences), which means anything larger cannot decode within the runtime string limit no matter its content, and anything smaller cannot be rejected on size alone without refusing inputs that work today, including multibyte-heavy files that pass through the chunked decode. Intentionally not included: the CLI streaming reader (readCliJsonFile) already reads in 64 KiB chunks and needs no admission, no configuration knob is added, and no other read path changes.

Contract and boundary impact

  • Semantic owner and earliest changed stage: application layer, readJsonFile in src/application/JsonFiles.ts, before UTF-8 decode; domain schemas untouched.
  • CLI and MCP/tool-catalog contract: no schema change. The error keeps the existing resource_constraint projection, its details record is an open string-to-JSON map, and no tool is added or removed, so the generated catalog does not move.
  • Provider, bridge, target-format, or platform compatibility: none. The path is provider-neutral, and the stat-on-open-handle check behaves the same on POSIX and Windows.
  • Evidence, artifact, provenance, or reconstruction contract: accepted inputs produce byte-identical evidence to before; the refusal reports observed input_bytes from the stat plus the derived max_decodable_input_bytes, keeping observed and derived values distinguishable, and reuses the existing remediation text for evidence workflows.
  • Process execution, authorization, cleanup, or containment impact: peak resident memory drops for oversized caller-selected inputs because their bytes are never read; the handle is closed by the existing withRegularFile finally path.
  • 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. No contract, catalog, or skill input changed, and no build output is committed.

Evidence and regression coverage

  • Tests added or updated: one case in tests/boundary/filesystem/jsonUtf8Input.test.ts creates a sparse file of 3 * MAX_STRING_LENGTH + 1 bytes and asserts the typed refusal with operation: "read_evidence_file", resource: "memory", observed input_bytes, max_string_code_units, and max_decodable_input_bytes.
  • Base reproduction or other evidence: on the base revision the same file loads the full buffer and reaches the chunked decode loop before reporting a constraint that lacks max_decodable_input_bytes; with this change the case finishes in about 50 ms because only the stat is consulted. The new test fails on the base revision for exactly the missing bound.
  • User-visible CLI/MCP output: resource_constraint details for refused oversized inputs now carry max_decodable_input_bytes alongside the existing fields; inputs below the bound are unchanged.
  • Remaining proof gaps: a file that is both over the bound and invalid UTF-8 now reports the size constraint instead of the UTF-8 failure, since the bytes are no longer decoded to tell the two apart, and both outcomes refuse the file. No real-provider run applies to this provider-neutral path and none was attempted.

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 tests/boundary/filesystem/jsonUtf8Input.test.ts — 3 passed, including the new admission case.
  • npx vitest run tests/boundary/cli/jsonInputSize.test.ts — 3 passed; this suite pins the whole-document string-limit classifications for both readers.
  • npx vitest run tests/boundary/filesystem/evidenceBundleFiles.test.ts tests/boundary/filesystem/cliJsonInput.test.ts — 34 passed, 4 skipped, 2 failed; both failures are EPERM inside the tests' own symlink() fixture setup on Windows, before any application read code runs.
  • npm run typecheck — exit 0 on the rebased branch.
  • npx oxlint on the two changed files — 0 warnings, 0 errors; oxfmt --check on the committed (LF) form — clean.
  • Real Hopper/Ghidra, browser, or OS lanes: not run, because no provider, bridge, or platform-specific code is touched.

Compatibility, safety, and release

  • Breaking changes or migration steps: none. Inputs that succeeded before still succeed with identical bytes, and inputs that failed before still fail with the same code, resource, and remediation; only files that could never be accepted lose the wasteful read.
  • Real Hopper/Ghidra, browser, or OS coverage: not applicable; the Windows-host boundary suites listed above cover the changed path.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: the change shrinks the memory-exhaustion surface for hostile or accidental multi-gigabyte selections, adds no network, process, or authority surface, and does not persist or transmit file content.

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/1472 **Author:** [@DeryFerd](https://github.com/DeryFerd) **Created:** 10/10/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/json-undecodable-input-admission` --- ### 📝 Commits (1) - [`549b68b`](https://github.com/morluto/rea/commit/549b68b8b61bfb4797c87588d7ce0a71abd9409f) fix(json): refuse evidence JSON that cannot decode before reading it ### 📊 Changes **2 files changed** (+64 additions, -4 deletions) <details> <summary>View changed files</summary> 📝 `src/application/JsonFiles.ts` (+33 -3) 📝 `tests/boundary/filesystem/jsonUtf8Input.test.ts` (+31 -1) </details> ### 📄 Description ## Summary `readJsonFile` backs evidence bundle imports and analysis snapshot loads, and it used to pull the caller-selected file into memory before anything checked whether the file could ever be accepted. This PR adds an admission check on the stat that the file handle already provides: an input larger than three times the runtime string limit cannot decode under any content, so it is refused immediately with the same typed `resource_constraint` error the decode path already produces, now carrying the failed bound as `max_decodable_input_bytes`. No issue is linked because this is a standalone boundary hardening found while reading the JSON input path, not a report-driven fix. ## Problem and expected behavior Smallest trigger: call `readJsonFile` on a regular file whose size exceeds `3 * constants.MAX_STRING_LENGTH`, roughly 1.5 GiB with Node's current ~512 MiB string limit. Prior behavior: the whole file was read into a `Buffer` first, and only then did `parseUtf8Json` chunk-decode it and raise `AnalysisResourceConstraintError` once the decoded length crossed the limit. The violated invariant: a caller should learn that input can never be accepted without the host first paying for every byte of it. On a machine that cannot hold the file, the process can die inside `readFile()` and the typed error, with its path, byte count, and remediation, never appears. Expected behavior: refuse from the admission stat, report the observed size and both limits, and never read the file's bytes. ## Change and scope The check lives in `readJsonFile` (`src/application/JsonFiles.ts`), which now goes through `withRegularFile` so the size is stat-ed on the same open handle that performs the read; a file that grows after admission still falls back to the existing decode classification, so the refusal can never be the only guard. The bound is `3 * constants.MAX_STRING_LENGTH` because UTF-8's smallest ratio is one UTF-16 code unit per three bytes (three-byte BMP sequences), which means anything larger cannot decode within the runtime string limit no matter its content, and anything smaller cannot be rejected on size alone without refusing inputs that work today, including multibyte-heavy files that pass through the chunked decode. Intentionally not included: the CLI streaming reader (`readCliJsonFile`) already reads in 64 KiB chunks and needs no admission, no configuration knob is added, and no other read path changes. ## Contract and boundary impact - Semantic owner and earliest changed stage: application layer, `readJsonFile` in `src/application/JsonFiles.ts`, before UTF-8 decode; domain schemas untouched. - CLI and MCP/tool-catalog contract: no schema change. The error keeps the existing `resource_constraint` projection, its `details` record is an open string-to-JSON map, and no tool is added or removed, so the generated catalog does not move. - Provider, bridge, target-format, or platform compatibility: none. The path is provider-neutral, and the stat-on-open-handle check behaves the same on POSIX and Windows. - Evidence, artifact, provenance, or reconstruction contract: accepted inputs produce byte-identical evidence to before; the refusal reports observed `input_bytes` from the stat plus the derived `max_decodable_input_bytes`, keeping observed and derived values distinguishable, and reuses the existing remediation text for evidence workflows. - Process execution, authorization, cleanup, or containment impact: peak resident memory drops for oversized caller-selected inputs because their bytes are never read; the handle is closed by the existing `withRegularFile` finally path. - 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. No contract, catalog, or skill input changed, and no build output is committed. ## Evidence and regression coverage - Tests added or updated: one case in `tests/boundary/filesystem/jsonUtf8Input.test.ts` creates a sparse file of `3 * MAX_STRING_LENGTH + 1` bytes and asserts the typed refusal with `operation: "read_evidence_file"`, `resource: "memory"`, observed `input_bytes`, `max_string_code_units`, and `max_decodable_input_bytes`. - Base reproduction or other evidence: on the base revision the same file loads the full buffer and reaches the chunked decode loop before reporting a constraint that lacks `max_decodable_input_bytes`; with this change the case finishes in about 50 ms because only the stat is consulted. The new test fails on the base revision for exactly the missing bound. - User-visible CLI/MCP output: `resource_constraint` details for refused oversized inputs now carry `max_decodable_input_bytes` alongside the existing fields; inputs below the bound are unchanged. - Remaining proof gaps: a file that is both over the bound and invalid UTF-8 now reports the size constraint instead of the UTF-8 failure, since the bytes are no longer decoded to tell the two apart, and both outcomes refuse the file. No real-provider run applies to this provider-neutral path and none was attempted. 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 tests/boundary/filesystem/jsonUtf8Input.test.ts` — 3 passed, including the new admission case. - `npx vitest run tests/boundary/cli/jsonInputSize.test.ts` — 3 passed; this suite pins the whole-document string-limit classifications for both readers. - `npx vitest run tests/boundary/filesystem/evidenceBundleFiles.test.ts tests/boundary/filesystem/cliJsonInput.test.ts` — 34 passed, 4 skipped, 2 failed; both failures are `EPERM` inside the tests' own `symlink()` fixture setup on Windows, before any application read code runs. - `npm run typecheck` — exit 0 on the rebased branch. - `npx oxlint` on the two changed files — 0 warnings, 0 errors; `oxfmt --check` on the committed (LF) form — clean. - Real Hopper/Ghidra, browser, or OS lanes: not run, because no provider, bridge, or platform-specific code is touched. ## Compatibility, safety, and release - Breaking changes or migration steps: none. Inputs that succeeded before still succeed with identical bytes, and inputs that failed before still fail with the same code, resource, and remediation; only files that could never be accepted lose the wasteful read. - Real Hopper/Ghidra, browser, or OS coverage: not applicable; the Windows-host boundary suites listed above cover the changed path. - Package or release metadata impact: none. - Security, privacy, process, or containment review: the change shrinks the memory-exhaustion surface for hostile or accidental multi-gigabyte selections, adds no network, process, or authority surface, and does not persist or transmit file content. ## 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-17 21:11:43 +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#2910
No description provided.