Skip to content

Hash password on update #54

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 on update #54

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
9 changes: 9 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,11 @@ class User {
*/
static async findByIdAndUpdate(id, updateData) {
try {
// Hash password if provided
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 = {};

Expand Down Expand Up @@ -236,6 +241,10 @@ class User {
*/
async save() {
try {
if (this.password) {
Copy link
Copy Markdown
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): Hashing all passwords on save may cause double-hashing

Currently, the save method re-hashes the password every time, even if it hasn't changed. This can result in double-hashing and lock users out. Only hash the password if it was modified.

const salt = await bcrypt.genSalt(10);
this.password = await bcrypt.hash(this.password, salt);
}
// Convert user object to snake_case for Supabase
const userData = {
username: this.username,
Comment on lines 249 to 250
Copy link
Copy Markdown
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 method save does not show the conversion of user data keys from camelCase to snake_case before updating the database. If the database schema expects snake_case (as seen in other parts of the code), this inconsistency can lead to bugs or data not being saved correctly. Ensure that data keys are consistently formatted before any database operations to prevent potential issues.

Expand Down
43 changes: 43 additions & 0 deletions tests/server/models/user-password.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
const bcrypt = require('bcrypt');

jest.mock('../../../server/utils/database', () => {
const chain = {
from: jest.fn(() => chain),
update: jest.fn(() => chain),
eq: jest.fn(() => chain),
select: jest.fn(() => chain),
single: jest.fn(() => Promise.resolve({ data: { id: 'user1', password: 'hashed' }, error: null }))
};
return { supabase: chain, supabaseAdmin: chain };
});

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

describe('User password hashing', () => {
beforeEach(() => {
jest.clearAllMocks();
});

it('hashes password on findByIdAndUpdate', async () => {
jest.spyOn(bcrypt, 'genSalt').mockResolvedValue('salt');
jest.spyOn(bcrypt, 'hash').mockResolvedValue('hashed_pw');
await User.findByIdAndUpdate('user1', { password: 'plain' });
expect(bcrypt.hash).toHaveBeenCalledWith('plain', 'salt');
const { supabaseAdmin } = require('../../../server/utils/database');
expect(supabaseAdmin.update).toHaveBeenCalledWith({ password: 'hashed_pw' });
});

it('hashes password on save', async () => {
jest.spyOn(bcrypt, 'genSalt').mockResolvedValue('salt2');
jest.spyOn(bcrypt, 'hash').mockResolvedValue('hashed_pw2');
const user = new User();
user.id = 'user2';
user.username = '';
user.email = '';
user.password = 'plain2';
await user.save();
expect(bcrypt.hash).toHaveBeenCalledWith('plain2', 'salt2');
const { supabaseAdmin } = require('../../../server/utils/database');
expect(supabaseAdmin.update).toHaveBeenCalledWith(expect.objectContaining({ password: 'hashed_pw2' }));
});
Comment on lines +41 to +42
Copy link
Copy Markdown
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 test uses expect.objectContaining to check if the password field in the object passed to supabaseAdmin.update contains the expected hashed password. While this is useful for partial matching, it might be beneficial to assert the entire object structure to ensure that no unexpected fields are being updated or that all necessary fields are included in the update. This would enhance the robustness of the test by verifying the complete payload.

Recommended Solution:
Consider using a more specific assertion to check the entire object structure, especially if the structure is known and critical. For example:

expect(supabaseAdmin.update).toHaveBeenCalledWith({ id: 'user2', password: 'hashed_pw2' });

});
6 changes: 5 additions & 1 deletion tests/server/routes/index.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@
const request = require('supertest');
const express = require('express');

process.env.SUPABASE_URL = process.env.SUPABASE_URL || 'http://localhost';
process.env.SUPABASE_KEY = process.env.SUPABASE_KEY || 'key';
process.env.SUPABASE_SERVICE_KEY = process.env.SUPABASE_SERVICE_KEY || 'service';
Comment on lines +7 to +9
Copy link
Copy Markdown
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.

Security Issue: Hardcoded Sensitive Keys

The environment variables for Supabase (SUPABASE_URL, SUPABASE_KEY, SUPABASE_SERVICE_KEY) are being set with hardcoded fallback values directly in the test file. This practice can lead to security risks if sensitive keys are exposed in the codebase.

Recommendation:

  • Use a secure method to manage environment variables, such as loading them from a .env file or a secure vault, especially for sensitive keys. Ensure that no sensitive information is hardcoded in the codebase.


// Mock dependencies
jest.mock('../../../server/models/User', () => ({
findRecent: jest.fn().mockResolvedValue([
Expand All @@ -21,7 +25,7 @@ jest.mock('../../../server/models/Item', () => ({
{ id: 3, title: 'Featured Item 1' },
{ id: 4, title: 'Featured Item 2' }
])
}));
}), { virtual: true });

// Mock express-handlebars
jest.mock('express-handlebars', () => ({
Expand Down
4 changes: 4 additions & 0 deletions tests/wir-transactions.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@
* Tests for the WIR currency system and transactions
*/

process.env.SUPABASE_URL = process.env.SUPABASE_URL || 'http://localhost';
process.env.SUPABASE_KEY = process.env.SUPABASE_KEY || 'key';
process.env.SUPABASE_SERVICE_KEY = process.env.SUPABASE_SERVICE_KEY || 'service';
Comment on lines +6 to +8
Copy link
Copy Markdown
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.

Hardcoding default values for sensitive keys (SUPABASE_KEY, SUPABASE_SERVICE_KEY) directly in the test file poses a security risk. It's recommended to manage these keys securely, for example, by using environment-specific configuration files or secret management tools that do not expose these keys in the codebase. This approach helps prevent accidental exposure and misuse of sensitive keys.

Comment on lines +6 to +8
Copy link
Copy Markdown
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.

Direct manipulation of environment variables within the test file (SUPABASE_URL, SUPABASE_KEY, SUPABASE_SERVICE_KEY) can lead to difficulties in managing changes in the environment configuration. Consider using a centralized configuration management approach, such as a dedicated configuration module or file that handles all environment settings. This would improve the maintainability and scalability of the test environment setup.


const { supabase } = require('../server/utils/database');
const WIRTransaction = require('../server/models/WIRTransaction');
const createWIRTransactionsTable = require('../scripts/migrations/create-wir-transactions-table');
Expand Down