diff --git a/README.md b/README.md index 68b8cf07f..518d7b6e3 100644 --- a/README.md +++ b/README.md @@ -172,6 +172,7 @@ Safe Logging can be found at [github.com/palantir/safe-logging](https://github.c - `JUnit5SuiteMisuse`: When migrating from JUnit4 -> JUnit5, classes annotated with `@RunWith(Suite.class)` are dangerous because if they reference any JUnit5 test classes, these tests will silently not run! - `PreferAssertj`: Prefer AssertJ fluent assertions. - `ThrowError`: Prefer throwing a RuntimeException rather than Error. +- `ReverseDnsLookup`: Calling address.getHostName may result in an unexpected DNS lookup. ### Programmatic Application diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ReverseDnsLookup.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ReverseDnsLookup.java new file mode 100644 index 000000000..83f0bff66 --- /dev/null +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ReverseDnsLookup.java @@ -0,0 +1,72 @@ +/* + * (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.fixes.SuggestedFixes; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.matchers.method.MethodMatchers; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MethodInvocationTree; +import java.net.InetAddress; +import java.net.InetSocketAddress; + +@AutoService(BugChecker.class) +@BugPattern( + name = "ReverseDnsLookup", + link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks", + linkType = BugPattern.LinkType.CUSTOM, + severity = BugPattern.SeverityLevel.WARNING, + summary = "Calling address.getHostName may result in a reverse DNS lookup which is a network request, making " + + "the invocation significantly more expensive than expected depending on the environment.\n" + + "This check is intended to be advisory - it's fine to @SuppressWarnings(\"ReverseDnsLookup\") " + + "in certain cases, but is usually not recommended.") +public final class ReverseDnsLookup extends BugChecker implements BugChecker.MethodInvocationTreeMatcher { + + private static final Matcher INET_SOCKET_ADDRESS_MATCHER = MethodMatchers.instanceMethod() + .onDescendantOf(InetSocketAddress.class.getName()) + .named("getHostName"); + + private static final Matcher INET_ADDRESS_MATCHER = MethodMatchers.instanceMethod() + .onDescendantOf(InetAddress.class.getName()) + .namedAnyOf("getHostName", "getCanonicalHostName"); + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + if (INET_SOCKET_ADDRESS_MATCHER.matches(tree, state)) { + return buildDescription(tree) + // Suggested fix exists to provide context when compilation fails, it shouldn't be used + // as a drop in replacement because the unresolved string may not be sufficient in some + // cases, particularly involving auditing. + .addFix(SuggestedFixes.renameMethodInvocation(tree, "getHostString", state)) + .build(); + } + if (INET_ADDRESS_MATCHER.matches(tree, state)) { + return buildDescription(tree) + // Suggested fix exists to provide context when compilation fails, it shouldn't be used + // as a drop in replacement because the unresolved string may not be sufficient in some + // cases, particularly involving auditing. + .addFix(SuggestedFixes.renameMethodInvocation(tree, "getHostAddress", state)) + .build(); + } + return Description.NO_MATCH; + } +} diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ReverseDnsLookupTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ReverseDnsLookupTest.java new file mode 100644 index 000000000..2176a628f --- /dev/null +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ReverseDnsLookupTest.java @@ -0,0 +1,58 @@ +/* + * (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.BugCheckerRefactoringTestHelper; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.parallel.Execution; +import org.junit.jupiter.api.parallel.ExecutionMode; + +@Execution(ExecutionMode.CONCURRENT) +class ReverseDnsLookupTest { + + @Test + void testFix() { + fix() + .addInputLines( + "Test.java", + "import java.net.InetAddress;", + "import java.net.InetSocketAddress;", + "class Test {", + " void f(InetAddress ia, InetSocketAddress isa) {", + " ia.getHostName();", + " ia.getCanonicalHostName();", + " isa.getHostName();", + " }", + "}") + .addOutputLines( + "Test.java", + "import java.net.InetAddress;", + "import java.net.InetSocketAddress;", + "class Test {", + " void f(InetAddress ia, InetSocketAddress isa) {", + " ia.getHostAddress();", + " ia.getHostAddress();", + " isa.getHostString();", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + private BugCheckerRefactoringTestHelper fix() { + return BugCheckerRefactoringTestHelper.newInstance(new ReverseDnsLookup(), getClass()); + } +} diff --git a/changelog/@unreleased/pr-970.v2.yml b/changelog/@unreleased/pr-970.v2.yml new file mode 100644 index 000000000..3952a5ea6 --- /dev/null +++ b/changelog/@unreleased/pr-970.v2.yml @@ -0,0 +1,13 @@ +type: improvement +improvement: + description: |- + Implement error prone ReverseDnsLookup for unexpected reverse dns lookups + + Calling address.getHostName may result in a DNS lookup which is a network request, + making the invocation significantly more expensive than expected depending on the + environment. + This check is intended to be advisory - it's fine to + @SuppressWarnings("ReverseDnsLookup") in certain cases, but is usually not + recommended. + links: + - https://github.com/palantir/gradle-baseline/pull/970