Race condition (TOCTOU) — concurrent replay is not prevented

[rohit-sourcefuse]

Race condition (TOCTOU) — concurrent replay is not prevented

Two simultaneous requests carrying the same auth code will both call revokedTokensRepo.get(...) and receive null (not revoked), both pass jwtVerifier, and both reach revokedTokensRepo.set(...) — issuing two valid tokens for a single authorization code. This is a realistic attack: an intercepted code replayed in parallel with the legitimate exchange will succeed for both.

The fix requires an atomic set-if-not-exists operation — a Redis SET key value NX EX ttl that marks the code as used and checks whether it was already used in a single step.

Add this method to RevokedTokenRepository:

// In services/authentication-service/src/repositories/revoked-token.repository.ts

/**
 * Atomically marks the key as used if not already present.
 * Returns true if the key was set (first use), false if it already existed (replay).
 * Uses Redis SET NX EX for atomicity.
 */
async setIfNotExists(
  key: string,
  value: RevokedToken,
  options: {ttl: number},
): Promise<boolean> {
  // ttl from LoopBack is in milliseconds; Redis EX is in seconds
  const ttlSeconds = Math.ceil(options.ttl / 1000);
  // eslint-disable-next-line @typescript-eslint/no-explicit-any
  const connector = (this.dataSource.connector as any);
  const result = await connector.execute(
    'SET',
    [key, JSON.stringify(value), 'NX', 'EX', ttlSeconds],
  );
  // Redis SET NX returns 'OK' on success, null if key already existed
  return result !== null;
}

Then replace the current get + separate set with a single atomic call in generateToken:

// Replace the isCodeRevoked block AND the later revokedTokensRepo.set block with:
const authCodeKeyPrefix = 'revoked:auth_code';
const revokedAuthCodeKey = `${authCodeKeyPrefix}:${resultCode}`;

let codeWasNew: boolean;
try {
  codeWasNew = await this.revokedTokensRepo.setIfNotExists(
    revokedAuthCodeKey,
    {token: resultCode},
    {ttl: authClient.authCodeExpiration * this.msInSecond},
  );
} catch (repoError) {
  this.logger.error(
    `[AUTH] Revocation store error during auth code exchange. Key: ${revokedAuthCodeKey}. Error: ${(repoError as Error).message}`,
  );
  throw new HttpErrors.ServiceUnavailable(
    'Authentication service temporarily unavailable. Please retry.',
  );
}

if (!codeWasNew) {
  this.logger.warn(
    `[AUTH][SECURITY] Auth code replay attempt. Key: ${revokedAuthCodeKey}, clientId: ${request.clientId}`,
  );
  throw new HttpErrors.Unauthorized(AuthErrorKeys.CodeExpired);
}

// No separate set() call needed — code is already atomically marked above
const payload = (await this.jwtVerifier(resultCode, {
  audience: request.clientId,
})) as ClientAuthCode<User, typeof User.prototype.id>;

Note: moving the atomic mark before jwtVerifier also avoids the window where a JWT verification failure leaves the code permanently revoked without a token ever being issued.

Originally posted by @rohit-sourcefuse in #2497 (comment)