Remove hostname check on domain validate #1
Conversation
|
Warning Rate limit exceeded@abnegate has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 11 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
Walkthroughcomposer.json: relaxed PHP requirement from ">=8.1" to ">=8.0" and updated dev dependencies for phpunit to "11." and laravel/pint to "1.". src/Validator/Domain.php: constructor changed to Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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
🧹 Nitpick comments (1)
src/Validator/Domain.php (1)
16-23:$allowUnderscoreconstructor parameter is currently a no‑opThe new constructor stores
$allowUnderscorebutisValid()never reads it, so passing a second argument has no effect on validation behavior.To avoid a misleading API surface:
- Either implement the behavior that
allowUnderscoreis supposed to control (e.g., relaxing the label character rules whentrue), or- Remove the parameter/property for now and reintroduce it when the behavior is actually wired up.
📜 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/Validator/Domain.php(2 hunks)src/Validator/IP.php(0 hunks)
💤 Files with no reviewable changes (1)
- src/Validator/IP.php
🧰 Additional context used
🪛 GitHub Actions: Linter
composer.json
[error] 1-1: Root composer.json requires PHP extension ext-xdebug * but it is missing from your system. Install or enable PHP's xdebug extension.
[error] 1-1: Your PHP version (8.1.33) does not satisfy the required PHP version (>=8.2) for multiple dependencies (e.g., laravel/pint, phpunit/*, etc.).
🪛 GitHub Actions: Tests
composer.json
[error] 1-1: composer install --no-progress --prefer-dist failed: laravel/pint (v1.25.1) requires PHP ^8.2.0, but PHP 8.1.33 is in use.
[error] 1-1: composer install --no-progress --prefer-dist failed: phpunit/php-code-coverage (11.0.11) requires PHP >=8.2, but PHP 8.1.33 is in use.
[error] 1-1: composer install --no-progress --prefer-dist failed: phpunit/php-file-iterator is locked to version 5.1.0 and an update of this package was not requested.
🔇 Additional comments (1)
src/Validator/Domain.php (1)
75-75: Verification confirms this is an intentional behavioral changeThe removal of
FILTER_FLAG_HOSTNAMEdoes broaden validation to accept a wider set of inputs (RFC-based only, without hostname restrictions). However, this is intentional:
- Commit message (112ac24): "Remove hostname check on domain validate" — explicitly states the purpose
- Docblock accuracy: The class docblock correctly describes RFC-based validation (RFC 1034, 1035, 952, 1123, 2732, 2181), which is the behavior after removing
FILTER_FLAG_HOSTNAME- Related change: The constructor was simultaneously updated to add
$allowUnderscore = false, indicating broader domain acceptance is by designThe semantic change is documented and the docblock remains accurate for the new behavior. No further action needed.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 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 (6)
.github/workflows/codeql-analysis.yml(1 hunks).github/workflows/lint.yml(1 hunks).github/workflows/test.yml(1 hunks)composer.json(1 hunks)src/Validator/Domain.php(2 hunks)tests/Validator/DomainTest.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Validator/Domain.php
- composer.json
🧰 Additional context used
🧬 Code graph analysis (1)
tests/Validator/DomainTest.php (1)
src/Validator/Domain.php (2)
Domain(14-144)isValid(65-119)
🔇 Additional comments (4)
.github/workflows/codeql-analysis.yml (1)
17-17: Verify PHP 8.4 availability and compatibility with project dependencies.Same concern as the lint workflow: ensure PHP 8.4 is stable and all dependencies are compatible. Consider testing against PHP 8.0 (the minimum supported version) as well to catch compatibility issues early.
.github/workflows/test.yml (1)
17-17: Verify PHP 8.4 availability and compatibility with project dependencies.Same concern as other workflows: ensure PHP 8.4 is stable and compatible. This is particularly important for the test workflow, as it validates the actual functionality of the codebase.
tests/Validator/DomainTest.php (1)
152-177: Well-structured test demonstrating strict vs. permissive validation.This test clearly demonstrates the difference between hostname-strict and permissive validation modes, covering edge cases (underscores, spaces, dashes) and confirming that compliant domains work in both modes.
.github/workflows/lint.yml (1)
17-17: Verification confirms PHP 8.4 availability and all dev dependency compatibility.PHP 8.4 is stable and available, with PHPUnit 11 requiring PHP >=8.2, Laravel Pint 1.* requiring PHP ^8.2.0, and PHPStan 1.* requiring PHP ^7.2|^8.0. All dependencies are compatible with PHP 8.4.
The workflow configuration is sound. However, consider the original review suggestion to add testing against the minimum supported version (8.0) in a separate workflow matrix to catch potential compatibility regressions:
matrix: php: ['8.0', '8.4']This would ensure backward compatibility with the
>=8.0constraint in composer.json while validating the latest PHP version works correctly.
| $this->assertEquals(true, $permissiveValidator->isValid('api-.appwrite.io')); // Dash at end of label | ||
| $this->assertEquals(true, $permissiveValidator->isValid('api.-appwrite.io')); // Dash at start of label | ||
| $this->assertEquals(true, $permissiveValidator->isValid('app write.io')); // Space in domain | ||
| $this->assertEquals(true, $permissiveValidator->isValid(' appwrite.io')); // Leading space | ||
| $this->assertEquals(true, $permissiveValidator->isValid('appwrite.io ')); // Trailing space | ||
| $this->assertEquals(true, $permissiveValidator->isValid('-appwrite.io')); // Leading dash | ||
| } |
There was a problem hiding this comment.
Permissive mode allows RFC-violating domain patterns.
While these assertions may technically pass with FILTER_VALIDATE_DOMAIN (no FILTER_FLAG_HOSTNAME), domains with:
- Dashes at the end of a label (line 81:
api-.appwrite.io) - Dashes at the start of a label (line 82:
api.-appwrite.io) - Leading dashes (line 87:
-appwrite.io) - Internal or boundary spaces (lines 83-86)
violate RFC 1035 and RFC 952 hostname specifications. Consider whether permissive mode should allow these patterns, as they may cause issues with DNS resolution and other domain-dependent systems.
Would you like me to propose stricter validation rules that filter out these edge cases even in permissive mode while still allowing underscores?
| // Note: These are actually allowed by FILTER_VALIDATE_DOMAIN without FILTER_FLAG_HOSTNAME | ||
| // but might be unexpected: | ||
| $this->assertEquals(true, $permissiveValidator->isValid('exam ple.com')); // Space in domain | ||
| $this->assertEquals(true, $permissiveValidator->isValid('example@.com')); // @ character | ||
| $this->assertEquals(true, $permissiveValidator->isValid('example#.com')); // # character | ||
| $this->assertEquals(true, $permissiveValidator->isValid('http://example.com')); // Protocol | ||
| $this->assertEquals(true, $permissiveValidator->isValid('example.com:8080')); // Port | ||
| $this->assertEquals(true, $permissiveValidator->isValid('example.com/path')); // Path | ||
| } |
There was a problem hiding this comment.
Test organization issue: valid assertions in "testInvalidDomains" method.
Lines 115-120 assert domains as true (valid) within a method named testInvalidDomainsWithHostnamesFalse. This is confusing. These assertions document that permissive mode allows:
- URLs with protocols (
http://example.com) - Ports (
example.com:8080) - Paths (
example.com/path) - Special characters (
@,#)
Security concern: Accepting these patterns as "valid domains" could introduce vulnerabilities:
- SSRF attacks: If domain validation is used before making HTTP requests
- Injection attacks: Special characters in domains used in SQL, shell commands, etc.
- Misuse: These are URLs, not domains
Consider either:
- Moving these assertions to a separate method like
testUnexpectedPermissiveBehavior()with documentation - Adding additional validation to reject these patterns even in permissive mode
- Documenting this behavior prominently in the Domain validator
Summary by CodeRabbit
Chores
Bug Fixes
Improvements
Tests