Fix: improve encoder functions according to RFC#8
Conversation
|
""" WalkthroughThis change refactors and enhances DNS record encoding in the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant Zone
Client->>Server: Send DNS query
Server->>Zone: (if needed) Parse zone file line (tokenize)
Zone-->>Server: Return parsed tokens
Server->>Server: Encode answer using encodeIP/encodeIPv6/encodeDomain/encodeText/encodeMx/encodeSrv/encodeCAA
Server-->>Client: Send DNS response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/DNS/Zone.php (1)
363-410: Enhanced tokenization implementation looks solid and RFC 1035 compliant.The new implementation correctly handles quoted strings and escape sequences as specified in RFC 1035. The state machine approach with
$inQuoteand$escapeflags properly manages the parsing logic.However, consider adding validation for edge cases:
protected function tokenize(string $line): array { $tokens = []; $current = ''; $inQuote = false; $escape = false; for ($i = 0, $len = strlen($line); $i < $len; $i++) { $c = $line[$i]; if ($escape) { $current .= $c; $escape = false; continue; } if ($c === '\\') { $escape = true; continue; } if ($inQuote) { if ($c === '"') { $inQuote = false; $tokens[] = $current; $current = ''; } else { $current .= $c; } } else { if ($c === '"') { $inQuote = true; } elseif ($c === ' ' || $c === "\t") { if ($current !== '') { $tokens[] = $current; $current = ''; } } else { $current .= $c; } } } + // Validate state at end of parsing + if ($inQuote) { + throw new \InvalidArgumentException('Unterminated quoted string in line'); + } + if ($escape) { + throw new \InvalidArgumentException('Escape character at end of line'); + } if ($current !== '') { $tokens[] = $current; } return $tokens; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/DNS/Server.php(1 hunks)src/DNS/Zone.php(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Unit & E2E
🔇 Additional comments (4)
src/DNS/Server.php (4)
340-348: IPv4 encoding implementation is correct and RFC 1035 compliant.The method properly validates the binary IP address length and uses the correct packing format with TTL and data length.
357-365: IPv6 encoding implementation is correct and RFC 3596 compliant.The method properly validates the 16-byte IPv6 address length and follows the same pattern as IPv4 encoding.
400-411: TXT record encoding implementation is RFC 1035 compliant.The method correctly handles text chunking into 255-byte segments with proper length prefixes. This follows the TXT record format specification.
283-293: Record type encoding integration looks well-structured.The match expression properly maps each record type to its corresponding encoding method, and the method signatures align correctly with the usage patterns.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/DNS/ZoneTest.php (2)
341-352: Fix indentation inconsistency.The method uses 5-space indentation while the rest of the file consistently uses 4-space indentation.
- public function testImportTxtWithSpecialChars() - { - $zone = new Zone(); - $zonefile = <<<ZONE + public function testImportTxtWithSpecialChars() + { + $zone = new Zone(); + $zonefile = <<<ZONE @ 3600 IN TXT "v=DMARC1; p=none; rua=mailto:jon@snow.got; ruf=mailto:jon@snow.got; fo=1;" ZONE; - $records = $zone->import('example.com', $zonefile); - $this->assertCount(1, $records); - $rec = $records[0]; - $this->assertEquals('TXT', $rec->getTypeName()); - $this->assertEquals('v=DMARC1; p=none; rua=mailto:jon@snow.got; ruf=mailto:jon@snow.got; fo=1;', trim($rec->getRdata(), '"')); - } + $records = $zone->import('example.com', $zonefile); + $this->assertCount(1, $records); + $rec = $records[0]; + $this->assertEquals('TXT', $rec->getTypeName()); + $this->assertEquals('v=DMARC1; p=none; rua=mailto:jon@snow.got; ruf=mailto:jon@snow.got; fo=1;', trim($rec->getRdata(), '"')); + }
344-346: Consider testing additional edge cases for comprehensive coverage.While the current test covers DMARC policy strings well, consider adding test cases for other TXT record edge cases to ensure the improved tokenization handles all scenarios mentioned in the RFC improvements.
Additional test cases could include:
- TXT records with escaped quotes:
"some \"quoted\" text"- Multiple quoted strings:
"first part" "second part"- Mixed quoted and unquoted content
- TXT records with backslash escapes
Would you like me to generate additional test methods for these edge cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/DNS/Zone.php(2 hunks)tests/DNS/ZoneTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/DNS/Zone.php
🔇 Additional comments (1)
tests/DNS/ZoneTest.php (1)
341-352: Excellent test coverage for TXT record parsing improvements.This test effectively validates the enhanced tokenization of quoted TXT records containing special characters like semicolons, colons, equals signs, and email addresses. The test logic correctly verifies both the record type and the RDATA content with proper quote handling.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/DNS/Zone.php (1)
404-447: Handle incomplete escape sequences at end of line.The tokenizer correctly implements RFC 1035 quoted string parsing, but doesn't handle the edge case where a backslash appears at the end of the line without a following character to escape.
Consider adding validation for incomplete escape sequences:
} if ($current !== '') { $tokens[] = $current; } + // Handle incomplete escape at end of line + if ($escape) { + // Log warning or throw exception for malformed input + } return $tokens;Otherwise, the implementation correctly handles quoted strings and escape sequences per RFC 1035.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/DNS/Zone.php(6 hunks)tests/DNS/ZoneTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/DNS/ZoneTest.php
🔇 Additional comments (3)
src/DNS/Zone.php (3)
232-239: LGTM: Correct TXT record data handling per RFC 1035.The concatenation without spaces for TXT records is correct according to RFC 1035, which specifies that multiple character strings in TXT records should be concatenated together.
285-289: LGTM: Proper TXT record encoding with correct escape sequence.The escaping order (backslashes first, then quotes) is correct to prevent double-escaping, and wrapping in quotes follows DNS zone file standards.
293-300: LGTM: Improved output formatting for consistency.The logic correctly removes trailing empty lines and ensures exactly one newline at the end, improving the consistency of exported zone files.
2 participants
Summary by CodeRabbit