Merge 543ae5671a into 54ee30a449

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

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<FileValidationOptions> = {
+  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<NormalizedChatIm
   if (file.size > 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)
   }
 

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<void>((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/)
+})

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,/)

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