We really shouldn't be adding these steps. Looking into AttackResult it looks like we just need to recognize StringEscapeUtils.escapeJson from org.apache.commons.lang3 as a taint step. Since this is mostly just escaping quotes " then I'd guess that it's a likely candidate for a general taint step, but I'll defer to @atorralba on that.

I tried to model with StringEscapeUtils.escapeJson() and then AttackResult objects will indeed be tainted (partially). However, they will be tainted with an access path (to the tainted field assigned to in the constructor) and apparently this then does not work in the last step, where that object flows into a return statement. I am happy to learn about the way to make that work though.

That's related to the modeling of the sink. Somehow Spring extracts the fields of AttackResult, so whichever rules leads us to model the sink, ought also to allow us to identify the relevant fields in QL. Once we have that, we can specify that these fields are implicitly read at the sink using Configuration::allowImplicitRead(DataFlow::Node node, DataFlow::Content c) (when overriding this predicate it is generally advisable to call super).
See e.g. the default implementation of allowImplicitRead on TaintTracking::Configuration - this specifies that array and collection contents can be implicitly read at sink nodes.

Oh, I didn't know about this, thanks for pointing it out! Sounds very plausible indeed, so I'll wait what Tony has to say about the sinks.

Agreed on adding StringEscapeUtils.escapeJson as a general taint step. The implicit read shouldn't be needed because a method returning an AttackResult isn't actually a XSS sink — see my comment below.

There must be some other and more generic way to capture this. Somehow Spring is able to interprete AttackResult class instances in a generic way, so we should too. @atorralba could you advise?

My understanding is that those things often get Jsonified or Xmlified or however it is configured. I guess the question is then how the data is interpreted on the other side with that side being, for example, JavaScript. I reckon it might cause trouble to say that all tainted return values will cause an XSS, when that might really depend on how the JS code uses the data. In the case of WebGoat it looks like they don't do any filtering of that data, other than removing some JSON characters

AttackResult seems to be the POJO that holds the response data of a call to one of the "attack exercise" APIs. As @zbazztian mentioned, this object is JSONified by Spring before being sent over the network to the client. So, I agree this sink shouldn't be in the standard library since it's too specific to have any use in real-world applications.

Regarding the XSS, this isn't actually a server-side issue but rather a client-side one: the server just exposes an API that returns a JSON object when a request is made -- that isn't XSS and it wouldn't make sense for the server to apply any validation or sanitization to the data, since it isn't being added to an HTML page but rather to a JSON object.

The client, on the other hand, is receiving external data from the server (the response to an API call) and directly adding it to the HTML page without sanitization. So I would say this is something that should be detected by the JavaScript queries, not the Java ones.

The LibrarySink class in the JS libraries seems to account for the relevant sink (jQuery's html method). The source we are interested in isn't included by default: importing AdditionalSources.qll into Customizations.qll should do the trick, since it defines RemoteServerResponse, which models jQuery Ajax call's responses.

Of course, the assumption here is that the attacker is able to manipulate the server's response (e.g. because the response reflects one or more of the request's parameters), but I think it's a good rule of thumb to consider any data coming from external systems as untrusted.

@atorralba Thanks for the detailed explanation, I agree! I will update the PR to only contain the taint steps we identified as generic enough for the standard library.