From 26d45cf2cbecd3ee630f14adea2094b6f0eaf2e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ullrich=20Sch=C3=A4fer?= Date: Wed, 24 Jun 2026 15:41:26 +0200 Subject: [PATCH] fix(journal): don't crash the process on a malformed request URL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A request for path `//` (also `///`, `/\`, ...) makes `new URL(req.url, base)` throw ERR_INVALID_URL. serveStatic runs synchronously inside the createServer callback, so the throw is an uncaught exception that kills the process. Docker (`unless-stopped`) restarts it, and a client looping on `//` crash-loops the journal — a trivial unauthenticated DoS. This fired the "Container restart loop" Grafana alert in production (journal restarted ~10x in 6 minutes). Guard the URL parse with try/catch and fall through to the React Router handler, which 404s malformed paths cleanly (the same way it already handles scanner probes like /root/.ssh/id_rsa). Extract serveStatic into its own module so it can be unit-tested without booting the HTTP server, and add a regression test covering the malformed-path cases. Widen the journal vitest include to discover co-located tests for root-level server infra. Co-Authored-By: Claude Opus 4.8 (1M context) --- apps/journal/serve-static.test.ts | 35 ++++++++++++++++++++ apps/journal/serve-static.ts | 55 +++++++++++++++++++++++++++++++ apps/journal/server.ts | 39 +--------------------- apps/journal/vitest.config.ts | 7 ++++ 4 files changed, 98 insertions(+), 38 deletions(-) create mode 100644 apps/journal/serve-static.test.ts create mode 100644 apps/journal/serve-static.ts diff --git a/apps/journal/serve-static.test.ts b/apps/journal/serve-static.test.ts new file mode 100644 index 0000000..f35debd --- /dev/null +++ b/apps/journal/serve-static.test.ts @@ -0,0 +1,35 @@ +import { describe, it, expect, vi } from "vitest"; +import type { IncomingMessage, ServerResponse } from "node:http"; +import { serveStatic } from "./serve-static.ts"; + +function mockReq(url: string, method = "GET"): IncomingMessage { + return { url, method, headers: { host: "trails.cool" } } as unknown as IncomingMessage; +} + +function mockRes(): ServerResponse { + return { setHeader: vi.fn() } as unknown as ServerResponse; +} + +describe("serveStatic", () => { + // Regression: a malformed path makes `new URL` throw ERR_INVALID_URL. + // Because serveStatic runs synchronously inside the HTTP server + // callback, an unhandled throw is process-fatal — a single `GET //` + // crash-looped the journal in prod. It must fall through (return false) + // so the request reaches React Router and 404s instead. + it.each(["//", "///", "/\\", "//foo\\bar"])( + "returns false without throwing for malformed path %j", + (path) => { + const req = mockReq(path); + expect(() => serveStatic(req, mockRes())).not.toThrow(); + expect(serveStatic(req, mockRes())).toBe(false); + }, + ); + + it("falls through for non-GET/HEAD methods", () => { + expect(serveStatic(mockReq("/assets/app.js", "POST"), mockRes())).toBe(false); + }); + + it("falls through for a well-formed path with no matching file", () => { + expect(serveStatic(mockReq("/does/not/exist.js"), mockRes())).toBe(false); + }); +}); diff --git a/apps/journal/serve-static.ts b/apps/journal/serve-static.ts new file mode 100644 index 0000000..3b317ef --- /dev/null +++ b/apps/journal/serve-static.ts @@ -0,0 +1,55 @@ +import type { IncomingMessage, ServerResponse } from "node:http"; +import { createReadStream, statSync } from "node:fs"; +import { join, extname, resolve } from "node:path"; + +const CLIENT_DIR = resolve(import.meta.dirname, "build", "client"); + +const MIME: Record = { + ".js": "application/javascript", + ".css": "text/css", + ".html": "text/html", + ".json": "application/json", + ".png": "image/png", + ".jpg": "image/jpeg", + ".svg": "image/svg+xml", + ".ico": "image/x-icon", + ".woff": "font/woff", + ".woff2": "font/woff2", +}; + +/** + * Serve a static asset from the client build. Returns true if the request + * was handled, false if it should fall through to the app request handler. + * + * Runs synchronously inside the HTTP server callback, so it must never + * throw: a thrown error here is an uncaught exception that crashes the + * whole process. Malformed request paths (e.g. `//`, `///`, `/\`) make + * `new URL` throw `ERR_INVALID_URL`; we catch that and fall through so + * React Router 404s the request instead of taking the server down. + */ +export function serveStatic(req: IncomingMessage, res: ServerResponse): boolean { + if (req.method !== "GET" && req.method !== "HEAD") return false; + + let url: URL; + try { + url = new URL(req.url ?? "/", `http://${req.headers.host}`); + } catch { + return false; + } + const filePath = resolve(join(CLIENT_DIR, url.pathname)); + + if (!filePath.startsWith(CLIENT_DIR)) return false; + + try { + if (!statSync(filePath).isFile()) return false; + } catch { + return false; + } + + res.setHeader("Content-Type", MIME[extname(filePath)] ?? "application/octet-stream"); + if (url.pathname.startsWith("/assets/")) { + res.setHeader("Cache-Control", "public, immutable, max-age=31536000"); + } + createReadStream(filePath).pipe(res); + return true; +} diff --git a/apps/journal/server.ts b/apps/journal/server.ts index 00a7687..2fe7039 100644 --- a/apps/journal/server.ts +++ b/apps/journal/server.ts @@ -2,8 +2,7 @@ import * as Sentry from "@sentry/node"; import { nodeSentryConfig, drop404s } from "@trails-cool/sentry-config"; import { createRequestListener } from "@react-router/node"; import { createServer, type IncomingMessage, type ServerResponse } from "node:http"; -import { createReadStream, statSync } from "node:fs"; -import { join, extname, resolve } from "node:path"; +import { serveStatic } from "./serve-static.ts"; import { logger, requestContext } from "./app/lib/logger.server.ts"; import { randomUUID } from "node:crypto"; import { httpRequestDuration, normalizeRoute, registry } from "./app/lib/metrics.server.ts"; @@ -26,42 +25,6 @@ if (process.env.SENTRY_DISABLED !== "true" && sentryDsn) { } const port = Number(process.env.PORT ?? 3000); -const CLIENT_DIR = resolve(import.meta.dirname, "build", "client"); - -const MIME: Record = { - ".js": "application/javascript", - ".css": "text/css", - ".html": "text/html", - ".json": "application/json", - ".png": "image/png", - ".jpg": "image/jpeg", - ".svg": "image/svg+xml", - ".ico": "image/x-icon", - ".woff": "font/woff", - ".woff2": "font/woff2", -}; - -function serveStatic(req: IncomingMessage, res: ServerResponse): boolean { - if (req.method !== "GET" && req.method !== "HEAD") return false; - - const url = new URL(req.url ?? "/", `http://${req.headers.host}`); - const filePath = resolve(join(CLIENT_DIR, url.pathname)); - - if (!filePath.startsWith(CLIENT_DIR)) return false; - - try { - if (!statSync(filePath).isFile()) return false; - } catch { - return false; - } - - res.setHeader("Content-Type", MIME[extname(filePath)] ?? "application/octet-stream"); - if (url.pathname.startsWith("/assets/")) { - res.setHeader("Cache-Control", "public, immutable, max-age=31536000"); - } - createReadStream(filePath).pipe(res); - return true; -} const listener = createRequestListener({ build: () => import("./build/server/index.js" as string) as never, diff --git a/apps/journal/vitest.config.ts b/apps/journal/vitest.config.ts index 558bad0..ecd0a99 100644 --- a/apps/journal/vitest.config.ts +++ b/apps/journal/vitest.config.ts @@ -3,6 +3,13 @@ import shared from "../../vitest.shared.ts"; import { resolve } from "node:path"; export default mergeConfig(shared, defineConfig({ + test: { + // mergeConfig concatenates arrays, so this adds to the shared + // include globs rather than replacing them. Picks up co-located + // tests for root-level server infra (server.ts, serve-static.ts) + // that live outside app/ and src/. + include: ["*.test.{ts,tsx}"], + }, resolve: { alias: { "~": resolve(import.meta.dirname, "app"),