Skip to content

Commit 63286dc

Browse files
committed
fix: enforce --sandbox-readonly for write_file, patch, batch_patch
When --sandbox is active, writes from the built-in file tools are now routed into the running container via docker cp so the container's mount options (including a read-only /workspace) are actually enforced. - Add cmd/odek/sandbox_file.go with hostToContainerPath and sandboxWriteFile. - Inject the container name into writeFileTool, patchTool, and batchPatchTool in setupSandbox. - Add cmd/odek/sandbox_file_test.go with path-translation and sandbox-routing regression tests. - Update docs/SECURITY.md and AGENTS.md with sandbox read-only enforcement and volume-confinement notes.
1 parent 5f6ba39 commit 63286dc

7 files changed

Lines changed: 297 additions & 12 deletions

File tree

AGENTS.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,8 @@ Layered prompt-injection / approval-fatigue defenses. Full reference: [docs/SECU
130130
- **Session file size cap** (`internal/session/session.go`) — session files larger than 32 MiB are rejected by `Load()` to prevent OOM from tampered or corrupted transcripts.
131131
- **Skill file size cap** (`internal/skills/loader.go`) — `SKILL.md` files larger than 1 MiB are skipped so a malicious project cannot OOM the process at startup or bloat the system prompt.
132132
- **Serve sandbox default-on**`odek serve` enables `--sandbox` automatically unless `--no-sandbox` is passed.
133+
- **Sandbox volume confinement** (`internal/sandbox/sandbox.go`) — extra `--sandbox-volume` host paths must resolve to a location under the working directory, cannot contain `..` or symlink escapes, and cannot match sensitive prefixes such as `/etc`, `/proc`, `/sys`, `/dev`, `/root`, `/home`, `/var`, `/run`, or `/var/run/docker.sock`.
134+
- **Sandbox read-only enforcement** (`cmd/odek/sandbox_file.go` + `cmd/odek/file_tool.go` + `cmd/odek/perf_tools.go`) — when a sandbox container is active, `write_file`, `patch`, and `batch_patch` translate host paths to `/workspace/...` and copy data into the container with `docker cp`, so a read-only workspace mount (`--sandbox-readonly`) is enforced for the agent's own file tools.
133135
- **Telegram photo caption wrapping** (`cmd/odek/telegram.go`) — photo captions cross the Telegram trust boundary, so they are wrapped as untrusted content both when passed to the local vision model and when injected into the main agent's user message.
134136
- **Secret redaction** (`internal/redact/redact.go`) — 20+ patterns: OpenAI, Anthropic, GitHub PAT, AWS, PEM, JWT, Vault, Google OAuth, SendGrid, Discord, DB URLs, etc.
135137

cmd/odek/file_tool.go

Lines changed: 38 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -179,6 +179,9 @@ type writeFileTool struct {
179179
dangerousConfig danger.DangerousConfig
180180
trustedClasses map[danger.RiskClass]bool
181181
restrictToCWD bool // when true, reject paths escaping the working directory
182+
// containerName, when set, routes writes through the sandbox container so
183+
// that read-only workspace mounts are enforced.
184+
containerName string
182185
}
183186

184187
func (t *writeFileTool) Name() string { return "write_file" }
@@ -248,14 +251,6 @@ func (t *writeFileTool) Call(argsJSON string) (string, error) {
248251
return jsonError(err.Error())
249252
}
250253

251-
// Create parent directories
252-
dir := filepath.Dir(args.Path)
253-
if dir != "." && dir != "/" {
254-
if err := os.MkdirAll(dir, 0755); err != nil {
255-
return jsonError(fmt.Sprintf("cannot create directory %q: %v", dir, err))
256-
}
257-
}
258-
259254
// Preserve the original file's mode when overwriting, so a temp file
260255
// created with default permissions does not change the accessibility
261256
// of an existing file (e.g., making a 0640 file world-readable).
@@ -264,6 +259,26 @@ func (t *writeFileTool) Call(argsJSON string) (string, error) {
264259
origMode = st.Mode().Perm()
265260
}
266261

262+
// When sandbox mode is active, route the write through the container so a
263+
// read-only workspace mount is actually enforced.
264+
if t.containerName != "" {
265+
if err := sandboxWriteFile(t.containerName, args.Path, []byte(args.Content), origMode); err != nil {
266+
return jsonError(fmt.Sprintf("cannot write %q via sandbox: %v", args.Path, err))
267+
}
268+
return jsonResult(writeFileResult{
269+
Success: true,
270+
Path: args.Path,
271+
})
272+
}
273+
274+
// Create parent directories
275+
dir := filepath.Dir(args.Path)
276+
if dir != "." && dir != "/" {
277+
if err := os.MkdirAll(dir, 0755); err != nil {
278+
return jsonError(fmt.Sprintf("cannot create directory %q: %v", dir, err))
279+
}
280+
}
281+
267282
// Atomic write via temp file + rename to prevent TOCTOU symlink races.
268283
// os.CreateTemp creates the file in the same directory (same filesystem),
269284
// and os.Rename atomically replaces the directory entry without following
@@ -593,6 +608,9 @@ type patchTool struct {
593608
dangerousConfig danger.DangerousConfig
594609
trustedClasses map[danger.RiskClass]bool
595610
restrictToCWD bool // when true, reject paths escaping the working directory
611+
// containerName, when set, routes writes through the sandbox container so
612+
// that read-only workspace mounts are enforced.
613+
containerName string
596614
}
597615

598616
func (t *patchTool) Name() string { return "patch" }
@@ -721,6 +739,18 @@ func (t *patchTool) Call(argsJSON string) (string, error) {
721739
truncateDiff(modified, 100),
722740
)
723741

742+
// When sandbox mode is active, route the write through the container so a
743+
// read-only workspace mount is actually enforced.
744+
if t.containerName != "" {
745+
if err := sandboxWriteFile(t.containerName, args.Path, []byte(modified), origMode); err != nil {
746+
return jsonError(fmt.Sprintf("cannot write %q via sandbox: %v", args.Path, err))
747+
}
748+
return jsonResult(patchResult{
749+
Success: true,
750+
Diff: wrapUntrusted("patch:"+args.Path, diff),
751+
})
752+
}
753+
724754
// Atomic write via temp file + rename to prevent TOCTOU symlink races.
725755
// The temp file is created in the same directory (same filesystem),
726756
// and os.Rename atomically replaces the directory entry without

cmd/odek/main.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1132,6 +1132,12 @@ func setupSandbox(tools []odek.Tool, cfg sandboxConfig) (containerName string, c
11321132
tool.containerName = containerName
11331133
case *parallelShellTool:
11341134
tool.containerName = containerName
1135+
case *writeFileTool:
1136+
tool.containerName = containerName
1137+
case *patchTool:
1138+
tool.containerName = containerName
1139+
case *batchPatchTool:
1140+
tool.containerName = containerName
11351141
}
11361142
}
11371143
return containerName, cleanup, nil

cmd/odek/perf_tools.go

Lines changed: 23 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,9 @@ const maxBatchPatches = 10
7474
type batchPatchTool struct {
7575
dangerousConfig danger.DangerousConfig
7676
restrictToCWD bool // when true, reject paths escaping the working directory
77+
// containerName, when set, routes writes through the sandbox container so
78+
// that read-only workspace mounts are enforced.
79+
containerName string
7780
}
7881

7982
func (t *batchPatchTool) Name() string { return "batch_patch" }
@@ -229,14 +232,30 @@ func (t *batchPatchTool) Call(argsJSON string) (result string, err error) {
229232
diff := fmt.Sprintf("--- a/%s\n+++ b/%s\n@@ -1 +1 @@\n-%s\n+%s\n",
230233
p.Path, p.Path, truncateDiff(original, 100), truncateDiff(modified, 100))
231234

232-
// Atomic write — preserve the original file's mode and surface any
233-
// write error so a short or failed write cannot silently corrupt
234-
// the target (see IMPROVEMENTS_ROADMAP.md B-H1, B-H2).
235-
dir := filepath.Dir(p.Path)
235+
// Preserve the original file's mode.
236236
origMode := os.FileMode(0644)
237237
if st, err := os.Stat(p.Path); err == nil {
238238
origMode = st.Mode().Perm()
239239
}
240+
241+
// When sandbox mode is active, route the write through the container so
242+
// a read-only workspace mount is actually enforced.
243+
if t.containerName != "" {
244+
if err := sandboxWriteFile(t.containerName, p.Path, []byte(modified), origMode); err != nil {
245+
entry.Error = fmt.Sprintf("cannot write %q via sandbox: %v", p.Path, err)
246+
results[idx] = entry
247+
continue
248+
}
249+
entry.Success = true
250+
entry.Diff = wrapUntrusted("batch_patch:"+p.Path, diff)
251+
results[idx] = entry
252+
continue
253+
}
254+
255+
// Atomic write — preserve the original file's mode and surface any
256+
// write error so a short or failed write cannot silently corrupt
257+
// the target (see IMPROVEMENTS_ROADMAP.md B-H1, B-H2).
258+
dir := filepath.Dir(p.Path)
240259
tmpFile, err := os.CreateTemp(dir, ".tmp_batchpatch_*")
241260
if err != nil {
242261
entry.Error = fmt.Sprintf("cannot create temp file: %v", err)

cmd/odek/sandbox_file.go

Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
1+
package main
2+
3+
import (
4+
"fmt"
5+
"os"
6+
"os/exec"
7+
"path/filepath"
8+
"strings"
9+
)
10+
11+
// hostToContainerPath translates a host path (relative or absolute) into the
12+
// corresponding path inside a sandbox container. The sandbox mounts the working
13+
// directory at /workspace, so any path under the host working directory becomes
14+
// /workspace/<rel>. Paths outside the working directory are rejected.
15+
func hostToContainerPath(hostPath string) (string, error) {
16+
if hostPath == "" {
17+
return "", fmt.Errorf("path is empty")
18+
}
19+
20+
cwd, err := os.Getwd()
21+
if err != nil {
22+
return "", fmt.Errorf("cannot determine working directory: %w", err)
23+
}
24+
// Resolve symlinks in the working directory so comparisons work on hosts
25+
// where the cwd is reached through a symlink (e.g. macOS /var -> /private/var).
26+
evalCwd, err := filepath.EvalSymlinks(cwd)
27+
if err != nil {
28+
return "", fmt.Errorf("cannot resolve working directory symlinks: %w", err)
29+
}
30+
evalCwd, err = filepath.Abs(evalCwd)
31+
if err != nil {
32+
return "", fmt.Errorf("cannot resolve working directory: %w", err)
33+
}
34+
evalCwd = filepath.Clean(evalCwd)
35+
36+
absHost := hostPath
37+
if !filepath.IsAbs(absHost) {
38+
absHost = filepath.Join(evalCwd, absHost)
39+
}
40+
absHost, err = filepath.Abs(absHost)
41+
if err != nil {
42+
return "", fmt.Errorf("cannot resolve path %q: %w", hostPath, err)
43+
}
44+
absHost = filepath.Clean(absHost)
45+
46+
// Resolve symlinks in the host path. If the path does not exist yet,
47+
// resolve its directory and keep the final component.
48+
evalHost := absHost
49+
if ev, err := filepath.EvalSymlinks(absHost); err == nil {
50+
evalHost = filepath.Clean(ev)
51+
} else {
52+
dir := filepath.Dir(absHost)
53+
base := filepath.Base(absHost)
54+
if evDir, err := filepath.EvalSymlinks(dir); err == nil {
55+
evalHost = filepath.Join(evDir, base)
56+
}
57+
}
58+
59+
if evalHost != evalCwd && !strings.HasPrefix(evalHost, evalCwd+string(filepath.Separator)) {
60+
return "", fmt.Errorf("path %q is outside working directory %q", hostPath, evalCwd)
61+
}
62+
63+
rel, err := filepath.Rel(evalCwd, evalHost)
64+
if err != nil {
65+
return "", fmt.Errorf("cannot relativise %q: %w", hostPath, err)
66+
}
67+
if rel == "." {
68+
return "/workspace", nil
69+
}
70+
return "/workspace/" + filepath.ToSlash(rel), nil
71+
}
72+
73+
// sandboxWriteFile writes data to a path inside a running sandbox container.
74+
// It is used by the file tools when sandbox mode is active so that writes go
75+
// through the container's filesystem view and respect its mount options (e.g.
76+
// a read-only /workspace mount will cause this to fail).
77+
//
78+
// The implementation writes the content to a temporary file on the host and
79+
// then copies it into the container with `docker cp`, creating parent
80+
// directories with `docker exec mkdir -p` first.
81+
func sandboxWriteFile(containerName, hostPath string, data []byte, mode os.FileMode) error {
82+
containerPath, err := hostToContainerPath(hostPath)
83+
if err != nil {
84+
return err
85+
}
86+
87+
// Write content to a host temp file. docker cp needs a real file path.
88+
tmpFile, err := os.CreateTemp("", "odek-sandbox-write-*")
89+
if err != nil {
90+
return fmt.Errorf("cannot create temp file: %w", err)
91+
}
92+
tmpPath := tmpFile.Name()
93+
removeTmp := true
94+
defer func() {
95+
if removeTmp {
96+
os.Remove(tmpPath)
97+
}
98+
}()
99+
100+
if _, err := tmpFile.Write(data); err != nil {
101+
tmpFile.Close()
102+
return fmt.Errorf("cannot write temp file: %w", err)
103+
}
104+
if err := tmpFile.Chmod(mode); err != nil {
105+
tmpFile.Close()
106+
return fmt.Errorf("cannot chmod temp file: %w", err)
107+
}
108+
if err := tmpFile.Close(); err != nil {
109+
return fmt.Errorf("cannot close temp file: %w", err)
110+
}
111+
112+
// Ensure parent directories exist inside the container.
113+
parent := filepath.ToSlash(filepath.Dir(containerPath))
114+
if parent != "/workspace" && parent != "/" {
115+
mkdir := exec.Command("docker", "exec", containerName, "mkdir", "-p", parent)
116+
if out, err := mkdir.CombinedOutput(); err != nil {
117+
return fmt.Errorf("cannot create parent dir %s in container: %w\n%s", parent, err, string(out))
118+
}
119+
}
120+
121+
// Copy the temp file into the container.
122+
cp := exec.Command("docker", "cp", tmpPath, containerName+":"+containerPath)
123+
if out, err := cp.CombinedOutput(); err != nil {
124+
return fmt.Errorf("cannot copy file to container: %w\n%s", err, string(out))
125+
}
126+
127+
removeTmp = false
128+
os.Remove(tmpPath)
129+
return nil
130+
}

cmd/odek/sandbox_file_test.go

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,96 @@
1+
package main
2+
3+
import (
4+
"encoding/json"
5+
"os"
6+
"path/filepath"
7+
"strings"
8+
"testing"
9+
)
10+
11+
func TestHostToContainerPath(t *testing.T) {
12+
dir := t.TempDir()
13+
t.Chdir(dir)
14+
15+
cases := []struct {
16+
name string
17+
host string
18+
want string
19+
wantErr bool
20+
}{
21+
{"relative file", "foo.txt", "/workspace/foo.txt", false},
22+
{"relative nested", "src/main.go", "/workspace/src/main.go", false},
23+
{"absolute under cwd", filepath.Join(dir, "bar.txt"), "/workspace/bar.txt", false},
24+
{"cwd itself", dir, "/workspace", false},
25+
{"outside cwd", "/etc/passwd", "", true},
26+
{"traversal", "../foo.txt", "", true},
27+
{"absolute traversal", filepath.Join(dir, "..", "foo.txt"), "", true},
28+
}
29+
30+
for _, tc := range cases {
31+
t.Run(tc.name, func(t *testing.T) {
32+
got, err := hostToContainerPath(tc.host)
33+
if tc.wantErr {
34+
if err == nil {
35+
t.Fatalf("expected error, got %q", got)
36+
}
37+
return
38+
}
39+
if err != nil {
40+
t.Fatalf("unexpected error: %v", err)
41+
}
42+
if got != tc.want {
43+
t.Errorf("hostToContainerPath(%q) = %q, want %q", tc.host, got, tc.want)
44+
}
45+
})
46+
}
47+
}
48+
49+
func TestHostToContainerPath_RejectsEmpty(t *testing.T) {
50+
_, err := hostToContainerPath("")
51+
if err == nil {
52+
t.Fatal("expected error for empty path")
53+
}
54+
}
55+
56+
func TestSandboxWriteFile_RequiresContainerName(t *testing.T) {
57+
dir := t.TempDir()
58+
t.Chdir(dir)
59+
// Empty container name will cause docker cp to fail quickly; we just check
60+
// that the path translation happens and the command errors.
61+
err := sandboxWriteFile("", "test.txt", []byte("hello"), 0644)
62+
if err == nil {
63+
t.Fatal("expected error for empty container name")
64+
}
65+
if !strings.Contains(err.Error(), "docker") && !strings.Contains(err.Error(), "container") {
66+
t.Errorf("error = %q, want docker/container mention", err)
67+
}
68+
}
69+
70+
// TestSandboxReadonly_RoutesWriteFileThroughContainer verifies that when a
71+
// sandbox container name is set on write_file, the tool attempts to route the
72+
// write through the container instead of touching the host filesystem. A
73+
// non-existent container makes the docker command fail, but the important
74+
// precondition is that no file appears on the host.
75+
func TestSandboxReadonly_RoutesWriteFileThroughContainer(t *testing.T) {
76+
dir := t.TempDir()
77+
t.Chdir(dir)
78+
79+
tool := &writeFileTool{restrictToCWD: true, containerName: "odek-test-nonexistent"}
80+
out, _ := tool.Call(`{"path":"routed.txt","content":"should not appear on host"}`)
81+
82+
if !strings.Contains(out, "via sandbox") {
83+
t.Fatalf("expected write_file to route through sandbox, got: %s", out)
84+
}
85+
86+
target := filepath.Join(dir, "routed.txt")
87+
if _, err := os.Stat(target); !os.IsNotExist(err) {
88+
t.Fatalf("write_file created a host file despite sandbox routing: %s", target)
89+
}
90+
91+
// Sanity-check the JSON envelope is an error, not a success.
92+
var res writeFileResult
93+
if err := json.Unmarshal([]byte(out), &res); err == nil && res.Success {
94+
t.Fatalf("expected failure for non-existent container, got success: %+v", res)
95+
}
96+
}

docs/SECURITY.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,8 @@ Out of scope:
2828
`odek run --sandbox` and `odek serve` (default) spawn an isolated Docker container per session:
2929

3030
- No filesystem access beyond the working directory (mounted read-only when configured).
31+
- `write_file`, `patch`, and `batch_patch` do not touch the host filesystem when `--sandbox` is active; they translate the host path to `/workspace/...` and copy content into the running container with `docker cp`. This makes `--sandbox-readonly` enforceable for the agent's own file tools, not only for commands run through `shell`.
32+
- Extra bind volumes supplied with `--sandbox-volume` are confined to the working directory: the host path must resolve to a location under the working directory, cannot contain `..` or symlink escapes, and cannot match sensitive prefixes such as `/etc`, `/proc`, `/sys`, `/dev`, `/root`, `/home`, `/var`, `/run`, or `/var/run/docker.sock`.
3133
- No network by default. `sandbox_network` defaults to `none`; `host` is rejected.
3234
- Zero kernel capabilities even as root inside the container.
3335
- No `setuid` escalation; `/tmp` is `noexec`.

0 commit comments

Comments
 (0)