feat: 405: Method not allowed (#2366)

There is an HTTP code 405 for routes that exist, but require different
method.
https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/405

Unfortunately, `req.route` is `undefined` when within middlewares, so
have to use `app.all()` as recommended.

CHANGELOG.md

@@ -2,6 +2,20 @@
 
 ## Version 22
 
+### v22.5.0
+
+- Feature: Ability to respond with status code `405` (Method not allowed) to requests having wrong method:
+  - Previously, in all cases where the method and route combination was not defined, the response had status code `404`;
+  - For situations where a known route does not support the method being used, there is a more appropriate code `405`:
+    - See https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/405 for details;
+  - You can activate this feature by setting the new `wrongMethodBehavior` config option `405` (default: `404`).
+
+```ts
+import { createConfig } from "express-zod-api";
+
+createConfig({ wrongMethodBehavior: 405 });
+```
+
 ### v22.4.2
 
 - Excluded 41 response-only headers from the list of well-known ones used to depict request params in Documentation.

README.md

@@ -905,7 +905,8 @@ origins of errors that could happen in runtime and be handled the following way:
     - Others, inheriting from `Error` class (`500`);
 - Ones related to routing, parsing and upload issues — handled by `ResultHandler` assigned to `errorHandler` in config:
   - Default is `defaultResultHandler` — it sets the response status code from the corresponding `HttpError`:
-    `400` for parsing, `404` for routing, `config.upload.limitError.statusCode` for upload issues, or `500` for others.
+    `400` for parsing, `404` for routing, `config.upload.limitError.statusCode` for upload issues, or `500` for others;
+  - You can also set `wrongMethodBehavior` config option to `405` (Method not allowed) for requests having wrong method;
   - `ResultHandler` must handle possible `error` and avoid throwing its own errors, otherwise:
 - Ones related to `ResultHandler` execution — handled by `LastResortHandler`:
   - Response status code is always `500` and the response itself is a plain text.

src/config-type.ts

@@ -37,6 +37,14 @@ export interface CommonConfig {
    * @desc You can override the default CORS headers by setting up a provider function here.
    */
   cors: boolean | HeadersProvider;
+  /**
+   * @desc How to respond to a request that uses a wrong method to an existing endpoint
+   * @example 404 — Not found
+   * @example 405 — Method not allowed, incl. the "Allowed" header with a list of methods
+   * @default 404
+   * @todo consider changing default to 405 in v23
+   * */
+  wrongMethodBehavior?: 404 | 405;
   /**
    * @desc The ResultHandler to use for handling routing, parsing and upload errors
    * @default defaultResultHandler

src/result-handler.ts

@@ -116,10 +116,13 @@ export const defaultResultHandler = new ResultHandler({
     if (error) {
       const httpError = ensureHttpError(error);
       logServerError(httpError, logger, request, input);
-      return void response.status(httpError.statusCode).json({
-        status: "error",
-        error: { message: getPublicErrorMessage(httpError) },
-      });
+      return void response
+        .status(httpError.statusCode)
+        .set(httpError.headers)
+        .json({
+          status: "error",
+          error: { message: getPublicErrorMessage(httpError) },
+        });
     }
     response
       .status(defaultStatusCodes.positive)

src/routing.ts

@@ -1,4 +1,5 @@
 import { IRouter, RequestHandler } from "express";
+import createHttpError from "http-errors";
 import { isProduction } from "./common-helpers";
 import { CommonConfig } from "./config-type";
 import { ContentType } from "./content-type";
@@ -16,6 +17,18 @@ export interface Routing {
 
 export type Parsers = Partial<Record<ContentType, RequestHandler[]>>;
 
+/** @link https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/405 */
+export const createWrongMethodHandler =
+  (allowedMethods: Array<Method | AuxMethod>): RequestHandler =>
+  ({ method }, res, next) => {
+    const Allow = allowedMethods.join(", ").toUpperCase();
+    res.set({ Allow }); // in case of a custom errorHandler configured that does not care about headers in error
+    const error = createHttpError(405, `${method} is not allowed`, {
+      headers: { Allow },
+    });
+    next(error);
+  };
+
 export const initRouting = ({
   app,
   getLogger,
@@ -30,7 +43,7 @@ export const initRouting = ({
   parsers?: Parsers;
 }) => {
   const doc = new Diagnostics(getLogger());
-  const corsedPaths = new Set<string>();
+  const familiar = new Map<string, Array<Method | AuxMethod>>();
   const onEndpoint: OnEndpoint = (endpoint, path, method, siblingMethods) => {
     if (!isProduction()) doc.check(endpoint, { path, method });
     const matchingParsers = parsers?.[endpoint.getRequestType()] || [];
@@ -56,11 +69,18 @@ export const initRouting = ({
       }
       return endpoint.execute({ request, response, logger, config });
     };
-    if (config.cors && !corsedPaths.has(path)) {
-      app.options(path, ...matchingParsers, handler);
-      corsedPaths.add(path);
+    if (!familiar.has(path)) {
+      familiar.set(path, []);
+      if (config.cors) {
+        app.options(path, ...matchingParsers, handler);
+        familiar.get(path)?.push("options");
+      }
     }
+    familiar.get(path)?.push(method);
     app[method](path, ...matchingParsers, handler);
   };
   walkRouting({ routing, onEndpoint, onStatic: app.use.bind(app) });
+  if (config.wrongMethodBehavior !== 405) return;
+  for (const [path, allowedMethods] of familiar.entries())
+    app.all(path, createWrongMethodHandler(allowedMethods));
 };

src/server-helpers.ts

@@ -27,7 +27,7 @@ interface HandlerCreatorParams {
   getLogger: GetLogger;
 }
 
-export const createParserFailureHandler =
+export const createCatcher =
   ({ errorHandler, getLogger }: HandlerCreatorParams): ErrorRequestHandler =>
   async (error, request, response, next) => {
     if (!error) return next();

src/server.ts

@@ -16,7 +16,7 @@ import { Parsers, Routing, initRouting } from "./routing";
 import {
   createLoggingMiddleware,
   createNotFoundHandler,
-  createParserFailureHandler,
+  createCatcher,
   createUploadParsers,
   makeGetLogger,
   installDeprecationListener,
@@ -40,12 +40,12 @@ const makeCommonEntities = (config: CommonConfig) => {
   const getLogger = makeGetLogger(logger);
   const commons = { getLogger, errorHandler };
   const notFoundHandler = createNotFoundHandler(commons);
-  const parserFailureHandler = createParserFailureHandler(commons);
+  const catcher = createCatcher(commons);
   return {
     ...commons,
     logger,
     notFoundHandler,
-    parserFailureHandler,
+    catcher,
     loggingMiddleware,
   };
 };
@@ -63,13 +63,8 @@ export const attachRouting = (config: AppConfig, routing: Routing) => {
 };
 
 export const createServer = async (config: ServerConfig, routing: Routing) => {
-  const {
-    logger,
-    getLogger,
-    notFoundHandler,
-    parserFailureHandler,
-    loggingMiddleware,
-  } = makeCommonEntities(config);
+  const { logger, getLogger, notFoundHandler, catcher, loggingMiddleware } =
+    makeCommonEntities(config);
   const app = express().disable("x-powered-by").use(loggingMiddleware);
 
   if (config.compression) {
@@ -91,7 +86,7 @@ export const createServer = async (config: ServerConfig, routing: Routing) => {
 
   await config.beforeRouting?.({ app, getLogger });
   initRouting({ app, routing, getLogger, config, parsers });
-  app.use(parserFailureHandler, notFoundHandler);
+  app.use(catcher, notFoundHandler);
 
   const created: Array<http.Server | https.Server> = [];
   const makeStarter =

tests/express-mock.ts

@@ -19,6 +19,7 @@ const appMock = {
   delete: vi.fn(),
   options: vi.fn(),
   init: vi.fn(),
+  all: vi.fn(),
 };
 
 const expressMock = () => appMock;

tests/system/__snapshots__/system.spec.ts.snap

@@ -60,7 +60,16 @@ exports[`App in production mode > Positive > Should compress the response in cas
 exports[`App in production mode > Protocol > Should fail on invalid method 1`] = `
 {
   "error": {
-    "message": "Can not PUT /v1/test",
+    "message": "PUT is not allowed",
+  },
+  "status": "error",
+}
+`;
+
+exports[`App in production mode > Protocol > Should fail on invalid path 1`] = `
+{
+  "error": {
+    "message": "Can not GET /v1/wrong",
   },
   "status": "error",
 }

tests/system/system.spec.ts

@@ -112,6 +112,7 @@ describe("App in production mode", async () => {
   const config = createConfig({
     http: { listen: port },
     compression: { threshold: 1 },
+    wrongMethodBehavior: 405,
     beforeRouting: ({ app, getLogger }) => {
       depd("express")("Sample deprecation message");
       app.use((req, {}, next) => {
@@ -300,6 +301,13 @@ describe("App in production mode", async () => {
   });
 
   describe("Protocol", () => {
+    test("Should fail on invalid path", async () => {
+      const response = await fetch(`http://127.0.0.1:${port}/v1/wrong`);
+      expect(response.status).toBe(404);
+      const json = await response.json();
+      expect(json).toMatchSnapshot();
+    });
+
     test("Should fail on invalid method", async () => {
       const response = await fetch(`http://127.0.0.1:${port}/v1/test`, {
         method: "PUT",
@@ -311,7 +319,8 @@ describe("App in production mode", async () => {
           something: "joke",
         }),
       });
-      expect(response.status).toBe(404);
+      expect(response.status).toBe(405);
+      expect(response.headers.get("Allow")).toBe("GET, POST");
       const json = await response.json();
       expect(json).toMatchSnapshot();
     });

tests/unit/routing.spec.ts

@@ -19,8 +19,9 @@ import {
   makeRequestMock,
   makeResponseMock,
 } from "../../src/testing";
-import { initRouting } from "../../src/routing";
+import { createWrongMethodHandler, initRouting } from "../../src/routing";
 import type { IRouter, RequestHandler } from "express";
+import createHttpError from "http-errors";
 
 describe("Routing", () => {
   describe("initRouting()", () => {
@@ -32,57 +33,66 @@ describe("Routing", () => {
       vi.clearAllMocks(); // resets call counters on mocked methods
     });
 
-    test("Should set right methods", () => {
-      const handlerMock = vi.fn();
-      const configMock = {
-        cors: true,
-        startupLogo: false,
-      };
-      const factory = new EndpointsFactory(defaultResultHandler);
-      const getEndpoint = factory.build({
-        output: z.object({}),
-        handler: handlerMock,
-      });
-      const postEndpoint = factory.build({
-        method: "post",
-        output: z.object({}),
-        handler: handlerMock,
-      });
-      const getAndPostEndpoint = factory.build({
-        method: ["get", "post"],
-        output: z.object({}),
-        handler: handlerMock,
-      });
-      const routing: Routing = {
-        v1: {
-          user: {
-            get: getEndpoint,
-            set: postEndpoint,
-            universal: getAndPostEndpoint,
+    test.each([404, 405])(
+      "Should set right methods %#",
+      (wrongMethodBehavior) => {
+        const handlerMock = vi.fn();
+        const configMock = {
+          cors: true,
+          startupLogo: false,
+          wrongMethodBehavior,
+        };
+        const factory = new EndpointsFactory(defaultResultHandler);
+        const getEndpoint = factory.build({
+          output: z.object({}),
+          handler: handlerMock,
+        });
+        const postEndpoint = factory.build({
+          method: "post",
+          output: z.object({}),
+          handler: handlerMock,
+        });
+        const getAndPostEndpoint = factory.build({
+          method: ["get", "post"],
+          output: z.object({}),
+          handler: handlerMock,
+        });
+        const routing: Routing = {
+          v1: {
+            user: {
+              get: getEndpoint,
+              set: postEndpoint,
+              universal: getAndPostEndpoint,
+            },
           },
-        },
-      };
-      const logger = makeLoggerMock();
-      initRouting({
-        app: appMock as unknown as IRouter,
-        getLogger: () => logger,
-        config: configMock as CommonConfig,
-        routing,
-      });
-      expect(appMock.get).toHaveBeenCalledTimes(2);
-      expect(appMock.post).toHaveBeenCalledTimes(2);
-      expect(appMock.put).toHaveBeenCalledTimes(0);
-      expect(appMock.delete).toHaveBeenCalledTimes(0);
-      expect(appMock.patch).toHaveBeenCalledTimes(0);
-      expect(appMock.options).toHaveBeenCalledTimes(3);
-      expect(appMock.get.mock.calls[0][0]).toBe("/v1/user/get");
-      expect(appMock.get.mock.calls[1][0]).toBe("/v1/user/universal");
-      expect(appMock.post.mock.calls[0][0]).toBe("/v1/user/set");
-      expect(appMock.post.mock.calls[1][0]).toBe("/v1/user/universal");
-      expect(appMock.options.mock.calls[0][0]).toBe("/v1/user/get");
-      expect(appMock.options.mock.calls[1][0]).toBe("/v1/user/set");
-      expect(appMock.options.mock.calls[2][0]).toBe("/v1/user/universal");
-    });
+        };
+        const logger = makeLoggerMock();
+        initRouting({
+          app: appMock as unknown as IRouter,
+          getLogger: () => logger,
+          config: configMock as CommonConfig,
+          routing,
+        });
+        expect(appMock.get).toHaveBeenCalledTimes(2);
+        expect(appMock.post).toHaveBeenCalledTimes(2);
+        expect(appMock.put).toHaveBeenCalledTimes(0);
+        expect(appMock.delete).toHaveBeenCalledTimes(0);
+        expect(appMock.patch).toHaveBeenCalledTimes(0);
+        expect(appMock.options).toHaveBeenCalledTimes(3);
+        expect(appMock.get.mock.calls[0][0]).toBe("/v1/user/get");
+        expect(appMock.get.mock.calls[1][0]).toBe("/v1/user/universal");
+        expect(appMock.post.mock.calls[0][0]).toBe("/v1/user/set");
+        expect(appMock.post.mock.calls[1][0]).toBe("/v1/user/universal");
+        expect(appMock.options.mock.calls[0][0]).toBe("/v1/user/get");
+        expect(appMock.options.mock.calls[1][0]).toBe("/v1/user/set");
+        expect(appMock.options.mock.calls[2][0]).toBe("/v1/user/universal");
+        if (wrongMethodBehavior !== 405) return;
+        expect(appMock.all).toHaveBeenCalledTimes(3);
+        expect(appMock.all.mock.calls[0][0]).toBe("/v1/user/get");
+        expect(appMock.all.mock.calls[1][0]).toBe("/v1/user/set");
+        expect(appMock.all.mock.calls[2][0]).toBe("/v1/user/universal");
+      },
+    );
 
     test("Should accept serveStatic", () => {
       const routing: Routing = {
@@ -418,4 +428,19 @@ describe("Routing", () => {
       ]);
     });
   });
+
+  describe("createWrongMethodHandler", () => {
+    test("should call forward 405 error with a header having list of allowed methods", () => {
+      const handler = createWrongMethodHandler(["post", "options"]);
+      const nextMock = vi.fn();
+      const resMock = makeResponseMock();
+      handler(makeRequestMock(), resMock, nextMock);
+      expect(resMock._getHeaders()).toHaveProperty("allow", "POST, OPTIONS");
+      expect(nextMock).toHaveBeenCalledWith(
+        createHttpError(405, "GET is not allowed", {
+          headers: { Allow: "POST, OPTIONS" },
+        }),
+      );
+    });
+  });
 });