Draft: Fix svg images not correctly displaying in e2ee rooms #8325
Conversation
github-actions
bot
added
the
T-Defect
Codecov Report
@@ Coverage Diff @@
## develop #8325 +/- ##
===========================================
- Coverage 29.88% 29.88% -0.01%
===========================================
Files 879 879
Lines 50074 50086 +12
Branches 12741 12743 +2
===========================================
Hits 14966 14966
- Misses 35108 35120 +12
|
- store original mimetype of blobs as well - if they differ, use a data url instead of a blob url (data urls have a unique origin)
justjanne
force-pushed
the
justjanne/fix/15094-18526-e2ee-broken-svg-embed
branch
from
6506f08 to
c779376
Compare
|
This PR uses data URLs for embedded SVGs in E2EE rooms and other situations where no PNG version is available. This ensures we get a high quality preview without the risk of exposing our origin to user-defined scripts. |
| public get sourceBlob() { | ||
| return this.sourceTypedBlob.value.then(it => it.data); | ||
| } | ||
| public get thumbnailBlob() { | ||
| return this.thumbnailTypedBlob.value.then(it => it.data); | ||
| } |
There was a problem hiding this comment.
these need return types, and a blank line between them
| public readonly sourceBlob: LazyValue<Blob>; | ||
| public readonly thumbnailBlob: LazyValue<Blob>; |
There was a problem hiding this comment.
these were deliberately exposed and should not be hidden. It's for overall performance of the app.
There was a problem hiding this comment.
(also used by projects downstream of us)
| if (this.media.isEncrypted) { | ||
| const content = this.event.getContent<IMediaEventContent>(); | ||
| return decryptFile(content.file, content.info); | ||
| return decryptFile(content.file, content.info).then(data => { | ||
| return { mimetype: content.info.mimetype, data }; |
There was a problem hiding this comment.
info might not exist, and decryptFile should be responsible for mimetype detection.
| if (!this.media.hasThumbnail) return Promise.resolve(null); | ||
|
|
||
| if (this.media.isEncrypted) { | ||
| const content = this.event.getContent<IMediaEventContent>(); | ||
| if (content.info?.thumbnail_file) { | ||
| return decryptFile(content.info.thumbnail_file, content.info.thumbnail_info); | ||
| return decryptFile(content.info.thumbnail_file, content.info.thumbnail_info).then(data => { | ||
| return { mimetype: content.info.thumbnail_info.mimetype, data }; |
There was a problem hiding this comment.
info might not exist, and decryptFile should be responsible for mimetype detection.
thumbnail_info also might not exist
| } | ||
|
|
||
| async function createDataUrl(blob: ITypedBlob): Promise<string> { | ||
| return `data:${blob.mimetype};base64,${await blob.data.text().then(btoa)}`; |
There was a problem hiding this comment.
This opens us up to XSS attacks, which we do not want.
|
@turt2live I appreciate the review, after all this is just a first attempt at experimenting with potential solutions that allow us to get SVG previews working. I'll experiment further with this over the easter weekend, though I don't expect that I'll get a clean fix done soon. (I actually thought I had left the PR in draft status, but that may have gotten lost at some point) |
justjanne
changed the title
|
ah, apologies for the early review then 😅 This is a relatively security sensitive area though, particularly the object URL handling. |
Type: Defect
Related: element-hq/element-web#15094
Related: element-hq/element-web#18526
Here's what your changelog entry will look like:
🐛 Bug Fixes