A fresh-context review before deploying found two things that would have
broken production, both in the runbook rather than the code.
The deploy order named only migration 003. Production has none of the three -
001 and 002 were only ever applied to the dev database - and the new API reads
the columns they add on every request, so following it literally would have
500'd every calendar call including the anonymous feed the public website
uses. Step 4 now carries a numbered checklist with a verification query.
APP_ORIGINS replaces the code's default list rather than adding to it, so
naming calendar.nachklang.art in DEFAULT_APP_ORIGINS is not enough if that
variable is set on the vhost - and its failure mode is the quiet one the
config already warns about, where everything works except sign-out. Added to
the same checklist.
Also from the review:
The two operands of the read guard on /json/next and /ical were swapped so the
password check short-circuits first. They are side-effect free, so the order
was free - but the old one put an admin-database query in front of the public
feed for any caller holding a .nachklang.art cookie, which is a dependency
that feed has never had. Two tests now assert the admin database is not
consulted at all.
/:calendar/json/next had no route-level test, despite being the endpoint the
public website actually calls and the property named as load-bearing. Covered
now, along with the rest of its credential matrix.
Migrations 001 and 002 gained IF NOT EXISTS. They are applied by hand with no
tracking table, so a partial re-run should be a no-op rather than an error
that aborts the rest of the paste. Verified by applying all three twice to a
throwaway container and diffing against the dev schema.
Swagger: two descriptions still claimed authentication was required where the
public calendar needs none, the calendar enum omitted `birthdays`, and a
`createdBy` request-body field was documented and read but never persisted -
misleading in a way that suggests a client can set authorship. Removed. The
CORS comment describing the calendar's query-parameter sessions is no longer
true and was rewritten.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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>
Prep PR for the admin auth module (docs/plan-admin-auth.md step 1).
better-auth 1.7 ships ESM only, so the API moves off CommonJS:
- "type": "module", module nodenext, target ES2024, .js suffixes on all
relative imports, require('mariadb'|'cors') replaced by imports, and
export= packages (winston, app-root-path, bcrypt) consumed via default
imports. The logger now uses appRoot.path explicitly.
- TypeScript 5.9, @types/node 26, tslint removed. Node 26 pinned via
engines and .nvmrc (Plesk runs 26).
- Jest 28 + ts-jest replaced by vitest 5. Eight test files depend on
hoisted module mocks with static imports and resetModules + require,
which Jest's ESM mode does not support; vitest keeps them nearly
verbatim. Coverage via @vitest/coverage-v8 (lcov), Sonar generic report
via vitest-sonar-reporter, so sonar-project.properties is unchanged.
vitest.config.ts sets FEEDBACK_IP_SALT so the suite passes without a
local .env.
- dotenv 8 -> 16 and axios 0.24 -> 1.x: their old typings are not
resolvable under nodenext.
- autoCommit: false dropped from the pool configs; it is not a mariadb
connector option and was silently ignored.
tsc clean, 96/96 tests green, compiled app boots and serves /, /docs and
CORS under Node ESM.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Paddy
