From 2c9d74bb0e911e1f92d3de31496a15979e32d1f2 Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Wed, 22 Feb 2023 16:57:02 -0500 Subject: [PATCH 1/7] Ensure stack traces are removed from all server side errors --- .changeset/tidy-jokes-sparkle.md | 5 + integration/error-sanitization-test.ts | 358 +++++++++++++++++++++ integration/helpers/create-fixture.ts | 8 +- packages/remix-server-runtime/errors.ts | 38 ++- packages/remix-server-runtime/responses.ts | 18 +- packages/remix-server-runtime/server.ts | 61 ++-- 6 files changed, 444 insertions(+), 44 deletions(-) create mode 100644 .changeset/tidy-jokes-sparkle.md create mode 100644 integration/error-sanitization-test.ts diff --git a/.changeset/tidy-jokes-sparkle.md b/.changeset/tidy-jokes-sparkle.md new file mode 100644 index 00000000000..9862266a585 --- /dev/null +++ b/.changeset/tidy-jokes-sparkle.md @@ -0,0 +1,5 @@ +--- +"@remix-run/server-runtime": patch +--- + +Ensure stack traces are removed from all server side errors in production diff --git a/integration/error-sanitization-test.ts b/integration/error-sanitization-test.ts new file mode 100644 index 00000000000..7c3cd1bc0a8 --- /dev/null +++ b/integration/error-sanitization-test.ts @@ -0,0 +1,358 @@ +import { test, expect } from "@playwright/test"; +import { ServerMode } from "@remix-run/server-runtime/mode"; + +import { createFixture, js } from "./helpers/create-fixture"; +import type { Fixture } from "./helpers/create-fixture"; + +const routeFiles = { + "app/root.jsx": js` + import { Links, Meta, Outlet, Scripts } from "@remix-run/react"; + + export default function Root() { + return ( + + + + + + +
+ +
+ + + + ); + } + `, + + "app/routes/index.jsx": js` + import { useLoaderData, useLocation } from "@remix-run/react"; + + export function loader({ request }) { + if (new URL(request.url).searchParams.has('loader')) { + throw new Error("Loader Error"); + } + return "LOADER" + } + + export default function Component() { + let data = useLoaderData(); + let location = useLocation(); + + if (location.search.includes('render')) { + throw new Error("Render Error"); + } + + return ( + <> +

Index Route

+

{JSON.stringify(data)}

+ + ); + } + + export function ErrorBoundary({ error }) { + return ( + <> +

Index Error

+

{"MESSAGE:" + error.message}

+ {error.stack ?

{"STACK:" + error.stack}

: null} + + ); + } + `, + + "app/routes/defer.jsx": js` + import * as React from 'react'; + import { defer } from "@remix-run/server-runtime"; + import { Await, useLoaderData, useRouteError } from "@remix-run/react"; + + export function loader({ request }) { + if (new URL(request.url).searchParams.has('loader')) { + return defer({ + lazy: Promise.reject(new Error("REJECTED")), + }) + } + return defer({ + lazy: Promise.resolve("RESOLVED"), + }) + } + + export default function Component() { + let data = useLoaderData(); + + return ( + <> +

Defer Route

+ Loading...

}> + }> + {(val) =>

{val}

} + + + + ); + } + + function AwaitError() { + let error = useRouteError(); + return ( + <> +

Defer Error

+

{error}

+ + ); + } + + export function ErrorBoundary({ error }) { + return ( + <> +

Index Error

+

{"MESSAGE:" + error.message}

+ {error.stack ?

{"STACK:" + error.stack}

: null} + + ); + } + `, + + "app/routes/resource.jsx": js` + export function loader({ request }) { + if (new URL(request.url).searchParams.has('loader')) { + throw new Error("Loader Error"); + } + return "RESOURCE LOADER" + } + `, +}; + +test.describe("Error Sanitization", () => { + let fixture: Fixture; + let oldConsoleError: () => void; + + test.beforeEach(() => { + oldConsoleError = console.error; + console.error = () => {}; + }); + + test.afterEach(() => { + console.error = oldConsoleError; + }); + + test.describe("serverMode=production", () => { + test.beforeAll(async () => { + fixture = await createFixture({ + files: routeFiles, + }); + }); + + test("renders document without errors", async () => { + let response = await fixture.requestDocument("/"); + let html = await response.text(); + expect(html).toMatch("Index Route"); + expect(html).toMatch("LOADER"); + expect(html).not.toMatch("MESSAGE:"); + expect(html).not.toMatch(/stack/i); + }); + + test("sanitizes loader errors in document requests", async () => { + let response = await fixture.requestDocument("/?loader"); + let html = await response.text(); + expect(html).toMatch("Index Error"); + expect(html).not.toMatch("LOADER"); + expect(html).toMatch("MESSAGE:Unexpected Server Error"); + expect(html).toMatch( + '{"routes/index":{"message":"Unexpected Server Error","__type":"Error"}}' + ); + expect(html).not.toMatch(/stack/i); + }); + + test("sanitizes render errors in document requests", async () => { + let response = await fixture.requestDocument("/?render"); + let html = await response.text(); + expect(html).toMatch("Index Error"); + expect(html).toMatch("MESSAGE:Unexpected Server Error"); + expect(html).toMatch( + '{"routes/index":{"message":"Unexpected Server Error","__type":"Error"}}' + ); + expect(html).not.toMatch(/stack/i); + }); + + test("renders deferred document without errors", async () => { + let response = await fixture.requestDocument("/defer"); + let html = await response.text(); + expect(html).toMatch("Defer Route"); + expect(html).toMatch("RESOLVED"); + expect(html).not.toMatch("MESSAGE:"); + expect(html).not.toMatch(/stack/i); + }); + + test("sanitizes defer errors in document requests", async () => { + let response = await fixture.requestDocument("/defer?loader"); + let html = await response.text(); + expect(html).toMatch("Defer Error"); + expect(html).not.toMatch("RESOLVED"); + // TODO: is this expected or should we get "Unexpected Server Error" here? + expect(html).toMatch('{"message":"REJECTED"}'); + expect(html).not.toMatch(/stack/i); + }); + + test("returns data without errors", async () => { + let response = await fixture.requestData("/", "routes/index"); + let text = await response.text(); + expect(text).toMatch("LOADER"); + expect(text).not.toMatch("MESSAGE:"); + expect(text).not.toMatch(/stack/i); + }); + + test("sanitizes loader errors in data requests", async () => { + let response = await fixture.requestData("/?loader", "routes/index"); + let text = await response.text(); + expect(text).toBe('{"message":"Unexpected Server Error"}'); + }); + + test("returns deferred data without errors", async () => { + let response = await fixture.requestData("/defer", "routes/defer"); + let text = await response.text(); + expect(text).toMatch("RESOLVED"); + expect(text).not.toMatch("REJECTED"); + expect(text).not.toMatch(/stack/i); + }); + + test("sanitizes loader errors in deferred data requests", async () => { + let response = await fixture.requestData("/defer?loader", "routes/defer"); + let text = await response.text(); + expect(text).toBe( + '{"lazy":"__deferred_promise:lazy"}\n\n' + + 'error:{"lazy":{"message":"Unexpected Server Error"}}\n\n' + ); + }); + + test("sanitizes loader errors in resource requests", async () => { + let response = await fixture.requestData( + "/resource?loader", + "routes/resource" + ); + let text = await response.text(); + expect(text).toBe('{"message":"Unexpected Server Error"}'); + }); + + test("sanitizes mismatched route errors in data requests", async () => { + let response = await fixture.requestData("/", "not-a-route"); + let text = await response.text(); + expect(text).toBe('{"message":"Unexpected Server Error"}'); + }); + }); + + test.describe("serverMode=development", () => { + test.beforeAll(async () => { + fixture = await createFixture( + { + files: routeFiles, + }, + ServerMode.Development + ); + }); + + test("renders document without errors", async () => { + let response = await fixture.requestDocument("/"); + let html = await response.text(); + expect(html).toMatch("Index Route"); + expect(html).toMatch("LOADER"); + expect(html).not.toMatch("MESSAGE:"); + expect(html).not.toMatch(/stack/i); + }); + + test("does not sanitize loader errors in document requests", async () => { + let response = await fixture.requestDocument("/?loader"); + let html = await response.text(); + expect(html).toMatch("Index Error"); + expect(html).not.toMatch("LOADER"); + expect(html).toMatch("

MESSAGE:Loader Error"); + expect(html).toMatch("

STACK:Error: Loader Error"); + expect(html).toMatch( + 'errors":{"routes/index":{"message":"Loader Error","stack":"Error: Loader Error\\n' + ); + }); + + test("does not sanitize render errors in document requests", async () => { + let response = await fixture.requestDocument("/?render"); + let html = await response.text(); + expect(html).toMatch("Index Error"); + expect(html).toMatch("

MESSAGE:Render Error"); + expect(html).toMatch("

STACK:Error: Render Error"); + expect(html).toMatch( + 'errors":{"routes/index":{"message":"Render Error","stack":"Error: Render Error\\n' + ); + }); + + test("renders deferred document without errors", async () => { + let response = await fixture.requestDocument("/defer"); + let html = await response.text(); + expect(html).toMatch("Defer Route"); + expect(html).toMatch("RESOLVED"); + expect(html).not.toMatch("MESSAGE:"); + expect(html).not.toMatch(/stack/i); + }); + + test("does not sanitize defer errors in document requests", async () => { + let response = await fixture.requestDocument("/defer?loader"); + let html = await response.text(); + expect(html).toMatch("Defer Error"); + expect(html).not.toMatch("RESOLVED"); + expect(html).toMatch('{"message":"REJECTED"}'); + // TODO: I think we should be getting the stack here too? + expect(html).toMatch(/stack/i); + }); + + test("returns data without errors", async () => { + let response = await fixture.requestData("/", "routes/index"); + let text = await response.text(); + expect(text).toMatch("LOADER"); + expect(text).not.toMatch("MESSAGE:"); + expect(text).not.toMatch(/stack/i); + }); + + test("does not sanitize loader errors in data requests", async () => { + let response = await fixture.requestData("/?loader", "routes/index"); + let text = await response.text(); + expect(text).toMatch( + '{"message":"Loader Error","stack":"Error: Loader Error' + ); + }); + + test("returns deferred data without errors", async () => { + let response = await fixture.requestData("/defer", "routes/defer"); + let text = await response.text(); + expect(text).toMatch("RESOLVED"); + expect(text).not.toMatch("REJECTED"); + expect(text).not.toMatch(/stack/i); + }); + + test("does not sanitize loader errors in deferred data requests", async () => { + let response = await fixture.requestData("/defer?loader", "routes/defer"); + let text = await response.text(); + expect(text).toMatch( + 'error:{"lazy":{"message":"REJECTED","stack":"Error: REJECTED' + ); + }); + + test("does not sanitize loader errors in resource requests", async () => { + let response = await fixture.requestData( + "/resource?loader", + "routes/resource" + ); + let text = await response.text(); + expect(text).toMatch( + '{"message":"Loader Error","stack":"Error: Loader Error' + ); + }); + + test("sanitizes mismatched route errors in data requests", async () => { + let response = await fixture.requestData("/", "not-a-route"); + let text = await response.text(); + expect(text).toMatch( + '{"message":"Route \\"not-a-route\\" does not match URL \\"/\\"","stack":"Error: Route \\"not-a-route\\" does not match URL \\"/\\"' + ); + }); + }); +}); diff --git a/integration/helpers/create-fixture.ts b/integration/helpers/create-fixture.ts index 4238d6a3b75..d5828db1f59 100644 --- a/integration/helpers/create-fixture.ts +++ b/integration/helpers/create-fixture.ts @@ -6,7 +6,7 @@ import getPort from "get-port"; import stripIndent from "strip-indent"; import { sync as spawnSync } from "cross-spawn"; import type { JsonObject } from "type-fest"; -import type { ServerMode } from "@remix-run/server-runtime/mode"; +import { ServerMode } from "@remix-run/server-runtime/mode"; import type { FutureConfig } from "@remix-run/server-runtime/entry"; import type { ServerBuild } from "../../build/node_modules/@remix-run/server-runtime"; @@ -34,11 +34,11 @@ export function json(value: JsonObject) { return JSON.stringify(value, null, 2); } -export async function createFixture(init: FixtureInit) { +export async function createFixture(init: FixtureInit, mode?: ServerMode) { let projectDir = await createFixtureProject(init); let buildPath = path.resolve(projectDir, "build"); let app: ServerBuild = await import(buildPath); - let handler = createRequestHandler(app, "production"); + let handler = createRequestHandler(app, mode || ServerMode.Production); let requestDocument = async (href: string, init?: RequestInit) => { let url = new URL(href, "test://test"); @@ -106,7 +106,7 @@ export async function createAppFixture(fixture: Fixture, mode?: ServerMode) { "*", createExpressHandler({ build: fixture.build, - mode: mode || "production", + mode: mode || ServerMode.Production, }) ); diff --git a/packages/remix-server-runtime/errors.ts b/packages/remix-server-runtime/errors.ts index 85f98407f8c..e53704d874c 100644 --- a/packages/remix-server-runtime/errors.ts +++ b/packages/remix-server-runtime/errors.ts @@ -1,6 +1,8 @@ import type { StaticHandlerContext } from "@remix-run/router"; import { isRouteErrorResponse } from "@remix-run/router"; +import { ServerMode } from "./mode"; + /** * This thing probably warrants some explanation. * @@ -50,6 +52,24 @@ export interface ThrownResponse { data: T; } +export function sanitizeError(error: T, serverMode: ServerMode) { + if (error instanceof Error && serverMode !== ServerMode.Development) { + let sanitized = new Error("Unexpected Server Error"); + sanitized.stack = undefined; + return sanitized; + } + return error; +} + +export function sanitizeErrors( + errors: NonNullable, + serverMode: ServerMode +) { + return Object.entries(errors).reduce((acc, [routeId, error]) => { + return Object.assign(acc, { [routeId]: sanitizeError(error, serverMode) }); + }, {}); +} + // must be type alias due to inference issues on interfaces // https://github.com/microsoft/TypeScript/issues/15300 export type SerializedError = { @@ -57,15 +77,20 @@ export type SerializedError = { stack?: string; }; -export async function serializeError(error: Error): Promise { +export function serializeError( + error: Error, + serverMode: ServerMode +): SerializedError { + let sanitized = sanitizeError(error, serverMode); return { - message: error.message, - stack: error.stack, + message: sanitized.message, + stack: sanitized.stack, }; } export function serializeErrors( - errors: StaticHandlerContext["errors"] + errors: StaticHandlerContext["errors"], + serverMode: ServerMode ): StaticHandlerContext["errors"] { if (!errors) return null; let entries = Object.entries(errors); @@ -76,9 +101,10 @@ export function serializeErrors( if (isRouteErrorResponse(val)) { serialized[key] = { ...val, __type: "RouteErrorResponse" }; } else if (val instanceof Error) { + let sanitized = sanitizeError(val, serverMode); serialized[key] = { - message: val.message, - stack: val.stack, + message: sanitized.message, + stack: sanitized.stack, __type: "Error", }; } else { diff --git a/packages/remix-server-runtime/responses.ts b/packages/remix-server-runtime/responses.ts index 967c10eb24f..2605edffccb 100644 --- a/packages/remix-server-runtime/responses.ts +++ b/packages/remix-server-runtime/responses.ts @@ -5,6 +5,7 @@ import { } from "@remix-run/router"; import { serializeError } from "./errors"; +import type { ServerMode } from "./mode"; export type TypedDeferredData> = Pick< DeferredData, @@ -142,7 +143,8 @@ function isTrackedPromise(value: any): value is TrackedPromise { const DEFERRED_VALUE_PLACEHOLDER_PREFIX = "__deferred_promise:"; export function createDeferredReadableStream( deferredData: DeferredData, - signal: AbortSignal + signal: AbortSignal, + serverMode: ServerMode ): any { let encoder = new TextEncoder(); let stream = new ReadableStream({ @@ -172,7 +174,8 @@ export function createDeferredReadableStream( controller, encoder, preresolvedKey, - deferredData.data[preresolvedKey] as TrackedPromise + deferredData.data[preresolvedKey] as TrackedPromise, + serverMode ); } @@ -182,7 +185,8 @@ export function createDeferredReadableStream( controller, encoder, settledKey, - deferredData.data[settledKey] as TrackedPromise + deferredData.data[settledKey] as TrackedPromise, + serverMode ); } }); @@ -199,14 +203,18 @@ function enqueueTrackedPromise( controller: any, encoder: TextEncoder, settledKey: string, - promise: TrackedPromise + promise: TrackedPromise, + serverMode: ServerMode ) { if ("_error" in promise) { controller.enqueue( encoder.encode( "error:" + JSON.stringify({ - [settledKey]: serializeError(promise._error), + [settledKey]: + promise._error instanceof Error + ? serializeError(promise._error, serverMode) + : promise._error, }) + "\n\n" ) diff --git a/packages/remix-server-runtime/server.ts b/packages/remix-server-runtime/server.ts index 0c6503a3588..7c778d469a0 100644 --- a/packages/remix-server-runtime/server.ts +++ b/packages/remix-server-runtime/server.ts @@ -14,7 +14,7 @@ import type { AppLoadContext } from "./data"; import type { ServerBuild } from "./build"; import type { EntryContext } from "./entry"; import { createEntryRouteModules } from "./entry"; -import { serializeError, serializeErrors } from "./errors"; +import { sanitizeErrors, serializeError, serializeErrors } from "./errors"; import { getDocumentHeadersRR } from "./headers"; import invariant from "./invariant"; import { ServerMode, isServerMode } from "./mode"; @@ -89,7 +89,7 @@ export const createRequestHandler: CreateRequestHandlerFunction = ( let match = matches!.find((match) => match.route.id == routeId)!; response = await build.entry.module.handleDataRequest(response, { context: loadContext, - params: match.params, + params: match ? match.params : {}, request, }); } @@ -159,7 +159,11 @@ async function handleDataRequestRR( if (DEFERRED_SYMBOL in response) { let deferredData = response[DEFERRED_SYMBOL] as DeferredData; - let body = createDeferredReadableStream(deferredData, request.signal); + let body = createDeferredReadableStream( + deferredData, + request.signal, + serverMode + ); let init = deferredData.init || {}; let headers = new Headers(init.headers); headers.set("Content-Type", "text/remix-deferred"); @@ -174,26 +178,24 @@ async function handleDataRequestRR( return error; } - let status = 500; - let errorInstance = error; - - if (isRouteErrorResponse(error)) { - status = error.status; - errorInstance = error.error || errorInstance; - } + let status = isRouteErrorResponse(error) ? error.status : 500; + let errorInstance = + isRouteErrorResponse(error) && error.error + ? error.error + : error instanceof Error + ? error + : new Error("Unexpected Server Error"); if (serverMode !== ServerMode.Test && !request.signal.aborted) { console.error(errorInstance); } - if ( - serverMode === ServerMode.Development && - errorInstance instanceof Error - ) { - return errorBoundaryError(errorInstance, status); - } - - return errorBoundaryError(new Error("Unexpected Server Error"), status); + return json(serializeError(errorInstance, serverMode), { + status, + headers: { + "X-Remix-Error": "yes", + }, + }); } } @@ -270,6 +272,11 @@ async function handleDocumentRequestRR( return context; } + // Sanitize errors outside of development environments + if (context.errors) { + context.errors = sanitizeErrors(context.errors, serverMode); + } + // Restructure context.errors to the right Catch/Error Boundary if (build.future.v2_errorBoundary !== true) { differentiateCatchVersusErrorBoundaries(build, context); @@ -285,7 +292,7 @@ async function handleDocumentRequestRR( state: { loaderData: context.loaderData, actionData: context.actionData, - errors: serializeErrors(context.errors), + errors: serializeErrors(context.errors, serverMode), }, future: build.future, dev: build.dev, @@ -309,6 +316,11 @@ async function handleDocumentRequestRR( error ); + // Sanitize errors outside of development environments + if (context.errors) { + context.errors = sanitizeErrors(context.errors, serverMode); + } + // Restructure context.errors to the right Catch/Error Boundary if (build.future.v2_errorBoundary !== true) { differentiateCatchVersusErrorBoundaries(build, context); @@ -322,7 +334,7 @@ async function handleDocumentRequestRR( state: { loaderData: context.loaderData, actionData: context.actionData, - errors: serializeErrors(context.errors), + errors: serializeErrors(context.errors, serverMode), }, future: build.future, }), @@ -373,15 +385,6 @@ async function handleResourceRequestRR( } } -async function errorBoundaryError(error: Error, status: number) { - return json(await serializeError(error), { - status, - headers: { - "X-Remix-Error": "yes", - }, - }); -} - function returnLastResortErrorResponse(error: any, serverMode?: ServerMode) { if (serverMode !== ServerMode.Test) { console.error(error); From 4e2096a668c788d71a904ad16bbca33d15c6c09c Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Mon, 6 Mar 2023 17:15:44 -0500 Subject: [PATCH 2/7] Fix up tests expecting unsanitized errors --- integration/error-boundary-test.ts | 19 +++++--- .../__tests__/server-test.ts | 46 +++++++++++++++---- 2 files changed, 49 insertions(+), 16 deletions(-) diff --git a/integration/error-boundary-test.ts b/integration/error-boundary-test.ts index e660a20ff04..a8360494e78 100644 --- a/integration/error-boundary-test.ts +++ b/integration/error-boundary-test.ts @@ -418,7 +418,9 @@ test.describe("ErrorBoundary", () => { await page.waitForSelector("#child-error"); // Preserves parent loader data expect(await app.getHtml("#parent-data")).toMatch("PARENT"); - expect(await app.getHtml("#child-error")).toMatch("Broken!"); + expect(await app.getHtml("#child-error")).toMatch( + "Unexpected Server Error" + ); }); test("renders own boundary in fetcher action submission without action from other routes", async ({ @@ -988,9 +990,12 @@ test.describe("Default ErrorBoundary", () => { test.describe("When the root route does not have a boundary", () => { test.beforeAll(async () => { - fixture = await createFixture({ - files: getFiles({ includeRootErrorBoundary: false }), - }); + fixture = await createFixture( + { + files: getFiles({ includeRootErrorBoundary: false }), + }, + ServerMode.Development + ); appFixture = await createAppFixture(fixture, ServerMode.Development); }); @@ -1074,7 +1079,8 @@ test.describe("Default ErrorBoundary", () => { expect(res.status).toBe(500); let text = await res.text(); expect(text).toMatch("Root Error Boundary"); - expect(text).toMatch("Loader Error"); + expect(text).toMatch("Unexpected Server Error"); + expect(text).not.toMatch("Loader Error"); expect(text).not.toMatch("Application Error"); }); @@ -1083,7 +1089,8 @@ test.describe("Default ErrorBoundary", () => { expect(res.status).toBe(500); let text = await res.text(); expect(text).toMatch("Root Error Boundary"); - expect(text).toMatch("Render Error"); + expect(text).toMatch("Unexpected Server Error"); + expect(text).not.toMatch("Render Error"); expect(text).not.toMatch("Application Error"); }); }); diff --git a/packages/remix-server-runtime/__tests__/server-test.ts b/packages/remix-server-runtime/__tests__/server-test.ts index 51b3a0353fb..8c1d33708a3 100644 --- a/packages/remix-server-runtime/__tests__/server-test.ts +++ b/packages/remix-server-runtime/__tests__/server-test.ts @@ -1235,7 +1235,9 @@ describe("shared server runtime", () => { expect(calls.length).toBe(1 * DATA_CALL_MULTIPIER); let context = calls[0][3].staticHandlerContext as StaticHandlerContext; expect(context.errors).toBeTruthy(); - expect(context.errors!.root.message).toBe("index"); + expect(context.errors!.root).toBeInstanceOf(Error); + expect(context.errors!.root.message).toBe("Unexpected Server Error"); + expect(context.errors!.root.stack).toBeUndefined(); expect(context.loaderData).toEqual({ root: "root", }); @@ -1278,7 +1280,11 @@ describe("shared server runtime", () => { expect(calls.length).toBe(1 * DATA_CALL_MULTIPIER); let context = calls[0][3].staticHandlerContext as StaticHandlerContext; expect(context.errors).toBeTruthy(); - expect(context.errors!["routes/index"].message).toBe("index"); + expect(context.errors!["routes/index"]).toBeInstanceOf(Error); + expect(context.errors!["routes/index"].message).toBe( + "Unexpected Server Error" + ); + expect(context.errors!["routes/index"].stack).toBeUndefined(); expect(context.loaderData).toEqual({ root: "root", }); @@ -1326,7 +1332,9 @@ describe("shared server runtime", () => { expect(calls.length).toBe(1 * DATA_CALL_MULTIPIER); let context = calls[0][3].staticHandlerContext as StaticHandlerContext; expect(context.errors).toBeTruthy(); - expect(context.errors!.root.message).toBe("test"); + expect(context.errors!.root).toBeInstanceOf(Error); + expect(context.errors!.root.message).toBe("Unexpected Server Error"); + expect(context.errors!.root.stack).toBeUndefined(); expect(context.loaderData).toEqual({ root: null, "routes/test": null, @@ -1375,7 +1383,9 @@ describe("shared server runtime", () => { expect(calls.length).toBe(1 * DATA_CALL_MULTIPIER); let context = calls[0][3].staticHandlerContext as StaticHandlerContext; expect(context.errors).toBeTruthy(); - expect(context.errors!.root.message).toBe("index"); + expect(context.errors!.root).toBeInstanceOf(Error); + expect(context.errors!.root.message).toBe("Unexpected Server Error"); + expect(context.errors!.root.stack).toBeUndefined(); expect(context.loaderData).toEqual({ root: null, "routes/index": null, @@ -1424,7 +1434,11 @@ describe("shared server runtime", () => { expect(calls.length).toBe(1 * DATA_CALL_MULTIPIER); let context = calls[0][3].staticHandlerContext as StaticHandlerContext; expect(context.errors).toBeTruthy(); - expect(context.errors!["routes/test"].message).toBe("test"); + expect(context.errors!["routes/test"]).toBeInstanceOf(Error); + expect(context.errors!["routes/test"].message).toBe( + "Unexpected Server Error" + ); + expect(context.errors!["routes/test"].stack).toBeUndefined(); expect(context.loaderData).toEqual({ root: "root", "routes/test": null, @@ -1473,7 +1487,11 @@ describe("shared server runtime", () => { expect(calls.length).toBe(1 * DATA_CALL_MULTIPIER); let context = calls[0][3].staticHandlerContext as StaticHandlerContext; expect(context.errors).toBeTruthy(); - expect(context.errors!["routes/index"].message).toBe("index"); + expect(context.errors!["routes/index"]).toBeInstanceOf(Error); + expect(context.errors!["routes/index"].message).toBe( + "Unexpected Server Error" + ); + expect(context.errors!["routes/index"].stack).toBeUndefined(); expect(context.loaderData).toEqual({ root: "root", "routes/index": null, @@ -1530,7 +1548,11 @@ describe("shared server runtime", () => { expect(calls.length).toBe(1 * DATA_CALL_MULTIPIER); let context = calls[0][3].staticHandlerContext as StaticHandlerContext; expect(context.errors).toBeTruthy(); - expect(context.errors!["routes/__layout"].message).toBe("action"); + expect(context.errors!["routes/__layout"]).toBeInstanceOf(Error); + expect(context.errors!["routes/__layout"].message).toBe( + "Unexpected Server Error" + ); + expect(context.errors!["routes/__layout"].stack).toBeUndefined(); expect(context.loaderData).toEqual({ root: "root", "routes/__layout": null, @@ -1588,7 +1610,11 @@ describe("shared server runtime", () => { expect(calls.length).toBe(1 * DATA_CALL_MULTIPIER); let context = calls[0][3].staticHandlerContext as StaticHandlerContext; expect(context.errors).toBeTruthy(); - expect(context.errors!["routes/__layout"].message).toBe("action"); + expect(context.errors!["routes/__layout"]).toBeInstanceOf(Error); + expect(context.errors!["routes/__layout"].message).toBe( + "Unexpected Server Error" + ); + expect(context.errors!["routes/__layout"].stack).toBeUndefined(); expect(context.loaderData).toEqual({ root: "root", "routes/__layout": null, @@ -1624,7 +1650,7 @@ describe("shared server runtime", () => { calledBefore = true; return ogHandleDocumentRequest.call(null, arguments); }) as any; - let handler = createRequestHandler(build, ServerMode.Test); + let handler = createRequestHandler(build, ServerMode.Development); let request = new Request(`${baseUrl}/`, { method: "get" }); @@ -1637,7 +1663,7 @@ describe("shared server runtime", () => { expect(calls.length).toBe(2 * DATA_CALL_MULTIPIER); let context = calls[1][3].staticHandlerContext; expect(context.errors.root).toBeTruthy(); - expect(context.errors.root.message).toBe("thrown"); + expect(context.errors!.root.message).toBe("thrown"); expect(context.loaderData).toEqual({}); }); From ef3042ebe5046573b62088e387ba28ab2301af5f Mon Sep 17 00:00:00 2001 From: Matt Brophy Date: Mon, 6 Mar 2023 18:14:53 -0500 Subject: [PATCH 3/7] Add sanitization to defer errors --- integration/error-sanitization-test.ts | 23 +++++++------- integration/helpers/create-fixture.ts | 24 +++++++++++--- packages/remix-react/components.tsx | 44 +++++++++++++++----------- 3 files changed, 57 insertions(+), 34 deletions(-) diff --git a/integration/error-sanitization-test.ts b/integration/error-sanitization-test.ts index 7c3cd1bc0a8..a26e407275e 100644 --- a/integration/error-sanitization-test.ts +++ b/integration/error-sanitization-test.ts @@ -140,9 +140,12 @@ test.describe("Error Sanitization", () => { test.describe("serverMode=production", () => { test.beforeAll(async () => { - fixture = await createFixture({ - files: routeFiles, - }); + fixture = await createFixture( + { + files: routeFiles, + }, + ServerMode.Production + ); }); test("renders document without errors", async () => { @@ -183,7 +186,7 @@ test.describe("Error Sanitization", () => { expect(html).toMatch("Defer Route"); expect(html).toMatch("RESOLVED"); expect(html).not.toMatch("MESSAGE:"); - expect(html).not.toMatch(/stack/i); + expect(html).not.toMatch(/"stack":/i); }); test("sanitizes defer errors in document requests", async () => { @@ -191,9 +194,8 @@ test.describe("Error Sanitization", () => { let html = await response.text(); expect(html).toMatch("Defer Error"); expect(html).not.toMatch("RESOLVED"); - // TODO: is this expected or should we get "Unexpected Server Error" here? - expect(html).toMatch('{"message":"REJECTED"}'); - expect(html).not.toMatch(/stack/i); + expect(html).toMatch('{"message":"Unexpected Server Error"}'); + expect(html).not.toMatch(/"stack":/i); }); test("returns data without errors", async () => { @@ -291,7 +293,7 @@ test.describe("Error Sanitization", () => { expect(html).toMatch("Defer Route"); expect(html).toMatch("RESOLVED"); expect(html).not.toMatch("MESSAGE:"); - expect(html).not.toMatch(/stack/i); + expect(html).not.toMatch(/"stack":/i); }); test("does not sanitize defer errors in document requests", async () => { @@ -299,9 +301,8 @@ test.describe("Error Sanitization", () => { let html = await response.text(); expect(html).toMatch("Defer Error"); expect(html).not.toMatch("RESOLVED"); - expect(html).toMatch('{"message":"REJECTED"}'); - // TODO: I think we should be getting the stack here too? - expect(html).toMatch(/stack/i); + // TODO: Is our ServerMode.Development not making it into the build somehow? + expect(html).toMatch('{"message":"REJECTED","stack":"}'); }); test("returns data without errors", async () => { diff --git a/integration/helpers/create-fixture.ts b/integration/helpers/create-fixture.ts index d5828db1f59..ff390ea226f 100644 --- a/integration/helpers/create-fixture.ts +++ b/integration/helpers/create-fixture.ts @@ -35,7 +35,7 @@ export function json(value: JsonObject) { } export async function createFixture(init: FixtureInit, mode?: ServerMode) { - let projectDir = await createFixtureProject(init); + let projectDir = await createFixtureProject(init, mode); let buildPath = path.resolve(projectDir, "build"); let app: ServerBuild = await import(buildPath); let handler = createRequestHandler(app, mode || ServerMode.Production); @@ -141,7 +141,8 @@ export async function createAppFixture(fixture: Fixture, mode?: ServerMode) { //////////////////////////////////////////////////////////////////////////////// export async function createFixtureProject( - init: FixtureInit = {} + init: FixtureInit = {}, + mode?: ServerMode ): Promise { let template = init.template ?? "node-template"; let integrationTemplateDir = path.join(__dirname, template); @@ -193,17 +194,30 @@ export async function createFixtureProject( } await writeTestFiles(init, projectDir); - build(projectDir, init.buildStdio, init.sourcemap); + build(projectDir, init.buildStdio, init.sourcemap, mode); return projectDir; } -function build(projectDir: string, buildStdio?: Writable, sourcemap?: boolean) { +function build( + projectDir: string, + buildStdio?: Writable, + sourcemap?: boolean, + mode?: ServerMode +) { let buildArgs = ["node_modules/@remix-run/dev/dist/cli.js", "build"]; if (sourcemap) { buildArgs.push("--sourcemap"); } - let buildSpawn = spawnSync("node", buildArgs, { cwd: projectDir }); + + console.log("Building with NODE_ENV =", mode || ServerMode.Production); + let buildSpawn = spawnSync("node", buildArgs, { + cwd: projectDir, + env: { + ...process.env, + NODE_ENV: mode || ServerMode.Production, + }, + }); // These logs are helpful for debugging. Remove comments if needed. // console.log("spawning @remix-run/dev/cli.js `build`:\n"); diff --git a/packages/remix-react/components.tsx b/packages/remix-react/components.tsx index 092d4e52e6f..f854ff5aa72 100644 --- a/packages/remix-react/components.tsx +++ b/packages/remix-react/components.tsx @@ -815,8 +815,9 @@ export function Scripts(props: ScriptProps) { : [ "__remixContext.p = function(v,e,p,x) {", " if (typeof e !== 'undefined') {", - " x=new Error(e.message);", - process.env.NODE_ENV === "development" ? `x.stack=e.stack;` : "", + process.env.NODE_ENV === "development" + ? " x=new Error(e.message);\n x.stack=e.stack;" + : ' x=new Error("Unexpected Server Error");\n x.stack=undefined;', " p=Promise.reject(x);", " } else {", " p=Promise.resolve(v);", @@ -835,8 +836,9 @@ export function Scripts(props: ScriptProps) { "__remixContext.r = function(i,k,v,e,p,x) {", " p = __remixContext.t[i][k];", " if (typeof e !== 'undefined') {", - " x=new Error(e.message);", - process.env.NODE_ENV === "development" ? `x.stack=e.stack;` : "", + process.env.NODE_ENV === "development" + ? " x=new Error(e.message);\n x.stack=e.stack;" + : ' x=new Error("Unexpected Server Error");\n x.stack=undefined;', " p.e(x);", " } else {", " p.r(v);", @@ -866,13 +868,16 @@ export function Scripts(props: ScriptProps) { } else { let trackedPromise = deferredData.data[key] as TrackedPromise; if (typeof trackedPromise._error !== "undefined") { - let toSerialize: { message: string; stack?: string } = { - message: trackedPromise._error.message, - stack: undefined, - }; - if (process.env.NODE_ENV === "development") { - toSerialize.stack = trackedPromise._error.stack; - } + let toSerialize: { message: string; stack?: string } = + process.env.NODE_ENV === "development" + ? { + message: trackedPromise._error.message, + stack: trackedPromise._error.stack, + } + : { + message: "Unexpected Server Error", + stack: undefined, + }; return `${JSON.stringify( key )}:__remixContext.p(!1, ${escapeHtml( @@ -1078,13 +1083,16 @@ function ErrorDeferredHydrationScript({ routeId: string; }) { let error = useAsyncError() as Error; - let toSerialize: { message: string; stack?: string } = { - message: error.message, - stack: undefined, - }; - if (process.env.NODE_ENV === "development") { - toSerialize.stack = error.stack; - } + let toSerialize: { message: string; stack?: string } = + process.env.NODE_ENV === "development" + ? { + message: error.message, + stack: error.stack, + } + : { + message: "Unexpected Server Error", + stack: undefined, + }; return (