T37: review round 1: OSC 99 reads base64 group by group, so chunks split before encoding or after join, and the text, any UTF-8, is checked once whole; plain text alone refuses controls; a chunk that breaks a rule drops what its notification gathered; the tap keeps an OSC 99 body to its metadata and a full payload

src/term/notify.rs

@@ -151,18 +151,78 @@ pub fn alive(id: &str) -> String {
     format!("\x1b]99;i={id}:p=alive;\x1b\\")
 }
 
-/// An OSC 99 payload's most bytes: plain, and base64.
+/// An OSC 99 payload's most bytes: plain, and base64; and its metadata's.
 const PLAIN: usize = 2048;
 const ENCODED: usize = 4096;
+const METADATA: usize = 1024;
+
+/// The longest OSC 99 body the tap's scanner keeps: `99;`, metadata, `;` and a payload.
+pub(super) const BODY: usize = 3 + METADATA + 1 + ENCODED;
 
 /// The most text one notification gathers from its chunks, and how many gather at once.
 const GATHERED: usize = 16384;
 const PENDING: usize = 32;
 
+/// A payload gathering its chunks: its bytes so far, and base64 short of a whole group.
+#[derive(Default)]
+struct Text {
+    bytes: Vec<u8>,
+    rest: Vec<u8>,
+}
+
+impl Text {
+    /// Adds a chunk: plain text as it is, base64 by whole groups, so a program may chunk before encoding or after.
+    fn add(&mut self, payload: &[u8], encoded: bool) -> Result<(), String> {
+        // Plain text is UTF-8 without a control character; base64 holds any UTF-8, checked once whole.
+        if !encoded {
+            let text = std::str::from_utf8(payload).map_err(|_| "payload is not UTF-8")?;
+            if text.chars().any(char::is_control) {
+                return Err("payload holds a control character".into());
+            }
+            self.finish()?;
+            self.bytes.extend_from_slice(payload);
+            return Ok(());
+        }
+        self.rest.extend_from_slice(payload);
+        let whole = self.rest.len() / 4 * 4;
+        for group in self.rest[..whole].chunks(4) {
+            self.bytes.extend(decode(group)?);
+        }
+        self.rest.drain(..whole);
+        Ok(())
+    }
+
+    /// Decodes the base64 left, padded or not, as the last chunk may leave it.
+    fn finish(&mut self) -> Result<(), String> {
+        if !self.rest.is_empty() {
+            self.bytes.extend(decode(&self.rest)?);
+            self.rest.clear();
+        }
+        Ok(())
+    }
+
+    fn len(&self) -> usize {
+        self.bytes.len() + self.rest.len()
+    }
+}
+
+fn decode(base64: &[u8]) -> Result<Vec<u8>, String> {
+    BASE64
+        .decode(base64)
+        .map_err(|error| format!("payload is no base64: {error}"))
+}
+
+/// A notification gathering its chunks.
+struct Gathering {
+    notice: Notice,
+    title: Text,
+    body: Text,
+}
+
 /// OSC 99 notifications gathered from their chunks, by id.
 #[derive(Default)]
 pub(crate) struct Kitty {
-    pending: Vec<(String, Notice)>,
+    pending: Vec<(String, Gathering)>,
 }
 
 impl Kitty {
@@ -180,6 +240,9 @@ impl Kitty {
             .position(|&byte| byte == b';')
             .ok_or("no payload")?;
         let (metadata, payload) = (&body[..at], &body[at + 1..]);
+        if metadata.len() > METADATA {
+            return Err(format!("metadata over {METADATA} bytes"));
+        }
         let mut options: Vec<(u8, &[u8])> = Vec::new();
         for pair in metadata
             .split(|&byte| byte == b':')
@@ -218,17 +281,6 @@ impl Kitty {
         if payload.len() > limit {
             return Err(format!("a payload over {limit} bytes"));
         }
-        let text = if encoded {
-            let bytes = BASE64
-                .decode(payload)
-                .map_err(|error| format!("payload is no base64: {error}"))?;
-            String::from_utf8(bytes).map_err(|_| "payload is not UTF-8")?
-        } else {
-            String::from_utf8(payload.to_vec()).map_err(|_| "payload is not UTF-8")?
-        };
-        if text.chars().any(char::is_control) {
-            return Err("payload holds a control character".into());
-        }
         // An answer names the asker's id, or 0 without one, as kitty's spec says.
         let named = || id.clone().unwrap_or_else(|| "0".into());
         match get(b'p').unwrap_or(b"title") {
@@ -251,26 +303,38 @@ impl Kitty {
                 return Err(format!("{PENDING} notifications gathering already"));
             }
             None => {
+                let notice = Notice {
+                    id: id.clone(),
+                    ..plain(String::new(), String::new())
+                };
+                let (title, body) = (Text::default(), Text::default());
                 self.pending.push((
-                    key.clone(),
-                    Notice {
-                        id: id.clone(),
-                        ..plain(String::new(), String::new())
+                    key,
+                    Gathering {
+                        notice,
+                        title,
+                        body,
                     },
                 ));
                 self.pending.len() - 1
             }
         };
-        let notice = &mut self.pending[at].1;
-        if get(b'p') == Some(b"body") {
-            notice.body.push_str(&text);
+        let gathering = &mut self.pending[at].1;
+        let text = if get(b'p') == Some(b"body") {
+            &mut gathering.body
         } else {
-            notice.title.push_str(&text);
+            &mut gathering.title
+        };
+        // A chunk that breaks the rules drops what its notification gathered.
+        if let Err(why) = text.add(payload, encoded) {
+            self.pending.remove(at);
+            return Err(why);
         }
-        if notice.title.len() + notice.body.len() > GATHERED {
+        if gathering.title.len() + gathering.body.len() > GATHERED {
             self.pending.remove(at);
             return Err(format!("a notification over {GATHERED} bytes"));
         }
+        let notice = &mut gathering.notice;
         if let Some(occasion) = get(b'o') {
             notice.occasion = match occasion {
                 b"unfocused" => Occasion::Unfocused,
@@ -303,7 +367,16 @@ impl Kitty {
         if get(b'd') == Some(b"0") {
             return Ok(None);
         }
-        let (_, mut notice) = self.pending.remove(at);
+        let (_, gathering) = self.pending.remove(at);
+        let Gathering {
+            mut notice,
+            mut title,
+            mut body,
+        } = gathering;
+        title.finish()?;
+        body.finish()?;
+        notice.title = String::from_utf8(title.bytes).map_err(|_| "title is not UTF-8")?;
+        notice.body = String::from_utf8(body.bytes).map_err(|_| "body is not UTF-8")?;
         if notice.title.is_empty() && notice.body.is_empty() {
             return Ok(None);
         }

src/term/notify/tests.rs

@@ -121,6 +121,34 @@ fn osc_99_decodes_base64_and_takes_plain_text() {
     );
 }
 
+#[test]
+fn osc_99_gathers_base64_chunked_before_or_after_encoding() {
+    let title = |chunks: &[&str]| {
+        let mut kitty = Kitty::default();
+        let (last, first) = chunks.split_last().expect("a chunk");
+        for chunk in first {
+            assert_eq!(kitty.read(format!("i=a:d=0:e=1;{chunk}").as_bytes()), None);
+        }
+        shown(kitty.read(format!("i=a:e=1;{last}").as_bytes())).title
+    };
+    assert_eq!(title(&["SGVsb", "G8="]), "Hello", "split inside a group");
+    assert_eq!(title(&["8J+Y", "gA=="]), "😀", "split inside a character");
+    assert_eq!(
+        title(&["SGVsbG8=", "IHdvcmxk"]),
+        "Hello world",
+        "each chunk padded"
+    );
+    assert_eq!(title(&["SGVsbG", "8"]), "Hello", "the last chunk unpadded");
+    let mut kitty = Kitty::default();
+    assert_eq!(kitty.read(b"i=n:d=0;Title"), None);
+    let notice = shown(kitty.read(b"i=n:p=body:e=1;TGluZSAxCkxpbmUgMg=="));
+    assert_eq!(
+        (notice.title.as_str(), notice.body.as_str()),
+        ("Title", "Line 1\nLine 2"),
+        "base64 carries any UTF-8"
+    );
+}
+
 #[test]
 fn osc_99_asks_to_close_and_what_is_alive_and_supported() {
     let mut kitty = Kitty::default();
@@ -158,8 +186,11 @@ fn osc_99_drops_what_breaks_its_rules() {
                 body.push(byte);
                 body
             }),
+        b";a\tb".to_vec(),
+        b"e=1;SGVsbG8gS".to_vec(),
         long(PLAIN + 1, ""),
         long(ENCODED + 4, "e=1"),
+        long(1, &"x".repeat(METADATA + 1)),
     ] {
         assert_eq!(
             kitty.read(&body),
@@ -168,6 +199,9 @@ fn osc_99_drops_what_breaks_its_rules() {
             String::from_utf8_lossy(&body)
         );
     }
+    assert_eq!(kitty.read(b"i=k:d=0;Hel"), None);
+    assert_eq!(kitty.read(b"i=k:d=0:e=1;!!!!"), None);
+    assert!(kitty.pending.is_empty(), "a bad chunk drops what gathered");
     for at in 0..PENDING {
         assert_eq!(kitty.read(format!("i=p{at}:d=0;x").as_bytes()), None);
     }

src/term/pty/tests.rs

@@ -147,6 +147,20 @@ fn notifications_and_progress_reach_the_session_in_order() {
     assert_eq!(ran.text, "x");
 }
 
+#[test]
+fn osc_99_gathers_a_full_encoded_chunk_and_the_lines_base64_carries() {
+    let ran = run(
+        "printf '\\033]99;i=m:d=0:e=1;'; yes YWFh | head -n 1024 | tr -d '\\n'; printf '\\033\\\\\\033]99;i=m:d=0:e=1;RU5E\\033\\\\\\033]99;i=m:p=body:e=1;TGluZSAxCkxpbmUgMg==\\007x'",
+        (40, 4),
+    );
+    let [Ask::Show(notice)] = ran.asks.as_slice() else {
+        panic!("one notification: {:?}", ran.asks);
+    };
+    assert_eq!(notice.title, format!("{}END", "a".repeat(3072)));
+    assert_eq!(notice.body, "Line 1\nLine 2");
+    assert_eq!(ran.text, "x");
+}
+
 #[test]
 fn a_long_escape_of_controls_keeps_the_pty_reading() {
     let ran = run(

src/term/tap.rs

@@ -239,7 +239,7 @@ impl Osc {
                 }
                 (State::Osc, byte) => {
                     self.length += 1;
-                    if self.body.len() < MOST {
+                    if self.body.len() < self.most() {
                         self.body.push(byte);
                     } else {
                         self.overflow = true;
@@ -251,9 +251,18 @@ impl Osc {
         }
     }
 
+    /// The longest body kept: an OSC 99 holds a payload of 4096 bytes past its metadata.
+    fn most(&self) -> usize {
+        if self.body.starts_with(b"99;") {
+            notify::BODY
+        } else {
+            MOST
+        }
+    }
+
     fn finish(&mut self, end: usize, found: &mut impl FnMut(Found)) {
         if self.overflow {
-            log::warn!("pty: an OSC over {MOST} bytes dropped");
+            log::warn!("pty: an OSC over {} bytes dropped", self.most());
         } else {
             found(Found::Osc(&self.body, self.length + end));
         }

src/term/tap/tests.rs

@@ -74,6 +74,13 @@ fn an_overlong_body_is_dropped() {
     assert_eq!(scan(&[&long, b"\x1b]7;b\x07"]), ["7;b"]);
 }
 
+#[test]
+fn an_osc_99_body_holds_its_metadata_and_a_full_payload() {
+    let osc = |length: usize| [&b"\x1b]99;"[..], &vec![b'a'; length - 3], b"\x07"].concat();
+    assert_eq!(scan(&[&osc(notify::BODY)]).concat().len(), notify::BODY);
+    assert!(scan(&[&osc(notify::BODY + 1)]).is_empty());
+}
+
 #[test]
 fn mouse_modes_follow_private_modes_in_order_across_reads() {
     use crate::term::mouse::{Format, Tracking};