fix(edit): re-validate path immediately before write (TOCTOU hardening)

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.
This commit is contained in:
2026-06-21 02:12:57 +01:00
parent a841210092
commit d0c07f0157
3 changed files with 103 additions and 21 deletions
+35 -20
View File
@@ -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,
+67
View File
@@ -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) {
+1 -1
View File
@@ -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),
}