From a8ad925c568a97e6922b9f58220c2c83ad85dd1e Mon Sep 17 00:00:00 2001 From: Tom Petracca Date: Fri, 25 Mar 2022 16:51:44 -0400 Subject: [PATCH 1/3] suggested fixes for log safety --- .../IllegalSafeLoggingArgument.java | 19 ++++++ .../IllegalSafeLoggingArgumentTest.java | 68 +++++++++++++++++++ .../BaselineErrorProneExtension.java | 1 + 3 files changed, 88 insertions(+) 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 d623c0119..1a03ffec3 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 @@ -20,6 +20,8 @@ import com.google.errorprone.BugPattern; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.fixes.SuggestedFixes; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; import com.google.errorprone.matchers.method.MethodMatchers; @@ -58,6 +60,11 @@ public final class IllegalSafeLoggingArgument extends BugChecker implements BugC private static final String UNSAFE = "com.palantir.logsafe.Unsafe"; private static final String DO_NOT_LOG = "com.palantir.logsafe.DoNotLog"; + private static final String UNSAFE_ARG = "com.palantir.logsafe.UnsafeArg"; + private static final Matcher SAFE_ARG_OF_METHOD_MATCHER = MethodMatchers.staticMethod() + .onClass("com.palantir.logsafe.SafeArg") + .named("of"); + private static final Matcher TO_STRING = MethodMatchers.instanceMethod().anyClass().named("toString").withNoParameters(); @@ -90,6 +97,7 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState .setMessage(String.format( "Dangerous argument value: arg is '%s' but the parameter requires '%s'.", argumentSafety, parameterSafety)) + .addFix(getSuggestedFix(tree, state, argumentSafety)) .build()); } } @@ -97,6 +105,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(); + } + private static Safety getSafety(ExpressionTree tree, VisitorState state) { Tree argument = ASTHelpers.stripParentheses(tree); // Check annotations on the result type 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 a0b63b3aa..914c92f3d 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 @@ -288,7 +288,75 @@ public void testIncomingParameter_toStringUsesObjectSafety() { .doTest(); } + @Test + public void testSafeArgOfUnsafe_recommendsUnsafeArgOf() { + refactoringHelper() + .addInputLines( + "Test.java", + "import com.palantir.logsafe.SafeArg;", + "import com.palantir.logsafe.Unsafe;", + "class Test {", + " @Unsafe static class UnsafeClass {}", + " void f() {", + " SafeArg.of(\"unsafe\", new UnsafeClass());", + " }", + "}") + .addOutputLines( + "Test.java", + "import com.palantir.logsafe.SafeArg;", + "import com.palantir.logsafe.Unsafe;", + "import com.palantir.logsafe.UnsafeArg;", + "class Test {", + " @Unsafe static class UnsafeClass {}", + " void f() {", + " UnsafeArg.of(\"unsafe\", new UnsafeClass());", + " }", + "}") + .doTest(); + } + + @Test + public void testSafeArgOfDoNotLog_noRecommendation() { + refactoringHelper() + .addInputLines( + "Test.java", + "import com.palantir.logsafe.SafeArg;", + "import com.palantir.logsafe.DoNotLog;", + "class Test {", + " @DoNotLog static class DoNotLogClass {}", + " void f() {", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'DO_NOT_LOG'", + " SafeArg.of(\"unsafe\", new DoNotLogClass());", + " }", + "}") + .expectUnchanged() + .doTest(); + } + + @Test + public void testUnsafeArgumentForSafeParameter_noRecommendation() { + refactoringHelper() + .addInputLines( + "Test.java", + "import com.palantir.logsafe.Safe;", + "import com.palantir.logsafe.Unsafe;", + "class Test {", + " @Unsafe static class UnsafeClass {}", + " void f() {", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE'", + " fun(new UnsafeClass());", + " }", + " private static void fun(@Safe Object obj) {}", + "}") + .expectUnchanged() + .doTest(); + } + private CompilationTestHelper helper() { return CompilationTestHelper.newInstance(IllegalSafeLoggingArgument.class, getClass()); } + + private RefactoringValidator refactoringHelper() { + return RefactoringValidator.of(IllegalSafeLoggingArgument.class, getClass()); + } } diff --git a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java index fcf0c1b15..ef1c84869 100644 --- a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java +++ b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java @@ -42,6 +42,7 @@ public class BaselineErrorProneExtension { "ExecutorSubmitRunnableFutureIgnored", "ExtendsErrorOrThrowable", "FinalClass", + "IllegalSafeLoggingArgument", "ImmutablesStyle", "ImplicitPublicBuilderConstructor", "JavaTimeDefaultTimeZone", From 18d48e96fb86459b1ba021e6cf701fadfb661fc9 Mon Sep 17 00:00:00 2001 From: svc-changelog Date: Fri, 25 Mar 2022 21:00:07 +0000 Subject: [PATCH 2/3] Add generated changelog entries --- changelog/@unreleased/pr-2133.v2.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelog/@unreleased/pr-2133.v2.yml diff --git a/changelog/@unreleased/pr-2133.v2.yml b/changelog/@unreleased/pr-2133.v2.yml new file mode 100644 index 000000000..5811dfe16 --- /dev/null +++ b/changelog/@unreleased/pr-2133.v2.yml @@ -0,0 +1,5 @@ +type: improvement +improvement: + description: suggested fixes for IllegalSafeLoggingArgument check + links: + - https://github.com/palantir/gradle-baseline/pull/2133 From 348503dabaa50effb805c7893339097679a75b50 Mon Sep 17 00:00:00 2001 From: Tom Petracca Date: Fri, 25 Mar 2022 17:09:09 -0400 Subject: [PATCH 3/3] remove related future work reference --- .../baseline/errorprone/IllegalSafeLoggingArgument.java | 2 -- 1 file changed, 2 deletions(-) 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 1a03ffec3..177328ca3 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 @@ -40,8 +40,6 @@ * Ensures that safe-logging annotated elements are handled correctly by annotated method parameters. * Potential future work: *
    - *
  • Suggest replacing {@code UnsafeArg.of(name, verifiableSafeValue)} with - * {@code SafeArg.of(name, verifiableSafeValue)}
    • *
  • We could check return statements in methods annotated for * safety to require consistency
    • *
  • Enforce propagation of safety annotations from fields and types to types which encapsulate them.