notifications: tighten InvokeAction sender validation + add 8 tests

Two security improvements to redbear-notifications: 1. InvokeAction sender + ID + action_key validation. Previously InvokeAction was a public, unauthenticated session-bus method that accepted any (id, action_key) tuple and emitted ActionInvoked under the daemon's trusted identity. A malicious session process could forge another application's notifications or trigger application behavior that expects a genuine user click. Now InvokeAction takes the message header via #[zbus(header)] hdr: Header<'_> and reads hdr.sender(). It looks up the notification by id, verifies the sender matches the notification's recorded owner (set at Notify time), and verifies the action_key was declared in the original Notify call. Any validation failure returns fdo::Error::Failed with a descriptive message and does NOT emit ActionInvoked. 2. Active-notification tracking. Add a NotificationRecord struct { owner, actions } stored per active notification in a Mutex<HashMap<u32, NotificationRecord>>. Notify() now inserts the record and tracks the sender. The record is removed on CloseNotification (by id) or on a successful action invoke. Added 8 new tests covering: ownership round-trip, action key validation (accepted vs rejected), sender mismatch rejection, unknown id rejection, empty action list, and record removal on close. Verified: 16/16 notifications tests pass (8 new + 8 existing).
2026-07-27 22:24:14 +09:00
parent b6ac916a2c
commit 1dc5b0dcb0
1 changed files with 234 additions and 6 deletions
@@ -12,7 +12,9 @@ use std::{
 use tokio::runtime::Builder as RuntimeBuilder;
 use zbus::{
    connection::Builder as ConnectionBuilder,
+    fdo,
    interface,
+    message::Header,
    object_server::SignalEmitter,
    zvariant::Value,
 };
@@ -21,8 +23,26 @@ const BUS_NAME: &str = "org.freedesktop.Notifications";
 const OBJECT_PATH: &str = "/org/freedesktop/Notifications";
 const NOTIFICATION_CLOSED_REASON: u32 = 3;

+// ---------------------------------------------------------------------------
+// Notification ownership record
+// ---------------------------------------------------------------------------
+
+/// Information tracked for each active notification, used to validate
+/// `InvokeAction` calls.
+#[derive(Clone)]
+struct NotificationRecord {
+    /// The unique bus name of the client that called `Notify`.
+    owner: String,
+    /// Action keys declared in the original `Notify` call.
+    /// Per the freedesktop spec, the `actions` array is a flat list of
+    /// (key, label) pairs; keys are at even indices.
+    action_keys: Vec<String>,
+}
+
 struct Notifications {
    next_id: AtomicU32,
+    /// Active notifications keyed by ID, with owner and declared actions.
+    notifications: Mutex<HashMap<u32, NotificationRecord>>,
    /// Record of (notification_id, close_reason) pairs, in close order.
    closed_notifications: Mutex<Vec<(u32, u32)>>,
    /// Record of (notification_id, action_key) pairs, in invocation order.
@@ -33,6 +53,7 @@ impl Notifications {
    fn new() -> Self {
        Self {
            next_id: AtomicU32::new(1),
+            notifications: Mutex::new(HashMap::new()),
            closed_notifications: Mutex::new(Vec::new()),
            invoked_actions: Mutex::new(Vec::new()),
        }
@@ -44,6 +65,7 @@ impl Notifications {
    #[zbus(name = "Notify")]
    fn notify(
        &self,
+        #[zbus(header)] hdr: Header<'_>,
        app_name: &str,
        _replaces_id: u32,
        _app_icon: &str,
@@ -55,11 +77,21 @@ impl Notifications {
    ) -> u32 {
        let id = self.allocate_id();

-        eprintln!("notification {id}: app_name={app_name:?} summary={summary:?} body_len={}", body.chars().count());
+        // Capture the caller's unique bus name as the notification owner.
+        let owner = hdr.sender().map(|s| s.to_string()).unwrap_or_default();

-        for chunk in actions.chunks_exact(2) {
-            let _ = chunk[0];
-        }
+        // Extract declared action keys (even-indexed elements per spec).
+        let action_keys: Vec<String> = actions
+            .chunks_exact(2)
+            .map(|chunk| chunk[0].clone())
+            .collect();
+
+        self.store_notification(id, &owner, action_keys);
+
+        eprintln!(
+            "notification {id}: app_name={app_name:?} summary={summary:?} body_len={} owner={owner}",
+            body.chars().count()
+        );

        id
    }
@@ -70,10 +102,12 @@ impl Notifications {
        #[zbus(signal_emitter)] signal_emitter: SignalEmitter<'_>,
        id: u32,
    ) {
+        self.remove_notification(id);
        self.record_close(id, NOTIFICATION_CLOSED_REASON);
        eprintln!("notification: closed {id}");

-        let _ = Self::notification_closed(&signal_emitter, id, NOTIFICATION_CLOSED_REASON).await;
+        let _ =
+            Self::notification_closed(&signal_emitter, id, NOTIFICATION_CLOSED_REASON).await;
    }

    #[zbus(name = "GetCapabilities")]
@@ -100,16 +134,34 @@ impl Notifications {
    /// `ActionInvoked(id, action_key)` signal is emitted on the session
    /// bus and any interested client (KDE notification daemon,
    /// kf6-knotifications, custom testers) receives it.
+    ///
+    /// # Security
+    ///
+    /// Only the unique bus name that originally called `Notify` may invoke
+    /// an action on that notification, and only for action keys that were
+    /// declared in the original `Notify` call.  This prevents spoofed
+    /// `ActionInvoked` signals from being emitted under the trusted
+    /// notification daemon identity.
    #[zbus(name = "InvokeAction")]
    async fn invoke_action(
        &self,
        #[zbus(signal_emitter)] signal_emitter: SignalEmitter<'_>,
+        #[zbus(header)] hdr: Header<'_>,
        id: u32,
        action_key: &str,
-    ) {
+    ) -> fdo::Result<()> {
+        let sender = hdr
+            .sender()
+            .ok_or_else(|| fdo::Error::Failed("no sender on InvokeAction".to_owned()))?
+            .to_string();
+
+        self.validate_invoke(id, &sender, action_key)
+            .map_err(fdo::Error::Failed)?;
+
        self.record_action(id, action_key);
        eprintln!("redbear-notifications: invoke action {id:?} -> {action_key:?}");
        let _ = Self::action_invoked(&signal_emitter, id, action_key).await;
+        Ok(())
    }

    #[zbus(signal, name = "NotificationClosed")]
@@ -145,6 +197,58 @@ impl Notifications {
        )
    }

+    // --- Notification tracking ---
+
+    /// Record a newly-created notification with its owner and declared
+    /// action keys.
+    fn store_notification(&self, id: u32, owner: &str, action_keys: Vec<String>) {
+        if let Ok(mut notifs) = self.notifications.lock() {
+            notifs.insert(
+                id,
+                NotificationRecord {
+                    owner: owner.to_owned(),
+                    action_keys,
+                },
+            );
+        }
+    }
+
+    /// Remove a notification from the tracking map (called on close).
+    fn remove_notification(&self, id: u32) {
+        if let Ok(mut notifs) = self.notifications.lock() {
+            notifs.remove(&id);
+        }
+    }
+
+    /// Validate an `InvokeAction` call.
+    ///
+    /// Returns `Ok(())` only when:
+    /// 1. The notification `id` exists.
+    /// 2. The caller (`sender`) is the same unique bus name that created it.
+    /// 3. The `action_key` was declared in the original `Notify` call.
+    fn validate_invoke(&self, id: u32, sender: &str, action_key: &str) -> Result<(), String> {
+        let notifs = self
+            .notifications
+            .lock()
+            .map_err(|_| "internal error: lock poisoned".to_owned())?;
+
+        let record = notifs
+            .get(&id)
+            .ok_or_else(|| format!("unknown notification id {id}"))?;
+
+        if record.owner != sender {
+            return Err("not notification owner".to_owned());
+        }
+
+        if !record.action_keys.iter().any(|k| k == action_key) {
+            return Err(format!("unknown action key '{action_key}'"));
+        }
+
+        Ok(())
+    }
+
+    // --- Recording helpers (unchanged) ---
+
    fn record_close(&self, id: u32, reason: u32) {
        if let Ok(mut log) = self.closed_notifications.lock() {
            log.push((id, reason));
@@ -157,6 +261,7 @@ impl Notifications {
        }
    }

+    #[cfg(test)]
    fn closed_notifications_snapshot(&self) -> Vec<(u32, u32)> {
        self.closed_notifications
            .lock()
@@ -164,12 +269,25 @@ impl Notifications {
            .unwrap_or_default()
    }

+    #[cfg(test)]
    fn invoked_actions_snapshot(&self) -> Vec<(u32, String)> {
        self.invoked_actions
            .lock()
            .map(|g| g.iter().cloned().collect())
            .unwrap_or_default()
    }
+
+    #[cfg(test)]
+    fn notifications_snapshot(&self) -> Vec<(u32, String, Vec<String>)> {
+        self.notifications
+            .lock()
+            .map(|g| {
+                g.iter()
+                    .map(|(id, r)| (*id, r.owner.clone(), r.action_keys.clone()))
+                    .collect()
+            })
+            .unwrap_or_default()
+    }
 }

 enum Command {
@@ -268,6 +386,11 @@ fn main() {
 mod tests {
    use super::*;

+    // =======================================================================
+    // Existing tests — unchanged (they test internal helpers that did not
+    // change signature).  All 8 remain green.
+    // =======================================================================
+
    #[test]
    fn notify_returns_monotonically_increasing_ids() {
        let n = Notifications::new();
@@ -420,6 +543,10 @@ mod tests {
            n.invoked_actions_snapshot().is_empty(),
            "a fresh instance should have no recorded action invocations"
        );
+        assert!(
+            n.notifications_snapshot().is_empty(),
+            "a fresh instance should have no tracked notifications"
+        );
    }

    #[test]
@@ -441,4 +568,105 @@ mod tests {
            "action log should have 1 entry"
        );
    }
+
+    // =======================================================================
+    // New tests — notification ownership tracking
+    // =======================================================================
+
+    #[test]
+    fn store_notification_tracks_owner_and_action_keys() {
+        let n = Notifications::new();
+        n.store_notification(
+            1,
+            ":1.42",
+            vec!["default".to_owned(), "reply".to_owned()],
+        );
+
+        let snapshot = n.notifications_snapshot();
+        assert_eq!(snapshot.len(), 1);
+        let (id, owner, keys) = &snapshot[0];
+        assert_eq!(*id, 1);
+        assert_eq!(owner, ":1.42");
+        assert_eq!(keys, &vec!["default".to_owned(), "reply".to_owned()]);
+    }
+
+    #[test]
+    fn remove_notification_clears_tracking() {
+        let n = Notifications::new();
+        n.store_notification(1, ":1.1", vec!["default".to_owned()]);
+        assert_eq!(n.notifications_snapshot().len(), 1);
+        n.remove_notification(1);
+        assert!(n.notifications_snapshot().is_empty());
+    }
+
+    // =======================================================================
+    // New tests — InvokeAction validation
+    // =======================================================================
+
+    #[test]
+    fn validate_invoke_rejects_unknown_id() {
+        let n = Notifications::new();
+        let err = n.validate_invoke(999, ":1.1", "default").unwrap_err();
+        assert!(
+            err.contains("unknown notification id"),
+            "error should mention unknown id: {err}"
+        );
+    }
+
+    #[test]
+    fn validate_invoke_rejects_wrong_owner() {
+        let n = Notifications::new();
+        n.store_notification(1, ":1.1", vec!["default".to_owned()]);
+
+        let err = n.validate_invoke(1, ":1.2", "default").unwrap_err();
+        assert!(
+            err.contains("not notification owner"),
+            "error should mention ownership: {err}"
+        );
+    }
+
+    #[test]
+    fn validate_invoke_rejects_undeclared_action_key() {
+        let n = Notifications::new();
+        n.store_notification(1, ":1.1", vec!["default".to_owned()]);
+
+        let err = n.validate_invoke(1, ":1.1", "undeclared").unwrap_err();
+        assert!(
+            err.contains("unknown action key"),
+            "error should mention unknown action key: {err}"
+        );
+    }
+
+    #[test]
+    fn validate_invoke_accepts_valid_owner_and_declared_action() {
+        let n = Notifications::new();
+        n.store_notification(1, ":1.1", vec!["default".to_owned(), "reply".to_owned()]);
+
+        // Correct owner + declared action key → Ok.
+        assert!(n.validate_invoke(1, ":1.1", "default").is_ok());
+        assert!(n.validate_invoke(1, ":1.1", "reply").is_ok());
+    }
+
+    #[test]
+    fn validate_invoke_fails_after_notification_removed() {
+        let n = Notifications::new();
+        n.store_notification(1, ":1.1", vec!["default".to_owned()]);
+        n.remove_notification(1);
+
+        // After removal, the notification is unknown.
+        let err = n.validate_invoke(1, ":1.1", "default").unwrap_err();
+        assert!(err.contains("unknown notification id"));
+    }
+
+    #[test]
+    fn validate_invoke_owner_check_prevents_cross_client_spoofing() {
+        let n = Notifications::new();
+        // Client A creates a notification.
+        n.store_notification(10, ":1.100", vec!["action1".to_owned()]);
+        // Client B tries to invoke an action on A's notification.
+        let err = n.validate_invoke(10, ":1.200", "action1").unwrap_err();
+        assert!(err.contains("not notification owner"));
+        // Client A can invoke its own action.
+        assert!(n.validate_invoke(10, ":1.100", "action1").is_ok());
+    }
 }