diff --git a/crates/git-remote-gitlawb/src/main.rs b/crates/git-remote-gitlawb/src/main.rs index ca46c9ce..113b362a 100644 --- a/crates/git-remote-gitlawb/src/main.rs +++ b/crates/git-remote-gitlawb/src/main.rs @@ -570,6 +570,13 @@ fn safe_error_body_excerpt(body: &str) -> String { continue; } + if gitlawb_core::sanitize::is_bidi_format(ch) { + // Drop Cf bidi/format controls (INV-6, #183) without marking + // pending_space: unlike a newline/tab they are not whitespace, so + // "a\u{202e}c" collapses to "ac" with no phantom gap. + continue; + } + if ch.is_whitespace() { pending_space = true; continue; @@ -1159,6 +1166,55 @@ mod tests { assert!(!message.contains('\n')); } + #[test] + fn http_error_message_strips_bidi_format_controls() { + // Each Cf bidi class must be stripped (INV-6, #183), not just U+202E: + // override (RLO), embedding + pop (LRE/PDF), isolate (LRI/PDI), and marks + // (LRM/ALM). Asserted per code point so a partial predicate fails loudly. + // The visible words must survive and JOIN with no phantom space at the + // strip points: a bidi char is dropped like a control byte, but unlike a + // newline/tab it is not whitespace, so it must not set pending_space. The + // leading bidi char (index 0) also exercises the empty-excerpt path. + let body = "\u{202e}err\u{202a}\u{202c}\u{2066}\u{2069}\u{200e}\u{061c}ok"; + let message = http_error_message( + "GET", + "/info/refs", + reqwest::StatusCode::BAD_GATEWAY, + body, + None, + ); + for c in [ + '\u{202e}', '\u{202a}', '\u{202c}', '\u{2066}', '\u{2069}', '\u{200e}', '\u{061c}', + ] { + assert!( + !message.contains(c), + "bidi U+{:04X} leaked: {message:?}", + c as u32 + ); + } + assert!( + message.contains("errok"), + "words should join with no phantom space: {message:?}" + ); + } + + #[test] + fn http_error_message_preserves_legitimate_and_rtl_text() { + // Must not over-strip: plain text, a genuine RTL SCRIPT letter (Arabic + // U+0627, category Lo), and ZWJ (U+200D, a legitimate Cf char) survive. + let message = http_error_message( + "GET", + "/info/refs", + reqwest::StatusCode::BAD_GATEWAY, + "ok \u{0627}\u{200D}b", + None, + ); + assert!( + message.ends_with("ok \u{0627}\u{200D}b"), + "legitimate text was altered: {message:?}" + ); + } + #[test] fn http_error_message_truncates_long_response_body() { let body = "x".repeat(MAX_ERROR_BODY_CHARS + 100); @@ -1194,6 +1250,33 @@ mod tests { assert_eq!(requests.len(), 1); } + #[test] + fn connect_error_strips_bidi_from_response_body() { + // Transport-path proof: a bidi override in the node's real HTTP error body + // must not reach the surfaced error text. Drives the full handle_connect + // error path, not just the safe_error_body_excerpt unit boundary (INV-6). + let (base_url, server) = serve_http(vec![TestResponse { + request_line: "GET /z6Mk/myrepo/info/refs?service=git-upload-pack HTTP/1.1", + status: "404 Not Found", + body: "not\u{202e}found", + }]); + + let repo_base = format!("{base_url}/z6Mk/myrepo"); + let mut stdin = std::io::Cursor::new(Vec::::new()); + let err = handle_connect(&repo_base, "git-upload-pack", None, &mut stdin).unwrap_err(); + let message = err.to_string(); + + assert!( + !message.contains('\u{202e}'), + "bidi override reached the terminal: {message:?}" + ); + assert!( + message.contains("notfound"), + "body should survive with the bidi char stripped: {message:?}" + ); + let _ = server.join(); + } + #[test] fn connect_post_error_includes_response_body() { let (base_url, server) = serve_http(vec![ diff --git a/crates/gitlawb-core/src/lib.rs b/crates/gitlawb-core/src/lib.rs index a9e91f60..efa99897 100644 --- a/crates/gitlawb-core/src/lib.rs +++ b/crates/gitlawb-core/src/lib.rs @@ -5,6 +5,7 @@ pub mod encrypt; pub mod error; pub mod http_sig; pub mod identity; +pub mod sanitize; pub mod ucan; pub use error::Error; diff --git a/crates/gitlawb-core/src/sanitize.rs b/crates/gitlawb-core/src/sanitize.rs new file mode 100644 index 00000000..2ccf9e68 --- /dev/null +++ b/crates/gitlawb-core/src/sanitize.rs @@ -0,0 +1,69 @@ +//! Text sanitization predicates shared across the workspace. +//! +//! Untrusted peer/remote bytes that reach an operator's terminal (node error +//! bodies, node-advertised strings) must be stripped of BOTH `Cc` control bytes +//! (`char::is_control()`) AND `Cf` bidi/format controls before display: a +//! right-to-left override or directional isolate can visually reorder the line +//! and spoof the text (Trojan Source, CWE-451). This is INV-6. +//! +//! `is_bidi_format` is the single workspace definition of the second half of +//! that obligation. It was split out of `gl`'s local copy so the two terminal +//! sanitizers that shipped with only the `Cc` half (#183) — and any future +//! sanitizer — reuse one audited predicate instead of re-deriving the set. + +/// True for the Unicode bidirectional/directional-isolate format characters +/// (the reordering subset of category `Cf`) that can visually reorder a terminal +/// line. These are NOT matched by [`char::is_control()`], which covers `Cc` only. +/// +/// This is deliberately the reordering subset, not the whole `Cf` category: +/// legitimate `Cf` characters such as ZWJ (`U+200D`, emoji sequences) and the +/// soft hyphen (`U+00AD`) are NOT matched, so honest international text passes +/// through unchanged. Widening this set is a behavior change for every consumer. +/// +/// Pair it with `!c.is_control()` in a terminal-bound sanitizer (INV-6); see the +/// wrappers in `gl`, `git-remote-gitlawb`, and `icaptcha-client`. +pub fn is_bidi_format(c: char) -> bool { + matches!(c, + '\u{200E}' | '\u{200F}' | '\u{061C}' // LRM, RLM, ALM + | '\u{202A}'..='\u{202E}' // LRE, RLE, PDF, LRO, RLO + | '\u{2066}'..='\u{2069}' // LRI, RLI, FSI, PDI + ) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn strips_every_reordering_code_point() { + // Asserted individually so a partial predicate (e.g. one that covers the + // marks but not the isolates) fails loudly rather than passing. + for c in [ + '\u{200E}', '\u{200F}', '\u{061C}', // LRM, RLM, ALM + '\u{202A}', '\u{202B}', '\u{202C}', '\u{202D}', + '\u{202E}', // LRE, RLE, PDF, LRO, RLO + '\u{2066}', '\u{2067}', '\u{2068}', '\u{2069}', // LRI, RLI, FSI, PDI + ] { + assert!( + is_bidi_format(c), + "U+{:04X} must be a bidi-format char", + c as u32 + ); + } + } + + #[test] + fn passes_legitimate_and_control_chars() { + // Must NOT over-strip: plain text, a genuine RTL SCRIPT letter (Lo, not a + // format char), ZWJ (legitimate Cf), and soft hyphen (legitimate Cf) are + // not bidi-format. ESC is a Cc control char — owned by is_control(), not + // this predicate, which is bidi-only. + for c in ['a', '\u{0627}', '\u{200D}', '\u{00AD}', '\u{1b}'] { + assert!( + !is_bidi_format(c), + "U+{:04X} must NOT be a bidi-format char", + c as u32 + ); + } + } +} diff --git a/crates/gl/src/sync.rs b/crates/gl/src/sync.rs index 72fb2758..634b1c0d 100644 --- a/crates/gl/src/sync.rs +++ b/crates/gl/src/sync.rs @@ -132,23 +132,11 @@ async fn read_body_capped(mut resp: reqwest::Response, cap: usize) -> String { /// `char::is_control` does not cover — they can reorder the displayed line). fn sanitize_node_msg(s: &str) -> String { s.chars() - .filter(|c| !c.is_control() && !is_bidi_format(*c)) + .filter(|c| !c.is_control() && !gitlawb_core::sanitize::is_bidi_format(*c)) .take(200) .collect() } -/// Unicode bidirectional and directional-isolate format characters (category -/// `Cf`). These are not `char::is_control()` (that is category `Cc` only), but a -/// right-to-left override or isolate can visually reorder a terminal line to -/// spoof the error text, so they are stripped alongside the control bytes. -fn is_bidi_format(c: char) -> bool { - matches!(c, - '\u{200E}' | '\u{200F}' | '\u{061C}' // LRM, RLM, ALM - | '\u{202A}'..='\u{202E}' // LRE, RLE, PDF, LRO, RLO - | '\u{2066}'..='\u{2069}' // LRI, RLI, FSI, PDI - ) -} - #[cfg(test)] mod tests { use super::*; @@ -293,6 +281,16 @@ mod tests { assert_eq!(sanitize_node_msg(&long).chars().count(), 200); } + #[test] + fn sanitize_preserves_legitimate_and_rtl_text() { + // Must not over-strip: a plain word, a genuine RTL SCRIPT letter (Arabic + // U+0627, category Lo — NOT a format char), and ZWJ (U+200D, a legitimate + // Cf char, e.g. emoji sequences) all survive. Guards the shared predicate + // against being widened into a blanket Cf stripper. + let out = sanitize_node_msg("ok \u{0627}\u{200D}b"); + assert_eq!(out, "ok \u{0627}\u{200D}b"); + } + #[tokio::test] async fn trigger_handles_oversized_error_body_without_unbounded_output() { // A hostile/broken node returns a 2 MB error body. The command must still diff --git a/crates/icaptcha-client/src/lib.rs b/crates/icaptcha-client/src/lib.rs index e32de31a..b9a811e9 100644 --- a/crates/icaptcha-client/src/lib.rs +++ b/crates/icaptcha-client/src/lib.rs @@ -64,6 +64,12 @@ fn sanitize_excerpt(s: &str) -> String { } continue; } + if gitlawb_core::sanitize::is_bidi_format(ch) { + // Drop Cf bidi/format controls (INV-6, #183) without setting + // pending_space: they are not whitespace, so stripping leaves no + // phantom gap. + continue; + } if ch.is_whitespace() { pending_space = true; continue; @@ -421,6 +427,34 @@ mod tests { assert!(clean.contains("owned") && clean.contains("done")); } + #[test] + fn sanitize_strips_bidi_format_controls() { + // The Cc-only test above is vacuous for Cf: it passes whether or not bidi + // controls survive, which is why this gap shipped (#183). Here every Cf + // bidi class is stripped — override (RLO), embedding + pop (LRE/PDF), + // isolate (LRI/PDI), marks (LRM/ALM) — asserted per code point, and the + // visible words JOIN with no phantom space (a bidi char is dropped like a + // control byte, but unlike a newline/tab it is not whitespace). The leading + // bidi char (index 0) also exercises the empty-out path. + let out = sanitize_excerpt("\u{202e}err\u{202a}\u{202c}\u{2066}\u{2069}\u{200e}\u{061c}ok"); + for c in [ + '\u{202e}', '\u{202a}', '\u{202c}', '\u{2066}', '\u{2069}', '\u{200e}', '\u{061c}', + ] { + assert!(!out.contains(c), "bidi U+{:04X} leaked: {out:?}", c as u32); + } + assert_eq!(out, "errok"); + } + + #[test] + fn sanitize_preserves_legitimate_and_rtl_text() { + // Must not over-strip: plain text, a genuine RTL SCRIPT letter (Arabic + // U+0627, category Lo), and ZWJ (U+200D, a legitimate Cf char) survive. + assert_eq!( + sanitize_excerpt("ok \u{0627}\u{200D}b"), + "ok \u{0627}\u{200D}b" + ); + } + #[test] fn sanitize_collapses_whitespace_and_caps_length() { let collapsed = sanitize_excerpt("a\n\n\t b"); @@ -429,6 +463,18 @@ mod tests { assert_eq!(sanitize_excerpt(&long).chars().count(), MAX_ERR_CHARS); } + #[test] + fn sanitize_bidi_chars_do_not_consume_the_length_cap() { + // A dropped bidi char must not count toward MAX_ERR_CHARS: it is dropped + // before `kept` increments, so interleaving bidi controls cannot truncate + // the visible output early. MAX_ERR_CHARS + 100 visible 'x', each trailed by + // a bidi override, must still yield exactly MAX_ERR_CHARS visible 'x' with + // no phantom spaces. (Would regress to fewer 'x' if bidi consumed budget, or + // to spaces if the drop branch wrongly set pending_space.) + let body = "x\u{202e}".repeat(MAX_ERR_CHARS + 100); + assert_eq!(sanitize_excerpt(&body), "x".repeat(MAX_ERR_CHARS)); + } + // ── P2: never talk to (or hand a key to) an untrusted node-chosen URL ── #[test]