diff --git a/CHANGELOG.md b/CHANGELOG.md index 29462bd..e240a19 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,32 @@ Notable changes to WinButler, by internal milestone. The current shipping version is **1.0.1**. +## v1.0.2 — 2026-07-29 · Ghost-device cleanup + Activision/CoD fix + +- **Ghost-device removal on the System Tools page**: "List ghost devices" (read-only) surfaces + non-present PnP device nodes via `pnputil.exe`. "Remove ghost devices" (Advanced) is + **permanent and cannot be undone** — to avoid touching live hardware that can also show as + "disconnected" (disks, GPU-integrated controllers, VSS snapshots, virtual/software device + stubs — all observed on real hardware during development), it only ever removes an + allow-listed shape of device (USB/HID devices by vendor+product ID, Bluetooth, audio + endpoints), explicitly excluding USB root hubs. The removal action runs an embedded PowerShell + script entirely in memory (`-EncodedCommand`, never written to disk) — see + `Services/EmbeddedScript.cs`. Credit to the original "remove ghost devices natively with + PowerShell" concept from theorypc.ca (2017) — see README Acknowledgements. +- **Script-backed System Tools actions are now data-driven**: they're declared in + `Scripts/scripts.json` and auto-register, so adding one is a drop-in — write a `.ps1`, add an + entry, no code change (see `Scripts/README.md`). The manifest only ever *names* a script embedded + in the binary plus a bare-identifier mode; it can't carry a command line, and it's loaded outside + the definitions merge path so a future remote-definitions rollout could never reach it. Built-in + Windows-tool actions (DISM, SFC, WMI reset, …) stay defined in code for the same reason. +- **Fixed a data-loss bug in the Activision/Call of Duty cleanup rule**: the old + `activision-crashes` entry treated every immediate child of `%LocalAppData%\Activision` — + including all of `Call of Duty`, which can hold `Call of Duty\players` (real user settings) — + as permanently-deletable junk. Replaced with two narrower entries: one scoped to + `Call of Duty` itself (now Recycle-Bin risk, not permanent, and excludes `players` via a new + `exclude` field on known-location rules), and one scoped to the bootstrapper's crash-reports + folder specifically. + ## v1.0.1 — 2026-07-18 · Program Files installer - **The installer is now a per-machine MSI** (`WinButler-win.msi`) that installs to diff --git a/Data/definitions/README.md b/Data/definitions/README.md index 07926a6..5972d18 100644 --- a/Data/definitions/README.md +++ b/Data/definitions/README.md @@ -41,6 +41,7 @@ deletion. Partial loads are never accepted — validate your JSON before committ "mode": "children", // children | files | self "pattern": "*.dmp", // files mode only: wildcard filter "recursive": true, // files mode only: recurse subdirs + "exclude": ["players"], // children mode only: child names to always skip "allDrives": false, // path is relative to every fixed drive root "risk": "safe", // safe | caution | risky "displayName": "Discord cache", @@ -50,6 +51,10 @@ deletion. Partial loads are never accepted — validate your JSON before committ ``` - **`mode: children`** — every immediate child of `path` is a delete target; the directory itself survives. + Optionally set `exclude` (child *names*, case-insensitive, not full paths) to skip specific children + even though they'd otherwise match — use this when a folder mixes junk with data that must never be + offered (e.g. a game's crash-report folder that also holds a `players` settings subfolder). Ignored + outside `children` mode. - **`mode: files`** — files under `path` matching `pattern` (optionally `recursive`) are targets. - **`mode: self`** — `path` itself is the target (a specific junk folder or file). - **`risk`** drives deletion policy: `safe` → deleted permanently; `caution`/`risky` → sent to the Recycle Bin and never auto-selected. Use `risky` for anything a user might miss (local edit history, package stores that are slow to rebuild). diff --git a/Data/definitions/games.json b/Data/definitions/games.json index 09c7e01..5e6b8d7 100644 --- a/Data/definitions/games.json +++ b/Data/definitions/games.json @@ -16,7 +16,8 @@ { "id": "arma3-reports", "path": "%LocalAppData%\\Arma 3", "mode": "files", "pattern": "*.rpt", "recursive": false, "risk": "safe", "displayName": "Arma 3 report logs", "description": "Arma 3 .rpt session logs", "group": "Games" }, { "id": "arma3-dumps", "path": "%LocalAppData%\\Arma 3", "mode": "files", "pattern": "*.mdmp", "recursive": false, "risk": "safe", "displayName": "Arma 3 crash dumps", "description": "Arma 3 minidumps", "group": "Games" }, { "id": "cod-mw-archive", "path": "%Documents%\\Call of Duty Modern Warfare\\archive", "mode": "children", "risk": "safe", "displayName": "CoD MW crash archive", "description": "Call of Duty crash archives", "group": "Games" }, - { "id": "activision-crashes", "path": "%LocalAppData%\\Activision", "mode": "children", "risk": "safe", "displayName": "Activision crash reports","description": "Activision crash-report folders", "group": "Games" }, + { "id": "activision-cod-crashes", "path": "%LocalAppData%\\Activision\\Call of Duty", "mode": "children", "exclude": ["players"], "risk": "caution", "displayName": "Call of Duty crash/report data", "description": "CoD crash/report folders under Activision (Recycle Bin, not permanent — the players settings folder is excluded, but the full junk-folder set under Call of Duty hasn't been verified against a real install)", "group": "Games" }, + { "id": "activision-bootstrapper-crashes","path": "%LocalAppData%\\Activision\\bootstrapper\\crash_reports","mode": "children", "risk": "safe", "displayName": "Activision bootstrapper crashes", "description": "Activision launcher bootstrapper crash reports", "group": "Games" }, { "id": "bf2042-crashdumps", "path": "%Documents%\\Battlefield 2042\\CrashDumps", "mode": "children", "risk": "safe", "displayName": "Battlefield 2042 crash dumps","description": "BF2042 crash dumps", "group": "Games" }, { "id": "bf6-crashdumps", "path": "%Documents%\\Battlefield 6\\CrashDumps", "mode": "children", "risk": "safe", "displayName": "Battlefield 6 crash dumps","description": "BF6 crash dumps", "group": "Games" }, { "id": "bf4-twinkle", "path": "%Documents%\\Battlefield 4\\twinkle", "mode": "children", "risk": "safe", "displayName": "Battlefield 4 web assets","description": "In-game browser assets (~200 MB)", "group": "Games" }, diff --git a/Models/RuleDefinitions.cs b/Models/RuleDefinitions.cs index 6968c87..ac31185 100644 --- a/Models/RuleDefinitions.cs +++ b/Models/RuleDefinitions.cs @@ -142,6 +142,11 @@ public sealed class KnownLocationEntry /// files mode only: recurse into subdirectories (junctions are not followed). public bool Recursive { get; set; } + /// children mode only: child names (case-insensitive, not full paths) to skip + /// even though they'd otherwise match — an additional per-rule carve-out alongside the + /// deny-list, for a folder that mixes junk with data that must never be offered. + public List? Exclude { get; set; } + /// When true, is resolved against every fixed drive root. public bool AllDrives { get; set; } diff --git a/Models/ScriptActionEntry.cs b/Models/ScriptActionEntry.cs new file mode 100644 index 0000000..21100f0 --- /dev/null +++ b/Models/ScriptActionEntry.cs @@ -0,0 +1,44 @@ +using System.Collections.Generic; + +namespace WinButler.Models; + +/// The parsed Scripts/scripts.json manifest — the script-backed half of the System +/// Tools catalog. See Scripts/README.md for the field reference. +public sealed class ScriptActionManifest +{ + public List Actions { get; set; } = new(); +} + +/// +/// One script-backed declared in Scripts/scripts.json. This is +/// metadata plus a reference to an embedded script — deliberately never a command line, an +/// executable name, or raw PowerShell. must resolve to a .ps1 embedded +/// from Scripts/ and must be a bare identifier, so the set of things this +/// manifest can execute is fixed at compile time. See 's note on why +/// executable commands are never data-driven. +/// +public sealed class ScriptActionEntry +{ + /// Unique id within the manifest; also the key tests and logs refer to. + public string Id { get; set; } = ""; + + public string Name { get; set; } = ""; + public string Description { get; set; } = ""; + + /// Extra caution shown in the confirm modal. Required unless . + public string Warning { get; set; } = ""; + + /// File name of an embedded Scripts/*.ps1 (e.g. "RemoveGhostDevices.ps1"). + public string Script { get; set; } = ""; + + /// Optional bare identifier passed to the script as $Mode, letting one script + /// back several actions (e.g. a read-only "List" preview and the real "Remove"). + public string? Mode { get; set; } + + /// Read-only actions change nothing, so they run for real even in dry-run and never + /// prompt for confirmation. + public bool IsReadOnly { get; set; } + + /// Groups the action under the UI's "Advanced" divider with the strongest warnings. + public bool IsAdvanced { get; set; } +} diff --git a/Models/SystemAction.cs b/Models/SystemAction.cs index 43286f8..31f71b9 100644 --- a/Models/SystemAction.cs +++ b/Models/SystemAction.cs @@ -5,9 +5,12 @@ namespace WinButler.Models; /// One external command to run (file + arguments), the unit a /// executes. These are defined in code, never in the editable definitions JSON — executable /// commands must not be data-driven. -public sealed record SystemCommand(string FileName, string Arguments) +public sealed record SystemCommand(string FileName, string Arguments, string? DisplayOverride = null) { - public string Display => string.IsNullOrEmpty(Arguments) ? FileName : $"{FileName} {Arguments}"; + /// What the dry-run preview and the runner's "> ..." line show. Defaults to + /// "FileName Arguments"; set when Arguments isn't human-readable + /// (e.g. a base64 -EncodedCommand payload — see ). + public string Display => DisplayOverride ?? (string.IsNullOrEmpty(Arguments) ? FileName : $"{FileName} {Arguments}"); } /// diff --git a/README.md b/README.md index 93af28c..2fef394 100644 --- a/README.md +++ b/README.md @@ -127,6 +127,16 @@ began life as a study of [FocusedWolf](https://www.reddit.com/user/FocusedWolf/) comprehensive Windows cleanup batch script, which WinButler absorbed into its native, rule-driven (and dry-run-guarded) form. Thank you! +The System Tools page's ghost-device removal is credited to the "remove ghost devices +natively with PowerShell" concept originally published at +[theorypc.ca](https://web.archive.org/web/2020/https://theorypc.ca/2017/06/28/remove-ghost-devices-natively-with-powershell/) +(2017) by TrententTye / Alexander Boersch — the live page now returns a 403, hence the +Wayback Machine link. An unofficial third-party fork with additional flags exists at +[github.com/istvans/scripts](https://github.com/istvans/scripts) (not the source of +WinButler's implementation). WinButler's version is a from-scratch reimplementation built +on `pnputil.exe`'s native device-management flags rather than the original's SetupAPI/CfgMgr32 +P/Invoke approach, which predates those flags. + ## License MIT — see [LICENSE](LICENSE). diff --git a/Scripts/README.md b/Scripts/README.md new file mode 100644 index 0000000..12365fc --- /dev/null +++ b/Scripts/README.md @@ -0,0 +1,85 @@ +# WinButler scripts + +PowerShell backing the System Tools page's script-based actions. Every `.ps1` here and +`scripts.json` are **embedded in the assembly** (`WinButler.csproj`) and run in memory — see +"Never written to disk" below. + +## Adding a script + +Two steps, no code change: + +1. Drop `YourScript.ps1` in this folder. +2. Add an entry to `scripts.json`. + +It auto-registers on the System Tools page next build (`Services/ScriptCatalog.cs`). + +```json +{ + "id": "my-action", // unique in this file; the key logs and tests use + "name": "Do the thing", // button row title + "description": "What it does.", // button row subtitle + "warning": "Why it's risky.", // shown in the confirm modal — REQUIRED unless isReadOnly + "script": "YourScript.ps1", // must be a .ps1 embedded from this folder + "mode": "Remove", // optional; assigned to $Mode before the script body + "isReadOnly": false, // true → runs even in dry-run, never prompts (changes nothing) + "isAdvanced": true // true → grouped under the "Advanced" divider +} +``` + +| Field | Rule | +|-------|------| +| `id` | Required, unique within this file. | +| `name`, `description` | Required, non-empty. | +| `warning` | **Required unless `isReadOnly`.** A destructive action must state its own risk — the confirm modal shows this. | +| `script` | Required. Must match `^[A-Za-z0-9._-]+\.ps1$` **and** resolve to a `.ps1` embedded from this folder. | +| `mode` | Optional. Must be a bare identifier (`^[A-Za-z][A-Za-z0-9]*$`). | +| `isReadOnly`, `isAdvanced` | Optional, default `false`. | + +**`isReadOnly` means "changes nothing"**, not "is quick" — it makes the action bypass both the +dry-run guard and the confirm modal. Only set it on an action that genuinely cannot mutate anything. + +### One script, several actions + +Use `mode` to back several actions with one script — `RemoveGhostDevices.ps1` does this, exposing a +read-only `List` preview and the real `Remove`. Because both run the *same* classification code, +the preview cannot drift out of sync with what the destructive action actually does: + +```powershell +if (-not $Mode) { $Mode = 'Remove' } # default when no mode is declared +``` + +## Never put commands in this JSON + +`scripts.json` carries **metadata plus a reference to a script**. It must never contain a command +line, an executable name, or raw PowerShell. Two reasons, both load-bearing: + +- **WinButler always runs elevated** (`requireAdministrator`). Anything expressible in data becomes + something that runs as administrator. +- Rule definitions under `Data/definitions/` can, by design, be overlaid at runtime from a remote + URL (`Services/Definitions/RemoteDefinitionSource.cs`, merged via `DefinitionsProvider.AddSource` + — currently unused, but the plumbing exists and is public). This manifest is deliberately loaded + by `ScriptCatalog` from its own embedded resource, **outside** that merge path, so it can never + be reached that way. + +The validation above is what keeps that true: `script` must name something already compiled into +the binary, and `mode` is restricted to letters and digits so it cannot escape the `$Mode = '…'` +assignment it is interpolated into. The result is that this file can only ever select among scripts +that shipped with the app — it can never introduce new executable content. + +See `Models/SystemAction.cs` for the same rule applied to the built-in Windows-tool actions (DISM, +SFC, `wevtutil`, …), which stay defined in C# for exactly this reason. + +## Never written to disk + +`Services/EmbeddedScript.cs` runs these via `powershell.exe -NoProfile -EncodedCommand `, +reading the script straight out of the assembly. It is never extracted to a temp file or to +`%APPDATA%`. Those locations are user-writable, so an unprivileged process could overwrite the +script between write and execute and have WinButler run it as administrator. + +## Fail-closed + +If `scripts.json` is missing, malformed, or **any** entry fails validation, the whole manifest is +rejected: zero script actions register and the error goes to `%APPDATA%\WinButler\logs\winbutler.log`. +It is all-or-nothing on purpose — a partial load could register a destructive action while dropping +the read-only preview that makes it safe to use. Built-in C# actions are unaffected, so the System +Tools page still works. diff --git a/Scripts/RemoveGhostDevices.ps1 b/Scripts/RemoveGhostDevices.ps1 new file mode 100644 index 0000000..214c73e --- /dev/null +++ b/Scripts/RemoveGhostDevices.ps1 @@ -0,0 +1,62 @@ +<# + Removes "ghost" (non-present) PnP devices -- device nodes Windows keeps around after the + underlying hardware is gone (uninstalled, unplugged, swapped) but never cleans up itself. + + Concept credited to the "remove ghost devices natively with PowerShell" technique originally + published at theorypc.ca (2017) by TrententTye / Alexander Boersch. The live page now returns + HTTP 403; archived copy via the Wayback Machine. An unofficial third-party fork with + additional flags exists at github.com/istvans/scripts -- not the source of this script. + + This is WinButler's own reimplementation: the original technique P/Invoked SetupAPI/CfgMgr32 + directly because no built-in tool exposed ghost-device removal in 2017; pnputil.exe's + /enum-devices and /remove-device flags now do, so this uses those instead. + + THIS IS DESTRUCTIVE AND HAS NO UNDO. pnputil's "/enum-devices /disconnected" reports every + PnP node Windows currently considers non-present -- verified against a real machine, that set + includes not just dead/removed peripherals but also live, currently-installed components that + are non-present for unrelated reasons: disk-drive PnP nodes for real mounted disks, a GPU's + integrated USB-C/HD-audio controller nodes, an integrated GPU sidelined by hybrid-graphics + switching, Volume Shadow Copy snapshot entries, and internal software/virtual device stubs + (MIDI service test loopbacks, Virtual HID Framework nodes). Removing any of those can + destabilize a running system. So this only ever removes an ALLOW-listed shape of instance ID + -- genuinely pluggable peripherals identified by vendor/product ID (USB\VID_*, HID\VID_*), + Bluetooth devices (BTH\*), and audio-endpoint stubs (SWD\MMDEVAPI\*) -- and explicitly still + skips USB root hubs even though they're USB\-rooted. Everything else found is left alone and + reported as skipped, never removed. +#> + +# $Mode may already be set by a prelude the caller prepends (EmbeddedScript.RunCommand's +# `prelude` param) — "List" previews the exact same classification below without removing +# anything, so the read-only action is always an accurate preview of what "Remove" will do. +if (-not $Mode) { $Mode = 'Remove' } + +function Test-SafeToRemove([string]$InstanceId) { + if ($InstanceId -like 'USB\ROOT_HUB*') { return $false } + if ($InstanceId -like 'USB\VID_*') { return $true } + if ($InstanceId -like 'HID\VID_*') { return $true } + if ($InstanceId -like 'BTH\*') { return $true } + if ($InstanceId -like 'SWD\MMDEVAPI\*') { return $true } + return $false +} + +$raw = & pnputil.exe /enum-devices /disconnected +$ids = $raw | Select-String '^Instance ID:\s*(.+)$' | + ForEach-Object { $_.Matches[0].Groups[1].Value.Trim() } + +if (-not $ids) { + Write-Output "No ghost devices found." + exit 0 +} + +foreach ($id in $ids) { + if (Test-SafeToRemove $id) { + if ($Mode -eq 'List') { + Write-Output "Ghost (would remove): $id" + } else { + Write-Output "Removing: $id" + & pnputil.exe /remove-device "$id" + } + } else { + Write-Output "Ghost (kept — not a recognised removable peripheral): $id" + } +} diff --git a/Scripts/scripts.json b/Scripts/scripts.json new file mode 100644 index 0000000..e5fdd1a --- /dev/null +++ b/Scripts/scripts.json @@ -0,0 +1,23 @@ +{ + "_comment": "Script-backed System Tools actions. 'script' MUST name a .ps1 embedded from Scripts/; never put a command line, an executable name, or raw PowerShell in this file. See Scripts/README.md.", + + "actions": [ + { + "id": "ghost-devices-list", + "name": "List ghost devices", + "description": "Preview non-present (phantom) devices, marking exactly which ones \"Remove ghost devices\" would remove vs. leave alone (e.g. disks and GPU components are always kept). Changes nothing.", + "script": "RemoveGhostDevices.ps1", + "mode": "List", + "isReadOnly": true + }, + { + "id": "ghost-devices-remove", + "name": "Remove ghost devices", + "description": "Permanently remove non-present (phantom) USB/HID/Bluetooth/audio devices. Destructive — cannot be undone.", + "warning": "This permanently deletes device entries from Windows; there is no undo. Only USB, HID, Bluetooth and audio-endpoint devices are ever removed — disks, GPU components, and other onboard hardware are never touched, even if they show as \"disconnected\" (real hardware can show that way for unrelated reasons). If a removed device is actually hardware that's just temporarily unplugged (a USB dock, a headset, a keyboard dongle), it may need a driver reinstall or reboot before it works again once reconnected.", + "script": "RemoveGhostDevices.ps1", + "mode": "Remove", + "isAdvanced": true + } + ] +} diff --git a/Services/EmbeddedScript.cs b/Services/EmbeddedScript.cs new file mode 100644 index 0000000..b42c998 --- /dev/null +++ b/Services/EmbeddedScript.cs @@ -0,0 +1,67 @@ +using System; +using System.IO; +using System.Linq; +using System.Reflection; +using System.Text; +using System.Text.RegularExpressions; +using WinButler.Models; + +namespace WinButler.Services; + +/// +/// Builds a that runs an embedded PowerShell script +/// (Scripts/*.ps1) via -EncodedCommand, entirely in memory — nothing is ever written +/// to disk. WinButler runs elevated (requireAdministrator); extracting a script to a +/// user-writable location (e.g. %APPDATA%) and then invoking it from that elevated process +/// would let any unprivileged process running as the user overwrite it first, so this avoids the +/// disk round-trip, and the privilege-escalation hole it opens, altogether. +/// +public static class EmbeddedScript +{ + private const string ResourceMarker = ".Scripts."; + + /// A bare identifier — no quotes, no whitespace, no newlines. See . + private static readonly Regex ModePattern = new(@"^[A-Za-z][A-Za-z0-9]*$", RegexOptions.Compiled); + + /// Builds the command for the named script (e.g. "RemoveGhostDevices.ps1", matched + /// against Scripts/*.ps1). is set so the + /// dry-run preview and the runner's "> ..." line show the script name instead of a base64 blob. + /// , if given, is assigned to $Mode ahead of the script body — + /// letting one script back several actions (e.g. a read-only preview and the real thing) + /// without a param()/-File invocation. + /// The mode is restricted to a bare identifier and validated here rather than being a + /// free-form PowerShell statement: it can originate from Scripts/scripts.json, and a + /// value carrying a quote or newline would inject arbitrary code into a process that always + /// runs elevated. Alphanumerics-only makes escaping the single-quoted assignment impossible. + public static SystemCommand RunCommand(string fileName, string? mode = null) + { + var body = mode is null ? ReadText(fileName) : $"$Mode = '{ValidMode(mode)}'\n" + ReadText(fileName); + var encoded = Convert.ToBase64String(Encoding.Unicode.GetBytes(body)); + return new SystemCommand("powershell.exe", $"-NoProfile -EncodedCommand {encoded}", + DisplayOverride: $"powershell.exe -File {fileName} (embedded)"); + } + + private static string ValidMode(string mode) => + ModePattern.IsMatch(mode) + ? mode + : throw new InvalidOperationException( + $"Script mode '{mode}' is not a bare identifier (letters and digits only). " + + "Modes are embedded in the script body, so anything else could inject PowerShell."); + + private static string ReadText(string fileName) + { + var asm = Assembly.GetExecutingAssembly(); + // Restricted to .ps1 on purpose: Scripts/ also holds scripts.json, and whatever this + // returns gets executed. Keeping the executable lookup structurally unable to reach a + // non-script means a slip in a caller's own validation can't turn into running data. + var name = asm.GetManifestResourceNames() + .FirstOrDefault(n => n.IndexOf(ResourceMarker, StringComparison.OrdinalIgnoreCase) >= 0 + && n.EndsWith(".ps1", StringComparison.OrdinalIgnoreCase) + && n.EndsWith("." + fileName, StringComparison.OrdinalIgnoreCase)) + ?? throw new InvalidOperationException($"Embedded script '{fileName}' not found in assembly."); + + using var stream = asm.GetManifestResourceStream(name)!; + using var reader = new StreamReader(stream); + return reader.ReadToEnd(); + } +} diff --git a/Services/KnownLocationsScanner.cs b/Services/KnownLocationsScanner.cs index e09b23a..0cde40a 100644 --- a/Services/KnownLocationsScanner.cs +++ b/Services/KnownLocationsScanner.cs @@ -117,6 +117,9 @@ private void AddChildren(KnownLocationEntry entry, string dir, RiskLevel risk, ct.ThrowIfCancellationRequested(); if (_safeCaches.IsDenied(child)) continue; + if (entry.Exclude is { Count: > 0 } exclude && + exclude.Contains(Path.GetFileName(child), StringComparer.OrdinalIgnoreCase)) + continue; var isDir = Directory.Exists(child); long size = SizeOf(child, isDir, ct); diff --git a/Services/ScriptCatalog.cs b/Services/ScriptCatalog.cs new file mode 100644 index 0000000..21fbed4 --- /dev/null +++ b/Services/ScriptCatalog.cs @@ -0,0 +1,119 @@ +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using System.Reflection; +using System.Text.Json; +using System.Text.RegularExpressions; +using WinButler.Models; +using WinButler.Services.Definitions; + +namespace WinButler.Services; + +/// +/// Builds the script-backed half of the System Tools catalog from the embedded +/// Scripts/scripts.json manifest, so adding an action is a drop-in: add a .ps1 under +/// Scripts/, add one entry, done — no code change. See Scripts/README.md. +/// Why this is not part of . That type is what +/// / +/// merge overlays into, including from +/// (an unauthenticated URL fetch). Anything +/// reachable from there is remotely overridable by construction, and this app runs elevated. The +/// action catalog is loaded from its own embedded resource, outside that merge path, so a future +/// remote-definitions rollout can never reach it. +/// The manifest still only ever names a script that shipped inside the assembly and +/// a bare-identifier mode — never a command line — so the executable surface stays fixed at compile +/// time either way. +/// +public sealed class ScriptCatalog +{ + /// The manifest resource, as it appears in the assembly manifest. + private const string ResourceSuffix = ".Scripts.scripts.json"; + + /// A plain script file name — no directory separators, no traversal. + private static readonly Regex ScriptNamePattern = new(@"^[A-Za-z0-9._-]+\.ps1$", RegexOptions.Compiled); + + public IReadOnlyList Actions { get; } + + private ScriptCatalog(IReadOnlyList actions) => Actions = actions; + + /// A catalog with no script actions — what yields when the + /// manifest is unusable. Exposed for tests that need that state without a broken manifest. + internal static ScriptCatalog Empty => new(Array.Empty()); + + /// Loads the bundled manifest, returning an EMPTY catalog (logged) if anything at all is + /// wrong with it. Callers keep their code-defined actions, so the page still works — an empty + /// action catalog is useless but, unlike an empty deny-list, not dangerous. + public static ScriptCatalog LoadBundled() + { + try + { + return new ScriptCatalog(Parse(ReadManifest())); + } + catch (Exception ex) + { + Log.Error("script-catalog", "scripts.json failed to load — no script actions registered.", ex); + return new ScriptCatalog(Array.Empty()); + } + } + + /// Parses and validates a manifest (test seam). Throws on the first problem: the load is + /// all-or-nothing, so a bad entry can't leave a destructive action registered while the read-only + /// preview that makes it safe to use silently goes missing. + internal static IReadOnlyList Parse(string json) + { + var manifest = JsonSerializer.Deserialize(json, DefinitionsJson.Options) + ?? throw new InvalidOperationException("scripts.json parsed to null."); + + var seen = new HashSet(StringComparer.OrdinalIgnoreCase); + var actions = new List(); + + foreach (var entry in manifest.Actions) + { + Require(!string.IsNullOrWhiteSpace(entry.Id), "an entry is missing 'id'."); + Require(seen.Add(entry.Id), $"'{entry.Id}' is declared more than once."); + Require(!string.IsNullOrWhiteSpace(entry.Name), $"'{entry.Id}' is missing 'name'."); + Require(!string.IsNullOrWhiteSpace(entry.Description), $"'{entry.Id}' is missing 'description'."); + Require(ScriptNamePattern.IsMatch(entry.Script), $"'{entry.Id}' has an invalid 'script' name."); + + // A destructive action's warning is what the confirm modal shows; make stating the risk + // mandatory rather than letting it silently fall back to the softer description. + Require(entry.IsReadOnly || !string.IsNullOrWhiteSpace(entry.Warning), + $"'{entry.Id}' is not read-only, so it must declare a 'warning'."); + + // Throws if the script isn't embedded, or if the mode isn't a bare identifier. + var step = EmbeddedScript.RunCommand(entry.Script, entry.Mode); + + actions.Add(new SystemAction + { + Id = entry.Id, + Name = entry.Name, + Description = entry.Description, + Warning = entry.Warning, + IsReadOnly = entry.IsReadOnly, + IsAdvanced = entry.IsAdvanced, + Steps = new[] { step }, + }); + } + + return actions; + } + + private static void Require(bool condition, string problem) + { + if (!condition) + throw new InvalidOperationException($"scripts.json: {problem}"); + } + + private static string ReadManifest() + { + var asm = Assembly.GetExecutingAssembly(); + var name = asm.GetManifestResourceNames() + .FirstOrDefault(n => n.EndsWith(ResourceSuffix, StringComparison.OrdinalIgnoreCase)) + ?? throw new InvalidOperationException("Embedded scripts.json not found in assembly."); + + using var stream = asm.GetManifestResourceStream(name)!; + using var reader = new StreamReader(stream); + return reader.ReadToEnd(); + } +} diff --git a/Tests/DefinitionsTests.cs b/Tests/DefinitionsTests.cs index b32f271..7485738 100644 --- a/Tests/DefinitionsTests.cs +++ b/Tests/DefinitionsTests.cs @@ -164,6 +164,26 @@ public void Merge_replaces_known_location_by_id() Assert.Equal("Overridden", merged.KnownLocations.Entries.Single(e => e.Id == first.Id).DisplayName); } + [Fact] + public void Known_location_exclude_field_binds_from_json() + { + var json = """ + { + "knownLocations": { + "entries": [ + { "id": "t", "path": "%LocalAppData%\\Foo", "mode": "children", "exclude": ["players", "Keep"], "risk": "safe", "displayName": "Foo" } + ] + } + } + """; + + var parsed = BundledDefinitionSource.Parse(json); + var entry = Assert.Single(parsed.KnownLocations.Entries); + + Assert.NotNull(entry.Exclude); + Assert.Equal(new[] { "players", "Keep" }, entry.Exclude); + } + [Fact] public void Merge_takes_highest_version() { diff --git a/Tests/EmbeddedScriptTests.cs b/Tests/EmbeddedScriptTests.cs new file mode 100644 index 0000000..6a34969 --- /dev/null +++ b/Tests/EmbeddedScriptTests.cs @@ -0,0 +1,70 @@ +using System; +using WinButler.Services; +using Xunit; + +namespace WinButler.Tests; + +/// +/// Covers — building an -EncodedCommand invocation entirely in +/// memory, with a readable Display for dry-run previews and the runner's "> ..." line. +/// +public sealed class EmbeddedScriptTests +{ + [Fact] + public void RunCommand_encodes_the_script_and_never_touches_disk() + { + var command = EmbeddedScript.RunCommand("RemoveGhostDevices.ps1"); + + Assert.Equal("powershell.exe", command.FileName); + Assert.Contains("-EncodedCommand", command.Arguments); + Assert.Contains("-NoProfile", command.Arguments); + } + + [Fact] + public void Display_shows_the_script_name_not_the_encoded_payload() + { + var command = EmbeddedScript.RunCommand("RemoveGhostDevices.ps1"); + + Assert.Contains("RemoveGhostDevices.ps1", command.Display); + Assert.DoesNotContain("EncodedCommand", command.Display); + } + + [Fact] + public void Unknown_script_name_throws_rather_than_silently_running_nothing() + { + Assert.Throws(() => EmbeddedScript.RunCommand("DoesNotExist.ps1")); + } + + /// Scripts/ also holds scripts.json. Whatever this resolves gets executed, so the + /// lookup must not reach a non-.ps1 resource even when asked for one by name. + [Fact] + public void A_non_script_resource_in_the_scripts_folder_is_not_executable() + { + Assert.Throws(() => EmbeddedScript.RunCommand("scripts.json")); + } + + [Fact] + public void Mode_changes_the_encoded_payload_but_not_the_display() + { + var plain = EmbeddedScript.RunCommand("RemoveGhostDevices.ps1"); + var withMode = EmbeddedScript.RunCommand("RemoveGhostDevices.ps1", "List"); + + Assert.NotEqual(plain.Arguments, withMode.Arguments); // different payload + Assert.Equal(plain.Display, withMode.Display); // same readable preview + } + + /// The mode is interpolated into the script body ($Mode = '...') and the resulting + /// script runs elevated, so anything that could escape the quotes must be refused outright + /// rather than encoded. + [Theory] + [InlineData("List'; Remove-Item C:\\ -Recurse #")] + [InlineData("List\nRemove-Item")] + [InlineData("List'")] + [InlineData("has space")] + [InlineData("")] + [InlineData("1StartsWithDigit")] + public void Invalid_mode_throws_rather_than_encoding(string mode) + { + Assert.Throws(() => EmbeddedScript.RunCommand("RemoveGhostDevices.ps1", mode)); + } +} diff --git a/Tests/Headless/SystemToolsPageTests.cs b/Tests/Headless/SystemToolsPageTests.cs index 5389ac0..8a65682 100644 --- a/Tests/Headless/SystemToolsPageTests.cs +++ b/Tests/Headless/SystemToolsPageTests.cs @@ -172,6 +172,56 @@ public void Advanced_actions_are_separated_from_the_regular_ones() Assert.DoesNotContain(vm.Actions, a => a.IsAdvanced); } + [AvaloniaFact] + public void A_failed_script_manifest_still_leaves_the_built_in_actions_usable() + { + // scripts.json is unusable → zero script actions, but the code-defined Windows-tool actions + // are unaffected, so the page still works rather than going blank. + var vm = new SystemToolsPageViewModel(new AppSettings { IsDryRun = true }, new FakeRunner(), + new PrivacyCleaner(new EmptyRegistry()), ScriptCatalog.Empty); + + Assert.Contains(vm.Actions, a => a.Id == "analyze-store"); + Assert.Contains(vm.AdvancedActions, a => a.Id == "wmi-reset"); + Assert.DoesNotContain(vm.Actions.Concat(vm.AdvancedActions), a => a.Id.StartsWith("ghost-devices")); + Assert.True(vm.HasAdvancedActions); // the built-ins keep the ADVANCED divider meaningful + } + + [AvaloniaFact] + public void Ghost_device_actions_are_catalogued_with_the_right_flags() + { + var vm = NewVm(true, new FakeRunner()); + + var list = vm.Actions.Single(a => a.Id == "ghost-devices-list"); + Assert.True(list.IsReadOnly); + Assert.False(list.IsAdvanced); + // List must run the SAME script as Remove (just in preview mode) so it can't drift into + // showing a different device set than what Remove would actually touch. + Assert.Contains("RemoveGhostDevices.ps1", list.Steps.Single().Display); + + var remove = vm.AdvancedActions.Single(a => a.Id == "ghost-devices-remove"); + Assert.True(remove.IsAdvanced); + Assert.False(remove.IsReadOnly); + // The confirm modal shows Warning (falling back to Description) — both must make the + // "permanent, no undo" nature of this action unmistakable, not just hint at it. + Assert.Contains("no undo", remove.Warning, StringComparison.OrdinalIgnoreCase); + Assert.Contains("permanent", remove.Description, StringComparison.OrdinalIgnoreCase); + } + + [AvaloniaFact] + public async Task Ghost_device_removal_dry_run_preview_is_readable_not_a_base64_blob() + { + var runner = new FakeRunner(); + var vm = NewVm(true, runner); + var remove = vm.AdvancedActions.Single(a => a.Id == "ghost-devices-remove"); + + await vm.RunActionCommand.ExecuteAsync(remove); + Dispatcher.UIThread.RunJobs(); + + Assert.Empty(runner.Ran); // dry-run — nothing executed + Assert.Contains(vm.Output, l => l.Contains("RemoveGhostDevices.ps1")); + Assert.DoesNotContain(vm.Output, l => l.Contains("EncodedCommand")); + } + /// /// Renders the actual view (not just the VM) to prove the per-item RUN button's /// $parent[ItemsControl]…RunActionCommand binding resolves — a wiring shape no other page @@ -201,4 +251,27 @@ public void Run_button_binding_resolves_to_the_command_and_invokes_it() Assert.Contains(vm.Output, l => l.Contains("DRY RUN")); } + + /// + /// The ADVANCED divider's IsVisible="{Binding HasAdvancedActions}" is the one binding + /// whose failure mode is silent *hiding* — a compiled binding that didn't resolve would leave + /// the "CAN BREAK THINGS" warning off the page while the advanced actions below it still render. + /// The VM-level assertions elsewhere can't see that, so prove it against the real view. + /// + [AvaloniaFact] + public void Advanced_divider_renders_when_advanced_actions_exist() + { + var vm = NewVm(true, new FakeRunner()); + var window = new Window { Content = new SystemToolsPageView { DataContext = vm }, Width = 900, Height = 640 }; + window.Show(); + Dispatcher.UIThread.RunJobs(); + + var banner = window.GetVisualDescendants().OfType() + .FirstOrDefault(t => t.Text is not null && t.Text.StartsWith("ADVANCED")); + + Assert.NotNull(banner); + Assert.True(vm.HasAdvancedActions); + Assert.True(banner!.IsVisible); // the binding resolved, not silently false + Assert.True(((Control)banner.Parent!).IsVisible); // ...on the Border that actually carries it + } } diff --git a/Tests/KnownLocationsScannerTests.cs b/Tests/KnownLocationsScannerTests.cs index 543aae0..663daeb 100644 --- a/Tests/KnownLocationsScannerTests.cs +++ b/Tests/KnownLocationsScannerTests.cs @@ -123,6 +123,56 @@ public void Deny_listed_children_are_never_offered() Assert.DoesNotContain(results, t => t.FullPath.Contains(".ssh", StringComparison.OrdinalIgnoreCase)); } + [Fact] + public void Excluded_children_are_never_offered_but_siblings_still_are() + { + var dir = Path.Combine(_root, "cod"); + WriteFile(Path.Combine(dir, "players", "settings.cfg")); + WriteFile(Path.Combine(dir, "junk", "crash.dmp")); + + var results = Scan(new KnownLocationEntry + { + Id = "t", Path = dir, Mode = "children", Risk = "safe", DisplayName = "CoD", + Exclude = new List { "players" }, + }); + + var target = Assert.Single(results); + Assert.EndsWith("junk", target.FullPath); + Assert.DoesNotContain(results, t => t.FullPath.Contains("players", StringComparison.OrdinalIgnoreCase)); + } + + [Fact] + public void Exclude_match_is_case_insensitive() + { + var dir = Path.Combine(_root, "cod-case"); + WriteFile(Path.Combine(dir, "Players", "settings.cfg")); + WriteFile(Path.Combine(dir, "junk", "crash.dmp")); + + var results = Scan(new KnownLocationEntry + { + Id = "t", Path = dir, Mode = "children", Risk = "safe", DisplayName = "CoD", + Exclude = new List { "players" }, + }); + + var target = Assert.Single(results); + Assert.EndsWith("junk", target.FullPath); + } + + [Fact] + public void Absent_exclude_keeps_every_child_as_before() + { + var dir = Path.Combine(_root, "no-exclude"); + WriteFile(Path.Combine(dir, "a", "dump1.bin")); + WriteFile(Path.Combine(dir, "b", "dump2.bin")); + + var results = Scan(new KnownLocationEntry + { + Id = "t", Path = dir, Mode = "children", Risk = "safe", DisplayName = "No exclude", + }); + + Assert.Equal(2, results.Count); // Exclude is null — no regression vs. pre-existing behavior + } + [Fact] public void Risky_entries_map_to_the_risky_level_and_the_recycle_bin() { diff --git a/Tests/ScriptCatalogTests.cs b/Tests/ScriptCatalogTests.cs new file mode 100644 index 0000000..7a94f3a --- /dev/null +++ b/Tests/ScriptCatalogTests.cs @@ -0,0 +1,93 @@ +using System; +using System.Linq; +using WinButler.Services; +using Xunit; + +namespace WinButler.Tests; + +/// +/// Covers — the data-driven half of the System Tools catalog. The +/// validation cases matter beyond tidiness: this manifest selects what an elevated process runs, so +/// every rejection path is a security boundary, and the load is all-or-nothing so a bad entry can +/// never leave a destructive action registered without its read-only preview. +/// +public sealed class ScriptCatalogTests +{ + private const string ValidEntry = """ + { "id": "a", "name": "A", "description": "D", "script": "RemoveGhostDevices.ps1", "isReadOnly": true } + """; + + private static string Manifest(params string[] entries) => + $$"""{ "actions": [ {{string.Join(",", entries)}} ] }"""; + + [Fact] + public void Bundled_manifest_registers_the_ghost_device_actions() + { + var actions = ScriptCatalog.LoadBundled().Actions; + + var list = actions.Single(a => a.Id == "ghost-devices-list"); + Assert.True(list.IsReadOnly); + Assert.False(list.IsAdvanced); + + var remove = actions.Single(a => a.Id == "ghost-devices-remove"); + Assert.True(remove.IsAdvanced); + Assert.False(remove.IsReadOnly); + Assert.Contains("no undo", remove.Warning, StringComparison.OrdinalIgnoreCase); + + // Both drive the SAME script, so the read-only preview can't drift from what Remove does. + Assert.All(actions, a => Assert.Contains("RemoveGhostDevices.ps1", a.Steps.Single().Display)); + } + + [Fact] + public void Valid_manifest_parses() + { + var actions = ScriptCatalog.Parse(Manifest(ValidEntry)); + + var action = Assert.Single(actions); + Assert.Equal("a", action.Id); + Assert.Single(action.Steps); + } + + /// The load is all-or-nothing: one bad entry must take the whole manifest down rather + /// than register the good ones. Each case pairs the offender with a VALID entry to prove the + /// valid one is dropped too. + [Theory] + // A mode is interpolated into the script body, so anything but a bare identifier could inject + // PowerShell into a process that always runs elevated. This is the case that matters most. + [InlineData("""{ "id": "b", "name": "B", "description": "D", "script": "RemoveGhostDevices.ps1", "mode": "List'; Remove-Item C:\\ -Recurse #", "isReadOnly": true }""")] + [InlineData("""{ "id": "b", "name": "B", "description": "D", "script": "RemoveGhostDevices.ps1", "mode": "List\nRemove-Item", "isReadOnly": true }""")] + // Script must resolve to something embedded in the assembly. + [InlineData("""{ "id": "b", "name": "B", "description": "D", "script": "NotEmbedded.ps1", "isReadOnly": true }""")] + // ...and must be a plain file name — no path traversal, no arbitrary extension. + [InlineData("""{ "id": "b", "name": "B", "description": "D", "script": "..\\..\\evil.ps1", "isReadOnly": true }""")] + [InlineData("""{ "id": "b", "name": "B", "description": "D", "script": "calc.exe", "isReadOnly": true }""")] + // A destructive action must state its own risk — the confirm modal shows Warning. + [InlineData("""{ "id": "b", "name": "B", "description": "D", "script": "RemoveGhostDevices.ps1" }""")] + // Required metadata. + [InlineData("""{ "id": "", "name": "B", "description": "D", "script": "RemoveGhostDevices.ps1", "isReadOnly": true }""")] + [InlineData("""{ "id": "b", "name": "", "description": "D", "script": "RemoveGhostDevices.ps1", "isReadOnly": true }""")] + [InlineData("""{ "id": "b", "name": "B", "description": "", "script": "RemoveGhostDevices.ps1", "isReadOnly": true }""")] + public void Invalid_entry_rejects_the_whole_manifest(string badEntry) + { + Assert.ThrowsAny(() => ScriptCatalog.Parse(Manifest(ValidEntry, badEntry))); + } + + [Fact] + public void Duplicate_ids_are_rejected() + { + Assert.ThrowsAny(() => ScriptCatalog.Parse(Manifest(ValidEntry, ValidEntry))); + } + + [Fact] + public void Malformed_json_is_rejected() + { + Assert.ThrowsAny(() => ScriptCatalog.Parse("{ not valid json ")); + } + + [Fact] + public void A_read_only_entry_needs_no_warning() + { + // isReadOnly actions never reach the confirm modal, so the warning requirement doesn't apply. + Assert.Single(ScriptCatalog.Parse(Manifest(ValidEntry))); + } +} diff --git a/ViewModels/SystemToolsPageViewModel.cs b/ViewModels/SystemToolsPageViewModel.cs index b2d7a05..5586628 100644 --- a/ViewModels/SystemToolsPageViewModel.cs +++ b/ViewModels/SystemToolsPageViewModel.cs @@ -2,6 +2,7 @@ using System.Collections.Generic; using System.Collections.ObjectModel; using System.IO; +using System.Linq; using System.Threading; using System.Threading.Tasks; using CommunityToolkit.Mvvm.ComponentModel; @@ -43,13 +44,25 @@ public partial class SystemToolsPageViewModel : ViewModelBase public bool IsDryRun => _settings.IsDryRun; - public SystemToolsPageViewModel(AppSettings settings, SystemActionRunner runner, PrivacyCleaner privacy) + /// Whether any Advanced action registered — the view's "ADVANCED" divider hides when + /// none did (the script-backed ones come from a manifest that can legitimately be empty). + /// Intentionally has no change notification: the catalog is built once in the constructor and + /// never mutates, so the binding's single read at attach time is always correct. + public bool HasAdvancedActions => AdvancedActions.Count > 0; + + /// Script-backed actions from Scripts/scripts.json; defaults to the + /// bundled manifest (tests inject their own). Mirrors 's + /// bundled-by-default convenience. + public SystemToolsPageViewModel(AppSettings settings, SystemActionRunner runner, PrivacyCleaner privacy, + ScriptCatalog? scripts = null) { _settings = settings; _runner = runner; _privacy = privacy; - var catalog = BuildCatalog(); + // Built-in Windows-tool actions are defined in code (executable commands must never be + // data-driven — see SystemCommand); script-backed ones are appended from the manifest. + var catalog = BuildCatalog().Concat((scripts ?? ScriptCatalog.LoadBundled()).Actions); Actions = new ObservableCollection(); AdvancedActions = new ObservableCollection(); foreach (var a in catalog) @@ -236,6 +249,9 @@ private static void ClearDirectoryContents(string dir, IProgress output, } } + /// The built-in Windows-tool actions. These stay in code on purpose: they are literal + /// executable + argument pairs, and must never be data-driven. + /// Script-backed actions come from Scripts/scripts.json via . private static IReadOnlyList BuildCatalog() => new[] { new SystemAction diff --git a/Views/SystemToolsPageView.axaml b/Views/SystemToolsPageView.axaml index e06be5e..5f737c9 100644 --- a/Views/SystemToolsPageView.axaml +++ b/Views/SystemToolsPageView.axaml @@ -67,7 +67,7 @@ Foreground="{DynamicResource WbAccentBrush}" Margin="0,0,0,6"/> - @@ -84,6 +84,7 @@ + + +