a symlink in the sorted directory no longer redirects a step

Placeholders were already stopped from sending a file out of the
directory a rule named. A symlink is a name too, and the directory
krino sorts is by the threat model's own premise a place the internet
writes into: a link named after a rule's destination sent moves and
copies anywhere, and under (on-conflict overwrite) trashed a file
OUTSIDE the sorted directory - while the plan showed the in-tree text
and the run reported success.

A step whose destination passes through a symlink at or below the
directory being sorted now fails. A destination the configuration names
outside it - ~/docs on another disk - is the user's own arrangement and
is followed as before; both cases have a test.

End to end, the review's scenario (Out -> ~/secret, overwrite):

  before: 1 applied, the user's file replaced and trashed
  after:  0 applied 1 failed, the file untouched, nothing trashed

Two bookkeeping bugs in MkdirAllTracked went with it: a dangling
symlink read as a missing directory and was then recorded as one krino
had created - undo would have unlinked a link krino never made - and a
directory created by someone else between the check and the mkdir was
recorded the same way.

6 files changed, 143 insertions, 6 deletions

diff --git a/docs/design.md b/docs/design.md
index 6299288..e2c3cf6 100644
--- a/docs/design.md
+++ b/docs/design.md
@@ -924,7 +924,13 @@ What that means, and the tests that hold it (plans 8 and 9):
   resolve at or under the directory its text names before the first
   placeholder, so a capture of "..", "~" or nothing cannot move it
   elsewhere; a rename whose placeholders produce "", "." or ".." is skipped
-  with a reason.
+  with a reason. A symlink is a name too: a step whose destination passes
+  through a symlink **inside the directory being sorted** fails rather than
+  following it, since anything at all may be written there - by a download,
+  or an unpacked archive - and the plan the user approved shows only the
+  text. A destination the configuration itself names outside that
+  directory, `~/docs` on another disk say, is the user's own arrangement
+  and is followed as before.
 - A file is acted on only while it is still the file that was planned:
   same size and modification time, still a regular file (not a symlink put
   in its place), and, at its planned path, the same inode. A step that had
diff --git a/internal/apply/apply.go b/internal/apply/apply.go
index b36035b..fdeed57 100644
--- a/internal/apply/apply.go
+++ b/internal/apply/apply.go
@@ -86,6 +86,12 @@ func ChainLogged(ctx context.Context, c plan.Chain, done func(i int, sr StepResu
 				stopWhy = "an earlier step in this chain failed"
 				break
 			}
+			if at := UnderSymlink(filepath.Dir(step.Dst), c.Root); at != "" {
+				results[i] = StepResult{Step: step, Status: "failed",
+					Detail: "a symlink inside the sorted directory redirects this step: " + at}
+				stopWhy = "an earlier step in this chain failed"
+				break
+			}
 			res := runStep(step, func(entry string) error {
 				if displaced == nil {
 					return nil
diff --git a/internal/apply/fs.go b/internal/apply/fs.go
index a8feccc..0eee780 100644
--- a/internal/apply/fs.go
+++ b/internal/apply/fs.go
@@ -6,6 +6,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"io/fs"
 	"os"
 	"path/filepath"
 	"strings"
@@ -206,7 +207,17 @@ func splitExt(name string) (stem, ext string) {
 // is not included, and nothing is created or returned on error.
 func MkdirAllTracked(dir string) ([]string, error) {
 	dir = filepath.Clean(dir)
-	if fi, err := os.Stat(dir); err == nil {
+	// Lstat, not Stat: a dangling symlink here reports ENOENT to Stat, so
+	// the "already a directory" branch is missed, os.Mkdir then fails with
+	// EEXIST, and the path used to be recorded as one krino created - which
+	// undo would later remove, unlinking a symlink krino never made.
+	if fi, err := os.Lstat(dir); err == nil {
+		if fi.Mode()&fs.ModeSymlink != 0 {
+			if resolved, serr := os.Stat(dir); serr != nil || !resolved.IsDir() {
+				return nil, fmt.Errorf("%s is a symlink that does not lead to a directory", dir)
+			}
+			return nil, nil
+		}
 		if !fi.IsDir() {
 			return nil, fmt.Errorf("%s exists and is not a directory", dir)
 		}
@@ -224,8 +235,52 @@ func MkdirAllTracked(dir string) ([]string, error) {
 		}
 		made = parentMade
 	}
-	if err := os.Mkdir(dir, 0o755); err != nil && !os.IsExist(err) {
+	if err := os.Mkdir(dir, 0o755); err != nil {
+		if os.IsExist(err) {
+			// Something else created it between the Lstat and here: it is
+			// not ours to remove again.
+			return made, nil
+		}
 		return made, err
 	}
 	return append(made, dir), nil
 }
+
+// UnderSymlink reports the first component of path, at or below root, that
+// is a symlink - "" when there is none, and "" when path is not under root
+// at all.
+//
+// Placeholders are already stopped from redirecting a step out of the
+// directory a rule named (internal/plan's expandDir); a symlink is a name
+// too, and one planted inside the directory krino sorts - by an unpacked
+// archive, say - redirects a move or a copy exactly the same way, while the
+// plan the user approved shows only the text. A destination the
+// configuration itself names, outside the sorted directory, is the user's
+// own arrangement and is not second-guessed.
+func UnderSymlink(path, root string) string {
+	if root == "" {
+		return ""
+	}
+	root = filepath.Clean(root)
+	path = filepath.Clean(path)
+	rel, err := filepath.Rel(root, path)
+	if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) {
+		return ""
+	}
+	at := root
+	for _, part := range strings.Split(filepath.ToSlash(rel), "/") {
+		if part == "." || part == "" {
+			continue
+		}
+		at = filepath.Join(at, part)
+		fi, err := os.Lstat(at)
+		if err != nil {
+			// Not there yet: nothing below it can be a symlink either.
+			return ""
+		}
+		if fi.Mode()&fs.ModeSymlink != 0 {
+			return at
+		}
+	}
+	return ""
+}
diff --git a/internal/apply/swap_test.go b/internal/apply/swap_test.go
index 297db72..dd7cc07 100644
--- a/internal/apply/swap_test.go
+++ b/internal/apply/swap_test.go
@@ -303,3 +303,66 @@ func TestDisplaceThatCannotBeReportedFailsTheStep(t *testing.T) {
 		t.Error("the copy ran even though the displace could not be logged")
 	}
 }
+
+// TestSymlinkInsideTheSortedDirectoryStopsTheStep: placeholders are already
+// stopped from redirecting a step out of the directory a rule named. A
+// symlink is a name too: one planted in the sorted directory - by an
+// unpacked archive, say - named after a rule's destination sends the file
+// anywhere, while the plan the user approved shows only "Out/".
+func TestSymlinkInsideTheSortedDirectoryStopsTheStep(t *testing.T) {
+	root := t.TempDir()
+	outside := t.TempDir()
+	if err := os.Symlink(outside, filepath.Join(root, "Out")); err != nil {
+		t.Skipf("symlinks unavailable: %v", err)
+	}
+	src := filepath.Join(root, "a.pdf")
+	dst := filepath.Join(root, "Out", "a.pdf")
+	c := planned(t, root, "a.pdf", "body", plan.Step{Kind: plan.Move, Src: src, Dst: dst})
+	c.Root = root
+
+	res, err := ChainLogged(context.Background(), c, nil, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if res[0].Status != "failed" || !strings.Contains(res[0].Detail, "symlink") {
+		t.Errorf("step = %+v; want a failure naming the symlink", res[0])
+	}
+	if _, err := os.Lstat(filepath.Join(outside, "a.pdf")); err == nil {
+		t.Error("the file left the sorted directory through the symlink")
+	}
+	if _, err := os.Lstat(src); err != nil {
+		t.Errorf("the file is no longer where it started: %v", err)
+	}
+}
+
+// TestASymlinkedDestinationOutsideTheSortedDirectoryIsFine: a destination
+// the configuration itself names - "~/docs/work", where ~/docs is a symlink
+// to another disk - is the user's own arrangement, not something planted,
+// and must keep working.
+func TestASymlinkedDestinationOutsideTheSortedDirectoryIsFine(t *testing.T) {
+	root := t.TempDir()
+	elsewhere := t.TempDir()
+	real := filepath.Join(elsewhere, "real")
+	if err := os.MkdirAll(real, 0o755); err != nil {
+		t.Fatal(err)
+	}
+	link := filepath.Join(elsewhere, "docs")
+	if err := os.Symlink(real, link); err != nil {
+		t.Skipf("symlinks unavailable: %v", err)
+	}
+	src := filepath.Join(root, "a.pdf")
+	dst := filepath.Join(link, "a.pdf")
+	c := planned(t, root, "a.pdf", "body", plan.Step{Kind: plan.Move, Src: src, Dst: dst})
+	c.Root = root
+
+	res, err := ChainLogged(context.Background(), c, nil, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if res[0].Status != "ok" {
+		t.Fatalf("step = %+v; want it to go through", res[0])
+	}
+	if _, err := os.Lstat(filepath.Join(real, "a.pdf")); err != nil {
+		t.Errorf("the file did not reach the configured destination: %v", err)
+	}
+}
diff --git a/internal/plan/chain.go b/internal/plan/chain.go
index 7c95d9b..f24bbfe 100644
--- a/internal/plan/chain.go
+++ b/internal/plan/chain.go
@@ -92,7 +92,7 @@ func Build(root string, in []Input, now time.Time, d Disk, claims *Claims) []Cha
 // buildOne builds the chain for a single file. claim is shared with every
 // other file processed by the same Build call.
 func buildOne(root string, in Input, now time.Time, d Disk, claim claimed) Chain {
-	c := Chain{File: in.File}
+	c := Chain{File: in.File, Root: root}
 	cur := in.File.Path
 
 	var deletedBy string
diff --git a/internal/plan/step.go b/internal/plan/step.go
index c613ce5..0b554d3 100644
--- a/internal/plan/step.go
+++ b/internal/plan/step.go
@@ -52,8 +52,15 @@ type Step struct {
 
 // Chain is one file's steps, in order.
 type Chain struct {
-	File     scan.File
-	Steps    []Step
+	File  scan.File
+	Steps []Step
+
+	// Root is the directory being sorted. Apply uses it to tell a
+	// destination inside that directory - where anything at all may have
+	// been written, by a download or an unpacked archive - from one the
+	// configuration named itself, which is the user's own arrangement.
+	Root string
+
 	Warnings []string
 }