From 4debd6a4c1b7680fffc692553bd599eaea55b802 Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Tue, 7 Jul 2026 16:09:13 +0000 Subject: [PATCH 1/2] Fix unsafe timing-safe comparison with user input in MCP server When using crypto.timingSafeEqual, comparing the string lengths before buffer creation is unsafe because strings with multibyte characters can have the same length but different byte sizes. This causes timingSafeEqual to throw an error, which in this case might just get caught and ignored, but is bad practice and can result in unwanted behaviors or crashes. This fix correctly converts the strings into Buffers first, and then compares the length of those buffers before calling timingSafeEqual. Co-authored-by: cmuench <211294+cmuench@users.noreply.github.com> --- lib/mcp.js | 15 +++++++++++---- tests/mcp-timing.test.js | 29 +++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 4 deletions(-) create mode 100644 tests/mcp-timing.test.js diff --git a/lib/mcp.js b/lib/mcp.js index 577eba8..11b781b 100644 --- a/lib/mcp.js +++ b/lib/mcp.js @@ -182,11 +182,18 @@ export async function startMcpServer(options) { let authenticated = false; try { - if (queryToken && queryToken.length === token.length && crypto.timingSafeEqual(Buffer.from(queryToken), Buffer.from(token))) { - authenticated = true; - } else if (authHeader && authHeader.startsWith('Bearer ')) { + const tBuffer = Buffer.from(token); + if (queryToken) { + const qBuffer = Buffer.from(queryToken); + if (qBuffer.length === tBuffer.length && crypto.timingSafeEqual(qBuffer, tBuffer)) { + authenticated = true; + } + } + + if (!authenticated && authHeader && authHeader.startsWith('Bearer ')) { const headerToken = authHeader.substring(7); - if (headerToken.length === token.length && crypto.timingSafeEqual(Buffer.from(headerToken), Buffer.from(token))) { + const hBuffer = Buffer.from(headerToken); + if (hBuffer.length === tBuffer.length && crypto.timingSafeEqual(hBuffer, tBuffer)) { authenticated = true; } } diff --git a/tests/mcp-timing.test.js b/tests/mcp-timing.test.js new file mode 100644 index 0000000..a059e6f --- /dev/null +++ b/tests/mcp-timing.test.js @@ -0,0 +1,29 @@ +import { jest } from '@jest/globals'; +import crypto from 'crypto'; + +describe('MCP Authentication Vulnerability', () => { + test('timingSafeEqual works properly with multibyte character attacks', () => { + const token = "regular_token_123"; + // '£' has length 1 in javascript string, but 2 bytes in utf-8 + // "regular_token_12£" has length 17, same as token + const queryToken = "regular_token_12£"; + + expect(queryToken.length).toBe(token.length); + + let authenticated = false; + try { + const tBuffer = Buffer.from(token); + if (queryToken) { + const qBuffer = Buffer.from(queryToken); + if (qBuffer.length === tBuffer.length && crypto.timingSafeEqual(qBuffer, tBuffer)) { + authenticated = true; + } + } + } catch(e) { + // this should not be reached! + expect(false).toBe(true); + } + + expect(authenticated).toBe(false); + }); +}); From 7cd3dd79a29fe4c6f50fc04708d8d5157cf953e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20M=C3=BCnch?= Date: Tue, 7 Jul 2026 18:11:59 +0200 Subject: [PATCH 2/2] Potential fix for pull request finding 'Unused variable, import, function or class' Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com> --- tests/mcp-timing.test.js | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/mcp-timing.test.js b/tests/mcp-timing.test.js index a059e6f..154c84a 100644 --- a/tests/mcp-timing.test.js +++ b/tests/mcp-timing.test.js @@ -1,4 +1,3 @@ -import { jest } from '@jest/globals'; import crypto from 'crypto'; describe('MCP Authentication Vulnerability', () => {