Streamline Throwable safety handling

baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SafeLoggingPropagation.java

@@ -25,14 +25,12 @@
 import com.google.errorprone.matchers.Description;
 import com.google.errorprone.matchers.Matcher;
 import com.google.errorprone.matchers.Matchers;
-import com.google.errorprone.util.ASTHelpers;
 import com.palantir.baseline.errorprone.safety.Safety;
 import com.palantir.baseline.errorprone.safety.SafetyAnnotations;
 import com.sun.source.tree.AnnotationTree;
 import com.sun.source.tree.ClassTree;
 import com.sun.source.tree.MethodTree;
 import com.sun.source.tree.Tree;
-import com.sun.tools.javac.code.Symbol.ClassSymbol;
 import javax.lang.model.element.Modifier;
 
 @AutoService(BugChecker.class)
@@ -45,7 +43,6 @@
                 + "tooling to work with as much information as possible. This check can be auto-fixed using "
                 + "`./gradlew classes testClasses -PerrorProneApply=SafeLoggingPropagation`")
 public final class SafeLoggingPropagation extends BugChecker implements BugChecker.ClassTreeMatcher {
-
     private static final Matcher<Tree> SAFETY_ANNOTATION_MATCHER = Matchers.anyOf(
             Matchers.isSameType(SafetyAnnotations.SAFE),
             Matchers.isSameType(SafetyAnnotations.UNSAFE),
@@ -58,8 +55,7 @@ public final class SafeLoggingPropagation extends BugChecker implements BugCheck
 
     @Override
     public Description matchClass(ClassTree classTree, VisitorState state) {
-        ClassSymbol classSymbol = ASTHelpers.getSymbol(classTree);
-        Safety existingClassSafety = SafetyAnnotations.getSafety(classSymbol, state);
+        Safety existingClassSafety = SafetyAnnotations.getSafety(classTree, state);
         Safety safety = SafetyAnnotations.getSafety(classTree.getExtendsClause(), state);
         for (Tree implemented : classTree.getImplementsClause()) {
             safety = safety.leastUpperBound(SafetyAnnotations.getSafety(implemented, state));

baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnnotations.java

@@ -45,6 +45,9 @@ public final class SafetyAnnotations {
     public static final String UNSAFE = "com.palantir.logsafe.Unsafe";
     public static final String DO_NOT_LOG = "com.palantir.logsafe.DoNotLog";
 
+    private static final com.google.errorprone.suppliers.Supplier<Type> throwableSupplier =
+            Suppliers.typeFromClass(Throwable.class);
+
     private static final TypeArgumentHandlers SAFETY_IS_COMBINATION_OF_TYPE_ARGUMENTS = new TypeArgumentHandlers(
             new TypeArgumentHandler(Iterable.class),
             new TypeArgumentHandler(Iterator.class),
@@ -115,7 +118,10 @@ private static Safety getSafetyInternal(Type type, VisitorState state, Set<Strin
                 return Safety.SAFE;
             }
         }
-        return SAFETY_IS_COMBINATION_OF_TYPE_ARGUMENTS.getSafety(type, state, dejaVu);
+        Safety typeArgumentCombination = SAFETY_IS_COMBINATION_OF_TYPE_ARGUMENTS.getSafety(type, state, dejaVu);
+        return ASTHelpers.isSubtype(type, throwableSupplier.get(state), state)
+                ? Safety.UNSAFE.leastUpperBound(typeArgumentCombination)
+                : typeArgumentCombination;
     }
 
     private static final class TypeArgumentHandlers {

baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java

@@ -202,14 +202,20 @@ public final class SafetyPropagationTransfer implements ForwardTransferFunction<
             // really matter, however the rest are required to be safe, so they cannot poison results.
             .namedAnyOf("of", "valueOf");
 
+    private static final Matcher<ExpressionTree> THROWABLES_STACK_TRACE_AS_STRING = MethodMatchers.staticMethod()
+            .onClass("com.google.common.base.Throwables")
+            .named("getStackTraceAsString")
+            .withParameters(Throwable.class.getName());
+
     // These methods do not take the receiver (generally a static class) into account, only the inputs.
     private static final Matcher<ExpressionTree> RETURNS_SAFETY_COMBINATION_OF_ARGS = Matchers.anyOf(
             STRING_FORMAT,
             OBJECTS_TO_STRING,
             IMMUTABLE_COLLECTION_FACTORY,
             OPTIONAL_FACTORIES,
             STATIC_STREAM_FACTORIES,
-            RID_FACTORY);
+            RID_FACTORY,
+            THROWABLES_STACK_TRACE_AS_STRING);
 
     private static final Matcher<ExpressionTree> OPTIONAL_ACCESSORS = Matchers.anyOf(
             MethodMatchers.instanceMethod()
@@ -271,7 +277,8 @@ public final class SafetyPropagationTransfer implements ForwardTransferFunction<
                     .named("getLocator")
                     .withNoParameters(),
             OPTIONAL_ACCESSORS,
-            STREAM_ACCESSORS);
+            STREAM_ACCESSORS,
+            THROWABLE_GET_MESSAGE);
 
     private static final Matcher<ExpressionTree> RETURNS_SAFETY_OF_FIRST_ARG = Matchers.anyOf(
             MethodMatchers.staticMethod().onClass(Objects.class.getName()).named("requireNonNull"),
@@ -715,7 +722,7 @@ public TransferResult<Safety, AccessPathStore<Safety>> visitLocalVariable(
 
             // Cast a wide net for all throwables (covers catch statements)
             if (THROWABLE_SUBTYPE.matches(node.getTree(), state)) {
-                safety = Safety.UNSAFE;
+                safety = Safety.UNSAFE.leastUpperBound(SafetyAnnotations.getSafety(node.getTree(), state));
             } else {
                 // No safety information found, likely a captured reference used within a lambda or anonymous class.
                 safety = getCapturedLocalVariableSafety(node);
@@ -1003,11 +1010,7 @@ public TransferResult<Safety, AccessPathStore<Safety>> visitMethodInvocation(
 
     private Safety getKnownMethodSafety(
             MethodInvocationNode node, TransferInput<Safety, AccessPathStore<Safety>> input) {
-        if (THROWABLE_GET_MESSAGE.matches(node.getTree(), state)) {
-            // Auth failures are sometimes annotated '@DoNotLog', which getMessage should inherit.
-            return Safety.mergeAssumingUnknownIsSame(
-                    Safety.UNSAFE, getValueOfSubNode(input, node.getTarget().getReceiver()));
-        } else if (RETURNS_SAFETY_OF_ARGS_AND_RECEIVER.matches(node.getTree(), state)) {
+        if (RETURNS_SAFETY_OF_ARGS_AND_RECEIVER.matches(node.getTree(), state)) {
             Safety safety = getValueOfSubNode(input, node.getTarget().getReceiver());
             for (Node argument : node.getArguments()) {
                 safety = safety.leastUpperBound(getValueOfSubNode(input, argument));

baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java

@@ -818,6 +818,65 @@ public void testThrowableMessageInheritsDoNotLogFromThrowable() {
                 .doTest();
     }
 
+    @Test
+    public void testThrowableParameterIsUnsafe() {
+        helper().addSourceLines(
+                        "Test.java",
+                        "import com.palantir.logsafe.*;",
+                        "class Test {",
+                        "  void f(IllegalStateException e) {",
+                        "    // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE' "
+                                + "but the parameter requires 'SAFE'.",
+                        "    fun(e);",
+                        "  }",
+                        "  private static void fun(@Safe Object obj) {}",
+                        "}")
+                .doTest();
+    }
+
+    @Test
+    public void testThrowableStackTraceAsStringIsUnsafe() {
+        helper().addSourceLines(
+                        "Test.java",
+                        "import com.palantir.logsafe.*;",
+                        "import com.google.common.base.Throwables;",
+                        "class Test {",
+                        "  void f(IllegalStateException e) {",
+                        "    String strVal = Throwables.getStackTraceAsString(e);",
+                        "    // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE' "
+                                + "but the parameter requires 'SAFE'.",
+                        "    fun(strVal);",
+                        "  }",
+                        "  private static void fun(@Safe Object obj) {}",
+                        "}")
+                .doTest();
+    }
+
+    @Test
+    public void testCatchDoNotLog() {
+        helper().addSourceLines(
+                        "Test.java",
+                        "import com.palantir.logsafe.*;",
+                        "import com.google.common.base.Throwables;",
+                        "class Test {",
+                        "  void f() {",
+                        "    try {",
+                        "      action();",
+                        "    } catch (@DoNotLog RuntimeException e) {",
+                        "      // BUG: Diagnostic contains: Dangerous argument value: arg is 'DO_NOT_LOG' "
+                                + "but the parameter requires 'UNSAFE'.",
+                        "      fun(e);",
+                        "      // BUG: Diagnostic contains: Dangerous argument value: arg is 'DO_NOT_LOG' "
+                                + "but the parameter requires 'UNSAFE'.",
+                        "      fun(e.getMessage());",
+                        "    }",
+                        "  }",
+                        "  protected void action() {}",
+                        "  private static void fun(@Unsafe Object obj) {}",
+                        "}")
+                .doTest();
+    }
+
     @Test
     public void testStringFormat() {
         helper().addSourceLines(

changelog/@unreleased/pr-2224.v2.yml

@@ -0,0 +1,5 @@
+type: improvement
+improvement:
+  description: Streamline Throwable safety handling
+  links:
+  - https://github.com/palantir/gradle-baseline/pull/2224