From 701d0bcdca05a4f238dc4e580d2899ea84022c76 Mon Sep 17 00:00:00 2001
From: Chris Smowton <smowton@github.com>
Date: Tue, 22 Jun 2021 19:26:16 +0100
Subject: [PATCH] Spring content types: recognise constant content-type strings

---
 .../java/frameworks/spring/SpringController.qll    | 13 +++++++++++--
 .../code/java/frameworks/spring/SpringHttp.qll     | 14 ++++++++++++--
 .../security/CWE-079/semmle/tests/SpringXSS.java   |  2 +-
 3 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/java/ql/lib/semmle/code/java/frameworks/spring/SpringController.qll b/java/ql/lib/semmle/code/java/frameworks/spring/SpringController.qll
index f935a4803a9..34bbcca3631 100644
--- a/java/ql/lib/semmle/code/java/frameworks/spring/SpringController.qll
+++ b/java/ql/lib/semmle/code/java/frameworks/spring/SpringController.qll
@@ -121,9 +121,18 @@ class SpringRequestMappingMethod extends SpringControllerMethod {
   SpringRequestMappingParameter getARequestParameter() { result = getAParameter() }
 
   /** Gets the "produces" @RequestMapping annotation value, if present. */
+  Expr getProducesExpr() { result = requestMappingAnnotation.getValue("produces") }
+
+  /** Gets the "produces" @RequestMapping annotation value, if present. */
+  Expr getAProducesExpr() {
+    result = this.getProducesExpr() and not result instanceof ArrayInit
+    or
+    result = this.getProducesExpr().(ArrayInit).getAnInit()
+  }
+
+  /** Gets the "produces" @RequestMapping annotation value, if present and a string constant. */
   string getProduces() {
-    result =
-      requestMappingAnnotation.getValue("produces").(CompileTimeConstantExpr).getStringValue()
+    result = this.getProducesExpr().(CompileTimeConstantExpr).getStringValue()
   }
 
   /** Gets the "value" @RequestMapping annotation value, if present. */
diff --git a/java/ql/lib/semmle/code/java/frameworks/spring/SpringHttp.qll b/java/ql/lib/semmle/code/java/frameworks/spring/SpringHttp.qll
index 8a960728ecb..8090fbf6fa6 100644
--- a/java/ql/lib/semmle/code/java/frameworks/spring/SpringHttp.qll
+++ b/java/ql/lib/semmle/code/java/frameworks/spring/SpringHttp.qll
@@ -145,14 +145,19 @@ private class SpringHttpFlowStep extends SummaryModelCsv {
   }
 }
 
+private predicate specifiesContentType(SpringRequestMappingMethod method) {
+  method.getProducesExpr().(ArrayInit).getSize() != 0 or
+  not method.getProducesExpr() instanceof ArrayInit
+}
+
 private class SpringXssSink extends XSS::XssSink {
   SpringXssSink() {
     exists(SpringRequestMappingMethod requestMappingMethod, ReturnStmt rs |
       requestMappingMethod = rs.getEnclosingCallable() and
       this.asExpr() = rs.getResult() and
       (
-        not exists(requestMappingMethod.getProduces()) or
-        requestMappingMethod.getProduces().matches("text/%")
+        not specifiesContentType(requestMappingMethod) or
+        isXssVulnerableContentTypeExpr(requestMappingMethod.getAProducesExpr())
       )
     |
       // If a Spring request mapping method is either annotated with @ResponseBody (or equivalent),
@@ -251,6 +256,11 @@ private string getSpringConstantContentType(FieldAccess e) {
   )
 }
 
+private predicate isXssVulnerableContentTypeExpr(Expr e) {
+  XSS::isXssVulnerableContentType(e.(CompileTimeConstantExpr).getStringValue()) or
+  XSS::isXssVulnerableContentType(getSpringConstantContentType(e))
+}
+
 private predicate isXssSafeContentTypeExpr(Expr e) {
   XSS::isXssSafeContentType(e.(CompileTimeConstantExpr).getStringValue()) or
   XSS::isXssSafeContentType(getSpringConstantContentType(e))
diff --git a/java/ql/test/query-tests/security/CWE-079/semmle/tests/SpringXSS.java b/java/ql/test/query-tests/security/CWE-079/semmle/tests/SpringXSS.java
index eecf0167ae2..d8f7b6c6986 100644
--- a/java/ql/test/query-tests/security/CWE-079/semmle/tests/SpringXSS.java
+++ b/java/ql/test/query-tests/security/CWE-079/semmle/tests/SpringXSS.java
@@ -60,7 +60,7 @@ public class SpringXSS {
 
   @GetMapping(value = "/xyz", produces = MediaType.TEXT_HTML_VALUE)
   public static ResponseEntity<String> methodContentTypeUnsafe(String userControlled) {
-    return ResponseEntity.ok(userControlled); // $MISSING: xss
+    return ResponseEntity.ok(userControlled); // $xss
   }
 
   @GetMapping(value = "/xyz", produces = "text/html")