diff --git a/VERSION b/VERSION index 1a46c7f..f314d02 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.10.8 +0.10.9 diff --git a/internal/cli/sanitize.go b/internal/cli/sanitize.go index f14ae13..0847bf2 100644 --- a/internal/cli/sanitize.go +++ b/internal/cli/sanitize.go @@ -3,41 +3,98 @@ package cli import ( "regexp" "strings" + "unicode" ) -// csiSequence matches an ANSI CSI sequence: ESC '[' . This is what a terminal emits for arrow keys / cursor moves -// (ESC[A/B/C/D, ESC[1;5C, ESC[3~ …) and for bracketed-paste wrappers -// (ESC[200~ … ESC[201~). It is deliberately broader than submit.stripANSI, -// which only strips SGR colour codes (final byte 'm'). -var csiSequence = regexp.MustCompile("\x1b\\[[0-9;]*[A-Za-z~]") +// escSequence matches the two ANSI escape families a terminal actually sends for +// cursor / function keys: +// +// CSI ESC '[' +// ESC[A/B/C/D (arrows), ESC[1;5C (Ctrl+arrow), ESC[3~ (Delete), +// ESC[200~ … ESC[201~ (bracketed paste) +// SS3 ESC 'O' +// ESC OA/OB/OC/OD (arrows), ESC OH/OF (Home/End), ESC OP…OS (F1–F4) +// +// SS3 is what the *same* keys emit once the terminal is in DECCKM +// application-cursor mode — the state vim, less or tmux leave behind on an +// unclean exit (cli#516). Handling only CSI was the remaining hole: an ESC alone +// is a C0 byte and gets dropped below, but 'O' and the final byte are printable, +// so "\x1bOD\x1bOD\x1bOA" survived as the plausible-looking name "ODODOA" and +// minted the immutable namespace "ododoa". CSI residue never did: it cleans to +// empty, fails the non-empty check and re-prompts. +// +// Deliberately broader than submit.stripANSI, which only strips SGR colour codes +// (final byte 'm'). +var escSequence = regexp.MustCompile("\x1b(?:\\[[0-9;]*|O)[A-Za-z~]") + +// escResidue is the post-sanitise floor's *probe*, not a strip: ESC, any run of +// non-final intermediate bytes, then AT MOST TWO bytes that could be escape final +// bytes. Its only job is to answer one question — "is there an alphanumeric here +// that did NOT come from an escape final byte?" — and its output is never +// returned as a name. +// +// Two, not one and not unbounded. One is too few: it leaves the 'D' of an +// unrecognised SS3-shaped pair behind, and the floor stops firing on exactly the +// family shape this ticket is about. Unbounded is too many: `\x1bNChello` would +// have the whole name swallowed and be refused, while `\x1bNC日本` survives — +// making keep-vs-reject depend on the script the user's name is written in +// (Bugbot, tracebloc/client#736). Two is the honest bound: an escape final is one +// byte, an intro plus a final is two, and every keyboard-input escape family +// (SS2, SS3, the 7-bit C1 forms) fits in that. +var escResidue = regexp.MustCompile("\x1b[^A-Za-z0-9~]*[A-Za-z~]{1,2}") // sanitizeClientName strips terminal escape sequences and C0 control characters // from a user-supplied client name or location before it becomes the stored // display name and is slugified into an immutable Kubernetes namespace. // -// Defense-in-depth for the name-garble bug (customer-reported 2026-07-20): typing -// arrow keys at the installer's name prompt injected raw ESC[D/ESC[A bytes. The -// installer now strips them at the source, but a name can also arrive here -// directly via --name / $TRACEBLOC_CLIENT_NAME, and slug.Slugify would otherwise -// turn each ESC[ run into a "-" — ESC (0x1B) survives Slugify's ASCII pass, so -// "se-\e[D\e[D\e[A\e[A" mints the garbage namespace "se-d-d-a-a". Cleaning here, -// at the CLI boundary, keeps slug.Slugify a faithful mirror of the backend's -// slug.py (which must NOT strip — the backend validates exactly what it produces); -// input hygiene belongs at ingestion, not in the shared slug rule. +// Defense-in-depth for the name-garble bug (customer-reported 2026-07-20, fixed +// 2026-07-21 in cli#364 / client#362): typing arrow keys at the installer's name +// prompt injected raw ESC[D/ESC[A bytes. The installer strips them at the source, +// but a name can also arrive here directly via --name / $TRACEBLOC_CLIENT_NAME, +// and slug.Slugify would otherwise turn each ESC[ run into a "-" — ESC (0x1B) +// survives Slugify's ASCII pass, so "se-\e[D\e[D\e[A\e[A" mints the garbage +// namespace "se-d-d-a-a". Cleaning here, at the CLI boundary, keeps slug.Slugify +// a faithful mirror of the backend's slug.py (which must NOT strip — the backend +// validates exactly what it produces); input hygiene belongs at ingestion, not in +// the shared slug rule. +// +// Escape-derived garbage cannot be caught downstream: is_dns1123_label validates +// by idempotence against the slug rule, so "ododoa" is a perfectly canonical +// label. Form is exactly what this class of input preserves — hence the floor +// below, which is about *content*, not form. // // UTF-8 bytes (>= 0x80) are preserved so international names survive. func sanitizeClientName(s string) string { - // 1) Whole CSI sequences (arrow keys, paste wrappers). ReplaceAll handles - // consecutive sequences in one pass; any orphaned ESC left behind is a C0 - // byte and is removed by step 3. - s = csiSequence.ReplaceAllString(s, "") + // 1) Whole escape sequences (arrow keys in either cursor mode, function keys, + // paste wrappers). ReplaceAll handles consecutive sequences in one pass; + // any orphaned ESC left behind is a C0 byte and is removed by step 4. + s = escSequence.ReplaceAllString(s, "") // 2) Post-corruption case: an earlier (buggy) sanitizer dropped the ESC but // left the literal bracketed-paste markers. Only these two well-defined // markers are removed — a generic "[x~" could be real name content. s = strings.ReplaceAll(s, "[200~", "") s = strings.ReplaceAll(s, "[201~", "") - // 3) Drop any remaining C0 control characters and DEL; keep printable ASCII + // 3) The floor. Steps 1–2 know CSI, SS3 and the paste markers; they cannot + // know the escape family nobody has reported yet, and that is precisely how + // SS3 got here — one rule, three hand-copied implementations, no member of + // the family beyond CSI ever tested. So: if an ESC SURVIVED step 1, this + // value carries an escape shape we do not recognise, and the printable + // bytes around it are not trustworthy name content. Require at least one + // alphanumeric that did not come from an escape final byte (at most two of + // them — see escResidue); if there is none, the whole value was residue — + // return "" and let the caller auto-name (the same path an omitted --name + // takes). + // + // Scoped to "an ESC survived" on purpose. It is the narrowest trigger that + // still covers unknown families: a clean name never reaches it, a name with + // real content next to an unknown escape keeps that content, and only a + // value that is *nothing but* escape residue is rejected. The failure it + // chooses is the recoverable one — auto-naming a name is annoying; minting + // a permanent namespace from keyboard noise is not. + if strings.ContainsRune(s, 0x1b) && !hasAlphanumeric(escResidue.ReplaceAllString(s, "")) { + return "" + } + // 4) Drop any remaining C0 control characters and DEL; keep printable ASCII // and all multi-byte UTF-8 (>= 0x80). return strings.Map(func(r rune) rune { if r < 0x20 || r == 0x7f { @@ -46,3 +103,11 @@ func sanitizeClientName(s string) string { return r }, s) } + +// hasAlphanumeric reports whether s contains any letter or digit, Unicode +// included — a name written entirely in a non-Latin script is real content. +func hasAlphanumeric(s string) bool { + return strings.IndexFunc(s, func(r rune) bool { + return unicode.IsLetter(r) || unicode.IsDigit(r) + }) >= 0 +} diff --git a/internal/cli/sanitize_test.go b/internal/cli/sanitize_test.go index ec4c765..5e9915a 100644 --- a/internal/cli/sanitize_test.go +++ b/internal/cli/sanitize_test.go @@ -22,6 +22,30 @@ func TestSanitizeClientName(t *testing.T) { {"CSI with params (Ctrl+arrow)", "x\x1b[1;5Dy", "xy"}, {"Delete key (ESC[3~)", "ab\x1b[3~", "ab"}, + // SS3 (cli#516): the SAME keys once the terminal is in DECCKM + // application-cursor mode, which vim/less/tmux leave behind on an unclean + // exit. Unlike CSI residue these used to survive as a plausible name. + {"SS3 arrows around real content", "na\x1bODme", "name"}, + {"SS3 arrows only cleans to empty (→ auto-name)", "\x1bOD\x1bOD\x1bOD\x1bOA\x1bOA\x1bOA", ""}, + {"SS3 Home/End only", "\x1bOH\x1bOF", ""}, + {"SS3 F1/F2 only", "\x1bOP\x1bOQ", ""}, + {"SS3 and CSI mixed", "a\x1bODb\x1b[Dc", "abc"}, + {"truncated SS3 (ESC O, no final)", "\x1bO", ""}, + {"a bare O is not an escape", "OPTIMUS-01", "OPTIMUS-01"}, + + // The floor: an ESC that survived the known families means an escape shape + // we don't recognise, so what is left has to prove it is real content. + // SS2 (ESC N ) stands in for "the next family" — it is not stripped + // by escSequence, so these exercise the floor and nothing else. + {"unknown escape family, nothing but residue (→ auto-name)", "\x1bNB\x1bNC", ""}, + {"unknown escape family beside real content is kept", "box\x1bNC", "boxNC"}, + {"floor counts non-Latin letters as real content", "\x1bNC日本", "NC日本"}, + // The probe's final-byte run is bounded at two, so an ASCII name after an + // unknown escape is kept just like a non-Latin one — keep-vs-reject must + // not depend on the script the name is written in (Bugbot, + // tracebloc/client#736). + {"floor keeps an ASCII name after an unknown escape", "\x1bNChello", "NChello"}, + // Bracketed paste. {"bracketed-paste wrappers", "\x1b[200~hello\x1b[201~", "hello"}, {"post-corruption literal paste markers", "[200~hello[201~", "hello"}, @@ -63,3 +87,26 @@ func TestSanitizeClientName_neutralizesSlugGarble(t *testing.T) { t.Fatalf("slug.Slugify(sanitizeClientName(raw)) = %q, want %q", got, "se") } } + +// TestSanitizeClientName_ss3SurvivesAsAPlausibleName is the cli#516 half: SS3 +// residue was worse than the CSI residue fixed in cli#364 / client#362, because +// it did NOT clean to empty. 'O' and the final byte are printable, so dropping +// only the ESC left a non-empty, plausible-looking name that passed every +// downstream check — is_dns1123_label validates by idempotence against the slug +// rule, and "odododoaoaoa" is a perfectly canonical label. Form is exactly what +// this input preserves, so nothing but this function can refuse it. +func TestSanitizeClientName_ss3SurvivesAsAPlausibleName(t *testing.T) { + const garbled = "\x1bOD\x1bOD\x1bOD\x1bOA\x1bOA\x1bOA" // ← ← ← ↑ ↑ ↑ in DECCKM mode + + // What the CSI-only sanitizer produced: ESC removed as a C0 byte, the rest + // kept verbatim — and slugifying that mints a permanent namespace. + if got := slug.Slugify("ODODODOAOAOA"); got != "odododoaoaoa" { + t.Fatalf("precondition: slug.Slugify(%q) = %q, want the documented garble %q", "ODODODOAOAOA", got, "odododoaoaoa") + } + + // With SS3 handled, the value cleans to empty, which client create treats + // exactly like an omitted --name: it auto-names instead of minting garbage. + if got := sanitizeClientName(garbled); got != "" { + t.Fatalf("sanitizeClientName(%q) = %q, want %q (the auto-name path)", garbled, got, "") + } +}