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.

Sign up for GitHub

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

Jump to bottom

DangerousThrowableMessageSafeArg disallows Throwables #997

Merged
merged 4 commits into from
Oct 30, 2019
Merged

DangerousThrowableMessageSafeArg disallows Throwables #997

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9acd67f
DangerousThrowableMessageSafeArg disallows Throwables
carterkozak Oct 24, 2019
3f1e419
space
carterkozak Oct 24, 2019
90ef7a4
Add generated changelog entries
carterkozak Oct 24, 2019
3923b78
cleaner implementation
carterkozak Oct 24, 2019
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
18 changes: 13 additions & 5 deletions ...rone/src/main/java/com/palantir/baseline/errorprone/DangerousThrowableMessageSafeArg.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import com.google.errorprone.matchers.Description;
import com.google.errorprone.matchers.Matcher;
import com.google.errorprone.matchers.method.MethodMatchers;
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.MethodInvocationTree;
import java.util.List;
Expand All @@ -43,7 +44,8 @@ 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())
Expand All @@ -56,17 +58,23 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
}

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 (ASTHelpers.isCastable(
ASTHelpers.getResultType(safeValueArgument),
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we match against unsafe args too? I can't see any reason to log them at all wrapped as an Arg

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That stretches the DangerousThrowableMessageSafeArg a bit too far, I don't think it's unsafe to do that, just silly.

I think that belongs in a separate check (potentially capable of suggesting a fix to unwrap the throwable and apply it as the last param)

state.getTypeFromString(Throwable.class.getName()),
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;
}
}
13 changes: 13 additions & 0 deletions .../src/test/java/com/palantir/baseline/errorprone/DangerousThrowableMessageSafeArgTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}
7 changes: 7 additions & 0 deletions changelog/@unreleased/pr-997.v2.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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