From e79c78e545d88b695a5cccff85463f3b100743d4 Mon Sep 17 00:00:00 2001 From: Brad Barnett <127794626+bdbarnett@users.noreply.github.com> Date: Mon, 21 Sep 2026 07:46:35 -0500 Subject: [PATCH 1/6] restart-esp-usb: the task called a cmdlet that does not exist, so no S3 flash was ever unattended `mpftp-restart-esp-usb` ran, inline, as SYSTEM: Get-PnpDevice -PresentOnly | Where-Object { $_.InstanceId -like 'USB\VID_303A*' } | Restart-PnpDevice -Confirm:$false Windows PowerShell's PnpDevice module ships Get-, Enable- and Disable-PnpDevice and nothing else. So the pipeline raised CommandNotFoundException on every device, the task reported LastTaskResult 1, and from outside that is indistinguishable from a board that refused to come back. It never worked once. It cost the 2026-09-17 pin-move run its S3 half and the 2026-09-21 live-audio spike its auto-suspend measurement, and last night it cost the owner a trip out of bed. Confirmed on the bench: Get-Command -Module PnpDevice returns exactly those four names, and the registered action is still the text above. The task now runs tools/windows/restart-esp-usb.ps1, which: - takes ONE device, by instance id, from a request file. The old VID_303A sweep would have bounced every Espressif board attached together -- on this bench that is a second board mid-demo; - matches that id whole against an allow-list for a VID_303A composite parent, never evaluates it, hands it to pnputil as an argument array, and deletes the request on read so a stale one is never replayed; - uses pnputil /restart-device, with Disable+Enable as the fallback; - writes a transcript, so LastTaskResult 1 can be told from a board that really did not come back; - exits 0 restarted, 2 no request, 3 request refused, 4 no such device, 5 the restart failed -- a wrong id fails loudly rather than quietly. The task runs as SYSTEM, so it must never execute anything an ordinary account can write. install-restart-esp-usb-task.ps1 is the one elevated step: it puts the script in C:\Program Files\mpftp with an explicit ACL, creates the user-writable request directory beside it in ProgramData, registers the task, and self-checks the registration. A later edit to the script costs another elevated install; that is the correct price for the SYSTEM principal. `mpftp usb-restart --status` reads the action the task is really registered with, so an agent can tell "the recovery is not installed" from "the board did not make it" before planning around it. --list finds instance ids instead of hard-coding them. Proved as far as an unprivileged session goes: 29 tests, including the script's own refusals driven through -DryRun at medium integrity with the boards untouched -- wrong vendor, an &MI_ child, three injection shapes, a wildcard, a path, two devices in one request, an over-long request, empty, missing, and a well-formed id for an absent device exiting 4 rather than 0. Both rules were watched failing against deliberately loosened copies: the PowerShell regex loosened to ^USB\VID_ turns five red (including accepting the other board's UART), the one-line check loosened turns the two-device case red, and the same loosening in Python turns six red. NOT yet proved, and waiting on that one elevated install: the task reaching LastTaskResult 0, COM12 disappearing and coming back, and a wrong instance id failing through the task rather than through a direct run. --- CHANGELOG.md | 21 ++ cli/src/mpftp/cli.py | 34 ++ cli/src/mpftp/espusb.py | 302 ++++++++++++++++++ cli/tests/test_esp_usb_restart.py | 238 ++++++++++++++ docs/agent-guide.md | 82 +++-- .../windows/install-restart-esp-usb-task.ps1 | 148 +++++++++ tools/windows/restart-esp-usb.ps1 | 251 ++++++++++++--- 7 files changed, 1006 insertions(+), 70 deletions(-) create mode 100644 cli/src/mpftp/espusb.py create mode 100644 cli/tests/test_esp_usb_restart.py create mode 100644 tools/windows/install-restart-esp-usb-task.ps1 diff --git a/CHANGELOG.md b/CHANGELOG.md index 7773086..37cca3c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,26 @@ ## Unreleased +- Fix the no-UAC ESP32 USB recovery, which had never worked (mpftp#31). The + scheduled task `mpftp-restart-esp-usb` ran `Get-PnpDevice | Restart-PnpDevice` + inline, and Windows PowerShell has no `Restart-PnpDevice` — so it failed on + every device and reported `LastTaskResult 1`, which from outside looks exactly + like a board that refused to come back. It cost the 2026-09-17 pin-move run + its S3 half and the 2026-09-21 live-audio spike its auto-suspend measurement. + `tools/windows/restart-esp-usb.ps1` is now what the task runs: one device per + run named by instance id (the old `USB\VID_303A*` sweep would have bounced + every Espressif board on the bench together), `pnputil /restart-device` as the + verb with Disable+Enable as the fallback, a transcript, and exit codes that + separate "no such device" from "the restart failed". +- Add `mpftp usb-restart` — `--status` to ask whether the recovery is really + installed before planning around it, `--list` to find instance ids rather than + hard-coding them, `--instance` to drive it. `--status` inspects the action the + task is registered with, so a dead recovery reads as dead. +- Add `tools/windows/install-restart-esp-usb-task.ps1`: the one elevated step. + The task runs as SYSTEM, so the script it executes is installed where only + administrators can write it; the one thing an unprivileged caller supplies is + an instance id in `C:\ProgramData\mpftp\restart-esp-usb.target`, matched whole + against an allow-list, never executed, and deleted on read. + - Add `monitor`: read-only console capture on a COM, held open for `--seconds` (or until Ctrl-C), streaming bytes to stdout and appending to `--log-path`. This is the capture `debug-tee` could not do from the CLI: the one-shot diff --git a/cli/src/mpftp/cli.py b/cli/src/mpftp/cli.py index 3cd9db9..7e0409c 100755 --- a/cli/src/mpftp/cli.py +++ b/cli/src/mpftp/cli.py @@ -1317,6 +1317,27 @@ def cmd_bootloader(ns: argparse.Namespace) -> None: client.close() +def cmd_usb_restart(ns: argparse.Namespace) -> None: + """Re-enumerate an ESP32's USB node after `bootloader` wedges it (mpftp#31). + + No board connection and no elevation: this drives the SYSTEM scheduled task + that does the privileged part. --status first, always -- the task on a given + machine may not be the one this repo ships. + """ + from . import espusb + + if ns.list: + out({"devices": espusb.espressif_devices()}) + return + if ns.status or not ns.instance: + state = espusb.task_state() + out(state) + if not state["usable"]: + raise SystemExit(1) + return + out(espusb.restart_device(ns.instance)) + + def cmd_rtc(ns: argparse.Namespace) -> None: client, mode = get_client() try: @@ -1900,6 +1921,19 @@ def build_parser() -> argparse.ArgumentParser: func=cmd_bootloader ) + ur = sub.add_parser( + "usb-restart", + help="Windows: re-enumerate an ESP32 USB node wedged by `bootloader` (no elevation)", + ) + ur.add_argument("--instance", help="Device instance id, e.g. 'USB\\VID_303A&PID_4003\\'") + ur.add_argument("--list", action="store_true", help="List attached VID_303A devices and their instance ids") + ur.add_argument( + "--status", + action="store_true", + help="Report whether the no-UAC recovery task is installed and would work; exit 1 if not", + ) + ur.set_defaults(func=cmd_usb_restart) + dtee = sub.add_parser( "debug-tee", help="Read-only monitor on a second COM (e.g. ESP native USB CDC)", diff --git a/cli/src/mpftp/espusb.py b/cli/src/mpftp/espusb.py new file mode 100644 index 0000000..fd9e28c --- /dev/null +++ b/cli/src/mpftp/espusb.py @@ -0,0 +1,302 @@ +"""No-UAC recovery for an ESP32 wedged on its own USB (mpftp#31). + +An ESP32-S3 whose only serial line is its native USB -- the LilyGO T-Embed -- +cannot be reset by the host: DTR and RTS go nowhere, so `mpftp bootloader` asks +the *firmware* to delete its USB PHY and reboot. Windows is often left serving +the node the chip no longer presents, and every open fails until that node is +re-enumerated. Re-enumerating it needs elevation, which an unattended session +has nobody to grant. + +The way through is a scheduled task, ``mpftp-restart-esp-usb``, registered once +with elevation and started on demand by any account. This module is the +unprivileged half: it finds the instance id, leaves it in the request file the +task's script reads, starts the task, and reports what came back. + +**Check before you plan around it.** The task registered on a machine may not +be the task this repo ships. The first one on this bench ran +``Get-PnpDevice | Restart-PnpDevice``, and there is no ``Restart-PnpDevice`` in +Windows PowerShell -- so it failed on every device, reported LastTaskResult 1, +and was indistinguishable from a board that refused to come back. That cost a +night's unattended flashing. :func:`task_state` exists so a caller can tell +"the recovery is not installed" from "the board did not make it", and +:func:`recovery_available` is the one-line question to ask first. +""" + +from __future__ import annotations + +import re +import shutil +import subprocess +import sys +import time +from pathlib import Path +from typing import Any + +TASK_NAME = "mpftp-restart-esp-usb" + +#: The one shape a request may take: the composite parent of an Espressif +#: (VID_303A) USB device. Anchored at both ends, so a trailing ``; calc`` or a +#: second path component cannot ride along, and the ``\\`` after the product id +#: rejects ``&MI_00`` children -- restarting the parent brings its interfaces +#: with it. The task's PowerShell side enforces this same rule independently; +#: this copy is here so a bad id is refused before it is ever written down. +INSTANCE_ID_RE = re.compile(r"^USB\\VID_303A&PID_[0-9A-Fa-f]{4}\\[0-9A-Za-z&_.\-]+$") + +MAX_REQUEST_CHARS = 200 +_TASK_WAIT_SECS = 45.0 + + +class RecoveryError(RuntimeError): + """The recovery could not be driven -- not the same as the board failing.""" + + +# --------------------------------------------------------------- powershell + +def _powershell() -> str: + exe = shutil.which("powershell.exe") or shutil.which("powershell") + if not exe: + raise RecoveryError( + "powershell.exe is not on PATH. This recovery is Windows-only; from WSL it " + "reaches Windows through interop, which needs /mnt/c/.../powershell.exe visible." + ) + return exe + + +def _ps(script: str, timeout: float = 60.0) -> str: + """Run a fixed PowerShell snippet. Never interpolate caller data in here.""" + proc = subprocess.run( + [_powershell(), "-NoProfile", "-NonInteractive", "-Command", script], + capture_output=True, + text=True, + timeout=timeout, + ) + if proc.returncode != 0 and not proc.stdout.strip(): + raise RecoveryError(f"powershell failed: {proc.stderr.strip() or proc.returncode}") + return proc.stdout + + +def _win_to_local(win_path: str) -> Path: + """Translate a Windows path to whatever this interpreter can open.""" + win_path = win_path.strip() + if sys.platform == "win32": + return Path(win_path) + wslpath = shutil.which("wslpath") + if not wslpath: + raise RecoveryError(f"cannot translate {win_path!r} without wslpath") + proc = subprocess.run([wslpath, "-u", win_path], capture_output=True, text=True) + if proc.returncode != 0: + raise RecoveryError(f"wslpath could not translate {win_path!r}: {proc.stderr.strip()}") + return Path(proc.stdout.strip()) + + +def _known_folder(var: str) -> str: + out = _ps(f"[Console]::Out.Write($env:{var})").strip() + if not out: + raise RecoveryError(f"Windows did not report %{var}%") + return out + + +def request_path() -> Path: + """Where an unprivileged caller leaves the instance id. Data, never code.""" + return _win_to_local(_known_folder("ProgramData")) / "mpftp" / "restart-esp-usb.target" + + +def log_path() -> Path: + """The task's transcript. Written by SYSTEM, readable here, not writable here.""" + return _win_to_local(_known_folder("ProgramFiles")) / "mpftp" / "restart-esp-usb.log" + + +# ------------------------------------------------------------------ devices + +def espressif_devices() -> list[dict[str, str]]: + """Every attached VID_303A composite parent, so ids are found and not guessed. + + ``&MI_`` children are left out: they are what you must *not* restart. + """ + out = _ps( + "Get-PnpDevice -PresentOnly " + "| Where-Object { $_.InstanceId -like 'USB\\VID_303A*' -and $_.InstanceId -notmatch '&MI_' } " + "| ForEach-Object { $_.InstanceId + '|' + $_.Status + '|' + $_.FriendlyName }" + ) + devices = [] + for line in out.splitlines(): + parts = line.strip().split("|", 2) + if len(parts) == 3 and parts[0]: + devices.append({"instanceId": parts[0], "status": parts[1], "name": parts[2]}) + return devices + + +def validate_instance_id(instance_id: str) -> str: + """Return *instance_id* if it is one we will ever write down, else raise.""" + if not isinstance(instance_id, str) or not instance_id.strip(): + raise ValueError("no instance id given") + candidate = instance_id.strip() + if len(candidate) > MAX_REQUEST_CHARS: + raise ValueError(f"instance id is {len(candidate)} chars, limit is {MAX_REQUEST_CHARS}") + if not INSTANCE_ID_RE.match(candidate): + raise ValueError( + f"{instance_id!r} is not an Espressif USB instance id. Expected the composite " + f"parent of a VID_303A device, e.g. 'USB\\VID_303A&PID_4003\\3485186BFCAC0000' -- " + f"not an &MI_ child, not another vendor, nothing appended. " + f"`mpftp usb-restart --list` prints the attached ones." + ) + return candidate + + +# --------------------------------------------------------------- the task + +def task_state() -> dict[str, Any]: + """What the registered task would actually do, and how it last went.""" + out = _ps( + f"$t = Get-ScheduledTask -TaskName '{TASK_NAME}' -ErrorAction SilentlyContinue; " + "if (-not $t) { 'installed|no' } else { " + " 'installed|yes'; " + " 'execute|' + $t.Actions[0].Execute; " + " 'arguments|' + ($t.Actions[0].Arguments -replace '[\\r\\n]+', ' '); " + " 'user|' + $t.Principal.UserId; " + " 'runlevel|' + $t.Principal.RunLevel; " + f" $i = Get-ScheduledTaskInfo -TaskName '{TASK_NAME}'; " + " 'lastResult|' + $i.LastTaskResult; " + " 'lastRun|' + $i.LastRunTime; " + " 'state|' + $t.State }" + ) + fields: dict[str, str] = {} + for line in out.splitlines(): + key, _, value = line.strip().partition("|") + if key: + fields[key] = value + + state: dict[str, Any] = { + "taskName": TASK_NAME, + "installed": fields.get("installed") == "yes", + "action": None, + "runsAs": fields.get("user"), + "runLevel": fields.get("runlevel"), + "lastResult": None, + "lastRun": fields.get("lastRun") or None, + "usable": False, + "reason": "", + } + if fields.get("lastResult", "").strip().lstrip("-").isdigit(): + state["lastResult"] = int(fields["lastResult"]) + + if not state["installed"]: + state["reason"] = ( + f"no scheduled task named {TASK_NAME}. Nothing can recover a wedged ESP32 USB " + f"node without elevation until an administrator runs " + f"tools/windows/install-restart-esp-usb-task.ps1." + ) + return state + + arguments = fields.get("arguments", "") + state["action"] = f"{fields.get('execute', '')} {arguments}".strip() + + if "Restart-PnpDevice" in arguments: + state["reason"] = ( + "the registered task calls Restart-PnpDevice, which does not exist in Windows " + "PowerShell's PnpDevice module -- it fails on every device and reports " + "LastTaskResult 1, which looks exactly like a board that refused to come back " + "(mpftp#31). Re-register it with tools/windows/install-restart-esp-usb-task.ps1 " + "from an administrator PowerShell." + ) + elif "restart-esp-usb.ps1" not in arguments: + state["reason"] = ( + "the registered task does not run restart-esp-usb.ps1, so mpftp cannot say what " + f"it would do. Its action is: {state['action']}" + ) + elif (state["runsAs"] or "").upper() not in ("SYSTEM", "NT AUTHORITY\\SYSTEM"): + state["reason"] = ( + f"the task runs as {state['runsAs']}, not SYSTEM, so pnputil /restart-device " + f"inside it will fail with Access is denied." + ) + else: + state["usable"] = True + state["reason"] = "ready" + return state + + +def recovery_available() -> tuple[bool, str]: + """Ask before planning around it: is the no-prompt recovery really there?""" + try: + state = task_state() + except RecoveryError as exc: + return False, str(exc) + return bool(state["usable"]), state["reason"] + + +def _task_is_running() -> bool: + out = _ps(f"(Get-ScheduledTask -TaskName '{TASK_NAME}').State") + return out.strip().lower() == "running" + + +def restart_device(instance_id: str, wait: float = _TASK_WAIT_SECS) -> dict[str, Any]: + """Restart one Espressif USB node through the task. No elevation here. + + Returns the task's exit code and the tail of its transcript. The exit codes + are the script's: 0 restarted, 2 no request, 3 request refused, 4 no such + device attached, 5 the restart itself failed. + """ + target = validate_instance_id(instance_id) + + available, reason = recovery_available() + if not available: + raise RecoveryError(reason) + + request = request_path() + try: + request.parent.mkdir(parents=True, exist_ok=True) + # The id goes into a file and nowhere else. It is never spliced into a + # command line, here or on the PowerShell side. + request.write_text(target + "\n", encoding="utf-8") + except OSError as exc: + raise RecoveryError(f"could not write the request file {request}: {exc}") from exc + + before = task_state().get("lastRun") + _ps(f"Start-ScheduledTask -TaskName '{TASK_NAME}'") + + deadline = time.monotonic() + wait + while time.monotonic() < deadline: + if not _task_is_running(): + break + time.sleep(0.5) + else: + raise RecoveryError( + f"{TASK_NAME} was still running after {wait:.0f}s. Read its transcript at {log_path()}." + ) + + after = task_state() + result = { + "instanceId": target, + "taskResult": after.get("lastResult"), + "lastRun": after.get("lastRun"), + "ranThisTime": after.get("lastRun") != before, + "log": tail_log(), + } + result["ok"] = result["taskResult"] == 0 + if not result["ok"]: + result["hint"] = _EXIT_HINTS.get( + result["taskResult"], + "see the transcript; a non-zero result is the script's own exit code.", + ) + return result + + +_EXIT_HINTS = { + 1: "PowerShell itself failed before the script ran -- check the task's action.", + 2: "the task found no request file. Something consumed it first, or the write did not land.", + 3: "the request was refused as malformed. mpftp validates the same rule before writing, " + "so this means the file was changed between the two.", + 4: "no device with that instance id is attached. It may already have re-enumerated -- " + "look for 303A:1001 on a new COM number.", + 5: "the restart itself failed. The transcript has pnputil's own words.", +} + + +def tail_log(lines: int = 20) -> list[str]: + try: + path = log_path() + if not path.exists(): + return [] + return path.read_text(encoding="utf-8", errors="replace").splitlines()[-lines:] + except (OSError, RecoveryError): + return [] diff --git a/cli/tests/test_esp_usb_restart.py b/cli/tests/test_esp_usb_restart.py new file mode 100644 index 0000000..869d140 --- /dev/null +++ b/cli/tests/test_esp_usb_restart.py @@ -0,0 +1,238 @@ +"""The no-UAC ESP32 USB recovery: what it accepts, and what it refuses (mpftp#31). + +The privileged half of this recovery runs as SYSTEM, so the only thing an +ordinary account hands it is the request file -- one instance id, data, never +code. These tests are about that boundary. + +Two layers. The pure-Python ones check the copy of the rule that stops a bad id +being written down at all. The PowerShell ones drive the real script through +its ``-DryRun`` switch, which does everything up to the restart -- validation, +and the presence check against attached hardware -- and stops before touching +the device, so they are safe to run as an ordinary user with boards in use on +the bench. They skip where there is no powershell.exe. + +To show these can fail, point MPFTP_ESP_USB_SCRIPT at a copy with the rule +loosened; the refusal cases go red. Loosening the regex to ``^USB\\VID_`` on +2026-09-21 turned five of them red, including accepting another vendor's UART. +""" + +from __future__ import annotations + +import os +import shutil +import subprocess +import unittest +from pathlib import Path +from unittest import mock + +from mpftp import espusb + +REPO_SCRIPT = Path(__file__).resolve().parents[2] / "tools" / "windows" / "restart-esp-usb.ps1" + +TEMBED = r"USB\VID_303A&PID_4003\3485186BFCAC0000" +ABSENT = r"USB\VID_303A&PID_4003\DEADBEEFDEADBEEF" +P4_UART = r"USB\VID_1A86&PID_55D3\5ABA052144" +MI_CHILD = r"USB\VID_303A&PID_4003&MI_00\6&BFF214A&0&0000" + + +class ValidateInstanceIdTests(unittest.TestCase): + """mpftp refuses a bad id before it reaches the file the SYSTEM task reads.""" + + def test_a_composite_parent_is_accepted(self): + self.assertEqual(espusb.validate_instance_id(TEMBED), TEMBED) + + def test_surrounding_whitespace_is_trimmed_not_refused(self): + self.assertEqual(espusb.validate_instance_id(f" {TEMBED}\n"), TEMBED) + + def test_another_vendor_is_refused(self): + # The bench that found mpftp#31 had a second board whose UART must not + # be bounced; a VID check is what keeps it out. + with self.assertRaises(ValueError): + espusb.validate_instance_id(P4_UART) + + def test_an_mi_child_is_refused(self): + with self.assertRaises(ValueError): + espusb.validate_instance_id(MI_CHILD) + + def test_a_trailing_command_is_refused(self): + with self.assertRaises(ValueError): + espusb.validate_instance_id(TEMBED + "; calc") + + def test_a_quote_break_is_refused(self): + with self.assertRaises(ValueError): + espusb.validate_instance_id(TEMBED + '" ; calc ; "') + + def test_a_wildcard_is_refused(self): + # Get-PnpDevice -InstanceId takes wildcards; one here would widen the + # request from a device to a set of them. + with self.assertRaises(ValueError): + espusb.validate_instance_id(r"USB\VID_303A&PID_4003\*") + + def test_a_path_is_refused(self): + with self.assertRaises(ValueError): + espusb.validate_instance_id(r"C:\Windows\System32\calc.exe") + + def test_a_second_line_is_refused(self): + with self.assertRaises(ValueError): + espusb.validate_instance_id(TEMBED + "\n" + TEMBED) + + def test_empty_is_refused(self): + for bad in ("", " ", "\n"): + with self.assertRaises(ValueError): + espusb.validate_instance_id(bad) + + def test_an_over_long_id_is_refused(self): + with self.assertRaises(ValueError): + espusb.validate_instance_id(r"USB\VID_303A&PID_4003\\" + "A" * 400) + + +class TaskStateTests(unittest.TestCase): + """Tell 'the recovery is not installed' from 'the board did not come back'.""" + + @staticmethod + def _state_for(ps_output: str) -> dict: + with mock.patch.object(espusb, "_ps", return_value=ps_output): + return espusb.task_state() + + def test_a_missing_task_is_not_usable(self): + state = self._state_for("installed|no\n") + self.assertFalse(state["installed"]) + self.assertFalse(state["usable"]) + self.assertIn("install-restart-esp-usb-task.ps1", state["reason"]) + + def test_the_broken_cmdlet_is_named_not_just_reported_as_failing(self): + # The exact action registered on the bench until 2026-09-21. + state = self._state_for( + "installed|yes\nexecute|powershell.exe\n" + "arguments|-NoProfile -Command \"Get-PnpDevice -PresentOnly | Where-Object " + "{ $_.InstanceId -like 'USB\\VID_303A*' } | Restart-PnpDevice -Confirm:$false\"\n" + "user|SYSTEM\nrunlevel|Highest\nlastResult|1\nlastRun|9/17/2026 5:01:14 PM\nstate|Ready\n" + ) + self.assertTrue(state["installed"]) + self.assertFalse(state["usable"]) + self.assertIn("Restart-PnpDevice", state["reason"]) + self.assertEqual(state["lastResult"], 1) + + def test_a_task_running_the_script_as_system_is_usable(self): + state = self._state_for( + "installed|yes\nexecute|powershell.exe\n" + 'arguments|-NoProfile -NonInteractive -ExecutionPolicy Bypass -WindowStyle Hidden ' + '-File "C:\\Program Files\\mpftp\\restart-esp-usb.ps1"\n' + "user|SYSTEM\nrunlevel|Highest\nlastResult|0\nlastRun|9/21/2026 7:39:00 AM\nstate|Ready\n" + ) + self.assertTrue(state["usable"]) + self.assertEqual(state["reason"], "ready") + self.assertEqual(state["lastResult"], 0) + + def test_the_script_run_as_an_ordinary_user_is_not_usable(self): + # pnputil /restart-device inside it would fail with Access is denied. + state = self._state_for( + "installed|yes\nexecute|powershell.exe\n" + 'arguments|-File "C:\\Program Files\\mpftp\\restart-esp-usb.ps1"\n' + "user|bradb\nrunlevel|Limited\nlastResult|0\nlastRun|9/21/2026 7:39:00 AM\nstate|Ready\n" + ) + self.assertFalse(state["usable"]) + self.assertIn("Access is denied", state["reason"]) + + def test_restart_refuses_to_run_when_the_task_is_not_usable(self): + with ( + mock.patch.object(espusb, "recovery_available", return_value=(False, "no such task")), + self.assertRaises(espusb.RecoveryError), + ): + espusb.restart_device(TEMBED) + + +def _powershell() -> str | None: + return shutil.which("powershell.exe") or shutil.which("powershell") + + +@unittest.skipIf(_powershell() is None, "needs Windows PowerShell (interop from WSL)") +class DryRunRefusalTests(unittest.TestCase): + """The script's own rule, exercised at medium integrity, device untouched.""" + + @classmethod + def setUpClass(cls): + override = os.environ.get("MPFTP_ESP_USB_SCRIPT") + if override: + cls.script = override + cls.work = os.path.dirname(override) or "." + return + # A .ps1 under \\wsl.localhost\ is awkward for powershell.exe to run; + # stage it on the Windows side instead. + temp = subprocess.run( + [_powershell(), "-NoProfile", "-NonInteractive", "-Command", "[Console]::Out.Write($env:TEMP)"], + capture_output=True, text=True, + ).stdout.strip() + if not temp: + raise unittest.SkipTest("Windows did not report %TEMP%") + cls.work = temp + r"\mpftp-esp-usb-tests" + local = espusb._win_to_local(cls.work) + local.mkdir(parents=True, exist_ok=True) + shutil.copy(REPO_SCRIPT, local / "restart-esp-usb.ps1") + cls.script = cls.work + r"\restart-esp-usb.ps1" + + def _run(self, request_text: str | None) -> int: + local_dir = espusb._win_to_local(self.work) + request = local_dir / "test.target" + if request_text is None: + request.unlink(missing_ok=True) + else: + request.write_text(request_text, encoding="utf-8") + proc = subprocess.run( + [_powershell(), "-NoProfile", "-NonInteractive", "-ExecutionPolicy", "Bypass", + "-File", self.script, + "-RequestPath", self.work + r"\test.target", + "-LogPath", self.work + r"\test.log", + "-DryRun"], + capture_output=True, text=True, + ) + return proc.returncode + + def test_a_missing_request_exits_2(self): + self.assertEqual(self._run(None), 2) + + def test_an_empty_request_exits_2(self): + self.assertEqual(self._run(""), 2) + self.assertEqual(self._run(" \n\n"), 2) + + def test_another_vendors_device_is_refused(self): + self.assertEqual(self._run(P4_UART + "\n"), 3) + + def test_an_mi_child_is_refused(self): + self.assertEqual(self._run(MI_CHILD + "\n"), 3) + + def test_a_trailing_command_is_refused(self): + self.assertEqual(self._run(TEMBED + "; calc\n"), 3) + + def test_a_quote_break_is_refused(self): + self.assertEqual(self._run(TEMBED + '" ; calc ; "\n'), 3) + + def test_a_subexpression_is_refused(self): + self.assertEqual(self._run("$(calc)\n"), 3) + + def test_a_wildcard_is_refused(self): + self.assertEqual(self._run(TEMBED + "*\n"), 3) + + def test_a_path_is_refused(self): + self.assertEqual(self._run("C:\\Windows\\System32\\calc.exe\n"), 3) + + def test_two_devices_in_one_request_are_refused(self): + self.assertEqual(self._run(TEMBED + "\n" + ABSENT + "\n"), 3) + + def test_an_over_long_request_is_refused(self): + self.assertEqual(self._run("USB\\VID_303A&PID_4003\\" + "A" * 400 + "\n"), 3) + + def test_a_well_formed_id_for_an_absent_device_fails_loudly(self): + # The whole point: a wrong id must not exit 0. A caller cannot tell a + # silent success from a real one. + self.assertEqual(self._run(ABSENT + "\n"), 4) + + def test_the_request_is_consumed_on_read(self): + # A request left behind is replayed by the next run, on somebody else's + # device. + self._run(ABSENT + "\n") + self.assertFalse((espusb._win_to_local(self.work) / "test.target").exists()) + + +if __name__ == "__main__": + unittest.main() diff --git a/docs/agent-guide.md b/docs/agent-guide.md index bd9e385..8f79f4d 100644 --- a/docs/agent-guide.md +++ b/docs/agent-guide.md @@ -408,24 +408,70 @@ discriminator while you are guessing — a board that answers HTTP is running your firmware, and a board that answers nothing while its COM port refuses to open is in ROM mode behind a stale USB node. -To lose the prompt as well, an administrator can register a scheduled task -that runs the restart with SYSTEM privileges and let ordinary accounts start -it; an agent must not create that task itself. - -**Check what that task actually runs, because there is no `Restart-PnpDevice`.** -Windows PowerShell's PnpDevice module ships Get-, Enable- and Disable-PnpDevice -and nothing else, so the obvious-looking one-liner fails with -`CommandNotFoundException` on every device. The task then completes, reports -`LastTaskResult 1`, and from the outside is indistinguishable from a board that -refused to come back — which is how the 2026-09-17 pin-move run spent its S3 -half on a repair that had never worked. `schtasks /run` returns SUCCESS for -*starting* the task, never for what it did: read -`Get-ScheduledTaskInfo -TaskName | Select LastTaskResult` instead. -`pnputil /restart-device ` is the mechanism that works; -`tools/windows/restart-esp-usb.ps1` uses it now, with Disable+Enable as the -fallback, and says so when it is not elevated rather than blaming the board. -The task registered on this machine still carries the old one-liner and needs -re-registering by hand. +#### Losing the UAC prompt: the `mpftp-restart-esp-usb` task + +To lose the click as well, the privileged part runs in a scheduled task that +ordinary accounts may start on demand. **Ask whether it works before you plan +around it:** + +```bash +mpftp usb-restart --status # exits 1, and says why, if it would not work +mpftp usb-restart --list # attached VID_303A devices and their instance ids +mpftp usb-restart --instance 'USB\VID_303A&PID_4003\' +``` + +`--status` reads the action the task is really registered with, not just +whether a task by that name exists. That distinction is the whole reason this +section was rewritten: the task on this bench ran +`Get-PnpDevice | Restart-PnpDevice`, **there is no `Restart-PnpDevice`** in +Windows PowerShell's PnpDevice module, and so it failed on every device and +reported `LastTaskResult 1` — indistinguishable, from outside, from a board +that refused to come back. It cost the 2026-09-17 pin-move run its S3 half and +the 2026-09-21 spike its auto-suspend measurement. `schtasks /run` reports +success for *starting* a task, never for what it did, so read +`Get-ScheduledTaskInfo -TaskName | Select LastTaskResult` — or let +`mpftp usb-restart` read it for you. + +**It needs one elevated install before any of that works**, and an agent must +not try to do it. Copy `tools/windows/` to a plain Windows path — an elevated +shell cannot reliably read `\\wsl.localhost\...`, and on this bench it could +not read it at all — then ask the owner to run, once, in an administrator +PowerShell: + +```powershell +powershell.exe -NoProfile -ExecutionPolicy Bypass -File C:\\install-restart-esp-usb-task.ps1 +``` + +It is idempotent, it prints what it did, and it self-checks the registration +before it returns. Until it has run, **flashing an S3 over native USB needs a +human nearby** — plan the session so a wedge costs a wait, not a lost night. + +Two constraints shape the design, and both are worth keeping if you touch it. +The task runs as SYSTEM, so it must never execute anything an ordinary account +can write: the script is installed into `C:\Program Files\mpftp`, and changing +it costs another elevated install. What you *do* write unprivileged is one +instance id into `C:\ProgramData\mpftp\restart-esp-usb.target`, which the +script matches whole against an allow-list for a VID_303A composite parent, +never evaluates, and deletes on read. Name the device explicitly — the old +task matched `USB\VID_303A*` and would have bounced every Espressif board on +the bench together, which on this one is a second board mid-demo. + +The transcript is `C:\Program Files\mpftp\restart-esp-usb.log`, readable by +everyone and writable only by SYSTEM, so `LastTaskResult 1` can be told from a +board that really did not come back. Exit codes: 0 restarted, 2 no request, +3 request refused, 4 no such device attached, 5 the restart failed. + +`tools/windows/restart-esp-usb.ps1` also runs by hand with `-InstanceId` from +an elevated shell, and `-DryRun` does everything except touch the device. + +**Not yet proved on hardware** (mpftp#31, 2026-09-21): the fix was written and +tested as far as an unprivileged session can go — the refusals, the request +handling and the absent-device case all pass against the real script, and both +rules were watched failing against a deliberately loosened copy. Three things +wait on that one elevated install: the task reaching `LastTaskResult 0`, COM12 +disappearing and coming back, and a deliberately wrong instance id failing +through the *task* rather than through a direct run. Run them the moment it is +installed. ### Ctrl-C is not an interrupt inside `atexit` diff --git a/tools/windows/install-restart-esp-usb-task.ps1 b/tools/windows/install-restart-esp-usb-task.ps1 new file mode 100644 index 0000000..be8253d --- /dev/null +++ b/tools/windows/install-restart-esp-usb-task.ps1 @@ -0,0 +1,148 @@ +# Install (or repair) the no-UAC ESP32 USB recovery: run this ONCE, elevated. +# +# Restarting a USB device node needs elevation, and an unattended session has +# nobody to click a UAC prompt. The way round that is a scheduled task running +# as SYSTEM which any account may start on demand -- the pattern +# docs/agent-guide.md describes. This script is the whole of the elevated half: +# it puts the recovery script somewhere only administrators can write, creates +# the unprivileged request directory beside it, and registers the task to run +# the one against the other. +# +# From an administrator PowerShell (mpftp#31): +# +# powershell.exe -NoProfile -ExecutionPolicy Bypass -File "" +# +# It is idempotent -- run it again after editing restart-esp-usb.ps1, which is +# what a later fix costs. That price is deliberate: the task runs as SYSTEM, so +# nothing it executes may be writable by an ordinary account. The one thing an +# ordinary account writes is the request file, and that is data the recovery +# script matches against an allow-list, never code. +[CmdletBinding()] +param( + [string]$TaskName = 'mpftp-restart-esp-usb', + # Administrator-writable only. This is where the code goes. + [string]$InstallDir = (Join-Path $env:ProgramFiles 'mpftp'), + # User-writable. This is where the data goes. + [string]$RequestDir = (Join-Path $env:ProgramData 'mpftp') +) + +Set-StrictMode -Version Latest +$ErrorActionPreference = 'Stop' + +$source = Join-Path $PSScriptRoot 'restart-esp-usb.ps1' +$target = Join-Path $InstallDir 'restart-esp-usb.ps1' +$logPath = Join-Path $InstallDir 'restart-esp-usb.log' +$requestPath = Join-Path $RequestDir 'restart-esp-usb.target' + +$identity = [Security.Principal.WindowsIdentity]::GetCurrent() +$principal = New-Object Security.Principal.WindowsPrincipal($identity) +if (-not $principal.IsInRole([Security.Principal.WindowsBuiltInRole]::Administrator)) { + Write-Error ("This must run in an ADMINISTRATOR PowerShell -- it writes to $InstallDir and " + + "registers a SYSTEM task, and both are refused at medium integrity. " + + "Right-click PowerShell, Run as administrator, then run this file again.") + exit 1 +} +if (-not (Test-Path -LiteralPath $source)) { + Write-Error "cannot find the recovery script next to this installer: $source" + exit 1 +} + +function Set-ExplicitAcl { + # Inheritance off, and exactly the three rules we mean. Stated rather than + # inherited, so it does not quietly change when a parent directory does. + param([string]$Path, [System.Security.AccessControl.FileSystemRights]$UsersRights) + + $acl = Get-Acl -LiteralPath $Path + $acl.SetAccessRuleProtection($true, $false) + foreach ($rule in @($acl.Access)) { [void]$acl.RemoveAccessRuleSpecific($rule) } + $inherit = 'ContainerInherit,ObjectInherit' + foreach ($pair in @( + @{ Sid = 'S-1-5-18'; Rights = [System.Security.AccessControl.FileSystemRights]::FullControl } # SYSTEM + @{ Sid = 'S-1-5-32-544'; Rights = [System.Security.AccessControl.FileSystemRights]::FullControl } # Administrators + @{ Sid = 'S-1-5-32-545'; Rights = $UsersRights } # Users + )) { + $acl.AddAccessRule((New-Object System.Security.AccessControl.FileSystemAccessRule( + (New-Object System.Security.Principal.SecurityIdentifier $pair.Sid), + $pair.Rights, $inherit, 'None', 'Allow'))) + } + Set-Acl -LiteralPath $Path -AclObject $acl +} + +# ------------------------------------------------------- the code, admin-only + +New-Item -ItemType Directory -Force -Path $InstallDir | Out-Null +Set-ExplicitAcl -Path $InstallDir -UsersRights ([System.Security.AccessControl.FileSystemRights]::ReadAndExecute) +Copy-Item -LiteralPath $source -Destination $target -Force +Write-Output "installed $target (Administrators/SYSTEM write, Users read+execute)" + +if (-not (Test-Path -LiteralPath $logPath)) { + New-Item -ItemType File -Force -Path $logPath | Out-Null +} +Write-Output "transcript $logPath (SYSTEM appends, Users read -- the account that writes requests cannot rewrite the record of them)" + +# -------------------------------------------------- the request, user-writable + +New-Item -ItemType Directory -Force -Path $RequestDir | Out-Null +Set-ExplicitAcl -Path $RequestDir -UsersRights ([System.Security.AccessControl.FileSystemRights]::Modify) +Write-Output "requests $requestPath (Users write -- data only; one instance id, matched against an allow-list, never executed)" + +# ------------------------------------------------------------------- the task + +$action = New-ScheduledTaskAction -Execute 'powershell.exe' -Argument ( + '-NoProfile -NonInteractive -ExecutionPolicy Bypass -WindowStyle Hidden -File "{0}"' -f $target) +$taskPrincipal = New-ScheduledTaskPrincipal -UserId 'SYSTEM' -LogonType ServiceAccount -RunLevel Highest +$settings = New-ScheduledTaskSettingsSet -AllowStartIfOnBatteries -DontStopIfGoingOnBatteries ` + -MultipleInstances IgnoreNew -ExecutionTimeLimit (New-TimeSpan -Minutes 5) +$settings.AllowDemandStart = $true + +Register-ScheduledTask -TaskName $TaskName -Action $action -Principal $taskPrincipal ` + -Settings $settings -Description ( + 'mpftp#31: restart one Espressif USB device node named by ' + $requestPath + + '. Recovers an ESP32-S3 wedged by machine.bootloader() without a UAC prompt.') -Force | Out-Null +Write-Output "registered task '$TaskName' -> $target" + +# Let ordinary accounts start it, which is the entire point. This is the +# default for a SYSTEM task registered by an administrator; state it anyway, so +# the no-prompt path does not depend on a default staying put. +try { + $service = New-Object -ComObject 'Schedule.Service' + $service.Connect() + $registered = $service.GetFolder('\').GetTask($TaskName) + # Administrators and SYSTEM full control; Authenticated Users read + run. + $registered.SetSecurityDescriptor('D:(A;;GA;;;BA)(A;;GA;;;SY)(A;;GRGX;;;AU)', 0) + Write-Output "task security Administrators/SYSTEM full, Authenticated Users read+run" +} catch { + Write-Output "task security left at the default (could not set it: $($_.Exception.Message))" +} + +# ------------------------------------------------------------- prove the wiring + +$registeredArgs = (Get-ScheduledTask -TaskName $TaskName).Actions[0].Arguments +if ($registeredArgs -notlike "*$target*") { + Write-Error "the registered action does not point at $target -- it is: $registeredArgs" + exit 1 +} +if ($registeredArgs -like '*Restart-PnpDevice*') { + Write-Error "the registered action still calls Restart-PnpDevice, which does not exist: $registeredArgs" + exit 1 +} + +& powershell.exe -NoProfile -NonInteractive -ExecutionPolicy Bypass -File $target ` + -InstanceId 'not-an-instance-id' -LogPath $logPath -DryRun | Out-Null +if ($LASTEXITCODE -ne 3) { + Write-Error "self-check failed: a malformed instance id should exit 3, got $LASTEXITCODE" + exit 1 +} +Write-Output "self-check a malformed instance id is refused with exit 3" + +Write-Output '' +Write-Output 'Done. Nothing else needs elevation. To prove it on a board, as an ordinary user:' +Write-Output '' +Write-Output ' mpftp usb-restart --status' +Write-Output ' mpftp usb-restart --instance "USB\VID_303A&PID_4003\"' +Write-Output '' +Write-Output "or without mpftp: write the instance id into $requestPath, then" +Write-Output " schtasks /run /tn $TaskName" +Write-Output " Get-ScheduledTaskInfo -TaskName $TaskName | Select-Object LastTaskResult" +Write-Output " Get-Content '$logPath' -Tail 20" +exit 0 diff --git a/tools/windows/restart-esp-usb.ps1 b/tools/windows/restart-esp-usb.ps1 index 67d4664..bc26975 100644 --- a/tools/windows/restart-esp-usb.ps1 +++ b/tools/windows/restart-esp-usb.ps1 @@ -1,74 +1,221 @@ -# Restart the USB device node of any attached Espressif board (VID 303A). +# Restart one Espressif USB device node, named by instance id. # # This is the repair for the ESP32-S3 native-USB wedge described in # docs/agent-guide.md: after `mpftp bootloader` or a direct -# `machine.bootloader()`, Windows goes on reporting a healthy device while -# every attempt to open its COM port fails as "busy or locked", with no -# process holding the handle. Restarting the device node makes Windows -# enumerate what the chip is really presenting -- a board sitting in ROM -# download mode then appears as 303A:1001 on a new COM number. +# `machine.bootloader()`, the firmware deletes its own USB PHY and reboots, and +# Windows can be left serving a node the chip no longer presents. Every open +# then fails -- "busy or locked", error 31, no process holding the handle -- +# until the node is re-enumerated. Restarting it makes Windows read what the +# chip is really presenting; a board sitting in ROM download mode then appears +# as 303A:1001 on a new COM number. # -# Needs elevation. From an ordinary shell (WSL included), this pops one UAC -# prompt and grants nothing that outlives it: +# Restarting a device node needs elevation, so unattended sessions reach this +# script through the scheduled task `mpftp-restart-esp-usb`, which runs it as +# SYSTEM and which any account may start on demand without a UAC prompt. +# Register it with tools/windows/install-restart-esp-usb-task.ps1, once, from an +# administrator PowerShell. # -# powershell.exe -NoProfile -Command "Start-Process powershell -Verb RunAs ` -# -ArgumentList '-NoProfile','-ExecutionPolicy','Bypass','-File',''" +# ## The rules that keep a SYSTEM task from being a back door # -# **There is no `Restart-PnpDevice`.** Windows PowerShell's PnpDevice module -# ships Get-, Enable- and Disable-PnpDevice and nothing else, so a script that -# calls Restart-PnpDevice fails with CommandNotFoundException on every device -# and exits 1 -- while a scheduled task wrapping it reports LastTaskResult 1 -# and looks, from the outside, exactly like a board that refused to restart. -# Found the expensive way on 2026-09-17, mid flash cycle. `pnputil -# /restart-device` is the real mechanism; Disable+Enable is the fallback for a -# Windows older than 2004. +# A task running as SYSTEM must never execute anything an ordinary user can +# write, so this script lives beside the task in an administrator-only +# directory (C:\Program Files\mpftp), and changing it costs another elevated +# install. That is the correct price. +# +# What an ordinary user may write is the *request file*, and it is data, never +# code. One line, matched whole against a strict allow-list for an Espressif +# instance id, never evaluated, never concatenated into a command string -- +# pnputil is called with an argument array. Anything else is refused with a +# non-zero exit and a logged sentence. The request is consumed (deleted) on +# read, so a rejected or stale one is never replayed. +# +# One device per run, by explicit id. The predecessor matched `USB\VID_303A*` +# and would have bounced every attached Espressif board together -- on this +# bench that is a second board mid-demo. +# +# ## There is no Restart-PnpDevice +# +# Windows PowerShell's PnpDevice module ships Get-, Enable- and Disable-PnpDevice +# and nothing else, so a script that calls Restart-PnpDevice fails with +# CommandNotFoundException on every device and exits 1 -- while a scheduled task +# wrapping it reports LastTaskResult 1 and looks, from the outside, exactly like +# a board that refused to restart. That is what the task registered here +# actually ran, from whenever it was created until 2026-09-21, and it is why +# mpftp#31 exists. `pnputil /restart-device` is the mechanism that works; +# Disable+Enable is the fallback for a Windows older than 10 2004. +# +# Exit codes are the whole report when this runs as a task: 0 restarted (or, +# under -DryRun, would have), 2 no request, 3 request refused, 4 no such device +# attached, 5 the restart itself failed. [CmdletBinding()] param( - # Espressif's vendor ID. Narrow it (e.g. 'USB\VID_303A&PID_4001*') when two - # boards are attached and only one should be restarted. - [string]$Match = 'USB\VID_303A*' + # The device to restart. Omitted -- which is how the scheduled task runs -- + # it is read from -RequestPath instead. + [string]$InstanceId, + + # Where an unprivileged caller leaves the instance id. Deleted on read. + [string]$RequestPath = (Join-Path $env:ProgramData 'mpftp\restart-esp-usb.target'), + + # Appended to, never truncated. Lives in the admin-only directory so the + # account that writes requests cannot rewrite the record of them. + [string]$LogPath = (Join-Path $PSScriptRoot 'restart-esp-usb.log'), + + # Validate, resolve and report, but do not touch the device. Everything up + # to the restart is read-only, so this is the part that can be tested by an + # ordinary user with a board in use nearby. + [switch]$DryRun ) -$identity = [Security.Principal.WindowsIdentity]::GetCurrent() -$principal = New-Object Security.Principal.WindowsPrincipal($identity) -if (-not $principal.IsInRole([Security.Principal.WindowsBuiltInRole]::Administrator)) { - Write-Output "not elevated -- every restart below will fail with Access is denied" +Set-StrictMode -Version Latest +$ErrorActionPreference = 'Stop' + +# Espressif's vendor ID, a 4-hex-digit product id, and a serial of the +# characters Windows actually puts there. Anchored both ends, so a trailing +# `; calc` or a second path component cannot ride along. The `\\` after the +# product id is deliberate: it admits the composite *parent* and rejects its +# `&MI_00` children -- re-enumerating the parent brings the interfaces with it, +# and a child whose parent is about to vanish reports a confusing failure of +# its own. +$ALLOWED_INSTANCE_ID = '^USB\\VID_303A&PID_[0-9A-Fa-f]{4}\\[0-9A-Za-z&_.\-]+$' +$MAX_REQUEST_CHARS = 200 + +function Write-Log { + param([string]$Message) + $line = '{0} {1}' -f (Get-Date -Format 'yyyy-MM-ddTHH:mm:ssK'), $Message + Write-Output $line + try { + $dir = Split-Path -Parent $LogPath + if ($dir -and -not (Test-Path -LiteralPath $dir)) { + New-Item -ItemType Directory -Force -Path $dir | Out-Null + } + Add-Content -LiteralPath $LogPath -Value $line -Encoding UTF8 -ErrorAction Stop + } catch { + # A task with nowhere to write its transcript still has its exit code, + # which is the thing the caller reads first. Do not fail the repair. + Write-Output ('{0} (transcript unavailable: {1})' -f $line, $_.Exception.Message) + } } -# Restart the composite parent, not its MI_ children: re-enumerating the -# parent brings the interfaces with it, and a child whose parent is about to -# vanish reports a confusing failure of its own. -$devices = Get-PnpDevice -PresentOnly | - Where-Object { $_.InstanceId -like $Match -and $_.InstanceId -notmatch '&MI_' } -if (-not $devices) { - Write-Output "no device matching $Match is attached" - exit 1 +function Show-Refused { + # Echo a refused request back to the log without letting it forge log lines + # or blow the file up: control characters out, length capped. + param([string]$Text) + if ($null -eq $Text) { return '' } + $flat = ($Text -replace '[\x00-\x1F\x7F]', '?') + if ($flat.Length -gt $MAX_REQUEST_CHARS) { + $flat = $flat.Substring(0, $MAX_REQUEST_CHARS) + '...' + } + return "'" + $flat + "'" } -$failed = 0 -foreach ($d in $devices) { - Write-Output ("restarting {0} [{1}]" -f $d.InstanceId, $d.Status) - $out = & pnputil.exe /restart-device $d.InstanceId 2>&1 - if ($LASTEXITCODE -eq 0 -and ($out -join ' ') -notmatch 'Failed to restart') { - Write-Output " ok (pnputil)" - continue +Write-Log ('--- restart-esp-usb starting (dryRun={0}, user={1}) ---' -f ` + [bool]$DryRun, [Security.Principal.WindowsIdentity]::GetCurrent().Name) + +# ---------------------------------------------------------------- the request + +if (-not $InstanceId) { + if (-not (Test-Path -LiteralPath $RequestPath)) { + Write-Log ('no request: {0} does not exist. Nothing to restart.' -f $RequestPath) + exit 2 + } + $raw = $null + try { + $raw = Get-Content -LiteralPath $RequestPath -Raw -ErrorAction Stop + } catch { + Write-Log ('request unreadable: {0}' -f $_.Exception.Message) + exit 2 + } finally { + # Consume it either way. A request that is left behind gets replayed by + # the next run of the task, which is somebody else's device. + try { Remove-Item -LiteralPath $RequestPath -Force -ErrorAction Stop } + catch { Write-Log ('warning: could not delete {0}: {1}' -f $RequestPath, $_.Exception.Message) } + } + + if ([string]::IsNullOrWhiteSpace($raw)) { + Write-Log ('request refused: {0} is empty.' -f $RequestPath) + exit 2 + } + if ($raw.Length -gt $MAX_REQUEST_CHARS) { + Write-Log ('request refused: {0} chars, limit is {1}.' -f $raw.Length, $MAX_REQUEST_CHARS) + exit 3 + } + $lines = @($raw -split "`r`n|`n|`r" | Where-Object { -not [string]::IsNullOrWhiteSpace($_) }) + if ($lines.Count -ne 1) { + Write-Log ('request refused: expected exactly one line, got {0}. One device per run.' -f $lines.Count) + exit 3 } - Write-Output (" pnputil: {0}" -f (($out | Where-Object { $_ -match '\S' }) -join '; ')) + $InstanceId = $lines[0].Trim() +} + +if ($InstanceId -notmatch $ALLOWED_INSTANCE_ID) { + Write-Log ('request refused: {0} is not an Espressif USB instance id. It must match {1} -- the composite parent of a VID_303A device, not an &MI_ child and not anything else.' -f ` + (Show-Refused $InstanceId), $ALLOWED_INSTANCE_ID) + exit 3 +} +Write-Log ('target {0}' -f $InstanceId) + +# ---------------------------------------------------------------- the device + +$device = $null +try { + $device = Get-PnpDevice -PresentOnly -InstanceId $InstanceId -ErrorAction Stop +} catch { + $device = $null +} +if (-not $device) { + Write-Log ('no device with instance id {0} is attached. Refusing to report success for a board that is not there.' -f $InstanceId) + exit 4 +} +Write-Log ('found "{0}" status={1}' -f $device.FriendlyName, $device.Status) + +if ($DryRun) { + Write-Log 'dry run: would restart it now. Stopping here without touching the device.' + exit 0 +} + +# ---------------------------------------------------------------- the restart + +$identity = [Security.Principal.WindowsIdentity]::GetCurrent() +$principal = New-Object Security.Principal.WindowsPrincipal($identity) +if (-not $principal.IsInRole([Security.Principal.WindowsBuiltInRole]::Administrator)) { + Write-Log 'not elevated -- the restart below will fail with "Access is denied". Run this through the mpftp-restart-esp-usb scheduled task, which runs as SYSTEM.' +} + +$restarted = $false +# An argument array, never a command string: the id reached this line as data +# and it stays data. +$pnputilArgs = @('/restart-device', $InstanceId) +$out = & pnputil.exe @pnputilArgs 2>&1 +$text = (($out | Where-Object { "$_" -match '\S' }) -join '; ') +if ($LASTEXITCODE -eq 0 -and $text -notmatch 'Failed to restart') { + Write-Log ('pnputil /restart-device ok: {0}' -f $text) + $restarted = $true +} else { + Write-Log ('pnputil /restart-device failed (exit {0}): {1}' -f $LASTEXITCODE, $text) try { - Disable-PnpDevice -InstanceId $d.InstanceId -Confirm:$false -ErrorAction Stop + Disable-PnpDevice -InstanceId $InstanceId -Confirm:$false -ErrorAction Stop Start-Sleep -Milliseconds 700 - Enable-PnpDevice -InstanceId $d.InstanceId -Confirm:$false -ErrorAction Stop - Write-Output " ok (disable/enable)" + Enable-PnpDevice -InstanceId $InstanceId -Confirm:$false -ErrorAction Stop + Write-Log 'disable/enable fallback ok' + $restarted = $true } catch { - Write-Output (" failed: {0}" -f $_.Exception.Message) - $failed++ + Write-Log ('disable/enable fallback failed: {0}' -f $_.Exception.Message) } } -Start-Sleep -Seconds 3 -Get-PnpDevice -PresentOnly | - Where-Object { $_.InstanceId -like 'USB\VID_303A*' } | - Select-Object Status, InstanceId | - Format-Table -AutoSize | Out-String | Write-Output +if (-not $restarted) { + Write-Log 'restart FAILED.' + exit 5 +} -exit $(if ($failed) { 1 } else { 0 }) +Start-Sleep -Seconds 3 +$after = Get-PnpDevice -PresentOnly -InstanceId $InstanceId -ErrorAction SilentlyContinue +if ($after) { + Write-Log ('back as "{0}" status={1}' -f $after.FriendlyName, $after.Status) +} else { + # Expected, and not a failure: a board that entered ROM download mode + # re-enumerates as a different device (303A:1001 on a new COM number). + Write-Log 'that instance id is gone -- the board has re-enumerated as a different device. Look for 303A:1001 on a new COM number.' +} +Write-Log '--- restart-esp-usb done ---' +exit 0 From efbbb4ba18323da186086b77f7530c08800e9606 Mon Sep 17 00:00:00 2001 From: Brad Barnett <127794626+bdbarnett@users.noreply.github.com> Date: Mon, 21 Sep 2026 08:02:07 -0500 Subject: [PATCH 2/6] restart-esp-usb: a SYSTEM task reading inside a user-owned directory is a link-following bug waiting to happen The task runs as SYSTEM and both reads and deletes C:\ProgramData\mpftp\restart-esp-usb.target, inside a directory ordinary accounts write to. The first cut granted Users Modify on that directory, which includes DELETE on the directory itself -- so a user could remove it and recreate it as a junction pointing anywhere, and SYSTEM's Remove-Item would follow. Closing both halves. The directory. Users now get exactly CreateFiles + ReadAndExecute on the folder itself, and Modify on files within it (ObjectInherit, InheritOnly). No Delete on the folder, no CreateDirectories, no WriteAttributes, no ChangePermissions. Ownership goes to Administrators, because an owner keeps the implicit right to rewrite the permissions that constrain it. An admin-only .keep file keeps the directory non-empty, and a non-empty directory cannot be converted into a reparse point. The installer refuses to install onto an existing reparse point, re-applies the ACL if the directory is already there, self-checks that .keep landed, and prints the resulting Users ACEs so the claim is checkable. Verified against a scratch directory with the shipped helpers, as an ordinary user: drop a request allowed, rewrite it allowed, delete it allowed; create a subdirectory refused, delete .keep refused, delete the directory refused, rmdir it refused. (Measured while still owning the directory, so the DACL is doing the work -- an owner's implicit rights are READ_CONTROL and WRITE_DAC, not data access.) The script. Before reading, and again before deleting, it refuses a request that is a symlink, a hard link, a directory, or sits beneath a reparse point -- exit 3, nothing read, nothing deleted, and the contents are NOT echoed, because the log is world-readable and with Developer Mode on the link could point at something only SYSTEM can read. Both conditions are load-bearing: measured here, all three link types are creatable unprivileged, and a hard link carries no ReparsePoint attribute (only LinkType says so) while a WSL symlink carries the attribute with a blank LinkType. Either check alone misses one. The re-check before the delete narrows but does not close the check-to-syscall gap; doing that needs FILE_FLAG_OPEN_REPARSE_POINT, which PowerShell cannot open. 33 tests, four new ones covering a symlinked request, a hard-linked request, a request inside a junction, and the absence of a content leak. Each was watched failing against a planted fault: link checks neutered turns all four red, the regex loosened to ^USB\VID_ turns five red, the one-line check loosened turns one red, echoing contents on a link refusal turns two red. Also fixes the harness itself, which was quietly wrong: it used os.path.dirname on a Windows path under Linux Python, which returns "", so the override used for planted-fault runs pointed every request at the current directory. The earlier fault runs resolved by accident. ntpath.dirname now, and the numbers above were re-measured after the fix. What this does not defend against: malware already running as the desktop account on a machine where that account is an administrator has other ways up. This is about mpftp not adding one. Said as much in the guide. The owner's one command is unchanged. --- CHANGELOG.md | 10 ++ cli/tests/test_esp_usb_restart.py | 140 ++++++++++++++++-- docs/agent-guide.md | 11 ++ .../windows/install-restart-esp-usb-task.ps1 | 134 ++++++++++++++--- tools/windows/restart-esp-usb.ps1 | 75 +++++++++- 5 files changed, 329 insertions(+), 41 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 37cca3c..0bad13e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,16 @@ installed before planning around it, `--list` to find instance ids rather than hard-coding them, `--instance` to drive it. `--status` inspects the action the task is registered with, so a dead recovery reads as dead. +- Harden the request path against link-following, since a SYSTEM task reads and + deletes inside a directory ordinary accounts write to. The request directory + grants Users only CreateFiles on the folder plus Modify on files within it — + no Delete on the folder, no CreateDirectories — is owned by Administrators, + and holds an admin-only `.keep` so it can never be emptied and converted into + a junction; the installer refuses to run onto an existing reparse point. The + script refuses a request that is a symlink, a hard link, or sits beneath a + reparse point, deletes nothing when it does, and does not echo the contents. + Both conditions are needed: measured here, a hard link carries no ReparsePoint + attribute, and a WSL symlink carries it with a blank `LinkType`. - Add `tools/windows/install-restart-esp-usb-task.ps1`: the one elevated step. The task runs as SYSTEM, so the script it executes is installed where only administrators can write it; the one thing an unprivileged caller supplies is diff --git a/cli/tests/test_esp_usb_restart.py b/cli/tests/test_esp_usb_restart.py index 869d140..f19d402 100644 --- a/cli/tests/test_esp_usb_restart.py +++ b/cli/tests/test_esp_usb_restart.py @@ -11,17 +11,23 @@ the device, so they are safe to run as an ordinary user with boards in use on the bench. They skip where there is no powershell.exe. -To show these can fail, point MPFTP_ESP_USB_SCRIPT at a copy with the rule -loosened; the refusal cases go red. Loosening the regex to ``^USB\\VID_`` on -2026-09-21 turned five of them red, including accepting another vendor's UART. +To show these can fail, point MPFTP_ESP_USB_SCRIPT at a copy of the script with +a rule loosened. Measured 2026-09-21, against the four that matter: + + regex loosened to ``^USB\\VID_`` 5 red, incl. accepting another vendor's UART + one-line check ``-ne 1`` -> ``-lt 1`` 1 red, two devices in one request + link checks made to return $null 4 red, every link case + contents echoed on a link refusal 2 red, incl. the leak assertion """ from __future__ import annotations +import ntpath import os import shutil import subprocess import unittest +import uuid from pathlib import Path from unittest import mock @@ -146,16 +152,21 @@ def _powershell() -> str | None: return shutil.which("powershell.exe") or shutil.which("powershell") -@unittest.skipIf(_powershell() is None, "needs Windows PowerShell (interop from WSL)") -class DryRunRefusalTests(unittest.TestCase): - """The script's own rule, exercised at medium integrity, device untouched.""" +class _StagedScript: + """Stages the real script somewhere powershell.exe can run it, and drives it.""" @classmethod def setUpClass(cls): override = os.environ.get("MPFTP_ESP_USB_SCRIPT") if override: cls.script = override - cls.work = os.path.dirname(override) or "." + # ntpath, not os.path: this is a Windows path and these tests run + # under Linux Python, where os.path.dirname sees no separator in it + # and returns "" -- which silently pointed every request path at the + # current directory instead. + cls.work = ntpath.dirname(override) + if not cls.work: + raise unittest.SkipTest(f"MPFTP_ESP_USB_SCRIPT needs an absolute path, got {override!r}") return # A .ps1 under \\wsl.localhost\ is awkward for powershell.exe to run; # stage it on the Windows side instead. @@ -171,23 +182,35 @@ def setUpClass(cls): shutil.copy(REPO_SCRIPT, local / "restart-esp-usb.ps1") cls.script = cls.work + r"\restart-esp-usb.ps1" - def _run(self, request_text: str | None) -> int: - local_dir = espusb._win_to_local(self.work) - request = local_dir / "test.target" - if request_text is None: - request.unlink(missing_ok=True) - else: - request.write_text(request_text, encoding="utf-8") + def _run_path(self, win_request: str) -> int: proc = subprocess.run( [_powershell(), "-NoProfile", "-NonInteractive", "-ExecutionPolicy", "Bypass", "-File", self.script, - "-RequestPath", self.work + r"\test.target", + "-RequestPath", win_request, "-LogPath", self.work + r"\test.log", "-DryRun"], capture_output=True, text=True, ) return proc.returncode + def _run(self, request_text: str | None) -> int: + local_dir = espusb._win_to_local(self.work) + request = local_dir / "test.target" + if request_text is None: + request.unlink(missing_ok=True) + else: + request.write_text(request_text, encoding="utf-8") + return self._run_path(self.work + r"\test.target") + + def _log_text(self) -> str: + log = espusb._win_to_local(self.work) / "test.log" + return log.read_text(encoding="utf-8", errors="replace") if log.exists() else "" + + +@unittest.skipIf(_powershell() is None, "needs Windows PowerShell (interop from WSL)") +class DryRunRefusalTests(_StagedScript, unittest.TestCase): + """The script's own rule, exercised at medium integrity, device untouched.""" + def test_a_missing_request_exits_2(self): self.assertEqual(self._run(None), 2) @@ -234,5 +257,92 @@ def test_the_request_is_consumed_on_read(self): self.assertFalse((espusb._win_to_local(self.work) / "test.target").exists()) +@unittest.skipIf(_powershell() is None, "needs Windows PowerShell (interop from WSL)") +class LinkFollowingTests(_StagedScript, unittest.TestCase): + """SYSTEM reads and deletes inside a directory an ordinary account owns. + + So the request must be a real file in a real directory. All three links + below were creatable unprivileged on this bench, and they do not present + alike: a hard link carries no ReparsePoint attribute, and a WSL symlink + carries the attribute with a blank LinkType. Either check alone misses one. + + This does not defend against malware already running as an administrator's + desktop account -- that has other routes up. It is about not adding one. + """ + + SECRET = "MPFTP31-SECRET-THAT-MUST-NOT-REACH-THE-LOG" + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.local = espusb._win_to_local(cls.work) + + def setUp(self): + for name in ("real.target", "link.target", "hard.target", "secret.txt"): + (self.local / name).unlink(missing_ok=True) + (self.local / "realdir").mkdir(exist_ok=True) + + def _make_junction(self) -> str: + # A fresh name per test: a junction left behind by an earlier run makes + # mklink fail with "Access is denied", which reads like a privilege + # problem and is not one. + name = "jdir-" + uuid.uuid4().hex[:8] + win = self.work + "\\" + name + proc = subprocess.run( + ["cmd.exe", "/c", "mklink", "/J", win, self.work + r"\realdir"], + capture_output=True, text=True, + ) + if not (self.local / name).exists(): + raise unittest.SkipTest(f"could not create a junction here: {proc.stdout} {proc.stderr}") + self.addCleanup(subprocess.run, ["cmd.exe", "/c", "rmdir", win], + capture_output=True, text=True) + return win + + def test_a_symlinked_request_is_refused_and_not_deleted(self): + real = self.local / "real.target" + real.write_text(TEMBED + "\n", encoding="utf-8") + link = self.local / "link.target" + try: + link.symlink_to(real) + except (OSError, NotImplementedError) as exc: + raise unittest.SkipTest(f"cannot create a file symlink here: {exc}") from None + self.assertEqual(self._run_path(self.work + r"\link.target"), 3) + # Refusing is half of it; SYSTEM must not have deleted through the link. + self.assertTrue(real.exists(), "the symlink's target was deleted") + + def test_a_hard_linked_request_is_refused(self): + real = self.local / "real.target" + real.write_text(TEMBED + "\n", encoding="utf-8") + hard = self.local / "hard.target" + try: + os.link(real, hard) + except (OSError, NotImplementedError) as exc: + raise unittest.SkipTest(f"cannot create a hard link here: {exc}") from None + # A hard link has no ReparsePoint attribute at all -- only LinkType + # tells you, which is why both conditions are checked. + self.assertEqual(self._run_path(self.work + r"\hard.target"), 3) + self.assertTrue(real.exists(), "the hard link's target was deleted") + + def test_a_request_inside_a_junction_is_refused(self): + junction = self._make_junction() + (self.local / "realdir" / "test.target").write_text(TEMBED + "\n", encoding="utf-8") + self.assertEqual(self._run_path(junction + r"\test.target"), 3) + self.assertTrue((self.local / "realdir" / "test.target").exists(), + "the request was deleted through a junction") + + def test_a_refused_link_does_not_leak_its_contents_to_the_log(self): + # The log is readable by everyone; with Developer Mode on, the link + # could point at something only SYSTEM can read. + secret = self.local / "secret.txt" + secret.write_text(self.SECRET + "\n", encoding="utf-8") + link = self.local / "link.target" + try: + link.symlink_to(secret) + except (OSError, NotImplementedError) as exc: + raise unittest.SkipTest(f"cannot create a file symlink here: {exc}") from None + self.assertEqual(self._run_path(self.work + r"\link.target"), 3) + self.assertNotIn(self.SECRET, self._log_text()) + + if __name__ == "__main__": unittest.main() diff --git a/docs/agent-guide.md b/docs/agent-guide.md index 8f79f4d..699bc5a 100644 --- a/docs/agent-guide.md +++ b/docs/agent-guide.md @@ -456,6 +456,17 @@ never evaluates, and deletes on read. Name the device explicitly — the old task matched `USB\VID_303A*` and would have bounced every Espressif board on the bench together, which on this one is a second board mid-demo. +A SYSTEM process that reads and deletes inside a user-writable directory is +the shape of a link-following bug, so the request directory grants Users only +"add a file here" and "modify files in here" — not delete-the-folder, not +make-a-subdirectory — and holds a `.keep` the user cannot remove, because a +non-empty directory cannot be turned into a junction. The script refuses a +request that is a symlink, a hard link, or sits under a reparse point, and +says so without echoing what it found. **To be honest about what that buys:** +malware already running as the desktop account on a machine where that account +is an administrator has other ways up, and this does not stop it — the point +is that mpftp should not *add* one. + The transcript is `C:\Program Files\mpftp\restart-esp-usb.log`, readable by everyone and writable only by SYSTEM, so `LastTaskResult 1` can be told from a board that really did not come back. Exit codes: 0 restarted, 2 no request, diff --git a/tools/windows/install-restart-esp-usb-task.ps1 b/tools/windows/install-restart-esp-usb-task.ps1 index be8253d..d8a9546 100644 --- a/tools/windows/install-restart-esp-usb-task.ps1 +++ b/tools/windows/install-restart-esp-usb-task.ps1 @@ -47,31 +47,81 @@ if (-not (Test-Path -LiteralPath $source)) { exit 1 } -function Set-ExplicitAcl { - # Inheritance off, and exactly the three rules we mean. Stated rather than - # inherited, so it does not quietly change when a parent directory does. - param([string]$Path, [System.Security.AccessControl.FileSystemRights]$UsersRights) - - $acl = Get-Acl -LiteralPath $Path - $acl.SetAccessRuleProtection($true, $false) - foreach ($rule in @($acl.Access)) { [void]$acl.RemoveAccessRuleSpecific($rule) } - $inherit = 'ContainerInherit,ObjectInherit' - foreach ($pair in @( - @{ Sid = 'S-1-5-18'; Rights = [System.Security.AccessControl.FileSystemRights]::FullControl } # SYSTEM - @{ Sid = 'S-1-5-32-544'; Rights = [System.Security.AccessControl.FileSystemRights]::FullControl } # Administrators - @{ Sid = 'S-1-5-32-545'; Rights = $UsersRights } # Users - )) { - $acl.AddAccessRule((New-Object System.Security.AccessControl.FileSystemAccessRule( - (New-Object System.Security.Principal.SecurityIdentifier $pair.Sid), - $pair.Rights, $inherit, 'None', 'Allow'))) +$RIGHTS = [System.Security.AccessControl.FileSystemRights] +$SID_SYSTEM = 'S-1-5-18' +$SID_ADMINS = 'S-1-5-32-544' +$SID_USERS = 'S-1-5-32-545' + +function New-Sid { param([string]$Value) New-Object System.Security.Principal.SecurityIdentifier $Value } + +function New-Rule { + param([string]$Sid, $Rights, [string]$Inherit = 'None', [string]$Propagation = 'None') + New-Object System.Security.AccessControl.FileSystemAccessRule( + (New-Sid $Sid), $Rights, $Inherit, $Propagation, 'Allow') +} + +function Clear-InheritedAcl { + # Inheritance off and every existing rule dropped, so what follows is + # exactly what we mean rather than whatever a parent directory grants. + param($Acl) + $Acl.SetAccessRuleProtection($true, $false) + foreach ($rule in @($Acl.Access)) { [void]$Acl.RemoveAccessRuleSpecific($rule) } + return $Acl +} + +function Set-AdminOnlyAcl { + # For the directory that holds code, and for the placeholder file. Users may + # look; only SYSTEM and administrators may write. + param([string]$Path, [switch]$NoInherit) + + $acl = Clear-InheritedAcl (Get-Acl -LiteralPath $Path) + $inherit = if ($NoInherit) { 'None' } else { 'ContainerInherit,ObjectInherit' } + $acl.AddAccessRule((New-Rule $SID_SYSTEM $RIGHTS::FullControl $inherit)) + $acl.AddAccessRule((New-Rule $SID_ADMINS $RIGHTS::FullControl $inherit)) + if (-not $NoInherit) { + $acl.AddAccessRule((New-Rule $SID_USERS $RIGHTS::ReadAndExecute $inherit)) } Set-Acl -LiteralPath $Path -AclObject $acl } +function Set-RequestDirAcl { + # The one directory an ordinary account writes to -- and the one SYSTEM + # reads and deletes from, which is what makes its permissions matter. + # + # Users get exactly enough to leave a request and manage their own: add a + # file to this folder, and Modify on files inside it. They do NOT get Delete + # on the folder itself, nor CreateDirectories, nor WriteAttributes, nor + # ChangePermissions. So the directory cannot be removed and recreated as a + # junction pointing at \RPC Control or anywhere else -- which is the whole + # reason for spelling this out rather than granting Modify and moving on. + param([string]$Path) + + $acl = Clear-InheritedAcl (Get-Acl -LiteralPath $Path) + $acl.AddAccessRule((New-Rule $SID_SYSTEM $RIGHTS::FullControl 'ContainerInherit,ObjectInherit')) + $acl.AddAccessRule((New-Rule $SID_ADMINS $RIGHTS::FullControl 'ContainerInherit,ObjectInherit')) + # This folder only: look at it, and add a file to it. Nothing else. + $acl.AddAccessRule((New-Rule $SID_USERS ($RIGHTS::ReadAndExecute -bor $RIGHTS::CreateFiles) 'None' 'None')) + # Files inside it, inherit-only: write and remove your own request. + $acl.AddAccessRule((New-Rule $SID_USERS $RIGHTS::Modify 'ObjectInherit' 'InheritOnly')) + Set-Acl -LiteralPath $Path -AclObject $acl + + # An owner always keeps the implicit right to rewrite the permissions above, + # so the directory must not be owned by the account it constrains. + try { + $owner = Get-Acl -LiteralPath $Path + $owner.SetOwner((New-Sid $SID_ADMINS)) + # Explicit, because Set-Acl reports this one as a non-terminating error + # and the catch below would otherwise never run. + Set-Acl -LiteralPath $Path -AclObject $owner -ErrorAction Stop + } catch { + Write-Output (" note: could not set the owner to Administrators: " + $_.Exception.Message) + } +} + # ------------------------------------------------------- the code, admin-only New-Item -ItemType Directory -Force -Path $InstallDir | Out-Null -Set-ExplicitAcl -Path $InstallDir -UsersRights ([System.Security.AccessControl.FileSystemRights]::ReadAndExecute) +Set-AdminOnlyAcl -Path $InstallDir Copy-Item -LiteralPath $source -Destination $target -Force Write-Output "installed $target (Administrators/SYSTEM write, Users read+execute)" @@ -82,9 +132,31 @@ Write-Output "transcript $logPath (SYSTEM appends, Users read -- the account th # -------------------------------------------------- the request, user-writable -New-Item -ItemType Directory -Force -Path $RequestDir | Out-Null -Set-ExplicitAcl -Path $RequestDir -UsersRights ([System.Security.AccessControl.FileSystemRights]::Modify) -Write-Output "requests $requestPath (Users write -- data only; one instance id, matched against an allow-list, never executed)" +if (Test-Path -LiteralPath $RequestDir) { + # It may already have been replaced by something that redirects SYSTEM's + # read and delete elsewhere. Do not install onto that. + $existing = Get-Item -LiteralPath $RequestDir -Force + if ($existing.Attributes -band [IO.FileAttributes]::ReparsePoint) { + Write-Error ("$RequestDir is a reparse point (a junction or mount point), not a real directory. " + + "Refusing to install onto it. Inspect it, remove it by hand, and run this again.") + exit 1 + } +} else { + New-Item -ItemType Directory -Force -Path $RequestDir | Out-Null +} +Set-RequestDirAcl -Path $RequestDir + +# A directory with anything at all in it cannot be converted into a reparse +# point, and an ordinary account cannot remove this file. Together with the +# absent Delete on the directory, that is what keeps the path a real directory. +$keep = Join-Path $RequestDir '.keep' +Set-Content -LiteralPath $keep -Encoding UTF8 -Value @( + 'Keeps this directory non-empty, so it cannot be converted into a junction.', + 'An ordinary account cannot delete it. Do not remove it. mpftp#31.') +Set-AdminOnlyAcl -Path $keep -NoInherit + +Write-Output "requests $requestPath (Users may add a file here and rewrite their own; not delete the folder, not make one)" +Write-Output " $keep (SYSTEM/Administrators only -- keeps the folder non-empty)" # ------------------------------------------------------------------- the task @@ -135,6 +207,26 @@ if ($LASTEXITCODE -ne 3) { } Write-Output "self-check a malformed instance id is refused with exit 3" +if (-not (Test-Path -LiteralPath $keep)) { + Write-Error "self-check failed: $keep is missing, so the request directory could be emptied and converted into a junction" + exit 1 +} +if ((Get-Item -LiteralPath $RequestDir -Force).Attributes -band [IO.FileAttributes]::ReparsePoint) { + Write-Error "self-check failed: $RequestDir is a reparse point" + exit 1 +} +Write-Output "self-check the request directory is a real directory and cannot be emptied" + +# Print what Users actually ended up with, so the claim above is checkable +# rather than asserted. +Write-Output '' +Write-Output "what BUILTIN\Users may do in $RequestDir :" +(Get-Acl -LiteralPath $RequestDir).Access | + Where-Object { $_.IdentityReference -match 'Users$' } | + ForEach-Object { + Write-Output (' {0} [inherit {1}, propagate {2}]' -f $_.FileSystemRights, $_.InheritanceFlags, $_.PropagationFlags) +} + Write-Output '' Write-Output 'Done. Nothing else needs elevation. To prove it on a board, as an ordinary user:' Write-Output '' diff --git a/tools/windows/restart-esp-usb.ps1 b/tools/windows/restart-esp-usb.ps1 index bc26975..91cd56b 100644 --- a/tools/windows/restart-esp-usb.ps1 +++ b/tools/windows/restart-esp-usb.ps1 @@ -96,6 +96,52 @@ function Write-Log { } } +function Test-UnsafeRequestPath { + # SYSTEM reads and deletes this path, and an ordinary account owns the + # directory it sits in -- so refuse anything that could redirect either + # operation somewhere else. Returns a sentence to log, or $null if safe. + # + # Both conditions below are load-bearing, and neither subsumes the other. + # Measured on this bench, all three creatable by an ordinary user: + # a junction Attributes: Directory, ReparsePoint LinkType: Junction + # a WSL symlink Attributes: Archive, ReparsePoint LinkType: (blank) + # a hard link Attributes: Archive LinkType: HardLink + # The attribute alone misses the hard link; LinkType alone misses the + # symlink. A junction needs no privilege at all to create. + param([string]$Path) + + $reparse = [IO.FileAttributes]::ReparsePoint + $parent = Split-Path -Parent $Path + if ($parent) { + try { + $dir = Get-Item -LiteralPath $parent -Force -ErrorAction Stop + if ($dir.Attributes -band $reparse) { + return 'sits in a directory that is a reparse point (a junction or mount point), which would redirect this read somewhere else.' + } + } catch { + return ('sits in a directory that cannot be inspected: {0}' -f $_.Exception.Message) + } + } + + try { + $item = Get-Item -LiteralPath $Path -Force -ErrorAction Stop + } catch { + return ('cannot be inspected: {0}' -f $_.Exception.Message) + } + if ($item -is [System.IO.DirectoryInfo]) { + return 'is a directory, not a file.' + } + if ($item.Attributes -band $reparse) { + return 'is a reparse point (a symbolic link), not a regular file.' + } + $linkType = $null + try { $linkType = $item.LinkType } catch { $linkType = $null } + if (-not [string]::IsNullOrEmpty($linkType)) { + return ('is a {0}, not a regular file.' -f $linkType) + } + return $null +} + function Show-Refused { # Echo a refused request back to the log without letting it forge log lines # or blow the file up: control characters out, length capped. @@ -118,19 +164,38 @@ if (-not $InstanceId) { Write-Log ('no request: {0} does not exist. Nothing to restart.' -f $RequestPath) exit 2 } + # Before the read, and before the delete. Nothing about a refused path is + # echoed but the path itself -- with Developer Mode on, an ordinary user can + # make this a link to a file only SYSTEM can read, and this log is readable + # by everyone. + $unsafe = Test-UnsafeRequestPath -Path $RequestPath + if ($unsafe) { + Write-Log ('request refused: {0} {1} Nothing was read and nothing was deleted.' -f $RequestPath, $unsafe) + exit 3 + } + $raw = $null try { $raw = Get-Content -LiteralPath $RequestPath -Raw -ErrorAction Stop } catch { Write-Log ('request unreadable: {0}' -f $_.Exception.Message) exit 2 - } finally { - # Consume it either way. A request that is left behind gets replayed by - # the next run of the task, which is somebody else's device. - try { Remove-Item -LiteralPath $RequestPath -Force -ErrorAction Stop } - catch { Write-Log ('warning: could not delete {0}: {1}' -f $RequestPath, $_.Exception.Message) } } + # Re-checked immediately before the delete. This does not close the gap + # between check and syscall -- doing that needs an open handle with + # FILE_FLAG_OPEN_REPARSE_POINT, which is beyond PowerShell -- but it is the + # delete that is the dangerous half, so it is worth narrowing. + $unsafe = Test-UnsafeRequestPath -Path $RequestPath + if ($unsafe) { + Write-Log ('request refused between read and delete: {0} {1} Nothing was deleted.' -f $RequestPath, $unsafe) + exit 3 + } + # Consume it. A request that is left behind gets replayed by the next run + # of the task, which is somebody else's device. + try { Remove-Item -LiteralPath $RequestPath -Force -ErrorAction Stop } + catch { Write-Log ('warning: could not delete {0}: {1}' -f $RequestPath, $_.Exception.Message) } + if ([string]::IsNullOrWhiteSpace($raw)) { Write-Log ('request refused: {0} is empty.' -f $RequestPath) exit 2 From 857567526d6e0cddf7ea7ad7fd102ceb34709cdd Mon Sep 17 00:00:00 2001 From: Brad Barnett <127794626+bdbarnett@users.noreply.github.com> Date: Mon, 21 Sep 2026 11:16:07 -0500 Subject: [PATCH 3/6] restart-esp-usb: the task died in its own param block, and said nothing Under 'powershell.exe -File' -- which is how the scheduled task runs this -- $PSScriptRoot is still empty while the param block's defaults are evaluated (Windows PowerShell 5.1). The -LogPath default was 'Join-Path $PSScriptRoot ...', so the first real install ran, threw before its first log line, left the request unconsumed and exited 1: LastTaskResult 1, no transcript, the board untouched -- indistinguishable from the broken task it replaced. Every test passed -LogPath explicitly, and so did the installer's self-check, so the production invocation was the one form nothing exercised. The default is resolved in the script body now, the task is registered with -LogPath as well, the self-check runs the script the way the task does, and two tests run it with -File and no -LogPath (they fail against the old default: 2 of 35). --- cli/tests/test_esp_usb_restart.py | 33 +++++++++++++++++++ .../windows/install-restart-esp-usb-task.ps1 | 10 ++++-- tools/windows/restart-esp-usb.ps1 | 17 +++++++++- 3 files changed, 57 insertions(+), 3 deletions(-) diff --git a/cli/tests/test_esp_usb_restart.py b/cli/tests/test_esp_usb_restart.py index f19d402..3b719c5 100644 --- a/cli/tests/test_esp_usb_restart.py +++ b/cli/tests/test_esp_usb_restart.py @@ -202,6 +202,22 @@ def _run(self, request_text: str | None) -> int: request.write_text(request_text, encoding="utf-8") return self._run_path(self.work + r"\test.target") + def _run_as_the_task_does(self, *extra: str) -> int: + """`-File`, and NO `-LogPath` -- the scheduled task's own invocation. + + Every other leg here passes `-LogPath`, and that is what hid the first + install's failure (2026-09-21): under `powershell.exe -File`, + `$PSScriptRoot` is still empty while the param block's defaults are + evaluated, so a default of `Join-Path $PSScriptRoot ...` threw before + the first log line and the task exited 1 having done nothing. + """ + proc = subprocess.run( + [_powershell(), "-NoProfile", "-NonInteractive", "-ExecutionPolicy", "Bypass", + "-File", self.script, *extra], + capture_output=True, text=True, + ) + return proc.returncode + def _log_text(self) -> str: log = espusb._win_to_local(self.work) / "test.log" return log.read_text(encoding="utf-8", errors="replace") if log.exists() else "" @@ -344,5 +360,22 @@ def test_a_refused_link_does_not_leak_its_contents_to_the_log(self): self.assertNotIn(self.SECRET, self._log_text()) +@unittest.skipIf(_powershell() is None, "needs Windows PowerShell (interop from WSL)") +class TheTaskRunsItWithFileAndNoLogPath(_StagedScript, unittest.TestCase): + """The production invocation, which no other leg exercises.""" + + def test_a_malformed_id_is_refused_not_a_param_block_crash(self): + # 3 = request refused. 1 is what PowerShell returns when the script + # never got as far as its first statement. + self.assertEqual(self._run_as_the_task_does("-InstanceId", "not-an-id", "-DryRun"), 3) + + def test_it_writes_its_transcript_beside_itself_by_default(self): + log = espusb._win_to_local(self.work) / "restart-esp-usb.log" + log.unlink(missing_ok=True) + self._run_as_the_task_does("-InstanceId", "not-an-id", "-DryRun") + self.assertTrue(log.exists(), "no transcript beside the script: the default -LogPath did not resolve") + self.assertIn("request refused", log.read_text(encoding="utf-8", errors="replace")) + + if __name__ == "__main__": unittest.main() diff --git a/tools/windows/install-restart-esp-usb-task.ps1 b/tools/windows/install-restart-esp-usb-task.ps1 index d8a9546..8ca9d07 100644 --- a/tools/windows/install-restart-esp-usb-task.ps1 +++ b/tools/windows/install-restart-esp-usb-task.ps1 @@ -160,8 +160,11 @@ Write-Output " $keep (SYSTEM/Administrators only -- keeps the folder # ------------------------------------------------------------------- the task +# -LogPath is passed explicitly as well as defaulted inside the script: the +# first install (2026-09-21) registered a task whose script died in its own +# param block under -File, and said nothing. Belt and braces. $action = New-ScheduledTaskAction -Execute 'powershell.exe' -Argument ( - '-NoProfile -NonInteractive -ExecutionPolicy Bypass -WindowStyle Hidden -File "{0}"' -f $target) + '-NoProfile -NonInteractive -ExecutionPolicy Bypass -WindowStyle Hidden -File "{0}" -LogPath "{1}"' -f $target, $logPath) $taskPrincipal = New-ScheduledTaskPrincipal -UserId 'SYSTEM' -LogonType ServiceAccount -RunLevel Highest $settings = New-ScheduledTaskSettingsSet -AllowStartIfOnBatteries -DontStopIfGoingOnBatteries ` -MultipleInstances IgnoreNew -ExecutionTimeLimit (New-TimeSpan -Minutes 5) @@ -199,8 +202,11 @@ if ($registeredArgs -like '*Restart-PnpDevice*') { exit 1 } +# Run it the way the TASK runs it: -File, and NO -LogPath, so the script's own +# default is what gets exercised. Passing -LogPath here is what hid the +# param-block failure from the first install's self-check. & powershell.exe -NoProfile -NonInteractive -ExecutionPolicy Bypass -File $target ` - -InstanceId 'not-an-instance-id' -LogPath $logPath -DryRun | Out-Null + -InstanceId 'not-an-instance-id' -DryRun | Out-Null if ($LASTEXITCODE -ne 3) { Write-Error "self-check failed: a malformed instance id should exit 3, got $LASTEXITCODE" exit 1 diff --git a/tools/windows/restart-esp-usb.ps1 b/tools/windows/restart-esp-usb.ps1 index 91cd56b..5eba78d 100644 --- a/tools/windows/restart-esp-usb.ps1 +++ b/tools/windows/restart-esp-usb.ps1 @@ -58,7 +58,13 @@ param( # Appended to, never truncated. Lives in the admin-only directory so the # account that writes requests cannot rewrite the record of them. - [string]$LogPath = (Join-Path $PSScriptRoot 'restart-esp-usb.log'), + # Resolved below, NOT here: under `powershell.exe -File` -- which is how the + # scheduled task runs this -- $PSScriptRoot is still empty while the param + # block's defaults are evaluated (Windows PowerShell 5.1), so a default of + # `Join-Path $PSScriptRoot ...` throws before the first log line and the + # task exits 1 having done nothing. That is exactly what the first install + # did on 2026-09-21; every test had passed -LogPath explicitly. + [string]$LogPath = '', # Validate, resolve and report, but do not touch the device. Everything up # to the restart is read-only, so this is the part that can be tested by an @@ -69,6 +75,15 @@ param( Set-StrictMode -Version Latest $ErrorActionPreference = 'Stop' +if (-not $LogPath) { + # In the script body $PSScriptRoot is populated under -File as well as + # under `& script`; the fallbacks are for a host that sets neither. + $here = $PSScriptRoot + if (-not $here -and $PSCommandPath) { $here = Split-Path -Parent $PSCommandPath } + if (-not $here) { $here = Split-Path -Parent $MyInvocation.MyCommand.Path } + $LogPath = Join-Path $here 'restart-esp-usb.log' +} + # Espressif's vendor ID, a 4-hex-digit product id, and a serial of the # characters Windows actually puts there. Anchored both ends, so a trailing # `; calc` or a second path component cannot ride along. The `\\` after the From 902da99d7bcfbe663497745ba6c10645cff28cd9 Mon Sep 17 00:00:00 2001 From: Brad Barnett <127794626+bdbarnett@users.noreply.github.com> Date: Mon, 21 Sep 2026 14:43:37 -0500 Subject: [PATCH 4/6] restart-esp-usb: never leave a device disabled, and enable one that is The Disable+Enable fallback ran when pnputil /restart-device said 'the device is not connected' (exit 1167) -- a board that had gone into ROM download mode. Disable took; Enable failed with 'Generic failure'; and when the board came back it was CM_PROB_DISABLED: enumerated, no COM port, and a later /restart-device 'succeeded' on it without changing anything. It took an administrator to enable it by hand (2026-09-21, a LilyGO T-Embed S3). The fallback is for a Windows with no /restart-device verb, so it only runs there now; if it disables and cannot enable, it enables again on the way out; and every run first enables a node it finds disabled. --- tools/windows/restart-esp-usb.ps1 | 53 ++++++++++++++++++++++++++----- 1 file changed, 45 insertions(+), 8 deletions(-) diff --git a/tools/windows/restart-esp-usb.ps1 b/tools/windows/restart-esp-usb.ps1 index 5eba78d..4ced554 100644 --- a/tools/windows/restart-esp-usb.ps1 +++ b/tools/windows/restart-esp-usb.ps1 @@ -262,6 +262,27 @@ if (-not $principal.IsInRole([Security.Principal.WindowsBuiltInRole]::Administra } $restarted = $false + +# A node this script (or anything else) left DISABLED cannot be restarted back +# to life: pnputil /restart-device reports success and the device stays +# CM_PROB_DISABLED, with no COM port. That happened on 2026-09-21 -- an earlier +# version of the fallback below disabled a board's node and then failed to +# enable it again, and the board was unreachable until an administrator +# enabled it by hand. So: enable first if it is disabled, and never leave it +# disabled on the way out. +function Enable-IfDisabled { + param([string]$Id) + $now = Get-PnpDevice -InstanceId $Id -ErrorAction SilentlyContinue + if ($now -and "$($now.Problem)" -eq 'CM_PROB_DISABLED') { + Write-Log 'the node is DISABLED -- enabling it.' + $e = & pnputil.exe @('/enable-device', $Id) 2>&1 + Write-Log ('pnputil /enable-device (exit {0}): {1}' -f $LASTEXITCODE, (($e | Where-Object { "$_" -match '\S' }) -join '; ')) + return $true + } + return $false +} +[void](Enable-IfDisabled $InstanceId) + # An argument array, never a command string: the id reached this line as data # and it stays data. $pnputilArgs = @('/restart-device', $InstanceId) @@ -272,14 +293,30 @@ if ($LASTEXITCODE -eq 0 -and $text -notmatch 'Failed to restart') { $restarted = $true } else { Write-Log ('pnputil /restart-device failed (exit {0}): {1}' -f $LASTEXITCODE, $text) - try { - Disable-PnpDevice -InstanceId $InstanceId -Confirm:$false -ErrorAction Stop - Start-Sleep -Milliseconds 700 - Enable-PnpDevice -InstanceId $InstanceId -Confirm:$false -ErrorAction Stop - Write-Log 'disable/enable fallback ok' - $restarted = $true - } catch { - Write-Log ('disable/enable fallback failed: {0}' -f $_.Exception.Message) + # The Disable+Enable fallback is for a Windows older than 10 2004, which + # has no `pnputil /restart-device` at all. It is NOT a second attempt when + # pnputil exists and said no -- exit 1167 "the device is not connected" + # is a board that has gone away, and disabling a node that is not there + # leaves it disabled for whenever the board comes back. + $hasRestartVerb = ((& pnputil.exe '/?' 2>&1) -join ' ') -match '/restart-device' + if (-not $hasRestartVerb) { + $disabled = $false + try { + Disable-PnpDevice -InstanceId $InstanceId -Confirm:$false -ErrorAction Stop + $disabled = $true + Start-Sleep -Milliseconds 700 + Enable-PnpDevice -InstanceId $InstanceId -Confirm:$false -ErrorAction Stop + $disabled = $false + Write-Log 'disable/enable fallback ok' + $restarted = $true + } catch { + Write-Log ('disable/enable fallback failed: {0}' -f $_.Exception.Message) + } finally { + if ($disabled) { + Write-Log 'the fallback left the node disabled -- enabling it again before leaving.' + & pnputil.exe @('/enable-device', $InstanceId) 2>&1 | Out-Null + } + } } } From 39a8b6f4476d3d8b96ae14b56a42f992f5963f00 Mon Sep 17 00:00:00 2001 From: Brad Barnett <127794626+bdbarnett@users.noreply.github.com> Date: Mon, 21 Sep 2026 15:10:11 -0500 Subject: [PATCH 5/6] restart-esp-usb: tests that run the real script over pretend devices, and the guide says what the hardware has shown The never-leave-a-node-disabled fix went in with no test. PowerShell resolves a function before a cmdlet or an executable, so esp_usb_doubles.ps1 defines Get-PnpDevice, pnputil.exe and the PnpDevice cmdlets, records each call, and dot-sources the real script. Four scenarios; each of the three rules was watched going red against a copy with one line changed. The guide's 'not yet proved on hardware' paragraph is replaced by what has been: the two proofs through the task, the first real rescue, what the task cannot do, and how the first installed version left a node disabled. --- CHANGELOG.md | 8 ++- cli/tests/esp_usb_doubles.ps1 | 87 +++++++++++++++++++++++++++++++ cli/tests/test_esp_usb_restart.py | 67 ++++++++++++++++++++++++ docs/agent-guide.md | 34 +++++++++--- 4 files changed, 186 insertions(+), 10 deletions(-) create mode 100644 cli/tests/esp_usb_doubles.ps1 diff --git a/CHANGELOG.md b/CHANGELOG.md index 0bad13e..5b7b598 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,8 +9,12 @@ `tools/windows/restart-esp-usb.ps1` is now what the task runs: one device per run named by instance id (the old `USB\VID_303A*` sweep would have bounced every Espressif board on the bench together), `pnputil /restart-device` as the - verb with Disable+Enable as the fallback, a transcript, and exit codes that - separate "no such device" from "the restart failed". + verb, a transcript, and exit codes that separate "no such device" from "the + restart failed". Disable+Enable is the fallback only where `pnputil` has no + restart verb: the first installed version tried it after a 1167 ("the device + is not connected"), the Disable stuck, and a board came back with its USB node + disabled. The script now enables a disabled node before anything else and + never leaves one disabled on the way out. - Add `mpftp usb-restart` — `--status` to ask whether the recovery is really installed before planning around it, `--list` to find instance ids rather than hard-coding them, `--instance` to drive it. `--status` inspects the action the diff --git a/cli/tests/esp_usb_doubles.ps1 b/cli/tests/esp_usb_doubles.ps1 new file mode 100644 index 0000000..d03ef2c --- /dev/null +++ b/cli/tests/esp_usb_doubles.ps1 @@ -0,0 +1,87 @@ +# Test doubles for tools/windows/restart-esp-usb.ps1 (mpftp#31). +# +# The part of that script which touches a device cannot be exercised for real +# by an ordinary account, and should not be exercised for real by a test at +# all. But what it does to a device is a handful of calls -- Get-PnpDevice, +# pnputil.exe, Disable-/Enable-PnpDevice -- and PowerShell resolves a function +# before a cmdlet or an executable of the same name. So this file defines +# those names, records every call, and dot-sources the REAL script underneath +# them. The script's own text runs; only the hardware is pretend. +# +# Scenarios: +# healthy a present node, pnputil restarts it +# disabled the node is CM_PROB_DISABLED when the script finds it +# gone-away pnputil has /restart-device and answers 1167 "not connected" +# old-windows pnputil has no /restart-device; Disable works, Enable throws +param( + [Parameter(Mandatory = $true)][string]$Script, + [Parameter(Mandatory = $true)][string]$Scenario, + [Parameter(Mandatory = $true)][string]$Calls, + [Parameter(Mandatory = $true)][string]$Log +) + +$script:DoubleCalls = $Calls +$script:DoubleScenario = $Scenario +$script:DoubleProblem = if ($Scenario -eq 'disabled') { 'CM_PROB_DISABLED' } else { 'CM_PROB_NONE' } + +function Note-Call { + param([string]$What) + Add-Content -LiteralPath $script:DoubleCalls -Value $What -Encoding UTF8 +} + +function Get-PnpDevice { + [pscustomobject]@{ + FriendlyName = 'a test double' + Status = if ($script:DoubleProblem -eq 'CM_PROB_NONE') { 'OK' } else { 'Error' } + Problem = $script:DoubleProblem + } +} + +function Disable-PnpDevice { + Note-Call 'Disable-PnpDevice' + $script:DoubleProblem = 'CM_PROB_DISABLED' +} + +function Enable-PnpDevice { + Note-Call 'Enable-PnpDevice' + if ($script:DoubleScenario -eq 'old-windows') { + throw 'The device is not connected.' + } + $script:DoubleProblem = 'CM_PROB_NONE' +} + +function Start-Sleep { } + +function pnputil.exe { + # `& pnputil.exe @('/verb', $id)` hands an executable two arguments and a + # function one array, so flatten before reading the verb. + $flat = @($args | ForEach-Object { $_ }) + $verb = "$($flat[0])" + Note-Call ('pnputil ' + $verb) + $global:LASTEXITCODE = 0 + switch ($verb) { + '/?' { + if ($script:DoubleScenario -eq 'old-windows') { + 'PNPUTIL /add-driver /delete-driver /enum-devices /enable-device /disable-device' + } else { + 'PNPUTIL /add-driver /delete-driver /enum-devices /enable-device /disable-device /restart-device' + } + } + '/enable-device' { + $script:DoubleProblem = 'CM_PROB_NONE' + 'Device enabled successfully.' + } + '/restart-device' { + if ($script:DoubleScenario -eq 'gone-away' -or $script:DoubleScenario -eq 'old-windows') { + $global:LASTEXITCODE = 1167 + 'Failed to restart device. The device is not connected.' + } else { + 'Device restarted successfully.' + } + } + } +} + +. $Script -InstanceId 'USB\VID_303A&PID_4003\D0UB1E0000000000' -LogPath $Log +# `exit` in a dot-sourced script returns here, with its code in $LASTEXITCODE. +exit $LASTEXITCODE diff --git a/cli/tests/test_esp_usb_restart.py b/cli/tests/test_esp_usb_restart.py index 3b719c5..fde6ecc 100644 --- a/cli/tests/test_esp_usb_restart.py +++ b/cli/tests/test_esp_usb_restart.py @@ -377,5 +377,72 @@ def test_it_writes_its_transcript_beside_itself_by_default(self): self.assertIn("request refused", log.read_text(encoding="utf-8", errors="replace")) +@unittest.skipIf(_powershell() is None, "needs Windows PowerShell (interop from WSL)") +class ItNeverLeavesANodeDisabled(_StagedScript, unittest.TestCase): + """The restart itself, run for real over pretend hardware. + + On 2026-09-21 the first installed version disabled a board's USB node and + then failed to enable it; the board enumerated with no COM port until an + administrator ran `pnputil /enable-device`. `esp_usb_doubles.ps1` defines + Get-PnpDevice, pnputil.exe and the two PnpDevice cmdlets as functions -- + which PowerShell resolves first -- records every call, and dot-sources the + real script under them. + + Shown failing, 2026-09-21, against copies of the script with one line + changed each (MPFTP_ESP_USB_SCRIPT): + + the `[void](Enable-IfDisabled ...)` call removed 1 red: the disabled node + `if (-not $hasRestartVerb)` -> `if ($true)` 1 red: the board that went away + the `finally` block's enable removed 1 red: the old-Windows fallback + """ + + DOUBLES = Path(__file__).resolve().parent / "esp_usb_doubles.ps1" + + @classmethod + def setUpClass(cls): + super().setUpClass() + shutil.copy(cls.DOUBLES, espusb._win_to_local(cls.work) / "esp_usb_doubles.ps1") + + def _play(self, scenario: str) -> tuple[int, list[str]]: + calls = espusb._win_to_local(self.work) / "doubles.calls" + calls.unlink(missing_ok=True) + proc = subprocess.run( + [_powershell(), "-NoProfile", "-NonInteractive", "-ExecutionPolicy", "Bypass", + "-File", self.work + r"\esp_usb_doubles.ps1", + "-Script", self.script, + "-Scenario", scenario, + "-Calls", self.work + r"\doubles.calls", + "-Log", self.work + r"\doubles.log"], + capture_output=True, text=True, + ) + seen = calls.read_text(encoding="utf-8-sig").split("\n") if calls.exists() else [] + return proc.returncode, [line.strip() for line in seen if line.strip()] + + def test_a_healthy_node_is_restarted_and_nothing_else_is_done_to_it(self): + code, calls = self._play("healthy") + self.assertEqual(code, 0) + self.assertEqual(calls, ["pnputil /restart-device"]) + + def test_a_disabled_node_is_enabled_before_it_is_restarted(self): + code, calls = self._play("disabled") + self.assertEqual(code, 0) + self.assertEqual(calls, ["pnputil /enable-device", "pnputil /restart-device"]) + + def test_a_board_that_has_gone_away_is_never_disabled(self): + # pnputil has the verb and said 1167: the board left. Disabling a node + # that is not there is what stuck, so there must be no second attempt. + code, calls = self._play("gone-away") + self.assertEqual(code, 5) + self.assertNotIn("Disable-PnpDevice", calls) + self.assertNotIn("Enable-PnpDevice", calls) + + def test_the_old_windows_fallback_enables_again_when_its_own_enable_fails(self): + code, calls = self._play("old-windows") + self.assertEqual(code, 5) + self.assertIn("Disable-PnpDevice", calls) + self.assertEqual(calls[-1], "pnputil /enable-device", + "the node was disabled and the last thing done to it was not an enable") + + if __name__ == "__main__": unittest.main() diff --git a/docs/agent-guide.md b/docs/agent-guide.md index 699bc5a..b6cc3b9 100644 --- a/docs/agent-guide.md +++ b/docs/agent-guide.md @@ -475,14 +475,32 @@ board that really did not come back. Exit codes: 0 restarted, 2 no request, `tools/windows/restart-esp-usb.ps1` also runs by hand with `-InstanceId` from an elevated shell, and `-DryRun` does everything except touch the device. -**Not yet proved on hardware** (mpftp#31, 2026-09-21): the fix was written and -tested as far as an unprivileged session can go — the refusals, the request -handling and the absent-device case all pass against the real script, and both -rules were watched failing against a deliberately loosened copy. Three things -wait on that one elevated install: the task reaching `LastTaskResult 0`, COM12 -disappearing and coming back, and a deliberately wrong instance id failing -through the *task* rather than through a direct run. Run them the moment it is -installed. +**What it has done on hardware** (mpftp#31, 2026-09-21, a LilyGO T-Embed S3): +a deliberately wrong instance id sent through the *task* came back +`LastTaskResult 3`, and the board's real id came back `0` with COM12 dropping +and returning. Later the same day it made its first real rescue: after a +hard reset the port answered Windows error 31 ("a device attached to the +system is not functioning") — the node present and stuck — and one request +file plus `schtasks /run` had the board answering again, with nobody at the +bench. + +**What it cannot do:** bring back a board that is not on the bus. No node +means result 4, and that is a hand on a cable. Do not send it a request for a +board that is sitting in ROM download mode under a different PID, either — the +id you name is absent just then. + +That last case is how the first installed version hurt a board. Its fallback +for a Windows without `pnputil /restart-device` was Disable then Enable; run +against a node that had just gone away, the Disable stuck, the Enable failed +with 1167 ("the device is not connected"), and when the board came back +Windows kept it disabled — enumerated, no COM port, and only an administrator's +`pnputil /enable-device` would undo it. The script now uses that fallback only +where `pnputil` really has no restart verb, re-enables on the way out whatever +happened, and enables any disabled node it is asked about before it does +anything else. **That repair has not yet run on hardware** — the installed +copy only changes when the installer is run again. What stands in for it is +`cli/tests/esp_usb_doubles.ps1`, which runs the real script over pretend +devices and was watched going red for each of the three rules. ### Ctrl-C is not an interrupt inside `atexit` From ca14e46f3de7382738f178f7bcf0d6fc234a238a Mon Sep 17 00:00:00 2001 From: Brad Barnett <127794626+bdbarnett@users.noreply.github.com> Date: Mon, 21 Sep 2026 15:12:58 -0500 Subject: [PATCH 6/6] test_wslenv: combine the nested with-statements ruff rejects (SIM117) main has been red on lint since 79a3ee3; these five are the whole of it. --- cli/tests/test_wslenv.py | 40 +++++++++++++++++++++++++--------------- 1 file changed, 25 insertions(+), 15 deletions(-) diff --git a/cli/tests/test_wslenv.py b/cli/tests/test_wslenv.py index 83932b2..390d5bd 100644 --- a/cli/tests/test_wslenv.py +++ b/cli/tests/test_wslenv.py @@ -135,19 +135,25 @@ class StaleWslInteropTests(unittest.TestCase): """ def test_a_live_socket_is_left_alone(self): - with mock.patch.dict(os.environ, {"WSL_INTEROP": "/run/WSL/99_interop"}, clear=True): - with mock.patch("os.path.exists", return_value=True): - self.assertIsNone(_live_wsl_interop()) + with ( + mock.patch.dict(os.environ, {"WSL_INTEROP": "/run/WSL/99_interop"}, clear=True), + mock.patch("os.path.exists", return_value=True), + ): + self.assertIsNone(_live_wsl_interop()) def test_a_dead_socket_falls_back_to_inits(self): - with mock.patch.dict(os.environ, {"WSL_INTEROP": "/run/WSL/99_interop"}, clear=True): - with mock.patch("os.path.exists", lambda p: p == _WSL_INTEROP_FALLBACK): - self.assertEqual(_live_wsl_interop(), _WSL_INTEROP_FALLBACK) + with ( + mock.patch.dict(os.environ, {"WSL_INTEROP": "/run/WSL/99_interop"}, clear=True), + mock.patch("os.path.exists", lambda p: p == _WSL_INTEROP_FALLBACK), + ): + self.assertEqual(_live_wsl_interop(), _WSL_INTEROP_FALLBACK) def test_no_fallback_when_init_socket_is_absent_too(self): - with mock.patch.dict(os.environ, {"WSL_INTEROP": "/run/WSL/99_interop"}, clear=True): - with mock.patch("os.path.exists", return_value=False): - self.assertIsNone(_live_wsl_interop()) + with ( + mock.patch.dict(os.environ, {"WSL_INTEROP": "/run/WSL/99_interop"}, clear=True), + mock.patch("os.path.exists", return_value=False), + ): + self.assertIsNone(_live_wsl_interop()) def test_nothing_to_do_off_wsl(self): with mock.patch.dict(os.environ, {}, clear=True): @@ -155,17 +161,21 @@ def test_nothing_to_do_off_wsl(self): def test_spawn_env_substitutes_the_live_socket(self): env = {"WSL_DISTRO_NAME": "Ubuntu", "WSL_INTEROP": "/run/WSL/99_interop"} - with mock.patch.dict(os.environ, env, clear=True): - with mock.patch("os.path.exists", lambda p: p == _WSL_INTEROP_FALLBACK): - spawned = _wslenv_forwarded_env("python.exe") + with ( + mock.patch.dict(os.environ, env, clear=True), + mock.patch("os.path.exists", lambda p: p == _WSL_INTEROP_FALLBACK), + ): + spawned = _wslenv_forwarded_env("python.exe") self.assertIsNotNone(spawned) self.assertEqual(spawned["WSL_INTEROP"], _WSL_INTEROP_FALLBACK) def test_spawn_env_still_returns_none_when_everything_is_healthy(self): env = {"WSL_DISTRO_NAME": "Ubuntu", "WSL_INTEROP": "/run/WSL/99_interop"} - with mock.patch.dict(os.environ, env, clear=True): - with mock.patch("os.path.exists", return_value=True): - self.assertIsNone(_wslenv_forwarded_env("python.exe")) + with ( + mock.patch.dict(os.environ, env, clear=True), + mock.patch("os.path.exists", return_value=True), + ): + self.assertIsNone(_wslenv_forwarded_env("python.exe")) class SidecarDiedMessageTests(unittest.TestCase):