fix(rest): extract uploaded tar before CopyInto in runner file upload by G4614 · Pull Request #688 · boxlite-ai/boxlite

G4614 · 2026-06-09T04:39:15Z

copy_in saves the uploaded tar as a binary blob instead of extracting

Bug

PUT /v1/{prefix}/boxes/{id}/files (the runner handler for SDK copy_in)
silently broke both single-file and directory uploads:

copy_in("host.txt", "/root/hello.txt")
  → /root/hello.txt = raw tar bytes (ustar header + payload), not the file
copy_in("host_dir/", "/root/flatdest/")
  → /root/flatdest/boxlite-upload-XXXXXX.tar = the directory as one big tar

Root cause: the SDK Rust REST client serialises the source as a tar archive
in the body. The runner buffered it to <tmp>/boxlite-upload-*.tar and
called Boxlite.CopyInto(tarPath, dest), which treats src as a literal
filesystem path — so the tar archive itself was the "file" that landed in
the guest.

Fix

Extract the uploaded tar at the runner before calling CopyInto:

Archive shape	Behaviour
1 regular file, no other entries	write at `dest` (single-file copy_in)
Multiple entries / dirs / symlinks / specials	`unpack` into `dest` (directory copy_in)
Any entry with `..` in path	reject (zip-slip guard)

Test plan — two-sided verified

Pin: scripts/test/e2e/cases/test_files_io.py

Stack: local e2e. Runner binary swapped between main and PR builds.

Case	Pre-fix (main)	Post-fix (this PR)
`test_copy_in_text_roundtrips_byte_exact` (host writes `"üñîçødé\n"`, copy_in, guest cat)	`cat` returned raw tar archive text + ustar header (`'hello.txt…ustar…<payload>'`) — file at expected path didn't exist	byte-exact payload at `/root/hello.txt`
`test_copy_in_directory_include_parent_false` (copy_in dir with nested files; `find -type f`)	only `/root/flatdest/boxlite-upload-XXXXXX.tar` (the raw tar landed as one file)	both `top.txt` and `sub/deep.txt` under `/root/flatdest/`
`test_copy_in_overwrite_false_rejects_conflict`	empty	empty — runner-side extract always uses `O_TRUNC`. `overwrite=False` policy not honoured on REST yet
`test_copy_out_binary_roundtrips_sha256`	`IsADirectoryError` on host_dest	`IsADirectoryError` on host_dest — SDK Rust copy_out has the mirror bug (raw tar saved as dir)

This PR closes the two known-broken copy_in paths (single file + directory). The two residual failures are different bugs:

overwrite=False needs the tar-extract path to respect the option; minor follow-up (5 lines in extract_tar).
copy_out needs the SAME fix on the SDK side (the SDK currently saves the runner's tar response without extracting). That's a Rust-SDK PR (fix(rest): copy_out single-file mode writes at host_dst, not inside it #692), scope of a separate change.

Summary by CodeRabbit

Improvements
- Enhanced file upload handling to support efficient transfers of both individual files and directory structures.
Tests
- Added comprehensive end-to-end tests for file operations, verifying data integrity, text preservation, binary round-tripping, recursive directory handling, and overwrite behavior.

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

coderabbitai · 2026-06-09T04:39:23Z

📝 Walkthrough

Walkthrough

This PR refactors tar file upload handling to stream-extract into a temporary staging directory on the runner, then intelligently routes to either a single file or the full directory to CopyInto. A new helper function adds path traversal protection and file-mode preservation. Comprehensive E2E tests validate byte-exact preservation of UTF-8 text, binary blob round-tripping via SHA-256, recursive directory copying, and overwrite conflict behavior.

Changes

File upload refactoring and E2E validation

Layer / File(s)	Summary
Tar streaming extraction and upload handler `apps/runner/pkg/api/controllers/boxlite_files.go`	`BoxliteFileUpload` refactored to stream-extract uploaded tar into a staging directory. New `extractTarToDir` helper processes tar entries with absolute-path and traversal rejection, mode preservation, and single-file detection to route between file and directory mode.
File I/O round-trip and conflict E2E tests `scripts/test/e2e/cases/test_files_io.py`	New test module validates copy_in/copy_out pipeline: UTF-8 text byte preservation, 256KB binary SHA-256 round-trip, recursive directory flattening with `include_parent=False`, and `overwrite=false` conflict safety.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A tar stream unpacked without a temp file in sight,

Path checks stand guard against escapes in the night,

Single files chosen, or directories roam free,

Four tests confirm the bytes flow perfectly! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 71.43% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main fix: extracting uploaded tar before passing to CopyInto in the runner's file upload handler. It clearly identifies the core change and aligns with the primary objective of fixing the tar handling bug.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

The fix in the preceding commit closes the gap this test exercises. Mirrors the layout of `scripts/test/e2e/cases/` on main.

test_exec_user.py → boxlite-ai#686, test_network_allow_net.py → boxlite-ai#687, test_files_io.py → boxlite-ai#688, test_box_metrics.py → boxlite-ai#689, test_snapshot_clone.py → boxlite-ai#694, test_images_pull_list.py → boxlite-ai#696. Remaining 3 cases (test_exec_attach.py, test_volume_readonly.py, test_cli_detach_recovery.py) stay here because they pin REST-path gaps that don't have a matching fix PR in this session — they document the contract for future work to land against.

coderabbitai

🧹 Nitpick comments (1)

apps/runner/pkg/api/controllers/boxlite_files.go (1)

94-98: ⚡ Quick win

Consider strengthening path traversal protection with containment verification.

The current approach (checking if cleanName is absolute, equals "..", or starts with "../") should catch typical zip-slip attacks after filepath.Clean normalization. However, a more robust and direct approach would verify that the final target path remains under destDir:

cleanName := filepath.Clean(header.Name)
if filepath.IsAbs(cleanName) {
    return "", false, fmt.Errorf("tar entry has absolute path: %q", header.Name)
}
target := filepath.Join(destDir, cleanName)
// Verify target is under destDir
absTarget, err := filepath.Abs(target)
if err != nil {
    return "", false, fmt.Errorf("resolve target path: %w", err)
}
absDestDir, err := filepath.Abs(destDir)
if err != nil {
    return "", false, fmt.Errorf("resolve dest dir: %w", err)
}
if !strings.HasPrefix(absTarget, absDestDir + string(filepath.Separator)) {
    return "", false, fmt.Errorf("tar entry escapes staging dir: %q", header.Name)
}

This explicitly confirms containment after path construction rather than inferring it from the relative path structure.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/runner/pkg/api/controllers/boxlite_files.go` around lines 94 - 98, The
current check on cleanName can miss cases; after building target :=
filepath.Join(destDir, cleanName) resolve both absTarget := filepath.Abs(target)
and absDestDir := filepath.Abs(destDir) (handling errors) and then verify
containment by ensuring absTarget has absDestDir + string(filepath.Separator) as
a prefix; if not, return the same escape error for header.Name — update the code
paths around cleanName, target, header.Name and destDir to perform this
containment verification.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@apps/runner/pkg/api/controllers/boxlite_files.go`:
- Around line 94-98: The current check on cleanName can miss cases; after
building target := filepath.Join(destDir, cleanName) resolve both absTarget :=
filepath.Abs(target) and absDestDir := filepath.Abs(destDir) (handling errors)
and then verify containment by ensuring absTarget has absDestDir +
string(filepath.Separator) as a prefix; if not, return the same escape error for
header.Name — update the code paths around cleanName, target, header.Name and
destDir to perform this containment verification.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: d86f4143-8968-4345-8bf1-864508dcc428

📥 Commits

Reviewing files that changed from the base of the PR and between e4bbd0e and df4fed9.

📒 Files selected for processing (2)

apps/runner/pkg/api/controllers/boxlite_files.go
scripts/test/e2e/cases/test_files_io.py

#692) copy_out single-file mode writes a directory wrapping the file instead of the file at host_dst ## Bug `box.copy_out("/guest/blob", "/host/blob_copy")` over REST landed at the wrong layout on the host: ``` expected: /host/blob_copy = the file actual: /host/blob_copy/blob = the file inside a new directory ``` Local-FFI `copy_out` keeps single-file semantics (dest is the file path); REST broke that contract. Root cause: the SDK Rust REST client used `tar::Archive::unpack(host_dst)` unconditionally. `unpack` always treats its arg as a destination directory and extracts every entry under it. For a single-file archive that means writing the entry as a child of `host_dst` rather than AT `host_dst`. ## Fix Detect single-file archives and write the entry directly at `host_dst`: | Archive shape | Behaviour | |---|---| | 1 regular file, no other entries | write at `host_dst` | | Multiple / dirs / symlinks / specials | `unpack(host_dst)` as before | Mirror of #688's runner-side single-file detection — same situation, opposite direction. ## Test plan — two-sided verified Pin: `scripts/test/e2e/cases/test_files_io.py::test_copy_out_binary_roundtrips_sha256` Stack: local e2e. C lib + Python wheel rebuilt; no runner change needed (this is SDK-side only). | | Pre-fix | Post-fix | |---|---|---| | `copy_out('/root/blob', '/host/blob_copy')` then `read_bytes('/host/blob_copy')` | `IsADirectoryError: '/host/blob_copy'` (host wrote a directory wrapping the file) | **bytes match guest sha256** | | `copy_out('/root/dir/', '/host/dir/')` (multi-entry archive) | unchanged | unchanged (falls through to `archive.unpack`) | Defensive: the single-file path is gated on `file_count == 1 && other_count == 0`, so any directory/symlink/link entry demotes to the multi-entry path. Zero-file archives go through the multi-entry path too (does nothing, same as before).  ## Summary by CodeRabbit * **Bug Fixes** * Tar extraction now preserves single-file archives by writing the single file directly to the intended destination (creating its parent directory if needed), while multi-entry or non-regular archives continue to unpack into a destination directory.

#710) add 5 e2e cases pinning REST contracts not covered today ## Coverage gaps Three classes of behaviour run through the SDK → API → Runner → VM chain but had no e2e pin on the chain itself; bugs in any one of them would silently regress through `make test:integration:*` (which only exercises local-FFI): ``` 1. CLI detach lifecycle — `boxlite run -d` returns, CLI process exits, does a fresh CLI process still see / exec the box? FFI side src/boxlite/tests/detach.rs, recovery.rs ✓ E2E side scripts/test/e2e/cases/ ✗ 2. Execution.attach / reattach contract — bogus exec_id should be a typed client error; reattach to a completed exec should return a usable handle. Runner side apps/runner/.../boxlite_exec_attach_test.go ✓ SDK <-> API end-to-end ✗ 3. host bind mount via REST — the cloud runtime intentionally dropped host bind mounts (#639); REST callers passing host paths must silently no-op, no /mnt/<x> in the guest. FFI surface src/boxlite/tests/mount_security.rs ✓ REST contract ✗ ``` ## Cases shipped ``` scripts/test/e2e/cases/test_cli_detach_recovery.py (2 cases) scripts/test/e2e/cases/test_exec_attach.py (2 cases) scripts/test/e2e/cases/test_volume_readonly.py (1 case) ``` 5 cases total. Author also dropped 6 cases from this branch's earlier incarnation that were already committed alongside their respective fix PRs (#686 / #688 / #689 / #691 / #692 / #696), so the diff is strictly net-new coverage. ## Test plan — run against current main Stack: local e2e, runner unchanged, no source edits. | Case | Result | Notes | |---|---|---| | `test_detached_box_exec_propagates_exit_code_on_fresh_cli` | ✅ PASS | exit-code passthrough across CLI processes | | `test_detached_box_survives_cli_exit_and_is_reusable` | ⚠️ XFAIL (strict) | reaches step 3 (`boxlite exec <id> echo still-alive`) then hits the stdout-drop race that #563 fixes — marker drops when #563 lands | | `test_attach_with_bogus_id_is_typed_error` | ✅ PASS | bogus exec_id → typed `Exception` (not 5xx, not silent) | | `test_reattach_after_original_completes` | ⚠️ XFAIL (strict) | same stdout-drop race (#563) on the original exec's `out=='first-output'` assertion | | `test_host_bind_mount_via_rest_is_silently_ignored` | ✅ PASS | the box created with `volumes=[(host_dir, "/mnt/ro", True)]` reports `MOUNT_LINE=<none>` from `/proc/mounts` and the host marker file is untouched — REST silently dropped the host path | The 2 XFAILs are tied to #563 (`fix(go-sdk): fold stream drain into Execution.Wait`). Once #563 merges, both xfails flip xpass-strict, the markers come off, and the suite is 5/5 green. No additional fix work is needed in this PR. 🤖 Generated with [Claude Code](https://claude.com/claude-code)  ## Summary by CodeRabbit * **Tests** * Added end-to-end tests verifying CLI detach survival and box reusability across fresh CLI processes, including exit code propagation tests. * Added end-to-end tests for SDK reattach functionality, validating session state after execution completion. * Added end-to-end test confirming proper host bind mount handling during REST execution.  --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

This was referenced Jun 9, 2026

fix(rest): copy_out single-file mode writes at host_dst, not inside it #692

Merged

fix(rest): honour copy_in(overwrite=False) end-to-end #691

Open

test(e2e): add test_files_io.py regression pin

df4fed9

The fix in the preceding commit closes the gap this test exercises. Mirrors the layout of `scripts/test/e2e/cases/` on main.

G4614 marked this pull request as ready for review June 9, 2026 12:10

G4614 requested a review from a team as a code owner June 9, 2026 12:10

G4614 enabled auto-merge June 9, 2026 12:10

coderabbitai Bot reviewed Jun 9, 2026

View reviewed changes

G4614 mentioned this pull request Jun 9, 2026

test(e2e): add cli-detach-recovery, exec-attach, volume-readonly cases #710

Merged

DorianZheng approved these changes Jun 9, 2026

View reviewed changes

DorianZheng disabled auto-merge June 9, 2026 13:46

DorianZheng merged commit 56bba3e into boxlite-ai:main Jun 9, 2026
20 of 21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(rest): extract uploaded tar before CopyInto in runner file upload#688

fix(rest): extract uploaded tar before CopyInto in runner file upload#688
DorianZheng merged 2 commits into
boxlite-ai:mainfrom
G4614:fix/rest-copy-in-tar-extract

G4614 commented Jun 9, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jun 9, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Poem

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

G4614 commented Jun 9, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bug

Fix

Test plan — two-sided verified

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jun 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

G4614 commented Jun 9, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jun 9, 2026 •

edited

Loading