Skip to content

Commit

Permalink
Validate annotated variable and field assignment safety (#2160)
Browse files Browse the repository at this point in the history 
Validate annotated variable and field assignment safety
  • Loading branch information
@carterkozak
carterkozak authored Apr 1, 2022
1 parent a4f1b77 commit 58521fa
Show file tree
Hide file tree
Showing 4 changed files with 129 additions and 13 deletions.
50 changes: 41 additions & 9 deletions ...rror-prone/src/main/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgument.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@
import com.palantir.baseline.errorprone.safety.Safety;
import com.palantir.baseline.errorprone.safety.SafetyAnalysis;
import com.palantir.baseline.errorprone.safety.SafetyAnnotations;
import com.sun.source.tree.AssignmentTree;
import com.sun.source.tree.CompoundAssignmentTree;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.MethodInvocationTree;
import com.sun.source.tree.MethodTree;
Expand Down Expand Up @@ -56,7 +58,10 @@
severity = BugPattern.SeverityLevel.ERROR,
summary = "safe-logging annotations must agree between args and method parameters")
public final class IllegalSafeLoggingArgument extends BugChecker
implements BugChecker.MethodInvocationTreeMatcher, BugChecker.ReturnTreeMatcher {
implements BugChecker.MethodInvocationTreeMatcher,
BugChecker.ReturnTreeMatcher,
BugChecker.AssignmentTreeMatcher,
BugChecker.CompoundAssignmentTreeMatcher {

private static final String UNSAFE_ARG = "com.palantir.logsafe.UnsafeArg";
private static final Matcher<ExpressionTree> SAFE_ARG_OF_METHOD_MATCHER = MethodMatchers.staticMethod()
Expand Down Expand Up @@ -102,6 +107,17 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
return Description.NO_MATCH;
}

private static SuggestedFix getSuggestedFix(MethodInvocationTree tree, VisitorState state, Safety argumentSafety) {
if (SAFE_ARG_OF_METHOD_MATCHER.matches(tree, state) && Safety.UNSAFE.allowsValueWith(argumentSafety)) {
SuggestedFix.Builder fix = SuggestedFix.builder();
String unsafeQualifiedClassName = SuggestedFixes.qualifyType(state, fix, UNSAFE_ARG);
String replacement = String.format("%s.of", unsafeQualifiedClassName);
return fix.replace(tree.getMethodSelect(), replacement).build();
}

return SuggestedFix.emptyFix();
}

@Override
public Description matchReturn(ReturnTree tree, VisitorState state) {
if (tree.getExpression() == null) {
Expand Down Expand Up @@ -132,14 +148,30 @@ public Description matchReturn(ReturnTree tree, VisitorState state) {
.build();
}

private static SuggestedFix getSuggestedFix(MethodInvocationTree tree, VisitorState state, Safety argumentSafety) {
if (SAFE_ARG_OF_METHOD_MATCHER.matches(tree, state) && Safety.UNSAFE.allowsValueWith(argumentSafety)) {
SuggestedFix.Builder fix = SuggestedFix.builder();
String unsafeQualifiedClassName = SuggestedFixes.qualifyType(state, fix, UNSAFE_ARG);
String replacement = String.format("%s.of", unsafeQualifiedClassName);
return fix.replace(tree.getMethodSelect(), replacement).build();
}
@Override
public Description matchAssignment(AssignmentTree tree, VisitorState state) {
return handleAssignment(tree, tree.getVariable(), tree.getExpression(), state);
}

return SuggestedFix.emptyFix();
@Override
public Description matchCompoundAssignment(CompoundAssignmentTree tree, VisitorState state) {
return handleAssignment(tree, tree.getVariable(), tree.getExpression(), state);
}

private Description handleAssignment(
ExpressionTree assignmentTree, ExpressionTree variable, ExpressionTree expression, VisitorState state) {
Safety variableDeclaredSafety = SafetyAnnotations.getSafety(ASTHelpers.getSymbol(variable), state);
if (variableDeclaredSafety.allowsAll()) {
return Description.NO_MATCH;
}
Safety assignmentValue = SafetyAnalysis.of(state.withPath(new TreePath(state.getPath(), expression)));
if (variableDeclaredSafety.allowsValueWith(assignmentValue)) {
return Description.NO_MATCH;
}
return buildDescription(assignmentTree)
.setMessage(String.format(
"Dangerous assignment: value is '%s' but the variable is annotated '%s'.",
assignmentValue, variableDeclaredSafety))
.build();
}
}
14 changes: 10 additions & 4 deletions ...rone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -588,14 +588,18 @@ public TransferResult<Safety, AccessPathStore<Safety>> visitSwitchExpressionNode
public TransferResult<Safety, AccessPathStore<Safety>> visitAssignment(
AssignmentNode node, TransferInput<Safety, AccessPathStore<Safety>> input) {
ReadableUpdates updates = new ReadableUpdates();
Safety safety = input.getValueOfSubNode(node.getExpression());
Safety expressionSafety = input.getValueOfSubNode(node.getExpression());
Safety targetSymbolSafety = SafetyAnnotations.getSafety(
ASTHelpers.getSymbol(node.getTarget().getTree()), state);
Safety safety = Safety.mergeAssumingUnknownIsSame(expressionSafety, targetSymbolSafety);
Node target = node.getTarget();
if (target instanceof LocalVariableNode) {
updates.trySet(target, safety);
} else if (target instanceof ArrayAccessNode) {
Node arrayNode = ((ArrayAccessNode) target).getArray();
Safety arrayCombinedSafety = input.getValueOfSubNode(arrayNode).leastUpperBound(safety);
updates.trySet(arrayNode, arrayCombinedSafety);
Safety arrayNodeSafety = input.getValueOfSubNode(arrayNode);
safety = arrayNodeSafety == null ? safety : arrayNodeSafety.leastUpperBound(safety);
updates.trySet(arrayNode, safety);
} else if (target instanceof FieldAccessNode) {
FieldAccessNode fieldAccess = (FieldAccessNode) target;
updates.set(fieldAccess, safety);
Expand Down Expand Up @@ -626,8 +630,10 @@ private static boolean hasNonNullConstantValue(LocalVariableNode node) {
@Override
public TransferResult<Safety, AccessPathStore<Safety>> visitVariableDeclaration(
VariableDeclarationNode node, TransferInput<Safety, AccessPathStore<Safety>> input) {
Safety safety =
Safety variableTypeSafety =
SafetyAnnotations.getSafety(ASTHelpers.getSymbol(node.getTree().getType()), state);
Safety variableSafety = SafetyAnnotations.getSafety(ASTHelpers.getSymbol(node.getTree()), state);
Safety safety = Safety.mergeAssumingUnknownIsSame(variableTypeSafety, variableSafety);
return noStoreChanges(safety, input);
}

Expand Down
73 changes: 73 additions & 0 deletions ...-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -818,6 +818,79 @@ public void testImmutableCollectionFactories() {
.doTest();
}

@Test
public void testFieldAnnotation() {
helper().addSourceLines(
"Test.java",
"import com.palantir.logsafe.*;",
"class Test {",
" @Unsafe private static final String SECRET = System.getProperty(\"foo\");",
" void f() {",
" // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE' "
+ "but the parameter requires 'SAFE'.",
" fun(SECRET);",
" }",
" private static void fun(@Safe Object obj) {}",
"}")
.doTest();
}

@Test
public void testLocalAnnotation() {
helper().addSourceLines(
"Test.java",
"import com.palantir.logsafe.*;",
"class Test {",
" void f() {",
" @DoNotLog String localVar = System.getProperty(\"bar\");",
" // BUG: Diagnostic contains: Dangerous argument value: arg is 'DO_NOT_LOG' "
+ "but the parameter requires 'SAFE'.",
" fun(localVar);",
" }",
" private static void fun(@Safe Object obj) {}",
"}")
.doTest();
}

@Test
public void testLocalAnnotationAssignment() {
helper().addSourceLines(
"Test.java",
"import com.palantir.logsafe.*;",
"class Test {",
" void f(@Safe String safeParam, @Unsafe String unsafeParam, @DoNotLog String dnlParam) {",
" @Safe String local = safeParam;",
" // BUG: Diagnostic contains: Dangerous assignment: value is 'UNSAFE' "
+ "but the variable is annotated 'SAFE'.",
" local = unsafeParam;",
" // BUG: Diagnostic contains: Dangerous assignment: value is 'DO_NOT_LOG' "
+ "but the variable is annotated 'SAFE'.",
" local = dnlParam;",
" }",
"}")
.doTest();
}

@Test
public void testFieldAnnotationAssignment() {
helper().addSourceLines(
"Test.java",
"import com.palantir.logsafe.*;",
"class Test {",
" @Safe private static String field;",
" void f(@Safe String safeParam, @Unsafe String unsafeParam, @DoNotLog String dnlParam) {",
" field = safeParam;",
" // BUG: Diagnostic contains: Dangerous assignment: value is 'UNSAFE' "
+ "but the variable is annotated 'SAFE'.",
" field = unsafeParam;",
" // BUG: Diagnostic contains: Dangerous assignment: value is 'DO_NOT_LOG' "
+ "but the variable is annotated 'SAFE'.",
" field = dnlParam;",
" }",
"}")
.doTest();
}

@Test
public void testSafeArgOfUnsafe_recommendsUnsafeArgOf() {
refactoringHelper()
Expand Down
5 changes: 5 additions & 0 deletions changelog/@unreleased/pr-2160.v2.yml
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
type: improvement
improvement:
description: Validate annotated variable and field assignment safety
links:
- https://github.com/palantir/gradle-baseline/pull/2160

0 comments on commit 58521fa

Please sign in to comment.