From 543ae5671a378dbc933481ee5d496733d614ec0a Mon Sep 17 00:00:00 2001 From: Chris Date: Thu, 8 Oct 2026 21:05:59 +0000 Subject: [PATCH] fix(chat): accept image attachments whatever the case of their extension A file named IMG_0001.JPG was refused with "is not a valid image upload", although the page lets it be attached. The chat route gave the upload parser a list of extensions, and @adonisjs/bodyparser compares an extension case-sensitively, so only jpg, jpeg, png and webp in lower case were valid. The parser now checks size only, and normalizeChatImage checks the parser's extname against the same list without regard to case. The extname is the type the parser read from the file's bytes when the name has no extension or a lower-case one it recognizes, and the extension as written otherwise. A refusal is the same 422. The options move to CHAT_IMAGE_UPLOAD_OPTIONS in chat_images.ts, so a test can use the controller's value; size is MAX_CHAT_IMAGE_BYTES, the same 8 MB. Behavior, from 62 combinations of file name and content run through the real parser and normalizeChatImages on dev and on this change: nothing that was accepted is refused, 39 are unchanged, and every difference comes from an upper-case or mixed-case extension. Upper-case and mixed-case JPEG, PNG and WebP names are accepted, and so is an animated PNG named .PNG: the parser does not read the bytes behind an upper-case name, so sharp takes its first frame, as it already does for one named .jpeg. A GIF, TIFF, AVIF or SVG named .JPG or .PNG gets 415 from sharp instead of 422. Text, PDF and one-byte files named .JPG or .PNG keep their 422, and the message changes from "is not a valid image upload" to "could not be decoded as an image". Tests: tests/unit/chat_image_upload.spec.ts sends files through the real multipart parser with those options, then normalizeChatImages. It fails if the list goes back into the parser options, if the extension check is dropped, if it is made case-sensitive, or if a file with no extname is let through. The hand-built files in chat_images.spec.ts and ollama_controller_chat.spec.ts gain an extname, as the parser sets one. The 13 tests in chat_image_upload.spec.ts and chat_images.spec.ts pass, and typecheck shows the same 11 errors as dev, from packages missing or older in my checkout. Not run: the controller in a running install, and its Japa spec (openai is missing in my checkout); that spec's hand-built file was checked against normalizeChatImages directly. --- admin/app/controllers/ollama_controller.ts | 6 +- admin/app/utils/chat_images.ts | 26 +++- admin/tests/unit/chat_image_upload.spec.ts | 133 ++++++++++++++++++ admin/tests/unit/chat_images.spec.ts | 11 +- .../tests/unit/ollama_controller_chat.spec.ts | 3 + 5 files changed, 171 insertions(+), 8 deletions(-) create mode 100644 admin/tests/unit/chat_image_upload.spec.ts diff --git a/admin/app/controllers/ollama_controller.ts b/admin/app/controllers/ollama_controller.ts index d0dce83b..68c4a22a 100644 --- a/admin/app/controllers/ollama_controller.ts +++ b/admin/app/controllers/ollama_controller.ts @@ -21,6 +21,7 @@ import logger from '@adonisjs/core/services/logger' import { rm } from 'node:fs/promises' import { attachImagesToLatestUserMessage, + CHAT_IMAGE_UPLOAD_OPTIONS, ChatImageError, normalizeChatImages, type NormalizedChatImage, @@ -65,10 +66,7 @@ export default class OllamaController { } async chat({ request, response }: HttpContext) { - const uploadedImages = request.files('images', { - size: '8mb', - extnames: ['jpg', 'jpeg', 'png', 'webp'], - }) + const uploadedImages = request.files('images', CHAT_IMAGE_UPLOAD_OPTIONS) const cleanupUploadedImages = () => Promise.all( uploadedImages diff --git a/admin/app/utils/chat_images.ts b/admin/app/utils/chat_images.ts index d26e665d..59fe4ec1 100644 --- a/admin/app/utils/chat_images.ts +++ b/admin/app/utils/chat_images.ts @@ -1,4 +1,4 @@ -import type { MultipartFile } from '@adonisjs/bodyparser/types' +import type { FileValidationOptions, MultipartFile } from '@adonisjs/bodyparser/types' import type { ChatCompletionMessageParam } from 'openai/resources/chat/completions.js' import sharp from 'sharp' @@ -8,7 +8,29 @@ export const MAX_CHAT_IMAGE_PIXELS = 40_000_000 export const MAX_NORMALIZED_IMAGE_BYTES = 4 * 1024 * 1024 export const MAX_NORMALIZED_IMAGE_DIMENSION = 2048 +/** + * How the upload parser reads the `images` field: by size only. Its own + * extension check compares case-sensitively and refuses names like + * IMG_0001.JPG, so the extension is checked in normalizeChatImage instead. + */ +export const CHAT_IMAGE_UPLOAD_OPTIONS: Partial = { + size: MAX_CHAT_IMAGE_BYTES, +} + const SUPPORTED_IMAGE_FORMATS = new Set(['jpeg', 'png', 'webp']) +const SUPPORTED_IMAGE_EXTENSIONS = new Set(['jpg', 'jpeg', 'png', 'webp']) + +/** + * The extension list the parser used to check, now compared without regard to + * case. The parser's extname is the type it read from the file's bytes when the + * name has no extension or a lower-case one it recognizes, and the extension as + * written otherwise, so IMG_0001.JPG arrives as "JPG". A file with no extname is + * refused. + */ +function hasSupportedExtension(file: MultipartFile) { + const extension = file.extname?.toLowerCase() + return extension !== undefined && SUPPORTED_IMAGE_EXTENSIONS.has(extension) +} export class ChatImageError extends Error { constructor( @@ -44,7 +66,7 @@ async function normalizeChatImage(file: MultipartFile): Promise MAX_CHAT_IMAGE_BYTES) { throw new ChatImageError(`"${file.clientName}" exceeds the 8 MB per-image limit.`, 413) } - if (!file.isValid) { + if (!file.isValid || !hasSupportedExtension(file)) { throw new ChatImageError(`"${file.clientName}" is not a valid image upload.`, 422) } diff --git a/admin/tests/unit/chat_image_upload.spec.ts b/admin/tests/unit/chat_image_upload.spec.ts new file mode 100644 index 00000000..e0aca6d9 --- /dev/null +++ b/admin/tests/unit/chat_image_upload.spec.ts @@ -0,0 +1,133 @@ +import assert from 'node:assert/strict' +import { randomUUID } from 'node:crypto' +import http from 'node:http' +import type { AddressInfo } from 'node:net' +import { mkdtemp, rm } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import test from 'node:test' +import { BodyParserMiddlewareFactory } from '@adonisjs/core/factories/bodyparser' +import { HttpContextFactory, RequestFactory, ResponseFactory } from '@adonisjs/core/factories/http' +import sharp from 'sharp' +import { + CHAT_IMAGE_UPLOAD_OPTIONS, + ChatImageError, + normalizeChatImages, +} from '../../app/utils/chat_images.js' + +/** + * Sends one file through the real multipart parser, with the options + * OllamaController.chat uses for the `images` field, and then through + * normalizeChatImages. The parser sets a file's extname and isValid, so this + * spec uses it rather than hand-built file objects. + */ +async function sendImage(name: string, bytes: Buffer) { + const directory = await mkdtemp(join(tmpdir(), 'nomad-chat-upload-')) + const middleware = new BodyParserMiddlewareFactory() + .merge({ + multipart: { + autoProcess: true, + limit: '20mb', + convertEmptyStringsToNull: true, + tmpFileName: () => join(directory, randomUUID()), + }, + }) + .create() + + const server = http.createServer((req, res) => { + const request = new RequestFactory().merge({ req, res }).create() + const response = new ResponseFactory().merge({ req, res }).create() + const ctx = new HttpContextFactory().merge({ request, response }).create() + middleware + .handle(ctx, async () => { + const files = ctx.request.files('images', CHAT_IMAGE_UPLOAD_OPTIONS) + let status = 200 + let message = 'accepted' + try { + const images = await normalizeChatImages(files) + message = `accepted ${images.length}` + } catch (error) { + if (!(error instanceof ChatImageError)) throw error + status = error.status + message = error.message + } + res.writeHead(status, { 'content-type': 'application/json' }) + res.end(JSON.stringify({ message })) + }) + .catch((error) => { + // Answer anyway, or fetch waits for a reply that never comes and the run hangs. + res.writeHead(500, { 'content-type': 'application/json' }) + res.end(JSON.stringify({ message: String(error) })) + }) + }) + await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)) + + try { + const form = new FormData() + form.append('images', new File([new Uint8Array(bytes)], name)) + const { port } = server.address() as AddressInfo + const reply = await fetch(`http://127.0.0.1:${port}/`, { + method: 'POST', + body: form, + signal: AbortSignal.timeout(10_000), + }) + const { message } = (await reply.json()) as { message: string } + return { status: reply.status, message } + } finally { + server.closeAllConnections() + await new Promise((resolve) => server.close(resolve)) + await rm(directory, { recursive: true, force: true }) + } +} + +const picture = () => + sharp({ create: { width: 32, height: 24, channels: 3, background: '#556b2f' } }) + +test('accepts an image whatever the case of its file extension', async () => { + const jpeg = await picture().jpeg().toBuffer() + const png = await picture().png().toBuffer() + const webp = await picture().webp().toBuffer() + const named: Array<[string, Buffer]> = [ + ['IMG_0001.jpg', jpeg], + ['IMG_0001.JPG', jpeg], + ['IMG_0001.Jpeg', jpeg], + ['Screenshot.PNG', png], + ['PIC.WEBP', webp], + ] + + for (const [name, bytes] of named) { + const result = await sendImage(name, bytes) + assert.equal(result.status, 200, `${name}: ${result.message}`) + assert.equal(result.message, 'accepted 1', name) + } +}) + +test('refuses what is not a JPEG, PNG or WebP', async () => { + const jpeg = await picture().jpeg().toBuffer() + const gif = await picture().gif().toBuffer() + + const text = Buffer.from('this is not an image') + const refused: Array<[string, Buffer, number, string]> = [ + // The parser reads the type from the bytes when the extension is a lower-case + // one it knows, such as jpg, so this GIF arrives as "gif". + ['photo.jpg', gif, 422, 'a GIF named .jpg'], + // A name outside the list is refused even when the bytes are a JPEG. + ['photo.txt', jpeg, 422, 'a JPEG named .txt'], + // The parser does not read the bytes behind an upper-case name, so sharp decides. + ['photo.JPG', gif, 415, 'a GIF named .JPG'], + ['notes.JPG', text, 422, 'text named .JPG'], + ] + + for (const [name, bytes, status, what] of refused) { + const result = await sendImage(name, bytes) + assert.equal(result.status, status, `${what}: ${result.message}`) + } +}) + +test('refuses a file whose type the parser could not read', async () => { + // Text behind a lower-case name is read from its bytes and not recognized, so the + // parser leaves extname unset. + const result = await sendImage('notes.jpg', Buffer.from('this is not an image')) + assert.equal(result.status, 422) + assert.match(result.message, /not a valid image upload/) +}) diff --git a/admin/tests/unit/chat_images.spec.ts b/admin/tests/unit/chat_images.spec.ts index b2755f42..88d88637 100644 --- a/admin/tests/unit/chat_images.spec.ts +++ b/admin/tests/unit/chat_images.spec.ts @@ -17,11 +17,17 @@ import { visionCapabilityFromLlamaProps, } from '../../app/utils/model_capabilities.js' -function uploadedFile(path: string, size: number, name = 'sample.png'): MultipartFile { +function uploadedFile( + path: string, + size: number, + name = 'sample.png', + extname = 'png' +): MultipartFile { return { tmpPath: path, size, clientName: name, + extname, isValid: true, } as MultipartFile } @@ -72,7 +78,8 @@ test('normalizes image bytes when the filename extension does not match the enco try { const [image] = await normalizeChatImages([ - uploadedFile(path, input.byteLength, 'mislabeled.png'), + // The parser reads the type from the bytes behind a lower-case name. + uploadedFile(path, input.byteLength, 'mislabeled.png', 'webp'), ]) assert.equal(image.name, 'mislabeled.png') assert.match(image.dataUrl, /^data:image\/jpeg;base64,/) diff --git a/admin/tests/unit/ollama_controller_chat.spec.ts b/admin/tests/unit/ollama_controller_chat.spec.ts index 13564682..059adb4b 100644 --- a/admin/tests/unit/ollama_controller_chat.spec.ts +++ b/admin/tests/unit/ollama_controller_chat.spec.ts @@ -56,6 +56,7 @@ test('unknown backend image rejection returns actionable compatibility details o tmpPath: imagePath, size: image.byteLength, clientName: 'sample.png', + extname: 'png', isValid: true, }, ], @@ -137,6 +138,7 @@ test('unknown backend image rejection returns actionable compatibility details w tmpPath: imagePath, size: image.byteLength, clientName: 'sample.png', + extname: 'png', isValid: true, }, ], @@ -227,6 +229,7 @@ test('unknown backend image requests do not mislabel preprocessing failures as i tmpPath: imagePath, size: image.byteLength, clientName: 'sample.png', + extname: 'png', isValid: true, }, ],