diff --git a/crates/core/src/feed.rs b/crates/core/src/feed.rs index 66d62ac3..33dde8da 100644 --- a/crates/core/src/feed.rs +++ b/crates/core/src/feed.rs @@ -206,7 +206,12 @@ impl FeedRelease { name: if self.name.is_empty() { self.tag.clone() } else { truncate(&self.name, 120) }, tag: self.tag.clone(), published_at: self.published_at, - html_url: if self.html_url.starts_with(&releases_page) { self.html_url.clone() } else { format!("{releases_page}tag/{}", self.tag) }, + html_url: release::safe_release_html_url(repo, &self.html_url) + .or_else(|| { + let fallback = format!("{releases_page}tag/{}", self.tag); + release::safe_release_html_url(repo, &fallback) + }) + .unwrap_or_else(|| format!("https://github.com/{repo}/releases")), notes: truncate(&self.notes, 64 * 1024), prerelease: self.prerelease, assets, @@ -275,7 +280,7 @@ struct RawApp { impl RawApp { fn into_app(self) -> FeedApp { FeedApp { - repo: self.repo.filter(|r| r.split('/').count() == 2), + repo: self.repo.filter(|r| r.split('/').count() == 2 && r.split('/').all(|part| !part.is_empty() && part != "." && part != "..")), release: self.latest_release.and_then(RawRelease::into_release), fetched_at: self.app_last_fetched.as_deref().and_then(parse_rfc3339), } @@ -372,7 +377,7 @@ impl RawRepoRename { fn into_rename(self) -> Option { let ok = |r: &str| { let mut parts = r.split('/'); - matches!((parts.next(), parts.next(), parts.next()), (Some(a), Some(b), None) if a.eq_ignore_ascii_case(GITHUB_OWNER) && !b.is_empty() + matches!((parts.next(), parts.next(), parts.next()), (Some(a), Some(b), None) if a.eq_ignore_ascii_case(GITHUB_OWNER) && !b.is_empty() && b != "." && b != ".." && r.bytes().all(|c| c.is_ascii_alphanumeric() || b"-_./".contains(&c))) }; (ok(&self.old_name) && ok(&self.new_name)).then(|| RepoRename { diff --git a/crates/core/src/release.rs b/crates/core/src/release.rs index 648c1481..ca5419e4 100644 --- a/crates/core/src/release.rs +++ b/crates/core/src/release.rs @@ -85,6 +85,31 @@ pub fn is_shell_meta(c: char) -> bool { matches!(c, '|' | '&' | ';' | '<' | '>' | '(' | ')' | '$' | '`' | '"' | '\'' | '^' | '!' | '*' | '%' | '{' | '}' | '[' | ']') } +/// Whether `url` is a valid release page URL strictly for `repo` on GitHub: +/// starts with `https://github.com//releases`, contains no dot-segments (`.` or `..`), +/// no percent-encoded path separators (`%2f`, `%5c`), no query/fragment/backslash, and +/// satisfies [`safe_web_link`] (no shell metacharacters or control bytes, CWE-78). +pub fn safe_release_html_url(repo: &str, url: &str) -> Option { + if !safe_web_link(url) { + return None; + } + let releases = format!("https://github.com/{repo}/releases"); + if url == releases { + return Some(url.to_string()); + } + let rest = url.strip_prefix(&releases)?; + let path = rest.strip_prefix('/')?; + if path.is_empty() || path.contains(['?', '#', '\\']) { + return None; + } + for seg in path.split('/') { + if seg.is_empty() || seg == "." || seg == ".." || seg.contains("%2f") || seg.contains("%2F") || seg.contains("%5c") || seg.contains("%5C") { + return None; + } + } + Some(url.to_string()) +} + /// The prefix every download of `repo` must start with. pub fn download_prefix(repo: &str) -> String { format!("https://github.com/{repo}/releases/download/") @@ -168,10 +193,7 @@ fn release_from_value(v: &serde_json::Value, repo: &str) -> Result", REPO).is_err()); } + #[test] + fn release_html_url_rejects_traversal_and_metacharacters() { + let repo = "storytold/photocraft"; + assert_eq!( + safe_release_html_url(repo, "https://github.com/storytold/photocraft/releases/tag/v1.0.0"), + Some("https://github.com/storytold/photocraft/releases/tag/v1.0.0".to_string()) + ); + assert_eq!( + safe_release_html_url(repo, "https://github.com/storytold/photocraft/releases"), + Some("https://github.com/storytold/photocraft/releases".to_string()) + ); + for bad in [ + "https://github.com/storytold/photocraft/releases/../../attacker/repo/releases/tag/v1", + "https://github.com/storytold/photocraft/releases/tag/v1/../../../attacker/repo", + "https://github.com/storytold/photocraft/releases/tag/v1|calc.exe", + "https://github.com/storytold/photocraft/releases/tag/v1%2f..%2f..", + "https://github.com/storytold/photocraft/releases/tag/v1?x=1", + "https://github.com/storytold/photocraft/releases/tag/v1#hash", + "https://github.com/storytold/other/releases/tag/v1.0.0", + "http://github.com/storytold/photocraft/releases/tag/v1", + ] { + assert!(safe_release_html_url(repo, bad).is_none(), "should reject: {bad}"); + } + } + #[test] fn traversal_cannot_escape_the_repository() { // Raw-prefix matches but the interpreted path climbs into another repository (CWE-180). diff --git a/crates/ui-egui/src/widgets.rs b/crates/ui-egui/src/widgets.rs index 5786cbea..11277853 100644 --- a/crates/ui-egui/src/widgets.rs +++ b/crates/ui-egui/src/widgets.rs @@ -280,7 +280,11 @@ pub fn hud_link(ui: &mut Ui, text: &str, url: &str) -> Response { resp.widget_info(|| egui::WidgetInfo::labeled(egui::WidgetType::Link, true, text)); let resp = resp.on_hover_text(url).on_hover_cursor(egui::CursorIcon::PointingHand); if resp.clicked() { - ui.ctx().open_url(egui::OpenUrl::new_tab(url)); + if artcraft_launcher_core::release::safe_web_link(url) { + ui.ctx().open_url(egui::OpenUrl::new_tab(url)); + } else { + log::warn!("refusing to open unsafe URL: {url}"); + } } resp }