Skip symlinks during traversal (#68)

* Skip symbolic links during traversal

`find-replace` uses `File.Info()` (a thin wrapper around `os.Stat`) to
decide whether a directory entry is a directory. `os.Stat` follows
symbolic links, so any symlink inside the working tree was transparently
resolved and walked — letting `find-replace` rewrite file contents and
rename files inside the symlink's target, anywhere on the filesystem.

This violates the documented contract ("Searches are performed
recursively from the current working directory") and is a real
privilege-escalation primitive when the tool is run with elevated
permissions (CI, root). See issue #2 for the reproducer.

Fix: in `WalkDir`, check the `fs.ModeSymlink` bit on the
`fs.DirEntry` returned by `os.ReadDir` and skip the entry before it
reaches `HandleFile`. The DirEntry already carries the symlink-bit from
the underlying `getdents64`/`readdir` call, so no extra `lstat` is
needed. The skip is logged as "Skipping symlink: <path>" so the
behavior is visible to operators.

No CLI surface change: there is intentionally no `--follow-symlinks`
opt-in flag in this commit. That would be additive and is filed as a
follow-up. Issue #13 (drop the per-entry `os.Stat` in `HandleFile`) is
related but requires a separate refactor of `HandleFile`'s dispatch and
is left for a follow-up.

Tests cover: outside-the-root file targets, outside-the-root directory
targets, inside-the-root targets (the symlink is still skipped, the
real path is still rewritten exactly once), broken symlinks (silently
skipped, no error recorded), and sanity checks for regular files and
directories.

Fixes #2

* Probe symlink capability instead of skipping by GOOS

Review feedback on #68: rather than blanket-skipping the symlink tests
when runtime.GOOS == "windows", explicitly probe for the ability to
create a symlink and skip on actual failure. That way the tests run
wherever the capability is available (e.g. a Windows runner with
SeCreateSymbolicLinkPrivilege, a sandboxed container that happens to
allow symlink creation) and are correctly skipped on a host that lacks
it (including a non-Windows host with a restrictive seccomp/AppArmor
profile or filesystem).

The probe is a t.Helper named requireSymlinks that creates a real
symlink in a fresh t.TempDir; if Symlink returns an error, the test
t.Skips with that error embedded so it's clear why.

The Windows guards in TestWalkDir_PermissionDeniedSubdirContinues and
TestWriteCleansUpTempFileOnRenameFailure are intentionally kept: those
skips are about non-symlink semantic differences (permission bits and
rename-onto-directory), not symlink capability.

---------

Co-authored-by: Claude <noreply@anthropic.com>

Author: dolph

find_replace.go

@@ -4,6 +4,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"io/fs"
 	"log"
 	"os"
 	"path/filepath"
@@ -125,6 +126,16 @@ func (fr *findReplace) WalkDir(f *File) {
 
 	for _, file := range files {
 		childPath := filepath.Join(f.Path, file.Name())
+		// Skip symbolic links by default. os.Stat (used downstream by
+		// File.Info) silently follows symlinks, which would let a link
+		// inside the working tree escape the search root and rewrite
+		// arbitrary files elsewhere on the filesystem (see issue #2).
+		// fs.DirEntry.Type() reports the symlink bit directly from the
+		// readdir call, so we don't need an extra lstat here.
+		if file.Type()&fs.ModeSymlink != 0 {
+			log.Printf("Skipping symlink: %v", childPath)
+			continue
+		}
 		childFile, err := NewFile(childPath)
 		if err != nil {
 			log.Print(err)

find_replace_test.go

@@ -74,6 +74,25 @@ func readOrFatal(tb testing.TB, f *File) string {
 	return s
 }
 
+// requireSymlinks skips the calling test if the test process cannot create
+// symbolic links. On Windows this typically requires
+// SeCreateSymbolicLinkPrivilege; on sandboxed Linux filesystems and certain
+// container configurations the syscall can also fail with EPERM/ENOSYS.
+// Probing for the actual capability avoids skipping on a host where symlinks
+// happen to work, and avoids running on a host where they happen not to.
+func requireSymlinks(t *testing.T) {
+	t.Helper()
+	probe := t.TempDir()
+	target := filepath.Join(probe, "target")
+	if err := os.WriteFile(target, nil, 0600); err != nil {
+		t.Fatalf("requireSymlinks setup: WriteFile(%q): %v", target, err)
+	}
+	link := filepath.Join(probe, "link")
+	if err := os.Symlink(target, link); err != nil {
+		t.Skipf("symlinks not supported in this environment: %v", err)
+	}
+}
+
 func expectedPathAfterRename(f *File, fr *findReplace) string {
 	return filepath.Join(f.Dir(), strings.ReplaceAll(f.Base(), fr.find, fr.replace))
 }
@@ -684,3 +703,260 @@ func BenchmarkNova(b *testing.B) {
 		fr.WalkDir(d)
 	}
 }
+
+// TestWalkDir_SymlinkToFileOutsideCWDNotFollowed ensures the walker does not
+// rewrite a file reached via a symlink whose target lives outside the search
+// root. Following the symlink would violate find-replace's documented contract
+// ("Searches are performed recursively from the current working directory")
+// and is a known privilege-escalation primitive (see issue #2).
+func TestWalkDir_SymlinkToFileOutsideCWDNotFollowed(t *testing.T) {
+	requireSymlinks(t)
+
+	// outside holds the file that must NOT be touched by the walk.
+	outside := t.TempDir()
+	outsideFile := filepath.Join(outside, "outside.txt")
+	const outsideContent = "alpha-outside"
+	if err := os.WriteFile(outsideFile, []byte(outsideContent), 0600); err != nil {
+		t.Fatalf("WriteFile(%q): %v", outsideFile, err)
+	}
+
+	// root is the search root. It contains a real file and a symlink that
+	// escapes to outsideFile.
+	root := t.TempDir()
+	insideFile := filepath.Join(root, "inside.txt")
+	if err := os.WriteFile(insideFile, []byte("alpha-inside"), 0600); err != nil {
+		t.Fatalf("WriteFile(%q): %v", insideFile, err)
+	}
+	escape := filepath.Join(root, "escape.txt")
+	if err := os.Symlink(outsideFile, escape); err != nil {
+		t.Fatalf("Symlink(%q, %q): %v", outsideFile, escape, err)
+	}
+
+	rootFile := newFileOrFatal(t, root)
+	fr := findReplace{find: "alpha", replace: "beta"}
+	fr.WalkDir(rootFile)
+
+	if err := fr.errs.err(); err != nil {
+		t.Errorf("WalkDir reported unexpected error(s): %v", err)
+	}
+
+	// The real file inside the root must have been rewritten.
+	got, err := os.ReadFile(insideFile)
+	if err != nil {
+		t.Fatalf("ReadFile(%q): %v", insideFile, err)
+	}
+	if string(got) != "beta-inside" {
+		t.Errorf("inside.txt = %q; want %q", string(got), "beta-inside")
+	}
+
+	// The symlink target outside the root must NOT have been rewritten.
+	got, err = os.ReadFile(outsideFile)
+	if err != nil {
+		t.Fatalf("ReadFile(%q): %v", outsideFile, err)
+	}
+	if string(got) != outsideContent {
+		t.Errorf("outside.txt = %q; want %q (symlink was followed)", string(got), outsideContent)
+	}
+
+	// The symlink itself should still be present as a symlink (the walker
+	// should have skipped it, not replaced it with a regular file or renamed
+	// it).
+	info, err := os.Lstat(escape)
+	if err != nil {
+		t.Fatalf("Lstat(%q) after walk: %v (symlink was removed/renamed)", escape, err)
+	}
+	if info.Mode()&fs.ModeSymlink == 0 {
+		t.Errorf("Lstat(%q).Mode() = %v; want a symlink (was replaced with a regular file)", escape, info.Mode())
+	}
+}
+
+// TestWalkDir_SymlinkToDirOutsideCWDNotTraversed ensures the walker does not
+// recurse through a symlink whose target is a directory outside the search
+// root. This is the directory variant of the issue #2 reproducer.
+func TestWalkDir_SymlinkToDirOutsideCWDNotTraversed(t *testing.T) {
+	requireSymlinks(t)
+
+	// outside is a directory tree we must not touch.
+	outside := t.TempDir()
+	outsideFile := filepath.Join(outside, "victim.txt")
+	const outsideContent = "alpha-outside"
+	if err := os.WriteFile(outsideFile, []byte(outsideContent), 0600); err != nil {
+		t.Fatalf("WriteFile(%q): %v", outsideFile, err)
+	}
+
+	root := t.TempDir()
+	escape := filepath.Join(root, "escape")
+	if err := os.Symlink(outside, escape); err != nil {
+		t.Fatalf("Symlink(%q, %q): %v", outside, escape, err)
+	}
+
+	rootFile := newFileOrFatal(t, root)
+	fr := findReplace{find: "alpha", replace: "beta"}
+	fr.WalkDir(rootFile)
+
+	if err := fr.errs.err(); err != nil {
+		t.Errorf("WalkDir reported unexpected error(s): %v", err)
+	}
+
+	// The outside file must NOT have been rewritten.
+	got, err := os.ReadFile(outsideFile)
+	if err != nil {
+		t.Fatalf("ReadFile(%q): %v", outsideFile, err)
+	}
+	if string(got) != outsideContent {
+		t.Errorf("victim.txt = %q; want %q (symlinked directory was traversed)", string(got), outsideContent)
+	}
+
+	// The outside file must NOT have been renamed.
+	renamed := filepath.Join(outside, "beta-victim.txt")
+	if _, err := os.Lstat(renamed); err == nil {
+		t.Errorf("Lstat(%q) succeeded; file inside symlinked dir was renamed", renamed)
+	}
+
+	// The symlink itself should still be present.
+	if _, err := os.Lstat(escape); err != nil {
+		t.Errorf("Lstat(%q) after walk: %v (symlink was removed/renamed)", escape, err)
+	}
+}
+
+// TestWalkDir_SymlinkToFileInsideCWDNotFollowed verifies that even when a
+// symlink's target is inside the search root, the symlink itself is still
+// skipped. The target is still picked up via the normal walker pass over its
+// real path, so it is rewritten exactly once — never via the symlink.
+func TestWalkDir_SymlinkToFileInsideCWDNotFollowed(t *testing.T) {
+	requireSymlinks(t)
+
+	root := t.TempDir()
+	real := filepath.Join(root, "real.txt")
+	if err := os.WriteFile(real, []byte("alpha"), 0600); err != nil {
+		t.Fatalf("WriteFile(%q): %v", real, err)
+	}
+	link := filepath.Join(root, "link.txt")
+	if err := os.Symlink(real, link); err != nil {
+		t.Fatalf("Symlink(%q, %q): %v", real, link, err)
+	}
+
+	rootFile := newFileOrFatal(t, root)
+	fr := findReplace{find: "alpha", replace: "beta"}
+	fr.WalkDir(rootFile)
+
+	if err := fr.errs.err(); err != nil {
+		t.Errorf("WalkDir reported unexpected error(s): %v", err)
+	}
+
+	// The real file should be rewritten via the normal pass — exactly once
+	// (not twice — that is, the symlink was not also followed and rewritten).
+	got, err := os.ReadFile(real)
+	if err != nil {
+		t.Fatalf("ReadFile(%q): %v", real, err)
+	}
+	if string(got) != "beta" {
+		t.Errorf("real.txt = %q; want %q", string(got), "beta")
+	}
+
+	// The symlink should still be a symlink pointing to the (now-rewritten)
+	// real file. The walker must not have renamed or removed it.
+	info, err := os.Lstat(link)
+	if err != nil {
+		t.Fatalf("Lstat(%q) after walk: %v (symlink was removed/renamed)", link, err)
+	}
+	if info.Mode()&fs.ModeSymlink == 0 {
+		t.Errorf("Lstat(%q).Mode() = %v; want a symlink (was replaced with a regular file)", link, info.Mode())
+	}
+}
+
+// TestWalkDir_BrokenSymlinkSkippedWithoutError ensures a dangling symlink is
+// silently skipped, not reported as an error and not "rewritten" or "renamed".
+func TestWalkDir_BrokenSymlinkSkippedWithoutError(t *testing.T) {
+	requireSymlinks(t)
+
+	root := t.TempDir()
+	broken := filepath.Join(root, "alpha-broken")
+	target := filepath.Join(root, "does-not-exist")
+	if err := os.Symlink(target, broken); err != nil {
+		t.Fatalf("Symlink(%q, %q): %v", target, broken, err)
+	}
+
+	rootFile := newFileOrFatal(t, root)
+	fr := findReplace{find: "alpha", replace: "beta"}
+	fr.WalkDir(rootFile)
+
+	if err := fr.errs.err(); err != nil {
+		t.Errorf("WalkDir reported unexpected error(s) for a broken symlink: %v", err)
+	}
+
+	// The symlink must still be present under its original name (it was not
+	// renamed alpha-broken -> beta-broken, nor was it followed).
+	info, err := os.Lstat(broken)
+	if err != nil {
+		t.Fatalf("Lstat(%q) after walk: %v (broken symlink was removed/renamed)", broken, err)
+	}
+	if info.Mode()&fs.ModeSymlink == 0 {
+		t.Errorf("Lstat(%q).Mode() = %v; want a symlink (was replaced with a regular file)", broken, info.Mode())
+	}
+
+	// The renamed name should not have been created either.
+	renamed := filepath.Join(root, "beta-broken")
+	if _, err := os.Lstat(renamed); err == nil {
+		t.Errorf("Lstat(%q) succeeded; broken symlink was renamed", renamed)
+	}
+}
+
+// TestWalkDir_RegularFileStillProcessed is a sanity check that the
+// symlink-skipping fix does not break the common case for regular files.
+func TestWalkDir_RegularFileStillProcessed(t *testing.T) {
+	root := t.TempDir()
+	src := filepath.Join(root, "alpha.txt")
+	if err := os.WriteFile(src, []byte("alpha"), 0600); err != nil {
+		t.Fatalf("WriteFile(%q): %v", src, err)
+	}
+
+	rootFile := newFileOrFatal(t, root)
+	fr := findReplace{find: "alpha", replace: "beta"}
+	fr.WalkDir(rootFile)
+
+	if err := fr.errs.err(); err != nil {
+		t.Errorf("WalkDir reported unexpected error(s): %v", err)
+	}
+
+	renamed := filepath.Join(root, "beta.txt")
+	got, err := os.ReadFile(renamed)
+	if err != nil {
+		t.Fatalf("ReadFile(%q): %v", renamed, err)
+	}
+	if string(got) != "beta" {
+		t.Errorf("beta.txt = %q; want %q", string(got), "beta")
+	}
+}
+
+// TestWalkDir_RegularDirStillTraversed is the directory variant of the sanity
+// check.
+func TestWalkDir_RegularDirStillTraversed(t *testing.T) {
+	root := t.TempDir()
+	sub := filepath.Join(root, "alpha-dir")
+	if err := os.Mkdir(sub, 0700); err != nil {
+		t.Fatalf("Mkdir(%q): %v", sub, err)
+	}
+	child := filepath.Join(sub, "alpha.txt")
+	if err := os.WriteFile(child, []byte("alpha"), 0600); err != nil {
+		t.Fatalf("WriteFile(%q): %v", child, err)
+	}
+
+	rootFile := newFileOrFatal(t, root)
+	fr := findReplace{find: "alpha", replace: "beta"}
+	fr.WalkDir(rootFile)
+
+	if err := fr.errs.err(); err != nil {
+		t.Errorf("WalkDir reported unexpected error(s): %v", err)
+	}
+
+	renamedDir := filepath.Join(root, "beta-dir")
+	renamedChild := filepath.Join(renamedDir, "beta.txt")
+	got, err := os.ReadFile(renamedChild)
+	if err != nil {
+		t.Fatalf("ReadFile(%q): %v", renamedChild, err)
+	}
+	if string(got) != "beta" {
+		t.Errorf("beta-dir/beta.txt = %q; want %q", string(got), "beta")
+	}
+}