[PR #6418] [CLOSED] fix(electron): admit file pages whose query or fragment encodes a separator #7347

Closed
opened 2026-11-01 21:47:13 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/6418
Author: @viniciusdsandrade
Created: 10/22/2026
Status: ❌ Closed

Base: main ← Head: fix/5872-electron-encoded-separators-outside-path


📝 Commits (3)

  • 36ff03a fix(electron): admit file pages whose query or fragment encodes a separator
  • 3d19fe4 test(electron): list and inspect pages with encoded separators after the path
  • 7ef0211 test(electron): keep refusing an encoded backslash in a POSIX file path

📊 Changes

3 files changed (+58 additions, -1 deletions)

View changed files

📝 src/browser/ElectronFileScope.ts (+2 -1)
📝 tests/boundary/browser/cdpElectronProvider.test.ts (+42 -0)
📝 tests/boundary/filesystem/electronFileScope.test.ts (+14 -0)

📄 Description

Summary

Fixes #5872

list_electron_targets excluded a local file page as unsupported_url, and inspect_electron_page refused it as target_not_allowed, when only its query or fragment contained %2F or %5C, for example index.html?next=%2Fsettings or index.html#%5Csettings. The encoded-separator guard in the shared Electron file authorization tested the complete URL string before new URL separated its components, while fileURLToPath selects the file from the path alone.

The guard now tests the parsed pathname. Query and fragment text no longer affects authorization, and an encoded separator in the path is still refused.

Problem and expected behavior

  • Smallest trigger: a CDP page target whose URL is the file URL of a regular file followed by ?next=%2Fsettings, ?next=%5Csettings, #%2Fsettings or #%5Csettings.
  • Violated invariant: docs/electron-observation.md accepts hostless local file:// URLs that resolve to regular files and rejects encoded path separators. Existing tests already accept query and fragment text (?isBookDoubleClicked=0#/library, a 70,000-character query). fileURLToPath ignores the query and fragment, so these URLs select the same file as the plain URL.
  • Expected: the four suffix forms are listed and inspected with the same file_path as the plain URL; an encoded separator in the path stays refused.

Change and scope

  • src/browser/ElectronFileScope.ts: the /%(?:2f|5c)/iu test moves after URL parsing and the protocol, host and credential checks, and applies to url.pathname. Once those checks pass, the path is the only remaining component that can select a file. src/domain/javascript/javascriptStaticAnalysisHelpers.ts already applies the same rule to the part before ? or #.
  • tests/boundary/browser/cdpElectronProvider.test.ts: lists and inspects a regular file page through the provider for each of the four suffixes from the issue.
  • tests/boundary/filesystem/electronFileScope.test.ts: on POSIX, refuses %5C in the path even when it decodes to an existing file name containing a backslash. There fileURLToPath decodes %5C without complaint, so this pins the guard that moved; it is skipped on Windows, where fileURLToPath itself refuses encoded separators in the path.
  • Not included: the encoded-separator checks in JavaScript module and source-map path resolution, which belong to other owners. No documentation change: the target boundary already describes the rule as rejecting encoded path separators.

Follow the contribution scope.
Scope decision: not applicable (ordinary fix within the existing Electron target authorization).

Contract and boundary impact

  • Semantic owner and earliest changed stage: resolveAuthorizedElectronFile in src/browser/ElectronFileScope.ts, behind authorizedElectronFile, which the Electron provider uses for targets, frames, scripts, resources and workers, and the V8 Inspector runtime scope uses for file locations.
  • Contribution scope fit and linked maintainer decision: not applicable.
  • CLI and MCP/tool-catalog contract: schemas unchanged. A file URL whose path names a regular file or ASAR member is authorized regardless of encoded separators in its query or fragment.
  • Provider, bridge, target-format, or platform compatibility: CDP Electron provider and V8 Inspector file scope on every platform.
  • Evidence, artifact, provenance, or reconstruction contract: unchanged; file_path is still the canonical path.
  • Target selection, authority, upstream behavior, and cleanup details (if applicable): authorization still requires a hostless file URL without credentials whose path names a regular file or an ASAR member, and still refuses encoded separators in the path. Admitted pages continue through the existing CDP attachment path unchanged.
  • 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: CdpElectronProvider URL suffixes in tests/boundary/browser/cdpElectronProvider.test.ts (four cases) and refuses an encoded backslash that names an existing POSIX file in tests/boundary/filesystem/electronFileScope.test.ts.
  • Base reproduction or other evidence: source-derived. On the base revision resolveAuthorizedElectronFile returns undefined at the whole-string test for each suffix, so listTargets counts the page under excluded.unsupported_url and the new provider test's targets expectation fails. The issue reports the same exclusion and the target_not_allowed refusal through the built CLI against a simulated CDP discovery endpoint. The POSIX path test passes on both revisions and guards the moved check.
  • User-visible CLI/MCP output (if applicable): by reading, the issue's seven-target reproduction now lists all seven targets with excluded.unsupported_url: 0 instead of three targets and unsupported_url: 4; not replayed here.
  • Remaining proof gaps: no real Electron application; the fake CDP browser supplies discovery and inspection replies.

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

  • No local test, build or typecheck commands were run; verification is source-derived and CI executes the test suite. The repository pre-commit hook ran oxlint --fix and oxfmt on the staged files without changes to the committed content.

Compatibility, safety, and release

  • Breaking changes or migration steps: none. Only file URLs with encoded separators confined to the query or fragment change outcome.
  • Real Hopper/Ghidra, browser, or OS coverage: none run.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: the path that selects a file is checked exactly as before; fileURLToPath ignores the query and fragment, so they never reach stat or ASAR resolution.

Author readiness

Human owner: @viniciusdsandrade

  • 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/6418 **Author:** [@viniciusdsandrade](https://github.com/viniciusdsandrade) **Created:** 10/22/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/5872-electron-encoded-separators-outside-path` --- ### 📝 Commits (3) - [`36ff03a`](https://github.com/morluto/rea/commit/36ff03a62c2746f47b6027c2847ec2d12dbccc0b) fix(electron): admit file pages whose query or fragment encodes a separator - [`3d19fe4`](https://github.com/morluto/rea/commit/3d19fe48efebd08818a29d6469089cea9677e99e) test(electron): list and inspect pages with encoded separators after the path - [`7ef0211`](https://github.com/morluto/rea/commit/7ef02110e9714a02cd113d5908c9e33d73c3cf23) test(electron): keep refusing an encoded backslash in a POSIX file path ### 📊 Changes **3 files changed** (+58 additions, -1 deletions) <details> <summary>View changed files</summary> 📝 `src/browser/ElectronFileScope.ts` (+2 -1) 📝 `tests/boundary/browser/cdpElectronProvider.test.ts` (+42 -0) 📝 `tests/boundary/filesystem/electronFileScope.test.ts` (+14 -0) </details> ### 📄 Description ## Summary Fixes #5872 `list_electron_targets` excluded a local file page as `unsupported_url`, and `inspect_electron_page` refused it as `target_not_allowed`, when only its query or fragment contained `%2F` or `%5C`, for example `index.html?next=%2Fsettings` or `index.html#%5Csettings`. The encoded-separator guard in the shared Electron file authorization tested the complete URL string before `new URL` separated its components, while `fileURLToPath` selects the file from the path alone. The guard now tests the parsed pathname. Query and fragment text no longer affects authorization, and an encoded separator in the path is still refused. ## Problem and expected behavior - Smallest trigger: a CDP page target whose URL is the file URL of a regular file followed by `?next=%2Fsettings`, `?next=%5Csettings`, `#%2Fsettings` or `#%5Csettings`. - Violated invariant: [docs/electron-observation.md](https://github.com/morluto/rea/blob/main/docs/electron-observation.md#target-boundary) accepts hostless local `file://` URLs that resolve to regular files and rejects encoded path separators. Existing tests already accept query and fragment text (`?isBookDoubleClicked=0#/library`, a 70,000-character query). `fileURLToPath` ignores the query and fragment, so these URLs select the same file as the plain URL. - Expected: the four suffix forms are listed and inspected with the same `file_path` as the plain URL; an encoded separator in the path stays refused. ## Change and scope - `src/browser/ElectronFileScope.ts`: the `/%(?:2f|5c)/iu` test moves after URL parsing and the protocol, host and credential checks, and applies to `url.pathname`. Once those checks pass, the path is the only remaining component that can select a file. `src/domain/javascript/javascriptStaticAnalysisHelpers.ts` already applies the same rule to the part before `?` or `#`. - `tests/boundary/browser/cdpElectronProvider.test.ts`: lists and inspects a regular file page through the provider for each of the four suffixes from the issue. - `tests/boundary/filesystem/electronFileScope.test.ts`: on POSIX, refuses `%5C` in the path even when it decodes to an existing file name containing a backslash. There `fileURLToPath` decodes `%5C` without complaint, so this pins the guard that moved; it is skipped on Windows, where `fileURLToPath` itself refuses encoded separators in the path. - Not included: the encoded-separator checks in JavaScript module and source-map path resolution, which belong to other owners. No documentation change: the target boundary already describes the rule as rejecting encoded path separators. Follow the [contribution scope](https://github.com/morluto/rea/blob/main/CONTRIBUTING.md#contribution-scope). Scope decision: not applicable (ordinary fix within the existing Electron target authorization). ## Contract and boundary impact - Semantic owner and earliest changed stage: `resolveAuthorizedElectronFile` in `src/browser/ElectronFileScope.ts`, behind `authorizedElectronFile`, which the Electron provider uses for targets, frames, scripts, resources and workers, and the V8 Inspector runtime scope uses for file locations. - Contribution scope fit and linked maintainer decision: not applicable. - CLI and MCP/tool-catalog contract: schemas unchanged. A file URL whose path names a regular file or ASAR member is authorized regardless of encoded separators in its query or fragment. - Provider, bridge, target-format, or platform compatibility: CDP Electron provider and V8 Inspector file scope on every platform. - Evidence, artifact, provenance, or reconstruction contract: unchanged; `file_path` is still the canonical path. - Target selection, authority, upstream behavior, and cleanup details (if applicable): authorization still requires a hostless file URL without credentials whose path names a regular file or an ASAR member, and still refuses encoded separators in the path. Admitted pages continue through the existing CDP attachment path unchanged. - 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: `CdpElectronProvider URL suffixes` in `tests/boundary/browser/cdpElectronProvider.test.ts` (four cases) and `refuses an encoded backslash that names an existing POSIX file` in `tests/boundary/filesystem/electronFileScope.test.ts`. - Base reproduction or other evidence: source-derived. On the base revision `resolveAuthorizedElectronFile` returns `undefined` at the whole-string test for each suffix, so `listTargets` counts the page under `excluded.unsupported_url` and the new provider test's `targets` expectation fails. The issue reports the same exclusion and the `target_not_allowed` refusal through the built CLI against a simulated CDP discovery endpoint. The POSIX path test passes on both revisions and guards the moved check. - User-visible CLI/MCP output (if applicable): by reading, the issue's seven-target reproduction now lists all seven targets with `excluded.unsupported_url: 0` instead of three targets and `unsupported_url: 4`; not replayed here. - Remaining proof gaps: no real Electron application; the fake CDP browser supplies discovery and inspection replies. 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 - No local test, build or typecheck commands were run; verification is source-derived and CI executes the test suite. The repository pre-commit hook ran `oxlint --fix` and `oxfmt` on the staged files without changes to the committed content. ## Compatibility, safety, and release - Breaking changes or migration steps: none. Only file URLs with encoded separators confined to the query or fragment change outcome. - Real Hopper/Ghidra, browser, or OS coverage: none run. - Package or release metadata impact: none. - Security, privacy, process, or containment review: the path that selects a file is checked exactly as before; `fileURLToPath` ignores the query and fragment, so they never reach `stat` or ASAR resolution. ## Author readiness Human owner: @viniciusdsandrade - [ ] 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 - [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>
bsakaguchi 2026-11-01 21:47:13 +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#7347
No description provided.