diff --git a/app/entry.server.tsx b/app/entry.server.tsx index 77cfdf5..2ff5275 100644 --- a/app/entry.server.tsx +++ b/app/entry.server.tsx @@ -1,18 +1,26 @@ import * as Sentry from "@sentry/react-router"; import { PassThrough } from "node:stream"; import { logger } from "~/lib/logger"; +import { shouldReportServerError } from "~/lib/error-reporting"; -import type { AppLoadContext, EntryContext } from "react-router"; +import type { AppLoadContext, EntryContext, HandleErrorFunction } from "react-router"; import { createReadableStreamFromReadable } from "@react-router/node"; import { ServerRouter } from "react-router"; import { isbot } from "isbot"; import type { RenderToPipeableStreamOptions } from "react-dom/server"; import { renderToPipeableStream } from "react-dom/server"; -export const handleError = Sentry.createSentryHandleError({ +const sentryHandleError = Sentry.createSentryHandleError({ logErrors: true, }); +export const handleError: HandleErrorFunction = (error, args) => { + // Unrecognised URLs and methods are bot scans, not bugs. Skipping early also + // keeps them out of the `logErrors` console output; morgan still logs the request. + if (!shouldReportServerError(error, args.request)) return; + return sentryHandleError(error, args); +}; + export const streamTimeout = 5_000; async function handleRequest( diff --git a/app/lib/__tests__/error-reporting.test.ts b/app/lib/__tests__/error-reporting.test.ts new file mode 100644 index 0000000..ed46e5b --- /dev/null +++ b/app/lib/__tests__/error-reporting.test.ts @@ -0,0 +1,230 @@ +import { describe, it, expect } from "vitest"; +import { createStaticHandler } from "react-router"; +import { shouldReportServerError } from "../error-reporting"; + +const ORIGIN = "https://brackt.com"; + +/** Shaped like the ErrorResponse React Router hands to `handleError`. */ +function routeError( + status: number, + internal: boolean, + statusText = "Not Found", +) { + return { + status, + statusText, + internal, + data: `Error: No route matches URL "/blog/wp/v2/posts/999999"`, + }; +} + +function request(path: string, referer?: string, method = "GET") { + return new Request(`${ORIGIN}${path}`, { + method, + headers: referer ? { referer } : {}, + }); +} + +describe("shouldReportServerError", () => { + it("drops a router 404 for a scanner hitting a URL cold", () => { + expect( + shouldReportServerError( + routeError(404, true), + request("/blog/wp/v2/posts/999999"), + ), + ).toBe(false); + }); + + it("drops a router 404 linked from another site", () => { + expect( + shouldReportServerError( + routeError(404, true), + request("/blog/", "https://evil.example/"), + ), + ).toBe(false); + }); + + it("reports a router 404 linked from one of our own pages", () => { + expect( + shouldReportServerError( + routeError(404, true), + request("/leagues/gone", `${ORIGIN}/leagues`), + ), + ).toBe(true); + }); + + it("drops the 405 from a POST to a route with no action", () => { + expect( + shouldReportServerError( + routeError(405, true, "Method Not Allowed"), + request("/", undefined, "POST"), + ), + ).toBe(false); + }); + + it("reports a 404 the app threw deliberately", () => { + expect( + shouldReportServerError( + routeError(404, false), + request("/leagues/missing"), + ), + ).toBe(true); + }); + + it("reports a 403 the app threw from an ownership check", () => { + expect( + shouldReportServerError( + routeError(403, false, "Forbidden"), + request("/admin/sports"), + ), + ).toBe(true); + }); + + it("reports a router-internal 500", () => { + expect( + shouldReportServerError( + routeError(500, true, "Internal Server Error"), + request("/leagues"), + ), + ).toBe(true); + }); + + it("reports a plain exception", () => { + expect( + shouldReportServerError(new Error("boom"), request("/leagues")), + ).toBe(true); + }); + + it("reports anything that is not a route error response", () => { + expect(shouldReportServerError("just a string", request("/leagues"))).toBe( + true, + ); + expect(shouldReportServerError(null, request("/leagues"))).toBe(true); + }); + + it("drops a router 404 whose referer header is not a URL", () => { + expect( + shouldReportServerError( + routeError(404, true), + request("/blog/", "not a url"), + ), + ).toBe(false); + }); +}); + +/** + * The unit tests above use hand-written error objects. These drive real requests + * through React Router so the suite fails if the shape it throws ever changes. + */ +describe("shouldReportServerError against real React Router errors", () => { + const handler = createStaticHandler([ + { + id: "root", + path: "/", + children: [{ id: "home", index: true, loader: () => null }], + }, + ]); + + async function errorFor(req: Request) { + const ctx = await handler.query(req); + if (ctx instanceof Response) return null; + return Object.values(ctx.errors ?? {})[0] ?? null; + } + + it('drops the 404 for an unmatched URL (No route matches URL "...")', async () => { + const req = request("/blog/wp/v2/posts/999999"); + const error = await errorFor(req); + expect(error).toMatchObject({ status: 404, internal: true }); + expect(shouldReportServerError(error, req)).toBe(false); + }); + + it("reports the same 404 when it came from a link on our own site", async () => { + const req = request("/nope", `${ORIGIN}/leagues`); + expect(shouldReportServerError(await errorFor(req), req)).toBe(true); + }); + + it("drops the 405 from a POST to a route with no action", async () => { + const req = request("/", undefined, "POST"); + const error = await errorFor(req); + expect(error).toMatchObject({ status: 405, internal: true }); + expect(shouldReportServerError(error, req)).toBe(false); + }); +}); + +describe("static asset 404s", () => { + it("drops a stale hashed bundle even with a same-host referer", () => { + // Every deploy leaves clients requesting the previous build's assets. + const req = request("/assets/index-OLDHASH.js", `${ORIGIN}/leagues`); + expect(shouldReportServerError(routeError(404, true), req)).toBe(false); + }); + + it.each([ + "/assets/app-x1.css", + "/fonts/inter.woff2", + "/images/logo.png", + "/favicon.ico", + ])("drops a 404 for %s", (path) => { + expect( + shouldReportServerError( + routeError(404, true), + request(path, `${ORIGIN}/`), + ), + ).toBe(false); + }); + + it("still follows the referer rule for a non-asset path containing a dot", () => { + expect( + shouldReportServerError( + routeError(404, true), + request("/leagues/v1.2", `${ORIGIN}/leagues`), + ), + ).toBe(true); + expect( + shouldReportServerError(routeError(404, true), request("/leagues/v1.2")), + ).toBe(false); + }); +}); + +describe("React Router internal statuses that are not 404/405", () => { + it("reports an internal 400 (route is missing a loader)", () => { + expect( + shouldReportServerError( + routeError(400, true, "Bad Request"), + request("/leagues"), + ), + ).toBe(true); + }); + + it("reports an internal 403 (route does not match URL)", () => { + expect( + shouldReportServerError( + routeError(403, true, "Forbidden"), + request("/leagues"), + ), + ).toBe(true); + }); +}); + +describe("production shape: TLS terminated upstream", () => { + it("reports a 404 linked from our own site when the proxy strips https", () => { + // Express builds request.url from req.protocol, which is `http` inside the + // container. Real browsers send an https referer. Comparing full origins + // would never match, silencing every broken internal link. + const req = new Request("http://brackt.com/leagues/gone", { + headers: { referer: "https://brackt.com/leagues" }, + }); + expect(shouldReportServerError(routeError(404, true), req)).toBe(true); + }); + + it("still drops a cold scanner hit under that same shape", () => { + const req = new Request("http://brackt.com/blog/wp/v2/posts/999999"); + expect(shouldReportServerError(routeError(404, true), req)).toBe(false); + }); + + it("still drops a 404 linked from another site under that same shape", () => { + const req = new Request("http://brackt.com/nope", { + headers: { referer: "https://evil.example/" }, + }); + expect(shouldReportServerError(routeError(404, true), req)).toBe(false); + }); +}); diff --git a/app/lib/error-reporting.ts b/app/lib/error-reporting.ts new file mode 100644 index 0000000..19f1351 --- /dev/null +++ b/app/lib/error-reporting.ts @@ -0,0 +1,87 @@ +/** + * Decides which server-side errors are worth sending to Sentry. + * + * Automated scanners probe for CMS paths that have never existed here + * (`/blog/wp/v2/posts/999999`, `/wp-login.php`, a bare `POST /`). React Router + * throws for each one — a 404 when no route matches, a 405 when a route has no + * `action` — and every throw reaches `handleError` in `app/entry.server.tsx`. + * Reporting those burns the Sentry quota without ever describing a real bug. + */ +import { isRouteErrorResponse } from "react-router"; + +/** + * Statuses React Router uses to say "nothing here matched this request": + * 404 when no route matches the URL, 405 when the route has no `action` or the + * method is invalid. Its other internal statuses (400 "did not provide a + * `loader`", 403 "Route does not match URL") describe a misconfigured route + * rather than an unrecognised request, so those keep reporting. + */ +const UNMATCHED_REQUEST_STATUSES = new Set([404, 405]); + +/** + * Static assets 404 in bulk for reasons that are never actionable: scanners + * guessing filenames, and clients running stale HTML that still references the + * previous deploy's hashed bundles. + */ +const ASSET_EXT_RE = + /\.(css|js|mjs|map|png|jpe?g|gif|svg|webp|avif|ico|woff2?|ttf|eot)$/i; + +/** React Router stamps `internal: true` on the errors it generates itself. */ +function isInternalRouterError(error: unknown): boolean { + return (error as { internal?: unknown }).internal === true; +} + +/** + * True when the request was linked from a page on this same site. + * + * Compares host rather than origin on purpose. Production terminates TLS + * upstream and serves plain HTTP in the container, so `request.url` — which + * `@react-router/express` builds from `req.protocol` — says `http` while the + * browser sends an `https` referer. Comparing full origins would therefore + * never match in production. (`app/routes/leagues/$leagueId.server.ts` works + * around the same mismatch for invite URLs.) Protocol tells us nothing about + * whether the link was ours; host does. + */ +function hasSameHostReferer(request: Request): boolean { + const referer = request.headers.get("referer"); + if (!referer) return false; + try { + return new URL(referer).host === new URL(request.url).host; + } catch { + // Scanners send garbage in this header; a referer we can't parse isn't ours. + return false; + } +} + +/** + * Whether `error` should be reported to Sentry. + * + * Drops the 404s and 405s React Router generated for a request that matched + * nothing. Everything else is reported: real exceptions, 5xx, React Router's + * other internal statuses, and responses the app threw deliberately + * (`internal: false`), so a 403 from an ownership check still shows up. + * + * The exception is a request carrying a same-host `Referer`: a 404 reached from + * one of our own pages is a broken internal link, not a scanner, and stays + * visible in Sentry. Asset paths are excluded from that exception — a stale + * client requesting last deploy's bundle sends a same-host referer too, and + * would otherwise spike Sentry on every release. + */ +export function shouldReportServerError( + error: unknown, + request: Request, +): boolean { + if (!isRouteErrorResponse(error)) return true; + if (!isInternalRouterError(error)) return true; + if (!UNMATCHED_REQUEST_STATUSES.has(error.status)) return true; + + let pathname: string; + try { + pathname = new URL(request.url).pathname; + } catch { + pathname = ""; + } + if (ASSET_EXT_RE.test(pathname)) return false; + + return hasSameHostReferer(request); +} diff --git a/instrument.server.mjs b/instrument.server.mjs index 486b0b6..9d46ae9 100644 --- a/instrument.server.mjs +++ b/instrument.server.mjs @@ -5,12 +5,6 @@ Sentry.init({ enabled: process.env.NODE_ENV === "production", sendDefaultPii: true, tracesSampleRate: 0, - ignoreErrors: [ - /No route matches URL ".*\.css"/, - /No route matches URL ".*\.js"/, - /No route matches URL ".*\.(php|env|xml|aspx|asp|bak|sql|ini)"/i, - /No route matches URL ".*\/(wp-admin|wp-login|phpmyadmin|xmlrpc)"/i, - ], beforeSend(event) { const msg = event.exception?.values?.[0]?.value ?? ""; // Drop React Flight protocol probe errors (e.g. $1:aa:aa in multipart body) diff --git a/server/app.ts b/server/app.ts index 2de2ca3..0835bef 100644 --- a/server/app.ts +++ b/server/app.ts @@ -11,9 +11,11 @@ export const app = express(); app.use((_, __, next) => DatabaseContext.run(db, next)); -// Block common bot probe paths before React Router (and Sentry) see them +// Block common bot probe paths before React Router (and Sentry) see them. +// `blog` is here only because scanners hammer /blog/wp/v2/* — drop it from this +// list if a real blog route is ever added. const BOT_PROBE_RE = - /\.(php|env|htaccess|aspx|asp|jsp|config|bak|sql|ini|swp|DS_Store)$|^\/(wp-admin|wp-login|phpmyadmin|xmlrpc|server-status|cgi-bin|shell|cmd|console|actuator)(\/|$)/i; + /\.(php|env|htaccess|aspx|asp|jsp|config|bak|sql|ini|swp|DS_Store)$|^\/(wp-admin|wp-login|phpmyadmin|xmlrpc|server-status|cgi-bin|shell|cmd|console|actuator|blog)(\/|$)/i; app.use((req, res, next) => { if (BOT_PROBE_RE.test(req.path)) {