From ba2289e1cd9f2387d069b3b6c51b404d56c4a1e9 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Mon, 9 Nov 2026 03:42:34 -0800 Subject: [PATCH] Stop concurrent browse tunnel starts from leaking an ngrok session (GST-2117) (#4210) Two /tunnel/start calls that both saw the tunnel off each opened an ngrok session; the daemon kept only the second, so the first stayed public and closeTunnel could not reach it. ctx.tunnel.start is now single-flight, so a concurrent caller shares the in-flight start and its URL. A start that failed after ngrok.forward() reset only tunnelListener, leaving tunnelActive, tunnelUrl and tunnelServer set, so the daemon never idled out. The catch now clears all four. Tests found and first written by GEVA-449's pstack verification. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- browse/src/server.ts | 8 +- browse/test/dual-listener.test.ts | 2 +- browse/test/tunnel-start-state.test.ts | 112 ++++++++++++++++++ ...owse-tunnel-start-leaks-and-stuck-state.md | 4 + 4 files changed, 123 insertions(+), 3 deletions(-) create mode 100644 browse/test/tunnel-start-state.test.ts create mode 100644 changelog.d/fix-browse-tunnel-start-leaks-and-stuck-state.md 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.