Skip to content

Commit 34106c1

Browse files
committed
fix: add circular import detection and duplicate key validation
Detect circular YAML imports via a visited-path set to prevent stack overflow. Validate yamlKeyMap values are unique in ModuleConfigDescriptor to catch silent data loss from array_flip. Fix CLAUDE.md PHPStan level to match phpstan.neon (10, not 8).
1 parent 9307e78 commit 34106c1

5 files changed

Lines changed: 66 additions & 6 deletions

File tree

CLAUDE.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ Shared configuration accessor library for OpenCoreEMR modules (`oce-module-*`).
99
| Composer name | `opencoreemr/oce-lib-module-config` |
1010
| PHP | >= 8.2, `declare(strict_types=1)` everywhere |
1111
| Style | PSR-12 |
12-
| PHPStan | Level 8 |
12+
| PHPStan | Level 10 |
1313

1414
## Architecture
1515

src/ModuleConfigDescriptor.php

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,12 @@ public function __construct(
4343
public string $configFileEnvVar,
4444
public string $secretsFileEnvVar,
4545
) {
46-
$this->reverseKeyMap = array_flip($yamlKeyMap);
46+
$reversed = array_flip($yamlKeyMap);
47+
if (count($reversed) !== count($yamlKeyMap)) {
48+
throw new \InvalidArgumentException(
49+
'yamlKeyMap values must be unique — multiple YAML keys map to the same internal config key'
50+
);
51+
}
52+
$this->reverseKeyMap = $reversed;
4753
}
4854
}

src/YamlConfigLoader.php

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -51,8 +51,9 @@ public function __construct(private readonly ?SecretProviderInterface $secretPro
5151
public function load(array $filePaths): array
5252
{
5353
$merged = [];
54+
$visited = [];
5455
foreach ($filePaths as $filePath) {
55-
$fileData = $this->loadFile($filePath);
56+
$fileData = $this->loadFile($filePath, $visited);
5657
$merged = array_merge($merged, $fileData);
5758
}
5859

@@ -168,17 +169,30 @@ private function getSecretProvider(string $provider): SecretProviderInterface
168169
/**
169170
* Load a single YAML file, processing any imports
170171
*
172+
* @param array<string, true> $visited Real paths already loaded (cycle detection)
171173
* @return array<string, mixed>
172-
* @throws ConfigurationException if file is not readable or contains invalid YAML
174+
* @throws ConfigurationException if file is not readable, contains invalid YAML, or creates an import cycle
173175
*/
174-
private function loadFile(string $filePath): array
176+
private function loadFile(string $filePath, array &$visited = []): array
175177
{
176178
if (!is_readable($filePath)) {
177179
throw new ConfigurationException(
178180
sprintf('Configuration file is not readable: %s', $filePath)
179181
);
180182
}
181183

184+
$realPath = realpath($filePath);
185+
if ($realPath === false) {
186+
$realPath = $filePath;
187+
}
188+
189+
if (isset($visited[$realPath])) {
190+
throw new ConfigurationException(
191+
sprintf('Circular import detected: %s', $filePath)
192+
);
193+
}
194+
$visited[$realPath] = true;
195+
182196
$contents = file_get_contents($filePath);
183197
if ($contents === false) {
184198
throw new ConfigurationException(
@@ -216,7 +230,7 @@ private function loadFile(string $filePath): array
216230
continue;
217231
}
218232
$importPath = $baseDir . DIRECTORY_SEPARATOR . $resource;
219-
$importedData = array_merge($importedData, $this->loadFile($importPath));
233+
$importedData = array_merge($importedData, $this->loadFile($importPath, $visited));
220234
}
221235
unset($data['imports']);
222236
}

tests/Unit/ModuleConfigDescriptorTest.php

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,22 @@ public function testReverseKeyMapIsDerivedFromYamlKeyMap(): void
2525
$this->assertSame('other', $descriptor->reverseKeyMap['internal_other']);
2626
}
2727

28+
public function testThrowsOnDuplicateYamlKeyMapValues(): void
29+
{
30+
$this->expectException(\InvalidArgumentException::class);
31+
$this->expectExceptionMessage('yamlKeyMap values must be unique');
32+
33+
new ModuleConfigDescriptor(
34+
yamlKeyMap: ['key_a' => 'same_internal', 'key_b' => 'same_internal'],
35+
envOverrideMap: ['same_internal' => 'ENV_VAR'],
36+
envConfigVar: 'ENV_CONFIG',
37+
conventionalConfigPath: '/etc/config.yaml',
38+
conventionalSecretsPath: '/etc/secrets.yaml',
39+
configFileEnvVar: 'CONFIG_FILE',
40+
secretsFileEnvVar: 'SECRETS_FILE',
41+
);
42+
}
43+
2844
public function testAllPropertiesAreAccessible(): void
2945
{
3046
$descriptor = TestDescriptor::create();

tests/Unit/YamlConfigLoaderTest.php

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -188,6 +188,30 @@ public function testLoadThrowsOnMalformedYaml(): void
188188
$loader->load([$path]);
189189
}
190190

191+
public function testLoadThrowsOnCircularImport(): void
192+
{
193+
$loader = new YamlConfigLoader();
194+
$this->writeYaml('a.yaml', "imports:\n - { resource: b.yaml }\nfrom_a: true\n");
195+
$this->writeYaml('b.yaml', "imports:\n - { resource: a.yaml }\nfrom_b: true\n");
196+
$aPath = $this->tmpDir . '/a.yaml';
197+
198+
$this->expectException(ConfigurationException::class);
199+
$this->expectExceptionMessage('Circular import detected');
200+
201+
$loader->load([$aPath]);
202+
}
203+
204+
public function testLoadThrowsOnSelfImport(): void
205+
{
206+
$loader = new YamlConfigLoader();
207+
$path = $this->writeYaml('self.yaml', "imports:\n - { resource: self.yaml }\nkey: val\n");
208+
209+
$this->expectException(ConfigurationException::class);
210+
$this->expectExceptionMessage('Circular import detected');
211+
212+
$loader->load([$path]);
213+
}
214+
191215
public function testLoadThrowsOnNonMappingYaml(): void
192216
{
193217
$loader = new YamlConfigLoader();

0 commit comments

Comments
 (0)