From 9e95659b677dc0abe6b885682fcb656ec8bab4ec Mon Sep 17 00:00:00 2001 From: Viet Anh Nguyen Date: Sun, 19 Jul 2026 14:23:19 +0700 Subject: [PATCH 1/9] test: launch-test the real packages in clean containers Nothing proved the packages RUN. 'npm run tauri build exited 0' and 'the .deb is 8MB' are both true of a binary that dies before it draws a window -- and this app builds a tray icon unconditionally against libayatana-appindicator3, the classic dlopen-panic shape. The hard part is that ThinkUtils reads /proc/acpi/ibm/fan and /sys/class/power_supply/BAT*, none of which exist in a container. So the suite does not test hardware paths. It tests that the app starts, renders its full UI, and degrades cleanly when the hardware is absent, using three hardware-independent signals: index.html ships only two visible strings plus empty containers. Every other word on screen arrives because templateLoader.js fetched a template over tauri:// and injected it. Those labels are literal markup that no /proc or /sys read produces, so OCR finding them proves the JS ran, on any machine. OCR finding index.html's static text while finding NO injected label is the exact signature of 'WebKit loaded the page, the JS died' -- checked explicitly, because every other assertion passes in that state. The app now prints hw probe / hw mode, so 'no ThinkPad here' is an observed state rather than an inference. The frontend reports uncaught exceptions to the backend. A view dying on an absent sysfs path leaves the sidebar painted and the process alive; that error line is the only tell. Running it immediately found two real bugs, both fixed here: hw mode reported 'full' inside a container. Containers inherit the host's /sys, so bat0 and cpufreq were present and only the fan interface was missing. Keyed on the fan interface now -- a battery and cpufreq exist on every Linux laptop and prove nothing about ThinkPad support. The OCR assertion missed the first-run permissions dialog. On a machine that has never been set up -- every container -- the app correctly opens that dialog over the main view, so the sidebar is not what is on screen. It comes from templates/dialogs.html, so it is injected template text and proves the same thing. Verified locally against real builds: deb on ubuntu:24.04 PASSES, and rpm on fedora:41 PASSES -- the first time the .rpm has ever been tested by anything. Artifact selection resolves the CURRENT version rather than taking the only match or the first: three stale builds were sitting in the bundle directory, and release.yml selects with 'ls *.deb | head -n 1', which sorts 0.1.10 before 0.1.5 and would rename an old package to the new version's name and publish it. Also fixes .gitignore, where a missing newline had merged two entries into 'docs/.vitepress/cachebuild/' -- so neither the VitePress cache nor build output was ignored. --- .github/workflows/ci.yml | 58 ++++ .gitignore | 5 +- scripts/test-gui-packages-docker.sh | 480 ++++++++++++++++++++++++++++ src-tauri/src/lib.rs | 71 ++++ src/js/app.js | 38 +++ 5 files changed, 651 insertions(+), 1 deletion(-) create mode 100755 scripts/test-gui-packages-docker.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 52ba497..26c4617 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -301,6 +301,64 @@ jobs: working-directory: src-tauri run: cargo test --test hardware_profiles + # Everything above proves the code COMPILES and LINTS. Not one job has ever + # started the application. A Tauri binary can pass every check in this file and + # still die on launch with a dlopen panic for a library the .deb forgot to + # depend on, or come up showing an empty window because the frontend never + # mounted. The only assertion that catches that is installing the real package + # and looking at the real pixels. + # + # It runs in a container with no ThinkPad hardware, which is the point: it + # proves the app starts, renders its full UI, and degrades cleanly when + # /proc/acpi/ibm/fan is absent -- the environment a bug is most likely to hide + # in. See the header of the script for how it tells those states apart. + gui-launch: + needs: [rust, frontend] + runs-on: ubuntu-24.04 + timeout-minutes: 45 + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-node@v4 + with: + node-version: 20 + cache: npm + cache-dependency-path: package-lock.json + - uses: dtolnay/rust-toolchain@stable + - uses: Swatinem/rust-cache@v2 + with: + workspaces: src-tauri + + - name: Install Tauri build dependencies + run: | + sudo apt-get update + sudo apt-get install -y --no-install-recommends \ + libwebkit2gtk-4.1-dev libgtk-3-dev libayatana-appindicator3-dev \ + librsvg2-dev patchelf build-essential file libssl-dev rpm + + - run: npm ci + + # Built here rather than downloaded, so a PR is tested before anything is + # ever published. + - name: Build the real packages + run: npm run tauri build + + - name: List what was built + run: find src-tauri/target/release/bundle -type f \( -name '*.deb' -o -name '*.rpm' -o -name '*.AppImage' \) -print + + - name: Launch-test the packages in clean containers + run: scripts/test-gui-packages-docker.sh + + # A red run should come with the actual picture of the broken window, not + # just whatever reached stdout before the container was discarded. + - name: Upload screenshots, OCR text and app logs + if: always() + uses: actions/upload-artifact@v4 + with: + name: gui-launch-evidence + path: build/gui-test-out/ + if-no-files-found: warn + retention-days: 14 + # A full VitePress production build, not a link check. VitePress compiles every # page as a Vue SFC, so a literal {{ }} in prose parses as an interpolation and # fails the build - and only the production build catches it. diff --git a/.gitignore b/.gitignore index 23a5178..64abb67 100755 --- a/.gitignore +++ b/.gitignore @@ -26,4 +26,7 @@ dist-ssr # VitePress docs/.vitepress/dist -docs/.vitepress/cache \ No newline at end of file +docs/.vitepress/cache + +# GUI launch-test output (screenshots, OCR text, app logs) +build/ diff --git a/scripts/test-gui-packages-docker.sh b/scripts/test-gui-packages-docker.sh new file mode 100755 index 0000000..f1a6b51 --- /dev/null +++ b/scripts/test-gui-packages-docker.sh @@ -0,0 +1,480 @@ +#!/usr/bin/env bash +# +# Launch-test the built packages in clean containers. +# +# Why this exists: CI proves the packages BUILD. Nothing has ever proved they +# RUN. "npm run tauri build exited 0" and "the .deb is 8MB" are both true of a +# binary that dies before it draws a window -- and this app builds a tray icon +# unconditionally against libayatana-appindicator3, which is exactly the +# dlopen-panic shape that has shipped broken in comparable projects. +# +# THE HARDWARE PROBLEM +# -------------------- +# ThinkUtils reads /proc/acpi/ibm/fan and /sys/class/power_supply/BAT*. None of +# those exist in a container and none of them can. So this script does not try to +# test the hardware paths. It tests the thing that is currently untested and far +# more likely to break a release: that the app STARTS, RENDERS ITS FULL UI, and +# DEGRADES CLEANLY when the hardware is absent -- rather than crashing, hanging, +# or painting an empty window. +# +# It draws that distinction three ways, none of which touch hardware: +# +# (a) The assertion surface is hardware-independent by construction. +# src/index.html ships exactly two visible strings ("Home" and "Quick +# settings and overview") plus four EMPTY containers. Every other word on +# screen -- the sidebar labels, the section headings -- appears only because +# templateLoader.js fetched a template over tauri:// and injected it. Those +# labels are literal template markup; no /proc or /sys read produces them. +# So OCR finding "Fan Control" proves the JS ran, on any machine. And OCR +# finding "Home" while finding NO sidebar label is the exact signature of +# "WebKit loaded the page, the JS died" -- checked explicitly below, because +# every other assertion in this file passes in that state. +# +# (b) The app declares its own hardware state. It prints "hw probe:" naming each +# path it found and "hw mode:". In a container the correct, PASSING answer +# is "degraded". An app claiming "full" in a container is detecting hardware +# that is not there, which is reported as a warning. +# +# (c) The frontend reports uncaught exceptions to the backend, which prints +# them. This is the one that matters most: a view dying on an absent sysfs +# path leaves the sidebar painted and the process alive, and that error line +# is the only tell. +# +# Usage: +# scripts/test-gui-packages-docker.sh # every target +# scripts/test-gui-packages-docker.sh deb:debian:12 # one target +# SETTLE=15 scripts/test-gui-packages-docker.sh +# +# Approach adapted from the sibling Bulwark project (Apache-2.0). + +set -uo pipefail + +REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +ASSETS="${ASSETS:-${REPO_ROOT}/src-tauri/target/release/bundle}" +OUTDIR="${OUTDIR:-${REPO_ROOT}/build/gui-test-out}" +READY_TIMEOUT="${READY_TIMEOUT:-60}" +SETTLE="${SETTLE:-5}" +STAGE="$(mktemp -d)" +trap 'rm -rf "${STAGE}"' EXIT + +mkdir -p "${OUTDIR}" + +# ThinkPads are x86_64. There is deliberately no arm64 matrix and no qemu: +# emulating a WebKit GUI is slow and fails on graphics paths no user touches, so +# a red result would be uninformative rather than useful. +ARCH="$(uname -m)" +if [ "${ARCH}" != "x86_64" ]; then + echo "ERROR: this suite is x86_64-only (got ${ARCH})" >&2 + exit 1 +fi + +shopt -s nullglob + +# The version currently declared, so the right artifact is picked out of a bundle +# directory that accumulates old builds. Resolving by glob alone would silently +# test whatever sorted first -- and `ls | head -1` sorts "0.1.10" before "0.1.5", +# so "newest" and "first" are not the same thing. +VERSION="$(jq -r .version "${REPO_ROOT}/package.json")" + +# Resolve the artifact for the CURRENT version. Never a hardcoded version (it +# would stop matching and silently test nothing) and never just "the only one" +# (stale builds accumulate locally, and a fresh CI checkout hides that). +stage_one() { + local pattern="$1" + local versioned="${pattern//VERSION/${VERSION}}" + local matches=("${ASSETS}"/*/${versioned}) + + if [ "${#matches[@]}" -eq 0 ]; then + echo "ERROR: no artifact matching '${versioned}' under ${ASSETS}" >&2 + echo " build first: npm run tauri build" >&2 + local any=("${ASSETS}"/*/${pattern//VERSION/*}) + if [ "${#any[@]}" -gt 0 ]; then + echo " found these other versions instead:" >&2 + printf ' %s\n' "${any[@]}" >&2 + fi + return 1 + fi + if [ "${#matches[@]}" -gt 1 ]; then + echo "ERROR: ${#matches[@]} artifacts match '${versioned}' - cannot choose" >&2 + printf ' %s\n' "${matches[@]}" >&2 + return 1 + fi + + # Stale artifacts are not fatal here, but they are worth saying out loud: + # release.yml selects with `ls *.deb | head -n 1`, which would rename an old + # package to the new version's name and publish it. + local all=("${ASSETS}"/*/${pattern//VERSION/*}) + if [ "${#all[@]}" -gt 1 ]; then + echo " note: ${#all[@]} builds present, testing v${VERSION}. Stale artifacts:" >&2 + for f in "${all[@]}"; do + [ "$f" = "${matches[0]}" ] || echo " $(basename "$f")" >&2 + done + fi + + cp "${matches[0]}" "${STAGE}/" + basename "${matches[0]}" +} + +TARGETS=("$@") +if [ "${#TARGETS[@]}" -eq 0 ]; then + TARGETS=( + deb:ubuntu:22.04 # the build floor -- release.yml builds here, and it + # is the oldest release carrying libwebkit2gtk-4.1 + deb:ubuntu:24.04 # current LTS, where most users are + deb:debian:12 # non-Ubuntu Debian: different WebKit and GTK point + # revisions, different appindicator packaging + rpm:fedora:41 # the ONLY coverage the .rpm has ever had. It is + # bundled by Tauri running on Ubuntu, so its + # generated Requires: come from a Debian view of + # the world. Pinned, not :latest -- a Fedora rebase + # must not turn CI red for unrelated reasons. + appimage:ubuntu:22.04 # an AppImage is a portability claim; test it on the + # oldest supported base where a missing bundled + # library actually surfaces + ) +fi + +# Xvfb plus software rendering. Containers have no GPU, and WebKitGTK's DMA-BUF +# renderer fails without one. These are harness settings, not app requirements. +# +# xcompmgr is here for a ThinkUtils-specific reason: tauri.conf.json sets +# transparent:true and decorations:false. An ARGB window with no compositing +# manager has undefined content behind it under Xvfb, which makes the colour +# count and OCR read garbage. If it is unavailable the run continues and the +# pixel checks degrade to warnings rather than lying. +read -r -d '' GUI_ENV <<'ENVEOF' || true + for t in Xvfb import identify compare xdotool pgrep tesseract convert; do + command -v "$t" >/dev/null || { + echo "HARNESS ERROR: $t missing - test dependencies failed to install." + echo "This is a broken test environment, NOT an application failure." + exit 90; } + done + export DISPLAY=:99 + export WEBKIT_DISABLE_COMPOSITING_MODE=1 + export WEBKIT_DISABLE_DMABUF_RENDERER=1 + export LIBGL_ALWAYS_SOFTWARE=1 + export GDK_BACKEND=x11 + export NO_AT_BRIDGE=1 + Xvfb :99 -screen 0 1280x800x24 >/dev/null 2>&1 & + # Poll for the X socket rather than sleeping a guessed interval. + for i in $(seq 1 30); do [ -e /tmp/.X11-unix/X99 ] && break; sleep 0.5; done + [ -e /tmp/.X11-unix/X99 ] || { echo "HARNESS ERROR: Xvfb never came up"; exit 90; } + command -v xcompmgr >/dev/null && { xcompmgr -a >/dev/null 2>&1 & sleep 1; } + # Baseline of the EMPTY display, captured before launch. The colour count alone + # cannot tell "the app painted" from "Xvfb has a noisy root"; this can, and it + # stays valid if the UI is restyled. + import -window root /tmp/baseline.png 2>/dev/null || true +ENVEOF + +# Wait for the readiness marker instead of sleeping a guessed interval. A flat +# sleep is slower than needed on a fast runner and flaky on a slow one. +read -r -d '' WAIT_READY <<'WEOF' || true + for i in $(seq 1 __READY_TIMEOUT__); do + grep -q "\[thinkutils\] frontend ready:" /tmp/app.log 2>/dev/null && break + kill -0 $APP_PID 2>/dev/null || break + sleep 1 + done + sleep __SETTLE__ +WEOF +WAIT_READY="${WAIT_READY//__READY_TIMEOUT__/${READY_TIMEOUT}}" +WAIT_READY="${WAIT_READY//__SETTLE__/${SETTLE}}" + +# Package installs are retried: mirror hiccups are the single most common cause +# of a red run that has nothing to do with the code. +read -r -d '' RETRY <<'REOF' || true + retry() { for i in 1 2 3; do "$@" && return 0; echo " (attempt $i failed, retrying)"; sleep 5; done; return 1; } +REOF + +read -r -d '' VERDICT <<'VEOF' || true + echo "----- app output -----"; cat /tmp/app.log; echo "----------------------" + fail=0 + cp /tmp/app.log /out/app.log 2>/dev/null || true + + # -------------------------------------------------------------- crashes + grep -qi "panicked" /tmp/app.log && { echo "FAIL: panicked on launch"; fail=1; } + + # Failures that neither panic nor kill the process: the app keeps running and + # the user gets a broken window. The appindicator entry is not hypothetical -- + # this app builds a tray icon unconditionally. + for sig in "undefined symbol" "cannot open shared object" "Segmentation fault" \ + "Failed to load module" "WebKitWebProcess.*crashed" "Exec format error"; do + grep -qE "${sig}" /tmp/app.log && { + echo "FAIL: log contains a known-bad signature: ${sig}"; fail=1; } + done + + # ------------------------------------------------ must be a RELEASE build + grep -qE "webview url: https?://" /tmp/app.log && { + echo "FAIL: DEV build - the UI loads over http, not the embedded frontend"; fail=1; } + grep -q "webview url: tauri://" /tmp/app.log || { + echo "FAIL: webview did not load the embedded frontend"; fail=1; } + + # ----------------------------------------------------- hardware, honestly + # The probe line existing at all proves the detection path RAN and returned + # rather than panicking on a missing file. + if grep -q "\[thinkutils\] hw probe:" /tmp/app.log; then + echo "OK: $(grep -m1 'hw probe:' /tmp/app.log)" + else + echo "FAIL: no hardware probe line - detection never completed"; fail=1 + fi + if grep -q "hw mode: degraded" /tmp/app.log; then + echo "OK: correctly reports degraded mode (no ThinkPad hardware in a container)" + elif grep -q "hw mode: full" /tmp/app.log; then + echo "WARN: reports FULL hardware mode inside a container - detection is" + echo " producing false positives. Not failing this suite, but it is a bug." + fi + + # ------------------------------------------- the frontend finished booting + if grep -q "\[thinkutils\] frontend ready:" /tmp/app.log; then + echo "OK: $(grep -m1 'frontend ready:' /tmp/app.log)" + else + echo "FAIL: frontend never signalled ready - JS init did not complete"; 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 + echo "FAIL: uncaught frontend exception(s):" + grep "frontend error:" /tmp/app.log | head -10 | sed "s/^/ /" + fail=1 + fi + + kill -0 $APP_PID 2>/dev/null || { echo "FAIL: process died during settle"; fail=1; } + + # -------------------------------------------------- the web engine started + if pgrep -f "WebKitWebProcess" >/dev/null 2>&1; then + echo "OK: WebKitWebProcess is running" + else + echo "FAIL: no WebKitWebProcess - the webview never started"; fail=1 + fi + + # --------------------------------------------------------- a real window + # Pick the LARGEST match, never the first: there is no window manager, and GTK + # maps small helper windows carrying the same name. + WIDS="$(xdotool search --name "ThinkUtils" 2>/dev/null || true)" + # decorations:false means GTK, not a WM, owns the title. Fall back to all + # visible windows so a naming change degrades rather than falsely failing. + [ -z "${WIDS}" ] && WIDS="$(xdotool search --onlyvisible --name "." 2>/dev/null || true)" + WID=""; BESTA=0; W=0; H=0 + for w in ${WIDS}; do + GW="$(xdotool getwindowgeometry --shell "${w}" 2>/dev/null | sed -n "s/^WIDTH=//p")" + GH="$(xdotool getwindowgeometry --shell "${w}" 2>/dev/null | sed -n "s/^HEIGHT=//p")" + [ -n "${GW}" ] && [ -n "${GH}" ] || continue + A=$(( GW * GH )) + echo " candidate window ${w}: ${GW}x${GH}" + [ "${A}" -gt "${BESTA}" ] && { BESTA="${A}"; WID="${w}"; W="${GW}"; H="${GH}"; } + done + if [ -n "${WID}" ]; then + echo "window: id=${WID} ${W}x${H}" + # tauri.conf.json declares 1200x700 with minimums of 700x600. Assert well + # below the minimum so a legitimate size change never breaks CI, but 0x0 does. + if [ "${W}" -lt 600 ] || [ "${H}" -lt 400 ]; then + echo "FAIL: largest window is only ${W}x${H} - too small to be usable"; fail=1; fi + else + echo "FAIL: no window is mapped on the display"; fail=1 + fi + + # ---------------------------------------------------------------- pixels + # Capture the ROOT and crop rather than shooting the window: the window is ARGB + # (transparent:true), and import -window on an ARGB drawable without a + # compositor returns unreliable alpha. The root is always plain RGB. + import -window root /tmp/shot_root.png 2>/dev/null || true + if [ -n "${WID}" ] && [ -f /tmp/shot_root.png ]; then + X="$(xdotool getwindowgeometry --shell "${WID}" | sed -n "s/^X=//p")" + Y="$(xdotool getwindowgeometry --shell "${WID}" | sed -n "s/^Y=//p")" + convert /tmp/shot_root.png -crop "${W}x${H}+${X:-0}+${Y:-0}" +repage /tmp/shot.png 2>/dev/null \ + || cp /tmp/shot_root.png /tmp/shot.png + else + cp /tmp/shot_root.png /tmp/shot.png 2>/dev/null || true + fi + cp /tmp/shot.png /out/shot.png 2>/dev/null || true + cp /tmp/shot_root.png /out/shot_root.png 2>/dev/null || true + + if [ -f /tmp/shot.png ]; then + colors=$(identify -format "%k" /tmp/shot.png 2>/dev/null || echo 0) + echo "distinct colours on screen: ${colors}" + # Dark theme (--bg-primary #1a1a1a on --text-primary #fff). Antialiased white + # on near-black still yields hundreds of greys, so this threshold fails only + # on genuinely empty output, never on a restyle. + [ "${colors:-0}" -lt 40 ] && { + echo "FAIL: window appears blank (${colors} colours)"; fail=1; } + + # The screen actually CHANGED versus the pre-launch capture. Compared + # root-to-root on purpose: compare ERRORS on differing dimensions rather than + # reporting a difference, and that error would read as "not zero" and pass. + if [ -f /tmp/baseline.png ] && [ -f /tmp/shot_root.png ]; then + RAW="$(compare -metric AE /tmp/baseline.png /tmp/shot_root.png null: 2>&1 || true)" + # compare prints "1023970 (0.999969)" and uses scientific notation for large + # counts. Take the first field and compare in awk, which handles 5.4e+10. + DIFF="${RAW%% *}" + echo "pixels changed vs the pre-launch display: ${RAW}" + if echo "${DIFF}" | grep -qE "^[0-9]+([.][0-9]+)?([eE][+-]?[0-9]+)?$"; then + awk -v d="${DIFF}" "BEGIN{exit !(d>0)}" \ + && echo "OK: ${DIFF} pixels changed after launch" \ + || { echo "FAIL: display is pixel-identical to before launch"; fail=1; } + else + echo "WARN: could not measure the pre/post difference (${RAW})" + fi + fi + + # ------------------------------------------------------------------ OCR + # Preprocessing is mandatory here. This UI is white text on #1a1a1a, and + # tesseract is trained on dark-on-light. So: greyscale, invert, upscale (small + # UI text is below tesseract's comfortable x-height at 1280x800), sharpen. + # Both the inverted and raw images are read and their output unioned, so a + # future light theme still works without touching this script. + convert /tmp/shot.png -colorspace Gray -negate -resize 200% -sharpen 0x1 \ + /tmp/ocr_in.png 2>/dev/null || true + tesseract /tmp/ocr_in.png /tmp/ocr_inv --psm 6 >/dev/null 2>&1 || true + tesseract /tmp/shot.png /tmp/ocr_raw --psm 6 >/dev/null 2>&1 || true + OCRTXT="$(cat /tmp/ocr_inv.txt /tmp/ocr_raw.txt 2>/dev/null || echo "")" + printf '%s' "${OCRTXT}" > /out/ocr.txt 2>/dev/null || true + echo "----- OCR -----"; echo "${OCRTXT}" | grep -v "^[[:space:]]*$" | head -30; echo "---------------" + + # WHY THESE STRINGS: every label below lives in src/templates/, and reaches + # the screen only if templateLoader.js fetched it over tauri:// and injected + # it. They are literal markup -- not one is produced by reading /proc or /sys. + # So they prove the JS ran AND are hardware-independent, which is exactly what + # makes this test meaningful in a container with no ThinkPad in it. + # + # Deliberately lenient about WHICH labels: OCR on a software-rendered + # screenshot is not reliable enough to demand a specific string, and pinning + # one would make every copy change a release blocker. + # Includes the first-run permissions dialog. On a machine that has never been + # set up -- which every container is -- the app correctly opens that dialog + # over the main view, so the sidebar is not what OCR sees. The dialog lives in + # templates/dialogs.html, so it is injected-template text too and proves the + # same thing. The first run of this test failed on exactly that. + LABELS="Fan Control|Battery|Performance|Monitor|System Info|Security|AI Integration|Sync|About|System Overview|Quick Settings|Power Profile|CPU Governor|Turbo Boost|Permissions Required|One-time setup|WHAT WILL BE CONFIGURED|Fan speed control|Battery charge thresholds" + HITS="$(echo "${OCRTXT}" | grep -oiE "${LABELS}" | tr "[:upper:]" "[:lower:]" | sort -u)" + NHITS="$(echo "${HITS}" | grep -c . || true)" + echo "injected-template labels recognised (${NHITS}): $(echo ${HITS} | tr '\n' ' ')" + + if [ "${NHITS:-0}" -ge 3 ]; then + echo "OK: the frontend fetched, injected and painted its templates" + else + # The specific, nameable failure: WebKit rendered index.html's own static + # text but templateLoader never ran. Every other check above passes here. + if echo "${OCRTXT}" | grep -qiE "quick settings and overview"; then + echo "FAIL: only index.html's STATIC text is on screen - templateLoader.js" + echo " never injected the sidebar or views. WebKit loaded the page;" + echo " the JS did not run." + else + NALNUM="$(echo "${OCRTXT}" | tr -cd "[:alnum:]" | wc -c)" + echo "FAIL: no injected UI text recognised (${NALNUM} alphanumeric chars)" + echo " see shot.png and ocr.txt in the uploaded artifacts" + fi + fail=1 + fi + else + echo "WARN: could not capture a screenshot; rendering not verified" + fi + + # ------------------------------------------------------- clean shutdown + # A crash on teardown is still a crash the user meets every time they close the + # window, and nothing above would reach it. This app runs a fan-curve background + # task and an optional MCP server, both with teardown paths worth exercising -- + # and the fan curve now restores the fan to auto on exit. + kill -TERM $APP_PID 2>/dev/null || true + for i in $(seq 1 20); do kill -0 $APP_PID 2>/dev/null || break; sleep 1; done + if kill -0 $APP_PID 2>/dev/null; then + echo "FAIL: did not exit within 20s of SIGTERM (hung on shutdown)" + kill -KILL $APP_PID 2>/dev/null || true; fail=1 + else + wait $APP_PID 2>/dev/null; rc=$? + case "${rc}" in + 0|143) echo "OK: exited cleanly on SIGTERM (rc=${rc})" ;; + 139) echo "FAIL: segfaulted during shutdown"; fail=1 ;; + 134) echo "FAIL: aborted during shutdown"; fail=1 ;; + *) echo "NOTE: exited with rc=${rc} on SIGTERM" ;; + esac + fi + grep -qi "panicked" /tmp/app.log && { echo "FAIL: panicked during shutdown"; fail=1; } + cp /tmp/app.log /out/app.log 2>/dev/null || true + + [ $fail -eq 0 ] && echo "PASS" || true + exit $fail +VEOF + +overall=0 +for spec in "${TARGETS[@]}"; do + kind="${spec%%:*}" + image="${spec#*:}" + [ "${image}" = "${kind}" ] && image="" + slug="$(echo "${spec}" | tr ':/' '__')" + tout="${OUTDIR}/${slug}" + mkdir -p "${tout}" + + echo + echo "==============================================================" + echo ">> GUI launch test: ${kind} on ${image:-}" + echo "==============================================================" + + case "${kind}" in + deb) + pkg="$(stage_one "thinkutils_VERSION_amd64.deb")" || { overall=1; continue; } + docker run --rm -v "${STAGE}:/a:ro" -v "${tout}:/out" "${image:-ubuntu:24.04}" bash -c " + set -u + export DEBIAN_FRONTEND=noninteractive + ${RETRY} + retry apt-get update -qq >/dev/null + retry apt-get install -y -qq xvfb imagemagick x11-apps xdotool xcompmgr procps tesseract-ocr >/dev/null 2>&1 + apt-get install -y -qq /a/${pkg} >/dev/null 2>&1 || { echo 'FAIL: apt install failed'; exit 1; } + ${GUI_ENV} + thinkutils >/tmp/app.log 2>&1 & + APP_PID=\$! + ${WAIT_READY} + ${VERDICT} + " + ;; + rpm) + pkg="$(stage_one "thinkutils-VERSION-*.x86_64.rpm")" || { overall=1; continue; } + docker run --rm -v "${STAGE}:/a:ro" -v "${tout}:/out" "${image:-fedora:41}" bash -c " + set -u + ${RETRY} + retry dnf install -y -q xorg-x11-server-Xvfb ImageMagick xdotool xcompmgr procps-ng tesseract >/dev/null 2>&1 + dnf install -y -q /a/${pkg} >/dev/null 2>&1 || { echo 'FAIL: dnf install failed'; exit 1; } + ${GUI_ENV} + thinkutils >/tmp/app.log 2>&1 & + APP_PID=\$! + ${WAIT_READY} + ${VERDICT} + " + ;; + appimage) + # --appimage-extract rather than a direct run: mounting an AppImage + # needs FUSE, which a container does not have. Extraction exercises + # the same payload. + pkg="$(stage_one "thinkutils_VERSION_amd64.AppImage")" || { overall=1; continue; } + docker run --rm -v "${STAGE}:/a:ro" -v "${tout}:/out" "${image:-ubuntu:22.04}" bash -c " + set -u + export DEBIAN_FRONTEND=noninteractive + ${RETRY} + retry apt-get update -qq >/dev/null + retry apt-get install -y -qq xvfb imagemagick x11-apps xdotool xcompmgr procps tesseract-ocr >/dev/null 2>&1 + cd /tmp && cp /a/${pkg} app.AppImage && chmod +x app.AppImage + ./app.AppImage --appimage-extract >/dev/null 2>&1 || { echo 'FAIL: AppImage extract failed'; exit 1; } + ${GUI_ENV} + ./squashfs-root/AppRun >/tmp/app.log 2>&1 & + APP_PID=\$! + ${WAIT_READY} + ${VERDICT} + " + ;; + *) + echo "unknown target: ${kind}" >&2 + exit 2 + ;; + esac + + rc=$? + if [ "${rc}" -eq 0 ]; then + echo ">> ${kind} on ${image:-default}: PASS" + else + echo ">> ${kind} on ${image:-default}: FAIL (rc=${rc}) - artifacts in ${tout}" + overall=1 + fi +done + +echo +echo "==============================================================" +[ "${overall}" -eq 0 ] && echo "ALL GUI PACKAGES PASSED" || echo "SOME GUI PACKAGES FAILED" +exit "${overall}" diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index 7aef32b..450b28c 100755 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -79,6 +79,29 @@ async fn start_drag(app: AppHandle) -> Result<(), String> { } } +/// Called by the frontend once every template is injected and every view has +/// initialised. +/// +/// Its absence in a log means the JS never finished booting, which no pixel +/// check can prove on its own — a window can be fully painted by a frontend +/// that died halfway through init. +#[tauri::command] +fn report_frontend_ready(templates: usize, views: usize) { + println!("[thinkutils] frontend ready: templates={templates} views={views}"); +} + +/// Any uncaught frontend exception or rejection. +/// +/// This is the check that catches a view dying on an absent sysfs path: the +/// sidebar still paints, the process still lives, and without this line nothing +/// would ever notice. +#[tauri::command] +fn report_frontend_error(msg: String) { + // Truncated because an unhandled rejection can carry an entire stack. + let msg: String = msg.chars().take(500).collect(); + eprintln!("[thinkutils] frontend error: {msg}"); +} + #[cfg_attr(mobile, tauri::mobile_entry_point)] pub fn run() { tauri::Builder::default() @@ -93,6 +116,52 @@ pub fn run() { .plugin(tauri_plugin_opener::init()) .plugin(tauri_plugin_store::Builder::default().build()) .setup(|app| { + // --- Diagnostic instrumentation --- + // + // These lines are the contract scripts/test-gui-packages-docker.sh + // asserts against. In a headless container they are the only evidence + // that the app started correctly, so do not remove or reword them + // without updating that script. Format: "[thinkutils] : ". + if let Some(w) = app.get_webview_window("main") { + match w.url() { + // An http(s) URL here means the build points at devUrl, which + // renders an empty window on any machine without a dev server. + Ok(url) => println!("[thinkutils] webview url: {url}"), + Err(e) => eprintln!("[thinkutils] warning: could not read webview url: {e}"), + } + } + + // Probe the hardware surfaces this app depends on, and say so. In a + // container every one is absent, and that is a legitimate supported + // state -- not a failure. Printing it is what lets a test tell + // "started fine, no ThinkPad here" from "broken", which are otherwise + // indistinguishable from the outside. + let present = |p: &str| { + if std::path::Path::new(p).exists() { + "present" + } else { + "absent" + } + }; + let ibm_fan = present("/proc/acpi/ibm/fan"); + let bat0 = present("/sys/class/power_supply/BAT0"); + let cpufreq = present("/sys/devices/system/cpu/cpu0/cpufreq"); + println!("[thinkutils] hw probe: ibm_fan={ibm_fan} bat0={bat0} cpufreq={cpufreq}"); + + // Only the thinkpad_acpi fan interface distinguishes a supported + // machine. A battery and cpufreq exist on every Linux laptop, and a + // container inherits the host's /sys -- so keying the mode off those + // reported full ThinkPad support from inside a container that had no + // fan interface at all. The first run of the launch test caught it. + println!( + "[thinkutils] hw mode: {}", + if ibm_fan == "present" { + "full" + } else { + "degraded" + } + ); + // Load fan curve config from persistent storage let saved_config = fan_curve::load_config_from_store(app.handle()); let fan_curve_state = @@ -194,6 +263,8 @@ pub fn run() { fan_control::get_sensor_data, fan_control::get_fan_capability, environment::get_system_report, + report_frontend_ready, + report_frontend_error, fan_control::enable_fan_control, fan_control::set_fan_speed, fan_control::check_permissions, diff --git a/src/js/app.js b/src/js/app.js index 5194440..6e6ee59 100755 --- a/src/js/app.js +++ b/src/js/app.js @@ -1,6 +1,29 @@ // Main Application Entry Point console.log('[ThinkUtils] Script loaded'); +// Report uncaught frontend errors to the backend so they reach the process log. +// +// This is registered before any other import runs, because an exception thrown +// during module evaluation is exactly the case that would otherwise vanish. A +// view dying on an absent sysfs path leaves the sidebar painted and the process +// alive, so nothing outside the browser console would ever notice. +const reportError = (message) => { + try { + window.__TAURI__?.core?.invoke('report_frontend_error', { + msg: String(message).slice(0, 500) + }); + } catch { + // Reporting must never itself throw and take down init. + } +}; + +window.addEventListener('error', (e) => { + reportError(`${e.message} @ ${e.filename}:${e.lineno}`); +}); +window.addEventListener('unhandledrejection', (e) => { + reportError(`unhandled rejection: ${e.reason}`); +}); + import { initializeElements } from './dom.js'; import { setupTitlebar } from './titlebar.js'; import { setupFeatureNavigation } from './navigation.js'; @@ -99,12 +122,15 @@ function setupPermissionDialog() { async function initializeApp() { console.log('[ThinkUtils] Initializing...'); + let loadedTemplateCount = 0; + // If using modular HTML, load templates first if (isModularMode()) { console.log('[ThinkUtils] Modular mode detected, loading templates...'); try { const templates = await loadTemplates(); injectTemplates(templates); + loadedTemplateCount = Object.keys(templates).length; } catch (error) { console.error('[ThinkUtils] Failed to load templates:', error); // Continue anyway - app might still work with inline HTML @@ -142,6 +168,18 @@ async function initializeApp() { }, 2000); console.log('[ThinkUtils] Ready'); + + // Signal that init actually completed. A headless test can see a fully painted + // window from a frontend that died halfway through, so reaching this line is + // the only proof the boot sequence finished. + try { + await window.__TAURI__?.core?.invoke('report_frontend_ready', { + templates: loadedTemplateCount, + views: document.querySelectorAll('#views-container > *').length + }); + } catch (error) { + console.error('[ThinkUtils] Could not report ready state:', error); + } } window.addEventListener('DOMContentLoaded', initializeApp); From 9a4d277648b1d87474cd2a184f5d68d1c43401e2 Mon Sep 17 00:00:00 2001 From: Viet Anh Nguyen Date: Sun, 19 Jul 2026 14:25:46 +0700 Subject: [PATCH 2/9] docs: add a download page and rewrite getting started as a path The Download nav link pointed straight at the GitHub releases list, which shows every asset for every version and leaves the reader to work out which file they want. docs/download.md resolves the latest release through the GitHub API at view time rather than baking a version in at build time -- the docs site and the release pipeline deploy independently, so a hard-coded version would go stale the moment a release ships without a docs rebuild. If the API is unreachable or rate-limits (60/hour unauthenticated), every button falls back to the releases page, which always works. Assets are matched by predicate rather than exact filename so a version bump needs no edit here, and each predicate pins the architecture suffix. That is load-bearing rather than tidiness: find() returns the FIRST match, so a loose predicate would silently hand out the wrong package the day a second architecture is added, and the failure is quiet and user-side -- the page looks right, the download works, the package refuses to install. getting-started.md is restructured as an ordered path rather than a list of prerequisites, and leads with the question a reader actually has -- 'will this work on my machine' -- answered by one command. Step 2 is now stated as the step people miss, because it is: the thinkpad_acpi module refuses every fan write unless loaded with fan_control=1, and that is fixed at module load, so granting permissions cannot fix it. Presenting it as one prerequisite among several is what produces the 'I granted permissions and nothing happened' report. Also documents the Ubuntu 22.04 polkit 0.105 limitation, which silently makes passwordless fan control not work there, and the fan safety behaviour -- revert on disable, on unreadable sensors, on exit, plus the firmware watchdog. --- docs/.vitepress/config.mjs | 5 +- docs/.vitepress/theme/custom.css | 65 ++++++++++++ docs/download.md | 175 +++++++++++++++++++++++++++++++ docs/guide/getting-started.md | 140 ++++++++++++++++++------- 4 files changed, 341 insertions(+), 44 deletions(-) create mode 100644 docs/download.md diff --git a/docs/.vitepress/config.mjs b/docs/.vitepress/config.mjs index ed4e542..d9cde30 100644 --- a/docs/.vitepress/config.mjs +++ b/docs/.vitepress/config.mjs @@ -12,10 +12,7 @@ export default withMermaid( nav: [ { text: 'Guide', link: '/guide/getting-started' }, { text: 'Development', link: '/development/architecture' }, - { - text: 'Download', - link: 'https://github.com/vietanhdev/ThinkUtils/releases', - }, + { text: 'Download', link: '/download' }, ], sidebar: { '/guide/': [ diff --git a/docs/.vitepress/theme/custom.css b/docs/.vitepress/theme/custom.css index d195786..6328dcf 100644 --- a/docs/.vitepress/theme/custom.css +++ b/docs/.vitepress/theme/custom.css @@ -96,3 +96,68 @@ max-width: 100%; height: auto; } + +/* --- Download page -------------------------------------------------------- + Cards for the package downloads. Kept in the theme rather than inline in + download.md so the markdown stays readable and the styling can be reused. */ + +.dl-version { + margin: 0 0 1.5rem; + color: var(--vp-c-text-2); + font-size: 0.95rem; +} + +.dl-grid { + display: grid; + + /* auto-fit rather than a fixed column count, so three cards sit in a row on a + desktop and stack cleanly on a phone without a media query. */ + grid-template-columns: repeat(auto-fit, minmax(210px, 1fr)); + gap: 0.85rem; + margin: 1.5rem 0; +} + +.dl-card { + display: flex; + flex-direction: column; + gap: 0.3rem; + padding: 1.1rem 1.25rem; + border: 1px solid var(--vp-c-divider); + border-radius: 10px; + background: var(--vp-c-bg-soft); + text-decoration: none; + transition: + border-color 0.2s ease, + transform 0.2s ease; +} + +.dl-card:hover { + border-color: var(--vp-c-brand-1); + transform: translateY(-2px); +} + +/* Keyboard users get the same affordance as the hover state. */ +.dl-card:focus-visible { + outline: 2px solid var(--vp-c-brand-1); + outline-offset: 2px; +} + +.dl-card-title { + font-weight: 600; + color: var(--vp-c-text-1); +} + +.dl-card-sub { + font-size: 0.85rem; + color: var(--vp-c-text-2); +} + +@media (prefers-reduced-motion: reduce) { + .dl-card { + transition: none; + } + + .dl-card:hover { + transform: none; + } +} diff --git a/docs/download.md b/docs/download.md new file mode 100644 index 0000000..a10a10f --- /dev/null +++ b/docs/download.md @@ -0,0 +1,175 @@ +--- +title: Download +description: Download ThinkUtils for Linux — .deb, .rpm and AppImage for ThinkPad laptops. +--- + + + +# Download ThinkUtils + +

+ + + +

+ +For **Lenovo ThinkPad** laptops running Linux on **x86_64**. Built on Ubuntu 22.04 +(glibc 2.35), so it runs on **Ubuntu 22.04+, Debian 12+ and Fedora 36+**. + +Every release is installed into a clean container and launched under a virtual +display before it ships — on Ubuntu 22.04 and 24.04, Debian 12, and Fedora 41 — +with a screenshot checked by OCR to confirm the interface actually rendered. + + + +```bash +# Debian / Ubuntu +sudo apt install ./thinkutils_*_amd64.deb + +# Fedora / RHEL +sudo dnf install ./thinkutils-*.x86_64.rpm + +# AppImage — portable, nothing to install +chmod +x thinkutils_*_amd64.AppImage +./thinkutils_*_amd64.AppImage +``` + +::: tip Use `apt install ./file.deb`, not `dpkg -i` +`apt` pulls in the WebKit and GTK libraries ThinkUtils needs. `dpkg -i` does not, +and leaves you resolving them by hand. +::: + +## Ubuntu APT repository + +For automatic updates through `apt`: + +```bash +echo "deb [trusted=yes] https://gh.vietanh.dev/ThinkUtils/apt ./" \ + | sudo tee /etc/apt/sources.list.d/thinkutils.list +sudo apt update +sudo apt install thinkutils +``` + +## Before fan control works + +One step is not optional, and it is the most common reason people think the app +is broken. The `thinkpad_acpi` kernel module **refuses every fan change** unless +it was loaded with `fan_control=1`: + +```bash +echo 'options thinkpad_acpi fan_control=1' \ + | sudo tee /etc/modprobe.d/thinkpad_acpi.conf +sudo modprobe -r thinkpad_acpi && sudo modprobe thinkpad_acpi +``` + +The app detects this and offers to do it for you on the Fan Control page. It is +worth knowing why granting permissions alone cannot fix it: the setting is a +kernel module parameter, fixed at load time, so no amount of privilege changes it +while the module is running. + +Reboot if the reload fails — the module is often held open by something else. + +::: warning Ubuntu 22.04 will still ask for your password +Ubuntu 22.04 ships polkit 0.105, which Debian and Ubuntu patched to ignore +JavaScript rule files. That is the mechanism ThinkUtils uses to grant passwordless +fan control, so on 22.04 every fan change prompts for a password. Everything +works; it is just not silent. Upgrading the distribution is the only fix. +::: + +## Which ThinkPads are supported + +Fan control needs the `thinkpad_acpi` kernel module, which covers most ThinkPads +from the X, T, P and L series. To check before installing: + +```bash +ls /proc/acpi/ibm/fan && echo "supported" +``` + +Dual-fan machines — P1, P15, X1 Extreme and similar — are supported, and both +fans are reported. The firmware drives them together, so they cannot be set to +different speeds; that is a hardware limitation, not an app one. + +Battery thresholds, CPU governor and system monitoring work on any Linux laptop. +Only fan control is ThinkPad-specific. + +## Building from source + +```bash +git clone https://github.com/vietanhdev/ThinkUtils.git +cd ThinkUtils +npm install +npm run tauri build +``` + +Packages land in `src-tauri/target/release/bundle/`. See the +[development guide](/development/architecture) for the toolchain you will need. diff --git a/docs/guide/getting-started.md b/docs/guide/getting-started.md index 9695c8c..646eada 100644 --- a/docs/guide/getting-started.md +++ b/docs/guide/getting-started.md @@ -1,68 +1,128 @@ # Getting Started -ThinkUtils is a native Linux desktop app for ThinkPad laptops, built with [Tauri](https://tauri.app/). It gives you direct control over hardware that's normally locked behind command-line tools or config files. +ThinkUtils gives you direct control over ThinkPad hardware that Linux normally +hides behind kernel module parameters and root-owned files in `/sys` — fan speed, +battery charge limits, CPU governor. ![ThinkUtils Home Dashboard](/screenshots/home.png) -## Prerequisites +## Will it work on my machine? -ThinkUtils requires a few system packages: +One command answers it: + +```bash +ls /proc/acpi/ibm/fan && echo "fan control supported" +``` + +If that prints a path, you have a supported ThinkPad. If it prints nothing, fan +control will not work on this machine — but battery thresholds, CPU governor and +system monitoring still will, because those are not ThinkPad-specific. + +Dual-fan machines (P1, P15, X1 Extreme) are supported and both fans are reported. +The firmware drives them together, so they cannot be set to different speeds. + +## 1. Install + +Grab a package from the [download page](/download), then: ::: code-group ```bash [Debian/Ubuntu] -sudo apt install lm-sensors policykit-1 +sudo apt install ./thinkutils_*_amd64.deb ``` ```bash [Fedora/RHEL] -sudo dnf install lm_sensors polkit +sudo dnf install ./thinkutils-*.x86_64.rpm ``` -```bash [Arch Linux] -sudo pacman -S lm_sensors polkit +```bash [AppImage] +chmod +x thinkutils_*_amd64.AppImage +./thinkutils_*_amd64.AppImage ``` ::: -## Enable Fan Control +Use `apt install ./file.deb` rather than `dpkg -i` — `apt` pulls in the WebKit and +GTK libraries the app needs, and `dpkg` leaves you resolving them by hand. + +## 2. Enable fan control in the kernel + +**This is the step people miss**, and it produces the most confusing symptom: the +app looks fine, you grant permissions, and every fan change is silently refused. + +The `thinkpad_acpi` module rejects all fan writes unless it was loaded with +`fan_control=1`. That is a module parameter fixed at load time, so no amount of +granting permissions changes it while the module is running. + +ThinkUtils detects this and offers a button on the Fan Control page. To do it by +hand: + +```bash +echo 'options thinkpad_acpi fan_control=1' \ + | sudo tee /etc/modprobe.d/thinkpad_acpi.conf +sudo modprobe -r thinkpad_acpi && sudo modprobe thinkpad_acpi +``` + +If the reload fails, something else is holding the module open — reboot instead. -ThinkPad fan control requires the `thinkpad_acpi` kernel module with fan control enabled: +To confirm it worked: -1. Create or edit the configuration: - ```bash - sudo nano /etc/modprobe.d/thinkpad_acpi.conf - ``` +```bash +grep commands: /proc/acpi/ibm/fan +``` -2. Add this line: - ``` - options thinkpad_acpi fan_control=1 - ``` +Lines here mean writes will be accepted. No lines means they will not. -3. Reboot or reload the module: - ```bash - sudo modprobe -r thinkpad_acpi - sudo modprobe thinkpad_acpi - ``` +## 3. Grant permissions -## First Launch +Launch ThinkUtils and click **Setup Permissions** when prompted. You will be asked +for your password once. -1. Install ThinkUtils (see [Installation](./installation)) -2. Launch from your application menu or run `thinkutils` -3. Click **"Setup Permissions"** when prompted and enter your password once -4. All features now work without further password prompts +That installs a small helper at `/usr/local/bin/thinkutils-fan-control`, which +accepts fan level commands and nothing else, plus a polkit rule scoped to that one +binary. The app itself never runs as root. -See [Permissions](./permissions) for details on what gets configured. +::: warning Ubuntu 22.04 will keep asking for your password +Ubuntu 22.04 ships polkit 0.105, which Debian and Ubuntu patched to ignore +JavaScript rule files — the mechanism that grants passwordless fan control. Every +fan change will prompt. Everything works; it is just not silent. Upgrading the +distribution is the only fix. +::: -## Navigation +See [Permissions](./permissions) for exactly what gets installed. -Use the left sidebar to switch between features: +## Where things are -| View | Purpose | -|------|---------| -| **Home** | Dashboard with quick controls | -| **Fan Control** | Temperature monitoring and fan speed | -| **Battery** | Charge thresholds and health | -| **Performance** | CPU governor and power profiles | -| **Monitor** | Real-time system stats | +| View | What it does | +|------|--------------| +| **Home** | Dashboard with the controls you reach for most | +| **Fan Control** | Fan speed, temperature curve, live readings | +| **Battery** | Charge thresholds and battery health | +| **Performance** | CPU governor, turbo boost, power profiles | +| **Monitor** | Live CPU, memory, disk and network | | **System Info** | Hardware details | -| **Security** | Virus scanning | -| **MCP** | AI integration settings | -| **Sync** | Google Drive backup | +| **Security** | ClamAV virus scanning | +| **AI Integration** | MCP server for AI assistants | +| **Sync** | Google Drive settings backup | + +## A note on fan safety + +Manual fan control means you are overriding the firmware's thermal management. +ThinkUtils will not leave the fan stranded: it returns control to the firmware +when you disable the curve, when temperature sensors become unreadable, and when +the app exits. It also arms the firmware's own watchdog while a manual level is +set, so the fan reverts to automatic even if the app is killed outright. + +Setting a low fixed level under sustained load is still your call to make, and +worth making deliberately. + +## If something is not working + +Start with the Fan Control page — it reports what is blocking it and names the +specific obstacle rather than guessing. The most common causes, in order: + +1. `fan_control=1` is not set (step 2 above) +2. Permissions have not been granted (step 3) +3. The machine is not a ThinkPad, or `thinkpad_acpi` is not loaded + +Optional features degrade rather than fail: without `lm-sensors` you lose +temperature readings, and without ClamAV the Security page cannot scan. The app +tells you which package to install for your distribution. From 9ba526509c80d8e1d1c83f6bde952f884464d3af Mon Sep 17 00:00:00 2001 From: Viet Anh Nguyen Date: Sun, 19 Jul 2026 14:37:04 +0700 Subject: [PATCH 3/9] feat(packaging): move the helper out of /usr/local and add AUR/COPR manifests Distro packaging was blocked by one thing: the fan helper installed to /usr/local/bin at runtime. Debian Policy 9.1.2 and the Fedora guidelines both forbid a package writing there, and a helper materialised by a button click is not package-owned -- dpkg -L would not list the most security-sensitive file the app uses, and uninstalling would leave a root-owned binary and a polkit rule behind. HELPER_PATH becomes HELPER_CANDIDATES, searched in order: /usr/lib/thinkutils/... Debian and Arch convention /usr/libexec/thinkutils/... Fedora convention /usr/local/bin/... legacy self-install, kept so existing installs keep working setup_permissions() now skips installing the helper and rule entirely when helper_is_packaged(), because overwriting those files puts the package database out of sync with the filesystem. The path had been duplicated in four places -- fan_control, mcp, fan_curve, and inside the polkit rule text. The rule is now generated from the constant, since a rule naming a path the helper is not at grants nothing while looking correct. Two security improvements fall out of generating it: subject.local && subject.active is now required. Without it any SSH session belonging to a wheel/sudo user inherited passwordless hardware control, as did a background session the user had switched away from. Packages ship the rule to /usr/share/polkit-1/rules.d, not /etc. /etc is the administrator's namespace; a package writing there shadows their rules and is never cleaned up. packaging/ adds the AUR PKGBUILD and COPR spec, plus the helper and rule as generated artifacts (cargo run --example gen-packaging). tests/packaging.rs is the point of all this: 7 tests asserting the committed files match what the source generates, that each format installs where the app actually searches, that nothing writes to /usr/local, that the rule goes under /usr/share, and that versions agree. The drift they prevent fails SILENTLY -- polkit denies, the app falls back to a password prompt, and it reads as a permissions problem rather than a packaging bug. bump-version.sh now covers PKGBUILD and the spec too, and ci.yml calls it rather than keeping its own copy of the file list, so adding a packaging file cannot leave CI checking a stale subset. Suite: 80 -> 87. --- .github/workflows/ci.yml | 28 +---- packaging/aur/PKGBUILD | 53 ++++++++ packaging/copr/thinkutils.spec | 80 ++++++++++++ packaging/helper/thinkutils-fan-control | 16 +++ packaging/polkit/50-thinkutils.rules | 23 ++++ scripts/bump-version.sh | 13 ++ src-tauri/examples/gen-packaging.rs | 46 +++++++ src-tauri/src/environment.rs | 2 +- src-tauri/src/fan_control.rs | 156 +++++++++++++++++------ src-tauri/src/fan_curve.rs | 12 +- src-tauri/src/lib.rs | 2 +- src-tauri/src/mcp.rs | 10 +- src-tauri/src/permissions.rs | 48 ++++--- src-tauri/tests/packaging.rs | 161 ++++++++++++++++++++++++ 14 files changed, 554 insertions(+), 96 deletions(-) create mode 100644 packaging/aur/PKGBUILD create mode 100644 packaging/copr/thinkutils.spec create mode 100755 packaging/helper/thinkutils-fan-control create mode 100644 packaging/polkit/50-thinkutils.rules create mode 100644 src-tauri/examples/gen-packaging.rs create mode 100644 src-tauri/tests/packaging.rs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 26c4617..88c4e63 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -31,30 +31,10 @@ jobs: runs-on: ubuntu-24.04 steps: - uses: actions/checkout@v4 - - name: Check the four version declarations agree - run: | - set -euo pipefail - pkg=$(jq -r .version package.json) - lock=$(jq -r .version package-lock.json) - lockpkg=$(jq -r '.packages."".version' package-lock.json) - cargo=$(sed -n 's/^version = "\(.*\)"/\1/p' src-tauri/Cargo.toml | head -1) - tauri=$(jq -r .version src-tauri/tauri.conf.json) - - printf 'package.json : %s\n' "$pkg" - printf 'package-lock.json (top) : %s\n' "$lock" - printf 'package-lock.json (pkgs) : %s\n' "$lockpkg" - printf 'src-tauri/Cargo.toml : %s\n' "$cargo" - printf 'src-tauri/tauri.conf.json: %s\n' "$tauri" - - fail=0 - for v in "$lock" "$lockpkg" "$cargo" "$tauri"; do - [ "$v" = "$pkg" ] || fail=1 - done - if [ "$fail" = "1" ]; then - echo "::error::version declarations disagree - see CLAUDE.md 'Version Bumping'" - exit 1 - fi - echo "OK: all version declarations agree at $pkg" + # One script owns the list, so adding a packaging file cannot leave CI + # checking a stale subset of it. + - name: Check every version declaration agrees + run: ./scripts/bump-version.sh --check # Runs exactly what husky's pre-commit hook runs. That hook only sees STAGED # files, on machines that ran npm install - a convenience, not a gate. diff --git a/packaging/aur/PKGBUILD b/packaging/aur/PKGBUILD new file mode 100644 index 0000000..afe80b0 --- /dev/null +++ b/packaging/aur/PKGBUILD @@ -0,0 +1,53 @@ +# Maintainer: Viet Anh Nguyen +pkgname=thinkutils +pkgver=0.1.10 +pkgrel=1 +pkgdesc="ThinkPad fan control, battery care and system monitoring for Linux" +# thinkpad_acpi is an x86 platform driver and ThinkPads are x86_64. aarch64 would +# build but there is no hardware to run it on -- do not claim it. +arch=('x86_64') +url="https://github.com/vietanhdev/ThinkUtils" +license=('LGPL-3.0-or-later') +depends=('webkit2gtk-4.1' 'gtk3' 'libayatana-appindicator' 'librsvg' 'polkit') +makedepends=('cargo' 'pkgconf' 'nodejs' 'npm') +optdepends=( + 'lm_sensors: temperature readings and the fan curve' + 'clamav: virus scanning on the Security page' + 'power-profiles-daemon: power profile switching' +) +source=("$pkgname-$pkgver.tar.gz::$url/archive/refs/tags/v$pkgver.tar.gz") +sha256sums=('SKIP') + +prepare() { + cd "ThinkUtils-$pkgver/src-tauri" + export RUSTUP_TOOLCHAIN=stable + cargo fetch --locked --target "$(rustc -vV | sed -n 's/host: //p')" +} + +build() { + cd "ThinkUtils-$pkgver/src-tauri" + export RUSTUP_TOOLCHAIN=stable + export CARGO_TARGET_DIR=target + # Plain cargo, not `tauri build`: the bundler downloads linuxdeploy and emits + # .deb/.AppImage artifacts that are meaningless inside a PKGBUILD. + cargo build --frozen --release +} + +package() { + cd "ThinkUtils-$pkgver" + install -Dm0755 src-tauri/target/release/thinkutils "$pkgdir/usr/bin/thinkutils" + + # Privileged fan helper: package-owned, root:root, and NOT in /usr/local -- + # Arch packages may not write there. Matches HELPER_CANDIDATES[0]. + install -Dm0755 packaging/helper/thinkutils-fan-control \ + "$pkgdir/usr/lib/thinkutils/thinkutils-fan-control" + + # Vendor polkit rules belong under /usr/share; /etc is the admin's namespace. + install -Dm0644 packaging/polkit/50-thinkutils.rules \ + "$pkgdir/usr/share/polkit-1/rules.d/50-thinkutils.rules" + + install -Dm0644 thinkutils.desktop "$pkgdir/usr/share/applications/thinkutils.desktop" + install -Dm0644 src-tauri/icons/128x128.png \ + "$pkgdir/usr/share/icons/hicolor/128x128/apps/thinkutils.png" + install -Dm0644 LICENSE "$pkgdir/usr/share/licenses/$pkgname/LICENSE" +} diff --git a/packaging/copr/thinkutils.spec b/packaging/copr/thinkutils.spec new file mode 100644 index 0000000..5f6023c --- /dev/null +++ b/packaging/copr/thinkutils.spec @@ -0,0 +1,80 @@ +# NETWORK: %build fetches crates from crates.io, which requires the COPR +# *project* to have networking enabled: +# copr-cli modify thinkutils --enable-net on +# mock disables builder networking by default. Without it every build dies with +# "Could not resolve host: index.crates.io". This is a property of the project, +# not of this spec, so it does not travel with the repository. +Name: thinkutils +Version: 0.1.10 +Release: 1%{?dist} +Summary: ThinkPad fan control, battery care and system monitoring + +License: LGPL-3.0-or-later +URL: https://github.com/vietanhdev/ThinkUtils +Source0: %{url}/archive/refs/tags/v%{version}.tar.gz#/ThinkUtils-%{version}.tar.gz + +BuildRequires: cargo +BuildRequires: rust +BuildRequires: gcc +BuildRequires: pkgconfig(webkit2gtk-4.1) +BuildRequires: pkgconfig(gtk+-3.0) +BuildRequires: pkgconfig(ayatana-appindicator3-0.1) +BuildRequires: pkgconfig(librsvg-2.0) +BuildRequires: pkgconfig(openssl) +BuildRequires: desktop-file-utils + +Requires: webkit2gtk4.1 +Requires: polkit + +Recommends: lm_sensors + +# thinkpad_acpi is an x86 platform driver; there is no ThinkPad to run this on +# anywhere else. +ExclusiveArch: x86_64 + +%description +ThinkUtils is a desktop utility for Lenovo ThinkPad laptops running Linux, +providing manual and temperature-curve fan control via thinkpad_acpi, battery +charge threshold management, CPU governor and turbo-boost control, and live +system monitoring. + +Privileged operations go through a dedicated, package-owned helper authorised by +a narrow polkit rule; the application itself runs unprivileged. + +%prep +%autosetup -n ThinkUtils-%{version} + +%build +# Plain cargo, not `tauri build`: the bundler downloads linuxdeploy and emits +# .deb/.AppImage artifacts that are meaningless inside an RPM build. +cd src-tauri && cargo build --release + +%install +install -Dpm0755 src-tauri/target/release/thinkutils %{buildroot}%{_bindir}/thinkutils + +# %{_libexecdir} is the canonical Fedora home for an internal helper that must +# not be on $PATH. NEVER /usr/local -- forbidden by the packaging guidelines. +# Matches HELPER_CANDIDATES[1] in src-tauri/src/fan_control.rs. +install -Dpm0755 packaging/helper/thinkutils-fan-control \ + %{buildroot}%{_libexecdir}/thinkutils/thinkutils-fan-control + +install -Dpm0644 packaging/polkit/50-thinkutils.rules \ + %{buildroot}%{_datadir}/polkit-1/rules.d/50-thinkutils.rules + +desktop-file-install --dir=%{buildroot}%{_datadir}/applications thinkutils.desktop +install -Dpm0644 src-tauri/icons/128x128.png \ + %{buildroot}%{_datadir}/icons/hicolor/128x128/apps/thinkutils.png + +%files +%license LICENSE +%doc README.md +%{_bindir}/thinkutils +%dir %{_libexecdir}/thinkutils +%{_libexecdir}/thinkutils/thinkutils-fan-control +%{_datadir}/polkit-1/rules.d/50-thinkutils.rules +%{_datadir}/applications/thinkutils.desktop +%{_datadir}/icons/hicolor/128x128/apps/thinkutils.png + +%changelog +* Sun Jul 19 2026 Viet Anh Nguyen - 0.1.10-1 +- Initial COPR package. diff --git a/packaging/helper/thinkutils-fan-control b/packaging/helper/thinkutils-fan-control new file mode 100755 index 0000000..dd0b623 --- /dev/null +++ b/packaging/helper/thinkutils-fan-control @@ -0,0 +1,16 @@ +#!/bin/bash +set -e +FAN="/proc/acpi/ibm/fan" +# Exact-match whitelist. "watchdog 30" is permitted because the firmware +# watchdog can only ever return the fan to automatic control -- it is the +# recovery path if this app dies while holding a manual level. No other +# watchdog value is accepted, and enable/disable are deliberately absent. +case "$1" in + "level auto"|"level full-speed"|"level 0"|"level 1"|"level 2"|"level 3"|"level 4"|"level 5"|"level 6"|"level 7"|"watchdog 30") + echo "$1" > "$FAN" + ;; + *) + echo "Invalid command" >&2 + exit 1 + ;; +esac diff --git a/packaging/polkit/50-thinkutils.rules b/packaging/polkit/50-thinkutils.rules new file mode 100644 index 0000000..e4c6891 --- /dev/null +++ b/packaging/polkit/50-thinkutils.rules @@ -0,0 +1,23 @@ +/* ThinkUtils: passwordless fan control via the dedicated helper only. + * + * Generated from HELPER_CANDIDATES -- do not edit by hand. The helper accepts + * fan level commands and nothing else, so this grants exactly that and no + * general privilege. + */ +polkit.addRule(function(action, subject) { + if (action.id == "org.freedesktop.policykit.exec") { + var program = action.lookup("program"); + if ( + program == "/usr/lib/thinkutils/thinkutils-fan-control" || + program == "/usr/libexec/thinkutils/thinkutils-fan-control" || + program == "/usr/local/bin/thinkutils-fan-control" + ) { + /* Local, active sessions only: an SSH session must not inherit + passwordless hardware control. */ + if (subject.local && subject.active && + (subject.isInGroup("wheel") || subject.isInGroup("sudo"))) { + return polkit.Result.YES; + } + } + } +}); diff --git a/scripts/bump-version.sh b/scripts/bump-version.sh index b6d5e14..3650285 100755 --- a/scripts/bump-version.sh +++ b/scripts/bump-version.sh @@ -46,6 +46,11 @@ read_versions() { printf 'package-lock.json (pkgs)|%s\n' "$(jq -r '.packages."".version' package-lock.json)" printf 'src-tauri/Cargo.toml|%s\n' "$(sed -n 's/^version = "\(.*\)"/\1/p' src-tauri/Cargo.toml | head -1)" printf 'src-tauri/tauri.conf.json|%s\n' "$(jq -r .version src-tauri/tauri.conf.json)" + # Packaging manifests. A stale version here publishes a package whose + # filename and contents disagree, and the AUR/COPR sources point at a git tag + # that does not exist. + printf 'packaging/aur/PKGBUILD|%s\n' "$(sed -n 's/^pkgver=\(.*\)/\1/p' packaging/aur/PKGBUILD)" + printf 'packaging/copr spec|%s\n' "$(sed -n 's/^Version: *\(.*\)/\1/p' packaging/copr/thinkutils.spec)" } if [ "$CHECK_ONLY" -eq 1 ]; then @@ -113,6 +118,14 @@ edit package-lock.json \ "0,/\"version\": *\"[^\"]*\"/s//\"version\": \"$NEW_VERSION\"/" \ "package-lock.json (top level)" +edit packaging/aur/PKGBUILD \ + "0,/^pkgver=.*/s//pkgver=$NEW_VERSION/" \ + "packaging/aur/PKGBUILD" + +edit packaging/copr/thinkutils.spec \ + "0,/^Version: +.*/s//Version: $NEW_VERSION/" \ + "packaging/copr/thinkutils.spec" + python3 - "$NEW_VERSION" <<'PY' import json, sys version = sys.argv[1] diff --git a/src-tauri/examples/gen-packaging.rs b/src-tauri/examples/gen-packaging.rs new file mode 100644 index 0000000..e85b419 --- /dev/null +++ b/src-tauri/examples/gen-packaging.rs @@ -0,0 +1,46 @@ +//! Emit the packaging artifacts that must stay in lockstep with the Rust source. +//! +//! The polkit rule and the fan helper are installed by three different package +//! formats, and the app searches for them at paths defined in `fan_control`. If a +//! packaged copy drifts from the constants, the rule grants access to a path the +//! helper is not at — which fails silently and looks exactly like a permissions +//! problem. +//! +//! Regenerate with: +//! +//! ```sh +//! cargo run --example gen-packaging -- ../packaging +//! ``` +//! +//! `packaging_matches_source` in tests/packaging.rs fails if the committed files +//! and these outputs disagree, so the two cannot diverge unnoticed. + +use std::io::Write; + +fn main() { + let out_dir = std::env::args() + .nth(1) + .unwrap_or_else(|| "../packaging".to_string()); + + let polkit_dir = format!("{}/polkit", out_dir); + let helper_dir = format!("{}/helper", out_dir); + std::fs::create_dir_all(&polkit_dir).expect("create polkit dir"); + std::fs::create_dir_all(&helper_dir).expect("create helper dir"); + + let rule_path = format!("{}/50-thinkutils.rules", polkit_dir); + std::fs::write(&rule_path, thinkutils_lib::fan_control::polkit_rule()).expect("write rule"); + println!("wrote {}", rule_path); + + let helper_path = format!("{}/thinkutils-fan-control", helper_dir); + let mut f = std::fs::File::create(&helper_path).expect("create helper"); + f.write_all(thinkutils_lib::fan_control::HELPER_SCRIPT.as_bytes()) + .expect("write helper"); + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(&helper_path, std::fs::Permissions::from_mode(0o755)) + .expect("chmod helper"); + } + println!("wrote {}", helper_path); +} diff --git a/src-tauri/src/environment.rs b/src-tauri/src/environment.rs index e5e592b..518637b 100644 --- a/src-tauri/src/environment.rs +++ b/src-tauri/src/environment.rs @@ -463,7 +463,7 @@ pub fn get_system_report() -> ApiResponse { command_exists("pkexec"), fan_control_enabled, modprobe_conf_present, - std::path::Path::new(crate::fan_control::HELPER_PATH).exists(), + crate::fan_control::helper_path().is_some(), ); ApiResponse { diff --git a/src-tauri/src/fan_control.rs b/src-tauri/src/fan_control.rs index 51ba3ca..a45ac18 100755 --- a/src-tauri/src/fan_control.rs +++ b/src-tauri/src/fan_control.rs @@ -5,11 +5,57 @@ use std::fs; use std::process::Command; const PROC_FAN: &str = "/proc/acpi/ibm/fan"; -pub const HELPER_PATH: &str = "/usr/local/bin/thinkutils-fan-control"; + +/// Where the privileged helper may live, in the order it is searched. +/// +/// A distro package ships it at one of the first two paths, both of which are +/// root-owned and package-managed. `/usr/local` is reserved for the local +/// administrator — Debian Policy §9.1.2 and the Fedora guidelines both forbid a +/// package writing there — so it appears only as the legacy location used by the +/// app's own installer, kept so existing installs keep working. +pub const HELPER_CANDIDATES: &[&str] = &[ + // Debian and Arch convention for an internal helper not on $PATH. + "/usr/lib/thinkutils/thinkutils-fan-control", + // Fedora convention (%{_libexecdir}). + "/usr/libexec/thinkutils/thinkutils-fan-control", + // Legacy: written at runtime by setup_permissions() on a non-packaged install. + "/usr/local/bin/thinkutils-fan-control", +]; + +/// Where `setup_permissions()` writes the helper when the app installs its own. +/// +/// Only used when no packaged helper is present — a package-owned file must +/// never be overwritten at runtime. +pub const HELPER_SELF_INSTALL_PATH: &str = "/usr/local/bin/thinkutils-fan-control"; + +/// The first helper that actually exists, if any. +pub fn helper_path() -> Option<&'static str> { + HELPER_CANDIDATES + .iter() + .copied() + .find(|p| std::path::Path::new(p).exists()) +} + +/// Whether the helper came from a distro package. +/// +/// When true the app must not install, overwrite, or offer to reinstall it: those +/// files belong to dpkg/rpm/pacman, and rewriting them puts the package database +/// out of sync with the filesystem. +pub fn helper_is_packaged() -> bool { + HELPER_CANDIDATES + .iter() + .take(2) + .any(|p| std::path::Path::new(p).exists()) +} + +/// Vendor-supplied polkit rules belong under /usr/share; /etc is the +/// administrator's namespace. A package writing to /etc/polkit-1/rules.d shadows +/// the admin's own rules and is never cleaned up on uninstall. pub const POLKIT_RULE_PATH: &str = "/etc/polkit-1/rules.d/50-thinkutils.rules"; +pub const POLKIT_RULE_PACKAGED_PATH: &str = "/usr/share/polkit-1/rules.d/50-thinkutils.rules"; /// Dedicated fan control helper script - validates input before writing to fan. -/// Installed at HELPER_PATH by setup_permissions(). +/// Installed at one of HELPER_CANDIDATES. pub const HELPER_SCRIPT: &str = r#"#!/bin/bash set -e FAN="/proc/acpi/ibm/fan" @@ -33,20 +79,51 @@ esac /// Must stay in sync with the literal in HELPER_SCRIPT above; a test asserts it. pub const FAN_WATCHDOG_SECS: u32 = 30; -/// Polkit rule that only allows the dedicated helper script without password. -/// Much tighter than allowing arbitrary bash execution. -pub const POLKIT_RULE: &str = r#"/* ThinkUtils: Allow passwordless fan control via dedicated helper only */ -polkit.addRule(function(action, subject) { - if (action.id == "org.freedesktop.policykit.exec") { +/// Build the polkit rule granting passwordless exec to the helper, and nothing +/// else. +/// +/// Generated from HELPER_CANDIDATES rather than written out, because the path +/// used to be duplicated in the rule text and in the constant — and a rule +/// naming a path the helper is not installed at grants nothing while looking +/// correct. +/// +/// `subject.local && subject.active` is deliberate: without it, any SSH session +/// belonging to a wheel/sudo user gets passwordless root-adjacent exec, as does +/// a backgrounded session the user has switched away from. Fan control is a +/// physical-console concern. +pub fn polkit_rule() -> String { + let allowed = HELPER_CANDIDATES + .iter() + .map(|p| format!(" program == \"{}\"", p)) + .collect::>() + .join(" ||\n"); + + format!( + r#"/* ThinkUtils: passwordless fan control via the dedicated helper only. + * + * Generated from HELPER_CANDIDATES -- do not edit by hand. The helper accepts + * fan level commands and nothing else, so this grants exactly that and no + * general privilege. + */ +polkit.addRule(function(action, subject) {{ + if (action.id == "org.freedesktop.policykit.exec") {{ var program = action.lookup("program"); - if (program == "/usr/local/bin/thinkutils-fan-control") { - if (subject.isInGroup("wheel") || subject.isInGroup("sudo")) { + if ( +{} + ) {{ + /* Local, active sessions only: an SSH session must not inherit + passwordless hardware control. */ + if (subject.local && subject.active && + (subject.isInGroup("wheel") || subject.isInGroup("sudo"))) {{ return polkit.Result.YES; - } - } - } -}); -"#; + }} + }} + }} +}}); +"#, + allowed + ) +} /// Valid fan speed values (whitelist) const VALID_SPEEDS: &[&str] = &["auto", "full-speed", "0", "1", "2", "3", "4", "5", "6", "7"]; @@ -412,9 +489,9 @@ pub async fn set_fan_speed(speed: String) -> ApiResponse { println!("[Fan] Need elevated permissions"); // 2. Use dedicated helper if installed (passwordless via polkit rule) - if std::path::Path::new(HELPER_PATH).exists() { + if let Some(helper) = helper_path() { match tokio::process::Command::new("pkexec") - .arg(HELPER_PATH) + .arg(helper) .arg(&command_str) .output() .await @@ -504,7 +581,7 @@ pub fn check_permissions() -> ApiResponse { // Check if the dedicated helper is installed (installed alongside the polkit rule). // We only check the helper because /etc/polkit-1/rules.d/ is root-only, // so Path::exists() on the polkit rule always fails for normal users. - let helper_installed = std::path::Path::new(HELPER_PATH).exists(); + let helper_installed = helper_path().is_some(); let has_permission = direct_write || helper_installed; @@ -753,35 +830,40 @@ commands:\twatchdog (0 disables, timeout is 0-120) } } - /// The polkit rule grants passwordless root. It must name the helper binary and - /// nothing else -- granting a shell would make the tight helper pointless. + /// The polkit rule grants passwordless root. It must name the helper binaries + /// and nothing else -- granting a shell would make the tight helper pointless. #[test] - fn polkit_rule_grants_only_the_helper_binary() { - assert!( - POLKIT_RULE.contains(HELPER_PATH), - "polkit rule must reference the helper path" - ); + fn polkit_rule_grants_only_the_helper_binaries() { + let rule = polkit_rule(); + for candidate in HELPER_CANDIDATES { + assert!( + rule.contains(candidate), + "polkit rule must cover {}", + candidate + ); + } for forbidden in ["/bin/bash", "/bin/sh", "/usr/bin/bash", "/usr/bin/env"] { assert!( - !POLKIT_RULE.contains(forbidden), + !rule.contains(forbidden), "polkit rule must not grant {}", forbidden ); } } - /// The helper is written into a root-owned path by the setup script. If the - /// constant ever drifted to a user-writable location, any local user could - /// replace the binary that polkit grants passwordless root to. + /// Without local+active, any SSH session belonging to a wheel/sudo user + /// inherits passwordless hardware control, as does a background session the + /// user has switched away from. #[test] - fn helper_path_is_not_user_writable_location() { - assert!(HELPER_PATH.starts_with("/usr/local/bin/") || HELPER_PATH.starts_with("/usr/bin/")); - for bad in ["/tmp/", "/var/tmp/", "/home/", "/dev/shm/"] { - assert!( - !HELPER_PATH.starts_with(bad), - "helper must not live in {}", - bad - ); - } + fn polkit_rule_requires_a_local_active_session() { + let rule = polkit_rule(); + assert!( + rule.contains("subject.local"), + "rule must require a local session" + ); + assert!( + rule.contains("subject.active"), + "rule must require an active session" + ); } } diff --git a/src-tauri/src/fan_curve.rs b/src-tauri/src/fan_curve.rs index 20fa870..cbe57ed 100755 --- a/src-tauri/src/fan_curve.rs +++ b/src-tauri/src/fan_curve.rs @@ -221,8 +221,6 @@ fn get_cpu_temperature() -> Result { Err("Could not read CPU temperature".to_string()) } -const HELPER_PATH: &str = "/usr/local/bin/thinkutils-fan-control"; - /// How long the firmware watchdog waits before forcing the fan back to auto. /// /// Shared with the helper script's whitelist, which only accepts this exact @@ -270,9 +268,9 @@ pub fn restore_fan_to_auto_blocking() { } } - if std::path::Path::new(HELPER_PATH).exists() { + if let Some(helper) = crate::fan_control::helper_path() { match std::process::Command::new("pkexec") - .arg(HELPER_PATH) + .arg(helper) .arg("level auto") .output() { @@ -314,12 +312,12 @@ async fn write_fan_command(command: &str) -> Result<(), String> { // Use dedicated helper if installed (polkit rule grants passwordless access). // We only check the helper because /etc/polkit-1/rules.d/ is root-only. - if !std::path::Path::new(HELPER_PATH).exists() { + let Some(helper) = crate::fan_control::helper_path() else { return Err("No write permission. Grant permissions to enable fan curve mode.".to_string()); - } + }; let output = tokio::process::Command::new("pkexec") - .arg(HELPER_PATH) + .arg(helper) .arg(command) .output() .await diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index 450b28c..bd6dd45 100755 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -1,7 +1,7 @@ mod auth; mod battery; pub mod environment; -mod fan_control; +pub mod fan_control; mod fan_curve; pub mod hardware_root; mod mcp; diff --git a/src-tauri/src/mcp.rs b/src-tauri/src/mcp.rs index 8546287..2b0cd8b 100644 --- a/src-tauri/src/mcp.rs +++ b/src-tauri/src/mcp.rs @@ -8,8 +8,6 @@ use std::sync::Arc; use tokio::sync::Mutex; use tokio_util::sync::CancellationToken; -const HELPER_PATH: &str = "/usr/local/bin/thinkutils-fan-control"; - const VALID_FAN_SPEEDS: &[&str] = &["auto", "full-speed", "0", "1", "2", "3", "4", "5", "6", "7"]; fn validate_fan_speed(speed: &str) -> Option { @@ -125,12 +123,8 @@ impl ThinkUtilsHandler { if fs::write("/proc/acpi/ibm/fan", &command).is_ok() { return format!("Fan speed set to: {}", req.speed); } - if std::path::Path::new(HELPER_PATH).exists() { - match Command::new("pkexec") - .arg(HELPER_PATH) - .arg(&command) - .output() - { + if let Some(helper) = crate::fan_control::helper_path() { + match Command::new("pkexec").arg(helper).arg(&command).output() { Ok(o) if o.status.success() => return format!("Fan speed set to: {}", req.speed), Ok(o) => return format!("Failed: {}", String::from_utf8_lossy(&o.stderr)), Err(e) => return format!("Error: {}", e), diff --git a/src-tauri/src/permissions.rs b/src-tauri/src/permissions.rs index 02db239..808f53a 100755 --- a/src-tauri/src/permissions.rs +++ b/src-tauri/src/permissions.rs @@ -2,7 +2,10 @@ use serde::{Deserialize, Serialize}; use std::fs; use std::path::Path; -use crate::fan_control::{HELPER_PATH, HELPER_SCRIPT, POLKIT_RULE, POLKIT_RULE_PATH}; +use crate::fan_control::{ + helper_is_packaged, helper_path, polkit_rule, HELPER_SCRIPT, HELPER_SELF_INSTALL_PATH, + POLKIT_RULE_PATH, +}; #[derive(Debug, Serialize, Deserialize)] pub struct ApiResponse { @@ -47,7 +50,7 @@ pub async fn check_permissions_status() -> ApiResponse { // Also check if fan control helper + polkit rule are installed // Only check the helper — /etc/polkit-1/rules.d/ is root-only so // Path::exists() on the polkit rule always fails for normal users. - if !Path::new(HELPER_PATH).exists() { + if helper_path().is_none() { // Can we at least write to the fan file directly? let fan_path = "/proc/acpi/ibm/fan"; if Path::new(fan_path).exists() @@ -119,22 +122,31 @@ pub async fn setup_permissions() -> ApiResponse { script_lines.push("fi".to_string()); } - // Also install the fan control helper + polkit rule so the user - // doesn't have to click "Grant Permissions" again on the fan page. - // Uses shared constants from fan_control module to avoid duplication. - script_lines.push(format!("cat > {} << 'HELPEREOF'", HELPER_PATH)); - script_lines.push(HELPER_SCRIPT.trim().to_string()); - script_lines.push("HELPEREOF".to_string()); - script_lines.push(format!("chmod 755 {}", HELPER_PATH)); - - script_lines.push("mkdir -p /etc/polkit-1/rules.d".to_string()); - script_lines.push(format!("cat > {} << 'RULEEOF'", POLKIT_RULE_PATH)); - script_lines.push(POLKIT_RULE.trim().to_string()); - script_lines.push("RULEEOF".to_string()); - script_lines.push( - "systemctl reload polkit 2>/dev/null || killall -HUP polkitd 2>/dev/null || true" - .to_string(), - ); + // Install the fan helper and polkit rule -- but only when this app owns + // them. On a distro-packaged install both files belong to dpkg/rpm/pacman, + // and rewriting them would put the package database out of sync with the + // filesystem and leave orphans behind on uninstall. + if helper_is_packaged() { + println!("[Permissions] Packaged helper detected; only adjusting sysfs permissions"); + } else { + script_lines.push(format!( + "mkdir -p \"$(dirname {})\"", + HELPER_SELF_INSTALL_PATH + )); + script_lines.push(format!("cat > {} << 'HELPEREOF'", HELPER_SELF_INSTALL_PATH)); + script_lines.push(HELPER_SCRIPT.trim().to_string()); + script_lines.push("HELPEREOF".to_string()); + script_lines.push(format!("chmod 755 {}", HELPER_SELF_INSTALL_PATH)); + + script_lines.push("mkdir -p /etc/polkit-1/rules.d".to_string()); + script_lines.push(format!("cat > {} << 'RULEEOF'", POLKIT_RULE_PATH)); + script_lines.push(polkit_rule().trim().to_string()); + script_lines.push("RULEEOF".to_string()); + script_lines.push( + "systemctl reload polkit 2>/dev/null || killall -HUP polkitd 2>/dev/null || true" + .to_string(), + ); + } script_lines.push("echo 'Permissions setup complete!'".to_string()); script_lines.push("exit 0".to_string()); diff --git a/src-tauri/tests/packaging.rs b/src-tauri/tests/packaging.rs new file mode 100644 index 0000000..d88761b --- /dev/null +++ b/src-tauri/tests/packaging.rs @@ -0,0 +1,161 @@ +//! Keeps the packaging files in lockstep with the Rust source. +//! +//! Three package formats install the fan helper and the polkit rule, and the app +//! looks for them at paths defined in `fan_control::HELPER_CANDIDATES`. If a +//! packaged copy drifts from the constants, the rule grants access to a path the +//! helper is not installed at — and that fails *silently*: polkit denies, the app +//! falls back to a password prompt, and it looks exactly like a permissions +//! problem rather than a packaging bug. +//! +//! Regenerate the derived files with: +//! +//! ```sh +//! cargo run --example gen-packaging -- ../packaging +//! ``` + +use std::path::PathBuf; +use thinkutils_lib::fan_control::{ + polkit_rule, HELPER_CANDIDATES, HELPER_SCRIPT, POLKIT_RULE_PACKAGED_PATH, +}; + +fn repo_file(rel: &str) -> PathBuf { + PathBuf::from(concat!(env!("CARGO_MANIFEST_DIR"), "/..")).join(rel) +} + +fn read(rel: &str) -> String { + std::fs::read_to_string(repo_file(rel)) + .unwrap_or_else(|e| panic!("missing packaging file {}: {}", rel, e)) +} + +/// The committed rule must be exactly what the source generates. Regenerating is +/// one command; letting them diverge costs a silent permissions failure. +#[test] +fn committed_polkit_rule_matches_source() { + assert_eq!( + read("packaging/polkit/50-thinkutils.rules"), + polkit_rule(), + "packaging/polkit/50-thinkutils.rules is stale - regenerate with:\n \ + cargo run --example gen-packaging -- ../packaging" + ); +} + +#[test] +fn committed_helper_matches_source() { + assert_eq!( + read("packaging/helper/thinkutils-fan-control"), + HELPER_SCRIPT, + "packaging/helper/thinkutils-fan-control is stale - regenerate with:\n \ + cargo run --example gen-packaging -- ../packaging" + ); +} + +/// Each package format has its own convention, and each must install to a path +/// the app actually searches. A package installing to an unsearched path +/// produces a working install whose fan control silently never works. +#[test] +fn each_package_installs_the_helper_where_the_app_looks() { + let deb_arch_path = HELPER_CANDIDATES[0]; // /usr/lib/thinkutils/... + let fedora_path = HELPER_CANDIDATES[1]; // /usr/libexec/thinkutils/... + + let pkgbuild = read("packaging/aur/PKGBUILD"); + assert!( + pkgbuild.contains(deb_arch_path.trim_start_matches('/')), + "PKGBUILD must install the helper to {}", + deb_arch_path + ); + + let spec = read("packaging/copr/thinkutils.spec"); + assert!( + spec.contains("%{_libexecdir}/thinkutils/thinkutils-fan-control"), + "spec must install the helper to {} via %{{_libexecdir}}", + fedora_path + ); +} + +/// Distro policy forbids packages writing to /usr/local. A package that does +/// fails lintian/rpmlint and would be rejected outright. +#[test] +fn no_package_writes_to_usr_local() { + for f in ["packaging/aur/PKGBUILD", "packaging/copr/thinkutils.spec"] { + let content = read(f); + for line in content.lines() { + // Comments explain *why* /usr/local is avoided, so only real + // install directives count. + let trimmed = line.trim_start(); + if trimmed.starts_with('#') { + continue; + } + assert!( + !line.contains("/usr/local"), + "{} installs to /usr/local, which distro policy forbids:\n {}", + f, + line + ); + } + } +} + +/// Vendor rules go under /usr/share; /etc belongs to the administrator. A +/// package shipping to /etc creates a conffile it can never cleanly remove. +#[test] +fn packages_ship_the_polkit_rule_under_usr_share() { + let expected_dir = POLKIT_RULE_PACKAGED_PATH + .rsplit_once('/') + .expect("packaged rule path has a directory") + .0; + + for f in ["packaging/aur/PKGBUILD", "packaging/copr/thinkutils.spec"] { + let content = read(f); + assert!( + content.contains("polkit-1/rules.d"), + "{} does not install a polkit rule", + f + ); + assert!( + content.contains(expected_dir.trim_start_matches('/')) + || content.contains("%{_datadir}/polkit-1/rules.d"), + "{} must install the polkit rule under {}", + f, + expected_dir + ); + assert!( + !content.contains("/etc/polkit-1/rules.d"), + "{} must not write into the administrator's /etc namespace", + f + ); + } +} + +/// ThinkPads are x86_64 and thinkpad_acpi is an x86 platform driver. Claiming an +/// architecture with no hardware to run on produces builds nobody can use. +#[test] +fn packages_claim_x86_64_only() { + assert!(read("packaging/aur/PKGBUILD").contains("arch=('x86_64')")); + assert!(read("packaging/copr/thinkutils.spec").contains("ExclusiveArch: x86_64")); +} + +/// The version appears in the PKGBUILD and the spec as well as the four files +/// CLAUDE.md lists, so bump-version.sh has more to keep in step than it did. +#[test] +fn packaging_versions_match_the_manifest() { + let pkg_json = read("package.json"); + let version = pkg_json + .lines() + .find_map(|l| { + let l = l.trim(); + l.strip_prefix("\"version\":") + .map(|v| v.trim().trim_matches(|c| c == '"' || c == ',').to_string()) + }) + .expect("package.json has a version"); + + assert!( + read("packaging/aur/PKGBUILD").contains(&format!("pkgver={}", version)), + "PKGBUILD pkgver does not match package.json ({})", + version + ); + assert!( + read("packaging/copr/thinkutils.spec").contains(&format!("Version: {}", version)), + "spec Version does not match package.json ({})", + version + ); +} From 2ef76c009b3be568012cc9aede69c7aa622756d6 Mon Sep 17 00:00:00 2001 From: Viet Anh Nguyen Date: Sun, 19 Jul 2026 20:29:44 +0700 Subject: [PATCH 4/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 d88761b..08d15df 100644 --- a/src-tauri/tests/packaging.rs +++ b/src-tauri/tests/packaging.rs @@ -159,3 +159,65 @@ fn packaging_versions_match_the_manifest() { version ); } + +/// 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 6675a19e1ebea7fc4bf56a1e75b9d59a3d829519 Mon Sep 17 00:00:00 2001 From: Viet Anh Nguyen Date: Sun, 19 Jul 2026 20:33:40 +0700 Subject: [PATCH 5/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 c1a4658..48dd122 100755 --- a/src-tauri/src/performance.rs +++ b/src-tauri/src/performance.rs @@ -128,7 +128,6 @@ pub async fn set_cpu_governor(governor: String) -> ApiResponse { println!("[Performance] Found {} CPU cores", cpu_count); // Create script to set governor for all CPUs - let temp_script = format!("/tmp/set_governor_{}.sh", std::process::id()); let mut script_content = String::from("#!/bin/bash\nset -e\n"); for i in 0..cpu_count { @@ -141,32 +140,10 @@ pub async fn set_cpu_governor(governor: String) -> ApiResponse { 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); @@ -196,7 +173,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, @@ -373,28 +349,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, @@ -411,28 +372,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 5165c8b0835b606e9ab139ad5945dceb9330f5a8 Mon Sep 17 00:00:00 2001 From: Viet Anh Nguyen Date: Sun, 19 Jul 2026 20:44:21 +0700 Subject: [PATCH 6/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 | 24 +++++--- 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, 230 insertions(+), 45 deletions(-) diff --git a/docs/guide/mcp.md b/docs/guide/mcp.md index 890dc1e..4f36a9e 100644 --- a/docs/guide/mcp.md +++ b/docs/guide/mcp.md @@ -4,6 +4,14 @@ ThinkUtils includes a built-in MCP (Model Context Protocol) server that exposes ![AI Integration](/screenshots/ai_integration.png) + +::: tip Port changed in a recent release +The default MCP port is now **8779**. It was 8765, which collided with the +port the Google Drive sync login uses for its OAuth callback — with the MCP +server running, sign-in would silently never complete. If you configured a +client against 8765, update it or set the port back on the AI Integration page. +::: + ## 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 +36,7 @@ Start the MCP server from the app's MCP page, then configure your AI tool: ### Claude Code ```bash -claude mcp add --transport sse thinkutils http://127.0.0.1:8765/sse +claude mcp add --transport sse thinkutils http://127.0.0.1:8779/sse ``` Or add to `.mcp.json` in your project: @@ -38,7 +46,7 @@ Or add to `.mcp.json` in your project: "mcpServers": { "thinkutils": { "type": "sse", - "url": "http://127.0.0.1:8765/sse" + "url": "http://127.0.0.1:8779/sse" } } } @@ -52,7 +60,7 @@ Add to `~/.config/Claude/claude_desktop_config.json`: { "mcpServers": { "thinkutils": { - "url": "http://127.0.0.1:8765/sse" + "url": "http://127.0.0.1:8779/sse" } } } @@ -66,7 +74,7 @@ Add to `.cursor/mcp.json` (project) or `~/.cursor/mcp.json` (global): { "mcpServers": { "thinkutils": { - "url": "http://127.0.0.1:8765/sse" + "url": "http://127.0.0.1:8779/sse" } } } @@ -80,7 +88,7 @@ Add to `~/.codeium/windsurf/mcp_config.json`: { "mcpServers": { "thinkutils": { - "url": "http://127.0.0.1:8765/sse" + "url": "http://127.0.0.1:8779/sse" } } } @@ -94,7 +102,7 @@ Add to `~/.lmstudio/mcp.json`: { "mcpServers": { "thinkutils": { - "url": "http://127.0.0.1:8765/sse" + "url": "http://127.0.0.1:8779/sse" } } } @@ -107,7 +115,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/sse` +- **Server URL**: `http://127.0.0.1:8779/sse` ::: info Requires ChatGPT Desktop with MCP support (Plus/Team/Enterprise). @@ -115,4 +123,4 @@ Requires ChatGPT Desktop with MCP support (Plus/Team/Enterprise). ### Other Tools -For any MCP-compatible client, configure an SSE server with URL `http://127.0.0.1:8765/sse`. +For any MCP-compatible client, configure an SSE server with URL `http://127.0.0.1:8779/sse`. 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 2b0cd8b..f9a0d5b 100644 --- a/src-tauri/src/mcp.rs +++ b/src-tauri/src/mcp.rs @@ -8,6 +8,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 { @@ -68,7 +75,7 @@ impl Default for McpServerState { Self { cancel_token: None, host: "127.0.0.1".to_string(), - port: 8765, + port: DEFAULT_MCP_PORT, } } } @@ -166,7 +173,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"), ) } @@ -175,18 +182,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)), } @@ -511,11 +519,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 84c1df3..8227cb4 100755 --- a/src-tauri/src/sync.rs +++ b/src-tauri/src/sync.rs @@ -17,6 +17,16 @@ use std::sync::{Arc, Mutex}; // Create credentials at https://console.cloud.google.com (APIs & Services > Credentials). const GOOGLE_CLIENT_ID: Option<&str> = option_env!("THINKUTILS_GOOGLE_CLIENT_ID"); const GOOGLE_CLIENT_SECRET: Option<&str> = option_env!("THINKUTILS_GOOGLE_CLIENT_SECRET"); +/// 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)] @@ -245,8 +255,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); @@ -687,3 +697,22 @@ mod tests { let _ = fs::remove_file(&path); } } + +#[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 0646036..1dfc6df 100644 --- a/src/js/views/mcp.js +++ b/src/js/views/mcp.js @@ -147,7 +147,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 ad0bcad..04b88b7 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/sse

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

Setup Instructions

Run this command:

-claude mcp add --transport sse 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/sse" } } }Setup Instructions { "mcpServers": { "thinkutils": { - "url": "http://127.0.0.1:8765/sse" + "url": "http://127.0.0.1:8779/sse" } } }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/sse" } } }Setup Instructions { "mcpServers": { "thinkutils": { - "url": "http://127.0.0.1:8765/sse" + "url": "http://127.0.0.1:8779/sse" } } }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/sse
 Transport: SSE (Server-Sent Events)

From 45ad34966afb5f115503a7afc9dc4dd10ce6858e Mon Sep 17 00:00:00 2001 From: Viet Anh Nguyen Date: Sun, 19 Jul 2026 21:49:48 +0700 Subject: [PATCH 7/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. --- scripts/test-gui-packages-docker.sh | 10 +- 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 - 17 files changed, 361 insertions(+), 196 deletions(-) create mode 100644 src/js/dialog.js create mode 100644 src/js/views/registry.js diff --git a/scripts/test-gui-packages-docker.sh b/scripts/test-gui-packages-docker.sh index f1a6b51..94173db 100755 --- a/scripts/test-gui-packages-docker.sh +++ b/scripts/test-gui-packages-docker.sh @@ -74,7 +74,15 @@ shopt -s nullglob # directory that accumulates old builds. Resolving by glob alone would silently # test whatever sorted first -- and `ls | head -1` sorts "0.1.10" before "0.1.5", # so "newest" and "first" are not the same thing. -VERSION="$(jq -r .version "${REPO_ROOT}/package.json")" +VERSION="$(jq -r .version "${REPO_ROOT}/package.json" 2>/dev/null || true)" +if [ -z "${VERSION}" ] || [ "${VERSION}" = "null" ]; then + # Without this the version interpolates as empty and every glob becomes + # something like thinkutils__amd64.deb, which matches nothing -- so the run + # reports "no artifact" and the real cause (jq missing) stays hidden. + echo "ERROR: could not read .version from package.json" >&2 + command -v jq >/dev/null || echo " jq is not installed" >&2 + exit 1 +fi # Resolve the artifact for the CURRENT version. Never a hardcoded version (it # would stop matching and silently test nothing) and never just "the only one" 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 04b88b7..1e7baff 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

-
-