Skip to content

Fix: return name assigned to the answer if different #11

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lohanidamodar merged 8 commits into from
Oct 14, 2025
Merged

Fix: return name assigned to the answer if different #11

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
73e90c0
Fix: return name assigned to the answer instead of from question if i…
lohanidamodar Oct 6, 2025
11dcca4
fix and add test
lohanidamodar Oct 6, 2025
329ff02
revert encoder change
lohanidamodar Oct 6, 2025
4ca4c96
handle SOA type differently
lohanidamodar Oct 6, 2025
69efa06
fix dns server
lohanidamodar Oct 14, 2025
2ed8db6
Fix test
lohanidamodar Oct 14, 2025
4037778
fix email
lohanidamodar Oct 14, 2025
78e5a1f
fix
lohanidamodar Oct 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions src/DNS/Resolver/Memory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,25 @@ public function resolve(array $question): array
return $this->records[$key];
}

// Special handling for SOA records: if not found at the exact domain,
// walk up the domain hierarchy to find the zone apex SOA record
if ($question['type'] === 'SOA') {
$domain = $question['name'];
$parts = explode('.', $domain);

// Try each parent domain level, starting from immediate parent
// e.g., for "dev.sub.example.com", try "sub.example.com", then "example.com"
while (count($parts) > 1) {
array_shift($parts); // Remove leftmost subdomain
$parentDomain = implode('.', $parts);
$parentKey = $parentDomain . '_SOA';

if (\array_key_exists($parentKey, $this->records)) {
return $this->records[$parentKey];
}
}
}

return [];
}

Expand Down
30 changes: 28 additions & 2 deletions src/DNS/Server.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,8 +291,27 @@ public function start(): void

// Add answers section
foreach ($answers as $answer) {
$response .= chr(192) . chr(12); // 192 indicates this is pointer, 12 is offset to question.
// Pack the answer's type, not the question type
$answerDomain = $answer->getName();
$type = $answer->getTypeName();

Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
// For SOA records, check if we need to encode a different domain name
if ($type === 'SOA' && $answerDomain !== $domain) {
// Encode the full domain name since it's different from question (apex vs subdomain)
$labels = explode('.', rtrim($answerDomain, '.'));
foreach ($labels as $label) {
$labelLength = strlen($label);
if ($labelLength > self::MAX_LABEL_LEN) {
throw new \Exception("Label too long in SOA domain: {$label}");
}
$response .= chr($labelLength) . $label;
}
$response .= chr(0); // End of domain name
} else {
// Use compression pointer for all other cases
$response .= chr(192) . chr(12); // 192 indicates this is pointer, 12 is offset to question.
}
Comment on lines +298 to +312

@coderabbitai coderabbitai Bot Oct 6, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Extend logic to all record types and use a helper method.

The condition restricts this behavior to SOA records only, but the PR objectives state "Response uses the DNS name from the answer if it's different than the question" without limiting to SOA. Records like CNAME, NS, and others can also have answer domains that differ from the question domain.

Additionally, the inline domain encoding (lines 300-308) duplicates logic already present in the encodeDomain method. According to the AI summary, a new protected method encodeDomainName(string $domain): string was supposed to be introduced to handle domain encoding without TTL/length prefix, but it's missing from this file.

Apply this diff to generalize the logic and extract the helper method:

+    /**
+     * Encode just the domain name part (without TTL and length prefix)
+     * Used for domain names in answer sections
+     *
+     * @param string $domain
+     * @return string
+     */
+    protected function encodeDomainName(string $domain): string
+    {
+        $labels = explode('.', rtrim($domain, '.'));
+        $result = '';
+        $totalLength = 0;
+        foreach ($labels as $label) {
+            $labelLength = strlen($label);
+            if ($labelLength === 0) {
+                throw new \Exception("Empty label in domain: '{$domain}'");
+            }
+            if ($labelLength > self::MAX_LABEL_LEN) {
+                throw new \Exception("Label too long in domain: {$label}");
+            }
+            $result .= chr($labelLength) . $label;
+            $totalLength += 1 + $labelLength;
+        }
+        $result .= chr(0);
+        $totalLength += 1;
+        if ($totalLength > self::MAX_DOMAIN_NAME_LEN) {
+            throw new \Exception("Encoded domain name too long: {$domain}");
+        }
+        return $result;
+    }

Then update the answer encoding logic to use the helper and remove the SOA restriction:

         $answerDomain = $answer->getName();
         $type = $answer->getTypeName();
         
-        // For SOA records, check if we need to encode a different domain name
-        if ($type === 'SOA' && $answerDomain !== $domain) {
-            // Encode the full domain name since it's different from question (apex vs subdomain)
-            $labels = explode('.', rtrim($answerDomain, '.'));
-            foreach ($labels as $label) {
-                $labelLength = strlen($label);
-                if ($labelLength > self::MAX_LABEL_LEN) {
-                    throw new \Exception("Label too long in SOA domain: {$label}");
-                }
-                $response .= chr($labelLength) . $label;
-            }
-            $response .= chr(0); // End of domain name
+        // Check if we need to encode a different domain name
+        if ($answerDomain !== $domain) {
+            // Encode the full domain name since it's different from question
+            $response .= $this->encodeDomainName($answerDomain);
         } else {
             // Use compression pointer for all other cases
             $response .= chr(192) . chr(12); // 192 indicates this is pointer, 12 is offset to question.
         }
🤖 Prompt for AI Agents 
In src/DNS/Server.php around lines 298 to 312, the code currently encodes the
full answer domain only for SOA records and inlines label encoding instead of
using a helper; remove the SOA-only restriction so any record type uses the
answer's name when $answerDomain !== $domain, and refactor the inline encoding
into a new protected method encodeDomainName(string $domain): string that: trims
the trailing dot, splits labels, validates each label against
self::MAX_LABEL_LEN throwing an Exception on overflow, concatenates
chr(labelLength) . label for each label and appends chr(0) as terminator, and
returns the encoded string (no TTL/length prefix); then replace the inline loop
with a call to $this->encodeDomainName($answerDomain) and keep the compression
pointer (chr(192) . chr(12)) for the else branch.


// Pack the answer's type and class
$response .= pack('nn', $answer->getType(), $classByte);
Comment on lines +294 to 315

@coderabbitai coderabbitai Bot Oct 6, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

🧩 Analysis chain

Verify functionality with CNAME chains and NS records.

While the implementation logic is sound, please verify that the changes work correctly for common cases where answer domains differ from the question:

  1. CNAME chains (e.g., www.example.com → cdn.example.com → 192.0.2.1)
  2. NS records (e.g., querying example.com NS returns ns1.example.com)
  3. Mixed answer sets where some answers match the question domain and others don't

Run the following script to check the test coverage for these scenarios:

🏁 Script executed:

#!/bin/bash
# Description: Verify test coverage for CNAME, NS records and domain name encoding

# Check for existing tests of CNAME chains
echo "=== CNAME chain tests ==="
rg -n -A5 -B2 --type=php 'CNAME' tests/

echo -e "\n=== NS record tests ==="
rg -n -A5 -B2 --type=php '\bNS\b' tests/

echo -e "\n=== Domain name encoding tests ==="
rg -n -A5 -B2 --type=php 'encodeDomainName' tests/

Length of output: 9394

Add tests for pointer compression and mixed answer sets

  • ClientTest covers CNAME/NS queries but doesn’t exercise encodeDomainName vs pointer compression in Server.php
  • Add tests for:
    • answerDomain===questionDomain (pointer compression)
    • answerDomain≠questionDomain (full domain encoding)
    • mixed answer sets combining matching and non-matching domains
🤖 Prompt for AI Agents 
In src/DNS/Server.php around lines 294 to 306, tests are missing that validate
pointer compression vs full domain encoding; add unit tests that (1) simulate a
query where an answer's name equals the question name and assert the serialized
response contains the two-byte pointer sequence 0xC0 0x0C at the answer name
position, (2) simulate a query where an answer's name differs from the question
name and assert the response contains the encoded label sequence for that domain
(no 0xC0 0x0C), and (3) create a mixed-answer response containing both matching
and non-matching answer names and assert the response contains both the pointer
and the full-encoded domain respectively; reuse existing ClientTest setup or
helper methods to build queries and parse responses, and assert on raw response
bytes and correct packing of type/class following the name bytes.

/**
* @var string $type
Expand Down Expand Up @@ -338,6 +357,13 @@ public function start(): void
}
}

/**
* Encode just the domain name part (without TTL and length prefix)
* Used for domain names in answer sections
*
* @param string $domain
* @return string
*/
Comment on lines +360 to +366

@coderabbitai coderabbitai Bot Oct 6, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Orphaned documentation comment.

This doc comment has no corresponding method implementation. The method declaration and body are missing, though line 367 begins a different doc comment for the resolve method.

This appears to be an incomplete implementation of the encodeDomainName helper method mentioned in the AI summary.

This is addressed by the diff in the previous review comment, which adds the complete encodeDomainName method implementation before line 360.

🤖 Prompt for AI Agents 
In src/DNS/Server.php around lines 360 to 366 there is an orphaned docblock for
an encodeDomainName helper that has no method implementation; add the missing
private function encodeDomainName(string $domain): string immediately before the
docblock range so the comment matches code. Implement it to split the domain
into labels, prefix each label with its length byte, concatenate them and append
a zero byte terminator, and ensure it returns the binary string (handle
empty/root domain as "\0"). Keep visibility private and add minimal input
validation (trim trailing dot) to match usage from the answer encoding logic.

/**
* Resolve domain name to IP by record type
*
Expand Down
4 changes: 1 addition & 3 deletions tests/DNS/ClientTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,11 +181,9 @@ public function testSOARecords(): void

$this->assertCount(1, $records);
$rdata = $records[0]->getRdata();
$this->assertEquals('appwrite.io', $records[0]->getName());
$this->assertStringContainsString('ns1.dev2.appwrite.io.', $rdata);
$this->assertStringContainsString('admin.dev2.appwrite.io.', $rdata);
$this->assertStringContainsString('2025011802', $rdata);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

$records = $this->client->query('dev3.appwrite.io', 'SOA');
$this->assertCount(0, $records);
}
}
4 changes: 0 additions & 4 deletions tests/DNS/ServerMemory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,10 +111,6 @@
'ttl' => 3600
]);

$resolver->addRecord('dev2.appwrite.io', 'SOA', [
'value' => 'ns1.dev2.appwrite.io. admin.dev2.appwrite.io. 2025011802 14400 7200 2419200 3600'
]);

$dns = new Server($server, $resolver);
$dns->setDebug(false);

Expand Down
Loading