fix(core): tighten tag, file name and notice link checks (#776, #782) (#1024)

* fix(core): tighten tag, file name and notice link checks (#776, #782)

- Tags from GitHub, the feed and the latest-release redirect must be up to
  64 of [A-Za-z0-9._+-], not starting with a dot (release::valid_tag).
  A feed tag like `v0.6.0~beta-` plus a bidi override still read as
  0.6.0-beta and was served; such a release now falls back to GitHub.
- Asset file names use the same allowlist, with no trailing dot and no
  Windows device names (CON, nul.txt, COM1.msi).
- Notice links go through safe_web_link, so plain http and user-info
  links are dropped, as in release notes.
- The 20-notice cap now counts the rename notices too.

* fix(core): treat only COM1-COM9 and LPT1-LPT9 as Windows device names

Microsoft's naming rules reserve COM1 to COM9 and LPT1 to LPT9 (plus the
superscript forms, which the ASCII allowlist already refuses), not COM0
or LPT0. Match them with a slice pattern instead of indexing.

* test(core): cover the hostile tags, names and links from review (#776, #782)

Adds a NUL in a tag, a file: notice link, 1,000 announcements, and direct
checks that a download link and a redirect use the same rules.

* fix(core): keep rename notices inside the notice cap (#782)

Twenty announcements left no room, so the cap cut the rename added after them.

* docs(core): notice links are https only now (#776)

crates/core/src/feed.rs

@@ -90,7 +90,7 @@ pub struct Notice {
     pub id: String,
     pub title: String,
     pub body: String,
-    /// Only `https://` / `http://` links survive parsing.
+    /// Only `https://` links survive parsing.
     pub link: Option<String>,
     pub created_at: Option<i64>,
 }
@@ -120,7 +120,8 @@ impl Feed {
             alerts: raw.important_notices.alerts.into_iter().filter_map(|n| n.into_notice("alert")).take(MAX_NOTICES).collect(),
             mirror: None,
         };
-        // App renames are news too: show them with the announcements.
+        // App renames are news too: show them with the announcements, inside the cap.
+        feed.announcements.truncate(MAX_NOTICES.saturating_sub(feed.app_renames.len()));
         for r in &feed.app_renames {
             feed.announcements.push(Notice {
                 id: format!("rename:{}:{}", r.old_name_lower, r.new_name_lower),
@@ -130,6 +131,7 @@ impl Feed {
                 created_at: r.renamed_at,
             });
         }
+        feed.announcements.truncate(MAX_NOTICES);
         Some(feed)
     }
 
@@ -183,9 +185,9 @@ impl Feed {
 
 impl FeedRelease {
     /// The release with only the downloads that belong to `repo` (or sit under `mirror`). `None`
-    /// when it doesn't hold up (no tag, or not a version).
+    /// when it doesn't hold up (no tag, a tag with odd characters, or not a version).
     fn to_release(&self, repo: &str, mirror: Option<&str>) -> Option<Release> {
-        let version = version::parse(&self.tag)?;
+        let version = version::parse(&self.tag).filter(|_| release::valid_tag(&self.tag))?;
         let allowed = |name: &str, url: &str| {
             (release::download_url_under(url, &release::download_prefix(repo))
                 && url == format!("{}{}/{}", release::download_prefix(repo), self.tag, name))
@@ -415,13 +417,7 @@ impl RawNotice {
         if self.title.trim().is_empty() && self.body.trim().is_empty() {
             return None;
         }
-        // The link may reach the system opener (whose WSL fallback shells out), so refuse any
-        // shell/cmd metacharacter or control character in it as well as whitespace (CWE-78).
-        let link = self.maybe_web_link.filter(|l| {
-            (l.starts_with("https://") || l.starts_with("http://"))
-                && l.len() <= 2000
-                && !l.chars().any(|c| c.is_whitespace() || c.is_control() || release::is_shell_meta(c))
-        });
+        let link = self.maybe_web_link.filter(|l| release::safe_web_link(l));
         Some(Notice {
             id: format!("{kind}:{}:{}", self.created_at.as_deref().unwrap_or(""), self.title.trim()),
             title: truncate(self.title.trim(), 200),
@@ -633,7 +629,10 @@ mod tests {
                 r#""alerts": [
             {"title": "Security update", "body": "Update PdfCraft now.", "maybe_web_link": "https://getartcraft.com/security", "created_at": null},
             {"title": "Bad link", "body": "x", "maybe_web_link": "javascript:alert(1)"},
-            {"title": "Shell link", "body": "x", "maybe_web_link": "https://getartcraft.com/a|calc.exe"}]"#,
+            {"title": "Shell link", "body": "x", "maybe_web_link": "https://getartcraft.com/a|calc.exe"},
+            {"title": "Plain http", "body": "x", "maybe_web_link": "http://getartcraft.com/security"},
+            {"title": "User-info", "body": "x", "maybe_web_link": "https://getartcraft.com@evil.example/"},
+            {"title": "File link", "body": "x", "maybe_web_link": "file:///etc/passwd"}]"#,
             )
             .replace(
                 "\"app_renames\": []",
@@ -641,13 +640,44 @@ mod tests {
             "new_name_lower": "docucraft", "description": "Same app, new name.", "renamed_at": "2026-10-09T08:00:00Z"}]"#,
             );
         let feed = Feed::parse(&with_notices).unwrap();
-        assert_eq!(feed.alerts.len(), 3);
+        assert_eq!(feed.alerts.len(), 6);
         assert_eq!(feed.alerts[0].link.as_deref(), Some("https://getartcraft.com/security"));
         assert_eq!(feed.alerts[1].link, None, "only web links are kept");
         assert_eq!(feed.alerts[2].link, None, "a link with shell metacharacters is dropped");
+        assert_eq!(feed.alerts[3].link, None, "plain http is dropped");
+        assert_eq!(feed.alerts[4].link, None, "a link with user-info is dropped");
+        assert_eq!(feed.alerts[5].link, None, "a file link is dropped");
         let titles: Vec<&str> = feed.announcements.iter().map(|n| n.title.as_str()).collect();
         assert_eq!(titles, ["New: CADCraft 1.0", "PdfCraft is now DocuCraft"]);
         // Ids are stable across parses, so dismissals stick.
         assert_eq!(Feed::parse(&with_notices).unwrap().alerts[0].id, feed.alerts[0].id);
     }
+
+    #[test]
+    fn rename_notices_count_toward_the_cap() {
+        let rename = |i: usize| {
+            format!(
+                r#"{{"old_name": "PdfCraft{i}", "old_name_lower": "pdfcraft{i}", "new_name": "DocuCraft{i}", "new_name_lower": "docucraft{i}",
+            "description": "Same app, new name.", "renamed_at": "2026-10-09T08:00:00Z"}}"#
+            )
+        };
+        for (news, renames) in [(MAX_NOTICES, 1), (1000, 1), (0, 30)] {
+            let many: Vec<String> = (0..news).map(|i| format!(r#"{{"title": "News {i}", "body": "x"}}"#)).collect();
+            let moved: Vec<String> = (0..renames).map(rename).collect();
+            let full = sample()
+                .replace("\"announcements\": []", &format!("\"announcements\": [{}]", many.join(",")))
+                .replace("\"app_renames\": []", &format!("\"app_renames\": [{}]", moved.join(",")));
+            let notices = Feed::parse(&full).unwrap().announcements;
+            assert_eq!(notices.len(), MAX_NOTICES, "{news} announcements, {renames} renames");
+            assert!(notices.iter().any(|n| n.title == "PdfCraft0 is now DocuCraft0"), "{news} announcements: the rename was cut");
+        }
+    }
+
+    #[test]
+    fn a_feed_release_with_an_odd_tag_asks_github() {
+        for odd in ["v0.6.0~beta-\\u202e", "v0.6.0~beta-a/b"] {
+            let feed = sample().replace("\"tag\": \"v0.6.0\"", &format!("\"tag\": \"{odd}\""));
+            assert_eq!(Feed::parse(&feed).unwrap().answer(app("photocraft"), NOW), FeedAnswer::AskGitHub, "{odd:?}");
+        }
+    }
 }

crates/core/src/release.rs

@@ -143,7 +143,7 @@ fn download_suffix_ok(rest: &str) -> bool {
     }
     let mut parts = rest.split('/');
     let (Some(tag), Some(name), None) = (parts.next(), parts.next(), parts.next()) else { return false };
-    !tag.is_empty() && tag != "." && tag != ".." && tag.len() <= 64 && !tag.contains('\\') && valid_file_name(name)
+    valid_tag(tag) && valid_file_name(name)
 }
 
 /// Any `.` or `..` path segment that could climb out of the repository.
@@ -228,7 +228,7 @@ fn release_from_value(v: &serde_json::Value, repo: &str) -> Result<Release, Stri
     if let Some(msg) = v["message"].as_str() {
         return Err(if msg == "Not Found" { "no releases yet".into() } else { format!("GitHub says: {}", truncate(msg, 200)) });
     }
-    let tag = v["tag_name"].as_str().filter(|t| !t.is_empty() && t.len() <= 64).ok_or("no release found")?.to_string();
+    let tag = v["tag_name"].as_str().filter(|t| valid_tag(t)).ok_or("no release found")?.to_string();
     let releases = format!("https://github.com/{repo}/releases");
     let html_url = v["html_url"].as_str().and_then(|u| safe_release_html_url(repo, u)).unwrap_or_else(|| releases.clone());
     let prefix = download_prefix(repo);
@@ -284,7 +284,18 @@ pub fn release_from_sums(repo: &str, tag: &str, sums: &[(String, String)]) -> Re
 /// The tag in a `https://github.com/<repo>/releases/tag/<tag>` redirect target.
 pub fn tag_from_redirect(location: &str, repo: &str) -> Option<String> {
     let rest = location.strip_prefix(&format!("https://github.com/{repo}/releases/tag/"))?;
-    (!rest.is_empty() && rest.len() <= 64 && rest.bytes().all(|b| b.is_ascii_alphanumeric() || b"._-+".contains(&b))).then(|| rest.to_string())
+    valid_tag(rest).then(|| rest.to_string())
+}
+
+/// A release tag we'd put in a URL, a file or the UI: up to 64 of `[A-Za-z0-9._+-]`, not
+/// starting with a dot, so no separators, control or bidi characters and no `.`/`..`.
+pub fn valid_tag(tag: &str) -> bool {
+    tag.len() <= 64 && !tag.starts_with('.') && plain_name(tag)
+}
+
+/// Non-empty and only `[A-Za-z0-9._+-]`, the characters every Craft release tag and asset uses.
+fn plain_name(s: &str) -> bool {
+    !s.is_empty() && s.bytes().all(|b| b.is_ascii_alphanumeric() || b"._-+".contains(&b))
 }
 
 /// Reads `sha256sum` output: `<hex>  <name>` (or `<hex> *<name>`) per line.
@@ -298,14 +309,16 @@ pub fn parse_sha256sums(text: &str) -> Vec<(String, String)> {
         .collect()
 }
 
-/// A bare file name we'd be willing to save: no paths, no control characters.
+/// A bare file name we'd be willing to save: only `[A-Za-z0-9._+-]`, no leading or trailing dot,
+/// and not a name Windows reserves for a device (`CON`, `nul.txt`, `COM1.msi`).
 pub fn valid_file_name(name: &str) -> bool {
-    !name.is_empty()
-        && name.len() <= 200
-        && name != "."
-        && name != ".."
-        && !name.starts_with('.')
-        && !name.chars().any(|c| c.is_control() || matches!(c, '/' | '\\' | ':' | '<' | '>' | '"' | '|' | '?' | '*'))
+    name.len() <= 200 && !name.starts_with('.') && !name.ends_with('.') && plain_name(name) && !windows_device(name)
+}
+
+fn windows_device(name: &str) -> bool {
+    let stem = name.split('.').next().unwrap_or_default().to_ascii_uppercase();
+    matches!(stem.as_str(), "CON" | "PRN" | "AUX" | "NUL")
+        || matches!(stem.as_bytes(), [b'C', b'O', b'M', b'1'..=b'9'] | [b'L', b'P', b'T', b'1'..=b'9'])
 }
 
 /// `s` cut to at most `max` bytes (on a char boundary) with an ellipsis, for text from a
@@ -524,9 +537,56 @@ mod tests {
 
     #[test]
     fn file_names_cannot_escape() {
-        for bad in ["", "..", "a/b", "a\\b", ".hidden", "c:x", "x\u{0}"] {
+        for bad in [
+            "",
+            "..",
+            "a/b",
+            "a\\b",
+            ".hidden",
+            "c:x",
+            "x\u{0}",
+            "a b.dmg",
+            "a%2e.msi",
+            "a#b",
+            "evil\u{202e}gpj.exe",
+            "x.msi.",
+            "CON",
+            "nul.txt",
+            "COM1.msi",
+            "lpt9",
+            "prn",
+            "aux.log",
+            "a".repeat(201).as_str(),
+        ] {
             assert!(!valid_file_name(bad), "{bad:?}");
         }
-        assert!(valid_file_name("photocraft-0.5.0-linux-x86_64.AppImage"));
+        for good in [
+            "photocraft-0.5.0-linux-x86_64.AppImage",
+            "lightcraft_0.9.0_amd64.deb",
+            "x-1.0+git.tar.gz",
+            "console.zip",
+            "COM10.txt",
+            "COM0.msi",
+            "lpt0",
+        ] {
+            assert!(valid_file_name(good), "{good:?}");
+        }
+    }
+
+    #[test]
+    fn tags_are_plain() {
+        for good in ["v0.6.0", "v1.0.0-beta.2", "nightly-2026-10-29", "v1+build.7"] {
+            assert!(valid_tag(good), "{good:?}");
+        }
+        for bad in ["", ".", "..", ".v1", "v1/x", "v1\\x", "v1\nx", "v1\u{0}x", "v1\u{202e}", "v 1", "v1?x", "v".repeat(65).as_str()] {
+            assert!(!valid_tag(bad), "{bad:?}");
+        }
+        let api = r#"{"tag_name": "v1\u202egpj", "html_url": "", "assets": []}"#;
+        assert!(parse_github_release(api, REPO).is_err());
+        let prefix = download_prefix(REPO);
+        assert!(download_url_under(&format!("{prefix}v1/a.dmg"), &prefix));
+        assert!(!download_url_under(&format!("{prefix}v1\u{202e}/a.dmg"), &prefix), "an odd tag in a download link");
+        assert!(!download_url_under(&format!("{prefix}v1/a\u{202e}.dmg"), &prefix), "an odd name in a download link");
+        assert_eq!(tag_from_redirect(&format!("https://github.com/{REPO}/releases/tag/v1\u{202e}"), REPO), None);
     }
 }