CalvinBackup/gstack
1
0
Fork 0
mirror of https://github.com/garrytan/gstack.git synced 2026-09-15 09:25:28 +02:00
Code Issues Packages Projects Releases Wiki Activity

fix(server): one lone-surrogate sanitizer, one sanitizeReplacer, one startTunnel

Browse Source 
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 <noreply@anthropic.com>
This commit is contained in:
Garry Tan
2026-08-14 21:12:21 -07:00
co-authored by Claude Fable 5
parent 60134c4320
commit e426293017
5 changed files with 231 additions and 195 deletions
Download Patch File Download Diff File Expand all files Collapse all files
browse/test/dual-listener.test.ts
+45 -16
View File
@@ -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,');
});
});
browse/test/server-sanitize-surrogates.test.ts
+31 -24
View File
@@ -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)');
});
});