Repository navigation
🏷️ fix: Type an Image by Its Bytes, Not by Its File Name - #16806
JeongCheolLee wants to merge 2 commits into
Conversation
`resizeImageBuffer` already reports the media type of the bytes it returns, and `saveBase64Image` reads it (LibreChat-AI#15606). The upload path drops it and asks the file name instead, so a file whose name already carries the output extension skips conversion whatever is inside it. A JPEG named `photo.png` is then written as JPEG bytes and recorded `image/png`. Conversion is now skipped only when the name and the bytes both match `imageOutputType`, across every strategy that made the decision from the extension alone: local, Firebase, Azure, the shared `ImageService` behind S3 and CloudFront, and `convertImage`. An unnamed format converts too, since bytes sharp could not name are not evidence of a match.
…orded As A stored type is a claim about bytes it need not describe, and `encodeAndFormatImages` hands it to providers verbatim as Anthropic's `media_type`, Google's `inlineData.mimeType` and the OpenAI data URL prefix. Anthropic refuses a request whose `media_type` disagrees with the bytes, and keeps refusing it, because the image stays in the conversation's history — one mislabeled upload ends the conversation. `sniffImageMimeType` names an image from its signature, which the encode path can afford because it already holds the decoded buffer: no sharp call, no second decode. Records written before the upload path was corrected recover on their next turn. Bytes with no recognized signature keep the stored type rather than being guessed at.
codegraph-librechat
Bot
added
🗺️ Platform Security
|
@codex review the latest head, final review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b6dbd6926
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if ( | ||
| extension.toLowerCase() === targetExtension && | ||
| type === `image/${appConfig.imageOutputType}` |
There was a problem hiding this comment.
Every upload using the local path shown here—and the matching Firebase and Azure edits—now executes this new byte/type branch in legacy CJS, while S3 and CloudFront execute a separate copy in packages/api/src/storage/images.ts; convertImage adds another CJS copy. This splits one storage-format invariant across five implementations, so the next format fix can leave some file strategies writing bytes that disagree with their recorded type; move the conversion/skip decision into a shared TypeScript service/helper and leave these CJS modules as delegators.
AGENTS.md reference: AGENTS.md:L77-L81
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed against AGENTS.md:77-81 at this head. The added type checks in Local, Firebase, Azure, and convertImage are behavior, while ImageService maintains another copy. A shared TypeScript conversion/skip helper called by those strategies would satisfy the requirement without migrating unrelated storage code. This remains open pending that change.
![lia-by-librechat[bot]](https://avatars.githubusercontent.com/in/4931833?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed 0b6dbd692685ec9f588c0b687d06be08369d5d09. The JPEG upload/history fix has passing backend, typecheck, build, integration, Docker smoke, and Lighthouse CI. The head merges cleanly with current dev 012e01dc7faca859d939017580d1b2739c0d6194.
Remaining blockers:
- Fix the generic HEIF-brand regression noted inline.
- Address the existing CJS-boundary thread: AGENTS.md:77-81 requires the format decision in a shared TypeScript helper, not separate new conditions in legacy strategies.
- Static Checks fails only on import ordering in
packages/api/src/storage/__tests__/images.test.ts. Reproduced locally; runnpm run sort-imports -- packages/api/src/storage/__tests__/images.test.ts.
No workflow is awaiting approval: this head's runs were already rerun on October 18. Please update this PR, then rerun checks and request review of the new SHA.
| heix: 'image/heic', | ||
| hevc: 'image/heic', | ||
| hevx: 'image/heic', | ||
| mif1: 'image/heic', |
There was a problem hiding this comment.
[P2] Do not infer HEIC from generic HEIF brands
mif1/msf1 describe generic HEIF containers, not the codec. An AVIF can retain its avif compatible brand with mif1 as the major brand. This lookup returns image/heic, so encodeAndFormatImages replaces a correct stored image/avif with the wrong provider-facing type.
Reproduced at this head using an encoded AVIF fixture: changing only bytes 8..11 from avif to mif1, leaving its AVIF compatible brand and encoded payload intact, changes this helper's result from image/avif to image/heic. Also confirmed the branch and lookup value at a debugger breakpoint.
Remove the generic-brand mappings so the existing stored-type fallback applies, or inspect compatible brands before choosing a codec-specific MIME type. Add a regression for a generic major brand with AVIF compatibility.
|
Thank you, @JeongCheolLee, for the diagnosis, fix, and regression coverage. Closing as superseded by #17230, the canonical fix for #16658. That PR builds on and credits this contribution, centralizes conversion in shared TypeScript, and corrects the compatible-brand AVIF handling. Follow-up review and CI are tracked there. |
2 participants
Summary
An uploaded image is converted to
imageOutputType, but whether it is already in that format is decided from the file extension alone. A file whose name already carries the target extension skips conversion whatever is inside it, so a JPEG namedphoto.pngis stored as JPEG bytes and the record is typedimage/png. Browsers' "Save image as", messengers and some screenshot tools produce exactly that file.encodeAndFormatImageshands the recorded type to providers verbatim —data:${file.type}for OpenAI,inlineData.mimeTypefor Google,source.media_typefor Anthropic. Anthropic refuses a request whose declared media type disagrees with the bytes:messages.0.content.2.image.source.base64: The image was specified using the image/png media type, but the image appears to be a image/jpeg image. The image stays in the conversation's history, so every later message in that conversation fails the same way, and the conversation is over. OpenAI and Google accept the same file, so it only surfaces on Claude.This is the same bug #15606 fixed for the base64 path, on the upload path it did not reach. That PR added
resolveImageMimeTypeand taughtresizeImageBufferto report the media type of the bytes it returns;saveBase64Imagereads it, and the upload strategies discard it. This PR makes them read it too, and settles the provider-facing type from the bytes so that conversations already holding a mislabeled image recover rather than staying broken.Fixes #16658.
How it works
resizeImageBufferhas reported the real type since #15606, and the upload path drops it on the floor:const { buffer: resizedBuffer, width, height, + type, } = await resizeImageBuffer(inputBuffer, resolution, endpoint); -if (extension.toLowerCase() === targetExtension) { +if ( + extension.toLowerCase() === targetExtension && + type === `image/${appConfig.imageOutputType}` +) {Every strategy made that decision from the extension alone, so each one gets the same treatment:
Bytes sharp could not name convert as well, since an unnamed format is not evidence of a match. With the stored bytes genuinely in the output format,
process.js:509—type: \image/${appConfig.imageOutputType}`` — becomes a true statement again rather than needing to change.That corrects what gets written from now on. Records already written stay wrong, and they are the ones holding conversations hostage, so the encode path settles the type from the bytes it is already holding:
sniffImageMimeTypeis new inpackages/api/src/files/mime.ts, besideresolveImageMimeType, and names an image from its signature rather than by decoding it. The encode path can afford that because the buffer is already decoded a few lines above forvalidateImage, so the addition is a comparison against the first twelve bytes: no sharp call, no second decode, nothing per-image that was not already paid. UsingresolveImageMimeTypehere instead would mean a decode per image per turn. Bytes with no recognized signature keep the stored type rather than being guessed at.Type of change
Testing
Tested environments/configuration:
fileStrategy: local, defaultimageOutputType(png)Repro, from the issue:
node -e "require('sharp')({create:{width:320,height:200,channels:3,background:'#d22'}}).jpeg().toFile('fake.png')"—file fake.pngreportsJPEG image data.fake.pngwith the upload button. Pasting re-encodes in the browser, so it has to be the button.Before: 400 on the first message and on every message after it. After: the file is stored as a real PNG, and a conversation that already contains a mislabeled image recovers on its next turn, because the history is re-encoded each time.
Automated tests:
packages/api/src/files/mime.spec.ts— signatures for png/jpeg/webp/gif/tiff agree with what sharp reads back out of the same bytes; AVIF and HEIC are told apart byftypbrand; a RIFF that is not a WebP, anisomMP4, a short buffer and a non-image all returnundefined.packages/api/src/files/encode/image.spec.ts— a record typedimage/pngholding JPEG bytes is declaredimage/jpegto Anthropic, to Google and in the data URL; unrecognized bytes keep the stored type; external URLs are left alone.api/server/services/Files/Local/__tests__/images.spec.js—uploadLocalImageconverts a JPEG given a.pngname, converts when sharp named no format, and writes the resized bytes untouched only when the name and the bytes both match.packages/api/src/storage/__tests__/images.test.ts— the same forImageService. Its existing "skips format conversion when extension matches target" case asserted the behavior this PR changes, so it now asserts the name and the bytes together, with the mismatch covered by a new case.cd packages/api && npx jest src/storage src/files→ 1407 passed. Three failures inlibreoffice.spec.tsare pre-existing and environment-dependent — they need LibreOffice on$PATHand fail identically on a cleandev.cd api && npx jest server/services/Files→ 456 passed.tsc --noEmitandeslintclean on the changed files.Screenshots / recordings
No user-facing change.
Risk / compatibility
Uploads that previously skipped conversion on the strength of their name now convert when the bytes disagree, which is the fix rather than a side effect, but it does mean such a file is re-encoded where before it was passed through. Files whose name and bytes already agree are unaffected and still skip conversion.
One allocation moves: in
encodeAndFormatImagestheBuffer.from(imageContent, 'base64')was guarded byfile.height && file.width, and now runs for every non-URL image, so records without stored dimensions pay a base64 decode they did not before. The bytes have to be read for the type to be right, and stored images carry dimensions in the common case, but I am happy to gate it differently if you would rather not spend that.No schema, configuration or API changes.
Checklist