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

Updates to the youtube regex #3

Closed
wants to merge 4 commits into from
Closed

Conversation

techdragon
Copy link
Contributor

Improved matching of different possible youtube video urls

@coveralls
Copy link

Coverage Status

Coverage decreased (-0%) when pulling f8693c3 on techdragon:master into 231e378 on yetty:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0%) when pulling 5032e0b on techdragon:master into 231e378 on yetty:master.

@yetty
Copy link
Collaborator

yetty commented Aug 12, 2013

Hi, thank you for your commit. I agree, that youtube regex is too complicated. But I guess it is necessary. But your regex allows bad urls to pass. Look at d0d357b and test_detect_bad_urls. Regex should protect against faked urls.

I know, that if you use {% video ... %} tag, you will work with correct url, because it is parsed, even if origin url is invalid. But even so I consider it to be security risk.

@yetty yetty closed this Aug 12, 2013
@techdragon
Copy link
Contributor Author

Are you happy merging in the changes after I fix the input validation issue you highlighted? I have some improvements to the Vimeo regex coming up and I'm happy to fix this and include them both in the one request if your ok with the overall purpose of the code.

I'd rather not maintain a permanent fork of this app in order to enable my site to accept youtube urls that are shortened or from the embed code, or any one of the dozens of other ways a youtube video can be linked to. And I'm sure I'm not the only one who would prefer the URL input be more 'accepting' of other youtube video url formats.

@yetty
Copy link
Collaborator

yetty commented Aug 22, 2013

If it passes the tests, I have no problem to integrate it :)

And please provide also these URLs, which now doesn't pass.
You can add them EmbedVideoTestCase.youtube_url. I am not sure with whom URLs is problem. I tried to cover to most common ones, but I admit, there could be anothers, which doesn't work.

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

Successfully merging this pull request may close these issues.

None yet

3 participants