From efc36bce791df6a78a34419fcd4fe2aba78061e5 Mon Sep 17 00:00:00 2001 From: Jake Bromberg Date: Sat, 21 Feb 2026 08:21:10 -0800 Subject: [PATCH 1/6] fix: use authenticated user ID for bin operations instead of client-supplied dj_id All bin endpoints accepted dj_id from the request body/query, enabling any authenticated DJ to read/modify any other DJ's bin (IDOR). Co-authored-by: Cursor --- apps/backend/controllers/djs.controller.ts | 36 +++++------- tests/mocks/database.mock.ts | 9 +++ tests/unit/controllers/djs.bin-idor.test.ts | 64 +++++++++++++++++++++ 3 files changed, 88 insertions(+), 21 deletions(-) create mode 100644 tests/unit/controllers/djs.bin-idor.test.ts diff --git a/apps/backend/controllers/djs.controller.ts b/apps/backend/controllers/djs.controller.ts index 1e160cf..bc6583b 100644 --- a/apps/backend/controllers/djs.controller.ts +++ b/apps/backend/controllers/djs.controller.ts @@ -10,12 +10,12 @@ export type binBody = { }; export const addToBin: RequestHandler = async (req, res, next) => { - if (req.body.album_id === undefined || req.body.dj_id === undefined) { + if (req.body.album_id === undefined) { console.error('Bad Request, Missing Album Identifier: album_id'); - res.status(400).send('Bad Request, Missing DJ or album identifier: album_id'); + res.status(400).send('Bad Request, Missing album identifier: album_id'); } else { const bin_entry: NewBinEntry = { - dj_id: req.body.dj_id, + dj_id: req.auth!.id!, album_id: req.body.album_id, track_title: req.body.track_title === undefined ? null : req.body.track_title, }; @@ -38,13 +38,12 @@ export type binQuery = { }; export const deleteFromBin: RequestHandler = async (req, res, next) => { - if (req.query.album_id === undefined || req.query.dj_id === undefined) { - console.error('Bad Request, Missing Bin Entry Identifier: album_id or dj_id'); - res.status(400).send('Bad Request, Missing Bin Entry Identifier: album_id or dj_id'); + if (req.query.album_id === undefined) { + console.error('Bad Request, Missing Bin Entry Identifier: album_id'); + res.status(400).send('Bad Request, Missing Bin Entry Identifier: album_id'); } else { try { - //check that the dj_id === dj_id of bin entry - const removed_bin_item = await DJService.removeFromBin(parseInt(req.query.album_id), req.query.dj_id); + const removed_bin_item = await DJService.removeFromBin(parseInt(req.query.album_id), req.auth!.id!); res.status(200).json(removed_bin_item); } catch (e) { console.error(e); @@ -53,19 +52,14 @@ export const deleteFromBin: RequestHandler = } }; -export const getBin: RequestHandler = async (req, res, next) => { - if (req.query.dj_id === undefined) { - console.error('Bad Request, Missing DJ Identifier: dj_id'); - res.status(400).send('Bad Request, Missing DJ Identifier: dj_id'); - } else { - try { - const dj_bin = await DJService.getBinFromDB(req.query.dj_id); - res.status(200).json(dj_bin); - } catch (e) { - console.error("Error: Failed to retrieve dj's bin"); - console.error(e); - next(e); - } +export const getBin: RequestHandler = async (req, res, next) => { + try { + const dj_bin = await DJService.getBinFromDB(req.auth!.id!); + res.status(200).json(dj_bin); + } catch (e) { + console.error("Error: Failed to retrieve dj's bin"); + console.error(e); + next(e); } }; diff --git a/tests/mocks/database.mock.ts b/tests/mocks/database.mock.ts index 64a7e21..7e9bfb0 100644 --- a/tests/mocks/database.mock.ts +++ b/tests/mocks/database.mock.ts @@ -86,6 +86,7 @@ export const flowsheet = { message: 'message', add_time: 'add_time', }; +export const bins = {}; export const shows = {}; export const show_djs = {}; export const user = {}; @@ -131,3 +132,11 @@ export type NewFSEntry = Partial; export type Show = Record; export type ShowDJ = Record; export type User = Record; + +export type BinEntry = { + id: number; + dj_id: string; + album_id: number; + track_title: string | null; +}; +export type NewBinEntry = Omit; diff --git a/tests/unit/controllers/djs.bin-idor.test.ts b/tests/unit/controllers/djs.bin-idor.test.ts new file mode 100644 index 0000000..219d927 --- /dev/null +++ b/tests/unit/controllers/djs.bin-idor.test.ts @@ -0,0 +1,64 @@ +import { jest } from '@jest/globals'; +import type { Request, Response, NextFunction } from 'express'; + +jest.mock('../../../apps/backend/services/djs.service', () => ({ + addToBin: jest.fn().mockResolvedValue({ id: 1, dj_id: 'user-A', album_id: 1, track_title: null } as never), + removeFromBin: jest.fn().mockResolvedValue({ id: 1, dj_id: 'user-A', album_id: 1, track_title: null } as never), + getBinFromDB: jest.fn().mockResolvedValue([] as never), +})); + +import * as DJService from '../../../apps/backend/services/djs.service'; +import { addToBin, deleteFromBin, getBin } from '../../../apps/backend/controllers/djs.controller'; + +function mockResponse(): Response { + const res = {} as Response; + res.status = jest.fn().mockReturnValue(res) as unknown as Response['status']; + res.json = jest.fn().mockReturnValue(res) as unknown as Response['json']; + res.send = jest.fn().mockReturnValue(res) as unknown as Response['send']; + return res; +} + +const next: NextFunction = jest.fn(); + +describe('djs.controller bin endpoints – IDOR protection', () => { + describe('addToBin', () => { + it('uses authenticated user ID, not client-supplied dj_id', async () => { + const req = { + auth: { id: 'user-A' }, + body: { dj_id: 'user-B', album_id: 1 }, + } as unknown as Request; + + await addToBin(req, mockResponse(), next); + + expect(DJService.addToBin).toHaveBeenCalledWith( + expect.objectContaining({ dj_id: 'user-A' }) + ); + }); + }); + + describe('deleteFromBin', () => { + it('uses authenticated user ID, not client-supplied dj_id', async () => { + const req = { + auth: { id: 'user-A' }, + query: { dj_id: 'user-B', album_id: '1' }, + } as unknown as Request; + + await deleteFromBin(req, mockResponse(), next); + + expect(DJService.removeFromBin).toHaveBeenCalledWith(1, 'user-A'); + }); + }); + + describe('getBin', () => { + it('uses authenticated user ID, not client-supplied dj_id', async () => { + const req = { + auth: { id: 'user-A' }, + query: { dj_id: 'user-B' }, + } as unknown as Request; + + await getBin(req, mockResponse(), next); + + expect(DJService.getBinFromDB).toHaveBeenCalledWith('user-A'); + }); + }); +}); From 407c2ba8250aa616710dc2b7ce697f4dfc69313c Mon Sep 17 00:00:00 2001 From: Jake Bromberg Date: Fri, 27 Feb 2026 09:46:24 -0800 Subject: [PATCH 2/6] style: format files with Prettier --- tests/unit/controllers/djs.bin-idor.test.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/unit/controllers/djs.bin-idor.test.ts b/tests/unit/controllers/djs.bin-idor.test.ts index 219d927..106be81 100644 --- a/tests/unit/controllers/djs.bin-idor.test.ts +++ b/tests/unit/controllers/djs.bin-idor.test.ts @@ -30,9 +30,7 @@ describe('djs.controller bin endpoints – IDOR protection', () => { await addToBin(req, mockResponse(), next); - expect(DJService.addToBin).toHaveBeenCalledWith( - expect.objectContaining({ dj_id: 'user-A' }) - ); + expect(DJService.addToBin).toHaveBeenCalledWith(expect.objectContaining({ dj_id: 'user-A' })); }); }); From 37cd4ce1cdf1aab9b7538257909190fb97191236 Mon Sep 17 00:00:00 2001 From: Jake Bromberg Date: Fri, 27 Feb 2026 14:28:26 -0800 Subject: [PATCH 3/6] test: update bin integration tests for IDOR fix Bin endpoints now derive dj_id from req.auth.id instead of accepting it from the client. Remove dj_id from request bodies and query params in integration tests, and remove tests for missing dj_id validation since it is no longer a client-supplied parameter. Also update requirePermissions middleware to decode (without verifying) the JWT in AUTH_BYPASS mode so that req.auth.id is still populated for controllers that depend on it. --- shared/authentication/src/auth.middleware.ts | 19 ++++++- tests/integration/djs.spec.js | 58 ++++---------------- 2 files changed, 28 insertions(+), 49 deletions(-) diff --git a/shared/authentication/src/auth.middleware.ts b/shared/authentication/src/auth.middleware.ts index b7c7eb8..43a8871 100644 --- a/shared/authentication/src/auth.middleware.ts +++ b/shared/authentication/src/auth.middleware.ts @@ -1,5 +1,5 @@ import { NextFunction, Request, Response } from 'express'; -import { createRemoteJWKSet, jwtVerify, type JWTPayload } from 'jose'; +import { createRemoteJWKSet, decodeJwt, jwtVerify, type JWTPayload } from 'jose'; import { AccessControlStatement, WXYCRole, WXYCRoles, normalizeRole } from './auth.roles'; // JWT payload structure expected from better-auth JWT plugin @@ -53,6 +53,23 @@ export type RequiredPermissions = { export function requirePermissions(required: RequiredPermissions) { return async (req: Request, res: Response, next: NextFunction) => { if (process.env.AUTH_BYPASS === 'true') { + // In bypass mode, still decode the JWT (without verification) so that + // req.auth is populated for controllers that rely on req.auth.id. + const authHeader = req.headers.authorization; + if (authHeader) { + const token = authHeader.startsWith('Bearer ') ? authHeader.slice(7).trim() : authHeader.trim(); + if (token) { + try { + const payload = decodeJwt(token) as WXYCAuthJwtPayload; + const userId = payload.id || payload.sub; + if (userId) { + req.auth = { ...payload, id: userId } as WXYCAuthJwtPayload; + } + } catch { + // Token may not be a valid JWT in bypass mode; ignore decode errors + } + } + } return next(); } diff --git a/tests/integration/djs.spec.js b/tests/integration/djs.spec.js index 46e0cc3..2f5eaa6 100644 --- a/tests/integration/djs.spec.js +++ b/tests/integration/djs.spec.js @@ -6,9 +6,9 @@ const { createAuthRequest, expectErrorContains, expectFields, expectArray } = re * DJ Endpoints Integration Tests * * Tests for: - * - GET /djs/bin - Retrieve DJ's bin - * - POST /djs/bin - Add entry to bin - * - DELETE /djs/bin - Remove entry from bin + * - GET /djs/bin - Retrieve authenticated DJ's bin + * - POST /djs/bin - Add entry to authenticated DJ's bin + * - DELETE /djs/bin - Remove entry from authenticated DJ's bin * - GET /djs/playlists - Get playlists for a DJ */ @@ -20,20 +20,8 @@ describe('DJ Bin', () => { }); describe('GET /djs/bin', () => { - test('returns array for DJ', async () => { - const res = await auth.get('/djs/bin').query({ dj_id: global.primary_dj_id }).expect(200); - - expectArray(res); - }); - - test('returns 400 when dj_id is missing', async () => { - const res = await auth.get('/djs/bin').expect(400); - - expectErrorContains(res, 'Missing DJ Identifier'); - }); - - test('returns empty array for DJ with no bin entries', async () => { - const res = await auth.get('/djs/bin').query({ dj_id: global.secondary_dj_id }).expect(200); + test('returns array for authenticated DJ', async () => { + const res = await auth.get('/djs/bin').expect(200); expectArray(res); }); @@ -42,28 +30,25 @@ describe('DJ Bin', () => { describe('POST /djs/bin', () => { afterEach(async () => { // Clean up any bin entries created during tests - await auth.delete('/djs/bin').query({ dj_id: global.primary_dj_id, album_id: 1 }); + await auth.delete('/djs/bin').query({ album_id: 1 }); }); test('adds entry to bin successfully', async () => { const res = await auth .post('/djs/bin') .send({ - dj_id: global.primary_dj_id, album_id: 1, }) .expect(200); expectFields(res.body, 'album_id', 'dj_id'); expect(res.body.album_id).toBe(1); - expect(res.body.dj_id).toBe(global.primary_dj_id); }); test('adds entry with track title to bin', async () => { const res = await auth .post('/djs/bin') .send({ - dj_id: global.primary_dj_id, album_id: 1, track_title: 'Carry the Zero', }) @@ -75,23 +60,7 @@ describe('DJ Bin', () => { }); test('returns 400 when album_id is missing', async () => { - const res = await auth - .post('/djs/bin') - .send({ - dj_id: global.primary_dj_id, - }) - .expect(400); - - expectErrorContains(res, 'Missing'); - }); - - test('returns 400 when dj_id is missing', async () => { - const res = await auth - .post('/djs/bin') - .send({ - album_id: 1, - }) - .expect(400); + const res = await auth.post('/djs/bin').send({}).expect(400); expectErrorContains(res, 'Missing'); }); @@ -101,31 +70,24 @@ describe('DJ Bin', () => { beforeEach(async () => { // Set up a bin entry to delete await auth.post('/djs/bin').send({ - dj_id: global.primary_dj_id, album_id: 2, }); }); test('removes entry from bin successfully', async () => { - const res = await auth.delete('/djs/bin').query({ dj_id: global.primary_dj_id, album_id: 2 }).expect(200); + const res = await auth.delete('/djs/bin').query({ album_id: 2 }).expect(200); expect(res.body).toBeDefined(); // Verify it was removed - const binRes = await auth.get('/djs/bin').query({ dj_id: global.primary_dj_id }).expect(200); + const binRes = await auth.get('/djs/bin').expect(200); const entry = binRes.body.find((e) => e.album_id === 2); expect(entry).toBeUndefined(); }); test('returns 400 when album_id is missing', async () => { - const res = await auth.delete('/djs/bin').query({ dj_id: global.primary_dj_id }).expect(400); - - expectErrorContains(res, 'Missing'); - }); - - test('returns 400 when dj_id is missing', async () => { - const res = await auth.delete('/djs/bin').query({ album_id: 2 }).expect(400); + const res = await auth.delete('/djs/bin').expect(400); expectErrorContains(res, 'Missing'); }); From f7ba169e8215c615a9c3cc0ff5bd4d0727ffc400 Mon Sep 17 00:00:00 2001 From: Jake Bromberg Date: Fri, 27 Feb 2026 16:27:18 -0800 Subject: [PATCH 4/6] fix: populate req.auth in AUTH_BYPASS mode for bin endpoints When AUTH_BYPASS=true and the token is not a valid JWT (e.g. integration tests pass a raw user ID string), the middleware now falls back to using the token value as req.auth.id instead of silently leaving req.auth undefined. This prevents 500 errors in bin endpoints that access req.auth!.id!. The integration test setup now passes the primary DJ's user ID as the Bearer token in bypass mode, so the middleware can populate req.auth.id with the correct value for database operations. --- shared/authentication/src/auth.middleware.ts | 7 +++- tests/setup/integration.setup.js | 4 +- .../authentication/auth.middleware.test.ts | 40 +++++++++++++++++-- 3 files changed, 45 insertions(+), 6 deletions(-) diff --git a/shared/authentication/src/auth.middleware.ts b/shared/authentication/src/auth.middleware.ts index 43a8871..a927067 100644 --- a/shared/authentication/src/auth.middleware.ts +++ b/shared/authentication/src/auth.middleware.ts @@ -53,8 +53,10 @@ export type RequiredPermissions = { export function requirePermissions(required: RequiredPermissions) { return async (req: Request, res: Response, next: NextFunction) => { if (process.env.AUTH_BYPASS === 'true') { - // In bypass mode, still decode the JWT (without verification) so that + // In bypass mode, try to decode the JWT (without verification) so that // req.auth is populated for controllers that rely on req.auth.id. + // If the token is not a valid JWT (e.g. integration tests pass a raw + // user ID), fall back to using the token value as the user ID directly. const authHeader = req.headers.authorization; if (authHeader) { const token = authHeader.startsWith('Bearer ') ? authHeader.slice(7).trim() : authHeader.trim(); @@ -66,7 +68,8 @@ export function requirePermissions(required: RequiredPermissions) { req.auth = { ...payload, id: userId } as WXYCAuthJwtPayload; } } catch { - // Token may not be a valid JWT in bypass mode; ignore decode errors + // Token is not a valid JWT -- treat it as a raw user ID. + req.auth = { id: token } as WXYCAuthJwtPayload; } } } diff --git a/tests/setup/integration.setup.js b/tests/setup/integration.setup.js index d8b95c9..fa05596 100644 --- a/tests/setup/integration.setup.js +++ b/tests/setup/integration.setup.js @@ -51,7 +51,9 @@ beforeAll(async () => { await getUserIdsFromDatabase(); if (process.env.AUTH_BYPASS === 'true') { - global.access_token = 'Auth Bypass Enabled'; + // Pass the DJ's user ID as the token so the bypass middleware can + // populate req.auth.id for endpoints that derive identity from the JWT. + global.access_token = `Bearer ${global.primary_dj_id}`; } else { global.access_token = await get_access_token(); } diff --git a/tests/unit/authentication/auth.middleware.test.ts b/tests/unit/authentication/auth.middleware.test.ts index dd4b685..ece3818 100644 --- a/tests/unit/authentication/auth.middleware.test.ts +++ b/tests/unit/authentication/auth.middleware.test.ts @@ -6,17 +6,19 @@ process.env.BETTER_AUTH_ISSUER = 'https://test.example.com'; process.env.BETTER_AUTH_AUDIENCE = 'https://test.example.com'; // Mock jose -- the middleware calls createRemoteJWKSet at module scope -// and jwtVerify on each request +// and jwtVerify on each request. decodeJwt is used in AUTH_BYPASS mode. jest.mock('jose', () => ({ createRemoteJWKSet: jest.fn(() => jest.fn()), jwtVerify: jest.fn(), + decodeJwt: jest.fn(), })); -import { jwtVerify } from 'jose'; +import { jwtVerify, decodeJwt } from 'jose'; import { requirePermissions } from '../../../shared/authentication/src/auth.middleware'; import type { Request, Response, NextFunction } from 'express'; const mockedJwtVerify = jwtVerify as jest.MockedFunction; +const mockedDecodeJwt = decodeJwt as jest.MockedFunction; function createMocks(authHeader?: string) { const req = { @@ -62,8 +64,11 @@ describe('requirePermissions middleware', () => { }); describe('AUTH_BYPASS', () => { - it('should skip all validation when AUTH_BYPASS is "true"', async () => { + beforeEach(() => { process.env.AUTH_BYPASS = 'true'; + }); + + it('should skip all validation when AUTH_BYPASS is "true"', async () => { const { req, res, next } = createMocks(); const middleware = requirePermissions({ catalog: ['read'] }); @@ -72,6 +77,35 @@ describe('requirePermissions middleware', () => { expect(next).toHaveBeenCalled(); expect(res.status).not.toHaveBeenCalled(); }); + + it('should populate req.auth from decoded JWT when token is a valid JWT', async () => { + mockedDecodeJwt.mockReturnValue({ + sub: 'jwt-user-id', + email: 'test@wxyc.org', + role: 'dj', + } as any); + const { req, res, next } = createMocks('Bearer some.jwt.token'); + const middleware = requirePermissions({ bin: ['read'] }); + + await middleware(req, res, next); + + expect(next).toHaveBeenCalled(); + expect(req.auth?.id).toBe('jwt-user-id'); + }); + + it('should use raw token as user ID when token is not a valid JWT', async () => { + mockedDecodeJwt.mockImplementation(() => { + throw new Error('Invalid token'); + }); + const userId = 'abc-123-user-id'; + const { req, res, next } = createMocks(`Bearer ${userId}`); + const middleware = requirePermissions({ bin: ['read'] }); + + await middleware(req, res, next); + + expect(next).toHaveBeenCalled(); + expect(req.auth?.id).toBe(userId); + }); }); describe('token validation', () => { From 6d6a5f6bc40a84bcbb7f9a03cf3f5b2d12717b74 Mon Sep 17 00:00:00 2001 From: Jake Bromberg Date: Fri, 27 Feb 2026 16:30:19 -0800 Subject: [PATCH 5/6] fix: always use real JWT in integration tests The AUTH_BYPASS code path in integration.setup.js was sending a raw user ID string as the Bearer token. The middleware's decodeJwt() silently failed on this non-JWT string, leaving req.auth undefined and causing 500 errors in bin endpoints that access req.auth.id. Always obtain a real JWT from the auth service instead, which decodeJwt() can parse to populate req.auth even when AUTH_BYPASS skips verification. --- tests/setup/integration.setup.js | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/tests/setup/integration.setup.js b/tests/setup/integration.setup.js index fa05596..20029f0 100644 --- a/tests/setup/integration.setup.js +++ b/tests/setup/integration.setup.js @@ -49,12 +49,5 @@ async function getUserIdsFromDatabase() { // Note: Service readiness is checked in globalSetup.js (runs once for all tests) beforeAll(async () => { await getUserIdsFromDatabase(); - - if (process.env.AUTH_BYPASS === 'true') { - // Pass the DJ's user ID as the token so the bypass middleware can - // populate req.auth.id for endpoints that derive identity from the JWT. - global.access_token = `Bearer ${global.primary_dj_id}`; - } else { - global.access_token = await get_access_token(); - } + global.access_token = await get_access_token(); }); From 5671d4bd611a466fa84c952d093181f211c004d1 Mon Sep 17 00:00:00 2001 From: Jake Bromberg Date: Fri, 27 Feb 2026 16:41:22 -0800 Subject: [PATCH 6/6] fix: set auth credentials in CI for real JWT tokens The integration setup was sending a non-JWT string as the Bearer token in AUTH_BYPASS mode. The middleware's decodeJwt() silently failed on this, leaving req.auth undefined and causing 500s in bin endpoints. Fix by always obtaining a real JWT from the auth service: - Remove the AUTH_BYPASS branch in integration.setup.js that set a fake token; always call get_access_token() instead - Add AUTH_USERNAME and AUTH_PASSWORD to the CI workflow env so the better-auth sign-in flow succeeds against the seeded test users --- .github/workflows/test.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index bd895ab..2491c03 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -26,6 +26,8 @@ env: DB_PASSWORD: test-pw DB_NAME: wxyc_db AUTH_BYPASS: true + AUTH_USERNAME: test_dj1 + AUTH_PASSWORD: testpassword123 jobs: # Detect what changed to conditionally run jobs