fix: reject environment archives with linked roots or metadata - #238
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: needs real behavior proof before merge. Reviewed September 16, 2026, 12:01 AM ET / 04:01 UTC (Revision 2). ClawSweeper reviewWhat this changesThe PR validates environment archive roots and metadata before import or legacy snapshot restore, adds rejection and internal-symlink tests, and documents the restriction. Merge readiness⛔ Blocked before merge - 2 items remain The fix remains necessary on main and v0.2.47. No actionable patch defect was found; the earlier request for captured real CLI evidence remains unresolved. Priority: P2 Review scores
Verification
How this fits togetherOCM extracts environment archives into staging before importing an environment or restoring a legacy snapshot. Its shared archive reader supplies metadata and an environment directory to the subsequent copy operations. flowchart TD
A[Environment archive] --> B[Extract into staging]
B --> C[Check structural entry types]
C -->|Linked or wrong type| D[Reject archive]
C -->|Valid| E[Read metadata]
E --> F[Import environment]
E --> G[Restore legacy snapshot]
Before merge
Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Keep structural validation in the shared reader while preserving OCM-generated archives, legacy snapshots, and ordinary internal symlinks. Do we have a high-confidence way to reproduce the issue? Yes, source establishes the path: linked structural entries pass main's existence checks, allowing external metadata reads or import-root traversal. This review did not execute the CLI. Is this the best way to solve the issue? Yes, validating structural entries in the shared reader is a narrow repair that protects both consumers without changing the archive format or rejecting supported internal symlinks. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 88d514515751. 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 (1 earlier review cycle)
|
Validate structural archive entries before reading metadata or copying environment data. Keep ordinary symlinks within the extracted environment supported. Include CLI regressions for external root and metadata links and stabilize the daemon/rollback fixtures required by the full gate.
d8f4eb0 to
abccda8
Compare
What Problem This Solves
Importing an environment archive with a symlink at
root/could copy files from an external directory into the imported environment. Linkedmeta/ormeta/env.jsonentries could also make OCM read metadata outside the archive.User Impact
Import and legacy snapshot restore reject linked or incorrectly typed structural entries before reading metadata or copying environment contents. Normal symlinks inside an environment remain supported.
Why This Change Was Made
Tar extraction contains where links are created, but the archive consumer must validate what those links point to before following them. Check structural entries with
symlink_metadata, validating the metadata directory before its child. This fixes the shared reader used by import and legacy snapshot restore.Evidence
The unchanged built CLI accepted a synthetic archive pointing
root/at an external fixture and copied its sentinel file. Separate probes accepted externally linked metadata directories and files. The regression failed before the fix withaccepted linked root.The fixed built CLI rejects all three shapes, preserves external sentinel/metadata bytes, and creates no target environment. Import tests, the archive roundtrip/internal-symlink controls, and all 40 snapshot tests pass. Independent P0–P2 reviews found no actionable defects. Full local and exact-head CI results will be recorded after completion.
The full Rust suite passed on isolated Linux: 1,219 tests, zero failures.
cargo fmt --checkandcargo check --workspace --all-targets --lockedpassed. The final committed candidate is independently reviewed with no actionable P0–P2 finding. Exact-head cross-platform CI is the remaining merge gate.Rebased onto merged #149 at
88d5145; the shared fixture repairs are now on main and this PR changes only the archive reader, its regression tests, documentation and changelog. The only rebase conflict was the adjacent changelog entry; both entries are retained. Formatting, all-target checking, all 10 import tests and all nine archive unit tests pass after the rebase.All nine CI jobs and CodeQL checks passed on final head
abccda82cf46b32a9cf464c5ba0dfbf4bbdafe22: https://github.com/openclaw/ocm/actions/runs/35053726077.