plan 10: stderr messages cannot be split by quoted newlines; config paths escaped; krino new refuses control characters; independent terminal oracle

9 files changed, 115 insertions, 32 deletions

diff --git a/cmd/krino/check.go b/cmd/krino/check.go
index 97c1f16..d4aee01 100644
--- a/cmd/krino/check.go
+++ b/cmd/krino/check.go
@@ -29,25 +29,25 @@ func cmdCheck(g *globals, args []string, stdout, stderr io.Writer) int {
 	}
 	r := e.Check()
 
-	fmt.Fprintf(stdout, "config: %s\n", xdg.Abbrev(r.MainFile))
-	fmt.Fprintf(stdout, "log: %s\n", xdg.Abbrev(r.LogFile))
-	fmt.Fprintf(stdout, "cache: %s\n", xdg.Abbrev(cacheDir()))
+	fmt.Fprintf(stdout, "config: %s\n", display(xdg.Abbrev(r.MainFile)))
+	fmt.Fprintf(stdout, "log: %s\n", display(xdg.Abbrev(r.LogFile)))
+	fmt.Fprintf(stdout, "cache: %s\n", display(xdg.Abbrev(cacheDir())))
 	if len(r.Dirs) == 0 {
 		fmt.Fprintln(stdout, "no directories included; add one with: krino new NAME PATH")
 	}
 	for _, dr := range r.Dirs {
-		fmt.Fprintf(stdout, "\n%s  %s\n", dr.Dir.Name, xdg.Abbrev(dr.Dir.Root))
+		fmt.Fprintf(stdout, "\n%s  %s\n", display(dr.Dir.Name), display(xdg.Abbrev(dr.Dir.Root)))
 		if dr.Missing {
-			fmt.Fprintf(stdout, "  warning: %s is not a directory right now; it will be skipped\n", xdg.Abbrev(dr.Dir.Root))
+			fmt.Fprintf(stdout, "  warning: %s is not a directory right now; it will be skipped\n", display(xdg.Abbrev(dr.Dir.Root)))
 		}
 		for _, x := range dr.Dir.Excludes {
-			fmt.Fprintf(stdout, "  %s\n", x.Text)
+			fmt.Fprintf(stdout, "  %s\n", display(x.Text))
 		}
 		if len(dr.Dir.Rules) == 0 {
 			fmt.Fprintln(stdout, "  no rules yet")
 		}
 		for i, rule := range dr.Dir.Rules {
-			fmt.Fprintf(stdout, "  %2d  %-16s %s\n", i+1, rule.Name, describeActions(rule.Conf))
+			fmt.Fprintf(stdout, "  %2d  %-16s %s\n", i+1, display(rule.Name), display(describeActions(rule.Conf)))
 		}
 	}
 
diff --git a/cmd/krino/common.go b/cmd/krino/common.go
index a9c92f3..15c32f6 100644
--- a/cmd/krino/common.go
+++ b/cmd/krino/common.go
@@ -55,7 +55,8 @@ func mainFile(g *globals) string {
 
 // usageError reports a command-line mistake and returns exit status 2.
 func usageError(stderr io.Writer, msg string) int {
-	fmt.Fprintf(stderr, "krino: %s\nrun 'krino -h' for help\n", msg)
+	fmt.Fprintf(stderr, "krino: %s\n", msg)
+	fmt.Fprintln(stderr, "run 'krino -h' for help")
 	return 2
 }
 
diff --git a/cmd/krino/display.go b/cmd/krino/display.go
index 193d8b8..d468c6b 100644
--- a/cmd/krino/display.go
+++ b/cmd/krino/display.go
@@ -58,18 +58,22 @@ func controlRune(r rune) bool {
 	return (r >= 0x80 && r <= 0x9f) || unicode.Is(unicode.Bidi_Control, r) || unicode.In(r, unicode.Zl, unicode.Zp)
 }
 
-// safeWriter writes through display line by line, keeping the newlines:
-// every error and warning krino writes to stderr may quote a file name or a
-// tool's message, and nothing krino itself writes there is styled (review
-// M5).
+// safeWriter writes through display: every error and warning krino writes
+// to stderr may quote a file name or a tool's message, and nothing krino
+// itself writes there is styled (review M5). Each Write is one message line:
+// only its final newline is kept, and a newline inside it - from quoted text
+// - is escaped, so it cannot start a line that reads as krino's own
+// (re-review term F1). A message of several lines is written with one Write
+// per line.
 type safeWriter struct{ w io.Writer }
 
 func (s safeWriter) Write(p []byte) (int, error) {
-	lines := strings.Split(string(p), "\n")
-	for i, l := range lines {
-		lines[i] = display(l)
+	text := string(p)
+	end := ""
+	if strings.HasSuffix(text, "\n") {
+		text, end = text[:len(text)-1], "\n"
 	}
-	if _, err := io.WriteString(s.w, strings.Join(lines, "\n")); err != nil {
+	if _, err := io.WriteString(s.w, display(text)+end); err != nil {
 		return 0, err
 	}
 	return len(p), nil
diff --git a/cmd/krino/display_test.go b/cmd/krino/display_test.go
index 0f4cf90..a1b199c 100644
--- a/cmd/krino/display_test.go
+++ b/cmd/krino/display_test.go
@@ -6,15 +6,24 @@ import (
 	"fmt"
 	"strings"
 	"testing"
-	"unicode"
 	"unicode/utf8"
 )
 
-// firstUnsafe names the first thing in s a terminal could act on - a
-// control character (a newline too, unless allowNewline), a Unicode
-// bidirectional control, a line or paragraph separator, or invalid UTF-8 -
-// or returns "" when there is none. It is built from Go's Unicode tables,
-// not from display's own ranges, so it can catch a class display misses.
+// terminalRunes is the test's own, hand-written list of code points a
+// terminal acts on or that reorder what it shows: C0 controls, DEL, C1
+// controls, the Unicode bidirectional marks, embeddings, overrides and
+// isolates (UAX #9), and the line and paragraph separators. It is written
+// out, not derived from Go's tables or from display, so it can catch a
+// class display misses (re-review term F3).
+var terminalRunes = [][2]rune{
+	{0x00, 0x1f}, {0x7f, 0x9f},
+	{0x061c, 0x061c}, {0x200e, 0x200f}, {0x202a, 0x202e}, {0x2066, 0x2069},
+	{0x2028, 0x2029},
+}
+
+// firstUnsafe names the first thing in s from terminalRunes (a newline
+// allowed when allowNewline), or invalid UTF-8, or returns "" when there is
+// none.
 func firstUnsafe(s string, allowNewline bool) string {
 	if !utf8.ValidString(s) {
 		return "invalid UTF-8"
@@ -23,8 +32,10 @@ func firstUnsafe(s string, allowNewline bool) string {
 		if r == '\n' && allowNewline {
 			continue
 		}
-		if unicode.IsControl(r) || unicode.Is(unicode.Bidi_Control, r) || unicode.In(r, unicode.Zl, unicode.Zp) {
-			return fmt.Sprintf("%U", r)
+		for _, span := range terminalRunes {
+			if r >= span[0] && r <= span[1] {
+				return fmt.Sprintf("%U", r)
+			}
 		}
 	}
 	return ""
@@ -92,8 +103,8 @@ func TestReviewEscapesHostileNames(t *testing.T) {
 }
 
 // TestSafeWriterEscapesButKeepsNewlines: everything written to stderr goes
-// through display line by line, so a quoted file name cannot act on the
-// terminal while messages keep their lines (review M5).
+// through display, so a quoted file name cannot act on the terminal; each
+// Write keeps its final newline (review M5).
 func TestSafeWriterEscapesButKeepsNewlines(t *testing.T) {
 	var b strings.Builder
 	in := "a\x1b[2J\nb\u202e\n"
@@ -101,7 +112,9 @@ func TestSafeWriterEscapesButKeepsNewlines(t *testing.T) {
 	if err != nil || n != len(in) {
 		t.Fatalf("Write = %d, %v", n, err)
 	}
-	if got, want := b.String(), "a\\x1b[2J\nb\\u202e\n"; got != want {
+	// Only the Write's final newline ends a line; one inside it came from
+	// quoted text and could fake a line of krino's own (re-review term F1).
+	if got, want := b.String(), "a\\x1b[2J\\x0ab\\u202e\n"; got != want {
 		t.Errorf("wrote %q, want %q", got, want)
 	}
 }
diff --git a/cmd/krino/hostile_test.go b/cmd/krino/hostile_test.go
index d5ce041..16a5b19 100644
--- a/cmd/krino/hostile_test.go
+++ b/cmd/krino/hostile_test.go
@@ -10,6 +10,8 @@ import (
 	"strings"
 	"testing"
 	"time"
+
+	"krino/internal/journal"
 )
 
 // hostileNames are file names a download can carry that try to take over
@@ -104,6 +106,36 @@ func TestHostileNamesNeverReachTheTerminal(t *testing.T) {
 	checkAny("-\x1b[2Jflag.txt")
 	os.Remove(link)
 
+	// A newline in a name given on the command line must not start a line
+	// that reads as krino's own (re-review term F1).
+	forged := filepath.Join(dl, "gone\nkrino: FORGED line")
+	for _, line := range strings.Split(checkAny("explain", forged), "\n") {
+		if strings.HasPrefix(line, "krino: FORGED") {
+			t.Errorf("a quoted name forged a stderr line: %q", line)
+		}
+	}
+
+	// Hand-written hostile log fields: a damaged or edited log is shown
+	// escaped too (re-review term F4).
+	j, err := journal.Open(filepath.Join(h, ".local", "state", "krino", "krino.log"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	future := time.Date(2027, 1, 1, 0, 0, 0, 0, time.UTC)
+	hostileRun := "20270101T000000-\x1b]0;pwned\x07\u202e"
+	for _, e := range []journal.Entry{
+		{Time: future, Run: hostileRun, Action: "run-start", Status: "ok"},
+		{Time: future, Run: hostileRun, Dir: "dl\x1b[2J", File: "a.pdf", Step: 1, Action: "mo\x1b[31mve", Status: "ok", Src: "/x", Dst: "/y"},
+		{Time: future, Run: hostileRun, Action: "run-end", Status: "ok"},
+	} {
+		if err := j.Append(e); err != nil {
+			t.Fatal(err)
+		}
+	}
+	j.Close()
+	checkAny("log")
+	checkAny("undo", "-n")
+
 	// A newline inside file content (a zip entry's name) must not forge an
 	// explain trace line (review M5).
 	var buf bytes.Buffer
@@ -130,3 +162,34 @@ func TestHostileNamesNeverReachTheTerminal(t *testing.T) {
 	check("log")
 	check("undo", "-n")
 }
+
+// TestHostileDirectoryPathIsEscaped: a (path ...) holding escape codes - a
+// directory unpacked from a download - is shown escaped by check and in the
+// plan's header (re-review cli F1, term F5).
+func TestHostileDirectoryPathIsEscaped(t *testing.T) {
+	h := home(t)
+	dir := filepath.Join(h, "x\x1b]0;pwned\x07\u202ey")
+	if err := os.MkdirAll(dir, 0o755); err != nil {
+		t.Fatal(err)
+	}
+	if code, _, errOut := runCLI(t, "init"); code != 0 {
+		t.Fatal(errOut)
+	}
+	conf := filepath.Join(h, ".config", "krino")
+	if err := os.WriteFile(filepath.Join(conf, "krino.conf"), []byte("(include \"evil\")\n"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	body := "(path \"" + strings.ReplaceAll(dir, `\`, `\\`) + "\")\n(rule \"all\" (move \"Out\"))\n"
+	if err := os.WriteFile(filepath.Join(conf, "dirs", "evil.conf"), []byte(body), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	for _, args := range [][]string{{"check"}, {"-n"}} {
+		code, out, errOut := runCLI(t, args...)
+		if code != 0 {
+			t.Fatalf("krino %q: exit %d\n%s\n%s", args, code, out, errOut)
+		}
+		if bad := firstUnsafe(out+errOut, true); bad != "" {
+			t.Errorf("krino %q printed %s:\n%q", args, bad, out)
+		}
+	}
+}
diff --git a/cmd/krino/init.go b/cmd/krino/init.go
index 4383a14..1806ab9 100644
--- a/cmd/krino/init.go
+++ b/cmd/krino/init.go
@@ -31,7 +31,7 @@ func cmdInit(g *globals, args []string, stdout, stderr io.Writer) int {
 		return 2
 	}
 	for _, f := range created {
-		fmt.Fprintf(stdout, "created %s\n", xdg.Abbrev(f))
+		fmt.Fprintf(stdout, "created %s\n", display(xdg.Abbrev(f)))
 	}
 	fmt.Fprintln(stdout, "next: krino new NAME PATH, for example: krino new downloads ~/Downloads")
 	return 0
diff --git a/cmd/krino/main.go b/cmd/krino/main.go
index 29da31a..c787cbf 100644
--- a/cmd/krino/main.go
+++ b/cmd/krino/main.go
@@ -110,7 +110,8 @@ func run(args []string, stdout, stderr io.Writer) int {
 	// -weird-dir" ambiguous between the two syntaxes.
 	for _, a := range rest {
 		if strings.HasPrefix(a, "-") {
-			fmt.Fprintf(stderr, "krino: %s: flags must come before directory names\nrun 'krino -h' for help\n", a)
+			fmt.Fprintf(stderr, "krino: %s: flags must come before directory names\n", a)
+			fmt.Fprintln(stderr, "run 'krino -h' for help")
 			return 2
 		}
 	}
@@ -159,7 +160,8 @@ func parse(fs *flag.FlagSet, args []string, stdout, stderr io.Writer) (int, bool
 		fmt.Fprint(stdout, usage)
 		return 0, false
 	default:
-		fmt.Fprintf(stderr, "krino: %v\nrun 'krino -h' for help\n", err)
+		fmt.Fprintf(stderr, "krino: %v\n", err)
+		fmt.Fprintln(stderr, "run 'krino -h' for help")
 		return 2, false
 	}
 }
diff --git a/cmd/krino/new.go b/cmd/krino/new.go
index 37f2418..ced9dc2 100644
--- a/cmd/krino/new.go
+++ b/cmd/krino/new.go
@@ -30,7 +30,7 @@ func cmdNew(g *globals, args []string, stdout, stderr io.Writer) int {
 		fmt.Fprintf(stderr, "krino: %v\n", err)
 		return 2
 	}
-	fmt.Fprintf(stdout, "created %s and added %q to include\n", xdg.Abbrev(file), name)
+	fmt.Fprintf(stdout, "created %s and added %q to include\n", display(xdg.Abbrev(file)), name)
 	fmt.Fprintf(stdout, "edit its rules, then check them with: krino check %s\n", name)
 	return 0
 }
diff --git a/cmd/krino/sort.go b/cmd/krino/sort.go
index 52875ed..4d5f509 100644
--- a/cmd/krino/sort.go
+++ b/cmd/krino/sort.go
@@ -170,7 +170,7 @@ func cmdSort(g *globals, names []string, stdout, stderr io.Writer) int {
 					fmt.Fprintln(stdout)
 				}
 				printed = true
-				fmt.Fprintln(stdout, p.bold(fmt.Sprintf("krino: %s  %s", d.Name, xdg.Abbrev(d.Root))))
+				fmt.Fprintln(stdout, p.bold(fmt.Sprintf("krino: %s  %s", display(d.Name), display(xdg.Abbrev(d.Root)))))
 			}
 			// C3: directory-level warnings go to stderr after the header
 			// line above, not before it, so on a terminal they read as