feat: create new detector for rule MET04-J

MET04-J

CHANGELOG.md

@@ -34,6 +34,7 @@ Currently the versioning policy of this project follows [Semantic Versioning v2.
 - Make TypeQualifierResolver recognize org.apache.avro.reflect.Nullable ([#2066](https://github.com/spotbugs/spotbugs/pull/2066))
 - New detector `FindArgumentAssertions` detecting bug `ASSERTION_OF_ARGUMENTS` in case of validation of arguments of public functions using assertions (See [MET01-J. Never use assertions to validate method arguments](https://wiki.sei.cmu.edu/confluence/display/java/MET01-J.+Never+use+assertions+to+validate+method+arguments))
 - Add new detector `CT_CONSTRUCTOR_THROW` for detecting constructors that throw exceptions.
+- New detector `FindIncreasedAccessibilityOfMethods` for new bug type `IAOM_DO_NOT_INCREASE_METHOD_ACCESSIBILITY`. This detector reports a bug if a class increases the accessibility of overridden or hidden methods. (See [SEI CERT rule MET04-J](https://wiki.sei.cmu.edu/confluence/display/java/MET04-J.+Do+not+increase+the+accessibility+of+overridden+or+hidden+methods))
 
 ### Security
 - Disable access to external entities when processing XML ([#2217](https://github.com/spotbugs/spotbugs/pull/2217))

spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/FindIncreasedAccessibilityOfMethodsTest.java

@@ -0,0 +1,97 @@
+package edu.umd.cs.findbugs.detect;
+
+import edu.umd.cs.findbugs.AbstractIntegrationTest;
+import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcher;
+import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcherBuilder;
+import org.junit.Test;
+
+import static edu.umd.cs.findbugs.test.CountMatcher.containsExactly;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.hasItem;
+
+public class FindIncreasedAccessibilityOfMethodsTest extends AbstractIntegrationTest {
+
+    @Test
+    public void findIAOMBugInClass_IncreasedAccessibilityOfMethods_SubClassFromSamePackage() {
+        performAnalysis(
+                "increasedAccessibilityOfMethods/SuperClassOfSuperClass.class",
+                "increasedAccessibilityOfMethods/SuperClass.class",
+                "increasedAccessibilityOfMethods/SubClassFromSamePackage.class");
+
+        assertNumberOfIAOMBugs(6);
+
+        assertIAOMBug("SubClassFromSamePackage", "protectedMethodToPublic");
+        assertIAOMBug("SubClassFromSamePackage", "packagePrivateMethodToPublic");
+        assertIAOMBug("SubClassFromSamePackage", "packagePrivateMethodToProtected");
+        assertIAOMBug("SubClassFromSamePackage", "superProtectedMethodToPublic");
+        assertIAOMBug("SubClassFromSamePackage", "superPackagePrivateMethodToPublic");
+        assertIAOMBug("SubClassFromSamePackage", "superPackagePrivateMethodToProtected");
+    }
+
+    @Test
+    public void findIAOMBugInClass_IncreasedAccessibilityOfMethods_SubClassFromAnotherPackage() {
+        performAnalysis(
+                "increasedAccessibilityOfMethods/SuperClassOfSuperClass.class",
+                "increasedAccessibilityOfMethods/SuperClass.class",
+                "increasedAccessibilityOfMethods/anotherPackage/SubClassFromAnotherPackage.class");
+
+        assertNumberOfIAOMBugs(1);
+
+        assertIAOMBug("SubClassFromAnotherPackage", "protectedMethodToPublic");
+    }
+
+    @Test
+    public void findNoIAOMBugInClass_IncreasedAccessibilityOfMethods_SubClassFromAnotherPackage() {
+        performAnalysis(
+                "increasedAccessibilityOfMethods/SuperClassOfSuperClass.class",
+                "increasedAccessibilityOfMethods/SuperClass.class",
+                "increasedAccessibilityOfMethods/anotherPackage/CorrectSubClass.class");
+
+        assertNumberOfIAOMBugs(0);
+    }
+
+    @Test
+    public void findNoIAOMBugInClass_IncreasedAccessibilityOfMethods_CloneableImplementation() {
+        performAnalysis(
+                "increasedAccessibilityOfMethods/CloneableImplementation.class");
+
+        assertNumberOfIAOMBugs(0);
+    }
+
+    @Test
+    public void findNoIAOMBugInClass_IncreasedAccessibilityOfMethods_InterfaceImplementation() {
+        performAnalysis(
+                "increasedAccessibilityOfMethods/InterfaceImplementation.class");
+
+        assertNumberOfIAOMBugs(0);
+    }
+
+    @Test
+    public void findNoIAOMBugInClass_IncreasedAccessibilityOfMethods_GenericImplementation() {
+        performAnalysis(
+                "increasedAccessibilityOfMethods/GenericSuperClass.class",
+                "increasedAccessibilityOfMethods/GenericSubClass.class");
+
+        assertNumberOfIAOMBugs(3);
+
+        assertIAOMBug("GenericSubClass", "protectedMethodToPublicWithGenericParameter");
+        assertIAOMBug("GenericSubClass", "packagePrivateMethodToPublicWithGenericParameter");
+        assertIAOMBug("GenericSubClass", "packagePrivateMethodToProtectedWithGenericParameter");
+    }
+
+    private void assertNumberOfIAOMBugs(int numberOfBugs) {
+        BugInstanceMatcher bugTypeMatcher = new BugInstanceMatcherBuilder()
+                .bugType("IAOM_DO_NOT_INCREASE_METHOD_ACCESSIBILITY").build();
+        assertThat(getBugCollection(), containsExactly(numberOfBugs, bugTypeMatcher));
+    }
+
+    private void assertIAOMBug(String className, String methodName) {
+        BugInstanceMatcher bugInstanceMatcher = new BugInstanceMatcherBuilder()
+                .bugType("IAOM_DO_NOT_INCREASE_METHOD_ACCESSIBILITY")
+                .inClass(className)
+                .inMethod(methodName)
+                .build();
+        assertThat(getBugCollection(), hasItem(bugInstanceMatcher));
+    }
+
+}

spotbugs/etc/findbugs.xml

@@ -679,8 +679,11 @@
                     reports="VSC_VULNERABLE_SECURITY_CHECK_METHODS"/>
           <Detector class="edu.umd.cs.findbugs.detect.FindArgumentAssertions" speed="fast"
                     reports="AA_ASSERTION_OF_ARGUMENTS"/>
-    <!-- Bug Categories -->
-           <BugCategory category="NOISE" hidden="true"/>
+          <Detector class="edu.umd.cs.findbugs.detect.FindIncreasedAccessibilityOfMethods" speed="fast"
+                    reports="IAOM_DO_NOT_INCREASE_METHOD_ACCESSIBILITY"/>
+
+          <!-- Bug Categories -->
+          <BugCategory category="NOISE" hidden="true"/>
 
           <!-- Bug Codes -->
           <BugCode abbrev="USELESS_STRING"/>
@@ -1312,4 +1315,5 @@
           <BugPattern abbrev="PA" type="PA_PUBLIC_MUTABLE_OBJECT_ATTRIBUTE" category="BAD_PRACTICE"/>
           <BugPattern abbrev="VSC" type="VSC_VULNERABLE_SECURITY_CHECK_METHODS"
                     category="MALICIOUS_CODE"/>
+          <BugPattern abbrev="IAOM" type="IAOM_DO_NOT_INCREASE_METHOD_ACCESSIBILITY" category="CORRECTNESS" />
 </FindbugsPlugin>

spotbugs/etc/messages.xml

@@ -1811,6 +1811,14 @@ factory pattern to create these objects.</p>
       ]]>
     </Details>
   </Detector>
+  <Detector class="edu.umd.cs.findbugs.detect.FindIncreasedAccessibilityOfMethods">
+    <Details>
+      <![CDATA[
+            <p>Finds classes where the accessibility of the overridden or hidden method was increased.</p>
+      ]]>
+    </Details>
+  </Detector>
+
   <!--
   **********************************************************************
   BugPatterns
@@ -8991,11 +8999,28 @@ Using floating-point variables should not be used as loop counters, as they are
         ]]>
     </Details>
   </BugPattern>
+  <BugPattern type="IAOM_DO_NOT_INCREASE_METHOD_ACCESSIBILITY">
+    <ShortDescription>Accessibility of the overridden or hidden method was increased.</ShortDescription>
+    <LongDescription>Accessibility of the overridden or hidden method {1} was increased from {2} to {3}.</LongDescription>
+    <Details>
+      <![CDATA[<p>
+        Increasing the accessibility of overridden or hidden methods permits a malicious subclass to offer wider
+        access to the restricted method than was originally intended. Consequently, programs must override methods
+        only when necessary and must declare methods final whenever possible to prevent malicious subclassing. When
+        methods cannot be declared final, programs must refrain from increasing the accessibility of overridden methods.
+      </p>
+      <p>
+        See SEI CERT rule <a href="https://wiki.sei.cmu.edu/confluence/display/java/MET04-J.+Do+not+increase+the+accessibility+of+overridden+or+hidden+methods">MET04-J. Do not increase the accessibility of overridden or hidden methods</a>.
+      </p>]]>
+    </Details>
+  </BugPattern>
+
   <!--
   **********************************************************************
    BugCodes
   **********************************************************************
    -->
+
   <BugCode abbrev="FS">Format string problem</BugCode>
   <BugCode abbrev="SKIPPED">Analysis skipped</BugCode>
   <BugCode abbrev="IL">Infinite Loop</BugCode>
@@ -9149,4 +9174,5 @@ Using floating-point variables should not be used as loop counters, as they are
   <BugCode abbrev="PA">Public Attribute</BugCode>
   <BugCode abbrev="VSC">Vulnerable security check performing methods</BugCode>
   <BugCode abbrev="AA">Misuse of assertions for checking arguments of public methods</BugCode>
+  <BugCode abbrev="IAOM">Do not increase the accessibility of overridden or hidden methods</BugCode>
 </MessageCollection>

spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindIncreasedAccessibilityOfMethods.java

@@ -0,0 +1,123 @@
+/*
+ * SpotBugs - Find bugs in Java programs
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+package edu.umd.cs.findbugs.detect;
+
+import edu.umd.cs.findbugs.BugInstance;
+import edu.umd.cs.findbugs.BugReporter;
+import edu.umd.cs.findbugs.Detector;
+import edu.umd.cs.findbugs.ba.AnalysisContext;
+import edu.umd.cs.findbugs.ba.ClassContext;
+import org.apache.bcel.Repository;
+import org.apache.bcel.classfile.JavaClass;
+import org.apache.bcel.classfile.Method;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Objects;
+
+public class FindIncreasedAccessibilityOfMethods implements Detector {
+
+    private final BugReporter bugReporter;
+
+    public FindIncreasedAccessibilityOfMethods(BugReporter bugReporter) {
+        this.bugReporter = bugReporter;
+    }
+
+    @Override
+    public void visitClassContext(ClassContext classContext) {
+        JavaClass[] superClasses;
+        JavaClass directSuperClass;
+        JavaClass subClass = classContext.getJavaClass();
+        try {
+            directSuperClass = subClass.getSuperClass();
+            superClasses = subClass.getSuperClasses();
+        } catch (ClassNotFoundException e) {
+            AnalysisContext.reportMissingClass(e);
+            return;
+        }
+
+        Method[] subClassMethods = subClass.getMethods();
+
+        for (JavaClass javaClass : superClasses) {
+            for (Method superClassMethod : getMethodsExceptConstructors(javaClass)) {
+                if (isAccessibleFromSubClass(superClassMethod, subClass, directSuperClass)) {
+                    for (Method subClassMethod : subClassMethods) {
+                        if (isOverridable(superClassMethod, subClassMethod) && !isCloneMethodFromCloneable(subClass, superClassMethod)) {
+                            if (superClassMethod.isProtected() && subClassMethod.isPublic()) {
+                                bugReporter.reportBug(new BugInstance(this, "IAOM_DO_NOT_INCREASE_METHOD_ACCESSIBILITY", NORMAL_PRIORITY)
+                                        .addClassAndMethod(subClass, subClassMethod)
+                                        .addString("protected")
+                                        .addString("public"));
+                            }
+                            if (isPackagePrivate(superClassMethod)) {
+                                if (subClassMethod.isPublic()) {
+                                    bugReporter.reportBug(new BugInstance(this, "IAOM_DO_NOT_INCREASE_METHOD_ACCESSIBILITY", NORMAL_PRIORITY)
+                                            .addClassAndMethod(subClass, subClassMethod)
+                                            .addString("package private")
+                                            .addString("public"));
+                                } else if (subClassMethod.isProtected()) {
+                                    bugReporter.reportBug(new BugInstance(this, "IAOM_DO_NOT_INCREASE_METHOD_ACCESSIBILITY", NORMAL_PRIORITY)
+                                            .addClassAndMethod(subClass, subClassMethod)
+                                            .addString("package private")
+                                            .addString("protected"));
+                                }
+                            }
+                        }
+                    }
+                }
+            }
+        }
+    }
+
+    @Override
+    public void report() {
+    }
+
+    private boolean isPackagePrivate(Method method) {
+        return !(method.isPublic() || method.isProtected() || method.isPrivate());
+    }
+
+    private boolean isAccessibleFromSubClass(Method method, JavaClass subClass, JavaClass directSuperClass) {
+        return method.isProtected() || (subClass.getPackageName().equals(directSuperClass.getPackageName()) && isPackagePrivate(method));
+    }
+
+    private boolean isOverridable(Method original, Method overrider) {
+        return Objects.equals(original, overrider) && !original.isFinal();
+    }
+
+    private List<Method> getMethodsExceptConstructors(JavaClass javaClass) {
+        ArrayList<Method> methods = new ArrayList<>();
+        for (Method method : javaClass.getMethods()) {
+            if (!method.getName().equals("<init>")) {
+                methods.add(method);
+            }
+        }
+        return methods;
+    }
+
+    private boolean isCloneMethodFromCloneable(JavaClass javaClass, Method method) {
+        try {
+            return javaClass.implementationOf(Repository.lookupClass(Cloneable.class)) && method.getName().equals("clone");
+        } catch (ClassNotFoundException e) {
+            AnalysisContext.reportMissingClass(e);
+            return false;
+        }
+    }
+
+}

spotbugsTestCases/src/java/increasedAccessibilityOfMethods/CloneableImplementation.java

@@ -0,0 +1,14 @@
+package increasedAccessibilityOfMethods;
+
+public class CloneableImplementation implements Cloneable {
+
+    @Override
+    public CloneableImplementation clone() {
+        try {
+            return (CloneableImplementation) super.clone();
+        } catch (CloneNotSupportedException e) {
+            throw new AssertionError();
+        }
+    }
+
+}

spotbugsTestCases/src/java/increasedAccessibilityOfMethods/GenericSubClass.java

@@ -0,0 +1,35 @@
+package increasedAccessibilityOfMethods;
+
+public class GenericSubClass<K> extends GenericSuperClass<K> {
+
+    @Override
+    public String publicMethodWithGenericParameter(K parameter) {
+        return "GenericSubClass.publicMethodWithGenericParameter";
+    }
+
+    @Override
+    public String protectedMethodToPublicWithGenericParameter(K parameter) {
+        return "GenericSubClass.protectedMethodToPublicWithGenericParameter";
+    }
+
+    @Override
+    protected String protectedMethodWithGenericParameter(K parameter) {
+        return "GenericSubClass.protectedMethodWithGenericParameter";
+    }
+
+    @Override
+    public String packagePrivateMethodToPublicWithGenericParameter(K parameter) {
+        return "GenericSubClass.packagePrivateMethodToPublicWithGenericParameter";
+    }
+
+    @Override
+    protected String packagePrivateMethodToProtectedWithGenericParameter(K parameter) {
+        return "GenericSubClass.packagePrivateMethodToProtectedWithGenericParameter";
+    }
+
+    @Override
+    String packagePrivateWithGenericParameter(K parameter) {
+        return "GenericSubClass.packagePrivateWithGenericParameter";
+    }
+
+}

spotbugsTestCases/src/java/increasedAccessibilityOfMethods/GenericSuperClass.java

@@ -0,0 +1,36 @@
+package increasedAccessibilityOfMethods;
+
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
+public class GenericSuperClass<T> {
+
+    public String publicMethodWithGenericParameter(T parameter) {
+        return "GenericSuperClass.publicMethodWithGenericParameter";
+    }
+
+    protected String protectedMethodToPublicWithGenericParameter(T parameter) {
+        return "GenericSuperClass.protectedMethodToPublicWithGenericParameter";
+    }
+
+    protected String protectedMethodWithGenericParameter(T parameter) {
+        return "GenericSuperClass.protectedMethodWithGenericParameter";
+    }
+
+    String packagePrivateMethodToPublicWithGenericParameter(T parameter) {
+        return "GenericSuperClass.packagePrivateMethodToPublicWithGenericParameter";
+    }
+
+    String packagePrivateMethodToProtectedWithGenericParameter(T parameter) {
+        return "GenericSuperClass.packagePrivateMethodToProtectedWithGenericParameter";
+    }
+
+    String packagePrivateWithGenericParameter(T parameter) {
+        return "GenericSuperClass.packagePrivateWithGenericParameter";
+    }
+
+    @SuppressFBWarnings("UPM")
+    private String privateMethodWithGenericParameter(T parameter) {
+        return "GenericSuperClass.privateMethodWithGenericParameter";
+    }
+
+}

spotbugsTestCases/src/java/increasedAccessibilityOfMethods/Interface.java

@@ -0,0 +1,7 @@
+package increasedAccessibilityOfMethods;
+
+public interface Interface {
+
+    String packagePrivateInterfaceMethod();
+
+}