[PR #1611] [CLOSED] fix(browser): redact escaped scenario artifact secrets #3004

Closed
opened 2026-10-17 21:12:47 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/1611
Author: @Aliciawque
Created: 10/10/2026
Status: ❌ Closed

Base: main ← Head: fix/redact-escaped-browser-artifacts-2026-10-10


📝 Commits (2)

  • a5872d4 fix(browser): redact escaped scenario artifact secrets
  • d2e3d5c fix(browser): integrate upstream redaction hardening

📊 Changes

5 files changed (+320 additions, -2 deletions)

View changed files

📝 docs/browser-scenario-contract.md (+13 -0)
📝 src/browser/BrowserScenario.test.ts (+107 -0)
📝 src/browser/BrowserScenarioSecrets.ts (+100 -0)
📝 src/browser/PlaywrightScenarioArtifacts.test.ts (+98 -0)
📝 src/browser/PlaywrightScenarioArtifacts.ts (+2 -2)

📄 Description

Integration update (2026-10-11 00:02 UTC)

Published head: d2e3d5ce1f7a125e065287b873d0203aeb9d1825. This is a non-force, two-parent integration commit retaining the original head and upstream c9168508527302ac57bc86385ccf0d824697b5dc. The reviewed remote tree is d8c698cfe77c672936ab264bcd56465db4e3ca4a.

Resolve the two conflicts in BrowserScenarioSecrets.ts and PlaywrightScenarioArtifacts.test.ts by retaining upstream #1590's safe-marker/raw/URL/byte behavior and storage sanitization, alongside this PR's artifact matcher and both test sets. Add four artifact-marker integration cases. The delta against the integrated upstream remains five files; #1612's complementary accessibility quoting/whitespace behavior is not included or claimed combined-tested.

Validation on Linux, Node 24.18.0/npm 11.16.0:

  • Six focused browser suites: 82 tests passed, including marker and storage regressions
  • CLI/MCP boundary suites: 3 tests passed (85 focused tests total)
  • npm run check, npm run docs:check, npm run build:cached, and git diff --check: passed; 664 existing nonfatal lint warnings remain
  • npm run verify:test-discovery: all 794 test files discovered exactly once, not a full-suite execution
  • Independent review of this exact merged source: 187 assertions across 51 synthetic spellings passed, including marker fallbacks, reconstructed-secret fail-closed behavior, retained bytes/SHA-256, binary preservation, and combined cookie/storage sanitization
  • Fresh supported npm run verify:browser with /usr/bin/chromium and sandbox enabled: blocked before page load by Chromium process socket() EPERM/SIGABRT, with a crashpad directory error. Cleanup ran; no security bypass or local live-browser success is claimed
  • Exact-head CI and real-browser verification: action_required, awaiting maintainer approval, zero checks executed

CassieFox23's external reproduction reports that original head a5872d44a fixes the original leaks on Linux/Chromium 154 and that its focused tests pass on Node 22/24/26. Those are third-party results on the original head, not this integration's CI or locally executed browser evidence. Their later main-branch check also distinguishes #1590's marker/storage scope from these escaped artifact spellings.

Upstream advanced to 5485483f7da2f6cb66441bb784aaf2a1a5f8731a during verification, changing only JavaScript export analysis and its tests. Local results above are specifically for integration base c9168508; GitHub reports this PR mergeable after the push. The PR remains a draft.

Summary

Fixes #1608.

Remove declared secrets from DOM and accessibility artifact text when the browser represents them using standard HTML or URI/form escaping. Preserve unrelated evidence and compute artifact identity from the retained, redacted text.

Problem and expected behavior

A declared synthetic value such as SECRET-a&b-7f3a appears in page.content() as SECRET-a&b-7f3a; SECRET c8 7f3a can appear in both DOM and accessibility link URLs as SECRET%20c8%207f3a. Raw-string replacement misses these spellings, contrary to the declared-secret redaction contract.

The executed mocked-artifact regression reproduces these misses on main 0d89978333a7f139c53f35f1bb622d08eb73affa. The actual browser reproduction reported in the issue is external evidence; local live-browser execution was blocked as described below.

Change and scope

Add an artifact-specific matcher owned by BrowserScenarioSecrets, used only by the DOM and accessibility capture paths. It recognizes raw values, browser HTML text/attribute serialization, URI/component/form serialization, and URI spellings inside HTML. Hexadecimal percent-escape digits can use either case; literal letters remain case-sensitive.

Match original snapshot spans, prefer the longest match at the same position, and use a stable secret ID for ties. Do not decode or normalize surrounding evidence or rematch inserted markers. Empty declarations are ignored; lone surrogates do not prevent raw/HTML redaction.

Marker creation stays with the existing raw-redaction policy. A final supported-spelling check retains empty text if replacement reconstructs a declared secret across a marker boundary. The artifact byte count and SHA-256 describe that empty retained representation. This small guard was required by an independently reproduced interaction with the exact #1590 patch; no marker-selection, cookie, or storage changes from that PR are included here.

Contract and boundary impact

  • Semantic owner and earliest changed stage: declared-secret matching in the browser scenario owner, before DOM/accessibility text artifact hashing
  • CLI and MCP/tool-catalog contract: no schema or tool changes; both adapters receive the shared artifact representation
  • Provider, bridge, target-format, or platform compatibility: Playwright DOM/accessibility snapshots; no bridge or browser-lifecycle changes
  • Evidence, artifact, provenance, or reconstruction contract: unrelated snapshot text is preserved; bytes and SHA-256 identify retained redacted text, including documented empty fail-closed output
  • Process execution, authorization, cleanup, or containment impact: none
  • Generated metadata, package, or installation impact: none; no version bump or generated output is committed

Evidence and regression coverage

  • Tests added or updated: HTML text/attributes, URI/component/form and mixed hex case, Unicode and lone surrogates, newlines, regex punctuation, empty/no-secret controls, overlapping/colliding spellings, ordinary-suffix preservation, residual reconstruction, and retained/empty artifact identity
  • Base reproduction or other evidence: the new mocked Playwright artifact regression fails on original main (one new failure; two prior tests pass), then passes with the patch; independent review also tests the exact #1590 composition
  • User-visible CLI/MCP output: production-owner regression observes DOM text such as <p>seen [REDACTED:text_token]</p> and accessibility /url: /next?t=[REDACTED:link_token]&plain=kept; these are executed adapter outputs, not claimed live CLI captures
  • Remaining proof gaps: live browser execution, external-connect mode, full repository test suite, and other operating systems were not verified

For evidence-bearing changes:

  • Observed, derived, and inferred claims remain distinguishable
  • Artifact identity is verified against the retained representation; provenance and failed-capture paths are unchanged
  • Unavailable coverage and bounded redaction behavior are documented

Original submission validation

Final base: c1217e711b7260a04a75f58d46c97e1320595153. Linux, repository-pinned Node 24.18.0 and npm 11.16.0.

  • npm run test:focused -- src/browser/BrowserScenario.test.ts src/browser/PlaywrightScenarioArtifacts.test.ts src/browser/PlaywrightScenarioEvents.test.ts: 38 tests passed
  • npm run check: passed, including typecheck, lint/module boundaries, formatting, knip, metadata, and architecture guards; repository-wide nonfatal lint warnings remain
  • npm run docs:check: passed
  • npm run verify:test-discovery: all 766 test files discovered exactly once
  • git diff --check: passed
  • Independent synthetic validation: 17 tests passed, including 1,467 bounded spellings across 256 invented secrets and both exact-#1590 composition regressions
  • Local Chromium 154.0.8037.57 CLI fixture, with invented secrets, localhost, and disposable profiles: attempted on baseline and patched code, but Chromium's required process socket fails with EPERM before page load, including through the supported elevated execution route; no live-browser success is claimed

Compatibility, safety, and release

  • Breaking changes or migration steps: none
  • Real Hopper/Ghidra, browser, or OS coverage: native analysis providers are not affected; Linux source/static checks passed; the real-browser lane was unavailable and macOS/Windows were not run
  • Package or release metadata impact: none
  • Security, privacy, process, or containment review: only explicitly declared synthetic secrets were used. This is bounded spelling matching, not general-purpose sanitization. Arbitrary entities, repeated encoding, base64, page-defined transformations, and additional accessibility quoting/whitespace normalization are outside scope. Raw, URL, byte, cookie, and storage behavior outside these artifacts remains unchanged.

Review checklist

  • This PR addresses a concrete problem
  • The PR has one focused outcome and the title follows type(scope): outcome
  • Related issue is linked
  • Tests cover changed observable behavior and meaningful failure paths
  • Owning documentation is updated; generated metadata changes are not needed
  • Live user-visible CLI/MCP output verified; browser startup is blocked as documented
  • Final diff checked 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/1611 **Author:** [@Aliciawque](https://github.com/Aliciawque) **Created:** 10/10/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/redact-escaped-browser-artifacts-2026-10-10` --- ### 📝 Commits (2) - [`a5872d4`](https://github.com/morluto/rea/commit/a5872d44a92e18dae999dcecb4ee5d409845628c) fix(browser): redact escaped scenario artifact secrets - [`d2e3d5c`](https://github.com/morluto/rea/commit/d2e3d5ce1f7a125e065287b873d0203aeb9d1825) fix(browser): integrate upstream redaction hardening ### 📊 Changes **5 files changed** (+320 additions, -2 deletions) <details> <summary>View changed files</summary> 📝 `docs/browser-scenario-contract.md` (+13 -0) 📝 `src/browser/BrowserScenario.test.ts` (+107 -0) 📝 `src/browser/BrowserScenarioSecrets.ts` (+100 -0) 📝 `src/browser/PlaywrightScenarioArtifacts.test.ts` (+98 -0) 📝 `src/browser/PlaywrightScenarioArtifacts.ts` (+2 -2) </details> ### 📄 Description ## Integration update (2026-10-11 00:02 UTC) Published head: `d2e3d5ce1f7a125e065287b873d0203aeb9d1825`. This is a non-force, two-parent integration commit retaining the original head and upstream `c9168508527302ac57bc86385ccf0d824697b5dc`. The reviewed remote tree is `d8c698cfe77c672936ab264bcd56465db4e3ca4a`. Resolve the two conflicts in `BrowserScenarioSecrets.ts` and `PlaywrightScenarioArtifacts.test.ts` by retaining upstream #1590's safe-marker/raw/URL/byte behavior and storage sanitization, alongside this PR's artifact matcher and both test sets. Add four artifact-marker integration cases. The delta against the integrated upstream remains five files; #1612's complementary accessibility quoting/whitespace behavior is not included or claimed combined-tested. Validation on Linux, Node 24.18.0/npm 11.16.0: - Six focused browser suites: 82 tests passed, including marker and storage regressions - CLI/MCP boundary suites: 3 tests passed (85 focused tests total) - `npm run check`, `npm run docs:check`, `npm run build:cached`, and `git diff --check`: passed; 664 existing nonfatal lint warnings remain - `npm run verify:test-discovery`: all 794 test files discovered exactly once, not a full-suite execution - Independent review of this exact merged source: 187 assertions across 51 synthetic spellings passed, including marker fallbacks, reconstructed-secret fail-closed behavior, retained bytes/SHA-256, binary preservation, and combined cookie/storage sanitization - Fresh supported `npm run verify:browser` with `/usr/bin/chromium` and sandbox enabled: blocked before page load by Chromium process `socket()` EPERM/SIGABRT, with a crashpad directory error. Cleanup ran; no security bypass or local live-browser success is claimed - Exact-head [CI](https://github.com/morluto/rea/actions/runs/38097056922) and [real-browser verification](https://github.com/morluto/rea/actions/runs/38097056902): `action_required`, awaiting maintainer approval, zero checks executed CassieFox23's [external reproduction](https://github.com/morluto/rea/issues/1608#issuecomment-6101125011) reports that original head `a5872d44a` fixes the original leaks on Linux/Chromium 154 and that its focused tests pass on Node 22/24/26. Those are third-party results on the original head, not this integration's CI or locally executed browser evidence. Their [later main-branch check](https://github.com/morluto/rea/issues/1608#issuecomment-6103413411) also distinguishes #1590's marker/storage scope from these escaped artifact spellings. Upstream advanced to `5485483f7da2f6cb66441bb784aaf2a1a5f8731a` during verification, changing only JavaScript export analysis and its tests. Local results above are specifically for integration base `c9168508`; GitHub reports this PR mergeable after the push. The PR remains a draft. ## Summary Fixes #1608. Remove declared secrets from DOM and accessibility artifact text when the browser represents them using standard HTML or URI/form escaping. Preserve unrelated evidence and compute artifact identity from the retained, redacted text. ## Problem and expected behavior A declared synthetic value such as `SECRET-a&b-7f3a` appears in `page.content()` as `SECRET-a&amp;b-7f3a`; `SECRET c8 7f3a` can appear in both DOM and accessibility link URLs as `SECRET%20c8%207f3a`. Raw-string replacement misses these spellings, contrary to the declared-secret redaction contract. The executed mocked-artifact regression reproduces these misses on main `0d89978333a7f139c53f35f1bb622d08eb73affa`. The actual browser reproduction reported in the issue is external evidence; local live-browser execution was blocked as described below. ## Change and scope Add an artifact-specific matcher owned by `BrowserScenarioSecrets`, used only by the DOM and accessibility capture paths. It recognizes raw values, browser HTML text/attribute serialization, URI/component/form serialization, and URI spellings inside HTML. Hexadecimal percent-escape digits can use either case; literal letters remain case-sensitive. Match original snapshot spans, prefer the longest match at the same position, and use a stable secret ID for ties. Do not decode or normalize surrounding evidence or rematch inserted markers. Empty declarations are ignored; lone surrogates do not prevent raw/HTML redaction. Marker creation stays with the existing raw-redaction policy. A final supported-spelling check retains empty text if replacement reconstructs a declared secret across a marker boundary. The artifact byte count and SHA-256 describe that empty retained representation. This small guard was required by an independently reproduced interaction with the exact #1590 patch; no marker-selection, cookie, or storage changes from that PR are included here. ## Contract and boundary impact - Semantic owner and earliest changed stage: declared-secret matching in the browser scenario owner, before DOM/accessibility text artifact hashing - CLI and MCP/tool-catalog contract: no schema or tool changes; both adapters receive the shared artifact representation - Provider, bridge, target-format, or platform compatibility: Playwright DOM/accessibility snapshots; no bridge or browser-lifecycle changes - Evidence, artifact, provenance, or reconstruction contract: unrelated snapshot text is preserved; bytes and SHA-256 identify retained redacted text, including documented empty fail-closed output - Process execution, authorization, cleanup, or containment impact: none - Generated metadata, package, or installation impact: none; no version bump or generated output is committed ## Evidence and regression coverage - Tests added or updated: HTML text/attributes, URI/component/form and mixed hex case, Unicode and lone surrogates, newlines, regex punctuation, empty/no-secret controls, overlapping/colliding spellings, ordinary-suffix preservation, residual reconstruction, and retained/empty artifact identity - Base reproduction or other evidence: the new mocked Playwright artifact regression fails on original main (one new failure; two prior tests pass), then passes with the patch; independent review also tests the exact #1590 composition - User-visible CLI/MCP output: production-owner regression observes DOM text such as `<p>seen [REDACTED:text_token]</p>` and accessibility `/url: /next?t=[REDACTED:link_token]&plain=kept`; these are executed adapter outputs, not claimed live CLI captures - Remaining proof gaps: live browser execution, external-connect mode, full repository test suite, and other operating systems were not verified For evidence-bearing changes: - [x] Observed, derived, and inferred claims remain distinguishable - [x] Artifact identity is verified against the retained representation; provenance and failed-capture paths are unchanged - [x] Unavailable coverage and bounded redaction behavior are documented ## Original submission validation Final base: `c1217e711b7260a04a75f58d46c97e1320595153`. Linux, repository-pinned Node 24.18.0 and npm 11.16.0. - `npm run test:focused -- src/browser/BrowserScenario.test.ts src/browser/PlaywrightScenarioArtifacts.test.ts src/browser/PlaywrightScenarioEvents.test.ts`: 38 tests passed - `npm run check`: passed, including typecheck, lint/module boundaries, formatting, knip, metadata, and architecture guards; repository-wide nonfatal lint warnings remain - `npm run docs:check`: passed - `npm run verify:test-discovery`: all 766 test files discovered exactly once - `git diff --check`: passed - Independent synthetic validation: 17 tests passed, including 1,467 bounded spellings across 256 invented secrets and both exact-#1590 composition regressions - Local Chromium 154.0.8037.57 CLI fixture, with invented secrets, localhost, and disposable profiles: attempted on baseline and patched code, but Chromium's required process socket fails with `EPERM` before page load, including through the supported elevated execution route; no live-browser success is claimed ## Compatibility, safety, and release - Breaking changes or migration steps: none - Real Hopper/Ghidra, browser, or OS coverage: native analysis providers are not affected; Linux source/static checks passed; the real-browser lane was unavailable and macOS/Windows were not run - Package or release metadata impact: none - Security, privacy, process, or containment review: only explicitly declared synthetic secrets were used. This is bounded spelling matching, not general-purpose sanitization. Arbitrary entities, repeated encoding, base64, page-defined transformations, and additional accessibility quoting/whitespace normalization are outside scope. Raw, URL, byte, cookie, and storage behavior outside these artifacts remains unchanged. ## Review checklist - [x] This PR addresses a concrete problem - [x] The PR has one focused outcome and the title follows `type(scope): outcome` - [x] Related issue is linked - [x] Tests cover changed observable behavior and meaningful failure paths - [x] Owning documentation is updated; generated metadata changes are not needed - [ ] Live user-visible CLI/MCP output verified; browser startup is blocked as documented - [x] Final diff checked 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-10-17 21:12:47 +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#3004
No description provided.