From f3706afe32002b77262a83ace1b92c8b4363c5aa Mon Sep 17 00:00:00 2001 From: smittie2000 Date: Tue, 27 Oct 2026 08:58:04 +0200 Subject: [PATCH] craft-update: accept exactly one download link per asset (craft-launcher#764) craft-update was extracted from ArtCraft Launcher (#311) with the launcher's release.rs as it was, so it has the hole craft-launcher#764 reports: github.com resolves `..` and `%2e%2e` segments before routing, and `download_url_under` only rejected literal `..` and `%2f`/`%5c`. This ports storytold/craft-launcher#921: - release::expected_download_url(repo, tag, name): `/`, tag and name limited to ASCII letters, digits and `._+-`. release_from_value keeps an asset only when its link equals it, which also ties the link to its own release. - contains_encoded_separator rejects `%2e` too, and valid_file_name rejects percent-encoded separators and dots, so release_from_sums can't build a link from `%2e%2e%2fother`. - install::check_download_link, called just before the download in install_at and in self_update::update. The self-update path doesn't go through install_at, so without the second call every app's self-update would skip the check. - truncate uses floor_char_boundary; bind_artifact_version uses split_once. Co-Authored-By: Claude Opus 5.5 --- crates/update/src/install.rs | 29 ++++++++++++- crates/update/src/release.rs | 71 +++++++++++++++++++++++++++----- crates/update/src/self_update.rs | 30 ++++++++++++++ 3 files changed, 118 insertions(+), 12 deletions(-) diff --git a/crates/update/src/install.rs b/crates/update/src/install.rs index 09de4e0c2..3686058ef 100644 --- a/crates/update/src/install.rs +++ b/crates/update/src/install.rs @@ -90,6 +90,7 @@ pub fn install_at( return Err(format!("Cannot update {} to {}: installed version {} is newer", app.name, release.version_label(), inst_v)); } log::info!("{}: starting {} to {} ({})", app.name, if existing.is_some() { "update" } else { "install" }, release.version_label(), kind.label()); + check_download_link(app, release, asset)?; let file = download(release, asset, job)?; log::info!("{}: download ready ({}), running installer...", app.name, file.display()); // Past this point the install can't be interrupted halfway. @@ -170,7 +171,7 @@ pub(crate) fn bind_artifact_version(app: &App, release: &Release, asset: &Asset) } // The release tag and filename embedded in a release download URL (`.../releases/download//`). - if let Some(rest) = asset.url.split("/releases/download/").nth(1) { + if let Some((_, rest)) = asset.url.split_once("/releases/download/") { let mut parts = rest.split('/'); let tag = parts.next().unwrap_or(""); let url_filename = parts.next().unwrap_or(""); @@ -187,6 +188,19 @@ pub(crate) fn bind_artifact_version(app: &App, release: &Release, asset: &Asset) Ok(()) } +/// The last check before any bytes are fetched (storytold/craft-launcher#764): the link must be +/// exactly the app's own GitHub download for this release's tag and this asset's name, whichever +/// converter (API, feed or `SHA256SUMS.txt`) built the asset. A `local-mirror` build also accepts +/// the developer's loopback mirror, whose links the feed parser has already confined; so do this +/// crate's tests, whose downloads come from a server on 127.0.0.1. +pub(crate) fn check_download_link(app: &App, release: &Release, asset: &Asset) -> Result<(), String> { + let mirrored = (cfg!(feature = "local-mirror") || cfg!(test)) && crate::http::is_loopback(&asset.url); + if mirrored || release::expected_download_url(app.repo, &release.tag, &asset.name).as_deref() == Some(asset.url.as_str()) { + return Ok(()); + } + Err(format!("{}'s download link isn't the one its release publishes ({}), so it wasn't downloaded.", app.name, asset.name)) +} + /// What a new install would write over: something already at its destination that the launcher /// didn't put there. pub fn occupied( @@ -2640,6 +2654,19 @@ mod tests { ..Asset::default() }; assert!(bind_artifact_version(app, &release, &bad_tag).is_err()); + + // storytold/craft-launcher#764: another app's release with the right tag and name passes + // the bind (it reads only the path after `/releases/download/`), but the check before the + // download refuses it. + let other_repo = Asset { + name: "photocraft-99.0.0-linux-x86_64.AppImage".into(), + url: "https://github.com/storytold/vectorcraft/releases/download/v99.0.0/photocraft-99.0.0-linux-x86_64.AppImage".into(), + ..Asset::default() + }; + assert!(bind_artifact_version(app, &release, &other_repo).is_ok()); + let err = install_at(app, &release, &other_repo, PackageKind::AppImage, None, None, &mut job, None).unwrap_err(); + assert!(err.contains("isn't the one its release publishes"), "{err}"); + assert!(check_download_link(app, &release, &good).is_ok()); } #[cfg(target_os = "linux")] diff --git a/crates/update/src/release.rs b/crates/update/src/release.rs index ca5419e4f..b0c545208 100644 --- a/crates/update/src/release.rs +++ b/crates/update/src/release.rs @@ -115,6 +115,17 @@ pub fn download_prefix(repo: &str) -> String { format!("https://github.com/{repo}/releases/download/") } +/// The one link a GitHub release asset may have: `https://github.com//releases/download//`, +/// built from the release's own tag and the asset's own name, so a link can't point at another +/// release or anywhere else (#764). `None` when the tag or name holds anything but ASCII letters, +/// digits and `._+-`, which no Craft App release uses and which github.com could re-interpret. +pub fn expected_download_url(repo: &str, tag: &str, name: &str) -> Option { + let plain = |s: &str, max: usize| { + !s.is_empty() && s.len() <= max && s != "." && s != ".." && s.bytes().all(|b| b.is_ascii_alphanumeric() || b"._+-".contains(&b)) + }; + (plain(tag, 64) && plain(name, 200) && valid_file_name(name)).then(|| format!("{}{tag}/{name}", download_prefix(repo))) +} + /// Whether `url` is a download that truly belongs to `repo`. /// /// A raw `starts_with(prefix)` check is not enough: a value such as @@ -151,11 +162,12 @@ fn contains_traversal(rest: &str) -> bool { rest.split('/').any(|seg| seg == "." || seg == "..") } -/// Percent-encoded slash or backslash (`%2f`, `%5c`): a client could decode these back into -/// separators after the check, so a confined URL must not contain them. +/// Percent-encoded slash, backslash or dot (`%2f`, `%5c`, `%2e`): a client or github.com could +/// decode these back into separators or a `..` segment after the check (#764), so a confined URL +/// must not contain them. fn contains_encoded_separator(rest: &str) -> bool { let bytes = rest.as_bytes(); - bytes.windows(3).any(|w| w[0] == b'%' && matches!(&w[1..].to_ascii_lowercase()[..], b"2f" | b"5c")) + bytes.windows(3).any(|w| w[0] == b'%' && matches!(&w[1..].to_ascii_lowercase()[..], b"2f" | b"5c" | b"2e")) } /// Reads `GET /repos//releases/latest` (or one element of `/releases`). @@ -187,6 +199,9 @@ pub fn parse_github_release_list(json: &str, repo: &str, allow_prerelease: bool) Ok(best) } +/// One release object of the GitHub API, with every text capped and only the assets whose name is +/// a plain file name and whose link is exactly [`expected_download_url`]. An API error object +/// (`message`) becomes its text. fn release_from_value(v: &serde_json::Value, repo: &str) -> Result { 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)) }); @@ -194,14 +209,14 @@ fn release_from_value(v: &serde_json::Value, repo: &str) -> Result bool { && name != "." && name != ".." && !name.starts_with('.') + && !contains_encoded_separator(name) && !name.chars().any(|c| c.is_control() || matches!(c, '/' | '\\' | ':' | '<' | '>' | '"' | '|' | '?' | '*')) } @@ -274,11 +290,7 @@ pub(crate) fn truncate(s: &str, max: usize) -> String { if s.len() <= max { return s.to_string(); } - let mut end = max; - while !s.is_char_boundary(end) { - end -= 1; - } - format!("{}…", &s[..end]) + format!("{}…", &s[..s.floor_char_boundary(max)]) } #[cfg(test)] @@ -347,6 +359,29 @@ mod tests { assert_eq!(r.assets[0].size, 123); } + /// #764: an API asset's link must be exactly this release's `/`. + #[test] + fn a_download_link_must_be_its_releases_own() { + for url in [ + // Another release of the same app (a rollback), or another file name. + "https://github.com/storytold/photocraft/releases/download/v0.4.0/photocraft-0.5.0-macos-universal.dmg", + "https://github.com/storytold/photocraft/releases/download/v0.5.0/photocraft-0.4.0-macos-universal.dmg", + "https://github.com/storytold/photocraft/releases/download/%2e%2e/%2E%2E/%2e%2e/%2e%2e/other/repo/releases/download/v1/photocraft-0.5.0-macos-universal.dmg", + ] { + assert!(parse_github_release(&api_json(url), REPO).unwrap().assets.is_empty(), "{url}"); + } + assert_eq!( + expected_download_url(REPO, "v0.6.0-rc.1", "photocraft-0.6.0-rc.1-macos-universal.dmg").as_deref(), + Some("https://github.com/storytold/photocraft/releases/download/v0.6.0-rc.1/photocraft-0.6.0-rc.1-macos-universal.dmg") + ); + for (tag, name) in [("", "x.dmg"), ("..", "x.dmg"), ("v1", ".."), ("v1", "a/b.dmg"), ("v1", "a%2fb.dmg"), ("v 1", "x.dmg"), ("v1", ".x.dmg")] + { + assert_eq!(expected_download_url(REPO, tag, name), None, "{tag:?} {name:?}"); + } + } + + /// Links to other hosts or repositories, or that climb out with `..` or encoded separators, + /// are dropped. #[test] fn drops_downloads_that_point_elsewhere() { for url in ["https://evil.example/photocraft.dmg", "https://github.com/storytold/other/releases/download/v1/x.dmg", "javascript:alert(1)"] { @@ -392,6 +427,9 @@ mod tests { "https://github.com/storytold/photocraft/releases/download/v1/sub/x.dmg", "https://github.com/storytold/photocraft/releases/download/v1%2f..%2f..%2fx.dmg", "https://github.com/storytold/photocraft/releases/download/v1/x%2fy.dmg", + "https://github.com/storytold/photocraft/releases/download/%2e%2e/x.dmg", + "https://github.com/storytold/photocraft/releases/download/%2E%2E/x.dmg", + "https://github.com/storytold/photocraft/releases/download/v1/%2e%2e", ] { let prefix = download_prefix(REPO); assert!(!download_url_under(url, &prefix), "{url}"); @@ -428,11 +466,22 @@ mod tests { assert_eq!(r.version, Some(Version::new(0, 5, 0))); } + /// A name with a path, a hidden or reserved form, a control character or a percent-encoded + /// separator isn't a file name we'd save. #[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}", "%2e%2e%2fother", "a%2Fb", "a%5cb", "x%2E%2E"] { assert!(!valid_file_name(bad), "{bad:?}"); } assert!(valid_file_name("photocraft-0.5.0-linux-x86_64.AppImage")); } + + /// A `SHA256SUMS.txt` line whose name hides a traversal is dropped, so `release_from_sums` + /// never builds a link from it; the ordinary line beside it is kept. + #[test] + fn checksum_names_with_encoded_separators_make_no_download() { + let hash = "a".repeat(64); + let sums = parse_sha256sums(&format!("{hash} %2e%2e%2f%2e%2e%2fother%2frepo%2fx.dmg\n{hash} photocraft-0.5.0-macos-universal.dmg\n")); + assert_eq!(sums.iter().map(|(_, n)| n.as_str()).collect::>(), ["photocraft-0.5.0-macos-universal.dmg"]); + } } diff --git a/crates/update/src/self_update.rs b/crates/update/src/self_update.rs index f22dfb35a..fa3948382 100644 --- a/crates/update/src/self_update.rs +++ b/crates/update/src/self_update.rs @@ -313,6 +313,7 @@ pub fn update( .ok_or_else(|| format!("{} {} has no {} for {}", app.name, release.version_label(), install.kind().label(), platform.label()))?; let asset = asset.clone(); crate::install::bind_artifact_version(app, release, &asset)?; + crate::install::check_download_link(app, release, &asset)?; let mut job = Job { http, paths, stop, phase, install_locations: &Default::default(), replace: false }; let file = install::download(release, &asset, &mut job)?; #[cfg(windows)] @@ -1738,6 +1739,35 @@ mod tests { assert!(result.unwrap_err().contains("is not newer than currently running version")); } + /// storytold/craft-launcher#764: a self-update downloads only its own release's link. One into + /// another app's releases, with the right tag and name, is refused before anything is fetched. + #[test] + fn update_refuses_a_link_that_is_not_its_releases_own() { + let dir = tempfile::tempdir().unwrap(); + let paths = Paths::under(dir.path()); + let http = Http::new(crate::TEST_AGENT).unwrap(); + let mut phase = |_| (); + let last = || Ok(()); + let stop = Stop::default(); + let name = "artcraft-launcher-0.3.0-linux-x86_64.AppImage"; + let release = Release { + version: version::parse("0.3.0"), + tag: "v0.3.0".into(), + assets: vec![crate::release::Asset { + name: name.into(), + size: 1, + url: format!("https://github.com/storytold/photocraft/releases/download/v0.3.0/{name}"), + sha256: None, + }], + ..Release::default() + }; + let install = Install::AppImage(dir.path().join("launcher.AppImage")); + use crate::{Arch, Os}; + let platform = Platform { os: Os::Linux, arch: Arch::X64 }; + let result = update(&crate::suite::LAUNCHER, &install, &release, platform, &http, &paths, &stop, &mut phase, &last, Some("0.2.0")); + assert!(result.unwrap_err().contains("isn't the one its release publishes")); + } + #[cfg(target_os = "linux")] #[test] fn parses_flatpak_metadata_for_restart() {