From 344f2990ba72540c3b3d0f6560e60eb0c7e61f35 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Wed, 11 Sep 2019 17:24:04 -0700 Subject: [PATCH 1/2] SafeLoggableExceptionMessageFormat disallows `{}` in safelog exception messages Validates safelog exception creation the same way as safe preconditions --- README.md | 1 + .../SafeLoggingExceptionMessageFormat.java | 84 ++++++++++++++++++ ...afeLoggingExceptionMessageFormatTests.java | 88 +++++++++++++++++++ 3 files changed, 173 insertions(+) create mode 100644 baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SafeLoggingExceptionMessageFormat.java create mode 100644 baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/SafeLoggingExceptionMessageFormatTests.java diff --git a/README.md b/README.md index a84f6e433..265e72519 100644 --- a/README.md +++ b/README.md @@ -165,6 +165,7 @@ Safe Logging can be found at [github.com/palantir/safe-logging](https://github.c - `DangerousStringInternUsage`: Disallow String.intern() invocations in favor of more predictable, scalable alternatives. - `OptionalOrElseThrowThrows`: Optional.orElseThrow argument must return an exception, not throw one. - `LambdaMethodReference`: Lambda should use a method reference. +- `SafeLoggingExceptionMessageFormat`: SafeLoggable exceptions do not interpolate parameters. ## com.palantir.baseline-checkstyle Checkstyle rules can be suppressed on a per-line or per-block basis. (It is good practice to first consider formatting diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SafeLoggingExceptionMessageFormat.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SafeLoggingExceptionMessageFormat.java new file mode 100644 index 000000000..5bd3668dc --- /dev/null +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SafeLoggingExceptionMessageFormat.java @@ -0,0 +1,84 @@ +/* + * (c) Copyright 2019 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.baseline.errorprone; + +import com.google.auto.service.AutoService; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.matchers.Matchers; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.LiteralTree; +import com.sun.source.tree.NewClassTree; +import com.sun.source.tree.Tree; +import java.util.List; + +@AutoService(BugChecker.class) +@BugPattern( + name = "SafeLoggingExceptionMessageFormat", + link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks", + linkType = BugPattern.LinkType.CUSTOM, + severity = BugPattern.SeverityLevel.ERROR, + summary = "SafeLoggable exceptions do not interpolate parameters") +public final class SafeLoggingExceptionMessageFormat extends BugChecker implements BugChecker.NewClassTreeMatcher { + + private static final long serialVersionUID = 1L; + + // github.com/palantir/safe-logging/tree/develop/preconditions/src/main/java/com/palantir/logsafe/exceptions + private static final Matcher STANDARD_SAFE_LOGGABLE_EXCEPTIONS = Matchers.anyOf( + Matchers.isSameType("com.palantir.logsafe.exceptions.SafeIllegalArgumentException"), + Matchers.isSameType("com.palantir.logsafe.exceptions.SafeIllegalStateException"), + Matchers.isSameType("com.palantir.logsafe.exceptions.SafeIoException"), + Matchers.isSameType("com.palantir.logsafe.exceptions.SafeNullPointerException"), + Matchers.isSameType("com.palantir.logsafe.exceptions.SafeRuntimeException")); + + @Override + public Description matchNewClass(NewClassTree tree, VisitorState state) { + if (!STANDARD_SAFE_LOGGABLE_EXCEPTIONS.matches(tree.getIdentifier(), state)) { + return Description.NO_MATCH; + } + + List args = tree.getArguments(); + + if (args.isEmpty()) { + return Description.NO_MATCH; + } + + ExpressionTree messageArg = args.get(0); + if (!messageArg.getKind().equals(Tree.Kind.STRING_LITERAL)) { + return Description.NO_MATCH; + } + + String message; + try { + message = (String) ((LiteralTree) messageArg).getValue(); + } catch (ClassCastException exception) { + return Description.NO_MATCH; + } + + if (message.contains("{}")) { + return buildDescription(tree) + .setMessage("Do not use slf4j-style formatting in logsafe Exceptions. " + + "Logsafe exceptions provide a simple message and key-value pairs of arguments, " + + "no interpolation is performed.") + .build(); + } + return Description.NO_MATCH; + } +} diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/SafeLoggingExceptionMessageFormatTests.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/SafeLoggingExceptionMessageFormatTests.java new file mode 100644 index 000000000..41cb64f95 --- /dev/null +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/SafeLoggingExceptionMessageFormatTests.java @@ -0,0 +1,88 @@ +/* + * (c) Copyright 2019 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.baseline.errorprone; + +import com.google.errorprone.CompilationTestHelper; +import org.junit.Before; +import org.junit.Test; + +public class SafeLoggingExceptionMessageFormatTests { + + private CompilationTestHelper compilationHelper; + + @Before + public void before() { + compilationHelper = CompilationTestHelper.newInstance(SafeLoggingExceptionMessageFormat.class, getClass()); + } + + @Test + public void testAttemptedSlf4jInterpolation() { + compilationHelper.addSourceLines( + "Test.java", + "import com.palantir.logsafe.exceptions.SafeIllegalArgumentException;", + "import com.palantir.logsafe.SafeArg;", + "class Test {", + "// BUG: Diagnostic contains: Do not use slf4j-style formatting in logsafe Exceptions", + "Exception foo = new SafeIllegalArgumentException(\"Foo {}\", SafeArg.of(\"foo\", 1));", + "}").doTest(); + } + + @Test + public void testAttemptedSlf4jInterpolationWithCause() { + compilationHelper.addSourceLines( + "Test.java", + "import com.palantir.logsafe.exceptions.SafeIllegalArgumentException;", + "import com.palantir.logsafe.SafeArg;", + "class Test {", + "// BUG: Diagnostic contains: Do not use slf4j-style formatting in logsafe Exceptions", + "Exception foo = new SafeIllegalArgumentException(\"Foo {}\",", + "new RuntimeException(), SafeArg.of(\"foo\", 1));", + "}").doTest(); + } + + @Test + public void testAttemptedSlf4jInterpolationNoArgs() { + compilationHelper.addSourceLines( + "Test.java", + "import com.palantir.logsafe.exceptions.SafeIllegalArgumentException;", + "class Test {", + "// BUG: Diagnostic contains: Do not use slf4j-style formatting in logsafe Exceptions", + "Exception foo = new SafeIllegalArgumentException(\"Foo {}\");", + "}").doTest(); + } + + @Test + public void testNegativeWithArg() { + compilationHelper.addSourceLines( + "Test.java", + "import com.palantir.logsafe.exceptions.SafeIllegalArgumentException;", + "import com.palantir.logsafe.SafeArg;", + "class Test {", + "Exception foo = new SafeIllegalArgumentException(\"Foo\", SafeArg.of(\"foo\", 1));", + "}").doTest(); + } + + @Test + public void testNegativeNoArgs() { + compilationHelper.addSourceLines( + "Test.java", + "import com.palantir.logsafe.exceptions.SafeIllegalArgumentException;", + "class Test {", + "Exception foo = new SafeIllegalArgumentException();", + "}").doTest(); + } +} From adea92e5736f0d78e7940f45445c35e776d8311f Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Thu, 12 Sep 2019 00:24:04 +0000 Subject: [PATCH 2/2] Add generated changelog entries --- changelog/@unreleased/pr-815.v2.yml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changelog/@unreleased/pr-815.v2.yml diff --git a/changelog/@unreleased/pr-815.v2.yml b/changelog/@unreleased/pr-815.v2.yml new file mode 100644 index 000000000..e036423a8 --- /dev/null +++ b/changelog/@unreleased/pr-815.v2.yml @@ -0,0 +1,6 @@ +type: improvement +improvement: + description: SafeLoggingExceptionMessageFormat disallows `{}` in safelog exception + messages + links: + - https://github.com/palantir/gradle-baseline/pull/815