Skip to content

Ensure email is lowercase and check availability on user creation#5442

Open
ChinoKou wants to merge 3 commits into
NginxProxyManager:developfrom
ChinoKou:develop
Open

Ensure email is lowercase and check availability on user creation#5442
ChinoKou wants to merge 3 commits into
NginxProxyManager:developfrom
ChinoKou:develop

Conversation

@ChinoKou
Copy link
Copy Markdown

This pull request adds email normalization and uniqueness validation to the user creation logic in backend/internal/user.js. Now, emails are converted to lowercase and trimmed, and the system checks if the email is already in use before proceeding.

User validation improvements:

  • Emails are now normalized (lowercased and trimmed) before processing to ensure consistency.
  • Added a uniqueness check for emails during user creation, throwing a validation error if the email is already in use.

@ChinoKou
Copy link
Copy Markdown
Author

Fixes: #4983 #5323

@ChinoKou ChinoKou marked this pull request as ready for review March 30, 2026 15:05
@jc21
Copy link
Copy Markdown
Member

jc21 commented May 14, 2026

Code Review

Thanks for this PR — closing the gap between create and update is the right thing to do. The update path already had both of these checks, so it's good to bring create in line.

A couple of issues worth addressing before merging:


1. Missing null/undefined guard (medium)

data.email = data.email.toLowerCase().trim();

If data.email is null or undefined, this throws an unhandled TypeError rather than a clean validation error. The update path guards against this first:

if (typeof data.email !== "undefined") {
    data.email = data.email.toLowerCase().trim();

Even if upstream schema validation currently guarantees email is present, matching this pattern would be more defensive and consistent.


2. Uniqueness check runs before access control (low)

The new block is inserted before access.can("users:create", data). This means an unauthenticated or unauthorized caller can probe whether an email is already in use before being rejected for lack of permissions. The check should move to after the access check.


3. Explicit undefined argument (nit)

await internalUser.isEmailAvailable(data.email, undefined);

Passing undefined explicitly adds noise — omitting the second argument or passing null would be cleaner.


Suggested ordering

await access.can("users:create", data); // access check first

if (typeof data.email !== "undefined" && data.email !== null) {
    data.email = data.email.toLowerCase().trim();
    const emailAvailable = await internalUser.isEmailAvailable(data.email);
    if (!emailAvailable) {
        throw new errs.ValidationError(`Email address already in use - ${data.email}`);
    }
}

data.avatar = gravatar.url(data.email, { default: "mm" });

The error message format and the overall approach are consistent with the existing update logic — just the ordering and guard need a tweak.

Copilot AI review requested due to automatic review settings May 14, 2026 02:06
@ChinoKou
Copy link
Copy Markdown
Author

Code Review

Thanks for this PR — closing the gap between create and update is the right thing to do. The update path already had both of these checks, so it's good to bring create in line.

A couple of issues worth addressing before merging:

1. Missing null/undefined guard (medium)

data.email = data.email.toLowerCase().trim();

If data.email is null or undefined, this throws an unhandled TypeError rather than a clean validation error. The update path guards against this first:

if (typeof data.email !== "undefined") {
    data.email = data.email.toLowerCase().trim();

Even if upstream schema validation currently guarantees email is present, matching this pattern would be more defensive and consistent.

2. Uniqueness check runs before access control (low)

The new block is inserted before access.can("users:create", data). This means an unauthenticated or unauthorized caller can probe whether an email is already in use before being rejected for lack of permissions. The check should move to after the access check.

3. Explicit undefined argument (nit)

await internalUser.isEmailAvailable(data.email, undefined);

Passing undefined explicitly adds noise — omitting the second argument or passing null would be cleaner.

Suggested ordering

await access.can("users:create", data); // access check first

if (typeof data.email !== "undefined" && data.email !== null) {
    data.email = data.email.toLowerCase().trim();
    const emailAvailable = await internalUser.isEmailAvailable(data.email);
    if (!emailAvailable) {
        throw new errs.ValidationError(`Email address already in use - ${data.email}`);
    }
}

data.avatar = gravatar.url(data.email, { default: "mm" });

The error message format and the overall approach are consistent with the existing update logic — just the ordering and guard need a tweak.

Thanks for the great feedback! I've pushed the updates to fix the ordering, add the null guard, and clean up the method call.

Also, spot on about the validation. The frontend already checks this, but I completely agree the backend check is necessary. It adds a solid layer of defense-in-depth for direct API calls.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR improves user creation in the backend by normalizing emails (trim + lowercase) and checking whether the email is already in use before inserting a new user.

Changes:

  • Normalize data.email during user creation (toLowerCase().trim()).
  • Add a pre-insert uniqueness check via internalUser.isEmailAvailable() and throw a ValidationError if taken.
Comments suppressed due to low confidence (1)

backend/internal/user.js:44

  • isEmailAvailable() compares with where("email", "=", normalizedEmail), which can be case-sensitive depending on the DB/collation. If any existing rows contain mixed/upper-case emails from before this normalization, this check may miss them and allow duplicates. Consider using a case-insensitive comparison (e.g., LOWER(email) = ?) or migrating existing user emails to the normalized form before relying on this check.
			data.email = data.email.toLowerCase().trim();
			const emailAvailable = await internalUser.isEmailAvailable(data.email);
			if (!emailAvailable) {
				throw new errs.ValidationError(`Email address already in use - ${data.email}`);
			}

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread backend/internal/user.js
Comment on lines +40 to +44
data.email = data.email.toLowerCase().trim();
const emailAvailable = await internalUser.isEmailAvailable(data.email);
if (!emailAvailable) {
throw new errs.ValidationError(`Email address already in use - ${data.email}`);
}
Comment thread backend/internal/user.js
Comment on lines +39 to +45
if (typeof data.email !== "undefined" && data.email !== null) {
data.email = data.email.toLowerCase().trim();
const emailAvailable = await internalUser.isEmailAvailable(data.email);
if (!emailAvailable) {
throw new errs.ValidationError(`Email address already in use - ${data.email}`);
}
}
Comment thread backend/internal/user.js
Comment on lines +39 to +45
if (typeof data.email !== "undefined" && data.email !== null) {
data.email = data.email.toLowerCase().trim();
const emailAvailable = await internalUser.isEmailAvailable(data.email);
if (!emailAvailable) {
throw new errs.ValidationError(`Email address already in use - ${data.email}`);
}
}
@nginxproxymanagerci
Copy link
Copy Markdown

Docker Image for build 2 is available on DockerHub:

nginxproxymanager/nginx-proxy-manager-dev:pr-5442

Note

Ensure you backup your NPM instance before testing this image! Especially if there are database changes.
This is a different docker image namespace than the official image.

Warning

Changes and additions to DNS Providers require verification by at least 2 members of the community!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants