From 326cf698e02613059dfeacb586c2b68081080e44 Mon Sep 17 00:00:00 2001 From: mountford Date: Thu, 12 Oct 2017 11:21:57 -0700 Subject: [PATCH] Don't validate RDAP nameserver names using validateDomainName The nameserver may be external, in which case its TLD will not appear in our list of valid TLDs, and the search will be rejected erroneously. Tests for letter case canonicalizations also added at reviewer's suggestion. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=171985702 --- .../flows/domain/DomainFlowUtils.java | 2 +- .../registry/flows/host/HostFlowUtils.java | 2 +- java/google/registry/rdap/BUILD | 1 + java/google/registry/rdap/RdapActionBase.java | 17 ---- .../registry/rdap/RdapDomainAction.java | 12 ++- .../registry/rdap/RdapNameserverAction.java | 12 ++- .../registry/rdap/RdapDomainActionTest.java | 97 ++++++++----------- .../rdap/RdapDomainSearchActionTest.java | 6 +- .../registry/rdap/RdapEntityActionTest.java | 1 - .../rdap/RdapEntitySearchActionTest.java | 1 - .../registry/rdap/RdapHelpActionTest.java | 1 - .../registry/rdap/RdapJsonFormatterTest.java | 1 - .../rdap/RdapNameserverActionTest.java | 40 +++++++- .../rdap/RdapNameserverSearchActionTest.java | 10 +- .../registry/rdap/RdapSearchPatternTest.java | 1 - 15 files changed, 113 insertions(+), 91 deletions(-) diff --git a/java/google/registry/flows/domain/DomainFlowUtils.java b/java/google/registry/flows/domain/DomainFlowUtils.java index f4804bec1..3d3cdff94 100644 --- a/java/google/registry/flows/domain/DomainFlowUtils.java +++ b/java/google/registry/flows/domain/DomainFlowUtils.java @@ -169,7 +169,7 @@ public class DomainFlowUtils { * * @see #validateDomainNameWithIdnTables(InternetDomainName) */ - static InternetDomainName validateDomainName(String name) + public static InternetDomainName validateDomainName(String name) throws EppException { if (!ALLOWED_CHARS.matchesAllOf(name)) { throw new BadDomainNameCharacterException(); diff --git a/java/google/registry/flows/host/HostFlowUtils.java b/java/google/registry/flows/host/HostFlowUtils.java index 6facd351c..49b11df08 100644 --- a/java/google/registry/flows/host/HostFlowUtils.java +++ b/java/google/registry/flows/host/HostFlowUtils.java @@ -39,7 +39,7 @@ import org.joda.time.DateTime; public class HostFlowUtils { /** Checks that a host name is valid. */ - static InternetDomainName validateHostName(String name) throws EppException { + public static InternetDomainName validateHostName(String name) throws EppException { checkArgumentNotNull(name, "Must specify host name to validate"); if (name.length() > 253) { throw new HostNameTooLongException(); diff --git a/java/google/registry/rdap/BUILD b/java/google/registry/rdap/BUILD index 297fbaf68..0f21676fa 100644 --- a/java/google/registry/rdap/BUILD +++ b/java/google/registry/rdap/BUILD @@ -9,6 +9,7 @@ java_library( srcs = glob(["*.java"]), deps = [ "//java/google/registry/config", + "//java/google/registry/flows", "//java/google/registry/model", "//java/google/registry/request", "//java/google/registry/request/auth", diff --git a/java/google/registry/rdap/RdapActionBase.java b/java/google/registry/rdap/RdapActionBase.java index c56ea7b4f..2b1dfd4be 100644 --- a/java/google/registry/rdap/RdapActionBase.java +++ b/java/google/registry/rdap/RdapActionBase.java @@ -19,8 +19,6 @@ import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkState; import static com.google.common.net.HttpHeaders.ACCESS_CONTROL_ALLOW_ORIGIN; import static google.registry.model.ofy.ObjectifyService.ofy; -import static google.registry.model.registry.Registries.findTldForName; -import static google.registry.model.registry.Registries.getTlds; import static google.registry.util.DateTimeUtils.END_OF_TIME; import static google.registry.util.DomainNameUtils.canonicalizeDomainName; import static javax.servlet.http.HttpServletResponse.SC_BAD_REQUEST; @@ -28,7 +26,6 @@ import static javax.servlet.http.HttpServletResponse.SC_INTERNAL_SERVER_ERROR; import static javax.servlet.http.HttpServletResponse.SC_OK; import com.google.common.collect.ImmutableMap; -import com.google.common.net.InternetDomainName; import com.google.common.net.MediaType; import com.google.re2j.Pattern; import com.googlecode.objectify.Key; @@ -39,8 +36,6 @@ import google.registry.model.registrar.Registrar; import google.registry.rdap.RdapSearchResults.IncompletenessWarningType; import google.registry.request.Action; import google.registry.request.HttpException; -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.RequestMethod; @@ -247,18 +242,6 @@ public abstract class RdapActionBase implements Runnable { && (!registrarParam.isPresent() || registrarParam.get().equals(registrar.getClientId())); } - void validateDomainName(String name) { - try { - Optional tld = findTldForName(InternetDomainName.from(name)); - if (!tld.isPresent() || !getTlds().contains(tld.get().toString())) { - throw new NotFoundException(name + " not found"); - } - } catch (IllegalArgumentException e) { - throw new BadRequestException( - name + " is not a valid " + getHumanReadableObjectTypeName()); - } - } - String canonicalizeName(String name) { name = canonicalizeDomainName(name); if (name.endsWith(".")) { diff --git a/java/google/registry/rdap/RdapDomainAction.java b/java/google/registry/rdap/RdapDomainAction.java index 356e49f9c..df8f20f13 100644 --- a/java/google/registry/rdap/RdapDomainAction.java +++ b/java/google/registry/rdap/RdapDomainAction.java @@ -14,14 +14,17 @@ package google.registry.rdap; +import static google.registry.flows.domain.DomainFlowUtils.validateDomainName; import static google.registry.model.EppResourceUtils.loadByForeignKey; import static google.registry.request.Action.Method.GET; import static google.registry.request.Action.Method.HEAD; import com.google.common.collect.ImmutableMap; +import google.registry.flows.EppException; import google.registry.model.domain.DomainResource; 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.auth.Auth; import google.registry.util.Clock; @@ -57,7 +60,14 @@ public class RdapDomainAction extends RdapActionBase { String pathSearchString, boolean isHeadRequest, String linkBase) { DateTime now = clock.nowUtc(); pathSearchString = canonicalizeName(pathSearchString); - validateDomainName(pathSearchString); + try { + validateDomainName(pathSearchString); + } catch (EppException e) { + throw new BadRequestException( + String.format( + "%s is not a valid %s: %s", + pathSearchString, getHumanReadableObjectTypeName(), e.getMessage())); + } // The query string is not used; the RDAP syntax is /rdap/domain/mydomain.com. DomainResource domainResource = loadByForeignKey(DomainResource.class, pathSearchString, now); if (domainResource == null) { diff --git a/java/google/registry/rdap/RdapNameserverAction.java b/java/google/registry/rdap/RdapNameserverAction.java index fa587b30b..8da8a79d6 100644 --- a/java/google/registry/rdap/RdapNameserverAction.java +++ b/java/google/registry/rdap/RdapNameserverAction.java @@ -14,15 +14,18 @@ package google.registry.rdap; +import static google.registry.flows.host.HostFlowUtils.validateHostName; import static google.registry.model.EppResourceUtils.loadByForeignKey; import static google.registry.request.Action.Method.GET; import static google.registry.request.Action.Method.HEAD; import static google.registry.util.DateTimeUtils.START_OF_TIME; import com.google.common.collect.ImmutableMap; +import google.registry.flows.EppException; import google.registry.model.host.HostResource; 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.auth.Auth; import google.registry.util.Clock; @@ -59,7 +62,14 @@ public class RdapNameserverAction extends RdapActionBase { DateTime now = clock.nowUtc(); pathSearchString = canonicalizeName(pathSearchString); // The RDAP syntax is /rdap/nameserver/ns1.mydomain.com. - validateDomainName(pathSearchString); + try { + validateHostName(pathSearchString); + } catch (EppException e) { + throw new BadRequestException( + String.format( + "%s is not a valid %s: %s", + pathSearchString, getHumanReadableObjectTypeName(), e.getMessage())); + } // If there are no undeleted nameservers with the given name, the foreign key should point to // the most recently deleted one. HostResource hostResource = diff --git a/javatests/google/registry/rdap/RdapDomainActionTest.java b/javatests/google/registry/rdap/RdapDomainActionTest.java index ce22cc7f9..822b01792 100644 --- a/javatests/google/registry/rdap/RdapDomainActionTest.java +++ b/javatests/google/registry/rdap/RdapDomainActionTest.java @@ -15,7 +15,6 @@ package google.registry.rdap; import static com.google.common.truth.Truth.assertThat; -import static com.google.common.truth.Truth8.assertThat; import static google.registry.testing.DatastoreHelper.createTld; import static google.registry.testing.DatastoreHelper.persistResource; import static google.registry.testing.DatastoreHelper.persistSimpleResources; @@ -322,21 +321,44 @@ public class RdapDomainActionTest { } } + private void assertProperResponseForCatLol(String queryString, String expectedOutputFile) { + assertJsonEqual( + generateActualJson(queryString), + generateExpectedJsonWithTopLevelEntries( + "cat.lol", + null, + "C-LOL", + expectedOutputFile.equals("rdap_domain.json") + ? ImmutableList.of("4-ROID", "6-ROID", "2-ROID") + : null, + expectedOutputFile)); + assertThat(response.getStatus()).isEqualTo(200); + } + @Test public void testInvalidDomain_returns400() throws Exception { assertJsonEqual( generateActualJson("invalid/domain/name"), generateExpectedJson( - "invalid/domain/name is not a valid domain name", null, "1", "rdap_error_400.json")); + "invalid/domain/name is not a valid domain name: Domain names can only contain a-z," + + " 0-9, '.' and '-'", + null, + "1", + "rdap_error_400.json")); assertThat(response.getStatus()).isEqualTo(400); } @Test - public void testUnknownDomain_returns404() throws Exception { + public void testUnknownDomain_returns400() throws Exception { assertJsonEqual( generateActualJson("missingdomain.com"), - generateExpectedJson("missingdomain.com not found", null, "1", "rdap_error_404.json")); - assertThat(response.getStatus()).isEqualTo(404); + generateExpectedJson( + "missingdomain.com is not a valid domain name: Domain name is under tld com which" + + " doesn't exist", + null, + "1", + "rdap_error_400.json")); + assertThat(response.getStatus()).isEqualTo(400); } @Test @@ -349,15 +371,7 @@ public class RdapDomainActionTest { @Test public void testValidDomain_works() throws Exception { - assertJsonEqual( - generateActualJson("cat.lol"), - generateExpectedJsonWithTopLevelEntries( - "cat.lol", - null, - "C-LOL", - ImmutableList.of("4-ROID", "6-ROID", "2-ROID"), - "rdap_domain.json")); - assertThat(response.getStatus()).isEqualTo(200); + assertProperResponseForCatLol("cat.lol", "rdap_domain.json"); } @Test @@ -366,69 +380,34 @@ public class RdapDomainActionTest { action.authResult = AuthResult.create(AuthLevel.USER, adminUserAuthInfo); when(sessionUtils.checkRegistrarConsoleLogin(request, adminUserAuthInfo)).thenReturn(false); when(sessionUtils.getRegistrarClientId(request)).thenReturn("noregistrar"); - assertJsonEqual( - generateActualJson("cat.lol"), - generateExpectedJsonWithTopLevelEntries( - "cat.lol", - null, - "C-LOL", - ImmutableList.of("4-ROID", "6-ROID", "2-ROID"), - "rdap_domain.json")); - assertThat(response.getStatus()).isEqualTo(200); + assertProperResponseForCatLol("cat.lol", "rdap_domain.json"); } @Test public void testValidDomain_notLoggedIn_noContacts() throws Exception { when(sessionUtils.checkRegistrarConsoleLogin(request, userAuthInfo)).thenReturn(false); - assertJsonEqual( - generateActualJson("cat.lol"), - generateExpectedJsonWithTopLevelEntries( - "cat.lol", - null, - "C-LOL", - null, - "rdap_domain_no_contacts.json")); - assertThat(response.getStatus()).isEqualTo(200); + assertProperResponseForCatLol("cat.lol", "rdap_domain_no_contacts.json"); } @Test public void testValidDomain_loggedInAsOtherRegistrar_noContacts() throws Exception { when(sessionUtils.getRegistrarClientId(request)).thenReturn("otherregistrar"); - assertJsonEqual( - generateActualJson("cat.lol"), - generateExpectedJsonWithTopLevelEntries( - "cat.lol", - null, - "C-LOL", - null, - "rdap_domain_no_contacts.json")); - assertThat(response.getStatus()).isEqualTo(200); + assertProperResponseForCatLol("cat.lol", "rdap_domain_no_contacts.json"); + } + + @Test + public void testUpperCase_ignored() throws Exception { + assertProperResponseForCatLol("CaT.lOl", "rdap_domain.json"); } @Test public void testTrailingDot_ignored() throws Exception { - assertJsonEqual( - generateActualJson("cat.lol."), - generateExpectedJsonWithTopLevelEntries( - "cat.lol", - null, - "C-LOL", - ImmutableList.of("4-ROID", "6-ROID", "2-ROID"), - "rdap_domain.json")); - assertThat(response.getStatus()).isEqualTo(200); + assertProperResponseForCatLol("cat.lol.", "rdap_domain.json"); } @Test public void testQueryParameter_ignored() throws Exception { - assertJsonEqual( - generateActualJson("cat.lol?key=value"), - generateExpectedJsonWithTopLevelEntries( - "cat.lol", - null, - "C-LOL", - ImmutableList.of("4-ROID", "6-ROID", "2-ROID"), - "rdap_domain.json")); - assertThat(response.getStatus()).isEqualTo(200); + assertProperResponseForCatLol("cat.lol?key=value", "rdap_domain.json"); } @Test diff --git a/javatests/google/registry/rdap/RdapDomainSearchActionTest.java b/javatests/google/registry/rdap/RdapDomainSearchActionTest.java index a86f4bf3e..7fb48cf15 100644 --- a/javatests/google/registry/rdap/RdapDomainSearchActionTest.java +++ b/javatests/google/registry/rdap/RdapDomainSearchActionTest.java @@ -16,7 +16,6 @@ package google.registry.rdap; import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.truth.Truth.assertThat; -import static com.google.common.truth.Truth8.assertThat; import static google.registry.testing.DatastoreHelper.createTld; import static google.registry.testing.DatastoreHelper.persistDomainAsDeleted; import static google.registry.testing.DatastoreHelper.persistResource; @@ -586,6 +585,11 @@ public class RdapDomainSearchActionTest { runSuccessfulTestWithCatLol(RequestType.NAME, "cat.lol", "rdap_domain.json"); } + @Test + public void testDomainMatch_foundWithUpperCase() throws Exception { + runSuccessfulTestWithCatLol(RequestType.NAME, "CaT.lOl", "rdap_domain.json"); + } + @Test public void testDomainMatch_found_asAdministrator() throws Exception { UserAuthInfo adminUserAuthInfo = UserAuthInfo.create(user, true); diff --git a/javatests/google/registry/rdap/RdapEntityActionTest.java b/javatests/google/registry/rdap/RdapEntityActionTest.java index 0e97dc3eb..8aca601bf 100644 --- a/javatests/google/registry/rdap/RdapEntityActionTest.java +++ b/javatests/google/registry/rdap/RdapEntityActionTest.java @@ -15,7 +15,6 @@ package google.registry.rdap; import static com.google.common.truth.Truth.assertThat; -import static com.google.common.truth.Truth8.assertThat; import static google.registry.testing.DatastoreHelper.createTld; import static google.registry.testing.DatastoreHelper.persistResource; import static google.registry.testing.DatastoreHelper.persistSimpleResources; diff --git a/javatests/google/registry/rdap/RdapEntitySearchActionTest.java b/javatests/google/registry/rdap/RdapEntitySearchActionTest.java index d7920928c..b69a6a94e 100644 --- a/javatests/google/registry/rdap/RdapEntitySearchActionTest.java +++ b/javatests/google/registry/rdap/RdapEntitySearchActionTest.java @@ -16,7 +16,6 @@ package google.registry.rdap; import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.truth.Truth.assertThat; -import static com.google.common.truth.Truth8.assertThat; import static google.registry.testing.DatastoreHelper.createTld; import static google.registry.testing.DatastoreHelper.persistResource; import static google.registry.testing.DatastoreHelper.persistResources; diff --git a/javatests/google/registry/rdap/RdapHelpActionTest.java b/javatests/google/registry/rdap/RdapHelpActionTest.java index 6a1f8047c..7700bd6ad 100644 --- a/javatests/google/registry/rdap/RdapHelpActionTest.java +++ b/javatests/google/registry/rdap/RdapHelpActionTest.java @@ -15,7 +15,6 @@ package google.registry.rdap; import static com.google.common.truth.Truth.assertThat; -import static com.google.common.truth.Truth8.assertThat; import static google.registry.testing.TestDataHelper.loadFileWithSubstitutions; import com.google.common.collect.ImmutableMap; diff --git a/javatests/google/registry/rdap/RdapJsonFormatterTest.java b/javatests/google/registry/rdap/RdapJsonFormatterTest.java index f62720fec..f89ad4b8a 100644 --- a/javatests/google/registry/rdap/RdapJsonFormatterTest.java +++ b/javatests/google/registry/rdap/RdapJsonFormatterTest.java @@ -15,7 +15,6 @@ package google.registry.rdap; import static com.google.common.truth.Truth.assertThat; -import static com.google.common.truth.Truth8.assertThat; import static google.registry.testing.DatastoreHelper.createTld; import static google.registry.testing.DatastoreHelper.persistResource; import static google.registry.testing.DatastoreHelper.persistSimpleResources; diff --git a/javatests/google/registry/rdap/RdapNameserverActionTest.java b/javatests/google/registry/rdap/RdapNameserverActionTest.java index 9253bba26..8b0d01dd9 100644 --- a/javatests/google/registry/rdap/RdapNameserverActionTest.java +++ b/javatests/google/registry/rdap/RdapNameserverActionTest.java @@ -15,7 +15,6 @@ package google.registry.rdap; import static com.google.common.truth.Truth.assertThat; -import static com.google.common.truth.Truth8.assertThat; import static google.registry.testing.DatastoreHelper.createTld; import static google.registry.testing.DatastoreHelper.persistResource; import static google.registry.testing.FullFieldsTestEntityHelper.makeAndPersistHostResource; @@ -94,6 +93,8 @@ public class RdapNameserverActionTest { // other registrar persistResource( makeRegistrar("otherregistrar", "Yes Virginia