Batch 20260917: security hardening, safe legacy migration, diagnostics, installer lifecycle

BRANCH_MIGRATION_REPORT.md

@@ -0,0 +1,69 @@
+# Legacy branch migration review
+
+Reviewed against **origin/main `d7280791dc2b0b6b08e3e7dae1f1230813aaa362`** on 2026-09-17. Working branch: `hermes/pc-remote-project-batch-20260917`. No merges, cherry-picks, branch switches, commits, stash operations or pushes were performed in this review.
+
+## Method and accounting
+
+Read README, `packaging/README.md`, `docs/testing.md`, `.github/workflows/release.yml`, current implementation, and individual legacy diffs. For every remote topic ref, ran `git merge-base --is-ancestor <ref> origin/main`, `git log origin/main..<ref>`, `git cherry origin/main <ref>`, and inspected relevant file diffs/commits. All comparisons below are against the pinned main, not concurrently changing batch work.
+
+There are **12 topic refs**, not 11: nine `agent/*` and three `fix/*`. Five names point at the same already-contained commit. The remaining seven tips have 12 distinct out-of-main commits after deduplicating shared installer commits. `git cherry` marks every one `+`: **none is patch-equivalent to a main commit**. That does not mean every change is valuable: main implements some equivalent functionality differently.
+
+## Branch disposition
+
+| Remote branch (prefix `origin/`) | Tip | Main relationship / individual work | Decision |
+|---|---|---|---|
+| `agent/windows-installer` | `7b493f4` | `+ 2ee77ae` Inno template; `+ 7b493f4` build helper | **Superseded in purpose, not patch-equivalent.** Keep main's installer and release workflow. Do not introduce the alternate product identity or duplicate helper. |
+| `agent/installer-firewall` | `9bdebd2` | Shared two installer commits; `+ 9bdebd2` admin install and netsh rules | **Reject machine-scope behavior.** Contradicts per-user/no-elevation contract; see firewall analysis below. |
+| `agent/installer-autostart` | `69964a1` | Shared two installer commits; `+ b9b2b04` unconditional HKCU Run registration; `+ 69964a1` stacks firewall/admin behavior | **Reject wholesale.** Do not silently enable autostart or import admin firewall logic. Retain the idea of uninstall cleanup only, adapted with ownership checks. |
+| `agent/registry-autostart` | `7dfb81b` | `+ 7dfb81b` replaces Startup `.cmd` implementation with HKCU Run | **Defer backend migration.** Not a fix required for current installer. Requires coordinated API/status/legacy conflict policy and behavioral tests. |
+| `agent/windows-installer-docs` | `8717e1f` | Ancestor; zero unique commits; empty `git cherry` | **Already contained.** Ref name does not establish that installer work exists here. |
+| `agent/windows-installer-final` | `8717e1f` | Ancestor; zero unique commits; empty `git cherry` | **Already contained.** |
+| `agent/windows-installer-impl` | `8717e1f` | Ancestor; zero unique commits; empty `git cherry` | **Already contained.** |
+| `agent/windows-installer-pr` | `8717e1f` | Ancestor; zero unique commits; empty `git cherry` | **Already contained.** |
+| `agent/windows-installer-work` | `8717e1f` | Ancestor; zero unique commits; empty `git cherry` | **Already contained.** |
+| `fix/preserve-legacy-settings` | `eaf8fc5` | `+ bdf5a0f` avoid deleting source when target exists; `+ eaf8fc5` regression | **Carry behavior, not whole branch**, through migration owner. Confirmed defect on main in `auth._migrate_from_source`; also review atomicity/error handling. |
+| `fix/preserve-legacy-autostart` | `32861dd` | `+ 06f509b` retain legacy when target exists; `+ 32861dd` regression | **Carry behavior**, through migration owner. Main deletes legacy on name collision even without verifying replacement. Installer review does not edit `autostart.py`. |
+| `fix/issue-9-network-settings-migration` | `2ded08b` | `+ 3ac15ac` preserve legacy when target exists; `+ 2ded08b` regression | **Carry behavior**, through migration owner. `net_utils` has the same source-deletion bug; network settings share `settings.json`, not a separate network file. |
+
+The five `8717e1f` refs actually point to “1.4.1 fix session sync, power actions, pinned storage, launch args”; its changed files are apps/main/tests/web, not an unmerged installer implementation.
+
+## Why main wins for packaging
+
+Main's `f08ae47` introduced the current installer/release flow. Subsequent commits add testable manual packaging (`9e94816`), unattended install/ARP checks and distribution metadata (`c1cf623`), reliable exit-code capture (`019bbc7`), correct ARP display-name check (`ee1bf96`), and approved WinGet schema (`d728079`). The build helper proposed in `7b493f4` is unnecessary for this workflow.
+
+Keep these contracts:
+
+- Main AppId `{8A5675B9-1F20-4E2B-8BB4-D0C42973F9E4}` and corresponding `_is1` ARP key. The experimental installer used **different** AppId `{B0D5BE86-E060-4F59-AE9C-78C0E67B4A9D}`. Replacing main's identity breaks upgrade/metadata continuity. Installing an experimental package is not evidence of upgrading the supported package.
+- `PrivilegesRequired=lowest`, default `%LOCALAPPDATA%\Programs\PC Remote`, no permission override, current-user shortcuts.
+- Version supplied with `/DAppVersion=...`, versioned filenames under `release`, main publisher/URL/icon metadata. Old helper hard-codes version `1.4.1` and writes `dist/installer/PC-Remote-Setup-x64.exe`.
+- Silent installs do not launch the app; autostart is a user opt-in managed by the app, not a checked-by-default installer task.
+- Preserve `%APPDATA%\PC Remote` and legacy settings directories on install, upgrade and uninstall.
+
+## Firewall and service boundary
+
+`9bdebd2` switches to `{autopf}` / administrator privileges and runs shell-wrapped netsh delete/add commands for `PC Remote LAN`. `69964a1` brings that behavior into the autostart branch. The rules apply on private/domain profiles; they are machine rules, not current-user rules. Deleting by a display name alone cannot establish ownership and can remove an unrelated/pre-existing rule. The installer also does not establish robust command-success verification.
+
+**Do not port those commands.** Main neither installs a service nor creates a firewall rule, so the supported lifecycle should leave machine configuration unchanged. The smoke matrix compares service definitions and firewall rule/application-filter inventories before/after. It never adds/removes a service or firewall rule itself.
+
+This does **not** promise cleanup of experimental admin packages, manually created rules, or rules Windows may create after a person approves a firewall prompt. Removing such state requires a separately authorized, ownership-aware administrative procedure. Do not compensate by elevating the user installer or deleting all rules with matching names.
+
+## Confirmed installer gap and narrowly adapted fix
+
+On pinned main, `autostart.install()` creates an opt-in Startup file at runtime, outside Inno's tracked files. The installer has no uninstall event, uninstall delete section or application cleanup invocation. Consequently uninstall can leave an entry pointing to a deleted EXE. This is a confirmed source-level gap, not a claim based on executing an installer here.
+
+`installer/PC-Remote.iss` now removes only:
+
+- `PC Remote.cmd` / `PC-Android.cmd` whose complete UTF-8 contents match the app-generated command targeting **this** `{app}\PC Remote.exe`;
+- the `PC Remote` HKCU Run value, only when it exactly equals the quoted EXE path of this installation (compatibility with the experimental Run-key installer).
+
+Startup comparison accepts the generated CRLF form and the historical CRCRLF form: Windows `Path.write_text` with default newline conversion doubles an explicitly supplied CRLF. A temporary-file Python check confirmed `b'@echo off\r\r\n'` on the review host. No real Startup file was written.
+
+Unrelated/portable/source/customized entries are preserved; no wildcard deletion, current-user settings removal, runtime app launch during uninstall, forced opt-in, service action or firewall action was added. Failed owned-entry deletion is logged; VM residue assertions must still pass before lifecycle sign-off. Exact ownership matching intentionally prefers leaving an unrecognized entry over deleting someone else's.
+
+## Verification and remaining gates
+
+See [installer smoke matrix](docs/installer-smoke.md). Static contract tests first failed on the missing uninstall event, then on absent historical-newline handling, and passed after the minimal changes. PowerShell harness tests exercise **plan-only** output and denial when either explicit consent gate is absent. They do not execute the lifecycle.
+
+The Inno compiler is not installed (not on PATH; neither standard Inno Setup 6 location exists), and `dist/PC Remote/PC Remote.exe` is absent. **Compilation, real upgrade, login autostart, and Windows uninstall residue are unverified.** No setup/uninstaller, service/firewall change or real autostart operation was run on this workstation. Do not label static or gate tests as real Windows lifecycle evidence.
+
+Migration implementation belongs to the parallel migration task; its final changes and test results must be reviewed separately. No bulk import is recommended for any remaining branch.

SECURITY.md

@@ -29,6 +29,51 @@ Useful information for a private report includes:
 
 PC Remote is designed for trusted local-network use. The default transport is plain HTTP and the application should not be exposed directly to the public internet.
 
+## Runtime boundaries and residual risks
+
+- Both servers bind all interfaces by default. Use only a trusted private LAN;
+  guest Wi-Fi, port forwarding and direct internet exposure are unsupported.
+  Plain HTTP does not protect PINs or session tokens from network observers.
+- A paired client has the current Windows user's application-launch, window,
+  media and power privileges. This is not a sandbox or a multi-user access-control
+  system. Executable extension checks and `shell=False` are not an allowlist.
+  Do not run PC Remote elevated; do not give untrusted people the PIN/QR token.
+- PIN hashes are salted PBKDF2; tokens are random, short-lived and memory-only.
+  A four-digit PIN still has low entropy. `/login` has a per-peer attempt budget;
+  legacy action PINs and PIN changes share a separate global attempt budget.
+  A malicious peer can temporarily exhaust that legacy budget; valid session
+  tokens are not blocked by it. This is not a distributed-attack defence.
+- API requests carrying a browser Origin must match the explicit local-origin
+  list. CORS alone is not authorization. Requests without Origin remain supported
+  for the local tray/native clients and must satisfy endpoint authorization.
+  Pairing-token issuance is loopback-only; an unauthenticated restart is accepted
+  only from loopback, while an authenticated remote restart remains supported.
+  Local malware is outside this trust boundary. Uvicorn forwarded-client headers
+  are disabled deliberately.
+- Settings and logs are in user-writable AppData. They are not protected from
+  another process running as the same Windows user. Diagnostics emits categories,
+  never settings contents or raw log/error messages.
+
+## Installation and updates
+
+There is no in-app download-and-execute updater. Obtain releases from the project's
+GitHub release page, and verify the published SHA-256 checksums before installing.
+Checksums hosted with a release detect corruption, not compromise of the publisher.
+The current distribution has no guaranteed Authenticode signing trust chain.
+Installer user scope is intentional: no service or elevated firewall provisioning
+is required. Do not import old elevated installer branches wholesale.
+
+`build_release.py` preserves settings by default. `--reset-settings` and clean
+restart explicitly delete settings; they are not upgrade/migration mechanisms.
+Keep backups before intentional resets. An actual clean-install/upgrade/uninstall
+claim requires the disposable-Windows smoke matrix, not just Python tests.
+
+## Review evidence
+
+See `docs/SECURITY_REVIEW.md`, `docs/diagnostics.md` and
+`BRANCH_MIGRATION_REPORT.md`. Automated defensive tests do not constitute a
+penetration test or guarantee that all vulnerabilities have been found.
+
 ## Disclosure
 
 Please allow reasonable time to investigate and prepare a fix before publishing exploit details.

auth.py

@@ -56,42 +56,26 @@ def _legacy_appdata_settings_paths() -> list[Path]:
 
 
 def _migrate_from_source(source: Path, target: Path) -> bool:
-    if not source.exists():
-        return False
-    try:
-        if source.resolve() == target.resolve():
-            return False
-    except Exception as exc:
-        logger.warning("Failed to resolve legacy settings path %s: %s", source, exc)
-
-    if target.exists():
-        try:
-            source.unlink()
-        except Exception as exc:
-            logger.warning("Failed to remove legacy settings file %s: %s", source, exc)
-        return True
-
-    target.parent.mkdir(parents=True, exist_ok=True)
-    try:
-        shutil.move(str(source), str(target))
-        return True
-    except Exception as exc:
-        logger.warning("Failed to move legacy settings %s -> %s: %s", source, target, exc)
-        try:
-            shutil.copy2(source, target)
-            source.unlink(missing_ok=True)
-            return True
-        except Exception as copy_exc:
-            logger.exception("Failed to migrate legacy settings %s -> %s: %s", source, target, copy_exc)
-            return False
+    """Unified safe migration (see migration.safe_migrate)."""
+    import migration
+    return migration.safe_migrate(source, target)
 
 
 def _migrate_legacy_settings(target: Path) -> None:
     candidates = _legacy_appdata_settings_paths()
     candidates.append(_legacy_runtime_settings_path())
     for source in candidates:
         if _migrate_from_source(source, target):
-            break
+            return
+    # Destination already authoritative: leave any legacy copy alone.
+    if target.exists():
+        return
+    leftover = [source for source in candidates if source.exists()]
+    if leftover:
+        raise SettingsError(
+            "Legacy settings exist but could not be migrated to "
+            f"{target}. The legacy files were not deleted."
+        )
 
 
 def settings_path() -> Path:

autostart.py

@@ -47,23 +47,28 @@ def _legacy_autostart_file() -> Path:
     return _startup_dir() / LEGACY_AUTOSTART_FILENAME
 
 
+def _startup_cmd_content() -> str:
+    return "@echo off\r\nstart \"\" /b " + _command_line() + "\r\n"
+
+
+def _write_cmd_file(target: Path) -> None:
+    # Newline="" preserves the exact CRLF bytes we write.
+    with open(target, "w", encoding="utf-8", newline="") as handle:
+        handle.write(_startup_cmd_content())
+
+
 def _migrate_legacy_autostart() -> None:
     legacy = _legacy_autostart_file()
     target = _startup_dir() / AUTOSTART_FILENAME
 
     if not legacy.exists() or legacy == target:
         return
     if target.exists():
-        try:
-            legacy.unlink()
-        except Exception as exc:
-            logger.warning("Failed to remove legacy autostart file %s: %s", legacy, exc)
+        # Keep the legacy file as a user-visible fallback; never delete data.
         return
-
     target.parent.mkdir(parents=True, exist_ok=True)
     try:
-        content = f'@echo off\r\nstart "" /b {_command_line()}\r\n'
-        target.write_text(content, encoding="utf-8")
+        _write_cmd_file(target)
         legacy.unlink()
     except Exception as exc:
         logger.exception("Failed to migrate autostart file %s -> %s: %s", legacy, target, exc)
@@ -76,9 +81,7 @@ def is_enabled() -> bool:
 def install() -> Path:
     target = autostart_file()
     target.parent.mkdir(parents=True, exist_ok=True)
-    cmd = _command_line()
-    content = f'@echo off\r\nstart "" /b {cmd}\r\n'
-    target.write_text(content, encoding="utf-8")
+    _write_cmd_file(target)
     return target
 
 

diagnostics.py

@@ -0,0 +1,93 @@
+"""Read-only diagnostics. Never initialize authentication or migrate settings.
+
+Secrets policy: this module must never echo PIN hashes, session tokens,
+settings.json contents or application log text. Errors are reported as
+categories, never as raw messages.
+"""
+from __future__ import annotations
+
+import json
+import os
+import subprocess
+from pathlib import Path
+
+import config
+
+_FIREWALL_PS = (
+    "$ErrorActionPreference='Stop'; "
+    "$profiles=@(Get-NetFirewallProfile | Where-Object Enabled -eq True); "
+    "$rules=@(Get-NetFirewallRule | Where-Object {"
+    "$_.DisplayName -like 'PC Remote*' -or $_.DisplayName -like 'PC-Android*'}); "
+    "@{enabled_profiles=$profiles.Count;app_rules=$rules.Count} | ConvertTo-Json -Compress"
+)
+
+
+def firewall_status() -> dict:
+    try:
+        result = subprocess.run(
+            ['powershell.exe', '-NoProfile', '-NonInteractive', '-Command', _FIREWALL_PS],
+            capture_output=True, text=True, timeout=10,
+        )
+    except (OSError, subprocess.TimeoutExpired):
+        return {'state': 'unknown', 'reason': 'query_failed'}
+    if result.returncode != 0:
+        return {'state': 'unknown', 'reason': 'query_failed'}
+    try:
+        data = json.loads(result.stdout or '{}')
+    except ValueError:
+        return {'state': 'unknown', 'reason': 'query_failed'}
+    if not isinstance(data, dict) or any(
+        type(data.get(key)) is not int or data[key] < 0
+        for key in ('enabled_profiles', 'app_rules')
+    ):
+        return {'state': 'unknown', 'reason': 'query_failed'}
+    return {
+        'state': 'observed',
+        'enabled_profiles': data['enabled_profiles'],
+        'app_rules': data['app_rules'],
+        'effective_reachability': 'not_tested',
+    }
+
+
+def _last_error_state() -> dict | None:
+    path = config.app_dir() / config.LOG_FILE_NAME
+    try:
+        with path.open('rb') as handle:
+            handle.seek(0, 2)
+            handle.seek(max(0, handle.tell() - 65536))
+            for line in reversed(handle.read().decode('utf-8', errors='replace').splitlines()):
+                if ' - ERROR - ' in line:
+                    return {'state': 'recorded', 'code': 'application_error',
+                            'detail': 'omitted_for_privacy'}
+    except OSError:
+        pass
+    return None
+
+
+def collect_diagnostics() -> dict:
+    path = config.app_dir() / config.SETTINGS_FILENAME
+    startup = Path(os.environ.get('APPDATA', '')) / 'Microsoft/Windows/Start Menu/Programs/Startup'
+    names = (config.APP_NAME, *config.LEGACY_APP_NAMES)
+    enabled = bool(os.environ.get('APPDATA')) and any(
+        (startup / f'{name}.cmd').is_file() for name in names
+    )
+    return {
+        'version': config.APP_VERSION,
+        'bind': {
+            'api': {'address': config.API_HOST, 'port': config.API_PORT},
+            'web': {'address': config.WEB_HOST, 'port': config.WEB_PORT},
+        },
+        'autostart': {'state': 'enabled' if enabled else 'disabled'},
+        'firewall': firewall_status(),
+        'config': {'path': str(path), 'exists': path.is_file()},
+        'last_error': _last_error_state(),
+    }
+
+
+def main() -> int:
+    print(json.dumps(collect_diagnostics(), ensure_ascii=True, indent=2))
+    return 0
+
+
+if __name__ == '__main__':
+    raise SystemExit(main())