Skip to content

fix(launcher): System32 tools, instance-lock contention vs failure, fragment-only credentials - #330

Closed
dovvnloading wants to merge 3 commits into
mainfrom
fix/launcher-hardening
Closed

dovvnloading wants to merge 3 commits into
mainfrom
fix/launcher-hardening

Conversation

@dovvnloading

Copy link
Copy Markdown
Owner

Problem

Three launcher defects:

  • LP-12.
    • What: the WebView2 signature check (the gate before the bundled installer runs), the private-folder ACL step and the dev-server tree kill started powershell.exe, icacls and taskkill by bare name.
    • Effect: whichever copy came first on a user-modified PATH ran instead of Windows' own.
  • LP-10.
    • What: InstanceLock.acquire() returned None for every failure, exactly as for "another Cortex holds the lock". That included an icacls error, a lock file that could not be opened, and a secret or record that could not be written.
    • Effect: the launch then looked for a running instance that did not exist and exited 2. Several failing exits also printed to a console a windowed build does not have and wrote no startup diagnostic, while the dialog said the log "could not be written".
  • LP-02 (remainder).
    • What: readLauncherCredentials() still honoured ?bootstrap= and ?handoff= in the query string.
    • Context: the launcher has only ever sent them in the fragment (main.py _desktop_url), which never reaches a server, request line or log. Only Playwright specs and screenshot scripts used the query form.

Change

One commit each.

  1. core/win_paths.system32(name) resolves under the directory GetSystemDirectoryW reports, with %SystemRoot% / %windir% only as a fallback. PowerShell is looked up in WindowsPowerShell\v1.0. It falls back to the bare name only when that absolute path does not exist. It is used at all three call sites.
  2. acquire() returns None only for a contended lock. That means EACCES/EAGAIN/EWOULDBLOCK/EDEADLK from the lock call or from writing the locked byte.
    • Anything else raises InstanceLockError(reason). Its message is fixed text plus the OS error's class name, never a path or secret.
    • _run_web records it under the stage "instance lock", and the dialog names the data-folder remedy.
    • The lock-file size is read with fstat (reading a locked byte fails), and normalising writes go straight to the descriptor.
    • Every non-zero exit goes through _fail(stage, message, code), which writes the diagnostic. The dialog mentions an unwritable log only when a write was attempted.
  3. The frontend reads both credentials from the fragment only. The query is still scrubbed from the address bar. The e2e specs and screenshot scripts open /#bootstrap=... as the launcher does.

Not included from the original branch, because they add features rather than fix defects: remembering the window's size and position, and asking before closing during a reply.

Compatibility and rollback

  • No API or data change.
  • A URL with credentials in the query string no longer signs in. Only test tooling ever built one, and it is updated here.
  • Each commit reverts on its own.

Checks

Windows, Python 3.14 unless stated.

  • Focused tests: python -m pytest -q tests/test_launcher.py tests/test_win_paths.py gives 222 passed. Python 3.12 (py -3.12): 222 passed.
  • Frontend: npx vitest run src/app/App.test.tsx gives 39 passed.
  • Against the old code: the new tests import the new modules, so they cannot be collected on origin/main. Instead, the old behaviour was put back inside the new code: _contended() made to always return True (every failure treated as contention), and system32() made to skip the system directory. Result: 6 failed, 216 passed, including test_instance_lock_distinguishes_contention_from_setup_failure, the PowerShell, icacls and taskkill path tests, and the WebView2 check.
  • Pre-push hook (scripts/check.ps1 quick tier): all 11 checks passed in 637 s (backend 450.8 s, vitest 99 s).
  • Playwright specs with the changed URLs: not run locally; the CI "Browser tests" job runs them.

Security

  • LP-12 removes a PATH dependency from the installer's signature gate.
  • LP-02 removes the one way a credential could appear in a URL that is sent to the server.
  • The lock error messages carry no paths or secrets.

Limits

  • Not exercised in the real packaged WebView2 window.
  • The contention errno set covers msvcrt locking and flock as documented; an unusual filesystem could report contention differently and would now show as a setup failure, not as "already running".

Plan item: LP-12, LP-10, LP-02 (remainder)

🤖 Generated with Claude Code

dovvnloading and others added 3 commits October 17, 2026 09:41
… PATH

The WebView2 signature check (the gate before the bundled installer runs),
the private-folder ACL and the dev-server tree kill started their tools by
bare name, so whichever powershell.exe, icacls or taskkill came first on a
user-modified PATH ran instead. core/win_paths.system32() resolves them under
the directory GetSystemDirectoryW reports (%SystemRoot% / %windir% only as a
fallback), with PowerShell's WindowsPowerShell\v1.0 subfolder, and falls back
to the bare name only when that absolute path does not exist.

Plan item: LP-12

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…log every failed exit

InstanceLock.acquire() returned None for every failure -- an icacls error,
a lock file that could not be opened, a secret or record that could not be
written -- exactly as it did for "another Cortex holds the lock". The launch
then went looking for a running instance that did not exist and exited 2.
acquire() now returns None only for a contended lock (EACCES/EAGAIN/
EWOULDBLOCK/EDEADLK from the lock call, or from writing the locked byte) and
raises InstanceLockError(reason) for anything else; _run_web records it under
the stage "instance lock" and the dialog names the --data-dir remedy. The
lock-file size is read with fstat instead of reading the byte, which fails
under a held lock, and normalising writes go straight to the descriptor.

Several failing exits (no record behind a held lock, a dead holder's lock that
never frees, an unreadable handoff secret, a headless backend that stopped on
its own) printed to a console a windowed build does not have and wrote no
startup diagnostic, while the dialog said the log "could not be written".
Every non-zero exit now goes through _fail(stage, message, code), which writes
the diagnostic and prints; the dialog only mentions an unwritable log when a
write was actually attempted.

Plan item: LP-10

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
readLauncherCredentials() still honoured ?bootstrap= and ?handoff= in the
query string, although the launcher has only ever sent them in the fragment
(main.py _desktop_url), which never reaches a server, a request line or a
log. Nothing in production used the query branches -- only the Playwright
specs and the screenshot scripts did. The query is no longer read (it is
still scrubbed from the address bar), and those specs and scripts now open
/#bootstrap=... like the launcher does.

Plan item: LP-02 (remainder)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dovvnloading

Copy link
Copy Markdown
Owner Author

Superseded by #331, which combines this with the other pending fixes into one pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant