Don't fail on uploading unparsable images#3249
Don't fail on uploading unparsable images#3249db3000 wants to merge 6 commits intobluesky-social:mainfrom
Conversation
|
I think that the PDS shouldn't try to detect what kind of file the uploaded blob is—from what I've heard, at least, this autodetection has only caused issues. The example SVG upload in #3151 still throws an error even if it is in the middle of a text file, in effect preventing HTML with any tags from being uploaded as a blob. The most that the PDS should do is make sure that the specified mimetype exists. |
|
Agree with @Saturn-VI — I think if the PDS wants to try to detect extra metadata that's fine, but ultimately it should just fall back to treating the blob as opaque. |
|
is there any status updates on this getting merged? its blocking a project of mine and i really dont want to upload userfiles as zips or something |
db3000
force-pushed
the
explicit_image_error
branch
from
375f2de to
e8f2f7f
Compare
db3000
force-pushed
the
explicit_image_error
branch
from
6373f7c to
4ed0840
Compare
db3000
changed the title
|
Changes the PR as per comments here and in #3151 , this PR now changes it so the PDS no longer validates the image but just continues if it is malformed |
matthieusieben
left a comment
matthieusieben
left a comment
There was a problem hiding this comment.
The suggested change will introduce other errors. This might also not be aligned with the way we'd actually want to fix this.
| // The buffer has a corrupted image or no image at all. | ||
| return null |
There was a problem hiding this comment.
This is not a proper fix as it will also mute any pipeline(stream, processor) error.
There was a problem hiding this comment.
Good point, I will limit the exceptions that are ignored here
There was a problem hiding this comment.
I put back the previous code and just added another specific error message that gets ignored
|
|
||
| export const errHasMsg = (err: unknown, msg: string): boolean => { | ||
| return !!err && typeof err === 'object' && err['message'] === msg | ||
| return !!err && typeof err === 'object' && err['message'].startsWith(msg) |
There was a problem hiding this comment.
This was necessary as the error is actually:
Input buffer has corrupt header: glib: XML parse error: Error domain 1 code 73 on line 1 column 6 of data: Couldn't find end of Start Tag svgQ line 1
|
Superseded by #4560 |
6 participants
Fixes #3151
Uploading an attachment using
com.atproto.repo.uploadBlobwill fail with a 500 Internal Server Error if the contents of the upload has the signature of an image (as determined by sharp via ImageMagick) but is not fully parsable as the corresponding image type.While it is possible to handle such uploads by silencing this error, this PR errs on the side of caution and continues to reject corrupted images but with a more explicit error. It seems to me that the risk of allowing people to upload malformed images outweighs the benefit of more flexible uploads. I can change the code to silently accept such uploads though if that is desirable instead.