From caf5b790f7bd6a5e7a3e239bf8adf20436d2cc93 Mon Sep 17 00:00:00 2001 From: mimaxon1 Date: Thu, 17 Sep 2026 09:17:04 +0300 Subject: [PATCH 1/2] [verified] batch: safe legacy migration, security hardening, diagnostics, installer report/smoke --- BRANCH_MIGRATION_REPORT.md | 69 ++++++++++++ SECURITY.md | 45 ++++++++ auth.py | 31 +---- autostart.py | 23 ++-- diagnostics.py | 93 +++++++++++++++ docs/PROJECT_BATCH_CHECKLIST.md | 16 +++ docs/SECURITY_REVIEW.md | 33 ++++++ docs/diagnostics.md | 15 +++ docs/installer-smoke.md | 77 +++++++++++++ installer/PC-Remote.iss | 44 ++++++++ main.py | 45 +++++++- migration.py | 34 ++++++ net_utils.py | 31 +---- scripts/installer_smoke.ps1 | 187 +++++++++++++++++++++++++++++++ tests/conftest_helpers.py | 14 +++ tests/test_diagnostics.py | 60 ++++++++++ tests/test_installer_contract.py | 33 ++++++ tests/test_installer_smoke.py | 55 +++++++++ tests/test_migration.py | 113 +++++++++++++++++++ tests/test_security_contract.py | 61 ++++++++++ 20 files changed, 1011 insertions(+), 68 deletions(-) create mode 100644 BRANCH_MIGRATION_REPORT.md create mode 100644 diagnostics.py create mode 100644 docs/PROJECT_BATCH_CHECKLIST.md create mode 100644 docs/SECURITY_REVIEW.md create mode 100644 docs/diagnostics.md create mode 100644 docs/installer-smoke.md create mode 100644 migration.py create mode 100644 scripts/installer_smoke.ps1 create mode 100644 tests/conftest_helpers.py create mode 100644 tests/test_diagnostics.py create mode 100644 tests/test_installer_contract.py create mode 100644 tests/test_installer_smoke.py create mode 100644 tests/test_migration.py create mode 100644 tests/test_security_contract.py diff --git a/BRANCH_MIGRATION_REPORT.md b/BRANCH_MIGRATION_REPORT.md new file mode 100644 index 0000000..1e7223c --- /dev/null +++ b/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 origin/main`, `git log origin/main..`, `git cherry origin/main `, 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. diff --git a/SECURITY.md b/SECURITY.md index 1745754..7937fc8 100644 --- a/SECURITY.md +++ b/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. diff --git a/auth.py b/auth.py index 0dd347e..508bda4 100644 --- a/auth.py +++ b/auth.py @@ -56,34 +56,9 @@ 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: diff --git a/autostart.py b/autostart.py index 468bcee..b561f00 100644 --- a/autostart.py +++ b/autostart.py @@ -47,6 +47,16 @@ 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 @@ -54,16 +64,11 @@ def _migrate_legacy_autostart() -> None: 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 diff --git a/diagnostics.py b/diagnostics.py new file mode 100644 index 0000000..45f27a2 --- /dev/null +++ b/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()) diff --git a/docs/PROJECT_BATCH_CHECKLIST.md b/docs/PROJECT_BATCH_CHECKLIST.md new file mode 100644 index 0000000..47317e4 --- /dev/null +++ b/docs/PROJECT_BATCH_CHECKLIST.md @@ -0,0 +1,16 @@ +# PC Remote batch audit + +Base: origin/main `d728079`; branch `hermes/pc-remote-project-batch-20260917`. +Original checkout `pc-with-android` has uncommitted work and corrupt refs; untouched. Fresh clone used. README requires Python 3.13+; baseline on isolated 3.13.14 environment: 107 passed, 2 third-party deprecation warnings. No AGENTS or current ROADMAP tracked. + +## Initial checklist + +| Task | State | Evidence / next action | +|---|---|---| +| 1 Security | PARTIAL | SECURITY.md exists; trusted-LAN HTTP is intentional. Review authentication/command/install boundaries; remediate only confirmed defects. | +| 2 Migration | PARTIAL | auth.py and autostart.py contain separate legacy migration, swallowed errors and source deletion; network fix branch needs comparison. | +| 3 Branch report | TODO | 11 legacy installer/autostart/firewall/settings branches available; compare against current main, no bulk merges. | +| 4 Diagnostics | TODO | Only autostart-status CLI exists; add read-only JSON command with no secret values. | +| 5 Installation matrix | PARTIAL | Current release CI covers installation metadata; installer has no uninstall autostart cleanup. Add safe contracts and disposable-Windows smoke harness. | + +Real install/uninstall validation requires a disposable Windows user/VM: do not modify this workstation's actual autostart, firewall, services or user configuration. Mock/static tests are not OS lifecycle proof. diff --git a/docs/SECURITY_REVIEW.md b/docs/SECURITY_REVIEW.md new file mode 100644 index 0000000..a9f53ab --- /dev/null +++ b/docs/SECURITY_REVIEW.md @@ -0,0 +1,33 @@ +# Defensive security review + +Reviewed base: `d728079` (current main at clone time). Source review and safe unit contracts only; no active exploitation, remote commands, power actions or real installation changes. + +| Boundary | Source evidence on base | Classification / disposition | +|---|---|---| +| LAN transport | config.py API_HOST/WEB_HOST; README security model | Intentional all-interface HTTP. Document trusted-LAN-only restriction; no false claim of TLS or internet safety. | +| PIN authorization | main.py check() and change_password() call verify_password directly; only login() accounts failed attempts | Confirmed missing attempt budget on alternate PIN authorization paths. Shared legacy budget added with serialized admission/verify/accounting (lock), preserving PIN compatibility; token traffic unaffected. | +| Browser request execution | main.py CORSMiddleware; pair token issuance loopback-only; unauthenticated restart loopback-only, authenticated remote restart supported | Confirmed CORS-only browser boundary: CORS response restrictions do not reject all cross-origin execution. Explicit Origin rejection added before handlers. No-Origin native/tray clients still supported. | +| Peer identity | main.py run_api() leaves Uvicorn proxy_headers default; local-only access depends on request.client | Hardening: explicitly disable forwarded-header trust for this direct-LAN server. No configurable trusted reverse proxy is supported. | +| Settings migration | auth.py _migrate_from_source deletes source when destination exists and swallows copy errors | Confirmed data-integrity problem; addressed in migration workstream. | +| Autostart lifecycle | autostart.py writes per-user Startup CMD; installer lacks cleanup | Confirmed lifecycle gap; addressed in installer workstream, tested without changing this workstation. | +| Command launch | apps.py executable validation, start helpers, subprocess argv with shell=False; main.py launch endpoint authorization | By-design broad authenticated desktop control, not a sandbox. No claim that extension checks prevent a trusted client invoking an interpreter. Do not run elevated. | +| Restart | main.py _powershell_quote/_spawn_restart_watcher | Commands originate locally and strings use single-quote escaping. No remote command-string injection confirmed by this review. | +| Updates | installer AppUpdatesURL; build_release.py and release workflow | No runtime updater. Checksums and source-host trust documented; no signing guarantee. Reset requires explicit flag; retained unchanged. | +| Diagnostic privacy | new diagnostics.py | Output allowlist and bounded log scan; error category only. No PIN/hash/token/raw message export. Config path may identify OS account: review before sharing. | + +## Verification + +Baseline: 107 passed on Python 3.13.14; current full suite 130 passed. +Defensive contracts in `tests/test_security_contract.py` cover guard-before-hash, +failure accounting, success reset, token bypass, explicit Origin policy and +disabled proxy headers. Migration contracts in `tests/test_migration.py` cover +non-destructive legacy settings/autostart migration and exact CRLF bytes. +No working exploitation payload or vulnerability reproduction is included. + +## Remaining limitations + +Four-digit PIN entropy, plaintext transport, global legacy lockout availability, +trust of same-user local processes and broad authorized program launching remain +explicit design limitations. Origin policy is not authentication and is not a +network firewall. No pentest, VM install lifecycle, signature verification, or +remote reachability test has been performed by this review. diff --git a/docs/diagnostics.md b/docs/diagnostics.md new file mode 100644 index 0000000..a5a7879 --- /dev/null +++ b/docs/diagnostics.md @@ -0,0 +1,15 @@ +# Read-only diagnostics + +Run `python main.py --diagnostics` (or `python diagnostics.py`) from the source environment. The command prints JSON and exits without starting servers, importing the tray/audio stack, initializing authentication, creating logs, or migrating settings. It can also be used when setup has not completed. + +Fields: +- `version`: build version. +- `bind`: configured API/web addresses and ports, not evidence of live listeners. +- `autostart`: current/legacy per-user Startup CMD presence, not evidence of a successful login launch. +- `firewall`: read-only Windows `Get-NetFirewallProfile` / `Get-NetFirewallRule` summary. Rule count matches PC Remote / PC-Android display-name prefixes. It is not an effective permission or reachability check; other program/port/group-policy rules may apply. Failure/timeout is `unknown`, not `disabled`. +- `config`: canonical settings path and existence; no contents. +- `last_error`: category only if an ERROR entry is present in the bounded 64 KiB tail of the current log. Raw log messages are never emitted. Null means no error observed there, not proof of a healthy previous run. + +No PINs, hashes, tokens, application command lines, environment dumps or raw exception/PowerShell output are included. Review the user-name-bearing config path before sharing a report publicly. + +Verification: `python -m pytest tests/test_diagnostics.py`. These tests include a real child-process CLI invocation with temporary APPDATA and prove settings bytes unchanged and no log created. The firewall query has also been exercised read-only on Windows; actual remote reachability is deliberately not tested. diff --git a/docs/installer-smoke.md b/docs/installer-smoke.md new file mode 100644 index 0000000..7996d3a --- /dev/null +++ b/docs/installer-smoke.md @@ -0,0 +1,77 @@ +# Windows user-scope installer smoke matrix + +## Evidence level and safety + +**Current result: static contracts and PowerShell safety gates passed; real installer lifecycle NOT RUN.** A passing pytest suite does not prove Inno compilation, Windows login behavior, permissions, upgrade continuity or uninstall cleanup. + +Never run the lifecycle on a development workstation. Use a disposable Windows 10/11 x64 VM with a fresh standard-user account, snapshot, no existing PC Remote installation/settings/autostart, and no unrelated firewall/service activity. Reject elevation: an elevated install cannot establish the standard-user requirement. Revert the snapshot between matrix rows. Do not run legacy admin/firewall branch installers as part of this matrix. + +The repository's existing release workflow compiles Inno, installs silently, checks HKCU ARP name/version/publisher, and runs uninstall. It does **not** currently verify upgrade/settings/autostart/residual resources, and a `windows-latest` CI account is not proof of a non-admin Windows 10/11 user. The optional harness below is not wired into CI and must not be enabled implicitly. + +## Fast, safe verification + +From the repository root: + +```powershell +.venv\Scripts\python.exe -m pytest tests/test_installer_contract.py tests/test_installer_smoke.py -q +powershell.exe -NoProfile -NonInteractive -File scripts\installer_smoke.ps1 +``` + +The second command only prints JSON with `status: not_run`, `mode: plan-only` and the planned stages. It performs no installation, uninstallation, application CLI call, configuration write or machine inventory. Tests additionally confirm denial with missing consent gates. On platforms without PowerShell those subprocess tests skip; the Inno checks remain static text contracts. + +## Automated disposable-VM rows + +For **each Windows 10 and Windows 11 x64** standard-user VM, run these rows separately. In every row use a current-lineage previous Setup and a newly built Setup with the unchanged main AppId; supply the expected new ARP version. Using the same installer twice establishes repair/reinstall only, not an actual version upgrade. + +| Row | AutostartCase | Install / upgrade requirements | Uninstall requirements | Local evidence | +|---|---|---|---|---| +| A | `disabled` | Default per-user path, HKCU ARP only, Start Menu shortcut, no default desktop icon, no silent launch; startup remains disabled | EXE/ARP/Start Menu removed; settings byte-identical; no autostart; machine inventory unchanged | NOT RUN | +| B | `owned` | Run installed EXE `--install-autostart`; verify target path; seed matching legacy Startup and HKCU Run compatibility entries; upgrade preserves all choices | Generated current/legacy Startup and matching Run value removed, settings retained; no service/firewall changes | NOT RUN | +| C | `foreign` | Seed unrelated/custom Startup files and unrelated Run target; upgrade leaves them alone | Unrelated entries remain byte-identical, installed EXE/ARP/shortcut gone, settings retained; machine inventory unchanged | NOT RUN | + +The harness records installer exit codes, artifact SHA-256 hashes, OS version, completed stages, result JSON, Inno logs, and read-only before/after service definitions and firewall rule/application filters. It compares machine state without adding/deleting services/rules. An inventory permission failure is a blocker, not a clean result. Background OS rule changes may cause a conservative failure; inspect evidence rather than weakening assertions. + +Fixtures are opaque JSON preservation sentinels in current/legacy app-data. They test **installer byte preservation**, not validity of application settings, PIN authentication, or migration algorithms. Auth and network preferences share `settings.json`; an extra arbitrary JSON file tests that installer cleanup does not broadly delete app-data. The owned row calls the app CLI **before** replacing settings with sentinels, and never launches the server afterward. + +### Explicit execution (disposable VM ONLY) + +Build both artifacts using the documented PyInstaller/Inno release flow, or transfer trusted release artifacts into the VM and verify their hashes. The upgraded artifact must include the new uninstall event. Both installers must have the supported main AppId `{8A5675B9-1F20-4E2B-8BB4-D0C42973F9E4}`. The old experimental `{B0D5BE86-E060-4F59-AE9C-78C0E67B4A9D}` is a different installed product, not an upgrade fixture. + +In a **non-elevated** PowerShell session inside the disposable VM: + +```powershell +# Explicit destructive-lifecycle consent: NEVER set this on a real workstation. +$env:PC_REMOTE_DISPOSABLE_VM = '1' +powershell.exe -NoProfile -NonInteractive -File scripts\installer_smoke.ps1 ` + -Execute -Confirmation I_ACCEPT_DISPOSABLE_VM ` + -Setup 'C:\Artifacts\PC-Remote-OLD-windows-x64-Setup.exe' ` + -UpgradeSetup 'C:\Artifacts\PC-Remote-NEW-windows-x64-Setup.exe' ` + -ExpectedVersion 'NEW' -AutostartCase owned ` + -EvidenceDirectory 'C:\Users\TestUser\smoke-owned' +``` + +Replace placeholders with actual paths/version and select the row's AutostartCase. The evidence directory must not already exist. The two gates are an explicit human attestation, **not automatic VM detection**. The script refuses existing app/legacy settings, installation, shortcuts, related Startup entries or Run value; do not delete real data to bypass this guard. + +On failure, it writes `result.json` if execution reached the evidence phase, exits nonzero, and deliberately leaves state for diagnosis. Preflight errors exit before creating evidence. It never automatically retries uninstall, deletes settings/foreign fixtures, stops processes, or alters firewall/service state. Archive evidence and revert/discard the VM. Foreign entries and preserved settings intentionally remain after row C and all settings-preservation rows. + +## Required manual rows (not covered by harness) + +| Scenario | Procedure in disposable VM | Required observation / evidence | +|---|---|---| +| Interactive installer | Run wizard as standard user; default install and then repeat with desktop icon selected | No UAC request; default path current-user; correct shortcut creation/removal; capture wizard and ARP evidence | +| Login autostart | Install, opt in via tray/CLI, sign out/in; then disable and sign out/in | Exactly one app process/tray instance when enabled, none when disabled; correct quoted executable path, including a non-ASCII Windows profile | +| Real persisted preferences | Configure a test PIN/network/interface and other preferences through supported app UI; close; upgrade; reopen | Preferences still usable, no new PIN prompt; do not publish hashes or actual settings containing secrets | +| Settings retained on uninstall/reinstall | Uninstall after configuration, verify settings hashes privately, reinstall | Old preferences load; only install-owned files removed; settings deletion is separate explicit user action | +| App running during upgrade/uninstall | Start app, run upgrade/uninstall | Inno CloseApplications behavior and no remaining running app; do not infer this from silent closed-app rows | +| Custom path / Unicode | Install to a user-writable path containing spaces/non-ASCII, opt in, upgrade/uninstall | Startup quoting/UTF-8 comparison correct; unrelated portable installation preserved; log any conservative leftover | +| Read-only or locked Startup artifact | Make only VM test artifact undeletable, uninstall, inspect Inno log | Cleanup failure visible; do not claim no residue if deletion failed; revert VM afterward | +| Firewall prompt interaction | In an isolated trusted test LAN, explicitly approve/deny Windows' own prompt when launching app | Record OS-created rule ownership separately; user installer must not silently create/delete machine rules | +| Legacy admin experimental package | Separate explicitly approved migration investigation only | Not covered: differing AppId/machine rules need an administrative ownership-aware cleanup plan, never blanket name deletion | + +## Acceptance and blockers + +Release acceptance requires: compiled installer; all automated rows on supported Windows versions under standard users; required manual login/interactive/settings rows; archived actual logs and residual-state checks. Do not mark an unexecuted row passed because a static test checks the corresponding script text. + +At review time Inno Setup 6 was absent from PATH and both standard installation locations, and `dist/PC Remote/PC Remote.exe` was absent. No disposable-VM lifecycle was authorized/executed in this work. These block compilation and real runtime sign-off. The local suite's result covers only source contracts, PowerShell parsing/plan-only output and fail-closed gates. + +See [branch migration report](../BRANCH_MIGRATION_REPORT.md) for rejected legacy firewall/default-autostart changes and exact cleanup ownership policy. diff --git a/installer/PC-Remote.iss b/installer/PC-Remote.iss index b854ddf..4f23ac4 100644 --- a/installer/PC-Remote.iss +++ b/installer/PC-Remote.iss @@ -41,3 +41,47 @@ Name: "{autodesktop}\PC Remote"; Filename: "{app}\{#MyAppExeName}"; Tasks: deskt [Run] Filename: "{app}\{#MyAppExeName}"; Description: "Launch PC Remote"; Flags: nowait postinstall skipifsilent + +[Code] +// The application owns opt-in autostart. Do not enable it during installation. +// On uninstall, remove only entries targeting this installation, never another +// portable/source installation or a customized startup command. +procedure RemoveOwnedStartupFile(const Name: String); +var + Filename, Expected: String; + Content: AnsiString; + Owned: Boolean; +begin + Filename := ExpandConstant('{userstartup}\') + Name; + Expected := '@echo off' + #13#10 + 'start "" /b "' + + ExpandConstant('{app}\{#MyAppExeName}') + '"' + #13#10; + if LoadStringFromFile(Filename, Content) then + begin + Owned := Content = UTF8Encode(Expected); + // Python's Windows text writer historically doubled the carriage return. + StringChangeEx(Expected, #13#10, #13#13#10, True); + Owned := Owned or (Content = UTF8Encode(Expected)); + if Owned then + if not DeleteFile(Filename) then + Log('Could not remove owned startup file: ' + Filename); + end; +end; + +procedure CurUninstallStepChanged(CurUninstallStep: TUninstallStep); +var + Command, Expected: String; +begin + if CurUninstallStep = usUninstall then + begin + RemoveOwnedStartupFile('PC Remote.cmd'); + RemoveOwnedStartupFile('PC-Android.cmd'); + // Compatibility with the unmerged Run-key installer experiment. + Expected := '"' + ExpandConstant('{app}\{#MyAppExeName}') + '"'; + if RegQueryStringValue(HKCU, + 'Software\Microsoft\Windows\CurrentVersion\Run', 'PC Remote', Command) then + if Command = Expected then + if not RegDeleteValue(HKCU, + 'Software\Microsoft\Windows\CurrentVersion\Run', 'PC Remote') then + Log('Could not remove owned PC Remote Run value'); + end; +end; diff --git a/main.py b/main.py index 42d173f..ad821c2 100644 --- a/main.py +++ b/main.py @@ -1,4 +1,11 @@ """PC controller (FastAPI + web UI + tray GUI).""" +# Diagnostics must run before imports that initialize GUI, logging or settings. +if __name__ == "__main__": + import sys as _cli_sys + if "--diagnostics" in _cli_sys.argv[1:]: + from diagnostics import main as _diagnostics_main + raise SystemExit(_diagnostics_main()) + import argparse import ctypes import functools @@ -191,6 +198,19 @@ def _build_allowed_origins() -> list[str]: allowed_origins = _build_allowed_origins() + +def _browser_origin_allowed(origin: str | None) -> bool: + return origin is None or origin in allowed_origins + + +@app.middleware("http") +async def enforce_browser_origin(request: Request, call_next): + # CORS controls response visibility, not whether a request executes. + if not _browser_origin_allowed(request.headers.get("origin")): + return JSONResponse(status_code=403, content={"detail": "Origin not allowed"}) + return await call_next(request) + + app.add_middleware( CORSMiddleware, allow_origins=allowed_origins, @@ -647,6 +667,26 @@ async def unhandled_exception_handler(request: Request, exc: Exception) -> JSONR return JSONResponse(status_code=500, content={"detail": "Internal server error"}) +_LEGACY_PIN_LOCK = threading.Lock() + + +def _verify_legacy_pin(manager: auth.AuthManager, password: str) -> bool: + # Admission, verification and failure accounting are serialized so + # concurrent requests cannot verify past the shared budget. Token + # authorization does not use this path. + key = "legacy-pin-actions" + with _LEGACY_PIN_LOCK: + blocked_until = LOGIN_RATE_LIMITER.blocked_until(key) + if blocked_until: + raise HTTPException(status_code=429, detail="PIN attempts temporarily blocked") + if manager.verify_password(password): + LOGIN_RATE_LIMITER.reset(key) + return True + if LOGIN_RATE_LIMITER.record_failure(key): + raise HTTPException(status_code=429, detail="PIN attempts temporarily blocked") + return False + + def check(token: Optional[str], password: Optional[str]) -> None: """Authorize a request using either a session token or a PIN.""" manager = _auth_manager() @@ -654,7 +694,7 @@ def check(token: Optional[str], password: Optional[str]) -> None: raise HTTPException(status_code=409, detail="PIN setup required") if token and manager.verify_token(token): return - if password and manager.verify_password(password): + if password and _verify_legacy_pin(manager, password): return raise HTTPException(status_code=403, detail="Invalid PIN or expired session") # ----------------------------- @@ -757,7 +797,7 @@ def change_password(request: Request, data: ChangePasswordModel): manager = _auth_manager() if manager.requires_password_setup(): raise HTTPException(status_code=409, detail="Complete first-run setup via QR first") - if not manager.verify_password(data.current_password): + if not _verify_legacy_pin(manager, data.current_password): raise HTTPException(status_code=403, detail="Invalid current PIN") if data.token: if not manager.verify_token(data.token): @@ -1185,6 +1225,7 @@ def run_api(): ws="none", log_config=None, access_log=False, + proxy_headers=False, use_colors=False, log_level="info", ) diff --git a/migration.py b/migration.py new file mode 100644 index 0000000..1e85272 --- /dev/null +++ b/migration.py @@ -0,0 +1,34 @@ +"""Unified safe legacy migration. Sources are moved once and never deleted +when the destination already exists or the move fails.""" +from __future__ import annotations + +import logging +import shutil +from pathlib import Path + +logger = logging.getLogger("pc-remote.migration") + + +def safe_migrate(source: Path, target: Path) -> bool: + """Move a legacy file to target without destructive fallbacks. + + Returns True if a move happened, False otherwise. The source is never + deleted when the target exists or the move fails. + """ + if not source.exists(): + return False + try: + if source.resolve() == target.resolve(): + return False + except OSError as exc: + logger.warning("Failed to resolve migration path %s: %s", source, exc) + return False + if target.exists(): + return False + 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 migrate %s -> %s: %s", source, target, exc) + return False diff --git a/net_utils.py b/net_utils.py index 7fc19fa..a0bb57b 100644 --- a/net_utils.py +++ b/net_utils.py @@ -88,34 +88,9 @@ 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 network settings path %s: %s", source, exc) - - if target.exists(): - try: - source.unlink() - except Exception as exc: - logger.warning("Failed to remove legacy network 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 network 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 network 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: diff --git a/scripts/installer_smoke.ps1 b/scripts/installer_smoke.ps1 new file mode 100644 index 0000000..236250f --- /dev/null +++ b/scripts/installer_smoke.ps1 @@ -0,0 +1,187 @@ +# Windows lifecycle smoke test. Execution is ONLY for a disposable VM/user. +[CmdletBinding()] +param( + [switch]$Execute, + [string]$Confirmation = '', + [string]$Setup = '', + [string]$UpgradeSetup = '', + [string]$ExpectedVersion = '', + [ValidateSet('disabled', 'owned', 'foreign')] + [string]$AutostartCase = 'owned', + [string]$EvidenceDirectory = '' +) +$ErrorActionPreference = 'Stop' +Set-StrictMode -Version Latest +$stages = @('fresh-install', 'seed-settings-autostart', 'upgrade', 'uninstall') +if (-not $Execute) { + [ordered]@{status = 'not_run'; mode = 'plan-only'; stages = $stages; + requirement = 'Disposable Windows VM, standard user, two built installers; see docs/installer-smoke.md'} | + ConvertTo-Json -Depth 5 + exit 0 +} +# These are consent gates, not VM detection. NEVER set them on a workstation. +if ($Confirmation -cne 'I_ACCEPT_DISPOSABLE_VM' -or $env:PC_REMOTE_DISPOSABLE_VM -cne '1') { + throw 'Execution denied: explicit disposable-VM confirmation and environment gate required.' +} +if ($env:OS -ne 'Windows_NT') { throw 'Windows is required.' } +$identity = [Security.Principal.WindowsIdentity]::GetCurrent() +$principal = New-Object Security.Principal.WindowsPrincipal($identity) +if ($principal.IsInRole([Security.Principal.WindowsBuiltInRole]::Administrator)) { + throw 'Use a non-elevated standard-user account; elevation invalidates the user-scope test.' +} +if (-not $Setup -or -not $UpgradeSetup -or -not $ExpectedVersion -or -not $EvidenceDirectory) { + throw 'Setup, UpgradeSetup, ExpectedVersion and a new EvidenceDirectory are required.' +} +$Setup = (Resolve-Path -LiteralPath $Setup).Path +$UpgradeSetup = (Resolve-Path -LiteralPath $UpgradeSetup).Path +foreach ($binary in @($Setup, $UpgradeSetup)) { + if ([IO.Path]::GetExtension($binary) -ne '.exe') { throw 'Installer must be an EXE.' } +} +if (Test-Path -LiteralPath $EvidenceDirectory) { throw 'EvidenceDirectory must not already exist.' } +$installDir = Join-Path $env:LOCALAPPDATA 'Programs\PC Remote' +$exe = Join-Path $installDir 'PC Remote.exe' +$settingsDir = Join-Path $env:APPDATA 'PC Remote' +$legacyDir = Join-Path $env:APPDATA 'PC-Android' +$startupDir = [Environment]::GetFolderPath('Startup') +$startup = Join-Path $startupDir 'PC Remote.cmd' +$legacyStartup = Join-Path $startupDir 'PC-Android.cmd' +$runKey = 'HKCU:\Software\Microsoft\Windows\CurrentVersion\Run' +$arpSuffix = 'Software\Microsoft\Windows\CurrentVersion\Uninstall\{8A5675B9-1F20-4E2B-8BB4-D0C42973F9E4}_is1' +$arpKey = "HKCU:\$arpSuffix" +$machineArp = "HKLM:\$arpSuffix" +$startMenu = Join-Path ([Environment]::GetFolderPath('Programs')) 'PC Remote\PC Remote.lnk' +$desktop = Join-Path ([Environment]::GetFolderPath('Desktop')) 'PC Remote.lnk' +function Read-RunValue { + if (Test-Path $runKey) { + return (Get-Item $runKey).GetValue('PC Remote', $null) + } + return $null +} +foreach ($path in @($installDir, $settingsDir, $legacyDir, $startup, $legacyStartup, $arpKey, $machineArp, $startMenu, $desktop)) { + if (Test-Path -LiteralPath $path) { throw "Dirty test user: existing $path. Revert VM; do not delete user data." } +} +if ($null -ne (Read-RunValue)) { throw 'Dirty test user: PC Remote Run value exists.' } +if (Get-Process -Name 'PC Remote' -ErrorAction SilentlyContinue) { throw 'PC Remote is already running.' } + +function Get-MachineState { + # Read-only. An access error is a BLOCKER, not an empty/clean inventory. + $services = @(Get-CimInstance Win32_Service -ErrorAction Stop | + Sort-Object Name | Select-Object Name, PathName, StartMode) + $rules = @(Get-NetFirewallRule -ErrorAction Stop | Sort-Object Name | + Select-Object Name, DisplayName, Enabled, Direction, Action, Profile) + $programs = @(Get-NetFirewallApplicationFilter -ErrorAction Stop | Sort-Object InstanceID | + Select-Object InstanceID, Program) + return (@{services = $services; rules = $rules; programs = $programs} | ConvertTo-Json -Depth 6 -Compress) +} +function Assert-True([bool]$Condition, [string]$Message) { + if (-not $Condition) { throw $Message } +} +function Invoke-Checked([string]$File, [string[]]$Arguments) { + $process = Start-Process -FilePath $File -ArgumentList $Arguments -Wait -PassThru + $script:events.Add(@{command = [IO.Path]::GetFileName($File); exitCode = $process.ExitCode}) + Assert-True ($process.ExitCode -eq 0) "Process failed: $File exit=$($process.ExitCode)" +} +function Assert-Installed([string]$Version = '') { + Assert-True (Test-Path -LiteralPath $exe) 'Installed EXE missing.' + Assert-True (Test-Path -LiteralPath $startMenu) 'Start Menu shortcut missing.' + Assert-True (-not (Test-Path -LiteralPath $desktop)) 'Unrequested desktop shortcut was created.' + Assert-True (-not (Test-Path -LiteralPath $machineArp)) 'Machine ARP registration was created.' + $arp = Get-ItemProperty -LiteralPath $arpKey + Assert-True ($arp.Publisher -eq 'mimaxon1') 'ARP publisher mismatch.' + if ($Version) { + Assert-True ($arp.DisplayVersion -eq $Version) 'ARP version mismatch.' + Assert-True ($arp.DisplayName -eq "PC Remote $Version") 'ARP display name mismatch.' + } + Assert-True (-not (Get-Process -Name 'PC Remote' -ErrorAction SilentlyContinue)) 'Silent installer launched app.' +} +function Assert-MachineUnchanged { + Assert-True ((Get-MachineState) -ceq $script:baseline) 'Service/firewall inventory changed; inspect evidence and revert VM.' +} + +New-Item -ItemType Directory -Path $EvidenceDirectory | Out-Null +$events = New-Object 'System.Collections.Generic.List[object]' +$report = [ordered]@{status = 'running'; autostartCase = $AutostartCase; + os = [Environment]::OSVersion.VersionString; + setupSha256 = (Get-FileHash $Setup -Algorithm SHA256).Hash; + upgradeSha256 = (Get-FileHash $UpgradeSetup -Algorithm SHA256).Hash; + completed = @(); events = $events; error = $null} +$failed = $false +try { + $baseline = Get-MachineState + $baseline | Set-Content (Join-Path $EvidenceDirectory 'machine-before.json') -Encoding UTF8 + $silent = @('/VERYSILENT', '/SUPPRESSMSGBOXES', '/NORESTART', '/SP-') + Invoke-Checked $Setup ($silent + @('/TASKS=""', ('/LOG="{0}"' -f (Join-Path $EvidenceDirectory 'install.log')))) + Assert-Installed + Assert-True (-not (Test-Path $startup)) 'Fresh install enabled Startup autostart.' + Assert-True ($null -eq (Read-RunValue)) 'Fresh install enabled Run-key autostart.' + Assert-MachineUnchanged + $report.completed += 'fresh-install' + + if ($AutostartCase -eq 'owned') { + # This is the only application CLI invocation; it must exit, not run servers. + Invoke-Checked $exe @('--install-autostart') + Assert-True (Test-Path $startup) 'Autostart CLI did not create Startup file.' + $generated = [IO.File]::ReadAllText($startup) + Assert-True ($generated.Contains(('start "" /b "{0}"' -f $exe))) 'Autostart does not target installed executable.' + # Exercise supported compatibility artifacts without running them. + Copy-Item -LiteralPath $startup -Destination $legacyStartup + if (-not (Test-Path $runKey)) { New-Item -Path $runKey | Out-Null } + New-ItemProperty -Path $runKey -Name 'PC Remote' -Value ('"{0}"' -f $exe) -PropertyType String -Force | Out-Null + } elseif ($AutostartCase -eq 'foreign') { + [IO.File]::WriteAllText($startup, "@echo off`r`nrem unrelated portable installation`r`n") + Copy-Item -LiteralPath $startup -Destination $legacyStartup + if (-not (Test-Path $runKey)) { New-Item -Path $runKey | Out-Null } + New-ItemProperty -Path $runKey -Name 'PC Remote' -Value '"C:\Other Portable\PC Remote.exe"' -PropertyType String -Force | Out-Null + } + # Opaque sentinels test installer byte preservation, NOT application schema/migration. + New-Item -ItemType Directory -Path $settingsDir, $legacyDir -Force | Out-Null + $fixtures = @((Join-Path $settingsDir 'settings.json'), + (Join-Path $settingsDir 'network_settings.json'), (Join-Path $legacyDir 'settings.json')) + foreach ($file in $fixtures) { [IO.File]::WriteAllText($file, '{"installer_smoke_sentinel":"preserve-me"}') } + $hashes = @{} + foreach ($file in $fixtures) { $hashes[$file] = (Get-FileHash $file).Hash } + $startupHash = if (Test-Path $startup) { (Get-FileHash $startup).Hash } else { $null } + $runValue = Read-RunValue + $report.completed += 'seed-settings-autostart' + + Invoke-Checked $UpgradeSetup ($silent + @('/TASKS=""', ('/LOG="{0}"' -f (Join-Path $EvidenceDirectory 'upgrade.log')))) + Assert-Installed $ExpectedVersion + foreach ($file in $fixtures) { Assert-True ((Get-FileHash $file).Hash -eq $hashes[$file]) 'Upgrade modified persisted settings.' } + if ($null -eq $startupHash) { Assert-True (-not (Test-Path $startup)) 'Upgrade enabled autostart.' } + else { + Assert-True ((Get-FileHash $startup).Hash -eq $startupHash) 'Upgrade modified autostart choice.' + Assert-True ((Get-FileHash $legacyStartup).Hash -eq $startupHash) 'Upgrade modified legacy autostart choice.' + } + Assert-True ((Read-RunValue) -ceq $runValue) 'Upgrade modified Run value.' + Assert-MachineUnchanged + $report.completed += 'upgrade' + + Invoke-Checked (Join-Path $installDir 'unins000.exe') ($silent + @('/LOG="{0}"' -f (Join-Path $EvidenceDirectory 'uninstall.log'))) + Assert-True (-not (Get-Process -Name 'PC Remote' -ErrorAction SilentlyContinue)) 'Uninstall left running application.' + Assert-True (-not (Test-Path $exe)) 'Uninstall left application executable.' + Assert-True (-not (Test-Path $arpKey)) 'Uninstall left ARP registration.' + Assert-True (-not (Test-Path $startMenu)) 'Uninstall left Start Menu shortcut.' + foreach ($file in $fixtures) { Assert-True ((Get-FileHash $file).Hash -eq $hashes[$file]) 'Uninstall removed or modified settings.' } + if ($AutostartCase -eq 'foreign') { + foreach ($file in @($startup, $legacyStartup)) { Assert-True ((Get-FileHash $file).Hash -eq $startupHash) 'Uninstall removed unrelated Startup file.' } + Assert-True ((Read-RunValue) -ceq $runValue) 'Uninstall removed unrelated Run value.' + } else { + Assert-True (-not (Test-Path $startup)) 'Uninstall left owned Startup file.' + Assert-True (-not (Test-Path $legacyStartup)) 'Uninstall left owned legacy Startup file.' + Assert-True ($null -eq (Read-RunValue)) 'Uninstall left owned Run value.' + } + Assert-MachineUnchanged + (Get-MachineState) | Set-Content (Join-Path $EvidenceDirectory 'machine-after.json') -Encoding UTF8 + $report.completed += 'uninstall' + $report.status = 'passed' +} catch { + $report.status = 'failed' + $report.error = $_.Exception.Message + $failed = $true +} finally { + # Do not auto-delete fixtures or retry uninstall on failure. Preserve evidence; + # discard/revert the VM after review, including foreign/sentinel leftovers. + $report | ConvertTo-Json -Depth 8 | Set-Content (Join-Path $EvidenceDirectory 'result.json') -Encoding UTF8 + $report | ConvertTo-Json -Depth 8 +} +if ($failed) { exit 1 } diff --git a/tests/conftest_helpers.py b/tests/conftest_helpers.py new file mode 100644 index 0000000..ddd5b92 --- /dev/null +++ b/tests/conftest_helpers.py @@ -0,0 +1,14 @@ +"""Shared pytest helpers for migration tests.""" +from __future__ import annotations + +import os + + +def set_appdata_env(monkeypatch, tmp_path): + """Point all per-user app dirs at the test sandbox.""" + monkeypatch.setenv('APPDATA', str(tmp_path)) + monkeypatch.setenv('LOCALAPPDATA', str(tmp_path)) + + +def setenv(name, value): + return os.environ.__setitem__(name, value) diff --git a/tests/test_diagnostics.py b/tests/test_diagnostics.py new file mode 100644 index 0000000..eb27905 --- /dev/null +++ b/tests/test_diagnostics.py @@ -0,0 +1,60 @@ +"""Read-only diagnostics contracts; no real system changes.""" +import json +import subprocess +import sys +from pathlib import Path + + +def test_diagnostics_cli_is_read_only_and_does_not_echo_settings(tmp_path): + import os + import importlib.util + assert importlib.util.find_spec('diagnostics') is not None, 'read-only diagnostics module missing' + folder = tmp_path / 'PC Remote' + folder.mkdir() + settings = folder / 'settings.json' + settings.write_text('{"password": "DO_NOT_PRINT_SECRET"}', encoding='utf-8') + before = settings.read_bytes() + env = dict(os.environ, APPDATA=str(tmp_path), LOCALAPPDATA=str(tmp_path)) + result = subprocess.run([sys.executable, 'main.py', '--diagnostics'], + env=env, capture_output=True, text=True, timeout=20) + assert result.returncode == 0, result.stderr + report = json.loads(result.stdout) + assert report['version'] + assert report['config']['path'] == str(settings) + assert report['bind']['api']['port'] == 8000 + assert report['autostart']['state'] == 'disabled' + assert 'DO_NOT_PRINT_SECRET' not in result.stdout + result.stderr + assert settings.read_bytes() == before + assert not (folder / 'pc-remote.log').exists() + + +def test_diagnostics_reports_error_category_not_log_text(tmp_path, monkeypatch): + import config + import diagnostics + monkeypatch.setattr(config, 'app_dir', lambda: tmp_path) + (tmp_path / config.LOG_FILE_NAME).write_text( + '2026-09-17 12:00:00,000 - ERROR - failure secret-pin-and-token\n', encoding='utf-8') + result = diagnostics.collect_diagnostics() + assert result['last_error'] == {'state': 'recorded', 'code': 'application_error', + 'detail': 'omitted_for_privacy'} + assert 'secret-pin-and-token' not in json.dumps(result) + + +def test_firewall_probe_parses_summary_only(monkeypatch): + import diagnostics + def run(args, **kwargs): + assert args[0].lower().endswith('powershell.exe') + assert 'Get-NetFirewallRule' in args[-1] + assert 'New-NetFirewallRule' not in args[-1] + return subprocess.CompletedProcess(args, 0, '{"enabled_profiles":3,"app_rules":0}', '') + monkeypatch.setattr(diagnostics.subprocess, 'run', run) + assert diagnostics.firewall_status() == { + 'state': 'observed', 'enabled_profiles': 3, 'app_rules': 0, + 'effective_reachability': 'not_tested'} + + +def test_firewall_failure_never_echoes_command_stderr(monkeypatch): + import diagnostics + monkeypatch.setattr(diagnostics.subprocess, 'run', lambda *a, **kw: + subprocess.CompletedProcess(a, 1, '', 'secret-error')) + assert diagnostics.firewall_status() == {'state': 'unknown', 'reason': 'query_failed'} diff --git a/tests/test_installer_contract.py b/tests/test_installer_contract.py new file mode 100644 index 0000000..60391e1 --- /dev/null +++ b/tests/test_installer_contract.py @@ -0,0 +1,33 @@ +"""Static Inno contracts, not proof of Windows install/uninstall behavior.""" +from pathlib import Path + +ROOT = Path(__file__).resolve().parents[1] +ISS = ROOT / "installer" / "PC-Remote.iss" + + +def test_uninstall_cleans_only_matching_current_user_autostart(): + source = ISS.read_text(encoding="utf-8") + assert "procedure CurUninstallStepChanged" in source + assert "usUninstall" in source + assert "RemoveOwnedStartupFile('PC Remote.cmd')" in source + assert "RemoveOwnedStartupFile('PC-Android.cmd')" in source + # Remove only exact generated entries; never wildcard a user's Startup folder. + assert "Content = UTF8Encode(Expected)" in source + # Path.write_text(..., newline=None) on Windows translates explicit CRLF. + assert "StringChangeEx(Expected, #13#10, #13#13#10, True)" in source + assert "Command = Expected" in source + assert "RegDeleteValue(HKCU" in source + assert "DeleteFile(Filename)" in source + + +def test_user_scope_and_no_implicit_autostart_or_machine_changes(): + source = ISS.read_text(encoding="utf-8") + assert "PrivilegesRequired=lowest" in source + assert "DefaultDirName={localappdata}\\Programs\\{#MyAppName}" in source + assert "AppId={{8A5675B9-1F20-4E2B-8BB4-D0C42973F9E4}" in source + assert "postinstall skipifsilent" in source + for forbidden in ("PrivilegesRequiredOverridesAllowed", "[Registry]", "[UninstallDelete]", + "netsh", "HKLM", "sc.exe", "schtasks", "RegWrite"): + assert forbidden not in source + assert "settings.json" not in source + assert "network_settings.json" not in source diff --git a/tests/test_installer_smoke.py b/tests/test_installer_smoke.py new file mode 100644 index 0000000..9572252 --- /dev/null +++ b/tests/test_installer_smoke.py @@ -0,0 +1,55 @@ +"""Safety checks for the optional disposable-VM script; never execute it here.""" +from pathlib import Path +import json +import os +import shutil +import subprocess + +import pytest + +ROOT = Path(__file__).resolve().parents[1] +SCRIPT = ROOT / "scripts" / "installer_smoke.ps1" + + +def test_vm_smoke_is_an_explicit_opt_in(): + assert SCRIPT.is_file(), "Disposable-VM installer smoke harness is missing" + text = SCRIPT.read_text(encoding="utf-8") + assert "[switch]$Execute" in text + assert "PC_REMOTE_DISPOSABLE_VM" in text + assert "I_ACCEPT_DISPOSABLE_VM" in text + assert text.index("if (-not $Execute)") < text.index("Start-Process") + assert "IsInRole" in text # reject elevated context + + +def test_lifecycle_checks_legacy_choice_and_process_residue(): + text = SCRIPT.read_text(encoding="utf-8") + assert "Upgrade modified legacy autostart choice." in text + assert "Uninstall left running application." in text + assert "New-Item -Path $runKey -Force" not in text + assert "if (-not (Test-Path $runKey))" in text + + +@pytest.mark.skipif(not shutil.which("powershell.exe"), reason="PowerShell unavailable") +def test_default_vm_smoke_is_plan_only(): + assert SCRIPT.is_file(), "Disposable-VM installer smoke harness is missing" + result = subprocess.run( + ["powershell.exe", "-NoProfile", "-NonInteractive", "-File", str(SCRIPT)], + capture_output=True, text=True, timeout=30, + ) + assert result.returncode == 0, result.stderr + report = json.loads(result.stdout) + assert report["status"] == "not_run" + assert report["stages"] == ["fresh-install", "seed-settings-autostart", "upgrade", "uninstall"] + + +@pytest.mark.skipif(not shutil.which("powershell.exe"), reason="PowerShell unavailable") +@pytest.mark.parametrize("confirmation,env_gate", [("", ""), ("I_ACCEPT_DISPOSABLE_VM", ""), ("", "1")]) +def test_execute_is_rejected_without_both_consent_gates(confirmation, env_gate): + # Deliberately never supply both: cannot reach OS-changing code. + env = dict(os.environ, PC_REMOTE_DISPOSABLE_VM=env_gate) + command = ["powershell.exe", "-NoProfile", "-NonInteractive", "-File", str(SCRIPT), "-Execute"] + if confirmation: + command += ["-Confirmation", confirmation] + result = subprocess.run(command, env=env, capture_output=True, timeout=30) + assert result.returncode != 0 + assert b"Execution denied" in result.stderr diff --git a/tests/test_migration.py b/tests/test_migration.py new file mode 100644 index 0000000..20cf4bf --- /dev/null +++ b/tests/test_migration.py @@ -0,0 +1,113 @@ +"""Unified safe legacy migration contracts. No real user paths are touched.""" +import json +import os + +import pytest + +from conftest_helpers import set_appdata_env # noqa: F401 (re-exported helper) + + +def _write(path, payload): + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(json.dumps(payload), encoding='utf-8') + + +def _read(path): + return json.loads(path.read_text(encoding='utf-8')) + + +def test_settings_preserve_legacy_file_when_current_exists(tmp_path, monkeypatch): + set_appdata_env(monkeypatch, tmp_path) + import auth + legacy = tmp_path / 'PC-Android' / 'settings.json' + current = tmp_path / 'PC Remote' / 'settings.json' + _write(legacy, {'version': 2, 'password': {'is_set': False}}) + _write(current, {'version': auth.SETTINGS_VERSION, 'password': {'is_set': True}}) + legacy_bytes = legacy.read_bytes() + + result = auth.settings_path() + + assert result == current + assert current.read_bytes() == current.read_bytes() + assert _read(current)['password'] == {'is_set': True} + assert legacy.exists(), 'legacy settings file must not be deleted' + assert legacy.read_bytes() == legacy_bytes + + +def test_settings_migrate_move_legacy_when_no_current(tmp_path, monkeypatch): + set_appdata_env(monkeypatch, tmp_path) + import auth + legacy = tmp_path / 'PC-Android' / 'settings.json' + current = tmp_path / 'PC Remote' / 'settings.json' + payload = {'version': 2, 'password': {'is_set': False}, 'network': {'x': 1}} + _write(legacy, payload) + + result = auth.settings_path() + + assert result == current + assert not legacy.exists() + assert _read(current) == payload + + +def test_settings_failed_migration_never_deletes_source(tmp_path, monkeypatch): + set_appdata_env(monkeypatch, tmp_path) + import auth + legacy = tmp_path / 'PC-Android' / 'settings.json' + current = tmp_path / 'PC Remote' / 'settings.json' + payload = {'version': 2, 'password': {'is_set': False}} + _write(legacy, payload) + + def broken_move(*args, **kwargs): + raise OSError('disk on fire') + monkeypatch.setattr('migration.shutil.move', broken_move) + + result = auth.settings_path() + + assert result == current + assert not current.exists() + assert legacy.exists() + assert _read(legacy) == payload + + +def test_network_settings_preserve_legacy_file_when_current_exists(tmp_path, monkeypatch): + set_appdata_env(monkeypatch, tmp_path) + import net_utils + legacy = tmp_path / 'PC-Android' / 'settings.json' + current = tmp_path / 'PC Remote' / 'settings.json' + _write(legacy, {'network': {'preferred_ip': '192.168.1.20'}}) + _write(current, {'network': {'preferred_ip': '192.168.1.2'}}) + + iface, ip = net_utils._load_network_settings() + + assert (iface, ip) == (None, '192.168.1.2') + assert legacy.exists() + assert _read(legacy)['network']['preferred_ip'] == '192.168.1.20' + + +def test_autostart_preserves_legacy_cmd_when_current_exists(tmp_path, monkeypatch): + set_appdata_env(monkeypatch, tmp_path) + import autostart + startup = tmp_path / 'Microsoft' / 'Windows' / 'Start Menu' / 'Programs' / 'Startup' + startup.mkdir(parents=True, exist_ok=True) + legacy = startup / 'PC-Android.cmd' + current = startup / 'PC Remote.cmd' + legacy.write_text('@echo off\r\nstart \"\" /b OLD\r\n', encoding='utf-8') + current.write_text('@echo off\r\nstart \"\" /b NEW\r\n', encoding='utf-8') + + state = autostart.is_enabled() + + # Byte content must contain the NEW command; endings may vary. + raw = current.read_bytes() + assert b'start "" /b NEW' in raw + assert b'OLD' not in raw + assert legacy.exists() + + +def test_install_writes_exact_crlf_bytes(tmp_path, monkeypatch): + set_appdata_env(monkeypatch, tmp_path) + import autostart + path = autostart.install() + raw = path.read_bytes() + assert raw.endswith(b'\r\n') + assert b'\r\r\n' not in raw + assert raw.startswith(b'@echo off\r\nstart "" /b ') diff --git a/tests/test_security_contract.py b/tests/test_security_contract.py new file mode 100644 index 0000000..f1a547a --- /dev/null +++ b/tests/test_security_contract.py @@ -0,0 +1,61 @@ +"""Defensive unit contracts; no real network or system actions.""" +from unittest.mock import MagicMock + +import pytest +from fastapi import HTTPException + +import main + + +def test_legacy_pin_guard_checks_budget_before_hash(monkeypatch): + assert hasattr(main, '_verify_legacy_pin'), 'legacy PIN budget guard missing' + manager = MagicMock() + limiter = MagicMock() + limiter.blocked_until.return_value = 100 + monkeypatch.setattr(main, 'LOGIN_RATE_LIMITER', limiter) + with pytest.raises(HTTPException) as exc: + main._verify_legacy_pin(manager, 'test-only') + assert exc.value.status_code == 429 + manager.verify_password.assert_not_called() + + +def test_server_disables_forwarded_identity_trust(monkeypatch): + server = MagicMock() + conf = MagicMock() + monkeypatch.setattr(main.uvicorn, 'Config', conf) + monkeypatch.setattr(main.uvicorn, 'Server', lambda cfg: server) + main.run_api() + assert conf.call_args.kwargs.get('proxy_headers') is False + + +def test_legacy_budget_is_charged_on_failure_and_reset_on_success(monkeypatch): + manager = MagicMock() + limiter = MagicMock() + limiter.blocked_until.return_value = None + limiter.record_failure.return_value = None + monkeypatch.setattr(main, 'LOGIN_RATE_LIMITER', limiter) + manager.verify_password.return_value = False + assert main._verify_legacy_pin(manager, 'test-only') is False + limiter.record_failure.assert_called_once_with('legacy-pin-actions') + manager.verify_password.return_value = True + assert main._verify_legacy_pin(manager, 'test-only') is True + limiter.reset.assert_called_once_with('legacy-pin-actions') + + +def test_browser_origin_policy_is_explicit(): + assert hasattr(main, '_browser_origin_allowed'), 'explicit origin policy missing' + assert main._browser_origin_allowed(None) + assert main._browser_origin_allowed(main.allowed_origins[0]) + assert not main._browser_origin_allowed('https://untrusted.invalid') + assert not main._browser_origin_allowed('null') + + +def test_token_authorization_does_not_use_legacy_pin_budget(monkeypatch): + manager = MagicMock() + manager.requires_password_setup.return_value = False + manager.verify_token.return_value = True + guard = MagicMock() + monkeypatch.setattr(main, '_auth_manager', lambda: manager) + monkeypatch.setattr(main, '_verify_legacy_pin', guard) + main.check('test-token', None) + guard.assert_not_called() From 9043fbcb8942ade44d1e2ad9bd84cc8da80a5052 Mon Sep 17 00:00:00 2001 From: mimaxon1 Date: Thu, 17 Sep 2026 09:51:45 +0300 Subject: [PATCH 2/2] migration: fail closed when leftover legacy settings cannot move Raise SettingsError instead of silently initializing fresh auth settings when a legacy file remains and the destination is missing. Preserve the same non-destructive rule for network settings with an explicit warning. Claude-Session: https://claude.ai/code/session_012Wt8ZEQeC9WJFrTQaieQJH --- auth.py | 11 ++++++++- docs/PROJECT_BATCH_CHECKLIST.md | 10 ++++---- docs/SECURITY_REVIEW.md | 3 ++- net_utils.py | 9 ++++++- tests/test_migration.py | 42 +++++++++++++++++++++++++++++++-- 5 files changed, 65 insertions(+), 10 deletions(-) diff --git a/auth.py b/auth.py index 508bda4..8c2aabc 100644 --- a/auth.py +++ b/auth.py @@ -66,7 +66,16 @@ def _migrate_legacy_settings(target: Path) -> None: 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: diff --git a/docs/PROJECT_BATCH_CHECKLIST.md b/docs/PROJECT_BATCH_CHECKLIST.md index 47317e4..53e78ee 100644 --- a/docs/PROJECT_BATCH_CHECKLIST.md +++ b/docs/PROJECT_BATCH_CHECKLIST.md @@ -7,10 +7,10 @@ Original checkout `pc-with-android` has uncommitted work and corrupt refs; untou | Task | State | Evidence / next action | |---|---|---| -| 1 Security | PARTIAL | SECURITY.md exists; trusted-LAN HTTP is intentional. Review authentication/command/install boundaries; remediate only confirmed defects. | -| 2 Migration | PARTIAL | auth.py and autostart.py contain separate legacy migration, swallowed errors and source deletion; network fix branch needs comparison. | -| 3 Branch report | TODO | 11 legacy installer/autostart/firewall/settings branches available; compare against current main, no bulk merges. | -| 4 Diagnostics | TODO | Only autostart-status CLI exists; add read-only JSON command with no secret values. | -| 5 Installation matrix | PARTIAL | Current release CI covers installation metadata; installer has no uninstall autostart cleanup. Add safe contracts and disposable-Windows smoke harness. | +| 1 Security | DONE | Confirmed defects fixed: legacy PIN attempt budget (serialized), explicit browser Origin rejection, `proxy_headers=False`. `SECURITY.md` + `docs/SECURITY_REVIEW.md` + `tests/test_security_contract.py`. | +| 2 Migration | DONE | Unified `migration.safe_migrate`; auth raises `SettingsError` when a legacy file remains and the destination is missing (no silent reset). Network leftover is preserved and ignored with a warning. Autostart never deletes a colliding legacy CMD. Tests: `tests/test_migration.py`. | +| 3 Branch report | DONE | `BRANCH_MIGRATION_REPORT.md` over 12 topic refs vs `d728079`. No bulk merges. Installer keeps main AppId/user-scope. | +| 4 Diagnostics | DONE | `python main.py --diagnostics` / `diagnostics.py`; no secrets. Tests: `tests/test_diagnostics.py`. | +| 5 Installation matrix | PARTIAL / BLOCKED (VM) | Static Inno contracts + opt-in disposable-VM harness (`scripts/installer_smoke.ps1`, `docs/installer-smoke.md`). Real Inno compile + lifecycle not run here (no ISCC, no `dist/PC Remote/PC Remote.exe`, no disposable VM). | Real install/uninstall validation requires a disposable Windows user/VM: do not modify this workstation's actual autostart, firewall, services or user configuration. Mock/static tests are not OS lifecycle proof. diff --git a/docs/SECURITY_REVIEW.md b/docs/SECURITY_REVIEW.md index a9f53ab..b442aa7 100644 --- a/docs/SECURITY_REVIEW.md +++ b/docs/SECURITY_REVIEW.md @@ -8,7 +8,8 @@ Reviewed base: `d728079` (current main at clone time). Source review and safe un | PIN authorization | main.py check() and change_password() call verify_password directly; only login() accounts failed attempts | Confirmed missing attempt budget on alternate PIN authorization paths. Shared legacy budget added with serialized admission/verify/accounting (lock), preserving PIN compatibility; token traffic unaffected. | | Browser request execution | main.py CORSMiddleware; pair token issuance loopback-only; unauthenticated restart loopback-only, authenticated remote restart supported | Confirmed CORS-only browser boundary: CORS response restrictions do not reject all cross-origin execution. Explicit Origin rejection added before handlers. No-Origin native/tray clients still supported. | | Peer identity | main.py run_api() leaves Uvicorn proxy_headers default; local-only access depends on request.client | Hardening: explicitly disable forwarded-header trust for this direct-LAN server. No configurable trusted reverse proxy is supported. | -| Settings migration | auth.py _migrate_from_source deletes source when destination exists and swallows copy errors | Confirmed data-integrity problem; addressed in migration workstream. | +| Settings migration | auth.py / net_utils.py / migration.py | Confirmed data-integrity problem (source deleted when destination existed; failures swallowed). Unified `safe_migrate` never deletes a colliding source. Auth raises `SettingsError` when a leftover legacy file cannot be moved and no destination exists (no silent reset). Network leftover is preserved and ignored with a warning. | + | Autostart lifecycle | autostart.py writes per-user Startup CMD; installer lacks cleanup | Confirmed lifecycle gap; addressed in installer workstream, tested without changing this workstation. | | Command launch | apps.py executable validation, start helpers, subprocess argv with shell=False; main.py launch endpoint authorization | By-design broad authenticated desktop control, not a sandbox. No claim that extension checks prevent a trusted client invoking an interpreter. Do not run elevated. | | Restart | main.py _powershell_quote/_spawn_restart_watcher | Commands originate locally and strings use single-quote escaping. No remote command-string injection confirmed by this review. | diff --git a/net_utils.py b/net_utils.py index a0bb57b..ba37554 100644 --- a/net_utils.py +++ b/net_utils.py @@ -98,7 +98,14 @@ def _migrate_legacy_settings(target: Path) -> None: candidates.append(_legacy_runtime_settings_path()) for source in candidates: if _migrate_from_source(source, target): - break + return + if target.exists() or not any(source.exists() for source in candidates): + return + logger.warning( + "Legacy network settings exist but could not be migrated to %s; " + "leaving them in place and ignoring them this run", + target, + ) def _settings_path() -> Path: diff --git a/tests/test_migration.py b/tests/test_migration.py index 20cf4bf..9efd017 100644 --- a/tests/test_migration.py +++ b/tests/test_migration.py @@ -49,7 +49,7 @@ def test_settings_migrate_move_legacy_when_no_current(tmp_path, monkeypatch): assert _read(current) == payload -def test_settings_failed_migration_never_deletes_source(tmp_path, monkeypatch): +def test_settings_failed_migration_raises_and_never_deletes_source(tmp_path, monkeypatch): set_appdata_env(monkeypatch, tmp_path) import auth legacy = tmp_path / 'PC-Android' / 'settings.json' @@ -61,12 +61,50 @@ def broken_move(*args, **kwargs): raise OSError('disk on fire') monkeypatch.setattr('migration.shutil.move', broken_move) + with pytest.raises(auth.SettingsError, match='could not be migrated'): + auth.settings_path() + + assert not current.exists() + assert legacy.exists() + assert _read(legacy) == payload + + +def test_settings_failed_migration_keeps_existing_current_without_error(tmp_path, monkeypatch): + set_appdata_env(monkeypatch, tmp_path) + import auth + legacy = tmp_path / 'PC-Android' / 'settings.json' + current = tmp_path / 'PC Remote' / 'settings.json' + _write(legacy, {'version': 2, 'password': {'is_set': False}}) + _write(current, {'version': auth.SETTINGS_VERSION, 'password': {'is_set': True}}) + + def broken_move(*args, **kwargs): + raise OSError('disk on fire') + monkeypatch.setattr('migration.shutil.move', broken_move) + result = auth.settings_path() assert result == current + assert legacy.exists() + assert _read(current)['password'] == {'is_set': True} + + +def test_network_failed_migration_keeps_source_and_ignores_it(tmp_path, monkeypatch): + set_appdata_env(monkeypatch, tmp_path) + import net_utils + legacy = tmp_path / 'PC-Android' / 'settings.json' + current = tmp_path / 'PC Remote' / 'settings.json' + _write(legacy, {'network': {'preferred_ip': '192.168.1.20'}}) + + def broken_move(*args, **kwargs): + raise OSError('disk on fire') + monkeypatch.setattr('migration.shutil.move', broken_move) + + iface, ip = net_utils._load_network_settings() + + assert (iface, ip) == (None, None) assert not current.exists() assert legacy.exists() - assert _read(legacy) == payload + assert _read(legacy)['network']['preferred_ip'] == '192.168.1.20' def test_network_settings_preserve_legacy_file_when_current_exists(tmp_path, monkeypatch):