Conversation
Reviewer's GuideImplements bcrypt-based password hashing in the User.update flow, adds unit tests for both hashing and non-hashing scenarios, and adjusts index route tests to reference the renamed ScrapyardItem model and include document count mocks. Sequence Diagram for Password Hashing in User UpdatesequenceDiagram
participant C as Caller
participant UM as User Model
participant B as bcrypt
participant DB as Database
C->>UM: findByIdAndUpdate(id, updateData)
alt updateData contains password
UM->>UM: Check for password in updateData
UM->>B: genSalt(10)
B-->>UM: salt
UM->>B: hash(updateData.password, salt)
B-->>UM: hashedPassword
UM->>UM: updateData.password = hashedPassword
end
UM->>UM: Convert updateData to snake_case for DB
UM->>DB: Update user record with snake_case_data
DB-->>UM: Updated user data / Confirmation
UM-->>C: Result / Updated User
Updated Class Diagram for User ModelclassDiagram
class User {
<<Model>>
+static findByIdAndUpdate(id, updateData) Promise~User~
}
class bcrypt {
<<Library>>
+static genSalt(rounds) Promise~string~
+static hash(data, salt) Promise~string~
}
User ..> bcrypt : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
| */ | ||
| static async findByIdAndUpdate(id, updateData) { | ||
| try { | ||
| // Hash password if it is being updated | ||
| if (updateData.password) { | ||
| const salt = await bcrypt.genSalt(10); | ||
| updateData.password = await bcrypt.hash(updateData.password, salt); | ||
| } | ||
|
|
||
| // Convert to snake_case for Supabase | ||
| const snakeCaseData = {}; | ||
|
|
There was a problem hiding this comment.
Potential Issue with User Existence Check
The method findByIdAndUpdate updates user data without verifying if the user with the provided id actually exists in the database. This could lead to errors or unintended behavior if an invalid or non-existent id is used.
Recommendation:
- Implement a check to verify the user's existence before attempting to update. This could be done by querying the database for the user
idand proceeding with the update only if the user is found. This would enhance the reliability and robustness of the method.
| // Convert to snake_case for Supabase | ||
| const snakeCaseData = {}; | ||
|
|
There was a problem hiding this comment.
Refactoring Suggestion for Key Conversion
The method manually converts keys from camelCase to snake_case using a regex directly within the method. This approach is repeated across different methods, which could lead to code duplication and maintenance challenges.
Recommendation:
- Consider creating a utility function that handles the conversion of object keys from camelCase to snake_case. Use this utility function across all methods that require such conversion. This would not only reduce code duplication but also centralize changes to the conversion logic, making the codebase easier to maintain and less error-prone.
| describe('User.findByIdAndUpdate', () => { | ||
| it('hashes password when provided', async () => { | ||
| await User.findByIdAndUpdate('1', { password: 'secret' }); | ||
|
|
||
| expect(bcrypt.genSalt).toHaveBeenCalledWith(10); | ||
| expect(bcrypt.hash).toHaveBeenCalledWith('secret', 'salt'); | ||
| expect(updateMock).toHaveBeenCalled(); | ||
| const arg = updateMock.mock.calls[0][0]; | ||
| expect(arg.password).toBe('hashedPassword'); | ||
| }); | ||
|
|
||
| it('does not hash when password not provided', async () => { | ||
| await User.findByIdAndUpdate('1', { displayName: 'Test' }); | ||
|
|
||
| expect(bcrypt.hash).not.toHaveBeenCalled(); | ||
| const arg = updateMock.mock.calls[0][0]; | ||
| expect(arg.display_name).toBe('Test'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The current tests in User.findByIdAndUpdate are well-implemented for positive scenarios. However, they lack coverage for error handling and edge cases, such as what happens if bcrypt.hash fails or if the database operation returns an error. Recommendation: Add additional test cases to handle exceptions and ensure the function behaves correctly under error conditions. This will improve the robustness and fault tolerance of the application.
| ]), | ||
| countDocuments: jest.fn().mockResolvedValue(2), | ||
| find: jest.fn().mockResolvedValue([ | ||
| { username: 'testuser1', lastActive: new Date() } |
There was a problem hiding this comment.
Using new Date() directly in the mock (line 15) introduces non-deterministic behavior in tests, as the value changes with each test execution. This can lead to flaky tests. Consider using a fixed timestamp or a mock date library like jest-date-mock to ensure consistency in test outcomes.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @numbpill3d - I've reviewed your changes - here's some feedback:
- Extract the bcrypt password-hashing logic into a dedicated helper or class method so it can be reused (e.g., in user creation) and keep the model method concise.
- Replace the hardcoded salt rounds (10) with a configurable constant to make it easier to adjust security parameters in one place.
- Consider adding a guard in findByIdAndUpdate to skip re-hashing if the incoming password is already hashed to prevent unintended double-hashing.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| static async findByIdAndUpdate(id, updateData) { | ||
| try { | ||
| // Hash password if it is being updated | ||
| if (updateData.password) { |
There was a problem hiding this comment.
issue (bug_risk): Falsy password check may skip hashing and store plaintext
Checking if (updateData.password) may miss empty or falsy passwords, resulting in unhashed plaintext storage. Use property existence checks or validate for empty values before hashing.
1 participant
Summary
Testing
npm test(fails: Missing required Supabase environment variables)https://chatgpt.com/codex/tasks/task_e_68450196e69c832fbf9dfda55bd6bee1
Summary by Sourcery
Ensure that password updates are securely hashed and update test suite to reflect model changes and verify hashing behavior.
Bug Fixes:
Enhancements:
Tests: