Open razor-x opened 2 years ago
@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 data-style
instead of style
on the app you are recording? I don't think CSP will care about data attributes.
@razor-x would you mind if we stripped all the CSP policies on replay? I think that would solve this issue for you.
@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 style
attributes it will conflict with a strict CSP.
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: https://github.com/rxfork/rrweb/commit/1d6d8a1f2bbbc5c67f4d94a289e9afdbae86f3b4
(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!
The data-style
solution wouldn't work as the style attribute is special and has an API associated with it (old.style.getPropertyValue).
The this.doc.createElement('span')
bit is just to create a dummy element so we can access this API (the style attribute comes into the mutation as text information).
We'll need to come up with something better than this.doc.createElement
(this did bother me at the time, as this is in the recording!)
I wonder do we need to go so far as creating an iframe object (could that have a different CSP policy) and executing createElement
within that? Would that be something you could check? Or let me know how to create a document with this type of restrictive CSP policy.
@razor-x would you mind if we stripped all the CSP policies on replay? I think that would solve this issue for you.
@Juice10 This is on the record side; we are creating and discarding an anonymous here just to access it's style
attribute.
@razor-x how does the document itself modify the style in the first place? (the original modification that generated the mutation) Could you have a look at the calling code for that and check how it's actually doing it?
(via dev tools > inspector > (the element) > break on attribute modifications)
@razor-x how does the document itself modify the style in the first place? (the original modification that generated the mutation) Could you have a look at the calling code for that and check how it's actually doing it?
(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.
node.style.transition = theme.transitions.create('opacity', transitionProps);
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 aria-owns
and not style
)
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):
var throwaway = document.createElement('iframe');
throwaway.setAttribute('sandbox', 'allow-same-origin allow-scripts'); // if it works, I'd also want to know if it works without this!
var sp = throwaway.contentDocument.createElement('span');
sp.setAttribute('style', 'background:black;');
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.
(I'm ignoring the second screenshot as the attribute being set there is
aria-owns
and notstyle
)
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.
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):
var throwaway = document.createElement('iframe'); throwaway.setAttribute('sandbox', 'allow-same-origin allow-scripts'); // if it works, I'd also want to know if it works without this! var sp = throwaway.contentDocument.createElement('span'); sp.setAttribute('style', 'background:black;');
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
var throwaway = document.createElement('iframe');
throwaway.setAttribute('sandbox', 'allow-same-origin allow-scripts'); // if it works, I'd also want to know if it works without this!
document.body.appendChild(throwaway);
var sp = throwaway.contentDocument.createElement('span');
sp.setAttribute('style', 'background:black;');
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 unsafe-inline
.
Here's what's I found that seems to suggest this:
right, contentDocument
isn't available until the iframe is appended to the page; I thought I got the opposite result while testing but must have accidentally added my variable to the page or reused one or something.
Could you try simpler:
var throwaway = new Document();
var sp = throwaway.createElement('span');
sp.setAttribute('style', 'background:black;');
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!)
right,
contentDocument
isn't available until the iframe is appended to the page; I thought I got the opposite result while testing but must have accidentally added my variable to the page or reused one or something.Could you try simpler:
var throwaway = new Document(); var sp = throwaway.createElement('span'); sp.setAttribute('style', 'background:black;');
I imagine the CSP is inherited by the document, but maybe not given the new document isn't actually attached to anything?
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
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? This is a priority for me to ensure our dev instructions are better but the people with the most insight into this are the ones setting things up for the first time.
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? This is a priority for me to ensure our dev instructions are better but the people with the most insight into this are the ones setting things up for the first time.
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 package.json
or in README or .nvmrc
) and just do yarn
then yarn test
and expect it to work (as long as master CI is green upstream). I don't see any other external dependencies listed beyond that. Unfortunately this fails and I don't have much else to go on to fix it. See log below and let me know if you want to move this to a different issue.
Websocket is not open
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.
Here is some more feedback after going a little further.
I tried to run yarn test
in just the packages/rrweb
directory but I got a bunch of errors. I thought maybe this was because I did not run yarn dev
first, so I tried that and it seemed to work, but then web app just loaded a blank page, so not sure what to do with that (see screenshots). I tried to rerun the packages/rrweb
tests again but same errors as before.
This looks a bit like you haven't run yarn install
in the root directory. Could you run yarn install
, and then maybe yarn build:all
just in case.
If you run yarn test
in the package/rrweb directory after that some of these errors should disappear.
This looks a bit like you haven't run
yarn install
in the root directory. Could you runyarn install
, and then maybeyarn build:all
just in case. If you runyarn test
in the package/rrweb directory after that some of these errors should disappear.
yarn install
did not change the result (I had run this before anyway). Running yarn build:all
in the root did let me run the tests inside packages/rrweb
but they failed with new errors, see below:
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. If possible, a general CSP test would be good to catch regression on this, or the emergence of new CSP violations.
@razor-x I've fixed up the conflict — we are trying to be more proactive about merging PRs, so if you could have another look at #846 to get an extra set of eyes on it, e.g. have you been running that patch in production or anything?
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.
I don't have the whole context but from what I understand you could modify an element with element.style.color = 'red';
instead of dealing with its attribute (the latter triggering the CSP).
In the meantime, my workaround is to use style-src-attr 'unsafe-inline'
instead of doing it on style-src
which would break in case of already using a nonce
for third-party files.
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
The optimization in PR https://github.com/rrweb-io/rrweb/pull/464 broke support for users who run a strict Content-Security-Policy (CSP).
Specifically, this line sets the
style
attribute on a DOM node and will be blocked withoutstyle-src: 'unsafe-inline'
(which is the unsafe CSP).https://github.com/rrweb-io/rrweb/blob/661c746b14373dc3c360b2f6ce3e604e33887678/packages/rrweb/src/record/mutation.ts#L487
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.