test(routes): integration-grade route-test harness with real auth middleware (#3962)

[Yeraze]

Summary

Task 1.3 of the remediation plan (#3962, Phase 1).

Route tests previously monkey-patched the permission layer ((databaseService as any).checkPermissionAsync = vi.fn()...), so the real middleware chain was never exercised. This adds the shared fixture and converts 4 representative files as the template:

• New src/server/test-helpers/routeTestApp.ts — createRouteTestApp({ mount }) boots a real Express app with real express-session (MemoryStore), real optionalAuth()/requireAuth/requirePermission, against the live in-memory-SQLite databaseService singleton (full migration chain — the vitest env already points the singleton at :memory:). Returns seeded admin/limited/anonymous users, two sources, loginAs() (session minted via a test-only login route — no bcrypt), per-source grant(), and cleanup().
• Converted: sourceRoutes.permissions (14 tests), v1/messages.permissions (3), channelDatabaseRoutes.permissions (11), waypoints (16). Each keeps its original scenarios and gains genuine source-isolation assertions — granted on source A → 200, denied on source B → 403/filtered — driven by real permission rows and real getUserPermissionSetAsync scoping, not mock lambdas.
• CLAUDE.md documents the harness as the standard for new/changed route tests; the monkey-patch pattern is deprecated for routes. No mass conversion (per plan) — 64 files with the old pattern convert opportunistically.

The conversion immediately paid for itself: exercising the real route code surfaced a production bug — GET /api/v1/messages without a sourceId param 500'd against the Task-1.1 fail-closed guard (two call sites in v1/messages.ts missing the ?? ALL_SOURCES disposition their sibling line already had). Fixed here with the intentional-cross-source comment.

Also includes a harness-robustness fix: poll for the fire-and-forget ensureAnonymousUser() seeding after waitForReady() (constructor race under fresh per-file module instances).

Verification (independent orchestrator run)

• npm run typecheck: clean. npm run typecheck:tests: 283 (== baseline, no increase).
• Full suite: 8120 passed, 0 failed, 0 suite failures (success: true).
• Harness and converted files contain zero .prepare/.exec/.query raw SQL and zero monkey-patched permission fakes.

Refs #3962

🤖 Generated with Claude Code

https://claude.ai/code/session_01AphLfgUJWaFNAgU5UXdJ4n

[github-actions]

Claude finished @Yeraze's task —— View job

---

PR Review

• Read changed files and gather context
• Review routeTestApp.ts (new harness)
• Review converted test files
• Review messages.ts bug fix
• Review CLAUDE.md updates
• Post final review

---

This is a well-designed, high-value change. The harness correctly identifies and fixes a real testing anti-pattern. A few observations below, ranging from significant to minor.

routeTestApp.ts — the harness

Potential issue: grant() can silently fail with duplicate rows

The grant() helper unconditionally calls createPermission() (line 241) without checking for an existing (userId, resource, sourceId) row. If a test calls grant() twice for the same triple (easy to do in nested describes or during development), the second call will hit the UNIQUE constraint on SQLite and throw. The thrown error surfaces as an unhandled rejection in beforeEach, making failures hard to diagnose. Consider an upsert or a pre-delete in grant():

// belt-and-suspenders: remove the row first so createPermission never hits the UNIQUE constraint
await databaseService.auth.deletePermissionsForUserByScope(userId, sourceId ?? null).catch(() => {});
await databaseService.auth.createPermission({ ... });

Minor: revokeAll silently swallows errors but cleanup wraps its calls with .catch(() => {})

revokeAll (line 253) doesn't have a .catch(() => {}), but cleanup wraps both deletePermissionsForUser calls. If a test calls revokeAll and the user doesn't exist, it will throw. Given the singleton DB and unique-per-file users this is low-risk, but it's an inconsistency worth aligning.

Design note (not a bug): fixed source IDs across the whole file

SOURCE_A = 'rt-source-a' and SOURCE_B = 'rt-source-b' are constants, not counter-suffixed. cleanup() deletes and beforeEach recreates them, which is correct. The comment at line 138–141 explains this. Just noting it's intentional — having fixed names simplifies URL construction in tests.

loginAs(null) comment (line 273) is slightly misleading

The comment says findUserByUsernameAsync('anonymous') fallback path, but this path is exercised only when optionalAuth is mounted (line 227). When useOptionalAuth: false, loginAs(null) produces a request with no req.user at all — the route handler sees req.user as undefined. The JSDoc could acknowledge this distinction.

messages.ts — bug fix

Lines 124–129: the fix (?? ALL_SOURCES) is correct and the // intentional cross-source when sourceId omitted comments are helpful. The three call sites are now consistent with each other.

One follow-up worth noting: GET /:messageId at line 306 calls getMessages(10000, 0, msgLookupSourceId) without the ?? ALL_SOURCES fallback, which means it passes undefined when no sourceId is given. That's a fourth call site that wasn't fixed here. This was presumably pre-existing and may be covered by the repository's own default handling, but it's worth a look:

// line 306 — note: no ?? ALL_SOURCES
const allMessages = await databaseService.messages.getMessages(10000, 0, msgLookupSourceId);

Test files — converted

messages.permissions.test.ts — the spy-passthrough pattern (lines 70–71) is a nice touch: tests observe call routing while the real SQL runs. The source-isolation baseline (grant on A only, assert B returns count 0) is exactly the regression that motivated the harness.

sourceRoutes.permissions.test.ts — the parametric loop over endpoints (line 66–80) is clean. The /channels exclusion is well-documented with a reference to MM-SEC-7.

waypoints.test.ts — the note about the UNIQUE constraint on (userId, resource, sourceId) at line 22–26 is important for future test authors and matches the real schema. Good documentation.

channelDatabaseRoutes.permissions.test.ts — the hybrid approach (real checkPermissionAsync for gate enforcement + vi.spyOn on repo data methods) is the right split. Spying on getAllAsync etc. avoids seeding real channel rows while still exercising the real permission gate.

CLAUDE.md

The additions are accurate and well-placed. The deprecation notice for the monkey-patch pattern and the template reference to sourceRoutes.permissions.test.ts will help future contributors land correctly.

Summary

The harness design is sound, the converted tests are meaningful improvements over the monkey-patch pattern, and the messages.ts fix is correct. The main things worth addressing before merge:

1. The grant() duplicate-key risk (silent UNIQUE constraint failure in beforeEach) — worth a defensive upsert or pre-delete.
2. GET /:messageId (line 306 of messages.ts) missing ?? ALL_SOURCES — wasn't part of this PR's scope but is now visible; worth a follow-up ticket or fixing inline.

[Yeraze]

CodeQL triage note (orchestrator): the three alerts (#159 clear-text cookie, #160 missing CSRF, #161 session not rotated on login) are all on src/server/test-helpers/routeTestApp.ts — a Vitest-only fixture (in-memory session store, ephemeral per-file database, test-only login route, never mounted in the production app). Recommend dismissing as used in tests; leaving the alerts open for a human decision.