morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 06:45:22 +00:00

[PR #3860] [MERGED] fix(javascript): reject Webpack getters replaced through any identity write #5955

New issue

Closed

opened 2026-10-31 01:10:42 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:10:42 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/3860
Author: @kaoru0822-kitauji
Created: 10/15/2026
Status: ✅ Merged
Merged: 10/15/2026
Merged by: @kaoru0822-kitauji

Base: main ← Head: fix/3315-getter-identity-writes

📝 Commits (1)

62911c5 fix(javascript): reject Webpack getters replaced through any identity write

📊 Changes

2 files changed (+72 additions, -54 deletions)

View changed files

📝 src/domain/javascript/electronBundledRequireIdentity.test.ts (+32 -0)
📝 src/domain/javascript/javascriptBundledRequireOrigin.ts (+40 -54)

📄 Description

Summary

Fixes #3315. The Webpack n.n default-export getter proof ran its own write scan. That scan saw only direct loader.n = … assignments and four Object.* methods. A getter replaced through Reflect.set or Reflect.defineProperty, removed with delete, or overwritten by destructuring or an unknown key kept the proof, so BrowserWindow calls behind a non-canonical getter were still attributed to Electron. Separately, any Object.defineProperty/assign on the loader rejected the proof, even for an unrelated key.

Problem and expected behavior

Smallest trigger: Reflect.set(load, "n", mod => () => ({ BrowserWindow: class {} })) before entry dispatch still yields one Electron BrowserWindow fact. A direct load.n = … correctly yields none. The proof must reject every visible write that may select n, through any write form the analyzer recognizes.

Change and scope

Write owner: canonicalWebpackInteropBindings now consumes the shared identity-write owner, collectJavaScriptIdentityWritesSteps, with the module interop's intrinsic-invocation resolution. Ambient global writes already use the same owner. It covers:
- assignment, update, delete and destructuring targets;
- Object.defineProperty/defineProperties/assign/setPrototypeOf;
- Reflect.set/deleteProperty/defineProperty/setPrototypeOf, including intrinsic aliases and bind/call/apply.
Counted writes: a write counts when its path may select n, meaning the key itself, an unknown key, or the whole loader. As before, exactly one canonical assignment, installed before entry dispatch, is still required.
Removed: the hand-rolled assignment/Object.* scan is gone.
Not included: the bootstrap integrity proof (javascriptBootstrapIntegrity.ts) has its own table and cache write checks. Those cover different slots and are not changed here.

Contract and boundary impact

Semantic owner and earliest changed stage: JavaScript bundled-require origin (javascriptBundledRequireOrigin.ts), using the identity-write owner in javascriptElectronMemberWrites.ts.
Contribution scope fit and linked maintainer decision: an ordinary fix within existing boundaries, so not applicable.
CLI and MCP/tool-catalog contract: no schema change.
Provider, bridge, target-format, or platform compatibility: none.
Evidence, artifact, provenance, or reconstruction contract: Electron facts behind a replaced or deleted getter are no longer reported as direct observations. A reflective write to an unrelated loader key no longer suppresses them.
Target selection, authority, upstream behavior, and cleanup details: not applicable.
Generated metadata, package, or installation impact: none.

Expected effects:

Read existing files or captured data.

Evidence and regression coverage

Tests added to src/domain/javascript/electronBundledRequireIdentity.test.ts:
- getter writes through Reflect.set, Reflect.defineProperty, delete, a destructuring write and an unknown key;
- a positive case, Object.defineProperty(n, "d", …), which keeps the attribution.
All 6 fail on main.
Base reproduction (executed): the issue's four-variant CLI script through scripts/rea.mjs analyze-javascript-application, main cf0a38ad9 vs. this branch.

Variant	main BrowserWindows	branch BrowserWindows	Evidence bytes (main → branch)
reflective (`Reflect.set`)	1	0	183,004 → 179,795
deleted (`delete load.n`)	1	0	169,742 → 166,533
direct (`load.n = …`)	0	0	175,304 → 175,310
intact	1	1	166,210 → 166,216

Real-app comparison (executed): main vs. this branch over every JS file in six corpora (Discord, Notion, Signal, Claude, Tabby and VS Code out/). It covers 3,300 files for semantic IR and 3,311 for static facts. Static facts include the Electron inventory.
- Every file is byte-identical in both semantic IR and static facts.
- One Claude chunk throws RangeError on both builds; that is the pre-existing #3839.
- No real bundle used a write that the new owner now rejects.
Timing: the proof adds one identity-write pass per Webpack bootstrap that passes the integrity checks. Proofs are cached per bootstrap call.
- Notion's Webpack main/index.js, which runs the proof: median of 9 alternating static+semantic runs was 8,925 → 9,054 ms (+1.4%). CPU profiles of single runs: 9,464 → 9,509 ms.
- VS Code bundles, 5 alternating runs: within ±1.6% (agentHostMain.js 6,693 → 6,741 ms, extensionHostProcess.js 4,457 → 4,526 ms, extensionHostWorkerMain.js 3,835 → 3,824 ms).
Remaining proof gaps:
- No MCP-adapter run; the CLI and MCP share the static analysis producer.
- Timings come from one machine.

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

npm run test:local -- src/domain/javascript/electronBundledRequireIdentity.test.ts: 41 passed. On main, the 6 new cases fail.
161 JavaScript/domain test files via npm run test:local: all passed after rebasing onto cf0a38ad9.
npm run check:fast: typecheck and lint pass. The pre-push hook also passed.
The CLI reproduction, corpus comparisons and timing above.

Compatibility, safety, and release

Breaking changes or migration steps: none.
Real Hopper/Ghidra, browser, or OS coverage: not applicable (static JavaScript analysis).
Package or release metadata impact: none.
Security, privacy, process, or containment review: no target interaction. Private app bundles are referenced only by name and path.

Author readiness

Human owner:

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.

🤖 Generated with Claude Code

_{🔄 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/3860 **Author:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Created:** 10/15/2026 **Status:** ✅ Merged **Merged:** 10/15/2026 **Merged by:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Base:** `main` ← **Head:** `fix/3315-getter-identity-writes` --- ### 📝 Commits (1) - [`62911c5`](https://github.com/morluto/rea/commit/62911c5b3c634aa1b83a40c2bdca874d35e5ec25) fix(javascript): reject Webpack getters replaced through any identity write ### 📊 Changes **2 files changed** (+72 additions, -54 deletions) <details> <summary>View changed files</summary> 📝 `src/domain/javascript/electronBundledRequireIdentity.test.ts` (+32 -0) 📝 `src/domain/javascript/javascriptBundledRequireOrigin.ts` (+40 -54) </details> ### 📄 Description ## Summary Fixes #3315. The Webpack `n.n` default-export getter proof ran its own write scan. That scan saw only direct `loader.n = …` assignments and four `Object.*` methods. A getter replaced through `Reflect.set` or `Reflect.defineProperty`, removed with `delete`, or overwritten by destructuring or an unknown key kept the proof, so BrowserWindow calls behind a non-canonical getter were still attributed to Electron. Separately, any `Object.defineProperty`/`assign` on the loader rejected the proof, even for an unrelated key. ## Problem and expected behavior Smallest trigger: `Reflect.set(load, "n", mod => () => ({ BrowserWindow: class {} }))` before entry dispatch still yields one Electron BrowserWindow fact. A direct `load.n = …` correctly yields none. The proof must reject every visible write that may select `n`, through any write form the analyzer recognizes. ## Change and scope - **Write owner:** `canonicalWebpackInteropBindings` now consumes the shared identity-write owner, `collectJavaScriptIdentityWritesSteps`, with the module interop's intrinsic-invocation resolution. Ambient global writes already use the same owner. It covers: - assignment, update, `delete` and destructuring targets; - `Object.defineProperty`/`defineProperties`/`assign`/`setPrototypeOf`; - `Reflect.set`/`deleteProperty`/`defineProperty`/`setPrototypeOf`, including intrinsic aliases and `bind`/`call`/`apply`. - **Counted writes:** a write counts when its path may select `n`, meaning the key itself, an unknown key, or the whole loader. As before, exactly one canonical assignment, installed before entry dispatch, is still required. - **Removed:** the hand-rolled assignment/`Object.*` scan is gone. - **Not included:** the bootstrap integrity proof (`javascriptBootstrapIntegrity.ts`) has its own table and cache write checks. Those cover different slots and are not changed here. ## Contract and boundary impact - Semantic owner and earliest changed stage: JavaScript bundled-require origin (`javascriptBundledRequireOrigin.ts`), using the identity-write owner in `javascriptElectronMemberWrites.ts`. - Contribution scope fit and linked maintainer decision: an ordinary fix within existing boundaries, so not applicable. - CLI and MCP/tool-catalog contract: no schema change. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: Electron facts behind a replaced or deleted getter are no longer reported as direct observations. A reflective write to an unrelated loader key no longer suppresses them. - Target selection, authority, upstream behavior, and cleanup details: not applicable. - Generated metadata, package, or installation impact: none. Expected effects: - [x] Read existing files or captured data. ## Evidence and regression coverage - Tests added to `src/domain/javascript/electronBundledRequireIdentity.test.ts`: - getter writes through `Reflect.set`, `Reflect.defineProperty`, `delete`, a destructuring write and an unknown key; - a positive case, `Object.defineProperty(n, "d", …)`, which keeps the attribution. All 6 fail on main. - Base reproduction (executed): the issue's four-variant CLI script through `scripts/rea.mjs analyze-javascript-application`, main cf0a38ad9 vs. this branch. | Variant | main BrowserWindows | branch BrowserWindows | Evidence bytes (main → branch) | |---|---:|---:|---| | reflective (`Reflect.set`) | 1 | 0 | 183,004 → 179,795 | | deleted (`delete load.n`) | 1 | 0 | 169,742 → 166,533 | | direct (`load.n = …`) | 0 | 0 | 175,304 → 175,310 | | intact | 1 | 1 | 166,210 → 166,216 | - Real-app comparison (executed): main vs. this branch over every JS file in six corpora (Discord, Notion, Signal, Claude, Tabby and VS Code `out/`). It covers 3,300 files for semantic IR and 3,311 for static facts. Static facts include the Electron inventory. - Every file is byte-identical in both semantic IR and static facts. - One Claude chunk throws `RangeError` on both builds; that is the pre-existing #3839. - No real bundle used a write that the new owner now rejects. - Timing: the proof adds one identity-write pass per Webpack bootstrap that passes the integrity checks. Proofs are cached per bootstrap call. - Notion's Webpack `main/index.js`, which runs the proof: median of 9 alternating static+semantic runs was 8,925 → 9,054 ms (+1.4%). CPU profiles of single runs: 9,464 → 9,509 ms. - VS Code bundles, 5 alternating runs: within ±1.6% (`agentHostMain.js` 6,693 → 6,741 ms, `extensionHostProcess.js` 4,457 → 4,526 ms, `extensionHostWorkerMain.js` 3,835 → 3,824 ms). - Remaining proof gaps: - No MCP-adapter run; the CLI and MCP share the static analysis producer. - Timings come from one machine. 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 - `npm run test:local -- src/domain/javascript/electronBundledRequireIdentity.test.ts`: 41 passed. On main, the 6 new cases fail. - 161 JavaScript/domain test files via `npm run test:local`: all passed after rebasing onto cf0a38ad9. - `npm run check:fast`: typecheck and lint pass. The pre-push hook also passed. - The CLI reproduction, corpus comparisons and timing above. ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: not applicable (static JavaScript analysis). - Package or release metadata impact: none. - Security, privacy, process, or containment review: no target interaction. Private app bundles are referenced only by name and path. ## Author readiness Human owner:  - [ ] 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. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 01:10:42 +00:00

closed this issue
added the
pull-request
label

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#5955

No description provided.

Rows
Columns