diff --git a/crates/update/src/install.rs b/crates/update/src/install.rs index 9c3870c6..fae711a9 100644 --- a/crates/update/src/install.rs +++ b/crates/update/src/install.rs @@ -67,6 +67,11 @@ fn not_running(app: &App, answer: Result) -> Result<(), String> { } } +/// [`install`], with `commit` called once a portable ZIP's new files are in place and its version +/// checked, so the caller can save its record; an error from it rolls the previous copy back +/// (Windows only; ignored elsewhere). Refuses before downloading when the app is running, the +/// asset isn't the release's version (`bind_artifact_version`), a newer copy is installed, or the +/// link isn't the release's own (`check_download_link`). #[allow(clippy::too_many_arguments)] pub fn install_at( app: &App, @@ -97,6 +102,7 @@ pub fn install_at( { return Err(format!("Cannot update {} to {}: installed version {} is newer", app.name, release.version_label(), inst_v)); } + check_download_link(app, release, asset)?; log::info!("{}: starting {} to {} ({})", app.name, if existing.is_some() { "update" } else { "install" }, release.version_label(), kind.label()); let file = download(release, asset, job)?; log::info!("{}: download ready ({}), running installer...", app.name, file.display()); @@ -180,13 +186,15 @@ 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) { + // A `+` in either is `%2B` there, as GitHub and `expected_download_url` write it; no other + // escape is decoded, so anything else still has to match as written. + 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(""); + let tag = parts.next().unwrap_or("").replace("%2B", "+"); + let url_filename = parts.next().unwrap_or("").replace("%2B", "+"); if tag.is_empty() || release_version - .map_or(tag != release.tag, |release_version| tag != release.tag && version::parse(tag).as_ref() != Some(release_version)) + .map_or(tag != release.tag, |release_version| tag != release.tag && version::parse(&tag).as_ref() != Some(release_version)) { return Err(mismatch()); } @@ -197,6 +205,29 @@ 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 GitHub download for this release's tag and this asset's name in the release's +/// repository ([`Release::repo`], else the app's own), whichever converter (API, feed or +/// `SHA256SUMS.txt`) built the asset, and so must the link of the release's `SHA256SUMS.txt` that +/// the download is checked against. A `+` may be written plainly or as `%2B` +/// ([`release::same_download_url`]). +/// +/// With this crate's `local-mirror` feature, the developer's loopback mirror is accepted too. The +/// consumer confines those links (the launcher's feed parser does) and must turn the feature on +/// itself (`craft-update/local-mirror`): its own feature of that name doesn't. This crate's tests +/// accept loopback as well, as their downloads come from a server on 127.0.0.1. +pub fn check_download_link(app: &App, release: &Release, asset: &Asset) -> Result<(), String> { + let repo = if release.repo.is_empty() { app.repo } else { release.repo.as_str() }; + let own = |a: &Asset| { + ((cfg!(feature = "local-mirror") || cfg!(test)) && crate::http::is_loopback(&a.url)) + || release::expected_download_url(repo, &release.tag, &a.name).is_some_and(|expected| release::same_download_url(&a.url, &expected)) + }; + if own(asset) && release.sums_asset().is_none_or(own) { + 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( @@ -3292,6 +3323,8 @@ esac assert!(result.unwrap_err().contains("installed version 0.5.0 is newer")); } + /// An asset whose file name, link tag or link file name isn't the release's is refused before + /// the download, and so is one in another app's releases or with a foreign `SHA256SUMS.txt`. #[test] fn install_rejects_an_artifact_whose_version_is_not_the_advertised_one() { // The release advertises 99.0.0, but the selected artifact is an older official build @@ -3337,6 +3370,175 @@ esac ..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()); + + // The checksums the download is verified against must come from the release too. + let mut foreign_sums = release.clone(); + foreign_sums.assets.push(Asset { + name: "SHA256SUMS.txt".into(), + url: "https://github.com/storytold/vectorcraft/releases/download/v99.0.0/SHA256SUMS.txt".into(), + ..Asset::default() + }); + assert!(check_download_link(app, &foreign_sums, &good).is_err()); + + // A release the feed reads from a renamed repository records it, and its links are + // checked against that one; the same links are refused for a release that doesn't. + let renamed = + release::release_from_sums("storytold/photocraft-next", "v0.5.0", &[("a".repeat(64), "photocraft-0.5.0-linux-x86_64.AppImage".into())]); + assert!(check_download_link(app, &renamed, &renamed.assets[0]).is_ok()); + let unrecorded = Release { repo: String::new(), ..renamed.clone() }; + assert!(check_download_link(app, &unrecorded, &unrecorded.assets[0]).is_err()); + + // A `+` written plainly, as an older launcher's feed and saved releases have it, passes. + let plus = release::release_from_sums(app.repo, "v1.0.0+b.1", &[("a".repeat(64), "photocraft-1.0.0+b.1-linux-x86_64.AppImage".into())]); + let mut plain = plus.clone(); + for a in &mut plain.assets { + a.url = a.url.replace("%2B", "+"); + } + assert!(check_download_link(app, &plain, &plain.assets[0]).is_ok()); + } + + /// A release whose tag has a `+` (semver build metadata) has `%2B` in its links. Both checks + /// before the download accept them, so `install_at` gets as far as the download, which the + /// cancelled job ends before anything is fetched. A link with another escape is still refused. + #[test] + fn install_accepts_a_plus_release_up_to_the_download() { + let dir = tempfile::tempdir().unwrap(); + let paths = Paths::under(dir.path()); + let http = Http::new(crate::TEST_AGENT).unwrap(); + let phase = &mut |_| (); + let locations = InstallLocations::default(); + let app = App::by_id("photocraft").unwrap(); + let name = "photocraft-1.0.0+build.1-linux-x86_64.AppImage"; + let release = release::release_from_sums(app.repo, "v1.0.0+build.1", &[("a".repeat(64), name.to_string())]); + let asset = release.assets[0].clone(); + assert!(asset.url.ends_with("/download/v1.0.0%2Bbuild.1/photocraft-1.0.0%2Bbuild.1-linux-x86_64.AppImage"), "{}", asset.url); + assert_eq!(bind_artifact_version(app, &release, &asset), Ok(())); + + let stop = Stop::default(); + stop.cancel(); + let mut job = Job { http: &http, paths: &paths, stop: &stop, phase, install_locations: &locations, replace: false }; + let result = install_at(app, &release, &asset, PackageKind::AppImage, None, None, &mut job, None); + assert_eq!(result.unwrap_err(), crate::protocol::CANCELLED); + + let other_escape = Asset { url: asset.url.replace("%2B", "%2b"), ..asset }; + assert!(bind_artifact_version(app, &release, &other_escape).is_err()); + } + + /// craft-launcher#764: `install_at` refuses, before downloading, an asset whose link is the + /// right path on another host, and a release whose `SHA256SUMS.txt` (what the download is + /// checked against) is linked elsewhere. The stop is cancelled, so if the check stopped working + /// the install would end at the download with `CANCELLED` instead of fetching anything. + #[test] + fn install_refuses_links_off_the_releases_own_before_downloading() { + let dir = tempfile::tempdir().unwrap(); + let paths = Paths::under(dir.path()); + let http = Http::new(crate::TEST_AGENT).unwrap(); + let locations = InstallLocations::default(); + let app = App::by_id("photocraft").unwrap(); + let name = "photocraft-0.5.0-linux-x86_64.AppImage"; + let own = release::release_from_sums(app.repo, "v0.5.0", &[("a".repeat(64), name.to_string())]); + let refuse = |release: &Release, asset: &Asset| { + let stop = Stop::default(); + stop.cancel(); + let phase = &mut |_| (); + let mut job = Job { http: &http, paths: &paths, stop: &stop, phase, install_locations: &locations, replace: false }; + let result = install_at(app, release, asset, PackageKind::AppImage, None, None, &mut job, None); + assert!(result.as_ref().is_err_and(|e| e.contains("isn't the one its release publishes")), "{}: {result:?}", asset.url); + }; + + for url in [ + format!("https://evil.example/storytold/photocraft/releases/download/v0.5.0/{name}"), + format!("http://github.com/storytold/photocraft/releases/download/v0.5.0/{name}"), + format!("https://github.com.evil.example/storytold/photocraft/releases/download/v0.5.0/{name}"), + ] { + refuse(&own, &Asset { url, ..own.assets[0].clone() }); + } + + let mut sums_elsewhere = own.clone(); + let sums = sums_elsewhere.assets.iter_mut().find(|a| a.name == "SHA256SUMS.txt").unwrap(); + sums.url = "https://evil.example/storytold/photocraft/releases/download/v0.5.0/SHA256SUMS.txt".into(); + refuse(&sums_elsewhere, &sums_elsewhere.assets[0]); + } + + /// A download that fails its checksum (twice, as a resumed file gets one fresh try) replaces + /// nothing: the installed copy keeps its bytes, the install never starts, the record isn't + /// committed, and the bad file isn't kept for the next try. + #[test] + fn a_download_that_fails_its_checksum_replaces_nothing() { + use crate::http::tests::{Reply, body, serve}; + let app = App::by_id("photocraft").unwrap(); + let (body, sha) = body(); + let (url, requests) = serve(body.clone(), vec![Reply::Corrupt, Reply::Corrupt]); + let name = "photocraft-0.5.0-linux-x86_64.AppImage"; + let asset = Asset { name: name.into(), size: body.len() as u64, url, sha256: Some(sha) }; + let release = Release { version: version::parse("0.5.0"), tag: "v0.5.0".into(), assets: vec![asset.clone()], ..Release::default() }; + let dir = tempfile::tempdir().unwrap(); + let paths = Paths::under(dir.path()); + let http = Http::new(crate::TEST_AGENT).unwrap(); + let old = dir.path().join("Apps").join("PhotoCraft.AppImage"); + std::fs::create_dir_all(old.parent().unwrap()).unwrap(); + std::fs::write(&old, b"the installed 0.4.0").unwrap(); + let installed = + Installed { version: version::parse("0.4.0"), source: InstallSource::Launcher, location: old.display().to_string(), package: None }; + let previous = record("0.4.0", PackageKind::AppImage, old.clone(), vec![], false); + let mut phases = Vec::new(); + let mut phase = |p: JobPhase| phases.push(p); + let locations = InstallLocations::default(); + let stop = Stop::default(); + let mut job = Job { http: &http, paths: &paths, stop: &stop, phase: &mut phase, install_locations: &locations, replace: false }; + let mut committed = false; + let mut commit = |_: &Record| { + committed = true; + Ok(()) + }; + let result = install_at(app, &release, &asset, PackageKind::AppImage, Some(&installed), Some(&previous), &mut job, Some(&mut commit)); + assert!(result.as_ref().is_err_and(|e| e.contains("checksum")), "{result:?}"); + assert_eq!(std::fs::read(&old).unwrap(), b"the installed 0.4.0"); + assert!(!phases.contains(&JobPhase::Installing), "{phases:?}"); + assert!(!committed); + assert!(!download_path(&paths, &asset).exists()); + assert_eq!(requests.lock().unwrap().len(), 2); + } + + /// A running installed copy isn't replaced; the stop is cancelled in case the check stops + /// working. Windows finds a process by its file name, so the running test binary stands in for + /// the copy. Elsewhere `pgrep -f` matches the command line, so a probe runs with the copy's path. + #[test] + fn install_refuses_while_the_installed_copy_is_running() { + let dir = tempfile::tempdir().unwrap(); + let paths = Paths::under(dir.path()); + let http = Http::new(crate::TEST_AGENT).unwrap(); + let phase = &mut |_| (); + let locations = InstallLocations::default(); + let app = App::by_id("photocraft").unwrap(); + let name = "photocraft-0.5.0-linux-x86_64.AppImage"; + let release = release::release_from_sums(app.repo, "v0.5.0", &[("a".repeat(64), name.to_string())]); + #[cfg(windows)] + let location = std::env::current_exe().unwrap().display().to_string(); + #[cfg(unix)] + let location = dir.path().join("PhotoCraft.AppImage").display().to_string(); + #[cfg(unix)] + let _probe = crate::process::tests::RunningProbe::start(&location); + assert!(process::is_running(Path::new(&location)), "{location} should be seen as running"); + let installed = Installed { version: version::parse("0.4.0"), source: InstallSource::Launcher, location, package: None }; + let stop = Stop::default(); + stop.cancel(); + let mut job = Job { http: &http, paths: &paths, stop: &stop, phase, install_locations: &locations, replace: false }; + let result = install_at(app, &release, &release.assets[0], PackageKind::AppImage, Some(&installed), None, &mut job, None); + assert!(result.as_ref().is_err_and(|e| e.contains("is running")), "{result:?}"); } #[cfg(target_os = "linux")] diff --git a/crates/update/src/process.rs b/crates/update/src/process.rs index 1793ba63..13ed8dda 100644 --- a/crates/update/src/process.rs +++ b/crates/update/src/process.rs @@ -488,16 +488,17 @@ fn pgrep_pattern(path: &str) -> String { #[cfg(test)] #[cfg(unix)] -mod tests { +pub(crate) mod tests { use std::io::{BufRead, BufReader}; use std::process::Child; use super::*; - struct RunningProbe(Child); + /// A process whose command line is `path`, as `pgrep -f` sees it, until dropped. + pub(crate) struct RunningProbe(Child); impl RunningProbe { - fn start(path: &str) -> Self { + pub(crate) fn start(path: &str) -> Self { let child = command("/bin/sh") .args(["-c", "printf 'ready\\n'; read line", path]) .stdin(Stdio::piped()) diff --git a/crates/update/src/release.rs b/crates/update/src/release.rs index ef725f48..d0a74ef7 100644 --- a/crates/update/src/release.rs +++ b/crates/update/src/release.rs @@ -35,6 +35,12 @@ pub struct Release { pub notes: String, pub prerelease: bool, pub assets: Vec, + /// The `owner/name` repository the release was read from, whose download links its assets + /// must be (`install::check_download_link`). Usually the app's own; a feed that announces a + /// renamed repository reads the release from the new one. Empty means the app's own (a + /// release saved before this field, or one built by hand). + #[serde(default)] + pub repo: String, } impl Release { @@ -117,6 +123,25 @@ 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` unless the tag is a [`valid_tag`] and the name a +/// [`valid_file_name`]: only ASCII letters, digits and `._+-`, which github.com can't re-interpret. +/// A `+` (semver build metadata) is written `%2B`, as GitHub writes it in `browser_download_url` +/// (`.../download/17.0.9%2B9/jdk-17.0.9%2B9.deb` for the tag `17.0.9+9`). +pub fn expected_download_url(repo: &str, tag: &str, name: &str) -> Option { + let encoded = |s: &str| s.replace('+', "%2B"); + (valid_tag(tag) && valid_file_name(name)).then(|| format!("{}{}/{}", download_prefix(repo), encoded(tag), encoded(name))) +} + +/// Whether `url` is `expected`, an [`expected_download_url`], with each `%2B` in either read as +/// `+`. GitHub serves a `+` in a download path the same either way, and links built before +/// `%2B` (an older launcher's feed, or a release saved by an older version) write it plainly. +/// No other escape is read, so `%2b` or `%2e` still has to match as written. +pub fn same_download_url(url: &str, expected: &str) -> bool { + url == expected || url.replace("%2B", "+") == expected.replace("%2B", "+") +} + /// Whether `url` is a download that truly belongs to `repo`. /// /// A raw `starts_with(prefix)` check is not enough: a value such as @@ -145,7 +170,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. @@ -153,11 +178,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`). @@ -189,21 +215,24 @@ 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)) }); } - 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); let assets = v["assets"] .as_array() .map(|list| { list.iter() .filter_map(|a| { let name = a["name"].as_str().filter(|n| valid_file_name(n))?.to_string(); - let url = a["browser_download_url"].as_str().filter(|u| download_url_under(u, &prefix))?.to_string(); + let url = + a["browser_download_url"].as_str().filter(|u| expected_download_url(repo, &tag, &name).as_deref() == Some(*u))?.to_string(); let sha256 = a["digest"].as_str().and_then(|d| d.strip_prefix("sha256:")).filter(|h| is_hex64(h)).map(str::to_ascii_lowercase); Some(Asset { name, size: a["size"].as_u64().unwrap_or(0), url, sha256 }) }) @@ -219,18 +248,18 @@ fn release_from_value(v: &serde_json::Value, repo: &str) -> Result Release { - let prefix = download_prefix(repo); - let mut assets: Vec = sums - .iter() - .map(|(hash, name)| Asset { name: name.clone(), size: 0, url: format!("{prefix}{tag}/{name}"), sha256: Some(hash.clone()) }) - .collect(); - assets.push(Asset { name: "SHA256SUMS.txt".into(), size: 0, url: format!("{prefix}{tag}/SHA256SUMS.txt"), sha256: None }); + let asset = + |name: &str, sha256: Option| expected_download_url(repo, tag, name).map(|url| Asset { name: name.to_string(), size: 0, url, sha256 }); + let assets: Vec = sums.iter().filter_map(|(hash, name)| asset(name, Some(hash.clone()))).chain(asset("SHA256SUMS.txt", None)).collect(); Release { tag: tag.to_string(), version: version::parse(tag), @@ -240,13 +269,25 @@ pub fn release_from_sums(repo: &str, tag: &str, sums: &[(String, String)]) -> Re notes: String::new(), prerelease: false, assets, + repo: repo.to_string(), } } /// 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, one entry per name. @@ -281,14 +322,19 @@ pub fn parse_sha256sums(text: &str) -> Vec<(String, String)> { sums } -/// 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._+-]` (so no path, escape, control +/// or bidi character), 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) +} + +/// Whether `name`'s part before the first dot is a Windows device name, which opens the device +/// instead of a file in any folder. +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 @@ -297,11 +343,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)] @@ -370,6 +412,79 @@ 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", + // The right path on another host, over plain http, or behind a look-alike host. + "https://evil.example/storytold/photocraft/releases/download/v0.5.0/photocraft-0.5.0-macos-universal.dmg", + "http://github.com/storytold/photocraft/releases/download/v0.5.0/photocraft-0.5.0-macos-universal.dmg", + "https://github.com.evil.example/storytold/photocraft/releases/download/v0.5.0/photocraft-0.5.0-macos-universal.dmg", + "https://github.com@evil.example/storytold/photocraft/releases/download/v0.5.0/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:?}"); + } + } + + /// Two entries for one asset name, one with the release's own link and one elsewhere: only the + /// release's own survives, in either order, so a later lookup by name can't pick the other. + /// Built from `SHA256SUMS.txt`, every entry for a repeated name gets that same single link. + #[test] + fn two_links_for_one_asset_keep_only_the_releases_own() { + let name = "photocraft-0.5.0-macos-universal.dmg"; + let own = format!("https://github.com/storytold/photocraft/releases/download/v0.5.0/{name}"); + let asset = |url: &str| format!(r#"{{"name":"{name}","size":123,"browser_download_url":"{url}","digest":"sha256:{HASH}"}}"#); + let elsewhere = "https://evil.example/storytold/photocraft/releases/download/v0.5.0/photocraft-0.5.0-macos-universal.dmg"; + for (first, second) in [(elsewhere, own.as_str()), (own.as_str(), elsewhere)] { + let json = format!( + r#"{{"tag_name":"v0.5.0","name":"PhotoCraft v0.5.0","html_url":"https://github.com/storytold/photocraft/releases/tag/v0.5.0", + "published_at":"2026-10-08T14:52:50Z","prerelease":false,"body":"Notes","assets":[{},{}]}}"#, + asset(first), + asset(second) + ); + let r = parse_github_release(&json, REPO).unwrap(); + assert_eq!(r.assets.len(), 1, "{first} {second}"); + assert_eq!(r.assets[0].url, own); + } + + let r = release_from_sums(REPO, "v0.5.0", &[("a".repeat(64), name.to_string()), ("b".repeat(64), name.to_string())]); + let links: Vec<&str> = r.assets.iter().filter(|a| a.name == name).map(|a| a.url.as_str()).collect(); + assert!(!links.is_empty() && links.iter().all(|url| *url == own), "{links:?}"); + } + + /// A `+` in the tag or name (semver build metadata) is `%2B` in GitHub's download links, but a + /// plain `+` in the asset's name and in the `releases/latest` redirect, as on bell-sw/Liberica's + /// releases. The API's assets, and those built from `SHA256SUMS.txt`, get the same link. + #[test] + fn a_plus_in_the_tag_is_percent_encoded_in_the_link() { + let name = "photocraft-1.0.0+build.1-linux-x86_64.AppImage"; + let github = "https://github.com/storytold/photocraft/releases/download/v1.0.0%2Bbuild.1/photocraft-1.0.0%2Bbuild.1-linux-x86_64.AppImage"; + let api = |url: &str| format!(r#"{{"tag_name":"v1.0.0+build.1","assets":[{{"name":"{name}","size":1,"browser_download_url":"{url}"}}]}}"#); + let r = parse_github_release(&api(github), REPO).unwrap(); + assert_eq!((r.assets[0].name.as_str(), r.assets[0].url.as_str()), (name, github)); + // GitHub never writes the `+` itself into the link. + assert!(parse_github_release(&api(&github.replace("%2B", "+")), REPO).unwrap().assets.is_empty()); + + let tag = tag_from_redirect("https://github.com/storytold/photocraft/releases/tag/v1.0.0+build.1", REPO).unwrap(); + let r = release_from_sums(REPO, &tag, &[(HASH.to_string(), name.to_string())]); + assert_eq!(r.assets[0].url, github); + assert_eq!(r.sums_asset().unwrap().url, "https://github.com/storytold/photocraft/releases/download/v1.0.0%2Bbuild.1/SHA256SUMS.txt"); + } + + /// 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)"] { @@ -405,6 +520,8 @@ mod tests { } } + /// A link that shares the download prefix as text but climbs into another repository with `..` + /// or an encoded separator or dot isn't under it. #[test] fn traversal_cannot_escape_the_repository() { // Raw-prefix matches but the interpreted path climbs into another repository (CWE-180). @@ -415,6 +532,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}"); @@ -439,6 +559,8 @@ mod tests { assert_eq!(parse_github_release_list("[]", REPO, false).unwrap(), None); } + /// `SHA256SUMS.txt` lines, the tag in a `releases/latest` redirect, and the release built from + /// both; malformed lines, traversing names and `.`/`..` tags are dropped. #[test] fn reads_checksums_and_redirects() { let sums = parse_sha256sums(&format!("{HASH} photocraft-0.5.0-macos-universal.dmg\n{HASH} *b.zip\nnot a line\n{HASH} ../escape\n")); @@ -446,6 +568,9 @@ mod tests { assert_eq!(tag_from_redirect("https://github.com/storytold/photocraft/releases/tag/v0.5.0", REPO).as_deref(), Some("v0.5.0")); assert_eq!(tag_from_redirect("https://github.com/storytold/photocraft/releases", REPO), None); assert_eq!(tag_from_redirect("https://evil/storytold/photocraft/releases/tag/v1", REPO), None); + for dots in [".", ".."] { + assert_eq!(tag_from_redirect(&format!("https://github.com/storytold/photocraft/releases/tag/{dots}"), REPO), None); + } let r = release_from_sums(REPO, "v0.5.0", &sums); assert_eq!(r.assets[0].url, "https://github.com/storytold/photocraft/releases/download/v0.5.0/photocraft-0.5.0-macos-universal.dmg"); assert_eq!(r.version, Some(Version::new(0, 5, 0))); @@ -484,11 +609,92 @@ mod tests { assert!(sums.iter().enumerate().all(|(i, (h, n))| *h == HASH && *n == format!("f{}", i + 1))); } + /// 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", + "a b.dmg", + "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:?}"); + } + } + + /// The tag rule the launcher's feed shares (craft-launcher#1024): plain characters, no + /// leading dot, at most 64 bytes, for the API's `tag_name`, a redirect and a download link. + #[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); + } + + /// A `+` matches written plainly or as `%2B`, either side; no other escape is read. + #[test] + fn a_plus_link_matches_either_way_and_nothing_else_does() { + let expected = expected_download_url(REPO, "v1.0.0+b.1", "photocraft-1.0.0+b.1-linux-x86_64.AppImage").unwrap(); + assert!(same_download_url(&expected, &expected)); + assert!(same_download_url(&expected.replace("%2B", "+"), &expected)); + assert!(!same_download_url(&expected.replace("%2B", "%2b"), &expected)); + assert!(!same_download_url(&expected.replace("v1.0.0", "v1.0.1"), &expected)); + } + + /// Both converters record the repository they read the release from. + #[test] + fn a_release_knows_its_repository() { + assert_eq!(release_from_sums(REPO, "v0.5.0", &[]).repo, REPO); + let api = r#"{"tag_name": "v0.5.0", "assets": []}"#; + assert_eq!(parse_github_release(api, REPO).unwrap().repo, REPO); + } + + /// 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 4fe2175b..780590db 100644 --- a/crates/update/src/self_update.rs +++ b/crates/update/src/self_update.rs @@ -575,6 +575,11 @@ fn exe_after_msi(app: &App, registered: bool, exe: &Path) -> PathBuf { .unwrap_or_else(|| exe.to_path_buf()) } +/// How the running copy at `exe` was installed on Linux, or `None` when it isn't this app's to +/// update: a Flatpak release bundle (per user or system-wide), an AppImage, the deb/rpm/Arch +/// package at `/usr/bin/`, or a standalone copy outside the package managers' folders +/// ([`linux_location`]). A `/usr/bin/` that none of those package databases claims is also +/// `None`: it sits in a system folder, so it isn't treated as a standalone copy. #[cfg(target_os = "linux")] fn detect(app: &App, exe: &Path) -> Option { if process::in_sandbox() { @@ -589,6 +594,11 @@ fn detect(app: &App, exe: &Path) -> Option { if let Some(appimage) = std::env::var_os("APPIMAGE") { return Some(Install::AppImage(PathBuf::from(appimage))); } + match linux_location(app, exe) { + LinuxLocation::PackagePath => {} + LinuxLocation::System => return None, + LinuxLocation::Elsewhere => return exe.is_file().then(|| Install::Standalone(exe.to_path_buf())), + } let t = std::time::Duration::from_secs(10); if process::capture("dpkg-query", &["-W", "-f", "${db:Status-Status}", app.id], t).is_some_and(|s| s.trim() == "installed") { return Some(Install::Package(PackageKind::Deb, exe.to_path_buf())); @@ -601,12 +611,47 @@ fn detect(app: &App, exe: &Path) -> Option { if crate::detect::linux::has_pacman_db() && process::capture("pacman", &["-Q", &app.bundle_id()], t).is_some() { return Some(Install::Package(PackageKind::ArchPkg, exe.to_path_buf())); } - if exe.is_file() { - return Some(Install::Standalone(exe.to_path_buf())); - } + // `/usr/bin/` that no package database claims, e.g. copied there by hand or put there by a + // package whose name isn't the app's id. Replacing it would write over a system folder without + // a package to do it, so it doesn't update itself. + log::info!("{}: {} belongs to no deb, rpm or Arch package, so it doesn't update itself", app.name, exe.display()); None } +/// Where a Linux executable outside a Flatpak or AppImage runs from, for [`detect`]. +#[cfg_attr(not(target_os = "linux"), allow(dead_code))] +#[derive(Debug, PartialEq, Eq)] +enum LinuxLocation { + /// `/usr/bin/`, where the release's deb, rpm and Arch packages put it. Only this path is + /// checked against the package databases: a copy elsewhere isn't the package's even when the + /// package is installed too. + PackagePath, + /// Anywhere else under a folder a package manager owns (`/usr` but not `/usr/local`, `/opt`, + /// `/bin`, `/sbin`, `/nix`, `/snap` or `/var/lib/snapd/snap` (Snap on Fedora and Arch), Guix's + /// `/gnu`, or a Homebrew `.linuxbrew` prefix): a + /// distribution, AUR, Nix, Snap, Guix or Homebrew copy. It's theirs to update, and replacing + /// it in place would write over a file the package manager owns (or fail on a read-only + /// store), as on macOS ([`macos_package_manager_owns`]). + System, + /// Anywhere else, such as a folder in the user's home: a standalone copy that replaces itself. + Elsewhere, +} + +/// Which [`LinuxLocation`] `exe` is in, from its path alone. +#[cfg_attr(not(target_os = "linux"), allow(dead_code))] +fn linux_location(app: &App, exe: &Path) -> LinuxLocation { + if exe == Path::new("/usr/bin").join(app.id) { + LinuxLocation::PackagePath + } else if (!exe.starts_with("/usr/local") && ["/usr", "/opt", "/bin", "/sbin", "/nix", "/snap", "/var/lib/snapd/snap", "/gnu"].iter().any(|dir| exe.starts_with(dir))) + // Homebrew on Linux: /home/linuxbrew/.linuxbrew, or ~/.linuxbrew for one user + || exe.components().any(|c| c.as_os_str() == ".linuxbrew") + { + LinuxLocation::System + } else { + LinuxLocation::Elsewhere + } +} + /// Where this sandbox's Flatpak copy is installed, from `app-path` in `/.flatpak-info`. #[cfg(target_os = "linux")] #[derive(Clone, Copy, Debug, PartialEq, Eq)] @@ -750,6 +795,7 @@ pub fn prepare_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)?; // Nothing is downloaded for a copy that can't install it. #[cfg(windows)] windows_precheck(app, install, paths)?; @@ -4233,6 +4279,89 @@ mod tests { assert!(result.unwrap_err().contains("is not newer than currently running version")); } + /// Only `/usr/bin/` is checked against the package databases, as before the launcher's + /// self-update moved here; another copy under a system folder (an AUR build, say) is left to + /// its package manager, and only a copy outside them (`/usr/local` included, which no package + /// manager owns) replaces itself. + #[test] + fn only_the_packages_own_path_is_a_package_and_system_folders_are_left_alone() { + let app = &crate::suite::LAUNCHER; + assert_eq!(linux_location(app, Path::new("/usr/bin/artcraft-launcher")), LinuxLocation::PackagePath); + for system in [ + "/usr/bin/artcraft-launcher-bin", + "/nix/store/abc-artcraft-launcher/bin/artcraft-launcher", + "/opt/artcraft-launcher/artcraft-launcher", + "/bin/artcraft-launcher", + "/gnu/store/abc-artcraft-launcher/bin/artcraft-launcher", + "/snap/artcraft-launcher/12/bin/artcraft-launcher", + "/var/lib/snapd/snap/artcraft-launcher/12/bin/artcraft-launcher", + "/home/linuxbrew/.linuxbrew/Cellar/artcraft-launcher/1.0/bin/artcraft-launcher", + "/home/me/.linuxbrew/bin/artcraft-launcher", + ] { + assert_eq!(linux_location(app, Path::new(system)), LinuxLocation::System, "{system}"); + } + for own in [ + "/usr/local/bin/artcraft-launcher", + "/home/me/bin/artcraft-launcher", + "/home/me/.local/bin/artcraft-launcher", + "/usrlocal/artcraft-launcher", + "/var/lib/snapdx/artcraft-launcher", + ] { + assert_eq!(linux_location(app, Path::new(own)), LinuxLocation::Elsewhere, "{own}"); + } + } + + /// 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")); + } + + /// A launcher release whose tag has a `+` (semver build metadata) has `%2B` in its links. + /// The self-update accepts them and gets as far as the download, which the cancelled stop ends + /// before anything is fetched. + #[test] + fn update_accepts_a_plus_release_up_to_the_download() { + 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(); + stop.cancel(); + let app = &crate::suite::LAUNCHER; + let name = "artcraft-launcher-0.3.0+build.1-linux-x86_64.AppImage"; + let release = crate::release::release_from_sums(app.repo, "v0.3.0+build.1", &[("a".repeat(64), name.to_string())]); + 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(app, &install, &release, platform, &http, &paths, &stop, &mut phase, &last, Some("0.2.0")); + assert_eq!(result.unwrap_err(), crate::protocol::CANCELLED); + } + #[cfg(target_os = "linux")] #[test] fn parses_flatpak_metadata_for_restart() { diff --git a/crates/update/src/source.rs b/crates/update/src/source.rs index 75a46fd2..076b6c63 100644 --- a/crates/update/src/source.rs +++ b/crates/update/src/source.rs @@ -130,7 +130,8 @@ impl GitHubSource { { return Ok(Answer { latest: LatestRelease::Found(r.clone()), source: Source::GitHubPages }); } - let sums_url = format!("{}{tag}/SHA256SUMS.txt", release::download_prefix(repo)); + // The same link `release_from_sums` stores and the check before the download accepts. + let sums_url = release::expected_download_url(repo, &tag, "SHA256SUMS.txt").ok_or("the latest release's tag isn't one we'd fetch")?; let sums = self.http.get_text(&sums_url)?.map(|t| release::parse_sha256sums(&t)).unwrap_or_default(); let r = release::release_from_sums(repo, &tag, &sums); self.store(repo, Entry { etag: None, release: Some(r.clone()), prereleases: false, fetched_at: crate::now() });