From 1ada60485e2782f743bd34772dbc263da3f5b2f3 Mon Sep 17 00:00:00 2001 From: Sam Powers <35611153+sam-powers@users.noreply.github.com> Date: Sat, 13 Jun 2026 17:46:51 -0400 Subject: [PATCH] security: confine file commands and validate deep-link targets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit read_file/write_file/delete_file were unconstrained filesystem primitives reachable from the frontend and — via the quill:// deep link — indirectly from any web page. A crafted path could read /etc/passwd or overwrite an arbitrary file. - ensure_allowed_path confines the three commands to Markdown documents and their .comments.json sidecars, the only paths any legitimate caller uses. The native open/save dialogs already restrict the user to .md, so no real capability is lost. - parse_quill_open now resolves the decoded path and accepts it only if it canonicalizes to an existing regular .md/.markdown file (symlink re-checked), so a hostile quill://open?file=/etc/passwd can no longer coax Quill into touching arbitrary files. Adds unit coverage for the path policy, the confined commands, and deep-link target validation (existing file, missing file, non-Markdown target, directory-with-.md-suffix, wrong host). Co-Authored-By: Claude Opus 4.8 --- src-tauri/src/lib.rs | 125 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 124 insertions(+), 1 deletion(-) diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index 3bcd9d9..74f2782 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -10,13 +10,34 @@ use tauri::ipc::Channel; use tauri::{Manager, State}; use tauri_plugin_dialog::DialogExt; +/// Document-like paths Quill is allowed to touch through the general file +/// commands. These commands are reachable from the frontend (and, via the +/// `quill://` deep link, indirectly from a hostile web page), so they must not +/// be general-purpose filesystem primitives. Every legitimate caller operates +/// on a Markdown document or its `.comments.json` sidecar; confining the +/// commands to those suffixes means a crafted path can never coax Quill into +/// reading `/etc/passwd` or overwriting an arbitrary file. The native open/save +/// dialogs already restrict the user to `.md`, so this loses no real capability. +fn ensure_allowed_path(path: &str) -> Result<(), String> { + let lower = path.to_ascii_lowercase(); + let allowed = + lower.ends_with(".md") || lower.ends_with(".markdown") || lower.ends_with(".comments.json"); + if allowed { + Ok(()) + } else { + Err("Refusing to access a file Quill does not manage".to_string()) + } +} + #[tauri::command] fn read_file(path: String) -> Result { + ensure_allowed_path(&path)?; std::fs::read_to_string(&path).map_err(|e| e.to_string()) } #[tauri::command] fn write_file(path: String, content: String) -> Result<(), String> { + ensure_allowed_path(&path)?; if let Some(parent) = PathBuf::from(&path).parent() { std::fs::create_dir_all(parent).map_err(|e| e.to_string())?; } @@ -25,6 +46,7 @@ fn write_file(path: String, content: String) -> Result<(), String> { #[tauri::command] fn delete_file(path: String) -> Result<(), String> { + ensure_allowed_path(&path)?; if std::path::Path::new(&path).exists() { std::fs::remove_file(&path).map_err(|e| e.to_string())?; } @@ -358,6 +380,77 @@ mod tests { assert!(path2.exists()); } + // --- path policy (ensure_allowed_path) --- + + #[test] + fn allowed_paths_accept_documents_and_sidecars() { + assert!(ensure_allowed_path("/tmp/notes.md").is_ok()); + assert!(ensure_allowed_path("/tmp/notes.markdown").is_ok()); + assert!(ensure_allowed_path("/tmp/notes.comments.json").is_ok()); + // Case-insensitive: macOS paths are commonly mixed-case. + assert!(ensure_allowed_path("/tmp/NOTES.MD").is_ok()); + } + + #[test] + fn disallowed_paths_are_rejected() { + assert!(ensure_allowed_path("/etc/passwd").is_err()); + assert!(ensure_allowed_path("/Users/me/.ssh/id_rsa").is_err()); + // A bare `.json` is not a Quill sidecar. + assert!(ensure_allowed_path("/tmp/secrets.json").is_err()); + // Suffix games: the real extension is what matters. + assert!(ensure_allowed_path("/tmp/notes.md.exe").is_err()); + } + + #[test] + fn confined_commands_refuse_disallowed_paths() { + // The commands themselves enforce the policy, not just the helper. + assert!(read_file("/etc/passwd".to_string()).is_err()); + assert!(write_file("/tmp/evil.sh".to_string(), "x".to_string()).is_err()); + assert!(delete_file("/tmp/evil.sh".to_string()).is_err()); + } + + // --- deep-link target validation (parse_quill_open / validate_open_target) --- + + #[test] + fn deep_link_opens_existing_markdown_file() { + let dir = tempdir().unwrap(); + let path = dir.path().join("doc.md"); + fs::write(&path, "# Doc").unwrap(); + let url = format!("quill://open?file={}", path.to_str().unwrap()); + let result = parse_quill_open(&url); + // Canonicalized, so compare against the canonical form. + let canonical = fs::canonicalize(&path).unwrap(); + assert_eq!(result, Some(canonical.to_string_lossy().into_owned())); + } + + #[test] + fn deep_link_rejects_nonexistent_file() { + let url = "quill://open?file=/tmp/quill_does_not_exist_xyz.md"; + assert_eq!(parse_quill_open(url), None); + } + + #[test] + fn deep_link_rejects_non_markdown_target() { + // The classic attack: point the scheme at a sensitive file. + let url = "quill://open?file=/etc/passwd"; + assert_eq!(parse_quill_open(url), None); + } + + #[test] + fn deep_link_rejects_directory_even_with_md_suffix() { + let dir = tempdir().unwrap(); + let bogus = dir.path().join("notes.md"); + fs::create_dir(&bogus).unwrap(); + let url = format!("quill://open?file={}", bogus.to_str().unwrap()); + assert_eq!(parse_quill_open(&url), None); + } + + #[test] + fn deep_link_rejects_wrong_host() { + let url = "quill://evil?file=/tmp/whatever.md"; + assert_eq!(parse_quill_open(url), None); + } + // --- classify_claude_outcome --- #[test] @@ -1590,6 +1683,14 @@ fn update_recent_menu(app: tauri::AppHandle, paths: Vec) -> Result<(), S fn parse_quill_open(url: &str) -> Option { // Expected form: quill://open?file= + // + // This is an OS-level entry point: any web page can fire `quill://open?...`, + // so the target is attacker-influenced. We never hand back a raw path. The + // decoded path must point at an existing **regular** Markdown file; anything + // else (a directory, a device, a non-document, a non-existent path, or a + // symlink to one) is rejected so the deep link can only ever open a real + // document the user already has on disk — not coax Quill into touching + // arbitrary files. let rest = url.strip_prefix("quill://")?; let (host, query) = rest.split_once('?')?; if host != "open" { @@ -1597,12 +1698,34 @@ fn parse_quill_open(url: &str) -> Option { } for pair in query.split('&') { if let Some(v) = pair.strip_prefix("file=") { - return Some(percent_decode(v)); + let decoded = percent_decode(v); + return validate_open_target(&decoded); } } None } +/// Accept a deep-link target only if it resolves to an existing regular +/// Markdown file. Returns the canonicalized path (symlinks resolved) so callers +/// open the real file, not a redirect. +fn validate_open_target(path: &str) -> Option { + let lower = path.to_ascii_lowercase(); + if !(lower.ends_with(".md") || lower.ends_with(".markdown")) { + return None; + } + let canonical = std::fs::canonicalize(path).ok()?; + if !canonical.is_file() { + return None; + } + // Re-check the suffix on the canonical path: a symlink could end in `.md` + // while pointing at something else. + let canon_lower = canonical.to_string_lossy().to_ascii_lowercase(); + if !(canon_lower.ends_with(".md") || canon_lower.ends_with(".markdown")) { + return None; + } + Some(canonical.to_string_lossy().into_owned()) +} + fn percent_decode(s: &str) -> String { let bytes = s.as_bytes(); let mut out: Vec = Vec::with_capacity(bytes.len());