From 1f55cfc66aca7aa0efc5d2f7332231c0007f97bb Mon Sep 17 00:00:00 2001 From: smittie2000 Date: Tue, 27 Oct 2026 10:25:43 +0200 Subject: [PATCH] craft-update: check the checksums' link too; no `.` tag from a redirect; /usr/local self-updates Review follow-ups on the two commits before: - check_download_link also requires the release's SHA256SUMS.txt to be its own link: the download is verified against it, and a feed- or cache-built release only had the prefix check. - tag_from_redirect refuses `.` and `..`, which its character list let through and which source.rs would then put in the SHA256SUMS.txt link. - linux_location: /usr/local belongs to the user, not a package manager, so a copy there keeps updating itself; Nix and Snap stores are left alone like /usr and /opt. Co-Authored-By: Claude Opus 5.5 --- crates/update/src/install.rs | 23 ++++++++++++++++++----- crates/update/src/release.rs | 6 +++++- crates/update/src/self_update.rs | 21 ++++++++++++++------- 3 files changed, 37 insertions(+), 13 deletions(-) diff --git a/crates/update/src/install.rs b/crates/update/src/install.rs index 3686058ef..51b445a92 100644 --- a/crates/update/src/install.rs +++ b/crates/update/src/install.rs @@ -190,12 +190,16 @@ pub(crate) fn bind_artifact_version(app: &App, release: &Release, asset: &Asset) /// 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. +/// 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 `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()) { + let own = |a: &Asset| { + ((cfg!(feature = "local-mirror") || cfg!(test)) && crate::http::is_loopback(&a.url)) + || release::expected_download_url(app.repo, &release.tag, &a.name).as_deref() == Some(a.url.as_str()) + }; + 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)) @@ -2667,6 +2671,15 @@ mod tests { 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()); } #[cfg(target_os = "linux")] diff --git a/crates/update/src/release.rs b/crates/update/src/release.rs index b0c545208..f63cf8123 100644 --- a/crates/update/src/release.rs +++ b/crates/update/src/release.rs @@ -259,7 +259,8 @@ pub fn release_from_sums(repo: &str, tag: &str, sums: &[(String, String)]) -> 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()) + (!rest.is_empty() && rest != "." && rest != ".." && rest.len() <= 64 && rest.bytes().all(|b| b.is_ascii_alphanumeric() || b"._-+".contains(&b))) + .then(|| rest.to_string()) } /// Reads `sha256sum` output: ` ` (or ` *`) per line. @@ -461,6 +462,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))); diff --git a/crates/update/src/self_update.rs b/crates/update/src/self_update.rs index b8a7b84f6..87363d0f4 100644 --- a/crates/update/src/self_update.rs +++ b/crates/update/src/self_update.rs @@ -254,9 +254,10 @@ enum LinuxLocation { /// 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 the system's package manager owns (`/usr`, `/opt`, `/bin`, - /// `/sbin`): a distribution or AUR package's copy, or one a system administrator put there. - /// It's theirs to update, and replacing it in place would write over a file pacman or dpkg owns. + /// Anywhere else under a folder a package manager owns (`/usr` but not `/usr/local`, `/opt`, + /// `/bin`, `/sbin`, `/nix`, `/snap`): a distribution, AUR, Nix or Snap 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). System, /// Anywhere else, such as a folder in the user's home: a standalone copy that replaces itself. Elsewhere, @@ -266,7 +267,7 @@ enum LinuxLocation { fn linux_location(app: &App, exe: &Path) -> LinuxLocation { if exe == Path::new("/usr/bin").join(app.id) { LinuxLocation::PackagePath - } else if ["/usr", "/opt", "/bin", "/sbin"].iter().any(|dir| exe.starts_with(dir)) { + } else if !exe.starts_with("/usr/local") && ["/usr", "/opt", "/bin", "/sbin", "/nix", "/snap"].iter().any(|dir| exe.starts_with(dir)) { LinuxLocation::System } else { LinuxLocation::Elsewhere @@ -1770,20 +1771,26 @@ mod tests { /// 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 replaces itself. + /// 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", - "/usr/local/bin/artcraft-launcher", + "/nix/store/abc-artcraft-launcher/bin/artcraft-launcher", "/opt/artcraft-launcher/artcraft-launcher", "/bin/artcraft-launcher", ] { assert_eq!(linux_location(app, Path::new(system)), LinuxLocation::System, "{system}"); } - for own in ["/home/me/bin/artcraft-launcher", "/home/me/.local/bin/artcraft-launcher", "/usrlocal/artcraft-launcher"] { + for own in [ + "/usr/local/bin/artcraft-launcher", + "/home/me/bin/artcraft-launcher", + "/home/me/.local/bin/artcraft-launcher", + "/usrlocal/artcraft-launcher", + ] { assert_eq!(linux_location(app, Path::new(own)), LinuxLocation::Elsewhere, "{own}"); } }