fix: harden schedule atomic writes against symlink attacks

writeJSONAtomic previously used a predictable temp path (path + ".tmp")
and os.WriteFile, which follows symlinks. Replace it with
internal/fsatomic.WriteFile, which uses a random temp name with O_EXCL,
fsyncs data and directory, and renames over the target so a swapped-in
symlink is replaced rather than followed.

- Update internal/schedule/store.go to delegate to fsatomic.WriteFile.
- Add TestAtomicWrite_SymlinkTargetReplaced regression test.
- Update coverage tests that relied on the old predictable temp path to use
  read-only directories instead.
- Update docs/SECURITY.md and AGENTS.md.

Author: jkyberneees

AGENTS.md

@@ -134,6 +134,7 @@ Layered prompt-injection / approval-fatigue defenses. Full reference: [docs/SECU
 - **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.
 - **Project config sensitive-field rejection** (`internal/config/loader.go`) — `./odek.json` is untrusted, so `base_url`, `api_key`, `system`, and the `dangerous` section set there are ignored (with stderr warnings). These can only be configured from operator-controlled sources: `~/.odek/config.json`, `ODEK_*` env vars, or CLI flags.
 - **MCP subprocess environment sanitisation** (`internal/mcpclient/client.go`) — MCP server children receive only a minimal allowlist of safe environment variables plus explicit `env` overrides. Keys matching secret patterns (`*_API_KEY`, `*_TOKEN`, `*_SECRET`, `*_PASSWORD`, etc.) are stripped, preventing a compromised or malicious MCP server from reading parent secrets.
+- **Schedule atomic-write hardening** (`internal/schedule/store.go` + `internal/fsatomic`) — schedule file writes now use `fsatomic.WriteFile`, which creates a random temp file with `O_EXCL`, fsyncs data and directory, and renames over the target. A swapped-in symlink is replaced rather than followed, closing the symlink-override attack on `schedules.json` / `schedule-state.json`.
 - **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.
 - **Secret redaction** (`internal/redact/redact.go`) — 20+ patterns: OpenAI, Anthropic, GitHub PAT, AWS, PEM, JWT, Vault, Google OAuth, SendGrid, Discord, DB URLs, etc.
 

docs/SECURITY.md

@@ -312,6 +312,10 @@ These fields can only be set from operator-controlled sources: `~/.odek/config.j
 
 MCP server subprocesses no longer inherit the full odek process environment. They receive only a minimal allowlist of safe variables (e.g. `PATH`, `HOME`, `LANG`, `TMPDIR`) plus any explicit `env` overrides from the server config. Keys matching secret patterns — `*_API_KEY`, `*_TOKEN`, `*_SECRET`, `*_PASSWORD`, `*_CREDENTIAL`, `*_PRIVATE_KEY`, etc. — are stripped even when listed in `env`. This prevents a compromised or malicious MCP server from reading secrets loaded from `~/.odek/secrets.env` or other provider keys that were present in the parent environment.
 
+### 20. Schedule file atomic-write hardening
+
+Schedule persistence (`schedules.json` and `schedule-state.json`) now writes through `internal/fsatomic.WriteFile`. It creates a uniquely-named temp file with `O_EXCL` (so a pre-created symlink cannot be opened), fsyncs the data and parent directory, and atomically renames over the target. This means a swapped-in symlink is replaced rather than followed, closing the symlink-override attack where an attacker points `schedules.json.tmp` or `schedule-state.json.tmp` at sensitive files.
+
 ### YOLO mode
 
 ```json

internal/schedule/coverage_test.go

@@ -88,19 +88,21 @@ func TestLoadState_Error(t *testing.T) {
 
 // ── saveDoc / writeJSONAtomic error paths ─────────────────────────────────
 
-// makeTmpDir creates a directory at "<file>.tmp" so writeJSONAtomic's WriteFile
-// to that temp path fails (it can't write a file over a directory).
-func makeTmpDir(t *testing.T, path string) {
+// makeReadOnly makes dir read-only so fsatomic.WriteFile's temp-file creation
+// fails, exercising the save error path.
+func makeReadOnly(t *testing.T, dir string) func() {
 	t.Helper()
-	if err := os.MkdirAll(path+".tmp", 0755); err != nil {
-		t.Fatalf("mkdir %s.tmp: %v", path, err)
+	if err := os.Chmod(dir, 0500); err != nil {
+		t.Fatalf("chmod %s: %v", dir, err)
 	}
+	return func() { os.Chmod(dir, 0755) }
 }
 
 func TestAdd_SaveDocError(t *testing.T) {
 	dir := t.TempDir()
 	st, _ := NewStoreAt(dir)
-	makeTmpDir(t, filepath.Join(dir, schedulesFile))
+	cleanup := makeReadOnly(t, dir)
+	defer cleanup()
 	if _, err := st.Add(sampleJob()); err == nil {
 		t.Error("Add should fail when the definitions file can't be written")
 	}
@@ -113,7 +115,8 @@ func TestRemove_SaveDocError(t *testing.T) {
 	if err != nil {
 		t.Fatalf("Add: %v", err)
 	}
-	makeTmpDir(t, filepath.Join(dir, schedulesFile))
+	cleanup := makeReadOnly(t, dir)
+	defer cleanup()
 	if err := st.Remove(a.ID); err == nil {
 		t.Error("Remove should fail when the definitions file can't be rewritten")
 	}
@@ -170,11 +173,18 @@ func TestWriteJSONAtomic_Errors(t *testing.T) {
 		t.Error("writeJSONAtomic should fail to marshal a channel")
 	}
 
-	// Write error: the temp path is a directory.
-	wpath := filepath.Join(dir, "w.json")
-	makeTmpDir(t, wpath)
+	// Write error: the directory is read-only, so temp-file creation fails.
+	wdir := filepath.Join(dir, "wro")
+	if err := os.Mkdir(wdir, 0755); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.Chmod(wdir, 0500); err != nil {
+		t.Fatal(err)
+	}
+	defer os.Chmod(wdir, 0755)
+	wpath := filepath.Join(wdir, "w.json")
 	if err := writeJSONAtomic(wpath, map[string]int{"a": 1}); err == nil {
-		t.Error("writeJSONAtomic should fail when the temp path is a directory")
+		t.Error("writeJSONAtomic should fail when the directory is not writable")
 	}
 
 	// Rename error: the destination is a non-empty directory, so rename of the
@@ -335,7 +345,8 @@ func TestReconcile_SkipSaveStateError(t *testing.T) {
 		t.Fatal(err)
 	}
 	// Break state writes so the skip-record persistence fails (logged, not fatal).
-	makeTmpDir(t, filepath.Join(dir, stateFile))
+	cleanup := makeReadOnly(t, dir)
+	defer cleanup()
 	s := New(st, &fakeRunner{}, &fakeDeliverer{}, Options{})
 	now := time.Date(2026, 6, 4, 10, 0, 0, 0, time.UTC)
 	s.reconcile(now) // exercises the SaveState-error log branch
@@ -353,7 +364,8 @@ func TestExecute_SaveStateError(t *testing.T) {
 	t0 := time.Date(2026, 6, 4, 10, 0, 0, 0, time.UTC)
 	s.reconcile(t0)
 	// Break state writes; the run still completes, the SaveState error is logged.
-	makeTmpDir(t, filepath.Join(dir, stateFile))
+	cleanup := makeReadOnly(t, dir)
+	defer cleanup()
 	s.fireDue(context.Background(), s.peekNext(job.ID))
 	s.Wait()
 }

internal/schedule/store.go

@@ -11,6 +11,8 @@ import (
 	"sync"
 	"syscall"
 	"time"
+
+	"github.com/BackendStack21/odek/internal/fsatomic"
 )
 
 // File names under ~/.odek.
@@ -350,20 +352,18 @@ func readJSON(path string, v any) error {
 // so a reader never observes a half-written file and a swapped-in symlink is
 // replaced rather than followed. Files are 0600 since tasks may reference
 // secrets.
+//
+// The actual atomic write is delegated to internal/fsatomic, which uses a
+// random temp name with O_EXCL (so a pre-created symlink cannot be opened)
+// and fsyncs both the data and the parent directory before returning.
 func writeJSONAtomic(path string, v any) error {
 	data, err := json.MarshalIndent(v, "", "  ")
 	if err != nil {
 		return fmt.Errorf("schedule: marshal %s: %w", filepath.Base(path), err)
 	}
-	tmp := path + ".tmp"
-	if err := os.WriteFile(tmp, data, 0600); err != nil {
-		os.Remove(tmp)
+	if err := fsatomic.WriteFile(path, data, 0600); err != nil {
 		return fmt.Errorf("schedule: write %s: %w", filepath.Base(path), err)
 	}
-	if err := os.Rename(tmp, path); err != nil {
-		os.Remove(tmp)
-		return fmt.Errorf("schedule: rename %s: %w", filepath.Base(path), err)
-	}
 	return nil
 }
 

internal/schedule/store_test.go

@@ -271,6 +271,42 @@ func TestAtomicWrite_NoTempLeftover(t *testing.T) {
 	}
 }
 
+func TestAtomicWrite_SymlinkTargetReplaced(t *testing.T) {
+	dir := t.TempDir()
+
+	// Simulate an attacker swapping schedules.json with a symlink to a sensitive
+	// file. The write must replace the symlink (the directory entry), not follow
+	// it and overwrite the sensitive target.
+	decoy := filepath.Join(dir, "decoy-sensitive.txt")
+	if err := os.WriteFile(decoy, []byte("original-secret"), 0600); err != nil {
+		t.Fatal(err)
+	}
+	target := filepath.Join(dir, schedulesFile)
+	if err := os.Symlink(decoy, target); err != nil {
+		t.Fatal(err)
+	}
+
+	if err := writeJSONAtomic(target, scheduleDoc{Version: 1, Jobs: []Job{sampleJob()}}); err != nil {
+		t.Fatalf("writeJSONAtomic: %v", err)
+	}
+
+	got, err := os.ReadFile(decoy)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if string(got) != "original-secret" {
+		t.Errorf("symlink target was overwritten: %q", string(got))
+	}
+
+	info, err := os.Lstat(target)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if info.Mode()&os.ModeSymlink != 0 {
+		t.Error("target is still a symlink; symlink was followed instead of replaced")
+	}
+}
+
 // ── Concurrency (run with -race) ────────────────────────────────────────
 
 func TestConcurrentStateWrites(t *testing.T) {