From 7f6d03723df3fefe98d7c9c1b1281b15c975bfe3 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Mon, 16 May 2022 10:53:49 -0400 Subject: [PATCH 1/2] Immutables redactions may be unsafe (previously forced do-not-log) We still assume do-not-log by default, however type and annotation information may be used to suggest unsafe. --- .../errorprone/SafeLoggingPropagation.java | 12 ++++++-- .../SafeLoggingPropagationTest.java | 30 +++++++++++++++++++ 2 files changed, 39 insertions(+), 3 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SafeLoggingPropagation.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SafeLoggingPropagation.java index a5fefd304..62f97c0a4 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SafeLoggingPropagation.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SafeLoggingPropagation.java @@ -200,6 +200,7 @@ private Description matchClassOrInterface(ClassTree classTree, ClassSymbol class return matchBasedOnToString(classTree, classSymbol, state); } + @SuppressWarnings("checkstyle:CyclomaticComplexity") private Description matchImmutables(ClassTree classTree, ClassSymbol classSymbol, VisitorState state) { Safety existingClassSafety = SafetyAnnotations.getSafety(classTree, state); Safety safety = getTypeSafetyFromAncestors(classTree, state); @@ -220,9 +221,11 @@ private Description matchImmutables(ClassTree classTree, ClassSymbol classSymbol // The redaction check allows us to add @DoNotLog to redacted fields in the same sweep as // adding class-level safety annotations. Otherwise, we would have to run the automatic // fixes twice. - Safety getterSafety = redacted - ? Safety.DO_NOT_LOG - : SafetyAnnotations.getSafety(methodMember.getReturnType(), state); + Safety getterSafety = SafetyAnnotations.getSafety(methodMember.getReturnType(), state); + if (redacted && (getterSafety == Safety.UNKNOWN || getterSafety == Safety.SAFE)) { + // unsafe data may be redacted, however we assume redaction means do-not-log by default + getterSafety = Safety.DO_NOT_LOG; + } if (getterSafety != Safety.UNKNOWN) { hasKnownGetter = true; } @@ -295,6 +298,7 @@ private Description annotate(Tree tree, ModifiersTree treeModifiers, VisitorStat } @Override + @SuppressWarnings("checkstyle:CyclomaticComplexity") public Description matchMethod(MethodTree method, VisitorState state) { if (METHOD_RETURNS_VOID.matches(method, state) || method.getReturnType() == null) { return Description.NO_MATCH; @@ -307,7 +311,9 @@ public Description matchMethod(MethodTree method, VisitorState state) { Safety methodDeclaredSafety = Safety.mergeAssumingUnknownIsSame( SafetyAnnotations.getSafety(methodSymbol, state), SafetyAnnotations.getSafety(method.getReturnType(), state)); + // Redacted is do-not-log by default, but may be unsafe. if (methodDeclaredSafety != Safety.DO_NOT_LOG + && methodDeclaredSafety != Safety.UNSAFE && ASTHelpers.hasAnnotation(methodSymbol, "org.immutables.value.Value.Redacted", state)) { return handleSafety(method, method.getModifiers(), state, methodDeclaredSafety, Safety.DO_NOT_LOG); } diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/SafeLoggingPropagationTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/SafeLoggingPropagationTest.java index 9bc52e849..1b19688a8 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/SafeLoggingPropagationTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/SafeLoggingPropagationTest.java @@ -566,6 +566,36 @@ void testRedactedIsDoNotLog() { .doTest(); } + @Test + void testRedactedMayBeUnsafe() { + fix().addInputLines( + "Test.java", + "import com.palantir.logsafe.*;", + "import com.fasterxml.jackson.annotation.*;", + "import org.immutables.value.Value;", + "@Value.Immutable", + "interface Test {", + " @Unsafe", + " @JsonValue", + " @Value.Redacted", + " String token();", + "}") + .addOutputLines( + "Test.java", + "import com.palantir.logsafe.*;", + "import com.fasterxml.jackson.annotation.*;", + "import org.immutables.value.Value;", + "@Unsafe", + "@Value.Immutable", + "interface Test {", + " @Unsafe", + " @JsonValue", + " @Value.Redacted", + " String token();", + "}") + .doTest(); + } + @Test void testRedactedIsDoNotLog_jsonSerializable() { fix().addInputLines( From 892257068d474b7ee88b302acf6da26bb2ef8a4b Mon Sep 17 00:00:00 2001 From: svc-changelog Date: Mon, 16 May 2022 14:57:49 +0000 Subject: [PATCH 2/2] Add generated changelog entries --- changelog/@unreleased/pr-2277.v2.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelog/@unreleased/pr-2277.v2.yml diff --git a/changelog/@unreleased/pr-2277.v2.yml b/changelog/@unreleased/pr-2277.v2.yml new file mode 100644 index 000000000..fe1233b76 --- /dev/null +++ b/changelog/@unreleased/pr-2277.v2.yml @@ -0,0 +1,5 @@ +type: improvement +improvement: + description: Immutables redactions may be unsafe (previously forced do-not-log) + links: + - https://github.com/palantir/gradle-baseline/pull/2277