Closes issue #420 #423
Closes issue #420 #423
Conversation
There was a problem hiding this comment.
Great start on this issue, I’ve included the full screenshot of all vulnerable code paths.
There are other areas we have the same issue like:
makeListenURL()(which addressesanchorTimestamps())makeSearchURL()
Perhaps we can address all of these with adding encodeURI() inside cleanURL(), will need some manual testing. See my inline comment.
If you’re up for it, I would love to see some tests added for this area!
| // Use a function to replace matches | ||
| return text.replace(re, function(match) { | ||
| // Escape the match to prevent XSS | ||
| var url = encodeURIComponent(match); |
There was a problem hiding this comment.
Thanks for the PR @ChandraShekharAgrawal!
We actually want encodeURI(match) here instead, otherwise we will get invalid URLs.
e.g.
If match is http://smarturl.it/outlines, then url becomes a non-valid URL http%3A%2F%2Fsmarturl.it%2Foutlines, which is then interpreted by the browser as http://localhost:8080/http%3A%2F%2Fsmarturl.it%2Foutlines when running locally.
There was a problem hiding this comment.
You can test this yourself locally after you run npm install and npm start...
- Go to
http://localhost:8080/?v=03O2yKUgrKw(or other video with links in the description, this is one of our demo videos). - Click
Show Description - Inspect any links in the browser
There was a problem hiding this comment.
@ChandraShekharAgrawal hi, any chance you can revisit this?
Types of changes
What types of changes does your code introduce? Check all the boxes that apply:
Description
Final checklist:
Go over all the following points and check all the boxes that apply
If you're unsure about any of these, don't hesitate to ask. We're here to help!