From e009b0d3b46431e21058d745537759a29cb75cbc Mon Sep 17 00:00:00 2001 From: Oleksander Piskun Date: Tue, 11 Aug 2026 09:57:27 +0000 Subject: [PATCH 1/2] fix: do not deploy ExApp environment variables with an empty default as the literal string Array Signed-off-by: Oleksander Piskun --- lib/Service/ExAppEnvVarsHelper.php | 72 ++++++++ lib/Service/ExAppService.php | 32 +--- tests/php/Service/ExAppEnvVarsHelperTest.php | 145 ++++++++++++++++ .../Service/ExAppServiceGetAppInfoTest.php | 157 ++++++++++++++++++ 4 files changed, 383 insertions(+), 23 deletions(-) create mode 100644 lib/Service/ExAppEnvVarsHelper.php create mode 100644 tests/php/Service/ExAppEnvVarsHelperTest.php create mode 100644 tests/php/Service/ExAppServiceGetAppInfoTest.php diff --git a/lib/Service/ExAppEnvVarsHelper.php b/lib/Service/ExAppEnvVarsHelper.php new file mode 100644 index 000000000..385acc32d --- /dev/null +++ b/lib/Service/ExAppEnvVarsHelper.php @@ -0,0 +1,72 @@ +` or ``) becomes an empty array instead of an empty string. + * Left as-is, such a value passes the "drop variables with an empty value" filter and is later + * stringified to the literal `Array` in the container environment. + * + * The helper produces a canonical NAME => {name, displayName, description, default, value} map with + * every field a string, applies caller overrides (occ `--env`, UI deploy options, stored deploy + * options on update), and drops variables whose final value is empty. + */ +class ExAppEnvVarsHelper { + /** + * @param array $variables raw `environment-variables.variable` entries: a list, or a single entry as produced by SimpleXML for one `` element + * @param array $overrides deploy-option overrides, NAME => value or NAME => ['value' => value]; overrides for undeclared names are ignored + * @return array normalized NAME-keyed map, entries with an empty final value removed + * @throws InvalidArgumentException on the first malformed variable; message identifies the entry and field + */ + public static function normalizeAndValidate(array $variables, array $overrides): array { + if (!array_is_list($variables)) { + $variables = [$variables]; + } + $envVars = []; + foreach ($variables as $index => $variable) { + if (!is_array($variable)) { + throw new InvalidArgumentException(sprintf('variable #%d: entry must be an object, got %s', $index, get_debug_type($variable))); + } + $name = $variable['name'] ?? null; + if (!is_string($name) || trim($name) === '') { + throw new InvalidArgumentException(sprintf("variable #%d: 'name' must be a non-empty string, got %s", $index, get_debug_type($name))); + } + $default = self::toString($variable['default'] ?? ''); + $envVars[$name] = [ + 'name' => $name, + 'displayName' => self::toString($variable['display-name'] ?? ''), + 'description' => self::toString($variable['description'] ?? ''), + 'default' => $default, + 'value' => $default, + ]; + } + foreach ($overrides as $name => $value) { + if (array_key_exists($name, $envVars)) { + $envVars[$name]['value'] = self::toString($value['value'] ?? $value ?? ''); + } + } + return array_filter($envVars, static function (array $envVar) { + return $envVar['value'] !== ''; + }); + } + + /** + * An empty XML element arrives as [] after the simplexml/json roundtrip: treat any + * non-scalar as an empty string so the empty-value filter applies to every input shape. + */ + private static function toString(mixed $value): string { + return is_scalar($value) ? (string)$value : ''; + } +} diff --git a/lib/Service/ExAppService.php b/lib/Service/ExAppService.php index a0a538241..f2dd0aaa9 100644 --- a/lib/Service/ExAppService.php +++ b/lib/Service/ExAppService.php @@ -293,31 +293,17 @@ public function getAppInfo(string $appId, ?string $infoXml, ?string $jsonInfo, ? } // Advanced deploy options if (isset($appInfo['external-app']['environment-variables']['variable'])) { - $envVars = []; - if (!isset($appInfo['external-app']['environment-variables']['variable'][0])) { - $appInfo['external-app']['environment-variables']['variable'] = [$appInfo['external-app']['environment-variables']['variable']]; + $variables = $appInfo['external-app']['environment-variables']['variable']; + if (!is_array($variables)) { + return ['error' => sprintf("ExApp '%s' has invalid environment variable definition. 'variable' must be an object or a list of objects, got %s", $appId, get_debug_type($variables))]; } - foreach ($appInfo['external-app']['environment-variables']['variable'] as $envVar) { - $envVars[$envVar['name']] = [ - 'name' => $envVar['name'], - 'displayName' => $envVar['display-name'] ?? '', - 'description' => $envVar['description'] ?? '', - 'default' => $envVar['default'] ?? '', - 'value' => $envVar['default'] ?? '', - ]; - } - if (isset($deployOptions['environment_variables']) && count(array_keys($deployOptions['environment_variables'])) > 0) { - // override with given deploy options values - foreach ($deployOptions['environment_variables'] as $key => $value) { - if (array_key_exists($key, $envVars)) { - $envVars[$key]['value'] = $value['value'] ?? $value ?? ''; - } - } + try { + $appInfo['external-app']['environment-variables'] = ExAppEnvVarsHelper::normalizeAndValidate( + $variables, $deployOptions['environment_variables'] ?? [] + ); + } catch (InvalidArgumentException $e) { + return ['error' => sprintf("ExApp '%s' has invalid environment variable definition. %s", $appId, $e->getMessage())]; } - $envVars = array_filter($envVars, function ($envVar) { - return $envVar['value'] !== ''; - }); - $appInfo['external-app']['environment-variables'] = $envVars; } if (isset($appInfo['external-app']['k8s-service-roles']['role'])) { $roles = $appInfo['external-app']['k8s-service-roles']['role']; diff --git a/tests/php/Service/ExAppEnvVarsHelperTest.php b/tests/php/Service/ExAppEnvVarsHelperTest.php new file mode 100644 index 000000000..150d1fc18 --- /dev/null +++ b/tests/php/Service/ExAppEnvVarsHelperTest.php @@ -0,0 +1,145 @@ + + * element must not survive as an empty array and end up as the literal string `Array` in + * the container environment. The input is produced by the same simplexml/json roundtrip + * getAppInfo uses, so the [] shape is real, not hand-crafted. + */ + #[DataProvider('emptyDefaultXmlProvider')] + public function testEmptyDefaultElementFromRealXmlIsDropped(string $xml): void { + $parsed = json_decode(json_encode((array)simplexml_load_string($xml)), true); + $variables = $parsed['environment-variables']['variable']; + + // lock in the SimpleXML behavior the bug depends on: empty element parses to [] + self::assertSame([], $variables['default']); + + self::assertSame([], ExAppEnvVarsHelper::normalizeAndValidate($variables, [])); + } + + public static function emptyDefaultXmlProvider(): array { + return [ + '' => [ + '' + . 'EMPTY_ELEMEmptyd' + . '', + ], + '' => [ + '' + . 'EMPTY_ELEMEmptyd' + . '', + ], + ]; + } + + #[DataProvider('validVariablesProvider')] + public function testNormalizeAndValidate(array $variables, array $overrides, array $expected): void { + self::assertSame($expected, ExAppEnvVarsHelper::normalizeAndValidate($variables, $overrides)); + } + + public static function validVariablesProvider(): array { + return [ + 'single arrives as one object, not a list' => [ + ['name' => 'A', 'display-name' => 'Var A', 'description' => 'desc', 'default' => 'x'], + [], + ['A' => ['name' => 'A', 'displayName' => 'Var A', 'description' => 'desc', 'default' => 'x', 'value' => 'x']], + ], + 'variable without is dropped' => [ + [['name' => 'A', 'display-name' => 'Var A', 'description' => 'desc']], + [], + [], + ], + 'empty-element default ([]) is dropped, sibling with a value survives' => [ + [ + ['name' => 'EMPTY_ELEM', 'display-name' => 'Empty', 'description' => 'd', 'default' => []], + ['name' => 'KEPT', 'display-name' => 'Kept', 'description' => 'd', 'default' => 'v'], + ], + [], + ['KEPT' => ['name' => 'KEPT', 'displayName' => 'Kept', 'description' => 'd', 'default' => 'v', 'value' => 'v']], + ], + 'empty-element display-name and description become empty strings' => [ + [['name' => 'A', 'display-name' => [], 'description' => [], 'default' => 'x']], + [], + ['A' => ['name' => 'A', 'displayName' => '', 'description' => '', 'default' => 'x', 'value' => 'x']], + ], + 'override replaces the default value' => [ + [['name' => 'A', 'default' => 'x']], + ['A' => 'y'], + ['A' => ['name' => 'A', 'displayName' => '', 'description' => '', 'default' => 'x', 'value' => 'y']], + ], + 'override with empty value drops the variable' => [ + [['name' => 'A', 'default' => 'x']], + ['A' => ''], + [], + ], + 'override in stored deploy-options shape' => [ + [['name' => 'A', 'default' => 'x']], + ['A' => ['name' => 'A', 'displayName' => '', 'description' => '', 'default' => 'x', 'value' => 'y']], + ['A' => ['name' => 'A', 'displayName' => '', 'description' => '', 'default' => 'x', 'value' => 'y']], + ], + 'stored pre-fix deploy option with [] value is dropped, not deployed as Array' => [ + [['name' => 'A', 'default' => []]], + ['A' => ['name' => 'A', 'displayName' => '', 'description' => '', 'default' => [], 'value' => []]], + [], + ], + 'override for an undeclared variable is ignored' => [ + [['name' => 'A', 'default' => 'x']], + ['B' => 'y'], + ['A' => ['name' => 'A', 'displayName' => '', 'description' => '', 'default' => 'x', 'value' => 'x']], + ], + 'no variables declared' => [[], ['A' => 'y'], []], + 'numeric override is canonicalized to string' => [ + [['name' => 'A', 'default' => 'x']], + ['A' => 123], + ['A' => ['name' => 'A', 'displayName' => '', 'description' => '', 'default' => 'x', 'value' => '123']], + ], + ]; + } + + #[DataProvider('invalidVariablesProvider')] + public function testNormalizeAndValidateRejects(array $variables, string $expectedMessageFragment): void { + try { + ExAppEnvVarsHelper::normalizeAndValidate($variables, []); + self::fail('Expected InvalidArgumentException, none thrown'); + } catch (InvalidArgumentException $e) { + self::assertStringContainsString($expectedMessageFragment, $e->getMessage()); + } + } + + public static function invalidVariablesProvider(): array { + return [ + 'entry is not an array' => [ + ['not-an-object'], + 'variable #0: entry must be an object', + ], + 'missing name' => [ + [['display-name' => 'X', 'default' => 'v']], + "variable #0: 'name' must be a non-empty string", + ], + 'empty element parses to an array' => [ + [['name' => [], 'default' => 'v']], + "variable #0: 'name' must be a non-empty string", + ], + 'whitespace-only name' => [ + [['name' => ' ', 'default' => 'v']], + "variable #0: 'name' must be a non-empty string", + ], + ]; + } +} diff --git a/tests/php/Service/ExAppServiceGetAppInfoTest.php b/tests/php/Service/ExAppServiceGetAppInfoTest.php new file mode 100644 index 000000000..71cde71a7 --- /dev/null +++ b/tests/php/Service/ExAppServiceGetAppInfoTest.php @@ -0,0 +1,157 @@ + + * (parsed to [] by the simplexml/json roundtrip) never reaches the deploy actions. + */ +class ExAppServiceGetAppInfoTest extends TestCase { + private ExAppService $service; + private string $infoXmlPath = ''; + + protected function setUp(): void { + parent::setUp(); + + $cacheFactory = $this->createMock(ICacheFactory::class); + $cacheFactory->method('isAvailable')->willReturn(false); + + $this->service = new ExAppService( + $this->createMock(LoggerInterface::class), + $cacheFactory, + $this->createMock(IUserManager::class), + $this->createMock(ExAppFetcher::class), + $this->createMock(ExAppArchiveFetcher::class), + $this->createMock(ExAppMapper::class), + $this->createMock(TopMenuService::class), + $this->createMock(InitialStateService::class), + $this->createMock(ScriptsService::class), + $this->createMock(StylesService::class), + $this->createMock(FilesActionsMenuService::class), + $this->createMock(TaskProcessingService::class), + $this->createMock(TalkBotsService::class), + $this->createMock(SettingsService::class), + $this->createMock(ExAppOccService::class), + $this->createMock(ExAppDeployOptionsService::class), + $this->createMock(ExAppSetupCheckService::class), + $this->createMock(IConfig::class), + ); + } + + protected function tearDown(): void { + if ($this->infoXmlPath !== '' && file_exists($this->infoXmlPath)) { + unlink($this->infoXmlPath); + } + parent::tearDown(); + } + + private function writeInfoXml(string $environmentVariables): string { + $this->infoXmlPath = tempnam(sys_get_temp_dir(), 'appapi-test-info-') . '.xml'; + file_put_contents($this->infoXmlPath, << + + test_app + + +$environmentVariables + + + +XML); + return $this->infoXmlPath; + } + + public function testEmptyDefaultElementIsDroppedFromInfoXml(): void { + $infoXml = $this->writeInfoXml(<< + EMPTY_ELEM + Empty + d + + + + NO_DEFAULT + No default + + + KEPT + Kept + v + +XML); + + $appInfo = $this->service->getAppInfo('test_app', $infoXml, null); + + self::assertArrayNotHasKey('error', $appInfo); + self::assertSame( + ['KEPT' => ['name' => 'KEPT', 'displayName' => 'Kept', 'description' => '', 'default' => 'v', 'value' => 'v']], + $appInfo['external-app']['environment-variables'] + ); + } + + public function testDeployOptionsOverrideDeclaredVariables(): void { + $infoXml = $this->writeInfoXml(<< + A + x + + + B + y + +XML); + + $appInfo = $this->service->getAppInfo('test_app', $infoXml, null, [ + 'environment_variables' => ['A' => 'overridden', 'B' => ''], + ]); + + self::assertArrayNotHasKey('error', $appInfo); + self::assertSame( + ['A' => ['name' => 'A', 'displayName' => '', 'description' => '', 'default' => 'x', 'value' => 'overridden']], + $appInfo['external-app']['environment-variables'] + ); + } + + public function testVariableWithEmptyNameElementReturnsError(): void { + $infoXml = $this->writeInfoXml(<< + + v + +XML); + + $appInfo = $this->service->getAppInfo('test_app', $infoXml, null); + + self::assertArrayHasKey('error', $appInfo); + self::assertStringContainsString('invalid environment variable definition', $appInfo['error']); + } +} From e0c015acf9a18d0642f6de33465579ca374656d0 Mon Sep 17 00:00:00 2001 From: Oleksander Piskun Date: Tue, 11 Aug 2026 10:15:45 +0000 Subject: [PATCH 2/2] fix: normalize ExApp environment variables for json-info registrations too Signed-off-by: Oleksander Piskun --- lib/Service/ExAppService.php | 31 ++++---- .../Service/ExAppServiceGetAppInfoTest.php | 75 ++++++++++++++++++- 2 files changed, 87 insertions(+), 19 deletions(-) diff --git a/lib/Service/ExAppService.php b/lib/Service/ExAppService.php index f2dd0aaa9..d39fab8c4 100644 --- a/lib/Service/ExAppService.php +++ b/lib/Service/ExAppService.php @@ -261,7 +261,7 @@ public function getAppInfo(string $appId, ?string $infoXml, ?string $jsonInfo, ? # fill 'id' if it is missing(this field was called `appid` in previous versions in json) $appInfo['id'] = $appInfo['id'] ?? $appId; # during manual install JSON can have all values at root level - foreach (['docker-install', 'translations_folder', 'routes', 'k8s-service-roles'] as $key) { + foreach (['docker-install', 'translations_folder', 'routes', 'k8s-service-roles', 'environment-variables'] as $key) { if (isset($appInfo[$key])) { $appInfo['external-app'][$key] = $appInfo[$key]; unset($appInfo[$key]); @@ -291,20 +291,6 @@ public function getAppInfo(string $appId, ?string $infoXml, ?string $jsonInfo, ? $appInfo['external-app']['routes'] = [$appInfo['external-app']['routes']['route']]; } } - // Advanced deploy options - if (isset($appInfo['external-app']['environment-variables']['variable'])) { - $variables = $appInfo['external-app']['environment-variables']['variable']; - if (!is_array($variables)) { - return ['error' => sprintf("ExApp '%s' has invalid environment variable definition. 'variable' must be an object or a list of objects, got %s", $appId, get_debug_type($variables))]; - } - try { - $appInfo['external-app']['environment-variables'] = ExAppEnvVarsHelper::normalizeAndValidate( - $variables, $deployOptions['environment_variables'] ?? [] - ); - } catch (InvalidArgumentException $e) { - return ['error' => sprintf("ExApp '%s' has invalid environment variable definition. %s", $appId, $e->getMessage())]; - } - } if (isset($appInfo['external-app']['k8s-service-roles']['role'])) { $roles = $appInfo['external-app']['k8s-service-roles']['role']; if (!isset($roles[0])) { @@ -330,6 +316,21 @@ public function getAppInfo(string $appId, ?string $infoXml, ?string $jsonInfo, ? } } } + // Advanced deploy options; runs for both the XML and the JSON path so the + // environment-variables contract of the returned appInfo is input-format independent + if (isset($appInfo['external-app']['environment-variables']['variable'])) { + $variables = $appInfo['external-app']['environment-variables']['variable']; + if (!is_array($variables)) { + return ['error' => sprintf("ExApp '%s' has invalid environment variable definition. 'variable' must be an object or a list of objects, got %s", $appId, get_debug_type($variables))]; + } + try { + $appInfo['external-app']['environment-variables'] = ExAppEnvVarsHelper::normalizeAndValidate( + $variables, $deployOptions['environment_variables'] ?? [] + ); + } catch (InvalidArgumentException $e) { + return ['error' => sprintf("ExApp '%s' has invalid environment variable definition. %s", $appId, $e->getMessage())]; + } + } if (isset($appInfo['external-app']['routes'])) { if (!is_array($appInfo['external-app']['routes'])) { return ['error' => sprintf("ExApp '%s' has invalid route definition. 'routes' must be a list of route objects, got %s", $appId, get_debug_type($appInfo['external-app']['routes']))]; diff --git a/tests/php/Service/ExAppServiceGetAppInfoTest.php b/tests/php/Service/ExAppServiceGetAppInfoTest.php index 71cde71a7..8e052df8c 100644 --- a/tests/php/Service/ExAppServiceGetAppInfoTest.php +++ b/tests/php/Service/ExAppServiceGetAppInfoTest.php @@ -31,9 +31,10 @@ use Psr\Log\LoggerInterface; /** - * getAppInfo-level coverage for the environment-variable normalization wiring: the info.xml - * path must feed declared variables through ExAppEnvVarsHelper, so an empty - * (parsed to [] by the simplexml/json roundtrip) never reaches the deploy actions. + * getAppInfo-level coverage for the environment-variable normalization wiring: both the + * info.xml and the --json-info path must feed declared variables through ExAppEnvVarsHelper, + * so an empty (parsed to [] by the simplexml/json roundtrip) or an + * unnormalized JSON definition never reaches the deploy actions. */ class ExAppServiceGetAppInfoTest extends TestCase { private ExAppService $service; @@ -75,7 +76,7 @@ protected function tearDown(): void { } private function writeInfoXml(string $environmentVariables): string { - $this->infoXmlPath = tempnam(sys_get_temp_dir(), 'appapi-test-info-') . '.xml'; + $this->infoXmlPath = tempnam(sys_get_temp_dir(), 'appapi-test-info-'); file_put_contents($this->infoXmlPath, << @@ -154,4 +155,70 @@ public function testVariableWithEmptyNameElementReturnsError(): void { self::assertArrayHasKey('error', $appInfo); self::assertStringContainsString('invalid environment variable definition', $appInfo['error']); } + + public function testJsonInfoEnvironmentVariablesAreNormalized(): void { + $jsonInfo = json_encode([ + 'id' => 'test_app', + 'external-app' => [ + 'environment-variables' => [ + 'variable' => [ + ['name' => 'EMPTY', 'display-name' => 'Empty', 'default' => ''], + ['name' => 'KEPT', 'display-name' => 'Kept', 'default' => 'v'], + ['name' => 'TYPED', 'default' => 5], + ], + ], + ], + ]); + + $appInfo = $this->service->getAppInfo('test_app', null, $jsonInfo, [ + 'environment_variables' => ['KEPT' => 'overridden'], + ]); + + self::assertArrayNotHasKey('error', $appInfo); + self::assertSame( + [ + 'KEPT' => ['name' => 'KEPT', 'displayName' => 'Kept', 'description' => '', 'default' => 'v', 'value' => 'overridden'], + 'TYPED' => ['name' => 'TYPED', 'displayName' => '', 'description' => '', 'default' => '5', 'value' => '5'], + ], + $appInfo['external-app']['environment-variables'] + ); + } + + public function testJsonInfoRootLevelEnvironmentVariablesAreNormalized(): void { + $jsonInfo = json_encode([ + 'id' => 'test_app', + 'environment-variables' => [ + 'variable' => [ + ['name' => 'EMPTY', 'default' => ''], + ['name' => 'KEPT', 'default' => 'v'], + ], + ], + ]); + + $appInfo = $this->service->getAppInfo('test_app', null, $jsonInfo); + + self::assertArrayNotHasKey('error', $appInfo); + self::assertSame( + ['KEPT' => ['name' => 'KEPT', 'displayName' => '', 'description' => '', 'default' => 'v', 'value' => 'v']], + $appInfo['external-app']['environment-variables'] + ); + } + + public function testJsonInfoVariableWithEmptyNameReturnsError(): void { + $jsonInfo = json_encode([ + 'id' => 'test_app', + 'external-app' => [ + 'environment-variables' => [ + 'variable' => [ + ['name' => '', 'default' => 'v'], + ], + ], + ], + ]); + + $appInfo = $this->service->getAppInfo('test_app', null, $jsonInfo); + + self::assertArrayHasKey('error', $appInfo); + self::assertStringContainsString('invalid environment variable definition', $appInfo['error']); + } }