Java: Add a default taint sanitizer for contains-checks on lists of constants

[aschackmull]

Builds on top of #17863

This is a reimplementation of #17051 expressed in terms of a universal flow library.

[intrigus-lgtm]

Curious, should this also handle kotlin? Or is this java only right now?

[aschackmull]

Curious, should this also handle kotlin? Or is this java only right now?

To the extent that Kotlin calls the same methods, i.e. java.util.Collection::contains etc., then this should work just fine for Kotlin as well. Of course, if there are some similar methods in Kotlin with different qualified names, then they would need to be added alongside their Java equivalents in this QL code. I'm not fluent enough in canonical Kotlin to tell whether that's the case or not.

[intrigus-lgtm]

Curious, should this also handle kotlin? Or is this java only right now?

To the extent that Kotlin calls the same methods, i.e. java.util.Collection::contains etc., then this should work just fine for Kotlin as well. Of course, if there are some similar methods in Kotlin with different qualified names, then they would need to be added alongside their Java equivalents in this QL code. I'm not fluent enough in canonical Kotlin to tell whether that's the case or not.

I was mostly thinking of https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.collections/list-of.html and friends for unmodifiableCollectionStep which I believe aren't covered right now. I haven't used Kotlin since almost a decade, so there might be more methods.

[owen-mc]

If I've understood correctly that we are following any java.util.Collection, then I think we ought to have more addition methods here. For example, java.util.SequencedCollection has addFirst and addLast.

[aschackmull]

I don't expect allowlists constructions to use those, but I've added them now - it can't hurt.

[owen-mc]

I think it would be good to include the tests from #17051, modified so they pass where you have made different decisions (e.g. allowing reads from fields that aren't static and final).

[aschackmull]

I think it would be good to include the tests from #17051, modified so they pass where you have made different decisions (e.g. allowing reads from fields that aren't static and final).

Done. I did the modification in a separate commit, so it's easy for you to see the diff.

[owen-mc]

The code all looks good. Test expectations need to be updated for one test. And you've added a module and a bunch of predicates without QLDocs in TypeFlow::FlowStepsInput. (It also seems to be complaining about some that you didn't add in BaseSSA::BaseSsaSourceVariable and BaseSSA::BaseSsaVariable, that may be worth just fixing in this PR so the alert stops going off.)

[aschackmull]

Test expectations need to be updated for one test.

Yes, there was a semantic merge conflict. Fixed now.

And you've added a module and a bunch of predicates without QLDocs in TypeFlow::FlowStepsInput. (It also seems to be complaining about some that you didn't add in BaseSSA::BaseSsaSourceVariable and BaseSSA::BaseSsaVariable, that may be worth just fixing in this PR so the alert stops going off.)

All fixed.