-
Notifications
You must be signed in to change notification settings - Fork 126
Conversation
The XPath injection query is moved to the supported queries. Removed unnecessary code from the go test file
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.
Thanks again! This is getting there, but I think I'd like to see a few more tests for the various cases.
@shati-patel, could you take a look at the query help? Here is a preview. |
Thanks, the documentation mostly looks clear! I've made a few suggestions based on our existing style guide. (I can't comment on the
|
Co-Authored-By: Sauyon Lee <sauyon@github.com>
How many tests do you want? |
Many thanks for you continued efforts! Ideally we'd like one test per supported library.
Unfortunately, yes. I don't know of a way to automate it. |
The query doesn't find any results on LGTM.com at the moment, which isn't entirely unexpected. |
Note: This is currently missing appropriate vendoring so will probably fail for now.
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.
Almost there, thanks a lot for your hard work in creating the stubs! I have one more question and a few stylistic comments; also, we still need a change note.
@sauyon, could you run an evaluation of this query?
ql/test/query-tests/Security/CWE-643/vendor/github.com/ChrisTrenkamp/goxpath/goxpath.go
Outdated
Show resolved
Hide resolved
I've added a change note and a precision modifier. |
Thanks! Once @sauyon has done the evaluation, this is good to go in. |
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.
Evaluation (internal link) was clean, merging.
🎉 Thank you for your contribution, @intrigus-lgtm. |
Thank you for guiding me and answering all my questions :) |
This has been modeled after the existing
SqlInjection/Injections.qll
.Things this is missing:
TODO
s