Skip to content

fix(download): cap fetch_json decoded bytes - #117

Open
SebTardif wants to merge 1 commit into
openclaw:mainfrom
SebTardif:fix/f003-fetch-json-byte-cap
Open

fix(download): cap fetch_json decoded bytes#117
SebTardif wants to merge 1 commit into
openclaw:mainfrom
SebTardif:fix/f003-fetch-json-byte-cap

Conversation

@SebTardif

Copy link
Copy Markdown
Contributor

What Problem This Solves

Fixes an issue where users running ocm runtime releases, ocm runtime install --manifest-url, or an official npm packument fetch would hang or run the process out of memory when the URL returned a huge, streaming, or gzip-expanded JSON body.

download_to_file already rejects bodies over 512 MiB (#92). fetch_json and fetch_json_with_accept still request gzip and parse the decoded stream with serde_json::from_reader and no decoded-byte cap. A 65 KB gzip payload in this session expanded to 64 MiB plus 11 bytes of JSON.

Why This Change Was Made

Copy the decoded JSON through the existing copy_capped helper into a bounded buffer, then parse that buffer. JSON is kept in memory, so the cap is 64 MiB, which is above today's official openclaw npm packument (about 16 MiB) and far below the 512 MiB artifact cap.

User Impact

A runtime manifest or npm packument that expands past 64 MiB now fails with download exceeded 67108864 bytes instead of growing without bound. Official ocm runtime releases still lists published OpenClaw versions.

Evidence

terminal output from the patched ocm binary against a local gzip-expanded JSON URL, then against registry.npmjs.org:

A 65,265-byte gzip body decoded to 67,108,875 bytes (11 bytes past the 64 MiB cap). The CLI rejected it:

$ ./target/debug/ocm runtime releases --manifest-url http://127.0.0.1:18765/manifest.json
ocm: failed to download runtime URL "http://127.0.0.1:18765/manifest.json": download exceeded 67108864 bytes

The same binary still loaded the official catalog (248 releases) and the stable channel (2026.7.1-2).

$ ./target/debug/ocm runtime releases --json
count 248
sample ['2026.9.1-beta.1', '2026.8.1-beta.3', '2026.8.1-beta.2']

$ ./target/debug/ocm runtime releases --channel stable --json
count 1
first 2026.7.1-2

Real behavior proof

  • Behavior or issue addressed: fetch_json accepted unbounded decoded JSON (including gzip-expanded bodies) on ocm runtime releases and --manifest-url installs. Those commands now reject a body past 64 MiB decoded bytes.

  • Real environment tested: macOS Darwin 25.6.0 arm64, rustc 1.98.0, ocm 0.2.33 built from this branch at /tmp/oc-pr-ocm-F003. Isolated OCM_HOME under /tmp/ocm-f003-proof-ocm.

  • Exact steps or command run after this patch:

    Built ./target/debug/ocm. Served a gzip JSON body of 67,108,875 decoded bytes (65,265 bytes on the wire) from 127.0.0.1:18765. Then ran ocm runtime releases --manifest-url http://127.0.0.1:18765/manifest.json, then ocm runtime releases --json and ocm runtime releases --channel stable --json against registry.npmjs.org.

  • Evidence after fix: terminal output from the patched CLI:

    $ ./target/debug/ocm runtime releases --manifest-url http://127.0.0.1:18765/manifest.json
    ocm: failed to download runtime URL "http://127.0.0.1:18765/manifest.json": download exceeded 67108864 bytes
    
    $ ./target/debug/ocm runtime releases --json
    count 248
    sample ['2026.9.1-beta.1', '2026.8.1-beta.3', '2026.8.1-beta.2']
  • Observed result after fix: The gzip-expanded manifest URL exits 1 with the decoded-byte cap. The official npm catalog still returns 248 releases.

  • What was not tested: A live attacker-controlled HTTPS host on the public internet. ocm self update GitHub release JSON (that path does not use fetch_json).

Related: #92 added ureq timeouts and the download_to_file size cap. Gzip request wrapping landed in f0b7f2d. fetch_json itself dates to 768baf1. Same class of bound as rust-lang/cargo#11151 (unpacked crate size).

Copy gzip-decoded JSON through copy_capped into a 64 MiB buffer
before serde_json parse so a huge or gzip-expanded body fails closed.

Signed-off-by: Sebastien Tardif <[email protected]>
@clawsweeper

clawsweeper Bot commented Aug 29, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

@clawsweeper clawsweeper Bot added merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Aug 29, 2026
@clawsweeper

clawsweeper Bot commented Aug 29, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed August 29, 2026, 6:52 PM ET / 22:52 UTC.

ClawSweeper review

What this changes

The PR limits gzip-decoded runtime manifest and npm metadata JSON to 64 MiB before parsing, with a regression test for an oversized compressed response.

Merge readiness

⚠️ Ready for maintainer review - 3 items remain

Keep this PR open for normal maintainer review. It closes a source-proven unbounded decoded-JSON path and includes direct CLI proof; the remaining choice is whether the new 64 MiB hard limit is the intended compatibility policy for custom manifest and registry URLs.

Priority: P2
Reviewed head: dfb553d829d8be1427be09a222d76fbf9d57c389
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused, well-covered reliability fix with convincing real CLI proof; only the intentional custom-input compatibility boundary needs maintainer confirmation.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (terminal): The changed production owner is the shared decoded-JSON parser used by runtime manifests and npm metadata; the PR body records an after-fix CLI run against a local gzip-expanded response that rejected the oversized body, followed by successful retrieval of the real npm catalog.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The changed production owner is the shared decoded-JSON parser used by runtime manifests and npm metadata; the PR body records an after-fix CLI run against a local gzip-expanded response that rejected the oversized body, followed by successful retrieval of the real npm catalog.
Evidence reviewed 6 items Current-main gap: The PR base passes the decoded reader directly to serde_json with no decoded-byte bound, so the reported resource-exhaustion path remains on current main.
Introduced bounded parse: The PR buffers through the existing exact-cap helper before parsing, rejecting a decoded response once it exceeds 64 MiB.
Affected production callers: Runtime manifests and official npm release metadata both use the shared JSON fetch functions, including the compact npm metadata representation used during installation.
Findings None None.
Security None None.

How this fits together

OCM fetches runtime manifests and npm package metadata to list and install OpenClaw runtimes. This limit is applied after HTTP decoding and before JSON parsing, so oversized decoded responses fail before unbounded buffering.

flowchart LR
A[Runtime command] --> B[Manifest or npm URL]
B --> C[HTTP response decoding]
C --> D[64 MiB decoded limit]
D --> E[JSON parsing]
E --> F[Runtime selection or install]
Loading

Decision needed

Question Recommendation
Should OCM make 64 MiB a hard supported decoded-JSON maximum for every custom runtime manifest and npm metadata source? Adopt the hard limit: Treat 64 MiB as the supported maximum because it bounds memory use while remaining well above the currently cited official packument size.

Why: The change deliberately prevents unbounded memory use but rejects previously accepted well-formed custom inputs above the limit.

Before merge

  • Resolve merge risk (P1) - Existing custom --manifest-url or alternate registry inputs whose decoded JSON exceeds 64 MiB will change from parsing successfully to failing at runtime; maintainers should explicitly accept that compatibility boundary.
  • Complete next step (P2) - No mechanical repair is needed; a maintainer must accept or alter the deliberate compatibility boundary before merge.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Focused implementation and regression coverage production +8/-2, tests +23/-2, changelog +1 The branch is limited to the shared JSON download boundary, its focused gzip-expansion regression test, and release notes.

Merge-risk options

Maintainer options:

  1. Confirm the hard metadata limit (recommended)
    Accept the documented 64 MiB decoded maximum for custom runtime metadata to retain the bounded-memory guarantee.
  2. Preserve a trusted-source escape hatch
    Add a narrowly scoped, documented override only if maintainers must support existing custom metadata above the new limit.

Technical review

Best possible solution:

Adopt a documented 64 MiB decoded-JSON limit for runtime metadata, retaining the exact-bound behavior and clear size error for custom sources.

Do we have a high-confidence way to reproduce the issue?

Yes, at source level: the current base streams decoded data directly into serde_json without a byte limit, and the reviewed branch supplies after-fix CLI evidence for the gzip-expanded case.

Is this the best way to solve the issue?

Unclear until maintainers accept the compatibility policy; the bounded shared fetch path is the narrowest implementation, but the fixed 64 MiB maximum deliberately changes custom-input behavior.

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning high; reviewed against b5df48d9a8ad.

Labels

Label changes:

  • add P2: This is a bounded reliability fix for oversized runtime metadata responses with a limited compatibility surface.
  • add merge-risk: 🚨 compatibility: Custom manifest or registry JSON larger than 64 MiB will now fail rather than parse successfully.
  • add proof: sufficient: Contributor real behavior proof is sufficient. The changed production owner is the shared decoded-JSON parser used by runtime manifests and npm metadata; the PR body records an after-fix CLI run against a local gzip-expanded response that rejected the oversized body, followed by successful retrieval of the real npm catalog.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The changed production owner is the shared decoded-JSON parser used by runtime manifests and npm metadata; the PR body records an after-fix CLI run against a local gzip-expanded response that rejected the oversized body, followed by successful retrieval of the real npm catalog.

Label justifications:

  • P2: This is a bounded reliability fix for oversized runtime metadata responses with a limited compatibility surface.
  • merge-risk: 🚨 compatibility: Custom manifest or registry JSON larger than 64 MiB will now fail rather than parse successfully.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The changed production owner is the shared decoded-JSON parser used by runtime manifests and npm metadata; the PR body records an after-fix CLI run against a local gzip-expanded response that rejected the oversized body, followed by successful retrieval of the real npm catalog.
  • proof: sufficient: Contributor real behavior proof is sufficient. The changed production owner is the shared decoded-JSON parser used by runtime manifests and npm metadata; the PR body records an after-fix CLI run against a local gzip-expanded response that rejected the oversized body, followed by successful retrieval of the real npm catalog.

Evidence

What I checked:

  • Current-main gap: The PR base passes the decoded reader directly to serde_json with no decoded-byte bound, so the reported resource-exhaustion path remains on current main. (src/infra/download.rs:109, b5df48d9a8ad)
  • Introduced bounded parse: The PR buffers through the existing exact-cap helper before parsing, rejecting a decoded response once it exceeds 64 MiB. (src/infra/download.rs:114, dfb553d829d8)
  • Affected production callers: Runtime manifests and official npm release metadata both use the shared JSON fetch functions, including the compact npm metadata representation used during installation. (src/runtime/releases.rs:171, dfb553d829d8)
  • Regression coverage: The new test serves a gzip body whose decoded JSON exceeds the cap and asserts that fetch_json returns the size-exceeded error. (tests/download_tests.rs:160, dfb553d829d8)
  • Real CLI proof: The reviewed PR body records a patched CLI rejecting a 67,108,875-byte gzip-decoded local manifest while still loading the official npm catalog and stable channel. (dfb553d829d8)
  • Feature history: History shows the shared capped-copy helper arrived with the earlier merged download-hardening work, while gzip metadata handling predates it; this PR extends that established boundary to decoded JSON. (src/infra/download.rs:78, a101e103dea5)

Likely related people:

  • SebTardif: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • Vincent Koc: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Confirm that a hard 64 MiB decoded-JSON limit is the intended supported policy for existing custom metadata sources.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant