Conversation
📝 Walkthrough""" WalkthroughThis change introduces support for validating CAA (Certification Authority Authorization) DNS records in the domain verification process. Two new environment variables are added: Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ 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). (18)
✨ 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. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
commit: |
✨ Benchmark results
⚡ Benchmark Comparison
|
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
app/controllers/api/proxy.php (1)
289-300: Fix hardcoded CA name in error message.Same issue as in the certificates worker - the error message should use the environment variable value instead of hardcoding "certainly.com".
Apply this diff:
- throw new Exception(Exception::RULE_VERIFICATION_FAILED, 'Domain verification failed because CAA records do not allow certainly.com to issue certificates.'); + throw new Exception(Exception::RULE_VERIFICATION_FAILED, 'Domain verification failed because CAA records do not allow ' . System::getEnv('_APP_DOMAIN_TARGET_CAA', '') . ' to issue certificates.');
🧹 Nitpick comments (2)
tests/unit/Network/Validators/DNSTest.php (1)
69-69: Fix typo in comment.Apply this diff:
- // Valid becasue no CAA record configured + // Valid because no CAA record configuredsrc/Appwrite/Network/Validator/DNS.php (1)
24-26: Consider dependency injection for better testabilityThe DNS client is created directly in the validation method, making it difficult to test. Consider injecting it through the constructor.
+ private Client $dnsClient; + /** * @param string $target + * @param Client|null $dnsClient */ - public function __construct(protected string $target, protected string $type = self::RECORD_CNAME) + public function __construct( + protected string $target, + protected string $type = self::RECORD_CNAME, + ?Client $dnsClient = null + ) { + $this->dnsClient = $dnsClient ?? new Client(System::getEnv('_APP_DNS', '8.8.8.8')); }Then update the isValid method:
- $dnsServer = System::getEnv('_APP_DNS', '8.8.8.8'); - $dns = new Client($dnsServer); - try { - $query = $dns->query($value, $this->type); + $query = $this->dnsClient->query($value, $this->type);This would also allow reusing the same DNS client instance in recursive calls.
Also applies to: 56-58, 78-79
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
.env(1 hunks)app/config/variables.php(1 hunks)app/controllers/api/console.php(1 hunks)app/controllers/api/proxy.php(1 hunks)app/views/install/compose.phtml(4 hunks)composer.json(1 hunks)docker-compose.yml(4 hunks)src/Appwrite/Network/Validator/DNS.php(2 hunks)src/Appwrite/Platform/Workers/Certificates.php(1 hunks)src/Appwrite/Utopia/Response/Model/ConsoleVariables.php(1 hunks)tests/e2e/Services/Console/ConsoleConsoleClientTest.php(1 hunks)tests/unit/Network/Validators/DNSTest.php(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: stnguyen90
PR: appwrite/appwrite#10119
File: app/controllers/api/account.php:1226-1232
Timestamp: 2025-07-08T01:20:14.364Z
Learning: In Appwrite, `_APP_DOMAIN` is a required environment variable that must always be set for the system to function properly.
📚 Learning: in appwrite, `_app_domain` is a required environment variable that must always be set for the system...
Learnt from: stnguyen90
PR: appwrite/appwrite#10119
File: app/controllers/api/account.php:1226-1232
Timestamp: 2025-07-08T01:20:14.364Z
Learning: In Appwrite, `_APP_DOMAIN` is a required environment variable that must always be set for the system to function properly.
Applied to files:
src/Appwrite/Utopia/Response/Model/ConsoleVariables.phpapp/controllers/api/console.phpapp/config/variables.php.envapp/views/install/compose.phtmldocker-compose.yml
🧬 Code Graph Analysis (1)
tests/unit/Network/Validators/DNSTest.php (1)
src/Appwrite/Network/Validator/DNS.php (2)
DNS(9-130)isValid(50-105)
🪛 dotenv-linter (3.3.0)
.env
[warning] 24-24: [UnorderedKey] The _APP_DNS key should go before the _APP_EDITION key
[warning] 25-25: [UnorderedKey] The _APP_DOMAIN key should go before the _APP_EDITION key
[warning] 26-26: [UnorderedKey] The _APP_CONSOLE_DOMAIN key should go before the _APP_CONSOLE_HOSTNAMES key
[warning] 27-27: [UnorderedKey] The _APP_DOMAIN_FUNCTIONS key should go before the _APP_EDITION key
[warning] 28-28: [UnorderedKey] The _APP_DOMAIN_SITES key should go before the _APP_EDITION key
[warning] 29-29: [UnorderedKey] The _APP_DOMAIN_TARGET_CNAME key should go before the _APP_EDITION key
[warning] 30-30: [UnorderedKey] The _APP_DOMAIN_TARGET_A key should go before the _APP_DOMAIN_TARGET_CNAME key
[warning] 31-31: [UnorderedKey] The _APP_DOMAIN_TARGET_AAAA key should go before the _APP_DOMAIN_TARGET_CNAME key
[warning] 32-32: [UnorderedKey] The _APP_DOMAIN_TARGET_CAA key should go before the _APP_DOMAIN_TARGET_CNAME key
⏰ 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). (4)
- GitHub Check: E2E Service Test (Messaging)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Databases)
- GitHub Check: Benchmark
🔇 Additional comments (14)
composer.json (1)
58-58: LGTM! DNS package dependency added for CAA validation.The addition of
utopia-php/dnspackage is appropriate for implementing CAA record validation functionality as described in the PR objectives.tests/e2e/Services/Console/ConsoleConsoleClientTest.php (2)
27-27: Correctly updated test count for new environment variable.The count has been appropriately increased from 13 to 14 to account for the new
_APP_DOMAIN_TARGET_CAAvariable.
31-31: LGTM! New CAA variable assertion added.The assertion correctly validates that the new
_APP_DOMAIN_TARGET_CAAenvironment variable is returned as a string type..env (2)
24-24: LGTM! DNS server configuration added.The addition of
_APP_DNS=8.8.8.8provides a sensible default DNS server for CAA record validation.
32-32: LGTM! CAA domain target configured.The
_APP_DOMAIN_TARGET_CAA=digicert.comsetting appropriately configures the CAA domain for certificate authority validation.app/controllers/api/console.php (1)
74-75: LGTM! Correct CAA record formatting.The formatting follows proper CAA DNS record syntax with flags (0), tag (issue), and quoted domain value. The comment clearly explains the purpose.
app/config/variables.php (2)
154-162: LGTM! CAA domain target variable properly configured.The
_APP_DOMAIN_TARGET_CAAvariable is well-documented with a clear description. Making it optional (not required) with no default value is appropriate for CAA validation functionality.
163-171: LGTM! DNS server variable properly configured.The
_APP_DNSvariable configuration is appropriate with Google's public DNS (8.8.8.8) as the default. The description clearly explains its purpose for domain validation.src/Appwrite/Utopia/Response/Model/ConsoleVariables.php (1)
31-36: LGTM: CAA environment variable properly added.The new
_APP_DOMAIN_TARGET_CAAenvironment variable follows the established pattern of other domain target variables and is correctly configured with appropriate type, description, and example value.tests/unit/Network/Validators/DNSTest.php (1)
51-76: Excellent test coverage for CAA validation.The test method comprehensively covers various CAA record scenarios including different flag values, CA domains, and domains without CAA records. This provides good validation of the DNS validator's CAA functionality.
docker-compose.yml (4)
123-124: LGTM: Environment variables properly configured.The new CAA and DNS environment variables are consistently added across all relevant services that handle domain validation and certificate operations.
540-541: Consistent environment variable configuration.The variables are properly included in the certificate worker service environment.
711-712: Consistent environment variable configuration.The variables are properly included in the migrations worker service environment.
747-748: Consistent environment variable configuration.The variables are properly included in the maintenance task service environment.
What does this PR do?
Adds CAA validation before adding rule, to prevent most common certificate issue problem
Test Plan
Related PRs and Issues
x
Checklist