From 4267fa7e48974be40cab30794da5a96603793d15 Mon Sep 17 00:00:00 2001 From: mountford Date: Mon, 23 Oct 2017 14:40:05 -0700 Subject: [PATCH] Return proper RDAP error messages when invalid IP addresses are specified We were relying on Dagger to validate the IP address, but that resulted in 500 errors when the IP address was not valid, which is undesirable. Instead, accept the parameters as strings, then convert them to IP addresses and throw a proper error when conversion fails. Also fixes an improperly specified test. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=173172516 --- .../registry/rdap/RdapDomainSearchAction.java | 11 ++++++++-- java/google/registry/rdap/RdapModule.java | 9 ++++---- .../rdap/RdapNameserverSearchAction.java | 11 ++++++++-- .../registry/request/RequestParameters.java | 22 ------------------- .../rdap/RdapDomainSearchActionTest.java | 8 ++++++- .../rdap/RdapNameserverSearchActionTest.java | 16 +++++++++----- 6 files changed, 39 insertions(+), 38 deletions(-) diff --git a/java/google/registry/rdap/RdapDomainSearchAction.java b/java/google/registry/rdap/RdapDomainSearchAction.java index 394ec3b99..677d8c0fd 100644 --- a/java/google/registry/rdap/RdapDomainSearchAction.java +++ b/java/google/registry/rdap/RdapDomainSearchAction.java @@ -26,6 +26,7 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSortedSet; import com.google.common.collect.Iterables; import com.google.common.collect.Streams; +import com.google.common.net.InetAddresses; import com.google.common.primitives.Booleans; import com.googlecode.objectify.Key; import com.googlecode.objectify.cmd.Query; @@ -79,7 +80,7 @@ public class RdapDomainSearchAction extends RdapActionBase { @Inject Clock clock; @Inject @Parameter("name") Optional nameParam; @Inject @Parameter("nsLdhName") Optional nsLdhNameParam; - @Inject @Parameter("nsIp") Optional nsIpParam; + @Inject @Parameter("nsIp") Optional nsIpParam; @Inject RdapDomainSearchAction() {} @Override @@ -132,7 +133,13 @@ public class RdapDomainSearchAction extends RdapActionBase { RdapSearchPattern.create(nsLdhNameParam.get(), true), now); } else { // syntax: /rdap/domains?nsIp=1.2.3.4 - results = searchByNameserverIp(nsIpParam.get(), now); + InetAddress inetAddress; + try { + inetAddress = InetAddresses.forString(nsIpParam.get()); + } catch (IllegalArgumentException e) { + throw new BadRequestException("Invalid value of nsIp parameter"); + } + results = searchByNameserverIp(inetAddress, now); } if (results.jsonList().isEmpty()) { throw new NotFoundException("No domains found"); diff --git a/java/google/registry/rdap/RdapModule.java b/java/google/registry/rdap/RdapModule.java index 3527f7bf6..5e8e0acf3 100644 --- a/java/google/registry/rdap/RdapModule.java +++ b/java/google/registry/rdap/RdapModule.java @@ -18,7 +18,6 @@ import dagger.Module; import dagger.Provides; import google.registry.request.Parameter; import google.registry.request.RequestParameters; -import java.net.InetAddress; import java.util.Optional; import javax.servlet.http.HttpServletRequest; @@ -40,14 +39,14 @@ public final class RdapModule { @Provides @Parameter("nsIp") - static Optional provideNsIp(HttpServletRequest req) { - return RequestParameters.extractOptionalInetAddressParameter(req, "nsIp"); + static Optional provideNsIp(HttpServletRequest req) { + return RequestParameters.extractOptionalParameter(req, "nsIp"); } @Provides @Parameter("ip") - static Optional provideIp(HttpServletRequest req) { - return RequestParameters.extractOptionalInetAddressParameter(req, "ip"); + static Optional provideIp(HttpServletRequest req) { + return RequestParameters.extractOptionalParameter(req, "ip"); } @Provides diff --git a/java/google/registry/rdap/RdapNameserverSearchAction.java b/java/google/registry/rdap/RdapNameserverSearchAction.java index 4611e31da..e4f72815b 100644 --- a/java/google/registry/rdap/RdapNameserverSearchAction.java +++ b/java/google/registry/rdap/RdapNameserverSearchAction.java @@ -22,6 +22,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSortedSet; import com.google.common.collect.Iterables; +import com.google.common.net.InetAddresses; import com.google.common.primitives.Booleans; import com.googlecode.objectify.cmd.Query; import google.registry.model.domain.DomainResource; @@ -66,7 +67,7 @@ public class RdapNameserverSearchAction extends RdapActionBase { @Inject Clock clock; @Inject @Parameter("name") Optional nameParam; - @Inject @Parameter("ip") Optional ipParam; + @Inject @Parameter("ip") Optional ipParam; @Inject RdapNameserverSearchAction() {} @Override @@ -107,7 +108,13 @@ public class RdapNameserverSearchAction extends RdapActionBase { results = searchByName(RdapSearchPattern.create(Idn.toASCII(nameParam.get()), true), now); } else { // syntax: /rdap/nameservers?ip=1.2.3.4 - results = searchByIp(ipParam.get(), now); + InetAddress inetAddress; + try { + inetAddress = InetAddresses.forString(ipParam.get()); + } catch (IllegalArgumentException e) { + throw new BadRequestException("Invalid value of ip parameter"); + } + results = searchByIp(inetAddress, now); } if (results.jsonList().isEmpty()) { throw new NotFoundException("No nameservers found"); diff --git a/java/google/registry/request/RequestParameters.java b/java/google/registry/request/RequestParameters.java index 502176613..4b2f1f947 100644 --- a/java/google/registry/request/RequestParameters.java +++ b/java/google/registry/request/RequestParameters.java @@ -20,9 +20,7 @@ import static com.google.common.base.Strings.nullToEmpty; import com.google.common.base.Ascii; import com.google.common.collect.ImmutableSet; -import com.google.common.net.InetAddresses; import google.registry.request.HttpException.BadRequestException; -import java.net.InetAddress; import java.util.Optional; import javax.annotation.Nullable; import javax.servlet.http.HttpServletRequest; @@ -215,26 +213,6 @@ public final class RequestParameters { return datesBuilder.build(); } - /** - * Returns first request parameter associated with {@code name} parsed as an optional - * {@link InetAddress} (which might be IPv6). - * - * @throws BadRequestException if request parameter named {@code name} is present but could not - * be parsed as an {@link InetAddress} - */ - public static Optional extractOptionalInetAddressParameter( - HttpServletRequest req, String name) { - Optional paramVal = extractOptionalParameter(req, name); - if (!paramVal.isPresent()) { - return Optional.empty(); - } - try { - return Optional.of(InetAddresses.forString(paramVal.get())); - } catch (IllegalArgumentException e) { - throw new BadRequestException("Not an IPv4 or IPv6 address: " + name); - } - } - private static boolean equalsFalse(@Nullable String value) { return nullToEmpty(value).equalsIgnoreCase("false"); } diff --git a/javatests/google/registry/rdap/RdapDomainSearchActionTest.java b/javatests/google/registry/rdap/RdapDomainSearchActionTest.java index e9697c054..a03fc5e3d 100644 --- a/javatests/google/registry/rdap/RdapDomainSearchActionTest.java +++ b/javatests/google/registry/rdap/RdapDomainSearchActionTest.java @@ -126,7 +126,7 @@ public class RdapDomainSearchActionTest { case NS_IP: action.nameParam = Optional.empty(); action.nsLdhNameParam = Optional.empty(); - action.nsIpParam = Optional.of(InetAddresses.forString(paramValue)); + action.nsIpParam = Optional.of(paramValue); break; default: action.nameParam = Optional.empty(); @@ -1438,6 +1438,12 @@ public class RdapDomainSearchActionTest { assertThat(response.getStatus()).isEqualTo(200); } + @Test + public void testAddressMatchV4Address_invalidAddress() throws Exception { + generateActualJson(RequestType.NS_IP, "1.2.3.4.5.6.7.8.9"); + assertThat(response.getStatus()).isEqualTo(400); + } + @Test public void testAddressMatchV4Address_foundMultiple() throws Exception { assertThat(generateActualJson(RequestType.NS_IP, "1.2.3.4")) diff --git a/javatests/google/registry/rdap/RdapNameserverSearchActionTest.java b/javatests/google/registry/rdap/RdapNameserverSearchActionTest.java index 03e5e648c..921e8ce58 100644 --- a/javatests/google/registry/rdap/RdapNameserverSearchActionTest.java +++ b/javatests/google/registry/rdap/RdapNameserverSearchActionTest.java @@ -33,7 +33,6 @@ import com.google.appengine.api.users.User; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; -import com.google.common.net.InetAddresses; import com.googlecode.objectify.Key; import google.registry.model.domain.DomainResource; import google.registry.model.host.HostResource; @@ -78,7 +77,6 @@ public class RdapNameserverSearchActionTest { private DomainResource domainCatLol; private HostResource hostNs1CatLol; private HostResource hostNs2CatLol; - private HostResource hostNs1Cat2Lol; private Object generateActualJsonWithName(String name) { action.nameParam = Optional.of(name); @@ -87,7 +85,7 @@ public class RdapNameserverSearchActionTest { } private Object generateActualJsonWithIp(String ipString) { - action.ipParam = Optional.of(InetAddresses.forString(ipString)); + action.ipParam = Optional.of(ipString); action.run(); return JSONValue.parse(response.getPayload()); } @@ -104,7 +102,7 @@ public class RdapNameserverSearchActionTest { "ns1.cat.lol", "1.2.3.4", clock.nowUtc().minusYears(1)); hostNs2CatLol = makeAndPersistHostResource( "ns2.cat.lol", "bad:f00d:cafe::15:beef", clock.nowUtc().minusYears(1)); - hostNs1Cat2Lol = makeAndPersistHostResource( + makeAndPersistHostResource( "ns1.cat2.lol", "1.2.3.3", "bad:f00d:cafe::15:beef", clock.nowUtc().minusYears(1)); makeAndPersistHostResource("ns1.cat.external", null, null, clock.nowUtc().minusYears(1)); @@ -487,8 +485,8 @@ public class RdapNameserverSearchActionTest { @Test public void testNameMatchDeletedHost_foundTheOtherHost() throws Exception { persistResource( - hostNs1Cat2Lol.asBuilder().setDeletionTime(clock.nowUtc().minusDays(1)).build()); - assertThat(generateActualJsonWithIp("bad:f00d:cafe::15:beef")) + hostNs1CatLol.asBuilder().setDeletionTime(clock.nowUtc().minusDays(1)).build()); + assertThat(generateActualJsonWithName("ns*.cat.lol")) .isEqualTo( generateExpectedJsonForNameserver( "ns2.cat.lol", @@ -586,6 +584,12 @@ public class RdapNameserverSearchActionTest { assertThat(response.getStatus()).isEqualTo(404); } + @Test + public void testAddressMatch_invalidAddress() throws Exception { + generateActualJsonWithIp("It is to laugh"); + assertThat(response.getStatus()).isEqualTo(400); + } + @Test public void testAddressMatchV4Address_found() throws Exception { assertThat(generateActualJsonWithIp("1.2.3.4"))