From f249764e4946fe371d519dde41e6dc36c07e7e15 Mon Sep 17 00:00:00 2001 From: Tom Riglar Date: Mon, 26 Oct 2026 08:20:44 +0000 Subject: [PATCH] fix(device): open URLs without a shell, and keep device calls across a session renewal openBrowser: Windows used `cmd /c start`, which parses the URL again, so the login URL's `&` split it and ran the rest as a command (CodeQL: uncontrolled command line). It now uses rundll32, and opens only http(s) URLs. dcd mcp: renewing a `dcd login` session closed the hosted connection under calls still running, so a long `run` failed with "Connection closed" while the flow carried on. The bridge now keeps one connection and puts the current auth on each request. Calls that arrive together share one connect. Co-Authored-By: Claude Opus 5.5 --- src/mcp/bridge.ts | 47 ++++++++++++++++++++++++------------ src/utils/browser.ts | 25 ++++++++++++++++--- test/unit/browser.test.ts | 11 +++++++++ test/unit/mcp-bridge.test.ts | 35 +++++++++++++++++++++++++-- 4 files changed, 97 insertions(+), 21 deletions(-) create mode 100644 test/unit/browser.test.ts diff --git a/src/mcp/bridge.ts b/src/mcp/bridge.ts index a716a7b..cd34889 100644 --- a/src/mcp/bridge.ts +++ b/src/mcp/bridge.ts @@ -124,31 +124,48 @@ export async function resolveLocalInputs( return args; } -/** The hosted device tools, connected lazily and remade when auth changes. */ +/** + * The hosted device tools, over one connection made on first use and shared + * by every call. + * + * Auth goes on each request, not on the connection. Remaking the connection + * when a session is renewed would close it under calls still running (a `run` + * can take minutes), and they would fail with "Connection closed" while the + * flow carried on. The hosted /mcp is stateless, so nothing ties the + * connection to the auth it started with. + */ class RemoteDevices { - private client: Client | null = null; - private auth: AuthContext | null = null; + private connecting: Promise | null = null; private tools: Tool[] | null = null; - async connect(): Promise<{ client: Client; ctx: { apiUrl: string; auth: AuthContext } }> { - const ctx = await getContext(); - if (this.client && this.auth === ctx.auth) return { client: this.client, ctx }; + /** Calls that arrive together share one connect. A failed one is retried by the next call. */ + connect(): Promise { + this.connecting ??= this.open().catch((error: unknown) => { + this.connecting = null; + throw error; + }); + return this.connecting; + } - await this.client?.close().catch(() => {}); + private async open(): Promise { + const { apiUrl } = await getContext(); const client = new Client({ name: 'dcd-mcp-bridge', version: getCliVersion() }); await client.connect( - new StreamableHTTPClientTransport(new URL(`${ctx.apiUrl}/mcp`), { - requestInit: { headers: { ...ctx.auth.headers } }, + new StreamableHTTPClientTransport(new URL(`${apiUrl}/mcp`), { + fetch: async (url, init) => { + const { auth } = await getContext(); + const headers = new Headers(init?.headers); + for (const [key, value] of Object.entries(auth.headers)) headers.set(key, value); + return fetch(url, { ...init, headers }); + }, }), ); - this.client = client; - this.auth = ctx.auth; - return { client, ctx }; + return client; } async list(): Promise { if (this.tools) return this.tools; - const { client } = await this.connect(); + const client = await this.connect(); const { tools } = await client.listTools(); this.tools = tools; return tools; @@ -224,8 +241,8 @@ export async function createBridgeServer( if (readOnly && !remote.isReadOnly(name)) { return errorResult(`${name} is not available in read-only mode.`); } - const { client, ctx } = await remote.connect(); - const remoteArgs = await resolveLocalInputs(name, args, ctx); + const client = await remote.connect(); + const remoteArgs = await resolveLocalInputs(name, args, await getContext()); return (await client.callTool({ name, arguments: remoteArgs }, undefined, options)) as CallToolResult; }); }); diff --git a/src/utils/browser.ts b/src/utils/browser.ts index e196034..627cae9 100644 --- a/src/utils/browser.ts +++ b/src/utils/browser.ts @@ -1,13 +1,30 @@ import { spawn } from 'node:child_process'; -/** Open `url` in the default browser. False if no opener could be started. */ +/** + * Open `url` in the default browser. False if it isn't an http(s) URL or no + * opener could be started. + * + * Only http(s) is opened, so a URL can't name a local file or start with `-` + * and be read as an option. No opener goes through a shell: Windows uses + * rundll32, because `cmd /c start` parses the URL again and treats its `&` + * as a command separator. + */ export function openBrowser(url: string): boolean { + let href: string; + try { + const parsed = new URL(url); + if (parsed.protocol !== 'http:' && parsed.protocol !== 'https:') return false; + href = parsed.href; + } catch { + return false; + } + const cmd = process.platform === 'darwin' - ? { bin: 'open', args: [url] } + ? { bin: 'open', args: [href] } : process.platform === 'win32' - ? { bin: 'cmd', args: ['/c', 'start', '""', url] } - : { bin: 'xdg-open', args: [url] }; + ? { bin: 'rundll32', args: ['url.dll,FileProtocolHandler', href] } + : { bin: 'xdg-open', args: [href] }; try { const child = spawn(cmd.bin, cmd.args, { detached: true, stdio: 'ignore' }); child.unref(); diff --git a/test/unit/browser.test.ts b/test/unit/browser.test.ts new file mode 100644 index 0000000..0934735 --- /dev/null +++ b/test/unit/browser.test.ts @@ -0,0 +1,11 @@ +import { expect } from 'chai'; + +import { openBrowser } from '../../src/utils/browser.js'; + +describe('openBrowser', () => { + it('opens nothing that is not an http(s) URL', () => { + expect(openBrowser('file:///etc/passwd')).to.equal(false); + expect(openBrowser('--help')).to.equal(false); + expect(openBrowser('not a url')).to.equal(false); + }); +}); diff --git a/test/unit/mcp-bridge.test.ts b/test/unit/mcp-bridge.test.ts index 32e2d2a..622cdbd 100644 --- a/test/unit/mcp-bridge.test.ts +++ b/test/unit/mcp-bridge.test.ts @@ -18,8 +18,14 @@ import { clearContextCache } from '../../src/mcp/context.js'; * A stand-in for the hosted /mcp: stateless Streamable HTTP, one server per * POST, as the API runs it. Records each call and the auth header it saw. */ -function startHosted(): Promise<{ calls: { args: unknown; auth?: string; name: string }[]; http: HttpServer; url: string }> { +function startHosted(): Promise<{ + calls: { args: unknown; auth?: string; name: string }[]; + http: HttpServer; + inits: number; + url: string; +}> { const calls: { args: unknown; auth?: string; name: string }[] = []; + const hosted = { calls, inits: 0 }; const http = createHttpServer(async (req, res) => { // Stateless, as the API: no GET stream, no session to delete. if (req.method !== 'POST') { @@ -30,6 +36,7 @@ function startHosted(): Promise<{ calls: { args: unknown; auth?: string; name: s const chunks: Buffer[] = []; for await (const c of req) chunks.push(c as Buffer); const body = JSON.parse(Buffer.concat(chunks).toString() || '{}'); + if (body.method === 'initialize') hosted.inits++; const auth = req.headers['x-app-api-key'] as string | undefined; const server = new McpServer({ name: 'hosted', version: '1' }); const record = (name: string) => async (args: unknown) => { @@ -69,7 +76,7 @@ function startHosted(): Promise<{ calls: { args: unknown; auth?: string; name: s return new Promise((resolve) => { http.listen(0, '127.0.0.1', () => { const { port } = http.address() as AddressInfo; - resolve({ calls, http, url: `http://127.0.0.1:${port}` }); + resolve(Object.assign(hosted, { http, url: `http://127.0.0.1:${port}` })); }); }); } @@ -127,6 +134,30 @@ describe('dcd mcp bridge', () => { await client.close(); }); + it('keeps a running call when the session is renewed, and sends the new auth', async () => { + const client = await connect(); + await client.listTools(); + const slow = client.callTool({ name: 'get_logs', arguments: { ms: 300 } }); + await new Promise((r) => setTimeout(r, 50)); + // As if a `dcd login` session were renewed: getContext resolves new auth. + process.env.DEVICE_CLOUD_API_KEY = 'renewed-key'; + clearContextCache(); + await client.callTool({ name: 'inspect_screen', arguments: {} }); + const result = await slow; + expect(result.isError).to.not.equal(true); + expect(JSON.stringify(result.content)).to.include('late'); + expect(hosted.calls).to.deep.equal([{ name: 'inspect_screen', args: {}, auth: 'renewed-key' }]); + expect(hosted.inits).to.equal(1); + await client.close(); + }); + + it('shares one connection between calls that arrive together', async () => { + const client = await connect(); + await Promise.all([client.listTools(), client.listTools(), client.callTool({ name: 'inspect_screen', arguments: {} })]); + expect(hosted.inits).to.equal(1); + await client.close(); + }); + it("applies the bridge's call timeout, not the SDK's 60s default", async () => { // The agent's own client allows long calls; only the bridge's limit applies. const client = await connect(300);