Skip to content

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

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

SebTardif wants to merge 2 commits 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).

@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 review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@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: blocked before merge. Reviewed October 2, 2026, 9:47 PM ET / October 3, 2026, 01:47 UTC (Revision 7).

ClawSweeper review

What this changes

Caps downloaded release manifests and npm metadata at 64 MiB after decompression and adds an oversized-gzip regression test.

Merge readiness

⛔ Blocked before merge - 4 items remain

This remains useful: current main and v0.2.48 still parse downloaded JSON without a decoded-byte cap. The patch has convincing runtime proof and no concrete correctness findings, but the previously raised custom-source compatibility decision remains unresolved.

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

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused patch with direct production-path proof and no correctness findings; the compatibility choice and targeted upgrade validation remain.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (terminal): The captured macOS CLI transcript exercises the shared JSON reader through real HTTP and gzip decoding, showing oversized-manifest rejection and successful official catalog loading. Installed-runtime rejection coverage remains a separate upgrade concern; no stored-data format changes.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The captured macOS CLI transcript exercises the shared JSON reader through real HTTP and gzip decoding, showing oversized-manifest rejection and successful official catalog loading. Installed-runtime rejection coverage remains a separate upgrade concern; no stored-data format changes.
Evidence reviewed 10 items Pinned introduced change: The verified main-to-head delta changes only the shared JSON parser and download tests. Both JSON entrypoints use the capped copier after gzip decoding; artifact download behavior is unchanged.
Still necessary on main: Current main calls serde_json::from_reader directly. Its existing 512 MiB bound applies to artifact downloads rather than decoded JSON.
Latest release comparison: The supplied latest release, v0.2.48, also parses JSON without this decoded-byte bound.
Findings None None.
Security None None.

How this fits together

OCM downloads release metadata to list OpenClaw versions and select runtime installations or updates. Its shared JSON reader decompresses HTTP responses before release validation.

flowchart TD
  A[Release listing or runtime update] --> B[Registry or custom manifest]
  B --> C[HTTP and gzip decoding]
  C --> D{Within decoded size limit?}
  D -->|Yes| E[Parse and validate metadata]
  D -->|No| F[Return download error]
  E --> G[List or select runtime]
Loading

Decision needed

Question Recommendation
Should the 64 MiB decoded limit reject previously usable custom runtime manifests and alternate metadata sources? Approve the universal bound: Accept the 64 MiB cutoff with documented upgrade impact and proof that rejection preserves installed runtime state.

Why: The resource bound is well motivated, but choosing whether existing large custom sources lose update support requires repository-owner intent.

Before merge

  • Resolve merge risk (P1) - Existing custom manifests or alternate registry metadata exceeding 64 MiB decoded will stop loading after upgrade; maintainer acceptance of this new cutoff is still unrecorded.
  • Resolve merge risk (P1) - The supplied CLI proof covers listing and rejection, but does not verify that oversized-source rejection preserves an installed runtime's files and metadata.
  • Complete next step (P2) - Record the custom-source size-policy decision, document the chosen upgrade behavior, and verify oversized-source rejection preserves an installed runtime.
  • Resolve maintainer decision - Resolve the maintainer decision shown above before merge.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test delta Production +8/-2 (net +6); tests +23/-2 (net +21) The justified production growth reuses the existing capped copier and adds one focused gzip regression.

Merge-risk options

Maintainer options:

  1. Approve and verify the cutoff (recommended)
    Record acceptance of the custom-source cutoff, document it, and demonstrate oversized-source rejection with an existing installed runtime.
  2. Define bounded compatibility
    Pause landing while maintainers choose a bounded path that preserves supported large custom sources.

Technical review

Best possible solution:

Keep a bounded shared JSON reader with an explicit custom-source upgrade policy; if large existing sources remain supported, provide a bounded compatibility path and verify safe rejection.

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

Yes: main's unbounded decoded JSON reader and an oversized gzip response establish a concrete trigger. Current-main execution was not performed in this read-only review.

Is this the best way to solve the issue?

Yes for the mechanism: capping decoded bytes before parsing is a narrow repair. The universal custom-source threshold still requires compatibility acceptance and installed-state validation.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against eb48770b06f4.

Labels

Label changes:

No label changes.

Label justifications:

  • P2: This is a focused resource-limit fix without evidence of an active widespread outage.
  • merge-risk: 🚨 compatibility: The unconditional threshold rejects previously accepted custom metadata when persisted runtime sources are reloaded.
  • 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 captured macOS CLI transcript exercises the shared JSON reader through real HTTP and gzip decoding, showing oversized-manifest rejection and successful official catalog loading. Installed-runtime rejection coverage remains a separate upgrade concern; no stored-data format changes.
  • proof: sufficient: Contributor real behavior proof is sufficient. The captured macOS CLI transcript exercises the shared JSON reader through real HTTP and gzip decoding, showing oversized-manifest rejection and successful official catalog loading. Installed-runtime rejection coverage remains a separate upgrade concern; no stored-data format changes.

Evidence

What I checked:

  • Pinned introduced change: The verified main-to-head delta changes only the shared JSON parser and download tests. Both JSON entrypoints use the capped copier after gzip decoding; artifact download behavior is unchanged. (src/infra/download.rs:175, 1b9e6e41b601)
  • Still necessary on main: Current main calls serde_json::from_reader directly. Its existing 512 MiB bound applies to artifact downloads rather than decoded JSON. (src/infra/download.rs:172, eb48770b06f4)
  • Latest release comparison: The supplied latest release, v0.2.48, also parses JSON without this decoded-byte bound. (src/infra/download.rs:172, 6c563651c7f0)
  • Real production-path proof: The captured body, sourceRevision 80a4b8f94424fb8597f1bebf56c161f7ca65382215ebe020663d0aacfdd84356, reports a patched macOS CLI rejecting 67,108,875 decoded bytes delivered through a real local HTTP gzip response. The same binary loaded 248 official releases and selected the stable channel. The exact proof build SHA is unspecified.
  • Existing-source upgrade boundary: Runtime updates reload the persisted source_manifest_url. Previously accepted custom sources exceeding 64 MiB will therefore fail after upgrading OCM. Resolution precedes runtime preparation and publication, supporting safe rejection; supplied runtime proof does not exercise this installed-state scenario. (src/runtime/install.rs:488, 1b9e6e41b601)
  • Fresh-install failure ordering: Custom-manifest fetching and selection occur before the runtime installation owner is invoked, so the new cap rejects oversized metadata before installation begins. (src/store/runtimes.rs:2664, 1b9e6e41b601)

Likely related people:

  • shakkernerd: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • SebTardif: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • vincentkoc: 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.

  • Resolve and document the custom-source size policy, then verify oversized-source rejection preserves an installed runtime's files and metadata.

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.

History

Review history (6 earlier review cycles)
  • reviewed 2026-08-29T22:52:41.238Z sha dfb553d :: needs maintainer review before merge. :: none
  • reviewed 2026-09-05T00:02:01.965Z sha dfb553d :: blocked before merge. :: none
  • reviewed 2026-09-07T14:22:40.714Z sha a87cfb4 :: blocked before merge. :: none
  • reviewed 2026-09-07T14:25:59.721Z sha a87cfb4 :: blocked before merge. :: none
  • reviewed 2026-09-09T01:40:28.027Z sha 9675e51 :: blocked before merge. :: none
  • reviewed 2026-09-11T16:30:44.394Z sha f8f83cd :: blocked before merge. :: none

@SebTardif
SebTardif force-pushed the fix/f003-fetch-json-byte-cap branch from dfb553d to a87cfb4 Compare September 7, 2026 14:18
@SebTardif

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Sep 7, 2026 •

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event exact_review_queue).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

@SebTardif
SebTardif force-pushed the fix/f003-fetch-json-byte-cap branch from a87cfb4 to 9675e51 Compare September 9, 2026 01:36
Layer copy_capped on parse_json_reader so gzip-decoded JSON cannot
exceed MAX_JSON_BYTES. Replay onto current main after leftover macOS
reds went green there.

Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
(cherry picked from commit 9675e51)
@SebTardif
SebTardif force-pushed the fix/f003-fetch-json-byte-cap branch from 9675e51 to f8f83cd Compare September 11, 2026 16:26
SebTardif added a commit to SebTardif/ocm that referenced this pull request Sep 11, 2026
dev_stop_acknowledgement_refuses_live_recorded_ownership failed once
on macos-latest; the same test passed on openclaw#117 and openclaw#136 from the same
main. This PR does not touch that test.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
The branch was behind main. Main CI is green, including the macOS
npm fixture fix from openclaw#216. This merge keeps the 64 MiB JSON cap.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
steipete added a commit that referenced this pull request Oct 7, 2026
Bound decoded JSON before parsing and preserve installed runtime state when
an oversized update manifest is rejected. Add exact-limit and installed-state
regressions, document the custom-source compatibility boundary, and record
previously landed round-eight fixes in Unreleased.

Complete the approach proposed in #117.

Co-authored-by: Sebastien Tardif <sebtardif@ncf.ca>

This branch has not been deployed

No deployments
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