diff --git a/docs/architecture.md b/docs/architecture.md index 509a73932a..0dd5317746 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -526,7 +526,7 @@ GitHub event ──► SHIM WORKFLOW (fullsend.yml in enrolled repo) ║ │ │ OPENSHELL SANDBOX │ │ ║ ║ │ │ │ │ ║ ║ │ │ Created with --from image, --policy code.yaml. │ │ ║ - ║ │ │ Bootstrapped via SCP/SSH: │ │ ║ + ║ │ │ Bootstrapped via openshell upload/exec: │ │ ║ ║ │ │ agent def → /tmp/claude-config/agents/ │ │ ║ ║ │ │ skills → /tmp/claude-config/skills/ │ │ ║ ║ │ │ .env, host files (GCP creds), security hooks │ │ ║ diff --git a/docs/superpowers/plans/2026-05-06-openshell-native-sandbox-transport.md b/docs/superpowers/plans/2026-05-06-openshell-native-sandbox-transport.md new file mode 100644 index 0000000000..f2d89f0e58 --- /dev/null +++ b/docs/superpowers/plans/2026-05-06-openshell-native-sandbox-transport.md @@ -0,0 +1,952 @@ +# OpenShell Native Sandbox Transport Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Replace SSH/SCP/rsync `exec.Command` wrappers in `internal/sandbox/` with OpenShell native CLI commands and `os.Root` containment for local writes. + +**Architecture:** The sandbox package's public API changes from `SSH(sshConfigPath, sandboxName, ...)` to `Exec(sandboxName, ...)` — the `sshConfigPath` parameter is removed from all functions. Transport uses `openshell sandbox exec/upload/download` instead of `ssh`/`scp`/`rsync`. Local write containment uses `os.Root` (Go 1.24+). A post-download `sanitizeDownload` function removes symlinks and `.git/hooks/` to replace `rsync --no-links --exclude .git/hooks/`. + +**Tech Stack:** Go 1.26, OpenShell CLI, `os.Root` (stdlib) + +--- + +## File Structure + +| File | Role | +|---|---| +| `internal/sandbox/sandbox.go` | Replace SSH/SCP/rsync functions with OpenShell native equivalents; add `sanitizeDownload`; update `ExtractTranscripts`/`ExtractOutputFiles` to use `os.Root` | +| `internal/sandbox/sandbox_test.go` | Tests for `sanitizeDownload`, `os.Root` containment, updated path traversal tests | +| `internal/cli/run.go` | Remove `sshConfigPath` plumbing; update all call sites to new sandbox API | + +--- + +### Task 1: Add `sanitizeDownload` with tests + +This is a standalone function with no dependencies on the migration — build and test it first. + +**Files:** +- Modify: `internal/sandbox/sandbox.go` +- Modify: `internal/sandbox/sandbox_test.go` + +- [ ] **Step 1: Write failing tests for `sanitizeDownload`** + +Add to `internal/sandbox/sandbox_test.go`: + +```go +func TestSanitizeDownload_RemovesSymlinks(t *testing.T) { + dir := t.TempDir() + + // Create a regular file. + require.NoError(t, os.WriteFile(filepath.Join(dir, "real.txt"), []byte("ok"), 0o644)) + + // Create a symlink (dangling is fine — we just need it to exist). + require.NoError(t, os.Symlink("/nonexistent/target", filepath.Join(dir, "danger"))) + + err := sanitizeDownload(dir) + require.NoError(t, err) + + // Regular file should survive. + _, err = os.Stat(filepath.Join(dir, "real.txt")) + assert.NoError(t, err) + + // Symlink should be removed. + _, err = os.Lstat(filepath.Join(dir, "danger")) + assert.True(t, os.IsNotExist(err), "symlink should have been removed") +} + +func TestSanitizeDownload_RemovesGitHooks(t *testing.T) { + dir := t.TempDir() + + // Create .git/hooks/ with a script. + hooksDir := filepath.Join(dir, ".git", "hooks") + require.NoError(t, os.MkdirAll(hooksDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(hooksDir, "pre-commit"), []byte("#!/bin/sh\nmalicious"), 0o755)) + + // Create a safe file under .git/. + require.NoError(t, os.WriteFile(filepath.Join(dir, ".git", "config"), []byte("[core]"), 0o644)) + + err := sanitizeDownload(dir) + require.NoError(t, err) + + // .git/hooks/ should be removed entirely. + _, err = os.Stat(hooksDir) + assert.True(t, os.IsNotExist(err), ".git/hooks/ should have been removed") + + // .git/config should survive. + _, err = os.Stat(filepath.Join(dir, ".git", "config")) + assert.NoError(t, err) +} + +func TestSanitizeDownload_NestedSymlinks(t *testing.T) { + dir := t.TempDir() + + // Create nested structure with symlinks at various depths. + require.NoError(t, os.MkdirAll(filepath.Join(dir, "a", "b"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "a", "b", "real.txt"), []byte("ok"), 0o644)) + require.NoError(t, os.Symlink("/etc/passwd", filepath.Join(dir, "a", "b", "link"))) + require.NoError(t, os.Symlink("/etc/shadow", filepath.Join(dir, "a", "top-link"))) + + err := sanitizeDownload(dir) + require.NoError(t, err) + + // Real file survives. + _, err = os.Stat(filepath.Join(dir, "a", "b", "real.txt")) + assert.NoError(t, err) + + // Both symlinks removed. + _, err = os.Lstat(filepath.Join(dir, "a", "b", "link")) + assert.True(t, os.IsNotExist(err)) + _, err = os.Lstat(filepath.Join(dir, "a", "top-link")) + assert.True(t, os.IsNotExist(err)) +} + +func TestSanitizeDownload_EmptyDir(t *testing.T) { + dir := t.TempDir() + err := sanitizeDownload(dir) + assert.NoError(t, err) +} +``` + +- [ ] **Step 2: Run tests to verify they fail** + +Run: `go test ./internal/sandbox/ -run 'TestSanitizeDownload' -v` +Expected: compilation error — `sanitizeDownload` not defined. + +- [ ] **Step 3: Implement `sanitizeDownload`** + +Add to `internal/sandbox/sandbox.go`, after the imports (add `"io/fs"` to imports): + +```go +// sanitizeDownload walks a downloaded directory and removes symlinks and +// .git/hooks/ to prevent a compromised sandbox from injecting content into +// the host. Equivalent to rsync's --no-links and --exclude .git/hooks/. +func sanitizeDownload(localDir string) error { + return filepath.WalkDir(localDir, func(path string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + rel, _ := filepath.Rel(localDir, path) + + if d.Type()&fs.ModeSymlink != 0 { + return os.Remove(path) + } + + if d.IsDir() && rel == filepath.Join(".git", "hooks") { + os.RemoveAll(path) + return filepath.SkipDir + } + + return nil + }) +} +``` + +- [ ] **Step 4: Run tests to verify they pass** + +Run: `go test ./internal/sandbox/ -run 'TestSanitizeDownload' -v` +Expected: all 4 tests pass. + +- [ ] **Step 5: Commit** + +```bash +git add internal/sandbox/sandbox.go internal/sandbox/sandbox_test.go && git commit -m "feat(sandbox): add sanitizeDownload for symlink and git hooks cleanup" +``` + +--- + +### Task 2: Replace `SSH` with `Exec` + +Replace the `SSH()` function that shells out to `ssh` with `Exec()` that uses `openshell sandbox exec`. The old `SSH` function is removed. + +**Files:** +- Modify: `internal/sandbox/sandbox.go` +- Modify: `internal/sandbox/sandbox_test.go` + +- [ ] **Step 1: Write failing test for `Exec`** + +The existing codebase doesn't have integration tests for SSH (it requires a running sandbox). We'll add a unit test that verifies `Exec` constructs the right command when openshell is unavailable (same pattern as `TestEnsureAvailable_OpenshellNotInPath`). + +Add to `internal/sandbox/sandbox_test.go`: + +```go +func TestExec_OpenshellNotInPath(t *testing.T) { + t.Setenv("PATH", "") + + _, _, _, err := Exec("test-sandbox", "echo hello", 10*time.Second) + assert.Error(t, err) +} +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `go test ./internal/sandbox/ -run 'TestExec_OpenshellNotInPath' -v` +Expected: compilation error — `Exec` not defined. + +- [ ] **Step 3: Implement `Exec` and remove `SSH`** + +Replace the `SSH` function in `internal/sandbox/sandbox.go` with: + +```go +// Exec runs a command inside a sandbox using openshell sandbox exec and returns +// stdout, stderr, and exit code. The timeout is in seconds. +func Exec(sandboxName, command string, timeout time.Duration) (stdout, stderr string, exitCode int, err error) { + timeoutSecs := fmt.Sprintf("%d", int(timeout.Seconds())) + + cmd := exec.Command("openshell", "sandbox", "exec", + "--name", sandboxName, + "--no-tty", + "--timeout", timeoutSecs, + "--", "sh", "-c", command, + ) + + var stdoutBuf, stderrBuf strings.Builder + cmd.Stdout = &stdoutBuf + cmd.Stderr = &stderrBuf + + runErr := cmd.Run() + exitCode = -1 + if cmd.ProcessState != nil { + exitCode = cmd.ProcessState.ExitCode() + } + + if runErr != nil && cmd.ProcessState == nil { + return "", "", exitCode, fmt.Errorf("openshell exec failed to start: %w", runErr) + } + + if exitCode == 124 { + return stdoutBuf.String(), stderrBuf.String(), exitCode, + fmt.Errorf("command timed out after %s", timeout) + } + + return stdoutBuf.String(), stderrBuf.String(), exitCode, nil +} +``` + +Remove the old `SSH` function (lines 197-227) and `GetSSHConfig` function (lines 167-173). + +- [ ] **Step 4: Run test to verify it passes** + +Run: `go test ./internal/sandbox/ -run 'TestExec' -v` +Expected: PASS. + +- [ ] **Step 5: Run full sandbox tests** + +Run: `go test ./internal/sandbox/ -v` +Expected: all tests pass. (Build may fail due to callers of the old `SSH` — that's expected and will be fixed in Task 5.) + +- [ ] **Step 6: Commit** + +```bash +git add internal/sandbox/sandbox.go internal/sandbox/sandbox_test.go && git commit -m "feat(sandbox): replace SSH with Exec using openshell sandbox exec" +``` + +--- + +### Task 3: Replace `SSHStream` and `SSHStreamReader` with `ExecStream` and `ExecStreamReader` + +**Files:** +- Modify: `internal/sandbox/sandbox.go` + +- [ ] **Step 1: Implement `ExecStream` replacing `SSHStream`** + +Replace `SSHStream` (lines 229-257) in `internal/sandbox/sandbox.go` with: + +```go +// ExecStream runs a command inside a sandbox, streaming output to the given writers. +func ExecStream(sandboxName, command string, timeout time.Duration, stdoutW, stderrW *os.File) (int, error) { + timeoutSecs := fmt.Sprintf("%d", int(timeout.Seconds())) + + cmd := exec.Command("openshell", "sandbox", "exec", + "--name", sandboxName, + "--no-tty", + "--timeout", timeoutSecs, + "--", "sh", "-c", command, + ) + cmd.Stdout = stdoutW + cmd.Stderr = stderrW + + err := cmd.Run() + exitCode := -1 + if cmd.ProcessState != nil { + exitCode = cmd.ProcessState.ExitCode() + } + + if err != nil && cmd.ProcessState == nil { + return exitCode, fmt.Errorf("openshell exec failed to start: %w", err) + } + + if exitCode == 124 { + return exitCode, fmt.Errorf("command timed out after %s", timeout) + } + + return exitCode, nil +} +``` + +- [ ] **Step 2: Implement `ExecStreamReader` replacing `SSHStreamReader`** + +Replace `SSHStreamReader` (lines 259-284) with: + +```go +// ExecStreamReader runs a command inside a sandbox, returning an io.ReadCloser for +// stdout so the caller can parse structured output. Stderr is forwarded to the +// given writer. The caller must read stdout to completion, then call cmd.Wait(). +func ExecStreamReader(sandboxName, command string, timeout time.Duration, stderrW io.Writer) (io.ReadCloser, *exec.Cmd, context.CancelFunc, error) { + ctx, cancel := context.WithTimeout(context.Background(), timeout) + timeoutSecs := fmt.Sprintf("%d", int(timeout.Seconds())) + + cmd := exec.CommandContext(ctx, "openshell", "sandbox", "exec", + "--name", sandboxName, + "--no-tty", + "--timeout", timeoutSecs, + "--", "sh", "-c", command, + ) + cmd.Stderr = stderrW + + stdout, err := cmd.StdoutPipe() + if err != nil { + cancel() + return nil, nil, nil, fmt.Errorf("creating stdout pipe: %w", err) + } + + if err := cmd.Start(); err != nil { + cancel() + return nil, nil, nil, fmt.Errorf("starting openshell exec: %w", err) + } + + return stdout, cmd, cancel, nil +} +``` + +- [ ] **Step 3: Verify sandbox package compiles** + +Run: `go build ./internal/sandbox/` +Expected: success. + +- [ ] **Step 4: Run sandbox tests** + +Run: `go test ./internal/sandbox/ -v` +Expected: all tests pass. + +- [ ] **Step 5: Commit** + +```bash +git add internal/sandbox/sandbox.go && git commit -m "feat(sandbox): replace SSHStream/SSHStreamReader with ExecStream/ExecStreamReader" +``` + +--- + +### Task 4: Replace `SCP`, `SCPFrom`, and `RsyncFrom` with `Upload` and `Download` + +**Files:** +- Modify: `internal/sandbox/sandbox.go` + +- [ ] **Step 1: Implement `Upload` replacing `SCP`** + +Replace `SCP` (lines 176-194) in `internal/sandbox/sandbox.go` with: + +```go +// Upload copies a local file or directory into a sandbox using openshell sandbox upload. +func Upload(sandboxName, localPath, remotePath string) error { + ctx, cancel := context.WithTimeout(context.Background(), transferTimeout) + defer cancel() + + cmd := exec.CommandContext(ctx, "openshell", "sandbox", "upload", + sandboxName, + localPath, + remotePath, + ) + out, err := cmd.CombinedOutput() + if err != nil { + if ctx.Err() != nil { + return fmt.Errorf("upload to sandbox %q timed out after %s", sandboxName, transferTimeout) + } + return fmt.Errorf("upload to sandbox %q failed: %s: %w", sandboxName, string(out), err) + } + return nil +} +``` + +- [ ] **Step 2: Implement `Download` replacing `SCPFrom`** + +Replace `SCPFrom` (lines 322-340) with: + +```go +// Download copies a file or directory from a sandbox to the local machine +// using openshell sandbox download. +func Download(sandboxName, remotePath, localPath string) error { + ctx, cancel := context.WithTimeout(context.Background(), transferTimeout) + defer cancel() + + cmd := exec.CommandContext(ctx, "openshell", "sandbox", "download", + sandboxName, + remotePath, + localPath, + ) + out, err := cmd.CombinedOutput() + if err != nil { + if ctx.Err() != nil { + return fmt.Errorf("download from sandbox %q timed out after %s", sandboxName, transferTimeout) + } + return fmt.Errorf("download from sandbox %q failed: %s: %w", sandboxName, string(out), err) + } + return nil +} +``` + +- [ ] **Step 3: Implement `SafeDownload` replacing `RsyncFrom`** + +Replace `RsyncFrom` (lines 286-319) with: + +```go +// SafeDownload copies a directory from a sandbox to the local machine with +// security protections: symlinks are removed and .git/hooks/ is deleted after +// download. Replaces rsync --no-links --exclude .git/hooks/. +func SafeDownload(sandboxName, remoteDir, localDir string) error { + if err := Download(sandboxName, remoteDir, localDir); err != nil { + return err + } + return sanitizeDownload(localDir) +} +``` + +- [ ] **Step 4: Verify sandbox package compiles** + +Run: `go build ./internal/sandbox/` +Expected: success. + +- [ ] **Step 5: Run sandbox tests** + +Run: `go test ./internal/sandbox/ -v` +Expected: all tests pass. + +- [ ] **Step 6: Commit** + +```bash +git add internal/sandbox/sandbox.go && git commit -m "feat(sandbox): replace SCP/SCPFrom/RsyncFrom with Upload/Download/SafeDownload" +``` + +--- + +### Task 5: Update `ExtractTranscripts` and `ExtractOutputFiles` to use new API + `os.Root` + +These functions call `SSH` and `SCPFrom` internally. Update them to use `Exec` and `Download`, and replace `filepath.Clean` + `HasPrefix` with `os.Root`. + +**Files:** +- Modify: `internal/sandbox/sandbox.go` +- Modify: `internal/sandbox/sandbox_test.go` + +- [ ] **Step 1: Write failing test for `os.Root` containment** + +Update `TestPathTraversalContainment` in `internal/sandbox/sandbox_test.go` to verify `os.Root` rejects traversal: + +```go +func TestOsRootContainment(t *testing.T) { + dir := t.TempDir() + + root, err := os.OpenRoot(dir) + require.NoError(t, err) + defer root.Close() + + // Normal file creation should work. + f, err := root.Create("safe.txt") + require.NoError(t, err) + f.Close() + + // Path traversal should fail. + _, err = root.Create("../../../etc/passwd") + assert.Error(t, err) + + // Traversal with prefix should fail. + _, err = root.Create("../../home/runner/.bashrc") + assert.Error(t, err) + + // Dot segments in middle should fail. + _, err = root.Create("subdir/../../etc/shadow") + assert.Error(t, err) +} +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `go test ./internal/sandbox/ -run 'TestOsRootContainment' -v` +Expected: PASS (this test validates stdlib behavior, so it should pass immediately — the real migration test is that `ExtractTranscripts`/`ExtractOutputFiles` compile with the new API). + +- [ ] **Step 3: Update `ExtractTranscripts`** + +Replace the `ExtractTranscripts` function (lines 362-407) with: + +```go +// ExtractTranscripts copies Claude transcript files (.jsonl) from the sandbox +// to a local output directory. Uses os.Root for path containment. +func ExtractTranscripts(sandboxName, agentName, outputDir string) error { + if err := os.MkdirAll(outputDir, 0o755); err != nil { + return fmt.Errorf("creating output dir: %w", err) + } + + root, err := os.OpenRoot(outputDir) + if err != nil { + return fmt.Errorf("opening output root: %w", err) + } + defer root.Close() + + stdout, _, _, err := Exec(sandboxName, + fmt.Sprintf("find %s -name '*.jsonl' 2>/dev/null || true", SandboxClaudeConfig), + 10*time.Second, + ) + if err != nil { + return fmt.Errorf("finding transcripts: %w", err) + } + + trimmed := strings.TrimSpace(stdout) + if trimmed == "" { + fmt.Fprintf(os.Stderr, " [%s] No transcripts found\n", agentName) + return nil + } + files := strings.Split(trimmed, "\n") + + for _, remotePath := range files { + remotePath = strings.TrimSpace(remotePath) + if remotePath == "" { + continue + } + localName := fmt.Sprintf("%s-%s", agentName, filepath.Base(remotePath)) + + // Use os.Root to create the file — kernel-enforced path containment. + f, createErr := root.Create(localName) + if createErr != nil { + fmt.Fprintf(os.Stderr, " [%s] Skipping (path rejected): %s: %v\n", agentName, localName, createErr) + continue + } + f.Close() + + localPath := filepath.Join(outputDir, localName) + if scpErr := Download(sandboxName, remotePath, localPath); scpErr != nil { + fmt.Fprintf(os.Stderr, " [%s] Failed to copy transcript: %v\n", agentName, scpErr) + continue + } + fmt.Fprintf(os.Stderr, " [%s] Saved transcript: %s\n", agentName, localName) + } + + return nil +} +``` + +- [ ] **Step 4: Update `ExtractOutputFiles`** + +Replace the `ExtractOutputFiles` function (lines 409-463) with: + +```go +// ExtractOutputFiles copies all files under a remote directory in the sandbox +// to a local output directory, preserving relative paths. Uses os.Root for +// path containment. +func ExtractOutputFiles(sandboxName, remoteDir, localDir string) ([]string, error) { + if err := os.MkdirAll(localDir, 0o755); err != nil { + return nil, fmt.Errorf("creating local output dir: %w", err) + } + + root, err := os.OpenRoot(localDir) + if err != nil { + return nil, fmt.Errorf("opening output root: %w", err) + } + defer root.Close() + + stdout, _, _, err := Exec(sandboxName, + fmt.Sprintf("find %s -type f 2>/dev/null || true", remoteDir), + 10*time.Second, + ) + if err != nil { + return nil, fmt.Errorf("listing output files: %w", err) + } + + trimmed := strings.TrimSpace(stdout) + if trimmed == "" { + return nil, nil + } + lines := strings.Split(trimmed, "\n") + + var extracted []string + for _, remotePath := range lines { + remotePath = strings.TrimSpace(remotePath) + if remotePath == "" { + continue + } + relPath := strings.TrimPrefix(remotePath, remoteDir) + relPath = strings.TrimPrefix(relPath, "/") + + // Use os.Root to validate the path — kernel-enforced containment. + f, createErr := root.Create(relPath) + if createErr != nil { + // os.Root rejects path traversal attempts. + fmt.Fprintf(os.Stderr, " Skipping (path rejected): %s: %v\n", relPath, createErr) + continue + } + f.Close() + + localPath := filepath.Join(localDir, relPath) + if err := os.MkdirAll(filepath.Dir(localPath), 0o755); err != nil { + fmt.Fprintf(os.Stderr, " Failed to create dir for %s: %v\n", relPath, err) + continue + } + + if dlErr := Download(sandboxName, remotePath, localPath); dlErr != nil { + fmt.Fprintf(os.Stderr, " Failed to copy %s: %v\n", relPath, dlErr) + continue + } + extracted = append(extracted, localPath) + } + + return extracted, nil +} +``` + +- [ ] **Step 5: Remove old `TestPathTraversalContainment`** + +The old test validates the `filepath.Clean` + `HasPrefix` pattern which is no longer used. Remove it from `internal/sandbox/sandbox_test.go` (the `TestOsRootContainment` test added in Step 1 replaces it). + +- [ ] **Step 6: Verify sandbox package compiles** + +Run: `go build ./internal/sandbox/` +Expected: success. + +- [ ] **Step 7: Run sandbox tests** + +Run: `go test ./internal/sandbox/ -v` +Expected: all tests pass. + +- [ ] **Step 8: Commit** + +```bash +git add internal/sandbox/sandbox.go internal/sandbox/sandbox_test.go && git commit -m "feat(sandbox): update ExtractTranscripts/ExtractOutputFiles to use Exec/Download and os.Root" +``` + +--- + +### Task 6: Clean up sandbox.go — remove dead imports and old functions + +After Tasks 2-5, the old `SSH`, `SSHStream`, `SSHStreamReader`, `SCP`, `SCPFrom`, `RsyncFrom`, and `GetSSHConfig` functions should all be removed. Verify no dead code remains. + +**Files:** +- Modify: `internal/sandbox/sandbox.go` + +- [ ] **Step 1: Remove unused imports** + +The `"context"` import is still needed by `ExecStreamReader`. Remove any imports that are no longer used. Run: + +```bash +go build ./internal/sandbox/ 2>&1 +``` + +If there are unused import errors, remove them. The following imports should remain: +- `"context"` — used by `ExecStreamReader` +- `"fmt"` +- `"io"` — used by `ExecStreamReader` +- `"io/fs"` — used by `sanitizeDownload` +- `"os"` +- `"os/exec"` +- `"path/filepath"` +- `"strings"` +- `"time"` + +- [ ] **Step 2: Verify no references to old functions remain in the sandbox package** + +Run: `grep -n 'func SSH\|func SCP\|func SCPFrom\|func RsyncFrom\|func GetSSHConfig\|func SSHStream' internal/sandbox/sandbox.go` +Expected: no output (all old functions removed). + +- [ ] **Step 3: Run sandbox tests** + +Run: `go test ./internal/sandbox/ -v` +Expected: all tests pass. + +- [ ] **Step 4: Commit** + +```bash +git add internal/sandbox/sandbox.go && git commit -m "refactor(sandbox): remove dead imports and verify clean state" +``` + +--- + +### Task 7: Migrate `run.go` — remove SSH config plumbing and update all call sites + +This is the largest task. All 38 `sshConfigPath` references in `run.go` need to be removed, and every `sandbox.SSH()`/`sandbox.SCP()`/etc. call updated to the new API. + +**Files:** +- Modify: `internal/cli/run.go` + +- [ ] **Step 1: Remove SSH config creation and cleanup from `runAgent`** + +Remove lines 282-300 from `runAgent` (the `GetSSHConfig` + temp file creation + defer cleanup block): + +```go +// DELETE this entire block: +// 4. Get SSH config. +sshConfig, err := sandbox.GetSSHConfig(sandboxName) +// ... through ... +defer os.Remove(sshConfigPath) +``` + +- [ ] **Step 2: Remove `sshConfigPath` parameter from internal functions** + +Update function signatures — remove `sshConfigPath` from: + +- `bootstrapSandbox(sshConfigPath, sandboxName, ...)` → `bootstrapSandbox(sandboxName, ...)` (line 581) +- `bootstrapEnv(sshConfigPath, sandboxName, ...)` → `bootstrapEnv(sandboxName, ...)` (line 711) +- `bootstrapSecurityHooks(sshConfigPath, sandboxName, ...)` → `bootstrapSecurityHooks(sandboxName, ...)` (line 1154) +- `runAgentWithProgress(sshConfigPath, sandboxName, ...)` → `runAgentWithProgress(sandboxName, ...)` (line 819) +- `injectTraceID(sshConfigPath, sandboxName, ...)` → `injectTraceID(sandboxName, ...)` (line 1237) + +- [ ] **Step 3: Update all `sandbox.SSH()` calls to `sandbox.Exec()`** + +Replace every `sandbox.SSH(sshConfigPath, sandboxName, ...)` with `sandbox.Exec(sandboxName, ...)`. There are 12 call sites: + +In `runAgent`: +- Line 323: `sandbox.SSH(sshConfigPath, sandboxName, mkRepoCmd, 10*time.Second)` → `sandbox.Exec(sandboxName, mkRepoCmd, 10*time.Second)` +- Line 338: `sandbox.SSH(sshConfigPath, sandboxName, mkInputCmd, 10*time.Second)` → `sandbox.Exec(sandboxName, mkInputCmd, 10*time.Second)` +- Line 380: `sandbox.SSH(sshConfigPath, sandboxName, scanCmd, 60*time.Second)` → `sandbox.Exec(sandboxName, scanCmd, 60*time.Second)` +- Line 443: `sandbox.SSH(sshConfigPath, sandboxName, clearCmd, 10*time.Second)` → `sandbox.Exec(sandboxName, clearCmd, 10*time.Second)` + +In `bootstrapSandbox`: +- Line 588: `sandbox.SSH(sshConfigPath, sandboxName, mkdirCmd, 10*time.Second)` → `sandbox.Exec(sandboxName, mkdirCmd, 10*time.Second)` +- Line 608: `sandbox.SSH(sshConfigPath, sandboxName, chmodCmd, 10*time.Second)` → `sandbox.Exec(sandboxName, chmodCmd, 10*time.Second)` + +In `bootstrapEnv`: +- Line 796: `sandbox.SSH(sshConfigPath, sandboxName, chmodCmd, 10*time.Second)` → `sandbox.Exec(sandboxName, chmodCmd, 10*time.Second)` + +In `bootstrapSecurityHooks`: +- Line 1178: `sandbox.SSH(sshConfigPath, sandboxName, chmodCmd, 10*time.Second)` → `sandbox.Exec(sandboxName, chmodCmd, 10*time.Second)` +- Line 1218: `sandbox.SSH(sshConfigPath, sandboxName, envCmd, 10*time.Second)` → `sandbox.Exec(sandboxName, envCmd, 10*time.Second)` +- Line 1227: `sandbox.SSH(sshConfigPath, sandboxName, envCmd, 10*time.Second)` → `sandbox.Exec(sandboxName, envCmd, 10*time.Second)` + +In `injectTraceID`: +- Line 1243: `sandbox.SSH(sshConfigPath, sandboxName, cmd, 10*time.Second)` → `sandbox.Exec(sandboxName, cmd, 10*time.Second)` + +- [ ] **Step 4: Update all `sandbox.SCP()` calls to `sandbox.Upload()`** + +Replace every `sandbox.SCP(sshConfigPath, sandboxName, local, remote)` with `sandbox.Upload(sandboxName, local, remote)`. There are 10 call sites: + +In `runAgent`: +- Line 326: `sandbox.SCP(sshConfigPath, sandboxName, repoSrc+"/.", repoDir+"/")` → `sandbox.Upload(sandboxName, repoSrc+"/.", repoDir+"/")` +- Line 341: `sandbox.SCP(sshConfigPath, sandboxName, h.AgentInput+"/.", remoteInput+"/")` → `sandbox.Upload(sandboxName, h.AgentInput+"/.", remoteInput+"/")` + +In `bootstrapSandbox`: +- Line 604: `sandbox.SCP(sshConfigPath, sandboxName, localBinary, remoteBinary)` → `sandbox.Upload(sandboxName, localBinary, remoteBinary)` +- Line 641: `sandbox.SCP(sshConfigPath, sandboxName, h.Agent, ...)` → `sandbox.Upload(sandboxName, h.Agent, ...)` +- Line 679: `sandbox.SCP(sshConfigPath, sandboxName, skillPath, ...)` → `sandbox.Upload(sandboxName, skillPath, ...)` + +In `bootstrapEnv`: +- Line 740: `sandbox.SCP(sshConfigPath, sandboxName, tmpFile.Name(), remoteEnvFile)` → `sandbox.Upload(sandboxName, tmpFile.Name(), remoteEnvFile)` +- Line 778: `sandbox.SCP(sshConfigPath, sandboxName, tmp.Name(), hf.Dest)` → `sandbox.Upload(sandboxName, tmp.Name(), hf.Dest)` +- Line 784: `sandbox.SCP(sshConfigPath, sandboxName, hostPath, hf.Dest)` → `sandbox.Upload(sandboxName, hostPath, hf.Dest)` + +In `bootstrapSecurityHooks`: +- Line 1170: `sandbox.SCP(sshConfigPath, sandboxName, tmpFile.Name(), remotePath)` → `sandbox.Upload(sandboxName, tmpFile.Name(), remotePath)` +- Line 1201: `sandbox.SCP(sshConfigPath, sandboxName, tmpSettings.Name(), remoteSettings)` → `sandbox.Upload(sandboxName, tmpSettings.Name(), remoteSettings)` + +- [ ] **Step 5: Update `sandbox.SSHStreamReader()` to `sandbox.ExecStreamReader()`** + +In `runAgentWithProgress` (line 820): + +```go +// Before: +stdout, cmd, cancel, err := sandbox.SSHStreamReader(sshConfigPath, sandboxName, claudeCmd, timeout, os.Stderr) + +// After: +stdout, cmd, cancel, err := sandbox.ExecStreamReader(sandboxName, claudeCmd, timeout, os.Stderr) +``` + +Also update the error message on line 839: + +```go +// Before: +return exitCode, fmt.Errorf("ssh failed: %w", waitErr) + +// After: +return exitCode, fmt.Errorf("openshell exec failed: %w", waitErr) +``` + +- [ ] **Step 6: Update `sandbox.RsyncFrom()` to `sandbox.SafeDownload()`** + +In `runAgent` (line 504): + +```go +// Before: +if err := sandbox.RsyncFrom(sshConfigPath, sandboxName, repoDir, repoSrc); err != nil { + +// After: +if err := sandbox.SafeDownload(sandboxName, repoDir, repoSrc); err != nil { +``` + +- [ ] **Step 7: Update `sandbox.SCPFrom()` to `sandbox.Download()`** + +In `runAgent` (line 550): + +```go +// Before: +if scpErr := sandbox.SCPFrom(sshConfigPath, sandboxName, remoteFindingsDir, findingsDir); scpErr != nil { + +// After: +if scpErr := sandbox.Download(sandboxName, remoteFindingsDir, findingsDir); scpErr != nil { +``` + +- [ ] **Step 8: Update `sandbox.ExtractOutputFiles()` and `sandbox.ExtractTranscripts()` calls** + +These functions lost the `sshConfigPath` parameter. Update call sites: + +Line 478: +```go +// Before: +extracted, extractErr := sandbox.ExtractOutputFiles(sshConfigPath, sandboxName, remoteSrc, iterOutputDir) + +// After: +extracted, extractErr := sandbox.ExtractOutputFiles(sandboxName, remoteSrc, iterOutputDir) +``` + +Line 493: +```go +// Before: +if err := sandbox.ExtractTranscripts(sshConfigPath, sandboxName, agentName, iterTranscriptDir); err != nil { + +// After: +if err := sandbox.ExtractTranscripts(sandboxName, agentName, iterTranscriptDir); err != nil { +``` + +- [ ] **Step 9: Update call sites for internal functions** + +Update the calls to the refactored internal functions: + +Line 313: +```go +// Before: +if err := bootstrapSandbox(sshConfigPath, sandboxName, repoDir, fullsendBinary, h); err != nil { + +// After: +if err := bootstrapSandbox(sandboxName, repoDir, fullsendBinary, h); err != nil { +``` + +Line 370: +```go +// Before: +if err := injectTraceID(sshConfigPath, sandboxName, traceID); err != nil { + +// After: +if err := injectTraceID(sandboxName, traceID); err != nil { +``` + +Line 457: +```go +// Before: +exitCode, runErr := runAgentWithProgress(sshConfigPath, sandboxName, claudeCmd, timeout, printer, agentStart, &metrics) + +// After: +exitCode, runErr := runAgentWithProgress(sandboxName, claudeCmd, timeout, printer, agentStart, &metrics) +``` + +Line 686: +```go +// Before: +if err := bootstrapEnv(sshConfigPath, sandboxName, repoDir, h); err != nil { + +// After: +if err := bootstrapEnv(sandboxName, repoDir, h); err != nil { +``` + +Line 692: +```go +// Before: +if err := bootstrapSecurityHooks(sshConfigPath, sandboxName, h); err != nil { + +// After: +if err := bootstrapSecurityHooks(sandboxName, h); err != nil { +``` + +- [ ] **Step 10: Remove stale comments referencing SSH/SCP** + +Update the comment on line 499 (above `RsyncFrom` call): + +```go +// Before: +// 9d. Extract target repo back to host. Uses rsync with --no-links +// and --exclude .git/hooks/ to prevent sandbox escape via symlinks +// or injected git hooks. + +// After: +// 9d. Extract target repo back to host. SafeDownload removes symlinks +// and .git/hooks/ after download to prevent sandbox escape. +``` + +Update the comment on line 646: + +```go +// Before: +// Copy skills (SCP -r copies the entire directory tree, including any +// scripts/, references/, and assets/ bundled with the skill per the +// agentskills.io specification). + +// After: +// Copy skills (Upload copies the entire directory tree, including any +// scripts/, references/, and assets/ bundled with the skill per the +// agentskills.io specification). +``` + +- [ ] **Step 11: Verify full project compiles** + +Run: `go build ./...` +Expected: success — no compilation errors. + +- [ ] **Step 12: Run all tests** + +Run: `go test ./... 2>&1 | tail -30` +Expected: all tests pass. + +- [ ] **Step 13: Run vet and lint** + +Run: `make lint` +Expected: no issues. + +- [ ] **Step 14: Commit** + +```bash +git add internal/cli/run.go && git commit -m "refactor(cli): migrate run.go from SSH/SCP to openshell exec/upload/download + +Remove sshConfigPath plumbing from all internal functions. Update 38 +call sites to use the new sandbox.Exec/Upload/Download/SafeDownload API. +SSH config temp file creation and cleanup are no longer needed." +``` + +--- + +### Task 8: Final verification and cleanup + +**Files:** +- All files in previous tasks + +- [ ] **Step 1: Verify no references to old API remain** + +Run: +```bash +grep -rn 'sandbox\.SSH\b\|sandbox\.SCP\b\|sandbox\.SCPFrom\|sandbox\.RsyncFrom\|sandbox\.SSHStream\|sandbox\.GetSSHConfig\|sshConfigPath' internal/ --include='*.go' +``` +Expected: no output. + +- [ ] **Step 2: Verify no references to ssh/scp/rsync binaries in sandbox package** + +Run: +```bash +grep -n '"ssh"\|"scp"\|"rsync"' internal/sandbox/sandbox.go +``` +Expected: no output. + +- [ ] **Step 3: Run full test suite** + +Run: `make go-test` +Expected: all tests pass. + +- [ ] **Step 4: Run vet** + +Run: `make go-vet` +Expected: clean. + +- [ ] **Step 5: Run lint** + +Run: `make lint` +Expected: clean. diff --git a/docs/superpowers/specs/2026-05-06-openshell-native-sandbox-transport-design.md b/docs/superpowers/specs/2026-05-06-openshell-native-sandbox-transport-design.md new file mode 100644 index 0000000000..6fcbd018f8 --- /dev/null +++ b/docs/superpowers/specs/2026-05-06-openshell-native-sandbox-transport-design.md @@ -0,0 +1,164 @@ +# OpenShell Native Sandbox Transport + +Replace `exec.Command` SSH/SCP/rsync wrappers in `internal/sandbox/` with OpenShell's native CLI commands (`sandbox exec`, `sandbox upload`, `sandbox download`) and add `os.Root` containment for local writes. + +Addresses [#261](https://github.com/fullsend-ai/fullsend/issues/261). + +## Motivation + +The sandbox package shells out to `ssh`, `scp`, and `rsync` via `exec.Command` for all sandbox communication. This creates several defensive-workaround classes: + +- **Path traversal**: `scp -r` follows remote directory structure blindly; containment requires manual `filepath.Clean` + `HasPrefix` at every extraction site. +- **Symlink following**: `scp -r` follows symlinks by default; `rsync --no-links` is used for write-back but not all transfers. +- **No native timeout**: `SCP`/`SCPFrom` rely on `exec.CommandContext` for timeout; no per-operation deadline support. +- **ProcessState nil panics**: if the process fails to start, `cmd.ProcessState` is nil; every call site needs a nil guard. + +OpenShell already provides native CLI commands that use gRPC internally, eliminating the need for SSH entirely. + +## Approach + +**Hybrid: OpenShell native CLI for transport + `os.Root` for local write containment.** + +### Why not Go-native SSH/SFTP libraries? + +`x/crypto/ssh` + `github.com/pkg/sftp` would eliminate subprocesses entirely, but: + +- OpenShell's `exec`/`upload`/`download` commands already use gRPC internally — we'd be reimplementing what they provide. +- Parsing SSH config to extract host/port/key adds complexity; OpenShell handles connection routing internally. +- Two new dependencies for functionality that already exists in the tool we depend on. + +### Why not OpenShell CLI alone (without `os.Root`)? + +Testing confirmed that `openshell sandbox download` preserves symlinks as-is on the host (e.g., a sandbox symlink to `/etc/passwd` becomes a local symlink to `/etc/passwd`). While less dangerous than `scp -r` (which follows and copies the target content), a symlink pointing to a valid host path could still be exploited. `os.Root` provides kernel-level path containment that eliminates this class of issue, including TOCTOU races that `filepath.Clean` + `HasPrefix` cannot prevent. + +## Validated Assumptions + +Tested against a live OpenShell sandbox: + +| Capability | Verified behavior | +|---|---| +| `sandbox exec` stdout piping | Streams line-by-line; NDJSON parsing works via `exec.Command` + `StdoutPipe()` | +| `sandbox exec` exit codes | Remote exit code propagated; timeout returns exit code 124 | +| `sandbox exec` timeout | `--timeout ` works; kills the remote process | +| `sandbox exec` newlines | Command *arguments* cannot contain newlines; `sh -c 'single string'` works (matches current usage) | +| `sandbox upload` directory semantics | Copies *contents* of source into destination (matches `scp -r src/. dest/` pattern) | +| `sandbox upload` single file | Works as expected | +| `sandbox download` symlinks | Preserves symlinks as-is — does **not** follow them, but creates them locally | + +## Design + +### Functions replaced + +| Current function | Replacement | Notes | +|---|---|---| +| `SSH()` | `Exec()` | Uses `openshell sandbox exec --no-tty --timeout `. Captures stdout/stderr via `exec.Command`. | +| `SSHStream()` | `ExecStream()` | Same as `Exec()` but wires stdout/stderr to provided writers. | +| `SSHStreamReader()` | `ExecStreamReader()` | Uses `StdoutPipe()` on the `openshell sandbox exec` command. Returns `io.ReadCloser` + `*exec.Cmd` + `context.CancelFunc`. | +| `SCP()` | `Upload()` | Uses `openshell sandbox upload `. | +| `SCPFrom()` | `Download()` | Uses `openshell sandbox download `. | +| `RsyncFrom()` | `Download()` + post-download cleanup | Download replaces rsync. Symlink and `.git/hooks/` protections move to local post-processing (see below). | +| `GetSSHConfig()` | Removed | No longer needed — OpenShell handles connection routing. | + +### Caller migration (`internal/cli/run.go`) + +15 call sites in `run.go` reference the old functions. Each maps directly: + +- 10× `SCP()` → `Upload()` — bootstrap steps (repo, agent binary, skills, env, settings, host files) +- 1× `SSHStreamReader()` → `ExecStreamReader()` — agent progress tracking +- 1× `RsyncFrom()` → `Download()` + symlink cleanup — repo extraction +- 1× `SCPFrom()` → `Download()` — findings extraction +- 2× `SSH()` → `Exec()` — called indirectly via `ExtractTranscripts` and `ExtractOutputFiles` + +The SSH config file creation/cleanup in `run.go` (lines ~289-300) is also removed. + +### Local write containment with `os.Root` + +`ExtractTranscripts()` and `ExtractOutputFiles()` currently use `filepath.Clean` + `strings.HasPrefix` to prevent path traversal from sandbox-controlled filenames. This is replaced with `os.Root`: + +```go +root, err := os.OpenRoot(outputDir) +if err != nil { + return fmt.Errorf("opening root dir: %w", err) +} +defer root.Close() + +// All file operations go through root — kernel-enforced containment. +f, err := root.Create(relativePath) +``` + +This eliminates: +- TOCTOU races between the check and the file operation +- Manual `filepath.Clean` + `HasPrefix` at each call site +- The possibility of a missed check when adding new extraction code + +### Post-download symlink and hooks cleanup + +`RsyncFrom()` currently uses `--no-links` and `--exclude .git/hooks/` to prevent a compromised sandbox from injecting content. Since `openshell sandbox download` preserves symlinks, we add post-download cleanup: + +```go +func sanitizeDownload(localDir string) error { + return filepath.WalkDir(localDir, func(path string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + rel, _ := filepath.Rel(localDir, path) + + // Remove symlinks (equivalent to rsync --no-links). + if d.Type()&fs.ModeSymlink != 0 { + return os.Remove(path) + } + + // Remove .git/hooks/ contents (equivalent to rsync --exclude .git/hooks/). + if d.IsDir() && rel == filepath.Join(".git", "hooks") { + os.RemoveAll(path) + return filepath.SkipDir + } + + return nil + }) +} +``` + +Note: `sanitizeDownload` operates on absolute paths after download completes — it doesn't need `os.Root` because it's cleaning up a directory we own, not writing sandbox-controlled content. `os.Root` is used in `ExtractTranscripts`/`ExtractOutputFiles` where sandbox-controlled filenames determine the write path. + +### Functions unchanged + +These `exec.Command` calls target the `openshell` binary directly (not SSH/SCP/rsync) and are out of scope: + +- `EnsureProvider()` — `openshell provider create` +- `EnsureAvailable()` — `exec.LookPath("openshell")` +- `EnsureGateway()` — `openshell gateway info/start` +- `Create()` — `openshell sandbox create` +- `Delete()` — `openshell sandbox delete` +- `CollectLogs()` — `openshell logs` + +### API surface changes + +The `sshConfigPath` parameter is removed from all public function signatures. Functions take `sandboxName` directly. Callers no longer need to create, write, or clean up SSH config temp files. + +Before: +```go +func SSH(sshConfigPath, sandboxName, command string, timeout time.Duration) (stdout, stderr string, exitCode int, err error) +func SCP(sshConfigPath, sandboxName, localPath, remotePath string) error +``` + +After: +```go +func Exec(sandboxName, command string, timeout time.Duration) (stdout, stderr string, exitCode int, err error) +func Upload(sandboxName, localPath, remotePath string) error +``` + +## Testing + +- **Unit tests**: Existing `TestPathTraversalContainment` updated to use `os.Root`. New tests for `sanitizeDownload` (symlink removal, `.git/hooks/` removal). +- **Integration tests (`make e2e-test`)**: The e2e tests exercise the full run flow against a live sandbox — they are the primary validation that the migration works end-to-end. +- **Manual verification**: Run a fullsend agent in a sandbox, confirm bootstrap uploads, agent execution with progress streaming, and repo extraction all work. + +## Risks + +| Risk | Mitigation | +|---|---| +| `openshell sandbox exec` behavior differs subtly from `ssh` | Validated core behaviors (streaming, exit codes, timeout) in live testing. E2e tests cover the full flow. | +| `download` symlink handling changes in future OpenShell versions | `os.Root` + `sanitizeDownload` provide defense-in-depth regardless of transport behavior. | +| `upload`/`download` performance differs from `scp`/`rsync` | Both use gRPC streaming internally. If performance regresses, it's an OpenShell issue to report upstream. | +| Breaking change to sandbox package API (`sshConfigPath` removed) | All callers are internal (`run.go`). No external consumers. | diff --git a/internal/cli/run.go b/internal/cli/run.go index 7442487749..c3c2d0c3f4 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -291,27 +291,7 @@ func runAgent(agentName, fullsendDir, outputBase, targetRepo, fullsendBinary str }() printer.StepDone(fmt.Sprintf("Sandbox created (%.1fs)", time.Since(createStart).Seconds())) - // 4. Get SSH config. - sshConfig, err := sandbox.GetSSHConfig(sandboxName) - if err != nil { - printer.StepFail("Failed to get SSH config") - return err - } - - sshConfigFile, err := os.CreateTemp("", "openshell-ssh-*.config") - if err != nil { - return fmt.Errorf("creating SSH config temp file: %w", err) - } - sshConfigPath := sshConfigFile.Name() - if _, err := sshConfigFile.WriteString(sshConfig); err != nil { - sshConfigFile.Close() - os.Remove(sshConfigPath) - return fmt.Errorf("writing SSH config: %w", err) - } - sshConfigFile.Close() - defer os.Remove(sshConfigPath) - - // 6. Resolve target repo path (needed by bootstrap for env vars). + // 4. Resolve target repo path (needed by bootstrap for env vars). repoSrc, err := filepath.Abs(targetRepo) if err != nil { return fmt.Errorf("resolving target repo path: %w", err) @@ -322,7 +302,7 @@ func runAgent(agentName, fullsendDir, outputBase, targetRepo, fullsendBinary str // 7. Bootstrap sandbox. bootstrapStart := time.Now() printer.StepStart("Bootstrapping sandbox") - if err := bootstrapSandbox(sshConfigPath, sandboxName, repoDir, fullsendBinary, h); err != nil { + if err := bootstrapSandbox(sandboxName, repoDir, fullsendBinary, h); err != nil { printer.StepFail("Failed to bootstrap sandbox") return err } @@ -332,10 +312,10 @@ func runAgent(agentName, fullsendDir, outputBase, targetRepo, fullsendBinary str copyStart := time.Now() printer.StepStart("Copying project code into sandbox") mkRepoCmd := fmt.Sprintf("mkdir -p %s", repoDir) - if _, _, _, err := sandbox.SSH(sshConfigPath, sandboxName, mkRepoCmd, 10*time.Second); err != nil { + if _, _, _, err := sandbox.Exec(sandboxName, mkRepoCmd, 10*time.Second); err != nil { return fmt.Errorf("creating repo dir in sandbox: %w", err) } - if err := sandbox.SCP(sshConfigPath, sandboxName, repoSrc+"/.", repoDir+"/"); err != nil { + if err := sandbox.Upload(sandboxName, repoSrc+"/.", repoDir+"/"); err != nil { printer.StepFail("Failed to copy project code") return fmt.Errorf("copying project code: %w", err) } @@ -349,12 +329,12 @@ func runAgent(agentName, fullsendDir, outputBase, targetRepo, fullsendBinary str if !hasAgentsMD(repoSrc) { orgAgentsMD := filepath.Join(absFullsendDir, "AGENTS.md") if _, err := os.Stat(orgAgentsMD); err == nil { - if err := sandbox.SCP(sshConfigPath, sandboxName, orgAgentsMD, repoDir+"/AGENTS.md"); err != nil { + if err := sandbox.Upload(sandboxName, orgAgentsMD, repoDir+"/AGENTS.md"); err != nil { printer.StepWarn("Could not inject org AGENTS.md: " + err.Error()) } else { // Hide the injected file from git status so agents don't stage it. excludeCmd := fmt.Sprintf("echo 'AGENTS.md' >> %s/.git/info/exclude", repoDir) - if _, _, _, err := sandbox.SSH(sshConfigPath, sandboxName, excludeCmd, 5*time.Second); err != nil { + if _, _, _, err := sandbox.Exec(sandboxName, excludeCmd, 5*time.Second); err != nil { printer.StepWarn("Could not add AGENTS.md to git exclude: " + err.Error()) } printer.StepDone("Injected org-level AGENTS.md (target repo has none)") @@ -368,10 +348,10 @@ func runAgent(agentName, fullsendDir, outputBase, targetRepo, fullsendBinary str printer.StepStart("Copying agent-input files into sandbox") remoteInput := fmt.Sprintf("%s/agent-input", sandbox.SandboxWorkspace) mkInputCmd := fmt.Sprintf("mkdir -p %s", remoteInput) - if _, _, _, err := sandbox.SSH(sshConfigPath, sandboxName, mkInputCmd, 10*time.Second); err != nil { + if _, _, _, err := sandbox.Exec(sandboxName, mkInputCmd, 10*time.Second); err != nil { return fmt.Errorf("creating agent-input dir in sandbox: %w", err) } - if err := sandbox.SCP(sshConfigPath, sandboxName, h.AgentInput+"/.", remoteInput+"/"); err != nil { + if err := sandbox.Upload(sandboxName, h.AgentInput+"/.", remoteInput+"/"); err != nil { printer.StepFail("Failed to copy agent-input files") return fmt.Errorf("copying agent-input files: %w", err) } @@ -400,7 +380,7 @@ func runAgent(agentName, fullsendDir, outputBase, targetRepo, fullsendBinary str // 9a. Generate trace ID for security finding correlation. traceID := security.GenerateTraceID() printer.KeyValue("Trace ID", traceID) - if err := injectTraceID(sshConfigPath, sandboxName, traceID); err != nil { + if err := injectTraceID(sandboxName, traceID); err != nil { printer.StepWarn("Could not inject trace ID into sandbox: " + err.Error()) } @@ -410,11 +390,11 @@ func runAgent(agentName, fullsendDir, outputBase, targetRepo, fullsendBinary str if h.SecurityEnabled() { printer.StepStart("Running pre-agent security scan") scanCmd := buildScanContextCommand(repoDir, traceID) - stdout, stderr, exitCode, sshErr := sandbox.SSH(sshConfigPath, sandboxName, scanCmd, 60*time.Second) - if sshErr != nil { - printer.StepFail("Security scan SSH failed: " + sshErr.Error()) + stdout, stderr, exitCode, execErr := sandbox.Exec(sandboxName, scanCmd, 60*time.Second) + if execErr != nil { + printer.StepFail("Security scan failed: " + execErr.Error()) if h.FailModeClosed() { - return fmt.Errorf("pre-agent security scan failed: %w", sshErr) + return fmt.Errorf("pre-agent security scan failed: %w", execErr) } printer.StepWarn("Continuing despite scan failure (fail_mode: open)") } else if exitCode != 0 { @@ -461,7 +441,7 @@ func runAgent(agentName, fullsendDir, outputBase, targetRepo, fullsendBinary str oidcWg.Add(1) go func() { defer oidcWg.Done() - runOIDCRefresh(oidcCtx, sshConfigPath, sandboxName, oidcURL, oidcAuth, printer) + runOIDCRefresh(oidcCtx, sandboxName, oidcURL, oidcAuth, printer) }() } } @@ -493,7 +473,7 @@ func runAgent(agentName, fullsendDir, outputBase, targetRepo, fullsendBinary str if iteration > 1 { clearCmd := fmt.Sprintf("rm -rf %s/output/* %s/*.jsonl", sandbox.SandboxWorkspace, sandbox.SandboxClaudeConfig) - if _, _, _, clearErr := sandbox.SSH(sshConfigPath, sandboxName, clearCmd, 10*time.Second); clearErr != nil { + if _, _, _, clearErr := sandbox.Exec(sandboxName, clearCmd, 10*time.Second); clearErr != nil { printer.StepWarn("Failed to clear sandbox output: " + clearErr.Error()) } } @@ -507,7 +487,7 @@ func runAgent(agentName, fullsendDir, outputBase, targetRepo, fullsendBinary str go runHeartbeat(printer, agentStart, timeout, heartbeatDone) var metrics RunMetrics - exitCode, runErr := runAgentWithProgress(sshConfigPath, sandboxName, claudeCmd, timeout, printer, agentStart, &metrics) + exitCode, runErr := runAgentWithProgress(sandboxName, claudeCmd, timeout, printer, agentStart, &metrics) close(heartbeatDone) if runErr != nil { @@ -528,7 +508,7 @@ func runAgent(agentName, fullsendDir, outputBase, targetRepo, fullsendBinary str extractStart := time.Now() printer.StepStart("Extracting output files") remoteSrc := fmt.Sprintf("%s/output", sandbox.SandboxWorkspace) - extracted, extractErr := sandbox.ExtractOutputFiles(sshConfigPath, sandboxName, remoteSrc, iterOutputDir) + extracted, extractErr := sandbox.ExtractOutputFiles(sandboxName, remoteSrc, iterOutputDir) if extractErr != nil { printer.StepWarn("Failed to extract output files: " + extractErr.Error()) } else if len(extracted) == 0 { @@ -543,18 +523,17 @@ func runAgent(agentName, fullsendDir, outputBase, targetRepo, fullsendBinary str // 9c. Extract transcripts for this iteration. transcriptStart := time.Now() printer.StepStart("Extracting transcripts") - if err := sandbox.ExtractTranscripts(sshConfigPath, sandboxName, agentName, iterTranscriptDir); err != nil { + if err := sandbox.ExtractTranscripts(sandboxName, agentName, iterTranscriptDir); err != nil { printer.StepWarn("Failed to extract transcripts: " + err.Error()) } else { printer.StepDone(fmt.Sprintf("Transcripts extracted (%.1fs)", time.Since(transcriptStart).Seconds())) } - // 9d. Extract target repo back to host. Uses rsync with --no-links - // and --exclude .git/hooks/ to prevent sandbox escape via symlinks - // or injected git hooks. + // 9d. Extract target repo back to host. SafeDownload removes symlinks + // and .git/hooks/ after download to prevent sandbox escape. repoExtractStart := time.Now() printer.StepStart("Extracting target repo") - if err := sandbox.RsyncFrom(sshConfigPath, sandboxName, repoDir, repoSrc); err != nil { + if err := sandbox.SafeDownload(sandboxName, repoDir, repoSrc); err != nil { printer.StepWarn("Failed to extract target repo: " + err.Error()) } else { printer.StepDone(fmt.Sprintf("Target repo extracted to %s (%.1fs)", repoSrc, time.Since(repoExtractStart).Seconds())) @@ -600,7 +579,7 @@ func runAgent(agentName, fullsendDir, outputBase, targetRepo, fullsendBinary str findingsDir := filepath.Join(runDir, "security") if err := os.MkdirAll(findingsDir, 0o755); err == nil { remoteFindingsDir := sandbox.SandboxWorkspace + "/.security/" - if scpErr := sandbox.SCPFrom(sshConfigPath, sandboxName, remoteFindingsDir, findingsDir); scpErr != nil { + if dlErr := sandbox.Download(sandboxName, remoteFindingsDir, findingsDir); dlErr != nil { printer.StepInfo("No sandbox security findings to extract") } else { printer.StepDone("Security findings extracted") @@ -631,14 +610,14 @@ func runAgent(agentName, fullsendDir, outputBase, targetRepo, fullsendBinary str return nil } -func bootstrapSandbox(sshConfigPath, sandboxName, repoDir, fullsendBinary string, h *harness.Harness) error { +func bootstrapSandbox(sandboxName, repoDir, fullsendBinary string, h *harness.Harness) error { // Create workspace structure and Claude config dir for transcripts. // Agent and skill definitions go in CLAUDE_CONFIG_DIR so `claude --agent` // finds them regardless of the repo's own .claude/ directory. When // CLAUDE_CONFIG_DIR is set, Claude uses it instead of ~/.claude/. mkdirCmd := fmt.Sprintf("mkdir -p %s/agents %s/skills %s/hooks %s/bin %s/.env.d %s/.security %s %s/.claude/hooks", sandbox.SandboxClaudeConfig, sandbox.SandboxClaudeConfig, sandbox.SandboxClaudeConfig, sandbox.SandboxWorkspace, sandbox.SandboxWorkspace, sandbox.SandboxWorkspace, sandbox.SandboxClaudeConfig, sandbox.SandboxWorkspace) - if _, _, _, err := sandbox.SSH(sshConfigPath, sandboxName, mkdirCmd, 10*time.Second); err != nil { + if _, _, _, err := sandbox.Exec(sandboxName, mkdirCmd, 10*time.Second); err != nil { return fmt.Errorf("creating workspace dirs: %w", err) } @@ -678,11 +657,11 @@ func bootstrapSandbox(sshConfigPath, sandboxName, repoDir, fullsendBinary string return fmt.Errorf("fullsend binary %q is not valid for the sandbox: %w", localBinary, err) } remoteBinary := fmt.Sprintf("%s/bin/fullsend", sandbox.SandboxWorkspace) - if err := sandbox.SCP(sshConfigPath, sandboxName, localBinary, remoteBinary); err != nil { + if err := sandbox.Upload(sandboxName, localBinary, remoteBinary); err != nil { return fmt.Errorf("copying fullsend binary to sandbox: %w", err) } chmodCmd := fmt.Sprintf("chmod +x %s", remoteBinary) - if _, _, _, err := sandbox.SSH(sshConfigPath, sandboxName, chmodCmd, 10*time.Second); err != nil { + if _, _, _, err := sandbox.Exec(sandboxName, chmodCmd, 10*time.Second); err != nil { return fmt.Errorf("chmod fullsend binary: %w", err) } } @@ -716,12 +695,12 @@ func bootstrapSandbox(sshConfigPath, sandboxName, repoDir, fullsendBinary string } // Copy agent definition to $CLAUDE_CONFIG_DIR/agents/. - if err := sandbox.SCP(sshConfigPath, sandboxName, h.Agent, + if err := sandbox.Upload(sandboxName, h.Agent, fmt.Sprintf("%s/agents/", sandbox.SandboxClaudeConfig)); err != nil { return fmt.Errorf("copying agent definition: %w", err) } - // Copy skills (SCP -r copies the entire directory tree, including any + // Copy skills (Upload copies the entire directory tree, including any // scripts/, references/, and assets/ bundled with the skill per the // agentskills.io specification). for _, skillPath := range h.Skills { @@ -754,20 +733,20 @@ func bootstrapSandbox(sshConfigPath, sandboxName, repoDir, fullsendBinary string } } - if err := sandbox.SCP(sshConfigPath, sandboxName, skillPath, + if err := sandbox.Upload(sandboxName, skillPath, fmt.Sprintf("%s/skills/", sandbox.SandboxClaudeConfig)); err != nil { return fmt.Errorf("copying skill %q: %w", skillPath, err) } } // Write .env file (infrastructure vars) and copy host files. - if err := bootstrapEnv(sshConfigPath, sandboxName, repoDir, h); err != nil { + if err := bootstrapEnv(sandboxName, repoDir, h); err != nil { return fmt.Errorf("bootstrapping environment: %w", err) } // Install security hooks if enabled. if h.SecurityEnabled() { - if err := bootstrapSecurityHooks(sshConfigPath, sandboxName, h); err != nil { + if err := bootstrapSecurityHooks(sandboxName, h); err != nil { return fmt.Errorf("bootstrapping security hooks: %w", err) } } @@ -786,7 +765,7 @@ func bootstrapSandbox(sshConfigPath, sandboxName, repoDir, fullsendBinary string // host_files entries copy files from the host into the sandbox at specified // destination paths. Src values may contain ${VAR} references expanded from // the host environment. When expand is true, file content is also expanded. -func bootstrapEnv(sshConfigPath, sandboxName, repoDir string, h *harness.Harness) error { +func bootstrapEnv(sandboxName, repoDir string, h *harness.Harness) error { remoteEnvFile := sandbox.SandboxWorkspace + "/.env" outputDir := sandbox.SandboxWorkspace + "/output" @@ -815,7 +794,7 @@ func bootstrapEnv(sshConfigPath, sandboxName, repoDir string, h *harness.Harness } tmpFile.Close() - if err := sandbox.SCP(sshConfigPath, sandboxName, tmpFile.Name(), remoteEnvFile); err != nil { + if err := sandbox.Upload(sandboxName, tmpFile.Name(), remoteEnvFile); err != nil { return fmt.Errorf("copying .env file to sandbox: %w", err) } @@ -853,13 +832,13 @@ func bootstrapEnv(sshConfigPath, sandboxName, repoDir string, h *harness.Harness } tmp.Close() - if err := sandbox.SCP(sshConfigPath, sandboxName, tmp.Name(), hf.Dest); err != nil { + if err := sandbox.Upload(sandboxName, tmp.Name(), hf.Dest); err != nil { os.Remove(tmp.Name()) return fmt.Errorf("copying expanded file %s to %s: %w", hf.Src, hf.Dest, err) } os.Remove(tmp.Name()) } else { - if err := sandbox.SCP(sshConfigPath, sandboxName, hostPath, hf.Dest); err != nil { + if err := sandbox.Upload(sandboxName, hostPath, hf.Dest); err != nil { return fmt.Errorf("copying host file %s to %s: %w", hf.Src, hf.Dest, err) } } @@ -871,8 +850,8 @@ func bootstrapEnv(sshConfigPath, sandboxName, repoDir string, h *harness.Harness // https://github.com/fullsend-ai/fullsend/issues/345#issuecomment-4300740512 if strings.Contains(hf.Dest, "/bin/") { chmodCmd := fmt.Sprintf("chmod +x %s", hf.Dest) - if _, _, _, sshErr := sandbox.SSH(sshConfigPath, sandboxName, chmodCmd, 10*time.Second); sshErr != nil { - return fmt.Errorf("chmod host file %s in sandbox: %w", hf.Dest, sshErr) + if _, _, _, execErr := sandbox.Exec(sandboxName, chmodCmd, 10*time.Second); execErr != nil { + return fmt.Errorf("chmod host file %s in sandbox: %w", hf.Dest, execErr) } } } @@ -894,8 +873,8 @@ func envToList(env map[string]string) []string { return list } -func runAgentWithProgress(sshConfigPath, sandboxName, claudeCmd string, timeout time.Duration, printer *ui.Printer, start time.Time, metrics *RunMetrics) (int, error) { - stdout, cmd, cancel, err := sandbox.SSHStreamReader(sshConfigPath, sandboxName, claudeCmd, timeout, os.Stderr) +func runAgentWithProgress(sandboxName, claudeCmd string, timeout time.Duration, printer *ui.Printer, start time.Time, metrics *RunMetrics) (int, error) { + stdout, cmd, cancel, err := sandbox.ExecStreamReader(sandboxName, claudeCmd, timeout, os.Stderr) if err != nil { return -1, err } @@ -914,7 +893,7 @@ func runAgentWithProgress(sshConfigPath, sandboxName, claudeCmd string, timeout } if waitErr != nil && cmd.ProcessState == nil { - return exitCode, fmt.Errorf("ssh failed: %w", waitErr) + return exitCode, fmt.Errorf("openshell exec failed: %w", waitErr) } return exitCode, nil @@ -961,7 +940,7 @@ func readOIDCAuthFile(path string) (string, error) { var oidcRefreshInterval = 4 * time.Minute -func runOIDCRefresh(ctx context.Context, sshConfigPath, sandboxName, oidcURL, oidcAuth string, printer *ui.Printer) { +func runOIDCRefresh(ctx context.Context, sandboxName, oidcURL, oidcAuth string, printer *ui.Printer) { ticker := time.NewTicker(oidcRefreshInterval) defer ticker.Stop() @@ -970,7 +949,7 @@ func runOIDCRefresh(ctx context.Context, sshConfigPath, sandboxName, oidcURL, oi case <-ctx.Done(): return case <-ticker.C: - if err := refreshOIDCToken(ctx, sshConfigPath, sandboxName, oidcURL, oidcAuth); err != nil { + if err := refreshOIDCToken(ctx, sandboxName, oidcURL, oidcAuth); err != nil { if ctx.Err() != nil { return } @@ -982,7 +961,7 @@ func runOIDCRefresh(ctx context.Context, sshConfigPath, sandboxName, oidcURL, oi } } -func refreshOIDCToken(ctx context.Context, sshConfigPath, sandboxName, oidcURL, oidcAuth string) error { +func refreshOIDCToken(ctx context.Context, sandboxName, oidcURL, oidcAuth string) error { req, err := http.NewRequestWithContext(ctx, "GET", oidcURL, nil) if err != nil { return fmt.Errorf("creating request: %w", err) @@ -1023,7 +1002,7 @@ func refreshOIDCToken(ctx context.Context, sshConfigPath, sandboxName, oidcURL, tmpFile.Close() remotePath := sandbox.SandboxWorkspace + "/.gcp-oidc-token" - if err := sandbox.SCP(sshConfigPath, sandboxName, tmpFile.Name(), remotePath); err != nil { + if err := sandbox.Upload(sandboxName, tmpFile.Name(), remotePath); err != nil { return fmt.Errorf("copying token to sandbox: %w", err) } @@ -1045,7 +1024,7 @@ func buildClaudeCommand(agentName, model, repoDir string) string { // --verbose increases log output in the job log. If artifact upload is // added to this workflow, consider whether verbose output should be // redacted or made conditional via an env var. - "cd %s && source %s && claude --print --verbose --output-format stream-json %s--agent '%s' --dangerously-skip-permissions 'Run the agent task'", + "cd %s && . %s && claude --print --verbose --output-format stream-json %s--agent '%s' --dangerously-skip-permissions 'Run the agent task'", repoDir, envFile, modelFlag, safe, ) } @@ -1055,7 +1034,7 @@ func buildClaudeCommand(agentName, model, repoDir string) string { // (buildScanContextCommand) scans to ensure parity. const maxContextScanDepth = 5 -// buildScanContextCommand builds the SSH command to run `fullsend scan context` +// buildScanContextCommand builds the command to run `fullsend scan context` // inside the sandbox. It finds known context files (including SKILL.md in // skill directories) in the repo directory and passes them as arguments. func buildScanContextCommand(repoDir, traceID string) string { @@ -1094,7 +1073,7 @@ func buildScanContextCommand(repoDir, traceID string) string { envFile := sandbox.SandboxWorkspace + "/.env" return fmt.Sprintf( - "source %s && FULLSEND_TRACE_ID='%s' find '%s' -maxdepth %d -type f \\( %s \\) -exec fullsend scan context {} +", + ". %s && FULLSEND_TRACE_ID='%s' find '%s' -maxdepth %d -type f \\( %s \\) -exec fullsend scan context {} +", envFile, traceID, escapedDir, maxContextScanDepth, inameExpr, ) } @@ -1326,7 +1305,7 @@ func scanOutputFiles(outputDir, traceID string, printer *ui.Printer) error { // bootstrapSecurityHooks installs Claude Code hook scripts and settings.json // inside the sandbox. Hook scripts are embedded in the binary via go:embed. -func bootstrapSecurityHooks(sshConfigPath, sandboxName string, h *harness.Harness) error { +func bootstrapSecurityHooks(sandboxName string, h *harness.Harness) error { // Write hook scripts. hookFiles := security.HookFiles(h) for name, content := range hookFiles { @@ -1342,7 +1321,7 @@ func bootstrapSecurityHooks(sshConfigPath, sandboxName string, h *harness.Harnes tmpFile.Close() remotePath := fmt.Sprintf("%s/.claude/hooks/%s", sandbox.SandboxWorkspace, name) - if err := sandbox.SCP(sshConfigPath, sandboxName, tmpFile.Name(), remotePath); err != nil { + if err := sandbox.Upload(sandboxName, tmpFile.Name(), remotePath); err != nil { os.Remove(tmpFile.Name()) return fmt.Errorf("copying hook %s to sandbox: %w", name, err) } @@ -1350,7 +1329,7 @@ func bootstrapSecurityHooks(sshConfigPath, sandboxName string, h *harness.Harnes // Make executable. chmodCmd := fmt.Sprintf("chmod +x %s", remotePath) - if _, _, _, err := sandbox.SSH(sshConfigPath, sandboxName, chmodCmd, 10*time.Second); err != nil { + if _, _, _, err := sandbox.Exec(sandboxName, chmodCmd, 10*time.Second); err != nil { return fmt.Errorf("chmod hook %s: %w", name, err) } } @@ -1373,7 +1352,7 @@ func bootstrapSecurityHooks(sshConfigPath, sandboxName string, h *harness.Harnes tmpSettings.Close() remoteSettings := fmt.Sprintf("%s/.claude/settings.json", sandbox.SandboxWorkspace) - if err := sandbox.SCP(sshConfigPath, sandboxName, tmpSettings.Name(), remoteSettings); err != nil { + if err := sandbox.Upload(sandboxName, tmpSettings.Name(), remoteSettings); err != nil { os.Remove(tmpSettings.Name()) return fmt.Errorf("copying settings.json to sandbox: %w", err) } @@ -1390,7 +1369,7 @@ func bootstrapSecurityHooks(sshConfigPath, sandboxName string, h *harness.Harnes escapedFailOn := strings.ReplaceAll(tirithCfg.FailOn, "'", "'\\''") envCmd := fmt.Sprintf("echo 'export TIRITH_FAIL_ON=%s' >> %s/.env", escapedFailOn, sandbox.SandboxWorkspace) - if _, _, _, err := sandbox.SSH(sshConfigPath, sandboxName, envCmd, 10*time.Second); err != nil { + if _, _, _, err := sandbox.Exec(sandboxName, envCmd, 10*time.Second); err != nil { return fmt.Errorf("setting TIRITH_FAIL_ON: %w", err) } } @@ -1399,7 +1378,7 @@ func bootstrapSecurityHooks(sshConfigPath, sandboxName string, h *harness.Harnes // fails closed if the binary is missing from the sandbox image. if harness.BoolDefault(tirithCfg.Enabled, true) { envCmd := fmt.Sprintf("echo 'export TIRITH_REQUIRED=1' >> %s/.env", sandbox.SandboxWorkspace) - if _, _, _, err := sandbox.SSH(sshConfigPath, sandboxName, envCmd, 10*time.Second); err != nil { + if _, _, _, err := sandbox.Exec(sandboxName, envCmd, 10*time.Second); err != nil { return fmt.Errorf("setting TIRITH_REQUIRED: %w", err) } } @@ -1409,13 +1388,13 @@ func bootstrapSecurityHooks(sshConfigPath, sandboxName string, h *harness.Harnes } // injectTraceID appends the FULLSEND_TRACE_ID to the sandbox .env file. -func injectTraceID(sshConfigPath, sandboxName, traceID string) error { +func injectTraceID(sandboxName, traceID string) error { if !security.IsValidTraceID(traceID) { return fmt.Errorf("invalid trace ID format: %q", traceID) } // Safe: IsValidTraceID() above ensures traceID matches UUID v4 format only. cmd := fmt.Sprintf("echo 'export FULLSEND_TRACE_ID=%s' >> %s/.env", traceID, sandbox.SandboxWorkspace) - _, _, _, err := sandbox.SSH(sshConfigPath, sandboxName, cmd, 10*time.Second) + _, _, _, err := sandbox.Exec(sandboxName, cmd, 10*time.Second) return err } diff --git a/internal/cli/run_test.go b/internal/cli/run_test.go index 35aac9a599..ed0c1666b9 100644 --- a/internal/cli/run_test.go +++ b/internal/cli/run_test.go @@ -100,7 +100,7 @@ func TestBuildClaudeCommand_EscapesQuotes(t *testing.T) { func TestBuildScanContextCommand_SourcesEnv(t *testing.T) { traceID := "aabbccdd-1122-4334-8556-aabbccddeeff" cmd := buildScanContextCommand("/tmp/workspace/repo", traceID) - assert.Contains(t, cmd, "source /tmp/workspace/.env &&") + assert.Contains(t, cmd, ". /tmp/workspace/.env &&") assert.Contains(t, cmd, "FULLSEND_TRACE_ID='"+traceID+"'") assert.Contains(t, cmd, "-exec fullsend scan context") } @@ -386,7 +386,7 @@ func TestRefreshOIDCToken_FetchSucceedsSCPFails(t *testing.T) { })) defer srv.Close() - err := refreshOIDCToken(context.Background(), "nonexistent-ssh-config", "nonexistent-sandbox", srv.URL, "bearer test-auth") + err := refreshOIDCToken(context.Background(), "nonexistent-sandbox", srv.URL, "bearer test-auth") require.Error(t, err) assert.Contains(t, err.Error(), "copying token to sandbox") } @@ -397,7 +397,7 @@ func TestRefreshOIDCToken_HTTPError(t *testing.T) { })) defer srv.Close() - err := refreshOIDCToken(context.Background(), "nonexistent-ssh-config", "nonexistent-sandbox", srv.URL, "bearer test-auth") + err := refreshOIDCToken(context.Background(), "nonexistent-sandbox", srv.URL, "bearer test-auth") require.Error(t, err) assert.Contains(t, err.Error(), "403") } @@ -408,7 +408,7 @@ func TestRefreshOIDCToken_EmptyResponse(t *testing.T) { })) defer srv.Close() - err := refreshOIDCToken(context.Background(), "nonexistent-ssh-config", "nonexistent-sandbox", srv.URL, "bearer test-auth") + err := refreshOIDCToken(context.Background(), "nonexistent-sandbox", srv.URL, "bearer test-auth") require.Error(t, err) assert.Contains(t, err.Error(), "empty token") } @@ -419,7 +419,7 @@ func TestRefreshOIDCToken_NonJSONResponse(t *testing.T) { })) defer srv.Close() - err := refreshOIDCToken(context.Background(), "nonexistent-ssh-config", "nonexistent-sandbox", srv.URL, "bearer test-auth") + err := refreshOIDCToken(context.Background(), "nonexistent-sandbox", srv.URL, "bearer test-auth") require.Error(t, err) assert.Contains(t, err.Error(), "non-JSON response") } @@ -433,7 +433,7 @@ func TestRefreshOIDCToken_CancelledContext(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) cancel() - err := refreshOIDCToken(ctx, "nonexistent-ssh-config", "nonexistent-sandbox", srv.URL, "bearer test-auth") + err := refreshOIDCToken(ctx, "nonexistent-sandbox", srv.URL, "bearer test-auth") require.Error(t, err) assert.Contains(t, err.Error(), "fetching OIDC token") } @@ -455,7 +455,7 @@ func TestRunOIDCRefresh_TicksAndStops(t *testing.T) { finished := make(chan struct{}) go func() { - runOIDCRefresh(ctx, "nonexistent-ssh-config", "nonexistent-sandbox", srv.URL, "bearer test-auth", printer) + runOIDCRefresh(ctx, "nonexistent-sandbox", srv.URL, "bearer test-auth", printer) close(finished) }() diff --git a/internal/sandbox/sandbox.go b/internal/sandbox/sandbox.go index 2da531e347..a81e747143 100644 --- a/internal/sandbox/sandbox.go +++ b/internal/sandbox/sandbox.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "io" + "io/fs" "os" "os/exec" "path/filepath" @@ -23,6 +24,26 @@ const ( transferTimeout = 5 * time.Minute ) +func sanitizeDownload(localDir string) error { + return filepath.WalkDir(localDir, func(path string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if d.Type()&fs.ModeSymlink != 0 { + return os.Remove(path) + } + + if d.IsDir() && d.Name() == "hooks" && filepath.Base(filepath.Dir(path)) == ".git" { + if err := os.RemoveAll(path); err != nil { + return fmt.Errorf("removing .git/hooks: %w", err) + } + return filepath.SkipDir + } + + return nil + }) +} + // EnsureProvider creates or updates a provider on the gateway. Credential // values may contain ${VAR} references which are expanded from the host // environment before being passed to openshell. @@ -163,45 +184,18 @@ func Delete(name string) error { return nil } -// GetSSHConfig retrieves the SSH config for a sandbox. -func GetSSHConfig(name string) (string, error) { - out, err := exec.Command("openshell", "sandbox", "ssh-config", name).Output() - if err != nil { - return "", fmt.Errorf("getting SSH config for sandbox %q: %w", name, err) - } - return string(out), nil -} - -// SCP copies a local file or directory into a sandbox. -func SCP(sshConfigPath, sandboxName, localPath, remotePath string) error { - ctx, cancel := context.WithTimeout(context.Background(), transferTimeout) +// Exec runs a command inside a sandbox and returns stdout, stderr, and exit code. +func Exec(sandboxName, command string, timeout time.Duration) (stdout, stderr string, exitCode int, err error) { + ctx, cancel := context.WithTimeout(context.Background(), timeout+10*time.Second) defer cancel() - cmd := exec.CommandContext(ctx, "scp", - "-F", sshConfigPath, - "-r", - localPath, - fmt.Sprintf("openshell-%s:%s", sandboxName, remotePath), - ) - out, err := cmd.CombinedOutput() - if err != nil { - if ctx.Err() != nil { - return fmt.Errorf("scp to sandbox %q timed out after %s", sandboxName, transferTimeout) - } - return fmt.Errorf("scp to sandbox %q failed: %s: %w", sandboxName, string(out), err) - } - return nil -} - -// SSH runs a command inside a sandbox and returns stdout, stderr, and exit code. -func SSH(sshConfigPath, sandboxName, command string, timeout time.Duration) (stdout, stderr string, exitCode int, err error) { - ctx, cancel := context.WithTimeout(context.Background(), timeout) - defer cancel() + timeoutSecs := fmt.Sprintf("%d", int(timeout.Seconds())) - cmd := exec.CommandContext(ctx, "ssh", - "-F", sshConfigPath, - fmt.Sprintf("openshell-%s", sandboxName), - command, + cmd := exec.CommandContext(ctx, "openshell", "sandbox", "exec", + "--name", sandboxName, + "--no-tty", + "--timeout", timeoutSecs, + "--", "sh", "-c", command, ) var stdoutBuf, stderrBuf strings.Builder @@ -214,58 +208,30 @@ func SSH(sshConfigPath, sandboxName, command string, timeout time.Duration) (std exitCode = cmd.ProcessState.ExitCode() } - if runErr != nil && ctx.Err() != nil { - return stdoutBuf.String(), stderrBuf.String(), exitCode, - fmt.Errorf("ssh command timed out after %s", timeout) - } - if runErr != nil && cmd.ProcessState == nil { - return "", "", exitCode, fmt.Errorf("ssh failed to start: %w", runErr) + return "", "", exitCode, fmt.Errorf("openshell exec failed to start: %w", runErr) } - return stdoutBuf.String(), stderrBuf.String(), exitCode, nil -} - -// SSHStream runs a command inside a sandbox, streaming output to the given writers. -func SSHStream(sshConfigPath, sandboxName, command string, timeout time.Duration, stdoutW, stderrW *os.File) (int, error) { - ctx, cancel := context.WithTimeout(context.Background(), timeout) - defer cancel() - - cmd := exec.CommandContext(ctx, "ssh", - "-F", sshConfigPath, - fmt.Sprintf("openshell-%s", sandboxName), - command, - ) - cmd.Stdout = stdoutW - cmd.Stderr = stderrW - - err := cmd.Run() - exitCode := -1 - if cmd.ProcessState != nil { - exitCode = cmd.ProcessState.ExitCode() - } - - if err != nil && ctx.Err() != nil { - return exitCode, fmt.Errorf("ssh command timed out after %s", timeout) - } - - if err != nil && cmd.ProcessState == nil { - return exitCode, fmt.Errorf("ssh failed to start: %w", err) + if exitCode == 124 { + return stdoutBuf.String(), stderrBuf.String(), exitCode, + fmt.Errorf("command timed out after %s", timeout) } - return exitCode, nil + return stdoutBuf.String(), stderrBuf.String(), exitCode, nil } -// SSHStreamReader runs a command inside a sandbox, returning an io.ReadCloser for +// ExecStreamReader runs a command inside a sandbox, returning an io.ReadCloser for // stdout so the caller can parse structured output. Stderr is forwarded to the // given writer. The caller must read stdout to completion, then call cmd.Wait(). -func SSHStreamReader(sshConfigPath, sandboxName, command string, timeout time.Duration, stderrW io.Writer) (io.ReadCloser, *exec.Cmd, context.CancelFunc, error) { +func ExecStreamReader(sandboxName, command string, timeout time.Duration, stderrW io.Writer) (io.ReadCloser, *exec.Cmd, context.CancelFunc, error) { ctx, cancel := context.WithTimeout(context.Background(), timeout) + timeoutSecs := fmt.Sprintf("%d", int(timeout.Seconds())) - cmd := exec.CommandContext(ctx, "ssh", - "-F", sshConfigPath, - fmt.Sprintf("openshell-%s", sandboxName), - command, + cmd := exec.CommandContext(ctx, "openshell", "sandbox", "exec", + "--name", sandboxName, + "--no-tty", + "--timeout", timeoutSecs, + "--", "sh", "-c", command, ) cmd.Stderr = stderrW @@ -277,68 +243,81 @@ func SSHStreamReader(sshConfigPath, sandboxName, command string, timeout time.Du if err := cmd.Start(); err != nil { cancel() - return nil, nil, nil, fmt.Errorf("starting ssh command: %w", err) + return nil, nil, nil, fmt.Errorf("starting openshell exec: %w", err) } return stdout, cmd, cancel, nil } -// RsyncFrom copies a directory from a sandbox to the local machine using rsync -// with safety flags: symlinks are skipped (--no-links) and .git/hooks/ is -// excluded to prevent a compromised sandbox from injecting executable content -// into the host repo. Requires rsync on both host and sandbox. -func RsyncFrom(sshConfigPath, sandboxName, remoteDir, localDir string) error { - // Trailing slashes ensure rsync copies contents, not the directory itself. - if !strings.HasSuffix(remoteDir, "/") { - remoteDir += "/" - } - if !strings.HasSuffix(localDir, "/") { - localDir += "/" - } - +// Upload copies a local file or directory into a sandbox. +func Upload(sandboxName, localPath, remotePath string) error { ctx, cancel := context.WithTimeout(context.Background(), transferTimeout) defer cancel() - remote := fmt.Sprintf("openshell-%s:%s", sandboxName, remoteDir) - cmd := exec.CommandContext(ctx, "rsync", - "-a", - "--no-links", - "--exclude", ".git/hooks/", - "-e", fmt.Sprintf("ssh -F %s", sshConfigPath), - remote, - localDir, + cmd := exec.CommandContext(ctx, "openshell", "sandbox", "upload", + sandboxName, + localPath, + remotePath, ) out, err := cmd.CombinedOutput() if err != nil { if ctx.Err() != nil { - return fmt.Errorf("rsync from sandbox %q timed out after %s", sandboxName, transferTimeout) + return fmt.Errorf("upload to sandbox %q timed out after %s", sandboxName, transferTimeout) } - return fmt.Errorf("rsync from sandbox %q failed: %s: %w", sandboxName, string(out), err) + return fmt.Errorf("upload to sandbox %q failed: %s: %w", sandboxName, string(out), err) } return nil } -// SCPFrom copies a file or directory from a sandbox to the local machine. -func SCPFrom(sshConfigPath, sandboxName, remotePath, localPath string) error { +// Download copies a file or directory from a sandbox to the local machine. +// The localPath is always treated as a directory by openshell — for single-file +// downloads use DownloadFile instead. +func Download(sandboxName, remotePath, localPath string) error { ctx, cancel := context.WithTimeout(context.Background(), transferTimeout) defer cancel() - cmd := exec.CommandContext(ctx, "scp", - "-F", sshConfigPath, - "-r", - fmt.Sprintf("openshell-%s:%s", sandboxName, remotePath), + cmd := exec.CommandContext(ctx, "openshell", "sandbox", "download", + sandboxName, + remotePath, localPath, ) out, err := cmd.CombinedOutput() if err != nil { if ctx.Err() != nil { - return fmt.Errorf("scp from sandbox %q timed out after %s", sandboxName, transferTimeout) + return fmt.Errorf("download from sandbox %q timed out after %s", sandboxName, transferTimeout) } - return fmt.Errorf("scp from sandbox %q failed: %s: %w", sandboxName, string(out), err) + return fmt.Errorf("download from sandbox %q failed: %s: %w", sandboxName, string(out), err) } return nil } +// DownloadFile copies a single file from a sandbox to a specific local path. +// openshell sandbox download always treats the destination as a directory, so +// this downloads to the parent directory and renames if the resulting filename +// differs from the desired local name. +func DownloadFile(sandboxName, remotePath, localPath string) error { + destDir := filepath.Dir(localPath) + downloadedPath := filepath.Join(destDir, filepath.Base(remotePath)) + + os.Remove(downloadedPath) + if err := Download(sandboxName, remotePath, destDir); err != nil { + return err + } + if downloadedPath != localPath { + return os.Rename(downloadedPath, localPath) + } + return nil +} + +// SafeDownload copies a directory from a sandbox to the local machine and then +// sanitizes the result by removing symlinks and .git/hooks/. +func SafeDownload(sandboxName, remoteDir, localDir string) error { + if err := Download(sandboxName, remoteDir, localDir); err != nil { + return err + } + return sanitizeDownload(localDir) +} + // CollectLogs runs `openshell logs --source -n 0` and returns // the log output. The -n 0 flag requests all available log lines (no limit). // This is a host-side command that talks to the gateway — no SSH needed. @@ -359,13 +338,18 @@ func CollectLogs(name, source string) (string, error) { // ExtractTranscripts copies Claude transcript files (.jsonl) from the sandbox // to a local output directory. -func ExtractTranscripts(sshConfigPath, sandboxName, agentName, outputDir string) error { +func ExtractTranscripts(sandboxName, agentName, outputDir string) error { if err := os.MkdirAll(outputDir, 0o755); err != nil { return fmt.Errorf("creating output dir: %w", err) } - // Find transcript files in the sandbox. - stdout, _, _, err := SSH(sshConfigPath, sandboxName, + root, err := os.OpenRoot(outputDir) + if err != nil { + return fmt.Errorf("opening output root: %w", err) + } + defer root.Close() + + stdout, _, _, err := Exec(sandboxName, fmt.Sprintf("find %s -name '*.jsonl' 2>/dev/null || true", SandboxClaudeConfig), 10*time.Second, ) @@ -380,24 +364,26 @@ func ExtractTranscripts(sshConfigPath, sandboxName, agentName, outputDir string) } files := strings.Split(trimmed, "\n") - cleanBase := filepath.Clean(outputDir) + string(filepath.Separator) - for _, remotePath := range files { remotePath = strings.TrimSpace(remotePath) if remotePath == "" { continue } localName := fmt.Sprintf("%s-%s", agentName, filepath.Base(remotePath)) - localPath := filepath.Join(outputDir, localName) - // Prevent path traversal from sandbox-controlled filenames. - if !strings.HasPrefix(filepath.Clean(localPath), cleanBase) { - fmt.Fprintf(os.Stderr, " [%s] Skipping path traversal attempt: %s\n", agentName, localName) + // Validate path stays within outputDir (kernel-enforced), then remove + // the probe file so DownloadFile can write the actual content. + f, createErr := root.Create(localName) + if createErr != nil { + fmt.Fprintf(os.Stderr, " [%s] Skipping (path rejected): %s: %v\n", agentName, localName, createErr) continue } + f.Close() - if scpErr := SCPFrom(sshConfigPath, sandboxName, remotePath, localPath); scpErr != nil { - fmt.Fprintf(os.Stderr, " [%s] Failed to copy transcript: %v\n", agentName, scpErr) + localPath := filepath.Join(outputDir, localName) + os.Remove(localPath) + if dlErr := DownloadFile(sandboxName, remotePath, localPath); dlErr != nil { + fmt.Fprintf(os.Stderr, " [%s] Failed to copy transcript: %v\n", agentName, dlErr) continue } fmt.Fprintf(os.Stderr, " [%s] Saved transcript: %s\n", agentName, localName) @@ -408,13 +394,18 @@ func ExtractTranscripts(sshConfigPath, sandboxName, agentName, outputDir string) // ExtractOutputFiles copies all files under a remote directory in the sandbox // to a local output directory, preserving relative paths. -func ExtractOutputFiles(sshConfigPath, sandboxName, remoteDir, localDir string) ([]string, error) { +func ExtractOutputFiles(sandboxName, remoteDir, localDir string) ([]string, error) { if err := os.MkdirAll(localDir, 0o755); err != nil { return nil, fmt.Errorf("creating local output dir: %w", err) } - // List files in the sandbox output directory. - stdout, _, _, err := SSH(sshConfigPath, sandboxName, + root, err := os.OpenRoot(localDir) + if err != nil { + return nil, fmt.Errorf("opening output root: %w", err) + } + defer root.Close() + + stdout, _, _, err := Exec(sandboxName, fmt.Sprintf("find %s -type f 2>/dev/null || true", remoteDir), 10*time.Second, ) @@ -428,32 +419,36 @@ func ExtractOutputFiles(sshConfigPath, sandboxName, remoteDir, localDir string) } lines := strings.Split(trimmed, "\n") - cleanBase := filepath.Clean(localDir) + string(filepath.Separator) - var extracted []string for _, remotePath := range lines { remotePath = strings.TrimSpace(remotePath) if remotePath == "" { continue } - // Preserve the relative path under remoteDir. relPath := strings.TrimPrefix(remotePath, remoteDir) relPath = strings.TrimPrefix(relPath, "/") - localPath := filepath.Join(localDir, relPath) - // Prevent path traversal from sandbox-controlled filenames. - if !strings.HasPrefix(filepath.Clean(localPath), cleanBase) { - fmt.Fprintf(os.Stderr, " Skipping path traversal attempt: %s\n", relPath) - continue + if dir := filepath.Dir(relPath); dir != "." { + if mkErr := root.MkdirAll(dir, 0o755); mkErr != nil { + fmt.Fprintf(os.Stderr, " Skipping (dir rejected): %s: %v\n", relPath, mkErr) + continue + } } - if err := os.MkdirAll(filepath.Dir(localPath), 0o755); err != nil { - fmt.Fprintf(os.Stderr, " Failed to create dir for %s: %v\n", relPath, err) + // Validate path stays within localDir (kernel-enforced), then remove + // the probe file so DownloadFile can write the actual content. + f, createErr := root.Create(relPath) + if createErr != nil { + fmt.Fprintf(os.Stderr, " Skipping (path rejected): %s: %v\n", relPath, createErr) continue } + f.Close() + + localPath := filepath.Join(localDir, relPath) + os.Remove(localPath) - if scpErr := SCPFrom(sshConfigPath, sandboxName, remotePath, localPath); scpErr != nil { - fmt.Fprintf(os.Stderr, " Failed to copy %s: %v\n", relPath, scpErr) + if dlErr := DownloadFile(sandboxName, remotePath, localPath); dlErr != nil { + fmt.Fprintf(os.Stderr, " Failed to copy %s: %v\n", relPath, dlErr) continue } extracted = append(extracted, localPath) diff --git a/internal/sandbox/sandbox_test.go b/internal/sandbox/sandbox_test.go index 09617cbf72..e62db23768 100644 --- a/internal/sandbox/sandbox_test.go +++ b/internal/sandbox/sandbox_test.go @@ -1,9 +1,11 @@ package sandbox import ( + "os" "path/filepath" "strings" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -119,28 +121,126 @@ func TestCollectLogs_InvalidSource(t *testing.T) { assert.Error(t, err) } -func TestPathTraversalContainment(t *testing.T) { - // Simulate the containment check used in ExtractOutputFiles. - localDir := "/tmp/output" - cleanBase := filepath.Clean(localDir) + string(filepath.Separator) - - tests := []struct { - name string - relPath string - safe bool - }{ - {"normal file", "report.md", true}, - {"nested file", "subdir/report.md", true}, - {"traversal", "../../../etc/passwd", false}, - {"traversal with prefix", "../../home/runner/.bashrc", false}, - {"dot segments in middle", "subdir/../../etc/shadow", false}, - } +func TestExec_OpenshellNotInPath(t *testing.T) { + t.Setenv("PATH", "") - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - localPath := filepath.Join(localDir, tt.relPath) - contained := strings.HasPrefix(filepath.Clean(localPath), cleanBase) - assert.Equal(t, tt.safe, contained, "relPath=%q localPath=%q", tt.relPath, localPath) - }) - } + _, _, _, err := Exec("test-sandbox", "echo hello", 10*time.Second) + assert.Error(t, err) +} + +func TestOsRootContainment(t *testing.T) { + dir := t.TempDir() + + root, err := os.OpenRoot(dir) + require.NoError(t, err) + defer root.Close() + + f, err := root.Create("safe.txt") + require.NoError(t, err) + f.Close() + + _, err = root.Create("../../../etc/passwd") + assert.Error(t, err) + + _, err = root.Create("../../home/runner/.bashrc") + assert.Error(t, err) + + _, err = root.Create("subdir/../../etc/shadow") + assert.Error(t, err) +} + +func TestSanitizeDownload_RemovesSymlinks(t *testing.T) { + dir := t.TempDir() + + // Create a regular file. + require.NoError(t, os.WriteFile(filepath.Join(dir, "real.txt"), []byte("ok"), 0o644)) + + // Create a symlink (dangling is fine — we just need it to exist). + require.NoError(t, os.Symlink("/nonexistent/target", filepath.Join(dir, "danger"))) + + err := sanitizeDownload(dir) + require.NoError(t, err) + + // Regular file should survive. + _, err = os.Stat(filepath.Join(dir, "real.txt")) + assert.NoError(t, err) + + // Symlink should be removed. + _, err = os.Lstat(filepath.Join(dir, "danger")) + assert.True(t, os.IsNotExist(err), "symlink should have been removed") +} + +func TestSanitizeDownload_RemovesGitHooks(t *testing.T) { + dir := t.TempDir() + + // Create .git/hooks/ with a script. + hooksDir := filepath.Join(dir, ".git", "hooks") + require.NoError(t, os.MkdirAll(hooksDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(hooksDir, "pre-commit"), []byte("#!/bin/sh\nmalicious"), 0o755)) + + // Create a safe file under .git/. + require.NoError(t, os.WriteFile(filepath.Join(dir, ".git", "config"), []byte("[core]"), 0o644)) + + err := sanitizeDownload(dir) + require.NoError(t, err) + + // .git/hooks/ should be removed entirely. + _, err = os.Stat(hooksDir) + assert.True(t, os.IsNotExist(err), ".git/hooks/ should have been removed") + + // .git/config should survive. + _, err = os.Stat(filepath.Join(dir, ".git", "config")) + assert.NoError(t, err) +} + +func TestSanitizeDownload_NestedSymlinks(t *testing.T) { + dir := t.TempDir() + + // Create nested structure with symlinks at various depths. + require.NoError(t, os.MkdirAll(filepath.Join(dir, "a", "b"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "a", "b", "real.txt"), []byte("ok"), 0o644)) + require.NoError(t, os.Symlink("/etc/passwd", filepath.Join(dir, "a", "b", "link"))) + require.NoError(t, os.Symlink("/etc/shadow", filepath.Join(dir, "a", "top-link"))) + + err := sanitizeDownload(dir) + require.NoError(t, err) + + // Real file survives. + _, err = os.Stat(filepath.Join(dir, "a", "b", "real.txt")) + assert.NoError(t, err) + + // Both symlinks removed. + _, err = os.Lstat(filepath.Join(dir, "a", "b", "link")) + assert.True(t, os.IsNotExist(err)) + _, err = os.Lstat(filepath.Join(dir, "a", "top-link")) + assert.True(t, os.IsNotExist(err)) +} + +func TestSanitizeDownload_RemovesSubmoduleGitHooks(t *testing.T) { + dir := t.TempDir() + + // Create submodule .git/hooks/ with a script. + subHooks := filepath.Join(dir, "vendor", "dep", ".git", "hooks") + require.NoError(t, os.MkdirAll(subHooks, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(subHooks, "post-checkout"), []byte("#!/bin/sh\nmalicious"), 0o755)) + + // Create a safe file in the submodule .git/. + require.NoError(t, os.WriteFile(filepath.Join(dir, "vendor", "dep", ".git", "config"), []byte("[core]"), 0o644)) + + err := sanitizeDownload(dir) + require.NoError(t, err) + + // Submodule .git/hooks/ should be removed. + _, err = os.Stat(subHooks) + assert.True(t, os.IsNotExist(err), "submodule .git/hooks/ should have been removed") + + // Submodule .git/config should survive. + _, err = os.Stat(filepath.Join(dir, "vendor", "dep", ".git", "config")) + assert.NoError(t, err) +} + +func TestSanitizeDownload_EmptyDir(t *testing.T) { + dir := t.TempDir() + err := sanitizeDownload(dir) + assert.NoError(t, err) }