Uh oh!
There was an error while loading. Please reload this page.
fix(client create): strip control chars from --name before slugifying - #364
Conversation
Defense-in-depth for the name-garble bug (customer-reported 2026-07-20), the CLI-side companion to the installer fix in client#362. Typing arrow keys at the installer's name prompt injected raw ESC[D/ESC[A bytes into the name. slug.Slugify keeps ESC (0x1B < 128) through its ASCII pass and then collapses each ESC[<x> run to a "-", minting the garbage namespace the customer saw: "se-\e[D\e[D\e[A\e[A" -> "se-d-d-a-a". The installer now strips at the source, but a --name / $TRACEBLOC_CLIENT_NAME can also arrive here directly, so harden the CLI boundary too: - New sanitizeClientName() removes ANSI CSI sequences (arrow keys, cursor moves, bracketed-paste wrappers), the literal [200~/[201~ paste markers, and any remaining C0/DEL control chars; UTF-8 (>= 0x80) is preserved. Mirrors the installer's _strip_paste_garbage. - runClientCreate cleans both name and location right after resolving them, before the auto-name check and before slug.Derive. A name that is ONLY control chars cleans to "" and falls through to auto-naming — the same graceful path as an omitted --name. Both cleanups log the before/after to the install trace; a non-empty change also prints a one-line notice. Deliberately NOT done in slug.Slugify: that package must stay a faithful mirror of the backend's slug.py (the backend validates exactly what it produces). Input hygiene belongs at ingestion, not in the shared slug rule. Tests: sanitize_test.go covers arrow keys, Ctrl+arrow (CSI params), Delete, paste wrappers, post-corruption literal markers, bare C0/DEL, lone ESC, and UTF-8/legitimate-bracket preservation — plus an end-to-end case asserting the raw input slugifies to the documented "se-d-d-a-a" garble while the sanitized input slugifies clean to "se". Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
LukasWodka
commented
Jul 21, 2026
@BugBot run |
LukasWodka
commented
Jul 21, 2026
👋 Heads-up — Code review queue is at 31 / 30 Above the WIP limit. The team convention is to review existing PRs before opening new work. Open PRs currently in Code review (oldest first):
Pull from review before opening new work. (This is a nudge from the kanban WIP check, not a block.) |
Uh oh!
There was an error while loading. Please reload this page.
Bugbot "stale opts after empty sanitize" (Medium) on #364. sanitizeClientName updated only the local `name`/`location`; opts kept the raw escapes until the write-back further down. When a control-char-only --name cleans to "" and ListClients then fails, runClientCreate returns from the auto-name block BEFORE that write-back — so the failure defer's resumeCommand(opts) reprinted the raw ESC bytes into the terminal and offered a resume line that reintroduced the garbled name. Reflect the cleaned values into opts immediately after sanitizing; the later write-back still runs to capture the auto-generated name. Adds TestClientCreate_GarbledNameNotReprintedOnFailure, which drives exactly that path (arrows-only --name + a 500 on the client list) and asserts the failure output carries no ESC bytes and no --name in the resume line. Verified it fails without the fix (the resume line printed `--name \e[D\e[D\e[A`). Comments at the call site trimmed (the full rationale lives on sanitizeClientName) to keep client.go within its file-budget ceiling. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
LukasWodka
commented
Jul 21, 2026
@BugBot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 1e03eb5. Configure here.
aptracebloc
left a comment
There was a problem hiding this comment.
Verified — approving. Solid defense-in-depth input hardening:
- Sanitizer order is correct: whole CSI sequences (incl. their
ESC) stripped first, then the literal[200~/[201~paste markers, then bare C0/DEL last — strippingESCfirst would have broken the CSI match and left[D. UTF-8 is preserved (strings.Mapoperates on runes, so any rune ≥ 0x80 survives). The regex covers the real threat: arrow keys,Ctrl+arrowparams,Delete(ESC[3~), and paste wrappers. A control-only name cleans to""→ graceful auto-naming, same as an omitted--name. - Failure-path echo closed: writing the cleaned values back into
optsimmediately means an early return'sresumeCommand(opts)hint can't reprint the raw escapes (the resolved "stale opts" case). Nicely subtle. - Right layer: sanitizing at the CLI boundary rather than in
slug.Slugifykeepssluga faithful mirror of the backend'sslug.py. - Strong tests: 15 sanitizer cases plus the e2e assertion that pins both the garble repro (
se-d-d-a-a) and the clean result (se).
Two non-blocking micro-gaps (outside the threat model — no change needed):
- An incomplete CSI (
ESC[with no final byte) isn't matched, so after theESCis stripped a stray[remains. - A private-mode CSI (
ESC[?25h— the?= 0x3F isn't in the[0-9;]param class) likewise falls through, leaving[?25hafter the ESC-strip.
Neither arises from real terminal arrow-key/paste input, both leave only printable chars slug handles (not garbage namespaces), and this is defense-in-depth behind the installer fix (client#362) — so nothing to change here. Flagging only for the record: if you ever want to be exhaustive, broadening the param class (e.g. [0-9;?<>=]*) and/or dropping a trailing lone ESC[ would close them.
LGTM.
— drafted with Claude (Opus 4.8), sent by @aptracebloc
saadqbal
commented
Jul 21, 2026
/fr-pass |
Summary
CLI-side defense-in-depth for the name-garble bug (customer-reported 2026-07-20) — the companion to the installer fix in tracebloc/client#362.
Typing arrow keys at the installer's name prompt injected raw
ESC[D/ESC[Abytes into the client name.slug.SlugifykeepsESC(0x1B< 128) through its ASCII pass and then collapses eachESC[<x>run into a-, minting the exact garbage namespace the customer saw:The installer now strips at the source (client#362), but a
--name/$TRACEBLOC_CLIENT_NAMEcan also reachclient createdirectly, so this hardens the CLI boundary too.Changes
internal/cli/sanitize.go— newsanitizeClientName(): strips ANSI CSI sequences (arrow keys, cursor moves, bracketed-paste wrappers), the literal[200~/[201~paste markers, and any remaining C0/DEL control chars. UTF-8 (≥0x80) preserved. Mirrors the installer's_strip_paste_garbage.internal/cli/client.go—runClientCreatecleans bothnameandlocationright after resolving them, before the auto-name check and beforeslug.Derive. A name that is only control chars cleans to""and falls through to auto-naming (the same graceful path as an omitted--name). Both cleanups log before/after to the install trace; a non-empty change also prints a one-line notice.Why not fix it in
slug.Slugify?slugmust stay a faithful mirror of the backend'sslug.py— the backend validates exactly what it produces (RFC-0001). Stripping there would diverge the two. Input hygiene belongs at the CLI ingestion boundary, not in the shared slug rule.Test plan
internal/cli/sanitize_test.go— 15 cases: arrow keys, Ctrl+arrow (CSI params), Delete (ESC[3~), paste wrappers, post-corruption literal markers, bare C0/DEL, loneESC, and UTF-8 / legitimate-bracket preservation.se-d-d-a-agarble and the sanitized input slugifies clean tose— so a futureslugchange that alters either is caught here.go build ./...,go vet, gofmt -s, goimports -local, staticcheck (all,-ST1005), deadcode, and file-budget all clean.🤖 Generated with Claude Code
Note
Low Risk
Localized input sanitization on client create flags/env with broad test coverage; no auth, backend contract, or slug parity changes.
Overview
Adds CLI-side defense-in-depth so arrow keys and other terminal junk in
--name/$TRACEBLOC_CLIENT_NAME(and--location) no longer produce garbage Kubernetes namespaces likese-d-d-a-awhenslug.Slugifyruns.Introduces
sanitizeClientNameto strip ANSI CSI sequences, bracketed-paste markers, and C0/DEL control bytes while keeping UTF-8.runClientCreateapplies it to name and location before auto-naming and slug derivation; control-only names become empty and follow the same auto-name path as a missing--name. Cleaned values are written back intooptsimmediately so failure-pathresumeCommanddoes not echo raw escapes.Tests cover sanitization cases, slug end-to-end, and a create failure where garbled input must not reappear in output or the resume line.
Reviewed by Cursor Bugbot for commit 1e03eb5. Bugbot is set up for automated code reviews on this repo. Configure here.