From 7292a76ee485d5a203bd68854403f3bdcf1819f2 Mon Sep 17 00:00:00 2001 From: Napalys Klicius Date: Thu, 12 Jun 2025 13:38:48 +0200 Subject: [PATCH 1/7] JS: add test cases for false positives in `loop-iteration-skipped-due-to-shifting` --- ...LoopIterationSkippedDueToShifting.expected | 3 +++ .../LoopIterationSkippedDueToShifting/tst.js | 25 +++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/javascript/ql/test/query-tests/Statements/LoopIterationSkippedDueToShifting/LoopIterationSkippedDueToShifting.expected b/javascript/ql/test/query-tests/Statements/LoopIterationSkippedDueToShifting/LoopIterationSkippedDueToShifting.expected index 4b7becd8e163..a11ea4d7d403 100644 --- a/javascript/ql/test/query-tests/Statements/LoopIterationSkippedDueToShifting/LoopIterationSkippedDueToShifting.expected +++ b/javascript/ql/test/query-tests/Statements/LoopIterationSkippedDueToShifting/LoopIterationSkippedDueToShifting.expected @@ -1,3 +1,6 @@ | tst.js:4:27:4:44 | parts.splice(i, 1) | Removing an array item without adjusting the loop index 'i' causes the subsequent array item to be skipped. | | tst.js:13:29:13:46 | parts.splice(i, 1) | Removing an array item without adjusting the loop index 'i' causes the subsequent array item to be skipped. | | tst.js:24:9:24:26 | parts.splice(i, 1) | Removing an array item without adjusting the loop index 'i' causes the subsequent array item to be skipped. | +| tst.js:128:11:128:33 | pending ... e(i, 1) | Removing an array item without adjusting the loop index 'i' causes the subsequent array item to be skipped. | +| tst.js:136:32:136:47 | toc.splice(i, 1) | Removing an array item without adjusting the loop index 'i' causes the subsequent array item to be skipped. | +| tst.js:143:10:143:25 | toc.splice(i, 1) | Removing an array item without adjusting the loop index 'i' causes the subsequent array item to be skipped. | diff --git a/javascript/ql/test/query-tests/Statements/LoopIterationSkippedDueToShifting/tst.js b/javascript/ql/test/query-tests/Statements/LoopIterationSkippedDueToShifting/tst.js index 68c50516da04..1f23604d76a7 100644 --- a/javascript/ql/test/query-tests/Statements/LoopIterationSkippedDueToShifting/tst.js +++ b/javascript/ql/test/query-tests/Statements/LoopIterationSkippedDueToShifting/tst.js @@ -121,3 +121,28 @@ function inspectNextElement(string) { } return parts.join('/'); } + +function withTryCatch(pendingCSS) { + for (let i = 0; i < pendingCSS.length; ++i) { + try { + pendingCSS.splice(i, 1); // $ SPURIOUS:Alert + i -= 1; + } catch (ex) {} + } +} + +function andOperand(toc) { + for (let i = 0; i < toc.length; i++) { + toc[i].ignoreSubHeading && toc.splice(i, 1) && i--; // $ SPURIOUS:Alert + } +} + +function ifStatement(toc) { + for (let i = 0; i < toc.length; i++) { + if(toc[i].ignoreSubHeading){ + if(toc.splice(i, 1)){ // $ SPURIOUS:Alert + i--; + } + } + } +} From 66d66fe87d58496354010faf3372792769522ee8 Mon Sep 17 00:00:00 2001 From: Napalys Klicius Date: Thu, 12 Jun 2025 14:51:10 +0200 Subject: [PATCH 2/7] JS: fix false positives for splice with conditional index decrement --- .../LoopIterationSkippedDueToShifting.ql | 7 ++++++- .../LoopIterationSkippedDueToShifting.expected | 3 +-- .../LoopIterationSkippedDueToShifting/tst.js | 14 ++++++++++++-- 3 files changed, 19 insertions(+), 5 deletions(-) diff --git a/javascript/ql/src/Statements/LoopIterationSkippedDueToShifting.ql b/javascript/ql/src/Statements/LoopIterationSkippedDueToShifting.ql index 08ed90777467..35e632a17fb1 100644 --- a/javascript/ql/src/Statements/LoopIterationSkippedDueToShifting.ql +++ b/javascript/ql/src/Statements/LoopIterationSkippedDueToShifting.ql @@ -146,7 +146,12 @@ class ArrayIterationLoop extends ForStmt { or this.hasPathThrough(splice, cfg.getAPredecessor()) and this.getLoopEntry().dominates(cfg.getBasicBlock()) and - not this.hasIndexingManipulation(cfg) + not this.hasIndexingManipulation(cfg) and + // Don't continue through a branch that tests the splice call's return value + not exists(ConditionGuardNode guard | cfg = guard | + guard.getTest() = splice.asExpr() and + guard.getOutcome() = false + ) } } diff --git a/javascript/ql/test/query-tests/Statements/LoopIterationSkippedDueToShifting/LoopIterationSkippedDueToShifting.expected b/javascript/ql/test/query-tests/Statements/LoopIterationSkippedDueToShifting/LoopIterationSkippedDueToShifting.expected index a11ea4d7d403..cfe2b5f4f58d 100644 --- a/javascript/ql/test/query-tests/Statements/LoopIterationSkippedDueToShifting/LoopIterationSkippedDueToShifting.expected +++ b/javascript/ql/test/query-tests/Statements/LoopIterationSkippedDueToShifting/LoopIterationSkippedDueToShifting.expected @@ -2,5 +2,4 @@ | tst.js:13:29:13:46 | parts.splice(i, 1) | Removing an array item without adjusting the loop index 'i' causes the subsequent array item to be skipped. | | tst.js:24:9:24:26 | parts.splice(i, 1) | Removing an array item without adjusting the loop index 'i' causes the subsequent array item to be skipped. | | tst.js:128:11:128:33 | pending ... e(i, 1) | Removing an array item without adjusting the loop index 'i' causes the subsequent array item to be skipped. | -| tst.js:136:32:136:47 | toc.splice(i, 1) | Removing an array item without adjusting the loop index 'i' causes the subsequent array item to be skipped. | -| tst.js:143:10:143:25 | toc.splice(i, 1) | Removing an array item without adjusting the loop index 'i' causes the subsequent array item to be skipped. | +| tst.js:153:11:153:26 | toc.splice(i, 1) | Removing an array item without adjusting the loop index 'i' causes the subsequent array item to be skipped. | diff --git a/javascript/ql/test/query-tests/Statements/LoopIterationSkippedDueToShifting/tst.js b/javascript/ql/test/query-tests/Statements/LoopIterationSkippedDueToShifting/tst.js index 1f23604d76a7..01f046d1c1e2 100644 --- a/javascript/ql/test/query-tests/Statements/LoopIterationSkippedDueToShifting/tst.js +++ b/javascript/ql/test/query-tests/Statements/LoopIterationSkippedDueToShifting/tst.js @@ -133,16 +133,26 @@ function withTryCatch(pendingCSS) { function andOperand(toc) { for (let i = 0; i < toc.length; i++) { - toc[i].ignoreSubHeading && toc.splice(i, 1) && i--; // $ SPURIOUS:Alert + toc[i].ignoreSubHeading && toc.splice(i, 1) && i--; } } function ifStatement(toc) { for (let i = 0; i < toc.length; i++) { if(toc[i].ignoreSubHeading){ - if(toc.splice(i, 1)){ // $ SPURIOUS:Alert + if(toc.splice(i, 1)){ i--; } } } } + +function ifStatement2(toc) { + for (let i = 0; i < toc.length; i++) { + if(toc[i].ignoreSubHeading){ + if(!toc.splice(i, 1)){ // $Alert + i--; + } + } + } +} From 885e8369aa817c115b486e76a0386ddd2f326a80 Mon Sep 17 00:00:00 2001 From: Napalys Klicius Date: Thu, 12 Jun 2025 15:18:26 +0200 Subject: [PATCH 3/7] JS: add `quality` and `reliability` tags to `loop-iteration-skipped-due-to-shifting` --- .../query-suite/javascript-code-quality.qls.expected | 1 + .../ql/src/Statements/LoopIterationSkippedDueToShifting.ql | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/javascript/ql/integration-tests/query-suite/javascript-code-quality.qls.expected b/javascript/ql/integration-tests/query-suite/javascript-code-quality.qls.expected index 451a8b4bf273..bbb98b7fa815 100644 --- a/javascript/ql/integration-tests/query-suite/javascript-code-quality.qls.expected +++ b/javascript/ql/integration-tests/query-suite/javascript-code-quality.qls.expected @@ -5,3 +5,4 @@ ql/javascript/ql/src/LanguageFeatures/SpuriousArguments.ql ql/javascript/ql/src/Quality/UnhandledErrorInStreamPipeline.ql ql/javascript/ql/src/RegExp/DuplicateCharacterInCharacterClass.ql ql/javascript/ql/src/RegExp/RegExpAlwaysMatches.ql +ql/javascript/ql/src/Statements/LoopIterationSkippedDueToShifting.ql diff --git a/javascript/ql/src/Statements/LoopIterationSkippedDueToShifting.ql b/javascript/ql/src/Statements/LoopIterationSkippedDueToShifting.ql index 35e632a17fb1..d36884e15a32 100644 --- a/javascript/ql/src/Statements/LoopIterationSkippedDueToShifting.ql +++ b/javascript/ql/src/Statements/LoopIterationSkippedDueToShifting.ql @@ -5,7 +5,9 @@ * @kind problem * @problem.severity warning * @id js/loop-iteration-skipped-due-to-shifting - * @tags correctness + * @tags quality + * reliability + * correctness * @precision high */ From 10d10286f7d849d4b4ab1a0c64b06808cffabdc1 Mon Sep 17 00:00:00 2001 From: Napalys Klicius Date: Thu, 12 Jun 2025 15:23:31 +0200 Subject: [PATCH 4/7] JS: add change notes --- .../ql/src/change-notes/2025-06-12-loop-iteration-fix.md | 4 ++++ javascript/ql/src/change-notes/2025-06-12-loop-iteration.md | 4 ++++ 2 files changed, 8 insertions(+) create mode 100644 javascript/ql/src/change-notes/2025-06-12-loop-iteration-fix.md create mode 100644 javascript/ql/src/change-notes/2025-06-12-loop-iteration.md diff --git a/javascript/ql/src/change-notes/2025-06-12-loop-iteration-fix.md b/javascript/ql/src/change-notes/2025-06-12-loop-iteration-fix.md new file mode 100644 index 000000000000..c34e91360af1 --- /dev/null +++ b/javascript/ql/src/change-notes/2025-06-12-loop-iteration-fix.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* Fixed false positives in the `js/loop-iteration-skipped-due-to-shifting` query when `splice` is used as a condition that adjusts the loop counter. diff --git a/javascript/ql/src/change-notes/2025-06-12-loop-iteration.md b/javascript/ql/src/change-notes/2025-06-12-loop-iteration.md new file mode 100644 index 000000000000..1458ccdbb8eb --- /dev/null +++ b/javascript/ql/src/change-notes/2025-06-12-loop-iteration.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* The `js/loop-iteration-skipped-due-to-shifting` query has been updated with `reliability` tag. From 88f668781dd90e8fed488c11a515206c3cda4cb3 Mon Sep 17 00:00:00 2001 From: Napalys Klicius Date: Thu, 19 Jun 2025 10:24:39 +0200 Subject: [PATCH 5/7] Updated extended expected file after merge --- .../query-suite/javascript-code-quality-extended.qls.expected | 1 + 1 file changed, 1 insertion(+) diff --git a/javascript/ql/integration-tests/query-suite/javascript-code-quality-extended.qls.expected b/javascript/ql/integration-tests/query-suite/javascript-code-quality-extended.qls.expected index bf646822ddc4..e1a6cb98325e 100644 --- a/javascript/ql/integration-tests/query-suite/javascript-code-quality-extended.qls.expected +++ b/javascript/ql/integration-tests/query-suite/javascript-code-quality-extended.qls.expected @@ -7,3 +7,4 @@ ql/javascript/ql/src/LanguageFeatures/TemplateSyntaxInStringLiteral.ql ql/javascript/ql/src/Quality/UnhandledErrorInStreamPipeline.ql ql/javascript/ql/src/RegExp/DuplicateCharacterInCharacterClass.ql ql/javascript/ql/src/RegExp/RegExpAlwaysMatches.ql +ql/javascript/ql/src/Statements/LoopIterationSkippedDueToShifting.ql From 5448071e0997d87c06a41745331b6d4f09fe3167 Mon Sep 17 00:00:00 2001 From: Napalys Klicius Date: Thu, 19 Jun 2025 14:20:37 +0200 Subject: [PATCH 6/7] Update javascript/ql/src/change-notes/2025-06-12-loop-iteration-fix.md Co-authored-by: Taus --- javascript/ql/src/change-notes/2025-06-12-loop-iteration-fix.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/src/change-notes/2025-06-12-loop-iteration-fix.md b/javascript/ql/src/change-notes/2025-06-12-loop-iteration-fix.md index c34e91360af1..2716069fb711 100644 --- a/javascript/ql/src/change-notes/2025-06-12-loop-iteration-fix.md +++ b/javascript/ql/src/change-notes/2025-06-12-loop-iteration-fix.md @@ -1,4 +1,4 @@ --- category: minorAnalysis --- -* Fixed false positives in the `js/loop-iteration-skipped-due-to-shifting` query when `splice` is used as a condition that adjusts the loop counter. +* Fixed false positives in the `js/loop-iteration-skipped-due-to-shifting` query when the return value of `splice` is used to decide whether to adjust the loop counter. From 8679151ace18829fde40feda40df1d1c793e0e1c Mon Sep 17 00:00:00 2001 From: Napalys Klicius Date: Thu, 19 Jun 2025 14:21:08 +0200 Subject: [PATCH 7/7] Update javascript/ql/src/change-notes/2025-06-12-loop-iteration.md Co-authored-by: Taus --- javascript/ql/src/change-notes/2025-06-12-loop-iteration.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/src/change-notes/2025-06-12-loop-iteration.md b/javascript/ql/src/change-notes/2025-06-12-loop-iteration.md index 1458ccdbb8eb..13b9fcf592aa 100644 --- a/javascript/ql/src/change-notes/2025-06-12-loop-iteration.md +++ b/javascript/ql/src/change-notes/2025-06-12-loop-iteration.md @@ -1,4 +1,4 @@ --- category: minorAnalysis --- -* The `js/loop-iteration-skipped-due-to-shifting` query has been updated with `reliability` tag. +* The `js/loop-iteration-skipped-due-to-shifting` query now has the `reliability` tag.