DangerousThrowableMessageSafeArg disallows Throwables (#997)

DangerousThrowableMessageSafeArg disallows Throwables

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

@@ -24,6 +24,7 @@
 import com.google.errorprone.bugpatterns.BugChecker;
 import com.google.errorprone.matchers.Description;
 import com.google.errorprone.matchers.Matcher;
+import com.google.errorprone.matchers.Matchers;
 import com.google.errorprone.matchers.method.MethodMatchers;
 import com.sun.source.tree.ExpressionTree;
 import com.sun.source.tree.MethodInvocationTree;
@@ -43,30 +44,36 @@ public final class DangerousThrowableMessageSafeArg extends BugChecker
 
     private static final Matcher<ExpressionTree> SAFEARG_FACTORY_METHOD = MethodMatchers.staticMethod()
             .onClass("com.palantir.logsafe.SafeArg")
-            .named("of");
+            .named("of")
+            .withParameters(String.class.getName(), Object.class.getName());
 
     private static final Matcher<ExpressionTree> THROWABLE_MESSAGE_METHOD = MethodMatchers.instanceMethod()
             .onDescendantOf(Throwable.class.getName())
             .named("getMessage");
 
+    private static final Matcher<ExpressionTree> THROWABLE_MATCHER = Matchers.isSubtypeOf(Throwable.class);
+
     @Override
     public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
         if (!SAFEARG_FACTORY_METHOD.matches(tree, state)) {
             return Description.NO_MATCH;
         }
 
         List<? extends ExpressionTree> args = tree.getArguments();
-        if (args.size() != 2) {
-            return Description.NO_MATCH;
-        }
-
         ExpressionTree safeValueArgument = args.get(1);
         if (THROWABLE_MESSAGE_METHOD.matches(safeValueArgument, state)) {
             return buildDescription(tree)
                     .setMessage("Do not use throwable messages as SafeArg values. "
                             + "SafeLoggable.getLogMessage is guaranteed to be safe.")
                     .build();
         }
+        if (THROWABLE_MATCHER.matches(safeValueArgument, state)) {
+            return buildDescription(tree)
+                    .setMessage("Do not use throwables as SafeArg values. "
+                            + "Throwables must be logged without an Arg wrapper as the last parameter, otherwise "
+                            + "unsafe data may be leaked from the unsafe message or the unsafe message of a cause.")
+                    .build();
+        }
         return Description.NO_MATCH;
     }
 }

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

@@ -63,4 +63,17 @@ public void safe_safearg_value() {
                 "}").doTest();
     }
 
+    @Test
+    public void unsafe_safearg_throwable() {
+        compilationHelper.addSourceLines(
+                "Bean.java",
+                "import " + SafeIllegalArgumentException.class.getName() + ';',
+                "import " + SafeArg.class.getName() + ';',
+                "class Bean {",
+                "  public SafeArg<?> foo() {",
+                "    // BUG: Diagnostic contains: Do not use throwables as SafeArg values",
+                "    return SafeArg.of(\"foo\", new SafeIllegalArgumentException(\"Foo\"));",
+                "  }",
+                "}").doTest();
+    }
 }

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

@@ -0,0 +1,7 @@
+type: improvement
+improvement:
+  description: |-
+    DangerousThrowableMessageSafeArg disallows Throwables in SafeArg values.
+    Throwables must be logged without an Arg wrapper as the last parameter, otherwise unsafe data may be leaked from the unsafe message or the unsafe message of a cause.
+  links:
+  - https://github.com/palantir/gradle-baseline/pull/997