From 6901e2661ed74e69f19c52ce046ee911b818d463 Mon Sep 17 00:00:00 2001 From: Marian Date: Fri, 15 May 2026 19:40:03 +0000 Subject: [PATCH 1/3] feat(api/v1): accept category_id on POST /events/:id/photos The v1 photo upload endpoint previously ignored any caller-supplied category and inserted photos with category_id=NULL. That meant programmatic uploads via API tokens (e.g. a photobox sidecar) landed in picpeak as uncategorized, forcing operators to bulk-assign category in the admin UI after each event. Mirror the adminPhotos.js category-handling logic on v1: - Read optional `category_id` from the multipart form body. - Reject unknown ids with 400 (with the id in the error) so callers fail fast on misconfigured envs instead of silently uncategorized uploads. - Set photos.category_id on insert. - Flip photos.type to 'collage' when the category's slug is collage/collages, matching adminPhotos. Backwards-compatible: omitting category_id keeps the prior behavior (insert with NULL category, type='individual'). OpenAPI spec + 201 response body updated to include the new field. Co-Authored-By: Claude Opus 4.7 (1M context) --- backend/src/routes/v1/events.js | 39 +++++++++++++++++++++++++++++++-- 1 file changed, 37 insertions(+), 2 deletions(-) diff --git a/backend/src/routes/v1/events.js b/backend/src/routes/v1/events.js index 06ad6fcb..2de46fd6 100644 --- a/backend/src/routes/v1/events.js +++ b/backend/src/routes/v1/events.js @@ -338,6 +338,13 @@ router.get('/events/:id', apiTokenAuth, requireApiScope('read'), async (req, res * required: [photo] * properties: * photo: { type: string, format: binary } + * category_id: + * type: integer + * description: | + * Optional. If provided, the photo is filed under the + * given photo_categories.id (must belong to the event + * or be a global category). If omitted, the photo + * lands uncategorized. * responses: * 201: * description: Photo uploaded @@ -351,6 +358,7 @@ router.get('/events/:id', apiTokenAuth, requireApiScope('read'), async (req, res * path: { type: string } * thumbnail_path: { type: string, nullable: true } * size_bytes: { type: integer } + * category_id: { type: integer, nullable: true } * 400: { description: No file or invalid type } * 404: { description: Event not found } */ @@ -368,6 +376,25 @@ router.post( const event = await db('events').where({ id: req.params.id }).first(); if (!event) return res.status(404).json({ error: 'Event not found' }); + // Optional category assignment, mirroring the admin upload route + // (adminPhotos.js). Multipart form field `category_id`. If the + // category looks up to a "collage" slug, the photo's `type` flips + // accordingly so existing collage-aware UI paths still work. + const rawCategoryId = req.body?.category_id; + const parsedCategoryId = rawCategoryId ? parseInt(rawCategoryId, 10) : NaN; + let categoryId = null; + let photoType = 'individual'; + if (!Number.isNaN(parsedCategoryId)) { + const category = await db('photo_categories').where({ id: parsedCategoryId }).first(); + if (!category) { + return res.status(400).json({ error: `Unknown category_id ${parsedCategoryId}` }); + } + categoryId = category.id; + if (category.slug === 'collage' || category.slug === 'collages') { + photoType = 'collage'; + } + } + const ext = path.extname(req.file.originalname); const finalName = `${Date.now()}_${crypto.randomBytes(4).toString('hex')}${ext}`; // photo.path is stored relative to events/active so resolvePhotoStorageKey @@ -408,7 +435,8 @@ router.post( original_filename: req.file.originalname, path: relPath, thumbnail_path: thumbRel, - type: 'individual', + type: photoType, + category_id: categoryId, size_bytes: stat.size, width, height, @@ -432,7 +460,14 @@ router.post( }); } catch (e) { /* non-fatal */ } - res.status(201).json({ id, filename: finalName, path: relPath, thumbnail_path: thumbRel, size_bytes: stat.size }); + res.status(201).json({ + id, + filename: finalName, + path: relPath, + thumbnail_path: thumbRel, + size_bytes: stat.size, + category_id: categoryId + }); } catch (error) { logger.error('v1 POST /events/:id/photos failed', { error: error.message }); if (tempPath) await fs.unlink(tempPath).catch(() => {}); From 92bb9e1a12f77ce5e8c1286716198362b2bfdff2 Mon Sep 17 00:00:00 2001 From: Marian Date: Sat, 16 May 2026 18:20:41 +0000 Subject: [PATCH 2/3] fix(api/v1): scope category lookup to event_owned or global MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR review pointed out the original lookup db('photo_categories').where({ id: parsedCategoryId }).first() accepted any category id — including one that belongs to a different event. photo_categories carries both event_id (per-event) and is_global (see backend/migrations/legacy/004_add_categories_and_cms.js); the v1 upload route should require either match. Not a privilege issue (apiTokenAuth.js inherits the admin's powers, no per-event scoping), but it lets a misconfigured uploader silently file photos under a category the target event doesn't own — and the 201 echo includes a category_id that makes no semantic sense. Tighten to: .where({ id: parsedCategoryId }) .andWhere(function () { this.where({ event_id: event.id }).orWhere('is_global', true); }) …and update the 400 message to "Unknown or out-of-scope category_id N". OpenAPI description already documents the intended scope. Tests deferred to a follow-up; v1 has no jest harness today, see PR discussion. Co-Authored-By: Claude Opus 4.7 (1M context) --- backend/src/routes/v1/events.js | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/backend/src/routes/v1/events.js b/backend/src/routes/v1/events.js index 2de46fd6..78c68f94 100644 --- a/backend/src/routes/v1/events.js +++ b/backend/src/routes/v1/events.js @@ -385,9 +385,22 @@ router.post( let categoryId = null; let photoType = 'individual'; if (!Number.isNaN(parsedCategoryId)) { - const category = await db('photo_categories').where({ id: parsedCategoryId }).first(); + // Scope to categories owned by this event (event_id = event.id) or + // marked global (is_global = true) — see migration + // backend/migrations/legacy/004_add_categories_and_cms.js. An API + // token inherits its owning admin's powers (no per-event scoping + // in apiTokenAuth), so accepting any category_id would silently + // mis-file uploads under a category belonging to a different event. + const category = await db('photo_categories') + .where({ id: parsedCategoryId }) + .andWhere(function () { + this.where({ event_id: event.id }).orWhere('is_global', true); + }) + .first(); if (!category) { - return res.status(400).json({ error: `Unknown category_id ${parsedCategoryId}` }); + return res.status(400).json({ + error: `Unknown or out-of-scope category_id ${parsedCategoryId}`, + }); } categoryId = category.id; if (category.slug === 'collage' || category.slug === 'collages') { From df83b3e923f0b8c8cdb7ca03cb9fd28baa1d8840 Mon Sep 17 00:00:00 2001 From: Marian Date: Sat, 16 May 2026 20:32:48 +0000 Subject: [PATCH 3/3] test(api/v1): cover category scoping clause + 400 response MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Unit test for the v1 upload route's category lookup, requested in the PR review. Mocks db (chainable, mirroring src/routes/__tests__/ adminAuth.test.js) plus apiTokenAuth/requireApiScope (pass-through) and multer (stub req.file). Two cases: 1. The scoping clause: the andWhere callback applied to a knex builder spy produces .where({event_id: }).orWhere( 'is_global', true) — exactly the contract the reviewer asked for, exercising the OR-clause rather than just asserting the callback was passed. 2. Null lookup result yields 400 with "Unknown or out-of-scope category_id ". No v1 jest scaffolding existed before, but the project-wide harness (backend/jest.config.js + jest.setup.js) already covers the new file via testMatch '**/__tests__/**/*.test.js'. Happy-path tests deferred — would require stubbing fs/sharp/imageProcessor/share linkService and several more db chains, which the reviewer was willing to accept as a separate follow-up. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../v1/__tests__/events.category.test.js | 147 ++++++++++++++++++ 1 file changed, 147 insertions(+) create mode 100644 backend/src/routes/v1/__tests__/events.category.test.js diff --git a/backend/src/routes/v1/__tests__/events.category.test.js b/backend/src/routes/v1/__tests__/events.category.test.js new file mode 100644 index 00000000..6ea0f7f4 --- /dev/null +++ b/backend/src/routes/v1/__tests__/events.category.test.js @@ -0,0 +1,147 @@ +/** + * Regression test for PR the_luap/picpeak#500. + * + * The v1 upload endpoint POST /events/:id/photos used to accept any + * photo_categories.id, including ones belonging to a different event. + * apiTokenAuth has no per-event scoping, so this let a programmatic + * uploader silently mis-file photos under a category that doesn't + * belong to the target event. + * + * The fix scopes the lookup to (event_id = event.id OR is_global = true) + * — see backend/migrations/legacy/004_add_categories_and_cms.js for the + * photo_categories columns. These tests verify both that the scoping + * clause is exactly that, and that the 400 response carries the new + * "Unknown or out-of-scope category_id" error string. + * + * Pattern lifted from src/routes/__tests__/adminAuth.test.js. + */ + +const request = require('supertest'); +const express = require('express'); + +const buildChain = ({ firstResult, insertResult } = {}) => ({ + where: jest.fn().mockReturnThis(), + andWhere: jest.fn().mockReturnThis(), + orWhere: jest.fn().mockReturnThis(), + select: jest.fn().mockReturnThis(), + first: jest.fn().mockResolvedValue(firstResult), + insert: jest.fn().mockResolvedValue(insertResult ?? [1]), +}); + +jest.mock('../../../database/db', () => { + const dbMock = jest.fn(); + dbMock.raw = jest.fn(); + dbMock.__setImplementations = (...chains) => { + dbMock.mockReset(); + chains.forEach((chain) => { + dbMock.mockImplementationOnce(() => chain); + }); + }; + return { + db: dbMock, + logActivity: jest.fn().mockResolvedValue(undefined), + }; +}); + +jest.mock('../../../middleware/apiTokenAuth', () => ({ + apiTokenAuth: (req, _res, next) => { + req.apiToken = { id: 1, admin_id: 1, scopes: ['write'] }; + req.admin = { id: 1, username: 'token-admin' }; + next(); + }, + requireApiScope: () => (_req, _res, next) => next(), +})); + +// photoUpload is built inside events.js (multer({...})), not imported +// from a shared module. Mock the multer factory so .single(field) +// returns middleware that injects a stub req.file synchronously. +// +// Caveat: `path` points at a file that doesn't exist on disk. The +// current 400-path tests short-circuit before the handler touches the +// filesystem. Any future test that exercises a happy-path category +// match must either create the file under beforeAll() or stub the +// `fs`/`fsSync` modules — otherwise `fsSync.statSync(tempPath)` will +// throw and the test will surface a misleading 500. +jest.mock('multer', () => { + const fakeUpload = { + single: () => (req, _res, next) => { + req.file = { + path: '/tmp/fake-v1-upload.jpg', + originalname: 'fake.jpg', + size: 1, + mimetype: 'image/jpeg', + }; + next(); + }, + }; + const factory = jest.fn(() => fakeUpload); + factory.diskStorage = jest.fn(() => ({})); + return factory; +}); + +const { db } = require('../../../database/db'); +const eventsRouter = require('../events'); + +const buildApp = () => { + const app = express(); + app.use(express.json()); + app.use('/', eventsRouter); + return app; +}; + +describe('v1 POST /events/:id/photos — category scoping', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('scopes the category lookup to event-owned or global rows', async () => { + const eventChain = buildChain({ firstResult: { id: 42, slug: 'wedding-2026' } }); + const categoryChain = buildChain({ firstResult: null }); + db.__setImplementations(eventChain, categoryChain); + + // Use JSON body (express.json parses it before the mocked multer + // middleware runs). The route reads req.body.category_id either + // way — multer would have parsed the field as a string, json sends + // a string too. + // .expect(400) also pins the response status — without it a future + // regression that swallowed the error and returned 500 would still + // satisfy the scoping-call assertions below. + await request(buildApp()) + .post('/events/42/photos') + .send({ category_id: '7' }) + .expect(400); + + // The category lookup chain receives the id filter… + expect(categoryChain.where).toHaveBeenCalledWith({ id: 7 }); + // …and a single andWhere() with the scoping callback. + expect(categoryChain.andWhere).toHaveBeenCalledTimes(1); + const scopingCb = categoryChain.andWhere.mock.calls[0][0]; + expect(typeof scopingCb).toBe('function'); + + // Invoke the callback against a knex-shaped builder spy and verify + // the OR-clause it builds: event_id = 42 OR is_global = true. + const builderSpy = { + where: jest.fn().mockReturnThis(), + orWhere: jest.fn().mockReturnThis(), + }; + scopingCb.call(builderSpy); + expect(builderSpy.where).toHaveBeenCalledWith({ event_id: 42 }); + expect(builderSpy.orWhere).toHaveBeenCalledWith('is_global', true); + }); + + it('returns 400 with out-of-scope error when no category row matches', async () => { + db.__setImplementations( + buildChain({ firstResult: { id: 42, slug: 'wedding-2026' } }), + buildChain({ firstResult: null }), + ); + + const response = await request(buildApp()) + .post('/events/42/photos') + .send({ category_id: '7' }) + .expect(400); + + expect(response.body).toEqual({ + error: 'Unknown or out-of-scope category_id 7', + }); + }); +});