diff --git a/browse/src/server.ts b/browse/src/server.ts index db4348312..3f80d3b28 100644 --- a/browse/src/server.ts +++ b/browse/src/server.ts @@ -156,6 +156,7 @@ let tunnelActive = false; let tunnelUrl: string | null = null; let tunnelListener: any = null; // ngrok listener handle let tunnelServer: ReturnType | null = null; // tunnel HTTP listener +let tunnelStarting: Promise | null = null; export type { Surface }; @@ -467,6 +468,9 @@ async function startTunnel(opts: { try { if (tunnelListener) await tunnelListener.close(); } catch {} try { boundTunnel.stop(true); } catch {} tunnelListener = null; + tunnelServer = null; + tunnelUrl = null; + tunnelActive = false; return { ok: false, stage: 'ngrok', error: err }; } } @@ -1784,11 +1788,11 @@ export function buildFetchHandler(cfg: ServerConfig): ServerHandle { state: () => ({ active: tunnelActive, url: tunnelUrl, hasListener: tunnelServer !== null }), close: closeTunnel, resolveAuthtoken: resolveNgrokAuthtoken, - start: (authtoken) => startTunnel({ + start: (authtoken) => (tunnelStarting ??= startTunnel({ fetchHandler: makeFetchHandler('tunnel'), authtoken, consent: 'pair_agent=on (isPairAgentEnabled gate at /tunnel/start)', - }), + }).finally(() => { tunnelStarting = null; })), }, commands: { handle: handleCommand, diff --git a/browse/test/dual-listener.test.ts b/browse/test/dual-listener.test.ts index 47f1f5fbf..c61e095dd 100644 --- a/browse/test/dual-listener.test.ts +++ b/browse/test/dual-listener.test.ts @@ -332,7 +332,7 @@ describe('Tunnel listener lifecycle', () => { // The route calls ctx.tunnel.start (server-auth.test.ts pins that call); // the factory wires it to the shared startTunnel() helper with the // factory-scoped tunnel-surface handler. - const startBlock = sliceBetween(SERVER_SRC, 'start: (authtoken) => startTunnel({', 'commands: {'); + const startBlock = sliceBetween(SERVER_SRC, 'start: (authtoken) => (tunnelStarting ??= startTunnel({', 'commands: {'); expect(startBlock).toContain("makeFetchHandler('tunnel')"); // The helper owns the ephemeral bind and points ngrok at the TUNNEL // port — never the local daemon port. diff --git a/browse/test/tunnel-start-state.test.ts b/browse/test/tunnel-start-state.test.ts new file mode 100644 index 000000000..dcad42e71 --- /dev/null +++ b/browse/test/tunnel-start-state.test.ts @@ -0,0 +1,112 @@ +/** + * POST /tunnel/start state under a mocked ngrok: two concurrent starts share + * one session, and a start that fails after ngrok opens leaves the daemon + * idle-stoppable. Each probe runs in a child `bun test` so the ngrok module + * mock and the module-level tunnel state cannot leak into other shard files. + */ + +import { expect, test } from 'bun:test'; +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; + +const SRC = path.join(import.meta.dir, '..', 'src'); + +const PRELUDE = ` +import { expect, mock, test } from 'bun:test'; +import * as fs from 'node:fs'; +import * as path from 'node:path'; +const opened: string[] = []; +const closed: string[] = []; +let forwardGate: Promise = Promise.resolve(); +mock.module(${JSON.stringify(Bun.resolveSync('@ngrok/ngrok', SRC))}, () => ({ + forward: async () => { + const id = 'session-' + (opened.length + 1); + opened.push(id); + await forwardGate; + return { url: () => 'https://' + id + '.invalid', close: async () => { closed.push(id); } }; + }, +})); +const { buildFetchHandler, __testInternals__ } = await import(${JSON.stringify(path.join(SRC, 'server.ts'))}); +const { resolveConfig } = await import(${JSON.stringify(path.join(SRC, 'config.ts'))}); +const stateFile = process.env.BROWSE_STATE_FILE!; +const token = 'tunnel-start-token-0123456789'; +const handle = buildFetchHandler({ + authToken: token, browsePort: 34998, config: resolveConfig(), ownsTerminalAgent: false, startTime: Date.now(), + browserManager: { getConnectionMode: () => 'launched', isWatching: () => false, stopWatch: () => {}, close: async () => {}, onDisconnect: null } as any, +}); +const start = () => handle.fetchLocal(new Request('http://127.0.0.1:34998/tunnel/start', { method: 'POST', headers: { Authorization: 'Bearer ' + token } }), null); +`; + +async function runProbe(body: string): Promise<{ code: number; tail: string }> { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'tunnel-start-')); + fs.writeFileSync(path.join(dir, 'probe.test.ts'), PRELUDE + body); + try { + const child = Bun.spawn([process.execPath, 'test', './probe.test.ts'], { + cwd: dir, + stdout: 'pipe', + stderr: 'pipe', + env: { + ...process.env, + BROWSE_STATE_FILE: path.join(dir, 'state', 'browse.json'), + GSTACK_HOME: path.join(dir, 'gstack'), + GSTACK_PAIR_AGENT: 'on', + NGROK_AUTHTOKEN: 'fake-token-for-test', + NGROK_DOMAIN: '', + }, + }); + const timer = setTimeout(() => child.kill(), 60_000); + const [out, err, code] = await Promise.all([ + new Response(child.stdout).text(), + new Response(child.stderr).text(), + child.exited, + ]); + clearTimeout(timer); + return { code, tail: (out + err).slice(-3000) }; + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } +} + +test('two concurrent tunnel starts share one ngrok session and URL', async () => { + const result = await runProbe(` +test('probe', async () => { + fs.mkdirSync(path.dirname(stateFile), { recursive: true }); + fs.writeFileSync(stateFile, JSON.stringify({ pid: process.pid, instanceId: __testInternals__.serverInstanceId })); + let release!: () => void; + forwardGate = new Promise(r => { release = r; }); + const a = start(); + const b = start(); + await new Promise(r => setTimeout(r, 50)); + release(); + const [ra, rb] = await Promise.all([a, b]); + expect(opened).toEqual(['session-1']); + expect(closed).toEqual([]); + expect([ra.status, rb.status]).toEqual([200, 200]); + const [ja, jb] = [await ra.json(), await rb.json()] as any[]; + expect(ja.url).toBe('https://session-1.invalid'); + expect(jb.url).toBe(ja.url); +}); +`); + expect(result).toMatchObject({ code: 0 }); + expect(result.tail).toContain('1 pass'); +}, 90_000); + +test('a tunnel start that fails after ngrok opens leaves the daemon idle-stoppable', async () => { + const result = await runProbe(` +test('probe', async () => { + const resp = await start(); + expect(resp.status).toBe(500); + expect(opened).toEqual(['session-1']); + expect(closed).toEqual(['session-1']); + const exitMock = mock((_c?: number) => {}); + (process as any).exit = exitMock; + __testInternals__.setLastActivity(Date.now() - 31 * 60 * 1000); + __testInternals__.idleCheckTick(); + for (let i = 0; i < 6; i++) await new Promise(r => setImmediate(r)); + expect(exitMock.mock.calls.length).toBeGreaterThan(0); +}); +`); + expect(result).toMatchObject({ code: 0 }); + expect(result.tail).toContain('1 pass'); +}, 90_000); diff --git a/changelog.d/fix-browse-tunnel-start-leaks-and-stuck-state.md b/changelog.d/fix-browse-tunnel-start-leaks-and-stuck-state.md new file mode 100644 index 000000000..660e18c26 --- /dev/null +++ b/changelog.d/fix-browse-tunnel-start-leaks-and-stuck-state.md @@ -0,0 +1,4 @@ +--- +important: security +--- +- **Two quick `/tunnel/start` calls no longer leave a public ngrok tunnel you can't close.** When two callers asked the browse daemon to start its tunnel at the same moment, both saw it off and each opened its own ngrok session. The daemon kept only the second, so the first stayed public on your ngrok account until the daemon exited, and closing the tunnel didn't reach it. Now a start that arrives while another is running waits for it and gets the same URL, so there is only ever one session. Also, when a start failed after ngrok had opened (for example, the state file write failed), the daemon cleaned up the session but still believed a tunnel was live, so it never shut down on idle; it now resets its tunnel state on that failure too.