From 0ccdb11e6e8f5120c7b3134633b15f08b85649c7 Mon Sep 17 00:00:00 2001 From: MotherSphere Date: Thu, 27 Aug 2026 15:59:22 +0200 Subject: [PATCH 01/18] fix(download): use a read timeout instead of a 300 s total deadline reqwest's ClientBuilder::timeout is a total deadline covering connect, TLS, redirects and the entire body stream. At 300 s that made any asset larger than the link could carry in five minutes impossible to fetch: a 40 MB binary needed a sustained ~1.2 Mbit/s or it failed every single time, and since nothing is resumed, each attempt spent a full asset's worth of quota for no retained progress. The same client backs the launcher self-update, so Colony could not update itself on such a line either. read_timeout bounds the only thing worth bounding - a connection that has stopped delivering bytes. A stalled socket still dies in 60 s; a slow but live one now runs to completion. --- src/download.rs | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/src/download.rs b/src/download.rs index 04f384d..41667d5 100644 --- a/src/download.rs +++ b/src/download.rs @@ -12,12 +12,23 @@ use std::time::Duration; use crate::github::{APP_VERSION, CONNECT_TIMEOUT, GITHUB_ACCOUNT, LAUNCHER_OWNER, LAUNCHER_REPO}; use crate::persistence::colony_data_dir; -/// Build the HTTP client used for large asset downloads (longer read timeout -/// than the API client). +/// How long a download may stall before we give up. This is an *inactivity* +/// budget, not a total one: a slow-but-alive link keeps its transfer, a dead +/// socket still dies promptly. +const DOWNLOAD_READ_TIMEOUT: Duration = Duration::from_secs(60); + +/// Build the HTTP client used for large asset downloads. +/// +/// Deliberately no `.timeout()`: that is a *total* deadline covering connect, +/// TLS, redirects and the whole body stream, so a 300 s cap made any asset +/// larger than the line could carry in five minutes impossible to fetch at all +/// (a 40 MB binary needed a sustained ~1.2 Mbit/s or it could never finish, on +/// every attempt, with no partial progress kept). A read timeout bounds the +/// only thing worth bounding - a connection that has stopped delivering bytes. fn download_client() -> Result { Ok(reqwest::Client::builder() .user_agent(format!("Colony-Launcher/{APP_VERSION}")) - .timeout(Duration::from_secs(300)) + .read_timeout(DOWNLOAD_READ_TIMEOUT) .connect_timeout(CONNECT_TIMEOUT) .build()?) } From 318f8b7a487f52b2fab7185e19168fbfd1df5816 Mon Sep 17 00:00:00 2001 From: MotherSphere Date: Thu, 27 Aug 2026 15:59:32 +0200 Subject: [PATCH 02/18] fix(update): never report apps as up to date when the check did not run check_update_available returned Option, collapsing 'no update' and 'the check could not run' into the same None. updates_checked then REPLACED available_updates wholesale and wrote the all-clear status line, so going offline - or simply hitting the anonymous rate limit on a second launch within the hour - cleared every update badge and affirmatively told the user their apps were current. No toast, no log, no way to notice. The correct invariant already exists one file over: check_launcher_update returns Result and update/launcher.rs refuses to claim 'up to date' on a failed check, with a regression test. This gives the app-side check the same contract: - check_update_available returns Result> ('not installed' stays a legitimate Ok(None), a failed tag resolution is now an Err) - UpdatesChecked carries one outcome per repo, so a partial failure keeps the results that did come back - updates_checked MERGES: a repo whose check failed keeps the badge it had, a repo that checked clean loses it, and any failure suppresses the all-clear line in favour of a warning naming the count Also chains the update queue through download_release's two silent early returns. A repo that left the catalog mid-run (a refresh replaces it wholesale) stranded every remaining entry, which a later unrelated install would then silently drain into an unannounced cascade of downloads. --- src/github/releases.rs | 28 ++++++++------- src/i18n/en.rs | 8 +++++ src/i18n/fr.rs | 8 +++++ src/message.rs | 5 ++- src/update/mod.rs | 69 ++++++++++++++++++++++++++++++++++++ src/update/store.rs | 79 +++++++++++++++++++++++++++++++++--------- 6 files changed, 167 insertions(+), 30 deletions(-) diff --git a/src/github/releases.rs b/src/github/releases.rs index 781c9f2..dd226c5 100644 --- a/src/github/releases.rs +++ b/src/github/releases.rs @@ -267,8 +267,13 @@ pub fn parse_version_tag(tag: &str) -> Option { } /// Check if an update is available for a repo whose manifest pins `pinned_tag` -/// for the current platform. Returns Some(target_tag) if the installed version -/// differs from what the manifest would install, None otherwise. +/// for the current platform. +/// +/// `Ok(Some(tag))` = an update to `tag` is available. `Ok(None)` = the check +/// RAN and there is nothing to install (either the app is not installed at all, +/// or it is current). `Err` = the check could NOT run, and the caller must not +/// turn that into "up to date" — the same fail-loud contract +/// [`check_launcher_update`] already keeps for the launcher's own check. /// /// `pinned_tag` is compared directly unless it is "latest", in which case the /// repo's latest release is resolved. This avoids a perpetual "update @@ -279,11 +284,14 @@ pub async fn check_update_available( client: &reqwest::Client, repo_name: &str, pinned_tag: &str, -) -> Option { - let installed = load_installed_version(repo_name)?; +) -> Result> { + // Not installed is a real answer, not a failure: there is nothing to update. + let Some(installed) = load_installed_version(repo_name) else { + return Ok(None); + }; let target = if pinned_tag.eq_ignore_ascii_case("latest") { - fetch_latest_release_tag(client, repo_name).await.ok()? + fetch_latest_release_tag(client, repo_name).await? } else { pinned_tag.to_string() }; @@ -291,22 +299,18 @@ pub async fn check_update_available( // Case-insensitive: "Nightly" vs "nightly" must not read as an update // (with non-semver tags the string fallback below would flag it forever). if target.eq_ignore_ascii_case(&installed) { - return None; + return Ok(None); } match (parse_version_tag(&installed), parse_version_tag(&target)) { (Some(installed_ver), Some(target_ver)) => { - if target_ver > installed_ver { - Some(target) - } else { - None - } + Ok((target_ver > installed_ver).then_some(target)) } _ => { tracing::warn!( "Non-semver tags for {repo_name} (installed '{installed}', target '{target}'); using string comparison" ); - Some(target) + Ok(Some(target)) } } } diff --git a/src/i18n/en.rs b/src/i18n/en.rs index b6efead..89b2df0 100644 --- a/src/i18n/en.rs +++ b/src/i18n/en.rs @@ -101,6 +101,14 @@ pub(super) fn insert_all(strings: &mut HashMap) { "updates_available".into(), "{count} update(s) available: {names}".into(), ); + strings.insert( + "update_skipped".into(), + "Skipped {name}: no longer in the catalog, or no build for this platform".into(), + ); + strings.insert( + "update_check_failed".into(), + "Could not check {count} app(s) for updates — they may not be up to date".into(), + ); // Sidebar section names (localized) strings.insert("section_all".into(), "All".into()); diff --git a/src/i18n/fr.rs b/src/i18n/fr.rs index 85263fe..2d41fb9 100644 --- a/src/i18n/fr.rs +++ b/src/i18n/fr.rs @@ -115,6 +115,14 @@ pub(super) fn insert_all(strings: &mut HashMap) { "updates_available".into(), "{count} mise(s) à jour disponible(s) : {names}".into(), ); + strings.insert( + "update_skipped".into(), + "{name} ignorée : absente du catalogue, ou aucune version pour cette plateforme".into(), + ); + strings.insert( + "update_check_failed".into(), + "Impossible de vérifier {count} application(s) — elles ne sont peut-être pas à jour".into(), + ); // Sidebar section names (localized) strings.insert("section_all".into(), "Tout".into()); diff --git a/src/message.rs b/src/message.rs index b57d07b..b50833b 100644 --- a/src/message.rs +++ b/src/message.rs @@ -49,7 +49,10 @@ pub enum Message { AnimationTick, KeyboardEvent(keyboard::Event), CheckUpdates, - UpdatesChecked(Vec<(String, String)>), // Vec<(repo_name, latest_tag)> + /// One entry per checked repo. `Ok(Some(tag))` = update to `tag`, + /// `Ok(None)` = current, `Err(msg)` = the check could not run for that repo + /// and its existing badge must be left alone rather than cleared. + UpdatesChecked(Vec<(String, Result, String>)>), /// One-click sequential update of every app with a pending update. UpdateAll, /// Fetch the release notes ("what's new") for a repo's available update. diff --git a/src/update/mod.rs b/src/update/mod.rs index d5bf0b0..baaaab4 100644 --- a/src/update/mod.rs +++ b/src/update/mod.rs @@ -678,6 +678,75 @@ mod tests { assert_eq!(app.notifications.len(), 1); } + #[test] + fn a_repo_that_left_the_catalog_skips_forward_instead_of_stranding_the_queue() { + let mut app = App::new_for_test(); + // "Gone" was queued by Update All, then a catalog refresh dropped it. + app.colony_repo_list = vec![repo("Still", "")]; + app.update_queue = vec!["Next".to_string()]; + + let _ = app.update(Message::DownloadRelease( + "Gone".to_string(), + github::current_platform_key().to_string(), + )); + + assert!( + app.update_queue.is_empty(), + "the skipped repo must hand the queue on, not park it for the next unrelated install" + ); + assert_eq!( + app.notifications.len(), + 1, + "the user must be told which app was skipped" + ); + } + + #[test] + fn app_check_failure_never_clears_a_badge_or_claims_up_to_date() { + let mut app = App::new_for_test(); + app.is_checking_updates = true; + app.available_updates + .insert("Grape".to_string(), "v2.0.0".to_string()); + app.available_updates + .insert("Spotter".to_string(), "v3.0.0".to_string()); + + // Grape's check could not run; Spotter's ran and came back current. + let _ = app.update(Message::UpdatesChecked(vec![ + ("Grape".to_string(), Err("rate limited".to_string())), + ("Spotter".to_string(), Ok(None)), + ])); + + assert!(!app.is_checking_updates); + assert_eq!( + app.available_updates.get("Grape").map(String::as_str), + Some("v2.0.0"), + "a check that did not run must leave the existing badge alone" + ); + assert!( + !app.available_updates.contains_key("Spotter"), + "a check that DID run and found nothing must clear its badge" + ); + assert!( + !app.status_message.contains("applications found"), + "the all-clear line must not be written when a check failed, got: {}", + app.status_message + ); + assert_eq!( + app.notifications.len(), + 1, + "the user must be told the check was incomplete" + ); + + // Every check succeeding and finding nothing IS the all-clear. + app.notifications.clear(); + let _ = app.update(Message::UpdatesChecked(vec![( + "Grape".to_string(), + Ok(None), + )])); + assert!(app.available_updates.is_empty()); + assert!(app.notifications.is_empty()); + } + #[test] fn window_resize_bumps_generation_and_stale_saves_are_ignored() { let mut app = App::new_for_test(); diff --git a/src/update/store.rs b/src/update/store.rs index 9ef0ddd..6515219 100644 --- a/src/update/store.rs +++ b/src/update/store.rs @@ -117,7 +117,18 @@ impl App { self.status_message = i18n::t_fmt("no_release_for", &[("platform", &platform_key)]); } } - Task::none() + + // We got here without starting a download: either the repo vanished + // from the catalog (a refresh can land mid-queue and replaces it + // wholesale) or it ships nothing for this platform. Say so, and keep + // the "Update all" chain moving - the queue is otherwise only advanced + // by a completion, so it would sit parked until some later, unrelated + // install silently drained it. + let skipped = i18n::t_fmt("update_skipped", &[("name", &repo_name)]); + Task::batch([ + self.push_notification(skipped, NotificationLevel::Warning), + self.dispatch_next_queued_update(), + ]) } pub(super) fn download_progress( @@ -357,7 +368,16 @@ impl App { async move { let client = match github::build_update_client(token.as_deref()) { Ok(c) => c, - Err(_) => return Vec::new(), + Err(e) => { + // The check did not run for ANY repo. Report that per + // repo rather than returning an empty list, which the + // handler would read as "everything is current". + let e = e.to_string(); + return repos + .into_iter() + .map(|(name, _)| (name, Err(e.clone()))) + .collect(); + } }; let futs: Vec<_> = repos .iter() @@ -366,17 +386,14 @@ impl App { let n = name.clone(); let t = tag.clone(); async move { - github::check_update_available(&c, &n, &t) + let outcome = github::check_update_available(&c, &n, &t) .await - .map(|v| (n, v)) + .map_err(|e| e.to_string()); + (n, outcome) } }) .collect(); - futures::future::join_all(futs) - .await - .into_iter() - .flatten() - .collect() + futures::future::join_all(futs).await }, Message::UpdatesChecked, ) @@ -461,24 +478,52 @@ impl App { Task::none() } - pub(super) fn updates_checked(&mut self, updates: Vec<(String, String)>) -> Task { + pub(super) fn updates_checked( + &mut self, + outcomes: Vec<(String, Result, String>)>, + ) -> Task { self.is_checking_updates = false; - // Record which apps have a pending update so the grid cards can - // show an update badge (not just a transient toast). - self.available_updates = updates.iter().cloned().collect(); - let notif_task = if updates.is_empty() { + + // Merge, never replace: a repo whose check could not run keeps the + // badge it already had. Replacing the whole map meant that going + // offline (or simply hitting the anonymous rate limit on a second + // launch) cleared every badge and told the user they were current. + let mut failed = 0usize; + let mut fresh: Vec = Vec::new(); + for (name, outcome) in &outcomes { + match outcome { + Ok(Some(tag)) => { + self.available_updates.insert(name.clone(), tag.clone()); + fresh.push(name.clone()); + } + Ok(None) => { + self.available_updates.remove(name); + } + Err(e) => { + failed += 1; + tracing::warn!("Update check failed for {name}: {e}"); + } + } + } + + let notif_task = if failed > 0 { + // Never write the all-clear line when part of the check did not + // run - say so, and say how many apps we could not speak for. + let msg = i18n::t_fmt("update_check_failed", &[("count", &failed.to_string())]); + self.status_message = msg.clone(); + self.push_notification(msg, NotificationLevel::Warning) + } else if fresh.is_empty() { self.status_message = i18n::t_fmt( "apps_found", &[("count", &self.applications.len().to_string())], ); Task::none() } else { - let names: Vec<&str> = updates.iter().map(|(n, _)| n.as_str()).collect(); let msg = i18n::t_fmt( "updates_available", &[ - ("count", &updates.len().to_string()), - ("names", &names.join(", ")), + ("count", &fresh.len().to_string()), + ("names", &fresh.join(", ")), ], ); self.push_notification(msg, NotificationLevel::Info) From b1d57a4b9e0306af5264f163a5f72bf07e520aff Mon Sep 17 00:00:00 2001 From: MotherSphere Date: Thu, 27 Aug 2026 16:04:21 +0200 Subject: [PATCH 03/18] fix(ui): make failures visible - status line everywhere, logs, and retry Colony had no channel that reliably reached the user when something failed. Five parts of the same problem: status_message had exactly one render site, inside the grid header. The detail page returns early and settings and the GitHub panel replace the content pane, so on three of the four pages the status line did not exist - and it was the sole feedback for "no release for your platform", a failed uninstall, a failed release-notes fetch and a rate-limited refresh. It is now a footer in App::view, rendered on every page, which also stops a long raw transport error from laying the Fill search input out at width zero. Diagnostics reached nobody either. EnvFilter::from_default_env().add_directive( INFO) reads as "default to info", but a bare RUST_LOG=debug parses to a directive that compares Equal to the added one, so add_directive REPLACED the user's request. There was no log file at all, and a .desktop launch has no terminal (on Windows, no console). RUST_LOG is now honoured verbatim, and logs go to the cache dir as well as stderr, truncated per run. colony --version opened the GUI, while the bug template makes its output a required field. --version and --help are now answered before the window opens. Toasts never expired with reduce-motion or animations off: the expiry timer was gated on animations, so the retain(!is_expired) branch was unreachable and the stack grew without bound. Since the overlay grows upward from the bottom, the oldest toasts left clicking range and could never be dismissed - the accessibility settings were the ones that silted the UI up. The timer is now always armed and the stack is capped at five. Finally, a failed catalog fetch left an anonymous user with no way out but signing in or restarting. Refresh is now offered in the disconnected and error arms of the GitHub panel and in the grid's empty state - the same anonymous fetch boot already runs. A refresh the user clicked also toasts on failure now; the anti-noise rule stays for the boot path. --- src/app.rs | 32 ++++++++++++++- src/main.rs | 75 ++++++++++++++++++++++++++++++++--- src/state.rs | 6 +++ src/ui/app_grid.rs | 82 ++++++++++++++++++++++++++++----------- src/ui/github_panel.rs | 51 ++++++++++++++++++++++-- src/update/github_auth.rs | 14 ++++--- src/update/mod.rs | 53 ++++++++++++++++++++----- src/update/store.rs | 8 +++- 8 files changed, 269 insertions(+), 52 deletions(-) diff --git a/src/app.rs b/src/app.rs index ea796b8..9ec3fc8 100644 --- a/src/app.rs +++ b/src/app.rs @@ -190,6 +190,8 @@ impl App { is_checking_updates: false, // A catalog fetch (token'd or anonymous) always starts at boot. is_fetching_repos: true, + // The boot fetch is not a click. + repos_refresh_manual: false, // Settings section state persistence settings_expanded_sections: HashSet::new(), // Detail tabs @@ -279,7 +281,33 @@ impl App { let main_layout = row![sidebar, content].spacing(0); - let page = container(main_layout).width(Fill).height(Fill); + // The status line used to render only inside the grid header, which is + // three of the four pages short: the detail page returns early, and + // settings and the GitHub panel replace the content pane outright. So + // "no release for your platform", a failed uninstall, a failed + // release-notes fetch and a rate-limited refresh all reported into a + // widget that was not on screen. As a footer it is always there, and + // it no longer competes with the search input for row width. + let page: Element<'_, Message> = if self.status_message.is_empty() { + container(main_layout).width(Fill).height(Fill).into() + } else { + let status_bar = container( + text(&self.status_message) + .size(self.sz(12)) + .font(self.app_font()) + .color(Palette::TEXT_DIMMER()), + ) + .padding([6, 16]) + .width(Fill) + .style(|_theme| container::Style { + background: Some(Palette::BG_SIDEBAR().into()), + ..Default::default() + }); + container(column![main_layout, status_bar]) + .width(Fill) + .height(Fill) + .into() + }; // Build overlay toasts (download progress + notifications) anchored to bottom-left let mut overlay_items: Vec> = Vec::new(); @@ -421,7 +449,7 @@ impl App { // Build the base page with overlays let base: Element<'_, Message> = if overlay_items.is_empty() { - page.into() + page } else { let overlay = container(Column::with_children(overlay_items).spacing(6)) .padding(iced::Padding { diff --git a/src/main.rs b/src/main.rs index ab03270..232cb75 100644 --- a/src/main.rs +++ b/src/main.rs @@ -21,13 +21,76 @@ mod update; use state::{default_font, App}; +/// Where diagnostics land. A plain truncate-on-start file, not a rolling +/// appender: one run's worth of log is what a bug report needs, and it keeps +/// the dependency set unchanged. +fn log_file_path() -> Option { + let dir = dirs::cache_dir()?.join("colony"); + std::fs::create_dir_all(&dir).ok()?; + Some(dir.join("colony.log")) +} + +/// Answer `--version` / `--help` without opening a window. The bug template +/// makes `colony --version` a required field, and on Windows +/// `windows_subsystem = "windows"` means stderr goes nowhere, so the GUI was +/// the only possible answer to either flag. +/// +/// Returns true when the process should exit without starting the UI. +fn handle_cli_flags() -> bool { + let Some(arg) = std::env::args().nth(1) else { + return false; + }; + match arg.as_str() { + "--version" | "-V" => { + println!("colony {}", env!("CARGO_PKG_VERSION")); + true + } + "--help" | "-h" => { + println!( + "colony {}\nThe hub for the Colony ecosystem.\n\n\ + Usage: colony [OPTIONS]\n\n\ + Options:\n \ + -V, --version Print the version and exit\n \ + -h, --help Print this help and exit\n\n\ + Colony takes no other arguments; everything else is configured in the app.\n\n\ + Diagnostics are written to {} and to stderr.\n\ + Set RUST_LOG=debug for more detail.", + env!("CARGO_PKG_VERSION"), + log_file_path() + .map(|p| p.display().to_string()) + .unwrap_or_else(|| "(no cache directory)".into()), + ); + true + } + _ => false, + } +} + pub fn main() -> iced::Result { - tracing_subscriber::fmt() - .with_env_filter( - tracing_subscriber::EnvFilter::from_default_env() - .add_directive(tracing::Level::INFO.into()), - ) - .init(); + if handle_cli_flags() { + return Ok(()); + } + + // `EnvFilter::from_default_env().add_directive(INFO)` looked like "default + // to info", but a bare `RUST_LOG=debug` parses to a directive that compares + // Equal to the added one, so add_directive REPLACED it and the user's + // request was silently discarded. Build the default only when RUST_LOG is + // absent or unparseable, and otherwise honour it verbatim. + let filter = tracing_subscriber::EnvFilter::try_from_default_env() + .unwrap_or_else(|_| tracing_subscriber::EnvFilter::new("info")); + + // Log to a file as well as stderr: a .desktop launch has no terminal, and + // on Windows there is no console at all, so stderr-only meant that every + // warning in the codebase reached nobody in any shipped configuration. + use tracing_subscriber::fmt::writer::MakeWriterExt; + match log_file_path().and_then(|p| std::fs::File::create(p).ok()) { + Some(file) => tracing_subscriber::fmt() + .with_env_filter(filter) + .with_writer(std::sync::Mutex::new(file).and(std::io::stderr)) + .with_ansi(false) + .init(), + None => tracing_subscriber::fmt().with_env_filter(filter).init(), + } // Honor the saved language preference over environment locale detection, // and reopen at the last persisted window size (clamped to sanity). diff --git a/src/state.rs b/src/state.rs index 3ad2c85..dc48e73 100644 --- a/src/state.rs +++ b/src/state.rs @@ -224,6 +224,11 @@ pub struct App { pub is_downloading: bool, pub is_checking_updates: bool, pub is_fetching_repos: bool, + /// Whether the catalog fetch in flight was started by the user clicking + /// Refresh rather than by boot. A boot fetch failing while a cached catalog + /// is on screen is routine and stays in the status line; a click the user + /// just made deserves a toast, otherwise the button looks broken. + pub repos_refresh_manual: bool, // Settings section state persistence pub settings_expanded_sections: HashSet, // Detail tabs @@ -540,6 +545,7 @@ impl App { is_downloading: false, is_checking_updates: false, is_fetching_repos: false, + repos_refresh_manual: false, settings_expanded_sections: std::collections::HashSet::new(), detail_tab: DetailTab::ReadMe, detail_blocks: Vec::new(), diff --git a/src/ui/app_grid.rs b/src/ui/app_grid.rs index e22b9a6..56d731c 100644 --- a/src/ui/app_grid.rs +++ b/src/ui/app_grid.rs @@ -318,8 +318,15 @@ impl App { .id(crate::ui::tutorial::ID_SEARCH) .width(Fill); - // Show search result count when query is active - let status_text = if !self.search_query.is_empty() { + // Only the search result count lives here. `status_message` used to + // share this row, which had two problems: it was the ONLY render site + // for the status line (so uninstall, platform and release-notes errors + // were invisible on the detail, settings and GitHub pages), and a long + // raw transport error laid the Fill search input out at width zero. + // It now renders as a footer in `App::view`, on every page. + let status_text = if self.search_query.is_empty() { + String::new() + } else { let filtered_count = self.filtered_applications().len() + self.filtered_colony_repos().len(); crate::i18n::t_fmt( @@ -329,8 +336,6 @@ impl App { ("query", &self.search_query), ], ) - } else { - self.status_message.clone() }; // Show spinner indicator for async operations @@ -438,25 +443,56 @@ impl App { } else { crate::i18n::t("no_apps_found") }; - return container( - column![ - text("\u{f002}") - .size(self.sz(32)) - .font(self.app_font()) - .color(Palette::TEXT_DIMMEST()), - container(text("")).height(12), - text(empty_msg) - .size(self.sz(16)) - .font(self.app_font()) - .color(Palette::TEXT_PLACEHOLDER()), - ] - .align_x(iced::Alignment::Center), - ) - .width(Fill) - .height(Fill) - .center_x(Fill) - .center_y(Fill) - .into(); + let mut empty = column![ + text("\u{f002}") + .size(self.sz(32)) + .font(self.app_font()) + .color(Palette::TEXT_DIMMEST()), + container(text("")).height(12), + text(empty_msg) + .size(self.sz(16)) + .font(self.app_font()) + .color(Palette::TEXT_PLACEHOLDER()), + ] + .align_x(iced::Alignment::Center); + + // An empty store with no search query means the catalog fetch never + // landed. Without a button here the only recovery was to sign in or + // restart Colony - on a page whose whole pitch is that browsing + // needs no account. + if self.search_query.is_empty() && self.colony_repo_list.is_empty() { + empty = empty.push(container(text("")).height(16)).push( + button( + text(crate::i18n::t("github_refresh")) + .size(self.sz(13)) + .font(self.app_font()), + ) + .on_press_maybe( + (!self.is_fetching_repos).then_some(Message::GitHubRefreshRepos), + ) + .padding([8, 16]) + .style(|_theme, status| { + let bg = match status { + button::Status::Hovered => Palette::BTN_HOVER(), + button::Status::Pressed => Palette::BTN_PRESSED(), + _ => Palette::BTN_DEFAULT(), + }; + button::Style { + background: Some(bg.into()), + text_color: Palette::TEXT_PRIMARY(), + border: iced::Border::default().rounded(8), + ..Default::default() + } + }), + ); + } + + return container(empty) + .width(Fill) + .height(Fill) + .center_x(Fill) + .center_y(Fill) + .into(); } // Chunk cards into a grid whose column count adapts to the available diff --git a/src/ui/github_panel.rs b/src/ui/github_panel.rs index 6e25d5a..a1a34e9 100644 --- a/src/ui/github_panel.rs +++ b/src/ui/github_panel.rs @@ -61,12 +61,40 @@ impl App { .font(self.app_font()) .color(Palette::TEXT_MUTED()); + // Browsing the catalog is anonymous, so a user whose boot fetch + // failed (flaky network, VPN, captive portal) is signed out by + // definition - and this arm used to offer sign-in as the only + // way forward. It is the same anonymous fetch the boot path + // already runs; no token, no sign-in required. + let retry_btn = button( + text(crate::i18n::t("github_refresh")) + .size(self.sz(13)) + .font(self.app_font()), + ) + .on_press_maybe((!self.is_fetching_repos).then_some(Message::GitHubRefreshRepos)) + .padding([8, 16]) + .style(|_theme, status| { + let bg = match status { + button::Status::Hovered => Palette::BTN_HOVER(), + button::Status::Pressed => Palette::BTN_PRESSED(), + _ => Palette::BTN_DEFAULT(), + }; + button::Style { + background: Some(bg.into()), + text_color: Palette::TEXT_PRIMARY(), + border: iced::Border::default().rounded(8), + ..Default::default() + } + }); + column![ desc, container(text("")).height(16), login_btn, container(text("")).height(12), - info + info, + container(text("")).height(12), + retry_btn ] .spacing(8) .into() @@ -254,9 +282,24 @@ impl App { .on_press(Message::GitHubLogin) .padding([8, 16]); - column![err, container(text("")).height(12), retry_btn] - .spacing(8) - .into() + // Retrying the sign-in is not the only thing that can go wrong + // here; the catalog fetch can fail on its own, and refetching + // it needs no account at all. + let refresh_btn = button( + text(crate::i18n::t("github_refresh")) + .size(self.sz(13)) + .font(self.app_font()), + ) + .on_press_maybe((!self.is_fetching_repos).then_some(Message::GitHubRefreshRepos)) + .padding([8, 16]); + + column![ + err, + container(text("")).height(12), + row![retry_btn, refresh_btn].spacing(8) + ] + .spacing(8) + .into() } }; diff --git a/src/update/github_auth.rs b/src/update/github_auth.rs index 0d38307..2559605 100644 --- a/src/update/github_auth.rs +++ b/src/update/github_auth.rs @@ -107,6 +107,7 @@ impl App { &mut self, repos: Vec, ) -> Task { + self.repos_refresh_manual = false; self.is_fetching_repos = false; let count = repos.len(); if let Err(e) = crate::persistence::save_repos_cache(&repos) { @@ -140,6 +141,7 @@ impl App { pub(super) fn github_error(&mut self, e: String) -> Task { self.is_fetching_repos = false; + let was_manual = std::mem::take(&mut self.repos_refresh_manual); tracing::error!(error = %e, "GitHub error"); if self.colony_repo_list.is_empty() { if let Some(cached) = crate::persistence::load_repos_cache() { @@ -150,16 +152,17 @@ impl App { // Offline fallback repos may have cached icons on disk. self.reload_app_icons(); self.status_message = i18n::t_fmt("github_api_error", &[("error", &e)]); - if self.colony_repo_list.is_empty() { + if self.colony_repo_list.is_empty() || was_manual { + // The anti-noise rule holds for the BOOT path only: a toast on + // every offline start, with a cached catalog already on screen, + // would be pure noise. A refresh the user just clicked is the + // opposite case - without feedback the button reads as broken, + // since the repo count does not move either. self.push_notification( i18n::t_fmt("github_api_error", &[("error", &e)]), NotificationLevel::Error, ) } else { - // The catalog is showing (cached or previously fetched): a - // toast on every offline boot would be pure noise - the - // status line already carries the error. Only an EMPTY - // catalog warrants interrupting the user. Task::none() } } @@ -168,6 +171,7 @@ impl App { if self.is_fetching_repos { return Task::none(); } + self.repos_refresh_manual = true; self.is_fetching_repos = true; // Anonymous refresh is supported: the token only raises the // rate limit (60 req/h unauthenticated vs 5000 signed-in). diff --git a/src/update/mod.rs b/src/update/mod.rs index baaaab4..21534c2 100644 --- a/src/update/mod.rs +++ b/src/update/mod.rs @@ -36,17 +36,26 @@ impl App { let timeout = level.timeout(); self.notifications .push(Notification::new(id, message, level)); - // When reduce_motion or animations off, don't auto-dismiss (user must click) - if self.reduce_motion || !self.animations { - Task::none() - } else { - Task::perform( - async move { - tokio::time::sleep(timeout).await; - }, - |_| Message::TickNotifications, - ) + // The overlay column is anchored to the bottom and grows upward, so an + // unbounded stack pushes the OLDEST toasts off the top of the window - + // where they can never be clicked, and a toast is only dismissed by + // clicking it. Cap it so the overlay can never exceed the window. + const MAX_TOASTS: usize = 5; + while self.notifications.len() > MAX_TOASTS { + self.notifications.remove(0); } + // Always arm the expiry timer. Gating it on animations meant that with + // reduce-motion (or animations off) nothing ever sent TickNotifications, + // so the `retain(!is_expired)` branch below was unreachable and toasts + // were permanent - the accessibility settings were the ones that + // silted the UI up. The animation gate belongs on the FADE, not on the + // expiry, and it is already applied there. + Task::perform( + async move { + tokio::time::sleep(timeout).await; + }, + |_| Message::TickNotifications, + ) } /// Decode any cached app icons that aren't yet in memory into image handles, @@ -678,6 +687,30 @@ mod tests { assert_eq!(app.notifications.len(), 1); } + #[test] + fn the_toast_stack_is_capped_even_with_animations_off() { + let mut app = App::new_for_test(); + // The accessibility settings were the ones that silted the UI up: with + // no animation tick, nothing ever expired the toasts, and the overlay + // grows upward from the bottom so the oldest scrolled out of clicking + // range and could never be dismissed at all. + app.animations = false; + app.reduce_motion = true; + for i in 0..12 { + let _ = app.push_notification(format!("toast {i}"), NotificationLevel::Info); + } + assert!( + app.notifications.len() <= 5, + "the overlay must never outgrow the window, got {}", + app.notifications.len() + ); + assert_eq!( + app.notifications.last().map(|n| n.message.as_str()), + Some("toast 11"), + "the newest toast is the one that must survive the cap" + ); + } + #[test] fn a_repo_that_left_the_catalog_skips_forward_instead_of_stranding_the_queue() { let mut app = App::new_for_test(); diff --git a/src/update/store.rs b/src/update/store.rs index 6515219..abb668c 100644 --- a/src/update/store.rs +++ b/src/update/store.rs @@ -294,8 +294,12 @@ impl App { Ok(app_dir) => { if app_dir.exists() { if let Err(e) = std::fs::remove_dir_all(&app_dir) { - self.status_message = - i18n::t_fmt("uninstall_error", &[("error", &e.to_string())]); + // Uninstall is always confirmed from the detail page, + // and the app stays on disk half-removed. Toast it - + // this is not a background condition. + let msg = i18n::t_fmt("uninstall_error", &[("error", &e.to_string())]); + self.status_message = msg.clone(); + return self.push_notification(msg, NotificationLevel::Error); } else { self.status_message = i18n::t_fmt("uninstalled", &[("name", &repo_name)]); // AFTER the directory removal, so the cache From 62102a0487cb6ebafa80c527509e4de7a5296c61 Mon Sep 17 00:00:00 2001 From: MotherSphere Date: Thu, 27 Aug 2026 16:10:03 +0200 Subject: [PATCH 04/18] fix(security): validate remote strings before they reach URLs and paths The most serious of these is the release URL. `tag` comes from colony.json and was interpolated straight into https://github.com/{org}/{repo}/releases/download/{tag}/{filename}. reqwest parses through the WHATWG URL parser, which collapses `..` segments BEFORE the request is issued, so a tag reading `v1/../../../../../EvilOrg/EvilRepo/releases/download/v1` shortens the path into an account outside the org - and Colony then installs and trusts those bytes. That is reachable from write access to one line of a catalog repo, with no release-publishing rights anywhere, and it survives a JSON diff review. It breaks the single containment claim the trust model rests on. URLs are now built from percent-encoded path segments (build_url), applied to the release URL, the tagged-release API call and the contents API call that carries the manifest's icon path. Encoding rather than rejecting, because a legitimate git tag may contain a slash. Also in this batch: - ensure_safe_component was POSIX-shaped. Rust's Windows path parser treats "payload:stream" as a single Normal component, so it passed the guard and wrote an NTFS alternate data stream; reserved device names (CON, NUL, COM1) resolve to devices from any directory; and a trailing dot or space is stripped by the filesystem, so the name checked differed from the name written. Enforced on every platform, since the catalog is shared. - .meta and .blockmap join NON_INSTALLABLE_SUFFIXES. Without them, the day an ecosystem app adopts the signed sidecar, a documented substring filePattern starts matching two assets and fails - which the spec promises cannot happen. - The staged `.new` write, the staged .sig/.meta/.meta.sig writes and the OAuth token file now use create_new. fs::write follows a symlink planted at those predictable names; for .new that meant the rename afterwards moved the SYMLINK over Colony's own binary. - Buffered sidecar fetches are capped at 64 KiB. A release can publish a multi-gigabyte file named foo-linux.sig, which was an OOM on click. - The install path reads the staged file once and checks the signature and the digest against that one buffer, instead of three separate reads of a predictable path between verification and install. - OAuthSession has a hand-written Debug that redacts the token. Message derives Debug and carries one, so a future `tracing::debug!(?message)` would have put it in the log file. --- src/download.rs | 242 +++++++++++++++++++++++++++++++++++++---- src/github/catalog.rs | 30 ++++- src/github/releases.rs | 17 ++- src/oauth.rs | 30 ++++- src/persistence.rs | 9 ++ 5 files changed, 298 insertions(+), 30 deletions(-) diff --git a/src/download.rs b/src/download.rs index 41667d5..d5d087f 100644 --- a/src/download.rs +++ b/src/download.rs @@ -130,6 +130,35 @@ async fn download_to_file( Ok(()) } +/// Ceiling for the small sidecars Colony buffers whole (`.sig` is 64 bytes, +/// `.meta` three short lines). Whoever controls a release can publish a +/// multi-gigabyte file named `foo-linux.sig`; without a cap that is an OOM the +/// moment a user clicks Install. Generous by four orders of magnitude. +const MAX_SIDECAR_BYTES: u64 = 64 * 1024; + +/// Read a response body with an upper bound, so a body with no Content-Length - +/// or one that lies about it - cannot be unbounded. +async fn bounded_body(resp: reqwest::Response, url: &str, max: u64) -> Result> { + if let Some(len) = resp.content_length() { + anyhow::ensure!( + len <= max, + "Refusing {url}: declares {len} bytes, over the {max}-byte limit" + ); + } + use futures::StreamExt; + let mut stream = resp.bytes_stream(); + let mut out: Vec = Vec::new(); + while let Some(chunk) = stream.next().await { + let chunk = chunk?; + anyhow::ensure!( + out.len() as u64 + chunk.len() as u64 <= max, + "Refusing {url}: body exceeds the {max}-byte limit" + ); + out.extend_from_slice(&chunk); + } + Ok(out) +} + /// Fetch a small OPTIONAL resource: `Ok(None)` on HTTP 404 (the resource /// genuinely is not published), `Err` on any other failure - so a transient /// network error can never be mistaken for "not published" (an attacker able @@ -150,7 +179,7 @@ async fn fetch_optional_bytes( if !resp.status().is_success() { anyhow::bail!("HTTP {} for {url}", resp.status()); } - Ok(Some(resp.bytes().await?.to_vec())) + Ok(Some(bounded_body(resp, url, MAX_SIDECAR_BYTES).await?)) } /// Fetch a small resource (e.g. a detached signature) fully into memory. @@ -163,15 +192,16 @@ async fn fetch_bytes(client: &reqwest::Client, url: &str, token: Option<&str>) - if !resp.status().is_success() { anyhow::bail!("HTTP {} for {url}", resp.status()); } - Ok(resp.bytes().await?.to_vec()) + bounded_body(resp, url, MAX_SIDECAR_BYTES).await } -/// Verify SHA256 checksum of a file against an expected hex digest. -fn verify_sha256(path: &std::path::Path, expected_hex: &str) -> Result<()> { - let mut file = std::fs::File::open(path)?; - let mut hasher = Sha256::new(); - std::io::copy(&mut file, &mut hasher)?; - let computed = format!("{:x}", hasher.finalize()); +/// Verify a SHA256 digest over bytes already in memory. +/// +/// Takes bytes rather than a path so the caller checks exactly what it is about +/// to install: re-opening the staged file to hash it means the digest describes +/// one read and the install uses another. +fn verify_sha256_bytes(bytes: &[u8], expected_hex: &str) -> Result<()> { + let computed = format!("{:x}", Sha256::digest(bytes)); if computed != expected_hex.to_lowercase() { anyhow::bail!( "SHA256 mismatch: expected {}, got {}", @@ -204,6 +234,13 @@ pub fn launcher_is_system_managed() -> bool { /// Ensure a filename is a single normal path component (no `..`, no path /// separators, not absolute) before it is joined into a destination directory. /// Shared by archive extraction and raw-asset download to block path traversal. +/// Names Win32 resolves to devices no matter which directory contains them, +/// and regardless of any extension (`CON.txt` is still the console). +const RESERVED_DEVICE_NAMES: &[&str] = &[ + "CON", "PRN", "AUX", "NUL", "COM1", "COM2", "COM3", "COM4", "COM5", "COM6", "COM7", "COM8", + "COM9", "LPT1", "LPT2", "LPT3", "LPT4", "LPT5", "LPT6", "LPT7", "LPT8", "LPT9", +]; + pub(crate) fn ensure_safe_component(name: &str) -> Result<()> { let p = std::path::Path::new(name); anyhow::ensure!( @@ -211,9 +248,74 @@ pub(crate) fn ensure_safe_component(name: &str) -> Result<()> { && matches!(p.components().next(), Some(std::path::Component::Normal(_))), "Invalid file name (path traversal attempt?): {name}" ); + + // The check above is the shape of a POSIX path, which is only half the + // question on the three platforms Colony supports. Rust's Windows parser + // recognises a drive prefix only when exactly one letter precedes the + // colon, so "payload:stream" is a single Normal component and joining it + // writes an NTFS alternate data stream on a file named "payload"; a + // reserved device name resolves to a device from any directory; and a + // trailing dot or space is stripped by the filesystem, so the name written + // differs from the name checked. + // + // Enforced on every platform, not behind cfg(windows): the catalog is + // shared, so a manifest that would be refused on Windows must be refused + // everywhere rather than installing differently per user. + anyhow::ensure!( + !name.contains( + |c: char| matches!(c, ':' | '\\' | '/' | '<' | '>' | '"' | '|' | '?' | '*') + || c.is_control() + ), + "Invalid file name (reserved character): {name}" + ); + anyhow::ensure!( + !name.ends_with('.') && !name.ends_with(' '), + "Invalid file name (trailing dot or space is silently stripped): {name}" + ); + let stem = name.split('.').next().unwrap_or(name); + anyhow::ensure!( + !RESERVED_DEVICE_NAMES + .iter() + .any(|d| stem.eq_ignore_ascii_case(d)), + "Invalid file name (reserved device name): {name}" + ); Ok(()) } +/// Build a URL from a trusted base and remote-controlled path segments, +/// percent-encoding each segment so it can never be structural. +/// +/// `format!`-ing a remote string into a URL is not safe even when the string +/// looks harmless in a JSON diff. `reqwest::Client::get` parses through the +/// WHATWG URL parser, which collapses `..` segments (and `%2e%2e`) *before* the +/// request is made: a `colony.json` whose `tag` reads +/// `v1/../../../../../EvilOrg/EvilRepo/releases/download/v1` shortens the path +/// into a different account entirely, and Colony would install and trust bytes +/// that were never published under the org. That is the one containment claim +/// the whole trust model rests on, and it was reachable from write access to a +/// single line of a catalog repo - no release-publishing rights needed. +/// +/// Segments are pushed through `path_segments_mut`, which percent-encodes them, +/// so a `/` or a `..` inside a segment stays data. Legitimate git tags may +/// contain `/` (`release/1.0`), so encoding is the honest fix here; rejecting +/// the value outright would break them. +pub(crate) fn build_url(base: &str, segments: &[&str]) -> Result { + let mut url = reqwest::Url::parse(base)?; + { + let mut path = url + .path_segments_mut() + .map_err(|_| anyhow::anyhow!("URL base cannot have path segments: {base}"))?; + for segment in segments { + anyhow::ensure!( + !segment.is_empty(), + "empty URL path segment for base {base}" + ); + path.push(segment); + } + } + Ok(url.into()) +} + /// Normalized form of `raw` when it is an absolute `http`/`https` URL with a /// host, else `None`. /// @@ -260,6 +362,17 @@ fn create_new_file(path: &std::path::Path) -> Result { .open(path)?) } +/// `std::fs::write` for the trust path: same result, but through +/// [`create_new_file`] so a symlink planted at the (entirely predictable) +/// staging name is never followed. +fn write_new_file(path: &std::path::Path, contents: &[u8]) -> Result<()> { + use std::io::Write; + let mut file = create_new_file(path)?; + file.write_all(contents)?; + file.flush()?; + Ok(()) +} + /// Extract a single file from a .zip archive. fn extract_from_zip( archive_path: &std::path::Path, @@ -420,9 +533,20 @@ pub async fn download_release_asset( // never truncates the currently-installed binary. let temp_path = dest_dir.join(format!("{filename}.part")); - let url = format!( - "https://github.com/{GITHUB_ACCOUNT}/{repo_name}/releases/download/{tag}/{filename}" - ); + // Every segment after the host is remote-controlled (`repo_name` from the + // API listing, `tag` and `filename` from colony.json), so build the URL + // from encoded segments instead of interpolating them. + let url = build_url( + "https://github.com", + &[ + GITHUB_ACCOUNT, + &repo_name, + "releases", + "download", + &tag, + &filename, + ], + )?; let client = download_client()?; download_to_file(&client, &url, token.as_deref(), &temp_path, progress_tx).await?; @@ -478,9 +602,19 @@ pub async fn download_release_asset( tokio::task::spawn_blocking(move || -> Result { let was_signed = signature.is_some(); + // Read the staged file ONCE and check everything against that one + // buffer. Each separate read of `/.part` - a + // predictable path - is another chance to check one set of bytes + // and install a different set; the launcher path already avoids + // this by installing the buffer it verified. + let staged_bytes = if signature.is_some() || expected_sha256.is_some() { + Some(std::fs::read(&temp_path)?) + } else { + None + }; if let Some(sig) = signature { - let bytes = std::fs::read(&temp_path)?; - if let Err(e) = crate::signing::verify_release_signature(&bytes, &sig) { + let bytes = staged_bytes.as_deref().unwrap_or_default(); + if let Err(e) = crate::signing::verify_release_signature(bytes, &sig) { let _ = std::fs::remove_file(&temp_path); anyhow::bail!( "Signature verification FAILED for {filename} - refusing to install: {e}" @@ -489,12 +623,14 @@ pub async fn download_release_asset( tracing::info!("ed25519 signature verified for {filename}"); } if let Some(ref expected) = expected_sha256 { - if let Err(e) = verify_sha256(&temp_path, expected) { + let bytes = staged_bytes.as_deref().unwrap_or_default(); + if let Err(e) = verify_sha256_bytes(bytes, expected) { let _ = std::fs::remove_file(&temp_path); return Err(e); } tracing::info!("SHA256 verified for {filename}"); } + drop(staged_bytes); let final_path = if let Some(ref bin) = binary_name { // Archive install: extract the named binary (atomically renamed @@ -636,13 +772,13 @@ pub async fn download_launcher_asset( // file could be swapped between download and apply. The metadata sidecar is // staged for the same reason. let sig_path = staged_signature_path(&dest_path); - if let Err(e) = std::fs::write(&sig_path, &signature) { + if let Err(e) = write_new_file(&sig_path, &signature) { let _ = std::fs::remove_file(&dest_path); anyhow::bail!("Could not stage update signature: {e}"); } let (meta_path, meta_sig_path) = staged_metadata_paths(&dest_path); - if let Err(e) = std::fs::write(&meta_path, &meta_bytes) - .and_then(|()| std::fs::write(&meta_sig_path, &meta_sig)) + if let Err(e) = write_new_file(&meta_path, &meta_bytes) + .and_then(|()| write_new_file(&meta_sig_path, &meta_sig)) { let _ = std::fs::remove_file(&dest_path); anyhow::bail!("Could not stage update metadata: {e}"); @@ -824,8 +960,14 @@ pub fn apply_launcher_update(new_binary: &std::path::Path) -> Result { } // Write the byte buffer that was just VERIFIED - copying the file again // would re-read from disk and install bytes the signature check never saw - // (a swap between read and copy would slip through). - std::fs::write(&staged_next, &staged_bytes) + // (a swap between read and copy would slip through). Through + // `write_new_file`, because `.new` is a predictable name in a + // user-writable directory: `fs::write` follows a symlink planted there, so + // the verified bytes would land at the attacker's path and the rename below + // would then move the SYMLINK over the running binary - turning Colony's + // own executable into a link to a file rewritable afterwards, cleanly past + // every signature check. + write_new_file(&staged_next, &staged_bytes) .map_err(|e| anyhow::anyhow!("Failed to stage new binary: {e}"))?; #[cfg(unix)] { @@ -870,6 +1012,62 @@ pub fn apply_launcher_update(new_binary: &std::path::Path) -> Result { mod tests { use super::*; + /// A `tag` from colony.json used to be interpolated straight into the + /// release URL. reqwest parses with the WHATWG parser, which collapses + /// `..` BEFORE the request is issued, so one line of a catalog repo could + /// redirect the install to an account outside the org entirely - defeating + /// the only containment claim the trust model has. + #[test] + fn a_hostile_tag_cannot_walk_the_release_url_out_of_the_org() { + let hostile = "v1/../../../../../EvilOrg/EvilRepo/releases/download/v1"; + let url = build_url( + "https://github.com", + &[ + "Project-Colony", + "Grape", + "releases", + "download", + hostile, + "grape-linux", + ], + ) + .expect("segments are encoded, not rejected"); + + assert!( + url.starts_with("https://github.com/Project-Colony/Grape/releases/download/"), + "the request must stay inside the org, got {url}" + ); + assert!( + !url.contains("EvilOrg/EvilRepo/releases"), + "the traversal must not survive as structure, got {url}" + ); + + // Reparsing is what reqwest itself does; the collapse must not happen + // there either, which is the whole point of encoding the segment. + let reparsed = reqwest::Url::parse(&url).expect("valid URL"); + assert_eq!( + reparsed.path_segments().map(|s| s.count()), + Some(6), + "no segment may be structural: {url}" + ); + + // A legitimate tag containing a slash (`release/1.0`) still works - + // rejecting `/` outright would have broken real repos. + let ok = build_url( + "https://github.com", + &[ + "Project-Colony", + "Grape", + "releases", + "download", + "release/1.0", + "grape-linux", + ], + ) + .expect("slashes in a tag are encoded, not an error"); + assert!(ok.contains("release%2F1.0"), "got {ok}"); + } + /// The raw-binary branch of `extract_binary_from_archive` used to join the /// manifest's `binary` field straight into the install dir, so a hostile /// manifest could write anywhere (the caller then chmods 0755 and writes a @@ -1185,7 +1383,7 @@ mod tests { // SHA256 of "hello world" let expected = "b94d27b9934d3e08a52e52d7da7dabfac484efe37a5380ee9088f7ace2efcde9"; - assert!(verify_sha256(&file_path, expected).is_ok()); + assert!(verify_sha256_bytes(&std::fs::read(&file_path).unwrap(), expected).is_ok()); let _ = std::fs::remove_dir_all(&dir); } @@ -1200,7 +1398,9 @@ mod tests { f.write_all(b"hello world").unwrap(); f.flush().unwrap(); - assert!(verify_sha256(&file_path, "0000000000000000").is_err()); + assert!( + verify_sha256_bytes(&std::fs::read(&file_path).unwrap(), "0000000000000000").is_err() + ); let _ = std::fs::remove_dir_all(&dir); } diff --git a/src/github/catalog.rs b/src/github/catalog.rs index a32b2c5..f83ae4b 100644 --- a/src/github/catalog.rs +++ b/src/github/catalog.rs @@ -260,6 +260,20 @@ async fn fetch_license_with_fallback( } } +/// Build a `contents` API URL with every path segment percent-encoded. +/// +/// `repo_name` comes from the API listing and `path` can be the manifest's +/// `icon` field - both remote data, on a request that carries the user's token. +/// Interpolating them lets a `..` shorten the path onto a different endpoint +/// before the request is sent. An icon path is legitimately nested +/// ("assets/icons/icon.png"), so it is split on '/' and each part encoded +/// rather than rejected wholesale. +fn contents_url(repo_name: &str, path: &str) -> Result { + let mut segments = vec!["repos", GITHUB_ACCOUNT, repo_name, "contents"]; + segments.extend(path.split('/').filter(|s| !s.is_empty())); + crate::download::build_url(GITHUB_API, &segments) +} + /// Fetch a file from a repo, trying multiple candidate paths. /// Returns the decoded UTF-8 content of the first file found, or None if all return 404. async fn fetch_repo_file_candidates( @@ -268,7 +282,13 @@ async fn fetch_repo_file_candidates( candidates: &[&str], ) -> Result> { for path in candidates { - let url = format!("{GITHUB_API}/repos/{GITHUB_ACCOUNT}/{repo_name}/contents/{path}"); + let url = match contents_url(repo_name, path) { + Ok(u) => u, + Err(e) => { + tracing::warn!("skipping unusable path {path:?} for {repo_name}: {e}"); + continue; + } + }; match cached_get(client, &url).await { Ok((body, _)) => { let content: GithubContent = serde_json::from_str(&body)?; @@ -305,7 +325,13 @@ async fn fetch_icon( candidates.push("icon.png"); } for path in candidates { - let url = format!("{GITHUB_API}/repos/{GITHUB_ACCOUNT}/{repo_name}/contents/{path}"); + let url = match contents_url(repo_name, path) { + Ok(u) => u, + Err(e) => { + tracing::warn!("skipping unusable path {path:?} for {repo_name}: {e}"); + continue; + } + }; match cached_get(client, &url).await { Ok((body, _)) => { let content: GithubContent = serde_json::from_str(&body)?; diff --git a/src/github/releases.rs b/src/github/releases.rs index dd226c5..1f52a19 100644 --- a/src/github/releases.rs +++ b/src/github/releases.rs @@ -67,10 +67,17 @@ pub async fn fetch_release_info( repo_name: &str, tag: &str, ) -> Result { + // `tag` is remote data (colony.json) and this request carries the user's + // bearer token, so build the path from encoded segments: interpolating it + // lets `..` shorten the path onto a different endpoint entirely. See + // `crate::download::build_url`. let url = if tag.eq_ignore_ascii_case("latest") { format!("{GITHUB_API}/repos/{GITHUB_ACCOUNT}/{repo_name}/releases/latest") } else { - format!("{GITHUB_API}/repos/{GITHUB_ACCOUNT}/{repo_name}/releases/tags/{tag}") + crate::download::build_url( + GITHUB_API, + &["repos", GITHUB_ACCOUNT, repo_name, "releases", "tags", tag], + )? }; let (body, _) = cached_get(client, &url).await?; @@ -103,8 +110,16 @@ pub async fn fetch_release_info( const NON_INSTALLABLE_SUFFIXES: &[&str] = &[ ".sig", ".asc", + // The signed metadata sidecar (see crate::signing). Colony's own releases + // already ship `colony-linux.meta`, so the day an ecosystem app adopts the + // sidecar, a documented legacy substring pattern like "linux" would start + // matching two assets and fail with "Ambiguous pattern" - which is exactly + // what docs/colony-spec.md promises cannot happen. + ".meta", ".sha256", ".sha256sum", + // electron-builder publishes one per installer (SphereCord already does). + ".blockmap", ".txt", ".yml", ".yaml", diff --git a/src/oauth.rs b/src/oauth.rs index abbd95a..96f2636 100644 --- a/src/oauth.rs +++ b/src/oauth.rs @@ -12,12 +12,27 @@ const DEVICE_CODE_URL: &str = "https://github.com/login/device/code"; const TOKEN_URL: &str = "https://github.com/login/oauth/access_token"; /// Stored OAuth session. -#[derive(Debug, Clone)] +#[derive(Clone)] pub struct OAuthSession { pub access_token: String, pub username: Option, } +/// Hand-written so the token can never be printed. `Message` derives Debug and +/// carries an `OAuthSession` in `GitHubLoginCompleted`, so the single most +/// natural debugging line anyone will ever add to `App::update` - +/// `tracing::debug!(?message)` - would otherwise put the plaintext token on +/// stderr and into the log file for any user running with RUST_LOG=debug. The +/// redaction is inherited by every type that contains a session. +impl std::fmt::Debug for OAuthSession { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("OAuthSession") + .field("access_token", &"") + .field("username", &self.username) + .finish() + } +} + /// Pending device code, returned by the first step of the flow. #[derive(Debug, Clone)] pub struct DeviceCode { @@ -282,16 +297,19 @@ fn write_private(path: &std::path::Path, contents: &[u8]) -> Result<()> { // Remove any pre-existing file first so `.mode(0o600)` on create always // applies (a truncated-open of a 0644 file would keep the loose bits). let _ = std::fs::remove_file(path); + // create_new, not create: with `create`, a symlink that wins the race + // between the unlink above and this open is FOLLOWED, and `.mode()` + // does not apply to an already-existing inode - so the token would be + // written through the link, at the link target's permissions. The + // unlink makes create_new succeed on the normal path, and because it + // guarantees a fresh inode, the mode it sets is the mode that sticks + // (the old set_permissions re-assertion chmod'd the link's target). let mut f = std::fs::OpenOptions::new() .write(true) - .create(true) - .truncate(true) + .create_new(true) .mode(0o600) .open(path)?; f.write_all(contents)?; - // Re-assert perms in case the file pre-existed with looser bits. - use std::os::unix::fs::PermissionsExt; - std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o600))?; Ok(()) } #[cfg(not(unix))] diff --git a/src/persistence.rs b/src/persistence.rs index 10afdc4..e520ffd 100644 --- a/src/persistence.rs +++ b/src/persistence.rs @@ -375,6 +375,13 @@ fn desktop_value(raw: &str) -> Result { if matches!(c, '\\' | '"' | '`' | '$') { out.push('\\'); } + // The Desktop Entry spec writes a literal percent as `%%`. Unescaped, + // the launcher expands the FIELD CODE instead: a manifest declaring + // `binary: "app%f"` yields Exec="/…/app%f", glib substitutes an empty + // file list, and the entry silently launches the wrong path. + if c == '%' { + out.push('%'); + } out.push(c); } Ok(out) @@ -517,6 +524,8 @@ mod tests { assert!(desktop_value("app\nExec=sh").is_err()); assert!(desktop_value("app\rExec=sh").is_err()); assert!(desktop_value("app\0").is_err()); + // A literal percent must be doubled, or the field code is expanded. + assert_eq!(desktop_value("app%f").unwrap(), "app%%f"); } #[test] From 5cabb5ce93f7f762e855769a3753c6630cb364c1 Mon Sep 17 00:00:00 2001 From: MotherSphere Date: Thu, 27 Aug 2026 16:12:50 +0200 Subject: [PATCH 05/18] feat(github): persist the conditional-request cache across launches HTTP_CACHE was a process-local LazyLock that nothing ever serialised, so a cold boot and a warm boot cost exactly the same number of GitHub requests. Replaying the org catalog against the live org measures ~55 requests for a full refresh, against an anonymous budget of 60/h - so opening Colony twice within the hour rate-limited the second launch, and the user got a stale catalog. GitHub does not bill a 304, so the map only ever needed to survive the process. It now loads from /cache/http_etags.json at first use and is written back after a successful catalog refresh, where it is most complete. The file is bounded: entries are selected smallest-first so a squeeze drops the handful of giant READMEs rather than the dozens of small manifest responses that make up most of the request count, and any single body over 512 KiB is never persisted. 404s are remembered too, for six hours. The catalog deliberately generates them - several CHANGELOG names, several licence filenames, a second icon path, and colony.json for every repo in the org including those that will never have one - and an ETag structurally cannot help there, because a 404 carries none. That is the difference between comfortably under and comfortably over the anonymous quota. The TTL is the cost: a repo that adds a CHANGELOG is picked up on the next window, or immediately via Settings > Clear caches, which now clears this cache too. --- src/github/http.rs | 171 ++++++++++++++++++++++++++++++++++++-- src/persistence.rs | 26 ++++++ src/update/github_auth.rs | 4 + src/update/store.rs | 3 + 4 files changed, 199 insertions(+), 5 deletions(-) diff --git a/src/github/http.rs b/src/github/http.rs index b54d71d..657de33 100644 --- a/src/github/http.rs +++ b/src/github/http.rs @@ -30,13 +30,116 @@ pub(crate) const MAX_CONCURRENT_REPO_FETCHES: usize = 8; // --- HTTP ETag Cache --- +/// How long a remembered 404 is trusted before Colony probes the URL again. +/// +/// The catalog deliberately generates 404s: it tries several CHANGELOG names, +/// several licence filenames, a second icon path, and colony.json for every +/// repo in the org including the ones that will never have one. GitHub bills +/// each of those against the quota and an ETag can never help, because a 404 +/// carries no ETag to send back. Remembering them is what keeps a cold boot +/// inside the 60/h anonymous budget. The TTL is the cost of the trade: a repo +/// that ADDS a CHANGELOG is picked up on the next window, or immediately via +/// Settings > "Clear caches". +const NOT_FOUND_TTL_SECS: u64 = 6 * 3600; + +/// Skip persisting any single body larger than this. READMEs are the big +/// entries here and a pathological one should not bloat every launch. +const MAX_CACHED_BODY_BYTES: usize = 512 * 1024; + +/// Total on-disk budget for the persisted cache. +const MAX_CACHE_FILE_BYTES: usize = 4 * 1024 * 1024; + +#[derive(Clone, serde::Serialize, serde::Deserialize)] struct CacheEntry { etag: String, body: String, } -static HTTP_CACHE: std::sync::LazyLock>> = - std::sync::LazyLock::new(|| Mutex::new(HashMap::new())); +#[derive(Default, serde::Serialize, serde::Deserialize)] +struct PersistedCache { + /// url -> last ETag and the body it described. + #[serde(default)] + entries: HashMap, + /// url -> unix seconds when GitHub last answered 404. + #[serde(default)] + not_found: HashMap, +} + +fn now_secs() -> u64 { + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap_or_default() + .as_secs() +} + +/// The conditional-request cache, restored from disk at first use. +/// +/// This used to start empty on every launch, which made a cold boot and a warm +/// boot cost exactly the same: ~55 full 200s for the org catalog, against an +/// anonymous budget of 60/h. Opening Colony twice within the hour rate-limited +/// the second launch. GitHub does not bill a 304, so replaying the ETags turns +/// almost the whole refresh free - it only ever needed to survive the process. +static HTTP_CACHE: std::sync::LazyLock> = std::sync::LazyLock::new(|| { + Mutex::new(crate::persistence::load_http_cache_json().unwrap_or_default()) +}); + +/// Write the conditional-request cache to disk. Called after a successful +/// catalog refresh, where the map is at its most useful and most complete. +pub fn save_http_cache() { + let Ok(cache) = HTTP_CACHE.lock() else { + return; + }; + let cutoff = now_secs().saturating_sub(NOT_FOUND_TTL_SECS); + let snapshot = PersistedCache { + entries: bound_entries(&cache.entries, MAX_CACHE_FILE_BYTES), + not_found: cache + .not_found + .iter() + .filter(|(_, &seen)| seen > cutoff) + .map(|(k, v)| (k.clone(), *v)) + .collect(), + }; + drop(cache); + if let Err(e) = crate::persistence::save_http_cache_json(&snapshot) { + tracing::warn!("could not persist the HTTP cache: {e}"); + } +} + +/// Select the entries that fit the on-disk budget. +/// +/// Smallest first, so a squeeze drops the handful of giant READMEs rather than +/// the dozens of small manifest and release responses that make up most of the +/// request count - saving quota is the point, not saving bytes. +fn bound_entries( + entries: &HashMap, + mut budget: usize, +) -> HashMap { + let mut sorted: Vec<(&String, &CacheEntry)> = entries.iter().collect(); + sorted.sort_by_key(|(url, e)| (e.body.len(), url.as_str())); + let mut keep = HashMap::new(); + for (url, entry) in sorted { + if entry.body.len() > MAX_CACHED_BODY_BYTES { + continue; + } + let cost = url.len() + entry.etag.len() + entry.body.len(); + if cost > budget { + break; + } + budget -= cost; + keep.insert(url.clone(), entry.clone()); + } + keep +} + +/// Drop every remembered response. Wired to Settings > "Clear caches" so a user +/// who suspects staleness has a button, including for remembered 404s. +pub fn clear_http_cache() { + if let Ok(mut cache) = HTTP_CACHE.lock() { + cache.entries.clear(); + cache.not_found.clear(); + } + let _ = crate::persistence::save_http_cache_json(&PersistedCache::default()); +} /// Per-URL lock to prevent concurrent requests to the same endpoint. static URL_LOCKS: std::sync::LazyLock>>>> = @@ -72,7 +175,16 @@ pub(crate) async fn cached_get( // Add If-None-Match if we have a cached ETag if let Ok(cache) = HTTP_CACHE.lock() { - if let Some(entry) = cache.get(url) { + // A 404 we saw recently costs nothing to answer from here, and unlike a + // 200 it can never be revalidated with an ETag. + if let Some(&seen) = cache.not_found.get(url) { + if now_secs().saturating_sub(seen) < NOT_FOUND_TTL_SECS { + tracing::debug!("negative cache hit for {url}"); + return Err(anyhow::Error::new(HttpStatus(404)) + .context(format!("GitHub API error 404 (remembered): {url}"))); + } + } + if let Some(entry) = cache.entries.get(url) { request = request.header("If-None-Match", &entry.etag); } } @@ -105,7 +217,7 @@ pub(crate) async fn cached_get( 304 => { // Not Modified — return cached body if let Ok(cache) = HTTP_CACHE.lock() { - if let Some(entry) = cache.get(url) { + if let Some(entry) = cache.entries.get(url) { tracing::debug!("Cache hit (304) for {}", url); return Ok((entry.body.clone(), rate_limit)); } @@ -123,7 +235,9 @@ pub(crate) async fn cached_get( // Store in cache if we got an ETag if let Some(etag) = etag { if let Ok(mut cache) = HTTP_CACHE.lock() { - cache.insert( + // The resource exists again; drop any remembered 404. + cache.not_found.remove(url); + cache.entries.insert( url.to_string(), CacheEntry { etag, @@ -159,6 +273,12 @@ pub(crate) async fn cached_get( } } } + if status == 404 { + if let Ok(mut cache) = HTTP_CACHE.lock() { + cache.not_found.insert(url.to_string(), now_secs()); + cache.entries.remove(url); + } + } let body = resp.text().await.unwrap_or_default(); Err(anyhow::Error::new(HttpStatus(status)) .context(format!("GitHub API error {status}: {body}"))) @@ -238,3 +358,44 @@ pub(crate) fn build_client(token: Option<&str>) -> Result { .connect_timeout(CONNECT_TIMEOUT) .build()?) } + +#[cfg(test)] +mod tests { + use super::*; + + fn entry(body_len: usize) -> CacheEntry { + CacheEntry { + etag: "e".into(), + body: "x".repeat(body_len), + } + } + + #[test] + fn the_cache_budget_keeps_the_many_small_entries_over_the_one_huge_one() { + let mut entries = HashMap::new(); + entries.insert("u/big".to_string(), entry(900)); + entries.insert("u/a".to_string(), entry(10)); + entries.insert("u/b".to_string(), entry(10)); + entries.insert("u/c".to_string(), entry(10)); + + // Room for the three small ones and their keys, not for the big one. + let kept = bound_entries(&entries, 100); + assert_eq!(kept.len(), 3, "kept: {:?}", kept.keys().collect::>()); + assert!(!kept.contains_key("u/big")); + + // A single body over the per-entry ceiling is never persisted, however + // much room is left. + let mut huge = HashMap::new(); + huge.insert("u/huge".to_string(), entry(MAX_CACHED_BODY_BYTES + 1)); + assert!(bound_entries(&huge, MAX_CACHE_FILE_BYTES).is_empty()); + } + + #[test] + fn a_remembered_404_expires() { + let now = now_secs(); + let fresh = now.saturating_sub(NOT_FOUND_TTL_SECS / 2); + let stale = now.saturating_sub(NOT_FOUND_TTL_SECS + 1); + assert!(now.saturating_sub(fresh) < NOT_FOUND_TTL_SECS); + assert!(now.saturating_sub(stale) >= NOT_FOUND_TTL_SECS); + } +} diff --git a/src/persistence.rs b/src/persistence.rs index e520ffd..579d337 100644 --- a/src/persistence.rs +++ b/src/persistence.rs @@ -222,6 +222,32 @@ pub fn load_repos_cache() -> Option> { Some(repos) } +fn http_cache_path() -> Result { + let cache_dir = colony_data_dir()?.join("cache"); + std::fs::create_dir_all(&cache_dir)?; + Ok(cache_dir.join("http_etags.json")) +} + +/// Load the persisted conditional-request cache (see `github::http`). +/// +/// Generic so the cache's shape stays private to the HTTP layer - this module +/// only knows where the file goes. A corrupt or older-shaped file simply +/// deserialises to `None` and the cache starts empty, which costs quota but is +/// never wrong. +pub fn load_http_cache_json() -> Option { + let path = http_cache_path().ok()?; + let content = std::fs::read_to_string(&path).ok()?; + serde_json::from_str(&content).ok() +} + +/// Persist the conditional-request cache. The caller is responsible for +/// bounding what it hands over. +pub fn save_http_cache_json(value: &T) -> Result<()> { + let path = http_cache_path()?; + std::fs::write(&path, serde_json::to_string(value)?)?; + Ok(()) +} + fn favorites_path() -> Result { let dir = colony_data_dir()?.join("preferences"); std::fs::create_dir_all(&dir)?; diff --git a/src/update/github_auth.rs b/src/update/github_auth.rs index 2559605..bcbd12c 100644 --- a/src/update/github_auth.rs +++ b/src/update/github_auth.rs @@ -109,6 +109,10 @@ impl App { ) -> Task { self.repos_refresh_manual = false; self.is_fetching_repos = false; + // The catalog refresh is where the conditional-request cache is at its + // most complete: persisting it here means the next launch replays those + // ETags as 304s, which GitHub does not bill. + crate::github::save_http_cache(); let count = repos.len(); if let Err(e) = crate::persistence::save_repos_cache(&repos) { tracing::warn!("Failed to save repos cache: {e}"); diff --git a/src/update/store.rs b/src/update/store.rs index abb668c..7545bc6 100644 --- a/src/update/store.rs +++ b/src/update/store.rs @@ -323,6 +323,9 @@ impl App { pub(super) fn clear_store_caches(&mut self) -> Task { let removed = crate::persistence::clear_store_caches(); + // Including the remembered 404s: a user who clears caches because a + // repo just added a CHANGELOG should not wait out the negative TTL. + crate::github::clear_http_cache(); self.app_icons.clear(); self.release_notes.clear(); self.detail_md_source = None; From ecef473ba3660b493adce32cc414b5d41227c682 Mon Sep 17 00:00:00 2001 From: MotherSphere Date: Thu, 27 Aug 2026 16:20:03 +0200 Subject: [PATCH 06/18] feat(download): resume interrupted transfers A dropped connection used to throw away every byte received. download_to_file opened the staging file with create_new (which unlinks first) and every failure path deleted the partial, so a drop at 95% of a 40 MB asset cost the user 38 MB and the next click re-paid the full 40. There was no retry either, so 'the network blipped' was indistinguishable from 'this app cannot be installed'. Transfers now resume. A partial file survives a transport failure alongside a small identity sidecar recording the server's ETag and the total length; the next attempt sends Range and appends. download_with_resume wraps that in three bounded attempts with backoff, so an ordinary blip is invisible to the user rather than a failed install. Resuming means stitching two responses into one file, so identity is checked rather than assumed. If-Range is sent but NOT trusted: GitHub redirects release assets to Azure blob storage, which ignores the header and answers 206 to a stale validator just the same - verified against a real release asset. So a 206 is only accepted when the response's own ETag and the total in Content-Range still match what the partial file was recorded against; on any disagreement the partial is discarded and the retry starts clean. Nothing weakens verification: the signature and digest still run over the completed file. Covered by a test that stands up a truncating Range server on std::net (no new dependency) and asserts the stitched file is byte-identical. Also in this batch: - Advertised bodies over 4 GiB are refused up front, and the stream aborts if one exceeds the ceiling anyway. The whole asset is written to disk before any check can run, so a release could otherwise fill the user's partition on its own say-so. - The launcher download stages to .part and is renamed only once complete. It used to write straight to the path apply_launcher_update consumes. - prune_staging() runs at boot and removes orphaned .part/.new/.old files under apps/ and update-staging/, plus the launcher's own backup. Cancel was the only sweep that existed, so a crash or a closed window leaked the whole partial asset with no UI that showed it. The launcher backup is removed by exact path, never by sweeping the executable's directory - which may well be /usr/bin. - The .colony_asset marker is written BEFORE .colony_version. The two writes are individually non-atomic, and in the old order a kill between them left a filePattern app claiming the new version while the asset marker still named the old binary - so Colony reported the new version, Launch ran the old one, and no update was offered to correct it. This order makes a torn install simply re-offer the update. --- src/app.rs | 11 ++ src/download.rs | 422 ++++++++++++++++++++++++++++++++++++++++++-- src/persistence.rs | 66 +++++++ src/update/store.rs | 8 +- 4 files changed, 485 insertions(+), 22 deletions(-) diff --git a/src/app.rs b/src/app.rs index 9ec3fc8..43f2bf5 100644 --- a/src/app.rs +++ b/src/app.rs @@ -227,6 +227,17 @@ impl App { app.reload_app_icons(); app.refresh_install_status(); + // Nothing is in flight yet, so any staging file on disk is left over + // from a crash, an OOM, or a window closed mid-download. Cancel was the + // only thing that ever swept them. + let reclaimed = crate::persistence::prune_staging(); + if reclaimed > 0 { + tracing::info!( + "reclaimed {} from interrupted transfers", + state::human_bytes(reclaimed) + ); + } + set_active_theme(&app.selected_theme, &app.selected_variant); set_high_contrast(app.high_contrast); if !app.auto_accent { diff --git a/src/download.rs b/src/download.rs index d5d087f..9144fe8 100644 --- a/src/download.rs +++ b/src/download.rs @@ -33,10 +33,83 @@ fn download_client() -> Result { .build()?) } -/// Stream an HTTP GET to `dest_path`, sending throttled progress (0.0..1.0) -/// over `progress_tx`. Verifies the received length against Content-Length when -/// present and rejects empty/truncated downloads. Removes `dest_path` on any -/// failure. Shared by app-asset install and launcher self-update. +/// Sanity ceiling on an advertised body. The whole asset is written to disk +/// before any signature or digest check can run - that is unavoidable for a +/// detached signature - so a misconfigured or hostile release could otherwise +/// fill the user's home partition on its own say-so. Generous: the largest +/// asset the org ships is two orders of magnitude below this. +const MAX_ASSET_BYTES: u64 = 4 * 1024 * 1024 * 1024; + +/// Identity of the transfer a `.part` file belongs to, written beside it. +/// +/// Resuming means stitching two responses into one file, so it is only sound +/// while both halves describe the same artifact. A re-tagged release, or an +/// asset rebuilt under the same name, must restart from zero rather than +/// produce a file that never existed anywhere. +#[derive(PartialEq, Eq)] +struct PartIdentity { + etag: String, + total: u64, +} + +impl PartIdentity { + fn path(dest: &std::path::Path) -> PathBuf { + let mut name = dest.as_os_str().to_os_string(); + name.push(".id"); + PathBuf::from(name) + } + + fn read(dest: &std::path::Path) -> Option { + let raw = std::fs::read_to_string(Self::path(dest)).ok()?; + let (etag, total) = raw.split_once('\n')?; + Some(Self { + etag: etag.to_string(), + total: total.trim().parse().ok()?, + }) + } + + fn write(&self, dest: &std::path::Path) { + let _ = std::fs::write(Self::path(dest), format!("{}\n{}\n", self.etag, self.total)); + } + + fn forget(dest: &std::path::Path) { + let _ = std::fs::remove_file(Self::path(dest)); + } +} + +/// Whether a 206 response still describes the artifact a partial file belongs +/// to. Both the validator and the total length must agree. +fn response_matches_identity(resp: &reqwest::Response, id: &PartIdentity) -> bool { + let etag = resp + .headers() + .get(reqwest::header::ETAG) + .and_then(|v| v.to_str().ok()); + if etag != Some(id.etag.as_str()) { + return false; + } + // "bytes -/" - the total is the part we can compare. + resp.headers() + .get(reqwest::header::CONTENT_RANGE) + .and_then(|v| v.to_str().ok()) + .and_then(|v| v.rsplit('/').next()) + .and_then(|total| total.trim().parse::().ok()) + == Some(id.total) +} + +/// Discard a partial transfer and the identity that described it. +fn discard_partial(dest_path: &std::path::Path) { + let _ = std::fs::remove_file(dest_path); + PartIdentity::forget(dest_path); +} + +/// Stream an HTTP GET to `dest_path`, sending throttled progress over +/// `progress_tx`, resuming a previous partial transfer when one is present and +/// provably describes the same bytes. +/// +/// Verifies the received length against Content-Length when present and rejects +/// empty/truncated downloads. A partial file is KEPT on a transport failure so +/// the next attempt can continue it, and removed on anything that makes it +/// meaningless. Shared by app-asset install and launcher self-update. async fn download_to_file( client: &reqwest::Client, url: &str, @@ -44,10 +117,35 @@ async fn download_to_file( dest_path: &std::path::Path, progress_tx: Option)>>, ) -> Result<()> { + // What is already on disk, and may we continue it? Both the server's ETag + // and the total length must match what the previous attempt recorded. + let resume_from = match ( + std::fs::metadata(dest_path).map(|m| m.len()).ok(), + PartIdentity::read(dest_path), + ) { + (Some(have), Some(id)) if have > 0 && have < id.total => Some((have, id)), + // Anything else - no identity file, a complete or over-long file, an + // empty one - is not resumable. Start clean. + _ => { + discard_partial(dest_path); + None + } + }; + let mut request = client.get(url); if let Some(t) = token { request = request.header(reqwest::header::AUTHORIZATION, format!("Bearer {t}")); } + if let Some((have, ref id)) = resume_from { + // If-Range is sent, but is NOT trusted: GitHub redirects release assets + // to Azure blob storage, which ignores the header and answers 206 to a + // stale validator just the same (verified against a real release + // asset). So the 206 branch below re-checks the ETag and the total + // itself rather than taking the status code as proof. + request = request + .header(reqwest::header::RANGE, format!("bytes={have}-")) + .header(reqwest::header::IF_RANGE, id.etag.clone()); + } let resp = request.send().await.map_err(|e| { if e.is_timeout() { @@ -61,14 +159,66 @@ async fn download_to_file( anyhow::bail!("Download failed: HTTP {} for {url}", resp.status()); } - let total = resp.content_length(); - let mut downloaded: u64 = 0; + // A 206 is a claim, not a proof - the storage backend ignores If-Range. + // Accept it only when the response still describes the artifact the partial + // file belongs to: same ETag, and the same total in Content-Range. On any + // disagreement, throw the partial away and let the retry start clean rather + // than stitching two different bodies into a file that never existed. + let resuming = if resp.status().as_u16() == 206 { + match resume_from { + Some((_, ref id)) if response_matches_identity(&resp, id) => true, + Some(_) => { + discard_partial(dest_path); + anyhow::bail!("The release changed while downloading {url}; restarting"); + } + None => false, + } + } else { + false + }; + let already: u64 = if resuming { + resume_from.as_ref().map(|(have, _)| *have).unwrap_or(0) + } else { + 0 + }; + + // Total size of the WHOLE asset, not of this response. + let total = if resuming { + resume_from.as_ref().map(|(_, id)| id.total) + } else { + resp.content_length() + }; + + if let Some(total) = total { + anyhow::ensure!( + total <= MAX_ASSET_BYTES, + "Refusing {url}: the release advertises {total} bytes, over Colony's {MAX_ASSET_BYTES}-byte ceiling" + ); + } use futures::StreamExt; use std::io::Write; - // Staging names are predictable, so never follow whatever sits there. - let mut file = create_new_file(dest_path)?; + let mut file = if resuming { + tracing::info!("resuming {url} at {already} bytes"); + std::fs::OpenOptions::new().append(true).open(dest_path)? + } else { + // Fresh transfer. Record what we are about to fetch so a later attempt + // can tell whether continuing this file is sound, and never follow + // whatever sits at the (predictable) staging name. + discard_partial(dest_path); + if let (Some(etag), Some(total)) = ( + resp.headers() + .get(reqwest::header::ETAG) + .and_then(|v| v.to_str().ok()) + .map(str::to_string), + resp.content_length(), + ) { + PartIdentity { etag, total }.write(dest_path); + } + create_new_file(dest_path)? + }; let mut stream = resp.bytes_stream(); + let mut downloaded: u64 = already; let mut last_pct: u32 = 0; let stream_result: Result<()> = async { @@ -76,6 +226,10 @@ async fn download_to_file( let chunk = chunk?; file.write_all(&chunk)?; downloaded += chunk.len() as u64; + anyhow::ensure!( + downloaded <= MAX_ASSET_BYTES, + "Refusing {url}: body exceeded Colony's {MAX_ASSET_BYTES}-byte ceiling" + ); if let Some(ref tx) = progress_tx { // Throttle: send on whole-percent changes when the total is @@ -111,25 +265,77 @@ async fn download_to_file( .await; if let Err(e) = stream_result { - let _ = std::fs::remove_file(dest_path); + // KEEP the partial file. A drop at 95% used to throw away everything + // and charge the user the full asset again on the next click; with the + // identity sidecar beside it, the next attempt continues instead. Only + // a transfer we can no longer describe is discarded. + if PartIdentity::read(dest_path).is_none() { + discard_partial(dest_path); + } return Err(e); } - // Guard against a silently-truncated or empty transfer. + // Guard against a silently-truncated or empty transfer. Both are terminal + // for THIS file: a short body means the server disagrees with the length it + // advertised, so continuing from it would be guesswork. if let Some(total) = total { if downloaded != total { - let _ = std::fs::remove_file(dest_path); + discard_partial(dest_path); anyhow::bail!("Incomplete download: got {downloaded} of {total} bytes for {url}"); } } if downloaded == 0 { - let _ = std::fs::remove_file(dest_path); + discard_partial(dest_path); anyhow::bail!("Empty download (0 bytes) for {url}"); } + // Complete: the identity file has done its job. + PartIdentity::forget(dest_path); Ok(()) } +/// `download_to_file` with a bounded retry, so a dropped connection continues +/// from where it stopped without the user having to notice and click again. +/// +/// Only transport failures are retried, and only because the partial file now +/// survives them: each attempt resumes from what the previous one wrote, so +/// three tries cost three connections, not three assets. A verification failure +/// never reaches here - it happens after this returns. +async fn download_with_resume( + client: &reqwest::Client, + url: &str, + token: Option<&str>, + dest_path: &std::path::Path, + progress_tx: Option)>>, +) -> Result<()> { + const ATTEMPTS: u32 = 3; + let mut last_err = None; + for attempt in 0..ATTEMPTS { + if attempt > 0 { + // Linear backoff; the point is to ride out a blip, not to hammer. + tokio::time::sleep(Duration::from_secs(2 * attempt as u64)).await; + tracing::info!("retrying {url} (attempt {} of {ATTEMPTS})", attempt + 1); + } + let had_partial = PartIdentity::read(dest_path).is_some(); + match download_to_file(client, url, token, dest_path, progress_tx.clone()).await { + Ok(()) => return Ok(()), + Err(e) => { + last_err = Some(e); + // Retry when the next attempt would do something DIFFERENT: + // either a partial survived and will be continued, or one was + // just discarded (the release changed under us) and the next + // attempt starts clean. Otherwise - a 404, a refused URL - the + // next attempt would only repeat this one. + let resumable = PartIdentity::read(dest_path).is_some(); + if !resumable && !had_partial { + break; + } + } + } + } + Err(last_err.unwrap_or_else(|| anyhow::anyhow!("Download failed for {url}"))) +} + /// Ceiling for the small sidecars Colony buffers whole (`.sig` is 64 bytes, /// `.meta` three short lines). Whoever controls a release can publish a /// multi-gigabyte file named `foo-linux.sig`; without a cap that is an OOM the @@ -549,7 +755,7 @@ pub async fn download_release_asset( )?; let client = download_client()?; - download_to_file(&client, &url, token.as_deref(), &temp_path, progress_tx).await?; + download_with_resume(&client, &url, token.as_deref(), &temp_path, progress_tx).await?; // `manifest.signed` lives in the very repo the signature protects, so a // compromised repo could flip it to false and drop the `.sig` to install @@ -662,10 +868,21 @@ pub async fn download_release_asset( // installed binary with no version file, silently excluded from // every future update check (or, in the filePattern case, an // orphaned binary the app no longer even sees as installed). - crate::persistence::save_installed_version(&repo_name, &tag)?; + // + // Order matters: the ASSET marker lands first and the VERSION + // marker last. The two writes are individually non-atomic, so a + // kill between them leaves a torn state - and only this order makes + // that state honest. With the version written first, a + // filePattern app whose asset name carries the version would claim + // the new version while `.colony_asset` still named the old file: + // installed_app_path resolves through the asset marker, so Colony + // would report the new version and Launch would run the old binary, + // with no update offered to correct it. This way round, a torn + // install simply re-offers the update. if record_asset { crate::persistence::save_installed_asset(&repo_name, &filename)?; } + crate::persistence::save_installed_version(&repo_name, &tag)?; // Pin the signature requirement for future updates: a repo that // ships signatures today must not be able to stop tomorrow. Only // ever raises the bar - the marker is written, never cleared, while @@ -705,14 +922,31 @@ pub async fn download_launcher_asset( let temp_dir = colony_data_dir()?.join("update-staging"); std::fs::create_dir_all(&temp_dir)?; let dest_path = temp_dir.join(&filename); + // Stream to `.part` and only rename onto the apply path once the + // whole file is here. Writing straight to the final name meant a download + // the user never applied - or a cancelled one - left a partial file at + // exactly the path apply_launcher_update consumes. Apply re-verifies, so it + // was refused fail-closed rather than being a hole, but the file sat there. + let part_path = temp_dir.join(format!("{filename}.part")); - let url = format!( - "https://github.com/{LAUNCHER_OWNER}/{LAUNCHER_REPO}/releases/download/{tag}/{filename}" - ); + let url = build_url( + "https://github.com", + &[ + LAUNCHER_OWNER, + LAUNCHER_REPO, + "releases", + "download", + &tag, + &filename, + ], + )?; let client = download_client()?; - // download_to_file validates the length and rejects an empty/truncated body. - download_to_file(&client, &url, token.as_deref(), &dest_path, progress_tx).await?; + // Validates the length, rejects an empty/truncated body, and resumes a + // previous partial transfer when one provably describes the same asset. + download_with_resume(&client, &url, token.as_deref(), &part_path, progress_tx).await?; + let _ = std::fs::remove_file(&dest_path); + std::fs::rename(&part_path, &dest_path)?; // Fail-closed signature check: fetch the detached signature and verify the // downloaded binary against the embedded release key BEFORE it can be @@ -1012,6 +1246,156 @@ pub fn apply_launcher_update(new_binary: &std::path::Path) -> Result { mod tests { use super::*; + /// A one-shot HTTP server that TRUNCATES the first response and honours + /// Range on the next one - the shape of a real dropped connection. + /// + /// Hand-rolled on `std::net` rather than pulled in as a dependency: the + /// point is to prove the resume path end to end without adding a test-only + /// crate to a project that counts its dependencies. + fn spawn_truncating_server( + body: Vec, + cut: usize, + etag: &str, + ) -> (String, std::thread::JoinHandle<()>) { + use std::io::{BufRead, BufReader, Write}; + let listener = std::net::TcpListener::bind("127.0.0.1:0").expect("bind"); + let url = format!("http://{}/asset", listener.local_addr().unwrap()); + let etag = etag.to_string(); + let handle = std::thread::spawn(move || { + // Two connections: the truncated one, then the ranged one. + for _ in 0..2 { + let Ok((stream, _)) = listener.accept() else { + return; + }; + let mut reader = BufReader::new(&stream); + let mut range_start: Option = None; + loop { + let mut line = String::new(); + if reader.read_line(&mut line).unwrap_or(0) == 0 { + return; + } + if let Some(v) = line.to_ascii_lowercase().strip_prefix("range: bytes=") { + range_start = v.split('-').next().and_then(|n| n.trim().parse().ok()); + } + if line == "\r\n" || line == "\n" { + break; + } + } + let mut stream = &stream; + match range_start { + Some(start) => { + let chunk = &body[start..]; + let head = format!( + "HTTP/1.1 206 Partial Content\r\nContent-Range: bytes {}-{}/{}\r\nContent-Length: {}\r\nETag: {}\r\nConnection: close\r\n\r\n", + start, + body.len() - 1, + body.len(), + chunk.len(), + etag + ); + let _ = stream.write_all(head.as_bytes()); + let _ = stream.write_all(chunk); + } + None => { + let head = format!( + "HTTP/1.1 200 OK\r\nContent-Length: {}\r\nETag: {}\r\nConnection: close\r\n\r\n", + body.len(), + etag + ); + let _ = stream.write_all(head.as_bytes()); + // Promise the whole body, deliver part of it, hang up. + let _ = stream.write_all(&body[..cut]); + } + } + let _ = stream.flush(); + } + }); + (url, handle) + } + + /// The behaviour this whole batch exists for: a connection that died at 40% + /// used to throw away every byte and charge the user the full asset again. + #[test] + fn a_dropped_connection_is_resumed_instead_of_restarted() { + let body: Vec = (0..300_000usize) + .map(|i| ((i * 7 + 3) % 256) as u8) + .collect(); + let (url, server) = spawn_truncating_server(body.clone(), 120_000, "\"v1\""); + + let dir = std::env::temp_dir().join("colony_test_resume"); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let dest = dir.join("asset.part"); + + let rt = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .unwrap(); + let result = rt.block_on(async { + let client = download_client().unwrap(); + download_with_resume(&client, &url, None, &dest, None).await + }); + + assert!( + result.is_ok(), + "the retry must finish the transfer: {result:?}" + ); + assert_eq!( + std::fs::read(&dest).unwrap(), + body, + "the stitched file must be byte-identical to the asset" + ); + assert!( + !PartIdentity::path(&dest).exists(), + "a completed transfer leaves no identity file behind" + ); + + let _ = std::fs::remove_dir_all(&dir); + let _ = server.join(); + } + + /// Resuming stitches two responses into one file, so it is only sound + /// while both describe the same artifact. The identity sidecar is what + /// makes that decidable; without it, a re-tagged release could be silently + /// assembled from two different bodies. + #[test] + fn a_partial_transfer_is_only_resumable_against_its_own_identity() { + let dir = std::env::temp_dir().join("colony_test_part_identity"); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let part = dir.join("grape-linux.part"); + + // A partial transfer plus the identity that describes it. + std::fs::write(&part, vec![0u8; 512]).unwrap(); + PartIdentity { + etag: "\"abc\"".into(), + total: 4096, + } + .write(&part); + + let id = PartIdentity::read(&part).expect("identity round-trips"); + assert_eq!(id.etag, "\"abc\""); + assert_eq!(id.total, 4096); + assert!( + std::fs::metadata(&part).unwrap().len() < id.total, + "a short file against a known total is what makes a resume possible" + ); + + // Discarding takes the sidecar with it, so the next attempt cannot + // resume from bytes nothing describes. + discard_partial(&part); + assert!(!part.exists()); + assert!(PartIdentity::read(&part).is_none()); + + // A truncated or garbage sidecar is not an identity. + std::fs::write(PartIdentity::path(&part), "no-newline-here").unwrap(); + assert!(PartIdentity::read(&part).is_none()); + std::fs::write(PartIdentity::path(&part), "\"abc\"\nnot-a-number\n").unwrap(); + assert!(PartIdentity::read(&part).is_none()); + + let _ = std::fs::remove_dir_all(&dir); + } + /// A `tag` from colony.json used to be interpolated straight into the /// release URL. reqwest parses with the WHATWG parser, which collapses /// `..` BEFORE the request is issued, so one line of a catalog repo could diff --git a/src/persistence.rs b/src/persistence.rs index 579d337..3363198 100644 --- a/src/persistence.rs +++ b/src/persistence.rs @@ -464,6 +464,72 @@ pub fn clear_store_caches() -> usize { removed } +/// Delete staging leftovers from interrupted transfers, and report the bytes +/// reclaimed. +/// +/// The only sweep that existed ran inside Cancel, so a crash, an OOM, a SIGKILL +/// or simply closing the window mid-download left the whole partial asset in +/// `apps//` with no UI that showed or removed it. A `filePattern` app +/// whose asset name carries the version leaves one per version, so it +/// accumulates. Run once at boot: any staging file present then is by +/// definition orphaned, because nothing is in flight yet. +/// +/// The `update-staging` directory gets the same treatment, plus the `.old` +/// backup a completed self-update leaves next to the executable - two full +/// copies of the launcher could otherwise sit on disk indefinitely. +pub fn prune_staging() -> u64 { + fn sweep(dir: &std::path::Path, reclaimed: &mut u64) { + let Ok(entries) = std::fs::read_dir(dir) else { + return; + }; + for entry in entries.flatten() { + let path = entry.path(); + let Some(name) = path.file_name().and_then(|n| n.to_str()) else { + continue; + }; + if !(name.ends_with(".part") + || name.ends_with(".part.id") + || name.ends_with(".new") + || name.ends_with(".old")) + { + continue; + } + let size = entry.metadata().map(|m| m.len()).unwrap_or(0); + if std::fs::remove_file(&path).is_ok() { + tracing::info!("pruned staging leftover {}", path.display()); + *reclaimed += size; + } + } + } + + let mut reclaimed = 0; + if let Ok(apps) = colony_apps_dir() { + if let Ok(entries) = std::fs::read_dir(&apps) { + for entry in entries.flatten() { + if entry.path().is_dir() { + sweep(&entry.path(), &mut reclaimed); + } + } + } + } + if let Ok(base) = colony_data_dir() { + sweep(&base.join("update-staging"), &mut reclaimed); + } + // Only our OWN backup, by exact path - never a directory sweep here. The + // executable may well live in /usr/bin, where an extension-based sweep + // would happily delete somebody else's `.old` file. + if let Ok(exe) = std::env::current_exe() { + for stale in [exe.with_extension("old"), exe.with_extension("new")] { + let size = std::fs::metadata(&stale).map(|m| m.len()).unwrap_or(0); + if stale.exists() && std::fs::remove_file(&stale).is_ok() { + tracing::info!("pruned launcher leftover {}", stale.display()); + reclaimed += size; + } + } + } + reclaimed +} + /// Remove doc/icon caches for repos that are NO LONGER in the catalog, so a /// deleted or renamed repo does not leave its caches behind forever. Runs /// after each successful catalog fetch (never on a cache fallback, where a diff --git a/src/update/store.rs b/src/update/store.rs index 7545bc6..7c2dc04 100644 --- a/src/update/store.rs +++ b/src/update/store.rs @@ -221,14 +221,16 @@ impl App { if let Some(handle) = self.download_abort.take() { handle.abort(); } - // The aborted task cannot clean up its staging file: sweep - // the cancelled repo's *.part leftovers here. + // The aborted task cannot clean up its staging file: sweep the + // cancelled repo's leftovers here, including the `.part.id` sidecar + // that would otherwise invite a resume of a transfer the user stopped + // on purpose. if let Some(repo) = self.downloading_repo.take() { if let Ok(app_dir) = crate::persistence::colony_app_dir(&repo) { if let Ok(entries) = std::fs::read_dir(&app_dir) { for entry in entries.flatten() { let name = entry.file_name().to_string_lossy().to_string(); - if name.ends_with(".part") { + if name.ends_with(".part") || name.ends_with(".part.id") { let _ = std::fs::remove_file(entry.path()); } } From 9c4ffca5c700ed00a7051304a2bdb8b2de863960 Mon Sep 17 00:00:00 2001 From: MotherSphere Date: Thu, 27 Aug 2026 16:26:02 +0200 Subject: [PATCH 07/18] fix(platform): replace running binaries safely, and stop overpromising Windows/macOS The install path could not update an app the user had left open. A plain std::fs::rename over a live executable fails on Windows - the image is held with FILE_SHARE_READ|FILE_SHARE_DELETE, so MoveFileExW cannot delete the destination - and it failed AFTER downloading and verifying the whole asset, with a raw 'Access is denied' and no hint that closing the app would help. Colony already knew the parade and applied it to its own self-update; replace_file() factors it out so both halves work the same way. Uninstall gets the same treatment: a directory whose binary cannot be deleted has its files renamed aside, and the boot sweep collects them once the process holding them has exited. launcher_is_system_managed() was #[cfg(unix)] and matched on /usr and /opt, so a Windows install under Program Files - not writable unelevated - reported false, offered the update button, and died on the same rename. It is now behavioural: probe whether a file can be created next to the executable. One test covers /usr, /opt, Program Files, a read-only mount and /Applications, with no per-platform path list to keep in sync. keyring was built with only the Secret Service backend. keyring 3 has no default feature, so on Windows and macOS its backend resolved to the in-memory mock backend, whose set_password returns Ok while storing nothing - and Colony reads that Ok as success and then deletes its plaintext fallback on purpose. The token was written nowhere and every restart logged the user out, while the log said 'Token saved to OS keychain'. Both native backends are target-gated, so the Linux build is unchanged and no openssl enters the tree (verified with cargo tree --target all). An app whose asset name carries the version - the case filePattern exists to serve - left its previous binary behind on every update, invisible to the user and reclaimed only by an uninstall. SphereCord's AppImage is 166 MB. The superseded file is now removed once the new one is committed. The self-update relaunch no longer exits unconditionally. apply_launcher_update has already committed the swap by then, so discarding the spawn result meant a binary that would not exec made Colony vanish for good, with no window, no toast, and no way to know a .old backup exists. A failed spawn now surfaces as an error naming both paths. Finally, the platform promise is now honest. The README advertised all three as 'Supported'; Windows and macOS are marked best-effort with the specific gaps named. The shipped categories.json had a hardcoded 'Windows' and 'Linux' section, so a Windows user got a permanently empty 'Linux' entry and a macOS user found /Applications filed under one labelled 'Linux'. Sections take an optional "platforms" list, the three system sections are gated to their own OS, and the missing macOS one is added. Its icons are restored too - the shipped config had regressed every glyph to an empty string while the unreachable built-in fallback kept the real ones. --- Cargo.lock | 46 ++++++++++++++++++++ Cargo.toml | 13 +++++- README.md | 25 +++++++---- config/categories.json | 20 ++++++++- src/download.rs | 98 ++++++++++++++++++++++++++++++++++++------ src/i18n/en.rs | 5 +++ src/i18n/fr.rs | 5 +++ src/i18n/mod.rs | 1 + src/persistence.rs | 39 +++++++++++++++++ src/sections.rs | 73 ++++++++++++++++++++++++++++++- src/update/launcher.rs | 25 ++++++++++- src/update/store.rs | 2 +- 12 files changed, 324 insertions(+), 28 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 1f7d3af..28fb4c6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -387,6 +387,12 @@ dependencies = [ "syn 2.0.114", ] +[[package]] +name = "byteorder" +version = "1.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1fd0f2584146f6f2ef48085050886acf353beff7305ebd1ae69500e27c67f64b" + [[package]] name = "byteorder-lite" version = "0.1.0" @@ -2031,9 +2037,13 @@ version = "3.6.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "eebcc3aff044e5944a8fbaf69eb277d11986064cba30c468730e8b9909fb551c" dependencies = [ + "byteorder", "dbus-secret-service", "log", "openssl", + "security-framework 2.11.1", + "security-framework 3.6.0", + "windows-sys 0.60.2", "zeroize", ] @@ -3462,6 +3472,42 @@ dependencies = [ "tiny-skia", ] +[[package]] +name = "security-framework" +version = "2.11.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "897b2245f0b511c87893af39b033e5ca9cce68824c4d7e7630b5a1d339658d02" +dependencies = [ + "bitflags 2.10.0", + "core-foundation 0.9.4", + "core-foundation-sys", + "libc", + "security-framework-sys", +] + +[[package]] +name = "security-framework" +version = "3.6.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d17b898a6d6948c3a8ee4372c17cb384f90d2e6e912ef00895b14fd7ab54ec38" +dependencies = [ + "bitflags 2.10.0", + "core-foundation 0.10.1", + "core-foundation-sys", + "libc", + "security-framework-sys", +] + +[[package]] +name = "security-framework-sys" +version = "2.17.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6ce2691df843ecc5d231c0b14ece2acc3efb62c0a398c7e1d875f3983ce020e3" +dependencies = [ + "core-foundation-sys", + "libc", +] + [[package]] name = "self_cell" version = "1.2.2" diff --git a/Cargo.toml b/Cargo.toml index 8f02689..f8f66ed 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -19,7 +19,18 @@ futures = "0.3" open = "5" base64 = "0.23" dirs = "6" -keyring = { version = "3", features = ["sync-secret-service", "vendored"] } +# apple-native / windows-native are REQUIRED, not optional: keyring 3 has no +# default feature, and without them its macOS and Windows backends resolve to +# the in-memory `mock`, whose set_password returns Ok while storing nothing. +# Colony reads that Ok as success and deletes its plaintext fallback on purpose, +# so the token was written nowhere and every restart logged the user out. Both +# are target-gated in keyring's manifest, so they add nothing to the Linux build. +keyring = { version = "3", features = [ + "apple-native", + "windows-native", + "sync-secret-service", + "vendored", +] } semver = "1" sha2 = "0.10" # Pure-Rust ed25519 verification for signed launcher self-updates (no openssl). diff --git a/README.md b/README.md index 9bf7bbe..cd66002 100644 --- a/README.md +++ b/README.md @@ -7,7 +7,7 @@ Colony is the central piece of [Project Colony](https://github.com/Project-Colon [![License: GPL-3.0-or-later](https://img.shields.io/badge/License-GPL--3.0--or--later-blue.svg)](LICENSE) [![AUR: colony-bin](https://img.shields.io/badge/AUR-colony--bin-blue)](https://aur.archlinux.org/packages/colony-bin) [![AUR: colony-git](https://img.shields.io/badge/AUR-colony--git-blue)](https://aur.archlinux.org/packages/colony-git) -[![Platforms](https://img.shields.io/badge/platforms-linux%20%7C%20windows%20%7C%20macOS-lightgrey)](#installation) +[![Platforms](https://img.shields.io/badge/platforms-linux%20%7C%20windows*%20%7C%20macOS*-lightgrey)](#platforms)