From 8def1c2c137e6bedf306dff0f3769f55ac64f266 Mon Sep 17 00:00:00 2001
From: Michael Nebel <michaelnebel@github.com>
Date: Fri, 3 May 2024 11:09:34 +0200
Subject: [PATCH] Java: Address review comments and some other code quality
 improvements.

---
 .../semmle/code/java/dataflow/FlowSinks.qll   |  2 +-
 .../AndroidSensitiveCommunicationQuery.qll    |  4 ++--
 .../CleartextStorageAndroidDatabaseQuery.qll  |  8 ++++----
 ...CleartextStorageAndroidFilesystemQuery.qll |  8 ++++----
 .../security/CleartextStorageCookieQuery.qll  |  8 ++++----
 .../CleartextStorageSharedPrefsQuery.qll      |  8 ++++----
 .../ExternallyControlledFormatStringQuery.qll |  4 ++--
 .../java/security/ImplicitPendingIntents.qll  |  2 --
 .../ImproperIntentVerificationQuery.qll       | 14 +++-----------
 .../java/security/InsecureRandomnessQuery.qll |  3 ++-
 java/ql/lib/semmle/code/java/security/JWT.qll |  1 -
 .../security/SensitiveResultReceiverQuery.qll |  4 ++--
 .../code/java/security/SensitiveUiQuery.qll   |  4 ++--
 .../java/security/StackTraceExposureQuery.qll |  6 ++----
 ...TempDirLocalInformationDisclosureQuery.qll | 19 ++++++-------------
 .../security/WebviewDebuggingEnabledQuery.qll |  4 ++--
 java/ql/lib/semmle/code/java/security/XSS.qll |  2 +-
 .../code/java/security/ZipSlipQuery.qll       |  4 ++--
 18 files changed, 43 insertions(+), 62 deletions(-)

diff --git a/java/ql/lib/semmle/code/java/dataflow/FlowSinks.qll b/java/ql/lib/semmle/code/java/dataflow/FlowSinks.qll
index 3b7fd191779..72cd96f6745 100644
--- a/java/ql/lib/semmle/code/java/dataflow/FlowSinks.qll
+++ b/java/ql/lib/semmle/code/java/dataflow/FlowSinks.qll
@@ -11,7 +11,7 @@ private import semmle.code.java.dataflow.DataFlow
 abstract class ApiSinkNode extends DataFlow::Node { }
 
 /**
- * Add all models as data sinks.
+ * Add all sink models as data sinks.
  */
 private class ApiSinkNodeExternal extends ApiSinkNode {
   ApiSinkNodeExternal() { sinkNode(this, _) }
diff --git a/java/ql/lib/semmle/code/java/security/AndroidSensitiveCommunicationQuery.qll b/java/ql/lib/semmle/code/java/security/AndroidSensitiveCommunicationQuery.qll
index 607ced09b2c..a4f9713ac30 100644
--- a/java/ql/lib/semmle/code/java/security/AndroidSensitiveCommunicationQuery.qll
+++ b/java/ql/lib/semmle/code/java/security/AndroidSensitiveCommunicationQuery.qll
@@ -153,9 +153,9 @@ deprecated class SensitiveCommunicationConfig extends TaintTracking::Configurati
 }
 
 /**
- * A class of sensitive communication sink nodes.
+ * A sensitive communication sink node.
  */
-class SensitiveCommunicationSink extends ApiSinkNode {
+private class SensitiveCommunicationSink extends ApiSinkNode {
   SensitiveCommunicationSink() {
     isSensitiveBroadcastSink(this)
     or
diff --git a/java/ql/lib/semmle/code/java/security/CleartextStorageAndroidDatabaseQuery.qll b/java/ql/lib/semmle/code/java/security/CleartextStorageAndroidDatabaseQuery.qll
index b4162f2c695..5ee9248d9eb 100644
--- a/java/ql/lib/semmle/code/java/security/CleartextStorageAndroidDatabaseQuery.qll
+++ b/java/ql/lib/semmle/code/java/security/CleartextStorageAndroidDatabaseQuery.qll
@@ -99,16 +99,16 @@ private predicate localDatabaseStore(DataFlow::Node database, MethodCall store)
 }
 
 /**
- * A class of local database open method call source nodes.
+ * A local database open method call source node.
  */
-class LocalDatabaseOpenMethodCallSource extends ApiSourceNode {
+private class LocalDatabaseOpenMethodCallSource extends ApiSourceNode {
   LocalDatabaseOpenMethodCallSource() { this.asExpr() instanceof LocalDatabaseOpenMethodCall }
 }
 
 /**
- * A class of local database sink nodes.
+ * A local database sink node.
  */
-class LocalDatabaseSink extends ApiSinkNode {
+private class LocalDatabaseSink extends ApiSinkNode {
   LocalDatabaseSink() { localDatabaseInput(this, _) or localDatabaseStore(this, _) }
 }
 
diff --git a/java/ql/lib/semmle/code/java/security/CleartextStorageAndroidFilesystemQuery.qll b/java/ql/lib/semmle/code/java/security/CleartextStorageAndroidFilesystemQuery.qll
index 8b1af7b4971..06fa8381312 100644
--- a/java/ql/lib/semmle/code/java/security/CleartextStorageAndroidFilesystemQuery.qll
+++ b/java/ql/lib/semmle/code/java/security/CleartextStorageAndroidFilesystemQuery.qll
@@ -82,16 +82,16 @@ private class CloseFileMethod extends Method {
 }
 
 /**
- * A class of local file open call source nodes.
+ * A local file open call source node.
  */
-class LocalFileOpenCallSource extends ApiSourceNode {
+private class LocalFileOpenCallSource extends ApiSourceNode {
   LocalFileOpenCallSource() { this.asExpr() instanceof LocalFileOpenCall }
 }
 
 /**
- * A class of local file sink nodes.
+ * A local file sink node.
  */
-class LocalFileSink extends ApiSinkNode {
+private class LocalFileSink extends ApiSinkNode {
   LocalFileSink() {
     filesystemInput(this, _) or
     closesFile(this, _)
diff --git a/java/ql/lib/semmle/code/java/security/CleartextStorageCookieQuery.qll b/java/ql/lib/semmle/code/java/security/CleartextStorageCookieQuery.qll
index c3684646bdd..a36a4754584 100644
--- a/java/ql/lib/semmle/code/java/security/CleartextStorageCookieQuery.qll
+++ b/java/ql/lib/semmle/code/java/security/CleartextStorageCookieQuery.qll
@@ -40,16 +40,16 @@ private predicate cookieStore(DataFlow::Node cookie, Expr store) {
 }
 
 /**
- * A class of cookie source nodes.
+ * A cookie source node.
  */
-class CookieSource extends ApiSourceNode {
+private class CookieSource extends ApiSourceNode {
   CookieSource() { this.asExpr() instanceof Cookie }
 }
 
 /**
- * A class of cookie store sink nodes.
+ * A cookie store sink node.
  */
-class CookieStoreSink extends ApiSinkNode {
+private class CookieStoreSink extends ApiSinkNode {
   CookieStoreSink() { cookieStore(this, _) }
 }
 
diff --git a/java/ql/lib/semmle/code/java/security/CleartextStorageSharedPrefsQuery.qll b/java/ql/lib/semmle/code/java/security/CleartextStorageSharedPrefsQuery.qll
index 80dc2fca1f4..f72d40106e3 100644
--- a/java/ql/lib/semmle/code/java/security/CleartextStorageSharedPrefsQuery.qll
+++ b/java/ql/lib/semmle/code/java/security/CleartextStorageSharedPrefsQuery.qll
@@ -70,18 +70,18 @@ private predicate sharedPreferencesStore(DataFlow::Node editor, MethodCall m) {
 }
 
 /**
- * A shared preferences editor method call source nodes.
+ * A shared preferences editor method call source node.
  */
-class SharedPreferencesEditorMethodCallSource extends ApiSourceNode {
+private class SharedPreferencesEditorMethodCallSource extends ApiSourceNode {
   SharedPreferencesEditorMethodCallSource() {
     this.asExpr() instanceof SharedPreferencesEditorMethodCall
   }
 }
 
 /**
- * A class of shared preferences sink nodes.
+ * A shared preferences sink node.
  */
-class SharedPreferencesSink extends ApiSinkNode {
+private class SharedPreferencesSink extends ApiSinkNode {
   SharedPreferencesSink() {
     sharedPreferencesInput(this, _) or
     sharedPreferencesStore(this, _)
diff --git a/java/ql/lib/semmle/code/java/security/ExternallyControlledFormatStringQuery.qll b/java/ql/lib/semmle/code/java/security/ExternallyControlledFormatStringQuery.qll
index 8d6fe0426c3..606e31a07cb 100644
--- a/java/ql/lib/semmle/code/java/security/ExternallyControlledFormatStringQuery.qll
+++ b/java/ql/lib/semmle/code/java/security/ExternallyControlledFormatStringQuery.qll
@@ -6,9 +6,9 @@ private import semmle.code.java.dataflow.FlowSources
 private import semmle.code.java.StringFormat
 
 /**
- * A class of string format sink nodes.
+ * A string format sink node.
  */
-class StringFormatSink extends ApiSinkNode {
+private class StringFormatSink extends ApiSinkNode {
   StringFormatSink() { this.asExpr() = any(StringFormat formatCall).getFormatArgument() }
 }
 
diff --git a/java/ql/lib/semmle/code/java/security/ImplicitPendingIntents.qll b/java/ql/lib/semmle/code/java/security/ImplicitPendingIntents.qll
index 5c4094de3d3..a5d8f256b03 100644
--- a/java/ql/lib/semmle/code/java/security/ImplicitPendingIntents.qll
+++ b/java/ql/lib/semmle/code/java/security/ImplicitPendingIntents.qll
@@ -3,8 +3,6 @@
 import java
 private import semmle.code.java.dataflow.ExternalFlow
 private import semmle.code.java.dataflow.FlowSources
-private import semmle.code.java.dataflow.TaintTracking
-private import semmle.code.java.frameworks.android.Intent
 private import semmle.code.java.frameworks.android.PendingIntent
 
 private newtype TPendingIntentState =
diff --git a/java/ql/lib/semmle/code/java/security/ImproperIntentVerificationQuery.qll b/java/ql/lib/semmle/code/java/security/ImproperIntentVerificationQuery.qll
index e8bfc97b0fc..bca045bc8e4 100644
--- a/java/ql/lib/semmle/code/java/security/ImproperIntentVerificationQuery.qll
+++ b/java/ql/lib/semmle/code/java/security/ImproperIntentVerificationQuery.qll
@@ -4,7 +4,6 @@ import java
 import semmle.code.java.dataflow.DataFlow
 import semmle.code.xml.AndroidManifest
 import semmle.code.java.frameworks.android.Intent
-private import semmle.code.java.dataflow.FlowSources
 
 /** An `onReceive` method of a `BroadcastReceiver` */
 private class OnReceiveMethod extends Method {
@@ -14,18 +13,11 @@ private class OnReceiveMethod extends Method {
   Parameter getIntentParameter() { result = this.getParameter(1) }
 }
 
-/**
- * A class of verified intent source nodes.
- */
-class VerifiedIntentConfigSource extends ApiSourceNode {
-  VerifiedIntentConfigSource() {
-    this.asParameter() = any(OnReceiveMethod orm).getIntentParameter()
-  }
-}
-
 /** A configuration to detect whether the `action` of an `Intent` is checked. */
 private module VerifiedIntentConfig implements DataFlow::ConfigSig {
-  predicate isSource(DataFlow::Node src) { src instanceof VerifiedIntentConfigSource }
+  predicate isSource(DataFlow::Node src) {
+    src.asParameter() = any(OnReceiveMethod orm).getIntentParameter()
+  }
 
   predicate isSink(DataFlow::Node sink) {
     exists(MethodCall ma |
diff --git a/java/ql/lib/semmle/code/java/security/InsecureRandomnessQuery.qll b/java/ql/lib/semmle/code/java/security/InsecureRandomnessQuery.qll
index f983876d7b3..423046b6746 100644
--- a/java/ql/lib/semmle/code/java/security/InsecureRandomnessQuery.qll
+++ b/java/ql/lib/semmle/code/java/security/InsecureRandomnessQuery.qll
@@ -4,6 +4,7 @@ import java
 private import semmle.code.java.frameworks.OpenSaml
 private import semmle.code.java.frameworks.Servlets
 private import semmle.code.java.dataflow.ExternalFlow
+private import semmle.code.java.dataflow.FlowSinks
 private import semmle.code.java.dataflow.TaintTracking
 private import semmle.code.java.security.Cookies
 private import semmle.code.java.security.RandomQuery
@@ -49,7 +50,7 @@ abstract class InsecureRandomnessSink extends DataFlow::Node { }
 /**
  * A node which sets the value of a cookie.
  */
-private class CookieSink extends InsecureRandomnessSink {
+private class CookieSink extends InsecureRandomnessSink, ApiSinkNode {
   CookieSink() { this.asExpr() instanceof SetCookieValue }
 }
 
diff --git a/java/ql/lib/semmle/code/java/security/JWT.qll b/java/ql/lib/semmle/code/java/security/JWT.qll
index c84ebffabdb..5ba47072dc6 100644
--- a/java/ql/lib/semmle/code/java/security/JWT.qll
+++ b/java/ql/lib/semmle/code/java/security/JWT.qll
@@ -1,7 +1,6 @@
 /** Provides classes for working with JSON Web Token (JWT) libraries. */
 
 import java
-private import semmle.code.java.dataflow.DataFlow
 private import semmle.code.java.dataflow.FlowSinks
 private import semmle.code.java.dataflow.FlowSources
 
diff --git a/java/ql/lib/semmle/code/java/security/SensitiveResultReceiverQuery.qll b/java/ql/lib/semmle/code/java/security/SensitiveResultReceiverQuery.qll
index c0179860a01..8269a42c5c2 100644
--- a/java/ql/lib/semmle/code/java/security/SensitiveResultReceiverQuery.qll
+++ b/java/ql/lib/semmle/code/java/security/SensitiveResultReceiverQuery.qll
@@ -52,9 +52,9 @@ deprecated private class SensitiveResultReceiverConf extends TaintTracking::Conf
 }
 
 /**
- * A class of sensitive result receiver sink nodes.
+ * A sensitive result receiver sink node.
  */
-class SensitiveResultReceiverSink extends ApiSinkNode {
+private class SensitiveResultReceiverSink extends ApiSinkNode {
   SensitiveResultReceiverSink() {
     exists(ResultReceiverSendCall call |
       untrustedResultReceiverSend(_, call) and
diff --git a/java/ql/lib/semmle/code/java/security/SensitiveUiQuery.qll b/java/ql/lib/semmle/code/java/security/SensitiveUiQuery.qll
index f9ff3f24040..a7e76d0e2e3 100644
--- a/java/ql/lib/semmle/code/java/security/SensitiveUiQuery.qll
+++ b/java/ql/lib/semmle/code/java/security/SensitiveUiQuery.qll
@@ -55,9 +55,9 @@ private class MaskCall extends MethodCall {
 }
 
 /**
- * A class of text field sink nodes.
+ * A text field sink node.
  */
-class TextFieldSink extends ApiSinkNode {
+private class TextFieldSink extends ApiSinkNode {
   TextFieldSink() {
     exists(SetTextCall call |
       this.asExpr() = call.getStringArgument() and
diff --git a/java/ql/lib/semmle/code/java/security/StackTraceExposureQuery.qll b/java/ql/lib/semmle/code/java/security/StackTraceExposureQuery.qll
index 5de0b0098e9..1a2fe31e879 100644
--- a/java/ql/lib/semmle/code/java/security/StackTraceExposureQuery.qll
+++ b/java/ql/lib/semmle/code/java/security/StackTraceExposureQuery.qll
@@ -1,9 +1,7 @@
 /** Provides predicates to reason about exposure of stack-traces. */
 
 import java
-private import semmle.code.java.dataflow.DataFlow
 private import semmle.code.java.dataflow.FlowSources
-private import semmle.code.java.dataflow.TaintTracking
 private import semmle.code.java.security.InformationLeak
 
 /**
@@ -97,9 +95,9 @@ predicate stringifiedStackFlowsExternally(DataFlow::Node externalExpr, Expr stac
 }
 
 /**
- * A class of get message source nodes.
+ * A get message source node.
  */
-class GetMessageFlowSource extends ApiSourceNode {
+private class GetMessageFlowSource extends ApiSourceNode {
   GetMessageFlowSource() {
     exists(Method method | this.asExpr().(MethodCall).getMethod() = method |
       method.hasName("getMessage") and
diff --git a/java/ql/lib/semmle/code/java/security/TempDirLocalInformationDisclosureQuery.qll b/java/ql/lib/semmle/code/java/security/TempDirLocalInformationDisclosureQuery.qll
index 96db99fe1b4..f1ffcaecc51 100644
--- a/java/ql/lib/semmle/code/java/security/TempDirLocalInformationDisclosureQuery.qll
+++ b/java/ql/lib/semmle/code/java/security/TempDirLocalInformationDisclosureQuery.qll
@@ -30,7 +30,7 @@ private class MethodFileFileCreation extends MethodFileSystemFileCreation {
 /**
  * A dataflow node that creates a file or directory in the file system.
  */
-abstract private class FileCreationSink extends DataFlow::Node { }
+abstract private class FileCreationSink extends ApiSinkNode { }
 
 /**
  * The qualifier of a call to one of `File`'s file-creating or directory-creating methods,
@@ -154,17 +154,6 @@ module TempDirSystemGetPropertyToCreateConfig implements DataFlow::ConfigSig {
 module TempDirSystemGetPropertyToCreate =
   TaintTracking::Global<TempDirSystemGetPropertyToCreateConfig>;
 
-/**
- * A class of method file directory creation sink nodes.
- */
-class MethodFileDirectoryCreationSink extends ApiSinkNode {
-  MethodFileDirectoryCreationSink() {
-    exists(MethodCall ma | ma.getMethod() instanceof MethodFileDirectoryCreation |
-      ma.getQualifier() = this.asExpr()
-    )
-  }
-}
-
 /**
  * Configuration that tracks calls to to `mkdir` or `mkdirs` that are are directly on the temp directory system property.
  * Examples:
@@ -184,7 +173,11 @@ module TempDirSystemGetPropertyDirectlyToMkdirConfig implements DataFlow::Config
     )
   }
 
-  predicate isSink(DataFlow::Node node) { node instanceof MethodFileDirectoryCreationSink }
+  predicate isSink(DataFlow::Node node) {
+    exists(MethodCall ma | ma.getMethod() instanceof MethodFileDirectoryCreation |
+      ma.getQualifier() = node.asExpr()
+    )
+  }
 
   predicate isBarrier(DataFlow::Node sanitizer) {
     isFileConstructorArgument(sanitizer.asExpr(), _, _)
diff --git a/java/ql/lib/semmle/code/java/security/WebviewDebuggingEnabledQuery.qll b/java/ql/lib/semmle/code/java/security/WebviewDebuggingEnabledQuery.qll
index c7fd51b1c36..f10b0132b5a 100644
--- a/java/ql/lib/semmle/code/java/security/WebviewDebuggingEnabledQuery.qll
+++ b/java/ql/lib/semmle/code/java/security/WebviewDebuggingEnabledQuery.qll
@@ -46,9 +46,9 @@ deprecated class WebviewDebugEnabledConfig extends DataFlow::Configuration {
 }
 
 /**
- * A class of webview debug sink nodes.
+ * A webview debug sink node.
  */
-class WebviewDebugSink extends ApiSinkNode {
+private class WebviewDebugSink extends ApiSinkNode {
   WebviewDebugSink() {
     exists(MethodCall ma |
       ma.getMethod().hasQualifiedName("android.webkit", "WebView", "setWebContentsDebuggingEnabled") and
diff --git a/java/ql/lib/semmle/code/java/security/XSS.qll b/java/ql/lib/semmle/code/java/security/XSS.qll
index daf025141f5..777e5fae062 100644
--- a/java/ql/lib/semmle/code/java/security/XSS.qll
+++ b/java/ql/lib/semmle/code/java/security/XSS.qll
@@ -108,7 +108,7 @@ class XssVulnerableWriterSource extends MethodCall {
 }
 
 /**
- * A class of xss vulnerable writer source nodes.
+ * A xss vulnerable writer source node.
  */
 class XssVulnerableWriterSourceNode extends ApiSourceNode {
   XssVulnerableWriterSourceNode() { this.asExpr() instanceof XssVulnerableWriterSource }
diff --git a/java/ql/lib/semmle/code/java/security/ZipSlipQuery.qll b/java/ql/lib/semmle/code/java/security/ZipSlipQuery.qll
index 0ab889f7372..75e2f7000c5 100644
--- a/java/ql/lib/semmle/code/java/security/ZipSlipQuery.qll
+++ b/java/ql/lib/semmle/code/java/security/ZipSlipQuery.qll
@@ -23,9 +23,9 @@ private class ArchiveEntryNameMethod extends Method {
 }
 
 /**
- * A class of entry name method source nodes.
+ * An entry name method source node.
  */
-class ArchiveEntryNameMethodSource extends ApiSourceNode {
+private class ArchiveEntryNameMethodSource extends ApiSourceNode {
   ArchiveEntryNameMethodSource() {
     this.asExpr().(MethodCall).getMethod() instanceof ArchiveEntryNameMethod
   }