Move BanSerializableRead annotations to an internal package

and fix some broken javadoc. PiperOrigin-RevId: 350871257
google · Jan 9, 2021 · 92fd464 · 92fd464
1 parent ea7b968
commit 92fd464
Show file tree

Hide file tree

Showing 7 changed files with 18 additions and 114 deletions.
diff --git a/...ava/com/google/errorprone/annotations/SuppressBanSerializableCompletedSecurityReview.java b/...ava/com/google/errorprone/annotations/SuppressBanSerializableCompletedSecurityReview.java
diff --git a/...src/main/java/com/google/errorprone/annotations/SuppressBanSerializableForLegacyCode.java b/...src/main/java/com/google/errorprone/annotations/SuppressBanSerializableForLegacyCode.java
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/BanSerializableRead.java b/core/src/main/java/com/google/errorprone/bugpatterns/BanSerializableRead.java
@@ -28,34 +28,17 @@
 import com.google.errorprone.BugPattern;
 import com.google.errorprone.BugPattern.SeverityLevel;
 import com.google.errorprone.VisitorState;
-import com.google.errorprone.annotations.SuppressBanSerializableCompletedSecurityReview;
-import com.google.errorprone.annotations.SuppressBanSerializableForLegacyCode;
 import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
-import com.google.errorprone.fixes.SuggestedFix;
-import com.google.errorprone.fixes.SuggestedFixes;
 import com.google.errorprone.matchers.Description;
 import com.google.errorprone.matchers.Matcher;
 import com.sun.source.tree.ExpressionTree;
 import com.sun.source.tree.MethodInvocationTree;
-import java.util.Optional;
 
 /** A {@link BugChecker} that detects use of the unsafe {@link java.io.Serializable} API. */
 @BugPattern(
     name = "BanSerializableRead",
     summary = "Deserializing user input via the `Serializable` API is extremely dangerous",
-    explanation =
-        "The Java `Serializable` API is very powerful, and very dangerous. Any consumption of a"
-            + " serialized object that cannot be explicitly trusted will likely result in a"
-            + " critical remote code execution bug that will give an attacker control of the"
-            + " application."
-            + " Consider using less powerful serialization methods, such as JSON or XML.\n\n"
-            + "If using safer APIs is difficult and content that is processed is not user content,"
-            + " add ise-hardening-reviews as a reviewer for a suppression annotation.",
-    severity = SeverityLevel.ERROR,
-    suppressionAnnotations = {
-      SuppressBanSerializableCompletedSecurityReview.class,
-      SuppressBanSerializableForLegacyCode.class
-    })
+    severity = SeverityLevel.ERROR)
 public final class BanSerializableRead extends BugChecker implements MethodInvocationTreeMatcher {
 
   private static final Matcher<ExpressionTree> EXEMPT =
@@ -111,14 +94,8 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
       return Description.NO_MATCH;
     }
 
-    Optional<SuggestedFix> fix =
-        SuggestedFixes.suggestExemptingAnnotation(
-            SuppressBanSerializableForLegacyCode.class.getName(), state.getPath(), state);
+    Description.Builder description = buildDescription(tree);
 
-    if (!fix.isPresent()) {
-      return describeMatch(tree);
-    }
-
-    return describeMatch(tree, fix.get());
+    return description.build();
   }
 }
diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/BanSerializableReadTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/BanSerializableReadTest.java
@@ -17,7 +17,6 @@
 package com.google.errorprone.bugpatterns;
 
 import com.google.errorprone.BugCheckerRefactoringTestHelper;
-import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode;
 import com.google.errorprone.CompilationTestHelper;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -50,12 +49,4 @@ public void testNegativeCaseUnchanged() {
         .setArgs("-XepCompilingTestOnlyCode")
         .doTest();
   }
-
-  @Test
-  public void testRefactor() {
-    refactoringHelper
-        .addInput("BanSerializableReadPositiveCases.java")
-        .addOutput("BanSerializableReadPositiveCases_expected.java")
-        .doTest(TestMode.TEXT_MATCH);
-  }
 }
diff --git a/...est/java/com/google/errorprone/bugpatterns/testdata/BanSerializableReadNegativeCases.java b/...est/java/com/google/errorprone/bugpatterns/testdata/BanSerializableReadNegativeCases.java
@@ -16,8 +16,6 @@
 
 package com.google.errorprone.bugpatterns.testdata;
 
-import com.google.errorprone.annotations.SuppressBanSerializableCompletedSecurityReview;
-import com.google.errorprone.annotations.SuppressBanSerializableForLegacyCode;
 import com.google.errorprone.bugpatterns.BanSerializableReadTest;
 import java.io.IOException;
 import java.io.ObjectInputStream;
@@ -49,7 +47,7 @@ public static final void noCrimesHere() {
    * @throws IOException
    * @throws ClassNotFoundException
    */
-  @SuppressBanSerializableForLegacyCode
+  @SuppressWarnings("BanSerializableRead")
   public static final void directCall() throws IOException, ClassNotFoundException {
     PipedInputStream in = new PipedInputStream();
     PipedOutputStream out = new PipedOutputStream(in);
@@ -68,7 +66,7 @@ public static final void directCall() throws IOException, ClassNotFoundException
    * @throws IOException
    * @throws ClassNotFoundException
    */
-  @SuppressBanSerializableCompletedSecurityReview
+  @SuppressWarnings("BanSerializableRead")
   public static final void sayHi() throws IOException, ClassNotFoundException {
     PipedInputStream in = new PipedInputStream();
     PipedOutputStream out = new PipedOutputStream(in);
@@ -87,7 +85,7 @@ public static final void sayHi() throws IOException, ClassNotFoundException {
   // These test the more esoteric annotations
 
   // code has gone through a security review
-  @SuppressBanSerializableCompletedSecurityReview
+  @SuppressWarnings("BanSerializableRead")
   public static final void directCall2() throws IOException, ClassNotFoundException {
     PipedInputStream in = new PipedInputStream();
     PipedOutputStream out = new PipedOutputStream(in);
@@ -100,7 +98,7 @@ public static final void directCall2() throws IOException, ClassNotFoundExceptio
   }
 
   // code is well-tested legacy
-  @SuppressBanSerializableForLegacyCode
+  @SuppressWarnings("BanSerializableRead")
   public static final void directCall3() throws IOException, ClassNotFoundException {
     PipedInputStream in = new PipedInputStream();
     PipedOutputStream out = new PipedOutputStream(in);

diff --git a/...com/google/errorprone/bugpatterns/testdata/BanSerializableReadPositiveCases_expected.java b/...com/google/errorprone/bugpatterns/testdata/BanSerializableReadPositiveCases_expected.java
@@ -16,7 +16,6 @@
 
 package com.google.errorprone.bugpatterns.testdata;
 
-import com.google.errorprone.annotations.SuppressBanSerializableForLegacyCode;
 import com.google.errorprone.bugpatterns.BanSerializableReadTest;
 import java.io.IOException;
 import java.io.ObjectInputStream;
@@ -39,7 +38,7 @@ class BanSerializableReadPositiveCases implements Serializable {
    * @throws IOException
    * @throws ClassNotFoundException
    */
-  @SuppressBanSerializableForLegacyCode
+  @SuppressWarnings("BanSerializableRead")
   public static final void sayHi() throws IOException, ClassNotFoundException {
     PipedInputStream in = new PipedInputStream();
     PipedOutputStream out = new PipedOutputStream(in);
@@ -64,7 +63,7 @@ public static final void sayHi() throws IOException, ClassNotFoundException {
    * @throws IOException
    * @throws ClassNotFoundException
    */
-  @SuppressBanSerializableForLegacyCode
+  @SuppressWarnings("BanSerializableRead")
   public static final void directCall() throws IOException, ClassNotFoundException {
     PipedInputStream in = new PipedInputStream();
     PipedOutputStream out = new PipedOutputStream(in);

diff --git a/docs/bugpattern/BanSerializableRead.md b/docs/bugpattern/BanSerializableRead.md
@@ -0,0 +1,9 @@
+The Java `Serializable` API is very powerful, and very dangerous. Any
+consumption of a serialized object that cannot be explicitly trusted will likely
+result in a critical remote code execution bug that will give an attacker
+control of the application. (See
+[Effective Java 3rd Edition §84][ej3e-84])
+
+Consider using less powerful serialization methods, such as JSON or XML.
+
+[ej3e-84]: https://books.google.com/books?id=BIpDDwAAQBAJ