Port Registrar: Local-view port registration + Origin gate#2376
Open
TalZaccai wants to merge 4 commits into
Open
Port Registrar: Local-view port registration + Origin gate#2376TalZaccai wants to merge 4 commits into
TalZaccai wants to merge 4 commits into
Conversation
…kdown)
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>
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>
948c6eb to
2b36099
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates how per-session “view” servers (browser views/PDF viewer, montage gallery, markdown preview + Yjs) register their loopback ports to avoid (agent, role) collisions, and adds Origin-based gating to reduce localhost cross-origin exposure.
Changes:
- Migrates view servers from
setLocalHostPortto explicitsessionContext.registerPort("view", port)with release plumbing in agents. - Adds
LOCAL_VIEW_ROLEfallback lookup in the dispatcher so callers that still assume"default"continue to work during migration. - Introduces per-package Origin allowlists and middleware/upgrade checks for the browser views, montage, and markdown view servers.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| ts/pnpm-lock.yaml | Updates lockfile for new workspace deps and updated dependency graph. |
| ts/packages/dispatcher/dispatcher/src/context/appAgentManager.ts | Adds "view" role fallback for local port lookups and documents the migration via LOCAL_VIEW_ROLE. |
| ts/packages/agents/montage/src/route/route.ts | Adds early Origin-gating middleware for the montage HTTP server. |
| ts/packages/agents/montage/src/route/originAllowlist.ts | Adds shared allowlist wrapper for montage view server Origin checks. |
| ts/packages/agents/montage/src/agent/montageActionHandler.ts | Registers montage view server port under role "view" and releases it on disable/close. |
| ts/packages/agents/montage/package.json | Adds websocket-utils dependency for shared Origin allowlist. |
| ts/packages/agents/markdown/src/view/route/service.ts | Adds Origin gate middleware and WS-upgrade gating for the markdown preview/Yjs server. |
| ts/packages/agents/markdown/src/view/route/originAllowlist.ts | Adds shared allowlist wrapper for markdown view server Origin checks. |
| ts/packages/agents/markdown/src/agent/markdownActionHandler.ts | Registers markdown view server port under role "view" and releases it on disable. |
| ts/packages/agents/markdown/package.json | Adds websocket-utils dependency for shared Origin allowlist. |
| ts/packages/agents/browser/src/views/server/core/originAllowlist.ts | Adds shared allowlist wrapper for browser views server Origin checks. |
| ts/packages/agents/browser/src/views/server/core/baseServer.ts | Adds early Origin-gating middleware for browser views server. |
| ts/packages/agents/browser/src/agent/browserActions.mts | Extends browser action context type to track view port registration handle. |
| ts/packages/agents/browser/src/agent/browserActionHandler.mts | Registers browser views server port under role "view" and releases it on disable/close. |
Files not reviewed (1)
- ts/pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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>
…s-phase-a # Conflicts: # ts/pnpm-lock.yaml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Two gaps motivated the change:
(browser, "default"); the views server'ssetLocalHostPortregistered another entry under the same(agent, role)slot, so whichever ran second overwrote the other.@system portsanddiscoverPort("browser")could return either depending on init order.http://localhost:<port>viafetch/XHR/iframe. The PDF viewer, image-gallery folder paths, and markdown live document + Yjs WS were all readable cross-origin to anything running on the same host.What
originAllowlist.tsper package (browser-views, montage, markdown): loopback-only HTTP(S); absent/null/empty Origin allowed for same-origin fetch and Nodewsclients. Markdown's Yjs WebSocket upgrade reuses the same allowlist.context.registerPort("view", port)after the child reports its bound port, stores the release handle onagentContext.viewPortRegistration, and releases it in the disable + close paths.setLocalHostPortcalls are removed.BrowserActionContext,MontageActionContext,MarkdownActionContext) gain aviewPortRegistration?: { release: () => void }field.appAgentManager.getLocalHostPortandgetSharedLocalHostPortfall back to role"view"after"default"so cross-agent site lookup (browserActionHandler.mts:1335) keeps working through the migration. A newLOCAL_VIEW_ROLEconstant documents the alias.Validation
pnpm --filter browser-typeagent --filter montage-agent --filter markdown-agent --filter agent-dispatcher build— clean.pnpm --filter ... prettier— clean.pnpm --filter agent-dispatcher test— 671 tests pass (coversportRegistrar.spec.js,appAgentManager-adjacent suites).pr-phase-a-manual-tests.md.Reading order
dispatcher/dispatcher/src/context/appAgentManager.ts— fallback lookup +LOCAL_VIEW_ROLEconstant.agents/browser/src/views/server/core/baseServer.ts+ neworiginAllowlist.ts— Origin middleware pattern.agents/browser/src/agent/browserActionHandler.mts—registerPort("view", ...)+ release plumbing (template for the other two).agents/montage/src/route/route.ts+ handler.agents/markdown/src/view/route/service.ts+ handler — note the WS upgrade gate alongside the HTTP middleware.