plan 9: errors, explain traces, JSON and log fields never reach the terminal raw

8 files changed, 150 insertions, 16 deletions

diff --git a/cmd/krino/display.go b/cmd/krino/display.go
index 52e8385..193d8b8 100644
--- a/cmd/krino/display.go
+++ b/cmd/krino/display.go
@@ -4,8 +4,12 @@ package main
 
 import (
 	"fmt"
+	"io"
 	"strings"
+	"unicode"
 	"unicode/utf8"
+
+	"krino/internal/cond"
 )
 
 // display makes s safe to print to a terminal (spec §15.1). File names,
@@ -46,9 +50,58 @@ func display(s string) string {
 	return b.String()
 }
 
-// controlRune reports whether r is a C1 control or a Unicode bidirectional
-// embedding, override or isolate: printable-looking code points a terminal
-// acts on.
+// controlRune reports whether r is a C1 control, a Unicode bidirectional
+// control (embeddings, overrides, isolates, and the marks U+061C, U+200E and
+// U+200F), or a line or paragraph separator: code points a terminal acts on,
+// or that reorder or break the lines krino prints.
 func controlRune(r rune) bool {
-	return (r >= 0x80 && r <= 0x9f) || (r >= 0x202a && r <= 0x202e) || (r >= 0x2066 && r <= 0x2069)
+	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).
+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)
+	}
+	if _, err := io.WriteString(s.w, strings.Join(lines, "\n")); err != nil {
+		return 0, err
+	}
+	return len(p), nil
+}
+
+// jsonSafe escapes, in an encoded JSON document, the code points
+// encoding/json leaves raw that a terminal acts on - DEL, C1 controls,
+// bidirectional controls - as \uXXXX. They can only occur inside strings,
+// where the escape is the same value (review terminal F4).
+func jsonSafe(b []byte) []byte {
+	s := string(b)
+	var out strings.Builder
+	out.Grow(len(s))
+	for _, r := range s {
+		if r == 0x7f || controlRune(r) {
+			fmt.Fprintf(&out, `\u%04x`, r)
+			continue
+		}
+		out.WriteRune(r)
+	}
+	return []byte(out.String())
+}
+
+// displayTrace returns a copy of t with every label and error passed through
+// display, so a newline or escape in a file name or a tool's message cannot
+// forge or disturb an explain trace line (review M5).
+func displayTrace(t *cond.Trace) *cond.Trace {
+	c := *t
+	c.Label, c.Err = display(t.Label), display(t.Err)
+	c.Children = make([]*cond.Trace, len(t.Children))
+	for i, ch := range t.Children {
+		c.Children[i] = displayTrace(ch)
+	}
+	return &c
 }
diff --git a/cmd/krino/display_test.go b/cmd/krino/display_test.go
index 4c1cefe..0f4cf90 100644
--- a/cmd/krino/display_test.go
+++ b/cmd/krino/display_test.go
@@ -6,13 +6,15 @@ import (
 	"fmt"
 	"strings"
 	"testing"
+	"unicode"
 	"unicode/utf8"
 )
 
-// firstUnsafe names the first thing in s a terminal could act on - a C0
-// control (a newline too, unless allowNewline), DEL, a C1 control, a
-// bidirectional control, or invalid UTF-8 - or returns "" when there is
-// none.
+// 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.
 func firstUnsafe(s string, allowNewline bool) string {
 	if !utf8.ValidString(s) {
 		return "invalid UTF-8"
@@ -21,7 +23,7 @@ func firstUnsafe(s string, allowNewline bool) string {
 		if r == '\n' && allowNewline {
 			continue
 		}
-		if r < 0x20 || r == 0x7f || (r >= 0x80 && r <= 0x9f) || (r >= 0x202a && r <= 0x202e) || (r >= 0x2066 && r <= 0x2069) {
+		if unicode.IsControl(r) || unicode.Is(unicode.Bidi_Control, r) || unicode.In(r, unicode.Zl, unicode.Zp) {
 			return fmt.Sprintf("%U", r)
 		}
 	}
@@ -46,6 +48,10 @@ func TestDisplayEscapesTerminalControls(t *testing.T) {
 		"isolate\u2066x\u2069.pdf": `isolate\u2066x\u2069.pdf`,
 		"bad\xffbyte.pdf":          `bad\xffbyte.pdf`,
 		`back\slash.pdf`:           `back\slash.pdf`,
+		"alm\u061c.pdf":            `alm\u061c.pdf`,
+		"lrm\u200e.pdf":            `lrm\u200e.pdf`,
+		"rlm\u200f.pdf":            `rlm\u200f.pdf`,
+		"ls\u2028ps\u2029.pdf":     `ls\u2028ps\u2029.pdf`,
 		"replacement\ufffd.pdf":    "replacement\ufffd.pdf",
 	} {
 		if got := display(in); got != want {
@@ -84,3 +90,29 @@ func TestReviewEscapesHostileNames(t *testing.T) {
 		t.Errorf("undo review printed %s:\n%q", bad, undoOut)
 	}
 }
+
+// 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).
+func TestSafeWriterEscapesButKeepsNewlines(t *testing.T) {
+	var b strings.Builder
+	in := "a\x1b[2J\nb\u202e\n"
+	n, err := safeWriter{&b}.Write([]byte(in))
+	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 {
+		t.Errorf("wrote %q, want %q", got, want)
+	}
+}
+
+// TestJSONSafeEscapesTerminalRunes: in an encoded JSON document, DEL, C1 and
+// bidirectional controls come out as \uXXXX escapes - the same string
+// values - and everything else is untouched (review terminal F4).
+func TestJSONSafeEscapesTerminalRunes(t *testing.T) {
+	in := "{\"rel\": \"c1\u009bx\u202ey\u007fz ż\"}"
+	want := `{"rel": "c1\u009bx\u202ey\u007fz ż"}`
+	if got := string(jsonSafe([]byte(in))); got != want {
+		t.Errorf("jsonSafe = %q, want %q", got, want)
+	}
+}
diff --git a/cmd/krino/explain.go b/cmd/krino/explain.go
index b69f50f..8d5418e 100644
--- a/cmd/krino/explain.go
+++ b/cmd/krino/explain.go
@@ -82,8 +82,8 @@ func cmdExplain(g *globals, args []string, stdout, stderr io.Writer) int {
 // spaces.
 func printTrace(w io.Writer, t *cond.Trace) {
 	var buf bytes.Buffer
-	t.Format(&buf)
+	displayTrace(t).Format(&buf)
 	for _, line := range strings.Split(strings.TrimRight(buf.String(), "\n"), "\n") {
-		fmt.Fprintf(w, "  %s\n", display(line))
+		fmt.Fprintf(w, "  %s\n", line)
 	}
 }
diff --git a/cmd/krino/hostile_test.go b/cmd/krino/hostile_test.go
index a2b5d39..d5ce041 100644
--- a/cmd/krino/hostile_test.go
+++ b/cmd/krino/hostile_test.go
@@ -3,6 +3,8 @@
 package main
 
 import (
+	"archive/zip"
+	"bytes"
 	"os"
 	"path/filepath"
 	"strings"
@@ -80,6 +82,50 @@ func TestHostileNamesNeverReachTheTerminal(t *testing.T) {
 		t.Errorf("names and the tool's message should be shown escaped:\n%s", out)
 	}
 	check("explain", filepath.Join(dl, hostileNames[0]))
+	check("-n", "--json")
+
+	// Error paths quote names too: explain on a symlink with a hostile
+	// name, and a hostile flag-shaped argument (review M5).
+	checkAny := func(args ...string) string {
+		t.Helper()
+		_, out, errOut := runCLI(t, args...)
+		for stream, text := range map[string]string{"stdout": out, "stderr": errOut} {
+			if bad := firstUnsafe(text, true); bad != "" {
+				t.Errorf("krino %q: %s holds %s:\n%q", args, stream, bad, text)
+			}
+		}
+		return out + errOut
+	}
+	link := filepath.Join(dl, "link\x1b[2Jsym.txt")
+	if err := os.Symlink(filepath.Join(dl, hostileNames[0]), link); err != nil {
+		t.Fatal(err)
+	}
+	checkAny("explain", link)
+	checkAny("-\x1b[2Jflag.txt")
+	os.Remove(link)
+
+	// A newline inside file content (a zip entry's name) must not forge an
+	// explain trace line (review M5).
+	var buf bytes.Buffer
+	zw := zip.NewWriter(&buf)
+	w, err := zw.Create("xl/worksheets/sheet1.xml)\nrule forged: MATCH\nx.xml")
+	if err != nil {
+		t.Fatal(err)
+	}
+	w.Write([]byte("<a"))
+	zw.Close()
+	xlsx := filepath.Join(dl, "report.xlsx")
+	if err := os.WriteFile(xlsx, buf.Bytes(), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	os.Chtimes(xlsx, old, old)
+	for _, line := range strings.Split(checkAny("explain", xlsx), "\n") {
+		if strings.TrimSpace(line) == "rule forged: MATCH" {
+			t.Errorf("a zip entry name forged an explain line: %q", line)
+		}
+	}
+	os.Remove(xlsx)
+
 	check("-y")
 	check("log")
 	check("undo", "-n")
diff --git a/cmd/krino/log.go b/cmd/krino/log.go
index 6904a93..2f6c521 100644
--- a/cmd/krino/log.go
+++ b/cmd/krino/log.go
@@ -107,7 +107,7 @@ func styleUndone(line string, p palette) string {
 // the directories it touched, and its counts (see countsText), with
 // "(undone)" appended when a later run has reversed it.
 func formatRun(r journal.Run) string {
-	line := fmt.Sprintf("%s  %s  %s  %s", r.ID, r.Start.Format("2006-01-02 15:04"), display(strings.Join(r.Dirs, ", ")), countsText(r.Counts))
+	line := fmt.Sprintf("%s  %s  %s  %s", display(r.ID), r.Start.Format("2006-01-02 15:04"), display(strings.Join(r.Dirs, ", ")), countsText(r.Counts))
 	if r.Undone {
 		line += "  (undone)"
 	}
diff --git a/cmd/krino/main.go b/cmd/krino/main.go
index a8fab2d..7a6a5f7 100644
--- a/cmd/krino/main.go
+++ b/cmd/krino/main.go
@@ -77,6 +77,9 @@ func main() {
 
 // run is main without the process exit, so tests can drive it.
 func run(args []string, stdout, stderr io.Writer) int {
+	// Errors and warnings quote file names and tool messages: all of stderr
+	// goes through display (spec §15.1).
+	stderr = safeWriter{stderr}
 	g := &globals{}
 	fs := flagSet("krino", g)
 	fs.BoolVar(&g.yes, "y", false, "")
diff --git a/cmd/krino/sort.go b/cmd/krino/sort.go
index 947d21a..5e52f84 100644
--- a/cmd/krino/sort.go
+++ b/cmd/krino/sort.go
@@ -294,7 +294,7 @@ func cmdSort(g *globals, names []string, stdout, stderr io.Writer) int {
 			fmt.Fprintf(stderr, "krino: %v\n", err)
 			return 1
 		}
-		stdout.Write(b)
+		stdout.Write(jsonSafe(b))
 		fmt.Fprintln(stdout)
 	}
 
diff --git a/cmd/krino/undo.go b/cmd/krino/undo.go
index c942ae1..4c85185 100644
--- a/cmd/krino/undo.go
+++ b/cmd/krino/undo.go
@@ -146,7 +146,7 @@ func cmdUndo(g *globals, args []string, stdout, stderr io.Writer) int {
 		}()
 	}
 
-	fmt.Fprintln(stdout, p.bold("krino: undo "+up.Run))
+	fmt.Fprintln(stdout, p.bold("krino: undo "+display(up.Run)))
 	var buf bytes.Buffer
 	printUndoPlan(&buf, up)
 	text := colourRefused(buf.String(), p)
@@ -465,7 +465,7 @@ const undoStepWidth = len("undo-displace")
 // no root-relative form to render here the way actionCell has.
 func undoActionCell(s engine.UndoStep) string {
 	if s.Refused != "" {
-		return padCell(s.Action, undoStepWidth) + "  refused: " + display(s.Refused)
+		return padCell(display(s.Action), undoStepWidth) + "  refused: " + display(s.Refused)
 	}
 	switch s.Action {
 	case "undo-mkdir":
@@ -473,7 +473,7 @@ func undoActionCell(s engine.UndoStep) string {
 	case "undo-copy":
 		return padCell(s.Action, undoStepWidth) + "  " + display(xdg.Abbrev(s.Src)) + " → trash"
 	}
-	return padCell(s.Action, undoStepWidth) + "  → " + display(xdg.Abbrev(s.Dst))
+	return padCell(display(s.Action), undoStepWidth) + "  → " + display(xdg.Abbrev(s.Dst))
 }
 
 // printUndoPlan renders an undo plan the way krino undo shows it, below the