Read calendar event creators from the admin module, and archive the old ones (#14)
Jenkins Production Deployment
Jenkins Production Deployment
Reviewed-on: #14 Co-authored-by: Patrick Müller <mail@pmueller.me> Co-committed-by: Patrick Müller <mail@pmueller.me>
This commit was merged in pull request #14.
This commit is contained in:
+18
-17
@@ -5,27 +5,28 @@ These items were identified during a security review on 2026-05-02 and conscious
|
||||
|
||||
---
|
||||
|
||||
## 1. Session credentials in URL query parameters (logged-in users)
|
||||
## 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` are currently read from query parameters, which means they appear in server access logs, browser history, proxy logs, and `Referer` headers.
|
||||
`sessionId` and `sessionKey` were read from query parameters, which meant they appeared in
|
||||
server access logs, browser history, proxy logs, and `Referer` headers.
|
||||
|
||||
**Fix (updated 2026-09-06):** Move the calendar onto the shared admin identity -
|
||||
`requireAppAccess('calendar')` against the better-auth session cookie, per
|
||||
`docs/calendar-auth-migration.md`. That closes this item outright rather than moving the
|
||||
credential to a safer place, and it is now the cheaper of the two: the feedback and tickets
|
||||
modules made the same move on 2026-09-06 for one line each.
|
||||
**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.
|
||||
|
||||
~~Move to request headers (`X-Session-Id` / `X-Session-Key`) or the request body.~~ No longer
|
||||
the recommendation. Nothing on the server reads those two headers any more - the calendar's
|
||||
query parameters are the last legacy credential path in the API - so this would build a second
|
||||
mechanism just as the first is being retired. They survive only in the CORS `allowedHeaders`
|
||||
list, and only until both frontends are redeployed.
|
||||
Two things this did *not* change, both deliberate:
|
||||
|
||||
Either fix 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.
|
||||
- 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.
|
||||
|
||||
---
|
||||
|
||||
@@ -36,9 +37,9 @@ Either fix requires a corresponding frontend 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.
|
||||
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:** 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.
|
||||
**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.
|
||||
|
||||
---
|
||||
|
||||
|
||||
Reference in New Issue
Block a user