Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughPhotoUrlRule now accepts an optional DNS resolver, resolves hostnames to A/AAAA records, and validates resolved IPs for private/reserved and localhost addresses via new helpers. A migration bumps the stored version to 7.5.2 and clears cache; tests were updated to inject mocked DNS responses and adjust assertions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 650bc952-8b6a-44ac-ba0b-2aa7d8c5e554
📒 Files selected for processing (4)
app/Rules/PhotoUrlRule.phpdatabase/migrations/2026_03_22_094636_bump_version070502.phptests/Unit/Rules/PhotoUrlRuleTest.phpversion.md
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/Rules/PhotoUrlRule.php (1)
94-109: This only hardens validation, not the actual connect path.
$resolved_ipsare discarded aftervalidate(), and the importer still receives the original hostname. Please verify the fetch path pins one of these vetted IPs or re-checks the peer after connect; otherwise a low-TTL rebind can still hit an internal address after this rule passes.tests/Unit/Rules/PhotoUrlRuleTest.php (1)
188-217: Add an AAAA regression here too.The new rule has separate
DNS_AAAAand IPv6-localhost branches, but the added security tests only driveDNS_A. OneAAAA => [['ipv6' => '::1']]case would keep the IPv6 path from regressing unnoticed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2bb147a9-9240-4ac9-bf23-123970dd346f
📒 Files selected for processing (2)
app/Rules/PhotoUrlRule.phptests/Unit/Rules/PhotoUrlRuleTest.php
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/Rules/PhotoUrlRule.php (1)
137-150: Consider defensive array key access.The DNS record arrays are accessed directly via
$record['ip']and$record['ipv6']. Whiledns_get_recordnormally returns these keys for valid A/AAAA records, adding defensive checks would prevent potential "undefined array key" notices if the DNS response is malformed or incomplete.🛡️ Proposed defensive fix
$a_records = call_user_func($this->dns_get_record, $host, DNS_A); if ($a_records !== false) { foreach ($a_records as $record) { - $ips[] = $record['ip']; + if (isset($record['ip'])) { + $ips[] = $record['ip']; + } } } // Resolve AAAA records (IPv6). $aaaa_records = call_user_func($this->dns_get_record, $host, DNS_AAAA); if ($aaaa_records !== false) { foreach ($aaaa_records as $record) { - $ips[] = $record['ipv6']; + if (isset($record['ipv6'])) { + $ips[] = $record['ipv6']; + } } }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4c15f2cd-5ddd-4f5d-9c5b-de84436e36c5
📒 Files selected for processing (1)
app/Rules/PhotoUrlRule.php
Codecov Report❌ Patch coverage is 🚀 New features to boost your workflow:
|
2 participants
Summary by CodeRabbit
Security Improvements
Version Update
Tests