Conversation
📝 WalkthroughWalkthroughThis PR adds a logs attribute to the webhooks collection and introduces four RULE_STATUS_* constants. It replaces single-server DNS validation with a multi-server, coroutine-based validator and adds a new Action class that centralizes domain restriction and verification logic (CAA/CNAME/A/AAAA target resolution driven by environment). Multiple proxy rule handlers (Create/Get/Function/Site/Redirect/Verification/XList) were refactored to use string-based domains, inject logging, update status transitions (CREATED → VERIFIED → CERTIFICATE_GENERATING), and call the new verification flow. It also adds CoreDNS test infrastructure, updates docker-compose and .env DNS settings, and expands E2E/unit tests and rule response metadata. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Areas to pay extra attention:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Appwrite/Platform/Modules/Proxy/Http/Rules/Function/Create.php (1)
134-150: Rule status and logs changes are not persisted to the database.After successful verification, the status is changed to
RULE_STATUS_CERTIFICATE_GENERATING(line 137), and on failure, logs are set (line 139). However, these in-memory changes are never persisted back to the database viaupdateDocument. The response will show the updated status, but the database record will remain withRULE_STATUS_CREATED.Add a database update after modifying the rule:
if ($rule->getAttribute('status', '') === RULE_STATUS_CREATED) { try { $this->verifyRule($rule, $log); $rule->setAttribute('status', RULE_STATUS_CERTIFICATE_GENERATING); } catch (Exception $err) { $rule->setAttribute('logs', $err->getMessage()); } + $rule = $dbForPlatform->updateDocument('rules', $rule->getId(), $rule); }src/Appwrite/Platform/Modules/Proxy/Http/Rules/Site/Create.php (1)
134-150: Rule status and logs changes are not persisted to the database.After creating the rule at line 129, the status and logs attributes are modified on lines 137-139, but there's no
updateDocumentcall to persist these changes. The response will show the updated values, but the database will retain the original state, causing data inconsistency.Add an update call after the verification/certificate logic:
if ($rule->getAttribute('status', '') === RULE_STATUS_CERTIFICATE_GENERATING) { $queueForCertificates ->setDomain(new Document([ 'domain' => $rule->getAttribute('domain'), 'domainType' => $rule->getAttribute('deploymentResourceType', $rule->getAttribute('type')), ])) ->trigger(); } + $rule = $dbForPlatform->updateDocument('rules', $rule->getId(), $rule); + $queueForEvents->setParam('ruleId', $rule->getId());
🧹 Nitpick comments (4)
app/init/constants.php (1)
211-216: Consider adding clarifying comments for semantic alignment.The constant names describe internal implementation details (certificate generation), while the values represent user-facing API states. This is likely intentional for API backward compatibility, but may cause confusion for developers.
Consider enhancing the comments to clarify the mapping:
// Rule statuses const RULE_STATUS_CREATED = 'created'; // This is also the status when domain DNS verification fails. -const RULE_STATUS_CERTIFICATE_GENERATING = 'verifying'; -const RULE_STATUS_CERTIFICATE_GENERATION_FAILED = 'unverified'; +const RULE_STATUS_CERTIFICATE_GENERATING = 'verifying'; // API value: 'verifying' - certificate issuance in progress +const RULE_STATUS_CERTIFICATE_GENERATION_FAILED = 'unverified'; // API value: 'unverified' - certificate issuance failed const RULE_STATUS_VERIFIED = 'verified';src/Appwrite/Platform/Modules/Proxy/Http/Rules/Redirect/Create.php (1)
81-160: Consider extracting common rule creation logic to reduce duplication.The
actionmethods inAPI/Create.php,Function/Create.php, andRedirect/Create.phpshare significant duplicate code for status determination, owner assignment, verification flow, and certificate queueing. Consider extracting this common logic into the baseActionclass or a shared trait.src/Appwrite/Platform/Modules/Proxy/Http/Rules/Action.php (2)
75-79: Shadowed variable$domainis unused after validation.The
Domainobject is created to validate the format, but it shadows the string$domainparameter and is never used. If the intent is purely format validation, the object is discarded correctly, but the variable shadowing could cause confusion in future maintenance.Consider using a different variable name or an inline validation:
try { - $domain = new Domain($domain); + new Domain($domain); // Validates domain format } catch (\Throwable) { throw new Exception(Exception::GENERAL_ARGUMENT_INVALID, 'Domain may not start with http:// or https://.'); }
140-171: Redundant null checks for$mainValidatorassignment.The first validator added to the array will always satisfy the
is_null($mainValidator)condition, making subsequent checks unnecessary. The pattern is repeated three times.Simplify by assigning directly on the first validator:
$validators = []; - $mainValidator = null; // Validator to use for error description if (!is_null($targetCNAME)) { $validator = new $dnsValidatorClass($targetCNAME->get(), Record::TYPE_CNAME, $dnsServers); $validators[] = $validator; - - if (\is_null($mainValidator)) { - $mainValidator = $validator; - } } // Ensure at least one of CNAME/A/AAAA record points to our servers properly $targetA = System::getEnv('_APP_DOMAIN_TARGET_A', ''); if ((new IP(IP::V4))->isValid($targetA)) { $validator = new $dnsValidatorClass($targetA, Record::TYPE_A, $dnsServers); $validators[] = $validator; - - if (\is_null($mainValidator)) { - $mainValidator = $validator; - } } $targetAAAA = System::getEnv('_APP_DOMAIN_TARGET_AAAA', ''); if ((new IP(IP::V6))->isValid($targetAAAA)) { $validator = new $dnsValidatorClass($targetAAAA, Record::TYPE_AAAA, $dnsServers); $validators[] = $validator; - - if (\is_null($mainValidator)) { - $mainValidator = $validator; - } } if (empty($validators)) { throw new Exception(Exception::GENERAL_SERVER_ERROR, 'At least one of domain targets environment variable must be configured.'); } + $mainValidator = $validators[0]; // Use first validator for error description $validator = new AnyOf($validators, AnyOf::TYPE_STRING);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
app/config/collections/platform.php(1 hunks)app/init/constants.php(1 hunks)src/Appwrite/Network/Validator/DNS.php(1 hunks)src/Appwrite/Platform/Modules/Proxy/Http/Rules/API/Create.php(5 hunks)src/Appwrite/Platform/Modules/Proxy/Http/Rules/Action.php(1 hunks)src/Appwrite/Platform/Modules/Proxy/Http/Rules/Function/Create.php(7 hunks)src/Appwrite/Platform/Modules/Proxy/Http/Rules/Get.php(2 hunks)src/Appwrite/Platform/Modules/Proxy/Http/Rules/Redirect/Create.php(7 hunks)src/Appwrite/Platform/Modules/Proxy/Http/Rules/Site/Create.php(7 hunks)src/Appwrite/Platform/Modules/Proxy/Http/Rules/Verification/Update.php(5 hunks)src/Appwrite/Platform/Modules/Proxy/Http/Rules/XList.php(2 hunks)src/Appwrite/Utopia/Response/Model/Rule.php(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-10T04:12:13.037Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Indexes/Delete.php:85-86
Timestamp: 2025-05-10T04:12:13.037Z
Learning: The class `Appwrite\Platform\Modules\Databases\Http\Databases\Collections\Indexes\Action` defines a method `getGrantParentNotFoundException()` which is used to handle cases where a parent resource (like a database collection) is not found.
Applied to files:
src/Appwrite/Platform/Modules/Proxy/Http/Rules/Get.php
🧬 Code graph analysis (5)
src/Appwrite/Platform/Modules/Proxy/Http/Rules/XList.php (2)
src/Appwrite/Platform/Modules/Proxy/Http/Rules/Get.php (1)
__construct(24-54)src/Appwrite/Platform/Modules/Proxy/Http/Rules/Delete.php (1)
__construct(28-62)
src/Appwrite/Platform/Modules/Proxy/Http/Rules/Function/Create.php (1)
src/Appwrite/Platform/Modules/Proxy/Http/Rules/Action.php (4)
Action(17-188)__construct(19-21)validateDomainRestrictions(31-80)verifyRule(89-187)
src/Appwrite/Network/Validator/DNS.php (1)
src/Appwrite/Platform/Modules/Proxy/Http/Rules/Action.php (1)
__construct(19-21)
src/Appwrite/Platform/Modules/Proxy/Http/Rules/Site/Create.php (1)
src/Appwrite/Platform/Modules/Proxy/Http/Rules/Action.php (3)
Action(17-188)validateDomainRestrictions(31-80)verifyRule(89-187)
src/Appwrite/Platform/Modules/Proxy/Http/Rules/Get.php (1)
src/Appwrite/Utopia/Response/Model/Rule.php (1)
__construct(10-107)
🪛 PHPMD (2.15.0)
src/Appwrite/Platform/Modules/Proxy/Http/Rules/Verification/Update.php
75-75: Avoid unused parameters such as '$platform'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (13)
app/config/collections/platform.php (1)
1320-1330: LGTM!The new
logsattribute for therulescollection follows the established pattern and is consistent with similarlogsfields in thecertificates(line 1034) andwebhooks(line 947) collections. The 1MB size limit is appropriate for log storage.src/Appwrite/Utopia/Response/Model/Rule.php (1)
93-98: LGTM!The updated description accurately documents the log prioritization behavior (certificate logs take precedence), and the example provides a realistic DNS verification failure scenario that helps API consumers understand the expected log format.
src/Appwrite/Platform/Modules/Proxy/Http/Rules/XList.php (2)
29-31: LGTM!The constructor pattern aligns with the
Get.phpimplementation, properly forwarding parameters to the parent class.
113-117: LGTM!The conditional log handling correctly prioritizes certificate generation logs over rule verification logs when present, matching the documented behavior in the
Rulemodel and consistent with theGet.phpimplementation.src/Appwrite/Platform/Modules/Proxy/Http/Rules/Get.php (2)
24-27: LGTM!Constructor properly forwards parameters to parent, consistent with the pattern in
XList.php.
69-73: LGTM!The conditional log prioritization logic is consistent with
XList.phpand correctly implements the documented behavior where certificate logs take precedence when available.src/Appwrite/Platform/Modules/Proxy/Http/Rules/Verification/Update.php (2)
86-89: Verify early return doesn't skip event emission.The early return when status is
RULE_STATUS_VERIFIEDorRULE_STATUS_CERTIFICATE_GENERATINGmeansqueueForEventsis set withruleId(line 84) but no event is triggered or explicitly skipped. Verify this is the intended behavior.
94-97: This pattern is correct. TheupdateDocument()method in Utopia Database is designed for partial updates—passing a Document with only the fields to update (logs,status,$updatedAt) merges those fields with the existing document as intended. This is the standard and idiomatic approach used consistently throughout the Appwrite codebase. No changes needed.src/Appwrite/Platform/Modules/Proxy/Http/Rules/Function/Create.php (1)
33-35: LGTM!The variadic constructor pattern properly forwards parameters to the parent
Actionclass, enabling dependency injection for the DNS validator class.src/Appwrite/Platform/Modules/Proxy/Http/Rules/Action.php (1)
89-186: LGTM on the overall verification flow.The
verifyRulemethod correctly validates CAA records, supports multiple DNS record types (CNAME, A, AAAA), uses configurable DNS servers, and provides detailed logging for debugging DNS timing issues. The use ofAnyOfvalidator allows flexible DNS configurations.src/Appwrite/Platform/Modules/Proxy/Http/Rules/Site/Create.php (3)
33-36: LGTM on constructor changes.The variadic parameter forwarding to the parent constructor is clean and allows the base
Actionclass to handle optional dependencies likednsValidatorClass.
143-150: Confirm that Appwrite-managed domains should skip certificate generation.When a domain ends with
functionsDomainorsitesDomain, the status is set toRULE_STATUS_VERIFIED(line 105), which bypasses both the verification block (line 134) and the certificate queue (line 143).Please confirm this is intentional—presumably these domains are covered by wildcard certificates.
103-106: No changes needed—rule status constants are properly defined and accessible.The constants
RULE_STATUS_CREATED,RULE_STATUS_VERIFIED, andRULE_STATUS_CERTIFICATE_GENERATINGare properly defined as global constants in app/init/constants.php, which is loaded during application bootstrap. No explicit imports are required for global PHP constants.
src/Appwrite/Platform/Modules/Proxy/Http/Rules/Function/Create.php
Outdated
Show resolved
Hide resolved
src/Appwrite/Platform/Modules/Proxy/Http/Rules/Redirect/Create.php
Outdated
Show resolved
Hide resolved
src/Appwrite/Platform/Modules/Proxy/Http/Rules/Verification/Update.php
Outdated
Show resolved
Hide resolved
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
✨ Benchmark results
⚡ Benchmark Comparison
|
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/Appwrite/Platform/Modules/Proxy/Http/Rules/Verification/Update.php (1)
119-121: Potential undefined variable$certificate.The variable
$certificateis only assigned inside theif (!empty($certificateId))block (line 100). If$certificateIdis empty, accessing$certificateon line 119 will cause an undefined variable error in PHP 8+.Apply this fix:
$certificateId = $rule->getAttribute('certificateId', ''); + $certificate = null; // Reset logs for the associated certificate. if (!empty($certificateId)) { $certificate = $dbForPlatform->updateDocument('certificates', $certificateId, new Document([ 'logs' => '', ])); } } catch (Exception $err) { ... } - if (!empty($certificate)) { + if ($certificate !== null) { $rule->setAttribute('renewAt', $certificate->getAttribute('renewDate', '')); }
🧹 Nitpick comments (4)
tests/resources/coredns/Corefile (3)
1-4: Consider documenting the duplicate forward directive.The global forward at line 3 and the zone-specific forward at line 63 both target 1.1.1.1. While zone-specific forwards can override global behavior in CoreDNS, this duplication may appear redundant. If intentional, consider adding a comment explaining why both are needed.
Also applies to: 63-63
10-10: Consider using RFC 5737 reserved IP ranges for test records.The IP addresses 203.0.0.1 and 203.0.0.5 are not within the RFC 5737 reserved ranges for documentation and testing (192.0.2.0/24, 198.51.100.0/24, 203.0.113.0/24). Using reserved ranges helps avoid potential conflicts with real IPs and clearly signals that these are test values.
Example replacement:
- answer "{{ .Name }} 60 IN A 203.0.0.1" + answer "{{ .Name }} 60 IN A 203.0.113.1"Also applies to: 40-40, 53-53, 70-70
1-73: Consider adding inline comments to document test scenarios.While the naming conventions (e.g., "stage-wrong-caa", "stage-correct-caa") are self-explanatory, inline comments would improve maintainability by explicitly documenting the purpose of each template and the specific validation scenarios they're designed to test.
Example:
+ # Test case: Validate A record resolution for base domain template IN A { match "^webapp\.com\.$" answer "{{ .Name }} 60 IN A 203.0.0.1" fallthrough }tests/unit/Network/Validators/DNSTest.php (1)
11-27: Consider adding positive validation tests for more complete coverage.The tests verify that empty/null inputs fail validation, but there are no tests asserting successful validation when a domain correctly resolves to the expected target. This limits confidence that the multi-server coroutine-based validation actually works for valid scenarios.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
docker-compose.yml(4 hunks)src/Appwrite/Platform/Modules/Proxy/Http/Rules/API/Create.php(4 hunks)src/Appwrite/Platform/Modules/Proxy/Http/Rules/Function/Create.php(6 hunks)src/Appwrite/Platform/Modules/Proxy/Http/Rules/Redirect/Create.php(6 hunks)src/Appwrite/Platform/Modules/Proxy/Http/Rules/Site/Create.php(6 hunks)src/Appwrite/Platform/Modules/Proxy/Http/Rules/Verification/Update.php(3 hunks)tests/e2e/Services/Proxy/ProxyCustomServerTest.php(6 hunks)tests/resources/coredns/Corefile(1 hunks)tests/unit/Network/Validators/DNSTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
src/Appwrite/Platform/Modules/Proxy/Http/Rules/Redirect/Create.php (2)
src/Appwrite/Platform/Modules/Proxy/Http/Rules/Action.php (4)
Action(17-188)__construct(19-21)validateDomainRestrictions(31-80)verifyRule(89-187)src/Appwrite/Utopia/Response/Model/Rule.php (1)
__construct(10-107)
src/Appwrite/Platform/Modules/Proxy/Http/Rules/API/Create.php (1)
src/Appwrite/Platform/Modules/Proxy/Http/Rules/Action.php (3)
Action(17-188)validateDomainRestrictions(31-80)verifyRule(89-187)
src/Appwrite/Platform/Modules/Proxy/Http/Rules/Function/Create.php (1)
src/Appwrite/Platform/Modules/Proxy/Http/Rules/Action.php (4)
Action(17-188)__construct(19-21)validateDomainRestrictions(31-80)verifyRule(89-187)
src/Appwrite/Platform/Modules/Proxy/Http/Rules/Verification/Update.php (3)
src/Appwrite/Platform/Modules/Proxy/Http/Rules/Action.php (3)
Action(17-188)__construct(19-21)verifyRule(89-187)src/Appwrite/Network/Validator/DNS.php (1)
__construct(20-25)src/Appwrite/Event/Certificate.php (1)
setDomain(29-34)
src/Appwrite/Platform/Modules/Proxy/Http/Rules/Site/Create.php (1)
src/Appwrite/Platform/Modules/Proxy/Http/Rules/Action.php (4)
Action(17-188)__construct(19-21)validateDomainRestrictions(31-80)verifyRule(89-187)
tests/unit/Network/Validators/DNSTest.php (1)
src/Appwrite/Network/Validator/DNS.php (2)
DNS(9-67)isValid(33-66)
🪛 GitHub Actions: Linter
tests/unit/Network/Validators/DNSTest.php
[error] 1-1: PSR-12: single_blank_line_at_eof issue detected. Fix end-of-file newline. (rule: single_blank_line_at_eof). Command: 'vendor/bin/pint --test --config pint.json' failed with exit code 1.
⏰ 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). (19)
- GitHub Check: E2E Service Test (Databases/TablesDB)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Storage)
- GitHub Check: E2E Service Test (Proxy)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Databases/Legacy)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: Unit Test
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: E2E General Test
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: Benchmark
- GitHub Check: scan
🔇 Additional comments (20)
tests/unit/Network/Validators/DNSTest.php (1)
29-40: LGTM!The test correctly verifies that validation failure exposes the expected public properties (
count,value,records) andgetDescription()returns meaningful output. Usinguniqid()ensures the domain doesn't exist.docker-compose.yml (2)
1085-1098: LGTM! CoreDNS service configuration is well-structured for testing.The service is properly isolated with a fixed IP, read-only volume mount for the Corefile, and restart policy. The commented-out port forwarding is a nice touch for debugging flexibility.
1230-1232: Network IPAM configuration correctly supports the fixed CoreDNS IP.The subnet
172.16.238.0/24properly encompasses the CoreDNS fixed address172.16.238.100.src/Appwrite/Platform/Modules/Proxy/Http/Rules/Verification/Update.php (2)
29-31: Constructor pattern aligns with parent class.The variadic parameter forwarding to parent constructor is consistent with the Action base class pattern used across the codebase.
84-87: Early return logic correctly prevents redundant verification.Returning early for
RULE_STATUS_VERIFIEDandRULE_STATUS_CERTIFICATE_GENERATINGstates prevents unnecessary re-processing of already verified or in-progress rules.tests/e2e/Services/Proxy/ProxyCustomServerTest.php (2)
573-705: Comprehensive test coverage for rule verification scenarios.The test methodically covers site rules, function rules, apex domains, CNAME records, CAA records, and various failure modes. This provides good coverage for the new verification flow.
707-738: Good test for timestamp update behavior on re-verification.The test correctly validates that re-running verification updates the timestamp while preserving logs, which is important for audit and debugging purposes.
src/Appwrite/Platform/Modules/Proxy/Http/Rules/Function/Create.php (2)
103-106: Status check now correctly guards against empty domain strings.The condition properly checks
$functionsDomain != ''and$sitesDomain != ''before callingstr_ends_with(), addressing the previous review concern about empty strings matching any domain.
128-135: Verification flow with error logging is well-structured.The try-catch block properly attempts verification and captures error messages in logs on failure, allowing the rule to be created even when verification fails initially.
src/Appwrite/Platform/Modules/Proxy/Http/Rules/API/Create.php (2)
85-91: Status and owner check correctly guards against empty domain strings.The condition properly validates that environment variables are non-empty before using
str_ends_with(), preventing false matches when$functionsDomainor$sitesDomainare empty.
107-114: Verification flow is consistent with other Create handlers.The try-catch pattern for verification with error logging matches the implementation in
Function/Create.php, maintaining consistency across rule creation paths.src/Appwrite/Platform/Modules/Proxy/Http/Rules/Redirect/Create.php (5)
8-8: LGTM: Structural changes align with refactoring goals.The addition of
Actionbase class import,Loginjection, and variadic constructor properly support the new centralized verification flow with logging capabilities.Also applies to: 18-18, 34-36, 77-77
102-108: Empty string guard issue from previous review is now resolved.The current code properly guards against empty environment variables with
$functionsDomain != ''and$sitesDomain != ''checks before callingstr_ends_with(). The short-circuit&&evaluation ensures thatstr_ends_with($domain, '')is never evaluated when the environment variable is empty, preventing the status from being incorrectly set toVERIFIED.
129-136: Persistence issue from previous review is now resolved.The modifications to
statusandlogsare correctly persisted. The code modifies the$ruleDocument object in-memory (lines 132, 134), then calls$dbForPlatform->createDocument('rules', $rule)at line 139, which persists all attributes including the verification changes. This is the standard creation pattern in Utopia Database.The verification flow is well-structured:
- On successful verification: status transitions to
CERTIFICATE_GENERATING- On failure: error message captured in logs, status remains
CREATED
81-83: LGTM: Improved observability and centralized validation.The addition of the
Logparameter enables DNS timing metrics and diagnostic logging during verification. Delegating domain validation tovalidateDomainRestrictions()centralizes the domain restriction logic and reduces code duplication across rule types.
98-99: LGTM: Clean domain handling and clear status lifecycle.The shift to string-based domain handling simplifies the codebase. The new
RULE_STATUS_*constants make the status lifecycle explicit:
CREATED: Initial state for external domains requiring verificationVERIFIED: For Appwrite-owned domains (*.sites or *.functions) that skip verificationCERTIFICATE_GENERATING: Triggered only after successful verificationThe certificate generation is correctly gated on the
CERTIFICATE_GENERATINGstatus, ensuring certificates are only issued for verified domains.Also applies to: 114-114, 124-124, 144-144
src/Appwrite/Platform/Modules/Proxy/Http/Rules/Site/Create.php (4)
8-8: LGTM: Consistent refactoring across rule types.The structural changes mirror those in
Redirect/Create.php, maintaining consistency across the rule creation handlers. TheActionbase class extension andLoginjection enable the centralized verification flow with observability.Also applies to: 18-18, 33-35, 74-74
97-103: Empty string guards properly implemented.The guards
$functionsDomain != ''and$sitesDomain != ''correctly preventstr_ends_with($domain, '')from being evaluated with empty environment variables, avoiding the incorrectVERIFIEDstatus assignment.
125-132: Verification flow correctly implemented.The rule modifications are properly persisted through
createDocument()at line 135. The verification logic correctly:
- Attempts verification for
CREATEDstatus rules- Transitions to
CERTIFICATE_GENERATINGon success- Captures error messages in
logson failure while maintainingCREATEDstatus for retry attempts
78-78: LGTM: Consistent implementation with site-specific enhancements.The implementation follows the same refactored patterns as
Redirect/Create.php:
- Centralized domain validation via
validateDomainRestrictions()- String-based domain handling
- Clear status lifecycle with
RULE_STATUS_*constants- Log-enabled verification for observability
The inclusion of the
$branchparameter in the search field (line 120) is appropriate for site rules that support VCS-based deployments.Also applies to: 80-80, 93-94, 109-109, 120-120, 140-140
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tests/unit/Network/Validators/DNSTest.php (2)
42-52: CoreDNS dependency requires skip logic or integration test placement.This test depends on CoreDNS running at
172.16.238.100with no mechanism to skip when unavailable. Consider adding a connectivity check withmarkTestSkipped()or moving to an integration test suite.
52-53: Add missing newline at end of file.PSR-12 requires files to end with a single blank line.
} } +
🧹 Nitpick comments (2)
tests/unit/Network/Validators/DNSTest.php (2)
11-18: Tests lack success case validation.Both
testSingleDNSServerandtestMultipleDNSServersonly verify failure scenarios (empty/null inputs). Consider adding assertions for successful validation where a domain correctly resolves to the expected target to actually exercise the validation logic.Additionally, these two tests have identical assertions—consider differentiating them by testing behavior unique to single vs. multi-server validation (e.g., partial failure scenarios).
20-27: No explicit test for multi-server consensus behavior.The DNS validator's core multi-server feature (fail if ANY server disagrees) isn't directly tested. The CoreDNS setup in
testCoreDNSFailurepartially addresses this, but consider a test where servers are expected to return differing results to verify the fail-fast behavior documented in the implementation.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.env(1 hunks)docker-compose.yml(4 hunks)src/Appwrite/Platform/Modules/Proxy/Http/Rules/API/Create.php(4 hunks)tests/e2e/Services/Proxy/ProxyCustomServerTest.php(6 hunks)tests/unit/Network/Validators/DNSTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docker-compose.yml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-08T01:20:14.364Z
Learnt from: stnguyen90
Repo: appwrite/appwrite PR: 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:
.env
🧬 Code graph analysis (2)
tests/e2e/Services/Proxy/ProxyCustomServerTest.php (3)
tests/e2e/General/UsageTest.php (1)
tearDown(1380-1383)tests/e2e/Services/Sites/SitesBase.php (2)
setupSite(21-34)cleanupSite(85-94)tests/e2e/Services/Functions/FunctionsBase.php (2)
setupFunction(20-33)cleanupFunction(69-78)
tests/unit/Network/Validators/DNSTest.php (1)
src/Appwrite/Network/Validator/DNS.php (2)
DNS(9-67)isValid(33-66)
🪛 dotenv-linter (4.0.0)
.env
[warning] 24-24: [UnorderedKey] The _APP_DNS key should go before the _APP_EDITION key
(UnorderedKey)
[warning] 24-24: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
[warning] 25-25: [UnorderedKey] The _APP_DOMAIN key should go before the _APP_EDITION key
(UnorderedKey)
[warning] 26-26: [UnorderedKey] The _APP_CONSOLE_DOMAIN key should go before the _APP_CONSOLE_HOSTNAMES key
(UnorderedKey)
[warning] 27-27: [UnorderedKey] The _APP_DOMAIN_FUNCTIONS key should go before the _APP_EDITION key
(UnorderedKey)
[warning] 28-28: [UnorderedKey] The _APP_DOMAIN_SITES key should go before the _APP_EDITION key
(UnorderedKey)
[warning] 29-29: [UnorderedKey] The _APP_DOMAIN_TARGET_CNAME key should go before the _APP_EDITION key
(UnorderedKey)
[warning] 30-30: [UnorderedKey] The _APP_DOMAIN_TARGET_A key should go before the _APP_DOMAIN_TARGET_CNAME key
(UnorderedKey)
⏰ 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). (13)
- GitHub Check: E2E Service Test (Tokens)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Databases/Legacy)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (Databases/TablesDB)
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: Unit Test
- GitHub Check: E2E General Test
- GitHub Check: Benchmark
- GitHub Check: scan
🔇 Additional comments (13)
tests/unit/Network/Validators/DNSTest.php (1)
29-40: LGTM!Good use of
\uniqid()to generate unique domains avoiding DNS caching, and proper verification of public properties exposed after validation failure..env (2)
24-24: DNS configuration updated correctly for CoreDNS integration.The IP address
172.16.238.100correctly points to the CoreDNS service as indicated in the docker-compose.yml changes mentioned in the AI summary.Optional style note: The static analysis tool suggests surrounding the value in quotes (
"172.16.238.100"), though this is not required for .env files to function correctly.
29-30: Domain target updates align with CoreDNS test infrastructure.The CNAME and A record target changes support the new multi-server DNS validation approach referenced in the AI summary and are consumed by the verification tests.
tests/e2e/Services/Proxy/ProxyCustomServerTest.php (4)
18-46: Well-implemented tearDown with correct query serialization.The cleanup logic properly deletes test rules and verifies completion. The previous issue with
Query::limit(1)missing->toString()on line 39 has been correctly resolved.
89-89: Excellent refactoring to use status constants.Replacing string literals with
RULE_STATUS_CREATEDandRULE_STATUS_VERIFIEDconstants improves maintainability and reduces the risk of typos.Also applies to: 391-391, 400-400, 409-409
573-705: Comprehensive verification test coverage.This test method provides excellent coverage of the rule verification flow, including:
- Successful verifications (site, function, API with correct DNS records)
- Failure scenarios (missing/incorrect CNAME, wrong A record, wrong CAA)
- Proper status transitions (CREATED → CERTIFICATE_GENERATING)
- Log message validation
The use of the static domain
webapp.comexplains the need for the tearDown cleanup.
707-738: Timestamp update validation is well-designed.This test correctly verifies that re-running verification with unchanged DNS data updates the
$updatedAttimestamp while preserving the original logs, ensuring proper idempotency behavior.src/Appwrite/Platform/Modules/Proxy/Http/Rules/API/Create.php (6)
31-34: Constructor properly delegates to parent.The variadic parameter pattern allows flexible dependency injection and consistent initialization through the parent Action class.
69-69: Log injection properly configured.The Log dependency is correctly injected and utilized in the verification flow at line 109, supporting the PR objective to enhance logging.
Also applies to: 73-73
75-75: Domain validation centralized effectively.Calling
validateDomainRestrictions($domain, $platform)centralizes validation logic in the Action parent class, reducing code duplication across rule handlers.
81-91: Status initialization with proper empty string guards.The empty string checks at lines 86-87 correctly prevent
str_ends_with($domain, '')from returning true for any domain, addressing the previous review concern. Appwrite-managed domains are appropriately marked as VERIFIED.
107-114: Verification logic correctly integrated into creation flow.The verification executes before document creation (line 117), so both the status change to
RULE_STATUS_CERTIFICATE_GENERATINGand any error logs are properly persisted. The previous review concern about persistence was based on a misunderstanding—this is the CREATE path, not an UPDATE, so the subsequentcreateDocumentcall persists all changes.
122-129: Certificate queueing correctly gated by verification status.Triggering certificate generation only when
status === RULE_STATUS_CERTIFICATE_GENERATINGensures certificates are not attempted for unverified domains.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/e2e/Services/Proxy/ProxyCustomServerTest.php (2)
573-705: Comprehensive verification test with DNS infrastructure dependency.This test thoroughly validates 9 domain verification scenarios covering A, CNAME, and CAA record configurations. The hardcoded domains (e.g., 'stage-site.webapp.com', 'wrong-a-webapp.com') require corresponding DNS records in the test environment, which aligns with the CoreDNS test infrastructure mentioned in the PR summary.
The test structure is clear with numbered scenario comments and consistent create-assert-cleanup patterns.
Consider splitting this into separate test methods (e.g.,
testRuleVerificationWithCorrectARecord,testRuleVerificationWithWrongCNAME) for better isolation and maintainability. However, keeping them together in an E2E test reduces setup/teardown overhead, which is also reasonable.
718-718: Fix variable naming for consistency.The variable
$initiallogsshould be$initialLogsto maintain camelCase consistency with other variables like$initialUpdatedAt(line 717).Apply this diff:
- $initiallogs = $rule['body']['logs']; + $initialLogs = $rule['body']['logs'];And update references on lines 725 and 730:
- $this->assertStringContainsString($initiallogs, $updatedRule['body']['message']); + $this->assertStringContainsString($initialLogs, $updatedRule['body']['message']);- $this->assertEquals($initiallogs, $ruleAfterUpdate['body']['logs']); + $this->assertEquals($initialLogs, $ruleAfterUpdate['body']['logs']);src/Appwrite/Platform/Modules/Proxy/Http/Rules/Function/Create.php (1)
97-99: Consider using strict comparison for clarity.While
!=works correctly here, using!==would be more idiomatic and explicit about type-safe comparison.Apply this diff for stricter comparison:
if ( - ($functionsDomain != '' && \str_ends_with($domain, $functionsDomain)) || - ($sitesDomain != '' && \str_ends_with($domain, $sitesDomain)) + ($functionsDomain !== '' && \str_ends_with($domain, $functionsDomain)) || + ($sitesDomain !== '' && \str_ends_with($domain, $sitesDomain)) ) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Appwrite/Platform/Modules/Proxy/Http/Rules/Function/Create.php(6 hunks)tests/e2e/Services/Proxy/ProxyCustomServerTest.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Appwrite/Platform/Modules/Proxy/Http/Rules/Function/Create.php (1)
src/Appwrite/Platform/Modules/Proxy/Http/Rules/Action.php (3)
Action(17-188)validateDomainRestrictions(31-80)verifyRule(89-187)
⏰ 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). (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (8)
tests/e2e/Services/Proxy/ProxyCustomServerTest.php (2)
18-46: LGTM! Previous serialization issue has been addressed.The
tearDownmethod correctly cleans up test rules. Line 39 now properly serializes the query with->toString(), addressing the previous review comment.
707-738: Test logic correctly validates timestamp updates.The test properly verifies that re-running verification with the same data updates the
$updatedAttimestamp while preserving the existing logs. Thesleep(1)on line 720 is necessary to ensure a measurable timestamp difference in E2E testing.src/Appwrite/Platform/Modules/Proxy/Http/Rules/Function/Create.php (6)
33-36: LGTM! Clean constructor pattern.The variadic parameter forwarding to the parent constructor is a clean pattern that enables flexible initialization.
78-78: LGTM! Log injection aligns with new verification flow.The addition of the
Logparameter supports the new verification workflow and logging requirements.
80-80: LGTM! Centralized domain validation improves maintainability.Delegating domain restriction validation to the
Actionbase class reduces code duplication and centralizes the validation logic.
125-132: LGTM! Verification flow with appropriate error handling.The new verification workflow correctly:
- Attempts DNS validation for custom domains (status = CREATED)
- Transitions to CERTIFICATE_GENERATING on success
- Logs errors and maintains CREATED status on failure
97-103: Consistency verified across all Rule Create handlers.The owner and status determination logic is identical across all create rules files (API, Function, Redirect, Site). No inconsistencies were found.
101-101: Certificate generation logic for VERIFIED domains is intentional and working as designed.The current implementation correctly distinguishes between Appwrite-managed domains and custom domains:
- Appwrite-managed domains (ending with
$functionsDomainor$sitesDomain) are set toRULE_STATUS_VERIFIEDat creation and skip both verification and certificate generation queuing in the Create handler- Custom domains follow the flow:
RULE_STATUS_CREATED→ verification →RULE_STATUS_CERTIFICATE_GENERATING→ certificate generation queueThis pattern is consistent across all Create handlers (Function, Site, Redirect, API). The Verification/Update handler reinforces this design by returning early when status is already
RULE_STATUS_VERIFIED(line 85), preventing redundant verification attempts.Appwrite-managed domains do not require individual certificate generation because they are pre-configured with wildcard or shared certificates.
2 participants
Related SER-721
Along the way, refactored the code to use common constants & action for rule verification.