Injection
First, close the injection vulnerability. User input should never be trusted, and never passed directly to another subsystem. The best thing to do is pass user data in a side-channel; in SQL, this means using prepared queries. For example, instead of connection.query, you could use connection.execute:
connection.execute(
'SELECT Count(*) AS usernameCount FROM adminuser WHERE username=?',
[username],
(err, data) => {...}
)
The available MySQL driver may instead only support escaping arguments and simulate prepared statements, but that's better than nothing (though if that's the case, you should seriously consider switching to one that supports prepared statements):
connection.query(
mysql.format('SELECT Count(*) AS usernameCount FROM adminuser WHERE username=?', [username]),
...
Logic Error
Take a closer look at the block in the multiple statement example:
if(password === confirm){
if(error_mssg.lenght > 0){
*******query to insert user information*******
}else{
res.send(error_mssg)
}
...
Note that the user information is inserted if there are error messages, and the (empty) error message list is returned if not. This is the opposite of what you want. Debuggers can help catch this sort of logic error.
Rather than only sending the existence error messages if the passwords do not match, you could instead send all error messages. This way, a user can correct all errors at once, rather than having to submit only to discover other errors. This will also better separate out request validation from request handling.
if (password !== confirm) {
error_msgs.push("passwords do not match");
}
if (error_msgs.length) {
res.send(error_msgs);
} else {
// create new user
...
}
Existence Testing
Another approach to querying for existing records is to use a single query to check for existing usernames and e-mail addresses in the adminuser database. This has an efficiency advantage over chained queries or awaits because one query doesn't need to wait for the other to finish (though the performance cost of the latter two is likely negligible).
Since the two fields are separate columns in the same database, a query is straightforward:
SELECT username, email FROM adminuser WHERE username=? OR email=?;
The data requirement that usernames and e-mails should be unique should be reflected in the schema by the use of UNIQUE indices. This will limit the result rows to at most 2 (and the max occurrence of each value to 1), which can then be checked for the presence of the unique values.
connection.execute(
'SELECT username, email FROM adminuser WHERE username=? OR email=?;',
[username, email],
(err, data) => {
if (err) throw err;
/*** Validate user creation request ***/
/* There should be at most 2 rows, with at most 1 occurrence each of `username` and `email`. */
for (var iRow=0; iRow < data.length; ++iRow) {
if (username == data[iRow].username) {
error_msgs.push("username already exists");
}
if (email == data[iRow].email) {
error_msgs.push("email already exists");
}
}
if (password !== confirm) {
error_msgs.push("passwords do not match");
}
/*** Handle user creation request ***/
if (error_msgs.length) {
res.send(error_msgs);
} else {
// create new user
...
}
}
);
The existence checking can be made more robust (in case something goes wrong with the database and multiple occurrences of a username or email are returned) at the cost of a more complex SQL statement. If supported by the driver, named placeholders (instead of '?') can be used both for clarity and to reduce redundancy.
// namedPlaceholders can also be an `execute` option
connection.config.namedPlaceholders = true
...
connection.execute(
'SELECT Sum(If(:username=username, 1, 0) As usernameCount,
Sum(If(:email=email, 1, 0) AS emailCount
FROM adminuser
WHERE username=:username OR email=:email;',
{ username, email }, // you could instead pass req.body, assuming it's been checked for 'username' and 'email' properties
(err, data) => {
if (err) throw err;
/*** Validate user creation request ***/
if (data[0].usernameCount) {
error_msgs.push("username already exists");
}
if (data[0].emailCount) {
error_msgs.push("email already exists");
}
if (password !== confirm) {
error_msgs.push("passwords do not match");
}
/*** Handle user creation request ***/
if (error_msgs.length) {
res.send(error_msgs);
} else {
// create new user
...
}
});
Additional design considerations
The sample code mixes a couple of different concerns: database access and request handling (validation &c.). Put another way, the sample code bears multiple responsibilities, which is undesirable. A code unit should have as few responsibilities as possible; a class should have a single responsibility. The most prevalent definition of single responsibility is the Single Responsibility Principle: "a class should have only one reason to change". This definition doesn't exactly apply to code units larger than classes (depending on how the reasons to change are expressed).
In the sample, consider maintenance tasks that might arise:
- adding/switching DBMS support
- updating user field validation (including adding fields)
- sending messages (to e.g. admins, mods, the new user) when a user is created
Each of these is a different reason for code to change, and thus a different responsibility.
Separating concerns will result in classes with reduced responsibilities. One module, the Data Access Layer, should handle managing users in the database (e.g. checking for username or email in existing records, adding new users), and another should handle the user creation request (validation & user creation) by making the appropriate calls to the DAL (perhaps indirectly). In particular, SQL statements and database calls shouldn't appear in the request handler. The request handler might look something like:
async function validateUserCreationRequest(req) {
const { username, email, password, confirm } = req.body;
var error_msgs = [],
userExists = await User.exists({ username, email }); // here, User is responsible for accessing the DAL
if (userExists) {
if (userExists.usernameCount) {
error_msgs.push("username already exists");
}
if (userExists.emailCount) {
error_msgs.push("email already exists");
}
}
if (password !== confirm) {
error_msgs.push("passwords do not match");
}
if (error_msgs.length) {
return error_msgs;
}
return null;
}
function createUser(req) {
return User.create(req); // again, User is responsible for accessing the DAL
}
function handleUserCreationRequest(req) {
var errors = validateUserCreationRequest(req);
if (errors) {
res.send(errors);
} else {
createUser(req);
}
}
When code bears multiple responsibilities, it's generally highly coupled (there's a high degree of interdependence between code that handles different tasks), which makes maintenance more difficult and results in more bugs. Separating those responsibilities reduces coupling and increases cohesion (the code in modules belong together, in that they're narrowly focused on what tasks they accomplish), as coupling and cohesion are inversely related.
There's a fair amount of overlap between concerns and responsibilities, but they are distinct principles.
There can be situations when these principles can be violated, but it should arise from the software requirements and be a conscious decision to do so.
