Conversation
billyvg
left a comment
There was a problem hiding this comment.
Seems like a good idea, nice!
There was a problem hiding this comment.
Pull request overview
This PR updates rrweb’s inline style attribute diffing to avoid triggering CSP style-src-attr violations by parsing prior inline style text via CSSStyleSheet.replaceSync() (when supported) instead of setting a detached element’s style attribute.
Changes:
- Introduce
StyleDeclarationParserto parse inline declaration lists via constructable stylesheets with a detached-element fallback. - Update mutation style-diff logic to use the new parser rather than creating a detached element per mutation.
- Add unit and integration tests covering quoted/semicolon-containing values and the constructable-stylesheet path.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/rrweb/src/record/style-declaration-parser.ts | New parser abstraction using replaceSync() to avoid CSP issues. |
| packages/rrweb/src/record/mutation.ts | Switch style-diff old-style parsing to StyleDeclarationParser. |
| packages/rrweb/test/style-declaration-parser.test.ts | Add unit tests for fallback vs constructable stylesheet parsing. |
| packages/rrweb/test/record.test.ts | Add regression test for style mutations with separators inside quoted strings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return ( | ||
| this.parseWithConstructableStylesheet(styleText) || | ||
| this.parseWithDetachedElement(styleText) | ||
| ); |
There was a problem hiding this comment.
parse() always falls back to parseWithDetachedElement() when constructable stylesheet parsing returns null (including when replaceSync() throws on invalid CSS). In strict CSP environments this can reintroduce the very style-src-attr violation this change is trying to avoid. Consider not using the detached-element fallback when replaceSync is available but parsing fails; instead return null/an empty declaration list and let the caller treat it as “unknown old style” (which is safer under CSP).
| return ( | |
| this.parseWithConstructableStylesheet(styleText) || | |
| this.parseWithDetachedElement(styleText) | |
| ); | |
| if ( | |
| typeof CSSStyleSheet === 'undefined' || | |
| typeof CSSStyleSheet.prototype.replaceSync !== 'function' | |
| ) { | |
| return this.parseWithDetachedElement(styleText); | |
| } | |
| return this.parseWithConstructableStylesheet(styleText); |
| try { | ||
| const sheet = new CSSStyleSheet(); | ||
| sheet.replaceSync(`x { ${styleText} }`); | ||
|
|
There was a problem hiding this comment.
parseWithConstructableStylesheet() allocates a new CSSStyleSheet on every parse. Style mutations can be frequent, so this can add avoidable GC/CPU overhead. Consider caching a single CSSStyleSheet instance on the parser when replaceSync is available and reusing it for subsequent replaceSync() calls.
| public parse(styleText: string): CSSStyleDeclaration | null { | ||
| return ( | ||
| this.parseWithConstructableStylesheet(styleText) || | ||
| this.parseWithDetachedElement(styleText) | ||
| ); | ||
| } |
There was a problem hiding this comment.
The parse() return type is CSSStyleDeclaration | null, but with the current implementation it will always return a CSSStyleDeclaration (because parseWithDetachedElement() always returns one and isn’t guarded). Either make parse() non-nullable to match behavior, or add error handling so parseWithDetachedElement() can fail and return null (which also makes it easier to avoid CSP-unsafe fallbacks when replaceSync is available).
Reuse a single constructable stylesheet across parse() calls instead of allocating a new one per style mutation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…nParser" This reverts commit 60ae612.
|
AI found an optimization with style allocation but decided to scrap it because it can cause bugs in the future where cached references values change by parsing new values. |
Bumps all rrweb dependencies in tests and internal packages to the latest versions. The changelog for these new releases is: - [(rrweb-snapshot) Rewrite vulnerable regexes](getsentry/rrweb#289) - [Use CSS Declaration replaceSync to parse styles to avoid CSP violations](getsentry/rrweb#286) - [Wrap iframe contentWindow access in try-catch](getsentry/rrweb#275) Full changelog can be [found here](https://github.com/getsentry/rrweb/releases/tag/2.42.0).
Partially addresses getsentry/sentry-javascript#10481 at least from rrweb side.
The problem is that rrweb computes
style-attribute diffs by reparsing the previous inline style string through a detached DOM element. The old code did:That works as a parser, but in browsers with strict CSP it can trigger a
style-src-attrviolation, because it gets treated as applying an inlinestyleattribute even though the element is never attached to the page. This means rrweb can hit CSP errors while trying to diff inline style mutations.The idea of the fix is to keep using the browser’s CSS parser, but avoid parsing through a DOM element. In browsers that support constructable stylesheets, we wrap the inline declaration list in a harmless CSS rule and parse it with
CSSStyleSheet.replaceSync():replaceSync()expects full stylesheet text, sox { ... }is just the smallest valid rule wrapper around the inline declaration block. The selector itself does not matter because we never use it for matching; we only read backcssRules[0].style, which gives us the parsedCSSStyleDeclaration.This works because rrweb only needs to compare explicit inline declarations, not computed or cascaded styles. Parsing
style="color:red; left:10px"through a detached element and parsingx { color:red; left:10px }through the stylesheet parser both yield the same declaration list, including priorities like!important.For older supported browsers that do not implement constructable stylesheets, we keep the existing detached-element fallback so behavior remains unchanged.
I verified this in a test app that has CSP headers and triggered this error on current builds, it doesn't after this change.