mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-10-18 21:16:47 +00:00
Make a bind failure fatal again, and undo four contract tightenings
An adversarial audit over the three merged migration PRs. Sixteen findings
survived triple refutation; these are the ones with a defect behind them.
The serious one is mine, from the gateway move. ws registers
`error: this.emit.bind(this, 'error')` on the http server it attaches to, and
the adapter now runs inside buildApp, before listen(). So from that moment the
http server HAS an error listener and Node stops throwing on one. The adapter's
handler was `() => {}`, which meant EADDRINUSE was forwarded there and dropped:
the process survived, the listen callback never ran, so the banner never
printed and the scheduler never started. A container that looks healthy and
serves nothing. Reproduced, then fixed at both ends: index.ts owns the fatal
decision on the http server, and the adapter logs instead of swallowing.
Four DTOs were tightening contracts rather than describing them, which is the
thing the pipe-before-handler ordering makes easy to do by accident:
- OauthClientCreateDto replaced 'Name is required' with a Zod field report, and
pre-empted the handler's 403 on an instance with the MCP addon off.
- BookingImportConfirmDto demanded per-item fields the service never reads, so
a caller assembling items itself got a 400 about `source`.
- TagCreateDto/TagUpdateDto typed name and color as strings; the legacy route
bound whatever arrived, so a numeric name used to create a tag.
- PluginUserSettingsUpdateDto typed config as a record, where the handler
answers 200 for anything it cannot use.
Two of my own tests were false-green, which the audit demonstrated by mutation
rather than by reading. WSGW-008 asserted on frames that never carry the user
object at all, so it passed with the password_version strip deleted; it reads
the retained identity now. GEO-009 made the same assertion before and after the
write; it distinguishes a cached negative from a miss now, through a hasCached
seam that says what getCached cannot. Both were re-run against the mutation
they exist to catch, and both fail on it.
booking-import geocodes every venue and endpoint in one request loop, up to
thirty sequential calls. Folding maps onto the shared Nominatim cursor put that
loop on the interactive lane, so an import starved every concurrent keystroke
for half a minute. It takes the background lane now, which is what the two-lane
split was built for.
Also: the README's in-container bridge list had outlived one entry and was
missing another, and the journey e2e still mocked a module this branch deleted.
This commit is contained in:
@@ -104,6 +104,20 @@ async function bootstrap(): Promise<void> {
|
||||
// would serve the REST API fine and leave the gateway attached to a socket
|
||||
// nobody listens on.
|
||||
server = getHttpServer();
|
||||
|
||||
// A bind failure has to be fatal and loud, and it needs saying explicitly.
|
||||
// listen() reports failure by event, not by rejecting, so the catch around
|
||||
// bootstrap() never sees it. And since buildApp attaches the ws server to this
|
||||
// http server, ws has already registered its own `error` listener on it, which
|
||||
// is enough for Node to stop throwing on an unhandled one. Without this the
|
||||
// process would survive EADDRINUSE, never run onListen, and serve nothing
|
||||
// while looking healthy.
|
||||
server.on('error', (err: NodeJS.ErrnoException) => {
|
||||
const where = HOST ? `${HOST}:${PORT}` : `:${PORT}`;
|
||||
console.error(`Fatal: cannot listen on ${where} — ${err.code ?? ''} ${err.message}`);
|
||||
process.exit(1);
|
||||
});
|
||||
|
||||
if (HOST) server.listen(PORT, HOST, onListen);
|
||||
else server.listen(PORT, onListen);
|
||||
}
|
||||
|
||||
@@ -243,8 +243,9 @@ through. `@trek/nest-mcp` hands the gate its declaring instance now, and
|
||||
`AddonsService`. `reservations.mcp.ts` (assignments) and `atlas.mcp.ts` /
|
||||
`journey.mcp.ts` (auth) went the same way.
|
||||
|
||||
Three in-container uses are left, each a real module cycle that injection does
|
||||
not fix:
|
||||
Four in-container uses are left, each for a reason that injection does not fix.
|
||||
Keep this list current: an entry that outlives its cause reads as a standing
|
||||
exception and invites the next one.
|
||||
|
||||
- `places.mcp.ts` → `assignments.bridge`: a real cycle, `DaysModule →
|
||||
PlacesModule → AssignmentsModule → DaysModule`.
|
||||
@@ -252,11 +253,20 @@ not fix:
|
||||
`TripsModule` imports both, so importing it back closes the loop.
|
||||
- `auth/user-cleanup.service.ts` → `budget.bridge`: `BudgetModule` imports
|
||||
`AuthModule`, same shape.
|
||||
- `backup/backup.impl.ts` → `permissions.bridge`: free functions, not a
|
||||
provider, so there is nothing for an injected service to hang off. Its second
|
||||
caller, `src/scheduler.ts`, is gone; the cache invalidation is not.
|
||||
|
||||
(A fourth — `files.controller.ts` / `journey.controller.ts` → `files.bridge` for
|
||||
multer's module-scope interceptor options — was resolved 2026-08-10 exactly as
|
||||
`files.bridge` used to be the fifth and is gone, resolved 2026-08-10 exactly as
|
||||
predicted: `MulterModule.registerAsync` over an `AllowedFileTypesService` leaf,
|
||||
and `files.bridge.ts` is deleted.)
|
||||
and `files.controller.ts` / `journey.controller.ts` build their multer options
|
||||
from that.
|
||||
|
||||
**Four bridges have no production consumer at all** — `days`, `packing`,
|
||||
`photos`, `reservations`. Only their own delegation tests import them, and each
|
||||
still builds a service outside the container at module load for nobody. They are
|
||||
deletable, along with the comments in `days.service.ts`, `packing.service.ts` and
|
||||
`reservations.service.ts` that still send readers to them.
|
||||
|
||||
The nine fire-and-forget notification senders inject too. They were lazy
|
||||
`import('../notifications/notifications.bridge').then(({ send }) => …)` calls
|
||||
|
||||
@@ -153,7 +153,7 @@ export class BookingImportService {
|
||||
].filter((q): q is string => !!q);
|
||||
|
||||
for (const q of queries) {
|
||||
const results = await this.maps.searchNominatim(q);
|
||||
const results = await this.maps.searchNominatim(q, undefined, 'background');
|
||||
const hit = results[0];
|
||||
if (hit?.lat != null && hit?.lng != null) {
|
||||
lat = hit.lat;
|
||||
@@ -185,7 +185,7 @@ export class BookingImportService {
|
||||
for (const ep of reservationData.endpoints) {
|
||||
if ((ep.lat == null || ep.lng == null) && ep.name) {
|
||||
try {
|
||||
const hit = (await this.maps.searchNominatim(ep.name))[0];
|
||||
const hit = (await this.maps.searchNominatim(ep.name, undefined, 'background'))[0];
|
||||
if (hit?.lat != null && hit?.lng != null) {
|
||||
ep.lat = hit.lat;
|
||||
ep.lng = hit.lng;
|
||||
|
||||
@@ -99,6 +99,18 @@ export function getCached<T>(key: string): T | undefined {
|
||||
return cache.has(key) ? (cache.get(key) as T) : undefined;
|
||||
}
|
||||
|
||||
/**
|
||||
* Whether the key is present at all, negative answers included.
|
||||
*
|
||||
* `getCached` cannot express the difference: a cached `undefined` and a miss
|
||||
* both read as `undefined`. Callers that only want the value do not care, but a
|
||||
* caller deciding whether to go back to the network does, and so does any test
|
||||
* asserting that a negative answer is retained.
|
||||
*/
|
||||
export function hasCached(key: string): boolean {
|
||||
return cache.has(key);
|
||||
}
|
||||
|
||||
export function setCached(key: string, value: unknown): void {
|
||||
cache.set(key, value);
|
||||
}
|
||||
|
||||
@@ -13,7 +13,7 @@ import { decrypt_api_key } from '../common/crypto/apiKeyCrypto';
|
||||
// ── Photo cache (disk-backed) ────────────────────────────────────────────────
|
||||
import { PlacePhotoCacheService } from '../place-photos/place-photo-cache.service';
|
||||
import { DatabaseService } from '../database/database.service';
|
||||
import { nominatimFetch } from '../geo/nominatim.client';
|
||||
import { nominatimFetch, type GeoLane } from '../geo/nominatim.client';
|
||||
import {
|
||||
UA,
|
||||
toApiLang,
|
||||
@@ -540,7 +540,17 @@ export class MapsService {
|
||||
|
||||
// ── Nominatim search ───────────────────────────────────────────────────────
|
||||
|
||||
async searchNominatim(query: string, lang?: string) {
|
||||
/**
|
||||
* `lane` defaults to interactive because most callers are a keystroke.
|
||||
*
|
||||
* Bulk callers must pass 'background': booking-import geocodes every venue and
|
||||
* every uncoordinated endpoint of an import in one request loop, which is up
|
||||
* to thirty sequential calls. On the interactive lane those thirty take the
|
||||
* next slot each time, so somebody typing in the place search waits behind the
|
||||
* whole import. Yielding does not make the import faster, it stops it from
|
||||
* being the only thing the process will do for half a minute.
|
||||
*/
|
||||
async searchNominatim(query: string, lang?: string, lane: GeoLane = 'interactive') {
|
||||
const params = new URLSearchParams({
|
||||
q: query,
|
||||
format: 'json',
|
||||
@@ -553,9 +563,8 @@ export class MapsService {
|
||||
limit: '10',
|
||||
'accept-language': toApiLang(lang),
|
||||
});
|
||||
// Through the shared client: one throttle for the whole process, and the
|
||||
// interactive lane so a keystroke does not queue behind the atlas backfill.
|
||||
const response = await nominatimFetch('search', params);
|
||||
// Through the shared client: one throttle for the whole process.
|
||||
const response = await nominatimFetch('search', params, { lane });
|
||||
if (!response.ok) {
|
||||
const text = await response.text().catch(() => '');
|
||||
throw new Error(
|
||||
|
||||
@@ -1,17 +1,31 @@
|
||||
import { createZodDto } from 'nestjs-zod';
|
||||
import { oauthClientCreateRequestSchema, oauthConsentRequestSchema } from '@trek/shared';
|
||||
import { z } from 'zod';
|
||||
import { oauthConsentRequestSchema } from '@trek/shared';
|
||||
|
||||
/**
|
||||
* Server-side createZodDto wrappers over the @trek/shared OAuth contracts.
|
||||
* Server-side createZodDto wrappers for the OAuth bodies.
|
||||
*
|
||||
* Both schemas already described these two bodies exactly; they were written
|
||||
* with the contracts and then never wired to the handlers, which declared the
|
||||
* same shape inline as an anonymous type. The inline types are gone now, so the
|
||||
* wire shape has one definition instead of two that could drift.
|
||||
* Consent takes the shared schema unchanged: every field it requires is one the
|
||||
* handler dereferences immediately (redirect_uri is parsed as a URL, the PKCE
|
||||
* pair goes straight into the stored code), so a body missing them had no
|
||||
* meaningful old behaviour to preserve.
|
||||
*
|
||||
* Neither handler owns a bespoke required-field message: consent redirects with
|
||||
* `error=access_denied` on a rejection, and client creation surfaces whatever
|
||||
* the service returns. So nothing user-visible moves in front of the pipe here.
|
||||
* Client creation does NOT, and the difference is deliberate. The shared schema
|
||||
* declares `name: z.string().min(1)` and `allowed_scopes` as required, but the
|
||||
* handler answers 403 `{ error: 'MCP is not enabled' }` before it looks at the
|
||||
* body at all, and the service owns `Name is required` and
|
||||
* `At least one redirect URI is required`. The pipe runs first, so adopting the
|
||||
* schema verbatim turned a 403 into a 400 on an instance with the addon off, and
|
||||
* replaced two error strings a client shows the user. Describing the shape
|
||||
* without demanding it keeps every one of those answers where it was.
|
||||
*/
|
||||
export class OauthConsentDto extends createZodDto(oauthConsentRequestSchema) {}
|
||||
export class OauthClientCreateDto extends createZodDto(oauthClientCreateRequestSchema) {}
|
||||
|
||||
export class OauthClientCreateDto extends createZodDto(
|
||||
z.looseObject({
|
||||
name: z.string().optional(),
|
||||
redirect_uris: z.array(z.string()).optional(),
|
||||
allowed_scopes: z.array(z.string()).optional(),
|
||||
allows_client_credentials: z.boolean().optional(),
|
||||
}),
|
||||
) {}
|
||||
|
||||
@@ -80,7 +80,7 @@ export class PluginUserSettingsController {
|
||||
): { config: Record<string, unknown> } {
|
||||
const userId = req.user?.id;
|
||||
if (!pluginsEnabled() || userId == null || !this.activeWithUserFields(id)) return { config: {} };
|
||||
const patch = body?.config && typeof body.config === 'object' ? body.config : {};
|
||||
const patch = body?.config && typeof body.config === 'object' ? (body.config as Record<string, unknown>) : {};
|
||||
return { config: this.plugins.updateUserConfig(id, userId, patch) };
|
||||
}
|
||||
}
|
||||
|
||||
@@ -59,8 +59,13 @@ export class PluginEgressHostsDto extends createZodDto(
|
||||
*/
|
||||
export class PluginConfigDto extends createZodDto(z.looseObject({})) {}
|
||||
|
||||
/**
|
||||
* `config` is unknown rather than a record: the handler answers 200 with the
|
||||
* stored config for anything it cannot use, including a non-object, and a schema
|
||||
* that rejected first would turn that into a 400.
|
||||
*/
|
||||
export class PluginUserSettingsUpdateDto extends createZodDto(
|
||||
z.looseObject({ config: z.record(z.string(), z.unknown()).optional() }),
|
||||
z.looseObject({ config: z.unknown().optional() }),
|
||||
) {}
|
||||
|
||||
/**
|
||||
|
||||
@@ -6,6 +6,7 @@ import { WebSocketServer } from 'ws';
|
||||
import type { Observable } from 'rxjs';
|
||||
import { readEnv } from '../../app-config';
|
||||
import { setServer, type TrekWebSocket } from './ws-state';
|
||||
import { logError } from '../audit/audit-log.logger';
|
||||
|
||||
// Per-connection message rate limiting. It lives in the adapter, not in the
|
||||
// gateway's handlers, because the original counted EVERY inbound frame before
|
||||
@@ -96,9 +97,16 @@ export class TrekWsAdapter extends WsAdapter {
|
||||
: {}),
|
||||
});
|
||||
|
||||
// A server-level error must not reach the process. The base binds one on the
|
||||
// instances it creates; this one is ours, so it needs its own.
|
||||
server.on('error', () => {});
|
||||
// A server-level error is LOGGED, never swallowed.
|
||||
//
|
||||
// ws hangs its own `error: this.emit.bind(this, 'error')` on the http server
|
||||
// (websocket-server.js), so from the moment this runs the http server has a
|
||||
// listener and Node will not throw on one. Everything it forwards lands
|
||||
// here. An empty handler therefore eats the http server's failures too,
|
||||
// including EADDRINUSE at listen() — the process would stay up, serving
|
||||
// nothing, with no output at all. index.ts owns the fatal decision for bind
|
||||
// failures; this one keeps the transport's own errors visible.
|
||||
server.on('error', (err) => logError(`ws server error: ${err instanceof Error ? err.message : String(err)}`));
|
||||
|
||||
// The fan-out primitives reach the live server through here, and so do the
|
||||
// out-of-container bridges.
|
||||
|
||||
@@ -185,6 +185,8 @@ export class ReservationImportController {
|
||||
throw new HttpException({ error: 'items must be a non-empty array' }, 400);
|
||||
}
|
||||
|
||||
return this.bookingImport.confirm(tripId, items, socketId);
|
||||
// The DTO asserts an array of objects and stops there, so the per-item shape
|
||||
// is still the service's to interpret — as it was before the contract landed.
|
||||
return this.bookingImport.confirm(tripId, items as BookingImportPreviewItem[], socketId);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1,17 +1,18 @@
|
||||
import { createZodDto } from 'nestjs-zod';
|
||||
import { z } from 'zod';
|
||||
import { bookingImportPreviewItemSchema } from '@trek/shared';
|
||||
|
||||
/**
|
||||
* Server-side createZodDto wrappers for the booking-import bodies.
|
||||
*
|
||||
* Confirm validates each item against the shared contract but leaves `items`
|
||||
* itself optional, which is narrower than `bookingImportConfirmRequestSchema`
|
||||
* on purpose. That schema declares `.min(1)`, and the pipe reports a rejection
|
||||
* as `field: message` — so adopting it verbatim would replace the endpoint's
|
||||
* `{ error: 'items must be a non-empty array' }`. The handler keeps the check
|
||||
* that owns the message, and the DTO adds what the handler never did: per-item
|
||||
* shape validation.
|
||||
* Confirm asserts that `items` is an array of objects, and nothing more.
|
||||
*
|
||||
* Two things pushed it there. `bookingImportConfirmRequestSchema` declares
|
||||
* `.min(1)`, and the pipe reports a rejection as `field: message`, so adopting
|
||||
* it verbatim would replace the endpoint's own
|
||||
* `{ error: 'items must be a non-empty array' }`. And validating each item
|
||||
* against `bookingImportPreviewItemSchema` demanded fields the service never
|
||||
* reads, such as `source`: a caller assembling items itself rather than echoing
|
||||
* a preview used to get a 201 and now got a 400 about a field nobody uses.
|
||||
*
|
||||
* Preview and its async twin carry a multipart body, so every field arrives as a
|
||||
* string and the real payload is the files. `mode` is described here for the
|
||||
@@ -21,7 +22,7 @@ import { bookingImportPreviewItemSchema } from '@trek/shared';
|
||||
* adds and the handler ignores.
|
||||
*/
|
||||
export class BookingImportConfirmDto extends createZodDto(
|
||||
z.object({ items: z.array(bookingImportPreviewItemSchema).optional() }),
|
||||
z.object({ items: z.array(z.looseObject({})).optional() }),
|
||||
) {}
|
||||
|
||||
export class BookingImportPreviewDto extends createZodDto(
|
||||
|
||||
@@ -27,7 +27,10 @@ export class TagsController {
|
||||
|
||||
@Post()
|
||||
create(@CurrentUser() user: User, @Body() body: TagCreateDto): { tag: Tag } {
|
||||
const { name, color } = body;
|
||||
// Cast rather than validate: the legacy route bound whatever arrived
|
||||
// straight into SQLite, and a contract that rejected a numeric name here
|
||||
// would be a new 400 for anything already sending one.
|
||||
const { name, color } = body as { name?: string; color?: string };
|
||||
if (!name) {
|
||||
throw new HttpException({ error: 'Tag name is required' }, 400);
|
||||
}
|
||||
@@ -40,7 +43,7 @@ export class TagsController {
|
||||
@Param('id') id: string,
|
||||
@Body() body: TagUpdateDto,
|
||||
): { tag: Tag } {
|
||||
const { name, color } = body;
|
||||
const { name, color } = body as { name?: string; color?: string };
|
||||
if (!this.tags.getByIdAndUser(id, user.id)) {
|
||||
throw new HttpException({ error: 'Tag not found' }, 404);
|
||||
}
|
||||
|
||||
@@ -1,24 +1,25 @@
|
||||
import { createZodDto } from 'nestjs-zod';
|
||||
import { z } from 'zod';
|
||||
import { updateTagRequestSchema } from '@trek/shared';
|
||||
|
||||
/**
|
||||
* Server-side createZodDto wrappers over the @trek/shared tag contracts.
|
||||
*
|
||||
* Create deliberately does NOT use `createTagRequestSchema` as written. That
|
||||
* schema declares `name: z.string().min(1)`, and the pipe reports a rejection as
|
||||
* Neither uses the shared schema as written. `createTagRequestSchema` declares
|
||||
* `name: z.string().min(1)`, and the pipe reports a rejection as
|
||||
* `field: message` — so adopting it verbatim would replace the endpoint's
|
||||
* `{ error: 'Tag name is required' }` with `name: ...`. Two tests pin that
|
||||
* string, and more to the point it is what a client shows the user.
|
||||
*
|
||||
* So the DTO accepts the shape and leaves `name` optional, and the handler keeps
|
||||
* the check that owns the message. The endpoint's actual contract is unchanged:
|
||||
* a request without a name still gets 400, with the same body it always had.
|
||||
* What the DTO does add is type and shape validation on everything else, which
|
||||
* is what took this route off the allow-list.
|
||||
* `{ error: 'Tag name is required' }`, which two tests pin and which is what a
|
||||
* client shows the user. The handler keeps the check that owns the message.
|
||||
*/
|
||||
export class TagCreateDto extends createZodDto(
|
||||
z.object({ name: z.string().optional(), color: z.string().optional() }),
|
||||
) {}
|
||||
/**
|
||||
* Both fields are `unknown`, not `string`.
|
||||
*
|
||||
* The handlers read them off `@Body('name')` / `@Body('color')` and passed
|
||||
* whatever arrived to the service, which binds them into SQLite: a numeric name
|
||||
* created a tag. Typing them as strings turns those requests into a 400 from the
|
||||
* pipe, which is a contract change for anything already sending one. The service
|
||||
* is where the value is interpreted, and it stays there.
|
||||
*/
|
||||
const tagBody = z.looseObject({ name: z.unknown().optional(), color: z.unknown().optional() });
|
||||
|
||||
export class TagUpdateDto extends createZodDto(updateTagRequestSchema) {}
|
||||
export class TagCreateDto extends createZodDto(tagBody) {}
|
||||
export class TagUpdateDto extends createZodDto(tagBody) {}
|
||||
|
||||
@@ -29,7 +29,6 @@ const { isAddonEnabled } = vi.hoisted(() => ({ isAddonEnabled: vi.fn(() => true)
|
||||
// The controller's pure helpers (isVideoExtension/isVideoMime/MAX_VIDEO_SIZE)
|
||||
// now come from the real files.constants; only the request-time app_settings
|
||||
// read is mocked, preserving the old '*'-allowlist semantics for the fixtures.
|
||||
vi.mock('../../src/nest/files/files.bridge', () => ({ getAllowedExtensions: () => '*' }));
|
||||
// The memories providers are injected since the fold — stubbed on the prototype
|
||||
// so JourneyModule still resolves them through DI.
|
||||
vi.mock('../../src/nest/memories/immich.service', async (importOriginal) => {
|
||||
|
||||
@@ -15,6 +15,7 @@ import {
|
||||
cacheKeyFor,
|
||||
clearGeoCache,
|
||||
getCached,
|
||||
hasCached,
|
||||
nominatimFetch,
|
||||
setCached,
|
||||
setGeoThrottleInterval,
|
||||
@@ -153,12 +154,17 @@ describe('geo cache', () => {
|
||||
expect(cacheKeyFor(52.53, 13.405, 'zoom18')).not.toBe(cacheKeyFor(52.52, 13.405, 'zoom18'));
|
||||
});
|
||||
|
||||
it('GEO-009: distinguishes a cached undefined from a miss', () => {
|
||||
it('GEO-009: a cached negative answer is a HIT, not a miss', () => {
|
||||
// The earlier version asserted `undefined` before and after the write, which
|
||||
// is the same assertion twice and held with the `has` guard removed. What
|
||||
// the guard buys is that the second lookup does not go back to the network:
|
||||
// the ocean does not become a country, so a null answer is worth keeping.
|
||||
const key = cacheKeyFor(0, 0, 'zoom3');
|
||||
expect(getCached(key)).toBeUndefined();
|
||||
expect(hasCached(key)).toBe(false);
|
||||
|
||||
setCached(key, undefined);
|
||||
// A negative answer is worth caching: the ocean does not become a country.
|
||||
// `has` rather than a truthy check is what keeps it from being re-fetched.
|
||||
|
||||
expect(hasCached(key)).toBe(true);
|
||||
expect(getCached(key)).toBeUndefined();
|
||||
});
|
||||
|
||||
|
||||
@@ -20,6 +20,7 @@ import {
|
||||
joinRoom,
|
||||
registerSocket,
|
||||
setServer,
|
||||
userOf,
|
||||
type TrekWebSocket,
|
||||
} from '../../../src/nest/realtime/ws-state';
|
||||
import { emitPluginEvent } from '../../../src/plugin-event-sink';
|
||||
@@ -134,10 +135,16 @@ describe('RealtimeGateway handshake', () => {
|
||||
});
|
||||
|
||||
it('WSGW-008: never leaks password_version past the handshake', () => {
|
||||
const { gw, ws } = connect('/ws?token=x');
|
||||
gw.handleJoin({ tripId: 7 }, ws);
|
||||
broadcast(7, 'trip:updated' as never, {} as never);
|
||||
expect(ws.sent.join('')).not.toContain('password_version');
|
||||
// Asserted on what the socket registry HOLDS, not on the frames it sends.
|
||||
// The frames never carry the user object at all, so the earlier version of
|
||||
// this case passed with the strip deleted: it proved nothing. What matters
|
||||
// is that the retained identity is clean, because that object is what
|
||||
// getOnlineUserIds and the onlyUserId filter read.
|
||||
const { ws } = connect('/ws?token=x');
|
||||
const held = userOf(ws) as unknown as Record<string, unknown> | undefined;
|
||||
expect(held).toBeDefined();
|
||||
expect(held).not.toHaveProperty('password_version');
|
||||
expect(held).toMatchObject({ id: 3, email: 'm@x.test' });
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
@@ -12,7 +12,11 @@ import { describe, it, expect, vi, beforeEach } from 'vitest';
|
||||
// The origin allowlist is env-driven and empty in the test environment, so the
|
||||
// branch that builds verifyClient would never run. Driving readEnv lets both
|
||||
// sides of it be asserted rather than assumed.
|
||||
const { wsOrigins } = vi.hoisted(() => ({ wsOrigins: { value: null as string[] | null } }));
|
||||
const { wsOrigins, logError } = vi.hoisted(() => ({
|
||||
wsOrigins: { value: null as string[] | null },
|
||||
logError: vi.fn(),
|
||||
}));
|
||||
vi.mock('../../../src/nest/audit/audit-log.logger', () => ({ logError, logInfo: vi.fn(), logDebug: vi.fn(), logWarn: vi.fn() }));
|
||||
vi.mock('../../../src/app-config', async (importOriginal) => {
|
||||
const actual = (await importOriginal()) as Record<string, unknown>;
|
||||
return {
|
||||
@@ -243,12 +247,21 @@ describe('TrekWsAdapter server creation', () => {
|
||||
}
|
||||
});
|
||||
|
||||
it('WSAD-031c: a server-level error is absorbed, not rethrown at the process', () => {
|
||||
it('WSAD-031c: a server-level error is LOGGED, never swallowed', () => {
|
||||
// ws forwards the http server's errors here, so an empty handler eats the
|
||||
// bind failure too and the process survives EADDRINUSE serving nothing.
|
||||
// index.ts owns the fatal decision; this handler owes a log line.
|
||||
wsOrigins.value = null;
|
||||
const ad = new TrekWsAdapter(httpServer);
|
||||
const server = ad.create(0, { path: '/ws' }) as { emit: (e: string, err: Error) => boolean };
|
||||
const server = ad.create(0, { path: '/ws' }) as { emit: (e: string, err: unknown) => boolean };
|
||||
try {
|
||||
expect(() => server.emit('error', new Error('upstream reset'))).not.toThrow();
|
||||
expect(logError).toHaveBeenCalledWith(expect.stringContaining('upstream reset'));
|
||||
|
||||
// A non-Error rejection reads as itself rather than as [object Object].
|
||||
logError.mockClear();
|
||||
server.emit('error', 'ECONNRESET');
|
||||
expect(logError).toHaveBeenCalledWith(expect.stringContaining('ECONNRESET'));
|
||||
} finally {
|
||||
void ad.close(server as never);
|
||||
}
|
||||
|
||||
@@ -0,0 +1,79 @@
|
||||
import { describe, it, expect, vi } from 'vitest';
|
||||
import http from 'node:http';
|
||||
import { WebSocketServer } from 'ws';
|
||||
|
||||
/**
|
||||
* A bind failure must stay loud after the ws server moved in front of listen().
|
||||
*
|
||||
* `ws` registers `error: this.emit.bind(this, 'error')` on the http server it
|
||||
* attaches to (websocket-server.js). Since buildApp now creates the ws server
|
||||
* BEFORE index.ts calls listen(), the http server already has an error listener
|
||||
* by the time the bind is attempted — which is exactly the condition under which
|
||||
* Node stops throwing on an unhandled one.
|
||||
*
|
||||
* The failure that hides behind that is not subtle: EADDRINUSE gets forwarded to
|
||||
* the WebSocketServer, and if its handler is a no-op the process survives, the
|
||||
* listen callback never runs, so the startup banner never prints and the
|
||||
* scheduler never starts. A container that looks healthy and serves nothing.
|
||||
*
|
||||
* These cases pin the mechanism rather than index.ts itself, because the module
|
||||
* calls process.exit and boots the whole app on import.
|
||||
*/
|
||||
async function occupyAPort(): Promise<{ port: number; release: () => void }> {
|
||||
const blocker = http.createServer();
|
||||
await new Promise<void>((resolve) => blocker.listen(0, '127.0.0.1', resolve));
|
||||
const { port } = blocker.address() as { port: number };
|
||||
return { port, release: () => blocker.close() };
|
||||
}
|
||||
|
||||
describe('ws attached before listen()', () => {
|
||||
it('WSBIND-001: ws silences the http server, so a no-op handler loses EADDRINUSE entirely', async () => {
|
||||
const { port, release } = await occupyAPort();
|
||||
try {
|
||||
const server = http.createServer();
|
||||
const wss = new WebSocketServer({ server, path: '/ws' });
|
||||
const swallowed = vi.fn();
|
||||
wss.on('error', swallowed); // the shape this used to have
|
||||
|
||||
let listenCallbackRan = false;
|
||||
server.listen(port, '127.0.0.1', () => { listenCallbackRan = true; });
|
||||
await new Promise((r) => setTimeout(r, 300));
|
||||
|
||||
// The bind failed, and the only thing that heard about it was the ws
|
||||
// handler. Nothing threw, nothing exited.
|
||||
expect(swallowed).toHaveBeenCalledTimes(1);
|
||||
expect((swallowed.mock.calls[0][0] as NodeJS.ErrnoException).code).toBe('EADDRINUSE');
|
||||
expect(listenCallbackRan).toBe(false);
|
||||
|
||||
wss.close();
|
||||
server.close();
|
||||
} finally {
|
||||
release();
|
||||
}
|
||||
});
|
||||
|
||||
it('WSBIND-002: an explicit http-server error handler is what makes it fatal again', async () => {
|
||||
const { port, release } = await occupyAPort();
|
||||
try {
|
||||
const server = http.createServer();
|
||||
const wss = new WebSocketServer({ server, path: '/ws' });
|
||||
wss.on('error', () => {});
|
||||
|
||||
// What index.ts registers. It has to sit on the HTTP server: by the time
|
||||
// ws re-emits, the process has already declined to throw.
|
||||
const fatal = vi.fn();
|
||||
server.on('error', fatal);
|
||||
|
||||
server.listen(port, '127.0.0.1', () => {});
|
||||
await new Promise((r) => setTimeout(r, 300));
|
||||
|
||||
expect(fatal).toHaveBeenCalledTimes(1);
|
||||
expect((fatal.mock.calls[0][0] as NodeJS.ErrnoException).code).toBe('EADDRINUSE');
|
||||
|
||||
wss.close();
|
||||
server.close();
|
||||
} finally {
|
||||
release();
|
||||
}
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user