From 7f1967af353162e5ec3263c9a0d8c2cfc84329a6 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Mon, 25 Apr 2022 11:45:10 -0400 Subject: [PATCH 1/2] Streamline Throwable safety handling Throwable are considered unsafe, and getMessage returns the safety of the throwable itself. This allows us to annotate a catch block as `@DoNotLog` in cases that an exception may contain credential information. This change also includes the guava `Throwables.getStackTraceAsString` utility as a safety-passthrough method. --- .../errorprone/SafeLoggingPropagation.java | 6 +- .../errorprone/safety/SafetyAnnotations.java | 8 ++- .../safety/SafetyPropagationTransfer.java | 19 +++--- .../IllegalSafeLoggingArgumentTest.java | 59 +++++++++++++++++++ 4 files changed, 78 insertions(+), 14 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 b99dd11f1..9276dcd6f 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 @@ -25,14 +25,12 @@ import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; import com.google.errorprone.matchers.Matchers; -import com.google.errorprone.util.ASTHelpers; import com.palantir.baseline.errorprone.safety.Safety; import com.palantir.baseline.errorprone.safety.SafetyAnnotations; import com.sun.source.tree.AnnotationTree; import com.sun.source.tree.ClassTree; import com.sun.source.tree.MethodTree; import com.sun.source.tree.Tree; -import com.sun.tools.javac.code.Symbol.ClassSymbol; import javax.lang.model.element.Modifier; @AutoService(BugChecker.class) @@ -45,7 +43,6 @@ + "tooling to work with as much information as possible. This check can be auto-fixed using " + "`./gradlew classes testClasses -PerrorProneApply=SafeLoggingPropagation`") public final class SafeLoggingPropagation extends BugChecker implements BugChecker.ClassTreeMatcher { - private static final Matcher SAFETY_ANNOTATION_MATCHER = Matchers.anyOf( Matchers.isSameType(SafetyAnnotations.SAFE), Matchers.isSameType(SafetyAnnotations.UNSAFE), @@ -58,8 +55,7 @@ public final class SafeLoggingPropagation extends BugChecker implements BugCheck @Override public Description matchClass(ClassTree classTree, VisitorState state) { - ClassSymbol classSymbol = ASTHelpers.getSymbol(classTree); - Safety existingClassSafety = SafetyAnnotations.getSafety(classSymbol, state); + Safety existingClassSafety = SafetyAnnotations.getSafety(classTree, state); Safety safety = SafetyAnnotations.getSafety(classTree.getExtendsClause(), state); for (Tree implemented : classTree.getImplementsClause()) { safety = safety.leastUpperBound(SafetyAnnotations.getSafety(implemented, state)); diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnnotations.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnnotations.java index e0a773a4d..eac6a160d 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnnotations.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnnotations.java @@ -45,6 +45,9 @@ public final class SafetyAnnotations { public static final String UNSAFE = "com.palantir.logsafe.Unsafe"; public static final String DO_NOT_LOG = "com.palantir.logsafe.DoNotLog"; + private static final com.google.errorprone.suppliers.Supplier throwableSupplier = + Suppliers.typeFromClass(Throwable.class); + private static final TypeArgumentHandlers SAFETY_IS_COMBINATION_OF_TYPE_ARGUMENTS = new TypeArgumentHandlers( new TypeArgumentHandler(Iterable.class), new TypeArgumentHandler(Iterator.class), @@ -115,7 +118,10 @@ private static Safety getSafetyInternal(Type type, VisitorState state, Set THROWABLES_STACK_TRACE_AS_STRING = MethodMatchers.staticMethod() + .onClass("com.google.common.base.Throwables") + .named("getStackTraceAsString") + .withParameters(Throwable.class.getName()); + // These methods do not take the receiver (generally a static class) into account, only the inputs. private static final Matcher RETURNS_SAFETY_COMBINATION_OF_ARGS = Matchers.anyOf( STRING_FORMAT, @@ -209,7 +214,8 @@ public final class SafetyPropagationTransfer implements ForwardTransferFunction< IMMUTABLE_COLLECTION_FACTORY, OPTIONAL_FACTORIES, STATIC_STREAM_FACTORIES, - RID_FACTORY); + RID_FACTORY, + THROWABLES_STACK_TRACE_AS_STRING); private static final Matcher OPTIONAL_ACCESSORS = Matchers.anyOf( MethodMatchers.instanceMethod() @@ -271,7 +277,8 @@ public final class SafetyPropagationTransfer implements ForwardTransferFunction< .named("getLocator") .withNoParameters(), OPTIONAL_ACCESSORS, - STREAM_ACCESSORS); + STREAM_ACCESSORS, + THROWABLE_GET_MESSAGE); private static final Matcher RETURNS_SAFETY_OF_FIRST_ARG = Matchers.anyOf( MethodMatchers.staticMethod().onClass(Objects.class.getName()).named("requireNonNull"), @@ -715,7 +722,7 @@ public TransferResult> visitLocalVariable( // Cast a wide net for all throwables (covers catch statements) if (THROWABLE_SUBTYPE.matches(node.getTree(), state)) { - safety = Safety.UNSAFE; + safety = Safety.UNSAFE.leastUpperBound(SafetyAnnotations.getSafety(node.getTree(), state)); } else { // No safety information found, likely a captured reference used within a lambda or anonymous class. safety = getCapturedLocalVariableSafety(node); @@ -1003,11 +1010,7 @@ public TransferResult> visitMethodInvocation( private Safety getKnownMethodSafety( MethodInvocationNode node, TransferInput> input) { - if (THROWABLE_GET_MESSAGE.matches(node.getTree(), state)) { - // Auth failures are sometimes annotated '@DoNotLog', which getMessage should inherit. - return Safety.mergeAssumingUnknownIsSame( - Safety.UNSAFE, getValueOfSubNode(input, node.getTarget().getReceiver())); - } else if (RETURNS_SAFETY_OF_ARGS_AND_RECEIVER.matches(node.getTree(), state)) { + if (RETURNS_SAFETY_OF_ARGS_AND_RECEIVER.matches(node.getTree(), state)) { Safety safety = getValueOfSubNode(input, node.getTarget().getReceiver()); for (Node argument : node.getArguments()) { safety = safety.leastUpperBound(getValueOfSubNode(input, argument)); 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 741c7e49c..186f8ab69 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,65 @@ public void testThrowableMessageInheritsDoNotLogFromThrowable() { .doTest(); } + @Test + public void testThrowableParameterIsUnsafe() { + helper().addSourceLines( + "Test.java", + "import com.palantir.logsafe.*;", + "class Test {", + " void f(IllegalStateException e) {", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE' " + + "but the parameter requires 'SAFE'.", + " fun(e);", + " }", + " private static void fun(@Safe Object obj) {}", + "}") + .doTest(); + } + + @Test + public void testThrowableStackTraceAsStringIsUnsafe() { + helper().addSourceLines( + "Test.java", + "import com.palantir.logsafe.*;", + "import com.google.common.base.Throwables;", + "class Test {", + " void f(IllegalStateException e) {", + " String strVal = Throwables.getStackTraceAsString(e);", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE' " + + "but the parameter requires 'SAFE'.", + " fun(strVal);", + " }", + " private static void fun(@Safe Object obj) {}", + "}") + .doTest(); + } + + @Test + public void testCatchDoNotLog() { + helper().addSourceLines( + "Test.java", + "import com.palantir.logsafe.*;", + "import com.google.common.base.Throwables;", + "class Test {", + " void f() {", + " try {", + " action();", + " } catch (@DoNotLog RuntimeException e) {", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'DO_NOT_LOG' " + + "but the parameter requires 'UNSAFE'.", + " fun(e);", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'DO_NOT_LOG' " + + "but the parameter requires 'UNSAFE'.", + " fun(e.getMessage());", + " }", + " }", + " protected void action() {}", + " private static void fun(@Unsafe Object obj) {}", + "}") + .doTest(); + } + @Test public void testStringFormat() { helper().addSourceLines( From 8fa1283878a2b8462089da2ec6ed533fc0b8c539 Mon Sep 17 00:00:00 2001 From: svc-changelog Date: Mon, 25 Apr 2022 15:56:00 +0000 Subject: [PATCH 2/2] Add generated changelog entries --- changelog/@unreleased/pr-2224.v2.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelog/@unreleased/pr-2224.v2.yml diff --git a/changelog/@unreleased/pr-2224.v2.yml b/changelog/@unreleased/pr-2224.v2.yml new file mode 100644 index 000000000..ad4fed138 --- /dev/null +++ b/changelog/@unreleased/pr-2224.v2.yml @@ -0,0 +1,5 @@ +type: improvement +improvement: + description: Streamline Throwable safety handling + links: + - https://github.com/palantir/gradle-baseline/pull/2224