diff --git a/server/src/nest/budget/budget.controller.ts b/server/src/nest/budget/budget.controller.ts index ce9a84096..319c3ded6 100644 --- a/server/src/nest/budget/budget.controller.ts +++ b/server/src/nest/budget/budget.controller.ts @@ -94,6 +94,11 @@ export class BudgetController { { from_user_id: body.from_user_id, to_user_id: body.to_user_id, amount: body.amount, currency: body.currency }, user.id, ); + // A party who is not on this trip gets the same answer as a settlement that + // does not exist, so the endpoint cannot be used to probe for user ids. + if (!settlement) { + throw new HttpException({ error: 'Settlement not found' }, 404); + } this.budget.broadcast(tripId, 'budget:settlement-created', { settlement }, socketId); return { settlement }; } diff --git a/server/src/nest/budget/budget.mcp.ts b/server/src/nest/budget/budget.mcp.ts index 49f1602dd..5d2b74217 100644 --- a/server/src/nest/budget/budget.mcp.ts +++ b/server/src/nest/budget/budget.mcp.ts @@ -330,6 +330,7 @@ export class BudgetMcp { // Freeze-then-write composite, same as the REST path (no-op while the // schema has no currency input — see the #1445 note in the class doc). const settlement = await this.budget.createSettlement(tripId, { from_user_id, to_user_id, amount }, ctx.userId); + if (!settlement) return errorResult('Settlement not found.'); this.guards.safeBroadcast(tripId, 'budget:settlement-created', { settlement }); return ok({ settlement }); } diff --git a/server/src/nest/budget/budget.service.ts b/server/src/nest/budget/budget.service.ts index c5c3556ae..0239033ad 100644 --- a/server/src/nest/budget/budget.service.ts +++ b/server/src/nest/budget/budget.service.ts @@ -126,21 +126,25 @@ export class BudgetService { return rows.map(p => ({ ...p, avatar_url: avatarUrl(p) })); } - private knownUserIds(userIds: number[]): Set { - const unique = [...new Set(userIds)]; - if (unique.length === 0) return new Set(); - const rows = this.db.all<{ id: number }>( - `SELECT id FROM users WHERE id IN (${unique.map(() => '?').join(',')})`, - ...unique, - ); - return new Set(rows.map(r => r.id)); + /** + * The subset of `userIds` that is actually on this trip. Used to be a plain + * existence check against `users`, which let any id on the instance become a + * payer or a split member and come back out through the read-back join with a + * name and an avatar attached. Off-roster ids drop silently, the way the + * packing and reservations assignee paths handle them. + */ + private rosterMemberIds(tripId: string | number, userIds: number[]): Set { + const unique = new Set(userIds); + if (unique.size === 0) return new Set(); + const roster = this.db.rosterUserIds(tripId); + return new Set([...unique].filter(id => roster.has(id))); } /** Replace the payer rows of an item and keep total_price = sum of payer amounts. */ - private writeItemPayers(itemId: number | string, payers: { user_id: number; amount: number }[]) { + private writeItemPayers(itemId: number | string, tripId: string | number, payers: { user_id: number; amount: number }[]) { this.db.run('DELETE FROM budget_item_payers WHERE budget_item_id = ?', itemId); const insert = this.db.prepare('INSERT OR IGNORE INTO budget_item_payers (budget_item_id, user_id, amount) VALUES (?, ?, ?)'); - const known = this.knownUserIds(payers.map(p => p.user_id)); + const known = this.rosterMemberIds(tripId, payers.map(p => p.user_id)); let total = 0; for (const p of payers) { if (!(p.amount > 0) || !known.has(p.user_id)) continue; @@ -347,9 +351,9 @@ export class BudgetService { const payerTotal = (data.payers || []).reduce((a, p) => a + (p.amount > 0 ? p.amount : 0), 0); const total = data.payers && data.payers.length > 0 ? payerTotal : (data.total_price || 0); - const knownMembers = data.members ? this.knownUserIds(data.members.map(m => m.user_id)) : null; + const knownMembers = data.members ? this.rosterMemberIds(tripId, data.members.map(m => m.user_id)) : null; const members = data.members && knownMembers ? data.members.filter(m => knownMembers.has(m.user_id)) : undefined; - const knownIds = data.member_ids ? this.knownUserIds(data.member_ids) : null; + const knownIds = data.member_ids ? this.rosterMemberIds(tripId, data.member_ids) : null; const memberIds = data.member_ids && knownIds ? data.member_ids.filter(uid => knownIds.has(uid)) : undefined; const { note, ticket } = splitLegacyTicketNote(data.note, data.ticket_json); @@ -373,7 +377,7 @@ export class BudgetService { ); const itemId = result.lastInsertRowid as number; - if (data.payers && data.payers.length > 0) this.writeItemPayers(itemId, data.payers); + if (data.payers && data.payers.length > 0) this.writeItemPayers(itemId, tripId, data.payers); if (members && members.length > 0) { const insert = this.db.prepare('INSERT OR IGNORE INTO budget_item_members (budget_item_id, user_id, paid, amount) VALUES (?, ?, 0, ?)'); for (const m of members) insert.run(itemId, m.user_id, m.amount !== undefined && m.amount !== null ? m.amount : null); @@ -461,7 +465,7 @@ export class BudgetService { // Optional inline payer/member replacement (the edit modal saves all at once). if (data.payers !== undefined) { - this.writeItemPayers(id, data.payers); + this.writeItemPayers(id, tripId, data.payers); // writeItemPayers derives total_price from the payer sum (0 for no payers). // A "recorded total, nobody assigned" expense clears payers but still carries // an explicit total_price — re-apply it so it isn't clobbered to 0. @@ -470,14 +474,14 @@ export class BudgetService { } } if (data.members !== undefined) { - const known = this.knownUserIds(data.members.map(m => m.user_id)); + const known = this.rosterMemberIds(tripId, data.members.map(m => m.user_id)); const members = data.members.filter(m => known.has(m.user_id)); this.db.run('DELETE FROM budget_item_members WHERE budget_item_id = ?', id); const insert = this.db.prepare('INSERT OR IGNORE INTO budget_item_members (budget_item_id, user_id, paid, amount) VALUES (?, ?, 0, ?)'); for (const m of members) insert.run(id, m.user_id, m.amount !== undefined && m.amount !== null ? m.amount : null); this.db.run('UPDATE budget_items SET persons = ? WHERE id = ?', members.length || null, id); } else if (data.member_ids !== undefined) { - const known = this.knownUserIds(data.member_ids); + const known = this.rosterMemberIds(tripId, data.member_ids); const memberIds = data.member_ids.filter(uid => known.has(uid)); this.db.run('DELETE FROM budget_item_members WHERE budget_item_id = ?', id); const insert = this.db.prepare('INSERT OR IGNORE INTO budget_item_members (budget_item_id, user_id, paid, amount) VALUES (?, ?, 0, NULL)'); @@ -510,7 +514,7 @@ export class BudgetService { return this.db.transaction(() => { const item = this.db.get('SELECT id FROM budget_items WHERE id = ? AND trip_id = ?', id, tripId); if (!item) return null; - this.writeItemPayers(id, payers); + this.writeItemPayers(id, tripId, payers); const updated = this.db.get('SELECT * FROM budget_items WHERE id = ?', id)!; updated.members = this.loadItemMembers(id); updated.payers = this.loadItemPayers(id); @@ -540,7 +544,7 @@ export class BudgetService { this.db.run('DELETE FROM budget_item_members WHERE budget_item_id = ?', id); - const known = this.knownUserIds(userIds); + const known = this.rosterMemberIds(tripId, userIds); const memberIds = userIds.filter(uid => known.has(uid)); if (memberIds.length > 0) { const insert = this.db.prepare('INSERT OR IGNORE INTO budget_item_members (budget_item_id, user_id, paid) VALUES (?, ?, ?)'); @@ -949,7 +953,21 @@ export class BudgetService { return this.setItemPayers(id, tripId, payers); } + /** + * Unlike a split member, a settlement cannot drop an off-roster id: it has + * exactly two named parties and both columns are NOT NULL, so a stranger in + * either slot makes the row meaningless rather than merely wider. Refuse the + * whole write. Returning null lands on the caller's existing "Settlement not + * found" 404, which is also what keeps the endpoint from confirming whether + * an id it rejected exists at all. + */ + private settlementPartiesOnTrip(tripId: string | number, data: { from_user_id: number; to_user_id: number }): boolean { + const roster = this.db.rosterUserIds(tripId); + return roster.has(data.from_user_id) && roster.has(data.to_user_id); + } + async createSettlement(tripId: string | number, data: { from_user_id: number; to_user_id: number; amount: number; currency?: string | null }, userId: number) { + if (!this.settlementPartiesOnTrip(tripId, data)) return null; // Freeze the FX rate for the display currency the amount was entered in so the // transfer keeps cancelling its expense when live rates drift (#1445). await this.freezeForeignRate(tripId, data); @@ -960,6 +978,7 @@ export class BudgetService { // Pass the settlement's stored currency so an edit that doesn't change it keeps // the already-frozen rate (#1445) — otherwise a live-rate drift would re-open a // settled position on an unrelated edit. + if (!this.settlementPartiesOnTrip(tripId, data)) return null; const existing = this.getSettlement(id, tripId); await this.freezeForeignRate(tripId, data, undefined, existing?.currency ?? null); return this.applySettlementUpdate(id, tripId, data); diff --git a/server/tests/e2e/budget.e2e.test.ts b/server/tests/e2e/budget.e2e.test.ts index 580e86222..0bb965214 100644 --- a/server/tests/e2e/budget.e2e.test.ts +++ b/server/tests/e2e/budget.e2e.test.ts @@ -73,6 +73,9 @@ describe('Budget e2e (real auth guard + temp SQLite, real budget SQL)', () => { "INSERT INTO users (id, username, email, password_hash, role, password_version) VALUES (2, 'e2e-peer', 'peer@example.test', 'x', 'user', 0)", ).run(); tripId = Number(db.prepare("INSERT INTO trips (user_id, title, currency) VALUES (1, 'E2E Trip', 'EUR')").run().lastInsertRowid); + // The peer settles up with the owner below, so they have to be on the trip: + // a settlement between people who do not share one is refused. + db.prepare('INSERT INTO trip_members (trip_id, user_id) VALUES (?, 2)').run(tripId); app = await build(); checkPermission = vi.spyOn(app.get(PermissionsService), 'checkPermission'); server = app.getHttpServer(); diff --git a/server/tests/unit/nest/budget.service.db.test.ts b/server/tests/unit/nest/budget.service.db.test.ts index 2e8247258..6becc5019 100644 --- a/server/tests/unit/nest/budget.service.db.test.ts +++ b/server/tests/unit/nest/budget.service.db.test.ts @@ -55,7 +55,7 @@ vi.mock('../../../src/nest/budget/exchange-rates.service', () => ({ import { createTables } from '../../../src/db/schema'; import { runMigrations } from '../../../src/db/migrations'; import { resetTestDb } from '../../helpers/test-db'; -import { createUser, createTrip } from '../../helpers/factories'; +import { createUser, createTrip, addTripMember } from '../../helpers/factories'; import { BudgetService } from '../../../src/nest/budget/budget.service'; import { DatabaseService } from '../../../src/nest/database/database.service'; import { PermissionsService } from '../../../src/nest/permissions/permissions.service'; @@ -230,6 +230,7 @@ describe('calculateSettlement custom splits', () => { const { user: alice } = createUser(testDb, { username: 'alice' }); const { user: bob } = createUser(testDb, { username: 'bob' }); const trip = createTrip(testDb, alice.id, { title: 'Trip' }); + addTripMember(testDb, trip.id, bob.id); // 100 total, custom split: Alice owes 90, Bob owes 10. Alice paid the whole bill. budget.createBudgetItem(trip.id, { @@ -261,6 +262,8 @@ describe('calculateSettlement squares up to the cent (#1382)', () => { const { user: bob } = createUser(testDb, { username: 'bob' }); const { user: carol } = createUser(testDb, { username: 'carol' }); const trip = createTrip(testDb, alice.id, { title: 'Trip' }); + addTripMember(testDb, trip.id, bob.id); + addTripMember(testDb, trip.id, carol.id); const members = [{ user_id: alice.id }, { user_id: bob.id }, { user_id: carol.id }]; // Totals that never divide by three, so every expense leaves a remainder cent @@ -306,6 +309,8 @@ function seedIssue1543Trip(tripCurrency: string) { const { user: danil } = createUser(testDb, { username: 'danil' }); const { user: serega } = createUser(testDb, { username: 'serega' }); const trip = createTrip(testDb, me.id, { title: 'Trip' }); + addTripMember(testDb, trip.id, danil.id); + addTripMember(testDb, trip.id, serega.id); testDb.prepare('UPDATE trips SET currency = ? WHERE id = ?').run(tripCurrency, trip.id); const members = [{ user_id: me.id }, { user_id: danil.id }, { user_id: serega.id }]; @@ -384,6 +389,7 @@ describe('rebaseTripCurrency', () => { const { user: alice } = createUser(testDb, { username: 'alice' }); const { user: bob } = createUser(testDb, { username: 'bob' }); const trip = createTrip(testDb, alice.id, { title: 'Trip' }); + addTripMember(testDb, trip.id, bob.id); testDb.prepare("UPDATE trips SET currency = 'EUR' WHERE id = ?").run(trip.id); const members = [{ user_id: alice.id }, { user_id: bob.id }]; @@ -524,6 +530,57 @@ describe('composite service paths (ex budget.bridge delegation)', () => { }); }); +// A settlement names two parties and both columns are NOT NULL, so an id that is +// not on the trip cannot simply be dropped the way a split member can — the whole +// write is refused. Refusing also means the endpoint stops answering "does this +// user id exist", which it used to do by 500ing on the foreign key. +describe('settlement parties are confined to the trip', () => { + it('BUDGET-SVC-DB-028: refuses a payer who is not on the trip', async () => { + const { user: alice } = createUser(testDb, { username: 'alice' }); + const { user: outsider } = createUser(testDb, { username: 'outsider' }); + const trip = createTrip(testDb, alice.id); + + const created = await budget.createSettlement(trip.id, { from_user_id: outsider.id, to_user_id: alice.id, amount: 10 }, alice.id); + + expect(created).toBeNull(); + expect(budget.listSettlements(trip.id)).toEqual([]); + }); + + it('BUDGET-SVC-DB-029: answers a nonexistent user the same way, without throwing', async () => { + const { user: alice } = createUser(testDb, { username: 'alice' }); + const trip = createTrip(testDb, alice.id); + + await expect(budget.createSettlement(trip.id, { from_user_id: 999999, to_user_id: alice.id, amount: 10 }, alice.id)) + .resolves.toBeNull(); + }); + + it('BUDGET-SVC-DB-030: still records a settlement between the owner and a member', async () => { + const { user: alice } = createUser(testDb, { username: 'alice' }); + const { user: bob } = createUser(testDb, { username: 'bob' }); + const trip = createTrip(testDb, alice.id); + addTripMember(testDb, trip.id, bob.id); + + const created = await budget.createSettlement(trip.id, { from_user_id: bob.id, to_user_id: alice.id, amount: 10 }, alice.id); + + expect(created).toMatchObject({ from_user_id: bob.id, to_user_id: alice.id }); + }); + + it('BUDGET-SVC-DB-031: drops an off-trip payer from an item instead of refusing it', () => { + const { user: alice } = createUser(testDb, { username: 'alice' }); + const { user: outsider } = createUser(testDb, { username: 'outsider' }); + const trip = createTrip(testDb, alice.id); + + const item = budget.createBudgetItem(trip.id, { + name: 'Dinner', + payers: [{ user_id: alice.id, amount: 40 }, { user_id: outsider.id, amount: 60 }], + }) as { payers: { user_id: number }[]; total_price: number }; + + expect(item.payers.map(p => p.user_id)).toEqual([alice.id]); + // total_price is the sum of the payers that actually landed. + expect(item.total_price).toBe(40); + }); +}); + describe('post-fold quirk fixes', () => { it('BUDGET-SVC-DB-019: settlements prefer display_name over username (quirk fix)', () => { const { user: alice } = createUser(testDb, { username: 'alice' }); diff --git a/server/tests/unit/nest/budget.service.test.ts b/server/tests/unit/nest/budget.service.test.ts index 501598d79..f7317cc7b 100644 --- a/server/tests/unit/nest/budget.service.test.ts +++ b/server/tests/unit/nest/budget.service.test.ts @@ -139,8 +139,14 @@ describe('BudgetService', () => { }); describe('settlement ledger wrappers (#1445 freeze-then-write)', () => { + // Both wrappers refuse a party who is not on the trip before they freeze + // anything, so the roster lookup has to answer for these to reach the write + // at all. Users 1 and 2 are the two parties every case here settles between. + const rosterHas = (...userIds: number[]) => dbMock._stmt.all.mockReturnValue(userIds.map(user_id => ({ user_id }))); + it('createSettlement freezes the FX rate (await) before the raw insert', async () => { const s = svc(); + rosterHas(1, 2); const freezeSpy = vi.spyOn(s, 'freezeForeignRate').mockResolvedValue(); const insertSpy = vi.spyOn(s, 'insertSettlement').mockReturnValue({ id: 7 } as never); await s.createSettlement('5', { from_user_id: 1, to_user_id: 2, amount: 10 }, 3); @@ -150,6 +156,7 @@ describe('BudgetService', () => { it('updateSettlement threads the stored currency through the freeze', async () => { const s = svc(); + rosterHas(1, 2); const freezeSpy = vi.spyOn(s, 'freezeForeignRate').mockResolvedValue(); // Quirk fix: the stored-currency lookup is a targeted getSettlement, not // a full listSettlements scan. @@ -161,6 +168,18 @@ describe('BudgetService', () => { expect(freezeSpy).toHaveBeenCalledWith('5', { from_user_id: 1, to_user_id: 2, amount: 12, currency: 'USD' }, undefined, 'USD'); expect(applySpy).toHaveBeenCalledWith('7', '5', { from_user_id: 1, to_user_id: 2, amount: 12, currency: 'USD' }); }); + + it('refuses a party who is not on the trip, before freezing or writing', async () => { + const s = svc(); + rosterHas(1); // user 2 is not on this trip + const freezeSpy = vi.spyOn(s, 'freezeForeignRate').mockResolvedValue(); + const insertSpy = vi.spyOn(s, 'insertSettlement').mockReturnValue({ id: 7 } as never); + + await expect(s.createSettlement('5', { from_user_id: 1, to_user_id: 2, amount: 10 }, 3)).resolves.toBeNull(); + + expect(freezeSpy).not.toHaveBeenCalled(); + expect(insertSpy).not.toHaveBeenCalled(); + }); }); describe('syncReservationPrice', () => {