Merge pull request #6246 from Marcono1234/marcono1234/annotation-improvements

Java: Improve and add predicates and classes for annotations

Author: smowton

java/ql/lib/change-notes/2022-04-01-annotation-deprecations.md

@@ -0,0 +1,7 @@
+---
+category: deprecated
+---
+* The predicate `Annotation.getAValue()` has been deprecated because it might lead to obtaining the value of the wrong annotation element by accident. `getValue(string)` (or one of the value type specific predicates) should be used to explicitly specify the name of the annotation element.
+* The predicate `Annotation.getAValue(string)` has been renamed to `getAnArrayValue(string)`.
+* The predicate `SuppressWarningsAnnotation.getASuppressedWarningLiteral()` has been deprecated because it unnecessarily restricts the result type; `getASuppressedWarning()` should be used instead.
+* The predicates `TargetAnnotation.getATargetExpression()` and `RetentionAnnotation.getRetentionPolicyExpression()` have been deprecated because getting the enum constant read expression is rarely useful, instead the corresponding predicates for getting the name of the referenced enum constants should be used.

java/ql/lib/change-notes/2022-04-01-annotation-features.md

@@ -0,0 +1,9 @@
+---
+category: feature
+---
+* The predicates of the CodeQL class `Annotation` have been improved:
+  * Convenience value type specific predicates have been added, such as `getEnumConstantValue(string)` or `getStringValue(string)`.
+  * Convenience predicates for elements with array values have been added, such as `getAnEnumConstantArrayValue(string)`. While the behavior of the existing predicates has not changed, usage of them should be reviewed (or replaced with the newly added predicate) to make sure they work correctly for elements with array values.
+  * Some internal CodeQL usage of the `Annotation` predicates has been adjusted and corrected; this might affect the results of some queries.
+* New predicates have been added to the CodeQL class `Annotatable` to support getting declared and associated annotations. As part of that, `hasAnnotation()` has been changed to also consider inherited annotations, to be consistent with `hasAnnotation(string, string)` and `getAnAnnotation()`. The newly added predicate `hasDeclaredAnnotation()` can be used as replacement for the old functionality.
+* New predicates have been added to the CodeQL class `AnnotationType` to simplify getting information about usage of JDK meta-annotations, such as `@Retention`.

java/ql/lib/semmle/code/java/Annotation.qll

@@ -71,7 +71,8 @@ predicate depends(RefType t, RefType dep) {
       a.getAnnotatedElement().(Member).getDeclaringType() = t
     |
       usesType(a.getType(), dep) or
-      usesType(a.getAValue().getType(), dep)
+      usesType(a.getValue(_).getType(), dep) or
+      usesType(a.getAnArrayValue(_).getType(), dep)
     )
     or
     // the type accessed in an `instanceof` expression in `t`.

java/ql/lib/semmle/code/java/Dependency.qll

@@ -90,7 +90,7 @@ predicate numDepends(RefType t, RefType dep, int value) {
       |
         elem = a and usesType(a.getType(), dep)
         or
-        elem = a.getAValue() and
+        elem = [a.getValue(_), a.getAnArrayValue(_)] and
         elem.getFile().getExtension() = "java" and
         usesType(elem.(Expr).getType(), dep)
       )

java/ql/lib/semmle/code/java/DependencyCounts.qll

@@ -18,76 +18,75 @@ class OverrideAnnotation extends Annotation {
 class SuppressWarningsAnnotation extends Annotation {
   SuppressWarningsAnnotation() { this.getType().hasQualifiedName("java.lang", "SuppressWarnings") }
 
-  /** Gets the `StringLiteral` of a warning suppressed by this annotation. */
-  StringLiteral getASuppressedWarningLiteral() {
-    result = this.getAValue() or
-    result = this.getAValue().(ArrayInit).getAnInit()
-  }
+  /**
+   * DEPRECATED: This predicate restricts the results to `StringLiteral`; prefer `getASuppressedWarning()`
+   * to get the name of a suppressed warning.
+   *
+   * Gets the `StringLiteral` of a warning suppressed by this annotation.
+   */
+  deprecated StringLiteral getASuppressedWarningLiteral() { result = this.getAnArrayValue("value") }
 
   /** Gets the name of a warning suppressed by this annotation. */
-  string getASuppressedWarning() { result = this.getASuppressedWarningLiteral().getValue() }
+  string getASuppressedWarning() { result = this.getAStringArrayValue("value") }
 }
 
 /** A `@Target` annotation. */
 class TargetAnnotation extends Annotation {
   TargetAnnotation() { this.getType().hasQualifiedName("java.lang.annotation", "Target") }
 
   /**
+   * DEPRECATED: Getting the field access expression is rarely useful. Use `getATargetElementType()`
+   * to get the name of the target element.
+   *
    * Gets a target expression within this annotation.
    *
    * For example, the field access `ElementType.FIELD` is a target expression in
    * `@Target({ElementType.FIELD, ElementType.METHOD})`.
    */
-  Expr getATargetExpression() {
-    not result instanceof ArrayInit and
-    (
-      result = this.getAValue() or
-      result = this.getAValue().(ArrayInit).getAnInit()
-    )
-  }
+  deprecated Expr getATargetExpression() { result = this.getAnArrayValue("value") }
 
   /**
    * Gets the name of a target element type.
    *
    * For example, `METHOD` is the name of a target element type in
    * `@Target({ElementType.FIELD, ElementType.METHOD})`.
    */
-  string getATargetElementType() {
-    exists(EnumConstant ec |
-      ec = this.getATargetExpression().(VarAccess).getVariable() and
-      ec.getDeclaringType().hasQualifiedName("java.lang.annotation", "ElementType")
-    |
-      result = ec.getName()
-    )
-  }
+  string getATargetElementType() { result = this.getAnEnumConstantArrayValue("value").getName() }
 }
 
 /** A `@Retention` annotation. */
 class RetentionAnnotation extends Annotation {
   RetentionAnnotation() { this.getType().hasQualifiedName("java.lang.annotation", "Retention") }
 
   /**
+   * DEPRECATED: Getting the field access expression is rarely useful. Use `getRetentionPolicy()`
+   * to get the name of the retention policy.
+   *
    * Gets the retention policy expression within this annotation.
    *
    * For example, the field access `RetentionPolicy.RUNTIME` is the
    * retention policy expression in `@Retention(RetentionPolicy.RUNTIME)`.
    */
-  Expr getRetentionPolicyExpression() { result = this.getValue("value") }
+  deprecated Expr getRetentionPolicyExpression() { result = this.getValue("value") }
 
   /**
    * Gets the name of the retention policy of this annotation.
    *
    * For example, `RUNTIME` is the name of the retention policy
    * in `@Retention(RetentionPolicy.RUNTIME)`.
    */
-  string getRetentionPolicy() {
-    exists(EnumConstant ec |
-      ec = this.getRetentionPolicyExpression().(VarAccess).getVariable() and
-      ec.getDeclaringType().hasQualifiedName("java.lang.annotation", "RetentionPolicy")
-    |
-      result = ec.getName()
-    )
-  }
+  string getRetentionPolicy() { result = this.getEnumConstantValue("value").getName() }
+}
+
+/** A `@Repeatable` annotation. */
+class RepeatableAnnotation extends Annotation {
+  RepeatableAnnotation() { this.getType().hasQualifiedName("java.lang.annotation", "Repeatable") }
+
+  /**
+   * Gets the annotation type which acts as _containing type_, grouping multiple
+   * repeatable annotations together.
+   */
+  AnnotationType getContainingType() { result = this.getTypeValue("value") }
 }
 
 /**
@@ -119,11 +118,7 @@ abstract class NonReflectiveAnnotation extends Annotation { }
 
 library class StandardNonReflectiveAnnotation extends NonReflectiveAnnotation {
   StandardNonReflectiveAnnotation() {
-    exists(AnnotationType anntp | anntp = this.getType() |
-      anntp.hasQualifiedName("java.lang", "Override") or
-      anntp.hasQualifiedName("java.lang", "Deprecated") or
-      anntp.hasQualifiedName("java.lang", "SuppressWarnings") or
-      anntp.hasQualifiedName("java.lang", "SafeVarargs")
-    )
+    this.getType()
+        .hasQualifiedName("java.lang", ["Override", "Deprecated", "SuppressWarnings", "SafeVarargs"])
   }
 }

java/ql/lib/semmle/code/java/JDKAnnotations.qll

@@ -120,7 +120,7 @@ private newtype TPrintAstNode =
     shouldPrint(lvde, _) and lvde.getParent() instanceof SingleLocalVarDeclParent
   } or
   TAnnotationsNode(Annotatable ann) {
-    shouldPrint(ann, _) and ann.hasAnnotation() and not partOfAnnotation(ann)
+    shouldPrint(ann, _) and ann.hasDeclaredAnnotation() and not partOfAnnotation(ann)
   } or
   TParametersNode(Callable c) { shouldPrint(c, _) and not c.hasNoParameters() } or
   TBaseTypesNode(ClassOrInterface ty) { shouldPrint(ty, _) } or

java/ql/lib/semmle/code/java/PrintAst.qll

@@ -161,15 +161,13 @@ class TestNGTestMethod extends Method {
     exists(TestNGTestAnnotation testAnnotation |
       testAnnotation = this.getAnAnnotation() and
       // The data provider must have the same name as the referenced data provider
-      result.getDataProviderName() =
-        testAnnotation.getValue("dataProvider").(StringLiteral).getValue()
+      result.getDataProviderName() = testAnnotation.getStringValue("dataProvider")
     |
       // Either the data provider should be on the current class, or a supertype
       this.getDeclaringType().getAnAncestor() = result.getDeclaringType()
       or
       // Or the data provider class should be declared
-      result.getDeclaringType() =
-        testAnnotation.getValue("dataProviderClass").(TypeLiteral).getReferencedType()
+      result.getDeclaringType() = testAnnotation.getTypeValue("dataProviderClass")
     )
   }
 }
@@ -227,9 +225,7 @@ class TestNGListenersAnnotation extends TestNGAnnotation {
   /**
    * Gets a listener defined in this annotation.
    */
-  TestNGListenerImpl getAListener() {
-    result = this.getAValue("value").(TypeLiteral).getReferencedType()
-  }
+  TestNGListenerImpl getAListener() { result = this.getATypeArrayValue("value") }
 }
 
 /**

java/ql/lib/semmle/code/java/UnitTests.qll

@@ -60,7 +60,7 @@ class JaxbType extends Class {
       this.getAnAnnotation() = a and
       a.getType().(JaxbAnnotationType).hasName("XmlAccessorType")
     |
-      result.getAnAccess() = a.getValue("value")
+      result = a.getEnumConstantValue("value")
     )
   }
 

java/ql/lib/semmle/code/java/frameworks/JAXB.qll

@@ -64,5 +64,5 @@ class RunWithAnnotation extends Annotation {
   /**
    * Gets the runner that will be used.
    */
-  Type getRunner() { result = this.getValue("value").(TypeLiteral).getReferencedType() }
+  Type getRunner() { result = this.getTypeValue("value") }
 }

java/ql/lib/semmle/code/java/frameworks/JUnitAnnotations.qll

@@ -296,11 +296,7 @@ class JaxRSProducesAnnotation extends JaxRSAnnotation {
   /**
    * Gets a declared content type that can be produced by this resource.
    */
-  Expr getADeclaredContentTypeExpr() {
-    result = this.getAValue() and not result instanceof ArrayInit
-    or
-    result = this.getAValue().(ArrayInit).getAnInit()
-  }
+  Expr getADeclaredContentTypeExpr() { result = this.getAnArrayValue("value") }
 }
 
 /** An `@Consumes` annotation that describes content types can be consumed by this resource. */

java/ql/lib/semmle/code/java/frameworks/JaxWS.qll

@@ -85,9 +85,7 @@ class IbatisSqlOperationAnnotation extends Annotation {
   /**
    * Gets this annotation's SQL statement string.
    */
-  string getSqlValue() {
-    result = this.getAValue("value").(CompileTimeConstantExpr).getStringValue()
-  }
+  string getSqlValue() { result = this.getAStringArrayValue("value") }
 }
 
 /**

java/ql/lib/semmle/code/java/frameworks/MyBatis.qll

@@ -33,12 +33,12 @@ class PersistentEntity extends RefType {
   }
 
   /**
-   * Gets the access type for this entity as defined by a `@javax.persistence.Access` annotation, if any.
+   * Gets the access type for this entity as defined by a `@javax.persistence.Access` annotation,
+   * if any, in lower case.
    */
   string getAccessTypeFromAnnotation() {
     exists(AccessAnnotation accessType | accessType = this.getAnAnnotation() |
-      result =
-        accessType.getValue("value").(FieldRead).getField().(EnumConstant).getName().toLowerCase()
+      result = accessType.getEnumConstantValue("value").getName().toLowerCase()
     )
   }
 }

java/ql/lib/semmle/code/java/frameworks/javaee/Persistence.qll

@@ -311,9 +311,7 @@ class SpringQualifierDefinitionAnnotation extends Annotation {
   /**
    * Gets the value of the qualifier field for this qualifier.
    */
-  string getQualifierValue() {
-    result = this.getValue("value").(CompileTimeConstantExpr).getStringValue()
-  }
+  string getQualifierValue() { result = this.getStringValue("value") }
 }
 
 /**
@@ -325,9 +323,7 @@ class SpringQualifierAnnotation extends Annotation {
   /**
    * Gets the value of the qualifier field for this qualifier.
    */
-  string getQualifierValue() {
-    result = this.getValue("value").(CompileTimeConstantExpr).getStringValue()
-  }
+  string getQualifierValue() { result = this.getStringValue("value") }
 
   /**
    * Gets the bean definition in an XML file that this qualifier resolves to, if any.
@@ -350,9 +346,7 @@ class SpringResourceAnnotation extends Annotation {
   /**
    * Gets the specified name value, if any.
    */
-  string getNameValue() {
-    result = this.getValue("name").(CompileTimeConstantExpr).getStringValue()
-  }
+  string getNameValue() { result = this.getStringValue("name") }
 
   /**
    * Gets the bean definition in an XML file that the resource resolves to, if any.

java/ql/lib/semmle/code/java/frameworks/spring/SpringAutowire.qll

@@ -40,16 +40,10 @@ class SpringComponentScan extends Annotation {
    */
   string getBasePackages() {
     // "value" and "basePackages" are synonymous, and are simple strings
-    result = this.getAValue("basePackages").(StringLiteral).getValue()
+    result = this.getAStringArrayValue(["basePackages", "value"])
     or
-    result = this.getAValue("value").(StringLiteral).getValue()
-    or
-    exists(TypeLiteral typeLiteral |
-      // Base package classes are type literals whose package should be considered a base package.
-      typeLiteral = this.getAValue("basePackageClasses")
-    |
-      result = typeLiteral.getReferencedType().(RefType).getPackage().getName()
-    )
+    // Base package classes are type literals whose package should be considered a base package.
+    result = this.getATypeArrayValue("basePackageClasses").(RefType).getPackage().getName()
   }
 }
 
@@ -144,8 +138,7 @@ class SpringComponent extends RefType {
     if exists(this.getComponentAnnotation().getValue("value"))
     then
       // If the name has been specified in the component annotation, use that.
-      result =
-        this.getComponentAnnotation().getValue("value").(CompileTimeConstantExpr).getStringValue()
+      result = this.getComponentAnnotation().getStringValue("value")
     else
       // Otherwise use the name of the class, with the initial letter lower cased.
       exists(string name | name = this.getName() |
@@ -204,7 +197,7 @@ class SpringComponent extends RefType {
           .getType()
           .hasQualifiedName("org.springframework.context.annotation", "Profile")
     |
-      result = profileAnnotation.getAValue("value").(StringLiteral).getValue()
+      result = profileAnnotation.getAStringArrayValue("value")
     )
   }
 }

java/ql/lib/semmle/code/java/frameworks/spring/SpringComponentScan.qll

@@ -154,9 +154,7 @@ class SpringRequestMappingMethod extends SpringControllerMethod {
   }
 
   /** Gets the "value" @RequestMapping annotation value, if present. */
-  string getValue() {
-    result = requestMappingAnnotation.getValue("value").(CompileTimeConstantExpr).getStringValue()
-  }
+  string getValue() { result = requestMappingAnnotation.getStringValue("value") }
 
   /** Holds if this is considered an `@ResponseBody` method. */
   predicate isResponseBody() {

java/ql/lib/semmle/code/java/frameworks/spring/SpringController.qll

@@ -34,5 +34,5 @@ class StrutsActionsAnnotation extends StrutsAnnotation {
   /**
    * Gets an Action annotation contained in this Actions annotation.
    */
-  StrutsActionAnnotation getAnAction() { result = this.getAValue("value") }
+  StrutsActionAnnotation getAnAction() { result = this.getAnArrayValue("value") }
 }

java/ql/lib/semmle/code/java/frameworks/struts/StrutsAnnotations.qll

@@ -23,7 +23,7 @@ class SuppressionAnnotation extends SuppressWarningsAnnotation {
   string text;
 
   SuppressionAnnotation() {
-    text = this.getASuppressedWarningLiteral().getValue() and
+    text = this.getASuppressedWarning() and
     exists(getAnnotationText(text))
   }
 

java/ql/src/AlertSuppressionAnnotations.ql

@@ -19,8 +19,5 @@ where
   m.getNumberOfParameters() = 1 and
   c.getArgument(0).getType() = p and
   p.getATypeArgument() = t and
-  not exists(RetentionAnnotation a |
-    t.getAnAnnotation() = a and
-    a.getAValue().(VarAccess).getVariable().hasName("RUNTIME")
-  )
+  t.getRetentionPolicy() != "RUNTIME"
 select c, "Call to isAnnotationPresent where no annotation has the RUNTIME retention policy."

java/ql/src/Likely Bugs/Reflection/AnnotationPresentCheck.ql

@@ -47,8 +47,8 @@ class SpringControllerRequestMappingGetMethod extends SpringControllerGetMethod
         .getType()
         .hasQualifiedName("org.springframework.web.bind.annotation", "RequestMapping") and
     (
-      this.getAnAnnotation().getValue("method").(VarAccess).getVariable().getName() = "GET" or
-      this.getAnAnnotation().getValue("method").(ArrayInit).getSize() = 0 //Java code example: @RequestMapping(value = "test")
+      this.getAnAnnotation().getAnEnumConstantArrayValue("method").getName() = "GET" or
+      not exists(this.getAnAnnotation().getAnArrayValue("method")) //Java code example: @RequestMapping(value = "test")
     ) and
     not this.getAParamType().getName() = "MultipartFile"
   }

java/ql/src/experimental/Security/CWE/CWE-352/JsonpInjectionLib.qll

@@ -4,6 +4,6 @@ from Field f, Annotation ann, Expr value, Expr valueChild
 where
   f.getDeclaringType().fromSource() and
   ann = f.getAnAnnotation() and
-  value = ann.getAValue() and
+  value = ann.getValue(_) and
   valueChild.getParent() = value
 select f, ann, value, valueChild