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

Twitch iframes are filtered out by sanitizer #2897

Closed
humphd opened this issue Feb 12, 2022 · 18 comments · Fixed by #3021 or #3023
Closed

Twitch iframes are filtered out by sanitizer #2897

humphd opened this issue Feb 12, 2022 · 18 comments · Fixed by #3021 or #3023
Assignees
Labels
type: bug Something isn't working type: security Security concerns
Milestone

Comments

@humphd
Copy link
Contributor

humphd commented Feb 12, 2022

Screen Shot 2022-02-12 at 9 29 50 AM

We need to update the sanitizer to allow the twitch iframe origin.

@humphd humphd added type: bug Something isn't working type: security Security concerns labels Feb 12, 2022
@joelazwar
Copy link
Contributor

Am I right to assume the change should be made in of these files?

directives: {
defaultSrc: ["'self'"],
fontSrc: ["'self'", 'https:', 'data:'],
frameSrc: ["'self'", '*.youtube.com', '*.vimeo.com'],
frameAncestors: ["'self'"],
imgSrc: ["'self'", 'data:', 'https:'],
scriptSrc: [

or

contentSecurityPolicy: {
directives: {
defaultSrc: ["'self'"],
fontSrc: ["'self'", 'https:', 'data:'],
frameSrc: ["'self'", '*.youtube.com', '*.vimeo.com'],
frameAncestors: ["'self'"],

@humphd
Copy link
Contributor Author

humphd commented Feb 12, 2022

That's for the front-end, we also need to fix https://github.com/Seneca-CDOT/telescope/blob/master/src/backend/utils/html/sanitize.js#L50-L57 in the backend. And the parser service would need this too.

@HyperTHD
Copy link
Contributor

We also need to update the tests as well to ensure that this is working as intended. This is where they should go https://github.com/Seneca-CDOT/telescope/blob/master/test/sanitize-html.test.js

@HyperTHD HyperTHD self-assigned this Feb 15, 2022
@HyperTHD
Copy link
Contributor

Taking this

@menghif menghif added this to the 2.8 Release milestone Feb 17, 2022
@humphd
Copy link
Contributor Author

humphd commented Feb 20, 2022

I'm seeing this for a second time, still unfixed. @HyperTHD do you have time to work on this, or should we give to someone else?

@HyperTHD
Copy link
Contributor

I'm seeing this for a second time, still unfixed. @HyperTHD do you have time to work on this, or should we give to someone else?

Been pretty busy lately, but got some time. I'll have a PR up today hopefully for this

@manekenpix
Copy link
Member

The issue is still there
image

@manekenpix manekenpix reopened this Feb 21, 2022
@humphd
Copy link
Contributor Author

humphd commented Feb 21, 2022

Won't it need to re-index for this to get applied to that post?

@manekenpix
Copy link
Member

manekenpix commented Feb 21, 2022

@humphd oh, I forgot about that, let me make the changes on staging to see if it solves the issue

@manekenpix
Copy link
Member

No, still the same issue
image

@manekenpix
Copy link
Member

manekenpix commented Feb 21, 2022

I think this is probably related to #2306 , I see the same behaviour in both cases.

@DukeManh
Copy link
Contributor

Could be because the hostname is supposed to be player.twitch.tv vs twtich.tv
@HyperTHD

@DukeManh
Copy link
Contributor

This is Alex's twitch iframe

<iframe src="https://player.twitch.tv/?video=1303803789&amp;parent=dev.to&amp;autoplay=false" allowfullscreen="true" scrolling="no" width="710" height="399" frameborder="0">
</iframe>

@HyperTHD
Copy link
Contributor

This is Alex's twitch iframe

<iframe src="https://player.twitch.tv/?video=1303803789&amp;parent=dev.to&amp;autoplay=false" allowfullscreen="true" scrolling="no" width="710" height="399" frameborder="0">
</iframe>

I tested that hostname, still not viewing it locally

@humphd
Copy link
Contributor Author

humphd commented Feb 22, 2022

Screen Shot 2022-02-21 at 7 58 05 PM

For me on staging, I don't get a URL on the iframe, what am I missing? Did we try re-indexing?

@manekenpix
Copy link
Member

@humphd yes, all the feeds were re-indexed on staging, but the issue persists.

@humphd
Copy link
Contributor Author

humphd commented Feb 22, 2022

@HyperTHD I'd say that it's the sanitizer using 'www.twitch.tv' vs. 'player.twitch.tv' and stripping it during parsing.

@HyperTHD
Copy link
Contributor

I'm not able to see any changes when switching it out, but I can put up a PR to fix this, and hopefully re-indexing on staging will fix that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working type: security Security concerns
Projects
None yet
6 participants