fix: strip Cf bidi/format controls in terminal-bound error sanitizers (#183)

[beardthelion]

Two terminal-bound error sanitizers filtered only Cc control bytes and let Cf bidi/format controls through to the operator's terminal, so a right-to-left override in an attacker-influenceable error line could visually reorder it (Trojan Source, CWE-451). gl already stripped these; git-remote-gitlawb::safe_error_body_excerpt and icaptcha-client::sanitize_excerpt did not.

Change

• Centralize the bidi predicate as gitlawb_core::sanitize::is_bidi_format: the 12 reordering Cf code points (LRM/RLM/ALM, the LRE..RLO embedding/override range, the LRI..PDI isolate range). All three client crates already depend on gitlawb-core, so this adds no dependency edges and leaves one audited definition instead of three copies that can drift.
• Migrate gl's local copy to the shared predicate.
• Add a drop-and-continue branch in the two affected sanitizers, dropping bidi chars without setting pending_space (they are not whitespace, so stripping leaves no phantom gap).

This deliberately diverges from the issue's "port into each crate" wording. Centralizing removes the exact drift that produced the gap: two divergent copies, one incomplete.

Scope

Strips only the bidi-reordering subset, not all of Cf. Legitimate characters like ZWJ (U+200D) and soft hyphen pass through unchanged; homoglyph and other Trojan-Source vectors are out of scope.

Not addressed here (separate class, follow-up): several gl commands print node-advertised strings (PR/issue titles, agent model, registration errors) straight to the terminal with no sanitizer at all. Same vector, different shape, tracked separately.

Tests

Per-class strip coverage (each Cf class asserted individually), a must-not-over-strip guard, a transport-path proof through handle_connect, and a cap-boundary test. Every strip and preserve assertion was verified load-bearing by mutation: predicate returning false, predicate over-broad, and each of the 12 arms dropped individually all go red; the real predicate is green.

Closes #183.

Summary by CodeRabbit

• Bug Fixes
  • Improved error-message rendering by removing hidden Unicode formatting characters that could reorder or obscure displayed text.
  • Prevented invisible characters from introducing unexpected spaces or consuming available error-message length.
  • Preserved legitimate right-to-left text and other meaningful Unicode characters.
  • Applied the improved sanitization consistently across connection, synchronization, and challenge-related error messages.

[coderabbitai]

📝 Walkthrough

Walkthrough

The change centralizes bidi-format detection in gitlawb-core and applies it to terminal-bound errors in the node sync, iCaptcha, and remote Git clients. Tests cover removal of bidi controls, preservation of legitimate text, spacing behavior, and visible-length limits.

Changes

Bidi sanitization

Layer / File(s)	Summary
Shared bidi-format predicate crates/gitlawb-core/src/lib.rs, crates/gitlawb-core/src/sanitize.rs	Exports and tests is_bidi_format, matching bidi reordering controls while excluding characters such as RTL letters and ZWJ.
Terminal sanitizer adoption crates/gl/src/sync.rs, crates/icaptcha-client/src/lib.rs	Uses the shared predicate when sanitizing node and iCaptcha messages, with coverage for preservation, spacing, and length-cap behavior.
Remote error rendering crates/git-remote-gitlawb/src/main.rs	Removes bidi controls from HTTP error excerpts and verifies sanitized output through unit and connect-path tests.

Estimated code review effort: 3 (Moderate) | ~20 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly describes the main change: stripping bidi/format controls from terminal-bound error sanitizers.
Description check	✅ Passed	The description covers the motivation, concrete changes, scope, tests, and linked issue, though it omits the template's exact headings and checklist.
Linked Issues check	✅ Passed	The PR implements #183 by stripping bidi/format Cf controls in both affected sanitizers and adding coverage, while preserving Cc handling and allowed characters.
Out of Scope Changes check	✅ Passed	No unrelated functionality stands out; the gl predicate migration and shared module export support the stated sanitization fix.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/issue-183-bidi-sanitizer

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

crates/icaptcha-client/src/lib.rs (1)

67-72: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift

Consider hoisting the whole sanitizer loop into gitlawb-core, not just the predicate. sanitize_excerpt and safe_error_body_excerpt are byte-for-byte identical terminal sanitizers (trim → strip Cc → strip bidi → collapse whitespace → cap), differing only in the char-cap constant. This PR adds the same bidi-drop block to both, so the two security-sensitive sanitizers must now be kept in sync by hand — the divergence risk that centralizing the predicate was meant to eliminate. A shared gitlawb_core::sanitize::sanitize_terminal_excerpt(s, max_chars) would collapse both.

• crates/icaptcha-client/src/lib.rs#L67-L72: replace sanitize_excerpt's hand-rolled loop with a call to the shared helper, passing MAX_ERR_CHARS.
• crates/git-remote-gitlawb/src/main.rs#L573-L578: replace safe_error_body_excerpt's hand-rolled loop with the same shared helper, passing MAX_ERROR_BODY_CHARS.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/icaptcha-client/src/lib.rs` around lines 67 - 72, Centralize the
duplicated terminal sanitization loop in a shared
gitlawb_core::sanitize::sanitize_terminal_excerpt(s, max_chars) helper,
preserving the existing trim, control/bidi stripping, whitespace collapsing, and
character-cap behavior. In crates/icaptcha-client/src/lib.rs at lines 67-72,
replace sanitize_excerpt’s hand-rolled loop with the helper using MAX_ERR_CHARS;
in crates/git-remote-gitlawb/src/main.rs at lines 573-578, replace
safe_error_body_excerpt’s loop using MAX_ERROR_BODY_CHARS.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@crates/icaptcha-client/src/lib.rs`:
- Around line 67-72: Centralize the duplicated terminal sanitization loop in a
shared gitlawb_core::sanitize::sanitize_terminal_excerpt(s, max_chars) helper,
preserving the existing trim, control/bidi stripping, whitespace collapsing, and
character-cap behavior. In crates/icaptcha-client/src/lib.rs at lines 67-72,
replace sanitize_excerpt’s hand-rolled loop with the helper using MAX_ERR_CHARS;
in crates/git-remote-gitlawb/src/main.rs at lines 573-578, replace
safe_error_body_excerpt’s loop using MAX_ERROR_BODY_CHARS.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 2dffc881-3ae8-46f3-b63e-d96da6221710

📥 Commits

Reviewing files that changed from the base of the PR and between ad7c2b2 and 78c7038.

📒 Files selected for processing (5)

• crates/git-remote-gitlawb/src/main.rs
• crates/gitlawb-core/src/lib.rs
• crates/gitlawb-core/src/sanitize.rs
• crates/gl/src/sync.rs
• crates/icaptcha-client/src/lib.rs