Skip to content

Add email validation #29

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 2 commits into
base: main
Choose a base branch
from
Open

Add email validation #29

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
29bd507
Validate email and test
numbpill3d Jun 8, 2025
d2b1f0c
Update server/routes/users.js
numbpill3d Jun 8, 2025
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/routes/users.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,12 @@ router.post('/register', async (req, res) => {
errors.push({ msg: 'Username can only contain letters, numbers, underscores, and hyphens' });
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 regex used for username validation (/^[a-zA-Z0-9_-]+$/) is very restrictive, only allowing alphanumeric characters, underscores, and hyphens. This might be unnecessarily limiting for users who wish to use other characters in their usernames, such as periods or spaces. Consider relaxing this restriction if your application context allows for more diverse username characters.

For example, allowing periods and spaces:

if (username && !/^[a-zA-Z0-9_\-\.\s]+$/.test(username)) {
    errors.push({ msg: 'Username can only contain letters, numbers, underscores, hyphens, periods, and spaces' });
}

}

// Normalize and validate email format
let normalizedEmail = email ? email.trim().toLowerCase() : email;
if (normalizedEmail && !/^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(normalizedEmail)) {
errors.push({ msg: 'Invalid email address' });
}

// Validate custom glyph
if (customGlyph && customGlyph.length > 2) {
errors.push({ msg: 'Custom glyph must be at most 2 characters' });
Comment on lines 74 to 75
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 validation for customGlyph only checks the length but does not sanitize or escape the input, which could lead to security vulnerabilities if harmful input is submitted. To enhance security, consider implementing input sanitization for customGlyph.

For example, using a library like sanitize-html:

const sanitizeHtml = require('sanitize-html');
customGlyph = sanitizeHtml(customGlyph);

Expand Down
13 changes: 9 additions & 4 deletions tests/server/routes/index.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,20 +7,25 @@ const express = require('express');
// Mock dependencies
jest.mock('../../../server/models/User', () => ({
findRecent: jest.fn().mockResolvedValue([
{ username: 'testuser1', displayName: 'Test User 1' },
{ username: 'testuser2', displayName: 'Test User 2' }
{ username: 'testuser1', displayName: 'Test User 1', lastActive: new Date() },
{ username: 'testuser2', displayName: 'Test User 2', lastActive: new Date() }
]),
countDocuments: jest.fn().mockResolvedValue(2),
find: jest.fn().mockResolvedValue([
{ username: 'testuser1', lastActive: new Date() }
Comment on lines +10 to +15
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 use of new Date() within the mocked return values for findRecent and find methods introduces non-deterministic behavior in tests. Each invocation of new Date() generates a new timestamp, which can lead to inconsistent test results if the exact time of object creation becomes relevant in assertions or logic.

Recommendation:
To ensure consistent test outcomes, consider using a fixed timestamp or a mocking library like jest.useFakeTimers() to control time-related functions. This approach will make the tests deterministic and more reliable.

])
}));

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
51 changes: 51 additions & 0 deletions tests/server/routes/users.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
const request = require('supertest');
const express = require('express');

// Mock User model methods used in the route
jest.mock('../../../server/models/User', () => ({
findOne: jest.fn().mockResolvedValue(null),
create: jest.fn().mockResolvedValue({ id: 1 })
}));

// Create mock app
const app = express();
app.use(express.urlencoded({ extended: false }));
app.use(express.json());
app.set('view engine', 'handlebars');
app.set('views', './server/views');
app.use((req, res, next) => {
req.flash = jest.fn();
next();
});
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.

suggestion (testing): Add a test case to ensure valid emails pass the format validation.

Including a test with a valid email, such as [email protected], will confirm that the validation allows correct formats and does not trigger errors for valid input.

app.use((req, res, next) => {
res.render = jest.fn().mockImplementation((view, options) => {
res.status(200).send({ view, options });
});
next();
});

const usersRouter = require('../../../server/routes/users');
app.use('/users', usersRouter);

describe('Users Routes', () => {
describe('POST /users/register', () => {
it('should return an error for invalid email', async () => {
const response = await request(app)
.post('/users/register')
.send({
username: 'testuser',
email: 'invalid-email',
password: 'password',
password2: 'password'
});

expect(response.status).toBe(200);
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 expects a HTTP 200 status code even when the email provided is invalid. Typically, a 4xx status code (e.g., 400 Bad Request) should be used to indicate client-side input errors. Using HTTP 200 might be misleading as it indicates a successful request.

Recommendation: Modify the expected status code to reflect client errors appropriately, such as expecting a 400 status code for invalid inputs.

expect(response.body.view).toBe('users/register');
expect(response.body.options.errors).toEqual(
expect.arrayContaining([
expect.objectContaining({ msg: 'Invalid email address' })
])
);
});
});
});