Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix] Prevent null from flagging as authentication information in PreventTokenLogging check #674

Merged
merged 3 commits into from
Jul 2, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's also a Matchers.isNonNull()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I remember about Matchers.isNonNull(), is that it only returns true when the expression is provably non-null via a null analysis to make its determination and could sometimes return false when we need true since I believe we are only concerned with the use of literal null.


@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.")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,16 @@ public void testSlf4jTraceNoArgs() {
passSlf4j("log.trace(message);");
}

@Test
public void testSlf4jTraceNullMessageNoArgs() {
passSlf4j("log.trace(null);");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the use-case for this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there's a use case for this, perhaps it should fail, but I don't think the failure should be caused by PreventTokenLogging.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, no use case that I could think of, just a technically allowable case that I didn't think made sense to be enforced by this check.

}

@Test
public void testSlf4jTraceNullArg() {
passSlf4j("log.trace(message, arg1, null);");
}

@Test
public void testSlf4jDebug() {
passSlf4j("log.debug(message, arg1);");
Expand All @@ -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);");
Expand All @@ -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);");
Expand All @@ -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);");
Expand All @@ -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);");
Expand All @@ -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",
Expand Down
6 changes: 6 additions & 0 deletions changelog/@unreleased/pr-674.v2.yml
Original file line number Diff line number Diff line change
@@ -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