Paddy
aa95ab2745
Step 5, the last one, of docs/calendar-auth-migration.md. Step 4 is deployed and verified, which is what this was waiting on: it removes the fallbacks that step 4 still leaned on. Gone: src/models/calendar/users/ entirely - registration, login, activation, both password-reset routes, and the session checking that the feedback and tickets admin areas used to authenticate against - along with its mount. That was the API's last unauthenticated account-creation and mail-sending endpoint. A survey confirmed nothing outside that directory imported it and nothing else touched its tables. Also gone: the two joins against the calendar users table in events.service.ts and the created_by_id / version_created_by_id columns they read, from the SQL, the row mapper, the Event interface and the swagger schema; and X-Session-Id / X-Session-Key from the CORS allowedHeaders, which nothing has read since the first cutover and nothing has sent since the second. An event's author still renders, because migration 002 snapshotted the names before this could erase them. That was brought forward from this step on purpose, and it is the reason 004 can rename the accounts aside at all. The accounts are renamed rather than dropped - they still hold e-mail addresses and password hashes, and a rename makes them unreachable without destroying anything. InnoDB rewires the sessions foreign key to the new name; verified on MariaDB 11, along with the whole 001-004 chain from the pre-cutover production schema, which lands byte-identical to a fresh dev database. Migration 004 must be applied AFTER deploying, not before - the reverse of step 4, whose migration only added things. Its own header and the runbook both say so, since getting it wrong by analogy is the obvious mistake. DEFERRED_SECURITY.md items 3 and 4 close with it: the activation and reset tokens that never expired are gone along with the code that issued them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
66 lines
3.3 KiB
Markdown
66 lines
3.3 KiB
Markdown
# 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) — CLOSED 2026-09-06
|
|
|
|
**Files:** `src/models/calendar/events/events.router.ts` — all GET/PUT/DELETE handlers
|
|
|
|
`sessionId` and `sessionKey` were read from query parameters, which meant they appeared in
|
|
server access logs, browser history, proxy logs, and `Referer` headers.
|
|
|
|
**Fixed** by step 4 of `docs/calendar-auth-migration.md`: the calendar's write routes now sit
|
|
behind `requireAppAccess('calendar')` against the better-auth session cookie, and the read
|
|
routes resolve the same cookie optionally. No route reads `sessionId`/`sessionKey` any more,
|
|
and the Angular frontend sends `withCredentials` instead of appending them to every URL. That
|
|
closed the item outright rather than moving the credential somewhere safer.
|
|
|
|
Two things this did *not* change, both deliberate:
|
|
|
|
- The shared calendar `password` parameter stays. An iCal client cannot send a cookie, so
|
|
this is the one caller that genuinely needs a credential in the URL. It grants read access
|
|
to one calendar and nothing else - `test/calendar/events.router.test.ts` pins that it can
|
|
never be used to write.
|
|
- ~~The legacy `/calendar/users/*` routes still exist.~~ Removed by step 5 on 2026-09-06,
|
|
along with the `users` and `sessions` tables they used - renamed aside rather than dropped,
|
|
so nothing was destroyed.
|
|
|
|
---
|
|
|
|
## 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 account holding the `calendar` permission can edit, move, or delete any event regardless of who created it. This is acceptable while everyone holding it is a trusted admin.
|
|
|
|
**Fix (updated 2026-09-06):** fetch the event first and verify `event.createdByUserId === res.locals.admin.id` before allowing the mutation — `createdById`, the legacy INT, is no longer written and is gone at step 5. Rather than an `isAdmin` flag, the bypass belongs in the permission model that already exists: `requireAppAccess('calendar', 'manage')` alongside the current `access` role, which needs a row in `APP_ROLES` on both sides and nothing else.
|
|
|
|
---
|
|
|
|
## 3. Activation token has no expiry — CLOSED 2026-09-06
|
|
|
|
**File:** ~~`src/models/calendar/users/users.service.ts`~~ — deleted.
|
|
|
|
The e-mail activation link was valid indefinitely. Closed not by adding an expiry but by
|
|
removing the thing that issued it: step 5 of `docs/calendar-auth-migration.md` deleted the
|
|
calendar's own account system. Accounts now come only from the admin module's `invitations`,
|
|
whose tokens expire after 7 days and are stored as a SHA-256 hash.
|
|
|
|
Any activation link still sitting in an inbox now 404s. It only ever activated a legacy
|
|
account, which no longer opens anything.
|
|
|
|
---
|
|
|
|
## 4. Password reset token has no expiry — CLOSED 2026-09-06
|
|
|
|
**File:** ~~`src/models/calendar/users/users.service.ts`~~ — deleted.
|
|
|
|
Same as item 3: `pw_reset_token_hash` never expired, and the code that set it no longer
|
|
exists. Password resets go through better-auth, whose reset tokens expire after one hour.
|