From 7a27cf4ac7ac77a4d7f3459d3ec4f036f2c81a55 Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Fri, 27 Feb 2026 01:44:46 +1300 Subject: [PATCH] fix: address review feedback for installer PR - Initialize $isUpgrade=false in Install.php action() to prevent undefined variable - Assign $this->lockedDatabase in Upgrade.php before calling parent::action() - Remove stack trace exposure from buildErrorDetails() in Http Install action - Suppress raw exception messages for 500+ errors in Error handler - Remove sessionSecret from progress details to prevent credential leak - Hash name/email in analytics payload to avoid sending raw PII - Validate and default dbService in compose.phtml to prevent invalid output - Fix host normalization in progress.js redirect URL builder - Release global lock on early return for existing installation conflict - Consolidate duplicate database host/port assignment blocks - Add @runInSeparateProcess to testRouteRegistration to prevent global state leak Co-Authored-By: Claude Opus 4.6 --- app/views/install/compose.phtml | 6 +++++- app/views/install/installer/js/modules/progress.js | 2 +- .../Platform/Installer/Http/Installer/Error.php | 3 ++- .../Platform/Installer/Http/Installer/Install.php | 8 ++------ src/Appwrite/Platform/Tasks/Install.php | 14 ++++---------- src/Appwrite/Platform/Tasks/Upgrade.php | 2 ++ .../unit/Platform/Modules/Installer/ModuleTest.php | 3 +++ 7 files changed, 19 insertions(+), 19 deletions(-) diff --git a/app/views/install/compose.phtml b/app/views/install/compose.phtml index e431ad8284..7664155718 100644 --- a/app/views/install/compose.phtml +++ b/app/views/install/compose.phtml @@ -12,7 +12,11 @@ $version = $this->getParam('version', ''); $organization = $this->getParam('organization', ''); $image = $this->getParam('image', ''); $enableAssistant = $this->getParam('enableAssistant', false); -$dbService = $this->getParam('database'); +$dbService = $this->getParam('database', 'mongodb'); +$allowedDbServices = ['mariadb', 'mongodb']; +if (!\in_array($dbService, $allowedDbServices, true)) { + $dbService = 'mongodb'; +} $hostPath = rtrim($this->getParam('hostPath', ''), '/'); ?>services: traefik: diff --git a/app/views/install/installer/js/modules/progress.js b/app/views/install/installer/js/modules/progress.js index 02c1e0ef2b..162ebd0aa0 100644 --- a/app/views/install/installer/js/modules/progress.js +++ b/app/views/install/installer/js/modules/progress.js @@ -271,7 +271,7 @@ port = httpsPort; } if (!hasPort && port && ((protocol === 'http' && port !== '80') || (protocol === 'https' && port !== '443'))) { - host = `${rawDomain}:${port}`; + host = `${host}:${port}`; } return `${protocol}://${host}`; }; diff --git a/src/Appwrite/Platform/Installer/Http/Installer/Error.php b/src/Appwrite/Platform/Installer/Http/Installer/Error.php index 51a8d2e1e8..506c545125 100644 --- a/src/Appwrite/Platform/Installer/Http/Installer/Error.php +++ b/src/Appwrite/Platform/Installer/Http/Installer/Error.php @@ -31,6 +31,7 @@ class Error extends Action $code = 500; } $response->setStatusCode($code); - $response->json(['success' => false, 'message' => $error->getMessage()]); + $message = $code >= 500 ? 'Internal installer error' : $error->getMessage(); + $response->json(['success' => false, 'message' => $message]); } } diff --git a/src/Appwrite/Platform/Installer/Http/Installer/Install.php b/src/Appwrite/Platform/Installer/Http/Installer/Install.php index fc0ec365a2..366d4bcb28 100644 --- a/src/Appwrite/Platform/Installer/Http/Installer/Install.php +++ b/src/Appwrite/Platform/Installer/Http/Installer/Install.php @@ -181,6 +181,7 @@ class Install extends Action if (file_exists($existingPath)) { $existing = $state->readProgressFile($installId); if (!empty($existing['steps']) && $retryStep === null) { + $state->updateGlobalLock($installId, Server::STATUS_ERROR); if ($wantsStream) { $this->writeSseEvent($swooleResponse, Server::STATUS_ERROR, ['message' => 'Installation already started']); $swooleResponse->end(); @@ -389,12 +390,7 @@ class Install extends Action private function buildErrorDetails(\Throwable $e): array { - $details = ['trace' => $e->getTraceAsString()]; - $previous = $e->getPrevious(); - if ($previous instanceof \Throwable && $previous->getMessage() !== '') { - $details['output'] = $previous->getMessage(); - } - return $details; + return []; } private function hasPayload(mixed $data): bool diff --git a/src/Appwrite/Platform/Tasks/Install.php b/src/Appwrite/Platform/Tasks/Install.php index 9b04ded454..1d1c28d4b0 100644 --- a/src/Appwrite/Platform/Tasks/Install.php +++ b/src/Appwrite/Platform/Tasks/Install.php @@ -64,6 +64,7 @@ class Install extends Action bool $noStart, string $database ): void { + $isUpgrade = false; $defaultHttpPort = '80'; $defaultHttpsPort = '443'; $config = Config::getParam('variables'); @@ -273,13 +274,7 @@ class Install extends Action if ($database === 'postgresql') { $input['_APP_DB_HOST'] = 'postgresql'; $input['_APP_DB_PORT'] = 5432; - } elseif ($database === 'mariadb') { - $input['_APP_DB_HOST'] = 'mariadb'; - $input['_APP_DB_PORT'] = 3306; - } - - $database = $input['_APP_DB_ADAPTER']; - if ($database === 'mongodb') { + } elseif ($database === 'mongodb') { $input['_APP_DB_HOST'] = 'mongodb'; $input['_APP_DB_PORT'] = 27017; } elseif ($database === 'mariadb') { @@ -634,7 +629,6 @@ class Install extends Action details: [ 'userId' => $userId, 'sessionId' => $session['id'], - 'sessionSecret' => $session['secret'], 'sessionExpire' => $session['expire'] ?? null ], messageOverride: 'Account created successfully' @@ -684,8 +678,8 @@ class Install extends Action 'label' => 'self_hosted_' . $type, 'version' => $version, 'data' => json_encode([ - 'name' => $name, - 'email' => $email, + 'nameHash' => hash('sha256', trim($name)), + 'emailHash' => hash('sha256', strtolower(trim($email))), 'domain' => $domain, 'database' => $database, ]), diff --git a/src/Appwrite/Platform/Tasks/Upgrade.php b/src/Appwrite/Platform/Tasks/Upgrade.php index e50cde9440..baa33bcfd8 100644 --- a/src/Appwrite/Platform/Tasks/Upgrade.php +++ b/src/Appwrite/Platform/Tasks/Upgrade.php @@ -81,6 +81,8 @@ class Upgrade extends Install throw new \Exception('Database type not found, can not updgrade. Ensure `_APP_DB_ADAPTER` is set in your environment.'); } + $this->lockedDatabase = $database; + parent::action($httpPort, $httpsPort, $organization, $image, $interactive, $noStart, $database); } diff --git a/tests/unit/Platform/Modules/Installer/ModuleTest.php b/tests/unit/Platform/Modules/Installer/ModuleTest.php index 6281ba3361..343b119813 100644 --- a/tests/unit/Platform/Modules/Installer/ModuleTest.php +++ b/tests/unit/Platform/Modules/Installer/ModuleTest.php @@ -112,6 +112,9 @@ class ModuleTest extends TestCase $this->assertActionInjects($action, ['error', 'response']); } + /** + * @runInSeparateProcess + */ public function testRouteRegistration(): void { $services = $this->module->getServicesByType(Service::TYPE_HTTP);