Skip to content

Commit 2085e2b

Browse files
JohnVillalovosJohnVillalovos
committed
fix(config): reject case-insensitive key collisions
Reject config schema definitions that collapse to the same lookup key once matching is treated case-insensitively. Add collision checks to AbstractConfigKeys so both canonical and legacy keys are validated before lookup, and fail fast with a clear exception when two different entries normalize to the same key. Add a focused plugin-schema test and fixture covering the collision case.
1 parent 27e7e31 commit 2085e2b

File tree

3 files changed

+264
-5
lines changed

3 files changed

+264
-5
lines changed

‎lib/Config/AbstractConfigKeys.php‎

Lines changed: 96 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@ abstract class AbstractConfigKeys
44
{
55
/** @var array<class-string, array> */
66
private static array $allCache = [];
7+
/** @var array<class-string, array> */
8+
private static array $allWithEntryIdsCache = [];
79

810
/**
911
* Returns all configuration entries defined in the class.
@@ -16,18 +18,45 @@ public static function all(): array
1618
return self::$allCache[$class];
1719
}
1820

21+
$entries = [];
22+
foreach (self::allWithEntryIds() as $value) {
23+
unset($value['_entry_id']);
24+
$entries[] = $value;
25+
}
26+
27+
self::$allCache[$class] = $entries;
28+
29+
return $entries;
30+
}
31+
32+
/**
33+
* @return array<int, array<string, mixed>>
34+
*/
35+
private static function allWithEntryIds(): array
36+
{
37+
$class = static::class;
38+
if (isset(self::$allWithEntryIdsCache[$class])) {
39+
return self::$allWithEntryIdsCache[$class];
40+
}
41+
1942
$constants = (new \ReflectionClass($class))->getConstants();
2043

21-
$all = [];
22-
foreach ($constants as $value) {
44+
$configsWithIds = [];
45+
foreach ($constants as $name => $value) {
2346
if (is_array($value) && isset($value['key'])) {
24-
$all[] = $value;
47+
// Preserve the constant name so collision detection can distinguish
48+
// between different schema entries that may share the same key text,
49+
// e.g. SERVER1_KEY and SERVER1_KEY_DUPLICATE_SECTION_CASE both using
50+
// 'key' => 'key'.
51+
$value['_entry_id'] = $name;
52+
$configsWithIds[] = $value;
2553
}
2654
}
2755

28-
self::$allCache[$class] = $all;
56+
self::assertNoCaseInsensitiveLookupCollisions(configsWithIds: $configsWithIds);
57+
self::$allWithEntryIdsCache[$class] = $configsWithIds;
2958

30-
return $all;
59+
return $configsWithIds;
3160
}
3261

3362
/**
@@ -107,4 +136,66 @@ protected static function getCanonicalLookupKey(array $config): ?string
107136

108137
return is_string($key) && $key !== '' ? $key : null;
109138
}
139+
140+
private static function assertNoCaseInsensitiveLookupCollisions(array $configsWithIds): void
141+
{
142+
$seen = [];
143+
144+
foreach ($configsWithIds as $configWithId) {
145+
// Validate both the canonical lookup key and any legacy alias because
146+
// either one can collide once lookups become case-insensitive.
147+
self::assertUniqueCaseInsensitiveKey(
148+
seen: $seen,
149+
rawKey: static::getCanonicalLookupKey(config: $configWithId),
150+
configWithId: $configWithId,
151+
source: 'key'
152+
);
153+
self::assertUniqueCaseInsensitiveKey(
154+
seen: $seen,
155+
rawKey: $configWithId['legacy'] ?? null,
156+
configWithId: $configWithId,
157+
source: 'legacy'
158+
);
159+
}
160+
}
161+
162+
private static function assertUniqueCaseInsensitiveKey(array &$seen, ?string $rawKey, array $configWithId, string $source): void
163+
{
164+
if (!is_string($rawKey) || $rawKey === '') {
165+
return;
166+
}
167+
168+
$normalizedKey = strtolower($rawKey);
169+
if (!isset($seen[$normalizedKey])) {
170+
// Remember the first schema entry that claims this normalized lookup key.
171+
$seen[$normalizedKey] = [
172+
'rawKey' => $rawKey,
173+
'entryId' => $configWithId['_entry_id'],
174+
'key' => $configWithId['key'],
175+
'source' => $source,
176+
];
177+
return;
178+
}
179+
180+
$existing = $seen[$normalizedKey];
181+
// Allow the same schema entry to contribute both a canonical key and a
182+
// legacy alias that normalize to the same lookup string.
183+
if ($existing['entryId'] === $configWithId['_entry_id']) {
184+
return;
185+
}
186+
187+
throw new LogicException(sprintf(
188+
'Case-insensitive config key collision detected in %s for "%s" between %s "%s" (entry "%s", constant "%s") and %s "%s" (entry "%s", constant "%s")',
189+
static::class,
190+
$normalizedKey,
191+
$existing['source'],
192+
$existing['rawKey'],
193+
$existing['key'],
194+
$existing['entryId'],
195+
$source,
196+
$rawKey,
197+
$configWithId['key'],
198+
$configWithId['_entry_id']
199+
));
200+
}
110201
}

‎tests/Infrastructure/Config/ConfigTest.php‎

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
require_once(ROOT_DIR . 'lib/Config/namespace.php');
66
require_once(ROOT_DIR . 'tests/data/test_plugin_configclass.php');
7+
require_once(ROOT_DIR . 'tests/data/test_plugin_configclass_collision.php');
78

89
class ConfigTest extends TestBase
910
{
@@ -138,6 +139,70 @@ public function testMixedCasePluginConfigResolveCorrectly()
138139
$this->assertEquals('value3', $pluginConfig->GetKey(TestPluginConfigKeys::SERVER2_KEY));
139140
}
140141

142+
public function testPluginConfigRejectsCaseInsensitiveKeyCollisions()
143+
{
144+
$this->expectException(LogicException::class);
145+
$this->expectExceptionMessage(
146+
sprintf(
147+
'Case-insensitive config key collision detected in %s',
148+
TestPluginConfigKeysCollision::class
149+
)
150+
);
151+
152+
TestPluginConfigKeysCollision::findByKey('server1.key');
153+
}
154+
155+
public function testPluginConfigRejectsCaseInsensitiveKeyCollisionsWhenEntriesShareTheSameKeyName()
156+
{
157+
$this->expectException(LogicException::class);
158+
$this->expectExceptionMessage(
159+
sprintf(
160+
'Case-insensitive config key collision detected in %s',
161+
TestPluginConfigKeysCollisionSameKey::class
162+
)
163+
);
164+
165+
TestPluginConfigKeysCollisionSameKey::findByKey('server1.key');
166+
}
167+
168+
public function testPluginConfigAllowsTheSameKeyNameInDifferentSections()
169+
{
170+
$this->assertSame(
171+
TestPluginConfigKeysNoCollisionSameKeyDifferentSections::SERVER1_KEY,
172+
TestPluginConfigKeysNoCollisionSameKeyDifferentSections::findByKey('server1.key')
173+
);
174+
$this->assertSame(
175+
TestPluginConfigKeysNoCollisionSameKeyDifferentSections::SERVER2_KEY,
176+
TestPluginConfigKeysNoCollisionSameKeyDifferentSections::findByKey('server2.key')
177+
);
178+
}
179+
180+
public function testPluginConfigRejectsCaseInsensitiveLegacyToCanonicalCollisions()
181+
{
182+
$this->expectException(LogicException::class);
183+
$this->expectExceptionMessage(
184+
sprintf(
185+
'Case-insensitive config key collision detected in %s',
186+
TestPluginConfigKeysCollisionLegacyVsCanonical::class
187+
)
188+
);
189+
190+
TestPluginConfigKeysCollisionLegacyVsCanonical::findByKey('server1.key');
191+
}
192+
193+
public function testPluginConfigRejectsCaseInsensitiveLegacyToLegacyCollisions()
194+
{
195+
$this->expectException(LogicException::class);
196+
$this->expectExceptionMessage(
197+
sprintf(
198+
'Case-insensitive config key collision detected in %s',
199+
TestPluginConfigKeysCollisionLegacyVsLegacy::class
200+
)
201+
);
202+
203+
TestPluginConfigKeysCollisionLegacyVsLegacy::findByKey('server1.key1');
204+
}
205+
141206
public function testPluginConfigValidation()
142207
{
143208
$errorLogs = $this->captureErrorLog(function () {

‎tests/data/test_plugin_configclass_collision.php‎

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
require_once(ROOT_DIR . 'lib/Config/PluginConfigKeys.php');
6+
7+
class TestPluginConfigKeysCollision extends PluginConfigKeys
8+
{
9+
public const CONFIG_ID = 'test_plugin_collision';
10+
11+
public const SERVER1_KEY = [
12+
'key' => 'Key',
13+
'type' => 'string',
14+
'default' => 'default1',
15+
'section' => 'server1'
16+
];
17+
18+
public const SERVER1_KEY_DUPLICATE_CASE = [
19+
'key' => 'key',
20+
'type' => 'string',
21+
'default' => 'default2',
22+
'section' => 'SERVER1'
23+
];
24+
}
25+
26+
class TestPluginConfigKeysCollisionSameKey extends PluginConfigKeys
27+
{
28+
public const CONFIG_ID = 'test_plugin_collision_same_key';
29+
30+
public const SERVER1_KEY = [
31+
'key' => 'key',
32+
'type' => 'string',
33+
'default' => 'default1',
34+
'section' => 'server1'
35+
];
36+
37+
public const SERVER1_KEY_DUPLICATE_SECTION_CASE = [
38+
'key' => 'key',
39+
'type' => 'string',
40+
'default' => 'default2',
41+
'section' => 'SERVER1'
42+
];
43+
}
44+
45+
class TestPluginConfigKeysNoCollisionSameKeyDifferentSections extends PluginConfigKeys
46+
{
47+
public const CONFIG_ID = 'test_plugin_no_collision_same_key';
48+
49+
public const SERVER1_KEY = [
50+
'key' => 'key',
51+
'type' => 'string',
52+
'default' => 'default1',
53+
'section' => 'server1'
54+
];
55+
56+
public const SERVER2_KEY = [
57+
'key' => 'key',
58+
'type' => 'string',
59+
'default' => 'default2',
60+
'section' => 'server2'
61+
];
62+
}
63+
64+
class TestPluginConfigKeysCollisionLegacyVsCanonical extends PluginConfigKeys
65+
{
66+
public const CONFIG_ID = 'test_plugin_collision_legacy_vs_canonical';
67+
68+
public const SERVER1_KEY = [
69+
'key' => 'key',
70+
'type' => 'string',
71+
'default' => 'default1',
72+
'section' => 'server1',
73+
'legacy' => 'server1.legacy-key'
74+
];
75+
76+
public const SERVER1_LEGACY_KEY = [
77+
'key' => 'legacy-key',
78+
'type' => 'string',
79+
'default' => 'default2',
80+
'section' => 'SERVER1'
81+
];
82+
}
83+
84+
class TestPluginConfigKeysCollisionLegacyVsLegacy extends PluginConfigKeys
85+
{
86+
public const CONFIG_ID = 'test_plugin_collision_legacy_vs_legacy';
87+
88+
public const SERVER1_KEY = [
89+
'key' => 'key1',
90+
'type' => 'string',
91+
'default' => 'default1',
92+
'section' => 'server1',
93+
'legacy' => 'server1.shared-legacy'
94+
];
95+
96+
public const SERVER1_KEY_2 = [
97+
'key' => 'key2',
98+
'type' => 'string',
99+
'default' => 'default2',
100+
'section' => 'server1',
101+
'legacy' => 'SERVER1.shared-legacy'
102+
];
103+
}

0 commit comments

Comments
 (0)