diff --git a/README.md b/README.md index 98c75e404..0219eaf70 100644 --- a/README.md +++ b/README.md @@ -190,6 +190,7 @@ Safe Logging can be found at [github.com/palantir/safe-logging](https://github.c - `ImmutablesStyleCollision`: Prevent unintentionally voiding immutables Style meta-annotations through the introduction of inline style annotations. - `TooManyArguments`: Prefer Interface that take few arguments rather than many. - `PreferStaticLoggers`: Prefer static loggers over instance loggers. +- `LogsafeArgName`: Prevent certain named arguments as being logged as safe. Specify unsafe argument names using `LogsafeArgName:UnsafeArgNames` errorProne flag. ### Programmatic Application diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/LogsafeArgName.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/LogsafeArgName.java new file mode 100644 index 000000000..c2f6dbb92 --- /dev/null +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/LogsafeArgName.java @@ -0,0 +1,90 @@ +/* + * (c) Copyright 2020 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.common.collect.ImmutableSet; +import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.SeverityLevel; +import com.google.errorprone.ErrorProneFlags; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.fixes.SuggestedFixes; +import com.google.errorprone.matchers.CompileTimeConstantExpressionMatcher; +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.MethodInvocationTree; +import com.sun.tools.javac.tree.JCTree; +import java.util.List; +import java.util.Set; + +@AutoService(BugChecker.class) +@BugPattern( + name = "LogsafeArgName", + link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks", + linkType = BugPattern.LinkType.CUSTOM, + providesFix = BugPattern.ProvidesFix.REQUIRES_HUMAN_ATTENTION, + severity = SeverityLevel.ERROR, + summary = "Prevent certain argument names from being logged as safe.") +public final class LogsafeArgName extends BugChecker implements MethodInvocationTreeMatcher { + static final String UNSAFE_ARG_NAMES_FLAG = "LogsafeArgName:UnsafeArgNames"; + + private static final Matcher SAFE_ARG_OF = + Matchers.staticMethod().onClass("com.palantir.logsafe.SafeArg").named("of"); + private final Matcher compileTimeConstExpressionMatcher = + new CompileTimeConstantExpressionMatcher(); + + private final Set unsafeParamNames; + + // Must have default constructor for service loading to work correctly + public LogsafeArgName() { + this.unsafeParamNames = ImmutableSet.of(); + } + + public LogsafeArgName(ErrorProneFlags flags) { + this.unsafeParamNames = + flags.getList(UNSAFE_ARG_NAMES_FLAG).map(ImmutableSet::copyOf).orElseGet(ImmutableSet::of); + } + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + if (unsafeParamNames.isEmpty() || !SAFE_ARG_OF.matches(tree, state)) { + return Description.NO_MATCH; + } + + List args = tree.getArguments(); + ExpressionTree argNameExpression = args.get(0); + if (compileTimeConstExpressionMatcher.matches(argNameExpression, state)) { + String argName = (String) ((JCTree.JCLiteral) argNameExpression).getValue(); + if (unsafeParamNames.stream().anyMatch(unsafeArgName -> unsafeArgName.equalsIgnoreCase(argName))) { + SuggestedFix.Builder builder = SuggestedFix.builder(); + String unsafeArg = SuggestedFixes.qualifyType(state, builder, "com.palantir.logsafe.UnsafeArg"); + return buildDescription(tree) + .setMessage("Arguments with name '" + argName + "' must be marked as unsafe.") + .addFix(builder.replace(tree.getMethodSelect(), unsafeArg + ".of") + .build()) + .build(); + } + } + + return Description.NO_MATCH; + } +} diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/LogsafeArgNameTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/LogsafeArgNameTest.java new file mode 100644 index 000000000..7215c6cbc --- /dev/null +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/LogsafeArgNameTest.java @@ -0,0 +1,110 @@ +/* + * (c) Copyright 2020 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.common.collect.ImmutableList; +import com.google.errorprone.CompilationTestHelper; +import com.google.errorprone.ErrorProneFlags; +import org.junit.jupiter.api.Test; + +public final class LogsafeArgNameTest { + + @Test + public void catches_unsafe_arg_name() { + getCompilationHelper() + .addSourceLines( + "Test.java", + "import com.palantir.logsafe.SafeArg;", + "class Test {", + " void f() {", + " // BUG: Diagnostic contains: must be marked as unsafe", + " SafeArg.of(\"foo\", 1);", + " }", + "", + "}") + .doTest(); + } + + @Test + public void catches_case_insensitive_unsafe_arg_name() { + getCompilationHelper() + .addSourceLines( + "Test.java", + "import com.palantir.logsafe.SafeArg;", + "class Test {", + " void f() {", + " // BUG: Diagnostic contains: must be marked as unsafe", + " SafeArg.of(\"Foo\", 1);", + " }", + "", + "}") + .doTest(); + } + + @Test + public void fixes_unsafe_arg_name() { + getRefactoringHelper() + .addInputLines( + "Test.java", + "import com.palantir.logsafe.SafeArg;", + "class Test {", + " void f() {", + " SafeArg.of(\"Foo\", 1);", + " }", + "", + "}") + .addOutputLines( + "Test.java", + "import com.palantir.logsafe.SafeArg;", + "import com.palantir.logsafe.UnsafeArg;", + "class Test {", + " void f() {", + " UnsafeArg.of(\"Foo\", 1);", + " }", + "", + "}") + .doTest(); + } + + @Test + public void ignores_safe_arg_names() { + getCompilationHelper() + .addSourceLines( + "Test.java", + "import com.palantir.logsafe.SafeArg;", + "class Test {", + " void f() {", + " SafeArg.of(\"baz\", 1);", + " }", + "", + "}") + .doTest(); + } + + private static RefactoringValidator getRefactoringHelper() { + return RefactoringValidator.of( + new LogsafeArgName(ErrorProneFlags.builder() + .putFlag(LogsafeArgName.UNSAFE_ARG_NAMES_FLAG, "foo,bar") + .build()), + LogsafeArgNameTest.class); + } + + private static CompilationTestHelper getCompilationHelper() { + return CompilationTestHelper.newInstance(LogsafeArgName.class, LogsafeArgNameTest.class) + .setArgs(ImmutableList.of("-XepOpt:" + LogsafeArgName.UNSAFE_ARG_NAMES_FLAG + "=foo,bar")); + } +} diff --git a/changelog/@unreleased/pr-1459.v2.yml b/changelog/@unreleased/pr-1459.v2.yml new file mode 100644 index 000000000..fcec64a13 --- /dev/null +++ b/changelog/@unreleased/pr-1459.v2.yml @@ -0,0 +1,6 @@ +type: feature +feature: + description: add LogsafeArgName errorprone rule which allows users to specify a + list of argument names that must always be tagged as unsafe. + links: + - https://github.com/palantir/gradle-baseline/pull/1459 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 98f98020e..816014598 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 @@ -30,14 +30,15 @@ public class BaselineErrorProneExtension { // Baseline checks "BracesRequired", "CatchBlockLogException", - "CollectionStreamForEach", // TODO(ckozak): re-enable pending scala check // "CatchSpecificity", - "ExtendsErrorOrThrowable", + "CollectionStreamForEach", "ExecutorSubmitRunnableFutureIgnored", + "ExtendsErrorOrThrowable", "FinalClass", "LambdaMethodReference", "LoggerEnclosingClass", + "LogsafeArgName", "OptionalFlatMapOfNullable", "OptionalOrElseMethodInvocation", "PreferBuiltInConcurrentKeySet",