Repository navigation
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 October 2, 2026, 9:47 PM ET / October 3, 2026, 01:47 UTC (Revision 7). ClawSweeper reviewWhat this changesCaps 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 Review scores
Verification
How this fits togetherOCM 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]
Decision needed
Why: The resource bound is well motivated, but choosing whether existing large custom sources lose update support requires repository-owner intent. Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest 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. LabelsLabel changes: No label changes. Label 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 (6 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>
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>
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>
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).