From 962ef38dcdd83465c3d84b380981adeebac11c20 Mon Sep 17 00:00:00 2001
From: Rahul-s-007 <rahulrtg7703@gmail.com>
Date: Thu, 10 Sep 2026 20:10:15 +0400
Subject: [PATCH] fix(server-legacy): use constant-time compare for OAuth
 client_secret

authenticateClient compared client_secret with plain !==, which is a
timing side channel (CWE-208) on a value meant to be secret. Swap to
crypto.timingSafeEqual, matching the constant-time approach this repo
already uses for its own HMAC verification in requestStateCodec.ts.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
---
 .../src/auth/middleware/clientAuth.ts           | 17 ++++++++++++++++-
 .../test/auth/middleware/clientAuth.test.ts     | 14 ++++++++++++++
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/packages/server-legacy/src/auth/middleware/clientAuth.ts b/packages/server-legacy/src/auth/middleware/clientAuth.ts
index ad9d931253..b9c22d42bd 100644
--- a/packages/server-legacy/src/auth/middleware/clientAuth.ts
+++ b/packages/server-legacy/src/auth/middleware/clientAuth.ts
@@ -1,3 +1,5 @@
+import { timingSafeEqual } from 'node:crypto';
+
 import type { OAuthClientInformationFull } from '@modelcontextprotocol/core-internal';
 import type { RequestHandler } from 'express';
 import * as z from 'zod/v4';
@@ -26,6 +28,19 @@ declare module 'express-serve-static-core' {
     }
 }
 
+/**
+ * Constant-time string comparison, to avoid leaking `client_secret` via a
+ * timing side channel. `timingSafeEqual` requires equal-length buffers, so a
+ * length mismatch is handled as an immediate non-match — this still leaks
+ * length, not content, the same tradeoff every constant-time-compare helper
+ * makes.
+ */
+function secretsMatch(a: string, b: string): boolean {
+    const aBuf = Buffer.from(a);
+    const bBuf = Buffer.from(b);
+    return aBuf.length === bBuf.length && timingSafeEqual(aBuf, bBuf);
+}
+
 export function authenticateClient({ clientsStore }: ClientAuthenticationMiddlewareOptions): RequestHandler {
     return async (req, res, next) => {
         try {
@@ -42,7 +57,7 @@ export function authenticateClient({ clientsStore }: ClientAuthenticationMiddlew
                 if (!client_secret) {
                     throw new InvalidClientError('Client secret is required');
                 }
-                if (client.client_secret !== client_secret) {
+                if (!secretsMatch(client.client_secret, client_secret)) {
                     throw new InvalidClientError('Invalid client_secret');
                 }
                 if (client.client_secret_expires_at && client.client_secret_expires_at < Math.floor(Date.now() / 1000)) {
diff --git a/packages/server-legacy/test/auth/middleware/clientAuth.test.ts b/packages/server-legacy/test/auth/middleware/clientAuth.test.ts
index df21c5d205..71cb31be77 100644
--- a/packages/server-legacy/test/auth/middleware/clientAuth.test.ts
+++ b/packages/server-legacy/test/auth/middleware/clientAuth.test.ts
@@ -84,6 +84,20 @@ describe('clientAuth middleware', () => {
         expect(response.body.error_description).toBe('Invalid client_secret');
     });
 
+    it('rejects invalid client_secret of a different length than the real one', async () => {
+        // Exercises the constant-time comparison's length-mismatch path
+        // (`timingSafeEqual` throws on unequal-length buffers, so this must
+        // be handled explicitly rather than left to throw).
+        const response = await supertest(app).post('/protected').send({
+            client_id: 'valid-client',
+            client_secret: 'short'
+        });
+
+        expect(response.status).toBe(400);
+        expect(response.body.error).toBe('invalid_client');
+        expect(response.body.error_description).toBe('Invalid client_secret');
+    });
+
     it('rejects missing client_id', async () => {
         const response = await supertest(app).post('/protected').send({
             client_secret: 'valid-secret'