From 72623a8c809645e82d3de804ee90b2a595c47097 Mon Sep 17 00:00:00 2001 From: forozco Date: Tue, 28 Jul 2020 11:39:40 -0400 Subject: [PATCH 1/4] add LogsafeArgName errorprone rule --- README.md | 1 + .../baseline/errorprone/LogsafeArgName.java | 89 ++++++++++++++ .../errorprone/LogsafeArgNameTest.java | 111 ++++++++++++++++++ 3 files changed, 201 insertions(+) create mode 100644 baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/LogsafeArgName.java create mode 100644 baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/LogsafeArgNameTest.java 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..297a8b63e --- /dev/null +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/LogsafeArgName.java @@ -0,0 +1,89 @@ +/* + * (c) Copyright 2017 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.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))) { + return buildDescription(tree) + .setMessage("Arguments with name '" + argName + "' must be marked as unsafe.") + .addFix(SuggestedFix.builder() + .replace(tree.getMethodSelect(), "UnsafeArg.of") + .addImport("com.palantir.logsafe.UnsafeArg") + .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..5b3c951da --- /dev/null +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/LogsafeArgNameTest.java @@ -0,0 +1,111 @@ +/* + * (c) Copyright 2017 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.BeforeEach; +import org.junit.jupiter.api.Test; + +public final class LogsafeArgNameTest { + private CompilationTestHelper compilationHelper; + private RefactoringValidator refactoringHelper; + + @BeforeEach + public void before() { + compilationHelper = CompilationTestHelper.newInstance(LogsafeArgName.class, getClass()) + .setArgs(ImmutableList.of("-XepOpt:" + LogsafeArgName.UNSAFE_ARG_NAMES_FLAG + "=foo,bar")); + refactoringHelper = RefactoringValidator.of( + new LogsafeArgName(ErrorProneFlags.builder() + .putFlag(LogsafeArgName.UNSAFE_ARG_NAMES_FLAG, "foo,bar") + .build()), + getClass()); + } + + @Test + public void catches_unsafe_arg_name() { + compilationHelper + .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() { + compilationHelper + .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() { + refactoringHelper + .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() { + compilationHelper + .addSourceLines( + "Test.java", + "import com.palantir.logsafe.SafeArg;", + "class Test {", + " void f() {", + " SafeArg.of(\"baz\", 1);", + " }", + "", + "}") + .doTest(); + } +} From c2e084345ba5b5301a91f29a0b9f858f01b2981d Mon Sep 17 00:00:00 2001 From: forozco Date: Tue, 28 Jul 2020 15:39:40 +0000 Subject: [PATCH 2/4] Add generated changelog entries --- changelog/@unreleased/pr-1459.v2.yml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changelog/@unreleased/pr-1459.v2.yml 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 From ebad5c86d5a9474a0f81b2d95bc4149d7db8162e Mon Sep 17 00:00:00 2001 From: forozco Date: Tue, 28 Jul 2020 11:51:22 -0400 Subject: [PATCH 3/4] register check --- .../com/palantir/baseline/errorprone/LogsafeArgName.java | 2 +- .../com/palantir/baseline/errorprone/LogsafeArgNameTest.java | 2 +- .../baseline/extensions/BaselineErrorProneExtension.java | 5 +++-- 3 files changed, 5 insertions(+), 4 deletions(-) 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 index 297a8b63e..daeeb4dab 100644 --- 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 @@ -1,5 +1,5 @@ /* - * (c) Copyright 2017 Palantir Technologies Inc. All rights reserved. + * (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. 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 index 5b3c951da..a53ed3550 100644 --- 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 @@ -1,5 +1,5 @@ /* - * (c) Copyright 2017 Palantir Technologies Inc. All rights reserved. + * (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. 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", From 9635426c5c486deaba8f5999a67aa7ce65a551f5 Mon Sep 17 00:00:00 2001 From: forozco Date: Tue, 28 Jul 2020 17:56:56 -0400 Subject: [PATCH 4/4] CR --- .../baseline/errorprone/LogsafeArgName.java | 7 ++-- .../errorprone/LogsafeArgNameTest.java | 35 +++++++++---------- 2 files changed, 21 insertions(+), 21 deletions(-) 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 index daeeb4dab..c2f6dbb92 100644 --- 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 @@ -25,6 +25,7 @@ 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; @@ -74,11 +75,11 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState 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(SuggestedFix.builder() - .replace(tree.getMethodSelect(), "UnsafeArg.of") - .addImport("com.palantir.logsafe.UnsafeArg") + .addFix(builder.replace(tree.getMethodSelect(), unsafeArg + ".of") .build()) .build(); } 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 index a53ed3550..7215c6cbc 100644 --- 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 @@ -19,27 +19,13 @@ import com.google.common.collect.ImmutableList; import com.google.errorprone.CompilationTestHelper; import com.google.errorprone.ErrorProneFlags; -import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; public final class LogsafeArgNameTest { - private CompilationTestHelper compilationHelper; - private RefactoringValidator refactoringHelper; - - @BeforeEach - public void before() { - compilationHelper = CompilationTestHelper.newInstance(LogsafeArgName.class, getClass()) - .setArgs(ImmutableList.of("-XepOpt:" + LogsafeArgName.UNSAFE_ARG_NAMES_FLAG + "=foo,bar")); - refactoringHelper = RefactoringValidator.of( - new LogsafeArgName(ErrorProneFlags.builder() - .putFlag(LogsafeArgName.UNSAFE_ARG_NAMES_FLAG, "foo,bar") - .build()), - getClass()); - } @Test public void catches_unsafe_arg_name() { - compilationHelper + getCompilationHelper() .addSourceLines( "Test.java", "import com.palantir.logsafe.SafeArg;", @@ -55,7 +41,7 @@ public void catches_unsafe_arg_name() { @Test public void catches_case_insensitive_unsafe_arg_name() { - compilationHelper + getCompilationHelper() .addSourceLines( "Test.java", "import com.palantir.logsafe.SafeArg;", @@ -71,7 +57,7 @@ public void catches_case_insensitive_unsafe_arg_name() { @Test public void fixes_unsafe_arg_name() { - refactoringHelper + getRefactoringHelper() .addInputLines( "Test.java", "import com.palantir.logsafe.SafeArg;", @@ -96,7 +82,7 @@ public void fixes_unsafe_arg_name() { @Test public void ignores_safe_arg_names() { - compilationHelper + getCompilationHelper() .addSourceLines( "Test.java", "import com.palantir.logsafe.SafeArg;", @@ -108,4 +94,17 @@ public void ignores_safe_arg_names() { "}") .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")); + } }