From 394e29ba13b3c07edbd86639a623d5308f5bbdf3 Mon Sep 17 00:00:00 2001
From: Lahfir <nmhlahfir2@gmail.com>
Date: Thu, 16 Apr 2026 06:18:04 -0700
Subject: [PATCH] fix(ffi): UTF-8 fail-closed for optional filter pointers
 (todo 010)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Closes the UTF-8 portion of P2 todo 010. The resolver relaxed-pass
stays — it's the right behavior for ad_find's lossy-rebuild fallback
and the recently-landed bounds_hash preservation already constrains
the duplicate-label drift case.

The UTF-8 side needed a tighter contract. `c_to_string` returned
`Option<String>` and collapsed null + invalid-bytes into the same
`None`, so an app_filter passed in with hostile bytes was silently
treated as "no filter" and widened ad_list_windows,
ad_dismiss_all_notifications, ad_find, etc. to every app on the box.

Add `try_c_to_string` returning `Result<Option<String>, ()>`:
- `Ok(None)` — null pointer (caller treats as absent).
- `Ok(Some(s))` — valid UTF-8.
- `Err(())` — non-null + invalid UTF-8 (caller must surface
  AD_RESULT_ERR_INVALID_ARGS).

Add `decode_optional_filter!` macro so call sites read as a single
line:
  let role_filter = decode_optional_filter!(q.role, "query.role");
The macro shortcircuits the enclosing AdResult fn with InvalidArgs
and a tailored last-error ("<label> is not valid UTF-8").

Applied to every optional filter pointer consumer:
- observation/find.rs: role, name_substring, value_substring
- observation/is.rs: role, name_substring, value_substring
- windows/list.rs: app_filter
- notifications/dismiss.rs: app_filter
- notifications/dismiss_all.rs: app_filter
- notifications/filter.rs: filter_from_c returns Result; app/text
  fields fail-closed. list_notifications call site propagates.

Regression test in c_abi_harness.rs:
- invalid_utf8_filter_rejected_not_silently_widened: passes a
  truncated-UTF-8 byte pair as ad_list_windows app_filter; must return
  InvalidArgs (or ErrInternal on worker) with list still null, never
  a populated list built from the widened "all apps" scope.

85 FFI tests pass, clippy clean.
---
 crates/ffi/src/convert/string.rs            | 65 +++++++++++++++++++++
 crates/ffi/src/notifications/dismiss.rs     |  4 +-
 crates/ffi/src/notifications/dismiss_all.rs |  4 +-
 crates/ffi/src/notifications/filter.rs      | 30 +++++++---
 crates/ffi/src/notifications/list.rs        |  8 ++-
 crates/ffi/src/observation/find.rs          |  8 +--
 crates/ffi/src/observation/is.rs            |  8 +--
 crates/ffi/src/windows/list.rs              |  4 +-
 crates/ffi/tests/c_abi_harness.rs           | 21 +++++++
 9 files changed, 129 insertions(+), 23 deletions(-)

diff --git a/crates/ffi/src/convert/string.rs b/crates/ffi/src/convert/string.rs
index 3fb15c3..91b4b93 100644
--- a/crates/ffi/src/convert/string.rs
+++ b/crates/ffi/src/convert/string.rs
@@ -48,6 +48,50 @@ pub(crate) unsafe fn c_to_string(ptr: *const c_char) -> Option<String> {
     CStr::from_ptr(ptr).to_str().ok().map(str::to_owned)
 }
 
+/// Tri-state decode of a foreign C string used for optional filter
+/// fields where conflating null with invalid UTF-8 would silently widen
+/// an operation (e.g. treat "bad bytes" as "no filter").
+///
+/// - `Ok(None)` — pointer is null. Caller should treat as "filter
+///   absent".
+/// - `Ok(Some(s))` — pointer is non-null and decodes as valid UTF-8.
+/// - `Err(())` — pointer is non-null but the bytes are not UTF-8.
+///   Caller should surface `AD_RESULT_ERR_INVALID_ARGS` instead of
+///   treating this as missing.
+///
+/// # Safety
+/// `ptr` must be null or a NUL-terminated C string.
+pub(crate) unsafe fn try_c_to_string(ptr: *const c_char) -> Result<Option<String>, ()> {
+    if ptr.is_null() {
+        return Ok(None);
+    }
+    match CStr::from_ptr(ptr).to_str() {
+        Ok(s) => Ok(Some(s.to_owned())),
+        Err(_) => Err(()),
+    }
+}
+
+/// Decode an optional filter string, short-circuiting the enclosing
+/// `AdResult`-returning fn with `AD_RESULT_ERR_INVALID_ARGS` (and a
+/// tailored last-error diagnostic) when the pointer is non-null but
+/// the bytes are not UTF-8. Null → `None` (treated as "no filter").
+macro_rules! decode_optional_filter {
+    ($ptr:expr, $label:expr) => {{
+        match $crate::convert::string::try_c_to_string($ptr) {
+            Ok(value) => value,
+            Err(()) => {
+                $crate::error::set_last_error(&agent_desktop_core::error::AdapterError::new(
+                    agent_desktop_core::error::ErrorCode::InvalidArgs,
+                    concat!($label, " is not valid UTF-8"),
+                ));
+                return $crate::error::AdResult::ErrInvalidArgs;
+            }
+        }
+    }};
+}
+
+pub(crate) use decode_optional_filter;
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -100,4 +144,25 @@ mod tests {
         assert_eq!(back, "\u{FFFD}\u{FFFD}\u{FFFD}");
         unsafe { free_c_string(c) };
     }
+
+    #[test]
+    fn try_c_to_string_null_is_ok_none() {
+        let result = unsafe { try_c_to_string(ptr::null()) };
+        assert!(matches!(result, Ok(None)));
+    }
+
+    #[test]
+    fn try_c_to_string_valid_utf8_is_some() {
+        let c = string_to_c("agent");
+        let result = unsafe { try_c_to_string(c) };
+        assert!(matches!(result, Ok(Some(ref s)) if s == "agent"));
+        unsafe { free_c_string(c) };
+    }
+
+    #[test]
+    fn try_c_to_string_invalid_utf8_is_err() {
+        let bad: [u8; 3] = [0xC3, 0xFF, 0x00];
+        let result = unsafe { try_c_to_string(bad.as_ptr() as *const c_char) };
+        assert!(matches!(result, Err(())));
+    }
 }
diff --git a/crates/ffi/src/notifications/dismiss.rs b/crates/ffi/src/notifications/dismiss.rs
index e695211..81269a6 100644
--- a/crates/ffi/src/notifications/dismiss.rs
+++ b/crates/ffi/src/notifications/dismiss.rs
@@ -1,4 +1,4 @@
-use crate::convert::string::c_to_string;
+use crate::convert::string::decode_optional_filter;
 use crate::error::{set_last_error, AdResult};
 use crate::ffi_try::trap_panic;
 use crate::AdAdapter;
@@ -23,7 +23,7 @@ pub unsafe extern "C" fn ad_dismiss_notification(
         }
         crate::pointer_guard::guard_non_null!(adapter, c"adapter is null");
         let adapter = &*adapter;
-        let filter = c_to_string(app_filter);
+        let filter = decode_optional_filter!(app_filter, "app_filter");
         let filter_ref = filter.as_deref();
         match adapter
             .inner
diff --git a/crates/ffi/src/notifications/dismiss_all.rs b/crates/ffi/src/notifications/dismiss_all.rs
index 1816500..c26d72f 100644
--- a/crates/ffi/src/notifications/dismiss_all.rs
+++ b/crates/ffi/src/notifications/dismiss_all.rs
@@ -1,5 +1,5 @@
 use crate::convert::notification::notification_info_to_c;
-use crate::convert::string::c_to_string;
+use crate::convert::string::decode_optional_filter;
 use crate::error::{set_last_error, AdResult};
 use crate::ffi_try::trap_panic;
 use crate::notifications::list::ad_notification_list_free;
@@ -40,7 +40,7 @@ pub unsafe extern "C" fn ad_dismiss_all_notifications(
         }
         crate::pointer_guard::guard_non_null!(adapter, c"adapter is null");
         let adapter = &*adapter;
-        let filter = c_to_string(app_filter);
+        let filter = decode_optional_filter!(app_filter, "app_filter");
         let filter_ref = filter.as_deref();
         match adapter.inner.dismiss_all_notifications(filter_ref) {
             Ok((dismissed, failed_messages)) => {
diff --git a/crates/ffi/src/notifications/filter.rs b/crates/ffi/src/notifications/filter.rs
index be5721c..6c3b72a 100644
--- a/crates/ffi/src/notifications/filter.rs
+++ b/crates/ffi/src/notifications/filter.rs
@@ -1,25 +1,39 @@
-use crate::convert::string::c_to_string;
+use crate::convert::string::try_c_to_string;
 use crate::types::AdNotificationFilter;
+use agent_desktop_core::error::{AdapterError, ErrorCode};
 use agent_desktop_core::notification::NotificationFilter;
 
 /// Converts a C `AdNotificationFilter` into the core filter type.
-/// Null pointers become `None`; `has_limit == false` clears the limit.
+///
+/// - `null` pointer → `Ok(NotificationFilter::default())` (no filter).
+/// - `has_limit == false` → the numeric limit is cleared regardless of
+///   `limit` contents.
+/// - Non-null `app` / `text` with invalid UTF-8 → `Err` rather than
+///   silently dropping the filter (which would widen operations like
+///   `ad_dismiss_all_notifications` to every app on the system).
 ///
 /// # Safety
 /// `filter` must be null or point to a valid `AdNotificationFilter`.
 /// The embedded C strings must outlive this call.
-pub(crate) unsafe fn filter_from_c(filter: *const AdNotificationFilter) -> NotificationFilter {
+pub(crate) unsafe fn filter_from_c(
+    filter: *const AdNotificationFilter,
+) -> Result<NotificationFilter, AdapterError> {
     if filter.is_null() {
-        return NotificationFilter::default();
+        return Ok(NotificationFilter::default());
     }
     let f: &AdNotificationFilter = unsafe { &*filter };
-    NotificationFilter {
-        app: unsafe { c_to_string(f.app) },
-        text: unsafe { c_to_string(f.text) },
+    let app = unsafe { try_c_to_string(f.app) }
+        .map_err(|()| AdapterError::new(ErrorCode::InvalidArgs, "filter.app is not valid UTF-8"))?;
+    let text = unsafe { try_c_to_string(f.text) }.map_err(|()| {
+        AdapterError::new(ErrorCode::InvalidArgs, "filter.text is not valid UTF-8")
+    })?;
+    Ok(NotificationFilter {
+        app,
+        text,
         limit: if f.has_limit {
             Some(f.limit as usize)
         } else {
             None
         },
-    }
+    })
 }
diff --git a/crates/ffi/src/notifications/list.rs b/crates/ffi/src/notifications/list.rs
index dcfc2de..0d7aa75 100644
--- a/crates/ffi/src/notifications/list.rs
+++ b/crates/ffi/src/notifications/list.rs
@@ -31,7 +31,13 @@ pub unsafe extern "C" fn ad_list_notifications(
         }
         crate::pointer_guard::guard_non_null!(adapter, c"adapter is null");
         let adapter = &*adapter;
-        let core_filter = filter_from_c(filter);
+        let core_filter = match filter_from_c(filter) {
+            Ok(f) => f,
+            Err(e) => {
+                set_last_error(&e);
+                return crate::error::last_error_code();
+            }
+        };
         match adapter.inner.list_notifications(&core_filter) {
             Ok(notifications) => {
                 let items: Vec<AdNotificationInfo> =
diff --git a/crates/ffi/src/observation/find.rs b/crates/ffi/src/observation/find.rs
index eee739f..2f24459 100644
--- a/crates/ffi/src/observation/find.rs
+++ b/crates/ffi/src/observation/find.rs
@@ -1,4 +1,4 @@
-use crate::convert::string::c_to_string;
+use crate::convert::string::decode_optional_filter;
 use crate::error::{set_last_error, AdResult};
 use crate::ffi_try::trap_panic;
 use crate::observation::walk::find_first_match;
@@ -47,9 +47,9 @@ pub unsafe extern "C" fn ad_find(
             }
         };
         let q = &*query;
-        let role_filter = c_to_string(q.role);
-        let name_filter = c_to_string(q.name_substring);
-        let value_filter = c_to_string(q.value_substring);
+        let role_filter = decode_optional_filter!(q.role, "query.role");
+        let name_filter = decode_optional_filter!(q.name_substring, "query.name_substring");
+        let value_filter = decode_optional_filter!(q.value_substring, "query.value_substring");
 
         // include_bounds must be true: the resolver disambiguates
         // duplicate-label siblings using bounds_hash, and without the
diff --git a/crates/ffi/src/observation/is.rs b/crates/ffi/src/observation/is.rs
index 412fc91..5494815 100644
--- a/crates/ffi/src/observation/is.rs
+++ b/crates/ffi/src/observation/is.rs
@@ -1,4 +1,4 @@
-use crate::convert::string::c_to_string;
+use crate::convert::string::{c_to_string, decode_optional_filter};
 use crate::error::{set_last_error, AdResult};
 use crate::ffi_try::trap_panic;
 use crate::types::{AdFindQuery, AdWindowInfo};
@@ -62,9 +62,9 @@ pub unsafe extern "C" fn ad_is(
             }
         };
         let q = &*query;
-        let role_filter = c_to_string(q.role);
-        let name_filter = c_to_string(q.name_substring);
-        let value_filter = c_to_string(q.value_substring);
+        let role_filter = decode_optional_filter!(q.role, "query.role");
+        let name_filter = decode_optional_filter!(q.name_substring, "query.name_substring");
+        let value_filter = decode_optional_filter!(q.value_substring, "query.value_substring");
         let prop = match c_to_string(property) {
             Some(s) => s,
             None => {
diff --git a/crates/ffi/src/windows/list.rs b/crates/ffi/src/windows/list.rs
index 09d8219..93e7059 100644
--- a/crates/ffi/src/windows/list.rs
+++ b/crates/ffi/src/windows/list.rs
@@ -1,4 +1,4 @@
-use crate::convert::string::c_to_string;
+use crate::convert::string::decode_optional_filter;
 use crate::convert::window::{free_window_info_fields, window_info_to_c};
 use crate::error::{set_last_error, AdResult};
 use crate::ffi_try::{trap_panic, trap_panic_void};
@@ -29,7 +29,7 @@ pub unsafe extern "C" fn ad_list_windows(
         let adapter = &*adapter;
         let filter = WindowFilter {
             focused_only,
-            app: c_to_string(app_filter),
+            app: decode_optional_filter!(app_filter, "app_filter"),
         };
         match adapter.inner.list_windows(&filter) {
             Ok(windows) => {
diff --git a/crates/ffi/tests/c_abi_harness.rs b/crates/ffi/tests/c_abi_harness.rs
index eb12db6..523c6f7 100644
--- a/crates/ffi/tests/c_abi_harness.rs
+++ b/crates/ffi/tests/c_abi_harness.rs
@@ -198,6 +198,27 @@ fn dirty_out_param_is_cleared_before_early_return_on_worker_thread() {
     });
 }
 
+#[test]
+fn invalid_utf8_filter_rejected_not_silently_widened() {
+    // Regression for todo 010: prior c_to_string conflated null with
+    // invalid UTF-8, so a non-null buffer with bogus bytes in the
+    // app_filter slot would be treated as "no filter" and widen
+    // ad_list_windows to every app on the system. Must now fail closed.
+    with_adapter(|adapter| unsafe {
+        let bad: [u8; 2] = [0xC3, 0x00];
+        let mut list: *mut AdWindowList = std::ptr::null_mut();
+        let rc = ad_list_windows(adapter, bad.as_ptr() as *const c_char, false, &mut list);
+        // Main-thread guard (ErrInternal on worker) or UTF-8 rejection
+        // (ErrInvalidArgs) — either way we do NOT produce a list by
+        // silently treating bad bytes as "no filter".
+        assert!(matches!(
+            rc,
+            AdResult::ErrInvalidArgs | AdResult::ErrInternal
+        ));
+        assert!(list.is_null());
+    });
+}
+
 #[test]
 fn null_out_param_rejected_before_write() {
     with_adapter(|adapter| unsafe {