From 7487afde9183d1823ff7fd4dc5071a7568343f1d Mon Sep 17 00:00:00 2001 From: Kyle June Date: Thu, 24 Sep 2026 02:30:08 -0400 Subject: [PATCH] fix: answer router-rejected data requests A data request that React Router's queryRoute rejects before any loader or action runs (an X-Juniper-Route-Id that doesn't match the URL, a POST to a route with no action, a GET to a route with no loader) threw an ErrorResponseImpl. Hono only passes Error instances to onError, so the request escaped Juniper's error handling. It now becomes an HttpError and goes out as a data-error envelope: 404, 405 with Allow, or 400. Co-Authored-By: Claude Opus 5.5 --- docs/error-handling.md | 15 +++ src/_server.tsx | 56 +++++++- src/server.test.tsx | 282 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 352 insertions(+), 1 deletion(-) diff --git a/docs/error-handling.md b/docs/error-handling.md index 1c67204..530ad72 100644 --- a/docs/error-handling.md +++ b/docs/error-handling.md @@ -117,6 +117,21 @@ A loader or action can also throw a `Response` other than a redirect, or throw `HttpError` with that status and those headers. A status outside 400–599 becomes `500`. +A data request that no loader or action can handle also gets an `HttpError`: + +- `404` when its `X-Juniper-Route-Id` doesn't name a route that matches the URL. +- `405` when the route has no handler for the method, such as a `POST` to a + route without an action. The `Allow` header lists the methods the route + accepts. It's left off while a lazily loaded route's module hasn't loaded on + the server yet. +- `400` for a `GET` to a route without a loader. Until a lazily loaded route's + module has loaded on the server, React Router answers that request with + `undefined` data instead. + +These errors carry the generic message for their status. React Router's own +message, which names the route, is logged on the server. Outside development, it +isn't sent to the browser. + ## Error Boundaries Error boundaries catch errors thrown during rendering, in loaders, actions, or diff --git a/src/_server.tsx b/src/_server.tsx index 9f0a8af..d7985ec 100644 --- a/src/_server.tsx +++ b/src/_server.tsx @@ -14,12 +14,15 @@ import type { ActionFunctionArgs, LoaderFunctionArgs } from "react-router"; import { createStaticHandler, createStaticRouter, + isRouteErrorResponse, + matchRoutes, StaticRouterProvider, } from "react-router"; import type { DataRouteObject, DataStrategyFunctionArgs, DataStrategyResult, + ErrorResponse, RouterContextProvider, StaticHandlerContext, } from "react-router"; @@ -350,6 +353,22 @@ async function responseToHttpError(response: Response): Promise { }); } +function isRouterRejection(error: ErrorResponse): boolean { + return (error as { internal?: unknown }).internal === true; +} + +function routeErrorResponseToHttpError(error: ErrorResponse): HttpError { + const fromRouter = isRouterRejection(error); + return new HttpError( + fromRouter && error.status === 403 ? 404 : errorStatus(error.status), + { + message: typeof error.data === "string" ? error.data : error.statusText, + expose: fromRouter ? false : undefined, + cause: error, + }, + ); +} + async function convertToHttpError(cause: unknown): Promise { if ( cause !== null && @@ -368,6 +387,7 @@ async function convertToHttpError(cause: unknown): Promise { headers: new Headers(init?.headers), }); } + if (isRouteErrorResponse(cause)) return routeErrorResponseToHttpError(cause); return HttpError.from(cause); } @@ -1047,6 +1067,33 @@ async function dataRequestStrategy( return results; } +function allowedDataMethods(route: DataRouteObject): string { + return [ + ...(route.loader ? ["GET", "HEAD"] : []), + ...(route.action ? ["POST", "PUT", "PATCH", "DELETE"] : []), + ].join(", "); +} + +async function toDataRequestError( + cause: unknown, + request: Request, + dataRoutes: DataRouteObject[], + routeId: string, +): Promise { + const error = await convertToHttpError(cause); + if ( + error.status === 405 && isRouteErrorResponse(cause) && + isRouterRejection(cause) + ) { + const route = matchRoutes(dataRoutes, new URL(request.url).pathname) + ?.find((match) => match.route.id === routeId)?.route; + if (route && !route.lazy) { + error.headers.set("Allow", allowedDataMethods(route)); + } + } + return error; +} + /** * Builds the Hono handlers for the client routes — server-rendered documents * and data requests — plus the error handler `createServer` installs alongside @@ -1107,13 +1154,20 @@ export function createHandlers< }, async function handleDataRequest(c) { return await startActiveSpan("handleDataRequest", async (_span) => { - const routeId = c.req.header("X-Juniper-Route-Id"); + const routeId = c.req.header("X-Juniper-Route-Id") ?? ""; const requestContext = c.get("context"); const dataOrResponse = await queryRoute(c.req.raw, { requestContext, routeId, dataStrategy: dataRequestStrategy, + }).catch(async (cause: unknown) => { + throw await toDataRequestError( + cause, + c.req.raw, + dataRoutes, + routeId, + ); }); if (dataOrResponse instanceof Response) { diff --git a/src/server.test.tsx b/src/server.test.tsx index 30e4474..965fd1e 100644 --- a/src/server.test.tsx +++ b/src/server.test.tsx @@ -14,6 +14,7 @@ import { Outlet, redirect, redirectDocument, + UNSAFE_ErrorResponseImpl as ErrorResponseImpl, useLoaderData, useParams, } from "react-router"; @@ -1733,6 +1734,287 @@ describe("the headers a loader or action response sets itself", () => { } }); +describe("data requests React Router rejects before a loader or action runs", () => { + const appMiddleware: MiddlewareHandler = async (c, next) => { + c.header("X-Application", "app"); + await next(); + }; + + function serverWithRoutes(readsResponseFirst: boolean) { + const client = new Client({ + path: "/", + main: { default: () => }, + children: [ + { path: "page", main: { default: () =>
Page
} }, + { path: "reader", main: { default: () =>
Reader
} }, + { + path: "lazy", + main: () => Promise.resolve({ default: () =>
Lazy
}), + }, + { path: "writer", main: { default: () =>
Writer
} }, + { path: "thrower", main: { default: () =>
Thrower
} }, + { path: "members", main: { default: () =>
Members
} }, + { path: "closed", main: { default: () =>
Closed
} }, + ], + }); + const middleware = readsResponseFirst + ? [cors(), appMiddleware] + : [appMiddleware]; + return createServer(import.meta.url, client, { + path: "/", + main: { default: new Hono().use(...middleware) }, + children: [ + { path: "reader", main: { loader: () => ({ read: true }) } }, + { path: "writer", main: { action: () => ({ written: true }) } }, + { + path: "thrower", + main: { + loader: () => { + throw "a thrown value that must stay on the server"; + }, + }, + }, + { + path: "members", + main: { + loader: () => { + throw new ErrorResponseImpl(403, "Forbidden", "Members only"); + }, + }, + }, + { + path: "closed", + main: { + loader: () => { + throw new ErrorResponseImpl(405, "Method Not Allowed", "Closed"); + }, + action: () => { + throw new Response("Closed", { + status: 405, + headers: { Allow: "GET" }, + }); + }, + }, + }, + ], + }); + } + + async function assertDataError( + response: Response, + status: number, + hiddenDetail: string, + ): Promise { + const body = await response.text(); + assertEquals(response.status, status); + assertEquals(response.headers.get("X-Juniper"), "data"); + assertEquals(response.headers.get("Content-Type"), "application/json"); + assertEquals(response.headers.get("Cache-Control"), "private, no-cache"); + assertEquals(response.headers.get("X-Application"), "app"); + const error = deserializeError(deserializeLoaderData(body)); + assertInstanceOf(error, HttpError); + assertEquals(error.status, status); + assertEquals( + error.message, + new HttpError(status, { expose: false }).exposedMessage, + ); + assertFalse( + body.includes(hiddenDetail), + `the body leaks "${hiddenDetail}"`, + ); + } + + for ( + const [order, readsResponseFirst] of [ + ["after middleware has read the response", true], + ["when no middleware has read the response", false], + ] as const + ) { + describe(order, () => { + for (const method of ["GET", "POST"]) { + for (const routeId of ["/unknown", "/reader"]) { + it(`answers a ${method} data request whose route id ${routeId} is not on the URL with a 404 data error`, async () => { + using _log = stub(console, "error"); + const server = serverWithRoutes(readsResponseFirst); + const response = await server.request("http://localhost/page", { + method, + headers: { "X-Juniper-Route-Id": routeId }, + }); + await assertDataError(response, 404, "does not match URL"); + assertEquals(response.headers.get("Allow"), null); + }); + } + } + + for ( + const [path, allow] of [ + ["/page", ""], + ["/reader", "GET, HEAD"], + ["/lazy", ""], + ] as const + ) { + it(`answers a POST data request to ${path}, which has no action, with a 405 data error that lists the methods it allows`, async () => { + using _log = stub(console, "error"); + const server = serverWithRoutes(readsResponseFirst); + const response = await server.request(`http://localhost${path}`, { + method: "POST", + headers: { "X-Juniper-Route-Id": path }, + }); + await assertDataError(response, 405, "did not provide an `action`"); + assertEquals(response.headers.get("Allow"), allow); + }); + } + + for ( + const [path, allow] of [ + ["/reader", "GET, HEAD"], + ["/writer", "POST, PUT, PATCH, DELETE"], + ] as const + ) { + it(`answers a data request to ${path} with a method React Router does not accept with a 405 data error that lists the methods the route allows`, async () => { + using _log = stub(console, "error"); + const server = serverWithRoutes(readsResponseFirst); + const response = await server.request(`http://localhost${path}`, { + method: "PROPFIND", + headers: { "X-Juniper-Route-Id": path }, + }); + await assertDataError(response, 405, "Invalid request method"); + assertEquals(response.headers.get("Allow"), allow); + }); + } + + for ( + const [path, routeId, reason] of [ + ["/page", "/reader", "whose route id is not on the URL"], + ["/lazy", "/lazy", "whose lazy route has not loaded"], + ] as const + ) { + it(`leaves Allow off a 405 data error for a method React Router does not accept, ${reason}`, async () => { + using _log = stub(console, "error"); + const server = serverWithRoutes(readsResponseFirst); + const response = await server.request(`http://localhost${path}`, { + method: "PROPFIND", + headers: { "X-Juniper-Route-Id": routeId }, + }); + await assertDataError(response, 405, "Invalid request method"); + assertEquals(response.headers.get("Allow"), null); + }); + } + + it("keeps the Allow header of a 405 Response an action throws on a data request", async () => { + using _log = stub(console, "error"); + const server = serverWithRoutes(readsResponseFirst); + const response = await server.request("http://localhost/closed", { + method: "POST", + headers: { "X-Juniper-Route-Id": "/closed" }, + }); + await response.arrayBuffer(); + assertEquals(response.status, 405); + assertEquals(response.headers.get("Allow"), "GET"); + }); + + it("adds no Allow header to a 405 ErrorResponse a loader throws on a data request", async () => { + using _log = stub(console, "error"); + const server = serverWithRoutes(readsResponseFirst); + const response = await server.request("http://localhost/closed", { + headers: { "X-Juniper-Route-Id": "/closed" }, + }); + const error = deserializeError( + deserializeLoaderData(await response.text()), + ); + assertInstanceOf(error, HttpError); + assertEquals(error.message, "Closed"); + assertEquals(response.status, 405); + assertEquals(response.headers.get("Allow"), null); + }); + + for (const path of ["/", "/page"]) { + it(`answers a GET data request to ${path}, which has no loader, with a 400 data error`, async () => { + using _log = stub(console, "error"); + const server = serverWithRoutes(readsResponseFirst); + const response = await server.request(`http://localhost${path}`, { + headers: { "X-Juniper-Route-Id": path }, + }); + await assertDataError(response, 400, "did not provide a `loader`"); + assertEquals(response.headers.get("Allow"), null); + }); + } + + it("sends a value that is not an Error, thrown by a loader, to a data request as a 500 data error", async () => { + using _log = stub(console, "error"); + const server = serverWithRoutes(readsResponseFirst); + const response = await server.request("http://localhost/thrower", { + headers: { "X-Juniper-Route-Id": "/thrower" }, + }); + await assertDataError( + response, + 500, + "a thrown value that must stay on the server", + ); + }); + + it("keeps the status and message of an ErrorResponse a loader throws on a data request", async () => { + using _log = stub(console, "error"); + const server = serverWithRoutes(readsResponseFirst); + const response = await server.request("http://localhost/members", { + headers: { "X-Juniper-Route-Id": "/members" }, + }); + assertEquals(response.status, 403); + assertEquals(response.headers.get("X-Juniper"), "data"); + const error = deserializeError( + deserializeLoaderData(await response.text()), + ); + assertInstanceOf(error, HttpError); + assertEquals(error.status, 403); + assertEquals(error.message, "Members only"); + }); + + it("still sends a loader's data to a data request for its route", async () => { + const server = serverWithRoutes(readsResponseFirst); + const response = await server.request("http://localhost/reader", { + headers: { "X-Juniper-Route-Id": "/reader" }, + }); + assertEquals(response.status, 200); + assertEquals(response.headers.get("X-Juniper"), "data"); + assertEquals( + response.headers.get("Cache-Control"), + "private, no-cache", + ); + assertEquals(response.headers.get("X-Application"), "app"); + assertEquals(deserializeLoaderData(await response.text()), { + read: true, + }); + }); + + it("still renders a document for a route without a loader", async () => { + const server = serverWithRoutes(readsResponseFirst); + const response = await server.request("http://localhost/page"); + assertEquals(response.status, 200); + assertEquals( + response.headers.get("Content-Type"), + "text/html; charset=utf-8", + ); + assertStringIncludes(await response.text(), "
Page
"); + }); + + it("still renders a 405 error document for a POST to a route without an action", async () => { + using _log = stub(console, "error"); + const server = serverWithRoutes(readsResponseFirst); + const response = await server.request("http://localhost/page", { + method: "POST", + }); + assertEquals(response.status, 405); + assertEquals( + response.headers.get("Content-Type"), + "text/html; charset=utf-8", + ); + assertEquals(response.headers.get("X-Juniper"), null); + assertStringIncludes(await response.text(), " { const revalidate = "private, no-cache, must-revalidate, max-age=0"; const longLived = "public, max-age=14400";