fix(transfer): accept external nameservers without registry validation (#525)#537
Open
msiouti wants to merge 2 commits into
Open
fix(transfer): accept external nameservers without registry validation (#525)#537msiouti wants to merge 2 commits into
msiouti wants to merge 2 commits into
Conversation
openprovider#525) APITools::createNameserversArray gated nameserver inclusion on the ability to resolve an IP for every NS — first via Openprovider's searchNsRequest registry lookup, then via gethostbyname. For out-of-bailiwick nameservers (the vast majority of real-world cases — ns1.cloudflare.com, dns.google, etc.) neither path is appropriate: the registry lookup is for glue records only, and gethostbyname can fail or return the input unchanged. When that happened the NS was silently dropped from the array, the caller passed the truncated list to transferDomainRequest, and the registry substituted reseller defaults — silently changing customer DNS delegation at the moment of transfer. This change scopes IP resolution to in-bailiwick glue records (NS that are the apex domain itself or a subdomain of the domain being registered/transferred). External NS are accepted by name alone and serialized without an ip field. Glue records still attempt the registry then DNS resolution; if neither produces a valid IPv4 the function now throws with a clear message instead of dropping the NS. Fixes openprovider#525.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #525.
Problem
APITools::createNameserversArraygated nameserver inclusion on the ability to resolve an IP for every NS — first via Openprovider'ssearchNsRequestregistry lookup, then viagethostbyname. For out-of-bailiwick nameservers (the vast majority of real-world cases —ns1.cloudflare.com,dns.google, etc.) neither path is appropriate: the registry lookup is for glue records only, andgethostbynamecan fail or return the input unchanged. When that happened the NS was silently dropped from the array, the truncated list was passed totransferDomainRequest, and the registry substituted the reseller's default nameservers — silently changing customer DNS delegation at the moment of transfer, with no exception or warning.Fix
Scope IP resolution to in-bailiwick glue records only — i.e. NS that are the apex domain itself or a subdomain of the domain being registered/transferred. Out-of-bailiwick NS are now accepted by name alone and serialized without an
ipfield. Glue records still try the registry first, then DNS resolution; if neither produces a valid IPv4 the function throws with a clear message instead of dropping the NS.The two code paths in the original (
test_mode == 'on'and the default branch) collapsed into one because their differences were incidental to the fix and the new path is a strict superset.Changes
OpenProvider/API/APITools.php—createNameserversArrayrewritten; two new private helpersisGlueRecordandresolveGlueIpextracted.tests/API/APIToolsTest.php— new PHPUnit cases:test_external_nameservers_are_accepted_without_ip_lookup— the direct regression test for [BUG] transferDomain silently replaces external nameservers with module defaults — APITools::createNameserversArray regression #525; assertsapiHelper::getNameserverListis not called for non-glue NS.test_explicit_ip_in_name_slash_ip_form_is_preservedtest_glue_record_without_resolvable_ip_throwstest_glue_record_uses_registry_ip_when_availabletest_throws_when_fewer_than_two_nameservers_suppliedtest_mixed_glue_and_external_nameserverstest_nameserver_names_are_lowercasedRisk / verification
DomainNameServer::$ipalready defaults tonull, so omitting the key from the constructor array leaves the property null. The SymfonyPropertyNormalizerused byApiHelperwill emitip: nullfor external NS in the request body. If, in review, we'd rather suppress the null entirely, the smallest follow-up is to makeDomainNameServerimplementJsonSerializableand skip null fields — happy to add that in this PR if reviewers prefer.DomainController::transfer,DomainController::register,NameserverController::saveNameservers) use the return value as-is — no caller-side changes required.composer.jsonresolves to >= 8.1 (pervendor/composer/platform_check.php), sostr_ends_withis available natively.Test plan
ns1.<being-transferred>.tld) with explicitname/ipand confirm IP is preserved.