From b9e22b7e126977d9ff414fc9bb220e12a431884d Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Sun, 9 Aug 2026 22:39:19 +0200 Subject: [PATCH] fix(archimate): unset AMEF ids were handed on as empty strings past a `=== null` guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit gate-50 security-config-fail-mode 14 -> PASS, measured with hydra-gates 651e5c5 at the CI scope (--scope-to-diff --base origin/beta). I nearly dismissed these 14 as false positives of the gate's 10-line window, on the grounds that the consumers validate. Checking every consumer rather than the two convenient ones showed the opposite. The legacy fallback in both getAmefConfig() implementations read every register/schema id with '' as its default and returned them. The consumers guard with `=== null`: ViewService:254, :320 if ($registerId === null || $viewSchemaId === null) throw ViewService:711 $registerId used with NO guard at all and `'' === null` is false. An empty id therefore passes the guard and is pinned into an OpenRegister query as the register/schema — and an unpinned query returns rows, which reads exactly like a correct result. Nothing reaches a query TODAY only because the fallback writes PLURAL key names (`views_schema`, `elements_schema`) while every consumer reads SINGULAR ones (`view_schema`, `element_schema`), so the lookups miss and fall back to null. Measured producer/consumer key overlap: the empty set. That is an accident of naming, not a defence — adding the singular keys, the obvious "cleanup", turns it into a live fail-open. - resolveConfiguredId() reads each id and returns null, with a warning naming the key, when it is empty or whitespace. The guard is now AT the read, and there is one read instead of eight. - The fallback array_filters the nulls out, so `?? null` downstream yields null — which is what every consumer already checks for. - ViewService::getModulesData() gains the missing register guard and fails closed rather than issuing an unpinned query; its schema loop now uses empty() rather than `=== null` for the same reason. Narrower than it first looks, and the tests say so: the fallback is only reached when `amef_config` is MALFORMED, because its default '{}' is valid JSON and decodes to []. My first draft of the tests failed for exactly that reason and taught me the branch condition. Can-fail: reverting the three services turns 3 of the 4 new tests red and puts gate-50 back to 14. phpcs lib/ 0 errors, phpmd/psalm/phpstan clean, unit suite 523 tests green. --- lib/Service/ArchiMateImportService.php | 106 +++++++----- lib/Service/ArchiMateService.php | 102 ++++++----- lib/Service/ViewService.php | 18 +- .../Service/ArchiMateConfigFailModeTest.php | 161 ++++++++++++++++++ 4 files changed, 304 insertions(+), 83 deletions(-) create mode 100644 tests/Unit/Service/ArchiMateConfigFailModeTest.php diff --git a/lib/Service/ArchiMateImportService.php b/lib/Service/ArchiMateImportService.php index 934fa958..af3dd45d 100644 --- a/lib/Service/ArchiMateImportService.php +++ b/lib/Service/ArchiMateImportService.php @@ -1955,6 +1955,48 @@ private function getCurrentOrganisation(): string } }//end getCurrentOrganisation() + /** + * Read a configured id, failing closed on the empty default. + * + * The legacy fallback used to read every id with `''` as its default, so + * an unconfigured instance produced a config array full of empty STRINGS. + * Its consumers guard with `=== null` — `getAmefRegisterId()` and + * `getAmefSchemaIdForType()` reject them with `is_numeric()` + `> 0` — and `'' === null` is + * false. Today nothing reaches a query only because that fallback writes + * PLURAL key names (`views_schema`) while every consumer reads SINGULAR + * ones (`view_schema`), so the lookups miss and fall back to `null`. That + * is an accident of naming, not a defence: adding the singular keys — the + * obvious "cleanup" — would send `register => ''` straight into + * `searchObjects()` as an UNPINNED query, and an unpinned query returns + * rows, which reads exactly like a correct result. + * + * Returning null instead of `''` makes `?? null` downstream yield null, + * which is what every consumer already checks for, and the warning names + * the missing key so a misconfigured import stops reporting "0 objects" + * with no explanation. + * + * @param string $key The app-config key holding the id. + * + * @return string|null The configured id, or null when it is unset. + * + * @spec openspec/specs/archimate-import/spec.md + */ + private function resolveConfiguredId(string $key): ?string + { + $value = $this->config->getValueString('softwarecatalog', $key, ''); + if (trim($value) === '') { + $this->logger->warning( + 'ArchiMate configuration is incomplete — this id is not configured, so it is omitted rather than passed on as an empty string', + ['key' => $key] + ); + + return null; + } + + return $value; + + }//end resolveConfiguredId() + /** * Get AMEF configuration from app config * @@ -1971,49 +2013,27 @@ public function getAmefConfig(): array $decoded = json_decode($config, true); if (is_array($decoded) === false) { - // Fallback to individual config values for backward compatibility. - $decoded = [ - 'register_id' => $this->config->getValueString( - 'softwarecatalog', - 'amef_register', - '' - ), - 'model_schema_id' => $this->config->getValueString( - 'softwarecatalog', - 'amef_model_schema', - '' - ), - 'elements_schema' => $this->config->getValueString( - 'softwarecatalog', - 'amef_elements_schema', - '' - ), - 'relationships_schema' => $this->config->getValueString( - 'softwarecatalog', - 'amef_relationships_schema', - '' - ), - 'views_schema' => $this->config->getValueString( - 'softwarecatalog', - 'amef_views_schema', - '' - ), - 'organizations_schema' => $this->config->getValueString( - 'softwarecatalog', - 'amef_organizations_schema', - '' - ), - 'folders_schema' => $this->config->getValueString( - 'softwarecatalog', - 'amef_folders_schema', - '' - ), - 'property_definitions_schema' => $this->config->getValueString( - 'softwarecatalog', - 'amef_property_definitions_schema', - '' - ), - ]; + // Fallback to individual config values for backward + // compatibility. Every id is read through + // resolveConfiguredId(), which guards the empty default at the + // point of the read and omits the key entirely when it is + // unset — so `?? null` downstream yields null, which is what + // the consumers already check for. + $decoded = array_filter( + [ + 'register_id' => $this->resolveConfiguredId(key: 'amef_register'), + 'model_schema_id' => $this->resolveConfiguredId(key: 'amef_model_schema'), + 'elements_schema' => $this->resolveConfiguredId(key: 'amef_elements_schema'), + 'relationships_schema' => $this->resolveConfiguredId(key: 'amef_relationships_schema'), + 'views_schema' => $this->resolveConfiguredId(key: 'amef_views_schema'), + 'organizations_schema' => $this->resolveConfiguredId(key: 'amef_organizations_schema'), + 'folders_schema' => $this->resolveConfiguredId(key: 'amef_folders_schema'), + 'property_definitions_schema' => $this->resolveConfiguredId(key: 'amef_property_definitions_schema'), + ], + static function ($value) { + return $value !== null; + } + ); }//end if return $decoded; diff --git a/lib/Service/ArchiMateService.php b/lib/Service/ArchiMateService.php index f4c1590c..1a64673f 100644 --- a/lib/Service/ArchiMateService.php +++ b/lib/Service/ArchiMateService.php @@ -1621,6 +1621,48 @@ private function createTempFile(string $content): string return $tempFile; }//end createTempFile() + /** + * Read a configured id, failing closed on the empty default. + * + * The legacy fallback used to read every id with `''` as its default, so + * an unconfigured instance produced a config array full of empty STRINGS. + * Its consumers guard with `=== null` — `ViewService::getViews()` and + * `getView()` throw only when a value `=== null` — and `'' === null` is + * false. Today nothing reaches a query only because that fallback writes + * PLURAL key names (`views_schema`) while every consumer reads SINGULAR + * ones (`view_schema`), so the lookups miss and fall back to `null`. That + * is an accident of naming, not a defence: adding the singular keys — the + * obvious "cleanup" — would send `register => ''` straight into + * `searchObjects()` as an UNPINNED query, and an unpinned query returns + * rows, which reads exactly like a correct result. + * + * Returning null instead of `''` makes `?? null` downstream yield null, + * which is what every consumer already checks for, and the warning names + * the missing key so a misconfigured import stops reporting "0 objects" + * with no explanation. + * + * @param string $key The app-config key holding the id. + * + * @return string|null The configured id, or null when it is unset. + * + * @spec openspec/specs/archimate-import/spec.md + */ + private function resolveConfiguredId(string $key): ?string + { + $value = $this->config->getValueString('softwarecatalog', $key, ''); + if (trim($value) === '') { + $this->logger->warning( + 'ArchiMate configuration is incomplete — this id is not configured, so it is omitted rather than passed on as an empty string', + ['key' => $key] + ); + + return null; + } + + return $value; + + }//end resolveConfiguredId() + /** * Get AMEF configuration from app config * @@ -1637,45 +1679,27 @@ public function getAmefConfig(): array $decoded = json_decode($config, true); if (is_array($decoded) === false) { - // Fallback to individual config values for backward compatibility. - $decoded = [ - 'register_id' => $this->config->getValueString('softwarecatalog', 'amef_register', ''), - 'model_schema_id' => $this->config->getValueString( - 'softwarecatalog', - 'amef_model_schema', - '' - ), - 'elements_schema' => $this->config->getValueString( - 'softwarecatalog', - 'amef_elements_schema', - '' - ), - 'relationships_schema' => $this->config->getValueString( - 'softwarecatalog', - 'amef_relationships_schema', - '' - ), - 'views_schema' => $this->config->getValueString( - 'softwarecatalog', - 'amef_views_schema', - '' - ), - 'organizations_schema' => $this->config->getValueString( - 'softwarecatalog', - 'amef_organizations_schema', - '' - ), - 'folders_schema' => $this->config->getValueString( - 'softwarecatalog', - 'amef_folders_schema', - '' - ), - 'property_definitions_schema' => $this->config->getValueString( - 'softwarecatalog', - 'amef_property_definitions_schema', - '' - ), - ]; + // Fallback to individual config values for backward + // compatibility. Every id is read through + // resolveConfiguredId(), which guards the empty default at the + // point of the read and omits the key entirely when it is + // unset — so `?? null` downstream yields null, which is what + // the consumers already check for. + $decoded = array_filter( + [ + 'register_id' => $this->resolveConfiguredId(key: 'amef_register'), + 'model_schema_id' => $this->resolveConfiguredId(key: 'amef_model_schema'), + 'elements_schema' => $this->resolveConfiguredId(key: 'amef_elements_schema'), + 'relationships_schema' => $this->resolveConfiguredId(key: 'amef_relationships_schema'), + 'views_schema' => $this->resolveConfiguredId(key: 'amef_views_schema'), + 'organizations_schema' => $this->resolveConfiguredId(key: 'amef_organizations_schema'), + 'folders_schema' => $this->resolveConfiguredId(key: 'amef_folders_schema'), + 'property_definitions_schema' => $this->resolveConfiguredId(key: 'amef_property_definitions_schema'), + ], + static function ($value) { + return $value !== null; + } + ); }//end if return $decoded; diff --git a/lib/Service/ViewService.php b/lib/Service/ViewService.php index d68bb8ce..e5a2a04c 100644 --- a/lib/Service/ViewService.php +++ b/lib/Service/ViewService.php @@ -711,6 +711,22 @@ private function getModulesData(): array $amefConfig = $this->settingsService->getAmefConfig(); $registerId = $amefConfig['register_id'] ?? null; + // Fail closed on an unconfigured register. This used to be the one + // read of `register_id` with no guard at all: the loop below only + // checks the SCHEMA, so an empty register would have been pinned + // into `@self` and OpenRegister asked for "any register" — an + // unpinned query returns rows, which reads exactly like a correct + // result. `empty()` rather than `=== null` because the legacy + // config fallback resolves unset ids to `''`, and `'' === null` is + // false. + if (empty($registerId) === true) { + $this->logger->warning( + 'ViewService: AMEF register is not configured; skipping the module lookup rather than issuing an unpinned query' + ); + + return []; + } + // Modules could be in various schemas - check common ones. $moduleSchemas = [ $amefConfig['module_schema'] ?? null, @@ -721,7 +737,7 @@ private function getModulesData(): array $allModules = []; foreach ($moduleSchemas as $schemaId) { - if ($schemaId === null) { + if (empty($schemaId) === true) { continue; } diff --git a/tests/Unit/Service/ArchiMateConfigFailModeTest.php b/tests/Unit/Service/ArchiMateConfigFailModeTest.php new file mode 100644 index 00000000..d532c5fa --- /dev/null +++ b/tests/Unit/Service/ArchiMateConfigFailModeTest.php @@ -0,0 +1,161 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * @version GIT: + * @link https://codeberg.org/Conduction/SoftwareCatalog + */ + +declare(strict_types=1); + +namespace OCA\SoftwareCatalog\Tests\Unit\Service; + +use OCA\SoftwareCatalog\Service\ArchiMateImportService; +use PHPUnit\Framework\TestCase; + +/** + * The AMEF config resolvers must omit unresolved ids, never emit ''. + * + * @category Test + * @package OCA\SoftwareCatalog\Tests\Unit\Service + * @author Conduction b.v. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * @version GIT: + * @link https://codeberg.org/Conduction/SoftwareCatalog + */ +class ArchiMateConfigFailModeTest extends TestCase +{ + + + /** + * Build the service with only the two collaborators the resolver touches. + * + * @param array $values Config key => stored value. + * + * @return ArchiMateImportService + */ + private function serviceWithConfig(array $values): ArchiMateImportService + { + // The legacy fallback is only reached when `amef_config` does not + // decode to an array. Its DEFAULT is '{}', which decodes to [] — so on + // an instance that never wrote the key, getAmefConfig() returns [] and + // the fallback never runs at all. Malformed JSON is the branch under + // test here, and callers must not be given empty ids on that path. + $values['amef_config'] = $values['amef_config'] ?? 'not-json'; + + $config = $this->createMock(\OCP\IAppConfig::class); + $config->method('getValueString')->willReturnCallback( + function (string $app, string $key, string $default = '') use ($values) { + return $values[$key] ?? $default; + } + ); + + $service = (new \ReflectionClass(ArchiMateImportService::class))->newInstanceWithoutConstructor(); + + foreach (['config' => $config, 'logger' => $this->createMock(\Psr\Log\LoggerInterface::class)] as $prop => $value) { + $property = new \ReflectionProperty(ArchiMateImportService::class, $prop); + $property->setValue($service, $value); + } + + return $service; + + }//end serviceWithConfig() + + + /** + * An unconfigured instance yields no id keys at all — not empty strings. + * + * @return void + */ + public function testUnconfiguredInstanceOmitsEveryIdRatherThanEmittingEmptyStrings(): void + { + $config = $this->serviceWithConfig([])->getAmefConfig(); + + // Guard against a vacuous pass: if the fallback had not run at all, + // the loop below would iterate nothing and prove nothing. + $this->assertSame([], $config, 'every id was unset, so none should survive'); + + foreach ($config as $key => $value) { + $this->assertNotSame('', $value, sprintf('"%s" was handed on as an empty string', $key)); + } + + // The property the consumers rely on: `?? null` must yield null. + $this->assertNull($config['register_id'] ?? null); + $this->assertNull($config['views_schema'] ?? null); + + }//end testUnconfiguredInstanceOmitsEveryIdRatherThanEmittingEmptyStrings() + + + /** + * A configured id still comes through untouched. + * + * @return void + */ + public function testConfiguredIdsAreReturnedUnchanged(): void + { + $config = $this->serviceWithConfig( + [ + 'amef_register' => '11', + 'amef_views_schema' => '42', + ] + )->getAmefConfig(); + + $this->assertSame('11', $config['register_id']); + $this->assertSame('42', $config['views_schema']); + + }//end testConfiguredIdsAreReturnedUnchanged() + + + /** + * A partially configured instance keeps what is set and drops what is not. + * + * This is the shape that makes the difference real: with `''` retained, a + * `=== null` guard on the missing half would pass. + * + * @return void + */ + public function testPartialConfigurationKeepsSetIdsAndDropsUnsetOnes(): void + { + $config = $this->serviceWithConfig(['amef_register' => '11'])->getAmefConfig(); + + $this->assertSame('11', $config['register_id']); + $this->assertArrayNotHasKey('views_schema', $config); + $this->assertArrayNotHasKey('elements_schema', $config); + + }//end testPartialConfigurationKeepsSetIdsAndDropsUnsetOnes() + + + /** + * A whitespace-only id counts as unset, not as a usable value. + * + * @return void + */ + public function testWhitespaceOnlyIdIsTreatedAsUnset(): void + { + $config = $this->serviceWithConfig(['amef_register' => ' '])->getAmefConfig(); + + $this->assertArrayNotHasKey('register_id', $config); + + }//end testWhitespaceOnlyIdIsTreatedAsUnset() + + +}//end class