Java: add more Spring RestTemplate request forgery sinks#20930
Java: add more Spring RestTemplate request forgery sinks#20930owen-mc merged 6 commits intogithub:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends SSRF (Server-Side Request Forgery) detection for Spring RestTemplate to include all methods with uriVariables parameters, not just getForObject. This addresses customer feedback about enterprise code usage patterns that weren't previously covered.
Key changes:
- Refactored specific
getForObjectsink detection to a generic approach covering allRestTemplatemethods withuriVariablesparameters - Added comprehensive test coverage for all 11 affected methods (
delete,exchange,execute,getForEntity,getForObject,headForHeaders,optionsForAllow,patchForObject,postForEntity,postForLocation,postForObject,put) - Updated expected test results to reflect the expanded sink detection
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
java/ql/lib/semmle/code/java/frameworks/spring/SpringWebClient.qll |
Refactored sink detection from method-specific to generic class that handles all RestTemplate methods with uriVariables parameters; added method position mapping |
java/ql/test/query-tests/security/CWE-918/SpringSSRF.java |
Added comprehensive test cases for all 11 RestTemplate methods with various URL patterns and variable substitution scenarios |
java/ql/test/query-tests/security/CWE-918/RequestForgery.expected |
Updated expected results to include new alerts from expanded RestTemplate method coverage |
java/ql/lib/change-notes/2025-11-27-spring-rest-template-request-forgery-sinks.md |
Added change note documenting the expansion of SSRF sink detection |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
java/ql/lib/semmle/code/java/frameworks/spring/SpringWebClient.qll
Outdated
Show resolved
Hide resolved
java/ql/lib/semmle/code/java/frameworks/spring/SpringWebClient.qll
Outdated
Show resolved
Hide resolved
java/ql/lib/semmle/code/java/frameworks/spring/SpringWebClient.qll
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
java/ql/lib/semmle/code/java/frameworks/spring/SpringWebClient.qll
Outdated
Show resolved
Hide resolved
michaelnebel
left a comment
There was a problem hiding this comment.
Very nice with some more sinks.
| int pos; | ||
|
|
||
| SpringRestTemplateMethodWithUriVariablesParameter() { | ||
| this.getDeclaringType() instanceof SpringRestTemplate and |
There was a problem hiding this comment.
- What about
patchForObject? - The current method filtering also captures overloads that doesn't have a
uriVariablesparameter. Is that intended?
Another approach could be to declare something like
private class SpringRestTemplateMethodWithUriVariablesParameter extends Method {
int pos;
SpringRestTemplateMethodWithUriVariablesParameter() {
this.getDeclaringType() instanceof SpringRestTemplate and
this.getParameter(pos).getName() = "uriVariables"
}
int getUriVariablesPosition() { result = pos }
}There was a problem hiding this comment.
patchForObjectis in the list of method names, and it is tested. I'm not sure what you mean. But it's probably irrelevant, because of the next point.- Yes, that's much better! I'm used to Go, which doesn't have overloads, so I'm not familiar with dealing with them and I'd never thought of that approach. I've pushed a commit which uses your suggestion.
There was a problem hiding this comment.
Ups, sorry - didn't see it in the list.
This is an extension of #18153 to include all the other methods on the class
RestTemplatewhich have a parameter nameduriVariables. They should all be request forgery sinks, but the original PR only did it forgetForObject.It is hard to do performance analysis or evaluate precision because they are very few uses of these APIs in public repos. (We know from customer feedback that they are used in enterprise code.) I looked at the tuple counts from running the query on the test and didn't see any sign of bad join orders.