feat(browser-tools): add native Streamable HTTP MCP transport - #591
feat(browser-tools): add native Streamable HTTP MCP transport#591keithah wants to merge 2 commits into
Conversation
The MCP binary only spoke stdio, so hosts that need an HTTP URL had to run a gateway process or write their own server around registerMcpBrowserTools. Add --transport streamable-http alongside the default stdio transport, with --host, --port, and --path. One process now serves many clients. Each MCP session gets its own server, provider, and toolkit so clients never share browser state, and sessions default to a throwaway browser profile because concurrent clients cannot share one persistent Chromium profile. Reject unknown session IDs so a bogus header cannot allocate a browser, return 400 for malformed JSON, and cap request bodies at 1 MiB. Run the test job for browser-tools changes. The path filter only matched the libretto package, so the browser-tools suite never ran in CI.
|
@keithah is attempting to deploy a commit to the Libretto Team on Vercel. A member of the Team first needs to authorize it. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41778c4d47
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const transport = new StreamableHTTPServerTransport({ | ||
| sessionIdGenerator: randomUUID, | ||
| }); |
There was a problem hiding this comment.
When this unauthenticated server listens on loopback, a hostile page can rebind its own hostname to 127.0.0.1:40103, initialize an MCP session, and invoke the browser-control tools. StreamableHTTPServerTransport leaves Host and Origin checks off by default unless DNS-rebinding protection is enabled, so configure it with the allowed host values rather than relying on the loopback bind.
Useful? React with 👍 / 👎.
| options.authProfile === false || this.defaultAuthProfile === false | ||
| ? undefined | ||
| : (options.authProfile ?? | ||
| (provider.supportsAuthProfiles ? "default" : undefined)); | ||
| (this.defaultAuthProfile ?? | ||
| (provider.supportsAuthProfiles ? "default" : undefined))); |
There was a problem hiding this comment.
When the HTTP toolkit sets defaultAuthProfile: false, this condition resolves every request to undefined, including browser_open({ authProfile: "work" }). As a result, HTTP clients cannot use the named persistent profile that the new docs promise; apply the false default only when the caller omits authProfile, while explicit strings must take precedence.
Useful? React with 👍 / 👎.
| req: IncomingMessage, | ||
| res: ServerResponse, | ||
| ): Promise<void> { | ||
| const pathname = new URL(req.url ?? "/", `http://${options.host}`).pathname; |
There was a problem hiding this comment.
When the CLI receives a valid IPv6 bind address such as --host ::1, http.listen accepts it, but new URL(..., "http://::1") throws because IPv6 literals need brackets in URLs. Since this statement is also outside the request try, the first request rejects without a response; the advertised URL later has the same malformed form. Keep the raw address for listen and use a bracketed form when building URLs.
Useful? React with 👍 / 👎.
| await transport.handleRequest(req, res, body); | ||
| // The transport assigns the ID while handling initialize. | ||
| if (transport.sessionId) sessions.set(transport.sessionId, session); |
There was a problem hiding this comment.
When a client initializes a session, opens a browser, and then crashes or loses its network without sending DELETE, the stateful transport must keep working across HTTP connection loss and therefore does not call onclose. This map retains the transport, toolkit, and browser indefinitely, so repeated abandoned clients can exhaust local processes or leave paid cloud sessions running; track request activity and close sessions after an idle limit.
Useful? React with 👍 / 👎.
| async function closeSession(session: McpHttpSession): Promise<void> { | ||
| await session.transport.close().catch(() => undefined); | ||
| await session.server.close().catch(() => undefined); | ||
| await session.toolkit.dispose().catch(() => undefined); |
There was a problem hiding this comment.
When a provider fails to close a browser session, toolkit.dispose() resolves to a BrowserCleanupError value rather than rejecting, so this .catch does not handle it. The server then reports a successful close, and signal shutdown can exit while the local or cloud browser remains open; inspect the resolved value and surface or log the recovery step.
AGENTS.md reference: AGENTS.md:L44-L44
Useful? React with 👍 / 👎.
| if (listening) { | ||
| return new Error( | ||
| `Could not listen on ${options.host}:${options.port} (${errorMessage(listening)}). Pass a free --port, or stop the process already using it.`, | ||
| ); |
There was a problem hiding this comment.
When --host cannot resolve or names an address that is not assigned to this machine, listen returns errors such as ENOTFOUND or EADDRNOTAVAIL, but the message only tells the caller to change or free the port. Neither step fixes the failure, so include guidance to correct --host or use a local bind address based on the error code.
AGENTS.md reference: AGENTS.md:L69-L71
Useful? React with 👍 / 👎.
| closed ??= (async () => { | ||
| httpServer.closeAllConnections(); | ||
| await new Promise<void>((resolve) => httpServer.close(() => resolve())); |
There was a problem hiding this comment.
When a connection reaches the listener between closeAllConnections() and close(), the first call cannot destroy it and the second call waits for it to end. A new SSE request or slow body can therefore leave started.close() pending and prevent the SIGINT/SIGTERM handler from exiting; initiate httpServer.close() first, then force-close connections so no new socket can enter the set.
Useful? React with 👍 / 👎.
Enable DNS-rebinding protection. The server has no authentication, so a hostile page could rebind its own name to the listener and drive the browser tools; it now rejects Host headers other than the bound address. Expire idle HTTP sessions. The transport keeps a session alive across connection loss, so a client that crashed without sending DELETE held its browser open forever. Let an explicit authProfile override the HTTP default. The false default resolved every request to no profile, so HTTP clients could not reach a named persistent profile. Bracket IPv6 addresses when building URLs, so --host ::1 no longer throws on the first request and advertises a valid URL. Report cleanup failures from dispose, which resolves them as a value rather than rejecting, and point host bind failures at --host instead of --port. Close the listener before forcing connections shut, so a socket arriving mid-shutdown cannot keep the process alive.
|
Thanks — all seven findings were reproducible, and P1 — DNS rebinding. Confirmed the SDK leaves Host and Origin checks off by default. Now constructs the transport with P1 — abandoned sessions. Confirmed: P2 — explicit P2 — IPv6 bind addresses. Reproduced: P2 — cleanup errors returned as values. Right — P2 — host failures pointed at P2 — shutdown ordering. Fixed: Docs updated for the Host check and the idle timeout. Verification: 115 passed, 12 skipped; |
1 participant
Summary
The MCP binary only spoke stdio, so a host that needs a Streamable HTTP URL had to run a gateway process in front of it or write its own server around
registerMcpBrowserTools. This adds the transport to the binary.--transport streamable-httpalongside the defaultstdio, with--host,--port, and--path.DELETE, transport close, and process shutdown.stdio stays the default, so existing clients are unaffected.
{ "mcpServers": { "libretto-browser-tools": { "url": "http://127.0.0.1:40103/mcp" } } }Notes for review
Three things worth a look, each with a test:
POSTcarrying an unknownmcp-session-idand aninitializebody must not fall through to session creation, or any caller could spawn a browser per bogus header.--hostcan bind beyond loopback, so an unbounded body is a memory risk.The server binds to
127.0.0.1by default and has no authentication; the docs say to put it behind a proxy before binding wider.CI change
.github/workflows/cli-tests.yml— thetestjob's path filter only matchedpackages/libretto/*, so nopackages/browser-toolschange has been running the suite. The filter now coversbrowser-tools. This is why the new tests would otherwise be skipped on this PR.Verification
pnpm --dir packages/browser-tools test --run— 110 passed, 12 skipped.pnpm check:mirrors,pnpm --dir packages/browser-tools type-check,pnpm lint(oxlint --type-awareexit 0), andpnpm docs:buildall pass.browser_open,browser_exec, andDELETEcleanup.