morluto/rea

Fork 0

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

[PR #2705] [MERGED] fix(electron): reject non-executable paths and contain launch rejections #5329

New issue

Closed

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

bsakaguchi commented

2026-10-31 01:05:46 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/2705
Author: @CassieFox23
Created: 10/13/2026
Status: ✅ Merged
Merged: 10/13/2026
Merged by: @morluto

Base: main ← Head: fix/electron-launch-rejections

📝 Commits (10+)

2f0e9f3 fix(electron): reject non-executable paths and contain launch rejections
8396893 fix(electron): absorb only this launch's orphaned rejections
31f2761 fix(electron): share launch rejection ownership across concurrent captures
ab090f3 test(electron): distinguish launch success from capture ownership
c296182 test(electron): assert cleanup ownership failures around overlapping launches
021594e chore: merge pinned main for monotonic provider deadlines
b72b949 fix(comparison): recheck historical calls-only changes (#2223)
0ee5cad fix(android): make DEX descriptor pattern portable
aa4d47d fix(ci): isolate Linux Hopper process fixture inventory
44b5ac5 fix(browser): avoid replaying observed launch rejections

📊 Changes

4 files changed (+562 additions, -5 deletions)

View changed files

📝 src/application/javascript/ElectronActiveObservationService.ts (+27 -2)
📝 src/browser/PlaywrightElectronActiveProvider.ts (+87 -2)
📝 tests/acceptance/applications/electronInvalidPaths.test.ts (+125 -1)
➕ tests/boundary/process/electronLaunchRejections.test.ts (+323 -0)

📄 Description

TL;DR: A bad executable_path could kill the whole REA process. Now a file you can't run is refused up front. A launch that fails gets a normal error, and the server keeps running. I tested this on Linux only.

Summary

Refs #2269. I tested on Linux only, so this does not close the issue.

capture_electron_scenario could end the whole process. In MCP the server exited and the client saw Connection closed. All retained Evidence went with it. In the CLI there was a Node stack trace and no JSON.

This PR does two things. It refuses an executable_path that is not an executable file, before any launch. It also turns a failed launch into a typed execution_failure instead of a crash.

Problem and expected behavior

Playwright starts four promises in electron.launch(). Each waits for output. It awaits only one. When the child exits or the launch times out, all four reject. Nobody awaits the other three. They become unhandled rejections, and Node ends the process.

I checked this with a short script that calls _electron.launch directly. For an executable that exits at once, three unhandledRejection events fired about 2 ms before the catch ran. A launch that timed out showed the same shape, with Playwright's own TimeoutError.

On Linux, the smallest trigger is a file with the execute bit that never prints Debugger listening on. Three such files crashed the process:

a script that runs exit 0
a script that sleeps (the timeout case, 60 s)
a plain text file with mode 755

A text file without the execute bit did not crash on Linux. The spawn fails with EACCES, and Playwright rejects cleanly. That case already returned execution_failure. On Windows the launch goes through a shell, so any file can reach the crash path. I could not test Windows.

Expected: a typed error, a live server, and Evidence that survives.

Change and scope

Two small changes, one at each boundary.

src/application/javascript/ElectronActiveObservationService.ts checks the resolved executable_path while paths are canonicalized. It must be a regular file. On non-Windows hosts it must also pass access(X_OK). A failure is an invalid_request with issues[0].path = ["executable_path"]. It is the same shape as the unresolved-path issues, and it joins the same list.
src/browser/PlaywrightElectronActiveProvider.ts wraps only the launch call in containLaunchRejections. While the call runs, it listens for unhandledRejection. A rejection that does not come from Playwright is re-thrown at once. A Playwright rejection is held until the call settles. If the launch failed, a held rejection is dropped only when its message is part of the launch's own error. Every other held rejection is raised again, so the process still fails. If the launch worked, all held rejections are raised again. The listener is removed on every path. The real error still reaches the existing catch and becomes the usual typed failure.

Not included:

No check that the file is an Electron binary. A wrapper script is a valid launcher, so a magic-byte check would be wrong.
No Windows executable check. A .cmd shim can launch there, and I cannot test PE rules.
No dependency bump, and no child process for launches.

Closed PR #2501 checked a Windows PE header inside canonicalPaths. That function is gone from main. It did not touch the Playwright rejection. The commit that closed #2272 says this crash is separate.

Follow the contribution scope.
Unresolved scope, authority, side effects, or required verification block
integration. Link the required maintainer scope decision for new providers or
capabilities involving target execution, injection, mutation, or protection
recovery; ordinary fixes within existing boundaries may state "not applicable".
Draft PRs may leave the scope decision pending and the final review checklist
unchecked. Maintainers can help resolve open questions.

This is a fix inside existing limits. It adds no new capability. A scope decision is not applicable. See the contribution scope.

Contract and boundary impact

Semantic owner and earliest changed stage: the application service owns path checks, and they run first. The provider owns the launch and its rejections.
Contribution scope fit and linked maintainer decision: a bug fix within existing limits. No maintainer decision is needed.
CLI and MCP/tool-catalog contract: no schema or catalog change. A non-executable file now gives invalid_request instead of execution_failure. Both codes already exist.
Provider, bridge, target-format, or platform compatibility: tested on Linux. Windows and macOS were not run. The X_OK check is skipped on Windows.
Evidence, artifact, provenance, or reconstruction contract: no change. Earlier Evidence stays retained after a failed launch. I checked this over MCP.
Target selection, authority, upstream behavior, and cleanup details (if applicable): the launch is unchanged. Cleanup still runs through the existing path. No stray child was left after my failed launches.
Generated metadata (docs/public/product-catalog.json), package, or installation impact (CI generates and validates ignored catalogs and skills/; edit authored instructions in .agents/skills/reverse-engineer-anything/ and never commit build outputs): 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.

The only new file calls are stat and access on the path the caller chose.

Evidence and regression coverage

The evidence is executed on Linux. It is not source-derived.

Tests added or updated: two tests in tests/acceptance/applications/electronInvalidPaths.test.ts, and eleven in the new tests/boundary/process/electronLaunchRejections.test.ts. The first two are unchanged. One covers a mode 644 file through CLI and MCP. The other covers an executable that exits at once, through CLI and MCP. It checks that the server survives. It checks that binary_session works. It checks that get_evidence_bundle returns the same Evidence id. The new file runs the built provider in a child process. It checks that an unrelated rejection still ends the process. It checks that the listener is gone after a launch fails or works. More cases test two launches at once.
Base reproduction or other evidence: on main, the exit 0 script ended the MCP server. The client saw Connection closed. The server printed Error: Process failed to launch! from electron.js:247. The sleeping script ended the CLI after about 61 s with TimeoutError from progress.js:86. The new tests fail on main under Node 22, 24 and 26.
User-visible CLI/MCP output (if applicable): see below.
Remaining proof gaps: no Windows run, no macOS run, no real Electron launch. The new tests run in a child process, because the Vitest worker registers its own unhandledRejection listener. Most use a fake launch. Two use a real Playwright launch of a script that exits at once. The overlapping cases use fake launches only.

Output after the fix, for the mode 644 text file:

{"error":"Electron analysis failed","code":"invalid_request","details":{"operation":"capture_electron_scenario","issues":[{"path":["executable_path"],"reason":"invalid_value","message":"executable_path <file> is not an executable file: EACCES: permission denied, access '<file>'"}]}}

For the script that exits at once, the exit code is 1 and the code is execution_failure. The diagnostics say Process failed to launch!. The MCP server stays up.

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:focused -- tests/acceptance/applications/electronInvalidPaths.test.ts src/browser/PlaywrightElectronActiveProvider.test.ts tests/acceptance/applications/electronBlankPathCli.test.ts tests/boundary/mcp/electronMcp.test.ts tests/boundary/process/electronLaunchRejections.test.ts on Node 22.23.1, 24.18.0 and 26.11.1: 5 files and 25 tests passed on each.
The same command on unfixed main, on all three Node versions: 2 of the 3 tests in the new file failed.
I broke the fix four ways and ran the new file against each. Skipping the one-turn wait failed 1 test. Never removing the listener failed 9. Skipping the shared record of absorbed errors failed 2. Dropping every held rejection, which is the old behavior, failed 3.
npm run test:focused on the 22 test files with "electron" in the name, on Node 24: 342 tests passed.
npm run check:fast on Node 24: passed.
npm run verify:test-discovery: all 1039 test files found once.
oxfmt --check on the four changed files: passed.
Manual CLI and MCP runs on the built tree, before and after, with five files. They were a .har, a .txt, a script that exits, a mode 755 text file and a script that sleeps. After the fix, none ended the process. After the revision I reran three of them through the CLI. They were the script that exits at once, the mode 644 file, and a script that sleeps 300 s. The last one hits the real 60 s timeout. None ended the process. I did not rerun the MCP checks on the revised code.

Not run: the full suite, npm run check:pr, npm run docs:check, npm run verify:package, Windows, macOS, and a real Electron launch.

Compatibility, safety, and release

Breaking changes or migration steps: none. On Linux, a non-executable file used to give execution_failure. It now gives invalid_request.
Real Hopper/Ghidra, browser, or OS coverage: Linux only.
Package or release metadata impact: none.
Security, privacy, process, or containment review: a failed launch no longer ends the process. Playwright leaves extra errors behind, and the launch now drops them. Every other rejection still does. That includes an unrelated Playwright one. The match needs a Playwright path in the stack, and the launch error must contain the orphan's message. If Playwright changes either, the old crash comes back. It does not hide new bugs. Two captures at once share one record of absorbed errors, so neither raises the other's. One gap remains. Say an unrelated Playwright error has the same message as the launch failure. If it lands in the same window, it is dropped too.

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 (Sonnet 5.5, verified by Opus 5.5) 🦊

_{🔄 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/2705 **Author:** [@CassieFox23](https://github.com/CassieFox23) **Created:** 10/13/2026 **Status:** ✅ Merged **Merged:** 10/13/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/electron-launch-rejections` --- ### 📝 Commits (10+) - [`2f0e9f3`](https://github.com/morluto/rea/commit/2f0e9f377eee7be60277ee3df8e33cf826e3fe9c) fix(electron): reject non-executable paths and contain launch rejections - [`8396893`](https://github.com/morluto/rea/commit/8396893b0e5bd1e2aa15e7f010070454e1e09162) fix(electron): absorb only this launch's orphaned rejections - [`31f2761`](https://github.com/morluto/rea/commit/31f27616a033846a3a1cab52860518fd75a83b47) fix(electron): share launch rejection ownership across concurrent captures - [`ab090f3`](https://github.com/morluto/rea/commit/ab090f3de3869b3406d9996aa0d0647cf111ae78) test(electron): distinguish launch success from capture ownership - [`c296182`](https://github.com/morluto/rea/commit/c296182b8018c69a5e20197ff4ce96c416458233) test(electron): assert cleanup ownership failures around overlapping launches - [`021594e`](https://github.com/morluto/rea/commit/021594e026b5c2b9cfd325b1479b85b3c2db6b4c) chore: merge pinned main for monotonic provider deadlines - [`b72b949`](https://github.com/morluto/rea/commit/b72b9495c77826f79c8e2668478d65471cfa7fc0) fix(comparison): recheck historical calls-only changes (#2223) - [`0ee5cad`](https://github.com/morluto/rea/commit/0ee5cad5ab4b4742eefd42492f4526574835bc9e) fix(android): make DEX descriptor pattern portable - [`aa4d47d`](https://github.com/morluto/rea/commit/aa4d47d40a723a583b82046fd6a214cf4ae5cac7) fix(ci): isolate Linux Hopper process fixture inventory - [`44b5ac5`](https://github.com/morluto/rea/commit/44b5ac5a6414d43c75d5612d57aea79044e0cb0b) fix(browser): avoid replaying observed launch rejections ### 📊 Changes **4 files changed** (+562 additions, -5 deletions) <details> <summary>View changed files</summary> 📝 `src/application/javascript/ElectronActiveObservationService.ts` (+27 -2) 📝 `src/browser/PlaywrightElectronActiveProvider.ts` (+87 -2) 📝 `tests/acceptance/applications/electronInvalidPaths.test.ts` (+125 -1) ➕ `tests/boundary/process/electronLaunchRejections.test.ts` (+323 -0) </details> ### 📄 Description **TL;DR:** A bad `executable_path` could kill the whole REA process. Now a file you can't run is refused up front. A launch that fails gets a normal error, and the server keeps running. I tested this on Linux only. ## Summary Refs #2269. I tested on Linux only, so this does not close the issue. `capture_electron_scenario` could end the whole process. In MCP the server exited and the client saw `Connection closed`. All retained Evidence went with it. In the CLI there was a Node stack trace and no JSON. This PR does two things. It refuses an `executable_path` that is not an executable file, before any launch. It also turns a failed launch into a typed `execution_failure` instead of a crash. ## Problem and expected behavior Playwright starts four promises in `electron.launch()`. Each waits for output. It awaits only one. When the child exits or the launch times out, all four reject. Nobody awaits the other three. They become unhandled rejections, and Node ends the process. I checked this with a short script that calls `_electron.launch` directly. For an executable that exits at once, three `unhandledRejection` events fired about 2 ms before the `catch` ran. A launch that timed out showed the same shape, with Playwright's own `TimeoutError`. On Linux, the smallest trigger is a file with the execute bit that never prints `Debugger listening on`. Three such files crashed the process: - a script that runs `exit 0` - a script that sleeps (the timeout case, 60 s) - a plain text file with mode 755 A text file without the execute bit did not crash on Linux. The spawn fails with `EACCES`, and Playwright rejects cleanly. That case already returned `execution_failure`. On Windows the launch goes through a shell, so any file can reach the crash path. I could not test Windows. Expected: a typed error, a live server, and Evidence that survives. ## Change and scope Two small changes, one at each boundary. - `src/application/javascript/ElectronActiveObservationService.ts` checks the resolved `executable_path` while paths are canonicalized. It must be a regular file. On non-Windows hosts it must also pass `access(X_OK)`. A failure is an `invalid_request` with `issues[0].path = ["executable_path"]`. It is the same shape as the unresolved-path issues, and it joins the same list. - `src/browser/PlaywrightElectronActiveProvider.ts` wraps only the `launch` call in `containLaunchRejections`. While the call runs, it listens for `unhandledRejection`. A rejection that does not come from Playwright is re-thrown at once. A Playwright rejection is held until the call settles. If the launch failed, a held rejection is dropped only when its message is part of the launch's own error. Every other held rejection is raised again, so the process still fails. If the launch worked, all held rejections are raised again. The listener is removed on every path. The real error still reaches the existing `catch` and becomes the usual typed failure. Not included: - No check that the file is an Electron binary. A wrapper script is a valid launcher, so a magic-byte check would be wrong. - No Windows executable check. A `.cmd` shim can launch there, and I cannot test PE rules. - No dependency bump, and no child process for launches. Closed PR #2501 checked a Windows PE header inside `canonicalPaths`. That function is gone from main. It did not touch the Playwright rejection. The commit that closed #2272 says this crash is separate. Follow the [contribution scope](https://github.com/morluto/rea/blob/main/CONTRIBUTING.md#contribution-scope). Unresolved scope, authority, side effects, or required verification block integration. Link the required maintainer scope decision for new providers or capabilities involving target execution, injection, mutation, or protection recovery; ordinary fixes within existing boundaries may state "not applicable". Draft PRs may leave the scope decision pending and the final review checklist unchecked. Maintainers can help resolve open questions. This is a fix inside existing limits. It adds no new capability. A scope decision is not applicable. See the [contribution scope](https://github.com/morluto/rea/blob/main/CONTRIBUTING.md#contribution-scope). ## Contract and boundary impact - Semantic owner and earliest changed stage: the application service owns path checks, and they run first. The provider owns the launch and its rejections. - Contribution scope fit and linked maintainer decision: a bug fix within existing limits. No maintainer decision is needed. - CLI and MCP/tool-catalog contract: no schema or catalog change. A non-executable file now gives `invalid_request` instead of `execution_failure`. Both codes already exist. - Provider, bridge, target-format, or platform compatibility: tested on Linux. Windows and macOS were not run. The X_OK check is skipped on Windows. - Evidence, artifact, provenance, or reconstruction contract: no change. Earlier Evidence stays retained after a failed launch. I checked this over MCP. - Target selection, authority, upstream behavior, and cleanup details (if applicable): the launch is unchanged. Cleanup still runs through the existing path. No stray child was left after my failed launches. - Generated metadata (`docs/public/product-catalog.json`), package, or installation impact (CI generates and validates ignored catalogs and `skills/`; edit authored instructions in `.agents/skills/reverse-engineer-anything/` and never commit build outputs): 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. - [x] 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. The only new file calls are `stat` and `access` on the path the caller chose. ## Evidence and regression coverage The evidence is executed on Linux. It is not source-derived. - Tests added or updated: two tests in `tests/acceptance/applications/electronInvalidPaths.test.ts`, and eleven in the new `tests/boundary/process/electronLaunchRejections.test.ts`. The first two are unchanged. One covers a mode 644 file through CLI and MCP. The other covers an executable that exits at once, through CLI and MCP. It checks that the server survives. It checks that `binary_session` works. It checks that `get_evidence_bundle` returns the same Evidence id. The new file runs the built provider in a child process. It checks that an unrelated rejection still ends the process. It checks that the listener is gone after a launch fails or works. More cases test two launches at once. - Base reproduction or other evidence: on main, the `exit 0` script ended the MCP server. The client saw `Connection closed`. The server printed `Error: Process failed to launch!` from `electron.js:247`. The sleeping script ended the CLI after about 61 s with `TimeoutError` from `progress.js:86`. The new tests fail on main under Node 22, 24 and 26. - User-visible CLI/MCP output (if applicable): see below. - Remaining proof gaps: no Windows run, no macOS run, no real Electron launch. The new tests run in a child process, because the Vitest worker registers its own `unhandledRejection` listener. Most use a fake launch. Two use a real Playwright launch of a script that exits at once. The overlapping cases use fake launches only. Output after the fix, for the mode 644 text file: ```json {"error":"Electron analysis failed","code":"invalid_request","details":{"operation":"capture_electron_scenario","issues":[{"path":["executable_path"],"reason":"invalid_value","message":"executable_path <file> is not an executable file: EACCES: permission denied, access '<file>'"}]}} ``` For the script that exits at once, the exit code is 1 and the code is `execution_failure`. The diagnostics say `Process failed to launch!`. The MCP server stays up. 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:focused -- tests/acceptance/applications/electronInvalidPaths.test.ts src/browser/PlaywrightElectronActiveProvider.test.ts tests/acceptance/applications/electronBlankPathCli.test.ts tests/boundary/mcp/electronMcp.test.ts tests/boundary/process/electronLaunchRejections.test.ts` on Node 22.23.1, 24.18.0 and 26.11.1: 5 files and 25 tests passed on each. - The same command on unfixed main, on all three Node versions: 2 of the 3 tests in the new file failed. - I broke the fix four ways and ran the new file against each. Skipping the one-turn wait failed 1 test. Never removing the listener failed 9. Skipping the shared record of absorbed errors failed 2. Dropping every held rejection, which is the old behavior, failed 3. - `npm run test:focused` on the 22 test files with "electron" in the name, on Node 24: 342 tests passed. - `npm run check:fast` on Node 24: passed. - `npm run verify:test-discovery`: all 1039 test files found once. - `oxfmt --check` on the four changed files: passed. - Manual CLI and MCP runs on the built tree, before and after, with five files. They were a `.har`, a `.txt`, a script that exits, a mode 755 text file and a script that sleeps. After the fix, none ended the process. After the revision I reran three of them through the CLI. They were the script that exits at once, the mode 644 file, and a script that sleeps 300 s. The last one hits the real 60 s timeout. None ended the process. I did not rerun the MCP checks on the revised code. Not run: the full suite, `npm run check:pr`, `npm run docs:check`, `npm run verify:package`, Windows, macOS, and a real Electron launch. ## Compatibility, safety, and release - Breaking changes or migration steps: none. On Linux, a non-executable file used to give `execution_failure`. It now gives `invalid_request`. - Real Hopper/Ghidra, browser, or OS coverage: Linux only. - Package or release metadata impact: none. - Security, privacy, process, or containment review: a failed launch no longer ends the process. Playwright leaves extra errors behind, and the launch now drops them. Every other rejection still does. That includes an unrelated Playwright one. The match needs a Playwright path in the stack, and the launch error must contain the orphan's message. If Playwright changes either, the old crash comes back. It does not hide new bugs. Two captures at once share one record of absorbed errors, so neither raises the other's. One gap remains. Say an unrelated Playwright error has the same message as the launch failure. If it lands in the same window, it is dropped too. ## 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. 🤖 Generated with Claude Code (Sonnet 5.5, verified by Opus 5.5) 🦊 --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 01:05:46 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

2026-11-01 21:43:58 +00:00

[PR #5372] [MERGED] fix(pwndbg): separate the words in the debugger-context limit message #6906

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

No description provided.

Rows
Columns