Test: Add support for RelatedLocation tag and use in a JS query #18810
Conversation
This query flags the cookie-parsing middleware in order to consolidate huge numbers of alerts into a single alert, which is more manageable. But simply annotating the cookie-parsing middleware with 'Alert' isn't a very useful, we want to annotate which middlewares are vulnerable.
Source tags should no longer be used when on the same line as the Alert. The ones in this file went unnoticed however because *all* of them were on the same line as an Alert, which made the test library ignore all Source tags.
asgerf
force-pushed
the
js/test-related-locations
branch
from
a7e9dd1 to
cd0fd02
Compare
There was a problem hiding this comment.
PR Overview
This PR migrates several JavaScript and Rust security query tests to use inline expectations for indicating alerts and related location annotations, thereby standardizing test annotations. Key changes include:
- Adding inline comments (e.g. “// $ Alert”, “// $ RelatedLocation”) to annotate test expectations.
- Removing “NOT OK” markers in favor of inline annotations for clarity.
- Switching alert tags in Rust tests from “$ Source Alert” to “$ Alert” for consistency.
Reviewed Changes
| File | Description |
|---|---|
| javascript/ql/test/query-tests/Security/CWE-352/fastify2.js | Adds inline alert and related location annotations for the Fastify-based test. |
| javascript/ql/test/query-tests/Security/CWE-352/fastify.js | Applies similar inline annotation changes as fastify2.js. |
| javascript/ql/test/query-tests/Security/CWE-352/tst.js | Updates inline annotations on cookie usage and related locations. |
| javascript/ql/test/query-tests/Security/CWE-352/MissingCsrfMiddlewareBad.js | Migrates inline expectations in the missing CSRF middleware test. |
| javascript/ql/test/query-tests/Security/CWE-352/csurf_example.js | Adds inline alert and related annotations for csurf examples. |
| javascript/ql/test/query-tests/Security/CWE-352/csurf_api_example.js | Standardizes inline comments for API examples regarding cookie usage. |
| javascript/ql/test/query-tests/Security/CWE-352/unused_cookies.js | Updates inline annotations to reflect cookie-related vulnerability expectations. |
| javascript/ql/test/query-tests/Security/CWE-352/lusca_example.js | Aligns annotations for lusca examples with the new inline expectation style. |
| rust/ql/test/query-tests/security/CWE-328/test.rs | Adjusts alert annotations for weak-sensitive-data-hashing tests to use unified tags. |
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
rust/ql/test/query-tests/security/CWE-328/test.rs:74
- The inline expectation tag here is marked as '$ MISSING: Alert'. Consider updating it to '$ Alert' to maintain consistency with the rest of the changes.
_ = md5::Md5::new().chain_update(harmless).chain_update(password).chain_update(harmless).finalize(); // $ MISSING: Alert[rust/weak-sensitive-data-hashing]
Tip: If you use Visual Studio Code, you can request a review from Copilot before you push from the "Source Control" tab. Learn more
| getQueryKind() = "path-problem" and | ||
| n = exactDivide(column - 8, 3) | ||
| or | ||
| getQueryKind() = "problem" and | ||
| n = exactDivide(column - 3, 3) |
There was a problem hiding this comment.
It's hard to figure out where these numbers come from, can you add an inline explanation to each line?
There was a problem hiding this comment.
Good call. I added a clarifying comment and noticed a bug, namely that the 8 should have been a 7.
There was no path-problem query using related locations, so it didn't get caught initially. I went ahead and added some better tests for this as well.
| /** Gets the `n`th related location selected in `row`. */ | ||
| private TestLocation getRelatedLocation(int row, int n, string element) { |
There was a problem hiding this comment.
I don't see n being used outside of the predicate, so maybe the predicate can be simplified?
Yes but it was too much of a hassle for what we need right now. |
This adds support for marking related locations with
// $ RelatedLocationinline comments. They are only expected if at least one such comment exists in the test; and if so then all related locations must be annotated.Some queries use a primary alert location such that a large number of alerts get consolidated into a single alert. For such queries simply putting
$ Alerton the consolidated alert line isn't a good choice. For path problems this is rarely a problem as we haveSourceandSinktags, but for non-path problems it can be an issue.This PR migrates the JS
MissingCsrfMiddlewaretest to inline expectations so the behaviour is exercised. This query flags the cookie-parsing middleware function, as opposed to the vulnerable route handler function, because there can be a huge number of vulnerable route handlers that are all far away from the root cause of the vulnerability.