diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/DangerousThrowableMessageSafeArg.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/DangerousThrowableMessageSafeArg.java index 8ba881f97..8ad02eb0f 100755 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/DangerousThrowableMessageSafeArg.java +++ b/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,12 +44,15 @@ public final class DangerousThrowableMessageSafeArg extends BugChecker private static final Matcher 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 THROWABLE_MESSAGE_METHOD = MethodMatchers.instanceMethod() .onDescendantOf(Throwable.class.getName()) .named("getMessage"); + private static final Matcher THROWABLE_MATCHER = Matchers.isSubtypeOf(Throwable.class); + @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { if (!SAFEARG_FACTORY_METHOD.matches(tree, state)) { @@ -56,10 +60,6 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState } List 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) @@ -67,6 +67,13 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState + "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; } } diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/DangerousThrowableMessageSafeArgTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/DangerousThrowableMessageSafeArgTest.java index f70b7b00d..117b3cceb 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/DangerousThrowableMessageSafeArgTest.java +++ b/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(); + } } diff --git a/changelog/@unreleased/pr-997.v2.yml b/changelog/@unreleased/pr-997.v2.yml new file mode 100644 index 000000000..8a05cec32 --- /dev/null +++ b/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