Use _APP_CONSOLE_DOMAIN, if not found, then use _APP_DOMAIN #9999
Conversation
📝 WalkthroughWalkthroughThis change introduces a new environment variable, Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. 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! |
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (1)
1443-1451: 💡 Verification agent❓ Verification inconclusive
Same empty-hostname edge case in Git status links
$hostnamemay still be empty here, yielding malformed links pushed to GitHub (https:///console/...). Re-use the validation added above or centralise hostname retrieval in a helper to guarantee non-empty output across the worker.No diff provided as fix will mirror the pattern suggested earlier.
#!/bin/bash # Show context around lines 1443–1451 echo "=== File snippet (lines 1400–1500) ===" sed -n '1400,1500p' src/Appwrite/Platform/Modules/Functions/Workers/Builds.php # Find all System::getEnv calls to see how hostname is fetched/validated echo -e "\n=== All getEnv calls in Builds.php ===" rg "System::getEnv" -n src/Appwrite/Platform/Modules/Functions/Workers/Builds.php # Search for any hostname validation or empty checks in this file echo -e "\n=== Checks for empty(\$hostname) or similar patterns ===" rg -n "empty\(\$hostname" -n src/Appwrite/Platform/Modules/Functions/Workers/Builds.php rg -n "throw.*hostname" src/Appwrite/Platform/Modules/Functions/Workers/Builds.php
Enforce non-empty hostname before building console URLs
The call to
$hostname = System::getEnv('_APP_CONSOLE_DOMAIN', System::getEnv('_APP_DOMAIN', ''));can still return an empty string, producing links like
https:///console/.... Please reuse the validation added earlier (or extract hostname resolution into a helper) to guarantee a non-empty value. For example, insrc/Appwrite/Platform/Modules/Functions/Workers/Builds.php(lines 1443–1451):• Replace raw getEnv calls with a validated helper:
- $hostname = System::getEnv('_APP_CONSOLE_DOMAIN', System::getEnv('_APP_DOMAIN', '')); + $hostname = $this->getConsoleDomain(); // throws or falls back to a valid host• Or add an explicit guard:
if (empty($hostname)) { throw new \Exception('Console domain is not configured'); }This ensures all generated URLs are well-formed.
🧹 Nitpick comments (6)
tests/e2e/Services/Projects/ProjectsCustomServerTest.php (1)
86-89: Duplicate constant look-ups – tighten up to avoid shadowing
$functionsDomainis fetched twice (lines 55 and 88) and$sitesDomaintwice (64 and 87). Not harmful but it’s unnecessary noise and risks future divergence.- $functionsDomain = System::getEnv('_APP_DOMAIN_FUNCTIONS', ''); - ... - $sitesDomain = System::getEnv('_APP_DOMAIN_SITES', ''); + $functionsDomain = System::getEnv('_APP_DOMAIN_FUNCTIONS', ''); + $sitesDomain = System::getEnv('_APP_DOMAIN_SITES', '');Move the assignments once, before use, then re-use the variables.
src/Appwrite/Platform/Workers/Functions.php (1)
486-489: Same empty-host concern & duplication
Identical lookup appears here; consider extracting toDomain::getConsoleHost()(or similar) to avoid drift and to enforce a non-empty default.app/controllers/api/console.php (1)
46-48: Avoid repeated getenv call – compute once and reuse
System::getEnv('_APP_CONSOLE_DOMAIN', System::getEnv('_APP_DOMAIN', ''))is evaluated twice in consecutive lines. Cache the value in a local variable to improve readability and prevent any subtle divergence should the fallback logic ever change.- $validator = new Domain(System::getEnv('_APP_CONSOLE_DOMAIN', System::getEnv('_APP_DOMAIN', ''))); - $isDomainValid = !empty(System::getEnv('_APP_CONSOLE_DOMAIN', System::getEnv('_APP_DOMAIN', ''))) && $validator->isKnown() && !$validator->isTest(); + $domainEnv = System::getEnv('_APP_CONSOLE_DOMAIN', System::getEnv('_APP_DOMAIN', '')); + $validator = new Domain($domainEnv); + $isDomainValid = !empty($domainEnv) && $validator->isKnown() && !$validator->isTest();src/Appwrite/Vcs/Comment.php (2)
89-92: Duplicate getenv & potential empty hostnameThe same fallback chain appears multiple times and can still return
''. Generating comment markdown with an empty hostname will break console links.Refactor to a helper (e.g.
Hostname::getConsole()) and assert non-empty before composing URLs.
203-205: Reuse resolved hostnameMinor nit – reuse the hostname you already resolved in
generateComment()rather than recomputing it here.- $protocol = System::getEnv('_APP_OPTIONS_FORCE_HTTPS') == 'disabled' ? 'http' : 'https'; - $hostname = System::getEnv('_APP_CONSOLE_DOMAIN', System::getEnv('_APP_DOMAIN', '')); + $protocol = System::getEnv('_APP_OPTIONS_FORCE_HTTPS') == 'disabled' ? 'http' : 'https'; + $hostname = System::getEnv('_APP_CONSOLE_DOMAIN', System::getEnv('_APP_DOMAIN', ''));(A helper as suggested above would eliminate this duplication entirely.)
app/controllers/general.php (1)
451-451: Consolidate execution endpoint hostname fallback
Correctly prioritizes_APP_CONSOLE_DOMAINover_APP_DOMAIN. Consider extracting this pattern into a reusable helper to reduce duplication.Proposed diff:
+// In a common utilities file +function getMainConsoleDomain(): string { + return System::getEnv('_APP_CONSOLE_DOMAIN', System::getEnv('_APP_DOMAIN', '')); +} - $hostname = System::getEnv('_APP_CONSOLE_DOMAIN', System::getEnv('_APP_DOMAIN', '')); + $hostname = getMainConsoleDomain();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing f 10000 iles that changed from the base of the PR and between e267283 and 710b8bd.
📒 Files selected for processing (14)
app/config/templates/site.php(1 hunks)app/controllers/api/console.php(1 hunks)app/controllers/general.php(5 hunks)app/views/general/error.phtml(1 hunks)src/Appwrite/Platform/Modules/Functions/Http/Executions/Create.php(1 hunks)src/Appwrite/Platform/Modules/Functions/Workers/Builds.php(2 hunks)src/Appwrite/Platform/Modules/Proxy/Http/Rules/API/Create.php(1 hunks)src/Appwrite/Platform/Modules/Proxy/Http/Rules/Function/Create.php(1 hunks)src/Appwrite/Platform/Modules/Proxy/Http/Rules/Redirect/Create.php(1 hunks)src/Appwrite/Platform/Modules/Proxy/Http/Rules/Site/Create.php(1 hunks)src/Appwrite/Platform/Workers/Functions.php(1 hunks)src/Appwrite/Vcs/Comment.php(2 hunks)src/Executor/Executor.php(1 hunks)tests/e2e/Services/Projects/ProjectsCustomServerTest.php(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (8)
app/views/general/error.phtml (1)
17-22: Domain fallback logic looks solid
The new precedence order (_APP_CONSOLE_DOMAIN➜_APP_DOMAIN➜'') cleanly fulfils the PR objective without breaking existing deployments.src/Appwrite/Platform/Modules/Functions/Http/Executions/Create.php (1)
353-357: Potential empty host – verify env guarantees or add safe default
If neither_APP_CONSOLE_DOMAINnor_APP_DOMAINis set,$hostnamebecomes an empty string and$endpointturns intohttps:///v1, causing malformed requests inside functions.-$hostname = System::getEnv('_APP_CONSOLE_DOMAIN', System::getEnv('_APP_DOMAIN', '')); +// Fallback hierarchy + sane default +$hostname = System::getEnv('_APP_CONSOLE_DOMAIN', System::getEnv('_APP_DOMAIN', 'localhost'));Alternatively, centralise this recurring lookup in a helper to eliminate copy-paste and guarantee a single fallback policy.
src/Appwrite/Platform/Modules/Proxy/Http/Rules/API/Create.php (1)
79-81: Good catch – rule denial list now aligned with console domain
This update prevents users from adding the console domain as a custom API rule, matching the rest of the codebase.src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (1)
597-599: Gracefully handle missing console hostnameIf neither
_APP_CONSOLE_DOMAINnor_APP_DOMAINis defined,$hostnamebecomes an empty string and$endpointbecomes something likehttps:///v1, producing an invalid URL that later HTTP clients will fail on silently.Consider failing fast (or logging clearly) when
$hostnameis empty:- $hostname = System::getEnv('_APP_CONSOLE_DOMAIN', System::getEnv('_APP_DOMAIN', '')); - $endpoint = $protocol . '://' . $hostname . "/v1"; + $hostname = System::getEnv('_APP_CONSOLE_DOMAIN', System::getEnv('_APP_DOMAIN', '')); + if ($hostname === '') { + throw new \Exception('Missing console domain – set _APP_CONSOLE_DOMAIN or _APP_DOMAIN'); + } + $endpoint = $protocol . '://' . $hostname . '/v1';app/controllers/general.php (4)
79-79: Prioritize console domain for base URL
The new fallback to_APP_CONSOLE_DOMAINwith a fallback to_APP_DOMAINaligns with the PR objective of avoiding theregion.prefix.
265-265: Use console domain in preview auth redirect
Consistently applying the same fallback chain ensures preview redirects point to the correct domain.
812-812: Apply console domain fallback in router injection
The router initialization now correctly skips internal routing for the console domain by using the prioritized environment variable chain.
861-861: Use console domain for certificate issuance logic
The certificate generation flow now leverages the same fallback, ensuring TLS issuance targets the correct main domain.
app/config/templates/site.php
Outdated
| $protocol = System::getEnv('_APP_OPTIONS_FORCE_HTTPS') == 'disabled' ? 'http' : 'https'; | ||
| $hostname = System::getEnv('_APP_DOMAIN'); | ||
| $hostname = System::getEnv('_APP_CONSOLE_DOMAIN', System::getEnv('_APP_DOMAIN', '')); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fallback breaks when _APP_CONSOLE_DOMAIN is defined but empty
System::getEnv('_APP_CONSOLE_DOMAIN', System::getEnv('_APP_DOMAIN', '')) returns the empty string when the variable exists but is set to "", short-circuiting the fallback to _APP_DOMAIN.
That corner-case actually happens in several staging setups where the variable is exported but intentionally left blank.
Tweak the fallback so that we only honor _APP_CONSOLE_DOMAIN when it is truthy (non-empty):
-$hostname = System::getEnv('_APP_CONSOLE_DOMAIN', System::getEnv('_APP_DOMAIN', ''));
+$hostname = System::getEnv('_APP_CONSOLE_DOMAIN', null);
+if (empty($hostname)) {
+ $hostname = System::getEnv('_APP_DOMAIN', '');
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $protocol = System::getEnv('_APP_OPTIONS_FORCE_HTTPS') == 'disabled' ? 'http' : 'https'; | |
| $hostname = System::getEnv('_APP_DOMAIN'); | |
| $hostname = System::getEnv('_APP_CONSOLE_DOMAIN', System::getEnv('_APP_DOMAIN', '')); | |
| $protocol = System::getEnv('_APP_OPTIONS_FORCE_HTTPS') == 'disabled' ? 'http' : 'https'; | |
| $hostname = System::getEnv('_APP_CONSOLE_DOMAIN', null); | |
| if (empty($hostname)) { | |
| $hostname = System::getEnv('_APP_DOMAIN', ''); | |
| } |
🤖 Prompt for AI Agents
In app/config/templates/site.php around lines 9 to 11, the fallback logic for
$hostname uses System::getEnv('_APP_CONSOLE_DOMAIN',
System::getEnv('_APP_DOMAIN', '')), which returns an empty string if
_APP_CONSOLE_DOMAIN is defined but empty, preventing fallback to _APP_DOMAIN.
Modify the code to first retrieve _APP_CONSOLE_DOMAIN, check if it is non-empty
(truthy), and only use it if so; otherwise, fallback to _APP_DOMAIN. This
ensures that an empty _APP_CONSOLE_DOMAIN does not short-circuit the fallback.
src/Executor/Executor.php
Outdated
| if (empty($headers['host'])) { | ||
| $headers['host'] = System::getEnv('_APP_DOMAIN', ''); | ||
| $headers['host'] = System::getEnv('_APP_CONSOLE_DOMAIN', System::getEnv('_APP_DOMAIN', '')); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same empty-string pitfall for Host header
If _APP_CONSOLE_DOMAIN is exported but blank, the Host header will be set to '', producing an invalid HTTP request.
Mirror the safer fallback logic suggested for site.php:
- $headers['host'] = System::getEnv('_APP_CONSOLE_DOMAIN', System::getEnv('_APP_DOMAIN', ''));
+ $host = System::getEnv('_APP_CONSOLE_DOMAIN', null);
+ if (empty($host)) {
+ $host = System::getEnv('_APP_DOMAIN', '');
+ }
+ $headers['host'] = $host;🤖 Prompt for AI Agents
In src/Executor/Executor.php around lines 202 to 204, the code sets the Host
header to an environment variable that might be an empty string, resulting in an
invalid HTTP request. Update the logic to check if the environment variable is
non-empty before assigning it to the Host header, and if it is empty, fallback
to the next environment variable or a default value, mirroring the safer
fallback approach used in site.php.
| $mainDomain = System::getEnv('_APP_CONSOLE_DOMAIN', System::getEnv('_APP_DOMAIN', '')); | ||
| $deniedDomains[] = $mainDomain; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Empty _APP_CONSOLE_DOMAIN still blocks the fallback
Same reasoning as above – an explicitly empty console domain will incorrectly pass validation and then be added to $deniedDomains, rejecting otherwise valid user domains. Apply the guarded fallback pattern here (and in the other rule files for consistency).
🤖 Prompt for AI Agents
In src/Appwrite/Platform/Modules/Proxy/Http/Rules/Site/Create.php around lines
84 to 86, the code assigns $mainDomain using
System::getEnv('_APP_CONSOLE_DOMAIN', System::getEnv('_APP_DOMAIN', '')) but
does not check if _APP_CONSOLE_DOMAIN is empty before falling back. To fix this,
implement a guarded fallback pattern that only uses _APP_CONSOLE_DOMAIN if it is
set and non-empty; otherwise, fallback to _APP_DOMAIN. This prevents empty
_APP_CONSOLE_DOMAIN values from blocking valid domains by being added to
$deniedDomains. Apply this pattern consistently in this and other similar rule
files.
| $mainDomain = System::getEnv('_APP_CONSOLE_DOMAIN', System::getEnv('_APP_DOMAIN', '')); | ||
| $deniedDomains[] = $mainDomain; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against blank _APP_CONSOLE_DOMAIN
Replicate the non-empty check to make sure $mainDomain is populated with _APP_DOMAIN when the console domain is unset or blank.
🤖 Prompt for AI Agents
In src/Appwrite/Platform/Modules/Proxy/Http/Rules/Redirect/Create.php around
lines 87 to 89, the assignment to $mainDomain does not check if
_APP_CONSOLE_DOMAIN is blank before falling back to _APP_DOMAIN. Modify the code
to first check if _APP_CONSOLE_DOMAIN is set and non-empty; if it is empty or
unset, then assign $mainDomain to _APP_DOMAIN. This ensures $mainDomain is never
blank by replicating the non-empty check used elsewhere.
| $mainDomain = System::getEnv('_APP_CONSOLE_DOMAIN', System::getEnv('_APP_DOMAIN', '')); | ||
| $deniedDomains[] = $mainDomain; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consistency: use truthy check before falling back
For the function rule too, rely on _APP_DOMAIN when _APP_CONSOLE_DOMAIN is not set or empty to avoid accidental denial of domains.
🤖 Prompt for AI Agents
In src/Appwrite/Platform/Modules/Proxy/Http/Rules/Function/Create.php around
lines 84 to 86, update the assignment of $mainDomain to use a truthy check for
_APP_CONSOLE_DOMAIN before falling back to _APP_DOMAIN. This means first
retrieving _APP_CONSOLE_DOMAIN, and if it is not set or is empty, then use
_APP_DOMAIN as the fallback value. This ensures consistent behavior and avoids
accidentally denying domains when _APP_CONSOLE_DOMAIN is empty.
✨ Benchmark results
⚡ Benchmark Comparison
|
|
Replace |
…write/appwrite into chore-update-env-var-used-for-domain
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
src/Appwrite/Platform/Workers/Webhooks.php (1)
221-233: 🛠️ Refactor suggestion
⚠️ Potential issueUndefined variable
$curlErrorstill referenced in the updated block
$curlErroris set insideexecute()but isn’t passed intosendEmailAlert().
With the newly-added region logic you kept the reference ({{error}}), so the notice will still be triggered and the placeholder will render “The server returned …” even when cURL actually failed.- $template->setParam('{{error}}', $curlError ?? 'The server returned ' . $statusCode . ' status code'); + $template->setParam('{{error}}', isset($curlError) + ? $curlError + : 'The server returned ' . $statusCode . ' status code');Preferably expose the real error by adding a parameter to
sendEmailAlert()and passing it fromexecute().
Leaving it as-is continues to hide useful diagnostics and pollutes the logs with “undefined variable” notices.app/controllers/api/vcs.php (1)
363-383:⚠️ Potential issueSanitise dynamic URL segments and avoid leaking raw project names
$providerTargetUrlnow embeds$region,$projectId,$resourceCollection,$resourceType,$resourceIddirectly into the path.
If any of these values contain/, spaces, or unicode the resulting URL becomes invalid and may create reflected-XSS vectors when echoed back to the browser (e.g. GitHub status page).- $providerTargetUrl = $request->getProtocol() . '://' . $request->getHostname() . "/console/project-$region-$projectId/$resourceCollection/$resourceType-$resourceId"; + $providerTargetUrl = sprintf( + '%s://%s/console/project-%s-%s/%s/%s-%s', + $request->getProtocol(), + $request->getHostname(), + rawurlencode($region), + rawurlencode($projectId), + rawurlencode($resourceCollection), + rawurlencode($resourceType), + rawurlencode($resourceId), + );Encoding once at the point of construction is simpler than scattering
rawurlencode()throughout the codebase.
🧹 Nitpick comments (2)
src/Appwrite/Platform/Workers/Webhooks.php (1)
221-233: Path construction lacks encoding & may double-dash the “default” region
/console/project-$region-$projectId/...will output/console/project-default-<id>for single-region projects.
- Consider omitting the region when it equals
'default'to preserve clean URLs and backward compatibility.- Wrap dynamic segments with
rawurlencode()to avoid accidental injection (e.g. a project named"foo/bar").Example tweak:
- $template->setParam('{{path}}', "/console/project-$region-$projectId/settings/webhooks/$webhookId"); + $encodedRegion = $region === 'default' ? '' : rawurlencode($region) . '-'; + $encodedProject = rawurlencode($projectId); + $encodedWebhook = rawurlencode($webhookId); + $template->setParam('{{path}}', "/console/project-${encodedRegion}${encodedProject}/settings/webhooks/${encodedWebhook}");app/controllers/api/vcs.php (1)
496-507:$stateoverride logic may lose nested data – use the spread operator instead
array_merge($defaultState, $state ?? [])works, but overwriting$statewith the merged result discards any keys other thansuccess&failurethat GitHub may send back (e.g. future metadata).Safer pattern:
- $state = \array_merge($defaultState, $state ?? []); + $state = [...$defaultState, ...($state ?? [])]; // PHP 8.1+Spreading preserves non-conflicting keys while still giving caller-provided values precedence.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/controllers/api/vcs.php(3 hunks)app/controllers/general.php(8 hunks)src/Appwrite/Platform/Modules/Functions/Workers/Builds.php(2 hunks)src/Appwrite/Platform/Workers/Webhooks.php(2 hunks)src/Executor/Executor.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Appwrite/Platform/Modules/Functions/Workers/Builds.php
- src/Executor/Executor.php
- app/controllers/general.php
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (VCS)
- GitHub Check: E2E Service Test (Migrations)
- GitHub Check: E2E Service Test (Messaging)
- GitHub Check: E2E Service Test (Proxy)
- GitHub Check: E2E Service Test (Users)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: Unit Test
- GitHub Check: E2E General Test
- GitHub Check: Benchmark
- GitHub Check: scan
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.env (1)
25-25: Sort_APP_CONSOLE_DOMAINalphabetically within_APP_CONSOLE_keys
Static analysis (dotenv-linter) flags this key as out of order. To maintain grouping and readability, move_APP_CONSOLE_DOMAIN=localhostso it appears before_APP_CONSOLE_HOSTNAMES.Apply this diff:
@@ _APP_CONSOLE_COUNTRIES_DENYLIST=AQ +_APP_CONSOLE_DOMAIN=localhost _APP_CONSOLE_HOSTNAMES=localhost,appwrite.io,*.appwrite.io @@ -_APP_CONSOLE_DOMAIN=localhost
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.env(1 hunks)app/config/templates/site.php(1 hunks)app/controllers/api/vcs.php(4 hunks)app/controllers/general.php(3 hunks)docker-compose.yml(3 hunks)src/Appwrite/Platform/Modules/Functions/Workers/Builds.php(2 hunks)src/Appwrite/Vcs/Comment.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- app/config/templates/site.php
- app/controllers/general.php
- src/Appwrite/Platform/Modules/Functions/Workers/Builds.php
- src/Appwrite/Vcs/Comment.php
- app/controllers/api/vcs.php
🧰 Additional context used
🪛 dotenv-linter (3.3.0)
.env
[warning] 25-25: [UnorderedKey] The _APP_CONSOLE_DOMAIN key should go before the _APP_CONSOLE_HOSTNAMES key
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Benchmark
- GitHub Check: E2E Service Test (Teams)
- GitHub Check: E2E Service Test (Webhooks)
- GitHub Check: E2E Service Test (Messaging)
- GitHub Check: E2E Service Test (Databases)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E General Test
🔇 Additional comments (3)
docker-compose.yml (3)
118-120: Propagate_APP_CONSOLE_DOMAINto the Appwrite API service
Adding this environment variable ensures your new fallback logic uses the console domain before_APP_DOMAIN.
214-218: Upgrade the Appwrite Console image version
The image was bumped fromappwrite/console:6.0.41toappwrite/console:6.1.2. Verify that this tag exists in your registry and matches any compatibility guarantees.
484-486: Propagate_APP_CONSOLE_DOMAINto the Builds worker service
Ensures the builds worker honors the new console-domain-first logic.
…write/appwrite into chore-update-env-var-used-for-domain
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/controllers/general.php (2)
79-79: Inconsistent equality comparison - use strict equality for consistency.The change to prioritize
_APP_CONSOLE_DOMAINis correct and aligns with the PR objectives. However, this line uses loose equality (==) while line 456 uses strict equality (===) for the same environment variable check.Apply this diff for consistency:
- $url = (System::getEnv('_APP_OPTIONS_FORCE_HTTPS') == 'disabled' ? 'http' : 'https') . '://' . System::getEnv('_APP_CONSOLE_DOMAIN', System::getEnv('_APP_DOMAIN', '')); + $url = (System::getEnv('_APP_OPTIONS_FORCE_HTTPS') === 'disabled' ? 'http' : 'https') . '://' . System::getEnv('_APP_CONSOLE_DOMAIN', System::getEnv('_APP_DOMAIN', ''));
270-270: Inconsistent equality comparison - use strict equality for consistency.The domain fallback logic is implemented correctly. However, similar to line 79, this uses loose equality (
==) while line 456 uses strict equality (===).Apply this diff for consistency:
- $url = (System::getEnv('_APP_OPTIONS_FORCE_HTTPS') == 'disabled' ? 'http' : 'https') . "://" . System::getEnv('_APP_CONSOLE_DOMAIN', System::getEnv('_APP_DOMAIN', '')); + $url = (System::getEnv('_APP_OPTIONS_FORCE_HTTPS') === 'disabled' ? 'http' : 'https') . "://" . System::getEnv('_APP_CONSOLE_DOMAIN', System::getEnv('_APP_DOMAIN', ''));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/controllers/general.php(3 hunks)docker-compose.yml(3 hunks)src/Appwrite/Platform/Modules/Functions/Workers/Builds.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docker-compose.yml
🧰 Additional context used
🪛 PHPMD (2.15.0)
app/controllers/general.php
79-79: Avoid using static access to class '\Utopia\System\System' in method 'router'. (Clean Code Rules)
(StaticAccess)
79-79: Avoid using static access to class '\Utopia\System\System' in method 'router'. (Clean Code Rules)
(StaticAccess)
79-79: Avoid using static access to class '\Utopia\System\System' in method 'router'. (Clean Code Rules)
(StaticAccess)
270-270: Avoid using static access to class '\Utopia\System\System' in method 'router'. (Clean Code Rules)
(StaticAccess)
270-270: Avoid using static access to class '\Utopia\System\System' in method 'router'. (Clean Code Rules)
(StaticAccess)
270-270: Avoid using static access to class '\Utopia\System\System' in method 'router'. (Clean Code Rules)
(StaticAccess)
456-456: Avoid using static access to class '\Utopia\System\System' in method 'router'. (Clean Code Rules)
(StaticAccess)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php
597-597: Avoid using static access to class '\Utopia\System\System' in method 'buildDeployment'. (Clean Code Rules)
(StaticAccess)
597-597: Avoid using static access to class '\Utopia\System\System' in method 'buildDeployment'. (Clean Code Rules)
(StaticAccess)
1450-1450: Avoid using static access to class '\Utopia\System\System' in method 'runGitAction'. (Clean Code Rules)
(StaticAccess)
1450-1450: Avoid using static access to class '\Utopia\System\System' in method 'runGitAction'. (Clean Code Rules)
(StaticAccess)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
🔇 Additional comments (5)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (3)
597-597: LGTM: Hostname resolution updated to prioritize console domain.The change correctly implements the PR objective by prioritizing
_APP_CONSOLE_DOMAINover_APP_DOMAIN, which will prevent unwantedregion.prefixes from being added to domains.
1450-1450: LGTM: Consistent hostname resolution for GitHub integration.The change maintains consistency with the buildDeployment method by using the same hostname resolution pattern for GitHub provider target URLs.
597-597: Validate presence of domain environment variables before URL constructionIt appears
$hostnamecan be empty when neither_APP_CONSOLE_DOMAINnor_APP_DOMAINis set, leading to malformed URLs likehttps:///…. I didn’t find any code‐level guard against an empty hostname, so please ensure one of these vars is always populated or add a check.Locations to review:
- src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (Lines 597 & 1450)
- src/Appwrite/Vcs/Comment.php (URL templates using
$hostname)- src/Appwrite/Platform/Workers/Mails.php (
$variables['host'] = $protocol . '://' . $hostname;)- src/Appwrite/Platform/Modules/Functions/Workers/Functions.php and Create.php (endpoint
$protocol://$hostname/v1)Suggested guard:
$hostname = System::getEnv('_APP_CONSOLE_DOMAIN', System::getEnv('_APP_DOMAIN', '')); if (empty($hostname)) { throw new \RuntimeException('Environment variable _APP_CONSOLE_DOMAIN or _APP_DOMAIN must be set'); }Or document that these variables are required in all environments.
app/controllers/general.php (2)
456-456: LGTM! Good improvement to use strict equality.The change from loose (
==) to strict (===) equality comparison is a good practice for environment variable checks.
457-458: Confirmed use of APP_DOMAIN for internal API endpoints is intentional
The endpoint constructed in app/controllers/general.php (around lines 457–458) is meant for internal function/site API communication and should always use_APP_DOMAINrather than falling back to_APP_CONSOLE_DOMAIN. Other parts of the code (console URLs, VCS callbacks, worker builds) require the console domain fallback, but function runtime vars do not. No change needed here.
…write/appwrite into chore-update-env-var-used-for-domain
What does this PR do?
This would help prevent
region.prefix from domains.Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Screenshots may also be helpful.)
Related PRs and Issues
Checklist