diff --git a/browse/src/server.ts b/browse/src/server.ts index 3f80d3b28..ad4588ec2 100644 --- a/browse/src/server.ts +++ b/browse/src/server.ts @@ -401,8 +401,14 @@ type StartTunnelResult = * * On success, sets the module tunnel state (tunnelListener / tunnelUrl / * tunnelServer / tunnelActive) and records the tunnel in the state file. + * Concurrent calls from either caller share the one in-flight start. */ -async function startTunnel(opts: { +async function startTunnel(opts: Parameters[0]): Promise { + tunnelStarting ??= openTunnel(opts).finally(() => { tunnelStarting = null; }); + return tunnelStarting; +} + +async function openTunnel(opts: { fetchHandler: (req: Request, server: any) => Promise; authtoken: string; consent: string; @@ -1788,11 +1794,11 @@ export function buildFetchHandler(cfg: ServerConfig): ServerHandle { state: () => ({ active: tunnelActive, url: tunnelUrl, hasListener: tunnelServer !== null }), close: closeTunnel, resolveAuthtoken: resolveNgrokAuthtoken, - start: (authtoken) => (tunnelStarting ??= startTunnel({ + start: (authtoken) => 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 c61e095dd..47f1f5fbf 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) => (tunnelStarting ??= startTunnel({', 'commands: {'); + const startBlock = sliceBetween(SERVER_SRC, 'start: (authtoken) => 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 index dcad42e71..ad64da138 100644 --- a/browse/test/tunnel-start-state.test.ts +++ b/browse/test/tunnel-start-state.test.ts @@ -1,6 +1,7 @@ /** * 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 + * one session, a route start that overlaps the BROWSE_TUNNEL=1 auto-start + * shares it too, 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. */ @@ -27,8 +28,9 @@ mock.module(${JSON.stringify(Bun.resolveSync('@ngrok/ngrok', SRC))}, () => ({ return { url: () => 'https://' + id + '.invalid', close: async () => { closed.push(id); } }; }, })); -const { buildFetchHandler, __testInternals__ } = await import(${JSON.stringify(path.join(SRC, 'server.ts'))}); +const { buildFetchHandler, start: startDaemon, __testInternals__ } = await import(${JSON.stringify(path.join(SRC, 'server.ts'))}); const { resolveConfig } = await import(${JSON.stringify(path.join(SRC, 'config.ts'))}); +const { __resetRegistry } = await import(${JSON.stringify(path.join(SRC, 'token-registry.ts'))}); const stateFile = process.env.BROWSE_STATE_FILE!; const token = 'tunnel-start-token-0123456789'; const handle = buildFetchHandler({ @@ -38,7 +40,7 @@ const handle = buildFetchHandler({ 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 }> { +async function runProbe(body: string, env: Record = {}): 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 { @@ -50,9 +52,11 @@ async function runProbe(body: string): Promise<{ code: number; tail: string }> { ...process.env, BROWSE_STATE_FILE: path.join(dir, 'state', 'browse.json'), GSTACK_HOME: path.join(dir, 'gstack'), + CHROMIUM_PROFILE: path.join(dir, 'profile'), GSTACK_PAIR_AGENT: 'on', NGROK_AUTHTOKEN: 'fake-token-for-test', NGROK_DOMAIN: '', + ...env, }, }); const timer = setTimeout(() => child.kill(), 60_000); @@ -92,6 +96,37 @@ test('probe', async () => { expect(result.tail).toContain('1 pass'); }, 90_000); +test('a route start that overlaps the BROWSE_TUNNEL=1 auto-start shares its ngrok session', async () => { + const result = await runProbe(` +test('probe', async () => { + (process as any).exit = () => {}; + let release!: () => void; + forwardGate = new Promise(r => { release = r; }); + __resetRegistry(); + let booting = true; + const booted = startDaemon().finally(() => { booting = false; }); + for (let i = 0; i < 900 && booting && opened.length === 0; i++) await new Promise(r => setTimeout(r, 50)); + try { + expect(opened).toEqual(['session-1']); + const state = JSON.parse(fs.readFileSync(stateFile, 'utf-8')); + const routed = fetch('http://127.0.0.1:' + state.port + '/tunnel/start', { method: 'POST', headers: { Authorization: 'Bearer ' + state.token } }); + await new Promise(r => setTimeout(r, 100)); + release(); + const [resp] = await Promise.all([routed, booted]); + expect(opened).toEqual(['session-1']); + expect(closed).toEqual([]); + expect(resp.status).toBe(200); + expect((await resp.json()).url).toBe('https://session-1.invalid'); + } finally { + release(); + await __testInternals__.browserManager.close(); + } +}, 60_000); +`, { BROWSE_TUNNEL: '1', BROWSE_PARENT_PID: '0' }); + 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 () => { diff --git a/changelog.d/fix-browse-tunnel-auto-start-shares-start-lock.md b/changelog.d/fix-browse-tunnel-auto-start-shares-start-lock.md new file mode 100644 index 000000000..ed5214e25 --- /dev/null +++ b/changelog.d/fix-browse-tunnel-auto-start-shares-start-lock.md @@ -0,0 +1,4 @@ +--- +important: security +--- +- **A daemon launched with `BROWSE_TUNNEL=1` no longer opens a second ngrok tunnel when `/tunnel/start` arrives during its own start.** The earlier fix made overlapping `/tunnel/start` calls share one start, but the daemon's own startup tunnel skipped that lock. A `/tunnel/start` that arrived while the startup tunnel was still opening saw the tunnel off and opened its own ngrok session, and the daemon kept only one of the two. The other stayed public on your ngrok account until the daemon exited, and closing the tunnel didn't reach it. Now every tunnel start, from the route or from startup, goes through one shared lock and gets the same URL, so there is only ever one session.