Repository navigation
feat(auth): open the login URL in a Windows browser from WSL - #206
pjcdawkins wants to merge 7 commits into
Conversation
On WSL there is usually no DISPLAY and no xdg-open, so the login prompt was not offered, and login printed the URL instead of opening it. Under WSL, URLs are now opened with wslview if it is installed, or else with Windows' rundll32.exe (as on Windows), found in PATH or in its default location. The local server's 127.0.0.1 address works from Windows, as WSL forwards localhost. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Warning
Changes suggested — 🟡 2 warnings
🔍 Full review · 2 files reviewed
Verification
browserCommandkeeps the explicit--browseroption and the "0" opt-out ahead of the new WSL case, so users can still override the WSL choice.isWSLreturns false right away on non-Linux GOOS, so Windows and macOS behaviour is unchanged.- The rundll32 loop's inner
pshadows the loop variable, but it returns the resolved absolute path, whichTestBrowserCommand_WSLasserts. TestBrowserCommand_WSLsets PATH to the temp dir only, so a realwslviewon the host cannot change its result.
The new unit tests TestBrowserCommand_WSL (Linux only, stubbed rundll32.exe on PATH) and TestIsWSL_NotWSL cover detection and command selection, and run through make test. Nothing covers the case where no Windows opener is found, or opening a URL from real WSL; the PR says this needs a manual check.
Review details
- Commit: 80b7a16
- Model: claude-opus-5-5
Review 1 of 10 for this pull request · View the full run
… found WSL counted as having a display even without a Windows opener, e.g. with interop disabled, or in a Docker Desktop container, which shares the WSL kernel. The CLI then offered a browser login with no display, and xdg-open could start a terminal browser or fail. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Warning
Changes suggested — 🟡 1 warning
🔁 Incremental · 2 files reviewed
Verification
hasDisplay()now returns true for WSL only whenwslBrowserCommand()finds wslview or rundll32.exe, so the xdg-open fallback is never reached with DISPLAY unset.- A Docker Desktop container with the microsoft kernel, no
/mnt/cand no wslview now getshasDisplay()false and is not offered a browser login. - If
openURLfails,runBrowserLogin(auth_login.go:166) falls back to printing the URL, so a failed opener does not block login.
The new TestBrowserCommand_WSLWithoutOpener covers WSL with no opener file on PATH or under /mnt/c, and TestBrowserCommand_WSL covers the case where an opener is found. Nothing covers interop being disabled, or real WSL hosts, which the PR leaves to a manual check.
Review 2 of 10 for this pull request · View the full run
On WSL, the legacy CLI found no display or xdg-open, so legacy commands did not offer "Log in via a browser?" (with or without Go auth), and the PHP login printed the URL instead of opening it. Match the Go detection: under WSL (from WSL_DISTRO_NAME/WSL_INTEROP or the kernel release), count as having a display if a Windows opener is found, and open URLs with wslview, or else rundll32.exe from PATH or /mnt/c/Windows/System32. The lookup only runs on WSL, when a URL is to be opened. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
📋 PR Summary This PR lets the CLI open the login URL in a Windows browser when it runs under WSL. On WSL with Windows interop enabled, the CLI now counts as having a display and opens URLs with Changes
|
There was a problem hiding this comment.
Warning
Changes suggested — 🟡 1 warning · 1 still open
🔁 Incremental · 4 files reviewed
Outstanding from earlier reviews:
- 🟡 #4225570767 —
commands/auth.go:280: The user is asked a browser prompt that cannot open a browser. —wslBrowserCommand()still only callsexec.LookPath, so with interop disabled and/mnt/cmounted the browser is still offered. The new PHPgetWslBrowser()has the same gap.
Verification
Url::hasDisplay()on WSL now needsgetWslBrowser()to find an opener. A Docker Desktop container, with no/mnt/cand no Windows PATH, therefore gets no display.getDefaultBrowser()checks for the WSL opener before falling back toxdg-open/gnome-open, and only whenisWsl()is true, so plain Linux behaves as before.openUrl()addsurl.dll,FileProtocolHandlerboth whenrundll32.exeis found on PATH and when the/mnt/cabsolute path is used, becausestr_ends_withmatches both.- Go
hasDisplay()now ANDsisWSL()withwslBrowserCommand() != nil, which matches the PHP condition.
The diff adds legacy/tests/Service/UrlTest.php, which mocks Shell::commandExists/execute to cover the rundll32, /mnt/c fallback, wslview and no-opener cases. It also adds OsUtilTest::testIsWsl, which tests env-var detection. No test covers a found opener that cannot execute (interop disabled), and real WSL behaviour still needs the manual check.
Review 3 of 10 for this pull request · View the full run
With WSL interop off, rundll32.exe still exists under /mnt/c, so the CLI offered "Log in via a browser?", but running it fails (exec format error). Require WSL's binfmt_misc interop entry (WSLInterop or WSLInterop-late) to be enabled before using wslview or rundll32.exe, in Go and in the legacy CLI. Checked on WSL1: the entry exists and is "enabled" with interop on, and is missing with interop off. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
WSL2 keeps the WSLInterop binfmt_misc entry enabled when wsl.conf sets [interop] enabled=false, so the CLI offered a browser login and then waited about 10 seconds for rundll32.exe to fail. Also check wsl.conf. So that the detection lives in one place, the Go wrapper passes the WSL opener to the legacy CLI in <PREFIX>WSL_BROWSER (always set, empty when there is none), and the PHP Url service uses it instead of its own WSL checks. Checked on Windows 11 with WSL1 (VirtualBox) and WSL2 (QEMU/KVM): with interop on, legacy commands offer the prompt and open Edge; with it off, there's no prompt and login prints the URL at once. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Note
Reviewed — No blocking findings · 🔵 1 minor point
🔁 Incremental · 8 files reviewed
🔵 Minor point
commands/root.go:348—makeLegacyCLIWrappernow callswslBrowser()for every legacy invocation. That includes_complete, which runs on every Tab press, pluslist,completion,initand the session-export migrator. Under WSL,wslBrowser()globs binfmt_misc and reads/etc/wsl.conf. It then callsexec.LookPath("wslview")andexec.LookPath("rundll32.exe"), which stat each PATH entry. With the defaultappendWindowsPath=true, PATH holds many/mnt/c/...directories. On WSL2 those are on the drvfs/9p mount, where each stat is slow. Whenwslviewis not installed, the whole PATH is scanned once for it and again for rundll32. Every legacy command and every completion request on WSL now pays this cost, even though only browser-opening commands needWSL_BROWSER. Computing it lazily, or caching it once per process, would avoid the cost.
Verification
- The Go wrapper appends
WSL_BROWSERafteros.Environ(), and Go's exec keeps the last duplicate. So a user-set value is replaced, as the comment says. Url's newConfigconstructor argument is autowired bylegacy/config/services.yaml(autowire: true), and nothing inlegacy/srcbuildsUrlby hand.- The PHP side reads
application.env_prefix, which is the same prefix the wrapper uses (cnf.Application.EnvPrefix) to setWSL_BROWSER. An empty value maps tofalsethrough?:. wslBrowser()returns onlyc[0].Url::openUrladdsurl.dll,FileProtocolHandlerback for any browser ending inrundll32.exe, so the rundll32 path still gets its argument.
New Go tests cover the wsl.conf parsing (TestWSLInteropEnabled) and wslBrowser() with fake binfmt and PATH stubs. The rewritten legacy/tests/Service/UrlTest.php covers the env-var handoff. The CI jobs make test and scripts/test/unit.sh in ci.yml run these tests. Nothing tests real WSL, and no test covers the WSL_BROWSER env injection in CLIWrapper.Exec.
Review 5 of 10 for this pull request · View the full run
The wrapper looks up the WSL opener for every legacy command. In WSL2, PATH includes Windows directories on a slow mount: searching it for wslview (usually missing) and rundll32.exe took about 70ms, against 6ms for checking /mnt/c/Windows/System32/rundll32.exe. Check that path first, fall back to PATH, and drop wslview, which rundll32.exe makes unneeded. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Note
Reviewed — No new issues found · 1 still open
🔁 Incremental · 3 files reviewed
Outstanding from earlier reviews:
- 🔵
commands/root.go:348: Shell completion and every legacy command get slower on WSL2. — makeLegacyCLIWrapper still calls wslBrowser() on every legacy run, including _complete. Trying System32 first skips the PATH scan only when /mnt/c holds rundll32.exe; without it, the binfmt glob, the wsl.conf read and the full PATH scan still happen on every run. (first raised)
Verification
wslBrowserCommandreturns before anyLookPathwhenwslInteropEnabled()is false, so turning interop off now skips the System32 stat as well as the PATH scan.- Every test that calls
setWSLInteropnow pointswslRundll32Pathat a temp file and restores it int.Cleanup, so a real/mnt/con the host can no longer change the test results. - Removing the
wslviewbranch leaves nothing broken: legacyUrl::openUrladdsurl.dll,FileProtocolHandlerfor any browser path ending inrundll32.exe, which is the only kind the Go side now passes. - In
TestBrowserCommand_WSL,rundll32.exeexists both in System32 and on PATH, and the test asserts the System32 one wins, which pins the new lookup order.
The Go behaviour is covered by tests in this change: the TestBrowserCommand_WSL* tests, a new PATH-fallback test, and TestWSLInteropEnabled, all with faked binfmt, wsl.conf and System32 paths. On the PHP side the change only removes the wslview case from UrlTest. The project runs these tests with make test (Go) and PHPUnit (legacy). As the PR description says, nothing in CI runs on real WSL.
Review 6 of 10 for this pull request · View the full run
|
Re the minor point on
🤖 Replied by Claude Code |
Stacked on #203 (Go auth, behind
<PREFIX>GO_AUTH=1).On WSL there is usually no
DISPLAYand noxdg-open, so the "Log in via a browser?" prompt was not offered, andloginprinted the URL instead of opening it. Under WSL (detected fromWSL_DISTRO_NAME/WSL_INTEROPor the kernel release), if Windows interop is enabled (WSL'sWSLInteropbinfmt_misc entry, and not disabled in/etc/wsl.conf, which WSL2 needs), the CLI now counts as having a display, and opens URLs with Windows'rundll32.exe url.dll,FileProtocolHandler(as on Windows), from/mnt/c/Windows/System32or elsePATH(searchingPATHis slow in WSL2). The local login server's 127.0.0.1 address works from Windows, as WSL forwards localhost.The Go wrapper passes the opener it finds to the legacy CLI in
<PREFIX>WSL_BROWSER, so legacy commands also offer the prompt in WSL (the legacy CLI decides whether to offer it, with or without Go auth), and the PHP login opens the URL too.Checked manually on Windows 11 with WSL1 and WSL2 (Ubuntu 24.04), as CI has no WSL.
loginopens the URL in Edge, which reaches the login page, withrundll32.exefrom/mnt/c/Windows/System32and fromPATH.project:list) offers "Log in via a browser?" and opens the URL, with and without Go auth.wsl.conf, there is no prompt andloginprints the URL at once.🤖 Generated with Claude Code