JS: Update test suite to use post-processed inline expectations #18670
Conversation
asgerf
force-pushed
the
js/test-suite
branch
4 times, most recently
from
6e464f3 to
3a86b71
Compare
asgerf
force-pushed
the
js/test-suite
branch
from
3a86b71 to
684fefb
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -1 +1 @@ | |||
| Comments/CommentedOutCode.ql | |||
| query: Comments/CommentedOutCode.ql | |||
Check warning
Code scanning / CodeQL
Query test without inline test expectations Warning test
| @@ -1 +1 @@ | |||
| Comments/TodoComments.ql | |||
| query: Comments/TodoComments.ql | |||
Check warning
Code scanning / CodeQL
Query test without inline test expectations Warning test
| @@ -1 +1 @@ | |||
| JSDoc/JSDocForNonExistentParameter.ql | |||
| query: JSDoc/JSDocForNonExistentParameter.ql | |||
Check warning
Code scanning / CodeQL
Query test without inline test expectations Warning test
| @@ -1 +1 @@ | |||
| LanguageFeatures/ConditionalComments.ql | |||
| query: LanguageFeatures/ConditionalComments.ql | |||
Check warning
Code scanning / CodeQL
Query test without inline test expectations Warning test
| @@ -1 +1 @@ | |||
| NodeJS/UnusedDependency.ql | |||
| query: NodeJS/UnusedDependency.ql | |||
Check warning
Code scanning / CodeQL
Query test without inline test expectations Warning test
| @@ -1 +1 @@ | |||
| Security/CWE-094/ExpressionInjection.ql | |||
| query: Security/CWE-094/ExpressionInjection.ql | |||
Check warning
Code scanning / CodeQL
Query test without inline test expectations Warning test
| @@ -1 +1 @@ | |||
| Security/CWE-300/InsecureDependencyResolution.ql | |||
| query: Security/CWE-300/InsecureDependencyResolution.ql | |||
Check warning
Code scanning / CodeQL
Query test without inline test expectations Warning test
| @@ -1 +1 @@ | |||
| Security/CWE-312/ActionsArtifactLeak.ql | |||
| query: Security/CWE-312/ActionsArtifactLeak.ql | |||
Check warning
Code scanning / CodeQL
Query test without inline test expectations Warning test
| @@ -1 +1 @@ | |||
| Security/CWE-313/PasswordInConfigurationFile.ql | |||
| query: Security/CWE-313/PasswordInConfigurationFile.ql | |||
Check warning
Code scanning / CodeQL
Query test without inline test expectations Warning test
| @@ -1 +1 @@ | |||
| Security/CWE-862/EmptyPasswordInConfigurationFile.ql | |||
| query: Security/CWE-862/EmptyPasswordInConfigurationFile.ql | |||
Check warning
Code scanning / CodeQL
Query test without inline test expectations Warning test
| @@ -1 +1 @@ | |||
| JSDoc/BadParamTag.ql | |||
| query: JSDoc/BadParamTag.ql | |||
Check warning
Code scanning / CodeQL
Query test without inline test expectations Warning test
| @@ -1 +1 @@ | |||
| LanguageFeatures/SyntaxError.ql | |||
| query: LanguageFeatures/SyntaxError.ql | |||
Check warning
Code scanning / CodeQL
Query test without inline test expectations Warning test
asgerf
force-pushed
the
js/test-suite
branch
2 times, most recently
from
a456a5b to
77a27de
Compare
| @@ -1 +1 @@ | |||
| Security/CWE-312/BuildArtifactLeak.ql | |||
| query: Security/CWE-312/BuildArtifactLeak.ql | |||
Check warning
Code scanning / CodeQL
Query test without inline test expectations Warning test
| @@ -1 +1 @@ | |||
| LanguageFeatures/WrongExtensionJSON.ql | |||
| query: LanguageFeatures/WrongExtensionJSON.ql | |||
Check warning
Code scanning / CodeQL
Query test without inline test expectations Warning test
asgerf
force-pushed
the
js/test-suite
branch
3 times, most recently
from
bc599d1 to
9df0bf6
Compare
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.
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.
We had two 'NOT OK' comments for the same alert. The alert appears on the 'pref' object above.
Using RelatedLocations to add clarity
This file was added on main while this branch was in progress. Porting the whole file in one step.
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.
asgerf
force-pushed
the
js/test-suite
branch
from
9df0bf6 to
2a194a5
Compare
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
// $ Alertinstead of// NOT OK. Our oldNOT 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:
.qlreffiles and run the test to get a list of discrepancies.NOT OKcomment, 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.In a few cases the underlying bugs were simple enough that I went ahead and fixed them on the spot.