From cc75a1a97e4c95072edfc7da663522154edf7340 Mon Sep 17 00:00:00 2001
From: Anders Schack-Mulligen <aschackmull@github.com>
Date: Tue, 7 Mar 2023 11:39:03 +0100
Subject: [PATCH] Java: Refactor RequestForgery.ql

---
 .../java/security/RequestForgeryConfig.qll    | 27 ++++++++++++++++++-
 .../Security/CWE/CWE-918/RequestForgery.ql    |  6 ++---
 2 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/java/ql/lib/semmle/code/java/security/RequestForgeryConfig.qll b/java/ql/lib/semmle/code/java/security/RequestForgeryConfig.qll
index c8a59f65b0e..62755d28277 100644
--- a/java/ql/lib/semmle/code/java/security/RequestForgeryConfig.qll
+++ b/java/ql/lib/semmle/code/java/security/RequestForgeryConfig.qll
@@ -8,9 +8,11 @@ import semmle.code.java.dataflow.FlowSources
 import semmle.code.java.security.RequestForgery
 
 /**
+ * DEPRECATED: Use `RequestForgeryConfiguration` module instead.
+ *
  * A taint-tracking configuration characterising request-forgery risks.
  */
-class RequestForgeryConfiguration extends TaintTracking::Configuration {
+deprecated class RequestForgeryConfiguration extends TaintTracking::Configuration {
   RequestForgeryConfiguration() { this = "Server-Side Request Forgery" }
 
   override predicate isSource(DataFlow::Node source) {
@@ -29,3 +31,26 @@ class RequestForgeryConfiguration extends TaintTracking::Configuration {
 
   override predicate isSanitizer(DataFlow::Node node) { node instanceof RequestForgerySanitizer }
 }
+
+/**
+ * A taint-tracking configuration characterising request-forgery risks.
+ */
+module RequestForgeryConfiguration implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) {
+    source instanceof RemoteFlowSource and
+    // Exclude results of remote HTTP requests: fetching something else based on that result
+    // is no worse than following a redirect returned by the remote server, and typically
+    // we're requesting a resource via https which we trust to only send us to safe URLs.
+    not source.asExpr().(MethodAccess).getCallee() instanceof UrlConnectionGetInputStreamMethod
+  }
+
+  predicate isSink(DataFlow::Node sink) { sink instanceof RequestForgerySink }
+
+  predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
+    any(RequestForgeryAdditionalTaintStep r).propagatesTaint(pred, succ)
+  }
+
+  predicate isBarrier(DataFlow::Node node) { node instanceof RequestForgerySanitizer }
+}
+
+module RequestForgeryFlow = TaintTracking::Make<RequestForgeryConfiguration>;
diff --git a/java/ql/src/Security/CWE/CWE-918/RequestForgery.ql b/java/ql/src/Security/CWE/CWE-918/RequestForgery.ql
index 604dd366513..c3455dc4beb 100644
--- a/java/ql/src/Security/CWE/CWE-918/RequestForgery.ql
+++ b/java/ql/src/Security/CWE/CWE-918/RequestForgery.ql
@@ -13,9 +13,9 @@
 
 import java
 import semmle.code.java.security.RequestForgeryConfig
-import DataFlow::PathGraph
+import RequestForgeryFlow::PathGraph
 
-from DataFlow::PathNode source, DataFlow::PathNode sink, RequestForgeryConfiguration conf
-where conf.hasFlowPath(source, sink)
+from RequestForgeryFlow::PathNode source, RequestForgeryFlow::PathNode sink
+where RequestForgeryFlow::hasFlowPath(source, sink)
 select sink.getNode(), source, sink, "Potential server-side request forgery due to a $@.",
   source.getNode(), "user-provided value"