Paddy
b848d6eab9
Step 4 of docs/calendar-auth-migration.md, and the close of
DEFERRED_SECURITY.md item 1: no calendar route reads sessionId/sessionKey from
the query string any more, so a live credential no longer travels through
access logs, browser history and Referer headers.
The four write routes sit behind requireAppAccess('calendar'), which also
narrows who may edit from "any activated @nachklang.art account" to an
explicit per-user permission. They answer 401 signed out and 403 without the
permission, where they previously answered 403 for both.
The three read routes cannot use the middleware: one URL serves an anonymous
visitor, an iCal subscription holding a shared password, and a signed-in
editor who should see drafts. They resolve the session optionally instead, and
a signed-in user without the calendar permission is treated as anonymous
rather than refused - so they keep the public calendar access anyone has.
That public calendar staying anonymous is load-bearing: nachklang.art reads it
to show the next upcoming event. It is now pinned at both the password-table
and the route level, and so is the rule that a shared password can never be
used to write.
credentials.service.ts loses its session half and becomes the password table
it always wanted to be. The shared passwords survive only for iCal clients,
which cannot send a cookie.
Writes record the author as an admin user id and no longer have a legacy int
to write, which is what migration 003 makes room for.
/calendar/users/* is left in place: nothing calls it and a session it mints
opens nothing, but they are still live password-accepting endpoints, so
removing them belongs with the rest of the legacy path in step 5.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
80 lines
3.9 KiB
Markdown
80 lines
3.9 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. Nothing calls them any more, and a
|
|
legacy session they mint no longer opens anything, but they are still live
|
|
password-accepting endpoints. Step 5 removes them.
|
|
|
|
---
|
|
|
|
## 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
|
|
|
|
> **Superseded for new accounts (2026-09-05).** The admin module
|
|
> (`src/models/admin/`) replaced account creation for the feedback, tickets and admin
|
|
> apps: accounts now come from `invitations`, whose tokens expire after 7 days and are
|
|
> stored only as a SHA-256 hash. The item below still stands for the legacy calendar
|
|
> `users` table, which the admin module deliberately left alone - see
|
|
> `docs/calendar-auth-migration.md`.
|
|
|
|
**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
|
|
|
|
> **Superseded for new accounts (2026-09-05).** Password resets for admin-module accounts
|
|
> go through better-auth, whose reset tokens expire after one hour. As with item 3, the
|
|
> text below still applies to the legacy calendar `users` table.
|
|
|
|
**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.
|