From c210ea2a781b7017158065e0bef1c40b7b7af7b8 Mon Sep 17 00:00:00 2001 From: Arpit Dalal Date: Sun, 27 Sep 2026 00:41:21 -0400 Subject: [PATCH 1/6] feat: adopt 8 low-risk Epic Stack improvements (#7) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cherry-picks the Tier 1 items from upstream Epic Stack. This repo is already ahead of upstream `main` on react-router, remix-utils, tailwindcss, eslint, zod and typescript, so these are targeted ports rather than a sync. - Guard `decodeURIComponent` in the morgan `url` token. morgan evaluates its tokens from the response `finished` event, so a URIError there escapes as an uncaught exception rather than failing one log line. Extracted to `server/utils/request-url.ts` so the guard is unit-testable. - Replace `chalk` with `node:util`'s `styleText` and drop the dependency. The dev-server banner still renders coloured output. - Set `appType: 'custom'` on the Vite dev server so its SPA HTML fallback stops competing with the manually-wired `createRequestHandler`. - Opt in to the React Router 8 config defaults: `ssr: true`, `routeDiscovery: { mode: 'initial' }`, and drop the now-redundant `splitRouteModules`. `mode: 'initial'` ships the whole route manifest with the initial document; the built manifest chunk still holds all 15 routes and the chunk count is unchanged at 43, so route splitting did not collapse. - Add `app/utils/timing.server.ts` and wire `Server-Timing` into the root loader and the document render. A root `headers` export is required for the loader's header to reach the client; without it only the render metric is emitted. `curl -sI /talks` now returns `root_loader;dur=…,honeypot;desc=…;dur=…,render;desc=…;dur=…`. - Add `app/utils/sentry-event-filters.ts` and use it in `handleError` so React Router's "no loader/action" and "invalid request method" errors, which scanners trigger constantly, are logged but not reported to Sentry. Healthcheck filtering is left out on purpose: `server/utils/monitoring.ts` already drops those in `tracesSampler`. - Add `ALLOW_INDEXING` support: the schema, `getEnv()`, `.env.example`, an `X-Robots-Tag: noindex, nofollow` middleware and a matching `` tag, for non-production deploys. The root loader's payload moved into `getRootData` so `RootLoaderData` can be derived from it. `data()` makes `Awaited>` a `DataWithResponseInit`, and `talks.tsx` reads the root match's `loaderData` out of `matches` where nothing unwraps it for us. Task 7 (drop the `#app/*`/`#types/*` mirrors from `tsconfig.json` `paths`) is NOT included. It does not work: TypeScript does not do extension substitution on a wildcard `imports` target, so `"#app/*": "./app/*"` resolves `#app/utils/misc` to the literal `./app/utils/misc` and `tsc` reports TS2307 across the whole app. Reproduced on TS 5.9 and 6.0.3 under both `bundler` and `nodenext`. Fixing it would mean adding extensions to the `imports` patterns, which is out of scope here. Refs #7 --- .env.example | 8 +- app/entry.server.tsx | 26 ++++++- app/root.tsx | 69 +++++++++++++++-- app/routes/_marketing+/talks.tsx | 5 +- app/utils/env.server.ts | 6 ++ app/utils/schemas.ts | 1 + app/utils/sentry-event-filters.ts | 43 +++++++++++ app/utils/timing.server.ts | 88 +++++++++++++++++++++ package-lock.json | 1 - package.json | 1 - react-router.config.ts | 15 +++- server/index.ts | 29 +++++-- server/utils/request-url.ts | 16 ++++ tests/request-url.test.ts | 21 +++++ tests/sentry-event-filters.test.ts | 119 +++++++++++++++++++++++++++++ tests/server.smoke.test.ts | 13 ++++ tests/timing.test.ts | 80 +++++++++++++++++++ 17 files changed, 511 insertions(+), 30 deletions(-) create mode 100644 app/utils/sentry-event-filters.ts create mode 100644 app/utils/timing.server.ts create mode 100644 server/utils/request-url.ts create mode 100644 tests/request-url.test.ts create mode 100644 tests/sentry-event-filters.test.ts create mode 100644 tests/timing.test.ts diff --git a/.env.example b/.env.example index cfc66f3..5d89543 100644 --- a/.env.example +++ b/.env.example @@ -15,4 +15,10 @@ UMAMI_WEBSITE_ID="your-umami-website-id" UMAMI_DOMAINS="your-umami-domains" # comma separated list of domains the umami script should track - https://umami.is/docs/tracker-configuration#data-domains UMAMI_DOMAIN="your-umami-domain" # umami domain - https://umami.is/docs/bypass-ad-blockers#using-environment-variables-self-hosting UMAMI_SCRIPT_NAME="your-umami-script-name" # umami script name - https://umami.is/docs/bypass-ad-blockers#using-environment-variables-self-hosting -UMAMI_PUBLIC_ANALYTICS_URL="your-umami-public-analytics-url" # https://umami.is/docs/enable-share-url \ No newline at end of file +UMAMI_PUBLIC_ANALYTICS_URL="your-umami-public-analytics-url" # https://umami.is/docs/enable-share-url + +# Set to "false" on non-production deploys (staging, previews) so they are not +# indexed and do not compete with production in search results. It emits +# `X-Robots-Tag: noindex, nofollow` plus a tag. Leave it +# unset (or "true") in production. +ALLOW_INDEXING="false" diff --git a/app/entry.server.tsx b/app/entry.server.tsx index 5a07275..0e26816 100644 --- a/app/entry.server.tsx +++ b/app/entry.server.tsx @@ -1,7 +1,7 @@ import { PassThrough } from 'node:stream' +import { styleText } from 'node:util' import { createReadableStreamFromReadable } from '@react-router/node' import * as Sentry from '@sentry/node' -import chalk from 'chalk' import { isbot } from 'isbot' import { renderToPipeableStream } from 'react-dom/server' import { @@ -12,6 +12,8 @@ import { } from 'react-router' import { getEnv, init } from './utils/env.server' import { NonceProvider } from './utils/nonce-provider' +import { isExpectedReactRouterErrorMessage } from './utils/sentry-event-filters' +import { makeTimings } from './utils/timing.server' export const streamTimeout = 5000 @@ -37,6 +39,9 @@ export default async function handleRequest(...args: DocRequestArgs) { const nonce = request.headers.get('x-csp-nonce') ?? '' return new Promise(async (resolve, reject) => { let didError = false + // NOTE: this timing will only include things that are rendered in the shell + // and will not include suspended components and deferred loaders + const timings = makeTimings('render', 'renderToPipeableStream') const { pipe, abort } = renderToPipeableStream( @@ -50,6 +55,7 @@ export default async function handleRequest(...args: DocRequestArgs) { [callbackName]: () => { const body = new PassThrough() responseHeaders.set('Content-Type', 'text/html') + responseHeaders.append('Server-Timing', timings.toString()) resolve( new Response(createReadableStreamFromReadable(body), { headers: responseHeaders, @@ -88,11 +94,23 @@ export function handleError( if (request.signal.aborted) { return } + + // Bots and scanners request every URL in the sitemap with methods the + // routes do not handle, and React Router throws for each of those. They + // are expected answers to invalid traffic, not bugs, so they get logged + // (they still show up in `fly logs`) but never reported to Sentry. + if ( + error instanceof Error && + isExpectedReactRouterErrorMessage(error.message) + ) { + console.error(styleText('red', String(error.stack))) + return + } + if (error instanceof Error) { - console.error(chalk.red(error.stack)) - void Sentry.captureException(error) + console.error(styleText('red', String(error.stack))) } else { console.error(error) - void Sentry.captureException(error) } + void Sentry.captureException(error) } diff --git a/app/root.tsx b/app/root.tsx index 8714535..bf27e5d 100644 --- a/app/root.tsx +++ b/app/root.tsx @@ -1,4 +1,5 @@ import { + data, href, Link, Links, @@ -29,6 +30,7 @@ import { honeypot } from '#app/utils/honeypot.server' import { getDomainUrl, getUrl } from '#app/utils/misc' import { useNonce } from '#app/utils/nonce-provider' import { getSocialMetas } from '#app/utils/seo' +import { makeTimings, time } from '#app/utils/timing.server' import { type Theme } from '#types/index' import { type Route } from './+types/root' @@ -67,20 +69,64 @@ export const meta: Route.MetaFunction = ({ loaderData }) => { ] } -export async function loader({ request }: Route.LoaderArgs) { - const honeyProps = await honeypot.getInputProps() +/** + * Split out of `loader` so `RootLoaderData` can be derived from it. The loader + * wraps this payload in `data()` to attach a `Server-Timing` header, which + * means `Awaited>` is a `DataWithResponseInit` rather + * than the payload. `useLoaderData`/`useRouteLoaderData` unwrap it, but the + * `meta` functions in child routes read the root match's `loaderData` straight + * out of `matches`, where they need the payload type explicitly. + */ +async function getRootData(request: Request) { + const timings = makeTimings('root loader') + const honeyProps = await time(() => honeypot.getInputProps(), { + timings, + type: 'honeypot', + desc: 'get honeypot input props in root', + }) return { - requestInfo: { - hints: getHints(request), - origin: getDomainUrl(request), - path: new URL(request.url).pathname, + timings, + payload: { + requestInfo: { + hints: getHints(request), + origin: getDomainUrl(request), + path: new URL(request.url).pathname, + }, + ENV: getEnv(), + honeyProps, }, - ENV: getEnv(), - honeyProps, } } +export type RootLoaderData = Awaited>['payload'] + +export async function loader({ request }: Route.LoaderArgs) { + const { payload, timings } = await getRootData(request) + + return data(payload, { + headers: { 'Server-Timing': timings.toString() }, + }) +} + +/** + * React Router only puts a loader's response headers on the document response + * if a matched route exports a `headers` function, so without this the root + * loader's `Server-Timing` never leaves the server. `entry.server.tsx` appends + * the render timings to the same header. + */ +export const headers: Route.HeadersFunction = ({ + loaderHeaders, + parentHeaders, +}) => { + const headers = new Headers(parentHeaders) + const serverTiming = loaderHeaders.get('Server-Timing') + if (serverTiming) { + headers.set('Server-Timing', serverTiming) + } + return headers +} + function Document({ children, nonce, @@ -105,6 +151,13 @@ function Document({ + {env.ALLOW_INDEXING === 'false' ? ( + // Staging deploys opt out of indexing with ALLOW_INDEXING=false + // so they do not compete with production in search results. The + // server sends the matching X-Robots-Tag header; this covers + // crawlers that read the document instead. + + ) : null} diff --git a/app/routes/_marketing+/talks.tsx b/app/routes/_marketing+/talks.tsx index 234f687..8b328b7 100644 --- a/app/routes/_marketing+/talks.tsx +++ b/app/routes/_marketing+/talks.tsx @@ -7,7 +7,7 @@ import { } from '#app/components/highlight' import { LineGlow } from '#app/components/line-glow' import { TalkCard } from '#app/components/talk-card' -import { type loader as rootLoader } from '#app/root' +import { type RootLoaderData } from '#app/root' import { formatDateWithHints } from '#app/utils/client-hints' import { getUrl } from '#app/utils/misc' import { getSocialMetas } from '#app/utils/seo' @@ -20,8 +20,7 @@ const PAGE_DESCRIPTION = export const meta: Route.MetaFunction = ({ matches }) => { const rootMatch = matches.find((m) => m?.id === 'root') - const rootData = rootMatch?.loaderData as - Awaited> | undefined + const rootData = rootMatch?.loaderData as RootLoaderData | undefined const requestInfo = rootData?.requestInfo return [ { title: PAGE_TITLE }, diff --git a/app/utils/env.server.ts b/app/utils/env.server.ts index 64cc380..1493b3c 100644 --- a/app/utils/env.server.ts +++ b/app/utils/env.server.ts @@ -42,6 +42,12 @@ export function getEnv() { UMAMI_DOMAINS: process.env.UMAMI_DOMAINS, UMAMI_SCRIPT_NAME: process.env.UMAMI_SCRIPT_NAME, UMAMI_PUBLIC_ANALYTICS_URL: process.env.UMAMI_PUBLIC_ANALYTICS_URL, + // Setting this to 'false' tells the server to send an + // `X-Robots-Tag: noindex, nofollow` response header and the document to + // render a matching `` tag. Intended for non-production + // deploys (staging, previews) so they do not compete with production in + // search results. Leave it unset or 'true' in production. + ALLOW_INDEXING: process.env.ALLOW_INDEXING, } } diff --git a/app/utils/schemas.ts b/app/utils/schemas.ts index d534919..ec93743 100644 --- a/app/utils/schemas.ts +++ b/app/utils/schemas.ts @@ -66,6 +66,7 @@ export const envSchema = z.object({ UMAMI_DOMAINS: z.string(), UMAMI_SCRIPT_NAME: z.string(), UMAMI_PUBLIC_ANALYTICS_URL: z.string().optional(), + ALLOW_INDEXING: z.enum(['true', 'false']).optional(), }) // Type exports for TypeScript inference diff --git a/app/utils/sentry-event-filters.ts b/app/utils/sentry-event-filters.ts new file mode 100644 index 0000000..9e5f077 --- /dev/null +++ b/app/utils/sentry-event-filters.ts @@ -0,0 +1,43 @@ +/** + * Pure helpers for dropping expected Sentry noise from a public site — the + * bots and scanners that hit `/sitemap.xml` and then request every path in it + * with methods the routes do not handle. + * + * React Router throws a 405-style error for a request with no matching + * `loader` or `action`, and for unsupported request methods. Those are expected + * answers to invalid traffic, not bugs, so they should not page anyone. + * + * NOTE: Healthcheck noise is *not* handled here. `server/utils/monitoring.ts` + * drops those transactions in its `tracesSampler` and the client sets + * `denyUrls`, so a filter for them would be dead code. + */ + +export const EXPECTED_REACT_ROUTER_ERROR_PATTERNS = [ + /did not provide an `action`/i, + /did not provide a `loader`/i, + /^Invalid request method /i, +] + +export function isExpectedReactRouterErrorMessage(message: string): boolean { + return EXPECTED_REACT_ROUTER_ERROR_PATTERNS.some((pattern) => + pattern.test(message), + ) +} + +type ErrorEventLike = { + message?: string + exception?: { values?: Array<{ value?: string | null } | null> | null } | null +} + +export function getEventErrorMessages(event: ErrorEventLike): string[] { + const messages: string[] = [] + for (const value of event.exception?.values ?? []) { + if (value?.value) messages.push(value.value) + } + if (event.message) messages.push(event.message) + return messages +} + +export function shouldDropErrorEvent(event: ErrorEventLike): boolean { + return getEventErrorMessages(event).some(isExpectedReactRouterErrorMessage) +} diff --git a/app/utils/timing.server.ts b/app/utils/timing.server.ts new file mode 100644 index 0000000..09ec510 --- /dev/null +++ b/app/utils/timing.server.ts @@ -0,0 +1,88 @@ +export type Timings = Record< + string, + Array< + { desc?: string } & ( + { time: number; start?: never } | { time?: never; start: number } + ) + > +> + +export function makeTimings(type: string, desc?: string) { + const timings: Timings = { + [type]: [{ desc, start: performance.now() }], + } + Object.defineProperty(timings, 'toString', { + value: function () { + return getServerTimeHeader(timings) + }, + enumerable: false, + }) + return timings +} + +function createTimer(type: string, desc?: string) { + const start = performance.now() + return { + end(timings: Timings) { + let timingType = timings[type] + + if (!timingType) { + timingType = timings[type] = [] + } + timingType.push({ desc, time: performance.now() - start }) + }, + } +} + +export async function time( + fn: Promise | (() => ReturnType | Promise), + { + type, + desc, + timings, + }: { + type: string + desc?: string + timings?: Timings + }, +): Promise { + const timer = createTimer(type, desc) + const promise = typeof fn === 'function' ? fn() : fn + if (!timings) return promise + + const result = await promise + + timer.end(timings) + return result +} + +export function getServerTimeHeader(timings?: Timings) { + if (!timings) return '' + return Object.entries(timings) + .map(([key, timingInfos]) => { + const dur = timingInfos + .reduce((acc, timingInfo) => { + const time = timingInfo.time ?? performance.now() - timingInfo.start + return acc + time + }, 0) + .toFixed(1) + const desc = timingInfos + .map((t) => t.desc) + .filter(Boolean) + .join(' & ') + return [ + key.replaceAll(/(:| |@|=|;|,|\/|\\)/g, '_'), + desc ? `desc=${JSON.stringify(desc)}` : null, + `dur=${dur}`, + ] + .filter(Boolean) + .join(';') + }) + .join(',') +} + +export function combineServerTimings(headers1: Headers, headers2: Headers) { + const newHeaders = new Headers(headers1) + newHeaders.append('Server-Timing', headers2.get('Server-Timing') ?? '') + return newHeaders.get('Server-Timing') ?? '' +} diff --git a/package-lock.json b/package-lock.json index 2924bf9..409bc22 100644 --- a/package-lock.json +++ b/package-lock.json @@ -24,7 +24,6 @@ "@tailwindcss/postcss": "^4.3.3", "@tailwindcss/typography": "^0.5.20", "address": "^2.0.3", - "chalk": "^5.6.2", "class-variance-authority": "^0.7.1", "close-with-grace": "^2.5.0", "clsx": "^2.1.1", diff --git a/package.json b/package.json index 6df2306..0b12708 100644 --- a/package.json +++ b/package.json @@ -62,7 +62,6 @@ "@tailwindcss/postcss": "^4.3.3", "@tailwindcss/typography": "^0.5.20", "address": "^2.0.3", - "chalk": "^5.6.2", "class-variance-authority": "^0.7.1", "close-with-grace": "^2.5.0", "clsx": "^2.1.1", diff --git a/react-router.config.ts b/react-router.config.ts index 888d000..bc8d658 100644 --- a/react-router.config.ts +++ b/react-router.config.ts @@ -1,10 +1,17 @@ import { type Config } from '@react-router/dev/config' export default { - // In v8 the future.v8_splitRouteModules flag moved here and defaults to - // true. v8_viteEnvironmentApi is gone because the Vite Environment API is - // now always on. - splitRouteModules: true, + // Defaults to true. Set to false to enable SPA for all routes. + ssr: true, + + // This is a small site (12 routes). Opting out of lazy route discovery + // ships the whole route manifest with the initial HTML document, which + // removes a round-trip on every client-side navigation. + routeDiscovery: { mode: 'initial' }, + + // `future.v8_splitRouteModules` moved to the top level in v8 and defaults to + // true, so the explicit `splitRouteModules: true` is redundant. + // `v8_viteEnvironmentApi` is gone — the Vite Environment API is always on. future: { unstable_optimizeDeps: true, }, diff --git a/server/index.ts b/server/index.ts index 708c96b..50aa94e 100644 --- a/server/index.ts +++ b/server/index.ts @@ -1,8 +1,8 @@ import crypto from 'node:crypto' +import { styleText } from 'node:util' import { createRequestHandler } from '@react-router/express' import * as Sentry from '@sentry/node' import { ip as ipAddress } from 'address' -import chalk from 'chalk' import closeWithGrace from 'close-with-grace' import compression from 'compression' import express from 'express' @@ -11,10 +11,12 @@ import getPort, { portNumbers } from 'get-port' import helmet from 'helmet' import morgan from 'morgan' import { type ServerBuild, RouterContextProvider } from 'react-router' +import { decodeRequestUrl } from './utils/request-url.js' const MODE = process.env.NODE_ENV ?? 'development' const IS_PROD = MODE === 'production' const IS_DEV = MODE === 'development' +const ALLOW_INDEXING = process.env.ALLOW_INDEXING !== 'false' const SENTRY_ENABLED = IS_PROD && process.env.SENTRY_DSN if (SENTRY_ENABLED) { @@ -26,6 +28,9 @@ const viteDevServer = IS_PROD : await import('vite').then((vite) => vite.createServer({ server: { middlewareMode: true }, + // We tell Vite we are running a custom app instead of + // the SPA default so it doesn't run HTML middleware + appType: 'custom', }), ) @@ -66,6 +71,13 @@ app.use(compression()) // http://expressjs.com/en/advanced/best-practice-security.html#at-a-minimum-disable-x-powered-by-header app.disable('x-powered-by') +if (!ALLOW_INDEXING) { + app.use((_, res, next) => { + res.set('X-Robots-Tag', 'noindex, nofollow') + next() + }) +} + if (viteDevServer) { app.use(viteDevServer.middlewares) } else { @@ -87,7 +99,7 @@ app.get(['/img/{*splat}', '/favicons/{*splat}'], (_req, res) => { return }) -morgan.token('url', (req) => decodeURIComponent(req.url ?? '')) +morgan.token('url', (req) => decodeRequestUrl(req.url)) app.use( morgan('tiny', { skip: (req, res) => @@ -234,7 +246,8 @@ if (!portAvailable && !IS_DEV) { const server = app.listen(portToUse, () => { if (!portAvailable) { console.warn( - chalk.yellow( + styleText( + 'yellow', `⚠️ Port ${desiredPort} is not available, using ${portToUse} instead.`, ), ) @@ -252,9 +265,9 @@ const server = app.listen(portToUse, () => { console.log( ` -${chalk.bold('Local:')} ${chalk.cyan(localUrl)} -${lanUrl ? `${chalk.bold('On Your Network:')} ${chalk.cyan(lanUrl)}` : ''} -${chalk.bold('Press Ctrl+C to stop')} +${styleText('bold', 'Local:')} ${styleText('cyan', localUrl)} +${lanUrl ? `${styleText('bold', 'On Your Network:')} ${styleText('cyan', lanUrl)}` : ''} +${styleText('bold', 'Press Ctrl+C to stop')} `.trim(), ) }) @@ -264,8 +277,8 @@ closeWithGrace(async ({ err }) => { server.close((e) => (e ? reject(e) : resolve('ok'))) }) if (err) { - console.error(chalk.red(err)) - console.error(chalk.red(err.stack)) + console.error(styleText('red', String(err))) + console.error(styleText('red', String(err.stack))) if (SENTRY_ENABLED) { Sentry.captureException(err) await Sentry.flush(500) diff --git a/server/utils/request-url.ts b/server/utils/request-url.ts new file mode 100644 index 0000000..8d45c00 --- /dev/null +++ b/server/utils/request-url.ts @@ -0,0 +1,16 @@ +/** + * Decodes a request URL for logging. + * + * morgan evaluates its format tokens from the response's `finished` event, so a + * token that throws does not fail a log line — it escapes as an uncaught + * exception and takes the process down mid-request. `decodeURIComponent` throws + * a `URIError` on a malformed percent-encoding, and scanners send paths like + * `/%` constantly, so fall back to the raw URL. + */ +export function decodeRequestUrl(url: string | undefined): string { + try { + return decodeURIComponent(url ?? '') + } catch { + return url ?? '' + } +} diff --git a/tests/request-url.test.ts b/tests/request-url.test.ts new file mode 100644 index 0000000..eaa89d9 --- /dev/null +++ b/tests/request-url.test.ts @@ -0,0 +1,21 @@ +import { describe, expect, it } from 'vitest' +import { decodeRequestUrl } from '../server/utils/request-url' + +describe('decodeRequestUrl', () => { + it('decodes a well-formed URL', () => { + expect(decodeRequestUrl('/talks?q=a%20b')).toBe('/talks?q=a b') + }) + + it('falls back to the raw URL when decoding throws', () => { + // morgan evaluates its tokens from the response's `finished` event, so a + // throw here escapes as an uncaught exception and takes the process down + // rather than failing a single log line. + expect(decodeRequestUrl('/%')).toBe('/%') + expect(decodeRequestUrl('/%zz')).toBe('/%zz') + expect(decodeRequestUrl('/%e0%a4%a')).toBe('/%e0%a4%a') + }) + + it('treats a missing URL as empty rather than throwing', () => { + expect(decodeRequestUrl(undefined)).toBe('') + }) +}) diff --git a/tests/sentry-event-filters.test.ts b/tests/sentry-event-filters.test.ts new file mode 100644 index 0000000..ceaad1c --- /dev/null +++ b/tests/sentry-event-filters.test.ts @@ -0,0 +1,119 @@ +import { describe, expect, it } from 'vitest' +import { + EXPECTED_REACT_ROUTER_ERROR_PATTERNS, + getEventErrorMessages, + isExpectedReactRouterErrorMessage, + shouldDropErrorEvent, +} from '#app/utils/sentry-event-filters' + +describe('isExpectedReactRouterErrorMessage', () => { + it('matches a request with no matching action', () => { + expect( + isExpectedReactRouterErrorMessage( + 'You made a POST request to "/" but did not provide an `action` for route "root", so there is no way to handle the request.', + ), + ).toBe(true) + }) + + it('matches a request with no matching loader', () => { + expect( + isExpectedReactRouterErrorMessage( + 'You made a GET request to "/resources/theme-switch" but did not provide a `loader` for route "routes/resources/theme-switch", so there is no way to handle the request.', + ), + ).toBe(true) + }) + + it('matches an unsupported request method', () => { + expect( + isExpectedReactRouterErrorMessage('Invalid request method "OPTIONS"'), + ).toBe(true) + }) + + it('does not match an unrelated error', () => { + expect( + isExpectedReactRouterErrorMessage('Database connection failed'), + ).toBe(false) + expect(isExpectedReactRouterErrorMessage('')).toBe(false) + expect( + isExpectedReactRouterErrorMessage('Error: getServerSideProps failed'), + ).toBe(false) + }) + + // The `action`/`loader` patterns are unanchored, which is what lets them + // survive React Router rewording the surrounding text between versions. The + // cost is that a genuine bug whose message happens to contain the phrase is + // dropped too. That is inherited from upstream deliberately: React Router's + // own message is not part of its public API, so anchoring is not an option. + it('matches unanchored, so a real error containing the phrase is dropped too', () => { + expect( + isExpectedReactRouterErrorMessage( + 'Newsletter route did not provide an `action` handler', + ), + ).toBe(true) + }) + + it('exposes its patterns for callers that want to inspect them', () => { + expect(EXPECTED_REACT_ROUTER_ERROR_PATTERNS).toHaveLength(3) + }) +}) + +describe('getEventErrorMessages', () => { + it('collects the top-level message', () => { + expect(getEventErrorMessages({ message: 'boom' })).toEqual(['boom']) + }) + + it('collects every exception value, skipping empty ones', () => { + expect( + getEventErrorMessages({ + exception: { values: [{ value: 'first' }, { value: null }, {}] }, + }), + ).toEqual(['first']) + }) + + it('returns the exception values before the message', () => { + expect( + getEventErrorMessages({ + message: 'top', + exception: { values: [{ value: 'nested' }] }, + }), + ).toEqual(['nested', 'top']) + }) + + it('tolerates a missing or null exception', () => { + expect(getEventErrorMessages({})).toEqual([]) + expect(getEventErrorMessages({ exception: null })).toEqual([]) + expect(getEventErrorMessages({ exception: { values: null } })).toEqual([]) + }) +}) + +describe('shouldDropErrorEvent', () => { + it('drops an event whose exception value is an expected router error', () => { + expect( + shouldDropErrorEvent({ + exception: { + values: [ + { + value: + 'You made a POST request to "/" but did not provide an `action` for route "root", so there is no way to handle the request.', + }, + ], + }, + }), + ).toBe(true) + }) + + it('drops an event whose message is an expected router error', () => { + expect( + shouldDropErrorEvent({ message: 'Invalid request method "TRACE"' }), + ).toBe(true) + }) + + it('keeps an event that is a real failure', () => { + expect( + shouldDropErrorEvent({ + exception: { values: [{ value: 'Unexpected server failure' }] }, + }), + ).toBe(false) + expect(shouldDropErrorEvent({})).toBe(false) + }) +}) diff --git a/tests/server.smoke.test.ts b/tests/server.smoke.test.ts index 7245b9b..9a2aa2d 100644 --- a/tests/server.smoke.test.ts +++ b/tests/server.smoke.test.ts @@ -249,4 +249,17 @@ describe('production server', () => { const response = await fetch(`${base}/talks`) expect(response.headers.get('ratelimit-limit')).toBeTruthy() }) + + // Express 5 / path-to-regexp v8 decodes route params itself and throws a + // URIError on a malformed percent-encoding, so this path is handled before + // it reaches any of our own middleware. It must come back as a client error + // and leave the process serving rather than taking it down. + it('survives a URL with a malformed percent-encoding', async () => { + const response = await fetch(`${base}/%`, { redirect: 'manual' }) + expect(response.status).toBeLessThan(500) + + // The point is the process surviving, so prove it still serves. + const after = await fetch(`${base}/talks`) + expect(after.status).toBe(200) + }) }) diff --git a/tests/timing.test.ts b/tests/timing.test.ts new file mode 100644 index 0000000..916d302 --- /dev/null +++ b/tests/timing.test.ts @@ -0,0 +1,80 @@ +import { describe, expect, it } from 'vitest' +import { + combineServerTimings, + getServerTimeHeader, + makeTimings, + time, +} from '#app/utils/timing.server' + +describe('getServerTimeHeader', () => { + it('returns an empty string with no timings', () => { + expect(getServerTimeHeader()).toBe('') + }) + + it('sanitises characters that are not allowed in a metric name', () => { + // The Server-Timing grammar does not allow `:`, `/`, `,` and friends in + // a metric name, so they are replaced with underscores. + const header = getServerTimeHeader({ + 'cache:get/user,1': [{ time: 1 }], + }) + expect(header).toBe('cache_get_user_1;dur=1.0') + }) +}) + +describe('makeTimings', () => { + it('serialises the open timer to a duration measured from creation', () => { + const timings = makeTimings('root loader', 'root loader') + const header = timings.toString() + + expect(header).toMatch(/^root_loader;desc="root loader";dur=\d+\.\d$/) + }) + + it('does not expose toString as an enumerable key', () => { + // The timings object is spread into nothing, but if it were ever logged + // or serialised an enumerable toString would show up as noise. + const timings = makeTimings('root loader') + expect(Object.keys(timings)).toEqual(['root loader']) + }) +}) + +describe('time', () => { + it('records a duration for the timed work', async () => { + const timings = makeTimings('root loader') + + const result = await time(async () => 'value', { + timings, + type: 'work', + desc: 'the work', + }) + + expect(result).toBe('value') + expect(getServerTimeHeader(timings)).toContain('work;desc="the work";dur=') + }) + + it('accepts an already-started promise', async () => { + const timings = makeTimings('root loader') + await expect( + time(Promise.resolve(1), { timings, type: 'work' }), + ).resolves.toBe(1) + expect(getServerTimeHeader(timings)).toContain('work;dur=') + }) + + it('resolves without timings and does not throw', async () => { + await expect(time(async () => 2, { type: 'work' })).resolves.toBe(2) + }) +}) + +describe('combineServerTimings', () => { + // `Headers.append` joins repeated values with ", " per the Fetch spec. + it('appends the second header to the first', () => { + const headers1 = new Headers({ 'Server-Timing': 'a;dur=1' }) + const headers2 = new Headers({ 'Server-Timing': 'b;dur=2' }) + + expect(combineServerTimings(headers1, headers2)).toBe('a;dur=1, b;dur=2') + }) + + it('appends an empty entry when the second header is absent', () => { + const headers1 = new Headers({ 'Server-Timing': 'a;dur=1' }) + expect(combineServerTimings(headers1, new Headers())).toBe('a;dur=1, ') + }) +}) From efed544d175f2ba1c6d22fcb8a8f74d9fd1c9432 Mon Sep 17 00:00:00 2001 From: Arpit Dalal Date: Sun, 27 Sep 2026 00:48:36 -0400 Subject: [PATCH 2/6] fix: match the ErrorResponse React Router actually throws (#7) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Sentry filter from the previous commit could never fire. React Router reports a request with no matching loader/action, or an unsupported method, by throwing an `ErrorResponse`, not an `Error` — verified against a running server: `handleError` receives an `ErrorResponseImpl` with `isError: false`, `message: undefined`, and the text on `.data` as a string in production and as an `Error` in development. So `error instanceof Error && isExpectedReactRouterErrorMessage(error.message)` was false for every scanner request it was meant to catch. Upstream Epic Stack has the same dead check. Replace it with `isExpectedReactRouterRouteError`, which unwraps the message from either shape of `.data` and additionally requires status 400 or 405 — the only statuses React Router pairs with those three messages. Checking the status also keeps the filter from swallowing an unrelated error whose text happens to contain one of the unanchored phrases, which is what CodeRabbit flagged on the first pass. Verified against the production server: `POST /robots.txt` returns 405, the error is logged, and the Sentry capture is skipped. Refs #7 --- app/entry.server.tsx | 15 ++--- app/utils/sentry-event-filters.ts | 37 ++++++++++++ tests/sentry-event-filters.test.ts | 96 +++++++++++++++++++++++------- 3 files changed, 119 insertions(+), 29 deletions(-) diff --git a/app/entry.server.tsx b/app/entry.server.tsx index 0e26816..fb0c1a0 100644 --- a/app/entry.server.tsx +++ b/app/entry.server.tsx @@ -12,7 +12,7 @@ import { } from 'react-router' import { getEnv, init } from './utils/env.server' import { NonceProvider } from './utils/nonce-provider' -import { isExpectedReactRouterErrorMessage } from './utils/sentry-event-filters' +import { isExpectedReactRouterRouteError } from './utils/sentry-event-filters' import { makeTimings } from './utils/timing.server' export const streamTimeout = 5000 @@ -96,14 +96,11 @@ export function handleError( } // Bots and scanners request every URL in the sitemap with methods the - // routes do not handle, and React Router throws for each of those. They - // are expected answers to invalid traffic, not bugs, so they get logged - // (they still show up in `fly logs`) but never reported to Sentry. - if ( - error instanceof Error && - isExpectedReactRouterErrorMessage(error.message) - ) { - console.error(styleText('red', String(error.stack))) + // routes do not handle, and React Router throws for each of those. They are + // expected answers to invalid traffic, not bugs, so they get logged (they + // still show up in `fly logs`) but never reported to Sentry. + if (isExpectedReactRouterRouteError(error)) { + console.error(error) return } diff --git a/app/utils/sentry-event-filters.ts b/app/utils/sentry-event-filters.ts index 9e5f077..92a9665 100644 --- a/app/utils/sentry-event-filters.ts +++ b/app/utils/sentry-event-filters.ts @@ -12,18 +12,55 @@ * `denyUrls`, so a filter for them would be dead code. */ +import { isRouteErrorResponse } from 'react-router' + export const EXPECTED_REACT_ROUTER_ERROR_PATTERNS = [ /did not provide an `action`/i, /did not provide a `loader`/i, /^Invalid request method /i, ] +/** + * The only statuses React Router uses for these three errors: 400 for a request + * with no matching `loader`, 405 for no matching `action` or an unsupported + * method. + */ +const EXPECTED_REACT_ROUTER_ERROR_STATUSES = [400, 405] + export function isExpectedReactRouterErrorMessage(message: string): boolean { return EXPECTED_REACT_ROUTER_ERROR_PATTERNS.some((pattern) => pattern.test(message), ) } +/** + * Narrows a value thrown by a loader/action to the React Router "no handler for + * this request" errors. + * + * These arrive as an `ErrorResponse`, not an `Error`, so `error instanceof + * Error` is false and `error.message` is undefined — in production the message + * lives on `.data` as a string. Matching on the message alone would also mean + * any unrelated error whose text happened to contain one of these phrases got + * dropped, so the status is checked first: the messages above are only ever + * produced alongside one of these two statuses. + */ +export function isExpectedReactRouterRouteError(error: unknown): boolean { + if (!isRouteErrorResponse(error)) return false + if (!EXPECTED_REACT_ROUTER_ERROR_STATUSES.includes(error.status)) return false + + // `data` is the message string in production and the underlying `Error` in + // development. + const { data } = error + const message = + typeof data === 'string' + ? data + : data instanceof Error + ? data.message + : null + + return message ? isExpectedReactRouterErrorMessage(message) : false +} + type ErrorEventLike = { message?: string exception?: { values?: Array<{ value?: string | null } | null> | null } | null diff --git a/tests/sentry-event-filters.test.ts b/tests/sentry-event-filters.test.ts index ceaad1c..0268af5 100644 --- a/tests/sentry-event-filters.test.ts +++ b/tests/sentry-event-filters.test.ts @@ -1,26 +1,25 @@ +import { UNSAFE_ErrorResponseImpl as ErrorResponseImpl } from 'react-router' import { describe, expect, it } from 'vitest' import { EXPECTED_REACT_ROUTER_ERROR_PATTERNS, getEventErrorMessages, isExpectedReactRouterErrorMessage, + isExpectedReactRouterRouteError, shouldDropErrorEvent, } from '#app/utils/sentry-event-filters' +const NO_ACTION = + 'You made a POST request to "/robots.txt" but did not provide an `action` for route "routes/_seo+/robots[.]txt", so there is no way to handle the request.' +const NO_LOADER = + 'You made a GET request to "/resources/theme-switch" but did not provide a `loader` for route "routes/resources/theme-switch", so there is no way to handle the request.' + describe('isExpectedReactRouterErrorMessage', () => { it('matches a request with no matching action', () => { - expect( - isExpectedReactRouterErrorMessage( - 'You made a POST request to "/" but did not provide an `action` for route "root", so there is no way to handle the request.', - ), - ).toBe(true) + expect(isExpectedReactRouterErrorMessage(NO_ACTION)).toBe(true) }) it('matches a request with no matching loader', () => { - expect( - isExpectedReactRouterErrorMessage( - 'You made a GET request to "/resources/theme-switch" but did not provide a `loader` for route "routes/resources/theme-switch", so there is no way to handle the request.', - ), - ).toBe(true) + expect(isExpectedReactRouterErrorMessage(NO_LOADER)).toBe(true) }) it('matches an unsupported request method', () => { @@ -89,16 +88,7 @@ describe('getEventErrorMessages', () => { describe('shouldDropErrorEvent', () => { it('drops an event whose exception value is an expected router error', () => { expect( - shouldDropErrorEvent({ - exception: { - values: [ - { - value: - 'You made a POST request to "/" but did not provide an `action` for route "root", so there is no way to handle the request.', - }, - ], - }, - }), + shouldDropErrorEvent({ exception: { values: [{ value: NO_ACTION }] } }), ).toBe(true) }) @@ -117,3 +107,69 @@ describe('shouldDropErrorEvent', () => { expect(shouldDropErrorEvent({})).toBe(false) }) }) + +describe('isExpectedReactRouterRouteError', () => { + // React Router reports these as an `ErrorResponse`, not an `Error`, so + // `error instanceof Error` is false and `error.message` is undefined. A + // filter written against `instanceof Error` silently never fires. + const routeError = (status: number, data: unknown) => + new ErrorResponseImpl(status, 'status', data, true) + + it('matches a 405 with no matching action', () => { + expect(isExpectedReactRouterRouteError(routeError(405, NO_ACTION))).toBe( + true, + ) + }) + + it('matches a 400 with no matching loader', () => { + expect(isExpectedReactRouterRouteError(routeError(400, NO_LOADER))).toBe( + true, + ) + }) + + it('matches an unsupported method', () => { + expect( + isExpectedReactRouterRouteError( + routeError(405, 'Invalid request method "OPTIONS"'), + ), + ).toBe(true) + }) + + it('matches when data is an Error, as it is in development', () => { + expect( + isExpectedReactRouterRouteError(routeError(405, new Error(NO_ACTION))), + ).toBe(true) + }) + + it('does not match a route error with another status', () => { + expect(isExpectedReactRouterRouteError(routeError(404, NO_ACTION))).toBe( + false, + ) + expect(isExpectedReactRouterRouteError(routeError(500, NO_ACTION))).toBe( + false, + ) + }) + + it('does not match a real error whose text contains the phrase', () => { + // The patterns are unanchored, so the status check is what keeps a + // genuine failure reportable. + expect( + isExpectedReactRouterRouteError( + new Error('newsletter route did not provide an `action` handler'), + ), + ).toBe(false) + expect(isExpectedReactRouterRouteError(routeError(500, NO_ACTION))).toBe( + false, + ) + }) + + it('does not match values that are not route errors', () => { + expect(isExpectedReactRouterRouteError(undefined)).toBe(false) + expect(isExpectedReactRouterRouteError(null)).toBe(false) + expect(isExpectedReactRouterRouteError(NO_ACTION)).toBe(false) + expect(isExpectedReactRouterRouteError({ status: 405 })).toBe(false) + expect(isExpectedReactRouterRouteError(routeError(405, undefined))).toBe( + false, + ) + }) +}) From 5bfa323053fa5cf8f534bbd9828b90bc9d5fbfa4 Mon Sep 17 00:00:00 2001 From: Arpit Dalal Date: Sun, 27 Sep 2026 01:20:05 -0400 Subject: [PATCH 3/6] fix: address review findings on the Tier 1 port (#7) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Task 1's regression test could not fail. It requested `/%`, but a malformed *path* is thrown on by Express's own `decodeParam` before morgan runs, so the 400 came from Express and the assertion held with the guard removed. The query string is not decoded by the router, so that is the vector that actually reaches the token. Measured against `main`: `GET /talks?x=%` kills the process (`child alive: false`); on this branch it serves 200 and survives. The test now uses that path and asserts the child is still up after the `finished` event has had a chance to fire, so it fails on the old behaviour. - `handleError` reported the `ErrorResponse` wrapper to Sentry, which files a stackless issue grouped on a synthesised message. Unwrap `.error` first, as React Router's own default handler does. This also means the filtered branch now logs a real stack rather than an object dump with the message buried in `.data`. - The morgan `url` token override dropped `req.originalUrl`, so mounting morgan at a path prefix would silently strip the prefix from every log line. - `ALLOW_INDEXING` was parsed by two incompatible grammars: lenient in `server/index.ts`, a strict enum in `envSchema`. `ALLOW_INDEXING=` (an empty Fly secret is the easy way to produce that) failed `init()`, and because the app bundle is imported per request inside `getBuild()` — whose catch swallows the failure — that became a permanent silent 500 on every route with no Sentry event, while the server's lenient parse still said "indexing allowed". Normalise to `'false' | undefined` so the two can never disagree. - Corrected two comments that stated something false: the `.data` shape is not a dev/prod split, and `routeDiscovery: 'initial'` does not remove a round-trip on navigation (the lazy default prefetches every `` on the page) — it trades an immutable cached manifest asset for inlining it into every document, which is the right call at 15 routes and the wrong one once the HTML is edge-cached. New tests, each verified to fail on the regression it guards: - `tests/server.smoke.test.ts` asserts the document carries both the `root_loader` and `render` Server-Timing metrics. The loader metric depends on `app/root.tsx`'s `headers` export; removing it silently costs half the instrumentation. - `tests/allow-indexing.test.ts` boots a server per state and pins both sides of the app/server boundary: header plus meta tag when `false`, neither when unset, no boot failure on an unrecognised value, and the tag on a 404 too. Dropping `ALLOW_INDEXING` from `getEnv()` keeps the header and loses the tag, which nothing else caught. Refs #7 --- app/entry.server.tsx | 29 +++-- app/utils/schemas.ts | 13 ++- app/utils/sentry-event-filters.ts | 19 +++- react-router.config.ts | 20 +++- server/index.ts | 7 +- tests/allow-indexing.test.ts | 165 +++++++++++++++++++++++++++++ tests/sentry-event-filters.test.ts | 24 +++++ tests/server.smoke.test.ts | 53 +++++++-- 8 files changed, 305 insertions(+), 25 deletions(-) create mode 100644 tests/allow-indexing.test.ts diff --git a/app/entry.server.tsx b/app/entry.server.tsx index fb0c1a0..854cdb2 100644 --- a/app/entry.server.tsx +++ b/app/entry.server.tsx @@ -12,7 +12,10 @@ import { } from 'react-router' import { getEnv, init } from './utils/env.server' import { NonceProvider } from './utils/nonce-provider' -import { isExpectedReactRouterRouteError } from './utils/sentry-event-filters' +import { + getRouteErrorCause, + isExpectedReactRouterRouteError, +} from './utils/sentry-event-filters' import { makeTimings } from './utils/timing.server' export const streamTimeout = 5000 @@ -95,19 +98,29 @@ export function handleError( return } + // React Router hands `handleError` an `ErrorResponse` rather than an + // `Error` for anything a route rejected, so the message and stack live on + // `.error`. Report that instead of the wrapper, otherwise Sentry files a + // stackless issue grouped on a synthesised message. This mirrors React + // Router's own default handler. + const reported = getRouteErrorCause(error) + const logReportedError = () => { + if (reported instanceof Error) { + console.error(styleText('red', String(reported.stack))) + } else { + console.error(reported) + } + } + // Bots and scanners request every URL in the sitemap with methods the // routes do not handle, and React Router throws for each of those. They are // expected answers to invalid traffic, not bugs, so they get logged (they // still show up in `fly logs`) but never reported to Sentry. if (isExpectedReactRouterRouteError(error)) { - console.error(error) + logReportedError() return } - if (error instanceof Error) { - console.error(styleText('red', String(error.stack))) - } else { - console.error(error) - } - void Sentry.captureException(error) + logReportedError() + void Sentry.captureException(reported) } diff --git a/app/utils/schemas.ts b/app/utils/schemas.ts index ec93743..4de6cc8 100644 --- a/app/utils/schemas.ts +++ b/app/utils/schemas.ts @@ -66,7 +66,18 @@ export const envSchema = z.object({ UMAMI_DOMAINS: z.string(), UMAMI_SCRIPT_NAME: z.string(), UMAMI_PUBLIC_ANALYTICS_URL: z.string().optional(), - ALLOW_INDEXING: z.enum(['true', 'false']).optional(), + // `server/index.ts` reads the same variable with a deliberately lenient + // grammar: only the exact string 'false' opts out of indexing, everything + // else (including unset and '') means indexing is allowed. An enum would + // reject '', '0' and 'FALSE' in `init()`, and because the app bundle is + // imported per request inside `getBuild()` — whose catch swallows the + // failure — a mistyped value would turn into a permanent silent 500 on + // every route instead of a boot error. Normalise instead of rejecting, so + // the two parsers can never disagree. + ALLOW_INDEXING: z + .string() + .optional() + .transform((value) => (value === 'false' ? 'false' : undefined)), }) // Type exports for TypeScript inference diff --git a/app/utils/sentry-event-filters.ts b/app/utils/sentry-event-filters.ts index 92a9665..a4fd44d 100644 --- a/app/utils/sentry-event-filters.ts +++ b/app/utils/sentry-event-filters.ts @@ -33,6 +33,19 @@ export function isExpectedReactRouterErrorMessage(message: string): boolean { ) } +/** + * The `Error` behind a route error, when React Router kept one. + * + * `ErrorResponseImpl.error` is marked private, but it is what React Router's + * own default `handleError` reads and it holds the real message and stack. + * Handing the wrapper to Sentry instead files a stackless issue grouped on a + * synthesised message. + */ +export function getRouteErrorCause(error: unknown): unknown { + if (!isRouteErrorResponse(error)) return error + return (error as { error?: unknown }).error ?? error +} + /** * Narrows a value thrown by a loader/action to the React Router "no handler for * this request" errors. @@ -48,8 +61,10 @@ export function isExpectedReactRouterRouteError(error: unknown): boolean { if (!isRouteErrorResponse(error)) return false if (!EXPECTED_REACT_ROUTER_ERROR_STATUSES.includes(error.status)) return false - // `data` is the message string in production and the underlying `Error` in - // development. + // `.data` carries the message two ways. React Router wraps it in an `Error` + // (see `getInternalRouterError` in react-router's router.js) and unwraps it + // again when it renders a document, so the document path yields a string; + // the single-fetch path yields the `Error`. Accept both. const { data } = error const message = typeof data === 'string' diff --git a/react-router.config.ts b/react-router.config.ts index bc8d658..4a7c512 100644 --- a/react-router.config.ts +++ b/react-router.config.ts @@ -1,12 +1,24 @@ import { type Config } from '@react-router/dev/config' export default { - // Defaults to true. Set to false to enable SPA for all routes. + // Defaults to true, but stated explicitly because it is load-bearing rather + // than decorative: @react-router/dev reads it to pick the Vite `appType` + // (its own plugin value wins over the one server/index.ts passes, and only + // resolves to 'spa' when ssr is false) and to force routeDiscovery to + // 'initial'. Setting this to false would flip both at once. ssr: true, - // This is a small site (12 routes). Opting out of lazy route discovery - // ships the whole route manifest with the initial HTML document, which - // removes a round-trip on every client-side navigation. + // The default is 'lazy' (fog of war), which ships only the matched routes + // and lets React Router discover the rest through a batched request that + // prefetches every on the page. 'initial' trades that away: the full + // route manifest is inlined into every HTML document instead of being a + // separate content-hashed, immutable asset cached once and reused. + // + // Worth it here because the manifest is 15 routes — a rounding error beside + // the document — so no navigation ever waits on a manifest request. Flip it + // back to 'lazy' if the route count grows or the HTML starts being + // edge-cached, since then the manifest is the thing that benefits from + // being cached on its own. routeDiscovery: { mode: 'initial' }, // `future.v8_splitRouteModules` moved to the top level in v8 and defaults to diff --git a/server/index.ts b/server/index.ts index 50aa94e..a0ad85b 100644 --- a/server/index.ts +++ b/server/index.ts @@ -99,7 +99,12 @@ app.get(['/img/{*splat}', '/favicons/{*splat}'], (_req, res) => { return }) -morgan.token('url', (req) => decodeRequestUrl(req.url)) +// Same fallback morgan's own `url` token uses, so mounting morgan at a path +// prefix cannot silently drop the prefix from every log line. morgan types the +// request as a bare IncomingMessage, but Express puts `originalUrl` on it. +morgan.token('url', (req) => + decodeRequestUrl((req as express.Request).originalUrl ?? req.url), +) app.use( morgan('tiny', { skip: (req, res) => diff --git a/tests/allow-indexing.test.ts b/tests/allow-indexing.test.ts new file mode 100644 index 0000000..0a089e6 --- /dev/null +++ b/tests/allow-indexing.test.ts @@ -0,0 +1,165 @@ +import { spawn, type ChildProcess } from 'node:child_process' +import { existsSync } from 'node:fs' +import getPort, { portNumbers } from 'get-port' +import { afterAll, beforeAll, describe, expect, it } from 'vitest' + +/** + * `ALLOW_INDEXING` is read in two places that sit on either side of the + * app/server boundary: `server/index.ts` sets the `X-Robots-Tag` response + * header, and `app/root.tsx` renders a `` tag from the same + * value via `getEnv()`. Nothing in the type system links the two, so removing + * the key from `getEnv()` would leave the header behind and silently drop the + * tag while lint, typecheck, format and every other test stayed green. + * + * The flag is read once at process start, so each state needs its own process. + */ + +// Two different build outputs. The server runs the esbuild bundle +// (server-build/), which is what imports the React Router server build +// (build/server/) on demand. Only the second one is worth checking for +// existence, because that is what is missing on a tree that never built. +const REACT_ROUTER_BUILD = 'build/server/index.js' +const SERVER_BUNDLE = 'server-build/index.js' +const BOOT_TIMEOUT_MS = 30_000 + +/** + * NOTE: `ALLOW_INDEXING` is deliberately absent. `spawn` inherits + * `process.env`, so a developer who exports it in their shell would otherwise + * silently change what these tests assert; the two runs below set it + * explicitly. + */ +const BASE_ENV = { + NODE_ENV: 'production', + SESSION_SECRET: 'test-session-secret', + HONEYPOT_SECRET: 'test-honeypot-secret', + INTERNAL_COMMAND_TOKEN: 'test-command-token', + SENTRY_DSN: '', + NODEMAILER_HOST: 'test-host', + NODEMAILER_USER: 'test-user', + NODEMAILER_PASSWORD: 'test-password', + HASHNODE_PUBLICATION_ID: 'test-publication-id', + HASHNODE_PUBLICATION_HOST: 'test-publication-host', + POSTHOG_API_KEY: 'test-posthog-key', + UMAMI_WEBSITE_ID: 'test-umami-id', + UMAMI_DOMAINS: 'test-umami-domains', + UMAMI_DOMAIN: 'test-umami-domain', + UMAMI_SCRIPT_NAME: 'test-umami-script', + UMAMI_PUBLIC_ANALYTICS_URL: '', +} as const + +const servers: ChildProcess[] = [] + +/** Boots a server with `ALLOW_INDEXING` forced to `value` (or unset). */ +async function bootServer(allowIndexing: string | undefined) { + if (!existsSync(REACT_ROUTER_BUILD)) { + throw new Error( + `${REACT_ROUTER_BUILD} is missing. Run \`npm run build\` before this test; the deploy workflow's test job does this.`, + ) + } + + const port = await getPort({ port: portNumbers(51_000, 59_000) }) + const env = { ...process.env, ...BASE_ENV, PORT: String(port) } + if (allowIndexing === undefined) { + delete env.ALLOW_INDEXING + } else { + // `envSchema` types this as 'true' | 'false', and the point of the third + // run is a value the type forbids, so widen it here on purpose. + env.ALLOW_INDEXING = allowIndexing as NodeJS.ProcessEnv['ALLOW_INDEXING'] + } + + const child = spawn(process.execPath, [SERVER_BUNDLE], { + env, + stdio: ['ignore', 'pipe', 'pipe'], + }) + servers.push(child) + + let output = '' + child.stdout?.on('data', (chunk: Buffer) => (output += chunk.toString())) + child.stderr?.on('data', (chunk: Buffer) => (output += chunk.toString())) + + // /robots.txt is a resource route whose loader only reads request headers, + // so it proves the handler is mounted without reaching Hashnode. + const deadline = Date.now() + BOOT_TIMEOUT_MS + while (Date.now() < deadline) { + if (child.exitCode !== null) { + throw new Error(`server exited during startup\n${output}`) + } + try { + const response = await fetch(`http://127.0.0.1:${port}/robots.txt`, { + redirect: 'manual', + }) + if (response.status < 500) return `http://127.0.0.1:${port}` + } catch { + // not listening yet + } + await new Promise((resolve) => setTimeout(resolve, 250)) + } + + throw new Error(`server did not become ready\n${output}`) +} + +afterAll(() => { + for (const server of servers) server.kill('SIGKILL') +}) + +describe('ALLOW_INDEXING unset', () => { + let base = '' + + beforeAll(async () => { + base = await bootServer(undefined) + }, 60_000) + + it('is indexable: no header and no meta tag', async () => { + const response = await fetch(`${base}/talks`) + expect(response.status).toBe(200) + expect(response.headers.get('x-robots-tag')).toBeNull() + expect(await response.text()).not.toContain('name="robots"') + }) +}) + +describe('ALLOW_INDEXING=false', () => { + let base = '' + + beforeAll(async () => { + base = await bootServer('false') + }, 60_000) + + it('sends X-Robots-Tag: noindex, nofollow', async () => { + const response = await fetch(`${base}/talks`) + expect(response.headers.get('x-robots-tag')).toBe('noindex, nofollow') + }) + + it('renders the matching meta tag, so both boundaries agree', async () => { + const response = await fetch(`${base}/talks`) + expect(await response.text()).toContain( + ' { + const response = await fetch(`${base}/nope-does-not-exist`, { + redirect: 'manual', + }) + expect(response.status).toBe(404) + expect(response.headers.get('x-robots-tag')).toBe('noindex, nofollow') + }) +}) + +describe('ALLOW_INDEXING with a value the server does not recognise', () => { + // `server/index.ts` treats anything other than the exact string 'false' as + // "indexing allowed", and `envSchema` has to agree or a typo would fail + // validation inside `init()`. `init()` runs from the app bundle, which + // `getBuild()` imports per request behind a catch, so a disagreement here + // does not fail the boot — it 500s every route with no Sentry event. + let base = '' + + beforeAll(async () => { + base = await bootServer('') + }, 60_000) + + it('boots and serves instead of failing env validation', async () => { + const response = await fetch(`${base}/talks`) + expect(response.status).toBe(200) + expect(response.headers.get('x-robots-tag')).toBeNull() + }) +}) diff --git a/tests/sentry-event-filters.test.ts b/tests/sentry-event-filters.test.ts index 0268af5..52290b1 100644 --- a/tests/sentry-event-filters.test.ts +++ b/tests/sentry-event-filters.test.ts @@ -3,6 +3,7 @@ import { describe, expect, it } from 'vitest' import { EXPECTED_REACT_ROUTER_ERROR_PATTERNS, getEventErrorMessages, + getRouteErrorCause, isExpectedReactRouterErrorMessage, isExpectedReactRouterRouteError, shouldDropErrorEvent, @@ -173,3 +174,26 @@ describe('isExpectedReactRouterRouteError', () => { ) }) }) + +describe('getRouteErrorCause', () => { + it('returns the wrapped Error behind a route error', () => { + // Without this, `handleError` hands Sentry an `ErrorResponse` wrapper, + // which Sentry files as a stackless issue. + const cause = new Error(NO_ACTION) + const routeError = new ErrorResponseImpl(405, 'status', cause, true) + + expect(getRouteErrorCause(routeError)).toBe(cause) + }) + + it('falls back to the route error when it carries no cause', () => { + const routeError = new ErrorResponseImpl(404, 'Not Found', 'nope', true) + expect(getRouteErrorCause(routeError)).toBe(routeError) + }) + + it('passes a plain value straight through', () => { + const error = new Error('boom') + expect(getRouteErrorCause(error)).toBe(error) + expect(getRouteErrorCause('boom')).toBe('boom') + expect(getRouteErrorCause(undefined)).toBeUndefined() + }) +}) diff --git a/tests/server.smoke.test.ts b/tests/server.smoke.test.ts index 9a2aa2d..ca90713 100644 --- a/tests/server.smoke.test.ts +++ b/tests/server.smoke.test.ts @@ -250,15 +250,50 @@ describe('production server', () => { expect(response.headers.get('ratelimit-limit')).toBeTruthy() }) - // Express 5 / path-to-regexp v8 decodes route params itself and throws a - // URIError on a malformed percent-encoding, so this path is handled before - // it reaches any of our own middleware. It must come back as a client error - // and leave the process serving rather than taking it down. - it('survives a URL with a malformed percent-encoding', async () => { - const response = await fetch(`${base}/%`, { redirect: 'manual' }) - expect(response.status).toBeLessThan(500) - - // The point is the process surviving, so prove it still serves. + it('reports Server-Timing for both the root loader and the render', async () => { + // The root loader's metric only reaches the client because + // `app/root.tsx` exports a `headers` function. React Router ignores a + // loader's response headers on the document without one, so dropping + // that export silently costs half the instrumentation and nothing else + // would notice. + const response = await fetch(`${base}/talks`) + const serverTiming = response.headers.get('server-timing') ?? '' + + expect(serverTiming).toContain('root_loader') + expect(serverTiming).toContain('render') + }) + + it('sends no X-Robots-Tag by default', async () => { + // ALLOW_INDEXING is unset for this server. The noindex behaviour is + // covered in tests/allow-indexing.test.ts, which boots its own process. + const response = await fetch(`${base}/talks`) + expect(response.headers.get('x-robots-tag')).toBeNull() + expect(await response.text()).not.toContain('name="robots"') + }) + + // The morgan `url` token decodes the whole request URL, query string + // included. morgan evaluates its tokens from the response's `finished` + // event with no try/catch, so a URIError there escapes as an uncaught + // exception and the process exits mid-request. Verified against `main`: + // this request kills the server, and the guard in + // `server/utils/request-url.ts` is what keeps it alive. + // + // The malformed value has to be in the query. A malformed *path* is thrown + // on by Express's own `decodeParam` before it reaches morgan, which returns + // a 400 and never evaluates the token, so that path cannot exercise this. + it('survives a query string with a malformed percent-encoding', async () => { + const response = await fetch(`${base}/talks?x=%`, { redirect: 'manual' }) + expect(response.status).toBe(200) + + // The uncaught throw lands on the `finished` event, which races the + // client's response resolving, so give the process a moment to die + // before asserting it is still up. + await new Promise((resolve) => setTimeout(resolve, 500)) + expect( + server?.exitCode, + `server should still be running${serverOutput ? `\n${serverOutput}` : ''}`, + ).toBeNull() + const after = await fetch(`${base}/talks`) expect(after.status).toBe(200) }) From 9d4bb4fde96aeae049ae7ea6a55db62d33e9a0f0 Mon Sep 17 00:00:00 2001 From: Arpit Dalal Date: Sun, 27 Sep 2026 10:18:42 -0400 Subject: [PATCH 4/6] fix: stop short-circuited requests 500ing, plus robustness nits (#7) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pre-existing, found by review, not caused by this branch — verified on `main`. `POST`, `PUT`, `DELETE` and `PATCH` to any page route returned 500. React Router answers a request with no matching loader/action, or an unsupported method, *before* running any loader. The root loader therefore never ran, and `useRequestInfo` asserted its data was present — inside the document render, via `Document` → `useHints()`. So the error boundary crashed while rendering, which turned a 405 into a 500 and buried the real cause. The crash goes through renderToPipeableStream's `onError`, not `handleError`, so Sentry never saw it either. Scanners send these constantly. `useRequestInfo` now returns `undefined` when the root loader never ran and the callers degrade: `useHints()` falls back to each hint's own default via `getHints()`, which already accepts a missing request, and `usePosthogPageView()` skips the page view, since there is no page to report. `POST /talks` now answers 405. The invariant it removed was never true. Robustness: - `time()` starts its timer before calling `fn` and ends it in a `finally`, so a rejecting or throwing `fn` no longer drops the metric — which is the case you most want a timing for. Calling `fn()` inside the `try` matters: hoisted above it, a synchronous throw escaped before the timer was ever ended. - `ErrorBoundary` passes `env`, so the 500 page emits the same robots meta tag the server's `X-Robots-Tag` header already promised. - `talks.tsx` guards `decodeURIComponent(location.hash)`, which threw a URIError in the browser for a malformed hash. Tests, each verified to fail on the regression it guards: the 405 status, and both `time()` rejection paths. Refs #7 --- app/root.tsx | 6 +++++- app/routes/_marketing+/talks.tsx | 9 ++++++++- app/utils/analytics.tsx | 8 +++++++- app/utils/client-hints.tsx | 5 ++++- app/utils/request-info.ts | 16 ++++++++++------ app/utils/timing.server.ts | 18 +++++++++++------- tests/server.smoke.test.ts | 15 +++++++++++++++ tests/timing.test.ts | 32 ++++++++++++++++++++++++++++++++ 8 files changed, 92 insertions(+), 17 deletions(-) diff --git a/app/root.tsx b/app/root.tsx index bf27e5d..21a58f2 100644 --- a/app/root.tsx +++ b/app/root.tsx @@ -8,6 +8,7 @@ import { Scripts, ScrollRestoration, useLoaderData, + useRouteLoaderData, } from 'react-router' import { ClientOnly } from 'remix-utils/client-only' import { HoneypotProvider } from 'remix-utils/honeypot/react' @@ -377,9 +378,12 @@ function Footer() { export function ErrorBoundary() { const nonce = useNonce() + // The server sends `X-Robots-Tag` on every response including this one, so + // pass the same env to keep the document's robots meta tag in agreement. + const data = useRouteLoaderData('root') return ( - + ) diff --git a/app/routes/_marketing+/talks.tsx b/app/routes/_marketing+/talks.tsx index 8b328b7..4938b31 100644 --- a/app/routes/_marketing+/talks.tsx +++ b/app/routes/_marketing+/talks.tsx @@ -52,7 +52,14 @@ export default function Talks() { React.useEffect(() => { const raw = location.hash.slice(1) if (!raw) return - const id = decodeURIComponent(raw) + // A malformed percent-encoding in the hash would throw a URIError here, + // in the browser, and take the effect down with it. + let id: string + try { + id = decodeURIComponent(raw) + } catch { + return + } const run = () => { const el = document.getElementById(id) if (el instanceof HTMLLIElement && el.tabIndex === -1) { diff --git a/app/utils/analytics.tsx b/app/utils/analytics.tsx index 54e593d..342f34d 100644 --- a/app/utils/analytics.tsx +++ b/app/utils/analytics.tsx @@ -4,11 +4,17 @@ import { useState, useEffect, useRef } from 'react' import { useRequestInfo } from './request-info' export function usePosthogPageView() { - const { origin, path } = useRequestInfo() + const requestInfo = useRequestInfo() + // No root loader means no request info (see `useRequestInfo`), which happens + // for the error responses React Router short-circuits. There is no page view + // to report in that case, and the hooks below still have to run. + const origin = requestInfo?.origin + const path = requestInfo?.path const [previousLocation, setPreviousLocation] = useState(path) const isInitialRender = useRef(true) useEffect(() => { + if (path === undefined) return try { if (isInitialRender.current) { isInitialRender.current = false diff --git a/app/utils/client-hints.tsx b/app/utils/client-hints.tsx index 7455015..46ce92d 100644 --- a/app/utils/client-hints.tsx +++ b/app/utils/client-hints.tsx @@ -99,7 +99,10 @@ export function formatDateWithHints(isoDate: string, request: Request) { */ export function useHints() { const requestInfo = useRequestInfo() - return requestInfo.hints + // With no root loader there are no request headers to read the cookies from + // (see `useRequestInfo`), so fall back to each hint's own default rather + // than throwing in the middle of the document render. + return requestInfo?.hints ?? getHints() } /** diff --git a/app/utils/request-info.ts b/app/utils/request-info.ts index a6dd659..30729f6 100644 --- a/app/utils/request-info.ts +++ b/app/utils/request-info.ts @@ -1,13 +1,17 @@ -import { invariant } from '@epic-web/invariant' import { useRouteLoaderData } from 'react-router' import { type loader as rootLoader } from '#app/root' /** - * @returns the request info from the root loader + * @returns the request info from the root loader, or `undefined` when the root + * loader never ran. + * + * That second case is not hypothetical. React Router short-circuits a request + * with no matching `loader`/`action`, or with an unsupported method, *before* + * any loader runs — so for a 400/405 there is no root data, yet the document + * (and therefore the error boundary) still has to render. Asserting the data is + * present here turned every such request into a 500 with the real cause + * buried, so callers degrade instead. */ export function useRequestInfo() { - const data = useRouteLoaderData('root') - invariant(data?.requestInfo, 'No requestInfo found in root loader') - - return data.requestInfo + return useRouteLoaderData('root')?.requestInfo } diff --git a/app/utils/timing.server.ts b/app/utils/timing.server.ts index 09ec510..fd26a23 100644 --- a/app/utils/timing.server.ts +++ b/app/utils/timing.server.ts @@ -46,14 +46,18 @@ export async function time( timings?: Timings }, ): Promise { - const timer = createTimer(type, desc) - const promise = typeof fn === 'function' ? fn() : fn - if (!timings) return promise - - const result = await promise + if (!timings) return typeof fn === 'function' ? fn() : fn - timer.end(timings) - return result + // Start the timer before invoking `fn` and end it in a `finally`. A + // rejecting or throwing `fn` is exactly when a timing is most worth having, + // and it must not be the one case that drops the metric — so `fn()` has to + // be called inside the `try`, not hoisted above it. + const timer = createTimer(type, desc) + try { + return await (typeof fn === 'function' ? fn() : fn) + } finally { + timer.end(timings) + } } export function getServerTimeHeader(timings?: Timings) { diff --git a/tests/server.smoke.test.ts b/tests/server.smoke.test.ts index ca90713..0f8600a 100644 --- a/tests/server.smoke.test.ts +++ b/tests/server.smoke.test.ts @@ -154,6 +154,21 @@ describe('production server', () => { expect(response.status).toBe(200) }) + // React Router short-circuits these before any loader runs, so the root + // loader never produced data. The document still has to render the error + // boundary, and `useRequestInfo` used to assert the data was there — which + // turned a 405 into a 500 and buried the real cause. Scanners send these + // constantly. + it('answers an unsupported method with 405, not 500', async () => { + for (const method of ['POST', 'PUT', 'DELETE', 'PATCH']) { + const response = await fetch(`${base}/talks`, { + method, + redirect: 'manual', + }) + expect(response.status, `${method} /talks`).toBe(405) + } + }) + it('404s an unknown path rather than serving the app shell', async () => { const response = await fetch(`${base}/nope-does-not-exist`) expect(response.status).toBe(404) diff --git a/tests/timing.test.ts b/tests/timing.test.ts index 916d302..6e9adab 100644 --- a/tests/timing.test.ts +++ b/tests/timing.test.ts @@ -62,6 +62,38 @@ describe('time', () => { it('resolves without timings and does not throw', async () => { await expect(time(async () => 2, { type: 'work' })).resolves.toBe(2) }) + + it('still records a duration when the timed work rejects', async () => { + // A failing loader is exactly when a timing is most worth having, so it + // must not be the one case that drops the metric. + const timings = makeTimings('root loader') + + await expect( + time( + async () => { + throw new Error('boom') + }, + { timings, type: 'work', desc: 'the work' }, + ), + ).rejects.toThrow('boom') + + expect(getServerTimeHeader(timings)).toContain('work;desc="the work";dur=') + }) + + it('records a duration when the work throws synchronously', async () => { + const timings = makeTimings('root loader') + + await expect( + time( + () => { + throw new Error('boom') + }, + { timings, type: 'work' }, + ), + ).rejects.toThrow('boom') + + expect(getServerTimeHeader(timings)).toContain('work;dur=') + }) }) describe('combineServerTimings', () => { From b3644d8c417936824a2667f8cd99edd617950c42 Mon Sep 17 00:00:00 2001 From: Arpit Dalal Date: Sun, 27 Sep 2026 10:27:00 -0400 Subject: [PATCH 5/6] fix: strip the serialised error name before matching (#7) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex review finding, confirmed against a running server. React Router raises an unsupported method as `new ErrorResponseImpl(405, 'Method Not Allowed', new Error('Invalid request method "OPTIONS"'))`, and on the document path `.data` ends up holding the *stringified* error — `'Error: Invalid request method "OPTIONS"'`. The anchored `/^Invalid request method /i` pattern does not match that, so every `OPTIONS` or `PROPFIND` from a scanner still went to Sentry while the unanchored `action`/`loader` patterns kept working. The 405 smoke test missed it because it only sent methods that produce the unanchored message. Strip a leading `Error: ` before testing, which keeps the patterns anchored to the message itself and covers the whole class rather than this one pattern. The other two were unaffected only by luck of being unanchored. Tests use the payloads captured from a running server, and `OPTIONS` joins the 405 smoke test. Reverting the strip fails exactly those two cases. Refs #7 --- app/utils/sentry-event-filters.ts | 19 +++++++++++++++--- tests/sentry-event-filters.test.ts | 31 +++++++++++++++++++++++++++++- tests/server.smoke.test.ts | 2 +- 3 files changed, 47 insertions(+), 5 deletions(-) diff --git a/app/utils/sentry-event-filters.ts b/app/utils/sentry-event-filters.ts index a4fd44d..8902844 100644 --- a/app/utils/sentry-event-filters.ts +++ b/app/utils/sentry-event-filters.ts @@ -27,6 +27,17 @@ export const EXPECTED_REACT_ROUTER_ERROR_PATTERNS = [ */ const EXPECTED_REACT_ROUTER_ERROR_STATUSES = [400, 405] +/** + * React Router's message arrives either bare (`Error.message`) or stringified + * (`String(error)`, which is what `.data` holds on the document path). The + * latter is prefixed with the error's name, which breaks the anchored + * `^Invalid request method` pattern. Strip it before matching so the patterns + * can stay anchored to the message itself. + */ +function stripErrorNamePrefix(message: string): string { + return message.replace(/^\w*Error:\s*/, '') +} + export function isExpectedReactRouterErrorMessage(message: string): boolean { return EXPECTED_REACT_ROUTER_ERROR_PATTERNS.some((pattern) => pattern.test(message), @@ -63,8 +74,8 @@ export function isExpectedReactRouterRouteError(error: unknown): boolean { // `.data` carries the message two ways. React Router wraps it in an `Error` // (see `getInternalRouterError` in react-router's router.js) and unwraps it - // again when it renders a document, so the document path yields a string; - // the single-fetch path yields the `Error`. Accept both. + // again when it renders a document, so the document path yields a + // stringified error; the single-fetch path yields the `Error`. Accept both. const { data } = error const message = typeof data === 'string' @@ -73,7 +84,9 @@ export function isExpectedReactRouterRouteError(error: unknown): boolean { ? data.message : null - return message ? isExpectedReactRouterErrorMessage(message) : false + return message + ? isExpectedReactRouterErrorMessage(stripErrorNamePrefix(message)) + : false } type ErrorEventLike = { diff --git a/tests/sentry-event-filters.test.ts b/tests/sentry-event-filters.test.ts index 52290b1..5494389 100644 --- a/tests/sentry-event-filters.test.ts +++ b/tests/sentry-event-filters.test.ts @@ -11,6 +11,15 @@ import { const NO_ACTION = 'You made a POST request to "/robots.txt" but did not provide an `action` for route "routes/_seo+/robots[.]txt", so there is no way to handle the request.' +/** + * Captured from a running production server. React Router stringifies the + * internal `Error` into `.data` on the document path, so the message arrives + * with an `Error: ` prefix — which is what silently broke the anchored + * `^Invalid request method` pattern. + */ +const NO_ACTION_STRINGIFIED = `Error: ${NO_ACTION}` +const INVALID_METHOD = 'Invalid request method "OPTIONS"' +const INVALID_METHOD_STRINGIFIED = `Error: ${INVALID_METHOD}` const NO_LOADER = 'You made a GET request to "/resources/theme-switch" but did not provide a `loader` for route "routes/resources/theme-switch", so there is no way to handle the request.' @@ -129,9 +138,29 @@ describe('isExpectedReactRouterRouteError', () => { }) it('matches an unsupported method', () => { + expect( + isExpectedReactRouterRouteError(routeError(405, INVALID_METHOD)), + ).toBe(true) + }) + + it('matches a stringified message, prefix and all', () => { + // This is the shape the document path actually produces. The anchored + // pattern does not match the `Error: ` prefix, so without stripping it + // these scanner requests keep paging. + expect( + isExpectedReactRouterRouteError( + routeError(405, INVALID_METHOD_STRINGIFIED), + ), + ).toBe(true) + expect( + isExpectedReactRouterRouteError(routeError(405, NO_ACTION_STRINGIFIED)), + ).toBe(true) + }) + + it('strips a custom error name prefix too', () => { expect( isExpectedReactRouterRouteError( - routeError(405, 'Invalid request method "OPTIONS"'), + routeError(405, `TypeError: ${INVALID_METHOD}`), ), ).toBe(true) }) diff --git a/tests/server.smoke.test.ts b/tests/server.smoke.test.ts index 0f8600a..220e505 100644 --- a/tests/server.smoke.test.ts +++ b/tests/server.smoke.test.ts @@ -160,7 +160,7 @@ describe('production server', () => { // turned a 405 into a 500 and buried the real cause. Scanners send these // constantly. it('answers an unsupported method with 405, not 500', async () => { - for (const method of ['POST', 'PUT', 'DELETE', 'PATCH']) { + for (const method of ['POST', 'PUT', 'DELETE', 'PATCH', 'OPTIONS']) { const response = await fetch(`${base}/talks`, { method, redirect: 'manual', From 9cd838a968dbbb8e115012432fe49a809589a7d6 Mon Sep 17 00:00:00 2001 From: Arpit Dalal Date: Sun, 27 Sep 2026 10:42:57 -0400 Subject: [PATCH 6/6] fix: require an internal error response before suppressing it (#7) Two CodeRabbit review findings, both valid. `isExpectedReactRouterRouteError` accepted any 400/405 route error whose text contained one of the three phrases, so a route throwing its own `new Response(body, { status: 405 })` would be dropped from Sentry. React Router marks the errors it raises itself `internal: true` and a route's own error response `internal: false`; confirmed against a running server, where both the no-action and invalid-method 405s arrive with `internal: true`. The filter now requires that flag, which narrows suppression to errors React Router generated rather than to errors that merely look like one. The smoke-test child inherited `process.env`, and `TEST_ENV` does not mention `ALLOW_INDEXING`, so a developer who exported it would get the new "sends no X-Robots-Tag by default" test failing on their machine only. The env is now built explicitly and the key deleted. Both verified by mutation: dropping the `internal` check fails the new route-thrown cases, and dropping the `delete` fails the header test when run with `ALLOW_INDEXING=false` exported. Refs #7 --- app/utils/sentry-event-filters.ts | 10 ++++++++-- tests/sentry-event-filters.test.ts | 21 +++++++++++++++++++-- tests/server.smoke.test.ts | 8 +++++++- 3 files changed, 34 insertions(+), 5 deletions(-) diff --git a/app/utils/sentry-event-filters.ts b/app/utils/sentry-event-filters.ts index 8902844..3207f54 100644 --- a/app/utils/sentry-event-filters.ts +++ b/app/utils/sentry-event-filters.ts @@ -65,11 +65,17 @@ export function getRouteErrorCause(error: unknown): unknown { * Error` is false and `error.message` is undefined — in production the message * lives on `.data` as a string. Matching on the message alone would also mean * any unrelated error whose text happened to contain one of these phrases got - * dropped, so the status is checked first: the messages above are only ever - * produced alongside one of these two statuses. + * dropped, so the status and the `internal` flag are checked first: the + * messages above are only ever produced alongside one of these two statuses, on + * an error React Router raised itself. */ export function isExpectedReactRouterRouteError(error: unknown): boolean { if (!isRouteErrorResponse(error)) return false + // `internal: true` is how React Router marks the errors it generated itself + // (see `getInternalRouterError`). A route that throws its own 400/405 is + // marked `internal: false`, and that is a real bug worth reporting even if + // its text happens to contain one of the phrases below. + if ((error as { internal?: unknown }).internal !== true) return false if (!EXPECTED_REACT_ROUTER_ERROR_STATUSES.includes(error.status)) return false // `.data` carries the message two ways. React Router wraps it in an `Error` diff --git a/tests/sentry-event-filters.test.ts b/tests/sentry-event-filters.test.ts index 5494389..4c27eca 100644 --- a/tests/sentry-event-filters.test.ts +++ b/tests/sentry-event-filters.test.ts @@ -122,8 +122,8 @@ describe('isExpectedReactRouterRouteError', () => { // React Router reports these as an `ErrorResponse`, not an `Error`, so // `error instanceof Error` is false and `error.message` is undefined. A // filter written against `instanceof Error` silently never fires. - const routeError = (status: number, data: unknown) => - new ErrorResponseImpl(status, 'status', data, true) + const routeError = (status: number, data: unknown, internal = true) => + new ErrorResponseImpl(status, 'status', data, internal) it('matches a 405 with no matching action', () => { expect(isExpectedReactRouterRouteError(routeError(405, NO_ACTION))).toBe( @@ -171,6 +171,23 @@ describe('isExpectedReactRouterRouteError', () => { ).toBe(true) }) + it('does not match a route-thrown error response', () => { + // React Router marks the errors it raises itself `internal: true` and a + // route's own 400/405 `internal: false`. A route throwing its own error + // is a real bug even when the text contains one of these phrases. + expect( + isExpectedReactRouterRouteError(routeError(405, NO_ACTION, false)), + ).toBe(false) + expect( + isExpectedReactRouterRouteError( + routeError(405, INVALID_METHOD_STRINGIFIED, false), + ), + ).toBe(false) + expect( + isExpectedReactRouterRouteError(routeError(400, NO_LOADER, false)), + ).toBe(false) + }) + it('does not match a route error with another status', () => { expect(isExpectedReactRouterRouteError(routeError(404, NO_ACTION))).toBe( false, diff --git a/tests/server.smoke.test.ts b/tests/server.smoke.test.ts index 220e505..a2dfa8f 100644 --- a/tests/server.smoke.test.ts +++ b/tests/server.smoke.test.ts @@ -106,8 +106,14 @@ beforeAll(async () => { port = await getPort({ port: portNumbers(41_000, 49_000) }) base = `http://127.0.0.1:${port}` + const env = { ...process.env, ...TEST_ENV, PORT: String(port) } + // `spawn` inherits `process.env`, and TEST_ENV does not mention + // ALLOW_INDEXING. A developer who exports it would otherwise make the + // "sends no X-Robots-Tag by default" test below fail on their machine only. + delete env.ALLOW_INDEXING + const child = spawn(process.execPath, ['server-build/index.js'], { - env: { ...process.env, ...TEST_ENV, PORT: String(port) }, + env, stdio: ['ignore', 'pipe', 'pipe'], }) server = child