From d4f8ac0bd8fea784bb83dca834c48f507720edf9 Mon Sep 17 00:00:00 2001 From: Viet Anh Nguyen Date: Sun, 19 Jul 2026 20:29:44 +0700 Subject: [PATCH 1/9] security(frontend): escape untrusted strings and set a real CSP Closes the other half of the local privilege-escalation chain. The first half -- unvalidated governor into a root shell -- was fixed earlier; this is the part that let a local user reach it. monitor.js rendered proc.name straight into innerHTML, and that string is the COMMAND column of `ps aux`. Any local user can name a binary ``. With csp:null and withGlobalTauri:true, the injected script got the full __TAURI__ API -- including commands that end in pkexec. escapeHtml existed but was private to security.js, so every other view rendering external strings had none. It moves to utils.js and is applied to process names and status, disk mount points and devices, network interface names, battery strings, and sensor labels. The CSP replaces null with default-src 'self'; script-src 'self'; object-src 'none'; frame-ancestors 'none'. script-src deliberately has no unsafe-inline or unsafe-eval, which would defeat the point. style-src does allow unsafe-inline, because the templates use inline style attributes -- verified rather than assumed. Verified by building the real packages and running the container launch test: the frontend still fetches its 12 templates, injects them and paints. A CSP that broke template loading would have looked identical to a working one in unit tests. Tests: csp_is_set_and_restrictive asserts the directives and that script-src stays strict; views_escape_untrusted_strings asserts escapeHtml is shared and that proc.name specifically is escaped. --- src-tauri/tauri.conf.json | 2 +- src-tauri/tests/packaging.rs | 62 ++++++++++++++++++++++++++++++++++++ src/js/utils.js | 18 +++++++++++ src/js/views/battery.js | 8 ++--- src/js/views/fan.js | 10 +++--- src/js/views/monitor.js | 11 ++++--- src/js/views/security.js | 7 +--- 7 files changed, 97 insertions(+), 21 deletions(-) diff --git a/src-tauri/tauri.conf.json b/src-tauri/tauri.conf.json index 393936e..e246b2f 100755 --- a/src-tauri/tauri.conf.json +++ b/src-tauri/tauri.conf.json @@ -23,7 +23,7 @@ } ], "security": { - "csp": null + "csp": "default-src 'self'; script-src 'self'; style-src 'self' 'unsafe-inline'; img-src 'self' data: asset: http://asset.localhost; font-src 'self' data:; connect-src 'self' ipc: http://ipc.localhost; object-src 'none'; base-uri 'self'; form-action 'none'; frame-ancestors 'none'" } }, "bundle": { diff --git a/src-tauri/tests/packaging.rs b/src-tauri/tests/packaging.rs index 331d4b8..f309f63 100644 --- a/src-tauri/tests/packaging.rs +++ b/src-tauri/tests/packaging.rs @@ -301,3 +301,65 @@ fn the_dialog_container_is_a_flex_column() { "container must stack header, content and actions vertically" ); } + +/// The app enables `withGlobalTauri`, so any injected script reaches the full +/// `__TAURI__` API -- including commands that end in `pkexec`. A null CSP made +/// an XSS in a view (process names from `ps aux` are rendered) into a path to +/// root. Both halves are fixed; this guards the CSP half. +#[test] +fn csp_is_set_and_restrictive() { + let conf = read("src-tauri/tauri.conf.json"); + let parsed: serde_json::Value = serde_json::from_str(&conf).expect("tauri.conf.json parses"); + let csp = parsed["app"]["security"]["csp"] + .as_str() + .expect("csp must be a string, not null"); + + for required in [ + "default-src 'self'", + "script-src 'self'", + "object-src 'none'", + "frame-ancestors 'none'", + ] { + assert!(csp.contains(required), "CSP is missing {}", required); + } + + // 'unsafe-inline' on script-src would defeat the entire point; templates do + // use inline style attributes, so style-src legitimately needs it. + let script_src = csp + .split(';') + .find(|d| d.trim().starts_with("script-src")) + .expect("script-src directive present"); + assert!( + !script_src.contains("unsafe-inline") && !script_src.contains("unsafe-eval"), + "script-src must not allow unsafe-inline or unsafe-eval: {}", + script_src + ); +} + +/// escapeHtml lived privately in security.js, so every other view rendering +/// untrusted strings had no escaping at all. It belongs in utils.js, and the +/// views that render process names, mount points and device labels must use it. +#[test] +fn views_escape_untrusted_strings() { + assert!( + read("src/js/utils.js").contains("export function escapeHtml"), + "escapeHtml must be shared from utils.js, not private to one view" + ); + + for view in ["monitor", "battery", "fan", "security"] { + let src = read(&format!("src/js/views/{}.js", view)); + assert!( + src.contains("escapeHtml"), + "{}.js renders external strings but does not escape them", + view + ); + } + + // The specific reachable case: `ps aux` output is attacker-controllable by + // any local user, who can name a binary ``. + let monitor = read("src/js/views/monitor.js"); + assert!( + monitor.contains("escapeHtml(proc.name)"), + "process names from `ps aux` must be escaped" + ); +} diff --git a/src/js/utils.js b/src/js/utils.js index be1f854..a8bcae6 100755 --- a/src/js/utils.js +++ b/src/js/utils.js @@ -43,3 +43,21 @@ export function showStatus(message, type = 'info') { } }, timeout); } + +/** + * Escape text for safe interpolation into innerHTML. + * + * Several views render strings that originate outside the app — process names + * from `ps aux`, mount points, network interface names, ClamAV threat names. + * Any local user can create a process named ``, and with + * `withGlobalTauri` enabled that script would reach the full `__TAURI__` API. + * + * Lives here rather than in one view because it was previously private to + * security.js, so every other view rendering untrusted strings had no escaping + * at all. + */ +export function escapeHtml(text) { + const div = document.createElement('div'); + div.textContent = text ?? ''; + return div.innerHTML; +} diff --git a/src/js/views/battery.js b/src/js/views/battery.js index 809a23b..349dce7 100755 --- a/src/js/views/battery.js +++ b/src/js/views/battery.js @@ -1,7 +1,7 @@ // Battery View const { invoke } = window.__TAURI__.core; import { elements } from '../dom.js'; -import { showStatus } from '../utils.js'; +import { showStatus, escapeHtml } from '../utils.js'; export function setupBatteryHandlers() { if (elements.thresholdStart) { @@ -47,8 +47,8 @@ function displayBatteries(batteries) { card.className = 'battery-card'; card.innerHTML = `
- ${battery.name} - ${battery.status} + ${escapeHtml(battery.name)} + ${escapeHtml(battery.status)}
${battery.capacity}%
@@ -66,7 +66,7 @@ function displayBatteries(batteries) {
Technology - ${battery.technology} + ${escapeHtml(battery.technology)}
`; diff --git a/src/js/views/fan.js b/src/js/views/fan.js index ed31d0f..a98f11d 100755 --- a/src/js/views/fan.js +++ b/src/js/views/fan.js @@ -2,7 +2,7 @@ const { invoke } = window.__TAURI__.core; import { elements } from '../dom.js'; import { setState, getState } from '../state.js'; -import { showStatus } from '../utils.js'; +import { showStatus, escapeHtml } from '../utils.js'; import { initFanCurve, startCurveMode, stopCurveMode } from '../fanCurve.js'; export function setupFanControl() { @@ -68,8 +68,8 @@ function updateTemperatureDisplay(temps) { const row = document.createElement('div'); row.className = 'metric-row'; row.innerHTML = ` - ${label} - ${value} + ${escapeHtml(label)} + ${escapeHtml(value)} `; elements.tempMetrics.appendChild(row); }); @@ -108,8 +108,8 @@ function updateFanDisplay(fans) { const row = document.createElement('div'); row.className = label === 'Fan1' ? 'metric-row highlight' : 'metric-row'; row.innerHTML = ` - ${label} - ${value} + ${escapeHtml(label)} + ${escapeHtml(value)} `; elements.fanMetrics.appendChild(row); }); diff --git a/src/js/views/monitor.js b/src/js/views/monitor.js index b8e14e1..0157192 100755 --- a/src/js/views/monitor.js +++ b/src/js/views/monitor.js @@ -1,3 +1,4 @@ +import { escapeHtml } from '../utils.js'; // Monitor View const { invoke } = window.__TAURI__.core; import { setState, getState } from '../state.js'; @@ -98,14 +99,14 @@ function displayDiskMonitor(disks) { diskDiv.className = 'disk-item'; diskDiv.innerHTML = `
- ${disk.mount_point} + ${escapeHtml(disk.mount_point)} ${disk.usage_percent.toFixed(1)}%
- ${disk.device} + ${escapeHtml(disk.device)} ${usedGB} GB / ${totalGB} GB
`; @@ -125,7 +126,7 @@ function displayNetworkMonitor(interfaces) { ifaceDiv.className = 'network-item'; ifaceDiv.innerHTML = `
- ${iface.interface} + ${escapeHtml(iface.interface)}
@@ -161,10 +162,10 @@ function displayProcessMonitor(processes) { procDiv.className = 'process-row'; procDiv.innerHTML = ` ${proc.pid} - ${proc.name} + ${escapeHtml(proc.name)} ${proc.cpu_percent.toFixed(1)}% ${proc.memory_mb.toFixed(0)} MB - ${proc.status} + ${escapeHtml(proc.status)} `; container.appendChild(procDiv); }); diff --git a/src/js/views/security.js b/src/js/views/security.js index 41be999..c93c525 100755 --- a/src/js/views/security.js +++ b/src/js/views/security.js @@ -1,3 +1,4 @@ +import { escapeHtml } from '../utils.js'; // Security View - Antivirus and Security Settings const { invoke } = window.__TAURI__.core; @@ -365,12 +366,6 @@ function showNotification(message, type = 'info') { } } -function escapeHtml(text) { - const div = document.createElement('div'); - div.textContent = text; - return div.innerHTML; -} - function showScanLogs(scanType) { const logsSection = document.getElementById('scan-logs-section'); const logsContent = document.getElementById('scan-logs-content'); From deebec115e9123eccafe9e9db24838f5268e539e Mon Sep 17 00:00:00 2001 From: Viet Anh Nguyen Date: Sun, 19 Jul 2026 20:33:40 +0700 Subject: [PATCH 2/9] security: one safe path for running a script as root Five call sites each had their own copy of: build a script, write it to a predictable /tmp path with plain fs::write, chmod it, hand it to pkexec bash. The copies had drifted, so only some had either fix. fs::write on a predictable path follows symlinks and will happily open a file another user pre-created. auth.rs was the worst: /tmp/thinkutils_auth.sh, a fixed name with no randomness at all, so any local user could plant that path and have their content executed as root. privileged::run_script() replaces all of them. Creation is O_EXCL with a random name and mode 0600, which fails rather than following a symlink or reusing a planted file, and the script is always removed -- including when pkexec fails to launch, which several copies leaked. Migrated: performance.rs governor/turbo/boost, battery.rs thresholds, auth.rs, and fan_control.rs's own fallback. fan_control's create_secure_temp_script is gone; it was a second implementation of the same idea, which is how the drift started. Honest about what this does not fix: the file is owned by the invoking user between write and root execution, so that user could swap its contents. That matters only where an administrator authenticates on behalf of a less-privileged user, and closing it means not handing root a user-owned script at all -- the shape the fan helper already uses. Said so in the module docs rather than implying the problem is gone. security.rs also calls pkexec but passes arguments directly with no script file, so it has no equivalent exposure. Tests: mode is 0600, consecutive calls get distinct paths, and create_new refuses an existing path -- the last being the property that actually defeats the planted-file attack. --- src-tauri/src/auth.rs | 31 ++------ src-tauri/src/battery.rs | 38 ++-------- src-tauri/src/fan_control.rs | 91 ++---------------------- src-tauri/src/lib.rs | 1 + src-tauri/src/performance.rs | 64 ++--------------- src-tauri/src/privileged.rs | 134 +++++++++++++++++++++++++++++++++++ 6 files changed, 157 insertions(+), 202 deletions(-) create mode 100644 src-tauri/src/privileged.rs diff --git a/src-tauri/src/auth.rs b/src-tauri/src/auth.rs index 39e5820..6312e2c 100755 --- a/src-tauri/src/auth.rs +++ b/src-tauri/src/auth.rs @@ -1,5 +1,4 @@ use serde::{Deserialize, Serialize}; -use std::fs; #[derive(Debug, Serialize, Deserialize)] pub struct ApiResponse { @@ -15,32 +14,11 @@ pub async fn authenticate_once() -> ApiResponse { // Create a simple script that does nothing but succeeds let script_content = "#!/bin/bash\necho 'Authentication successful'\nexit 0"; - let temp_script = "/tmp/thinkutils_auth.sh"; - if let Err(e) = fs::write(temp_script, script_content) { - return ApiResponse { - success: false, - data: None, - error: Some(format!("Failed to create auth script: {}", e)), - }; - } - - // Make it executable - let _ = std::process::Command::new("chmod") - .arg("+x") - .arg(temp_script) - .output(); - - // Run with pkexec - match tokio::process::Command::new("pkexec") - .env("PKEXEC_UID", std::env::var("UID").unwrap_or_default()) - .arg("bash") - .arg(temp_script) - .output() - .await - { + // Was a fixed path, /tmp/thinkutils_auth.sh, written with plain fs::write -- + // so any local user could pre-create it, or point a symlink at it, and have + // their content executed as root. + match crate::privileged::run_script(script_content).await { Ok(output) => { - let _ = fs::remove_file(temp_script); - if output.status.success() { println!("[Auth] ✓ Authentication successful"); ApiResponse { @@ -59,7 +37,6 @@ pub async fn authenticate_once() -> ApiResponse { } } Err(e) => { - let _ = fs::remove_file(temp_script); println!("[Auth] ✗ Failed to execute pkexec: {}", e); ApiResponse { success: false, diff --git a/src-tauri/src/battery.rs b/src-tauri/src/battery.rs index 6d18b70..f6d2006 100755 --- a/src-tauri/src/battery.rs +++ b/src-tauri/src/battery.rs @@ -203,36 +203,13 @@ pub async fn set_battery_thresholds(start: u8, stop: u8) -> ApiResponse } // Need elevated permissions. Writes stay in the order chosen above. - let temp_script = format!("/tmp/battery_thresholds_{}.sh", std::process::id()); let script_content = format!( "#!/bin/bash\nset -e\necho {} > {}\necho {} > {}\nexit 0\n", first_value, first_path, second_value, second_path ); - if let Err(e) = fs::write(&temp_script, script_content) { - return ApiResponse { - success: false, - data: None, - error: Some(format!("Failed to create script: {}", e)), - }; - } - - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; - let perms = std::fs::Permissions::from_mode(0o755); - let _ = fs::set_permissions(&temp_script, perms); - } - - match tokio::process::Command::new("pkexec") - .arg("bash") - .arg(&temp_script) - .output() - .await - { + match crate::privileged::run_script(&script_content).await { Ok(output) => { - let _ = fs::remove_file(&temp_script); - if output.status.success() { ApiResponse { success: true, @@ -247,14 +224,11 @@ pub async fn set_battery_thresholds(start: u8, stop: u8) -> ApiResponse } } } - Err(e) => { - let _ = fs::remove_file(&temp_script); - ApiResponse { - success: false, - data: None, - error: Some(format!("Failed to execute: {}", e)), - } - } + Err(e) => ApiResponse { + success: false, + data: None, + error: Some(format!("Failed to execute: {}", e)), + }, } } diff --git a/src-tauri/src/fan_control.rs b/src-tauri/src/fan_control.rs index a45ac18..bf61918 100755 --- a/src-tauri/src/fan_control.rs +++ b/src-tauri/src/fan_control.rs @@ -262,25 +262,7 @@ pub async fn enable_fan_control() -> ApiResponse { MODPROBE_CONF_PATH ); - let temp_script = match create_secure_temp_script(&script) { - Ok(p) => p, - Err(e) => { - return ApiResponse { - success: false, - data: None, - error: Some(e), - } - } - }; - - let result = tokio::process::Command::new("pkexec") - .arg("bash") - .arg(&temp_script) - .output() - .await; - let _ = fs::remove_file(&temp_script); - - match result { + match crate::privileged::run_script(&script).await { Ok(output) if output.status.success() => { // Re-probe rather than assume the reload worked. let now_ready = crate::hardware_root::read_to_string(PROC_FAN) @@ -326,44 +308,6 @@ pub struct ApiResponse { pub error: Option, } -/// Create a temp script securely (O_EXCL prevents symlink attacks, random name, restricted perms) -#[cfg(unix)] -pub fn create_secure_temp_script(content: &str) -> Result { - use std::io::Write; - use std::os::unix::fs::OpenOptionsExt; - - let random = std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .unwrap_or_default() - .as_nanos(); - let path = format!("/tmp/thinkutils_{}.sh", random); - - let mut file = fs::OpenOptions::new() - .create_new(true) // O_EXCL: fail if exists, don't follow symlinks - .write(true) - .mode(0o700) // Only owner can read/write/execute - .open(&path) - .map_err(|e| format!("Failed to create temp script: {}", e))?; - - file.write_all(content.as_bytes()).map_err(|e| { - let _ = fs::remove_file(&path); - format!("Failed to write temp script: {}", e) - })?; - - Ok(path) -} - -#[cfg(not(unix))] -pub fn create_secure_temp_script(content: &str) -> Result { - let random = std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .unwrap_or_default() - .as_nanos(); - let path = format!("/tmp/thinkutils_{}.sh", random); - fs::write(&path, content).map_err(|e| format!("Failed to create temp script: {}", e))?; - Ok(path) -} - #[tauri::command] pub fn get_sensor_data() -> ApiResponse { let mut temps = HashMap::new(); @@ -527,26 +471,8 @@ pub async fn set_fan_speed(speed: String) -> ApiResponse { command_str, PROC_FAN ); - let temp_script = match create_secure_temp_script(&script_content) { - Ok(path) => path, - Err(e) => { - return ApiResponse { - success: false, - data: None, - error: Some(e), - }; - } - }; - - match tokio::process::Command::new("pkexec") - .arg("bash") - .arg(&temp_script) - .output() - .await - { + match crate::privileged::run_script(&script_content).await { Ok(output) => { - let _ = fs::remove_file(&temp_script); - if output.status.success() { println!("[Fan] ✓ Speed set via pkexec"); ApiResponse { @@ -562,14 +488,11 @@ pub async fn set_fan_speed(speed: String) -> ApiResponse { } } } - Err(e) => { - let _ = fs::remove_file(&temp_script); - ApiResponse { - success: false, - data: None, - error: Some(format!("Failed to execute pkexec: {}", e)), - } - } + Err(e) => ApiResponse { + success: false, + data: None, + error: Some(format!("Failed to execute pkexec: {}", e)), + }, } } diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index bd6dd45..0cddb7a 100755 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -8,6 +8,7 @@ mod mcp; mod monitor; mod performance; mod permissions; +mod privileged; mod security; mod settings; mod sync; diff --git a/src-tauri/src/performance.rs b/src-tauri/src/performance.rs index 99dd7d6..fccd486 100755 --- a/src-tauri/src/performance.rs +++ b/src-tauri/src/performance.rs @@ -154,37 +154,14 @@ pub async fn set_cpu_governor(governor: String) -> ApiResponse { }; } - let temp_script = format!("/tmp/set_governor_{}.sh", std::process::id()); let script_content = governor_script(&governor, CPU_GLOB); println!("[Performance] Script content:\n{}", script_content); - if let Err(e) = fs::write(&temp_script, &script_content) { - return ApiResponse { - success: false, - data: None, - error: Some(format!("Failed to create script: {}", e)), - }; - } - - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; - let perms = std::fs::Permissions::from_mode(0o755); - let _ = fs::set_permissions(&temp_script, perms); - } - println!("[Performance] Executing pkexec..."); - match tokio::process::Command::new("pkexec") - .arg("bash") - .arg(&temp_script) - .output() - .await - { + match crate::privileged::run_script(&script_content).await { Ok(output) => { - let _ = fs::remove_file(&temp_script); - let stdout = String::from_utf8_lossy(&output.stdout); let stderr = String::from_utf8_lossy(&output.stderr); @@ -214,7 +191,6 @@ pub async fn set_cpu_governor(governor: String) -> ApiResponse { } } Err(e) => { - let _ = fs::remove_file(&temp_script); println!("[Performance] Failed to execute pkexec: {}", e); ApiResponse { success: false, @@ -391,28 +367,13 @@ pub async fn set_turbo_boost(enabled: bool) -> ApiResponse { // Try Intel P-state first if std::path::Path::new(intel_pstate).exists() { - let temp_script = format!("/tmp/set_turbo_{}.sh", std::process::id()); let script_content = format!( "#!/bin/bash\nset -e\necho {} > {}\nexit 0\n", value, intel_pstate ); - if fs::write(&temp_script, script_content).is_ok() { - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; - let perms = std::fs::Permissions::from_mode(0o755); - let _ = fs::set_permissions(&temp_script, perms); - } - - if let Ok(output) = tokio::process::Command::new("pkexec") - .arg("bash") - .arg(&temp_script) - .output() - .await - { - let _ = fs::remove_file(&temp_script); - + { + if let Ok(output) = crate::privileged::run_script(&script_content).await { if output.status.success() { return ApiResponse { success: true, @@ -429,28 +390,13 @@ pub async fn set_turbo_boost(enabled: bool) -> ApiResponse { // Try cpufreq boost if std::path::Path::new(cpufreq_boost).exists() { - let temp_script = format!("/tmp/set_boost_{}.sh", std::process::id()); let script_content = format!( "#!/bin/bash\nset -e\necho {} > {}\nexit 0\n", boost_value, cpufreq_boost ); - if fs::write(&temp_script, script_content).is_ok() { - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; - let perms = std::fs::Permissions::from_mode(0o755); - let _ = fs::set_permissions(&temp_script, perms); - } - - if let Ok(output) = tokio::process::Command::new("pkexec") - .arg("bash") - .arg(&temp_script) - .output() - .await - { - let _ = fs::remove_file(&temp_script); - + { + if let Ok(output) = crate::privileged::run_script(&script_content).await { if output.status.success() { return ApiResponse { success: true, diff --git a/src-tauri/src/privileged.rs b/src-tauri/src/privileged.rs new file mode 100644 index 0000000..58d90f8 --- /dev/null +++ b/src-tauri/src/privileged.rs @@ -0,0 +1,134 @@ +//! Running a shell script as root, once, safely. +//! +//! Five call sites each had their own copy of this: build a script, write it to +//! a predictable `/tmp` path with plain `fs::write`, chmod it, hand it to +//! `pkexec bash`. That pattern has two problems, and the copies had drifted so +//! only some of them had either fix. +//! +//! `fs::write` on a predictable path follows symlinks and happily opens a file +//! another user pre-created. `/tmp/thinkutils_auth.sh` was a fixed name with no +//! randomness at all, so another local user could plant that path and have their +//! content executed as root. +//! +//! Creation here is `O_EXCL` with a random name and mode 0600, which fails +//! rather than following a symlink or reusing a planted file. Root can still +//! read it — root bypasses permission bits — so the script runs as intended. +//! +//! What this does NOT solve: the file is owned by the invoking user for the +//! window between writing and root executing it, so that user could swap its +//! contents. That matters only where an administrator authenticates on behalf of +//! a less-privileged user, and closing it properly means not passing a +//! user-owned script to root at all — the shape the fan helper already uses. + +use std::process::Output; + +/// Create a script only this user can read, at an unpredictable path. +/// +/// Returns the path; the caller is responsible for removing it, which +/// [`run_script`] does. +#[cfg(unix)] +fn create_secure_script(content: &str) -> Result { + use std::io::Write; + use std::os::unix::fs::OpenOptionsExt; + + // Nanosecond clock plus pid: enough to make the name unpredictable in + // practice, and O_EXCL below is what actually enforces exclusivity. + let nanos = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap_or_default() + .as_nanos(); + let path = format!("/tmp/thinkutils_{}_{}.sh", std::process::id(), nanos); + + let mut file = std::fs::OpenOptions::new() + .create_new(true) // O_EXCL: refuse to follow a symlink or reuse a planted file + .write(true) + .mode(0o600) + .open(&path) + .map_err(|e| format!("Failed to create privileged script: {}", e))?; + + file.write_all(content.as_bytes()).map_err(|e| { + let _ = std::fs::remove_file(&path); + format!("Failed to write privileged script: {}", e) + })?; + + Ok(path) +} + +#[cfg(not(unix))] +fn create_secure_script(content: &str) -> Result { + let path = format!("/tmp/thinkutils_{}.sh", std::process::id()); + std::fs::write(&path, content) + .map_err(|e| format!("Failed to create privileged script: {}", e))?; + Ok(path) +} + +/// Run a script as root via pkexec, then remove it. +/// +/// The script is always cleaned up, including when pkexec fails to launch — +/// the previous copies leaked the file on some error paths. +pub async fn run_script(script: &str) -> Result { + let path = create_secure_script(script)?; + + let result = tokio::process::Command::new("pkexec") + .arg("bash") + .arg(&path) + .output() + .await + .map_err(|e| format!("Failed to execute pkexec: {}", e)); + + let _ = std::fs::remove_file(&path); + result +} + +#[cfg(test)] +mod tests { + use super::*; + + #[cfg(unix)] + #[test] + fn script_is_created_unreadable_to_other_users() { + use std::os::unix::fs::PermissionsExt; + + let path = create_secure_script("#!/bin/bash\nexit 0\n").expect("create"); + let mode = std::fs::metadata(&path).unwrap().permissions().mode() & 0o777; + assert_eq!(mode, 0o600, "expected 0600, got {:o}", mode); + let _ = std::fs::remove_file(&path); + } + + /// The name must not be guessable from the pid alone: two calls from the + /// same process must not collide, or a second invocation could reuse a path + /// an attacker already knows. + #[cfg(unix)] + #[test] + fn consecutive_scripts_get_distinct_paths() { + let a = create_secure_script("a").expect("first"); + let b = create_secure_script("b").expect("second"); + assert_ne!(a, b); + assert_eq!(std::fs::read_to_string(&a).unwrap(), "a"); + assert_eq!(std::fs::read_to_string(&b).unwrap(), "b"); + let _ = std::fs::remove_file(&a); + let _ = std::fs::remove_file(&b); + } + + /// O_EXCL is the load-bearing part. Without it, a path another user planted + /// (or a symlink they pointed at a file they want overwritten as root) would + /// be opened and used. + #[cfg(unix)] + #[test] + fn refuses_to_reuse_an_existing_path() { + let path = create_secure_script("original").expect("create"); + + // Simulate the planted-file case by trying to create the same path again + // through the same code path the attacker's target would take. + let direct = std::fs::OpenOptions::new() + .create_new(true) + .write(true) + .open(&path); + assert!( + direct.is_err(), + "create_new must fail on an existing path - without it a planted file would be reused" + ); + + let _ = std::fs::remove_file(&path); + } +} From e134454238454fb7f7c488a5bb4dd84e5724b01f Mon Sep 17 00:00:00 2001 From: Viet Anh Nguyen Date: Sun, 19 Jul 2026 20:44:21 +0700 Subject: [PATCH 3/9] fix: battery thresholds now grantable, and MCP stops stealing the OAuth port Two silent failures, both from the same cause: the same thing named in two places, drifting apart. BATTERY THRESHOLDS permissions.rs granted write access to charge_start_threshold and charge_stop_threshold, while battery.rs wrote charge_control_start_threshold and charge_control_end_threshold. On a ThinkPad BOTH pairs exist and report the same value -- confirmed on hardware, both 75/80 -- but they are separate sysfs files, so a chmod on one never affected the other. The result: 'Grant Permissions' reported success and battery thresholds stayed unwritable, so every change fell through to a password prompt with no explanation. mcp.rs named a third variant. battery::threshold_paths() is now the single source of truth, preferring the generic kernel names and falling back to the thinkpad_acpi spelling. permissions.rs and mcp.rs both go through it. Also removed /sys/devices/platform/thinkpad_hwmon/pwm1 from the required list: that path does not exist. The real attribute is under .../thinkpad_hwmon/hwmon/hwmonN/pwm1, and the exists() guard meant the wrong path was skipped rather than reported. It is discovered now. PORT COLLISION The MCP server defaulted to 8765, which is the port sync.rs binds for the OAuth callback. With MCP running the callback listener could not bind, so Google sign-in never completed and nothing said why. MCP moves to 8779. It was the one to move: its port is local config, while the callback port is registered as the redirect URI in Google Cloud Console and cannot change without updating the OAuth client. Tests pin both: that the two ports differ, that REDIRECT_URI still embeds the callback port (it is a literal, since a const cannot call format!), that the generic attribute names are preferred, and that a candidate pair never mixes naming schemes -- writing a generic start with a legacy stop would touch two different files. Docs and the MCP view updated to 8779, with a note explaining the change for anyone who configured a client against the old port. --- docs/guide/mcp.md | 33 ++++++++--- src-tauri/src/battery.rs | 107 +++++++++++++++++++++++++++++++++-- src-tauri/src/mcp.rs | 43 ++++++++++---- src-tauri/src/permissions.rs | 48 ++++++++++++---- src-tauri/src/sync.rs | 33 ++++++++++- src/js/views/mcp.js | 2 +- src/templates/views/mcp.html | 18 +++--- 7 files changed, 239 insertions(+), 45 deletions(-) diff --git a/docs/guide/mcp.md b/docs/guide/mcp.md index 2270d8e..98b1e52 100644 --- a/docs/guide/mcp.md +++ b/docs/guide/mcp.md @@ -4,6 +4,23 @@ ThinkUtils includes a built-in MCP (Model Context Protocol) server that exposes ![AI Integration](/screenshots/ai_integration.png) + + + +::: warning Transport, endpoint and port all changed +The server now speaks **Streamable HTTP** at `/mcp`, not SSE at `/sse`. The rmcp +library removed its SSE server transport, and Streamable HTTP is where the fix +for a DNS-rebinding advisory landed — it validates `Host` and `Origin`, which +stops a page you visit from reaching the server on loopback. + +The default port is now **8779**, not 8765, which collided with the Google Drive +sign-in callback and made sign-in silently never complete while the MCP server +was running. + +Existing client configs need all three: `--transport http` and +`http://127.0.0.1:8779/mcp`. +::: + ## What is MCP? [Model Context Protocol](https://modelcontextprotocol.io) is a standard protocol that lets AI assistants interact with external tools. ThinkUtils implements an MCP server so AI tools can monitor and control your ThinkPad settings. @@ -28,7 +45,7 @@ Start the MCP server from the app's MCP page, then configure your AI tool: ### Claude Code ```bash -claude mcp add --transport http thinkutils http://127.0.0.1:8765/mcp +claude mcp add --transport http thinkutils http://127.0.0.1:8779/mcp ``` Or add to `.mcp.json` in your project: @@ -38,7 +55,7 @@ Or add to `.mcp.json` in your project: "mcpServers": { "thinkutils": { "type": "http", - "url": "http://127.0.0.1:8765/mcp" + "url": "http://127.0.0.1:8779/mcp" } } } @@ -52,7 +69,7 @@ Add to `~/.config/Claude/claude_desktop_config.json`: { "mcpServers": { "thinkutils": { - "url": "http://127.0.0.1:8765/mcp" + "url": "http://127.0.0.1:8779/mcp" } } } @@ -66,7 +83,7 @@ Add to `.cursor/mcp.json` (project) or `~/.cursor/mcp.json` (global): { "mcpServers": { "thinkutils": { - "url": "http://127.0.0.1:8765/mcp" + "url": "http://127.0.0.1:8779/mcp" } } } @@ -80,7 +97,7 @@ Add to `~/.codeium/windsurf/mcp_config.json`: { "mcpServers": { "thinkutils": { - "url": "http://127.0.0.1:8765/mcp" + "url": "http://127.0.0.1:8779/mcp" } } } @@ -94,7 +111,7 @@ Add to `~/.lmstudio/mcp.json`: { "mcpServers": { "thinkutils": { - "url": "http://127.0.0.1:8765/mcp" + "url": "http://127.0.0.1:8779/mcp" } } } @@ -107,7 +124,7 @@ Or in the app: switch to the **Program** tab, click **Install**, then **Edit mcp In ChatGPT Desktop, click your profile > **Settings** > **Connectors** > **Advanced settings**, enable **Developer mode**, then go back to Connectors and click **Create**: - **Name**: ThinkUtils -- **Server URL**: `http://127.0.0.1:8765/mcp` +- **Server URL**: `http://127.0.0.1:8779/mcp` ::: info Requires ChatGPT Desktop with MCP support (Plus/Team/Enterprise). @@ -115,4 +132,4 @@ Requires ChatGPT Desktop with MCP support (Plus/Team/Enterprise). ### Other Tools -For any MCP-compatible client, configure a Streamable HTTP server with URL `http://127.0.0.1:8765/mcp`. +For any MCP-compatible client, configure a Streamable HTTP server with URL `http://127.0.0.1:8779/mcp`. diff --git a/src-tauri/src/battery.rs b/src-tauri/src/battery.rs index f6d2006..a15b992 100755 --- a/src-tauri/src/battery.rs +++ b/src-tauri/src/battery.rs @@ -117,10 +117,54 @@ fn read_battery_info(path: &str, index: usize) -> Result { }) } +/// Attribute names for the charge thresholds, most-standard first. +/// +/// `charge_control_*` is the generic kernel power-supply API and works beyond +/// ThinkPads. `charge_*_threshold` is the older thinkpad_acpi-specific spelling. +/// +/// On a ThinkPad BOTH exist and report the same value, but they are separate +/// sysfs files — so a chmod on one does not affect the other. That is exactly +/// how this broke: permissions.rs granted access to the legacy pair while +/// battery.rs wrote the standard pair, so "Grant Permissions" never made battery +/// thresholds writable and every change fell through to a password prompt. +const THRESHOLD_ATTRS: &[(&str, &str)] = &[ + ( + "charge_control_start_threshold", + "charge_control_end_threshold", + ), + ("charge_start_threshold", "charge_stop_threshold"), +]; + +/// The threshold file pair this machine actually exposes. +/// +/// Returns the first pair where both files exist. Every caller must go through +/// here — the duplication between modules is what allowed them to disagree. +pub fn threshold_paths() -> Option<(String, String)> { + THRESHOLD_ATTRS.iter().find_map(|(start, stop)| { + let start_path = format!("{}/{}", BAT0_PATH, start); + let stop_path = format!("{}/{}", BAT0_PATH, stop); + (Path::new(&start_path).exists() && Path::new(&stop_path).exists()) + .then_some((start_path, stop_path)) + }) +} + #[tauri::command] pub fn get_battery_thresholds() -> ApiResponse { - let start_path = format!("{}/charge_control_start_threshold", BAT0_PATH); - let stop_path = format!("{}/charge_control_end_threshold", BAT0_PATH); + let (start_path, stop_path) = match threshold_paths() { + Some(pair) => pair, + None => { + // Preserve the previous defaults so callers that ignore `success` + // keep behaving as before. + return ApiResponse { + success: true, + data: Some(BatteryThresholds { + start: 0, + stop: 100, + }), + error: None, + }; + } + }; let start = fs::read_to_string(&start_path) .ok() @@ -171,8 +215,18 @@ pub async fn set_battery_thresholds(start: u8, stop: u8) -> ApiResponse }; } - let start_path = format!("{}/charge_control_start_threshold", BAT0_PATH); - let stop_path = format!("{}/charge_control_end_threshold", BAT0_PATH); + let (start_path, stop_path) = match threshold_paths() { + Some(pair) => pair, + None => { + return ApiResponse { + success: false, + data: None, + error: Some( + "This machine exposes no battery charge threshold controls.".to_string(), + ), + } + } + }; // get_battery_thresholds() has no failure path — it substitutes defaults on a // failed read — so there is nothing to match on. Note the substituted default @@ -325,3 +379,48 @@ mod tests { assert!(!write_start_first(0, 80)); } } + +#[cfg(test)] +mod threshold_path_tests { + use super::*; + + /// The generic kernel API must be preferred. Both spellings exist on a + /// ThinkPad and report the same value, but only the generic one exists on + /// other hardware, so choosing the legacy pair first would silently limit + /// support to ThinkPads. + #[test] + fn prefers_the_generic_kernel_attribute_names() { + assert_eq!( + THRESHOLD_ATTRS[0], + ( + "charge_control_start_threshold", + "charge_control_end_threshold" + ) + ); + } + + /// The legacy thinkpad_acpi spelling stays as a fallback for older kernels + /// that expose only it. + #[test] + fn keeps_the_legacy_spelling_as_a_fallback() { + assert!(THRESHOLD_ATTRS + .iter() + .any(|(s, e)| *s == "charge_start_threshold" && *e == "charge_stop_threshold")); + } + + /// Start and stop must never come from different naming schemes: writing a + /// generic start and a legacy stop would touch two different sysfs files and + /// could leave the pair inconsistent. + #[test] + fn each_candidate_pair_uses_one_naming_scheme() { + for (start, stop) in THRESHOLD_ATTRS { + let start_is_generic = start.starts_with("charge_control_"); + let stop_is_generic = stop.starts_with("charge_control_"); + assert_eq!( + start_is_generic, stop_is_generic, + "mixed naming scheme in pair ({}, {})", + start, stop + ); + } + } +} diff --git a/src-tauri/src/mcp.rs b/src-tauri/src/mcp.rs index 0714cf3..4e63cf4 100644 --- a/src-tauri/src/mcp.rs +++ b/src-tauri/src/mcp.rs @@ -11,6 +11,13 @@ use std::sync::Arc; use tokio::sync::Mutex; use tokio_util::sync::CancellationToken; +/// Default port for the MCP server. +/// +/// Deliberately not 8765: that is sync.rs's OAuth callback port, and while the +/// MCP server held it the callback listener could not bind, so Google login +/// failed with no visible error. A test asserts the two stay different. +pub const DEFAULT_MCP_PORT: u16 = 8779; + const VALID_FAN_SPEEDS: &[&str] = &["auto", "full-speed", "0", "1", "2", "3", "4", "5", "6", "7"]; fn validate_fan_speed(speed: &str) -> Option { @@ -79,7 +86,7 @@ impl Default for McpServerState { Self { cancel_token: None, host: "127.0.0.1".to_string(), - port: 8765, + port: DEFAULT_MCP_PORT, } } } @@ -177,7 +184,7 @@ impl ThinkUtilsHandler { format!( "Status: {}\nCapacity: {}%\nCycle Count: {}\nTechnology: {}\nStart Threshold: {}%\nStop Threshold: {}%", r("status"), r("capacity"), r("cycle_count"), r("technology"), - r("charge_start_threshold"), r("charge_stop_threshold"), + r("charge_control_start_threshold"), r("charge_control_end_threshold"), ) } @@ -189,18 +196,19 @@ impl ThinkUtilsHandler { if let Some(err) = validate_battery_thresholds(req.start, req.stop) { return err; } + // Resolved rather than hardcoded: the attribute names differ between the + // generic kernel API and thinkpad_acpi's older spelling, and this module + // used to name a different pair than battery.rs. + let Some((start_path, stop_path)) = crate::battery::threshold_paths() else { + return "This machine exposes no battery charge threshold controls.".to_string(); + }; + let mut r = Vec::new(); - match fs::write( - "/sys/class/power_supply/BAT0/charge_stop_threshold", - req.stop.to_string(), - ) { + match fs::write(&stop_path, req.stop.to_string()) { Ok(_) => r.push(format!("Stop set to {}%", req.stop)), Err(e) => r.push(format!("Stop failed: {}", e)), } - match fs::write( - "/sys/class/power_supply/BAT0/charge_start_threshold", - req.start.to_string(), - ) { + match fs::write(&start_path, req.start.to_string()) { Ok(_) => r.push(format!("Start set to {}%", req.start)), Err(e) => r.push(format!("Start failed: {}", e)), } @@ -705,11 +713,24 @@ mod tests { // -- McpServerState defaults -- + /// The MCP server and the OAuth callback listener cannot both bind the same + /// port, and the failure is silent: with MCP running, the callback server + /// fails to bind and Google login simply never completes. They used to share + /// 8765. + #[test] + fn mcp_port_does_not_collide_with_the_oauth_callback() { + assert_ne!( + DEFAULT_MCP_PORT, + crate::sync::OAUTH_CALLBACK_PORT, + "MCP and the OAuth callback would fight over the same port" + ); + } + #[test] fn default_state() { let state = McpServerState::default(); assert_eq!(state.host, "127.0.0.1"); - assert_eq!(state.port, 8765); + assert_eq!(state.port, DEFAULT_MCP_PORT); assert!(state.cancel_token.is_none()); } diff --git a/src-tauri/src/permissions.rs b/src-tauri/src/permissions.rs index 808f53a..fbd6ba3 100755 --- a/src-tauri/src/permissions.rs +++ b/src-tauri/src/permissions.rs @@ -20,28 +20,56 @@ pub struct PermissionStatus { pub missing_files: Vec, } -// Files that need write permissions +// Files that need write permissions and are the same on every machine. +// +// The battery thresholds are NOT here: their attribute names vary, and this list +// previously named the legacy thinkpad_acpi pair while battery.rs wrote the +// standard kernel pair. Both exist on a ThinkPad and report the same value, but +// they are separate sysfs files, so granting one never affected the other -- +// "Grant Permissions" silently never fixed battery thresholds. They come from +// battery::threshold_paths() now, which is the single source of truth. +// +// thinkpad_hwmon/pwm1 is also gone: that path does not exist. The real attribute +// lives under .../thinkpad_hwmon/hwmon/hwmonN/pwm1, and the exists() guard below +// meant the wrong path was silently skipped rather than reported. const REQUIRED_FILES: &[&str] = &[ "/sys/devices/system/cpu/cpu0/cpufreq/scaling_governor", "/sys/devices/system/cpu/intel_pstate/no_turbo", - "/sys/devices/platform/thinkpad_hwmon/pwm1", - "/sys/class/power_supply/BAT0/charge_start_threshold", - "/sys/class/power_supply/BAT0/charge_stop_threshold", ]; +/// Every sysfs file the app wants writable, resolved for this machine. +fn required_files() -> Vec { + let mut files: Vec = REQUIRED_FILES.iter().map(|s| s.to_string()).collect(); + if let Some((start, stop)) = crate::battery::threshold_paths() { + files.push(start); + files.push(stop); + } + // The thinkpad hwmon PWM lives under a numbered hwmon directory, so it has to + // be discovered rather than hardcoded. + if let Ok(entries) = fs::read_dir("/sys/devices/platform/thinkpad_hwmon/hwmon") { + for entry in entries.flatten() { + let pwm = entry.path().join("pwm1"); + if pwm.exists() { + files.push(pwm.to_string_lossy().to_string()); + } + } + } + files +} + #[tauri::command] pub async fn check_permissions_status() -> ApiResponse { let mut missing_files = Vec::new(); - for file_path in REQUIRED_FILES { - if Path::new(file_path).exists() { + for file_path in required_files() { + if Path::new(&file_path).exists() { // Check if we can write to it - match fs::OpenOptions::new().write(true).open(file_path) { + match fs::OpenOptions::new().write(true).open(&file_path) { Ok(_) => { // We have permission } Err(_) => { - missing_files.push(file_path.to_string()); + missing_files.push(file_path.clone()); } } } @@ -98,8 +126,8 @@ pub async fn setup_permissions() -> ApiResponse { ]; // Add chmod commands for each file that exists - for file_path in REQUIRED_FILES { - if Path::new(file_path).exists() { + for file_path in required_files() { + if Path::new(&file_path).exists() { script_lines.push(format!("if [ -f {} ]; then", file_path)); script_lines.push(format!(" chmod 666 {} 2>/dev/null || true", file_path)); script_lines.push(format!( diff --git a/src-tauri/src/sync.rs b/src-tauri/src/sync.rs index 94cfbd1..0fd72a9 100755 --- a/src-tauri/src/sync.rs +++ b/src-tauri/src/sync.rs @@ -26,6 +26,16 @@ use std::sync::{Arc, Mutex}; // credential as an "OAuth client ID" of type "Desktop app" at // https://console.cloud.google.com (APIs & Services > Credentials). const GOOGLE_CLIENT_ID: Option<&str> = option_env!("THINKUTILS_GOOGLE_CLIENT_ID"); +/// Port the OAuth callback listener binds while a login is in flight. +/// +/// Must differ from the MCP server's default port: they cannot both bind it, and +/// the failure is silent -- with MCP running, the callback server fails to bind +/// and Google login just never completes. mcp.rs asserts they differ. +/// +/// This value is also registered as the redirect URI in Google Cloud Console, so +/// changing it requires updating the OAuth client. That is why the MCP port moved +/// instead of this one. +pub const OAUTH_CALLBACK_PORT: u16 = 8765; const REDIRECT_URI: &str = "http://localhost:8765/callback"; #[derive(Debug, Serialize, Deserialize, Clone)] @@ -275,8 +285,8 @@ pub async fn google_auth_init() -> ApiResponse { async fn start_callback_server() -> Result<(), String> { use tiny_http::{Response, Server}; - let server = - Server::http("127.0.0.1:8765").map_err(|e| format!("Failed to start server: {}", e))?; + let server = Server::http(format!("127.0.0.1:{}", OAUTH_CALLBACK_PORT).as_str()) + .map_err(|e| format!("Failed to start server: {}", e))?; println!("[OAuth] Callback server listening on {}", REDIRECT_URI); @@ -876,3 +886,22 @@ mod public_client_tests { ); } } + +#[cfg(test)] +mod port_tests { + use super::*; + + /// REDIRECT_URI embeds the port as a literal because a const cannot call + /// format!. If the constant moves and the URI does not, the callback listens + /// on one port while Google redirects to another -- and login hangs with no + /// error anywhere. + #[test] + fn redirect_uri_matches_the_callback_port() { + assert!( + REDIRECT_URI.contains(&format!(":{}/", OAUTH_CALLBACK_PORT)), + "REDIRECT_URI ({}) does not use OAUTH_CALLBACK_PORT ({})", + REDIRECT_URI, + OAUTH_CALLBACK_PORT + ); + } +} diff --git a/src/js/views/mcp.js b/src/js/views/mcp.js index de4d727..766b889 100644 --- a/src/js/views/mcp.js +++ b/src/js/views/mcp.js @@ -152,7 +152,7 @@ async function toggleMcpServer() { const hostInput = document.getElementById('mcp-host'); const portInput = document.getElementById('mcp-port'); const host = hostInput ? hostInput.value : '127.0.0.1'; - const port = portInput ? parseInt(portInput.value) || 8765 : 8765; + const port = portInput ? parseInt(portInput.value) || 8779 : 8779; const response = await invoke('start_mcp_server', { host, port }); if (!response.success && text) { diff --git a/src/templates/views/mcp.html b/src/templates/views/mcp.html index e20d4dc..87496de 100644 --- a/src/templates/views/mcp.html +++ b/src/templates/views/mcp.html @@ -16,7 +16,7 @@

MCP Server

- +
@@ -72,7 +72,7 @@

Available Tools

Setup Instructions

Start the MCP server above, then add it to your AI tool. Server URL: - http://127.0.0.1:8765/sse + http://127.0.0.1:8779/mcp

@@ -87,7 +87,7 @@

Setup Instructions

Run this command:

-claude mcp add --transport http thinkutils http://127.0.0.1:8765/sse

Or add to .mcp.json in your project:

@@ -96,7 +96,7 @@

Setup Instructions

"mcpServers": { "thinkutils": { "type": "sse", - "url": "http://127.0.0.1:8765/sse" + "url": "http://127.0.0.1:8779/mcp" } } }Setup Instructions { "mcpServers": { "thinkutils": { - "url": "http://127.0.0.1:8765/sse" + "url": "http://127.0.0.1:8779/mcp" } } }Setup Instructions 

 Name: ThinkUtils
-Server URL: http://127.0.0.1:8765/sse

Requires ChatGPT Desktop with MCP support (Plus/Team/Enterprise).

@@ -142,7 +142,7 @@

Setup Instructions

{ "mcpServers": { "thinkutils": { - "url": "http://127.0.0.1:8765/sse" + "url": "http://127.0.0.1:8779/mcp" } } }Setup Instructions { "mcpServers": { "thinkutils": { - "url": "http://127.0.0.1:8765/sse" + "url": "http://127.0.0.1:8779/mcp" } } }Setup Instructions

For any MCP-compatible client, configure an SSE server with:

-URL: http://127.0.0.1:8765/sse
+URL: http://127.0.0.1:8779/mcp
 Transport: SSE (Server-Sent Events)

From 76c1f7cf584e7f4409f670ae429587e29233c53e Mon Sep 17 00:00:00 2001 From: Viet Anh Nguyen Date: Sun, 19 Jul 2026 21:49:48 +0700 Subject: [PATCH 4/9] refactor(ui): give views a lifecycle, and make dialogs keyboard-usable navigation.js held a 9-branch hide block, a separate titles map, and a 9-case show switch. The titles map and the view templates had already drifted -- the MCP subtitle differed between them -- and every view repeated its own title and subtitle directly under the page header that already showed both. views/registry.js is now the single source of truth: id, title, subtitle, element, display mode, onShow, onHide. navigation.js reads it and is ~80 lines shorter. The duplicated headers are gone from seven templates. The missing piece was hiding. Nothing was ever torn down, so timers were either global-forever or had to re-check currentView on every tick. The fan sensor poll did neither: it started at app launch and polled /proc every second for the life of the process, on any view, on a battery utility. It now starts when the fan view is shown and stops when it is left. Monitor gains the same treatment, and the home refresh interval is tracked in state -- beforeunload listed two of three timers while reading as though it were complete. Dialogs were plain divs toggled with style.display. The About dialog registered a fresh Escape listener on document every time it opened but removed it only inside the Escape branch, so closing via the X button or the overlay left it attached -- open it five times and five handlers fired on the next Escape. The permission dialog had no Escape handler at all, which made it impossible to dismiss from the keyboard. dialog.js replaces both with one implementation: role=dialog, aria-modal, a Tab trap, Escape on the capture phase, focus moved in on open and restored on close, and re-opening an already-open dialog returns the existing closer rather than stacking handlers. Also: aria-current on the active sidebar item, since a CSS class says nothing to assistive technology, and role/aria-live on the status banner, which meant every success and error message was previously unannounced. Verified by building the real packages and running the container launch test -- a broken registry would have left the app painting nothing. ci: the launch-test container was missing jq, so VERSION resolved empty and every glob became thinkutils__amd64.deb, which matches nothing. The run reported 'no artifact' while the real cause stayed hidden. jq is installed now, and an unreadable version fails loudly instead of producing a pattern that silently matches nothing. --- src/js/about.js | 31 +----- src/js/app.js | 41 +++---- src/js/dialog.js | 124 +++++++++++++++++++++ src/js/navigation.js | 154 ++++++++------------------- src/js/state.js | 3 +- src/js/utils.js | 6 ++ src/js/views/fan.js | 11 ++ src/js/views/monitor.js | 17 ++- src/js/views/registry.js | 125 ++++++++++++++++++++++ src/templates/views/battery.html | 5 - src/templates/views/mcp.html | 5 - src/templates/views/monitor.html | 5 - src/templates/views/performance.html | 5 - src/templates/views/security.html | 5 - src/templates/views/sync.html | 5 - src/templates/views/system.html | 5 - 16 files changed, 352 insertions(+), 195 deletions(-) create mode 100644 src/js/dialog.js create mode 100644 src/js/views/registry.js diff --git a/src/js/about.js b/src/js/about.js index aba61f6..589fb11 100755 --- a/src/js/about.js +++ b/src/js/about.js @@ -1,4 +1,5 @@ // About Dialog +import { openDialog, closeDialog } from './dialog.js'; export function setupAboutDialog() { const aboutLink = document.getElementById('about-link'); const closeAboutBtn = document.getElementById('close-about'); @@ -22,36 +23,12 @@ export function setupAboutDialog() { function showAbout() { console.log('[About] Opening dialog'); - const dialog = document.getElementById('about-dialog'); - if (dialog) { - dialog.style.display = 'flex'; - - if (!dialog.hasAttribute('data-listener')) { - dialog.setAttribute('data-listener', 'true'); - dialog.addEventListener('click', (e) => { - if (e.target === dialog) { - closeAbout(); - } - }); - } - - const escapeHandler = (e) => { - if (e.key === 'Escape') { - closeAbout(); - document.removeEventListener('keydown', escapeHandler); - } - }; - document.addEventListener('keydown', escapeHandler); - - setupAboutLinks(); - } + openDialog('about-dialog'); + setupAboutLinks(); } function closeAbout() { - const dialog = document.getElementById('about-dialog'); - if (dialog) { - dialog.style.display = 'none'; - } + closeDialog('about-dialog'); } function setupAboutLinks() { diff --git a/src/js/app.js b/src/js/app.js index 6e6ee59..8198915 100755 --- a/src/js/app.js +++ b/src/js/app.js @@ -27,13 +27,14 @@ window.addEventListener('unhandledrejection', (e) => { import { initializeElements } from './dom.js'; import { setupTitlebar } from './titlebar.js'; import { setupFeatureNavigation } from './navigation.js'; -import { setupFanControl, checkInitialPermissions, startAutoUpdate } from './views/fan.js'; +import { setupFanControl, checkInitialPermissions } from './views/fan.js'; import { setupHomeActions, updateHomeView } from './views/home.js'; import { setupSyncHandlers } from './views/sync.js'; import { setupBatteryHandlers } from './views/battery.js'; import { setupSecurityHandlers } from './views/security.js'; import { setupAboutDialog } from './about.js'; -import { state } from './state.js'; +import { openDialog, closeDialog } from './dialog.js'; +import { state, setState } from './state.js'; import { initializeSettings } from './settingsManager.js'; import { isModularMode, loadTemplates, injectTemplates } from './templateLoader.js'; @@ -65,17 +66,13 @@ async function checkAndSetupPermissions() { } function showPermissionDialog() { - const dialog = document.getElementById('permission-dialog'); - if (dialog) { - dialog.style.display = 'flex'; - } + // Was a bare style.display toggle with no Escape handler, which made this + // dialog impossible to dismiss from the keyboard. + openDialog('permission-dialog'); } function hidePermissionDialog() { - const dialog = document.getElementById('permission-dialog'); - if (dialog) { - dialog.style.display = 'none'; - } + closeDialog('permission-dialog'); } async function setupPermissions() { @@ -149,7 +146,11 @@ async function initializeApp() { setupSecurityHandlers(); setupAboutDialog(); setupPermissionDialog(); - startAutoUpdate(); + + // The fan sensor poll is NOT started here any more. It runs every second, and + // starting it at launch meant it polled /proc for the life of the app no + // matter which view was open. navigation.js starts it when the fan view is + // shown and stops it when the view is left. // Check all permissions at startup (sysfs + fan helper + polkit rule). // One dialog handles everything. After setup, re-check fan permissions. @@ -160,12 +161,14 @@ async function initializeApp() { console.log('[ThinkUtils] Loading settings...'); await initializeSettings(); - // Update home view periodically - setInterval(() => { + // Home refresh. Tracked in state so beforeunload can clear it -- this used to + // be an untracked setInterval that the cleanup handler claimed to cover. + const homeInterval = setInterval(() => { if (state.currentView === 'home') { updateHomeView(); } }, 2000); + setState('homeInterval', homeInterval); console.log('[ThinkUtils] Ready'); @@ -184,11 +187,13 @@ async function initializeApp() { window.addEventListener('DOMContentLoaded', initializeApp); +// Clear every tracked timer. The previous version listed two of the three and +// read as though it were complete. window.addEventListener('beforeunload', () => { - if (state.updateInterval) { - clearInterval(state.updateInterval); - } - if (state.monitorInterval) { - clearInterval(state.monitorInterval); + for (const key of ['updateInterval', 'monitorInterval', 'homeInterval']) { + if (state[key]) { + clearInterval(state[key]); + setState(key, null); + } } }); diff --git a/src/js/dialog.js b/src/js/dialog.js new file mode 100644 index 0000000..fd3a732 --- /dev/null +++ b/src/js/dialog.js @@ -0,0 +1,124 @@ +// Shared modal dialog behaviour. +// +// Both dialogs were plain divs toggled with style.display: no role, no focus +// management, and no consistent way to close them. The About dialog registered a +// fresh Escape listener on `document` every time it opened but only removed it +// inside the Escape branch, so closing via the X button or the overlay left the +// listener attached — open it five times and five handlers fired on the next +// Escape. The permission dialog had no Escape handler at all, which left it +// keyboard-inescapable. + +const openDialogs = new Map(); + +/** + * Show a dialog as a modal, and return a function that closes it. + * + * Focus moves into the dialog and is restored to whatever had it when the dialog + * closes — without that, dismissing a dialog drops keyboard users back at the + * top of the document. + */ +export function openDialog(dialogId, { onClose } = {}) { + const dialog = document.getElementById(dialogId); + if (!dialog) { + console.warn('[Dialog] No such dialog:', dialogId); + return () => {}; + } + + // Re-opening an already-open dialog must not stack a second set of handlers. + if (openDialogs.has(dialogId)) { + return openDialogs.get(dialogId); + } + + const previouslyFocused = document.activeElement; + + dialog.style.display = 'flex'; + dialog.setAttribute('role', 'dialog'); + dialog.setAttribute('aria-modal', 'true'); + dialog.removeAttribute('aria-hidden'); + + const focusable = () => + Array.from( + dialog.querySelectorAll( + 'button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"])' + ) + ).filter((el) => !el.disabled && el.offsetParent !== null); + + const onKeyDown = (e) => { + if (e.key === 'Escape') { + e.preventDefault(); + close(); + return; + } + + // Trap Tab inside the dialog. Without this, tabbing walks out into the page + // behind the overlay, where the user cannot see what is focused. + if (e.key !== 'Tab') { + return; + } + const items = focusable(); + if (items.length === 0) { + return; + } + const first = items[0]; + const last = items[items.length - 1]; + + if (e.shiftKey && document.activeElement === first) { + e.preventDefault(); + last.focus(); + } else if (!e.shiftKey && document.activeElement === last) { + e.preventDefault(); + first.focus(); + } + }; + + const onOverlayClick = (e) => { + if (e.target === dialog) { + close(); + } + }; + + function close() { + if (!openDialogs.has(dialogId)) { + return; + } + openDialogs.delete(dialogId); + + document.removeEventListener('keydown', onKeyDown, true); + dialog.removeEventListener('click', onOverlayClick); + + dialog.style.display = 'none'; + dialog.setAttribute('aria-hidden', 'true'); + dialog.removeAttribute('aria-modal'); + + if (previouslyFocused && typeof previouslyFocused.focus === 'function') { + previouslyFocused.focus(); + } + if (onClose) { + onClose(); + } + } + + // Capture phase so the dialog sees Escape before anything in the page can + // swallow it. + document.addEventListener('keydown', onKeyDown, true); + dialog.addEventListener('click', onOverlayClick); + + const initial = focusable()[0]; + if (initial) { + initial.focus(); + } + + openDialogs.set(dialogId, close); + return close; +} + +export function closeDialog(dialogId) { + const close = openDialogs.get(dialogId); + if (close) { + close(); + } +} + +export function isDialogOpen(dialogId) { + return openDialogs.has(dialogId); +} diff --git a/src/js/navigation.js b/src/js/navigation.js index c1c1226..15ea349 100755 --- a/src/js/navigation.js +++ b/src/js/navigation.js @@ -1,14 +1,7 @@ // Navigation and View Switching import { elements } from './dom.js'; -import { setState } from './state.js'; -import { updateHomeView } from './views/home.js'; -import { checkSyncStatus } from './views/sync.js'; -import { loadSystemInfo } from './views/system.js'; -import { loadBatteryInfo } from './views/battery.js'; -import { loadPerformanceInfo } from './views/performance.js'; -import { startMonitoring } from './views/monitor.js'; -import { loadSecurityStatus } from './views/security.js'; -import { loadMcpStatus, setupMcpView } from './views/mcp.js'; +import { setState, getState } from './state.js'; +import { VIEWS, getView, viewElement } from './views/registry.js'; export function setupFeatureNavigation() { const menuItems = document.querySelectorAll('.menu-item'); @@ -26,119 +19,62 @@ export function setupFeatureNavigation() { const feature = item.dataset.feature; console.log('[Navigation] Switching to:', feature); - menuItems.forEach((i) => i.classList.remove('active')); + menuItems.forEach((i) => { + i.classList.remove('active'); + i.removeAttribute('aria-current'); + }); item.classList.add('active'); + // Screen readers announce the active item only if it is marked as such; + // a CSS class alone says nothing to assistive technology. + item.setAttribute('aria-current', 'page'); switchView(feature); }); }); } -export function switchView(view) { - setState('currentView', view); - - // Hide all views - if (elements.homeView) { - elements.homeView.style.display = 'none'; - } - if (elements.fanView) { - elements.fanView.style.display = 'none'; - } - if (elements.syncView) { - elements.syncView.style.display = 'none'; - } - if (elements.systemView) { - elements.systemView.style.display = 'none'; - } - if (elements.batteryView) { - elements.batteryView.style.display = 'none'; +export function switchView(id) { + const next = getView(id); + if (!next) { + console.warn('[Navigation] Unknown view:', id); + return; } - if (elements.performanceView) { - elements.performanceView.style.display = 'none'; - } - if (elements.monitorView) { - elements.monitorView.style.display = 'none'; - } - if (elements.securityView) { - elements.securityView.style.display = 'none'; + + const previous = getView(getState('currentView')); + + // Tear down before building up. Without this every view's timers kept running + // for the life of the app -- three concurrent poll loops while sitting on one + // page, each rebuilding its DOM on every tick. + if (previous && previous.id !== next.id && previous.onHide) { + try { + previous.onHide(); + } catch (error) { + // A failing teardown must not block navigation, or the user is stuck. + console.error(`[Navigation] Failed to tear down ${previous.id}:`, error); + } } - if (elements.mcpView) { - elements.mcpView.style.display = 'none'; + + for (const view of VIEWS) { + const el = viewElement(view); + if (el) { + el.style.display = view.id === next.id ? view.display : 'none'; + } } - // Update page title - const titles = { - home: { title: 'Home', subtitle: 'Quick settings and overview' }, - fan: { title: 'Fan Control', subtitle: 'Manage cooling and fan speeds' }, - battery: { title: 'Battery', subtitle: 'Monitor and optimize battery health' }, - performance: { title: 'Performance', subtitle: 'Optimize CPU and power settings' }, - monitor: { title: 'System Monitor', subtitle: 'Real-time resource monitoring' }, - system: { title: 'System Info', subtitle: 'Your ThinkPad details' }, - sync: { title: 'Cloud Sync', subtitle: 'Sync settings across devices' }, - security: { title: 'Security', subtitle: 'Antivirus protection and security settings' }, - mcp: { title: 'AI Integration', subtitle: 'MCP server for AI assistants' } - }; + setState('currentView', next.id); - if (titles[view] && elements.pageTitle && elements.pageSubtitle) { - elements.pageTitle.textContent = titles[view].title; - elements.pageSubtitle.textContent = titles[view].subtitle; + if (elements.pageTitle) { + elements.pageTitle.textContent = next.title; + } + if (elements.pageSubtitle) { + elements.pageSubtitle.textContent = next.subtitle; } - // Show selected view - switch (view) { - case 'home': - if (elements.homeView) { - elements.homeView.style.display = 'block'; - updateHomeView(); - } - break; - case 'fan': - if (elements.fanView) { - elements.fanView.style.display = 'grid'; - } - break; - case 'sync': - if (elements.syncView) { - elements.syncView.style.display = 'block'; - checkSyncStatus(); - } - break; - case 'system': - if (elements.systemView) { - elements.systemView.style.display = 'block'; - loadSystemInfo(); - } - break; - case 'battery': - if (elements.batteryView) { - elements.batteryView.style.display = 'block'; - loadBatteryInfo(); - } - break; - case 'performance': - if (elements.performanceView) { - elements.performanceView.style.display = 'block'; - loadPerformanceInfo(); - } - break; - case 'monitor': - if (elements.monitorView) { - elements.monitorView.style.display = 'block'; - startMonitoring(); - } - break; - case 'security': - if (elements.securityView) { - elements.securityView.style.display = 'block'; - loadSecurityStatus(); - } - break; - case 'mcp': - if (elements.mcpView) { - elements.mcpView.style.display = 'block'; - setupMcpView(); - loadMcpStatus(); - } - break; + if (next.onShow) { + try { + next.onShow(); + } catch (error) { + console.error(`[Navigation] Failed to initialise ${next.id}:`, error); + } } } diff --git a/src/js/state.js b/src/js/state.js index 9f203cd..9d2057a 100755 --- a/src/js/state.js +++ b/src/js/state.js @@ -5,7 +5,8 @@ export const state = { fanControlInProgress: false, lastFanSpeedSet: null, currentView: 'home', - monitorInterval: null + monitorInterval: null, + homeInterval: null }; export function setState(key, value) { diff --git a/src/js/utils.js b/src/js/utils.js index a8bcae6..610a8be 100755 --- a/src/js/utils.js +++ b/src/js/utils.js @@ -21,6 +21,12 @@ export function showStatus(message, type = 'info') { document.body.appendChild(statusEl); } + // Without a live region this banner is invisible to screen readers, so every + // success and failure message went unannounced. Errors are assertive because + // the action failed and the user needs to know now; the rest are polite. + statusEl.setAttribute('role', 'status'); + statusEl.setAttribute('aria-live', type === 'error' ? 'assertive' : 'polite'); + statusEl.textContent = message; const colors = { diff --git a/src/js/views/fan.js b/src/js/views/fan.js index a98f11d..d20ec57 100755 --- a/src/js/views/fan.js +++ b/src/js/views/fan.js @@ -312,7 +312,18 @@ async function tryUpdatePermissions() { } export function startAutoUpdate() { + // Guard against double-start: switching to the fan view twice without a hide + // in between would otherwise leak a second interval polling the same files. + stopAutoUpdate(); updateSensorData(); const interval = setInterval(updateSensorData, 1000); setState('updateInterval', interval); } + +export function stopAutoUpdate() { + const interval = getState('updateInterval'); + if (interval) { + clearInterval(interval); + setState('updateInterval', null); + } +} diff --git a/src/js/views/monitor.js b/src/js/views/monitor.js index 0157192..141a387 100755 --- a/src/js/views/monitor.js +++ b/src/js/views/monitor.js @@ -4,13 +4,12 @@ const { invoke } = window.__TAURI__.core; import { setState, getState } from '../state.js'; export async function startMonitoring() { + stopMonitoring(); await updateMonitorData(); - const interval = getState('monitorInterval'); - if (interval) { - clearInterval(interval); - } - + // The currentView check inside the tick is no longer load-bearing now that + // navigation stops this on hide, but it costs nothing and keeps the interval + // harmless if it ever outlives its view again. const newInterval = setInterval(async () => { if (getState('currentView') === 'monitor') { await updateMonitorData(); @@ -170,3 +169,11 @@ function displayProcessMonitor(processes) { container.appendChild(procDiv); }); } + +export function stopMonitoring() { + const interval = getState('monitorInterval'); + if (interval) { + clearInterval(interval); + setState('monitorInterval', null); + } +} diff --git a/src/js/views/registry.js b/src/js/views/registry.js new file mode 100644 index 0000000..7972777 --- /dev/null +++ b/src/js/views/registry.js @@ -0,0 +1,125 @@ +// View registry — one place that knows what a view is called, where its element +// lives, and what to start and stop when it becomes visible. +// +// Before this, navigation.js held a 9-branch hide block, a separate titles map, +// and a 9-case show switch. The titles map and the per-view templates had +// already drifted: the MCP subtitle differed between them. +// +// It also had no concept of hiding. Nothing was ever torn down, so every timer +// was either global-forever or had to re-check `currentView` on each tick. The +// fan sensor poll did neither and ran every second for the life of the app, +// on a battery utility. + +import { elements } from '../dom.js'; +import { updateHomeView } from './home.js'; +import { checkSyncStatus } from './sync.js'; +import { loadSystemInfo } from './system.js'; +import { loadBatteryInfo } from './battery.js'; +import { loadPerformanceInfo } from './performance.js'; +import { startMonitoring, stopMonitoring } from './monitor.js'; +import { loadSecurityStatus } from './security.js'; +import { loadMcpStatus, setupMcpView } from './mcp.js'; +import { startAutoUpdate, stopAutoUpdate } from './fan.js'; + +/** + * Every view, in sidebar order. + * + * `title` and `subtitle` are the single source of truth — the page header reads + * them, and view templates must not repeat them. + * + * `display` matters: most views are `block`, but the fan view is a grid and + * would collapse if shown as a block. + * + * `onShow` runs when the view becomes visible; `onHide` when it is replaced. + * A view that starts a timer must stop it in `onHide`. + */ +export const VIEWS = [ + { + id: 'home', + title: 'Home', + subtitle: 'Quick settings and overview', + element: 'homeView', + display: 'block', + onShow: updateHomeView + }, + { + id: 'fan', + title: 'Fan Control', + subtitle: 'Manage cooling and fan speeds', + element: 'fanView', + display: 'grid', + // Polls sensors every second. It used to start once at app launch and never + // stop, so it kept polling /proc while the user sat on any other view. + onShow: startAutoUpdate, + onHide: stopAutoUpdate + }, + { + id: 'battery', + title: 'Battery', + subtitle: 'Monitor and optimize battery health', + element: 'batteryView', + display: 'block', + onShow: loadBatteryInfo + }, + { + id: 'performance', + title: 'Performance', + subtitle: 'Optimize CPU and power settings', + element: 'performanceView', + display: 'block', + onShow: loadPerformanceInfo + }, + { + id: 'monitor', + title: 'System Monitor', + subtitle: 'Real-time resource monitoring', + element: 'monitorView', + display: 'block', + onShow: startMonitoring, + onHide: stopMonitoring + }, + { + id: 'system', + title: 'System Info', + subtitle: 'Your ThinkPad details', + element: 'systemView', + display: 'block', + onShow: loadSystemInfo + }, + { + id: 'security', + title: 'Security', + subtitle: 'Antivirus protection and security settings', + element: 'securityView', + display: 'block', + onShow: loadSecurityStatus + }, + { + id: 'mcp', + title: 'AI Integration', + subtitle: 'Connect AI assistants to your ThinkPad via MCP', + element: 'mcpView', + display: 'block', + onShow: () => { + setupMcpView(); + loadMcpStatus(); + } + }, + { + id: 'sync', + title: 'Cloud Sync', + subtitle: 'Sync settings across devices', + element: 'syncView', + display: 'block', + onShow: checkSyncStatus + } +]; + +export function getView(id) { + return VIEWS.find((v) => v.id === id) ?? null; +} + +/** The DOM element for a view, or null when templates failed to inject. */ +export function viewElement(view) { + return elements[view.element] ?? null; +} diff --git a/src/templates/views/battery.html b/src/templates/views/battery.html index eb75bf2..81efdab 100755 --- a/src/templates/views/battery.html +++ b/src/templates/views/battery.html @@ -1,8 +1,3 @@ -

-

Battery Management

-

Monitor and optimize battery health

-
-
diff --git a/src/templates/views/mcp.html b/src/templates/views/mcp.html index 87496de..fcf720d 100644 --- a/src/templates/views/mcp.html +++ b/src/templates/views/mcp.html @@ -1,8 +1,3 @@ -
-

AI Integration

-

Connect AI assistants to your ThinkPad via MCP

-
-
diff --git a/src/templates/views/monitor.html b/src/templates/views/monitor.html index c675fe5..b4dfa8a 100755 --- a/src/templates/views/monitor.html +++ b/src/templates/views/monitor.html @@ -1,8 +1,3 @@ -
-

System Monitor

-

Real-time resource monitoring

-
-
diff --git a/src/templates/views/performance.html b/src/templates/views/performance.html index 64df1f5..29a257c 100755 --- a/src/templates/views/performance.html +++ b/src/templates/views/performance.html @@ -1,8 +1,3 @@ -
-

Performance Settings

-

Optimize CPU and power settings

-
-
diff --git a/src/templates/views/security.html b/src/templates/views/security.html index e345487..7806f01 100755 --- a/src/templates/views/security.html +++ b/src/templates/views/security.html @@ -1,8 +1,3 @@ -
-

Security

-

Antivirus protection and security settings

-
-
diff --git a/src/templates/views/sync.html b/src/templates/views/sync.html index ffcc139..2ce6b3c 100755 --- a/src/templates/views/sync.html +++ b/src/templates/views/sync.html @@ -1,8 +1,3 @@ -
-

Cloud Sync

-

Sync settings across devices

-
-