Move the calendar onto the shared session cookie
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>
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