-
Notifications
You must be signed in to change notification settings - Fork 3
Add CDS Utils path injection query #224
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
Add CDS Utils path injection query #224
Conversation
javascript/frameworks/cap/test/queries/path-traversal/pathinjection.js
Dismissed
Show dismissed
Hide dismissed
javascript/frameworks/cap/test/queries/path-traversal/pathinjection.js
Dismissed
Show dismissed
Hide dismissed
javascript/frameworks/cap/test/queries/path-traversal/pathinjection.js
Dismissed
Show dismissed
Hide dismissed
javascript/frameworks/cap/test/queries/path-traversal/pathinjection.js
Dismissed
Show dismissed
Hide dismissed
javascript/frameworks/cap/test/queries/path-traversal/pathinjection.js
Dismissed
Show dismissed
Hide dismissed
javascript/frameworks/cap/test/queries/path-traversal/pathinjection.js
Dismissed
Show dismissed
Hide dismissed
javascript/frameworks/cap/test/queries/path-traversal/pathinjection.js
Dismissed
Show dismissed
Hide dismissed
jeongsoolee09
left a comment
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.
Nice work. I'd like to suggest two things:
Lack of barriers
A barrier is lacking in this example. Research some well-known ways to neutralize path traversal and ones that are specific to CAP, and add both to the Recommendations section of the help file and to the isBarrier predicate of the configuration.
CDSAdditionalFlowStep hard to understand at first glance
It's a bit hard to follow the logic of CDSAdditionalFlowStep, and much of it is coming from the fact that conceptually a flow step is a pair of two dataflow nodes but the implementation at the moment is a DataFlow::Node.
But admittedly encoding a tuple can be a bit verbose to model in QL. So I think it's better to remove the hierarchy only for additional flow steps and directly inline the class definition into the isAdditionalStep predicate. It seems like the default queries are following this practice.
javascript/frameworks/cap/test/queries/path-traversal/pathinjection.js
Dismissed
Show dismissed
Hide dismissed
javascript/frameworks/cap/test/queries/path-traversal/pathinjection.js
Dismissed
Show dismissed
Hide dismissed
javascript/frameworks/cap/test/queries/path-traversal/pathinjection.js
Dismissed
Show dismissed
Hide dismissed
…thub.com/advanced-security/codeql-sap-js into knewbury01/cds-util-path-traversal-query
jeongsoolee09
left a comment
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.
Good work! LGTM!
What This PR Contributes
PathInjection.qlFuture Works
Add additional unit tests if the extra API cases described here in the future works are covered in the future.