[PR #2876] fix(web-capture): bound the mitmproxy headline facet index #5460

Open
opened 2026-10-31 01:06:49 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/2876
Author: @Dragolone
Created: 10/13/2026
Status: 🔄 Open

Base: main ← Head: fix/web-capture-preview-mitmproxy-headline


📝 Commits (10+)

  • 59b3a38 fix(web-capture): keep mitmproxy request targets and status codes in previews
  • b94ae49 fix(comparison): recheck historical calls-only changes (#2223)
  • 9618936 fix(ci): isolate Linux Hopper process fixture inventory
  • c448482 fix(android): make DEX descriptor pattern portable
  • 1f346b0 style: format comparison and IDA sources
  • dfb36b5 Merge upstream main into fix/web-capture-preview-mitmproxy-headline
  • 7eab486 fix(web-capture): preview mitmproxy targets only as safe scalars
  • b2ea94d fix(web-capture): keep the asterisk-form OPTIONS target in mitmproxy previews
  • d0ab028 Merge branch 'main' into fix/web-capture-preview-mitmproxy-headline
  • a8df0da Merge branch 'main' into fix/web-capture-preview-mitmproxy-headline

📊 Changes

2 files changed (+144 additions, -36 deletions)

View changed files

📝 src/domain/analysisView/webNetworkCaptureView.test.ts (+95 -1)
📝 src/domain/analysisView/webNetworkCaptureView.ts (+49 -35)

📄 Description

Summary

The mitmproxy headline preview from the earlier revisions of this PR landed on main in f6905719c. This PR now carries only the two follow-up review fixes that came after that revision. Both bound the per-record facet index that webNetworkCaptureView.ts builds for mitmproxy HTTP records.

As landed on main, indexFacets has two problems:

  1. Unbounded ancestor expansion. It adds every strict ancestor of every facet pointer to containers. pointerSchema puts no length or depth bound on facet pointers, so a single 1,000,000-character pointer such as "/".repeat(1_000_000) produces a million set entries.
  2. Indexing facets nobody queries. It groups every numeric sidecar and byte descriptor by pointer and copies all their keys into scalars. The mitmproxy bridge emits one sidecar per number, so a record with a large integer array under an unrelated extension allocates an entry for each element. Only /type, the six target/status coordinates and their ancestors are ever queried.

Problem and expected behavior

Expected: the index size depends neither on how many facets a record has nor on how long or deep their pointers are, and every headline decision is unchanged.

Change and scope

indexFacets now keeps a facet only when its pointer is a headline coordinate or one of their ancestors: "", /type, /request, /response and the six target/status fields. It keeps at most two facets per pointer, because a second facet already fails every representation check.

Every other facet only marks the headline coordinates it lies beneath. It does this with startsWith against seven fixed "<coordinate>/" prefixes, so no strings are derived from its pointer.

Not included: moving headline normalization into the mitmproxy producer. That would interpret native state in the bridge and add a field to the shared record schema; it is under discussion in the review thread.

Contribution scope: a resource-bound fix in existing code; a maintainer scope decision is not applicable.

Contract and boundary impact

  • Semantic owner and earliest changed stage: indexFacets in src/domain/analysisView/webNetworkCaptureView.ts.
  • Contribution scope fit and linked maintainer decision: in scope; not applicable.
  • CLI and MCP/tool-catalog contract: none.
  • Provider, bridge, target-format, or platform compatibility: none.
  • Evidence, artifact, provenance, or reconstruction contract: none; the same headlines and sidecars are selected.
  • Target selection, authority, upstream behavior, and cleanup details (if applicable): not applicable.
  • Generated metadata (docs/public/product-catalog.json), package, or installation impact: none.

Expected effects (check known effects; use documentation-only on its own).
Unchecked items do not establish absence; leave uncertainties visible.

  • Read existing files or captured data.
  • Launch or attach to a process.
  • Inject instrumentation or run scripts/code.
  • Change process memory, registers, files, or configuration.
  • Contact network services.
  • Recover protected code or work around protections.
  • Documentation or usability only; no target interaction.
  • Not sure yet; investigation or maintainer guidance needed.

Evidence and regression coverage

  • Tests added or updated: src/domain/analysisView/webNetworkCaptureView.test.ts. The new tests are "indexes an unrelated / a path-descendant million-character facet pointer by its headline prefix only". They check that a 1,000,000-character pointer is still classified correctly: unrelated, or beneath /request/path. They also count the strings longer than 1,000 characters that the preview call adds to any Set and expect none. Only the pointer and strings cut from it are that long, so the count stands in for the index size without timing or heap checks. "keeps headline omissions and controls beside many unrelated facets" places 20,000 unrelated sidecars beside a redacted host and a path with a facet beneath it: both stay withheld, while the scheme, port and status remain with only their own sidecars. It passes against both the current main projection and this change, so it pins the omission/uncertainty behavior rather than the bound.
  • Base reproduction or other evidence: executed.
    • With main's webNetworkCaptureView.ts (47cedcc93) the two million-character tests fail on Node 24.22.1: the old index adds 999,000 and 499,507 such strings (expected 999000 to be +0, expected 499507 to be +0). With this change it adds none.
    • Before 67c355028 these tests checked only classification. On Node 22 and 24 they passed on the base file, and only Node 26 caught the old index, by running out of heap (about 4 GB), as @CassieFox23 reproduced twice in this comment. The set-addition count follows the nit in the latest review from @CassieFox23.
    • Same comment, one run each on a shared box: 200,000 sidecars plus a 1,000,000-character pointer took 450 ms (Node 22) / 296 ms (Node 24) / heap out of memory (Node 26) on the base file, and 86 / 62 / 91 ms with this change. 6,000 random records produced identical previews on the base file and this change.
    • My own local benchmark (not committed) on Node 24: about 510 ms on main vs about 295 ms with this change, including fixture building and schema parsing.
  • User-visible CLI/MCP output (if applicable): unchanged.
  • Remaining proof gaps: no committed test asserts the index size directly, because the index is internal; the tests count long-string set additions over the whole preview call instead. I ran that count on Node 24.22.1 only.

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 src/domain/analysisView/ src/browser/history/ on this branch merged with main 47cedcc93 (Node 24.22.1) — 76 files, 1,733 tests passed.
  • npx vitest run src/domain/analysisView/webNetworkCaptureView.test.ts — 55 passed. With webNetworkCaptureView.ts replaced by main's, the two million-character tests fail as shown above, and the boundary-control test passes.
  • npx tsc --noEmit — 0 errors.
  • npx oxlint on the changed files — 0 errors. The three warnings on webNetworkCaptureView.ts (max-lines, max-params, max-lines-per-function) are also present on main's file.
  • npx oxfmt --check on the changed files — clean.
  • npm run verify:module-boundaries — passed.
  • npm run check:fast (typecheck, lint) — passed.
  • node scripts/check-doc-facts.mjs after npm run compile — passed.

Compatibility, safety, and release

  • Breaking changes or migration steps: none.
  • Real Hopper/Ghidra, browser, or OS coverage: not applicable.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: reduces transient allocation on the synchronous inspect_analysis_view path for admitted mitmproxy Evidence.

Author readiness

Human owner: @Dragolone

  • I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work.
  • I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps.
  • I declare this PR ready for initial maintainer review.

Before merge

  • This PR addresses a concrete problem or an agreed enhancement.
  • Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff.
  • 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/2876 **Author:** [@Dragolone](https://github.com/Dragolone) **Created:** 10/13/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `fix/web-capture-preview-mitmproxy-headline` --- ### 📝 Commits (10+) - [`59b3a38`](https://github.com/morluto/rea/commit/59b3a38ec33eaeee23fa71942c2e280302a91732) fix(web-capture): keep mitmproxy request targets and status codes in previews - [`b94ae49`](https://github.com/morluto/rea/commit/b94ae4991d089916c95dbbbdebb361cece0e5134) fix(comparison): recheck historical calls-only changes (#2223) - [`9618936`](https://github.com/morluto/rea/commit/961893666dee69f04623deae0018df52c246a676) fix(ci): isolate Linux Hopper process fixture inventory - [`c448482`](https://github.com/morluto/rea/commit/c448482810624ecf23c5348674bc73dee9e7d114) fix(android): make DEX descriptor pattern portable - [`1f346b0`](https://github.com/morluto/rea/commit/1f346b07b344ae45aa600e80f08abb70fa7958ce) style: format comparison and IDA sources - [`dfb36b5`](https://github.com/morluto/rea/commit/dfb36b565582c3774ff462fc3d57ad8d60f5760f) Merge upstream main into fix/web-capture-preview-mitmproxy-headline - [`7eab486`](https://github.com/morluto/rea/commit/7eab486b77890d29febc7b4ec49c9198d6a40253) fix(web-capture): preview mitmproxy targets only as safe scalars - [`b2ea94d`](https://github.com/morluto/rea/commit/b2ea94dc7d9c093b80bdd56d614ed0f17b379f37) fix(web-capture): keep the asterisk-form OPTIONS target in mitmproxy previews - [`d0ab028`](https://github.com/morluto/rea/commit/d0ab02875938c4cec150b7b69a25f1eabb3a69b5) Merge branch 'main' into fix/web-capture-preview-mitmproxy-headline - [`a8df0da`](https://github.com/morluto/rea/commit/a8df0da0b61b389490aa4ec257378fe2e4f40b6f) Merge branch 'main' into fix/web-capture-preview-mitmproxy-headline ### 📊 Changes **2 files changed** (+144 additions, -36 deletions) <details> <summary>View changed files</summary> 📝 `src/domain/analysisView/webNetworkCaptureView.test.ts` (+95 -1) 📝 `src/domain/analysisView/webNetworkCaptureView.ts` (+49 -35) </details> ### 📄 Description ## Summary The mitmproxy headline preview from the earlier revisions of this PR landed on main in f6905719c. This PR now carries only the two follow-up review fixes that came after that revision. Both bound the per-record facet index that `webNetworkCaptureView.ts` builds for mitmproxy HTTP records. As landed on main, `indexFacets` has two problems: 1. **Unbounded ancestor expansion.** It adds every strict ancestor of every facet pointer to `containers`. `pointerSchema` puts no length or depth bound on facet pointers, so a single 1,000,000-character pointer such as `"/".repeat(1_000_000)` produces a million set entries. 2. **Indexing facets nobody queries.** It groups every numeric sidecar and byte descriptor by pointer and copies all their keys into `scalars`. The mitmproxy bridge emits one sidecar per number, so a record with a large integer array under an unrelated extension allocates an entry for each element. Only `/type`, the six target/status coordinates and their ancestors are ever queried. ## Problem and expected behavior Expected: the index size depends neither on how many facets a record has nor on how long or deep their pointers are, and every headline decision is unchanged. ## Change and scope `indexFacets` now keeps a facet only when its pointer is a headline coordinate or one of their ancestors: `""`, `/type`, `/request`, `/response` and the six target/status fields. It keeps at most two facets per pointer, because a second facet already fails every representation check. Every other facet only marks the headline coordinates it lies beneath. It does this with `startsWith` against seven fixed `"<coordinate>/"` prefixes, so no strings are derived from its pointer. Not included: moving headline normalization into the mitmproxy producer. That would interpret native state in the bridge and add a field to the shared record schema; it is under discussion in the review thread. Contribution scope: a resource-bound fix in existing code; a maintainer scope decision is not applicable. ## Contract and boundary impact - Semantic owner and earliest changed stage: `indexFacets` in `src/domain/analysisView/webNetworkCaptureView.ts`. - Contribution scope fit and linked maintainer decision: in scope; not applicable. - CLI and MCP/tool-catalog contract: none. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: none; the same headlines and sidecars are selected. - Target selection, authority, upstream behavior, and cleanup details (if applicable): not applicable. - Generated metadata (`docs/public/product-catalog.json`), package, or installation impact: none. Expected effects (check known effects; use documentation-only on its own). Unchecked items do not establish absence; leave uncertainties visible. - [x] Read existing files or captured data. - [ ] Launch or attach to a process. - [ ] Inject instrumentation or run scripts/code. - [ ] Change process memory, registers, files, or configuration. - [ ] Contact network services. - [ ] Recover protected code or work around protections. - [ ] Documentation or usability only; no target interaction. - [ ] Not sure yet; investigation or maintainer guidance needed. ## Evidence and regression coverage - Tests added or updated: `src/domain/analysisView/webNetworkCaptureView.test.ts`. The new tests are "indexes an unrelated / a path-descendant million-character facet pointer by its headline prefix only". They check that a 1,000,000-character pointer is still classified correctly: unrelated, or beneath `/request/path`. They also count the strings longer than 1,000 characters that the preview call adds to any `Set` and expect none. Only the pointer and strings cut from it are that long, so the count stands in for the index size without timing or heap checks. "keeps headline omissions and controls beside many unrelated facets" places 20,000 unrelated sidecars beside a redacted host and a path with a facet beneath it: both stay withheld, while the scheme, port and status remain with only their own sidecars. It passes against both the current main projection and this change, so it pins the omission/uncertainty behavior rather than the bound. - Base reproduction or other evidence: executed. - With main's `webNetworkCaptureView.ts` (47cedcc93) the two million-character tests fail on Node 24.22.1: the old index adds 999,000 and 499,507 such strings (`expected 999000 to be +0`, `expected 499507 to be +0`). With this change it adds none. - Before 67c355028 these tests checked only classification. On Node 22 and 24 they passed on the base file, and only Node 26 caught the old index, by running out of heap (about 4 GB), as @CassieFox23 reproduced twice in [this comment](https://github.com/morluto/rea/pull/2876#issuecomment-6256982577). The set-addition count follows the nit in [the latest review](https://github.com/morluto/rea/pull/2876#pullrequestreview-5746756668) from @CassieFox23. - Same comment, one run each on a shared box: 200,000 sidecars plus a 1,000,000-character pointer took 450 ms (Node 22) / 296 ms (Node 24) / heap out of memory (Node 26) on the base file, and 86 / 62 / 91 ms with this change. 6,000 random records produced identical previews on the base file and this change. - My own local benchmark (not committed) on Node 24: about 510 ms on main vs about 295 ms with this change, including fixture building and schema parsing. - User-visible CLI/MCP output (if applicable): unchanged. - Remaining proof gaps: no committed test asserts the index size directly, because the index is internal; the tests count long-string set additions over the whole preview call instead. I ran that count on Node 24.22.1 only. 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 src/domain/analysisView/ src/browser/history/` on this branch merged with main 47cedcc93 (Node 24.22.1) — 76 files, 1,733 tests passed. - `npx vitest run src/domain/analysisView/webNetworkCaptureView.test.ts` — 55 passed. With `webNetworkCaptureView.ts` replaced by main's, the two million-character tests fail as shown above, and the boundary-control test passes. - `npx tsc --noEmit` — 0 errors. - `npx oxlint` on the changed files — 0 errors. The three warnings on `webNetworkCaptureView.ts` (`max-lines`, `max-params`, `max-lines-per-function`) are also present on main's file. - `npx oxfmt --check` on the changed files — clean. - `npm run verify:module-boundaries` — passed. - `npm run check:fast` (typecheck, lint) — passed. - `node scripts/check-doc-facts.mjs` after `npm run compile` — passed. ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: not applicable. - Package or release metadata impact: none. - Security, privacy, process, or containment review: reduces transient allocation on the synchronous `inspect_analysis_view` path for admitted mitmproxy Evidence. ## Author readiness <!-- The human owner completes these declarations before requesting review. The PR author is the owner by default. A different owner must explicitly accept ownership on this PR. AI agents must leave these declarations to a human to dictate to them or there could be catastrophic consequences for many users of REA. Draft PRs may leave them unchecked. These declarations record initial readiness when opening the PR for review; later pushes do not require a new human sign-off. Keep the description and validation evidence current as the work changes. See CONTRIBUTING.md#pull-request-ownership-and-readiness. --> Human owner: @Dragolone - [x] I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work. - [x] I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps. - [x] I declare this PR ready for initial maintainer review. ## Before merge - [x] This PR addresses a concrete problem or an agreed enhancement. - [x] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. - [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>
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#5460
No description provided.