fix(journal): don't crash the process on a malformed request URL
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) <noreply@anthropic.com>
This commit is contained in:
parent
3d7dfda45f
commit
26d45cf2cb
4 changed files with 98 additions and 38 deletions
35
apps/journal/serve-static.test.ts
Normal file
35
apps/journal/serve-static.test.ts
Normal file
|
|
@ -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);
|
||||||
|
});
|
||||||
|
});
|
||||||
55
apps/journal/serve-static.ts
Normal file
55
apps/journal/serve-static.ts
Normal file
|
|
@ -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<string, string> = {
|
||||||
|
".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;
|
||||||
|
}
|
||||||
|
|
@ -2,8 +2,7 @@ import * as Sentry from "@sentry/node";
|
||||||
import { nodeSentryConfig, drop404s } from "@trails-cool/sentry-config";
|
import { nodeSentryConfig, drop404s } from "@trails-cool/sentry-config";
|
||||||
import { createRequestListener } from "@react-router/node";
|
import { createRequestListener } from "@react-router/node";
|
||||||
import { createServer, type IncomingMessage, type ServerResponse } from "node:http";
|
import { createServer, type IncomingMessage, type ServerResponse } from "node:http";
|
||||||
import { createReadStream, statSync } from "node:fs";
|
import { serveStatic } from "./serve-static.ts";
|
||||||
import { join, extname, resolve } from "node:path";
|
|
||||||
import { logger, requestContext } from "./app/lib/logger.server.ts";
|
import { logger, requestContext } from "./app/lib/logger.server.ts";
|
||||||
import { randomUUID } from "node:crypto";
|
import { randomUUID } from "node:crypto";
|
||||||
import { httpRequestDuration, normalizeRoute, registry } from "./app/lib/metrics.server.ts";
|
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 port = Number(process.env.PORT ?? 3000);
|
||||||
const CLIENT_DIR = resolve(import.meta.dirname, "build", "client");
|
|
||||||
|
|
||||||
const MIME: Record<string, string> = {
|
|
||||||
".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({
|
const listener = createRequestListener({
|
||||||
build: () => import("./build/server/index.js" as string) as never,
|
build: () => import("./build/server/index.js" as string) as never,
|
||||||
|
|
|
||||||
|
|
@ -3,6 +3,13 @@ import shared from "../../vitest.shared.ts";
|
||||||
import { resolve } from "node:path";
|
import { resolve } from "node:path";
|
||||||
|
|
||||||
export default mergeConfig(shared, defineConfig({
|
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: {
|
resolve: {
|
||||||
alias: {
|
alias: {
|
||||||
"~": resolve(import.meta.dirname, "app"),
|
"~": resolve(import.meta.dirname, "app"),
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue