fix(replay): stop SCRIPT_PLACEHOLDER rendering in shadow roots - #4703
Draft
posthog[bot] wants to merge 2 commits into
Draft
fix(replay): stop SCRIPT_PLACEHOLDER rendering in shadow roots#4703posthog[bot] wants to merge 2 commits into
posthog[bot] wants to merge 2 commits into
Conversation
A recorded `<script>` is serialized with its text replaced by the `SCRIPT_PLACEHOLDER` sentinel and rebuilt as `<noscript>` so it cannot execute. Scripting is off in the replay iframe, so `<noscript>` content renders as visible text, and the `noscript { display: none }` replay style is document-scoped — it never reaches shadow roots. So the placeholder leaked over any third-party widget recorded in a shadow root.
Rebuild now emits an empty text node for the placeholder, so it cannot render in any context, on both the full-snapshot and incremental paths. The sentinel is shared between snapshot and rebuild through one constant. The document-scoped style is kept, because it still hides genuine `<noscript>` fallback content during replay.
Generated-By: PostHog Desktop
Task-Id: af10c549-b6bb-48b0-a1ee-8d0171bf5ca5
Contributor
Author
🦔 PostHog Review reviewed this pull requestFound 1 must fix, 1 should fix, 1 consider. Published 3 findings (view the review). Resolved comments: 1 fixed, 2 left for you |
Contributor
Replay incident risk checkThis diff touches code involved in past incidents. This is a heads-up, not a verdict: read the matched sections of INCIDENTS.md and answer their review questions before merging. For a judgment on whether this diff has the same failure mode, run the |
Contributor
Contributor
|
Size Change: +3.95 kB (+0.02%) Total Size: 20.8 MB 📦 View Changed
ℹ️ View Unchanged
|
Contributor
Author
|
PostHog Review alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
Comment on lines
+26
to
+32
| /** | ||
| * Sentinel that replaces the text content of every `<script>` node during | ||
| * serialization (see snapshot.ts). It keeps real script source out of the | ||
| * snapshot and marks the node as a script for rebuild. `rebuild.ts` reads this | ||
| * value back and must not render it, so the two sides share this one constant. | ||
| */ | ||
| export const SCRIPT_PLACEHOLDER = 'SCRIPT_PLACEHOLDER'; |
Contributor
Author
There was a problem hiding this comment.
Redact script character-data mutations
Why we think it's a valid issue
- Checked: the record-side character-data path (
packages/rrweb/rrweb/src/record/mutation.ts), the payload filter that decides which text mutations ship, the replay-side apply loop (packages/rrweb/rrweb/src/replay/index.ts),isIgnoredinpackages/rrweb/rrweb/src/utils.ts, the slimDOM gate inpackages/rrweb/rrweb-snapshot/src/snapshot.ts, and the posthog-js recorder defaults inpackages/browser/src/extensions/replay/external/lazy-loaded-session-recorder.ts. - Found:
packages/rrweb/rrweb/src/record/mutation.ts:666readsdom.textContent(m.target)and pushes it at line 673. The only transform isneedMaskingText. There is no parent-tag check, so noSCRIPT_PLACEHOLDERsubstitution.serializeTextNode(packages/rrweb/rrweb-snapshot/src/snapshot.ts:681) is the only place that applies the sentinel, and the character-data path never calls it. - Found: the mutation survives the payload filters at
packages/rrweb/rrweb/src/record/mutation.ts:549-551. Those filters drop ids not present in the mirror. A script text node holds a real id, becauseserializeTextNodegives it the non-whitespace valueSCRIPT_PLACEHOLDER, so the whitespace-only ignore rule atpackages/rrweb/rrweb-snapshot/src/snapshot.ts:1329-1333does not apply. Recorded event fixtures confirm the id, for examplepackages/rrweb/rrweb/test/events/webgl.ts:84. - Found:
isIgnored(packages/rrweb/rrweb/src/utils.ts:300-314) only rejectsIGNORED_NODEand slim-DOM title mutations. It does not reject script text nodes. - Found: replay assigns the value with no sentinel check at
packages/rrweb/rrweb/src/replay/index.ts:2018(target.textContent = mutation.value;). The target is the text child of the<noscript>thattagMapproduced atpackages/rrweb/rrweb-snapshot/src/rebuild.ts:21. The new empty-text-node guard atpackages/rrweb/rrweb-snapshot/src/rebuild.ts:133runs only on the build path, so it cannot undo this later write. - Found: the path is on by default. posthog-js sets
slimDOMOptions: {}atpackages/browser/src/extensions/replay/external/lazy-loaded-session-recorder.ts:2598, soslimDOMOptions.scriptis falsy and<script>elements are recorded, not excluded. - Impact: confirmed. A direct write to the script's Text node —
script.firstChild.data = ...,.nodeValue = ...,appendData, orreplaceData— stores the raw script text in the recording and renders it inside the rebuilt<noscript>on replay. In a shadow root the document-scopednoscript { display: none }rule does not reach it, so the text becomes visible. This is the same leak class the PR closes for the sentinel. - Impact: the reachable set is narrower than the finding implies.
textContent =,.text =,innerHTML =, andappendChildall produce childList mutations, which re-serialize throughserializeTextNodeand stay redacted. Only in-place Text-node writes reach the gap, and the common instance of that pattern is a JSON or JSON-LD data script, whose content is public. - Priority: lowered to
should_fix. The gap is real and verified, but it predates this PR, sits in two files the PR does not touch, and the PR neither creates nor widens it — before the change the same write overwrote the sentinel text node in exactly the same way. It should not block a narrow, correct fix.
Issue description
The new contract says every script body uses the sentinel. Character-data mutations do not call serializeTextNode. The recorder stores their raw value, and replay assigns it directly. A changed script text node can therefore store and render its source inside the rebuilt <noscript>. This bypasses the privacy boundary for incremental updates.
Suggested fix
Store the sentinel when a character-data mutation belongs to a script. Also ignore script text mutations during replay for older recordings. Add a test that changes script.firstChild.data after a full snapshot.
Prompt to fix with AI (copy-paste)
## Context
@packages/rrweb/rrweb-snapshot/src/utils.ts#L26-32
<issue_description>
The new contract says every script body uses the sentinel. Character-data mutations do not call `serializeTextNode`. The recorder stores their raw value, and replay assigns it directly. A changed script text node can therefore store and render its source inside the rebuilt `<noscript>`. This bypasses the privacy boundary for incremental updates.
</issue_description>
<issue_validation>
- **Checked:** the record-side character-data path (`packages/rrweb/rrweb/src/record/mutation.ts`), the payload filter that decides which text mutations ship, the replay-side apply loop (`packages/rrweb/rrweb/src/replay/index.ts`), `isIgnored` in `packages/rrweb/rrweb/src/utils.ts`, the slimDOM gate in `packages/rrweb/rrweb-snapshot/src/snapshot.ts`, and the posthog-js recorder defaults in `packages/browser/src/extensions/replay/external/lazy-loaded-session-recorder.ts`.
- **Found:** `packages/rrweb/rrweb/src/record/mutation.ts:666` reads `dom.textContent(m.target)` and pushes it at line 673. The only transform is `needMaskingText`. There is no parent-tag check, so no `SCRIPT_PLACEHOLDER` substitution. `serializeTextNode` (`packages/rrweb/rrweb-snapshot/src/snapshot.ts:681`) is the only place that applies the sentinel, and the character-data path never calls it.
- **Found:** the mutation survives the payload filters at `packages/rrweb/rrweb/src/record/mutation.ts:549-551`. Those filters drop ids not present in the mirror. A script text node holds a real id, because `serializeTextNode` gives it the non-whitespace value `SCRIPT_PLACEHOLDER`, so the whitespace-only ignore rule at `packages/rrweb/rrweb-snapshot/src/snapshot.ts:1329-1333` does not apply. Recorded event fixtures confirm the id, for example `packages/rrweb/rrweb/test/events/webgl.ts:84`.
- **Found:** `isIgnored` (`packages/rrweb/rrweb/src/utils.ts:300-314`) only rejects `IGNORED_NODE` and slim-DOM title mutations. It does not reject script text nodes.
- **Found:** replay assigns the value with no sentinel check at `packages/rrweb/rrweb/src/replay/index.ts:2018` (`target.textContent = mutation.value;`). The target is the text child of the `<noscript>` that `tagMap` produced at `packages/rrweb/rrweb-snapshot/src/rebuild.ts:21`. The new empty-text-node guard at `packages/rrweb/rrweb-snapshot/src/rebuild.ts:133` runs only on the build path, so it cannot undo this later write.
- **Found:** the path is on by default. posthog-js sets `slimDOMOptions: {}` at `packages/browser/src/extensions/replay/external/lazy-loaded-session-recorder.ts:2598`, so `slimDOMOptions.script` is falsy and `<script>` elements are recorded, not excluded.
- **Impact:** confirmed. A direct write to the script's Text node — `script.firstChild.data = ...`, `.nodeValue = ...`, `appendData`, or `replaceData` — stores the raw script text in the recording and renders it inside the rebuilt `<noscript>` on replay. In a shadow root the document-scoped `noscript { display: none }` rule does not reach it, so the text becomes visible. This is the same leak class the PR closes for the sentinel.
- **Impact:** the reachable set is narrower than the finding implies. `textContent =`, `.text =`, `innerHTML =`, and `appendChild` all produce childList mutations, which re-serialize through `serializeTextNode` and stay redacted. Only in-place Text-node writes reach the gap, and the common instance of that pattern is a JSON or JSON-LD data script, whose content is public.
- **Priority:** lowered to `should_fix`. The gap is real and verified, but it predates this PR, sits in two files the PR does not touch, and the PR neither creates nor widens it — before the change the same write overwrote the sentinel text node in exactly the same way. It should not block a narrow, correct fix.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Store the sentinel when a character-data mutation belongs to a script. Also ignore script text mutations during replay for older recordings. Add a test that changes `script.firstChild.data` after a full snapshot.
</potential_solution>
Contributor
Author
There was a problem hiding this comment.
Confirmed this is a real, pre-existing gap, but it is out of scope for this PR, so I'm escalating rather than fixing it here.
What I verified at the current head:
- Record side (
record/mutation.ts, thecharacterDatabranch): a text mutation on a<script>'s text child is captured with only text-masking applied — there is no parent-tag check and noSCRIPT_PLACEHOLDERsubstitution. The sentinel is only ever applied inserializeTextNode(snapshot.ts), which the character-data path never calls. - Replay side (
replay/index.ts): the text mutation's value is written straight onto the target with no sentinel/redaction guard. - This PR's new empty-text-node guard (
rebuild.ts) only runs on the build path, so it can't intercept a later character-data write.
So an in-place write to a script's text node (.data =, .nodeValue =, appendData, replaceData) can store raw script source and render it inside the rebuilt <noscript> — visible in a shadow root, the same leak class this PR closes for the sentinel.
Why I'm not fixing it in this PR: it lives entirely in record/mutation.ts and replay/index.ts, two files this PR doesn't touch, and (as the finding itself notes) this PR neither creates nor widens it — before the change the same write overwrote the sentinel text node in exactly the same way. It's a separate, pre-existing privacy-redaction boundary, and a correct fix is a design decision (redact script-parented character-data mutations on the record side by substituting the sentinel keyed on the target's parent tag, with new recording tests, and/or add a replay-side guard).
What a human needs to decide: whether to take this on as its own follow-up PR against the record/replay paths, and whether the reachable set (in practice, in-place writes to public JSON/JSON-LD data scripts) justifies the record-side change now. It should not block this narrow rebuild-path fix from merging.
| } | ||
| if (isScript) { | ||
| text = 'SCRIPT_PLACEHOLDER'; | ||
| text = SCRIPT_PLACEHOLDER; |
Contributor
Author
There was a problem hiding this comment.
Redact lowercase script elements
Why we think it's a valid issue
- Checked:
serializeTextNodeand itsisScriptguard,getValidTagName,isSVGElement,slimDOMExcluded, the rebuild path for SVG-namespaced nodes inpackages/rrweb/rrweb-snapshot/src/rebuild.ts, the posthog-js recorder defaults inpackages/browser/src/extensions/replay/external/lazy-loaded-session-recorder.ts, andgit diff main..HEADto see what the PR actually changed here. - Found: the premise holds, and the repo itself relies on it.
isSVGElementatpackages/rrweb/rrweb-snapshot/src/snapshot.ts:167-169testsel.tagName === 'svg'in lowercase. That check only works because SVG-namespace elements report a lowercasetagName. The same rule makes an SVG<script>reportscript, sopackages/rrweb/rrweb-snapshot/src/snapshot.ts:646(parentTagName === 'SCRIPT') does not match it and the sentinel assignment at line 682 never runs. - Found: the codebase is inconsistent on this.
getValidTagNamenormalizes withtoLowerCaseatpackages/rrweb/rrweb-snapshot/src/snapshot.ts:63, andslimDOMExcludedcompares the normalized name against lowercase'script'atpackages/rrweb/rrweb-snapshot/src/snapshot.ts:1100. Only the redaction check compares the raw DOMtagNameagainst an uppercase literal. - Found: nothing else stops the text node.
serializeNodeWithIdrecurses into SVG children, and the slimDOM script exclusion is off by default — posthog-js setsslimDOMOptions: {}atpackages/browser/src/extensions/replay/external/lazy-loaded-session-recorder.ts:2598. So the raw source of an inline<svg><script>is stored verbatim in the snapshot. - Found: the render half of the claim does not hold for SVG. The element serializes with
isSVG: true(packages/rrweb/rrweb-snapshot/src/snapshot.ts:1072), so rebuild takes thecreateElementNSbranch atpackages/rrweb/rrweb-snapshot/src/rebuild.ts:174and produces an SVG-namespacenoscript. That is an unrecognized SVG element, so neither it nor its children render. The visible-text leak is limited to the XHTML case, which the finding scopes correctly. - Found: the gap predates this PR. The diff on this file is only
text = 'SCRIPT_PLACEHOLDER'→text = SCRIPT_PLACEHOLDER, plus the import. TheisScriptcomparison is untouched. - Impact: confirmed but narrow. An inline
<svg><script>writes its real source into the recording instead of the sentinel, which breaks the redaction the PR just documented. The PR's new comment atpackages/rrweb/rrweb-snapshot/src/utils.ts:26-32asserts the sentinel replaces the text of "every<script>node", and that wording is now demonstrably wrong. The fix is one normalization, and it causes no regression: an SVG script would then rebuild as an empty text node, exactly like an HTML one. - Priority: lowered to
consider. Inline SVG scripts are uncommon on real pages, the leaked content is page code rather than user data, and the SVG case produces no visible render. The XHTML variant is the only path that renders, and XHTML-served documents are effectively extinct, so this does not justify blocking a narrow fix.
Issue description
This assignment runs only when parent.tagName equals uppercase SCRIPT. SVG and XHTML elements expose lowercase script. Their snapshots therefore store the real script source. XHTML script text can also render inside a replayed shadow root.
Suggested fix
Normalize the parent name before the check, such as parentTagName?.toLowerCase() === 'script'. Add browser tests for SVG and XHTML scripts. Assert that snapshot bytes exclude the source and replay shows no script text.
Prompt to fix with AI (copy-paste)
## Context
@packages/rrweb/rrweb-snapshot/src/snapshot.ts#L682
<issue_description>
This assignment runs only when `parent.tagName` equals uppercase `SCRIPT`. SVG and XHTML elements expose lowercase `script`. Their snapshots therefore store the real script source. XHTML script text can also render inside a replayed shadow root.
</issue_description>
<issue_validation>
- **Checked:** `serializeTextNode` and its `isScript` guard, `getValidTagName`, `isSVGElement`, `slimDOMExcluded`, the rebuild path for SVG-namespaced nodes in `packages/rrweb/rrweb-snapshot/src/rebuild.ts`, the posthog-js recorder defaults in `packages/browser/src/extensions/replay/external/lazy-loaded-session-recorder.ts`, and `git diff main..HEAD` to see what the PR actually changed here.
- **Found:** the premise holds, and the repo itself relies on it. `isSVGElement` at `packages/rrweb/rrweb-snapshot/src/snapshot.ts:167-169` tests `el.tagName === 'svg'` in lowercase. That check only works because SVG-namespace elements report a lowercase `tagName`. The same rule makes an SVG `<script>` report `script`, so `packages/rrweb/rrweb-snapshot/src/snapshot.ts:646` (`parentTagName === 'SCRIPT'`) does not match it and the sentinel assignment at line 682 never runs.
- **Found:** the codebase is inconsistent on this. `getValidTagName` normalizes with `toLowerCase` at `packages/rrweb/rrweb-snapshot/src/snapshot.ts:63`, and `slimDOMExcluded` compares the normalized name against lowercase `'script'` at `packages/rrweb/rrweb-snapshot/src/snapshot.ts:1100`. Only the redaction check compares the raw DOM `tagName` against an uppercase literal.
- **Found:** nothing else stops the text node. `serializeNodeWithId` recurses into SVG children, and the slimDOM script exclusion is off by default — posthog-js sets `slimDOMOptions: {}` at `packages/browser/src/extensions/replay/external/lazy-loaded-session-recorder.ts:2598`. So the raw source of an inline `<svg><script>` is stored verbatim in the snapshot.
- **Found:** the render half of the claim does not hold for SVG. The element serializes with `isSVG: true` (`packages/rrweb/rrweb-snapshot/src/snapshot.ts:1072`), so rebuild takes the `createElementNS` branch at `packages/rrweb/rrweb-snapshot/src/rebuild.ts:174` and produces an SVG-namespace `noscript`. That is an unrecognized SVG element, so neither it nor its children render. The visible-text leak is limited to the XHTML case, which the finding scopes correctly.
- **Found:** the gap predates this PR. The diff on this file is only `text = 'SCRIPT_PLACEHOLDER'` → `text = SCRIPT_PLACEHOLDER`, plus the import. The `isScript` comparison is untouched.
- **Impact:** confirmed but narrow. An inline `<svg><script>` writes its real source into the recording instead of the sentinel, which breaks the redaction the PR just documented. The PR's new comment at `packages/rrweb/rrweb-snapshot/src/utils.ts:26-32` asserts the sentinel replaces the text of "every `<script>` node", and that wording is now demonstrably wrong. The fix is one normalization, and it causes no regression: an SVG script would then rebuild as an empty text node, exactly like an HTML one.
- **Priority:** lowered to `consider`. Inline SVG scripts are uncommon on real pages, the leaked content is page code rather than user data, and the SVG case produces no visible render. The XHTML variant is the only path that renders, and XHTML-served documents are effectively extinct, so this does not justify blocking a narrow fix.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Normalize the parent name before the check, such as `parentTagName?.toLowerCase() === 'script'`. Add browser tests for SVG and XHTML scripts. Assert that snapshot bytes exclude the source and replay shows no script text.
</potential_solution>
Contributor
Author
There was a problem hiding this comment.
Confirmed this is a real redaction gap at the current head, but I'm escalating rather than changing it unattended because it's a privacy/redaction boundary and its verification needs the browser suite.
What I verified: in serializeTextNode, the script check compares the raw DOM tagName against the uppercase literal 'SCRIPT'. HTML elements report an uppercased tag name, but SVG-namespace and XHTML <script> elements report lowercase script, so the check misses them and their real source is written into the snapshot instead of the SCRIPT_PLACEHOLDER sentinel. That also makes this PR's new comment ("the sentinel replaces the text of every <script> node") inaccurate for that edge case. The rest of the file already lowercases tag names (e.g. getValidTagName, the slimDOM script exclusion), so only this check is inconsistent.
The suggested one-token normalization (comparing parentTagName?.toLowerCase() === 'script') is contained and, per the finding, causes no regression — an SVG script would then rebuild as an empty text node like an HTML one.
Why a human should take it: (1) this is redaction logic whose job is keeping potentially-sensitive script bodies out of recordings, so a maintainer should sign off on the change rather than have it applied autonomously; (2) the finding's own acceptance test — assert the snapshot bytes exclude the source and replay shows no script text, for both SVG and XHTML — needs the real-Chrome integration suite, which cannot run in this sandbox (only the jsdom serialization half is provable here). While deciding, please also consider whether the sibling isStyle check (same uppercase-only comparison, so SVG <style> is affected too) should get the same normalization.
Impact is narrow, so this shouldn't block the PR: inline SVG scripts are uncommon, the SVG case rebuilds as a non-rendering SVG-namespace element (so no visible leak, only a stored-bytes leak), and XHTML-served documents — the only path that would render — are effectively extinct.
The changeset bumped @posthog/rrweb-snapshot, an internal package that is deliberately excluded from the release publish matrix. posthog-js links the rrweb packages only via devDependencies, so the changesets cascade would not bump or publish posthog-js, and the rebuild fix (bundled into the browser SDK at build time) would never ship. Retarget the changeset at posthog-js, matching the convention every other browser-SDK changeset already follows. Generated-By: PostHog Desktop Task-Id: b70c9594-d617-4924-9264-39b41fe39eaa
Contributor
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
SCRIPT_PLACEHOLDERtext over the real UI. Zero end-user impact on the recorded site itself.<script>is serialized with its text replaced by theSCRIPT_PLACEHOLDERsentinel (snapshot.ts) and rebuilt as<noscript>so it cannot execute (rebuild.ts). Scripting is off in the replay iframe, so<noscript>content renders as visible text.noscript { display: none !important; }style (replay/styles/inject-style.ts). That rule never reaches shadow roots, so the placeholder leaks there.Changes
script→noscriptmapping, so scripts still cannot execute on replay.noscript { display: none }style — it still hides genuine<noscript>fallback content during replay (the user had JS on, so they never saw it).snapshot.ts(write) andrebuild.ts(read) through one exported constant, so the contract is explicit.<noscript>outputs are now empty. Serialization output is unchanged (the snapshot still stores the sentinel).Release info Sub-libraries affected
Libraries affected
Checklist
If releasing new changes
pnpm changesetto generate a changeset file🤖 Agent context
Autonomy: Fully autonomous
rebuildunit test asserting a<script>rebuilds as an empty<noscript>; ran the full@posthog/rrweb-snapshotjsdom suites (rebuild,snapshot,utils,css— all pass), plustsc --noEmitandeslint.integration.test.tssuite launches real Chrome, which cannot start in this sandbox. Its snapshots were updated by mechanical substitution limited to the rebuild-output lines (<noscript>SCRIPT_PLACEHOLDER</noscript>→<noscript></noscript>); the serialization-output snapshots were left unchanged. Please let CI regenerate/verify these.noscript { display: none }style (would expose genuine<noscript>fallback content on replay); rebuilding<script>as a neutralized<script>(loses the execution-safety guarantee of thenoscriptmapping); fixing onlysnapshot.ts(would not repair the millions of existing recordings that already carry the sentinel — the playback fix covers both old and new).Created with PostHog Desktop from this inbox report.