modelcontextprotocol / modelcontextprotocol/typescript-sdk
OAuth client_secret comparison in clientAuth.ts is not constant-time
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
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- 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