More robust error handling for desktop autotype windows implementation (#16501)

* Desktop autotype windows error handling

* create a subdir

* extract window handle to separate file

* remove println in case tracing doesn't make it in

* touchups

* reduce scope of unsafe call

* use tracing

* Fix comparison on GetLastError result

* Remove the WindowHandle wrapper and save it for the unit testing PR

* restore apps/browser/src/platform/system-notifications/browser-system-notification.service.ts

* use the human readable message for GetLastError debug

* don't call GetLastError outside of error path

* add some more debug statements

* feedback coltonhorst: nits, fix false positive when len zero, re-add handle validation

* lint

* feedback coltonhurst: add comments and update var names

apps/desktop/desktop_native/Cargo.lock

@@ -342,6 +342,7 @@ checksum = "ace50bade8e6234aa140d9a2f552bbee1db4d353f69b8217bc503490fc1a9f26"
 name = "autotype"
 version = "0.0.0"
 dependencies = [
+ "anyhow",
  "tracing",
  "windows 0.61.1",
  "windows-core 0.61.0",
@@ -2897,9 +2898,9 @@ dependencies = [
 
 [[package]]
 name = "security-framework"
-version = "3.4.0"
+version = "3.5.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "60b369d18893388b345804dc0007963c99b7d665ae71d275812d828c6f089640"
+checksum = "cc198e42d9b7510827939c9a15f5062a0c913f3371d765977e586d2fe6c16f4a"
 dependencies = [
  "bitflags",
  "core-foundation",

apps/desktop/desktop_native/autotype/Cargo.toml

@@ -5,6 +5,9 @@ license.workspace = true
 edition.workspace = true
 publish.workspace = true
 
+[dependencies]
+anyhow = { workspace = true }
+
 [target.'cfg(windows)'.dependencies]
 tracing.workspace = true
 windows = { workspace = true, features = [

apps/desktop/desktop_native/autotype/src/lib.rs

@@ -1,3 +1,5 @@
+use anyhow::Result;
+
 #[cfg_attr(target_os = "linux", path = "linux.rs")]
 #[cfg_attr(target_os = "macos", path = "macos.rs")]
 #[cfg_attr(target_os = "windows", path = "windows.rs")]
@@ -5,18 +7,26 @@ mod windowing;
 
 /// Gets the title bar string for the foreground window.
 ///
-/// TODO: The error handling will be improved in a future PR: PM-23615
+/// # Errors
-#[allow(clippy::result_unit_err)]
+///
-pub fn get_foreground_window_title() -> std::result::Result<String, ()> {
+/// This function returns an `anyhow::Error` if there is any
+/// issue obtaining the window title. Detailed reasons will
+/// vary based on platform implementation.
+pub fn get_foreground_window_title() -> Result<String> {
     windowing::get_foreground_window_title()
 }
 
 /// Attempts to type the input text wherever the user's cursor is.
 ///
-/// `input` must be an array of utf-16 encoded characters to insert.
+/// # Arguments
 ///
-/// TODO: The error handling will be improved in a future PR: PM-23615
+/// * `input` must be an array of utf-16 encoded characters to insert.
-#[allow(clippy::result_unit_err)]
+///
-pub fn type_input(input: Vec<u16>, keyboard_shortcut: Vec<String>) -> std::result::Result<(), ()> {
+/// # Errors
+///
+/// This function returns an `anyhow::Error` if there is any
+/// issue obtaining the window title. Detailed reasons will
+/// vary based on platform implementation.
+pub fn type_input(input: Vec<u16>, keyboard_shortcut: Vec<String>) -> Result<()> {
     windowing::type_input(input, keyboard_shortcut)
 }

apps/desktop/desktop_native/autotype/src/linux.rs

@@ -1,10 +1,7 @@
-pub fn get_foreground_window_title() -> std::result::Result<String, ()> {
+pub fn get_foreground_window_title() -> anyhow::Result<String> {
     todo!("Bitwarden does not yet support Linux autotype");
 }
 
-pub fn type_input(
+pub fn type_input(_input: Vec<u16>, _keyboard_shortcut: Vec<String>) -> anyhow::Result<()> {
-    _input: Vec<u16>,
-    _keyboard_shortcut: Vec<String>,
-) -> std::result::Result<(), ()> {
     todo!("Bitwarden does not yet support Linux autotype");
 }

apps/desktop/desktop_native/autotype/src/macos.rs

@@ -1,10 +1,7 @@
-pub fn get_foreground_window_title() -> std::result::Result<String, ()> {
+pub fn get_foreground_window_title() -> anyhow::Result<String> {
     todo!("Bitwarden does not yet support macOS autotype");
 }
 
-pub fn type_input(
+pub fn type_input(_input: Vec<u16>, _keyboard_shortcut: Vec<String>) -> anyhow::Result<()> {
-    _input: Vec<u16>,
-    _keyboard_shortcut: Vec<String>,
-) -> std::result::Result<(), ()> {
     todo!("Bitwarden does not yet support macOS autotype");
 }

apps/desktop/desktop_native/autotype/src/windows.rs

@@ -1,38 +1,141 @@
-use std::ffi::OsString;
+use std::{ffi::OsString, os::windows::ffi::OsStringExt};
-use std::os::windows::ffi::OsStringExt;
 
-use tracing::debug;
+use anyhow::{anyhow, Result};
-use windows::Win32::Foundation::{GetLastError, HWND};
+use tracing::{debug, error, warn};
-use windows::Win32::UI::Input::KeyboardAndMouse::{
+use windows::Win32::{
-    SendInput, INPUT, INPUT_0, INPUT_KEYBOARD, KEYBDINPUT, KEYEVENTF_KEYUP, KEYEVENTF_UNICODE,
+    Foundation::{GetLastError, SetLastError, HWND, WIN32_ERROR},
-    VIRTUAL_KEY,
+    UI::{
-};
+        Input::KeyboardAndMouse::{
-use windows::Win32::UI::WindowsAndMessaging::{
+            SendInput, INPUT, INPUT_0, INPUT_KEYBOARD, KEYBDINPUT, KEYEVENTF_KEYUP,
-    GetForegroundWindow, GetWindowTextLengthW, GetWindowTextW,
+            KEYEVENTF_UNICODE, VIRTUAL_KEY,
+        },
+        WindowsAndMessaging::{GetForegroundWindow, GetWindowTextLengthW, GetWindowTextW},
+    },
 };
 
+const WIN32_SUCCESS: WIN32_ERROR = WIN32_ERROR(0);
+
+fn clear_last_error() {
+    debug!("Clearing last error with SetLastError.");
+    unsafe {
+        SetLastError(WIN32_ERROR(0));
+    }
+}
+
+fn get_last_error() -> WIN32_ERROR {
+    let last_err = unsafe { GetLastError() };
+    debug!("GetLastError(): {}", last_err.to_hresult().message());
+    last_err
+}
+
+// The handle should be validated before any unsafe calls referencing it.
+fn validate_window_handle(handle: &HWND) -> Result<()> {
+    if handle.is_invalid() {
+        error!("Window handle is invalid.");
+        return Err(anyhow!("Window handle is invalid."));
+    }
+    Ok(())
+}
+
+// ---------- Window title --------------
+
 /// Gets the title bar string for the foreground window.
-pub fn get_foreground_window_title() -> std::result::Result<String, ()> {
+pub fn get_foreground_window_title() -> Result<String> {
-    let Ok(window_handle) = get_foreground_window() else {
+    // https://learn.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-getforegroundwindow
-        return Err(());
+    let window_handle = unsafe { GetForegroundWindow() };
-    };
-    let Ok(Some(window_title)) = get_window_title(window_handle) else {
-        return Err(());
-    };
 
-    Ok(window_title)
+    debug!("GetForegroundWindow() called.");
+
+    validate_window_handle(&window_handle)?;
+
+    get_window_title(&window_handle)
 }
 
+/// Gets the length of the window title bar text.
+///
+/// https://learn.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-getwindowtextlengthw
+fn get_window_title_length(window_handle: &HWND) -> Result<usize> {
+    // GetWindowTextLengthW does not itself clear the last error so we must do it ourselves.
+    clear_last_error();
+
+    validate_window_handle(window_handle)?;
+
+    let length = unsafe { GetWindowTextLengthW(*window_handle) };
+
+    let length = usize::try_from(length)?;
+
+    debug!(length, "window text length retrieved from handle.");
+
+    if length == 0 {
+        // attempt to retreive win32 error
+        let last_err = get_last_error();
+        if last_err != WIN32_SUCCESS {
+            let last_err = last_err.to_hresult().message();
+            error!(last_err, "Error getting window text length.");
+            return Err(anyhow!("Error getting window text length: {last_err}"));
+        }
+    }
+
+    Ok(length)
+}
+
+/// Gets the window title bar title.
+///
+/// https://learn.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-getwindowtextw
+fn get_window_title(window_handle: &HWND) -> Result<String> {
+    let expected_window_title_length = get_window_title_length(window_handle)?;
+
+    // This isn't considered an error by the windows API, but in practice it means we can't
+    // match against the title so we'll stop here.
+    // The upstream will make a contains comparison on what we return, so an empty string
+    // will not result on a match.
+    if expected_window_title_length == 0 {
+        warn!("Window title length is zero.");
+        return Ok(String::from(""));
+    }
+
+    let mut buffer: Vec<u16> = vec![0; expected_window_title_length + 1]; // add extra space for the null character
+
+    validate_window_handle(window_handle)?;
+
+    let actual_window_title_length = unsafe { GetWindowTextW(*window_handle, &mut buffer) };
+
+    debug!(actual_window_title_length, "window title retrieved.");
+
+    if actual_window_title_length == 0 {
+        // attempt to retreive win32 error
+        let last_err = get_last_error();
+        if last_err != WIN32_SUCCESS {
+            let last_err = last_err.to_hresult().message();
+            error!(last_err, "Error retrieving window title.");
+            return Err(anyhow!("Error retrieving window title. {last_err}"));
+        }
+        // in practice, we should not get to the below code, since we asserted the len > 0
+        // above. but it is an extra protection in case the windows API didn't set an error.
+        warn!(expected_window_title_length, "No window title retrieved.");
+    }
+
+    let window_title = OsString::from_wide(&buffer);
+
+    Ok(window_title.to_string_lossy().into_owned())
+}
+
+// ---------- Type Input --------------
+
 /// Attempts to type the input text wherever the user's cursor is.
 ///
 /// `input` must be a vector of utf-16 encoded characters to insert.
 /// `keyboard_shortcut` must be a vector of Strings, where valid shortcut keys: Control, Alt, Super, Shift, letters a - Z
 ///
 /// https://learn.microsoft.com/en-in/windows/win32/api/winuser/nf-winuser-sendinput
-pub fn type_input(input: Vec<u16>, keyboard_shortcut: Vec<String>) -> Result<(), ()> {
+pub fn type_input(input: Vec<u16>, keyboard_shortcut: Vec<String>) -> Result<()> {
     const TAB_KEY: u8 = 9;
 
-    let mut keyboard_inputs: Vec<INPUT> = Vec::new();
+    // the length of this vec is always shortcut keys to release + (2x length of input chars)
+    let mut keyboard_inputs: Vec<INPUT> =
+        Vec::with_capacity(keyboard_shortcut.len() + (input.len() * 2));
+
+    debug!(?keyboard_shortcut, "Converting keyboard shortcut to input.");
 
     // Add key "up" inputs for the shortcut
     for key in keyboard_shortcut {
@@ -63,7 +166,7 @@ pub fn type_input(input: Vec<u16>, keyboard_shortcut: Vec<String>) -> Result<(),
 /// Converts a valid shortcut key to an "up" keyboard input.
 ///
 /// `input` must be a valid shortcut key: Control, Alt, Super, Shift, letters [a-z][A-Z]
-fn convert_shortcut_key_to_up_input(key: String) -> Result<INPUT, ()> {
+fn convert_shortcut_key_to_up_input(key: String) -> Result<INPUT> {
     const SHIFT_KEY: u8 = 0x10;
     const SHIFT_KEY_STR: &str = "Shift";
     const CONTROL_KEY: u8 = 0x11;
@@ -89,9 +192,15 @@ fn convert_shortcut_key_to_up_input(key: String) -> Result<INPUT, ()> {
 /// Because we only accept [a-z][A-Z], the decimal u16
 /// cast of the letter is safe because the unicode code point
 /// of these characters fits in a u16.
-fn get_alphabetic_hotkey(letter: String) -> Result<u16, ()> {
+fn get_alphabetic_hotkey(letter: String) -> Result<u16> {
     if letter.len() != 1 {
-        return Err(());
+        error!(
+            len = letter.len(),
+            "Final keyboard shortcut key should be a single character."
+        );
+        return Err(anyhow!(
+            "Final keyboard shortcut key should be a single character: {letter}"
+        ));
     }
 
     let c = letter.chars().next().expect("letter is size 1");
@@ -99,65 +208,20 @@ fn get_alphabetic_hotkey(letter: String) -> Result<u16, ()> {
     // is_ascii_alphabetic() checks for:
     // U+0041 `A` ..= U+005A `Z`, or  U+0061 `a` ..= U+007A `z`
     if !c.is_ascii_alphabetic() {
-        return Err(());
+        error!(letter = %c, "Letter is not ASCII Alphabetic ([a-z][A-Z]).");
+        return Err(anyhow!(
+            "Letter is not ASCII Alphabetic ([a-z][A-Z]): '{letter}'",
+        ));
     }
 
-    Ok(c as u16)
+    let c = c as u16;
+
+    debug!(c, letter, "Got alphabetic hotkey.");
+
+    Ok(c)
 }
 
-/// Gets the foreground window handle.
+/// An input key can be either pressed (down), or released (up).
-///
-/// https://learn.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-getforegroundwindow
-fn get_foreground_window() -> Result<HWND, ()> {
-    let foreground_window_handle = unsafe { GetForegroundWindow() };
-
-    if foreground_window_handle.is_invalid() {
-        return Err(());
-    }
-
-    Ok(foreground_window_handle)
-}
-
-/// Gets the length of the window title bar text.
-///
-/// https://learn.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-getwindowtextlengthw
-fn get_window_title_length(window_handle: HWND) -> Result<usize, ()> {
-    if window_handle.is_invalid() {
-        return Err(());
-    }
-
-    match usize::try_from(unsafe { GetWindowTextLengthW(window_handle) }) {
-        Ok(length) => Ok(length),
-        Err(_) => Err(()),
-    }
-}
-
-/// Gets the window title bar title.
-///
-/// https://learn.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-getwindowtextw
-fn get_window_title(window_handle: HWND) -> Result<Option<String>, ()> {
-    if window_handle.is_invalid() {
-        return Err(());
-    }
-
-    let window_title_length = get_window_title_length(window_handle)?;
-    if window_title_length == 0 {
-        return Ok(None);
-    }
-
-    let mut buffer: Vec<u16> = vec![0; window_title_length + 1]; // add extra space for the null character
-
-    let window_title_length = unsafe { GetWindowTextW(window_handle, &mut buffer) };
-    if window_title_length == 0 {
-        return Ok(None);
-    }
-
-    let window_title = OsString::from_wide(&buffer);
-
-    Ok(Some(window_title.to_string_lossy().into_owned()))
-}
-
-/// Used in build_input() to specify if an input key is being pressed (down) or released (up).
 enum InputKeyPress {
     Down,
     Up,
@@ -233,18 +297,29 @@ fn build_virtual_key_input(key_press: InputKeyPress, virtual_key: u8) -> INPUT {
 /// Attempts to type the provided input wherever the user's cursor is.
 ///
 /// https://learn.microsoft.com/en-in/windows/win32/api/winuser/nf-winuser-sendinput
-fn send_input(inputs: Vec<INPUT>) -> Result<(), ()> {
+fn send_input(inputs: Vec<INPUT>) -> Result<()> {
     let insert_count = unsafe { SendInput(&inputs, std::mem::size_of::<INPUT>() as i32) };
 
-    let e = unsafe { GetLastError().to_hresult().message() };
+    debug!("SendInput() called.");
-    debug!("type_input() called, GetLastError() is: {:?}", e);
 
     if insert_count == 0 {
-        return Err(()); // input was blocked by another thread
+        let last_err = get_last_error().to_hresult().message();
+        error!(GetLastError = %last_err, "SendInput sent 0 inputs. Input was blocked by another thread.");
+
+        return Err(anyhow!("SendInput sent 0 inputs. Input was blocked by another thread. GetLastError: {last_err}"));
     } else if insert_count != inputs.len() as u32 {
-        return Err(()); // input insertion not completed
+        let last_err = get_last_error().to_hresult().message();
+        error!(sent = %insert_count, expected = inputs.len(), GetLastError = %last_err,
+            "SendInput sent does not match expected."
+        );
+        return Err(anyhow!(
+            "SendInput does not match expected. sent: {insert_count}, expected: {}",
+            inputs.len()
+        ));
     }
 
+    debug!(insert_count, "Autotype sent input.");
+
     Ok(())
 }
 
@@ -263,16 +338,16 @@ mod tests {
     }
 
     #[test]
-    #[should_panic = ""]
+    #[should_panic = "Final keyboard shortcut key should be a single character: foo"]
     fn get_alphabetic_hot_key_fail_not_single_char() {
         let letter = String::from("foo");
         get_alphabetic_hotkey(letter).unwrap();
     }
 
     #[test]
-    #[should_panic = ""]
+    #[should_panic = "Letter is not ASCII Alphabetic ([a-z][A-Z]): '}'"]
     fn get_alphabetic_hot_key_fail_not_alphabetic() {
-        let letter = String::from("🚀");
+        let letter = String::from("}");
         get_alphabetic_hotkey(letter).unwrap();
     }
 }