Update documentation, rename things and add more comments to explain how the implementation works, remove filter for test code (prefer to filter in code scanning ui than in query logic)

Author: joefarebrother

java/ql/src/Security/CWE/CWE-1004/SensitiveCookieNotHttpOnly.qhelp

@@ -2,11 +2,13 @@
 <qhelp>
 
   <overview>
-    <p>Cross-Site Scripting (XSS) is categorized as one of the OWASP Top 10 Security Vulnerabilities. The <code>HttpOnly</code> flag directs compatible browsers to prevent client-side script from accessing cookies. Including the <code>HttpOnly</code> flag in the Set-Cookie HTTP response header for a sensitive cookie helps mitigate the risk associated with XSS where an attacker's script code attempts to read the contents of a cookie and exfiltrate information obtained.</p>
+    <p>Cookies without the <code>HttpOnly</code> flag set are accessible to client-side scripts (such as JavaScript) running in the same origin. 
+In case of a Cross-Site Scripting (XSS) vulnerability, the cookie can be stolen by a malicious script.
+If a sensitive cookie does not need to be accessed directly by client-side scripts, the <code>HttpOnly</code> flag should be set.</p>
   </overview>
 
   <recommendation>
-    <p>Use the <code>HttpOnly</code> flag when generating a cookie containing sensitive information to help mitigate the risk of client side script accessing the protected cookie.</p>
+    <p>Use the <code>HttpOnly</code> flag when generating a cookie containing sensitive information to help mitigate the risk of client-side scripts accessing the protected cookie.</p>
   </recommendation>
 
   <example>
@@ -23,5 +25,6 @@
       OWASP:
       <a href="https://owasp.org/www-community/HttpOnly">HttpOnly</a>
     </li>
+    <li>MDN: <a href="https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Set-Cookie#httponly">Set-Cookie HttpOnly</a>.</li>
   </references>
 </qhelp>

java/ql/src/Security/CWE/CWE-1004/SensitiveCookieNotHttpOnly.ql

@@ -14,7 +14,7 @@
  * Sketch of the structure of this query: we track cookie names that appear to be sensitive
  * (e.g. `session` or `token`) to a `ServletResponse.addHeader(...)` or `.addCookie(...)`
  * method that does not set the `httpOnly` flag. Subsidiary configurations
- * `MatchesHttpOnlyConfiguration` and `SetHttpOnlyInCookieConfiguration` are used to establish
+ * `MatchesHttpOnlyToRawHeaderConfiguration` and `SetHttpOnlyInCookieConfiguration` are used to establish
  * when the `httpOnly` flag is likely to have been set, before configuration
  * `MissingHttpOnlyConfiguration` establishes that a non-`httpOnly` cookie has a sensitive-seeming name.
  */
@@ -49,8 +49,8 @@ class SensitiveCookieNameExpr extends Expr {
 }
 
 /** A method call that sets a `Set-Cookie` header. */
-class SetCookieMethodCall extends MethodCall {
-  SetCookieMethodCall() {
+class SetCookieRawHeaderMethodCall extends MethodCall {
+  SetCookieRawHeaderMethodCall() {
     (
       this.getMethod() instanceof ResponseAddHeaderMethod or
       this.getMethod() instanceof ResponseSetHeaderMethod
@@ -61,19 +61,19 @@ class SetCookieMethodCall extends MethodCall {
 
 /**
  * A taint configuration tracking flow from the text `httponly` to argument 1 of
- * `SetCookieMethodCall`.
+ * `SetCookieRawHeaderMethodCall`.
  */
-module MatchesHttpOnlyConfig implements DataFlow::ConfigSig {
+module MatchesHttpOnlyToRawHeaderConfig implements DataFlow::ConfigSig {
   predicate isSource(DataFlow::Node source) {
     source.asExpr().(CompileTimeConstantExpr).getStringValue().toLowerCase().matches("%httponly%")
   }
 
   predicate isSink(DataFlow::Node sink) {
-    sink.asExpr() = any(SetCookieMethodCall ma).getArgument(1)
+    sink.asExpr() = any(SetCookieRawHeaderMethodCall ma).getArgument(1)
   }
 }
 
-module MatchesHttpOnlyFlow = TaintTracking::Global<MatchesHttpOnlyConfig>;
+module MatchesHttpOnlyToRawHeaderFlow = TaintTracking::Global<MatchesHttpOnlyToRawHeaderConfig>;
 
 /** A class descended from `javax.servlet.http.Cookie`. */
 class CookieClass extends RefType {
@@ -101,30 +101,11 @@ predicate removesCookie(MethodCall ma) {
   ma.getArgument(0).(IntegerLiteral).getIntValue() = 0
 }
 
-/**
- * Holds if the MethodCall `ma` is a test method call indicated by:
- *    a) in a test directory such as `src/test/java`
- *    b) in a test package whose name has the word `test`
- *    c) in a test class whose name has the word `test`
- *    d) in a test class implementing a test framework such as JUnit or TestNG
- */
-predicate isTestMethod(MethodCall ma) {
-  exists(Method m |
-    m = ma.getEnclosingCallable() and
-    (
-      m.getDeclaringType().getName().toLowerCase().matches("%test%") or // Simple check to exclude test classes to reduce FPs
-      m.getDeclaringType().getPackage().getName().toLowerCase().matches("%test%") or // Simple check to exclude classes in test packages to reduce FPs
-      exists(m.getLocation().getFile().getAbsolutePath().indexOf("/src/test/java")) or //  Match test directory structure of build tools like maven
-      m instanceof TestMethod // Test method of a test case implementing a test framework such as JUnit or TestNG
-    )
-  )
-}
-
 /**
  * A taint configuration tracking flow of a method that sets the `HttpOnly` flag,
  * or one that removes a cookie, to a `ServletResponse.addCookie` call.
  */
-module SetHttpOnlyOrRemovesCookieConfig implements DataFlow::ConfigSig {
+module SetHttpOnlyOrRemovesCookieToAddCookieConfig implements DataFlow::ConfigSig {
   predicate isSource(DataFlow::Node source) {
     source.asExpr() =
       any(MethodCall ma | setsCookieHttpOnly(ma) or removesCookie(ma)).getQualifier()
@@ -136,25 +117,25 @@ module SetHttpOnlyOrRemovesCookieConfig implements DataFlow::ConfigSig {
   }
 }
 
-module SetHttpOnlyOrRemovesCookieFlow = TaintTracking::Global<SetHttpOnlyOrRemovesCookieConfig>;
+module SetHttpOnlyOrRemovesCookieToAddCookieFlow =
+  TaintTracking::Global<SetHttpOnlyOrRemovesCookieToAddCookieConfig>;
 
 /**
  * A cookie that is added to an HTTP response and which doesn't have `httpOnly` set, used as a sink
  * in `MissingHttpOnlyConfiguration`.
  */
-class CookieResponseSink extends DataFlow::ExprNode {
-  CookieResponseSink() {
+class CookieResponseWithoutHttpOnlySink extends DataFlow::ExprNode {
+  CookieResponseWithoutHttpOnlySink() {
     exists(MethodCall ma |
       (
         ma.getMethod() instanceof ResponseAddCookieMethod and
         this.getExpr() = ma.getArgument(0) and
-        not SetHttpOnlyOrRemovesCookieFlow::flowTo(this)
+        not SetHttpOnlyOrRemovesCookieToAddCookieFlow::flowTo(this)
         or
-        ma instanceof SetCookieMethodCall and
+        ma instanceof SetCookieRawHeaderMethodCall and
         this.getExpr() = ma.getArgument(1) and
-        not MatchesHttpOnlyFlow::flowTo(this) // response.addHeader("Set-Cookie", "token=" +authId + ";HttpOnly;Secure")
-      ) and
-      not isTestMethod(ma) // Test class or method
+        not MatchesHttpOnlyToRawHeaderFlow::flowTo(this) // response.addHeader("Set-Cookie", "token=" +authId + ";HttpOnly;Secure")
+      )
     )
   }
 }
@@ -178,14 +159,18 @@ predicate setsHttpOnlyInNewCookie(ClassInstanceExpr cie) {
 /**
  * A taint configuration tracking flow from a sensitive cookie without the `HttpOnly` flag
  * set to its HTTP response.
+ * Tracks string literals containing sensitive names (`SensitiveNameExpr`), to an `addCookie` call (as a `Cookie` object)
+ * or an `addHeader` call (as a string) (`CookieResponseWithoutHttpOnly`).
+ * Passes through `Cookie` constructors and `toString` calls.
  */
 module MissingHttpOnlyConfig implements DataFlow::ConfigSig {
   predicate isSource(DataFlow::Node source) { source.asExpr() instanceof SensitiveCookieNameExpr }
 
-  predicate isSink(DataFlow::Node sink) { sink instanceof CookieResponseSink }
+  predicate isSink(DataFlow::Node sink) { sink instanceof CookieResponseWithoutHttpOnlySink }
 
   predicate isBarrier(DataFlow::Node node) {
     // JAX-RS's `new NewCookie("session-access-key", accessKey, "/", null, null, 0, true, true)` and similar
+    // Cookie constructors, but barriers to considering the flow of the sensitive name, as httponly flag is set.
     setsHttpOnlyInNewCookie(node.asExpr())
   }
 
@@ -211,13 +196,6 @@ module MissingHttpOnlyConfig implements DataFlow::ConfigSig {
 
 module MissingHttpOnlyFlow = TaintTracking::Global<MissingHttpOnlyConfig>;
 
-deprecated query predicate problems(
-  DataFlow::Node sinkNode, MissingHttpOnlyFlow::PathNode source, MissingHttpOnlyFlow::PathNode sink,
-  string message1, DataFlow::Node sourceNode, string message2
-) {
-  MissingHttpOnlyFlow::flowPath(source, sink) and
-  sinkNode = sink.getNode() and
-  message1 = "$@ doesn't have the HttpOnly flag set." and
-  sourceNode = source.getNode() and
-  message2 = "This sensitive cookie"
-}
+from MissingHttpOnlyFlow::PathNode source, MissingHttpOnlyFlow::PathNode sink
+where MissingHttpOnlyFlow::flowPath(source, sink)
+select sink, source, sink, "$@ doesn't have the HttpOnly flag set.", source, "This sensitive cookie"