From a91b713dcec4d17f76155f0cd6b26903c59b9c19 Mon Sep 17 00:00:00 2001 From: Lukasz Kasprzak Date: Mon, 14 Sep 2026 22:31:12 +0200 Subject: plan 10: stderr messages cannot be split by quoted newlines; config paths escaped; krino new refuses control characters; independent terminal oracle --- cmd/krino/check.go | 14 +++++------ cmd/krino/common.go | 3 ++- cmd/krino/display.go | 20 +++++++++------ cmd/krino/display_test.go | 35 +++++++++++++++++--------- cmd/krino/hostile_test.go | 63 +++++++++++++++++++++++++++++++++++++++++++++++ cmd/krino/init.go | 2 +- cmd/krino/main.go | 6 +++-- cmd/krino/new.go | 2 +- cmd/krino/sort.go | 2 +- 9 files changed, 115 insertions(+), 32 deletions(-) (limited to 'cmd') 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 -- cgit v1.3