From a2d6ad7e2cb6edf1106ccba7360f3825508e05eb Mon Sep 17 00:00:00 2001 From: Liam Miller-Cushon Date: Tue, 1 Aug 2023 16:15:02 -0700 Subject: [PATCH] Add an Error Prone check to discourage APIs that convert a hostname to a single IP address PiperOrigin-RevId: 552955605 --- .../bugpatterns/AddressSelection.java | 110 +++++++++++++++ .../scanner/BuiltInCheckerSuppliers.java | 2 + .../bugpatterns/AddressSelectionTest.java | 126 ++++++++++++++++++ docs/bugpattern/AddressSelection.md | 82 ++++++++++++ 4 files changed, 320 insertions(+) create mode 100644 core/src/main/java/com/google/errorprone/bugpatterns/AddressSelection.java create mode 100644 core/src/test/java/com/google/errorprone/bugpatterns/AddressSelectionTest.java create mode 100644 docs/bugpattern/AddressSelection.md diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/AddressSelection.java b/core/src/main/java/com/google/errorprone/bugpatterns/AddressSelection.java new file mode 100644 index 00000000000..3e2be15e29a --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/AddressSelection.java @@ -0,0 +1,110 @@ +/* + * Copyright 2023 The Error Prone Authors. + * + * 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.google.errorprone.bugpatterns; + +import static com.google.common.collect.Iterables.getOnlyElement; +import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +import static com.google.errorprone.fixes.SuggestedFixes.qualifyType; +import static com.google.errorprone.fixes.SuggestedFixes.renameMethodInvocation; +import static com.google.errorprone.matchers.Description.NO_MATCH; +import static com.google.errorprone.matchers.method.MethodMatchers.constructor; +import static com.google.errorprone.matchers.method.MethodMatchers.staticMethod; +import static com.google.errorprone.util.ASTHelpers.constValue; + +import com.google.common.collect.ImmutableSet; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; +import com.google.errorprone.bugpatterns.BugChecker.NewClassTreeMatcher; +import com.google.errorprone.fixes.SuggestedFix; +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.source.tree.NewClassTree; +import java.util.Objects; +import java.util.function.Supplier; + +/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */ +@BugPattern( + summary = + "Prefer InetAddress.getAllName to APIs that convert a hostname to a single IP address", + severity = WARNING) +public final class AddressSelection extends BugChecker + implements NewClassTreeMatcher, MethodInvocationTreeMatcher { + + private static final Matcher CONSTRUCTORS = + Matchers.anyOf( + constructor().forClass("java.net.Socket").withParameters("java.lang.String", "int"), + constructor() + .forClass("java.net.InetSocketAddress") + .withParameters("java.lang.String", "int")); + private static final Matcher METHODS = + staticMethod() + .onClass("java.net.InetAddress") + .named("getByName") + .withParameters("java.lang.String"); + + @Override + public Description matchNewClass(NewClassTree tree, VisitorState state) { + if (!CONSTRUCTORS.matches(tree, state)) { + return NO_MATCH; + } + ExpressionTree argument = tree.getArguments().get(0); + return handleMatch( + argument, + argument, + () -> { + SuggestedFix.Builder fix = SuggestedFix.builder(); + fix.replace( + argument, qualifyType(state, fix, "java.net.InetAddress") + ".getLoopbackAddress()"); + return fix.build(); + }); + } + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + if (!METHODS.matches(tree, state)) { + return NO_MATCH; + } + ExpressionTree argument = getOnlyElement(tree.getArguments()); + return handleMatch( + argument, + tree, + () -> + SuggestedFix.builder() + .merge(renameMethodInvocation(tree, "getLoopbackAddress", state)) + .delete(argument) + .build()); + } + + private static final ImmutableSet LOOPBACK = ImmutableSet.of("127.0.0.1", "::1"); + + private Description handleMatch( + ExpressionTree argument, ExpressionTree replacement, Supplier fix) { + String value = constValue(argument, String.class); + if (Objects.equals(value, "localhost")) { + return NO_MATCH; + } + Description.Builder description = buildDescription(replacement); + if (LOOPBACK.contains(value)) { + description.addFix(fix.get()); + } + return description.build(); + } +} diff --git a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java index e05e7602f41..5efa078e05b 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -22,6 +22,7 @@ import com.google.common.collect.Streams; import com.google.errorprone.BugCheckerInfo; import com.google.errorprone.bugpatterns.ASTHelpersSuggestions; +import com.google.errorprone.bugpatterns.AddressSelection; import com.google.errorprone.bugpatterns.AlreadyChecked; import com.google.errorprone.bugpatterns.AlwaysThrows; import com.google.errorprone.bugpatterns.AmbiguousMethodReference; @@ -817,6 +818,7 @@ public static ScannerSupplier warningChecks() { getSuppliers( // keep-sorted start ASTHelpersSuggestions.class, + AddressSelection.class, AlmostJavadoc.class, AlreadyChecked.class, AmbiguousMethodReference.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/AddressSelectionTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/AddressSelectionTest.java new file mode 100644 index 00000000000..0d1537570ac --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/AddressSelectionTest.java @@ -0,0 +1,126 @@ +/* + * Copyright 2023 The Error Prone Authors. + * + * 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.google.errorprone.bugpatterns; + +import com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.CompilationTestHelper; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class AddressSelectionTest { + + private final CompilationTestHelper compilationHelper = + CompilationTestHelper.newInstance(AddressSelection.class, getClass()); + + private final BugCheckerRefactoringTestHelper refactoringTestHelper = + BugCheckerRefactoringTestHelper.newInstance(AddressSelection.class, getClass()); + + @Test + public void positive() { + compilationHelper + .addSourceLines( + "Test.java", + "import java.net.InetAddress;", + "import java.net.InetSocketAddress;", + "import java.net.Socket;", + "class Test {", + " void f() throws Exception{", + " // BUG: Diagnostic contains:", + " InetAddress.getByName(\"example.com\");", + " // BUG: Diagnostic contains:", + " new Socket(\"example.com\", 80);", + " // BUG: Diagnostic contains:", + " new InetSocketAddress(\"example.com\", 80);", + " }", + "}") + .doTest(); + } + + @Test + public void negative() throws Exception { + compilationHelper + .addSourceLines( + "Test.java", + "import java.net.InetAddress;", + "import java.net.InetSocketAddress;", + "import java.net.Socket;", + "class Test {", + " void f() throws Exception{", + " new Socket(InetAddress.getLoopbackAddress(), 80);", + " InetAddress.getAllByName(\"example.com\");", + " new InetSocketAddress(InetAddress.getLoopbackAddress(), 80);", + " }", + "}") + .doTest(); + } + + @Test + public void negativeLocalhost() throws Exception { + compilationHelper + .addSourceLines( + "Test.java", + "import java.net.InetAddress;", + "import java.net.InetSocketAddress;", + "import java.net.Socket;", + "class Test {", + " void f() throws Exception{", + " new Socket(\"localhost\", 80);", + " InetAddress.getByName(\"localhost\");", + " new InetSocketAddress(\"localhost\", 80);", + " }", + "}") + .doTest(); + } + + @Test + public void refactor() throws Exception { + refactoringTestHelper + .addInputLines( + "Test.java", + "import java.net.InetAddress;", + "import java.net.InetSocketAddress;", + "import java.net.Socket;", + "class Test {", + " void f() throws Exception{", + " new Socket(\"127.0.0.1\", 80);", + " InetAddress.getByName(\"127.0.0.1\");", + " new InetSocketAddress(\"127.0.0.1\", 80);", + " new Socket(\"::1\", 80);", + " InetAddress.getByName(\"::1\");", + " new InetSocketAddress(\"::1\", 80);", + " }", + "}") + .addOutputLines( + "Test.java", + "import java.net.InetAddress;", + "import java.net.InetSocketAddress;", + "import java.net.Socket;", + "class Test {", + " void f() throws Exception{", + " new Socket(InetAddress.getLoopbackAddress(), 80);", + " InetAddress.getLoopbackAddress();", + " new InetSocketAddress(InetAddress.getLoopbackAddress(), 80);", + " new Socket(InetAddress.getLoopbackAddress(), 80);", + " InetAddress.getLoopbackAddress();", + " new InetSocketAddress(InetAddress.getLoopbackAddress(), 80);", + " }", + "}") + .doTest(); + } +} diff --git a/docs/bugpattern/AddressSelection.md b/docs/bugpattern/AddressSelection.md new file mode 100644 index 00000000000..37c6905496f --- /dev/null +++ b/docs/bugpattern/AddressSelection.md @@ -0,0 +1,82 @@ +Avoid APIs that convert a hostname to a single IP address: + +* [`java.net.Socket(String,int)`](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/net/Socket.html#%3Cinit%3E\(java.lang.String,int,boolean\)) +* [`java.net.InetSocketAddress(String,int)`](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/net/InetSocketAddress.html#%3Cinit%3E\(java.lang.String,int\)) +* [`java.net.InetAddress.html#getByName(String)`](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/net/InetAddress.html#getByName\(java.lang.String\)) + +Depending on the value of the +[`-Djava.net.preferIPv6Addresses=true`](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/net/doc-files/net-properties.html) +system property, those APIs will return an IPv4 or IPv6 address. If a client +only has IPv4 connectivity, it will fail to connect with +`-Djava.net.preferIPv6Addresses=true`. If a client only has IPv6 connectivity, +it will fail to connect with `-Djava.net.preferIPv6Addresses=false`. + +The preferred alternative is for clients to consider all addresses returned by +[`java.net.InetAddress.html#getAllByName(String)`](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/net/InetAddress.html#getAllByName\(java.lang.String\)), +and try to connect to each one until a successful connection is made. + +TIP: To resolve a loopback address, prefer `InetAddress.getLoopbackAddress()` +over hard-coding an IPv4 or IPv6 loopback address with +`InetAddress.getByName("127.0.0.1")` or `InetAddress.getByName("::1")`. + +This is, prefer this: + +```java + Socket doConnect(String hostname, int port) throws IOException { + IOException exception = null; + for (InetAddress address : InetAddress.getAllByName(hostname)) { + try { + return new Socket(address, port); + } catch (IOException e) { + if (exception == null) { + exception = e; + } else { + exception.addSuppressed(e); + } + } + } + throw exception; + } +``` + +```java + Socket doConnect(String hostname, int port) throws IOException { + IOException exception = null; + for (InetAddress address : InetAddress.getAllByName(hostname)) { + try { + Socket s = new Socket(); + s.connect(new InetSocketAddress(address, port)); + return s; + } catch (IOException e) { + if (exception == null) { + exception = e; + } else { + exception.addSuppressed(e); + } + } + } + throw exception; + } +``` + +instead of this: + +```java + Socket doConnect(String hostname, int port) throws IOException { + return new Socket(hostname, port); + } +``` + +```java + void doConnect(String hostname, int port) throws IOException { + Socket s = new Socket(); + s.connect(new InetSocketAddress(hostname, port)); + } +``` + +```java + void doConnect(String hostname, int port) throws IOException { + Socket s = new Socket(); + s.connect(new InetSocketAddress(InetAddress.getByName(hostname), port)); + } +```