Sonar Fix (node.js backend)

This is a record for sonar fixing for backend based on node.js.

Security Hotspots

Security Hotspots aren’t necessarily issues, but they need to be reviewed to make sure they aren’t vulnerabilities.

Make sure that using a regular expression is safe here

Rule: Using regular expressions is security-sensitive

1. original code:

let re = /^\d+(\.\d+)?$/;

the rule recommend not using group() to reduce evaluate work load. 个人理解是，group内外都有\d+，可能有重复校验的问题

solution

Tried several regex but all failed, used brute force algo and solved.

static parseFloatStrict(str) {
    if (str.includes('.')) {
        const splitedStr = str.split('.');
        if (splitedStr.length > 2) {
            return false;
        } else if (!this.parseIntStrict(splitedStr[0]) || !this.parseIntStrict(splitedStr[1])){
            return false;
        } else {
            return parseFloat(str);
        }
    } else {
        return this.parseIntStrict(str);
    }
}

2. original code:

let reg = /^.*(?=.{8,20})(?=.*\d)(?=.*[A-Z])(?=.*[a-z])(?=.*[-!@#$%^&*?_.]).*$/;

solution

//NOSONAR

3. original code:

if (!body.name || !/(\w+){6,16}$/.test(body.name)){
}

solution

if (!body.name || !/^\w{6,16}$/.test(body.name))

4. original code:

if (/\/login.*/.test(req.path) || /.*login\/*/.test(req.headers.referer) || req.path.startsWith("/api/") || req.path === "/") {  
}

solution

if (... || /\/login.*/.test(req.headers.referer) || ...)

5. original code:

var pattern1 = /SubNetwork=.*?MeContext=.*?ManagedElement=.*?MceFunction=.*?TermPointToMmeM3=[\s\S]*?TermPointToMmeM3Id[\s\S]*?ipAddress[\s\S]*?ipv6Address2.*/g;

solution

//NOSONAR

---

Make sure that executing SQL queries is safe here.

Rule: Formatting SQL queries is security-sensitive

1. original code:

var p = sequelize.query(`${sql}`, {type: sequelize.QueryTypes.SELECT});

solution

var p = sequelize.query(sql, {type: sequelize.QueryTypes.SELECT});

Note: not a good solution, just exploited loopholes. cuz This rule’s current implementation does not follow variables. It will only detect SQL queries which are formatted directly in the function call.

---

Review this potentially hardcoded credential.

Rule: Hard-coded credentials are security-sensitive

1. original code:

const initUser = [
    {
        userId: "admin",
        password: "8c6976e5b5410415bde908bd4dee15dfb167a9c873fc4bb8a81f6f2ab448a918",
        roles: "Admin",
        userExpiryDate: new Date(new Date().getTime() - 24 * 60 * 60 * 1000),
    }
];
await Users.bulkCreate(initUser);
password: "8c6976e5b5410415bde908bd4dee15dfb167a9c873fc4bb8a81f6f2ab448a918",

solution

//NOSONAR

This solution failed, TBD.

2. original code:

data.value.password = "********";

solution

data.value.password = Setting.getDisplayPassword();
getDisplayPassword: function() {
    return "********";
},

---

Make sure that hashing data is safe here.

Rule: Hashing data is security-sensitive

1. original code:

let encryptedPass = crypto.createHash('sha256').update(newBDC.ncmPassword, 'utf8').digest('hex');

solution

The issue recommend with

use a hashing algorithm that generate its own salts as part of the hashing. If you generate your own salts, make sure that a cryptographically strong salt algorithm is used, that generated salts are credential-specific, and finally, that the salt is applied correctly before the hashing.

save both the salt and the hashed value in the relevant database record; during future validation operations, the salt and hash can then be retrieved from the database. The hash is recalculated with the stored salt and the value being validated, and the result compared to the stored hash.

Further effort TBD.

---

---

MITRE, CWE-563 - Assignment to Variable without Use (‘Unused Variable’)

Remove this useless assignment to variable “xxx”.

Rule: Unused assignments should be removed

1. original code:

let res = await client.connect();
res = await client.query(checkUserSQL);

Some explains here for this issue: https://stackoverflow.com/questions/42151535/use-of-unassigned-local-variable-when-using-a-foreach-loop

solution

client.connect() returns a promise, when succeed it returns none, if fail return a error or none:

connect(): Promise<void>;
connect(callback: (err: Error) => void): void;

so just remove the res part:

await client.connect();

2. original code:

let res = -1;
...
if (opts && opts.length > 0) {
  res = await ServerMoUtils.generateCreateAreaMO(postData, opts, profiles);
}
if (opts && opts.length > 0) {
  res = await ServerMoUtils.generateDeleteAreaMO(postData, opts);
}
if (opts) {
  res = await ServerMoUtils.generateUpdateAreaMO(postData, opts, profiles);
}

check the function:

static async generateCreateAreaMO(postData, opts, profiles) {
  ...
  opts.forEach(function(opt){
    ...
  }
  ...
  return 0;
}

As the related function used forEach, so await is a must, but found no use with return 0.

solution

remove return 0 for all three funcs, and also remove res = for each.