Repository navigation
The last four rooms findings: a channel switch that was ignored, and a clash that named other people's shows - #95
Merged
Merged
Conversation
GET /api/bookings, /api/rooms, /api/venues and /api/users all return
{ items, total, limit, offset }, but their OpenAPI blocks still declared a
bare array. Someone generating a client from /_scalar got a type saying
Booking[], iterated the response, found nothing to iterate and rendered an
empty list with no error explaining it.
The same blocks omitted limit and offset from their query parameters
although every one of the four accepts them, so the request was
misdescribed as well as the response. Both are now declared, with the
bounds validation.ts enforces.
notifyAdmins filtered on the ADMIN_NEW_BOOKINGS preference and then bcc'd everyone it had left, without ever reading notification_channels. An admin who turned the Email switch off on the settings page stopped receiving their own booking mail, because notifyBookingUpdate does read the column, and carried on receiving every new booking request and every member cancellation. One column governed one class of mail and was ignored for the other. It now filters on both. Push delivers nothing here, so an admin with email off receives no fan-out at all; the decision record explains why the switch is authoritative rather than advisory, and the fan-out logs a warning when it leaves nobody to tell, so a request nobody saw shows up in the log instead of vanishing. The settings page said the Email switch covered booking updates. It covers the admin fan-out too, and now says so.
GET /api/rooms/available checked only that endTime came after startTime, then read every overlapping booking joined to its user row with no limit. Asking for the year 2000 to the year 2100 pulled the whole bookings table into one Worker isolate, and any signed-in member could ask. D1 bills by rows read, so this was paid for on every call, and it grows into the booking form losing the check it depends on. Two bounds now. The schema refuses a window spanning more than 31 days, which no booking form can produce because both ends come from one event date. The sweep asks for at most 1001 clashing rows and refuses with a 400 over 1000, rather than answering from a truncated set: the split between available and unavailable rooms is derived from those rows, so a dropped clash would offer an occupied room to the next member. The limit is passed only by the sweep, so the occupancy gate on the write path still sees every clash.
The 409 a clashing write returns listed each conflicting booking with its real event title, whoever asked. GET /api/rooms/available has masked that field as "Booked" for anyone without booking.read.any since the permission split landed, and the same reasoning applies here: the owner of a pending request that already holds a room reaches the same payload by nudging their own times, and could step a window through an evening to learn which production has the room and when. The payload is now allow-listed on the caller, not stripped: an admin still gets the titles that make the error worth reading, and everyone else gets "Booked" with the id, window and status they need to pick another slot. The option defaults to masked, so a route added later has to ask for the titles rather than leak them by omission.
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.
The last four review findings in this repo, one commit each. Gates: lint, typecheck, comments and build, all verified independently of the change author. No test suite here, so each path was exercised by hand against a local dev server on :3002 with both personas.
An admin who turned email off still received the fan-out.
notifyAdminsfiltered on theADMIN_NEW_BOOKINGStype preference and then bcc'd everyone it had left, never readingnotification_channels. So one column governed booking-update mail, whichnotifyBookingUpdatedoes check, and was ignored for new-request mail. It filters on both now.The consequence is worth a committee view, and is why ADR-0010 exists. Push is recorded but never delivered in this repo, so an admin with email off now receives nothing at all. That is the honest reading of a switch headed "Where can we notify you?", and
ADMIN_NEW_BOOKINGSremains the narrower opt-out for people who want other mail but not this. It does mean a committee can silence itself: if every admin turns email off, a request sits PENDING with only a log line. The/admin/bookingsqueue still shows it, so this is nobody being nudged rather than nobody being able to see it. The fan-out logs a warning when it resolves recipients but none take email. If the committee would rather this were operational mail that ignores the channel column, supersede ADR-0010 and the filter comes back out.A booking clash named other members' productions. The 409 payload carried
eventTitlefor every conflicting booking, to any caller. It is allow-listed on the caller rather than stripped:conflictPayloademitseventTitle: 'Booked'unless the caller holdsbooking.read.any, which is whatavailable.get.tsalready did. The default is masked, so a route added later has to ask for titles rather than remember to remove them. No entry ever carried theuserobject, and the client types conflicts as{ id: number }[], so no screen loses information.The availability window was uncapped, so one request could load every booking in the table, on a query D1 bills by rows read. Two server-side bounds: the schema refuses a span over 31 days, and the sweep asks for 1001 rows and returns 400 if it gets more than 1000. It refuses rather than truncates, deliberately: the available/unavailable split is derived from those rows, so a dropped clash would offer an occupied room to the next member. The limit is an argument only the sweep passes, so the write gate and both availability checks still read every clash and the 409 stays complete.
The OpenAPI blocks declared a bare array for four list endpoints that have always returned
{ items, total, limit, offset }, and none declared thelimitandoffsetparameterspageQueryaccepts. Both corrected, with the bounds the schema enforces.Two numbers are policy, not derived: 31 days and 1000 rows. They appear in the Zod schema, the route's OpenAPI block (which cannot read a constant, since
defineRouteMetais extracted statically), the API reference and ADR-0011. Change them together.Closes #86, #88, #89, #90.