Skip to content

JS: Stop treating parseInt and parseFloat as whole-string number guards #23121

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

Merged
sam-robson merged 2 commits into from
Nov 13, 2026
Merged

JS: Stop treating parseInt and parseFloat as whole-string number guards #23121

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
162de33
fix(js): stop treating parseInt and parseFloat as whole-string number…
sam-robson Nov 12, 2026
1317435
fix(js): preserve parseInt and parseFloat guard for prototype pollution
sam-robson Nov 13, 2026
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
4 changes: 4 additions & 0 deletions javascript/ql/lib/change-notes/2026-11-12-isnumberguard-parseint.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* Checks of the form `isNaN(parseInt(x))` and `isNaN(parseFloat(x))` are no longer treated as checking that `x` is a number, since `parseInt` and `parseFloat` only validate a numeric prefix of `x`. This can reveal previously missed alerts, for example from `js/shell-command-constructed-from-input` and `js/unsafe-jquery-plugin`.
7 changes: 0 additions & 7 deletions javascript/ql/lib/semmle/javascript/dataflow/TaintTracking.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -989,13 +989,6 @@ module TaintTracking {
|
operand = isNaN.getArgument(0).asExpr()
or

@owen-mc owen-mc Nov 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should be investigated before merging.

exists(DataFlow::CallNode parse |
parse = DataFlow::globalVarRef(["parseInt", "parseFloat"]).getACall()
|
parse = isNaN.getArgument(0) and
operand = parse.getArgument(0).asExpr()
)
or
Comment on lines -992 to -998

@owen-mc owen-mc Nov 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A corresponding disjunct should be added to the characteristic predicate of NumberGuard for prototype pollution, as the case that is being removed is still valid for the purpose of that class.

@sam-robson sam-robson Nov 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

👍🏻 Added in 1317435

exists(UnaryExpr unary | unary.getOperator() = ["+", "-"] |
unary = isNaN.getArgument(0).asExpr() and
operand = unary.getOperand()
Expand Down
17 changes: 15 additions & 2 deletions javascript/ql/lib/semmle/javascript/security/dataflow/PrototypePollutingAssignmentQuery.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,12 +312,25 @@ private class TypeofCheck extends BarrierGuard, DataFlow::ValueNode {
}
}

/** A guard that checks whether `x` is a number. */
/**
* A guard that checks whether `x` is a number, or starts with a numeric prefix
* that rules out `"__proto__"`.
*/
class NumberGuard extends BarrierGuard instanceof DataFlow::CallNode {
Expr x;
boolean polarity;

NumberGuard() { TaintTracking::isNumberGuard(this, x, polarity) }
NumberGuard() {
TaintTracking::isNumberGuard(this, x, polarity)
or
this = DataFlow::globalVarRef("isNaN").getACall() and
polarity = false and
exists(DataFlow::CallNode parse |
parse = DataFlow::globalVarRef(["parseInt", "parseFloat"]).getACall() and
parse = super.getArgument(0) and
x = parse.getArgument(0).asExpr()
)
}

override predicate blocksExpr(boolean outcome, Expr e) { e = x and outcome = polarity }
}
Expand Down
13 changes: 13 additions & 0 deletions ...s/Security/CWE-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,10 @@
| lib/lib.js:510:10:510:25 | "rm -rf " + name | lib/lib.js:509:39:509:42 | name | lib/lib.js:510:22:510:25 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:509:39:509:42 | name | library input | lib/lib.js:510:2:510:26 | cp.exec ... + name) | shell command |
| lib/lib.js:513:11:513:26 | "rm -rf " + name | lib/lib.js:509:39:509:42 | name | lib/lib.js:513:23:513:26 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:509:39:509:42 | name | library input | lib/lib.js:513:3:513:27 | cp.exec ... + name) | shell command |
| lib/lib.js:519:11:519:26 | "rm -rf " + name | lib/lib.js:509:39:509:42 | name | lib/lib.js:519:23:519:26 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:509:39:509:42 | name | library input | lib/lib.js:519:3:519:27 | cp.exec ... + name) | shell command |
| lib/lib.js:521:11:521:26 | "rm -rf " + name | lib/lib.js:509:39:509:42 | name | lib/lib.js:521:23:521:26 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:509:39:509:42 | name | library input | lib/lib.js:521:3:521:27 | cp.exec ... + name) | shell command |
| lib/lib.js:525:11:525:26 | "rm -rf " + name | lib/lib.js:509:39:509:42 | name | lib/lib.js:525:23:525:26 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:509:39:509:42 | name | library input | lib/lib.js:525:3:525:27 | cp.exec ... + name) | shell command |
| lib/lib.js:531:11:531:26 | "rm -rf " + name | lib/lib.js:509:39:509:42 | name | lib/lib.js:531:23:531:26 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:509:39:509:42 | name | library input | lib/lib.js:531:3:531:27 | cp.exec ... + name) | shell command |
| lib/lib.js:533:11:533:26 | "rm -rf " + name | lib/lib.js:509:39:509:42 | name | lib/lib.js:533:23:533:26 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:509:39:509:42 | name | library input | lib/lib.js:533:3:533:27 | cp.exec ... + name) | shell command |
| lib/lib.js:537:11:537:26 | "rm -rf " + name | lib/lib.js:509:39:509:42 | name | lib/lib.js:537:23:537:26 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:509:39:509:42 | name | library input | lib/lib.js:537:3:537:27 | cp.exec ... + name) | shell command |
| lib/lib.js:543:11:543:26 | "rm -rf " + name | lib/lib.js:509:39:509:42 | name | lib/lib.js:543:23:543:26 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:509:39:509:42 | name | library input | lib/lib.js:543:3:543:27 | cp.exec ... + name) | shell command |
| lib/lib.js:545:11:545:26 | "rm -rf " + name | lib/lib.js:509:39:509:42 | name | lib/lib.js:545:23:545:26 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:509:39:509:42 | name | library input | lib/lib.js:545:3:545:27 | cp.exec ... + name) | shell command |
Expand Down Expand Up @@ -117,6 +119,8 @@
| lib/lib.js:729:26:729:29 | args | lib/lib.js:643:48:643:51 | name | lib/lib.js:729:26:729:29 | args | This shell argument which depends on $@ is later used in a $@. | lib/lib.js:643:48:643:51 | name | library input | lib/lib.js:729:9:729:47 | cp.spaw ... true }) | shell command |
| lib/lib.js:730:29:730:32 | args | lib/lib.js:643:48:643:51 | name | lib/lib.js:730:29:730:32 | args | This shell argument which depends on $@ is later used in a $@. | lib/lib.js:643:48:643:51 | name | library input | lib/lib.js:730:9:730:55 | cp.exec ... /sh" }) | shell command |
| lib/lib.js:736:26:736:31 | [name] | lib/lib.js:643:48:643:51 | name | lib/lib.js:736:26:736:31 | [name] | This shell argument which depends on $@ is later used in a $@. | lib/lib.js:643:48:643:51 | name | library input | lib/lib.js:736:9:736:41 | cp.spaw ... ptions) | shell command |
| lib/lib.js:752:11:752:26 | "rm -rf " + name | lib/lib.js:750:48:750:51 | name | lib/lib.js:752:23:752:26 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:750:48:750:51 | name | library input | lib/lib.js:752:3:752:27 | cp.exec ... + name) | shell command |
| lib/lib.js:754:11:754:26 | "rm -rf " + name | lib/lib.js:750:48:750:51 | name | lib/lib.js:754:23:754:26 | name | This string concatenation which depends on $@ is later used in a $@. | lib/lib.js:750:48:750:51 | name | library input | lib/lib.js:754:3:754:27 | cp.exec ... + name) | shell command |
| lib/subLib2/compiled-file.ts:4:13:4:28 | "rm -rf " + name | lib/subLib2/compiled-file.ts:3:26:3:29 | name | lib/subLib2/compiled-file.ts:4:25:4:28 | name | This string concatenation which depends on $@ is later used in a $@. | lib/subLib2/compiled-file.ts:3:26:3:29 | name | library input | lib/subLib2/compiled-file.ts:4:5:4:29 | cp.exec ... + name) | shell command |
| lib/subLib2/special-file.js:4:10:4:25 | "rm -rf " + name | lib/subLib2/special-file.js:3:28:3:31 | name | lib/subLib2/special-file.js:4:22:4:25 | name | This string concatenation which depends on $@ is later used in a $@. | lib/subLib2/special-file.js:3:28:3:31 | name | library input | lib/subLib2/special-file.js:4:2:4:26 | cp.exec ... + name) | shell command |
| lib/subLib3/my-file.ts:4:10:4:25 | "rm -rf " + name | lib/subLib3/my-file.ts:3:28:3:31 | name | lib/subLib3/my-file.ts:4:22:4:25 | name | This string concatenation which depends on $@ is later used in a $@. | lib/subLib3/my-file.ts:3:28:3:31 | name | library input | lib/subLib3/my-file.ts:4:2:4:26 | cp.exec ... + name) | shell command |
Expand Down Expand Up @@ -237,8 +241,10 @@ edges
| lib/lib.js:509:39:509:42 | name | lib/lib.js:510:22:510:25 | name | provenance | |
| lib/lib.js:509:39:509:42 | name | lib/lib.js:513:23:513:26 | name | provenance | |
| lib/lib.js:509:39:509:42 | name | lib/lib.js:519:23:519:26 | name | provenance | |
| lib/lib.js:509:39:509:42 | name | lib/lib.js:521:23:521:26 | name | provenance | |
| lib/lib.js:509:39:509:42 | name | lib/lib.js:525:23:525:26 | name | provenance | |
| lib/lib.js:509:39:509:42 | name | lib/lib.js:531:23:531:26 | name | provenance | |
| lib/lib.js:509:39:509:42 | name | lib/lib.js:533:23:533:26 | name | provenance | |
| lib/lib.js:509:39:509:42 | name | lib/lib.js:537:23:537:26 | name | provenance | |
| lib/lib.js:509:39:509:42 | name | lib/lib.js:543:23:543:26 | name | provenance | |
| lib/lib.js:509:39:509:42 | name | lib/lib.js:545:23:545:26 | name | provenance | |
Expand Down Expand Up @@ -375,6 +381,8 @@ edges
| lib/lib.js:734:14:734:27 | runWithOptions [name] | lib/lib.js:738:5:738:18 | runWithOptions [name] | provenance | |
| lib/lib.js:736:27:736:30 | name | lib/lib.js:736:26:736:31 | [name] | provenance | |
| lib/lib.js:738:5:738:18 | runWithOptions [name] | lib/lib.js:736:27:736:30 | name | provenance | |
| lib/lib.js:750:48:750:51 | name | lib/lib.js:752:23:752:26 | name | provenance | |
| lib/lib.js:750:48:750:51 | name | lib/lib.js:754:23:754:26 | name | provenance | |
| lib/subLib2/compiled-file.ts:3:26:3:29 | name | lib/subLib2/compiled-file.ts:4:25:4:28 | name | provenance | |
| lib/subLib2/special-file.js:3:28:3:31 | name | lib/subLib2/special-file.js:4:22:4:25 | name | provenance | |
| lib/subLib3/my-file.ts:3:28:3:31 | name | lib/subLib3/my-file.ts:4:22:4:25 | name | provenance | |
Expand Down Expand Up @@ -542,8 +550,10 @@ nodes
| lib/lib.js:510:22:510:25 | name | semmle.label | name |
| lib/lib.js:513:23:513:26 | name | semmle.label | name |
| lib/lib.js:519:23:519:26 | name | semmle.label | name |
| lib/lib.js:521:23:521:26 | name | semmle.label | name |
| lib/lib.js:525:23:525:26 | name | semmle.label | name |
| lib/lib.js:531:23:531:26 | name | semmle.label | name |
| lib/lib.js:533:23:533:26 | name | semmle.label | name |
| lib/lib.js:537:23:537:26 | name | semmle.label | name |
| lib/lib.js:543:23:543:26 | name | semmle.label | name |
| lib/lib.js:545:23:545:26 | name | semmle.label | name |
Expand Down Expand Up @@ -646,6 +656,9 @@ nodes
| lib/lib.js:736:26:736:31 | [name] | semmle.label | [name] |
| lib/lib.js:736:27:736:30 | name | semmle.label | name |
| lib/lib.js:738:5:738:18 | runWithOptions [name] | semmle.label | runWithOptions [name] |
| lib/lib.js:750:48:750:51 | name | semmle.label | name |
| lib/lib.js:752:23:752:26 | name | semmle.label | name |
| lib/lib.js:754:23:754:26 | name | semmle.label | name |
| lib/subLib2/compiled-file.ts:3:26:3:29 | name | semmle.label | name |
| lib/subLib2/compiled-file.ts:4:25:4:28 | name | semmle.label | name |
| lib/subLib2/special-file.js:3:28:3:31 | name | semmle.label | name |
Expand Down
21 changes: 17 additions & 4 deletions javascript/ql/test/query-tests/Security/CWE-078/UnsafeShellCommandConstruction/lib/lib.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -515,10 +515,10 @@ module.exports.sanitizer4 = function (name) { // $ Source
cp.exec("rm -rf " + name);
}

if (isNaN(parseInt(name))) {
if (isNaN(parseInt(name))) { // <- only checks a numeric prefix
cp.exec("rm -rf " + name); // $ Alert
} else {
cp.exec("rm -rf " + name);
cp.exec("rm -rf " + name); // $ Alert
}

if (isNaN(+name)) {
Expand All @@ -527,10 +527,10 @@ module.exports.sanitizer4 = function (name) { // $ Source
cp.exec("rm -rf " + name);
}

if (isNaN(parseInt(name, 10))) {
if (isNaN(parseInt(name, 10))) { // <- only checks a numeric prefix
cp.exec("rm -rf " + name); // $ Alert
} else {
cp.exec("rm -rf " + name);
cp.exec("rm -rf " + name); // $ Alert
}

if (isNaN(name - 0)) {
Expand Down Expand Up @@ -746,3 +746,16 @@ module.exports.shellOptionsOnly = function (options) {
cp.execFile("echo", options, () => {});
cp.execFileSync("echo", options);
};

module.exports.sanitizerParseFloat = function (name) { // $ Source
if (isNaN(parseFloat(name))) { // <- only checks a numeric prefix
cp.exec("rm -rf " + name); // $ Alert
} else {
cp.exec("rm -rf " + name); // $ Alert
}

const parsed = parseInt(name, 10);
if (!isNaN(parsed)) {
cp.exec("rm -rf " + parsed);
}
}
15 changes: 15 additions & 0 deletions ...tests/Security/CWE-915/PrototypePollutingAssignment/PrototypePollutingAssignment.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@
| tst.js:97:5:97:46 | obj[req ... g, '')] | tst.js:97:9:97:19 | req.query.x | tst.js:97:5:97:46 | obj[req ... g, '')] | This assignment may alter Object.prototype if a malicious '__proto__' string is injected from $@. | tst.js:97:9:97:19 | req.query.x | user controlled input |
| tst.js:105:5:105:17 | object[taint] | tst.js:102:24:102:37 | req.query.data | tst.js:105:5:105:17 | object[taint] | This assignment may alter Object.prototype if a malicious '__proto__' string is injected from $@. | tst.js:102:24:102:37 | req.query.data | user controlled input |
| tst.js:130:5:130:53 | obj[req ... ), '')] | tst.js:130:9:130:19 | req.query.x | tst.js:130:5:130:53 | obj[req ... ), '')] | This assignment may alter Object.prototype if a malicious '__proto__' string is injected from $@. | tst.js:130:9:130:19 | req.query.x | user controlled input |
| tst.js:139:9:139:16 | obj[key] | tst.js:135:22:135:34 | req.query.key | tst.js:139:9:139:16 | obj[key] | This assignment may alter Object.prototype if a malicious '__proto__' string is injected from $@. | tst.js:135:22:135:34 | req.query.key | user controlled input |
| tst.js:145:9:145:16 | obj[key] | tst.js:135:22:135:34 | req.query.key | tst.js:145:9:145:16 | obj[key] | This assignment may alter Object.prototype if a malicious '__proto__' string is injected from $@. | tst.js:135:22:135:34 | req.query.key | user controlled input |
edges
| lib.js:1:38:1:40 | obj | lib.js:6:7:6:9 | obj | provenance | |
| lib.js:1:43:1:46 | path | lib.js:2:21:2:24 | path | provenance | |
Expand Down Expand Up @@ -136,6 +138,12 @@ edges
| tst.js:105:12:105:16 | taint | tst.js:105:5:105:17 | object[taint] | provenance | Config |
| tst.js:130:9:130:19 | req.query.x | tst.js:130:9:130:52 | req.que ... '), '') | provenance | Config |
| tst.js:130:9:130:52 | req.que ... '), '') | tst.js:130:5:130:53 | obj[req ... ), '')] | provenance | Config |
| tst.js:135:9:135:11 | key | tst.js:139:13:139:15 | key | provenance | |
| tst.js:135:9:135:11 | key | tst.js:145:13:145:15 | key | provenance | |
| tst.js:135:15:135:35 | String( ... ry.key) | tst.js:135:9:135:11 | key | provenance | |
| tst.js:135:22:135:34 | req.query.key | tst.js:135:15:135:35 | String( ... ry.key) | provenance | Config |
| tst.js:139:13:139:15 | key | tst.js:139:9:139:16 | obj[key] | provenance | Config |
| tst.js:145:13:145:15 | key | tst.js:145:9:145:16 | obj[key] | provenance | Config |
nodes
| lib.js:1:38:1:40 | obj | semmle.label | obj |
| lib.js:1:43:1:46 | path | semmle.label | path |
Expand Down Expand Up @@ -265,5 +273,12 @@ nodes
| tst.js:130:5:130:53 | obj[req ... ), '')] | semmle.label | obj[req ... ), '')] |
| tst.js:130:9:130:19 | req.query.x | semmle.label | req.query.x |
| tst.js:130:9:130:52 | req.que ... '), '') | semmle.label | req.que ... '), '') |
| tst.js:135:9:135:11 | key | semmle.label | key |
| tst.js:135:15:135:35 | String( ... ry.key) | semmle.label | String( ... ry.key) |
| tst.js:135:22:135:34 | req.query.key | semmle.label | req.query.key |
| tst.js:139:9:139:16 | obj[key] | semmle.label | obj[key] |
| tst.js:139:13:139:15 | key | semmle.label | key |
| tst.js:145:9:145:16 | obj[key] | semmle.label | obj[key] |
| tst.js:145:13:145:15 | key | semmle.label | key |
subpaths
| tst.js:14:27:14:31 | taint | tst.js:55:29:55:32 | prop | tst.js:56:12:56:33 | obj ? o ... : null | tst.js:14:5:14:32 | unsafeG ... taint) |
17 changes: 17 additions & 0 deletions javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingAssignment/tst.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,3 +130,20 @@ app.get('/foo', (req, res) => {
obj[req.query.x.replace(new RegExp('_', ''), '')].x = 'foo'; // $ Alert
obj[req.query.x.replace(new RegExp('_', unknownFlags()), '')].x = 'foo';
});

app.get('/number-guard', (req, res) => {
let key = String(req.query.key); // $ Source
let obj = {};

if (isNaN(parseInt(key))) {
obj[key].x = 'foo'; // $ Alert
} else {
obj[key].x = 'foo';
}

if (isNaN(parseFloat(key))) {
obj[key].x = 'foo'; // $ Alert
} else {
obj[key].x = 'foo';
}
});
Loading