feat: audit codebase (#72)

Reviewed-on: #72

crates/common/Cargo.toml

@@ -9,6 +9,7 @@ gpui.workspace = true
 nostr.workspace = true
 instant.workspace = true
 nostr-sdk.workspace = true
+sha2.workspace = true
 
 itertools.workspace = true
 chrono.workspace = true

crates/common/src/event.rs

@@ -1,20 +1,18 @@
-use std::hash::{DefaultHasher, Hash, Hasher};
-
 use itertools::Itertools;
 use nostr_sdk::prelude::*;
+use sha2::{Digest, Sha256};
+
+const HEX_DIGITS: [u8; 16] = *b"0123456789abcdef";
 
 pub trait EventExt {
+    /// Id over the event's participants, stable across Rust versions.
     fn uniq_id(&self) -> u64;
     fn extract_public_keys(&self) -> Vec<PublicKey>;
 }
 
 impl EventExt for Event {
     fn uniq_id(&self) -> u64 {
-        let mut hasher = DefaultHasher::new();
-        let mut pubkeys: Vec<PublicKey> = self.extract_public_keys();
-        pubkeys.sort();
-        pubkeys.hash(&mut hasher);
-        hasher.finish()
+        pubkey_set_uniq_id(&self.extract_public_keys())
     }
 
     fn extract_public_keys(&self) -> Vec<PublicKey> {
@@ -27,31 +25,62 @@ impl EventExt for Event {
 
 impl EventExt for UnsignedEvent {
     fn uniq_id(&self) -> u64 {
-        let mut hasher = DefaultHasher::new();
-        let mut pubkeys: Vec<PublicKey> = vec![];
-
-        // Add all public keys from event
-        pubkeys.push(self.pubkey);
-        pubkeys.extend(self.tags.public_keys().collect::<Vec<_>>());
-
-        // Generate unique hash
-        pubkeys
-            .into_iter()
-            .unique()
-            .sorted()
-            .collect::<Vec<_>>()
-            .hash(&mut hasher);
-
-        hasher.finish()
+        pubkey_set_uniq_id(&self.extract_public_keys())
     }
 
     fn extract_public_keys(&self) -> Vec<PublicKey> {
         let mut public_keys: Vec<PublicKey> = self.tags.public_keys().collect();
         public_keys.push(self.pubkey);
+
         public_keys.into_iter().unique().sorted().collect()
     }
 }
 
+/// Digest over NUL-separated parts, so part boundaries stay unambiguous.
+fn parts_digest(parts: &[&str]) -> [u8; 32] {
+    let mut hasher = Sha256::new();
+
+    for part in parts {
+        hasher.update(part.as_bytes());
+        hasher.update([0]);
+    }
+
+    hasher.finalize().into()
+}
+
+/// Stable id over parts: hex-encoded SHA-256 truncated to 128 bits.
+pub fn stable_id(parts: &[&str]) -> String {
+    let digest = parts_digest(parts);
+
+    let mut id = String::with_capacity(32);
+    for byte in &digest[..16] {
+        id.push(HEX_DIGITS[usize::from(byte >> 4)] as char);
+        id.push(HEX_DIGITS[usize::from(byte & 0x0f)] as char);
+    }
+
+    id
+}
+
+/// Stable 64-bit id over parts, for callers that key ids on `u64`.
+pub fn stable_id_u64(parts: &[&str]) -> u64 {
+    let digest = parts_digest(parts);
+
+    let mut lead = [0u8; 8];
+    for (slot, byte) in lead.iter_mut().zip(digest.iter()) {
+        *slot = *byte;
+    }
+
+    u64::from_be_bytes(lead)
+}
+
+/// Hashes the sorted hex forms of the participant set.
+fn pubkey_set_uniq_id(public_keys: &[PublicKey]) -> u64 {
+    let mut parts: Vec<String> = public_keys.iter().map(|key| key.to_string()).collect();
+    parts.sort();
+    let parts: Vec<&str> = parts.iter().map(String::as_str).collect();
+    stable_id_u64(&parts)
+}
+
 /// Custom emoji (NIP-30) declared by `emoji` tags, first per shortcode wins
 pub fn emojis_from_tags(tags: &Tags) -> Vec<(String, Url)> {
     let mut emojis: Vec<(String, Url)> = Vec::new();
@@ -100,4 +129,51 @@ mod tests {
             Url::parse("https://example.com/soapbox.png").unwrap()
         );
     }
+
+    fn event_with_participants(public_key: PublicKey, participants: Vec<PublicKey>) -> Event {
+        let tags = Tags::from_list(participants.into_iter().map(Tag::public_key).collect());
+
+        Event::new(
+            EventId::from_byte_array([0; 32]),
+            public_key,
+            Timestamp::from(0u64),
+            Kind::TextNote,
+            tags,
+            "content".to_string(),
+            Signature::from_byte_array([0; 64]),
+        )
+    }
+
+    #[test]
+    fn uniq_id_ignores_participant_order() {
+        let me = Keys::new(SecretKey::generate()).public_key();
+        let alice = Keys::new(SecretKey::generate()).public_key();
+        let bob = Keys::new(SecretKey::generate()).public_key();
+
+        let first = event_with_participants(me, vec![alice, bob]);
+        let second = event_with_participants(me, vec![bob, alice]);
+
+        assert_eq!(first.uniq_id(), second.uniq_id());
+    }
+
+    #[test]
+    fn uniq_id_tracks_participants() {
+        let me = Keys::new(SecretKey::generate()).public_key();
+        let alice = Keys::new(SecretKey::generate()).public_key();
+        let bob = Keys::new(SecretKey::generate()).public_key();
+
+        let with_alice = event_with_participants(me, vec![alice]);
+        let with_bob = event_with_participants(me, vec![bob]);
+
+        assert_ne!(with_alice.uniq_id(), with_bob.uniq_id());
+    }
+
+    #[test]
+    fn stable_id_is_deterministic_128_bit_hex() {
+        let id = stable_id(&["alpha", "beta"]);
+
+        assert_eq!(id.len(), 32);
+        assert!(id.chars().all(|character| character.is_ascii_hexdigit()));
+        assert_eq!(id, stable_id(&["alpha", "beta"]));
+    }
 }

crates/common/src/media_extractor.rs

@@ -1,65 +1,161 @@
+use std::sync::LazyLock;
+
 use gpui::SharedUri;
 use regex::Regex;
 
-/// Extracts media URLs from a string and returns both
-/// the extracted URLs and the string with media URLs removed
-struct MediaExtractor {
-    image_regex: Regex,
-}
+/// Matches image URLs with common image extensions
+static IMAGE_RE: LazyLock<Regex> = LazyLock::new(|| {
+    Regex::new(
+        r#"(?i)\bhttps?://[^\s<>"']+\.(?:jpg|jpeg|png|gif|bmp|webp|svg|ico)(?:\?[^\s<>"']*)?\b"#,
+    )
+    .unwrap()
+});
+
+/// Extracts media URLs from a string and returns the URLs and the cleaned text
+struct MediaExtractor;
 
 impl MediaExtractor {
-    /// Creates a new MediaExtractor with compiled regex patterns
-    fn new() -> Self {
-        MediaExtractor {
-            // Match common image extensions
-            image_regex: Regex::new(
-                r#"(?i)\bhttps?://[^\s<>"']+\.(?:jpg|jpeg|png|gif|bmp|webp|svg|ico)(?:\?[^\s<>"']*)?\b"#,
-            ).unwrap(),
-        }
-    }
-
     /// Extracts all media URLs from a string
-    fn extract_media_urls(&self, text: &str) -> Vec<SharedUri> {
-        let mut urls = Vec::new();
-
-        // Extract image URLs
-        for capture in self.image_regex.find_iter(text) {
-            urls.push(capture.as_str().to_string().into());
-        }
-
-        urls
+    fn extract_media_urls(text: &str) -> Vec<SharedUri> {
+        IMAGE_RE
+            .find_iter(text)
+            .map(|capture| capture.as_str().to_string().into())
+            .collect()
     }
 
     /// Removes all media URLs from a string and returns the cleaned text
-    fn remove_media_urls(&self, text: &str) -> String {
-        let mut result = text.to_string();
+    fn remove_media_urls(text: &str) -> String {
+        let mut cleaned = String::with_capacity(text.len());
+        let mut last_end = 0;
+        // Whitespace bordered a removed URL, so one space stands in for it
+        let mut spaced = false;
 
-        // Remove image URLs
-        result = self.image_regex.replace_all(&result, "").to_string();
+        for url in IMAGE_RE.find_iter(text) {
+            let raw_gap = &text[last_end..url.start()];
+            let surrounded = raw_gap.ends_with(char::is_whitespace)
+                || text[url.end()..].starts_with(char::is_whitespace);
 
-        // Clean up extra whitespace that might result from removal
-        self.cleanup_text(&result)
+            let gap = if last_end == 0 {
+                raw_gap.trim_end()
+            } else {
+                raw_gap.trim()
+            };
+
+            if !gap.is_empty() {
+                if spaced && !cleaned.is_empty() {
+                    cleaned.push(' ');
+                }
+                cleaned.push_str(gap);
+            }
+
+            spaced |= surrounded;
+            last_end = url.end();
+        }
+
+        let tail = text[last_end..].trim_start();
+        if spaced && !cleaned.is_empty() && !tail.is_empty() {
+            cleaned.push(' ');
+        }
+        cleaned.push_str(tail);
+
+        cleaned.trim().to_string()
     }
 
     /// Extracts media URLs and removes them from the string, returning both
-    fn extract_and_remove(&self, text: &str) -> (Vec<SharedUri>, String) {
-        let urls = self.extract_media_urls(text);
-        let cleaned_text = self.remove_media_urls(text);
-        (urls, cleaned_text)
-    }
-
-    /// Helper function to clean up text after URL removal
-    fn cleanup_text(&self, text: &str) -> String {
-        let text = text.trim();
-
-        // Remove multiple consecutive spaces
-        let re = Regex::new(r"\s+").unwrap();
-        re.replace_all(text, " ").trim().to_string()
+    fn extract_and_remove(text: &str) -> (Vec<SharedUri>, String) {
+        (
+            Self::extract_media_urls(text),
+            Self::remove_media_urls(text),
+        )
     }
 }
 
 /// Convenience function for one-time extraction and removal
 pub fn extract_and_remove_media_urls(text: &str) -> (Vec<SharedUri>, String) {
-    let extractor = MediaExtractor::new();
-    extractor.extract_and_remove(text)
+    MediaExtractor::extract_and_remove(text)
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn multi_line_text_without_urls_keeps_line_breaks() {
+        let text = "first line\n\nsecond line";
+        let (urls, cleaned) = extract_and_remove_media_urls(text);
+
+        assert!(urls.is_empty());
+        assert_eq!(cleaned, text);
+    }
+
+    #[test]
+    fn removes_url_and_tidies_around_it() {
+        let (urls, cleaned) =
+            extract_and_remove_media_urls("check https://example.com/pic.png here");
+
+        assert_eq!(urls, vec![SharedUri::from("https://example.com/pic.png")]);
+        assert_eq!(cleaned, "check here");
+    }
+
+    #[test]
+    fn removing_url_keeps_other_line_breaks() {
+        let (urls, cleaned) = extract_and_remove_media_urls(
+            "first\nsecond https://example.com/pic.png third\nfourth",
+        );
+
+        assert_eq!(urls.len(), 1);
+        assert_eq!(cleaned, "first\nsecond third\nfourth");
+    }
+
+    #[test]
+    fn url_on_its_own_line_joins_neighbors_with_one_space() {
+        let (urls, cleaned) =
+            extract_and_remove_media_urls("first\n\nhttps://example.com/pic.png\n\nlast");
+
+        assert_eq!(urls.len(), 1);
+        assert_eq!(cleaned, "first last");
+    }
+
+    #[test]
+    fn multiple_urls_collapse_to_single_spaces() {
+        let (urls, cleaned) = extract_and_remove_media_urls(
+            "a https://example.com/a.png b https://example.com/b.png c",
+        );
+
+        assert_eq!(urls.len(), 2);
+        assert_eq!(cleaned, "a b c");
+    }
+
+    #[test]
+    fn message_with_only_a_url_becomes_empty() {
+        let (urls, cleaned) = extract_and_remove_media_urls("https://example.com/pic.png");
+
+        assert_eq!(urls.len(), 1);
+        assert_eq!(cleaned, "");
+    }
+
+    #[test]
+    fn digits_glued_to_url_scheme_prevent_match() {
+        let text = "v1.2https://example.com/pic.png";
+        let (urls, cleaned) = extract_and_remove_media_urls(text);
+
+        assert!(urls.is_empty());
+        assert_eq!(cleaned, text);
+    }
+
+    #[test]
+    fn digits_around_removed_url_survive() {
+        let (urls, cleaned) = extract_and_remove_media_urls("123 https://example.com/pic.png 456");
+
+        assert_eq!(urls.len(), 1);
+        assert_eq!(cleaned, "123 456");
+    }
+
+    #[test]
+    fn emoji_around_removed_url_survives() {
+        let (urls, cleaned) = extract_and_remove_media_urls("👍 https://example.com/pic.png 👍");
+
+        assert_eq!(urls.len(), 1);
+        assert_eq!(cleaned, "👍 👍");
+    }
 }

crates/common/src/parser.rs

@@ -184,20 +184,11 @@ impl Iterator for FindMatches<'_> {
     }
 }
 
-enum HandleMatch {
-    Token(Token),
-    Recursion,
-}
-
 pub struct NostrParserIter<'a> {
     /// The original text
     text: &'a str,
     /// Matches found
     matches: FindMatches<'a>,
-    /// A pending match
-    pending_match: Option<Match>,
-    /// Last match end index
-    last_match_end: usize,
 }
 
 impl<'a> NostrParserIter<'a> {
@@ -205,62 +196,65 @@ impl<'a> NostrParserIter<'a> {
         Self {
             text,
             matches: FindMatches::new(text),
-            pending_match: None,
-            last_match_end: 0,
         }
     }
 
-    fn handle_match(&mut self, mat: Match) -> HandleMatch {
-        // Update last match end
-        self.last_match_end = mat.end;
-
+    fn handle_match(&self, mat: Match) -> Option<Token> {
         // Extract the matched string
         let data: &str = &self.text[mat.start..mat.end];
 
         // Parse NIP-21 URI
         match Nip21::parse(data) {
-            Ok(uri) => HandleMatch::Token(Token {
+            Ok(uri) => Some(Token {
                 value: uri,
                 range: mat.start..mat.end,
             }),
             // If the nostr URI parsing is invalid, skip it
-            Err(_) => HandleMatch::Recursion,
+            Err(_) => None,
         }
     }
 }
 
-impl<'a> Iterator for NostrParserIter<'a> {
+impl Iterator for NostrParserIter<'_> {
     type Item = Token;
 
     fn next(&mut self) -> Option<Self::Item> {
-        // Handle a pending match
-        if let Some(pending_match) = self.pending_match.take() {
-            return match self.handle_match(pending_match) {
-                HandleMatch::Token(token) => Some(token),
-                HandleMatch::Recursion => self.next(),
-            };
-        }
+        // Iterative, not recursive: message text controls the match count
+        loop {
+            let mat = self.matches.next()?;
 
-        match self.matches.next() {
-            Some(mat) => {
-                // Skip any text before this match
-                if mat.start > self.last_match_end {
-                    // Update pending match
-                    // This will be handled at next iteration, in `handle_match` method.
-                    self.pending_match = Some(mat);
-
-                    // Skip the text before the match
-                    self.last_match_end = mat.start;
-                    return self.next();
-                }
-
-                // Handle match
-                match self.handle_match(mat) {
-                    HandleMatch::Token(token) => Some(token),
-                    HandleMatch::Recursion => self.next(),
-                }
+            if let Some(token) = self.handle_match(mat) {
+                return Some(token);
             }
-            None => None,
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    // A long run of scanner matches that fail `Nip21::parse`: the count is
+    // attacker-controlled, so it must be consumed iteratively.
+    #[test]
+    fn skips_long_run_of_invalid_uris() {
+        let content = "nostr:1q".repeat(50_000);
+
+        let tokens: Vec<Token> = NostrParser::new().parse(&content).collect();
+
+        assert!(tokens.is_empty());
+    }
+
+    #[test]
+    fn yields_valid_uri_after_invalid_run() {
+        let public_key = Keys::new(SecretKey::generate()).public_key();
+        let npub = public_key.to_bech32().unwrap();
+        let content = format!("{}nostr:{npub}", "nostr:1q".repeat(50_000));
+
+        let tokens: Vec<Token> = NostrParser::new().parse(&content).collect();
+
+        assert_eq!(tokens.len(), 1);
+        assert_eq!(tokens[0].range.start, 400_000);
+        assert_eq!(tokens[0].value, Nip21::Pubkey(public_key));
+    }
+}