Fix review findings across auth, schema, and board state#2
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 0 minutes and 44 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR reorganizes shared utility functions across CLI and server packages, adds viewer identity propagation through headers, enhances authentication with timing-safe token comparison, and improves web app state management. Changes span configuration extensions, database URL normalization, GraphQL schema improvements, and localStorage access safety. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors shared utilities across the CLI and server, centralizing environment loading and database URL normalization. It introduces support for viewer identification via custom headers in the CLI, server, and web client. The server-side GraphQL resolvers are optimized to pre-fetch relations and now utilize constant-time comparisons for authentication tokens. Additionally, the web client's board state management is improved with a reconciliation strategy for optimistic updates and local overrides, alongside safer localStorage access. Review feedback highlights a security improvement for token validation to prevent leaking secret lengths through timing attacks.
| @@ -1,8 +1,14 @@ | |||
| import { timingSafeEqual } from 'node:crypto'; | |||
There was a problem hiding this comment.
Import createHash from node:crypto to support a more secure token comparison method that avoids leaking the secret's length via timing.
| import { timingSafeEqual } from 'node:crypto'; | |
| import { createHash, timingSafeEqual } from 'node:crypto'; |
| function tokensMatch(left: string, right: string): boolean { | ||
| const leftBuffer = Buffer.from(left, 'utf8'); | ||
| const rightBuffer = Buffer.from(right, 'utf8'); | ||
|
|
||
| if (leftBuffer.length !== rightBuffer.length) { | ||
| return false; | ||
| } | ||
|
|
||
| return timingSafeEqual(leftBuffer, rightBuffer); | ||
| } |
There was a problem hiding this comment.
The current implementation of tokensMatch leaks the length of the authToken because it returns false immediately if the lengths of the two buffers differ. An attacker can use this timing difference to determine the length of the secret token. To prevent this, hash both tokens using a fixed-length algorithm (like SHA-256) and then compare the resulting hashes using timingSafeEqual.
function tokensMatch(left: string, right: string): boolean {
const leftHash = createHash('sha256').update(left).digest();
const rightHash = createHash('sha256').update(right).digest();
return timingSafeEqual(leftHash, rightHash);
}![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| parsedUrl.username = safeDecode(parsedUrl.username); | ||
| parsedUrl.password = safeDecode(parsedUrl.password); |
There was a problem hiding this comment.
🔴 normalizeDatabaseUrl corrupts URLs containing percent-encoded percent signs (%25)
The refactored normalizeDatabaseUrl using the URL API fails to properly round-trip %25 sequences (literal % characters) in usernames or passwords. When safeDecode decodes %25 to %, the URL API's setter does NOT re-encode % back to %25 because % (0x25) is not in the WHATWG URL spec's userinfo percent-encode set. This produces invalid percent-encoding in the output (e.g., p%word instead of p%25word), which can cause database connection failures.
Reproduction and behavioral difference
For input postgres://user:p%25word@host/db:
- Old implementation (using
encodeURIComponent): correctly outputspostgres://user:p%25word@host/db - New implementation (using URL API): incorrectly outputs
postgres://user:p%word@host/db
The %wo in the output is not a valid percent-encoded triplet, which would likely cause PostgreSQL driver connection failures for any user whose database password contains a literal % character.
Prompt for agents
The issue is in the normalizeDatabaseUrl function in packages/server/src/database-url.ts. The URL API's username/password setters do not percent-encode the % character (because it's not in the WHATWG URL spec's userinfo percent-encode set). When safeDecode decodes %25 to %, and then the decoded value is assigned back via parsedUrl.username/password, the % is left as-is, producing invalid percent-encoding in the output.
Two approaches to fix:
1. Instead of using safeDecode + URL API setter, use encodeURIComponent(safeDecode(...)) to explicitly re-encode the decoded value, then manually reconstruct the URL string (similar to the old approach).
2. Keep the URL API approach but replace the URL API setter assignment with a manual encoding step: parsedUrl.username = encodeURIComponent(safeDecode(parsedUrl.username)) would not work directly since the setter double-encodes, but you could build the final URL string manually after parsing.
The simplest correct approach is to revert to using encodeURIComponent for the re-encoding step, since it correctly encodes % as %25, while still using the URL API for parsing the components.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
Validation
Summary by CodeRabbit
Release Notes
New Features
viewer-emailandviewer-idsettings for enhanced user identity management.Bug Fixes
Refactor