Backend and database hardening: finish the identity migration and pay down the debt around it - #198
Open
enko wants to merge 136 commits into
Open
Backend and database hardening: finish the identity migration and pay down the debt around it#198enko wants to merge 136 commits into
enko wants to merge 136 commits into
Conversation
Updating `auth.users.email` alone desynchronised it from `auth."user".email`, and `authMiddleware` resolves the legacy row by the session email, so a single successful request bricked the account with a permanent 401. Email changes need Better Auth's verified change-email flow, which is not wired up yet. Until then the address is read-only: the route, the shared `UpdateProfileRequestSchema`, the `UpdateUser` / `UpdateUserReturningWithSelfProfile` / `GetUserByExternalId` queries (the latter had no callers), the frontend client function and the account-page edit form all go away. BREAKING CHANGE: PUT /api/users/me is removed; email is read-only until a verified change-email flow exists.
Preferences are written to `auth."user".preferences` (see `UpdateUserPreferences`), but `GetEnabledChannelsDueAt` read `auth.users.preferences`, which no write path touches. Every notification digest therefore fell back to `en` regardless of the user's setting. Join the Better Auth row via `bu.id = u.external_id::text` and read the language from there.
Better Auth lowercases on sign-up, but nothing stopped the legacy table or a direct insert from storing mixed case, and every non-Better-Auth lookup (SabreDAV principal/CardDAV/app-password, MCP basic auth) compared raw strings. A mixed-case address therefore matched in the app and missed in DAV. Backfill both tables and add CHECK constraints so the invariant holds for all three consumers, and normalise the input side: the app-password service lowercases once (MCP reaches the database through it), the PHP lookups compare `email = lower(:email)`. Case collisions abort the migration instead of merging: two rows differing only in case are two accounts with two sets of friends, and only the operator can decide which survives.
`TEST_DATABASE_URL` now overrides the testcontainers path in `global-setup.ts`: the `test` template database is built on the given server and every suite clones from it exactly as before. Two reasons. The CI jobs added for schema verification already run a postgres service, so spawning a second server per job is waste. And testcontainers needs Docker to publish ports, which fails outright on hosts whose running kernel has no matching netfilter module tree — the suite was then unrunnable rather than slow. Container behaviour is unchanged when the variable is unset.
… lookup Both shipped compose files put nginx in front of the backend, but neither set `TRUST_PROXY`, so `getClientIdentifier` fell back to the socket peer address — the proxy — and every client in the world shared a single rate limit bucket. The address-lookup router was also the only one without a limiter, while being the only one that proxies to third-party geocoders whose usage policies apply per deployment IP. One abusive client could get the whole instance banned from Overpass/Nominatim. The self-hosting note that told operators to add `TRUST_PROXY` themselves is updated: it ships set now.
Every vCard PUT returned a 500. `createCard`/`updateCard` still wrote `job_title`, `organization`, `department` and `work_notes` to `friends.friends`, but the professional-history migration moved those columns to `friends.friend_professional_history` — so the statement failed on an undefined column before touching a row. The parsed history array the mapper already produces is now inserted into that table (and cleared with the other sub-resources on update). Two more faults on the same paths: - PDO's pgsql driver binds PHP `false` as an empty string, which Postgres rejects for a boolean column, so any card with a non-primary phone, email, address or an unset `is_favorite` failed. Bind the literal. - `external_id` is a uuid column, so a client card URI that is not a UUID raised 22P02, i.e. a 500. Reads and deletes now miss cleanly and `createCard` answers 400, because a card that cannot be addressed by the URI the client chose is not storable. Scope is `backend` because `sabredav` is not an allowed commit scope.
The suite gated on `pnpm --version`, but pnpm was dropped for aube (mise.toml), so all four integration classes skipped silently — while being the only guard against the PHP SQL drifting from the migrated schema. Migrations now run the same way the backend does: node-pg-migrate under tsx from the backend workspace. Docker-less local runs still skip; under `CI=true` a missing prerequisite fails instead. Everything the un-skip surfaced is fixed too: - `TEST_DATABASE_URL` selects an existing server, matching the Node suite. - Sequence resets resolve the sequence from its owning column: the tables were renamed contacts -> friends, their sequences were not. - `cleanupData` also clears `auth.account`, `auth.session` and `auth."user"`, and `createTestUser` creates the Better Auth identity row that the DAV lookups actually read. - `createTestFriend` writes professional data to `friends.friend_professional_history`; the assertions read it back from there. - A local `SapiMock` replaces `Sabre\HTTP\SapiMock`, which lives in sabre/http's own test suite and is not in the dist package. - Card URIs in fixtures are UUIDs, the email-case test asserts case-insensitive lookup against lowercase storage, and the vCard version assertions match what sabre/dav negotiates for a client that does not advertise 4.0. Scope is `backend` because `sabredav` is not an allowed commit scope.
`failOnError` was false and nothing regenerated the queries in CI, so a `.sql` edit that was never re-run through PgTyped shipped a `.queries.ts` whose row types no longer matched the database — a type lie the compiler cannot catch. The new job migrates a throwaway PostGIS service, regenerates, and fails on `git diff`. `failOnError` is on so a query that no longer prepares stops the run instead of emitting a partial file. `dbUrl` (with its committed dev credentials) is gone from `pgtyped.config.json`: PgTyped already prefers `DATABASE_URL` from the environment. The scripts load `.env` when present with `--env-file-if-exists`, which also lets the migrate scripts run in CI where there is no `.env`.
The size check ran in `PhotoService.uploadPhoto`, i.e. after `c.req.formData()` had already buffered the whole body into memory — an unauthenticated-adjacent route that any signed-in user could use to push arbitrary bytes through the process. A `bodyLimit` now rejects with 413 before buffering. `sharp()` also ran without `limitInputPixels`, so a small, highly compressed file could expand to gigabytes of decoded pixels. Cap it at 50 MP and decode once, cloning the pipeline for the two outputs instead of parsing the buffer twice.
The guard only pattern-matched the hostname string, so any public name that resolves to a private address walked straight through — a wildcard DNS service like `127.0.0.1.nip.io`, or simply an attacker-controlled A record. The user's own notification channel is enough to make the backend issue requests into the deployment's network, including the cloud metadata endpoint. The hostname is now resolved and every returned address checked against one `isPrivateAddress` helper (loopback, link-local incl. 169.254.169.254, RFC-1918, RFC-6598 CGNAT, "this network", IPv6 unique/link-local, and the IPv4-mapped IPv6 forms of all of them). `http:` is rejected outright; a homeserver reachable only over plaintext is either private or already broken. DNS rebinding between check and connect remains theoretically possible; the access token is the user's own, so there is no useful response channel.
`tsc --project tsconfig.build.json` compiled every colocated `*.test.ts` into `dist`, shipping vitest imports and mocks in the runtime image. Exclude them from the build and from the coverage denominator, and move `src/routes/auth.test.ts` to `tests/routes/` where the rest of the route tests live.
`jsonwebtoken` has had no importers since Better Auth took over sessions, and `sharp` ships its own types — `@types/sharp@0.32` described a version three majors old. The lockfile is pruned by hand: `aube remove` rewrites the manifest from its own model (it dropped `scripts`, `exports`, `type` and `private`) and then fails to relock a workspace package, and re-resolving with pnpm rewrites unrelated ranges. Only the three `apps/backend` importer entries are gone; `jsonwebtoken`'s package entry stays because `danger` depends on it. Verified with `aube ci`.
…upload migration Everything removed here has zero callers, verified by grep across `apps/`, `packages/` and `docs/`: - `InvalidSessionError`, `InvalidTokenError`, `UserAlreadyExistsError`, `UserCreationError`, `PreferencesUpdateError`, `ServiceNotConfiguredError` and `getErrorStatusCode` in `utils/errors.ts` — leftovers from the pre-Better-Auth session system. The AGENTS.md error table is trimmed to match, since it is the reference contributors copy from. - `ENABLE_API_DOCS` in `utils/config.ts`: nothing read it. Its tests are retargeted at `TRUST_PROXY`, which uses the same `BooleanString` parser and is now load-bearing for rate limiting. - `cleanupAllAddressCaches` in `utils/cache.ts`. - `PhotoService.migrateFromLegacyPath` and its startup call: the contacts -> friends rename shipped several releases ago, so every install has already migrated and the check only cost a `stat` per boot. The two migrations that share timestamp 1779667500000 keep it — both are applied in production and renaming either re-runs it — with a comment on each saying so.
…r for dev BETTER_AUTH_SECRET `JWT_SECRET` and `SESSION_SECRET` have had no reader since Better Auth took over sessions; keeping them in `.env.example` and the dev compose file implied they still mattered. The dev compose default for `BETTER_AUTH_SECRET` was `dev-better-auth-secret-change-in-prod`, which passes `SecretType` — so copying the dev compose into production yielded a boot with a publicly known secret. `...-change-this-before-prod` trips the placeholder lookahead and refuses to boot instead.
…l_id The Better Auth migration was never finished. It left two user tables with nothing relating them but the email address: legacy `auth.users` (SERIAL `id`, UUID `external_id`, email, password hash) and Better Auth's `auth."user"` (TEXT `id`). Every authenticated request joined them by email, and three consumers each kept their own copy of that join. `auth.users` stays as the FK anchor because eleven domain tables carry an integer FK to its `id`; re-keying those to TEXT is eleven FK rewrites plus wider indexes on the largest tables. The six Better Auth child FKs (`session`, `account`, `passkey`, `oauth_application`, `oauth_access_token`, `oauth_consent`) get `ON UPDATE CASCADE` instead, so re-keying `auth."user".id` to the legacy UUID propagates — needed for accounts created after the original migration, whose ids are not UUIDs. Dropped from `auth.users`: `email`, `password_hash`, `preferences`, `self_profile_id`, plus the pre-Better-Auth `sessions` and `password_reset_tokens` tables, which no code has written to since the migration. `auth.users` keeps `id`, `external_id`, `created_at`, `updated_at` and nothing else. Two safety rails: the migration aborts on any row the other table cannot match rather than guessing, and `auth.delete_orphan_legacy_users()` reaps anchor rows left behind by a sign-up that failed after allocating one. `down()` restores the columns and tables and backfills from `auth."user"`; `password_hash` comes back empty, since Better Auth has owned credentials since ADR 0001.
…d drop email bridge
Sign-up now allocates the `auth.users` anchor in a `user.create.before`
hook and returns its `external_id` as the new Better Auth user id, so
`auth."user".id = auth.users.external_id` holds from the first write
instead of being reconciled by email afterwards. Better Auth merges the
hook's `data` into the create payload and inserts with `forceAllowId`.
`authMiddleware` therefore drops its per-request lookup — one DB round trip
less on every authenticated request — and `AuthContext` collapses to
`{ userId, email }`. `betterAuthId` is gone; callers no longer have to
decide which of two ids a query wants.
A `user.delete.after` hook removes the anchor row, which cascades the
user's friends, encounters and collectives. Nothing did that before: the
two identity tables have no FK between them, so deleting a Better Auth
user left the domain data behind.
`user.changeEmail` is set to `enabled: false` explicitly. Better Auth
already rejects `email` on `/update-user`, but the address is no longer
mirrored anywhere, so the decision to require a verified flow deserves to
be visible in the config.
Query changes: `GetUserByEmailWithSelfProfile` becomes
`GetUserWithSelfProfile` keyed by id, `SetUserSelfProfile` pairs the tables
on `external_id::text` instead of email, and the bridge queries
(`GetLegacyExternalIdByEmail`, `CreateLegacyUserForBetterAuth`) are gone.
Fixtures and the seed script create both rows the same way the hook does.
…word users via auth."user" The hourly cleanup still deleted from `auth.sessions` and `auth.password_reset_tokens`, which the identity migration dropped — Better Auth has owned both since its migration. It now calls `auth.delete_orphan_legacy_users()` instead, which is the one thing that actually needs sweeping: a sign-up that fails after the create hook allocated its anchor row. `GetUserByEmailWithInternalId` reads the address from `auth."user"` and joins the anchor on `external_id::text`, since `auth.users.email` no longer exists. Column names in the result are unchanged, so the service and its tests are untouched.
`resolveLegacyExternalId` joined `auth."user"` to `auth.users` by email to translate an OAuth token's subject into the id the MCP tools scope by. The subject already *is* that id, so the join is gone; only the address for the session log still needs a read, and it comes from `auth."user"` directly. Test fixtures create the anchor row and adopt its UUID, matching the backend's create hook.
`auth.users.email` no longer exists, so every DAV lookup that resolved a principal or an app-password login by address joins the identity row on `external_id::text = id` instead: the app-password backend, the address-book lookup, and the five principal-backend queries (list, by-path, the two searches, and findByUri). Fixtures create the anchor row without identity columns and the Better Auth row with the address, matching the backend's create hook. Scope is `backend` because `sabredav` is not an allowed commit scope.
Records why two user tables existed, why the Better Auth ids were re-keyed to the legacy UUIDs rather than the reverse, and what the arrangement costs — an invariant Postgres cannot enforce, and a two-step sign-up that needs an orphan reaper. The database conventions doc gains the invariant next to its existing Better Auth exception, since that is where contributors look before adding a query.
Four queries accepted an external id with no ownership predicate: `GetRolesForType` and `GetRulesForType` returned the roles and auto-relationship rules of *any* collective type, and `GetRoleInternalId` / `GetRoleByExternalId` resolved any role id as long as the collective id was well-formed. System types stay visible to everyone; custom types now resolve only for their owner, and role lookups join the collective's owner. `GetMemberPreviewBatch` relied on its caller having filtered the collective ids, and `GetMemberPreview` had no caller at all and is deleted. `tests/sql-tenancy.test.ts` makes the rule enforceable instead of a habit: it parses every `*.sql` block and requires a join on `auth.users` whenever a user-owned table appears. Queries that legitimately cannot scope by user (cron sweeps, internal-id helpers whose caller already checked ownership, identity-keyed reads) are allowlisted with a reason, and a second assertion fails when an allowlist entry no longer matches any query. The conventions doc promised row-level security that was never implemented — a policy would need a per-request `current_setting` and a second enforcement point that can disagree with the first. It now documents what is actually enforced, and where.
…schema
"At most one primary" lived only in `SubResourceService`, which clears the
flag before setting it. SabreDAV writes the same tables straight from vCard
PUTs and never had that logic, so a card with two `TYPE=pref` entries
produced two primaries — and every read path picks the primary with
`LIMIT 1`, so which one won depended on physical row order.
A partial unique index on `(owner) WHERE is_primary` binds all three
consumers. Existing duplicates are normalised to the lowest id per owner
first: that is the oldest row, and therefore the one the user most likely
set deliberately.
Covers `friend_{phones,emails,addresses,professional_history}` and
`collective_{phones,emails,addresses}`.
`getCards` and the mapper's single-card read both filtered `archived_at IS NULL`, but three other paths did not, so the collection and its members disagreed: - `getChangesForAddressBook` reads the change log, which knows nothing about archiving, and reported an archived friend as added or modified — a URI the client then could not fetch. Those URIs are now reported as deletions, which is what the client has to do with them anyway. - `updateCard` located the row without the archived predicate, so a PUT silently edited a card the client cannot read back. - `deleteCard` likewise. Scope is `backend` because `sabredav` is not an allowed commit scope.
A migration nobody ever rolls back is a migration whose `down()` is
fiction, which is exactly when it matters — during an incident. The new job
migrates a throwaway PostGIS service all the way up, all the way down, and
up again.
It immediately caught one: `1768100000000_faceted-search-indexes.down()`
dropped `idx_contacts_{organization,job_title,department}` unconditionally,
but the professional-history migration had already moved those columns and
their indexes away, so a full rollback aborted there and left the chain
half-reverted. Those drops are `ifExists` now.
…ters The maintainer's deployment migrates from `deploy.sh` before restarting the stack. Anyone else following the self-hosting guide had a manual step after every upgrade, and a window in which new code talked to an old schema. A one-shot `migrate` service runs the compiled migrations from `database/dist` (already in the backend image, and `node-pg-migrate` is a production dependency), and `backend` waits on `service_completed_successfully`. A failed migration now keeps the old backend running instead of replacing it with one that expects a schema it did not get. `BETTER_AUTH_SECRET` is added to the backend service — it was only on mcp-server, so the shipped file described a backend that cannot boot. The five pre-Better-Auth secrets (`JWT_SECRET`, `SESSION_SECRET`, `JWT_EXPIRY`, `SESSION_EXPIRY_DAYS`, `PASSWORD_RESET_EXPIRY_HOURS`) are gone from the service and from the test env stubs; nothing has read them since ADR 0001. `docs/self-hosting.md` documents the new behaviour, the two migrations that deliberately abort rather than guess, and `pg_dump`/`pg_restore` commands with an explicit "dump before upgrading" — `unify-user-identity` drops columns and its `down()` cannot restore password hashes.
…ated_at Nine indexes were costing write throughput and buffer cache for nothing. Duplicates of the index a UNIQUE constraint already creates: `auth.idx_ba_user_email` (`user_email_key`), `auth.idx_ba_session_token` (`session_token_key`), `auth.idx_ba_passkey_credential_id` (`passkey_credential_id_key`), `system.idx_address_cache_key` (`address_cache_cache_key_key`). Unusable by any query: `friends.idx_friend_dates_upcoming` is an expression index on `EXTRACT(month/day)` but `GetUpcomingDates` filters on the user and only projects those expressions; `friends.idx_friend_emails_address` is a btree on a column only ever matched with `ILIKE '%…%'`. Left-prefixes of a composite index every query already uses: `idx_friends_user_id` (covered by `idx_friends_display_name`), `idx_encounters_user_id` (`idx_encounters_user_date`), `idx_circles_user_id` (`idx_circles_sort_order`). `idx_friend_phones_trgm` is *not* dropped here even though the review listed it: phone search still uses `phone_number ILIKE '%…%'`, which the trigram index does serve. It is replaced in the German-FTS migration, where its successor exists. Also: `encounters.location_address_id` becomes `bigint` to match `geodata.addresses.id`; `geodata.addresses.import_batch_id` gets the cascading FK the OSM import already assumes; and the eleven user-editable sub-resource tables get `updated_at` plus a trigger, so a client can tell an edited phone number from a re-created one. `friends.friend_changes` gains a 90-day retention sweep in the hourly cleanup — it is append-only and had no bound at all.
`listFn` and `mapListResult` were optional in `SubResourceConfig`, and `list()` threw a raw `Error` at runtime for the six friend services that did not set them — even though each already imported exactly the query it needed as `countFn` (used only to detect "is this the first entry, make it primary"). Both fields are required now, `countFn` is gone (it was always the same query as `listFn`), and the runtime throw with it. Each service hoists its row mapper to a module-level `mapRow`, so the list and single-row paths map identically by construction instead of by two copies that happened to match. `DateService` — which does not extend the base class, because of its birthday rules — gains the matching `list()`.
Eleven route files were the same forty lines with different nouns: parse a UUID owner param, parse a body, call the service, map null to a 404, return a delete message. `createSubResourceRouter` takes the parts that actually differ — param names, labels, schema, owner-not-found error, and how to construct the service — and the eleven files become eleven configs. This is what finally adds `GET /` for the friend sub-resources. Collectives had list endpoints; friends never did, even though every list query already existed in the service (it was the `countFn`). Phones, emails, addresses, urls, dates, social profiles and professional history are now listable. Phone normalisation moves to `services/phone-normalization.ts`. Four copies of it lived in the two phone route files (create and update), and each loaded the entire friend — with every sub-resource — just to read one country off the primary address. The factory's `preprocess` hook runs it against the addresses alone. `getAddressLookupService` moves from `routes/address-lookup.ts` to `services/address-lookup.registry.ts`: services depend on it, so a route module owning it pointed the dependency arrow backwards. It also no longer claims to return `undefined` — it always constructs an instance. The routes that are genuinely different stay hand-written: singleton met-info, relationships with their inverse edges, collective circle join/leave verbs, and the two read-only nested lists. With the delegating routes gone, `FriendsService`'s twenty-one add/update/delete pass-throughs have no callers and are deleted; `getUpcomingDates` stays, since it is not a sub-resource CRUD verb.
Three different mechanisms decided that a write collided:
`CirclesService` and `NotificationChannelsService` each had a private
`isUniqueViolation` that matched only the SQLSTATE, and the relationships
route matched `error.message.includes('unique_relationship')` — a substring
of a driver message, in a route, returning a bare 409 body.
Matching on the code alone is also wrong on any table with two unique
constraints: `friends.circles` has `circles_external_id_key` next to
`idx_circles_unique_name`, so a UUID collision would have been reported as
"a circle with that name exists".
`rethrowUniqueViolation(error, byConstraint)` keys on the constraint name
and rethrows anything it does not recognise. New `ConflictError` (409,
`code: 'CONFLICT'`) covers collisions with no more specific class, so the
relationship case moves out of the route and into
`RelationshipService.addRelationship` where the write happens.
That method also loses its hand-rolled `BEGIN`/`COMMIT`/`ROLLBACK` for
`withTransaction`, which is what the rest of the codebase uses and what
makes the rethrow safe (the rollback is not in the catch path any more).
`related_friend_id` is declared `string.uuid` in the shared schema, so the
route no longer re-checks it by hand.
`routes/users.ts` assembled the `User` DTO in three places, ran the
self-profile queries inline, and threw a bare `Error('Failed to set
self-profile after creation')` — a 500 with no code, on a path a user hits
during onboarding. The four operations move to `UsersService` with one
private `mapUser`, and the route becomes parse → service → `c.json`.
`mapMember` and `mapRole` existed as byte-identical private methods in
three collective services, each typed against a different PgTyped result
interface for the same projection. They move to
`models/mappers/collectives.mappers.ts` with structural row types, so a
column rename breaks one mapper instead of drifting three.
`SetSelfProfileSchema` moves to `@freundebuch/shared`, where the other
request schemas live.
Adding a member runs the collective's relationship rules, and each derived edge goes through `CreateRelationshipWithSource`. When the edge already exists the conflict clause overwrote `source_membership_id` with the membership that had just re-derived it — including when the existing row was one the user created by hand, where the column is NULL precisely to record "nobody owns this". `removeMember` then calls `DeleteRelationshipsByMembershipId`, so taking a member out of a collective deleted the manual relationship the user had entered themselves. There is nothing to undo it and no trace of why it went: from the outside a relationship simply disappears after an unrelated membership edit. The conflict clause now keeps whatever owner the row already has. `COALESCE(existing, EXCLUDED)` looks like the fix but is not one — a manual edge has `source_membership_id IS NULL`, so COALESCE yields EXCLUDED and the membership still takes it. Self-assignment is deliberate over `DO NOTHING`, which would stop `RETURNING` producing a row and break the caller. First-writer-wins has a known edge: if two memberships derive the same edge and the first is removed, the edge goes even though the second still justifies it. That is a pre-existing limit of tracking one source per relationship and needs a join table to fix properly; it is strictly better than deleting edges no membership ever created.
…owner `CheckDuplicateActiveMembership` took `:collectiveExternalId` and `:contactExternalId` straight from the request and joined no user at all. Both callers — `addMember` and `previewRelationships` — run it *before* the ownership lookups that resolve the internal ids, so it answered for any collective and any contact in the database. The result is a membership oracle: post someone else's collective id and someone else's friend id and the response distinguishes 409 (that friend is an active member of that collective) from 404 (they are not). No write happens either way, which is why it went unnoticed, but the read is exactly the information the tenancy boundary exists to withhold. The query now joins `auth.users` through the collective and matches `:userExternalId`, the way `GetRoleInternalId` and every other collective query does. Both callers already hold `userExternalId` in their signature, so nothing above them changes: a foreign pair now yields count 0, falls through to the ownership lookup, and gets the same 404 any other unknown collective gets. The allowlist entry in `tests/sql-tenancy.test.ts` claimed the collective id was "checked by the same caller". It was — three statements later.
`USER_OWNED` listed `collectives.collective_members`. The table is `collectives.collective_memberships`, so the substring never matched and every membership query was exempt from the scoping assertion — including the four that genuinely do not join `auth.users`, whose allowlist entries were therefore dead weight the test could never have exercised. `collective_roles` and `collective_relationship_rules` were absent from the list altogether. With the names corrected the test flags exactly four queries, all already allowlisted; three are honest internal-id helpers, and the fourth turned out to be a real tenancy hole (fixed in the preceding commit). The stale-entry assertion is also strengthened. It only caught entries whose query had been deleted, not entries whose query has since grown its own `auth.users` join and no longer needs the exemption. That is the more dangerous kind: the entry sits there looking justified and keeps the query exempt if the join is ever removed again. Two such leftovers are dropped — `ReactivateMembership`, which scopes through the collective owner like its siblings, and `GetEnabledChannelsDueAt`, whose deliberate cross-user scope is now stated in the SQL where a reader of the query will see it.
Setting a sub-resource as primary has to clear the old primary first — the partial unique indexes from `1779668200000` allow one primary per owner, so the two writes are ordered and wrapped in a transaction. `update()` cleared the flag, ran `updateFn`, and when that matched no row returned `null` and let `withTransaction` commit. So a PUT to a phone id that does not exist — a stale client, a mistyped id, a row deleted on another device — answered 404 *and* left the owner with no primary phone at all. Nothing in the response hints that anything was written, and the next read just shows every entry as non-primary. `add()` had the same shape for an unknown owner. Both now abort the transaction when the write produced no row, via a module-private sentinel thrown inside the callback and caught immediately outside it, so the rollback happens while the caller still sees `null` and the generated router still answers 404. No route or status changes. The `client?` last parameter of `add`/`update` is gone rather than fixed. No caller ever passed it — the router calls with three or four arguments and the two address overrides only forwarded it — so it existed solely to select a non-transactional path that cannot be made safe. Deleting it leaves one always-transactional path. `createMany` genuinely does need the caller's transaction: `FriendsService.createFriend` writes the friend and all its sub-resources atomically, and the sub-resource rows must see the uncommitted parent. Its per-row body moved to a `protected addWithin(client, …)` with a required client, and the address services override that instead of `add` — overriding only the public method would have silently dropped background geocoding for addresses created during friend creation.
`PaginatedFullTextSearch` and `FacetedSearch` both compute a `sort_position` via `row_number()` in their sorted CTE and end with `ORDER BY sr.sort_position`. `FilterOnlyList` — the listing you get with filters and no query, i.e. the default friend list — kept its `ORDER BY` inside the CTE only and had none on the outer select. Ordering a subquery guarantees nothing once the result is joined again, and the outer select cross-joins the `total_count` CTE. Postgres happens to plan a nested loop over that single row and emit the CTE's order, so this is not a visible bug today; it is an unspecified result that any plan change is free to reorder, on the one listing a user looks at most often. Made identical to its two siblings so the guarantee is explicit and the three queries can be read against each other. `sort_position` stays out of the outer projection, as in the siblings, so the generated result type and `SearchService.mapFilterOnlyResult` are unchanged. The test pins the contract in both directions but does not reproduce a failure: no reachable plan reorders the old query, which is why the defect survived review in the first place.
Better Auth caches the session in a signed cookie for five minutes, so `session.user.preferences` is whatever it was when the cookie was written. `/api/auth/me` served that copy, which means changing the language — or page size, or any other preference — and reloading showed the old value back again, for up to five minutes, with the database already holding the new one. It reads as the setting not having saved. The handler already had this exact problem for `selfProfileId` and already works around it by reading that column fresh; `preferences` was simply missed when the workaround was added. `u.preferences` joins the existing `GetUserSelfProfile` query rather than adding a second round trip — the handler already runs it on every call for the self-profile id.
`friend-changes.sql` was reworded from "at or below that watermark" to "below that watermark" without regenerating, so the embedded statement and the doc comment in `friend-changes.queries.ts` still carried the old text. Harmless in itself — the SQL body is identical — but `pgtyped-drift` in CI regenerates and runs `git diff --exit-code` over the directory, so the branch would have failed that job.
…story `deleteSubResources` gained a `DELETE FROM friends.friend_professional_history` when the professional-history table was wired into the CardDAV path, and `updateCard` then re-inserted whatever `Mapper::vcardToFriend` had synthesised — exactly one row, built from ORG/TITLE/NOTE, dated `from_month`/`from_year` = today, `to_*` NULL. A vCard has one ORG/TITLE slot and cannot express a history, so every PUT destroyed the friend's entire employment history: past positions gone, date ranges gone, and the current position silently re-dated to today. Clients re-PUT the whole card for any trivial edit — iOS does it on a phone-number change — so this needed no unusual action to trigger. Before the table existed the same PUT failed loudly on the missing columns, which is why nothing was lost until it started succeeding. `updateCard` now reconciles instead of replacing: it updates the primary row's job title, organization, department and notes and leaves `from_*`/`to_*` alone, inserts if there is no primary row yet, and does nothing at all when the vCard carries no professional data. That last case is a judgement call — absence in a vCard means "this client cannot represent it", not "the user cleared it", and most clients drop properties they do not understand. Treating it as a deletion would hand any such client the power to wipe the history it could not see. `createCard` still inserts: a friend that does not exist yet has nothing to preserve.
Archiving a friend removes them from the address book: `getCard`, `getCards` and `updateCard` all exclude `archived_at IS NOT NULL`. SabreDAV's `CorePlugin::httpPut` treats a node it cannot find as a creation and calls `createFile`, so a client that synced before the archive and then saves an edit lands in `createCard` — which INSERTs the same `external_id`. That column is globally UNIQUE, so the insert raises 23505, the transaction rolls back, and the client gets a 500. A 500 is the worst available answer here. iOS and macOS Contacts treat it as a transient server fault and keep the edit queued, so the client retries the same doomed PUT indefinitely and reports the account as broken, while the actual situation — this card is gone — is one it knows how to handle. `createCard` now rejects an `external_id` that already exists with `Sabre\DAV\Exception\Forbidden`, which Sabre maps to 403. The client drops the edit and picks up the `delete` the archive trigger already logged on its next sync. The check is deliberately not user-scoped: what would otherwise fail is a global uniqueness constraint, so scoping it to the requesting user would let the 23505 through for a card belonging to someone else.
The initial-sync branch listed the cards first and only then asked for the sync token, outside a transaction. A friend created between the two reads is therefore in neither: not in `added`, because the listing had already been taken, and not in any later incremental sync, because the token the client is handed is already newer than that friend's change-log entry. The friend then stays invisible on that device until something else touches the row — indefinitely, for a friend nobody edits again. Taking the token first inverts the failure: such a friend appears in `added` and again in the next incremental sync. A duplicate `added` is idempotent for every client, a missing one is not.
`friends.photo_url` holds a URL the frontend puts straight into an `<img src>`. The mapper wrote whatever the vCard `PHOTO` property contained, and Apple clients send photos inline — `data:image/jpeg;base64,…` in vCard 4, or a bare base64 body under `PHOTO;ENCODING=b;TYPE=JPEG` in vCard 3. So a multi-megabyte payload went into the column verbatim (accepted since the varchar-to-text migration) and the friend rendered as a broken image. The mapper now only sets `photo_url` for an absolute http(s) URL. That alone would have made things worse rather than better: `updateCard` binds `$friendData['photo_url'] ?? null`, so an omitted key nulls the column, and every Apple round-trip would have *deleted* the photo instead of corrupting it. The pre-transaction lookup therefore also reads the current `photo_url` and the binding falls back to it. The consequence is deliberate and matches the professional-history decision: a photo can no longer be cleared over CardDAV, only in the app. An inline payload is "not representable by this client", and treating it as a deletion would let any client that cannot express a URL destroy an upload it never saw. `createCard` keeps `?? null` — a new friend has nothing to preserve.
Apple Contacts writes grouped properties — `item1.EMAIL`, `item2.TEL`, `item1.X-ABLabel` — to tie a value to its custom label. The root cause was not the name matching but `parseLine`, the single place every property name comes from: its pattern `^([A-Za-z0-9-]+)(;[^:]*)?:(.*)$` cannot cross the `.`, so a grouped line failed to parse and was discarded before anything looked at its name. So an address book authored on an iPhone imported with its grouped emails, phone numbers and URLs silently missing — no error, no log line, just fewer contact details than the card contained. Anyone who had ever set a custom label was affected. `parseLine` now accepts an optional `itemN.` prefix, strips it, and returns the group alongside the property. Fixing it there covers both enumeration sites at once (`vcardToFriend`'s switch and `vcardToJson`); patching one of them would have left the other dropping the same lines. `vcardToJson` records the group under `entry['group']`, because its contract is to preserve every property and losing the prefix would break the `item1`↔`item1.X-ABLabel` association that gives a grouped value its label.
`check()` lowercased the authenticated login with PHP `strtolower()`, which is byte-wise and only folds ASCII. The lookup that authenticated the request matches with Postgres `lower()`, which is locale-aware under the cluster's `en_US.utf8` collation. For `MÜLLER@example.com` the two disagree: `müller@example.com` from Postgres, `mÜller@example.com` from PHP. The principal URI it returns therefore did not match the one `FreundebuchPrincipalBackend` exposes, so SabreDAV could not resolve the principal and sync failed for any address with a non-ASCII uppercase letter — after the password had already been accepted, which makes it look like an authorization bug rather than a string-folding one. Rather than reaching for `mb_strtolower`, `validateUserPass` now keeps the `auth."user".email` it matched and `check()` returns that. `auth."user"` carries `CHECK (email = lower(email))`, so the stored value is already the canonical form Postgres produced — identical by construction instead of by choosing a PHP function that happens to agree. Checked the other `strtolower` call sites in `apps/sabredav`: log level names and vCard TYPE keywords, none of which is a principal.
A leftover debugging block decoded the Basic `Authorization` header on every
single CardDAV/CalDAV request and wrote the username — an e-mail address — plus
the encoded, decoded and username byte lengths to `error_log`, which is the
webserver log. Unconditional: no level gate, no environment check.
In a personal CRM the account address is exactly the kind of identifier that
should not be sitting in a plaintext log that rotates on its own schedule and
is readable by anyone with host or container log access. Syncing clients poll,
so this accumulated one line per device per poll interval indefinitely. The
length fields make it worse than an identifier leak: `decoded_len` minus
`username_len` discloses the app password's length to anyone reading the log.
Deleted rather than gated. It was written to chase a header-truncation problem
that is long fixed, everything in it is either an identifier or a credential
oracle, and the structured `$logger->debug('CardDAV request received', …)` line
directly above already records the method and path. Failed authentication is
still logged, with its own reason code, by `AppPasswordBackend`.
Seven date fields reached Postgres `date` columns without ever being validated, so a bad value was a 22007 from the driver — a 500 — instead of the 400 the boundary exists to produce. Two distinct causes. `EncounterUpdateSchema`, `MembershipInputSchema`, `MembershipUpdateSchema` and `MembershipDeactivateSchema` guarded their regex with bare truthiness (`if (data.x && !/…/.test(x))`), which is exactly what docs/principles.md §7 warns about: `""` is falsy, so the empty string skipped the check entirely and went straight to the column. `EncounterListQuerySchema` `from_date`/`to_date` and `friends.ts` `date_value`/`met_date` were typed as plain `string` with no validation at all, so `?from_date=last-tuesday` and `date_value: '15.05.1990'` were accepted too. `EncounterInputSchema` was already safe — its narrow tests the regex unconditionally — which is why the create path never showed this. The pattern now lives once in `dates.ts` as `IsoDateString`, with `IsoDateFilter` for the query fields that also accept `""` (matching the neighbouring `friend_id?: '"" | string.uuid'` convention; `parseEncounterListQuery` already maps `""` to undefined). Four copies of the same regex are gone with it. `date_value` deliberately stays strict ISO despite birthdays with an unknown year: the column is `NOT NULL date` and an unknown year is carried by the separate `year_known` boolean, not by a partial date string. There is a test pinning that combination so the distinction is not re-litigated by accident.
The client declared `Promise<{ message: string }>`; the route answers
`c.json({ success: true })`. The only caller ignores the body, so nothing
misbehaves today — but the type is a standing invitation to read `.message`
and get `undefined`, and the test asserted nothing about the shape, so it
would not have caught the drift either.
Declared as `{ success: boolean }` and the test now asserts the envelope, so
the type is load-bearing rather than decorative.
This branch moved the list endpoints to `{ data, pagination }`, and the
production clients were updated with it, but five fixtures still resolved the
old `{ collectives: [] }` / `{ encounters: [] }` shape. Tests that mock a
response the server cannot produce assert nothing about the contract — they
would have stayed green through a genuine regression in the envelope handling.
Fixtures now return the real `Paginated<T>` shape, and one list test per file
asserts the whole envelope so the next stale fixture fails instead of being
quietly ignored.
The remaining `friends: []` in `friend-subresources.test.ts` is the store's own
`FriendsState`, not a response body, and stays as it is.
Every user-visible string in `app-password-manager.svelte` was hard-coded
English, while `profile.appPasswords.*` already existed in both locale files
with 13 of the strings translated — the keys were written and never wired up.
So a German user setting up CalDAV sync hit a fully English panel in the middle
of a translated profile page, on one of the few screens where getting it wrong
means their sync does not work.
Dates were formatted with a literal `'en-US'` locale too; they now follow
`getCurrentLanguage()` like the rest of the app.
Five strings genuinely had no key (`dismiss`, `never`, and the three
`failedTo*` error messages) and were added to both locales. Two existing German
progress labels were identical to their idle counterparts ("Erstellen..." for
both the button and the in-flight state), which conveys no state change; they
now read "Wird erstellt..." / "Wird widerrufen...", matching the passkey
manager.
The two bare-truthiness guards in the component are made explicit while the
file was open, per docs/principles.md §7.
The test renders against the real i18next bundles and asserts every string
against the locale JSON rather than against literals, so re-hard-coding one
fails rather than passing in English by accident.
The account page rendered `{#if $currentUser?.createdAt}`. `createdAt` is
optional on the shared `User` interface and nothing ever populates it:
`/api/auth/me` builds its `user` from five fields — externalId, email,
selfProfileId, displayName, hasCompletedOnboarding — and the auth store copies
exactly those. `/api/auth/me` is the only writer of `auth.user`, so the
condition has always been false and the two `memberSince` keys in both locales
were dead.
Removed rather than made to work. Better Auth's session user does carry
`createdAt`, so exposing it is a one-line backend change — but nobody has asked
for the value, and a block that silently renders nothing is worse than no block
at all: it looks like a bug in the data rather than an unfinished feature.
The account card's description promised "Email, user ID, and membership info",
which the page no longer shows and, given the above, never did.
All nine sub-resource rows on the friend detail page — phone, email, address, url, date, social profile, professional history, circle, collective — carried hard-coded English: the type and platform label maps, the "Primary" badge, the edit and delete aria-labels, "Remove from circle"/"Remove from collective". Professional history had a full 12-month English month-name table of its own. That is most of the friend detail page, so a German user sees a screen where their own data is translated and every label around it is not. The aria-labels matter beyond appearance: a screen reader in a German session announced English controls. Every type and platform label reused a key that already existed; 20 keys were genuinely new and are in both locales with real German. The month table is replaced by `Intl`, which is what the rest of the app uses for dates. `subresource-row` and `detail-actions` had `editLabel = 'Edit'` and `deleteLabel = 'Delete'` as default prop values, which is how the English survived a translated caller. Both props are required now, so a row that forgets to pass a translated label fails to compile instead of silently rendering English. The row tests assert against the locale bundles in both languages rather than against English literals, which is what makes re-hard-coding a string detectable; `useLanguage()` in `$lib/test` boots the real bundles for them.
`docker-compose.prod.yml` never passed `WEBAUTHN_RP_ID`, so a fresh self-hosted install got the config default and the backend fell back to an `rpID` of `localhost`. Every browser rejects a relying-party id that does not match the origin, so passkey registration failed on every deployment that followed the guide — and only when a user first tried to register one, long after the deploy looked successful. `docs/self-hosting.md` already listed the variable as required while the compose file it describes did not set it. No default is correct here. `localhost` is right for the dev compose and wrong everywhere else, and the maintainer's own domain would be silently wrong on somebody else's host. The variable is therefore declared with compose's `:?` form, so `docker compose up` and `config` abort with a message naming the expected shape rather than starting an instance whose passkeys cannot work. BREAKING CHANGE: `docker-compose.prod.yml` now requires `WEBAUTHN_RP_ID` to be set. An existing deployment that relied on the (broken) `localhost` fallback will refuse to start until it is added to `.env` as the bare registration domain — no scheme, no port, e.g. `freundebuch.example.com`.
`.env.example` is what the self-hosting guide tells you to copy, but it never
listed `POSTGRES_DB`, `POSTGRES_USER`, `POSTGRES_PASSWORD`, `VERSION` or the
two `POSTGIS_ADDRESS_*` switches — all of which the compose files interpolate.
Copying it therefore produced a file that renders with "variable is not set"
warnings and a database container whose credentials do not match the
`DATABASE_URL` two lines above.
Enumerated every `${VAR}` both compose files read and added the missing ones
with development values. `TRUST_PROXY` and `TRUSTED_PROXY_HOPS` stay out
deliberately: the production compose hardcodes them, so they are not read from
the environment at all, and the guide already flags them as production-only.
`NOMINATIM_CONTACT_EMAIL` was a related hole worth closing in the same pass.
`config.ts` reads it, `nominatim.client.ts` warns when it is absent and the
guide lists it as recommended, yet the production compose never passed it — so
it was unsettable in production and OSM's usage policy could not be honoured
however the operator filled in their `.env`.
The guide's claim about which variables the example omits is updated with it,
since it was describing the old file.
`aube check` is aube's own dependency-tree verifier — it prints "node_modules symlink tree is consistent (checked 1164 packages)" and exits 0. The repo's Biome script is `aube run check`. Both the pre-push `check` step and `mise run ci` used the bare form, so the step that was supposed to lint and format the whole codebase before a push verified symlinks instead and always passed. `.github/workflows/ci.yml` already uses `aube run check` and notes the trap, which is why CI kept catching what the local hook waved through — the exact inversion the pre-push hook exists to prevent. Both call sites now match CI, with a comment naming the trap so the shorter form does not look like a harmless simplification next time. Nothing else about the hook's coverage changes; `mise run ci` gains no steps beyond the corrected command.
The all-in-one image's entrypoint rendered the nginx template and went straight to `exec supervisord`, which starts the backend, the MCP server and php-fpm immediately. None of them waits for a schema, so a fresh container came up against an empty database and every request failed until somebody worked out that migrations had to be run by hand from inside the container. The five-container deployment gets this for free from the `migrate` service and `service_completed_successfully`; a single container has no such primitive, so the entrypoint has to do it. It now runs the compiled migrations with the same flags as the prod compose `migrate` service before handing over to supervisord. The script's existing `set -e` makes a migration failure exit the container rather than let the services answer against a half-migrated schema — the loud failure is the point, since a partially applied migration is worse than no container. Documented in the guide, which previously described the image without saying anything about schema management.
`docs/self-hosting.md` offers `ghcr.io/datenknoten/freundebuch-all-in-one` as an alternative deployment, with the same tags and attestations as the other five. No workflow ever built it. The image existed only as a root `Dockerfile` that nobody ran, so it was neither published nor known to compile. Added to `release.yml` rather than given its own workflow: that job already owns the registry naming, the four-tag manifest fan-out, the provenance attestation and the SBOM, all of which this image should inherit rather than reimplement. It joins the `docker-build` matrix for amd64 and arm64 and the manifest and sbom matrices. The `VITE_SENTRY_DSN` build-arg condition widens to cover it, since it builds the static frontend too. It also joins the PR-only `docker-smoke` job in `ci.yml`. `release.yml` runs after semantic-release has already tagged and published, so a build failure there is a broken release rather than a failed check — and this image is the one toolchain path nothing else covers (the mise shell installer on `node:24-bookworm-slim`, needed for php8.2), so it is exactly the one worth building on a PR.
Three sync behaviours are deliberate, non-obvious, and will otherwise reach the maintainer as bug reports: an edit from a client updates the current position but never the employment history or its dates, an inline photo from an Apple client is ignored rather than stored (so a photo cannot be removed over CardDAV, only in the app), and a PUT to an archived friend answers 403 once before the client drops the card. Each is the correct behaviour — a vCard is narrower than the data model, and treating "this client cannot express it" as "the user deleted it" would let any client destroy data it never saw. But none of it is guessable from the outside, and "my employment history did not sync" looks exactly like a bug until you know the rule.
`aube ci` in the `deps` stage installs the full dependency tree, devDependencies included, and `testcontainers` pulls `ssh2` -> `cpu-features`, whose install script compiles a probe to detect the local compiler. `node:24-bookworm-slim` has no compiler, so the install died with "Unable to detect compiler type" and the whole image failed to build. Nothing noticed because no workflow had ever built this image. Adding it to `docker-smoke` in the preceding commits is what surfaced it, on the first CI run that tried. `docker/Dockerfile.backend.prod` already installs `python3 make g++` for exactly these packages, with a comment naming them; this mirrors it. The toolchain stays in the builder stage — the production stage runs `aube install --prod`, which prunes `testcontainers` away, so the runtime image gains no compiler.
By default aube symlinks `node_modules` into its shared virtual store under `~/.cache/aube`. The production stage installs as root, so the tree pointed at `/root/.cache/aube/virtual-store/…` — and `/root` is 0700. supervisord runs backend and mcp-server as `node`, which cannot traverse it, so both processes died on their first import with `ERR_MODULE_NOT_FOUND: Cannot find package '@hono/node-server'` and supervisord gave up on them. nginx then answered every request with 502. The symlinks themselves were intact, which is what made this confusing: the tree resolves perfectly as root and only fails for the user that actually runs the app. `--disable-global-virtual-store` materialises the packages inside `/app/node_modules/.aube` instead, which the `chown -R node:node /app` at the end of the stage hands to `node` along with everything else. The store cache mount is unaffected — it feeds the install, it is not what the result points at.
The supervisord program set `user=www-data`, so the FPM *master* ran unprivileged. A master that is not root cannot open its log or bind its socket, and it refused to start: "failed to open error_log (/var/log/php8.2-fpm.log): Permission denied" -> "FPM initialization failed", three restarts, FATAL. Every `/carddav` and `/caldav` request in the all-in-one image was a 502 for the life of the container. The `user=` was also unnecessary. `pool.d/www.conf` already carries `user = www-data` / `group = www-data`, which is how FPM is meant to drop privileges — the master stays root, each worker does not. Removing the directive restores the standard arrangement rather than loosening anything: the processes that execute PHP still run as www-data. Pointing the master log at `/proc/self/fd/2` goes with it, matching the intent already stated in `php-fpm-pool.conf` (which sends worker errors to stderr) and supervisord's own stderr forwarding for this program. Opening that path needs root as well, so on its own it moved the failure rather than fixing it.
`Backend Coverage` failed on `should handle concurrent sign-in attempts` with `expected 500 to be 200`, while the uninstrumented `Unit and Integration Tests` job ran the same file in the same CI run and passed. The difference is v8 coverage instrumentation, which stretches a hashing-bound sign-in roughly 15x: 1.1s plain, 17.8s instrumented. The 500 came from the pg pool, not the app. `TEST_POOL_MAX` is deliberately 4 to keep `workers x pools` under the server's `max_connections`, which leaves the Better Auth pool two clients, so five concurrent sign-ins queue by design. Queuing is the intended behaviour; the 5s `connectionTimeoutMillis` production default then fired mid-queue and surfaced as a request error. Raised for integration suites only, next to the pool sizes it belongs with — production keeps its 5s, where a connect that slow is a real fault. That alone left the test passing at 29.8s against a 30s `testTimeout`, i.e. trading an HTTP 500 for a timeout on the next slow runner. `testTimeout` is matched to the existing `hookTimeout` of 60s for the same reason the 5s/10s defaults were raised to begin with, recorded in the comment there. Verified by reproducing both states locally with `--coverage`: fails at 18.8s without the change, and the full suite is 593 passed with it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
An architecture review of
apps/backend+database/migrations(plus the other two DB consumers,apps/sabredavandapps/mcp-server) found one structural fault and a lot of ordinary debt around it.The structural fault: the Better Auth migration was never finished. Two user tables coexisted — legacy
auth.users(SERIAL id, UUIDexternal_id) and Better Authauth."user"(TEXT id) — joined by email on every single request. Email was the de-facto foreign key across TypeScript, PHP and the MCP server. Anything that changed an address, or differed in case, silently pointed a request at the wrong row or none at all.Everything else followed from the same pattern: an invariant that lived in three places instead of one.
What changed
58 commits in five phases. Each commit is self-contained and type-checks on its own — verified by replaying the branch commit by commit and running
aube --recursive run type-checkat each one (58/58 clean).Phase 0 — Stop the bleeding
PUT /api/users/melet a client changeauth.users.emailalone, desynchronising it fromauth."user".emailand bricking the account with a permanent 401. Email is read-only until a verified change-email flow exists. Breaking.auth.users.preferences, which no write path ever touches — every digest was English regardless of the user's setting.CHECKon both identity tables, plus lowercased lookups in the PHP and MCP paths.TRUST_PROXY: rate limiters keyed on the proxy address collapsed the whole instance into one bucket. The address-lookup endpoint (outbound Nominatim/Overpass traffic) had no limiter at all.Phase 1 — Identity cutover
auth."user".idnow always equalsauth.users.external_id::text. The email bridge is gone;auth.usersis a pure identity anchor holding no editable data. Better Authcreate/deletehooks keep the anchor allocated and reaped. See ADR 0003.Phase 2 — Schema contract
Tenancy scoping on collective role/rule queries (a user id was missing from the predicate), DB-enforced "one primary per owner" partial unique indexes, consistent archived/soft-deleted predicates in CardDAV, a migration round-trip CI job (up → down × 43 → up), a compose
migrateinit service so self-hosters stop applying migrations by hand, and index housekeeping.Phase 3 — Route consolidation
Eleven near-identical sub-resource route files became one
createSubResourceRouterfactory; every sub-resource service gained thelistoperation six of them were missing. Malformed JSON returned 500 —await c.req.json()was uncaught in most routes — now 400. Postgres unique-violation translation lives in one place instead of three. Options-object constructors across all nine services (85 call sites).Breaking: four paginated endpoints returned three different envelopes (
{ friends, total, page },{ results, total },{ collectives }, bare arrays). All now return{ data, pagination }. Backend, shared types and frontend move in one commit — there is no intermediate state where the API and its only client disagree.Phase 4 — Search
Full-text search used the
englishdictionary on DACH-focused data:"Bücher"did not match"Buch". Nowgerman, with a trigram index on the formatted phone number replacing one the old query could not use at all.ts_headlinemoved afterLIMIT— it was formatting thousands of snippets to throw all but 25 away.DISTINCT ONtiebreak made deterministic.Phase 5 — Ops and security
requireEmailVerificationdeliberately stays off (it would lock out every existing account). AddsDISABLE_SIGNUP.password_prefixstored a third of the secret in plaintext. Constant-time rejection and a hashed prefix, agreeing across Node, PHP and Postgres.If-None-Matchwith 304 (they were read fully into memory and always 200)./healthsplit into liveness (no DB) and/health/ready; a DB blip no longer restarts a healthy container.Verification
@freundebuch/backend@freundebuch/frontend@freundebuch/mcp-server@freundebuch/sharedapps/sabredav(phpunit)aube run type-checkhealth.test.tsand the PostGIS suites could not run at all before this branch — they started their own testcontainer instead of using the shared migrated one. All suites now clone the shared template, which letfileParallelismbe turned on; stable across consecutive runs.Enabling parallelism exposed a latent bug in the clone helper: it opened its admin connection on the template database, and
CREATE DATABASE … TEMPLATErequires no other session on the template, so concurrent workers deadlocked each other. Fixed.Breaking changes
PUT /api/users/meno longer acceptsemail./api/friends,/api/friends/search*,/api/collectivesand/api/encountersreturn{ data, pagination }. Third-party clients must update; the frontend ships in the same release.auth.app_passwords.password_prefixstores a hash. An operator who downgrades the schema must also downgrade the application; in that combination affected users must recreate their app passwords (documented in the migration).Notes for review
pnpm-lock.yamlentries fornodemailerwere written by hand.aube add nodemailerfails in this workspace: its resolver aborts on an unrelated packument (@tailwindcss/oxide-wasm32-wasi: duplicate field bundledDependencies) and never writes the lockfile. nodemailer 10 has zero dependencies, so the closure is exactly itself;aube ciresolves and installs it cleanly. Worth an independent look, and the underlyingaubebug is worth reporting upstream.Two files owned by a concurrently-running change (
app-passwords.service.ts,notification-channels.service.ts) were swept into the malformed-JSON commit rather than their own; content is correct, attribution is off by one commit.The same crossing of parallel work initially left 7 mid-branch commits that did not type-check standalone —
mailer.tslanded four commits before theSMTP_SECURE/SMTP_FROMconfig keys it reads,scheduler.tsimportedtrimAddressCacheone commit before that query existed, and three integration suites importedtruncateUserDatabefore the test-infra commit exported it. Those commits were rebuilt so each piece arrives with the commit introducing its dependency. The final tree is byte-identical to before the repair (git diffbetween the two heads is empty) and the suites produce identical results.Rate limiters stay in-memory (single instance, per ADR 0004). A second replica would need
RateLimiterPostgres.Archived friends remain excluded from CardDAV — the existing predicate is taken as intended behaviour.
Two CI checks I added are report-only (
continue-on-error), and the reason is recorded next to each:osv-scannerandcomposer auditboth exit non-zero on any advisory, and the tree already carries a backlog (svelte, undici, vite, vitest, uuid, xml2js; guzzlehttp/psr7, phpunit, symfony/process). Those are fixed by dependency bumps, which Renovate owns — gating on them would red-light every unrelated PR. Worth making blocking once the backlog is clear.Danger's kebab-case rule now skips
.php: PSR-4 resolves a class to a file of the same name, so renamingSapiMock.phpwould have broken autoloading.Coverage — the Danger notice was partly my own measurement artifact. The backend coverage job excluded the integration suites (justified when every integration file started its own container against vitest's 5s/10s defaults, which v8 instrumentation blew through), but the test-infra commit removed both causes. Including them was the honest fix and moved reported numbers with no test changes:
utils/http.ts0 -> 88.9%,utils/type-guards.ts54.2 -> 87.5%,middleware/auth.tsfuncs 66.7 -> 100%.Where the notice was pointing at a real gap in this branch's new code, that is now tested, and each test was mutation-checked:
services/mailer.tssecureflag fails rather than passing a mockgetClientIdentifierhops[0]instead of last hop; honouringX-Forwarded-ForwithTRUST_PROXYoffutils/cache.tsmaxbreaks the two bounds testsutils/errors.tsutils/scheduler.tsTwo of those replace verification I had only done by hand: the mailer SMTP sink and the digest claim probe are now tests rather than throwaway scripts.
The numbers still under 80% are pre-existing untested surface that this branch only touched — the sub-resource routes and services rewritten by the factory cutover,
dashboard.routes.ts,date.service.ts,notification-dispatcher.ts. Backfilling those is a separate piece of work, not something to smuggle into an already large PR; theindex.tsfigures the notice quotes (65.22/68.75/42.86) are the app bootstrap.The docker-published-port path is broken on the dev machine used here (running kernel has no module tree), so the dev DB ran on the host network. This affects nothing in CI.