[PR #1645] [CLOSED] fix(browser): bound CDP message payloads to the shared protocol budget #4723

Closed
opened 2026-10-31 00:54:23 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/1645
Author: @DeryFerd
Created: 10/10/2026
Status: ❌ Closed

Base: main ← Head: fix/cdp-default-payload-ceiling


📝 Commits (1)

  • 3d65f3e fix(browser): bound CDP message payloads to the shared protocol budget

📊 Changes

2 files changed (+43 additions, -2 deletions)

View changed files

📝 src/browser/CdpConnection.ts (+8 -2)
📝 tests/boundary/browser/cdpConnection.test.ts (+35 -0)

📄 Description

Summary

CdpConnection.connect is the single transport owner for every CDP WebSocket REA opens, and it defaulted maxPayload to 0. In ws, 0 means unlimited, and because the value is passed explicitly, it also overrides the 100 MiB default ws would otherwise apply, so the receiver guard was off for every connection that did not select a limit. All page-level paths fall into that group: the four CdpBrowserProvider operations (page observation, WebMCP discovery, and their session variants) plus CdpElectronProvider call openCdpTargetSession without limits, which means page-controlled content could deliver a single message of arbitrary size before any of REA's capture accounting saw a byte. This PR makes an omitted selection fall back to WEB_RUNTIME_LIMITS.protocolBytes (64 MiB), the same protocol budget the runtime session already selects explicitly, while explicit per-session budgets stay authoritative and the existing typed payload_limit close reason is unchanged. No issue is linked because this is standalone hardening at the transport owner rather than a report-driven fix.

Problem and expected behavior

Smallest trigger: open a page-scoped CDP connection through openCdpTargetSession without a limits argument, then have the target deliver one message larger than any sane protocol budget. Prior behavior: the message is accepted. This is reproduced on the base revision with a fake CDP reply of WEB_RUNTIME_LIMITS.protocolBytes + 1 bytes: the pending Runtime.enable command resolved with the oversized payload instead of rejecting. The violated invariant: page-controlled content must not decide how much memory the host retains for one message, and an over-budget frame should surface as the same typed transport failure the explicit-limit path already produces. Expected behavior: the receiver refuses the frame, the connection reports payload_limit, and the user-visible message names the selected budget in bytes, exactly like the existing maxPayloadBytes: 512 path does today.

Change and scope

The change is one owner edit in CdpConnection.connect (src/browser/CdpConnection.ts): the selected budget is resolved once as limits?.maxPayloadBytes ?? WEB_RUNTIME_LIMITS.protocolBytes and used for both the ws option and the connection instance, with a comment recording why an explicit 0 must not survive. This fits the architecture by normalizing at the boundary that owns the socket instead of editing five scattered call sites: any future caller that forgets to pass limits is bounded by default, and callers that do pass a budget (runtime sessions at 64 MiB, the V8 inspector at 16 MiB) are untouched. Intentionally not included: no per-operation budget tuning, no change to observation or capture budgets such as WEB_NETWORK_CAPTURE_LIMITS, no edits to CdpBrowserProvider or CdpElectronProvider, and no new configuration knob, because the existing limits parameter remains the override for any session that needs a different ceiling.

Contract and boundary impact

  • Semantic owner and earliest changed stage: browser provider transport, CdpConnection.connect in src/browser/CdpConnection.ts; domain schemas and application workflows untouched.
  • CLI and MCP/tool-catalog contract: none. No tool, schema, annotation, or catalog change; the error surface reuses the existing payload_limit transport reason already projected by BrowserObservationError.
  • Provider, bridge, target-format, or platform compatibility: applies uniformly to Chromium, Electron, and Playwright-managed page sockets through the shared ws receiver guard; no platform-specific code and no bridge change.
  • Evidence, artifact, provenance, or reconstruction contract: none. The refusal happens at frame receipt before message text is parsed or retained, so no identity, digest, or coverage projection changes.
  • Process execution, authorization, cleanup, or containment impact: oversize frames are refused by ws (surfacing as WS_ERR_UNSUPPORTED_MESSAGE_LENGTH, already mapped to payload_limit); no new process, network, or authority surface, and cleanup paths are unchanged.
  • Generated metadata (docs/public/product-catalog.json), package, or installation impact (CI generates and validates ignored catalogs and skills/; edit authored instructions in skill-src/ and never commit build outputs): none. No contract, catalog, or skill input changed, and no build output is committed.

Evidence and regression coverage

  • Tests added or updated: one new case in tests/boundary/browser/cdpConnection.test.ts ("bounds command payloads to the shared protocol budget when no limits are selected") connects without limits, receives a reply of WEB_RUNTIME_LIMITS.protocolBytes + 1 bytes from the fake CDP browser, and asserts rejection with reason: "payload_limit" and a user message containing 67108864 byte protocol budget. The payload-budget tests now live in their own describe block so the callback stays within the repository's 150-line function limit, and the existing explicit-512 case is unchanged as the regression guard for the override path.
  • Base reproduction or other evidence: on base 0fa0b046 the new case fails with "promise resolved instead of rejecting", proving the default path accepted the oversize frame; after the change the same case passes and the full cdpConnection.test.ts file reports 11/11.
  • User-visible CLI/MCP output: unchanged in shape. Default connections now emit the same payload_limit failure the explicit path already produces, and the message names the 67108864-byte budget when the shared default is what refused the frame.
  • Remaining proof gaps: no real Chrome or Electron run was performed locally, and the ceiling reuses the maintainer-selected WEB_RUNTIME_LIMITS.protocolBytes value rather than a measurement from a live browser; realistic CDP messages such as screenshots and DOM snapshots sit far below that budget.

For evidence-bearing changes:

  • Observed, derived, and inferred claims remain distinguishable.
  • Artifact identity, source provenance, and failed attempts remain preserved.
  • Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.

Validation performed

  • npx vitest run tests/boundary/browser/cdpConnection.test.ts — 11 passed, including the new default-ceiling case, after rebasing onto be75ed5e.
  • npx vitest run tests/boundary/browser/ — 35 files passed; browserProcessStartup.test.ts, historicalCaptureDecoder.test.ts, and historicalWebCapture.test.ts fail 17 tests on this Windows host identically with and without the change (verified by stashing the diff and re-running the pristine base), so they are environmental and pre-existing.
  • npm run typecheck — exit 0 on the rebased branch.
  • npx oxlint on both changed files — 0 warnings, 0 errors; oxfmt --check on the committed (LF) form — clean.
  • Real browser lane (npm run verify:browser): not run locally because no Chrome runtime is available here; the fake-CDP boundary suite covers the changed connection path instead.

Compatibility, safety, and release

  • Breaking changes or migration steps: none for realistic traffic. Single messages above 64 MiB on connections that previously had no ceiling now fail with the typed payload_limit error instead of loading, which is the intended behavior change; sessions that need another ceiling already pass limits explicitly.
  • Real Hopper/Ghidra, browser, or OS coverage: not performed; the change is transport-level and platform-neutral, and the Windows-host boundary suites above exercised it.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: the change removes a page-controlled memory amplification path on the host that serves MCP clients, adds no network, process, or authority surface, and does not persist or transmit frame content.

Review checklist

  • This PR addresses a concrete problem or an agreed enhancement.
  • The PR has one focused outcome and the title follows type(scope): outcome.
  • Related issue is linked, or the reason for not linking one is stated above.
  • Tests cover changed observable behavior and meaningful failure paths.
  • Owning docs, contracts, and generated metadata are updated where needed.
  • User-visible CLI/MCP changes include representative output.
  • I checked the final diff for secrets, unrelated cleanup, and unsupported claims.

🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/1645 **Author:** [@DeryFerd](https://github.com/DeryFerd) **Created:** 10/10/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/cdp-default-payload-ceiling` --- ### 📝 Commits (1) - [`3d65f3e`](https://github.com/morluto/rea/commit/3d65f3e09d3d8543df9c1dc58d6e638af908cf0f) fix(browser): bound CDP message payloads to the shared protocol budget ### 📊 Changes **2 files changed** (+43 additions, -2 deletions) <details> <summary>View changed files</summary> 📝 `src/browser/CdpConnection.ts` (+8 -2) 📝 `tests/boundary/browser/cdpConnection.test.ts` (+35 -0) </details> ### 📄 Description ## Summary `CdpConnection.connect` is the single transport owner for every CDP WebSocket REA opens, and it defaulted `maxPayload` to `0`. In `ws`, `0` means unlimited, and because the value is passed explicitly, it also overrides the 100 MiB default `ws` would otherwise apply, so the receiver guard was off for every connection that did not select a limit. All page-level paths fall into that group: the four `CdpBrowserProvider` operations (page observation, WebMCP discovery, and their session variants) plus `CdpElectronProvider` call `openCdpTargetSession` without `limits`, which means page-controlled content could deliver a single message of arbitrary size before any of REA's capture accounting saw a byte. This PR makes an omitted selection fall back to `WEB_RUNTIME_LIMITS.protocolBytes` (64 MiB), the same protocol budget the runtime session already selects explicitly, while explicit per-session budgets stay authoritative and the existing typed `payload_limit` close reason is unchanged. No issue is linked because this is standalone hardening at the transport owner rather than a report-driven fix. ## Problem and expected behavior Smallest trigger: open a page-scoped CDP connection through `openCdpTargetSession` without a `limits` argument, then have the target deliver one message larger than any sane protocol budget. Prior behavior: the message is accepted. This is reproduced on the base revision with a fake CDP reply of `WEB_RUNTIME_LIMITS.protocolBytes + 1` bytes: the pending `Runtime.enable` command resolved with the oversized payload instead of rejecting. The violated invariant: page-controlled content must not decide how much memory the host retains for one message, and an over-budget frame should surface as the same typed transport failure the explicit-limit path already produces. Expected behavior: the receiver refuses the frame, the connection reports `payload_limit`, and the user-visible message names the selected budget in bytes, exactly like the existing `maxPayloadBytes: 512` path does today. ## Change and scope The change is one owner edit in `CdpConnection.connect` (`src/browser/CdpConnection.ts`): the selected budget is resolved once as `limits?.maxPayloadBytes ?? WEB_RUNTIME_LIMITS.protocolBytes` and used for both the `ws` option and the connection instance, with a comment recording why an explicit `0` must not survive. This fits the architecture by normalizing at the boundary that owns the socket instead of editing five scattered call sites: any future caller that forgets to pass `limits` is bounded by default, and callers that do pass a budget (runtime sessions at 64 MiB, the V8 inspector at 16 MiB) are untouched. Intentionally not included: no per-operation budget tuning, no change to observation or capture budgets such as `WEB_NETWORK_CAPTURE_LIMITS`, no edits to `CdpBrowserProvider` or `CdpElectronProvider`, and no new configuration knob, because the existing `limits` parameter remains the override for any session that needs a different ceiling. ## Contract and boundary impact - Semantic owner and earliest changed stage: browser provider transport, `CdpConnection.connect` in `src/browser/CdpConnection.ts`; domain schemas and application workflows untouched. - CLI and MCP/tool-catalog contract: none. No tool, schema, annotation, or catalog change; the error surface reuses the existing `payload_limit` transport reason already projected by `BrowserObservationError`. - Provider, bridge, target-format, or platform compatibility: applies uniformly to Chromium, Electron, and Playwright-managed page sockets through the shared `ws` receiver guard; no platform-specific code and no bridge change. - Evidence, artifact, provenance, or reconstruction contract: none. The refusal happens at frame receipt before message text is parsed or retained, so no identity, digest, or coverage projection changes. - Process execution, authorization, cleanup, or containment impact: oversize frames are refused by `ws` (surfacing as `WS_ERR_UNSUPPORTED_MESSAGE_LENGTH`, already mapped to `payload_limit`); no new process, network, or authority surface, and cleanup paths are unchanged. - Generated metadata (`docs/public/product-catalog.json`), package, or installation impact (CI generates and validates ignored catalogs and `skills/`; edit authored instructions in `skill-src/` and never commit build outputs): none. No contract, catalog, or skill input changed, and no build output is committed. ## Evidence and regression coverage - Tests added or updated: one new case in `tests/boundary/browser/cdpConnection.test.ts` ("bounds command payloads to the shared protocol budget when no limits are selected") connects without `limits`, receives a reply of `WEB_RUNTIME_LIMITS.protocolBytes + 1` bytes from the fake CDP browser, and asserts rejection with `reason: "payload_limit"` and a user message containing `67108864 byte protocol budget`. The payload-budget tests now live in their own `describe` block so the callback stays within the repository's 150-line function limit, and the existing explicit-`512` case is unchanged as the regression guard for the override path. - Base reproduction or other evidence: on base `0fa0b046` the new case fails with "promise resolved instead of rejecting", proving the default path accepted the oversize frame; after the change the same case passes and the full `cdpConnection.test.ts` file reports 11/11. - User-visible CLI/MCP output: unchanged in shape. Default connections now emit the same `payload_limit` failure the explicit path already produces, and the message names the 67108864-byte budget when the shared default is what refused the frame. - Remaining proof gaps: no real Chrome or Electron run was performed locally, and the ceiling reuses the maintainer-selected `WEB_RUNTIME_LIMITS.protocolBytes` value rather than a measurement from a live browser; realistic CDP messages such as screenshots and DOM snapshots sit far below that budget. For evidence-bearing changes: - [x] Observed, derived, and inferred claims remain distinguishable. - [x] Artifact identity, source provenance, and failed attempts remain preserved. - [x] Unsupported, incomplete, unavailable, or uncertain outcomes remain visible. ## Validation performed - `npx vitest run tests/boundary/browser/cdpConnection.test.ts` — 11 passed, including the new default-ceiling case, after rebasing onto `be75ed5e`. - `npx vitest run tests/boundary/browser/` — 35 files passed; `browserProcessStartup.test.ts`, `historicalCaptureDecoder.test.ts`, and `historicalWebCapture.test.ts` fail 17 tests on this Windows host identically with and without the change (verified by stashing the diff and re-running the pristine base), so they are environmental and pre-existing. - `npm run typecheck` — exit 0 on the rebased branch. - `npx oxlint` on both changed files — 0 warnings, 0 errors; `oxfmt --check` on the committed (LF) form — clean. - Real browser lane (`npm run verify:browser`): not run locally because no Chrome runtime is available here; the fake-CDP boundary suite covers the changed connection path instead. ## Compatibility, safety, and release - Breaking changes or migration steps: none for realistic traffic. Single messages above 64 MiB on connections that previously had no ceiling now fail with the typed `payload_limit` error instead of loading, which is the intended behavior change; sessions that need another ceiling already pass `limits` explicitly. - Real Hopper/Ghidra, browser, or OS coverage: not performed; the change is transport-level and platform-neutral, and the Windows-host boundary suites above exercised it. - Package or release metadata impact: none. - Security, privacy, process, or containment review: the change removes a page-controlled memory amplification path on the host that serves MCP clients, adds no network, process, or authority surface, and does not persist or transmit frame content. ## Review checklist - [x] This PR addresses a concrete problem or an agreed enhancement. - [x] The PR has one focused outcome and the title follows `type(scope): outcome`. - [x] Related issue is linked, or the reason for not linking one is stated above. - [x] Tests cover changed observable behavior and meaningful failure paths. - [x] Owning docs, contracts, and generated metadata are updated where needed. - [x] User-visible CLI/MCP changes include representative output. - [x] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
bsakaguchi 2026-10-31 00:54:23 +00:00
Sign in to join this conversation.
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
morluto/rea#4723
No description provided.