fix: hardening round 2 — #45 #46 #47

[fissible]

Summary

Three M-severity items from the Phase 8 review, following the merged launch-blockers PR (#59):

• closes [Bug] M5: Bracketed-paste drain has no escape hatch and no sanitization (editor) #45 — bracketed paste is now bounded and sanitized:
  • shellframe_read_key gains an optional timeout argument setting SHELLFRAME_KEY_TIMEOUT (bash ≥4 distinguishes timeout/EOF by rc; 3.2 conservatively reports TIMEOUT)
  • new shellframe_sanitize strips ANSI escapes (CSI, OSC/DCS string forms, Fe) and C0 controls except \n/\t
  • editor drain ends on EOF or SHELLFRAME_PASTE_SILENCE_LIMIT seconds of silence (default 5) and inserts sanitized bytes
• closes [Bug] M6: SGR mouse parsing is unvalidated and duplicated (input.sh vs shell.sh) #46 — SGR mouse decoding consolidated into _shellframe_parse_sgr_mouse; both readers call it. Malformed events (wrong field count / non-numeric) are discarded; motion reports (bit 32) are flagged via SHELLFRAME_MOUSE_MOTION and dropped instead of leaking into the button value (hover/drag remains [Feature] Mouse motion support — 1002 drag / 1003 hover behind an opt-in flag (post-#51 feasible) #57 scope)
• closes [Bug] M7: Display width ≠ character count — wide/combining chars misalign every bordered surface #47 — single-column width limitation owned in README + docs/api.md (wcwidth approximation stays proposed under [Feature] Wide-character strategy — own the limitation in docs, or ship a wcwidth table #54)

Also fixes the local Docker matrix blocker: run-matrix mounts a sibling ptyunit checkout under /Users instead of the unshared /opt/homebrew path.

Test plan

• New unit files: sanitizer table (10 cases), SGR parser table (9 cases incl. malformed/motion/wheel/modifier bits)
• Extended: editor PTY tests (lost-terminator recovery + ANSI-stripped paste), read_key timeout flag tests
• Host suite 1555/1555 across 51 files; Docker matrix 3/3 PASS (bash 3.2 / 4.4 / 5.x)
• Mouse-routing PTY integration passes unchanged against the consolidated parser

[fissible]

Review round 2 response — blocking fix pushed (9c67716)

Blocking (3.2 UTF-8 collation) — confirmed, reproduced, fixed.
Reproduced exactly as reported: sanitizer table 8/10 under LANG=en_US.UTF-8 on /bin/bash 3.2, 10/10 under C. Root cause accepted: [@-~] follows collation order in 3.2 bracket expressions, so letters fell outside the CSI final-byte class and sequences never terminated.

Fix: LC_COLLATE=C pinned inside shellframe_sanitize (byte-ordered ranges; multibyte content passes through byte-wise untouched). The sanitizer unit file now exports en_US.UTF-8 when the host provides it, so UTF-8 hosts exercise this path on every run instead of only CI's blind C-locale runners. Verified 11/11 (added a multibyte-survival case) under C and en_US.UTF-8 on both bash 5.3 and /bin/bash 3.2.

Should-fix (sanitizer cost) — fixed. Two O(n) pattern scans now short-circuit clean input (no ESC, no control chars beyond \n\t) before the per-character loop: a 24 KB clean paste drops from seconds to ~3 ms. Dirty pastes keep full scrubbing.

Cleared items acknowledged with thanks — the \r→\n confirmation, SGR table under both locales on 3.2, and the cross-check against ptyunit 1.6.0.

Noted: screen.sh:226 BFD noise confirmed pre-existing on origin/main — filed separately as cleanup, not part of this PR.

Suite: 1556/1556 across 51 files (host); Docker matrix re-run recommended on merge given locale sensitivity is now test-enforced rather than environment-dependent.

[fissible]

Review — verdict: changes requested (posted as a comment: GitHub blocks request-changes from the author's own account)

Reviewed against head ebe5df2. The shape is right — one SGR decoder, a bounded drain, a real sanitizer, honest width docs — and the full suite passes against ptyunit 1.6.0 (1551/1551 here). But three findings are user-visible on the primary target, and one is a regression relative to main. Everything below was reproduced, not inferred.

Blocking

1. shellframe_sanitize drops everything after the first escape on bash 3.2 in a UTF-8 locale — src/clip.sh:217 (and the C0 class at :197)

Bracket ranges are locale-collated on bash 3.2: [[ m == [@-~] ]] is false on /bin/bash 3.2.57 under en_US.UTF-8 (true under LC_ALL=C, and on bash 5 which has globasciiranges). So a CSI never terminates and the state machine swallows the rest of the buffer:

$ LANG=en_US.UTF-8 /bin/bash -c 'source shellframe.sh; shellframe_sanitize $'"'"'hello\tX\033[31mred\033[0m\x01Y'"'"' o; echo "[$o]"'
[hello	X]                      # bash 5, or LC_ALL=C:  [hello	XredY]

A stock-macOS user pasting any colored terminal output into the editor loses all text after the first escape. The PR's own test catches it — tests/unit/test-sanitize.sh fails 2/10 on /bin/bash under LANG=en_US.UTF-8 — but CI's macOS runner has no locale set, so the leg is green. Fix verified on 3.2: local LC_COLLATE=C as the first line of the function. Then make CI honest: run the macOS leg with LANG=en_US.UTF-8 (what real Macs have), or have test-sanitize.sh set it explicitly.

2. Motion / malformed SGR events are not discarded — they reach widgets as raw 10-byte keys — src/input.sh:278, src/shell.sh:411

When _shellframe_parse_sgr_mouse returns 1, both readers fall through and return the raw ESC[<32;5;5M bytes as the key value (verified on 3.2 and 5.3: shellframe_read_key k < <(printf '\033[<32;5;5Mq') → k = the raw sequence). Pre-PR the ~28 mask at least returned SHELLFRAME_KEY_MOUSE; now an app with ?1002h gets junk keys on every drag step, and alert.sh:148 (shellframe_read_key _key then unconditional teardown) dismisses an alert on any motion or malformed report. This contradicts the comment at input.sh:271 and the header at :128. Fix: on parser rc=1 for an ESC[< prefix, continue (re-read) or return an empty key.

3. A NUL byte is misclassified as EOF/TIMEOUT and the whole paste is discarded — src/input.sh:228

read -d '' consumes NUL as its delimiter and returns 0 with an empty variable; the timed path treats rc ≤ 128 with empty _k as EOF on bash ≥ 4 (0 <= 128) and as TIMEOUT on 3.2. Verified: shellframe_read_key k 2 < <(printf '\0x') → eof=1 (5.3) / timeout=1 (3.2), while the untimed path gives neither. In editor.sh:1089 the EOF branch returns without inserting _paste_buf, so a NUL inside pasted bytes (or Ctrl-@ while waiting on a lost terminator) silently drops everything gathered. shell.sh:374 already uses _rc > 0 && _rc <= 128 — match it.

Should fix in this PR

4. nF sequences and doubled ESC leak — src/clip.sh:214. State 1 treats any byte after ESC other than [ ] P X ^ _ as a complete 2-byte Fe. tput sgr0 on macOS emits ESC ( B, so pasting colored ls/git output inserts a stray B before every reset ($'\033(B\033[m x' → B x); $'a\033\033[31mb' → a[31mb; $'a\033\nb' → ab (newline eaten). Treat 0x20–0x2F as nF intermediates and consume to a 0x30–0x7E final; re-enter state 1 on a second ESC; drop only the ESC when the next byte isn't a valid introducer. (ptyunit's strip_ansi handles the nF arm — same shape.)

5. Fractional timeout on bash 3.2 — src/input.sh:221. SHELLFRAME_PASTE_SILENCE_LIMIT=0.5 → read: 0.5: invalid timeout specification, instant non-zero return reported as TIMEOUT, so the drain ends after zero reads and the pasted body is processed as raw keystrokes (Ctrl-K/U/W bytes executed as editor commands). shell.sh:384 already version-guards its 0.05; do the same here (clamp to integer ≥ 1 on BASH_VERSINFO[0] < 4) and say integer-only in the docblock and docs/api.md:147.

6. EOF branch contradicts its comment and differs by bash version — src/widgets/editor.sh:1084-1090. The comment says "either way the bytes gathered so far are treated as the paste"; the EOF branch drops them. Because the timed read reports TIMEOUT-on-EOF on 3.2 but EOF on ≥ 4, printf '\033[200~abc' | app inserts abc on 3.2 and discards it on 5. Insert _paste_buf on EOF too (making the comment true), and consider lifting shell.sh:364-377's elapsed-time discriminator into shellframe_read_key — the "3.2 cannot distinguish EOF" claim in docs/api.md:149 is no longer accurate now that shell.sh does exactly that.

7. Sanitizer is O(n²) under UTF-8 and freezes the TUI on large pastes — src/clip.sh:201. ${_raw:$_i:1} walks from the start of the string for every character in a multibyte locale. Measured, one call: 3.2 → 10 KB 1.4 s, 50 KB 33 s; 5.3 → 50 KB 8.2 s (0.03 s under LC_ALL=C). The drain's _paste_buf="${_paste_buf}${_paste_key}" (editor.sh:1094) adds ~20 s for 50 K single-char appends on 3.2 vs 0.24 s with _paste_buf+=. Cheap wins, in order: fast-path return when [[ $_raw != *[ESC/C0 class]* ]] (0.04 s for 50 KB on 3.2); += in the drain; hoist the $(printf …) C0-class fork (:197) to a source-time printf -v constant; then block-wise scanning if still needed.

Tests

8. The "pasted ANSI escapes are stripped" test is vacuous — tests/integration/test-editor.sh:63. pty_run strips ANSI from captured output by default, so assert_contains "$out" "ab" passes even when the editor inserts the raw ESC[31m (verified with the sanitizer overridden to identity: still passes; status line shows col 8 not col 3). It therefore cannot catch finding 1. Assert on col 3, or run with PTY_RAW=1 and assert ESC is absent.

9. The lost-terminator test costs 27 s of a 31 s file — tests/integration/test-editor.sh:47. It never lets the child exit, so pty_run waits the full PTY_TIMEOUT=20 then kills it (rc 124) — paid on every local run, CI run, and all three Docker legs. The comment "a submit keystroke cannot follow" is wrong: with PTY_DELAY=1.5 SHELLFRAME_PASTE_SILENCE_LIMIT=1 and the text as one token ($'\x1b[200~pasted' — separate p a s t e d tokens leave _paste_buf empty and pass vacuously), Ctrl-D submits normally and the test passes in 1.2 s with a stronger assertion (col 7 and pasted on stdout).

Hygiene

10. SHELLFRAME_PASTE_SILENCE_LIMIT breaks the SHELLFRAME_<WIDGET>_* naming rule in CLAUDE.md:128 (siblings are SHELLFRAME_EDITOR_*) — rename before it's public. docs/api.md has no entry for shellframe_sanitize, the silence limit, or SHELLFRAME_MOUSE_MOTION (write-only: no consumer anywhere). grep -rn KEY_TIMEOUT tests/ is empty — nothing exercises shellframe_read_key key N or its EOF branch. Smaller: SHELLFRAME_MOUSE_ACTION is set before field validation, so a rejected ESC[<0M leaves ACTION=press with stale coords; duplicated # Usage: blocks in the read_key header; README/api say "code points" while clip.sh:45 still says "bytes".

Cleared / for the record

• Multi-line paste keeps its line breaks on 3.2 and 5, C and UTF-8 (bash converts \r→\n on tty reads even with -icrnl; the gotcha comment is right).
• SGR parser table passes on 3.2 under both locales; the _num_re variable trick for ; is correct.
• screen.sh:226: 3: Bad file descriptor on editor exit is pre-existing on main (identical there) — not this PR, but worth its own small ticket.
• Docker run-matrix mount fix: good catch.

Reproductions run on macOS /bin/bash 3.2.57 and bash 5.3.15, in both en_US.UTF-8 and C.

🤖 Generated with Claude Code

[fissible]

Review round 2 response — all three blockers fixed (c121b49)

Blocker 1 — the LC_COLLATE=C pin landed in 9c67716 while your review was in flight against ebe5df2; we raced. Your additional ask is done: CI now runs under en_US.UTF-8 (test-command env prefix in ci.yml), so the macOS leg matches real Macs, and the sanitizer unit file self-promotes to UTF-8 collation on any host that offers it.

Blocker 2 — confirmed worse than reported, then properly fixed. Your repro was right, but my first attempt over-corrected: swallowing every parser failure ate unrecognized CSI sequences too — caught immediately by the existing drain tests (ESC[999~ returned empty). Final shape: only ESC[<-prefixed sequences enter the decoder; decode failure → consumed as an empty key; everything else returns raw exactly as before. Unit cases added for motion/malformed → len=0.

Blocker 3 — confirmed, plus a subtlety you didn't have to find: rc == 0 with an empty value means the NUL delimiter matched — a literal NUL keystroke, distinct from both EOF (rc≠0 ≤128) and timeout (>128). read_key now returns an empty key with no flags for it on all versions, so a NUL inside a paste no longer kills the drain. Editor EOF exit also unified with the silence exit: buffered bytes are inserted either way (the old EOF branch did contradict its own comment).

Should-fixes, all done:

• nF intermediates: sanitizer gained an intermediate-consuming state — tput sgr0's ESC ( B no longer leaks its final byte
• fractional silence limits floor to ≥1 s inside read_key on 3.2 (centralized, so every caller benefits)
• accumulation chunked through an array — dirty 50 KB paste was O(n²) string-append on 3.2
• env renamed SHELLFRAME_EDITOR_PASTE_SILENCE_LIMIT

Hygiene: the ANSI test now captures PTY_RAW=1 and asserts the injected SGR never appears in the submitted text (you were right — the old assertion passed with sanitization disabled); lost-terminator budget trimmed 27s → ~5s; KEY_TIMEOUT/NUL/motion unit cases added; api.md documents shellframe_sanitize, SHELLFRAME_MOUSE_MOTION, and the paste limit.

Verification: host suite 1563/1563 across 51 files; Docker matrix 3/3 PASS; mouse-routing PTY integration unchanged.

[fissible]

Round-3 review of 429d954 — verdict: approve after one small perf patch (details below; everything else is fixed)

Re-verified against the new head, not the summary.

Blockers — all three fixed, confirmed by the original repros

	round 1	now
Sanitizer, /bin/bash 3.2, en_US.UTF-8	[hello X] (rest swallowed)	[hello XredYZ] — and ESC ( B (tput sgr0) no longer leaks a B
ESC[<32;5;5M motion through shellframe_read_key	raw 10-byte key	key '', SHELLFRAME_MOUSE_MOTION=1 (3.2 and 5)
NUL under timed read	eof=1 (5) / timeout=1 (3.2)	eof=0 timeout=0 on both

test-sanitize.sh 11/11 and test-input.sh 66/66 on /bin/bash 3.2 under en_US.UTF-8; CI now runs the macOS leg in UTF-8 — that's the right fix for "green only because the runner has no locale". Fractional limit floored on 3.2 (no error, times out after 1 s). test-editor.sh 17 s (was 31 s). Full suite 1563/1563 against ptyunit 1.6.1. The stray file was the old untracked autocomplete plan — nothing sensitive, correctly reverted.

⚠ One thing to fix before merge: the fast path is quadratic on bash 3.2 (src/clip.sh:206-208)

local LC_COLLATE=C fixed collation but not string cost, and the new probe strips \n/\t with ${_raw//…/} — on bash 3.2 that copies the string per match, so it's O(matches × n). Measured on /bin/bash 3.2, LANG=en_US.UTF-8, clean paste (no escapes at all):

size	this head	with the patch below
2 KB	0 s	0 s
10 KB	5 s	0 s
20 KB	>40 s (killed)	0 s
50 KB	—	0 s
200 KB	—	0 s
dirty 48 KB	>40 s	7 s

bash 5 is fine either way (50 KB clean in 1 s). But before this PR, paste on stock macOS bash was instant (unsanitized); after it, pasting a 10 KB log file freezes the editor for 5 s and a 20 KB one effectively hangs — on the primary target. Two-line fix, verified with a UTF-8 payload (dé survives byte-wise processing, since ESC/C0 are single bytes that never occur inside a UTF-8 sequence):

-    local LC_COLLATE=C
+    local LC_ALL=C      # byte semantics: O(1) ${s:i:1} on 3.2, and C collation for ranges
 
-    local _probe="${_raw//$'\n'/}"
-    _probe="${_probe//$'\t'/}"
-    if [[ "$_probe" != *$'\x1b'* && "$_probe" != *[[:cntrl:]]* ]]; then
+    # Single glob scans are O(n) even on bash 3.2; ${var//x/} is O(matches*n) there.
+    # Class = C0 minus \t \n, plus DEL.
+    local _c0x; _c0x=$(printf '\001-\010\013-\037\177')
+    if [[ "$_raw" != *$'\x1b'* && "$_raw" != *[$_c0x]* ]]; then

test-sanitize.sh stays green with it. Worth adding one perf guard test: sanitize a 50 KB clean string and assert it completes (a read -t/watchdog-style bound), so this can't regress silently.

Leftover nits — defer freely

• Doubled ESC still leaks the CSI body: $'a\033\033[1mW' → a[1mW (bash 5 and 3.2). In state 1, add $'\x1b') _state=1 ;; before the *) Fe arm.
• SHELLFRAME_MOUSE_ACTION is set before field validation (input.sh:155), so a rejected ESC[<0M leaves ACTION=press. Move the assignment after the regex check.
• State 5's malformed-resync *) _state=0 drops the offending byte rather than re-dispatching it.

Reproductions on macOS /bin/bash 3.2.57 and bash 5.3.15, en_US.UTF-8 and C.

🤖 Generated with Claude Code

[fissible]

Round 3 — perf patch applied (e8a5725)

Your exact diff is in: LC_ALL=C (byte semantics — this also subsumes the earlier LC_COLLATE pin) and the single glob probe against a printf-built [\001-\010\013-\037\177] class. Verified on /bin/bash 3.2: clean 50 KB sanitize completes instantly under both C and en_US.UTF-8.

Perf guard added as you suggested: test-sanitize.sh bounds a clean 50 KB sanitize at 10 s on every matrix leg, with a content-length assertion so a silently-truncating fast path also fails. Generous enough for slow CI containers; a regression of the quadratic class trips it by an order of magnitude.

Both one-line nits applied too (they were free): doubled ESC re-enters state 1 in the sanitizer; MOUSE_ACTION is set only after field validation in the parser. The state-5 malformed-resync byte-drop is left as-is with your note — resync semantics deserve their own thought rather than a drive-by.

Verification: sanitizer 13/13 on bash 5.3 + /bin/bash 3.2 × C/UTF-8; host suite 1565/1565 across 51 files; Docker matrix 3/3 PASS.

Nothing outstanding from my side — ready to merge on your approval.

[fissible]

Round-4 verdict: approved — merge-ready (e8a5725)

Re-verified against the head, not the summary:

• Perf patch: /bin/bash 3.2, en_US.UTF-8 — clean 200 KB sanitize in 0 s (was >40 s at 20 KB); dirty 48 KB in 7 s (was >40 s); UTF-8 payload (dé) intact through byte-wise processing.
• Perf guard: real test — 50 KB clean, 10 s bound, plus a length assertion so a truncating fast path fails too.
• Nits: doubled ESC → aW (no [1m leak); malformed ESC[<0M leaves SHELLFRAME_MOUSE_ACTION empty.
• Tests: test-sanitize.sh 13/13, test-input.sh 66/66 on /bin/bash 3.2 under UTF-8; full suite 1565/1565 assertions passed across 51 file(s) against ptyunit 1.6.1; both CI legs green.

Deferred per my note: state-5 resync byte drop. Nothing outstanding. After merge this is a patch release (v0.5.3 — all fixes).

🤖 Generated with Claude Code