diff --git a/crates/core/src/feed.rs b/crates/core/src/feed.rs index 5e66acb5..0a790c5c 100644 --- a/crates/core/src/feed.rs +++ b/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, pub created_at: Option, } @@ -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 { - 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 = (0..news).map(|i| format!(r#"{{"title": "News {i}", "body": "x"}}"#)).collect(); + let moved: Vec = (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:?}"); + } + } } diff --git a/crates/core/src/release.rs b/crates/core/src/release.rs index f599b999..f9b30557 100644 --- a/crates/core/src/release.rs +++ b/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 Re /// The tag in a `https://github.com//releases/tag/` redirect target. pub fn tag_from_redirect(location: &str, repo: &str) -> Option { 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: ` ` (or ` *`) 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); } }