From 3f21cfa0aef902fe03fb1731ba516e5500f0dfb4 Mon Sep 17 00:00:00 2001 From: Zinadin Zidan Date: Fri, 30 Oct 2026 22:22:32 +0600 Subject: [PATCH 1/3] update: the prompt opens only a safe release link; document token logging (#391) `Release`'s fields are public, so an app that builds one from its own feed or cache could hand any URL to the prompt's "What's new" link and to Action::OpenUrl after Download. Both now go through `release::safe_web_link`, as the launcher does before every open_url. `Http::with_github_token` says that ureq's debug logs print the Bearer token, so apps should redact it or keep ureq below Debug. Co-Authored-By: Claude Opus 5.5 --- crates/update/src/dialog.rs | 32 +++++++++++++++++++++++++++++++- crates/update/src/http.rs | 4 ++++ crates/update/src/updater.rs | 29 +++++++++++++++++++++++++++-- 3 files changed, 62 insertions(+), 3 deletions(-) diff --git a/crates/update/src/dialog.rs b/crates/update/src/dialog.rs index f63da0869..fabdca24b 100644 --- a/crates/update/src/dialog.rs +++ b/crates/update/src/dialog.rs @@ -184,7 +184,10 @@ impl StartupPrompt { let ring = egui::Stroke::new(2.0, text_primary); ui.painter().rect_stroke(link.rect.expand(2.0), egui::CornerRadius::ZERO, ring, egui::StrokeKind::Outside); } - if link.on_hover_cursor(egui::CursorIcon::PointingHand).clicked() { + // `Release`'s fields are public: an app may build one from its own feed or + // cache, so the link is checked here too, not only when parsed from GitHub. + if link.on_hover_cursor(egui::CursorIcon::PointingHand).clicked() && crate::release::safe_web_link(&self.release.html_url) + { ui.ctx().open_url(egui::OpenUrl::new_tab(&self.release.html_url)); } @@ -548,6 +551,33 @@ mod tests { assert!((link.y - button.y).abs() <= 1.0, "link centred at {} but the button at {}", link.y, button.y); } + /// #391: the "What's new" link opens the release page only when it is a safe web link; an app + /// that builds its own `Release` (a feed, a cache) can't hand the browser anything else. + #[test] + fn whats_new_opens_only_a_safe_link() { + let opened = |url: &str| { + let ctx = dark_context(); + let mut prompt = prompt_for(false); + prompt.release.html_url = url.to_string(); + frame(&ctx, &mut prompt, Vec::new()); + let (_, output) = frame(&ctx, &mut prompt, Vec::new()); + let at = text_pos(&output, "What's new").expect("the link is painted"); + let button = |pressed| Event::PointerButton { pos: at, button: egui::PointerButton::Primary, pressed, modifiers: Modifiers::NONE }; + frame(&ctx, &mut prompt, vec![Event::PointerMoved(at)]); + frame(&ctx, &mut prompt, vec![button(true)]); + let (_, output) = frame(&ctx, &mut prompt, vec![button(false)]); + output.platform_output.commands.iter().find_map(|c| match c { + egui::OutputCommand::OpenUrl(u) => Some(u.url.clone()), + _ => None, + }) + }; + let page = "https://github.com/storytold/photocraft/releases/tag/v0.2.0"; + assert_eq!(opened(page).as_deref(), Some(page)); + for bad in ["", "file:///etc/passwd", "javascript:alert(1)", "https://evil.example/a&calc", "https://user@evil.example/"] { + assert_eq!(opened(bad), None, "{bad:?}"); + } + } + #[test] fn clicking_download_answers_download() { let ctx = dark_context(); diff --git a/crates/update/src/http.rs b/crates/update/src/http.rs index 963eed1e5..44846cd1d 100644 --- a/crates/update/src/http.rs +++ b/crates/update/src/http.rs @@ -131,6 +131,10 @@ impl Http { /// An optional token raises GitHub's limit from 60 to 5000 requests an hour. Only ever sent /// to api.github.com. Blank means none. + /// + /// ureq's debug and trace logs print request headers, this token's `Authorization: Bearer` + /// among them: an app that sets one should redact Bearer tokens in its logger, or keep + /// `ureq` and `ureq_proto` below Debug. #[must_use] pub fn with_github_token(mut self, token: Option) -> Self { self.token = token.map(|t| t.trim().to_string()).filter(|t| !t.is_empty()); diff --git a/crates/update/src/updater.rs b/crates/update/src/updater.rs index 636860013..5d61b9a79 100644 --- a/crates/update/src/updater.rs +++ b/crates/update/src/updater.rs @@ -243,9 +243,9 @@ impl Updater { Action::None } StartupPromptResponse::Download => { - let url = prompt.release.html_url.clone(); + let action = download_action(&prompt.release); self.prompt = None; - Action::OpenUrl(url) + action } StartupPromptResponse::Later => { // An installation carries on and reports through `tick`. @@ -308,6 +308,12 @@ fn install(app: &'static App, installed: &self_update::Install, mut release: Rel self_update::update(app, installed, &release, platform, &http, &paths, &Stop::default(), &mut |_| {}, &|| Ok(()), Some(version)) } +/// Download in the prompt: the release page, when it is a safe web link (an app may build its +/// `Release` from its own feed or cache, so the link isn't always one GitHub's parser checked). +fn download_action(release: &Release) -> Action { + if crate::release::safe_web_link(&release.html_url) { Action::OpenUrl(release.html_url.clone()) } else { Action::None } +} + #[cfg(test)] mod tests { use super::*; @@ -326,6 +332,25 @@ mod tests { } } + /// #391: Download opens only a safe web link, whoever built the `Release`. + #[test] + fn download_opens_only_a_safe_release_page() { + let with = |url: &str| Release { html_url: url.into(), ..newer() }; + let page = "https://github.com/storytold/photocraft/releases/tag/v9.0.0"; + assert_eq!(download_action(&with(page)), Action::OpenUrl(page.into())); + for bad in [ + "", + "http://github.com/x", + "file:///C:/Windows/system32/calc.exe", + "javascript:alert(1)", + "https://evil.example/a|calc", + "https://user@evil.example/", + "https://x.example/a b", + ] { + assert_eq!(download_action(&with(bad)), Action::None, "{bad:?}"); + } + } + fn newer() -> Release { Release { tag: "v9.0.0".into(), version: crate::version::parse("9.0.0"), ..Release::default() } } From 7ae5bbf07d3c6833fe311aa9caf031838bae8b6c Mon Sep 17 00:00:00 2001 From: Zinadin Zidan Date: Fri, 30 Oct 2026 23:56:36 +0600 Subject: [PATCH 2/3] update: Download falls back to the app's releases page (#391) Download closes the prompt, so a release link that fails `release::safe_web_link` left the user with nothing opened. It now opens `App::releases_url()` instead, which passes the check for every app in the suite; a release link with `&` or another shell metacharacter is still refused. Co-Authored-By: Claude Opus 5.5 --- crates/update/src/updater.rs | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/crates/update/src/updater.rs b/crates/update/src/updater.rs index 5d61b9a79..3a1c458f0 100644 --- a/crates/update/src/updater.rs +++ b/crates/update/src/updater.rs @@ -243,7 +243,7 @@ impl Updater { Action::None } StartupPromptResponse::Download => { - let action = download_action(&prompt.release); + let action = download_action(self.app, &prompt.release); self.prompt = None; action } @@ -310,8 +310,15 @@ fn install(app: &'static App, installed: &self_update::Install, mut release: Rel /// Download in the prompt: the release page, when it is a safe web link (an app may build its /// `Release` from its own feed or cache, so the link isn't always one GitHub's parser checked). -fn download_action(release: &Release) -> Action { - if crate::release::safe_web_link(&release.html_url) { Action::OpenUrl(release.html_url.clone()) } else { Action::None } +/// A link that fails the check falls back to the app's releases page: the prompt has closed, so +/// Download must still open something. +fn download_action(app: &App, release: &Release) -> Action { + let safe = crate::release::safe_web_link; + if safe(&release.html_url) { + return Action::OpenUrl(release.html_url.clone()); + } + let releases = app.releases_url(); + if safe(&releases) { Action::OpenUrl(releases) } else { Action::None } } #[cfg(test)] @@ -337,7 +344,8 @@ mod tests { fn download_opens_only_a_safe_release_page() { let with = |url: &str| Release { html_url: url.into(), ..newer() }; let page = "https://github.com/storytold/photocraft/releases/tag/v9.0.0"; - assert_eq!(download_action(&with(page)), Action::OpenUrl(page.into())); + assert_eq!(download_action(&PHOTOCRAFT, &with(page)), Action::OpenUrl(page.into())); + let releases = Action::OpenUrl(PHOTOCRAFT.releases_url()); for bad in [ "", "http://github.com/x", @@ -346,11 +354,22 @@ mod tests { "https://evil.example/a|calc", "https://user@evil.example/", "https://x.example/a b", + "https://github.com/storytold/photocraft/releases/tag/v9&calc", ] { - assert_eq!(download_action(&with(bad)), Action::None, "{bad:?}"); + assert_eq!(download_action(&PHOTOCRAFT, &with(bad)), releases, "{bad:?}"); } } + /// #391: the fallback passes the check for every app, so Download always opens a page. + #[test] + fn every_releases_page_is_a_safe_web_link() { + for app in crate::suite::SUITE.iter().chain([&crate::suite::LAUNCHER]) { + assert!(crate::release::safe_web_link(&app.releases_url()), "{}", app.id); + } + let broken = App::new("x", "X", "x", "a|b/c"); + assert_eq!(download_action(&broken, &Release { html_url: "javascript:x".into(), ..newer() }), Action::None); + } + fn newer() -> Release { Release { tag: "v9.0.0".into(), version: crate::version::parse("9.0.0"), ..Release::default() } } From 35b908febb225dbcd658c63e0043d4136bd07179 Mon Sep 17 00:00:00 2001 From: Zinadin Zidan Date: Fri, 30 Oct 2026 23:56:36 +0600 Subject: [PATCH 3/3] update: only ureq's Trace logs print the GitHub token (#391) ureq 3.5's Debug request log lists an allow-list of safe headers and leaves Authorization out; the unredacted wire is its Trace output (ureq-proto's log_data). `Http::with_github_token` now says so: apps should redact Bearer tokens or keep `ureq` and `ureq_proto` at Debug or quieter. Co-Authored-By: Claude Opus 5.5 --- crates/update/src/http.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/crates/update/src/http.rs b/crates/update/src/http.rs index 44846cd1d..fef1ef13b 100644 --- a/crates/update/src/http.rs +++ b/crates/update/src/http.rs @@ -132,9 +132,10 @@ impl Http { /// An optional token raises GitHub's limit from 60 to 5000 requests an hour. Only ever sent /// to api.github.com. Blank means none. /// - /// ureq's debug and trace logs print request headers, this token's `Authorization: Bearer` - /// among them: an app that sets one should redact Bearer tokens in its logger, or keep - /// `ureq` and `ureq_proto` below Debug. + /// ureq's Debug logs list only an allow-list of safe headers and leave `Authorization` out, + /// but its Trace logs are the unredacted wire, this token's `Authorization: Bearer` among it: + /// an app that sets one should redact Bearer tokens in its logger, or keep `ureq` and + /// `ureq_proto` at Debug or quieter. #[must_use] pub fn with_github_token(mut self, token: Option) -> Self { self.token = token.map(|t| t.trim().to_string()).filter(|t| !t.is_empty());