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"); + } +}