diff --git a/crates/sage-apps/src/lifecycle/install.rs b/crates/sage-apps/src/lifecycle/install.rs index 4b61e4392..0e5eb07fc 100644 --- a/crates/sage-apps/src/lifecycle/install.rs +++ b/crates/sage-apps/src/lifecycle/install.rs @@ -17,10 +17,10 @@ use tauri::{AppHandle, Manager, State}; use uuid::Uuid; use crate::{ - AppsHostState, SageAppCommon, SageAppIdentity, SageAppPackageManifest, SageAppSnapshot, - SageAppStorage, SageAppWalletScope, SageGrantedPermissionsInput, UserSageApp, - UserSageAppSource, allocate_new_storage, apps_root, emit_listed_apps_changed, - fresh_snapshot_dir, write_snapshot_manifest, + AppsHostState, RUNTIME_ID_PREFIX, SANDBOX_TEST_ID_PREFIX, SageAppCommon, SageAppIdentity, + SageAppPackageManifest, SageAppSnapshot, SageAppStorage, SageAppWalletScope, + SageGrantedPermissionsInput, UserSageApp, UserSageAppSource, allocate_new_storage, apps_root, + builtin_system_app_spec, emit_listed_apps_changed, fresh_snapshot_dir, write_snapshot_manifest, }; #[async_trait] @@ -100,6 +100,8 @@ where let (app_id, app_dir) = source.resolve_target(&root, base_path, &prepared_artifact)?; + ensure_installable_app_id(&app_id)?; + if host_state.db.app_exists(&app_id).await? { anyhow::bail!("App is already installed"); } @@ -225,6 +227,25 @@ pub fn fresh_origin_id(app_id: &str) -> String { format!("{}.{}", Uuid::new_v4(), app_id) } +/// Rejects app IDs that are reserved for builtin apps. +/// +/// IDs with the sandbox-test or runtime prefixes get special treatment (for +/// example, `__sage_test_` apps bypass the sandbox launch gate), so an +/// installed app must never be able to claim one. Current ID generators can't +/// produce these, but this must hold structurally rather than by coincidence +/// of slugification. +fn ensure_installable_app_id(app_id: &str) -> AnyResult<()> { + if app_id.starts_with(SANDBOX_TEST_ID_PREFIX) || app_id.starts_with(RUNTIME_ID_PREFIX) { + anyhow::bail!("app id uses a reserved prefix: {app_id}"); + } + + if builtin_system_app_spec(app_id).is_some() { + anyhow::bail!("app id collides with a builtin system app: {app_id}"); + } + + Ok(()) +} + pub fn create_app_dir(app_dir: &Path) -> AnyResult<()> { if app_dir.exists() { anyhow::bail!("app directory already exists, cannot create"); @@ -401,6 +422,35 @@ mod tests { assert_eq!(installed.source(), &UserSageAppSource::Zip); } + #[test] + fn ensure_installable_app_id_rejects_reserved_ids() { + for app_id in [ + "__sage_test_storage_isolation_persistent", + "__sage_test_anything", + "__sage_runtime_origin_cleanup", + "task-manager", + "app-update", + "bridge-approval", + ] { + assert!( + ensure_installable_app_id(app_id).is_err(), + "expected app id {app_id:?} to be rejected" + ); + } + } + + #[test] + fn ensure_installable_app_id_accepts_generated_ids() { + for app_id in [ + "my-app-8c8532e1-14d8-4d5f-b652-4ff29fc35a19", + "url-my-app-0123456789abcdef", + "task-manager-8c8532e1-14d8-4d5f-b652-4ff29fc35a19", + ] { + ensure_installable_app_id(app_id) + .unwrap_or_else(|err| panic!("expected app id {app_id:?} to be accepted: {err}")); + } + } + #[test] fn create_app_dir_rejects_existing_directory() { let dir = tempdir().unwrap(); diff --git a/crates/sage-apps/src/sandbox/builtin_apps.rs b/crates/sage-apps/src/sandbox/builtin_apps.rs index ff3cacbf1..97c70ee48 100644 --- a/crates/sage-apps/src/sandbox/builtin_apps.rs +++ b/crates/sage-apps/src/sandbox/builtin_apps.rs @@ -25,6 +25,7 @@ macro_rules! runtime_id_prefix { } pub const SANDBOX_TEST_ID_PREFIX: &str = sandbox_test_id_prefix!(); +pub const RUNTIME_ID_PREFIX: &str = runtime_id_prefix!(); pub const BUILTIN_STORAGE_ISOLATION_PERSISTENT_ID: &str = concat!(sandbox_test_id_prefix!(), "storage_isolation_persistent"); diff --git a/crates/sage-apps/src/types/storage.rs b/crates/sage-apps/src/types/storage.rs index 86c7eb9ad..c1921a6df 100644 --- a/crates/sage-apps/src/types/storage.rs +++ b/crates/sage-apps/src/types/storage.rs @@ -1,10 +1,96 @@ -use serde::{Deserialize, Serialize}; +use serde::{Deserialize, Deserializer, Serialize}; use specta::Type; #[derive(Debug, Clone, Serialize, Deserialize, Type, PartialEq, Eq)] #[serde(tag = "kind", rename_all = "camelCase")] pub enum SageAppStorage { - AppleDataStore { identifier_hex: String }, - WindowsProfile { directory_name: String }, + AppleDataStore { + identifier_hex: String, + }, + WindowsProfile { + #[serde(deserialize_with = "deserialize_profile_directory_name")] + directory_name: String, + }, Unmanaged, } + +/// Validates a Windows profile directory name loaded from persisted state. +/// +/// The name is later joined onto the app data directory (including for +/// `remove_dir_all` during storage cleanup), so a tampered database row like +/// `".."` or `"..\\.."` must never be accepted: it could escape the profiles +/// directory and delete arbitrary data. Legitimate names are only ever +/// `profile-` or `builtin-profile-`. +pub(crate) fn validate_profile_directory_name(name: &str) -> Result<(), String> { + if name.is_empty() { + return Err("profile directory name must not be empty".to_string()); + } + + if !name + .chars() + .all(|c| c.is_ascii_alphanumeric() || matches!(c, '-' | '_')) + { + return Err(format!("invalid profile directory name: {name}")); + } + + Ok(()) +} + +fn deserialize_profile_directory_name<'de, D>(deserializer: D) -> Result +where + D: Deserializer<'de>, +{ + let name = String::deserialize(deserializer)?; + validate_profile_directory_name(&name).map_err(serde::de::Error::custom)?; + Ok(name) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn windows_profile_json(directory_name: &str) -> String { + let valid = serde_json::to_string(&SageAppStorage::WindowsProfile { + directory_name: "PLACEHOLDER".to_string(), + }) + .unwrap(); + + valid.replace("PLACEHOLDER", directory_name) + } + + #[test] + fn windows_profile_accepts_expected_directory_names() { + for name in [ + "profile-8c8532e1-14d8-4d5f-b652-4ff29fc35a19", + "builtin-profile-__sage_test_storage_isolation_persistent", + ] { + let storage: SageAppStorage = + serde_json::from_str(&windows_profile_json(name)).unwrap(); + assert_eq!( + storage, + SageAppStorage::WindowsProfile { + directory_name: name.to_string() + } + ); + } + } + + #[test] + fn windows_profile_rejects_traversal_and_separator_directory_names() { + for name in [ + "..", + "../evil", + "..\\\\evil", + "profiles/../..", + "a/b", + ".", + "profile-abc.def", + "", + ] { + assert!( + serde_json::from_str::(&windows_profile_json(name)).is_err(), + "expected directory name {name:?} to be rejected" + ); + } + } +}