Explain unreachable loopback server in the Add machine dialog - #1695
Explain unreachable loopback server in the Add machine dialog#1695SawyerHood wants to merge 2 commits into
Conversation
Fixes #1690 Co-Authored-By: Claude <noreply@anthropic.com>
|
🚨 SLOP COP 🚨 · I am SlopCop. I am reviewing this pull request for security, code quality, performance, architecture, and end-to-end behavior. |
![bb-slop-cop[bot]](https://avatars.githubusercontent.com/in/4577281?s=60&v=4){kind=small}
|
|
||
| function UnreachableServerNotice({ serverUrl }: { serverUrl: string }) { | ||
| return ( | ||
| <div className="space-y-2 rounded-md border border-border bg-muted/40 p-3"> |
There was a problem hiding this comment.
🚨 slopcop/review — Announce the asynchronous result to screen readers
The dialog replaces “Creating a join code…” with this notice. Focus stays on the Done button.
Because the notice has no live region, a screen reader does not announce the result or the required action.
Add a polite status message to the noninteractive text. Add an accessibility assertion to the test.
There was a problem hiding this comment.
Fixed in 8d60b0c: the notice is a role="status" polite live region and the test queries it by role.
| </Button> | ||
| </div> | ||
| ) : unreachableServerUrl !== null ? ( | ||
| <UnreachableServerNotice serverUrl={unreachableServerUrl} /> |
There was a problem hiding this comment.
🚨 slopcop/review — Keep a retry path for a temporary Connect failure
createConnectMachineCode converts an HTTP 503 into null. This successful mutation now selects this notice for a loopback URL.
The notice has no retry action. A temporary plugin start failure therefore looks permanent until the user closes and reopens the dialog.
Treat only a confirmed not_paired response as this state. Show the normal error and retry action for a 503.
There was a problem hiding this comment.
Fixed in 8d60b0c: createConnectMachineCode now returns issued | unpaired | unavailable. A 503/422 with a local-only URL shows an error with the existing "Try again" action; only not_paired/404 selects the notice. Test added.
| */ | ||
| export function isLoopbackServerUrl(serverUrl: string): boolean { | ||
| try { | ||
| return isLoopbackHostname(new URL(serverUrl).hostname); |
There was a problem hiding this comment.
🚨 slopcop/review — Block non-routable local destinations before the installer command
This check accepts 0.0.0.0 and IPv4-mapped IPv6 loopback addresses. The launcher advertises 0.0.0.0 for wildcard binding.
On the new machine, curl can resolve these addresses to that same machine. It can then pipe an unrelated local /install.sh into sh.
Please reject unspecified and mapped-loopback destinations. Add cases for 0.0.0.0 and ::ffff:127.0.0.1.
There was a problem hiding this comment.
Fixed in 8d60b0c: added a shared isLocalOnlyUrl (loopback, 0.0.0.0, ::, dotted and hex IPv4-mapped loopback) with tests, and reused it in BrowserTabContent.
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Plain English summary: This change hides an installer command that another computer cannot use. It explains the problem and links to Remote access.
I found four issues that need fixes before merge. I posted each issue on the related line.
- The URL check accepts
0.0.0.0and mapped IPv6 loopback addresses. The generatedcurl | shcommand can contact the new machine itself. - The notice says the server is local-only. The browser URL cannot prove that claim when the server listens on
0.0.0.0. - A temporary Connect 503 becomes a permanent notice with no retry action.
- The new asynchronous notice has no live status announcement for screen readers.
I also found three low-priority cleanup items. The dialog still says “Run this” when no command exists. The hidden countdown still causes one render each second. The new URL parser duplicates BrowserTabContent and has an unused export.
The architecture otherwise uses the existing route, button, dialog, and hostname helpers. A shared isLoopbackUrl helper can remove the parser duplication.
Validation passed:
- The full
@bb/appTurbo test passed with 348 files and 2,769 tests. - The
@bb/appTurbo typecheck passed. - Browser QA confirmed the notice, Remote access link, dialog cleanup, light and dark layouts, and the compact drawer.
- Browser QA found no new React error. The worker stopped the development server and browser.
The final GPT-5.6-Sol review gate confirmed all seven findings. It returned REQUEST CHANGES. This GitHub review uses comment-only status, as required.
| return ( | ||
| <div className="space-y-2 rounded-md border border-border bg-muted/40 p-3"> | ||
| <p className="text-sm text-foreground"> | ||
| This bb is only reachable from this computer. |
There was a problem hiding this comment.
🚨 slopcop/review — Do not infer the listener exposure from the browser URL
The system config derives serverUrl from the request origin. It does not report the listener address.
A server can listen on 0.0.0.0 while this browser uses 127.0.0.1. The public API then remains reachable across the network.
This message gives false safety information. Describe only the URL limitation, or use the real bind address before you describe server exposure.
There was a problem hiding this comment.
Fixed in 8d60b0c: the notice now describes only the URL limitation ("Another machine cannot use this address" / "points to the machine that runs it") and makes no claim about the listener.
- Share isLocalOnlyUrl (loopback, 0.0.0.0, ::, mapped loopback) and reuse it in BrowserTabContent. - Distinguish unpaired from temporarily unavailable connect; offer a retry instead of a command that dials the new machine itself. - Notice describes the URL limitation only and is a polite live region. - Only tick the countdown while a command is on screen. Co-Authored-By: Claude <noreply@anthropic.com>
|
Addressed all four inline findings plus the three cleanup items in 8d60b0c: the dialog description changes when no command is shown, the countdown only ticks while a command is on screen, and the URL parser moved to a shared |
1 participant
Fixes #1690
Problem
A fresh desktop install listens on
http://127.0.0.1:38886. When bb connect is not paired, the Add machine dialog silently falls back to that direct URL and prints acurlcommand that no other machine can reach.Change
AddMachineDialognow detects the unpaired + loopback case (isLoopbackHostname) and shows a notice with a link to Settings → Remote access instead of an unusable command. The "Waiting for the machine to connect…" box is hidden in that state.BB_APP_URL, wildcard bind) keep the existing direct command.MemoryRouter.docs/multiple-devices.mdmentions the new behavior.Test plan
pnpm exec turbo run test --filter=@bb/app -- src/components/dialogs/AddMachineDialog.test.tsxpnpm exec turbo run typecheck --filter=@bb/app