diff --git a/java/google/registry/rdap/RdapDomainSearchAction.java b/java/google/registry/rdap/RdapDomainSearchAction.java index 1e15ea868..c99fdb8f0 100644 --- a/java/google/registry/rdap/RdapDomainSearchAction.java +++ b/java/google/registry/rdap/RdapDomainSearchAction.java @@ -261,51 +261,43 @@ public class RdapDomainSearchAction extends RdapActionBase { } // Handle queries with a wildcard. } else { - // If there is a suffix, it must be a domain. If it happens to be a domain that we manage, - // we can look up the domain and look through the subordinate hosts. This is more efficient, - // and lets us permit wildcard searches with no initial string. + // If there is a suffix, it must be a domain that we manage. That way, we can look up the + // domain and search through the subordinate hosts. This is more efficient, and lets us permit + // wildcard searches with no initial string. if (partialStringQuery.getSuffix() != null) { DomainResource domainResource = loadByForeignKey( DomainResource.class, partialStringQuery.getSuffix(), now); - if (domainResource != null) { - ImmutableList.Builder> builder = new ImmutableList.Builder<>(); - for (String fqhn : ImmutableSortedSet.copyOf(domainResource.getSubordinateHosts())) { - // We can't just check that the host name starts with the initial query string, because - // then the query ns.exam*.example.com would match against nameserver ns.example.com. - if (partialStringQuery.matches(fqhn)) { - Key hostKey = loadAndGetKey(HostResource.class, fqhn, now); - if (hostKey != null) { - builder.add(hostKey); - } else { - logger.warningfmt("Host key unexpectedly null"); - } + if (domainResource == null) { + // Don't allow wildcards with suffixes which are not domains we manage. That would risk a + // table scan in some easily foreseeable cases. + throw new UnprocessableEntityException( + "A suffix in a lookup by nameserver name must be an in-bailiwick domain"); + } + ImmutableList.Builder> builder = new ImmutableList.Builder<>(); + for (String fqhn : ImmutableSortedSet.copyOf(domainResource.getSubordinateHosts())) { + // We can't just check that the host name starts with the initial query string, because + // then the query ns.exam*.example.com would match against nameserver ns.example.com. + if (partialStringQuery.matches(fqhn)) { + Key hostKey = loadAndGetKey(HostResource.class, fqhn, now); + if (hostKey != null) { + builder.add(hostKey); + } else { + logger.warningfmt("Host key unexpectedly null"); } } - return builder.build(); } - } - // If there's no suffix, or it isn't a domain we manage, query the host resources. Query the - // resources themselves, rather than the foreign key indexes, because then we have an index on - // fully qualified host name and deletion time, so we can check the deletion status in the - // query itself. There are no pending deletes for hosts, so we can call queryUndeleted. In - // this case, the initial string must be present, to avoid querying every host in the system. - // This restriction is enforced by queryUndeleted(). + return builder.build(); + // If there's no suffix, query the host resources. Query the resources themselves, rather than + // the foreign key indexes, because then we have an index on fully qualified host name and + // deletion time, so we can check the deletion status in the query itself. There are no + // pending deletes for hosts, so we can call queryUndeleted. In this case, the initial string + // must be present, to avoid querying every host in the system. This restriction is enforced + // by queryUndeleted(). // TODO (b/24463238): figure out how to limit the size of these queries effectively - Iterable> keys = - queryUndeleted(HostResource.class, "fullyQualifiedHostName", partialStringQuery, 1000) - .keys(); - // queryUndeleted() ignores suffixes, so if one was specified, we must filter on the partial - // string query. - if (partialStringQuery.getSuffix() == null) { - return keys; } else { - ImmutableList.Builder> filteredKeys = new ImmutableList.Builder<>(); - for (Key key : keys) { - if (partialStringQuery.matches(key.getName())) { - filteredKeys.add(key); - } - } - return filteredKeys.build(); + return queryUndeleted( + HostResource.class, "fullyQualifiedHostName", partialStringQuery, 1000) + .keys(); } } } diff --git a/java/google/registry/rdap/RdapNameserverSearchAction.java b/java/google/registry/rdap/RdapNameserverSearchAction.java index de6db742c..5cc3d416f 100644 --- a/java/google/registry/rdap/RdapNameserverSearchAction.java +++ b/java/google/registry/rdap/RdapNameserverSearchAction.java @@ -35,11 +35,13 @@ import google.registry.rdap.RdapJsonFormatter.OutputDataType; import google.registry.request.Action; import google.registry.request.HttpException.BadRequestException; import google.registry.request.HttpException.NotFoundException; +import google.registry.request.HttpException.UnprocessableEntityException; import google.registry.request.Parameter; import google.registry.request.auth.Auth; import google.registry.util.Clock; import google.registry.util.Idn; import java.net.InetAddress; +import java.util.ArrayList; import java.util.List; import javax.inject.Inject; import org.joda.time.DateTime; @@ -140,38 +142,40 @@ public class RdapNameserverSearchAction extends RdapActionBase { hostResource, false, rdapLinkBase, rdapWhoisServer, now, OutputDataType.FULL))); // Handle queries with a wildcard. } else { - // If there is a suffix, it should be a domain. If it happens to be a domain that we manage, - // we can look up the domain and look through the subordinate hosts. This is more efficient, - // and lets us permit wildcard searches with no initial string. + // If there is a suffix, it should be a domain that we manage, so we can look up the domain + // and search through the subordinate hosts. This is more efficient, and lets us permit + // wildcard searches with no initial string. if (partialStringQuery.getSuffix() != null) { DomainResource domainResource = loadByForeignKey(DomainResource.class, partialStringQuery.getSuffix(), now); - ImmutableList.Builder hostListBuilder = new ImmutableList.Builder<>(); - if (domainResource != null) { - for (String fqhn : ImmutableSortedSet.copyOf(domainResource.getSubordinateHosts())) { - // We can't just check that the host name starts with the initial query string, because - // then the query ns.exam*.example.com would match against nameserver ns.example.com. - if (partialStringQuery.matches(fqhn)) { - HostResource hostResource = loadByForeignKey(HostResource.class, fqhn, now); - if (hostResource != null) { - hostListBuilder.add(hostResource); + if (domainResource == null) { + // Don't allow wildcards with suffixes which are not domains we manage. That would risk a + // table scan in many easily foreseeable cases. The user might ask for ns*.zombo.com, + // forcing us to query for all hosts beginning with ns, then filter for those ending in + // .zombo.com. It might well be that 80% of all hostnames begin with ns, leading to + // inefficiency. + throw new UnprocessableEntityException( + "A suffix after a wildcard in a nameserver lookup must be an in-bailiwick domain"); + } + List hostList = new ArrayList<>(); + for (String fqhn : ImmutableSortedSet.copyOf(domainResource.getSubordinateHosts())) { + // We can't just check that the host name starts with the initial query string, because + // then the query ns.exam*.example.com would match against nameserver ns.example.com. + if (partialStringQuery.matches(fqhn)) { + HostResource hostResource = loadByForeignKey(HostResource.class, fqhn, now); + if (hostResource != null) { + hostList.add(hostResource); + if (hostList.size() > rdapResultSetMaxSize) { + break; } } } - } else { - // If we don't recognize the domain, call queryUndeleted and filter. - // TODO(mountford): figure out how to size this correctly - for (HostResource hostResource : - queryUndeleted( - HostResource.class, "fullyQualifiedHostName", partialStringQuery, 1000)) { - if (partialStringQuery.matches(hostResource.getFullyQualifiedHostName())) { - hostListBuilder.add(hostResource); - } - } } - return makeSearchResults(hostListBuilder.build(), now); + return makeSearchResults(hostList, now); // Handle queries with a wildcard, but no suffix. There are no pending deletes for hosts, so - // we can call queryUndeleted. + // we can call queryUndeleted. Unlike the above problem with suffixes, we can safely search + // for nameservers beginning with a particular suffix, because we need only fetch the first + // rdapResultSetMaxSize entries, and ignore the rest. } else { return makeSearchResults( // Add 1 so we can detect truncation. diff --git a/javatests/google/registry/rdap/RdapDomainSearchActionTest.java b/javatests/google/registry/rdap/RdapDomainSearchActionTest.java index 55e062f70..2b54e2e3f 100644 --- a/javatests/google/registry/rdap/RdapDomainSearchActionTest.java +++ b/javatests/google/registry/rdap/RdapDomainSearchActionTest.java @@ -815,8 +815,8 @@ public class RdapDomainSearchActionTest { } @Test - public void testNameserverMatchWithWildcardAndTldSuffix_notFound() throws Exception { - generateActualJson(RequestType.NS_LDH_NAME, "ns2.cat*.lol"); + public void testNameserverMatchWithWildcardAndDomainSuffix_notFound() throws Exception { + generateActualJson(RequestType.NS_LDH_NAME, "ns5*.cat.lol"); assertThat(response.getStatus()).isEqualTo(404); } @@ -849,6 +849,12 @@ public class RdapDomainSearchActionTest { assertThat(response.getStatus()).isEqualTo(422); } + @Test + public void testNameserverMatchWithWildcardAndInvalidSuffix_unprocessable() throws Exception { + generateActualJson(RequestType.NS_LDH_NAME, "ns*.google.com"); + assertThat(response.getStatus()).isEqualTo(422); + } + @Test public void testNameserverMatch_ns2_cat_lol_found() throws Exception { generateActualJson(RequestType.NS_LDH_NAME, "ns2.cat.lol"); @@ -887,9 +893,9 @@ public class RdapDomainSearchActionTest { } @Test - public void testNameserverMatch_nsstar_test_notFound() throws Exception { + public void testNameserverMatch_nsstar_test_unprocessable() throws Exception { generateActualJson(RequestType.NS_LDH_NAME, "ns*.1.test"); - assertThat(response.getStatus()).isEqualTo(404); + assertThat(response.getStatus()).isEqualTo(422); } @Test @@ -949,10 +955,11 @@ public class RdapDomainSearchActionTest { } @Test - public void testNameserverMatchDeletedNameserverWithWildcardAndTld_notFound() throws Exception { + public void testNameserverMatchDeletedNameserverWithWildcardAndSuffix_notFound() + throws Exception { persistResource( hostNs1CatLol.asBuilder().setDeletionTime(clock.nowUtc().minusDays(1)).build()); - assertThat(generateActualJson(RequestType.NS_LDH_NAME, "ns1.cat*.lol")) + assertThat(generateActualJson(RequestType.NS_LDH_NAME, "ns1*.cat.lol")) .isEqualTo(generateExpectedJson("No matching nameservers found", "rdap_error_404.json")); assertThat(response.getStatus()).isEqualTo(404); } diff --git a/javatests/google/registry/rdap/RdapNameserverSearchActionTest.java b/javatests/google/registry/rdap/RdapNameserverSearchActionTest.java index d94421c07..6904da33d 100644 --- a/javatests/google/registry/rdap/RdapNameserverSearchActionTest.java +++ b/javatests/google/registry/rdap/RdapNameserverSearchActionTest.java @@ -239,10 +239,13 @@ public class RdapNameserverSearchActionTest { } @Test - public void testNonexistentDomainSuffix_notFound() throws Exception { + public void testNonexistentDomainSuffix_unprocessable() throws Exception { assertThat(generateActualJsonWithName("exam*.foo.bar")) - .isEqualTo(generateExpectedJson("No nameservers found", "rdap_error_404.json")); - assertThat(response.getStatus()).isEqualTo(404); + .isEqualTo( + generateExpectedJson( + "A suffix after a wildcard in a nameserver lookup must be an in-bailiwick domain", + "rdap_error_422.json")); + assertThat(response.getStatus()).isEqualTo(422); } @Test