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

Enable soundcloud player #90

Closed
wants to merge 2 commits into from
Closed

Enable soundcloud player #90

wants to merge 2 commits into from

Conversation

TigerND
Copy link
Contributor

@TigerND TigerND commented Jul 30, 2016

No description provided.

@TigerND
Copy link
Contributor Author

TigerND commented Jul 31, 2016

@jcalfee
Copy link
Contributor

jcalfee commented Aug 5, 2016

Interesting, this works? src.indexOf(reYoutube) .. I don't see this feature in the JavaScript docs (with a regex as a parameter).

Do you have some soundcloud url docs so we can see what we are whitelisting?

@svk31
Copy link
Contributor

svk31 commented Aug 5, 2016

@jcalfee reYoutube is simply a variable containing the original whitelist regex, he just moved it to the top together with the soundcloud and vimeo regexes.

@svk31
Copy link
Contributor

svk31 commented Aug 5, 2016

@svk31
Copy link
Contributor

svk31 commented Aug 5, 2016

I've just merged this manually in the develop branch, closing. Oh and thank you!

@svk31 svk31 closed this Aug 5, 2016
@jcalfee
Copy link
Contributor

jcalfee commented Aug 6, 2016

I reverted this for now it has bugs it was not tested. @roadscape was going to submit something he found. Please fix and create another merge request and let us have more time to review. This code touches user provided content so it needs to have extra testing .. We do this internally if we have to modify code like this.

@roadscape
Copy link
Contributor

We found some issues with this commit and ran out of time this week to fix...

if (src.indexOf(reYoutube) === 0) is not valid, it always returns -1, which would allow all annoying youtube parameters through. if(re === reYoutube) would fix this.

Also, in soundcloud embed I see there are multiple query params which need to be sanitized and whitelisted:

<iframe ... src="https://w.soundcloud.com/player/?url=https%3A//api.soundcloud.com/tracks/205462171&amp;auto_play=false&amp;hide_related=false&amp;show_comments=true&amp;show_user=true&amp;show_reposts=false&amp;visual=true">

We definitely want auto_play to be forced to false. I don't know about the other params but to start with we should force-set all params to reasonable defaults (except for 'url' of course). I'd recommend parsing out the url param and building the src in a strict manner. As stated above, this code concerns user-generated content so we need to be very careful.

@svk31
Copy link
Contributor

svk31 commented Aug 8, 2016

Yea I should've caught that, sorry. The indexOf won't work like that with a regex expression, it should be replaced with a regex test instead, or a srcYoutube string could be added.

eonwarped pushed a commit to eonwarped/condenser that referenced this pull request Aug 18, 2019
…coinpan_allowpw

Open password login for SCT
eonwarped pushed a commit to eonwarped/condenser that referenced this pull request Aug 29, 2019
…coinpan_allowpw

Open password login for SCT
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants