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

PNG uploads as a file attachment rather than image on develop #24761

Closed
kittykat opened this issue Mar 7, 2023 · 8 comments
Closed

PNG uploads as a file attachment rather than image on develop #24761

kittykat opened this issue Mar 7, 2023 · 8 comments
Labels
A-File-Upload Attachments and file uploads O-Occasional Affects or can be seen by some users regularly or most users rarely S-Major Severely degrades major functionality or product features, with no satisfactory workaround T-Defect X-Needs-Info This issue is blocked awaiting information from the reporter

Comments

@kittykat
Copy link
Contributor

kittykat commented Mar 7, 2023

Steps to reproduce

  1. In a room, attach a PNG

Outcome

What did you expect?

See image preview

What happened instead?

See file:

Screenshot from 2023-03-07 13-38-40

Operating system

No response

Browser information

Chromium 110.0.5481.177 (Official Build) Arch Linux (64-bit)

URL for webapp

develop.element.io

Application version

Element version: 5ae88eb-react-32aa18ff2e0e-js-b4cdc5a92397 Olm version: 3.2.12

Homeserver

matrix.org

Will you send logs?

No

@kittykat kittykat added T-Defect X-Regression S-Major Severely degrades major functionality or product features, with no satisfactory workaround A-File-Upload Attachments and file uploads O-Occasional Affects or can be seen by some users regularly or most users rarely labels Mar 7, 2023
@t3chguy
Copy link
Member

t3chguy commented Mar 7, 2023

@kittykat please send logs, this happens when your browser's canvas can't generate a thumbnail typically

@t3chguy t3chguy added X-Needs-Info This issue is blocked awaiting information from the reporter and removed X-Regression labels Mar 7, 2023
@kittykat
Copy link
Contributor Author

kittykat commented Mar 7, 2023

Room ID: !UlIlSOXSmCVQUaTbFh:matrix.org
Message ID: $tYcrlMrJW6kiz8djEGuvcsJZTybepoFzZkjbss_cZZ0

@t3chguy
Copy link
Member

t3chguy commented Mar 7, 2023

2023-03-07T13:54:24.431Z E Invalid .png file header
Error: Invalid .png file header
    at extractChunks (https://develop.element.io/bundles/b4fa679a5cbd84250cf6/vendors~init.js:178640:31)
    at https://develop.element.io/bundles/b4fa679a5cbd84250cf6/init.js:63467:50
    at async Promise.all (index 0)
    at async loadImageElement (https://develop.element.io/bundles/b4fa679a5cbd84250cf6/init.js:63477:19)
    at async infoForImageFile (https://develop.element.io/bundles/b4fa679a5cbd84250cf6/init.js:63511:24)
    at async ContentMessages_ContentMessages.sendContentToRoom (https://develop.element.io/bundles/b4fa679a5cbd84250cf6/init.js:63825:29)

@kittykat would you be able to share the problem file (internally on the rageshake)

@kittykat
Copy link
Contributor Author

kittykat commented Mar 7, 2023

@t3chguy done! It's a screenshot from a Pixel 6a running Graphene OS. It uploads fine as a photo when I attach it from Element Android

@t3chguy
Copy link
Member

t3chguy commented Mar 7, 2023

The file is called .png but it actually is a JPEG file

t3chguy@Michael-t3chguy-MBP ~/Desktop> file 223445092-3dab0ff7-8527-4861-929f-7e8dc5fc53dc.png 
223445092-3dab0ff7-8527-4861-929f-7e8dc5fc53dc.png: JPEG image data, JFIF standard 1.01, aspect ratio, density 1x1, segment length 16, baseline, precision 8, 1080x2400, components 3

@t3chguy t3chguy closed this as completed Mar 7, 2023
@kittykat
Copy link
Contributor Author

kittykat commented Mar 7, 2023

🤔 Wonder what my phone is doing…

Should JPEGs not be previewed? Is it assuming file type based on extension? If it is, can we get it to check file type instead?

@t3chguy
Copy link
Member

t3chguy commented Mar 7, 2023

JPEG should be, but for PNGs we need to check if it is hidpi by parsing it, so if we get given a faulty PNG we have to assume the file is broken not just mis-named

Is it assuming file type based on extension?

It is asking the browser but some browsers do indeed naively assume by file extension (sometimes being completely wrong for container formats like mkv which can be audio-only or video)

It is using this https://developer.mozilla.org/en-US/docs/Web/API/File/type

Note: Based on the current implementation, browsers won't actually read the bytestream of a file to determine its media type. It is assumed based on the file extension; a PNG image file renamed to .txt would give "text/plain" and not "image/png". Moreover, file.type is generally reliable only for common file types like images, HTML documents, audio and video. Uncommon file extensions would return an empty string. Client configuration (for instance, the Windows Registry) may result in unexpected values even for common types. Developers are advised not to rely on this property as a sole validation scheme.

can we get it to check file type instead?

We would need to bundle large libraries to parse image/video files to handle detection ourselves

@t3chguy
Copy link
Member

t3chguy commented Mar 7, 2023

@kittykat matrix-org/matrix-react-sdk#10308 should allow the PNG parsing to fail, but this file is likely to be problematic either way as we'll be sending it with a mime type of image/png (as the browser tells us) and other devices (native/mobile) may look at that, pick the specific renderer and fail to render it entirely. Not sure what Synapse's thumbnailer will do either, that could fail also.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-File-Upload Attachments and file uploads O-Occasional Affects or can be seen by some users regularly or most users rarely S-Major Severely degrades major functionality or product features, with no satisfactory workaround T-Defect X-Needs-Info This issue is blocked awaiting information from the reporter
Projects
None yet
Development

No branches or pull requests

2 participants