From 37419afe146f19134170887dd2b23b97e64f00ac Mon Sep 17 00:00:00 2001 From: skylar taylor-barrick Date: Sat, 17 Oct 2026 13:19:13 -0600 Subject: [PATCH] Open only web links and local libraries from a document A document carries things the app acts on for whoever opens it. Three of them were taken as given. Prototype Open link and Dev Mode dev resources both reached the platform's opener with any scheme. In the web build the opener is window.open, so a saved document could run script in the app's own origin; on the desktop any scheme went to the system's handler. Both now pass through the new engine module `doc_urls`, which allows http, https and mailto and nothing else. It is an allow-list on purpose: a list of known-bad schemes is outgrown by every handler a platform adds. A figcraft:// invite link joined the session at once. The system hands the app whatever such link was followed, which may come from a page or a message, so the Join dialog now opens with the link filled in and the person confirms. A library link stored in a document was loaded from wherever it pointed. Only a path on this computer is followed now, so opening someone else's file cannot make the app contact their server (which on Windows would also send credentials). Tests: each change has one that fails without it, verified by reverting. The prototype test fires through an afterDelay trigger, since no click is needed, and keeps a positive case so a refusal cannot pass vacuously. --- Cargo.lock | 6 ++ Cargo.toml | 3 + crates/engine/Cargo.toml | 1 + crates/engine/src/doc_urls.rs | 48 +++++++++++++ crates/engine/src/doc_urls_tests.rs | 87 +++++++++++++++++++++++ crates/engine/src/lib.rs | 5 ++ crates/engine/src/library_path_tests.rs | 30 ++++++++ crates/engine/src/library_server.rs | 7 ++ crates/ui-egui/src/dev_mode.rs | 5 +- crates/ui-egui/src/lib.rs | 2 + crates/ui-egui/src/live.rs | 12 ++-- crates/ui-egui/src/live_join_tests.rs | 11 +-- crates/ui-egui/src/present.rs | 4 +- crates/ui-egui/src/present_links_tests.rs | 71 ++++++++++++++++++ 14 files changed, 280 insertions(+), 12 deletions(-) create mode 100644 crates/engine/src/doc_urls.rs create mode 100644 crates/engine/src/doc_urls_tests.rs create mode 100644 crates/engine/src/library_path_tests.rs create mode 100644 crates/ui-egui/src/present_links_tests.rs diff --git a/Cargo.lock b/Cargo.lock index 9baacd58..a61d87cc 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -856,6 +856,11 @@ dependencies = [ "libc", ] +[[package]] +name = "craft-docpath" +version = "0.1.0" +source = "git+https://github.com/storytold/craft-libs?rev=82a24b006eefa036b43c09a67fb9a3fbd17ffe63#82a24b006eefa036b43c09a67fb9a3fbd17ffe63" + [[package]] name = "crc32fast" version = "1.5.2" @@ -1511,6 +1516,7 @@ name = "figcraft-engine" version = "0.6.0" dependencies = [ "base64 0.22.1", + "craft-docpath", "figcraft-collab", "figcraft-color", "figcraft-doc", diff --git a/Cargo.toml b/Cargo.toml index 8f125ac3..fe089328 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -32,6 +32,9 @@ figcraft-mcp = { path = "crates/mcp" } figcraft-collab-net = { path = "crates/collab-net" } figcraft-plugin = { path = "crates/plugin" } +# Which paths stored in a document may be followed (craft-libs). +craft-docpath = { version = "0.1.0", git = "https://github.com/storytold/craft-libs", rev = "82a24b006eefa036b43c09a67fb9a3fbd17ffe63" } + # foundation kurbo = { version = "0.13", features = ["serde"] } thiserror = "2" diff --git a/crates/engine/Cargo.toml b/crates/engine/Cargo.toml index f610c38b..545d872c 100644 --- a/crates/engine/Cargo.toml +++ b/crates/engine/Cargo.toml @@ -20,6 +20,7 @@ figcraft-render = { workspace = true } figcraft-format = { workspace = true } figcraft-svg = { workspace = true } figcraft-pdf = { workspace = true } +craft-docpath = { workspace = true } figcraft-plugin = { workspace = true } serde = { workspace = true } serde_json = { workspace = true } diff --git a/crates/engine/src/doc_urls.rs b/crates/engine/src/doc_urls.rs new file mode 100644 index 00000000..3b8417cd --- /dev/null +++ b/crates/engine/src/doc_urls.rs @@ -0,0 +1,48 @@ +//! Which URLs stored in a document may be handed to the browser or the system opener. +//! +//! A document can carry URLs that the app opens for whoever views it: a prototype's Open link +//! action ([`crate::proto_run`]) and Dev Mode's dev resources ([`crate::dev_resources`]). Both end +//! up at the platform's opener, so a document that someone else made chooses what that opener is +//! asked to do. Only the schemes a browser would follow on a plain link are allowed: +//! +//! - `javascript:` runs script. In the web build the opener is `window.open`, which executes it in +//! the app's own origin, so a saved document becomes stored XSS. +//! - `file:` and `smb:` reach the file system; on Windows a UNC host also sends credentials. +//! - Windows hands `ms-…:` schemes to application handlers, and macOS and Linux hand unknown +//! schemes to whichever application registered them. +//! +//! The check is lexical and does no I/O. It is deliberately an allow-list: a deny-list of known +//! dangerous schemes is outgrown by every new handler a platform adds. + +/// The schemes a URL stored in a document may use. +const WEB_SCHEMES: [&str; 3] = ["http", "https", "mailto"]; + +/// Whether `url`, stored in a document, may be opened. +/// +/// Requires a scheme from [`WEB_SCHEMES`], compared ignoring ASCII case. A URL with no scheme is +/// refused: the opener would resolve it against the app's own page on the web. Control characters +/// are refused anywhere, because browsers ignore some of them inside a scheme (`java\tscript:` +/// reaches a `javascript:` handler), and `http`/`https` must name a host. +pub fn openable(url: &str) -> bool { + let url = url.trim(); + if url.is_empty() || url.chars().any(char::is_control) { + return false; + } + let Some((scheme, rest)) = url.split_once(':') else { return false }; + // An RFC 3986 scheme, so a stray colon in text cannot look like one. + let mut chars = scheme.chars(); + if !chars.next().is_some_and(|c| c.is_ascii_alphabetic()) || !chars.all(|c| c.is_ascii_alphanumeric() || matches!(c, '+' | '-' | '.')) { + return false; + } + if !WEB_SCHEMES.iter().any(|s| scheme.eq_ignore_ascii_case(s)) { + return false; + } + if scheme.eq_ignore_ascii_case("mailto") { + // `mailto:` names an address. + return !rest.is_empty(); + } + // `http` and `https` name a host, so the authority is required: `http:x` is not a web link, + // and `http:///p` has no host. + let Some(after) = rest.strip_prefix("//") else { return false }; + !after.is_empty() && !after.starts_with('/') +} diff --git a/crates/engine/src/doc_urls_tests.rs b/crates/engine/src/doc_urls_tests.rs new file mode 100644 index 00000000..d42029a0 --- /dev/null +++ b/crates/engine/src/doc_urls_tests.rs @@ -0,0 +1,87 @@ +//! A URL stored in a document reaches the browser or the system opener, so only the schemes a +//! browser follows on a plain link are allowed ([`crate::doc_urls`]). + +use crate::doc_urls::openable; + +#[test] +fn web_links_are_opened() { + for url in [ + "http://example.com", + "https://example.com", + "https://example.com/a/b?c=d#e", + "HTTPS://EXAMPLE.COM", + "HtTp://example.com", + "mailto:a@b.c", + "mailto:a@b.c?subject=hi", + " https://example.com ", + "https://user:pw@example.com:8443/p", + ] { + assert!(openable(url), "{url:?} should open"); + } +} + +#[test] +fn script_and_handler_schemes_are_refused() { + for url in [ + // Runs in the app's own origin on the web build: a saved document would be stored XSS. + "javascript:alert(1)", + "JavaScript:alert(1)", + "jAvAsCrIpT:alert(document.cookie)", + "vbscript:msgbox(1)", + "data:text/html,", + "blob:https://example.com/x", + // Reach the file system; a UNC host also sends Windows credentials. + "file:///etc/passwd", + "file://attacker.example/share/x", + "smb://attacker.example/share/x", + "webdav://attacker.example/x", + // Windows application handlers. + "ms-msdt:/id PCWDiagnostic", + "ms-officecmd:x", + "search-ms:query=x", + "shell:startup", + // Anything else a platform may have registered. + "tel:+1234", + "sms:+1234", + "ssh://host", + "ftp://host/x", + "figcraft://join?host=127.0.0.1", + "zoommtg://zoom.us/join?x", + "view-source:https://example.com", + "jar:https://example.com/x!/y", + ] { + assert!(!openable(url), "{url:?} must be refused"); + } +} + +#[test] +fn a_scheme_is_required_and_control_characters_are_refused() { + for url in [ + // No scheme: the web opener would resolve these against the app's own page. + "", + " ", + "example.com", + "/etc/passwd", + "//example.com", + "\\\\host\\share\\x", + "about", + // Browsers ignore some control characters inside a scheme, so a smuggled one must not + // leave `javascript` looking like an unknown scheme that we then refuse by luck. + "java\tscript:alert(1)", + "java\nscript:alert(1)", + "java\rscript:alert(1)", + "javascript\u{0}:alert(1)", + "https://example.com/\u{0}", + // A colon in ordinary text is not a scheme. + "see page 2: the diagram", + "1https://example.com", + // The allowed schemes still need their own part. + "mailto:", + "http:", + "http:example.com", + "http:///p", + "https://", + ] { + assert!(!openable(url), "{url:?} must be refused"); + } +} diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 2b09c5c3..80de797e 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -26,6 +26,7 @@ pub mod dev_resources; pub mod devcompare; pub mod devinspect; pub mod distance; +pub mod doc_urls; pub mod drop; pub mod file_access; pub mod guard; @@ -1576,6 +1577,8 @@ mod devfocus_tests; #[cfg(test)] mod devinspect_tests; #[cfg(test)] +mod doc_urls_tests; +#[cfg(test)] mod double_click_tests; #[cfg(test)] mod draw_tools_tests; @@ -1636,6 +1639,8 @@ mod invite_links_tests; #[cfg(test)] mod keymap_tests; #[cfg(test)] +mod library_path_tests; +#[cfg(test)] mod library_range_styles_tests; #[cfg(test)] mod library_variables_tests; diff --git a/crates/engine/src/library_path_tests.rs b/crates/engine/src/library_path_tests.rs new file mode 100644 index 00000000..f15105dc --- /dev/null +++ b/crates/engine/src/library_path_tests.rs @@ -0,0 +1,30 @@ +//! A library link is stored in the document, so it is followed only on this computer +//! ([`crate::library_server`]): a path on another server would make opening someone else's file +//! contact that server, and on Windows send the credentials of whoever opened it. + +use crate::Session; + +#[test] +fn a_library_is_opened_only_from_this_computer() { + let mut s = Session::new(); + for path in [ + r"\\attacker.example\share\lib.figcraft", + "//attacker.example/share/lib.figcraft", + "file://attacker.example/share/lib.figcraft", + r"\\?\UNC\attacker.example\share\lib.figcraft", + r"\\.\pipe\lib", + "https://attacker.example/lib.figcraft", + ] { + let e = s.fetch_library(path, false).expect_err(&format!("{path} must be refused")); + assert!(e.to_string().contains("only from this computer"), "{path}: {e}"); + } +} + +#[test] +fn a_local_library_path_is_still_followed() { + let mut s = Session::new(); + // Refused for being absent, not for where it is: the gate let it through. + let missing = std::env::temp_dir().join("figcraft-no-such-library.figcraft"); + let e = s.fetch_library(&missing.to_string_lossy(), false).expect_err("the file does not exist"); + assert!(!e.to_string().contains("only from this computer"), "a local path must not be refused as remote: {e}"); +} diff --git a/crates/engine/src/library_server.rs b/crates/engine/src/library_server.rs index dbd97a0b..99d5f29a 100644 --- a/crates/engine/src/library_server.rs +++ b/crates/engine/src/library_server.rs @@ -129,6 +129,13 @@ impl Session { /// document uses the library. pub fn fetch_library(&mut self, path: &str, fresh: bool) -> Result> { if !is_server_link(path) { + // A library link is stored in the document, so opening someone else's file would + // otherwise reach whatever path it names. Only a path on this computer is followed: a + // network share would make opening a document contact that server, and on Windows + // send the credentials of whoever opened it. + if !craft_docpath::is_local(path) { + return Err(EngineError::Other(format!("{path}: a library is opened only from this computer"))); + } return self.libraries.load(path); } let have = match self.libraries.cached(path) { diff --git a/crates/ui-egui/src/dev_mode.rs b/crates/ui-egui/src/dev_mode.rs index 0fedccd7..f5c7e40f 100644 --- a/crates/ui-egui/src/dev_mode.rs +++ b/crates/ui-egui/src/dev_mode.rs @@ -635,8 +635,9 @@ pub fn dev_resources_section(app: &mut FigApp, ui: &mut Ui, doc: &Document, id: }; ui.painter().text(pos2(x, r.center().y + 7.0), Align2::LEFT_CENTER, widgets::elide(ui, &below, text_w), regular(10.0), t.text_secondary); crate::a11y::link(&resp, name); - // Opened only on a click, never fetched. - if resp.on_hover_text(url.as_str()).clicked() { + // Opened only on a click, never fetched, and a dev resource's URL comes from the + // document, so only a web link opens. + if resp.on_hover_text(url.as_str()).clicked() && figcraft_engine::doc_urls::openable(&url) { ui.ctx().open_url(egui::OpenUrl::new_tab(url)); } if let Some(i) = item.index { diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index e6a14ede..4dba320c 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -171,6 +171,8 @@ mod plugins_browse_tests; #[cfg(test)] mod popup_width_tests; #[cfg(test)] +mod present_links_tests; +#[cfg(test)] mod present_tests; #[cfg(test)] mod present_view_tests; diff --git a/crates/ui-egui/src/live.rs b/crates/ui-egui/src/live.rs index 35af5d14..513060ab 100644 --- a/crates/ui-egui/src/live.rs +++ b/crates/ui-egui/src/live.rs @@ -1047,12 +1047,14 @@ pub fn open_join(app: &mut FigApp, link: &str) { app.ui.live.focus_join = true; } -/// An invite link the app was opened with (`figcraft figcraft://join?…`): join at once; a link -/// that can't be used shows in the Join dialog with the reason. +/// An invite link the app was opened with (`figcraft figcraft://join?…`): the Join dialog opens +/// with the link filled in, so joining is the person's own choice. +/// +/// The system hands the app any `figcraft://` link that is followed, which may be a page, a +/// message or a document rather than something the person meant to act on, so the link is shown +/// and confirmed rather than acted on. pub fn open_invite(app: &mut FigApp, link: &str) { - if app.run("collab.join", json!({"link": link.trim()})).is_none() { - open_join(app, link.trim()); - } + open_join(app, link.trim()); } /// Escape closes the Join or Publish dialog (before the canvas's shortcuts see it: it doesn't diff --git a/crates/ui-egui/src/live_join_tests.rs b/crates/ui-egui/src/live_join_tests.rs index d7452163..61567ce0 100644 --- a/crates/ui-egui/src/live_join_tests.rs +++ b/crates/ui-egui/src/live_join_tests.rs @@ -47,12 +47,14 @@ fn escape_closes_the_join_dialog_and_keeps_the_selection() { } #[test] -fn opening_the_app_with_an_invite_link_joins() { +fn opening_the_app_with_an_invite_link_asks_before_joining() { let (mut h, _) = Harness::new(); let link = "figcraft://join?host=127.0.0.1:47900&file=abc&token=AbCdEfGhIjKlMnOpQrStUv"; crate::live::open_invite(&mut h.app, &format!(" {link} ")); - assert_eq!(h.app.session.live().status, Status::Connecting); - assert!(h.app.ui.live.join.is_none(), "no dialog: it joins at once"); + // The system hands the app whatever `figcraft://` link was followed, which may come from a + // page or a message, so the link is shown and confirmed rather than acted on. + assert_eq!(h.app.ui.live.join.as_deref(), Some(link), "the dialog opens with the link filled in"); + assert_eq!(h.app.session.live().status, Status::Off, "nothing is joined until the person confirms"); } /// The browser's link: joins, can't host; joining fails here to show the dialog path. @@ -82,7 +84,8 @@ fn a_link_that_cant_be_used_opens_the_dialog_with_it() { h.app.session.live.install(Box::new(Browser)); crate::live::open_invite(&mut h.app, "figcraft://nope"); assert_eq!(h.app.ui.live.join.as_deref(), Some("figcraft://nope"), "the link stays in the field to fix"); - assert!(h.app.ui.last_error.as_deref().is_some_and(|e| e.contains("not a live session invite"))); + // The link is not tried until the person presses Join, so its reason shows then, not now. + assert!(h.app.ui.last_error.is_none()); assert_eq!(h.app.session.live().status, Status::Off); } diff --git a/crates/ui-egui/src/present.rs b/crates/ui-egui/src/present.rs index 863b540f..cc78c86b 100644 --- a/crates/ui-egui/src/present.rs +++ b/crates/ui-egui/src/present.rs @@ -865,7 +865,9 @@ impl Presenter { } Action::CloseOverlay => self.close_overlay(now), Action::OpenUrl { url } => { - if !url.trim().is_empty() { + // The URL comes from the document and this action fires from whatever trigger the + // prototype sets, including a delay, so no click is implied: only a web link opens. + if figcraft_engine::doc_urls::openable(url) { self.urls.push(url.clone()); } true diff --git a/crates/ui-egui/src/present_links_tests.rs b/crates/ui-egui/src/present_links_tests.rs new file mode 100644 index 00000000..8223890c --- /dev/null +++ b/crates/ui-egui/src/present_links_tests.rs @@ -0,0 +1,71 @@ +//! A prototype's Open link action carries a URL from the document and fires from whatever trigger +//! the prototype sets, including a delay, so the player hands the opener only a web link +//! ([`figcraft_engine::doc_urls`]). Without that, a saved document opens `javascript:` in the web +//! build's own origin (stored XSS) and `smb:`/`ms-…:` through the desktop's handlers. + +use std::sync::{Arc, Mutex}; + +use figcraft_engine::Session; +use serde_json::{Value, json}; + +use crate::Services; +use crate::present_tests::{Ui as Harness, id}; + +/// Records every URL the player asks the platform to open. +#[derive(Default)] +struct Opened(Arc>>); + +impl Services for Opened { + fn open_url(&mut self, url: &str) { + if let Ok(mut v) = self.0.lock() { + v.push(url.to_string()); + } + } +} + +fn ex(s: &mut Session, cmd: &str, p: Value) -> Value { + s.execute(cmd, &p).unwrap_or_else(|e| panic!("{cmd} {p}: {e}")) +} + +/// A presented frame whose layer opens `url` 100ms after the screen appears — no click anywhere. +fn opens_after_a_delay(url: &str) -> Vec { + let mut s = Session::new(); + let f = id(&ex(&mut s, "frame.create", json!({"x": 0, "y": 0, "width": 300, "height": 400, "name": "A"}))); + let layer = id(&ex(&mut s, "shape.rectangle", json!({"x": 10, "y": 10, "width": 30, "height": 30, "parent": f}))); + ex( + &mut s, + "proto.interaction.add", + json!({"id": layer, "trigger": {"type": "afterDelay", "ms": 100}, "action": {"type": "openUrl", "url": url}, "transition": {"type": "instant"}}), + ); + let seen = Arc::new(Mutex::new(Vec::new())); + let mut u = Harness::new(s); + u.app.services = Box::new(Opened(seen.clone())); + u.run("proto.present", json!({"start": f})); + // Well past the delay. + u.frames(40, Vec::new); + seen.lock().map(|v| v.clone()).unwrap_or_default() +} + +#[test] +fn a_prototype_opens_web_links() { + assert_eq!(opens_after_a_delay("https://example.com/a"), ["https://example.com/a"]); + assert_eq!(opens_after_a_delay("mailto:a@b.c"), ["mailto:a@b.c"]); +} + +#[test] +fn a_prototype_cannot_open_a_script_or_handler_url() { + for url in [ + // In the web build the opener is `window.open`, which runs this in the app's own origin. + "javascript:alert(document.cookie)", + "JavaScript:alert(1)", + "data:text/html,", + // Reaches the file system; a UNC host also sends Windows credentials. + "file:///etc/passwd", + "smb://attacker.example/share/x", + // Handed to an application handler by the desktop. + "ms-msdt:/id PCWDiagnostic", + "figcraft://join?host=127.0.0.1:47900&file=f&token=t", + ] { + assert!(opens_after_a_delay(url).is_empty(), "{url:?} must never reach the opener"); + } +}