fix(cli): cp-style sandbox download and workspace-boundary check#1353
Open
laitingsheng wants to merge 3 commits into
Open
fix(cli): cp-style sandbox download and workspace-boundary check#1353laitingsheng wants to merge 3 commits into
laitingsheng wants to merge 3 commits into
Conversation
`openshell sandbox download` produced a directory at the host destination instead of writing the source bytes as a regular file, and did not refuse sandbox-side paths that resolved outside `/sandbox` (only `creds.json` was rejected, by filesystem policy rather than by an explicit check). Mirrors the cp-style design from the upload-side fixes (#667 / #694 and #885 / #952): destination resolution now respects trailing slashes and existing-directory destinations, single-file sources land as regular files via a staged extraction and atomic rename, and the function delegates the SSH tar stream to a shared helper. Adds an explicit lexical workspace-boundary check on the sandbox-side source — required on the download side because filesystem policy alone is not a substitute for an explicit check when crossing the trust boundary in the leak direction. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
…ember The lexical check in `validate_sandbox_source_path` clears any path whose components stay under `/sandbox` after `.` and `..` are folded out, but it cannot see symlinks. A workspace-relative symlink such as `/sandbox/etc-link -> /etc` lets `openshell sandbox download <name> /sandbox/etc-link/passwd /tmp/passwd` slip through, then `tar -C /sandbox/etc-link passwd` follows the link on the sandbox side and streams `/etc/passwd` back to the host. Add `canonicalize_sandbox_source_path` which runs `realpath -e --` over SSH and rejects the result unless it still equals `/sandbox` or starts with `/sandbox/`. The lexical guard stays as a cheap fail-fast for obviously-bad inputs, and the rest of the download flow now uses the canonical path everywhere a tar invocation or probe might dereference a symlink. While editing the single-file branch, also add the `--` separator before the basename in `tar cf - -C <parent> -- <name>`. Without it, a sandbox file whose basename starts with `-` (e.g. `--checkpoint-action=...`) is parsed by GNU tar as an option rather than a member, which at best fails the wrap and at worst triggers tar option behaviour. The directory branch passes literal `.` and is unaffected. Cover both fixes with pure-function tests: `is_under_sandbox_workspace` gets accept/reject pairs (the same predicate the post-realpath validator uses) and the new `build_single_file_tar_cmd` helper asserts the `--` separator and shell escaping survive any future refactor. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
`tempfile` was promoted from `[dev-dependencies]` to `[dependencies]` once the cp-style single-file download path started using `TempDir::new_in` from production code. The original `[dev-dependencies]` line stayed behind by accident — entries under `[dependencies]` are already available to test compilation, so the second declaration is dead weight. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
laitingsheng
requested review from
a team,
derekwaynecarr,
maxamillion and
mrunalp
as code owners
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.
Summary
openshell sandbox downloadproduced a directory at the host destination instead of writing the source bytes as a regular file, and did not refuse sandbox-side paths that resolved outside/sandbox. This brings the download path to parity with the upload-side cp-style fix shipped in #694 (#667) and #952 (#885), and adds the explicit workspace-boundary check that the upload side never needed.Related Issue
Fixes NVIDIA/NemoClaw#3345
Changes
sandbox_sync_downnow validates the sandbox-side source path with a lexical workspace-boundary check before issuing any SSH command. Paths that resolve outside/sandbox(e.g./etc/passwd,/sandbox/../etc/passwd,/sandboxed/secrets) are refused with a clear error.cp-style destination semantics:dest(ordestalready exists as a directory) → file lands at<dest>/<source-basename>.destis taken as the exact file path.dest(with an early refusal whendestexists as a non-directory).stream_sandbox_tarhelper, mirroring the upload-sidessh_tar_uploadrefactor from fix(cli): sandbox upload overwrites files instead of creating directories #694.sandbox_sync_downtakesdest: &strso trailing slashes survive (previously the value was converted to&Pathat the dispatch site, which dropped them).Testing
mise run pre-commitpassesChecklist