SER-518 Add DNS validator and test #17
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds three composer dependencies (utopia-php/validators ^0.0.2, utopia-php/domains 0.9., keeps utopia-php/telemetry 0.1.). Introduces a new DNS validator class Utopia\DNS\Validator\DNS with constructor (target, type, dnsServer), public properties (records, value, count, reason), and methods getDescription, isValid, isArray, getType; includes CAA-specific parent-domain/apex logic and failure reason constants. Updates DNS client to validate that the provided server is an IP address and throw if not. Adds unit tests for CNAME/A/AAAA/CAA and an e2e DNS test update that replaces localhost with 127.0.0.1 and adds testInvalidServer. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (2)tests/unit/DNS/Validator/DNSTest.php (3)
src/DNS/Validator/DNS.php (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
vermakhushboo
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a DNS validator class that verifies DNS records (A, AAAA, CNAME, and CAA) for domains, along with comprehensive test coverage. The validator supports custom DNS resolvers and implements special CAA record inheritance logic per RFC 8659.
- Adds
DNSvalidator class with support for A, AAAA, CNAME, and CAA record validation - Implements CAA record parent domain inheritance behavior
- Adds comprehensive unit tests for all supported record types
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| src/DNS/Validator/DNS.php | New DNS validator class with record validation logic and error reporting |
| tests/unit/DNS/Validator/DNSTest.php | Comprehensive unit tests for DNS validator covering all record types |
| composer.json | Adds dependencies for utopia-php/validators and utopia-php/domains |
| composer.lock | Updates lock file with new dependencies and PHPUnit version |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/DNS/Validator/DNS.php
Outdated
| public string $reason = ''; | ||
|
|
||
| /** | ||
| * @param string $target |
There was a problem hiding this comment.
The @param documentation for $target doesn't explain what it represents. For different record types, $target has different meanings (IP address for A/AAAA, domain for CNAME, issuer domain for CAA). Add a description explaining that $target is the expected value to validate against, with its format depending on the record type.
| * @param string $target | |
| * @param string $target The expected value to validate against. For A/AAAA records, this should be an IP address. For CNAME records, a domain name. For CAA records, an issuer domain. | |
| * @param string $type The DNS record type to validate (A, AAAA, CNAME, CAA). | |
| * @param string $dnsServer Optional DNS server to use for validation. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
composer.json(1 hunks)src/DNS/Validator/DNS.php(1 hunks)tests/unit/DNS/Validator/DNSTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/unit/DNS/Validator/DNSTest.php (1)
src/DNS/Validator/DNS.php (2)
DNS(12-225)isValid(109-112)
src/DNS/Validator/DNS.php (5)
src/DNS/Client.php (1)
Client(7-68)src/DNS/Message.php (2)
Message(11-208)response(111-139)src/DNS/Message/Question.php (1)
Question(7-51)src/DNS/Message/Record.php (3)
Record(7-527)typeNameToCode(311-314)typeCodeToName(316-319)src/DNS/Message/Domain.php (1)
Domain(7-149)
🪛 GitHub Actions: CodeQL
src/DNS/Validator/DNS.php
[error] 31-31: PHPStan: Property Utopia\DNS\Validator\DNS::$recordValues type has no value specified in iterable type array. missingType.iterableValue
[error] 109-109: PHPStan: PHPDoc tag @param references unknown parameter: $domain
[error] 121-121: PHPStan: PHPDoc tag @param references unknown parameter: $domain
[error] 137-137: PHPStan: Parameter #2 $type of class Utopia\DNS\Message\Question constructor expects int, int|null given. argument.type
[error] 184-184: PHPStan: Offset 0 on non-empty-list on left side of ?? always exists and is not nullable. nullCoalesce.offset
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/DNS/Validator/DNS.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/DNS/Validator/DNS.php (5)
src/DNS/Client.php (1)
Client(7-68)src/DNS/Message.php (2)
Message(11-208)response(111-139)src/DNS/Message/Question.php (1)
Question(7-51)src/DNS/Message/Record.php (3)
Record(7-527)typeNameToCode(311-314)typeCodeToName(316-319)src/DNS/Message/Domain.php (1)
Domain(7-149)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
src/DNS/Validator/DNS.php (4)
192-193: Validate CAA record format explicitly instead of masking errors.Using
?? ''afterexplode(' ', $rdata, 3)[2]silently falls back to an empty string if the CAA record is malformed. This masks data errors. Explicitly validate the CAA record format and handle malformed records as failures.Apply this diff:
// Extract domain; comments showcase extraction steps in most complex scenario $rdata = $record->rdata; // 255 issuewild "certainly.com;validationmethods=tls-alpn-01;retrytimeout=3600" - $rdata = \explode(' ', $rdata, 3)[2] ?? ''; // "certainly.com;validationmethods=tls-alpn-01;retrytimeout=3600" + $parts = \explode(' ', $rdata, 3); + if (\count($parts) < 3) { + $this->reason = self::FAILURE_REASON_INTERNAL; + $this->logs = ['error' => 'Malformed CAA record']; + return false; + } + $rdata = $parts[2]; // "certainly.com;validationmethods=tls-alpn-01;retrytimeout=3600"Based on past review comments.
176-182: Propagate parent CAA state to maintain diagnostic coherence.When delegating to a parent domain validator, only the boolean result is returned. This leaves the current instance with
count = 0, emptyrecordValues, and defaultreason. Callers will see "missing records" even when the parent provided conflicting CAA data. Copy the child validator's diagnostic properties before returning.Apply this diff:
$parts = \explode('.', $value); \array_shift($parts); $parentDomain = \implode('.', $parts); $validator = new self($this->target, DNS::RECORD_CAA, $dnsServer); - return $validator->isValidWithDNSServer($parentDomain, $dnsServer); + $result = $validator->isValidWithDNSServer($parentDomain, $dnsServer); + + $this->recordValues = $validator->recordValues; + $this->count = $validator->count; + $this->logs = $validator->getLogs(); + $this->reason = $validator->reason; + + return $result;Based on past review comments.
197-206: Use case-insensitive comparison for domain values.DNS labels are case-insensitive (RFC 4343), but the code uses strict
===comparison. A CAA record like0 issue "LetsEncrypt.org"or CNAMEFoo.EXAMPLE.COM.will fail to match even when semantically correct. Switch tostrcasecmp()for domain comparisons.Apply this diff:
$this->recordValues[] = $rdata; - if ($rdata === $this->target) { + if (\strcasecmp($rdata, $this->target) === 0) { return true; } } else { $this->recordValues[] = $record->rdata; } - if ($record->rdata === $this->target) { + if ( + ($this->type === self::RECORD_CNAME && \strcasecmp($record->rdata, $this->target) === 0) + || (($this->type === self::RECORD_A || $this->type === self::RECORD_AAAA) && $record->rdata === $this->target) + ) { return true; }Note: For A/AAAA records (IP addresses), keep strict comparison as IPs are case-sensitive in their string representation.
Based on past review comments.
204-206: Remove unreachable code for CAA records.For CAA records, after the extracted domain is checked at line 197, the code continues to line 204 where it compares
$record->rdata(the full CAA format like"255 issuewild \"certainly.com\"") against$this->target(just the domain). This comparison will never match for CAA records. Add acontinuestatement after line 199 or restructure the logic to skip this check for CAA records.Apply this diff:
$this->recordValues[] = $rdata; if (\strcasecmp($rdata, $this->target) === 0) { return true; } + continue; // Skip the unreachable comparison below for CAA records } else { $this->recordValues[] = $record->rdata; + if ( + ($this->type === self::RECORD_CNAME && \strcasecmp($record->rdata, $this->target) === 0) + || (($this->type === self::RECORD_A || $this->type === self::RECORD_AAAA) && $record->rdata === $this->target) + ) { + return true; + } } - - if ($record->rdata === $this->target) { - return true; - } }Based on past review comments.
🧹 Nitpick comments (2)
src/DNS/Validator/DNS.php (2)
17-17: Clarify the inline comment for CAA validation.The comment "You can provide domain only (as $target) for CAA validation" is somewhat unclear. Consider rephrasing to: "For CAA validation, $target should be the expected issuer domain (e.g., 'letsencrypt.org')."
92-92: Consider clarifying the CAA error message.The message "Add new CAA record, or remove all other CAA records" may be confusing because it presents two contradictory actions without context. Consider making it more specific, such as: "Add a CAA record for '{$this->target}' or remove conflicting CAA records."
Based on past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/DNS/Validator/DNS.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/DNS/Validator/DNS.php (5)
src/DNS/Client.php (1)
Client(7-68)src/DNS/Message.php (2)
Message(11-208)response(111-139)src/DNS/Message/Question.php (1)
Question(7-51)src/DNS/Message/Record.php (3)
Record(7-527)typeNameToCode(311-314)typeCodeToName(316-319)src/DNS/Message/Domain.php (1)
Domain(7-149)
🔇 Additional comments (3)
src/DNS/Validator/DNS.php (3)
98-115: LGTM!These delegation and accessor methods follow a clean pattern.
126-148: LGTM!The initial validation setup correctly checks input types, initializes state, validates the type code (addressing previous PHPStan errors), and constructs the DNS query.
212-234: LGTM!The
isArray()andgetType()methods correctly implement the validator interface.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/DNS/Validator/DNS.php (3)
178-183: Propagate parent CAA validator state.When recursively validating the parent domain for CAA records, the child validator's diagnostic state (count, recordValues, reason, logs) is not copied back to the current instance. Callers will see incorrect diagnostics (e.g., "missing records" when the parent actually had conflicting CAA data).
Apply this diff to synchronize the state:
$validator = new self($this->target, DNS::RECORD_CAA, $dnsServer); - return $validator->isValidWithDNSServer($parentDomain, $dnsServer); + $result = $validator->isValidWithDNSServer($parentDomain, $dnsServer); + + $this->recordValues = $validator->recordValues; + $this->count = $validator->count; + $this->logs = $validator->getLogs(); + $this->reason = $validator->reason; + + return $result;Based on learnings
141-166: Remove debug statements before merging.Two more
var_dumpstatements will output to the response and must be removed.Apply this diff:
$response = $dns->query($queryMessage); $rawQuery = $response->answers; - var_dump($rawQuery); // Some DNS servers return all records, not only type that's asked for // Likely occurs when no records of specific type are found $query = array_filter($rawQuery, function ($record) { $typeName = Record::typeCodeToName($record->type); return $typeName === $this->type; }); $this->logs = $query; } catch (\Exception $e) { - var_dump("error in isValidWithDNSServer"); $this->reason = self::FAILURE_REASON_QUERY; $this->logs = ['error' => $e->getMessage()]; return false;Based on learnings
189-211: Fix CAA validation logic and add case-insensitive domain comparison.This code segment has two major issues:
CAA records fall through to incorrect comparison: After extracting and comparing the CAA domain at lines 191-201, execution continues to line 206 where
$record->rdata(the full CAA format like"255 issuewild "certainly.com..."") is compared against$this->target. This comparison will never match and is unreachable for CAA records.DNS domain comparisons are case-sensitive: DNS labels are case-insensitive, but the code uses strict
===equality. Records like0 issue "LetsEncrypt.org"orCNAME Foo.EXAMPLE.COM.will fail to match even when semantically correct.Apply this diff to fix both issues:
foreach ($query as $record) { // CAA validation only needs to ensure domain if ($this->type === self::RECORD_CAA) { // Extract domain; comments showcase extraction steps in most complex scenario $rdata = $record->rdata; // 255 issuewild "certainly.com;validationmethods=tls-alpn-01;retrytimeout=3600" $rdata = \explode(' ', $rdata, 3)[2] ?? ''; // "certainly.com;validationmethods=tls-alpn-01;retrytimeout=3600" $rdata = \trim($rdata, '"'); // certainly.com;validationmethods=tls-alpn-01;retrytimeout=3600 $rdata = \explode(';', $rdata, 2)[0]; // certainly.com $this->recordValues[] = $rdata; - if ($rdata === $this->target) { + if (\strcasecmp($rdata, $this->target) === 0) { return true; } + continue; // Skip the comparison below for CAA records } else { $this->recordValues[] = $record->rdata; } - if ($record->rdata === $this->target) { + // Use case-insensitive comparison for CNAME, case-sensitive for A/AAAA (IP addresses) + if ($this->type === self::RECORD_CNAME) { + if (\strcasecmp($record->rdata, $this->target) === 0) { + return true; + } + } elseif ($record->rdata === $this->target) { return true; } }Based on learnings
🧹 Nitpick comments (2)
src/DNS/Validator/DNS.php (2)
38-41: Clarify$targetparameter documentation.The
$targetparameter has different meanings depending on the record type (IP address for A/AAAA, domain for CNAME, issuer domain for CAA). The current docblock doesn't explain this, which may confuse users.Apply this diff to improve the documentation:
/** - * @param string $target + * @param string $target The expected value to validate against. For A/AAAA records, this should be an IP address. For CNAME records, a domain name. For CAA records, an issuer domain. + * @param string $type The DNS record type to validate (A, AAAA, CNAME, CAA). + * @param string $dnsServer Optional DNS server to use for validation. */ public function __construct(protected string $target, protected string $type = self::RECORD_CNAME, string $dnsServer = '')Based on learnings
91-93: Consider clarifying the CAA error message.The message "Add new CAA record, or remove all other CAA records" could be more specific about the resolution path.
Consider this alternative:
if ($this->type === self::RECORD_CAA) { - $messages[] = 'Add new CAA record, or remove all other CAA records'; + $messages[] = "Add a CAA record for {$this->target} or remove conflicting CAA records"; }Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/DNS/Validator/DNS.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/DNS/Validator/DNS.php (3)
src/DNS/Client.php (1)
Client(7-68)src/DNS/Message.php (2)
Message(11-208)response(111-139)src/DNS/Message/Record.php (3)
Record(7-527)typeNameToCode(311-314)typeCodeToName(316-319)
🔇 Additional comments (5)
src/DNS/Validator/DNS.php (5)
12-23: LGTM!The class constants are well-defined with clear naming for record types, failure reasons, and a reasonable default DNS server.
24-36: LGTM!The properties are properly typed, and the
recordValuesarray now has the correct PHPDoc annotation addressing the previous PHPStan concern.
98-104: LGTM!The
getLogs()method is straightforward and correctly returns the logs.
106-115: LGTM!The delegation to
isValidWithDNSServeris appropriate.
214-236: LGTM!The
isArray()andgetType()methods correctly indicate this validator expects a single string value.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/DNS/Client.php(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Tests
src/DNS/Client.php
[error] 28-28: Exception: Failed to resolve DNS server hostname: ns1.digitalocean.com
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/DNS/Validator/DNS.php (1)
171-176: Propagate parent validator state back to current instance.When delegating to a parent domain for CAA validation, the current validator's diagnostic properties (
count,recordValues,reason,logs) are not updated. This means callers will seecount = 0and emptyrecordValueseven when the parent domain has CAA records, leading to incorrect error messages fromgetDescription().Apply this diff to propagate the parent validator's state:
// Recursive validation by parent domain $parts = \explode('.', $value); \array_shift($parts); $parentDomain = \implode('.', $parts); $validator = new self($this->target, $this->type, $this->dnsServer); - return $validator->isValid($parentDomain); + $result = $validator->isValid($parentDomain); + + // Propagate diagnostic state from parent validator + $this->recordValues = $validator->recordValues; + $this->count = $validator->count; + $this->logs = $validator->getLogs(); + $this->reason = $validator->reason; + + return $result; }Based on learnings
🧹 Nitpick comments (1)
src/DNS/Validator/DNS.php (1)
101-103: Consider clarifying the CAA error message.The message "Add new CAA record, or remove all other CAA records" is a bit ambiguous. Consider making it more specific to guide users on the resolution path.
Apply this diff for clearer guidance:
if ($this->type === self::RECORD_CAA) { - $messages[] = 'Add new CAA record, or remove all other CAA records'; + $messages[] = "Add a CAA record for {$this->target}, or remove all conflicting CAA records"; }Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
src/DNS/Validator/DNS.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/DNS/Validator/DNS.php (3)
src/DNS/Client.php (3)
Client(7-68)__construct(15-32)query(38-67)src/DNS/Message.php (2)
Message(11-208)response(111-139)src/DNS/Message/Record.php (3)
Record(7-527)typeNameToCode(311-314)typeCodeToName(316-319)
🪛 GitHub Actions: CodeQL
src/DNS/Validator/DNS.php
[error] 46-46: PHPStan error: Found 1 error. Parameter #2 $type of class Utopia\DNS\Message\Question constructor expects int, int|null given. (argument.type). Command run: './vendor/bin/phpstan analyse --level 8 -c phpstan.neon src tests'
🪛 GitHub Actions: Linter
src/DNS/Validator/DNS.php
[error] 1-1: Pint lint failed. 1 style issue detected (PSR-12). Issue example: single_space_around_construct, no_whitespace_in...
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/DNS/Validator/DNS.php (2)
175-176: Propagate parent validator state to maintain diagnostic coherence.When recursively validating the parent domain for CAA records, only the boolean result is returned. This leaves the current instance with
count = 0, emptyrecordValues, and defaultreason, so callers see "missing records" even when the parent domain provides conflicting CAA data (e.g.,issue "digicert.com").Apply this diff to propagate diagnostics:
$validator = new self($this->target, $this->type, $this->dnsServer); - return $validator->isValid($parentDomain); + $result = $validator->isValid($parentDomain); + + $this->recordValues = $validator->recordValues; + $this->count = $validator->count; + $this->logs = $validator->getLogs(); + $this->reason = $validator->reason; + + return $result;
182-202: Fix CAA record processing flow and normalize domain comparisons.The record comparison logic has several issues:
CAA fall-through bug (lines 184-201): After processing CAA records at lines 184-194, the code falls through to line 199, which compares raw
$record->rdata(format"FLAGS TAG VALUE") against$this->target. For CAA records, this comparison will never match and should be skipped.Case sensitivity (lines 192-193, 199-200): DNS domain names are case-insensitive, but comparisons use strict
===. Records likeCNAME Foo.EXAMPLE.COM.or CAA0 issue "LetsEncrypt.org"will fail even when valid.Fragile CAA parsing (line 187): Using
[2] ?? ''afterexplodewith limit 3 masks malformed CAA records instead of handling them explicitly.Apply this diff to fix all three issues:
foreach ($query as $record) { // CAA validation only needs to ensure domain if ($this->type === self::RECORD_CAA) { // Extract domain; comments showcase extraction steps in most complex scenario $rdata = $record->rdata; // 255 issuewild "certainly.com;validationmethods=tls-alpn-01;retrytimeout=3600" - $rdata = \explode(' ', $rdata, 3)[2] ?? ''; // "certainly.com;validationmethods=tls-alpn-01;retrytimeout=3600" + $parts = \explode(' ', $rdata, 3); + if (\count($parts) < 3) { + // Malformed CAA record, skip it + continue; + } + $rdata = $parts[2]; // "certainly.com;validationmethods=tls-alpn-01;retrytimeout=3600" $rdata = \trim($rdata, '"'); // certainly.com;validationmethods=tls-alpn-01;retrytimeout=3600 $rdata = \explode(';', $rdata, 2)[0]; // certainly.com $this->recordValues[] = $rdata; - if ($rdata === $this->target) { + if (\strcasecmp($rdata, $this->target) === 0) { return true; } + // Skip the generic check below for CAA records + continue; } else { $this->recordValues[] = $record->rdata; - } - - if ($record->rdata === $this->target) { - return true; + + // Use case-insensitive comparison for domain-based records (CNAME) + if ($this->type === self::RECORD_CNAME) { + if (\strcasecmp($record->rdata, $this->target) === 0) { + return true; + } + } else { + // Use strict comparison for IP addresses (A/AAAA records) + if ($record->rdata === $this->target) { + return true; + } + } } }
🧹 Nitpick comments (1)
src/DNS/Validator/DNS.php (1)
172-174: Consider using Domain class methods for parent extraction.The manual parent domain extraction using
explode,array_shift, andimplodeis functional but could be simplified. Since you're already using theUtopia\Domains\Domainclass (line 166), check if it provides a method to get the parent domain directly.For example, if
Domainhas agetParent()method:- // Recursive validation by parent domain - $parts = \explode('.', $value); - \array_shift($parts); - $parentDomain = \implode('.', $parts); + $parentDomain = $domain->getParent();Otherwise, the current implementation is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/DNS/Validator/DNS.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/DNS/Validator/DNS.php (4)
src/DNS/Client.php (3)
Client(7-68)__construct(15-32)query(38-67)src/DNS/Message.php (2)
Message(11-208)response(111-139)src/DNS/Message/Question.php (1)
Question(7-51)src/DNS/Message/Record.php (3)
Record(7-527)typeNameToCode(311-314)typeCodeToName(316-319)
Summary by CodeRabbit
New Features
Bug Fixes / Reliability
Tests
Chores