From 25eacfc02dc89fa1320ccdac3846a13e837c5092 Mon Sep 17 00:00:00 2001 From: nevaforget Date: Sat, 28 Mar 2026 00:37:36 +0100 Subject: [PATCH] fix: Audit-Befunde in Protokoll-Parsing, Error-Handling und Eingabe-Validierung MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BragiResponse-Felder korrekt zugeordnet (endpoint=raw[1], command=raw[2], status=raw[3]) gemäß Protokoll-Doku. PropertyNotSupported durch DeviceError ersetzt, parse_response_validated in device.rs aktiviert, flush() mit Iterationslimit gegen Endlosschleifen, Sidetone-Range per clap validiert statt clamp, JSON-Escaping im hidpp-battery-waybar.sh, udev auf uaccess umgestellt. 52 Tests grün. --- CLAUDE.md | 2 + scripts/hidpp-battery-waybar.sh | 35 ++++++++++++++ src/bragi/device.rs | 9 ++-- src/bragi/mod.rs | 1 + src/bragi/protocol.rs | 30 +++++++----- src/cli.rs | 3 +- src/error.rs | 4 +- src/hid.rs | 5 +- src/lib.rs | 1 + src/main.rs | 5 +- tests/cli_test.rs | 46 ++++++++++++++++++ tests/output_test.rs | 5 +- tests/properties_test.rs | 17 +++++++ tests/protocol_test.rs | 86 +++++++++++++++++++++++++-------- udev/99-corsair.rules | 7 ++- 15 files changed, 208 insertions(+), 48 deletions(-) create mode 100644 scripts/hidpp-battery-waybar.sh create mode 100644 tests/cli_test.rs diff --git a/CLAUDE.md b/CLAUDE.md index 8c99394..023a54c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -35,6 +35,8 @@ sudo ./target/debug/corsairctl info ## udev-Regel für rootless Zugriff +Nutzt `TAG+="uaccess"` — gibt dem am Seat eingeloggten User automatisch Zugriff, ohne Gruppen-Setup. + ```bash corsairctl udev | sudo tee /etc/udev/rules.d/99-corsair.rules sudo udevadm control --reload-rules && sudo udevadm trigger diff --git a/scripts/hidpp-battery-waybar.sh b/scripts/hidpp-battery-waybar.sh new file mode 100644 index 0000000..90388db --- /dev/null +++ b/scripts/hidpp-battery-waybar.sh @@ -0,0 +1,35 @@ +#!/bin/bash +# ABOUTME: Waybar-Modul für Logitech HID++ Geräte-Batterien. +# ABOUTME: Findet den richtigen hidpp_battery_* Eintrag dynamisch per MODEL_NAME. + +MODEL="G515 LS TKL" +ICON="󰌌" + +# hidpp_battery_* mit passendem MODEL_NAME finden +for dev in /sys/class/power_supply/hidpp_battery_*; do + [[ -d "$dev" ]] || continue + name=$(cat "$dev/model_name" 2>/dev/null) + if [[ "$name" == "$MODEL" ]]; then + capacity=$(cat "$dev/capacity" 2>/dev/null || echo 0) + status=$(cat "$dev/status" 2>/dev/null || echo "Unknown") + + # Eingabe-Validierung gegen JSON-Injection + [[ "$capacity" =~ ^[0-9]+$ ]] || capacity=0 + [[ "$status" =~ ^[A-Za-z\ ]+$ ]] || status="Unknown" + + class="normal" + if [[ "$status" == "Charging" ]]; then + class="charging" + elif (( capacity <= 15 )); then + class="critical" + elif (( capacity <= 30 )); then + class="warning" + fi + + echo "{\"text\":\"${capacity}% ${ICON}\",\"tooltip\":\"${MODEL}: ${capacity}% — ${status}\",\"class\":\"${class}\",\"percentage\":${capacity}}" + exit 0 + fi +done + +# Gerät nicht gefunden — nichts ausgeben, Waybar versteckt das Modul +exit 0 diff --git a/src/bragi/device.rs b/src/bragi/device.rs index 077baa5..19af5c6 100644 --- a/src/bragi/device.rs +++ b/src/bragi/device.rs @@ -4,7 +4,10 @@ use hidapi::{HidApi, HidDevice}; use crate::bragi::properties::{self, BatteryStatus, Property}; -use crate::bragi::protocol::{self, BragiResponse, ENDPOINT_HEADSET, ENDPOINT_RECEIVER}; +use crate::bragi::protocol::{ + self, BragiResponse, ENDPOINT_HEADSET, ENDPOINT_RECEIVER, + RESPONSE_ENDPOINT_HEADSET, CMD_GET, CMD_SET, +}; use crate::error::{CorsairError, Result}; use crate::hid; @@ -128,7 +131,7 @@ impl BragiDevice { let hex: Vec = raw.iter().take(10).map(|b| format!("{b:02X}")).collect(); eprintln!("[query] GET 0x{:02X} → {}", property.id(), hex.join(" ")); } - protocol::parse_response(&raw) + protocol::parse_response_validated(&raw, Some(RESPONSE_ENDPOINT_HEADSET), Some(CMD_GET)) } /// Setzt eine Property auf dem Headset. @@ -138,7 +141,7 @@ impl BragiDevice { hid::flush(&self.device)?; let packet = protocol::build_set_packet(ENDPOINT_HEADSET, property.id(), data); let raw = hid::send_recv(&self.device, &packet)?; - protocol::parse_response(&raw) + protocol::parse_response_validated(&raw, Some(RESPONSE_ENDPOINT_HEADSET), Some(CMD_SET)) } /// Batterie-Level in Prozent (0.0 - 100.0). diff --git a/src/bragi/mod.rs b/src/bragi/mod.rs index 235f1e7..4f6d0b1 100644 --- a/src/bragi/mod.rs +++ b/src/bragi/mod.rs @@ -9,4 +9,5 @@ pub use device::BragiDevice; pub use properties::{BatteryStatus, Property}; pub use protocol::{ BragiResponse, CORSAIR_VID, ENDPOINT_HEADSET, ENDPOINT_RECEIVER, HID_INTERFACE, + RESPONSE_ENDPOINT_HEADSET, RESPONSE_ENDPOINT_RECEIVER, }; diff --git a/src/bragi/protocol.rs b/src/bragi/protocol.rs index 3171e1f..6ceb178 100644 --- a/src/bragi/protocol.rs +++ b/src/bragi/protocol.rs @@ -33,6 +33,10 @@ pub const STATUS_OK: u8 = 0x00; pub const STATUS_ERROR_F0: u8 = 0xF0; pub const STATUS_ERROR_F1: u8 = 0xF1; +// Response-Endpoints (weichen von Request-Endpoints ab!) +pub const RESPONSE_ENDPOINT_RECEIVER: u8 = 0x00; +pub const RESPONSE_ENDPOINT_HEADSET: u8 = 0x01; + /// Baut ein Bragi-Paket (65 Bytes) für den HID-Versand. /// /// Format: [0x00, 0x02, endpoint, command, property, ...data, 0x00-padding] @@ -65,11 +69,14 @@ pub fn build_set_packet(endpoint: u8, property: u8, data: &[u8]) -> [u8; PACKET_ } /// Geparste Bragi-Antwort. +/// +/// Byte-Zuordnung laut Protokoll-Doku: +/// raw[0] = Report-Typ (0x01), raw[1] = Endpoint, raw[2] = Command, raw[3] = Status #[derive(Debug, Clone)] pub struct BragiResponse { - pub status: u8, pub endpoint: u8, pub command: u8, + pub status: u8, pub data: Vec, } @@ -97,11 +104,11 @@ impl BragiResponse { } } -/// Parst eine rohe HID-Response (64 Bytes ohne Report-ID) in eine BragiResponse. +/// Parst eine rohe HID-Response (64 Bytes) in eine BragiResponse. /// -/// Response-Format: [marker, status, endpoint, command, ...data] +/// Response-Format: [report_type, endpoint, command, status, ...data] /// -/// Validiert den Protokoll-Marker und optional den erwarteten Endpoint/Command, +/// Validiert 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) @@ -120,13 +127,14 @@ pub fn parse_response_validated( }); } - let status = raw[1]; - let endpoint = raw[2]; - let command = raw[3]; + // raw[0] = Report-Typ (0x01) + let endpoint = raw[1]; + let command = raw[2]; + let status = raw[3]; - // Fehler-Status prüfen - if status == STATUS_ERROR_F0 || status == STATUS_ERROR_F1 { - return Err(CorsairError::PropertyNotSupported { property: raw[3] }); + // Fehler-Status prüfen: Endpoint-Byte 0xF0/0xF1 signalisiert Fehler + if endpoint == STATUS_ERROR_F0 || endpoint == STATUS_ERROR_F1 { + return Err(CorsairError::DeviceError { status: endpoint }); } // Response-Korrelation: Endpoint und Command gegen die Anfrage prüfen @@ -148,9 +156,9 @@ pub fn parse_response_validated( }; Ok(BragiResponse { - status, endpoint, command, + status, data, }) } diff --git a/src/cli.rs b/src/cli.rs index ab06462..0203c24 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -22,7 +22,8 @@ pub enum Command { /// Sidetone-Level lesen oder setzen (0-23, ALSA-Mixer) Sidetone { /// Sidetone-Level (0-23). Ohne Angabe wird der aktuelle Wert gelesen. - level: Option, + #[arg(value_parser = clap::value_parser!(u8).range(0..=23))] + level: Option, }, /// LED-Helligkeit lesen oder setzen (0-1000) diff --git a/src/error.rs b/src/error.rs index 77ba9c9..974fec2 100644 --- a/src/error.rs +++ b/src/error.rs @@ -14,8 +14,8 @@ pub enum CorsairError { #[error("Headset antwortet nicht — möglicherweise ausgeschaltet")] HeadsetOffline, - #[error("Gerät-Fehler: Property 0x{property:02X} nicht unterstützt")] - PropertyNotSupported { property: u8 }, + #[error("Gerät-Fehler: Status 0x{status:02X} (Property nicht unterstützt)")] + DeviceError { status: u8 }, #[error("Ungültige Antwort: erwartet mindestens {expected} Bytes, bekommen {got}")] ResponseTooShort { expected: usize, got: usize }, diff --git a/src/hid.rs b/src/hid.rs index 6d0c98b..bb136af 100644 --- a/src/hid.rs +++ b/src/hid.rs @@ -89,10 +89,13 @@ pub fn send_recv_optional(device: &HidDevice, packet: &[u8; PACKET_SIZE]) -> Res } } +/// Maximale Iterationen beim Flushen, um Endlosschleifen zu verhindern. +const MAX_FLUSH_ITERATIONS: usize = 64; + /// Leert den HID-Empfangspuffer (nonblocking reads bis leer). pub fn flush(device: &HidDevice) -> Result<()> { let mut buf = [0u8; REPORT_SIZE]; - loop { + for _ in 0..MAX_FLUSH_ITERATIONS { // read_timeout mit 0ms = nonblocking let bytes_read = device.read_timeout(&mut buf, 0)?; if bytes_read == 0 { diff --git a/src/lib.rs b/src/lib.rs index 7e5fdef..a0c6449 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -2,6 +2,7 @@ // ABOUTME: Re-exportiert alle Module für externe Nutzung. pub mod bragi; +pub mod cli; pub mod error; pub mod hid; pub mod output; diff --git a/src/main.rs b/src/main.rs index 29770eb..18bbf96 100644 --- a/src/main.rs +++ b/src/main.rs @@ -25,9 +25,8 @@ fn run() -> error::Result<()> { Command::Sidetone { level } => { if let Some(value) = level { - let clamped = value.clamp(0, 23); - sidetone::set_level(clamped)?; - println!("{}", output::format_sidetone(clamped)); + sidetone::set_level(value as i64)?; + println!("{}", output::format_sidetone(value as i64)); } else { let current = sidetone::get_level()?; println!("{}", output::format_sidetone(current)); diff --git a/tests/cli_test.rs b/tests/cli_test.rs new file mode 100644 index 0000000..c058d81 --- /dev/null +++ b/tests/cli_test.rs @@ -0,0 +1,46 @@ +// ABOUTME: Unit-Tests für CLI-Argument-Parsing und Validierung. +// ABOUTME: Testet clap-Constraints wie Sidetone Range-Validierung. + +use clap::Parser; +use corsairctl::cli::Cli; + +/// Hilfsfunktion: Parst CLI-Argumente aus einem String-Slice. +fn parse_args(args: &[&str]) -> Result { + Cli::try_parse_from(args) +} + +#[test] +fn sidetone_rejects_negative() { + let result = parse_args(&["corsairctl", "sidetone", "--", "-1"]); + assert!(result.is_err()); +} + +#[test] +fn sidetone_rejects_above_23() { + let result = parse_args(&["corsairctl", "sidetone", "24"]); + assert!(result.is_err()); +} + +#[test] +fn sidetone_accepts_valid() { + let result = parse_args(&["corsairctl", "sidetone", "15"]); + assert!(result.is_ok()); +} + +#[test] +fn sidetone_accepts_zero() { + let result = parse_args(&["corsairctl", "sidetone", "0"]); + assert!(result.is_ok()); +} + +#[test] +fn sidetone_accepts_max() { + let result = parse_args(&["corsairctl", "sidetone", "23"]); + assert!(result.is_ok()); +} + +#[test] +fn sidetone_without_value_is_read_mode() { + let result = parse_args(&["corsairctl", "sidetone"]); + assert!(result.is_ok()); +} diff --git a/tests/output_test.rs b/tests/output_test.rs index 60fe2be..9c2323b 100644 --- a/tests/output_test.rs +++ b/tests/output_test.rs @@ -37,9 +37,8 @@ fn format_sidetone_value() { #[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")); + let expected = "Vendor ID: 0x1B1C\nProduct ID: 0x0A69\nFirmware: 1797.0"; + assert_eq!(result, expected); } // Waybar-JSON Tests diff --git a/tests/properties_test.rs b/tests/properties_test.rs index 186fcb0..b61752d 100644 --- a/tests/properties_test.rs +++ b/tests/properties_test.rs @@ -68,6 +68,23 @@ fn battery_promille_to_percent_fractional() { assert!((battery_promille_to_percent(735) - 73.5).abs() < f32::EPSILON); } +#[test] +fn battery_status_icons() { + // Jede Variante muss ein nicht-leeres Icon liefern + assert!(!BatteryStatus::Offline.icon().is_empty()); + assert!(!BatteryStatus::Discharging.icon().is_empty()); + assert!(!BatteryStatus::Low.icon().is_empty()); + assert!(!BatteryStatus::Charging.icon().is_empty()); + assert!(!BatteryStatus::FullyCharged.icon().is_empty()); + assert!(!BatteryStatus::Unknown(0xFF).icon().is_empty()); + + // Discharging und Low teilen das gleiche Icon + assert_eq!(BatteryStatus::Discharging.icon(), BatteryStatus::Low.icon()); + + // Charging und FullyCharged haben unterschiedliche Icons + assert_ne!(BatteryStatus::Charging.icon(), BatteryStatus::FullyCharged.icon()); +} + #[test] fn property_ids_match_protocol() { assert_eq!(Property::BatteryLevel.id(), 0x0F); diff --git a/tests/protocol_test.rs b/tests/protocol_test.rs index f67db58..b3228fd 100644 --- a/tests/protocol_test.rs +++ b/tests/protocol_test.rs @@ -46,22 +46,23 @@ fn build_packet_total_size() { #[test] fn parse_response_extracts_fields() { - // Simulierte Response: [marker, status, endpoint, command, data0, data1, ...] - let mut raw = vec![0x02, 0x00, 0x09, 0x02, 0xE8, 0x03]; + // Simulierte Response: [report_type, endpoint, command, status, data0, data1, ...] + // Laut Protokoll-Doku: raw[0]=0x01 Report-Typ, raw[1]=Endpoint, raw[2]=Command, raw[3]=Status + let mut raw = vec![0x01, RESPONSE_ENDPOINT_HEADSET, CMD_GET, STATUS_OK, 0xE8, 0x03]; raw.resize(REPORT_SIZE, 0x00); let resp = parse_response(&raw).unwrap(); - assert_eq!(resp.status, STATUS_OK); - assert_eq!(resp.endpoint, ENDPOINT_HEADSET); + assert_eq!(resp.endpoint, RESPONSE_ENDPOINT_HEADSET); assert_eq!(resp.command, CMD_GET); + assert_eq!(resp.status, STATUS_OK); assert_eq!(resp.data[0], 0xE8); assert_eq!(resp.data[1], 0x03); } #[test] fn parse_response_value_u16_little_endian() { - let mut raw = vec![0x02, 0x00, 0x09, 0x02, 0xE8, 0x03]; + let mut raw = vec![0x01, RESPONSE_ENDPOINT_HEADSET, CMD_GET, STATUS_OK, 0xE8, 0x03]; raw.resize(REPORT_SIZE, 0x00); let resp = parse_response(&raw).unwrap(); @@ -73,7 +74,7 @@ fn parse_response_value_u16_little_endian() { #[test] fn parse_response_value_u8() { - let mut raw = vec![0x02, 0x00, 0x09, 0x02, 0x03]; + let mut raw = vec![0x01, RESPONSE_ENDPOINT_HEADSET, CMD_GET, STATUS_OK, 0x03]; raw.resize(REPORT_SIZE, 0x00); let resp = parse_response(&raw).unwrap(); @@ -82,31 +83,31 @@ fn parse_response_value_u8() { #[test] fn parse_response_error_f0() { - let raw = vec![0x02, 0xF0, 0x09, 0x02]; + let raw = vec![0x01, 0xF0, CMD_GET, STATUS_OK]; let result = parse_response(&raw); assert!(result.is_err()); - assert!(matches!( - result.unwrap_err(), - CorsairError::PropertyNotSupported { .. } - )); + match result.unwrap_err() { + CorsairError::DeviceError { status } => assert_eq!(status, 0xF0), + other => panic!("Erwarteter DeviceError, bekommen: {other:?}"), + } } #[test] fn parse_response_error_f1() { - let raw = vec![0x02, 0xF1, 0x09, 0x02]; + let raw = vec![0x01, 0xF1, CMD_GET, STATUS_OK]; let result = parse_response(&raw); assert!(result.is_err()); - assert!(matches!( - result.unwrap_err(), - CorsairError::PropertyNotSupported { .. } - )); + match result.unwrap_err() { + CorsairError::DeviceError { status } => assert_eq!(status, 0xF1), + other => panic!("Erwarteter DeviceError, bekommen: {other:?}"), + } } #[test] fn parse_response_too_short() { - let raw = vec![0x02, 0x00]; + let raw = vec![0x01, 0x00]; let result = parse_response(&raw); assert!(result.is_err()); @@ -118,7 +119,7 @@ fn parse_response_too_short() { #[test] fn value_u16_on_empty_data_returns_error() { - let raw = vec![0x02, 0x00, 0x09, 0x02]; // Keine Daten-Bytes + let raw = vec![0x01, RESPONSE_ENDPOINT_HEADSET, CMD_GET, STATUS_OK]; // Keine Daten-Bytes let resp = parse_response(&raw).unwrap(); let result = resp.value_u16(); @@ -132,7 +133,7 @@ fn value_u16_on_empty_data_returns_error() { #[test] fn battery_level_full_is_1000_promille() { // Battery Level = 1000 (voll) → 0xE8, 0x03 LE - let mut raw = vec![0x02, 0x00, 0x09, 0x02, 0xE8, 0x03]; + let mut raw = vec![0x01, RESPONSE_ENDPOINT_HEADSET, CMD_GET, STATUS_OK, 0xE8, 0x03]; raw.resize(REPORT_SIZE, 0x00); let resp = parse_response(&raw).unwrap(); @@ -143,10 +144,55 @@ fn battery_level_full_is_1000_promille() { #[test] fn battery_level_half_is_500_promille() { // Battery Level = 500 (50%) → 0xF4, 0x01 LE - let mut raw = vec![0x02, 0x00, 0x09, 0x02, 0xF4, 0x01]; + let mut raw = vec![0x01, RESPONSE_ENDPOINT_HEADSET, CMD_GET, STATUS_OK, 0xF4, 0x01]; raw.resize(REPORT_SIZE, 0x00); let resp = parse_response(&raw).unwrap(); let promille = resp.value_u16().unwrap(); assert_eq!(promille, 500); } + +// --- Schritt 4: parse_response_validated Tests --- + +#[test] +fn parse_response_validated_rejects_wrong_endpoint() { + let mut raw = vec![0x01, RESPONSE_ENDPOINT_RECEIVER, CMD_GET, STATUS_OK, 0x00]; + raw.resize(REPORT_SIZE, 0x00); + + let result = parse_response_validated(&raw, Some(RESPONSE_ENDPOINT_HEADSET), None); + assert!(result.is_err()); + assert!(matches!( + result.unwrap_err(), + CorsairError::UnexpectedResponse { .. } + )); +} + +#[test] +fn parse_response_validated_rejects_wrong_command() { + let mut raw = vec![0x01, RESPONSE_ENDPOINT_HEADSET, CMD_SET, STATUS_OK, 0x00]; + raw.resize(REPORT_SIZE, 0x00); + + let result = parse_response_validated(&raw, None, Some(CMD_GET)); + assert!(result.is_err()); + assert!(matches!( + result.unwrap_err(), + CorsairError::UnexpectedResponse { .. } + )); +} + +#[test] +fn parse_response_validated_accepts_matching() { + let mut raw = vec![0x01, RESPONSE_ENDPOINT_HEADSET, CMD_GET, STATUS_OK, 0xE8, 0x03]; + raw.resize(REPORT_SIZE, 0x00); + + let resp = parse_response_validated( + &raw, + Some(RESPONSE_ENDPOINT_HEADSET), + Some(CMD_GET), + ) + .unwrap(); + + assert_eq!(resp.endpoint, RESPONSE_ENDPOINT_HEADSET); + assert_eq!(resp.command, CMD_GET); + assert_eq!(resp.status, STATUS_OK); +} diff --git a/udev/99-corsair.rules b/udev/99-corsair.rules index b091002..c8926d4 100644 --- a/udev/99-corsair.rules +++ b/udev/99-corsair.rules @@ -1,8 +1,7 @@ # 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="0660", GROUP="plugdev" -SUBSYSTEM=="usb", ATTRS{idVendor}=="1b1c", ATTRS{idProduct}=="0a6b", MODE="0660", GROUP="plugdev" +# HS80 RGB Wireless — TAG+="uaccess" gibt dem eingeloggten Seat-User Zugriff +SUBSYSTEM=="hidraw", ATTRS{idVendor}=="1b1c", ATTRS{idProduct}=="0a6b", TAG+="uaccess" +SUBSYSTEM=="usb", ATTRS{idVendor}=="1b1c", ATTRS{idProduct}=="0a6b", TAG+="uaccess"