Conversation
📝 WalkthroughWalkthroughThis pull request modifies SSL certificate handling and updates the deployment environment configuration. The SSL task action method is expanded with dependency injection for Document $console and Database $dbForPlatform parameters. Domain validation logic is introduced to reject unknown or test domains early. Rule retrieval and creation logic now considers the _APP_RULES_FORMAT environment variable, using MD5-hashed domains as rule keys when applicable. New rule documents are created with domain, type, status, project identifiers, owner (detected from environment variables for Appwrite-owned domains), search, and region fields. Existing rules are updated with status changes. Comprehensive logging is added throughout rule creation, updates, and certificate scheduling. Environment configuration transitions from development/test setup to production with DNS server and domain changes. Docker-compose infrastructure is simplified by removing CoreDNS service and related network IPAM subnet configuration. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/Appwrite/Platform/Tasks/SSL.php`:
- Around line 76-81: The current ownership check in the foreach loop uses
str_ends_with($domain->get(), $appwriteDomain) which yields false positives
(e.g. "evilappwrite.io"); change the condition in the loop to mark owner =
'Appwrite' only when $domain->get() equals $appwriteDomain exactly or when the
matched suffix is preceded by a dot (i.e. the char before the suffix is '.'),
keeping the rest of the foreach and the $owner assignment intact; update the
condition around $appwriteDomain and $domain->get() accordingly so subdomain
matches are valid but arbitrary suffix matches are not.
- Around line 98-103: The queued domain Document in SSL.php is missing the
domainType attribute; update the $queueForCertificates->setDomain(...) call to
include 'domainType' => $rule->getAttribute('type') alongside 'domain' (so the
new Document contains both keys), leaving the subsequent
->setSkipRenewCheck($skipCheck)->trigger() chain unchanged; locate the code
around the setDomain invocation on $queueForCertificates and add the domainType
entry sourced from $rule->getAttribute('type').
✨ Benchmark results
⚡ Benchmark Comparison
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.env (1)
1-30:⚠️ Potential issue | 🟠 MajorProduction env with localhost domains will likely break routing and console access.
With
_APP_ENV=production, keeping_APP_CONSOLE_DOMAIN,_APP_DOMAIN_FUNCTIONS, and_APP_DOMAIN_SITESaslocalhostis inconsistent and typically unusable in a real deployment. Please align these hostnames with the new_APP_DOMAIN(or document why localhost is intended in prod) to avoid broken console access, callbacks, and SSL rule handling.
🤖 Fix all issues with AI agents
In @.env:
- Line 26: Move the _APP_DOMAIN entry so dotenv-linter ordering is correct:
locate the environment keys named _APP_DOMAIN and _APP_EDITION and reorder them
so _APP_DOMAIN appears immediately before _APP_EDITION (i.e., place the line
starting with _APP_DOMAIN above the line starting with _APP_EDITION), then run
dotenv-linter to verify the warning is resolved.
.env
Outdated
| _APP_OPENSSL_KEY_V1=your-secret-key | ||
| _APP_DNS=172.16.238.100 # CoreDNS | ||
| _APP_DOMAIN=appwrite.test | ||
| _APP_DOMAIN=do-droplet.hmacr.space |
There was a problem hiding this comment.
Fix dotenv-linter key ordering for _APP_DOMAIN.
dotenv-linter flags _APP_DOMAIN as out of order; it should appear before _APP_EDITION.
♻️ Proposed reorder
-_APP_ENV=production
-_APP_EDITION=self-hosted
+_APP_ENV=production
+_APP_DOMAIN=do-droplet.hmacr.space
+_APP_EDITION=self-hosted
@@
-_APP_DOMAIN=do-droplet.hmacr.space🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 26-26: [UnorderedKey] The _APP_DOMAIN key should go before the _APP_EDITION key
(UnorderedKey)
🤖 Prompt for AI Agents
In @.env at line 26, Move the _APP_DOMAIN entry so dotenv-linter ordering is
correct: locate the environment keys named _APP_DOMAIN and _APP_EDITION and
reorder them so _APP_DOMAIN appears immediately before _APP_EDITION (i.e., place
the line starting with _APP_DOMAIN above the line starting with _APP_EDITION),
then run dotenv-linter to verify the warning is resolved.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.env:
- Line 1: Remove the committed production flag from the shared .env by deleting
or changing the _APP_ENV=production entry and instead add guidance to use
environment-specific files or deployment secrets; create or update an ignored
.env.production for actual production values (and ensure it is gitignored) and
provide a safe example value in a committed .env.example (e.g.,
_APP_ENV=development) so developers have defaults without shipping production
settings.
- Around line 25-26: Remove deployment-specific values from the tracked .env by
replacing the hardcoded _APP_DNS and _APP_DOMAIN with non-sensitive placeholders
or local defaults: update the _APP_DOMAIN entry (currently _APP_DOMAIN) to a
generic/example domain (e.g., example.local or appwrite.test) and change
_APP_DNS (currently _APP_DNS) to a local/default DNS (e.g., 127.0.0.1) or a
clearly marked placeholder; ensure any real deployment values are moved to env
files excluded from version control or injected by deployment tooling.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Appwrite/Platform/Tasks/SSL.php`:
- Around line 44-45: The $skipCheck normalization currently coerces boolean true
into a string and flips it; change the logic in the SSL task where $skipCheck is
normalized so that if $skipCheck is already a boolean it is left unchanged,
otherwise normalize string values case-insensitively (accepting 'true','1','yes'
as true) and set $skipCheck accordingly; update the code that assigns $skipCheck
(the variable named $skipCheck in this file) to perform an is_bool($skipCheck)
check first and only apply string-to-boolean parsing when it is not boolean.
Closes SER-1092