From e6510c0317aeb33d832cab5958a40d276acb0f9e Mon Sep 17 00:00:00 2001 From: FragginWagon Date: Fri, 11 Sep 2026 00:35:07 -0400 Subject: [PATCH] fix: inspect uploaded image dimensions on the server --- apps/api/src/app/server.js | 2 + .../images/sharp-image-inspector.js | 18 ++++++++ .../inventory/routes/admin-image-routes.js | 6 +-- .../services/inventory-image-service.js | 15 +++++- .../application/image-inspection.test.js | 46 +++++++++++++++++++ .../inventory-image-service.test.js | 5 ++ .../http/image-upload-inspection.test.js | 41 +++++++++++++++++ docs/API.md | 11 ++++- docs/IMAGES.md | 24 +++++++++- docs/IMPLEMENTATION-STATUS.md | 13 ++++-- packages/validation/image.js | 2 + 11 files changed, 170 insertions(+), 13 deletions(-) create mode 100644 apps/api/src/infrastructure/images/sharp-image-inspector.js create mode 100644 apps/api/tests/application/image-inspection.test.js create mode 100644 apps/api/tests/http/image-upload-inspection.test.js diff --git a/apps/api/src/app/server.js b/apps/api/src/app/server.js index 958cf3a..5bc630e 100644 --- a/apps/api/src/app/server.js +++ b/apps/api/src/app/server.js @@ -1,3 +1,4 @@ +import { createSharpImageInspector } from "../infrastructure/images/sharp-image-inspector.js"; import dotenv from "dotenv"; import { fileURLToPath } from "node:url"; @@ -193,6 +194,7 @@ async function startServer() { publicImageService, }), inventoryImageService: createInventoryImageService({ + imageInspector: createSharpImageInspector(), imageStorage: createLocalImageStorage(imageStorageRoot), imageDerivativeJobRepository, inventoryImageRepository, diff --git a/apps/api/src/infrastructure/images/sharp-image-inspector.js b/apps/api/src/infrastructure/images/sharp-image-inspector.js new file mode 100644 index 0000000..17c05fd --- /dev/null +++ b/apps/api/src/infrastructure/images/sharp-image-inspector.js @@ -0,0 +1,18 @@ +import sharp from 'sharp'; + +const options = { failOn: 'warning', limitInputPixels: 100_000_000 }; +const mimeTypes = { jpeg: 'image/jpeg', png: 'image/png', webp: 'image/webp' }; + +export function createSharpImageInspector() { + return { + async readMetadata(data) { + const metadata = await sharp(data, options).metadata(); + return { width: metadata.width, height: metadata.height, mimeType: mimeTypes[metadata.format] }; + }, + async validateDecoding(data) { + // Decode only after the service has checked metadata limits. The tiny + // output is discarded; originals and derivatives retain their own flow. + await sharp(data, options).resize(1, 1).raw().toBuffer(); + }, + }; +} diff --git a/apps/api/src/modules/inventory/routes/admin-image-routes.js b/apps/api/src/modules/inventory/routes/admin-image-routes.js index 64d7400..44f5bb8 100644 --- a/apps/api/src/modules/inventory/routes/admin-image-routes.js +++ b/apps/api/src/modules/inventory/routes/admin-image-routes.js @@ -1,5 +1,5 @@ import { toAdminInventoryImageDto } from '@pokemon-inventory/contracts/image'; -import { createInventoryImageSchema, moveInventoryImageSchema, updateImageOrderSchema, updateImageAltTextSchema } from '@pokemon-inventory/validation/image'; +import { createInventoryImageSchema, uploadInventoryImageSchema, moveInventoryImageSchema, updateImageOrderSchema, updateImageAltTextSchema } from '@pokemon-inventory/validation/image'; import { randomUUID } from 'node:crypto'; import { createApplicationError } from '../../../shared/errors/application-error.js'; @@ -42,13 +42,11 @@ export async function registerAdminImageRoutes(app, options) { const data = await part.toBuffer(); const fields = part.fields; const originalFilename = part.filename; - const result = createInventoryImageSchema.safeParse({ + const result = uploadInventoryImageSchema.safeParse({ storageKey: `inventory/${request.params.id}/${randomUUID()}`, originalFilename, mimeType: part.mimetype, fileSize: data.length, - width: Number(fields.width?.value), - height: Number(fields.height?.value), altText: fields.altText?.value, sortOrder: fields.sortOrder ? Number(fields.sortOrder.value) : undefined, isPrimary: fields.isPrimary?.value === 'true', diff --git a/apps/api/src/modules/inventory/services/inventory-image-service.js b/apps/api/src/modules/inventory/services/inventory-image-service.js index e11d48c..c6b6fa2 100644 --- a/apps/api/src/modules/inventory/services/inventory-image-service.js +++ b/apps/api/src/modules/inventory/services/inventory-image-service.js @@ -2,7 +2,7 @@ import { validateImageMove } from '../domain/image-order.js'; import { createApplicationError } from '../../../shared/errors/application-error.js'; import { normalizeImageAltText, validateImageSortOrder, validateImageSignature, validateInventoryImageMetadata } from '../domain/inventory-image.js'; -export function createInventoryImageService({ inventoryImageRepository, imageDerivativeJobRepository, imageStorage }) { +export function createInventoryImageService({ inventoryImageRepository, imageDerivativeJobRepository, imageStorage, imageInspector }) { return { async create(input) { validateInventoryImageMetadata(input); @@ -30,8 +30,19 @@ export function createInventoryImageService({ inventoryImageRepository, imageDer async upload(input) { const { data, ...metadata } = input; - validateInventoryImageMetadata(metadata); validateImageSignature(data, metadata.mimeType); + // Bound work before decoding; client dimensions and byte counts are not authoritative. + validateInventoryImageMetadata({ ...metadata, fileSize: data.length, width: 1, height: 1 }); + let inspected; + try { inspected = await imageInspector.readMetadata(data); } + catch { throw createApplicationError('IMAGE_DECODE_INVALID', 'Image could not be decoded.', 400); } + if (inspected.mimeType !== metadata.mimeType) { + throw createApplicationError('IMAGE_SIGNATURE_INVALID', 'Image format does not match the declared MIME type.', 400); + } + Object.assign(metadata, { width: inspected.width, height: inspected.height, fileSize: data.length }); + validateInventoryImageMetadata(metadata); + try { await imageInspector.validateDecoding(data); } + catch { throw createApplicationError('IMAGE_DECODE_INVALID', 'Image could not be decoded.', 400); } await imageStorage.put({ key: metadata.storageKey, data }); diff --git a/apps/api/tests/application/image-inspection.test.js b/apps/api/tests/application/image-inspection.test.js new file mode 100644 index 0000000..270fe1e --- /dev/null +++ b/apps/api/tests/application/image-inspection.test.js @@ -0,0 +1,46 @@ +import assert from 'node:assert/strict'; +import test from 'node:test'; +import sharp from 'sharp'; +import { createSharpImageInspector } from '../../src/infrastructure/images/sharp-image-inspector.js'; +import { createInventoryImageService } from '../../src/modules/inventory/services/inventory-image-service.js'; + +function createService() { + const writes = []; + return { writes, service: createInventoryImageService({ + imageInspector: createSharpImageInspector(), + imageStorage: { async put(input) { writes.push(input); }, async delete() {} }, + inventoryImageRepository: { async createForSeller(input) { return { id: 'image-1', ...input }; } }, + }) }; +} +const metadata = { inventoryItemId: 'item-1', sellerId: 'seller-1', storageKey: 'test', originalFilename: 'test.png', width: 9999, height: 9999, fileSize: 1 }; + +test('inspects JPEG, PNG and WebP bytes and stores their true dimensions and size', async () => { + for (const [format, mimeType] of [['jpeg', 'image/jpeg'], ['png', 'image/png'], ['webp', 'image/webp']]) { + const data = await sharp({ create: { width: 12, height: 8, channels: 3, background: '#123456' } }).toFormat(format).toBuffer(); + const { service, writes } = createService(); + const image = await service.upload({ ...metadata, data, mimeType }); + assert.equal(image.width, 12); + assert.equal(image.height, 8); + assert.equal(image.fileSize, data.length); + assert.equal(writes.length, 1); + } +}); + +test('rejects actual oversized dimensions, signature-only and truncated images before storage', async () => { + const oversized = await sharp({ create: { width: 10001, height: 1, channels: 3, background: 'white' } }).png().toBuffer(); + const valid = await sharp({ create: { width: 12, height: 8, channels: 3, background: 'white' } }).png().toBuffer(); + const incompletePixels = valid.subarray(0, valid.length - 15); + assert.equal((await createSharpImageInspector().readMetadata(incompletePixels)).width, 12); + for (const [data, code] of [[incompletePixels, 'IMAGE_DECODE_INVALID'], [oversized, 'IMAGE_DIMENSIONS_INVALID'], [Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]), 'IMAGE_DECODE_INVALID'], [valid.subarray(0, 40), 'IMAGE_DECODE_INVALID']]) { + const { service, writes } = createService(); + await assert.rejects(() => service.upload({ ...metadata, width: 1, height: 1, data, mimeType: 'image/png' }), { code }); + assert.deepEqual(writes, []); + } +}); + +test('checks true byte length before image inspection', async () => { + const data = Buffer.alloc(10 * 1024 * 1024 + 1); + Buffer.from([0xff, 0xd8, 0xff]).copy(data); + const service = createInventoryImageService({ imageInspector: { async readMetadata() { assert.fail('oversized input decoded'); } } }); + await assert.rejects(() => service.upload({ ...metadata, data, mimeType: 'image/jpeg' }), { code: 'IMAGE_FILE_TOO_LARGE' }); +}); diff --git a/apps/api/tests/application/inventory-image-service.test.js b/apps/api/tests/application/inventory-image-service.test.js index 2830d28..64f3741 100644 --- a/apps/api/tests/application/inventory-image-service.test.js +++ b/apps/api/tests/application/inventory-image-service.test.js @@ -19,6 +19,7 @@ test('stores validated upload bytes before persisting image metadata', async () const stored = []; const jobs = []; const service = createInventoryImageService({ + imageInspector: { async readMetadata() { return { width: 2, height: 3, mimeType: 'image/jpeg' }; }, async validateDecoding() {} }, imageStorage: { async put(value) { stored.push(value); }, async delete() {}, @@ -38,6 +39,9 @@ test('stores validated upload bytes before persisting image metadata', async () assert.equal(stored.length, 1); assert.deepEqual(stored[0], { key: input.storageKey, data: input.data }); assert.equal(image.id, 'image-1'); + assert.equal(image.width, 2); + assert.equal(image.height, 3); + assert.equal(image.fileSize, input.data.length); assert.equal(image.processingStatus, 'PENDING'); assert.equal('data' in image, false); assert.deepEqual(jobs, [{ @@ -50,6 +54,7 @@ test('stores validated upload bytes before persisting image metadata', async () test('removes stored bytes when metadata persistence fails', async () => { const deleted = []; const service = createInventoryImageService({ + imageInspector: { async readMetadata() { return { width: 2, height: 3, mimeType: 'image/jpeg' }; }, async validateDecoding() {} }, imageStorage: { async put() {}, async delete(key) { deleted.push(key); }, diff --git a/apps/api/tests/http/image-upload-inspection.test.js b/apps/api/tests/http/image-upload-inspection.test.js new file mode 100644 index 0000000..2493647 --- /dev/null +++ b/apps/api/tests/http/image-upload-inspection.test.js @@ -0,0 +1,41 @@ +import assert from 'node:assert/strict'; +import test from 'node:test'; +import sharp from 'sharp'; +import { buildApp } from '../../src/app/build-app.js'; +import { createInventoryImageService } from '../../src/modules/inventory/services/inventory-image-service.js'; +import { createSharpImageInspector } from '../../src/infrastructure/images/sharp-image-inspector.js'; + +test('multipart uploads derive dimensions and reject malformed bytes before writes', async (context) => { + const saved = []; + let stored = 0; + let jobs = 0; + const app = buildApp({ + authenticateAdmin: async () => ({ sellerId: 'seller-1' }), + inventoryImageService: createInventoryImageService({ + imageInspector: createSharpImageInspector(), + imageStorage: { async put() { stored += 1; }, async delete() {} }, + inventoryImageRepository: { async createForSeller(input, sellerId) { assert.equal(sellerId, 'seller-1'); saved.push(input); return { id: 'image-1', ...input }; } }, + imageDerivativeJobRepository: { async create() { jobs += 1; } }, + }), + }); + context.after(() => app.close()); + const data = await sharp({ create: { width: 7, height: 5, channels: 3, background: 'white' } }).png().toBuffer(); + const send = (bytes, fields = '') => app.inject({ + method: 'POST', url: '/api/v1/admin/inventory/item-1/images', + headers: { 'content-type': 'multipart/form-data; boundary=image-test' }, + payload: Buffer.concat([Buffer.from(`${fields}--image-test\r\nContent-Disposition: form-data; name="file"; filename="image.png"\r\nContent-Type: image/png\r\n\r\n`), bytes, Buffer.from('\r\n--image-test--\r\n')]), + }); + for (const fields of ['', '--image-test\r\nContent-Disposition: form-data; name="width"\r\n\r\n1\r\n--image-test\r\nContent-Disposition: form-data; name="height"\r\n\r\n1\r\n']) { + const response = await send(data, fields); + assert.equal(response.statusCode, 201); + assert.equal(response.json().data.width, 7); + assert.equal(response.json().data.height, 5); + assert.equal(response.json().data.fileSize, data.length); + } + const response = await send(data.subarray(0, 40)); + assert.equal(response.statusCode, 400); + assert.equal(response.json().error.code, 'IMAGE_DECODE_INVALID'); + assert.equal(saved.length, 2); + assert.equal(stored, 2); + assert.equal(jobs, 2); +}); diff --git a/docs/API.md b/docs/API.md index 606b51d..105bd80 100644 --- a/docs/API.md +++ b/docs/API.md @@ -123,9 +123,10 @@ an available listing to `CLOSED`; pending changes are available from draft or available listings. `POST /api/v1/admin/inventory/:id/images` stores validated image metadata for an -owned physical inventory item. File bytes are handled by the future +owned physical inventory item. File bytes are handled through the `ImageStorage` adapter; JSON metadata and multipart image uploads are accepted. -Multipart uploads require `width` and `height` fields and validate the binary +Multipart uploads derive dimensions from image bytes; supplied `width` and +`height` fields are ignored. They validate the binary signature before storage. Publication counts persisted inventory images rather than trusting a listing request field. @@ -218,3 +219,9 @@ Image update payloads are strict: Malformed, missing, mistyped, oversized, or extra fields return `400 VALIDATION_ERROR` without a persistence write. The service also enforces these constraints for non-HTTP callers. Existing ownership and CSRF checks apply. + + +Multipart image uploads inspect and decode JPEG/PNG/WebP bytes before storage. +Responses contain measured width/height and actual byte size. Invalid decoding +returns `400 IMAGE_DECODE_INVALID`; existing file-size, signature and dimension +limits remain enforced. The JSON metadata-registration path is unchanged. diff --git a/docs/IMAGES.md b/docs/IMAGES.md index 759ac30..42247dc 100644 --- a/docs/IMAGES.md +++ b/docs/IMAGES.md @@ -56,8 +56,10 @@ Validate: The metadata service currently enforces JPEG, PNG, and WebP MIME types, a 10 MiB default file-size limit, and a 10,000-pixel maximum width and height. Multipart upload validation now checks the declared MIME type against JPEG, -PNG, and WebP signatures before storage. Image dimensions are supplied by the -upload transport until a decoder-backed dimension extractor is added. +PNG, and WebP signatures before storage. The injected image inspector reads +dimensions and format from the bytes; the service validates those dimensions +and the actual buffer length, then verifies decoding before writing storage or +metadata. Multipart client dimensions are ignored. Generate: - thumbnail @@ -137,3 +139,21 @@ Explicit empty text or null may clear it; missing fields are invalid. The editor shows the limit, prevents typing past it, and flags oversized pre-existing drafts until corrected. Legacy absolute sort-order updates require a non-negative PostgreSQL integer; ordinary Move controls continue using the atomic move API. + + +## Server-side upload inspection + +The production `ImageInspector` uses the existing Sharp dependency behind +`readMetadata(data)` and `validateDecoding(data)` methods. The service checks +actual byte size before inspection, validates the 10,000-pixel dimension limit +against inspected metadata, and only then requests decoding. Sharp rejects +warnings/corruption and has a 100-million-input-pixel ceiling, matching the +maximum permitted dimension product. The tiny decoded output is discarded; +original storage and asynchronous derivatives retain their existing flow. + +Malformed or undecodable images return `400 IMAGE_DECODE_INVALID`; format mismatch +uses `IMAGE_SIGNATURE_INVALID`, and inspected dimension violations use +`IMAGE_DIMENSIONS_INVALID`. Rejections happen before storage, metadata, or job +creation. No dependency or database migration is required. The separate JSON +metadata-registration endpoint has no uploaded bytes to inspect and retains its +existing required dimensions; multipart uploads no longer require them. diff --git a/docs/IMPLEMENTATION-STATUS.md b/docs/IMPLEMENTATION-STATUS.md index 4062199..13fd183 100644 --- a/docs/IMPLEMENTATION-STATUS.md +++ b/docs/IMPLEMENTATION-STATUS.md @@ -120,7 +120,7 @@ docker context use orbstack | HTTP | Product and inventory creation, validation, authentication/recovery, CSRF, rate limiting, and session management | | Integration | PostgreSQL catalog/session/audit/password recovery persistence and Redis-shared login throttling | -Verified locally: `npm test` passes 174 tests with no failures (24 domain, 81 application, and 69 HTTP); worker tests, web tests, web build, Storybook build, Storybook interaction/accessibility checks, integration tests, and authenticated E2E workflows also pass when their required local services and credentials are available. +Verified locally: `npm test` passes 178 tests with no failures (24 domain, 84 application, and 70 HTTP); worker tests, web tests, web build, Storybook build, Storybook interaction/accessibility checks, integration tests, and authenticated E2E workflows also pass when their required local services and credentials are available. ### Public Seller Description @@ -184,14 +184,21 @@ Verified locally: `npm test` passes 174 tests with no failures (24 domain, 81 ap - Tests cover invalid requests, ownership, CSRF, missing images, boundary moves, tied positions, rollback after the first write, concurrent moves, and UI success/failure recovery. - Validation: 171 API tests, 26 isolated PostgreSQL/Redis integration tests, 11 web tests, production web build, Storybook build, and 32 Storybook suites / 126 tests. No migration required. -### Latest Vertical Slice: Image Update Validation +### Image Update Validation - Legacy order updates require a non-negative PostgreSQL integer; alt-text updates require trimmed text up to 1,000 characters or explicit null. Missing/extra/mistyped fields are rejected before persistence. - Domain validation protects non-HTTP service callers as well. Authentication, CSRF and seller ownership remain unchanged. - The alt-text editor shows its limit, trims saved drafts, prevents oversized input and gives associated accessible feedback for oversized pre-existing text. - Validation: 174 API tests, 26 isolated integration tests, 11 web tests, production web build, Storybook build, and 32 Storybook suites / 128 tests. No migration required. -Next bounded slice: server-derived upload dimensions. `docs/IMAGES.md` identifies that upload dimensions are still supplied by the client. Decode image metadata behind an injected image-inspection interface, enforce dimension limits on actual bytes, and test forged dimensions and malformed images through upload validation. +### Latest Vertical Slice: Server-Derived Upload Dimensions + +- Multipart uploads ignore client dimensions and persist inspected width/height plus actual buffer length. An injected Sharp inspector validates metadata and decoding before storage, metadata writes or job creation. +- Existing MIME signatures, 10 MiB byte limit and 10,000-pixel dimension bounds remain enforced; decoder work is pixel-bounded. JSON metadata registration retains its separate existing contract. +- Tests use actual JPEG/PNG/WebP data and cover forged/missing dimensions, forged byte counts, oversized dimensions, signature-only images and truncated pixel data whose header remains readable. +- Validation: 178 API tests, 26 isolated integration tests, 11 web tests, production web build, Storybook build, and 32 Storybook suites / 128 tests. Existing Sharp dependency reused; no migration or visual changes. + +Next bounded slice: enforce the configured per-item image count limit. `docs/IMAGES.md` requires configurable image counts and Settings exposes the limit, but image creation currently checks only individual-file metadata. Enforce the seller's limit atomically for uploads/metadata registration, reject excess images clearly, and cover concurrent uploads and UI feedback. ## Next Milestone diff --git a/packages/validation/image.js b/packages/validation/image.js index 4f3236f..38f8eee 100644 --- a/packages/validation/image.js +++ b/packages/validation/image.js @@ -22,3 +22,5 @@ export const updateImageOrderSchema = z.object({ export const updateImageAltTextSchema = z.object({ altText: z.string().trim().max(1000).nullable(), }).strict(); + +export const uploadInventoryImageSchema = createInventoryImageSchema.omit({ width: true, height: true });