[krassx]

Fixes #120

Verified against main: replay-masking.browser.e2e.ts (PR #122) already covers blocked iframes, shadow DOM, stop/start/blackout and unmask controls. This closes the remaining gaps:

• document.open(): blocked iframe rewritten in place contributes nothing; unblocked iframe stays masked (recorded, post-rewrite edits masked); masking-off control per mode proves rewrite and post-rewrite edits are detectable and the blocked frame still contributes nothing.
• Attribute/value smoke: prefilled and typed text/password/card/email, textarea, alt, title, placeholder, aria-label, data-*, element text, both before start (snapshot) and added/typed after, raw + provider modes, plus a blockAllMedia-off variant so attribute masking itself is exercised. Masking-off controls (both modes, both phases) prove every non-floor category is detectable; password/card floor asserted absent even then.
• Mutation check: weakening masking in the scenario made all new tests fail; rolled back.
• WebKit: not added. CI installs only chromium; the suite hard-codes the chromium launcher and WebKit needs extra system deps on the runner, so it is not a cheap addition.

Test-only change (one file).

🤖 Generated with Claude Code

[coderabbitai]

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 3108b50c-7607-42bf-8284-d0d14b361310

Commits

Reviewing files that changed from the base of the PR and between 4d3cb27 and 2df4e85.

Files selected for processing: 1

• packages/instrumentation-tests/test/replay-masking.browser.e2e.ts

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Summary

Summary by CodeRabbit

• Tests
  • Added browser-based coverage for replay masking across input values, typed text, page text, and attributes.
  • Added checks for masking behavior in rewritten iframe content, including blocked and unblocked frames.

The PR adds real-Chromium replay-masking tests in raw and provider modes. The tests cover values, text, and attributes before and after recording starts, including masking defaults, media unblocking, and masking-disabled controls. They also cover iframe content rewritten with document.open(), checking blocked and unblocked frames and later edits.

Possibly related issues

• #119 — Adds real-browser coverage for iframe rewrite and re-observation paths described in the issue.

Possibly related PRs

• bugsee/bugsee-javascript#122 — Adds blocked-iframe recorder behavior that these tests exercise.
• bugsee/bugsee-javascript#141 — Changes iframe unblockSelector handling covered by the browser scenarios.
• bugsee/bugsee-javascript#117 — Updates iframe observer and masking behavior covered by these tests.

Suggested labels: javascript

Priority: Normal

Change: Other

Merge Risk: Minimal · up to 2df4e

The change adds browser masking coverage without reported production-code changes. No actionable merge-blocking risk is established; merge after normal checks pass.

Pre-merge checks: 5

Check name	Status	Explanation
Title check		The title clearly summarizes the two main additions: real-browser attribute/value smoke tests and document.open masking cases.
Description check		The description directly explains the added tests, covered masking scenarios, validation results, and the decision not to add WebKit coverage.
Linked Issues check		Issue #120 requires a committed real-browser Chromium test for the built replay bundle. The added test file builds the scenario with esbuild, launches headless Chromium, and runs through Vitest. It co…
Out of Scope Changes check		The whole-PR change is limited to packages/instrumentation-tests/test/replay-masking.browser.e2e.ts. The added server, browser harness, scenarios, and assertions directly implement the real-browser …
Docstring Coverage		No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…

Full details: Linked Issues check

Explanation

Issue #120 requires a committed real-browser Chromium test for the built replay bundle. The added test file builds the scenario with esbuild, launches headless Chromium, and runs through Vitest. It covers raw and provider modes, blocked and unblocked iframe behavior including document.open() and edits, shadow DOM, stop/start and blackout behavior, attribute and value masking, and masking-off controls. The tests assert that controls are recorded, secrets are absent under masking, and password/card values remain absent in the masking-off control. The package and root scripts provide an e2e test entry point. WebKit is optional in issue #120 and is not required by the linked issue.

Full details: Out of Scope Changes check

Explanation

The whole-PR change is limited to packages/instrumentation-tests/test/replay-masking.browser.e2e.ts. The added server, browser harness, scenarios, and assertions directly implement the real-browser masking coverage requested by issue #120. No unrelated product code or unrelated test area is shown.

Full details: Docstring Coverage

Explanation

No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 1 files.

Finishing Touches

📝 Generate docstrings

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[claude]

Claude finished @krassx's task in 39s —— View job

---

Review of PR #161: replay masking smoke and document.open tests

• Read the diff and the surrounding harness (replay-masking-scenario.ts, helpers, masking option resolution)
• Checked fixture marker collisions, option names (maskAllText, maskAllInputs, blockAllMedia) and the replaceAll lib target (ES2023, fine)
• Posted findings

I did not run the suite. pnpm and the Playwright Chromium browser are not available in this environment. CI was still pending when I checked. The review below is from reading the code only.

Findings

1. P3 (inline, line 821): The tag AFTEROPEN is a prefix of AFTEROPENB, so the positive markers in the document.open control can be satisfied by the blocked frame's leak. The line 838 negative assertion would still fail in that case, so this only weakens the positive control. The fix is non-prefix tags such as AFTEROPEN_A and AFTEROPEN_B.

I checked and found no problem with:

• ATTRSMOKE_* markers: none is a substring of another (TITLE/IMGTITLE/DIVTITLE, DATA/DATAUSER and TYPED/PWTYPED/CCTYPED all differ in prefix).
• Options: blockAllMedia, maskAllText and maskAllInputs all exist in masking.ts.
• Timing: the fixed settle() delays match the existing file's style, and each negative assertion has a positive control. I can't judge flakiness without running it.

Summary

1. Overall risk: Low. This is a one-file, test-only change.
2. Merge recommendation: Approve once CI is green, ideally after the P3 tag fix.
3. Most important to fix: the P3 prefix collision above. Please also confirm the new controls pass on the CI Chromium run, since I could not.
4. Positives:
   • Each leak assertion has a masking-off control, so the tests can fail.
   • The always-masked password and card floor is asserted absent even with masking off.
   • Both the snapshot and mutation paths are covered, in both raw and provider modes.
   • The blockAllMedia: false variant makes attribute masking itself get exercised, rather than being hidden by image blocking.
   • The document.open blocked-frame control proves the rewrite is detectable while the blocked frame still contributes nothing.
     · branch test/issue-120-masking-smoke

[claude]

Claude finished @krassx's task in 24s —— View job

---

Re-review of PR #161 after the latest push

• Read the existing review comments and the PR conversation
• Re-read the new document.open and attribute-smoke tests against pokeFrame, rewriteFrame and mountFrames
• Checked CI status

I did not run the suite. CI was still in progress when I checked (CI, Claude PR Review and the PR #161 run were all pending), and Playwright Chromium is not available here. This review is from reading the code only.

New findings

None. I tried to find a new problem in the following places and could not construct a credible failure.

• Marker attribution in the blocked-frame control: REWRITTEN.replaceAll('REWRITTEN', 'REWRITTEN_BLOCKED') produces TYPED_SECRET_REWRITTEN_BLOCKED. The unblocked variants do not contain REWRITTEN_BLOCKED, so the not.toContain('REWRITTEN_BLOCKED') assertion cannot be satisfied or tripped by the other frame.
• pokeFrame and the REWRITTEN document: the rewritten document defines #ft and #fi, so all four poke effects (edit, typed value, placeholder, added node) apply after the rewrite. The positive markers are therefore produced as the test expects.
• Attribute smoke fixtures: the PRE and ADDED sections are inserted through the same helper, so both phases differ only in timing relative to start(). Each negative assertion has a matching masking-off control.

Previously reported, still open

The P3 prefix collision from my earlier inline comment is not fixed in the latest commits. AFTEROPEN is still a prefix of AFTEROPENB, in pokeFrame('a', 'AFTEROPEN') and pokeFrame('b', 'AFTEROPENB'). As a result, toContain('IFB_SECRET_EDIT_AFTEROPEN') and the other positive markers in the document.open control can still be satisfied by the blocked frame's leak. The not.toContain('AFTEROPENB') assertion would still catch that leak, so this only weakens the positive control. I am not repeating the inline comment. The fix is non-prefix tags such as AFTEROPEN_A and AFTEROPEN_B.

Summary

1. Overall risk: Low. This is a one-file, test-only change.
2. Merge recommendation: Approve once CI is green. Applying the tag rename first would be good, but it is not blocking.
3. Most important to fix: the P3 AFTEROPEN/AFTEROPENB prefix collision. Please also confirm the new controls pass on the CI Chromium run, since I could not run them.
4. Positives:
   • Every leak assertion has a masking-off control, so the tests can fail.
   • The password and card floor is asserted absent even with masking off.
   • Both the snapshot and mutation paths are covered, in raw and provider modes.
   • The blockAllMedia: false variant exercises attribute masking directly.
   • The blocked-frame document.open control uses distinct per-frame markers, so leaks can be attributed to a frame.
     · branch test/issue-120-masking-smoke

[claude]

Claude finished @krassx's task in 24s —— View job

---

Re-review of PR #161 after the latest push

I did not run the suite. CI was still pending or in progress when I checked, and Playwright Chromium is not available here. This review is from reading the code only.

What changed since my last review

Two small commits, b85e8698 and e5b4ef5f, add anchor assertions to the attribute smoke tests:

• Default-options smoke: it now also asserts "type":"password" and "class":"cc" appear in the events. This shows the password and card fields are in the snapshot, so their absence of secrets reflects masking and not the recorder missing them.
• blockAllMedia: false variant: it now asserts smoke_PRE and the base64 prefix R0lGODlhAQABAAAAACw appear. This shows the image was serialized and not replaced by a blocked placeholder.

New findings

None. I tried to break the new anchors and could not build a credible failure from the code.

• Anchor strings: the only source of "type":"password" and "class":"cc" is the insertAttrFixture markup. They cannot be satisfied by unrelated page content.
• Comment accuracy: the comment in b85e8698 mentioned password and card but only asserted password. e5b4ef5f fixed that.
• PWTYPED/CCTYPED checks: the comment says they have no failing control by design, because they are in the FLOOR set. I agree. This limit is documented, not hidden.

Previously reported, still open

The P3 prefix collision is not fixed. In replay-masking.browser.e2e.ts, AFTEROPEN (line 827) is still a prefix of AFTEROPENB (line 828). The positive markers at lines 836–839, such as IFB_SECRET_EDIT_AFTEROPEN, can still be satisfied by the blocked frame's leak. The not.toContain('AFTEROPENB') check at line 845 would still catch that leak, so only the positive control is weakened. I am not repeating the inline comment. The fix is non-prefix tags such as AFTEROPEN_A and AFTEROPEN_B, with the marker list and line 845 updated to match.

Summary

1. Overall risk: Low. This is a one-file, test-only change.
2. Merge recommendation: Approve once CI is green. The tag rename is worth doing but not blocking.
3. Most important to fix: the P3 AFTEROPEN/AFTEROPENB prefix collision. Please also confirm the new anchors and controls pass on the CI Chromium run, since I could not run them. The base64 anchor depends on how rrweb serializes the data: URI, so it is the one most likely to need adjusting.
4. Positives:
   • The new anchors make the "no leak" assertions mean something, because they prove the recorder saw the sensitive nodes.
   • Every leak assertion has a masking-off control, so the tests can fail.
   • The password and card floor is asserted absent even with masking off.
   • The snapshot and mutation paths are covered in both raw and provider modes.
   • The document.open blocked-frame control uses distinct per-frame markers, so a leak can be attributed to a frame.
     · branch test/issue-120-masking-smoke

[claude]

Claude finished @krassx's task in 16s —— View job

---

Re-review of PR #161 after the latest push

I did not run the suite. CI was still in progress when I checked, and Playwright Chromium is not available here. This review is from reading the code only.

What changed since my last review

One commit, 2df4e852, renames the document.open control tags to AFTEROPEN_UNBLOCKED and AFTEROPEN_BLOCKED. It updates the positive marker list and the negative assertion to match.

Previously reported finding: fixed

The P3 AFTEROPEN/AFTEROPENB prefix collision is resolved. I checked the new tags against pokeFrame (line 196). It builds IFB_SECRET_EDIT_${t}, TYPED_SECRET_${t}, IFB_SECRET_PH_${t} and IFB_SECRET_ADDED_${t}.

• AFTEROPEN_UNBLOCKED does not contain AFTEROPEN_BLOCKED, because _UN sits between the two words.
• REWRITTEN_UNBLOCKED does not contain REWRITTEN_BLOCKED, for the same reason.

The positive markers for the unblocked frame therefore cannot be satisfied by the blocked frame's output. The not.toContain assertions for the blocked frame also cannot be tripped by the unblocked frame.

New findings

None. I tried to build a failure from the rename and could not.

Summary

1. Overall risk: Low. This is a one-file, test-only change.
2. Merge recommendation: Approve once CI is green. The "PR test(replay): real-browser attribute/value smoke and document.open masking cases #161" run has passed. The "CI" run, which includes the Chromium e2e, was still in progress when I checked, so please confirm it passes before merging. The base64 anchor from e5b4ef5f (R0lGODlhAQABAAAAACw) depends on how rrweb serializes the data: URI, so it is the assertion most likely to need adjusting.
3. Most important to fix: nothing outstanding.
4. Positives:
   • Every leak assertion has a masking-off control, so the tests can fail.
   • The password and card floor is asserted absent even with masking off.
   • Both the snapshot and mutation paths are covered, in raw and provider modes.
   • The blockAllMedia: false variant exercises attribute masking directly.
   • The anchors prove the recorder saw the sensitive nodes, so the "no leak" assertions mean something.
   • The document.open control now uses non-overlapping per-frame markers, so a leak can be attributed to one frame.
     · branch test/issue-120-masking-smoke