From 8d626124793faaf25d7b541f2d9c7c21b5581b05 Mon Sep 17 00:00:00 2001 From: Justin Spahr-Summers Date: Tue, 18 Feb 2025 13:13:05 +0000 Subject: [PATCH] Extract middleware for 405 Method Not Allowed --- src/server/auth/handlers/authorize.ts | 13 ++-- src/server/auth/handlers/metadata.test.ts | 7 +- src/server/auth/handlers/metadata.ts | 2 + src/server/auth/handlers/register.test.ts | 27 ++++--- src/server/auth/handlers/register.ts | 17 +++-- src/server/auth/handlers/revoke.test.ts | 7 +- src/server/auth/handlers/revoke.ts | 7 +- src/server/auth/handlers/token.test.ts | 13 +++- src/server/auth/handlers/token.ts | 7 +- .../auth/middleware/allowedMethods.test.ts | 75 +++++++++++++++++++ src/server/auth/middleware/allowedMethods.ts | 23 ++++++ 11 files changed, 162 insertions(+), 36 deletions(-) create mode 100644 src/server/auth/middleware/allowedMethods.test.ts create mode 100644 src/server/auth/middleware/allowedMethods.ts diff --git a/src/server/auth/handlers/authorize.ts b/src/server/auth/handlers/authorize.ts index 5dfd4443..a4056ab2 100644 --- a/src/server/auth/handlers/authorize.ts +++ b/src/server/auth/handlers/authorize.ts @@ -3,6 +3,7 @@ import { z } from "zod"; import express from "express"; import { OAuthServerProvider } from "../provider.js"; import { rateLimit, Options as RateLimitOptions } from "express-rate-limit"; +import { allowedMethods } from "../middleware/allowedMethods.js"; export type AuthorizationHandlerOptions = { provider: OAuthServerProvider; @@ -31,7 +32,8 @@ const RequestAuthorizationParamsSchema = z.object({ export function authorizationHandler({ provider, rateLimit: rateLimitConfig }: AuthorizationHandlerOptions): RequestHandler { // Create a router to apply middleware const router = express.Router(); - + router.use(allowedMethods(["GET", "POST"])); + // Apply rate limiting unless explicitly disabled if (rateLimitConfig !== false) { router.use(rateLimit({ @@ -46,14 +48,9 @@ export function authorizationHandler({ provider, rateLimit: rateLimitConfig }: A ...rateLimitConfig })); } - + // Define the handler router.all("/", async (req, res) => { - if (req.method !== "GET" && req.method !== "POST") { - res.status(405).end("Method Not Allowed"); - return; - } - let client_id, redirect_uri; try { ({ client_id, redirect_uri } = ClientAuthorizationParamsSchema.parse(req.query)); @@ -115,6 +112,6 @@ export function authorizationHandler({ provider, rateLimit: rateLimitConfig }: A codeChallenge: params.code_challenge, }, res); }); - + return router; } \ No newline at end of file diff --git a/src/server/auth/handlers/metadata.test.ts b/src/server/auth/handlers/metadata.test.ts index 73a9075d..9f70b965 100644 --- a/src/server/auth/handlers/metadata.test.ts +++ b/src/server/auth/handlers/metadata.test.ts @@ -30,7 +30,12 @@ describe('Metadata Handler', () => { .post('/.well-known/oauth-authorization-server') .send({}); - expect(response.status).toBe(404); // 404 since router only handles GET + expect(response.status).toBe(405); + expect(response.headers.allow).toBe('GET'); + expect(response.body).toEqual({ + error: "method_not_allowed", + error_description: "The method POST is not allowed for this endpoint" + }); }); it('returns the metadata object', async () => { diff --git a/src/server/auth/handlers/metadata.ts b/src/server/auth/handlers/metadata.ts index a3f9a518..048a4d4a 100644 --- a/src/server/auth/handlers/metadata.ts +++ b/src/server/auth/handlers/metadata.ts @@ -1,6 +1,7 @@ import express, { RequestHandler } from "express"; import { OAuthMetadata } from "../../../shared/auth.js"; import cors from 'cors'; +import { allowedMethods } from "../middleware/allowedMethods.js"; export function metadataHandler(metadata: OAuthMetadata): RequestHandler { // Nested router so we can configure middleware and restrict HTTP method @@ -9,6 +10,7 @@ export function metadataHandler(metadata: OAuthMetadata): RequestHandler { // Configure CORS to allow any origin, to make accessible to web-based MCP clients router.use(cors()); + router.use(allowedMethods(['GET'])); router.get("/", (req, res) => { res.status(200).json(metadata); }); diff --git a/src/server/auth/handlers/register.test.ts b/src/server/auth/handlers/register.test.ts index 719075f8..5faf1a4a 100644 --- a/src/server/auth/handlers/register.test.ts +++ b/src/server/auth/handlers/register.test.ts @@ -10,7 +10,7 @@ describe('Client Registration Handler', () => { async getClient(_clientId: string): Promise { return undefined; }, - + async registerClient(client: OAuthClientInformationFull): Promise { // Return the client info as-is in the mock return client; @@ -30,7 +30,7 @@ describe('Client Registration Handler', () => { const options: ClientRegistrationHandlerOptions = { clientsStore: mockClientStoreWithoutRegistration }; - + expect(() => clientRegistrationHandler(options)).toThrow('does not support registering clients'); }); @@ -38,7 +38,7 @@ describe('Client Registration Handler', () => { const options: ClientRegistrationHandlerOptions = { clientsStore: mockClientStoreWithRegistration }; - + expect(() => clientRegistrationHandler(options)).not.toThrow(); }); }); @@ -54,7 +54,7 @@ describe('Client Registration Handler', () => { clientsStore: mockClientStoreWithRegistration, clientSecretExpirySeconds: 86400 // 1 day for testing }; - + app.use('/register', clientRegistrationHandler(options)); // Spy on the registerClient method @@ -72,7 +72,12 @@ describe('Client Registration Handler', () => { redirect_uris: ['https://example.com/callback'] }); - expect(response.status).toBe(404); // 404 since router only handles POST + expect(response.status).toBe(405); + expect(response.headers.allow).toBe('POST'); + expect(response.body).toEqual({ + error: "method_not_allowed", + error_description: "The method GET is not allowed for this endpoint" + }); expect(spyRegisterClient).not.toHaveBeenCalled(); }); @@ -112,14 +117,14 @@ describe('Client Registration Handler', () => { .send(clientMetadata); expect(response.status).toBe(201); - + // Verify the generated client information expect(response.body.client_id).toBeDefined(); expect(response.body.client_secret).toBeDefined(); expect(response.body.client_id_issued_at).toBeDefined(); expect(response.body.client_secret_expires_at).toBeDefined(); expect(response.body.redirect_uris).toEqual(['https://example.com/callback']); - + // Verify client was registered expect(spyRegisterClient).toHaveBeenCalledTimes(1); }); @@ -145,7 +150,7 @@ describe('Client Registration Handler', () => { clientsStore: mockClientStoreWithRegistration, clientSecretExpirySeconds: 3600 // 1 hour }; - + customApp.use('/register', clientRegistrationHandler(options)); const response = await supertest(customApp) @@ -155,7 +160,7 @@ describe('Client Registration Handler', () => { }); expect(response.status).toBe(201); - + // Verify the expiration time (~1 hour from now) const issuedAt = response.body.client_id_issued_at; const expiresAt = response.body.client_secret_expires_at; @@ -169,7 +174,7 @@ describe('Client Registration Handler', () => { clientsStore: mockClientStoreWithRegistration, clientSecretExpirySeconds: 0 // No expiry }; - + customApp.use('/register', clientRegistrationHandler(options)); const response = await supertest(customApp) @@ -205,7 +210,7 @@ describe('Client Registration Handler', () => { .send(fullClientMetadata); expect(response.status).toBe(201); - + // Verify all metadata was preserved Object.entries(fullClientMetadata).forEach(([key, value]) => { expect(response.body[key]).toEqual(value); diff --git a/src/server/auth/handlers/register.ts b/src/server/auth/handlers/register.ts index a2c0f19a..63916f33 100644 --- a/src/server/auth/handlers/register.ts +++ b/src/server/auth/handlers/register.ts @@ -4,6 +4,7 @@ import crypto from 'node:crypto'; import cors from 'cors'; import { OAuthRegisteredClientsStore } from "../clients.js"; import { rateLimit, Options as RateLimitOptions } from "express-rate-limit"; +import { allowedMethods } from "../middleware/allowedMethods.js"; export type ClientRegistrationHandlerOptions = { /** @@ -17,7 +18,7 @@ export type ClientRegistrationHandlerOptions = { * If not set, defaults to 30 days. */ clientSecretExpirySeconds?: number; - + /** * Rate limiting configuration for the client registration endpoint. * Set to false to disable rate limiting for this endpoint. @@ -28,10 +29,10 @@ export type ClientRegistrationHandlerOptions = { const DEFAULT_CLIENT_SECRET_EXPIRY_SECONDS = 30 * 24 * 60 * 60; // 30 days -export function clientRegistrationHandler({ - clientsStore, - clientSecretExpirySeconds = DEFAULT_CLIENT_SECRET_EXPIRY_SECONDS, - rateLimit: rateLimitConfig +export function clientRegistrationHandler({ + clientsStore, + clientSecretExpirySeconds = DEFAULT_CLIENT_SECRET_EXPIRY_SECONDS, + rateLimit: rateLimitConfig }: ClientRegistrationHandlerOptions): RequestHandler { if (!clientsStore.registerClient) { throw new Error("Client registration store does not support registering clients"); @@ -39,11 +40,13 @@ export function clientRegistrationHandler({ // Nested router so we can configure middleware and restrict HTTP method const router = express.Router(); - router.use(express.json()); // Configure CORS to allow any origin, to make accessible to web-based MCP clients router.use(cors()); - + + router.use(allowedMethods(["POST"])); + router.use(express.json()); + // Apply rate limiting unless explicitly disabled - stricter limits for registration if (rateLimitConfig !== false) { router.use(rateLimit({ diff --git a/src/server/auth/handlers/revoke.test.ts b/src/server/auth/handlers/revoke.test.ts index 2fe5e9c5..ddda759b 100644 --- a/src/server/auth/handlers/revoke.test.ts +++ b/src/server/auth/handlers/revoke.test.ts @@ -129,7 +129,12 @@ describe('Revocation Handler', () => { token: 'token_to_revoke' }); - expect(response.status).toBe(400); // Handler actually responds with 400 for any invalid request + expect(response.status).toBe(405); + expect(response.headers.allow).toBe('POST'); + expect(response.body).toEqual({ + error: "method_not_allowed", + error_description: "The method GET is not allowed for this endpoint" + }); expect(spyRevokeToken).not.toHaveBeenCalled(); }); diff --git a/src/server/auth/handlers/revoke.ts b/src/server/auth/handlers/revoke.ts index 03f3389c..48e1d91b 100644 --- a/src/server/auth/handlers/revoke.ts +++ b/src/server/auth/handlers/revoke.ts @@ -4,6 +4,7 @@ import cors from "cors"; import { authenticateClient } from "../middleware/clientAuth.js"; import { OAuthTokenRevocationRequestSchema } from "../../../shared/auth.js"; import { rateLimit, Options as RateLimitOptions } from "express-rate-limit"; +import { allowedMethods } from "../middleware/allowedMethods.js"; export type RevocationHandlerOptions = { provider: OAuthServerProvider; @@ -21,11 +22,13 @@ export function revocationHandler({ provider, rateLimit: rateLimitConfig }: Revo // Nested router so we can configure middleware and restrict HTTP method const router = express.Router(); - router.use(express.urlencoded({ extended: false })); // Configure CORS to allow any origin, to make accessible to web-based MCP clients router.use(cors()); - + + router.use(allowedMethods(["POST"])); + router.use(express.urlencoded({ extended: false })); + // Apply rate limiting unless explicitly disabled if (rateLimitConfig !== false) { router.use(rateLimit({ diff --git a/src/server/auth/handlers/token.test.ts b/src/server/auth/handlers/token.test.ts index be2a61df..f30fc83d 100644 --- a/src/server/auth/handlers/token.test.ts +++ b/src/server/auth/handlers/token.test.ts @@ -71,11 +71,11 @@ describe('Token Handler', () => { expires_in: 3600, refresh_token: 'new_mock_refresh_token' }; - + if (scopes) { response.scope = scopes.join(' '); } - + return response; } throw new Error('invalid_grant'); @@ -109,7 +109,12 @@ describe('Token Handler', () => { grant_type: 'authorization_code' }); - expect(response.status).toBe(400); // Handler responds with 400 for invalid requests + expect(response.status).toBe(405); + expect(response.headers.allow).toBe('POST'); + expect(response.body).toEqual({ + error: "method_not_allowed", + error_description: "The method GET is not allowed for this endpoint" + }); }); it('requires grant_type parameter', async () => { @@ -208,7 +213,7 @@ describe('Token Handler', () => { it('verifies code_verifier against challenge', async () => { // Setup invalid verifier (pkceChallenge.verifyChallenge as jest.Mock).mockReturnValueOnce(false); - + const response = await supertest(app) .post('/token') .type('form') diff --git a/src/server/auth/handlers/token.ts b/src/server/auth/handlers/token.ts index c49dfe7e..9cde48de 100644 --- a/src/server/auth/handlers/token.ts +++ b/src/server/auth/handlers/token.ts @@ -5,6 +5,7 @@ import cors from "cors"; import { verifyChallenge } from "pkce-challenge"; import { authenticateClient } from "../middleware/clientAuth.js"; import { rateLimit, Options as RateLimitOptions } from "express-rate-limit"; +import { allowedMethods } from "../middleware/allowedMethods.js"; export type TokenHandlerOptions = { provider: OAuthServerProvider; @@ -32,11 +33,13 @@ const RefreshTokenGrantSchema = z.object({ export function tokenHandler({ provider, rateLimit: rateLimitConfig }: TokenHandlerOptions): RequestHandler { // Nested router so we can configure middleware and restrict HTTP method const router = express.Router(); - router.use(express.urlencoded({ extended: false })); // Configure CORS to allow any origin, to make accessible to web-based MCP clients router.use(cors()); - + + router.use(allowedMethods(["POST"])); + router.use(express.urlencoded({ extended: false })); + // Apply rate limiting unless explicitly disabled if (rateLimitConfig !== false) { router.use(rateLimit({ diff --git a/src/server/auth/middleware/allowedMethods.test.ts b/src/server/auth/middleware/allowedMethods.test.ts new file mode 100644 index 00000000..61f8c801 --- /dev/null +++ b/src/server/auth/middleware/allowedMethods.test.ts @@ -0,0 +1,75 @@ +import { allowedMethods } from "./allowedMethods.js"; +import express, { Request, Response } from "express"; +import request from "supertest"; + +describe("allowedMethods", () => { + let app: express.Express; + + beforeEach(() => { + app = express(); + + // Set up a test router with a GET handler and 405 middleware + const router = express.Router(); + + router.get("/test", (req, res) => { + res.status(200).send("GET success"); + }); + + // Add method not allowed middleware for all other methods + router.all("/test", allowedMethods(["GET"])); + + app.use(router); + }); + + test("allows specified HTTP method", async () => { + const response = await request(app).get("/test"); + expect(response.status).toBe(200); + expect(response.text).toBe("GET success"); + }); + + test("returns 405 for unspecified HTTP methods", async () => { + const methods = ["post", "put", "delete", "patch"]; + + for (const method of methods) { + // @ts-expect-error - dynamic method call + const response = await request(app)[method]("/test"); + expect(response.status).toBe(405); + expect(response.body).toEqual({ + error: "method_not_allowed", + error_description: `The method ${method.toUpperCase()} is not allowed for this endpoint` + }); + } + }); + + test("includes Allow header with specified methods", async () => { + const response = await request(app).post("/test"); + expect(response.headers.allow).toBe("GET"); + }); + + test("works with multiple allowed methods", async () => { + const multiMethodApp = express(); + const router = express.Router(); + + router.get("/multi", (req: Request, res: Response) => { + res.status(200).send("GET"); + }); + router.post("/multi", (req: Request, res: Response) => { + res.status(200).send("POST"); + }); + router.all("/multi", allowedMethods(["GET", "POST"])); + + multiMethodApp.use(router); + + // Allowed methods should work + const getResponse = await request(multiMethodApp).get("/multi"); + expect(getResponse.status).toBe(200); + + const postResponse = await request(multiMethodApp).post("/multi"); + expect(postResponse.status).toBe(200); + + // Unallowed methods should return 405 + const putResponse = await request(multiMethodApp).put("/multi"); + expect(putResponse.status).toBe(405); + expect(putResponse.headers.allow).toBe("GET, POST"); + }); +}); \ No newline at end of file diff --git a/src/server/auth/middleware/allowedMethods.ts b/src/server/auth/middleware/allowedMethods.ts new file mode 100644 index 00000000..5ffdecd1 --- /dev/null +++ b/src/server/auth/middleware/allowedMethods.ts @@ -0,0 +1,23 @@ +import { RequestHandler } from "express"; + +/** + * Middleware to handle unsupported HTTP methods with a 405 Method Not Allowed response. + * + * @param allowedMethods Array of allowed HTTP methods for this endpoint (e.g., ['GET', 'POST']) + * @returns Express middleware that returns a 405 error if method not in allowed list + */ +export function allowedMethods(allowedMethods: string[]): RequestHandler { + return (req, res, next) => { + if (allowedMethods.includes(req.method)) { + next(); + return; + } + + res.status(405) + .set('Allow', allowedMethods.join(', ')) + .json({ + error: "method_not_allowed", + error_description: `The method ${req.method} is not allowed for this endpoint` + }); + }; +} \ No newline at end of file