Skip to content

fix: reject environment archives with linked roots or metadata - #238

Merged
steipete merged 1 commit into
mainfrom
fix/archive-structure-links
Sep 16, 2026
Merged

steipete merged 1 commit into
mainfrom
fix/archive-structure-links

Conversation

@steipete

@steipete steipete commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

What Problem This Solves

Importing an environment archive with a symlink at root/ could copy files from an external directory into the imported environment. Linked meta/ or meta/env.json entries 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 with accepted 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 --check and cargo check --workspace --all-targets --locked passed. 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.

@clawsweeper

clawsweeper Bot commented Sep 16, 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 P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Sep 16, 2026
@clawsweeper

clawsweeper Bot commented Sep 16, 2026 •

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed September 16, 2026, 12:01 AM ET / 04:01 UTC (Revision 2).

ClawSweeper review

What this changes

The 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
Reviewed head: abccda82cf46b32a9cf464c5ba0dfbf4bbdafe22

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The patch is focused and source-correct, but its reported runtime results still need inspectable evidence.
Proof confidence 🦪 silver shellfish (2/6) Needs stronger real behavior proof before merge: The body reports that built-CLI import probes exercised the shared reader and rejected all three linked structures without creating a target, but provides no captured output or artifact demonstrating those results. The earlier request for inspectable evidence remains outstanding. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Needs proof Needs stronger real behavior proof before merge: The body reports that built-CLI import probes exercised the shared reader and rejected all three linked structures without creating a target, but provides no captured output or artifact demonstrating those results. The earlier request for inspectable evidence remains outstanding. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 9 items Main still permits linked structure: The pinned main reader checks exists(), which follows symlinks, before reading metadata and returning the root directory.
Latest release also needs the guard: The v0.2.47 archive reader contains the same existence-only checks; the tag peels to b7e2802.
Guard precedes metadata reads: The introduced guard uses symlink_metadata, checks meta before its child, and validates all three structural entries before read_to_string.
Findings None None.
Security None None.

How this fits together

OCM 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]
Loading

Before merge

  • Add real behavior proof - Needs stronger real behavior proof before merge: The body reports that built-CLI import probes exercised the shared reader and rejected all three linked structures without creating a target, but provides no captured output or artifact demonstrating those results. The earlier request for inspectable evidence remains outstanding. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Complete next step (P2) - Attach captured evidence from the reported after-fix CLI probes and supported archive control. Terminal screenshots or recordings are preferred when useful; copied output and logs also count. Redact credentials, private paths, IP addresses, and endpoints. Updating the PR body should trigger a fresh review automatically; otherwise ask a maintainer to comment @clawsweeper re-review.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test growth Production net +24 lines; tests +100 lines Production growth implements the shared structural guard, with most additions devoted to rejection and compatibility coverage.

Technical review

Best 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.

Labels

Label justifications:

  • P2: This repairs a bounded filesystem-read defect triggered by importing or restoring a crafted environment archive.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The body reports that built-CLI import probes exercised the shared reader and rejected all three linked structures without creating a target, but provides no captured output or artifact demonstrating those results. The earlier request for inspectable evidence remains outstanding. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

  • Main still permits linked structure: The pinned main reader checks exists(), which follows symlinks, before reading metadata and returning the root directory. (src/infra/archive.rs:377, 88d514515751)
  • Latest release also needs the guard: The v0.2.47 archive reader contains the same existence-only checks; the tag peels to b7e2802. (src/infra/archive.rs:377, b7e2802d9ae0)
  • Guard precedes metadata reads: The introduced guard uses symlink_metadata, checks meta before its child, and validates all three structural entries before read_to_string. (src/infra/archive.rs:379, abccda82cf46)
  • Import and restore share the repaired boundary: Import calls the reader at src/store/envs.rs:1006 before traversing and copying the extracted root. Legacy snapshot restore calls it at src/store/snapshots.rs:647 before constructing the candidate tree. (src/store/snapshots.rs:647, abccda82cf46)
  • Supported archive compatibility: The writer emits regular metadata and a directory root. Existing export/import and legacy-tar restore tests exercise supported archives, while the new archive control preserves an internal symlink. No stored schema, dependency, or version changes are introduced. (tests/env_snapshot_tests.rs:1076, abccda82cf46)
  • Focused rejection coverage: The new isolated CLI regression covers linked root, meta, and meta/env.json entries, requiring rejection, no target environment, and unchanged external fixture bytes. These tests were inspected, not executed during this read-only review. (tests/env_import_tests.rs:11, abccda82cf46)

Likely related people:

  • unknown: The claimed source-line change could not be verified from bounded local history. (role: source history unknown; confidence: low)

Rank-up moves

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

  • Attach redacted terminal output or a recording from the reported after-fix CLI probes, including rejected structural links and a successful supported archive control.

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 (1 earlier review cycle)
  • reviewed 2026-09-16T03:48:45.287Z sha d8f4eb0 :: needs real behavior proof before merge. :: none

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.
@steipete
steipete force-pushed the fix/archive-structure-links branch from d8f4eb0 to abccda8 Compare September 16, 2026 03:57
@steipete
steipete merged commit b1557f6 into main Sep 16, 2026
15 checks passed
@steipete
steipete deleted the fix/archive-structure-links branch September 16, 2026 04:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant