Searching worked for the admin and 403'd for every other member. getMapsKey read
the calling user's own row first and then fell through to "any admin" via LIMIT 1
with no ORDER BY, so the admin who pasted the key hit their own row while everyone
else got a non-deterministic stranger's. That fallback is what CLAUDE.md forbids.
The Places and Unsplash keys bill the install, not the account that pasted them
in, so they become instance configuration: an encrypted app_settings row, resolved
as env -> instance -> the caller's own row, the same shape #1772 chose for the LLM
endpoint. The per-user column stays as the last step because /me/api-keys has
always accepted a personal key.
The trailing migration carries an existing install's key into the instance row so
nothing degrades to OpenStreetMap on upgrade, but only where that key already was
the whole install's: as soon as a second row holds one, it is left alone, because
the instance row outranks everybody's own column and an upgrade must not move a
member onto someone else's key and bill.
Two things from the same report:
The key test button probed with field mask places.displayName while the search
asks for places.rating and more, which is a separate SKU and can be restricted on
its own, so the button could pass while every search failed. It now probes the
resolved key with the search's own mask.
Saving API keys wrote no audit line. It now writes one settings.api_keys_update
row naming which fields changed, never their values, comparing cleartext because
the random IV makes every ciphertext look different.
Closes#1939
updatePositions has two branches. The legacy one updates
`WHERE id = ? AND trip_id = ?`; the per-day one, three lines below it, bound
the caller's ids straight into reservation_day_positions and never touched the
tripId it was handed. That table has no trip_id of its own and its two foreign
keys only require the ids to exist, so nothing further down caught a pair that
did not belong together.
The trip now comes from the join, so the row materialises only when the
reservation and the day agree on it. Doing that in the statement rather than as
a pre-check also turns a stale id into a quiet no-op instead of a foreign-key
error surfacing as a 500, and lets an omitted day_plan_position bind the way
the wire contract has always said it may.
reorder_reservations reports a day that is not on the trip, the way its sibling
tools in that file already do, and a migration clears rows whose reservation
and day disagree.
Google charges autocomplete per request unless the requests carry a session
token. With one, the keystrokes of a single search and the details lookup that
ends it are charged together, which is also how a user experiences it: they
looked up one place.
TREK sent no token at all, so every debounced keystroke was its own billable
request. On a busy trip that is the largest line on the Places bill, and it is
paid by whoever owns the key, which for a self-hosted instance is the person
running it.
A session opens with the first suggestion request and closes when the user picks
a result. It has to close on the abandoned paths too: a token carried into the
next search would merge two searches into one billed session and scope the
suggestions wrongly. So the search box clearing it, the text-search fallback and
the error paths all end it.
The token is validated on the way in. The autocomplete body goes through the Zod
pipe, the details query does not, so the controller shape-checks it there.
Anything that does not match Google's URL-safe 36-character form is dropped
rather than forwarded, which falls back to per-request billing instead of
breaking a lookup.
A cached details hit never reaches Google and so never closes its session. That
is billing-neutral: an unclosed session is charged as a plain autocomplete
session, which is still cheaper than the per-request behaviour it replaces.
A packing item is Common, Personal or Shared with named recipients. listItems()
always applied that rule, but every path that touches one item resolved it by
trip id alone, so trip membership plus packing_edit was enough to reach an item
the member could not see. Update, delete, clone, the contributor routes and the
sharing route all shared the lookup, and the same service methods sit behind the
MCP tools and the plugin RPC, so all three transports carried the gap.
The update was a read as well: it answered with the enriched row, which hands
back the name, category, quantity, weight, the owner's username and the recipient
list of an item the caller was never shown.
Two flows laundered restricted items into places they do not belong. Saving a
trip as a template read every row in the trip, so a template captured the names
and categories of everyone's Personal items. Copying a trip did the same and
re-inserted without is_private/owner_id, so both fell back to the column defaults
and another member's Personal or Shared item reappeared in the copy as a Common
item. That path needs only trip_create and trip access, not even packing_edit.
There is now one visibility rule in the service and every single-item lookup goes
through it. The actor is threaded through the HTTP, MCP and plugin-RPC entry
points, and a missing actor denies rather than falling through unfiltered. An
item the actor may not see is reported as missing, not as forbidden, so the
routes cannot be used to confirm that an id exists.
Two deliberate contract changes fall out of that. The sharing route answers 404
instead of 403 when the caller cannot see the item at all; a visible item they do
not own still answers 403. And a template or a trip copy now carries the Common
list plus the actor's own items, with a restricted item staying restricted and
owned by the copier.
Reported privately by Chichi (@vickyzer027-hash).
* fix(mcp): accept per-leg times on create_transport and update_transport
metadata was pinned to Record<string, string>, so a legs array could not get
through and a multi-leg flight built over MCP lost every intermediate time. The
booking ended up with correct endpoints and no legs, which every reader then
treats as a single segment.
legs is now a real, typed parameter with its contract in @trek/shared, shaped
exactly the way the planner form and the importers write metadata.legs. The
folding checks what the readers depend on: one leg fewer than endpoints (the
AirTrail dedupe builds its signatures on that), day ids that belong to the trip,
airport codes that agree with their endpoint, and the times, where a stopover
endpoint carries the departure and the last one the arrival.
metadata keeps its type and its replace semantics. Segments smuggled through
metadata.legs as a JSON string used to be stored and then ignored by everything
that read them; they are refused now and point at the legs parameter instead.
Closes#1914
* fix(atlas): refuse a bucket list entry that is already on the list
Nothing gave a bucket entry an identity, so the same country could be added as
often as it was clicked. A wish is now identified by name, country, target date
and, where it has them, coordinates. A different target date is a different wish
and still lands, which is what the report asked for; two real places sharing a
name stay separate because their coordinates differ.
Empty string and NULL are normalised on both sides of the comparison, otherwise
the dateless case, which is the one that was reported, would slip through.
Deliberately no unique index and no migration. better-sqlite3 is synchronous and
the server is a single process, so the race an index would guard against cannot
happen here, and an index would have needed a destructive dedupe of existing
rows first.
create_bucket_list_item had no target_date parameter at all, so over MCP the
allowed "same place, other date" case was unreachable. It takes one now. The
plugin RPC reports the duplicate as bad params instead of letting it surface as
an internal error.
Closes#1898
* fix(settings): keep a free-form LLM endpoint in admin hands
Any account could name the address the server calls, and the guard in front of
it deliberately allows loopback and LAN so a self-hosted Ollama works. That is a
reasonable trade for whoever runs the instance, because the network is theirs. It
is not one for an arbitrary account, which could aim a request from inside that
network at a target only it chose and read how the target answered.
A free base URL, and the local provider that is one by another name, now come
only from an admin controlled source: the addon config, the instance-wide
defaults, or an admin's own settings. OpenAI and Anthropic stay available to
everyone, because they go to a fixed address with the user's own key.
The resolver is where this is enforced, since it is the single point both the
booking import and the plugin host pass through, and the only one that also
covers values already sitting in the database. The write path refuses as well,
so nothing is stored that would never be read.
The hint text promised something users no longer have, so it changed with it.
Closes#1772
* fix(calendar): span a date-only booking to its end date
A DATE event without DTEND is exactly one day per RFC 5545, so a multi-day
booking that carries an end date still collapsed onto its first day. It gets an
exclusive DTEND now, built the same way the stay branch already builds one. A
booking without an end date serialises byte for byte as before.
While in here: the stays query joins reservations on the accommodation, and two
bookings pointed at the same stay fanned it out into two events sharing one UID.
Duplicate UIDs in a feed are undefined behaviour and calendar clients pick one at
random.
The same "at most one booking per stay" assumption sat in the accommodations
service, where it was worse than cosmetic: accommodation_id carries no foreign
key, so deleting a stay took only the first booking and left the second pointing
at something that no longer existed. Both the sync and the delete walk all of
them now. The callers keep reading linkedReservationId; the list is additional.
Refs #1869
* fix(vacay): say that the region is required, because it is
The placeholder called the region optional while the add button stayed disabled
until one was picked. The wording predates multiple calendars per plan, which is
what introduced the gate. Only the value of the existing key changed, in all
locales, so the key set stays identical.
Two things the same screen got wrong were in the way of a clean answer:
The button was clickable while the region list was still loading, which put the
user in exactly the state the wording describes, and it is the state the report
was filed from. It waits for the list now.
Changing the country on an existing calendar kept the previous country's regions
on screen, so a German region could be picked for the Netherlands. The list is
cleared with the country, which only works in combination with the loading gate:
without it, the empty list would happen to enable the button instead.
Closes#1813
* fix(collections): let a saved place's address be corrected
The edit form offered name, category and labels, and the address only on the way
in. The cause was not the form: the update contract never declared address, so
the validation pipe stripped it and the UPDATE had no column for it. Coordinates
were pulled through for #1435 and the address was missed.
It travels the whole chain now, contract to service to both forms. No migration,
the column has always been there.
The mobile add sheet had no address input at all, showing whatever the search
result carried as read-only, so editing would have worked while creating still
would not. It has one.
The MCP tool description had drifted further than the address, never having
mentioned the coordinates either. Both are in it now.
Closes#1870
* fix(collections): keep the place search usable after the first save
The add dialog stays open on purpose so several places can be added in a row, but
after a save it gave no way back into the search. A search with no results was
silent, with no empty state and no toast, so typing another name and pressing
enter looked like nothing happened. The reporter described both halves and it is
the silent search that matches "nothing happens" most closely.
There is an empty state now, using a key that already existed in every locale and
was unused. The search row is sticky inside the dialog body, so it stays reachable
after the save instead of sitting above the scroll position.
Also fixed while in here: refreshing the collection after each add reset the
select mode, the selection and the label filter. In the very flow this dialog is
built for, the user quietly lost all three.
Closes#1921
* fix(vacay): make the phone year grid and the dock button do what they look like
Two separate faults met on this screen.
Tapping a day in the year grid did nothing unless the edit mode was already on,
and the only way into it was a floating button that looks exactly like the one
the dock draws in the same spot. A tap now opens that month in the editor, where
the cells are big enough to hit. At 390px the grid cells are around 21px, which
is too small to be mutating data with in the first place.
The dock's plus was not wired for this route, so it fell through to creating a
trip on the dashboard. Two different actions were stacked in one place, and the
wrong one was on top whenever the screen had not painted yet, which includes the
time its chunk is still loading. The dock leaves the slot to the screen now, and
holds the geometry so the tab groups stay symmetric.
Closes#1811
* fix(budget): settle in whole cents so balances stop drifting
The report is a balance that moves on its own, with nothing added, edited or
removed, on a trip where everything is in euro. Two mechanisms were behind it and
both are real.
The display currency is a fourth value, a per-user setting, so "everything is in
euro" never covered it. Each balance was converted and rounded on its own, and
the set no longer summed to zero: at 0.855 and 0.9312 it came out one cent over,
at 0.94 it came out even. That cent appears and disappears with the six hour rate
cache alone, which is exactly the symptom. The flows were rounded separately from
the balances, so the two never had to agree.
And a receipt line nobody was ticked for counted toward the total, and therefore
toward the payer's credit, while landing in no share. That difference is
permanent, and the client sent it that way.
The settlement converts once, at the edge, and does integer arithmetic after
that. Equal splits distribute the credited cents by largest remainder rather than
the entered total, so credits and debits of one expense cancel exactly even in a
foreign currency. Display conversion goes through the same largest-remainder
allocation, so the sum survives it.
With that, the 0.01 epsilon can go. It could never create a cent, but it stranded
them: a one cent balance was filtered out of the flows while staying visible in
the list, which is the "settle up offers nothing" half of the report.
Closes#1382
* fix(journey): reach the book export and the suggestion toggle on a phone
Below 1024px the hero card is dropped, and it held the only way to either action.
The floating bar that replaces it was never given them. It has them now, and the
hero is unmounted rather than hidden, so there is no second invisible copy of the
same controls in the document. The bar's own buttons moved onto the appearance
tokens while they were being touched.
The phone screen never had either action at all and now carries both.
The book itself was hardwired to English, cover and dates included, while the
keys for it had been sitting in every locale unused. It takes the page's
translator and locale, sets the document language, and prints mood and weather,
which were on screen but never in the book.
Closes#1848
* fix(journey): offer the connected photo providers in the phone entry editor
The desktop editor grew an external photo source in #1632; the phone editor that
came out of the mobile rewrite never did, so editing an entry on a phone had no
way to Immich or Synology. It mirrors the desktop mechanics, including the part
that is easy to miss: a save that creates the entry and then fails on the photos
keeps the sheet open, and without remembering what was just created the retry
creates a second entry.
The sheet no longer kicks off its own addon load. The list is loaded once on
boot and this only reacts to it; a second load races the other consumers and
overwrites the store when it lands.
Closes#1808
* fix(mobile): let the document scroll so Safari can collapse its URL bar
iOS Safari only shrinks its address bar when the document scrolls, and TREK's
never did: the root was locked and the mobile shell put a scroll container inside
it. On the bottom tab layout that costs about a sixth of the viewport,
permanently.
Below 768px the document is the scroller again and the full-bleed screens measure
their own height instead of leaning on the shell.
That makes the body scroll locks real for the first time on a phone, and they
were not ready for it. Three of them reset the overflow unconditionally, one of
those on every run of its effect rather than on unmount, so a system notice could
hand the page back while a sheet was still open. They all go through one
reference counted lock now, which is the part that had to land first.
Worth knowing for the issue: this reaches the flow screens. The trip planner, the
journal, the atlas and the vacay screen keep their own full-height scrollers and
still will not collapse the bar.
Closes#1809
* fix(mobile): keep the screen gradient visible behind the translucent cards
The gradient layer introduced with the scroll change sat at z-index -10. A
negative layer paints above the canvas but below the backgrounds of ordinary
blocks, and body carries an opaque one, so the gradient was hidden behind it.
The mobile cards are translucent and take their depth from what is behind them,
so every surface ended up compositing against a single flat colour and the whole
phone UI read as one tone.
The layer is positioned at z-index 0 now and the content above it at 10, which
puts both over body's background. Same visual as before the scroll change, and
the shell still owns no scroll container.
* test(reservations): teach the accommodation delete stub about the id lists
The RPC walks every linked booking since the delete stopped stopping at the
first one. The stub still answered with the two singular fields, so the loop had
nothing to iterate and the cascade events never fired.
* fix(settings): move the free-form LLM endpoint out of user settings entirely
Hiding it from non-admins closed the hole, but it left the option sitting in the
admin's own personal settings, which is not what was asked for and not where it
belongs: an instance has exactly one address it calls out to, and there was a
second, invisible place to set it.
The provider list is now the two hosted ones for everybody, the endpoint field is
gone, and the resolver takes the address from the admin-controlled instance
defaults for every caller including an admin. It no longer reads the users table
at all. The write path refuses the keys for everyone, so nothing can be stored
that would never be read.
The endpoint is configured on the addon in the admin settings, which already
carries provider, model, base URL and key, plus the Ollama model list. Nothing
is lost, it just has one home now.
Behaviour change worth a release note: an admin who set a personal Ollama
endpoint has to move it to the addon config. A non-admin was already in that
position after the previous commit.
Refs #1772
* fix(journey): make the book preview usable on a phone
The preview document carried no viewport, so a phone laid it out at its default
980px while the page is A4 landscape and about 1123px wide. It spilled out of
the frame, which reads as being zoomed far in. Declaring the real width lets the
browser scale the page to fit. Printing is unaffected, it goes by @page.
The preview chrome had the same problem in miniature: a title and two labels on
one row, sized inline, which does not fit a phone and fits translated labels
even less. It moved to a stylesheet, and below 640px the card goes full-bleed,
the decorative title steps aside and the two actions share the width.
Refs #1848
* fix(mobile): stop the shell trapping the screens' own overlays
Lifting the background layer into the positioned range meant the content wrapper
had to be positioned too, and a positioned wrapper with a z-index is a stacking
context. Every fixed overlay a screen renders inside it got clamped to that
level: the vacay view/edit FAB sits at z-50 precisely so it beats the dock at
z-40, and it disappeared behind it.
The layer goes back to a negative z-index and the wrapper back to no positioning
at all. What it needed in the first place was for body to stop painting over it,
which is one line in the phone media block rather than a new stacking context.
* fix(mobile): let the phone background override actually win
A media query adds no specificity, so the themed body rule further down the file
kept winning on source order and painted over the shell's background layer. The
override is on html body now.
The gradient was back to being invisible, which is the flat single-tone phone UI
again.
* fix(vacay): draw today's ring around the day instead of replacing it
On the phone, today short-circuited the day-cell matrix and returned a ring on an
empty cell. A vacation day or a company holiday logged for today was stored and
counted in the totals, but the cell showed nothing, so it read as if the tap had
not taken.
Today is an annotation now, layered like the school band and the shared-calendar
rings, which is how the desktop month card has always treated it. The strong ink
stays only where the cell has no fill of its own, since --m-ink is light in dark
mode and would disappear on a pastel.
* fix(map): stop serving a stale GL style document from the service worker
The Workbox rule for Mapbox matched every URL under api.mapbox.com, and the
style document lives there too. Under StaleWhileRevalidate the map was built
from the previous revision on every load, with the refresh landing one load too
late, so a style republished in Studio with different label languages kept
rendering the old ones. The network tab showed a 200 throughout, because the
service worker's own answer is a real 200, which is why this looked like the app
rewriting text-field at runtime.
The style now has its own rule ahead of the tile rules, on NetworkFirst so it is
current whenever the network is, and still cached so the map opens offline. Its
own small cache too: sharing an LRU with three thousand tiles made the staleness
come and go at random. OpenFreeMap styles had the same problem and are covered
by the same rule.
Closes#1924
* feat(places): link an expense to a place, the way bookings already do
Recording what a museum ticket or a guided tour cost meant leaving the place,
opening the Costs tab and typing it in again — while a booking has had a Costs
block in its own form all along.
The place form now carries that block. Same component, same flow: with nothing
linked it offers Create expense, which saves the place and then opens the Costs
editor for a new expense pointing at it; once linked it shows the expense with
edit and remove. budget_items gains place_id beside the reservation_id it
already had, with the same nullable FK and the same ON DELETE SET NULL. Desktop
and phone both, and the mobile editor carries the link through its own save path
rather than the desktop one.
The expense belongs to the place, not to a day assignment, so a place sitting on
five days is not five tickets. Hanging it off the assignment would have meant
adding a place to three days quietly creating three expenses and removing a day
quietly deleting one.
Deleting a place takes its expense with it in the same transaction and announces
it, on every path that deletes a place: REST, both MCP tools and the plugin RPC.
Closes#1298
* test(places): teach the RPC stubs about linkedExpenseIds
The delete paths read the linked expense before the place is gone, so the two
hand-built PlacesService stubs threw on the new method instead of reaching the
hook, the remove and the broadcast — four failures out of one missing line.
* fix(places): match imports on the provider id so renamed places are not duplicated
Re-importing a Google Maps list duplicated every place the user had renamed.
buildDedupSet only ever collected names and, for unnamed places, coordinates,
and isPlaceDuplicate returns on the name before it reaches the coordinate
fallback, so a renamed place was findable by neither. The ids were stored all
along, and even backfilled onto matched places, they were simply never read.
Match on google_place_id / google_ftid / osm_id first, keeping name and
coordinates in their old roles below it. Widening the coordinate check instead
would have merged the restaurant and the bar in the same building.
The collection copy carried a hand-mirrored port of the same predicate; it now
uses the shared helpers and gets the fix with them.
Closes#1550
* feat(calendar): put accommodations into the subscribed feed as all-day stays
A hotel keeps its dates on the linked stay, not on the reservation: the booking
form writes reservation_time = NULL for type 'hotel', and day_accommodations
carries start day, end day and the check-in/out clock. The feed only ever read
the reservation, so a stay showed up once on the arrival day at best, and not at
all when it came from the booking form.
Join the stay in and emit the reservation as an all-day range over every night,
which also keeps it correct when days are reordered later — the dates now come
from the day rows rather than from the copy stamped at creation time. Check-in
and check-out become their own timed events when the stay records a clock,
because an all-day event cannot carry the time you actually need reminding of.
The check-in window, when set, becomes that event's DTEND.
Closes#1586
* fix(oauth): treat a concurrent refresh as a race, not as a stolen token
Refresh rotation is a race by construction. Several MCP clients share one
refresh token, both post it within the same second, and the loser was read as a
replay: chain revoked, MCP sessions torn down, browser window asking to log in
again. That is the daily re-authentication people were seeing, not a short
lifetime — access tokens last an hour and refresh tokens roll for thirty days.
A revoked refresh token now only counts as a replay once it is outside a
30-second grace window, and only when the rotation that revoked it left a
successor still alive. Inside the window the caller gets its own pair branched
off the same parent, so both clients walk away with a working token. Explicit
revocation leaves no live successor and a chain killed by a real replay has none
either, so neither can slip through the window.
Closes#1007
* feat(collections): mark a place visited from the trip it belongs to
A place saved in several lists had to be opened in each of them to say you had
been there, which is the wrong way round: you find out you have been somewhere
while looking at the trip, not while browsing the library.
The save picker now shows each list's own status and cycles it in place, with one
action for "visited, wherever this is saved". The planner's selection bar and the
mobile bulk toolbar carry the same thing for a whole selection — select all, one
tap, done. Matching happens on the server with the signals findMembership already
uses plus the source link a place saved from this trip carries, so a copy renamed
inside a list is still found. Lists you may only read are skipped rather than
refused; a shared list you cannot edit should not stop you marking your own.
Closes#1469
* feat(journey): jump to the top or back to the last entry in a long journal
Everything that adds to a journal sits at the top — the entry button, the
gallery upload — and where you left off sits at the bottom, so adding one photo
to a long journey meant scrolling the whole way twice.
Two buttons float over the middle of the feed, clear of the Add Entry column on
one side and the reorder arrows on the other, each appearing only when there is
more than a screenful to travel in that direction. They live in a zero-height
sticky box so they take no layout space and never push the timeline around.
While in the header, the status pill also picks up the height of the round
buttons beside it.
Closes#1088
* fix(packing): give the item name the row back on phones
An item name had roughly a third of the row to itself: a spelled-out sharing
sentence, an empty weight column and two round action buttons took the rest, so
anything longer than a word was cut off.
Sharing is now the icon it always was next to a label rather than a sentence in
the layout, an unset weight prints nothing instead of a dash, and edit/delete
sit behind one overflow button that opens the same inline menu the bag picker
already uses. The to-do list needed none of this — it wraps its chips onto a
second line already.
Closes#1525
* docs(wiki): cover the six community changes
- Calendar feeds: accommodations as an all-day range plus check-in/check-out
- Collections: setting the visited status from a trip, and how places are matched
- Journey: the jump buttons on a long journal
- Packing: what the phone row keeps and what moved behind the overflow button
- Places: re-importing a list no longer duplicates renamed places
- MCP setup: the rotation race no longer ends an OAuth session
* test(collections): assert the stored status instead of a private method
tests/ sits outside the build tsconfig, so reaching into getPlaceById only
showed up in the dedicated typecheck step. Reading the row is the better
assertion anyway — it pins what was persisted, not what a getter returns.
With block_weekends on, toggling a weekend day via MCP or the plugin RPC
still added the entry and burned allowance — the guard lived only in the
web UI (mcp-smoke-test I-02). toggleEntry now rejects inserts and
in-place conversions on blocked days with { error: 'weekend_blocked' }
(the votePoll pattern); removing an existing entry stays allowed so
stray weekend entries remain cleanable. The controller maps the code to
a 400, the MCP tool to an isError result.
Both advisories are the same mistake: a route that hands out a bearer token
treated trip membership as authorisation. Membership lets someone read the trip
while they are signed in. It does not let them mint a URL that reads it without
an account, keeps working after they are removed, and can be rotated out from
under the owner's calendar subscriptions.
The calendar feed token now sits behind TripAccessGuard plus
@RequirePermission('share_manage'), the same shape the rest of the trip-scoped
controllers use, which also replaces a hand-written access check that GET never
called at all. GET is gated with the writes because it returns the credential
itself, not its status. The share link's GET does what its own POST and DELETE
already did, and what the invite link next door already does for exactly this
reason.
Two things had to come with it or the gate would have been decoration. The MCP
tool get_share_link read the same token on membership alone, so it would simply
have become the new way in. And feed_token rides along in every trip payload
because TRIP_SELECT reads t.*: any member could have read the value out of
GET /api/trips/:id, the summary tool or the plugin RPC. It is blanked once in
the select and stripped in the two places that select the row directly.
The three service writes take the acting user and scope their UPDATE to a trip
that user can reach. The guard decides who may manage the credential; this only
makes sure a caller arriving another way cannot write to an id it guessed.
Client side, "Subscribe to calendar" is now behind the same permission on
desktop and mobile. Without that a member would have kept an entry whose enable
button silently does nothing, because IcsSubscribeModal swallows every non-200.
The one-off ICS download stays where it is: that is a file they may already read.
Instances that want members to manage these links can lower share_manage to
trip_member in the admin panel, which the tests cover.
Both were left open by the audit and are closed here.
days, packing, photos and reservations each built a service outside the
container at module load for nobody: only their own delegation tests imported
them, and three domain docblocks were still telling readers to go through them.
photos.bridge's own comment pointed at src/services/, a directory deleted two
PRs ago. The cases that exercised them kept their assertions and call the
service the bridge delegated to, so nothing that was proved stopped being
proved. Twelve bridges remain, each with a live out-of-container caller.
Coverage measured src/** but gated only src/nest/**, so the boot path, the
scheduler, the MCP handler and the migrations sat under no floor at all. That
is the same 9,602 lines where this audit found its worst defect, and a
swallowed EADDRINUSE is exactly the kind of thing an untested boot path hides.
Seven globs now pin what is measured today. They are floors to raise, not
targets that have been met: src/*.ts is at 39% statements and src/db at 38%
branches, and writing those numbers down is the point.
src/demo/** is deliberately left out. It measures 0%, and a floor of zero
asserts nothing; it needs tests before it needs a threshold.
The transport was a hand-rolled setupWebSocket(server) that index.ts kicked
off with a dynamic import after listen(). It is a @WebSocketGateway now, with
DatabaseService and EphemeralTokenService injected instead of imported, and
the heartbeat owned by a lifecycle hook rather than an interval nobody stops.
The wire protocol does not move. Nest's WsAdapter dispatches on
{ event, data } and every deployed client sends { type, tripId }, so
TrekWsAdapter translates rather than asking clients to change. It also builds
the ws server itself instead of delegating: the base's port-0 branch uses
noServer and performs the upgrade from its own registry, which never consults
verifyClient, so delegating would have dropped the origin check and answered
101 to anyone. The per-socket error listener stays attached first, because an
unhandled one kills the process (#1576), and the flood guard counts every
inbound frame before parsing, because malformed frames are exactly the ones
worth limiting.
Four things had to stay where they were, and each is a silent failure if it
does not:
The socket registry stays module state. Seventeen bridges and
notifications.instance build `new RealtimeService()` outside the container;
rooms on a provider instance would leave their broadcasts going to an empty
map, with no error and no log.
The socket id stays a monotonic integer. The client echoes it as X-Socket-Id
and broadcast excludes the originator with Number(excludeSid). A uuid is NaN,
NaN === NaN is false, and every client would receive its own writes back,
which shows up as drag-and-drop that jumps rather than as an error.
buildApp creates the http server and binds the adapter before app.init(), and
callers take it from getHttpServer(). Nest binds gateways during init, so a
later adapter binds nothing; an adapter given the app binds to Nest's own
internal server, which this process never listens on. Both boot green with no
client able to connect.
RealtimeGatewayModule is imported by AppModule alone. RealtimeModule is
@Global, so a gateway there follows into every hand-assembled TestingModule,
and Nest's SocketModule then loads an adapter none of them set: it reaches for
@nestjs/platform-socket.io and calls process.exit(1). Eighteen e2e harnesses
died that way before the split.
src/websocket.ts survives as a re-export of the three fan-out primitives. 115
test files vi.mock that path to assert on broadcasts.
The two transport suites pass unchanged, and 30 new cases pin the pieces
directly: the pv and MFA gates, the numeric socket id, the plugin announcement
firing before the room check, both sides of the origin allowlist, and the
flood guard counting frames no handler wants.
BODY_CONTRACT_ALLOW_LIST is empty. The last seventeen handlers type their
body with a createZodDto class, so the fail-closed boot gate has nothing left
to forgive; it still refuses a boot for a new handler that reads @Body()
without one.
Several of those contracts describe rather than constrain, and that is the
whole care in this change. The pipe runs before the handler, so a schema
stricter than the handler moves a user-visible answer:
- TagsController.create answers { error: 'Tag name is required' }, pinned
byte-for-byte in two tests, and the pipe formats as `field: message`. The
DTO leaves name optional and the handler keeps the check that owns the
message. Same shape for the booking-import confirm body, which additionally
gains per-item validation the handler never did.
- PluginsController.activate and install answer 503 when plugins are disabled,
and check that BEFORE looking at the body. A schema that rejected first
would turn that into a 400 for an operator whose server has plugins off.
- setEgressHosts reads a non-array `hosts` as "clear the list", which is the
documented reset. Typing it as string[] would have removed a working admin
action rather than tightened anything.
- PluginRoutesController.route answers { route: null } at 200 for everything
it dislikes, so the client falls back to straight lines. A 400 there would
turn a graceful degradation into a visible error.
files.bridge is gone. The multer options are built by
MulterModule.registerAsync from an AllowedFileTypesService leaf, so the
fileFilter reads the operator's extension list from the container instead of
from a second FilesService built outside it. Nest injects
MULTER_MODULE_OPTIONS with @Optional(), so a token that fails to resolve is
not a boot error: it silently falls back to defaults, with no filter and no
size cap. FILE-020 already proved that for trip files by uploading an .svg
over HTTP and expecting 400; journey uploads had no such case, and now do.
VIDEO_UPLOAD is untouched, including the filename callback that forces .jpg
regardless of what the client sends, which guards the public share proxy.
PluginFrame gets an onError. A frame whose document fails to load left a
blank element at full height, which reads as a hung app rather than as a
plugin that is gone.
typecheck:tests runs in CI now, with the test tree at zero errors. tz-lookup
is left as it is: the build puts it in a lazy helpers chunk, not the entry
chunk, and the remaining split would trade a synchronous render for an async
one to move 172 kB that is already off the critical path.
conditions.test.ts lost noTrips when the condition kind landed; oauth.test.ts
lost the RFC 8707 resource indicator when it became required. Both are
fixtures that stopped matching a contract that grew, and both are filled in
with the value that keeps every existing case reading as it did.
budget.service.calc.test.ts typed its prepare mock from the default
implementation rather than from prepare's own signature, so all ten later
mockImplementation calls had to reproduce that exact three-member literal.
One stmt() helper names the widening instead.
The Express shell set a 100kb limit and stopped applying it the moment the
Nest instance took over parsing: bootstrap passed bodyParser:false and
nothing replaced it. The number happened to stay the same, because it is also
the framework default - but it lived nowhere anybody would look for it, and
one NestFactory default away from silently becoming something else.
It goes through Nest's own parser rather than a second express.json(),
because that parser carries the verify hook that keeps req.rawBody, and the
plugin webhook routes need the exact payload bytes to check a provider's
HMAC. Adding a parser in front of it would have taken rawBody away without
failing anything.
MISC-009 and MISC-010 pin it from both sides.
The bodyParser flag on applyGlobalMiddleware goes with it: one caller, which
passed false.
Not moved: cookieParser stays on the raw Express layer rather than becoming a
configure(consumer) middleware. The platform routes - the OAuth consent flow
among them - are registered on that instance before Nest routing exists, so a
Nest-slot cookie parser would simply not run for them. helmet, CORS,
compression, the forced-HTTPS 301 and the res.on('finish') logging stay for
the same reason: they have to see requests Nest never routes.
The second flake recorded as 'costs' is not a timing flake. It is a live
HTTPS call: budget.service.ts settlement() reaches getRates()
unconditionally, unlike the two guarded call sites above it, and
ExchangeRatesService fetches api.frankfurter.dev with a 10 s abort.
tests/integration/budget.test.ts stubbed fetch nowhere at all, so the suite
talked to the internet - fast when the API answers, ten seconds per call
when it does not. The stub fails closed, which changes no assertion: every
settlement case here is single-currency, so rates never enter the
arithmetic. tools-budget-advanced had the stub inside a single case; hoisted
so a second settlement case cannot quietly reintroduce the call.
The five ReDoS budgets in maps.service.test.ts measured wall-clock time,
which on a loaded machine reports how long the test waited for a core.
MAPS-024 has gone red at 523 ms against an unchanged file for exactly that
reason. They now measure CPU time, which is what catastrophic backtracking
actually burns. This does not soften them: CPU time is always <= wall-clock
time, so every budget got stricter, and pool:'forks' keeps the measurement
to this file.
assignments, budget, collab, accommodations, reservations and booking-import
drop their copied requireTrip/requireEdit helpers for the guard. Status codes and
bodies are unchanged.
places is deliberately NOT converted, and that is the finding of this PR rather
than an omission. Its e2e suite documents that the body pipe answers 400 ahead of
the trip 404 — a parity shift somebody made on purpose — and a Nest guard always
runs before the pipe, so converting it silently reverses that ordering. The first
attempt also dropped the permission check on DELETE, which the integration suite
caught as a 200 where a 403 belonged. places keeps its in-handler check, and
README now says why so the next person does not rediscover it the same way.
collab applies the guard per handler for the same reason files does: its note-file
upload route would otherwise be refused before multer had read the body, and a
response sent mid-upload destroys the socket.
Two coverage notes. The per-domain thresholds are regenerated because deleting a
well-covered helper lowers a file's ratio without anything becoming less tested —
the drops are 0.4 to 1.2 points across six domains and match the helpers removed.
And the atlas integration suite gets the same admin1-bundle warm-up the unit suite
got: ATLAS-013 was paying 3.1s for the one-off decompression inside its own 5s
budget, which is why it only ever failed under load. It now runs in 116ms.
src/services/backupService.ts moves to src/nest/backup/backup.impl.ts, imported
only by its own domain now that the controller reaches it through BackupService.
src/services/ is down to airtrail/.
It stays a module of free functions rather than becoming methods on the service.
The restore path closes and reinitializes the core DB handle, which is the single
most dangerous sequence in the server, and rewriting its shape in the same step
as the move would make a regression there impossible to bisect. The db-lifecycle
question is the last step of this fold, not part of it.
The move also pulled 512 lines out of ungated src/services/ into a gated domain,
which is how the per-domain ratchet caught that the per-entry extraction loop had
no tests at all: the zip-slip refusal, the running decompressed-byte cap (the
declared size in the central directory is attacker-controlled, so only the
running total catches a lying archive), a corrupt stream not being dressed up as
a size refusal, and the reopen failure that has to report restart-required rather
than success. Six cases now cover it, and backup goes from 84% to 96% statements.
The last 20 wire methods move onto the decorators: db.* and the three
unconditional methods (DbRpc), meta.* (MetaRpc) and the host-mediated surface
of users/broadcasts/notify/ai/oauth/scheduler (HostSurfaceRpc). That closes the
ledger at 113/113, so the legacy has() blocks in rpc-host.ts are gone and the
router is down to what was never domain-specific: build the granted subset of
the dispatch map, dispatch, audit, map errors.
PluginHostDepsFactory was the 26-argument wiring sheet behind those blocks. With
the handlers in the domains it has two dependencies left, so it is now
PluginRpcHostFactory: the plugin's own sqlite handle, the inter-plugin router
bound with this plugin as the caller, and the audit sink.
The 18 host-to-plugin hook calls get the same treatment. They used to spell out
the fn name and the timeout at each controller, which meant nothing checked that
a hook:* grant on the consent screen had a consumer behind it, that the fn
matched what the SDK documents, or that two call sites agreed on the budget.
PluginHooks declares all 18 with @PluginHook and performs them, and the
controllers call it by name.
With both halves complete, requireTotalCoverage goes on: a KNOWN_METHOD without
a handler, or a hook without a consumer, now fails app boot instead of surfacing
as a PERMISSION_DENIED nobody can explain.
photoResolverService was two files in one. One half registers and drops rows in
trek_photos and touches no photo provider at all; the other dispatches a stream
or an info lookup to immich or synology. Keeping them together meant every
consumer that only wanted to store a row - the unified trip-photo service, the
journey service - dragged both providers in behind it.
The storage half becomes TrekPhotosRepository in nest/photos, over
DatabaseService. That is also where the photos/ vs memories/ line belongs:
storage here, provider dispatch with memories. photos.bridge.ts serves the two
modules still in services/ that register rows, and dies with them.
Every statement moved verbatim, including the two rules worth stating out loud:
getOrCreate re-encrypts a passphrase onto an existing row rather than inserting
a second one, and deleteIfOrphan never deletes a local photo, because its bytes
are ours and the file would outlive the row.
Four files taken apart rather than moved, because they only worked as a knot.
The mailer becomes its own leaf module. AuthService sends the password-reset
mail while NotificationsModule imports AuthModule for the demo gate in
NotificationsMcp - a mailer inside the notifications domain would have closed a
hard cycle that only a forwardRef could open again. Webhook and ntfy stay with
notifications; auth has no business importing them.
The channel registry stays a module singleton and does NOT become a provider.
PluginRuntimeService pushes its channel getter in at onModuleInit, and
notifications.bridge.ts builds its own NotificationsService for the scheduler
crons and the memories senders. A provider would hand the bridge a second, empty
registry - every plugin channel would then go quiet with no error anywhere.
CHOVR-015 drives that seam end to end: a channel published through the runtime,
delivered through the bridge. Nothing pinned it before; the existing cases all
set the source themselves.
The built-ins no longer self-register by side-effect import. They used to exist
only because notificationPreferencesService happened to `import
'./notifications/builtins'`; removing that line would have silenced email,
webhook and ntfy without a failure. They are built from the injected transports
and registered from NotificationsService's constructor, so both instances have
them.
No cached nodemailer transport: MailerService.createTransport re-reads the SMTP
config and skipTlsVerify per send, exactly as the three inline call sites did,
so an admin changing them does not have to restart.
Preferences become a provider over DatabaseService. Eight places built
NotificationsService by hand, so a tests/helpers/notifications.ts factory keeps
the next constructor parameter a one-file diff instead of an eight-file one.
* refactor(server): move the webauthn config and the account erasure into nest/auth
webauthnConfig resolved the RP ID through its own getSetting() on the db
singleton; it is a WebauthnConfigService over DatabaseService now, injected into
AuthService (passkey_configured) and PasskeyService (four ceremonies). readEnv()
is still read per call, so an operator flipping the RP ID at runtime takes effect
on the next ceremony instead of after a restart.
userCleanupService follows as UserCleanupService. It keeps taking
removeUserFromBudgetItems from budget.bridge rather than injecting BudgetService:
BudgetModule imports AuthModule for the demo guard in BudgetMcp, so AuthModule
cannot import BudgetModule back, and breaking that edge the other way drags
BudgetController, BudgetMcp and its trips.bridge chain into every graph that
imports AuthModule.
The legacy module had no tests of its own - the erasure only ever ran through
AdminService.deleteUser and TripsService.deleteGuest - so USER-CLEANUP-001..009
pin both halves, including the orphan data dir that no permissions row can
describe and the all-or-nothing deletion transaction.
* refactor(server): fold the oauth 2.1 server out of services/
694 lines move into OauthService over DatabaseService, AddonsService and
AuditService. The thin wrapper that only forwarded is gone, and so is the second
path to the MCP addon flag - validateAuthorizeRequest read it through
addons.bridge while the wrapper read it through the injected service, which would
have let the two drift.
The pending authorization codes stay a module singleton in oauth.pending-codes.ts
instead of becoming instance state. The SDK still serves /oauth/authorize,
/oauth/register and /oauth/consent from its own router, mounted on Express before
app.init(), so it reaches the domain through the new oauth.bridge.ts - two maps
would break the authorization-code flow silently, since the SDK only ever answers
"Authorization grant is invalid.". The import-time sweep keeps its .unref() and is
a named function now so a test can drive it.
scopes and sessionManager are imported deep, never through the src/mcp barrel:
the barrel re-exports revokeUserSessionsForClient and pulls the domain bridges
back in, which is the nest -> mcp -> nest cycle. Same comment as admin.service and
auth.service carry.
The 70 cases of the legacy suite moved over unchanged, bound to a hand-built
service. Fifteen more cover what free functions never let anyone reach: the code
store at capacity, an expired code, an expired access token, refresh against a
public and an unknown client, the RFC 8707 resource checks, and that a code
written through the container is read back through the bridge.
* refactor(server): move the wiki reader into the help module
wikiService had exactly one consumer, HelpController, and touches no database -
it stays a plain module rather than becoming a provider, the way
auth/ephemeral-tokens.ts does, so the text and asset caches keep being one per
process without any ceremony around it.
The WIKI_DIR anchor needed one more level: src/services sits three below the repo
root, src/nest/help sits four. The existing path-resolution case caught it, which
is what it is there for - a wrong anchor makes help degrade to the GitHub fallback
without anybody noticing.
Also brings the nest README back in line: services/ is down to five top-level
files, and the passkey and budget entries no longer point at paths that were
deleted since they were written.
The last file in be-places-helpers-fold, and the one that had to be a provider
rather than a move: it carries three in-memory structures that only do their job
if the process has exactly one of them.
recentFailures stops a screenful of markers hammering an API that is down
inFlight stops a stampede when several requests miss the same placeId
knownOnDisk saves an existsSync per cache hit
A second instance gives the stampede guard its own inFlight map, so two
simultaneous requests for the same uncached place both call the provider. That
is why the scheduler takes this one from the container instead of constructing
it: SchedulerDeps gains a placePhotos entry alongside backups, filled from
app.get() in index.ts — the shape PR #1827 was built for.
The module-scope mkdirSync becomes onModuleInit, and GOOGLE_PHOTO_DIR is read
per call through RuntimeEnvService rather than frozen at import. That is not
cosmetic: TREK_PLACE_PHOTO_DIR is the seam the tests use to keep off the real
uploads tree, and its own test used to have to import the module dynamically to
set the variable in time. The __dirname fallback moved a level with the file and
is corrected accordingly.
Consumers: maps, places and share inject it; places.helpers.reclaimPhotoCache
takes it as a parameter, since it is a free function.
Six test seams follow. Four path mocks become injected stubs with the same mock
functions behind them; the collections case keeps the REAL implementation,
because what it asserts is that removeIfUnreferenced counts collection_places as
a reference (#1081) — a stub there would assert nothing. The share e2e overrides
the provider in its container.
services/ is down to 14 top-level files, from 44 when this started.
* refactor(server): route every trip-access check through DatabaseService
Twelve Nest services carry the same verifyTripAccess/canEdit pair. Six already
asked DatabaseService.canAccessTrip; the other six went through
services/tripAccess, a nine-line re-export of the identical function. Same
query, same result, two ways to reach it.
All twelve now take the injected route, so services/tripAccess is gone.
The bigger duplication was in the types. Thirteen files declared their own
`Trip` for the same return value, in three different shapes:
NonNullable<ReturnType<typeof verifyTripAccess>> 6x
{ user_id: number } 4x
TripAccess 3x
The narrow one is why assignments, days, places and accommodations all wrote
`as Trip | null | undefined` on the call — the cast existed to shrink
TripAccess down to a local type that was never meant to differ from it. All
thirteen now say TripAccess, and the four casts are gone with no other change.
middleware/tripAccess.ts goes too: it was already dead in production — nothing
under src/ imported requireTripAccess or requireTripOwner, only comments in the
services that had absorbed them. Its test went with it.
* refactor(server): fold the query helpers and trip joins into the container
Two more legacy files whose only reason for staying outside the container was
that they predated it.
services/queryHelpers.ts held five exports doing two different jobs. Three are
the batch loaders that keep the list endpoints off N+1 queries — they hit the
database and are now QueryHelpersService over the injected connection, imported
by assignments, days, places and share. The other two, formatAssignmentWithPlace
and ratingAggregate, touch no database at all; they sat next to the loaders only
because the loaders were free functions too. Those stay free functions, in
common/rowShape.ts.
The README note on the assignments service claimed the loaders had to stay in
services/ because the day, share and place services were unmigrated. All three
have since moved, so the reason is gone.
services/tripMembership.ts becomes TripMembershipService in its own module. It
could have folded into auth or trip-invite, but both call it — as do oidc and
the plugin RPC host — and putting it in either would have made AuthModule and
TripInviteModule import each other.
Neither module is @Global, matching PermissionsModule: e2e TestingModules
resolve them transitively through each consumer's explicit import.
Test seams changed shape accordingly. Two path mocks of services/tripMembership
became injected stubs; the trip-invite e2e overrides the provider in the
container instead. The oidc test keeps the real implementation, which is what
OIDC-SVC-045 was always asserting.
ratingAggregate moved out of the ungated services/ tree into the coverage-gated
one, so it gets the direct tests it never had — as does the tags/participants
pass-through, which two long-dead fixture variables in the moved test file had
been waiting for.
Also drops four stale comments that outlived their subject: the requireTripAccess
middleware they pointed at was deleted in the previous commit.
Eight files in services/ hold no state and touch no database: avatarUrl,
conflictResult, demo, passwordPolicy, timezoneService, cookie, and the two
crypto helpers. They are imported from 65 places, 62 of them already inside
src/nest, so having them live in the legacy layer was pure inertia.
They stay plain functions rather than becoming providers. Three importers sit
outside the container and would have no way to reach an injectable one:
db/migrations.ts runs before the app is built, and middleware/auth.ts and
middleware/mfaPolicy.ts run beside it. Wrapping these in DI would buy ceremony
and nothing else.
mfaCrypto and apiKeyCrypto move together into common/crypto — they read the
same ENCRYPTION_KEY, and the directory says so.
services/ drops from 44 files to 36, 9,870 lines to 8,493.
One trap worth recording: src/app-config and src/nest/app-config both exist.
cookie.ts imported '../app-config' from services/, which resolved to the former;
after the move the same string silently pointed at the latter, which has no
readEnv. It is now explicit about which one it wants.
Nothing else changes — no signatures, no behaviour, no tests rewritten beyond
their import paths.
Every country now carries the status of the strongest source it came from:
a country reached by both a past and a future trip is visited, one that only
appears in upcoming trips is planned. Marking a country by hand still wins
over everything, since that is a statement of fact rather than a date.
totalCountries drops to the visited ones and totalCountriesPlanned sits
beside it; continents and mostVisited follow the same rule. countries[]
stays a superset of what it returned before, so an older client keeps
painting the map exactly as it did.
The endpoint query has to select trip_id to know which trip a leg belongs
to, which makes its DISTINCT per trip — the coordinates are collapsed again
in JS so the point-in-polygon lookup still runs once per location.
notifications.dto.ts wraps the @trek/shared request schemas via createZodDto
and types all five @Body() params; the five NotificationsController entries
leave the body-contract allow-list (ratchet gate enforced). Contract catch:
the client sends server/token: null on test-ntfy to mean 'use the saved
value', so testNtfyRequestSchema gains .nullable() there (spec pinned). The
inline response-enum and url-type checks die — malformed bodies now get the
pipe's standard envelope; valid bodies behave byte-identically. Two
integration cases pinning pre-ratchet tolerances (flat notify_* preferences
body, body-less test-smtp POST) update to the current wire contract.
The send() dispatcher (services/notificationService.ts) and the in-app store
(services/inAppNotifications.ts) fold together into the DI-native
NotificationsService over DatabaseService + RealtimeService; the prefs
matrix, transports, channel registry and inAppNotificationActions stay plain
infra modules. A 1-export notifications.bridge covers scheduler, legacy
adminService/memories and the six lazy fire-and-forget sends; AdminController
and the plugin RPC host (24th factory dep) inject. The 5-tool registrar +
notifications-in-app resource move to notifications.mcp.ts. Tests move with
case IDs preserved (NSVC-*, INOTIF-*, + NSVC-020 bridge pin); ~18 suites
repoint path mocks/warm-ups; the module e2e goes DI-native.
Also carries the verified-quirk fixes that would normally trail as a
separate fix(server) commit (file-overlap with the fold): toUtcIso on all
three created_at paths (INOTIF-013/014), respond claims the response CAS
before running the action handler with release-on-failure (INOTIF-011/012),
synology's fire-and-forget send gains .catch, the dispatch-failure log
unwraps Error messages (NSVC-021), and the MCP resource description drops
its false 'unread first' claim.
Fold the 1497-line services/authService.ts into the DI-native nest/auth
domain (Wave-5 chain opener, unblocked by the atlas fold):
- Pure crypto half (backup-code hash/match/generate, stripUserForClient,
key masking, the import-time DUMMY_PASSWORD_HASH timing equaliser and
avatarDir mkdir — documented parity exceptions) moves verbatim to the
plain module nest/auth/auth.helpers.ts.
- DB half folds into AuthService over DatabaseService + injected
PermissionsService (ex permissions.bridge) + AtlasService (ex
atlas.bridge, deleted on schedule). mfaSetupPending and the per-email
reset throttle stay module-scoped so the bridge and container instances
share them; avatarDir and require('../../../package.json') re-anchor one
directory deeper — with the two injection swaps, the only non-verbatim
lines. Controller-facing method surface unchanged.
- New 8-export auth.bridge.ts for out-of-container consumers: mcp/index.ts
token verification, the journey/notifications/transports registrars'
isDemoUser, and the still-legacy adminService/oidcService/passkeyService.
- 15 domain *.mcp.ts demo guards now inject AuthService (their modules
import AuthModule); atlas.mcp.ts alone stays on the bridge because
AuthService injects AtlasService and the reverse module edge would close
an AuthModule<->AtlasModule cycle (places.mcp precedent). OidcService and
PasskeyEnabledGuard inject AuthService too.
- Tests move with case IDs preserved: authService.test.ts ->
auth.helpers.test.ts, authServiceDb.test.ts -> auth.service.test.ts
(+ AUTH-BR-001..007 bridge delegation, + AUTH-DB-050..088 pinning the
previously untested getAppConfig/login/MFA/reset/admin-settings branches
now under the src/nest coverage gate). auth.e2e converts DI-native (real
bcrypt, real audit rows); oidc.e2e swaps its dead path mock for an
instance spy; websocket/integration suites repoint to bridge/helpers.
Quirks preserved byte-for-byte (0-coord drop in getTravelStats,
un-transactioned registerUser/verifyMfaLogin multi-writes, unguarded
revokeUserSessions in deleteMcpToken, current! assertions in
updateApiKeys) — fixes land separately.
The place fold capped the Google list-import body at 8 MB but left the Naver
importer reading its pages unbounded — same attacker-influenced URL (the folder
id), and worse in practice because that path is a pager that buffers a fresh
body on every iteration.
fetchPage now checks the declared content-length before reading and the actual
length after, returning the existing 'Failed to fetch list from Naver Maps' 502
on either. Reading the body as text and parsing it explicitly (rather than
apiRes.json()) is what makes the post-read check possible; the malformed-payload
branch keeps its 'Invalid list data received from Naver Maps' 400 because the
JSON.parse stays inside the same try.
The post-read check is the one that matters for a hostile server: a chunked
response carries no content-length, so the declared check alone is evadable.
Covered for both providers now — PLACE-SVC-073b (Google, chunked) and
PLACE-SVC-074..077 (Naver: declared, chunked, malformed, happy path).
The five Naver fetch mocks in the integration suite move from json() to text().
Fold the 1029-line services/placeService.ts into the DI-native PlacesService:
the CRUD + ratings SQL, the GPX/KML/KMZ importers and the Google/Naver list
importers become methods over the injected DatabaseService, byte-identical
statements, COALESCE semantics and the If-Match conflict protocol (#1135).
The pure half — frozen XML parsers, the KMZ unpacker, the dedup predicates,
the Google hex-id parsers, reclaimPhotoCache — moves to places.helpers.ts
(maps.helpers precedent).
The 10-tool mcp/tools/places.ts registrar plus the trek://trips/{tripId}/places
resource move onto the decorator registry as places.mcp.ts. search_place travels
with them because its gate is places:read, not the read-only geo group, and now
injects MapsService. TripsService, DaysMcp, BookingImportService and the plugin
RPC host (PluginHostDepsFactory's 21st constructor dep) all inject PlacesService,
so the domain needs no bridge at all. The two assignments.bridge imports that
places.mcp.ts keeps are a deliberate cycle break: AssignmentsModule imports
DaysModule and DaysModule now imports PlacesModule for days.mcp.ts's place
creation, so injecting AssignmentsService there would close
DaysModule -> PlacesModule -> AssignmentsModule -> DaysModule. Same seam
reservations.mcp.ts uses, same trade trips.bridge.ts documents.
Also folds services/placeEnrichment.ts in — its DB/websocket/Maps half becomes
PlacesService methods over the injected DatabaseService/RealtimeService/
MapsService, its pure match selector joins places.helpers.ts — which retires
nest/maps/maps.bridge.ts with its last consumer.
Adopts places.dto.ts for the seven grandfathered body contracts and clears every
PlacesController entry from body-contract-allow-list.ts. Two consequences worth
knowing: the bespoke 400s 'Place name is required', 'ids must be an array of
numbers' and 'URL is required' are now the ZodValidationPipe envelope, and
because a pipe runs before the handler a malformed body 400s ahead of the
trip-access 404 it used to follow — the same trade the todo and trips migrations
took. placeBulkUpdateRequestSchema.ids drops .min(1) so the endpoint's
empty-list short-circuit stays reachable.
Fixes four defects the relocation had faithfully carried, each with a
failing-first regression test:
- create dropped lat/lng of exactly 0, so a place on the equator or the prime
meridian lost its coordinates (`x || null` cannot tell absent from zero).
- duration_minutes: 0 was unsettable — replaced by the 60-minute default on
create, and read as "absent" by COALESCE on update.
- the journey delete hook fired on unscoped ids: DELETE /:id, POST /bulk-delete
and the delete_place MCP tool could detach another trip's journey entries for
an id they then refused. All three scope first via the new
PlacesService.scopedIds, mirroring the guard PluginHostDepsFactory already had.
bulk_delete_places also moves its hook ahead of the DELETE —
journey_entries.source_place_id is ON DELETE SET NULL, so running it afterwards
left the entries as orphans.
- the place search treated % and _ as LIKE wildcards; a bare % returned the whole
trip. The three clauses now carry ESCAPE with the term escaped.
Plus hardening on the Google list fetch: the response is capped at 8 MB
(declared content-length and post-read length, the transit.service precedent)
and its JSON.parse is guarded, so a malformed provider payload produces the
existing 'Invalid list data received from Google Maps' 400 instead of throwing.
Quirks deliberately preserved: the non-COALESCE route_color (#776),
currency/transport_mode staying unclearable, importGpx returning null for a file
that parses but yields no usable geometry, update skipping the If-Match check
when the stored updated_at is null, and every string-valued `x || null`.
Tests move with their case IDs: placeService.test.ts and the wrapper suite merge
into unit/nest/places.service.test.ts, kmzUnpack into places.helpers.test.ts,
placeEnrichment into places.enrichment.test.ts. The places e2e converts to the
DI-native pattern (real SQL against temp-db DDL), tools-places.test.ts is kept
and extended so it exercises the decorator path through the test registry, and
the plugin-host and booking-import path mocks become constructor stubs.
Opening one trip fires a 404 per place without a photo, 30 to 60 of them from
one IP in seconds. CrowdSec and fail2ban read that as scanning; the reporter
had three users banned within five minutes. A place having no photo is not an
error and no longer answers as one.
OSM places were also sent to Google as node:/way:/relation: ids, which can
only return 400 INVALID_ARGUMENT — a billed round-trip and about 1.3s per
place. getPlacePhoto already guarded coords: this way; the OSM prefixes now
get the same treatment, as does getPlaceDetailsExpanded, which had no guard.
The negative cache separates a place having no photo from a failed provider
call instead of expiring both after five minutes and replaying the storm.
214 items came out of writing the tests. Real defects are corrected, dead code
is removed rather than covered, and the tests that pinned the old behaviour
move with them.
Also drops two agent coverage scratch files that slipped into the coverage PR
and adds the ignore rule that keeps them out.
Maurice