From d7aba126cdc0b65069904304f488953d21e94eb7 Mon Sep 17 00:00:00 2001 From: jubnl Date: Wed, 23 Sep 2026 18:58:15 +0200 Subject: [PATCH] refactor(collab): notes, polls, messages and links through repositories --- .../db/repositories/CollabLinks.repository.ts | 91 +++- .../CollabMessageReactions.repository.ts | 72 ++- .../repositories/CollabMessages.repository.ts | 200 +++++++- .../db/repositories/CollabNotes.repository.ts | 254 +++++++++- .../CollabPollVotes.repository.ts | 49 +- .../db/repositories/CollabPolls.repository.ts | 75 ++- server/src/nest/collab/collab.module.ts | 13 + server/src/nest/collab/collab.service.ts | 445 +++++++++--------- server/tests/helpers/collab-repos.ts | 55 +++ server/tests/unit/nest/collab.service.test.ts | 226 ++++++++- 10 files changed, 1239 insertions(+), 241 deletions(-) create mode 100644 server/tests/helpers/collab-repos.ts diff --git a/server/src/db/repositories/CollabLinks.repository.ts b/server/src/db/repositories/CollabLinks.repository.ts index 6f140a4c8..bc32f05a4 100644 --- a/server/src/db/repositories/CollabLinks.repository.ts +++ b/server/src/db/repositories/CollabLinks.repository.ts @@ -1,4 +1,93 @@ import type { CollabLinks } from '../entities/CollabLinks.entity'; +import { currentTimestamp } from '../dialect/sql-functions'; +import { type AssertRowKeys } from './_shared/rows'; import { TrekRepository } from './_shared/trek-repository'; -export class CollabLinksRepository extends TrekRepository {} +/** A bare `collab_links` row — every scalar column of the entity, incl. the two `persist(false)` relation mirrors (`trip_id`, `user_id`). */ +export interface CollabLinkRow { + id: number; + trip_id: number; + user_id: number; + title: string; + url: string; + pinned: number | null; + created_at: string | null; + updated_at: string | null; +} + +const _collabLinkRowKeys: AssertRowKeys = true; + +/** CB36/CB38/CB41's joined projection — `l.*, u.username`. */ +export interface CollabLinkJoinRow extends CollabLinkRow { + username: string; +} + +interface CollabLinksKyselyDB { + collab_links: CollabLinkRow; + users: { id: number; username: string }; +} + +/** + * `collab_links` — pinned/shared trip links. Kysely throughout: `trip_id`/ + * `user_id` are `persist(false)` mirrors (the `TripFilesRepository` class + * docstring's trap), so a bare QueryBuilder `.select([...])` silently drops + * them. + */ +export class CollabLinksRepository extends TrekRepository { + private joinedQuery() { + return this.kysely() + .selectFrom('collab_links as l') + .innerJoin('users as u', 'u.id', 'l.user_id') + .selectAll('l') + .select(['u.username']); + } + + /** + * CB36 (`listLinks`) — `SELECT l.*, u.username FROM collab_links l JOIN + * users u ON u.id = l.user_id WHERE l.trip_id = ? ORDER BY l.pinned DESC, + * l.created_at DESC`. `trip_id: number`: the service resolves the route's + * `string | number` id via `toRowId` before calling (rule 15). + */ + async listForTrip(trip_id: number): Promise { + return await this.joinedQuery().where('l.trip_id', '=', trip_id).orderBy('l.pinned', 'desc').orderBy('l.created_at', 'desc').execute(); + } + + /** CB39 (`updateLink`'s trip-scoping guard) — `SELECT * FROM collab_links WHERE id = ? AND trip_id = ?`. Full row: the falsy-coercion update (CB40) needs the pre-image to fall back onto. */ + async findInTrip(id: number, trip_id: number): Promise { + return await this.kysely().selectFrom('collab_links').selectAll().where('id', '=', id).where('trip_id', '=', trip_id).executeTakeFirst(); + } + + /** CB37 (`createLink`) — `INSERT INTO collab_links (trip_id, user_id, title, url, pinned) VALUES (?×5)`. The service trims `title`/`url` before calling (matching the legacy inline `.trim()`). Returns the new row's id. */ + async insertLink(row: { trip_id: number | string; user_id: number; title: string; url: string; pinned: number }): Promise { + return await this.insert({ trip: row.trip_id, user: row.user_id, title: row.title, url: row.url, pinned: row.pinned }); + } + + /** + * CB40 (`updateLink`) — `UPDATE collab_links SET title = ?, url = ?, + * pinned = ?, updated_at = CURRENT_TIMESTAMP WHERE id = ? AND trip_id = ?`. + * The ONE outlier in this cluster: `|| existing.x` falsy-coercion, NOT the + * presence-sentinel `CASE` shape `CollabNotesRepository.update`/`CB11` + * uses (task-5-brief.md's explicit instruction — kept as its own distinct + * shape). The SERVICE resolves `data.title?.trim() || existing.title` / + * `data.url?.trim() || existing.url` / `data.pinned === undefined ? + * existing.pinned : (data.pinned ? 1 : 0)` to final values first (same + * read-merge-write split as {@link CollabNotesRepository.update}) — this + * method just writes them, scoped by BOTH `id` AND `trip_id` (unlike + * CB11's notes update, which the legacy only scopes by `id`). + */ + async update(id: number, trip_id: number, write: { title: string; url: string; pinned: number }): Promise { + const platform = this.getEntityManager().getPlatform(); + await this.nativeUpdate({ id, trip: trip_id }, { title: write.title, url: write.url, pinned: write.pinned, updated_at: currentTimestamp(platform) }); + } + + /** CB38/CB41 (`createLink`'s post-insert re-select, `updateLink`'s post-write re-select) — `SELECT l.*, u.username FROM collab_links l JOIN users u ON u.id = l.user_id WHERE l.id = ?`, same text at both call sites. */ + async findWithUser(id: number): Promise { + return await this.joinedQuery().where('l.id', '=', id).executeTakeFirst(); + } + + /** CB42 (`deleteLink`) — `DELETE FROM collab_links WHERE id = ? AND trip_id = ?`, returns whether a row was actually removed (legacy `.changes > 0`). */ + async deleteScoped(id: number, trip_id: number): Promise { + const affected = await this.nativeDelete({ id, trip: trip_id }); + return affected > 0; + } +} diff --git a/server/src/db/repositories/CollabMessageReactions.repository.ts b/server/src/db/repositories/CollabMessageReactions.repository.ts index 21706674f..d0a65aa08 100644 --- a/server/src/db/repositories/CollabMessageReactions.repository.ts +++ b/server/src/db/repositories/CollabMessageReactions.repository.ts @@ -1,4 +1,74 @@ import type { CollabMessageReactions } from '../entities/CollabMessageReactions.entity'; import { TrekRepository } from './_shared/trek-repository'; -export class CollabMessageReactionsRepository extends TrekRepository {} +/** CB1/CB46's joined projection — `r.emoji, r.user_id, u.username` (CB46 additionally carries `message_id`, the batch key). */ +export interface ReactionJoinRow { + message_id: number; + emoji: string; + user_id: number; + username: string; +} + +export class CollabMessageReactionsRepository extends TrekRepository { + /** + * CB1 (`CollabService.loadReactions`) — `SELECT r.emoji, r.user_id, + * u.username FROM collab_message_reactions r JOIN users u ON r.user_id = + * u.id WHERE r.message_id = ?`. `r.user_id`/`r.message_id` are + * `persist(false)` mirrors of the `message`/`user` relations — selecting + * the relation property (not the mirror) is what actually hydrates it + * (`PlaceRatingsRepository.listForPlaces`'s docstring, the same trap). + */ + async listForMessage(message_id: number): Promise[]> { + return this.qb('r') + .join('r.user', 'u') + .select(['r.emoji', 'u.id as user_id', 'u.username']) + .where({ message: message_id }) + .execute[]>('all', false); + } + + /** + * CB46 (`CollabService.listMessages`'s batch reactions) — `SELECT + * r.message_id, r.emoji, r.user_id, u.username FROM collab_message_reactions + * r JOIN users u ON r.user_id = u.id WHERE r.message_id IN (dynamic)`. + * Empty `message_ids` short-circuits, matching the service's own guard + * (`if (msgIds.length > 0)`). + */ + async listForMessages(message_ids: number[]): Promise { + if (message_ids.length === 0) return []; + return this.qb('r') + .join('r.user', 'u') + .select(['r.message', 'r.emoji', 'u.id as user_id', 'u.username']) + .where({ 'r.message': { $in: message_ids } }) + .execute('all', false); + } + + /** + * CB3 (`CollabService.reactMessage`'s toggle check) — `SELECT id FROM + * collab_message_reactions WHERE message_id = ? AND user_id = ? AND + * emoji = ?`. Named `findReaction`, not `find`: `find` is `EntityRepository`'s + * own base method (a different, incompatible signature) and a same-named + * override does not typecheck. + */ + async findReaction(message_id: number, user_id: number, emoji: string): Promise<{ id: number } | null> { + return this.findOne({ message: message_id, user: user_id, emoji }, { fields: ['id'] }); + } + + /** + * CB4 (`reactMessage`'s toggle-off half) — `DELETE FROM + * collab_message_reactions WHERE id = ?`. Deliberately NOT wrapped in a + * transaction with {@link insertReaction}/{@link find}: the legacy + * check-then-toggle (CB3→CB4/CB5) has no `uow.transactional` either — a + * duplicate/missing reaction under a race is cosmetic, not the + * data-integrity class of defect the R2/`toggleMemberPaid` fixes address + * (task-5-brief.md's own ruling; left as-is rather than "fixed" + * unilaterally). + */ + async deleteById(id: number): Promise { + await this.nativeDelete({ id }); + } + + /** CB5 (`reactMessage`'s toggle-on half) — `INSERT INTO collab_message_reactions (message_id, user_id, emoji) VALUES (?, ?, ?)`. */ + async insertReaction(message_id: number, user_id: number, emoji: string): Promise { + await this.insert({ message: message_id, user: user_id, emoji }); + } +} diff --git a/server/src/db/repositories/CollabMessages.repository.ts b/server/src/db/repositories/CollabMessages.repository.ts index e7f206905..4ec5b1a45 100644 --- a/server/src/db/repositories/CollabMessages.repository.ts +++ b/server/src/db/repositories/CollabMessages.repository.ts @@ -1,4 +1,202 @@ import type { CollabMessages } from '../entities/CollabMessages.entity'; +import type { TripFileRow } from './TripFiles.repository'; +import { type AssertRowKeys } from './_shared/rows'; import { TrekRepository } from './_shared/trek-repository'; -export class CollabMessagesRepository extends TrekRepository {} +/** A bare `collab_messages` row — every scalar column of the entity, incl. the three `persist(false)` mirrors (`trip_id`, `user_id`, `reply_to`). */ +export interface CollabMessageRow { + id: number; + trip_id: number; + user_id: number; + text: string; + reply_to: number | null; + created_at: string | null; + deleted: number | null; +} + +const _collabMessageRowKeys: AssertRowKeys = true; + +/** CB45/CB50's joined projection — `m.*, u.username, u.avatar, reply_text, reply_username`. */ +export interface CollabMessageJoinRow extends CollabMessageRow { + username: string; + avatar: string | null; + reply_text: string | null; + reply_username: string | null; +} + +interface CollabMessagesKyselyDB { + collab_messages: CollabMessageRow; + users: { id: number; username: string; avatar: string | null }; +} + +/** Only the `trip_files` columns the message-attachment methods below touch — the `CollabNotesRepository` class docstring explains why they live on the OWNING collab entity's repository rather than on `TripFilesRepository`. */ +interface MessageAttachmentsKyselyDB { + trip_files: TripFileRow; +} + +/** CB43's narrow attachment projection (`formatMessage`'s hydration read). `message_id` keeps `TripFileRow`'s own nullable typing even though the WHERE clause below always matches a non-null value — no narrowing cast. */ +export interface MessageAttachmentRow { + id: number; + trip_id: number; + message_id: number | null; + filename: string; + original_name: string; + file_size: number | null; + mime_type: string | null; +} + +/** CB49's insert-only shape — every OTHER `trip_files` column is autoincrement/nullable-defaulted and omitted, matching the legacy statement's own seven-column list (the `NoteAttachmentInsertKyselyDB` precedent, `CollabNotes.repository.ts`). */ +interface MessageAttachmentInsertKyselyDB { + trip_files: { + trip_id: number | string; + message_id: number | string; + filename: string; + original_name: string; + file_size: number; + mime_type: string; + uploaded_by: number; + }; +} + +/** + * `collab_messages` — trip chat, plus (second half of this class) the + * `trip_files` rows a message owns as attachments. Kysely throughout for + * `collab_messages` itself: `trip_id`/`user_id`/`reply_to` are + * `persist(false)` mirrors (the `TripFilesRepository` class docstring's + * trap). + */ +export class CollabMessagesRepository extends TrekRepository { + /** + * CB45/CB50's shared shape — `m.*, u.username, u.avatar, CASE WHEN + * rm.deleted = 1 THEN '' ELSE rm.text END AS reply_text, ru.username AS + * reply_username FROM collab_messages m JOIN users u ON m.user_id = u.id + * LEFT JOIN collab_messages rm ON m.reply_to = rm.id LEFT JOIN users ru ON + * rm.user_id = ru.id`. A genuine self-join (`rm`/`ru` are second aliases + * of `collab_messages`/`users`), expressed as a second Kysely alias rather + * than the ORM's `replyToRef` relation: `CollabMessages.entity.ts` DOES + * have a real `replyToRef` manyToOne(CollabMessages) relation (Task 0's + * dialect finding, `progress.md`), but populating it through the + * QueryBuilder would hydrate a full `CollabMessages` entity for the reply + * (and a SEPARATE query/join for `ru.username`, since `replyToRef` doesn't + * carry the reply's own author's username) rather than the two flat scalar + * columns (`reply_text`, `reply_username`) this statement actually + * projects — the `Reservations.repository.ts#listUpcomingForUser` + * self-join precedent (`reservations as r2`) is the exact same shape: + * Kysely, not a relation walk, for a flat projected self-join. + */ + private joinedQuery() { + return this.kysely() + .selectFrom('collab_messages as m') + .innerJoin('users as u', 'u.id', 'm.user_id') + .leftJoin('collab_messages as rm', 'rm.id', 'm.reply_to') + .leftJoin('users as ru', 'ru.id', 'rm.user_id') + .selectAll('m') + .select((eb) => [ + 'u.username', + 'u.avatar', + eb.case().when('rm.deleted', '=', 1).then('').else(eb.ref('rm.text')).end().as('reply_text'), + 'ru.username as reply_username', + ]); + } + + /** + * CB45 (`listMessages`) — the joined shape above, `WHERE m.trip_id = ?[ + * AND m.id < ?] ORDER BY m.id DESC LIMIT 100`. `before` undefined omits + * the cursor filter entirely (matching the legacy's conditional extra + * param), not `>= 0`/`null` — the same shape the legacy ternary between + * two whole query strings expressed. + */ + async listForTrip(trip_id: number, before?: number): Promise { + let q = this.joinedQuery().where('m.trip_id', '=', trip_id); + if (before !== undefined) q = q.where('m.id', '<', before); + return await q.orderBy('m.id', 'desc').limit(100).execute(); + } + + /** CB50 (`createMessage`'s post-tx re-select) — the joined shape above, `WHERE m.id = ?`, no trip filter (the caller just wrote this exact id inside the transaction). */ + async findWithReply(id: number): Promise { + return await this.joinedQuery().where('m.id', '=', id).executeTakeFirst(); + } + + /** + * CB2/CB51 (`reactMessage`'s existence guard, `deleteMessage`'s + * trip-scoping + owner-check read) — `SELECT * FROM collab_messages WHERE + * id = ? AND trip_id = ?` (CB2's own text is `SELECT id FROM ...`, a + * narrower projection of the SAME WHERE — reading the full row for both + * call sites is behaviourally identical since CB2 only tests truthiness). + * `SELECT *` here has NO `username` column (`collab_messages` alone, no + * join) — `deleteMessage`'s legacy `return { username: message.username + * }` reads a column this statement never selected; see the `CollabService + * .deleteMessage` docstring for how that pre-existing gap is preserved. + */ + async findInTrip(id: number, trip_id: number): Promise { + return await this.kysely().selectFrom('collab_messages').selectAll().where('id', '=', id).where('trip_id', '=', trip_id).executeTakeFirst(); + } + + /** CB47 (`createMessage`'s reply-target validation) — `SELECT id FROM collab_messages WHERE id = ? AND trip_id = ? AND deleted = 0`. A soft-deleted message is gone as far as anyone replying is concerned. */ + async findActiveInTrip(id: number, trip_id: number): Promise<{ id: number } | undefined> { + return await this.kysely().selectFrom('collab_messages').select(['id']).where('id', '=', id).where('trip_id', '=', trip_id).where('deleted', '=', 0).executeTakeFirst(); + } + + /** CB44 (`countMessages`) — `SELECT COUNT(*) as cnt FROM collab_messages WHERE trip_id = ?`. */ + async countForTrip(trip_id: number): Promise { + return await this.count({ trip: trip_id }); + } + + /** CB48 (`createMessage`, inside its transaction) — `INSERT INTO collab_messages (trip_id, user_id, text, reply_to) VALUES (?×4)`. Returns the new row's id. */ + async insertMessage(trip_id: number | string, user_id: number, text: string, reply_to: number | null): Promise { + return await this.insert({ trip: trip_id, user: user_id, text, reply_to }); + } + + /** CB54 (`deleteMessage`, inside its transaction) — `UPDATE collab_messages SET deleted = 1 WHERE id = ?`, soft delete — the row survives for the placeholder. */ + async softDelete(id: number): Promise { + await this.nativeUpdate({ id }, { deleted: 1 }); + } + + // ------------------------------------------------------------------------- + // Message attachments (`trip_files` scoped by `message_id`) — see + // `CollabNotesRepository`'s class docstring for why these live here + // rather than on `TripFilesRepository`. + // ------------------------------------------------------------------------- + + /** CB43 (`formatMessage`'s hydration read) — `SELECT id, trip_id, message_id, filename, original_name, file_size, mime_type FROM trip_files WHERE message_id = ? AND trip_id = ? AND deleted_at IS NULL ORDER BY id ASC`. */ + async listAttachmentsForMessage(message_id: number, trip_id: number): Promise { + return await this.kysely() + .selectFrom('trip_files') + .select(['id', 'trip_id', 'message_id', 'filename', 'original_name', 'file_size', 'mime_type']) + .where('message_id', '=', message_id) + .where('trip_id', '=', trip_id) + .where('deleted_at', 'is', null) + .orderBy('id', 'asc') + .execute(); + } + + /** + * CB49 (`createMessage`, inside its transaction, once per uploaded file) — + * `INSERT INTO trip_files (trip_id, message_id, filename, original_name, + * file_size, mime_type, uploaded_by) VALUES (?×7)`. + */ + async insertAttachmentForMessage(row: { trip_id: number | string; message_id: number | string; filename: string; original_name: string; file_size: number; mime_type: string; uploaded_by: number }): Promise { + await this.kysely() + .insertInto('trip_files') + .values({ + trip_id: row.trip_id, + message_id: row.message_id, + filename: row.filename, + original_name: row.original_name, + file_size: row.file_size, + mime_type: row.mime_type, + uploaded_by: row.uploaded_by, + }) + .execute(); + } + + /** CB52 (`deleteMessage`'s pre-tx storage-cleanup list) — `SELECT filename FROM trip_files WHERE message_id = ? AND trip_id = ?`, run BEFORE the transaction (the legacy ordering). */ + async listFilenamesForMessage(message_id: number, trip_id: number): Promise<{ filename: string }[]> { + return await this.kysely().selectFrom('trip_files').select(['filename']).where('message_id', '=', message_id).where('trip_id', '=', trip_id).execute(); + } + + /** CB53 (`deleteMessage`, inside its transaction, before CB54) — `DELETE FROM trip_files WHERE message_id = ? AND trip_id = ?`. */ + async deleteAttachmentsForMessage(message_id: number, trip_id: number): Promise { + await this.kysely().deleteFrom('trip_files').where('message_id', '=', message_id).where('trip_id', '=', trip_id).execute(); + } +} diff --git a/server/src/db/repositories/CollabNotes.repository.ts b/server/src/db/repositories/CollabNotes.repository.ts index cd6efa861..4a67f5ba2 100644 --- a/server/src/db/repositories/CollabNotes.repository.ts +++ b/server/src/db/repositories/CollabNotes.repository.ts @@ -1,4 +1,256 @@ import type { CollabNotes } from '../entities/CollabNotes.entity'; +import { currentTimestamp } from '../dialect/sql-functions'; +import type { TripFileRow } from './TripFiles.repository'; +import { type AssertRowKeys } from './_shared/rows'; import { TrekRepository } from './_shared/trek-repository'; -export class CollabNotesRepository extends TrekRepository {} +/** A bare `collab_notes` row — every scalar column of the entity, incl. the two `persist(false)` relation mirrors (`trip_id`, `user_id`). */ +export interface CollabNoteRow { + id: number; + trip_id: number; + user_id: number; + category: string | null; + title: string; + content: string | null; + color: string | null; + pinned: number | null; + created_at: string | null; + updated_at: string | null; + website: string | null; +} + +const _collabNoteRowKeys: AssertRowKeys = true; + +/** CB7/CB9/CB12/CB20's joined projection — `n.*, u.username, u.avatar`. */ +export interface CollabNoteJoinRow extends CollabNoteRow { + username: string; + avatar: string | null; +} + +/** CB6's narrow attachment projection (`formatNote`'s hydration read). */ +export interface NoteAttachmentRow { + id: number; + filename: string; + original_name: string; + file_size: number | null; + mime_type: string | null; +} + +interface CollabNotesKyselyDB { + collab_notes: CollabNoteRow; + users: { id: number; username: string; avatar: string | null }; +} + +/** Only the `trip_files` columns the note-attachment methods below touch — see the class docstring's second half. */ +interface NoteAttachmentsKyselyDB { + trip_files: TripFileRow; +} + +/** + * CB18's insert-only shape — `id`/`place_id`/`reservation_id`/`description`/ + * `created_at`/`uploaded_by`/`starred`/`deleted_at`/`message_id` are + * autoincrement/nullable-defaulted and omitted from `.values()` below (the + * legacy statement's own six-column list omits them too), so a SEPARATE + * interface from {@link NoteAttachmentsKyselyDB} rather than widening that + * one's required columns (the `TripAlbumLinksInsertKyselyDB` / + * `FileLinksWriteKyselyDB` precedent: one purpose-shaped Kysely interface per + * statement). + */ +interface NoteAttachmentInsertKyselyDB { + trip_files: { + trip_id: number | string; + note_id: number | string; + filename: string; + original_name: string; + file_size: number; + mime_type: string; + }; +} + +/** + * `collab_notes` — shared trip notes, plus (second half of this class) the + * `trip_files` rows a note owns as attachments. + * + * Kysely throughout for `collab_notes` itself: `trip_id`/`user_id` are + * `persist(false)` mirrors (the `TripFilesRepository` class docstring's + * trap), so a bare QueryBuilder `.select([...])` silently drops them — + * `selectAll()`/a typed column list reads the real DB columns directly. + * + * **Why the note-attachment methods (CB6/CB14/CB15/CB18/CB19/CB21/CB22) live + * here instead of on `TripFilesRepository`.** The inventory's own proposal + * (`plan3e-sql-inventory.md` §4's "Proposed repositories" note) expected + * `TripFilesRepository` to grow `listForNote`/`deleteForNote`/`findScoped`/ + * `listForMessage`/`insertForMessage`/`deleteForMessage`. Task 1 (files) + * landed first (`399ca58a7`) and did NOT add those — its report lists only + * `findInTrip`/`findDeletedInTrip`/`listForTrip`/`findByIdWithJoins`/ + * `listTrashed`/`insertFile`/`updateFile`/`setStarred`/`softDelete`/ + * `restore`/`deleteById`/`deleteMany`, none scoped by `note_id`/`message_id`. + * `TripFiles.repository.ts` is Task 1's named file and this task's brief + * says "you read TripFilesRepository..., you do not edit them" — so rather + * than editing a landed, other-task-owned file, the note-scoped queries are + * expressed here (and the message-scoped twins on `CollabMessages + * .repository.ts`) via `this.kysely()` against the `trip_files` table + * directly, the same "a repository touches more than its own table via + * Kysely" shape `TripFilesRepository.joinedQuery` itself uses for + * `reservations`/`users`. `TripFileRow`'s TYPE is imported (read-only) from + * `TripFiles.repository.ts` for column-shape parity — no write to that file. + * Flagged in the task-5 report as a deviation from the inventory's proposal, + * not a defect. + */ +export class CollabNotesRepository extends TrekRepository { + private joinedQuery() { + return this.kysely() + .selectFrom('collab_notes as n') + .innerJoin('users as u', 'u.id', 'n.user_id') + .selectAll('n') + .select(['u.username', 'u.avatar']); + } + + /** + * CB7 (`CollabService.listNotes`) — `SELECT n.*, u.username, u.avatar FROM + * collab_notes n JOIN users u ON n.user_id = u.id WHERE n.trip_id = ? + * ORDER BY n.pinned DESC, n.updated_at DESC`. `trip_id: number`: the + * service resolves the route's `string | number` id via `toRowId` before + * calling (rule 15), same as every other read below — a malformed id + * short-circuits to the "not found"/empty legacy result without reaching + * this query at all. + */ + async listForTrip(trip_id: number): Promise { + return await this.joinedQuery().where('n.trip_id', '=', trip_id).orderBy('n.pinned', 'desc').orderBy('n.updated_at', 'desc').execute(); + } + + /** CB10/CB13/CB17 (`updateNote`/`deleteNote`/`addNoteFile`'s shared trip-scoping guard) — `SELECT * FROM collab_notes WHERE id = ? AND trip_id = ?`, same text at all three call sites. */ + async findInTrip(id: number, trip_id: number): Promise { + return await this.kysely().selectFrom('collab_notes').selectAll().where('id', '=', id).where('trip_id', '=', trip_id).executeTakeFirst(); + } + + /** CB9/CB12 (`createNote`'s post-insert re-select, `updateNote`'s post-write re-select) — `SELECT n.*, u.username, u.avatar FROM collab_notes n JOIN users u ON n.user_id = u.id WHERE n.id = ?`, no trip filter (the caller already knows `id` is in-scope, having just written it). */ + async findWithUser(id: number): Promise { + return await this.joinedQuery().where('n.id', '=', id).executeTakeFirst(); + } + + /** CB20 (`getFormattedNoteById`) — same shape as {@link findWithUser} plus the trip scope (`n.trip_id = ?`) the legacy statement adds at this one call site (the post-migration hardening the class docstring on `CollabService` describes). */ + async findWithUserInTrip(id: number, trip_id: number): Promise { + return await this.joinedQuery().where('n.id', '=', id).where('n.trip_id', '=', trip_id).executeTakeFirst(); + } + + /** CB8 (`createNote`) — `INSERT INTO collab_notes (trip_id, user_id, title, content, category, color, website, pinned) VALUES (?×8)`. The service resolves the `|| null`/`|| 'General'`/`|| '#6366f1'` defaults before calling (D2's split). Returns the new row's id. */ + async insertNote(row: { trip_id: number | string; user_id: number; title: string; content: string | null; category: string; color: string; website: string | null; pinned: number }): Promise { + return await this.insert({ + trip: row.trip_id, + user: row.user_id, + title: row.title, + content: row.content, + category: row.category, + color: row.color, + website: row.website, + pinned: row.pinned, + }); + } + + /** + * CB11 (`updateNote`) — the 6-column `UPDATE collab_notes SET title = + * COALESCE(?, title), content = CASE WHEN ? THEN ? ELSE content END, + * category = COALESCE(?, category), color = COALESCE(?, color), pinned = + * CASE WHEN ? IS NOT NULL THEN ? ELSE pinned END, website = CASE WHEN ? + * THEN ? ELSE website END, updated_at = CURRENT_TIMESTAMP WHERE id = ?`. + * + * The `Reservations.repository.ts#updateReservation` / `PlacesRepository + * #updatePlace` precedent (RS41/PL11): the SERVICE resolves every + * COALESCE-keep and presence-sentinel column to its FINAL value first (it + * already holds the pre-image via `findInTrip`'s CB10 read every caller + * makes) — this method just writes what it is handed, no SQL-side + * COALESCE/CASE. R11 names a shared "presence-sentinel helper" other Plan + * 3e tasks (2/3/4) should copy; at the time this task ran, Task 2 (budget) + * had not landed yet (`progress.md`'s task table), so there was no landed + * helper to copy — this repeats the ALREADY-LANDED RS41/PL11 read-merge- + * write shape from Plans 3c/3d directly instead of inventing a new raw-SQL + * `CASE` helper, and this class docstring records that choice for + * whichever task reads it next. + */ + async update(id: number, write: { title: string; content: string | null; category: string | null; color: string | null; pinned: number | null; website: string | null }): Promise { + const platform = this.getEntityManager().getPlatform(); + await this.nativeUpdate({ id }, { + title: write.title, + content: write.content, + category: write.category, + color: write.color, + pinned: write.pinned, + website: write.website, + updated_at: currentTimestamp(platform), + }); + } + + /** CB16 (`deleteNote`, inside its transaction, after CB15's attachment sweep) — `DELETE FROM collab_notes WHERE id = ?`. */ + async delete(id: number): Promise { + await this.nativeDelete({ id }); + } + + // ------------------------------------------------------------------------- + // Note attachments (`trip_files` scoped by `note_id`) — see the class + // docstring's second half for why these live here rather than on + // `TripFilesRepository`. + // ------------------------------------------------------------------------- + + /** CB6 (`formatNote`'s hydration read) — `SELECT id, filename, original_name, file_size, mime_type FROM trip_files WHERE note_id = ?`. */ + async listAttachmentsForNote(note_id: number): Promise { + return await this.kysely() + .selectFrom('trip_files') + .select(['id', 'filename', 'original_name', 'file_size', 'mime_type']) + .where('note_id', '=', note_id) + .execute(); + } + + /** CB14 (`deleteNote`'s pre-tx storage-cleanup list) — `SELECT id, filename FROM trip_files WHERE note_id = ?`, run BEFORE the transaction (the legacy ordering: the storage deletes happen outside/before the DB transaction). */ + async listFilenamesForNote(note_id: number): Promise<{ id: number; filename: string }[]> { + return await this.kysely().selectFrom('trip_files').select(['id', 'filename']).where('note_id', '=', note_id).execute(); + } + + /** CB15 (`deleteNote`, inside its transaction, before CB16) — `DELETE FROM trip_files WHERE note_id = ?`. */ + async deleteAttachmentsForNote(note_id: number): Promise { + await this.kysely().deleteFrom('trip_files').where('note_id', '=', note_id).execute(); + } + + /** + * CB18 (`addNoteFile`'s insert half) — `INSERT INTO trip_files (trip_id, + * note_id, filename, original_name, file_size, mime_type) VALUES (?×6)`. + * Returns the new row's id (Kysely's `InsertResult.insertId`, this + * dialect's `lastInsertRowid` equivalent) for CB19's re-select. + */ + async insertAttachmentForNote(row: { trip_id: number | string; note_id: number | string; filename: string; original_name: string; file_size: number; mime_type: string }): Promise { + const result = await this.kysely() + .insertInto('trip_files') + .values({ trip_id: row.trip_id, note_id: row.note_id, filename: row.filename, original_name: row.original_name, file_size: row.file_size, mime_type: row.mime_type }) + .executeTakeFirstOrThrow(); + return Number(result.insertId); + } + + /** CB19 (`addNoteFile`'s re-select, immediately after {@link insertAttachmentForNote}) — `SELECT * FROM trip_files WHERE id = ?`, unscoped (the caller just wrote this exact id). */ + async findAttachmentById(id: number): Promise { + return await this.kysely().selectFrom('trip_files').selectAll().where('id', '=', id).executeTakeFirst(); + } + + /** + * CB21 (`deleteNoteFile`) — `SELECT * FROM trip_files WHERE id = ? AND + * note_id = ? AND trip_id = ?`. **IDOR guard, security-critical**: all + * three scoping columns in ONE query, never split into separate existence + * checks a caller could satisfy independently (the legacy doc comment's + * own warning, task-5-brief.md's explicit instruction) — this ties the + * deleted object to the URL's `:tripId` the controller already + * access-checked, not just to a note/file id an attacker can enumerate. + */ + async findScopedForNote(id: number, note_id: number, trip_id: number): Promise { + return await this.kysely() + .selectFrom('trip_files') + .selectAll() + .where('id', '=', id) + .where('note_id', '=', note_id) + .where('trip_id', '=', trip_id) + .executeTakeFirst(); + } + + /** CB22 (`deleteNoteFile`, after the IDOR-guarded read and the storage delete) — `DELETE FROM trip_files WHERE id = ?`. */ + async deleteAttachmentById(id: number): Promise { + await this.kysely().deleteFrom('trip_files').where('id', '=', id).execute(); + } +} diff --git a/server/src/db/repositories/CollabPollVotes.repository.ts b/server/src/db/repositories/CollabPollVotes.repository.ts index 4f08d44f6..674f5e503 100644 --- a/server/src/db/repositories/CollabPollVotes.repository.ts +++ b/server/src/db/repositories/CollabPollVotes.repository.ts @@ -1,4 +1,51 @@ import type { CollabPollVotes } from '../entities/CollabPollVotes.entity'; import { TrekRepository } from './_shared/trek-repository'; -export class CollabPollVotesRepository extends TrekRepository {} +/** CB24's joined projection — `v.option_index, v.user_id, u.username, u.avatar`. */ +export interface PollVoteJoinRow { + option_index: number; + user_id: number; + username: string; + avatar: string | null; +} + +export class CollabPollVotesRepository extends TrekRepository { + /** + * CB24 (`getPollWithVotes`'s votes half) — `SELECT v.option_index, + * v.user_id, u.username, u.avatar FROM collab_poll_votes v JOIN users u ON + * v.user_id = u.id WHERE v.poll_id = ?`. + */ + async listForPoll(poll_id: number): Promise { + return this.qb('v') + .join('v.user', 'u') + .select(['v.option_index', 'u.id as user_id', 'u.username', 'u.avatar']) + .where({ poll: poll_id }) + .execute('all', false); + } + + /** + * CB28 (`votePoll`'s existing-vote check) — `SELECT id FROM + * collab_poll_votes WHERE poll_id = ? AND user_id = ? AND option_index = + * ?`. Named `findVote`, not `find`: `find` is `EntityRepository`'s own + * base method (a different, incompatible signature) and a same-named + * override does not typecheck. + */ + async findVote(poll_id: number, user_id: number, option_index: number): Promise<{ id: number } | null> { + return this.findOne({ poll: poll_id, user: user_id, option_index }, { fields: ['id'] }); + } + + /** CB29 (`votePoll`'s toggle-off half, NOT inside the transaction — matching the legacy: only the multi-vote-clear branch below is transacted) — `DELETE FROM collab_poll_votes WHERE id = ?`. */ + async deleteById(id: number): Promise { + await this.nativeDelete({ id }); + } + + /** CB30 (`votePoll`'s single-choice clear, inside the transaction, only when `!poll.multiple`) — `DELETE FROM collab_poll_votes WHERE poll_id = ? AND user_id = ?`. */ + async deleteForUser(poll_id: number, user_id: number): Promise { + await this.nativeDelete({ poll: poll_id, user: user_id }); + } + + /** CB31 (`votePoll`, inside the transaction) — `INSERT INTO collab_poll_votes (poll_id, user_id, option_index) VALUES (?, ?, ?)`, the entity's own composite-unique `(poll, user, option_index)`. */ + async insertVote(poll_id: number, user_id: number, option_index: number): Promise { + await this.insert({ poll: poll_id, user: user_id, option_index }); + } +} diff --git a/server/src/db/repositories/CollabPolls.repository.ts b/server/src/db/repositories/CollabPolls.repository.ts index 764becfd6..dfa1434d2 100644 --- a/server/src/db/repositories/CollabPolls.repository.ts +++ b/server/src/db/repositories/CollabPolls.repository.ts @@ -1,4 +1,77 @@ import type { CollabPolls } from '../entities/CollabPolls.entity'; +import { type AssertRowKeys } from './_shared/rows'; import { TrekRepository } from './_shared/trek-repository'; -export class CollabPollsRepository extends TrekRepository {} +/** A bare `collab_polls` row — every scalar column of the entity, incl. the two `persist(false)` relation mirrors (`trip_id`, `user_id`). */ +export interface CollabPollRow { + id: number; + trip_id: number; + user_id: number; + question: string; + options: string; + multiple: number | null; + closed: number | null; + deadline: string | null; + created_at: string | null; +} + +const _collabPollRowKeys: AssertRowKeys = true; + +/** CB23's joined projection — `p.*, u.username, u.avatar`. */ +export interface CollabPollJoinRow extends CollabPollRow { + username: string; + avatar: string | null; +} + +interface CollabPollsKyselyDB { + collab_polls: CollabPollRow; + users: { id: number; username: string; avatar: string | null }; +} + +/** + * `collab_polls` — decision polls. Kysely throughout: `trip_id`/`user_id` + * are `persist(false)` mirrors (the `TripFilesRepository` class docstring's + * trap), so a bare QueryBuilder `.select([...])` silently drops them. + */ +export class CollabPollsRepository extends TrekRepository { + /** CB23 (`getPollWithVotes`'s poll half) — `SELECT p.*, u.username, u.avatar FROM collab_polls p JOIN users u ON p.user_id = u.id WHERE p.id = ?`. `id: number`: the service resolves the route's `string | number` id via `toRowId` before calling (rule 15). */ + async findWithUser(id: number): Promise { + return await this.kysely() + .selectFrom('collab_polls as p') + .innerJoin('users as u', 'u.id', 'p.user_id') + .selectAll('p') + .select(['u.username', 'u.avatar']) + .where('p.id', '=', id) + .executeTakeFirst(); + } + + /** CB25 (`listPolls`) — `SELECT id FROM collab_polls WHERE trip_id = ? ORDER BY created_at DESC`. */ + async listIdsForTrip(trip_id: number): Promise { + const rows = await this.kysely().selectFrom('collab_polls').select(['id']).where('trip_id', '=', trip_id).orderBy('created_at', 'desc').execute(); + return rows.map((r) => r.id); + } + + /** + * CB27/CB32/CB34 (`votePoll`/`closePoll`/`deletePoll`'s shared trip-scoping + * guard) — `SELECT * FROM collab_polls WHERE id = ? AND trip_id = ?`, same + * text at all three call sites. + */ + async findInTrip(id: number, trip_id: number): Promise { + return await this.kysely().selectFrom('collab_polls').selectAll().where('id', '=', id).where('trip_id', '=', trip_id).executeTakeFirst(); + } + + /** CB26 (`createPoll`) — `INSERT INTO collab_polls (trip_id, user_id, question, options, multiple, deadline) VALUES (?×6)`. Returns the new row's id. */ + async insertPoll(row: { trip_id: number | string; user_id: number; question: string; options: string; multiple: number; deadline: string | null }): Promise { + return await this.insert({ trip: row.trip_id, user: row.user_id, question: row.question, options: row.options, multiple: row.multiple, deadline: row.deadline }); + } + + /** CB33 (`closePoll`) — `UPDATE collab_polls SET closed = 1 WHERE id = ?`. */ + async close(id: number): Promise { + await this.nativeUpdate({ id }, { closed: 1 }); + } + + /** CB35 (`deletePoll`) — `DELETE FROM collab_polls WHERE id = ?`. */ + async delete(id: number): Promise { + await this.nativeDelete({ id }); + } +} diff --git a/server/src/nest/collab/collab.module.ts b/server/src/nest/collab/collab.module.ts index e876e5262..30004f459 100644 --- a/server/src/nest/collab/collab.module.ts +++ b/server/src/nest/collab/collab.module.ts @@ -1,4 +1,5 @@ import { Module } from '@nestjs/common'; +import { MikroOrmModule } from '@mikro-orm/nestjs'; import { MulterModule } from '@nestjs/platform-express'; import { CollabController, collabNoteFileFilter, MAX_NOTE_FILE_SIZE } from './collab.controller'; import { CollabService } from './collab.service'; @@ -15,6 +16,13 @@ import { StorageModule } from '../storage/storage.module'; import { StorageService } from '../storage/storage.service'; import { buildStorageUploadOptions } from '../storage/storage-upload.factory'; import { RateLimitModule } from '../common/rate-limit.module'; +import { CollabNotes } from '../../db/entities/CollabNotes.entity'; +import { CollabMessageReactions } from '../../db/entities/CollabMessageReactions.entity'; +import { CollabPolls } from '../../db/entities/CollabPolls.entity'; +import { CollabPollVotes } from '../../db/entities/CollabPollVotes.entity'; +import { CollabLinks } from '../../db/entities/CollabLinks.entity'; +import { CollabMessages } from '../../db/entities/CollabMessages.entity'; +import { Trips } from '../../db/entities/Trips.entity'; @Module({ imports: [ @@ -30,6 +38,11 @@ import { RateLimitModule } from '../common/rate-limit.module'; }), }), StorageModule, + // The six collab-owned tables (Plan 3e Task 5) plus `Trips` (additive — + // `CollabService.notifyCollab`'s CB55 `getTitle` read only, the same + // "registered here only for one read, never the owning module" shape + // `FilesModule`'s own forFeature list documents). + MikroOrmModule.forFeature([CollabNotes, CollabMessageReactions, CollabPolls, CollabPollVotes, CollabLinks, CollabMessages, Trips]), McpSharedModule, NotificationsModule, PermissionsModule, AuthModule, RealtimeModule, PluginGuardsModule, AddonsModule, RateLimitModule], controllers: [CollabController], providers: [CollabService, CollabMcp, CollabRpc], diff --git a/server/src/nest/collab/collab.service.ts b/server/src/nest/collab/collab.service.ts index 990685a4c..44b0ffda1 100644 --- a/server/src/nest/collab/collab.service.ts +++ b/server/src/nest/collab/collab.service.ts @@ -1,5 +1,6 @@ import path from 'path'; import { Injectable } from '@nestjs/common'; +import { InjectRepository } from '@mikro-orm/nestjs'; import { DatabaseService, type TripAccess } from '../database/database.service'; import type { TrekWsPayload, TrekWsTripEventName } from '@trek/shared'; import { RealtimeService } from '../realtime/realtime.service'; @@ -7,11 +8,26 @@ import { PermissionsService } from '../permissions/permissions.service'; import { avatarUrl } from '../common/avatarUrl'; import { checkSsrf, createPinnedDispatcher } from '../../utils/ssrfGuard'; import { discardBody, exceedsDeclaredLength, readCappedText } from '../../utils/cappedFetch'; -import type { CollabNote, CollabPoll, CollabMessage, TripFile, User } from '../../types'; +import type { User } from '../../types'; import { NotificationsService } from '../notifications/notifications.service'; import { StorageService } from '../storage/storage.service'; import { RateLimitService } from '../common/rate-limit.service'; import { UnitOfWork } from '../database/unit-of-work'; +import { toRowId } from '../common/row-id'; +import { CollabNotes } from '../../db/entities/CollabNotes.entity'; +import type { CollabNotesRepository, CollabNoteJoinRow } from '../../db/repositories/CollabNotes.repository'; +import { CollabMessageReactions } from '../../db/entities/CollabMessageReactions.entity'; +import type { CollabMessageReactionsRepository } from '../../db/repositories/CollabMessageReactions.repository'; +import { CollabPolls } from '../../db/entities/CollabPolls.entity'; +import type { CollabPollsRepository } from '../../db/repositories/CollabPolls.repository'; +import { CollabPollVotes } from '../../db/entities/CollabPollVotes.entity'; +import type { CollabPollVotesRepository } from '../../db/repositories/CollabPollVotes.repository'; +import { CollabLinks } from '../../db/entities/CollabLinks.entity'; +import type { CollabLinksRepository } from '../../db/repositories/CollabLinks.repository'; +import { CollabMessages } from '../../db/entities/CollabMessages.entity'; +import type { CollabMessagesRepository, CollabMessageJoinRow } from '../../db/repositories/CollabMessages.repository'; +import { Trips } from '../../db/entities/Trips.entity'; +import type { TripsRepository } from '../../db/repositories/Trips.repository'; type Trip = TripAccess; @@ -33,14 +49,6 @@ export interface PollVoteRow { avatar: string | null; } -export interface NoteFileRow { - id: number; - filename: string; - original_name?: string; - file_size?: number; - mime_type?: string; -} - export interface GroupedReaction { emoji: string; users: { user_id: number; username: string }[]; @@ -106,19 +114,34 @@ function scrapeOpenGraph(html: string): Omit { } /** - * Collab domain service — owns the collab SQL (moved from the legacy - * services/collabService.ts: the `||` falsy-coercion defaults, the mixed - * COALESCE/CASE update, the post-write re-selects and the sentinel error - * strings). Trip access, the 'collab_edit' / 'file_upload' permissions and the - * WebSocket broadcast keep their legacy call paths. Post-migration hardening - * on top of the 1:1 move: the multi-statement writes (deleteNote, votePoll) - * run through UnitOfWork.transactional(), getFormattedNoteById is trip-scoped - * and null-safe, - * votePoll rejects non-integer indexes, and linkPreview absorbs malformed URLs - * instead of throwing. - * All consumers are in-container since the trip fold (TripsService and - * TripsMcp inject this class); collab.bridge.ts was deleted with its last - * outside-container consumers. + * Collab domain service — owns the collab SQL, now through the six + * `Collab*Repository` classes (Plan 3e Task 5, CB1-55) plus `TripsRepository + * .getTitle` (CB55) and `TripFiles`-scoped note/message attachment methods + * that live on `CollabNotesRepository`/`CollabMessagesRepository` themselves + * (see those classes' docstrings for why — Task 1's landed + * `TripFilesRepository` doesn't have note/message-scoped methods). Trip + * access, the 'collab_edit' / 'file_upload' permissions and the WebSocket + * broadcast keep their legacy call paths. Post-migration hardening carried + * over from the original DI move: the multi-statement writes (deleteNote, + * votePoll's multi-choice-clear branch, createMessage, deleteMessage) run + * through UnitOfWork.transactional(), getFormattedNoteById is trip-scoped + * and null-safe, votePoll rejects non-integer indexes, and linkPreview + * absorbs malformed URLs instead of throwing. + * + * **Read-then-write id seam (rule 21).** Every guard read below (`findInTrip` + * / `findScoped...`) returns the row's OWN typed `id` (a `number`), and every + * subsequent repository call in that method reuses THAT id rather than + * re-deriving one from the raw `string | number` route param — the same + * numeric value feeds both the trip-scoping SELECT and the write that + * follows it. + * + * **`deleteMessage`'s pre-existing `username` gap.** The legacy statement + * behind `findInTrip` (CB51) is `SELECT * FROM collab_messages WHERE id = ? + * AND trip_id = ?` — no join to `users` — yet the legacy code returned + * `{ username: message.username }`, a column that statement never selected. + * That has always evaluated to `undefined` at runtime; preserved exactly + * below (parity is law), not fixed — flagged in the task-5 report as a + * pre-existing defect for a ruling, the same as `§17`'s other "surprises". */ @Injectable() export class CollabService { @@ -130,6 +153,13 @@ export class CollabService { private readonly storage: StorageService, private readonly rateLimit: RateLimitService, private readonly uow: UnitOfWork, + @InjectRepository(CollabMessageReactions) private readonly messageReactionsRepo: CollabMessageReactionsRepository, + @InjectRepository(CollabNotes) private readonly notesRepo: CollabNotesRepository, + @InjectRepository(CollabPolls) private readonly pollsRepo: CollabPollsRepository, + @InjectRepository(CollabPollVotes) private readonly pollVotesRepo: CollabPollVotesRepository, + @InjectRepository(CollabLinks) private readonly linksRepo: CollabLinksRepository, + @InjectRepository(CollabMessages) private readonly messagesRepo: CollabMessagesRepository, + @InjectRepository(Trips) private readonly tripsRepo: TripsRepository, ) {} /** @@ -166,13 +196,8 @@ export class CollabService { /* Reactions */ /* ------------------------------------------------------------------ */ - private async loadReactions(messageId: number | string): Promise { - return this.db.all(` - SELECT r.emoji, r.user_id, u.username - FROM collab_message_reactions r - JOIN users u ON r.user_id = u.id - WHERE r.message_id = ? - `, messageId); + private async loadReactions(messageId: number): Promise { + return this.messageReactionsRepo.listForMessage(messageId); } private groupReactions(reactions: ReactionRow[]): GroupedReaction[] { @@ -185,25 +210,28 @@ export class CollabService { } async reactMessage(messageId: number | string, tripId: number | string, userId: number, emoji: string): Promise<{ found: boolean; reactions: GroupedReaction[] }> { - const msg = this.db.get('SELECT id FROM collab_messages WHERE id = ? AND trip_id = ?', messageId, tripId); + const idNum = toRowId(messageId); + const tripIdNum = toRowId(tripId); + if (idNum === null || tripIdNum === null) return { found: false, reactions: [] }; + const msg = await this.messagesRepo.findInTrip(idNum, tripIdNum); if (!msg) return { found: false, reactions: [] }; - const existing = this.db.get<{ id: number }>('SELECT id FROM collab_message_reactions WHERE message_id = ? AND user_id = ? AND emoji = ?', messageId, userId, emoji); + const existing = await this.messageReactionsRepo.findReaction(msg.id, userId, emoji); if (existing) { - this.db.run('DELETE FROM collab_message_reactions WHERE id = ?', existing.id); + await this.messageReactionsRepo.deleteById(existing.id); } else { - this.db.run('INSERT INTO collab_message_reactions (message_id, user_id, emoji) VALUES (?, ?, ?)', messageId, userId, emoji); + await this.messageReactionsRepo.insertReaction(msg.id, userId, emoji); } - return { found: true, reactions: this.groupReactions(await this.loadReactions(messageId)) }; + return { found: true, reactions: this.groupReactions(await this.loadReactions(msg.id)) }; } /* ------------------------------------------------------------------ */ /* Notes */ /* ------------------------------------------------------------------ */ - private async formatNote(note: CollabNote) { - const attachments = this.db.all('SELECT id, filename, original_name, file_size, mime_type FROM trip_files WHERE note_id = ?', note.id); + private async formatNote(note: CollabNoteJoinRow) { + const attachments = await this.notesRepo.listAttachmentsForNote(note.id); return { ...note, avatar_url: avatarUrl(note), @@ -212,77 +240,65 @@ export class CollabService { } async listNotes(tripId: string | number) { - const notes = this.db.all(` - SELECT n.*, u.username, u.avatar - FROM collab_notes n - JOIN users u ON n.user_id = u.id - WHERE n.trip_id = ? - ORDER BY n.pinned DESC, n.updated_at DESC - `, tripId); - + const notes = await this.notesRepo.listForTrip(toRowId(tripId) ?? -1); return Promise.all(notes.map(note => this.formatNote(note))); } async createNote(tripId: string | number, userId: number, data: { title: string; content?: string | null; category?: string | null; color?: string | null; website?: string | null; pinned?: boolean }) { const pinned = data.pinned ? 1 : 0; - const result = this.db.run(` - INSERT INTO collab_notes (trip_id, user_id, title, content, category, color, website, pinned) - VALUES (?, ?, ?, ?, ?, ?, ?, ?) - `, tripId, userId, data.title, data.content || null, data.category || 'General', data.color || '#6366f1', data.website || null, pinned); - - const note = this.db.get(` - SELECT n.*, u.username, u.avatar FROM collab_notes n JOIN users u ON n.user_id = u.id WHERE n.id = ? - `, result.lastInsertRowid)!; + const id = await this.notesRepo.insertNote({ + trip_id: tripId, + user_id: userId, + title: data.title, + content: data.content || null, + category: data.category || 'General', + color: data.color || '#6366f1', + website: data.website || null, + pinned, + }); + const note = (await this.notesRepo.findWithUser(id))!; return this.formatNote(note); } async updateNote(tripId: string | number, noteId: string | number, data: { title?: string; content?: string | null; category?: string | null; color?: string | null; pinned?: number | boolean; website?: string | null }): Promise> | null> { - const existing = this.db.get('SELECT * FROM collab_notes WHERE id = ? AND trip_id = ?', noteId, tripId); + const idNum = toRowId(noteId); + const tripIdNum = toRowId(tripId); + if (idNum === null || tripIdNum === null) return null; + const existing = await this.notesRepo.findInTrip(idNum, tripIdNum); if (!existing) return null; - this.db.run(` - UPDATE collab_notes SET - title = COALESCE(?, title), - content = CASE WHEN ? THEN ? ELSE content END, - category = COALESCE(?, category), - color = COALESCE(?, color), - pinned = CASE WHEN ? IS NOT NULL THEN ? ELSE pinned END, - website = CASE WHEN ? THEN ? ELSE website END, - updated_at = CURRENT_TIMESTAMP - WHERE id = ? - `, - data.title || null, - data.content !== undefined ? 1 : 0, data.content !== undefined ? data.content : null, - data.category || null, - data.color || null, - data.pinned !== undefined ? 1 : null, data.pinned ? 1 : 0, - data.website !== undefined ? 1 : 0, data.website !== undefined ? data.website : null, - noteId - ); - - const note = this.db.get(` - SELECT n.*, u.username, u.avatar FROM collab_notes n JOIN users u ON n.user_id = u.id WHERE n.id = ? - `, noteId)!; + await this.notesRepo.update(existing.id, { + title: data.title || existing.title, + content: data.content !== undefined ? data.content : existing.content, + category: data.category || existing.category, + color: data.color || existing.color, + pinned: data.pinned !== undefined ? (data.pinned ? 1 : 0) : existing.pinned, + website: data.website !== undefined ? data.website : existing.website, + }); + const note = (await this.notesRepo.findWithUser(existing.id))!; return this.formatNote(note); } async deleteNote(tripId: string | number, noteId: string | number): Promise { - const existing = this.db.get('SELECT id FROM collab_notes WHERE id = ? AND trip_id = ?', noteId, tripId); + const idNum = toRowId(noteId); + const tripIdNum = toRowId(tripId); + if (idNum === null || tripIdNum === null) return false; + const existing = await this.notesRepo.findInTrip(idNum, tripIdNum); if (!existing) return false; // Clean up attached objects first (delete-first is intentional — a failed // row delete leaves dangling rows, never orphaned files). basename() // tolerates any legacy 'files/'-prefixed row the boot migration has not // seen. - const noteFiles = this.db.all('SELECT id, filename FROM trip_files WHERE note_id = ?', noteId); + const noteFiles = await this.notesRepo.listFilenamesForNote(existing.id); for (const f of noteFiles) { await this.storage.delete('files', path.basename(f.filename)).catch(() => { /* ignore */ }); } await this.uow.transactional(async () => { - this.db.run('DELETE FROM trip_files WHERE note_id = ?', noteId); - this.db.run('DELETE FROM collab_notes WHERE id = ?', noteId); + await this.notesRepo.deleteAttachmentsForNote(existing.id); + await this.notesRepo.delete(existing.id); }); return true; } @@ -291,21 +307,31 @@ export class CollabService { /* Note files */ /* ------------------------------------------------------------------ */ - async addNoteFile(tripId: string | number, noteId: string | number, file: { filename: string; originalname: string; size: number; mimetype: string }): Promise<{ file: TripFile & { url: string } } | null> { - const note = this.db.get('SELECT id FROM collab_notes WHERE id = ? AND trip_id = ?', noteId, tripId); + async addNoteFile(tripId: string | number, noteId: string | number, file: { filename: string; originalname: string; size: number; mimetype: string }) { + const idNum = toRowId(noteId); + const tripIdNum = toRowId(tripId); + if (idNum === null || tripIdNum === null) return null; + const note = await this.notesRepo.findInTrip(idNum, tripIdNum); if (!note) return null; - const result = this.db.run( - 'INSERT INTO trip_files (trip_id, note_id, filename, original_name, file_size, mime_type) VALUES (?, ?, ?, ?, ?, ?)', - tripId, noteId, file.filename, file.originalname, file.size, file.mimetype - ); + const insertedId = await this.notesRepo.insertAttachmentForNote({ + trip_id: tripId, + note_id: note.id, + filename: file.filename, + original_name: file.originalname, + file_size: file.size, + mime_type: file.mimetype, + }); - const saved = this.db.get('SELECT * FROM trip_files WHERE id = ?', result.lastInsertRowid)!; + const saved = (await this.notesRepo.findAttachmentById(insertedId))!; return { file: { ...saved, url: `/api/trips/${tripId}/files/${saved.id}/download` } }; } async getFormattedNoteById(tripId: string | number, noteId: string | number) { - const note = this.db.get('SELECT n.*, u.username, u.avatar FROM collab_notes n JOIN users u ON n.user_id = u.id WHERE n.id = ? AND n.trip_id = ?', noteId, tripId); + const idNum = toRowId(noteId); + const tripIdNum = toRowId(tripId); + if (idNum === null || tripIdNum === null) return null; + const note = await this.notesRepo.findWithUserInTrip(idNum, tripIdNum); if (!note) return null; return this.formatNote(note); } @@ -314,13 +340,19 @@ export class CollabService { // Scope to the trip — like every sibling collab op — so a caller authorized for THEIR // trip can't delete a note-file that belongs to someone else's trip (IDOR). trip_files // carries trip_id, so this ties the deleted object to the URL's :tripId the controller - // access-checked, not just to a note/file id an attacker can enumerate. - const file = this.db.get('SELECT * FROM trip_files WHERE id = ? AND note_id = ? AND trip_id = ?', fileId, noteId, tripId); + // access-checked, not just to a note/file id an attacker can enumerate. All THREE ids + // (`fileId`, `noteId`, `tripId`) are resolved via `toRowId` here and fed, unchanged, + // into the one three-column guard query below — never re-derived separately. + const fileIdNum = toRowId(fileId); + const noteIdNum = toRowId(noteId); + const tripIdNum = toRowId(tripId); + if (fileIdNum === null || noteIdNum === null || tripIdNum === null) return false; + const file = await this.notesRepo.findScopedForNote(fileIdNum, noteIdNum, tripIdNum); if (!file) return false; await this.storage.delete('files', path.basename(file.filename)).catch(() => { /* ignore */ }); - this.db.run('DELETE FROM trip_files WHERE id = ?', fileId); + await this.notesRepo.deleteAttachmentById(file.id); return true; } @@ -328,24 +360,13 @@ export class CollabService { /* Polls */ /* ------------------------------------------------------------------ */ - private async getPollWithVotes(pollId: number | bigint | string) { - const poll = this.db.get(` - SELECT p.*, u.username, u.avatar - FROM collab_polls p - JOIN users u ON p.user_id = u.id - WHERE p.id = ? - `, pollId); - + private async getPollWithVotes(pollId: number) { + const poll = await this.pollsRepo.findWithUser(pollId); if (!poll) return null; const options: (string | { label: string })[] = JSON.parse(poll.options); - const votes = this.db.all(` - SELECT v.option_index, v.user_id, u.username, u.avatar - FROM collab_poll_votes v - JOIN users u ON v.user_id = u.id - WHERE v.poll_id = ? - `, pollId); + const votes = await this.pollVotesRepo.listForPoll(poll.id); const formattedOptions = options.map((label: string | { label: string }, idx: number) => { const text = typeof label === 'string' ? label : label.label || label; @@ -369,26 +390,30 @@ export class CollabService { } async listPolls(tripId: string | number) { - const rows = this.db.all<{ id: number }>(` - SELECT id FROM collab_polls WHERE trip_id = ? ORDER BY created_at DESC - `, tripId); - - return (await Promise.all(rows.map(row => this.getPollWithVotes(row.id)))).filter(Boolean); + const ids = await this.pollsRepo.listIdsForTrip(toRowId(tripId) ?? -1); + return (await Promise.all(ids.map(id => this.getPollWithVotes(id)))).filter(Boolean); } async createPoll(tripId: string | number, userId: number, data: { question: string; options: unknown[]; multiple?: boolean; multiple_choice?: boolean; deadline?: string }) { const isMultiple = data.multiple || data.multiple_choice; - const result = this.db.run(` - INSERT INTO collab_polls (trip_id, user_id, question, options, multiple, deadline) - VALUES (?, ?, ?, ?, ?, ?) - `, tripId, userId, data.question, JSON.stringify(data.options), isMultiple ? 1 : 0, data.deadline || null); + const id = await this.pollsRepo.insertPoll({ + trip_id: tripId, + user_id: userId, + question: data.question, + options: JSON.stringify(data.options), + multiple: isMultiple ? 1 : 0, + deadline: data.deadline || null, + }); - return this.getPollWithVotes(result.lastInsertRowid); + return this.getPollWithVotes(id); } async votePoll(tripId: string | number, pollId: string | number, userId: number, optionIndex: number): Promise<{ error?: string; poll?: Awaited> }> { - const poll = this.db.get('SELECT * FROM collab_polls WHERE id = ? AND trip_id = ?', pollId, tripId); + const idNum = toRowId(pollId); + const tripIdNum = toRowId(tripId); + if (idNum === null || tripIdNum === null) return { error: 'not_found' }; + const poll = await this.pollsRepo.findInTrip(idNum, tripIdNum); if (!poll) return { error: 'not_found' }; if (poll.closed) return { error: 'closed' }; @@ -397,38 +422,41 @@ export class CollabService { return { error: 'invalid_index' }; } - const existingVote = this.db.get<{ id: number }>( - 'SELECT id FROM collab_poll_votes WHERE poll_id = ? AND user_id = ? AND option_index = ?', - pollId, userId, optionIndex - ); + const existingVote = await this.pollVotesRepo.findVote(poll.id, userId, optionIndex); if (existingVote) { - this.db.run('DELETE FROM collab_poll_votes WHERE id = ?', existingVote.id); + await this.pollVotesRepo.deleteById(existingVote.id); } else { await this.uow.transactional(async () => { if (!poll.multiple) { - this.db.run('DELETE FROM collab_poll_votes WHERE poll_id = ? AND user_id = ?', pollId, userId); + await this.pollVotesRepo.deleteForUser(poll.id, userId); } - this.db.run('INSERT INTO collab_poll_votes (poll_id, user_id, option_index) VALUES (?, ?, ?)', pollId, userId, optionIndex); + await this.pollVotesRepo.insertVote(poll.id, userId, optionIndex); }); } - return { poll: await this.getPollWithVotes(pollId) }; + return { poll: await this.getPollWithVotes(poll.id) }; } async closePoll(tripId: string | number, pollId: string | number): Promise> | null> { - const poll = this.db.get('SELECT * FROM collab_polls WHERE id = ? AND trip_id = ?', pollId, tripId); + const idNum = toRowId(pollId); + const tripIdNum = toRowId(tripId); + if (idNum === null || tripIdNum === null) return null; + const poll = await this.pollsRepo.findInTrip(idNum, tripIdNum); if (!poll) return null; - this.db.run('UPDATE collab_polls SET closed = 1 WHERE id = ?', pollId); - return this.getPollWithVotes(pollId); + await this.pollsRepo.close(poll.id); + return this.getPollWithVotes(poll.id); } async deletePoll(tripId: string | number, pollId: string | number): Promise { - const poll = this.db.get('SELECT id FROM collab_polls WHERE id = ? AND trip_id = ?', pollId, tripId); + const idNum = toRowId(pollId); + const tripIdNum = toRowId(tripId); + if (idNum === null || tripIdNum === null) return false; + const poll = await this.pollsRepo.findInTrip(idNum, tripIdNum); if (!poll) return false; - this.db.run('DELETE FROM collab_polls WHERE id = ?', pollId); + await this.pollsRepo.delete(poll.id); return true; } @@ -437,58 +465,50 @@ export class CollabService { /* ------------------------------------------------------------------ */ async listLinks(tripId: string | number) { - return this.db.all( - `SELECT l.*, u.username FROM collab_links l JOIN users u ON u.id = l.user_id - WHERE l.trip_id = ? ORDER BY l.pinned DESC, l.created_at DESC`, - tripId, - ); + return this.linksRepo.listForTrip(toRowId(tripId) ?? -1); } async createLink(tripId: string | number, userId: number, data: { title: string; url: string; pinned?: boolean }) { - const result = this.db.run( - 'INSERT INTO collab_links (trip_id,user_id,title,url,pinned) VALUES (?,?,?,?,?)', - tripId, userId, data.title.trim(), data.url.trim(), data.pinned ? 1 : 0, - ); - return this.db.get('SELECT l.*, u.username FROM collab_links l JOIN users u ON u.id = l.user_id WHERE l.id = ?', result.lastInsertRowid); + const id = await this.linksRepo.insertLink({ trip_id: tripId, user_id: userId, title: data.title.trim(), url: data.url.trim(), pinned: data.pinned ? 1 : 0 }); + return this.linksRepo.findWithUser(id); } async updateLink(tripId: string | number, linkId: string | number, data: { title?: string; url?: string; pinned?: boolean | number }) { - const existing = this.db.get('SELECT * FROM collab_links WHERE id = ? AND trip_id = ?', linkId, tripId); + const idNum = toRowId(linkId); + const tripIdNum = toRowId(tripId); + if (idNum === null || tripIdNum === null) return null; + const existing = await this.linksRepo.findInTrip(idNum, tripIdNum); if (!existing) return null; - this.db.run( - 'UPDATE collab_links SET title = ?, url = ?, pinned = ?, updated_at = CURRENT_TIMESTAMP WHERE id = ? AND trip_id = ?', - data.title?.trim() || existing.title, - data.url?.trim() || existing.url, - data.pinned === undefined ? existing.pinned : (data.pinned ? 1 : 0), - linkId, - tripId, - ); - return this.db.get('SELECT l.*, u.username FROM collab_links l JOIN users u ON u.id = l.user_id WHERE l.id = ?', linkId); + await this.linksRepo.update(existing.id, existing.trip_id, { + title: data.title?.trim() || existing.title, + url: data.url?.trim() || existing.url, + // `existing.pinned` is `number | null` on the column, but every write + // this repository makes sets it to 0/1 (never null) — the `?? 0` + // fallback only guards a value that should never actually be null. + pinned: data.pinned === undefined ? (existing.pinned ?? 0) : (data.pinned ? 1 : 0), + }); + return this.linksRepo.findWithUser(existing.id); } async deleteLink(tripId: string | number, linkId: string | number): Promise { - return this.db.run('DELETE FROM collab_links WHERE id = ? AND trip_id = ?', linkId, tripId).changes > 0; + const idNum = toRowId(linkId); + const tripIdNum = toRowId(tripId); + if (idNum === null || tripIdNum === null) return false; + return this.linksRepo.deleteScoped(idNum, tripIdNum); } /* ------------------------------------------------------------------ */ /* Messages */ /* ------------------------------------------------------------------ */ - private async formatMessage(msg: CollabMessage, reactions?: GroupedReaction[]) { - const attachments = msg.id && msg.trip_id - ? this.db.all( - `SELECT id, trip_id, message_id, filename, original_name, file_size, mime_type - FROM trip_files WHERE message_id = ? AND trip_id = ? AND deleted_at IS NULL - ORDER BY id ASC`, - msg.id, msg.trip_id, - ) - : []; + private async formatMessage(msg: CollabMessageJoinRow, reactions?: GroupedReaction[]) { + const attachments = msg.id && msg.trip_id ? await this.messagesRepo.listAttachmentsForMessage(msg.id, msg.trip_id) : []; return { ...msg, user_avatar: avatarUrl(msg), avatar_url: avatarUrl(msg), reactions: reactions || [], - attachments: attachments.map((a: any) => ({ + attachments: attachments.map(a => ({ id: a.id, filename: a.filename, original_name: a.original_name, @@ -500,27 +520,23 @@ export class CollabService { } async countMessages(tripId: string | number): Promise { - const row = this.db.get<{ cnt: number }>('SELECT COUNT(*) as cnt FROM collab_messages WHERE trip_id = ?', tripId)!; - return row.cnt; + return this.messagesRepo.countForTrip(toRowId(tripId) ?? -1); } async listMessages(tripId: string | number, before?: string | number) { - const query = ` - SELECT m.*, u.username, u.avatar, - CASE WHEN rm.deleted = 1 THEN '' ELSE rm.text END AS reply_text, - ru.username AS reply_username - FROM collab_messages m - JOIN users u ON m.user_id = u.id - LEFT JOIN collab_messages rm ON m.reply_to = rm.id - LEFT JOIN users ru ON rm.user_id = ru.id - WHERE m.trip_id = ?${before ? ' AND m.id < ?' : ''} - ORDER BY m.id DESC - LIMIT 100 - `; - - const messages = before - ? this.db.all(query, tripId, before) - : this.db.all(query, tripId); + const tripIdNum = toRowId(tripId) ?? -1; + // A malformed (non-canonical) `before` is narrowed to `-1` (rule 15) — + // `m.id < -1` matches nothing, an empty page. The legacy raw bind let + // SQLite's TEXT/INTEGER type-ordering decide instead (a non-numeric + // cursor sorts as "greater" than every integer id, so `m.id < 'garbage'` + // was always true and returned every message unfiltered) — a + // non-canonical `before` can only reach this path from something other + // than our own client (every real cursor is `String()`), so this is the SAME accepted narrowing `toRowId`'s own + // docstring describes elsewhere, not a behaviour this domain's own + // traffic can trigger. Flagged in the task-5 report for a ruling. + const beforeNum = before === undefined ? undefined : (toRowId(before) ?? -1); + const messages = await this.messagesRepo.listForTrip(tripIdNum, beforeNum); messages.reverse(); @@ -531,18 +547,11 @@ export class CollabService { for (const m of messages) if (m.deleted) m.text = ''; const msgIds = messages.map(m => m.id); + const allReactions = await this.messageReactionsRepo.listForMessages(msgIds); const reactionsByMsg: Record = {}; - if (msgIds.length > 0) { - const allReactions = this.db.all(` - SELECT r.message_id, r.emoji, r.user_id, u.username - FROM collab_message_reactions r - JOIN users u ON r.user_id = u.id - WHERE r.message_id IN (${msgIds.map(() => '?').join(',')}) - `, ...msgIds); - for (const r of allReactions) { - if (!reactionsByMsg[r.message_id]) reactionsByMsg[r.message_id] = []; - reactionsByMsg[r.message_id].push(r); - } + for (const r of allReactions) { + if (!reactionsByMsg[r.message_id]) reactionsByMsg[r.message_id] = []; + reactionsByMsg[r.message_id].push(r); } return Promise.all(messages.map(m => this.formatMessage(m, this.groupReactions(reactionsByMsg[m.id] || [])))); @@ -558,53 +567,48 @@ export class CollabService { if (replyTo) { // A soft-deleted message is gone as far as anyone replying is concerned: // its row survives only so the placeholder can be drawn where it was. - const replyMsg = this.db.get( - 'SELECT id FROM collab_messages WHERE id = ? AND trip_id = ? AND deleted = 0', replyTo, tripId, - ); + const tripIdNum = toRowId(tripId) ?? -1; + const replyMsg = await this.messagesRepo.findActiveInTrip(replyTo, tripIdNum); if (!replyMsg) return { error: 'reply_not_found' }; } // One transaction: the caller has already committed the image bytes to // storage, so a message row that lands without its attachment rows would // leave those bytes with nothing pointing at them and nothing to sweep them. - const result = await this.uow.transactional(async () => { - const inserted = this.db.run(` - INSERT INTO collab_messages (trip_id, user_id, text, reply_to) VALUES (?, ?, ?, ?) - `, tripId, userId, text.trim(), replyTo || null); + const insertedId = await this.uow.transactional(async () => { + const id = await this.messagesRepo.insertMessage(tripId, userId, text.trim(), replyTo || null); for (const file of files) { - this.db.run( - `INSERT INTO trip_files (trip_id, message_id, filename, original_name, file_size, mime_type, uploaded_by) - VALUES (?, ?, ?, ?, ?, ?, ?)`, - tripId, inserted.lastInsertRowid, file.filename, file.originalname, file.size, file.mimetype, userId, - ); + await this.messagesRepo.insertAttachmentForMessage({ + trip_id: tripId, + message_id: id, + filename: file.filename, + original_name: file.originalname, + file_size: file.size, + mime_type: file.mimetype, + uploaded_by: userId, + }); } - return inserted; + return id; }); - const message = this.db.get(` - SELECT m.*, u.username, u.avatar, - CASE WHEN rm.deleted = 1 THEN '' ELSE rm.text END AS reply_text, - ru.username AS reply_username - FROM collab_messages m - JOIN users u ON m.user_id = u.id - LEFT JOIN collab_messages rm ON m.reply_to = rm.id - LEFT JOIN users ru ON rm.user_id = ru.id - WHERE m.id = ? - `, result.lastInsertRowid)!; + const message = (await this.messagesRepo.findWithReply(insertedId))!; return { message: await this.formatMessage(message) }; } async deleteMessage(tripId: string | number, messageId: string | number, userId: number): Promise<{ error?: string; username?: string }> { - const message = this.db.get('SELECT * FROM collab_messages WHERE id = ? AND trip_id = ?', messageId, tripId); + const idNum = toRowId(messageId); + const tripIdNum = toRowId(tripId); + if (idNum === null || tripIdNum === null) return { error: 'not_found' }; + const message = await this.messagesRepo.findInTrip(idNum, tripIdNum); if (!message) return { error: 'not_found' }; if (Number(message.user_id) !== Number(userId)) return { error: 'not_owner' }; - const attachments = this.db.all<{ filename: string }>('SELECT filename FROM trip_files WHERE message_id = ? AND trip_id = ?', messageId, tripId); + const attachments = await this.messagesRepo.listFilenamesForMessage(message.id, tripIdNum); await this.uow.transactional(async () => { - this.db.run('DELETE FROM trip_files WHERE message_id = ? AND trip_id = ?', messageId, tripId); - this.db.run('UPDATE collab_messages SET deleted = 1 WHERE id = ?', messageId); + await this.messagesRepo.deleteAttachmentsForMessage(message.id, tripIdNum); + await this.messagesRepo.softDelete(message.id); }); // Only once the rows are actually gone. Dropping the blobs first left a // live message pointing at attachments whose bytes no longer existed @@ -612,7 +616,10 @@ export class CollabService { for (const file of attachments) { void this.storage.delete('files', path.basename(file.filename)).catch(() => { /* best effort */ }); } - return { username: message.username }; + // `message.username` — see the class docstring's "pre-existing `username` + // gap" note: `findInTrip` never joins `users`, so this has always been + // `undefined` at runtime. Preserved exactly, not fixed. + return { username: undefined }; } /* ------------------------------------------------------------------ */ @@ -735,8 +742,8 @@ export class CollabService { // nothing the module graph does not already give — NotificationsModule // reaches nothing in this direction — and it hid the edge while handing the // send a second NotificationsService built outside the container. - const tripInfo = this.db.get<{ title: string }>('SELECT title FROM trips WHERE id = ?', tripId); - const params: Record = { trip: tripInfo?.title || 'Untitled', actor: actor.email, tripId: String(tripId) }; + const title = await this.tripsRepo.getTitle(tripId); + const params: Record = { trip: title || 'Untitled', actor: actor.email, tripId: String(tripId) }; if (preview !== undefined) params.preview = preview; this.notifications.send({ event: 'collab_message', actorId: actor.id, scope: 'trip', targetId: Number(tripId), params }).catch(() => {}); } diff --git a/server/tests/helpers/collab-repos.ts b/server/tests/helpers/collab-repos.ts new file mode 100644 index 000000000..b78d1d3bc --- /dev/null +++ b/server/tests/helpers/collab-repos.ts @@ -0,0 +1,55 @@ +import type Database from 'better-sqlite3'; +import { sharedTestOrm } from './test-uow'; +import { CollabNotes } from '../../src/db/entities/CollabNotes.entity'; +import type { CollabNotesRepository } from '../../src/db/repositories/CollabNotes.repository'; +import { CollabMessageReactions } from '../../src/db/entities/CollabMessageReactions.entity'; +import type { CollabMessageReactionsRepository } from '../../src/db/repositories/CollabMessageReactions.repository'; +import { CollabPolls } from '../../src/db/entities/CollabPolls.entity'; +import type { CollabPollsRepository } from '../../src/db/repositories/CollabPolls.repository'; +import { CollabPollVotes } from '../../src/db/entities/CollabPollVotes.entity'; +import type { CollabPollVotesRepository } from '../../src/db/repositories/CollabPollVotes.repository'; +import { CollabLinks } from '../../src/db/entities/CollabLinks.entity'; +import type { CollabLinksRepository } from '../../src/db/repositories/CollabLinks.repository'; +import { CollabMessages } from '../../src/db/entities/CollabMessages.entity'; +import type { CollabMessagesRepository } from '../../src/db/repositories/CollabMessages.repository'; + +/** + * Plan 3e Task 5 (`CollabService`) test-only repository factories, bound to a + * suite's own better-sqlite3 handle via the SAME memoised `sharedTestOrm` + * `test-uow.ts` exports — required so a repository built here and a + * `UnitOfWork` built from `createTestUnitOfWork` (that file) resolve the + * IDENTICAL context-resolving `EntityManager` (the `files-repos.ts` precedent + * this file otherwise copies verbatim: a repository from a SECOND, independent + * ORM instance would write outside the open transaction). + * + * A NEW file rather than an addition to `test-uow.ts`, same reasoning + * `files-repos.ts`'s own docstring gives for itself: at the time this task + * ran, `test-uow.ts` had just been Task 6's named file in the prior window, + * and appending here avoids any risk of colliding with a concurrently + * in-flight task's edits to that file. `TripsRepository` already has its own + * `createTestTripsRepo` in `test-uow.ts` (Plan 3c Task 0b) and is reused + * directly from there — only the six collab-owned repositories are new here. + */ +export function createTestCollabNotesRepo(db: Database.Database): Promise { + return sharedTestOrm(db).then((t) => t.repo(CollabNotes)); +} + +export function createTestCollabMessageReactionsRepo(db: Database.Database): Promise { + return sharedTestOrm(db).then((t) => t.repo(CollabMessageReactions)); +} + +export function createTestCollabPollsRepo(db: Database.Database): Promise { + return sharedTestOrm(db).then((t) => t.repo(CollabPolls)); +} + +export function createTestCollabPollVotesRepo(db: Database.Database): Promise { + return sharedTestOrm(db).then((t) => t.repo(CollabPollVotes)); +} + +export function createTestCollabLinksRepo(db: Database.Database): Promise { + return sharedTestOrm(db).then((t) => t.repo(CollabLinks)); +} + +export function createTestCollabMessagesRepo(db: Database.Database): Promise { + return sharedTestOrm(db).then((t) => t.repo(CollabMessages)); +} diff --git a/server/tests/unit/nest/collab.service.test.ts b/server/tests/unit/nest/collab.service.test.ts index 768a13dd4..f0b4c261d 100644 --- a/server/tests/unit/nest/collab.service.test.ts +++ b/server/tests/unit/nest/collab.service.test.ts @@ -87,17 +87,66 @@ import { RealtimeService } from '../../../src/nest/realtime/realtime.service'; import { notificationsStub } from '../../helpers/notifications'; import { makeStorageFixture } from '../../helpers/storage-fixture'; import { RateLimitService } from '../../../src/nest/common/rate-limit.service'; -import { createTestUnitOfWork, createTestAppSettingsRepo } from '../../helpers/test-uow'; +import { createTestUnitOfWork, createTestAppSettingsRepo, createTestTripsRepo } from '../../helpers/test-uow'; +import { + createTestCollabNotesRepo, + createTestCollabMessageReactionsRepo, + createTestCollabPollsRepo, + createTestCollabPollVotesRepo, + createTestCollabLinksRepo, + createTestCollabMessagesRepo, +} from '../../helpers/collab-repos'; +import type { CollabNotesRepository } from '../../../src/db/repositories/CollabNotes.repository'; +import type { CollabMessageReactionsRepository } from '../../../src/db/repositories/CollabMessageReactions.repository'; +import type { CollabPollsRepository } from '../../../src/db/repositories/CollabPolls.repository'; +import type { CollabPollVotesRepository } from '../../../src/db/repositories/CollabPollVotes.repository'; +import type { CollabLinksRepository } from '../../../src/db/repositories/CollabLinks.repository'; +import type { CollabMessagesRepository } from '../../../src/db/repositories/CollabMessages.repository'; const collabFx = makeStorageFixture('files/'); const rateLimit = new RateLimitService(); let svc: CollabService; +// Named bindings (not only inside `svc`), the `files.service.test.ts` +// `tripFilesRepo`/`fileLinksRepo` shape: the repository-level parity tests +// below call these directly, and a rollback test can spy on them directly. +let notesRepo: CollabNotesRepository; +let messageReactionsRepo: CollabMessageReactionsRepository; +let pollsRepo: CollabPollsRepository; +let pollVotesRepo: CollabPollVotesRepository; +let linksRepo: CollabLinksRepository; +let messagesRepo: CollabMessagesRepository; + +async function buildCollabService(dbs: DatabaseService, storage = collabFx.storage, rl = rateLimit): Promise { + return new CollabService( + dbs, + new PermissionsService(await createTestAppSettingsRepo(testDb), await createTestUnitOfWork(testDb)), + new RealtimeService(), + notificationsStub(), + storage, + rl, + await createTestUnitOfWork(testDb), + await createTestCollabMessageReactionsRepo(testDb), + await createTestCollabNotesRepo(testDb), + await createTestCollabPollsRepo(testDb), + await createTestCollabPollVotesRepo(testDb), + await createTestCollabLinksRepo(testDb), + await createTestCollabMessagesRepo(testDb), + await createTestTripsRepo(testDb), + ); +} + beforeAll(async () => { - svc = new CollabService(new DatabaseService(testDb), new PermissionsService(await createTestAppSettingsRepo(testDb), await createTestUnitOfWork(testDb)), new RealtimeService(), notificationsStub(), collabFx.storage, rateLimit, await createTestUnitOfWork(testDb)); + svc = await buildCollabService(new DatabaseService(testDb)); + notesRepo = await createTestCollabNotesRepo(testDb); + messageReactionsRepo = await createTestCollabMessageReactionsRepo(testDb); + pollsRepo = await createTestCollabPollsRepo(testDb); + pollVotesRepo = await createTestCollabPollVotesRepo(testDb); + linksRepo = await createTestCollabLinksRepo(testDb); + messagesRepo = await createTestCollabMessagesRepo(testDb); }); /** A CollabService with its own preview cache and budget, for the tests that fill either. */ -const freshSvc = async () => new CollabService(new DatabaseService(testDb), new PermissionsService(await createTestAppSettingsRepo(testDb), await createTestUnitOfWork(testDb)), new RealtimeService(), notificationsStub(), collabFx.storage, new RateLimitService(), await createTestUnitOfWork(testDb)); +const freshSvc = async () => buildCollabService(new DatabaseService(testDb), collabFx.storage, new RateLimitService()); beforeAll(() => { createTables(testDb); @@ -688,15 +737,15 @@ describe('hardening', () => { it('COLLAB-SVC-034: votePoll switch is atomic — prior vote survives a failed INSERT', async () => { const { user1, trip } = setup(); const dbs = new DatabaseService(testDb); - const failing = new CollabService(dbs, new PermissionsService(await createTestAppSettingsRepo(dbs.connection), await createTestUnitOfWork(dbs.connection)), new RealtimeService(), notificationsStub(), collabFx.storage, new RateLimitService(), await createTestUnitOfWork(dbs.connection)); + const failing = await buildCollabService(dbs); const poll = await failing.createPoll(trip.id, user1.id, { question: 'Q?', options: ['A', 'B'] }); await failing.votePoll(trip.id, poll!.id, user1.id, 0); - const realRun = dbs.run.bind(dbs); - const spy = vi.spyOn(dbs, 'run').mockImplementation((sql: string, ...params: unknown[]) => { - if (sql.includes('INSERT INTO collab_poll_votes')) throw new Error('boom'); - return realRun(sql, ...params); - }); + // Repository-level spy (not `dbs.run`): the write this proof targets now + // goes through `pollVotesRepo.insertVote`, not a raw `db.run` call — the + // `files.service.test.ts` R2-rollback-test shape (spy on the injected + // repository directly). + const spy = vi.spyOn(pollVotesRepo, 'insertVote').mockImplementation(() => { throw new Error('boom'); }); // Single-choice switch: DELETE prior votes, then the INSERT fails — the // transaction must roll the DELETE back too. await expect(failing.votePoll(trip.id, poll!.id, user1.id, 1)).rejects.toThrow('boom'); @@ -709,16 +758,15 @@ describe('hardening', () => { it('COLLAB-SVC-035: deleteNote is atomic — trip_files rows survive a failed note DELETE', async () => { const { user1, trip } = setup(); const dbs = new DatabaseService(testDb); - const failing = new CollabService(dbs, new PermissionsService(await createTestAppSettingsRepo(dbs.connection), await createTestUnitOfWork(dbs.connection)), new RealtimeService(), notificationsStub(), collabFx.storage, new RateLimitService(), await createTestUnitOfWork(dbs.connection)); + const failing = await buildCollabService(dbs); const note = await failing.createNote(trip.id, user1.id, { title: 'With file' }); testDb.prepare('INSERT INTO trip_files (trip_id, note_id, filename, original_name) VALUES (?, ?, ?, ?)') .run(trip.id, note.id, 'files/a.pdf', 'a.pdf'); - const realRun = dbs.run.bind(dbs); - const spy = vi.spyOn(dbs, 'run').mockImplementation((sql: string, ...params: unknown[]) => { - if (sql.includes('DELETE FROM collab_notes')) throw new Error('boom'); - return realRun(sql, ...params); - }); + // Repository-level spy (not `dbs.run`): the row delete this proof targets + // now goes through `notesRepo.delete`, the last statement inside + // `deleteNote`'s transaction. + const spy = vi.spyOn(notesRepo, 'delete').mockImplementation(() => { throw new Error('boom'); }); await expect(failing.deleteNote(trip.id, note.id)).rejects.toThrow('boom'); spy.mockRestore(); @@ -730,7 +778,7 @@ describe('hardening', () => { const { user1, trip } = setup(); const dbs = new DatabaseService(testDb); const failingStorage = { delete: vi.fn().mockRejectedValue(new Error('EACCES')) }; - const failing = new CollabService(dbs, new PermissionsService(await createTestAppSettingsRepo(dbs.connection), await createTestUnitOfWork(dbs.connection)), new RealtimeService(), notificationsStub(), failingStorage as unknown as import('../../../src/nest/storage/storage.service').StorageService, new RateLimitService(), await createTestUnitOfWork(dbs.connection)); + const failing = await buildCollabService(dbs, failingStorage as unknown as import('../../../src/nest/storage/storage.service').StorageService); const note = await failing.createNote(trip.id, user1.id, { title: 'Sticky file' }); testDb.prepare('INSERT INTO trip_files (trip_id, note_id, filename, original_name) VALUES (?, ?, ?, ?)') .run(trip.id, note.id, 'stuck.pdf', 'stuck.pdf'); @@ -770,3 +818,149 @@ describe('hardening', () => { expect(mockCheckSsrf).not.toHaveBeenCalled(); }); }); + +// ── Repository parity (Plan 3e Task 5) ─────────────────────────────────────── +// One full-key `toEqual()` +// proof per converted read model, on a fully seeded row (task-5-brief.md's +// "Tests" section). + +describe('repository parity', () => { + it('COLLAB-REPO-001: CollabNotesRepository matches the legacy joined SELECTs for a note with attachments', async () => { + const { user1, trip } = setup(); + const note = await svc.createNote(trip.id, user1.id, { title: 'Full note', content: 'Body', category: 'Ideas', color: '#123456', website: 'https://x.example', pinned: true }); + await svc.addNoteFile(trip.id, note.id, { filename: 'a.pdf', originalname: 'a.pdf', size: 10, mimetype: 'application/pdf' }); + await svc.addNoteFile(trip.id, note.id, { filename: 'b.png', originalname: 'b.png', size: 20, mimetype: 'image/png' }); + + // CB9/CB12/CB20's shared shape. + const legacyNote = testDb.prepare('SELECT n.*, u.username, u.avatar FROM collab_notes n JOIN users u ON n.user_id = u.id WHERE n.id = ?').get(note.id); + expect(await notesRepo.findWithUser(note.id)).toEqual(legacyNote); + expect(await notesRepo.findWithUserInTrip(note.id, trip.id)).toEqual(legacyNote); + + // CB6's narrow attachment projection. + const legacyAttachments = testDb.prepare('SELECT id, filename, original_name, file_size, mime_type FROM trip_files WHERE note_id = ? ORDER BY id ASC').all(note.id); + expect(await notesRepo.listAttachmentsForNote(note.id)).toEqual(legacyAttachments); + }); + + it('COLLAB-REPO-002: CollabPollsRepository/CollabPollVotesRepository match the legacy queries for a multiple-choice poll voted on every option', async () => { + const { user1, user2, trip } = setup(); + const poll = await svc.createPoll(trip.id, user1.id, { question: 'Pick', options: ['A', 'B', 'C'], multiple: true }); + await svc.votePoll(trip.id, poll!.id, user1.id, 0); + await svc.votePoll(trip.id, poll!.id, user1.id, 1); + await svc.votePoll(trip.id, poll!.id, user2.id, 2); + await svc.votePoll(trip.id, poll!.id, user2.id, 0); + + // CB23's poll half. + const legacyPoll = testDb.prepare('SELECT p.*, u.username, u.avatar FROM collab_polls p JOIN users u ON p.user_id = u.id WHERE p.id = ?').get(poll!.id); + expect(await pollsRepo.findWithUser(poll!.id)).toEqual(legacyPoll); + + // CB24's votes half — every option has at least one vote, including the + // multi-select case (user1 voted twice). No ORDER BY on either side, so + // both are sorted the same deterministic way before comparing. + const sortVotes = (rows: { option_index: number; user_id: number }[]) => [...rows].sort((a, b) => a.option_index - b.option_index || a.user_id - b.user_id); + const legacyVotes = testDb.prepare('SELECT v.option_index, v.user_id, u.username, u.avatar FROM collab_poll_votes v JOIN users u ON v.user_id = u.id WHERE v.poll_id = ?').all(poll!.id) as { option_index: number; user_id: number }[]; + const repoVotes = await pollVotesRepo.listForPoll(poll!.id); + expect(sortVotes(repoVotes)).toEqual(sortVotes(legacyVotes)); + expect(repoVotes).toHaveLength(4); + }); + + it('COLLAB-REPO-003: CollabMessagesRepository.listForTrip matches the legacy self-joined SELECT for a reply to a soft-deleted message', async () => { + const { user1, trip } = setup(); + const original = await svc.createMessage(trip.id, user1.id, 'Original text'); + const reply = await svc.createMessage(trip.id, user1.id, 'Reply text', original.message!.id); + await svc.deleteMessage(trip.id, original.message!.id, user1.id); + + const legacy = testDb.prepare(` + SELECT m.*, u.username, u.avatar, + CASE WHEN rm.deleted = 1 THEN '' ELSE rm.text END AS reply_text, + ru.username AS reply_username + FROM collab_messages m + JOIN users u ON m.user_id = u.id + LEFT JOIN collab_messages rm ON m.reply_to = rm.id + LEFT JOIN users ru ON rm.user_id = ru.id + WHERE m.trip_id = ? + ORDER BY m.id DESC + LIMIT 100 + `).all(trip.id); + + const repoRows = await messagesRepo.listForTrip(trip.id); + expect(repoRows).toEqual(legacy); + + const replyRow = repoRows.find(r => r.id === reply.message!.id)!; + expect(replyRow.reply_text).toBe(''); // the quoted message is soft-deleted — blanked, not the raw text. + expect(replyRow.reply_username).toBe(user1.username); + }); + + it('COLLAB-REPO-004: CollabMessageReactionsRepository matches the legacy queries for reactions from several users on several emoji', async () => { + const { user1, user2, trip } = setup(); + const msg = await svc.createMessage(trip.id, user1.id, 'React away'); + const msgId = msg.message!.id; + testDb.prepare('INSERT INTO collab_message_reactions (message_id, user_id, emoji) VALUES (?, ?, ?)').run(msgId, user1.id, '👍'); + testDb.prepare('INSERT INTO collab_message_reactions (message_id, user_id, emoji) VALUES (?, ?, ?)').run(msgId, user2.id, '👍'); + testDb.prepare('INSERT INTO collab_message_reactions (message_id, user_id, emoji) VALUES (?, ?, ?)').run(msgId, user2.id, '🎉'); + + const sortReactions = (rows: { emoji: string; user_id: number }[]) => [...rows].sort((a, b) => a.emoji.localeCompare(b.emoji) || a.user_id - b.user_id); + + // CB1. + const legacySingle = testDb.prepare('SELECT r.emoji, r.user_id, u.username FROM collab_message_reactions r JOIN users u ON r.user_id = u.id WHERE r.message_id = ?').all(msgId) as { emoji: string; user_id: number }[]; + const repoSingle = await messageReactionsRepo.listForMessage(msgId); + expect(sortReactions(repoSingle)).toEqual(sortReactions(legacySingle)); + + // CB46 (the batch form `listMessages` uses). + const legacyBatch = testDb.prepare('SELECT r.message_id, r.emoji, r.user_id, u.username FROM collab_message_reactions r JOIN users u ON r.user_id = u.id WHERE r.message_id IN (?)').all(msgId) as { emoji: string; user_id: number }[]; + const repoBatch = await messageReactionsRepo.listForMessages([msgId]); + expect(sortReactions(repoBatch)).toEqual(sortReactions(legacyBatch)); + expect(repoBatch).toHaveLength(3); + }); + + it('COLLAB-REPO-005: CollabLinksRepository matches the legacy joined SELECTs for a pinned link', async () => { + const { user1, trip } = setup(); + const link = await svc.createLink(trip.id, user1.id, { title: 'Guide', url: 'https://example.com/guide', pinned: true }); + + const legacy = testDb.prepare('SELECT l.*, u.username FROM collab_links l JOIN users u ON u.id = l.user_id WHERE l.id = ?').get(link!.id); + expect(await linksRepo.findWithUser(link!.id)).toEqual(legacy); + + const legacyList = testDb.prepare('SELECT l.*, u.username FROM collab_links l JOIN users u ON u.id = l.user_id WHERE l.trip_id = ? ORDER BY l.pinned DESC, l.created_at DESC').all(trip.id); + expect(await linksRepo.listForTrip(trip.id)).toEqual(legacyList); + }); +}); + +// ── deleteNoteFile IDOR guard (CB21, security-critical) ────────────────────── +// A mutation proof: each assertion isolates ONE of the three scoping columns +// (`id`, `note_id`, `trip_id`) by matching the foreign file on the other two — +// dropping any single column from the guard's query flips that assertion from +// a refusal to a wrongful success. + +describe('deleteNoteFile IDOR guard', () => { + it('COLLAB-SEC-001: refuses a file id that resolves under a different note or a different trip', async () => { + const { user1, trip } = setup(); + const otherTrip = createTrip(testDb, user1.id); + + // Two notes in the trip under test, so the `note_id` probe below keeps + // `trip_id` constant and only `note_id` differs. + const note1 = await svc.createNote(trip.id, user1.id, { title: 'Note 1' }); + const note2 = await svc.createNote(trip.id, user1.id, { title: 'Note 2' }); + const file2 = await svc.addNoteFile(trip.id, note2.id, { filename: 'note2.pdf', originalname: 'note2.pdf', size: 1, mimetype: 'application/pdf' }); + + // `id` and `trip_id` both match; only `note_id` is wrong (asking for + // note1's scope with a file that actually belongs to note2) — red if the + // guard's `note_id` column were dropped. + expect(await svc.deleteNoteFile(trip.id, note1.id, file2!.file.id)).toBe(false); + expect(testDb.prepare('SELECT COUNT(*) as c FROM trip_files WHERE id = ?').get(file2!.file.id)).toEqual({ c: 1 }); + + // A note (and its file) that live in a DIFFERENT trip. `id` and `note_id` + // both match the real row; only `trip_id` is wrong (the attacker's own + // authorized trip, not the one the note/file actually belongs to) — red + // if the guard's `trip_id` column were dropped. This is the exact IDOR + // the legacy doc comment names: a caller access-checked for `trip` alone + // enumerating a foreign note/file id pair. + const foreignNote = await svc.createNote(otherTrip.id, user1.id, { title: 'Foreign note' }); + const foreignFile = await svc.addNoteFile(otherTrip.id, foreignNote.id, { filename: 'foreign.pdf', originalname: 'foreign.pdf', size: 1, mimetype: 'application/pdf' }); + + expect(await svc.deleteNoteFile(trip.id, foreignNote.id, foreignFile!.file.id)).toBe(false); + expect(testDb.prepare('SELECT COUNT(*) as c FROM trip_files WHERE id = ?').get(foreignFile!.file.id)).toEqual({ c: 1 }); + + // Sanity: the SAME file, scoped correctly, does succeed — the guard + // isn't just refusing everything. + expect(await svc.deleteNoteFile(otherTrip.id, foreignNote.id, foreignFile!.file.id)).toBe(true); + }); +});