diff --git a/apps/api/src/modules/inventory/domain/inventory-image.js b/apps/api/src/modules/inventory/domain/inventory-image.js index b9eba29..93a9410 100644 --- a/apps/api/src/modules/inventory/domain/inventory-image.js +++ b/apps/api/src/modules/inventory/domain/inventory-image.js @@ -42,4 +42,17 @@ export function validateImageSignature(data, mimeType) { } return true; -} \ No newline at end of file +} +export function validateImageSortOrder(sortOrder) { + if (!Number.isInteger(sortOrder) || sortOrder < 0 || sortOrder > 2147483647) { + throw createApplicationError('IMAGE_ORDER_INVALID', 'Image order must be an integer between 0 and 2147483647.', 400); + } +} + +export function normalizeImageAltText(altText) { + if (altText === null) return null; + if (typeof altText !== 'string' || altText.trim().length > 1000) { + throw createApplicationError('IMAGE_ALT_TEXT_INVALID', 'Alt text must be text up to 1,000 characters or null.', 400); + } + return altText.trim(); +} 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 f1c63a1..64d7400 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 } from '@pokemon-inventory/validation/image'; +import { createInventoryImageSchema, moveInventoryImageSchema, updateImageOrderSchema, updateImageAltTextSchema } from '@pokemon-inventory/validation/image'; import { randomUUID } from 'node:crypto'; import { createApplicationError } from '../../../shared/errors/application-error.js'; @@ -110,11 +110,13 @@ export async function registerAdminImageRoutes(app, options) { return { data: toAdminInventoryImageDto(image) }; }); - app.patch('/images/:id/order', async (request) => { + app.patch('/images/:id/order', async (request, reply) => { const admin = await authorizeAdminMutation(request); + const result = updateImageOrderSchema.safeParse(request.body); + if (!result.success) return validationError(reply, result); const image = await inventoryImageService.updateOrder({ imageId: request.params.id, - sortOrder: request.body?.sortOrder, + sortOrder: result.data.sortOrder, sellerId: admin.sellerId, }); return { data: toAdminInventoryImageDto(image) }; @@ -129,11 +131,13 @@ export async function registerAdminImageRoutes(app, options) { return { data: toAdminInventoryImageDto(image) }; }); - app.patch('/images/:id/alt-text', async (request) => { + app.patch('/images/:id/alt-text', async (request, reply) => { const admin = await authorizeAdminMutation(request); + const result = updateImageAltTextSchema.safeParse(request.body); + if (!result.success) return validationError(reply, result); const image = await inventoryImageService.updateAltText({ imageId: request.params.id, - altText: request.body?.altText ?? null, + altText: result.data.altText, sellerId: admin.sellerId, }); return { data: toAdminInventoryImageDto(image) }; 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 c3807aa..e11d48c 100644 --- a/apps/api/src/modules/inventory/services/inventory-image-service.js +++ b/apps/api/src/modules/inventory/services/inventory-image-service.js @@ -1,6 +1,6 @@ import { validateImageMove } from '../domain/image-order.js'; import { createApplicationError } from '../../../shared/errors/application-error.js'; -import { validateImageSignature, validateInventoryImageMetadata } from '../domain/inventory-image.js'; +import { normalizeImageAltText, validateImageSortOrder, validateImageSignature, validateInventoryImageMetadata } from '../domain/inventory-image.js'; export function createInventoryImageService({ inventoryImageRepository, imageDerivativeJobRepository, imageStorage }) { return { @@ -49,6 +49,7 @@ export function createInventoryImageService({ inventoryImageRepository, imageDer }, async updateOrder({ imageId, sortOrder, sellerId }) { + validateImageSortOrder(sortOrder); return requireOwnedImage(inventoryImageRepository.updateOrderForSeller(imageId, sortOrder, sellerId)); }, @@ -57,7 +58,7 @@ export function createInventoryImageService({ inventoryImageRepository, imageDer }, async updateAltText({ imageId, altText, sellerId }) { - return requireOwnedImage(inventoryImageRepository.updateAltTextForSeller(imageId, altText, sellerId)); + return requireOwnedImage(inventoryImageRepository.updateAltTextForSeller(imageId, normalizeImageAltText(altText), sellerId)); }, async delete({ imageId, sellerId }) { diff --git a/apps/api/tests/application/inventory-image-management.test.js b/apps/api/tests/application/inventory-image-management.test.js index e5f54e8..d3064bb 100644 --- a/apps/api/tests/application/inventory-image-management.test.js +++ b/apps/api/tests/application/inventory-image-management.test.js @@ -48,3 +48,19 @@ test('moves an image through the owned atomic repository operation', async () => await assert.rejects(() => service.move({ imageId: 'missing', direction: 'down', sellerId: 'seller-1' }), { code: 'IMAGE_NOT_FOUND' }); await assert.rejects(() => service.move({ imageId: 'owned', direction: 'left', sellerId: 'seller-1' }), { code: 'IMAGE_MOVE_INVALID' }); }); + +test('validates image updates before persistence and normalizes valid alt text', async () => { + const { service, calls } = createService(); + for (const sortOrder of [undefined, null, -1, 0.5, '1', NaN, Infinity, 2147483648]) { + await assert.rejects(() => service.updateOrder({ imageId: 'image-1', sortOrder, sellerId: 'seller-1' }), { code: 'IMAGE_ORDER_INVALID' }); + } + for (const altText of [undefined, 123, {}, [], 'x'.repeat(1001)]) { + await assert.rejects(() => service.updateAltText({ imageId: 'image-1', altText, sellerId: 'seller-1' }), { code: 'IMAGE_ALT_TEXT_INVALID' }); + } + assert.deepEqual(calls, []); + await service.updateOrder({ imageId: 'image-1', sortOrder: 2147483647, sellerId: 'seller-1' }); + for (const [input, expected] of [[' Front ', 'Front'], [' ', ''], [null, null], ['x'.repeat(1000), 'x'.repeat(1000)]]) { + const result = await service.updateAltText({ imageId: 'image-1', altText: input, sellerId: 'seller-1' }); + assert.equal(result.altText, expected); + } +}); diff --git a/apps/api/tests/http/image-update-validation.test.js b/apps/api/tests/http/image-update-validation.test.js new file mode 100644 index 0000000..eb8eb33 --- /dev/null +++ b/apps/api/tests/http/image-update-validation.test.js @@ -0,0 +1,51 @@ +import assert from 'node:assert/strict'; +import test from 'node:test'; +import { buildApp } from '../../src/app/build-app.js'; +import { createInventoryImageService } from '../../src/modules/inventory/services/inventory-image-service.js'; + +test('rejects malformed image updates without writes and accepts normalized boundary values', async (context) => { + const writes = []; + const app = buildApp({ + authenticateAdmin: async () => ({ sellerId: 'seller-1' }), + inventoryImageService: createInventoryImageService({ inventoryImageRepository: { + async updateOrderForSeller(id, sortOrder, sellerId) { writes.push({ id, sortOrder, sellerId }); return { id, sortOrder }; }, + async updateAltTextForSeller(id, altText, sellerId) { writes.push({ id, altText, sellerId }); return { id, altText }; }, + } }), + }); + context.after(() => app.close()); + const patch = (path, payload) => app.inject({ method: 'PATCH', url: `/api/v1/admin/images/image-1/${path}`, payload }); + for (const payload of [{}, { sortOrder: -1 }, { sortOrder: 0.2 }, { sortOrder: '1' }, { sortOrder: null }, { sortOrder: 2147483648 }, { sortOrder: 0, sellerId: 'other' }]) { + const response = await patch('order', payload); + assert.equal(response.statusCode, 400); + assert.equal(response.json().error.code, 'VALIDATION_ERROR'); + } + for (const payload of [{}, { altText: 1 }, { altText: {} }, { altText: [] }, { altText: 'x'.repeat(1001) }, { altText: 'Front', private: true }]) { + assert.equal((await patch('alt-text', payload)).statusCode, 400); + } + assert.deepEqual(writes, []); + for (const sortOrder of [0, 2147483647]) assert.equal((await patch('order', { sortOrder })).json().data.sortOrder, sortOrder); + for (const [altText, expected] of [[' Front ', 'Front'], [' ', ''], [null, null], ['x'.repeat(1000), 'x'.repeat(1000)]]) { + const response = await patch('alt-text', { altText }); + assert.equal(response.statusCode, 200); + assert.equal(response.json().data.altText, expected); + } + assert.ok(writes.every(({ sellerId }) => sellerId === 'seller-1')); +}); + +test('image update validation does not bypass authentication or CSRF', async (context) => { + for (const signedIn of [false, true]) { + const app = buildApp({ + authenticateAdmin: async () => signedIn ? { sellerId: 'seller-1' } : null, + authorizeUnsafeRequest: async () => { throw Object.assign(new Error('Forbidden'), { statusCode: 403 }); }, + inventoryImageService: { + async updateOrder() { assert.fail('blocked request reached order service'); }, + async updateAltText() { assert.fail('blocked request reached alt-text service'); }, + }, + }); + context.after(() => app.close()); + for (const path of ['order', 'alt-text']) { + const response = await app.inject({ method: 'PATCH', url: `/api/v1/admin/images/image-1/${path}`, payload: {} }); + assert.equal(response.statusCode, signedIn ? 403 : 401); + } + } +}); diff --git a/apps/web/src/components/organisms/AdminImageManager.stories.js b/apps/web/src/components/organisms/AdminImageManager.stories.js index 9fe84f6..1fb784e 100644 --- a/apps/web/src/components/organisms/AdminImageManager.stories.js +++ b/apps/web/src/components/organisms/AdminImageManager.stories.js @@ -55,3 +55,33 @@ export const DisabledAfterRefreshFailure = { } }, }; + +export const AltTextLimit = { + args: { images: [{ ...images[0], altText: 'card '.repeat(200) }] }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + await userEvent.click(canvas.getByRole('button', { name: 'Edit alt text' })); + const input = canvas.getByLabelText('Alt text'); + await expect(input).toHaveAttribute('maxlength', '1000'); + await expect(input).toHaveAccessibleDescription(/up to 1,000 characters/); + await userEvent.type(input, 'extra'); + await expect(input).toHaveValue('card '.repeat(200)); + await userEvent.clear(input); + await userEvent.type(input, ' Front of card '); + await userEvent.click(canvas.getByRole('button', { name: 'Save alt text' })); + await expect(input).toHaveValue('Front of card'); + }, +}; +export const OversizedExistingAltText = { + args: { images: [{ ...images[0], altText: 'card '.repeat(201) }] }, + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + await userEvent.click(canvas.getByRole('button', { name: 'Edit alt text' })); + await expect(canvas.getByRole('alert')).toHaveTextContent('Alt text must be 1,000 characters or fewer.'); + await expect(canvas.getByLabelText('Alt text')).toHaveAttribute('aria-invalid', 'true'); + await expect(canvas.getByRole('button', { name: 'Save alt text' })).toBeDisabled(); + await userEvent.clear(canvas.getByLabelText('Alt text')); + await expect(canvas.getByRole('button', { name: 'Save alt text' })).toBeEnabled(); + await expect(canvas.queryByRole('alert')).toBeNull(); + }, +}; diff --git a/apps/web/src/components/organisms/AdminImageManager.vue b/apps/web/src/components/organisms/AdminImageManager.vue index 80faa6b..edd0c40 100644 --- a/apps/web/src/components/organisms/AdminImageManager.vue +++ b/apps/web/src/components/organisms/AdminImageManager.vue @@ -23,7 +23,10 @@ function beginEditAltText(image) { } function saveAltText(imageId) { - emit('alt-text', { id: imageId, altText: altTextDrafts[imageId] ?? '' }); + const altText = (altTextDrafts[imageId] ?? '').trim(); + if (props.isSaving || props.disabled || altText.length > 1000) return; + altTextDrafts[imageId] = altText; + emit('alt-text', { id: imageId, altText }); } watch(() => props.images, (images) => { @@ -85,11 +88,14 @@ function handleDrop(event) {
- + - +
+

Describe the image in up to 1,000 characters. Clear the field to remove its alt text.

+ +
diff --git a/docs/API.md b/docs/API.md index daee988..606b51d 100644 --- a/docs/API.md +++ b/docs/API.md @@ -207,3 +207,14 @@ successful no-op. Missing/unowned images return `404 IMAGE_NOT_FOUND`; invalid bodies return `400 VALIDATION_ERROR`. The response uses the existing admin image DTO. The legacy absolute `/order` endpoint remains available, but UI moves use this new operation exclusively. + +Image update payloads are strict: +- `PATCH /api/v1/admin/images/:id/order` requires exactly `sortOrder`, an integer + from 0 through 2147483647 (the stored PostgreSQL integer range). +- `PATCH /api/v1/admin/images/:id/alt-text` requires exactly `altText`, either + trimmed text up to 1,000 characters or explicit `null`. An empty string clears + the text; omission no longer implicitly clears it. + +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. diff --git a/docs/IMAGES.md b/docs/IMAGES.md index ac15d26..759ac30 100644 --- a/docs/IMAGES.md +++ b/docs/IMAGES.md @@ -129,3 +129,11 @@ contiguous zero-based positions, resolving tied legacy/upload positions, within one PostgreSQL transaction. Any failure rolls back all changes. Concurrent moves within one item serialize; primary selection and image metadata are unchanged. Boundary moves are no-ops. No database migration is required. + +## Image update validation + +Alt-text edits use the same trimmed 1,000-character limit as image creation. +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. diff --git a/docs/IMPLEMENTATION-STATUS.md b/docs/IMPLEMENTATION-STATUS.md index e30954c..4062199 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 171 tests with no failures (24 domain, 80 application, and 67 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 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. ### Public Seller Description @@ -177,14 +177,21 @@ Verified locally: `npm test` passes 171 tests with no failures (24 domain, 80 ap - Dark-theme coverage also fixed destructive-button contrast by separating its solid background token from error-text color. - Validation: 167 API tests, 11 web tests, production web build, Storybook build, and 32 Storybook suites / 124 tests. No API or persistence changes. -### Latest Vertical Slice: Atomic Image Reordering +### Atomic Image Reordering - Move up/down now calls a single authenticated, CSRF-protected `POST /api/v1/admin/images/:id/move` operation. The server derives the adjacent image from current persisted order. - The repository locks the owning Inventory Item and its images, applies the domain move rule, and commits contiguous positions in one transaction. Ties are resolved deterministically; boundary moves are no-ops; primary state is preserved. - 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. -Next bounded slice: image mutation payload validation. The legacy absolute-order and alt-text endpoints currently pass unchecked request fields into the service. Add strict schemas and domain validation for these updates, provide clear UI validation feedback, and test malformed/oversized requests without changing valid image workflows. +### Latest Vertical Slice: 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. ## Next Milestone diff --git a/packages/validation/image.js b/packages/validation/image.js index 7f29edf..4f3236f 100644 --- a/packages/validation/image.js +++ b/packages/validation/image.js @@ -14,3 +14,11 @@ export const createInventoryImageSchema = z.object({ export const moveInventoryImageSchema = z.object({ direction: z.enum(['up', 'down']), }).strict(); + +export const updateImageOrderSchema = z.object({ + sortOrder: z.int().min(0).max(2147483647), +}).strict(); + +export const updateImageAltTextSchema = z.object({ + altText: z.string().trim().max(1000).nullable(), +}).strict();