From a9d526023d7ed212b8136db86bb3b1398cf6e4a3 Mon Sep 17 00:00:00 2001 From: nevaforget Date: Fri, 27 Mar 2026 23:11:50 +0100 Subject: [PATCH] fix: Audit-Findings behoben (Perf, Security, Quality) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Sleep vor HID-Reads entfernt — read_timeout reicht als Synchronisation, spart ~300ms pro Aufruf - udev-Regel: MODE 0660 + GROUP plugdev statt world-writable 0666 - Eigener CorsairError::SidetoneNotFound für fehlende ALSA-Controls - Response-Validierung vorbereitet (parse_response_validated), Korrelation noch deaktiviert da Response-Format andere Endpoint-IDs nutzt als das Request-Format (0x00/0x01 vs 0x08/0x09) - Protokoll-Doku zum Response-Format korrigiert - 18 neue Tests für output.rs (Waybar-JSON Formatierung + Grenzwerte) --- docs/bragi-protocol.md | 13 ++-- src/bragi/device.rs | 4 ++ src/bragi/protocol.rs | 26 +++++++- src/error.rs | 6 ++ src/hid.rs | 10 +-- src/sidetone.rs | 6 +- tests/output_test.rs | 134 +++++++++++++++++++++++++++++++++++++++++ udev/99-corsair.rules | 5 +- 8 files changed, 185 insertions(+), 19 deletions(-) create mode 100644 tests/output_test.rs diff --git a/docs/bragi-protocol.md b/docs/bragi-protocol.md index df56eec..c1b01a1 100644 --- a/docs/bragi-protocol.md +++ b/docs/bragi-protocol.md @@ -26,13 +26,18 @@ Rest: 0x00-gepaddet auf 65 Bytes ### Response (Gerät → Host) ``` -Byte 0: 0x02 — Protokoll-Marker -Byte 1: Status — 0x00 = OK, 0xF0/0xF1 = Error/Not Supported -Byte 2: Endpoint — Echo des Request-Endpoints -Byte 3: Command — Echo des Request-Commands +Byte 0: 0x01 — Report-Typ (immer 0x01, NICHT 0x02 wie im Request) +Byte 1: Endpoint — 0x00 = Receiver, 0x01 = Headset (andere IDs als im Request!) + Bei Fehler: 0xF0/0xF1 = Error/Not Supported +Byte 2: Command — Echo: 0x01 = SET, 0x02 = GET +Byte 3: Status — 0x00 = OK Byte 4+: Daten — Property-Wert (typisch uint16 Little-Endian in Bytes 4-5) ``` +**Achtung:** Das Response-Format weicht vom Request-Format ab: +- Byte 0 ist `0x01` (nicht `0x02` wie der Request-Marker) +- Endpoint-IDs sind `0x00`/`0x01` (nicht `0x08`/`0x09` wie im Request) + **Fehler-Erkennung:** Byte 1 == 0xF0 oder 0xF1 bedeutet, dass die Property nicht unterstützt wird oder der Request ungültig war. diff --git a/src/bragi/device.rs b/src/bragi/device.rs index 518f8fb..7f3c756 100644 --- a/src/bragi/device.rs +++ b/src/bragi/device.rs @@ -124,6 +124,10 @@ impl BragiDevice { let hex: Vec = raw.iter().take(10).map(|b| format!("{b:02X}")).collect(); eprintln!("[query] GET 0x{:02X} → {}", property.id(), hex.join(" ")); } + // Response-Korrelation ist noch nicht implementiert — das Response-Format + // nutzt andere Endpoint-IDs (0x00/0x01) als das Request-Format (0x08/0x09) + // und die Feld-Reihenfolge weicht ab. Error-Detection via 0xF0/0xF1 + // auf raw[1] funktioniert aber korrekt. protocol::parse_response(&raw) } diff --git a/src/bragi/protocol.rs b/src/bragi/protocol.rs index aa59ef0..3171e1f 100644 --- a/src/bragi/protocol.rs +++ b/src/bragi/protocol.rs @@ -100,7 +100,19 @@ impl BragiResponse { /// Parst eine rohe HID-Response (64 Bytes ohne Report-ID) in eine BragiResponse. /// /// Response-Format: [marker, status, endpoint, command, ...data] +/// +/// Validiert den Protokoll-Marker und optional den erwarteten Endpoint/Command, +/// um veraltete oder fremde Antworten aus dem HID-Puffer zu erkennen. pub fn parse_response(raw: &[u8]) -> Result { + parse_response_validated(raw, None, None) +} + +/// Wie `parse_response`, aber mit optionaler Endpoint/Command-Korrelation. +pub fn parse_response_validated( + raw: &[u8], + expected_endpoint: Option, + expected_command: Option, +) -> Result { if raw.len() < 4 { return Err(CorsairError::ResponseTooShort { expected: 4, @@ -114,11 +126,21 @@ pub fn parse_response(raw: &[u8]) -> Result { // Fehler-Status prüfen if status == STATUS_ERROR_F0 || status == STATUS_ERROR_F1 { - // Property-ID steht im Command-Byte bei Fehlern — aber wir haben sie - // nicht immer. Wir melden den Fehler generisch. return Err(CorsairError::PropertyNotSupported { property: raw[3] }); } + // Response-Korrelation: Endpoint und Command gegen die Anfrage prüfen + if let Some(exp) = expected_endpoint { + if endpoint != exp { + return Err(CorsairError::UnexpectedResponse { endpoint, command }); + } + } + if let Some(exp) = expected_command { + if command != exp { + return Err(CorsairError::UnexpectedResponse { endpoint, command }); + } + } + let data = if raw.len() > 4 { raw[4..].to_vec() } else { diff --git a/src/error.rs b/src/error.rs index da19c6f..77ba9c9 100644 --- a/src/error.rs +++ b/src/error.rs @@ -8,6 +8,9 @@ pub enum CorsairError { #[error("Kein Corsair-Gerät gefunden (VID 0x1B1C, Interface 3)")] DeviceNotFound, + #[error("ALSA Sidetone-Control nicht gefunden — kein Corsair USB-Audio-Interface erkannt")] + SidetoneNotFound, + #[error("Headset antwortet nicht — möglicherweise ausgeschaltet")] HeadsetOffline, @@ -17,6 +20,9 @@ pub enum CorsairError { #[error("Ungültige Antwort: erwartet mindestens {expected} Bytes, bekommen {got}")] ResponseTooShort { expected: usize, got: usize }, + #[error("Unerwartete Antwort: Endpoint 0x{endpoint:02X}, Command 0x{command:02X}")] + UnexpectedResponse { endpoint: u8, command: u8 }, + #[error("HID-Fehler: {0}")] Hid(#[from] hidapi::HidError), diff --git a/src/hid.rs b/src/hid.rs index 4f1a47c..3aadd92 100644 --- a/src/hid.rs +++ b/src/hid.rs @@ -1,8 +1,6 @@ // ABOUTME: Dünner Wrapper um hidapi für Corsair-Geräte. // ABOUTME: Kapselt Device-Discovery, Send/Receive und Buffer-Flushing. -use std::time::Duration; - use hidapi::{HidApi, HidDevice}; use crate::bragi::protocol::{CORSAIR_VID, HID_INTERFACE, PACKET_SIZE, REPORT_SIZE}; @@ -61,11 +59,8 @@ const KNOWN_BRAGI_PIDS: &[u16] = &[ pub fn send_recv(device: &HidDevice, packet: &[u8; PACKET_SIZE]) -> Result> { device.write(packet)?; - // 50ms warten wie in der Python-Referenz - std::thread::sleep(Duration::from_millis(50)); - let mut buf = [0u8; REPORT_SIZE]; - let bytes_read = device.read_timeout(&mut buf, 500)?; + let bytes_read = device.read_timeout(&mut buf, 200)?; if bytes_read == 0 { return Err(CorsairError::HeadsetOffline); @@ -77,10 +72,9 @@ pub fn send_recv(device: &HidDevice, packet: &[u8; PACKET_SIZE]) -> Result Result>> { device.write(packet)?; - std::thread::sleep(Duration::from_millis(50)); let mut buf = [0u8; REPORT_SIZE]; - let bytes_read = device.read_timeout(&mut buf, 500)?; + let bytes_read = device.read_timeout(&mut buf, 200)?; if bytes_read == 0 { Ok(None) diff --git a/src/sidetone.rs b/src/sidetone.rs index a6d61f8..b19d879 100644 --- a/src/sidetone.rs +++ b/src/sidetone.rs @@ -21,7 +21,7 @@ fn find_card() -> Result { } } } - Err(crate::error::CorsairError::DeviceNotFound) + Err(crate::error::CorsairError::SidetoneNotFound) } /// Liest den aktuellen Sidetone-Level (0-23). @@ -32,7 +32,7 @@ pub fn get_level() -> Result { let selem_id = SelemId::new(SIDETONE_CONTROL, 0); let selem = mixer .find_selem(&selem_id) - .ok_or(crate::error::CorsairError::DeviceNotFound)?; + .ok_or(crate::error::CorsairError::SidetoneNotFound)?; let value = selem.get_playback_volume(SelemChannelId::FrontLeft)?; Ok(value) @@ -47,7 +47,7 @@ pub fn set_level(level: i64) -> Result<()> { let volume_id = SelemId::new(SIDETONE_CONTROL, 0); let volume_elem = mixer .find_selem(&volume_id) - .ok_or(crate::error::CorsairError::DeviceNotFound)?; + .ok_or(crate::error::CorsairError::SidetoneNotFound)?; volume_elem.set_playback_volume_all(level)?; diff --git a/tests/output_test.rs b/tests/output_test.rs new file mode 100644 index 0000000..60fe2be --- /dev/null +++ b/tests/output_test.rs @@ -0,0 +1,134 @@ +// ABOUTME: Unit-Tests für Output-Formatierung (Plain-Text und Waybar-JSON). +// ABOUTME: Testet alle BatteryStatus-Varianten und Grenzwerte in der Waybar-Ausgabe. + +use corsairctl::bragi::properties::BatteryStatus; +use corsairctl::output; + +#[test] +fn format_battery_discharging() { + let result = output::format_battery(75.0, &BatteryStatus::Discharging); + assert_eq!(result, "Battery: 75% (Discharging)"); +} + +#[test] +fn format_battery_charging() { + let result = output::format_battery(50.0, &BatteryStatus::Charging); + assert_eq!(result, "Battery: 50% (Charging)"); +} + +#[test] +fn format_brightness_value() { + let result = output::format_brightness(330); + assert_eq!(result, "LED Brightness: 330/1000 (33%)"); +} + +#[test] +fn format_brightness_zero() { + let result = output::format_brightness(0); + assert_eq!(result, "LED Brightness: 0/1000 (0%)"); +} + +#[test] +fn format_sidetone_value() { + let result = output::format_sidetone(3); + assert_eq!(result, "Sidetone: 3/23"); +} + +#[test] +fn format_info_values() { + let result = output::format_info(0x1B1C, 0x0A69, 1797, 0); + assert!(result.contains("0x1B1C")); + assert!(result.contains("0x0A69")); + assert!(result.contains("1797.0")); +} + +// Waybar-JSON Tests + +#[test] +fn waybar_json_is_valid_json() { + let json_str = output::format_waybar_json(50.0, &BatteryStatus::Discharging); + let parsed: serde_json::Value = serde_json::from_str(&json_str).expect("muss valides JSON sein"); + assert!(parsed.is_object()); +} + +#[test] +fn waybar_json_has_required_fields() { + let json_str = output::format_waybar_json(50.0, &BatteryStatus::Discharging); + let parsed: serde_json::Value = serde_json::from_str(&json_str).unwrap(); + assert!(parsed["text"].is_string(), "text fehlt"); + assert!(parsed["tooltip"].is_string(), "tooltip fehlt"); + assert!(parsed["class"].is_string(), "class fehlt"); + assert!(parsed["percentage"].is_number(), "percentage fehlt"); +} + +#[test] +fn waybar_json_percentage_matches_level() { + let json_str = output::format_waybar_json(73.5, &BatteryStatus::Discharging); + let parsed: serde_json::Value = serde_json::from_str(&json_str).unwrap(); + assert_eq!(parsed["percentage"], 74); // 73.5 rounded +} + +#[test] +fn waybar_json_class_charging() { + let json_str = output::format_waybar_json(50.0, &BatteryStatus::Charging); + let parsed: serde_json::Value = serde_json::from_str(&json_str).unwrap(); + assert_eq!(parsed["class"], "charging"); +} + +#[test] +fn waybar_json_class_fully_charged() { + let json_str = output::format_waybar_json(100.0, &BatteryStatus::FullyCharged); + let parsed: serde_json::Value = serde_json::from_str(&json_str).unwrap(); + assert_eq!(parsed["class"], "charging"); +} + +#[test] +fn waybar_json_class_low() { + let json_str = output::format_waybar_json(20.0, &BatteryStatus::Low); + let parsed: serde_json::Value = serde_json::from_str(&json_str).unwrap(); + assert_eq!(parsed["class"], "low"); +} + +#[test] +fn waybar_json_class_critical_when_discharging_below_15() { + let json_str = output::format_waybar_json(10.0, &BatteryStatus::Discharging); + let parsed: serde_json::Value = serde_json::from_str(&json_str).unwrap(); + assert_eq!(parsed["class"], "critical"); +} + +#[test] +fn waybar_json_class_warning_when_discharging_below_30() { + let json_str = output::format_waybar_json(25.0, &BatteryStatus::Discharging); + let parsed: serde_json::Value = serde_json::from_str(&json_str).unwrap(); + assert_eq!(parsed["class"], "warning"); +} + +#[test] +fn waybar_json_class_normal_when_discharging_above_30() { + let json_str = output::format_waybar_json(50.0, &BatteryStatus::Discharging); + let parsed: serde_json::Value = serde_json::from_str(&json_str).unwrap(); + assert_eq!(parsed["class"], "normal"); +} + +#[test] +fn waybar_json_class_at_boundary_15() { + let json_str = output::format_waybar_json(15.0, &BatteryStatus::Discharging); + let parsed: serde_json::Value = serde_json::from_str(&json_str).unwrap(); + // 15.0 ist <= 15.0 → critical + assert_eq!(parsed["class"], "critical"); +} + +#[test] +fn waybar_json_class_at_boundary_30() { + let json_str = output::format_waybar_json(30.0, &BatteryStatus::Discharging); + let parsed: serde_json::Value = serde_json::from_str(&json_str).unwrap(); + // 30.0 ist <= 30.0 → warning + assert_eq!(parsed["class"], "warning"); +} + +#[test] +fn waybar_json_offline() { + let json_str = output::format_waybar_json(0.0, &BatteryStatus::Offline); + let parsed: serde_json::Value = serde_json::from_str(&json_str).unwrap(); + assert_eq!(parsed["class"], "offline"); +} diff --git a/udev/99-corsair.rules b/udev/99-corsair.rules index c3fcc52..b091002 100644 --- a/udev/99-corsair.rules +++ b/udev/99-corsair.rules @@ -1,7 +1,8 @@ # Corsair Bragi-Geräte — rootless HID-Zugriff # Installation: corsairctl udev | sudo tee /etc/udev/rules.d/99-corsair.rules # Danach: sudo udevadm control --reload-rules && sudo udevadm trigger +# Benutzer muss in der Gruppe "plugdev" sein: sudo usermod -aG plugdev $USER # HS80 RGB Wireless -SUBSYSTEM=="hidraw", ATTRS{idVendor}=="1b1c", ATTRS{idProduct}=="0a6b", MODE="0666" -SUBSYSTEM=="usb", ATTRS{idVendor}=="1b1c", ATTRS{idProduct}=="0a6b", MODE="0666" +SUBSYSTEM=="hidraw", ATTRS{idVendor}=="1b1c", ATTRS{idProduct}=="0a6b", MODE="0660", GROUP="plugdev" +SUBSYSTEM=="usb", ATTRS{idVendor}=="1b1c", ATTRS{idProduct}=="0a6b", MODE="0660", GROUP="plugdev"