JS: Should isNaN(parseInt(x)) count as a check that x is a number, given that parseInt only reads a numeric prefix? #23115

Description of the issue

TaintTracking::isNumberGuard treats isNaN(parseInt(x)) and isNaN(parseFloat(x)) as a check that x is a number, and js/shell-command-constructed-from-input uses it as a barrier guard for x. I'm wondering whether this is intended, because parseInt and parseFloat only read a numeric prefix and ignore the rest of the string: parseInt("1; touch /tmp/marker") is 1, so the check passes, while x itself is still the whole string.

Affected sanitizer

isNumberGuard holds for isNaN(e) with the false outcome, where the guarded operand is e itself or, if e is a call to parseInt or parseFloat, the argument of that call (L992-L997). The other forms are fine: isNaN(x), isNaN(+x) and isNaN(x - 0) convert the whole value with Number, so they do restrict x.

js/shell-command-constructed-from-input uses the predicate through NumberGuard, and the query test sanitizer4 expects no alert for cp.exec("rm -rf " + name) in the else branches of isNaN(parseInt(name)) and isNaN(parseInt(name, 10)) (L521 and L533), although name is unchanged there. The same predicate is used by the NumberGuard classes in UnsafeJQueryPluginCustomizations.qll (which js/xss-through-dom also uses), UnvalidatedDynamicMethodCallCustomizations.qll, PrototypePollutingAssignmentQuery.qll and TaintedObject.qll. The guard was added in #7892.

Steps to reproduce

index.js, the main module of a package ("main": "index.js" in package.json):

const cp = require("child_process");

// Returns the command name of a process, e.g. processName(1234).
module.exports.processName = function (pid) {
  if (isNaN(parseInt(pid))) {
    throw new Error("pid must be a number");
  }
  return cp.execSync("ps -o comm= -p " + pid).toString(); // not reported
};

module.exports.processNameFloat = function (pid) {
  if (isNaN(parseFloat(pid))) {
    throw new Error("pid must be a number");
  }
  return cp.execSync("ps -o comm= -p " + pid).toString(); // not reported
};

module.exports.processNameStrict = function (pid) {
  if (isNaN(pid)) {
    throw new Error("pid must be a number");
  }
  return cp.execSync("ps -o comm= -p " + pid).toString(); // not reported (correct: isNaN(pid) converts the whole string)
};

module.exports.processNameUnchecked = function (pid) {
  return cp.execSync("ps -o comm= -p " + pid).toString(); // reported
};

page.js:

function showCount() {
  const v = document.getElementById("count").value;
  if (isNaN(parseInt(v))) {
    return;
  }
  document.getElementById("out").innerHTML = "Count: " + v; // not reported
}

function showCountUnchecked() {
  const v = document.getElementById("count").value;
  document.getElementById("out").innerHTML = "Count: " + v; // reported
}

With javascript-security-extended.qls, only index.js:26 (js/shell-command-constructed-from-input) and page.js:11 (js/xss-through-dom) are reported. At runtime (Node.js v24.18.0), calling processName("1; touch /tmp/marker") passes the check and runs ps -o comm= -p 1; touch /tmp/marker, which creates the file; processNameFloat does the same. processNameStrict throws for the same input. In page.js, parseInt("1<img src=x onerror=alert(1)>") is 1, so that value passes the check and reaches innerHTML.

Question

Is it intended that isNaN(parseInt(x)) and isNaN(parseFloat(x)) sanitize x itself? Would it be reasonable to drop the parseInt/parseFloat case, so that only checks that convert the whole value (such as isNaN(x), isNaN(+x) and isNaN(x - 0)) are treated as number checks?

Environment

• CodeQL CLI 2.27.4 (codeql/javascript-queries 2.5.0, codeql/javascript-all 3.0.0)
• CodeQL repository commit for the links above: fbf9fca5ce7c3b64c2f5d719ea0444577850cb4a (the files are unchanged since 2.27.4)
• Node.js v24.18.0 on Linux/amd64