morluto/rea

Fork 0

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

[PR #6244] [MERGED] fix(capture): report scenario path search denial as access denied #7222

New issue

Closed

opened 2026-11-01 21:46:16 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-11-01 21:46:16 +00:00

Owner

📋 Pull Request Information

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

Base: main ← Head: fix/6117-electron-path-read-denial

📝 Commits (1)

fa54440 fix(capture): report scenario path search denial as access denied

📊 Changes

5 files changed (+505 additions, -50 deletions)

View changed files

➕ src/application/javascript/ElectronActiveObservationService.test.ts (+190 -0)
📝 src/application/javascript/ElectronActiveObservationService.ts (+93 -31)
📝 src/process/capture/ProcessScenarioRuntimeValidation.test.ts (+120 -2)
📝 src/process/capture/ProcessScenarioRuntimeValidation.ts (+61 -17)
📝 tests/boundary/cli/scenarioCliInputDiagnostics.test.ts (+41 -0)

📄 Description

What broke: capture_electron_scenario (and the sibling capture_process_scenario admission) reported a correct path beneath a directory without search permission as invalid_request / invalid_value with inspect_input_contract recovery, because every realpath/stat failure became an input issue.
Fix: Path admission now classifies EACCES/EPERM from path resolution and metadata reads as the existing AnalysisAccessDeniedError (access_denied, selected path, system_code, filesystem-read). Missing or unresolvable paths and reachable files without execute permission keep invalid_request.
Risk: Low. No schema change; only the denial case changes outcome. With mixed failures, Electron still reports the invalid fields first; process PATH search still skips denied directories.

Summary

Fixes #6117.

Both scenario admissions resolve caller-selected paths before any provider or child process starts. A host permission denial during that resolution is not malformed caller input (AnalysisAccessDeniedError contract in src/domain/analysisErrorCore.ts), so it now uses the existing access-denied outcome rather than schema-correction guidance. This matches the selected-input classification in JavaScriptArtifactReconstruction.resolveSelectedInput.

Problem and expected behavior

Smallest trigger: application_path = <tmp>/denied/main.js with <tmp>/denied at mode 000 (non-root). Before: invalid_request, issue ["application_path"] invalid_value "Cannot resolve application_path …: EACCES …". Expected and now observed: access_denied, category unavailable, details.path = the selected path, system_code: EACCES, boundary: filesystem-read, with the read-access remediation.

Change and scope

src/application/javascript/ElectronActiveObservationService.ts: each path admission returns a tagged failure (invalid issue or denied error). EACCES/EPERM from realpath (all three path fields) and from the executable stat become AnalysisAccessDeniedError for the selected path. The X_OK check is separate, so a reachable executable without execute permission (mode 0644 or 0000) stays an executable_path input issue (#2705 behavior). If any field is invalid, the input error lists only the invalid fields (the denial stays in the aggregate cause); a denial is returned only when no field is invalid.
src/process/capture/ProcessScenarioRuntimeValidation.ts: the same mistake at working_directory realpath/stat and at an explicitly selected executable. Candidates are now stated before X_OK so a denied lookup can be told apart from a non-executable file. A denied explicit executable reports access_denied for the resolved candidate. PATH search still records (EACCES) and moves on to later directories, as before.
Checked and not changed: the browser scenario executable_path has no admission path check (Playwright launch owns it). JavaScript application input admission is handled separately in #6116.
Not included: admission still does not check that application_path is readable. A mode-000 application file under a searchable directory is admitted, as before, and fails at launch.

Contribution scope: not applicable. This is an ordinary fix within existing boundaries.

Contract and boundary impact

Semantic owner and earliest changed stage: the Electron and process scenario path admission, before provider work or spawn.
Contribution scope fit and linked maintainer decision: not applicable (ordinary fix).
CLI and MCP/tool-catalog contract: no schema change. access_denied is already part of the error contract. CLI and MCP share the changed service; MCP was checked through the existing electronMcp boundary test (ENOENT path), not with a denial.
Provider, bridge, target-format, or platform compatibility: none. Windows behavior is unchanged except that executable candidates are stated before access.
Evidence, artifact, provenance, or reconstruction contract: none. The selected path and native code are kept in the structured error.
Target selection, authority, upstream behavior, and cleanup details: unchanged; denial still fails closed before any launch.
Generated metadata, package, or installation impact: none.
Read existing files or captured data.
Launch or attach to a process.

(These describe what the existing tools already do. This PR adds no new effects; admission adds one stat of the selected executable before the existing access check.)

Evidence and regression coverage

Tests added or updated:
- ElectronActiveObservationService.test.ts (new): search-denied application_path, application_root and executable_path each give access_denied with path/system_code/boundary and the provider is never called; missing application_path plus denied application_root gives invalid_request with only the missing field; non-executable executables at mode 0644 and 0000 stay invalid input; readable selections reach the provider with canonical paths (a recording stub; nothing is launched).
- ProcessScenarioRuntimeValidation.test.ts: denied working_directory and denied explicit executable give access_denied; PATH search resolves a later directory past a denied one, and a PATH with only a denied directory stays invalid_request with (EACCES).
- tests/boundary/cli/scenarioCliInputDiagnostics.test.ts: the built CLI reports the issue's application_path case as access_denied.
Base reproduction: with only the two source files reverted, 6 of the new assertions fail (3 Electron denial fields, Electron mixed failure, process working_directory, process explicit executable).

Built-CLI matrix (issue procedure, mode-000 parent, permissions restored in finally; 13 cases, about 0.37 s each, before and after):

Case	Before	After
Electron `application_path` / `application_root` / `executable_path` under denied dir	`invalid_request`	`access_denied` (`EACCES`, `filesystem-read`)
Process `working_directory` / explicit `executable` under denied dir	`invalid_request`	`access_denied` (`EACCES`, `filesystem-read`)
Electron missing + denied (mixed)	`invalid_request`, 2 issues	`invalid_request`, missing field only
Missing or relative path, non-executable or mode-000 executable, PATH-only denied dir, missing `working_directory`	`invalid_request`	`invalid_request` (unchanged)

CLI output after the fix (issue case, fixture location replaced by <tmp>):

{"error":"Electron analysis failed","code":"access_denied","category":"unavailable","message":"Host filesystem permissions denied read access to the selected path.","retryable":false,"remediation":{"action":"Check the current process's read access to the selected path. Retry with a readable local file."},"details":{"operation":"capture_electron_scenario","path":"<tmp>/denied/main.js","system_code":"EACCES","boundary":"filesystem-read"}}

Remaining proof gaps: EPERM was not exercised (it is classified the same as EACCES). Windows, root, and MCP with a real denial were not run. No Electron, browser or child process was launched.
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

Issue repro plus the counterexample matrix on the built CLI, before and after the fix (macOS arm64, non-root, Node.js v26.5.0; the 24.18.0 pin was not separately exercised).
npm run test:focused -- tests/boundary/cli/scenarioCliInputDiagnostics.test.ts tests/boundary/mcp/electronMcp.test.ts src/application/javascript/ElectronActiveObservationService.test.ts src/process/capture/ProcessScenarioRuntimeValidation.test.ts: 30 passed.
npm run test:local -- src/process/capture/*.test.ts src/application/javascript/*.test.ts (33 files): 679 passed, 4 skipped.
npm run check:fast: passed.
npm run format:check: passed.

Compatibility, safety, and release

Breaking changes or migration steps: callers that matched invalid_request for a permission-denied scenario path now receive access_denied.
Real Hopper/Ghidra, browser, or OS coverage: macOS only; no provider launched.
Package or release metadata impact: none.
Security, privacy, process, or containment review: the denial still fails closed before any launch, and no new filesystem access is added beyond a stat before the existing access check.

Author readiness

Human owner: @kaoru0822-kitauji

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/6244 **Author:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Created:** 10/22/2026 **Status:** ✅ Merged **Merged:** 10/22/2026 **Merged by:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Base:** `main` ← **Head:** `fix/6117-electron-path-read-denial` --- ### 📝 Commits (1) - [`fa54440`](https://github.com/morluto/rea/commit/fa544405999e3b9b1cc9a211de6c4676484b0bfb) fix(capture): report scenario path search denial as access denied ### 📊 Changes **5 files changed** (+505 additions, -50 deletions) <details> <summary>View changed files</summary> ➕ `src/application/javascript/ElectronActiveObservationService.test.ts` (+190 -0) 📝 `src/application/javascript/ElectronActiveObservationService.ts` (+93 -31) 📝 `src/process/capture/ProcessScenarioRuntimeValidation.test.ts` (+120 -2) 📝 `src/process/capture/ProcessScenarioRuntimeValidation.ts` (+61 -17) 📝 `tests/boundary/cli/scenarioCliInputDiagnostics.test.ts` (+41 -0) </details> ### 📄 Description **What broke:** `capture_electron_scenario` (and the sibling `capture_process_scenario` admission) reported a correct path beneath a directory without search permission as `invalid_request` / `invalid_value` with `inspect_input_contract` recovery, because every `realpath`/`stat` failure became an input issue. **Fix:** Path admission now classifies `EACCES`/`EPERM` from path resolution and metadata reads as the existing `AnalysisAccessDeniedError` (`access_denied`, selected path, `system_code`, `filesystem-read`). Missing or unresolvable paths and reachable files without execute permission keep `invalid_request`. **Risk:** Low. No schema change; only the denial case changes outcome. With mixed failures, Electron still reports the invalid fields first; process PATH search still skips denied directories. ## Summary Fixes #6117. Both scenario admissions resolve caller-selected paths before any provider or child process starts. A host permission denial during that resolution is not malformed caller input (`AnalysisAccessDeniedError` contract in `src/domain/analysisErrorCore.ts`), so it now uses the existing access-denied outcome rather than schema-correction guidance. This matches the selected-input classification in `JavaScriptArtifactReconstruction.resolveSelectedInput`. ## Problem and expected behavior Smallest trigger: `application_path` = `<tmp>/denied/main.js` with `<tmp>/denied` at mode 000 (non-root). Before: `invalid_request`, issue `["application_path"] invalid_value "Cannot resolve application_path …: EACCES …"`. Expected and now observed: `access_denied`, category `unavailable`, `details.path` = the selected path, `system_code: EACCES`, `boundary: filesystem-read`, with the read-access remediation. ## Change and scope - `src/application/javascript/ElectronActiveObservationService.ts`: each path admission returns a tagged failure (`invalid` issue or `denied` error). `EACCES`/`EPERM` from `realpath` (all three path fields) and from the executable `stat` become `AnalysisAccessDeniedError` for the selected path. The `X_OK` check is separate, so a reachable executable without execute permission (mode 0644 or 0000) stays an `executable_path` input issue (#2705 behavior). If any field is invalid, the input error lists only the invalid fields (the denial stays in the aggregate cause); a denial is returned only when no field is invalid. - `src/process/capture/ProcessScenarioRuntimeValidation.ts`: the same mistake at `working_directory` `realpath`/`stat` and at an explicitly selected `executable`. Candidates are now `stat`ed before `X_OK` so a denied lookup can be told apart from a non-executable file. A denied explicit executable reports `access_denied` for the resolved candidate. PATH search still records `(EACCES)` and moves on to later directories, as before. - Checked and not changed: the browser scenario `executable_path` has no admission path check (Playwright launch owns it). JavaScript application input admission is handled separately in #6116. - Not included: admission still does not check that `application_path` is readable. A mode-000 application file under a searchable directory is admitted, as before, and fails at launch. Contribution scope: not applicable. This is an ordinary fix within existing boundaries. ## Contract and boundary impact - Semantic owner and earliest changed stage: the Electron and process scenario path admission, before provider work or spawn. - Contribution scope fit and linked maintainer decision: not applicable (ordinary fix). - CLI and MCP/tool-catalog contract: no schema change. `access_denied` is already part of the error contract. CLI and MCP share the changed service; MCP was checked through the existing `electronMcp` boundary test (ENOENT path), not with a denial. - Provider, bridge, target-format, or platform compatibility: none. Windows behavior is unchanged except that executable candidates are `stat`ed before `access`. - Evidence, artifact, provenance, or reconstruction contract: none. The selected path and native code are kept in the structured error. - Target selection, authority, upstream behavior, and cleanup details: unchanged; denial still fails closed before any launch. - Generated metadata, package, or installation impact: none. - [x] Read existing files or captured data. - [x] Launch or attach to a process. (These describe what the existing tools already do. This PR adds no new effects; admission adds one `stat` of the selected executable before the existing `access` check.) ## Evidence and regression coverage - Tests added or updated: - `ElectronActiveObservationService.test.ts` (new): search-denied `application_path`, `application_root` and `executable_path` each give `access_denied` with path/`system_code`/boundary and the provider is never called; missing `application_path` plus denied `application_root` gives `invalid_request` with only the missing field; non-executable executables at mode 0644 and 0000 stay invalid input; readable selections reach the provider with canonical paths (a recording stub; nothing is launched). - `ProcessScenarioRuntimeValidation.test.ts`: denied `working_directory` and denied explicit `executable` give `access_denied`; PATH search resolves a later directory past a denied one, and a PATH with only a denied directory stays `invalid_request` with `(EACCES)`. - `tests/boundary/cli/scenarioCliInputDiagnostics.test.ts`: the built CLI reports the issue's `application_path` case as `access_denied`. - Base reproduction: with only the two source files reverted, 6 of the new assertions fail (3 Electron denial fields, Electron mixed failure, process `working_directory`, process explicit executable). - Built-CLI matrix (issue procedure, mode-000 parent, permissions restored in `finally`; 13 cases, about 0.37 s each, before and after): | Case | Before | After | |---|---|---| | Electron `application_path` / `application_root` / `executable_path` under denied dir | `invalid_request` | `access_denied` (`EACCES`, `filesystem-read`) | | Process `working_directory` / explicit `executable` under denied dir | `invalid_request` | `access_denied` (`EACCES`, `filesystem-read`) | | Electron missing + denied (mixed) | `invalid_request`, 2 issues | `invalid_request`, missing field only | | Missing or relative path, non-executable or mode-000 executable, PATH-only denied dir, missing `working_directory` | `invalid_request` | `invalid_request` (unchanged) | - CLI output after the fix (issue case, fixture location replaced by `<tmp>`): ```json {"error":"Electron analysis failed","code":"access_denied","category":"unavailable","message":"Host filesystem permissions denied read access to the selected path.","retryable":false,"remediation":{"action":"Check the current process's read access to the selected path. Retry with a readable local file."},"details":{"operation":"capture_electron_scenario","path":"<tmp>/denied/main.js","system_code":"EACCES","boundary":"filesystem-read"}} ``` - Remaining proof gaps: `EPERM` was not exercised (it is classified the same as `EACCES`). Windows, root, and MCP with a real denial were not run. No Electron, browser or child process was launched. - [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 - Issue repro plus the counterexample matrix on the built CLI, before and after the fix (macOS arm64, non-root, Node.js v26.5.0; the 24.18.0 pin was not separately exercised). - `npm run test:focused -- tests/boundary/cli/scenarioCliInputDiagnostics.test.ts tests/boundary/mcp/electronMcp.test.ts src/application/javascript/ElectronActiveObservationService.test.ts src/process/capture/ProcessScenarioRuntimeValidation.test.ts`: 30 passed. - `npm run test:local -- src/process/capture/*.test.ts src/application/javascript/*.test.ts` (33 files): 679 passed, 4 skipped. - `npm run check:fast`: passed. - `npm run format:check`: passed. ## Compatibility, safety, and release - Breaking changes or migration steps: callers that matched `invalid_request` for a permission-denied scenario path now receive `access_denied`. - Real Hopper/Ghidra, browser, or OS coverage: macOS only; no provider launched. - Package or release metadata impact: none. - Security, privacy, process, or containment review: the denial still fails closed before any launch, and no new filesystem access is added beyond a `stat` before the existing `access` check. ## Author readiness Human owner: @kaoru0822-kitauji - [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. 🤖 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-11-01 21:46:16 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

2026-11-03 13:39:55 +00:00

[PR #7222] [MERGED] fix(javascript): honor effective symbolic descriptor values #7726

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

No description provided.

Rows
Columns