Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CSP issue because of inline styling #878

Closed
ennair opened this issue Jul 11, 2023 · 7 comments
Closed

CSP issue because of inline styling #878

ennair opened this issue Jul 11, 2023 · 7 comments
Labels
feature New feature or request

Comments

@ennair
Copy link

ennair commented Jul 11, 2023

Describe:

At the moment I have CSP issues when using vidstack/player, because it uses inline styling. I have the feeling de inline styles are nog doing anything, but I cannot remove them. To solve this problem I would have to add style-src 'unsafe-inline', which I do not want to add, because that is unsafe.

Can the inline styles be removed? Or at least add an option to turn it off?

Code snippets of parts of the code with inline styles:
<media-player class="VidstackVideoPlayer_mediaPlayer__RF3jP" mk-h="" mk-d="" tabindex="0" role="region" data-title="Example" aria-label="Video - Example" data-orientation="landscape" aria-busy="false" data-can-seek="true" data-media-type="video" data-paused="true" data-stream-type="on-demand" data-view-type="video" data-bp-x="md" data-bp-y="sm" data-can-fullscreen="true" data-captions="true" data-can-load="true" data-can-play="true" style="--media-width: 600px; --media-height: 346px;">

<media-captions mk-h="" mk-d="" aria-hidden="false" data-dir="ltr" translate="yes" aria-live="off" aria-atomic="true" part="captions" style="--overlay-width: 588px; --overlay-height: 334px;"></media-captions>

@ennair ennair added the feature New feature or request label Jul 11, 2023
@mihar-22
Copy link
Member

mihar-22 commented Oct 2, 2023

This issue is outdated now since our new 1.0@next release. If the issue persits after upgrading, feel free to let me know here and we can reopen. If the issue has changed then please open a new one.

@mihar-22 mihar-22 closed this as completed Oct 2, 2023
@ennair
Copy link
Author

ennair commented Oct 5, 2023

@mihar-22 Even after upgrading to the latest version 1.1.7 I still have inline styling everywhere, which causes my CSP to fail.
I used this example: https://github.com/vidstack/examples/tree/main/player/react/default-theme

Here are some code snippets:
<div data-media-player="" role="region" data-title="De Toespraak van de Koning (Media library video example)" aria-label="Video - De Toespraak van de Koning (Media library video example)" data-orientation="landscape" aria-busy="true" data-buffering="" data-can-seek="" data-media-type="video" data-paused="" data-stream-type="unknown" data-view-type="video" class="Video_player__2q8Ei media-player" tabindex="0" style="--player-width: 920px; --player-height: 518px; --media-width: 920px; --media-height: 518px;" data-pointer="fine" data-can-fullscreen="">

<div data-media-gesture="" event="pointerup" action="toggle:paused" class="VideoLayout_gesture__RG1Gd" style="pointer-events: none;"></div>

<div class="VideoLayout_controlsGroup__J14Np vds-controls-group" style="pointer-events: auto;"></div>

@mihar-22
Copy link
Member

mihar-22 commented Oct 5, 2023

We won't be removing inline styles as they're a core part of the library. They're used in too many places to remove them all or make it optional. Unfortunately, the only options are to relax your CSP or try another player. I can't offer much here sadly.

@ennair
Copy link
Author

ennair commented Oct 12, 2023

Thanks for your response. Too bad, we love your player!

When I look at your code you are using the inline styles for dynamic styling. To still use dynamic styling, but to also be able to help users with strict CSP another solution would be to enable the use of nonce.
An approach for dynamic styling could be this https://github.com/vercel/styled-jsx#dynamic-styles which also supports strict CSP with nonce https://github.com/vercel/styled-jsx#content-security-policy.
Another example of another library with the same issue is this one https://github.com/cristianbote/goober with the possible solution cristianbote/goober#471.

Would you be willing to enable the use of nonce? That would solve our problem and we would still be able to use your player. We are also willing to help to make Vidstack even better.

@mihar-22 mihar-22 reopened this Oct 12, 2023
@mihar-22
Copy link
Member

As long as there's a reasonable way to do it that doesn't bloat the library or impact users who don't care about CSP, then I'm totally open to it. The problem here is we set and update CSS variables and occasionally set display property on some elements. How can provide a nonce for these? It seems the nonce has to be set on a <style> element which I don't believe works in this case.

@ennair
Copy link
Author

ennair commented Oct 18, 2023

Thanks for you response! We have dived into your code and the CSP problem.

We found out that when changing styling using the attributes (setAttribute) function on an element will trigger CSP issue. But when changing the CSS Object Model directly, which is done in the Vidstack code, this will not trigger CSP issues on the client side.

Only problem left is with server side rendering. In case of server side rendering the CSP blocks the inline styling, which makes sense as you are not able to distinguish between malicious user's injected styling and your own trusted styling.
So a solution for now for our CSP problem would be to render Vidstack client side instead of server side. Here is someone who has the same problem and explains it in more detail: w3c/webappsec-csp#174 (comment)

Is rendering Vidstack client side a good alternative, or would this generate other problems? Or do you have another solution?

Attached you can find a screenshot of our 11 CSP issues on page load when using server side rendering. By the way, we're using Next.js 13 using page router.

SSR-CSP-issues

@mihar-22
Copy link
Member

mihar-22 commented Oct 18, 2023

Thanks for the update! I think rendering client-side is a reasonable workaround. As long as you're setting up a container with a specified aspect ratio to prevent layout shifts, then it should be all good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants