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, }, ],