fix(electron): make launch-chooser selections authoritative + harden provisioning#379
Open
danshapiro wants to merge 6 commits into
Open
fix(electron): make launch-chooser selections authoritative + harden provisioning#379danshapiro wants to merge 6 commits into
danshapiro wants to merge 6 commits into
Conversation
…provisioning Independent review of PR #377 (the launch-connection chooser) surfaced several launch-flow defects. This addresses them: - Honor explicit chooser selections for the current launch. The handler used to persist config and re-run the policy, so alwaysAskOnLaunch looped straight back to the chooser and a "Start local" pick was overridden whenever any server was detected. Selections now flow through a one-shot ForcedLaunch that startup executes directly, bypassing discovery and policy. - Respect "Remember this choice": when unchecked, the server selection is no longer persisted (only the standalone always-ask preference is). - Pre-fill the saved remote URL in the chooser for saved-remote recovery via a tested buildLaunchOptions(). - Validate the local-server port (1024-65535, integers) in both the renderer and the handler before persisting or launching. - Fix the e2e selector: the candidate button's accessible name is "Connect to <label>"; also exercise the always-ask path now that a selection works without unchecking it first. - Correct stale Windows build prerequisites (no curl/tar/unzip needed). - Stop hand-writing JSON in NSIS (which cannot escape values). The installer writes a line-based desktop.provision file; the app converts it to a properly serialized desktop.json on first launch. Red-Green-Refactor TDD; new unit coverage for forced launch, remember gating, port validation, launch options, and provisioning. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d105666b9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+57
to
+61
| await deps.patchDesktopConfig({ | ||
| serverMode: 'remote', | ||
| remoteUrl, | ||
| remoteToken, | ||
| setupCompleted: true, |
There was a problem hiding this comment.
When a silent installer provisions an already-configured desktop that previously enabled alwaysAskOnLaunch, this patch merges only the remote fields into the existing config, so the preserved alwaysAskOnLaunch: true makes chooseLaunchAction() show the chooser instead of connecting to the newly provisioned remote server on first launch. The old installer overwrote desktop.json without this field, causing the schema default (false) to apply, so this is a regression for managed reinstall/update flows; include alwaysAskOnLaunch: false (or otherwise force the provisioned launch) with the provisioning patch.
Useful? React with 👍 / 👎.
…oning reads Addresses an independent review of the prior commit: - Scope choose-launch-option to the chooser window (isAllowedSender) and validate the chosen URL scheme (http/https) in the main process, so a crafted IPC choice can no longer force the app to load an arbitrary URL or file:// path. (critical) - Refuse "Start local" when the chosen port is already served by a detected local Freshell, before closing the chooser, avoiding a doomed spawn onto an occupied port. (major) - Catch read failures in applyProvisioningFile (locked file, a directory, bad permissions) so an unreadable desktop.provision can never abort startup; clear it best-effort instead. (major) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The port-collision guard was renderer-only and therefore advisory. Add an authoritative check in the choose-launch-option handler (injected isPortAvailable, wired to is-port-reachable on localhost in entry.ts). Reject before closing the chooser when the chosen port is occupied or when availability cannot be determined, so a stale candidate list, a click before getLaunchOptions resolves, an occupied non-Freshell port, or a crafted request can no longer trigger a doomed spawn onto a live port. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… provisioned values Addresses a further review pass: - Runtime-validate the choose-launch-option payload with a Zod schema (LaunchChoiceSchema) at the IPC boundary, and reject an unknown kind via an explicit default instead of letting it fall through to start-local. A non-object payload now returns a controlled error rather than throwing. - Replace the localhost reachability probe with a real net.Server bind attempt (createPortAvailabilityCheck), so a port occupied on another local interface (e.g. a 0.0.0.0 bind under WSL) is detected before spawning. - Preserve provisioned values verbatim (no trim), matching the documented raw-preservation contract so tokens with meaningful whitespace aren't corrupted. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ative check The renderer's "Start local" occupancy check used the initial candidate snapshot, so if a detected server exited while the chooser stayed open it could false-reject starting on the now-free port. Remove that stale-snapshot guard (and the now-unused localCandidatePort helper); the renderer keeps only port-range validation and submits the choice, letting the main process's fresh bind test (isPortAvailable) decide occupancy. The chooser stays open and shows the handler's error if the port is genuinely taken. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The renderer reads the token back via URLSearchParams.get, so a token containing +, &, #, or whitespace was corrupted by raw string concatenation in loadMainWindow -- a server that passed the new main-process token checks could then load unauthenticated. Percent-encode the token so it round-trips through URLSearchParams. Existing alphanumeric/hyphen tokens are unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Owner
Author
Independent (fresh-eyes / GPT) review logRan 5 rounds of independent review, fixing every finding TDD-style between rounds. Findings narrowed each round (the original PR #377 issues were cleared in round 1; later rounds surfaced progressively deeper hardening gaps in the new launch path):
Stopped at the 5-round cap. The final fix ( Local verification (final commit): electron unit suite 310 passing; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
An independent (fresh-eyes) review of PR #377 (the Electron launch-connection chooser, merged without review) surfaced several launch-flow defects. This PR fixes them, on a branch from
origin/main, with Red-Green-Refactor TDD.What
alwaysAskOnLaunchand looped back to the chooser. Selections now flow through a one-shotForcedLaunchthatrunStartupexecutes directly.start-localspawns a bundled server on the chosen port regardless of detected candidates or savedserverMode.buildLaunchOptions().Connect to <label>, notConnect. The test now also exercises the always-ask path (validating #1) without the prior workaround.prepare-bundled-node.tsuses Nodehttp/https+ thetar/extract-zippackages, not externalcurl/tar/unzip.desktop.provisionfile; the app converts it to a properly serializeddesktop.jsonon first launch (all escaping in JS, fully unit-tested).Tests
buildLaunchOptions, and provisioning parse/apply.npm run test:vitest -- --config vitest.electron.config.ts→ 298 passing.typecheck:client(the merge gate),typecheck:server, and both electrontscprojects are clean.Caveats
docs/development/windows-electron-build.md); it should be confirmed by the Windowselectron-buildCI matrix. The change is deliberately minimal (two plainFileWritelines) precisely because NSIS is unverifiable here.jsx-a11ylint error insrc/is unrelated to this PR (nosrc/files changed).🤖 Generated with Claude Code