From 2f495ffb69be67f3136b076534e078cc31b005c2 Mon Sep 17 00:00:00 2001 From: Lahfir Date: Sat, 21 Feb 2026 14:01:51 -0800 Subject: [PATCH] fix: address code review findings (double-free, CF leaks, injection) P1: fix double-free in find_scroll_area (el.clone() instead of AXElement(el.0)) P1: add input validation in close_app_impl to prevent AppleScript injection P2: fix CFRelease leaks in focus_window_impl and press_for_app_impl P2: remove duplicated copy_element_attr/copy_bool_attr from surfaces.rs P3: replace is_attr_settable_pub wrapper with direct pub export P3: extract TOGGLEABLE_ROLES constant, collapse try_parent_activation loop P3: fix fragile super::super::builder path, tighten list_windows_impl visibility --- crates/macos/src/actions/activate.rs | 42 ++++++++----------------- crates/macos/src/actions/dispatch.rs | 33 ++++++++----------- crates/macos/src/actions/extras.rs | 2 +- crates/macos/src/adapter.rs | 2 +- crates/macos/src/system/app_ops.rs | 18 ++++++++--- crates/macos/src/system/key_dispatch.rs | 15 +++++---- crates/macos/src/tree/element.rs | 11 ++++--- crates/macos/src/tree/surfaces.rs | 35 ++------------------- 8 files changed, 58 insertions(+), 100 deletions(-) diff --git a/crates/macos/src/actions/activate.rs b/crates/macos/src/actions/activate.rs index 4b1626a..0cce80c 100644 --- a/crates/macos/src/actions/activate.rs +++ b/crates/macos/src/actions/activate.rs @@ -84,10 +84,6 @@ mod imp { crate::actions::dispatch::click_via_bounds(el, MouseButton::Left, 3) } - pub fn is_attr_settable_pub(el: &AXElement, attr: &str) -> bool { - is_attr_settable(el, attr) - } - fn list_ax_actions(el: &AXElement) -> Vec { let mut actions_ref: core_foundation_sys::array::CFArrayRef = std::ptr::null(); let err = unsafe { AXUIElementCopyActionNames(el.0, &mut actions_ref) }; @@ -104,7 +100,7 @@ mod imp { result } - fn is_attr_settable(el: &AXElement, attr: &str) -> bool { + pub fn is_attr_settable(el: &AXElement, attr: &str) -> bool { let cf_attr = CFString::new(attr); let mut settable: c_uchar = 0; let err = unsafe { @@ -211,33 +207,21 @@ mod imp { } fn try_parent_activation(el: &AXElement) -> bool { - if let Some(parent) = crate::tree::copy_element_attr(el, "AXParent") { - let press = CFString::new("AXPress"); - if unsafe { AXUIElementPerformAction(parent.0, press.as_concrete_TypeRef()) } - == kAXErrorSuccess - { - return true; - } - let confirm = CFString::new("AXConfirm"); - if unsafe { AXUIElementPerformAction(parent.0, confirm.as_concrete_TypeRef()) } - == kAXErrorSuccess - { - return true; - } - if let Some(grandparent) = crate::tree::copy_element_attr(&parent, "AXParent") { - let press = CFString::new("AXPress"); - if unsafe { AXUIElementPerformAction(grandparent.0, press.as_concrete_TypeRef()) } - == kAXErrorSuccess - { - return true; - } - let confirm = CFString::new("AXConfirm"); - if unsafe { AXUIElementPerformAction(grandparent.0, confirm.as_concrete_TypeRef()) } + let mut current = crate::tree::copy_element_attr(el, "AXParent"); + for _ in 0..2 { + let ancestor = match ¤t { + Some(a) => a, + None => return false, + }; + for action_name in &["AXPress", "AXConfirm"] { + let action = CFString::new(action_name); + if unsafe { AXUIElementPerformAction(ancestor.0, action.as_concrete_TypeRef()) } == kAXErrorSuccess { return true; } } + current = crate::tree::copy_element_attr(ancestor, "AXParent"); } false } @@ -260,12 +244,12 @@ mod imp { pub fn smart_triple_activate(_el: &AXElement) -> Result<(), AdapterError> { Err(AdapterError::not_supported("smart_triple_activate")) } - pub fn is_attr_settable_pub(_el: &AXElement, _attr: &str) -> bool { + pub fn is_attr_settable(_el: &AXElement, _attr: &str) -> bool { false } } pub(crate) use imp::{ - is_attr_settable_pub, smart_activate, smart_double_activate, smart_right_activate, + is_attr_settable, smart_activate, smart_double_activate, smart_right_activate, smart_triple_activate, }; diff --git a/crates/macos/src/actions/dispatch.rs b/crates/macos/src/actions/dispatch.rs index 3e22a86..4920519 100644 --- a/crates/macos/src/actions/dispatch.rs +++ b/crates/macos/src/actions/dispatch.rs @@ -57,6 +57,15 @@ mod imp { }) } + const TOGGLEABLE_ROLES: &[&str] = &[ + "checkbox", + "switch", + "radiobutton", + "togglebutton", + "menuitemcheckbox", + "menuitemradio", + ]; + pub fn perform_action(el: &AXElement, action: &Action) -> Result { let label = action_label(action); match action { @@ -74,15 +83,7 @@ mod imp { Action::Toggle => { let role = element_role(el); - let toggle_roles = [ - "checkbox", - "switch", - "radiobutton", - "togglebutton", - "menuitemcheckbox", - "menuitemradio", - ]; - if !toggle_roles.iter().any(|r| role.as_deref() == Some(*r)) { + if !TOGGLEABLE_ROLES.iter().any(|r| role.as_deref() == Some(*r)) { return Err(AdapterError::new( ErrorCode::ActionNotSupported, format!( @@ -136,7 +137,7 @@ mod imp { Action::Expand => { if !try_ax_action(el, "AXExpand") { - if crate::actions::activate::is_attr_settable_pub(el, "AXDisclosing") { + if crate::actions::activate::is_attr_settable(el, "AXDisclosing") { let cf_attr = CFString::new("AXDisclosing"); let err = unsafe { AXUIElementSetAttributeValue( @@ -163,7 +164,7 @@ mod imp { Action::Collapse => { if !try_ax_action(el, "AXCollapse") { - if crate::actions::activate::is_attr_settable_pub(el, "AXDisclosing") { + if crate::actions::activate::is_attr_settable(el, "AXDisclosing") { let cf_attr = CFString::new("AXDisclosing"); let err = unsafe { AXUIElementSetAttributeValue( @@ -296,15 +297,7 @@ mod imp { fn check_uncheck(el: &AXElement, want_checked: bool) -> Result<(), AdapterError> { let role = element_role(el); - let valid_roles = [ - "checkbox", - "switch", - "radiobutton", - "togglebutton", - "menuitemcheckbox", - "menuitemradio", - ]; - if !valid_roles.iter().any(|r| role.as_deref() == Some(*r)) { + if !TOGGLEABLE_ROLES.iter().any(|r| role.as_deref() == Some(*r)) { return Err(AdapterError::new( ErrorCode::ActionNotSupported, format!( diff --git a/crates/macos/src/actions/extras.rs b/crates/macos/src/actions/extras.rs index 01cedc3..21ba201 100644 --- a/crates/macos/src/actions/extras.rs +++ b/crates/macos/src/actions/extras.rs @@ -166,7 +166,7 @@ fn find_scroll_area(el: &AXElement) -> Option { let role = crate::tree::copy_string_attr(el, kAXRoleAttribute)?; if role == "AXScrollArea" { - return Some(AXElement(el.0)); + return Some(el.clone()); } let parent = crate::tree::copy_element_attr(el, "AXParent")?; let parent_role = crate::tree::copy_string_attr(&parent, kAXRoleAttribute)?; diff --git a/crates/macos/src/adapter.rs b/crates/macos/src/adapter.rs index 3bafdb5..d22705e 100644 --- a/crates/macos/src/adapter.rs +++ b/crates/macos/src/adapter.rs @@ -198,7 +198,7 @@ fn execute_action_impl( Err(AdapterError::not_supported("execute_action")) } -pub fn list_windows_impl(filter: &WindowFilter) -> Result, AdapterError> { +pub(crate) fn list_windows_impl(filter: &WindowFilter) -> Result, AdapterError> { #[cfg(target_os = "macos")] { use core_foundation::base::{CFType, TCFType}; diff --git a/crates/macos/src/system/app_ops.rs b/crates/macos/src/system/app_ops.rs index 1694e72..c807c3a 100644 --- a/crates/macos/src/system/app_ops.rs +++ b/crates/macos/src/system/app_ops.rs @@ -8,15 +8,15 @@ pub fn focus_window_impl(win: &WindowInfo) -> Result<(), AdapterError> { }; use core_foundation::{base::TCFType, boolean::CFBoolean, string::CFString}; - let app_el = unsafe { AXUIElementCreateApplication(win.pid) }; - if app_el.is_null() { + let app_el = crate::tree::AXElement(unsafe { AXUIElementCreateApplication(win.pid) }); + if app_el.0.is_null() { return Err(AdapterError::internal("Failed to create AX app element")); } let frontmost_attr = CFString::new("AXFrontmost"); let err = unsafe { AXUIElementSetAttributeValue( - app_el, + app_el.0, frontmost_attr.as_concrete_TypeRef(), CFBoolean::true_value().as_CFTypeRef(), ) @@ -126,10 +126,18 @@ pub fn close_app_impl(id: &str, force: bool) -> Result<(), AdapterError> { let app_ax = crate::tree::element_for_pid(pid); let closed = try_quit_via_menu_bar(&app_ax); if !closed { - let safe_name = id.replace('"', ""); + if id + .chars() + .any(|c| !c.is_alphanumeric() && !matches!(c, ' ' | '-' | '.' | '_')) + { + return Err(AdapterError::new( + agent_desktop_core::error::ErrorCode::InvalidArgs, + format!("Invalid app name '{id}'"), + )); + } let script = format!( r#"tell application "System Events" - set theProc to first process whose name is "{safe_name}" + set theProc to first process whose name is "{id}" tell theProc to quit end tell"# ); diff --git a/crates/macos/src/system/key_dispatch.rs b/crates/macos/src/system/key_dispatch.rs index 58ceb36..002c5a7 100644 --- a/crates/macos/src/system/key_dispatch.rs +++ b/crates/macos/src/system/key_dispatch.rs @@ -8,19 +8,19 @@ use agent_desktop_core::{action::Modifier, adapter::WindowFilter}; #[cfg(target_os = "macos")] pub fn press_for_app_impl(app_name: &str, combo: &KeyCombo) -> Result { - use accessibility_sys::{AXUIElementCreateApplication, AXUIElementSetAttributeValue}; + use accessibility_sys::AXUIElementSetAttributeValue; use core_foundation::{base::TCFType, boolean::CFBoolean, string::CFString}; let pid = find_pid_by_name(app_name)?; - let app_el = unsafe { AXUIElementCreateApplication(pid) }; - if app_el.is_null() { + let app_el = crate::tree::element_for_pid(pid); + if app_el.0.is_null() { return Err(AdapterError::internal("Failed to create AX app element")); } let frontmost_attr = CFString::new("AXFrontmost"); unsafe { AXUIElementSetAttributeValue( - app_el, + app_el.0, frontmost_attr.as_concrete_TypeRef(), CFBoolean::true_value().as_CFTypeRef(), ) @@ -28,18 +28,17 @@ pub fn press_for_app_impl(app_name: &str, combo: &KeyCombo) -> Result Option { + pub fn copy_bool_attr(el: &AXElement, attr: &str) -> Option { let cf_attr = CFString::new(attr); let mut value: CFTypeRef = std::ptr::null_mut(); let err = unsafe { @@ -323,6 +323,9 @@ mod imp { pub fn copy_string_attr(_el: &AXElement, _attr: &str) -> Option { None } + pub fn copy_bool_attr(_el: &AXElement, _attr: &str) -> Option { + None + } pub fn copy_element_attr(_el: &AXElement, _attr: &str) -> Option { None } @@ -347,6 +350,6 @@ mod imp { } pub use imp::{ - copy_ax_array, copy_element_attr, copy_string_attr, element_for_pid, fetch_node_attrs, - read_bounds, resolve_element_name, AXElement, + copy_ax_array, copy_bool_attr, copy_element_attr, copy_string_attr, element_for_pid, + fetch_node_attrs, read_bounds, resolve_element_name, AXElement, }; diff --git a/crates/macos/src/tree/surfaces.rs b/crates/macos/src/tree/surfaces.rs index e8fe6cc..a8de6ee 100644 --- a/crates/macos/src/tree/surfaces.rs +++ b/crates/macos/src/tree/surfaces.rs @@ -1,40 +1,11 @@ -use super::element::{copy_ax_array, copy_string_attr, element_for_pid, AXElement}; +use super::element::{ + copy_ax_array, copy_bool_attr, copy_element_attr, copy_string_attr, element_for_pid, AXElement, +}; use agent_desktop_core::node::SurfaceInfo; #[cfg(target_os = "macos")] mod imp { use super::*; - use accessibility_sys::{kAXErrorSuccess, AXUIElementCopyAttributeValue, AXUIElementRef}; - use core_foundation::{ - base::{CFType, CFTypeRef, TCFType}, - boolean::CFBoolean, - string::CFString, - }; - - fn copy_element_attr(el: &AXElement, attr: &str) -> Option { - let cf_attr = CFString::new(attr); - let mut value: CFTypeRef = std::ptr::null_mut(); - let err = unsafe { - AXUIElementCopyAttributeValue(el.0, cf_attr.as_concrete_TypeRef(), &mut value) - }; - if err != kAXErrorSuccess || value.is_null() { - return None; - } - Some(AXElement(value as AXUIElementRef)) - } - - fn copy_bool_attr(el: &AXElement, attr: &str) -> Option { - let cf_attr = CFString::new(attr); - let mut value: CFTypeRef = std::ptr::null_mut(); - let err = unsafe { - AXUIElementCopyAttributeValue(el.0, cf_attr.as_concrete_TypeRef(), &mut value) - }; - if err != kAXErrorSuccess || value.is_null() { - return None; - } - let cf_type = unsafe { CFType::wrap_under_create_rule(value) }; - cf_type.downcast::().map(|b| b.into()) - } fn focused_window_element(pid: i32) -> Option { let app = element_for_pid(pid);