Regression: support for strict CSP broken by style mutation #816
Comments
|
@eoghanmurray The tech for this project is a bit beyond my area of knowledge, but since you are the original owner of the PR, I was wondering if you have any thoughts on workarounds. Would it be possible to implement this using a data attribute like |
|
@razor-x would you mind if we stripped all the CSP policies on replay? I think that would solve this issue for you. |
I'm not sure I understand this proposal. Can you point at an example? To further clarify, this is an issue when the DOM is being recorded not replayed. As I understand things, as long as there is JS code that tries to write |
|
This is what I am thinking might work, just read and write to a different attribute. Not sure what other spots if any would need an update, I tired to use the other PR to see what might need to change: rxfork@1d6d8a1 (I'd take this further, but I tried for a bit to get the tests passing on my local but hit various errors and had to give up.)
|
|
Thanks for alerting me on this!
I wonder do we need to go so far as creating an iframe object (could that have a different CSP policy) and executing |
|
@razor-x how does the document itself modify the style in the first place? (the original modification that generated the mutation) (via dev tools > inspector > (the element) > break on attribute modifications) |
Sure. Just some more context on how I discovered the issue. I recently updated our app to MUI v5 which is supposed to have strict CSP compatibility now (since they switched to emotion / styled-components and no longer use CSS in JS). I enabled the strict CSP headers and tested the site in staging where we are NOT running rrweb and saw no errors. When moved to prod where recording is enabled, I saw the CSP errors triggered by the referenced mutation line from rrweb. I actually would expect the app's attribute updates to trigger CSP but there must be a reason they are allowed. Here is one of the things triggering a modification that gets picked up by the rrweb mutation.
|
|
I loaded it up in dev mode and grabbed the non-minified react code that it breaks on.
|
Ok so maybe it's okay to access the style property but not set the attribute? (I'm ignoring the second screenshot as the attribute being set there is In terms of speculative investigation, could you check whether something like the following is permissable in your CSP context (you can paste it in in the console and see what errors arise): |
|
As a sanity check I deployed the restrictive CSP back to my testing environment and confirmed the (non-rrweb) updates do not trigger violations in chrome or firefox. I also ran your scripts against that.
Ah good catch, I played to the other breaks, hopefully this is move useful. I think the first one it the style that makes the box appear and the second is the box being hidden.
There maybe an issue with these, I got non-CSP errors in all cases. chrome
firefox
Then as a control I tried this on https://example.com/
|
|
I modified the script and tried again example.com
With strict CSP
|
|
I did some digging and I suspect the key may be that style updates using the CSSOM by an already-allowed CSP script are allowed without Here's what's I found that seems to suggest this: |
|
right, Could you try simpler: I imagine the CSP is inherited by the document, but maybe not given the new document isn't actually attached to anything? |
|
(I'd use the CSSOM, but the old style attribute is provided as a string, and we'd have to loop through it to parse it, which I'm not confident we'd get right for whatever edge cases - setting the attribute lets the css engine do this work!) |
This might actually work since as you said, the document is not actually rendering into the page! (I did both together to show the no-error / error cases in one place).
---- quick edit, showing the attribute update did get saved
|
eoghanmurray
added a commit
to statcounter/rrweb
that referenced
this issue
Feb 26, 2022
…y) error with `.setAttribute('style')`
|
I coded this up in #846 but haven't tested it or anything! |
|
Nice, I hope it's this simple. What's the best way for us to test this? I was never able to get the project dev env running on my local. I think the CSP will be fine now, so we just need to check the recoding / replay right? |
|
Would you be able to have another go at setting up a dev environment and let the core team know if there were any instructions that weren't clear or were misleading? |
eoghanmurray
added a commit
to statcounter/rrweb
that referenced
this issue
Mar 2, 2022
…y) error with `.setAttribute('style')`
I totally understand this. So my first check before hacking on most projects is to make sure all the tests pass before I change anything. Otherwise I can't really know if my changes broke or fixed anything useful. It seems like, looking at the docs and the travis config that I should be able to run on Node 12 (IMO the node version to develop on should be in CLI log |
|
I imagine the testing suite interacts with puppeteer over websockets or something. Can you check the version of puppeteer you have installed? @Juice10 any ideas? |
|
@eoghanmurray We are using quite an old version of puppeteer. Might be worth upgrading it just to see if this disappears. |
eoghanmurray
added a commit
to statcounter/rrweb
that referenced
this issue
Mar 3, 2022
…y) error with `.setAttribute('style')`
|
Here is some more feedback after going a little further. I tried to run
Errors on yarn test for packages/rrweb |
|
This looks a bit like you haven't run |
Test errors |
|
Seems like this stalled out a bit on the testing front. Sorry I wasn't able to get this running locally but the fix seems so simple. Any way we can quickly validate it works? |
|
I'm agnostic; just not sure if #846 actually fixes it ... presume it does. |
AntonioStipic
added a commit
to Analyzee/rrweb
that referenced
this issue
Jan 5, 2023
…y) error with setAttribute('style')
eoghanmurray
added a commit
to eoghanmurray/rrweb
that referenced
this issue
Mar 27, 2023
…y) error with `.setAttribute('style')`
eoghanmurray
added a commit
to eoghanmurray/rrweb
that referenced
this issue
Apr 3, 2023
…y) error with `.setAttribute('style')`
eoghanmurray
added a commit
to eoghanmurray/rrweb
that referenced
this issue
Apr 3, 2023
…y) error with `.setAttribute('style')`
eoghanmurray
added a commit
to eoghanmurray/rrweb
that referenced
this issue
Apr 12, 2023
…y) error with `.setAttribute('style')`
|
Our test suite was able to validate that it didn't work — I've added some commits today to make it actually work so #846 might be able to land in trunk soon. |
eoghanmurray
added a commit
to statcounter/rrweb
that referenced
this issue
Apr 13, 2023
…y) error with `.setAttribute('style')`
eoghanmurray
added a commit
to statcounter/rrweb
that referenced
this issue
Apr 13, 2023
…y) error with `.setAttribute('style')`
YunFeng0817
pushed a commit
that referenced
this issue
May 22, 2023
* Fix for #816 - avoid triggering a CSP (content security policy) error with `.setAttribute('style')` * The bare unattachedDoc that I previously naively created didn't have a doctype and wasn't a HTML document, so the child style element didn't have the `old.style` attribute available * Add a try/catch to provide some robustness in case `document.implementation.createHTMLDocument` isn't available
eoghanmurray
added a commit
to eoghanmurray/rrweb
that referenced
this issue
Jul 27, 2023
* Fix for rrweb-io#816 - avoid triggering a CSP (content security policy) error with `.setAttribute('style')` * The bare unattachedDoc that I previously naively created didn't have a doctype and wasn't a HTML document, so the child style element didn't have the `old.style` attribute available * Add a try/catch to provide some robustness in case `document.implementation.createHTMLDocument` isn't available
eoghanmurray
added a commit
to eoghanmurray/rrweb
that referenced
this issue
Jul 27, 2023
* Fix for rrweb-io#816 - avoid triggering a CSP (content security policy) error with `.setAttribute('style')` * The bare unattachedDoc that I previously naively created didn't have a doctype and wasn't a HTML document, so the child style element didn't have the `old.style` attribute available * Add a try/catch to provide some robustness in case `document.implementation.createHTMLDocument` isn't available
eoghanmurray
added a commit
to eoghanmurray/rrweb
that referenced
this issue
Jul 27, 2023
* Fix for rrweb-io#816 - avoid triggering a CSP (content security policy) error with `.setAttribute('style')` * The bare unattachedDoc that I previously naively created didn't have a doctype and wasn't a HTML document, so the child style element didn't have the `old.style` attribute available * Add a try/catch to provide some robustness in case `document.implementation.createHTMLDocument` isn't available
eoghanmurray
added a commit
to eoghanmurray/rrweb
that referenced
this issue
Aug 3, 2023
* Fix for rrweb-io#816 - avoid triggering a CSP (content security policy) error with `.setAttribute('style')` * The bare unattachedDoc that I previously naively created didn't have a doctype and wasn't a HTML document, so the child style element didn't have the `old.style` attribute available * Add a try/catch to provide some robustness in case `document.implementation.createHTMLDocument` isn't available
eoghanmurray
added a commit
to eoghanmurray/rrweb
that referenced
this issue
Aug 8, 2023
* Fix for rrweb-io#816 - avoid triggering a CSP (content security policy) error with `.setAttribute('style')` * The bare unattachedDoc that I previously naively created didn't have a doctype and wasn't a HTML document, so the child style element didn't have the `old.style` attribute available * Add a try/catch to provide some robustness in case `document.implementation.createHTMLDocument` isn't available
eoghanmurray
added a commit
to eoghanmurray/rrweb
that referenced
this issue
Aug 8, 2023
* Fix for rrweb-io#816 - avoid triggering a CSP (content security policy) error with `.setAttribute('style')` * The bare unattachedDoc that I previously naively created didn't have a doctype and wasn't a HTML document, so the child style element didn't have the `old.style` attribute available * Add a try/catch to provide some robustness in case `document.implementation.createHTMLDocument` isn't available
|
I don't have the whole context but from what I understand you could modify an element with In the meantime, my workaround is to use |
|
What is the status of this issue? I am working on an issue where rrweb styles are being dropped in a Svelte app and I'm wondering if it could be related to this. Happy to test / patch anything in production that might help |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The optimization in PR #464 broke support for users who run a strict Content-Security-Policy (CSP).
Specifically, this line sets the
styleattribute on a DOM node and will be blocked withoutstyle-src: 'unsafe-inline'(which is the unsafe CSP).rrweb/packages/rrweb/src/record/mutation.ts
Line 487 in 661c746
For any apps that pull in the affected versions and have a strict CSP, this issue will generate a very large numbers of errors like the one below (one for each time that line of code runs). As a possible side effect, any error reporting services or report-url endpoints may be quickly overwhelmed with error reports.
The text was updated successfully, but these errors were encountered: