refactor(ui) + security: view lifecycle, one implementation per control, and the pkexec/XSS fixes by vietanhdev · Pull Request #40 · vietanhdev/ThinkUtils

vietanhdev · 2026-07-20T03:22:06Z

Supersedes #27 and #28, which were split across two branches that both needed rebasing onto the same files. This is the combined, rebased work — 115 tests, clippy clean, lint + format clean.

Security

One safe path for running a script as root. Five call sites built a /tmp script path from std::process::id(), wrote it with plain fs::write (no O_EXCL, happily reusing an existing file), and discarded the set_permissions result with let _. On a shared machine an unprivileged user can pre-create every candidate path — the PID space is 32768 — as a file they own, then rewrite its contents between the write and pkexec bash <path>. That executes attacker-controlled bash as root. privileged.rs now owns the one implementation: random name, O_EXCL, mode 0600, always cleaned up.

Escaped untrusted strings and set a real CSP. Backend-derived strings (process names, sysfs contents) reached innerHTML.

MCP stopped stealing the OAuth port. The MCP server defaulted to 8765 — the same port sync.rs uses for the OAuth callback. While MCP was running the callback listener couldn't bind and Google login failed with no visible error. Now 8779, with a test asserting the two never converge.

Battery thresholds are actually grantable. permissions.rs granted access to one attribute spelling while battery.rs wrote the other, so "Grant Permissions" reported success and every threshold change still fell through to a password prompt. Both now resolve through battery::threshold_paths().

The last threshold callers (final commit)

mcp.rs's get_battery_info still named the generic pair directly, reporting N/A on machines exposing only thinkpad_acpi's legacy spelling — thresholds its own setter could write fine. And permissions.rs listed only intel_pstate/no_turbo, so on AMD ThinkPads (cpufreq/boost) turbo was never made writable.

tests/threshold_attribute_names.rs now enforces the rule the doc comment only asked for. It lives in its own integration-test file and builds the literals from fragments at runtime, because a guard that greps source for a string it also contains matches itself — which has bitten this repo repeatedly. A third test verifies the scanner can find those literals in the one file guaranteed to have them, so a clean result elsewhere means something. Mutation-verified.

Refactor

View lifecycle — views register in registry.js with setup/teardown, so intervals and Tauri listeners stop accumulating on every navigation
Accessible dialogs — dialog.js: focus trap, Escape to close, focus restored on dismiss
One implementation each for hardware controls (hardwareControls.js) and log panels (logPanel.js), replacing per-view copies
Docs in the UI — each control says what it does and what it costs

Note on the rebase

This rebased over #37, which fixed the MCP serve path independently. The conflict was resolved to take both: #37's type: "http" and backend-supplied path, plus this branch's port 8779. Verified both survived.

cloudflare-workers-and-pages · 2026-07-20T03:23:31Z

Deploying with Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status	Name	Latest Commit	Updated (UTC)
❌ Deployment failed View logs	thinkutils	`4e56c7c`	Jul 20 2026, 03:40 AM

vietanhdev · 2026-07-20T03:28:52Z

Pushed three more fixes from a frontend audit (ba7b2b5):

Canvas hit-testing was broken at small window sizes. The fan curve canvas has a fixed 600×400 backing store but is styled max-width: 100%; height: auto. The mouse handlers compared getBoundingClientRect coordinates — CSS pixels — against tempToX/levelToY, which are built from canvas.width. Once the rendered width drops below 600 the two spaces diverge.

Not a corner case: the window's own minWidth is 700px, and the nav rail plus status sidebar take ~470px of it, so at minimum size the canvas renders at roughly half its backing width. Curve points became ungrabbable, and a click near one latched a different point and flung it to the wrong temperature.

A single missing template aborted the whole boot. setupFanControl and its siblings dereference cached elements unguarded, so one undefined threw and every later setup call, the permission check, and settings loading never ran — while setupFeatureNavigation, wired one line earlier, still switched views. The app looked alive with every control inert. The template-failure handler's "continue anyway" comment described behaviour that did not exist.

Each step is isolated now. But that makes reaching ready a weaker signal than before, so the ready report carries which steps failed and the container launch test fails on a non-empty list — otherwise this would trade a loud failure for a silent one.

Home showed placeholders for its first two seconds, because switchView() only runs from a sidebar click and nothing painted the starting view.

115 tests, clippy clean, lint + format clean.

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 `<img src=x onerror=...>`. 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.

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.

…th 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.

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.

…anels Home and Performance implemented the same three controls twice, and the copies had drifted: Home disabled its governor buttons during the call and waited 500ms before reading back; Performance did neither, so a fast double-click could fire two governor changes -- each spawning a pkexec -- and the read-back could land before the kernel had applied the first. Home rebound its turbo handler on every render without removing the old one, so the toggle fired once per previous render. Performance bound once. The two used different wording for the same successful action. hardwareControls.js is now the single implementation. runAction() gives every privileged call the same status reporting, disables the controls it is told are busy, and always refreshes -- so a failure cannot leave the UI showing a state that was never applied. A failed turbo write flips the checkbox back, which only one of the two copies did. security.js carried two near-identical log panels, ~230 lines differing only in element prefix and reveal delay. They also shared a bug: each scheduled one setTimeout per log line and nothing cancelled them, so starting a second scan cleared the output while the first run's timers kept firing into it. A 2000-line scan queued 2000 timers spanning a minute. logPanel.js replaces both and cancels pending reveals on start and update. Lines are set with textContent -- they carry scanner output and file paths -- and the collapse button now reports aria-expanded. security.js 675 -> 469 lines; performance.js 125 -> 85. Verified with the container launch test rather than by inspection: a broken control binding still paints a full UI, so only running it proves anything.

The descriptions restated the system's own vocabulary -- 'Control CPU frequency scaling policy', 'System-wide power management profile' -- which tells someone who already knows what a governor is nothing they did not know, and someone who does not know nothing at all. Rewritten around what the user controls and the tradeoff they are making: powersave is quieter and lasts longer, performance responds faster and runs hotter. Turbo boost off caps peak speed but noticeably reduces heat and fan noise. Battery thresholds trade runtime per charge for battery lifespan, with the practical note to raise the limit before travelling. Fan control had no explanation at all, and it is the one that can damage hardware. Two additions: The control mode section now states that manual control overrides the firmware's thermal management, and that the app hands the fan back on mode change, on sensor failure and on exit, with the firmware watchdog armed meanwhile. That behaviour exists but was invisible, so users had no reason to trust manual mode. The slider says levels are 0-7 rather than a percentage -- the firmware picks the RPM -- and that level 0 stops the fan entirely, which is safe only at light load. Verified the templates still inject and paint via the container launch test; a malformed template would leave the view empty.

battery.rs already probed for whichever attribute pair a machine exposes -- the kernel's charge_control_* or thinkpad_acpi's older charge_* -- and its doc comment said every caller must go through it. Two did not. mcp.rs's get_battery_info named the generic pair directly, so on a machine exposing only the legacy spelling it reported N/A for thresholds its own setter could write successfully. permissions.rs listed only intel_pstate/no_turbo for turbo boost. AMD machines expose cpufreq/boost instead, so "Grant Permissions" reported success while leaving turbo unwritable there. Both paths are listed now and non-existent ones are skipped, so each machine gets the one it has. Adds tests/threshold_attribute_names.rs to enforce the rule the doc comment only asked for. It lives in its own integration-test file and builds the literals from fragments at runtime, because a guard that greps source for a string it also contains matches itself -- which has already happened repeatedly here. A third test checks the scanner can find those literals in the one file guaranteed to have them, so a clean result elsewhere means something. Mutation-verified: putting a hardcoded name back in mcp.rs fails the guard, reporting the file and line. 111 tests.

Three defects from a frontend audit. The fan curve canvas has a fixed 600x400 backing store but is styled max-width: 100%; height: auto. Mouse handlers compared getBoundingClientRect coordinates -- CSS pixels -- against tempToX/levelToY, which are built from canvas.width. The moment the rendered width drops below 600 the two spaces diverge. That is not a corner case: the window's own minWidth is 700px and the nav rail plus status sidebar take ~470px of it, so at minimum size the canvas renders at roughly half its backing width. Curve points became ungrabbable, and a click near one latched a different point and flung it to the wrong temperature. One canvasPoint() helper now scales both axes. setupFanControl and its siblings dereference cached elements with no guard, so a single missing template threw and took the rest of init with it -- every later setup call, the permission check and settings loading never ran, while the sidebar wired one line earlier still switched views. The app looked alive with every control inert, and the template-failure handler claimed to "continue anyway" while doing nothing of the sort. Each step is isolated now, so a broken view costs that view rather than the application. Because that makes reaching "ready" a weaker signal than before, the ready report carries which steps failed, and the container launch test fails on a non-empty list. A partially wired app paints exactly like a working one, so without that this would trade a loud failure for a silent one. Also paints the starting view at boot: switchView() only runs from a sidebar click, so Home showed template placeholders until the first 2s tick.

The MCP toggle read its own button label to decide what to do: const isRunning = btn.textContent === 'Stop Server'; loadMcpStatus only relabels the button when the status call succeeds, and its catch merely logs. So one failed status read left the label stuck on "Starting..." while the server was in fact running. The next click evaluated that as "not running" and issued a second start against the already-bound port -- reporting "Address already in use" for a healthy server, with Stop unreachable short of restarting the app. State now comes from the backend and is explicitly null when unknown, in which case the toggle resolves it before acting instead of guessing. A failed read says so in the UI rather than leaving the previous render up. Separately, Home's 2s poll reassigned the turbo checkbox from sysfs while the user's own change was still waiting on pkexec. sysfs still reports the old value at that point, so the toggle flipped back under them while the password prompt was open. It corrected itself afterwards, so nothing stayed wrong -- it just read as the app rejecting the click. Refreshes now leave a control alone while its privileged write is in flight. 115 tests, lint and format clean.

vietanhdev · 2026-07-20T03:39:34Z

Rebased onto current main (which now has #30/#32/#33/#34/#35/#36/#37/#38/#39). Three conflicts, all resolved to keep both sides — worth recording because one of them would have silently reverted a merged fix:

performance.rs — this branch predates #38, so it still carried the old count-based governor loop (for i in 0..cpu_count under set -e). A naive resolution would have reverted #38's offline-CPU fix. Resolved to governor_script(&governor, CPU_GLOB) from #38 executed via privileged::run_script from this branch — both fixes, not either.

sync.rs — this branch re-added GOOGLE_CLIENT_SECRET, which #32 deliberately removed. Kept OAUTH_CALLBACK_PORT and dropped the secret; the_client_secret_env_var_is_gone confirms. Both test modules (public_client_tests from #32, port_tests from here) kept.

packaging.rs — #34's dialog tests and this branch's CSP/escaping tests are independent; git only conflicted because they're adjacent. Kept both, 16 tests pass.

I found these by merging all four PRs onto a scratch branch first — none of them appear in any individual PR's CI, since each PR is only ever tested against main alone.

Verification: 128 tests, clippy clean, fmt clean, lint + format clean. Spot-checked that every merged fix is still present: governor_script(&governor, CPU_GLOB), privileged::run_script, watchdog_due, nest_service(MCP_PATH, OAUTH_CALLBACK_PORT, Policy::none, plus this branch's canvasPoint and isControlBusy.

This was referenced Jul 20, 2026

security: close the privesc chain, harden pkexec, fix two silent failures #27

Closed

refactor(ui): view lifecycle, accessible dialogs, and one implementation per control #28

Closed

vietanhdev added 9 commits July 20, 2026 10:37

vietanhdev force-pushed the frontend/ui-and-security-combined branch from ea28e18 to 4e56c7c Compare July 20, 2026 03:39

vietanhdev merged commit dc9c313 into main Jul 20, 2026
9 of 10 checks passed

vietanhdev deleted the frontend/ui-and-security-combined branch July 20, 2026 03:50

Navigation Menu

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

refactor(ui) + security: view lifecycle, one implementation per control, and the pkexec/XSS fixes#40
vietanhdev merged 9 commits into
mainfrom
frontend/ui-and-security-combined

vietanhdev commented Jul 20, 2026

Uh oh!

cloudflare-workers-and-pages Bot commented Jul 20, 2026 •

edited

Loading

Uh oh!

vietanhdev commented Jul 20, 2026

Uh oh!

vietanhdev commented Jul 20, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

vietanhdev commented Jul 20, 2026

Security

The last threshold callers (final commit)

Refactor

Note on the rebase

Uh oh!

cloudflare-workers-and-pages Bot commented Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deploying with Cloudflare Workers

Uh oh!

vietanhdev commented Jul 20, 2026

Uh oh!

vietanhdev commented Jul 20, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

cloudflare-workers-and-pages Bot commented Jul 20, 2026 •

edited

Loading