diff --git a/__tests__/ui-server.test.ts b/__tests__/ui-server.test.ts index df25858a8..4b4e63fc8 100644 --- a/__tests__/ui-server.test.ts +++ b/__tests__/ui-server.test.ts @@ -12,7 +12,7 @@ * name in undici, and forging it is the whole point of half these cases. */ -import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest'; import * as http from 'http'; import * as fs from 'fs'; import * as os from 'os'; @@ -21,12 +21,14 @@ import { browserOpenCommand, cacheControlFor, contentTypeFor, + describeBindFailure, isAllowedHost, isAllowedOrigin, isSafeRequestPath, PathRefusalError, resolveProjectFile, resolveStaticAsset, + shouldTryNextPort, startUiServer, type UiServerHandle, } from '../src/ui-server'; @@ -69,6 +71,31 @@ function request( }); } +/** A failed `listen()` as node reports it. */ +function bindError(code: string, port: number): NodeJS.ErrnoException { + return Object.assign(new Error(`listen ${code}: 127.0.0.1:${port}`), { code, syscall: 'listen' }); +} + +/** + * Make `listen()` on `refused` fail with EACCES, the way Windows refuses a port + * another program holds exclusively or one inside a reserved range, without + * needing either on the machine running the test (#2299). Every other port is + * a real bind. + */ +function refuseListenOn(...refused: number[]): { mockRestore(): void } { + const realListen = http.Server.prototype.listen; + return vi + .spyOn(http.Server.prototype, 'listen') + .mockImplementation(function (this: http.Server, ...args: unknown[]) { + const port = args[0]; + if (typeof port === 'number' && refused.includes(port)) { + process.nextTick(() => this.emit('error', bindError('EACCES', port))); + return this; + } + return (realListen as (...a: unknown[]) => http.Server).apply(this, args); + }); +} + describe('codegraph ui server', () => { let tempDir: string; let viewerDir: string; @@ -198,6 +225,50 @@ describe('codegraph ui server', () => { await new Promise((resolve) => blocker.close(() => resolve())); } }); + + /** + * #2299: on Windows a port another program holds exclusively, or one in a + * reserved range, fails with EACCES rather than EADDRINUSE, and the walk + * used to stop there. Port N is really taken, N+1 is refused, so the server + * has to land beyond both. N+1 is an ephemeral port, above 1023, so this + * holds on every platform. + */ + it('falls back past a port refused with EACCES', async () => { + const blocker = http.createServer(() => {}); + await new Promise((resolve) => blocker.listen(0, '127.0.0.1', resolve)); + const taken = (blocker.address() as { port: number }).port; + const refusal = refuseListenOn(taken + 1); + + try { + const second = await startUiServer({ projectRoot, viewerDir, port: taken }); + try { + expect(second.port).toBeGreaterThan(taken + 1); + // …and it actually works on the port it landed on. + const res = await request(second.port, '/'); + expect(res.status).toBe(200); + } finally { + await second.close(); + } + } finally { + refusal.mockRestore(); + await new Promise((resolve) => blocker.close(() => resolve())); + } + }); + + it('blames a refused pinned port on what refused it, not on privileged ports', async () => { + // The port from the report: nowhere near 1024. + const refusal = refuseListenOn(49912); + try { + const failure = startUiServer({ projectRoot, viewerDir, port: 49912, portFallback: false }); + await expect(failure).rejects.toThrow(/49912/); + const message = await failure.catch((err: Error) => err.message); + expect(message).not.toMatch(/below 1024|elevated privileges/); + expect(message).not.toMatch(/already in use/); + expect(message).toMatch(/refused/); + } finally { + refusal.mockRestore(); + } + }); }); describe('Host allowlist (DNS rebinding)', () => { @@ -538,6 +609,93 @@ describe('security helpers', () => { }); }); +/** + * What a bind failure means depends on the platform (#2299): Windows has no + * privileged ports and refuses a taken or reserved one with EACCES, while + * elsewhere EACCES below 1024 is the privileged-port rule. The platform is a + * parameter, so every host checks every platform's answer. + */ +describe('port fallback on each platform', () => { + const LIST_RESERVED = 'netsh int ipv4 show excludedportrange protocol=tcp'; + + it('moves past a port in use everywhere, and past EACCES on Windows at any port', () => { + for (const platform of ['win32', 'linux', 'darwin'] as const) { + expect(shouldTryNextPort('EADDRINUSE', 80, platform), platform).toBe(true); + expect(shouldTryNextPort('EADDRINUSE', 4747, platform), platform).toBe(true); + } + expect(shouldTryNextPort('EACCES', 49912, 'win32')).toBe(true); + // Windows has no privileged-port rule: a low port is held, not forbidden. + expect(shouldTryNextPort('EACCES', 80, 'win32')).toBe(true); + }); + + it('stops at EACCES below 1024 elsewhere, where the next port needs the same privilege', () => { + expect(shouldTryNextPort('EACCES', 80, 'linux')).toBe(false); + expect(shouldTryNextPort('EACCES', 1023, 'darwin')).toBe(false); + // From 1024 up the refusal is about that port, and the next may be allowed. + expect(shouldTryNextPort('EACCES', 1024, 'linux')).toBe(true); + expect(shouldTryNextPort('EACCES', 4747, 'darwin')).toBe(true); + }); + + it('never moves past any other failure', () => { + for (const platform of ['win32', 'linux'] as const) { + expect(shouldTryNextPort('EADDRNOTAVAIL', 4747, platform), platform).toBe(false); + expect(shouldTryNextPort('EPERM', 4747, platform), platform).toBe(false); + expect(shouldTryNextPort(undefined, 4747, platform), platform).toBe(false); + } + }); + + it('says why Windows refused a pinned port, without the POSIX privileged-port story', () => { + for (const port of [49912, 80]) { + const message = describeBindFailure(bindError('EACCES', port), port, { port, fallback: false }, 'win32').message; + expect(message).toMatch( + new RegExp(`^Windows refused port ${port}: another program holds it, or it is in a reserved range`) + ); + expect(message).toContain(LIST_RESERVED); + expect(message).toContain('omit --port'); + expect(message).not.toMatch(/below 1024|elevated privileges/); + } + }); + + it('keeps the privileged-port explanation off Windows, and only below 1024', () => { + for (const fallback of [false, true]) { + const message = describeBindFailure(bindError('EACCES', 80), 80, { port: 80, fallback }, 'linux').message; + expect(message).toBe( + 'Not allowed to listen on port 80. Ports below 1024 usually need elevated privileges — pick a higher one with --port.' + ); + } + const high = describeBindFailure(bindError('EACCES', 4747), 4747, { port: 4747, fallback: false }, 'darwin').message; + expect(high).toMatch(/^Not allowed to listen on port 4747: the system refused permission\./); + expect(high).not.toMatch(/below 1024|elevated privileges|Windows|netsh/); + }); + + it('says a walk that ran out met refusals, not only ports in use', () => { + const walk = { port: 4747, fallback: true }; + const inUse = bindError('EADDRINUSE', 4766); + const refused = bindError('EACCES', 4766); + + // Nothing refused: the message it always gave. + expect(describeBindFailure(inUse, 4766, walk, 'win32').message).toBe( + 'Ports 4747–4766 are all in use. Free one, or pick another with --port.' + ); + const windows = describeBindFailure(refused, 4766, walk, 'win32').message; + expect(windows).toMatch(/^Ports 4747–4766 are all in use or reserved/); + expect(windows).toContain(LIST_RESERVED); + // An earlier port was refused even though the last one was only in use. + expect(describeBindFailure(inUse, 4766, walk, 'win32', true).message).toBe(windows); + const posix = describeBindFailure(refused, 4766, walk, 'linux').message; + expect(posix).toMatch(/^Ports 4747–4766 are all in use or not allowed\./); + expect(posix).not.toMatch(/reserved|netsh/); + }); + + it('leaves the pinned in-use message and other failures as they were', () => { + expect( + describeBindFailure(bindError('EADDRINUSE', 8080), 8080, { port: 8080, fallback: false }, 'win32').message + ).toBe('Port 8080 is already in use. Pick another with --port, or omit --port to let CodeGraph find a free one.'); + const other = bindError('EADDRNOTAVAIL', 4747); + expect(describeBindFailure(other, 4747, { port: 4747, fallback: true }, 'linux')).toBe(other); + }); +}); + describe('browserOpenCommand', () => { it('uses the platform opener', () => { expect(browserOpenCommand('http://x', 'darwin')).toEqual({ command: 'open', args: ['http://x'] }); diff --git a/docs/viewer-launch-changelog.md b/docs/viewer-launch-changelog.md index 593ba632c..84ed81505 100644 --- a/docs/viewer-launch-changelog.md +++ b/docs/viewer-launch-changelog.md @@ -205,6 +205,8 @@ These describe `codegraph ui` and its screens. They were taken out of `## [Unrel - A viewer trail now holds up to 64 hops everywhere — the trail bar, saved trails and "Read as flow". Thanks @inth3shadows for the report and @danusha2345. (#1976) +- **`codegraph ui` on Windows moves on from a port another program holds.** Windows refuses a port that another program keeps for itself, or one inside a range it reserves, with a different error than a port that is simply in use, so the viewer gave up at the first such port instead of trying the next one, and blamed ports below 1024 needing elevated privileges — a rule Windows doesn't have. It now moves on to the next free port, and when none is left, or the port you pinned with `--port` is refused, it says what Windows did and how to list the reserved ranges. Thanks @ijbranch for the report and @sx4im. (#2299) + ## Entries whose graph and `codegraph_explore` half already shipped The release notes carry a reworded version of each of these. Keep only the viewer half at launch. diff --git a/src/ui-server/index.ts b/src/ui-server/index.ts index 40c170f70..e5d0cced8 100644 --- a/src/ui-server/index.ts +++ b/src/ui-server/index.ts @@ -397,9 +397,10 @@ function safeDecode(value: string): string { /** * Bind the first free port at or after `port`, on loopback only. * - * Only `EADDRINUSE` advances to the next port — a permission failure or a bad - * address will not get better one port over, and retrying twenty times would - * only bury the real error. + * Only a port that is taken advances to the next one (see + * {@link shouldTryNextPort}) — a permission failure every port shares, or a + * bad address, will not get better one port over, and retrying twenty times + * would only bury the real error. */ async function listenWithFallback( server: http.Server, @@ -407,6 +408,9 @@ async function listenWithFallback( ): Promise { // Port 0 means "any free port", so there is nothing to fall back from. const attempts = opts.port === 0 || !opts.fallback ? 1 : Math.max(1, opts.attempts); + const platform = process.platform; + // Whether any port was refused rather than in use — the message says so. + let refused = false; for (let i = 0; i < attempts; i++) { const candidate = opts.port === 0 ? 0 : opts.port + i; @@ -419,8 +423,9 @@ async function listenWithFallback( return address.port; } catch (err) { const code = (err as NodeJS.ErrnoException).code; - if (code !== 'EADDRINUSE' || i === attempts - 1) { - throw describeBindFailure(err, candidate, opts); + if (code === 'EACCES') refused = true; + if (!shouldTryNextPort(code, candidate, platform) || i === attempts - 1) { + throw describeBindFailure(err, candidate, opts, platform, refused); } } } @@ -434,7 +439,8 @@ async function listenWithFallback( * The same `http.Server` is reused across attempts: a `listen()` that failed * with EADDRINUSE never took a handle, so it can be listened on again directly * (verified on Node 20 and 22 — `server.listening` is still `false` afterwards, - * and `close()` on a never-listening server would itself throw). + * and `close()` on a never-listening server would itself throw). The same goes + * for a port Windows refused with EACCES (checked on Node 22 and 24). */ function listenOnce(server: http.Server, port: number): Promise { return new Promise((resolve, reject) => { @@ -452,22 +458,78 @@ function listenOnce(server: http.Server, port: number): Promise { }); } -/** Turn a bind failure into something a user can act on. */ -function describeBindFailure( +/** + * Whether a `listen()` on `port` that failed with `code` leaves the next port + * worth trying. + * + * `EADDRINUSE` does, and so does `EACCES` on Windows: a port another program + * holds exclusively (`SO_EXCLUSIVEADDRUSE`), or one inside a range the system + * reserves (Hyper-V and WinNAT reserve whole blocks of them), fails there with + * EACCES rather than EADDRINUSE. Elsewhere, EACCES below 1024 is the + * privileged-port rule, which the next port is under too, so walking on would + * only bury it. From 1024 up it is something refusing that one port — a + * security policy, say — and the next one may be allowed. + */ +export function shouldTryNextPort( + code: string | undefined, + port: number, + platform: NodeJS.Platform = process.platform +): boolean { + if (code === 'EADDRINUSE') return true; + return code === 'EACCES' && !isPrivilegedPort(port, platform); +} + +/** + * Whether `port` is one only an administrator may bind: below 1024, on every + * platform but Windows, which has no such rule. + */ +function isPrivilegedPort(port: number, platform: NodeJS.Platform): boolean { + return platform !== 'win32' && port > 0 && port < 1024; +} + +/** The command that lists the port ranges Windows reserves. */ +const LIST_RESERVED_PORTS = '`netsh int ipv4 show excludedportrange protocol=tcp`'; + +/** + * Turn a bind failure into something a user can act on, and that is true where + * it is read: Windows refuses ports for reasons POSIX doesn't have, and has no + * privileged ports. + * + * `refused` says whether any port a fallback walk tried was refused (EACCES) + * rather than in use; it only changes the message for a walk that ran out. + */ +export function describeBindFailure( err: unknown, port: number, - opts: { port: number; fallback: boolean; attempts: number } + opts: { port: number; fallback: boolean }, + platform: NodeJS.Platform = process.platform, + refused = false ): Error { const code = (err as NodeJS.ErrnoException).code; + const windows = platform === 'win32'; + if (opts.fallback && shouldTryNextPort(code, port, platform)) { + // A failure that moves on only ends the walk when there is nowhere left. + const taken = + !refused && code === 'EADDRINUSE' + ? 'in use' + : windows + ? `in use or reserved (${LIST_RESERVED_PORTS} lists the reserved ranges)` + : 'in use or not allowed'; + return new Error(`Ports ${opts.port}–${port} are all ${taken}. Free one, or pick another with --port.`); + } if (code === 'EADDRINUSE') { - return opts.fallback - ? new Error( - `Ports ${opts.port}–${port} are all in use. Free one, or pick another with --port.` - ) - : new Error(`Port ${port} is already in use. Pick another with --port, or omit --port to let CodeGraph find a free one.`); + return new Error(`Port ${port} is already in use. Pick another with --port, or omit --port to let CodeGraph find a free one.`); } if (code === 'EACCES') { - return new Error(`Not allowed to listen on port ${port}. Ports below 1024 usually need elevated privileges — pick a higher one with --port.`); + if (isPrivilegedPort(port, platform)) { + return new Error(`Not allowed to listen on port ${port}. Ports below 1024 usually need elevated privileges — pick a higher one with --port.`); + } + return new Error( + (windows + ? `Windows refused port ${port}: another program holds it, or it is in a reserved range (${LIST_RESERVED_PORTS} lists them).` + : `Not allowed to listen on port ${port}: the system refused permission.`) + + ' Pick another with --port, or omit --port to let CodeGraph find a free one.' + ); } return err instanceof Error ? err : new Error(String(err)); }