diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgument.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgument.java index ec9da9268..b1a7e6155 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgument.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgument.java @@ -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; @@ -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 SAFE_ARG_OF_METHOD_MATCHER = MethodMatchers.staticMethod() @@ -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) { @@ -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(); } } diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java index 6f01ab0ab..738fbb3e8 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java @@ -588,14 +588,18 @@ public TransferResult> visitSwitchExpressionNode public TransferResult> visitAssignment( AssignmentNode node, TransferInput> 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); @@ -626,8 +630,10 @@ private static boolean hasNonNullConstantValue(LocalVariableNode node) { @Override public TransferResult> visitVariableDeclaration( VariableDeclarationNode node, TransferInput> 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); } diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java index cb92caee3..2c367c884 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java @@ -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() diff --git a/changelog/@unreleased/pr-2160.v2.yml b/changelog/@unreleased/pr-2160.v2.yml new file mode 100644 index 000000000..1184ccc1b --- /dev/null +++ b/changelog/@unreleased/pr-2160.v2.yml @@ -0,0 +1,5 @@ +type: improvement +improvement: + description: Validate annotated variable and field assignment safety + links: + - https://github.com/palantir/gradle-baseline/pull/2160