Repository navigation
Six high-severity findings: erasure that undid itself, and a bulk delete that took whole series - #92
Merged
Merged
Conversation
An erasure of someone who had never used rooms wrote nothing, because there was no mirror row to scrub, and the merge hook deleted the losing row outright. Sessions here are sealed cookies with no revocation, so in both cases the person's next page load, or one request from a tab left open, ran ensureLocalUser against an id with no row: the insert half of the upsert is unguarded, so their real name and email went straight back into this database. Nothing would ever scrub it again. The auth service records the hook as done and does not resend it, and after a merge it disables the losing account, so every booking made on the stale cookie attached to an id the person can never sign in as again. Both hooks now write the same scrubbed row with anonymised_at stamped, as an upsert, so it does not matter whether a row was there. tombstoneMirroredUser in server/utils/mirrorUser.ts is the one implementation, which keeps that file the only writer of the mirror, and it is a single statement so it sits inside the batch each hook already had. The merge scrubs the loser in place rather than deleting it, which is what a merged-away id is: the auth service erases it moments later. ADR-0006 records the reasoning and supersedes ADR-0005, whose "an erased row exists and therefore always takes the conflict branch" is the assumption that let this through.
Three paths in this app write an address nobody can receive: erasure, a merge, and the collision placeholder an email clash falls back to. sendEmail handed whatever the mirror row held straight to Resend, which rejects it, and the route logs the failure and carries on. The live case was a merge: the hook re-points the loser's bookings onto a winner who has never used rooms, and until that person makes their next request every admin decision on those bookings was emailed to merged-<id>@placeholder.invalid. Nobody was told their room was confirmed or rejected, nobody saw a failure, and each attempt counted a hard bounce against the domain the rest of our mail goes out on. sendEmail and sendBatchEmail now check the recipient first and drop the message with a warning naming the subject and the id, rather than trying and failing. The merge hook also mints the winner with the loser's address rather than a synthetic one, since it is the same person's, so booking mail reaches them until they next sign in and ensureLocalUser refreshes the row.
Both update schemas compare the two ends only when a body carries both, and the write path resolved the missing half from the row without comparing them. So PUT /api/bookings/42 with nothing but a startTime could move a booking that ran 18:00 to 20:00 so that it ended at 20:00 and started at 21:00. The occupancy check ran on that inverted span and passed, because no real booking straddles it, and afterwards the row occupied nothing at all: the room read as free, a second group was confirmed into it, and the first group still had a confirmation saying the slot was theirs. Any admin who later confirmed the corrupted booking sent an email reading "at 10:00 pm - 8:00 pm". planBookingChange now compares the resolved window whenever the patch supplies either end, and returns 400. It is the single write path, so this covers the single, series and bulk routes at once. The check is gated on the patch touching a time so that a booking already holding an inverted window can still be cancelled or rejected. data-model.md carries the query that finds any such rows, since they are invisible to the occupancy check and have to be corrected by hand.
…ident Ticking the first occurrence of a twelve-week series in /admin/bookings along with two unrelated bookings deleted fourteen rows. The bulk route deleted the id list straight out, and parent_booking_id cascades, so the eleven later occurrences went with the head. The admin was told "3 booking(s) have been deleted" and the owner's email named only what had been ticked: eleven confirmed rehearsals left their account with no notification and the rooms were never re-offered. ADR-0003 fixed exactly this on the single-row route and the bulk one was left behind. The route now takes ?scope= like the single route, defaulting to occurrence. Under that default the ids are deleted one at a time, each promoting its successor immediately before its own delete, which a pre-pass over the whole list cannot do: promoting a head hands the series to its successor, and a later delete of that successor would cascade into everything the promotion saved. scope=series resolves each id to its series parent, deduplicates and deletes the heads, letting the cascade work as intended. The count and the notifications are built from the rows that are actually going, and the admin page prints the server's number instead of counting the ticked rows itself. RETURNING could not have supplied it: SQLite reports the rows a statement deleted, never the rows the cascade took. ADR-0007 records this and extends ADR-0003, which read as though the class of bug was closed.
The recurring-action modal filtered the series down to the occurrences it thought each action applied to, listed them, and labelled the button with their count. It then threw that list away: the page only checked whether it was empty to decide between ?scope=occurrence and ?scope=series, and the server resolves the series itself and applies the change to every occurrence that is not rejected or cancelled. So rejecting one pending occurrence of a series with "apply to series" ticked, on a preview naming two bookings, also rejected the four occurrences already confirmed, and the member was told about one of them. The modal now previews the same set the server writes, from one predicate in shared/utils/bookingStatus.ts that both sides use, and says which occurrences that is: every one still open, or for a deletion every one whatever its status. Note that the reported example, one AWAITING_EXTERNAL occurrence among five confirmed ones, did not reach the server as a series write: the old filter left the list empty, which the page read as ?scope=occurrence. It takes a non-empty subset for the two to disagree. The other half of that report stands and is not fixed here: a series-wide update still sends one notification, for the occurrence in the URL. Recorded in README §Known gaps.
The tombstone stops a merge or an erasure being undone from here on, but it does not clean up after the ones that already ran: a merge that deleted the losing mirror row could have had it re-created by the loser's own browser, with their real name and email, under an id the auth service has erased and will never send us a hook for again. Those rows are still here and nothing scrubs them. data-model.md now carries a query that narrows the search, and says plainly that it is a superset and that the auth service's audit log is what settles which ids were merged away. Completes the tombstone change.
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.
Six verified high-severity findings, in six commits. Closes #66. Closes #67. Closes #68. Closes #70. Closes #73. Closes #74.
Erasure could be undone (#67, #68, one cause). An erased id with no mirror row had nothing to carry
anonymised_at, so the guard onensureLocalUserhad nothing to protect and the person's next page load wrote them straight back. The merge hook made it worse by deleting the losing row, which the loser's still-valid cookie then recreated. Both now write a tombstone inside their existing batch. ADR-0006 supersedes ADR-0005, whose "an erased row exists and therefore always takes the conflict branch" is precisely the assumption that failed.Mail went to anonymised addresses (#74).
isDeliverablenow drops mail to a row carryinganonymised_ator an address under.invalid, in both send paths, and the merge hook mints the winner with the loser's real address rather than a synthetic one.A bulk delete destroyed whole series (#70).
DELETE /api/bookings/bulkcascaded through recurring series. It takes?scope=like the single route now, defaulting to occurrence. A pre-pass cannot work here, which is worth recording: promoting a head hands the series to a successor that a later chunked delete would then cascade through, so promotion happens immediately before each delete.A partial time update could invert a booking window (#73) and silently free the room.
planBookingChangecompares the resolved window whenever either end is supplied, so the single, series and bulk routes are covered by one check.The recurring-action modal promised a filtered subset (#66) while the server applied the change to every open occurrence. Both sides now read one predicate. The agent corrected the finding's own example: the case as reported never reached the server as a series write, and the real divergence is a non-empty strict subset, such as rejecting one occurrence of a part-pending series.
No test suite here, so the delete and tombstone SQL was verified against a throwaway SQLite database replicating the schema, including reproducing the series-destroying delete before the fix.
Two follow-ups recorded rather than done: a series-wide update still notifies only the occurrence in the URL, and rows earlier merges already deleted are not recovered by this change.
docs/data-model.mdcarries the query to hunt them, which needs settling against the auth service's merge audit log.