fix(cli): cp-style sandbox download and workspace-boundary check (#1353)

* fix(cli): cp-style sandbox download and workspace-boundary check

`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>

* fix(cli): canonicalise sandbox download path remotely and quote tar member

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>

* chore(cli): drop redundant tempfile dev-dependency entry

`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>

* test(cli): add E2E coverage for sandbox download paths

Adds four end-to-end tests for `openshell sandbox download` so the
happy path and both Codex regressions have explicit coverage:

* `sandbox_download_file_only` — seeds a single file via the new
  `SandboxGuard::exec` harness helper and pulls it back, exercising
  `sandbox_sync_down_file` without an upload phase.
* `sandbox_download_directory_only` — seeds a small directory tree and
  pulls it recursively, exercising `sandbox_sync_down_directory`.
* `sandbox_download_rejects_symlinks_pointing_outside_workspace` —
  regression for the high-severity finding. Plants three escape shapes
  (`/sandbox/etc-link -> /etc`, `/sandbox/passwd-link -> /etc/passwd`,
  and a path with a symlinked component `/sandbox/etc-link/passwd`) and
  asserts all three are refused with a workspace-boundary error before
  any tar streams. Also checks the host destination stays empty so a
  silent leak would surface.
* `sandbox_download_handles_dash_leading_basename` — regression for the
  medium-severity finding. Seeds a file named `--checkpoint-action=evil`
  and verifies it downloads with byte-for-byte contents, proving the
  `--` separator stops tar parsing the basename as an option.

The harness gains a single new method, `SandboxGuard::exec`, that wraps
`openshell sandbox exec --name <name> --no-tty -- <argv>`. Existing
tests preferred the upload path to populate sandbox state, but the
regression cases need to plant filesystem state the upload flow cannot
produce (symlinks, files whose basenames clap would otherwise reject).

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>

* docs(cli): note cp-style destination and workspace boundary on download

`openshell sandbox download` now refuses any source path that resolves
outside `/sandbox` (lexically or through a symlink), and the
single-file branch follows `cp`-style placement rules. Document both
so users hitting the workspace-boundary error or the
"land at exact path vs land inside dir" decision can self-serve.

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>

* test(cli): tolerate miette line-wrapping in symlink-rejection assertions

`sandbox_download_rejects_symlinks_pointing_outside_workspace` matched
the substring `"outside the sandbox workspace"` against the CLI's error
output. The check is correct in spirit but fragile in practice: miette
wraps long single-line errors across multiple lines at terminal width
and inserts a `│ ` continuation marker, so the substring breaks across
the wrap and the test fails even though the canonicalisation step
correctly refused the source.

Split the match into three short markers — `resolves to`, `outside the`,
`sandbox workspace` — each guaranteed to fit on one wrapped line. The
combined check is stricter than the original (it now requires evidence
that the remote `realpath -e` step actually ran, not just the boundary
refusal) without inheriting the wrap fragility.

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>

---------

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>

Author: laitingsheng

crates/openshell-cli/Cargo.toml

@@ -58,6 +58,7 @@ anyhow = { workspace = true }
 
 # File archiving (tar-over-SSH sync)
 tar = "0.4"
+tempfile = "3"
 
 # OIDC/Auth
 oauth2 = "5"
@@ -91,6 +92,5 @@ rcgen = { version = "0.13", features = ["crypto", "pem"] }
 reqwest = { workspace = true }
 serde_json = { workspace = true }
 temp-env = "0.3"
-tempfile = "3"
 tokio-stream = { workspace = true }
 url = { workspace = true }

crates/openshell-cli/src/main.rs

@@ -2488,12 +2488,8 @@ async fn main() -> Result<()> {
                     let ctx = resolve_gateway(&cli.gateway, &cli.gateway_endpoint)?;
                     let mut tls = tls.with_gateway_name(&ctx.name);
                     apply_auth(&mut tls, &ctx.name);
-                    let local_dest = std::path::Path::new(dest.as_deref().unwrap_or("."));
-                    eprintln!(
-                        "Downloading sandbox:{} -> {}",
-                        sandbox_path,
-                        local_dest.display()
-                    );
+                    let local_dest = dest.as_deref().unwrap_or(".");
+                    eprintln!("Downloading sandbox:{sandbox_path} -> {local_dest}");
                     run::sandbox_sync_down(&ctx.endpoint, &name, &sandbox_path, local_dest, &tls)
                         .await?;
                     eprintln!("{} Download complete", "✓".green().bold());

crates/openshell-cli/src/run.rs

@@ -2202,12 +2202,8 @@ pub async fn sandbox_sync_command(
             eprintln!("{} Sync complete", "✓".green().bold());
         }
         (None, Some(sandbox_path)) => {
-            let local_dest = Path::new(dest.unwrap_or("."));
-            eprintln!(
-                "Syncing sandbox:{} -> {}",
-                sandbox_path,
-                local_dest.display()
-            );
+            let local_dest = dest.unwrap_or(".");
+            eprintln!("Syncing sandbox:{sandbox_path} -> {local_dest}");
             sandbox_sync_down(server, name, sandbox_path, local_dest, tls).await?;
             eprintln!("{} Sync complete", "✓".green().bold());
         }