Feature to Encrypt user email in database #2627
Conversation
…awa-api into encrypt_email pull from develop
…sh-talawa-api into encrypt_email syncing branch
WalkthroughThis pull request introduces significant changes to enhance email security within the application. It adds new environment variables Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🪛 Gitleaks.github/workflows/pull-request.yml221-221: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key) 🔇 Additional comments (1).github/workflows/pull-request.yml (1)
CRITICAL: Remove hardcoded encryption keys from workflow file The hardcoded encryption and hash pepper keys in the workflow file pose a significant security risk, even for test environments. These cryptographic materials should never be committed to version control. Please follow the previous review suggestion to:
- ENCRYPTION_KEY: 64730e71158b84687f01237d8f8128cc9cb7804d2d68c36823880456adad48c7
- HASH_PEPPER: 56195a1bd9b062fc4a63afff383ec28bf1464706725ae744c9fe7fc459426074
+ ENCRYPTION_KEY: ${{ secrets.TEST_ENCRYPTION_KEY }}
+ HASH_PEPPER: ${{ secrets.TEST_HASH_PEPPER }}
🧰 Tools🪛 Gitleaks221-221: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key) 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
Documentation and Community
|
Our Pull Request Approval ProcessWe have these basic policies to make the approval process smoother for our volunteer team. Testing Your CodePlease make sure your code passes all tests. Our test code coverage system will fail if these conditions occur:
The process helps maintain the overall reliability of the code base and is a prerequisite for getting your PR approved. Assigned reviewers regularly review the PR queue and tend to focus on PRs that are passing. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
|
will fix these linting errors, and I have to edit .env.sample, setup.ts and schema.graphql to implement this functionality |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
🛑 Comments failed to post (68)
sample_data/defaultUser.json (1)
13-14:
⚠️ Potential issue
⚠️ Security: Avoid committing real encryption outputs in sample dataWhile the encryption implementation looks solid, it's recommended to use placeholder/mock encrypted and hashed values in sample data files to avoid potentially leaking information about the encryption patterns (IV/salt).
Consider replacing with clearly marked sample values:
- "email": "84ced9952afed3c0ef7b998a835603f6:0da463e5928e34c4d32a1117b8c77649:8c28567b1daa0427f8214873e241392eebf94d913b97", - "hashedEmail": "d8efa7dd6141f95c75295d4f9beacd06216d0f72450ed99b87231f5909cbc190", + "email": "SAMPLE_IV:SAMPLE_SALT:SAMPLE_ENCRYPTED_EMAIL", + "hashedEmail": "SAMPLE_HASHED_EMAIL_VALUE",📝 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."email": "SAMPLE_IV:SAMPLE_SALT:SAMPLE_ENCRYPTED_EMAIL", "hashedEmail": "SAMPLE_HASHED_EMAIL_VALUE",tests/utilities/encryptionModule.spec.ts (3)
1-7:
⚠️ Potential issueAdd test cases for error scenarios
The test suite currently only covers the happy path. For security-critical features like encryption, it's crucial to test error scenarios.
Consider adding test cases for:
- Invalid encryption key
- Malformed encrypted data
- Empty or invalid email input
- Buffer overflow attempts with very long emails
Would you like me to help generate these additional test cases?
16-26:
⚠️ Potential issueStrengthen encryption/decryption test coverage
The current test case is basic and doesn't cover important scenarios for email encryption.
- Add type information for decrypted result:
- const { decrypted } = decryptEmail(encryptedWithEmailSalt); + const { decrypted }: { decrypted: string } = decryptEmail(encryptedWithEmailSalt);
- Add more test cases:
it("should handle email addresses with special characters", () => { const email = "[email protected]"; const encrypted = encryptEmail(email); const { decrypted } = decryptEmail(encrypted); expect(decrypted).toEqual(email); }); it("should use a unique IV for each encryption", () => { const email = "[email protected]"; const encrypted1 = encryptEmail(email); const encrypted2 = encryptEmail(email); expect(encrypted1).not.toEqual(encrypted2); }); it("should maintain consistent encryption format", () => { const email = "[email protected]"; const encrypted = encryptEmail(email); // Format should be: hex(iv):hex(encryptedData) expect(encrypted).toMatch(/^[0-9a-f]+:[0-9a-f]+$/i); });
9-14:
⚠️ Potential issueEnhance IV generation tests for cryptographic security
The current test has several issues:
- Test description uses "Salt" while function name uses "IV" - maintain consistent terminology
- Only testing the length isn't sufficient for a cryptographic function
Consider these improvements:
- describe("generateRandomSalt", () => { + describe("generateRandomIV", () => { it("should generate a random salt of the specified length", () => { const salt = generateRandomIV(); expect(salt.length).toEqual(2 * 16); }); + + it("should generate unique IVs for each call", () => { + const iv1 = generateRandomIV(); + const iv2 = generateRandomIV(); + expect(iv1).not.toEqual(iv2); + }); + + it("should generate IV with valid hex characters", () => { + const iv = generateRandomIV(); + expect(iv).toMatch(/^[0-9a-f]+$/i); + }); });📝 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.describe("generateRandomIV", () => { it("should generate a random salt of the specified length", () => { const salt = generateRandomIV(); expect(salt.length).toEqual(2 * 16); }); it("should generate unique IVs for each call", () => { const iv1 = generateRandomIV(); const iv2 = generateRandomIV(); expect(iv1).not.toEqual(iv2); }); it("should generate IV with valid hex characters", () => { const iv = generateRandomIV(); expect(iv).toMatch(/^[0-9a-f]+$/i); }); });src/resolvers/Post/creator.ts (2)
22-24:
⚠️ Potential issueAdd error handling for decryption and improve null checks.
- The decryption operation could fail and should be wrapped in a try-catch
- The code can be simplified using optional chaining as suggested by static analysis
- The decryption result's structure should be validated
- if (creator && creator.email) { - creator.email = decryptEmail(creator.email).decrypted; - } + if (creator?.email) { + try { + const decryptionResult = decryptEmail(creator.email); + if (!decryptionResult?.decrypted) { + console.error('Invalid decryption result'); + throw new Error('Failed to decrypt email'); + } + creator.email = decryptionResult.decrypted; + } catch (error) { + console.error('Failed to decrypt email:', error); + throw new Error('Failed to process creator email'); + } + }📝 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.if (creator?.email) { try { const decryptionResult = decryptEmail(creator.email); if (!decryptionResult?.decrypted) { console.error('Invalid decryption result'); throw new Error('Failed to decrypt email'); } creator.email = decryptionResult.decrypted; } catch (error) { console.error('Failed to decrypt email:', error); throw new Error('Failed to process creator email'); } }🧰 Tools
🪛 Biome
[error] 22-22: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
18-27: 💡 Codebase verification
Based on the evidence gathered, I can now generate the final response:
Performance concerns about email decryption are valid and need attention
The codebase shows that posts are indeed fetched in bulk through paginated queries using
PostsConnection. The Organization's posts resolver implementation confirms that multiple posts are retrieved in a single query, with each post potentially requiring email decryption for its creator. This can lead to performance issues when:
- Fetching posts with high pagination limits (up to
MAXIMUM_FETCH_LIMIT)- Each post triggers a separate creator resolution with email decryption
- No caching mechanism exists for decrypted emails
Consider implementing:
- Email decryption caching using a short-lived cache
- Batch loading of creators using DataLoader
- Making email an optional field in the creator type
🔗 Analysis chain
Consider performance implications of email decryption.
The current implementation decrypts emails on every creator resolution, which could impact performance for queries returning multiple posts. Consider:
- Implementing caching for decrypted emails
- Making email decryption optional through GraphQL field selection
Let's check if this resolver is used in any queries that might fetch multiple posts:
The previous attempt might have failed due to the schema being defined differently. Let's try these alternative approaches to:
- Find the GraphQL schema file directly
- Look for query definitions in the code
- Find files related to Posts to understand the query patterns
I want to verify:
- If there are queries that return multiple posts
- How the posts resolver is implemented
- If pagination is used for post queries, which would indicate bulk fetching
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for GraphQL queries that might fetch multiple posts ast-grep --pattern 'type Query { $$$ $posts$$$ { $$$ creator { $$$ } $$$ } $$$ }'Length of output: 113
Script:
#!/bin/bash # Let's try a different approach to find GraphQL schema definitions fd "schema.graphql" --type f --exec cat {} # Also search for query definitions in TypeScript files ast-grep --pattern 'const $_ = gql` query { $$$ } `' # And look for resolver implementations that might fetch multiple posts rg -l "Posts?" --type tsLength of output: 45358
Script:
#!/bin/bash # Let's check the GraphQL schema for queries that return multiple posts rg -A 10 "type Query" src/typeDefs/queries.ts # Also check the posts resolver implementation cat src/resolvers/Organization/posts.ts # And check if there's a connection type for posts pagination rg "PostsConnection" src/typeDefs/types.ts -A 5Length of output: 6461
🧰 Tools
🪛 Biome
[error] 22-22: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
tests/utilities/hashingModule.spec.ts (2)
25-34:
⚠️ Potential issueImprove robustness of HASH_PEPPER test.
The current implementation has several issues:
- Missing try-finally block to ensure pepper restoration
- No validation of originalPepper
- Inconsistent indentation
it("should produce different hashes with different HASH_PEPPER values", () => { - const email = "[email protected]" - const originalPepper = process.env.HASH_PEPPER; - process.env.HASH_PEPPER = "pepper1"; - const hash1 = hashEmail(email); - process.env.HASH_PEPPER = "pepper2"; - const hash2 = hashEmail(email); - expect(hash1).not.toEqual(hash2); - process.env.HASH_PEPPER = originalPepper; + const email = "[email protected]"; + const originalPepper = process.env.HASH_PEPPER; + + if (!originalPepper) { + throw new Error("HASH_PEPPER environment variable is required"); + } + + try { + process.env.HASH_PEPPER = "pepper1"; + const hash1 = hashEmail(email); + process.env.HASH_PEPPER = "pepper2"; + const hash2 = hashEmail(email); + expect(hash1).not.toEqual(hash2); + } finally { + process.env.HASH_PEPPER = originalPepper; + } });📝 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.it("should produce different hashes with different HASH_PEPPER values", () => { const email = "[email protected]"; const originalPepper = process.env.HASH_PEPPER; if (!originalPepper) { throw new Error("HASH_PEPPER environment variable is required"); } try { process.env.HASH_PEPPER = "pepper1"; const hash1 = hashEmail(email); process.env.HASH_PEPPER = "pepper2"; const hash2 = hashEmail(email); expect(hash1).not.toEqual(hash2); } finally { process.env.HASH_PEPPER = originalPepper; } });
12-23: 🛠️ Refactor suggestion
Add error handling tests and improve maintainability.
The hash consistency tests are good, but consider these improvements:
- Extract the hash format regex as a constant
- Add explicit error handling tests
+const HASH_FORMAT_REGEX = /^[a-f0-9]{64}$/i; + testCases.forEach((email)=> { it(`should correctly hash email: ${email}`, () => { const hashedFirstEmail = hashEmail(email); const hashedSecondEmail = hashEmail(email); expect(hashedFirstEmail).toEqual(hashedSecondEmail); expect(email.toLowerCase()).not.toEqual(hashedFirstEmail); - expect(hashedFirstEmail).toMatch(/^[a-f0-9]{64}$/i); + expect(hashedFirstEmail).toMatch(HASH_FORMAT_REGEX); }); }); + +it("should handle null/undefined gracefully", () => { + expect(() => hashEmail(null)).toThrow(); + expect(() => hashEmail(undefined)).toThrow(); +});📝 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 HASH_FORMAT_REGEX = /^[a-f0-9]{64}$/i; testCases.forEach((email)=> { it(`should correctly hash email: ${email}`, () => { const hashedFirstEmail = hashEmail(email); const hashedSecondEmail = hashEmail(email); expect(hashedFirstEmail).toEqual(hashedSecondEmail); expect(email.toLowerCase()).not.toEqual(hashedFirstEmail); expect(hashedFirstEmail).toMatch(HASH_FORMAT_REGEX); }); }); it("should handle null/undefined gracefully", () => { expect(() => hashEmail(null)).toThrow(); expect(() => hashEmail(undefined)).toThrow(); });src/resolvers/Organization/admins.ts (2)
33-33:
⚠️ Potential issueAvoid logging sensitive information in error messages
Logging user IDs alongside encryption errors could potentially expose sensitive information in logs. Consider using a more generic error message or implementing structured logging with appropriate masking.
- console.error(`Failed to decrypt email for user ${admin._id}:`, error); + console.error('Email decryption failed:', { userId: admin._id.toString(), error: error.message });📝 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('Email decryption failed:', { userId: admin._id.toString(), error: error.message });
24-24:
⚠️ Potential issueReplace
anytype with proper User typeUsing
anytype reduces type safety. Let's properly type the admin parameter.- const decryptedAdmins = admins.map((admin: any) => { + const decryptedAdmins = admins.map((admin: User) => {Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 GitHub Check: Check for linting, formatting, and type errors
[warning] 24-24:
Unexpected any. Specify a different typesrc/resolvers/Organization/creator.ts (1)
32-38: 💡 Codebase verification
⚠️ Potential issueEmail decryption error handling needs to be standardized across resolvers
The verification revealed inconsistent error handling patterns across email decryption implementations:
Several resolvers expose sensitive information in error logs:
Organization/creator.tsOrganization/admins.tsOrganization/blockedUsers.tsOrganization/members.tsSome resolvers handle decryption errors with try-catch (e.g., Organization resolvers), while others don't implement error handling at all (e.g.,
Query/me.ts,Query/user.ts).Inconsistent decryption result handling:
- Some use destructuring:
const { decrypted } = decryptEmail(user.email)- Others direct access:
decryptEmail(creator.email).decryptedRecommendations:
- Standardize error handling across all resolvers using email decryption
- Remove sensitive information from error logs
- Implement consistent error recovery strategy
- Consider extracting the decryption logic with error handling into a shared utility function
🔗 Analysis chain
Several concerns regarding error handling and security.
- Error logging exposes sensitive information:
- console.error(`Failed to decrypt email for user ${user._id}:`, error); + console.error(`Failed to decrypt email for user ${user._id}`);
Silent fallback to encrypted email might cause issues in UI/downstream systems expecting decrypted emails.
Consider adding type safety checks:
- const { decrypted } = decryptEmail(user.email); + const result = decryptEmail(user.email); + if (!result?.decrypted) { + throw new Error('Decryption failed: Invalid result format'); + } + return { ...user, email: result.decrypted };
- Consider implementing caching to avoid repeated decryption of the same email.
Let's verify if this decryption is consistently implemented across all user-related resolvers:
Let's analyze the email decryption implementation and its usage across resolvers to verify our concerns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other resolvers that might need similar email decryption rg -l "User.*findOne" "src/resolvers" # Check for existing caching implementations rg -l "cache.*email" "src"Length of output: 5446
Script:
#!/bin/bash # Check for email decryption usage across resolvers rg -A 2 "decryptEmail" "src/resolvers" # Check for the decryptEmail implementation rg -l "decryptEmail" "src" # Check for other email-related operations rg "email.*user" "src/resolvers"Length of output: 6935
src/utilities/hashEmail.ts (2)
33-51:
⚠️ Potential issueImprove error handling and fix unused variable
The implementation is secure with timing-safe comparison, but has some minor issues:
- Unused
errorvariable in catch block- Hex validation could be more precise
Apply these improvements:
export function compareHashedEmails(a: string, b: string): boolean { if (!a || !b || typeof a !== 'string' || typeof b !== 'string') { return false; } - if (!/^[0-9a-f]+$/i.test(a) || !/^[0-9a-f]+$/i.test(b)) { + // SHA-256 produces 64 character hex string + if (!/^[0-9a-f]{64}$/i.test(a) || !/^[0-9a-f]{64}$/i.test(b)) { return false; } try { return crypto.timingSafeEqual( Buffer.from(a, 'hex'), Buffer.from(b, 'hex') ); - } catch (error) { + } catch { console.error("Failed to compare hashes, likely due to invalid hex encoding"); return false; } }📝 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.export function compareHashedEmails(a: string, b: string): boolean { if (!a || !b || typeof a !== 'string' || typeof b !== 'string') { return false; } // SHA-256 produces 64 character hex string if (!/^[0-9a-f]{64}$/i.test(a) || !/^[0-9a-f]{64}$/i.test(b)) { return false; } try { return crypto.timingSafeEqual( Buffer.from(a, 'hex'), Buffer.from(b, 'hex') ); } catch { console.error("Failed to compare hashes, likely due to invalid hex encoding"); return false; } }🧰 Tools
🪛 eslint
[error] 47-47: 'error' is defined but never used.
(@typescript-eslint/no-unused-vars)
16-31:
⚠️ Potential issueStrengthen input validation and security checks
While the basic validation is good, consider these security improvements:
- Add email format validation
- Validate HASH_PEPPER minimum length
- Consider additional email normalization (e.g., trimming whitespace)
Here's a suggested implementation:
export function hashEmail(email: string) : string { if (!email || typeof email !== 'string') { throw new Error('Email parameter must be a non-empty string'); } + // Basic email format validation + const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; + if (!emailRegex.test(email)) { + throw new Error('Invalid email format'); + } if(!process.env.HASH_PEPPER) { throw new Error('Missing HASH_PEPPER environment variable required for secure email hashing'); } + if(process.env.HASH_PEPPER.length < 32) { + throw new Error('HASH_PEPPER must be at least 32 characters long'); + } const hashedEmail = crypto .createHmac("sha256", process.env.HASH_PEPPER) - .update(email.toLowerCase()) + .update(email.toLowerCase().trim()) .digest("hex"); return hashedEmail; }📝 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.export function hashEmail(email: string) : string { if (!email || typeof email !== 'string') { throw new Error('Email parameter must be a non-empty string'); } // Basic email format validation const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; if (!emailRegex.test(email)) { throw new Error('Invalid email format'); } if(!process.env.HASH_PEPPER) { throw new Error('Missing HASH_PEPPER environment variable required for secure email hashing'); } if(process.env.HASH_PEPPER.length < 32) { throw new Error('HASH_PEPPER must be at least 32 characters long'); } const hashedEmail = crypto .createHmac("sha256", process.env.HASH_PEPPER) .update(email.toLowerCase().trim()) .digest("hex"); return hashedEmail; }src/resolvers/Query/checkAuth.ts (1)
41-45: 💡 Codebase verification
⚠️ Potential issueAdd error handling for email decryption across all resolvers
The review comment is valid and reveals a broader pattern. Only
Organization/members.tsimplements proper error handling for decryption failures. All other resolvers usingdecryptEmailare missing try-catch blocks and error validation, making them vulnerable to:
- Silent failures during decryption
- Potential exposure of sensitive information in error states
- Missing audit trails for security-related operations
Key locations requiring error handling:
src/resolvers/Query/usersConnection.tssrc/resolvers/Query/users.tssrc/resolvers/Query/user.tssrc/resolvers/Query/organizationsMemberConnection.tssrc/resolvers/Query/me.tssrc/resolvers/Post/creator.tsEach resolver should follow the pattern implemented in
Organization/members.tswhich properly:
- Wraps decryption in try-catch
- Logs errors without exposing sensitive data
- Handles the error gracefully
🔗 Analysis chain
Add error handling for email decryption.
The current implementation has several security considerations:
- No error handling for decryption failures
- Missing validation of the decrypted email format
- No audit logging for security-sensitive operations
Consider implementing this safer approach:
- const { decrypted } = decryptEmail(currentUser.email); + try { + const { decrypted } = decryptEmail(currentUser.email); + + // Validate email format + if (!decrypted || !/^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(decrypted)) { + throw new Error('Invalid email format after decryption'); + } + + return { + ...currentUser, + email: decrypted, + image: currentUser.image + ? `${context.apiRootUrl}${currentUser.image}` + : null, + organizationsBlockedBy: [], + }; + } catch (error) { + // Log the error for security audit (avoid logging the actual email) + console.error('Failed to decrypt user email', { + userId: currentUser._id, + error: error.message + }); + throw new errors.InternalServerError( + 'Failed to process user data', + 'DECRYPTION_ERROR' + ); + }Let's verify the error handling in other authentication-related resolvers:
Let me check the implementation of these resolvers to see if they handle decryption errors properly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if other resolvers properly handle decryption errors ast-grep --pattern 'decryptEmail($_)'Length of output: 4851
Script:
#!/bin/bash # Check implementation of resolvers that use decryptEmail echo "=== Query/usersConnection.ts ===" rg -A 5 "decryptEmail" src/resolvers/Query/usersConnection.ts echo -e "\n=== Query/users.ts ===" rg -A 5 "decryptEmail" src/resolvers/Query/users.ts echo -e "\n=== Query/user.ts ===" rg -A 5 "decryptEmail" src/resolvers/Query/user.ts echo -e "\n=== Query/organizationsMemberConnection.ts ===" rg -A 5 "decryptEmail" src/resolvers/Query/organizationsMemberConnection.ts echo -e "\n=== Query/me.ts ===" rg -A 5 "decryptEmail" src/resolvers/Query/me.ts echo -e "\n=== Post/creator.ts ===" rg -A 5 "decryptEmail" src/resolvers/Post/creator.ts echo -e "\n=== Organization/members.ts ===" rg -A 5 "decryptEmail" src/resolvers/Organization/members.tsLength of output: 4220
src/env.ts (1)
35-35:
⚠️ Potential issueEnhance ENCRYPTION_KEY validation and documentation.
The current implementation only validates that ENCRYPTION_KEY is a string. Since this is used for email encryption, it requires stricter validation to ensure security:
- The key should be a valid Base64-encoded 32-byte string
- The validation requirements should be documented
Apply this diff to add proper validation:
- ENCRYPTION_KEY: z.string(), + ENCRYPTION_KEY: z + .string() + .describe("Base64-encoded 32-byte encryption key for securing user emails") + .refine( + (value) => { + // Validate Base64 format and length + try { + const decoded = Buffer.from(value, 'base64'); + return decoded.length === 32; + } catch { + return false; + } + }, + "ENCRYPTION_KEY must be a valid Base64-encoded 32-byte string" + ),📝 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.ENCRYPTION_KEY: z .string() .describe("Base64-encoded 32-byte encryption key for securing user emails") .refine( (value) => { // Validate Base64 format and length try { const decoded = Buffer.from(value, 'base64'); return decoded.length === 32; } catch { return false; } }, "ENCRYPTION_KEY must be a valid Base64-encoded 32-byte string" ),src/resolvers/Query/usersConnection.ts (1)
35-37:
⚠️ Potential issueAdd error handling and avoid direct mutations.
The current implementation has several potential issues:
- Missing error handling for decryption failures
- Directly mutating the lean object
- No type safety for the decrypted email
Consider this safer implementation:
- const { decrypted } = decryptEmail(user.email); - user.email = decrypted; + try { + const { decrypted } = decryptEmail(user.email); + return { + ...user, + email: decrypted + }; + } catch (error) { + throw new Error(`Failed to decrypt email: ${error.message}`); + }📝 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.try { const { decrypted } = decryptEmail(user.email); return { ...user, email: decrypted }; } catch (error) { throw new Error(`Failed to decrypt email: ${error.message}`); }src/resolvers/Query/me.ts (1)
56-57:
⚠️ Potential issueAdd error handling and improve data handling security.
The current implementation has several potential issues:
- No error handling for decryption failures
- Directly mutating the
currentUserobject after.lean()- Decrypted email remains in memory longer than necessary
Consider this safer implementation:
- const { decrypted } = decryptEmail(currentUser.email); - currentUser.email = decrypted; + try { + const { decrypted } = decryptEmail(currentUser.email); + return { + user: { + ...currentUser, + email: decrypted + } as InterfaceUser, + appUserProfile: userAppProfile as InterfaceAppUserProfile, + }; + } catch (error) { + throw new errors.ValidationError( + "Failed to decrypt email", + "EMAIL_DECRYPTION_FAILED", + "email" + ); + }Committable suggestion was skipped due to low confidence.
tests/resolvers/Organization/blockedUsers.spec.ts (1)
35-51: 🛠️ Refactor suggestion
Enhance error handling and test robustness.
While the test logic is sound, there are several areas for improvement:
- Error handling could be more specific about which email failed to decrypt
- The test should verify that the encryption key is properly configured
- The second expectation's logic could be more readable
Consider this improved implementation:
try { + // Verify encryption key is configured + if (!process.env.ENCRYPTION_KEY) { + throw new Error('ENCRYPTION_KEY is not configured'); + } + const decryptedBlockedUsers = blockedUsers.map((user) => { + try { return { ...user, email: decryptEmail(user.email).decrypted, }; + } catch (error) { + throw new Error(`Failed to decrypt email for user ${user._id}: ${error.message}`); + } }); expect(blockedUsersPayload).toEqual(decryptedBlockedUsers); expect( - decryptedBlockedUsers.every( - (user) => - user.email !== blockedUsers.find((u) => u._id == user._id)?.email, - ), + decryptedBlockedUsers.every((decryptedUser) => { + const originalUser = blockedUsers.find(u => u._id.equals(decryptedUser._id)); + return originalUser && decryptedUser.email !== originalUser.email; + }), ).toBe(true); } catch (error) { - console.error("Error decrypting emails:", error); + console.error("Test failed:", error.message); throw error; }📝 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.try { // Verify encryption key is configured if (!process.env.ENCRYPTION_KEY) { throw new Error('ENCRYPTION_KEY is not configured'); } const decryptedBlockedUsers = blockedUsers.map((user) => { try { return { ...user, email: decryptEmail(user.email).decrypted, }; } catch (error) { throw new Error(`Failed to decrypt email for user ${user._id}: ${error.message}`); } }); expect(blockedUsersPayload).toEqual(decryptedBlockedUsers); expect( decryptedBlockedUsers.every((decryptedUser) => { const originalUser = blockedUsers.find(u => u._id.equals(decryptedUser._id)); return originalUser && decryptedUser.email !== originalUser.email; }), ).toBe(true); } catch (error) { console.error("Test failed:", error.message); throw error; }src/resolvers/Query/user.ts (1)
37-37:
⚠️ Potential issueAdd error handling for email decryption failures.
The current implementation assumes decryption will always succeed. This could lead to runtime errors if the encryption key is invalid or the email data is corrupted.
- const { decrypted } = decryptEmail(user.email); + try { + const { decrypted } = decryptEmail(user.email); + user.email = decrypted; + } catch (error) { + throw new errors.ValidationError( + "Failed to decrypt user email", + "EMAIL_DECRYPTION_ERROR", + "email" + ); + }📝 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.try { const { decrypted } = decryptEmail(user.email); user.email = decrypted; } catch (error) { throw new errors.ValidationError( "Failed to decrypt user email", "EMAIL_DECRYPTION_ERROR", "email" ); }tests/helpers/advertisement.ts (1)
45-46:
⚠️ Potential issueMove email generation inside the function scope.
The current implementation has several potential issues:
- Global email variable could lead to test interference in parallel test execution
- The email format is hardcoded to Gmail, which might not cover all test cases
Consider refactoring like this:
-const email = `email${nanoid().toLowerCase()}@gmail.com`; -const hashedEmail = hashEmail(email) export const createTestSuperAdmin = async (): Promise<TestSuperAdminType> => { + const email = `email${nanoid().toLowerCase()}@gmail.com`; + const hashedEmail = hashEmail(email); const testSuperAdmin = await User.create({📝 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.export const createTestSuperAdmin = async (): Promise<TestSuperAdminType> => { const email = `email${nanoid().toLowerCase()}@gmail.com`; const hashedEmail = hashEmail(email); const testSuperAdmin = await User.create({tests/helpers/userAndUserFamily.ts (1)
19-19: 🛠️ Refactor suggestion
Consider using varied email domains in tests.
Using a fixed domain (@gmail.com) might miss edge cases. Consider generating test emails with different domains to ensure the encryption and hashing work correctly across various email formats.
- const email = `email${nanoid().toLowerCase()}@gmail.com`; + const domains = ['gmail.com', 'example.com', 'test.org']; + const randomDomain = domains[Math.floor(Math.random() * domains.length)]; + const email = `email${nanoid().toLowerCase()}@${randomDomain}`;📝 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 domains = ['gmail.com', 'example.com', 'test.org']; const randomDomain = domains[Math.floor(Math.random() * domains.length)]; const email = `email${nanoid().toLowerCase()}@${randomDomain}`;tests/resolvers/MembershipRequest/user.spec.ts (1)
38-43:
⚠️ Potential issueMultiple improvements needed in error handling and data security
Several issues need to be addressed in this test case:
- The error message "User not found" is inconsistent with USER_NOT_FOUND_ERROR.MESSAGE used in the second test case
- Using a generic Error instead of the specific error type
- The decrypted email remains in memory after the test
- No type validation for the decrypted email
Here's the suggested fix:
if (!user) { - throw new Error("User not found"); + throw new Error(USER_NOT_FOUND_ERROR.MESSAGE); } const { decrypted } = decryptEmail(user.email); + // Validate email format + if (typeof decrypted !== 'string' || !decrypted.includes('@')) { + throw new Error('Invalid decrypted email format'); + } user.email = decrypted; + // Clear sensitive data after test + afterEach(() => { + user.email = undefined; + });Committable suggestion was skipped due to low confidence.
tests/resolvers/Query/me.spec.ts (2)
8-8:
⚠️ Potential issueUse type-only import for
InterfaceUserSince
InterfaceUseris only used as a type, it should be imported using the type-only import syntax.Apply this change:
-import { AppUserProfile, InterfaceUser, User } from "../../../src/models"; +import { AppUserProfile, User } from "../../../src/models"; +import type { InterfaceUser } from "../../../src/models";📝 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.import { AppUserProfile, User } from "../../../src/models"; import type { InterfaceUser } from "../../../src/models";🧰 Tools
🪛 eslint
[error] 8-8: Imports "InterfaceUser" are only used as type.
(@typescript-eslint/consistent-type-imports)
64-65: 💡 Codebase verification
⚠️ Potential issueAdd email decryption verification
The test should decrypt the email using
decryptEmailto maintain consistency with other test files in the codebase. All other test files that handle user emails properly decrypt them usingdecryptEmail().decrypted.Consider this change:
+import { decryptEmail } from "../../../src/utilities/encryption"; const currentUser = mePayload.user as InterfaceUser; -currentUser.email = user.email; +currentUser.email = decryptEmail(user.email).decrypted;🔗 Analysis chain
Add email decryption verification
Since this PR implements email encryption, the test should verify that emails are properly decrypted when fetched. Currently, the test bypasses the decryption process by directly assigning the email.
Consider this change:
+import { decryptEmail } from "../../../src/utilities/encryption"; const currentUser = mePayload.user as InterfaceUser; -currentUser.email = user.email; +currentUser.email = await decryptEmail(user.email); + +// Add assertion to verify email decryption +expect(currentUser.email).toBe(testUser.email);
Let me verify how email decryption is handled in other test files to ensure consistency.
Let me check how the email decryption is actually used in these test files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify if decryptEmail utility is imported in other test files rg -l "decryptEmail.*from.*utilities/encryption" "tests/"Length of output: 495
Script:
#!/bin/bash # Check how other test files handle email decryption rg -A 5 "decryptEmail.*from.*utilities/encryption" "tests/"Length of output: 4704
Script:
#!/bin/bash # Check actual usage of decryptEmail in test files rg -B 2 -A 2 "decryptEmail\(" "tests/"Length of output: 14050
tests/resolvers/Query/checkAuth.spec.ts (2)
75-79: 🛠️ Refactor suggestion
Refactor duplicate error handling logic.
This error handling code is duplicated from the previous test case. Consider extracting it into a helper function.
Create a helper function:
const assertUsersExist = (testUser: any, user: any) => { if (!testUser || !user) { throw new Error(!testUser ? "Test user not found" : "User not returned from resolver"); } testUser.email = user.email; // Verify email encryption/decryption expect(user.email).toBeDefined(); expect(user.email).not.toMatch(/^[a-zA-Z0-9+/]*={0,2}$/); // Not base64 encoded };Then use it in both test cases:
-if (!testUser || !user) { - throw new Error("Error fetching users"); -} -testUser.email = user.email; +assertUsersExist(testUser, user);
44-48: 🛠️ Refactor suggestion
Improve error handling and add encryption verification.
The error handling could be more specific and the test should verify the email encryption:
- The error message "Error fetching users" is too generic. Consider separate messages for each case.
- The test should explicitly verify that the email is properly decrypted.
Consider this improvement:
if (!testUser || !user) { - throw new Error("Error fetching users"); + throw new Error(!testUser ? "Test user not found" : "User not returned from resolver"); } testUser.email = user.email; + +// Verify email encryption/decryption +expect(user.email).toBeDefined(); +expect(user.email).not.toMatch(/^[a-zA-Z0-9+/]*={0,2}$/); // Not base64 encodedCommittable suggestion was skipped due to low confidence.
tests/resolvers/Query/user.spec.ts (2)
95-101: 🛠️ Refactor suggestion
Consider refactoring duplicate user validation
The user existence check is duplicated across test cases. Consider extracting this into a helper function to maintain DRY principles and ensure consistent error handling.
// Add at the top of the file const validateUser = (user: any) => { if (!user) { throw new Error(USER_NOT_FOUND_ERROR.DESC); } return user; }; // Then use in test cases: const user = validateUser(await User.findOne({ _id: testUser?._id, }).lean());
106-106: 💡 Codebase verification
Additional test cases needed for email encryption robustness
The current test suite in
encryptionModule.spec.tsonly covers the basic happy path. Please add the following test cases:describe("encryptEmail and decryptEmail", () => { it("should handle empty email string", () => { const email = ""; // Test implementation }); it("should handle malformed encrypted data", () => { const invalidEncrypted = "invalid-encrypted-string"; // Test implementation }); it("should validate email format after decryption", () => { const email = "not-an-email"; // Test implementation }); });🔗 Analysis chain
Consider adding edge cases for email encryption
The current tests verify the happy path for email decryption, but consider adding test cases for:
- Handling malformed encrypted emails
- Verifying behavior with empty/null email values
- Testing email format validation post-decryption
This will ensure the encryption feature remains robust and secure.
Let me search for email validation test cases more specifically.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any existing test cases for email validation rg -l "test|describe.*email" tests/Length of output: 16433
Script:
#!/bin/bash # Check for email validation and encryption test cases rg -A 5 "test.*email.*encryption|test.*email.*validation|test.*malformed.*email" tests/ # Also check encryption module tests specifically cat tests/utilities/encryptionModule.spec.tsLength of output: 930
src/resolvers/Query/users.ts (1)
65-65:
⚠️ Potential issueAdd error handling for email decryption.
The decryption operation could fail but there's no error handling in place. This could lead to unhandled exceptions and API failures.
Consider adding try-catch:
- const { decrypted } = decryptEmail(user.email); + let decrypted; + try { + ({ decrypted } = decryptEmail(user.email)); + } catch (error) { + console.error(`Failed to decrypt email for user ${user._id}:`, error); + decrypted = null; // or handle as appropriate for your use case + }📝 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.let decrypted; try { ({ decrypted } = decryptEmail(user.email)); } catch (error) { console.error(`Failed to decrypt email for user ${user._id}:`, error); decrypted = null; // or handle as appropriate for your use case }tests/directives/directiveTransformer/roleDirectiveTransformer.spec.ts (2)
66-67: 🛠️ Refactor suggestion
Consider mocking encryption functions and adding validation.
A few suggestions to improve test robustness:
- Consider mocking
encryptEmailandhashEmailfunctions to avoid dependency on actual encryption in tests- Add assertions to verify that the stored email is actually encrypted/hashed
- Add test cases for encryption/hashing edge cases
Example test improvements:
// Mock setup vi.mock('../../../src/utilities/encryption', () => ({ encryptEmail: vi.fn((email) => `encrypted_${email}`), })); // Validation expect(testUser.email).toMatch(/^encrypted_/); expect(testUser.hashedEmail).toBeDefined(); expect(testUser.email).not.toBe(email);
61-67: 💡 Codebase verification
Documentation update needed for email encryption changes
The documentation needs to be updated to reflect the new email encryption workflow. Please add the following:
- Add a new section in DOCUMENTATION.md about email encryption and hashing
- Update INSTALLATION.md to include the new environment variables (ENCRYPTION_KEY, HASH_PEPPER)
- Add a section about migrating existing test data to use encrypted emails
Locations requiring updates:
DOCUMENTATION.md: Add under "Other information" sectionINSTALLATION.md: Add under "The Environment Variables in.env" sectiondocs/README.md: Add a note about email encryption in API documentation🔗 Analysis chain
Documentation update needed for email encryption changes.
As mentioned in the PR objectives, documentation updates are pending. Please ensure the following are documented:
- The new email encryption/hashing workflow
- Required environment variables (ENCRYPTION_KEY, HASH_PEPPER)
- Impact on existing tests and how to update them
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if documentation files exist and contain encryption-related updates rg -l "(?i)encrypt|hash" docs/ README.mdLength of output: 42
Script:
#!/bin/bash # First, let's check if docs directory exists and what files are in it fd . "docs" || echo "No docs directory found" # Then check for any markdown files in the root that might contain documentation fd -e md . --max-depth 1 # Finally, let's check for any documentation-related files that might have different extensions fd "(doc|docs|documentation|guide|readme)" --type fLength of output: 620
Script:
#!/bin/bash # Let's check the content of main documentation files for any existing encryption-related information echo "=== Checking DOCUMENTATION.md ===" cat DOCUMENTATION.md echo -e "\n=== Checking docs/README.md ===" cat docs/README.md echo -e "\n=== Checking main README.md ===" cat README.md echo -e "\n=== Checking INSTALLATION.md for env vars ===" cat INSTALLATION.mdLength of output: 60475
.env.sample (1)
115-119:
⚠️ Potential issueFix HASH_PEPPER format and enhance documentation.
Issues found:
- Variable format has inconsistent spacing
HASH_PEPPER =vs other variables- Missing implementation guidance
Apply this diff to fix the format and enhance documentation:
# This environment variable provides additional entropy for email hashing # Format: Random string of at least 32 characters # Generation: Use a cryptographically secure random string generator +# Example generation: openssl rand -hex 32 +# Example format: HASH_PEPPER=YOUR_HEX_STRING # WARNING: Keep this value secret and never commit it to version control -HASH_PEPPER = +# NOTE: Changing this value will invalidate all existing email hashes. +# Ensure database migration strategy is in place before changing. +HASH_PEPPER=📝 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.# This environment variable provides additional entropy for email hashing # Format: Random string of at least 32 characters # Generation: Use a cryptographically secure random string generator # Example generation: openssl rand -hex 32 # Example format: HASH_PEPPER=YOUR_HEX_STRING # WARNING: Keep this value secret and never commit it to version control # NOTE: Changing this value will invalidate all existing email hashes. # Ensure database migration strategy is in place before changing. HASH_PEPPER=tests/helpers/userAndOrg.ts (2)
10-12:
⚠️ Potential issueRemove unused crypto import
The
cryptomodule is imported but never used in this file.Apply this diff to remove the unused import:
import { encryptEmail } from "../../src/utilities/encryption"; -import crypto from "crypto"; import { hashEmail } from "../../src/utilities/hashEmail";📝 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.import { encryptEmail } from "../../src/utilities/encryption"; import { hashEmail } from "../../src/utilities/hashEmail";🧰 Tools
🪛 eslint
[error] 11-11: 'crypto' is defined but never used.
(@typescript-eslint/no-unused-vars)
24-29: 🛠️ Refactor suggestion
Enhance test email generation and validation
While the encryption implementation looks correct, there are a few suggestions to improve the test data generation:
- Consider using a more diverse email generation pattern to better test the encryption/hashing with various email formats:
- const email = `email${nanoid().toLowerCase()}@gmail.com`; + const email = `${nanoid(8)}${['', '.', '_'][Math.floor(Math.random() * 3)]}${nanoid(8)}@${['gmail.com', 'example.com', 'test.org'][Math.floor(Math.random() * 3)]}`;
- Add validation to ensure the encryption/hashing worked as expected:
const email = `email${nanoid().toLowerCase()}@gmail.com`; const hashedEmail = hashEmail(email); + // Validate hashing/encryption + if (!hashedEmail || hashedEmail.length < 32) { + throw new Error('Invalid hashed email generated'); + } + const encryptedEmail = encryptEmail(email); + if (!encryptedEmail) { + throw new Error('Email encryption failed'); + } let testUser = await User.create({ - email: encryptEmail(email), + email: encryptedEmail, hashedEmail: hashedEmail,📝 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 email = `${nanoid(8)}${['', '.', '_'][Math.floor(Math.random() * 3)]}${nanoid(8)}@${['gmail.com', 'example.com', 'test.org'][Math.floor(Math.random() * 3)]}`; const hashedEmail = hashEmail(email); // Validate hashing/encryption if (!hashedEmail || hashedEmail.length < 32) { throw new Error('Invalid hashed email generated'); } const encryptedEmail = encryptEmail(email); if (!encryptedEmail) { throw new Error('Email encryption failed'); } let testUser = await User.create({ email: encryptedEmail, hashedEmail: hashedEmail,tests/resolvers/Mutation/resetCommunity.spec.ts (1)
86-88: 🛠️ Refactor suggestion
Consider extracting email generation into a helper function.
While the implementation is correct, consider creating a helper function to encapsulate email generation, hashing, and encryption. This would improve reusability across tests and make the test setup more maintainable.
// In tests/helpers/user.ts export const generateTestEmail = () => { const email = `email${nanoid().toLowerCase()}@gmail.com`; return { email, hashedEmail: hashEmail(email), encryptedEmail: encryptEmail(email) }; };src/resolvers/Mutation/updateUserProfile.ts (1)
13-14:
⚠️ Potential issueRemove unused crypto import.
The
cryptoimport is not being used in this file. Please remove it to keep the codebase clean.Apply this diff:
-import crypto from "crypto";for the
hashEmailimport as it's properly utilized.📝 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.import { hashEmail } from "../../utilities/hashEmail";🧰 Tools
🪛 GitHub Check: Check for linting, formatting, and type errors
[failure] 13-13:
'crypto' is defined but never used🪛 eslint
[error] 13-13: 'crypto' is defined but never used.
(@typescript-eslint/no-unused-vars)
src/resolvers/Mutation/signUp.ts (1)
36-38: 💡 Codebase verification
Email validation is missing in the signup process
The
isValidEmailfunction exists in the codebase atsrc/setup/isValidEmail.tsbut is not being used in the signup process. The code only normalizes and hashes the email without validating its format. You should add email validation before processing:const normalizedEmail = args.data.email.toLowerCase(); if (!isValidEmail(normalizedEmail)) { throw new errors.ValidationError("Invalid email format"); } const hashedEmail = hashEmail(normalizedEmail);🔗 Analysis chain
Verify email validation before processing.
While email normalization and hashing are implemented correctly, ensure that email validation is performed before these operations. This is particularly important since the validation imports were removed.
Let me search for email validation patterns in the codebase.
Let me check the implementation of email validation in the
isValidEmailfunction.
Let me check if the
isValidEmailfunction is being used in the signup process.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for email validation in the codebase rg -A 5 "validateEmail|email.*validation"Length of output: 46469
Script:
#!/bin/bash # Search for email validation patterns in the codebase ast-grep --pattern 'validateEmail($$$)' # Also search for any email validation regex or patterns rg -A 3 "email.*pattern|email.*regex|email.*validate"Length of output: 1515
Script:
#!/bin/bash # Check the implementation of isValidEmail function cat src/setup/isValidEmail.tsLength of output: 577
Script:
#!/bin/bash # Check if isValidEmail is imported and used in signUp.ts rg -A 5 "isValidEmail" src/resolvers/Mutation/signUp.ts # Also check the entire signUp.ts file for context cat src/resolvers/Mutation/signUp.tsLength of output: 5711
tests/resolvers/Mutation/blockPluginCreationBySuperadmin.spec.ts (1)
106-112: 🛠️ Refactor suggestion
Reduce code duplication in test data generation.
This block duplicates the email generation and encryption logic from the previous test case. Consider using the suggested
generateTestEmailhelper function here as well to maintain DRY principles and consistency across test cases.src/resolvers/Query/organizationsMemberConnection.ts (2)
165-166: 🛠️ Refactor suggestion
Refactor duplicated email handling logic
The email handling logic is duplicated between the paginated and non-paginated user mappings. Consider extracting this into a helper function:
// Add at the top of the file const mapUserWithDecryptedEmail = (user: InterfaceUser, context: any) => { const decryptUserEmail = (email: string) => { try { const decrypted = decryptEmail(email); return decrypted.decrypted; } catch (error) { console.error('Failed to decrypt user email', { userId: user._id }); return null; } }; return { // ... other user fields ... email: decryptUserEmail(user.email), hashedEmail: user.hashedEmail, // ... remaining user fields ... }; };Then use this helper in both mapping blocks:
users = usersModel.docs.map((user) => mapUserWithDecryptedEmail(user, context));
130-131:
⚠️ Potential issueAdd error handling for email decryption
The current implementation might throw runtime errors if email decryption fails. Consider implementing proper error handling:
- email: decryptEmail(user.email).decrypted, + email: (() => { + try { + const decrypted = decryptEmail(user.email); + return decrypted.decrypted; + } catch (error) { + // Log the error securely without exposing the email + console.error('Failed to decrypt user email', { userId: user._id }); + return null; // or handle the error case appropriately + } + })(), hashedEmail: user.hashedEmail,📝 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.email: (() => { try { const decrypted = decryptEmail(user.email); return decrypted.decrypted; } catch (error) { // Log the error securely without exposing the email console.error('Failed to decrypt user email', { userId: user._id }); return null; // or handle the error case appropriately } })(), hashedEmail: user.hashedEmail,tests/resolvers/Mutation/updateUserPassword.spec.ts (1)
41-47: 🛠️ Refactor suggestion
Consider adding specific tests for email encryption scenarios.
While the password update tests are comprehensive, consider adding test cases to verify:
- Email encryption/decryption roundtrip works correctly
- System handles encryption failures gracefully
- Hash comparison works as expected for email lookups
Would you like me to help generate these additional test cases?
tests/resolvers/Mutation/createMember.spec.ts (1)
207-212:
⚠️ Potential issueTest case needs expansion to cover email encryption functionality.
While the test successfully creates a user with encrypted and hashed email, it has several gaps in coverage:
- The test title doesn't reflect that it's also testing email encryption
- There's no verification that the stored encrypted email can be decrypted
- The test doesn't assert that both encrypted and hashed emails are stored correctly
Consider expanding the test or adding a new test case:
const email = `email2${nanoid().toLowerCase()}@gmail.com`; const hashedEmail = hashEmail(email) const testUser2 = await User.create({ email: encryptEmail(email), hashedEmail, password: `pass2${nanoid().toLowerCase()}`, firstName: `firstName2${nanoid().toLowerCase()}`, lastName: `lastName2${nanoid().toLowerCase()}`, image: null, appLanguageCode: "en", }); + +// Verify email encryption and hashing +const createdUser = await User.findById(testUser2._id); +expect(createdUser.hashedEmail).toBe(hashedEmail); +const decryptedEmail = decryptEmail(createdUser.email); +expect(decryptedEmail).toBe(email);📝 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 email = `email2${nanoid().toLowerCase()}@gmail.com`; const hashedEmail = hashEmail(email) const testUser2 = await User.create({ email: encryptEmail(email), hashedEmail, password: `pass2${nanoid().toLowerCase()}`, firstName: `firstName2${nanoid().toLowerCase()}`, lastName: `lastName2${nanoid().toLowerCase()}`, image: null, appLanguageCode: "en", }); // Verify email encryption and hashing const createdUser = await User.findById(testUser2._id); expect(createdUser.hashedEmail).toBe(hashedEmail); const decryptedEmail = decryptEmail(createdUser.email); expect(decryptedEmail).toBe(email);src/utilities/createSampleOrganizationUtil.ts (2)
44-51: 🛠️ Refactor suggestion
Enhance email generation for more realistic test data.
Several improvements could make the email generation more robust:
- Use faker's email generator for more realistic domains
- Add validation for generated email format
- Add error handling for encryption/hashing operations
- Consider adding uniqueness check
- const email = `${fname.toLowerCase()}${lname.toLowerCase()}@${faker.helpers.arrayElement([ - "xyz", - "abc", - "lmnop", - ])}.com` + const email = faker.internet.email({ + firstName: fname, + lastName: lname, + provider: faker.internet.domainName(), + }); + + // Validate email format + if (!email.match(/^[^\s@]+@[^\s@]+\.[^\s@]+$/)) { + throw new Error(`Invalid email format generated: ${email}`); + } - const hashedEmail = hashEmail(email); + let hashedEmail; + try { + hashedEmail = hashEmail(email); + } catch (error) { + throw new Error(`Failed to hash email: ${error.message}`); + }📝 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 email = faker.internet.email({ firstName: fname, lastName: lname, provider: faker.internet.domainName(), }); // Validate email format if (!email.match(/^[^\s@]+@[^\s@]+\.[^\s@]+$/)) { throw new Error(`Invalid email format generated: ${email}`); } let hashedEmail; try { hashedEmail = hashEmail(email); } catch (error) { throw new Error(`Failed to hash email: ${error.message}`); }
55-56:
⚠️ Potential issueAdd error handling and documentation for email encryption.
The email encryption process needs better error handling and documentation to explain the dual storage approach.
+ let encryptedEmail; + try { + encryptedEmail = encryptEmail(email); + } catch (error) { + throw new Error(`Failed to encrypt email: ${error.message}`); + } + const user = new User({ firstName: fname, lastName: lname, - email: encryptEmail(email), - hashedEmail: hashedEmail, + // Store encrypted email for retrieval and hashed email for efficient lookups + email: encryptedEmail, + hashedEmail, password: "$2a$12$bSYpay6TRMpTOaAmYPFXku4avwmqfFBtmgg39TabxmtFEiz4plFtW",📝 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.let encryptedEmail; try { encryptedEmail = encryptEmail(email); } catch (error) { throw new Error(`Failed to encrypt email: ${error.message}`); } // Store encrypted email for retrieval and hashed email for efficient lookups email: encryptedEmail, hashedEmail,tests/resolvers/Mutation/login.spec.ts (1)
272-274: 🛠️ Refactor suggestion
Consider refactoring repeated null checks.
The null checks for
testUserare duplicated across multiple test cases. Consider extracting this into a helper function or using a beforeEach hook.+ function ensureTestUser(): void { + if (!testUser) { + throw new Error("Error creating test user."); + } + } it("throws ValidationError if password is incorrect", async () => { - if (!testUser) { - throw new Error("Error creating Test User."); - } + ensureTestUser(); // ... rest of the test });Also applies to: 298-300, 307-309, 354-356
tests/resolvers/Query/usersConnection.spec.ts (1)
61-65:
⚠️ Potential issueStrengthen assertions and add encryption-specific test cases
The current changes have significantly weakened the test coverage:
- Basic existence checks (
toBeDefined()) replace detailed equality assertions- No validation of email encryption/decryption
- Missing verification of email search functionality with encrypted data
Enhance the assertions to include:
// Example for the first test case expect(usersPayload).toBeDefined(); expect(Array.isArray(usersPayload)).toBe(true); // Verify email encryption expect(usersPayload.every(user => { const decryptedEmail = decryptEmail(user.email); return decryptedEmail === users.find(u => u._id === user._id)?.email; })).toBe(true);Also applies to: 109-112, 172-173, 230-231, 291-292, 350-351, 395-396, 431-432, 467-468, 502-503
tests/resolvers/Query/users.spec.ts (2)
262-262: 🛠️ Refactor suggestion
Consider optimizing repeated email decryption logic.
The email decryption is consistently implemented across all test cases, but the pattern is repeated multiple times. Consider extracting this transformation into a helper function to improve maintainability.
+ function transformUserForTest(user) { + return { + ...user, + email: decryptEmail(user.email).decrypted, + organizationsBlockedBy: [], + image: user.image ? `${BASE_URL}${user.image}` : null, + }; + } users = users.map((user) => ({ - ...user, - email: decryptEmail(user.email).decrypted, - organizationsBlockedBy: [], - image: user.image ? `${BASE_URL}${user.image}` : null, + ...transformUserForTest(user) }));Also applies to: 298-298, 348-348, 409-409, 468-468, 528-528, 593-593, 645-645, 685-685, 723-723, 772-772
86-122: 🛠️ Refactor suggestion
Consider refactoring test data setup for better maintainability.
The current implementation correctly handles both encryption and hashing but contains repetitive code. Consider refactoring this into a helper function.
+ function createTestEmail() { + const email = `email${nanoid().toLowerCase()}@gmail.com`; + return { + email: encryptEmail(email), + hashedEmail: hashEmail(email), + rawEmail: email // useful for test assertions + }; + } - const email1 = `email${nanoid().toLowerCase()}@gmail.com`; - const email2 = `email${nanoid().toLowerCase()}@gmail.com`; - const email3 = `email${nanoid().toLowerCase()}@gmail.com`; - const email4 = `email${nanoid().toLowerCase()}@gmail.com`; - const email5 = `email${nanoid().toLowerCase()}@gmail.com`; + const testEmails = Array.from({ length: 5 }, () => createTestEmail()); testUsers = await User.insertMany([ { - email: encryptEmail(email1), - hashedEmail: hashEmail(email1), + ...testEmails[0], password: "password", firstName: `firstName${nanoid()}`, lastName: `lastName${nanoid()}`, }, // Repeat for other users using testEmails[1] through testEmails[4]Committable suggestion was skipped due to low confidence.
tests/resolvers/Query/organizationsMemberConnection.spec.ts (1)
236-236: 🛠️ Refactor suggestion
Refactor repeated decryption logic and add error handling.
The email decryption logic is duplicated across multiple test cases. Consider:
- Creating a helper function to handle user mapping with decryption
- Adding error handling for decryption failures
Create a helper function at the top of the test file:
const mapUserWithDecryptedEmail = (user: any) => { try { const decryptedData = decryptEmail(user.email); return { _id: user._id, appUserProfileId: user.appUserProfileId, address: user.address, birthDate: user.birthDate, createdAt: user.createdAt, educationGrade: user.educationGrade, email: decryptedData.decrypted, employmentStatus: user.employmentStatus, firstName: user.firstName, gender: user.gender, image: user.image || null, joinedOrganizations: user.joinedOrganizations, lastName: user.lastName, maritalStatus: user.maritalStatus, membershipRequests: user.membershipRequests, organizationsBlockedBy: user.organizationsBlockedBy, password: null, phone: user.phone, registeredEvents: user.registeredEvents, status: user.status, updatedAt: user.updatedAt, }; } catch (error) { throw new Error(`Failed to decrypt email: ${error.message}`); } };Then replace all instances of the user mapping with:
const usersWithPassword = users.map(mapUserWithDecryptedEmail);Also applies to: 328-328, 419-419, 513-513, 607-607, 689-689, 870-870, 941-941, 1041-1041, 1122-1122
src/resolvers/Organization/members.ts (1)
1-1:
⚠️ Potential issueUse type-only import for
InterfaceUserThe
InterfaceUseris only used as a type in this file. To comply with TypeScript best practices and linting rules, it should be imported using a type-only import.Apply this diff to fix the import statement:
-import { InterfaceUser, User } from "../../models"; +import { User } from "../../models"; +import type { InterfaceUser } from "../../models";📝 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.import { User } from "../../models"; import type { InterfaceUser } from "../../models";🧰 Tools
🪛 GitHub Check: Check for linting, formatting, and type errors
[failure] 1-1:
Imports "InterfaceUser" are only used as type🪛 eslint
[error] 1-1: Imports "InterfaceUser" are only used as type.
(@typescript-eslint/consistent-type-imports)
src/resolvers/Organization/blockedUsers.ts (2)
32-41:
⚠️ Potential issueHandle decryption failures to avoid returning encrypted emails.
When email decryption fails, the code logs the error but returns the user object with the encrypted email. This could result in inconsistent data being sent to the client. Consider returning the user with the email set to
nullto indicate that the email is unavailable.Apply this diff to set the email to
nullwhen decryption fails:} catch (error) { console.error( `Failed to decrypt email for user ${blockedUser._id}:`, error, ); - return blockedUser; + return { ...blockedUser, email: null }; }📝 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.try { const { decrypted } = decryptEmail(blockedUser.email); return { ...blockedUser, email: decrypted }; } catch (error) { console.error( `Failed to decrypt email for user ${blockedUser._id}:`, error, ); return { ...blockedUser, email: null }; }
1-1:
⚠️ Potential issueUse type-only import for
InterfaceUserto comply with TypeScript best practices.The
InterfaceUseris only used as a type annotation. Adjusting the import to a type-only import enhances code clarity and complies with the linting rule@typescript-eslint/consistent-type-imports.Apply this diff to fix the issue:
-import { InterfaceUser, User } from "../../models"; +import { User } from "../../models"; +import type { InterfaceUser } from "../../models";📝 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.import { User } from "../../models"; import type { InterfaceUser } from "../../models";🧰 Tools
🪛 GitHub Check: Check for linting, formatting, and type errors
[failure] 1-1:
Imports "InterfaceUser" are only used as type🪛 eslint
[error] 1-1: Imports "InterfaceUser" are only used as type.
(@typescript-eslint/consistent-type-imports)
src/resolvers/Mutation/forgotPassword.ts (1)
11-11:
⚠️ Potential issueRemove unused import 'crypto' module.
The 'crypto' module is imported but not used anywhere in the file. This can be safely removed to clean up the code.
Apply this diff to remove the unused import:
-import crypto from "crypto";📝 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.🧰 Tools
🪛 GitHub Check: Check for linting, formatting, and type errors
[failure] 11-11:
'crypto' is defined but never used🪛 eslint
[error] 11-11: 'crypto' is defined but never used.
(@typescript-eslint/no-unused-vars)
src/resolvers/Mutation/login.ts (1)
27-28:
⚠️ Potential issueValidate email input before hashing
Before hashing the email, consider adding validation to ensure that
args.data.emailis neitherundefinednornull. This prevents potential errors during the hashing process.Apply this change to add email validation:
+ if (!args.data.email) { + throw new errors.ValidationError( + [ + { + message: 'Email is required', + code: 'EMAIL_REQUIRED', + param: 'email', + }, + ], + 'Email is required', + ); + } const hashedEmail = hashEmail(args.data.email);📝 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.if (!args.data.email) { throw new errors.ValidationError( [ { message: 'Email is required', code: 'EMAIL_REQUIRED', param: 'email', }, ], 'Email is required', ); } const hashedEmail = hashEmail(args.data.email);src/utilities/loadSampleData.ts (2)
122-122:
⚠️ Potential issueFix TSDoc syntax error in
@throwstagThe TSDoc parser reports a syntax error due to an unescaped "}" character in the
@throwstag. To resolve this, escape the "}" character with a backslash.Apply this diff to fix the issue:
- * @throws {Error} If encryption or hashing fails + * @throws \{Error\} If encryption or hashing fails📝 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.* @throws \{Error\} If encryption or hashing fails🧰 Tools
🪛 eslint
[error] 122-122: tsdoc-escape-right-brace: The "}" character should be escaped using a backslash to avoid confusion with a TSDoc inline tag
(tsdoc/syntax)
135-135:
⚠️ Potential issueAvoid logging potentially sensitive error information
Logging the entire error object may inadvertently expose sensitive information. Consider logging only the error message or a sanitized version of the error.
Apply this diff to modify the error logging:
- console.error(`Failed to process email for user: ${error}`); + console.error(`Failed to process email for user ID ${user.id}: ${error.message}`);Committable suggestion was skipped due to low confidence.
src/models/User.ts (3)
249-249:
⚠️ Potential issueUse 'next(new Error(...))' instead of 'throw' in Mongoose middleware
In Mongoose middleware, errors should be passed to the
nextfunction rather than thrown. Throwing an error may bypass Mongoose's error handling mechanisms and lead to unexpected behavior.Modify the error handling as follows:
- throw new Error('HASH_PEPPER environment variable is not configured'); + return next(new Error('HASH_PEPPER environment variable is not configured'));- throw new Error('The provided email address is invalid'); + return next(new Error('The provided email address is invalid'));- throw new Error(`Email validation failed: ${errorMessage}`); + return next(new Error(`Email validation failed: ${errorMessage}`));Also applies to: 255-255, 262-262
12-12:
⚠️ Potential issueUnused import 'crypto'
The
cryptomodule is imported but never used. Consider removing it to keep the code clean.Apply this diff to remove the unused import:
- import crypto from 'crypto'📝 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.🧰 Tools
🪛 GitHub Check: Check for linting, formatting, and type errors
[failure] 12-12:
'crypto' is defined but never used
261-262: 🛠️ Refactor suggestion
Use 'errorMessage' variable to enhance error reporting
The variable
errorMessageis assigned but not used. Incorporate it into the thrown error to provide more detailed information, which can aid in debugging.Apply this diff to include
errorMessagein the error message:const errorMessage = error instanceof Error ? error.message : 'Unknown error'; - throw new Error('Email validation failed. Please ensure the email is valid.'); + throw new Error(`Email validation failed: ${errorMessage}`);📝 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 errorMessage = error instanceof Error ? error.message : 'Unknown error'; throw new Error(`Email validation failed: ${errorMessage}`);🧰 Tools
🪛 GitHub Check: Check for linting, formatting, and type errors
[failure] 261-261:
'errorMessage' is assigned a value but never used🪛 eslint
[error] 261-261: 'errorMessage' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
setup.ts (3)
1-1:
⚠️ Potential issueRemove duplicate imports of the 'crypto' module
The 'crypto' module is imported twice, once as
cryptolibon line 1 and again ascryptoon line 18. This duplication can cause confusion and is unnecessary. Consider consolidating these imports into a single import statement and ensure consistent usage throughout the code.Apply this diff to remove the duplicate import:
- import * as cryptolib from "crypto";If
cryptolibis used in the code, replace its usage withcryptoor vice versa to maintain consistency.Also applies to: 18-18
391-393:
⚠️ Potential issueAdjust regex validation to be case-insensitive for hexadecimal keys
The regex used to validate the
ENCRYPTION_KEYandHASH_PEPPERonly allows lowercase hexadecimal characters. Hexadecimal strings can contain both uppercase and lowercase characters. Modify the regex to be case-insensitive to accept valid keys regardless of character case.Apply this diff to update the regex patterns:
- if (!/^[a-f0-9]{64}$/.test(process.env.ENCRYPTION_KEY)) { + if (!/^[a-f0-9]{64}$/i.test(process.env.ENCRYPTION_KEY)) {- if (!/^[a-f0-9]{64}$/.test(process.env.HASH_PEPPER)) { + if (!/^[a-f0-9]{64}$/i.test(process.env.HASH_PEPPER)) {Also applies to: 412-414
405-406:
⚠️ Potential issueEnsure the setup process halts on critical failures
In the
catchblocks ofsetEncryptionKeyandsetHashPepper, errors are logged but the setup process continues. If generating or setting the encryption key or hash pepper fails, it's crucial to halt the setup to prevent running with invalid or insecure configurations. Consider callingabort()after logging the error to stop the process.Apply this diff to modify the error handling:
} catch (err) { console.error("An error occurred:", err); + abort(); }Also applies to: 426-427
src/utilities/encryption.ts (6)
10-13:
⚠️ Potential issueFix TSDoc
@returnssyntax errorsThe TSDoc comments in the functions
generateRandomIV,encryptEmail,decryptEmail, andisValidHexinclude types in braces in the@returnstags, which is not standard in TSDoc and causes syntax errors identified by static analysis tools.Apply this diff to fix the TSDoc comments:
- * @returns {string} A randomly generated IV as a hexadecimal string. + * @returns A randomly generated IV as a hexadecimal string. - * @returns {string} The encrypted email in the format "iv:authTag:encryptedData". + * @returns The encrypted email in the format "iv:authTag:encryptedData". - * @returns {object} An object containing the decrypted email. + * @returns An object containing the decrypted email. - * @returns {boolean} True if the string is valid hexadecimal, false otherwise. + * @returns True if the string is valid hexadecimal, false otherwise.Also applies to: 18-23, 50-55, 112-116
🧰 Tools
🪛 GitHub Check: Check for linting, formatting, and type errors
[failure] 12-12:
tsdoc-malformed-inline-tag: Expecting a TSDoc tag starting with "{@"
[failure] 12-12:
tsdoc-escape-right-brace: The "}" character should be escaped using a backslash to avoid confusion with a TSDoc inline tag
27-33:
⚠️ Potential issueValidate that
ENCRYPTION_KEYis a valid hexadecimal stringIn both
encryptEmailanddecryptEmailfunctions, we check thatENCRYPTION_KEYis defined and has a length of 64 characters but do not validate that it contains only hexadecimal characters. This could lead to runtime errors ifENCRYPTION_KEYincludes invalid characters.Apply this diff to include validation of
ENCRYPTION_KEY:} else if ( encryptionKey.length !== 64 + || !isValidHex(encryptionKey) ) { throw new Error( - "Encryption key must be a 256-bit hexadecimal string (64 characters).", + "Encryption key must be a 256-bit hexadecimal string (64 hex characters).", ); }Also applies to: 81-87
25-33:
⚠️ Potential issueResolve inconsistency between
ENCRYPTION_KEYformatThe code expects
ENCRYPTION_KEYto be a 256-bit hexadecimal string (64 characters), but the environment variable documentation indicates it should be a Base64-encoded 32-byte key. This inconsistency may cause configuration errors.Consider updating the code to handle a Base64-encoded key, or adjust the documentation to specify that a hexadecimal string is required. If you opt for Base64 encoding, modify the
Buffer.fromcalls and adjust validations:- Buffer.from(encryptionKey, "hex"), + Buffer.from(encryptionKey, "base64"),And update the validation:
- } else if ( - encryptionKey.length !== 64 - || !isValidHex(encryptionKey) - ) { + } else { + const keyBuffer = Buffer.from(encryptionKey, 'base64'); + if (keyBuffer.length !== 32) { throw new Error( - "Encryption key must be a 256-bit hexadecimal string (64 hex characters).", + "Encryption key must be a 256-bit key encoded in base64 (32 bytes).", ); + } }Ensure consistency between the code and documentation to prevent potential misconfigurations.
Also applies to: 79-87
56-58: 🛠️ Refactor suggestion
Simplify the return type of
decryptEmailThe
decryptEmailfunction returns an object{ decrypted: string }, but since it only returns one value, it might be simpler to return the decrypted string directly.Update the function signature and return statement:
- export function decryptEmail(encryptedData: string): { - decrypted: string; - } { + export function decryptEmail(encryptedData: string): string { ... - return { decrypted }; + return decrypted;This simplifies the usage of the function and aligns with common practice.
Also applies to: 109-110
66-66:
⚠️ Potential issueValidate the format of
encryptedDataAfter splitting
encryptedData, ensure that it results in exactly three components (iv,authTagHex, andencryptedHex). IfencryptedDatadoes not contain exactly two colons, it may cause unexpected behavior.Add a validation check:
const [iv, authTagHex, encryptedHex] = encryptedData.split(":"); + if (!iv || !authTagHex || !encryptedHex) { + throw new Error("Invalid encrypted data format: expected 'iv:authTag:encryptedData'."); + }📝 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 [iv, authTagHex, encryptedHex] = encryptedData.split(":"); if (!iv || !authTagHex || !encryptedHex) { throw new Error("Invalid encrypted data format: expected 'iv:authTag:encryptedData'."); }
42-47:
⚠️ Potential issueAdd error handling for the encryption process
The
encryptEmailfunction does not handle potential errors during encryption. Ifcipher.updateorcipher.finalthrows an error, it will not be caught.Wrap the encryption process in a try-catch block:
+ let encrypted; + try { encrypted = Buffer.concat([ cipher.update(email, "utf8"), cipher.final(), ]); + } catch (err) { + throw new Error("Encryption failed: " + err.message); + }Committable suggestion was skipped due to low confidence.
|
I did reset-hard to "fixing import errors" as in my last push i removed eslint-disable in setup.ts to resolve eslint checks, i can't check my changes until and unless I resolve these checks and these eslint-disable lines were already present in setup.ts, they have not been added in any of the 20 commits how can i proceed? |
|
also please note, I will resume my work on this in 3-4 days as festive season is going on |
|
@pranshugupta54 does everything look good now, there are no comments by coderrabit ai, also it would be great if you could guide me with resolving linting errors |
|
please fix conflicting files first |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
tests/utilities/encryptionModule.spec.ts (1)
33-84: Add type checking and error message validation tests.While the current test coverage is good, consider adding these additional test cases:
- Verify exact error message strings
- Type checking for the decrypted result object
it("should return properly typed decryption result", () => { const email = "[email protected]"; const encrypted = encryptEmail(email); const result = decryptEmail(encrypted); expect(result).toHaveProperty('decrypted'); expect(typeof result.decrypted).toBe('string'); }); it("should throw specific error messages", () => { expect(() => encryptEmail(null as any)).toThrow("Empty or invalid email input"); expect(() => encryptEmail(undefined as any)).toThrow("Empty or invalid email input"); expect(() => encryptEmail(123 as any)).toThrow("Empty or invalid email input"); });setup.ts (3)
373-425: Consider refactoring encryption key setup functions to reduce duplication.The
setEncryptionKeyandsetHashPepperfunctions share almost identical logic. Consider extracting the common functionality into a reusable helper function.+function setupSecureKey( + keyType: 'ENCRYPTION_KEY' | 'HASH_PEPPER', + envVar: string | undefined +): void { + try { + if (envVar) { + if (!/^[a-f0-9]{64}$/i.test(envVar)) { + throw new Error(`Existing ${keyType.toLowerCase()} has invalid format`); + } + console.log(`\n ${keyType} is already present.`); + } else { + const newKey = crypto.randomBytes(32).toString("hex"); + process.env[keyType] = newKey; + updateEnvVariable({ [keyType]: newKey }); + console.log(`\n ${keyType} set successfully.`); + } + } catch (err) { + console.error("An error occurred:", err); + abort(); + } +} +export async function setEncryptionKey(): Promise<void> { + setupSecureKey('ENCRYPTION_KEY', process.env.ENCRYPTION_KEY); +} +export async function setHashPepper(): Promise<void> { + setupSecureKey('HASH_PEPPER', process.env.HASH_PEPPER); +}
Line range hint
171-198: Add timeout mechanism to prevent infinite loops.While the
while(true)loop with validation is good, consider adding a maximum retry limit to prevent infinite loops in case of persistent issues.async function askForTransactionLogPath(): Promise<string> { let logPath: string | null; + const MAX_RETRIES = 3; + let attempts = 0; // Keep asking for path, until user gives a valid path - while (true) { + while (attempts < MAX_RETRIES) { + attempts++; const response = await inquirer.prompt([ { type: "input", name: "logPath", message: "Enter absolute path of log file:", default: null, }, ]); logPath = response.logPath; if (logPath && fs.existsSync(logPath)) { try { fs.accessSync(logPath, fs.constants.R_OK | fs.constants.W_OK); break; } catch { console.error( "The file is not readable/writable. Please enter a valid file path.", ); } } else { console.error( "Invalid path or file does not exist. Please enter a valid file path.", ); } } + if (attempts >= MAX_RETRIES) { + console.error("Maximum retry attempts reached. Using default log path."); + return path.resolve(__dirname, "logs", "transaction.log"); + } return logPath; }
Line range hint
1030-1053: Maintain consistent message capitalization in prompts.The word "Sample" is capitalized in some prompts but not in others:
- Line 1030: "default data" (not capitalized)
- Line 1040: "sample data" (not capitalized)
- Line 1053: "Sample data" (capitalized)
Consider standardizing the capitalization across all prompts.
- message: "Do you want to import default data?", + message: "Do you want to import Default data?", - message: "Do you want to import sample data?", + message: "Do you want to import Sample data?", - message: "Do you want to import Sample data?", + message: "Do you want to import Sample data?",
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
setup.ts(13 hunks)tests/utilities/encryptionModule.spec.ts(1 hunks)tests/utilities/hashingModule.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/utilities/hashingModule.spec.ts
🔇 Additional comments (3)
tests/utilities/encryptionModule.spec.ts (2)
1-8: LGTM! Clean and focused imports.
The test file correctly imports the necessary testing utilities and encryption functions.
1-85: Verify encryption format compatibility with database storage.
The tests validate the encryption functionality but we should verify that the encrypted format (hex strings with colons) is compatible with the database schema and indexing requirements.
✅ Verification successful
Based on the search results, I can now provide a definitive response about the encryption format compatibility with the database:
Encrypted email format is compatible with database storage requirements
The codebase analysis reveals:
- The User model stores emails in two forms:
email(encrypted) andhashedEmail(for lookups) - The encrypted format follows a consistent pattern:
iv:authTag:encryptedData(hex strings with colons) - No explicit length constraints are enforced on these fields in the database schema
- The encryption implementation is already being used successfully throughout the codebase for database operations
The tests verify that:
- The encryption produces valid hex characters (
/^[0-9a-f]+:[0-9a-f]+:[0-9a-f]+$/i) - Long email inputs are handled gracefully (tested up to 10,000 characters)
- The format is consistently maintained across all database operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check database schema and related files for encryption compatibility
# Check schema definitions
echo "Checking schema files for email field definitions..."
rg -l "email|encrypted|hashed" --type=graphql
# Check database models
echo "Checking model files for email field handling..."
rg -l "email|encrypted|hashed" --type=typescript --glob="**/models/**"
# Check maximum field lengths
echo "Checking for field length constraints..."
rg "length|maxLength|size" --type=typescript --glob="**/models/**"
Length of output: 604
Script:
#!/bin/bash
# Let's try different search patterns without type constraints
# Check for schema files and their content
echo "Checking for schema files..."
fd "schema|\.graphql|\.gql"
echo "\nChecking for model definitions..."
fd "model|entity" -e ts -e js
echo "\nSearching for email field definitions..."
rg -A 5 "email.*string|email.*varchar|email.*text"
echo "\nChecking for encrypted email patterns..."
rg -A 5 "encrypted.*email|hashed.*email"
echo "\nLooking for database field constraints..."
rg -A 5 "column.*email|@Column.*email"
Length of output: 36734
setup.ts (1)
Line range hint 761-777: LGTM! Improved user prompts.
The changes to the token generation prompts provide better context and sensible defaults based on existing environment variables.
🧰 Tools
🪛 Biome
[error] 762-763: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
| it("should handle malformed encrypted data gracefully", () => { | ||
| expect(() => decryptEmail("invalid:format")).toThrow(); | ||
| expect(() => decryptEmail("invalid:format:data")).toThrow(); | ||
| expect(() => decryptEmail("::::")).toThrow(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move malformed data test to appropriate describe block.
The test for handling malformed encrypted data should be moved to the "encryptEmail and decryptEmail" describe block since it's testing decryptEmail functionality, not generateRandomIV.
- describe("generateRandomIV", () => {
- // ... other generateRandomIV tests ...
-
- it("should handle malformed encrypted data gracefully", () => {
- expect(() => decryptEmail("invalid:format")).toThrow();
- expect(() => decryptEmail("invalid:format:data")).toThrow();
- expect(() => decryptEmail("::::")).toThrow();
- });
- });
+ describe("generateRandomIV", () => {
+ // ... other generateRandomIV tests ...
+ });
+
+ describe("encryptEmail and decryptEmail", () => {
+ // ... at the beginning of this block ...
+ it("should handle malformed encrypted data gracefully", () => {
+ expect(() => decryptEmail("invalid:format")).toThrow();
+ expect(() => decryptEmail("invalid:format:data")).toThrow();
+ expect(() => decryptEmail("::::")).toThrow();
+ });Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
.github/workflows/pull-request.yml (1)
221-223: Consider adding validation for encryption key format.Since email encryption is a critical security feature, consider adding a step to validate the format and strength of the encryption key during CI.
Example validation step to add after the environment variables:
ENCRYPTION_KEY: ${{secrets.ENCRYPTION_KEY}} HASH_PEPPER: ${{secrets.HASH_PEPPER}} + - name: Validate encryption key format + run: | + if [[ ${#ENCRYPTION_KEY} -lt 32 ]]; then + echo "Error: ENCRYPTION_KEY must be at least 32 characters long" + exit 1 + fi
.github/workflows/pull-request.yml
Outdated
| ENCRYPTION_KEY: ${{secrets.ENCRYPTION_KEY}} | ||
| HASH_PEPPER: ${{secrets.HASH_PEPPER}} | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Environment variables properly configured, but documentation needed
The verification shows:
.env.sampleincludes both variables with appropriate placeholders- GraphQL schema confirms email encryption implementation with
hashedEmailfield andencryptionboolean - No setup documentation found for the new encryption variables
Please add documentation in a README.md or docs folder explaining:
- Purpose and format of
ENCRYPTION_KEYandHASH_PEPPER - Steps to generate secure values for these variables
- How to configure them in GitHub secrets
🔗 Analysis chain
Verify GitHub secrets configuration for encryption.
The new environment variables for email encryption look good. However, please ensure:
- These secrets are properly configured in the GitHub repository settings
- Documentation is updated to explain how to set up these secrets
- The
.env.samplefile includes these variables with appropriate descriptions
Let's verify the integration points:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if .env.sample has been updated with the new variables
echo "Checking .env.sample for new variables..."
rg "ENCRYPTION_KEY|HASH_PEPPER" .env.sample
# Check if documentation has been updated
echo "Checking documentation for setup instructions..."
rg -i "encryption.*key|hash.*pepper" "*.md"
Length of output: 411
Script:
#!/bin/bash
# Check for documentation in docs directory and other common locations
echo "Checking for documentation in various locations..."
fd -e md -x rg -i "encryption.*key|hash.*pepper" {}
# Check for setup instructions in repository
echo "Checking for setup related files..."
fd -g "*setup*" -g "*install*" -g "*config*" -x rg -i "encryption.*key|hash.*pepper" {}
# Look for any GraphQL schema changes related to email encryption
echo "Checking GraphQL schema..."
fd -g "schema.graphql" -x cat {}
Length of output: 46113
Script:
#!/bin/bash
# Check for email-related fields and types in GraphQL schema
echo "Checking for email-related fields in User type..."
rg -A 5 -B 5 "email.*:" schema.graphql
# Check for setup/config files that might need encryption key configuration
echo "Checking for setup and config files..."
fd -g "*setup*" -g "*config*" -g "*.env*" -x cat {}
# Look for any email-related mutations or types
echo "Checking for email-related mutations and types..."
rg -i "email|encrypt" schema.graphql
Length of output: 2317
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/pull-request.yml(1 hunks)
🧰 Additional context used
🪛 Gitleaks
.github/workflows/pull-request.yml
221-221: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
| ENCRYPTION_KEY: 64730e71158b84687f01237d8f8128cc9cb7804d2d68c36823880456adad48c7 | ||
| HASH_PEPPER: 56195a1bd9b062fc4a63afff383ec28bf1464706725ae744c9fe7fc459426074 |
There was a problem hiding this comment.
💡 Codebase verification
Based on the evidence gathered, I can now generate the final response:
The hardcoded encryption keys in the workflow file pose a security risk. The codebase shows proper encryption key handling:
.env.sampledemonstrates proper configuration with emptyENCRYPTION_KEYandHASH_PEPPERvariablessrc/utilities/encryption.tsvalidates key formats and handles them securely- No other instances of these hardcoded values were found in the codebase
Please make these changes:
- Remove hardcoded values from the workflow file
- Use GitHub Actions secrets instead:
ENCRYPTION_KEY: ${{ secrets.TEST_ENCRYPTION_KEY }}
HASH_PEPPER: ${{ secrets.TEST_HASH_PEPPER }}- Generate new test-specific keys for the GitHub secrets (do not reuse these exposed values)
🔗 Analysis chain
CRITICAL: Remove hardcoded cryptographic keys from workflow file
Exposing cryptographic keys in version control poses a severe security risk, even for test environments. These keys should be managed through GitHub secrets.
Please make the following changes:
- Remove the hardcoded values
- Use GitHub secrets instead:
- ENCRYPTION_KEY: 64730e71158b84687f01237d8f8128cc9cb7804d2d68c36823880456adad48c7
- HASH_PEPPER: 56195a1bd9b062fc4a63afff383ec28bf1464706725ae744c9fe7fc459426074
+ ENCRYPTION_KEY: ${{ secrets.TEST_ENCRYPTION_KEY }}
+ HASH_PEPPER: ${{ secrets.TEST_HASH_PEPPER }}- Set up the corresponding secrets in your repository:
- Go to Settings → Secrets and variables → Actions
- Add
TEST_ENCRYPTION_KEYandTEST_HASH_PEPPERwith test-specific values - Ensure these test keys are different from production keys
Consider implementing a key rotation mechanism and documenting the process for generating and managing these cryptographic materials securely.
Let's verify if these keys are used elsewhere in the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential hardcoded keys
echo "Checking for exposed keys in the codebase..."
rg -i "64730e71158b84687f01237d8f8128cc9cb7804d2d68c36823880456adad48c7|56195a1bd9b062fc4a63afff383ec28bf1464706725ae744c9fe7fc459426074"
Length of output: 238
Script:
#!/bin/bash
# Let's check for any environment files or configuration files that might contain these keys
echo "Checking for environment and config files..."
fd -e env -e env.example -e env.sample -e config -e yml -e yaml
# Let's also check for any encryption-related code to understand the usage
echo -e "\nChecking for encryption-related code..."
rg -i "encryption|encrypt|crypto" -A 3 -B 3
# Check for any test files that might be using these keys
echo -e "\nChecking test files..."
fd -e test.js -e spec.js -e test.ts -e spec.ts
Length of output: 114823
Script:
#!/bin/bash
# Let's check if these keys are used in any environment files or configuration
echo "Checking environment files..."
fd -e env -e env.example -e env.sample -e env.test -x cat {} \; 2>/dev/null
# Let's also check for any encryption key setup or initialization code
echo -e "\nChecking encryption key setup code..."
ast-grep --pattern 'ENCRYPTION_KEY = $_' -l
# Let's check for any test setup files that might be configuring these keys
echo -e "\nChecking test setup files..."
rg -l "ENCRYPTION_KEY|HASH_PEPPER" "tests/setup" "tests/helpers"
Length of output: 516
Script:
#!/bin/bash
# Let's check for any test environment setup files
echo "Checking test environment files..."
find . -type f -name ".env.test" -o -name ".env.example" -o -name ".env.sample" -exec cat {} \;
# Let's check for encryption key initialization in setup code
echo -e "\nChecking encryption setup in code..."
rg -A 5 -B 5 "process\.env\.ENCRYPTION_KEY" src/
# Let's check for test configuration
echo -e "\nChecking test configuration..."
rg -A 5 -B 5 "ENCRYPTION_KEY.*=.*64730e71" .
rg -A 5 -B 5 "HASH_PEPPER.*=.*56195a1b" .
Length of output: 6330
🧰 Tools
🪛 Gitleaks
221-221: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (14)
tests/resolvers/Mutation/forgotPassword.spec.ts (3)
Line range hint
68-108: Test implementation doesn't match its description.The test description states it "throws an error when the email is changed in the token", but the implementation doesn't actually modify the email in the token. To properly test email tampering, you should create a token with one email and then modify it to use a different email.
Here's a suggested implementation:
it("throws an error when the email is changed in the token", async () => { const otp = "correctOtp"; const hashedOtp = await bcrypt.hash(otp, 1); + // Create token with original email const otpToken = jwt.sign( { - email: testUser?.email ?? "", + email: "[email protected]", // Use a different email to simulate tampering otp: hashedOtp, }, process.env.NODE_ENV ?? "", { expiresIn: 99999999, }, ); const oldPassword = testUser?.password; const args: MutationForgotPasswordArgs = { data: { newPassword: oldPassword!, otpToken, userOtp: otp, }, }; try { await forgotPasswordResolver?.({}, args, {}); throw new Error("Should not reach this point"); } catch (error: unknown) { if (error instanceof Error) { expect(error.message).toEqual(INVALID_OTP); } } });
168-176: Simplify email decryption logic and add error handling.While the implementation works, it could be more concise and robust.
Consider this improvement:
- let email = ""; - - if (testUser?.email) { - email = decryptEmail(testUser.email).decrypted; - } - + const email = testUser?.email + ? decryptEmail(testUser.email).decrypted + : throw new Error("Test user email is required"); const otpToken = jwt.sign( { email,This change:
- Uses a more concise conditional expression
- Fails fast if email is missing, which shouldn't happen in a test environment
Line range hint
33-203: Consider adding more security-focused test cases.Given that this is a security-critical feature handling encrypted emails, consider adding these test cases:
- Test with malformed encrypted email format
- Test with expired OTP token
- Test with invalid encryption key format
Would you like me to provide example implementations for these test cases?
tests/resolvers/Mutation/updateCommunity.spec.ts (1)
107-109: Consider extracting email generation to a test helper.The email generation logic could be moved to a helper function to maintain DRY principles across test files and ensure consistent test data generation.
- const email = `email${nanoid().toLowerCase()}@gmail.com`; - const hashedEmail = hashEmail(email); + const { email, hashedEmail } = generateTestEmail();Consider adding this helper to
tests/helpers/user.ts:export function generateTestEmail() { const email = `email${nanoid().toLowerCase()}@gmail.com`; return { email, hashedEmail: hashEmail(email), encryptedEmail: encryptEmail(email) }; }tests/resolvers/Mutation/login.spec.ts (4)
98-102: Consider adding validation for encrypted and hashed email formats.While the test correctly creates users with encrypted and hashed emails, it would be beneficial to add assertions that verify the correct format and structure of the encrypted and hashed data.
Add these assertions after user creation:
// Add after user creation expect(newUser.email).not.toBe(email); // Verify email was actually encrypted expect(newUser.hashedEmail).not.toBe(email); // Verify email was actually hashed expect(newUser.hashedEmail).toBe(hashEmail(email)); // Verify hash is reproducibleAlso applies to: 139-144
272-274: Consider centralizing testUser null checks.The same null check for
testUseris repeated in multiple test cases. Consider extracting this into a helper function or beforeEach hook to reduce duplication.Example refactor:
const getTestUserOrFail = () => { if (!testUser) { throw new Error("Error creating test user."); } return testUser; }; // Then in tests: const user = getTestUserOrFail();Also applies to: 298-300, 307-309, 354-356
277-277: Consider making email decryption more explicit in test args.The decryption of emails for login arguments could be more explicit to improve test readability. Consider extracting the decryption into a named variable.
Example:
const decryptedEmail = decryptEmail(testUser.email).decrypted; const args: MutationLoginArgs = { data: { email: decryptedEmail, password: "password", }, };Also applies to: 313-313, 360-360
Line range hint
326-326: Remove commented out expectation.There's a commented out expectation:
// expect((await loginPayload?.user)?.userType).toEqual("SUPERADMIN");. Either remove it or uncomment and fix it if it's still relevant.tests/resolvers/Mutation/signUp.spec.ts (4)
87-94: Consider extracting common test setup.While the email hashing implementation is correct, consider extracting the email generation and hashing into a test helper function since this pattern is repeated across multiple test cases. This would reduce code duplication and make tests more maintainable.
// Suggested helper function: function generateTestEmail() { const email = `email${nanoid().toLowerCase()}@gmail.com`; const hashedEmail = hashEmail(email); return { email, hashedEmail }; }
148-152: Enhance error message for configuration issue.While the null check is good, consider making the error message more descriptive to help with troubleshooting:
- throw new Error("LAST_RESORT_SUPERADMIN_EMAIL is undefined"); + throw new Error("LAST_RESORT_SUPERADMIN_EMAIL must be defined in environment variables for SUPER ADMIN creation");
Line range hint
274-293: Add verification for encrypted email field.While the test verifies the hashed email lookup, it should also verify that the email was properly encrypted in the database:
const createdUser = await User.findOne({ hashedEmail: hashedEmail, }).select("-password"); + // Verify that the email was encrypted + expect(createdUser?.email).not.toBe(email); + expect(decryptEmail(createdUser?.email).decrypted).toBe(email);
314-316: Consider moving test organization setup to beforeEach.The test organization creation logic could be moved to a beforeEach block to keep the test focused on the membership request functionality:
describe("membership request tests", () => { let testOrg; beforeEach(async () => { testOrg = await Organization.create({ name: `orgName${nanoid().toLowerCase()}`, description: `orgDesc${nanoid().toLowerCase()}`, userRegistrationRequired: true, creatorId: testUser?._id, admins: [], membershipRequests: [], members: [testUser?._id], visibleInSearch: false, }); }); it("send membership request if the user registration is required", async () => { const { email, hashedEmail } = generateTestEmail(); // ... rest of the test }); });tests/resolvers/Query/users.spec.ts (2)
87-123: Consider extracting test data setup into helper functions.The current implementation correctly handles email encryption and hashing but could be more maintainable. Consider:
- Creating a helper function to generate test emails
- Using a factory pattern for test user creation with encrypted emails
Example implementation:
function generateTestEmail(): string { return `email${nanoid().toLowerCase()}@gmail.com`; } function createTestUserWithEncryptedEmail(email: string) { return { email: encryptEmail(email), hashedEmail: hashEmail(email), password: "password", firstName: `firstName${nanoid()}`, lastName: `lastName${nanoid()}`, }; } // Usage const testEmails = Array.from({ length: 5 }, generateTestEmail); const testUsers = await User.insertMany( testEmails.map(createTestUserWithEncryptedEmail) );
263-263: Consider extracting email decryption transformation.The email decryption logic is repeated across multiple test cases. Consider extracting this transformation into a helper function to improve maintainability and reduce duplication.
function transformUserWithDecryptedEmail(user: any) { return { ...user, email: decryptEmail(user.email).decrypted, organizationsBlockedBy: [], image: user.image ? `${BASE_URL}${user.image}` : null, }; } // Usage in tests users = users.map(transformUserWithDecryptedEmail);Also applies to: 299-299, 349-349, 410-410, 469-469, 529-529, 594-594, 646-646, 686-686, 724-724, 773-773
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (12)
src/resolvers/MembershipRequest/user.ts(2 hunks)src/resolvers/Mutation/updateUserProfile.ts(2 hunks)src/resolvers/Query/checkAuth.ts(2 hunks)tests/resolvers/Mutation/forgotPassword.spec.ts(2 hunks)tests/resolvers/Mutation/login.spec.ts(10 hunks)tests/resolvers/Mutation/otp.spec.ts(2 hunks)tests/resolvers/Mutation/signUp.spec.ts(16 hunks)tests/resolvers/Mutation/updateCommunity.spec.ts(2 hunks)tests/resolvers/Mutation/updateUserProfile.spec.ts(6 hunks)tests/resolvers/Query/me.spec.ts(2 hunks)tests/resolvers/Query/organizationsMemberConnection.spec.ts(14 hunks)tests/resolvers/Query/users.spec.ts(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/resolvers/MembershipRequest/user.ts
- src/resolvers/Mutation/updateUserProfile.ts
- src/resolvers/Query/checkAuth.ts
- tests/resolvers/Mutation/updateUserProfile.spec.ts
- tests/resolvers/Query/me.spec.ts
🔇 Additional comments (13)
tests/resolvers/Mutation/otp.spec.ts (2)
12-12: LGTM: Import statement correctly added for encryption utility.
The import statement is properly placed and necessary for the new email encryption functionality.
Line range hint 31-69: Verify test coverage for all email encryption scenarios.
The test suite should be expanded to cover additional scenarios introduced by email encryption.
Let's check for other test files that might need similar updates:
Consider adding test cases for:
- Invalid encrypted email format
- Decryption failures
- Edge cases with special characters in emails
tests/resolvers/Mutation/forgotPassword.spec.ts (1)
18-18: LGTM! Import statement is correctly added.
The import of decryptEmail aligns with the PR's objective of implementing email encryption.
tests/resolvers/Mutation/updateCommunity.spec.ts (2)
26-27: LGTM! Imports align with email encryption feature.
The new utility imports for email hashing and encryption are appropriate for implementing the email security feature.
112-113: 🛠️ Refactor suggestion
Verify email encryption in the test.
While the test stores both encrypted and hashed emails, it doesn't verify if the encryption was successful. Consider adding assertions to ensure the stored email is actually encrypted.
const newUser = await User.create({
email: encryptEmail(email),
hashedEmail: hashedEmail,
password: `pass${nanoid().toLowerCase()}`,
firstName: `firstName${nanoid().toLowerCase()}`,
lastName: `lastName${nanoid().toLowerCase()}`,
image: null,
});
+
+ // Verify email encryption
+ expect(newUser.email).not.toBe(email);
+ expect(newUser.hashedEmail).toBe(hashedEmail);Let's verify if similar test improvements are needed in other test files:
tests/resolvers/Mutation/login.spec.ts (2)
29-30: LGTM! Import changes align with email encryption requirements.
The new imports for encryption utilities are correctly added and necessary for the email encryption feature.
Line range hint 1-385: Verify encryption key configuration in test environment.
The tests use encryption/decryption functions but we should verify that the encryption key is properly configured in the test environment.
✅ Verification successful
Encryption configuration is properly handled in tests
The verification shows that:
- The test file correctly imports
dotenv/configfor environment variables - The encryption tests properly handle both
ENCRYPTION_KEYandHASH_PEPPER - The test environment maintains encryption key configuration through environment variables
- The hashing tests include proper cleanup of environment variables after modification
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if encryption key is properly configured in test environment
echo "Checking for encryption key configuration..."
rg -l "ENCRYPTION_KEY|HASH_PEPPER" .env* test*
Length of output: 244
Script:
#!/bin/bash
# Let's check the content of encryption test files and env sample
echo "=== Checking .env.sample ==="
rg "ENCRYPTION_KEY|HASH_PEPPER" .env.sample
echo -e "\n=== Checking encryption test setup ==="
rg "beforeAll|beforeEach.*ENCRYPTION_KEY|HASH_PEPPER" tests/utilities/encryptionModule.spec.ts tests/utilities/hashingModule.spec.ts
echo -e "\n=== Checking if tests mock the encryption keys ==="
rg "mock.*ENCRYPTION_KEY|HASH_PEPPER" tests/utilities/
Length of output: 1763
Script:
#!/bin/bash
# Let's check how encryption is handled in the login test file
echo "=== Checking login test setup ==="
rg "process\.env|beforeAll.*env" tests/resolvers/Mutation/login.spec.ts -A 5
echo -e "\n=== Checking if dotenv is properly loaded ==="
rg "^import.*dotenv" tests/resolvers/Mutation/login.spec.ts tests/setup.ts
Length of output: 457
tests/resolvers/Mutation/signUp.spec.ts (1)
29-30: LGTM: Import statements are correctly added.
The new imports for email hashing and decryption utilities align with the PR's objective of implementing secure email handling.
tests/resolvers/Query/users.spec.ts (1)
22-22: Verify the necessity of dynamic import.
The dynamic import of FundraisingCampaignPledge seems unrelated to the email encryption changes. Consider moving this import to the top with other imports if it's required, or remove it if unused.
✅ Verification successful
Let me check how FundraisingCampaignPledge is actually used in the test file.
Dynamic import of FundraisingCampaignPledge is correctly placed
The dynamic import is intentionally placed in the beforeAll setup hook to ensure the model is registered with Mongoose before the tests run. This is a common pattern in Mongoose testing to handle model registration. Moving it to the top with static imports would not serve the same purpose.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if FundraisingCampaignPledge is used in the test file
rg -l "FundraisingCampaignPledge" tests/resolvers/Query/users.spec.ts
Length of output: 105
Script:
#!/bin/bash
# Show the usage of FundraisingCampaignPledge in the test file with context
rg -A 5 -B 5 "FundraisingCampaignPledge" tests/resolvers/Query/users.spec.ts
Length of output: 311
tests/resolvers/Query/organizationsMemberConnection.spec.ts (4)
14-15: LGTM: Encryption utility imports added correctly.
The new imports for email encryption and hashing utilities align with the PR objective.
24-31: LGTM: Email encryption properly implemented in test data.
The test data creation correctly implements both encryption and hashing of emails, which aligns with the PR's objective of storing both versions for efficient lookups.
Also applies to: 47-48, 64-65
243-244: Code duplication in user mapping functions.
The same user mapping logic with email decryption is repeated across multiple test cases. This duplication was previously noted and the suggestion to extract it into a helper function is still valid.
Also applies to: 338-339, 432-433, 529-530, 626-627, 711-712, 895-896, 969-970, 1072-1073, 1156-1157
243-244: Verify consistent email encryption handling.
While the email decryption is correctly implemented across all test cases, let's verify that all email-related operations are consistently using the encryption utilities.
Also applies to: 338-339, 432-433, 529-530, 626-627, 711-712, 895-896, 969-970, 1072-1073, 1156-1157
✅ Verification successful
Let me analyze the results from the shell scripts to verify the email encryption handling consistency.
The results show:
- Direct email assignments (potential issues):
- Found in
tests/resolvers/Mutation/forgotPassword.spec.ts - Found in
tests/resolvers/Query/helperFunctions/getWhere.spec.ts
Let's verify these potential issues:
Email encryption handling is consistently implemented
The direct email assignments found are in test fixtures and mock data scenarios where encryption is not required:
- In
forgotPassword.spec.ts: Used for testing non-existent email case - In
getWhere.spec.ts: Used for testing query conditions
All actual email handling in the test assertions properly uses:
decryptEmail()for reading emails (41 occurrences across test files)encryptEmail()for storing emails (39 occurrences across test files)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of email encryption utilities across test files
# Test 1: Check for any direct email assignments without encryption
echo "Checking for direct email assignments..."
rg -l "email: ['\"].*@.*['\"]" "tests/"
# Test 2: Verify all email decryption calls
echo "Checking email decryption usage..."
rg "decryptEmail\(" "tests/"
# Test 3: Verify all email encryption calls
echo "Checking email encryption usage..."
rg "encryptEmail\(" "tests/"
Length of output: 8739
Script:
#!/bin/bash
# Check the context of direct email assignments
echo "Checking context in forgotPassword.spec.ts..."
rg -B 2 -A 2 "email: ['\"].*@.*['\"]" "tests/resolvers/Mutation/forgotPassword.spec.ts"
echo "\nChecking context in getWhere.spec.ts..."
rg -B 2 -A 2 "email: ['\"].*@.*['\"]" "tests/resolvers/Query/helperFunctions/getWhere.spec.ts"
Length of output: 657
| let email = ""; | ||
| if (testUser?.email) { | ||
| email = decryptEmail(testUser.email).decrypted; | ||
| } |
There was a problem hiding this comment.
Improve error handling in email decryption logic.
The current implementation has several potential issues:
- No error handling for decryption failures
- Empty string initialization could mask errors
- Implicit handling of null/undefined emails
Consider this improved implementation:
-let email = "";
-if (testUser?.email) {
- email = decryptEmail(testUser.email).decrypted;
-}
+if (!testUser?.email) {
+ throw new Error('Test user email is required');
+}
+const { decrypted: email } = decryptEmail(testUser.email);This change will:
- Fail fast if email is missing
- Use destructuring for clearer intent
- Prevent potential undefined email usage in the test
📝 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.
| let email = ""; | |
| if (testUser?.email) { | |
| email = decryptEmail(testUser.email).decrypted; | |
| } | |
| if (!testUser?.email) { | |
| throw new Error('Test user email is required'); | |
| } | |
| const { decrypted: email } = decryptEmail(testUser.email); |
| data: { | ||
| email: testUser?.email, | ||
| email: email, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add assertions to verify email encryption handling.
While the test verifies OTP generation, it doesn't explicitly verify the email encryption/decryption flow.
Consider adding these assertions:
const args: MutationOtpArgs = {
data: {
email: email,
},
};
+
+// Verify the decrypted email matches expected format
+expect(email).not.toBe(testUser.email); // Should not be encrypted
+expect(email).toMatch(/^[^\s@]+@[^\s@]+\.[^\s@]+$/); // Should be valid email formatCommittable suggestion skipped: line range outside the PR's diff.
| @@ -330,6 +344,7 @@ describe("resolvers -> Mutation -> signUp", () => { | |||
| }); | |||
| it("creates appUserProfile with userId === createdUser._id", async () => { | |||
| const email = `email${nanoid().toLowerCase()}@gmail.com`; | |||
| const hashedEmail = hashEmail(email); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance app user profile verification.
While the test verifies the existence of the app user profile, consider adding more specific assertions:
const appUserProfile = await AppUserProfile.findOne({
userId: createdUser?._id,
});
expect(appUserProfile).toBeTruthy();
+ // Verify specific app user profile fields
+ expect(appUserProfile?.appLanguageCode).toBe(args.data.appLanguageCode);
+ expect(appUserProfile?.isSuperAdmin).toBe(false);Also applies to: 366-366
| let email = ""; | ||
| if (testUser?.email) { | ||
| email = decryptEmail(testUser.email).decrypted; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for email decryption.
The current implementation assumes decryption will always succeed. Consider adding error handling:
let email = "";
if (testUser?.email) {
- email = decryptEmail(testUser.email).decrypted;
+ try {
+ email = decryptEmail(testUser.email).decrypted;
+ } catch (error) {
+ throw new Error(`Failed to decrypt test user email: ${error.message}`);
+ }
}📝 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.
| let email = ""; | |
| if (testUser?.email) { | |
| email = decryptEmail(testUser.email).decrypted; | |
| } | |
| let email = ""; | |
| if (testUser?.email) { | |
| try { | |
| email = decryptEmail(testUser.email).decrypted; | |
| } catch (error) { | |
| throw new Error(`Failed to decrypt test user email: ${error.message}`); | |
| } | |
| } |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2627 +/- ##
===========================================
- Coverage 97.74% 97.19% -0.56%
===========================================
Files 358 360 +2
Lines 18114 18405 +291
Branches 2600 2666 +66
===========================================
+ Hits 17706 17889 +183
- Misses 404 511 +107
- Partials 4 5 +1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (10)
tests/utilities/hashingModule.spec.ts (1)
7-12: Add test case for email case sensitivity.The test cases should explicitly verify that hashing is case-insensitive for email addresses.
Add this test case to the array:
const testCases = [ "[email protected]", "[email protected]", "[email protected]", + "[email protected]", // Same as first case but with different casing ];tests/utilities/encryptionModule.spec.ts (2)
9-10: Avoid hard-coding encrypted test data.The
validEncryptedDataconstant contains a hard-coded encrypted string which could become invalid if the encryption method changes. Consider generating this data dynamically in a beforeEach hook using the actual encryption functions.- const validEncryptedData = - "11898325fe8807edeb99d37f0b168eaa:3991cd4d1a6372ed70492e23d499b066:4f209bb501460537fa9345ca16361023a19f9b2eff1860e8dadc80f29705d469cbe46edc4913e77d3418814b8eb7"; + let validEncryptedData: string; + + beforeEach(() => { + validEncryptedData = encryptEmail("[email protected]"); + });
99-129: Enhance error handling test coverage.While basic error cases are covered, consider adding tests for:
- Specific error types/classes
- Error message consistency
- Error handling during concurrent operations
it("should throw specific error types for different failure scenarios", () => { class EncryptionError extends Error { constructor(message: string) { super(message); this.name = 'EncryptionError'; } } expect(() => decryptEmail("invalid")).toThrow(EncryptionError); expect(() => decryptEmail("invalid")).toThrow(/format/i); }); it("should handle concurrent encryption/decryption errors gracefully", async () => { const promises = Array.from({ length: 10 }, () => Promise.all([ encryptEmail("[email protected]"), decryptEmail("invalid:data:format"), encryptEmail("[email protected]") ].map(p => p.catch(e => e))) ); const results = await Promise.all(promises); for (const [encrypted1, error, encrypted2] of results) { expect(error).toBeInstanceOf(Error); expect(encrypted1).toMatch(/^[0-9a-f]+:[0-9a-f]+:[0-9a-f]+$/i); expect(encrypted2).toMatch(/^[0-9a-f]+:[0-9a-f]+:[0-9a-f]+$/i); } });tests/resolvers/Mutation/login.spec.ts (4)
90-117: Clean up unused variables in validation test.The test case correctly verifies email validation, but contains unused variables:
requestContextis imported but never usedhashedEmailis computed but never usedApply this diff to clean up the test:
- const { requestContext } = await import("../../../src/libraries"); const email = `nonexistentuser${nanoid().toLowerCase()}@gmail.com`; - const hashedEmail = hashEmail(email);🧰 Tools
🪛 GitHub Check: Check for linting, formatting, and type errors
[failure] 91-91:
'requestContext' is assigned a value but never used
[failure] 97-97:
'hashedEmail' is assigned a value but never used
168-173: Consider extracting repeated user creation logic.The user creation logic with encryption and hashing is duplicated across multiple test cases. Consider extracting this into a test helper function.
Example helper function:
async function createTestUserWithEncryptedEmail(email: string, password: string) { const hashedEmail = hashEmail(email); return await User.create({ email: encryptEmail(email), hashedEmail, password, firstName: "firstName", lastName: "lastName", }); }Also applies to: 247-252
301-303: Extract duplicated error message to a constant.The error message "Error creating test user" is duplicated across multiple null checks. Consider extracting it to a constant at the top of the file.
+ const TEST_USER_ERROR = "Error creating test user."; - throw new Error("Error creating test user."); + throw new Error(TEST_USER_ERROR);Also applies to: 327-329, 336-338, 383-385
306-306: Consider extracting email decryption logic.The pattern
decryptEmail(user.email).decryptedis repeated multiple times. Consider extracting this into a helper function for better readability and maintainability.function getDecryptedEmail(encryptedEmail: string): string { return decryptEmail(encryptedEmail).decrypted; }Then use it like:
- email: decryptEmail(testUser.email).decrypted, + email: getDecryptedEmail(testUser.email),Also applies to: 342-342, 389-389
tests/resolvers/Mutation/signUp.spec.ts (3)
87-94: Consider adding error handling for email hashing.While the implementation is correct, consider adding error handling for the hashing operation:
- const hashedEmail = hashEmail(email); + let hashedEmail; + try { + hashedEmail = hashEmail(email); + } catch (error) { + throw new Error(`Failed to hash email: ${error.message}`); + }Also applies to: 103-103
209-230: Enhance email validation test coverage.While testing with an empty string is valid, consider adding more test cases for different invalid email formats:
+ it.each([ + ['invalid-email'], + ['invalid@'], + ['@invalid.com'], + ['[email protected]'], + ['invalid@com.'], + ['invalid@@com.com'], + ])('throws error for invalid email format: %s', async (invalidEmail) => { + try { + const args: MutationSignUpArgs = { + data: { + email: invalidEmail, + firstName: "firstName", + lastName: "lastName", + password: "password", + appLanguageCode: "en", + selectedOrganization: testOrganization?._id, + }, + }; + const { signUp: signUpResolver } = await import( + "../../../src/resolvers/Mutation/signUp" + ); + await signUpResolver?.({}, args, {}); + fail('Should have thrown an error'); + } catch (error: unknown) { + expect((error as Error).message).toEqual("Invalid email format"); + } + });
297-297: Consider creating a test helper for email hashing.There are multiple instances of email hashing throughout the tests. Consider creating a test helper function to reduce duplication:
const createTestEmail = () => { const email = `email${nanoid().toLowerCase()}@gmail.com`; return { email, hashedEmail: hashEmail(email) }; };This would simplify the test cases:
- const email = `email${nanoid().toLowerCase()}@gmail.com`; - const hashedEmail = hashEmail(email); + const { email, hashedEmail } = createTestEmail();Also applies to: 316-316, 338-339, 357-357, 370-370, 389-389
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
tests/resolvers/Mutation/login.spec.ts(11 hunks)tests/resolvers/Mutation/signUp.spec.ts(16 hunks)tests/utilities/encryptionModule.spec.ts(1 hunks)tests/utilities/hashingModule.spec.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Check for linting, formatting, and type errors
tests/resolvers/Mutation/login.spec.ts
[failure] 91-91:
'requestContext' is assigned a value but never used
[failure] 97-97:
'hashedEmail' is assigned a value but never used
🪛 Biome
tests/utilities/hashingModule.spec.ts
[error] 68-68: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (7)
tests/utilities/hashingModule.spec.ts (2)
1-4: LGTM! Test setup and imports are well-structured.
1-125: Overall, well-structured and comprehensive test suite.
The test file provides good coverage of the email hashing functionality, including security-critical aspects, input validation, and error handling. The suggested improvements will make the tests more robust and reliable.
Let's verify the test coverage:
✅ Verification successful
Test coverage is complete and well-aligned with implementation
The test file comprehensively covers both exported functions (hashEmail and compareHashedEmails) from the source file, including:
- All error conditions in
hashEmail:- Invalid email format
- Missing HASH_PEPPER
- Short HASH_PEPPER
- Null/undefined inputs
- Security aspects:
- Different pepper values produce different hashes
- Consistent hashing for same input
- Hash format validation
- All error conditions in
compareHashedEmails:- Invalid hash formats
- Hex encoding errors
- Null/undefined inputs
The test implementation correctly matches the source code's functionality and error handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all exported functions from hashEmail utility are tested
ast-grep --pattern 'export function $func' src/utilities/hashEmail.ts
Length of output: 71
Script:
#!/bin/bash
# Let's try to find the hashEmail.ts file first
fd hashEmail.ts
# Then let's examine its content
cat src/utilities/hashEmail.ts
Length of output: 2404
🧰 Tools
🪛 Biome
[error] 68-68: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
tests/resolvers/Mutation/login.spec.ts (2)
29-31: LGTM: New imports align with encryption requirements.
The added imports for encryption/decryption and hashing utilities are consistent with the PR's objective of implementing email encryption.
Line range hint 1-415: Overall implementation looks good with thorough test coverage.
The test suite effectively covers the new email encryption functionality including:
- Validation of email presence
- Proper encryption/decryption flow
- Error handling
- Edge cases
The suggested improvements are mainly for code maintainability and don't affect the functionality.
🧰 Tools
🪛 GitHub Check: Check for linting, formatting, and type errors
[failure] 91-91:
'requestContext' is assigned a value but never used
[failure] 97-97:
'hashedEmail' is assigned a value but never used
tests/resolvers/Mutation/signUp.spec.ts (3)
29-30: LGTM: Required imports for email encryption feature.
The imports for hashEmail and decryptEmail utilities are correctly added.
148-152: LGTM: Good defensive programming with null check.
The null check for LAST_RESORT_SUPERADMIN_EMAIL is a good practice to prevent runtime errors.
233-236: Past comment about error handling for email decryption still applies.
|
|
||
| it("should throw an error if HASH_PEPPER is missing", () => { | ||
| const originalPepper = process.env.HASH_PEPPER; | ||
| delete process.env.HASH_PEPPER; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace delete operator with undefined assignment.
Using the delete operator can impact performance.
Apply this diff:
- delete process.env.HASH_PEPPER;
+ process.env.HASH_PEPPER = undefined;📝 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.
| delete process.env.HASH_PEPPER; | |
| process.env.HASH_PEPPER = undefined; |
🧰 Tools
🪛 Biome
[error] 68-68: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
| it("should log an error and return false if crypto.timingSafeEqual fails due to invalid hex encoding", () => { | ||
| const invalidHash = "z".repeat(64); // deliberately invalid hex | ||
| let result; | ||
| try { | ||
| result = compareHashedEmails(invalidHash, invalidHash); | ||
| } catch (error) { | ||
| expect(result).toBe(false); | ||
| if (error instanceof Error) { | ||
| expect(error.message).toBe( | ||
| "Failed to compare hashes, likely due to invalid hex encoding", | ||
| ); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
Fix assertion placement and add positive test case.
The test has two issues:
- The
expect(result).toBe(false)assertion is inside the catch block, butresultis undefined at that point - Missing test for successful comparison of valid hashes
Apply these changes:
it("should log an error and return false if crypto.timingSafeEqual fails due to invalid hex encoding", () => {
const invalidHash = "z".repeat(64); // deliberately invalid hex
- let result;
- try {
- result = compareHashedEmails(invalidHash, invalidHash);
- } catch (error) {
- expect(result).toBe(false);
- if (error instanceof Error) {
- expect(error.message).toBe(
- "Failed to compare hashes, likely due to invalid hex encoding",
- );
- }
- }
+ expect(() => compareHashedEmails(invalidHash, invalidHash)).toThrow(
+ "Failed to compare hashes, likely due to invalid hex encoding"
+ );
});
+
+ it("should return true for matching valid hashes", () => {
+ const validHash = "a".repeat(64);
+ expect(compareHashedEmails(validHash, validHash)).toBe(true);
+ });📝 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.
| it("should log an error and return false if crypto.timingSafeEqual fails due to invalid hex encoding", () => { | |
| const invalidHash = "z".repeat(64); // deliberately invalid hex | |
| let result; | |
| try { | |
| result = compareHashedEmails(invalidHash, invalidHash); | |
| } catch (error) { | |
| expect(result).toBe(false); | |
| if (error instanceof Error) { | |
| expect(error.message).toBe( | |
| "Failed to compare hashes, likely due to invalid hex encoding", | |
| ); | |
| } | |
| } | |
| }); | |
| it("should log an error and return false if crypto.timingSafeEqual fails due to invalid hex encoding", () => { | |
| const invalidHash = "z".repeat(64); // deliberately invalid hex | |
| expect(() => compareHashedEmails(invalidHash, invalidHash)).toThrow( | |
| "Failed to compare hashes, likely due to invalid hex encoding" | |
| ); | |
| }); | |
| it("should return true for matching valid hashes", () => { | |
| const validHash = "a".repeat(64); | |
| expect(compareHashedEmails(validHash, validHash)).toBe(true); | |
| }); |
| if (pepper1 != undefined && pepper2 != undefined) { | ||
| process.env.HASH_PEPPER = pepper1; | ||
| const hash1 = hashEmail(email); | ||
| process.env.HASH_PEPPER = "pepper2"; | ||
| const hash2 = hashEmail(email); | ||
| expect(hash1).not.toEqual(hash2); | ||
| } |
There was a problem hiding this comment.
Fix pepper comparison test logic.
The test has two issues:
- Uses hardcoded "pepper2" instead of the generated
pepper2variable - Weak assertion of pepper existence using optional chaining
Apply this diff to fix the issues:
- if (pepper1 != undefined && pepper2 != undefined) {
+ if (!pepper1 || !pepper2) {
+ throw new Error("Failed to generate test peppers");
+ }
process.env.HASH_PEPPER = pepper1;
const hash1 = hashEmail(email);
- process.env.HASH_PEPPER = "pepper2";
+ process.env.HASH_PEPPER = pepper2;
const hash2 = hashEmail(email);
expect(hash1).not.toEqual(hash2);
- }📝 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.
| if (pepper1 != undefined && pepper2 != undefined) { | |
| process.env.HASH_PEPPER = pepper1; | |
| const hash1 = hashEmail(email); | |
| process.env.HASH_PEPPER = "pepper2"; | |
| const hash2 = hashEmail(email); | |
| expect(hash1).not.toEqual(hash2); | |
| } | |
| if (!pepper1 || !pepper2) { | |
| throw new Error("Failed to generate test peppers"); | |
| } | |
| process.env.HASH_PEPPER = pepper1; | |
| const hash1 = hashEmail(email); | |
| process.env.HASH_PEPPER = pepper2; | |
| const hash2 = hashEmail(email); | |
| expect(hash1).not.toEqual(hash2); |
| it("should generate a random salt of the specified length", () => { | ||
| const salt = generateRandomIV(); | ||
| expect(salt.length).toEqual(2 * 16); | ||
| }); | ||
|
|
||
| it("should generate unique IVs for each call", () => { | ||
| const iv1 = generateRandomIV(); | ||
| const iv2 = generateRandomIV(); | ||
| expect(iv1).not.toEqual(iv2); | ||
| }); | ||
|
|
||
| it("should generate IV with valid hex characters", () => { | ||
| const iv = generateRandomIV(); | ||
| expect(iv).toMatch(/^[0-9a-f]+$/i); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance IV generation tests with cryptographic randomness checks.
While the current tests verify length, uniqueness, and format, they don't verify the cryptographic quality of the generated IVs. Consider adding statistical tests to ensure proper randomness.
it("should generate cryptographically random IVs", () => {
// Generate a large sample of IVs
const samples = Array.from({ length: 1000 }, () => generateRandomIV());
// Check byte distribution
const byteFrequency = new Map<string, number>();
for (const iv of samples) {
for (let i = 0; i < iv.length; i += 2) {
const byte = iv.substr(i, 2);
byteFrequency.set(byte, (byteFrequency.get(byte) || 0) + 1);
}
}
// Chi-square test for uniform distribution
const expected = samples.length * 2 / 256; // Expected frequency per byte
let chiSquare = 0;
byteFrequency.forEach((freq) => {
chiSquare += Math.pow(freq - expected, 2) / expected;
});
// With 255 degrees of freedom, chi-square should be between 200 and 310 (p=0.99)
expect(chiSquare).toBeGreaterThan(200);
expect(chiSquare).toBeLessThan(310);
});| describe("encryptEmail and decryptEmail", () => { | ||
| it("should encrypt and decrypt an email correctly", () => { | ||
| const email = "[email protected]"; | ||
| const encryptedWithEmailSalt = encryptEmail(email); | ||
| const { decrypted }: { decrypted: string } = decryptEmail( | ||
| encryptedWithEmailSalt, | ||
| ); | ||
| expect(encryptedWithEmailSalt).not.toEqual(email); | ||
| expect(decrypted).toEqual(email); | ||
| }); | ||
|
|
||
| it("throws an error for invalid email format", () => { | ||
| expect(() => encryptEmail("a".repeat(10000))).toThrow( | ||
| "Invalid email format", | ||
| ); | ||
| }); | ||
|
|
||
| it("throws an error for empty email input", () => { | ||
| expect(() => encryptEmail("")).toThrow("Empty or invalid email input."); | ||
| }); | ||
|
|
||
| it("throws an error for an invalid encryption key", () => { | ||
| const originalKey = process.env.ENCRYPTION_KEY; | ||
| process.env.ENCRYPTION_KEY = "invalid_key"; | ||
| expect(() => encryptEmail("[email protected]")).toThrow( | ||
| "Encryption key must be a valid 256-bit hexadecimal string (64 characters).", | ||
| ); | ||
| process.env.ENCRYPTION_KEY = originalKey; | ||
| }); | ||
|
|
||
| it("should handle email addresses with special characters", () => { | ||
| const email = "[email protected]"; | ||
| const encrypted = encryptEmail(email); | ||
| const { decrypted } = decryptEmail(encrypted); | ||
| expect(decrypted).toEqual(email); | ||
| }); | ||
|
|
||
| it("handles very long email input gracefully", () => { | ||
| const longEmail = "a".repeat(10000) + "@example.com"; | ||
| expect(() => encryptEmail(longEmail)).not.toThrow(); | ||
| const encrypted = encryptEmail(longEmail); | ||
| const decrypted = decryptEmail(encrypted).decrypted; | ||
| expect(decrypted).toBe(longEmail); | ||
| }); | ||
|
|
||
| it("should use a unique IV for each encryption", () => { | ||
| const email = "[email protected]"; | ||
| const encrypted1 = encryptEmail(email); | ||
| const encrypted2 = encryptEmail(email); | ||
| expect(encrypted1).not.toEqual(encrypted2); | ||
| }); | ||
|
|
||
| it("should maintain consistent encryption format", () => { | ||
| const email = "[email protected]"; | ||
| const encrypted = encryptEmail(email); | ||
| expect(encrypted).toMatch(/^[0-9a-f]+:[0-9a-f]+:[0-9a-f]+$/i); | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add security-critical test cases.
The test suite is missing several important security scenarios:
- Memory cleanup after encryption/decryption
- Key rotation support
- Timing attack prevention (as noted in past comments)
it("should clear sensitive data from memory after encryption/decryption", () => {
const email = "[email protected]";
const encrypted = encryptEmail(email);
const heapDump = process.memoryUsage();
// Force garbage collection
if (global.gc) {
global.gc();
}
// Search for email in memory (this is a simplified example)
const heapStr = JSON.stringify(heapDump);
expect(heapStr).not.toContain(email);
});
it("should support key rotation", () => {
const email = "[email protected]";
const oldKey = "0".repeat(64);
const newKey = "1".repeat(64);
// Encrypt with old key
process.env.ENCRYPTION_KEY = oldKey;
const encrypted = encryptEmail(email);
// Decrypt with new key should fail
process.env.ENCRYPTION_KEY = newKey;
expect(() => decryptEmail(encrypted)).toThrow();
// Re-encrypt with new key
const reencrypted = reencryptEmail(encrypted, oldKey, newKey);
const { decrypted } = decryptEmail(reencrypted);
expect(decrypted).toEqual(email);
});
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
tests/resolvers/Mutation/login.spec.ts (5)
90-112: Consider adding more validation test cases.While the test case correctly validates null email, consider adding tests for:
- Empty string email (
"")- Undefined email
- Invalid email format
This would provide more comprehensive validation coverage.
it("throws ValidationError for empty string email", async () => { const args: MutationLoginArgs = { data: { email: "", password: "password", }, }; await expect(loginResolver?.({}, args, {})) .rejects .toThrow(ValidationError); });
296-298: Improve error handling for testUser null checks.While the null checks are good, consider:
- Using a more descriptive error message that includes the test context
- Extracting the check into a helper function since it's used multiple times
function assertTestUser(context: string): asserts testUser { if (!testUser) { throw new Error(`Test user not found in context: ${context}`); } }Also applies to: 322-324, 331-333, 378-380
301-301: Create a helper function for email decryption.The pattern of accessing
.decryptedfrom decryptEmail result is repeated. Consider creating a helper function to make this more concise and maintainable.function getDecryptedEmail(encryptedEmail: string): string { return decryptEmail(encryptedEmail).decrypted; }Also applies to: 327-327, 337-337, 384-384
Line range hint
90-404: Add test coverage for encryption edge cases.Consider adding tests for:
- Handling encryption/decryption failures
- Verifying the format of encrypted and hashed emails
- Attempting to login with an already encrypted email
it("throws error when trying to login with an encrypted email", async () => { const encryptedEmail = encryptEmail("[email protected]"); const args: MutationLoginArgs = { data: { email: encryptedEmail, // Attempting to use already encrypted email password: "password", }, }; await expect(loginResolver?.({}, args, {})) .rejects .toThrow(); });
Line range hint
1-404: Update documentation to reflect email encryption changes.As mentioned in the PR objectives, documentation needs to be updated to reflect these changes. Consider:
- Adding JSDoc comments explaining the encryption/hashing flow
- Updating relevant documentation files
- Adding comments explaining the test scenarios
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
tests/resolvers/Mutation/login.spec.ts(11 hunks)tests/resolvers/Mutation/removeMember.spec.ts(1 hunks)tests/resolvers/Query/getVolunteerRanks.spec.ts(1 hunks)
🔇 Additional comments (3)
tests/resolvers/Query/getVolunteerRanks.spec.ts (1)
61-61: Verify if changing weekly volunteer hours to 0 is intentional.
This change appears unrelated to the PR's objective of implementing email encryption. The modification reduces the expected weekly volunteer hours from 2 to 0, which could impact volunteer rank calculations.
Please confirm:
- Is this change intentional?
- If yes, why should weekly volunteer hours be 0 when other time periods (monthly: 2 hours, yearly: 8 hours, all-time: 10 hours) show non-zero values?
✅ Verification successful
Expected weekly hours of 0 is correct - no changes needed
After reviewing the implementation and test data setup, the change from 2 to 0 hours for weekly volunteer ranks is correct. The test data shows that the most recent volunteer hours were logged "yesterday" in the test setup, but the weekly calculation uses startOfWeek() which resets to the beginning of the current week. Since the test runs in an isolated environment with its own clock, there are no hours recorded within the current week, making 0 the expected value.
Key evidence:
- The
getVolunteerRanksresolver usesstartOfWeek(now)for weekly calculations - Test data in
createVolunteerAndActionssets hours for "yesterday" (2 hours), "two weeks ago" (4 hours), and older dates - When tests run, these dates are relative to the test environment's clock, falling outside the current week
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for related changes in volunteer hours calculation logic
# Look for recent changes in files related to volunteer hours calculation
rg -l "hoursVolunteered|getVolunteerRanks" src/
Length of output: 341
Script:
#!/bin/bash
# Let's check the implementation of getVolunteerRanks resolver and its test
cat src/resolvers/Query/getVolunteerRanks.ts
echo "---"
cat tests/resolvers/Query/getVolunteerRanks.spec.ts
Length of output: 7773
Script:
#!/bin/bash
# Let's check the test helper function that sets up the test data
cat tests/helpers/volunteers.ts
Length of output: 7342
tests/resolvers/Mutation/removeMember.spec.ts (1)
Line range hint 1-400: Consider separating unrelated changes.
The changes to error handling in this test file appear unrelated to the PR's objective of implementing email encryption. To maintain focus and facilitate easier review:
- Consider reverting these changes and including them in a separate PR focused on error handling improvements.
- If these changes are necessary for email encryption, please explain the connection in the PR description.
Let's verify if this file needs changes for email encryption:
tests/resolvers/Mutation/login.spec.ts (1)
29-30: LGTM! Necessary imports added for email encryption.
The new imports for encryption utilities align well with the PR's objective of implementing email encryption.
| } catch { | ||
| expect(spy).toHaveBeenCalledWith(ORGANIZATION_NOT_FOUND_ERROR.MESSAGE); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error message validation for consistency.
This test case handles the organization not found error differently from other test cases in the file. While other test cases validate both the translation call and the error message, this one only checks the translation call. For consistency and thorough testing, consider adding error message validation.
Apply this diff to align with other test cases:
- } catch {
+ } catch (error: unknown) {
expect(spy).toHaveBeenCalledWith(ORGANIZATION_NOT_FOUND_ERROR.MESSAGE);
+ expect((error as Error).message).toEqual(
+ `Translated ${ORGANIZATION_NOT_FOUND_ERROR.MESSAGE}`
+ );
}📝 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.
| } catch { | |
| expect(spy).toHaveBeenCalledWith(ORGANIZATION_NOT_FOUND_ERROR.MESSAGE); | |
| } | |
| } catch (error: unknown) { | |
| expect(spy).toHaveBeenCalledWith(ORGANIZATION_NOT_FOUND_ERROR.MESSAGE); | |
| expect((error as Error).message).toEqual( | |
| `Translated ${ORGANIZATION_NOT_FOUND_ERROR.MESSAGE}` | |
| ); | |
| } |
|
I can't increase the code coverage as due to the recent changes in the repo, the codecov report has no files that this pr have changed, this is happening after I updated my branch, can you help @pranshugupta54 |
|
Those files are in File changes in your PR. If some files got added to this PR due to some merge conflicts then u can get them back from |
It worked now, thanks! |
|
I wanted to clear some doubts, these are related to increase code coverage - |
|
Tip For best results, initiate chat on the files or code changes. To test the scenarios you mentioned, you can use mocking and stubbing to simulate the conditions where
Explanation:
Additional Tips:
Resources: Let me know if you need further guidance! |
|
Tip For best results, initiate chat on the files or code changes. To test that your
Here's an example of how you might write the tests: // tests/resolvers/Organization/members.spec.ts
import { members } from '../../../src/resolvers/Organization/members';
import { User } from '../../../src/models/User';
import { decryptEmail } from '../../../src/utilities/encryption';
import logger from '../../../src/logger';
import { GraphQLError } from 'graphql';
jest.mock('../../../src/models/User');
jest.mock('../../../src/utilities/encryption');
jest.mock('../../../src/logger');
describe('Organization members resolver', () => {
afterEach(() => {
jest.clearAllMocks();
});
it('should log a warning when user.email does not exist', async () => {
const mockUsers = [
{ _id: 'userId1', email: undefined },
];
(User.find as jest.Mock).mockResolvedValue(mockUsers);
const parent = { members: ['userId1'] };
await members(parent);
expect(logger.warn).toHaveBeenCalledWith('User missing email field', { userId: 'userId1' });
});
it('should throw an error when email decryption fails', async () => {
const mockUsers = [
{ _id: 'userId2', email: 'encryptedEmail' },
];
(User.find as jest.Mock).mockResolvedValue(mockUsers);
(decryptEmail as jest.Mock).mockImplementation(() => {
throw new Error('Decryption error');
});
const parent = { members: ['userId2'] };
await expect(members(parent)).rejects.toThrow(new GraphQLError('Failed to process user data'));
expect(logger.error).toHaveBeenCalledWith('Email decryption failed', {
userId: 'userId2',
error: 'Decryption error',
});
});
});Explanation:
Additional Tips:
This approach should help you increase code coverage by testing the different branches and error handling in your resolver. Let me know if you need further assistance! |
|
Tip For best results, initiate chat on the files or code changes. To test the scenarios you mentioned and increase code coverage, you can mock the database queries and the 1. Testing when You can mock the Example using Jest: // tests/resolvers/Organization/members.spec.ts
import { members } from '../../../src/resolvers/Organization/members';
import { User } from '../../../src/models/User';
import logger from '../../../src/logger';
import { InterfaceUser } from '../../../src/models/User';
jest.mock('../../../src/models/User');
jest.mock('../../../src/logger');
describe('Organization members resolver', () => {
afterEach(() => {
jest.clearAllMocks();
});
it('should log a warning if user.email does not exist', async () => {
const parent = { members: ['user1'] };
const mockUsers: InterfaceUser[] = [
{
_id: 'user1',
// email is missing
name: 'Test User',
},
];
(User.find as jest.Mock).mockReturnValue({
lean: () => mockUsers,
});
const loggerWarnSpy = jest.spyOn(logger, 'warn');
const result = await members(parent);
expect(loggerWarnSpy).toHaveBeenCalledWith('User missing email field', {
userId: 'user1',
});
expect(result).toEqual(mockUsers);
});
});Explanation:
2. Testing when Mock the Example: import { members } from '../../../src/resolvers/Organization/members';
import { User } from '../../../src/models/User';
import { decryptEmail } from '../../../src/utilities/encryption';
import logger from '../../../src/logger';
import { GraphQLError } from 'graphql';
jest.mock('../../../src/models/User');
jest.mock('../../../src/utilities/encryption');
jest.mock('../../../src/logger');
describe('Organization members resolver', () => {
afterEach(() => {
jest.clearAllMocks();
});
it('should log an error and throw GraphQLError if decryptEmail fails', async () => {
const parent = { members: ['user1'] };
const mockUsers = [
{
_id: 'user1',
email: 'encryptedEmail',
name: 'Test User',
},
];
(User.find as jest.Mock).mockReturnValue({
lean: () => mockUsers,
});
(decryptEmail as jest.Mock).mockImplementation(() => {
throw new Error('Decryption failed');
});
const loggerErrorSpy = jest.spyOn(logger, 'error');
await expect(members(parent)).rejects.toThrow(GraphQLError);
expect(loggerErrorSpy).toHaveBeenCalledWith('Email decryption failed', {
userId: 'user1',
error: 'Decryption failed',
});
});
});Explanation:
3. Testing successful decryption: Ensure that when Example: it('should decrypt emails and return users with decrypted emails', async () => {
const parent = { members: ['user1'] };
const mockUsers = [
{
_id: 'user1',
email: 'encryptedEmail',
name: 'Test User',
},
];
(User.find as jest.Mock).mockReturnValue({
lean: () => mockUsers,
});
(decryptEmail as jest.Mock).mockReturnValue({
decrypted: '[email protected]',
});
const result = await members(parent);
expect(result).toEqual([
{
_id: 'user1',
email: '[email protected]',
name: 'Test User',
},
]);
});Explanation:
Additional Tips:
By writing these tests, you'll cover the scenarios where Let me know if you need further assistance! |
|
the reason why code cov is less, is that we haven't implemented testing for scenarios where we do a db call (for example extracting users from db) and then testing if application throws error if user.email doesn't exist...can I follow the approach given by codderrabitai, or can you help me with this @pranshugupta54 (I slacked the same thing, but I didn't get reply) |
What kind of change does this PR introduce?
Feature to encrypt user email addresses in the Database
Issue Number:
#1756
Fixes #
Did you add tests for your changes?
Yes
Snapshots/Videos:
If relevant, did you update the documentation?
No
Summary
two functionalities have been implemented
encryption and decryption of emails
storing hashingEmails in db
storing encrypted emails in db, decrypted function is implemented to decrypt emails
hashed version of email is also stored in db, for faster lookups we can use hashing if a match occurs then we can return a encrypted or a decrypted email based on the functionality.
hashing was introduced to avoid going through all the users and decrypting every email to find a match, hashing makes this faster.
Does this PR introduce a breaking change?
Yes this is a significant change in the codebase
Other information
Have you read the contributing guide?
Summary by CodeRabbit
Release Notes
New Features
ENCRYPTION_KEYandHASH_PEPPERfor secure email handling.hashedEmailand encrypted email fields.Bug Fixes
Documentation
Tests