JS: Update test suite to use post-processed inline expectations #18670

asgerf · 2025-02-04T11:54:50Z

Updates all of JavaScript's query tests to use post-processed inline expectations except in a few cases where it's not possible. Overall 233 query tests consisting of 712 JavaScript files have been updated.

Inline expectations are now written as // $ Alert instead of // NOT OK. Our old NOT OK-style comments were not automatically verified for consistency except in a handful of cases, so some discrepancies have crept in over the years. This PR reviews and fixes up these discrepancies and of course prevents them from being introduced in the future.

Only query tests have been updated. Library tests have to be implemented a bit differently; that's for another PR.

This PR has the following overall structure:

Automatically translate all OK-style comments to $-style. This step tries to capture the original intent behind the test, and does not try to adhere to actual query output.
Enable post-processing in .qlref files and run the test to get a list of discrepancies.
Fix trivial/unsurprising discrepancies. There are many alerts that simply didn't have a NOT OK comment, but where it's farily easy too see from the test that it was intended to be flagged. There are many of these and I found it's probably easiest to spot check some results from a single commit than having 100+ commits with "trivial" changes.
A long tail of small but non-trivial changes in individual commits, usually with a commit message to explain the rationale for the change.

In a few cases the underlying bugs were simple enough that I went ahead and fixed them on the spot.

javascript/ql/test/query-tests/Comments/CommentedOutCode/CommentedOutCode.qlref

@@ -1 +1 @@
-Comments/CommentedOutCode.ql
+query: Comments/CommentedOutCode.ql


javascript/ql/test/query-tests/Comments/TodoComments/TodoComments.qlref

@@ -1 +1 @@
-Comments/TodoComments.ql
+query: Comments/TodoComments.ql


...pt/ql/test/query-tests/JSDoc/JSDocForNonExistentParameter/JSDocForNonExistentParameter.qlref

javascript/ql/test/query-tests/LanguageFeatures/ConditionalComments/ConditionalComments.qlref

@@ -1 +1 @@
-LanguageFeatures/ConditionalComments.ql
+query: LanguageFeatures/ConditionalComments.ql


javascript/ql/test/query-tests/NodeJS/UnusedDependency/UnusedDependency.qlref

javascript/ql/test/query-tests/Security/CWE-094/ExpressionInjection/ExpressionInjection.qlref

@@ -1 +1 @@
-Security/CWE-094/ExpressionInjection.ql
+query: Security/CWE-094/ExpressionInjection.ql


javascript/ql/test/query-tests/Security/CWE-300/InsecureDependencyResolution.qlref

javascript/ql/test/query-tests/Security/CWE-312/ActionsArtifactLeak.qlref

@@ -1 +1 @@
-Security/CWE-312/ActionsArtifactLeak.ql
+query: Security/CWE-312/ActionsArtifactLeak.ql


javascript/ql/test/query-tests/Security/CWE-313/PasswordInConfigurationFile.qlref

@@ -1 +1 @@
-Security/CWE-313/PasswordInConfigurationFile.ql
+query: Security/CWE-313/PasswordInConfigurationFile.ql


javascript/ql/test/query-tests/Security/CWE-862/EmptyPasswordInConfigurationFile.qlref

@@ -1 +1 @@
-Security/CWE-862/EmptyPasswordInConfigurationFile.ql
+query: Security/CWE-862/EmptyPasswordInConfigurationFile.ql


javascript/ql/test/query-tests/JSDoc/BadParamTag/BadParamTag.qlref

javascript/ql/test/query-tests/LanguageFeatures/SyntaxError/SyntaxError.qlref

@@ -1 +1 @@
-LanguageFeatures/SyntaxError.ql
+query: LanguageFeatures/SyntaxError.ql


javascript/ql/test/query-tests/Security/CWE-312/BuildArtifactLeak.qlref

javascript/ql/test/query-tests/WrongExtensionJSON/WrongExtensionJSON.qlref

Some OK-style comments had to be moved to the following line, shifting line numbers. In selected range also included the comments themselves. Lastly, the result sets were reordered by the CLI in some cases.

JSON does not support comments so we can't use inline expectations

The presence of a syntax error sometimes prevents us from parsing the inline comment correctly.

One of the diffs look confusing but: Previously parameter {2,3} where flagged, now parameter {1,2} are flagged. Note that for command injection, the SystemCommandExecution is flagged despite the test file claiming otherwise.

This adds Alert annotations for alerts that seem intentional by the test but has not been annotated with 'NOT OK', or the comment was in the wrong place. In a few cases I included 'Source' expectations to make it easier to see what happened. Other 'Source' expectations will be added in bulk a later commit.

See github/codeql-javascript-team#447

These are listed in a function called 'good' but it's difficult to say in isolation whether they should be flagged or not. Accepting the changes as they seem reasonable.

The history of updates to this test got messed up so just squashing into one commit. Some possible regressions have been accepted, but the query is strangely opinionated so it's just hard to say what it ought to flag.

Napalys

Amazing work! 🚀 You're officially #1 on my list for the biggest pull request I've ever reviewed! 😆

Napalys · 2025-03-03T13:53:51Z

javascript/ql/test/query-tests/Comments/TodoComments/tst.js

@@ -1,2 +1,2 @@
-// OK


This stands outs for me as an odd case. Do we need to remove this?

Don't see a reason to keep it

It seemed unclear whether it was meant to be a separate test case with just // OK or if it was part of the next line, indicating that the next line is // OK. That's why I wasn't sure if it should be removed or not 😄

Napalys · 2025-03-03T13:56:29Z

javascript/ql/test/query-tests/DOM/HTML/tst.js

-// not OK
-<a href="http://semmle.com" href={someValue()}>Semmle</a>;
+<a href="http://semmle.com" href={someValue()}>Semmle</a>; // $ Alert
+


Sorry, I couldn't help but notice the extra line 😄

Napalys · 2025-03-03T13:59:18Z

javascript/ql/test/query-tests/DOM/TargetBlank/tst.js

 var o = { rel: "noopener noreferrer "};

-// OK
+


This file now has many extra lines due to the replacements of // OK with /n. 🙈

Napalys · 2025-03-03T14:02:33Z

javascript/ql/test/query-tests/Declarations/AssignmentToConst/other.js

@@ -1,2 +1,2 @@
-// OK
+


File starting with new line 🙈

Napalys · 2025-03-03T17:46:59Z

javascript/ql/test/query-tests/Declarations/DeadStoreOfGlobal/tst.js

+g = 23; // $ Alert
+


Additional new line in //NO OK

javascript/ql/test/query-tests/Security/CWE-079/ExceptionXss/exception-xss.js

javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization/UnsafeHtmlExpansion.js

Napalys · 2025-03-04T10:14:25Z

javascript/ql/test/query-tests/Security/CWE-611/libxml.noent.js

-  libxmljs.parseXml(req.param("some-xml"), { noent: true }); // $ Alert
+  libxmljs.parseXml(req.param("some-xml"), { noent: true }); // $ Alert // $ Alert - unguarded entity expansion
 });

 express().post('/some/path', function (req, res) {
-  // NOT OK: unguarded entity expansion
-  libxmljs.parseXml(req.param("some-xml"), { noent: true }); // $ Alert
+  libxmljs.parseXml(req.param("some-xml"), { noent: true }); // $ Alert // $ Alert - unguarded entity expansion

-  // NOT OK: unguarded entity expansion
-  libxmljs.parseXmlString(req.param("some-xml"), { noent: true }) // $ Alert
-  // NOT OK: unguarded entity expansion
-  libxmljs.parseXmlString(req.files.products.data.toString('utf8'), { noent: true })// $ Alert
+  libxmljs.parseXmlString(req.param("some-xml"), { noent: true }) // $ Alert // $ Alert - unguarded entity expansion
+  libxmljs.parseXmlString(req.files.products.data.toString('utf8'), { noent: true })// $ Alert // $ Alert - unguarded entity expansion


This looks suspicious, shouldn't it be a single // $ Alert instead of // $ Alert // $ Alert?

javascript/ql/test/query-tests/Security/CWE-730/server-crash.js

javascript/ql/test/query-tests/Security/CWE-400/ReDoS/polynomial-redos.js

…t/uselesscat.js Co-authored-by: Napalys Klicius <napalys@github.com>

…t.js Co-authored-by: Napalys Klicius <napalys@github.com>

…xception-xss.js Co-authored-by: Napalys Klicius <napalys@github.com>

…tization/UnsafeHtmlExpansion.js Co-authored-by: Napalys Klicius <napalys@github.com>

Co-authored-by: Napalys Klicius <napalys@github.com>

…al-redos.js Co-authored-by: Napalys Klicius <napalys@github.com>

erik-krogh · 2025-03-11T12:13:51Z

LGTM 👍

But you got some merge-conflicts, again.

'Accept incoming changes' in vscode somehow deleted this line.

JS: Update test suite to use post-processed inline expectations

github-actions bot added the JS label Feb 4, 2025

asgerf force-pushed the js/test-suite branch 4 times, most recently from 6e464f3 to 3a86b71 Compare February 7, 2025 13:26

asgerf force-pushed the js/test-suite branch from 3a86b71 to 684fefb Compare February 25, 2025 16:30

github-actions bot added the documentation label Feb 25, 2025

github-advanced-security bot found potential problems Feb 25, 2025

View reviewed changes

github-advanced-security bot found potential problems Feb 26, 2025

View reviewed changes

asgerf force-pushed the js/test-suite branch 2 times, most recently from a456a5b to 77a27de Compare February 27, 2025 14:11

github-advanced-security bot found potential problems Feb 27, 2025

View reviewed changes

asgerf force-pushed the js/test-suite branch 3 times, most recently from bc599d1 to 9df0bf6 Compare February 28, 2025 12:21

asgerf added 15 commits February 28, 2025 13:27

JS: Allow more kinds of expectation comments

79e2a75

JS: Remove uses of old inline expectation test library

7e5c24a

JS: Update OK-style comments to $-style

9be041e

JS: Update output after line number change

426edd5

Some OK-style comments had to be moved to the following line, shifting line numbers. In selected range also included the comments themselves. Lastly, the result sets were reordered by the CLI in some cases.

JS: Enable post-processing for all .qlref files

d0ce53e

JS: Disable for comment-related alerts

ac6547f

JS: Disable for test with alerts in a JSON file

789a7bd

JSON does not support comments so we can't use inline expectations

JS: Disable for SyntaxError

795c110

The presence of a syntax error sometimes prevents us from parsing the inline comment correctly.

JS: Accept raw test output

f5911c9

JS: Move some alerts to their correct location

86932c5

One of the diffs look confusing but: Previously parameter {2,3} where flagged, now parameter {1,2} are flagged. Note that for command injection, the SystemCommandExecution is flagged despite the test file claiming otherwise.

JS: Add query ID to some alerts

0453ded

JS: Accept some alerts at the SystemCommandExecution location

07a876b

JS: Mark alert as MISSING

f395651

See github/codeql-javascript-team#447

JS: Accept some less obvious alerts

1f3c496

These are listed in a function called 'good' but it's difficult to say in isolation whether they should be flagged or not. Accepting the changes as they seem reasonable.

asgerf added 5 commits February 28, 2025 13:29

JS: Bulk update in UnneededDefensiveProgramming test

7bd01bf

The history of updates to this test got messed up so just squashing into one commit. Some possible regressions have been accepted, but the query is strangely opinionated so it's just hard to say what it ought to flag.

Disable for more queries with alerts in JSON

c67c585

JS: Convert some comments to JSX

33602ee

JS: Add query IDs

193b26e

raw test output

2a194a5

asgerf force-pushed the js/test-suite branch from 9df0bf6 to 2a194a5 Compare February 28, 2025 12:29

asgerf marked this pull request as ready for review February 28, 2025 13:41

asgerf requested a review from a team as a code owner February 28, 2025 13:41

Napalys reviewed Mar 5, 2025

View reviewed changes

asgerf and others added 15 commits March 10, 2025 14:18

Update javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCa…

24c9b2e

…t/uselesscat.js Co-authored-by: Napalys Klicius <napalys@github.com>

Update javascript/ql/test/query-tests/Security/CWE-078/UselessUseOfCa…

017f458

…t/uselesscat.js Co-authored-by: Napalys Klicius <napalys@github.com>

Update javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/ts…

dad4838

…t.js Co-authored-by: Napalys Klicius <napalys@github.com>

Update javascript/ql/test/query-tests/Security/CWE-079/ExceptionXss/e…

21d42bc

…xception-xss.js Co-authored-by: Napalys Klicius <napalys@github.com>

Update javascript/ql/test/query-tests/Security/CWE-116/IncompleteSani…

92dfdc8

…tization/UnsafeHtmlExpansion.js Co-authored-by: Napalys Klicius <napalys@github.com>

Update javascript/ql/test/query-tests/Security/CWE-730/server-crash.js

8ee5b23

Co-authored-by: Napalys Klicius <napalys@github.com>

Update javascript/ql/test/query-tests/Security/CWE-400/ReDoS/polynomi…

122f68e

…al-redos.js Co-authored-by: Napalys Klicius <napalys@github.com>

JS: Remove blank line

f7532c0

JS: Remove blank lines

b9dd594

JS: Remove blank line and add trailing newline to file

6fe3a36

JS: Remove blank lines and add trailing newline

0df893e

JS: Remove stray $ Alert comment inside a doc comment

c88eac4

JS: Fix broken alert comment in HeterogenousComparison

6a47678

JS: Remove duplicate '$ Alert' in libxml test

75ed0d0

JS: Line number changes in redos test case

0f201d2

asgerf added 2 commits March 11, 2025 13:17

Merge branch 'main' into js/test-suite

e8c5e4d

JS: Restore line lost in merge

6499e54

'Accept incoming changes' in vscode somehow deleted this line.

erik-krogh approved these changes Mar 11, 2025

View reviewed changes

asgerf merged commit 087c555 into github:main Mar 11, 2025
13 checks passed

Napalys pushed a commit to Napalys/codeql that referenced this pull request Mar 14, 2025

Merge pull request github#18670 from asgerf/js/test-suite

d4f76ff

JS: Update test suite to use post-processed inline expectations

		@@ -1 +1 @@
		Comments/CommentedOutCode.ql
		query: Comments/CommentedOutCode.ql

		@@ -1 +1 @@
		Comments/TodoComments.ql
		query: Comments/TodoComments.ql

		@@ -1 +1 @@
		LanguageFeatures/ConditionalComments.ql
		query: LanguageFeatures/ConditionalComments.ql

		@@ -1 +1 @@
		Security/CWE-094/ExpressionInjection.ql
		query: Security/CWE-094/ExpressionInjection.ql

		@@ -1 +1 @@
		Security/CWE-312/ActionsArtifactLeak.ql
		query: Security/CWE-312/ActionsArtifactLeak.ql

		@@ -1 +1 @@
		Security/CWE-313/PasswordInConfigurationFile.ql
		query: Security/CWE-313/PasswordInConfigurationFile.ql

		@@ -1 +1 @@
		Security/CWE-862/EmptyPasswordInConfigurationFile.ql
		query: Security/CWE-862/EmptyPasswordInConfigurationFile.ql

		@@ -1 +1 @@
		LanguageFeatures/SyntaxError.ql
		query: LanguageFeatures/SyntaxError.ql

JS: Update test suite to use post-processed inline expectations #18670

JS: Update test suite to use post-processed inline expectations #18670

Uh oh!

Conversation

asgerf commented Feb 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Check warning

Check warning

Check warning

Check warning

Check warning

Check warning

Check warning

Check warning

Check warning

Check warning

Check warning

Check warning

Check warning

Check warning

Napalys left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

erik-krogh commented Mar 11, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

asgerf commented Feb 4, 2025 •

edited

Loading