From b89efb4504d5520228863af3e22196e9fa246796 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Thu, 6 Jul 2023 16:16:34 -0400 Subject: [PATCH 1/2] Properly handle network erorrs on _data requests --- .changeset/handle-network-errors.md | 6 ++++ integration/error-boundary-v2-test.ts | 37 +++++++++++++++++-------- packages/remix-react/data.ts | 36 ++++++++++++++++++++++++ packages/remix-server-runtime/server.ts | 6 ++++ 4 files changed, 74 insertions(+), 11 deletions(-) create mode 100644 .changeset/handle-network-errors.md diff --git a/.changeset/handle-network-errors.md b/.changeset/handle-network-errors.md new file mode 100644 index 00000000000..ded0ba07a53 --- /dev/null +++ b/.changeset/handle-network-errors.md @@ -0,0 +1,6 @@ +--- +"@remix-run/react": patch +"@remix-run/server-runtime": patch +--- + +Properly handle `?_data` HTTP/Network errors that don't reach the Remix server and ensure they bubble to the `ErrorBoundary` diff --git a/integration/error-boundary-v2-test.ts b/integration/error-boundary-v2-test.ts index 9afb9527599..9e972418a58 100644 --- a/integration/error-boundary-v2-test.ts +++ b/integration/error-boundary-v2-test.ts @@ -164,20 +164,24 @@ test.describe("V2 Singular ErrorBoundary (future.v2_errorBoundary)", () => { test.describe("with JavaScript", () => { test.use({ javaScriptEnabled: true }); runBoundaryTests(); + + test("Network errors that never reach the Remix server", async ({ + page, + }) => { + // Cause a ?_data request to trigger an HTTP error that never reaches the + // Remix server, and ensure we properly handle it at the ErrorBoundary + await page.route( + "**/parent/child-with-boundary?_data=routes%2Fparent.child-with-boundary", + (route) => route.fulfill({ status: 500, body: "CDN Error!" }) + ); + let app = new PlaywrightFixture(appFixture, page); + await app.goto("/parent"); + await app.clickLink("/parent/child-with-boundary"); + await waitForAndAssert(page, app, "#child-error", "CDN Error!"); + }); }); function runBoundaryTests() { - // Shorthand util to wait for an element to appear before asserting it - async function waitForAndAssert( - page: Page, - app: PlaywrightFixture, - selector: string, - match: string - ) { - await page.waitForSelector(selector); - expect(await app.getHtml(selector)).toMatch(match); - } - test("No errors", async ({ page }) => { let app = new PlaywrightFixture(appFixture, page); await app.goto("/parent"); @@ -238,3 +242,14 @@ test.describe("V2 Singular ErrorBoundary (future.v2_errorBoundary)", () => { }); } }); + +// Shorthand util to wait for an element to appear before asserting it +async function waitForAndAssert( + page: Page, + app: PlaywrightFixture, + selector: string, + match: string +) { + await page.waitForSelector(selector); + expect(await app.getHtml(selector)).toMatch(match); +} diff --git a/packages/remix-react/data.ts b/packages/remix-react/data.ts index c2fde12f67e..f8e0d14b574 100644 --- a/packages/remix-react/data.ts +++ b/packages/remix-react/data.ts @@ -18,6 +18,25 @@ export function isErrorResponse(response: any): response is Response { return response.headers.get("X-Remix-Error") != null; } +export function isNetworkErrorResponse(response: any): response is Response { + // If we reach the Remix server, we can safely identify response types via the + // X-Remix-Error/X-Remix-Catch headers. However, if we never reach the Remix + // server, and instead receive a 4xx/5xx from somewhere in between (like + // Cloudflare), then we get a false negative n the isErrorResponse check and + // we incorrectly assume that the user returns the 4xx/5xx response and + // consider it successful. To alleviate this, we add X-Remix-Response to any + // non-Error/non-Catch responses coming back from the server. If we don't + // see this, we can conclude that a 4xx/5xx response never actually reached + // the Remix server and we can bubble it up as an error. + return ( + isResponse(response) && + response.status >= 400 && + response.headers.get("X-Remix-Error") == null && + response.headers.get("X-Remix-Catch") == null && + response.headers.get("X-Remix-Response") == null + ); +} + export function isRedirectResponse(response: Response): boolean { return response.headers.get("X-Remix-Redirect") != null; } @@ -26,6 +45,16 @@ export function isDeferredResponse(response: Response): boolean { return !!response.headers.get("Content-Type")?.match(/text\/remix-deferred/); } +function isResponse(value: any): value is Response { + return ( + value != null && + typeof value.status === "number" && + typeof value.statusText === "string" && + typeof value.headers === "object" && + typeof value.body !== "undefined" + ); +} + export async function fetchData( request: Request, routeId: string, @@ -85,6 +114,13 @@ export async function fetchData( return error; } + if (isNetworkErrorResponse(response)) { + let text = await response.text(); + let error = new Error(text); + error.stack = undefined; + return error; + } + return response; } diff --git a/packages/remix-server-runtime/server.ts b/packages/remix-server-runtime/server.ts index eb30cb36f18..81dfd59c05d 100644 --- a/packages/remix-server-runtime/server.ts +++ b/packages/remix-server-runtime/server.ts @@ -166,10 +166,16 @@ async function handleDataRequestRR( let init = deferredData.init || {}; let headers = new Headers(init.headers); headers.set("Content-Type", "text/remix-deferred"); + // Mark successful responses with a header so we can identify in-flight + // network errors that are missing this header + headers.set("X-Remix-Response", "yes"); init.headers = headers; return new Response(body, init); } + // Mark all successful responses with a header so we can identify in-flight + // network errors that are missing this header + response.headers.set("X-Remix-Response", "yes"); return response; } catch (error: unknown) { if (isResponse(error)) { From 4321b36c97b16bf0e139924df82ddc4520028b9c Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Tue, 11 Jul 2023 15:23:52 -0400 Subject: [PATCH 2/2] Add unit tests for header mutual exclusivity --- .../__tests__/data-test.ts | 174 +++++++++++++++++- 1 file changed, 172 insertions(+), 2 deletions(-) diff --git a/packages/remix-server-runtime/__tests__/data-test.ts b/packages/remix-server-runtime/__tests__/data-test.ts index 5b64f3c7118..01eabf1781e 100644 --- a/packages/remix-server-runtime/__tests__/data-test.ts +++ b/packages/remix-server-runtime/__tests__/data-test.ts @@ -1,4 +1,5 @@ import type { ServerBuild } from "../build"; +import { defer } from "../responses"; import { createRequestHandler } from "../server"; describe("loaders", () => { @@ -39,7 +40,133 @@ describe("loaders", () => { expect(await res.json()).toMatchInlineSnapshot(`"?foo=bar"`); }); - it("sets header for throw responses", async () => { + it("sets X-Remix-Response header for returned 2xx response", async () => { + let routeId = "routes/random"; + let build = { + routes: { + [routeId]: { + id: routeId, + path: "/random", + module: { + async loader() { + return new Response("text", { + status: 200, + headers: { "Content-Type": "text/plain" }, + }); + }, + }, + }, + }, + entry: { + module: { + handleError() {}, + }, + }, + future: {}, + } as unknown as ServerBuild; + + let handler = createRequestHandler(build); + + let request = new Request( + "http://example.com/random?_data=routes/random&foo=bar", + { + headers: { + "Content-Type": "application/json", + }, + } + ); + + let res = await handler(request); + expect(res.headers.get("X-Remix-Response")).toBeTruthy(); + expect(res.headers.get("X-Remix-Error")).toBeNull(); + expect(res.headers.get("X-Remix-Catch")).toBeNull(); + expect(res.headers.get("X-Remix-Redirect")).toBeNull(); + }); + + it("sets X-Remix-Response header for returned 2xx defer response", async () => { + let routeId = "routes/random"; + let build = { + routes: { + [routeId]: { + id: routeId, + path: "/random", + module: { + async loader() { + return defer({ lazy: Promise.resolve("hey!") }); + }, + }, + }, + }, + entry: { + module: { + handleError() {}, + }, + }, + future: {}, + } as unknown as ServerBuild; + + let handler = createRequestHandler(build); + + let request = new Request( + "http://example.com/random?_data=routes/random&foo=bar", + { + headers: { + "Content-Type": "application/json", + }, + } + ); + + let res = await handler(request); + expect(res.headers.get("X-Remix-Response")).toBeTruthy(); + expect(res.headers.get("X-Remix-Error")).toBeNull(); + expect(res.headers.get("X-Remix-Catch")).toBeNull(); + expect(res.headers.get("X-Remix-Redirect")).toBeNull(); + }); + + it("sets X-Remix-Redirect header for returned 3xx redirect", async () => { + let routeId = "routes/random"; + let build = { + routes: { + [routeId]: { + id: routeId, + path: "/random", + module: { + async loader() { + return new Response("text", { + status: 302, + headers: { Location: "https://remix.run" }, + }); + }, + }, + }, + }, + entry: { + module: { + handleError() {}, + }, + }, + future: {}, + } as unknown as ServerBuild; + + let handler = createRequestHandler(build); + + let request = new Request( + "http://example.com/random?_data=routes/random&foo=bar", + { + headers: { + "Content-Type": "application/json", + }, + } + ); + + let res = await handler(request); + expect(res.headers.get("X-Remix-Redirect")).toBeTruthy(); + expect(res.headers.get("X-Remix-Error")).toBeNull(); + expect(res.headers.get("X-Remix-Catch")).toBeNull(); + expect(res.headers.get("X-Remix-Response")).toBeNull(); + }); + + it("sets X-Remix-Catch header for throw responses", async () => { let loader = async ({ request }) => { throw new Response("null", { headers: { @@ -75,7 +202,50 @@ describe("loaders", () => { ); let res = await handler(request); - expect(await res.headers.get("X-Remix-Catch")).toBeTruthy(); + expect(res.headers.get("X-Remix-Catch")).toBeTruthy(); + expect(res.headers.get("X-Remix-Error")).toBeNull(); + expect(res.headers.get("X-Remix-Redirect")).toBeNull(); + expect(res.headers.get("X-Remix-Response")).toBeNull(); + }); + + it("sets X-Remix-Error header for throw error", async () => { + let routeId = "routes/random"; + let build = { + routes: { + [routeId]: { + id: routeId, + path: "/random", + module: { + async loader() { + throw new Error("broke!"); + }, + }, + }, + }, + entry: { + module: { + handleError() {}, + }, + }, + future: {}, + } as unknown as ServerBuild; + + let handler = createRequestHandler(build); + + let request = new Request( + "http://example.com/random?_data=routes/random&foo=bar", + { + headers: { + "Content-Type": "application/json", + }, + } + ); + + let res = await handler(request); + expect(res.headers.get("X-Remix-Error")).toBeTruthy(); + expect(res.headers.get("X-Remix-Catch")).toBeNull(); + expect(res.headers.get("X-Remix-Redirect")).toBeNull(); + expect(res.headers.get("X-Remix-Response")).toBeNull(); }); it("removes index from request.url", async () => {