From d0c07f015713790786929a7e61e3d3526c1f2f53 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Sun, 21 Jun 2026 02:12:57 +0100 Subject: [PATCH] fix(edit): re-validate path immediately before write (TOCTOU hardening) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit edit_apply validated the target with IsPathAllowed in the handler, then later wrote it via os.WriteFile on the raw path after reading+parsing+diffing. A symlink swapped in during that window could redirect the write outside the workspace root. Inject the admission check into the edit Engine (NewEngineWithValidator) and re-run it inside a new writeValidatedFile helper immediately before the write — IsPathAllowed resolves symlinks at write time, so a swapped-in link now resolving outside the root is rejected. This narrows the TOCTOU window to near-zero while still permitting legitimate in-workspace symlink edits (unlike a blanket O_NOFOLLOW). writeValidatedFile also dedups the perm-preserving write shared by AST and text modes. Tests: rejected-path edit leaves the file untouched; file mode is preserved. --- internal/edit/edit.go | 55 +++++++++++++++++++------------ internal/edit/edit_test.go | 67 ++++++++++++++++++++++++++++++++++++++ internal/server/server.go | 2 +- 3 files changed, 103 insertions(+), 21 deletions(-) diff --git a/internal/edit/edit.go b/internal/edit/edit.go index 84b5364..a9e7ee0 100644 --- a/internal/edit/edit.go +++ b/internal/edit/edit.go @@ -63,17 +63,46 @@ type Engine struct { registry *parser.Registry dmp *diffmatchpatch.DiffMatchPatch fileLocks sync.Map // map[string]*sync.Mutex for per-file locking + // pathAllowed re-checks that a target path is admissible (within the + // workspace root) immediately before a write. Injected to avoid coupling + // edit to config; nil disables the check (used in unit tests). + pathAllowed func(string) bool } -// NewEngine creates a new edit engine. +// NewEngine creates a new edit engine with no write-time path validation. func NewEngine(registry *parser.Registry) *Engine { + return NewEngineWithValidator(registry, nil) +} + +// NewEngineWithValidator creates an edit engine that re-validates each target +// path with pathAllowed immediately before writing it, closing most of the +// time-of-check/time-of-use window between the handler's admission check and the +// actual write (a symlink swapped in afterward is rejected). +func NewEngineWithValidator(registry *parser.Registry, pathAllowed func(string) bool) *Engine { return &Engine{ - registry: registry, - dmp: diffmatchpatch.New(), - fileLocks: sync.Map{}, + registry: registry, + dmp: diffmatchpatch.New(), + fileLocks: sync.Map{}, + pathAllowed: pathAllowed, } } +// writeValidatedFile writes content to path, preserving the file's existing +// permission bits, after re-checking that path is still admissible. The re-check +// resolves symlinks at write time, so a symlink swapped in after the caller's +// original validation cannot redirect the write outside the workspace root. +func (e *Engine) writeValidatedFile(path string, content []byte) error { + if e.pathAllowed != nil && !e.pathAllowed(path) { + return errors.New(errors.ErrPathNotAllowed, "path is outside the workspace root"). + WithContext("path", path) + } + perm := os.FileMode(0o600) // default fallback when the file can't be stat'd + if info, err := os.Stat(path); err == nil { + perm = info.Mode().Perm() + } + return os.WriteFile(path, content, perm) +} + // lockFile acquires a lock for the specified file and returns an unlock function. // This prevents concurrent edits to the same file which could cause corruption. func (e *Engine) lockFile(filePath string) func() { @@ -179,14 +208,7 @@ func (e *Engine) performASTEdit(ctx context.Context, edit *ASTEdit, apply bool) // Apply changes if requested if apply { - // Preserve original file permissions - fileInfo, err := os.Stat(edit.File) - perm := os.FileMode(0o600) // default fallback - if err == nil { - perm = fileInfo.Mode().Perm() - } - - if err := os.WriteFile(edit.File, newContent, perm); err != nil { + if err := e.writeValidatedFile(edit.File, newContent); err != nil { structuredErr := errors.NewFileNotWritableError(edit.File, err) return &EditResult{ Success: false, @@ -236,14 +258,7 @@ func (e *Engine) performTextEdit(_ context.Context, edit *ASTEdit, apply bool) ( // Apply changes if requested if apply { - // Preserve original file permissions - fileInfo, err := os.Stat(edit.File) - perm := os.FileMode(0o600) // default fallback - if err == nil { - perm = fileInfo.Mode().Perm() - } - - if err := os.WriteFile(edit.File, newContent, perm); err != nil { + if err := e.writeValidatedFile(edit.File, newContent); err != nil { structuredErr := errors.NewFileNotWritableError(edit.File, err) return &EditResult{ Success: false, diff --git a/internal/edit/edit_test.go b/internal/edit/edit_test.go index ebff33b..2ac88d0 100644 --- a/internal/edit/edit_test.go +++ b/internal/edit/edit_test.go @@ -369,6 +369,73 @@ func Hello() { } } +// TestApplyRevalidatesPathBeforeWrite verifies the write-time admission re-check: +// an Engine whose validator rejects the target must refuse the write and leave +// the file untouched (the TOCTOU guard against a symlink swapped in after the +// handler's original IsPathAllowed check). +func TestApplyRevalidatesPathBeforeWrite(t *testing.T) { + registry := parser.NewRegistry() + defer registry.Close() + + tmpDir := t.TempDir() + tmpFile := filepath.Join(tmpDir, "test.go") + original := "package main\n\nfunc Hello() {}\n" + if err := os.WriteFile(tmpFile, []byte(original), 0o600); err != nil { + t.Fatal(err) + } + + // Validator denies the path (as IsPathAllowed would for a path that now + // resolves outside the workspace root). + e := NewEngineWithValidator(registry, func(string) bool { return false }) + + result, err := e.Apply(context.Background(), &ASTEdit{ + File: tmpFile, + Operation: EditReplace, + Selector: ASTSelector{Kind: "function_declaration"}, + NewContent: "func Evil() {}", + }) + if err != nil { + t.Fatalf("apply returned transport error: %v", err) + } + if result.Success || result.Applied { + t.Errorf("rejected-path edit must not apply (Success=%v Applied=%v)", result.Success, result.Applied) + } + if got, _ := os.ReadFile(tmpFile); string(got) != original { + t.Errorf("file must be unchanged when path validation fails; got:\n%s", got) + } +} + +// TestApplyPreservesFileMode verifies an edit keeps the file's existing +// permission bits rather than resetting them. +func TestApplyPreservesFileMode(t *testing.T) { + registry := parser.NewRegistry() + defer registry.Close() + e := NewEngine(registry) + + tmpDir := t.TempDir() + tmpFile := filepath.Join(tmpDir, "script.go") + if err := os.WriteFile(tmpFile, []byte("package main\n\nfunc Hello() {}\n"), 0o755); err != nil { + t.Fatal(err) + } + + result, err := e.Apply(context.Background(), &ASTEdit{ + File: tmpFile, + Operation: EditReplace, + Selector: ASTSelector{Kind: "function_declaration"}, + NewContent: "func Hi() {}", + }) + if err != nil || !result.Success { + t.Fatalf("apply failed: err=%v result=%+v", err, result) + } + info, err := os.Stat(tmpFile) + if err != nil { + t.Fatal(err) + } + if perm := info.Mode().Perm(); perm != 0o755 { + t.Errorf("file mode not preserved: got %o, want 0755", perm) + } +} + // An edit on a CRLF file must normalize inserted LF content to CRLF so the file // keeps a single, consistent line-ending convention (a documented guarantee). func TestApplyPreservesCRLF(t *testing.T) { diff --git a/internal/server/server.go b/internal/server/server.go index a62da1d..e0c11b1 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -54,7 +54,7 @@ func New(cfg *config.Config, logger *slog.Logger) (*Server, error) { logger: logger, parser: parserRegistry, matcher: query.NewMatcher(parserRegistry), - editor: edit.NewEngine(parserRegistry), + editor: edit.NewEngineWithValidator(parserRegistry, cfg.IsPathAllowed), readSem: make(chan struct{}, MaxConcurrentReads), querySem: make(chan struct{}, MaxConcurrentQueries), }