fix: Handle errors properly in asV2Middleware

Author: rekmarks

packages/eth-json-rpc-provider/CHANGELOG.md

@@ -15,8 +15,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - **BREAKING:** Use `JsonRpcServer` instead of `JsonRpcEngine` ([#7001](https://github.com/MetaMask/core/pull/7001))
   - Adds a new `server` constructor option to the `InternalProvider` class, mutually exclusive with the now deprecated `engine` option.
   - Legacy `JsonRpcEngine` instances are wrapped in a `JsonRpcServer` internally
-  wherever they appear. Due to differences in error serialization, this may be
-  breaking for consumers.
+    wherever they appear. Due to differences in error serialization, this may be
+    breaking for consumers.
 
 ## [5.0.1]
 

packages/eth-json-rpc-provider/src/internal-provider.ts

@@ -197,7 +197,8 @@ export function convertEip1193RequestToJsonRpcRequest<
 }
 
 /**
- * Deserialize a JSON-RPC error.
+ * Deserialize a JSON-RPC error. Ignores the possibility of `stack` property, since this is
+ * stripped by `JsonRpcServer`.
  *
  * @param error - The JSON-RPC error to deserialize.
  * @returns The deserialized error.

packages/json-rpc-engine/src/asV2Middleware.test.ts

@@ -171,6 +171,21 @@ describe('asV2Middleware', () => {
       );
     });
 
+    it('does not forward undefined errors from legacy middleware', async () => {
+      const legacyMiddleware = jest.fn((_req, res, _next, end) => {
+        res.error = undefined;
+        res.result = 42;
+        end();
+      });
+
+      const v2Engine = JsonRpcEngineV2.create({
+        middleware: [asV2Middleware(legacyMiddleware)],
+      });
+
+      const result = await v2Engine.handle(makeRequest());
+      expect(result).toBe(42);
+    });
+
     it('allows v2 engine to continue when legacy middleware does not end', async () => {
       const legacyMiddleware = jest.fn((_req, _res, next) => {
         next();

packages/json-rpc-engine/src/asV2Middleware.ts

@@ -19,7 +19,7 @@ import {
   fromLegacyRequest,
   propagateToContext,
   propagateToRequest,
-  unserializeError,
+  deserializeError,
 } from './v2/compatibility-utils';
 import type {
   // Used in docs.
@@ -104,8 +104,11 @@ export function asV2Middleware<
     });
     propagateToContext(req, context);
 
-    if (hasProperty(response, 'error')) {
-      throw unserializeError(response.error);
+    // Mimic the behavior of JsonRpcEngine.#handle(), which only treats truthy errors as errors.
+    // Legacy middleware may violate the invariant that response objects have either a result or an
+    // error property. In practice, we may see response objects with results and `{ error: undefined }`.
+    if (hasProperty(response, 'error') && response.error) {
+      throw deserializeError(response.error);
     } else if (hasProperty(response, 'result')) {
       return response.result as ResultConstraint<Request>;
     }

packages/json-rpc-engine/src/v2/compatibility-utils.test.ts

@@ -7,7 +7,7 @@ import {
   makeContext,
   propagateToContext,
   propagateToRequest,
-  unserializeError,
+  deserializeError,
 } from './compatibility-utils';
 import { MiddlewareContext } from './MiddlewareContext';
 import { stringify } from './utils';
@@ -348,7 +348,7 @@ describe('compatibility-utils', () => {
     });
   });
 
-  describe('unserializeError', () => {
+  describe('deserializeError', () => {
     // Requires some special handling due to the possible existence or
     // non-existence of Error.isError
     describe('Error.isError', () => {
@@ -382,22 +382,22 @@ describe('compatibility-utils', () => {
         isError.mockReturnValueOnce(true);
         const originalError = new Error('test error');
 
-        const result = unserializeError(originalError);
+        const result = deserializeError(originalError);
         expect(result).toBe(originalError);
       });
 
       it('returns the thrown value when it is instanceof Error', () => {
         isError.mockReturnValueOnce(false);
         const originalError = new Error('test error');
 
-        const result = unserializeError(originalError);
+        const result = deserializeError(originalError);
         expect(result).toBe(originalError);
       });
     });
 
     it('creates a new Error when thrown value is a string', () => {
       const errorMessage = 'test error message';
-      const result = unserializeError(errorMessage);
+      const result = deserializeError(errorMessage);
 
       expect(result).toBeInstanceOf(Error);
       expect(result.message).toBe(errorMessage);
@@ -406,7 +406,7 @@ describe('compatibility-utils', () => {
     it.each([42, true, false, null, undefined, Symbol('test')])(
       'creates a new Error with stringified message for non-object values',
       (value) => {
-        const result = unserializeError(value);
+        const result = deserializeError(value);
 
         expect(result).toBeInstanceOf(Error);
         expect(result.message).toBe(`Unknown error: ${stringify(value)}`);
@@ -421,7 +421,7 @@ describe('compatibility-utils', () => {
         data: { foo: 'bar' },
       };
 
-      const result = unserializeError(thrownValue);
+      const result = deserializeError(thrownValue);
 
       expect(result).toBeInstanceOf(JsonRpcError);
       expect(result).toMatchObject({
@@ -439,7 +439,7 @@ describe('compatibility-utils', () => {
         data: { foo: 'bar' },
       };
 
-      const result = unserializeError(thrownValue);
+      const result = deserializeError(thrownValue);
 
       expect(result).toBeInstanceOf(Error);
       expect(result).not.toBeInstanceOf(JsonRpcError);
@@ -455,7 +455,7 @@ describe('compatibility-utils', () => {
         message: 'test error message',
       };
 
-      const result = unserializeError(thrownValue);
+      const result = deserializeError(thrownValue);
 
       expect(result).toBeInstanceOf(Error);
       expect(result).not.toBeInstanceOf(JsonRpcError);
@@ -469,7 +469,7 @@ describe('compatibility-utils', () => {
         stack: stackTrace,
       };
 
-      const result = unserializeError(thrownValue);
+      const result = deserializeError(thrownValue);
 
       expect(result).toBeInstanceOf(Error);
       expect(result.stack).toBe(stackTrace);
@@ -485,7 +485,7 @@ describe('compatibility-utils', () => {
         data,
       };
 
-      const result = unserializeError(thrownValue) as JsonRpcError<Json>;
+      const result = deserializeError(thrownValue) as JsonRpcError<Json>;
 
       expect(result.cause).toBe(cause);
       expect(result.data).toStrictEqual({
@@ -499,7 +499,7 @@ describe('compatibility-utils', () => {
         code: 1234,
       };
 
-      const result = unserializeError(thrownValue);
+      const result = deserializeError(thrownValue);
 
       expect(result.message).toBe('Unknown error');
     });
@@ -510,7 +510,7 @@ describe('compatibility-utils', () => {
         message: 42,
       };
 
-      const result = unserializeError(thrownValue);
+      const result = deserializeError(thrownValue);
 
       expect(result.message).toBe('Unknown error');
     });
@@ -521,7 +521,7 @@ describe('compatibility-utils', () => {
         message: 42,
       };
 
-      const result = unserializeError(thrownValue);
+      const result = deserializeError(thrownValue);
 
       expect(result.message).toBe('Internal JSON-RPC error.');
     });

packages/json-rpc-engine/src/v2/compatibility-utils.ts

@@ -135,7 +135,7 @@ export function propagateToRequest(
  * @param thrown - The thrown value to unserialize.
  * @returns The unserialized error.
  */
-export function unserializeError(thrown: unknown): Error | JsonRpcError<Json> {
+export function deserializeError(thrown: unknown): Error | JsonRpcError<Json> {
   // @ts-expect-error - New, but preferred if available.
   // See: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/isError
   if (typeof Error.isError === 'function' && Error.isError(thrown)) {