-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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.
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
Test: Add support for RelatedLocation tag and use in a JS query #18810
base: main
Are you sure you want to change the base?
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.
a7e9dd1
to
cd0fd02
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
This adds support for marking related locations with
// $ RelatedLocation
inline 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
$ Alert
on the consolidated alert line isn't a good choice. For path problems this is rarely a problem as we haveSource
andSink
tags, but for non-path problems it can be an issue.This PR migrates the JS
MissingCsrfMiddleware
test 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.