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

Fix CSS injection for sites with 'require-trusted-types-for' #449

Merged
merged 1 commit into from
Aug 10, 2024

Conversation

vvto33
Copy link
Contributor

@vvto33 vvto33 commented Aug 1, 2024

Description

This pull request modifies the CSS injection method in the OBS source code. Specifically, it replaces the use of innerHTML with createTextNode for inserting styles. This change allows the CSS injection to work properly on websites with strict Content Security Policy (CSP) that includes Trusted Types enforcement.

Motivation and Context

On certain websites, particularly those with strict CSP including require-trusted-types-for 'script', our previous CSS injection method failed. This resulted in a TypeError:

"This document requires 'TrustedHTML' assignment. Uncaught TypeError: Failed to set the 'innerHTML' property on 'Element': This document requires 'TrustedHTML' assignment."

resnponse_header_content-security-policy
obs_css_not_applied

This change is required to ensure OBS functions correctly across a wider range of websites, including those with stringent security policies.

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • Tweak (non-breaking change to improve existing functionality)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

@crhodesai
Copy link

This looks great, please merge this <:3

@WizardCM WizardCM self-requested a review August 4, 2024 00:14
@WizardCM WizardCM added the Bug Fix Non-breaking change which fixes an issue label Aug 4, 2024
This fix addresses an issue occurring on some pages where the response header includes
`content-security-policy: require-trusted-types-for 'script'`.
@NEKOYASAN
Copy link

NEKOYASAN commented Aug 7, 2024

While this may be a sufficient response to the current situation where YouTube comments cannot be displayed, i think we should check to see if we can guarantee that it will work when other CSP policies are applied.

For example, strict-dynamic with nonce

@vvto33 vvto33 changed the title Fix CSS injection for sites with strict CSP Fix CSS injection for sites with 'require-trusted-types-for' Aug 7, 2024
@HeyThisHaku
Copy link

Oh I was try and patch to latest version, I think currently that’s method work fine

@L0ckedkey
Copy link

This method actually worked on my machine tho, using windows 11 and latest obs version to use this patch. Maybe someone with better experience than me can validate this.

@NEKOYASAN
Copy link

Browser Source is a feature that allows the browser to be used as an OBS source, and not just for displaying Youtube chats, so I think we need to discuss whether putting in a dedicated patch just for Youtube is the right direction for the feature.

I’m not strongly opposed to this change. It is useful and agreeable as a temporary patch.
It would be for the OBS team to decide whether to include it as a temporary patch or not.

@vvto33
Copy link
Contributor Author

vvto33 commented Aug 8, 2024

I appreciate the feedback from @NEKOYASAN. You were absolutely right that the original commit title, "Strict CSP," was inaccurate.

To clarify, this modification was motivated by a specific issue with CSS injection on YouTube. However, I believe the updated code should work for general websites as well, not just YouTube.

This modification allows CSS injection with the require-trusted-types-for 'script' header but fails with strict-dynamic with nonce. However, the current code fails in both cases.

Of course, I haven't been able to test this across every possible website, so there may be additional limitations. However, based on my understanding, this modification should provide better than the original.

In summary, I believe this change, while not perfect, offers some incremental improvement over the current implementation.

@HeyThisHaku
Copy link

Refer from @NEKOYASAN, I think by default if some resource implements strict-dynamic with nonce in header This feature won't work right?

Isn't that normal? Because the purposed strict-dynamic with randomize nonce are made to strictly prevent xss? (Please correct me If I'm wrong). Is that possible to read header nonce and replicate it to script for make this feature dynamically run for other CSP?

and for youtube case and maybe other web who use require-trusted-types-for 'script' without strict-dynamic with nonce indirectly allows adding nodes without eval and innerhtml, right?

(Sorry if my statement was not in line with the intended context, please correct me if there is any misunderstanding.)

@Fade253
Copy link

Fade253 commented Aug 8, 2024

cool thing, but how do i install it so i can make my chat in obs work

@Fade253
Copy link

Fade253 commented Aug 8, 2024

hi, how do i make this work, i do not understand the slightest how to get this installed in any capacity as there is both no real way to downlod this or any real tutorial on how to put this in

@Fade253
Copy link

Fade253 commented Aug 8, 2024

also is there any way to make this work on an installed obs, or do i have to just roll it back to a previous version to make the entire thing work

@NEKOYASAN
Copy link

NEKOYASAN commented Aug 9, 2024

In its current state, if there is a CSP policy that affects Style or inject, no custom CSS is applied at all. (@HeyThisHaku is right.)
Personally, I think that Browser source custom CSS should be Injected and work correctly under all situation.
(I don't know if the current inability to work is intentional for the OBS team or an unexpected bug that we haven't encountered before.)
I suppose it would be possible to get a nonce and force Inject Style with nonce, but it wouldn't be something that could be done immediately and need time to think about whether there are any concerns

For maintainers
I believe that this patch is a necessary and very useful temporary patch to solve the current problem.
This inability to inject CSS into the YouTube chat display is a problem that affects many OBS users and streamers, so a version should be released as soon as possible.
Very sorry, this sentence was superfluous. very thanks to all the OBS project maintainers.

@dcmouser
Copy link

dcmouser commented Aug 9, 2024

Tested in my local build and it does indeed solve a problem that recently started where css modifications were no longer injecting successfully in browser sources. Appreciate the fix.

Copy link
Member

@WizardCM WizardCM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have verified that the old implementation doesn't work on YouTube since they changed their CSP, and that the new code fixes the behaviour.
I've also verified that the new code continues to work on Twitch.

Thanks for putting together a valid, workable solution so quickly. There's no ETA on when we can get this out in a patch, but it's on our priority list.

As a note to others in future - we generally review PRs around the weekend, so while we see every PR as it comes in, it may take a few days before a PR gets merged. Especially if it's browser related. Though we appreciate the enthusiasm, comments like "please merge this" will not change our opinions or timelines, they're just noise to anyone who has email notifications on.

@WizardCM WizardCM merged commit 22db20b into obsproject:master Aug 10, 2024
1 check passed
@mvbhz
Copy link

mvbhz commented Aug 14, 2024

Write how to put it in OBS because no one knows how to do it. A short text on how to do it would be useful because otherwise it's useless.

@EliasGNA
Copy link

I'm using OBS 27.2.4 and can't upgrade due to other aspects. Can someone tell me exactly what I need to do to get 27.2.4 to work with YouTube chat. I just can't seem to figure out how to use this injection. Please give clear instructions as the previous commenter mentioned. It would be greatly appreciated. Thank You,

@Warchamp7
Copy link
Member

Write how to put it in OBS because no one knows how to do it. A short text on how to do it would be useful because otherwise it's useless.

You will need to compile OBS yourself with this change or wait for the next update

@vargascarlitos
Copy link

I spent all day struggling to customize the chat for my YouTube live stream. I ended up in this PR. I'm anxious for the new version for macOS :v

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Fix Non-breaking change which fixes an issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.