From 41a0237a875b6cb85d9bcd3e4c7ff47057c5dfe3 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 20:11:46 +0000 Subject: [PATCH 1/2] Stop reporting bot-scanner 404s to Sentry Automated scanners probing for WordPress paths (/blog/wp/v2/posts/*, POST /) generate a React Router 404 or 405 on every hit, and handleError forwarded all of them to Sentry, exhausting the quota. The existing defence was a list of ignoreErrors regexes that needed a new entry for each scanner pattern. Filter on what the error is instead: shouldReportServerError drops 4xx responses React Router generated itself (internal: true) for requests that matched nothing. Responses the app throws deliberately still report, as do all 5xx and real exceptions. A request carrying a same-origin Referer is still reported, so a broken internal link remains visible in Sentry -- only cold scanner hits are dropped. No HTTP response changes; every URL returns the status and page it did before. Also adds /blog to the Express bot-probe filter so the highest-volume path 404s without an SSR render, and removes the now-redundant route-404 ignoreErrors entries. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01ESrxBMZMx2BD9rcKTCgLe4 --- app/entry.server.tsx | 12 +- app/lib/__tests__/error-reporting.test.ts | 152 ++++++++++++++++++++++ app/lib/error-reporting.ts | 49 +++++++ instrument.server.mjs | 6 - server/app.ts | 6 +- 5 files changed, 215 insertions(+), 10 deletions(-) create mode 100644 app/lib/__tests__/error-reporting.test.ts create mode 100644 app/lib/error-reporting.ts 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..8c1a348 --- /dev/null +++ b/app/lib/__tests__/error-reporting.test.ts @@ -0,0 +1,152 @@ +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); + }); +}); diff --git a/app/lib/error-reporting.ts b/app/lib/error-reporting.ts new file mode 100644 index 0000000..254295f --- /dev/null +++ b/app/lib/error-reporting.ts @@ -0,0 +1,49 @@ +/** + * 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"; + +/** 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 origin. */ +function hasSameOriginReferer(request: Request): boolean { + const referer = request.headers.get("referer"); + if (!referer) return false; + try { + return new URL(referer).origin === new URL(request.url).origin; + } 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 only the 4xx responses React Router generated for a request that + * matched nothing — that is, unrecognised URLs and methods. Everything else is + * reported, including 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-origin `Referer`: a 404 reached + * from one of our own pages is a broken internal link, not a scanner, and stays + * visible in Sentry. + */ +export function shouldReportServerError( + error: unknown, + request: Request, +): boolean { + if (!isRouteErrorResponse(error)) return true; + if (!isInternalRouterError(error)) return true; + if (error.status < 400 || error.status > 499) return true; + return hasSameOriginReferer(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)) { -- 2.45.3 From ed9c62571b38fbcf68ad173eed1bb4b39da30b6e Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 21:18:33 +0000 Subject: [PATCH 2/2] Fix three defects in the Sentry 404 filter Code review of 41a0237 found the same-origin Referer exception never fires in production, which defeats the guarantee that change was built on: broken internal links stay visible while scanners are dropped. @react-router/express builds request.url from req.protocol and nothing sets `trust proxy`, so request.url reads http:// inside the container while browsers send an https:// Referer. Comparing full origins never matched, so every router-internal 4xx was dropped. Compare host instead -- protocol says nothing about whether a link was ours, and this is the same mismatch $leagueId.server.ts already works around for invite URLs. Adding `trust proxy` would fix it too but changes req.ip and req.secure server-wide for one comparison. Removing ignoreErrors also dropped the .css/.js filters with nothing to replace them. Stale clients request the previous deploy's hashed bundles with a same-host Referer, so once the Referer check worked, every deploy would spike Sentry. Assets now never report, referer or not. Narrow the dropped statuses from all 4xx to 404 and 405, the two that mean nothing matched the request. React Router's internal 400 (route missing a loader) and 403 (route does not match URL) are real misconfigurations and were being swallowed whenever no Referer was sent. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01ESrxBMZMx2BD9rcKTCgLe4 --- app/lib/__tests__/error-reporting.test.ts | 78 +++++++++++++++++++++++ app/lib/error-reporting.ts | 62 ++++++++++++++---- 2 files changed, 128 insertions(+), 12 deletions(-) diff --git a/app/lib/__tests__/error-reporting.test.ts b/app/lib/__tests__/error-reporting.test.ts index 8c1a348..ed46e5b 100644 --- a/app/lib/__tests__/error-reporting.test.ts +++ b/app/lib/__tests__/error-reporting.test.ts @@ -150,3 +150,81 @@ describe("shouldReportServerError against real React Router errors", () => { 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 index 254295f..19f1351 100644 --- a/app/lib/error-reporting.ts +++ b/app/lib/error-reporting.ts @@ -9,17 +9,44 @@ */ 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 origin. */ -function hasSameOriginReferer(request: Request): boolean { +/** + * 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).origin === new URL(request.url).origin; + 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; @@ -29,14 +56,16 @@ function hasSameOriginReferer(request: Request): boolean { /** * Whether `error` should be reported to Sentry. * - * Drops only the 4xx responses React Router generated for a request that - * matched nothing — that is, unrecognised URLs and methods. Everything else is - * reported, including responses the app threw deliberately (`internal: false`), - * so a 403 from an ownership check still shows up. + * 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-origin `Referer`: a 404 reached - * from one of our own pages is a broken internal link, not a scanner, and stays - * visible in Sentry. + * 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, @@ -44,6 +73,15 @@ export function shouldReportServerError( ): boolean { if (!isRouteErrorResponse(error)) return true; if (!isInternalRouterError(error)) return true; - if (error.status < 400 || error.status > 499) return true; - return hasSameOriginReferer(request); + 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); } -- 2.45.3