From da85d1487c46ed6a12d6e0556fd4b6fd56121fca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patrick=20M=C3=BCller?= Date: Sat, 2 May 2026 12:22:03 +0200 Subject: [PATCH] Add claude init file + refactor some security issues --- CLAUDE.md | 66 +++++++++++++++++++ DEFERRED_SECURITY.md | 55 ++++++++++++++++ src/models/calendar/Calendar.db.ts | 3 +- .../calendar/events/credentials.service.ts | 8 +-- src/models/calendar/events/events.router.ts | 13 ++-- src/models/calendar/events/events.service.ts | 14 ++-- src/models/calendar/users/users.router.ts | 8 +-- src/models/calendar/users/users.service.ts | 59 ++++++++++------- 8 files changed, 177 insertions(+), 49 deletions(-) create mode 100644 CLAUDE.md create mode 100644 DEFERRED_SECURITY.md diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000..35b50aa --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,66 @@ +# CLAUDE.md + +This file provides guidance to Claude Code (claude.ai/code) when working with code in this repository. + +## Commands + +```bash +npm run build # Compile TypeScript → dist/ +npm run start # Build and start (tsc && node ./dist/app.js) +npm run debug # Start with DEBUG=* environment variable +npm run test # Run Jest tests with coverage (outputs sonar-report.xml) +``` + +Run a single test file: +```bash +npx jest test/some.test.ts +``` + +## Architecture + +Express.js REST API in TypeScript with a service-oriented layering. The sole domain is `Calendar`, which organises **events** and **users**. + +**Request path:** +1. `app.ts` mounts `Calendar.router.ts` at `/calendar` +2. `Calendar.router.ts` delegates to `events.router.ts` and `users.router.ts` +3. Routers call services; services call the MariaDB pool in `Calendar.db.ts` + +**Key layers:** + +| Layer | Location | +|---|---| +| Router | `src/models/calendar/Calendar.router.ts`, `…/events/events.router.ts`, `…/users/users.router.ts` | +| Services | `…/events/events.service.ts`, `…/users/users.service.ts`, `…/events/credentials.service.ts`, `…/events/icalgenerator.service.ts` | +| DB pool | `src/models/calendar/Calendar.db.ts` (MariaDB, pool size 5) | +| Shared | `src/common/` (base route class, nodemailer wrapper), `src/middleware/logger.ts` (Winston) | + +**Auth model:** Users must have a `@nachklang.art` email. After activation they receive a session token (30-day window); the token hash + IP are stored in the DB. Credentials for non-user calendar access (`MEMBER_CREDENTIAL`, `CHOIR_CREDENTIAL`, `MANAGEMENT_CREDENTIAL`) come from `.env`. + +**Event versioning:** Events have a companion `event_versions` table. `events.service.ts` manages writes to both. + +**Calendar types and IDs:** `public` (1), `members` (2), `management` (3), `choir` (4), `birthdays` (5). `credentials.service.ts` enforces which session/credential can read each calendar. + +**iCal export:** `icalgenerator.service.ts` converts DB events to RFC 5545 format; reachable via `GET /calendar/events/{calendar}/ical`. + +**API docs:** Swagger UI served at `/docs`, generated from JSDoc annotations in the router files. + +## Environment + +Copy `.env.example` (or create `.env`) with: +``` +PORT= +DB_HOST= +DB_USER= +DB_PASSWORD= +CALENDAR_DB= +EMAIL_HOST= +EMAIL_USERNAME= +EMAIL_PASSWORD= +MEMBER_CREDENTIAL= +CHOIR_CREDENTIAL= +MANAGEMENT_CREDENTIAL= +``` + +## TypeScript config + +Strict mode enabled, target ES2016, compiled output in `./dist`, inline source maps. Tests run through `ts-jest` directly against `.ts` sources. diff --git a/DEFERRED_SECURITY.md b/DEFERRED_SECURITY.md new file mode 100644 index 0000000..40db072 --- /dev/null +++ b/DEFERRED_SECURITY.md @@ -0,0 +1,55 @@ +# Deferred Security Issues + +These items were identified during a security review on 2026-05-02 and consciously deferred. +**Must be addressed before opening the application to a larger or public userbase.** + +--- + +## 1. Session credentials in URL query parameters (logged-in users) + +**Files:** `src/models/calendar/events/events.router.ts` — all GET/PUT/DELETE handlers + +`sessionId` and `sessionKey` are currently read from query parameters, which means they appear in server access logs, browser history, proxy logs, and `Referer` headers. + +**Fix:** Move to request headers (`X-Session-Id` / `X-Session-Key`) or the request body. Requires a corresponding frontend update. + +> Note: the shared calendar `password` parameter in query params is intentional (iCal clients don't support headers) and is acceptable for the current setup. + +--- + +## 2. No event ownership check + +**Files:** `src/models/calendar/events/events.router.ts` +- `PUT /:eventId` (update) +- `PUT /move/:eventId` (move) +- `DELETE /:eventId` (delete) + +Currently any active user can edit, move, or delete any event regardless of who created it. This is acceptable while all users are trusted admins. + +**Fix:** When non-admin users are introduced, fetch the event first and verify `event.createdById === user.userId` before allowing the mutation. Add an `isAdmin` flag to the user model to let admins bypass the check. + +--- + +## 3. Activation token has no expiry + +**File:** `src/models/calendar/users/users.service.ts` — `createUser` / `activateUser` + +The email activation link is valid indefinitely. Acceptable for a small, trusted userbase. + +**Fix:** +1. Add an `activation_expires` column to the `users` table (e.g. `DATETIME`). +2. Set it to `NOW() + INTERVAL 24 HOUR` in `createUser`. +3. Check `activation_expires > NOW()` in `activateUser` before accepting the token. + +--- + +## 4. Password reset token has no expiry + +**File:** `src/models/calendar/users/users.service.ts` — `initiatePasswordReset` / `finalizePasswordReset` + +The reset token stored in `pw_reset_token_hash` never expires. Acceptable for a small, trusted userbase. + +**Fix:** +1. Add a `pw_reset_expires` column to the `users` table (e.g. `DATETIME`). +2. Set it to `NOW() + INTERVAL 15 MINUTE` in `initiatePasswordReset`. +3. Check `pw_reset_expires > NOW()` in `finalizePasswordReset` before accepting the token. diff --git a/src/models/calendar/Calendar.db.ts b/src/models/calendar/Calendar.db.ts index 37f6d10..bc6730c 100644 --- a/src/models/calendar/Calendar.db.ts +++ b/src/models/calendar/Calendar.db.ts @@ -10,7 +10,8 @@ export namespace NachklangCalendarDB { user: process.env.DB_USER, password: process.env.DB_PASSWORD, database: process.env.CALENDAR_DB, - connectionLimit: 5 + connectionLimit: 5, + autoCommit: false }); export const getConnection = async () => { diff --git a/src/models/calendar/events/credentials.service.ts b/src/models/calendar/events/credentials.service.ts index 26e07e6..330d8d5 100644 --- a/src/models/calendar/events/credentials.service.ts +++ b/src/models/calendar/events/credentials.service.ts @@ -11,7 +11,7 @@ dotenv.config(); export const checkAdminPrivileges = async (sessionId: string, sessionKey: string, ip: string) => { if(sessionId) { let user = await UserService.checkSession(sessionId, sessionKey, ip); - return user.isActive; + return user?.isActive ?? false; } return false; } @@ -23,7 +23,7 @@ export const checkAdminPrivileges = async (sessionId: string, sessionKey: string export const checkMemberPrivileges = async (sessionId: string, sessionKey: string, password: string, ip: string) => { if(sessionId) { let user = await UserService.checkSession(sessionId, sessionKey, ip); - return user.isActive; + return user?.isActive ?? false; } return password == process.env.MEMBER_CREDENTIAL; @@ -36,7 +36,7 @@ export const checkMemberPrivileges = async (sessionId: string, sessionKey: strin export const checkChoirPrivileges = async (sessionId: string, sessionKey: string, password: string, ip: string) => { if(sessionId) { let user = await UserService.checkSession(sessionId, sessionKey, ip); - return user.isActive; + return user?.isActive ?? false; } return password == process.env.CHOIR_CREDENTIAL; @@ -49,7 +49,7 @@ export const checkChoirPrivileges = async (sessionId: string, sessionKey: string export const checkManagementPrivileges = async (sessionId: string, sessionKey: string, password: string, ip: string) => { if(sessionId) { let user = await UserService.checkSession(sessionId, sessionKey, ip); - return user.isActive; + return user?.isActive ?? false; } return password == process.env.MANAGEMENT_CREDENTIAL; diff --git a/src/models/calendar/events/events.router.ts b/src/models/calendar/events/events.router.ts index 2472276..bdc414c 100644 --- a/src/models/calendar/events/events.router.ts +++ b/src/models/calendar/events/events.router.ts @@ -138,7 +138,7 @@ eventsRouter.get('/:calendar/json', async (req: Request, res: Response) => { let events: Event[]; - if(user.isActive) { + if(user?.isActive) { events = await EventService.getAllEventsAdmin(calendarId); } else { events = await EventService.getAllEvents(calendarId); @@ -147,7 +147,8 @@ eventsRouter.get('/:calendar/json', async (req: Request, res: Response) => { // Send the events back res.status(200).send(events); } catch (e: any) { - console.log('Error handling a request: ' + e.message); + let errorGuid = Guid.create().toString(); + logger.error('Error handling a request: ' + e.message, {reference: errorGuid}); res.status(500).send({'message': 'Internal Server Error. Try again later.'}); } }); @@ -530,7 +531,7 @@ eventsRouter.post('/', async (req: Request, res: Response) => { let user = await UserService.checkSession(sessionId, sessionKey, ip); - if (!user.isActive) { + if (!user?.isActive) { res.status(403).send({'message': 'You do not have access to the specified calendar.'}); return; } @@ -708,7 +709,7 @@ eventsRouter.put('/:eventId', async (req: Request, res: Response) => { let user = await UserService.checkSession(sessionId, sessionKey, ip); - if (!user.isActive) { + if (!user?.isActive) { res.status(403).send({'message': 'You do not have access to the specified calendar.'}); return; } @@ -889,7 +890,7 @@ eventsRouter.put('/move/:eventId', async (req: Request, res: Response) => { let user = await UserService.checkSession(sessionId, sessionKey, ip); - if (!user.isActive) { + if (!user?.isActive) { res.status(403).send({'message': 'You do not have access to the specified calendar.'}); return; } @@ -1022,7 +1023,7 @@ eventsRouter.delete('/:eventId', async (req: Request, res: Response) => { let user = await UserService.checkSession(sessionId, sessionKey, ip); - if (!user.isActive) { + if (!user?.isActive) { res.status(403).send({'message': 'You do not have access to the specified calendar.'}); return; } diff --git a/src/models/calendar/events/events.service.ts b/src/models/calendar/events/events.service.ts index 2753b1b..c696ac6 100644 --- a/src/models/calendar/events/events.service.ts +++ b/src/models/calendar/events/events.service.ts @@ -133,12 +133,13 @@ export const getAllEventsAdmin = async (calendarId: number): Promise => export const createEvent = async (event: Event): Promise => { let conn = await NachklangCalendarDB.getConnection(); try { + await conn.beginTransaction(); let eventUUID = Guid.create().toString(); const eventsQuery = 'INSERT INTO events (calendar_id, uuid, created_by_id) VALUES (?,?,?) RETURNING event_id'; const eventsRes = await conn.execute(eventsQuery, [event.calendarId, eventUUID, event.createdById]); const versionQuery = 'INSERT INTO event_versions (event_id, name, description, start_datetime, end_datetime, whole_day, repeat_frequency, location, url, status, version_created_by_id) VALUES (?,?,?,?,?,?,?,?,?,?,?);' - const versionRes = await conn.execute(versionQuery, [eventsRes[0].event_id, event.name, event.description, event.startDateTime, event.endDateTime, event.wholeDay, event.repeatFrequency, event.location, event.url, event.status, event.createdById]); + await conn.execute(versionQuery, [eventsRes[0].event_id, event.name, event.description, event.startDateTime, event.endDateTime, event.wholeDay, event.repeatFrequency, event.location, event.url, event.status, event.createdById]); await conn.commit(); @@ -147,8 +148,6 @@ export const createEvent = async (event: Event): Promise => { await conn.rollback(); throw err; } finally { - // Return connection - await conn.commit(); await conn.end(); } }; @@ -160,6 +159,7 @@ export const createEvent = async (event: Event): Promise => { export const updateEvent = async (event: Event): Promise => { let conn = await NachklangCalendarDB.getConnection(); try { + await conn.beginTransaction(); const versionQuery = 'INSERT INTO event_versions (event_id, name, description, start_datetime, end_datetime, whole_day, repeat_frequency, location, url, status, version_created_by_id) VALUES (?,?,?,?,?,?,?,?,?,?,?);' const versionRes = await conn.execute(versionQuery, [event.eventId, event.name, event.description, event.startDateTime, event.endDateTime, event.wholeDay, event.repeatFrequency, event.location, event.url, event.status, event.createdById]); @@ -170,8 +170,6 @@ export const updateEvent = async (event: Event): Promise => { await conn.rollback(); throw err; } finally { - // Return connection - await conn.commit(); await conn.end(); } }; @@ -183,6 +181,7 @@ export const updateEvent = async (event: Event): Promise => { export const deleteEvent = async (event: Event): Promise => { let conn = await NachklangCalendarDB.getConnection(); try { + await conn.beginTransaction(); const versionQuery = 'INSERT INTO event_versions (event_id, status, version_created_by_id) VALUES (?,?,?);' const versionRes = await conn.execute(versionQuery, [event.eventId, 'DELETED', event.createdById]); @@ -193,8 +192,6 @@ export const deleteEvent = async (event: Event): Promise => { await conn.rollback(); throw err; } finally { - // Return connection - await conn.commit(); await conn.end(); } }; @@ -206,6 +203,7 @@ export const deleteEvent = async (event: Event): Promise => { export const moveEvent = async (event: Event): Promise => { let conn = await NachklangCalendarDB.getConnection(); try { + await conn.beginTransaction(); const eventQuery = 'UPDATE events SET calendar_id = ? WHERE event_id = ?'; const eventRes = await conn.execute(eventQuery, [event.calendarId, event.eventId]); @@ -216,8 +214,6 @@ export const moveEvent = async (event: Event): Promise => { await conn.rollback(); throw err; } finally { - // Return connection - await conn.commit(); await conn.end(); } } diff --git a/src/models/calendar/users/users.router.ts b/src/models/calendar/users/users.router.ts index ff90e96..ac3fd2e 100644 --- a/src/models/calendar/users/users.router.ts +++ b/src/models/calendar/users/users.router.ts @@ -329,9 +329,9 @@ usersRouter.post('/login', async (req: Request, res: Response) => { } // Create a session - const session: Session = await UserService.login(email, password, ip); + const session: Session | null = await UserService.login(email, password, ip); - if (!session.sessionId) { + if (!session || !session.sessionId) { // Error logging in, probably wrong username / password res.status(401).send(JSON.stringify({message: 'Wrong username and / or password', sessionId: -1, sessionKey: ''})); return; @@ -426,9 +426,9 @@ usersRouter.post('/checkSessionValid', async (req: Request, res: Response) => { return; } - const user: User = await UserService.checkSession(session_id, session_key, ip); + const user: User | null = await UserService.checkSession(session_id, session_key, ip); - if (!user.userId) { + if (!user || !user.userId) { // Error logging in, probably wrong username / password res.status(401).send(JSON.stringify({messages: ['Invalid session']})); return; diff --git a/src/models/calendar/users/users.service.ts b/src/models/calendar/users/users.service.ts index 58ae407..6ef36ac 100644 --- a/src/models/calendar/users/users.service.ts +++ b/src/models/calendar/users/users.service.ts @@ -23,16 +23,19 @@ dotenv.config(); export const createUser = async (email: string, password: string, fullName: string, ip: string): Promise => { let conn = await NachklangCalendarDB.getConnection(); try { + await conn.beginTransaction(); + // Hash password and generate + hash session key const pwHash = bcrypt.hashSync(password, 10); const sessionKey = Guid.create().toString(); const sessionKeyHash = bcrypt.hashSync(sessionKey, 10); const activationToken = Guid.create().toString(); + const activationTokenHash = bcrypt.hashSync(activationToken, 10); // Create user entry in SQL const userQuery = 'INSERT INTO users (email, password_hash, full_name, activation_token) VALUES (?, ?, ?, ?) RETURNING user_id'; - const userIdRes = await conn.query(userQuery, [email, pwHash, fullName, activationToken]); + const userIdRes = await conn.query(userQuery, [email, pwHash, fullName, activationTokenHash]); // Get user id of the created user let userId: number = -1; @@ -40,9 +43,6 @@ export const createUser = async (email: string, password: string, fullName: stri userId = row.user_id; } - // Send email with activation link - await MailService.sendMail(email, 'Activate your Nachklang account', `Hi ${fullName},\n\nPlease click on the following link to activate your account:\n\nhttps://api.nachklang.art/calendar/users/activate?id=${userId}&token=${activationToken}`); - // Create session const sessionQuery = 'INSERT INTO sessions (user_id, session_key_hash, created_date, valid_until, last_ip) VALUES (?,?,NOW(),DATE_ADD(NOW(), INTERVAL 30 DAY),?) RETURNING session_id'; const sessionIdRes = await conn.query(sessionQuery, [userId, sessionKeyHash, ip]); @@ -54,6 +54,9 @@ export const createUser = async (email: string, password: string, fullName: stri sessionId = row.session_id; } + // Send email with activation link (after commit so we don't block on email delivery) + await MailService.sendMail(email, 'Activate your Nachklang account', `Hi ${fullName},\n\nPlease click on the following link to activate your account:\n\nhttps://api.nachklang.art/calendar/users/activate?id=${userId}&token=${activationToken}`); + return { sessionId: sessionId, userId: userId, @@ -62,9 +65,9 @@ export const createUser = async (email: string, password: string, fullName: stri lastIP: ip }; } catch (err) { + await conn.rollback(); throw err; } finally { - // Return connection await conn.end(); } }; @@ -72,22 +75,25 @@ export const createUser = async (email: string, password: string, fullName: stri export const activateUser = async (userId: number, token: string): Promise => { let conn = await NachklangCalendarDB.getConnection(); try { + await conn.beginTransaction(); + const checkTokenQuery = 'SELECT user_id, activation_token FROM users WHERE user_id = ? AND is_active = 0'; const userNameRes = await conn.query(checkTokenQuery, [userId]); - let storedToken = ''; + let storedTokenHash = ''; for (const row of userNameRes) { - storedToken = row.activation_token; + storedTokenHash = row.activation_token; } - if (storedToken!== token) { + if (!storedTokenHash || !bcrypt.compareSync(token, storedTokenHash)) { return false; } - const activateQuery = 'UPDATE users SET is_active = 1, activation_token = null WHERE user_id =?'; + const activateQuery = 'UPDATE users SET is_active = 1, activation_token = null WHERE user_id = ?'; const activateRes = await conn.execute(activateQuery, [userId]); + await conn.commit(); return activateRes.affectedRows !== 0; } catch (err) { + await conn.rollback(); throw err; } finally { - // Return connection await conn.end(); } } @@ -96,9 +102,11 @@ export const activateUser = async (userId: number, token: string): Promise => { +export const login = async (email: string, password: string, ip: string): Promise => { let conn = await NachklangCalendarDB.getConnection(); try { + await conn.beginTransaction(); + // Get saved password hash const query = 'SELECT user_id, password_hash FROM users WHERE email = ?'; const userRows = await conn.query(query, email); @@ -111,8 +119,7 @@ export const login = async (email: string, password: string, ip: string): Promis // Check for correct password if (!bcrypt.compareSync(password, savedHash)) { - // Wrong password, return invalid - return {} as Session; + return null; } // Generate + hash session key @@ -138,9 +145,9 @@ export const login = async (email: string, password: string, ip: string): Promis lastIP: ip }; } catch (err) { + await conn.rollback(); throw err; } finally { - // Return connection await conn.end(); } }; @@ -148,9 +155,11 @@ export const login = async (email: string, password: string, ip: string): Promis /** * Checks if the given session information are valid and returns the user information if they are */ -export const checkSession = async (sessionId: string, sessionKey: string, ip: string): Promise => { +export const checkSession = async (sessionId: string, sessionKey: string, ip: string): Promise => { let conn = await NachklangCalendarDB.getConnection(); try { + await conn.beginTransaction(); + // Get saved session key hash const query = 'SELECT user_id, session_key_hash, valid_until FROM sessions WHERE session_id = ?'; const sessionRows = await conn.query(query, sessionId); @@ -165,30 +174,26 @@ export const checkSession = async (sessionId: string, sessionKey: string, ip: st // Check for correct key if (!bcrypt.compareSync(sessionKey, savedHash)) { - // Wrong key, return invalid - return {} as User; + return null; } // Check if the session is still valid if (validUntil <= new Date()) { - // Session expired, return invalid - return {} as User; + return null; } // Update session entry in SQL const updateSessionsQuery = 'UPDATE sessions SET last_IP = ? WHERE session_id = ?'; - const userIdRes = await conn.query(updateSessionsQuery, [ip, sessionId]); + await conn.query(updateSessionsQuery, [ip, sessionId]); await conn.commit(); // Get the other required user information const userQuery = 'SELECT user_id, email, full_name, is_active FROM users WHERE user_id = ?'; const userRows = await conn.query(userQuery, userId); - let username = ''; let email = ''; let fullName = ''; let is_active = false; for (const row of userRows) { - username = row.username; email = row.email; fullName = row.full_name; is_active = row.is_active; @@ -203,9 +208,9 @@ export const checkSession = async (sessionId: string, sessionKey: string, ip: st isActive: is_active }; } catch (err) { + await conn.rollback(); throw err; } finally { - // Return connection await conn.end(); } }; @@ -213,6 +218,8 @@ export const checkSession = async (sessionId: string, sessionKey: string, ip: st export const initiatePasswordReset = async (email: string): Promise => { let conn = await NachklangCalendarDB.getConnection(); try { + await conn.beginTransaction(); + const checkUsernameQuery = 'SELECT user_id, full_Name FROM users WHERE email = ?'; const userNameRes = await conn.query(checkUsernameQuery, [email]); if (userNameRes.length === 0) { @@ -239,9 +246,9 @@ export const initiatePasswordReset = async (email: string): Promise => await MailService.sendMail(email, 'Password Reset', `Hello ${fullName},\n\nYou requested a password reset for your BonkApp account. If you did not request this, please ignore this email.\n\nTo reset your password, please use the following reset token:\n\n${resetToken}`); return true; } catch (err) { + await conn.rollback(); throw err; } finally { - // Return connection await conn.end(); } } @@ -249,6 +256,8 @@ export const initiatePasswordReset = async (email: string): Promise => export const finalizePasswordReset = async (email: string, token: string, newPassword: string): Promise => { let conn = await NachklangCalendarDB.getConnection(); try { + await conn.beginTransaction(); + const checkTokenQuery = 'SELECT user_id, pw_reset_token_hash FROM users WHERE email = ?'; const userNameRes = await conn.query(checkTokenQuery, [email]); if (userNameRes.length === 0) { @@ -276,9 +285,9 @@ export const finalizePasswordReset = async (email: string, token: string, newPas return false; } catch (err) { + await conn.rollback(); throw err; } finally { - // Return connection await conn.end(); } }