- Notifications
You must be signed in to change notification settings - Fork 1.9k
java inline expectations proof-of-concept with tests#16911
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
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading. Please reload this page.
Conversation
ginsbach commented Jul 5, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
dc5f876 to 6c957f9Compare6c957f9 to 65655dbCompareUh oh!
There was an error while loading. Please reload this page.
| s = actualLines() and | ||
| posString = s.substring(s.indexOf("|", 0, 0) + 1, s.indexOf("|", 1, 0)).trim() and | ||
| filename = posString.substring(0, posString.indexOf(":", 0, 0)) and | ||
| lineString = posString.substring(posString.indexOf(":", 0, 0) + 1, posString.indexOf(":", 1, 0)) and | ||
| lineString = posString.substring(posString.indexOf(":", 2, 0) + 1, posString.indexOf(":", 3, 0)) and | ||
| colStart = | ||
| posString.substring(posString.indexOf(":", 1, 0) + 1, posString.indexOf(":", 2, 0)).toInt() and | ||
| colEnd = posString.substring(posString.indexOf(":", 3, 0) + 1, posString.length()).toInt() and | ||
| line = lineString.toInt() and | ||
| content = s.substring(s.indexOf("|", 2, 0) + 1, s.indexOf("|", 3, 0)).trim() |
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.
Is this meant as an example of best practice? If not, I'd like to see such an example. That will help us confirm we have the right interface.
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.
Do you mean whether my proof-of-concept inline expectations query is meant as best practice? I certainly wouldn't claim so, I just tried to have something that works well enough to verify the CLI part.
There is a test case below that gives an example of this code working in practice:
codeql/java/ql/test/query-tests/Postprocessing/passingWithDiff/OverlyLargeRangeQuery.expected
Line 2 in 65655db
| | SuspiciousRegexpRange.java:7:49:7:51 | unexpected alert | Suspicious character range that overlaps with A-Z in the same character class, and is equivalent to [A-Z\\[\\\\\\]^_`a-z]. | |
Perhaps I misunderstand what you mean, though?
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.
What I mean is that the QL code works by concatenating all the columns and then splitting by | to take them apart again. I hope that's unnecessary.
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.
Apologies, I see what you mean now.
This concatenation is completely unnecessary and merely an artifact of me having gone through multiple different interfaces while working on this issue and not properly cleaning up the QL code after the most recent change.
I have pushed a third commit to remove actualLines and with it the concatenation.
abf9a0a to 402f835Compare| */ | ||
| module; | ||
| query predicate resultRelations(string name){name = "#select"} |
Check warning
Code scanning / CodeQL
Dead code
| */ | ||
| module; | ||
| query predicate learnEdits(string name){none() } |
Check warning
Code scanning / CodeQL
Dead code
| */ | ||
| module; | ||
| query predicate extraQuery(string name){none() } |
Check warning
Code scanning / CodeQL
Dead code
| */ | ||
| module; | ||
| query predicate resultRelations(string name){name = "#select"} |
Check failure
Code scanning / CodeQL
Correct casing on name declarations
| */ | ||
| module; | ||
| query predicate learnEdits(string name){none() } |
Check failure
Code scanning / CodeQL
Correct casing on name declarations
| */ | ||
| module; | ||
| query predicate extraQuery(string name){none() } |
Check failure
Code scanning / CodeQL
Correct casing on name declarations
| */ | ||
| module; | ||
| query predicate resultRelations(string name, int arity){name = "#select" and arity = 5 } |
Check failure
Code scanning / CodeQL
Correct casing on name declarations
402f835 to 7370a2eCompare7078c40 to c86e008Compare
This is a proof-of-concept inline expectations query that I developed alongside the CLI implementation of test postprocessing to verify that everything works and the interface makes sense. It is not meant to be merged, but should serve as inspiration for a proper version.
Note that
java/ql/test/query-tests/Postprocessing/passing/SuspiciousRegexpRange.javais an identical copy ofjava/ql/test/query-tests/security/CWE-020/SuspiciousRegexpRange.java, with the other added (non-empty) .java files being slight variations.