refactor(ui) + security: view lifecycle, one implementation per control, and the pkexec/XSS fixes

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,12 +124,12 @@ 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).
 :::
 
 ### 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`.

scripts/test-gui-packages-docker.sh

@@ -237,6 +237,15 @@ read -r -d '' VERDICT <<'VEOF' || true
   else
     echo "FAIL: frontend never signalled ready - JS init did not complete"; fail=1
   fi
+  # Reaching "ready" no longer means every view wired up: each setup step is
+  # isolated so one failure cannot abort the boot. That is the right behaviour
+  # for users and a blind spot for this test, since a partially wired app paints
+  # exactly like a working one.
+  if grep -q "\[thinkutils\] frontend init had" /tmp/app.log; then
+    echo "FAIL: frontend booted with failing setup step(s):"
+    grep "frontend init had" /tmp/app.log | head -5 | sed "s/^/    /"
+    fail=1
+  fi
   # The check that catches a view dying on a missing sysfs path while the sidebar
   # still paints and the process still lives.
   if grep -q "\[thinkutils\] frontend error:" /tmp/app.log; then

src-tauri/src/auth.rs

@@ -1,5 +1,4 @@
 use serde::{Deserialize, Serialize};
-use std::fs;
 
 #[derive(Debug, Serialize, Deserialize)]
 pub struct ApiResponse<T> {
@@ -15,32 +14,11 @@ pub async fn authenticate_once() -> ApiResponse<String> {
     // 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<String> {
             }
         }
         Err(e) => {
-            let _ = fs::remove_file(temp_script);
             println!("[Auth] ✗ Failed to execute pkexec: {}", e);
             ApiResponse {
                 success: false,

src-tauri/src/battery.rs

@@ -117,10 +117,54 @@ fn read_battery_info(path: &str, index: usize) -> Result<BatteryInfo, String> {
     })
 }
 
+/// 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<BatteryThresholds> {
-    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<String>
         };
     }
 
-    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
@@ -203,36 +257,13 @@ pub async fn set_battery_thresholds(start: u8, stop: u8) -> ApiResponse<String>
     }
 
     // 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 +278,11 @@ pub async fn set_battery_thresholds(start: u8, stop: u8) -> ApiResponse<String>
                 }
             }
         }
-        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)),
+        },
     }
 }
 
@@ -351,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
+            );
+        }
+    }
+}