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); }