fix(photos): treat category_id 0 as uncategorized instead of storing it

Genuine product bug, found behind the adminPhotos.reference suite (which was
failing for an unrelated reason -- see below).

parseInt('0') is 0 and !isNaN(0) is true, so a '0' category_id was written
literally. photo_categories.id is an increments() column, so 0 can never be a
real category, and every read path already assumes it cannot happen: the list
mapper does `category_id || type` (0 is falsy, renders as uncategorized) and
the list filter explicitly skips '0'. The result was a filter black hole -- the
photo matches no numeric category filter, and misses the "uncategorized"
filter too because that is whereNull(). Displayed as uncategorized, reachable
by nothing.

null rather than a 400: unparseable input ('abc' -> NaN) already falls through
to null, so 400ing on '0' while silently accepting 'abc' would be incoherent,
and '0' is just the HTML <select> shape where the "none" option carries
value="0".

Fixed at all three call sites that share the branch -- PATCH /photos/:photoId,
POST /photos/bulk-update, and the upload route, where the dangling 0 was
written at creation time and the scope-validation guard
(`if (parsedCategoryId && ...)`) skipped on the falsy 0 and let it in
unvalidated. Only the PATCH one was behind the failing test; leaving the other
two would have left the bad state creatable.

The suite's 3 failures were all masked by a fixture gap, not this bug: it
stubs middleware/auth but not middleware/permissions, so requirePermission's
admin_users JOIN roles query hit tables the fixture never creates and every
request 500'd before reaching a handler. Stub it, bring the photos fixture up
to the 7 migrations it had drifted behind, and correct a stale 200 that became
202 when uploads went async in 851744c3.

Known adjacent gap, not fixed (wider than this bug): PATCH and bulk-update
accept any positive category_id with no existence or scope check, unlike the
upload route which validates event_id = X OR is_global per #500/#525 -- so a
photo can be PATCHed into another event's category.

Refs testplan REPORT.md #22 (Part 1.2.01).
This commit is contained in:
Paul Nothaft
2026-09-01 16:30:46 +02:00
parent 18715b5efd
commit 3f6c81a846
2 changed files with 61 additions and 6 deletions
@@ -40,6 +40,15 @@ describe('Admin photos in reference mode', () => {
} }
})); }));
// The routes gained requirePermission() after this fixture was written.
// It resolves the caller's role through admin_users/roles, which this
// minimal schema does not create, so every request died in the RBAC
// lookup before reaching the handler. RBAC is not what this suite is
// about — stub it out the same way adminAuth already is.
jest.doMock('../../src/middleware/permissions', () => ({
requirePermission: () => (_req, _res, next) => next()
}));
jest.doMock('../../src/services/imageProcessor', () => ({ jest.doMock('../../src/services/imageProcessor', () => ({
generateThumbnail: jest.fn().mockResolvedValue('thumbnails/mock-thumb.jpg'), generateThumbnail: jest.fn().mockResolvedValue('thumbnails/mock-thumb.jpg'),
ensureThumbnail: jest.fn() ensureThumbnail: jest.fn()
@@ -98,8 +107,21 @@ describe('Admin photos in reference mode', () => {
table.string('type').notNullable(); table.string('type').notNullable();
table.integer('size_bytes'); table.integer('size_bytes');
table.integer('category_id'); table.integer('category_id');
table.string('source_origin'); // Mirrors migration 041: the upload route never writes this column, it
// relies on the NOT NULL DEFAULT 'managed' to mark managed originals.
table.string('source_origin').notNullable().defaultTo('managed');
table.string('external_relpath'); table.string('external_relpath');
// Columns the upload insert writes (migrations 048, 062, 071, 085, 193)
// and the PATCH handler writes (migration 178). Without them the insert
// and the update both fail on "no such column".
table.string('original_filename', 512);
table.string('source_filename', 255);
table.datetime('captured_at').nullable();
table.string('media_type').defaultTo('image');
table.string('mime_type');
table.string('processing_status', 16).notNullable().defaultTo('complete');
table.string('upload_id', 64).nullable();
table.boolean('auto_categorized');
table.datetime('uploaded_at').defaultTo(db.fn.now()); table.datetime('uploaded_at').defaultTo(db.fn.now());
table.float('average_rating').defaultTo(0); table.float('average_rating').defaultTo(0);
table.integer('like_count').defaultTo(0); table.integer('like_count').defaultTo(0);
@@ -153,7 +175,10 @@ describe('Admin photos in reference mode', () => {
.field('category_id', String(categoryId)) .field('category_id', String(categoryId))
.attach('photos', Buffer.from('fake image data'), 'photo.jpg'); .attach('photos', Buffer.from('fake image data'), 'photo.jpg');
expect(uploadResponse.status).toBe(200); // 202 Accepted since the upload route went async (851744c3): the files are
// stored and a pending row is inserted, thumbnails/EXIF follow in the
// background worker. This assertion still said 200 from before that.
expect(uploadResponse.status).toBe(202);
expect(uploadResponse.body).toHaveProperty('photos'); expect(uploadResponse.body).toHaveProperty('photos');
expect(Array.isArray(uploadResponse.body.photos)).toBe(true); expect(Array.isArray(uploadResponse.body.photos)).toBe(true);
@@ -203,5 +228,24 @@ describe('Admin photos in reference mode', () => {
const updated = await db('photos').where({ id: photo.id }).first(); const updated = await db('photos').where({ id: photo.id }).first();
expect(updated.category_id).toBeNull(); expect(updated.category_id).toBeNull();
// A real id still round-trips — the '0' guard must not swallow it.
await request(app)
.patch(`/api/admin/events/1/photos/${photo.id}`)
.send({ category_id: String(categoryId) })
.expect(200);
expect((await db('photos').where({ id: photo.id }).first()).category_id).toBe(categoryId);
// Numeric 0 and unparseable input clear the category too, rather than
// writing a category id that can never exist.
for (const value of [0, 'not-a-category']) {
await request(app)
.patch(`/api/admin/events/1/photos/${photo.id}`)
.send({ category_id: value })
.expect(200);
expect((await db('photos').where({ id: photo.id }).first()).category_id).toBeNull();
await db('photos').where({ id: photo.id }).update({ category_id: categoryId });
}
}); });
}); });
+15 -4
View File
@@ -243,8 +243,11 @@ router.post('/:eventId/upload', adminAuth, requirePermission('photos.upload'), r
} }
// Parse category_id to number if provided (handle string values like 'individual', 'collage') // Parse category_id to number if provided (handle string values like 'individual', 'collage')
// Same 0-is-not-a-category rule as the PATCH route below: '0' is truthy, so
// it parsed to 0 and the scope-validation guard (`if (parsedCategoryId && ...)`)
// then skipped on the falsy 0 and let it into the insert unvalidated.
const rawParsed = category_id ? parseInt(category_id, 10) : NaN; const rawParsed = category_id ? parseInt(category_id, 10) : NaN;
const parsedCategoryId = !isNaN(rawParsed) ? rawParsed : null; const parsedCategoryId = rawParsed > 0 ? rawParsed : null;
// Determine photo type and category name // Determine photo type and category name
let photoType = 'individual'; // default let photoType = 'individual'; // default
@@ -846,9 +849,16 @@ router.patch('/:eventId/photos/:photoId', adminAuth, requirePermission('photos.e
// Explicitly clear category // Explicitly clear category
updateData.category_id = null; updateData.category_id = null;
} else { } else {
// Handle numeric category IDs from photo_categories table // Handle numeric category IDs from photo_categories table.
// 0 and negatives mean "no category", not category zero: photo_categories.id
// is an increments() column so it starts at 1, and a <select> whose "none"
// option carries value="0" is exactly how '0' reaches this route. Storing 0
// left the photo in a black hole — the grid's category filters never match
// it, and the "uncategorized" filter is whereNull() so it misses it too,
// while the list mapper renders it as uncategorized because 0 is falsy.
// NaN (unparseable input) already fell through to null and still does.
const numericCategoryId = parseInt(category_id, 10); const numericCategoryId = parseInt(category_id, 10);
if (!isNaN(numericCategoryId)) { if (numericCategoryId > 0) {
updateData.category_id = numericCategoryId; updateData.category_id = numericCategoryId;
} else { } else {
updateData.category_id = null; updateData.category_id = null;
@@ -1031,8 +1041,9 @@ router.post('/:eventId/photos/bulk-update', adminAuth, requirePermission('photos
updateData.category_id = null; updateData.category_id = null;
} else { } else {
// Handle numeric category IDs from photo_categories table // Handle numeric category IDs from photo_categories table
// (0/negative mean "no category" — see the PATCH route above)
const numericCategoryId = parseInt(updates.category_id, 10); const numericCategoryId = parseInt(updates.category_id, 10);
if (!isNaN(numericCategoryId)) { if (numericCategoryId > 0) {
updateData.category_id = numericCategoryId; updateData.category_id = numericCategoryId;
} else { } else {
updateData.category_id = null; updateData.category_id = null;