fix: ensure server errors are logged before sanitizing

remix-run · May 26, 2023 · fc512e5 · fc512e5
1 parent 33cc4c0
commit fc512e5
Show file tree

Hide file tree

Showing 7 changed files with 59 additions and 6 deletions.
diff --git a/.changeset/log-server-errors.md b/.changeset/log-server-errors.md
@@ -0,0 +1,5 @@
+---
+"@remix-run/server-runtime": patch
+---
+
+Ensure un-sanitized server errors are logged on the server during document requests
diff --git a/integration/error-sanitization-test.ts b/integration/error-sanitization-test.ts
@@ -128,10 +128,12 @@ const routeFiles = {
 test.describe("Error Sanitization", () => {
   let fixture: Fixture;
   let oldConsoleError: () => void;
+  let errorLogs: any[] = [];
 
   test.beforeEach(() => {
     oldConsoleError = console.error;
-    console.error = () => {};
+    errorLogs = [];
+    console.error = (...args) => errorLogs.push(args);
   });
 
   test.afterEach(() => {
@@ -167,6 +169,9 @@ test.describe("Error Sanitization", () => {
         '{"routes/index":{"message":"Unexpected Server Error","__type":"Error"}}'
       );
       expect(html).not.toMatch(/stack/i);
+      expect(errorLogs.length).toBe(1);
+      expect(errorLogs[0][0].message).toMatch("Loader Error");
+      expect(errorLogs[0][0].stack).toMatch(" at ");
     });
 
     test("sanitizes render errors in document requests", async () => {
@@ -178,6 +183,9 @@ test.describe("Error Sanitization", () => {
         '{"routes/index":{"message":"Unexpected Server Error","__type":"Error"}}'
       );
       expect(html).not.toMatch(/stack/i);
+      expect(errorLogs.length).toBe(1);
+      expect(errorLogs[0][0].message).toMatch("Render Error");
+      expect(errorLogs[0][0].stack).toMatch(" at ");
     });
 
     test("renders deferred document without errors", async () => {
@@ -200,6 +208,9 @@ test.describe("Error Sanitization", () => {
       // Defer errors are not not part of the JSON blob but rather rejected
       // against a pending promise and therefore are inlined JS.
       expect(html).toMatch("x.stack=undefined;");
+      // defer errors are not logged to the server console since the request
+      // has "succeeded"
+      expect(errorLogs.length).toBe(0);
     });
 
     test("returns data without errors", async () => {
@@ -214,6 +225,9 @@ test.describe("Error Sanitization", () => {
       let response = await fixture.requestData("/?loader", "routes/index");
       let text = await response.text();
       expect(text).toBe('{"message":"Unexpected Server Error"}');
+      expect(errorLogs.length).toBe(1);
+      expect(errorLogs[0][0].message).toMatch("Loader Error");
+      expect(errorLogs[0][0].stack).toMatch(" at ");
     });
 
     test("returns deferred data without errors", async () => {
@@ -231,6 +245,9 @@ test.describe("Error Sanitization", () => {
         '{"lazy":"__deferred_promise:lazy"}\n\n' +
           'error:{"lazy":{"message":"Unexpected Server Error"}}\n\n'
       );
+      // defer errors are not logged to the server console since the request
+      // has "succeeded"
+      expect(errorLogs.length).toBe(0);
     });
 
     test("sanitizes loader errors in resource requests", async () => {
@@ -240,12 +257,20 @@ test.describe("Error Sanitization", () => {
       );
       let text = await response.text();
       expect(text).toBe('{"message":"Unexpected Server Error"}');
+      expect(errorLogs.length).toBe(1);
+      expect(errorLogs[0][0].message).toMatch("Loader Error");
+      expect(errorLogs[0][0].stack).toMatch(" at ");
     });
 
     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"}');
+      expect(errorLogs.length).toBe(1);
+      expect(errorLogs[0][0].message).toMatch(
+        'Route "not-a-route" does not match URL "/"'
+      );
+      expect(errorLogs[0][0].stack).toMatch(" at ");
     });
   });
 
@@ -286,6 +311,9 @@ test.describe("Error Sanitization", () => {
       expect(html).toMatch(
         'errors":{"routes/index":{"message":"Loader Error","stack":"Error: Loader Error\\n'
       );
+      expect(errorLogs.length).toBe(1);
+      expect(errorLogs[0][0].message).toMatch("Loader Error");
+      expect(errorLogs[0][0].stack).toMatch(" at ");
     });
 
     test("does not sanitize render errors in document requests", async () => {
@@ -297,6 +325,9 @@ test.describe("Error Sanitization", () => {
       expect(html).toMatch(
         'errors":{"routes/index":{"message":"Render Error","stack":"Error: Render Error\\n'
       );
+      expect(errorLogs.length).toBe(1);
+      expect(errorLogs[0][0].message).toMatch("Render Error");
+      expect(errorLogs[0][0].stack).toMatch(" at ");
     });
 
     test("renders deferred document without errors", async () => {
@@ -316,6 +347,9 @@ test.describe("Error Sanitization", () => {
       // Defer errors are not not part of the JSON blob but rather rejected
       // against a pending promise and therefore are inlined JS.
       expect(html).toMatch("x.stack=e.stack;");
+      // defer errors are not logged to the server console since the request
+      // has "succeeded"
+      expect(errorLogs.length).toBe(0);
     });
 
     test("returns data without errors", async () => {
@@ -332,6 +366,9 @@ test.describe("Error Sanitization", () => {
       expect(text).toMatch(
         '{"message":"Loader Error","stack":"Error: Loader Error'
       );
+      expect(errorLogs.length).toBe(1);
+      expect(errorLogs[0][0].message).toMatch("Loader Error");
+      expect(errorLogs[0][0].stack).toMatch(" at ");
     });
 
     test("returns deferred data without errors", async () => {
@@ -348,6 +385,9 @@ test.describe("Error Sanitization", () => {
       expect(text).toMatch(
         'error:{"lazy":{"message":"REJECTED","stack":"Error: REJECTED'
       );
+      // defer errors are not logged to the server console since the request
+      // has "succeeded"
+      expect(errorLogs.length).toBe(0);
     });
 
     test("does not sanitize loader errors in resource requests", async () => {
@@ -359,6 +399,9 @@ test.describe("Error Sanitization", () => {
       expect(text).toMatch(
         '{"message":"Loader Error","stack":"Error: Loader Error'
       );
+      expect(errorLogs.length).toBe(1);
+      expect(errorLogs[0][0].message).toMatch("Loader Error");
+      expect(errorLogs[0][0].stack).toMatch(" at ");
     });
 
     test("sanitizes mismatched route errors in data requests", async () => {
@@ -367,6 +410,11 @@ test.describe("Error Sanitization", () => {
       expect(text).toMatch(
         '{"message":"Route \\"not-a-route\\" does not match URL \\"/\\"","stack":"Error: Route \\"not-a-route\\" does not match URL \\"/\\"'
       );
+      expect(errorLogs.length).toBe(1);
+      expect(errorLogs[0][0].message).toMatch(
+        'Route "not-a-route" does not match URL "/"'
+      );
+      expect(errorLogs[0][0].stack).toMatch(" at ");
     });
   });
 });
diff --git a/packages/remix-dev/config/defaults/cloudflare/entry.server.react-stream.tsx b/packages/remix-dev/config/defaults/cloudflare/entry.server.react-stream.tsx
@@ -14,7 +14,6 @@ export default async function handleRequest(
     {
       signal: request.signal,
       onError(error: unknown) {
-        console.error(error);
         responseStatusCode = 500;
       },
     }

diff --git a/packages/remix-dev/config/defaults/deno/entry.server.react-stream.tsx b/packages/remix-dev/config/defaults/deno/entry.server.react-stream.tsx
@@ -14,7 +14,6 @@ export default async function handleRequest(
     {
       signal: request.signal,
       onError(error: unknown) {
-        console.error(error);
         responseStatusCode = 500;
       },
     }

diff --git a/packages/remix-dev/config/defaults/node/entry.server.react-stream.tsx b/packages/remix-dev/config/defaults/node/entry.server.react-stream.tsx
@@ -62,7 +62,6 @@ function handleBotRequest(
         },
         onError(error: unknown) {
           responseStatusCode = 500;
-          console.error(error);
         },
       }
     );
@@ -103,7 +102,6 @@ function handleBrowserRequest(
           reject(error);
         },
         onError(error: unknown) {
-          console.error(error);
           responseStatusCode = 500;
         },
       }

diff --git a/packages/remix-react/errorBoundaries.tsx b/packages/remix-react/errorBoundaries.tsx
@@ -70,7 +70,6 @@ export class RemixErrorBoundary extends React.Component<
  * When app's don't provide a root level ErrorBoundary, we default to this.
  */
 export function RemixRootDefaultErrorBoundary({ error }: { error: Error }) {
-  console.error(error);
   return (
     <html lang="en">
       <head>

diff --git a/packages/remix-server-runtime/server.ts b/packages/remix-server-runtime/server.ts
@@ -248,6 +248,9 @@ async function handleDocumentRequestRR(
 
   // Sanitize errors outside of development environments
   if (context.errors) {
+    Object.values(context.errors).forEach((err) =>
+      logServerErrorIfNotAborted(err, request, serverMode)
+    );
     context.errors = sanitizeErrors(context.errors, serverMode);
   }
 
@@ -284,6 +287,8 @@ async function handleDocumentRequestRR(
       loadContext
     );
   } catch (error: unknown) {
+    logServerErrorIfNotAborted(error, request, serverMode);
+
     // Get a new StaticHandlerContext that contains the error at the right boundary
     context = getStaticContextFromError(
       staticHandler.dataRoutes,