Created a new detector for rule MET04-J

CHANGELOG.md

@@ -9,6 +9,9 @@ Currently the versioning policy of this project follows [Semantic Versioning v2.
 ### Fixed
 - Fix FP `SING_SINGLETON_GETTER_NOT_SYNCHRONIZED` with eager instances ([#2932]https://github.com/spotbugs/spotbugs/issues/2932)
 
+### Added
+- 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))
+
 ## 4.8.4 - 2024-04-07
 ### Fixed
 - Fix FP in SE_PREVENT_EXT_OBJ_OVERWRITE when the if statement checking for null value, checking multiple variables or the method exiting in the if branch with an exception. ([#2750](https://github.com/spotbugs/spotbugs/issues/2750))

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

@@ -0,0 +1,120 @@
+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.jupiter.api.Test;
+
+import static edu.umd.cs.findbugs.test.CountMatcher.containsExactly;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.hasItem;
+import static org.hamcrest.Matchers.not;
+
+class FindIncreasedAccessibilityOfMethodsTest extends AbstractIntegrationTest {
+    @Test
+    void findIAOMBugInClass_IncreasedAccessibilityOfMethods_SubClassFromSamePackage() {
+        performAnalysis(
+                "increasedAccessibilityOfMethods/SuperClassOfSuperClass.class",
+                "increasedAccessibilityOfMethods/SuperClass.class",
+                "increasedAccessibilityOfMethods/SubClassFromSamePackage.class");
+
+        assertContainsCorrectNumberOfIAOMBugs(6);
+
+        assertContainsIAOMBug("SubClassFromSamePackage", "protectedMethodToPublic");
+        assertContainsIAOMBug("SubClassFromSamePackage", "packagePrivateMethodToPublic");
+        assertContainsIAOMBug("SubClassFromSamePackage", "packagePrivateMethodToProtected");
+        assertContainsIAOMBug("SubClassFromSamePackage", "superProtectedMethodToPublic");
+        assertContainsIAOMBug("SubClassFromSamePackage", "superPackagePrivateMethodToPublic");
+        assertContainsIAOMBug("SubClassFromSamePackage", "superPackagePrivateMethodToProtected");
+    }
+
+    @Test
+    void findIAOMBugInClass_IncreasedAccessibilityOfMethods_SubClassFromAnotherPackage() {
+        performAnalysis(
+                "increasedAccessibilityOfMethods/SuperClassOfSuperClass.class",
+                "increasedAccessibilityOfMethods/SuperClass.class",
+                "increasedAccessibilityOfMethods/anotherPackage/SubClassFromAnotherPackage.class");
+
+        assertContainsCorrectNumberOfIAOMBugs(1);
+
+        assertContainsIAOMBug("SubClassFromAnotherPackage", "protectedMethodToPublic");
+    }
+
+    @Test
+    void findNoIAOMBugInClass_IncreasedAccessibilityOfMethods_CorrectSubClass() {
+        performAnalysis(
+                "increasedAccessibilityOfMethods/SuperClassOfSuperClass.class",
+                "increasedAccessibilityOfMethods/anotherPackage/CorrectSubClass.class");
+
+        assertContainsCorrectNumberOfIAOMBugs(0);
+    }
+
+    @Test
+    void findNoIAOMBugInClass_IncreasedAccessibilityOfMethods_CloneableImplementation() {
+        performAnalysis(
+                "increasedAccessibilityOfMethods/CloneableImplementation.class");
+
+        assertContainsCorrectNumberOfIAOMBugs(0);
+    }
+
+    @Test
+    void findNoIAOMBugInClass_IncreasedAccessibilityOfMethods_InterfaceImplementation() {
+        performAnalysis(
+                "increasedAccessibilityOfMethods/Interface.class",
+                "increasedAccessibilityOfMethods/InterfaceImplementation.class");
+
+        assertContainsCorrectNumberOfIAOMBugs(0);
+    }
+
+    @Test
+    void findNoIAOMBugInClass_IncreasedAccessibilityOfMethods_GenericImplementation() {
+        performAnalysis(
+                "increasedAccessibilityOfMethods/GenericSuperClass.class",
+                "increasedAccessibilityOfMethods/GenericSubClass.class");
+
+        assertContainsCorrectNumberOfIAOMBugs(3);
+
+        assertContainsIAOMBug("GenericSubClass", "protectedMethodToPublicWithGenericParameter");
+        assertContainsIAOMBug("GenericSubClass", "packagePrivateMethodToPublicWithGenericParameter");
+        assertContainsIAOMBug("GenericSubClass", "packagePrivateMethodToProtectedWithGenericParameter");
+    }
+
+    @Test
+    void findNoIAOMBugInClass_IncreasedAccessibilityOfMethods_SubClassWithRepeatedOverride() {
+        performAnalysis(
+                "increasedAccessibilityOfMethods/SuperClassOfSuperClass.class",
+                "increasedAccessibilityOfMethods/SuperClassWithOverride.class",
+                "increasedAccessibilityOfMethods/SubClassWithRepeatedOverride.class");
+
+        assertContainsCorrectNumberOfIAOMBugs(3);
+
+        assertContainsIAOMBug("SuperClassWithOverride", "superProtectedMethodToPublicOverriddenInSuperClass");
+        assertContainsIAOMBug("SuperClassWithOverride", "superPackagePrivateMethodToProtectedOverriddenInSuperClass");
+        assertContainsIAOMBug("SubClassWithRepeatedOverride", "superPackagePrivateMethodToProtectedOverriddenInSuperClass");
+        assertNotContainsIAOMBug("SubClassWithRepeatedOverride", "superProtectedMethodToPublicOverriddenInSuperClass");
+    }
+
+    private void assertContainsCorrectNumberOfIAOMBugs(int numberOfBugs) {
+        BugInstanceMatcher bugTypeMatcher = new BugInstanceMatcherBuilder()
+                .bugType("IAOM_DO_NOT_INCREASE_METHOD_ACCESSIBILITY").build();
+        assertThat(getBugCollection(), containsExactly(numberOfBugs, bugTypeMatcher));
+    }
+
+    private void assertContainsIAOMBug(String className, String methodName) {
+        BugInstanceMatcher bugInstanceMatcher = new BugInstanceMatcherBuilder()
+                .bugType("IAOM_DO_NOT_INCREASE_METHOD_ACCESSIBILITY")
+                .inClass(className)
+                .inMethod(methodName)
+                .build();
+        assertThat(getBugCollection(), hasItem(bugInstanceMatcher));
+    }
+
+    private void assertNotContainsIAOMBug(String className, String methodName) {
+        BugInstanceMatcher bugInstanceMatcher = new BugInstanceMatcherBuilder()
+                .bugType("IAOM_DO_NOT_INCREASE_METHOD_ACCESSIBILITY")
+                .inClass(className)
+                .inMethod(methodName)
+                .build();
+        assertThat(getBugCollection(), not(hasItem(bugInstanceMatcher)));
+    }
+}

spotbugs/etc/findbugs.xml

@@ -686,7 +686,10 @@
                     reports="ENV_USE_PROPERTY_INSTEAD_OF_ENV"/>
           <Detector class="edu.umd.cs.findbugs.detect.MultipleInstantiationsOfSingletons" speed="fast"
                     reports="SING_SINGLETON_IMPLEMENTS_CLONEABLE,SING_SINGLETON_INDIRECTLY_IMPLEMENTS_CLONEABLE,SING_SINGLETON_IMPLEMENTS_CLONE_METHOD,SING_SINGLETON_HAS_NONPRIVATE_CONSTRUCTOR,SING_SINGLETON_IMPLEMENTS_SERIALIZABLE,SING_SINGLETON_GETTER_NOT_SYNCHRONIZED" />
-    <!-- Bug Categories -->
+          <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 -->
@@ -1331,4 +1334,5 @@
           <BugPattern abbrev="SING" type="SING_SINGLETON_IMPLEMENTS_SERIALIZABLE" category="CORRECTNESS" />
           <BugPattern abbrev="SING" type="SING_SINGLETON_GETTER_NOT_SYNCHRONIZED" category="CORRECTNESS" />
           <BugPattern abbrev="ENV" type="ENV_USE_PROPERTY_INSTEAD_OF_ENV" category="BAD_PRACTICE" />
+          <BugPattern abbrev="IAOM" type="IAOM_DO_NOT_INCREASE_METHOD_ACCESSIBILITY" category="CORRECTNESS" />
 </FindbugsPlugin>

spotbugs/etc/messages.xml

@@ -1839,6 +1839,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
@@ -9269,11 +9277,28 @@ Using floating-point variables should not be used as loop counters, as they are
       </p>]]>
     </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>
@@ -9430,4 +9455,5 @@ Using floating-point variables should not be used as loop counters, as they are
   <BugCode abbrev="AA">Misuse of assertions for checking arguments of public methods</BugCode>
   <BugCode abbrev="PI">Do not reuse public identifiers from Java Standard Library</BugCode>
   <BugCode abbrev="ENV">Environment variable is used instead of the corresponding Java property</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,160 @@
+/*
+ * 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.Const;
+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;
+import java.util.stream.Collectors;
+
+public class FindIncreasedAccessibilityOfMethods implements Detector {
+    private static final String BUG_TYPE = "IAOM_DO_NOT_INCREASE_METHOD_ACCESSIBILITY";
+    private static final String PACKAGE_PRIVATE = "package private";
+    private static final String PROTECTED = "protected";
+    private static final String PUBLIC = "public";
+
+    private final BugReporter bugReporter;
+
+    public FindIncreasedAccessibilityOfMethods(BugReporter bugReporter) {
+        this.bugReporter = bugReporter;
+    }
+
+    @Override
+    public void visitClassContext(ClassContext classContext) {
+        JavaClass subClass = classContext.getJavaClass();
+        for (IAOMBug bug : findIAOMBugs(subClass)) {
+            bugReporter.reportBug(new BugInstance(this, BUG_TYPE, LOW_PRIORITY)
+                    .addClassAndMethod(bug.clazz, bug.method)
+                    .addString(bug.oldAccessibility)
+                    .addString(bug.newAccessibility));
+        }
+    }
+
+    @Override
+    public void report() {
+    }
+
+    private List<IAOMBug> findIAOMBugs(JavaClass subClass) {
+        List<IAOMBug> iaomBugs = new ArrayList<>();
+        JavaClass[] superClasses;
+        try {
+            superClasses = subClass.getSuperClasses();
+        } catch (ClassNotFoundException e) {
+            AnalysisContext.reportMissingClass(e);
+            return iaomBugs;
+        }
+
+        List<Method> subClassMethods = getMethodsExceptConstructors(subClass);
+        List<IAOMBug> superClassBugs = new ArrayList<>();
+        for (JavaClass superClass : superClasses) {
+            superClassBugs.addAll(findIAOMBugs(superClass));
+            for (Method superClassMethod : getMethodsExceptConstructors(superClass)) {
+                if (isAccessibleFromSubClass(superClassMethod, subClass, superClass)) {
+                    for (Method subClassMethod : subClassMethods) {
+                        if (isOverridable(superClassMethod, subClassMethod) && !isCloneMethodFromCloneable(subClass, superClassMethod)) {
+                            if (superClassMethod.isProtected() && subClassMethod.isPublic()) {
+                                iaomBugs.add(new IAOMBug(subClass, subClassMethod, PROTECTED, PUBLIC));
+                            }
+                            if (isPackagePrivate(superClassMethod)) {
+                                if (subClassMethod.isPublic()) {
+                                    iaomBugs.add(new IAOMBug(subClass, subClassMethod, PACKAGE_PRIVATE, PUBLIC));
+                                } else if (subClassMethod.isProtected()) {
+                                    iaomBugs.add(new IAOMBug(subClass, subClassMethod, PACKAGE_PRIVATE, PROTECTED));
+                                }
+                            }
+                        }
+                    }
+                }
+            }
+        }
+
+        return iaomBugs.stream().filter(bug -> !containsBug(superClassBugs, bug)).collect(Collectors.toList());
+    }
+
+    private List<Method> getMethodsExceptConstructors(JavaClass javaClass) {
+        ArrayList<Method> methods = new ArrayList<>();
+        for (Method method : javaClass.getMethods()) {
+            if (!Const.CONSTRUCTOR_NAME.equals(method.getName())) {
+                methods.add(method);
+            }
+        }
+        return methods;
+    }
+
+    private boolean isAccessibleFromSubClass(Method superClassMethod, JavaClass subClass, JavaClass superClass) {
+        return superClassMethod.isProtected() ||
+                (subClass.getPackageName().equals(superClass.getPackageName()) && isPackagePrivate(superClassMethod));
+    }
+
+    private boolean isOverridable(Method original, Method overrider) {
+        return Objects.equals(original, overrider) && !original.isFinal();
+    }
+
+    private boolean isPackagePrivate(Method method) {
+        return !(method.isPublic() || method.isProtected() || method.isPrivate());
+    }
+
+    private boolean isCloneMethodFromCloneable(JavaClass javaClass, Method method) {
+        try {
+            return javaClass.implementationOf(Repository.lookupClass(Cloneable.class)) && "clone".equals(method.getName());
+        } catch (ClassNotFoundException e) {
+            AnalysisContext.reportMissingClass(e);
+            return false;
+        }
+    }
+
+    private boolean containsBug(List<IAOMBug> bugs, IAOMBug bug) {
+        for (IAOMBug other : bugs) {
+            if (bug.isSameMethodAndAccessibility(other)) {
+                return true;
+            }
+        }
+        return false;
+    }
+
+    private static class IAOMBug {
+        final JavaClass clazz;
+        final Method method;
+        final String oldAccessibility;
+        final String newAccessibility;
+
+        IAOMBug(JavaClass clazz, Method method, String oldAccessibility, String newAccessibility) {
+            this.clazz = clazz;
+            this.method = method;
+            this.oldAccessibility = oldAccessibility;
+            this.newAccessibility = newAccessibility;
+        }
+
+        boolean isSameMethodAndAccessibility(IAOMBug other) {
+            return Objects.equals(method.getSignature(), other.method.getSignature())
+                    && Objects.equals(oldAccessibility, other.oldAccessibility)
+                    && Objects.equals(newAccessibility, other.newAccessibility);
+        }
+    }
+}

spotbugsTestCases/src/java/increasedAccessibilityOfMethods/CloneableImplementation.java

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