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 <noreply@anthropic.com>
This commit is contained in:
smittie2000andClaude Opus 5.5 committed 2026-10-27 10:25:43 +02:00
1 parent 6d0982eb05
commit 1f55cfc66a
3 files changed
+37 -13

No files matched your search

+18 -5
View File
@@ -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")]
+5 -1
View File
@@ -259,7 +259,8 @@ pub fn release_from_sums(repo: &str, tag: &str, sums: &[(String, String)]) -> Re
/// The tag in a `https://github.com/<repo>/releases/tag/<tag>` redirect target.
pub fn tag_from_redirect(location: &str, repo: &str) -> Option<String> {
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: `<hex> <name>` (or `<hex> *<name>`) 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)));
+14 -7
View File
@@ -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/<id>` 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}");
}
}