Merge pull request #513 from ljkx/update/sums-duplicate-names

update: one asset per name in SHA256SUMS.txt (#478)

crates/update/src/release.rs

@@ -3,6 +3,8 @@
 //! Everything here is untrusted input. Sizes are bounded by the caller, download links must point
 //! at the app's own release downloads, and checksums must look like checksums.
 
+use std::collections::{HashMap, HashSet};
+
 use serde::{Deserialize, Serialize};
 
 use crate::version::{self, Version};
@@ -247,15 +249,36 @@ pub fn tag_from_redirect(location: &str, repo: &str) -> Option<String> {
     (!rest.is_empty() && rest.len() <= 64 && rest.bytes().all(|b| b.is_ascii_alphanumeric() || b"._-+".contains(&b))).then(|| rest.to_string())
 }
 
-/// Reads `sha256sum` output: `<hex>  <name>` (or `<hex> *<name>`) per line.
+/// Reads `sha256sum` output: `<hex>  <name>` (or `<hex> *<name>`) per line, one entry per name.
+/// A line repeated as is counts once; a name listed with two different hashes is dropped,
+/// whichever line comes first, so the list vouches for neither (a file nothing else vouches for
+/// isn't installed).
 pub fn parse_sha256sums(text: &str) -> Vec<(String, String)> {
-    text.lines()
-        .filter_map(|line| {
-            let (hash, rest) = line.trim().split_once(char::is_whitespace)?;
-            let name = rest.trim_start().trim_start_matches('*');
-            (is_hex64(hash) && valid_file_name(name)).then(|| (hash.to_ascii_lowercase(), name.to_string()))
-        })
-        .collect()
+    let mut sums: Vec<(String, String)> = Vec::new();
+    // Where each name is in `sums`: a 4 MiB list holds some 60,000 names, too many to search.
+    let mut index: HashMap<&str, usize> = HashMap::new();
+    let mut conflicting: HashSet<&str> = HashSet::new();
+    for line in text.lines() {
+        let Some((hash, rest)) = line.trim().split_once(char::is_whitespace) else { continue };
+        let name = rest.trim_start().trim_start_matches('*');
+        if !is_hex64(hash) || !valid_file_name(name) {
+            continue;
+        }
+        let hash = hash.to_ascii_lowercase();
+        match index.get(name) {
+            None => {
+                index.insert(name, sums.len());
+                sums.push((hash, name.to_string()));
+            }
+            Some(&i) => {
+                if sums.get(i).is_some_and(|(h, _)| *h != hash) && conflicting.insert(name) {
+                    log::warn!("SHA256SUMS.txt lists {name} with different checksums; ignoring it");
+                }
+            }
+        }
+    }
+    sums.retain(|(_, n)| !conflicting.contains(n.as_str()));
+    sums
 }
 
 /// A bare file name we'd be willing to save: no paths, no control characters.
@@ -428,6 +451,39 @@ mod tests {
         assert_eq!(r.version, Some(Version::new(0, 5, 0)));
     }
 
+    #[test]
+    fn a_name_listed_twice_gives_one_asset_or_none() {
+        let other = "b".repeat(64);
+        // The same line twice (and the same hash in upper case) is one entry.
+        let sums = parse_sha256sums(&format!("{HASH}  a.zip\n{HASH} *a.zip\n{}  a.zip\n", HASH.to_ascii_uppercase()));
+        assert_eq!(sums, [(HASH.to_string(), "a.zip".to_string())]);
+        let r = release_from_sums(REPO, "v0.5.0", &sums);
+        assert_eq!(r.assets.iter().filter(|a| a.name == "a.zip").count(), 1);
+        // Two hashes for one name: the name is dropped in either line order, and the other
+        // files keep theirs.
+        for text in [format!("{HASH}  a.zip\n{other}  a.zip\n{HASH}  b.zip\n"), format!("{other}  a.zip\n{HASH}  b.zip\n{HASH}  a.zip\n")] {
+            let sums = parse_sha256sums(&text);
+            assert_eq!(sums, [(HASH.to_string(), "b.zip".to_string())], "{text}");
+            let r = release_from_sums(REPO, "v0.5.0", &sums);
+            assert!(r.assets.iter().all(|a| a.name != "a.zip"), "{text}");
+        }
+        // A third, matching line doesn't bring the name back.
+        assert!(parse_sha256sums(&format!("{HASH}  a.zip\n{other}  a.zip\n{HASH}  a.zip\n")).is_empty());
+    }
+
+    #[test]
+    fn a_full_size_list_keeps_its_order_and_finds_a_conflict_across_it() {
+        // About as many names as fit in the 4 MiB a list may be, and a conflict between the
+        // first and last lines.
+        let other = "b".repeat(64);
+        let mut text: String = (0..57_000).map(|i| format!("{HASH}  f{i}\n")).collect();
+        text.push_str(&format!("{other}  f0\n"));
+        assert!(text.len() <= 4 << 20);
+        let sums = parse_sha256sums(&text);
+        assert_eq!(sums.len(), 56_999);
+        assert!(sums.iter().enumerate().all(|(i, (h, n))| *h == HASH && *n == format!("f{}", i + 1)));
+    }
+
     #[test]
     fn file_names_cannot_escape() {
         for bad in ["", "..", "a/b", "a\\b", ".hidden", "c:x", "x\u{0}"] {