Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Use createObjectURL instead of readAsDataURL for videos #2176

Merged
merged 3 commits into from
Oct 4, 2018

Conversation

Half-Shot
Copy link
Contributor

@Half-Shot Half-Shot commented Sep 26, 2018

@Half-Shot Half-Shot requested a review from a team September 26, 2018 01:05
@Half-Shot
Copy link
Contributor Author

Don't really know what caused all those tests to fail :/

@dbkr
Copy link
Member

dbkr commented Sep 26, 2018

I assume you're aware of the minefield that is object URLs and e2e, as per 9701fd3 - the short version is that object URLs have the origin of the page, so any js in them has access to the user's private keys.

Basically, we need to be certain there's no way js could sneak into the object URI and get executed, eg. if the user is tricked into uploading a malicious file. Given that we just take the object URL and set it as the src of a video element, I can't see how that would happen, so I think this is fine. @ara4n - do you remember and was there a reason not to include this when you did #1864 ?

@Half-Shot
Copy link
Contributor Author

I assume you're aware of the minefield that is object URLs and e2e, as per 9701fd3 - the short version is that object URLs have the origin of the page, so any js in them has access to the user's private keys.

Yeah 😞

Even if the user managed to upload a js file as a video file (and I think chrome and friends even do some sort of crazy ffmpeg check when doing mimetypes), I'd expect the browser to be very careful about media source files anyway.

I really must stress that it's going to really limit the use of Riot for video content if we can't stick relatively small (for video) files into the client without it going tits up. Thanks for the explanation though.

@ara4n
Copy link
Member

ara4n commented Oct 3, 2018

i just forgot to do it, i think; this (cursorarily) seems ok to me

Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

lgtm then

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

Successfully merging this pull request may close these issues.

3 participants