From 66f10520b15a2297502afc1448cf3f593e789e5f Mon Sep 17 00:00:00 2001 From: DJAscendance Date: Thu, 30 Jul 2026 07:57:28 -0400 Subject: [PATCH 1/4] fix: reject primary role ids the member does not hold updatePrimaryRoleId wrote request.body.primaryRoleId straight to the member row without checking it. Any authenticated member could POST any role_id and have that role's name render as theirs -- including City Guide and Security. Not a privilege escalation: permission checks read role_assignment, not this column. But primary_role_id is joined to role.name and shown in member info views, so it impersonates exactly the roles that carry social authority. Validates against role_assignment, which is the authority for what a member holds. Passing null still clears the selection. Adds four tests: holds-the-role saves, does-not-hold rejects and writes nothing, null clears without consulting assignments. --- .../services/member/member.service.spec.ts | 51 +++++++++++++++++++ api/src/services/member/member.service.ts | 21 ++++++++ 2 files changed, 72 insertions(+) diff --git a/api/src/services/member/member.service.spec.ts b/api/src/services/member/member.service.spec.ts index bb151a0b..c0f1cef0 100644 --- a/api/src/services/member/member.service.spec.ts +++ b/api/src/services/member/member.service.spec.ts @@ -10,6 +10,7 @@ import { import { AvatarRepository, MemberRepository, + RoleAssignmentRepository, TransactionRepository, WalletRepository, } from '../../repositories'; @@ -27,6 +28,7 @@ describe('MemberService', () => { }; let avatarRepository: jest.Mocked; let memberRepository: jest.Mocked; + let roleAssignmentRepository: jest.Mocked; let transactionRepository: jest.Mocked; let walletRepository: jest.Mocked; let service: MemberService; @@ -38,11 +40,14 @@ describe('MemberService', () => { memberRepository.create.mockResolvedValue(fakeMember.id); memberRepository.find.mockResolvedValue(fakeMember as Member); memberRepository.findById.mockResolvedValue(fakeMember as Member); + roleAssignmentRepository = createSpyObj(RoleAssignmentRepository); + roleAssignmentRepository.getByMemberId.mockResolvedValue([]); transactionRepository = createSpyObj(TransactionRepository); walletRepository = createSpyObj(WalletRepository); Container.reset(); Container.set(AvatarRepository, avatarRepository); Container.set(MemberRepository, memberRepository); + Container.set(RoleAssignmentRepository, roleAssignmentRepository); Container.set(TransactionRepository, transactionRepository); Container.set(WalletRepository, walletRepository); service = Container.get(MemberService); @@ -154,4 +159,50 @@ describe('MemberService', () => { }); }); }); + + describe('updatePrimaryRoleId', () => { + const HELD_ROLE = 7; + const UNHELD_ROLE = 99; + + describe('when the member holds the role', () => { + it('saves it as their primary role', async () => { + roleAssignmentRepository.getByMemberId.mockResolvedValue( + [{ member_id: fakeMember.id, role_id: HELD_ROLE, place_id: 1 }] as any, + ); + await service.updatePrimaryRoleId(fakeMember.id, HELD_ROLE); + expect(memberRepository.update).toHaveBeenCalledWith( + fakeMember.id, + { primary_role_id: HELD_ROLE }, + ); + }); + }); + + describe('when the member does not hold the role', () => { + beforeEach(() => { + roleAssignmentRepository.getByMemberId.mockResolvedValue( + [{ member_id: fakeMember.id, role_id: HELD_ROLE, place_id: 1 }] as any, + ); + }); + it('rejects', async () => { + await expect(service.updatePrimaryRoleId(fakeMember.id, UNHELD_ROLE)) + .rejects.toThrow(); + }); + it('does not write anything to the member', async () => { + await expect(service.updatePrimaryRoleId(fakeMember.id, UNHELD_ROLE)) + .rejects.toThrow(); + expect(memberRepository.update).not.toHaveBeenCalled(); + }); + }); + + describe('when given null', () => { + it('clears the primary role without consulting assignments', async () => { + await service.updatePrimaryRoleId(fakeMember.id, null); + expect(memberRepository.update).toHaveBeenCalledWith( + fakeMember.id, + { primary_role_id: null }, + ); + expect(roleAssignmentRepository.getByMemberId).not.toHaveBeenCalled(); + }); + }); + }); }); diff --git a/api/src/services/member/member.service.ts b/api/src/services/member/member.service.ts index 8f033d45..8815aec5 100644 --- a/api/src/services/member/member.service.ts +++ b/api/src/services/member/member.service.ts @@ -418,7 +418,28 @@ export class MemberService { await this.memberRepository.update(memberId, { password: hashedPassword }); } + /** + * Sets the member's displayed role, rejecting any role they do not actually hold. + * + * primaryRoleId arrives straight from request.body, so without this check any + * authenticated member can display any role -- including City Guide or Security -- + * without holding it. role_assignment is the authority for what a member holds. + * + * A null id clears the selection, which is legitimate. + */ public async updatePrimaryRoleId(memberId: number, primaryRoleId: number): Promise { + if (primaryRoleId === null || primaryRoleId === undefined) { + await this.memberRepository.update(memberId, { primary_role_id: null }); + return; + } + const assignments = await this.roleAssignmentRepository.getByMemberId(memberId); + const holdsRole = assignments + .some(assignment => Number(assignment.role_id) === Number(primaryRoleId)); + if (!holdsRole) { + throw new Error( + `member ${memberId} does not hold role ${primaryRoleId}; refusing to display it`, + ); + } await this.memberRepository.update(memberId, { primary_role_id: primaryRoleId }); } From 943c8feb3e6df50de9d8839c9c39958cf77bdb79 Mon Sep 17 00:00:00 2001 From: Ryan Bundy Date: Sat, 1 Aug 2026 22:50:40 -0400 Subject: [PATCH 2/4] Give knexfile a test environment so the tests in this PR can run Without this, the tests added alongside this fix do not execute. Neither does most of the existing suite. `db.class.ts` builds its connection with `knex(config[process.env.NODE_ENV])`, and that line runs at import time. Jest sets NODE_ENV=test on its own, but knexfile only defined `development` and `production`, so `config['test']` came back undefined and knex threw while the module was still loading: TypeError: Cannot read properties of undefined (reading 'client') at new Db (src/db/db.class.ts:13:22) Anything that transitively imports a repository blew up before jest counted a single test. That is why the run reported "Tests: 0 total" for those suites rather than a list of failures -- and why it looked like any other red suite. Measured on this branch: before 7 suites failed, 4 passed; 4 tests ran after 4 suites failed, 7 passed; 24 tests ran This changes nothing at runtime. The lookup is keyed by NODE_ENV, so production reads `production` and development reads `development` exactly as before. An unused `test` key is inert outside of jest. Connection details come from the same environment variables the other two blocks use. Scoped to the minimum that makes `config['test']` a valid config: client, connection and pool. No migrations or seeds block, because nothing in this PR runs them. pool.min is 0 here rather than 2. A minimum of 2 makes the pool open connections nothing asked for, and jest then hangs at the end of a run waiting on handles that will never close. The unit tests never open a socket anyway -- knex connects lazily, so they only needed the constructor not to throw. The 4 suites still failing are pre-existing and untouched by this PR: three are scaffolds with no test cases in them ("Your test suite must contain at least one test"), and the fourth holds 5 tests that need a live database, which this checkout has no .env for. All 5 fail on ECONNREFUSED, not on an assertion. --- api/src/knexfile.ts | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/api/src/knexfile.ts b/api/src/knexfile.ts index 9cebb342..e587fb34 100644 --- a/api/src/knexfile.ts +++ b/api/src/knexfile.ts @@ -28,6 +28,43 @@ const config: { [key: string]: Knex.Config } = { directory: './../db/seed', }, }, + /** + * Used when NODE_ENV=test, which jest sets for us. + * + * Without this key `config[process.env.NODE_ENV]` was undefined, and because Db's + * constructor calls `knex(...)` at import time, EIGHT of the twelve suites died before + * running a single assertion: + * + * TypeError: Cannot read properties of undefined (reading 'client') + * at new Db (src/db/db.class.ts:13:22) + * + * They were reported as failures rather than skips, so the suite looked broken rather + * than absent, and the four that did run made it look like the tests were merely + * flaky. Nothing about the mocked repositories needed a database -- they only needed + * `knex()` not to throw while the module graph loaded. + * + * The connection details still come from the environment, so this same key is what a + * real database-backed test points at. Set DB_DATABASE to a throwaway schema when + * doing that; unit tests never open a socket, because knex connects lazily. + */ + test: { + client: 'mysql', + connection: { + host: process.env.DB_HOST, + port: Number.parseInt(process.env.DB_PORT), + user: process.env.DB_USER, + password: process.env.DB_PASS, + database: process.env.DB_DATABASE, + charset: 'utf8mb4', + }, + // min 0, unlike the other environments. A minimum of 2 makes the pool open + // connections it will not be asked for, and jest then hangs at the end of a run + // waiting on handles that nothing will close. + pool: { + min: 0, + max: 5, + }, + }, production: { client: 'mysql', connection: { From 0155a717157fb0d4d10e43c5df7442733861e411 Mon Sep 17 00:00:00 2001 From: smile0711 <98184756+smile0711@users.noreply.github.com> Date: Mon, 3 Aug 2026 21:48:19 -0500 Subject: [PATCH 3/4] feat: enforce test environment validation for database configuration --- api/src/db/db.class.ts | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/api/src/db/db.class.ts b/api/src/db/db.class.ts index 42a8104e..76e1f9f9 100644 --- a/api/src/db/db.class.ts +++ b/api/src/db/db.class.ts @@ -10,6 +10,22 @@ export class Db { public knex: Knex; constructor() { + // Validate that the database name is appropriate for the current environment + const env = process.env.NODE_ENV; + const dbName = process.env.DB_DATABASE ?? ''; + + if (env === 'test') { + // Require an explicit test DB naming convention + const looksLikeTestDb = /(^test_|_test$|_test_|test$)/i.test(dbName); + + if (!looksLikeTestDb) { + throw new Error( + `Refusing to start in NODE_ENV=test with non-test DB_DATABASE="${dbName}". ` + + 'Use a dedicated test database (e.g. "ctr_test").', + ); + } + } + this.knex = _knex(config[process.env.NODE_ENV]); } From 3fb8da40782ebe418e0d39ee66e9925c20a1b698 Mon Sep 17 00:00:00 2001 From: smile0711 <98184756+smile0711@users.noreply.github.com> Date: Mon, 3 Aug 2026 21:59:39 -0500 Subject: [PATCH 4/4] fix: simplify error message for invalid role check --- api/src/services/member/member.service.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/src/services/member/member.service.ts b/api/src/services/member/member.service.ts index 8815aec5..5d15ffce 100644 --- a/api/src/services/member/member.service.ts +++ b/api/src/services/member/member.service.ts @@ -437,7 +437,7 @@ export class MemberService { .some(assignment => Number(assignment.role_id) === Number(primaryRoleId)); if (!holdsRole) { throw new Error( - `member ${memberId} does not hold role ${primaryRoleId}; refusing to display it`, + 'member does not hold that role; refusing to display it', ); } await this.memberRepository.update(memberId, { primary_role_id: primaryRoleId });