fix: update HTTP error response format to be spec compliant (#7110)

* fix: update HTTP error response format to be spec compliant

* Fix error assertions

* Remove group tag from keymanager option

* Fix body has already been read

* Assert deepEqual

packages/beacon-node/src/api/rest/base.ts

@@ -15,6 +15,7 @@ export type RestApiServerOpts = {
   bearerToken?: string;
   headerLimit?: number;
   bodyLimit?: number;
+  stacktraces?: boolean;
   swaggerUI?: boolean;
 };
 
@@ -29,11 +30,27 @@ export type RestApiServerMetrics = SocketMetrics & {
   errors: Gauge<{operationId: string}>;
 };
 
+/**
+ * Error response body format as defined in beacon-api spec
+ *
+ * See https://github.com/ethereum/beacon-APIs/blob/v2.5.0/types/http.yaml
+ */
+type ErrorResponse = {
+  code: number;
+  message: string;
+  stacktraces?: string[];
+};
+
 /**
  * Error code used by Fastify if media type is not supported (415)
  */
 const INVALID_MEDIA_TYPE_CODE = errorCodes.FST_ERR_CTP_INVALID_MEDIA_TYPE().code;
 
+/**
+ * Error code used by Fastify if JSON schema validation failed
+ */
+const SCHEMA_VALIDATION_ERROR_CODE = errorCodes.FST_ERR_VALIDATION().code;
+
 /**
  * REST API powered by `fastify` server.
  */
@@ -71,15 +88,30 @@ export class RestApiServer {
 
     // To parse our ApiError -> statusCode
     server.setErrorHandler((err, _req, res) => {
+      const stacktraces = opts.stacktraces ? err.stack?.split("\n") : undefined;
       if (err.validation) {
-        void res.status(400).send(err.validation);
+        const {instancePath, message} = err.validation[0];
+        const payload: ErrorResponse = {
+          code: err.statusCode ?? 400,
+          message: `${instancePath.substring(instancePath.lastIndexOf("/") + 1)} ${message}`,
+          stacktraces,
+        };
+        void res.status(400).send(payload);
       } else {
         // Convert our custom ApiError into status code
         const statusCode = err instanceof ApiError ? err.statusCode : 500;
-        void res.status(statusCode).send(err);
+        const payload: ErrorResponse = {code: statusCode, message: err.message, stacktraces};
+        void res.status(statusCode).send(payload);
       }
     });
 
+    server.setNotFoundHandler((req, res) => {
+      const message = `Route ${req.raw.method}:${req.raw.url} not found`;
+      this.logger.warn(message);
+      const payload: ErrorResponse = {code: 404, message};
+      void res.code(404).send(payload);
+    });
+
     if (opts.cors) {
       void server.register(fastifyCors, {origin: opts.cors});
     }
@@ -127,7 +159,7 @@ export class RestApiServer {
 
       const operationId = getOperationId(req);
 
-      if (err instanceof ApiError || err.code === INVALID_MEDIA_TYPE_CODE) {
+      if (err instanceof ApiError || [INVALID_MEDIA_TYPE_CODE, SCHEMA_VALIDATION_ERROR_CODE].includes(err.code)) {
         this.logger.warn(`Req ${req.id} ${operationId} failed`, {reason: err.message});
       } else {
         this.logger.error(`Req ${req.id} ${operationId} error`, {}, err);

packages/beacon-node/src/api/rest/index.ts

@@ -22,6 +22,7 @@ export const beaconRestApiServerOpts: BeaconRestApiServerOpts = {
   cors: "*",
   // beacon -> validator API is trusted, and for large amounts of keys the payload is multi-MB
   bodyLimit: 20 * 1024 * 1024, // 20MB for big block + blobs
+  stacktraces: false,
 };
 
 export type BeaconRestApiServerModules = RestApiServerModules & {

packages/cli/src/cmds/dev/options.ts

@@ -90,6 +90,10 @@ const externalOptionsOverrides: Partial<Record<"network" | keyof typeof beaconNo
     defaultDescription: undefined,
     default: true,
   },
+  "rest.stacktraces": {
+    ...beaconNodeOptions["rest.stacktraces"],
+    default: true,
+  },
   "rest.swaggerUI": {
     ...beaconNodeOptions["rest.swaggerUI"],
     default: true,

packages/cli/src/cmds/validator/handler.ts

@@ -209,6 +209,7 @@ export async function validatorHandler(args: IValidatorCliArgs & GlobalArgs): Pr
         isAuthEnabled: args["keymanager.auth"],
         headerLimit: args["keymanager.headerLimit"],
         bodyLimit: args["keymanager.bodyLimit"],
+        stacktraces: args["keymanager.stacktraces"],
         tokenFile: args["keymanager.tokenFile"],
         tokenDir: dbPath,
       },

packages/cli/src/cmds/validator/keymanager/server.ts

@@ -21,6 +21,7 @@ export const keymanagerRestApiServerOptsDefault: KeymanagerRestApiServerOpts = {
   isAuthEnabled: true,
   // Slashing protection DB has been reported to be 3MB https://github.com/ChainSafe/lodestar/issues/4530
   bodyLimit: 20 * 1024 * 1024, // 20MB
+  stacktraces: false,
 };
 
 export type KeymanagerRestApiServerModules = RestApiServerModules & {

packages/cli/src/cmds/validator/options.ts

@@ -91,6 +91,7 @@ export type KeymanagerArgs = {
   "keymanager.cors"?: string;
   "keymanager.headerLimit"?: number;
   "keymanager.bodyLimit"?: number;
+  "keymanager.stacktraces"?: boolean;
 };
 
 export const keymanagerOptions: CliCommandOptions<KeymanagerArgs> = {
@@ -141,6 +142,11 @@ export const keymanagerOptions: CliCommandOptions<KeymanagerArgs> = {
     type: "number",
     description: "Defines the maximum payload, in bytes, the server is allowed to accept",
   },
+  "keymanager.stacktraces": {
+    hidden: true,
+    type: "boolean",
+    description: "Return stacktraces in HTTP error responses",
+  },
 };
 
 export const validatorOptions: CliCommandOptions<IValidatorCliArgs> = {

packages/cli/src/options/beaconNodeOptions/api.ts

@@ -12,6 +12,7 @@ export type ApiArgs = {
   "rest.port": number;
   "rest.headerLimit"?: number;
   "rest.bodyLimit"?: number;
+  "rest.stacktraces"?: boolean;
   "rest.swaggerUI"?: boolean;
 };
 
@@ -26,6 +27,7 @@ export function parseArgs(args: ApiArgs): IBeaconNodeOptions["api"] {
       port: args["rest.port"],
       headerLimit: args["rest.headerLimit"],
       bodyLimit: args["rest.bodyLimit"],
+      stacktraces: args["rest.stacktraces"],
       swaggerUI: args["rest.swaggerUI"],
     },
   };
@@ -92,6 +94,13 @@ export const options: CliCommandOptions<ApiArgs> = {
     description: "Defines the maximum payload, in bytes, the server is allowed to accept",
   },
 
+  "rest.stacktraces": {
+    hidden: true,
+    type: "boolean",
+    description: "Return stacktraces in HTTP error responses",
+    group: "api",
+  },
+
   "rest.swaggerUI": {
     type: "boolean",
     description: "Enable Swagger UI for API exploration at http://{address}:{port}/documentation",

packages/cli/test/sim/endpoints.test.ts

@@ -2,7 +2,7 @@
 import path from "node:path";
 import assert from "node:assert";
 import {toHexString} from "@chainsafe/ssz";
-import {routes} from "@lodestar/api";
+import {routes, fetch} from "@lodestar/api";
 import {Simulation} from "../utils/crucible/simulation.js";
 import {BeaconClient, ExecutionClient} from "../utils/crucible/interfaces.js";
 import {defineSimTestConfig, logFilesDir} from "../utils/crucible/utils/index.js";
@@ -105,6 +105,20 @@ await env.tracker.assert(
   }
 );
 
+await env.tracker.assert("should return HTTP error responses in a spec compliant format", async () => {
+  // ApiError with status 400 is thrown by handler
+  const res1 = await node.api.beacon.getStateValidator({stateId: "current", validatorId: 1});
+  assert.deepEqual(JSON.parse(await res1.errorBody()), {code: 400, message: "Invalid block id 'current'"});
+
+  // JSON schema validation failed
+  const res2 = await node.api.beacon.getPoolAttestationsV2({slot: "current" as unknown as number, committeeIndex: 123});
+  assert.deepEqual(JSON.parse(await res2.errorBody()), {code: 400, message: "slot must be integer"});
+
+  // Route does not exist
+  const res3 = await fetch(`${node.restPublicUrl}/not/implemented/route`);
+  assert.deepEqual(JSON.parse(await res3.text()), {code: 404, message: "Route GET:/not/implemented/route not found"});
+});
+
 await env.tracker.assert("BN Not Synced", async () => {
   const expectedSyncStatus: routes.node.SyncingStatus = {
     headSlot: 2,

packages/cli/test/unit/options/beaconNodeOptions.test.ts

@@ -17,6 +17,7 @@ describe("options / beaconNodeOptions", () => {
       "rest.port": 7654,
       "rest.headerLimit": 16384,
       "rest.bodyLimit": 30e6,
+      "rest.stacktraces": true,
 
       "chain.blsVerifyAllMultiThread": true,
       "chain.blsVerifyAllMainThread": true,
@@ -122,6 +123,7 @@ describe("options / beaconNodeOptions", () => {
           port: 7654,
           headerLimit: 16384,
           bodyLimit: 30e6,
+          stacktraces: true,
         },
       },
       chain: {