mirror of
https://github.com/storytold/craft-libs.git
synced 2026-11-09 03:51:47 +00:00
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 <noreply@anthropic.com>
This commit is contained in:
1 parent
04aa1703b3
commit
3f21cfa0ae
3 files changed
+62
-3
No files matched your search
@@ -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();
|
||||
|
||||
@@ -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<String>) -> Self {
|
||||
self.token = token.map(|t| t.trim().to_string()).filter(|t| !t.is_empty());
|
||||
|
||||
@@ -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() }
|
||||
}
|
||||
|
||||
Reference in new issue
Block a user