diff --git a/.gitignore b/.gitignore index f09b4e3e83f..6db3e0646c8 100644 --- a/.gitignore +++ b/.gitignore @@ -140,5 +140,8 @@ tmp/ **/.qwen/computer-use/ .playwright-mcp/ +# Tool state written to the repo root when $HOME is unset (e.g. gh CLI) +.local/ + # Brand build workspaces (created by the desktop-brand-builder skill) brand-builds/ diff --git a/docs/developers/qwen-serve-protocol.md b/docs/developers/qwen-serve-protocol.md index 4147504603f..2861be6aa1b 100644 --- a/docs/developers/qwen-serve-protocol.md +++ b/docs/developers/qwen-serve-protocol.md @@ -510,6 +510,7 @@ operator diagnostic snapshot documented below. | `persistent_workspace_registration` | a workspace registration store is wired into the daemon. Production `runQwenServe` supplies the user-level store automatically; direct `createServeApp` embeds must inject one explicitly and own startup restoration of their workspace registry. | | `scratch_workspace_registration` | managed scratch workspace creation is available — a runtime factory, a validated managed scratch root, and runtime disposal are wired, and every managed runtime respects the scratch root boundary. | | `workspace_runtime_removal` | removable dynamic or persistence-restored secondary runtimes can be drained and removed through the management route. | +| `native_directory_picker` | the daemon host can open a native OS directory picker (`osascript` on macOS, PowerShell on Windows, `zenity` on a Linux host with a display). Headless hosts omit the tag so clients hide the Browse affordance instead of surfacing a guaranteed picker failure. | | `workspace_qualified_acp` | ACP HTTP and multi-workspace runtimes are active, so the plural ACP endpoint can select a secondary runtime. | | `workspace_qualified_voice` | multi-workspace runtimes and the shared ACP/Voice WebSocket listener are active, so every workspace-qualified Voice modality is reachable for a secondary runtime. | | `workspace_qualified_memory` | ACP HTTP and multi-workspace runtimes are active, so workspace-qualified managed-memory routes can select a per-workspace task lane for remember, forget, and dream operations. | diff --git a/docs/users/qwen-serve.md b/docs/users/qwen-serve.md index 7a492021f39..2f5f8a1ad64 100644 --- a/docs/users/qwen-serve.md +++ b/docs/users/qwen-serve.md @@ -397,6 +397,7 @@ Notes: - **TLS is orthogonal to auth** — HTTPS encrypts the transport; the bearer token still gates every API route. Non-loopback binds require a token with or without TLS. - **Scope is TLS termination only** — no auto-generation, no ACME / Let's Encrypt. This is a LAN / dev convenience; for internet-facing deployments terminate TLS at a reverse proxy (see the threat model below). - **Channel workers dial the daemon back over `https://`** — so they need to trust the serving certificate too. A self-signed cert (or a fullchain that carries its own root) needs nothing: the daemon injects it into each worker's `NODE_EXTRA_CA_CERTS`. The mkcert flow above is **CA-issued**, so the leaf alone cannot anchor the chain — export `NODE_EXTRA_CA_CERTS="$(mkcert -CAROOT)/rootCA.pem"` in the daemon's launch environment before starting with `--channel`. An operator-set value is _merged_ with the daemon cert, not replaced. Without it the daemon boots green while every channel worker restart-loops on `UNABLE_TO_VERIFY_LEAF_SIGNATURE`; the daemon log names the gap at boot. +- **An IPv6 wildcard bind is dialled back on the loopback this host actually assigns** — `--hostname ::` (or `[::]`) binds an IPv6 socket, and an empty `--hostname` binds one too when IPv6 is available (when it is not, Node falls back to binding `0.0.0.0`). That socket is dual-stack (Node pins `IPV6_V6ONLY=0` on it, so the `net.ipv6.bindv6only` sysctl does not change this), and both loopbacks usually reach it — but a host with no IPv4 at all has only `::1`, while a host that binds `::` yet carries no `::1` on its loopback (for example `net.ipv6.conf.lo.disable_ipv6=1`) has only `127.0.0.1`. Workers are sent to `[::1]` when this host assigns it and to `127.0.0.1` otherwise. A serving certificate for an IPv6 wildcard bind should carry both loopbacks in its SANs (`mkcert localhost 127.0.0.1 ::1` covers it); the boot trust diagnostic inspects the exact URL workers will dial and names the gap. `--hostname 0.0.0.0` is unchanged and still needs `127.0.0.1`. - **Rotating `--tls-cert` in place needs a daemon restart** — the daemon serves the bytes it read at boot, so until it restarts, respawned workers can load newer contents than the daemon presents and their handshakes fail. ## CLI flags diff --git a/integration-tests/cli/qwen-serve-routes.test.ts b/integration-tests/cli/qwen-serve-routes.test.ts index bd99a960160..5cdc86c2880 100644 --- a/integration-tests/cli/qwen-serve-routes.test.ts +++ b/integration-tests/cli/qwen-serve-routes.test.ts @@ -40,6 +40,7 @@ import { Storage, type ChatRecord, } from '@qwen-code/qwen-code-core'; +import { isNativeDirectoryPickerAvailable } from '../../packages/cli/src/serve/native-directory-picker.js'; const __dirname = path.dirname(fileURLToPath(import.meta.url)); // Match the rest of the integration suite: prefer the bundled CLI @@ -293,6 +294,9 @@ describe('qwen serve — capabilities envelope', () => { // `require_auth`, `allow_origin`, `cdp_tunnel_over_ws`, // `prompt_absolute_deadline`, `writer_idle_timeout`, // `workspace_voice_transcription`, `rate_limit`, `channel_reload`. + // `native_directory_picker` is host-conditional (the daemon host's GUI + // environment, not a spawn flag) and is spliced at its registry + // position below. // Pool tags (`mcp_workspace_pool`, `mcp_pool_restart`) ARE present // because the workspace MCP pool is on by default, as are // `workspace_settings`, `workspace_permissions`, `workspace_voice`, @@ -410,6 +414,9 @@ describe('qwen serve — capabilities envelope', () => { 'persistent_workspace_registration', 'workspace_display_name', 'workspace_runtime_removal', + ...(isNativeDirectoryPickerAvailable() + ? ['native_directory_picker'] + : []), 'workspace_qualified_rest_core', 'extension_management_v2', 'extension_git_credentials', diff --git a/packages/cli/src/commands/channel/daemon-worker.test.ts b/packages/cli/src/commands/channel/daemon-worker.test.ts index 546884fcc96..9fd1dbfbfc6 100644 --- a/packages/cli/src/commands/channel/daemon-worker.test.ts +++ b/packages/cli/src/commands/channel/daemon-worker.test.ts @@ -1,4 +1,5 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import * as os from 'node:os'; const mockCanonicalizeWorkspace = vi.hoisted(() => vi.fn((p: string) => p)); const mockLoadChannelsConfig = vi.hoisted(() => vi.fn()); @@ -205,6 +206,21 @@ const mockSessionRouter = vi.hoisted(() => ), ); +const mockNetworkInterfaces = vi.hoisted(() => ({ + value: undefined as NodeJS.Dict | undefined, +})); + +vi.mock('node:os', async (importOriginal) => { + const actual = await importOriginal(); + const networkInterfaces = () => + mockNetworkInterfaces.value ?? actual.networkInterfaces(); + return { + ...actual, + networkInterfaces, + default: { ...actual, networkInterfaces }, + }; +}); + vi.mock('@qwen-code/acp-bridge/workspacePaths', () => ({ canonicalizeWorkspace: mockCanonicalizeWorkspace, })); @@ -278,6 +294,7 @@ import { daemonWorkerCommand, runChannelDaemonWorker, } from './daemon-worker.js'; +import { isOwnInterfaceAddress } from '../../serve/local-bind-addresses.js'; const parsedTelegram = { name: 'telegram', @@ -1442,7 +1459,7 @@ describe('runChannelDaemonWorker', () => { ).rejects.toThrow('Channel "missing" not found in settings.'); }); - it('rejects daemon URLs that are not http(s) loopback URLs', async () => { + it('rejects daemon URLs that name no address on this host', async () => { const sdk = createSdk(); for (const daemonUrl of [ @@ -1456,7 +1473,9 @@ describe('runChannelDaemonWorker', () => { selection: { mode: 'names', names: ['telegram'] }, loadDaemonSdk: async () => sdk, }), - ).rejects.toThrow('QWEN_DAEMON_URL must use an http(s) loopback URL.'); + ).rejects.toThrow( + "QWEN_DAEMON_URL must use an http(s) loopback URL or a literal address of one of this machine's interfaces.", + ); } expect(sdk.DaemonClient).not.toHaveBeenCalled(); }); @@ -1505,6 +1524,155 @@ describe('runChannelDaemonWorker', () => { }); }); + // R2-4/R15-1: a daemon bound to a concrete interface listens on that + // socket only — loopback is NOT bound, so rewriting the worker URL to + // `127.0.0.1` would trade this validator's rejection for `ECONNREFUSED`. + // The worker dials the bound address instead, and an own-interface address + // keeps the daemon token on this host exactly as loopback does — the + // property the rule protects. Without this widening the documented LAN + // flow (`qwen serve --hostname --channel …`) passes every boot + // check (`assertChannelWorkerDaemonUrlIsLocal` certifies the bind) and + // then throws in every worker: the first one exits the daemon, later ones + // restart-loop with /health green. + it("accepts a daemon URL bound to one of this host's own interfaces", async () => { + const ownAddress = Object.values(os.networkInterfaces()) + .flatMap((entries) => entries ?? []) + .find((entry) => entry.family === 'IPv4' && !entry.internal)?.address; + // A machine with no non-loopback IPv4 interface cannot exercise this. + if (!ownAddress) return; + + const sdk = createSdk(); + mockLoadChannelsConfig.mockReturnValueOnce({ + telegram: { type: 'telegram' }, + }); + mockParseConfiguredChannels.mockResolvedValueOnce([parsedTelegram]); + + await runChannelDaemonWorker({ + daemonUrl: `https://${ownAddress}:4170`, + workspace: '/workspace', + selection: { mode: 'all' }, + loadDaemonSdk: async () => sdk, + }); + + expect(sdk.DaemonClient).toHaveBeenCalledWith({ + baseUrl: `https://${ownAddress}:4170`, + }); + }); + + // The widening is to THIS host's addresses, not to routable addresses in + // general: a literal that belongs to no local interface stays rejected, so + // the daemon token still cannot be aimed off-box. + it('still rejects a routable address that is not on this host', async () => { + const sdk = createSdk(); + + await expect( + runChannelDaemonWorker({ + daemonUrl: 'https://203.0.113.7:4170', + workspace: '/workspace', + selection: { mode: 'names', names: ['telegram'] }, + loadDaemonSdk: async () => sdk, + }), + ).rejects.toThrow( + "QWEN_DAEMON_URL must use an http(s) loopback URL or a literal address of one of this machine's interfaces.", + ); + expect(sdk.DaemonClient).not.toHaveBeenCalled(); + }); + + // R18-1: mirrors the boot certifier — the primary Host gate answers only + // 127.0.0.1, localhost, and [::1], so a worker aimed at any other 127/8 + // spelling is refused by the daemon itself with 403 Invalid Host header. + // Reject it here too instead of letting it restart-loop. + it('rejects loopback spellings the daemon Host gate refuses', async () => { + const sdk = createSdk(); + + await expect( + runChannelDaemonWorker({ + daemonUrl: 'http://127.0.0.2:4170', + workspace: '/workspace', + selection: { mode: 'names', names: ['telegram'] }, + loadDaemonSdk: async () => sdk, + }), + ).rejects.toThrow( + /points at a loopback address the daemon's Host header gate refuses/, + ); + expect(sdk.DaemonClient).not.toHaveBeenCalled(); + }); + + // The refusal above is host-state-dependent: on a host that ASSIGNS the + // wide spelling (`ip addr add 127.0.0.2/8 dev lo`, a standard + // container-mesh pattern), the own-interface escape used to accept the URL + // and every worker dial then got `403 Invalid Host header`. Pin the + // assigned state so the refusal is witnessed on every host. + it('rejects an assigned wide loopback the Host gate answers 403', async () => { + const sdk = createSdk(); + mockNetworkInterfaces.value = { + lo: [ + { + address: '127.0.0.1', + netmask: '255.0.0.0', + family: 'IPv4', + mac: '00:00:00:00:00:00', + internal: true, + cidr: '127.0.0.1/8', + }, + { + address: '127.0.0.2', + netmask: '255.0.0.0', + family: 'IPv4', + mac: '00:00:00:00:00:00', + internal: true, + cidr: '127.0.0.2/8', + }, + { + address: '::1', + netmask: 'ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff', + family: 'IPv6', + mac: '00:00:00:00:00:00', + internal: true, + cidr: '::1/128', + scopeid: 0, + }, + ], + }; + try { + // Witness the assigned state: without this assert a broken mock would + // let the rejection below pass for the wrong (unassigned) reason. + expect(isOwnInterfaceAddress('127.0.0.2')).toBe(true); + await expect( + runChannelDaemonWorker({ + daemonUrl: 'http://127.0.0.2:4170', + workspace: '/workspace', + selection: { mode: 'names', names: ['telegram'] }, + loadDaemonSdk: async () => sdk, + }), + ).rejects.toThrow( + /points at a loopback address the daemon's Host header gate refuses/, + ); + expect(sdk.DaemonClient).not.toHaveBeenCalled(); + } finally { + mockNetworkInterfaces.value = undefined; + } + }); + + it('still accepts the loopback spellings the Host gate answers', async () => { + const sdk = createSdk(); + mockLoadChannelsConfig.mockReturnValueOnce({ + telegram: { type: 'telegram' }, + }); + mockParseConfiguredChannels.mockResolvedValueOnce([parsedTelegram]); + + await runChannelDaemonWorker({ + daemonUrl: 'http://localhost:4170', + workspace: '/workspace', + selection: { mode: 'all' }, + loadDaemonSdk: async () => sdk, + }); + + expect(sdk.DaemonClient).toHaveBeenCalledWith({ + baseUrl: 'http://localhost:4170', + }); + }); + it('fails fast when no channels are configured', async () => { const sdk = createSdk(); mockLoadChannelsConfig.mockReturnValueOnce({}); diff --git a/packages/cli/src/commands/channel/daemon-worker.ts b/packages/cli/src/commands/channel/daemon-worker.ts index 794cc6164d7..190a678511e 100644 --- a/packages/cli/src/commands/channel/daemon-worker.ts +++ b/packages/cli/src/commands/channel/daemon-worker.ts @@ -64,7 +64,11 @@ import { MAX_CHANNEL_STARTUP_FAILURE_MESSAGE_LENGTH, type ChannelStartupReportMessage, } from '../../serve/channel-worker-startup-ipc.js'; -import { isLoopbackBind } from '../../serve/loopback-binds.js'; +import { + isHostGateLoopback, + isLoopbackBind, +} from '../../serve/loopback-binds.js'; +import { isOwnInterfaceAddress } from '../../serve/local-bind-addresses.js'; import { ChannelLoopMcpWorkerHost } from '../../serve/channel-loop-mcp-ipc.js'; import { writeStderrLine, writeStdoutLine } from '../../utils/stdioHelpers.js'; import { resolveProxyUrl } from './proxy.js'; @@ -325,11 +329,35 @@ function validateDaemonWorkerUrl(daemonUrl: string): void { } catch { throw new Error(`${QWEN_DAEMON_URL_ENV} must be a valid URL.`); } - if ( - (parsed.protocol !== 'http:' && parsed.protocol !== 'https:') || - !isLoopbackBind(parsed.hostname) - ) { - throw new Error(`${QWEN_DAEMON_URL_ENV} must use an http(s) loopback URL.`); + // A daemon bound to a concrete interface (`--hostname 192.168.1.100`) + // listens on that socket ONLY — loopback is not bound, so rewriting the + // URL to `127.0.0.1` would trade this rejection for `ECONNREFUSED`. The + // worker dials the bound address itself, and an own-interface address + // keeps the daemon token on this host exactly as loopback does, which is + // the property this rule protects; anything else (a routable third-party + // host, a DNS name we would have to resolve to find out) stays refused. + if (parsed.protocol !== 'http:' && parsed.protocol !== 'https:') { + throw new Error( + `${QWEN_DAEMON_URL_ENV} must use an http(s) loopback URL or a ` + + `literal address of one of this machine's interfaces.`, + ); + } + if (isHostGateLoopback(parsed.hostname)) return; + if (isLoopbackBind(parsed.hostname)) { + // Order matters: this refusal must run BEFORE the own-interface escape + // below — a wide 127/8 address can be assigned to a local interface + // (`ip addr add 127.0.0.2/8 dev lo`), and the gate 403s it either way. + throw new Error( + `${QWEN_DAEMON_URL_ENV} points at a loopback address the daemon's ` + + `Host header gate refuses (it answers only 127.0.0.1, localhost, ` + + `and [::1]); use one of those spellings instead.`, + ); + } + if (!isOwnInterfaceAddress(parsed.hostname)) { + throw new Error( + `${QWEN_DAEMON_URL_ENV} must use an http(s) loopback URL or a ` + + `literal address of one of this machine's interfaces.`, + ); } } diff --git a/packages/cli/src/serve/capabilities.ts b/packages/cli/src/serve/capabilities.ts index b35c88db836..2ac5687ced2 100644 --- a/packages/cli/src/serve/capabilities.ts +++ b/packages/cli/src/serve/capabilities.ts @@ -357,6 +357,11 @@ export const SERVE_CAPABILITY_REGISTRY = { workspace_display_name: { since: 'v1' }, scratch_workspace_registration: { since: 'v1' }, workspace_runtime_removal: { since: 'v1' }, + // A native OS directory picker can be opened on the daemon host + // (osascript on macOS, PowerShell on Windows, zenity on a Linux host + // with a display). Headless hosts omit the tag so clients hide the + // Browse affordance instead of surfacing a guaranteed picker failure. + native_directory_picker: { since: 'v1' }, // Workspace-qualified core REST routes under `/workspaces/:workspace/...`. // Covers core file read/write/upload, status/permissions/trust/lifecycle/MCP/tool, // memory, workspace agent CRUD, and persisted session organization surfaces. @@ -501,6 +506,7 @@ export interface AdvertiseFeatureToggles { persistentWorkspaceRegistrationAvailable?: boolean; scratchWorkspaceRegistrationAvailable?: boolean; workspaceRuntimeRemovalAvailable?: boolean; + nativeDirectoryPickerAvailable?: boolean; /** * Whether the HTTP ACP surface is enabled (default on; opts out via * QWEN_SERVE_ACP_HTTP=0). Workspace-qualified ACP is only advertised when on. @@ -639,6 +645,10 @@ export const CONDITIONAL_SERVE_FEATURES: ReadonlyMap< 'workspace_runtime_removal', (toggles) => toggles.workspaceRuntimeRemovalAvailable === true, ], + [ + 'native_directory_picker', + (toggles) => toggles.nativeDirectoryPickerAvailable === true, + ], [ 'workspace_qualified_acp', // The plural routes are pre-mounted for workspaces registered after app diff --git a/packages/cli/src/serve/local-bind-addresses.test.ts b/packages/cli/src/serve/local-bind-addresses.test.ts new file mode 100644 index 00000000000..bc02499f167 --- /dev/null +++ b/packages/cli/src/serve/local-bind-addresses.test.ts @@ -0,0 +1,152 @@ +/** + * @license + * Copyright 2025 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, expect, it } from 'vitest'; +import { networkInterfaces, type NetworkInterfaceInfo } from 'node:os'; +import { + hostAssignsIpv6Loopback, + isOwnInterfaceAddress, +} from './local-bind-addresses.js'; + +/** + * `isOwnInterfaceAddress` is reached only through + * `assertChannelWorkerDaemonUrlIsLocal` and `validateDaemonWorkerUrl`, and in + * both of those every bracketed literal the suites feed short-circuits on + * `isLoopbackBind` first (`[::1]` is in `LOOPBACK_BINDS`, and the wildcard + * binds are rewritten to `127.0.0.1` before the assertion sees them). So the + * normalisation this function does on the way to `os.networkInterfaces()` — + * which reports BARE, lowercase addresses and carries the scope separately in + * `scopeid` — was exercised by nothing, and deleting it shipped green. + * + * What a lost normalisation step costs: `qwen serve --hostname + * --tls-cert … --channel telegram` binds fine and then refuses its own bind + * with "does not name an address on this host … Bind to … a literal address of + * one of this machine's interfaces" — at boot, and again in + * `validateDaemonWorkerUrl` for every hand-launched worker. + * + * Driven off the host's real interfaces rather than a mock: `node:os` is + * external to the module graph vitest transforms here, so a `vi.mock('node:os')` + * is visible to this file but NOT to the module under test — a mocked version + * of this suite passes against a deleted bracket strip. Every address below is + * one this machine actually answers on, so the assertions hold on an + * IPv4-only host too. + */ +describe('isOwnInterfaceAddress', () => { + const own = Object.values(networkInterfaces()) + .flatMap((entries) => entries ?? []) + .map((entry) => entry.address); + + it('reports at least one own address to test against', () => { + // Guards the loops below from passing vacuously on a host that somehow + // reports no interfaces at all. + expect(own.length).toBeGreaterThan(0); + }); + + it('accepts every own interface address in its bare form', () => { + for (const address of own) { + expect(isOwnInterfaceAddress(address)).toBe(true); + } + }); + + it('accepts an own address in the URL-bracketed form', () => { + // The bracketed spelling is what `workerDialHost` hands back out of a + // `https://[2001:db8::5]:8080` daemon URL, and what an operator passes to + // `--hostname`. `os.networkInterfaces()` never reports the brackets. + for (const address of own) { + expect(isOwnInterfaceAddress(`[${address}]`)).toBe(true); + } + }); + + it('strips an RFC 6874 zone identifier before matching', () => { + // A link-local bind is unusable without a zone, so a zone-carrying literal + // is the only form an operator can pass for one — and `networkInterfaces()` + // keeps the scope in `scopeid`, not in `address`. Both the percent-encoded + // URL spelling and the bare one have to survive. + for (const address of own) { + expect(isOwnInterfaceAddress(`${address}%eth0`)).toBe(true); + expect(isOwnInterfaceAddress(`[${address}%25eth0]`)).toBe(true); + } + }); + + it('matches an own address case-insensitively', () => { + // IPv6 literals are hex and an operator may type them uppercase, while + // `networkInterfaces()` reports them lowercase. + for (const address of own) { + expect(isOwnInterfaceAddress(address.toUpperCase())).toBe(true); + } + }); + + it('rejects a literal no interface holds', () => { + // RFC 5737 TEST-NET-3 and RFC 3849's documentation prefix: reserved for + // documentation, so no host is assigned one. + expect(own).not.toContain('203.0.113.255'); + expect(isOwnInterfaceAddress('203.0.113.255')).toBe(false); + expect(isOwnInterfaceAddress('[2001:db8::ffff]')).toBe(false); + }); + + it('rejects an empty or bracket-only hostname', () => { + // `[]` strips to the empty string, which must fail closed rather than + // match an interface that reports an empty address. + expect(isOwnInterfaceAddress('')).toBe(false); + expect(isOwnInterfaceAddress('[]')).toBe(false); + }); + + it('does not resolve DNS names, even ones that name this host', () => { + // Literals only, on purpose: resolving here would put a lookup — and + // whatever answers it — on every channel worker's startup path. + expect(isOwnInterfaceAddress('localhost')).toBe(false); + }); +}); + +describe('hostAssignsIpv6Loopback', () => { + const v4Loopback: NetworkInterfaceInfo = { + address: '127.0.0.1', + netmask: '255.0.0.0', + family: 'IPv4', + mac: '00:00:00:00:00:00', + internal: true, + cidr: '127.0.0.1/8', + }; + const v6Loopback: NetworkInterfaceInfo = { + address: '::1', + netmask: 'ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff', + family: 'IPv6', + mac: '00:00:00:00:00:00', + internal: true, + cidr: '::1/128', + scopeid: 0, + }; + // A global IPv6 address is not the loopback — a host that binds `::` with + // IPv6 only on non-loopback interfaces still has no `::1` to dial. + const v6Global: NetworkInterfaceInfo = { + address: '2001:db8::5', + netmask: 'ffff:ffff:ffff:ffff::', + family: 'IPv6', + mac: '00:00:00:00:00:00', + internal: false, + cidr: '2001:db8::5/64', + scopeid: 2, + }; + + it('reports true when the table assigns ::1', () => { + expect(hostAssignsIpv6Loopback({ lo: [v4Loopback, v6Loopback] })).toBe( + true, + ); + }); + + it('reports false when no entry is ::1', () => { + expect(hostAssignsIpv6Loopback({ lo: [v4Loopback] })).toBe(false); + expect(hostAssignsIpv6Loopback({ eth0: [v6Global] })).toBe(false); + expect(hostAssignsIpv6Loopback({})).toBe(false); + }); + + it('reads the live interface table when called with no argument', () => { + const live = Object.values(networkInterfaces()).some((entries) => + entries?.some((info) => info.family === 'IPv6' && info.address === '::1'), + ); + expect(hostAssignsIpv6Loopback()).toBe(live); + }); +}); diff --git a/packages/cli/src/serve/local-bind-addresses.ts b/packages/cli/src/serve/local-bind-addresses.ts new file mode 100644 index 00000000000..e0a9bb1b782 --- /dev/null +++ b/packages/cli/src/serve/local-bind-addresses.ts @@ -0,0 +1,77 @@ +/** + * @license + * Copyright 2025 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { networkInterfaces } from 'node:os'; + +/** + * Strip the URL brackets and the RFC 6874 zone identifier from an IPv6 + * literal so it can be compared against `os.networkInterfaces()`, which + * reports the bare address and carries the scope separately. Bracket + * stripping is load-bearing — `new URL(...).hostname` keeps brackets — but + * the zone handling is defensive only: production callers feed this from + * `new URL(...).hostname`, and WHATWG URL rejects zone IDs outright, so a + * zone never survives the parse layer to reach here. + */ +function bareAddress(hostname: string): string { + const unbracketed = + hostname.startsWith('[') && hostname.endsWith(']') + ? hostname.slice(1, -1) + : hostname; + const decoded = unbracketed.replace(/%25/gi, '%'); + const zoneAt = decoded.indexOf('%'); + return (zoneAt === -1 ? decoded : decoded.slice(0, zoneAt)).toLowerCase(); +} + +/** + * Whether `hostname` is an IP literal assigned to one of this machine's own + * interfaces — that is, an address the local host answers on. + * + * Channel workers are always spawned on the daemon's own machine, but a + * daemon bound to a concrete interface (`--hostname 192.168.1.100`) listens + * on that socket ONLY: loopback is not bound, so rewriting the worker's URL + * to `127.0.0.1` would trade a rejected URL for `ECONNREFUSED`. The worker + * therefore dials the address the daemon actually bound, and this is how it + * certifies that doing so keeps the daemon token on the local host — the + * property the loopback rule existed to guarantee. Traffic to an own + * interface address never reaches the wire; the kernel routes it back up the + * stack. + * + * Literals only, on purpose. Resolving a DNS name here would put a lookup — + * and whatever answers it — on the worker's startup path, so a non-literal + * bind is refused at boot instead (see the channel bind guard in + * `run-qwen-serve`), which fails loudly once rather than restart-looping + * every worker. + */ +export function isOwnInterfaceAddress(hostname: string): boolean { + const target = bareAddress(hostname); + if (target === '') return false; + for (const entries of Object.values(networkInterfaces())) { + for (const entry of entries ?? []) { + if (entry.address.toLowerCase() === target) return true; + } + } + return false; +} + +/** + * Whether this host has the IPv6 loopback (`::1`) assigned on any + * interface. A wildcard `::` listener is always dual-stack under Node + * (libuv pins `IPV6_V6ONLY=0` unless `ipv6Only` is requested), so such a + * daemon usually answers on BOTH loopbacks — but an IPv4-less host has only + * `::1`, and a host that binds `::` while its loopback carries no `::1` + * (e.g. `net.ipv6.conf.lo.disable_ipv6=1`) has only `127.0.0.1`. Channel + * workers must dial whichever one actually exists. + */ +export function hostAssignsIpv6Loopback( + interfaces = networkInterfaces(), +): boolean { + for (const entries of Object.values(interfaces)) { + for (const entry of entries ?? []) { + if (entry.family === 'IPv6' && entry.address === '::1') return true; + } + } + return false; +} diff --git a/packages/cli/src/serve/loopback-binds.ts b/packages/cli/src/serve/loopback-binds.ts index 1bd6518d7e4..bc990775182 100644 --- a/packages/cli/src/serve/loopback-binds.ts +++ b/packages/cli/src/serve/loopback-binds.ts @@ -47,3 +47,16 @@ export function isLoopbackBind(hostname: string): boolean { const normalized = hostname.toLowerCase(); return LOOPBACK_BINDS.has(normalized) || isIpv4Loopback(normalized); } + +/** + * The loopback spellings the primary Host gate (auth.ts) answers when the + * daemon binds loopback — its allowlist carries exactly these names (plus + * `host.docker.internal`, which is never a bind spelling). `isLoopbackBind` + * is wider because the kernel routes all of 127/8 to loopback, but a worker + * dialing any other 127.x.y.z gets `403 Invalid Host header` from the very + * daemon it is trying to reach, so the worker-URL validators must certify + * this narrower set. + */ +export function isHostGateLoopback(hostname: string): boolean { + return LOOPBACK_BINDS.has(hostname.toLowerCase()); +} diff --git a/packages/cli/src/serve/native-directory-picker.test.ts b/packages/cli/src/serve/native-directory-picker.test.ts index 2b0bec1676a..65e6c3c38f3 100644 --- a/packages/cli/src/serve/native-directory-picker.test.ts +++ b/packages/cli/src/serve/native-directory-picker.test.ts @@ -4,6 +4,9 @@ * SPDX-License-Identifier: Apache-2.0 */ +import { chmodSync, mkdirSync, mkdtempSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; const { execFileAsyncMock } = vi.hoisted(() => ({ @@ -22,8 +25,21 @@ vi.mock('node:child_process', async (importOriginal) => { }; }); -const { pickNativeDirectory, NativeDirectoryPickerUnavailableError } = - await import('./native-directory-picker.js'); +const { + pickNativeDirectory, + isNativeDirectoryPickerAvailable, + NativeDirectoryPickerUnavailableError, +} = await import('./native-directory-picker.js'); + +const zenityDir = mkdtempSync(join(tmpdir(), 'picker-zenity-')); +writeFileSync(join(zenityDir, 'zenity'), '#!/bin/sh\nexit 0\n'); +chmodSync(join(zenityDir, 'zenity'), 0o755); +const emptyDir = mkdtempSync(join(tmpdir(), 'picker-empty-')); +const nonExecutableZenityDir = mkdtempSync(join(tmpdir(), 'picker-nonexec-')); +writeFileSync(join(nonExecutableZenityDir, 'zenity'), '#!/bin/sh\nexit 0\n'); +chmodSync(join(nonExecutableZenityDir, 'zenity'), 0o644); +const zenityDirectoryEntryDir = mkdtempSync(join(tmpdir(), 'picker-subdir-')); +mkdirSync(join(zenityDirectoryEntryDir, 'zenity')); function setPlatform(platform: NodeJS.Platform) { vi.spyOn(process, 'platform', 'get').mockReturnValue(platform); @@ -189,3 +205,94 @@ describe('pickNativeDirectory', () => { ); }); }); + +describe('isNativeDirectoryPickerAvailable', () => { + it('requires positive graphical-session evidence on macOS and Windows', () => { + setPlatform('darwin'); + expect( + isNativeDirectoryPickerAvailable({}, { processUid: 0, consoleUid: 501 }), + ).toBe(false); + expect( + isNativeDirectoryPickerAvailable( + {}, + { processUid: 501, consoleUid: 501 }, + ), + ).toBe(true); + expect( + isNativeDirectoryPickerAvailable( + { SSH_CONNECTION: 'remote' }, + { processUid: 501, consoleUid: 501 }, + ), + ).toBe(false); + setPlatform('win32'); + expect(isNativeDirectoryPickerAvailable({})).toBe(false); + expect(isNativeDirectoryPickerAvailable({ SESSIONNAME: 'Console' })).toBe( + true, + ); + expect(isNativeDirectoryPickerAvailable({ SESSIONNAME: 'Services' })).toBe( + false, + ); + }); + + it('is unavailable on unsupported platforms', () => { + setPlatform('aix'); + expect( + isNativeDirectoryPickerAvailable({ DISPLAY: ':0', PATH: zenityDir }), + ).toBe(false); + }); + + it('requires a display on Linux', () => { + setPlatform('linux'); + expect(isNativeDirectoryPickerAvailable({ PATH: zenityDir })).toBe(false); + }); + + it('requires an executable zenity on PATH on Linux', () => { + setPlatform('linux'); + expect( + isNativeDirectoryPickerAvailable({ DISPLAY: ':0', PATH: emptyDir }), + ).toBe(false); + expect( + isNativeDirectoryPickerAvailable({ DISPLAY: ':0', PATH: zenityDir }), + ).toBe(true); + expect( + isNativeDirectoryPickerAvailable({ + WAYLAND_DISPLAY: 'wayland-0', + PATH: zenityDir, + }), + ).toBe(true); + }); + + // Windows has no exec bit: libuv's fs__access ignores X_OK entirely, so + // fs.accessSync succeeds for any existing path and this probe returns true + // there. `process.platform` is mocked to 'linux' but the filesystem is the + // real host's, so the assertion cannot hold on a Windows runner — and + // ci.yml's merge-queue `test_windows` job collects this file. Same shape as + // packages/core/src/utils/shellContextEnv.test.ts:157. + it.skipIf(process.platform === 'win32')( + 'rejects a zenity without the executable bit on Linux', + () => { + setPlatform('linux'); + expect( + isNativeDirectoryPickerAvailable({ + DISPLAY: ':0', + PATH: nonExecutableZenityDir, + }), + ).toBe(false); + }, + ); + + it('rejects a directory named zenity on PATH on Linux', () => { + setPlatform('linux'); + expect( + isNativeDirectoryPickerAvailable({ + DISPLAY: ':0', + PATH: zenityDirectoryEntryDir, + }), + ).toBe(false); + }); + + it('requires PATH to be set on Linux', () => { + setPlatform('linux'); + expect(isNativeDirectoryPickerAvailable({ DISPLAY: ':0' })).toBe(false); + }); +}); diff --git a/packages/cli/src/serve/native-directory-picker.ts b/packages/cli/src/serve/native-directory-picker.ts index 715e0d2aa5e..3dca622a3fb 100644 --- a/packages/cli/src/serve/native-directory-picker.ts +++ b/packages/cli/src/serve/native-directory-picker.ts @@ -5,6 +5,8 @@ */ import { execFile } from 'node:child_process'; +import { accessSync, constants, statSync } from 'node:fs'; +import { delimiter, join } from 'node:path'; import { promisify } from 'node:util'; const execFileAsync = promisify(execFile); @@ -16,6 +18,63 @@ const PICKER_TIMEOUT_MS = 300_000; export class NativeDirectoryPickerUnavailableError extends Error {} +interface MacOsSessionUids { + readonly processUid?: number; + readonly consoleUid?: number; +} + +// Startup probe so `/capabilities` can omit the picker feature on headless +// hosts and clients hide the Browse affordance instead of surfacing a +// guaranteed `cannot open display` failure. +export function isNativeDirectoryPickerAvailable( + env: Readonly> = process.env, + macOsSessionUids = process.platform === 'darwin' + ? readMacOsSessionUids() + : undefined, +): boolean { + if (process.platform === 'darwin') { + return ( + macOsSessionUids?.processUid !== undefined && + macOsSessionUids.processUid > 0 && + macOsSessionUids.consoleUid === macOsSessionUids.processUid && + !env['SSH_CONNECTION'] && + !env['SSH_TTY'] + ); + } + if (process.platform === 'win32') { + const sessionName = env['SESSIONNAME']?.trim(); + return Boolean(sessionName && sessionName.toLowerCase() !== 'services'); + } + if (process.platform !== 'linux') return false; + if (!env['DISPLAY'] && !env['WAYLAND_DISPLAY']) return false; + return (env['PATH'] ?? '') + .split(delimiter) + .some((dir) => dir !== '' && isExecutableFile(join(dir, 'zenity'))); +} + +function readMacOsSessionUids(): MacOsSessionUids { + try { + return { + processUid: process.getuid?.(), + consoleUid: statSync('/dev/console').uid, + }; + } catch { + return {}; + } +} + +function isExecutableFile(file: string): boolean { + try { + // A directory passes an X_OK probe (search permission) but cannot be + // exec'd, so it must not count as an installed zenity. + if (!statSync(file).isFile()) return false; + accessSync(file, constants.X_OK); + return true; + } catch { + return false; + } +} + export async function pickNativeDirectory( signal?: AbortSignal, ): Promise { diff --git a/packages/cli/src/serve/process-env-guard.test.ts b/packages/cli/src/serve/process-env-guard.test.ts index d4b62692246..cf08a1024a8 100644 --- a/packages/cli/src/serve/process-env-guard.test.ts +++ b/packages/cli/src/serve/process-env-guard.test.ts @@ -150,6 +150,16 @@ const allowedProcessEnvAccesses = normalizeAllowances([ accesses: { 'key:QWEN_AUDIT_RAW_PATHS': 1 }, }, ], + [ + 'packages/cli/src/serve/native-directory-picker.ts', + { + reason: + 'Picker availability probes process-scoped host session state ' + + '(SSH markers, display server, Windows session name), so embedded ' + + 'callers may omit the environment argument.', + accesses: { whole: 1 }, + }, + ], [ 'packages/cli/src/serve/pem-certificate-blocks.ts', { diff --git a/packages/cli/src/serve/run-qwen-serve.test.ts b/packages/cli/src/serve/run-qwen-serve.test.ts index 0a12f73d85b..d0259147133 100644 --- a/packages/cli/src/serve/run-qwen-serve.test.ts +++ b/packages/cli/src/serve/run-qwen-serve.test.ts @@ -10,6 +10,7 @@ import * as fs from 'node:fs'; import { X509Certificate } from 'node:crypto'; import { createServer } from 'node:http'; import * as https from 'node:https'; +import * as net from 'node:net'; import type { AddressInfo } from 'node:net'; import * as tls from 'node:tls'; import { describe, it, expect, vi, afterEach, afterAll } from 'vitest'; @@ -17,6 +18,7 @@ import express from 'express'; import { createLazyBridgeProxy, extractContextFilename, + assertChannelWorkerDaemonUrlIsLocal, formatChannelWorkerDaemonUrl, describeWorkerTlsTrustGaps, InvalidPolicyConfigError, @@ -36,6 +38,7 @@ import { loadServeFastPathEnvironment } from './fast-path-settings.js'; import { loadEnvironment } from '../config/environment.js'; import { RUNTIME_STARTUP_CANCELLED_MESSAGE } from './runtime-startup-errors.js'; import { isLoopbackBind } from './loopback-binds.js'; +import { isOwnInterfaceAddress } from './local-bind-addresses.js'; import { ChannelDeliveryAuthorizationStore } from './channel-delivery-authorization.js'; import * as acpBridge from '@qwen-code/acp-bridge/bridge'; import { @@ -95,6 +98,7 @@ afterEach(() => { // try/finally cleanup would otherwise leak the figure into later // memory-budget tests. mockTotalMemBytes.value = undefined; + mockNetworkInterfaces.value = undefined; }); afterAll(() => { @@ -644,16 +648,22 @@ const mockChannelWorkerEnabledState = vi.hoisted(() => ({ const mockTotalMemBytes = vi.hoisted(() => ({ value: undefined as number | undefined, })); +const mockNetworkInterfaces = vi.hoisted(() => ({ + value: undefined as NodeJS.Dict | undefined, +})); vi.mock('node:os', async (importOriginal) => { const actual = await importOriginal(); // Mock both the named and the default export: consumers do // `import os from 'node:os'`, which a bare spread would leave unmocked. const totalmem = () => mockTotalMemBytes.value ?? actual.totalmem(); + const networkInterfaces = () => + mockNetworkInterfaces.value ?? actual.networkInterfaces(); return { ...actual, totalmem, - default: { ...actual, totalmem }, + networkInterfaces, + default: { ...actual, totalmem, networkInterfaces }, }; }); @@ -1214,9 +1224,41 @@ describe('subSessionConcurrencyCapsFromSettings', () => { }); }); +const dialLoopback = ( + host: string, + port: number, +): Promise<{ ok: boolean; code?: string }> => + new Promise((resolve) => { + const socket = net.connect({ host, port, autoSelectFamily: false }, () => { + socket.destroy(); + resolve({ ok: true }); + }); + socket.on('error', (err: NodeJS.ErrnoException) => { + socket.destroy(); + resolve({ ok: false, code: err.code }); + }); + socket.setTimeout(2000, () => { + socket.destroy(); + resolve({ ok: false, code: 'ETIMEDOUT' }); + }); + }); + +const listenOn = (options: net.ListenOptions): Promise => + new Promise((resolve, reject) => { + const server = net.createServer((connection) => connection.end()); + server.once('error', reject); + server.listen(options, () => resolve(server)); + }); + describe('formatChannelWorkerDaemonUrl', () => { - it.each(['', '0.0.0.0', '::', '[::]'])( - 'uses loopback when the daemon binds wildcard host %j', + it('uses IPv4 loopback for the IPv4 wildcard bind', () => { + expect(formatChannelWorkerDaemonUrl('0.0.0.0', 4170)).toBe( + 'http://127.0.0.1:4170', + ); + }); + + it.each(['0', '0.0'])( + 'canonicalizes IPv4 wildcard spelling %j before choosing loopback', (host) => { expect(formatChannelWorkerDaemonUrl(host, 4170)).toBe( 'http://127.0.0.1:4170', @@ -1224,6 +1266,159 @@ describe('formatChannelWorkerDaemonUrl', () => { }, ); + // R7-7: the IPv6 wildcard's dial-back loopback follows what the host + // ASSIGNS — `[::1]` when the host carries it (an IPv4-less host has no + // other loopback). Node keeps the bound socket dual-stack (libuv pins + // IPV6_V6ONLY=0), so the `net.ipv6.bindv6only` sysctl never changes this. + it.each(['::', '[::]'])( + 'uses IPv6 loopback for the IPv6 wildcard host %j when the host assigns ::1', + (host) => { + expect( + formatChannelWorkerDaemonUrl(host, 4170, false, undefined, true), + ).toBe('http://[::1]:4170'); + }, + ); + + // The other half of R7-7 (#9406): a host that binds `::` while its + // loopback carries no `::1` (e.g. `net.ipv6.conf.lo.disable_ipv6=1`) + // reaches the dual-stack socket only through `127.0.0.1` — the old + // spelling-based `[::1]` dialled an address nothing owned there, and the + // first worker's `fetch failed` exited the daemon. + it.each(['::', '[::]'])( + 'falls back to IPv4 loopback for the IPv6 wildcard host %j when the host carries no ::1', + (host) => { + expect( + formatChannelWorkerDaemonUrl(host, 4170, false, undefined, false), + ).toBe('http://127.0.0.1:4170'); + }, + ); + + // R10-1: `listen(port, '')` tries the IPv6 unspecified address first and + // falls back to binding `0.0.0.0` when IPv6 is unavailable, so an empty + // --hostname decides by the socket that actually bound, not by spelling — + // on the fallback host the old spelling-based rule handed workers `[::1]`, + // which nothing listened on, and the first worker's failure exited the + // daemon. Explicit `::`/`0.0.0.0` keep their spelling-based mapping: those + // binds fail loud when their family is unavailable. + it('uses IPv6 loopback for an empty hostname on an IPv6 socket when the host assigns ::1', () => { + expect(formatChannelWorkerDaemonUrl('', 4170, false, undefined, true)).toBe( + 'http://[::1]:4170', + ); + expect(formatChannelWorkerDaemonUrl('', 4170, false, 'IPv6', true)).toBe( + 'http://[::1]:4170', + ); + }); + + it('falls back to IPv4 loopback for an empty hostname on an IPv6 socket when the host carries no ::1', () => { + expect( + formatChannelWorkerDaemonUrl('', 4170, false, undefined, false), + ).toBe('http://127.0.0.1:4170'); + expect(formatChannelWorkerDaemonUrl('', 4170, false, 'IPv6', false)).toBe( + 'http://127.0.0.1:4170', + ); + }); + + it('falls back to IPv4 loopback for an empty hostname on an IPv4-bound socket', () => { + expect(formatChannelWorkerDaemonUrl('', 4170, false, 'IPv4')).toBe( + 'http://127.0.0.1:4170', + ); + expect(formatChannelWorkerDaemonUrl('', 4170, true, 'IPv4')).toBe( + 'https://127.0.0.1:4170', + ); + }); + + it.each(['::0', '0::0', '[::0]', '0:0:0:0:0:0:0:0'])( + 'canonicalizes IPv6 wildcard spelling %j before choosing loopback', + (host) => { + expect( + formatChannelWorkerDaemonUrl(host, 4170, false, undefined, true), + ).toBe('http://[::1]:4170'); + }, + ); + + // R14-2: the v4 wildcard's IPv4-mapped spelling canonicalizes to + // `::ffff:0:0` (WHATWG URL serializes `[::ffff:0.0.0.0]` by dropping the + // dotted quad), so it matches NEITHER wildcard branch above and used to + // fall through to the raw literal — which `assertChannelWorkerDaemonUrlIsLocal` + // then refused, even though Node binds it as a working wildcard. The + // mapping is v4 loopback, NOT `[::1]`: measured against such a bind, + // `dial 127.0.0.1` -> ok while `dial ::1` -> ECONNREFUSED even though + // the socket reports family IPv6. + it.each([ + '::ffff:0.0.0.0', + '::ffff:0:0', + '[::ffff:0.0.0.0]', + '[::ffff:0:0]', + '::FFFF:0.0.0.0', + ])('uses IPv4 loopback for the v4-mapped wildcard spelling %j', (host) => { + expect(formatChannelWorkerDaemonUrl(host, 4170)).toBe( + 'http://127.0.0.1:4170', + ); + expect(formatChannelWorkerDaemonUrl(host, 4170, true)).toBe( + 'https://127.0.0.1:4170', + ); + }); + + // The oracle for the mappings above: a real socket per bind shape, dialled + // at the address the worker is actually handed. The certification reads + // the host's interface table, so whichever loopback it picks IS assigned + // and the dial must succeed on every host where the bind itself succeeds — + // including runners that bind `::` yet carry no `::1`, the one state where + // the old spelling-based mapping was wrong (#9406). There is no v6-only + // arm: the daemon listens via `server.listen(port, host)`, and libuv pins + // IPV6_V6ONLY=0 unless `ipv6Only` is requested, so the product never binds + // a v6-only wildcard socket (`net.ipv6.bindv6only` cannot change that). + it('hands workers a loopback address the bound socket really answers', async () => { + for (const [bind, listenOptions] of [ + ['::', { host: '::', port: 0 }], + ['0.0.0.0', { host: '0.0.0.0', port: 0 }], + // R14-2: the v4-mapped wildcard binds a WORKING wildcard that serves + // v4 loopback only — measured here through the same dial the workers + // use; mutating the mapping to `[::1]` reddens this arm. + ['::ffff:0.0.0.0', { host: '::ffff:0.0.0.0', port: 0 }], + // R10-1: the daemon's real bind shape for an empty --hostname. The + // loopback family is read from the socket that actually bound, so on a + // host without IPv6 this same arm binds 0.0.0.0 and exercises the + // IPv4 fallback instead. + ['', { host: '', port: 0 }], + ] as const) { + let server: net.Server; + try { + server = await listenOn(listenOptions); + } catch { + // No AF_INET6 (or no AF_INET) on this runner: nothing to measure. + continue; + } + try { + const addr = server.address() as AddressInfo; + const certified = new URL( + formatChannelWorkerDaemonUrl( + bind, + addr.port, + false, + bind === '' && (addr.family === 'IPv4' || addr.family === 'IPv6') + ? addr.family + : undefined, + ), + ); + // URL keeps IPv6 literals bracketed; net.connect wants them bare. + const dialHost = certified.hostname.replace(/^\[|\]$/g, ''); + const dial = await dialLoopback(dialHost, addr.port); + expect({ + bind: listenOptions, + certified: certified.host, + dial, + }).toEqual({ + bind: listenOptions, + certified: certified.host, + dial: { ok: true }, + }); + } finally { + await new Promise((resolve) => server.close(resolve)); + } + } + }); + it('formats concrete IPv6 hosts for URLs', () => { expect(formatChannelWorkerDaemonUrl('::1', 4170)).toBe('http://[::1]:4170'); }); @@ -1245,6 +1440,158 @@ describe('formatChannelWorkerDaemonUrl', () => { }); }); +describe('assertChannelWorkerDaemonUrlIsLocal', () => { + it('accepts loopback and wildcard-rewritten worker URLs', () => { + for (const host of [ + '', + '0', + '0.0', + '0.0.0.0', + '::', + '::0', + '0::0', + '[::]', + '[::0]', + '0:0:0:0:0:0:0:0', + '127.0.0.1', + '::1', + '::ffff:0.0.0.0', + '::ffff:0:0', + ]) { + expect(() => + assertChannelWorkerDaemonUrlIsLocal( + formatChannelWorkerDaemonUrl(host, 4170, true), + host, + ), + ).not.toThrow(); + } + }); + + it("accepts a concrete bind on one of this host's own interfaces", () => { + const ownAddress = Object.values(os.networkInterfaces()) + .flatMap((entries) => entries ?? []) + .find((entry) => entry.family === 'IPv4' && !entry.internal)?.address; + // A machine with no non-loopback IPv4 interface cannot exercise this. + if (!ownAddress) return; + expect(() => + assertChannelWorkerDaemonUrlIsLocal( + formatChannelWorkerDaemonUrl(ownAddress, 4170, true), + ownAddress, + ), + ).not.toThrow(); + }); + + it('refuses a bind this host cannot answer, naming the hostname', () => { + expect(() => + assertChannelWorkerDaemonUrlIsLocal( + formatChannelWorkerDaemonUrl('203.0.113.7', 4170, true), + '203.0.113.7', + ), + ).toThrow(/Channels cannot start: --hostname "203\.0\.113\.7"/); + }); + + it('refuses a DNS-name bind — resolving it is not on the worker startup path', () => { + expect(() => + assertChannelWorkerDaemonUrlIsLocal( + formatChannelWorkerDaemonUrl('daemon.internal', 4170, true), + 'daemon.internal', + ), + ).toThrow(/does not name an address on this host/); + }); + + // R18-1: the primary Host gate (auth.ts) answers only 127.0.0.1, localhost, + // and [::1] — the kernel routes every other 127.x.y.z to loopback too, and + // the certifier used to accept all of them, but a worker dialing such a + // spelling gets `403 Invalid Host header` from the daemon it is trying to + // reach: the first worker's failure exits the daemon, channels added later + // restart-loop while /health stays green. Refuse what the gate refuses, + // once, at boot. + it('refuses loopback spellings the Host gate answers 403', () => { + expect(() => + assertChannelWorkerDaemonUrlIsLocal('http://127.0.0.2:8080', '127.0.0.2'), + ).toThrow(/is a loopback address the daemon's Host header gate refuses/); + for (const host of ['127.0.0.2', '127.0.1.1', '127.255.255.254']) { + expect(() => + assertChannelWorkerDaemonUrlIsLocal( + formatChannelWorkerDaemonUrl(host, 4170, true), + host, + ), + ).toThrow(/Channels cannot start: --hostname/); + } + }); + + // The refusals above are host-state-dependent: on a host that ASSIGNS the + // wide spelling (`ip addr add 127.0.0.2/8 dev lo`, a standard + // container-mesh pattern), the own-interface escape used to accept it + // before the refusal ran, and every worker dial then got `403 Invalid Host + // header`. Pin the assigned state so the ordering — Host-gate refusal + // before the own-interface escape — is witnessed on every host. + it('refuses an assigned wide loopback the Host gate answers 403', () => { + mockNetworkInterfaces.value = { + lo: [ + { + address: '127.0.0.1', + netmask: '255.0.0.0', + family: 'IPv4', + mac: '00:00:00:00:00:00', + internal: true, + cidr: '127.0.0.1/8', + }, + { + address: '127.0.0.2', + netmask: '255.0.0.0', + family: 'IPv4', + mac: '00:00:00:00:00:00', + internal: true, + cidr: '127.0.0.2/8', + }, + { + address: '::1', + netmask: 'ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff', + family: 'IPv6', + mac: '00:00:00:00:00:00', + internal: true, + cidr: '::1/128', + scopeid: 0, + }, + ], + }; + // Witness the assigned state: without this assert a broken mock would + // let the throw below pass for the wrong (unassigned) reason. + expect(isOwnInterfaceAddress('127.0.0.2')).toBe(true); + expect(() => + assertChannelWorkerDaemonUrlIsLocal('http://127.0.0.2:8080', '127.0.0.2'), + ).toThrow(/is a loopback address the daemon's Host header gate refuses/); + }); + + it('still accepts the loopback spellings the Host gate answers', () => { + for (const host of ['localhost', 'LOCALHOST', '127.0.0.1', '[::1]']) { + expect(() => + assertChannelWorkerDaemonUrlIsLocal( + formatChannelWorkerDaemonUrl(host, 4170, true), + host, + ), + ).not.toThrow(); + } + }); + + // A zone-scoped link-local bind (`fe80::…%eth0`) is an address this host + // answers on, but `formatHostForUrl` percent-encodes the zone into the + // worker URL and WHATWG URL rejects zone IDs outright — so the parse + // inside the guard used to throw a raw `ERR_INVALID_URL` instead of the + // named boot diagnostic. Refuse it with an actionable message: the worker + // pipeline cannot carry a zone. + it('refuses a zone-scoped bind with the named diagnostic, not a raw URL error', () => { + const hostname = 'fe80::1%eth0'; + expect(() => + assertChannelWorkerDaemonUrlIsLocal( + formatChannelWorkerDaemonUrl(hostname, 4170, true), + hostname, + ), + ).toThrow(/Channels cannot start: --hostname "fe80::1%eth0"/); + }); +}); + // A CA-issued leaf, the shape the documented `mkcert` flow produces: usable // as a serving cert, useless as a trust anchor. Not a real secret. const TEST_TLS_CERT_CA_ISSUED = `-----BEGIN CERTIFICATE----- @@ -11237,6 +11584,95 @@ describe('runQwenServe channel worker supervisor', () => { } }); + it('certifies the channel worker daemon URL at boot before workers start', async () => { + // Deleting the assertChannelWorkerDaemonUrlIsLocal call site in + // ensureChannelWorkerManager leaves this uncalled (mutation M3 in the + // #9406 review): the direct-call suite above never observes the boot + // path, so pin the boot wiring itself. + tmpDir = fs.realpathSync( + fs.mkdtempSync(path.join(os.tmpdir(), 'qws-channel-url-certify-')), + ); + const worker = makeWorker({ + enabled: true, + state: 'running', + pid: 1234, + channels: ['telegram'], + }); + const factory = makeReadyWorkerFactory(worker); + const channelWorkerUrlCertifier = vi.fn(); + const handle = await runQwenServe( + { + port: 0, + hostname: '127.0.0.1', + mode: 'http-bridge', + workspace: tmpDir, + serveWebShell: false, + channelSelection: { mode: 'names', names: ['telegram'] }, + }, + { + bridge: makeFakeBridge(), + channelWorkerSupervisorFactory: factory, + channelServicePidfile: makePidfileDeps(), + channelWorkerUrlCertifier, + }, + ); + + try { + await handle.runtimeReady; + expect(channelWorkerUrlCertifier).toHaveBeenCalledTimes(1); + const [daemonUrl, hostname] = channelWorkerUrlCertifier.mock.calls[0]!; + expect(hostname).toBe('127.0.0.1'); + expect(daemonUrl).toMatch(/^http:\/\/127\.0\.0\.1:\d+$/); + expect(factory).toHaveBeenCalled(); + } finally { + await handle.close(); + } + }); + + it('fails the channel boot when the worker URL certification refuses the bind', async () => { + tmpDir = fs.realpathSync( + fs.mkdtempSync(path.join(os.tmpdir(), 'qws-channel-url-refuse-')), + ); + const worker = makeWorker({ + enabled: true, + state: 'running', + pid: 1234, + channels: ['telegram'], + }); + const factory = makeReadyWorkerFactory(worker); + const handle = await runQwenServe( + { + port: 0, + hostname: '127.0.0.1', + mode: 'http-bridge', + workspace: tmpDir, + serveWebShell: false, + channelSelection: { mode: 'names', names: ['telegram'] }, + }, + { + bridge: makeFakeBridge(), + channelWorkerSupervisorFactory: factory, + channelServicePidfile: makePidfileDeps(), + resolveOnListen: true, + channelWorkerUrlCertifier: () => { + throw new Error( + 'Channels cannot start: --hostname "127.0.0.1" is not a ' + + 'loopback bind', + ); + }, + }, + ); + + try { + await expect(handle.runtimeReady).rejects.toThrow( + /Channels cannot start/, + ); + expect(factory).not.toHaveBeenCalled(); + } finally { + await handle.close(); + } + }); + async function bootTlsDaemonForTrustGapLog( hostname: string, serving: { cert: string; key: string } = { diff --git a/packages/cli/src/serve/run-qwen-serve.ts b/packages/cli/src/serve/run-qwen-serve.ts index e4df21f40a4..94742ac1163 100644 --- a/packages/cli/src/serve/run-qwen-serve.ts +++ b/packages/cli/src/serve/run-qwen-serve.ts @@ -80,7 +80,11 @@ import { createBridgeFileSystemAdapter } from './bridge-file-system-adapter.js'; // the run-qwen-serve chunk. The launcher is only needed after listen(). import { PathMutexRegistry } from './fs/path-mutex-registry.js'; import { isDeepHealthQuery } from './health-query.js'; -import { isLoopbackBind } from './loopback-binds.js'; +import { + hostAssignsIpv6Loopback, + isOwnInterfaceAddress, +} from './local-bind-addresses.js'; +import { isHostGateLoopback, isLoopbackBind } from './loopback-binds.js'; import { RUNTIME_STARTUP_CANCELLED_MESSAGE } from './runtime-startup-errors.js'; import { resolveWebShellDir } from './web-shell-resolver.js'; import { resolveServeToken } from './serve-token.js'; @@ -678,24 +682,136 @@ function workspaceRuntimeEffectiveEnv( return runtime.env.effectiveEnv ?? daemonEnv; } +function canonicalIpLiteral(host: string): string | undefined { + const inner = + host.startsWith('[') && host.endsWith(']') ? host.slice(1, -1) : host; + try { + const hostname = new URL( + `http://${inner.includes(':') ? `[${inner}]` : inner}`, + ).hostname; + const canonical = + hostname.startsWith('[') && hostname.endsWith(']') + ? hostname.slice(1, -1) + : hostname; + return isIP(canonical) === 0 ? undefined : canonical; + } catch { + return undefined; + } +} + export function formatChannelWorkerDaemonUrl( host: string, port: number, tls = false, + boundFamily?: 'IPv4' | 'IPv6', + ipv6LoopbackAssigned: boolean = hostAssignsIpv6Loopback(), ): string { const scheme = tls ? 'https' : 'http'; const normalized = host.trim().toLowerCase(); - if ( - normalized === '' || - normalized === '0.0.0.0' || - normalized === '::' || - normalized === '[::]' - ) { + const canonicalIp = canonicalIpLiteral(normalized); + // R7-7: the loopback an IPv6 wildcard bind is dialled back on follows what + // the host ASSIGNS, not the bind spelling. Node keeps such sockets + // dual-stack (libuv pins `IPV6_V6ONLY=0` unless `ipv6Only` is requested, + // so `net.ipv6.bindv6only` never reaches this listener), so both loopbacks + // usually reach it — but an IPv4-less host has only `::1`, and a host that + // binds `::` while its loopback carries no `::1` (e.g. + // `net.ipv6.conf.lo.disable_ipv6=1`) has only `127.0.0.1`. The old + // spelling-based rule handed the latter `[::1]`, and the first worker's + // `fetch failed` exited the daemon. The v4 wildcard keeps v4 loopback: + // measured against `0.0.0.0`, `dial ::1` is ECONNREFUSED. + // + // An EMPTY --hostname decides by the socket that actually bound, not by + // spelling (R10-1): Node's `_listen2` tries the IPv6 unspecified address + // for it and falls back to `0.0.0.0` when IPv6 is unavailable, so on the + // fallback host the socket is IPv4 while the spelling said IPv6 — handing + // workers `[::1]` there dialled an address nothing listens on, and the + // first worker's failure exited the daemon. Explicit `::`/`[::]` and + // `0.0.0.0` keep their spelling-based family mapping: those binds fail + // loud when their family is unavailable, so the spelling cannot lie about + // them. + const v6Loopback = ipv6LoopbackAssigned + ? `${scheme}://[::1]:${port}` + : `${scheme}://127.0.0.1:${port}`; + if (normalized === '') { + return boundFamily === 'IPv4' + ? `${scheme}://127.0.0.1:${port}` + : v6Loopback; + } + if (canonicalIp === '::') { + return v6Loopback; + } + // The v4 wildcard's IPv4-mapped spelling `::ffff:0.0.0.0` canonicalizes to + // `::ffff:0:0` (WHATWG URL serializes the mapped form by dropping the + // dotted quad), so it matches neither wildcard above. Node binds it as a + // WORKING wildcard (R14-2): measured on this Node, the socket reports + // family IPv6 yet serves v4 loopback — `dial 127.0.0.1` -> ok while + // `dial ::1` -> ECONNREFUSED — so it maps to v4 loopback, NOT `[::1]`, + // and an operator who copied the address from `ss`/`netstat` (which render + // v4 connections on dual-stack sockets as `::ffff:...`) gets channels + // that start instead of a boot refusal. + if (canonicalIp === '0.0.0.0' || canonicalIp === '::ffff:0:0') { return `${scheme}://127.0.0.1:${port}`; } return `${scheme}://${formatHostForUrl(host)}:${port}`; } +/** + * Refuse a channel boot whose worker URL this host cannot answer. + * + * Workers run on the daemon's own machine and dial the address it actually + * bound. Loopback is not a fallback: `--hostname 192.168.1.100` binds that + * socket ONLY, so a worker sent to `127.0.0.1` gets `ECONNREFUSED`. A bind + * this host cannot certify as its own — a DNS name, or a literal on no local + * interface — passes every other boot check and then throws inside each + * worker: the first one's failure exits the daemon, and channels added later + * restart-loop while `/health` stays green. Name it once, at boot. + */ +export function assertChannelWorkerDaemonUrlIsLocal( + workerDaemonUrl: string, + hostname: string, +): void { + let host: string; + try { + host = new URL(workerDaemonUrl).hostname; + } catch { + // A zone-scoped bind (`fe80::1%eth0`) arrives percent-encoded and WHATWG + // URL rejects zone IDs outright, so the worker pipeline cannot carry it + // even though this host answers on the address — refuse with the named + // boot diagnostic instead of a raw ERR_INVALID_URL. + throw new Error( + `Channels cannot start: --hostname "${hostname}" cannot be carried in ` + + `a worker URL (a zone-scoped address has no spelling the URL parser ` + + `accepts). Bind to loopback, to the wildcard (0.0.0.0 / ::), or to ` + + `a zone-less literal address of one of this machine's interfaces.`, + ); + } + if (isHostGateLoopback(host)) return; + if (isLoopbackBind(host)) { + // A wide 127/8 bind passes `isLoopbackBind` and the kernel routes it to + // loopback, but the primary Host gate answers only the spellings in + // `LOOPBACK_BINDS` — every other 127.x.y.z gets 403 before a route. + // Order matters: this refusal must run BEFORE the own-interface escape + // below — a wide loopback address can be assigned to a local interface + // (`ip addr add 127.0.0.2/8 dev lo`), and the gate 403s it either way. + throw new Error( + `Channels cannot start: --hostname "${hostname}" is a loopback ` + + `address the daemon's Host header gate refuses (it answers only ` + + `127.0.0.1, localhost, and [::1]), so channel workers cannot reach ` + + `the daemon on it. Bind to one of those spellings, to the wildcard ` + + `(0.0.0.0 / ::), or to a literal address of one of this machine's ` + + `interfaces.`, + ); + } + if (isOwnInterfaceAddress(host)) return; + throw new Error( + `Channels cannot start: --hostname "${hostname}" is not a loopback bind ` + + `and does not name an address on this host, so channel workers have no ` + + `local URL to reach the daemon on. Bind to loopback, to the wildcard ` + + `(0.0.0.0 / ::), or to a literal address of one of this machine's ` + + `interfaces.`, + ); +} + export interface WorkerTlsTrustFailure { code: string; message: string; @@ -1972,6 +2088,14 @@ export interface RunQwenServeDeps { opts: CreateChannelWorkerSupervisorOptions, ) => ChannelWorkerSupervisor; workerTlsTrustVerifier?: typeof verifyWorkerTlsTrust; + /** + * Test/embed override for the boot-time certification that the channel + * worker daemon URL is local to this host. Production refuses a bind the + * host cannot answer through `assertChannelWorkerDaemonUrlIsLocal`; tests + * inject a recorder to pin that boot runs the certification before + * starting workers. + */ + channelWorkerUrlCertifier?: typeof assertChannelWorkerDaemonUrlIsLocal; channelServicePidfile?: ChannelServicePidfile; workspaceRegistrationStore?: WorkspaceRegistrationStore; /** Test/embed override; production uses the private user Conversations root. */ @@ -8225,7 +8349,17 @@ async function runQwenServeImpl( opts.hostname, actualPort, tlsOptions !== undefined, + typeof addr === 'object' && + addr && + (addr.family === 'IPv4' || addr.family === 'IPv6') + ? addr.family + : undefined, ); + + ( + deps.channelWorkerUrlCertifier ?? + assertChannelWorkerDaemonUrlIsLocal + )(workerDaemonUrl, opts.hostname); if ( tlsOptions && tlsCertPath && diff --git a/packages/cli/src/serve/server.test.ts b/packages/cli/src/serve/server.test.ts index c81b09d3a51..07f83cd4f72 100644 --- a/packages/cli/src/serve/server.test.ts +++ b/packages/cli/src/serve/server.test.ts @@ -70,6 +70,7 @@ import { SERVE_CAPABILITY_REGISTRY, type ServeProtocolVersion, } from './capabilities.js'; +import { isNativeDirectoryPickerAvailable } from './native-directory-picker.js'; import type { CancelNotification, PromptRequest, @@ -779,6 +780,7 @@ const EXPECTED_REGISTERED_FEATURES = [ 'workspace_display_name', 'scratch_workspace_registration', 'workspace_runtime_removal', + 'native_directory_picker', 'workspace_qualified_rest_core', 'workspace_qualified_voice', 'workspace_qualified_memory', @@ -3240,6 +3242,24 @@ describe('createServeApp', () => { ); continue; } + if (feature === 'native_directory_picker') { + expect(predicate({ nativeDirectoryPickerAvailable: true })).toBe( + true, + ); + expect(predicate({ nativeDirectoryPickerAvailable: false })).toBe( + false, + ); + expect(predicate({})).toBe(false); + expect( + getAdvertisedServeFeatures(undefined, { + nativeDirectoryPickerAvailable: true, + }), + ).toContain(feature); + expect(getAdvertisedServeFeatures(undefined, {})).not.toContain( + feature, + ); + continue; + } if (feature === 'workspace_trust_hot_reload') { expect(predicate({ workspaceTrustHotReloadAvailable: true })).toBe( true, @@ -4127,6 +4147,9 @@ describe('createServeApp', () => { sessionGenerationAvailable: true, workspaceGenerationAvailable: true, acpHttpEnabled: true, + // Mirror the server.ts probe so the expectation matches on both + // GUI and headless hosts. + nativeDirectoryPickerAvailable: isNativeDirectoryPickerAvailable(), }), ); expect(res.body.modelServices).toEqual([]); @@ -4140,6 +4163,30 @@ describe('createServeApp', () => { } }); + it('forwards the native directory picker probe result to capabilities', async () => { + // M5 witness (#9406): the envelope mirror above calls the same probe + // as the product path, so on a headless host both sides say "tag + // absent" even when server.ts stops wiring the flag. Inject the probe + // result instead so the wiring is assertable on every host. + const enabled = await request( + createServeApp(baseOpts, undefined, { + nativeDirectoryPickerAvailable: true, + }), + ) + .get('/capabilities') + .set('Host', `127.0.0.1:${baseOpts.port}`); + expect(enabled.body.features).toContain('native_directory_picker'); + + const disabled = await request( + createServeApp(baseOpts, undefined, { + nativeDirectoryPickerAvailable: false, + }), + ) + .get('/capabilities') + .set('Host', `127.0.0.1:${baseOpts.port}`); + expect(disabled.body.features).not.toContain('native_directory_picker'); + }); + it('omits artifact persistence when the durable sink is unavailable', async () => { const app = createServeApp(baseOpts, undefined, { sessionArtifactsPersistenceAvailable: false, diff --git a/packages/cli/src/serve/server.ts b/packages/cli/src/serve/server.ts index 3d908808eee..59936b049fb 100644 --- a/packages/cli/src/serve/server.ts +++ b/packages/cli/src/serve/server.ts @@ -243,6 +243,7 @@ import { type WorkspaceManagementHandle, type WorkspaceRuntimeRemovalController, } from './routes/workspace-management.js'; +import { isNativeDirectoryPickerAvailable } from './native-directory-picker.js'; import type { WorkspaceRegistrationStore } from './workspace-registration-store.js'; import { registerWorkspaceGitRoutes, @@ -587,6 +588,12 @@ export interface ServeAppDeps { assertGenerationOpen?: () => void, ) => Promise; sessionArtifactsPersistenceAvailable?: boolean; + /** + * Test/embed override for the native directory picker probe. Production + * evaluates `isNativeDirectoryPickerAvailable()`; tests pin this so the + * capability wiring is assertable on headless hosts too. + */ + nativeDirectoryPickerAvailable?: boolean; /** * Reverse tool channel (issue #5626, Phase 2). Shared sender registry that * bridges the daemon WS (per-connection `ClientMcpRegistrar`) and the ACP @@ -1005,6 +1012,9 @@ export function createServeApp( acpHttpEnabled: acpHttpEnabledAtBoot, workspaceRuntimeRemovalAvailable: deps.workspaceRuntimeRemoval !== undefined, + nativeDirectoryPickerAvailable: + deps.nativeDirectoryPickerAvailable ?? + isNativeDirectoryPickerAvailable(), workspaceTrustHotReloadAvailable: deps.workspaceTrustHotReloadAvailable === true, isPrimaryWorkspaceTrusted: () => isPrimaryWorkspaceTrusted(), diff --git a/packages/cli/src/serve/server/serve-features.ts b/packages/cli/src/serve/server/serve-features.ts index fab63a6eb67..5f049f0c5c3 100644 --- a/packages/cli/src/serve/server/serve-features.ts +++ b/packages/cli/src/serve/server/serve-features.ts @@ -59,6 +59,7 @@ interface CreateServeFeaturesDeps { standaloneSessionsAvailable?: () => boolean; acpHttpEnabled?: boolean; workspaceRuntimeRemovalAvailable?: boolean; + nativeDirectoryPickerAvailable?: boolean; workspaceTrustHotReloadAvailable?: boolean; isPrimaryWorkspaceTrusted?: () => boolean; env?: Readonly>; @@ -95,6 +96,7 @@ export function createServeFeatures( standaloneSessionsAvailable, acpHttpEnabled, workspaceRuntimeRemovalAvailable, + nativeDirectoryPickerAvailable, workspaceTrustHotReloadAvailable, } = deps; const getEnv = deps.getEnv ?? (() => deps.env ?? process.env); @@ -148,6 +150,7 @@ export function createServeFeatures( scratchWorkspaceRegistrationAvailable: scratchWorkspaceRegistrationAvailable(), workspaceRuntimeRemovalAvailable, + nativeDirectoryPickerAvailable, workspaceTrustHotReloadAvailable, acpHttpEnabled: currentAcpHttpEnabled, realtimeVoiceEnabled: realtimeVoiceEnabled(), diff --git a/packages/web-shell/client/App.test.tsx b/packages/web-shell/client/App.test.tsx index ca33ea28e7d..3a9e9856239 100644 --- a/packages/web-shell/client/App.test.tsx +++ b/packages/web-shell/client/App.test.tsx @@ -9622,6 +9622,7 @@ describe('App session callbacks', () => { 'dynamic_workspace_registration', 'persistent_workspace_registration', 'workspace_display_name', + 'native_directory_picker', ], workspaces: [ { @@ -9681,6 +9682,32 @@ describe('App session callbacks', () => { ).toHaveLength(1); }); + it('omits the directory picker on headless daemon hosts', async () => { + mockWorkspace.capabilities = { + features: ['dynamic_workspace_registration'], + workspaces: [ + { + id: 'primary', + cwd: '/tmp/project', + primary: true, + trusted: true, + }, + ], + } as typeof mockWorkspace.capabilities; + const { container } = renderApp(); + await flush(); + + act(() => { + container + .querySelector('[data-testid="open-add-workspace"]') + ?.click(); + }); + expect( + container.querySelectorAll('[data-testid="add-workspace-dialog"]'), + ).toHaveLength(1); + expect(testState.latestAddWorkspaceDialogProps?.onPick).toBeUndefined(); + }); + it('forwards a supported workspace display name through the shared mutation lane', async () => { const added = { id: 'payments', diff --git a/packages/web-shell/client/App.tsx b/packages/web-shell/client/App.tsx index cf21a477679..d1ef1d9bc63 100644 --- a/packages/web-shell/client/App.tsx +++ b/packages/web-shell/client/App.tsx @@ -2414,6 +2414,11 @@ export function App({ const workspaceDisplayNameSupported = workspace.capabilities?.features?.includes('workspace_display_name') === true; + // Headless daemon hosts omit the tag so the Browse affordance stays + // hidden instead of failing on every click. + const nativeDirectoryPickerSupported = + workspace.capabilities?.features?.includes('native_directory_picker') === + true; const gitHubPrsSupported = workspace.capabilities?.features?.includes('workspace_github_prs') === true; const [showAddWorkspaceDialog, setShowAddWorkspaceDialog] = useState(false); @@ -12219,10 +12224,15 @@ export function App({ onClose={() => setShowAddWorkspaceDialog(false)} onAdd={handleAddWorkspace} onSuggest={workspaceActions.suggestWorkspacePaths} - onPick={async () => { - const result = await workspaceActions.pickWorkspaceDirectory(); - return result.selected ? result.path : undefined; - }} + onPick={ + nativeDirectoryPickerSupported + ? async () => { + const result = + await workspaceActions.pickWorkspaceDirectory(); + return result.selected ? result.path : undefined; + } + : undefined + } persistenceSupported={ persistentWorkspaceRegistrationSupported } diff --git a/packages/web-shell/client/components/dialogs/AddWorkspaceDialog.test.tsx b/packages/web-shell/client/components/dialogs/AddWorkspaceDialog.test.tsx index 290942ba5fc..5256528a10f 100644 --- a/packages/web-shell/client/components/dialogs/AddWorkspaceDialog.test.tsx +++ b/packages/web-shell/client/components/dialogs/AddWorkspaceDialog.test.tsx @@ -75,6 +75,26 @@ describe('AddWorkspaceDialog', () => { expect(document.activeElement).toBe(input()); }); + it('renders no Browse… button when the host cannot pick a directory', () => { + // #9406 R1-7: every other browseButton() call site passes onPick, and + // App.test.tsx mocks this dialog out entirely, so nothing observed the + // `{onPick && (` guard — turning it into an unconditional render shipped + // green. On a headless daemon host that puts back the dead affordance + // this PR exists to remove: a Browse… button whose handler returns + // immediately. + mount(); + + expect(browseButton()).toBeUndefined(); + }); + + it('renders the Browse… button when the host can pick a directory', () => { + mount( + , + ); + + expect(browseButton()).toBeDefined(); + }); + it('hides the display name field unless the daemon supports it', () => { mount();