Add Name & CAA validators #25
Conversation
WalkthroughAdds two DNS validator classes and unit tests. src/DNS/Validator/CAA.php introduces class CAA with public property 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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
tests/unit/DNS/Validator/NameTest.php (2)
14-24: Consider adding a test case for maximum domain name length.The validator checks for
Domain::MAX_DOMAIN_NAME_LENat line 20 of Name.php, but there's no test case validating a DNS name at or near the 255-character limit. This would ensure the length boundary is correctly enforced.Example test case to add:
// Test name at exactly 255 characters (max allowed) str_repeat('a', 63) . '.' . str_repeat('b', 63) . '.' . str_repeat('c', 63) . '.' . str_repeat('d', 62) . '.com',
35-46: Consider adding a test case for names exceeding 255 characters.While line 41 tests a label exceeding 63 characters, there's no test for a domain name exceeding the overall 255-character limit (enforced at line 20 of Name.php).
Example test case to add:
str_repeat('a.', 128), // Exceeds 255 charssrc/DNS/Validator/CAA.php (2)
23-29: Fix PHPDoc parameter name.The PHPDoc at line 26 references
@param mixed $value, but the actual parameter name is$data./** * Check if the provided value matches the CAA record format * - * @param mixed $value + * @param mixed $data * @return bool */ public function isValid(mixed $data): bool
47-51: Strengthen flags validation to reject non-integer inputs.
is_numeric()accepts decimal strings like"1.5", which are not valid CAA flags. While the subsequent(int)cast would truncate the value, it's semantically more correct to validate that flags are integer strings.// Check flags is a number - if (!is_numeric($flags)) { + if (!ctype_digit($flags)) { $this->reason = self::FAILURE_REASON_INVALID_FLAGS; return false; }src/DNS/Validator/Name.php (1)
30-53: Minor optimization: Remove redundant length check.Line 39 checks
$len < 1, but this condition is already validated at line 31 withstrlen($label) < 1. This check is redundant.// Check first and last character are alphanumeric $len = strlen($label); if ( - $len < 1 || !ctype_alnum($label[0]) || !ctype_alnum($label[$len - 1]) ) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/DNS/Validator/CAA.php(1 hunks)src/DNS/Validator/Name.php(1 hunks)tests/unit/DNS/Validator/CAATest.php(1 hunks)tests/unit/DNS/Validator/NameTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/DNS/Validator/CAA.php (1)
src/DNS/Validator/Name.php (3)
getDescription(61-64)isArray(77-80)getType(69-72)
tests/unit/DNS/Validator/CAATest.php (1)
src/DNS/Validator/CAA.php (3)
CAA(7-117)isValid(29-83)getDescription(85-92)
tests/unit/DNS/Validator/NameTest.php (1)
src/DNS/Validator/Name.php (3)
Name(8-81)isValid(10-56)getDescription(61-64)
🔇 Additional comments (6)
tests/unit/DNS/Validator/CAATest.php (1)
31-41: LGTM!Good test coverage for invalid cases with proper assertion of both validation results and error messages.
src/DNS/Validator/CAA.php (2)
9-21: LGTM!The constants are well-defined and the public
$reasonproperty provides useful context for validation failures, aligning with the test expectations.
61-116: LGTM!The value validation correctly enforces quoted strings with non-empty content, and the metadata methods (
getDescription(),isArray(),getType()) are properly implemented.src/DNS/Validator/Name.php (3)
10-22: LGTM!Type and length validation are correctly implemented. The use of
Domain::MAX_DOMAIN_NAME_LENensures consistency with DNS standards.
24-27: Verify whether root domain "." should be allowed.The current implementation strips a trailing dot and then validates labels. If the input is just
".", the trimmed result is"", which splits into[""]and fails validation at line 31. This means the root domain is rejected.Confirm whether rejecting the root domain is the intended behavior for this DNS validator.
58-81: LGTM!The metadata methods (
getDescription(),getType(),isArray()) are correctly implemented and follow the expected validator patterns.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/DNS/Validator/CAA.php (1)
61-65: Consider validating tag format per RFC 8659.Per RFC 8659, CAA property tags should contain only alphanumeric characters (US-ASCII letters and digits). The current check only validates non-empty. Standard tags include "issue", "issuewild", and "iodef".
// Check tag is not empty - if (strlen($tag) === 0) { + if (strlen($tag) === 0 || !ctype_alnum($tag)) { $this->reason = self::FAILURE_REASON_INVALID_TAG; return false; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/DNS/Validator/CAA.php(1 hunks)tests/unit/DNS/Validator/CAATest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/DNS/Validator/CAATest.php
🧰 Additional context used
🧬 Code graph analysis (1)
src/DNS/Validator/CAA.php (2)
src/DNS/Validator/DNS.php (1)
DNS(12-200)src/DNS/Validator/Name.php (4)
isValid(10-56)getDescription(61-64)isArray(77-80)getType(69-72)
🪛 GitHub Actions: CodeQL
src/DNS/Validator/CAA.php
[error] 29-29: PHPStan: PHPDoc tag @param references unknown parameter: $value.
🪛 GitHub Actions: Linter
src/DNS/Validator/CAA.php
[error] 1-1: Pint: no_whitespace_in_blank_line violation detected.
🔇 Additional comments (3)
src/DNS/Validator/CAA.php (3)
36-41: Previous feedback addressed.The
explode()now correctly uses limit 3 to handle values containing spaces.
85-92: LGTM!The description method provides clear, contextual error messages based on the failure reason.
101-116: LGTM!The
isArray()andgetType()implementations are consistent with other validators in the codebase.
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/Name.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/DNS/Validator/Name.php (1)
src/DNS/Message/Domain.php (1)
Domain(7-149)
🪛 GitHub Actions: CodeQL
src/DNS/Validator/Name.php
[error] 31-31: PHPStan: Comparison operation "<" between int<1, 63> and 1 is always false.
[error] 39-39: PHPStan: Comparison operation "<" between int<1, 63> and 1 is always false.
🔇 Additional comments (3)
src/DNS/Validator/Name.php (3)
10-56: Solid DNS name validation logic.The core validation logic correctly implements DNS naming rules: proper type checking, length constraints (255 total, 63 per label), per-label character validation (alphanumeric boundaries, hyphens allowed in middle), and empty label rejection. The trailing dot handling for FQDNs is also appropriate.
Note: The validator uses
ctype_alnum()which accepts ASCII alphanumeric characters only. For internationalized domain names (IDNs), ensure input is in Punycode format, not raw Unicode.
58-80: Metadata methods look good.The
getDescription(),getType(), andisArray()methods correctly implement theValidatorinterface, providing appropriate metadata for the DNS name validator.
24-33: Remove misleading comments about root domain support.The comments at lines 18 and 29 suggest root domain "." should be allowed, but this validator intentionally rejects it. Root domain "." is neither tested as valid nor needed for DNS record names (which require at least one label like "example.com").
Clarify the comments to reflect the actual behavior: empty labels are disallowed unconditionally, and root domain "." is not a valid DNS record name.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/DNS/Validator/CAA.php (4)
29-46: Handle surrounding whitespace more robustly for CAA parsingRight now parsing is sensitive to exact single-space separators and no trailing spaces (e.g.
'0 issue "value" 'or multiple spaces before the quoted value will fail). Consider trimming the whole string and/or trimming each part afterexplode()so that benign extra whitespace does not make otherwise valid CAA records invalid.Example:
- $parts = explode(" ", $data, 3); + $data = trim($data); + $parts = explode(' ', $data, 3); + + $parts = array_map('trim', $parts);This keeps the three‑field structure while being more forgiving about spacing.
47-59: Tighten flags validation to accept only integer digits
is_numeric($flags)will accept inputs like'1.5'or'1e3', which then get truncated by(int)$flags. If you intend flags to be strictly integer bytes (0–255), use a digit‑only check instead:- if (!is_numeric($flags)) { + if ($flags === '' || !ctype_digit($flags)) { $this->reason = self::FAILURE_REASON_INVALID_FLAGS; return false; }This avoids surprising coercions while keeping the existing range check.
21-32: Avoid stale$reasonafter successful validationBecause
$reasonis only ever set on failures, a previous failure reason will remain visible even after a laterisValid()call succeeds (if someone callsgetDescription()after a successful validation). To keep the state self‑consistent, you could clear$reasonat the start of validation:public function isValid(mixed $data): bool { + $this->reason = ''; + if (!is_string($data)) { $this->reason = self::FAILURE_REASON_INVALID_FORMAT; return false; }That ensures
getDescription()reflects the outcome of the last validation call.
29-82: Align global function calls with the Name validator styleThe Name validator uses fully‑qualified global functions (
\strlen,\explode, etc.), while this class uses unqualified calls (strlen,explode, …). For consistency and a tiny perf win, you may want to match that style here:- if (!is_string($data)) { + if (!\is_string($data)) { ... - $parts = explode(" ", $data, 3); + $parts = \explode(' ', $data, 3); ... - if (count($parts) !== 3) { + if (\count($parts) !== 3) { ... - if (!is_numeric($flags)) { + if (!\is_numeric($flags)) { ... - if (strlen($tag) === 0) { + if (\strlen($tag) === 0) { ... - if (strlen($value) === 0 || $value[0] !== '"' || $value[strlen($value) - 1] !== '"') { + if (\strlen($value) === 0 || $value[0] !== '"' || $value[\strlen($value) - 1] !== '"') { ... - $value = substr($value, 1, strlen($value) - 2); + $value = \substr($value, 1, \strlen($value) - 2); ... - if (strlen($value) === 0) { + if (\strlen($value) === 0) {Purely a consistency/readability tweak; behavior stays the same.
tests/unit/DNS/Validator/NameTest.php (1)
10-52: Consider using data providers for per‑case test granularityRight now a single failure in the
foreachloops can obscure which specific input failed. Using PHPUnit data providers for valid/invalid cases would give each input its own test case name and easier debugging, while keeping the data arrays essentially the same.Not required, but could improve diagnostics if these tests ever start failing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/DNS/Validator/CAA.php(1 hunks)src/DNS/Validator/Name.php(1 hunks)tests/unit/DNS/Validator/CAATest.php(1 hunks)tests/unit/DNS/Validator/NameTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/DNS/Validator/Name.php
- tests/unit/DNS/Validator/CAATest.php
🧰 Additional context used
🧬 Code graph analysis (2)
src/DNS/Validator/CAA.php (1)
src/DNS/Validator/Name.php (4)
isValid(28-84)getDescription(89-96)isArray(109-112)getType(101-104)
tests/unit/DNS/Validator/NameTest.php (1)
src/DNS/Validator/Name.php (3)
Name(8-113)isValid(28-84)getDescription(89-96)
🔇 Additional comments (1)
tests/unit/DNS/Validator/NameTest.php (1)
10-52: Good coverage of Name validator behaviorThe valid/invalid datasets line up well with the implementation (including edge cases like max label length, empty labels, non‑string input, and description assertions). This is a solid, readable test suite for the Name validator.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/DNS/Validator/Name.php (1)
11-11: Consider including additional record types that use underscores.The current implementation allows underscores only for SRV and TXT records. However, other record types like TLSA also use underscores in service labels (e.g.,
_443._tcp.example.com). While the current list covers common cases, consider expanding this list for broader compatibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/DNS/Validator/Name.php(1 hunks)tests/unit/DNS/Validator/NameTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/DNS/Validator/NameTest.php
🔇 Additional comments (3)
src/DNS/Validator/Name.php (3)
38-84: LGTM! Past PHPStan issues resolved.The validation logic is well-structured and correct:
- Type checking, length constraints (1-255 for name, 1-63 per label), and trailing-dot handling for FQDNs are properly implemented
- Empty labels (including consecutive dots like
example..com) are correctly rejected- Past PHPStan errors about redundant length checks have been resolved in this version
86-93: LGTM! Character validation logic is correct.The implementation properly enforces DNS naming rules:
- First/last characters must be alphanumeric (or underscore for SRV/TXT records)
- Middle characters can include hyphens and underscores (when allowed)
- This correctly handles standard labels and service labels like
_http._tcp
95-122: LGTM! Metadata methods implemented correctly.The validator interface methods are properly implemented:
getDescription()provides specific failure reasons with a sensible fallbackgetType()andisArray()return appropriate values for string validation
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/DNS/Validator/Name.php (1)
64-67: Use more appropriate failure reason for empty labels.When an empty label is detected (e.g., from
example..comor.example.com), the code currently sets the failure reason toINVALID_LABEL_CHARACTERS_*. However, an empty label is fundamentally a length issue (0 characters), not a character composition issue.Apply this diff:
foreach ($labels as $label) { if ($label === '') { - $this->reason = $isUnderscoreAllowed ? self::FAILURE_REASON_INVALID_LABEL_CHARACTERS_WITH_UNDERSCORE : self::FAILURE_REASON_INVALID_LABEL_CHARACTERS_WITHOUT_UNDERSCORE; + $this->reason = self::FAILURE_REASON_INVALID_LABEL_LENGTH; return false; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/DNS/Validator/CAA.php(1 hunks)src/DNS/Validator/Name.php(1 hunks)tests/unit/DNS/Validator/NameTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/DNS/Validator/NameTest.php
🧰 Additional context used
🧬 Code graph analysis (2)
src/DNS/Validator/CAA.php (2)
src/DNS/Validator/DNS.php (1)
DNS(12-200)src/DNS/Validator/Name.php (4)
isValid(40-87)getDescription(100-107)isArray(120-123)getType(112-115)
src/DNS/Validator/Name.php (2)
src/DNS/Message/Domain.php (1)
Domain(7-149)src/DNS/Message/Record.php (1)
Record(10-542)
🔇 Additional comments (8)
src/DNS/Validator/Name.php (4)
1-32: LGTM: Well-structured validator class setup.The class declaration, constants, properties, and constructor are well-organized. The failure reason constants provide clear, descriptive error messages, and the record-type-specific underscore handling is a nice touch for SRV and TXT records.
69-87: LGTM: Solid label validation logic.The per-label validation correctly enforces:
- Maximum length of 63 characters
- Character-level validation through the helper method
- Appropriate failure reasons based on whether underscores are allowed
The character-by-character validation loop is efficient and correctly identifies first/last positions for stricter validation.
89-95: LGTM: Character validation helper is correct.The helper method correctly enforces DNS label character rules from RFC 1035, with proper support for underscore-prefixed labels in SRV and TXT records. The first/last position restrictions (alphanumeric only, except underscore when allowed) and middle position allowances (including hyphens) are properly implemented.
100-123: LGTM: Standard validator interface methods.The
getDescription(),getType(), andisArray()methods correctly implement the validator interface pattern, consistent with other validators in the codebase (e.g., DNS and CAA validators).src/DNS/Validator/CAA.php (4)
1-21: LGTM: Clean validator class setup.The class constants for flag boundaries and failure reasons are well-defined and descriptive. The structure follows the same pattern as the Name validator, providing good consistency across the codebase.
23-28: LGTM: PHPDoc correctly matches method signature.The PHPDoc
@param mixed $datacorrectly matches the method parameter name. This appears to have been fixed from a previous review.
29-83: LGTM: Comprehensive CAA record validation.The validation logic is thorough and correct:
- Uses
explode(..., 3)to properly handle values with embedded spaces (fixed from previous review)- Correctly validates flags as numeric and within range [0, 255]
- Ensures tag is non-empty
- Properly validates quoted value format and checks inner content is non-empty
- Handles edge cases like
""correctly (caught as empty after quote stripping)
85-116: LGTM: Standard validator interface implementation.The
getDescription(),isArray(), andgetType()methods correctly implement the validator interface, maintaining consistency with other validators in the codebase.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/unit/DNS/Validator/NameTest.php (1)
32-35: Add test coverage for underscore positions in all allowed locations.The current test only verifies that underscores are valid at the start of a label for SRV type. According to the implementation at lines 96-99 in
src/DNS/Validator/Name.php, underscores should be allowed in any position (first, middle, or last) when the record type permits them.Consider adding test cases for:
- Underscore in middle of label:
'exa_mple.com'- Underscore at end of label:
'example_.com'- TXT type with underscores (currently only SRV is tested)
Apply this diff:
// Type that allows underscores in name $validator = new Name(Record::TYPE_SRV); $this->assertTrue($validator->isValid('example._tcp.com'), "Expected valid: example._tcp.com"); + $this->assertTrue($validator->isValid('exa_mple.com'), "Expected valid: exa_mple.com"); + $this->assertTrue($validator->isValid('example_.com'), "Expected valid: example_.com"); + + // TXT type also allows underscores + $validator = new Name(Record::TYPE_TXT); + $this->assertTrue($validator->isValid('_dmarc.example.com'), "Expected valid: _dmarc.example.com"); + $this->assertTrue($validator->isValid('exa_mple.com'), "Expected valid: exa_mple.com"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/DNS/Validator/Name.php(1 hunks)tests/unit/DNS/Validator/NameTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/unit/DNS/Validator/NameTest.php (1)
src/DNS/Validator/Name.php (3)
Name(9-129)isValid(40-92)getDescription(105-112)
src/DNS/Validator/Name.php (2)
src/DNS/Message/Domain.php (1)
Domain(7-149)src/DNS/Message/Record.php (1)
Record(10-542)
🔇 Additional comments (6)
tests/unit/DNS/Validator/NameTest.php (1)
37-82: Well-structured test coverage for invalid inputs.The test method properly validates both the
isValid()return value and thegetDescription()error messages for different record types. The distinction between CNAME (no underscores) and TXT (underscores allowed) is correctly tested with appropriate error message constants.src/DNS/Validator/Name.php (5)
1-32: LGTM! Clean class structure and well-defined constants.The class properly extends
Validatorand defines clear failure reason constants. The record type parameter enables context-specific validation (e.g., underscore handling for SRV/TXT records).
40-60: LGTM! Proper input validation and edge case handling.The method correctly validates the input type, enforces DNS name length constraints (1-255 characters), and handles the special case of
'@'for zone origin references. The length check at line 52 is appropriate at this stage.
62-92: LGTM! Label validation logic is correct and past issues resolved.The label-by-label validation properly:
- Handles absolute FQDNs with trailing dots
- Detects empty labels
- Enforces the 63-character label length limit
- Validates characters based on position and record type
The past review comments about redundant length checks have been resolved through the current code structure that separates empty-label validation (line 69) from length validation (line 74).
94-100: LGTM! Correct character validation per DNS RFC rules.The method properly enforces:
- First/last characters: alphanumeric or underscore (when allowed)
- Middle characters: alphanumeric, hyphen, or underscore (when allowed)
This correctly implements the RFC constraint that hyphens cannot appear at label boundaries.
105-128: LGTM! Utility methods properly implement the Validator interface.The methods correctly:
- Return specific validation failure reasons via
getDescription()- Declare the validator type as string via
getType()- Indicate non-array validation via
isArray()
Related SER-650
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.