Validate annotated variable and field assignment safety

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<ExpressionTree> 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();
     }
 }

baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java

@@ -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);
@@ -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);
     }
 

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()

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