Skip to content

Conversation

@2witstudios
Copy link
Owner

@2witstudios 2witstudios commented Dec 29, 2025

Replace non-constant-time string comparisons with crypto.timingSafeEqual
to prevent timing attacks that could leak secret information:

  • OAuth state signature verification (google/callback/route.ts)
  • Cron secret authentication (cleanup-tokens/route.ts)
  • WebSocket fingerprint verification (ws-security.ts)

These vulnerabilities are similar to historical timing attacks like the
Xbox 360 bootloader HMAC bypass (2007-2008) and OAuth/OpenID library
vulnerabilities (2010) where byte-by-byte comparisons leaked timing
information that allowed attackers to forge valid signatures.

Summary by CodeRabbit

  • Security Improvements
    • Strengthened cryptographic verification for OAuth callbacks, cron endpoints, and WebSocket challenge/fingerprint checks by adopting constant-time comparisons and explicit length checks.
  • Tests
    • Improved tampering simulation in encryption tests to more reliably detect ciphertext modification.

✏️ Tip: You can customize this high-level summary in your review settings.

…ng attacks

Replace non-constant-time string comparisons with crypto.timingSafeEqual
to prevent timing attacks that could leak secret information:

- OAuth state signature verification (google/callback/route.ts)
- Cron secret authentication (cleanup-tokens/route.ts)
- WebSocket fingerprint verification (ws-security.ts)

These vulnerabilities are similar to historical timing attacks like the
Xbox 360 bootloader HMAC bypass (2007-2008) and OAuth/OpenID library
vulnerabilities (2010) where byte-by-byte comparisons leaked timing
information that allowed attackers to forge valid signatures.
@chatgpt-codex-connector
Copy link

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 29, 2025

📝 Walkthrough

Walkthrough

Replaces ad-hoc string comparisons with Node.js crypto.timingSafeEqual plus explicit length checks in OAuth callback, cron auth, and WebSocket security code; also updates an encryption test to mutate ciphertext by flipping a bit.

Changes

Cohort / File(s) Summary
API Route Security
apps/web/src/app/api/auth/google/callback/route.ts, apps/web/src/app/api/cron/cleanup-tokens/route.ts
Replace direct string comparisons with crypto.timingSafeEqual-based checks for HMAC/state and Authorization header. Add buffer length checks before comparison; preserve existing error responses and control flow.
WebSocket Security
apps/web/src/lib/websocket/ws-security.ts
Remove local timing-safe helper and use crypto.timingSafeEqual (aliased). Add explicit length checks and return distinct failure reasons when lengths differ; use UTF-8 buffers for fingerprint verification.
Tests — Encryption
packages/lib/src/__tests__/encryption-utils.test.ts
Change tampering approach: flip the least-significant bit of the final ciphertext byte (XOR with 0x01) to guarantee modification; test still expects decryption to fail.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I nudged a bit, a single flip,
Timings sealed — no sneaky sip,
OAuth, cron, and sockets tight,
Secrets hush in constant-night,
Rabbits dance — encryption's right! 🥕🔐

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: use constant-time comparison for HMAC signatures to prevent timing attacks' directly and accurately summarizes the main objective of the pull request: replacing non-constant-time comparisons with crypto.timingSafeEqual across multiple code paths to mitigate timing attacks.
✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a98a1ea and 933d7c5.

📒 Files selected for processing (1)
  • packages/lib/src/__tests__/encryption-utils.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{ts,tsx}: Never use any types - always use proper TypeScript types
Use camelCase for variable and function names
Use UPPER_SNAKE_CASE for constants
Use PascalCase for type and enum names
Use kebab-case for filenames, except React hooks (camelCase with use prefix), Zustand stores (camelCase with use prefix), and React components (PascalCase)
Lint with Next/ESLint as configured in apps/web/eslint.config.mjs
Message content should always use the message parts structure with { parts: [{ type: 'text', text: '...' }] }
Use centralized permission functions from @pagespace/lib/permissions (e.g., getUserAccessLevel, canUserEditPage) instead of implementing permission logic locally
Always use Drizzle client from @pagespace/db package for database access
Use ESM modules throughout the codebase

**/*.{ts,tsx}: Never use any types - always use proper TypeScript types
Write code that is explicit over implicit and self-documenting

Files:

  • packages/lib/src/__tests__/encryption-utils.test.ts
**/*.ts

📄 CodeRabbit inference engine (AGENTS.md)

**/*.ts: React hook files should use camelCase matching the exported hook name (e.g., useAuth.ts)
Zustand store files should use camelCase with use prefix (e.g., useAuthStore.ts)

Files:

  • packages/lib/src/__tests__/encryption-utils.test.ts
**/*.{ts,tsx,js,jsx,json}

📄 CodeRabbit inference engine (AGENTS.md)

Format code with Prettier

Files:

  • packages/lib/src/__tests__/encryption-utils.test.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T20:04:40.910Z
Learning: Applies to **/*auth*.{ts,tsx} : Use bcryptjs for password hashing
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Unit Tests
🔇 Additional comments (1)
packages/lib/src/__tests__/encryption-utils.test.ts (1)

167-171: Excellent improvement to test reliability.

The bit-flipping approach (XOR with 0x01) guarantees the ciphertext will always change, eliminating the 1/256 probability that the previous static replacement approach had of being a no-op. The use of padStart(2, '0') ensures proper hex formatting is maintained.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

… of static replacement

The previous test replaced the last ciphertext byte with 'FF', which had
a 1/256 chance of being a no-op if the byte was already 'FF'. This caused
flaky test failures in CI. Now we XOR with 0x01 to guarantee the byte changes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants