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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ESrxBMZMx2BD9rcKTCgLe4
This commit is contained in:
parent
41a0237a87
commit
ed9c62571b
2 changed files with 128 additions and 12 deletions
|
|
@ -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);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue