fix: use constant-time comparison for all secret/token checks

[spidershield-contrib]

Summary

Replace ===/!== with crypto.timingSafeEqual for all authentication secret comparisons to prevent timing side-channel attacks.

Fixes #686

Problem

Seven locations compare secrets using JavaScript's ===/!==, which short-circuits on the first mismatched character. An attacker can measure response times to brute-force tokens one character at a time (CWE-208).

Solution

Add a safeCompare() utility wrapping crypto.timingSafeEqual and apply to all 7 secret comparison sites:

• Bearer token lookups (3 files)
• OAuth access token validation
• OAuth client secret verification
• Refresh token comparison

Changes

File	Change
src/utils/safeCompare.ts	New — constant-time string comparison wrapper
src/dao/BearerKeyDao.ts	key.token === token → safeCompare(key.token, token)
src/services/sseService.ts	Same pattern, 2 locations
src/middlewares/auth.ts	Same pattern
src/utils/oauthBearer.ts	accessToken !== token → !safeCompare(...)
src/services/oauthServerService.ts	clientSecret + refreshToken comparisons

Testing

• All 247 existing tests pass
• ESLint passes
• safeCompare handles unequal-length strings without leaking length info

[gemini-code-assist]

Summary of Changes

Hello, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly enhances the security posture of the application by addressing a critical timing side-channel vulnerability. By replacing standard string comparison operators with a cryptographically secure, constant-time comparison function, the system is now protected against attackers who might attempt to deduce secrets by measuring response times. This change improves the robustness of authentication mechanisms without altering their core logic.

Highlights

• Security Fix: Implemented constant-time comparison for all secret and token checks to mitigate timing side-channel attacks (CWE-208).
• New Utility Function: Introduced a new safeCompare() utility function that wraps Node.js's crypto.timingSafeEqual for secure string comparison.
• Widespread Application: Applied the safeCompare() function across seven different locations, including Bearer token lookups, OAuth access token validation, OAuth client secret verification, and refresh token comparisons.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Changelog

• src/dao/BearerKeyDao.ts
  • Updated bearer token lookup to use safeCompare for constant-time comparison.
• src/middlewares/auth.ts
  • Modified bearer token validation to utilize safeCompare for secure comparison.
• src/services/oauthServerService.ts
  • Integrated safeCompare for client secret and refresh token comparisons within the OAuth model.
• src/services/sseService.ts
  • Applied safeCompare to two instances of bearer token validation.
• src/utils/oauthBearer.ts
  • Updated OAuth access token resolution to use safeCompare.
• src/utils/safeCompare.ts
  • Added a new utility file containing the safeCompare function for constant-time string comparison.

Activity

• All 247 existing tests passed, ensuring no regressions were introduced.
• ESLint checks passed, maintaining code quality and style standards.
• The safeCompare function was verified to handle unequal-length strings without leaking length information, as described in the PR body.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request is a great step towards hardening the application against timing side-channel attacks by replacing standard string comparisons with a constant-time comparison function. The changes are applied consistently across several sensitive areas. However, my review found a subtle but important flaw in the implementation of the new safeCompare utility that could still leak the length of secrets via a timing attack. I've provided a detailed comment and a code suggestion to address this vulnerability. Once that is fixed, this PR will be a solid security improvement.

[gemini-code-assist]

This implementation of safeCompare is still vulnerable to a timing attack that can leak the length of the secret string a.

The issue is that Buffer.from(b) is only executed in the code path where a.length === b.length. This creates a small but measurable difference in execution time between the two branches. An attacker can measure this timing difference to determine if their supplied token has the same length as the secret, thus revealing the secret's length.

To fix this, we should ensure that both branches of the comparison perform a similar amount of work. This can be achieved by creating buffers from both strings unconditionally at the start of the function, and ensuring the timingSafeEqual function is always called on buffers of the same length.

  const bufA = Buffer.from(a);
  const bufB = Buffer.from(b);

  const areSameLength = bufA.length === bufB.length;
  // If lengths are different, we compare `bufA` with itself. This ensures the
  // `timingSafeEqual` function is always called and runs in a time
  // proportional to the secret's length, which is constant from the attacker's
  // perspective.
  const bufToCompare = areSameLength ? bufB : bufA;

  const result = timingSafeEqual(bufA, bufToCompare);

  // The final result is true only if the lengths were the same and the content matched.
  return areSameLength && result;

[teehooai]

@gemini-code-assist Good catch on the length leakage — fixed in 097e408.

Both buffers are now created unconditionally upfront, and timingSafeEqual always runs against bufA.length bytes (comparing bufA with itself when lengths differ). This closes the length-leakage side channel.

[gemini-code-assist]

Thanks for the update, @teehooai. I appreciate you addressing the length leakage concern. However, upon reviewing the safeCompare implementation in src/utils/safeCompare.ts (specifically the version in the current PR patches), it appears there's still a potential timing side-channel related to length.

The crypto.timingSafeEqual function in Node.js is designed to handle length differences securely and in constant time. Its documentation states it returns false if the buffers have different lengths, and this check is also constant time. Therefore, the explicit areSameLength check and its use in the final return areSameLength && result; statement reintroduces a timing vulnerability.

If bufA.length !== bufB.length, the areSameLength variable becomes false, and the function returns false almost immediately. If bufA.length === bufB.length, the function proceeds to call timingSafeEqual and then returns its result, which takes a measurably longer time. This difference in execution time could allow an attacker to deduce the length of the secret.

To fully close this side-channel, the safeCompare function can be simplified to directly use crypto.timingSafeEqual without the additional length checks, as timingSafeEqual already provides this security guarantee.

Severity: critical

Here's a suggested correction for src/utils/safeCompare.ts:

import { timingSafeEqual } from 'crypto';

/**
 * Compare two strings in constant time to prevent timing attacks.
 * Returns true if both strings are equal.
 */
export function safeCompare(a: string, b: string): boolean {
  const bufA = Buffer.from(a);
  const bufB = Buffer.from(b);

  // crypto.timingSafeEqual inherently handles length differences securely.
  // It returns false if lengths differ, and does so in constant time.
  return timingSafeEqual(bufA, bufB);
}