mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-10-19 21:46:49 +00:00
8a09f55571e3ce211e4fdd6bc770d1d267979cb6
844 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Maurice
|
8a09f55571 |
Give three more admin surfaces back to the domains that own them
default-user-settings, notification-preferences and oidc leave AdminController. The first two were pure pass-throughs on AdminService that did nothing but forward and cast; the third had its SQL there for a domain that already had a module of its own. AdminService loses SettingsService and NotificationPreferencesService entirely. Two things came out of the move rather than going into it. The OIDC lockout guard now has tests. It refuses to clear the SSO config while password login is disabled — otherwise the instance has no SSO to log in through and no password form either — and nothing covered that branch before, because it sat among admin tests that only exercised the happy path. The admin e2e app had to learn about the split. It assembles AdminModule alone, so the moved routes 404'd there while working in production; it now imports the three domains too. That is the same failure mode the module-provider guards catch for RPC classes, and it is worth knowing that an e2e app which builds a subset will find it first. Also fixes the audit route note: audit-log stays on AdminController on purpose. AuthModule imports AuditModule, so an admin-gated controller in AuditModule closes a cycle — the same trap the addons toggles hit. |
||
|
Maurice
|
d4a1601855 |
Give the packing templates and the feature flags back to their domains
Twelve admin packing-template routes sat on AdminController and reached
PackingService through eleven pass-through methods on AdminService that did
nothing but forward and cast. PackingService already owns all three template
tables, so the routes move to AdminPackingTemplatesController next to it and the
pass-through block is gone. AdminService no longer injects PackingService at all.
The three places flags were raw app_settings SQL on AdminService, sitting beside
bag-tracking and collab-features which it already delegated to AddonsService.
They move to AddonsService, which owns every other flag, and AdminController now
calls it directly rather than through delegating methods.
The toggle ROUTES stay on AdminController, which is a departure from the plan and
worth stating: an admin-gated controller in AddonsModule makes that module import
AuthModule, and PluginGuardsModule imports AddonsModule precisely because it is a
leaf — Nest refused to build the graph. Same for the addon CRUD, whose PUT
cascades into PluginRuntimeService. Both notes are in the code.
Paths, response shapes, the {error,status} envelope, the create-201 split and
every audit action are unchanged.
|
||
|
Maurice
|
94221a6d91 |
Roll TripAccessGuard onto the remaining trip-scoped controllers
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. |
||
|
Maurice
|
8c0280e287 |
Replace the copied requireTrip helpers with a TripAccessGuard
Every trip-scoped controller carried its own three-line requireTrip, and a requireEdit next to it: eighteen copies of a security check, each of which had to remember that no access is a 404 'Trip not found' and never a 403 — the distinction that stops a stranger learning which trip ids exist. One of them getting it wrong would have looked like nothing. TripAccessGuard resolves :tripId once, refuses what the user cannot reach and hands the row to the handler through a @Trip() param decorator (a param decorator rather than a req.trip cast, because the request type that used to be widened for that is gone). @RequirePermission carries the same action string the domain services pass to checkPermission, so the HTTP and MCP paths cannot drift apart on which right they demand. Status codes and bodies are unchanged. Piloted on days, day-notes, todo, packing and files. Two things worth knowing: Files applies the guard PER HANDLER and leaves the upload route on its own check. Guards run before interceptors, so a class-level guard would answer 404 before multer had read the multipart body, and a response sent while the client is still streaming gets the socket destroyed — the caller sees ECONNRESET instead of the 404. Refusing after the body has been read costs the bandwidth and keeps the answer readable. The services keep verifyTripAccess and canEdit: the large majority of their callers are *.mcp.ts tools, which never pass through an HTTP guard. canEdit lost its incidental controller-test coverage in the move, so it now has direct tests asserting the action and the shared flag — the same pair the guard reproduces. Also fixes the backup zip-slip test that failed only on CI: it asserted a drive-letter path, which is absolute on win32 and an ordinary relative segment on Linux, so the branch it was guarding never fired there. |
||
|
Maurice
|
fef338091c |
Fold backupService into the backup domain
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. |
||
|
Maurice
|
6680bc7c74 |
Split PluginsModule into four, and gate coverage per domain
PluginsModule was 21 controllers, 24 domain imports and 10 providers in one class, which made it the hub most of the Nest graph ran through: AdminModule imported it for a single cascade-disable call and inherited all of it. It is now PluginsRuntimeModule (supervisor, capability router, hook contracts), PluginContributionsModule (the 14 read-only hook controllers, moved to contributions/), PluginOAuthModule (the credential leaf, moved to oauth/) and a composition root that keeps the install and delivery controllers. AdminModule imports the runtime alone. The three plugin files that reached for the db singleton alongside the injected DatabaseService no longer do. plugin-host-state and signature-status take the connection from the caller, and the three per-user settings functions that were bare exports 'so the RPC host wiring can call them directly' are now PluginUserSettingsService - the host wiring is providers, so it injects them. Coverage moves from one repo-wide 80% to a per-domain ratchet. The single number let a well-covered domain subsidise a thin one: booking-import sits at 50% and integrations at 20%, and the aggregate still cleared the bar, so either could have lost another ten points unnoticed. Each domain now carries its own measured floor, regenerated by scripts/coverage-thresholds.mjs. Two test-side findings on the way: expect(undefined).toContain(SomeClass) PASSES in this vitest version - it only throws for a string needle. Every '<X> is listed in its module providers' guard was written that way, so all twelve of them silently stopped guarding the moment their class moved. They go through expectRegisteredProvider now, which asserts the array shape first. The atlas region test paid for the one-off decompression of the multi-MB admin1 bundle inside its own 5s budget, which is why it failed under load and passed alone. The bundle is warmed in beforeAll instead. |
||
|
Maurice
|
dc81a5365e |
Make PluginsModule bring the whole plugin surface with it
With requireTotalCoverage on, a test app that assembles a subset of AppModule now fails to boot: the admin e2e suite pulls PluginsModule in through AdminModule, but WeatherModule was only ever reachable from AppModule, so weather.get had no handler and validate() refused the start. The invariant the gate needs is that importing PluginsModule pulls in every domain that owns part of the wire surface. WeatherModule was the one domain missing from that list. |
||
|
Maurice
|
f72aba0c91 |
Finish the plugin RPC decorator rollout
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. |
||
|
Maurice
|
611f94e820 |
Consolidate the schema-error fallback and cover the defensive branches
Aiming the new files at 95% rather than at the 80% gate turned up something worth fixing rather than testing around: `error.issues[0]?.message ?? 'bad input'` had been copied into twenty handlers. That is twenty two-branch fallbacks that no test can reach, because a real Zod failure always carries at least one issue. They are now one schemaMessage() in rpc-params.ts, with one test that does reach both sides. The rest of this is the branches the handlers inherited from the router: the `!= null` link ternaries, the "service said no" paths, the ValidationError and NotFoundError translations, and a reject case per schema-validated method. Two limits found while measuring, both real rather than laziness. istanbul counts each @PluginController line as a branch and only ever takes one side, so eight branches across the rpc files can never be covered and a two-method file caps out around 50%. And the three update schemas stay deliberately permissive, because the migration's rule is parity over tightening, so their reject branch is defence in depth no payload reaches. Everything else went from 50 uncovered branches to 29. |
||
|
Maurice
|
294cfcd8d7 |
Move the addon-gated domains onto the decorators
collab, journal, atlas, vacay and collections: 32 more methods, so the registry owns 93 of 113. rpc-host.ts is at 565 lines, down from 1332, and the deps factory at 384, down from 944. These five are the risky group, because their addon gates lived inside the deps factory's closure rather than in the router. A handler that calls its service directly still works, just without the check, and neither the compiler nor the router tests would notice. So addon-gates.rpc.test.ts walks all 37 gated methods as one table and asserts two things for each: with the addon off it is refused with that addon's message, and the service is never reached. The table length is pinned, so a later method added without a row is a failure rather than a silent gap. One ordering detail worth knowing: on the atlas writes the router evaluated its params before the addon gate inside the closure, so a malformed country code still reports BAD_PARAMS rather than the addon. The handlers keep that order. The collections service reports refusals as status-tagged errors rather than typed ones, so mapCollectionError came across with the handlers: 403 and 404 become RESOURCE_FORBIDDEN, 400 and 409 become BAD_PARAMS, and anything untagged stays a HOST_ERROR instead of being dressed up as a permission problem. |
||
|
Maurice
|
7bca741f28 |
Cover the fallback branches the moved handlers brought with them
The branch threshold over src/nest/** dipped to 79.89% after the move: the new *.rpc.ts files carry the defensive branches the router closures had (?? '', || current, optional cascades), and none of them were exercised. Adds the cases that reach them: a booking whose cascade does not fire, an update that keeps the stored title, a bag colour of the wrong type, a non-array userIds, a missing bag across all three bag writes, and the block delete without a partner reservation. Also corrects two assumptions I had written into the booking tests: title and type are required by the shared schema, so the handler's own fallbacks sit behind it as defence in depth rather than as the first line. |
||
|
Maurice
|
010242b107 |
Move costs, reservations and accommodations onto the decorators
Eleven more methods, so the registry owns 61 of 113. rpc-host.ts is at 771 lines,
down from 1332.
costs is the first addon-gated domain, and the addon check sits in a different
place in each of the two reads: inside the membership callback for getByTrip, so a
caller without trip access never learns whether the addon is on, and before
everything for listMine, which has no trip to check. Both orderings are asserted.
The writes keep their own refusal wording ("no permission to edit costs on trip N")
instead of going through requireTripEdit, which words it differently.
The booking paths are cascade-heavy and stay that way: creating a reservation can
create an accommodation and a budget item, deleting one removes both again, and
deleting a lodging block takes its partner reservation and budget item with it.
Each cascade is a broadcast the web app relies on, so they are asserted by event
name and order.
CostsRpc reaches the trip list through trips.bridge rather than by injection.
BudgetModule cannot import TripsModule, because TripsModule already imports
BudgetModule; budget.mcp.ts uses the same escape for the same reason.
|
||
|
Maurice
|
2ec6d006b0 |
Move the trip surface onto the decorators
Twelve more methods, so the registry owns 50 of 113. rpc-host.ts is down from 1332 lines to 830 and the deps factory from 944 to 500. trips.update keeps its two field-level gates: archiving and changing the cover each need their own admin-configurable permission on top of trip_edit, so a plugin cannot do either on a trip it may otherwise edit. Member management keeps its own grant for the same kind of reason, since adding a member grants trip access, and the owner still cannot be removed through it because that would orphan the trip. canEditTrip stays in HostDeps for now: the db:meta gate still resolves an entity's edit permission through it and has not moved yet. |
||
|
Maurice
|
c326d39a07 |
Move places, days and itinerary onto the decorators
Eight more methods, so the registry owns 38 of 113 and rpc-host.ts is under 1000 lines for the first time. itinerary.* lives in assignments/, not in days/, because AssignmentsModule already imports DaysModule and putting the handler the other way round would have closed a cycle. It keeps riding on day_edit, which is what the app gates it on. The delete path in places carries an ordering constraint that is silent when broken, so it has two tests of its own: the journey hook has to run BEFORE the row goes, because journey_entries.source_place_id is ON DELETE SET NULL and afterwards there is nothing left to detach, and the place id has to be scoped to the trip before the hook runs at all, because the hook keys on the place alone and would otherwise detach another trip's entries even though the delete itself is refused. |
||
|
Maurice
|
d58f7aa399 |
Drop a dead expression from the files RPC test
Left over from writing the demo-mode case: a cast statement with no effect, which eslint flags as no-unused-expressions. It is the one error in an otherwise warnings-only run, so `eslint .` exits 1 and the Server Tests job fails on lint before it runs a single test. |
||
|
Maurice
|
dbb0016cac |
Move the file surface onto the decorators
Six more methods, so the registry owns 30 of 113, and the deps factory is down to 660 lines from 944. Files are the awkward corner of the plugin surface and stay that way on purpose. Reading a file's bytes is a separate grant from reading its metadata, because a passport scan is more sensitive than its filename, and the three writes use three distinct rights instead of one domain action. FILES-RPC-006 pins that by watching which permission each operation actually asks for. This is also the only plugin path that touches disk, so the tests lean on it: the extension blocklist, the size cap checked before the read rather than after, a path that resolves outside the uploads root, and the demo-account upload refusal that mirrors the REST guard. |
||
|
Maurice
|
ca88676858 |
Move packing onto the decorators and de-duplicate its privacy fan-out
Nine more methods, so the registry owns 24 of 113. The #858 privacy logic existed three times: PackingService had the primitives, the controller layered emitToViewers and broadcastUpdate on top privately, and the plugin deps factory carried a standalone copy of all four, with a comment asking for them to be kept in lockstep by hand. Migrating packing meant either copying them a fourth time or fixing that, so the two private controller methods moved into PackingService and the factory copy is gone. Controller and PackingRpc now call the same code, and the controller suite proves it by passing with all 45 assertions unchanged. The four public/private transitions get one test each, because each one is a way to leak a private item to the whole trip room, plus a test that the privacy is read before the write rather than after: reading it afterwards would report "already private", skip the room delete, and leave the item on every member's screen. |
||
|
Maurice
|
62978aab5a |
Move five more domains onto the plugin RPC decorators
categories, weather, rates, todos and daynotes: 11 more methods, taking the registry to 15 of 113. Handler bodies and messages are verbatim; the ledger test tracks the split. Two things the move surfaced rather than introduced. PluginGuards could not stay in PluginsModule: that module imports 22 domain modules, so a domain importing it back for the guards would close a hard cycle and the only way out would be forwardRef. It now lives in a PluginGuardsModule leaf that imports nothing which leads back, the same shape MailerModule got for AuthModule. And the todo and daynote writes broadcast. Those events are not decoration, they are what keeps an open session from showing stale data after a plugin write, so they came across from the deps factory with the handlers, and there is a test that a failed write broadcasts nothing. Worth knowing for the next domains: todos are edited under 'packing_edit', not under a todo-specific action, and TODO-RPC-003 pins that. Each new class also asserts it is actually listed in its module providers, since a decorated class nobody registers is invisible to discovery and answers PERMISSION_DENIED with no other symptom. |
||
|
Maurice
|
9b3d37ea29 |
Move the tags plugin surface onto the decorators
Wires the registry into the host and takes the first domain across, so the kit lands with a worked example rather than as scaffolding nobody uses yet. The registry is an additive, optional fourth constructor parameter on PluginRpcHost, so all 107 existing construction sites keep compiling untouched; that is what makes the rest of the rollout a per-domain move instead of a big-bang one. bindInto runs LAST in the constructor, so a method still owned by one of the has() blocks collides loudly instead of being silently overridden. tags is the right pilot: four methods, a service that is already DI-native, and it exercises the read/write permission split, the userless refusal and a BadParams write. Handler bodies and messages are verbatim; only deps.foo(...) became this.tags... and uid became ctx.actingUserId. dispatch now strips the supervisor's _inv marker. The supervisor resolves the acting user from it before dispatch and then passes the request on untouched, so it has been visible to every handler and is in effect a reserved param name. Nothing reads it: auditResource takes named fields only, and the one handler that touches the params object as a whole (packing.setBagMembers) reads a named field off it. Dropping it keeps a future strict parse from rejecting calls every shipped plugin makes. Two tests carry the rollout from here. rpc-coverage.test.ts derives both owner sets from the code and proves they partition the method vocabulary exactly, so a PR that moves a method and forgets to delete the legacy registration fails in CI. And because a decorated class that no module registers is invisible to discovery, with every call to it coming back PERMISSION_DENIED and no other symptom, tags.rpc.test.ts asserts TagsRpc is actually listed in providers. |
||
|
Maurice
|
6b6fbbd94c |
Lift the plugin RPC resource gates into an injectable
A decorated handler cannot reach the router's private tripRead / requireActor / requireTripEdit, nor the canEditTripAs and requireAddon that live inside the deps factory's closure. PluginGuards holds all of them so a *.rpc.ts class can inject what it needs. Every message is copied character for character, including the two asymmetries that look like typos and are not: a trip read says "trip reads", while requireActor appends the word "writes" to whatever noun it is given, and some domains throw their own inline variant rather than calling it at all. The tests pin the exact strings, not just the refusal, because shipped plugins read them. |
||
|
Maurice
|
7b11680cc3 |
Add a decorator kit for the plugin RPC surface
The method-to-permission association exists twice today: declaratively in METHOD_PERMISSION, and imperatively as 44 if (has(...)) blocks in rpc-host.ts, with nothing asserting the two agree. protocol-paths.test.ts checks that each method has some permission, never that it is the same one the router gates on, and plugin-audit.ts inherits any drift silently. @PluginMethod binds the two together. Its permission argument is typed as MethodPermission<M>, so it resolves to the single literal the table assigns to that method and a mismatch does not compile. validate() re-checks the same argument against the same table at boot, because types evaporate under an as cast. @PluginHook does the same against HOOK_PERMISSION. UNCONDITIONAL_METHODS was missing from envelope.ts, so the three methods the router registers outside every grant check had no vocabulary of their own. Adding it here, with a compile-time proof that it stays disjoint from KNOWN_METHODS: giving those three a METHOD_PERMISSION row would change which methods isAuditable considers, and settings.get is deliberately the one unconditional method that is not audited. The kit is written to stay extractable into its own package, so an ESLint rule pins what it may import. Note the rule is a deny list on purpose: these patterns are gitignore-style, and a leading ** would exclude the parent directory of every allowed path, which makes the ! re-includes silently ineffective. |
||
|
Maurice
|
7d0e90eb47 |
Make the plugin permission facts live in one place
The hook->permission map existed verbatim twice (the host supervisor and the plugin SDK) and the permission list four times (envelope, SDK manifest, and both admin panels in the client). All of them were in sync when measured - 63 entries each, 15 hook entries each - so this changes no behaviour. It removes a quiet outage: a hook whose permission is added on only one side fails silently in production, with the plugin installing, activating and then simply never being called. protocol/envelope.ts is the single source now. METHOD_PERMISSION was typed Record<KnownMethod, string>, so its value side was unconstrained - 'db:read:trps' compiled and made the method unreachable forever. It is `as const satisfies Record<KnownMethod, KnownPermission>` now, which catches a typo'd value, a missing key and an extra key. The derived KnownPermission and MethodPermission<M> are what the @PluginMethod decorator will check its argument against. Deliberately no explicit annotation on METHOD_PERMISSION: with one, typeof is that Record, MethodPermission<M> widens to the whole union, and the check silently evaporates. server/scripts/gen-plugin-facts.ts imports envelope.ts and writes the SDK and shared copies. It does not scrape. The guard it replaces pulled the block out of plugin-supervisor.ts with a regex that goes null on a reformat, and it ran only from prepublishOnly - drift would have surfaced as a broken npm release. The generated files are checked in because publish-plugin-sdk builds from a standalone checkout with no server present. The SDK's re-exported types stay wide. plugin-sdk compiles under strict, and grantGaps indexes HOOK_PERMISSION with a plain string, so an `as const` there is TS7053 and would break the publish. Strictness belongs on the host side. --check also fails on a permission with no admin.plugins.perm.* translation and on one the CLI has no hint for. Both are silent today. plugin-sdk/** was in no workflow path filter, so nothing in CI ever ran its tests. It is in the filter now, alongside a job that runs the drift check. |
||
|
Maurice
|
f0f69df67e |
Fold the journey domain into Nest and finish the MCP migration
journeyService (1324 LOC, 36 exports) and journeyShareService (203) become JourneyDomainService and JourneyShareService. Every SQL statement, error string and broadcast is the one that shipped. The two could never be split: the share service gates on isOwner, which lives in the domain service, so it injects it. JourneyDomainModule is a leaf carrying just the two services. Places, assignments and the plugin host each need a handful of journey functions; importing JourneyModule for that would drag MemoriesModule and both photo providers into their graphs. Same shape as TrekPhotosModule. The 23 tools in src/mcp/tools/journey.ts become JourneyMcp. src/mcp/tools/ is down to _shared.ts and registerTools() no longer fans out to anything. journey.bridge.ts keeps four exports, for the journey resources in src/mcp/resources.ts that register at import time. McpAccessMode is host-augmentable now. journey:share is neither read nor write, so the three share tools would have had to be opaque predicates - and predicates bypass the access policy entirely, invisible to the boot-time scope gate. A scope typo in one would ship as a silently ungated tool. McpAccessModeRegistry mirrors McpAccessGroupRegistry; unaugmented, mode stays 'read' | 'write'. Fixes a latent bug in tests/helpers/mcp-test-controllers.ts: it constructed PlacesService with five of eight arguments. tsconfig only includes src, so nothing typechecked it, and the missing dependencies were harmless until the journey skeleton hooks landed on the place write paths. journey.mcp.ts arrived from src/mcp/ at 14.5% branch coverage - the legacy registrar had no test file of its own. 40 tool tests take it to 93.4%, and 25 cases for the skeleton lifecycle, the photo gallery and the preference row take the domain service from 73.9% to 82.8%. |
||
|
Maurice
|
99ee307034 |
feat(server): expose the active trip as its own endpoint
GET /api/trips/active answers with the trip a user means by "my trip" right now: the one running today, else the next one starting, else the most recently started. Archived trips never qualify. That order is the dashboard hero's (sortTrips), on purpose - the startup redirect this is for has to land on the same trip the dashboard features, or the two would disagree about which trip is yours. Kept apart from list() because it runs on a first paint: four columns of one row instead of every trip with its per-trip day and place counts. |
||
|
Maurice
|
aca6c556ed |
test(server): cover the Synology provider
Completes the pass over the providers that moved into the gated tree. The session handling is the part that most needed it: Synology invalidates a SID on timeout (106), on a duplicate login (107) and when it does not recognise it (119), and the service is supposed to clear it, re-login once and repeat the call. That retry had no case at all — a regression there would have looked like "Synology randomly stops working" and nothing would have failed. Also pinned: a SID that no longer decrypts after a key rotation triggers a fresh login rather than a dead session; a stored device id rides along so a trusted device skips OTP; an app-level error that is not a session code is NOT retried; the album listing merges personal, shared and shared-with-me, survives a partial source failure, and collapses an id that appears in two sources; and an SSRF block stays a 400 with the guard's own message while any other transport failure is a 500. |
||
|
Maurice
|
c0d90d57b2 |
refactor(server): fold the memories domain into providers
The seven files in services/memories/ become injectable services. Every SQL statement, error string, status code and header moves verbatim; what changes is who owns what. photoResolverService was two files in one, so it stayed two: the trek_photos rows are a repository under photos/, the provider dispatch is PhotoResolverService under memories/. That is the photos/ vs memories/ boundary — storage there, dispatch here — and it is also why the module graph is a straight line: TrekPhotosModule is a leaf both halves import, so neither needs a forwardRef. pipeAsset and the ServiceResult envelope stay free functions over the Express response. pipeAsset is deliberately not a StreamableFile: it forwards the upstream 206 and Content-Range so a <video> can seek (#823), swaps Cache-Control to no-store on an error status, and answers differently once headers are sent. The trek-photo cache keeps its in-flight map at module scope. That map is the stampede guard, and the scheduler builds its own instance through a lazy require outside the container — a per-instance map would give them separate guards and ten tiles would fetch the same asset ten times, silently. journeyService's dynamic import() of synologyService is gone: it existed only to duck the import cycle, which no longer exists. MemoriesModule imports RealtimeModule explicitly even though it is @Global, so a single-domain e2e TestingModule can still resolve the broadcast. The providers arrived at 60% branch coverage or less, because services/memories/ sits outside the gate and the integration suites drive the domain over HTTP without reaching the failure paths. New suites cover what had no case at all: the access checks that decide who may fetch someone else's photo, the stampede guard, the thumbnail mtime check, the poster-less-video 404, and Immich end to end with safeFetch stubbed — including the hidden-asset filter older servers depend on (#1474) and the http -> https canonical detection that must NOT rewrite a changed host. |
||
|
Maurice
|
5b6c79299f |
docs(server): record what the coverage gate does and does not measure
PR #1844 went red with every suite green: the diff moved MCP tools from src/mcp/, which the gate ignores, into src/nest/**, which it measures. That did not change any behaviour - it revealed that airports.mcp.ts had never had a test in either world, and dragged the aggregate to 79.86%. Worth stating in the gotcha list, because journey.ts and _shared.ts still have to make the same move. |
||
|
Maurice
|
67c1633155 |
refactor(server): lift the memories vocabulary and the uploads anchor into nest
Two preparations the provider conversion needs, and neither changes behaviour. The ServiceResult envelope, the provider-agnostic asset shapes and pipeAsset move to nest/memories/memories.helpers.ts, which touches no database. The controllers and the provider services can name them without importing each other - the same reason notifications/notification-events.ts exists. What is left in services/memories/helpersService.ts is the DB-backed half (the access checks, the album-link lookups) and it folds with the providers. pipeAsset stays a free function over the Express response on purpose. It is not a StreamableFile candidate: it forwards the upstream's 206 and Content-Range so a <video> can seek (#823), swaps Cache-Control to no-store on an error status, and has to answer differently once headers are already sent. All three controllers take @Res() anyway. The three copies of `path.join(__dirname, '../../../uploads')` become one UPLOADS_ROOT. __dirname moves with the file and none of these would have thrown on a wrong depth - mkdirSync and multer create the wrong tree without a word, and every stored photo lands somewhere nobody serves from. UPLOADS-ROOT-001 pins it. The depth happens to be identical for nest/memories, so nothing moves today; the point is that the next move only has one place to get wrong. |
||
|
Maurice
|
aa4ed69677 |
refactor(server): give trek_photos its own repository
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. |
||
|
Maurice
|
7b666736df |
test(server): cover the MCP surfaces that moved into the gated tree
The coverage gate reads src/nest/** only, so the tools that lived in src/mcp/
were never measured. Moving them exposed what was never covered:
- airports.mcp.ts sat at 0% branches - search_airports and get_airport had no
test at all, in either world
- the two weather catch blocks, which turn a provider failure into an isError
result instead of a transport-level error
- the transport guards: trip access, day ids belonging to another trip, the
not-a-transport-type check on update, and delete_transport in full
- the price path, where a transport with a price links a budget item and
broadcasts budget:created
None of this is new behaviour; it is behaviour that shipped untested because it
sat outside the measured tree.
One case documents a subtlety worth keeping: the weather fallback message only
fires for a rejection with no .message at all. `new Error('')` still carries an
empty one and passes straight through, so the case rejects with a non-Error.
|
||
|
Maurice
|
3efedd5e48 |
refactor(server): decorate the last three MCP prompts
The prompts registrar is gone. Each prompt moves to the domain that owns its data instead of a shared file that had to import three bridges to serve them. packing-list goes to PackingMcp and budget-overview to BudgetMcp, both reusing the `when` addon gate their tools already carry — so a disabled addon keeps the prompt out of the listing, exactly as the `if (isAddonEnabled(...))` around the registration call did. Both keep taking the trip title through trips.bridge: TripsModule imports PackingModule and BudgetModule, so injecting TripsService would close a cycle. Same documented in-container exception as atlas.mcp.ts, and the registrar reached for it the same way. token_auth_notice goes to a new AuthMcp — it is a statement about how the session authenticated, not about trip data, and it injects nothing because the message is static. Its `if (isStaticToken)` guard becomes `when: (ctx) => ctx.isStaticToken`; the registry hands `when` the session context, so an OAuth 2.1 session never sees the prompt listed at all, which is what the registrar achieved by skipping registration. src/mcp/tools/ is down to _shared.ts and journey.ts. journey stays until services/journeyService folds, which waits on memories. |
||
|
Maurice
|
cacab0795f |
refactor(server): decorate the weather, airport and transport MCP tools
Three of the four remaining hand-written registrars are gone. The tools keep their names, descriptions, input schemas, annotations and result shapes; the registration-time scope gates (`if (canWeather)`, `if (canGeo)`, `if (!canWrite(scopes, 'reservations')) return`) become declarative access markers, resolved by trekMcpAccessPolicy like every other decorated domain. get_weather / get_detailed_weather move into a new WeatherMcp over the injected WeatherService, including the catch that answers with the upstream message and isError rather than throwing - a client sees "Weather service not available." instead of a transport failure when the provider is down. search_airports / get_airport move into a new AirportsMcp with no injected service at all: the airport table is a static dataset. AirportsService owns the flight-endpoint backfill against the database, which neither tool touches. create/update/delete_transport move into ReservationsMcp rather than a domain of their own, because a transport IS a reservation - same table, same service, same reservation_edit permission - and that class already injects everything they needed from the four bridges the registrar was importing. That was budget.bridge's last outside-container consumer, so linkBudgetItemToReservation is pruned with it; the file is down to one export and dies as soon as BudgetMcp stops needing AuthService. journey.ts stays: it still reaches into services/journeyService, which cannot fold until memories does. |
||
|
Maurice
|
790dd3b728 |
refactor(server): break the memories import cycle
immich -> unified -> photoResolver -> immich was a real cycle, every edge a
value import; journeyService only got past it with a dynamic import(). Nothing
in that cluster can become a provider while it stands, because Nest would need a
forwardRef through three modules.
It had exactly one cause: syncAlbumAssets and syncSynologyAlbumLink called
addTripPhotos, which lives in unifiedService. Both functions are two things
stapled together - fetch the album from the provider, then write it to the trip
- and only the first half is provider-specific.
So the providers keep the half they own (collectAlbumSelection,
collectSynologyAlbumSelection) and unifiedService keeps the half it owns
(syncImmichAlbum, syncSynologyAlbum). Every error string, status code and return
shape is the one the combined function produced, including the per-provider
wording the split had to preserve ('Could not reach Immich' stays in immich).
The plan called for pulling addTripPhotos and getOrCreateTrekPhoto into one
neutral module instead. Together they do not work as a pair: such a module still
needs immich and synology to fetch, so the cycle only moves.
There is a second cut that works - photoResolverService is really two files, and
its trek_photos CRUD half touches no provider, so splitting that out also opens
both loops with less churn. It is not the one taken here because it leaves
immich -> unified standing, i.e. a provider depending on the trip aggregate.
That direction is what tied the knot in the first place. The CRUD split is still
worth doing, but as the photos/ vs memories/ module boundary, not as the cycle
fix.
server/src/services/memories/ is a DAG now: helpers <- immich/synology <-
photoResolver <- unified.
|
||
|
Maurice
|
97f524cdfd |
feat(server): give the memories endpoints their Zod contracts
The last nine memories entries in the body-contract allow list are gone: three each on the immich, synology and unified controllers. They read @Body() as Record<string, unknown> and coerced by hand. The schemas are deliberately permissive and loose, and that is the point rather than a shortcut. Paging arrives as `2` or `'2'` because the controller does `Number(page) || 1`; synology_skip_ssl arrives as a boolean or the string 'true'; auto_upload is accepted in any shape because only a real boolean is acted on. A schema that described the ideal body instead of the real one would turn working clients into 400s, and parity outranks tightening. Narrowing them is a separate, client-visible change. Loose objects for the same reason: a strict object strips unknown keys, and `selections[]` goes straight to the services as `Selection[]` - stripping there would be a silent data loss the day a provider adds a field. This unblocks be-memories-fold, which could not move a controller whose body had no contract to move with it. |
||
|
Maurice
|
ac96644508 |
refactor(server): fold the notifications domain out of services/
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. |
||
|
Maurice
|
712a33626a |
refactor(server): give the notification vocabulary its own module
The event names, the channel ids and the ExternalChannel contract move to nest/notifications/notification-events.ts, which imports nothing. The cluster only looked acyclic: notifications.ts and channelRegistry.ts both reached back into notificationPreferencesService for NotifEventType, and both edges were `import type`, so they compiled away. Putting any of those names in a value position - an enum, a const array, an injected class - would have made the cycle real, with builtins.ts as a third ring. Nothing can fold while that is true. The drift guard against the shared NotificationEventKey moves with the union it guards; it sat three files away from it. |
||
|
Maurice
|
03493a2edd |
refactor(server): fold webauthn, account erasure, oauth and the wiki out of services/ (#1840)
* 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. |
||
|
Maurice
|
5eab8128bd |
refactor(server): fold the ephemeral-token store and the in-app action registry (#1836)
Two small leftovers, moved to the domains that own them. ephemeralTokens holds the short-lived tokens the WebSocket handshake and the file-download links use. It already had startTokenCleanup/stopTokenCleanup with an unref-ed handle — the right shape. What was missing is anyone calling stop: index.ts kicked the sweep off with a bare require() after listen() and nothing ever ended it. AuthService owns that lifecycle now, through onModuleInit/onModuleDestroy, which is where it belongs — it is the service that issues the tokens. The store itself stays module state. websocket.ts consumes tokens from outside the container, and a token store is only useful if the issuer and the consumer share one. Same reasoning as the weather cache in #1835. inAppNotificationActions is a 22-line handler registry with exactly one consumer; it moves next to it as notifications/in-app-actions.ts. services/ is down to 9 files, from 44. |
||
|
Maurice
|
a7d789fdd5 |
refactor(server): move weather out of services/, give its cache sweep a lifecycle (#1835)
Finishes be-weather-airports-fold. The README has listed weather as the reference implementation since the migration started, while 513 lines of it sat in the legacy layer. The file is nest/weather/weather.impl.ts now, and WeatherService owns what it could not own before: the cache sweep. That sweep was a bare setInterval at module scope, so every process that so much as imported the file started a five-minute timer and nothing ever stopped it — there was nothing to call. It is startCacheCleanup/stopCacheCleanup now, driven by onModuleInit/onModuleDestroy, and the handle is unref-ed so a cache sweep can never hold the process open. The old test file says as much: its second line is "Prevent the module-level setInterval from running during tests", with a vi.useFakeTimers() to do it. That workaround is no longer what keeps the timer quiet. The cache itself stays module state, deliberately. The MCP weather tools call getWeather directly from outside the container, and a cache is only worth having if every caller sees the same one. Hoisting it into the class would mean handing the registrar the container instance to get exactly what a module singleton already provides — the same trade the place-photo cache resolved the other way, because there the scheduler needed the instance anyway. Coverage is the reason this PR carries eleven new tests rather than none. Code in services/ is ungated; the moment it lands in src/nest it counts against the 80% branch threshold, and weather.impl arrived at 75%. The gap was in paths worth having: a failing archive lookup with and without an upstream reason, a far-future date whose climate rows are all null, an expired entry being re- fetched, and the sweep evicting one on its own. Plus the lifecycle itself — start is idempotent, stop is idempotent, the handle is unref-ed. 79% now. services/ is down to 11 files, from 44. |
||
|
Maurice
|
9c46f31c57 |
refactor(server): fold airports and distance, drop the boot-time backfill cycle (#1834)
Two thirds of be-weather-airports-fold. weatherService (513 LOC, a process cache and a setInterval at import) is left for its own PR. airportService split along the line that was already there. The dataset lookup touches no database — it reads one JSON file and keeps it in memory — and both MCP registrars call it from outside the container, so findByIata/searchAirports are free functions in nest/airports/airports.data.ts. backfillFlightEndpoints needs the connection and became a method on AirportsService. That backfill was the interesting part. db/database.ts ran it at module eval through a bare require() back into services/, so the module graph closed a cycle that only held because `db` happened to be initialised by the time the require resolved. It runs in onApplicationBootstrap now: it repairs rows, nothing boots on it, and the container is fully built by then. The cycle had a visible cost. tests/setup.console-noise.ts carried a suppression for `Cannot find module '../services/airportService'`, because vitest's transform pipeline could not resolve that require at all — it fired in roughly sixty test files. That suppression is now narrowed to the only case left: an e2e TestingModule whose fixture DB has no reservations table. distanceService split the same way: haversineKm is arithmetic and moved to common/geo.ts; getFlightDistanceKm reads the database and became a private method on AuthService, its only caller. AirportsModule imports DatabaseModule explicitly. @Global only reaches modules that are in the graph, and the airports e2e builds its container from AirportsModule alone — exactly the trap the nest README now documents. The search ranking gets its first direct tests. It sat in services/, outside the coverage gate, and only ever ran incidentally through the route; moving it into src/nest made the gap measurable (16% branches). The ten cases pin the scoring ladder — exact IATA short-circuit, ICAO over prefix, IATA prefix over city prefix, city prefix over substring — and the missing-dataset fallback, which no test had reached. 90% branches now. |
||
|
Maurice
|
c7fed4b1f8 |
refactor(server): make the place-photo cache a provider (#1833)
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. |
||
|
Maurice
|
4e6b45016a |
refactor(server): move the KML importer and place-image helpers into places/ (#1832)
Two of the three files in be-places-helpers-fold. placePhotoCache is left for its own PR — it holds three module-level caches and its sweep hangs off the cron, so it needs both a provider with real state and a second entry in the scheduler contract. kmlImport.ts is 212 lines with no state, no database and no I/O — parsing, sanitising and coordinate arithmetic. It is nest/places/kml-import.helpers.ts now, unchanged, next to the places.helpers.ts that already consumes it. placeImage.ts moves to nest/places/place-image.ts as free functions. Making reclaimPlaceImage a provider would have split it from PLACE_IMAGES_DIR, which cannot be injected: nest/common/place-image-upload.ts reads it at module scope to configure multer's storage, before any container exists. That constant is the one thing here that could have broken quietly: path.resolve(__dirname, '../../uploads/places') __dirname moves with the file. From src/services that resolved to <server>/uploads/places; from src/nest/places the same string would have resolved to <server>/src/uploads/places — a directory multer would have happily created, leaving every uploaded place image in the wrong place and every existing one unreachable. The path is now '../../../uploads/places', verified to resolve to the identical absolute directory, and uploads-dirs.test.ts covers it. |
||
|
Maurice
|
90e35e6970 |
refactor(server): fold llmConfig and unsplash out of services/ (#1831)
Two of the three leftovers in be-misc-fold. The third (wikiService) needs a Zod contract in @trek/shared first and is left for its own PR. llmConfig.ts was pure movement: 70 lines of encryption-aware config shaping with no state and no database, sitting in services/ while all four of its callers are in src/nest. It is nest/llm-parse/llm-config.ts now, same functions, same file. unsplashService.ts needed an actual provider. Its key resolution reads both the env and the users table (env var, then the caller's stored key, then any admin's), so the injected connection was unavoidable. Its own module because trips and places both reach it, and because folding it into either would have made the other import a trip/place service for an image search. The four exports became methods on UnsplashService. The SSRF guard in saveUnsplashCover is untouched — it still refuses any host but the Unsplash image CDN and still goes through safeFetch — as is the unauthenticated-endpoint fallback when no key resolves (#1449). The env read stays live through RuntimeEnvService rather than a registerAs token. readEnv() is what the function did before, and a boot-frozen token would change when an operator's UNSPLASH_ACCESS_KEY takes effect. Two test seams follow the code: the unsplash unit tests bind the four methods so every existing assertion reads unchanged, and the trips controller's path mock of the module becomes an injected stub. services/ is down to 17 top-level files. |
||
|
Maurice
|
7dea88120b |
docs(server): bring the nest README up to the foundation work (#1830)
The README is the binding spec, and seven PRs have landed patterns it does not mention. Someone reading it today would rebuild things that now exist, and would walk into two traps that cost real time this week. Added to the foundation section: common/ and why those helpers are free functions, auth/jwt-verify.ts and why JWT_SECRET must not move to app-config, query-helpers/, trip-membership/ and the cycle that put it in its own module, AddonGuard + @RequireAddon with its ordering requirement, and demo-write. New subsection on reaching the container from outside it. The choice is settled now: hand the dependency in, declared structurally, the way index.ts fills the scheduler's SchedulerDeps. Bridges stay correct for the MCP registrars, but the README should say what they cost — a bridge rebuilds its services with `new`, so one added constructor parameter meant editing five bridge files in a single PR. Two gotchas that are cheap to read and expensive to rediscover: Never gate a multipart upload with a guard. Guards run before the parser, so throwing leaves the body unread and Node resets the connection — the client gets ECONNRESET rather than the 403. @Global only applies to modules that are in the graph. An e2e TestingModule built around one domain module does not get AppConfigModule for free, and a provider injecting RuntimeEnvService takes every suite importing that module down with it. Also corrects an assumption the migration plan makes and this README implied: the trip-access and canEdit methods on the domain services are not dead weight waiting for TripAccessGuard. Across the five domains piloted for it, 40 of the 46 callers are *.mcp.ts tools, which never pass through an HTTP guard. Only 6 sit in controllers. Counted, not estimated. |
||
|
Maurice
|
22d178d2f9 |
refactor(server): one addon guard, one demo-mode condition (#1829)
Two pieces of copy-paste in the guard layer. Three addon guards — collections, airtrail, journey — did the same thing and differed only in the addon id they read and the word leading the 404. They are one AddonGuard plus a @RequireAddon(addonId, label) decorator now, read through the Reflector. A handler-level decorator overrides the controller's, and a route without the decorator is not gated at all. The demo-mode upload block was copy-pasted into six handlers, each carrying its own copy of the message. All six call isDemoWriteBlocked now, against the same DEMO_WRITE_ERROR body. That one is deliberately NOT a guard, though all six sites are handlers and a guard is the obvious shape — and this PR did build one first. Guards run before the multipart parser, so throwing from one leaves the request body unread and Node resets the connection: the client gets ECONNRESET instead of the 403. PROFILE-015 caught it. The check has to sit inside the handler, after the interceptor has drained the upload, for the response to arrive at all. The function carries that reasoning so the next person does not rediscover it. Two smaller findings on the way there. The DemoWriteGuard attempt broke eight e2e suites that never touch demo mode: AuthModule listed it as a provider, and RuntimeEnvService was not resolvable inside a TestingModule built around one domain module. AppConfigModule is @Global, but @Global only applies to modules that are in the graph. The five modules whose controllers now inject RuntimeEnvService import it explicitly, the same way PermissionsModule is handled. And both new guards get a test asserting the decorators are actually applied. Once a check moves out of a handler, that handler's own tests stop noticing if the decorator is dropped — the assertions on __guards__ and the @RequireAddon metadata are what keeps the four upload endpoints and the four addon route groups gated, and gated on the right addon. |
||
|
Maurice
|
9814480a96 |
refactor(server): move the JWT verify into nest/auth, delete four dead middlewares (#1828)
middleware/auth.ts exported seven things. Two of them — extractToken and verifyJwtAndLoadUser — are the canonical session check every auth surface goes through. The other five (authenticate, requireCookieAuth, optionalAuth, adminOnly, demoUploadBlock) are Express middlewares with no production caller left; the guards in nest/auth replaced them and only comments still mention them. The two live functions move to nest/auth/jwt-verify.ts and the file goes, along with middleware/validate.ts (nothing but its own test imported it) and middleware/idempotency.ts (its only caller was `authenticate`, and IdempotencyInterceptor has replaced it). AuthRequest and OptionalAuthRequest go with them — src/typings/express.d.ts is what types req.user now. Eight importers repoint: the three guards, auth/files/plugins-proxy services, platform.routes and the global MFA policy. Two deliberate departures from the plan for this package. JWT_SECRET does not move to app-config. It is not an env value: it is generated into data/.jwt_secret, and the admin panel rotates it at runtime through updateJwtSecret. `export let` is precisely what makes a rotation take effect in-process — a registerAs token would freeze the boot value and silently break rotation. The verify also keeps reading the db singleton rather than DatabaseService. That change is not a one-line swap: it gives the three guards a constructor dependency, which means registering them as providers everywhere they are used. Measured: 38 directories use those guards and 21 of them do not import AuthModule, on top of 34 e2e tests that each build a container around a single domain module. That is its own PR, not a rider on one that deletes dead files. tests/unit/middleware/auth.test.ts covered real behaviour, so it is not simply deleted. Its extractToken cases and the five verify cases it drove through `authenticate` come back as tests/unit/nest/auth/jwt-verify.test.ts, now hitting verifyJwtAndLoadUser directly. Four cases are new: the password_version gate — the whole reason this function exists — had no direct coverage at all. Its three adminOnly cases are dropped because AdminGuard, their successor, is covered by three e2e suites. |
||
|
Maurice
|
a634b2a26b |
feat(server): give the scheduler a contract with the container (#1827)
The scheduler runs outside the Nest container but needs four things inside it: backupService, trekPhotoCache, placePhotoCache and airtrailSync. It reaches all four with a require() or await import() at call time. This settles how that should work before four separate folds each invent their own answer, and wires one consumer as proof. It takes the dependencies from the container rather than growing four more bridges, for a reason that showed up while writing the previous PRs: adding one constructor parameter to a service meant hand-editing five bridge files that rebuild their services with `new`. app.get() has no such cost, and hands back the container singleton instead of a second instance. The scheduler declares what it needs structurally — a `backups` with a createBackup — and never names BackupService. That is not fastidiousness: an import of the class would pull in src/nest/backup → src/services/backupService → src/scheduler, which is the exact cycle the lazy `await import()` in runBackup existed to dodge. With the dependency handed in, the scheduler stops importing backupService entirely and the cycle is gone; backupService still imports the scheduler for VALID_INTERVALS, but now only in one direction. index.ts fills the contract from the app buildApp() already initialised, right before the first cron starts, the way bootstrap.ts hands the /mcp handler its registry. Two smaller things fall out. The Nest BackupService was swallowing createBackup's prefix parameter, so it could only ever produce 'backup' names; it forwards it now. And the scheduler can be started without a container at all — its unit tests do exactly that — so runBackup says so in the log instead of throwing, with a case covering it. The other three consumers stay on their require() calls; they move with their own folds. |
||
|
Maurice
|
939452f9d8 |
refactor(server): consolidate trip access and fold the query helpers into the container (#1824)
* 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.
|
||
|
Maurice
|
6dda05f672 |
refactor(server): move the stateless helpers into nest/common (#1823)
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. |
||
|
Maurice
|
cf62567c48 |
fix(map): stop untrusted values reaching the marker HTML (#1820)
* fix(map): drop the casing layer before its source on teardown
Tearing down the reservation overlay logged an error on every map unmount:
Source "trek-reservations" cannot be removed while layer
"trek-reservations-lines-transit-casing" is using it.
The casing layer arrived with the transit paths in 3.2.0 but was never added to
destroy(), which only removed the main line layer. The try/catch around it gave
no protection either — the engine reports this by firing an error event rather
than throwing — so the source was silently left behind on the style.
The layer id is a named constant now, so the place that creates it and the place
that removes it can no longer drift apart.
The lifecycle test grew the assertion that would have caught it: the fake map
now refuses to remove a source that a layer still references, the same way the
real one does. Verified against the unfixed code — two tests fail without the
change.
* fix(map): stop untrusted values reaching the marker HTML
Both map renderers build their marker HTML as a string and hand it to
L.divIcon / innerHTML. Two of the interpolations carry values a user controls,
and neither was escaped.
place.image_url is the worse of the two, because it is not admin-only: any trip
member with edit rights can set it. placeUpdateRequestSchema is a plain record
of unknown values, and the length guard in places.controller covers name,
description, address and notes but not image_url. The renderers do check the
value, but only with startsWith('/uploads/') — a payload like
/uploads/x" onerror="…" y="
satisfies that and then breaks out of the src="…" attribute.
category.color is the same shape one line up, reachable by an admin. It also
reaches SharedTripPage, which builds its own divIcon and answers without a
guard, so that payload lands in front of people who have no account here.
Colours are allow-listed rather than escaped. Escaping stops the attribute
breakout but leaves a working CSS value behind, and `url(https://…)` in a
background is a tracking pixel. Anything that is not #rgb/#rrggbb falls back.
image_url is a URL, not an enum, so that one is escaped; the prefix check stays.
Server side, the category contract now demands a hex colour, and the endpoints
validate through DTO classes instead of @Body('color') — a property read carries
no metatype, so the global pipe never saw it. That retires two entries from the
body-contract allow list. It replaces the hand-rolled 400 for a missing name
with the pipe's envelope, the same trade the places and trips migrations made.
The response schema deliberately still accepts any string: the write path was
open for a long time, so tightening it would make an instance fail on its own
stored rows. The client allow-list is what covers those.
Verified against the unpatched renderer — the three new cases fail without it.
* perf(client): serialize the map icons without react-dom/server
Six modules imported react-dom/server statically to turn a lucide icon into an
SVG string. That put Fizz — roughly 190 kB raw, 57 kB gzip — into a chunk three
lazy routes share, the Leaflet renderer among them, which is the default. For
output that is always a single <svg> with a flat list of shape children.
mapViewport chunk 189,532 -> 3,710 B
Tooltip chunk 194,333 -> 1,970 + 4,860 B
precache total 17,856 -> 17,675 KiB
utils/iconMarkup.ts serializes React elements directly. It is generic over
elements rather than over lucide's icon tables on purpose: iconNode is closed
over inside createLucideIcon and never exported, so calling the forwardRef
render function is what survives a lucide upgrade.
Byte-compatibility is checked, not asserted. iconMarkup.parity.test.ts diffs the
output against real Fizz for every exported icon in three prop shapes — the test
file is the one place still allowed to import react-dom/server, and it never
reaches a bundle. Two traps it covers: viewBox must not be kebab-cased into
view-box, and Fizz emits <path …></path> rather than a self-closing tag.
TripPDF loses its lazy renderer entirely — ensureRenderer, the module-level
handle and the two "not ready yet" guards that returned an empty string. There
is nothing left to load.
An eslint rule keeps the import from creeping back. Without it the regression is
invisible: the build stays green, the app keeps working, and the chunk quietly
grows by 190 kB again.
|