From 73c56cb2b8159ac55721048abec3da9d80e756b2 Mon Sep 17 00:00:00 2001
From: David Dunn <2678226+dsd987@users.noreply.github.com>
Date: Tue, 2 Jul 2019 09:15:56 -0400
Subject: [PATCH] [fix] Prevent null from flagging as authentication
 information in PreventTokenLogging check (#674)

---
 .../errorprone/PreventTokenLogging.java       | 13 ++--
 .../errorprone/PreventTokenLoggingTests.java  | 70 +++++++++++++++++++
 changelog/@unreleased/pr-674.v2.yml           |  6 ++
 3 files changed, 84 insertions(+), 5 deletions(-)
 create mode 100644 changelog/@unreleased/pr-674.v2.yml

diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreventTokenLogging.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreventTokenLogging.java
index 5b26962ec..0804c68dd 100644
--- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreventTokenLogging.java
+++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreventTokenLogging.java
@@ -46,15 +46,18 @@ public final class PreventTokenLogging extends BugChecker implements BugChecker.
                             .onClassAny("com.palantir.logsafe.SafeArg", "com.palantir.logsafe.UnsafeArg")
                             .named("of"));
 
-    private static final Matcher<Tree> AUTH_MATCHER =
-            Matchers.anyOf(
-                    Matchers.isSubtypeOf("com.palantir.tokens.auth.AuthHeader"),
-                    Matchers.isSubtypeOf("com.palantir.tokens.auth.BearerToken"));
+    private static final Matcher<ExpressionTree> AUTH_MATCHER =
+            Matchers.allOf(
+                    Matchers.anyOf(
+                            Matchers.isSubtypeOf("com.palantir.tokens.auth.AuthHeader"),
+                            Matchers.isSubtypeOf("com.palantir.tokens.auth.BearerToken")),
+                    Matchers.not(
+                            Matchers.kindIs(Tree.Kind.NULL_LITERAL)));
 
     @Override
     public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
         if (METHOD_MATCHER.matches(tree, state)) {
-            for (Tree arg : tree.getArguments()) {
+            for (ExpressionTree arg : tree.getArguments()) {
                 if (AUTH_MATCHER.matches(arg, state)) {
                     return buildDescription(arg)
                             .setMessage("Authentication information is not allowed to be logged.")
diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/PreventTokenLoggingTests.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/PreventTokenLoggingTests.java
index 3fdc47894..ec054e270 100644
--- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/PreventTokenLoggingTests.java
+++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/PreventTokenLoggingTests.java
@@ -144,6 +144,16 @@ public void testSlf4jTraceNoArgs() {
         passSlf4j("log.trace(message);");
     }
 
+    @Test
+    public void testSlf4jTraceNullMessageNoArgs() {
+        passSlf4j("log.trace(null);");
+    }
+
+    @Test
+    public void testSlf4jTraceNullArg() {
+        passSlf4j("log.trace(message, arg1, null);");
+    }
+
     @Test
     public void testSlf4jDebug() {
         passSlf4j("log.debug(message, arg1);");
@@ -159,6 +169,16 @@ public void testSlf4jDebugNoArgs() {
         passSlf4j("log.debug(message);");
     }
 
+    @Test
+    public void testSlf4jDebugNullMessageNoArgs() {
+        passSlf4j("log.debug(null);");
+    }
+
+    @Test
+    public void testSlf4jDebugNullArg() {
+        passSlf4j("log.debug(message, arg1, null);");
+    }
+
     @Test
     public void testSlf4jInfo() {
         passSlf4j("log.info(message, arg1);");
@@ -174,6 +194,16 @@ public void testSlf4jInfoNoArgs() {
         passSlf4j("log.info(message);");
     }
 
+    @Test
+    public void testSlf4jInfoNullMessageNoArgs() {
+        passSlf4j("log.info(null);");
+    }
+
+    @Test
+    public void testSlf4jInfoNullArg() {
+        passSlf4j("log.info(message, arg1, null);");
+    }
+
     @Test
     public void testSlf4jWarn() {
         passSlf4j("log.warn(message, arg1);");
@@ -189,6 +219,16 @@ public void testSlf4jWarnNoArgs() {
         passSlf4j("log.warn(message);");
     }
 
+    @Test
+    public void testSlf4jWarnNullMessageNoArgs() {
+        passSlf4j("log.warn(null);");
+    }
+
+    @Test
+    public void testSlf4jWarnNullArg() {
+        passSlf4j("log.warn(message, arg1, null);");
+    }
+
     @Test
     public void testSlf4jError() {
         passSlf4j("log.error(message, arg1);");
@@ -204,6 +244,16 @@ public void testSlf4jErrorNoArgs() {
         passSlf4j("log.error(message);");
     }
 
+    @Test
+    public void testSlf4jErrorNullMessageNoArgs() {
+        passSlf4j("log.error(null);");
+    }
+
+    @Test
+    public void testSlf4jErrorNullArg() {
+        passSlf4j("log.error(message, arg1, null);");
+    }
+
     @Test
     public void testSafeArgAuthHeader() {
         failLogSafe("SafeArg.of(name, authHeader);");
@@ -229,11 +279,31 @@ public void testSafeArg() {
         passLogSafe("SafeArg.of(name, value);");
     }
 
+    @Test
+    public void testSafeArgNullName() {
+        passLogSafe("SafeArg.of(null, value);");
+    }
+
+    @Test
+    public void testSafeArgNullValue() {
+        passLogSafe("SafeArg.of(name, null);");
+    }
+
     @Test
     public void testUnsafeArg() {
         passLogSafe("UnsafeArg.of(name, value);");
     }
 
+    @Test
+    public void testUnsafeArgNullName() {
+        passLogSafe("UnsafeArg.of(null, value);");
+    }
+
+    @Test
+    public void testUnsafeArgNullValue() {
+        passLogSafe("UnsafeArg.of(name, null);");
+    }
+
     private void passSlf4j(String statement) {
         compilationHelper.addSourceLines(
                 "Test.java",
diff --git a/changelog/@unreleased/pr-674.v2.yml b/changelog/@unreleased/pr-674.v2.yml
new file mode 100644
index 000000000..71322c87f
--- /dev/null
+++ b/changelog/@unreleased/pr-674.v2.yml
@@ -0,0 +1,6 @@
+type: fix
+fix:
+  description: The PreventTokenLogging error-prone check will now correctly handle
+    null use in SLF4J and Safe/Unsafe Arg functions.
+  links:
+  - https://github.com/palantir/gradle-baseline/pull/674