Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: blocked before merge. Reviewed September 11, 2026, 12:30 PM ET / 16:30 UTC (Revision 6). ClawSweeper reviewWhat this changesLimits downloaded runtime manifests and npm metadata to 64 MiB after decompression and adds an oversized-gzip regression test. Merge readiness⛔ Blocked before merge - 4 items remain This remains useful work: main and v0.2.44 still parse downloaded JSON without a decoded-byte limit. The implementation and supplied runtime proof are convincing; the previously identified custom-source compatibility decision remains unresolved. Priority: P2 Review scores
Verification
How this fits togetherOCM downloads release metadata to list available OpenClaw versions and select runtime installations or updates. Its shared JSON reader decompresses HTTP responses before passing metadata to release validation. flowchart TD
A[Release listing or runtime installation] --> B[Official registry or custom manifest]
B --> C[HTTP reader and gzip decoding]
C --> D{Decoded body within 64 MiB?}
D -->|Yes| E[Parse and validate release metadata]
D -->|No| F[Return download error]
E --> G[List releases or select runtime]
Decision needed
Why: The limit is intentional and well above the demonstrated official catalog size, but accepting a new failure mode for persisted custom sources requires repository-owner judgment. Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Use the shared decoded-byte bound with a maintainer-approved, documented custom-source policy and verified preservation of installed runtimes on rejection. Do we have a high-confidence way to reproduce the issue? Yes, source establishes the unbounded decoded JSON path on main, and an oversized gzip response provides a concrete trigger. No current-main execution was performed in this read-only review. Is this the best way to solve the issue? Yes for the mechanism: reusing the capped copier after decompression is narrow and maintainable. Applying the same fixed threshold to existing custom sources still needs explicit compatibility acceptance. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against c5ac5392e142. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (5 earlier review cycles)
|
dfb553d to
a87cfb4
Compare
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
a87cfb4 to
9675e51
Compare
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)
9675e51 to
f8f83cd
Compare
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>
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_filealready rejects bodies over 512 MiB (#92).fetch_jsonandfetch_json_with_acceptstill request gzip and parse the decoded stream withserde_json::from_readerand 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_cappedhelper into a bounded buffer, then parse that buffer. JSON is kept in memory, so the cap is 64 MiB, which is above today's officialopenclawnpm 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 bytesinstead of growing without bound. Officialocm runtime releasesstill lists published OpenClaw versions.Evidence
terminal output from the patched
ocmbinary 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:
The same binary still loaded the official catalog (248 releases) and the stable channel (
2026.7.1-2).Real behavior proof
Behavior or issue addressed:
fetch_jsonaccepted unbounded decoded JSON (including gzip-expanded bodies) onocm runtime releasesand--manifest-urlinstalls. 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. IsolatedOCM_HOMEunder/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) from127.0.0.1:18765. Then ranocm runtime releases --manifest-url http://127.0.0.1:18765/manifest.json, thenocm runtime releases --jsonandocm runtime releases --channel stable --jsonagainst registry.npmjs.org.Evidence after fix: terminal output from the patched CLI:
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 updateGitHub release JSON (that path does not usefetch_json).Related: #92 added ureq timeouts and the
download_to_filesize cap. Gzip request wrapping landed inf0b7f2d.fetch_jsonitself dates to768baf1. Same class of bound as rust-lang/cargo#11151 (unpacked crate size).