Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Warning Rate limit exceeded@elie222 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 44 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (2)
WalkthroughThis pull request updates the project's environment variables and encryption logic. It introduces two new Google encryption variables and renames the existing encryption variables for Tinybird. Relevant changes are made in documentation, environment configuration files, and TypeScript definitions. New scripts and utility functions for the encryption and decryption of OAuth tokens are added, along with a Prisma extension that automatically encrypts tokens during database operations. Changes
Sequence Diagram(s)sequenceDiagram
participant S as Encrypt Script
participant D as Database (Prisma)
participant U as Token Checker
participant E as Encryption Util
S->>D: Query Google accounts (ID, tokens)
loop For each account
S->>U: Check if token is unencrypted
alt Token is unencrypted
S->>E: Encrypt access & refresh tokens
S->>D: Update account with encrypted tokens
else
S->>S: Skip account
end
end
S->>S: Log summary of processing
sequenceDiagram
participant A as Application
participant P as Prisma Client (with extension)
participant EE as Encryption Util
participant DE as Decryption Util
A->>P: Create/Update/Upsert account with tokens
P->>EE: Encrypt tokens before DB operation
P->>DB: Write encrypted tokens
A->>P: Query account tokens
P->>DE: Decrypt tokens after DB retrieval
P->>A: Return decrypted tokens
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
packages/tinybird/src/encrypt.ts (1)
3-6: Consider adding a security warning comment.While using a fixed IV for deterministic encryption is intentional here, it's worth adding a stronger warning comment about the security implications.
// don't store the text in plain text in our database to increase user privacy. // use a deterministic encryption algorithm so that we can perform analytics on the data // and decrypt it later. we use a fixed iv so that the encryption is deterministic. -// for stronger encryption don't use a fixed iv but for our use case this is fine. +// WARNING: Using a fixed IV reduces security but enables deterministic encryption. +// This is acceptable for our analytics use case where we need to match encrypted values, +// but should NOT be used for sensitive data requiring strong encryption guarantees.apps/web/utils/prisma-extensions.ts (2)
8-11: Handle potential decryption errors gracefully.
If the stored value inaccess_tokenis invalid or corrupted,decryptToken(account.access_token)may throw. Consider wrapping the decryption in a try-catch block or performing a preliminary validation of the encrypted token to avoid runtime errors.
24-32: Unify token encryption approach for create operations.
In thiscreatehandler, you directly encrypt the tokens if provided as strings. However, in theupdateandupserthandlers, you also handle the Prismasetsyntax. For consistency and maintainability, consider using the same logic forcreate, especially if Prisma’screatemethod can accept nestedset.apps/web/utils/prisma.ts (1)
10-12: Refine global assignment to preserve type safety.
Castingprismatoanycan obscure potential type errors. Consider defining a declared global interface for Prisma instead of usingas any, ensuring stricter type-checking while preserving the shared client instance in development.apps/web/scripts/encrypt-tokens.ts (2)
1-9: Consider using the TypeScript config alias for the import.Replace the relative import path with the TypeScript config alias for consistency.
-import { encryptToken } from "@/utils/encryption"; +import { encryptToken } from "~/utils/encryption";
11-78: Consider adding batch processing for better performance.The current implementation processes accounts one at a time. For better performance, consider implementing batch processing.
async function main() { console.log("Starting token encryption..."); const accounts = await prisma.account.findMany({ select: { id: true, access_token: true, refresh_token: true, }, where: { provider: "google", }, }); console.log(`Found ${accounts.length} Google accounts to process`); let updated = 0; let skipped = 0; + const BATCH_SIZE = 100; + const accountBatches = []; + + for (let i = 0; i < accounts.length; i += BATCH_SIZE) { + accountBatches.push(accounts.slice(i, i + BATCH_SIZE)); + } - for (const account of accounts) { + for (const batch of accountBatches) { try { - // Check if tokens are in their original unencrypted format - const hasUnencryptedAccessToken = isUnencryptedGoogleToken( - account.access_token, - "access", - ); - const hasUnencryptedRefreshToken = isUnencryptedGoogleToken( - account.refresh_token, - "refresh", - ); - - // If no unencrypted tokens found, skip this account - if (!hasUnencryptedAccessToken && !hasUnencryptedRefreshToken) { - skipped++; - continue; - } - - // Only encrypt tokens that are in their original format - await prisma.account.update({ - where: { id: account.id }, - data: { - access_token: hasUnencryptedAccessToken - ? encryptToken(account.access_token) - : account.access_token, - refresh_token: hasUnencryptedRefreshToken - ? encryptToken(account.refresh_token) - : account.refresh_token, - }, - }); + const updates = batch + .map(account => { + const hasUnencryptedAccessToken = isUnencryptedGoogleToken( + account.access_token, + "access", + ); + const hasUnencryptedRefreshToken = isUnencryptedGoogleToken( + account.refresh_token, + "refresh", + ); + + if (!hasUnencryptedAccessToken && !hasUnencryptedRefreshToken) { + skipped++; + return null; + } + + return prisma.account.update({ + where: { id: account.id }, + data: { + access_token: hasUnencryptedAccessToken + ? encryptToken(account.access_token) + : account.access_token, + refresh_token: hasUnencryptedRefreshToken + ? encryptToken(account.refresh_token) + : account.refresh_token, + }, + }); + }) + .filter(Boolean); + + await Promise.all(updates); + updated += updates.length; - updated++; - - if (updated % 100 === 0) { - console.log(`Processed ${updated} accounts...`); - } + console.log(`Processed ${updated} accounts...`); } catch (error) { - console.error(`Error processing account ${account.id}:`, error); + console.error(`Error processing batch:`, error); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
README.md(1 hunks)apps/web/.env.example(2 hunks)apps/web/env.ts(2 hunks)apps/web/scripts/encrypt-tokens.ts(1 hunks)apps/web/utils/encryption.ts(1 hunks)apps/web/utils/prisma-extensions.ts(1 hunks)apps/web/utils/prisma.ts(1 hunks)packages/tinybird/src/encrypt.ts(1 hunks)turbo.json(1 hunks)
🔇 Additional comments (9)
packages/tinybird/src/encrypt.ts (1)
10-11: LGTM! Clear separation of encryption configurations.The renaming of environment variables from generic to Tinybird-specific improves clarity and separation of concerns.
README.md (1)
127-128: LGTM! Clear documentation of new environment variables.The documentation clearly explains the new encryption variables and provides secure methods for generating values using OpenSSL.
turbo.json (1)
35-38: LGTM! Consistent environment variable updates.The environment variables have been properly updated to reflect the separation between Tinybird and Google encryption configurations.
apps/web/.env.example (1)
10-11: LGTM! Clear example configuration with helpful generation commands.The example configuration properly documents the new encryption variables and includes helpful OpenSSL commands for generating secure values.
Also applies to: 37-38
apps/web/utils/prisma-extensions.ts (3)
33-53: Looks good.
The encryption checks for string vs.setusage during updates are well-structured and consistent.
54-74: Consistent updateMany approach.
Similar to the update handler, you’ve correctly covered both string andsetpossibilities for tokens.
75-103: Well-structured upsert logic.
This mirrors the create/update approach, ensuring tokens are encrypted in both the create and update paths. Good job keeping the logic uniform here.apps/web/env.ts (2)
13-14: LGTM! New required environment variables for Google OAuth token encryption.The addition of
GOOGLE_ENCRYPT_SECRETandGOOGLE_ENCRYPT_SALTas required variables is appropriate for implementing secure token encryption.
38-39: LGTM! Renamed Tinybird encryption variables for better clarity.The renaming of
ENCRYPT_SECRETandENCRYPT_SALTtoTINYBIRD_ENCRYPT_SECRETandTINYBIRD_ENCRYPT_SALTimproves clarity by explicitly indicating their association with Tinybird.
apps/web/utils/encryption.ts
Outdated
| const key = scryptSync(env.GOOGLE_ENCRYPT_SECRET, env.GOOGLE_ENCRYPT_SALT, 32); // 32 bytes for AES-256 | ||
| const iv = Buffer.alloc(16, 0); // A fixed IV (all zeros) |
There was a problem hiding this comment.
Avoid using a static IV for AES encryption.
Using a fixed IV of all zeros can undermine security, allowing patterns in ciphertext. It’s generally recommended to generate a random IV for each encryption and store it with the ciphertext. Consider this refactor to incorporate a random IV:
-const iv = Buffer.alloc(16, 0); // A fixed IV (all zeros)
+function generateRandomIV() {
+ return crypto.randomBytes(16);
+}
export function encryptToken(text: string | null): string | null {
if (!text) return null;
- const cipher = createCipheriv(ALGORITHM, key, iv);
+ const iv = generateRandomIV();
+ const cipher = createCipheriv(ALGORITHM, key, iv);
...
// Prepend IV to the ciphertext before returning
return iv.toString("hex") + ":" + encrypted.toString("hex");
}And for decryption, parse out the IV supplied in the ciphertext.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const key = scryptSync(env.GOOGLE_ENCRYPT_SECRET, env.GOOGLE_ENCRYPT_SALT, 32); // 32 bytes for AES-256 | |
| const iv = Buffer.alloc(16, 0); // A fixed IV (all zeros) | |
| const key = scryptSync(env.GOOGLE_ENCRYPT_SECRET, env.GOOGLE_ENCRYPT_SALT, 32); // 32 bytes for AES-256 | |
| // Removed the fixed IV declaration | |
| // Added a helper function to generate a random IV | |
| function generateRandomIV() { | |
| return crypto.randomBytes(16); | |
| } | |
| export function encryptToken(text: string | null): string | null { | |
| if (!text) return null; | |
| const iv = generateRandomIV(); | |
| const cipher = createCipheriv(ALGORITHM, key, iv); | |
| // ... perform the encryption process using `cipher` | |
| // Prepend IV to the ciphertext before returning | |
| return iv.toString("hex") + ":" + encrypted.toString("hex"); | |
| } |
| function isUnencryptedGoogleToken( | ||
| token: string | null, | ||
| type: "access" | "refresh", | ||
| ): boolean { | ||
| if (!token) return false; | ||
|
|
||
| // Google OAuth tokens have specific prefixes | ||
| return type === "access" | ||
| ? token.startsWith("ya29.") | ||
| : token.startsWith("1//"); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Consider adding unit tests for the token validation function.
The isUnencryptedGoogleToken function contains critical logic for identifying unencrypted tokens. Consider adding unit tests to ensure its reliability.
Also, consider adding JSDoc comments to document the function's purpose and parameters:
+/**
+ * Checks if a Google OAuth token is in its original unencrypted format.
+ * @param token - The token to check
+ * @param type - The type of token ('access' or 'refresh')
+ * @returns boolean indicating if the token is unencrypted
+ */
function isUnencryptedGoogleToken(
token: string | null,
type: "access" | "refresh",
): boolean {🏁 Script executed:
#!/bin/bash
# Description: Check if there are any test files for the encrypt-tokens script
# Search for test files
fd "encrypt-tokens.test.ts" "apps/web/scripts"Length of output: 44
Action Required: Enhance Test Coverage & Documentation for isUnencryptedGoogleToken
It appears that no dedicated unit tests for the token validation logic were found in the expected location. To ensure the critical logic in this function is resilient, please:
- Add unit tests: Verify the behavior of
isUnencryptedGoogleTokenfor both “access” and “refresh” tokens across various edge cases (including null or unexpected values). If tests exist elsewhere in the repository, please confirm their scope covers this logic. - Include JSDoc comments: Document the function’s purpose, parameters, and return value. For example, you might add:
+/**
+ * Checks if a Google OAuth token is in its original unencrypted format.
+ * @param token - The token to check (can be null).
+ * @param type - The type of token ('access' or 'refresh').
+ * @returns true if the token starts with the expected prefix, false otherwise.
+ */
function isUnencryptedGoogleToken(
token: string | null,
type: "access" | "refresh",
): boolean {If further confirmation is needed regarding the existence of related tests, please run an additional verification such as:
Please verify manually if test coverage exists somewhere else or consider adding these tests if missing.
🏁 Script executed:
[run_scripts]
#!/bin/bash
# Search for references to `isUnencryptedGoogleToken` in test files across the repository
rg "isUnencryptedGoogleToken" -g "*test*.ts" .Length of output: 168
Action Required: Enhance Test Coverage & JSDoc Documentation
The current implementation of isUnencryptedGoogleToken in apps/web/scripts/encrypt-tokens.ts is critical, yet our search did not locate any dedicated unit tests for this function. Please add comprehensive tests to cover scenarios for both "access" and "refresh" token types—including tests for null or malformed tokens—to ensure this logic remains robust over time. In addition, please add JSDoc comments to clarify the function's purpose, input parameters, and expected output.
For example, you might update the code as follows:
+/**
+ * Checks if a Google OAuth token is in its original unencrypted format.
+ * @param token - The token to be validated (can be null).
+ * @param type - The token type, either 'access' or 'refresh'.
+ * @returns true if the token starts with the expected prefix; false otherwise.
+ */
function isUnencryptedGoogleToken(
token: string | null,
type: "access" | "refresh",
): boolean {
if (!token) return false;
// Google OAuth tokens have specific prefixes
return type === "access"
? token.startsWith("ya29.")
: token.startsWith("1//");
}Action Items:
- Add Unit Tests: Include tests that verify the function's behavior for both valid token types and edge cases (e.g., null tokens or invalid formats).
- Enhance Documentation: Insert the above JSDoc (or similar) to improve code clarity and maintainability.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function isUnencryptedGoogleToken( | |
| token: string | null, | |
| type: "access" | "refresh", | |
| ): boolean { | |
| if (!token) return false; | |
| // Google OAuth tokens have specific prefixes | |
| return type === "access" | |
| ? token.startsWith("ya29.") | |
| : token.startsWith("1//"); | |
| } | |
| /** | |
| * Checks if a Google OAuth token is in its original unencrypted format. | |
| * @param token - The token to check (can be null). | |
| * @param type - The type of token ('access' or 'refresh'). | |
| * @returns true if the token starts with the expected prefix, false otherwise. | |
| */ | |
| function isUnencryptedGoogleToken( | |
| token: string | null, | |
| type: "access" | "refresh", | |
| ): boolean { | |
| if (!token) return false; | |
| // Google OAuth tokens have specific prefixes | |
| return type === "access" | |
| ? token.startsWith("ya29.") | |
| : token.startsWith("1//"); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/web/utils/encryption.ts (1)
11-11: Consider strengthening scrypt parameters.While using scrypt for key derivation is good, you could make it more resistant to brute-force attacks by specifying the cost parameters.
-const key = scryptSync(env.GOOGLE_ENCRYPT_SECRET, env.GOOGLE_ENCRYPT_SALT, 32); +const key = scryptSync(env.GOOGLE_ENCRYPT_SECRET, env.GOOGLE_ENCRYPT_SALT, 32, { N: 32768, r: 8, p: 1 });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/utils/encryption.ts(1 hunks)
🔇 Additional comments (3)
apps/web/utils/encryption.ts (3)
1-10: LGTM! Good choice of encryption algorithm.The code uses AES-256-GCM which provides both confidentiality and authenticity. The imports are clean and focused.
13-30: LGTM! Secure implementation with random IV.The encryption implementation follows cryptographic best practices:
- Random IV for each encryption
- Proper use of authentication tag
- Clear concatenation format
7-7:Details
✅ Verification successful
Verify environment variable configuration.
Let's ensure the encryption environment is properly configured.
🏁 Script executed:
#!/bin/bash # Check if environment variables are defined in configuration files echo "Checking environment variable configuration..." rg "GOOGLE_ENCRYPT_SECRET|GOOGLE_ENCRYPT_SALT" -A 2 -B 2 # Check for any security advisories related to node:crypto gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: NPM, package: "node") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 2198
Environment Configuration Verified
- The environment variables
GOOGLE_ENCRYPT_SECRETandGOOGLE_ENCRYPT_SALTare properly defined in the configuration files (as evidenced in README.md,apps/web/env.ts, and turbo.json).- The security advisory check for the node crypto module returned no vulnerabilities.
The encryption setup using
aes-256-gcmalong with deriving the key viascryptSyncappears compliant and secure. No further changes are required.
apps/web/utils/encryption.ts
Outdated
| console.error("Decryption failed:", error); | ||
| return null; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid logging decryption errors with full error details.
The current error logging could potentially expose sensitive information about the encryption system.
- console.error("Decryption failed:", error);
+ console.error("Decryption failed");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.error("Decryption failed:", error); | |
| return null; | |
| console.error("Decryption failed"); | |
| return null; |
1 participant
Summary by CodeRabbit
New Features
GOOGLE_ENCRYPT_SECRETandGOOGLE_ENCRYPT_SALT.Documentation
Chores
ENCRYPT_SECRETandENCRYPT_SALTwithTINYBIRD_ENCRYPT_SECRETandTINYBIRD_ENCRYPT_SALT.