claude/gifted-allen-npeql8 #150
5 changed files with 331 additions and 10 deletions
|
|
@ -1,18 +1,26 @@
|
||||||
import * as Sentry from "@sentry/react-router";
|
import * as Sentry from "@sentry/react-router";
|
||||||
import { PassThrough } from "node:stream";
|
import { PassThrough } from "node:stream";
|
||||||
import { logger } from "~/lib/logger";
|
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 { createReadableStreamFromReadable } from "@react-router/node";
|
||||||
import { ServerRouter } from "react-router";
|
import { ServerRouter } from "react-router";
|
||||||
import { isbot } from "isbot";
|
import { isbot } from "isbot";
|
||||||
import type { RenderToPipeableStreamOptions } from "react-dom/server";
|
import type { RenderToPipeableStreamOptions } from "react-dom/server";
|
||||||
import { renderToPipeableStream } from "react-dom/server";
|
import { renderToPipeableStream } from "react-dom/server";
|
||||||
|
|
||||||
export const handleError = Sentry.createSentryHandleError({
|
const sentryHandleError = Sentry.createSentryHandleError({
|
||||||
logErrors: true,
|
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;
|
export const streamTimeout = 5_000;
|
||||||
|
|
||||||
async function handleRequest(
|
async function handleRequest(
|
||||||
|
|
|
||||||
230
app/lib/__tests__/error-reporting.test.ts
Normal file
230
app/lib/__tests__/error-reporting.test.ts
Normal file
|
|
@ -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);
|
||||||
|
});
|
||||||
|
});
|
||||||
87
app/lib/error-reporting.ts
Normal file
87
app/lib/error-reporting.ts
Normal file
|
|
@ -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);
|
||||||
|
}
|
||||||
|
|
@ -5,12 +5,6 @@ Sentry.init({
|
||||||
enabled: process.env.NODE_ENV === "production",
|
enabled: process.env.NODE_ENV === "production",
|
||||||
sendDefaultPii: true,
|
sendDefaultPii: true,
|
||||||
tracesSampleRate: 0,
|
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) {
|
beforeSend(event) {
|
||||||
const msg = event.exception?.values?.[0]?.value ?? "";
|
const msg = event.exception?.values?.[0]?.value ?? "";
|
||||||
// Drop React Flight protocol probe errors (e.g. $1:aa:aa in multipart body)
|
// Drop React Flight protocol probe errors (e.g. $1:aa:aa in multipart body)
|
||||||
|
|
|
||||||
|
|
@ -11,9 +11,11 @@ export const app = express();
|
||||||
|
|
||||||
app.use((_, __, next) => DatabaseContext.run(db, next));
|
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 =
|
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) => {
|
app.use((req, res, next) => {
|
||||||
if (BOT_PROBE_RE.test(req.path)) {
|
if (BOT_PROBE_RE.test(req.path)) {
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue