Skip to content

Commit 0c34154

Browse files
pngwnclaude
andcommitted
test: add testRequest() helper and fix harness hygiene (G1 + G2)
G1 — testRequest() Server specs invoked handlers directly as `GET({ locals, url } as never)`. The cast disabled type-checking at the test boundary, and nothing ever exercised the `handle` hook: CSRF, CORS, the admin Bearer gate, the global 401-on-mutation rule and cookie refresh had zero coverage. `src/lib/server/__tests__/testRequest.ts` builds a real Request and a real RequestEvent, then calls `handleRequest({ event, resolve })` with a resolve that runs the handler. Two auth modes: real (pass a session cookie, minted by `createTestUser()`) and override (pass `locals` — cheaper, for tests where auth is incidental). Thrown `error()`/`redirect()` come back as responses, as in production, so specs assert `res.status` rather than catching. `createTestUser()` now stores its session under sha256(secret) so real cookie auth resolves it, and returns `cookie` / `secretSessionId`. Converted misc, conversations and user specs (no assertions lost) and added 13 tests covering behaviour that was previously unreachable: the CSRF origin rule across all three native-form content types, JSON's exemption from it, CORS headers on /api/**, session-cookie refresh and expiry extension on POST, and identity resolved purely from a cookie. Type safety is restored: handler signature drift, misspelled options, wrong `locals` field types and missing options are now all compile errors. G2 — harness hygiene - `cleanupTestData()` covered 7 collections; it now covers all of them plus GridFS. The gap was a live flake risk — leaked `messageEvents` count against the rate limit on POST /conversation/[id]. - `await ready` is explicit in the 11 specs that touch `collections` directly; the helpers await it internally so they are safe in isolation. - Dropped `{ retry: 3 }` from migrations.spec.ts: green in 12 isolated runs and 5 full-suite runs. Verified: 37 files / 412 tests green over 6 runs, lint and check clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 594800d commit 0c34154

14 files changed

Lines changed: 702 additions & 140 deletions

src/lib/server/__tests__/conversation-stop-generating.spec.ts

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
1-
import { afterEach, describe, expect, it, vi } from "vitest";
1+
import { afterEach, beforeAll, describe, expect, it, vi } from "vitest";
22
import { ObjectId } from "mongodb";
33

4-
import { collections } from "$lib/server/database";
4+
import { collections, ready } from "$lib/server/database";
55
import { AbortRegistry } from "$lib/server/abortRegistry";
66
import {
77
cleanupTestData,
@@ -11,6 +11,11 @@ import {
1111
} from "$lib/server/api/__tests__/testHelpers";
1212
import { POST } from "../../../routes/conversation/[id]/stop-generating/+server";
1313

14+
// `collections` is undefined until the database IIFE resolves.
15+
beforeAll(async () => {
16+
await ready;
17+
});
18+
1419
describe.sequential("POST /conversation/[id]/stop-generating", () => {
1520
afterEach(async () => {
1621
vi.restoreAllMocks();
Lines changed: 319 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,319 @@
1+
/**
2+
* `testRequest()` — invoke a route handler the way production does: through the
3+
* real `handle` hook.
4+
*
5+
* ## Why this exists
6+
*
7+
* Server specs used to call handlers directly with a hand-built object:
8+
*
9+
* ```ts
10+
* const res = await GET({ locals, params, url } as never);
11+
* ```
12+
*
13+
* That has two costs. The `as never` cast disables type-checking at the test
14+
* boundary, so a change to the handler signature is invisible to the suite. And
15+
* — the bigger one — everything `src/lib/server/hooks/handle.ts` does is
16+
* skipped: CSRF, CORS, the admin Bearer gate, the global 401-on-mutation rule,
17+
* session-cookie refresh and the OAuth redirect allowlist are all untested.
18+
*
19+
* `testRequest()` builds a real `Request` and a real `RequestEvent`, then calls
20+
* `handleRequest({ event, resolve })` with a `resolve` that runs the handler.
21+
* The middleware runs for real; only the router (path -> handler) is stubbed,
22+
* because the caller names the handler directly.
23+
*
24+
* ## Two auth modes
25+
*
26+
* **Real** — pass a session cookie. `handle` resolves it through
27+
* `authenticateRequest()` and populates `event.locals` itself, exactly as in
28+
* production. Mint one with `createTestUser()`, which returns a ready-made
29+
* `cookie` header value:
30+
*
31+
* ```ts
32+
* const { cookie, user } = await createTestUser();
33+
* const res = await testRequest(GET, { path: "/api/v2/user", headers: { cookie } });
34+
* ```
35+
*
36+
* Use this whenever auth or middleware *is* the subject: session lookup, the
37+
* 401 rules, cookie refresh, admin gating.
38+
*
39+
* **Override** — pass `locals` directly. Cheaper: no user, session or cookie to
40+
* create. Use it when auth is incidental to what you are asserting:
41+
*
42+
* ```ts
43+
* const res = await testRequest(GET, {
44+
* path: "/api/v2/conversations?p=1",
45+
* locals: { sessionId: "s1", isAdmin: false },
46+
* });
47+
* ```
48+
*
49+
* The override is applied *inside* `resolve`, i.e. after the hook has finished
50+
* its own auth work. So it changes what the **handler** sees, not what the
51+
* **middleware** saw — to the middleware the request is still whatever the
52+
* cookies said (anonymous, by default). Middleware rules keyed on identity —
53+
* notably the `loginEnabled` 401-on-mutation rule — therefore evaluate against
54+
* the real request, not the override. Tests that assert those rules must use
55+
* real auth.
56+
*
57+
* ## Notes for callers
58+
*
59+
* - `path` is origin-relative and may carry a query string:
60+
* `"/api/v2/conversations?p=1"`. The origin is {@link TEST_ORIGIN}, which is
61+
* also the host the CSRF check accepts as a valid `Origin`.
62+
* - A handler that throws `error(...)` or `redirect(...)` comes back as a
63+
* `Response` with the matching status, as it would in production, rather than
64+
* propagating. Assert `res.status` instead of wrapping the call in try/catch.
65+
* - Cookies written during the request (e.g. by `refreshSessionCookie`) are
66+
* observable as `set-cookie` headers on the returned response.
67+
*
68+
* ## What is *not* reachable from here, and why
69+
*
70+
* Two middleware branches are gated on module-level constants that are read
71+
* from `.env` at import time, so no per-test option can flip them:
72+
*
73+
* - **The `loginEnabled` 401-on-mutation rule** (anonymous non-GET is 401 on
74+
* every path except `/login`, `/admin`, `/settings`). `loginEnabled` is
75+
* `!!OIDConfig.CLIENT_ID` and the repo `.env` ships `OPENID_CLIENT_ID=""`, so
76+
* it is **false** throughout the suite and the rule never fires. What this
77+
* file's specs pin is the complementary fact — with login disabled, anonymous
78+
* mutations are allowed through.
79+
* - **The admin Bearer gate** on `/admin/**` needs `ADMIN_API_SECRET` or
80+
* `PARQUET_EXPORT_SECRET`; both are empty in `.env`, so `/admin` requests get
81+
* `500 "Admin API is not configured"` rather than reaching the 401 branch.
82+
*
83+
* Covering either properly means controlling those env values — which belongs
84+
* to whoever owns the harness setup, not to individual specs. Flagged here for
85+
* A8, which owns the exhaustive middleware suite.
86+
*
87+
* Imports `handleRequest` rather than the `handle` export of
88+
* `src/hooks.server.ts` deliberately: that export is a thin wrapper whose only
89+
* extra behaviour is a `building` short-circuit, and `building` is always false
90+
* under Vitest — so this is the same code path. Importing `hooks.server.ts`
91+
* would additionally pull in `hooks/init.ts`, dragging the migration runner,
92+
* the MCP registry and the satori font loader into every spec as import side
93+
* effects, for no behavioural gain.
94+
*/
95+
import {
96+
isHttpError,
97+
isRedirect,
98+
type Cookies,
99+
type RequestEvent,
100+
type RequestHandler,
101+
} from "@sveltejs/kit";
102+
import { handleRequest } from "$lib/server/hooks/handle";
103+
import { ready } from "$lib/server/database";
104+
105+
/**
106+
* Origin every test request is issued from. The CSRF check in `handle` compares
107+
* the `Origin` header against the request's own host, so this is the value a
108+
* native-form POST must send to be accepted.
109+
*/
110+
export const TEST_ORIGIN = "http://localhost:5173";
111+
112+
export interface TestRequestOptions {
113+
/** Origin-relative path, query string included: `"/api/v2/conversations?p=1"`. */
114+
path: string;
115+
/** Defaults to `GET`. */
116+
method?: string;
117+
body?: BodyInit;
118+
headers?: HeadersInit;
119+
/** Route params the handler would receive, e.g. `{ id: conv._id.toString() }`. */
120+
params?: Record<string, string>;
121+
/**
122+
* Bypass real authentication and hand these locals to the handler. Applied
123+
* after the hook's own auth step — see the module docs for what that means
124+
* for middleware assertions.
125+
*/
126+
locals?: Partial<App.Locals>;
127+
}
128+
129+
type CookieSetOptions = Parameters<Cookies["set"]>[2];
130+
131+
function serializeCookie(name: string, value: string, opts: CookieSetOptions): string {
132+
const parts = [`${name}=${encodeURIComponent(value)}`];
133+
134+
if (opts.maxAge !== undefined) parts.push(`Max-Age=${Math.floor(opts.maxAge)}`);
135+
if (opts.domain) parts.push(`Domain=${opts.domain}`);
136+
if (opts.path) parts.push(`Path=${opts.path}`);
137+
if (opts.expires) parts.push(`Expires=${new Date(opts.expires).toUTCString()}`);
138+
if (opts.httpOnly) parts.push("HttpOnly");
139+
if (opts.secure) parts.push("Secure");
140+
if (opts.sameSite) {
141+
const sameSite = opts.sameSite === true ? "strict" : opts.sameSite;
142+
parts.push(`SameSite=${sameSite.charAt(0).toUpperCase()}${sameSite.slice(1)}`);
143+
}
144+
145+
return parts.join("; ");
146+
}
147+
148+
function parseCookieHeader(header: string | null): Map<string, string> {
149+
const jar = new Map<string, string>();
150+
if (!header) return jar;
151+
152+
for (const pair of header.split(";")) {
153+
const eq = pair.indexOf("=");
154+
if (eq < 0) continue;
155+
const name = pair.slice(0, eq).trim();
156+
if (!name) continue;
157+
jar.set(name, decodeURIComponent(pair.slice(eq + 1).trim()));
158+
}
159+
160+
return jar;
161+
}
162+
163+
/**
164+
* Minimal but behaviour-compatible stand-in for SvelteKit's cookie jar, which
165+
* is internal to the framework and not importable. Reads seed from the
166+
* request's `Cookie` header; writes are readable back through `get()` (as in
167+
* SvelteKit) and are emitted as `set-cookie` on the response.
168+
*/
169+
function createCookies(request: Request): {
170+
cookies: Cookies;
171+
setCookieHeaders: () => string[];
172+
} {
173+
const jar = parseCookieHeader(request.headers.get("cookie"));
174+
const written: string[] = [];
175+
176+
return {
177+
cookies: {
178+
get: (name) => jar.get(name),
179+
getAll: () => [...jar].map(([name, value]) => ({ name, value })),
180+
set: (name, value, opts) => {
181+
jar.set(name, value);
182+
written.push(serializeCookie(name, value, opts));
183+
},
184+
delete: (name, opts) => {
185+
jar.delete(name);
186+
written.push(serializeCookie(name, "", { ...opts, maxAge: 0 }));
187+
},
188+
serialize: (name, value, opts) => serializeCookie(name, value, opts),
189+
},
190+
setCookieHeaders: () => written,
191+
};
192+
}
193+
194+
/**
195+
* Tracing is off in tests. SvelteKit types these as OpenTelemetry `Span`s; a
196+
* self-returning proxy satisfies any chained call a handler might make. The
197+
* cast is deliberately confined to this constant — the `RequestEvent` built
198+
* below stays fully typed, which is what restores signature checking.
199+
*/
200+
const NOOP_SPAN: unknown = new Proxy(
201+
{},
202+
{
203+
get:
204+
() =>
205+
(...args: unknown[]) => {
206+
void args;
207+
return NOOP_SPAN;
208+
},
209+
}
210+
);
211+
212+
const NOOP_TRACING = {
213+
enabled: false,
214+
root: NOOP_SPAN,
215+
current: NOOP_SPAN,
216+
} as RequestEvent["tracing"];
217+
218+
/**
219+
* Turn a thrown `error()` / `redirect()` back into the `Response` SvelteKit
220+
* would have produced. Anything else is a genuine fault and propagates.
221+
*/
222+
function toResponse(thrown: unknown): Response {
223+
if (isRedirect(thrown)) {
224+
return new Response(undefined, {
225+
status: thrown.status,
226+
headers: { location: thrown.location },
227+
});
228+
}
229+
230+
if (isHttpError(thrown)) {
231+
return new Response(JSON.stringify(thrown.body), {
232+
status: thrown.status,
233+
headers: { "content-type": "application/json" },
234+
});
235+
}
236+
237+
throw thrown;
238+
}
239+
240+
export async function testRequest(
241+
handler: RequestHandler,
242+
opts: TestRequestOptions
243+
): Promise<Response> {
244+
// `handle` writes to `collections.sessions` on POST, and `collections` is
245+
// undefined until the database IIFE resolves.
246+
await ready;
247+
248+
const url = new URL(opts.path, TEST_ORIGIN);
249+
const method = (opts.method ?? "GET").toUpperCase();
250+
const hasBody = opts.body !== undefined && method !== "GET" && method !== "HEAD";
251+
252+
const request = new Request(url, {
253+
method,
254+
headers: opts.headers,
255+
...(hasBody ? { body: opts.body } : {}),
256+
});
257+
258+
const { cookies, setCookieHeaders } = createCookies(request);
259+
const deferredHeaders = new Map<string, string>();
260+
261+
const event: RequestEvent = {
262+
cookies,
263+
fetch: globalThis.fetch,
264+
getClientAddress: () => "127.0.0.1",
265+
// `handle` populates these before any handler runs.
266+
locals: {} as App.Locals,
267+
params: (opts.params ?? {}) as RequestEvent["params"],
268+
platform: undefined,
269+
request,
270+
route: { id: null },
271+
setHeaders: (headers) => {
272+
for (const [rawKey, value] of Object.entries(headers)) {
273+
const key = rawKey.toLowerCase();
274+
if (key === "set-cookie") {
275+
throw new Error("Use `cookies.set(...)` rather than `setHeaders` for set-cookie");
276+
}
277+
if (deferredHeaders.has(key)) {
278+
throw new Error(`"${key}" header is already set`);
279+
}
280+
deferredHeaders.set(key, value);
281+
}
282+
},
283+
url,
284+
isDataRequest: false,
285+
isSubRequest: false,
286+
isRemoteRequest: false,
287+
tracing: NOOP_TRACING,
288+
};
289+
290+
const response = await handleRequest({
291+
event,
292+
resolve: async (resolvedEvent) => {
293+
// After the hook's auth step, so the handler sees the override even
294+
// though the middleware did not.
295+
if (opts.locals) {
296+
Object.assign(resolvedEvent.locals, opts.locals);
297+
}
298+
299+
let res: Response;
300+
try {
301+
res = await handler(resolvedEvent);
302+
} catch (thrown) {
303+
res = toResponse(thrown);
304+
}
305+
306+
for (const [key, value] of deferredHeaders) {
307+
res.headers.set(key, value);
308+
}
309+
310+
return res;
311+
},
312+
});
313+
314+
for (const header of setCookieHeaders()) {
315+
response.headers.append("set-cookie", header);
316+
}
317+
318+
return response;
319+
}

src/lib/server/api/__tests__/conversations-id.spec.ts

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
1-
import { describe, expect, it, afterEach } from "vitest";
1+
import { describe, expect, it, afterEach, beforeAll } from "vitest";
22
import { ObjectId } from "mongodb";
33
import superjson from "superjson";
4-
import { collections } from "$lib/server/database";
4+
import { collections, ready } from "$lib/server/database";
55
import {
66
createTestLocals,
77
createTestUser,
@@ -19,6 +19,11 @@ function mockUrl(): URL {
1919
return new URL("http://localhost:5173/api/v2/conversations/some-id");
2020
}
2121

22+
// `collections` is undefined until the database IIFE resolves.
23+
beforeAll(async () => {
24+
await ready;
25+
});
26+
2227
describe.sequential("GET /api/v2/conversations/[id]", () => {
2328
afterEach(async () => {
2429
await cleanupTestData();

src/lib/server/api/__tests__/conversations-message.spec.ts

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
1-
import { describe, expect, it, afterEach } from "vitest";
1+
import { describe, expect, it, afterEach, beforeAll } from "vitest";
22
import { ObjectId } from "mongodb";
33
import { v4 } from "uuid";
44
import superjson from "superjson";
5-
import { collections } from "$lib/server/database";
5+
import { collections, ready } from "$lib/server/database";
66
import type { Message } from "$lib/types/Message";
77
import {
88
createTestLocals,
@@ -86,6 +86,11 @@ function buildMessageTree(): {
8686
};
8787
}
8888

89+
// `collections` is undefined until the database IIFE resolves.
90+
beforeAll(async () => {
91+
await ready;
92+
});
93+
8994
describe.sequential("DELETE /api/v2/conversations/[id]/message/[messageId]", () => {
9095
afterEach(async () => {
9196
await cleanupTestData();

0 commit comments

Comments
 (0)