From 90388f2fc77c2f57c7656dfd5b042f6b24a64607 Mon Sep 17 00:00:00 2001 From: Tal Zaccai Date: Wed, 20 May 2026 15:29:16 -0700 Subject: [PATCH 1/3] phase A: register local-view ports + Origin gate (browser/montage/markdown) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Migrates the three local-view child-process servers off the legacy `setLocalHostPort` shim and onto explicit `registerPort("view", port)`, and adds an Origin allowlist on each listener. Why: - `setLocalHostPort` registers under role `default`, which already collided in the browser agent with the WS bridge's own `(browser, default)` slot — the views server overwrote the bridge port. Moving the views to role `view` frees `default` for the bridge and gives `@system ports` a meaningful role string. - The three view servers (PDF viewer, montage gallery, markdown preview + Yjs collab WS) bound on loopback but had no Origin gate, so any local web page could fetch/XHR/iframe the served content cross-origin. What: - New `originAllowlist.ts` per package (browser-views, montage, markdown): loopback-only HTTP(S); absent/null Origin allowed for same-origin fetch and Node clients. Markdown's Yjs WS upgrade reuses the same allowlist. - Each agent's view-handler now calls `context.registerPort("view", port)` after the child reports its bound port, stores the release handle, and releases it in the disable + close paths. - `BrowserActionContext`, `MontageActionContext`, `MarkdownActionContext` gain a `viewPortRegistration` field. - `appAgentManager.getLocalHostPort` and `getSharedLocalHostPort` fall back to role `view` after `default` so cross-agent site lookup keeps working through the migration. Validation: pnpm build + prettier clean across the four packages. agent-dispatcher: 671 tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../src/agent/browserActionHandler.mts | 8 +++- .../browser/src/agent/browserActions.mts | 4 ++ .../src/views/server/core/baseServer.ts | 17 ++++++++ .../src/views/server/core/originAllowlist.ts | 40 +++++++++++++++++++ .../src/agent/markdownActionHandler.ts | 10 ++++- .../src/view/route/originAllowlist.ts | 36 +++++++++++++++++ .../agents/markdown/src/view/route/service.ts | 36 +++++++++++++++++ .../montage/src/agent/montageActionHandler.ts | 15 ++++++- .../montage/src/route/originAllowlist.ts | 34 ++++++++++++++++ ts/packages/agents/montage/src/route/route.ts | 16 ++++++++ .../dispatcher/src/context/appAgentManager.ts | 29 +++++++++++++- 11 files changed, 240 insertions(+), 5 deletions(-) create mode 100644 ts/packages/agents/browser/src/views/server/core/originAllowlist.ts create mode 100644 ts/packages/agents/markdown/src/view/route/originAllowlist.ts create mode 100644 ts/packages/agents/montage/src/route/originAllowlist.ts diff --git a/ts/packages/agents/browser/src/agent/browserActionHandler.mts b/ts/packages/agents/browser/src/agent/browserActionHandler.mts index 1a412517e8..aceadf7a55 100644 --- a/ts/packages/agents/browser/src/agent/browserActionHandler.mts +++ b/ts/packages/agents/browser/src/agent/browserActionHandler.mts @@ -783,6 +783,8 @@ async function updateBrowserContext( if (context.agentContext.viewProcess) { context.agentContext.viewProcess.kill(); context.agentContext.viewProcess = undefined; + context.agentContext.viewPortRegistration?.release(); + context.agentContext.viewPortRegistration = undefined; // Reset to OS-assigned so a subsequent re-enable forks // server.mjs with arg "0" instead of the stale port. The // killed child may still hold the old port for a brief @@ -806,6 +808,8 @@ async function closeBrowserContext( if (context.agentContext.viewProcess) { context.agentContext.viewProcess.kill(); context.agentContext.viewProcess = undefined; + context.agentContext.viewPortRegistration?.release(); + context.agentContext.viewPortRegistration = undefined; context.agentContext.localHostPort = 0; } } @@ -2471,7 +2475,9 @@ async function createViewServiceHost( childProcess.on("message", function (message: any) { if (message?.type === "Success") { context.agentContext.localHostPort = message.port; - context.setLocalHostPort(message.port); + context.agentContext.viewPortRegistration?.release(); + context.agentContext.viewPortRegistration = + context.registerPort("view", message.port); resolve(childProcess); } else if (message === "Failure") { resolve(undefined); diff --git a/ts/packages/agents/browser/src/agent/browserActions.mts b/ts/packages/agents/browser/src/agent/browserActions.mts index b766c3c0f1..2ec2606c86 100644 --- a/ts/packages/agents/browser/src/agent/browserActions.mts +++ b/ts/packages/agents/browser/src/agent/browserActions.mts @@ -38,6 +38,10 @@ export type BrowserActionContext = { index: IndexData | undefined; viewProcess?: ChildProcess | undefined; localHostPort: number; + // Handle returned by sessionContext.registerPort for the views + // server (PDF viewer / per-session HTML views). Released on + // updateBrowserContext(false, ...) or closeAgentContext. + viewPortRegistration?: { release: () => void } | undefined; webFlowStore?: WebFlowStore | undefined; choiceManager?: ChoiceManager | undefined; lastInferredActions?: any[] | undefined; diff --git a/ts/packages/agents/browser/src/views/server/core/baseServer.ts b/ts/packages/agents/browser/src/views/server/core/baseServer.ts index 9474391730..440c28f4e1 100644 --- a/ts/packages/agents/browser/src/views/server/core/baseServer.ts +++ b/ts/packages/agents/browser/src/views/server/core/baseServer.ts @@ -7,6 +7,7 @@ import path from "path"; import { fileURLToPath } from "url"; import { FeatureConfig, ServerConfig, SSEManager } from "./types.js"; import { SSEManagerImpl } from "./sseManager.js"; +import { isAllowedViewOrigin } from "./originAllowlist.js"; import registerDebug from "debug"; const debug = registerDebug("typeagent:views:server:core"); @@ -37,6 +38,22 @@ export class BaseServer { * Setup core middleware */ private setupMiddleware(): void { + // Origin allowlist — runs before everything else so non-loopback + // requests get HTTP 403 without consuming rate-limit budget or + // touching feature routes. The bind is loopback-only, but a + // malicious local web page in any browser tab can still hit + // `http://localhost:` via fetch/XHR/iframe; the gate stops + // that. + this.app.use((req, res, next) => { + const origin = req.headers.origin; + if (isAllowedViewOrigin(origin)) { + next(); + return; + } + debug(`Rejecting request from origin ${origin}`); + res.status(403).send("Origin not allowed"); + }); + // Rate limiting const limiter = rateLimit({ windowMs: this.config.rateLimitWindow || 60000, diff --git a/ts/packages/agents/browser/src/views/server/core/originAllowlist.ts b/ts/packages/agents/browser/src/views/server/core/originAllowlist.ts new file mode 100644 index 0000000000..36369e7629 --- /dev/null +++ b/ts/packages/agents/browser/src/views/server/core/originAllowlist.ts @@ -0,0 +1,40 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +/** + * Origin allowlist for the browser views server (PDF viewer + other + * per-session HTML views forked as a child process by the browser + * agent). + * + * Distinct from `agents/browser/src/agent/originAllowlist.mts`, which + * gates the WS bridge used by the typeagent Chrome/Edge extension. The + * views server is consumed by: + * - the Electron shell's inline browser (origin + * `http(s)://localhost(:port)` / `127.0.0.1` / `[::1]`), + * - external loopback browser tabs the user opens manually, + * - same-origin XHR/fetch from the served HTML (Origin absent or the + * server's own loopback origin). + * + * No browser-extension scheme is accepted here — this listener does not + * back any extension UI. + * + * Anything else is rejected with HTTP 403 before the route handler runs. + */ +export function isAllowedViewOrigin(origin: string | undefined): boolean { + if (origin === undefined || origin === "" || origin === "null") { + return true; + } + try { + const u = new URL(origin); + if (u.protocol !== "http:" && u.protocol !== "https:") { + return false; + } + return ( + u.hostname === "localhost" || + u.hostname === "127.0.0.1" || + u.hostname === "[::1]" + ); + } catch { + return false; + } +} diff --git a/ts/packages/agents/markdown/src/agent/markdownActionHandler.ts b/ts/packages/agents/markdown/src/agent/markdownActionHandler.ts index a7f7255d32..fdb7a505ba 100644 --- a/ts/packages/agents/markdown/src/agent/markdownActionHandler.ts +++ b/ts/packages/agents/markdown/src/agent/markdownActionHandler.ts @@ -44,6 +44,10 @@ type MarkdownActionContext = { currentFileName?: string | undefined; viewProcess?: ChildProcess | undefined; localHostPort: number; + // Handle returned by sessionContext.registerPort for the markdown + // preview / Yjs WebSocket server. Released on + // updateMarkdownContext(false, ...). + viewPortRegistration?: { release: () => void } | undefined; }; async function handleUICommand( @@ -304,7 +308,9 @@ async function updateMarkdownContext( if (result) { context.agentContext.viewProcess = result.process; context.agentContext.localHostPort = result.port; - context.setLocalHostPort(result.port); + context.agentContext.viewPortRegistration?.release(); + context.agentContext.viewPortRegistration = + context.registerPort("view", result.port); } } } @@ -314,6 +320,8 @@ async function updateMarkdownContext( if (context.agentContext.viewProcess) { context.agentContext.viewProcess.kill(); context.agentContext.viewProcess = undefined; + context.agentContext.viewPortRegistration?.release(); + context.agentContext.viewPortRegistration = undefined; } } } diff --git a/ts/packages/agents/markdown/src/view/route/originAllowlist.ts b/ts/packages/agents/markdown/src/view/route/originAllowlist.ts new file mode 100644 index 0000000000..e9b7113b7d --- /dev/null +++ b/ts/packages/agents/markdown/src/view/route/originAllowlist.ts @@ -0,0 +1,36 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +/** + * Origin allowlist for the markdown view server (HTTP preview + Yjs + * collaboration WebSocket). + * + * Allowed: + * - `http(s)://localhost(:port)`, `http(s)://127.0.0.1(:port)`, and + * `http(s)://[::1](:port)` (loopback browser tabs and the Electron + * shell's inline browser). + * - **No Origin header** — same-origin XHR/fetch from the preview page + * itself, plus Node `ws`/HTTP clients. The server binds to localhost, + * so this is loopback-restricted at the OS level. + * + * Anything else is rejected with HTTP 403 (HTTP routes) or 403 on the + * upgrade response (WebSocket). + */ +export function isAllowedViewOrigin(origin: string | undefined): boolean { + if (origin === undefined || origin === "" || origin === "null") { + return true; + } + try { + const u = new URL(origin); + if (u.protocol !== "http:" && u.protocol !== "https:") { + return false; + } + return ( + u.hostname === "localhost" || + u.hostname === "127.0.0.1" || + u.hostname === "[::1]" + ); + } catch { + return false; + } +} diff --git a/ts/packages/agents/markdown/src/view/route/service.ts b/ts/packages/agents/markdown/src/view/route/service.ts index e611c764e0..38fae7f7db 100644 --- a/ts/packages/agents/markdown/src/view/route/service.ts +++ b/ts/packages/agents/markdown/src/view/route/service.ts @@ -18,6 +18,7 @@ import * as encoding from "lib0/encoding"; import * as decoding from "lib0/decoding"; import registerDebug from "debug"; import sanitizeFilename from "sanitize-filename"; +import { isAllowedViewOrigin } from "./originAllowlist.js"; const debug = registerDebug("typeagent:markdown:service"); @@ -26,6 +27,23 @@ const port = parseInt(process.argv[2]); if (isNaN(port)) { throw new Error("Port must be a number"); } + +// Origin allowlist — runs before everything else so non-loopback +// requests get HTTP 403 without consuming rate-limit budget or hitting +// the route handlers. The server binds to localhost, but any local +// browser tab can still hit `http://localhost:` via fetch/XHR; +// the gate stops cross-origin reads of the live document and uploaded +// files. The Y.js WebSocket upgrade is gated separately in +// createYjsWSServer. +app.use((req, res, next) => { + const origin = req.headers.origin; + if (isAllowedViewOrigin(origin)) { + next(); + return; + } + debug(`Rejecting request from origin ${origin}`); + res.status(403).send("Origin not allowed"); +}); const limiter = rateLimit({ windowMs: 60000, max: 100, // limit each IP to 100 requests per windowMs @@ -2211,6 +2229,24 @@ function createYjsWSServer(server: http.Server): WebSocketServer { server.on("upgrade", (request, socket, head) => { try { + // Origin gate — same allowlist as the HTTP middleware. The + // Yjs WS carries the full document content + edit stream, + // so loopback bind alone isn't enough; any local web page + // could otherwise open a WS to localhost: and read / + // mutate the live doc. + const origin = request.headers.origin as string | undefined; + if (!isAllowedViewOrigin(origin)) { + debug(`Rejecting WS upgrade from origin ${origin}`); + socket.write( + "HTTP/1.1 403 Forbidden\r\n" + + "Connection: close\r\n" + + "Content-Length: 0\r\n" + + "\r\n", + ); + socket.destroy(); + return; + } + // Extract room name from URL path const url = new URL( request.url || "/", diff --git a/ts/packages/agents/montage/src/agent/montageActionHandler.ts b/ts/packages/agents/montage/src/agent/montageActionHandler.ts index fd1c1abb0d..ec4bf7ae5f 100644 --- a/ts/packages/agents/montage/src/agent/montageActionHandler.ts +++ b/ts/packages/agents/montage/src/agent/montageActionHandler.ts @@ -60,6 +60,10 @@ type MontageActionContext = { imageCollection?: im.ImageCollection | undefined; viewProcess?: ChildProcess | undefined; localHostPort: number; + // Handle returned by sessionContext.registerPort for the gallery + // view server. Released on updateMontageContext(false, ...) or + // closeMontageContext. + viewPortRegistration?: { release: () => void } | undefined; searchSettings: { minScore: number; exactMatch: boolean; @@ -233,6 +237,8 @@ async function closeMontageContext( if (context.agentContext.viewProcess) { context.agentContext.viewProcess.kill(); context.agentContext.viewProcess = undefined; + context.agentContext.viewPortRegistration?.release(); + context.agentContext.viewPortRegistration = undefined; } } @@ -298,7 +304,12 @@ async function updateMontageContext( ); if (viewServiceResult) { agentContext.viewProcess = viewServiceResult.process; - context.setLocalHostPort(viewServiceResult.port); + agentContext.localHostPort = viewServiceResult.port; + agentContext.viewPortRegistration?.release(); + agentContext.viewPortRegistration = context.registerPort( + "view", + viewServiceResult.port, + ); } // send the folder info @@ -333,6 +344,8 @@ async function updateMontageContext( if (agentContext.viewProcess) { agentContext.viewProcess.kill(); agentContext.viewProcess = undefined; + agentContext.viewPortRegistration?.release(); + agentContext.viewPortRegistration = undefined; } } } diff --git a/ts/packages/agents/montage/src/route/originAllowlist.ts b/ts/packages/agents/montage/src/route/originAllowlist.ts new file mode 100644 index 0000000000..9408a7b7a5 --- /dev/null +++ b/ts/packages/agents/montage/src/route/originAllowlist.ts @@ -0,0 +1,34 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +/** + * Origin allowlist for the montage view server. + * + * Allowed: + * - `http(s)://localhost(:port)`, `http(s)://127.0.0.1(:port)`, and + * `http(s)://[::1](:port)` (loopback browser tabs and the Electron + * shell's inline browser). + * - **No Origin header** — same-origin XHR/fetch from the gallery page + * itself, plus Node `ws`/HTTP clients. The server binds to localhost, + * so this is loopback-restricted at the OS level. + * + * Anything else is rejected with HTTP 403 before the route handler runs. + */ +export function isAllowedViewOrigin(origin: string | undefined): boolean { + if (origin === undefined || origin === "" || origin === "null") { + return true; + } + try { + const u = new URL(origin); + if (u.protocol !== "http:" && u.protocol !== "https:") { + return false; + } + return ( + u.hostname === "localhost" || + u.hostname === "127.0.0.1" || + u.hostname === "[::1]" + ); + } catch { + return false; + } +} diff --git a/ts/packages/agents/montage/src/route/route.ts b/ts/packages/agents/montage/src/route/route.ts index 54d86bb371..f6565dbed2 100644 --- a/ts/packages/agents/montage/src/route/route.ts +++ b/ts/packages/agents/montage/src/route/route.ts @@ -10,6 +10,7 @@ import path from "path"; import sharp from "sharp"; import fs from "node:fs"; import registerDebug from "debug"; +import { isAllowedViewOrigin } from "./originAllowlist.js"; const debug = registerDebug("typeagent:agent:montage:route"); const app: Express = express(); @@ -33,6 +34,21 @@ let rootImageFolder: string; // The last message sent to the clients let lastMessage: any = {}; +// Origin allowlist — runs before everything else so non-loopback +// requests get HTTP 403 without consuming rate-limit budget or hitting +// the route handlers. The server binds to localhost, but any local +// browser tab can still hit `http://localhost:` via fetch/XHR; +// the gate stops cross-origin reads of indexed images and folders. +app.use((req, res, next) => { + const origin = req.headers.origin; + if (isAllowedViewOrigin(origin)) { + next(); + return; + } + debug(`Rejecting request from origin ${origin}`); + res.status(403).send("Origin not allowed"); +}); + // limit request rage const limiter = rateLimit({ windowMs: 1000, diff --git a/ts/packages/dispatcher/dispatcher/src/context/appAgentManager.ts b/ts/packages/dispatcher/dispatcher/src/context/appAgentManager.ts index 876252e914..ca885921f1 100644 --- a/ts/packages/dispatcher/dispatcher/src/context/appAgentManager.ts +++ b/ts/packages/dispatcher/dispatcher/src/context/appAgentManager.ts @@ -55,6 +55,19 @@ import { RegistrationId, } from "./portRegistrar.js"; +/** + * Role string used by agents that have migrated off the legacy + * `setLocalHostPort` shim and onto explicit + * `sessionContext.registerPort("view", port)` for their per-session + * local view (browser views, montage gallery, markdown preview). + * + * The legacy "default" role and this "view" role coexist: lookups for + * `getLocalHostPort` / `getSharedLocalHostPort` try "default" first + * (back-compat) and fall back to "view" (new pattern). This lets + * cross-agent site lookup keep working through the migration. + */ +const LOCAL_VIEW_ROLE = "view"; + const debug = registerDebug("typeagent:dispatcher:agents"); const debugError = registerDebug("typeagent:dispatcher:agents:error"); const debugLoad = registerDebug("typeagent:dispatcher:agents:load"); @@ -435,7 +448,13 @@ export class AppAgentManager implements ActionConfigProvider { } public getLocalHostPort(appAgentName: string) { - return this.portRegistrar.lookup(appAgentName, DEFAULT_ROLE); + // Fall back to the "view" role for agents migrated off the + // legacy `setLocalHostPort` shim (browser-views, montage, + // markdown). Lookup order matches `getSharedLocalHostPort`. + return ( + this.portRegistrar.lookup(appAgentName, DEFAULT_ROLE) ?? + this.portRegistrar.lookup(appAgentName, LOCAL_VIEW_ROLE) + ); } /** @@ -490,7 +509,13 @@ export class AppAgentManager implements ActionConfigProvider { ); } - const port = this.portRegistrar.lookup(target, DEFAULT_ROLE); + // Lookup order: "default" first for back-compat with legacy + // `setLocalHostPort` callers, then "view" for agents that + // migrated to the explicit `registerPort("view", ...)` pattern + // (browser-views, montage, markdown). + const port = + this.portRegistrar.lookup(target, DEFAULT_ROLE) ?? + this.portRegistrar.lookup(target, LOCAL_VIEW_ROLE); if (port === undefined) { throw new Error(`Local view not available for agent '${target}'.`); } From 2b360993689c255a7e6b1810010e29aaca335e5b Mon Sep 17 00:00:00 2001 From: Tal Zaccai Date: Thu, 21 May 2026 12:54:33 -0700 Subject: [PATCH 2/3] view servers: use shared originAllowlist helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Now that PR #2358 has landed the shared websocket-utils/originAllowlist helper, collapse the three per-view-server copies into one-line calls to createAgentOriginAllowlist() (no extension schemes — these listeners serve loopback browser tabs and the Electron shell's inline browser only). Behavior unchanged; the per-file isAllowedViewOrigin export name is preserved so call sites don't need updates. Adds websocket-utils as a dependency on markdown-agent and montage-agent (browser already depends on it). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../src/views/server/core/originAllowlist.ts | 24 +- ts/packages/agents/markdown/package.json | 1 + .../src/view/route/originAllowlist.ts | 33 +-- ts/packages/agents/montage/package.json | 1 + .../montage/src/route/originAllowlist.ts | 33 +-- ts/pnpm-lock.yaml | 217 ++---------------- 6 files changed, 44 insertions(+), 265 deletions(-) diff --git a/ts/packages/agents/browser/src/views/server/core/originAllowlist.ts b/ts/packages/agents/browser/src/views/server/core/originAllowlist.ts index 36369e7629..da3595a625 100644 --- a/ts/packages/agents/browser/src/views/server/core/originAllowlist.ts +++ b/ts/packages/agents/browser/src/views/server/core/originAllowlist.ts @@ -1,6 +1,8 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. +import { createAgentOriginAllowlist } from "websocket-utils/originAllowlist"; + /** * Origin allowlist for the browser views server (PDF viewer + other * per-session HTML views forked as a child process by the browser @@ -16,25 +18,9 @@ * server's own loopback origin). * * No browser-extension scheme is accepted here — this listener does not - * back any extension UI. + * back any extension UI. See {@link createAgentOriginAllowlist} for the + * shared loopback + no-Origin baseline. * * Anything else is rejected with HTTP 403 before the route handler runs. */ -export function isAllowedViewOrigin(origin: string | undefined): boolean { - if (origin === undefined || origin === "" || origin === "null") { - return true; - } - try { - const u = new URL(origin); - if (u.protocol !== "http:" && u.protocol !== "https:") { - return false; - } - return ( - u.hostname === "localhost" || - u.hostname === "127.0.0.1" || - u.hostname === "[::1]" - ); - } catch { - return false; - } -} +export const isAllowedViewOrigin = createAgentOriginAllowlist(); diff --git a/ts/packages/agents/markdown/package.json b/ts/packages/agents/markdown/package.json index dd4f885131..85d63086db 100644 --- a/ts/packages/agents/markdown/package.json +++ b/ts/packages/agents/markdown/package.json @@ -57,6 +57,7 @@ "telemetry": "workspace:*", "typechat": "^0.1.1", "unist-util-visit": "^4.1.2", + "websocket-utils": "workspace:*", "ws": "^8.20.1", "y-prosemirror": "^1.2.3", "y-protocols": "^1.0.5", diff --git a/ts/packages/agents/markdown/src/view/route/originAllowlist.ts b/ts/packages/agents/markdown/src/view/route/originAllowlist.ts index e9b7113b7d..cf478200eb 100644 --- a/ts/packages/agents/markdown/src/view/route/originAllowlist.ts +++ b/ts/packages/agents/markdown/src/view/route/originAllowlist.ts @@ -1,36 +1,19 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. +import { createAgentOriginAllowlist } from "websocket-utils/originAllowlist"; + /** * Origin allowlist for the markdown view server (HTTP preview + Yjs * collaboration WebSocket). * - * Allowed: - * - `http(s)://localhost(:port)`, `http(s)://127.0.0.1(:port)`, and - * `http(s)://[::1](:port)` (loopback browser tabs and the Electron - * shell's inline browser). - * - **No Origin header** — same-origin XHR/fetch from the preview page - * itself, plus Node `ws`/HTTP clients. The server binds to localhost, - * so this is loopback-restricted at the OS level. + * Accepts only the shared loopback + no-Origin baseline documented on + * {@link createAgentOriginAllowlist} (loopback browser tabs, the + * Electron shell's inline browser, same-origin XHR/fetch from the + * preview page, and Node `ws`/HTTP clients). The server binds to + * localhost, so this is loopback-restricted at the OS level. * * Anything else is rejected with HTTP 403 (HTTP routes) or 403 on the * upgrade response (WebSocket). */ -export function isAllowedViewOrigin(origin: string | undefined): boolean { - if (origin === undefined || origin === "" || origin === "null") { - return true; - } - try { - const u = new URL(origin); - if (u.protocol !== "http:" && u.protocol !== "https:") { - return false; - } - return ( - u.hostname === "localhost" || - u.hostname === "127.0.0.1" || - u.hostname === "[::1]" - ); - } catch { - return false; - } -} +export const isAllowedViewOrigin = createAgentOriginAllowlist(); diff --git a/ts/packages/agents/montage/package.json b/ts/packages/agents/montage/package.json index 9dd7b2ecd3..db474dacff 100644 --- a/ts/packages/agents/montage/package.json +++ b/ts/packages/agents/montage/package.json @@ -44,6 +44,7 @@ "sharp": "^0.33.5", "typeagent": "workspace:*", "typechat": "^0.1.1", + "websocket-utils": "workspace:*", "winreg": "^1.2.5" }, "devDependencies": { diff --git a/ts/packages/agents/montage/src/route/originAllowlist.ts b/ts/packages/agents/montage/src/route/originAllowlist.ts index 9408a7b7a5..5130866b17 100644 --- a/ts/packages/agents/montage/src/route/originAllowlist.ts +++ b/ts/packages/agents/montage/src/route/originAllowlist.ts @@ -1,34 +1,17 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. +import { createAgentOriginAllowlist } from "websocket-utils/originAllowlist"; + /** * Origin allowlist for the montage view server. * - * Allowed: - * - `http(s)://localhost(:port)`, `http(s)://127.0.0.1(:port)`, and - * `http(s)://[::1](:port)` (loopback browser tabs and the Electron - * shell's inline browser). - * - **No Origin header** — same-origin XHR/fetch from the gallery page - * itself, plus Node `ws`/HTTP clients. The server binds to localhost, - * so this is loopback-restricted at the OS level. + * Accepts only the shared loopback + no-Origin baseline documented on + * {@link createAgentOriginAllowlist} (loopback browser tabs, the + * Electron shell's inline browser, same-origin XHR/fetch from the + * gallery page, and Node `ws`/HTTP clients). The server binds to + * localhost, so this is loopback-restricted at the OS level. * * Anything else is rejected with HTTP 403 before the route handler runs. */ -export function isAllowedViewOrigin(origin: string | undefined): boolean { - if (origin === undefined || origin === "" || origin === "null") { - return true; - } - try { - const u = new URL(origin); - if (u.protocol !== "http:" && u.protocol !== "https:") { - return false; - } - return ( - u.hostname === "localhost" || - u.hostname === "127.0.0.1" || - u.hostname === "[::1]" - ); - } catch { - return false; - } -} +export const isAllowedViewOrigin = createAgentOriginAllowlist(); diff --git a/ts/pnpm-lock.yaml b/ts/pnpm-lock.yaml index 732f7ef20e..1af3cf4246 100644 --- a/ts/pnpm-lock.yaml +++ b/ts/pnpm-lock.yaml @@ -577,7 +577,7 @@ importers: version: 24.37.5(typescript@5.4.5) ts-node: specifier: ^10.9.1 - version: 10.9.2(@types/node@22.19.19)(typescript@5.4.5) + version: 10.9.2(@types/node@25.9.1)(typescript@5.4.5) xml2js: specifier: ^0.6.2 version: 0.6.2 @@ -2450,6 +2450,9 @@ importers: unist-util-visit: specifier: ^4.1.2 version: 4.1.2 + websocket-utils: + specifier: workspace:* + version: link:../../utils/webSocketUtils ws: specifier: ^8.20.1 version: 8.20.1 @@ -2562,6 +2565,9 @@ importers: typechat: specifier: ^0.1.1 version: 0.1.1(typescript@5.4.5)(zod@3.25.76) + websocket-utils: + specifier: workspace:* + version: link:../../utils/webSocketUtils winreg: specifier: ^1.2.5 version: 1.2.5 @@ -3678,7 +3684,7 @@ importers: version: link:../telemetry ts-node: specifier: ^10.9.1 - version: 10.9.2(@types/node@22.19.19)(typescript@5.4.5) + version: 10.9.2(@types/node@25.9.1)(typescript@5.4.5) typechat-utils: specifier: workspace:* version: link:../utils/typechatUtils @@ -3697,7 +3703,7 @@ importers: version: 29.5.14 jest: specifier: ^29.7.0 - version: 29.7.0(@types/node@22.19.19)(ts-node@10.9.2(@types/node@22.19.19)(typescript@5.4.5)) + version: 29.7.0(@types/node@25.9.1)(ts-node@10.9.2(@types/node@25.9.1)(typescript@5.4.5)) prettier: specifier: ^3.5.3 version: 3.5.3 @@ -3706,7 +3712,7 @@ importers: version: 6.0.1 ts-jest: specifier: ^29.4.9 - version: 29.4.9(@babel/core@7.28.4)(@jest/transform@29.7.0)(@jest/types@29.6.3)(babel-jest@29.7.0(@babel/core@7.28.4))(jest-util@29.7.0)(jest@29.7.0(@types/node@22.19.19)(ts-node@10.9.2(@types/node@22.19.19)(typescript@5.4.5)))(typescript@5.4.5) + version: 29.4.9(@babel/core@7.28.4)(@jest/transform@29.7.0)(@jest/types@29.6.3)(babel-jest@29.7.0(@babel/core@7.28.4))(jest-util@29.7.0)(jest@29.7.0(@types/node@25.9.1)(ts-node@10.9.2(@types/node@25.9.1)(typescript@5.4.5)))(typescript@5.4.5) typescript: specifier: ~5.4.5 version: 5.4.5 @@ -5250,7 +5256,7 @@ importers: devDependencies: '@electron-toolkit/tsconfig': specifier: ^1.0.1 - version: 1.0.1(@types/node@22.19.19) + version: 1.0.1(@types/node@25.9.1) '@fontsource/lato': specifier: ^5.2.5 version: 5.2.5 @@ -5286,10 +5292,10 @@ importers: version: 26.8.1(dmg-builder@26.8.1) electron-vite: specifier: ^4.0.1 - version: 4.0.1(vite@6.4.2(@types/node@22.19.19)(jiti@2.5.1)(less@4.3.0)(terser@5.39.2)(tsx@4.21.0)(yaml@2.8.3)) + version: 4.0.1(vite@6.4.2(@types/node@25.9.1)(jiti@2.5.1)(less@4.3.0)(terser@5.39.2)(tsx@4.21.0)(yaml@2.8.3)) jest: specifier: ^29.7.0 - version: 29.7.0(@types/node@22.19.19)(ts-node@10.9.2(@types/node@22.19.19)(typescript@5.4.5)) + version: 29.7.0(@types/node@25.9.1)(ts-node@10.9.2(@types/node@25.9.1)(typescript@5.4.5)) less: specifier: ^4.2.0 version: 4.3.0 @@ -5307,7 +5313,7 @@ importers: version: 5.4.5 vite: specifier: ^6.4.2 - version: 6.4.2(@types/node@22.19.19)(jiti@2.5.1)(less@4.3.0)(terser@5.39.2)(tsx@4.21.0)(yaml@2.8.3) + version: 6.4.2(@types/node@25.9.1)(jiti@2.5.1)(less@4.3.0)(terser@5.39.2)(tsx@4.21.0)(yaml@2.8.3) packages/telemetry: dependencies: @@ -18114,9 +18120,9 @@ snapshots: dependencies: electron: 40.8.5 - '@electron-toolkit/tsconfig@1.0.1(@types/node@22.19.19)': + '@electron-toolkit/tsconfig@1.0.1(@types/node@25.9.1)': dependencies: - '@types/node': 22.19.19 + '@types/node': 25.9.1 '@electron/asar@3.4.1': dependencies: @@ -18982,41 +18988,6 @@ snapshots: - supports-color - ts-node - '@jest/core@29.7.0(ts-node@10.9.2(@types/node@22.19.19)(typescript@5.4.5))': - dependencies: - '@jest/console': 29.7.0 - '@jest/reporters': 29.7.0 - '@jest/test-result': 29.7.0 - '@jest/transform': 29.7.0 - '@jest/types': 29.6.3 - '@types/node': 22.15.18 - ansi-escapes: 4.3.2 - chalk: 4.1.2 - ci-info: 3.9.0 - exit: 0.1.2 - graceful-fs: 4.2.11 - jest-changed-files: 29.7.0 - jest-config: 29.7.0(@types/node@22.15.18)(ts-node@10.9.2(@types/node@22.19.19)(typescript@5.4.5)) - jest-haste-map: 29.7.0 - jest-message-util: 29.7.0 - jest-regex-util: 29.6.3 - jest-resolve: 29.7.0 - jest-resolve-dependencies: 29.7.0 - jest-runner: 29.7.0 - jest-runtime: 29.7.0 - jest-snapshot: 29.7.0 - jest-util: 29.7.0 - jest-validate: 29.7.0 - jest-watcher: 29.7.0 - micromatch: 4.0.8 - pretty-format: 29.7.0 - slash: 3.0.0 - strip-ansi: 6.0.1 - transitivePeerDependencies: - - babel-plugin-macros - - supports-color - - ts-node - '@jest/core@29.7.0(ts-node@10.9.2(@types/node@25.9.1)(typescript@5.4.5))': dependencies: '@jest/console': 29.7.0 @@ -21568,7 +21539,6 @@ snapshots: '@types/node@25.9.1': dependencies: undici-types: 7.24.6 - optional: true '@types/normalize-package-data@2.4.4': {} @@ -23331,21 +23301,6 @@ snapshots: - supports-color - ts-node - create-jest@29.7.0(@types/node@22.19.19)(ts-node@10.9.2(@types/node@22.19.19)(typescript@5.4.5)): - dependencies: - '@jest/types': 29.6.3 - chalk: 4.1.2 - exit: 0.1.2 - graceful-fs: 4.2.11 - jest-config: 29.7.0(@types/node@22.19.19)(ts-node@10.9.2(@types/node@22.19.19)(typescript@5.4.5)) - jest-util: 29.7.0 - prompts: 2.4.2 - transitivePeerDependencies: - - '@types/node' - - babel-plugin-macros - - supports-color - - ts-node - create-jest@29.7.0(@types/node@25.9.1)(ts-node@10.9.2(@types/node@25.9.1)(typescript@5.4.5)): dependencies: '@jest/types': 29.6.3 @@ -24004,7 +23959,7 @@ snapshots: transitivePeerDependencies: - supports-color - electron-vite@4.0.1(vite@6.4.2(@types/node@22.19.19)(jiti@2.5.1)(less@4.3.0)(terser@5.39.2)(tsx@4.21.0)(yaml@2.8.3)): + electron-vite@4.0.1(vite@6.4.2(@types/node@25.9.1)(jiti@2.5.1)(less@4.3.0)(terser@5.39.2)(tsx@4.21.0)(yaml@2.8.3)): dependencies: '@babel/core': 7.28.4 '@babel/plugin-transform-arrow-functions': 7.27.1(@babel/core@7.28.4) @@ -24012,7 +23967,7 @@ snapshots: esbuild: 0.25.11 magic-string: 0.30.17 picocolors: 1.1.1 - vite: 6.4.2(@types/node@22.19.19)(jiti@2.5.1)(less@4.3.0)(terser@5.39.2)(tsx@4.21.0)(yaml@2.8.3) + vite: 6.4.2(@types/node@25.9.1)(jiti@2.5.1)(less@4.3.0)(terser@5.39.2)(tsx@4.21.0)(yaml@2.8.3) transitivePeerDependencies: - supports-color @@ -25825,25 +25780,6 @@ snapshots: - supports-color - ts-node - jest-cli@29.7.0(@types/node@22.19.19)(ts-node@10.9.2(@types/node@22.19.19)(typescript@5.4.5)): - dependencies: - '@jest/core': 29.7.0(ts-node@10.9.2(@types/node@22.19.19)(typescript@5.4.5)) - '@jest/test-result': 29.7.0 - '@jest/types': 29.6.3 - chalk: 4.1.2 - create-jest: 29.7.0(@types/node@22.19.19)(ts-node@10.9.2(@types/node@22.19.19)(typescript@5.4.5)) - exit: 0.1.2 - import-local: 3.2.0 - jest-config: 29.7.0(@types/node@22.19.19)(ts-node@10.9.2(@types/node@22.19.19)(typescript@5.4.5)) - jest-util: 29.7.0 - jest-validate: 29.7.0 - yargs: 17.7.2 - transitivePeerDependencies: - - '@types/node' - - babel-plugin-macros - - supports-color - - ts-node - jest-cli@29.7.0(@types/node@25.9.1)(ts-node@10.9.2(@types/node@25.9.1)(typescript@5.4.5)): dependencies: '@jest/core': 29.7.0(ts-node@10.9.2(@types/node@25.9.1)(typescript@5.4.5)) @@ -25894,37 +25830,6 @@ snapshots: - babel-plugin-macros - supports-color - jest-config@29.7.0(@types/node@22.15.18)(ts-node@10.9.2(@types/node@22.19.19)(typescript@5.4.5)): - dependencies: - '@babel/core': 7.28.4 - '@jest/test-sequencer': 29.7.0 - '@jest/types': 29.6.3 - babel-jest: 29.7.0(@babel/core@7.28.4) - chalk: 4.1.2 - ci-info: 3.9.0 - deepmerge: 4.3.1 - glob: 7.2.3 - graceful-fs: 4.2.11 - jest-circus: 29.7.0 - jest-environment-node: 29.7.0 - jest-get-type: 29.6.3 - jest-regex-util: 29.6.3 - jest-resolve: 29.7.0 - jest-runner: 29.7.0 - jest-util: 29.7.0 - jest-validate: 29.7.0 - micromatch: 4.0.8 - parse-json: 5.2.0 - pretty-format: 29.7.0 - slash: 3.0.0 - strip-json-comments: 3.1.1 - optionalDependencies: - '@types/node': 22.15.18 - ts-node: 10.9.2(@types/node@22.19.19)(typescript@5.4.5) - transitivePeerDependencies: - - babel-plugin-macros - - supports-color - jest-config@29.7.0(@types/node@22.15.18)(ts-node@10.9.2(@types/node@25.9.1)(typescript@5.4.5)): dependencies: '@babel/core': 7.28.4 @@ -25956,37 +25861,6 @@ snapshots: - babel-plugin-macros - supports-color - jest-config@29.7.0(@types/node@22.19.19)(ts-node@10.9.2(@types/node@22.19.19)(typescript@5.4.5)): - dependencies: - '@babel/core': 7.28.4 - '@jest/test-sequencer': 29.7.0 - '@jest/types': 29.6.3 - babel-jest: 29.7.0(@babel/core@7.28.4) - chalk: 4.1.2 - ci-info: 3.9.0 - deepmerge: 4.3.1 - glob: 7.2.3 - graceful-fs: 4.2.11 - jest-circus: 29.7.0 - jest-environment-node: 29.7.0 - jest-get-type: 29.6.3 - jest-regex-util: 29.6.3 - jest-resolve: 29.7.0 - jest-runner: 29.7.0 - jest-util: 29.7.0 - jest-validate: 29.7.0 - micromatch: 4.0.8 - parse-json: 5.2.0 - pretty-format: 29.7.0 - slash: 3.0.0 - strip-json-comments: 3.1.1 - optionalDependencies: - '@types/node': 22.19.19 - ts-node: 10.9.2(@types/node@22.19.19)(typescript@5.4.5) - transitivePeerDependencies: - - babel-plugin-macros - - supports-color - jest-config@29.7.0(@types/node@25.9.1)(ts-node@10.9.2(@types/node@25.9.1)(typescript@5.4.5)): dependencies: '@babel/core': 7.28.4 @@ -26266,18 +26140,6 @@ snapshots: - supports-color - ts-node - jest@29.7.0(@types/node@22.19.19)(ts-node@10.9.2(@types/node@22.19.19)(typescript@5.4.5)): - dependencies: - '@jest/core': 29.7.0(ts-node@10.9.2(@types/node@22.19.19)(typescript@5.4.5)) - '@jest/types': 29.6.3 - import-local: 3.2.0 - jest-cli: 29.7.0(@types/node@22.19.19)(ts-node@10.9.2(@types/node@22.19.19)(typescript@5.4.5)) - transitivePeerDependencies: - - '@types/node' - - babel-plugin-macros - - supports-color - - ts-node - jest@29.7.0(@types/node@25.9.1)(ts-node@10.9.2(@types/node@25.9.1)(typescript@5.4.5)): dependencies: '@jest/core': 29.7.0(ts-node@10.9.2(@types/node@25.9.1)(typescript@5.4.5)) @@ -29693,12 +29555,12 @@ snapshots: '@jest/types': 29.6.3 babel-jest: 29.7.0(@babel/core@7.28.4) - ts-jest@29.4.9(@babel/core@7.28.4)(@jest/transform@29.7.0)(@jest/types@29.6.3)(babel-jest@29.7.0(@babel/core@7.28.4))(jest-util@29.7.0)(jest@29.7.0(@types/node@22.19.19)(ts-node@10.9.2(@types/node@22.19.19)(typescript@5.4.5)))(typescript@5.4.5): + ts-jest@29.4.9(@babel/core@7.28.4)(@jest/transform@29.7.0)(@jest/types@29.6.3)(babel-jest@29.7.0(@babel/core@7.28.4))(jest-util@29.7.0)(jest@29.7.0(@types/node@25.9.1)(ts-node@10.9.2(@types/node@25.9.1)(typescript@5.4.5)))(typescript@5.4.5): dependencies: bs-logger: 0.2.6 fast-json-stable-stringify: 2.1.0 handlebars: 4.7.9 - jest: 29.7.0(@types/node@22.19.19)(ts-node@10.9.2(@types/node@22.19.19)(typescript@5.4.5)) + jest: 29.7.0(@types/node@25.9.1)(ts-node@10.9.2(@types/node@25.9.1)(typescript@5.4.5)) json5: 2.2.3 lodash.memoize: 4.1.2 make-error: 1.3.6 @@ -29752,24 +29614,6 @@ snapshots: yn: 3.1.1 optional: true - ts-node@10.9.2(@types/node@22.19.19)(typescript@5.4.5): - dependencies: - '@cspotcode/source-map-support': 0.8.1 - '@tsconfig/node10': 1.0.9 - '@tsconfig/node12': 1.0.11 - '@tsconfig/node14': 1.0.3 - '@tsconfig/node16': 1.0.4 - '@types/node': 22.19.19 - acorn: 8.15.0 - acorn-walk: 8.3.0 - arg: 4.1.3 - create-require: 1.1.1 - diff: 4.0.4 - make-error: 1.3.6 - typescript: 5.4.5 - v8-compile-cache-lib: 3.0.1 - yn: 3.1.1 - ts-node@10.9.2(@types/node@25.9.1)(typescript@5.4.5): dependencies: '@cspotcode/source-map-support': 0.8.1 @@ -29787,7 +29631,6 @@ snapshots: typescript: 5.4.5 v8-compile-cache-lib: 3.0.1 yn: 3.1.1 - optional: true tslib@1.14.1: {} @@ -29929,8 +29772,7 @@ snapshots: undici-types@7.24.4: {} - undici-types@7.24.6: - optional: true + undici-types@7.24.6: {} undici@6.25.0: {} @@ -30108,23 +29950,6 @@ snapshots: tsx: 4.21.0 yaml: 2.8.3 - vite@6.4.2(@types/node@22.19.19)(jiti@2.5.1)(less@4.3.0)(terser@5.39.2)(tsx@4.21.0)(yaml@2.8.3): - dependencies: - esbuild: 0.25.11 - fdir: 6.5.0(picomatch@4.0.3) - picomatch: 4.0.3 - postcss: 8.5.14 - rollup: 4.59.0 - tinyglobby: 0.2.15 - optionalDependencies: - '@types/node': 22.19.19 - fsevents: 2.3.3 - jiti: 2.5.1 - less: 4.3.0 - terser: 5.39.2 - tsx: 4.21.0 - yaml: 2.8.3 - vite@6.4.2(@types/node@25.9.1)(jiti@2.5.1)(less@4.3.0)(terser@5.39.2)(tsx@4.21.0)(yaml@2.8.3): dependencies: esbuild: 0.25.11 From bb9ff0433dc04c12f77581858553ba5d08568276 Mon Sep 17 00:00:00 2001 From: Tal Zaccai Date: Thu, 21 May 2026 16:01:38 -0700 Subject: [PATCH 3/3] view servers: tighten Origin gate + release port on child crash MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses PR #2376 review feedback from copilot-pull-request-reviewer across three concerns, each raised once per view-server agent: 1. Reject Origin: "null" for view servers. Add an llowNullOrigin option (default true for backwards compatibility) to the shared createAgentOriginAllowlist factory and opt out from all three view servers (browser views, markdown preview, montage gallery). These listeners only ever serve regular browser tabs and same-origin fetches, so an opaque-origin caller ( ile:// page, sandboxed iframe) is necessarily something we do not want to honor. 2. Normalize string[] Origin headers defensively. Node's parser joins repeated Origin headers into a single comma-separated string at runtime, but the TS header type is string | string[] | undefined. The factory predicate now accepts the wider type: a length-1 array is normalized to its sole entry; any other array length is rejected as ambiguous. 3. Release the per-session port registration if the view child process crashes. Each of the three action handlers now attaches an identity-guarded once("exit") listener at the call site that releases iewPortRegistration, clears iewProcess, and (for browser) resets localHostPort so respawn forks with arg "0" (OS-assigned) instead of trying to re-bind the stale port. The identity guard prevents a late-firing xit event on a previously-replaced process from clobbering a newer registration. Adds eight new tests in agentWebSocketServer.test.ts covering both sides of the llowNullOrigin toggle and the array normalization path. The existing agent-side test suite (which depends on the default llowNullOrigin: true) is unchanged and continues to pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../src/agent/browserActionHandler.mts | 27 +++++++- .../src/views/server/core/originAllowlist.ts | 9 ++- .../test/unit/agentWebSocketServer.test.ts | 67 +++++++++++++++++++ .../src/agent/markdownActionHandler.ts | 17 ++++- .../src/view/route/originAllowlist.ts | 9 ++- .../montage/src/agent/montageActionHandler.ts | 17 ++++- .../montage/src/route/originAllowlist.ts | 9 ++- .../webSocketUtils/src/originAllowlist.ts | 58 +++++++++++++--- 8 files changed, 198 insertions(+), 15 deletions(-) diff --git a/ts/packages/agents/browser/src/agent/browserActionHandler.mts b/ts/packages/agents/browser/src/agent/browserActionHandler.mts index aceadf7a55..2fbe1176a0 100644 --- a/ts/packages/agents/browser/src/agent/browserActionHandler.mts +++ b/ts/packages/agents/browser/src/agent/browserActionHandler.mts @@ -592,8 +592,31 @@ async function updateBrowserContext( } if (!context.agentContext.viewProcess) { - context.agentContext.viewProcess = - await createViewServiceHost(context); + const viewProcess = await createViewServiceHost(context); + if (viewProcess) { + context.agentContext.viewProcess = viewProcess; + // Defensive cleanup if the child crashes mid-session. + // The dispatcher's PortRegistrar leaves stale entries + // bounded to "until respawn or session end", but a + // crashed child should release its registration eagerly + // so the entry doesn't shadow a fresh bind. The + // identity guard prevents a late-firing `exit` event on + // a previously-replaced process from clobbering a newer + // registration; the explicit disable path (which also + // releases) is naturally idempotent under `?.release()`. + viewProcess.once("exit", () => { + if (context.agentContext.viewProcess !== viewProcess) { + return; + } + context.agentContext.viewPortRegistration?.release(); + context.agentContext.viewPortRegistration = undefined; + context.agentContext.viewProcess = undefined; + // Reset cached port so respawn forks with arg "0" + // (OS-assigned) instead of trying to re-bind the + // stale port — mirrors the disable/close paths. + context.agentContext.localHostPort = 0; + }); + } } if (context.agentContext.browserSchemaEnabled) { diff --git a/ts/packages/agents/browser/src/views/server/core/originAllowlist.ts b/ts/packages/agents/browser/src/views/server/core/originAllowlist.ts index da3595a625..a7937aa920 100644 --- a/ts/packages/agents/browser/src/views/server/core/originAllowlist.ts +++ b/ts/packages/agents/browser/src/views/server/core/originAllowlist.ts @@ -21,6 +21,13 @@ import { createAgentOriginAllowlist } from "websocket-utils/originAllowlist"; * back any extension UI. See {@link createAgentOriginAllowlist} for the * shared loopback + no-Origin baseline. * + * `Origin: "null"` (sent by `file://` pages and sandboxed iframes) is + * rejected — only regular browser tabs and same-origin fetches are + * legitimate clients, so an opaque-origin caller is necessarily + * something we do not want to honor. + * * Anything else is rejected with HTTP 403 before the route handler runs. */ -export const isAllowedViewOrigin = createAgentOriginAllowlist(); +export const isAllowedViewOrigin = createAgentOriginAllowlist({ + allowNullOrigin: false, +}); diff --git a/ts/packages/agents/browser/test/unit/agentWebSocketServer.test.ts b/ts/packages/agents/browser/test/unit/agentWebSocketServer.test.ts index adcde8c398..9e4696fde3 100644 --- a/ts/packages/agents/browser/test/unit/agentWebSocketServer.test.ts +++ b/ts/packages/agents/browser/test/unit/agentWebSocketServer.test.ts @@ -63,6 +63,7 @@ jest.mock("debug", () => { import { AgentWebSocketServer } from "../../src/agent/agentWebSocketServer.mjs"; import { isAllowedAgentOrigin } from "../../src/agent/originAllowlist.mjs"; +import { createAgentOriginAllowlist } from "websocket-utils/originAllowlist"; function makeMockSocket() { const handlers: Record = {}; @@ -425,3 +426,69 @@ describe("isAllowedAgentOrigin", () => { expect(isAllowedAgentOrigin("not a url")).toBe(false); }); }); + +// Direct tests of the shared factory cover the policy knobs we expose +// (`allowNullOrigin`, the `string[]` header normalization). The +// agent-side wrapper above tests the default `allowNullOrigin: true` +// branch as a side effect; these tests add explicit coverage for the +// view-server posture where `"null"` must be rejected. +describe("createAgentOriginAllowlist", () => { + describe("allowNullOrigin option", () => { + it('rejects Origin: "null" when allowNullOrigin is false', () => { + const allow = createAgentOriginAllowlist({ + allowNullOrigin: false, + }); + expect(allow("null")).toBe(false); + }); + it("still allows missing/empty Origin when allowNullOrigin is false", () => { + const allow = createAgentOriginAllowlist({ + allowNullOrigin: false, + }); + expect(allow(undefined)).toBe(true); + expect(allow("")).toBe(true); + }); + it("still allows loopback origins when allowNullOrigin is false", () => { + const allow = createAgentOriginAllowlist({ + allowNullOrigin: false, + }); + expect(allow("http://localhost:1234")).toBe(true); + expect(allow("http://127.0.0.1")).toBe(true); + expect(allow("http://[::1]:5173")).toBe(true); + }); + it('accepts Origin: "null" by default (backwards-compat)', () => { + const allow = createAgentOriginAllowlist(); + expect(allow("null")).toBe(true); + }); + it('accepts Origin: "null" when allowNullOrigin is explicitly true', () => { + const allow = createAgentOriginAllowlist({ + allowNullOrigin: true, + }); + expect(allow("null")).toBe(true); + }); + }); + describe("string[] header normalization", () => { + // Node's TS header types claim repeated headers may surface as + // `string[]`. At runtime the parser joins repeated Origin + // headers into a single comma-separated string, so the array + // form is not expected — but if it ever does arrive, anything + // other than a single entry is inherently ambiguous and must be + // rejected. + it("rejects an empty array", () => { + const allow = createAgentOriginAllowlist(); + expect(allow([])).toBe(false); + }); + it("rejects an array with more than one entry", () => { + const allow = createAgentOriginAllowlist(); + expect( + allow(["http://localhost", "https://evil.example.com"]), + ).toBe(false); + // Even two loopback entries are ambiguous and rejected. + expect(allow(["http://localhost", "http://127.0.0.1"])).toBe(false); + }); + it("normalizes a single-element array to that entry", () => { + const allow = createAgentOriginAllowlist(); + expect(allow(["http://localhost:1234"])).toBe(true); + expect(allow(["https://evil.example.com"])).toBe(false); + }); + }); +}); diff --git a/ts/packages/agents/markdown/src/agent/markdownActionHandler.ts b/ts/packages/agents/markdown/src/agent/markdownActionHandler.ts index fdb7a505ba..d4158d0385 100644 --- a/ts/packages/agents/markdown/src/agent/markdownActionHandler.ts +++ b/ts/packages/agents/markdown/src/agent/markdownActionHandler.ts @@ -306,11 +306,26 @@ async function updateMarkdownContext( context.agentContext.localHostPort, ); if (result) { - context.agentContext.viewProcess = result.process; + const viewProcess = result.process; + context.agentContext.viewProcess = viewProcess; context.agentContext.localHostPort = result.port; context.agentContext.viewPortRegistration?.release(); context.agentContext.viewPortRegistration = context.registerPort("view", result.port); + // Defensive cleanup if the child crashes mid-session. + // The identity guard prevents a late-firing `exit` + // event on a previously-replaced process from + // clobbering a newer registration; the explicit + // disable path (which also releases) is naturally + // idempotent under `?.release()`. + viewProcess.once("exit", () => { + if (context.agentContext.viewProcess !== viewProcess) { + return; + } + context.agentContext.viewPortRegistration?.release(); + context.agentContext.viewPortRegistration = undefined; + context.agentContext.viewProcess = undefined; + }); } } } diff --git a/ts/packages/agents/markdown/src/view/route/originAllowlist.ts b/ts/packages/agents/markdown/src/view/route/originAllowlist.ts index cf478200eb..ce4e850ff3 100644 --- a/ts/packages/agents/markdown/src/view/route/originAllowlist.ts +++ b/ts/packages/agents/markdown/src/view/route/originAllowlist.ts @@ -13,7 +13,14 @@ import { createAgentOriginAllowlist } from "websocket-utils/originAllowlist"; * preview page, and Node `ws`/HTTP clients). The server binds to * localhost, so this is loopback-restricted at the OS level. * + * `Origin: "null"` (sent by `file://` pages and sandboxed iframes) is + * rejected — the preview server only ever serves regular browser tabs + * and same-origin fetches, so an opaque-origin caller is necessarily + * something we do not want to honor. + * * Anything else is rejected with HTTP 403 (HTTP routes) or 403 on the * upgrade response (WebSocket). */ -export const isAllowedViewOrigin = createAgentOriginAllowlist(); +export const isAllowedViewOrigin = createAgentOriginAllowlist({ + allowNullOrigin: false, +}); diff --git a/ts/packages/agents/montage/src/agent/montageActionHandler.ts b/ts/packages/agents/montage/src/agent/montageActionHandler.ts index ec4bf7ae5f..a9df81d835 100644 --- a/ts/packages/agents/montage/src/agent/montageActionHandler.ts +++ b/ts/packages/agents/montage/src/agent/montageActionHandler.ts @@ -303,13 +303,28 @@ async function updateMontageContext( agentContext.localHostPort, ); if (viewServiceResult) { - agentContext.viewProcess = viewServiceResult.process; + const viewProcess = viewServiceResult.process; + agentContext.viewProcess = viewProcess; agentContext.localHostPort = viewServiceResult.port; agentContext.viewPortRegistration?.release(); agentContext.viewPortRegistration = context.registerPort( "view", viewServiceResult.port, ); + // Defensive cleanup if the child crashes mid-session. + // The identity guard prevents a late-firing `exit` + // event on a previously-replaced process from + // clobbering a newer registration; the explicit + // disable path (which also releases) is naturally + // idempotent under `?.release()`. + viewProcess.once("exit", () => { + if (agentContext.viewProcess !== viewProcess) { + return; + } + agentContext.viewPortRegistration?.release(); + agentContext.viewPortRegistration = undefined; + agentContext.viewProcess = undefined; + }); } // send the folder info diff --git a/ts/packages/agents/montage/src/route/originAllowlist.ts b/ts/packages/agents/montage/src/route/originAllowlist.ts index 5130866b17..f2f245e22d 100644 --- a/ts/packages/agents/montage/src/route/originAllowlist.ts +++ b/ts/packages/agents/montage/src/route/originAllowlist.ts @@ -12,6 +12,13 @@ import { createAgentOriginAllowlist } from "websocket-utils/originAllowlist"; * gallery page, and Node `ws`/HTTP clients). The server binds to * localhost, so this is loopback-restricted at the OS level. * + * `Origin: "null"` (sent by `file://` pages and sandboxed iframes) is + * rejected — the gallery only ever serves regular browser tabs and + * same-origin fetches, so an opaque-origin caller is necessarily + * something we do not want to honor. + * * Anything else is rejected with HTTP 403 before the route handler runs. */ -export const isAllowedViewOrigin = createAgentOriginAllowlist(); +export const isAllowedViewOrigin = createAgentOriginAllowlist({ + allowNullOrigin: false, +}); diff --git a/ts/packages/utils/webSocketUtils/src/originAllowlist.ts b/ts/packages/utils/webSocketUtils/src/originAllowlist.ts index fa94e6c19a..134839eb73 100644 --- a/ts/packages/utils/webSocketUtils/src/originAllowlist.ts +++ b/ts/packages/utils/webSocketUtils/src/originAllowlist.ts @@ -13,9 +13,15 @@ * parser preserves IPv6 brackets in `hostname`, so we match the * bracketed form, while the unbracketed form is accepted defensively * against URL parser/serializer differences across runtimes). - * - A missing/empty/`"null"` Origin is allowed — Node `ws` clients and - * other non-browser callers don't send Origin, and the listener binds - * to loopback so this remains OS-level restricted. + * - A missing/empty Origin is allowed — Node `ws` clients and other + * non-browser callers don't send Origin, and the listener binds to + * loopback so this remains OS-level restricted. + * - An `Origin: "null"` header (sent by `file://` pages and sandboxed/ + * opaque-origin documents) is allowed only when + * {@link OriginAllowlistOptions.allowNullOrigin} is `true` (the + * default, for backwards compatibility with agent-side bridges that + * may see opaque-origin clients). View-server gates that only ever + * serve browser tabs should set this to `false`. * - All other `http(s)` origins are rejected; non-`http(s)` schemes are * rejected unless explicitly named in {@link extensionSchemes}. * @@ -25,6 +31,14 @@ * matched as a literal prefix against the Origin string (e.g. * `"chrome-extension://"` accepts `chrome-extension://abc123`). * + * The returned predicate accepts the raw Node header value type + * (`string | string[] | undefined`). In practice Node combines repeated + * Origin headers into a single comma-joined string at the parser level, + * so the array form is not expected at runtime — but if it ever + * appears, an array of length other than 1 is rejected outright and a + * single-element array is normalized to its sole entry. Repeated + * Origins are inherently ambiguous and should not be trusted. + * * Returned predicates are pure and reusable; build once per agent and * call from the WS server's `verifyClient` so denied clients are * rejected with HTTP 403 before the `connection` event fires. @@ -37,6 +51,15 @@ export type OriginAllowlistOptions = { * include the trailing `://`. */ extensionSchemes?: readonly string[]; + /** + * Whether to accept `Origin: "null"` (the opaque-origin sentinel + * sent by `file://` pages and sandboxed iframes). Defaults to + * `true` for backwards compatibility with agent-side bridges. View + * servers that only intend to serve same-origin browser tabs should + * set this to `false` so a malicious `file://` page can't read + * loopback responses. + */ + allowNullOrigin?: boolean; }; /** @@ -46,21 +69,40 @@ export type OriginAllowlistOptions = { */ export function createAgentOriginAllowlist( options: OriginAllowlistOptions = {}, -): (origin: string | undefined) => boolean { +): (origin: string | string[] | undefined) => boolean { const schemes = options.extensionSchemes ?? []; - return (origin: string | undefined): boolean => { - if (origin === undefined || origin === "" || origin === "null") { + const allowNullOrigin = options.allowNullOrigin ?? true; + return (origin: string | string[] | undefined): boolean => { + // Node's header types claim repeated headers may surface as + // `string[]`. In practice the parser joins repeated Origin + // headers into a single comma-separated string, but if an array + // ever does arrive, reject anything other than a single entry — + // multiple Origins are inherently ambiguous and the safer + // posture is to drop the request. + let header: string | undefined; + if (Array.isArray(origin)) { + if (origin.length !== 1) { + return false; + } + header = origin[0]; + } else { + header = origin; + } + if (header === undefined || header === "") { // No Origin header: legitimate for Node `ws` and other // non-browser clients. return true; } + if (header === "null") { + return allowNullOrigin; + } for (const scheme of schemes) { - if (origin.startsWith(scheme)) { + if (header.startsWith(scheme)) { return true; } } try { - const u = new URL(origin); + const u = new URL(header); if (u.protocol !== "http:" && u.protocol !== "https:") { return false; }