From e426293017e02ef95f86e43e33b09e54f04e4825 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Fri, 14 Aug 2026 19:29:29 -0700 Subject: [PATCH] fix(server): one lone-surrogate sanitizer, one sanitizeReplacer, one startTunnel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three copies of the surrogate sanitizer existed with two algorithms (sanitize.ts regex vs a hand-rolled charCodeAt walk in server.ts — verified byte-identical across 11 edge cases before converging) plus two identical sanitizeReplacer definitions each wrapping a different copy. sanitize.ts is now the single source of truth; the runs-INSIDE-JSON.stringify egress invariant is unchanged at every call site and its pin tests were adapted to the new import shape without losing intent. The ngrok tunnel-start sequence existed three times in server.ts — the /tunnel/start route and the BROWSE_TUNNEL=1 autostart were line-for-line equivalent (a comment admitted 'Same cleanup as /tunnel/start's error path'). One startTunnel() now owns the ephemeral loopback bind, the pre-send egress receipt, the state-file RMW via tmpStatePath(), and the ordered error-path cleanup; callers keep their distinct response surfaces. The BROWSE_TUNNEL_LOCAL_ONLY test path shares nothing (no ngrok, different state field) and deliberately stays separate. Co-Authored-By: Claude Fable 5 --- browse/src/sanitize.ts | 15 + browse/src/server.ts | 278 +++++++++--------- browse/src/sse-helpers.ts | 17 +- browse/test/dual-listener.test.ts | 61 +++- .../test/server-sanitize-surrogates.test.ts | 55 ++-- 5 files changed, 231 insertions(+), 195 deletions(-) diff --git a/browse/src/sanitize.ts b/browse/src/sanitize.ts index 800a077d4..72a3b1523 100644 --- a/browse/src/sanitize.ts +++ b/browse/src/sanitize.ts @@ -17,6 +17,21 @@ export function stripLoneSurrogates(s: string): string { return s.replace(LONE_SURROGATE_HIGH, '�').replace(LONE_SURROGATE_LOW, '�'); } +/** + * JSON.stringify replacer that strips lone UTF-16 surrogates from string + * values before they get escape-encoded. Pair with stringify when the + * consumer will JSON.parse the payload back into JS strings (SSE clients + * do this). Required at every JSON/SSE egress that ships page-content-derived + * fields — see CLAUDE.md "Unicode sanitization at server egress". + * + * The replacer must run INSIDE the encoding pipeline: post-stringify regex + * is a no-op because JSON.stringify has already converted \uD800 into the + * literal escape text "\\ud800" before a regex could see the surrogate. + */ +export function sanitizeReplacer(_key: string, value: unknown): unknown { + return typeof value === 'string' ? stripLoneSurrogates(value) : value; +} + // Matches \uD8XX-\uDFXX escape text where the pair is not completed by an // adjacent \uDC00-\uDFFF (high) or preceded by \uD800-\uDBFF (low). const LONE_SURROGATE_HIGH_ESCAPE = /\\u[Dd][89ABab][0-9A-Fa-f]{2}(?!\\u[Dd][C-Fc-f][0-9A-Fa-f]{2})/g; diff --git a/browse/src/server.ts b/browse/src/server.ts index 466c6f212..4df22e720 100644 --- a/browse/src/server.ts +++ b/browse/src/server.ts @@ -45,7 +45,7 @@ import { inspectElement, modifyStyle, resetModifications, getModificationHistory import { safeUnlink, safeUnlinkQuiet, safeKill } from './error-handling'; import { readAgentRecord, killAgentByRecord, agentRecordPath, spawnTerminalAgent } from './terminal-agent-control'; import { isProcessAlive } from './error-handling'; -import { sanitizeBody, stripLoneSurrogateEscapes } from './sanitize'; +import { sanitizeBody, stripLoneSurrogateEscapes, stripLoneSurrogates, sanitizeReplacer } from './sanitize'; import { startSocksBridge, testUpstream, type BridgeHandle } from './socks-bridge'; import { parseProxyConfig, toUpstreamConfig, ProxyConfigError } from './proxy-config'; import { writeReceipt } from '../../lib/egress-receipt'; @@ -68,41 +68,22 @@ import * as path from 'path'; import * as crypto from 'crypto'; // ─── Unicode Sanitization ─────────────────────────────────────── -// Remove unpaired UTF-16 surrogate halves (\uD800–\uDFFF). Page DOM text, -// OCR output, and other CDP-sourced strings can contain lone surrogates; -// JSON consumers downstream (Anthropic API in particular) reject them with -// "no low surrogate in string". Valid surrogate pairs (e.g. emoji) survive -// unchanged. Lone halves become U+FFFD (�). +// Unpaired UTF-16 surrogate halves (\uD800–\uDFFF) in page DOM text, OCR +// output, and other CDP-sourced strings are rejected by JSON consumers +// downstream (Anthropic API in particular: "no low surrogate in string"). +// The sanitizers live in sanitize.ts (single source of truth, shared with +// sse-helpers.ts and the read/snapshot pipeline): `stripLoneSurrogates` +// replaces lone halves with U+FFFD (valid pairs like emoji survive), and +// `sanitizeReplacer` runs it on every string value inside JSON.stringify. // // INVARIANT: every server egress path that ships page-content strings MUST -// route through this sanitizer. handleCommandInternal wraps the final +// route through the sanitizer. handleCommandInternal wraps the final // cr.result string (text/plain bodies carry lone surrogates verbatim; -// JSON.stringify already escapes them). The two SSE producers below -// stringify with `sanitizeReplacer` so payload string fields get cleaned -// BEFORE escaping. Plain post-stringify regex is a no-op there because -// JSON.stringify converts \uD800 → "\\ud800" — the regex can't see the -// surrogate after that point. -function sanitizeLoneSurrogates(str: string): string { - return str.replace(/[\uD800-\uDFFF]/g, (match, offset) => { - const code = match.charCodeAt(0); - if (code >= 0xD800 && code <= 0xDBFF) { - const next = str.charCodeAt(offset + 1); - if (next >= 0xDC00 && next <= 0xDFFF) return match; - } - if (code >= 0xDC00 && code <= 0xDFFF) { - const prev = str.charCodeAt(offset - 1); - if (prev >= 0xD800 && prev <= 0xDBFF) return match; - } - return '�'; - }); -} - -// JSON.stringify replacer that sanitizes string values before they get -// escape-encoded. Pair with stringify when the consumer will JSON.parse the -// payload back into JS strings (SSE clients do this). -function sanitizeReplacer(_key: string, value: unknown): unknown { - return typeof value === 'string' ? sanitizeLoneSurrogates(value) : value; -} +// JSON.stringify already escapes them). The SSE producers stringify with +// `sanitizeReplacer` so payload string fields get cleaned BEFORE escaping. +// Plain post-stringify regex is a no-op there because JSON.stringify +// converts \uD800 → "\\ud800" — the regex can't see the surrogate after +// that point. // ─── Config ───────────────────────────────────────────────────── const config = resolveConfig(); @@ -402,6 +383,100 @@ async function closeTunnel(): Promise { tunnelActive = false; } +/** + * Result of startTunnel(). `stage` tells the caller which half failed so it + * can keep its distinct error surface: 'bind' = the tunnel-surface Bun.serve + * listener could not bind (nothing to clean up), 'ngrok' = anything after the + * bind (ngrok forward, egress receipt, state-file write) — startTunnel has + * already torn down both ngrok and the Bun listener by the time it returns. + */ +type StartTunnelResult = + | { ok: true; url: string } + | { ok: false; stage: 'bind' | 'ngrok'; error: Error }; + +/** + * Start the ngrok tunnel using the dual-listener pattern: bind a dedicated + * tunnel-surface listener on an ephemeral 127.0.0.1 port and point + * ngrok.forward() at THAT port — the local listener (which serves + * /extension-token, /cookie-picker, /inspector/*, welcome, etc.) is never + * exposed to ngrok. Shared by the /tunnel/start route handler (which passes + * its in-closure makeFetchHandler('tunnel')) and the BROWSE_TUNNEL=1 + * auto-start flow in start() (which passes handle.fetchTunnel from the + * factory). The BROWSE_TUNNEL_LOCAL_ONLY=1 test path does NOT use this + * helper — it binds the tunnel surface with no ngrok forwarding at all. + * + * Hard fail on listener bind (`stage: 'bind'`) — NEVER fall back to the + * local port, which would silently defeat the whole security property. + * + * On success, sets the module tunnel state (tunnelListener / tunnelUrl / + * tunnelServer / tunnelActive) and records the tunnel in the state file. + */ +async function startTunnel(opts: { + fetchHandler: (req: Request, server: any) => Promise; + authtoken: string; + consent: string; +}): Promise { + // Bind the tunnel listener on an ephemeral port. HARD FAIL if this + // errors — never fall back to the local port. + let boundTunnel: ReturnType; + try { + boundTunnel = Bun.serve({ + port: 0, + hostname: '127.0.0.1', + fetch: opts.fetchHandler, + }); + } catch (err: any) { + return { ok: false, stage: 'bind', error: err }; + } + const tunnelPort = boundTunnel.port; + + // Point ngrok at the TUNNEL port (not the local port). If this fails, + // tear the listener back down so we don't leak sockets. + try { + const ngrok = await import('@ngrok/ngrok'); + const domain = process.env.NGROK_DOMAIN; + const forwardOpts: any = { addr: tunnelPort, authtoken: opts.authtoken }; + if (domain) forwardOpts.domain = domain; + + // Egress receipt BEFORE the tunnel session opens, fail-closed: a + // writeReceipt failure lands in this catch, which tears the tunnel + // listener back down and refuses the start. One receipt per session + // open; browse command behavior over the tunnel is unchanged. + writeReceipt({ + sink: 'browse-tunnel', + host: domain || 'connect.ngrok-agent.com', + payloadClass: 'tunnel-session-open (scoped-token browser-command surface)', + bytes: 0, + sha256: null, + consent: opts.consent, + }); + + tunnelListener = await ngrok.forward(forwardOpts); + tunnelUrl = tunnelListener.url(); + tunnelServer = boundTunnel; + tunnelActive = true; + console.log(`[browse] Tunnel listener bound on 127.0.0.1:${tunnelPort}, ngrok → ${tunnelUrl}`); + + // Update state file + const stateContent = JSON.parse(fs.readFileSync(config.stateFile, 'utf-8')); + stateContent.tunnel = { url: tunnelUrl, domain: domain || null, startedAt: new Date().toISOString() }; + const tmpState = tmpStatePath(); + fs.writeFileSync(tmpState, JSON.stringify(stateContent, null, 2), { mode: 0o600 }); + fs.renameSync(tmpState, config.stateFile); + + return { ok: true, url: tunnelUrl! }; + } catch (err: any) { + // Clean up BOTH ngrok and the Bun listener on failure. If + // ngrok.forward() succeeded but tunnelListener.url() or the + // state-file write threw, we'd otherwise leak an active ngrok + // session on the user's account. + try { if (tunnelListener) await tunnelListener.close(); } catch {} + try { boundTunnel.stop(true); } catch {} + tunnelListener = null; + return { ok: false, stage: 'ngrok', error: err }; + } +} + // Module-level validateAuth deleted in v1.35.0.0. Factory-scoped equivalent // in buildFetchHandler closes over cfg.authToken so every internal auth check // sees the same token the routes receive. @@ -1310,7 +1385,7 @@ async function handleCommandInternal( opts?: { skipRateCheck?: boolean; skipActivity?: boolean; chainDepth?: number }, ): Promise { const cr = await handleCommandInternalImpl(body, tokenInfo, opts); - return { ...cr, result: sanitizeLoneSurrogates(cr.result) }; + return { ...cr, result: stripLoneSurrogates(cr.result) }; } /** @@ -2379,71 +2454,24 @@ export function buildFetchHandler(cfg: ServerConfig): ServerHandle { }), { status: 400, headers: { 'Content-Type': 'application/json' } }); } - // 2) Bind the tunnel listener on an ephemeral port. HARD FAIL if - // this errors — never fall back to the local port. - let boundTunnel: ReturnType; - try { - boundTunnel = Bun.serve({ - port: 0, - hostname: '127.0.0.1', - fetch: makeFetchHandler('tunnel'), - }); - } catch (err: any) { + // 2) Bind the tunnel listener + open ngrok via the shared helper + // (see startTunnel — hard-fails the bind, cleans up both ngrok + // and the Bun listener on any post-bind failure). + const started = await startTunnel({ + fetchHandler: makeFetchHandler('tunnel'), + authtoken, + consent: 'pair_agent=on', + }); + if (!started.ok) { return new Response(JSON.stringify({ - error: `Failed to bind tunnel listener: ${err.message}`, - }), { status: 500, headers: { 'Content-Type': 'application/json' } }); - } - const tunnelPort = boundTunnel.port; - - // 3) Point ngrok at the TUNNEL port (not the local port). If this - // fails, tear the listener back down so we don't leak sockets. - try { - const ngrok = await import('@ngrok/ngrok'); - const domain = process.env.NGROK_DOMAIN; - const forwardOpts: any = { addr: tunnelPort, authtoken }; - if (domain) forwardOpts.domain = domain; - - // Egress receipt BEFORE the tunnel session opens, fail-closed: a - // writeReceipt failure lands in this catch, which tears the tunnel - // listener back down and refuses the start. One receipt per session - // open; browse command behavior over the tunnel is unchanged. - writeReceipt({ - sink: 'browse-tunnel', - host: domain || 'connect.ngrok-agent.com', - payloadClass: 'tunnel-session-open (scoped-token browser-command surface)', - bytes: 0, - sha256: null, - consent: 'pair_agent=on', - }); - - tunnelListener = await ngrok.forward(forwardOpts); - tunnelUrl = tunnelListener.url(); - tunnelServer = boundTunnel; - tunnelActive = true; - console.log(`[browse] Tunnel listener bound on 127.0.0.1:${tunnelPort}, ngrok → ${tunnelUrl}`); - - // Update state file - const stateContent = JSON.parse(fs.readFileSync(config.stateFile, 'utf-8')); - stateContent.tunnel = { url: tunnelUrl, domain: domain || null, startedAt: new Date().toISOString() }; - const tmpState = tmpStatePath(); - fs.writeFileSync(tmpState, JSON.stringify(stateContent, null, 2), { mode: 0o600 }); - fs.renameSync(tmpState, config.stateFile); - - return new Response(JSON.stringify({ url: tunnelUrl }), { - status: 200, headers: { 'Content-Type': 'application/json' }, - }); - } catch (err: any) { - // Clean up BOTH ngrok and the Bun listener on failure. If - // ngrok.forward() succeeded but tunnelListener.url() or the - // state-file write threw, we'd otherwise leak an active ngrok - // session on the user's account. - try { if (tunnelListener) await tunnelListener.close(); } catch {} - try { boundTunnel.stop(true); } catch {} - tunnelListener = null; - return new Response(JSON.stringify({ - error: `Failed to open ngrok tunnel: ${err.message}`, + error: started.stage === 'bind' + ? `Failed to bind tunnel listener: ${started.error.message}` + : `Failed to open ngrok tunnel: ${started.error.message}`, }), { status: 500, headers: { 'Content-Type': 'application/json' } }); } + return new Response(JSON.stringify({ url: started.url }), { + status: 200, headers: { 'Content-Type': 'application/json' }, + }); } // ─── SSE session cookie mint (auth required) ────────────────── @@ -3096,53 +3124,17 @@ export async function start() { if (!authtoken) { console.error('[browse] BROWSE_TUNNEL=1 but no NGROK_AUTHTOKEN found. Set it via env var or ~/.gstack/ngrok.env'); } else { - let boundTunnel: ReturnType | null = null; - try { - boundTunnel = Bun.serve({ - port: 0, - hostname: '127.0.0.1', - fetch: handle.fetchTunnel, - }); - const tunnelPort = boundTunnel.port; - - const ngrok = await import('@ngrok/ngrok'); - const domain = process.env.NGROK_DOMAIN; - const forwardOpts: any = { addr: tunnelPort, authtoken }; - if (domain) forwardOpts.domain = domain; - - // Egress receipt BEFORE the tunnel session opens, fail-closed: a - // writeReceipt failure lands in this catch, which cleans up the - // listener and skips the tunnel (same as any other startup failure). - writeReceipt({ - sink: 'browse-tunnel', - host: domain || 'connect.ngrok-agent.com', - payloadClass: 'tunnel-session-open (scoped-token browser-command surface)', - bytes: 0, - sha256: null, - consent: 'pair_agent=on (BROWSE_TUNNEL=1)', - }); - - tunnelListener = await ngrok.forward(forwardOpts); - tunnelUrl = tunnelListener.url(); - tunnelServer = boundTunnel; - tunnelActive = true; - - console.log(`[browse] Tunnel listener bound on 127.0.0.1:${tunnelPort}, ngrok → ${tunnelUrl}`); - - // Update state file with tunnel URL - const stateContent = JSON.parse(fs.readFileSync(config.stateFile, 'utf-8')); - stateContent.tunnel = { url: tunnelUrl, domain: domain || null, startedAt: new Date().toISOString() }; - const tmpState = tmpStatePath(); - fs.writeFileSync(tmpState, JSON.stringify(stateContent, null, 2), { mode: 0o600 }); - fs.renameSync(tmpState, config.stateFile); - } catch (err: any) { - console.error(`[browse] Failed to start tunnel: ${err.message}`); - // Same cleanup as /tunnel/start's error path: tear down BOTH - // ngrok and the Bun listener so we don't leak an ngrok session - // if the error happened after ngrok.forward() resolved. - try { if (tunnelListener) await tunnelListener.close(); } catch {} - try { if (boundTunnel) boundTunnel.stop(true); } catch {} - tunnelListener = null; + // Shared startTunnel helper: binds the tunnel listener, opens ngrok, + // and on any failure tears down BOTH ngrok and the Bun listener so we + // don't leak an ngrok session if the error happened after + // ngrok.forward() resolved. + const started = await startTunnel({ + fetchHandler: handle.fetchTunnel, + authtoken, + consent: 'pair_agent=on (BROWSE_TUNNEL=1)', + }); + if (!started.ok) { + console.error(`[browse] Failed to start tunnel: ${started.error.message}`); } } } else if (process.env.BROWSE_TUNNEL_LOCAL_ONLY === '1') { diff --git a/browse/src/sse-helpers.ts b/browse/src/sse-helpers.ts index ed4954112..a369658f0 100644 --- a/browse/src/sse-helpers.ts +++ b/browse/src/sse-helpers.ts @@ -12,18 +12,11 @@ // inherits the invariant — cleanup runs on abort, enqueue failure, AND // heartbeat failure, exactly once, regardless of which edge fires first. -import { stripLoneSurrogates } from './sanitize'; - -/** - * JSON.stringify replacer that strips lone UTF-16 surrogates from string - * values before they get escape-encoded. Pair with stringify when the - * consumer will JSON.parse the payload back into JS strings (SSE clients - * do this). Required at every SSE egress that ships page-content-derived - * fields — see CLAUDE.md "Unicode sanitization at server egress". - */ -function sanitizeReplacer(_key: string, value: unknown): unknown { - return typeof value === 'string' ? stripLoneSurrogates(value) : value; -} +// sanitizeReplacer strips lone UTF-16 surrogates from string values before +// they get escape-encoded — required at every SSE egress that ships +// page-content-derived fields. See CLAUDE.md "Unicode sanitization at +// server egress" and the canonical implementation in sanitize.ts. +import { sanitizeReplacer } from './sanitize'; /** Send an SSE event. Handles JSON encoding + lone-surrogate sanitization. */ export type SseSender = (event: string, data: unknown) => void; diff --git a/browse/test/dual-listener.test.ts b/browse/test/dual-listener.test.ts index 41df9e9c1..68066ccb1 100644 --- a/browse/test/dual-listener.test.ts +++ b/browse/test/dual-listener.test.ts @@ -140,15 +140,16 @@ describe('Request handler factory', () => { }); test('Tunnel listener bind uses handle.fetchTunnel from buildFetchHandler', () => { - // v1.35.0.0: factory returns handle.fetchTunnel; tunnel start sites use it - // (BROWSE_TUNNEL=1 startup + BROWSE_TUNNEL_LOCAL_ONLY=1 test path). + // v1.35.0.0: factory returns handle.fetchTunnel; tunnel start sites use it. + // The BROWSE_TUNNEL=1 startup passes it to the shared startTunnel() helper + // (which owns the Bun.serve bind); the BROWSE_TUNNEL_LOCAL_ONLY=1 test path + // binds its own listener with it directly. // The /tunnel/start handler INSIDE the factory still uses makeFetchHandler('tunnel') // because it has the local helper in closure scope. - const tunnelOccurrences = SERVER_SRC.match(/fetch: handle\.fetchTunnel/g); - expect(tunnelOccurrences).not.toBeNull(); - expect(tunnelOccurrences!.length).toBeGreaterThanOrEqual(2); + expect(SERVER_SRC).toContain('fetchHandler: handle.fetchTunnel'); + expect(SERVER_SRC).toContain('fetch: handle.fetchTunnel'); // The factory's internal makeFetchHandler('tunnel') still appears at least - // once for the /tunnel/start route's self-reference + the factory's return. + // once for the /tunnel/start route's startTunnel call + the factory's return. const internalOccurrences = SERVER_SRC.match(/makeFetchHandler\('tunnel'\)/g); expect(internalOccurrences).not.toBeNull(); }); @@ -244,16 +245,26 @@ describe('Tunnel listener lifecycle', () => { expect(helperBlock).toContain('tunnelServer.stop'); }); - test('/tunnel/start binds the tunnel listener on an ephemeral port', () => { + test('/tunnel/start binds the tunnel listener on an ephemeral port (via startTunnel)', () => { const startBlock = sliceBetween( SERVER_SRC, "url.pathname === '/tunnel/start' && req.method === 'POST'", "url.pathname === '/refs'" ); - expect(startBlock).toContain('Bun.serve'); - expect(startBlock).toContain('port: 0'); + // The route delegates to the shared startTunnel() helper, passing the + // factory-scoped tunnel-surface handler. + expect(startBlock).toContain('startTunnel('); expect(startBlock).toContain("makeFetchHandler('tunnel')"); - expect(startBlock).toContain("addr: tunnelPort"); + // The helper owns the ephemeral bind and points ngrok at the TUNNEL + // port — never the local daemon port. + const helperBlock = sliceBetween( + SERVER_SRC, + 'async function startTunnel(', + 'Module-level validateAuth deleted' + ); + expect(helperBlock).toContain('Bun.serve'); + expect(helperBlock).toContain('port: 0'); + expect(helperBlock).toContain("addr: tunnelPort"); }); test('/tunnel/start hard-fails on tunnel listener bind error (no local fallback)', () => { @@ -280,13 +291,22 @@ describe('Tunnel listener lifecycle', () => { }); test('/tunnel/start tears down tunnel listener when ngrok.forward fails', () => { + // startTunnel owns the error-path teardown: boundTunnel.stop(true) plus + // the ngrok listener close must both run on any post-bind failure, so a + // failed start can't leak sockets or an active ngrok session. + const helperBlock = sliceBetween( + SERVER_SRC, + 'async function startTunnel(', + 'Module-level validateAuth deleted' + ); + expect(helperBlock).toContain('boundTunnel.stop(true)'); + expect(helperBlock).toContain('tunnelListener.close()'); + // ...and the route maps that failure to the 500 response. const startBlock = sliceBetween( SERVER_SRC, "url.pathname === '/tunnel/start' && req.method === 'POST'", "url.pathname === '/refs'" ); - // boundTunnel.stop(true) must be called on ngrok error - expect(startBlock).toContain('boundTunnel.stop(true)'); expect(startBlock).toContain('Failed to open ngrok tunnel'); }); @@ -296,13 +316,22 @@ describe('Tunnel listener lifecycle', () => { "process.env.BROWSE_TUNNEL === '1'", 'start().catch' ); - expect(startupBlock).toContain('Bun.serve'); - expect(startupBlock).toContain('port: 0'); // v1.35.0.0: start() refactored to use handle.fetchTunnel from the factory. + // The ephemeral-port bind + ngrok forward now live in the shared + // startTunnel() helper the startup path delegates to. + expect(startupBlock).toContain('startTunnel('); expect(startupBlock).toContain('handle.fetchTunnel'); - expect(startupBlock).toContain('addr: tunnelPort'); - // Must NOT forward ngrok at the local port + // Must NOT forward ngrok at the local port — neither at the call site + // nor inside the helper, which binds port: 0 and forwards at tunnelPort. expect(startupBlock).not.toContain('addr: port,'); + const helperBlock = sliceBetween( + SERVER_SRC, + 'async function startTunnel(', + 'Module-level validateAuth deleted' + ); + expect(helperBlock).toContain('port: 0'); + expect(helperBlock).toContain('addr: tunnelPort'); + expect(helperBlock).not.toContain('addr: port,'); }); }); diff --git a/browse/test/server-sanitize-surrogates.test.ts b/browse/test/server-sanitize-surrogates.test.ts index d8abd1012..7513c05ca 100644 --- a/browse/test/server-sanitize-surrogates.test.ts +++ b/browse/test/server-sanitize-surrogates.test.ts @@ -2,23 +2,15 @@ import { describe, test, expect } from 'bun:test'; import * as fs from 'fs'; import * as path from 'path'; -// The sanitizer is module-private in server.ts. Rather than refactor it to a -// separate module just for testing, we extract its source via a regex slice and -// eval it in a fresh function scope. Keeps the production layout untouched. +// The sanitizer used to be module-private in server.ts (extracted here via a +// regex slice + eval). It now lives in sanitize.ts as the single source of +// truth for server.ts, sse-helpers.ts, and the read/snapshot pipeline — so +// this suite imports the canonical export and pins the server.ts wiring. +import { stripLoneSurrogates as sanitizeLoneSurrogates } from '../src/sanitize'; + const SERVER_PATH = path.resolve(import.meta.dir, '..', 'src', 'server.ts'); const SERVER_SRC = fs.readFileSync(SERVER_PATH, 'utf-8'); -const fnMatch = SERVER_SRC.match( - /function sanitizeLoneSurrogates\(str: string\): string \{[\s\S]*?\n\}/ -); -if (!fnMatch) throw new Error('Could not locate sanitizeLoneSurrogates in server.ts'); - -// Strip TS annotations so eval works under plain JS. -const jsSrc = fnMatch[0].replace('(str: string): string', '(str)'); -const sanitizeLoneSurrogates = new Function(`${jsSrc}\nreturn sanitizeLoneSurrogates;`)() as ( - s: string, -) => string; - describe('sanitizeLoneSurrogates — unit cases', () => { test('passthrough ASCII', () => { expect(sanitizeLoneSurrogates('hello')).toBe('hello'); @@ -110,7 +102,7 @@ describe('sanitizeLoneSurrogates — wiring invariants', () => { // refactor moves sanitization back to handleCommand only, this test // fails by detecting the missing wrapper. expect(SERVER_SRC).toContain('async function handleCommandInternalImpl('); - expect(SERVER_SRC).toContain('result: sanitizeLoneSurrogates(cr.result)'); + expect(SERVER_SRC).toContain('result: stripLoneSurrogates(cr.result)'); }); test('SSE activity feed routes outbound frames through createSseEndpoint', () => { @@ -142,16 +134,31 @@ describe('sanitizeLoneSurrogates — wiring invariants', () => { const helperSrc = fs.readFileSync(helperPath, 'utf-8'); expect(helperSrc).toContain('JSON.stringify('); expect(helperSrc).toContain('sanitizeReplacer'); - // The sanitizer itself uses stripLoneSurrogates (the shared utility in - // sanitize.ts) — not a private copy. Re-confirms the helper is wired - // to the canonical sanitizer, not a drift'd duplicate. - expect(helperSrc).toContain("import { stripLoneSurrogates } from './sanitize'"); + // The replacer is the canonical export from sanitize.ts — not a private + // copy. Re-confirms the helper is wired to the canonical sanitizer, not + // a drift'd duplicate. + expect(helperSrc).toContain("import { sanitizeReplacer } from './sanitize'"); }); - test('sanitizeReplacer is a function defined in server.ts (for non-SSE egress)', () => { - // server.ts keeps its own sanitizeReplacer for the non-SSE JSON egress - // paths (handleCommandInternal etc.). The SSE path uses sse-helpers.ts's - // own sanitizeReplacer; both must exist independently. - expect(SERVER_SRC).toContain('function sanitizeReplacer('); + test('sanitizeReplacer is the canonical export wrapping stripLoneSurrogates', () => { + // Single source of truth: sanitize.ts defines the one replacer, and it + // must wrap the shared stripLoneSurrogates (a fast-path rewrite that + // stops sanitizing string values would regress every JSON egress at once). + const sanitizePath = path.resolve(import.meta.dir, '..', 'src', 'sanitize.ts'); + const sanitizeSrc = fs.readFileSync(sanitizePath, 'utf-8'); + expect(sanitizeSrc).toContain('export function sanitizeReplacer('); + expect(sanitizeSrc).toContain( + "typeof value === 'string' ? stripLoneSurrogates(value) : value", + ); + }); + + test('server.ts imports sanitizeReplacer for non-SSE JSON egress and still uses it', () => { + // server.ts used to define its own private sanitizeReplacer for the + // non-SSE JSON egress paths (/pty-inject-scan, /memory snapshot, etc.). + // It now imports the canonical one — and must still pass it at those + // JSON.stringify egress sites. + expect(SERVER_SRC).toMatch(/import \{[^}]*sanitizeReplacer[^}]*\} from '\.\/sanitize'/); + expect(SERVER_SRC).not.toContain('function sanitizeReplacer('); + expect(SERVER_SRC).toContain(', sanitizeReplacer)'); }); });