GlobalDataFlow - support qualified static variables

[crankydillo]

What problem are you trying to solve?

I need to modify parts of the project based on the state of String literals that are scattered around the code-base. Ideally there would be a guarantee that any 'sink' traces back to a value that is immutable. I don't fully understand the code behind the GlobalDataFlow accumulator, but it does seem to cover quite a few of our case. However, it does not seem to work (at least in the way we expect it when a sink deals with a qualified constant (e.g. Constants.FOO).

Initially discussed here.

I have a similar use-case for enums, but I'll create a separate issue for that.

Have you considered any alternatives or workarounds?

For (public) constants, it's kind of easy to find those via a Scanning Recipe; however, there is an issue with tracking access if not public. Same for enums. But I haven't been able to solve a 'method chain' back to them, which I believe is one of the fundamental purposes of GlobalDataFlow.

Are you interested in contributing this feature to OpenRewrite?

Minimally, I'll create a test case. I have been trying to debug the code, but I'm not sure yet where the best place to work on this is. I can see that's it not going in as an 'edge', but not sure if that would be even appropriate.. Is the 'edge' the class, the qualified constant or neither:)

[JLLeitschuh]

However, it does not seem to work (at least in the way we expect it when a sink deals with a qualified constant (e.g. Constants.FOO).

I never truly got it working and I wasn't certain why. It's been a long time since I've looked at the code. There are still some pretty serious bugs in it that have never been resolved. I don't work for Moderne. If you want to discuss your use case and the problem you're trying to solve in more depth feel free to setup some time on my calendar and maybe I can assist you:

https://calendly.com/JLLeitschuh

[crankydillo]

However, it does not seem to work (at least in the way we expect it when a sink deals with a qualified constant (e.g. Constants.FOO).

I never truly got it working and I wasn't certain why. It's been a long time since I've looked at the code. There are still some pretty serious bugs in it that have never been resolved. I don't work for Moderne.

Just based on browsing around the code/issues/etc., I did wonder who owned this:) This really does seem like it contains the fundamentals of what we need (some kind of call/call-flow graph) to easily make code modifications based on method chains that reach some 'source'. @timtebeek what's your stance on this? Is there a better project out there that I'm unaware of? Does this project get a lot of use as far as you know? Does moderne have any immediate plans to work on this?

@JLLeitschuh regarding the bugs, are aware of any 'fundamental' bugs that basically makes this a start-from-scratch situation? Are all the big bugs recorded as issues?

If you want to discuss your use case and the problem you're trying to solve in more depth feel free to setup some time on my calendar and maybe I can assist you:

https://calendly.com/JLLeitschuh

Thanks for this! I have been pursuing this as part of a PoC that may not take priority over other work. Those decisions will be made soon. If I continue on, I'll likely find some time on your calendar.

[JLLeitschuh]

In general, I and my interns across 2022 and 2023 have been the primary authors of this codebase.

Unfortunately the primary consumer for this code rewrite-java-security was closed-soured by Moderne in the recent license changes.

https://infosecwriteups.com/when-open-source-isnt-how-openrewrite-lost-its-way-642053be287d

If you're interested, I'm currently available to offer my services as a consultant to get this working, but I'd like to better understand your use case.

IIRC, the major bugs were that multi-file dataflow didn't work properly. Also, there are issues with storing large data flow ASTs in memory

[crankydillo]

In general, I and my interns across 2022 and 2023 have been the primary authors of this codebase.

Unfortunately the primary consumer for this code rewrite-java-security was closed-soured by Moderne in the recent license changes.

https://infosecwriteups.com/when-open-source-isnt-how-openrewrite-lost-its-way-642053be287d

If you're interested, I'm currently available to offer my services as a consultant to get this working, but I'd like to better understand your use case.

Thanks. I'll keep this in mind.

IIRC, the major bugs were that multi-file dataflow didn't work properly. Also, there are issues with storing large data flow ASTs in memory

Multi-file 'data flow' (if I'm correctly understanding what you mean by 'data flow') is exactly what we need. While they do not include multi-files, you should see some of the core issues we have in the test PR attached to this issue. Is there a github issue that discusses the issue(s) you refer to?

[JLLeitschuh]

There are a couple of ways you could solve this. Data-flow is definitely one of them. CodeQL solves this a different way, by using a construct called CompileTimeConstantExpr:

• https://codeql.github.com/codeql-standard-libraries/java/semmle/code/java/Expr.qll/type.Expr$CompileTimeConstantExpr.html
• https://github.com/github/codeql/blob/d17c931939fdba4ffb7c0c4e33d3ebe4c6cdd8b6/java/ql/lib/semmle/code/java/Expr.qll#L131-L267

When writing CodeQL queries, this construct can be used to lookup CompileTimeConstant values across files or (IIRC) in library code. (I don't know if it would be possible to do the latter in OpenRewrite)

Doing this in OpenRewrite is going to require an accumulator to accumulate any compile time constants, and then a solver to compute the constant values. You can see that "solve" logic in CodeQL. Doing this would be simpler than multi-file dataflow. Thoughts?

[timtebeek]

We're working to make compile time constant values available for type table entries in this PR:

• Add support for annotations to TypeTable rewrite#5485

That should make it a whole lot easier to do the things you're after, as it shifts the complexity out of recipes to building up type tables. We'll likely extend that at some point in the near future to LST elements as well, for a much improved experience writing recipes. That's also why we've not been looking to extend this module: the serious bugs mentioned above, as well as the complexities both maintaining the module as well as recipes that use these patterns we feel are better served with a wholly different approach.

[crankydillo]

[snip]

Doing this in OpenRewrite is going to require an accumulator to accumulate any compile time constants, and then a solver to compute the constant values. You can see that "solve" logic in CodeQL. Doing this would be simpler than multi-file dataflow. Thoughts?

Generally aware of this solution. One of the first things we did was build an accumulator that captured constants, but it only worked if the constants were actual literals. As you say, you'd also have to 'reduce' (if possible) the expression if you wanted a general solution. While I'm aware expression reduction solutions, I was hoping to not have to do any of that if it's already done. I was hoping this project gave that:)

Before considering other options, I'm first trying to find out if this project has major, show-stopper problems when it comes to the multi-file data flow feature. It seems very close to what we need.

This really is our exact use case (in pseudocode):

J.MethodInvocation doVisitMethodInvocation(mi) {
   return codeQl.evaluateToLiteral(toCodeQlExpr(mi.getFirstArg))    // returns optional
               .filter(lit -> isNumeric(lit))
               .map(ignore -> newMethodInvociation())
               .orElse(mi);
}

You may be insinuating I'd build the codeql stuff myself (for my case). Whether codeql or custom code, it seems like we have 2 project-wide ASTs in memory (openrewrite's and codeql/custom-expr-model) or some form of lazy evaluation.

[crankydillo]

We're working to make compile time constant values available for type table entries in this PR:

• Add support for annotations to TypeTable rewrite#5485

That should make it a whole lot easier to do the things you're after, as it shifts the complexity out of recipes to building up type tables. We'll likely extend that at some point in the near future to LST elements as well, for a much improved experience writing recipes. That's also why we've not been looking to extend this module: the serious bugs mentioned above, as well as the complexities both maintaining the module as well as recipes that use these patterns we feel are better served with a wholly different approach.

RE 'the serious bug', that's what I was trying to find:). Regardless, I don't have to know those if the owners of this already know this planned for replacement. If it is, might think of some way to indicate that.

I do recall that PR being mentioned when I first opened the discussion, but I wasn't sure how far off you were to any 'expression-like' LST element ultimately having easy access to a resolved, immutable literal value.

[JLLeitschuh]

RE "serious bug": IIRC I think the current state of Global data flow was that it just didn't work yet. Local data flow does and has been well tested, but global does not currently work yet unfortunately. I haven't touched it in a while

[crankydillo]

@timtebeek You are saying we'll be able to implement recipes for use cases like I have created in #83 (and basically quite a bit of the existing tests in this project) with the work you mention here?

If so, is there some way I can track progress on that? Maybe an issue I can watch.

Update: When I mention the test cases, I'm not referring to the 'source'/'sink' marker assertions, but what those enable (changing code in one part of the project based on the state of code in other parts of the project).

[timtebeek]

Best to look at this PR, and work to follow:

• Add support for annotations to TypeTable rewrite#5485

In parallel we're also working on a reimplementation of control flow and data flow in an internal module; we'll look to open that up once the API stabilizes.