Java: Address review comments and some other code quality improvements.

michaelnebel · michaelnebel · commit 5245fd847fdf · 2024-05-03T11:09:34.000+02:00
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
   }

Original file line number	Diff line number	Diff line change
`@@ -99,16 +99,16 @@ private predicate localDatabaseStore(DataFlow::Node database, MethodCall store)`
`99`	`99`	`}`
`100`	`100`
`101`	`101`	`/**`
`102`		`- * A class of local database open method call source nodes.`
	`102`	`+ * A local database open method call source node.`
`103`	`103`	`*/`
`104`		`-class LocalDatabaseOpenMethodCallSource extends ApiSourceNode {`
	`104`	`+private class LocalDatabaseOpenMethodCallSource extends ApiSourceNode {`
`105`	`105`	`LocalDatabaseOpenMethodCallSource() { this.asExpr() instanceof LocalDatabaseOpenMethodCall }`
`106`	`106`	`}`
`107`	`107`
`108`	`108`	`/**`
`109`		`- * A class of local database sink nodes.`
	`109`	`+ * A local database sink node.`
`110`	`110`	`*/`
`111`		`-class LocalDatabaseSink extends ApiSinkNode {`
	`111`	`+private class LocalDatabaseSink extends ApiSinkNode {`
`112`	`112`	`LocalDatabaseSink() { localDatabaseInput(this, _) or localDatabaseStore(this, _) }`
`113`	`113`	`}`
`114`	`114`
Original file line number	Diff line number	Diff line change
`@@ -40,16 +40,16 @@ private predicate cookieStore(DataFlow::Node cookie, Expr store) {`
`40`	`40`	`}`
`41`	`41`
`42`	`42`	`/**`
`43`		`- * A class of cookie source nodes.`
	`43`	`+ * A cookie source node.`
`44`	`44`	`*/`
`45`		`-class CookieSource extends ApiSourceNode {`
	`45`	`+private class CookieSource extends ApiSourceNode {`
`46`	`46`	`CookieSource() { this.asExpr() instanceof Cookie }`
`47`	`47`	`}`
`48`	`48`
`49`	`49`	`/**`
`50`		`- * A class of cookie store sink nodes.`
	`50`	`+ * A cookie store sink node.`
`51`	`51`	`*/`
`52`		`-class CookieStoreSink extends ApiSinkNode {`
	`52`	`+private class CookieStoreSink extends ApiSinkNode {`
`53`	`53`	`CookieStoreSink() { cookieStore(this, _) }`
`54`	`54`	`}`
`55`	`55`