modelcontextprotocol / modelcontextprotocol/typescript-sdk

OAuth client_secret comparison in clientAuth.ts is not constant-time

Open
#2,780 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
TypeScript
Stars
13.4k
Forks
2.2k
Avg merge
3d 15h
Merged PRs (30d)
4

Description

What's broken

authenticateClient in packages/server-legacy/src/auth/middleware/clientAuth.ts compares the client-supplied client_secret against the stored one with a plain !==:

if (client.client_secret !== client_secret) {
    throw new InvalidClientError('Invalid client_secret');
}

Plain string comparison short-circuits at the first mismatching character, which is a timing side channel (CWE-208) on a value that's meant to be a secret. This SDK already treats this class of bug as worth avoiding elsewhere — packages/server/src/server/requestStateCodec.ts explicitly uses SubtleCrypto.verify for its own HMAC check specifically because it's "constant-time by spec... no manual byte compare, no timingSafeEqual dependency" — so this looks like an inconsistency rather than a deliberate choice.

Code we can run to see the problem

Not applicable in the classic sense (a timing side channel isn't a functional repro), but a raw microbenchmark of the comparison operator confirms it isn't constant-time at the language level — happy to share if useful. The practical exploitability over a real network is a separate, harder question (HTTP/event-loop jitter dominates at short string lengths), but the code itself has no defense-in-depth here regardless.

Suggested fix

Swap the !== for Node's crypto.timingSafeEqual, matching this repo's own pattern in requestStateCodec.ts for its constant-time check. timingSafeEqual requires equal-length buffers, so a length check up front is needed (a length mismatch is fine to fail fast on — it leaks length, not content, same tradeoff every constant-time-compare helper makes).

I have a small PR ready for this (few lines, plus a test covering the length-mismatch path) — will open it against this issue.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with authenticateClient in packages/server-legacy/src/auth/middleware/clientAuth.ts, then compare its secret-checking pattern with packages/server/src/server/requestStateCodec.ts. Done means the client_secret comparison avoids plain string comparison and a test covers the equal-length and length-mismatch paths; the issue notes that a small PR is already ready.

Written by the indexing model from the issue text.

Assessment

Tech stack
node.js, typescript
Domain
authentication, security
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.