diff --git a/browse/src/terminal-agent.ts b/browse/src/terminal-agent.ts index 2e39d99e40..0b9dc0698f 100644 --- a/browse/src/terminal-agent.ts +++ b/browse/src/terminal-agent.ts @@ -598,12 +598,10 @@ function buildServer() { // first that matches a known token. const protoHeader = req.headers.get('sec-websocket-protocol') || ''; let token: string | null = null; - let acceptedProtocol: string | null = null; for (const raw of protoHeader.split(',').map(s => s.trim()).filter(Boolean)) { const candidate = raw.startsWith('gstack-pty.') ? raw.slice('gstack-pty.'.length) : raw; if (validTokens.has(candidate)) { token = candidate; - acceptedProtocol = raw; break; } } @@ -632,13 +630,13 @@ function buildServer() { // sessionsById so /internal/restart and (Commit 3) re-attach // lookups can find it. const sessionId = validTokens.get(token) ?? null; + // No explicit Sec-WebSocket-Protocol echo: Bun >= 1.3 auto-echoes the + // first offered protocol in the 101 response, so setting the header + // here produced a DUPLICATE header — strict clients (Chromium, python + // websockets) reject the handshake per RFC 6455 and the sidebar + // terminal could never connect. Verified on Bun 1.3.6. const upgraded = server.upgrade(req, { data: { cookie: token, sessionId }, - // Echo the protocol back so the browser accepts the upgrade. - // Required when the client sends Sec-WebSocket-Protocol — the - // server MUST select one of the offered protocols, otherwise - // the browser closes the connection immediately. - ...(acceptedProtocol ? { headers: { 'Sec-WebSocket-Protocol': acceptedProtocol } } : {}), }); return upgraded ? undefined : new Response('upgrade failed', { status: 500 }); } diff --git a/browse/test/terminal-agent-integration.test.ts b/browse/test/terminal-agent-integration.test.ts index cdcbe8de56..c4a72c019d 100644 --- a/browse/test/terminal-agent-integration.test.ts +++ b/browse/test/terminal-agent-integration.test.ts @@ -227,6 +227,45 @@ describe('terminal-agent: PTY round-trip via real WebSocket (Cookie auth)', () = expect(resp.headers.get('sec-websocket-protocol')).toBe(`gstack-pty.${token}`); }); + test('upgrade response contains exactly ONE Sec-WebSocket-Protocol header', async () => { + // RFC 6455: the server MUST select at most one subprotocol. Bun >= 1.3 + // auto-echoes the first offered protocol in server.upgrade(), so a + // manual echo on top of that produced TWO Sec-WebSocket-Protocol + // headers — and strict clients (Chromium, python websockets) reject the + // handshake, leaving the sidebar terminal permanently disconnected. + // + // Headers.get() normalizes duplicates away, so this test handshakes + // over a raw socket and counts header lines in the response head. + const token = 'dup-proto-token-must-be-at-least-seventeen-chars'; + await grantToken(token); + + const head = await new Promise((resolve, reject) => { + const req = + 'GET /ws HTTP/1.1\r\n' + + `Host: 127.0.0.1:${agentPort}\r\n` + + 'Connection: Upgrade\r\n' + + 'Upgrade: websocket\r\n' + + 'Sec-WebSocket-Version: 13\r\n' + + 'Sec-WebSocket-Key: dGhlIHNhbXBsZSBub25jZQ==\r\n' + + `Sec-WebSocket-Protocol: gstack-pty.${token}\r\n` + + 'Origin: chrome-extension://test-extension-id\r\n' + + '\r\n'; + let buf = ''; + const socket = require('net').connect(agentPort, '127.0.0.1', () => socket.write(req)); + socket.setTimeout(5000, () => { socket.destroy(); reject(new Error('handshake timeout')); }); + socket.on('data', (chunk: Buffer) => { + buf += chunk.toString('utf8'); + const end = buf.indexOf('\r\n\r\n'); + if (end !== -1) { socket.destroy(); resolve(buf.slice(0, end)); } + }); + socket.on('error', reject); + }); + + expect(head).toContain('101'); + const protoLines = head.split('\r\n').filter(l => l.toLowerCase().startsWith('sec-websocket-protocol:')); + expect(protoLines).toEqual([`Sec-WebSocket-Protocol: gstack-pty.${token}`]); + }); + test('Sec-WebSocket-Protocol auth: rejects unknown token even with valid Origin', async () => { const resp = await fetch(`http://127.0.0.1:${agentPort}/ws`, { headers: { diff --git a/browse/test/terminal-agent.test.ts b/browse/test/terminal-agent.test.ts index d908052d21..450acc4e19 100644 --- a/browse/test/terminal-agent.test.ts +++ b/browse/test/terminal-agent.test.ts @@ -131,15 +131,18 @@ describe('Source-level guard: terminal-agent', () => { expect(wsHandler).toContain('validTokens.has'); }); - test('Sec-WebSocket-Protocol auth: strips gstack-pty. prefix and echoes back', () => { + test('Sec-WebSocket-Protocol auth: strips gstack-pty. prefix, no manual echo', () => { const wsHandler = AGENT_SRC.slice(AGENT_SRC.indexOf("if (url.pathname === '/ws')")); // Browsers send `Sec-WebSocket-Protocol: gstack-pty.`. The agent - // must strip the prefix before checking validTokens, AND echo the - // protocol back in the upgrade response — without the echo, the - // browser closes the connection immediately. + // must strip the prefix before checking validTokens. The protocol echo + // is Bun's job: Bun >= 1.3 auto-echoes the first offered protocol in the + // 101 response. A manual echo on top produced a DUPLICATE + // Sec-WebSocket-Protocol header, which strict clients (Chromium, python + // websockets) reject per RFC 6455 — the sidebar terminal could never + // connect. Pin the invariant: no manual echo in the upgrade call. expect(wsHandler).toContain("'gstack-pty.'"); - expect(wsHandler).toContain('Sec-WebSocket-Protocol'); - expect(wsHandler).toContain('acceptedProtocol'); + expect(wsHandler).toContain('sec-websocket-protocol'); + expect(wsHandler).not.toContain("headers: { 'Sec-WebSocket-Protocol'"); }); test('lazy spawn: claude PTY is spawned in message handler, not on upgrade', () => { @@ -152,8 +155,10 @@ describe('Source-level guard: terminal-agent', () => { ); expect(upgradeBlock).not.toContain('spawnClaude('); // Spawn must be invoked from the message handler (lazy on first byte). + // v1.44 routes both spawn triggers (explicit {type:"start"} text frame + // and the lazy binary-frame path) through the maybeSpawnPty helper. const messageHandler = AGENT_SRC.slice(AGENT_SRC.indexOf('message(ws, raw)')); - expect(messageHandler).toContain('spawnClaude('); + expect(messageHandler).toContain('maybeSpawnPty('); expect(messageHandler).toContain('!session.spawned'); }); diff --git a/extension/sidepanel-terminal.js b/extension/sidepanel-terminal.js index e6287abcae..7dae41c599 100644 --- a/extension/sidepanel-terminal.js +++ b/extension/sidepanel-terminal.js @@ -433,25 +433,15 @@ }); ro.observe(els.mount); - // IME composition handling for Korean/CJK input (issue #1272). - // Suppress partial jamo during composition; only send the final - // composed string on compositionend. Without this, Korean IME - // sends fragmented input or doubles characters. - let composing = false; - const ta = term.textarea; - if (ta) { - ta.addEventListener('compositionstart', () => { composing = true; }); - ta.addEventListener('compositionend', (e) => { - composing = false; - if (e.data && ws && ws.readyState === WebSocket.OPEN) { - ws.send(new TextEncoder().encode(e.data)); - } - }); - } - + // IME composition (Korean/CJK, issue #1272) is handled by xterm.js + // itself: partial jamo are suppressed while _isComposing, and the final + // composed string is emitted through onData once, asynchronously + // (setTimeout in _finalizeComposition). A previous local workaround + // sent e.data manually on compositionend — but xterm emits the same + // string one macrotask later, so every composed syllable went out + // TWICE. Do not re-add a manual compositionend send. term.onData((data) => { - if (composing) return; // suppress partial input events during IME composition if (ws && ws.readyState === WebSocket.OPEN) { ws.send(new TextEncoder().encode(data)); }