Skip to content

Conversation

@tamasvajk
Copy link
Contributor

@tamasvajk tamasvajk commented Mar 29, 2021

@github-actions github-actions bot added the Java label Mar 29, 2021
@tamasvajk tamasvajk force-pushed the feature/java-sinks-csv branch 3 times, most recently from deb60b6 to ece8b5b Compare April 1, 2021 11:21
@tamasvajk tamasvajk marked this pull request as ready for review April 7, 2021 08:09
@tamasvajk tamasvajk requested a review from a team as a code owner April 7, 2021 08:09
@tamasvajk tamasvajk marked this pull request as draft April 9, 2021 09:47
@tamasvajk tamasvajk force-pushed the feature/java-sinks-csv branch from ece8b5b to c096790 Compare April 9, 2021 11:06
@tamasvajk tamasvajk force-pushed the feature/java-sinks-csv branch from c096790 to 1df7db2 Compare April 9, 2021 11:12
@tamasvajk tamasvajk force-pushed the feature/java-sinks-csv branch from 1df7db2 to 351f35d Compare April 9, 2021 11:13
@tamasvajk tamasvajk marked this pull request as ready for review April 12, 2021 08:05
@tamasvajk tamasvajk added the no-change-note-required This PR does not need a change note label Apr 12, 2021
Copy link
Contributor

@aschackmull aschackmull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an overall comment, we're getting a bunch of opaque sink identifiers, which really could use qldoc. The best way to add qldoc to these identifiers is probably to introduce the convention that all references to sinkNode(n, "some-sink-id") occur as charpreds of simple wrapper classes. E.g.:

class XssSink extends DataFlow::Node {
  XssSink() { sinkNode(this, "xss") }
}

This allows us a place to introduce a nice QL class name and suitable qldoc that explains what the sink is.

/**
* A sink that represents a URL opening method call, such as a call to `java.net.URL.openConnection()`.
*/
private class URLOpenSink extends DataFlow::Node {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private class URLOpenSink extends DataFlow::Node {
private class UrlOpenSink extends DataFlow::Node {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mind changing this to UrlOpenSink, but a couple of lines above we already have HTTPStringToURLOpenMethodFlowConfig, so it would look a bit strange, wouldn't it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, that ought to be renamed to HttpStringToUrlOpenMethodFlowConfig.

@tamasvajk tamasvajk merged commit 4cc8866 into github:main Apr 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Java no-change-note-required This PR does not need a change note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants