From ed6596436ee7346f4296c6cb021b95c822977b87 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Tue, 22 Sep 2026 22:15:25 -0700 Subject: [PATCH] refactor(acl): shared command census module for build.rs and tests Moves the generate_handler! parser out of the lib.rs test into src/command_census.rs and adds the capability rules (one allow-* grant per command, in the file its name says, no deny-*) with unit tests. No behaviour change yet: build.rs does not use it until the next commit. Co-Authored-By: Claude Opus 5.5 (1M context) --- app/src-tauri/src/command_census.rs | 387 ++++++++++++++++++++++++++++ app/src-tauri/src/lib.rs | 49 +--- 2 files changed, 398 insertions(+), 38 deletions(-) create mode 100644 app/src-tauri/src/command_census.rs diff --git a/app/src-tauri/src/command_census.rs b/app/src-tauri/src/command_census.rs new file mode 100644 index 0000000..955d59d --- /dev/null +++ b/app/src-tauri/src/command_census.rs @@ -0,0 +1,387 @@ +//! The command census shared by `build.rs` and the `cargo test` suite. +//! +//! `build.rs` pulls this file in with `#[path = "src/command_census.rs"]` and `lib.rs` with +//! `#[cfg(test)] mod command_census;`, so the parser that decides what the Tauri `AppManifest` +//! declares is the parser the tests exercise, and the rules that decide whether the build +//! passes have unit tests. Nothing here may reference the crate: only `std` and `serde_json` +//! (a dependency of both the crate and the build script). +//! +//! Spec: `docs/superpowers/specs/2026-09-22-app-manifest-lockdown-design.md` §3.2. + +use std::collections::{BTreeMap, BTreeSet}; + +/// The command names inside `generate_handler![ … ])` in `lib.rs`, in registration order, +/// duplicates kept (the caller decides whether that is an error). `None` if the block is +/// missing or unterminated. +/// +/// Line-based, not `split(',')`: the list is grouped under `// Docker` style comments, and +/// splitting on commas glues each comment to the command that follows it. A +/// `starts_with("//")` filter then drops that command — silently, and once per group. +pub fn registered_commands(lib_rs: &str) -> Option> { + let (_, rest) = lib_rs.split_once("generate_handler![")?; + let (inside, _) = rest.split_once("])")?; + Some( + inside + .lines() + .map(str::trim) + .filter(|l| !l.is_empty() && !l.starts_with("//")) + .filter_map(|l| { + // `a::b::name, // comment` → `name` + let code = l.split("//").next().unwrap_or("").trim(); + code.trim_end_matches(',') + .rsplit("::") + .next() + .map(|n| n.trim().to_string()) + }) + .filter(|n| !n.is_empty()) + .collect(), + ) +} + +/// `viewer_read_file` → `allow-viewer-read-file`. tauri-utils 2.9.0 (`acl/build.rs:290`) +/// replaces only `_`; permission identifiers may not contain `_`, but the command name inside +/// the generated permission stays snake_case. +pub fn allow_permission(command: &str) -> String { + format!("allow-{}", command.replace('_', "-")) +} + +/// The `windows` list of the one capability file that may grant `command`. A command that +/// must be callable from both windows is a design change: make it here, visibly, rather than +/// by widening a capability file. +pub fn expected_windows(command: &str) -> &'static [&'static str] { + if command.starts_with("viewer_") { + &["file-viewer-*"] + } else { + &["main"] + } +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct CapabilityFile { + pub name: String, + pub windows: Vec, + pub bare: Vec, +} + +/// One `capabilities/*.json`, reduced to what the census checks. Plugin and core grants +/// (anything with a `:`) are not this module's business; the exact-set tests in `lib.rs` and +/// `file_viewer/mod.rs` pin those. +pub fn capability_file(name: &str, json: &str) -> Result { + let value: serde_json::Value = + serde_json::from_str(json).map_err(|e| format!("{name}: not valid JSON: {e}"))?; + let windows = value["windows"] + .as_array() + .ok_or_else(|| format!("{name}: `windows` must be an array"))? + .iter() + .map(|w| { + w.as_str() + .map(str::to_string) + .ok_or_else(|| format!("{name}: `windows` entries must be strings")) + }) + .collect::, _>>()?; + let mut bare = Vec::new(); + for grant in value["permissions"] + .as_array() + .ok_or_else(|| format!("{name}: `permissions` must be an array"))? + { + let id = match grant { + serde_json::Value::String(s) => s.as_str(), + serde_json::Value::Object(o) => o + .get("identifier") + .and_then(|i| i.as_str()) + .ok_or_else(|| format!("{name}: a scoped grant needs a string `identifier`"))?, + _ => return Err(format!("{name}: a grant is a string or an object")), + }; + if !id.contains(':') { + bare.push(id.to_string()); + } + } + Ok(CapabilityFile { name: name.to_string(), windows, bare }) +} + +/// Everything that must hold between the handler list and the capability files. Returns every +/// violation rather than the first, so a batch of forgotten grants is one build failure; an +/// empty vector is a pass. +pub fn check(commands: &[String], files: &[CapabilityFile]) -> Vec { + let mut problems = Vec::new(); + if commands.is_empty() { + problems.push( + "no commands were parsed out of generate_handler! — an empty AppManifest would \ + silently leave every app command ungated" + .to_string(), + ); + return problems; + } + + let mut seen: BTreeSet<&str> = BTreeSet::new(); + for c in commands { + if !c.bytes().all(|b| b.is_ascii_lowercase() || b.is_ascii_digit() || b == b'_') { + problems.push(format!("{c:?} is not a command name ([a-z0-9_]+)")); + } + if !seen.insert(c.as_str()) { + problems.push(format!("{c} is registered more than once")); + } + } + + let known: BTreeMap = + seen.iter().map(|c| (allow_permission(c), *c)).collect(); + for f in files { + let windows: Vec<&str> = f.windows.iter().map(String::as_str).collect(); + for id in &f.bare { + match known.get(id) { + Some(command) => { + let want = expected_windows(command); + if windows.as_slice() != want { + problems.push(format!( + "{}: {id} must be granted in the capability file whose windows are \ + {want:?}, not {windows:?}", + f.name + )); + } + } + None if id.starts_with("deny-") => problems.push(format!( + "{}: {id}: deny-* is global in tauri 2.11 — it would deny the command for \ + every window, not just this one; use allow-lists only", + f.name + )), + None if id.starts_with("allow-") => problems.push(format!( + "{}: {id} names no registered command (the identifier is allow- \ + with every `_` replaced by `-`)", + f.name + )), + None => problems.push(format!( + "{}: {id}: only allow- app grants are permitted as bare identifiers", + f.name + )), + } + } + } + + for c in &seen { + let id = allow_permission(c); + let holders: Vec<&str> = files + .iter() + .filter(|f| f.bare.iter().any(|b| b == &id)) + .map(|f| f.name.as_str()) + .collect(); + match holders.len() { + 0 => problems.push(format!( + "{c} is registered but no capability file grants {id}; add it to the file \ + whose windows are {:?}", + expected_windows(c) + )), + 1 => {} + _ => problems.push(format!( + "{id} is granted in more than one capability file: {holders:?}" + )), + } + } + problems +} + +#[cfg(test)] +mod tests { + use super::*; + + fn cmds(names: &[&str]) -> Vec { + names.iter().map(|n| n.to_string()).collect() + } + + fn file(name: &str, windows: &[&str], bare: &[&str]) -> CapabilityFile { + CapabilityFile { + name: name.to_string(), + windows: windows.iter().map(|w| w.to_string()).collect(), + bare: bare.iter().map(|b| b.to_string()).collect(), + } + } + + /// The two files as they must look after the lockdown, for a three-command app. + fn good_files() -> Vec { + vec![ + file("default.json", &["main"], &["allow-check-docker", "allow-open-file-viewer"]), + file("file-viewer.json", &["file-viewer-*"], &["allow-viewer-read-file"]), + ] + } + + const THREE: &[&str] = &["check_docker", "open_file_viewer", "viewer_read_file"]; + + #[test] + fn the_parser_reads_the_handler_list_in_order_and_ignores_comments() { + let lib_rs = r#" + .invoke_handler(tauri::generate_handler![ + // Docker + commands::docker_commands::check_docker, + commands::docker_commands::build_image, // trailing comment is not a command + url_open::open_url_external, + + // Viewer + commands::file_viewer_commands::viewer_read_file + ]) + .run(tauri::generate_context!()) + "#; + assert_eq!( + registered_commands(lib_rs).unwrap(), + cmds(&["check_docker", "build_image", "open_url_external", "viewer_read_file"]) + ); + } + + #[test] + fn the_parser_keeps_duplicates_so_the_caller_can_report_them() { + let lib_rs = "generate_handler![\n a::x,\n b::x,\n])"; + assert_eq!(registered_commands(lib_rs).unwrap(), cmds(&["x", "x"])); + } + + #[test] + fn the_parser_returns_none_without_a_handler_block() { + assert_eq!(registered_commands("fn main() {}"), None); + assert_eq!(registered_commands("generate_handler![ a::b, "), None, "unterminated"); + } + + #[test] + fn permission_identifiers_replace_only_underscores() { + assert_eq!(allow_permission("check_docker"), "allow-check-docker"); + assert_eq!(allow_permission("viewer_read_file"), "allow-viewer-read-file"); + assert_eq!(allow_permission("aws_sso_refresh"), "allow-aws-sso-refresh"); + } + + #[test] + fn viewer_commands_belong_to_the_viewer_windows_and_nothing_else_does() { + assert_eq!(expected_windows("viewer_read_file"), ["file-viewer-*"]); + assert_eq!(expected_windows("open_file_viewer"), ["main"]); + assert_eq!(expected_windows("check_docker"), ["main"]); + } + + #[test] + fn a_capability_file_yields_its_windows_and_bare_grants_only() { + let json = r#"{ + "identifier": "default", + "description": "x", + "windows": ["main"], + "permissions": [ + "core:event:allow-listen", + { "identifier": "fs:allow-read", "allow": [{ "path": "$APPDATA/*" }] }, + "allow-check-docker", + { "identifier": "allow-list-projects" } + ] + }"#; + let parsed = capability_file("default.json", json).unwrap(); + assert_eq!(parsed.name, "default.json"); + assert_eq!(parsed.windows, vec!["main"]); + assert_eq!(parsed.bare, vec!["allow-check-docker", "allow-list-projects"]); + } + + #[test] + fn a_capability_file_without_windows_or_permissions_is_an_error() { + assert!(capability_file("x.json", r#"{"permissions": []}"#).unwrap_err().contains("windows")); + assert!(capability_file("x.json", r#"{"windows": ["main"]}"#).unwrap_err().contains("permissions")); + assert!(capability_file("x.json", "not json").unwrap_err().contains("x.json")); + } + + #[test] + fn a_correct_census_has_no_problems() { + assert_eq!(check(&cmds(THREE), &good_files()), Vec::::new()); + } + + #[test] + fn an_empty_command_list_is_refused_because_it_would_disable_the_acl() { + let problems = check(&[], &good_files()); + assert_eq!(problems.len(), 1); + assert!(problems[0].contains("no commands"), "{problems:?}"); + } + + #[test] + fn a_command_without_a_grant_is_named_together_with_the_file_it_belongs_in() { + let files = vec![ + file("default.json", &["main"], &["allow-check-docker"]), + file("file-viewer.json", &["file-viewer-*"], &["allow-viewer-read-file"]), + ]; + let problems = check(&cmds(THREE), &files); + assert_eq!(problems.len(), 1, "{problems:?}"); + assert!(problems[0].contains("open_file_viewer")); + assert!(problems[0].contains("allow-open-file-viewer")); + assert!(problems[0].contains("[\"main\"]")); + } + + #[test] + fn a_grant_in_two_files_is_reported_once_naming_both() { + let files = vec![ + file("default.json", &["main"], &["allow-check-docker", "allow-open-file-viewer"]), + file("extra.json", &["main"], &["allow-check-docker"]), + file("file-viewer.json", &["file-viewer-*"], &["allow-viewer-read-file"]), + ]; + let problems = check(&cmds(THREE), &files); + assert_eq!(problems.len(), 1, "{problems:?}"); + assert!(problems[0].contains("allow-check-docker")); + assert!(problems[0].contains("default.json") && problems[0].contains("extra.json")); + } + + #[test] + fn a_grant_that_names_no_command_is_a_typo() { + let mut files = good_files(); + files[0].bare.push("allow-check-dokcer".to_string()); + let problems = check(&cmds(THREE), &files); + assert_eq!(problems.len(), 1, "{problems:?}"); + assert!(problems[0].contains("default.json: allow-check-dokcer")); + assert!(problems[0].contains("no registered command")); + } + + #[test] + fn deny_grants_are_refused_with_the_reason() { + let mut files = good_files(); + files[1].bare.push("deny-check-docker".to_string()); + let problems = check(&cmds(THREE), &files); + assert_eq!(problems.len(), 1, "{problems:?}"); + assert!(problems[0].contains("file-viewer.json: deny-check-docker")); + assert!(problems[0].contains("global")); + } + + #[test] + fn other_bare_identifiers_are_refused() { + let mut files = good_files(); + files[0].bare.push("default".to_string()); + let problems = check(&cmds(THREE), &files); + assert_eq!(problems.len(), 1, "{problems:?}"); + assert!(problems[0].contains("default.json: default")); + } + + #[test] + fn a_grant_in_the_wrong_file_is_refused_even_though_it_is_granted_exactly_once() { + let files = vec![ + file("default.json", &["main"], &["allow-check-docker", "allow-open-file-viewer", "allow-viewer-read-file"]), + file("file-viewer.json", &["file-viewer-*"], &[]), + ]; + let problems = check(&cmds(THREE), &files); + assert_eq!(problems.len(), 1, "{problems:?}"); + assert!(problems[0].contains("allow-viewer-read-file")); + assert!(problems[0].contains("[\"file-viewer-*\"]")); + } + + #[test] + fn a_widened_windows_list_is_the_wrong_file_too() { + let files = vec![ + file("default.json", &["main", "file-viewer-*"], &["allow-check-docker", "allow-open-file-viewer"]), + file("file-viewer.json", &["file-viewer-*"], &["allow-viewer-read-file"]), + ]; + let problems = check(&cmds(THREE), &files); + assert_eq!(problems.len(), 2, "{problems:?}"); + } + + #[test] + fn bad_names_and_duplicate_registrations_are_refused() { + let commands = cmds(&["check_docker", "Check-Docker", "check_docker", "open_file_viewer", "viewer_read_file"]); + let problems = check(&commands, &good_files()); + assert!(problems.iter().any(|p| p.contains("\"Check-Docker\"") && p.contains("[a-z0-9_]+")), "{problems:?}"); + assert!(problems.iter().any(|p| p.contains("check_docker is registered more than once")), "{problems:?}"); + } + + #[test] + fn every_problem_is_reported_in_one_pass() { + let files = vec![ + file("default.json", &["main"], &["allow-check-docker", "allow-nope", "deny-check-docker"]), + file("file-viewer.json", &["file-viewer-*"], &[]), + ]; + let problems = check(&cmds(THREE), &files); + // typo, deny, open_file_viewer missing, viewer_read_file missing + assert_eq!(problems.len(), 4, "{problems:?}"); + } +} diff --git a/app/src-tauri/src/lib.rs b/app/src-tauri/src/lib.rs index 9867e0e..9edb0a5 100644 --- a/app/src-tauri/src/lib.rs +++ b/app/src-tauri/src/lib.rs @@ -1,5 +1,7 @@ mod auth_bridge; mod browser_view; +#[cfg(test)] +mod command_census; mod commands; mod docker; pub mod file_viewer; @@ -844,30 +846,11 @@ mod tests { &mut defined, ); - // The registration list, read from this file rather than from a macro - // expansion so the test does not depend on `generate_handler!`'s shape. - let this = include_str!("lib.rs"); - let handler = this - .split_once("generate_handler![") - .and_then(|(_, rest)| rest.split_once("])")) - .map(|(inside, _)| inside) + // The registration list, read from this file by the same parser `build.rs` uses to + // declare the AppManifest — so if this test can see a command, the ACL can too. + let ordered = crate::command_census::registered_commands(include_str!("lib.rs")) .expect("lib.rs should contain a generate_handler! list"); - // Line-based, not `split(',')`: the list is grouped under `// Docker` - // style comments, and splitting on commas glues each comment to the - // command that follows it. A `starts_with("//")` filter then drops that - // command — silently, and once per group. - let registered: BTreeSet = handler - .lines() - .map(str::trim) - .filter(|l| !l.is_empty() && !l.starts_with("//")) - .filter_map(|l| { - l.trim_end_matches(',') - .rsplit("::") - .next() - .map(|n| n.trim().to_string()) - }) - .filter(|n| !n.is_empty()) - .collect(); + let registered: BTreeSet = ordered.iter().cloned().collect(); assert!( !defined.is_empty() && !registered.is_empty(), @@ -896,21 +879,11 @@ mod tests { // passed here. let mut seen: Vec<&str> = Vec::new(); let mut duplicated: Vec<&str> = Vec::new(); - for line in handler - .lines() - .map(str::trim) - .filter(|l| !l.is_empty() && !l.starts_with("//")) - { - if let Some(name) = line.trim_end_matches(',').rsplit("::").next() { - let name = name.trim(); - if name.is_empty() { - continue; - } - if seen.contains(&name) { - duplicated.push(name); - } else { - seen.push(name); - } + for name in &ordered { + if seen.contains(&name.as_str()) { + duplicated.push(name); + } else { + seen.push(name); } } assert!(