Skip to content

Hash password updates #23

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
numbpill3d wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Hash password updates #23

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions server/models/User.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,12 @@ class User {
*/
static async findByIdAndUpdate(id, updateData) {
try {
// Hash password if it is being updated
if (updateData.password) {
Copy link
Contributor

@sourcery-ai sourcery-ai bot Jun 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const salt = await bcrypt.genSalt(10);
updateData.password = await bcrypt.hash(updateData.password, salt);
}

// Convert to snake_case for Supabase
const snakeCaseData = {};

Comment on lines 203 to 214
Copy link
Contributor

@codereviewbot-ai codereviewbot-ai bot Jun 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 id and proceeding with the update only if the user is found. This would enhance the reliability and robustness of the method.

Comment on lines 212 to 214
Copy link
Contributor

@codereviewbot-ai codereviewbot-ai bot Jun 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Expand Down
49 changes: 49 additions & 0 deletions tests/server/models/user.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
// Mock bcrypt functions
jest.mock('bcrypt', () => ({
genSalt: jest.fn(() => Promise.resolve('salt')),
hash: jest.fn(() => Promise.resolve('hashedPassword'))
}));

const bcrypt = require('bcrypt');

// Mock database utilities
const updateMock = jest.fn(() => ({
eq: jest.fn(() => ({
select: jest.fn(() => ({
single: jest.fn().mockResolvedValue({ data: { id: '1', password: 'hashedPassword' }, error: null })
}))
}))
}));

const fromMock = jest.fn(() => ({ update: updateMock }));

jest.mock('../../../server/utils/database', () => ({
supabase: { from: fromMock },
supabaseAdmin: { from: fromMock }
}));

const User = require('../../../server/models/User');

beforeEach(() => {
jest.clearAllMocks();
});

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');
});
});
Comment on lines +31 to +49
Copy link
Contributor

@codereviewbot-ai codereviewbot-ai bot Jun 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

9 changes: 7 additions & 2 deletions tests/server/routes/index.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,18 +9,23 @@ jest.mock('../../../server/models/User', () => ({
findRecent: jest.fn().mockResolvedValue([
{ username: 'testuser1', displayName: 'Test User 1' },
{ username: 'testuser2', displayName: 'Test User 2' }
]),
countDocuments: jest.fn().mockResolvedValue(2),
find: jest.fn().mockResolvedValue([
{ username: 'testuser1', lastActive: new Date() }
Copy link
Contributor

@codereviewbot-ai codereviewbot-ai bot Jun 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

])
}));

jest.mock('../../../server/models/Item', () => ({
jest.mock('../../../server/models/ScrapyardItem', () => ({
findRecent: jest.fn().mockResolvedValue([
{ id: 1, title: 'Test Item 1' },
{ id: 2, title: 'Test Item 2' }
]),
findFeatured: jest.fn().mockResolvedValue([
{ id: 3, title: 'Featured Item 1' },
{ id: 4, title: 'Featured Item 2' }
])
]),
countDocuments: jest.fn().mockResolvedValue(2)
}));

// Mock express-handlebars
Expand Down