Skip to content

fix(tray): isolate Windows tray host from proxy sockets - #752

Closed
luvs01 wants to merge 1 commit into
lidge-jun:devfrom
luvs01:fix/733-windows-socket-inheritance
Closed

fix(tray): isolate Windows tray host from proxy sockets#752
luvs01 wants to merge 1 commit into
lidge-jun:devfrom
luvs01:fix/733-windows-socket-inheritance

Conversation

@luvs01

@luvs01 luvs01 commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Launch the long-lived Windows tray host through a short ProcessStartInfo.UseShellExecute=true PowerShell broker instead of spawning it directly from the live Bun proxy process tree.
  • Keep executable and CLI paths out of the PowerShell command text: the encoded launcher is constant, paths are validated with the existing safePath rules, and values are passed through environment variables.
  • Add a Windows regression test that keeps the tray child alive, stops the originating Bun listener, and proves that the same port can be rebound immediately. The fixture also covers Unicode paths and paths containing & and %TEMP%.

Fixes #733.

Root cause and scope

On Windows, Bun 1.3.14 and the tested Bun 1.4 canary can pass an inheritable proxy listen socket through the current proxy -> tray action -> detached tray host process lineage. The old proxy can then exit while the long-lived tray host keeps the socket handle open, leaving the prior port unavailable.

This change isolates only the long-lived tray host. It does not change tray registration, registry ownership rules, UAC/elevation behavior, heartbeat semantics, or the tray host's command-line contract. The existing heartbeat remains the authority for whether startup succeeded.

The relevant upstream Bun handle-list work (oven-sh/bun#33160) is currently open and not suitable as a released dependency, so this PR keeps the workaround local and removable.

Verification

  • Bun 1.3.14 focused tray/restart tests: 40 passed.
  • Bun 1.4.0-canary.1 focused tray/restart tests: 40 passed.
  • The new real-socket regression passed independently on both runtimes and verifies that the child remains alive after the original port is rebound.
  • bun run typecheck: passed on Bun 1.3.14 and Bun 1.4.0-canary.1.
  • bun run privacy:scan: passed.
  • git diff --check: passed.
  • The full suite reached a 304-second local limit before producing a final result. The exact descendant process chain belonging to that run was identified by worktree, command line, parent PID, and start time, then terminated and confirmed absent; this is not reported as a full-suite pass.

Draft status

This remains a draft pending an explicit review of the ShellExecute/environment boundary, AppLocker or EDR compatibility, and a final Windows full-suite run. Those checks are intentionally deferred rather than treated as complete here.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed (none required for this internal Windows launch-path fix).
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: b99b7d3e-5b47-4325-9c05-74d7d1009e28

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the bug Something isn't working label Jul 30, 2026
@lidge-jun

Copy link
Copy Markdown
Owner

NEEDS-SECURITY-REVIEW, then LAND — the mechanism holds up.

I checked this against #733 rather than taking the description on faith, and the isolation is real rather than cosmetic. src/tray/windows.ts:417-454 stops spawning the tray host directly and goes through a synchronous PowerShell broker that sets UseShellExecute = true, which routes creation through ShellExecuteEx instead of CreateProcess. Windows only inherits a socket when the handle is inheritable and the child is created with inheritance enabled, so crossing the shell boundary breaks the condition that keeps the port held. The broker itself may briefly inherit, but execFileSync waits for it to exit, and the process that outlives everything is created on the far side.

Passing the executable and CLI paths through environment variables instead of interpolating them into the encoded PowerShell program is the right call, and clearing those three variables at :462-465 before the long-lived WinForms process starts closes the obvious follow-on.

The regression at tests/windows-tray.test.ts:217-264 is what makes this reviewable: it keeps the launched child alive, stops the original Bun listener, immediately rebinds the same port, and asserts the child survived. That binds the reported failure instead of asserting on source strings. The fixture at tests/helpers/windows-tray-inheritance-child.ts is a genuinely long-lived descendant, not a stub.

Two things before merge. This changes an executable-launch trust boundary and deliberately invokes the Windows shell, which puts it under "other security-boundary changes" in MAINTAINERS.md:33-34 — so it needs explicit security review even though it touches no credentials. And the socket test skips off Windows, so fresh Windows CI is required evidence, not a formality.

The PR is still a draft and merges cleanly against the current dev head. dev CI is now fully green, so a red check here would be about this change.

What happens next: mark it ready when you are satisfied, get Windows CI green, and request security review. I found no code blocker in the isolation itself.

@lidge-jun

Copy link
Copy Markdown
Owner

Closing as superseded. c1ecbe1b5 feat(windows): add restart-safe tray controls landed the same protection on dev: runWindowsTrayHost() spawns the tray with stdio: "ignore" (src/tray/windows.ts:474-477), so the host cannot inherit the proxy's socket handles. git cherry dev <this branch> reports the commit as patch-equivalent.

#733 is closed with that evidence.

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

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants