Address review feedback.

Author: Max Schaefer

java/ql/automodel/src/AutomodelApplicationModeCharacteristics.qll

@@ -93,18 +93,6 @@ abstract private class ApplicationModeEndpoint extends TApplicationModeEndpoint
       else none() // if both exist, it would be a summaryModel (not yet supported)
   }
 
-  /**
-   * Gets a potential type of this endpoint to make sure that sources are
-   * associated with source types and sinks with sink types.
-   */
-  AutomodelEndpointTypes::EndpointType getAPotentialType() {
-    this.getExtensibleType() = "sourceModel" and
-    result instanceof AutomodelEndpointTypes::SourceType
-    or
-    this.getExtensibleType() = "sinkModel" and
-    result instanceof AutomodelEndpointTypes::SinkType
-  }
-
   abstract string toString();
 }
 

java/ql/automodel/src/AutomodelApplicationModeExtractNegativeExamples.ql

@@ -45,22 +45,28 @@ predicate candidate(
   string type, string subtypes, string name, string signature, string input, string output,
   string isVarargsArray, string extensibleType
 ) {
-  // the node is know not to be an endpoint of any appropriate type
-  forall(EndpointType tp | tp = endpoint.getAPotentialType() |
+  // the node is known not to be an endpoint of any appropriate type
+  forall(EndpointType tp | tp = CharacteristicsImpl::getAPotentialType(endpoint) |
     characteristic.hasImplications(tp, false, _)
   ) and
   // the lowest confidence across all endpoint types should be at least highConfidence
-  confidence = min(float c | characteristic.hasImplications(endpoint.getAPotentialType(), false, c)) and
+  confidence =
+    min(float c |
+      characteristic.hasImplications(CharacteristicsImpl::getAPotentialType(endpoint), false, c)
+    ) and
   confidence >= SharedCharacteristics::highConfidence() and
   any(ApplicationModeMetadataExtractor meta)
       .hasMetadata(endpoint, package, type, subtypes, name, signature, input, output,
         isVarargsArray, _, extensibleType) and
   // It's valid for a node to be both a potential source/sanitizer and a sink. We don't want to include such nodes
-  // as negative examples in the prompt, because they're ambiguous and might confuse the model, so we explicitly them here.
+  // as negative examples in the prompt, because they're ambiguous and might confuse the model, so we explicitly exclude them here.
   not exists(EndpointCharacteristic characteristic2, float confidence2 |
+    characteristic2 != characteristic
+  |
     characteristic2.appliesToEndpoint(endpoint) and
     confidence2 >= SharedCharacteristics::maximalConfidence() and
-    characteristic2.hasImplications(endpoint.getAPotentialType(), true, confidence2)
+    characteristic2
+        .hasImplications(CharacteristicsImpl::getAPotentialType(endpoint), true, confidence2)
   )
 }
 

java/ql/automodel/src/AutomodelFrameworkModeCharacteristics.qll

@@ -89,18 +89,6 @@ abstract class FrameworkModeEndpoint extends TFrameworkModeEndpoint {
 
   abstract string getExtensibleType();
 
-  /**
-   * Gets a potential type of this endpoint to make sure that sources are
-   * associated with source types and sinks with sink types.
-   */
-  AutomodelEndpointTypes::EndpointType getAPotentialType() {
-    this.getExtensibleType() = "sourceModel" and
-    result instanceof AutomodelEndpointTypes::SourceType
-    or
-    this.getExtensibleType() = "sinkModel" and
-    result instanceof AutomodelEndpointTypes::SinkType
-  }
-
   string toString() { result = this.asTop().toString() }
 
   Location getLocation() { result = this.asTop().getLocation() }

java/ql/automodel/src/AutomodelFrameworkModeExtractNegativeExamples.ql

@@ -21,20 +21,26 @@ from
 where
   characteristic.appliesToEndpoint(endpoint) and
   // the node is known not to be an endpoint of any appropriate type
-  forall(EndpointType tp | tp = endpoint.getAPotentialType() |
+  forall(EndpointType tp | tp = CharacteristicsImpl::getAPotentialType(endpoint) |
     characteristic.hasImplications(tp, false, _)
   ) and
   // the lowest confidence across all endpoint types should be at least highConfidence
-  confidence = min(float c | characteristic.hasImplications(endpoint.getAPotentialType(), false, c)) and
+  confidence =
+    min(float c |
+      characteristic.hasImplications(CharacteristicsImpl::getAPotentialType(endpoint), false, c)
+    ) and
   confidence >= SharedCharacteristics::highConfidence() and
   meta.hasMetadata(endpoint, package, type, subtypes, name, signature, input, output, parameterName,
     _, extensibleType) and
   // It's valid for a node to be both a potential source/sanitizer and a sink. We don't want to include such nodes
-  // as negative examples in the prompt, because they're ambiguous and might confuse the model, so we explicitly them here.
+  // as negative examples in the prompt, because they're ambiguous and might confuse the model, so we explicitly exclude them here.
   not exists(EndpointCharacteristic characteristic2, float confidence2 |
+    characteristic2 != characteristic
+  |
     characteristic2.appliesToEndpoint(endpoint) and
     confidence2 >= SharedCharacteristics::maximalConfidence() and
-    characteristic2.hasImplications(endpoint.getAPotentialType(), true, confidence2)
+    characteristic2
+        .hasImplications(CharacteristicsImpl::getAPotentialType(endpoint), true, confidence2)
   ) and
   message = characteristic
 select endpoint,

java/ql/automodel/src/AutomodelSharedCharacteristics.qll

@@ -17,7 +17,10 @@ signature module CandidateSig {
    * DataFlow node class, or a subtype thereof.
    */
   class Endpoint {
-    EndpointType getAPotentialType();
+    /**
+     * Gets the kind of this endpoint, either "sourceModel" or "sinkModel".
+     */
+    string getExtensibleType();
   }
 
   /**
@@ -122,14 +125,26 @@ module SharedCharacteristics<CandidateSig Candidate> {
     characteristic.hasImplications(endpointType, true, maximalConfidence())
   }
 
+  /**
+   * Gets a potential type of this endpoint to make sure that sources are
+   * associated with source types and sinks with sink types.
+   */
+  Candidate::EndpointType getAPotentialType(Candidate::Endpoint endpoint) {
+    endpoint.getExtensibleType() = "sourceModel" and
+    result instanceof Candidate::SourceType
+    or
+    endpoint.getExtensibleType() = "sinkModel" and
+    result instanceof Candidate::SinkType
+  }
+
   /**
    * Holds if the given `endpoint` should be considered as a candidate for type `endpointType`,
    * and classified by the ML model.
    *
    * A candidate is an endpoint that cannot be excluded from `endpointType` based on its characteristics.
    */
   predicate isCandidate(Candidate::Endpoint endpoint, Candidate::EndpointType endpointType) {
-    endpointType = endpoint.getAPotentialType() and
+    endpointType = getAPotentialType(endpoint) and
     not exists(getAnExcludingCharacteristic(endpoint, endpointType))
   }
 
@@ -375,7 +390,7 @@ module SharedCharacteristics<CandidateSig Candidate> {
      * A negative characteristic that indicates that an endpoint was manually modeled as a neutral model.
      */
     private class NeutralModelCharacteristic extends NeitherSourceNorSinkCharacteristic {
-      NeutralModelCharacteristic() { this = "known non-endpoint" }
+      NeutralModelCharacteristic() { this = "known non-sink" }
 
       override predicate appliesToEndpoint(Candidate::Endpoint e) { Candidate::isNeutral(e) }
     }

java/ql/automodel/test/AutomodelApplicationModeExtraction/AutomodelApplicationModeExtractNegativeExamples.expected

@@ -1,3 +1,3 @@
 | Test.java:48:10:50:3 | compareTo(...) | known sanitizer\nrelated locations: $@, $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:48:10:50:3 | compareTo(...) | CallContext | Test.java:48:10:50:3 | compareTo(...) | MethodDoc | Test.java:48:10:50:3 | compareTo(...) | ClassDoc | file://java.io:1:1:1:1 | java.io | package | file://File:1:1:1:1 | File | type | file://true:1:1:1:1 | true | subtypes | file://compareTo:1:1:1:1 | compareTo | name | file://(File):1:1:1:1 | (File) | signature | file://:1:1:1:1 |  | input | file://ReturnValue:1:1:1:1 | ReturnValue | output | file://false:1:1:1:1 | false | isVarargsArray | file://sourceModel:1:1:1:1 | sourceModel | extensibleType |
-| Test.java:49:4:49:5 | f2 | known non-endpoint\nrelated locations: $@, $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:48:10:50:3 | compareTo(...) | CallContext | Test.java:49:4:49:5 | f2 | MethodDoc | Test.java:49:4:49:5 | f2 | ClassDoc | file://java.io:1:1:1:1 | java.io | package | file://File:1:1:1:1 | File | type | file://true:1:1:1:1 | true | subtypes | file://compareTo:1:1:1:1 | compareTo | name | file://(File):1:1:1:1 | (File) | signature | file://Argument[0]:1:1:1:1 | Argument[0] | input | file://:1:1:1:1 |  | output | file://false:1:1:1:1 | false | isVarargsArray | file://sinkModel:1:1:1:1 | sinkModel | extensibleType |
+| Test.java:49:4:49:5 | f2 | known non-sink\nrelated locations: $@, $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:48:10:50:3 | compareTo(...) | CallContext | Test.java:49:4:49:5 | f2 | MethodDoc | Test.java:49:4:49:5 | f2 | ClassDoc | file://java.io:1:1:1:1 | java.io | package | file://File:1:1:1:1 | File | type | file://true:1:1:1:1 | true | subtypes | file://compareTo:1:1:1:1 | compareTo | name | file://(File):1:1:1:1 | (File) | signature | file://Argument[0]:1:1:1:1 | Argument[0] | input | file://:1:1:1:1 |  | output | file://false:1:1:1:1 | false | isVarargsArray | file://sinkModel:1:1:1:1 | sinkModel | extensibleType |
 | Test.java:55:4:55:4 | p | taint step\nrelated locations: $@, $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@. | Test.java:54:3:59:3 | walk(...) | CallContext | Test.java:55:4:55:4 | p | MethodDoc | Test.java:55:4:55:4 | p | ClassDoc | file://java.nio.file:1:1:1:1 | java.nio.file | package | file://Files:1:1:1:1 | Files | type | file://false:1:1:1:1 | false | subtypes | file://walk:1:1:1:1 | walk | name | file://(Path,FileVisitOption[]):1:1:1:1 | (Path,FileVisitOption[]) | signature | file://Argument[0]:1:1:1:1 | Argument[0] | input | file://:1:1:1:1 |  | output | file://false:1:1:1:1 | false | isVarargsArray | file://sinkModel:1:1:1:1 | sinkModel | extensibleType |

java/ql/automodel/test/AutomodelFrameworkModeExtraction/AutomodelFrameworkModeExtractNegativeExamples.expected

@@ -1,7 +1,7 @@
 | com/github/codeql/test/PublicClass.java:26:18:26:26 | isIgnored | unexploitable (is-style boolean method)\nrelated locations: $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@. | com/github/codeql/test/PublicClass.java:26:18:26:26 | isIgnored | MethodDoc | com/github/codeql/test/PublicClass.java:26:18:26:26 | isIgnored | ClassDoc | file://com.github.codeql.test:1:1:1:1 | com.github.codeql.test | package | file://PublicClass:1:1:1:1 | PublicClass | type | file://true:1:1:1:1 | true | subtypes | file://isIgnored:1:1:1:1 | isIgnored | name | file://(Object):1:1:1:1 | (Object) | signature | file://:1:1:1:1 |  | input | file://ReturnValue:1:1:1:1 | ReturnValue | output | file://:1:1:1:1 |  | parameterName | file://sourceModel:1:1:1:1 | sourceModel | extensibleType |
 | com/github/codeql/test/PublicClass.java:26:18:26:26 | isIgnored | unexploitable (is-style boolean method)\nrelated locations: $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@. | com/github/codeql/test/PublicClass.java:26:18:26:26 | isIgnored | MethodDoc | com/github/codeql/test/PublicClass.java:26:18:26:26 | isIgnored | ClassDoc | file://com.github.codeql.test:1:1:1:1 | com.github.codeql.test | package | file://PublicClass:1:1:1:1 | PublicClass | type | file://true:1:1:1:1 | true | subtypes | file://isIgnored:1:1:1:1 | isIgnored | name | file://(Object):1:1:1:1 | (Object) | signature | file://Argument[this]:1:1:1:1 | Argument[this] | input | file://:1:1:1:1 |  | output | file://this:1:1:1:1 | this | parameterName | file://sinkModel:1:1:1:1 | sinkModel | extensibleType |
 | com/github/codeql/test/PublicClass.java:26:28:26:39 | input | unexploitable (is-style boolean method)\nrelated locations: $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@. | com/github/codeql/test/PublicClass.java:26:28:26:39 | input | MethodDoc | com/github/codeql/test/PublicClass.java:26:28:26:39 | input | ClassDoc | file://com.github.codeql.test:1:1:1:1 | com.github.codeql.test | package | file://PublicClass:1:1:1:1 | PublicClass | type | file://true:1:1:1:1 | true | subtypes | file://isIgnored:1:1:1:1 | isIgnored | name | file://(Object):1:1:1:1 | (Object) | signature | file://Argument[0]:1:1:1:1 | Argument[0] | input | file://:1:1:1:1 |  | output | file://input:1:1:1:1 | input | parameterName | file://sinkModel:1:1:1:1 | sinkModel | extensibleType |
-| java/io/File.java:4:16:4:24 | compareTo | known non-endpoint\nrelated locations: $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@. | java/io/File.java:4:16:4:24 | compareTo | MethodDoc | java/io/File.java:4:16:4:24 | compareTo | ClassDoc | file://java.io:1:1:1:1 | java.io | package | file://File:1:1:1:1 | File | type | file://true:1:1:1:1 | true | subtypes | file://compareTo:1:1:1:1 | compareTo | name | file://(File):1:1:1:1 | (File) | signature | file://:1:1:1:1 |  | input | file://Parameter[this]:1:1:1:1 | Parameter[this] | output | file://this:1:1:1:1 | this | parameterName | file://sourceModel:1:1:1:1 | sourceModel | extensibleType |
-| java/io/File.java:4:16:4:24 | compareTo | known non-endpoint\nrelated locations: $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@. | java/io/File.java:4:16:4:24 | compareTo | MethodDoc | java/io/File.java:4:16:4:24 | compareTo | ClassDoc | file://java.io:1:1:1:1 | java.io | package | file://File:1:1:1:1 | File | type | file://true:1:1:1:1 | true | subtypes | file://compareTo:1:1:1:1 | compareTo | name | file://(File):1:1:1:1 | (File) | signature | file://Argument[this]:1:1:1:1 | Argument[this] | input | file://:1:1:1:1 |  | output | file://this:1:1:1:1 | this | parameterName | file://sinkModel:1:1:1:1 | sinkModel | extensibleType |
-| java/io/File.java:5:9:5:21 | pathname | known non-endpoint\nrelated locations: $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@. | java/io/File.java:5:9:5:21 | pathname | MethodDoc | java/io/File.java:5:9:5:21 | pathname | ClassDoc | file://java.io:1:1:1:1 | java.io | package | file://File:1:1:1:1 | File | type | file://true:1:1:1:1 | true | subtypes | file://compareTo:1:1:1:1 | compareTo | name | file://(File):1:1:1:1 | (File) | signature | file://:1:1:1:1 |  | input | file://Parameter[0]:1:1:1:1 | Parameter[0] | output | file://pathname:1:1:1:1 | pathname | parameterName | file://sourceModel:1:1:1:1 | sourceModel | extensibleType |
-| java/io/File.java:5:9:5:21 | pathname | known non-endpoint\nrelated locations: $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@. | java/io/File.java:5:9:5:21 | pathname | MethodDoc | java/io/File.java:5:9:5:21 | pathname | ClassDoc | file://java.io:1:1:1:1 | java.io | package | file://File:1:1:1:1 | File | type | file://true:1:1:1:1 | true | subtypes | file://compareTo:1:1:1:1 | compareTo | name | file://(File):1:1:1:1 | (File) | signature | file://Argument[0]:1:1:1:1 | Argument[0] | input | file://:1:1:1:1 |  | output | file://pathname:1:1:1:1 | pathname | parameterName | file://sinkModel:1:1:1:1 | sinkModel | extensibleType |
+| java/io/File.java:4:16:4:24 | compareTo | known non-sink\nrelated locations: $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@. | java/io/File.java:4:16:4:24 | compareTo | MethodDoc | java/io/File.java:4:16:4:24 | compareTo | ClassDoc | file://java.io:1:1:1:1 | java.io | package | file://File:1:1:1:1 | File | type | file://true:1:1:1:1 | true | subtypes | file://compareTo:1:1:1:1 | compareTo | name | file://(File):1:1:1:1 | (File) | signature | file://:1:1:1:1 |  | input | file://Parameter[this]:1:1:1:1 | Parameter[this] | output | file://this:1:1:1:1 | this | parameterName | file://sourceModel:1:1:1:1 | sourceModel | extensibleType |
+| java/io/File.java:4:16:4:24 | compareTo | known non-sink\nrelated locations: $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@. | java/io/File.java:4:16:4:24 | compareTo | MethodDoc | java/io/File.java:4:16:4:24 | compareTo | ClassDoc | file://java.io:1:1:1:1 | java.io | package | file://File:1:1:1:1 | File | type | file://true:1:1:1:1 | true | subtypes | file://compareTo:1:1:1:1 | compareTo | name | file://(File):1:1:1:1 | (File) | signature | file://Argument[this]:1:1:1:1 | Argument[this] | input | file://:1:1:1:1 |  | output | file://this:1:1:1:1 | this | parameterName | file://sinkModel:1:1:1:1 | sinkModel | extensibleType |
+| java/io/File.java:5:9:5:21 | pathname | known non-sink\nrelated locations: $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@. | java/io/File.java:5:9:5:21 | pathname | MethodDoc | java/io/File.java:5:9:5:21 | pathname | ClassDoc | file://java.io:1:1:1:1 | java.io | package | file://File:1:1:1:1 | File | type | file://true:1:1:1:1 | true | subtypes | file://compareTo:1:1:1:1 | compareTo | name | file://(File):1:1:1:1 | (File) | signature | file://:1:1:1:1 |  | input | file://Parameter[0]:1:1:1:1 | Parameter[0] | output | file://pathname:1:1:1:1 | pathname | parameterName | file://sourceModel:1:1:1:1 | sourceModel | extensibleType |
+| java/io/File.java:5:9:5:21 | pathname | known non-sink\nrelated locations: $@, $@.\nmetadata: $@, $@, $@, $@, $@, $@, $@, $@, $@. | java/io/File.java:5:9:5:21 | pathname | MethodDoc | java/io/File.java:5:9:5:21 | pathname | ClassDoc | file://java.io:1:1:1:1 | java.io | package | file://File:1:1:1:1 | File | type | file://true:1:1:1:1 | true | subtypes | file://compareTo:1:1:1:1 | compareTo | name | file://(File):1:1:1:1 | (File) | signature | file://Argument[0]:1:1:1:1 | Argument[0] | input | file://:1:1:1:1 |  | output | file://pathname:1:1:1:1 | pathname | parameterName | file://sinkModel:1:1:1:1 | sinkModel | extensibleType |