v0.8.7: fix #5 — TOCTOU race in session.Append()

Fixes the symlink-swap TOCTOU (time-of-check-time-of-use) race in
session.Append() between Load (read) and Save (write):

- saveLocked(): temp-file + os.Rename for atomic writes — os.Rename
  does NOT follow symlinks, replacing the directory entry itself
- Store.mu sync.Mutex serializes the full read-modify-write cycle in
  Append(), preventing concurrent-write data loss from interleaving

3 new tests: TestAppend_ConcurrentSafety, TestSave_AtomicWriteNoPartialFile,
TestSave_SymlinkNotFollowed. All 22+ existing tests pass.

Author: jkyberneees

internal/session/session.go

@@ -23,6 +23,7 @@ import (
 	"path/filepath"
 	"sort"
 	"strings"
+	"sync"
 	"time"
 
 	"github.com/BackendStack21/kode/internal/llm"
@@ -49,6 +50,7 @@ type Session struct {
 // Operations are simple file reads/writes — no locking, no caching.
 type Store struct {
 	dir string // e.g. /home/user/.kode/sessions/
+	mu  sync.Mutex
 }
 
 // NewStore creates a session store rooted at ~/.kode/sessions/.
@@ -111,6 +113,14 @@ func (s *Store) path(id string) string {
 	return filepath.Join(s.dir, id+".json")
 }
 
+// Path returns the absolute filesystem path for a session file.
+// Exported for testing and debugging.
+func (s *Store) Path(id string) string { return s.path(id) }
+
+// Dir returns the session store directory path.
+// Exported for testing and debugging.
+func (s *Store) Dir() string { return s.dir }
+
 // idFromPath extracts the session ID from a filename like "20260518-abc123.json".
 func idFromPath(name string) string {
 	return strings.TrimSuffix(name, ".json")
@@ -137,28 +147,54 @@ func (s *Store) Create(messages []llm.Message, model, task string) (*Session, er
 }
 
 // Append adds new messages to an existing session, updates timestamps
-// and turn counts, and saves the result.
+// and turn counts, and saves the result atomically.
+// The full read-modify-write is serialized by s.mu to prevent both
+// concurrent-write data loss and symlink-swap TOCTOU attacks.
 func (s *Store) Append(id string, newMsgs []llm.Message) error {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+
 	sess, err := s.Load(id)
 	if err != nil {
 		return err
 	}
 	sess.Messages = append(sess.Messages, newMsgs...)
 	sess.UpdatedAt = time.Now().UTC()
 	sess.Turns = countUserTurns(sess.Messages)
-	return s.Save(sess)
+	return s.saveLocked(sess)
 }
 
-// Save writes a session to disk, overwriting any existing file with
-// the same ID. This is the single write path — all mutations (append,
-// edit, truncate, rename) go through Save().
+// Save writes a session to disk atomically using a temp-file + rename
+// strategy. This prevents:
+//   - Partial writes from crashes (rename is atomic on POSIX)
+//   - Symlink-following TOCTOU attacks (os.Rename replaces the
+//     directory entry itself — it does NOT follow symlinks)
 func (s *Store) Save(sess *Session) error {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	return s.saveLocked(sess)
+}
+
+// saveLocked is the internal write path — caller must hold s.mu.
+// Writes to a temp file in the same directory, then atomically
+// renames over the target. os.Rename replaces the directory entry
+// without following symlinks, so a symlink swapped in between
+// read and write gets replaced with a regular file.
+func (s *Store) saveLocked(sess *Session) error {
 	data, err := json.MarshalIndent(sess, "", "  ")
 	if err != nil {
 		return fmt.Errorf("session: marshal: %w", err)
 	}
-	if err := os.WriteFile(s.path(sess.ID), data, 0600); err != nil {
-		return fmt.Errorf("session: write: %w", err)
+
+	target := s.path(sess.ID)
+	tmp := target + ".tmp"
+	if err := os.WriteFile(tmp, data, 0600); err != nil {
+		os.Remove(tmp)
+		return fmt.Errorf("session: write tmp: %w", err)
+	}
+	if err := os.Rename(tmp, target); err != nil {
+		os.Remove(tmp)
+		return fmt.Errorf("session: rename: %w", err)
 	}
 	return nil
 }

internal/session/session_test.go

@@ -3,6 +3,7 @@ package session
 import (
 	"fmt"
 	"os"
+	"path/filepath"
 	"strings"
 	"testing"
 	"time"
@@ -277,6 +278,143 @@ func TestSession_GetMessages_Empty(t *testing.T) {
 	}
 }
 
+// ── Security: TOCTOU Race (Findings #5) ────────────────────────────────
+//
+// Append() reads a session file, modifies it, then writes it back.
+// An attacker who swaps the file for a symlink between read and write
+// could redirect the write to an arbitrary path. We fix this with:
+//   - Atomic temp-file + os.Rename (Rename does NOT follow symlinks)
+//   - sync.Mutex to serialize concurrent read-modify-write in Append
+//
+// These tests verify the fix and guard against regression.
+
+func TestAppend_ConcurrentSafety(t *testing.T) {
+	// Two concurrent Appends to the same session should both be reflected
+	// in the final file — no lost writes.
+	store := newTestStore(t)
+	sess, err := store.Create(
+		[]llm.Message{{Role: "user", Content: "start"}},
+		"test", "start",
+	)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	done := make(chan error, 2)
+	appendMsg := func(content string) {
+		done <- store.Append(sess.ID, []llm.Message{{Role: "user", Content: content}})
+	}
+
+	go appendMsg("thread-a")
+	go appendMsg("thread-b")
+
+	for i := 0; i < 2; i++ {
+		if err := <-done; err != nil {
+			t.Errorf("concurrent Append error: %v", err)
+		}
+	}
+
+	loaded, err := store.Load(sess.ID)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(loaded.Messages) != 3 {
+		t.Errorf("expected 3 messages (start + 2 appends), got %d", len(loaded.Messages))
+	}
+}
+
+func TestSave_AtomicWriteNoPartialFile(t *testing.T) {
+	// Verify that Save produces a valid JSON file (not a temp file,
+	// not a truncated file) by checking the file path directly.
+	store := newTestStore(t)
+	sess, err := store.Create(
+		[]llm.Message{{Role: "user", Content: "data"}},
+		"test", "data",
+	)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Read the file directly — should be valid JSON
+	path := store.Path(sess.ID)
+	data, err := os.ReadFile(path)
+	if err != nil {
+		t.Fatalf("reading session file: %v", err)
+	}
+	if !strings.Contains(string(data), `"id":`) {
+		t.Error("saved file doesn't look like valid JSON")
+	}
+
+	// No .tmp files should remain in the session directory
+	entries, _ := os.ReadDir(store.Dir())
+	for _, e := range entries {
+		if strings.HasSuffix(e.Name(), ".tmp") {
+			t.Errorf("stale temp file found: %s", e.Name())
+		}
+	}
+}
+
+func TestSave_SymlinkNotFollowed(t *testing.T) {
+	// If the target path is a symlink, Save should replace the symlink
+	// (not follow it) — this is the TOCTOU defense.
+	store := newTestStore(t)
+	sess, err := store.Create(
+		[]llm.Message{{Role: "user", Content: "original"}},
+		"test", "original",
+	)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Create a decoy file
+	decoyPath := filepath.Join(store.Dir(), "decoy.txt")
+	if err := os.WriteFile(decoyPath, []byte("decoy"), 0644); err != nil {
+		t.Fatal(err)
+	}
+
+	// Replace session file with symlink to decoy
+	realPath := store.Path(sess.ID)
+	if err := os.Remove(realPath); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.Symlink("decoy.txt", realPath); err != nil {
+		t.Fatal(err)
+	}
+
+	// Save should NOT follow the symlink — it should replace it
+	sess.Messages = append(sess.Messages, llm.Message{Role: "assistant", Content: "response"})
+	if err := store.Save(sess); err != nil {
+		t.Fatal(err)
+	}
+
+	// After Save, the real path should be a regular file, not a symlink
+	fi, err := os.Lstat(realPath)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if fi.Mode()&os.ModeSymlink != 0 {
+		t.Error("Save left a symlink in place — should have been replaced with a regular file")
+	}
+
+	// Decoy file should still have its original content (intact)
+	decoyData, err := os.ReadFile(decoyPath)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if string(decoyData) != "decoy" {
+		t.Errorf("decoy file was overwritten! content: %q", string(decoyData))
+	}
+
+	// The session file should contain valid session JSON
+	loaded, err := store.Load(sess.ID)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(loaded.Messages) != 2 {
+		t.Errorf("expected 2 messages, got %d", len(loaded.Messages))
+	}
+}
+
 func TestCountUserTurns(t *testing.T) {
 	msgs := []llm.Message{
 		{Role: "system", Content: ""},