[fix] Prevent null from flagging as authentication information in Pre…

…ventTokenLogging check (#674)
palantir · Jul 2, 2019 · 73c56cb · 73c56cb
1 parent d5229ad
commit 73c56cb
Show file tree

Hide file tree

Showing 3 changed files with 84 additions and 5 deletions.
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
@@ -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/...-error-prone/src/test/java/com/palantir/baseline/errorprone/PreventTokenLoggingTests.java b/...-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
@@ -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