Merge pull request #452 from ZIDAN44/fix/391-safe-release-link

update: the prompt opens only a safe release link; document token logging (#391)

crates/update/src/dialog.rs

@@ -121,7 +121,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));
                             }
 
@@ -715,6 +718,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();

crates/update/src/http.rs

@@ -146,6 +146,11 @@ 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 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<String>) -> Self {
         self.token = token.map(|t| t.trim().to_string()).filter(|t| !t.is_empty());

crates/update/src/updater.rs

@@ -300,9 +300,9 @@ impl Updater {
                 Action::None
             }
             StartupPromptResponse::Download => {
-                let url = prompt.release.html_url.clone();
+                let action = download_action(self.app, &prompt.release);
                 self.prompt = None;
-                Action::OpenUrl(url)
+                action
             }
             StartupPromptResponse::Later => {
                 // An installation carries on and reports through `tick`.
@@ -369,6 +369,19 @@ 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).
+/// 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)]
 mod tests {
     use super::*;
@@ -392,6 +405,37 @@ 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(&PHOTOCRAFT, &with(page)), Action::OpenUrl(page.into()));
+        let releases = Action::OpenUrl(PHOTOCRAFT.releases_url());
+        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",
+            "https://github.com/storytold/photocraft/releases/tag/v9&calc",
+        ] {
+            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);
+    }
+
     pub(super) fn newer() -> Release {
         Release { tag: "v9.0.0".into(), version: crate::version::parse("9.0.0"), ..Release::default() }
     }