``\u200b``
\u200b`x
x`\u200b
``\u200b``\nafter", "`````\n``\u200b``\nafter\n`````"}, + {"
``\u200d``", "`````\n``\u200d``\n`````"}, + } { + got := toMarkdown(test.in) + if got != test.want { + t.Errorf("ToMarkdown(%q) = %q, want %q", test.in, got, test.want) + } + } +} + func TestToMarkdownFencedCodeCannotBeClosedFromInside(t *testing.T) { got := toMarkdown("
first\n```\nnot outside\n
after
") want := "````\nfirst\n```\nnot outside\n````\n\nafter" @@ -328,13 +347,33 @@ func TestToMarkdownAutolinkOnlyForAbsoluteURLs(t *testing.T) { } } -// A label that reads as one URL while pointing at another never collapses into an -// autolink: the destination is written beside the label, where it can be compared. +// A label that reads as one URL or host while pointing at another never collapses +// into an autolink: the destination is written beside the label, where it can be +// compared. That holds for a homoglyph host as much as an honest one — the Cyrillic +// а in "pаypal" is not detected, it simply is not the href — and a conformant +// renderer links to the destination while showing the label. func TestToMarkdownDeceptiveLabelShowsTheDestination(t *testing.T) { - got := toMarkdown(``) - want := "[https://bank.example/login](https://evil.example/login)" - if got != want { - t.Errorf("ToMarkdown = %q, want %q", got, want) + const homoglyphHost = "https://p\u0430ypal.com/login" + for _, test := range []struct { + name, label, href, want string + }{ + {"a URL", "https://bank.example/login", "https://evil.example/login", "[https://bank.example/login](https://evil.example/login)"}, + {"a homoglyph URL", homoglyphHost, "https://evil.example/login", "[" + homoglyphHost + "](https://evil.example/login)"}, + {"a www host", "www.bank.example", "https://evil.example", "[www.bank.example](https://evil.example)"}, + {"a bare host and path", "bank.example/login", "https://evil.example/login", "[bank.example/login](https://evil.example/login)"}, + {"the same host, a different path", "https://bank.example/", "https://bank.example/login", "[https://bank.example/](https://bank.example/login)"}, + {"a label that is its href", homoglyphHost, homoglyphHost, "<" + homoglyphHost + ">"}, + } { + got := toMarkdown(``) + if got != test.want { + t.Errorf("%s: ToMarkdown = %q, want %q", test.name, got, test.want) + } + if links := renderedLinks(t, got); len(links) != 1 || links[0] != test.href { + t.Errorf("%s: rendered links = %q, want the destination %q", test.name, links, test.href) + } + if text := renderedText(t, got); text != test.label { + t.Errorf("%s: rendered = %q, want the label %q", test.name, text, test.label) + } } } @@ -524,6 +563,7 @@ func FuzzToMarkdownTerminalSafety(f *testing.F) { `| | |
caf\u009c\u0085e
", + ``, } { f.Add(seed) } diff --git a/internal/markdown/render_test.go b/internal/markdown/render_test.go index f4624f6cc..4924716a6 100644 --- a/internal/markdown/render_test.go +++ b/internal/markdown/render_test.go @@ -58,6 +58,19 @@ func TestRenderLinksAreClickable(t *testing.T) { } } +// The confusable policy covers a link's destination as well as its text: a zero-width +// character in the href is dropped from the OSC 8 target the same way it is dropped from +// the label, so the link goes where the reader sees it going. +func TestRenderLinksToWhatItShows(t *testing.T) { + got := render("[https://example.com/a\u200bb](https://example.com/a\u200bb)", 80) + if strings.Contains(got, "\u200b") { + t.Errorf("Render = %q, a zero-width space survived", got) + } + if !strings.Contains(got, ";https://example.com/ab\a") { + t.Errorf("Render = %q, want the OSC 8 target to be the URL as shown", got) + } +} + func TestRenderFallsBackToDefaultWidth(t *testing.T) { if render("Hello", 0) == "" { t.Error("Render with no width returned nothing") diff --git a/internal/markdown/safety_test.go b/internal/markdown/safety_test.go index a01dc52a6..b3ca492a5 100644 --- a/internal/markdown/safety_test.go +++ b/internal/markdown/safety_test.go @@ -80,6 +80,36 @@ func TestRenderKeepsQueryStringsInHyperlinks(t *testing.T) { } } +// A label that reads as one host while pointing at another shows its destination in +// the rendering, and the hyperlink goes where the destination says. +func TestRenderDeceptiveLabelShowsTheDestination(t *testing.T) { + out := render("[https://p\u0430ypal.com/login](https://evil.example/login)", 200) + if !strings.Contains(visible(out), "https://evil.example/login") { + t.Errorf("Render = %q, want the destination visible", out) + } + if !strings.Contains(out, ";https://evil.example/login\x07") { + t.Errorf("Render = %q, want a hyperlink to the destination", out) + } + if strings.Contains(out, ";https://p\u0430ypal.com") { + t.Errorf("Render = %q, want no hyperlink to the label's host", out) + } +} + +// A body goes through the same confusable policy as a subject line: invisible +// format characters go, a stack of combining marks is cut, and an emoji family keeps +// its joiners. +func TestRenderStripsConfusables(t *testing.T) { + for md, want := range map[string]string{ + "pay\u200bpal and invoice\u00adpdf.exe": "paypal and invoicepdf.exe", + "Z" + strings.Repeat("\u0336", 50) + "algo": "Z" + strings.Repeat("\u0336", 8) + "algo", + "the 👨\u200d👩\u200d👧 family": "the 👨\u200d👩\u200d👧 family", + } { + if out := visible(render(md, 200)); !strings.Contains(out, want) { + t.Errorf("Render(%q) shows %q, want %q in it", md, out, want) + } + } +} + func TestRenderStripsRawControls(t *testing.T) { for name, md := range map[string]string{ "escape": "hello \x1b[31mRED", @@ -282,6 +312,7 @@ func FuzzContainment(f *testing.F) { "| a |\n| --- |\n| [31m |", "", "a\u0085b\u009cc", + "pay\u200bpal Z" + strings.Repeat("\u0336", 8) + "algo [https://p\u0430ypal.com](https://evil.example)", } { f.Add(seed) } diff --git a/internal/markdown/source.go b/internal/markdown/source.go index 8f0caad0b..c7a62abd9 100644 --- a/internal/markdown/source.go +++ b/internal/markdown/source.go @@ -55,10 +55,16 @@ func prepareSource(md string) (safe, forGlamour string, deep bool) { // milliseconds. const maxNestingDepth = 20 -// stripControls removes escape sequences, C0 and C1 controls and the bidirectional -// controls, keeping newlines and tabs. A body is prose, and the Trojan-source class -// — a right-to-left override that shows one thing and means another — is no more -// welcome in it than in a subject line; the JSON keeps the original. +// stripControls removes escape sequences, C0 and C1 controls, the bidirectional +// controls and the confusables terminal.Sanitize describes, keeping newlines and tabs. +// A body is prose, and the Trojan-source class — a right-to-left override that shows +// one thing and means another — is no more welcome in it than in a subject line; the +// JSON keeps the original. +// +// It runs over the whole source, link destinations included, on purpose: what a link +// shows and where it goes are then the same text, and a destination that differs from +// the one shown only by a zero-width character is exactly the confusable the policy +// removes. Sparing destinations would link a URL the reader cannot see to read. func stripControls(s string) string { return terminal.Sanitize(s) } diff --git a/internal/terminal/sanitize.go b/internal/terminal/sanitize.go index d0c5a03c3..32dc9303b 100644 --- a/internal/terminal/sanitize.go +++ b/internal/terminal/sanitize.go @@ -12,33 +12,197 @@ // "invoice" and "fdp.exe" shows a PDF on screen and an executable on disk, and an // isolate can swap the order of a sender's name and address. Nothing HEY shows in a single line needs // them — an RTL name still reads right-to-left without an explicit override. +// +// # Confusables +// +// Past the controls lies text that reads as one thing and is another. The policy, +// class by class: +// +// Format characters that draw nothing are stripped: the soft hyphen, the zero +// width space, the word joiner, the byte order mark, the combining grapheme joiner, +// the Mongolian vowel separator, the invisible mathematical operators and the +// deprecated format controls next to them. A zero width space in "paypal" or a soft +// hyphen in "invoicepdf.exe" renders as the honest spelling while being a different +// string, which is the same class as a bidi override. They are stripped rather than +// replaced, since a replacement mark is visible debris in a name column. The list is +// enumerated; it is not the Cf category wholesale, which also holds the joiners +// below, the Arabic number and verse signs that are visible, and the tag characters +// a subdivision flag is made of. +// +// The zero width joiner and non-joiner are kept where they can join. They carry +// text: an emoji family is emoji joined by U+200D, and Persian, Urdu and the Indic +// scripts write with both. A joiner survives only between two non-ASCII, +// non-space base characters — the marks on the left base, a virama or an emoji +// presentation selector, are part of it, but a mark is not a base on its own — and a +// run of them collapses to one; at the start or end of a string, or next to an +// ASCII letter — a joiner inside "paypal" — it joins nothing and is dropped. +// +// Combining marks are kept up to eight on a base and the rest of the run dropped. +// The deepest stacks a writing system produces reach five — a fully pointed Hebrew +// letter with its shin dot, dagesh, vowel, meteg and cantillation; a Tibetan stack +// with two subjoined letters and a vowel sign is four — where decomposed Vietnamese, +// Hindi and the keycap emoji use two, so eight is room for any of them and Zalgo +// needs dozens to climb out of its cell. Variation selectors are marks and sit +// under the same cap, so the U+FE0F that makes a heart red stays. A format +// character that is kept — a tag character, an Arabic number sign — is not a base: +// it draws nothing of its own, so the marks after it still belong to the letter +// before it and count against the same cap. +// +// A byte that is not UTF-8 is dropped. Left as it is, a stray 0x85 is a C1 control +// to a terminal that reads bytes; and it goes the way everything else here goes, +// stripped rather than replaced, so the output is never longer than the input. +// +// Spaces that are not U+0020 are left alone. A no-break space is ordinary in text +// that came out of HTML, as are the fixed-width spaces; they render as a space and +// read as one, and normalizing them would rewrite names for no safety gain. +// +// Homoglyphs — a Cyrillic "а" in a Latin name — are not detected. Without +// confusable tables and a script heuristic the check cannot tell a forged name from +// a multilingual one, and the false positives land on real people. What a link's +// label says against where it goes is the Markdown serializer's business: a label +// that reads as a URL is written beside its destination rather than collapsed into +// it (see htmlutil.ToMarkdown). package terminal import ( "strings" + "unicode" + "unicode/utf8" "github.com/charmbracelet/x/ansi" ) var lineBreaks = strings.NewReplacer("\n", " ", "\t", " ") -// Sanitize removes escape sequences, control characters and bidirectional controls -// from text on its way to a terminal. Newlines and tabs survive, because text is not -// necessarily one line: a message body, a jq result and a Markdown cell all carry -// them on purpose. +// maxCombiningMarks is how many combining marks may follow one base before the +// rest of the run is dropped: the package doc says why eight. +const maxCombiningMarks = 8 + +const ( + zeroWidthNonJoiner = 0x200c + zeroWidthJoiner = 0x200d +) + +// Sanitize removes escape sequences, control characters, bidirectional controls +// and the confusables described in the package doc from text on its way to a +// terminal. Newlines and tabs survive, because text is not necessarily one line: a +// message body, a jq result and a Markdown cell all carry them on purpose. +// +// One pass, and no allocation for text that needs nothing removed. Every decision +// is made on what has been kept, so sanitizing twice is the same as once. func Sanitize(value string) string { - return strings.Map(func(r rune) rune { + p := pass{in: ansi.Strip(value), kept: -1, base: -1} + for i, r := range p.in { switch { case r == '\n' || r == '\t': - return r - case r < 0x20 || r == 0x7f || (r >= 0x80 && r <= 0x9f): - return -1 - case isBidiControl(r): - return -1 + p.keep(r) + p.marks = 0 + case r == utf8.RuneError && !strings.HasPrefix(p.in[i:], "\ufffd"): + // An invalid byte, not a replacement character the text carried. + p.drop(i) + case isControl(r), isBidiControl(r), isInvisible(r): + p.drop(i) + case r == zeroWidthJoiner || r == zeroWidthNonJoiner: + p.hold(i, r) + case isCombiningMark(r): + if p.marks < maxCombiningMarks { + p.keep(r) + p.marks++ + } else { + p.drop(i) + } default: - return r + p.keep(r) + if !unicode.Is(unicode.Cf, r) { + p.marks = 0 + } + } + } + return p.String() +} + +// pass is one walk over the input. The output is only built once something is +// dropped; until then the input is its own output. A joiner is held back until the +// rune after it is kept, and written only when that rune is something it can join. +type pass struct { + in string + out *strings.Builder + kept rune // the last rune written, other than a joiner + base rune // the last rune written that is not a mark, other than a joiner + marks int // combining marks written since the last base + joiner rune // a joiner waiting for its right-hand side, or zero + joinerAt int // its byte offset +} + +func (p *pass) keep(r rune) { + if p.joiner != 0 { + if isBase(r) { + p.write(p.joiner) + } else { + p.diverge(p.joinerAt) } - }, ansi.Strip(value)) + p.joiner = 0 + } + p.write(r) + p.kept = r + if !isCombiningMark(r) { + p.base = r + } +} + +// isBase reports a rune a joiner can join: non-ASCII text that is neither a space, a +// mark nor a format character. The marks on a base ride along with it; a tag or any +// other format character ends it, so a joiner after one has nothing to join. +func isBase(r rune) bool { + return joinable(r) && !isCombiningMark(r) && !unicode.Is(unicode.Cf, r) +} + +func (p *pass) drop(i int) { + if p.joiner != 0 { + i = p.joinerAt + } + p.diverge(i) +} + +// hold keeps a joiner back until keep decides it. A joiner with no base on its +// left, or one held while another is pending, is dropped. +func (p *pass) hold(i int, r rune) { + switch { + case p.joiner != 0 || !isBase(p.base): + p.drop(i) + default: + p.joiner, p.joinerAt = r, i + } +} + +func (p *pass) write(r rune) { + if p.out != nil { + p.out.WriteRune(r) + } +} + +// diverge starts the output at the point the first dropped rune would have gone, +// which is i or the joiner pending before it. +func (p *pass) diverge(i int) { + if p.out == nil { + p.out = &strings.Builder{} + p.out.Grow(len(p.in)) + p.out.WriteString(p.in[:i]) + } +} + +func (p *pass) String() string { + if p.joiner != 0 { + p.diverge(p.joinerAt) + } + if p.out == nil { + return p.in + } + return p.out.String() +} + +func isControl(r rune) bool { + return r < 0x20 || r == 0x7f || (r >= 0x80 && r <= 0x9f) } // isBidiControl reports the Unicode Bidi_Control property: the Arabic letter mark, @@ -57,6 +221,34 @@ func isBidiControl(r rune) bool { } } +// isInvisible reports the format characters that draw nothing and carry no text: +// the soft hyphen, the combining grapheme joiner, the Mongolian vowel separator, +// the zero width space, the word joiner and the invisible operators after it, the +// deprecated format controls, and the byte order mark. +func isInvisible(r rune) bool { + switch { + case r == 0x00ad, r == 0x034f, r == 0x180e, r == 0x200b, r == 0xfeff: + return true + case r >= 0x2060 && r <= 0x2064: + return true + case r >= 0x206a && r <= 0x206f: + return true + default: + return false + } +} + +// joinable reports whether r is something a joiner next to it can join: non-ASCII +// text that is not a space. Whether it is a base rather than a mark is the caller's +// question. +func joinable(r rune) bool { + return r >= utf8.RuneSelf && !unicode.IsSpace(r) +} + +func isCombiningMark(r rune) bool { + return r >= 0x300 && unicode.In(r, unicode.Mn, unicode.Me, unicode.Mc) +} + // SanitizeLine is Sanitize for somewhere only one line fits — a table cell, a // confirmation — where a newline or a tab would move what comes after it rather // than merely reading oddly. Both become a space, so the words stay apart. diff --git a/internal/terminal/sanitize_test.go b/internal/terminal/sanitize_test.go index cfb295bdf..f69461840 100644 --- a/internal/terminal/sanitize_test.go +++ b/internal/terminal/sanitize_test.go @@ -1,28 +1,95 @@ package terminal -import "testing" +import ( + "strings" + "testing" -func TestSanitize(t *testing.T) { - tests := []struct { - name string - value string - want string - }{ - {"plain text is untouched", "Ryan Singer", "Ryan Singer"}, - {"a colour sequence goes with its payload", "\x1b[31mURGENT\x1b[0m", "URGENT"}, - {"a window title sequence leaves nothing behind", "\x1b]0;pwned\x07report.pdf", "report.pdf"}, - {"a stray escape takes the byte it would have escaped", "sub\x1bject", "subect"}, - {"a carriage return cannot rewrite the line", "Invoice\rPAID", "InvoicePAID"}, - {"a C1 control is removed", "caf" + string(rune(0x85)) + "e", "cafe"}, - {"DEL is removed", "note\x7f", "note"}, - {"newlines and tabs survive", "line one\nline\ttwo", "line one\nline\ttwo"}, - {"emoji and accents survive", "Café ☕", "Café ☕"}, - {"a right-to-left override cannot reverse a filename", "invoice\u202efdp.exe", "invoicefdp.exe"}, - {"every bidi control goes", "\u061ca\u200eb\u200fc\u202ad\u202be\u202cf\u202dg\u2066h\u2067i\u2068j\u2069k", "abcdefghijk"}, - {"right-to-left text itself survives", "שלום", "שלום"}, - } + "github.com/charmbracelet/x/ansi" +) - for _, test := range tests { +var sanitizeTests = []struct { + name string + value string + want string +}{ + {"plain text is untouched", "Ryan Singer", "Ryan Singer"}, + {"a colour sequence goes with its payload", "\x1b[31mURGENT\x1b[0m", "URGENT"}, + {"a window title sequence leaves nothing behind", "\x1b]0;pwned\x07report.pdf", "report.pdf"}, + {"a stray escape takes the byte it would have escaped", "sub\x1bject", "subect"}, + {"a carriage return cannot rewrite the line", "Invoice\rPAID", "InvoicePAID"}, + {"a C1 control is removed", "caf" + string(rune(0x85)) + "e", "cafe"}, + {"DEL is removed", "note\x7f", "note"}, + {"newlines and tabs survive", "line one\nline\ttwo", "line one\nline\ttwo"}, + {"emoji and accents survive", "Café ☕", "Café ☕"}, + {"a right-to-left override cannot reverse a filename", "invoice\u202efdp.exe", "invoicefdp.exe"}, + {"every bidi control goes", "\u061ca\u200eb\u200fc\u202ad\u202be\u202cf\u202dg\u2066h\u2067i\u2068j\u2069k", "abcdefghijk"}, + {"right-to-left text itself survives", "שלום", "שלום"}, + + // Invisible format characters draw nothing and are stripped. + {"a zero width space cannot split a domain", "pay\u200bpal@example.com", "paypal@example.com"}, + {"a soft hyphen cannot hide a filename's extension", "invoice\u00adpdf.exe", "invoicepdf.exe"}, + {"a byte order mark goes", "\ufeffReport", "Report"}, + {"a word joiner goes", "Q3\u2060 planning", "Q3 planning"}, + {"every invisible format character goes", "a\u00adb\u034fc\u180ed\u200be\u2060f\u2061g\u2062h\u2063i\u2064j\u206ak\u206fl\ufeffm", "abcdefghijklm"}, + + // Joiners stay where they join and go where they cannot. + {"an emoji family keeps its joiners", "👨\u200d👩\u200d👧", "👨\u200d👩\u200d👧"}, + {"a heart on fire keeps its selector and joiner", "❤\ufe0f\u200d🔥", "❤\ufe0f\u200d🔥"}, + {"a rainbow flag keeps its joiner", "🏳\ufe0f\u200d🌈", "🏳\ufe0f\u200d🌈"}, + {"a skin tone before a joiner keeps it", "👩🏽\u200d💻", "👩🏽\u200d💻"}, + {"Persian keeps its non-joiner", "می\u200cخواهم", "می\u200cخواهم"}, + {"Devanagari keeps a joiner between letters", "क्\u200dष", "क्\u200dष"}, + {"a joiner between ASCII letters goes", "pay\u200dpal", "paypal"}, + {"a non-joiner between ASCII letters goes", "pay\u200cpal", "paypal"}, + {"a joiner at either end goes", "\u200dRyan\u200d", "Ryan"}, + {"a joiner beside a space goes", "é\u200d é", "é é"}, + {"a run of joiners collapses to one", "é\u200d\u200d\u200dé", "é\u200dé"}, + {"a joiner after a stripped invisible is judged by what was kept", "a\u200b\u200dé", "aé"}, + {"a joiner at the end of non-ASCII text goes", "é\u200d", "é"}, + {"a joiner whose right-hand side is stripped goes", "é\u200d\u200b", "é"}, + {"a joiner whose right-hand side is a dropped mark goes", "e" + strings.Repeat("\u0301", 8) + "\u200d\u0301", "e" + strings.Repeat("\u0301", 8)}, + {"a run of joiners before a space goes", "é\u200d\u200d é", "é é"}, + {"a joiner after a virama joins", "क\u094d\u200dष", "क\u094d\u200dष"}, + {"a joiner after an emoji presentation selector joins", "❤\ufe0f\u200d🔥", "❤\ufe0f\u200d🔥"}, + {"a joiner before a mark goes", "é\u200d\u0301", "é\u0301"}, + {"a mark on an ASCII base does not make it joinable", "e\u0301\u200dé", "e\u0301é"}, + {"a joiner before a tag character goes", "é\u200d\U000e0020", "é\U000e0020"}, + {"a tag character is not a base for a joiner", "é\U000e0020\u200dé", "é\U000e0020é"}, + + // Combining marks are capped at eight on a base. + {"a precomposed accent survives", "Café", "Café"}, + {"a decomposed accent survives", "Cafe\u0301", "Cafe\u0301"}, + {"decomposed Vietnamese survives", "Vie\u0323\u0302t Nam", "Vie\u0323\u0302t Nam"}, + {"Hindi survives", "हिन्दी", "हिन्दी"}, + {"a nukta and virama survive", "क\u093c\u094dष", "क\u093c\u094dष"}, + {"Hebrew with four marks survives", "ש\u05c1\u05bc\u05b7\u05ab", "ש\u05c1\u05bc\u05b7\u05ab"}, + {"fully pointed Hebrew with five marks survives", "ש\u05c1\u05bc\u05b8\u05bd\u0591", "ש\u05c1\u05bc\u05b8\u05bd\u0591"}, + {"a Tibetan stack survives", "བསྒྲུབས", "བསྒྲུབས"}, + {"a keycap survives", "1\ufe0f\u20e3", "1\ufe0f\u20e3"}, + {"an emoji presentation selector survives", "❤\ufe0f", "❤\ufe0f"}, + {"Zalgo is cut at eight marks", "Z" + strings.Repeat("\u0336", 50) + "algo", "Z" + strings.Repeat("\u0336", 8) + "algo"}, + {"the cap resets on each base", "a" + strings.Repeat("\u0301", 9) + "b" + strings.Repeat("\u0301", 9), "a" + strings.Repeat("\u0301", 8) + "b" + strings.Repeat("\u0301", 8)}, + {"the cap resets on a newline", "a" + strings.Repeat("\u0301", 8) + "\n" + strings.Repeat("\u0301", 9), "a" + strings.Repeat("\u0301", 8) + "\n" + strings.Repeat("\u0301", 8)}, + {"a kept format character is not a base", "Z" + strings.Repeat("\u0336", 8) + "\U000e0020" + strings.Repeat("\u0336", 8), "Z" + strings.Repeat("\u0336", 8) + "\U000e0020"}, + {"a subdivision flag survives", "\U0001f3f4\U000e0067\U000e0062\U000e0073\U000e0063\U000e0074\U000e007f", "\U0001f3f4\U000e0067\U000e0062\U000e0073\U000e0063\U000e0074\U000e007f"}, + + // A byte that is not UTF-8 is dropped rather than reaching the terminal raw. + {"an invalid byte goes", "caf\xe9.txt", "caf.txt"}, + {"a raw C1 byte goes", "a\x85b", "ab"}, + {"a replacement character the text carried survives", "a\ufffdb", "a\ufffdb"}, + {"a joiner whose right-hand side is an invalid byte goes", "é\u200d\xe9", "é"}, + + // Spaces that are not U+0020 are left alone. + {"a no-break space survives", "Jean\u00a0Dupont", "Jean\u00a0Dupont"}, + {"an ideographic space survives", "山田\u3000太郎", "山田\u3000太郎"}, + {"a narrow no-break space survives", "12\u202f000", "12\u202f000"}, + + // Homoglyphs are not detected. + {"a Cyrillic letter in a Latin word is kept", "p\u0430ypal", "p\u0430ypal"}, +} + +func TestSanitize(t *testing.T) { + for _, test := range sanitizeTests { t.Run(test.name, func(t *testing.T) { if got := Sanitize(test.value); got != test.want { t.Errorf("Sanitize(%q) = %q, want %q", test.value, got, test.want) @@ -31,6 +98,30 @@ func TestSanitize(t *testing.T) { } } +// Sanitizing a second time changes nothing, and sanitizing never lengthens: every +// decision is made on what was kept, so the output is its own fixed point. +func TestSanitizeIsIdempotentAndNeverLonger(t *testing.T) { + for _, test := range sanitizeTests { + once := Sanitize(test.value) + if twice := Sanitize(once); twice != once { + t.Errorf("%s: Sanitize(Sanitize(%q)) = %q, want %q", test.name, test.value, twice, once) + } + if len(once) > len(test.value) { + t.Errorf("%s: Sanitize(%q) = %q is longer than its input", test.name, test.value, once) + } + } +} + +// Text that needs nothing removed is returned as it is: Sanitize allocates nothing +// beyond what ansi.Strip already does. +func TestSanitizeCleanTextDoesNotAllocate(t *testing.T) { + value := "Ryan Singer — Q3 planning ☕ Vie\u0323\u0302t Nam 👨\u200d👩\u200d👧" + stripping := testing.AllocsPerRun(100, func() { ansi.Strip(value) }) + if allocs := testing.AllocsPerRun(100, func() { Sanitize(value) }); allocs > stripping { + t.Errorf("Sanitize(%q) allocates %v times, want %v (ansi.Strip alone)", value, allocs, stripping) + } +} + func TestSanitizeLine(t *testing.T) { tests := []struct { name string @@ -41,6 +132,7 @@ func TestSanitizeLine(t *testing.T) { {"a tab becomes a space", "Ryan\tSinger", "Ryan Singer"}, {"an escape sequence still goes", "\x1b[2JLabel", "Label"}, {"plain text is untouched", "Q3 planning", "Q3 planning"}, + {"an invisible goes and a newline becomes a space", "pay\u200bpal\nRyan", "paypal Ryan"}, } for _, test := range tests { diff --git a/internal/tui/calendar.go b/internal/tui/calendar.go index a661399bf..411363d37 100644 --- a/internal/tui/calendar.go +++ b/internal/tui/calendar.go @@ -379,6 +379,11 @@ func (v *calendarView) handleTimeTrackCategoryKey(msg tea.KeyPressMsg) tea.Cmd { return v.createTimeTrackCategory(title) } if selected != nil { + // The input was filled with the title sanitized for the screen, so an + // unedited rename is no rename — saving it would rewrite the title. + if title == selected.Title || title == terminal.SanitizeLine(selected.Title) { + return nil + } return v.renameTimeTrackCategory(selected.Id, title) } manager.status = "Choose a category to rename" diff --git a/internal/tui/time_track_categories_test.go b/internal/tui/time_track_categories_test.go index 256386299..5f5da10d0 100644 --- a/internal/tui/time_track_categories_test.go +++ b/internal/tui/time_track_categories_test.go @@ -135,6 +135,10 @@ func TestCalendarTimeTrackCategoryMutations(t *testing.T) { if view.timeTrackCategories.mode != timeTrackCategoryRename { t.Fatal("enter should open category rename") } + if cmd := view.HandleContentKey(keyPress("enter")); cmd != nil || view.timeTrackCategories.mode != timeTrackCategoryBrowse { + t.Fatal("an unedited rename should send nothing") + } + view.HandleContentKey(keyPress("enter")) view.timeTrackCategories.input.SetValue("Customer work") rename := view.HandleContentKey(keyPress("enter")) if rename == nil {