Creepso/agent-desktop
1
0
Fork 0
mirror of https://github.com/lahfir/agent-desktop.git synced 2026-07-27 01:22:16 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 4

fix: address code review findings (double-free, CF leaks, injection)

Browse source 
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
This commit is contained in:
Lahfir 2026-02-21 14:01:51 -08:00
parent a2319623b4
commit 2f495ffb69
8 changed files with 58 additions and 100 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

42
crates/macos/src/actions/activate.rs
View file

@ -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<String> {
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 &current {
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,
};

33
crates/macos/src/actions/dispatch.rs
View file

@ -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<ActionResult, AdapterError> {
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!(

2
crates/macos/src/actions/extras.rs
View file

@ -166,7 +166,7 @@ fn find_scroll_area(el: &AXElement) -> Option<AXElement> {
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)?;

2
crates/macos/src/adapter.rs
View file

@ -198,7 +198,7 @@ fn execute_action_impl(
Err(AdapterError::not_supported("execute_action"))
}
pub fn list_windows_impl(filter: &WindowFilter) -> Result<Vec<WindowInfo>, AdapterError> {
pub(crate) fn list_windows_impl(filter: &WindowFilter) -> Result<Vec<WindowInfo>, AdapterError> {
#[cfg(target_os = "macos")]
{
use core_foundation::base::{CFType, TCFType};

18
crates/macos/src/system/app_ops.rs
View file

@ -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"#
);

15
crates/macos/src/system/key_dispatch.rs
View file

@ -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<ActionResult, AdapterError> {
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<ActionResu
std::thread::sleep(std::time::Duration::from_millis(50));
if !combo.modifiers.is_empty() {
let app_ax = std::mem::ManuallyDrop::new(crate::tree::AXElement(app_el));
if let Some(result) = try_menu_bar_shortcut(&app_ax, combo) {
if let Some(result) = try_menu_bar_shortcut(&app_el, combo) {
return result;
}
}
let simple_result = try_simple_key_action(app_el, combo);
let simple_result = try_simple_key_action(app_el.0, combo);
if let Some(result) = simple_result {
return result;
}
ax_post_keyboard_event(app_el, combo)?;
ax_post_keyboard_event(app_el.0, combo)?;
Ok(ActionResult::new("press_key".to_string()))
}

11
crates/macos/src/tree/element.rs
View file

@ -144,7 +144,7 @@ mod imp {
name.or_else(|| {
let children = copy_ax_array(el, kAXChildrenAttribute).unwrap_or_default();
super::super::builder::label_from_children(&children)
crate::tree::builder::label_from_children(&children)
})
}
@ -188,7 +188,7 @@ mod imp {
None
}
fn copy_bool_attr(el: &AXElement, attr: &str) -> Option<bool> {
pub fn copy_bool_attr(el: &AXElement, attr: &str) -> Option<bool> {
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<String> {
None
}
pub fn copy_bool_attr(_el: &AXElement, _attr: &str) -> Option<bool> {
None
}
pub fn copy_element_attr(_el: &AXElement, _attr: &str) -> Option<AXElement> {
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,
};

35
crates/macos/src/tree/surfaces.rs
View file

@ -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<AXElement> {
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<bool> {
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::<CFBoolean>().map(|b| b.into())
}
fn focused_window_element(pid: i32) -> Option<AXElement> {
let app = element_for_pid(pid);