fix(rest): honour copy_out(overwrite=False) on the SDK side#698
Draft
G4614 wants to merge 4 commits into
Draft
Conversation
The SDK Rust REST client sends `copy_in` as a tar archive (Content-Type:
application/x-tar) over PUT /boxes/{id}/files. The runner controller
was saving the body to `<tmp>/boxlite-upload-*.tar` and then handing
that path straight to `Boxlite.CopyInto`, which treats `src` as a real
filesystem path. End result: the guest got a single binary file
containing the raw tar header + payload at the destination path —
silently breaking both single-file and directory uploads.
e.g. `box.copy_in("/host/hello.txt", "/root/hello.txt")` →
guest cat /root/hello.txt prints
`hello.txt<NUL>0100664…ustar…<payload>` instead of the payload.
Fix: stream the request body through a `tar.Reader` into a staging dir,
then call CopyInto with the extracted path. Two output shapes:
- single regular file in archive → CopyInto(<extracted-file>, dest)
so the guest sees the file at dest exactly as the caller requested.
- anything else (multi-file, directories, symlinks)
→ CopyInto(<staging-dir>, dest), letting the Go SDK's recursive
copy handle the tree.
Hardening: rejects entries with absolute paths or `../` components
(zip-slip). Single-file detection is pessimistic — any non-regular
entry (dir, symlink, link, device) demotes the upload to the
"copy a directory" path.
E2E regression test pinned:
sdks/python/tests/e2e/test_files_io.py::test_copy_in_text_roundtrips_byte_exact
sdks/python/tests/e2e/test_files_io.py::test_copy_in_directory_include_parent_false
Before this change `box.copy_in(host, dst, copy_options=CopyOptions(
overwrite=False))` over REST silently clobbered existing guest files —
the option was dropped at three seams between the SDK and libboxlite:
1. SDK Rust `copy_into` ignored `_opts: CopyOptions` entirely;
2. Runner upload handler had no way to tell the Go SDK to refuse
to overwrite; its `r.Boxlite.CopyInto` took no options;
3. The C FFI `boxlite_copy_into` hard-coded `overwrite=true` in
`default_copy_options()`, so even if the Go SDK could pass an
override, the C side wouldn't honour it.
Plumbed end to end:
- C FFI : adds `boxlite_copy_into_with_options(handle, src, dst,
overwrite, cb, user_data, err)` next to the existing
`boxlite_copy_into` (kept for ABI continuity). Both call
the new `box_copy_into` internal that now takes
`CopyOptions` as an argument.
- Go SDK: adds `CopyIntoOptions{Overwrite bool}` and a
`CopyIntoWithOptions` method. `CopyInto` becomes a thin
wrapper that forwards `Overwrite: true` to keep existing
callers byte-compatible.
- Runner: client's `CopyIntoWithOptions(...)` and the file-upload
handler reads `overwrite=false` from the query string
(set by the SDK Rust REST client below).
- SDK Rust REST: `copy_into` puts `&overwrite=false` on the URL
when CopyOptions says so. Default omits the param so
servers built against the old contract behave unchanged.
E2E regression test:
sdks/python/tests/e2e/test_files_io.py::test_copy_in_overwrite_false_rejects_conflict
Symmetric to PR boxlite-ai#691 (`copy_in` overwrite=False), but for the receive side. Pre-fix `_opts` was ignored — the SDK silently overwrote existing host files even when the caller passed overwrite=False. Unlike copy_in (which had to plumb the option down through the C FFI to libboxlite, because the destination lives in the guest), copy_out's destination is a host path the SDK writes itself. So this check lives entirely in `src/boxlite/src/rest/litebox.rs::copy_out` — no runner, no C FFI, no Go SDK change required. Behaviour: - opts.overwrite=true (default): unchanged - opts.overwrite=false AND host_dst exists: returns `BoxliteError::AlreadyExists("copy_out refused: host destination {} already exists and overwrite=false")` BEFORE issuing the GET to the runner — saves a tar download whose payload would be discarded anyway. - opts.overwrite=false AND host_dst absent: pass-through E2E regression tests: scripts/test/e2e/cases/test_copy_out_overwrite.py ::test_copy_out_overwrite_false_raises_already_exists PRE: Internal "failed to extract tar to <dest>" (because the pre-existing extract path treats a file as a directory and unpacks into it — a tangential failure, not the overwrite contract) POST: typed "already exists" error, host content unchanged ::test_copy_out_overwrite_false_does_not_refuse_when_dest_absent Pin against the inverted-guard regression where a "refuse-when-absent" mis-implementation would still pass the primary test above. Branched off fix/rest-copy-in-overwrite-false (PR boxlite-ai#691) — same template, opposite direction.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Pre-merge fmt sweep against the two files CI's `fmt:check:rust` flagged: `box_copy_into` call wrap in `sdks/c/src/copy.rs`, and a `format!` string wrap in `rest/litebox.rs::copy_out`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Symmetric to PR #691 (`copy_in` overwrite=False), but for the receive side. Pre-fix `_opts` was ignored — the SDK silently overwrote existing host files even when the caller passed `overwrite=False`.
Unlike copy_in (which had to plumb the option down through the C FFI to libboxlite because the destination lives in the guest), copy_out's destination is a host path the SDK writes itself. So this check lives entirely in `src/boxlite/src/rest/litebox.rs::copy_out` — no runner, no C FFI, no Go SDK change required.
Test plan — two-sided verified
Pin: `scripts/test/e2e/cases/test_copy_out_overwrite.py`
Logically depends on #691's CopyOptions plumbing pattern landing.