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
Java: Finish dataflow refactor #12751
base: main
Are you sure you want to change the base?
Java: Finish dataflow refactor #12751
Conversation
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.
Mostly LGTM, with a couple of comments.
java/ql/test/query-tests/security/CWE-798/semmle/tests/HardcodedCredentialsSourceCall.ql
Outdated
Show resolved
Hide resolved
| predicate sensitiveResultReceiver( | ||
| DataFlow::PathNode src, DataFlow::PathNode sink, DataFlow::Node recSrc | ||
| SensitiveResultReceiverFlow::PathNode src, SensitiveResultReceiverFlow::PathNode sink, |
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.
Won't this API change affect potential users of this predicate, since it's public? In other words: should we deprecate sensitiveResultReceiver and create a new predicate that uses SensitiveResultReceiverFlow::PathNode?
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 point. I've done the deprecation in cac00b6.
| override predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet c) { | ||
| super.allowImplicitRead(node, c) | ||
| or | ||
| this.isSink(node) |
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.
This is still necessary. Note that the default implementation only allows implicit reads in sinks if defaultImplicitTaintRead holds, which is not the case here.
The failure in the tests is caused by this.
Co-authored-by: Tony Torralba <atorralba@users.noreply.github.com>
889a974
to
72907eb
Compare
No description provided.