Skip to content
This repository was archived by the owner on Jan 5, 2023. It is now read-only.

Make cwe 643 supported #73

Merged
merged 20 commits into from
Apr 2, 2020
Merged

Conversation

intrigus-lgtm
Copy link
Contributor

@intrigus-lgtm intrigus-lgtm commented Mar 26, 2020

This has been modeled after the existing SqlInjection/Injections.qll.

Things this is missing:

  • A change note.
  • Two TODOs

The XPath injection query is moved to the supported queries.
Removed unnecessary code from the go test file
Copy link
Contributor

@sauyon sauyon left a 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.

@max-schaefer
Copy link
Contributor

@shati-patel, could you take a look at the query help? Here is a preview.

@shati-patel
Copy link
Contributor

@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 .qhelp inline, so here they are:)

  • "For example, when using the github.com/ChrisTrenkamp/goxpath API, you can do this by creating a function that takes an *goxpath.Opts structure."
  • "This function can then be specified when calling Exec(), Exec{Bool|Num|Node}(), ParseExec(), or MustExec()."
    (comma after ParseExec())
  • "In the first example, the code accepts a username ..."
  • The code in the example overflows outside the box:
    image
    This may just be a problem with the preview, but if you're able to make the lines shorter that might be a good idea.

@intrigus-lgtm
Copy link
Contributor Author

Thanks again! This is getting there, but I think I'd like to see a few more tests for the various cases.

How many tests do you want?
Also, stubbing is quite tedious, is there an easy way to create stubs instead of creating them by hand?

@max-schaefer
Copy link
Contributor

Many thanks for you continued efforts!

Ideally we'd like one test per supported library.

stubbing is quite tedious

Unfortunately, yes. I don't know of a way to automate it.

@max-schaefer
Copy link
Contributor

The query doesn't find any results on LGTM.com at the moment, which isn't entirely unexpected.

Copy link
Contributor

@max-schaefer max-schaefer left a 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?

@intrigus-lgtm
Copy link
Contributor Author

I've added a change note and a precision modifier.
Since most Xpath injection queries in other languages are @precision high I've defaulted to this value here as well.

@max-schaefer
Copy link
Contributor

Thanks! @precision high should be fine, seeing as my run across all of LGTM.com above didn't find any results.

Once @sauyon has done the evaluation, this is good to go in.

Copy link
Contributor

@sauyon sauyon left a 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.

@sauyon sauyon merged commit bc59fa4 into github:master Apr 2, 2020
@max-schaefer
Copy link
Contributor

🎉 Thank you for your contribution, @intrigus-lgtm.

@intrigus-lgtm
Copy link
Contributor Author

Thank you for guiding me and answering all my questions :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants