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.
What's broken
authenticateClientinpackages/server-legacy/src/auth/middleware/clientAuth.tscompares the client-suppliedclient_secretagainst the stored one with a plain!==: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.tsexplicitly usesSubtleCrypto.verifyfor its own HMAC check specifically because it's "constant-time by spec... no manual byte compare, notimingSafeEqualdependency" — 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'scrypto.timingSafeEqual, matching this repo's own pattern inrequestStateCodec.tsfor its constant-time check.timingSafeEqualrequires 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.